Skip to content

Commit 6472241

Browse files
committed
fix(client): contain arbitrary POST failures and dedupe the status-error mapping
Address the third review round: - Broaden the SSE message POST's failure catch to a terminal containment boundary (except Exception): user-supplied auth flows and hooks raise arbitrary types from inside client.post(), so an enumerated catch cannot keep the caller from hanging. - Extract the status -> JSON-RPC error mapping into mcp.client._transport.status_error_data and use it from the message POST handler, the resumption GET, and the SSE POST; the message POST keeps its pre-session 404 -> METHOD_NOT_FOUND case locally. Wire-identical. - Contain the SSE error-resolution send against a concurrently closed read stream (BrokenResourceError/ClosedResourceError -> debug log), mirroring _resolve_abandoned_request, so the teardown race cannot kill the write loop. Tests: the auth-failure test is parametrized over OAuthTokenError and RuntimeError, and a raw-stream teardown-race test pins that a failing POST whose error is undeliverable leaves the write loop serving later messages. Both fail against the previous revision. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AuJi8kEB3bhikW2pzbmhUL
1 parent 51d99af commit 6472241

4 files changed

Lines changed: 93 additions & 43 deletions

File tree

src/mcp/client/_transport.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
from contextlib import AbstractAsyncContextManager
66
from typing import Protocol
77

8+
from mcp_types import INTERNAL_ERROR, INVALID_REQUEST, ErrorData
9+
810
from mcp.shared._stream_protocols import ReadStream, WriteStream
911
from mcp.shared.message import SessionMessage
1012

@@ -13,6 +15,18 @@
1315
TransportStreams = tuple[ReadStream[SessionMessage | Exception], WriteStream[SessionMessage]]
1416

1517

