fix(client): stop listen() rejections escaping as unhandledRejection during an in-flight send - #2642
fix(client): stop listen() rejections escaping as unhandledRejection during an in-flight send#2642claude[bot] wants to merge 1 commit into
Conversation
…during an in-flight send listen() suspended on transport.send() before awaiting the opening state-machine promise, so any settle() rejection that fired while the send was still pending (ack timeout, transport close, server cancel, caller abort) had no handler attached and escaped as a process-level unhandledRejection; a send that never settled also left listen() suspended forever. listen() now suspends on 'opening' directly and routes send failures into the state machine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DThnF41VH9fXQPjGmRySxa
🦋 Changeset detectedLatest commit: 1edbafd 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 — a well-scoped fix: listen() now suspends on the opening state machine instead of serially awaiting transport.send(), routing send failures into the existing idempotent settle() funnel. The send is still initiated synchronously before await opening, so wire ordering, the pre-registered _listenState entry, and the sync-ack race guard are unchanged; every prior termination path yields the same outcome. The one inline comment is a pre-existing sibling issue on unchanged lines (close() awaiting an un-abortable send), suitable as a follow-up alongside #2552 and not blocking here.
Extended reasoning...
Overview
The PR touches one function (Client.listen() in packages/client/src/client/client.ts), plus a new failing-first test block in packages/client/test/client/listen.test.ts and a patch changeset. The production delta is ~20 lines: instead of await transport.send(...) before await opening, the send promise gets .catch(routeSendFailure) attached (with a try/catch preserving the synchronous-throw path) and listen() suspends directly on the opening promise. This fixes both the process-level unhandledRejection (any settle path firing during an in-flight send previously rejected a handler-less promise) and the permanent hang when a send never settles (e.g. a stdio write parked on 'drain').
Correctness analysis
Reading the full listen() body confirms the invariants the PR description claims: settle() is guarded by state === 'closed' and transitions exactly once, so a late send rejection after an ack-timeout/close/cancel settle is a no-op; the send is still initiated synchronously inside listen() so the sync-ack race guard (_listenState registered before send) is untouched; and routeSendFailure cannot itself throw, so the floating send promise always has a handler and introduces no new escape path. On the success path the outcome is byte-for-byte equivalent — a send rejection after the ack transition funneled into settle({cause:'remote'}) before and still does. The four new tests cover the parked-send × {ack-timeout, transport-close, caller-abort} matrix plus an async-rejection behavior-preservation control, each with a process-level unhandledRejection capture.
Security risks
None — no auth, crypto, input parsing, or wire-format changes; this is client-side promise-wiring within an existing state machine.
Level of scrutiny
This is core client protocol code, so I read the surrounding state machine (settle, wireTeardown, ack timer, caller-signal listener, _onnotification demux) rather than just the hunk. The change is confined to how the returned promise is wired, with all termination semantics funneled through the pre-existing settle(); behavior deltas exist only in previously-pathological cases (which formerly hung or escaped). The existing listen test suite (~30 cases) exercises every termination path and is unchanged, which bounds regression risk well.
Other factors
The single bug-hunt finding is explicitly pre-existing on unchanged lines — McpSubscription.close() serially awaits wireTeardown()'s un-abortable cancel notification and retains the same hang class under a parked send. It predates the PR, is not widened by it, and is a reasonable follow-up alongside #2552 (which the PR description already names); it does not block this fix. Changeset prose matches the implementation. Tests could not be executed in this checkout (node_modules not installed), so correctness verification is by reading; the author reports the full client suite (805 tests) passing.
| // The send is NOT serially awaited: `opening` must be the promise | ||
| // listen() suspends on so that a settle() firing while the send is | ||
| // still in flight (ack timeout, transport close, server cancel, a | ||
| // stdio send parked on 'drain') rejects a promise whose handler is | ||
| // already attached — otherwise the rejection escapes as a | ||
| // process-level unhandledRejection the caller cannot prevent, and a | ||
| // send that never settles leaves listen() suspended forever even | ||
| // though the ack timer already fired. | ||
| const routeSendFailure = (error: unknown): void => { | ||
| // Send failure (including a per-request abort fired before | ||
| // response headers — `streamableHttp._send` rethrows with onerror | ||
| // suppressed). `settle()` is idempotent so a locally-aborted send | ||
| // hitting this path after `close()` is a no-op. | ||
| settle({ cause: 'remote', error: error instanceof Error ? error : new Error(String(error)) }); | ||
| }; | ||
| try { | ||
| await this.transport.send(jsonrpcRequest, { | ||
| requestSignal: requestAbort.signal, | ||
| onRequestStreamEnd: () => settle({ cause: 'remote', error: new Error('subscriptions/listen: stream ended') }) | ||
| }); | ||
| this.transport | ||
| .send(jsonrpcRequest, { | ||
| requestSignal: requestAbort.signal, | ||
| onRequestStreamEnd: () => settle({ cause: 'remote', error: new Error('subscriptions/listen: stream ended') }) | ||
| }) | ||
| .catch(routeSendFailure); | ||
| } catch (error) { | ||
| // Synchronous OR awaited send failure (including a per-request | ||
| // abort fired before response headers — `streamableHttp._send` | ||
| // rethrows with onerror suppressed). `settle()` is idempotent so | ||
| // a locally-aborted send hitting this path after `close()` is a | ||
| // no-op. | ||
| settle({ cause: 'remote', error: error instanceof Error ? error : new Error(String(error)) }); | ||
| // A synchronous throw from send() (before it returns a promise). | ||
| routeSendFailure(error); | ||
| } | ||
|
|
||
| const honored = await opening; |
There was a problem hiding this comment.
🟣 Pre-existing issue (unchanged lines, same state machine): McpSubscription.close() retains the same hang class this PR fixes for listen() — it serially await wireTeardown(), which ends in an un-abortable transport.send of notifications/cancelled, so under the same parked-send condition the new tests model (stdio write parked on 'drain'), await sub.close() suspends forever. Consider mirroring the fire-and-forget pattern the ack-timeout and caller-abort paths already use, or bounding the await — here or as a follow-up alongside #2552.
Extended reasoning...
The sibling path: close() still awaits a send that may never settle
This PR fixes listen() so it suspends on the opening state machine instead of the send promise — a send parked forever no longer hangs the caller. But the McpSubscription handle that listen() returns keeps the same hang class in its close() path, inside the very closure this PR edits:
const wireTeardown = async (): Promise<void> => {
requestAbort.abort();
await this.notification({ method: 'notifications/cancelled', params: { requestId: listenId } }).catch(() => {});
};
const close = async (): Promise<void> => {
if (state === 'closed') return;
settle({ cause: 'local' });
await wireTeardown(); // <-- serial await on an un-abortable send
};Protocol.notification() → _notificationViaCodec ends in await this._transport.send(jsonrpcNotification, options) (the debounce fast-path doesn't apply — notifications/cancelled carries params and debouncedNotificationMethods defaults to []). The .catch(() => {}) only guards rejection; it does nothing for a promise that never settles.
Concrete walk-through under the PR's own trigger scenario
- Stdio child stops reading stdin; the write buffer fills;
write()returnsfalse;StdioClientTransport.send()parks on'drain'indefinitely (stdio ignoresTransportSendOptions.requestSignal, per this PR's own comments and fix(client): reject stdio send() when the write fails instead of waiting for 'drain' #2552). - Every subsequent write parks too — including
wireTeardown'snotifications/cancelledwrite. - Caller runs
await sub.close().settle({ cause: 'local' })completes all observable local teardown (clears_listenState, ack timer, resolvessub.closedwith'local'). await wireTeardown()then suspends on the parked send forever. The promiseclose()returned never resolves, hanging the caller's cleanup/shutdown code exactly the waylisten()used to hang.
Why nothing existing covers it
- The other two locally-initiated teardown paths — the ack-timeout branch and the caller-signal-abort branch — deliberately use fire-and-forget
void wireTeardown().catch(() => {}). Only the user-facingclose()serially awaits, and since failures are swallowed anyway, the await conveys no error signal to the caller. - The named follow-up (fix(client): reject stdio send() when the write fails instead of waiting for 'drain' #2552, making stdio honor
requestSignal) would not fix this:wireTeardownsends the cancel notification with no signal, afterrequestAborthas already been aborted. - This matches the repo's Completeness recurring catch: a PR fixing a failure mode shouldn't leave a sibling path in the same function/state machine with the very hang it eliminates.
Suggested fix
Mirror the fire-and-forget pattern (settle(...) already completes all observable local teardown before the await), or bound the await with a timeout race if there's value in usually confirming the cancel reached the wire before close() resolves. One existing test ("close() sends notifications/cancelled referencing the listen id") relies on the cancel being on the wire when close() resolves, so a bounded race may be the gentler shape.
Why pre-existing severity
The close()/wireTeardown lines are byte-for-byte untouched by this diff; the hang predates the PR and isn't widened by it. It's worth a follow-up (or an opportunistic fix here alongside #2552) rather than a blocker.
|
The automated review's non-blocking finding — Generated by Claude Code |
1 participant
Requested by Felix Weinberger · Slack thread
Fixes #2641.
Root cause
Client.listen()(packages/client/src/client/client.ts) creates theopeningpromise for itsopening → open → closedstate machine, arms the ack timer and caller-signal listener, and then serially awaitstransport.send(...)before ever reachingawait opening. Any settle path that fires while the send is still pending — ack timeout, transport close, inbound server cancel, caller-signal abort,_resetConnectionStateon reconnect — callsrejectOpening(...)on a promise with no handler attached:unhandledRejectionthat caller-side handling cannot prevent (a.catch()on thelisten()promise does not help, becauselisten()is still suspended inside the send). This matches the reported production events.StdioClientTransport.send()parks indefinitely on'drain'under backpressure and ignoresTransportSendOptions.requestSignal(the overlapping fix(client): reject stdio send() when the write fails instead of waiting for 'drain' #2552 covers the write-failure half of that) —listen()also hangs forever, even though its own ack timer already fired.Fix
listen()now suspends onopeningdirectly. The send is fired without being serially awaited; its failure is routed into the existingsettle({ cause: 'remote', ... })funnel via an attached.catch(with a try/catch preserving the synchronous-throw path). The send is still initiated synchronously insidelisten()— beforeawait opening— so wire ordering, the pre-registered_listenStateentry, and the synchronous-ack race guard are all unchanged.settle()is idempotent, so a send failure landing after another settle path remains a no-op.Behavior deltas, all confined to the pathological cases:
listen()promise (previously: unhandled rejection +listen()kept waiting on the send).listen()— the ack timer (or caller abort / transport close) rejects it (previously: permanent hang).Tests
Written failing-first against
main(cc4b416); newdescribeblock inpackages/client/test/client/listen.test.tswith a process-levelunhandledRejectioncapture around each body:RequestTimeout, no hang, nothing escapes, no leaked_listenState(fails onmain: hangs +REQUEST_TIMEOUTescapes).main).main).listen()with the send failure (behavior-preservation control; passes before and after).pnpm --filter @modelcontextprotocol/client test: 33 files, 805 tests passed. Lint (ESLint + Prettier) and typecheck clean on the touched package. Changeset included (patch,@modelcontextprotocol/client).Not addressed here (follow-ups): the stdio transport's un-abortable
'drain'park itself (#2552 covers write failures; honoringrequestSignalin stdiosend()would let listen's abort reach the wire), and the unbounded auth awaits in streamable HTTP_send(separate issue to follow).Generated by Claude Code