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 3 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/client/subscriptions.py">
<violation number="1" location="src/mcp/client/subscriptions.py:289">
P2: When direct handler cleanup outlives this five-second scope, `drive` remains a child of `ClientSession._task_group`, so closing the session still waits indefinitely for that shielded handler. Bound or detach the outstanding driver during session shutdown as well, so the timeout actually prevents an uncooperative cleanup from blocking client exit.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| driver_scope.cancel() | ||
| # Only direct drivers own handler cleanup; remote courtesy writes remain session-owned. | ||
| if isinstance(dispatcher, DirectDispatcher): | ||
| with anyio.move_on_after(5, shield=True): |
There was a problem hiding this comment.
P2: When direct handler cleanup outlives this five-second scope, drive remains a child of ClientSession._task_group, so closing the session still waits indefinitely for that shielded handler. Bound or detach the outstanding driver during session shutdown as well, so the timeout actually prevents an uncooperative cleanup from blocking client exit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/subscriptions.py, line 289:
<comment>When direct handler cleanup outlives this five-second scope, `drive` remains a child of `ClientSession._task_group`, so closing the session still waits indefinitely for that shielded handler. Bound or detach the outstanding driver during session shutdown as well, so the timeout actually prevents an uncooperative cleanup from blocking client exit.</comment>
<file context>
@@ -241,42 +242,51 @@ async def listen(
+ driver_scope.cancel()
+ # Only direct drivers own handler cleanup; remote courtesy writes remain session-owned.
+ if isinstance(dispatcher, DirectDispatcher):
+ with anyio.move_on_after(5, shield=True):
+ await driver_done.wait()
finally:
</file context>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| route.settle("local") | ||
| driver_scope.cancel() | ||
| # Only direct drivers own handler cleanup; remote courtesy writes remain session-owned. | ||
| if isinstance(dispatcher, DirectDispatcher): |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Users driving an in-process server over memory streams (modern protocol, JSON-RPC framed) keep the re-listen slot race the PR fixes only for DirectDispatcher. The isinstance(dispatcher, DirectDispatcher) gate at src/mcp/client/subscriptions.py:288 skips the wait for every JSONRPCDispatcher, including in-memory ones where the handler cleanup is just as local and cheap to await. A sequential re-listen against max_subscriptions=1 can be rejected with "Subscription limit reached". Fix: gate on transport locality or on a dispatcher capability (e.g. an attribute meaning 'handler runs in-process'), not on the concrete DirectDispatcher class, so in-memory stream sessions get the same bounded wait.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
Population: tests and apps using in-memory JSON-RPC sessions (e.g. mcp.shared.memory helpers or ClientSession(dispatcher=JSONRPCDispatcher(...)) over anyio memory streams) with a 2026-07-28 server.
Caller exits a listen block; :285-286 settle and cancel driver_scope; :288 isinstance check is False; exit returns at once.
The courtesy notifications/cancelled is written by the drive task later; the server's ListenHandler finally at src/mcp/server/subscriptions.py:236-239 releases the slot only when the server processes that cancel.
Caller immediately re-listens; server at src/mcp/server/subscriptions.py:193-194 still counts the old stream and raises MCPError "Subscription limit reached".
The dismissing finder cited mode='legacy', which cannot listen at all (ListenNotSupportedError at :224-225), so its population statement was wrong; the real population is modern-protocol in-memory stream sessions.
Base behaved the same, but this PR is the deliberate design decision on exit semantics and picks a class check rather than a locality check.
Remedy: key the wait on an in-process…
Verification: pre-existing. Trigger: an in-memory JSON-RPC session (e.g. JSONRPCDispatcher over anyio memory streams / mcp.shared.memory.create_client_server_memory_streams) against a ListenHandler at its max_subscriptions cap, sequentially re-listening after exit, on the trio backend. Mechanism verified: src/mcp/client/subscriptions.py:285-290 settles the route, cancels driver_scope, and waits on…
1 participant
Extract subscription cleanup from #3511. Direct connections wait up to five seconds for handler cleanup, including when the closing task is cancelled; remote subscription exits leave courtesy cancellation writes to the session-owned driver instead of waiting for them.
Keep the behavior, regression tests, and documentation together. Slot reuse and cross-task cancellation run on both AnyIO backends; timeout and blocked-write cases use Trio's mock clock.
Validation
macOS, Python 3.14:
./scripts/testpasses with 6,019 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 subscription module passes all 37 cases on Python 3.10.Stack
Part 3 of 5, stacked on #3541 for a focused review diff. The subscription change is independent of the preceding server fixes and does not require an HTTPX2 update.
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.