Refuse a session when the server isn't the compiler the client asked for - #37
Open
dkulp wants to merge 1 commit into
Open
Refuse a session when the server isn't the compiler the client asked for#37dkulp wants to merge 1 commit into
dkulp wants to merge 1 commit into
Conversation
A server runs the compiler NAME a client sends, resolved on the server's own PATH. That is what lets a 64-bit machine serve a 32-bit client — both sides say arm-linux-gnueabihf-g++ and each resolves it locally — but it also means a name can mean something else on the other end. A plain "g++" exists on every machine and targets whatever that machine is. Unchecked, that mismatch has two outcomes, and the quiet one is the dangerous one. Compiling a file that includes a system header fails inside glibc, with an error pointing nowhere near the cause. Compiling a file that includes no system headers SUCCEEDS and returns an object of the server's architecture; nothing notices until the link. Verified on a 32-bit client against a 64-bit server: the build accepted an ELF 64-bit aarch64 object with exit code 0. A missing compiler failed differently but no better: the server reported "exec: executable file not found in $PATH" as a compilation error, so the build stopped even though the client could have compiled the file itself. Both are now refused up front. The client sends its compiler's target (cxx -dumpmachine) when starting a session; the server checks that its own CxxName exists and resolves to the same target, and returns FailedPrecondition otherwise, before anything is uploaded. Detection is cached per compiler name on both sides, so it costs one exec each, not one per file. A client that reports no target (an older one, or a compiler without -dumpmachine) is served as before, with existence still checked. A refusal is remembered per server and per compiler name, and the file-to-server choice now walks forward to the next server that can take the file instead of giving up on the first one. So an unusable server costs one local compilation rather than one per file, and only the files that hashed to it move — the rest of the mapping, and every other server's cache, is untouched. That applies to unavailable servers too, which previously sent their whole share to the local compiler. Checked on real hardware, 32-bit client against a 64-bit server: the wrong-target case now compiles locally and produces a correct 32-bit object; the missing compiler case does too; and with two servers where only one has the toolchain, a six-file build put five on the capable server and one local. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR hardens remote C++ compilation by having clients report their compiler target triplet and having servers refuse sessions when their locally-resolved compiler name targets a different architecture (or is missing), preventing silent cross-arch .o generation. It also improves client-side routing so files mapped to an incapable/unavailable server deterministically move to the next usable server instead of forcing widespread local compilation.
Changes:
- Add
CxxTargetTripletto the session-start protobuf and wire it through client → server. - Implement server-side compiler capability detection/caching (
-dumpmachine) and refuse sessions on target mismatch (FailedPrecondition) before uploads. - Update client routing to skip servers that are unavailable or have refused a specific compiler, plus add tests and documentation updates.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pb/nocc-protobuf.proto | Adds CxxTargetTriplet to the session request message. |
| pb/nocc-protobuf.pb.go | Regenerated Go protobuf bindings for the new field. |
| internal/server/nocc-server.go | Refuses session early when compiler capability check fails. |
| internal/server/cxx-capability.go | New server-side cache to detect compiler target and compare with client triplet. |
| internal/server/cxx-capability_test.go | Unit tests for server capability detection, mismatch refusal, and caching. |
| internal/client/remote-connection.go | Tracks per-remote per-compiler incapability and sends CxxTargetTriplet in requests. |
| internal/client/invocation.go | Captures local compiler triplet per invocation via daemon cache. |
| internal/client/cxx-target.go | New client-side triplet detection (cxx -dumpmachine) with daemon-lifetime caching. |
| internal/client/daemon.go | Routes around unavailable/incapable remotes; marks remotes incapable on FailedPrecondition; fixes hash reduction approach (but currently has a compile issue). |
| internal/client/routing_test.go | Tests deterministic rerouting away from incapable/unavailable servers and nil when none usable. |
| docs/architecture.md | Documents compiler-target validation and new “walk forward” routing behavior (needs wording cleanup). |
| cmd/nocc-server/main.go | Initializes the server’s CxxCapability cache. |
Files not reviewed (1)
- pb/nocc-protobuf.pb.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| startIndex := hasher.Sum32() % nRemotes | ||
|
|
||
| for offset := uint32(0); offset < nRemotes; offset++ { | ||
| remote := daemon.remoteConnections[(startIndex+offset)%nRemotes] |
Comment on lines
+60
to
+63
| every machine and targets whatever that machine is. So a client sends the target of its own compiler | ||
| (`cxx -dumpmachine`) when starting a session, and a server refuses the session unless its `CxxName` resolves to | ||
| a compiler with the same target, or isn't installed at all. The client then falls back — to another server, or | ||
| to a local compilation. |
Comment on lines
+81
to
+83
| If the chosen server can't take the file — it's unavailable, or it isn't the compiler that was asked for — the | ||
| daemon walks forward to the next server that can, and only compiles locally when none can. Just the files that | ||
| hashed to the unusable server move, so every other server keeps its share and its caches. |
Comment on lines
+51
to
+53
| // the target triplet of the client's compiler (`cxx -dumpmachine`), empty if it couldn't be detected. | ||
| // the server refuses the session unless its own CxxName targets the same triplet: otherwise it would | ||
| // silently return objects of a foreign architecture, which only surfaces much later, at link time |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A server runs the compiler name a client sends, resolved on the server's own
PATH. That's what lets a 64-bit machine serve a 32-bit client — both sides sayarm-linux-gnueabihf-g++and each resolves it locally — but it also means a name can mean something else on the other end. A plaing++exists on every machine and targets whatever that machine is.Unchecked, that mismatch has two outcomes, and the quiet one is the dangerous one:
gnu/stubs-soft.h: No such file or directory), with an error pointing nowhere near the actual misconfiguration;The second one is reproducible in a couple of lines. A 32-bit client against a 64-bit server, compiling a struct and a function with no includes: exit code 0, and the resulting
.oisELF 64-bit LSB relocatable, ARM aarch64in a 32-bit build.A compiler that isn't installed on the server failed differently but no better: the server reported
exec: "…": executable file not found in $PATHas a compilation error, so the build stopped even though the client could have compiled the file itself.What this does
The client sends its compiler's target (
cxx -dumpmachine) when starting a session. The server checks that its own resolution ofCxxNameexists and targets the same triplet, and returnsFailedPreconditionotherwise — before anything is uploaded. Detection is cached per compiler name on both sides, so it costs oneexeceach, not one per file.Backward compatible in both directions: a client that reports no target (an older one, or a compiler without
-dumpmachine) is served as before, with existence still checked. An older server simply ignores the new field.A refusal is remembered per server and per compiler name, and
chooseRemoteConnectionForCppCompilationnow walks forward to the next server that can take the file instead of giving up on the first one. So an unusable server costs one local compilation rather than one per file, and only the files that hashed to it move — the rest of the mapping, and every other server's cache, is untouched. That applies to unavailable servers too, which previously sent their whole share to the local compiler.Note on the hash reduction
Resolving the file-to-server choice in
uint32rather thanintis not cosmetic: on a 32-bit clientint(hasher.Sum32())is negative for about half of all file names, and Go's%keeps the sign of the dividend, so the existing expression can index out of range. That's a pre-existing bug independent of this change — happy to split it into its own PR if you'd prefer it separately.Testing
Unit tests cover the accept/refuse/missing/no-
-dumpmachine/older-client paths and the caching, plus the routing behaviour (files move off an incapable server, incapability is per-compiler, other files keep their server, nil when nothing can serve). Each was checked against a deliberately broken implementation to confirm it fails.Verified on hardware, 32-bit client against a 64-bit server: the wrong-target case now compiles locally and produces a correct 32-bit object, the missing-compiler case does too, and a matching triplet still compiles remotely. With two servers where only one had the toolchain, a six-file build put five on the capable server and one local.
The generated
pb/nocc-protobuf.pb.gowas regenerated with protoc-gen-go v1.26.0 (the version stamped in the committed file); the protoc version line in the header changes because a newer protoc did the regeneration.