Skip to content

Commit 51d99af

Browse files
committed
fix(client): cover OAuth failures and 404 session expiry on the SSE POST; document the new error contract
Address the second review round: - Widen the SSE message POST's failure catch to (httpx.HTTPError, OAuthFlowError): an OAuthClientProvider re-auth failing inside client.post() previously took the same swallowed path and hung the waiting caller forever. - Map a 404 on the SSE message POST to INVALID_REQUEST / "Session terminated" when the endpoint URL carries a session id (the SSE analogue of the streamable transport's session check); keep the generic error when it does not. - Document the changed error behavior in docs/migration.md: resumption GET and SSE message POST outcome tables, and scope the "connect-level failures still escape" sentence to the streamable message POST, the one place it still holds. Three new regression tests; the OAuth and 404-session ones fail against the previous revision. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AuJi8kEB3bhikW2pzbmhUL
1 parent b4edbd4 commit 51d99af

3 files changed

Lines changed: 119 additions & 6 deletions

File tree

docs/migration.md

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2188,6 +2188,25 @@ In v1, a non-2xx response to a message POST (other than 404) raised `httpx.HTTPS
21882188
| 404, no session yet | `McpError` with positive code `32600` | `MCPError(-32601, 'Not Found')` |
21892189
| Any other 4xx/5xx | `httpx.HTTPStatusError` escapes as `ExceptionGroup` | `MCPError(-32603, 'Server returned an error response')` |
21902190

2191+
The same contract covers the resumption GET — a request re-attached with a resumption token (`Last-Event-ID`). In v1 a failure there escaped the context as an `ExceptionGroup` that failed every pending request, or hung the resumed call forever:
2192+
2193+
| Resumption GET outcome | v1 | v2 |
2194+
| --- | --- | --- |
2195+
| 404, session established | `httpx.HTTPStatusError` escapes as `ExceptionGroup` | `MCPError(-32600, 'Session terminated')` |
2196+
| Any other non-2xx | `httpx.HTTPStatusError` escapes as `ExceptionGroup` | `MCPError(-32603, 'Server returned an error response')` |
2197+
| Stream drops mid-read | error escapes as `ExceptionGroup` | `MCPError(-32000, 'resumption stream ended without a response')` |
2198+
| Stream ends cleanly with no response | resumed call hangs forever | `MCPError(-32000, 'resumption stream ended without a response')` |
2199+
2200+
The SSE transport (`sse_client`) applies the same rule to its message POST. In v1 *any* POST failure — a non-2xx status, a network error, or an OAuth re-auth failure raised by the configured `auth` — was caught and logged inside the transport's writer task: the waiting caller hung forever and the write loop died, so every later send was silently dropped. In v2 the failing request resolves promptly and the session stays usable:
2201+
2202+
| Message POST outcome | v1 | v2 |
2203+
| --- | --- | --- |
2204+
| 404, endpoint URL carries a session id | caller hangs forever; write loop dies | `MCPError(-32600, 'Session terminated')` |
2205+
| Any other non-2xx | caller hangs forever; write loop dies | `MCPError(-32603, 'Server returned an error response')` |
2206+
| Network-level failure (`httpx2.ConnectError`, timeouts) or OAuth flow failure | caller hangs forever; write loop dies | `MCPError(-32000, 'Failed to send message: ...')` |
2207+
2208+
A failed POST of a *notification* has no caller to resolve; v2 logs and drops it, keeping the write loop (and every later send) alive.
2209+
21912210
Both common v1 patterns silently stop working: an `except* httpx.HTTPStatusError` around the transport context becomes dead code because status errors no longer escape the context, and a session-expiry check on `error.code == 32600` never matches again because the code is now the standard negative `-32600`.
21922211

21932212
**Before (v1):**
@@ -2230,7 +2249,7 @@ async with streamable_http_client(url) as (read, write):
22302249
raise
22312250
```
22322251

