Repository navigation
fix(client): surface HTTP errors on resumption GET and SSE message POST #3278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
15b394c
e5fe739
b4edbd4
51d99af
6472241
af77621
d25e6f8
66a9285
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,17 +120,39 @@ | |
| 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 httpx2.HTTPError as exc: | ||
| logger.exception("Error POSTing message") | ||
| error = types.ErrorData( | ||
| code=types.CONNECTION_CLOSED, message=f"Failed to send message: {exc}" | ||
| ) | ||
|
Check warning on line 143 in src/mcp/client/sse.py
|
||
|
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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 —
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Generated by Claude Code |
||
| error = types.ErrorData( | ||
| code=types.INTERNAL_ERROR, message="Server returned an error response" | ||
| ) | ||
| # A notification has no waiter to resolve, so its failure is only logged. | ||
|
Check warning on line 152 in src/mcp/client/sse.py
|
||
|
claude[bot] marked this conversation as resolved.
|
||
| if isinstance(message, types.JSONRPCRequest): | ||
| reply = types.JSONRPCError(jsonrpc="2.0", id=message.id, error=error) | ||
| await read_stream_writer.send(SessionMessage(reply)) | ||
|
|
||
| async for session_message in write_stream_reader: | ||
|
claude[bot] marked this conversation as resolved.
|
||
| sender_ctx = write_stream_reader.last_context | ||
|
claude[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,25 +248,51 @@ | |
| else: | ||
| raise ResumptionError("Resumption request requires a resumption token") # pragma: no cover | ||
|
|
||
| # Extract original request ID to map responses | ||
| original_request_id = None | ||
| if isinstance(ctx.session_message.message, JSONRPCRequest): # pragma: no branch | ||
| original_request_id = ctx.session_message.message.id | ||
| # Only requests resume: post_writer dispatches here on message type as well as | ||
| # metadata, so the original id is always available to map responses. | ||
| assert isinstance(ctx.session_message.message, JSONRPCRequest) | ||
| original_request_id = ctx.session_message.message.id | ||
|
|
||
| async with ctx.client.sse(self.url, headers=headers) as event_source: | ||
| event_source.response.raise_for_status() | ||
| logger.debug("Resumption GET SSE connection established") | ||
| try: | ||
| async with ctx.client.sse(self.url, headers=headers) as event_source: | ||
| if not event_source.response.is_success: | ||
| # Resolve the waiting caller with an error correlated to its request, | ||
| # mirroring `_handle_post_request`: an escaping `HTTPStatusError` would | ||
| # tear down the transport's task group and every stream with it (#2110). | ||
| if event_source.response.status_code == 404 and self.session_id is not None: | ||
| # The GET carried our Mcp-Session-Id, so a 404 is the session-expiry | ||
| # signal reconnect logic keys on - same mapping as the POST path. | ||
|
Check failure on line 264 in src/mcp/client/streamable_http.py
|
||
|
claude[bot] marked this conversation as resolved.
Outdated
|
||
| await self._resolve_abandoned_request( | ||
| ctx.read_stream_writer, original_request_id, "Session terminated", code=INVALID_REQUEST | ||
| ) | ||
| return | ||
| await self._resolve_abandoned_request( | ||
| ctx.read_stream_writer, | ||
| original_request_id, | ||
| "Server returned an error response", | ||
| code=INTERNAL_ERROR, | ||
| ) | ||
| return | ||
| logger.debug("Resumption GET SSE connection established") | ||
|
|
||
| async for sse in event_source: # pragma: no branch | ||
| is_complete = await self._handle_sse_event( | ||
| sse, | ||
| ctx.read_stream_writer, | ||
| original_request_id, | ||
| ctx.metadata.on_resumption_token_update if ctx.metadata else None, | ||
| ) | ||
| if is_complete: | ||
| await event_source.response.aclose() | ||
| break | ||
| async for sse in event_source: | ||
| is_complete = await self._handle_sse_event( | ||
| sse, | ||
| ctx.read_stream_writer, | ||
| original_request_id, | ||
|
Comment on lines
+261
to
+274
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 Pre-existing issue (not introduced by this PR): the automatic Last-Event-ID reconnection GET in Extended reasoning...What the bug is. This PR establishes a cross-path contract — a 404 while a session is held maps to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed this is a real gap — but it's pre-existing, on a path this PR doesn't rewrite ( Generated by Claude Code |
||
| ctx.metadata.on_resumption_token_update if ctx.metadata else None, | ||
| ) | ||
| if is_complete: | ||
| await event_source.response.aclose() | ||
| return | ||
| except Exception: | ||
| logger.debug("Resumption stream ended", exc_info=True) | ||
|
|
||
| # Stream ended without a response, cleanly or mid-read: resolve the waiter, | ||
| # mirroring `_handle_sse_response`, else the caller would hang forever. | ||
| await self._resolve_abandoned_request( | ||
| ctx.read_stream_writer, original_request_id, "resumption stream ended without a response" | ||
| ) | ||
|
|
||
| def _consume_modern_cancellation(self, session_message: SessionMessage) -> bool: | ||
| """Translate an outbound `notifications/cancelled` at 2026; True means "do not POST". | ||
|
|
@@ -556,8 +582,10 @@ | |
| else None | ||
| ) | ||
|
|
||
| # Check if this is a resumption request | ||
| is_resumption = bool(metadata and metadata.resumption_token) | ||
| # Only a request resumes: the token names an interrupted request's | ||
| # stream, and `_handle_resumption_request` needs the id to correlate | ||
| # its outcome. A notification stamped with one is POSTed as usual. | ||
| is_resumption = bool(metadata and metadata.resumption_token) and isinstance(message, JSONRPCRequest) | ||
|
|
||
| logger.debug(f"Sending client message: {message}") | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.