fix(codex): reap subprocesses on early stream close - #4804
Conversation
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ 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. |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
I traced the early-close path through stdout iteration, generator finalization, stderr draining, and child reaping. Moving the live-child kill ahead of await stderr_task removes the deadlock: a consumer can close after receiving output even when the subprocess keeps stderr open indefinitely. The nested try/finally also preserves the important ownership invariant that process.wait() still runs if stderr draining raises, so the child is reaped rather than merely signalled.
The focused regression exercises the reachable failure mode at the async-generator boundary and checks both ordering (kill before stderr EOF) and final reaping. Normal completion remains unchanged because the kill is still conditional on returncode is None. I did not find a blocking issue in this change.
|
Thanks for the detailed review and for checking the early-close ordering and reaping invariant. I rechecked the current head and focused regression; no additional code change is needed from this review. I’ll leave the PR open for maintainer review. |
|
Follow-up validation: with uv 0.12.9, uv run --locked pytest tests/extensions/experiemental/codex/test_codex_exec_thread.py -q passes all 48 tests. The worktree remains clean and no source changes were needed for the review. |
Summary
This pull request fixes
CodexExec.run()hanging when a consumer closes its async output stream after receiving a line. The cleanup path previously awaited stderr EOF before killing a still-live Codex subprocess, even though that subprocess could keep stderr open indefinitely.The finalizer now kills a live child before draining stderr and always awaits
process.wait()so the child is reaped. A regression test covers the caller-visible early-close behavior without changing public APIs or normal stream semantics.Test plan
OPENAI_AGENTS_TEST_IN_CODEX_SANDBOX=1 UV_DEFAULT_INDEX=https://pypi.org/simple bash .agents/skills/code-change-verification/scripts/run.sh(format, lint, typecheck, and full tests pass with the repository CI's uv 0.11.14)uv run pytest -q tests/extensions/experiemental/codex/test_codex_exec_thread.py -k closes_live_process_before_draining_stderr(1 passed)Issue number
N/A — found through source inspection.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR