fix(supervisor): stop terminate() racing watchChild's Cmd.Wait()

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")
 	}

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,16 +382,21 @@ 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
 		}()
 	}
 	wg.Wait()
 }
 
 // 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

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()")
+	}
+}