fix(mcp): detect TimeoutError as stale session in StreamableHTTP transport - #17662
fix(mcp): detect TimeoutError as stale session in StreamableHTTP transport#17662lsl9119 wants to merge 1 commit into
Conversation
…sport When a StreamableHTTP MCP server garbage-collects its server-side session after the GET SSE stream disconnects, subsequent tool calls on the stale session hang until the 120s tool-call timeout fires. TimeoutError has an empty str(), so _is_session_expired_error() returned False, and the transport reconnect path was never triggered. This fix treats TimeoutError as a session-expired signal, triggering the existing transport reconnect and single-retry path. Root cause: actual-mcp-server closes idle GET SSE streams after ~60s. The python-sdk transport reconnects with a fresh session ID, but tool calls on the old session hang indefinitely. Direct HTTP POSTs work fine. Tested: 17/17 session-expired tests pass (including new TimeoutError test).
|
Thanks for tracing the reconnect path. The stale-session recovery gap remains on current main: Problems
Suggested changes
Automated hermes-sweeper review. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Five PRs address the MCP stale-session recovery path: #15125 provides the merged reconnect-and-retry implementation, #13795 and most of #19208 overlap its current-main successor, while #17662 and #66841 propose additional timeout and INVALID_PARAMS classifiers that remain insufficiently discriminating.
Related pull requests
- #13795 [closed]
related— (+510/-47) — implemented on main: The diff separates session-identity and dead-transport errors, including “Session terminated,” and routes tool, resource, and prompt operations through one reconnect plus one retry. Although closed, it remains relevant as a broader reference implementation now covered on main and shipped in v2026.5.7. - #15125 [merged]
related— (+540/-4) — merged reference implementation: The stale-session portion adds narrow message-based detection and reconnects all five MCP operation paths once, directly addressing expired server-side StreamableHTTP sessions; the PR also contains unrelated OAuth serialization, callback hardening, and redirect-security fixes. - #17662
related— (+26/-1) — needs revision: Despite the keep_open review on #17662, the diff classifies every builtin TimeoutError as stale-session evidence, including ordinary configured tool-deadline expirations, and would therefore reconnect and repeat potentially slow calls without distinguishing a dead transport. Preserve the reported timeout gap, but add a transport-specific discriminator and handler-level positive/negative retry coverage before merging. - #19208
related— (+334/-28) — partially superseded; salvage narrowly: Despite the keep_open review on #19208, the session-replacement wait and “Session terminated” recovery are already present in newer shared reconnect plumbing on main. The breaker reset is still useful, but the diff incorrectly converts every retry JSON payload containing an error key—including ordinary tool errors—into stale-transport failure, so only a focused _reset_server_error change should be retained. - #66841
related— (+101/-1) — needs revision: Despite the keep_open review on #66841, the revised diff still classifies the exact generic McpError(-32602, “Invalid request parameters”) as stale-session evidence, although the same structural code and message can represent an ordinary JSON-RPC validation failure. The deployed-server repro is relevant, but the classifier needs an additional transport/session discriminator and a negative handler-level no-reconnect test.
Duplicates
#13795 and the stale-session/rebuilt-session portion of #19208 substantially overlap the reconnect-and-retry behavior merged via #15125 and subsequently strengthened on current main; #17662 and #66841 are distinct proposed classifier extensions, not duplicates of each other.
Suggested consolidation
Do not merge #17662, #19208, or #66841 as currently written. Treat merged #15125 and current main as the consolidated recovery implementation; keep #13795 closed as implemented-on-main, reduce #19208 to the focused full breaker-state reset, and require #17662 and #66841 to prove transport-specific predicates plus handler-level non-retry regressions before either classifier extension is eligible to merge.
Cross-PR triage: Reviewed 5 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 80 kB of PR diffs, 14 kB of issue/PR text, 6 kB of discussion (7 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Problem
MCP tools connected via StreamableHTTP transport (e.g.
actual-mcp-server) consistently time out with an empty error message ("call failed: "), even though direct HTTP POSTs to the same endpoint work fine.Root cause: When the server closes the GET SSE long-poll stream after idle timeout (
~60s), the python-sdk transport reconnects with a new session ID. But tool calls queued on the old session hang until the 120stool_timeoutfires.TimeoutErrorhas an emptystr(), so_is_session_expired_error()returnsFalse, and the transport reconnect path is never triggered.Evidence from agent.log
Fix
_is_session_expired_error()now returnsTrueforTimeoutError, triggering the existing_handle_session_expired_and_retry()path (transport reconnect + single retry). This is the same mechanism already used for"Invalid or expired session"text markers.Changes
tools/mcp_tool.py: Addisinstance(exc, TimeoutError)check before the text-marker matchingtests/tools/test_mcp_tool_session_expired.py: Addedtest_is_session_expired_detects_timeout_error(). All 17 tests pass.What this fixes
Testing
pytest tests/tools/test_mcp_tool_session_expired.py -v # 17 passed in 3.01sThe fix is narrow: it only affects
_is_session_expired_error()which is guarded by_handle_session_expired_and_retry()— a path that already tears down and rebuilds the transport. No new failure modes are introduced.