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".
| process.kill() | ||
| with contextlib.suppress(ProcessLookupError): | ||
| process.kill() | ||
| await _drain_remaining_stdout() |
There was a problem hiding this comment.
If the wrapped CLI spawns a helper process that inherits stdout and outlives the direct child, cancellation or early aclose() kills only the direct subprocess, but this unbounded drain waits for EOF from every remaining holder of the pipe. In that scenario cleanup hangs until the descendant exits, which preserves the cancellation deadlock this change is trying to fix; please bound the drain or ensure the whole process tree/stdout fd is closed before awaiting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks, good catch. Fixed in da45e04. Cleanup now bounds the combined stdout drain and process wait; if an outliving descendant keeps a pipe open past the deadline, it closes the subprocess transport and performs a second bounded wait. I also added a real subprocess regression where the direct child fills stdout while a helper inherits stdout and outlives it.
|
@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".
| if process.returncode is None: | ||
| process.kill() | ||
| with contextlib.suppress(ProcessLookupError): | ||
| process.kill() |
There was a problem hiding this comment.
When cancellation or aclose() happens after Codex has spawned a helper that inherited stdout/stderr, this only kills the immediate Codex process. Because the subprocess is not launched in an owned POSIX process group or Windows job/process group, _cleanup_process can return after closing the transport while the descendant continues running; the new descendant regression even has to kill helper_pid in its finally. Please make the executor own and terminate the process tree before returning from cleanup.
Useful? React with 👍 / 👎.
| process.kill() | ||
| with contextlib.suppress(ProcessLookupError): | ||
| process.kill() | ||
| await _cleanup_process() |
There was a problem hiding this comment.
If a caller cancels the stream and then applies its own timeout, or shutdown sends a second cancellation while this await is in progress, that second CancelledError interrupts _cleanup_process and skips the later stderr-task cleanup. In that repeated-cancellation path the child can remain unreaped or the drain task can survive, so cleanup should run in a shielded task and then re-raise the original cancellation or execution error.
Useful? React with 👍 / 👎.
|
@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".
| assert process.returncode is not None | ||
| for _ in range(500): | ||
| try: | ||
| os.kill(helper_pid, 0) |
There was a problem hiding this comment.
In Linux containers where PID 1 does not reap orphaned grandchildren, this os.kill(pid, 0) probe continues to succeed after the SIGKILL because the helper remains as a defunct zombie. The new test then spins for the full 5 seconds and fails even though no runnable descendant survived; either make the assertion tolerate zombie state or structure the child process to reap the helper before exiting.
Useful? React with 👍 / 👎.
|
@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".
| if original_error is not None or process.returncode is None: | ||
| try: | ||
| await _terminate_process_tree(process) |
There was a problem hiding this comment.
When the direct Codex CLI exits successfully after spawning a background helper that inherited stderr, process.returncode is already set here, so this branch skips _terminate_process_tree; the cleanup then cancels the stderr drain and run() returns while the helper keeps running. Since this change creates an owned POSIX process group for descendants, the cleanup path should also terminate that group before returning when descendants can still be holding subprocess pipes open.
Useful? React with 👍 / 👎.
|
@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. |
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