Rebuild the streamable HTTP server transport around per-request dispatch - #3192
Rebuild the streamable HTTP server transport around per-request dispatch#3192maxisbey wants to merge 8 commits into
Code review found 4 potential issues
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 4 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/mcp/server/streamable_http.py:1004-1030 |
_respond_sse lost its exception containment: catch-all re-sends a 500 after the SSE response started |
| 🟡 Nit | src/mcp/server/streamable_http.py:357-385 |
EventStore exceptions escape _MessageChannel.write into no-raise / handler-exception paths |
| 🟡 Nit | src/mcp/server/streamable_http.py:962-981 |
Stateful POST racing session teardown is served inline as if stateless |
| 🟡 Nit | docs/migration.md:751-759 |
migration.md omits the fourth clarified behaviour (concurrent POSTs sharing a JSON-RPC id) |
Annotations
Check warning on line 1030 in src/mcp/server/streamable_http.py
claude / Claude Code Review
_respond_sse lost its exception containment: catch-all re-sends a 500 after the SSE response started
The rewritten `_respond_sse` wraps `await response(scope, receive, send)` in try/finally with no `except`, so an ASGI send failure racing a client disconnect now propagates to `_handle_post_request`'s catch-all, which sends a fresh 500 — a second `http.response.start` after the SSE response already began (an ASGI protocol violation that escapes as "Exception in ASGI application" noise). The GET path and `_replay_events` kept their log-only `except Exception` around the same call in this file; re
Check warning on line 385 in src/mcp/server/streamable_http.py
claude / Claude Code Review
EventStore exceptions escape _MessageChannel.write into no-raise / handler-exception paths
Exceptions raised by the user-supplied `EventStore.store_event` propagate out of `_MessageChannel.write` into three call sites that assume it cannot raise: (1) the `send_cancel` closure here violates `RequestCorrelator.call`'s own contract ('it must swallow its own write failures'), replacing the documented `MCPError(REQUEST_TIMEOUT)` on the timeout arm and raising a fresh exception out of the caller-cancel arm; (2) `_StandaloneOutbound.notify` breaks `Connection.notify`'s documented 'Never rais
Check warning on line 981 in src/mcp/server/streamable_http.py
claude / Claude Code Review
Stateful POST racing session teardown is served inline as if stateless
In `_serve_request` (and `_spawn_or_run`), `self._task_group is None` is used to mean "stateless mode", but on a stateful transport `_task_group` is also `None` after `run()` exits during teardown — so a POST that passed the `_terminated` check and then suspended (e.g. `await request.body()`, or the event store's `store_event` in `_mint_priming_event`) can resume after a DELETE-triggered `terminate()` or idle-timeout teardown and be served inline via the session runner, on a `Connection` whose `
Check warning on line 759 in docs/migration.md
claude / Claude Code Review
migration.md omits the fourth clarified behaviour (concurrent POSTs sharing a JSON-RPC id)
The "Behaviour clarified in the same change" list in docs/migration.md has only three bullets, but the PR description enumerates four clarified behaviours and states they are all documented here — the fourth (two concurrent POSTs sharing a JSON-RPC id each keep their own response stream instead of the second silently taking over the first's queue, pinned by `test_concurrent_posts_reusing_a_request_id_each_receive_their_own_response`) is missing. Add the missing bullet so the migration guide matc