Kill the Codex probe's process group when its deadline expires - #694
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Process reaping can race with the group-killing cancellation callback, and the new test does not compile portably.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents timed-out Codex diagnostics from leaking descendant processes by managing process groups and stdout explicitly.
Changes:
- Adds platform-specific process-group lifecycle helpers.
- Bounds stdout reads and returns context deadline errors consistently.
- Expands timeout testing to verify descendant termination.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/harness/procgroup_unix.go |
Adds Unix process-group creation and termination. |
internal/harness/procgroup_other.go |
Adds non-Unix fallback behavior. |
internal/harness/codex.go |
Implements explicit command, cancellation, and pipe lifecycle handling. |
internal/harness/codex_test.go |
Verifies timeout errors and descendant termination. |
Review details
Suppressed comments (1)
internal/harness/codex.go:73
- The new test only backgrounds a child that remains in the wrapper's process group, so the group kill closes its pipe immediately and this timeout/
Closefallback is never exercised. A regression here would still pass while allowing asetsiddescendant to hang the probe—the failure mode this branch explicitly handles. Add a Unix-specific test whose descendant leaves the group while retaining stdout and verify that the call remains bounded (with explicit cleanup).
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()
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Test cleanup can kill a recycled PID, and the escaped-descendant timeout path remains untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The regression test assumes prompt orphan reaping and the PR description overstates process-group cancellation behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/harness/codex.go:58
- The implementation here contradicts the PR description's claim that
cmd.Cancelkills the process group when the deadline expires while the leader is running: after stdout reaches EOF, execution entersWait, whose default cancellation kills only the direct child, as this comment states. A leader that closes stdout and then lingers can therefore still leave descendants behind at timeout. Since the narrower behavior was intentional after the PID-reuse fix, please update the PR description/title-level guarantee to describe this limitation rather than promising a group kill for every deadline.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The lifecycle ordering addresses PID-reuse safety, preserves timeout behavior, and is covered across the relevant Unix edge cases.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3930390ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
…ix alone 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.
…he 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.
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.
…zombie 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/<pid>/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.
c393039 to
3f514e3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The Unix test breaks when the temporary directory path requires shell quoting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
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.
There was a problem hiding this comment.
🔵 Needs a closer look
The regression test’s timeout path can leave its long-running descendant behind.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/harness/codex_unix_test.go:45
- If this timeout branch catches a regression,
codexWrappercallst.Fatalbefore returning the recorded PID, so neither caller gets to register its cleanup and the intentionally long-lived descendant remains running for up to two minutes. Read the PID file and kill that known descendant before aborting the helper.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Not doing this — re the suppressed note on codex_unix_test.go:45: true that the 30-second guard calls t.Fatal before the pid is handed back, so on that path nothing registers a cleanup. What is left behind is a bare |
…endor-hash * origin/main: ci: bump zizmorcore/zizmor-action in the github-actions group (#698) Reference in-repo workflows with GitHub's self-repository syntax (#700) Kill the Codex probe's process group when its deadline expires (#694) Refresh the Nix vendorHash for the go-dependencies bump (#701)
queryCodexPluginrancodex plugin list --available --jsonthroughcmd.Outputwith aWaitDelay. That bounded the call (#629) but not what it left behind: wherecodexis a wrapper that exits at once and backgrounds the real work (an npm exec launcher, a mise shim), the direct child was gone long before the five-second deadline, so the exec package had stopped watching the context,cmd.Cancelnever ran, and the descendant holding the inherited stdout outlived every timed-outdoctor— one resident process per run. #630 records why the obviousSetpgid+cmd.Cancelattempt could not work.What
runCodexCommandnow owns the lifecycle instead ofOutput():SysProcAttr{Setpgid: true},procgroup_unix.go; a no-op with plainProcess.Killelsewhere).StdoutPipein our own goroutine. Onctx.Donethe whole group is killed beforeWaitis called — the group ID is the leader's PID and stays reserved only while a member of the group exists, so a kill issued before the leader is reaped can never land on a recycled PID, which is the safety property The Codex probe leaks a descendant when it times out #630 asked for. There is nocmd.Canceloverride: onceWaithas begun, a leader that closed stdout and lingers past the deadline is killed alone by the exec package's own cancel, and its descendants are out of reach in that shape — a group kill issued fromCancelwould race the reap and could land on a recycled ID.WaitDelaystays, and the read has its own bound: a descendant that leaves the group (setsid) is out of reach, so aftercodexWaitDelayour end of the pipe is closed and the read returns.ctx.Err()regardless of the exit status, socodexQueryFailurekeeps rendering "Cannot query Codex plugins" with the same hint.Verification
TestRunCodexCommandOutlivingGrandchildnow also asserts the grandchild is gone after the call returns (polled onkill(pid, 0)→ESRCH, 5 s bound) and that the error iscontext.DeadlineExceeded. Againstmain'scodex.goit fails withgrandchild <pid> outlived the deadline: the process group was not killed; with this change it passes in 0.5 s. Cleanup reaps the grandchild only on a failing run, and only ifkill(pid, 0)still finds it, so a passing run never signals a pid it has already watched disappear.GOOS=windows go build ./internal/harnesscompiles the stub side.bin/cigreen on Linux (thelio, Go 1.26.7): fmt, vet, lint, unit, e2e, naming, surface, skill drift, bare groups, provenance, tidy.Fixes #630
Summary by cubic
Kills the Codex probe's process group when its deadline expires, so timed-out
doctorruns no longer leave orphaned processes holding the stdout pipe. Fixes #630.queryCodexPluginpreviously usedcmd.Outputwith aWaitDelay, which bounded the call but not the descendants that outlived it.runCodexCommandnow startscodexin its own process group, reads stdout directly, and kills the whole group on context expiry.Bug Fixes
Waitreaps the leader, so it can never land on a recycled PID.WaitDelaystill bounds reads from descendants that escape the group (setsid); non-Unix platforms kill only the child.ctx.Err(), keeping the "Cannot query Codex plugins" failure and hint unchanged.Written for commit 787d57d. Summary will update on new commits.