diff --git a/internal/harness/codex.go b/internal/harness/codex.go index e1e29860..488f9718 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,65 @@ 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.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. 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) + 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..c0235065 100644 --- a/internal/harness/codex_test.go +++ b/internal/harness/codex_test.go @@ -6,10 +6,7 @@ import ( "os" "os/exec" "path/filepath" - "strconv" - "strings" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -204,60 +201,3 @@ func boolJSON(value bool) string { } return "false" } - -// TestRunCodexCommandOutlivingGrandchild pins the deadline that ten minutes of -// a hung `basecamp doctor` proved was not being enforced. -// -// 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. -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 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`. - pidFile := filepath.Join(t.TempDir(), "grandchild.pid") - script := "sleep 120 & echo $! > " + pidFile + "; exit 0" - - t.Cleanup(func() { - raw, readErr := os.ReadFile(pidFile) //nolint:gosec // G304: path is this test's own TempDir - if readErr != nil { - return - } - pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))) - if convErr != nil { - return - } - 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{}) - start := time.Now() - go func() { - defer close(done) - _, _ = runCodexCommand(ctx, sh, "-c", script) - }() - - select { - case <-done: - // The call must return on its own deadline, not the grandchild's. - assert.Less(t, time.Since(start), 30*time.Second, - "runCodexCommand blocked on a pipe held open by a surviving grandchild") - case <-time.After(30 * time.Second): - t.Fatal("runCodexCommand did not return: WaitDelay is not bounding Wait") - } -} diff --git a/internal/harness/codex_unix_test.go b/internal/harness/codex_unix_test.go new file mode 100644 index 00000000..db4fdbb3 --- /dev/null +++ b/internal/harness/codex_unix_test.go @@ -0,0 +1,134 @@ +//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" +) + +// codexWrapper runs script through sh as the probe's command, with a +// deadline well short of the sleep the script backgrounds, and returns the +// 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 { + t.Skip("sh not available") + } + pidFile := filepath.Join(t.TempDir(), "descendant.pid") + // 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() + + done := make(chan error, 1) + go func() { + _, err := runCodexCommand(ctx, sh, "-c", script) + done <- err + }() + select { + case err = <-done: + case <-time.After(30 * time.Second): + t.Fatal("runCodexCommand did not return: the deadline is not bounding the call") + } + + 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 pid, err +} + +// 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) + } +} + +// 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. +// +// 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() + 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() { + 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 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 +// 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() + 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) + 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") +} 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) +}