Repository navigation
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_AppServerSession.close()closes stdout before terminating its process. If the background reader is blocked reading that pipe and the server remains alive, closing the text stream waits for the reader's lock. Shutdown never reachesterminate()or its timeout/kill fallback.This also prevents
_run_app_server()from returning its request timeout: itsfinallyblock gets stuck insession.close(). Reproduced using a real local Python child that announces readiness then sleeps without reading stdin. The sleeping child and blocked reader are always rescued and reaped by the regression test cleanup.Change
Keep closing stdin first, then terminate and wait for the child, escalating to kill if needed. Wait after kill as well to reap it. Close stdout/stderr only after that process shutdown sequence, so a reader waiting for child output can reach EOF.
Add real-process regressions for direct close, request-timeout cleanup, and a child ignoring SIGTERM. The last case exercises the existing five-second grace period and forced kill/reap on POSIX. The two other cases are not POSIX-specific.
Validation
Python 3.12.13, macOS arm64, main
cb49662150aa794149d477379c605a3bff0c08a2:OPENAI_API_KEY=test-key-for-unit-tests python -m unittest discover tests: 1,006 tests run, 2 skipped; all remaining tests pass.git diff --check: pass.Tests launch only the running Python interpreter. No real Codex process, account, model call, or user workspace modification is involved. Native Windows was not run; the SIGTERM-ignore regression skips on non-POSIX systems.
This does not change JSON-RPC buffering/auth/sandbox handling in #558 or text encoding in #577; both patches were checked and do not change shutdown ordering.
Prepared with AI assistance from source review, with actual process and timeout-path verification.