Fail fast on server-to-client requests in JSON-response mode instead of hanging - #3195
Conversation
📚 Documentation preview
|
In stateful JSON-response mode a tool handler calling ctx.elicit() (or any server-to-client request tied to the in-flight call) hung forever: the loop dispatcher reported can_send_request=True, the request was routed into the call's own per-request queue, and the JSON POST drain loop silently discarded it, so the parked waiter never woke. The transport now supplies each message's TransportContext (StreamableHTTPServerTransport.transport_context_for): it owns what a request's response can carry, so can_send_request is False in JSON-response mode and the request raises NoBackChannelError at once - the same fail-fast the stateless path and the 2026-07-28 entries already have. serve_loop takes the builder as a keyword and the session manager passes it for both the stateful and stateless legs. The session's standalone GET stream is untouched and still carries unrelated messages.
In JSON-response mode a message tied to an in-flight request has no wire form and no POST could replay it, so message_router now drops it before storing or queueing rather than leaving it for a consumer to discard. The per-request queue then carries exactly the one response, and the JSON POST handler collapses from a discard loop to a single receive with a real arm for a session torn down mid-request. This removes the four coverage pragmas the discard loop needed.
…lder The JSON-response-mode refusal of request-scoped server requests takes effect through the dispatcher's transport_builder, so a caller driving the transport's streams by hand needs to pass transport_context_for the way the session manager does. No-Verification-Needed: docstring-only edit
41f2302 to
c8a4bb1
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/mcp/server/streamable_http.py:584-604— In JSON-response mode, a POST whose JSON-RPC id is the literal string "_GET_stream" registers its per-request stream under the same key as the standalone GET stream (GET_STREAM_KEY), so an unrelated server message (e.g.ctx.session.send_resource_updated(), a null-id error) can be routed into it — and the new singlereceive()serializes that non-response message as the HTTP body with noJSONRPCResponse | JSONRPCErrorcheck, dropping the real response. The key aliasing is pre-existing, but the removed drain loop used to skip non-responses, so returning a wrong body is new to this PR; a cheap guard (loop until a response arrives, or reject request ids equal toGET_STREAM_KEY) closes it.Extended reasoning...
The bug. The JSON-mode POST handler now does a single
await request_stream_reader.receive()and hands whatever arrives straight toself._create_json_response(event_message.message)with no check that the message is aJSONRPCResponse | JSONRPCError. The comment above thereceive()asserts the invariant thatmessage_routerdeposits only this request's own response here — but that invariant has a hole: per-request stream keys share a flat string namespace with the standalone-stream sentinelGET_STREAM_KEY = "_GET_stream"(line 62), andrequest_id = str(message.id)(line 575) is taken verbatim from the client's JSON-RPC id.RequestIdadmits arbitrary strings, and nothing anywhere validates or reserves the sentinel value.\n\nThe code path. A JSON-mode POST with"id": "_GET_stream"registers_request_streams["_GET_stream"]— the exact keymessage_routeruses for unrelated traffic (request_stream_id = target_request_id if target_request_id is not None else GET_STREAM_KEY). The PR's new router drop branch only fires whenrelated_request_id is not None, so it does not protect this path: messages with no id and no related id —ctx.session.send_resource_updated(), standalone-channel server messages, null-id errors (the router comment explicitly says these "fall through to the GET stream") — still route toGET_STREAM_KEY, i.e. into the parked POST's queue.\n\nStep-by-step proof. Take a statefuljson_response=Trueserver and this PR's own smoke-serverannouncetool (which callsctx.session.send_resource_updated(...)before returning):\n1. Client POSTs{"jsonrpc":"2.0","id":"_GET_stream","method":"tools/call","params":{"name":"announce",...}}.\n2._handle_post_requestsetsrequest_id = "_GET_stream"and registers_request_streams["_GET_stream"], then parks on the singlereceive().\n3. The tool runs;send_resource_updatedemits aJSONRPCNotificationwith no id and norelated_request_id. The router computesrequest_stream_id = GET_STREAM_KEYand deposits it into the parked POST's stream.\n4.receive()returns the notification;_create_json_responseserializes it, so the POST's JSON body is aJSONRPCNotification— not a response to the request (protocol violation; client-side parse confusion).\n5. The tool's realJSONRPCResponse(id"_GET_stream") reaches the router after thefinally: _clean_up_memory_streams(request_id)closed and removed the stream, so it is silently dropped. If a standalone GET stream was open, its_request_streamsentry was also clobbered at step 2 and it starves.\n\nWhy this is new to the PR. The removed drain loop'selsearm ("For notifications and requests, keep waiting") skipped non-responses and kept receiving until the real response arrived, so before this PR the identical collision still returned the correct body. The sentinel-key aliasing itself (and the GET-stream-entry hijack) is pre-existing and identical in SSE mode — only the wrong-body/dropped-response symptom is introduced here.\n\nWhy nit, not blocking. The trigger requires a client to deliberately choose the literal string"_GET_stream"as its JSON-RPC id — no conforming or accidental client does this — plus an unrelated server message in flight during that exact call. The damage is confined to the misbehaving client's own session (a mangled body for a request it crafted, plus starving its own GET stream, which was already possible pre-PR). It's an adversarial-robustness edge worth hardening, not a normal-operation failure.\n\nFix options. Any of: (a) in the JSON branch, loop until the received message is aJSONRPCResponse | JSONRPCError(restores the old safeguard cheaply); (b) reject inbound request ids equal toGET_STREAM_KEYbefore registering the stream; (c) key the standalone stream outside_request_streamsso no client-controlled id can alias it.
| * `host` / `port`: where to listen. Defaults `127.0.0.1` and `8000`. | ||
| * `streamable_http_path`: where the MCP endpoint lives. Default `/mcp`. | ||
| * `json_response=True`: answer with plain JSON instead of an SSE stream. | ||
| * `json_response=True`: answer each POST with a single JSON body instead of an SSE stream. That body has room for the response and nothing else, so a tool that calls back into the client mid-request (`ctx.elicit()`, sampling) raises `NoBackChannelError` on this leg, and notifications tied to the in-flight call (progress from `ctx.report_progress()`, per-call log messages) are dropped; the standalone `GET` stream still carries unrelated ones. |
There was a problem hiding this comment.
🟡 The new json_response=True bullet's parenthetical "(ctx.elicit(), sampling)" over-claims for sampling: the default sampling API (ctx.session.create_message()) sends with related_request_id=None, so it rides the connection's standalone channel — which never consults the can_send_request flag this PR wires up — and still routes to the GET stream (or silently hangs if none is attached, the out-of-scope case the PR description acknowledges) rather than raising NoBackChannelError. Only request-scoped calls (ctx.elicit(), Resolve-driven sampling, or an explicit related_request_id) get the new fail-fast; consider dropping "sampling" or qualifying it as request-scoped sampling.
Extended reasoning...
What the doc claims vs. what the code does
The new bullet in docs/run/index.md says that under json_response=True "a tool that calls back into the client mid-request (ctx.elicit(), sampling) raises NoBackChannelError on this leg". That is accurate for ctx.elicit() / ctx.elicit_url() and for Resolve-driven sampling, because those stamp related_request_id=ctx.request_id and therefore travel the request-scoped DispatchContext, whose can_send_request this PR sets to False in JSON-response mode. It is not accurate for the default sampling API.
The code path
ServerSession.create_message() defaults related_request_id=None, and ServerSession.send_request() selects the channel with channel = self._request_outbound if related is not None else self._connection.outbound (src/mcp/server/session.py:88-89). With no related id, sampling rides the connection's standalone channel. For a stateful loop session, Connection.for_loop installs the JSONRPCDispatcher itself as that outbound, and its send_raw_request never consults can_send_request — only the per-message DispatchContext does (src/mcp/shared/jsonrpc_dispatcher.py:167). The mcpserver Context has no sampling helper that stamps a related id (only elicit/elicit_url do, src/mcp/server/mcpserver/context.py:185,218), so the ordinary way a tool samples — ctx.session.create_message(...) — goes standalone by default.
Why the new guards don't fire on this path
Both new mechanisms in this PR are keyed to the request-scoped leg. The transport_context_for fail-fast is only checked in DispatchContext.send_raw_request, which the standalone path bypasses. And the new message_router drop branch in src/mcp/server/streamable_http.py requires related_request_id is not None before it fires; a sampling request with no related id falls through to GET_STREAM_KEY as before.
Step-by-step proof
- Deploy a stateful server with
json_response=Trueand a tool that callsawait ctx.session.create_message([...], max_tokens=10)mid-request. - Connect a legacy (
2025-11-25) client that has not opened the standalone GET stream, and call the tool. create_messagebuildsServerMessageMetadata(related_request_id=None)(session.py:266) →send_requestpicksself._connection.outbound→ the dispatcher writes the request and parks a waiter, with nocan_send_requestcheck anywhere on that path.message_routersees aJSONRPCRequestwith no related id →target_request_idstaysNone→ routes toGET_STREAM_KEY. No GET stream is registered, so the message is logged and dropped.- The waiter never wakes; the tool call and the POST stall indefinitely — the exact standalone-channel hang the PR description explicitly lists as out of scope. The developer who trusted the doc line expected an immediate
NoBackChannelError.
(If the client has a GET stream attached, the request is delivered there instead — also not the documented NoBackChannelError.)
Why this is a nit
Nothing in the code is wrong — the PR's actual behavior change is correct, tested, and matches its description. The inaccuracy is one word in one new doc bullet; the neighboring legacy-clients.md note and troubleshooting.md entries are correctly scoped to the request-scoped channel. All verifiers who traced this agreed.
Suggested fix
Drop "sampling" from the parenthetical, or qualify it, e.g.: "a tool that calls back into the client on the request-scoped channel (ctx.elicit(), sampling with a related request id)".
Instead of the transport supplying a `transport_builder` that the session manager has to ferry into the dispatcher, the transport now states its verdict on the message it already emits: ServerMessageMetadata grows `can_send_request`, which StreamableHTTPServerTransport stamps as `mcp_session_id is not None and not is_json_response_enabled`, and the dispatcher's default builder honors it. This drops the public transport_context_for method, the serve_loop keyword, and the manager's stateless special-case builder: both legs are now correct with no wiring at all, and so is a transport driven by hand. The metadata already carries transport facts the dispatcher consumes (related_request_id, on_request_unanswered), so the verdict follows an existing channel rather than opening a new one.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The previous commit had the transport also refuse whenever it holds no session id, but a missing session id is not what makes a client's reply unroutable - the session manager creating a fresh transport per request is. A single long-lived sessionless transport still receives the reply on the same instance, so server-initiated requests over SSE kept working there and must continue to. The transport now states only the fact it owns (a JSON body carries one response), and the manager keeps its own can_send_request=False builder for the stateless leg. The manager module is unchanged from main.
| await writer.send(session_message) | ||
| try: | ||
| # Process messages from the request-specific stream | ||
| # We need to collect all messages until we get a response | ||
| response_message = None | ||
|
|
||
| # Use similar approach to SSE writer for consistency | ||
| async for event_message in request_stream_reader: # pragma: no branch | ||
| # If it's a response, this is what we're waiting for | ||
| if isinstance(event_message.message, JSONRPCResponse | JSONRPCError): | ||
| response_message = event_message.message | ||
| break | ||
| # For notifications and requests, keep waiting | ||
| else: # pragma: no cover | ||
| logger.debug(f"received: {event_message.message.method}") | ||
|
|
||
| # At this point we should have a response | ||
| if response_message: | ||
| # Create JSON response | ||
| response = self._create_json_response(response_message) | ||
| await response(scope, receive, send) | ||
| else: # pragma: no cover | ||
| # This shouldn't happen in normal operation | ||
| logger.error("No response message received before stream closed") | ||
| response = self._create_error_response( | ||
| "Error processing request: No response received", | ||
| HTTPStatus.INTERNAL_SERVER_ERROR, | ||
| ) | ||
| await response(scope, receive, send) | ||
| except Exception: # pragma: no cover | ||
| logger.exception("Error processing JSON response") | ||
| # `message_router` deposits only this request's own response | ||
| # here: anything else scoped to the request has no wire in | ||
| # JSON-response mode. | ||
| event_message = await request_stream_reader.receive() | ||
| except (anyio.EndOfStream, anyio.ClosedResourceError): | ||
| # The stream closed with no response: the session was | ||
| # terminated while this request was in flight. | ||
| logger.debug(f"Session terminated with request {request_id} in flight; no response to send") | ||
| response = self._create_error_response( | ||
| "Error processing request", | ||
| "Session terminated before the request completed", | ||
| HTTPStatus.INTERNAL_SERVER_ERROR, | ||
| INTERNAL_ERROR, | ||
| ) | ||
| await response(scope, receive, send) | ||
| else: | ||
| response = self._create_json_response(event_message.message) | ||
| finally: | ||
| await self._clean_up_memory_streams(request_id) | ||
| await response(scope, receive, send) |
There was a problem hiding this comment.
🟡 A JSON-mode POST whose request id is the literal string "_GET_stream" registers its per-request queue under the same key the router uses for unrelated server messages, and the collapsed single-receive() path (which dropped the old loop's isinstance(JSONRPCResponse | JSONRPCError) guard) will then return the first unrelated notification/request as the HTTP 200 body while the real response is discarded in cleanup. Before this PR the drain loop skipped non-response messages, so the wrong-response-body observable is new here. Cheap hardening: loop until the received message is a JSONRPCResponse | JSONRPCError, or reject/namespace request ids equal to GET_STREAM_KEY.
Extended reasoning...
The bug
GET_STREAM_KEY = "_GET_stream" (src/mcp/server/streamable_http.py:62) lives in the same _request_streams dict namespace as client-chosen request ids, and JSON-RPC ids may be arbitrary strings (RequestId = int | str). Nothing validates a client id against the sentinel: _handle_post_request computes request_id = str(message.id) and registers _request_streams[request_id] unconditionally (SESSION_ID_PATTERN covers session ids only; jsonrpc_message_adapter accepts any string id). So a POST with "id": "_GET_stream" in JSON mode registers its response queue under the exact key message_router uses for unrelated server messages.
The code path
In message_router, an unrelated message (e.g. ctx.session.send_resource_updated(...), a tools/list_changed broadcast, or a standalone server request) has related_request_id is None, so it skips both the response branch and this PR's new JSON-mode drop branch (which is guarded by related_request_id is not None). It falls through to request_stream_id = GET_STREAM_KEY — which now names the colliding POST's own queue — and is deposited there (buffer size 16, so timing is not delicate). The PR's own test_json_response_streamable_http_delivers_only_unrelated_notifications demonstrates that unrelated messages survive the router in JSON mode via exactly this fall-through.
Why it's new to this PR
The removed drain loop (old lines ~590–612) checked isinstance(event_message.message, JSONRPCResponse | JSONRPCError) and log-and-skipped everything else, so pre-PR the same id collision still produced the correct JSON body: the stray unrelated message was discarded and the loop kept waiting for the real response. The collapsed path does one await request_stream_reader.receive() and unconditionally response = self._create_json_response(event_message.message) — the comment's invariant ("message_router deposits only this request's own response here") has this one hole, because the drop branch only covers related messages.
Impact
The client's POST is answered HTTP 200 whose body is a JSONRPCNotification (or a server-initiated JSONRPCRequest) — a protocol-violating response for its call. The finally then runs _clean_up_memory_streams("_GET_stream"), so when the handler's real response is routed moments later the stream is gone and the actual result is silently dropped. If a standalone GET stream was open, its registration is clobbered as collateral. The existing GET handler's 409 check guards a second GET, not a POST squatting the key.
Step-by-step proof
- Stateful server,
json_response=True. Client POSTs{"jsonrpc":"2.0","id":"_GET_stream","method":"tools/call","params":{"name":"announce"}}— a legal JSON-RPC string id;request_id = str(message.id) = "_GET_stream"and_request_streams["_GET_stream"]is created (streamable_http.py:614). - The
announcehandler runsawait ctx.session.send_resource_updated("file:///watched.txt")— an unrelated notification withrelated_request_id=None. message_router: not a response, not related → the new JSON-mode drop branch does not fire →request_stream_id = GET_STREAM_KEY = "_GET_stream", which IS in_request_streams→ the notification is deposited into the POST's queue.- The POST handler's
await request_stream_reader.receive()returns the notification;_create_json_responseserializes it as the 200 body — the client's "response" is{"jsonrpc":"2.0","method":"notifications/resources/updated",...}with noid/result. finallytears down_request_streams["_GET_stream"]; the handler's realJSONRPCResponsearrives at the router afterwards, finds no stream, and is dropped.
Why nit
Three independent verifiers confirmed the full chain and all rated it nit: it requires an adversarial or fuzzing client to deliberately choose the SDK-internal sentinel as its request id, plus an unrelated server message racing ahead of the response, and the damage is confined to that client's own session — no well-behaved client can hit it and there is no cross-client or security impact. Worth closing with a cheap guard, but not merge-blocking.
Fix
Either re-add the response-type guard in the JSON receive path (loop until isinstance(event_message.message, JSONRPCResponse | JSONRPCError), discarding anything else), or keep the collapsed single-receive and instead reject/namespace request ids that stringify to GET_STREAM_KEY so the sentinel can never collide with a client id.
Route every ServerMessageMetadata the transport frames through a single _message_metadata factory so the can_send_request stamp has one home a future construction site cannot forget. The field becomes a plain bool defaulting to True, dropping the None sentinel and the branch that read it. Docstrings now defer to TransportContext.can_send_request for the full rule instead of restating it, and the two direct-transport tests share one fake-ASGI scaffold.
1 participant
With
json_response=Trueon a stateful streamable-HTTP server, a tool handler that callsctx.elicit(...)(or any server-to-client request tied to the in-flight call) hangs forever. This makes it fail fast withNoBackChannelErrorinstead, and makes the JSON-mode per-request queue carry only what a JSON body actually can.Motivation and Context
The hang, traced end to end:
serve_loop, whoseJSONRPCDispatcheris built with notransport_builder, so every message got the defaultTransportContext(kind="jsonrpc", can_send_request=True). JSON-response mode didn't change that; only the stateless path saidFalse.Context.elicitstampsrelated_request_id=self.request_id, soServerSession.send_requestpicks the request-scoped channel.can_send_requestwasTrue, so the request was written withrelated_request_id=<tool call id>and a waiter parked in_pending.message_routerroutes it into_request_streams[str(tool_call_id)]— the tool call's own per-request queue.elsearm was# pragma: no cover— no test ever pushed a request through it).call_toolhangs client-side too.This is not a regression: the chain is byte-for-byte the same on
v1.x(the drain loop's discard arm dates to #553, requests started ridingrelated_request_idin #693, elicitation was born routing that way in #625). No SDK can deliver a server-initiated request inside a JSON response body — it's structurally one JSON object. The TypeScript SDK drops it (by design, modelcontextprotocol/typescript-sdk#299 and modelcontextprotocol/typescript-sdk#866) but is bounded by its 60s default request timeout; Go reroutes to the standalone GET stream and errors if none is attached. The intent was already written down here —TransportContext.can_send_requestwas documented as "False for … JSON response mode" andNoBackChannelError's docstring said the same — but the stateful path never wired it.The fix.
The transport owns what a request's response can carry, and it already attaches
ServerMessageMetadatato every message — the existing channel for transport-supplied, per-message facts the dispatcher consumes (related_request_id,close_sse_stream, and #3188'son_request_unanswered). So the verdict rides that channel:ServerMessageMetadatagainscan_send_request, whichStreamableHTTPServerTransportstampsFalsein JSON-response mode (the one fact it owns), and the dispatcher's default builder honors it. Effect:ctx.elicit()in stateful JSON mode raisesNoBackChannelErrorimmediately — anINVALID_REQUESTerror at the client — the same fail-fast the stateless path and both 2026-07-28 entries already had. The session manager and its stateless leg are untouched: statelessness ("a fresh transport per request, so the client's reply has nowhere to land") stays the manager's fact, applied by its existingcan_send_request=Falsebuilder, and a merely-sessionless long-lived transport keeps working over SSE as before. No driver-layer or manager wiring is needed at all, and a hand-driven JSON-mode transport is correct by construction. The session's standaloneGETstream is untouched — unrelated notifications still ride it — and the two intent docstrings are tightened to say "request-scoped channel" rather than "no channel", since the connection does keep one. (An earlier revision on this branch reached the same behavior by threading a builder callback throughserve_loop; the metadata form replaces it — no driver-layer parameter and no new transport API.)Then, with requests stopped at the source,
message_routeralso drops request-scoped non-response messages before storing or queueing in JSON mode (they have no wire form and no POST could replay them). The per-request queue carries exactly the one response, the JSON POST handler collapses from a discard loop to a singlereceive()with a real arm for a session torn down mid-request, and the four coverage pragmas the discard loop needed go away rather than being tested around. Its-32800cancellation terminator from #3188 is unaffected: it is keyed by the request id, so it takes the router's response arm and lands in the queue as the one response.How Has This Been Tested?
New tests:
test_json_response_streamable_http_rejects_request_scoped_server_requests— statefuljson_response=True+ a tool that doesctx.elicit(...), underanyio.fail_after(5); asserts a top-levelMCPErrorwithINVALID_REQUEST. Verified it fails at the 5s bound on unpatchedsrc/and passes in well under a second with the fix.test_json_response_streamable_http_delivers_only_unrelated_notifications— the call's own log notification is dropped, the unrelatedresources/updatedstill arrives over the standalone stream, the result comes back as the JSON body.test_json_post_answers_500_when_session_terminates_mid_request— drives the transport directly and tears the session down mid-request; the JSON POST answers a clean 500.test_transport_stamped_can_send_request_makes_the_request_channel_refuse— a message whose metadata sayscan_send_request=Falsegets a request-scoped channel that raisesNoBackChannelError, with no driver wiring.New requirement id
transport:streamable-http:json-response-restrictions. Full suite: 5559 passed, 100% branch coverage,strict-no-coverclean.Also driven live against a real uvicorn-served
MCPServerover raw HTTP: the elicitingtools/callreturns the-32600no-back-channel body in ~0.00s with the fix and hits a curl 8s timeout with zero bytes on the unpatched source;echo, a notification-emitting tool, JSON-onlyAccept, and stateless mode all behave as before; the default SSE control leg still streamselicitation/createas the first event; and a cancelled slow call in JSON mode completes its POST with-32800.Breaking Changes
None to the API. One behavior changes: a
json_response=Truetool that used to stall inctx.elicit()now gets an immediateNoBackChannelError(INVALID_REQUESTat the client). Documented indocs/migration.md(in the existingNoBackChannelErrorsection),docs/run/index.md,docs/run/legacy-clients.md, anddocs/troubleshooting.md.Types of changes
Checklist
Additional context
Two related shapes were surfaced while tracing this and are deliberately out of scope here — they share the symptom (a parked waiter nothing wakes) but not the cause (
can_send_requestcan't express them, since undeliverability is decided at routing time, not context construction). Happy to file them separately: a standalone-channel request (ctx.session.elicit_form()with no related id,list_roots(),send_ping()) hangs whenever noGETstream is attached, in JSON and SSE mode; and in SSE mode a client that disconnects mid-call leaves the handler running with its later request-scoped writes landing nowhere (the modern path haswatch_disconnect, the legacy path doesn't).AI Disclaimer