fix(mcp): reconnect message-less closed transports - #66547
Conversation
Related to #65111: both address closed MCP transports, while this PR additionally handles nested AnyIO exception groups and reconnect lifecycle/breaker reset behavior. The overlapping recovery paths need maintainer consolidation. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for covering the message-less AnyIO path. Current main still rejects empty exception text in tools/mcp_tool.py:3553-3562, while the handler reaches session recovery through tools/mcp_tool.py:4299-4304, so the premise is valid.
Problems
tools/mcp_tool.py:3587recursively classifies nested exceptions without the cycle/depth guard used by the new interruption helper. The new cycle test only exercises_exception_tree_contains_interruption(tests/tools/test_mcp_tool_session_expired.py:142-154), so a cyclic or deeply nested.exceptionsgraph can still recurse during classification.- This overlaps with #65111, as the maintainer comment notes. Its proposed classifier also covers
BrokenPipeErrorandEOFError; consolidate the transport-type coverage rather than retaining parallel recovery classifiers.
Suggested changes
- Use one iterative, identity-tracked exception-tree traversal for interruption precedence and transport-leaf detection; add classifier-level deep/cyclic tests.
- Fold in #65111's built-in pipe/EOF coverage while preserving the existing text fallback and one-retry contract.
Automated hermes-sweeper review.
| # AnyIO task groups can wrap the transport exception in an | ||
| # ExceptionGroup whose own string omits the message-less leaf details. | ||
| nested = getattr(exc, "exceptions", ()) | ||
| if nested and any(_is_session_expired_error(child) for child in nested): |
There was a problem hiding this comment.
This recursive walk does not share the cycle/depth protection added in _exception_tree_contains_interruption. A cyclic or deeply nested .exceptions graph can recurse here before classification; traverse the exception graph iteratively with an identity-based visited set, and add a classifier-level regression test.
Builds on diffen77's #66547 (cherry-picked as the previous commits). Extend _is_session_expired_error's iterative traversal to follow __cause__/__context__ in addition to ExceptionGroup .exceptions — SDK wrappers often raise a generic RuntimeError *from* the message-less ClosedResourceError, leaving the transport signal reachable only via the chain. The identity-visited set guards chain cycles (handlers re-raising previously seen exceptions), and a bounded node budget (_EXC_TRAVERSAL_MAX_NODES) caps pathological acyclic graphs. Adds regression tests: cause/context chain detection, interruption precedence through chains, cyclic cause/context termination, and budget-bounded termination.
Builds on diffen77's #66547 (cherry-picked as the previous commits). Extend _is_session_expired_error's iterative traversal to follow __cause__/__context__ in addition to ExceptionGroup .exceptions — SDK wrappers often raise a generic RuntimeError *from* the message-less ClosedResourceError, leaving the transport signal reachable only via the chain. The identity-visited set guards chain cycles (handlers re-raising previously seen exceptions), and a bounded node budget (_EXC_TRAVERSAL_MAX_NODES) caps pathological acyclic graphs. Adds regression tests: cause/context chain detection, interruption precedence through chains, cyclic cause/context termination, and budget-bounded termination.
Builds on diffen77's #66547 (cherry-picked as the previous commits). Extend _is_session_expired_error's iterative traversal to follow __cause__/__context__ in addition to ExceptionGroup .exceptions — SDK wrappers often raise a generic RuntimeError *from* the message-less ClosedResourceError, leaving the transport signal reachable only via the chain. The identity-visited set guards chain cycles (handlers re-raising previously seen exceptions), and a bounded node budget (_EXC_TRAVERSAL_MAX_NODES) caps pathological acyclic graphs. Adds regression tests: cause/context chain detection, interruption precedence through chains, cyclic cause/context termination, and budget-bounded termination.
|
Merged via #68660. Your message-less closed-transport classification was cherry-picked (4 commits, authorship preserved); we made the cause/context traversal cycle-guarded on top. |
For the NousResearch#66547 and NousResearch#66981 salvage cherry-picks in this branch.
Builds on diffen77's NousResearch#66547 (cherry-picked as the previous commits). Extend _is_session_expired_error's iterative traversal to follow __cause__/__context__ in addition to ExceptionGroup .exceptions — SDK wrappers often raise a generic RuntimeError *from* the message-less ClosedResourceError, leaving the transport signal reachable only via the chain. The identity-visited set guards chain cycles (handlers re-raising previously seen exceptions), and a bounded node budget (_EXC_TRAVERSAL_MAX_NODES) caps pathological acyclic graphs. Adds regression tests: cause/context chain detection, interruption precedence through chains, cyclic cause/context termination, and budget-bounded termination.
For the NousResearch#66547 and NousResearch#66981 salvage cherry-picks in this branch.
Builds on diffen77's NousResearch#66547 (cherry-picked as the previous commits). Extend _is_session_expired_error's iterative traversal to follow __cause__/__context__ in addition to ExceptionGroup .exceptions — SDK wrappers often raise a generic RuntimeError *from* the message-less ClosedResourceError, leaving the transport signal reachable only via the chain. The identity-visited set guards chain cycles (handlers re-raising previously seen exceptions), and a bounded node budget (_EXC_TRAVERSAL_MAX_NODES) caps pathological acyclic graphs. Adds regression tests: cause/context chain detection, interruption precedence through chains, cyclic cause/context termination, and budget-bounded termination.
Summary
ClosedResourceError,BrokenResourceError, andEndOfStreamfailures (including nested exception groups) as stale MCP transportsMCPServerTask.run()lifecycle for both stdio and Streamable HTTP configurationsThis is a follow-up to #13383: the existing text-based recovery handles explicit "expired session" responses, but AnyIO transport-close exceptions are commonly message-less and therefore bypassed that classifier.
Verification
scripts/run_tests.sh tests/tools/test_mcp_tool_session_expired.py tests/tools/test_mcp_circuit_breaker.py tests/tools/test_mcp_reconnect_signal.py tests/tools/test_mcp_reconnect_retry_reset.py -j 4— 37 passedscripts/run_tests.sh -j 8 <all tests/tools/test_mcp*.py + test_refresh_agent_mcp_tools.py + tests/tui_gateway/*.py + tests/test_tui_gateway*.py + test_tui_mcp_late_refresh.py>— 77 files, 1426 passedruff check .— passedgit diff --check— passedRecovery contract reviewed