Fix pending control requests hanging for the full timeout after close()#1095
Fix pending control requests hanging for the full timeout after close()#1095JoshKappler wants to merge 1 commit into
Conversation
close() cancels the read task, which raises the cancelled exception and skips the `except Exception` bulk-fail branch added in anthropics#388. Nothing else signals the in-flight control requests, so an interrupt() awaiting a response when the transport closes waited out the full 60s timeout for a control_response that could never arrive. Signal the unresolved pending requests from the reader's `finally` block instead, which runs on every exit path. Requests the except branch already resolved keep their richer error, and a response read just before EOF keeps its result.
tapheret2
left a comment
tapheret2
left a comment
There was a problem hiding this comment.
Review notes on “Fix pending control requests hanging for the full timeout after close()”
Touched files (2): src/claude_agent_sdk/_internal/query.py, tests/test_query.py
Diff size (approx): +120 / -2
Observations
- Nice that tests/fixtures are included with the change — that helps a lot for review confidence.
Questions / nits
- Does this need a changelog / release note entry for the target version?
- Any user-facing behavior change that should be called out in the PR body?
Thanks @JoshKappler for the contribution — left this as an independent review pass.
|
On the changelog: CHANGELOG.md here is generated at release time by the build-and-publish workflow, and no merged contributor PR edits it, so I left it out. Happy to add an entry if maintainers want one. On behavior: the only user-visible change is a path that used to hang. A control request still awaiting a response when the transport closes now raises CLIConnectionError immediately instead of blocking for the full 60s timeout. Callers that never hit close() mid-request see no difference. Still merges clean with main. test_query.py is 43 passed, ruff and mypy clean. |
2 participants
Fixes #1094.
close() cancels the read task. The cancelled exception is a BaseException, so it skips the
except Exceptionbulk-fail branch that #388 added for the CLI-crash case, and nothing else signals the in-flight outgoing control requests. An interrupt() still waiting on a control_response when the transport closes blocks for the full 60s timeout waiting for a response that can never arrive.Moved the signal into the reader's
finally, which runs on every exit path. The loop skips any request_id already in pending_control_results, so a request the except branch resolved keeps its ProcessError, and a control_response read just before EOF keeps its result. Event.set() never awaits, so the loop is safe under trio's level-triggered cancellation, which is the same constraint as the send_nowait call already in that block.Tests: added TestPendingControlRequestsOnClose, three tests, run on both asyncio and trio. Reverting just the query.py change fails four of the six; the two that still pass are the already-resolved guard, which is meant to pass either way. Full suite: 1040 passed, 15 failed, and those 15 fail the same way on a clean main (Windows path assertions in test_sessions, test_session_import, test_session_mutations). ruff check, ruff format --check, and mypy src/ are clean.