fix(client): bound McpSubscription.close()'s wait on the cancelled-notification send - #2645
fix(client): bound McpSubscription.close()'s wait on the cancelled-notification send#2645claude[bot] wants to merge 3 commits into
Conversation
…tification send close() serially awaited wireTeardown(), whose notification send can never settle on a transport parked past requestAbort (stdio 'drain' park ignores requestSignal) — hanging await sub.close() forever. Keep waiting so the cancel is on the wire on healthy transports, but cap the wait at 5s; the state machine settles immediately either way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
🦋 Changeset detectedLatest commit: 3d26e8f The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped bounded-wait fix with failing-first fake-timer coverage for both the parked and healthy paths; the one inline finding is a minor nit (guarded .unref() on the bound's timer) that doesn't block. Beyond that, I also checked the new void wireTeardown().finally(...) chain for unhandled rejections — wireTeardown() cannot reject (the notification send is caught internally at client.ts:2066 and requestAbort.abort() doesn't throw), so it holds.
Extended reasoning...
Overview
The PR bounds McpSubscription.close()'s wait on the notifications/cancelled teardown send in packages/client/src/client/client.ts: instead of serially awaiting wireTeardown() (which can park forever on a stdio write stuck on 'drain', since stdio ignores requestSignal), the wait is now capped by a new LISTEN_CLOSE_TEARDOWN_WAIT_MSEC = 5000, with the timer cleared when the send settles. A changeset (patch, @modelcontextprotocol/client) and a new fake-timers describe block in listen.test.ts round out the diff — ~20 lines of production code.
Security risks
None. This is client-side teardown timing; no auth, parsing, or input-handling surfaces are touched. The notification payload and wire behavior are unchanged — only how long close() waits for the send to settle.
Level of scrutiny
Moderate: it changes observable behavior of a public API (close() can now resolve before the cancel is on the wire, after 5s, on a degraded transport), which warrants a careful read of the async flow. I checked the repo's recurring async/lifecycle catches: wireTeardown() catches its own send rejection internally, so the new void ...finally(...) cannot leak an unhandled rejection; the deferred timer callback only resolves a local promise and mutates no this._* state, so the post-close-callback hazard doesn't apply; and the state machine (settle({cause:'local'}), _listenState cleanup, requestAbort.abort()) runs synchronously before the wait, so idempotency and cleanup are preserved regardless of how the wait resolves. The other two wireTeardown() call sites (ack timeout, caller-signal abort) are already fire-and-forget, so no sibling site retains the bug the PR fixes.
Other factors
Tests were written failing-first against main and pin both the degraded path (close resolves at the 5s bound, closed resolves 'local' immediately) and the healthy path (no added latency, cancel on the wire, vi.getTimerCount() === 0). The approach (bounded wait vs fire-and-forget) was the suggestion from the prior automated review of #2642, and the existing test pinning the on-the-wire guarantee is preserved. The single inline finding — the bound's timer isn't unref'd, so a parked teardown can hold an idle Node process open up to 5s — is a real but minor polish item consistent with the stdio.ts convention, and doesn't affect functional correctness; it can be addressed in a follow-up or a quick amend.
In the parked-send case clearTimeout is unreachable and the armed timer held an idle Node process open for the full bound. Guarded unref since client.ts is runtime-neutral (browser timers have no unref), matching the stdio.ts convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
There was a problem hiding this comment.
LGTM — small, well-tested bounded-wait fix, and the timer-unref concern from the previous automated review round is addressed in the latest commit (guarded unref?.() at client.ts:2085). The two inline notes are non-blocking: one is explicitly a pre-existing sibling issue in listen() worth a follow-up, the other a one-sentence JSDoc touch-up.
Extended reasoning...
Overview
The PR caps the wait in McpSubscription.close() on the notifications/cancelled teardown send with a new LISTEN_CLOSE_TEARDOWN_WAIT_MSEC = 5000 bound, so a transport send that never settles (a stdio write parked on 'drain', where requestSignal is ignored) can no longer park await sub.close() forever. Three files: the ~20-line change in packages/client/src/client/client.ts, a fake-timers test block in packages/client/test/client/listen.test.ts (parked-send and healthy-path cases, written failing-first), and a patch changeset.
Security risks
None. No auth, crypto, parsing, or input-handling surface is touched — the change only bounds an internal await inside client-side subscription teardown. The state machine (settle, _listenState cleanup) already settled synchronously before the wait, so no new window where cleanup can be skipped.
Level of scrutiny
Moderate: this is client lifecycle code, but the diff is small and additive — the healthy path is behavior-identical (the bound's timer is cleared on send settlement, pinned by the vi.getTimerCount() === 0 test), and the degraded path only converts an infinite hang into a 5s-capped resolve. The design choice (bounded wait vs fire-and-forget) is justified against an existing test that pins the on-the-wire guarantee, consistent with the #2641/#2642/#2643 robustness set. The finding from the previous automated review round (ref'd timer holding the event loop open) is resolved in the current revision via a runtime-neutral guarded unref?.().
Other factors
The two findings posted inline do not block: the listen() sibling parked-send/unhandled-rejection issue exists verbatim on main and is neither introduced nor worsened here (flagged per the repo's completeness convention as a follow-up candidate), and the stale close() JSDoc is a doc-only nit. Tests, lint, and typecheck are reported green; the changeset correctly targets @modelcontextprotocol/client at patch level and its prose matches the implemented behavior.
| const close = async (): Promise<void> => { | ||
| if (state === 'closed') return; | ||
| settle({ cause: 'local' }); | ||
| await wireTeardown(); | ||
| // Bounded wait: close() waits for the cancelled notification so | ||
| // it is on the wire when close() resolves (transports settle a | ||
| // healthy send quickly), but a send that never settles — a stdio | ||
| // write parked on 'drain' ignores `requestSignal`, so the | ||
| // `requestAbort.abort()` above cannot reach it — must not park | ||
| // close() forever. The bound cuts only the WAIT; the notification | ||
| // stays in flight, and the state machine settled above regardless. | ||
| await new Promise<void>(resolve => { | ||
| const timer = setTimeout(resolve, LISTEN_CLOSE_TEARDOWN_WAIT_MSEC); | ||
| // In the parked-send case clearTimeout below is unreachable, | ||
| // and the armed timer would hold an idle Node process open | ||
| // for the full bound. Guarded: this module is runtime-neutral | ||
| // and browser timers have no unref. | ||
| (timer as { unref?: () => void }).unref?.(); | ||
| void wireTeardown().finally(() => { | ||
| clearTimeout(timer); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| // The per-subscription state is registered BEFORE the request is sent |
There was a problem hiding this comment.
🟣 Pre-existing (not introduced by this PR): the sibling await this.transport.send(...) at line 2137 in this same listen() body has the exact unbounded-wait-on-a-parked-send shape this PR bounds in close() — and worse, because the opening promise (line 2007) has no rejection handler until line 2150, the ack timer firing during a parked send turns into an unhandledRejection (process crash under Node's default --unhandled-rejections=throw). Since this PR already migrates the same pattern in the same function, consider racing the send against opening (with a pre-attached handler) here too, mirroring what this diff did for close().
Extended reasoning...
What the bug is. This PR bounds one await-on-a-parked-send inside listen() — the wireTeardown() wait in close() (the diff hunk at client.ts:2069-2091). A sibling await in the same function body survives with the same bug class: await this.transport.send(jsonrpcRequest, ...) at client.ts:2137 has no bound, and under the exact parked-send premise this PR series is built on (a stdio write parked on 'drain'; stdio ignores TransportSendOptions.requestSignal, so requestAbort.abort() cannot reach it — the PR's own comment at 2074-2076 states this), that await never settles. The catch at 2141-2148 only handles a send that rejects; a send that never settles is precisely the case the PR description distinguishes ("a backpressured write that never fails still parks").\n\nWhy it's worse than a hang. The opening promise is created at line 2007, and the only handler ever attached to it is const honored = await opening at line 2150 — which execution never reaches while suspended at 2137. Every termination path funnels through settle(), which (state still 'opening') calls rejectOpening() at line 2047. Rejecting a promise with no handler attached emits unhandledRejection, which under Node's default --unhandled-rejections=throw crashes the host process. So in the degraded scenario this PR hardens close() against, opening a subscription escalates from "hangs forever" to "kills the process at ack-timeout".\n\nStep-by-step proof.\n1. A stdio pipe backs up; the subscriptions/listen write at line 2137 parks on 'drain' (stdio's send resolves only on the drain event and never reads requestSignal).\n2. listen() is now suspended at 2137. The opening promise (2007) has zero handlers — line 2150 is unreached.\n3. The ack timer (armed at 2099, default 60s) fires: settle({cause:'remote', error: SdkError(RequestTimeout, ...)}) clears the timer, transitions to 'closed', and — since state was 'opening' — calls rejectOpening(error) at 2047.\n4. requestAbort.abort() inside settle() is a no-op on stdio, so the send at 2137 stays parked; execution never advances to 2150.\n5. Node observes a rejected promise with no handler → unhandledRejection → process crash under default flags. Meanwhile the promise listen() returned to the caller never settles, so await client.listen(...) hangs forever regardless — despite the state machine's own "hangs are impossible by construction" comment (~line 1999).\n\nWhy existing safeguards don't cover it. The ack timer, the caller-signal abort (2115-2120), _resetConnectionState's settle loop, and the transport _onclose path all funnel through settle(), which only rejects opening — none of them can unpark the awaited send itself, and none attaches a handler to opening before line 2150. Each of them is therefore an unhandled-rejection trigger in this state, not a rescue. The auto-open path is also affected: connect() with ClientOptions.listChanged awaits this.listen() at line 1261; its try/catch wraps the outer listen() promise (which never settles), so connect() hangs and the inner opening rejection is still the unhandled one.\n\nImpact and fix. Impact: in the same degraded-transport scenario this PR series targets, listen() (and connect() with auto-open) hangs past its own documented ack timeout, and the timeout's arrival crashes the process instead of surfacing an error. Fix shape: attach a handler to opening up front and let settle() unpark listen — e.g. race the send against opening:\n\nts\nconst sendP = this.transport.send(jsonrpcRequest, { ... }).catch(error => {\n settle({ cause: 'remote', error: error instanceof Error ? error : new Error(String(error)) });\n});\nconst honored = await Promise.race([sendP.then(() => opening), opening]);\n\n\n(or simply void opening.catch(() => {}) before the send plus the race). This both bounds listen() by the ack timer and closes the unhandled-rejection window — mirroring what this diff did for close().\n\nWhy this is filed pre-existing. Lines 2007/2137-2150 are untouched by this diff; the bug exists verbatim on main, and merging this PR neither causes nor worsens it — so it should not block merge. It is flagged per the repo's Completeness convention ("partial migrations leave sibling code paths with the very bug the PR claims to fix — flag every leftover site"): this PR modifies the same function for the same root scenario, making it the natural opportunity to fix the surviving site. Note the PR's stated follow-up (stdio requestSignal support) would incidentally address the parked send here, but would not by itself remove the handlerless-opening window on other never-settling transports.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
|
Both review notes handled:
Generated by Claude Code |
There was a problem hiding this comment.
LGTM — and thanks for addressing both earlier notes (the guarded unref on the bound's timer and the close() JSDoc update); both are in the current diff.
What was reviewed this pass: the bounded-wait promise wiring in close() — the late wireTeardown().finally after the timer fires only hits no-op clearTimeout/resolve calls, so no double-settle or stale-state mutation; idempotency of a second close() (the state === 'closed' guard returns before re-arming a wait); and the two new fake-timer tests, which pin both the parked-send bound and the zero-added-latency healthy path. The other two wireTeardown() call sites (ack timeout, caller-signal abort) were confirmed already fire-and-forget and unaffected.
Extended reasoning...
Overview
This PR bounds the wait in McpSubscription.close() on the notifications/cancelled teardown send with a 5s cap (LISTEN_CLOSE_TEARDOWN_WAIT_MSEC), so a transport send that never settles (stdio write parked on 'drain', which ignores requestSignal) can no longer hang await sub.close() forever. Three files: the changeset, ~30 lines in packages/client/src/client/client.ts (constant + JSDoc + the bounded-wait promise in close()), and two new fake-timer tests in listen.test.ts. The subscription state machine still settles synchronously before the wait, so only the caller-visible resolution timing changes.
Security risks
None. No auth, crypto, input parsing, or network-surface changes — purely client-side teardown timing for the subscriptions/listen handle.
Level of scrutiny
Moderate: this is async/lifecycle code in the client, a category the repo's review conventions flag for deferred-callback hazards. I checked the specific hazard: when the 5s timer fires first and close() resolves, a later-settling wireTeardown() runs clearTimeout (no-op on a fired timer) and resolve() (no-op on a settled promise) — no state mutation, no double-settle. A second close() call returns via the state === 'closed' guard without re-arming a wait. The timer is .unref()'d via a runtime-neutral guarded call, matching the sibling bounded-teardown waits in stdio.ts. The other two wireTeardown() call sites (ack-timeout at client.ts:2107, caller-abort at client.ts:2122) are already fire-and-forget with .catch(() => {}), so they are unaffected.
Other factors
Both notes from the earlier automated review rounds are resolved in this PR's head: the ref'd-timer/process-exit-delay issue (commit c59d696) and the stale close() JSDoc (commit 3d26e8f). The third, purple-flagged pre-existing issue in listen()'s serial send is explicitly out of scope — it is the subject of #2642, and the author verified the hunks are disjoint. Tests were written failing-first per the description; the new fake-timer block covers both the degraded path (close resolves at the bound, closed resolves 'local' immediately) and the healthy path (no added latency, vi.getTimerCount() === 0 pins no leaked timer). The existing 'cancel on the wire when close() resolves' test continues to pin healthy-path behavior. The changeset accurately describes the new semantics. This is a well-scoped patch-level bugfix with all review feedback incorporated; no design decision remains open.
1 participant
Requested by Felix Weinberger · Slack thread
Follow-up to #2642, addressing the non-blocking finding from its automated review. Part of the #2641/#2643 robustness set.
Root cause
McpSubscription.close()(packages/client/src/client/client.ts) serially awaitswireTeardown(), which ends inthis.notification({ method: 'notifications/cancelled', ... })— an un-abortabletransport.send. Under the same parked-send condition that motivates #2641 (a stdio write parked on'drain'; stdio ignoresTransportSendOptions.requestSignal, so therequestAbort.abort()at the top ofwireTeardowncannot reach it), that send never settles andawait sub.close()hangs forever. #2552 alone would not fix this: it rejects failed writes, but a backpressured write that never fails still parks, and the cancel is sent with no signal afterrequestAborthas already fired.The other two
wireTeardown()call sites (ack timeout, caller-signal abort) are already fire-and-forget and are not affected.Fix
Per the review's suggestion, the gentler bounded wait rather than fire-and-forget: one existing test (
close() sends notifications/cancelled referencing the listen id on any transport) pins that the cancel is on the wire whenclose()resolves, and fire-and-forget would make that racy.close()still awaitswireTeardown(), but the wait is capped by a newLISTEN_CLOSE_TEARDOWN_WAIT_MSEC = 5000:close(); the notification stays in flight, and the subscription's state machine (closedresolving'local',_listenStatecleanup) settled synchronously before the wait either way.Tests
Fake-timers
describeblock inpackages/client/test/client/listen.test.ts, written failing-first againstmain(cc4b416):sub.closedresolves'local'immediately,close()is still pending at t=0, and resolves once the 5s bound elapses (fails onmain:close()never resolves).vi.getTimerCount() === 0(the bound's timer was cleared) — pins that the bound adds no latency and no leaked timer (passes before and after).pnpm --filter @modelcontextprotocol/client test: 33 files, 799 tests passed. Lint (ESLint + Prettier) and typecheck clean. Changeset included (patch,@modelcontextprotocol/client).No overlap with #2642's diff (different function in the same file; the hunks are disjoint, so the two PRs merge cleanly in either order). The remaining transport-level piece of the trio is stdio
requestSignalsupport (complementing #2552), not included here.Generated by Claude Code