fix(client): surface HTTP errors on resumption GET and SSE message POST by claude[bot] · Pull Request #3278 · modelcontextprotocol/python-sdk · GitHub
Skip to content
Open
21 changes: 20 additions & 1 deletion docs/migration.md
16 changes: 15 additions & 1 deletion src/mcp/client/_transport.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,28 @@
from contextlib import AbstractAsyncContextManager
from typing import Protocol

from mcp_types import INTERNAL_ERROR, INVALID_REQUEST, ErrorData

from mcp.shared._stream_protocols import ReadStream, WriteStream
from mcp.shared.message import SessionMessage

__all__ = ["ReadStream", "WriteStream", "Transport", "TransportStreams"]
__all__ = ["ReadStream", "WriteStream", "Transport", "TransportStreams", "status_error_data"]

TransportStreams = tuple[ReadStream[SessionMessage | Exception], WriteStream[SessionMessage]]


def status_error_data(status_code: int, *, has_session: bool) -> ErrorData:
"""Map a non-2xx HTTP status on a client transport request to the error its waiting caller receives.

A 404 while a session is held is the session-expiry signal (`INVALID_REQUEST`,
"Session terminated"); anything else gets the generic stand-in. A call site with
an extra status mapping (e.g. the message POST's pre-session 404) branches first.
"""
if status_code == 404 and has_session:
return ErrorData(code=INVALID_REQUEST, message="Session terminated")
return ErrorData(code=INTERNAL_ERROR, message="Server returned an error response")

Comment thread
claude[bot] marked this conversation as resolved.

