From b66194debd69adf8027e32e5b2103a6128cec459 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 05:37:44 +0000 Subject: [PATCH 1/3] fix(client): bound McpSubscription.close()'s wait on the cancelled-notification send MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa --- .changeset/listen-close-bounded-teardown.md | 5 ++ packages/client/src/client/client.ts | 24 +++++++- packages/client/test/client/listen.test.ts | 68 +++++++++++++++++++++ 3 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 .changeset/listen-close-bounded-teardown.md diff --git a/.changeset/listen-close-bounded-teardown.md b/.changeset/listen-close-bounded-teardown.md new file mode 100644 index 0000000000..bebea3b4ce --- /dev/null +++ b/.changeset/listen-close-bounded-teardown.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': patch +--- + +Bound the wait in `McpSubscription.close()`. It serially awaited the `notifications/cancelled` teardown send, so a transport send that never settles (e.g. a stdio write parked on `'drain'`, which ignores `requestSignal`) left `await sub.close()` hanging forever. `close()` still waits for the notification so it is on the wire when it resolves on healthy transports, but the wait is now capped (5s); the subscription's state machine settles immediately either way. diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index 0b386a63e8..8f4ef2b9af 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -435,6 +435,15 @@ const LIST_CHANGED_EVICTIONS: Readonly> = { const DEFAULT_LIST_MAX_PAGES = 64; +/** + * Upper bound on how long `McpSubscription.close()` waits for the + * `notifications/cancelled` teardown send before resolving anyway. A healthy + * transport settles the send in well under this; the bound exists so a send + * that never settles (e.g. a stdio write parked on `'drain'`, which ignores + * `requestSignal`) cannot park `close()` forever. See #2641/#2643. + */ +const LISTEN_CLOSE_TEARDOWN_WAIT_MSEC = 5000; + /** * A handle to an open `subscriptions/listen` stream (protocol revision * 2026-07-28). Change notifications delivered on the stream dispatch to the @@ -2060,7 +2069,20 @@ export class Client extends Protocol { const close = async (): Promise => { 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(resolve => { + const timer = setTimeout(resolve, LISTEN_CLOSE_TEARDOWN_WAIT_MSEC); + void wireTeardown().finally(() => { + clearTimeout(timer); + resolve(); + }); + }); }; // The per-subscription state is registered BEFORE the request is sent diff --git a/packages/client/test/client/listen.test.ts b/packages/client/test/client/listen.test.ts index 60f96be47a..375d894ebd 100644 --- a/packages/client/test/client/listen.test.ts +++ b/packages/client/test/client/listen.test.ts @@ -953,6 +953,74 @@ describe('_resetConnectionState() clears connection-scoped debounce timers (fake }); }); +describe('McpSubscription.close() — bounded teardown wait (fake timers)', () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it('close() resolves within the bound when the cancelled-notification send never settles', async () => { + const { clientTx } = await scriptedModern(); + const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } }); + const connecting = client.connect(clientTx); + await vi.runAllTimersAsync(); + await connecting; + + const subPromise = client.listen({ toolsListChanged: true }); + await vi.advanceTimersByTimeAsync(0); + const sub = await subPromise; + + // Every subsequent send parks forever — models a stdio pipe backed up + // on 'drain' (stdio send() ignores requestSignal, so the teardown's + // requestAbort cannot reach it). + clientTx.send = () => new Promise(() => {}); + + let closeSettled = false; + const closing = sub.close().then(() => { + closeSettled = true; + }); + // The state machine settles immediately even though the wire teardown + // is parked. + await expect(sub.closed).resolves.toBe('local'); + await vi.advanceTimersByTimeAsync(0); + expect(closeSettled).toBe(false); // still waiting on the parked send + // …but the wait is bounded: close() resolves once the bound elapses. + await vi.advanceTimersByTimeAsync(5000); + await closing; + expect(closeSettled).toBe(true); + + vi.useRealTimers(); + await client.close(); + }); + + it('a healthy close() still resolves with the cancelled notification already on the wire (no added latency)', async () => { + const { clientTx, written } = await scriptedModern(); + const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } }); + const connecting = client.connect(clientTx); + await vi.runAllTimersAsync(); + await connecting; + + const subPromise = client.listen({ toolsListChanged: true }); + await vi.advanceTimersByTimeAsync(0); + const sub = await subPromise; + written.length = 0; + + let closeSettled = false; + const closing = sub.close().then(() => { + closeSettled = true; + }); + // No timer advance beyond microtask draining: a settling send must + // resolve close() without waiting out any part of the bound. + await vi.advanceTimersByTimeAsync(0); + await closing; + expect(closeSettled).toBe(true); + expect(written.some(m => (m as { method?: string }).method === 'notifications/cancelled')).toBe(true); + // The bound's timer was cleared — nothing left armed. + expect(vi.getTimerCount()).toBe(0); + + vi.useRealTimers(); + await client.close(); + }); +}); + describe('Client.listen() — ack timeout (fake timers)', () => { beforeEach(() => vi.useFakeTimers()); afterEach(() => vi.useRealTimers()); From c59d6963c9362d7d6940cdff3eaf4ec7d379d6e3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 05:50:56 +0000 Subject: [PATCH 2/3] fix(client): unref the bounded close() teardown timer 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 Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa --- packages/client/src/client/client.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index 8f4ef2b9af..945d343766 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -2078,6 +2078,11 @@ export class Client extends Protocol { // stays in flight, and the state machine settled above regardless. await new Promise(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(); From 3d26e8fe5197bde42e8ff0063cffdd03dd75e888 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 06:02:37 +0000 Subject: [PATCH 3/3] docs(client): note the bounded teardown wait on McpSubscription.close() Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa --- packages/client/src/client/client.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index 945d343766..de5f91eb71 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -459,7 +459,10 @@ export interface McpSubscription { * Tears the subscription down. Idempotent. Aborts the listen request's * stream (where the transport supports it) AND sends * `notifications/cancelled` referencing the listen request id — both, - * always, so close works on any transport. + * always, so close works on any transport. The wait for the + * cancelled-notification send is capped (~5s); on a transport whose send + * never settles, `close()` resolves with the notification still in + * flight. */ close(): Promise; /**