From 376488a50cf97571249030255020b1d8f90388dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niclas=20H=C3=BClsmann?= Date: Mon, 13 Jul 2026 12:46:56 +0200 Subject: [PATCH 1/2] fix(supervisor): stop terminate() racing watchChild's Cmd.Wait() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/supervisor/supervisor.go | 44 ++++++++++++++++++++++---- internal/supervisor/supervisor_test.go | 40 +++++++++++++++++++++++ 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/internal/supervisor/supervisor.go b/internal/supervisor/supervisor.go index 42f21934..82007585 100644 --- a/internal/supervisor/supervisor.go +++ b/internal/supervisor/supervisor.go @@ -83,6 +83,12 @@ type Running struct { auditSkill string auditNamespace string exitOnce sync.Once + + // waitDone is closed by watchChild once it has reaped Cmd (Cmd.Wait + // returned). terminate() waits on this instead of calling Cmd.Wait + // itself: os/exec.Cmd.Wait must only ever have one caller in flight, + // and a second concurrent caller can block forever (see watchChild). + waitDone chan struct{} } // Supervisor coordinates all sidecars. @@ -159,7 +165,7 @@ func (s *Supervisor) StopSidecar(name string, timeout time.Duration) bool { if target == nil { return false } - _ = terminate(target.Cmd, timeout) + _ = terminate(target.Cmd, timeout, target.waitDone) s.auditExit(target) // closes LogFile via exitOnce return true } @@ -196,12 +202,21 @@ func (s *Supervisor) auditExit(r *Running) { // termination time, not time-to-teardown. auditExit is idempotent via // exitOnce: if StopSidecar/ShutdownAll later call auditExit on an // already-reaped child, it's a no-op. +// +// watchChild is the sole owner of r.Cmd.Wait() for this child's lifetime. +// terminate() must never call Wait() itself on a child that has a running +// watchChild reaper (i.e. anything reachable via s.children) — it waits on +// r.waitDone instead. Calling Cmd.Wait() concurrently from two goroutines +// is unsupported by os/exec and can block the second caller forever, even +// after the process has actually exited. func (s *Supervisor) watchChild(r *Running) { if r == nil || r.Cmd == nil || r.Cmd.Process == nil { return } + r.waitDone = make(chan struct{}) go func() { _ = r.Cmd.Wait() + close(r.waitDone) s.auditExit(r) }() } @@ -286,7 +301,10 @@ func (s *Supervisor) startOne(ctx context.Context, spec SidecarSpec) (*Running, r.auditNamespace = spec.Namespace if err := waitHealth(ctx, port, spec.Health); err != nil { - _ = terminate(cmd, 3*time.Second) + // No watchChild reaper is running yet for this child (it only + // starts after the health check passes below), so terminate must + // reap it itself: pass nil. + _ = terminate(cmd, 3*time.Second, nil) lf.Close() return nil, fmt.Errorf("%s: health: %w", spec.Name, err) } @@ -364,7 +382,7 @@ func (s *Supervisor) ShutdownAll(timeout time.Duration) { wg.Add(1) go func() { defer wg.Done() - _ = terminate(r.Cmd, timeout) + _ = terminate(r.Cmd, timeout, r.waitDone) s.auditExit(r) // closes LogFile via exitOnce }() } @@ -372,8 +390,13 @@ func (s *Supervisor) ShutdownAll(timeout time.Duration) { } // terminate sends SIGTERM to the child's process group, waits up to timeout, -// then sends SIGKILL. -func terminate(cmd *exec.Cmd, timeout time.Duration) error { +// then sends SIGKILL. reaped, when non-nil, is a channel already being +// closed by a watchChild reaper once it reaps cmd; terminate waits on it +// instead of calling cmd.Wait() itself, since Wait must only ever have one +// caller in flight (a second concurrent caller can hang forever). Pass nil +// only when no reaper is running yet for cmd (e.g. a health-check failure +// during startup, before watchChild is started). +func terminate(cmd *exec.Cmd, timeout time.Duration, reaped <-chan struct{}) error { if cmd == nil || cmd.Process == nil { return nil } @@ -382,8 +405,15 @@ func terminate(cmd *exec.Cmd, timeout time.Duration) error { pgid = cmd.Process.Pid } _ = syscall.Kill(-pgid, syscall.SIGTERM) - done := make(chan error, 1) - go func() { done <- cmd.Wait() }() + done := reaped + if done == nil { + ch := make(chan struct{}) + go func() { + _ = cmd.Wait() + close(ch) + }() + done = ch + } select { case <-done: return nil diff --git a/internal/supervisor/supervisor_test.go b/internal/supervisor/supervisor_test.go index e2b2c389..64b39a57 100644 --- a/internal/supervisor/supervisor_test.go +++ b/internal/supervisor/supervisor_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "sync" + "syscall" "testing" "time" @@ -224,3 +225,42 @@ func TestStopSidecarDoesNotDoubleEmitProcessExit(t *testing.T) { t.Fatalf("want 1 process.exit (no double-emit), got %d", got) } } + +// TestShutdownAllReapsLongRunningChildWithoutHanging is a regression test +// for a deadlock where watchChild's reaper goroutine and terminate() both +// called Cmd.Wait() concurrently on the same child. os/exec.Cmd.Wait must +// only ever have one caller in flight; a second concurrent caller can +// block forever even after the process has been killed. That bug only +// shows up for a child that is still running when shutdown starts (a +// self-terminated child, as in the tests above, has no live race to lose). +// Uses a long-running child (sleep) plus a hard test-level timeout so a +// regression fails fast instead of hanging the whole test binary. +func TestShutdownAllReapsLongRunningChildWithoutHanging(t *testing.T) { + s := New(nil, nil) + + cmd := exec.Command("sleep", "30") + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + r := &Running{Name: "long-lived", Cmd: cmd, startedAt: time.Now()} + s.mu.Lock() + s.children = append(s.children, r) + s.mu.Unlock() + + // Reaper is now racing to Wait() on a child that is still alive — + // exactly the state that triggered the deadlock. + s.watchChild(r) + + done := make(chan struct{}) + go func() { + s.ShutdownAll(2 * time.Second) + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("ShutdownAll did not return within 5s — terminate() likely deadlocked racing watchChild's Cmd.Wait()") + } +} From 3eb67ab8d64f7a52522b9d1caf2db5ed652d64ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niclas=20H=C3=BClsmann?= Date: Mon, 13 Jul 2026 12:56:14 +0200 Subject: [PATCH 2/2] fix(e2e): make harness-count tests aware of the darwin codex exclusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/e2e/harnesses_test.go | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/internal/e2e/harnesses_test.go b/internal/e2e/harnesses_test.go index b8c07590..e83200b6 100644 --- a/internal/e2e/harnesses_test.go +++ b/internal/e2e/harnesses_test.go @@ -3,18 +3,37 @@ package e2e import ( + "runtime" "testing" ) +// expectedHarnessNames is allHarnesses' expected content for the current +// GOOS: codex is excluded on darwin (see allHarnesses; its Rust HTTP client +// is incompatible with the macOS Seatbelt sandbox — issue #48). +func expectedHarnessNames() []string { + names := []string{"opencode", "claude-code", "codex", "copilot"} + if runtime.GOOS != "darwin" { + return names + } + out := names[:0:0] + for _, n := range names { + if n != "codex" { + out = append(out, n) + } + } + return out +} + func TestAllHarnessesReturnsFour(t *testing.T) { hs := allHarnesses() - if len(hs) != 4 { - t.Fatalf("expected 4 harnesses, got %d", len(hs)) + want := len(expectedHarnessNames()) + if len(hs) != want { + t.Fatalf("expected %d harnesses, got %d", want, len(hs)) } } func TestHarnessByName(t *testing.T) { - for _, name := range []string{"opencode", "claude-code", "codex", "copilot"} { + for _, name := range expectedHarnessNames() { h, ok := harnessByName(name) if !ok { t.Fatalf("harnessByName(%q) not found", name) @@ -23,6 +42,11 @@ func TestHarnessByName(t *testing.T) { t.Fatalf("harnessByName(%q) returned %q", name, h.Name) } } + if runtime.GOOS == "darwin" { + if _, ok := harnessByName("codex"); ok { + t.Fatal("harnessByName(\"codex\") should return false on darwin") + } + } if _, ok := harnessByName("nonexistent"); ok { t.Fatal("harnessByName should return false for unknown harness") }