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
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7ef5f8ec6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| yield events | ||
| finally: | ||
| if isinstance(events, _AsyncClosable): | ||
| await events.aclose() |
There was a problem hiding this comment.
When a reader is cancelled—such as during transport teardown or modern request cancellation—AnyIO's level cancellation remains active while this finally block runs, so an aclose() implementation that reaches an async checkpoint is cancelled before cleanup completes. The new tests miss this because their closers perform no checkpoint, while real iterator cleanup commonly awaits nested resources; repeated cancellations can therefore retain stream resources despite the newly documented guarantee. Run the close operation inside a shielded cancellation scope, ideally with an appropriate bound.
Useful? React with 👍 / 👎.
![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 7 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/shared/_httpx_utils.py">
<violation number="1" location="src/mcp/shared/_httpx_utils.py:182">
P2: When an SSE reader is cancelled, `sse_events` awaits `aclose()` inside the already-cancelled AnyIO scope, so asynchronous iterator cleanup can be cancelled and the response resources can leak. Run `events.aclose()` inside a shielded `anyio.CancelScope`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| yield events | ||
| finally: | ||
| if isinstance(events, _AsyncClosable): | ||
| await events.aclose() |
There was a problem hiding this comment.
P2: When an SSE reader is cancelled, sse_events awaits aclose() inside the already-cancelled AnyIO scope, so asynchronous iterator cleanup can be cancelled and the response resources can leak. Run events.aclose() inside a shielded anyio.CancelScope.
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 182:
<comment>When an SSE reader is cancelled, `sse_events` awaits `aclose()` inside the already-cancelled AnyIO scope, so asynchronous iterator cleanup can be cancelled and the response resources can leak. Run `events.aclose()` inside a shielded `anyio.CancelScope`.</comment>
<file context>
@@ -165,6 +166,22 @@ async def sse_within_origin(
+ yield events
+ finally:
+ if isinstance(events, _AsyncClosable):
+ await events.aclose()
+
+
</file context>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes cleanup/cancellation semantics on every client SSE read loop and is one piece of a five-PR Trio-enablement stack, a human look at how it composes with the rest of the stack would still be worthwhile.
What was reviewed:
- The
sse_eventshelper insrc/mcp/shared/_httpx_utils.pyand all five call sites; calling__aiter__()before the redirect/status checks does no eager work since async-generator creation is lazy. - Ordering of
response.aclose()then generatoraclose()on normal completion in_handle_sse_responseand the reconnect loop; closing a generator suspended atyieldonly unwindsGeneratorExit, so no spurious reconnect. - The new backend-parametrized test and the two partial-consumption test fixes;
_module_runner_leaseopt-out and parenthesizedasync withmatch existing repo patterns, and no new pragmas ortype: ignorewere introduced.
Extended reasoning...
Overview
The PR adds a small sse_events async context manager plus a @ runtime_checkable _AsyncClosable protocol to /home/claude/python-sdk/src/mcp/shared/_httpx_utils.py, then wraps all five client-side SSE consumption loops with it: the legacy reader in /home/claude/python-sdk/src/mcp/client/sse.py and the GET stream, resumption GET, POST-SSE response, and reconnect loops in /home/claude/python-sdk/src/mcp/client/streamable_http.py. The transport bodies are otherwise unchanged (re-indentation only), preserving existing # pragma: no branch markers rather than adding new ones. Tests add one backend-parametrized regression in /home/claude/python-sdk/tests/client/test_streamable_http.py that stubs httpx2.EventSource.__aiter__ with a generator, a closable class iterator, and a plain iterator, plus explicit aclose() calls in two existing tests that partially consume async generators. A docs admonition describes the behaviour.
Security risks
None identified. The change does not touch auth, origin checks, redirect handling, or header construction; sse_within_origin and _unfollowed_redirect are still evaluated before any event is consumed, and __aiter__() on an async generator function does not execute the generator body, so no bytes are read from an out-of-origin or error response earlier than before.
Level of scrutiny
Moderate. The helper itself is ten lines and mechanical, but it alters when cleanup runs on every client SSE path, including inside cancelled scopes during transport shutdown. The three candidate issues from the hunt (eager work in __aiter__, generator aclose() after response.aclose() triggering a spurious reconnect, and an extra await inside an already-cancelled scope) were all examined against the code and ruled out for this httpx2 version, but the third depends on what httpx2's generator does in its own finalization, which the PR itself notes is pending an upstream change. Because the PR is explicitly part 4 of a 5-PR stack aimed at Trio enablement, a maintainer familiar with the stack is better placed than an automated pass to judge whether this extraction lands cleanly ahead of the upstream httpx2 fix.
Other factors
No CODEOWNERS file exists in the repo. The _module_runner_lease autouse override follows the same pattern used in eight other test modules, parenthesized multi-item async with is already used in src/mcp/server/sse.py, and typing_extensions is already a runtime dependency imported across the codebase. The isinstance branch in sse_events is exercised on both sides by the parametrized test, consistent with the 100% branch coverage requirement. I could not execute the test suite or inspect the installed httpx2 source in this environment, so the author's coverage and cross-backend claims are unverified here.
1 participant
Extract client SSE iterator cleanup from #3511. Both HTTP transports explicitly close their outer event iterators when
aclose()is supported, accepting generators, closable class-based iterators, and plain async iterators.The compatibility regression runs on both AnyIO backends. This does not claim complete nested HTTPX2 cleanup: HTTPX2 #1212 remains the blocker for full-suite Trio enablement in #3511, not for this extraction's locked-dependency checks.
Validation
macOS, Python 3.14:
./scripts/testpasses with 6,025 passed, 9 skipped, 1 xfailed, 100% line/branch coverage, andstrict-no-coverpassing. Ruff lint/format and Pyright pass.Also validated independently on
main: 5,975 passed with 100% coverage andstrict-no-cover. The three changed test modules pass all 78 cases on Python 3.10.Stack
Part 4 of 5, stacked on #3542. The final backend-enablement PR is #3511.
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.