Fix stdio_client BrokenResourceError race condition during shutdown - #2268
Fix stdio_client BrokenResourceError race condition during shutdown#2268weiguangli-io wants to merge 1 commit into
Conversation
Sorry, something went wrong.
During shutdown, the finally block closes the receiving end of the read_stream memory channel while stdout_reader may still be blocked on send(). This causes BrokenResourceError (not ClosedResourceError) because the *other* end of the stream was closed. The existing except clause only caught ClosedResourceError, letting BrokenResourceError propagate into an ExceptionGroup. Catch BrokenResourceError alongside ClosedResourceError in both stdout_reader and stdin_writer to handle this race gracefully. Closes modelcontextprotocol#1960 Github-Issue:modelcontextprotocol#1960 Reported-by:maxisbey
Bortlesboat
left a comment
There was a problem hiding this comment.
ACK. Clean minimal fix — BrokenResourceError is the correct counterpart to catch when the receiving end closes under a pending send().
Sorry, something went wrong.
| session_message = SessionMessage(message) | ||
| await read_stream_writer.send(session_message) | ||
| except anyio.ClosedResourceError: # pragma: lax no cover | ||
| except (anyio.ClosedResourceError, anyio.BrokenResourceError): # pragma: lax no cover |
There was a problem hiding this comment.
Noting for future readers: the SSE client (sse.py) avoids this class of race by calling tg.cancel_scope.cancel() in its finally block, which cancels tasks before closing their streams. The stdio client takes a different approach — letting stream closure propagate to tasks and catching the resulting exceptions. Both are valid; this PR correctly handles the full set of exceptions that anyio can raise in this scenario.
Sorry, something went wrong.
|
This would also resolve the semaphore leak reported in openai/openai-agents-python#618 — the SDK currently works around it by pre-closing the write stream before exit stack teardown (openai/openai-agents-python#2802), but that reaches into BaseSession._write_stream which is a private attribute. A proper fix here would let the SDK drop that workaround. Happy to help test if useful. |
Sorry, something went wrong.
|
Friendly ping — CI is green. Would appreciate a review when you get a chance. Thanks! |
Sorry, something went wrong.
Summary
Fixes #1960. Supersedes #2219 (which was closed).
During stdio_client shutdown, the finally block closes read_stream (the receiving end of a zero-buffer memory channel) while stdout_reader may still be blocked on send(). When the receiving end closes underneath a pending send(), anyio raises BrokenResourceError — not ClosedResourceError. The existing except clause only caught ClosedResourceError, so BrokenResourceError escaped into an ExceptionGroup.
The fix catches BrokenResourceError alongside ClosedResourceError in both stdout_reader and stdin_writer. This is the minimal, correct change: ClosedResourceError means "you called send on a handle you already closed", while BrokenResourceError means "the other end was closed" — both are expected during shutdown and should be handled identically.
Test plan