From 67decaaea2f7264aa08105df453f8bcfaa319c79 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 9 Sep 2026 16:23:16 -0700 Subject: [PATCH 1/6] Kill the Codex probe's process group when its deadline expires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The probe ran codex through cmd.Output with a WaitDelay, which bounded the call but not what it left behind: where codex is a wrapper that exits at once and backgrounds the real work, the deadline expired after the exec package had stopped watching the context, so cmd.Cancel never ran and the descendant survived every timed-out doctor run. Start the child in its own process group, read its stdout directly, and kill the group when the context expires — before Wait reaps the leader, while the group ID is still ours. WaitDelay stays as the bound for a descendant that leaves the group. Fixes #630 --- internal/harness/codex.go | 66 ++++++++++++++++++++++++----- internal/harness/codex_test.go | 60 ++++++++++++++++---------- internal/harness/procgroup_other.go | 13 ++++++ internal/harness/procgroup_unix.go | 21 +++++++++ 4 files changed, 127 insertions(+), 33 deletions(-) create mode 100644 internal/harness/procgroup_other.go create mode 100644 internal/harness/procgroup_unix.go diff --git a/internal/harness/codex.go b/internal/harness/codex.go index e1e29860..03e0ca5c 100644 --- a/internal/harness/codex.go +++ b/internal/harness/codex.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -25,8 +26,8 @@ const ( // codexQueryTimeout bounds how long the Codex probe may run. codexQueryTimeout = 5 * time.Second - // codexWaitDelay is the grace period after the kill before Wait gives up on - // output pipes a surviving grandchild still holds open. + // codexWaitDelay is the grace period after the group kill before the + // probe gives up on a pipe some escaped descendant still holds open. codexWaitDelay = time.Second ) @@ -34,18 +35,63 @@ var ( codexLookPath = exec.LookPath runCodexCommand = func(ctx context.Context, path string, args ...string) ([]byte, error) { cmd := exec.CommandContext(ctx, path, args...) //nolint:gosec // path comes from exec.LookPath - // Bound Wait, not just the process. Canceling the context kills the - // child, but it does not close output pipes a *grandchild* inherited, - // and Wait blocks on those copies until they do — so the 5s timeout in - // queryCodexPlugin buys nothing on its own. `codex` is routinely a - // wrapper that shells out (an npm exec launcher, a mise shim), and one - // of those left `basecamp doctor` hanging for ten minutes rather than - // five seconds. WaitDelay is what makes the deadline real. + startInOwnProcessGroup(cmd) + cmd.Cancel = func() error { return killProcessGroup(cmd) } cmd.WaitDelay = codexWaitDelay - return cmd.Output() + stdout, err := cmd.StdoutPipe() + if err != nil { + return nil, err + } + if err := cmd.Start(); err != nil { + return nil, err + } + + // `codex` is routinely a wrapper (an npm exec launcher, a mise shim) + // that exits at once and leaves a descendant holding the inherited + // stdout. That descendant is why the output is read here rather + // than through Output: the exec package stops watching the context + // once the direct child exits, so cmd.Cancel never fires for a + // deadline that expires after that, and the group has to be killed + // before Wait reaps its leader — the group ID is the leader's PID, + // free for reuse from the reap onward. + read := make(chan codexRead, 1) + go func() { + data, err := io.ReadAll(stdout) + read <- codexRead{data: data, err: err} + }() + + var out codexRead + select { + case out = <-read: + case <-ctx.Done(): + _ = killProcessGroup(cmd) + select { + case out = <-read: + case <-time.After(codexWaitDelay): + // A descendant that left the group (setsid) is out of reach + // and still holds the pipe; closing our end ends the read. + _ = stdout.Close() + out = <-read + } + } + waitErr := cmd.Wait() + if ctx.Err() != nil { + return nil, ctx.Err() + } + if waitErr != nil { + return nil, waitErr + } + return out.data, out.err } ) +// codexRead is what the stdout reader hands back: everything the probe +// wrote, and the error that ended the read. +type codexRead struct { + data []byte + err error +} + var ( errCodexBinaryMissing = errors.New("codex executable not found") errCodexParse = errors.New("parse Codex plugin list") diff --git a/internal/harness/codex_test.go b/internal/harness/codex_test.go index f28606e7..9d36fc84 100644 --- a/internal/harness/codex_test.go +++ b/internal/harness/codex_test.go @@ -6,8 +6,10 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strconv" "strings" + "syscall" "testing" "time" @@ -205,59 +207,71 @@ func boolJSON(value bool) string { return "false" } -// TestRunCodexCommandOutlivingGrandchild pins the deadline that ten minutes of -// a hung `basecamp doctor` proved was not being enforced. +// TestRunCodexCommandOutlivingGrandchild pins two things ten minutes of a +// hung `basecamp doctor` proved were not being enforced: the deadline, and +// that nothing survives it. // // The stub above replaces runCodexCommand, so nothing else here exercises the // real one. This does. It stands in for the shape codex actually ships as on -// some machines — a wrapper script that backgrounds a longer-lived process — -// where canceling the context kills the wrapper but the grandchild keeps the -// inherited stdout pipe open. Without cmd.WaitDelay, Wait blocks on that pipe -// for as long as the grandchild lives, and the query timeout means nothing. +// some machines — a wrapper script that backgrounds a longer-lived process and +// exits at once — where the grandchild keeps the inherited stdout pipe open. +// The call has to return on its own deadline rather than the grandchild's, +// and the grandchild has to be dead when it does: the wrapper exited long +// before the deadline, so only a kill aimed at the process group reaches it. func TestRunCodexCommandOutlivingGrandchild(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no process groups on Windows") + } sh, err := exec.LookPath("sh") if err != nil { t.Skip("sh not available") } // The grandchild has to outlive the deadline by a wide margin, or the test - // passes on the sleep ending rather than on WaitDelay working. That makes - // it our job to reap it: WaitDelay closes the inherited pipe, it does not - // kill the process, which is reparented to init and would otherwise sit - // there for two minutes accumulating one orphan per `bin/ci`. + // passes on the sleep ending rather than on the kill working. The cleanup + // reaps it if the kill did not, so a failing run leaves no orphan behind. pidFile := filepath.Join(t.TempDir(), "grandchild.pid") script := "sleep 120 & echo $! > " + pidFile + "; exit 0" - t.Cleanup(func() { + grandchild := func() (int, bool) { raw, readErr := os.ReadFile(pidFile) //nolint:gosec // G304: path is this test's own TempDir if readErr != nil { - return + return 0, false } pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))) - if convErr != nil { - return - } - if proc, findErr := os.FindProcess(pid); findErr == nil { - _ = proc.Kill() + return pid, convErr == nil + } + t.Cleanup(func() { + if pid, ok := grandchild(); ok { + if proc, findErr := os.FindProcess(pid); findErr == nil { + _ = proc.Kill() + } } }) ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) defer cancel() - done := make(chan struct{}) + done := make(chan error, 1) start := time.Now() go func() { - defer close(done) - _, _ = runCodexCommand(ctx, sh, "-c", script) + _, err := runCodexCommand(ctx, sh, "-c", script) + done <- err }() select { - case <-done: - // The call must return on its own deadline, not the grandchild's. + case err := <-done: assert.Less(t, time.Since(start), 30*time.Second, "runCodexCommand blocked on a pipe held open by a surviving grandchild") + assert.ErrorIs(t, err, context.DeadlineExceeded) case <-time.After(30 * time.Second): - t.Fatal("runCodexCommand did not return: WaitDelay is not bounding Wait") + t.Fatal("runCodexCommand did not return: the deadline is not bounding the call") } + + pid, ok := grandchild() + require.True(t, ok, "wrapper did not record the grandchild's pid") + assert.Eventually(t, func() bool { + return syscall.Kill(pid, 0) == syscall.ESRCH + }, 5*time.Second, 50*time.Millisecond, + "grandchild %d outlived the deadline: the process group was not killed", pid) } diff --git a/internal/harness/procgroup_other.go b/internal/harness/procgroup_other.go new file mode 100644 index 00000000..002d6d13 --- /dev/null +++ b/internal/harness/procgroup_other.go @@ -0,0 +1,13 @@ +//go:build !unix + +package harness + +import "os/exec" + +// startInOwnProcessGroup is a no-op where process groups are unavailable. +func startInOwnProcessGroup(*exec.Cmd) {} + +// killProcessGroup kills the child alone; its descendants are out of reach. +func killProcessGroup(cmd *exec.Cmd) error { + return cmd.Process.Kill() +} diff --git a/internal/harness/procgroup_unix.go b/internal/harness/procgroup_unix.go new file mode 100644 index 00000000..05f0697d --- /dev/null +++ b/internal/harness/procgroup_unix.go @@ -0,0 +1,21 @@ +//go:build unix + +package harness + +import ( + "os/exec" + "syscall" +) + +// startInOwnProcessGroup makes the child a process group leader, so that it +// and every descendant that stays in the group can be signaled as one unit. +func startInOwnProcessGroup(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} +} + +// killProcessGroup kills the child and every descendant still in its group. +// The group ID is the child's PID and stays reserved only while a member of +// the group exists, so this must run before Wait reaps the child. +func killProcessGroup(cmd *exec.Cmd) error { + return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) +} From dc40d042901f7759a99e04d92c833f24615bab14 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 9 Sep 2026 16:39:56 -0700 Subject: [PATCH 2/6] Issue the group kill only before Wait, and build the probe test on unix alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installing the group kill as cmd.Cancel let the exec package's watcher fire between Process.Wait reaping the leader and Wait synchronizing with it — after the group ID could have been recycled, the race this change exists to exclude. The kill now happens in one place, on this goroutine, strictly before Wait; a leader still running past that point is killed alone by the default cancel. The grandchild assertion uses syscall.Kill, so the test moves behind a unix build tag instead of a runtime skip. --- internal/harness/codex.go | 10 ++-- internal/harness/codex_test.go | 74 ------------------------- internal/harness/codex_unix_test.go | 84 +++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 78 deletions(-) create mode 100644 internal/harness/codex_unix_test.go diff --git a/internal/harness/codex.go b/internal/harness/codex.go index 03e0ca5c..488f9718 100644 --- a/internal/harness/codex.go +++ b/internal/harness/codex.go @@ -36,7 +36,6 @@ var ( runCodexCommand = func(ctx context.Context, path string, args ...string) ([]byte, error) { cmd := exec.CommandContext(ctx, path, args...) //nolint:gosec // path comes from exec.LookPath startInOwnProcessGroup(cmd) - cmd.Cancel = func() error { return killProcessGroup(cmd) } cmd.WaitDelay = codexWaitDelay stdout, err := cmd.StdoutPipe() if err != nil { @@ -51,9 +50,12 @@ var ( // stdout. That descendant is why the output is read here rather // than through Output: the exec package stops watching the context // once the direct child exits, so cmd.Cancel never fires for a - // deadline that expires after that, and the group has to be killed - // before Wait reaps its leader — the group ID is the leader's PID, - // free for reuse from the reap onward. + // deadline that expires after that. The group kill below is the + // only one, and it runs strictly before Wait on this goroutine: the + // group ID is the leader's PID, reserved only until the leader is + // reaped, so a kill issued from cmd.Cancel could race the reap and + // land on a recycled ID. Once Wait begins, a leader still running + // is killed alone by the exec package's own cancel. read := make(chan codexRead, 1) go func() { data, err := io.ReadAll(stdout) diff --git a/internal/harness/codex_test.go b/internal/harness/codex_test.go index 9d36fc84..c0235065 100644 --- a/internal/harness/codex_test.go +++ b/internal/harness/codex_test.go @@ -6,12 +6,7 @@ import ( "os" "os/exec" "path/filepath" - "runtime" - "strconv" - "strings" - "syscall" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -206,72 +201,3 @@ func boolJSON(value bool) string { } return "false" } - -// TestRunCodexCommandOutlivingGrandchild pins two things ten minutes of a -// hung `basecamp doctor` proved were not being enforced: the deadline, and -// that nothing survives it. -// -// The stub above replaces runCodexCommand, so nothing else here exercises the -// real one. This does. It stands in for the shape codex actually ships as on -// some machines — a wrapper script that backgrounds a longer-lived process and -// exits at once — where the grandchild keeps the inherited stdout pipe open. -// The call has to return on its own deadline rather than the grandchild's, -// and the grandchild has to be dead when it does: the wrapper exited long -// before the deadline, so only a kill aimed at the process group reaches it. -func TestRunCodexCommandOutlivingGrandchild(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("no process groups on Windows") - } - sh, err := exec.LookPath("sh") - if err != nil { - t.Skip("sh not available") - } - - // The grandchild has to outlive the deadline by a wide margin, or the test - // passes on the sleep ending rather than on the kill working. The cleanup - // reaps it if the kill did not, so a failing run leaves no orphan behind. - pidFile := filepath.Join(t.TempDir(), "grandchild.pid") - script := "sleep 120 & echo $! > " + pidFile + "; exit 0" - - grandchild := func() (int, bool) { - raw, readErr := os.ReadFile(pidFile) //nolint:gosec // G304: path is this test's own TempDir - if readErr != nil { - return 0, false - } - pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))) - return pid, convErr == nil - } - t.Cleanup(func() { - if pid, ok := grandchild(); ok { - if proc, findErr := os.FindProcess(pid); findErr == nil { - _ = proc.Kill() - } - } - }) - - ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) - defer cancel() - - done := make(chan error, 1) - start := time.Now() - go func() { - _, err := runCodexCommand(ctx, sh, "-c", script) - done <- err - }() - - select { - case err := <-done: - assert.Less(t, time.Since(start), 30*time.Second, - "runCodexCommand blocked on a pipe held open by a surviving grandchild") - assert.ErrorIs(t, err, context.DeadlineExceeded) - case <-time.After(30 * time.Second): - t.Fatal("runCodexCommand did not return: the deadline is not bounding the call") - } - - pid, ok := grandchild() - require.True(t, ok, "wrapper did not record the grandchild's pid") - assert.Eventually(t, func() bool { - return syscall.Kill(pid, 0) == syscall.ESRCH - }, 5*time.Second, 50*time.Millisecond, - "grandchild %d outlived the deadline: the process group was not killed", pid) -} diff --git a/internal/harness/codex_unix_test.go b/internal/harness/codex_unix_test.go new file mode 100644 index 00000000..ae1d179b --- /dev/null +++ b/internal/harness/codex_unix_test.go @@ -0,0 +1,84 @@ +//go:build unix + +package harness + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRunCodexCommandOutlivingGrandchild pins two things ten minutes of a +// hung `basecamp doctor` proved were not being enforced: the deadline, and +// that nothing survives it. +// +// The stub above replaces runCodexCommand, so nothing else here exercises the +// real one. This does. It stands in for the shape codex actually ships as on +// some machines — a wrapper script that backgrounds a longer-lived process and +// exits at once — where the grandchild keeps the inherited stdout pipe open. +// The call has to return on its own deadline rather than the grandchild's, +// and the grandchild has to be dead when it does: the wrapper exited long +// before the deadline, so only a kill aimed at the process group reaches it. +func TestRunCodexCommandOutlivingGrandchild(t *testing.T) { + sh, err := exec.LookPath("sh") + if err != nil { + t.Skip("sh not available") + } + + // The grandchild has to outlive the deadline by a wide margin, or the test + // passes on the sleep ending rather than on the kill working. The cleanup + // reaps it if the kill did not, so a failing run leaves no orphan behind. + pidFile := filepath.Join(t.TempDir(), "grandchild.pid") + script := "sleep 120 & echo $! > " + pidFile + "; exit 0" + + grandchild := func() (int, bool) { + raw, readErr := os.ReadFile(pidFile) //nolint:gosec // G304: path is this test's own TempDir + if readErr != nil { + return 0, false + } + pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))) + return pid, convErr == nil + } + t.Cleanup(func() { + if pid, ok := grandchild(); ok { + if proc, findErr := os.FindProcess(pid); findErr == nil { + _ = proc.Kill() + } + } + }) + + ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + defer cancel() + + done := make(chan error, 1) + start := time.Now() + go func() { + _, err := runCodexCommand(ctx, sh, "-c", script) + done <- err + }() + + select { + case err := <-done: + assert.Less(t, time.Since(start), 30*time.Second, + "runCodexCommand blocked on a pipe held open by a surviving grandchild") + assert.ErrorIs(t, err, context.DeadlineExceeded) + case <-time.After(30 * time.Second): + t.Fatal("runCodexCommand did not return: the deadline is not bounding the call") + } + + pid, ok := grandchild() + require.True(t, ok, "wrapper did not record the grandchild's pid") + assert.Eventually(t, func() bool { + return syscall.Kill(pid, 0) == syscall.ESRCH + }, 5*time.Second, 50*time.Millisecond, + "grandchild %d outlived the deadline: the process group was not killed", pid) +} From 3d4b1496c0b6c510f4f9bbaba87afc8596804c64 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 9 Sep 2026 16:45:51 -0700 Subject: [PATCH 3/6] Reap test descendants only when they are verifiably ours, and cover the escaped one The cleanup killed whatever held the recorded pid, which on a passing run was a pid the test had just watched disappear. It now signals only a process kill(pid, 0) still finds, and only where one is expected: a failed group kill, or the setsid descendant the new test leaves behind on purpose to prove the read gives up on its own. --- internal/harness/codex_unix_test.go | 116 +++++++++++++++++----------- 1 file changed, 73 insertions(+), 43 deletions(-) diff --git a/internal/harness/codex_unix_test.go b/internal/harness/codex_unix_test.go index ae1d179b..b628ed9e 100644 --- a/internal/harness/codex_unix_test.go +++ b/internal/harness/codex_unix_test.go @@ -17,68 +17,98 @@ import ( "github.com/stretchr/testify/require" ) -// TestRunCodexCommandOutlivingGrandchild pins two things ten minutes of a -// hung `basecamp doctor` proved were not being enforced: the deadline, and -// that nothing survives it. -// -// The stub above replaces runCodexCommand, so nothing else here exercises the -// real one. This does. It stands in for the shape codex actually ships as on -// some machines — a wrapper script that backgrounds a longer-lived process and -// exits at once — where the grandchild keeps the inherited stdout pipe open. -// The call has to return on its own deadline rather than the grandchild's, -// and the grandchild has to be dead when it does: the wrapper exited long -// before the deadline, so only a kill aimed at the process group reaches it. -func TestRunCodexCommandOutlivingGrandchild(t *testing.T) { +// codexWrapper runs script through sh as the probe's command, with a +// deadline well short of the sleep the script backgrounds, and returns the +// error and the pid the script recorded. +func codexWrapper(t *testing.T, script string, deadline time.Duration) (error, int) { + t.Helper() sh, err := exec.LookPath("sh") if err != nil { t.Skip("sh not available") } + pidFile := filepath.Join(t.TempDir(), "descendant.pid") + script = strings.ReplaceAll(script, "PIDFILE", pidFile) - // The grandchild has to outlive the deadline by a wide margin, or the test - // passes on the sleep ending rather than on the kill working. The cleanup - // reaps it if the kill did not, so a failing run leaves no orphan behind. - pidFile := filepath.Join(t.TempDir(), "grandchild.pid") - script := "sleep 120 & echo $! > " + pidFile + "; exit 0" - - grandchild := func() (int, bool) { - raw, readErr := os.ReadFile(pidFile) //nolint:gosec // G304: path is this test's own TempDir - if readErr != nil { - return 0, false - } - pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))) - return pid, convErr == nil - } - t.Cleanup(func() { - if pid, ok := grandchild(); ok { - if proc, findErr := os.FindProcess(pid); findErr == nil { - _ = proc.Kill() - } - } - }) - - ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), deadline) defer cancel() done := make(chan error, 1) - start := time.Now() go func() { _, err := runCodexCommand(ctx, sh, "-c", script) done <- err }() - select { - case err := <-done: - assert.Less(t, time.Since(start), 30*time.Second, - "runCodexCommand blocked on a pipe held open by a surviving grandchild") - assert.ErrorIs(t, err, context.DeadlineExceeded) + case err = <-done: case <-time.After(30 * time.Second): t.Fatal("runCodexCommand did not return: the deadline is not bounding the call") } - pid, ok := grandchild() - require.True(t, ok, "wrapper did not record the grandchild's pid") + raw, readErr := os.ReadFile(pidFile) //nolint:gosec // G304: path is this test's own TempDir + require.NoError(t, readErr, "wrapper did not record the descendant's pid") + pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))) + require.NoError(t, convErr) + return err, pid +} + +// killIfAlive reaps a descendant the probe was expected to leave behind, or +// failed to kill. It is only ever called within seconds of the spawn, for a +// process that sleeps two minutes: one that kill(pid, 0) still finds is that +// process, not a recycled pid, so this can act only on our own. +func killIfAlive(pid int) { + if syscall.Kill(pid, 0) == nil { + _ = syscall.Kill(pid, syscall.SIGKILL) + } +} + +// TestRunCodexCommandOutlivingGrandchild pins two things ten minutes of a +// hung `basecamp doctor` proved were not being enforced: the deadline, and +// that nothing survives it. +// +// The stub above replaces runCodexCommand, so nothing else here exercises the +// real one. This does. It stands in for the shape codex actually ships as on +// some machines — a wrapper script that backgrounds a longer-lived process and +// exits at once — where the grandchild keeps the inherited stdout pipe open. +// The call has to return on its own deadline rather than the grandchild's, +// and the grandchild has to be dead when it does: the wrapper exited long +// before the deadline, so only a kill aimed at the process group reaches it. +func TestRunCodexCommandOutlivingGrandchild(t *testing.T) { + // The grandchild has to outlive the deadline by a wide margin, or the test + // passes on the sleep ending rather than on the kill working. + start := time.Now() + err, pid := codexWrapper(t, "sleep 120 & echo $! > PIDFILE; exit 0", 500*time.Millisecond) + // Only a failing run has a grandchild left to reap; a passing one has + // already seen it gone, and a pid seen gone is nobody's to signal. + t.Cleanup(func() { + if t.Failed() { + killIfAlive(pid) + } + }) + + assert.Less(t, time.Since(start), 30*time.Second, + "runCodexCommand blocked on a pipe held open by a surviving grandchild") + assert.ErrorIs(t, err, context.DeadlineExceeded) assert.Eventually(t, func() bool { return syscall.Kill(pid, 0) == syscall.ESRCH }, 5*time.Second, 50*time.Millisecond, "grandchild %d outlived the deadline: the process group was not killed", pid) } + +// TestRunCodexCommandEscapedDescendant covers the descendant a group kill +// cannot reach: one that started its own session and still holds the +// inherited stdout. The read has to give up on its own — the pipe is +// closed after codexWaitDelay — so the call returns on the deadline plus +// that grace, and the descendant is left alive, as documented. +func TestRunCodexCommandEscapedDescendant(t *testing.T) { + if _, err := exec.LookPath("setsid"); err != nil { + t.Skip("setsid not available") + } + + start := time.Now() + err, pid := codexWrapper(t, "setsid sleep 120 & echo $! > PIDFILE; exit 0", 500*time.Millisecond) + t.Cleanup(func() { killIfAlive(pid) }) + + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Less(t, time.Since(start), 10*time.Second, + "runCodexCommand waited on a pipe held by a descendant outside the group") + assert.NoError(t, syscall.Kill(pid, 0), "an escaped descendant is out of the group kill's reach by design") +} From e7b31e4b2ea99c66a7f6cfa35a4b7033d3c5bf93 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 9 Sep 2026 20:30:31 -0700 Subject: [PATCH 4/6] Return the pid before the error from the test wrapper revive's error-return rule wants the error last. The assertion message also says what a pid that is still found can be under a PID 1 that does not reap orphans: an uncollected zombie, not a survivor. --- internal/harness/codex_unix_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/harness/codex_unix_test.go b/internal/harness/codex_unix_test.go index b628ed9e..29d97311 100644 --- a/internal/harness/codex_unix_test.go +++ b/internal/harness/codex_unix_test.go @@ -19,8 +19,8 @@ import ( // codexWrapper runs script through sh as the probe's command, with a // deadline well short of the sleep the script backgrounds, and returns the -// error and the pid the script recorded. -func codexWrapper(t *testing.T, script string, deadline time.Duration) (error, int) { +// pid the script recorded and the error. +func codexWrapper(t *testing.T, script string, deadline time.Duration) (int, error) { t.Helper() sh, err := exec.LookPath("sh") if err != nil { @@ -47,7 +47,7 @@ func codexWrapper(t *testing.T, script string, deadline time.Duration) (error, i require.NoError(t, readErr, "wrapper did not record the descendant's pid") pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))) require.NoError(t, convErr) - return err, pid + return pid, err } // killIfAlive reaps a descendant the probe was expected to leave behind, or @@ -75,7 +75,7 @@ func TestRunCodexCommandOutlivingGrandchild(t *testing.T) { // The grandchild has to outlive the deadline by a wide margin, or the test // passes on the sleep ending rather than on the kill working. start := time.Now() - err, pid := codexWrapper(t, "sleep 120 & echo $! > PIDFILE; exit 0", 500*time.Millisecond) + pid, err := codexWrapper(t, "sleep 120 & echo $! > PIDFILE; exit 0", 500*time.Millisecond) // Only a failing run has a grandchild left to reap; a passing one has // already seen it gone, and a pid seen gone is nobody's to signal. t.Cleanup(func() { @@ -90,7 +90,7 @@ func TestRunCodexCommandOutlivingGrandchild(t *testing.T) { assert.Eventually(t, func() bool { return syscall.Kill(pid, 0) == syscall.ESRCH }, 5*time.Second, 50*time.Millisecond, - "grandchild %d outlived the deadline: the process group was not killed", pid) + "grandchild %d outlived the deadline: the process group was not killed (or, under a PID 1 that does not reap orphans, it is an uncollected zombie)", pid) } // TestRunCodexCommandEscapedDescendant covers the descendant a group kill @@ -104,7 +104,7 @@ func TestRunCodexCommandEscapedDescendant(t *testing.T) { } start := time.Now() - err, pid := codexWrapper(t, "setsid sleep 120 & echo $! > PIDFILE; exit 0", 500*time.Millisecond) + pid, err := codexWrapper(t, "setsid sleep 120 & echo $! > PIDFILE; exit 0", 500*time.Millisecond) t.Cleanup(func() { killIfAlive(pid) }) assert.ErrorIs(t, err, context.DeadlineExceeded) From 3f514e39b54cdeb9fb426dc34d3a2000d0c0afa0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 10 Sep 2026 12:00:36 -0700 Subject: [PATCH 5/6] Count a killed grandchild as terminated even where PID 1 leaves it a zombie MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The group-kill assertion polled kill(pid, 0) for ESRCH, which only arrives once whoever adopted the orphan has collected it. Under a PID 1 that never reaps — a container running go test as PID 1 — the kill works, the sleep is a zombie, and the test failed after five seconds anyway. terminated(pid) answers true on ESRCH or on a Z in /proc//stat, read after the last ')' so a comm with spaces cannot shift the field. Outside Linux there is no /proc and no init that leaves orphans uncollected, so ESRCH suffices. --- internal/harness/codex_unix_test.go | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/internal/harness/codex_unix_test.go b/internal/harness/codex_unix_test.go index 29d97311..a4fa270c 100644 --- a/internal/harness/codex_unix_test.go +++ b/internal/harness/codex_unix_test.go @@ -60,6 +60,26 @@ func killIfAlive(pid int) { } } +// terminated reports whether pid is gone, or is a zombie: killed, but not yet +// collected. Where this suite runs, whoever adopts the orphan reaps it at +// once, and kill(pid, 0) answers ESRCH. Under a PID 1 that does not reap — +// a container running go test as PID 1 — the group kill still worked, and +// the state field of /proc//stat is the only place that says so; it +// follows the parenthesized comm, which may itself hold spaces or parens, +// so the last ')' ends it. Outside Linux there is no /proc, and no init that +// leaves orphans uncollected. +func terminated(pid int) bool { + if syscall.Kill(pid, 0) == syscall.ESRCH { + return true + } + stat, err := os.ReadFile(filepath.Join("/proc", strconv.Itoa(pid), "stat")) //nolint:gosec // G304: /proc//stat for a pid this test spawned + if err != nil { + return false + } + fields := strings.Fields(string(stat)[strings.LastIndexByte(string(stat), ')')+1:]) + return len(fields) > 0 && fields[0] == "Z" +} + // TestRunCodexCommandOutlivingGrandchild pins two things ten minutes of a // hung `basecamp doctor` proved were not being enforced: the deadline, and // that nothing survives it. @@ -87,10 +107,8 @@ func TestRunCodexCommandOutlivingGrandchild(t *testing.T) { assert.Less(t, time.Since(start), 30*time.Second, "runCodexCommand blocked on a pipe held open by a surviving grandchild") assert.ErrorIs(t, err, context.DeadlineExceeded) - assert.Eventually(t, func() bool { - return syscall.Kill(pid, 0) == syscall.ESRCH - }, 5*time.Second, 50*time.Millisecond, - "grandchild %d outlived the deadline: the process group was not killed (or, under a PID 1 that does not reap orphans, it is an uncollected zombie)", pid) + assert.Eventually(t, func() bool { return terminated(pid) }, 5*time.Second, 50*time.Millisecond, + "grandchild %d outlived the deadline: the process group was not killed", pid) } // TestRunCodexCommandEscapedDescendant covers the descendant a group kill From 787d57d10d755221fc41ca67d71e2c44962034f0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 10 Sep 2026 12:05:37 -0700 Subject: [PATCH 6/6] Single-quote the pid file path the probe test hands to sh t.TempDir follows TMPDIR, so a temp root with a space or a shell metacharacter turned the unquoted redirect into a different command and the test failed before it reached the process cleanup it exists to check. The path is now single-quoted, with any embedded quote escaped. --- internal/harness/codex_unix_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/harness/codex_unix_test.go b/internal/harness/codex_unix_test.go index a4fa270c..db4fdbb3 100644 --- a/internal/harness/codex_unix_test.go +++ b/internal/harness/codex_unix_test.go @@ -27,7 +27,9 @@ func codexWrapper(t *testing.T, script string, deadline time.Duration) (int, err t.Skip("sh not available") } pidFile := filepath.Join(t.TempDir(), "descendant.pid") - script = strings.ReplaceAll(script, "PIDFILE", pidFile) + // TempDir follows TMPDIR, which may hold a space or a shell metacharacter, + // so the path goes into the script single-quoted. + script = strings.ReplaceAll(script, "PIDFILE", "'"+strings.ReplaceAll(pidFile, "'", `'\''`)+"'") ctx, cancel := context.WithTimeout(context.Background(), deadline) defer cancel()