Skip to content

Commit 47e156d

Browse files
committed
Rework channel liveness, stream ids, and init ordering after review
The second review round found several defects that traced back to a few structural gaps rather than isolated bugs; this addresses the gaps. - Channel delivery is now a value: write() returns whether the message reached the event store or an attached response, an EventStore failure is contained inside the write (resumability degrades, the stream survives, no store text on the wire), and attach() returns None on a dead channel so nothing can stream from one. A server-to-client request that cannot reach any client fails the caller with CONNECTION_CLOSED instead of parking it, and the JSON-response body derives from the channel's recorded outcome (result, terminated-404, or 500) rather than a "cannot happen" branch. - Stream ids handed to the EventStore are minted by the transport in a session-scoped namespace, so a client-chosen request id can no longer name the standalone GET stream and two sessions on one store can no longer replay each other's frames. - One SSE-response runner owns the pump, error containment, and cleanup for the POST, GET, and replay responses (the POST path had lost the guard its siblings kept). - A session-level gate holds requests that arrive while an initialize is still being served until the handshake commits, restoring the ordering the stream-pair driver's parked read loop used to guarantee. - The POSTed client message is delivered even when the 202 could not be written back, and the correlator marks the single site where the cancelled-request answer policy lives for every transport. Adds regression tests for each of the above.
1 parent 1e3be94 commit 47e156d

7 files changed

Lines changed: 659 additions & 143 deletions

File tree

docs/migration.md

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -744,10 +744,14 @@ transport = StreamableHTTPServerTransport(mcp_session_id=session_id, ...)
744744
async with transport.connect() as (read_stream, write_stream):
745745
await server.run(read_stream, write_stream, server.create_initialization_options())
746746

747-
# After (v2): let the manager own transports (mount it or use streamable_http_app())
748-
session_manager = StreamableHTTPSessionManager(app=server, event_store=..., json_response=...)
747+
# After (v2): serve the app the SDK builds ...
748+
app = server.streamable_http_app(event_store=..., json_response=...)
749749
```
750750

751+
... or, when composing your own Starlette/FastAPI app, mount a `StreamableHTTPSessionManager` and
752+
enter `session_manager.run()` in the lifespan — see [Mounting the ASGI app](run/asgi.md) for the
753+
full wiring.
754+
751755
Behaviour clarified in the same change:
752756

753757
- In JSON-response mode a *request-scoped* server-to-client request (`ctx.elicit()`, or any
@@ -757,6 +761,18 @@ Behaviour clarified in the same change:
757761
`related_request_id`) are unchanged and still ride the standalone GET stream.
758762
- A GET carrying `Last-Event-ID` on a server without an `EventStore` opens the standalone stream
759763
as a plain GET would, since there is nothing to replay.
764+
- Two concurrent POSTs that share a JSON-RPC request id each keep their own response stream; the
765+
second no longer silently takes over the first's queue.
766+
- Stream ids handed to your `EventStore` are minted by the transport in its own session-scoped
767+
namespace (previously the raw `str(request_id)` and a single global GET-stream key), so two
768+
sessions sharing one store no longer collide, and a `Last-Event-ID` replay only releases frames
769+
of the requesting session's own streams. Treat the ids as opaque.
770+
- A failing `EventStore.store_event` degrades resumability for that message rather than taking
771+
the stream down: the message is still delivered live (with no event id to resume from) and the
772+
store's exception is logged, never sent to the client.
773+
- A server-to-client request that can reach no client at all (no attached stream and nothing
774+
storing it, or a request-scoped one in JSON-response mode) fails the calling handler with
775+
`CONNECTION_CLOSED` instead of parking it for an answer that cannot arrive.
760776

761777
### `MCPServer.get_context()` removed
762778

src/mcp/server/runner.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,12 @@ async def _inner(ctx: ServerRequestContext[LifespanT, Any]) -> HandlerResult:
230230
result = _dump_result(await call(ctx))
231231
if method == "initialize":
232232
# Commit only on chain success, so a middleware veto leaves no state.
233-
# Race-free: the read loop is parked until this call returns.
233+
# Race-free for the session's first handshake: the transport runs no
234+
# other request until it returns (a stream driver's read loop is
235+
# parked here; streamable HTTP holds later requests behind the
236+
# in-progress initialize, and the session id only ships with its
237+
# response). A repeated initialize on an established session (a
238+
# recorded divergence) recommits alongside whatever is running.
234239
# TODO: this re-reads the wire `params`, so a middleware that rewrote
235240
# `ctx.params` (or `ctx.method`, or short-circuited without `call_next`)
236241
# can leave `connection.protocol_version` out of step with the

src/mcp/server/streamable_http.py

Lines changed: 243 additions & 127 deletions
Large diffs are not rendered by default.

src/mcp/shared/_correlation.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,11 @@ async def serve_inbound(
448448
# result write above did not happen - no double response.
449449
# TODO(L38): spec says SHOULD NOT respond after cancel;
450450
# the existing server always has, so match that for now.
451+
# This is the single site every transport shares for the
452+
# cancelled-request answer policy; change it here for all
453+
# of them (a transport whose response stream must still end
454+
# sees this write, so a suppressed answer needs the stream
455+
# terminated another way).
451456
answer_write_started = True
452457
await write_error(ErrorData(code=0, message="Request cancelled"))
453458
except anyio.get_cancelled_exc_class():

tests/interaction/README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,8 @@ but still inside an outer `async with`, and no restructure can avoid it.
280280

281281
A handful of `# pragma: lax no cover` markers in `src/` cover teardown exception handlers whose
282282
execution is timing-dependent under the in-process HTTP bridge — the `except Exception` arms
283-
around the standalone-GET and replay `response(...)` calls in `server/streamable_http.py`.
283+
around the SSE-response runner (`_run_sse_response`) and the replay entry path in
284+
`server/streamable_http.py`.
284285
`strict-no-cover` does not check `lax` lines; do not
285286
promote them to strict `no cover` without first making the teardown ordering deterministic. The
286287
suite also relies on a one-line `src/mcp/server/sse.py` fix (`sse_stream_reader.aclose()`) that

tests/server/test_streamable_http_manager.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@ async def mock_receive():
423423
assert transport._terminated, "Transport should be terminated after stateless request"
424424

425425
# Verify internal state is cleaned up: no request streams left open.
426-
assert not transport._channels, "Transport should have no active request channels"
426+
assert not transport._streams, "Transport should have no active request streams"
427427
assert not transport._standalone.attached, "Transport should have no standalone stream attached"
428428

429429

0 commit comments

Comments
 (0)