From bb8648938f76f7638b96c10b27000d1e76d901da Mon Sep 17 00:00:00 2001 From: Daniel Kulp Date: Wed, 19 Aug 2026 16:48:44 -0400 Subject: [PATCH] Fix daemon panic on 32-bit clients with more than one server 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) --- internal/client/daemon.go | 8 +++- internal/client/daemon_test.go | 70 ++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 internal/client/daemon_test.go diff --git a/internal/client/daemon.go b/internal/client/daemon.go index ed85910..2940f58 100644 --- a/internal/client/daemon.go +++ b/internal/client/daemon.go @@ -336,5 +336,11 @@ func (daemon *Daemon) areAllRemotesAvailable() bool { func (daemon *Daemon) chooseRemoteConnectionForCppCompilation(cppInFile string) *RemoteConnection { hasher := fnv.New32a() _, _ = hasher.Write([]byte(filepath.Base(cppInFile))) - return daemon.remoteConnections[int(hasher.Sum32())%len(daemon.remoteConnections)] + // Reduce in uint32, not int. Where int is 32 bits (armv7, i386), int(hasher.Sum32()) + // is negative for every hash with the top bit set -- about half of all file names -- + // and Go's % keeps the sign of the dividend, so the index could come out negative + // and panic. With len == 1 it never did (x%1 == 0 for any x), which masked this + // entirely; with len == n a name panics when its negative hash is not a multiple + // of n, i.e. for (n-1)/2n of all names -- a quarter of them at two servers. + return daemon.remoteConnections[hasher.Sum32()%uint32(len(daemon.remoteConnections))] } diff --git a/internal/client/daemon_test.go b/internal/client/daemon_test.go new file mode 100644 index 0000000..2ad1405 --- /dev/null +++ b/internal/client/daemon_test.go @@ -0,0 +1,70 @@ +package client + +import ( + "fmt" + "strconv" + "testing" +) + +func makeDaemonWithRemotes(numRemotes int) *Daemon { + conns := make([]*RemoteConnection, numRemotes) + for i := range conns { + conns[i] = &RemoteConnection{remoteHost: fmt.Sprintf("host%d", i)} + } + return &Daemon{remoteConnections: conns} +} + +// chooseRemoteConnectionForCppCompilation used to reduce the FNV hash in `int`: +// +// daemon.remoteConnections[int(hasher.Sum32())%len(daemon.remoteConnections)] +// +// Where `int` is 32 bits, that conversion is negative for every hash with the top +// bit set -- about half of all file names -- and Go's % keeps the sign of the +// dividend, so the index could come out negative and panic the daemon. It is not +// every such name: with n remotes the result is negative unless the hash is a +// multiple of n, so (n-1)/2n of all names panic, a quarter of them at n == 2. +// A single remote masked it completely, because x%1 == 0 for any x. +// +// This is the arch-specific half of the guard, and it is honest about it: on a +// 64-bit host the old code cannot produce a negative index, so the test would +// pass against the bug and prove nothing. Skip rather than report a green that +// did not exercise anything. Run the suite under GOARCH=386 or GOARCH=arm to +// actually execute this. +func TestChooseRemoteConnectionIndexIsNonNegativeOn32Bit(t *testing.T) { + if strconv.IntSize != 32 { + t.Skipf("int is %d bits here; this regression only reproduces where int is 32 bits (GOARCH=386, GOARCH=arm)", strconv.IntSize) + } + + for _, numRemotes := range []int{2, 3, 4, 5, 8} { + daemon := makeDaemonWithRemotes(numRemotes) + for i := 0; i < 20000; i++ { + cppInFile := fmt.Sprintf("src/file%d.cpp", i) + if remote := daemon.chooseRemoteConnectionForCppCompilation(cppInFile); remote == nil { + t.Fatalf("numRemotes=%d %s: got nil remote", numRemotes, cppInFile) + } + } + } +} + +// The invariant itself, which is worth checking on whatever architecture the +// suite happens to run on: every configured remote is reachable by some file +// name, and no name selects out of range. This one is not a guard against the +// 32-bit bug above -- it cannot be -- so it does not skip. +func TestChooseRemoteConnectionUsesEveryRemote(t *testing.T) { + for _, numRemotes := range []int{1, 2, 3, 4, 5, 8} { + daemon := makeDaemonWithRemotes(numRemotes) + + seen := make(map[string]bool, numRemotes) + for i := 0; i < 20000; i++ { + cppInFile := fmt.Sprintf("src/file%d.cpp", i) + remote := daemon.chooseRemoteConnectionForCppCompilation(cppInFile) + if remote == nil { + t.Fatalf("numRemotes=%d %s: got nil remote", numRemotes, cppInFile) + } + seen[remote.remoteHost] = true + } + if len(seen) != numRemotes { + t.Errorf("numRemotes=%d: only %d of the remotes were ever chosen", numRemotes, len(seen)) + } + } +}