fix: surface ClientSession transport faults - #3581
Steeve-Crypto wants to merge 1 commit into
Conversation
|
This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #1401. If a maintainer assigns you to #1401, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take. You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way. CONTRIBUTING.md has the full reasoning, but in short:
Maintainers: reopen, remove |
Fixes #1401
Summary
Transport-level
Exceptionitems on the client read stream were effectively silent in two places:_default_message_handler(and the mirrored default inClient) only awaited a checkpoint, so faults delivered via_deliver_stream_exceptionvanished unless a custommessage_handlerre-raised.JSONRPCDispatcher._dispatchobservedExceptionitems (or debug-logged them) but did not wake pendingsend_raw_requestwaiters — unlike the EOF path, which already fans outCONNECTION_CLOSEDvia_fan_out_closed.Fix
Exceptionitems so_deliver_stream_exceptionlogs them (message_handler raised on transport exception).Exceptionitem, fan outErrorData(code=CONNECTION_CLOSED, message="Transport error: …")to all pending waiters, then callon_stream_exceptionas before. The dispatcher is not marked closed (existing tests require the receive loop to keep serving after a single Exception item).Minimal and backwards-compatible: no new public APIs or error taxonomy.
Motivation and Context
Without this, in-flight client requests can hang when the transport yields an Exception (e.g. read timeout), which matches the production hang described on #1401.
How Has This Been Tested?
Result: 172 passed (including two new regression tests).
New coverage:
message_handlerre-raises a transport Exception and is logged via_deliver_stream_exception.send_raw_requestraisesMCPError(CONNECTION_CLOSED)promptly when an Exception arrives on the read stream; the loop still handles a later inbound request.Breaking Changes
None intended. Callers that previously hung on transport faults will now see
MCPError(CONNECTION_CLOSED)(or a logged re-raise from the default message handler). Custom handlers that already handledExceptionitems are unchanged.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
Issue #1401 is labeled
ready for work/good first issue/P1/bugbut is not assigned and does not carryhelp wanted. Per CONTRIBUTING, the require-linked-issue bot may auto-close this PR until a maintainer assigns the issue (or addshelp wanted). Branch left intact for reopen — please assign if this approach looks right.AI assistance: This change was prepared with AI assistance (implementation draft and test scaffolding). I reviewed the surrounding contracts (
_fan_out_closed, existing transport-exception tests) and can explain / adjust the patch.