fix(codex): clean up subprocess on cancellation#3915
Conversation
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the fix. The original cancellation deadlock is real, but the current cleanup can still hang when stdout has unread buffered data: after process.kill(), await process.wait() blocks if the stdout transport has paused due to backpressure, even though the child has already exited.
Please drain the remaining stdout in bounded chunks before awaiting process.wait(), and add a regression test using a real subprocess that fills stdout beyond the stream-reader limit before the stream is closed. The existing fake-process test cannot reproduce this pipe-level deadlock. I confirmed that draining stdout first resolves the issue while preserving the existing cancellation and stderr-cleanup behavior.
53c8fb6 to
8e7af59
Compare
|
Thanks for catching this — you were right that waiting for the process before draining a paused stdout transport can still deadlock. I've updated the cleanup path to cancel any in-flight timeout reader, kill the subprocess, drain the remaining stdout in 64 KiB chunks, and only then await The existing cancellation and stderr-cleanup coverage remains in place, and the full verification stack passes. |
![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: 8e7af5931a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@seratch I addressed the requested stdout backpressure case and the follow-up bot finding in da45e04. Cleanup is now bounded even when an outliving descendant keeps stdout open, with a real-subprocess regression covering that case. Codex review and security review on the latest head are both clean. When you have a chance, could you please take another look? Thanks! |
|
Thanks for addressing the stdout backpressure deadlock. A deeper lifecycle review found two remaining merge blockers. |
|
Addressed both remaining lifecycle blockers in c004807:
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da45e04d40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review Please review commit c004807 specifically: shiwenbin1617@c004807 The fork branch already points to this commit, while the PR head synchronization is delayed by the ongoing GitHub Pull Requests/Webhooks incident. |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0048076d5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 378c14f764
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@seratch Thank you for the careful review and for pointing out the deeper lifecycle cases. Thank you as well to Codex for the follow-up checks. I’m sorry for the extra review notifications earlier—the GitHub Pull Requests/Webhooks incident caused two reviews to run against a stale head. I believe the remaining blockers are now addressed on b6bd302: cleanup owns and terminates descendants across cancellation, errors, repeated cancellation, and clean parent exit, with focused real-subprocess coverage. Codex code and security reviews on the current head are clean. When you have time, could you please take another look? I appreciate your patience and detailed guidance. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 7552c7204c
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| def _terminate_windows_process_tree(pid: int) -> None: | ||
| with contextlib.suppress(OSError, subprocess.TimeoutExpired): | ||
| subprocess.run( | ||
| ["taskkill", "/PID", str(pid), "/T", "/F"], |
There was a problem hiding this comment.
On Windows this cleanup helper runs taskkill by bare name. If a Codex-enabled app runs from a workspace the model can write to, an attacker can place taskkill.exe there and then trigger cleanup/cancellation, causing the SDK process to execute that binary outside the Codex sandbox. Use an absolute trusted System32 path or a Win32 job/process-tree API instead of PATH/current-directory lookup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7552c7204c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Wait for the direct process independently of pipes inherited by descendants. | ||
| await _wait_for_direct_process() |
There was a problem hiding this comment.
When the direct Codex process exits after emitting its last event but a descendant keeps stdout inherited/open, the next stdout.readline() never returns EOF, so execution never reaches this direct-exit wait or the finally cleanup that kills the process group. I reproduced this with a subprocess that prints one line, spawns a sleeping child with stdout=sys.stdout, and exits; the parent returncode was already 0 while the second anext() hung. Race the direct-exit event with the stdout read, or trigger cleanup once the direct process exits, so clean exits with inherited stdout do not hang indefinitely.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 8b1c969db2
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| with contextlib.suppress(ProcessLookupError): | ||
| os.killpg(pid, signal.SIGKILL) |
There was a problem hiding this comment.
The new cleanup only kills the subprocess' original POSIX process group. A Codex-run command can fork and call setsid()/setpgrp() before cancellation; that descendant leaves pgid pid, so stream.aclose() or an idle timeout returns while it keeps running with the configured workspace/environment. If cancellation is meant to revoke Codex execution, use a containment primitive that covers reparented descendants, or document this as best-effort and enforce hard cleanup in the sandbox/CLI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks — this is a valid limitation. The SDK-level cleanup here is best-effort rather than a security boundary: on POSIX, a descendant that deliberately creates a new session can escape the owned process group, and covering that portably would require an OS-specific containment primitive owned by the Codex CLI or sandbox rather than recursive process scanning in the SDK. This PR will remain scoped to terminating the Codex CLI and ordinary, non-detached descendants during cancellation or stream closure. I’ll leave the implementation unchanged and defer hard containment to the CLI/sandbox layer.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
2 participants
Summary
This pull request fixes a subprocess cleanup deadlock in the experimental Codex executor.
When the async output stream was cancelled or closed early, cleanup waited for the stderr drain task before terminating the subprocess. Since stderr remained open while the process was running, cancellation could hang indefinitely and leave the child process unreaped.
The cleanup path now:
try/finally;asyncio.CancelledErrorsemantics.A deterministic regression test verifies that cancellation completes promptly, the subprocess is killed and reaped, and the stderr task is cancelled.
Test plan
bash .agents/skills/code-change-verification/scripts/run.shpytest -q tests/extensions/experiemental/codex/test_codex_exec_thread.py -k codex_exec_run-9.Issue number
N/A
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR