fix(mcp): bound tools/list drains and recover from wedged transports - #73377
fix(mcp): bound tools/list drains and recover from wedged transports#73377luijoc wants to merge 1 commit into
Conversation
|
Thanks for the focused MCP recovery fix. The premise remains present on current main: The proposed bounds target the two setup/refresh paths that lack a normal caller deadline. Resource and prompt list drains already execute through handlers whose The PR base predates substantial MCP work, but the target functions and surrounding control flow remain materially compatible on current main; salvage should be mostly mechanical. Automated hermes-sweeper review. |
An SSE server that answered `initialize` on a keepalive-triggered reconnect and then never responded to `tools/list` wedged every tool call on that server for 40h. `_discover_tools` awaited the response forever while holding `_rpc_lock`, and it runs after `self.session = session` but before `_ready.set()` and `_wait_for_lifecycle_event()` — so no keepalive watchdog was armed, the dead-session handler path never ran (session was non-None), and the circuit breaker's half-open probe just re-blocked on the same lock. - Pass `read_timeout_seconds` when building `ClientSession` (stdio, SSE, streamable HTTP). Without it the SDK awaits every response on `anyio.fail_after(None)`, so any unwrapped request can park forever. - Bound both `tools/list` drains with `connect_timeout` so discovery doesn't inherit a deliberately generous per-tool `timeout`. Discovery lets the timeout propagate into run()'s existing teardown + backoff; the notification-driven refresh logs and requests a reconnect. - Ask for a transport rebuild when consecutive tool calls time out at the circuit-breaker threshold: a call that never came back is a liveness failure, and nothing else in that state asks for a rebuild. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XD53FVxrhZaLDfXgdtRHvm
66c3404 to
5af0be4
Compare
What does this PR do?
An MCP server whose transport hangs (rather than errors) could wedge every
tool call on that server permanently. We hit this in production: an SSE server
behind a gateway answered
initializeon a keepalive-triggered reconnect, thennever delivered a response to
tools/list. Tool calls on that server were deadfor 40 hours until the process was restarted.
The mechanism:
_discover_tools()awaitstools/listwhile holding theserver's
_rpc_lock, and it runs afterself.session = sessionbut before_ready.set()and_wait_for_lifecycle_event(). So the keepalive watchdoghadn't started, the handler's dead-session recovery path never ran (
sessionwas non-None), the
TimeoutErrorseen by callers matched no_SESSION_EXPIRED_MARKERS, and the circuit breaker's half-open probe simplyre-blocked on the same lock. Nothing in the process could break the cycle,
because nothing bounded the original await: Hermes never passes
read_timeout_seconds, so the SDK'ssend_requestwaits onanyio.fail_after(None).This PR bounds the awaits and gives the "transport hangs" case a recovery path.
No behavior change on a healthy server; no new config keys.
Related Issue
No existing issue; the incident analysis is inline above. Happy to file one if maintainers prefer.
Type of Change
Changes Made
tools/mcp_tool.py— passread_timeout_seconds(the per-servertimeout,default 300s) when constructing
ClientSessionon all three transports(stdio, SSE, streamable HTTP). Without it any request whose response never
arrives parks forever at the protocol layer. This is the structural floor:
it bounds the whole request class, not just today's call sites.
tools/mcp_tool.py— bound thetools/listdrain in_discover_tools()with
connect_timeoutrather than letting it inherit the per-tooltimeout.Session setup shouldn't spend a deliberately generous tool budget, and a
per-request floor still permits up to
_MCP_LIST_MAX_PAGES(50) sequentialpages under one lock. On timeout the exception propagates into
run()'sexisting
except Exception, which tears the transport down and retries withbackoff.
tools/mcp_tool.py— same bound on_refresh_tools()(driven bynotifications/tools/list_changedvia a fire-and-forget task). On timeout itlogs and sets
_reconnect_event: an unansweredtools/listmeans responsedelivery is dead even if the stream still looks alive, and the keepalive
probe uses a different request (
ping), so under selective request loss itcan keep reporting healthy forever.
tools/mcp_tool.py— in_make_tool_handler, when consecutive tool callstime out at the transport level and the count reaches
_CIRCUIT_BREAKER_THRESHOLD, ask the server task to rebuild via the existing_signal_reconnect(). Signalled only at the threshold crossing, never onevery call. Without this the breaker opens, cools down, re-probes into the
same wedged transport, and re-arms — indefinitely.
tests/tools/test_mcp_list_drain_timeout.py— new regression tests.How to Test
scripts/run_tests.sh tests/tools/test_mcp_list_drain_timeout.py— 7 pass.refresh, handler reconnect, HTTP read timeout, stdio read timeout). The 2
that pass are deliberate controls — a server that answers with an error is
alive and must NOT trigger a rebuild, and a guard that the pinned SDK
(
mcp==1.26.0) still acceptsread_timeout_seconds.scripts/run_tests.sh tests/tools/test_mcp*.py tests/test_mcp_serve.py tests/hermes_cli/test_mcp_*.py— 987 pass, 0 fail.mcp_serversentry at an endpoint that completes thehandshake and then black-holes
tools/list. Before: tool calls hang untilthe 300s caller timeout, forever. After: discovery fails at
connect_timeoutand the reconnect loop retries with backoff.
Risks / notes for reviewers
than
connect_timeout(default 60s) to return up to 50 pages will now faildiscovery instead of succeeding slowly. The escape hatch is the existing
per-server
connect_timeoutsetting. Per-page bounding was the alternative,but it reinstates a 50 × 60s ceiling under one lock, which defeats the point.
wait_forcancelslist_tools()in the servertask's own frame (not a child of the session's task group), so the SDK's
send_requestfinallypops_response_streams[request_id]and closes bothmemory streams — no half-open request state. A late response then finds no
stream and is routed to
_handle_incomingas aRuntimeError, which Hermes'message handler already absorbs via its
isinstance(message, Exception)branch. This matters for the refresh path, where the session deliberately
stays alive after the cancel.
equality). If a rebuild doesn't clear the fault, later half-open probe
timeouts won't re-signal until something resets the count — a successful
reconnect calls
_reset_server_error, which re-arms it.Kept out of scope, deliberately
connect_timeout/timeoutdefaults.list_resources/list_promptsdrains were left alone: they run fromhandlers that already carry a caller-side deadline, and
read_timeout_secondsnow bounds each of their pages. Adding drain-level bounds there would be
unrelated churn.
McpError(408)as areconnect trigger and factored the timeouts into two helpers. Both were
dropped: 408 is unreachable here (the caller-side deadline starts earlier and
always fires first), and the helpers cost more lines than the two expressions
they hid.
Checklist
scripts/run_tests.sh) and MCP tests passcli-config.yaml.example— N/A (no new/changed config keys)CONTRIBUTING.md/AGENTS.md— N/Aplatform-adjacent one and it's a pure kwarg on the SDK session
🤖 Generated with Claude Code