18+
def status_error_data(status_code: int, *, has_session: bool) -> ErrorData:
19+
"""Map a non-2xx HTTP status on a client transport request to the error its waiting caller receives.
20+
21+
A 404 while a session is held is the session-expiry signal (`INVALID_REQUEST`,
22+
"Session terminated"); anything else gets the generic stand-in. A call site with
23+
an extra status mapping (e.g. the message POST's pre-session 404) branches first.
24+
"""
25+
if status_code == 404 and has_session:
26+
return ErrorData(code=INVALID_REQUEST, message="Session terminated")
27+
return ErrorData(code=INTERNAL_ERROR, message="Server returned an error response")
28+
29+
1630
class Transport(AbstractAsyncContextManager[TransportStreams], Protocol):
1731
"""Protocol for MCP transports.
1832

src/mcp/client/sse.py

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
from anyio.abc import TaskStatus
1111
from httpx2 import SSEError
1212

13-
from mcp.client.auth.exceptions import OAuthFlowError
13+
from mcp.client._transport import status_error_data
1414
from mcp.shared._compat import resync_tracer
1515
from mcp.shared._context_streams import create_context_streams
1616
from mcp.shared._httpx_utils import McpHttpClientFactory, create_mcp_http_client
@@ -137,9 +137,11 @@ async def _send_message(session_message: SessionMessage) -> None:
137137
exclude_unset=True,
138138
),
139139
)
140-
except (httpx2.HTTPError, OAuthFlowError) as exc:
141-
# OAuthFlowError: OAuthClientProvider re-auth failing inside
142-
# client.post() must resolve the waiter like any network error.
140+
except Exception as exc:
141+
# Terminal containment boundary: beyond httpx's own errors,
142+
# user-supplied auth flows and hooks can raise arbitrary types
143+
# from inside `client.post()`, so an enumerated catch cannot
144+
# keep the caller from hanging.
143145
logger.exception("Error POSTing message")
144146
error = types.ErrorData(
145147
code=types.CONNECTION_CLOSED, message=f"Failed to send message: {exc}"
@@ -149,21 +151,22 @@ async def _send_message(session_message: SessionMessage) -> None:
149151
logger.debug(f"Client message sent successfully: {response.status_code}")
150152
return
151153
logger.error(f"Message POST returned HTTP status {response.status_code}")
152-
if (
153-
response.status_code == 404
154-
and _extract_session_id_from_endpoint(endpoint_url) is not None
155-
):
156-
# The endpoint URL carries the session id, so a 404 is the
157-
# session-expiry signal - same mapping as streamable HTTP.
158-
error = types.ErrorData(code=types.INVALID_REQUEST, message="Session terminated")
159-
else:
160-
error = types.ErrorData(
161-
code=types.INTERNAL_ERROR, message="Server returned an error response"
162-
)
154+
# The endpoint URL carrying a session id is this transport's
155+
# "session established" signal, as `self.session_id` is for
156+
# streamable HTTP.
157+
error = status_error_data(
158+
response.status_code,
159+
has_session=_extract_session_id_from_endpoint(endpoint_url) is not None,
160+
)
163161
# A notification has no waiter to resolve, so its failure is only logged.
164162
if isinstance(message, types.JSONRPCRequest):
165163
reply = types.JSONRPCError(jsonrpc="2.0", id=message.id, error=error)
166-
await read_stream_writer.send(SessionMessage(reply))
164+
try:
165+
await read_stream_writer.send(SessionMessage(reply))
166+
except (anyio.BrokenResourceError, anyio.ClosedResourceError):
167+
# Teardown race: the reader is gone, so there is nobody
168+
# left to resolve - contain it, keeping the write loop up.
169+
logger.debug("read stream closed before request %r could be resolved", message.id)
167170

168171
async for session_message in write_stream_reader:
169172
sender_ctx = write_stream_reader.last_context

src/mcp/client/streamable_http.py

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
from httpx2 import EventSource, ServerSentEvent
1515
from mcp_types import (
1616
CONNECTION_CLOSED,
17-
INTERNAL_ERROR,
1817
INVALID_REQUEST,
1918
METHOD_NOT_FOUND,
2019
PARSE_ERROR,
@@ -30,7 +29,7 @@
3029
from mcp_types.version import MODERN_PROTOCOL_VERSIONS
3130
from pydantic import ValidationError
3231

33-
from mcp.client._transport import TransportStreams
32+
from mcp.client._transport import TransportStreams, status_error_data
3433
from mcp.shared._compat import resync_tracer
3534
from mcp.shared._context_streams import ContextReceiveStream, ContextSendStream, create_context_streams
3635
from mcp.shared._httpx_utils import create_mcp_http_client
@@ -259,18 +258,11 @@ async def _handle_resumption_request(self, ctx: RequestContext) -> None:
259258
# Resolve the waiting caller with an error correlated to its request,
260259
# mirroring `_handle_post_request`: an escaping `HTTPStatusError` would
261260
# tear down the transport's task group and every stream with it (#2110).
262-
if event_source.response.status_code == 404 and self.session_id is not None:
263-
# The GET carried our Mcp-Session-Id, so a 404 is the session-expiry
264-
# signal reconnect logic keys on - same mapping as the POST path.
265-
await self._resolve_abandoned_request(
266-
ctx.read_stream_writer, original_request_id, "Session terminated", code=INVALID_REQUEST
267-
)
268-
return
261+
error_data = status_error_data(
262+
event_source.response.status_code, has_session=self.session_id is not None
263+
)
269264
await self._resolve_abandoned_request(
270-
ctx.read_stream_writer,
271-
original_request_id,
272-
"Server returned an error response",
273-
code=INTERNAL_ERROR,
265+
ctx.read_stream_writer, original_request_id, error_data.message, code=error_data.code
274266
)
275267
return
276268
logger.debug("Resumption GET SSE connection established")
@@ -384,16 +376,13 @@ async def _handle_post_request(self, ctx: RequestContext) -> None:
384376
except (httpx2.StreamError, ValidationError):
385377
pass
386378
logger.debug("Non-2xx body was not a JSON-RPC error; using fallback")
387-
if response.status_code == 404:
388-
if self.session_id is None:
389-
# No session yet → 404 is the HTTP-level spelling of
390-
# METHOD_NOT_FOUND (gateway / legacy server doesn't know
391-
# this method); "Session terminated" would be a lie here.
392-
error_data = ErrorData(code=METHOD_NOT_FOUND, message="Not Found")
393-
else:
394-
error_data = ErrorData(code=INVALID_REQUEST, message="Session terminated")
379+
if response.status_code == 404 and self.session_id is None:
380+
# No session yet → 404 is the HTTP-level spelling of
381+
# METHOD_NOT_FOUND (gateway / legacy server doesn't know
382+
# this method); "Session terminated" would be a lie here.
383+
error_data = ErrorData(code=METHOD_NOT_FOUND, message="Not Found")
395384
else:
396-
error_data = ErrorData(code=INTERNAL_ERROR, message="Server returned an error response")
385+
error_data = status_error_data(response.status_code, has_session=self.session_id is not None)
397386
session_message = SessionMessage(JSONRPCError(jsonrpc="2.0", id=message.id, error=error_data))
398387
await ctx.read_stream_writer.send(session_message)
399388
return

tests/shared/test_sse.py

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
EmptyResult,
2020
Implementation,
2121
InitializeResult,
22+
JSONRPCRequest,
2223
JSONRPCResponse,
2324
ListToolsResult,
2425
PaginatedRequestParams,
@@ -44,6 +45,7 @@
4445
from mcp.server.transport_security import TransportSecuritySettings
4546
from mcp.shared._httpx_utils import McpHttpClientFactory
4647
from mcp.shared.exceptions import MCPError
48+
from mcp.shared.message import SessionMessage
4749
from tests.interaction.transports import StreamingASGITransport
4850

4951
SERVER_NAME = "test_server_for_SSE"
@@ -392,17 +394,21 @@ async def handle_post(request: Request) -> Response:
392394

393395

394396
@pytest.mark.anyio
395-
async def test_sse_client_oauth_failure_on_post_reaches_caller_and_session_survives() -> None:
396-
"""An SDK OAuth flow failure raised from inside a request's message POST reaches the waiting
397-
caller promptly as a JSON-RPC error correlated to the request, and the session stays usable
397+
@pytest.mark.parametrize("exc_type", [OAuthTokenError, RuntimeError])
398+
async def test_sse_client_auth_failure_on_post_reaches_caller_and_session_survives(
399+
exc_type: type[Exception],
400+
) -> None:
401+
"""A failure raised from inside a request's message POST by a user-supplied hook — an SDK
402+
OAuth flow error, or any exception from a custom auth flow — reaches the waiting caller
403+
promptly as a JSON-RPC error correlated to the request, and the session stays usable
398404
(SDK-defined; #2110 — like any network error, it used to be swallowed inside post_writer)."""
399405

