Repository navigation
fix(client): bound McpSubscription.close()'s wait on the cancelled-notification send #2645
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -435,6 +435,15 @@ | |
|
|
||
| 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 @@ | |
| 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); | ||
| void wireTeardown().finally(() => { | ||
| clearTimeout(timer); | ||
| resolve(); | ||
| }); | ||
| }); | ||
|
Check warning on line 2085 in packages/client/src/client/client.ts
|
||
| }; | ||
|
claude[bot] marked this conversation as resolved.
|
||
|
|
||
| // The per-subscription state is registered BEFORE the request is sent | ||
|
Comment on lines
2072
to
2096
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 Pre-existing (not introduced by this PR): the sibling Extended reasoning...What the bug is. This PR bounds one await-on-a-parked-send inside |
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.