class Transport(AbstractAsyncContextManager[TransportStreams], Protocol):
"""Protocol for MCP transports.

Expand Down
56 changes: 46 additions & 10 deletions src/mcp/client/sse.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from anyio.abc import TaskStatus
from httpx2 import SSEError

from mcp.client._transport import status_error_data
from mcp.shared._compat import resync_tracer
from mcp.shared._context_streams import create_context_streams
from mcp.shared._httpx_utils import McpHttpClientFactory, create_mcp_http_client
Expand Down Expand Up @@ -120,17 +121,52 @@ async def post_writer(endpoint_url: str):
async with write_stream_reader, write_stream:

async def _send_message(session_message: SessionMessage) -> None:
# A POST failure must not raise: the post_writer handler below
# would swallow it, hanging the waiting caller forever and killing
# the write loop (#2110). Mirror the streamable-HTTP transport
# instead: resolve the waiter with an error correlated to its
# request id, keeping the session usable.
logger.debug(f"Sending client message: {session_message}")
response = await client.post(
endpoint_url,
json=session_message.message.model_dump(
by_alias=True,
mode="json",
exclude_unset=True,
),
)
response.raise_for_status()
logger.debug(f"Client message sent successfully: {response.status_code}")
message = session_message.message
try:
response = await client.post(
endpoint_url,
json=message.model_dump(
by_alias=True,
mode="json",
exclude_unset=True,
),
)
except Exception as exc:
# Terminal containment boundary: beyond httpx's own errors,
Comment on lines +124 to +141

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing issue (not introduced by this PR, but in code it rewrites end-to-end): _send_message POSTs with client.post(endpoint_url, json=...) and never consults session_message.metadata, so the per-request headers documented as a cross-transport contract in CallOptions.headers (per-request auth, tracing) and the session-stamped MCP-Protocol-Version are silently dropped on every SSE message POST — while streamable HTTP honors the same metadata via headers.update(ctx.metadata.headers). Same class as the resumption/reconnection-GET header drop already slated for the grouped follow-up on streamable_http.py; the follow-up should cover this leg too, with a one-line headers= merge here.

Extended reasoning...

What the bug is. CallOptions.headers is documented at src/mcp/shared/dispatcher.py:127-128 as a transport-layer contract: "HTTP transports merge these onto the outgoing request; non-HTTP transports ignore." The SSE transport is an HTTP transport, but its message POST never honors the contract: _send_message in src/mcp/client/sse.py reads only session_message.message and calls client.post(endpoint_url, json=...) with no headers= argument and no look at session_message.metadata. Everything the caller stamped via the public CallOptions["headers"] — per-request auth tokens, tracing headers, tenant routing — is silently discarded, and nothing logs the drop.\n\nThe metadata is populated on essentially every SSE message. The dispatcher honors its side of the contract: _plan_outbound (src/mcp/shared/jsonrpc_dispatcher.py:250-256) attaches the caller's headers to ClientMessageMetadata(headers=headers) on the outbound SessionMessage. And ClientSession stamps headers regardless of transport: after a legacy handshake — the only kind an SSE server produces — self._stamp = _make_handshake_stamp(version) (src/mcp/client/session.py:666) writes opts["headers"][MCP_PROTOCOL_VERSION_HEADER] = version into every subsequent request and notification (session.py:113-117, applied at 530/573). So the drop fires constantly, not just for exotic callers — the SDK's own protocol-version stamp never reaches the wire on this transport.\n\nWhy the sibling transport makes this surprising. The streamable HTTP transport honors the identical metadata in _handle_post_request: headers.update(ctx.metadata.headers). Code that works there — e.g. per-request auth against a proxy — silently loses its headers when pointed at an SSE server, with no client-side signal to diagnose.\n\nStep-by-step proof. (1) A user calls session.call_tool(..., options={"headers": {"authorization": "Bearer per-request-token"}}) against an SSE server behind an auth proxy. (2) _plan_outbound attaches ClientMessageMetadata(headers={"authorization": ...}) to the outbound SessionMessage. (3) The SSE post_writer dequeues it and _send_message POSTs the JSON body with no headers — the token never leaves the client. (4) The proxy answers 401. (5) Under this PR's new mapping the caller receives a definitive-looking MCPError(INTERNAL_ERROR, "Server returned an error response") — or, since SseServerTransport endpoints always carry a session id, a 404-ing proxy yields the actively misleading MCPError(INVALID_REQUEST, "Session terminated") — when the real failure is that the client dropped the credentials it was explicitly given.\n\nWhy this is pre-existing, and why it is still worth flagging on this PR. The pre-PR _send_message had the identical bare client.post(endpoint_url, json=...) (visible in the diff's removed lines); the PR rewrote the function end-to-end around the gap without introducing it, so it should not block merging. It is flagged because (a) the PR touches exactly this function, and (b) its new status mapping gives the downstream failure a misleading shape — a client-side credential drop now reads as a definitive server error or session expiry, which reconnect logic keyed on Session terminated will act on falsely.\n\nNot a duplicate, and how to fix. The already-acknowledged follow-up item on streamable_http.py (dropped ctx.metadata.headers on the resumption/reconnection GETs) is the same conceptual class but a disjoint site with a disjoint fix — merging headers there never touches sse.py. This leg needs its own one-line change in _send_message, e.g.:\n\npython\nfrom mcp.shared.message import ClientMessageMetadata\n\nmetadata = session_message.metadata\nheaders = (\n dict(metadata.headers)\n if isinstance(metadata, ClientMessageMetadata) and metadata.headers\n else None\n)\nresponse = await client.post(endpoint_url, headers=headers, json=...)\n\n\nIt belongs in the grouped follow-up the author already planned for the other header-drop legs, extended to cover this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — the SSE message POST dropping session_message.metadata headers is a real, pre-existing gap, the same class as the two streamable GET-leg header drops already slated for the grouped follow-up; that follow-up should cover this leg too so per-message headers flow uniformly on every outbound HTTP call. Orthogonal to the error-surfacing contract this PR fixes, so declining here.


Generated by Claude Code

# user-supplied auth flows and hooks can raise arbitrary types
# from inside `client.post()`, so an enumerated catch cannot
# keep the caller from hanging.
logger.exception("Error POSTing message")
error = types.ErrorData(
code=types.CONNECTION_CLOSED, message=f"Failed to send message: {exc}"
)
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
else:
if response.is_success:
logger.debug(f"Client message sent successfully: {response.status_code}")
return
logger.error(f"Message POST returned HTTP status {response.status_code}")
Comment on lines +150 to +153

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Under the default client factory (create_mcp_http_client sets follow_redirects=True), a Location-bearing 301/302/303 on the SSE message POST is followed by httpx2, which rewrites the POST to a bodyless GET — so the JSON-RPC message is silently dropped, and a 2xx at the redirect target (e.g. an SSO login page) makes response.is_success pass, logging 'sent successfully' while the waiting caller hangs forever: the residual #2110 hang the is_success widening does not close, since the PR's 302 tests use Location-less responses that httpx cannot follow. Consider treating a method-rewriting redirect as a delivery failure (e.g. response.history non-empty and final request method != POST) and resolving the waiter via the same correlated path — method-preserving 307/308 keep working.

Extended reasoning...

What the bug is. The new success check at src/mcp/client/sse.py:150 — if response.is_success:return — only ever sees the final response of httpx2's redirect-following. The default factory create_mcp_http_client (src/mcp/shared/_httpx_utils.py:79) hardcodes follow_redirects=True (its docstring: "Always enables follow_redirects"), and sse_client uses it by default. httpx2's redirect handling rewrites POST to GET on 301/302/303 (_redirect_method, browser semantics) and drops the request body when the method changes (_redirect_stream returns no stream). So a Location-bearing 3xx on the message POST silently discards the JSON-RPC message, GETs the redirect target with no body, and if that target answers any 2xx, is_success is True: the transport logs "Client message sent successfully" at debug level and returns without resolving the waiter.\n\nWhy this hangs the caller. On this transport, unlike streamable HTTP, real responses only ever arrive on the SSE stream — the POST response body is never inspected. With the message never delivered, nothing will arrive on the SSE stream for that request id, and _send_message reported success, so the correlated-error path this PR built is never taken. The caller of session.call_tool() hangs until its own timeout — the exact #2110 symptom, indistinguishable from a slow server, with no log above debug.\n\nWhy the PR's hardening doesn't cover it. The is_success widening and the [302] parametrization of test_sse_client_request_post_http_error_reaches_caller_and_session_survives cover only unfollowed redirects: make_app_rejecting_posts returns Response(status_code=302) with no Location header, which httpx cannot follow (has_redirect_location is False), so the 3xx reaches the is_success check — the test docstring itself says "An unfollowed redirect counts." With a Location present (the realistic production shape), the 3xx never reaches the check; only the redirect target's 2xx does. The in-process test factory explicitly enables follow_redirects=True "to match create_mcp_http_client," confirming the followed case is simply untested.\n\nWhy only this leg. The sibling paths self-heal in the same scenario: on the streamable message POST, a followed redirect to a 200 HTML page falls into _handle_post_request's unexpected-content-type branch and resolves the waiter; on the resumption GET and SSE initial GET, a redirect landing on non-text/event-stream content makes httpx2's EventSource raise SSEError, which the new containment resolves. The SSE message POST is the one leg that never inspects the response, so a followed-redirect-to-2xx uniquely reads as success.\n\nStep-by-step proof. (1) A client connects through a corporate gateway; the SSE GET stream is established while auth is valid. (2) Auth expires mid-session; the gateway answers the next message POST with 302 Location: https://sso.example/login. (3) httpx2 follows: _redirect_method rewrites POST→GET, _redirect_stream drops the JSON-RPC body, and the login page returns 200 text/html. (4) response.is_success is True; _send_message logs "sent successfully" (debug) and returns. (5) The waiter is never resolved; session.call_tool() hangs into its timeout, and every subsequent request repeats the cycle — no MCPError, no visible log.\n\nSeverity and fix. This is behaviorally pre-existing — v1's raise_for_status() on the followed final 200 passed identically, so merging this PR causes no new failure, and the PR strictly improves the path (the migration table's non-2xx rows aren't literally violated: a followed redirect terminates in a 2xx). That's why this is a nit, not blocking. But since the PR rewrites this exact line, reasons explicitly about redirects on this path, and documents/tests a redirects-resolve-the-caller contract, it's worth closing here or in a follow-up: after the POST, treat a method-rewriting redirect as a delivery failure — e.g. if response.history and response.request.method != "POST": resolve the waiter via the same status_error_data/correlated path (using the first redirect's status, or a generic delivery-failure error). Method-preserving 307/308 redirects keep the POST and body, so they genuinely deliver and continue to work. All three verifiers confirmed this chain end-to-end; none refuted it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real residual case — but behaviorally pre-existing per your own analysis: v1 passed identically on the followed 200, so this PR neither introduces nor worsens it. Treating method-rewriting redirects (301/302/303 turning the POST into a GET) as delivery failures is a deliberate behavior change, and it belongs in the grouped follow-up rather than another round here. Adding it to that follow-up's list alongside the reconnection-loop raise_for_status(), the uncontained POST-path reply sends, and the resumption/reconnection GET header drops. Declining for this PR.


Generated by Claude Code

# The endpoint URL carrying a session id is this transport's
# "session established" signal, as `self.session_id` is for
# streamable HTTP.
error = status_error_data(
response.status_code,
has_session=_extract_session_id_from_endpoint(endpoint_url) is not None,
)
# A notification has no waiter to resolve, so its failure is only logged.
Comment thread
claude[bot] marked this conversation as resolved.
if isinstance(message, types.JSONRPCRequest):
reply = types.JSONRPCError(jsonrpc="2.0", id=message.id, error=error)
try:
await read_stream_writer.send(SessionMessage(reply))
except (anyio.BrokenResourceError, anyio.ClosedResourceError):
# Teardown race: the reader is gone, so there is nobody
# left to resolve - contain it, keeping the write loop up.
logger.debug("read stream closed before request %r could be resolved", message.id)

async for session_message in write_stream_reader:
Comment thread
claude[bot] marked this conversation as resolved.
sender_ctx = write_stream_reader.last_context
Expand Down
77 changes: 47 additions & 30 deletions src/mcp/client/streamable_http.py
Loading
Loading