Skip to content

Rebuild the streamable HTTP server transport around per-request dispatch - #3192

Open
maxisbey wants to merge 8 commits into
mainfrom
shttp-native-dispatch
Open

Rebuild the streamable HTTP server transport around per-request dispatch#3192
maxisbey wants to merge 8 commits into
mainfrom
shttp-native-dispatch

Inline the streamable-http transport-context kind literal

edb0abd
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jul 27, 2026 in 32m 27s

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

See this annotation in the file changed.

@claude 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

See this annotation in the file changed.

@claude 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

See this annotation in the file changed.

@claude 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

See this annotation in the file changed.

@claude 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