fix(stdio): resolve BrokenResourceError race condition on quick exit - #2079
Closed
gspeter-max wants to merge 4 commits into
Closed
fix(stdio): resolve BrokenResourceError race condition on quick exit#2079gspeter-max wants to merge 4 commits into
gspeter-max wants to merge 4 commits into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
ROOT CAUSE: The stdio_client async context manager had a race condition when exiting quickly (before subprocess finished outputting data). The cleanup code in the finally block closed memory streams while background tasks (stdout_reader and stdin_writer) were still using them, resulting in BrokenResourceError. Timeline of the bug: 1. User code exits the async with stdio_client(...) context 2. The finally block executes 3. Streams are closed immediately 4. Background tasks are still running and trying to send/receive data 5. Tasks encounter closed streams → BrokenResourceError CHANGES: 1. Added BrokenResourceError to exception handlers in both client and server - src/mcp/client/stdio.py: Lines 161, 177 (stdout_reader, stdin_writer) - src/mcp/server/stdio.py: Lines 67, 77 (stdin_reader, stdout_writer) - This allows tasks to exit gracefully if streams close during operation 2. Added task cancellation before stream closure in client transport - src/mcp/client/stdio.py: Line 210 (after process cleanup) - tg.cancel_scope.cancel() sends cancellation signal to background tasks - Tasks receive signal and finish their current operation - Then streams are closed (tasks aren't using them anymore) 3. Added regression test - tests/client/test_stdio.py: test_stdio_client_quick_exit_race_condition - Verifies that quick context exits don't cause ExceptionGroup crashes IMPACT: - No more ExceptionGroup crashes when exiting quickly - Graceful task shutdown with proper cancellation - Backward compatible - all existing tests pass - Better resource cleanup - tasks finish before streams close TECHNICAL NOTES: - Server transport only needed exception handler changes (not task cancellation) because it doesn't manage subprocess lifecycle - The fix uses defense-in-depth: both proper coordination AND graceful handling - anyio.BrokenResourceError is raised when operations are attempted on closed resources, distinct from ClosedResourceError (resource already closed) FILES MODIFIED: - src/mcp/client/stdio.py - src/mcp/server/stdio.py - tests/client/test_stdio.py
Pyright reports error when variables are assigned but never used. Changed (read_stream, write_stream) to (_, _) to indicate these are intentionally unused in the race condition test.
ROOT CAUSE: Manual tg.cancel_scope.cancel() was interfering with process cleanup in the async with block, causing CancelledError and ProcessLookupError during process termination. CHANGES: - Removed tg.cancel_scope.cancel() call from finally block - The async with block already handles task cancellation when exiting IMPACT: - Fixes test_stdio_client_sigint_only_process failure - Process cleanup now completes without interference - Background tasks still properly cancelled by task group exit FILES MODIFIED: - src/mcp/client/stdio.py
Copy link
Copy Markdown
Contributor
|
Thanks for the PR! Per our CONTRIBUTING.md, PRs require a corresponding issue with the ready for work label. Closing — note #2268 also addresses this area. |
Sorry, something went wrong.
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
3 participants
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.
ROOT CAUSE:
The stdio_client async context manager had a race condition when exiting quickly (before subprocess finished outputting data). The cleanup code in the finally block closed memory streams while background tasks (stdout_reader and stdin_writer) were still using them, resulting in BrokenResourceError.
Timeline of the bug:
CHANGES:
Added BrokenResourceError to exception handlers in both client and server
Added task cancellation before stream closure in client transport
Added regression test
IMPACT:
TECHNICAL NOTES:
FILES MODIFIED:
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context