Kill the Codex probe's process group when its deadline expires

internal/harness/codex.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -25,27 +26,74 @@ 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
 )
 
 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")

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

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

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

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