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 and verified against the latest diff
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/shared/_httpx_utils.py">
<violation number="1" location="src/mcp/shared/_httpx_utils.py:176">
P2: When an SSE consumer stops early, this closes only the outer `EventSource` iterator; HTTPX2's nested SSE/text iterators remain unclosed and still produce cleanup warnings. Close each nested iterator as well, or add a compatible HTTPX2 workaround before treating this helper as deterministic cleanup.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| yield events | ||
| finally: | ||
| assert isinstance(events, AsyncGenerator) | ||
| await events.aclose() |
There was a problem hiding this comment.
P2: When an SSE consumer stops early, this closes only the outer EventSource iterator; HTTPX2's nested SSE/text iterators remain unclosed and still produce cleanup warnings. Close each nested iterator as well, or add a compatible HTTPX2 workaround before treating this helper as deterministic cleanup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/shared/_httpx_utils.py, line 176:
<comment>When an SSE consumer stops early, this closes only the outer `EventSource` iterator; HTTPX2's nested SSE/text iterators remain unclosed and still produce cleanup warnings. Close each nested iterator as well, or add a compatible HTTPX2 workaround before treating this helper as deterministic cleanup.</comment>
<file context>
@@ -165,6 +165,17 @@ async def sse_within_origin(
+ yield events
+ finally:
+ assert isinstance(events, AsyncGenerator)
+ await events.aclose()
+
+
</file context>
There was a problem hiding this comment.
Confirmed; this remains the dependency blocker listed in the PR description. The helper closes the SDK-owned outer iterator, and its docstring now makes that scope explicit rather than implying that HTTPX2's nested iterators are closed. Leaving this thread open until the upstream cleanup is addressed, without traversing dependency internals or suppressing its resource warnings.
AI-assisted review.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
tests/server/test_streamable_http_router.py— nit: the new raw-ASGI test at tests/server/test_streamable_http_router.py:899 does not say why the public API could not drive it, which the test-quality skill requires for raw-ASGI peers. Its neighbour at line 812 does state the reason. Fix: add the one-line justification (a stalledreplay_events_afterneeds a hand-builtscope/receive/sendso the GET never completes) to the docstring, matching the sibling test.Extended reasoning...
The conventions block says raw-stream and raw-ASGI peers are a last resort and the docstring must say why the public API could not do it.
test_transport_shutdown_cancels_a_router_waiting_for_replaybuilds a rawscopedict, areceivethat sleeps forever and a list-collectingsend, and calls…Verification: nit. Triggering condition: whenever this test is read/triaged under the repo's test-quality rule — nothing runtime breaks. Note the candidate's line numbers are wrong: the file is 295 lines; the test in question,
test_transport_shutdown_cancels_a_router_waiting_for_replay, starts at /home/claude/python-sdk/tests/server/test_streamable_http_router.py:252 and its sibling… | nit. Triggering… -
🟣
src/mcp/server/streamable_http.py— Servers usingclose_standalone_sse_stream()for polling can still see a client's freshly resumed GET stream torn down by the previous GET's cleanup, forcing an extra reconnect. The standalone writer is never put in_sse_stream_writers, soclose_sse_stream(GET_STREAM_KEY)at src/mcp/server/streamable_http.py:276-284 closes only the request streams; a writer blocked at :800 on a slow client keeps running, and when it finally exits its finally at :808 calls_clean_up_memory_streams(GET_STREAM_KEY)unconditionally, closing whatever replay_sender registered at :972 in the meantime. The PR made the POST and replay finally blocks own-only (:379, :989) but left this sibling unconditional. …Extended reasoning...
…Fix: make :808 skip cleanup unless
_request_streams[GET_STREAM_KEY]is still the stream this writer created, matching the other two sites.The dismissing finder assumed no checkpoint between the close and the check at :464 because the writer wakes immediately from the closed reader. That only holds when the writer is parked in the async for at :792. When it is parked at :800 in sse_stream_writer.send (0-buffer, waiting for EventSourceResponse to drain to a slow or half-dead client) nothing wakes it, because close_standalone_sse_stream never closes this writer (no entry in _sse_stream_writers). Sequence: server calls close_standalone_sse_stream; _request_streams[GET_STREAM_KEY] is popped and closed at :281-284. The client, whose old connection is dead or timed out, reconnects with Last-Event-ID; replay_sender at :967-978 finds GET_STREAM_KEY absent, registers a new stream and writer, sends the priming event. Later the old GET's socket is detected dead, EventSourceResponse cancels the writer task, and :808 runs _clean_up_memory_streams(GET_STREAM_KEY): the check at :464 now finds…
Verification: pre-existing. Trigger: a server using the documented polling API
close_standalone_sse_stream()while the standalone GET writer is parked insse_stream_writer.send()(client not draining / half-dead TCP), and the client then reopens GET with Last-Event-ID before the old writer task exits. Mechanism verified in /home/claude/python-sdk/src/mcp/server/streamable_http.py: - The standalone writer…
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 5 findings from earlier reviews are still open above.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Still open from earlier reviews (5):
- 🔴
src/mcp/server/streamable_http.py:939—Servers using event_store= get a session-wide outbound freeze whenever a resuming client stops reading; the base branch… - Also unresolved: 4 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| @asynccontextmanager | ||
| async def sse_events(source: httpx2.EventSource) -> AsyncGenerator[AsyncIterator[httpx2.ServerSentEvent]]: | ||
| """Close the outer EventSource iterator if supported; HTTPX2 owns its nested iterators.""" | ||
| events = source.__aiter__() | ||
| try: | ||
| yield events | ||
| finally: | ||
| if isinstance(events, _AsyncClosable): | ||
| await events.aclose() |
There was a problem hiding this comment.
Shouldn't httpx2 own this?
1 participant
Run the suite on both
asyncioandtrio, close SSE iterators by capability, preserve request-stream ownership, and buffer replay before network delivery. Direct subscription cleanup is bounded, remote subscription exit does not wait for cancellation writes, and normal transport shutdown lets active event-store writes finish.Blocker
Not ready to merge: waiting for HTTPX2 #1212, which closes the remaining nested response iterators, to merge and be released. Then update the SDK's HTTPX2 minimum and lockfile; the locked-dependency suite still fails on cleanup, and its resource warnings remain errors.
Testing against that PR's actual source eliminates the async-generator warnings (11,964 passed); two oversized-replay test helpers also need adjustment for the default SSE event-size limit introduced after the SDK's HTTPX2 2.5.0 pin.
Trio's Linux-only
pidfdencoding warning is filtered only intrio._subprocess, where the descriptor is never read as text.Validation
With an isolated, uncommitted HTTPX2 cleanup patch:
./scripts/testpasses with 11,966 passed, 17 skipped, 2 xfailed, 100% line/branch coverage, andstrict-no-coverpassing.Without the HTTPX2 patch, all 48 cases in
tests/server/test_streamable_http_router.pyalso pass with minimum AnyIO 4.9.0 and Trio 0.26.2, including all eight request-ID reuse cases.Linux stdio was also verified for the earlier encoding-warning change without the HTTPX2 patch: 89 passed, 7 skipped with both locked Trio 0.31.0 and minimum Trio 0.26.2.
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.