400406
class _RefusingAuth(httpx2.Auth):
401-
"""Stands in for OAuthClientProvider whose re-auth fails mid-session."""
407+
"""Stands in for OAuthClientProvider (or any user auth hook) failing mid-session."""
402408

403409
async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx2.Request, httpx2.Response]:
404410
if request.method == "POST" and json.loads(request.content).get("method") == "resources/read":
405-
raise OAuthTokenError("re-authentication failed")
411+
raise exc_type("re-authentication failed")
406412
yield request
407413

408414
factory = in_process_client_factory(make_server_app())
@@ -423,6 +429,44 @@ async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx
423429
assert isinstance(await session.send_ping(), EmptyResult)
424430

425431

432+
@pytest.mark.anyio
433+
async def test_sse_client_post_error_after_reader_closed_is_contained() -> None:
434+
"""A failing POST whose error can no longer be delivered — the read stream already closed
435+
with the server's SSE stream — is contained: the write loop survives and later messages
436+
still reach the server (SDK-defined teardown-race guard). Raw streams, because the race
437+
needs the read side closed while the write side keeps sending."""
438+
posted: list[str] = []
439+
second_post = anyio.Event()
440+
441+
async def handle_sse(request: Request) -> StreamingResponse:
442+
async def stream() -> AsyncGenerator[str, None]:
443+
# The stream ends right after the endpoint event: the client's reader
444+
# observes EOF and closes the read stream.
445+
yield "event: endpoint\ndata: /messages/\n\n"
446+
447+
return StreamingResponse(stream(), media_type="text/event-stream")
448+
449+
async def handle_post(request: Request) -> Response:
450+
posted.append(json.loads(await request.body())["method"])
451+
if len(posted) == 2:
452+
second_post.set()
453+
return Response(status_code=500)
454+
455+
app = Starlette(routes=[Route("/sse", handle_sse), Route("/messages/", handle_post, methods=["POST"])])
456+
factory = in_process_client_factory(app)
457+
with anyio.fail_after(5):
458+
async with sse_client(f"{BASE_URL}/sse", httpx_client_factory=factory) as (read, write):
459+
# Wait for the reader to observe the server's EOF and close the read stream.
460+
with pytest.raises(anyio.EndOfStream):
461+
await read.receive()
462+
463+
await write.send(SessionMessage(JSONRPCRequest(jsonrpc="2.0", id=1, method="first/call", params={})))
464+
await write.send(SessionMessage(JSONRPCRequest(jsonrpc="2.0", id=2, method="second/call", params={})))
465+
await second_post.wait()
466+
# The first POST's undeliverable error was contained; the second still went out.
467+
assert posted == ["first/call", "second/call"]
468+
469+
426470
@pytest.mark.anyio
427471
async def test_sse_client_notification_post_http_error_leaves_session_usable() -> None:
428472
"""A non-2xx on a notification's message POST resolves no caller (a notification has no

0 commit comments

Comments
 (0)