fix(supervisor): stop terminate() racing watchChild's Cmd.Wait()#62
Merged
Conversation
The audit-trail commit (#38) added a watchChild reaper goroutine that calls Cmd.Wait() on a sidecar as soon as it starts, so process.exit can be audited at actual termination time. But terminate() (used by StopSidecar/ShutdownAll) also spawned its own goroutine calling Cmd.Wait() on the same *exec.Cmd. os/exec.Cmd.Wait must only ever have one caller in flight; concurrent callers race on shared internal state, and the loser can block forever reading a channel the other caller has already drained — even after the child has actually exited. This is the root cause of "agent did not exit within 5m0s" e2e hangs observed across every harness (opencode/codex/copilot/claude-code all register sidecars, so all are equally exposed) — confirmed with: - a goroutine dump from a hung process, showing the main goroutine stuck in ShutdownAll's WaitGroup.Wait() -> terminate() -> Cmd.Wait(), - a 10-iteration local repro (register echo-rest, `omac start` in a loop) that hung deterministically on every run after the first before this fix, and 0/10 after, - `go test -race` on a new regression test, which flags the exact data race between terminate.func1 and watchChild.func1 on the same Cmd before this fix, and passes clean after. Fix: Running now carries a waitDone channel that watchChild closes after it reaps the child. terminate() waits on that channel instead of calling Cmd.Wait() itself whenever a reaper is already running (i.e. for any child reachable via s.children). The one call site that runs before watchChild starts (a health-check failure during startup) still spawns its own Wait(), since no reaper exists yet there. This supersedes the speculative "reduce E2E concurrency" mitigation in a sibling PR — that PR's theory (shared GLM-backend contention) didn't hold up: claude-code, which uses a separate backend, hung with the same signature in a later run. This fix addresses the actual, reproducible cause and isn't backend- or harness-specific. Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
allHarnesses() has excluded codex on darwin since cba169f (its Rust HTTP client is incompatible with the macOS Seatbelt sandbox — issue #48), but TestAllHarnessesReturnsFour and TestHarnessByName still hardcoded an expectation of exactly 4 harnesses including codex. This has failed deterministically on every macOS E2E job since that commit landed, visible in CI as harnesses_test.go:12 "expected 4 harnesses, got 3" and harnesses_test.go:20 "harnessByName(\"codex\") not found". Derive the expected harness set from GOOS the same way allHarnesses() does, so the tests track that exclusion instead of duplicating a stale assumption. Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
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.
What
Fixes a data race between
terminate()andwatchChild()'s reaper goroutine, both of which calledos/exec.Cmd.Wait()concurrently on the same sidecar process.Why
This is the root cause of the
agent did not exit within 5m0sE2E hangs investigated in #60/#61 — every matrix job registers at least one sidecar (echo-rest/self-audit), so every harness (includingclaude-code) is equally exposed, which is why #61's "shared GLM-backend contention" theory stopped holding up onceclaude-codehung too in a later run.watchChild()(added in #38) spawns a goroutine that callsr.Cmd.Wait()on a sidecar as soon as it starts, soprocess.exitcan be audited at real termination time. Butterminate()(called byStopSidecar/ShutdownAllat teardown) also spawns a goroutine callingcmd.Wait()on the same*exec.Cmd.os/exec.Cmd.Waitmust only ever have one caller in flight — calling it concurrently races on shared internal state, and the loser can block forever reading a channel the other caller already drained, even though the child process has actually exited. Once that happens,ShutdownAll'sWaitGroup.Wait()never returns, so the parentomac startprocess (which the e2e test'scmd.Run()is blocked on) never exits either.How
Runningnow carries awaitDonechannel thatwatchChildcloses after it reaps the child.terminate()waits on that channel instead of callingCmd.Wait()itself whenever a reaper is already running (any child reachable vias.children— i.e.StopSidecarandShutdownAll). The one call site that runs beforewatchChildstarts (a health-check failure during sidecar startup) still spawns its ownWait(), since no reaper exists yet there.Verification
echo-rest,omac start opencode --no-sandboxin a loop against a fake fast-exiting harness) and captured aSIGQUITdump showing the main goroutine wedged inShutdownAll'sWaitGroup.Wait()→terminate()→Cmd.Wait(), forever.--no-sandboxand under the real bubblewrap sandbox.go test -race: addedTestShutdownAllReapsLongRunningChildWithoutHanging— confirmed it fails withWARNING: DATA RACEbetweenterminate.func1andwatchChild.func1on the pre-fix code (reverted via patch to check), and passes clean post-fix.go build ./...,go vet ./...,gofmt -l, fullgo test ./...(non-e2e) — all clean.internal/supervisorsuite passes under-race.Relationship to #60 / #61