Fix daemon panic on 32-bit clients with more than one server - #32
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a 32-bit-only panic in the client daemon’s remote-selection logic by avoiding a signed int reduction of a uint32 FNV-1a hash, and adds a regression test that exercises the failure mode under 32-bit int architectures.
Changes:
- Change remote selection to reduce the hash in
uint32to avoid negative modulo results on 32-bit clients. - Add a regression test that iterates many filenames across multiple remote counts and asserts selection stays valid and spreads across all remotes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/client/daemon.go | Fixes remote index calculation to prevent negative indices (32-bit int overflow/sign issue). |
| internal/client/daemon_test.go | Adds a regression test to catch the 32-bit panic scenario and validate remote selection distribution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Reduce in uint32, not int. On a 32-bit client (armv7 -- BeagleBone, Pi Zero/1) | ||
| // int is 32 bits, so int(hasher.Sum32()) is negative for every hash with the top | ||
| // bit set, i.e. about half of all file names. A negative dividend keeps its sign | ||
| // through Go's % operator, so the index became -1 with two servers configured and | ||
| // panicked the daemon on the first such file. With a single server it happened to | ||
| // be masked, since x%1 == 0 for any x. |
There was a problem hiding this comment.
Good catch, you are right and I have reworded it. I measured it rather than reasoning about it: with n remotes the remainder is negative unless the hash happens to be a multiple of n, so it is (n-1)/2n of all file names, not every negative hash. At n=2 that is 25% of names panicking and 25% hashing negative but landing on index 0 and working fine. Counted over 200k names:
n=1: 50.0% hash negative, 0.0% give a negative index
n=2: 50.0% hash negative, 25.0% give a negative index
n=3: 50.0% hash negative, 33.3% give a negative index
n=4: 50.0% hash negative, 37.5% give a negative index
The comment now says the index "could come out negative" and gives the (n-1)/2n rate.
| // On a 32-bit client (armv7 -- BeagleBone, Raspberry Pi Zero/1) `int` is 32 bits, | ||
| // so that conversion is negative for every hash with the top bit set, which is | ||
| // roughly half of all file names. Go's % keeps the sign of the dividend, so with | ||
| // two servers configured the expression yielded -1 and panicked the daemon on the | ||
| // first such file; every later invocation then fell back to compiling locally. |
There was a problem hiding this comment.
Same rewording applied here, thanks. The test comment now states the (n-1)/2n rate and says the index could come out negative rather than implying a deterministic -1 on the first affected file.
| if strconv.IntSize != 32 { | ||
| t.Logf("int is %d bits here; the regression this guards only reproduces with a 32-bit int", strconv.IntSize) | ||
| } |
There was a problem hiding this comment.
Agreed on the substance, and I have split the test in two rather than skipping the whole thing:
TestChooseRemoteConnectionIndexIsNonNegativeOn32Bitnow skips unlessstrconv.IntSize == 32, exactly as you suggest. On a 64-bit host it genuinely cannot fail against the old code, so reporting a pass there was misleading.TestChooseRemoteConnectionUsesEveryRemotekeeps running everywhere. It asserts that every configured remote is reachable by some file name and none selects out of range, which is worth having on the host arch too and is not arch-specific.
Verified both directions after the split: with the fix reverted the first test panics with index out of range [-1] under GOARCH=arm, and skips on arm64.
chooseRemoteConnectionForCppCompilation reduces the FNV-1a hash of the file
name in `int`:
daemon.remoteConnections[int(hasher.Sum32())%len(daemon.remoteConnections)]
Where `int` is 32 bits -- any armv7 or i386 client -- that conversion is
negative for every hash whose top bit is set, which is about half of all file
names. Go's % keeps the sign of the dividend, so the index can come out negative
and panic the daemon.
Not every such name trips it: with n remotes the remainder is negative unless
the hash is a multiple of n, so (n-1)/2n of all names panic -- a quarter of them
at two remotes, a third at three. A single remote masks it completely, since
x%1 == 0 for any x, which is probably why it went unnoticed.
The panic kills the daemon, so every later `nocc` invocation on that machine
fails to reach it and falls back to compiling locally. The build still succeeds,
just without any distribution at all -- which on the slow single-core boards
that benefit most from nocc is precisely the load it exists to move off the box,
and nothing in the output points at the cause.
Reduce in uint32 instead.
Two tests, deliberately split by what they can actually prove:
* the arch-specific guard skips unless int is 32 bits. On a 64-bit host the
old code cannot produce a negative index, so running it there would report
a green that exercised nothing. Run the suite under GOARCH=386 or
GOARCH=arm to execute it.
* the invariant that every configured remote is reachable and no name selects
out of range runs everywhere, since it is worth having on the host arch too.
Verified by reverting the fix: the first test panics with "index out of range
[-1]" under GOARCH=arm, and skips on arm64.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The bug
chooseRemoteConnectionForCppCompilationreduces the FNV-1a hash of the file name inint:Where
intis 32 bits — any armv7 or i386 client —int(hasher.Sum32())is negative for every hash whose top bit is set, i.e. roughly half of all file names. Go's%keeps the sign of the dividend, so with two or more servers configured this evaluates to a negative index and panics the daemon on the first such file.A single server masks it completely, since
x%1 == 0for anyx, which is probably why it went unnoticed.Why it is worth fixing rather than shrugging at
The panic kills the daemon, so every subsequent
noccon that machine cannot reach it and falls back to compiling locally. The build still succeeds — it just silently stops distributing anything, which on the slow single-core boards that benefit most from nocc is exactly the load it exists to move off the machine. Nothing in the build output points at the cause.The fix
Reduce in
uint32.Test
The added test walks 20000 file names against 1..8 configured servers and asserts the index stays in range and that every remote gets chosen.
Note that on a 64-bit host the old code passes this test, so it is worth running the suite under a 32-bit
GOARCHas well:Verified by reverting the fix: the test panics with
index out of range [-1]underGOARCH=arm, and passes onarm64.gofmt/go vetclean. Thetests/suite is unchanged from master's results on my machine.