fix(mcp): keep the Streamable HTTP session usable after a 5xx - #5061
hyeonsang010716 wants to merge 1 commit into
Conversation
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The session-recovery issue is worth fixing, and retaining handshake failure handling is the right direction.
Please scope the 5xx exemption to MCP transport messages and preserve the existing handling of OAuth subrequests. MCP v2 includes the complete token or registration error response body in its OAuth exceptions, which bypass our HTTP error mapping under this change. That expands the patch beyond session recovery and changes what appears in rendered exceptions.
Please add a regression test through MCPServerStreamableHttp.connect() with a synthetic OAuth endpoint 503, asserting the existing UserError mapping and absence of the response-body marker from the rendered exception chain.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed f163363fee25e43bec516c69f66db486dfe73da9, including MCP 2.0.0 and the HTTP client's authentication/response-hook paths. The shared-session recovery issue is real, but the existing OAuth concern is still unresolved.
Please limit the 5xx exemption to identified post-handshake MCP transport messages and retain the existing HTTP error handling for OAuth subrequests. Token and registration requests do not carry a handshake JSON-RPC method; this condition lets their 5xx responses reach MCP's body-bearing OAuth exceptions, which bypass the SDK's mapped HTTP UserError path. Add focused coverage through connect() verifying the existing error mapping and that a synthetic response-body marker is absent from the rendered error.
This is a focused correction to the current change, consistent with the existing review request. Validation was static source review and recorded CI; no local runtime probe was executed.
With MCP Python SDK v2, the response hook that `MCPServerStreamableHttp` installs on its HTTP client called `raise_for_status()` for every 5xx. The hook runs inside the MCP transport task group, so a single transient 5xx on any request tore down the shared transport. Every later `call_tool()` and `list_tools()` then failed with `MCPError: Connection closed` until `cleanup()` and `connect()`. A 5xx on the `notifications/initialized` notification to a legacy-protocol server did the same while `connect()` still reported success. The hook now skips the raise only for a post-handshake MCP transport message, meaning a request body that is a JSON-RPC message other than `server/discover` and `initialize`. MCP v2 fails such a message on its own, so the session stays usable and `max_retry_attempts` retries on it. Every other response keeps the existing HTTP error mapping. A handshake 5xx still fails `connect()` with the HTTP status, so a failed discovery probe is not mistaken for a legacy server. OAuth auth-flow sub-requests such as token and registration requests are not transport messages, so their failures also stay on that path instead of reaching MCP's OAuth exceptions, which carry the authorization server's response body.
f163363 to
0bf4ff3
Compare
|
Updated. The exemption now covers only a post-handshake MCP transport message, that is a request whose
The session recovery tests are unchanged and still fail on |
Summary
With MCP Python SDK v2, a single transient HTTP 5xx permanently broke an
MCPServerStreamableHttpconnection. The response hook that the SDK installs on the v2 HTTP client called
raise_for_status()for every 5xx. The hook runs inside the MCP transport task group, so one 5xx on any request, for example
tools/call, tore down the whole transport. Every latercall_tool()andlist_tools()on the sameserver object then failed with
MCPError: Connection closeduntil the application calledcleanup()and
connect().With the default
max_retry_attempts=0nothing recovered. With retries enabled,call_tool()recoveredonly by opening a new isolated HTTP client for every later call, because each call first failed on the
dead shared session.
list_tools()has no isolated-session path, so it kept failing. The same teardownhappened when a legacy-protocol server answered the
notifications/initializednotification with a5xx: notifications are sent inline by the transport writer, so
connect()reported success on asession that was already dead.
MCP v2 already handles those messages without the hook. It turns a 5xx for a post-handshake request
into a JSON-RPC error for that request alone, and it ignores a 5xx for a notification, so the transport
stays alive. The hook cannot simply stop raising, though. A 5xx on the
server/discoverprobe wouldthen look like a legacy server, and MCP v2 would silently fall back to
initializeand the olderprotocol.
The hook now skips the raise only for a post-handshake MCP transport message, that is, a request whose
body is a JSON-RPC message other than the
server/discoverandinitializehandshake requests:connect()withUserErrorand the HTTP status, and a failed discoveryprobe is not mistaken for a legacy server. Isolated-session setup keeps the same mapping.
MCPError, thesession stays usable, and
max_retry_attemptsretries on the same session. This matches how 4xxresponses already behaved.
sub-requests, such as token and dynamic client registration requests, are not transport messages, so
their failures do not reach MCP's OAuth exceptions, which carry the authorization server's response
body.
Behavior changes worth knowing about, both limited to MCP Python SDK v2:
The retry budget and backoff are unchanged.
MCPError. Only the message changes, fromConnection closedto the message MCP provides. The default model-visible tool error changes only inthat message, and
failure_error_function=Nonestill raisesMCPError.The MCP Python SDK v1 path is unchanged. It does not install this hook, and its transport raises on
error statuses inside the
mcppackage itself.Test plan
tests/mcp/test_mcp_v2_http.py, each exercisingMCPServerStreamableHttpthrough a mockhttpx2transport:
test_v2_streamable_http_5xx_fails_only_that_requestanswers the firsttools/callwith 503 andasserts that call raises
MCPError, the nextcall_tool()andlist_tools()succeed, and only oneHTTP client is created. It fails on
main.test_v2_streamable_http_retries_5xx_on_shared_sessionassertsmax_retry_attempts=1recovers fromone 503 on the same client. It replaces
test_v2_streamable_http_retries_5xx_on_isolated_session,which asserted the isolated-client mechanism; the classifier branch that test covered is still
exercised by
tests/mcp/test_client_session_retries.py. It fails onmain.test_v2_streamable_http_initialized_notification_5xx_keeps_session_usableanswersnotifications/initializedfrom a legacy server with 503 and asserts later calls succeed. It failson
main.test_v2_streamable_http_handshake_5xx_fails_connect_without_legacy_fallbackanswersserver/discoverwith 503 and assertsconnect()raisesUserErrorwithHTTP error 503and sendsno
initialize. It passes onmainand fails if the hook stops raising for handshake requests.test_v2_streamable_http_oauth_subrequest_5xx_keeps_http_error_mappingdrivesconnect()with anOAuth client-credentials provider whose token endpoint answers 503, and asserts
UserErrorwithHTTP error 503plus the absence of the authorization server's response body from the renderedexception chain. It passes on
mainand fails if the exemption covers non-transport requests.test_v2_response_hook_raises_5xx_for_a_request_that_is_not_a_transport_messageposts a dynamicclient registration body, which is JSON but not a JSON-RPC message, and asserts the hook still
raises. It fails if the JSON-RPC discriminator is removed.
MCPServerserved by uvicorn, with one 503 injected on the firsttools/call, now recovers onthe next call with default settings. On
main, every later call andlist_tools()failed withConnection closed..agents/skills/code-change-verification/scripts/run.shpassed: format, lint,mypy src(312 files),pyright, then the parallel suite (9645 passed, 29 skipped) and the serial suite (77 passed, 4skipped).
mcp==1.28.1installed,tests/mcpshows the same 4 failures intests/mcp/test_caching.pyonmainand on this branch, so the v1 path is unaffected. The packagedmcp-v1-compatprofile needs anetwork install and was not run locally; it passes in CI on this branch.
Issue number
None.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR