Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/streamable_http.py">
<violation number="1" location="src/mcp/server/streamable_http.py:662">
P2: When an older SSE handler calls `ctx.close_sse_stream()` after a later POST reuses its request ID, it closes the successor's streams. Bind the callback to the request's stream ownership and close it only if that owner is still registered.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| logger.exception("SSE response error") | ||
| await sse_stream_writer.aclose() | ||
| await self._clean_up_memory_streams(request_id) | ||
| self._sse_stream_writers[request_id] = sse_stream_writer |
There was a problem hiding this comment.
P2: When an older SSE handler calls ctx.close_sse_stream() after a later POST reuses its request ID, it closes the successor's streams. Bind the callback to the request's stream ownership and close it only if that owner is still registered.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/streamable_http.py, line 662:
<comment>When an older SSE handler calls `ctx.close_sse_stream()` after a later POST reuses its request ID, it closes the successor's streams. Bind the callback to the request's stream ownership and close it only if that owner is still registered.</comment>
<file context>
@@ -679,40 +657,38 @@ async def _handle_post_request(self, scope: Scope, request: Request, receive: Re
- logger.exception("SSE response error")
- await sse_stream_writer.aclose()
- await self._clean_up_memory_streams(request_id)
+ self._sse_stream_writers[request_id] = sse_stream_writer
+ self._request_streams[request_id] = request_streams
+ headers = {
</file context>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, two other things were checked and ruled out: the new module-scoped anyio_backend fixture in tests/server/test_streamable_http_router.py does not trip the ScopeMismatch that tests/conftest.py warns about for _module_runner_lease (the lease's dependency stays module-scoped, unlike the function-scoped parametrize pattern); and the SSE-mode POST now leaving its registry entry in place while the response drains is harmless, since the router's send hits the closed reader and the identity-checked cleanup removes only that entry, so a same-ID successor is never evicted.
Extended reasoning...
Two confirmed findings are posted inline, so this note only records what else was examined. The trio concern from tests/conftest.py:65-70 was investigated: the conftest docstring's ScopeMismatch warning applies to function-scoped anyio_backend parametrization, but this module defines a module-scoped anyio_backend fixture, so the module-scoped lease can depend on it; the direct-trio.run Windows caveat does not apply because the anyio plugin drives both backends. The extended dead-registry window during SSE response drain in src/mcp/server/streamable_http.py (POST handler finally at the end of the SSE branch) was traced: _run_sse_writer closes the request reader when it breaks on the response, a later router send raises BrokenResourceError, and _clean_up_memory_streams deletes the entry only if it is the same object, so a newer request reusing the ID is unaffected and any message for the finished request is dropped exactly as it was on the base branch (where it hit the not-found path). This is a concurrency-sensitive transport change with a first inline finding about close_sse_stream() that a human should weigh, so the decision is defer rather than approve.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/mcp/server/streamable_http.py— Servers that call ctx.close_sse_stream() from a long-running handler can still kill a newer request's SSE stream after request-ID reuse, so that newer client sees its stream end with no response. close_sse_stream at src/mcp/server/streamable_http.py:270-278 pops _sse_stream_writers and _request_streams by ID with no identity check, unlike every implicit cleanup this PR reworked. The new docs note promises "cleanup from an older connection does not close the newer request's streams", which this path breaks. Fix: make close_sse_stream (and the callback minted at line 316-317) only close the streams the calling request registered, e.g. capture the request's own stream tuple and writer in the closure and apply the sameisidentity check before popping. [also at: src/mcp/server/streamable_http.py:934 - Sessions can leave a resumed GET connection open forever, even after terminate() or DELETE, because its streams are unreachable from any registry.; src/mcp/server/streamable_http.py:663 - A client that reuses a request id while the earlier request is still open now keeps that earlier SSE connection open until the client disconnects, with no server-side release. streamable_http.py:663 overwrites _request_streams[request_id] unconditionally; the earlier handler's streams leave the…]Extended reasoning...
Client sends POST id=7 in SSE mode; handler is slow and later calls ctx.close_sse_stream() to poll. Before it does, the client (or a reconnected client in the same session) sends a new POST id=7. Line 664 registers the new request's sse writer and streams under "7", overwriting the old ones. Old handler now calls close_sse_stream("7"). Line 270 pops the NEW request's sse writer and closes it. Line 276-278 pops and closes the NEW request's memory streams. New POST's _run_sse_writer gets ClosedResourceError at 366, its response ends with no JSON-RPC response. When the old handler finally…
Verification: pre-existing. Triggering condition: a client reuses JSON-RPC request ID "7" while the earlier handler for "7" is still running (the dispatcher blind-overwrites duplicate inbound ids, src/mcp/shared/jsonrpc_dispatcher.py:588-591 "TODO(maxisbey): duplicate ids blind-overwrite"), and that earlier handler later calls ctx.close_sse_stream(). Mechanism verified:… | pre-existing. Trigger: a second…
| message, request, request_id, protocol_version | ||
| ) | ||
| await writer.send(session_message) | ||
| except Exception: # pragma: lax no cover |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: nit: AGENTS.md forbids except Exception: outside top-level handlers. The restructured POST SSE branch (line 684) and standalone GET handler (line 785) re-emit except Exception: # pragma: lax no cover blocks nested inside the request handlers' own outer except Exception as err catch-all, so they are not the top-level handler. These are pre-existing blocks moved/re-indented with their cleanup bodies trimmed (the diff also removed one except Exception from the old _clean_up_memory_streams). Fix: since cleanup now lives in finally, drop the inner catch-all and let the outer handler log, or narrow it to the specific errors EventSourceResponse/writer.send raise (e.g. anyio.BrokenResourceError, anyio.ClosedResourceError, OSError). …
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
…Same instruction at 2 sites (src/mcp/server/streamable_http.py:684, src/mcp/server/streamable_http.py:785).
Nothing fails at runtime. The instruction guards against catch-alls that swallow programmer errors and hide which exceptions a block really expects. Here the inner blocks only log and fall through to finally cleanup, so behaviour is the same as letting the outer except Exception as err handler log it; the only cost is a duplicated catch-all with a lax no cover pragma that coverage does not verify. Pre-existing in substance: both lines existed at the base and were re-indented when the diff wrapped them in a new try/finally.
Verification: AGENTS.md at base (Exception Handling section) reads verbatim «- FORBIDDEN except Exception: - unless in top-level handlers». The diff re-emits, as added lines, except Exception: # pragma: lax no cover at src/mcp/server/streamable_http.py:684 (inside _handle_post_request's SSE branch, nested within the method's outer except Exception as err: at line 693, so not the top-level…
1 participant
Extract server stream ownership and cancellation cleanup from #3511. Each HTTP request closes its own streams, so an older connection cannot remove a successor's streams after request-ID reuse; legacy SSE also closes all owned memory streams.
Replay locking and buffering are left to the next PR. The router regressions run on both
asyncioandtriowithout enabling Trio across the suite or requiring an HTTPX2 update.Validation
macOS, Python 3.14, locked dependencies:
./scripts/testpasses with 5,981 passed, 9 skipped, 1 xfailed, 100% line/branch coverage, andstrict-no-coverpassing. Ruff lint/format and Pyright pass.Stack
Part 1 of 5, based on
main. Merge the focused fixes before the final backend-enablement change in #3511.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.