2233-
Move HTTP-status failure handling from around the transport context to around the individual calls, catching `MCPError` (see [`McpError` renamed to `MCPError`](#mcperror-renamed-to-mcperror)). Connect-level failures such as `httpx2.ConnectError` still escape the transport context as before; keep context-level handling for those only.
2252+
Move HTTP-status failure handling from around the transport context to around the individual calls, catching `MCPError` (see [`McpError` renamed to `MCPError`](#mcperror-renamed-to-mcperror)). On the streamable HTTP transport, connect-level failures such as `httpx2.ConnectError` on a message POST still escape the transport context as beforekeep context-level handling for those; on the resumption GET and on the SSE transport's message POST they resolve the failing request instead, as above.
22342253

22352254
### `terminate_windows_process` removed
22362255

src/mcp/client/sse.py

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

13+
from mcp.client.auth.exceptions import OAuthFlowError
1314
from mcp.shared._compat import resync_tracer
1415
from mcp.shared._context_streams import create_context_streams
1516
from mcp.shared._httpx_utils import McpHttpClientFactory, create_mcp_http_client
@@ -136,7 +137,9 @@ async def _send_message(session_message: SessionMessage) -> None:
136137
exclude_unset=True,
137138
),
138139
)
139-
except httpx2.HTTPError as exc:
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.
140143
logger.exception("Error POSTing message")
141144
error = types.ErrorData(
142145
code=types.CONNECTION_CLOSED, message=f"Failed to send message: {exc}"
@@ -146,9 +149,17 @@ async def _send_message(session_message: SessionMessage) -> None:
146149
logger.debug(f"Client message sent successfully: {response.status_code}")
147150
return
148151
logger.error(f"Message POST returned HTTP status {response.status_code}")
149-
error = types.ErrorData(
150-
code=types.INTERNAL_ERROR, message="Server returned an error response"
151-
)
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+
)
152163
# A notification has no waiter to resolve, so its failure is only logged.
153164
if isinstance(message, types.JSONRPCRequest):
154165
reply = types.JSONRPCError(jsonrpc="2.0", id=message.id, error=error)

tests/shared/test_sse.py

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,12 @@
3131
)
3232
from starlette.applications import Starlette
3333
from starlette.requests import Request
34-
from starlette.responses import Response
34+
from starlette.responses import Response, StreamingResponse
3535
from starlette.routing import Mount, Route
3636
from starlette.types import ASGIApp, Receive, Scope, Send
3737

3838
import mcp.client.sse
39+
from mcp.client.auth.exceptions import OAuthTokenError
3940
from mcp.client.session import ClientSession
4041
from mcp.client.sse import _extract_session_id_from_endpoint, sse_client
4142
from mcp.server import Server, ServerRequestContext
@@ -340,6 +341,88 @@ def factory(
340341
assert isinstance(await session.send_ping(), EmptyResult)
341342

342343

344+
@pytest.mark.anyio
345+
async def test_sse_client_post_404_with_session_endpoint_reports_session_terminated() -> None:
346+
"""A 404 on a request's message POST while the endpoint URL carries a session id reports
347+
"Session terminated" (INVALID_REQUEST) to the caller, the same session-expiry mapping as
348+
the streamable HTTP transport (SDK-defined)."""
349+
factory = in_process_client_factory(make_app_rejecting_posts({"resources/read": 404}))
350+
with anyio.fail_after(5):
351+
# One parenthesized async-with: separately nested ones trip a phantom
352+
# branch arc under coverage on Python 3.14 (see the note in mcp.client.sse).
353+
async with (
354+
sse_client(f"{BASE_URL}/sse", httpx_client_factory=factory) as streams,
355+
ClientSession(*streams) as session,
356+
):
357+
await session.initialize()
358+
359+
with pytest.raises(MCPError) as exc_info:
360+
await session.read_resource(uri="foobar://should-work")
361+
assert exc_info.value.error.code == types.INVALID_REQUEST
362+
assert exc_info.value.error.message == snapshot("Session terminated")
363+
364+
365+
@pytest.mark.anyio
366+
async def test_sse_client_post_404_without_session_endpoint_keeps_generic_error() -> None:
367+
"""A 404 on a request's message POST when the endpoint URL carries no session id keeps the
368+
generic error: with no session to expire, "Session terminated" would be a lie (SDK-defined).
369+
The raw endpoint is scripted because `SseServerTransport` always issues a session id."""
370+
371+
async def handle_sse(request: Request) -> StreamingResponse:
372+
async def stream() -> AsyncGenerator[str, None]:
373+
yield "event: endpoint\ndata: /messages/\n\n"
374+
await anyio.Event().wait() # park until the client disconnects
375+
376+
return StreamingResponse(stream(), media_type="text/event-stream")
377+
378+
async def handle_post(request: Request) -> Response:
379+
return Response(status_code=404)
380+
381+
app = Starlette(routes=[Route("/sse", handle_sse), Route("/messages/", handle_post, methods=["POST"])])
382+
factory = in_process_client_factory(app)
383+
with anyio.fail_after(5):
384+
async with (
385+
sse_client(f"{BASE_URL}/sse", httpx_client_factory=factory) as streams,
386+
ClientSession(*streams) as session,
387+
):
388+
with pytest.raises(MCPError) as exc_info:
389+
await session.initialize()
390+
assert exc_info.value.error.code == types.INTERNAL_ERROR
391+
assert exc_info.value.error.message == snapshot("Server returned an error response")
392+
393+
394+
@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
398+
(SDK-defined; #2110 — like any network error, it used to be swallowed inside post_writer)."""
399+
400+
class _RefusingAuth(httpx2.Auth):
401+
"""Stands in for OAuthClientProvider whose re-auth fails mid-session."""
402+
403+
async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx2.Request, httpx2.Response]:
404+
if request.method == "POST" and json.loads(request.content).get("method") == "resources/read":
405+
raise OAuthTokenError("re-authentication failed")
406+
yield request
407+
408+
factory = in_process_client_factory(make_server_app())
409+
with anyio.fail_after(5):
410+
async with (
411+
sse_client(f"{BASE_URL}/sse", httpx_client_factory=factory, auth=_RefusingAuth()) as streams,
412+
ClientSession(*streams) as session,
413+
):
414+
await session.initialize()
415+
416+
with pytest.raises(MCPError) as exc_info:
417+
await session.read_resource(uri="foobar://should-work")
418+
assert exc_info.value.error.code == types.CONNECTION_CLOSED
419+
# The message embeds the auth exception's text; pin only the SDK-authored prefix.
420+
assert exc_info.value.error.message.startswith("Failed to send message:")
421+
422+
# The session survived the failed POST: the next request round-trips.
423+
assert isinstance(await session.send_ping(), EmptyResult)
424+
425+
343426
@pytest.mark.anyio
344427
async def test_sse_client_notification_post_http_error_leaves_session_usable() -> None:
345428
"""A non-2xx on a notification's message POST resolves no caller (a notification has no

0 commit comments

Comments
 (0)