fix(core): never send notifications/cancelled for the initialize hand… - #2668
fix(core): never send notifications/cancelled for the initialize hand…#2668KKonstantinov wants to merge 2 commits into
Conversation
…shake The spec is explicit that a client MUST NOT attempt to cancel its initialize request, but the outbound cancel closure fired for any in-flight request: aborting connect()'s AbortSignal, or letting the handshake time out, put a forbidden cancellation on the wire naming the initialize request id. Guard the send on the request method. The caller's promise still rejects with the same abort/timeout error and connect() still tears the connection down; only the wire notification is suppressed. The guard sits inside the non-stream-close branch, so the modern per-request-stream path is unchanged - initialize is legacy-era only, absent from the modern registry. The e2e conformance suite already tracked this as a known deviation, so drop the knownFailures entry on protocol:cancel:initialize-not-cancellable; its 8 transport x era cells now pass as real assertions. Ports #1932 onto the core-internal layout. Fixes #998.
🦋 Changeset detectedLatest commit: 596f1c6 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 review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // "A client MUST NOT attempt to cancel its `initialize` | ||
| // request" (spec basic/lifecycle, mirrored on | ||
| // `CancelledNotification`). The handshake is the one request | ||
| // whose cancellation is forbidden outright, so an abort or | ||
| // timeout on it settles purely locally: the promise still | ||
| // rejects below, but nothing goes on the wire. Only the | ||
| // legacy era can reach this — `initialize` is absent from the | ||
| // modern registry, which negotiates via `server/discover`. | ||
| if (request.method !== 'initialize') { | ||
| this._transport | ||
| ?.send( | ||
| this._envelopeOutbound({ | ||
| jsonrpc: '2.0', | ||
| method: 'notifications/cancelled', | ||
| params: { | ||
| requestId: messageId, | ||
| reason: String(reason) | ||
| } | ||
| }), | ||
| { relatedRequestId, resumptionToken, onresumptiontoken } | ||
| ) | ||
| .catch(error => this._onerror(new Error(`Failed to send cancellation: ${error}`))); | ||
| } |
There was a problem hiding this comment.
🟡 [quality] Migration guide now contradicts the new initialize-cancel exemption: docs/migration/upgrade-to-v2.md (lines 1504-1509, "The cancelled-on-timeout signal is unchanged on legacy-era connections and on stdio/in-memory at any era") still promises v1 parity, but this diff suppresses notifications/cancelled for a timed-out/aborted initialize on exactly those connections, and no guide entry documents the exemption.
Extended reasoning...
Concrete cost per the repo review checklist ("Bugfix or behavior change: check whether docs/**/*.md describes the old behavior and needs updating; flag prose that now contradicts the implementation"): a migrator whose v1 test suite asserts the cancelled-on-timeout wire signal for the initialize handshake reads the upgrade-to-v2.md 'Error-shape changes' bullet, concludes the signal is unchanged on legacy-era connections, and then sees the assertion fail with no migration-guide explanation — the guide's parity claim is now false for the one exempted request. Fix is a one-line doc update noting the initialize exemption next to that bullet (or in the same section).
Verification: nit — the claimed contradiction is factually true. The diff at /home/claude/typescript-sdk/packages/core-internal/src/shared/protocol.ts:1462 adds if (request.method !== 'initialize') around the notifications/cancelled send inside the requestAbort === undefined branch — the branch the code's own comment (lines 1453-1461) says is reachable only on legacy-era connections ("Only the legacy era
| // ARRANGE | ||
| const sent: JSONRPCMessage[] = []; | ||
| const tx = new MockTransport(); | ||
| tx.send = async (m: JSONRPCMessage, _opts?: TransportSendOptions) => { | ||
| sent.push(m); | ||
| }; | ||
| const proto = createTestProtocol(); | ||
| await proto.connect(tx); | ||
| setNegotiatedProtocolVersion(proto, '2025-11-25'); |
There was a problem hiding this comment.
🟡 [quality] Identical 8-line arrange block (recording MockTransport + createTestProtocol + connect + setNegotiatedProtocolVersion('2025-11-25')) is copy-pasted verbatim in all three new tests, and duplicates the sibling test at lines 866-873.
Extended reasoning...
Concrete cost: ~24 lines of duplicated setup within one describe block (lines 923-930, 944-951, 963-970, mirroring 866-873). Any future change to the arrange shape (e.g. a MockTransport API change or a different negotiated-version helper) must be edited in four places, and the tests' distinct intent (abort vs timeout vs regression guard) is buried under repeated boilerplate. A small local helper, e.g. const makeRecordingProto = async () => { ... return { proto, sent }; } next to the existing cancelledSent helper at line 840, does the same job in one place. Nit severity.
Verification: nit — the duplication is real: the arrange block const sent: JSONRPCMessage[] = []; const tx = new MockTransport(); tx.send = async (m, _opts) => { sent.push(m); }; const proto = createTestProtocol(); await proto.connect(tx); setNegotiatedProtocolVersion(proto, '2025-11-25'); is copy-pasted verbatim in all three new tests at packages/core-internal/test/shared/protocol.test.ts:923-930, 944-951, a
1 participant
The SDK puts a spec-forbidden
notifications/cancelledon the wire for theinitializehandshake wheneverconnect()is aborted or times out. This guards the send; the local abort/reject path is unchanged.Motivation and Context
— spec, basic/lifecycle, mirrored on
CancelledNotificationinpackages/core-internal/src/types/spec.types.2025-11-25.ts.The
cancelclosure in_requestWithSchemaViaCodecfires for any in-flight request, and reachesinitializeby both routes: the caller'sAbortSignal(Client.connect()passes itsRequestOptionsstraight through to the handshake) and the timeout handler. Either one emits a cancellation naming the initialize request id.This is a long-standing, independently-recorded deviation:
bug/ready for work/P2/v2/fix proposed, with the same root cause identified.test/e2e/requirements.ts— the conformance manifest already carried aknownFailuresentry onprotocol:cancel:initialize-not-cancellablereading "SDK sends notifications/cancelled for initialize when connect() is aborted; spec says initialize MUST NOT be cancelled."Blast radius is modest but real.
_legacyHandshakecloses the connection on any handshake failure, so the peer is torn down regardless — but a server that honours the cancel aborts its in-flightinitializehandler and suppresses the response before that. The defect is conformance: the SDK emits a message the spec forbids, and every non-SDK server sees it.The fix
Guard the notification send on the request method:
connect()still tears the connection down. Only the wire notification is suppressed.initializeis legacy-era only, absent from the modern registry, which negotiates viaserver/discover.Removing the
knownFailuresentry is the required companion change, not incidental cleanup:verifies()runs those cells astest.fails(), so they fail once the SDK is fixed. All 8 transport × era cells now pass as real assertions.How Has This Been Tested?
Three new unit tests in the existing
outbound request cancellation: stream-close vs notifications/cancelledblock, covering both triggers plus a regression guard. Verified they actually catch the bug — with the guard removed, the two initialize cases fail and the regression guard still passes.core-internalclient/server/server-legacytest/integrationprotocol:cancel:initialize-not-cancellabletest.fails)coverage.test.tsmanifest gatespnpm typecheck:all,pnpm lint:allOne pre-existing flake, attributed rather than assumed:
protocol:timeout:max-total [sse]fails under full-suite load. It passes in isolation, and it also fails on an untouched base with this change reverted — unrelated to this PR.Breaking Changes
No API surface change; ships as a
patch. The observable change is on the wire: a peer no longer receivesnotifications/cancelledforinitialize. Any server depending on that was depending on behaviour the spec forbids, and the handshake still fails and closes exactly as before.Types of changes
Checklist
Additional context
Supersedes #1932. That PR proposed the same guard back in April and never got a human review; it has been
CONFLICTINGsince, because it patchespackages/core/src/shared/protocol.ts— a path that no longer exists after the protocol moved tocore-internal. This is that fix ported to the current layout, with the e2e manifest update it predates. Credit to @ameenalkhaldi for the original.Adjacent work on the same closure:
resumptionToken, so on legacy-era Streamable HTTP the transport treats the cancel as a stream resume. Samecancelclosure; worth sequencing.requestId: 0swallowed by a truthiness guard). Independent lines, no conflict.Fixes #998.