fix: handle cancellation for request ID 0 - #2654
Conversation
🦋 Changeset detectedLatest commit: 26bc4a2 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: |
… test Parameterize the notifications/cancelled test over 0, 123, '' and 'req-1' so the ordinary non-zero path keeps its own coverage rather than being traded for the zero case, and treat a params-less cancel notification as a no-op instead of a TypeError. Add the missing changeset.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the one-token fix is correct and the parameterized regression tests pin all four id shapes (0, 123, '', 'req-1'); the two inline comments are nits (a pre-existing sibling truthiness check and a comment-wording inaccuracy) that don't block the fix itself.
Extended reasoning...
Overview
This PR changes a single guard in Protocol._oncancel (packages/core-internal/src/shared/protocol.ts) from a truthiness check on notification.params.requestId to an explicit === undefined comparison, so cancellation notifications carrying the legal JSON-RPC ids 0 and '' actually abort the in-flight handler. It also parameterizes the existing cancellation test over four id shapes and adds a changeset. Three files, ~60 changed lines, no public API surface change.
Security risks
None. The change makes cancellation more effective (previously a request with id 0 could not be cancelled and would run to completion); there is no new input parsing, no auth-adjacent code, and no new trust boundary. I verified the params?. optional chain cannot be reached with absent params — both era schemas (rev2025-11-25/buildSchemas.ts:165-168, rev2026-07-28/buildSchemas.ts:1239-1242) require params and validation runs before _oncancel — so the defensive chaining is harmless dead code, not a validation bypass.
Level of scrutiny
This is protocol-core code, but the behavioral delta is a one-token predicate change whose semantics are pinned by new test.each coverage including the previously-broken ids. The abort-controller map (_requestHandlerAbortControllers) is keyed by RequestId and already handles 0/'' correctly via Map.get, so the fix composes cleanly with the surrounding code. The two inline findings are both nits: (1) the same truthiness pattern survives in the debounce guard in _notificationViaCodec — a pre-existing line, only triggerable under opt-in debouncedNotificationMethods config, worth completing but not blocking; (2) the new comment's claim that cancelled notifications are not schema-validated before dispatch is inaccurate — I verified the spec-form handler wrapper runs codec.validateNotification first and both era schemas require params — but this is prose accuracy only, the === undefined comparison itself is the correct fix.
Other factors
The changeset accurately describes the fix and its impact (id 0 being the first id every peer assigns). Test coverage is genuine regression coverage: the old truthiness guard would fail the 0 and '' cases. The PR follows the repo's minimalism and completeness conventions apart from the one leftover site already flagged inline. Timeline shows only bot comments and a review request — no unaddressed reviewer feedback. Per the repo's review conventions this is a clean correctness fix with no design questions, so approval with the two inline nits is appropriate.
The debounce gate in _notificationViaCodec tested relatedRequestId for truthiness, so the legal JSON-RPC ids `0` and `''` read as absent and a related notification wrongly passed the gate. Because the pending set is keyed by method alone, a second such notification in the same tick was silently dropped instead of sent — the exact loss the guard's own comment says it prevents. This is the sibling site of the cancellation bug fixed in the previous commit (modelcontextprotocol#2117, alongside modelcontextprotocol#2283). Also drops the optional chain on notification.params in _oncancel: both era schemas declare params as required on notifications/cancelled and the spec-form handler wrapper validates before dispatch, so the chain was unreachable and its comment described a validation gap that does not exist. Only requestId can legitimately be absent. The new test fires both notifications in one tick; awaiting between them flushes the microtask and hides the coalescing.
|
@claude review |
There was a problem hiding this comment.
LGTM — both items from my previous review are addressed: the debounce gate in _notificationViaCodec now uses the same === undefined check (with same-tick parameterized coverage over 0, '', and string ids), and the _oncancel comment was reworded to the accurate rationale. The two inline comments on this run flag pre-existing issues in code this PR doesn't touch; they are follow-up material, not blockers for this fix.
Extended reasoning...
Overview
The PR is a two-token correctness fix in packages/core-internal/src/shared/protocol.ts: _oncancel and the debounce gate in _notificationViaCodec both replaced truthiness checks on a RequestId with === undefined, so the legal JSON-RPC ids 0 and '' are no longer treated as absent. Id 0 is mainstream — the outbound counter is zero-based, so it is the first id every peer assigns. The changeset accurately describes both fixes, and the tests parameterize over every legal id shape (0, 123, '', 'req-1') for both the cancellation-abort path and the same-tick debounce path (the latter deliberately fires two sends in one tick so a wrongly-coalesced notification would be visible).
Prior review resolution
My earlier review on this PR raised exactly two points: the surviving !options?.relatedRequestId truthiness at the debounce gate, and an inline comment claiming a validation gap that doesn't exist. The current revision fixes the gate (line 1620), adds tests pinning it, extends the changeset to cover it, and rewords the _oncancel comment to the accurate rationale (requestId is genuinely optional on the 2025-era schema) with the unreachable params?. optional chain removed. Nothing from the prior round remains outstanding.
Findings this run
The bug hunting system surfaced two issues, both explicitly pre-existing and in code this diff does not touch: the unconditional notifications/cancelled send for a timed-out initialize (spec MUST NOT, client-side send path), and the Number() id coercion in _onresponse/_onprogress conflating string id '' with numeric id 0. Both are legitimate follow-ups; neither is introduced or worsened in a blocking way by this PR (the receive-side fix here is itself spec-required — a server honoring a received cancel is correct; the MUST NOT binds the client's send side). I verified via grep that no other truthiness-on-RequestId guard survives in this file.
Security risks
None. The change makes cancellation and debounce behavior strictly more correct for edge-value ids; it does not widen any input acceptance (both schemas already admitted 0/''), touch auth, or change error surfaces.
Level of scrutiny
protocol.ts is core shared code, so it warrants real scrutiny despite the tiny diff — which it received across two review rounds plus the parameterized regression tests added here. The semantic change is narrow and well-understood: only the values 0 and '' change behavior, in exactly the direction the JSON-RPC spec requires. With the prior feedback fully addressed and the remaining findings being pre-existing follow-ups, this is safe to approve.
| } | ||
|
|
||
| private async _oncancel(notification: CancelledNotification): Promise<void> { | ||
| if (!notification.params.requestId) { | ||
| // `requestId` is optional on the 2025-era wire schema. Absent is the | ||
| // only thing that means "no id": `0` and `''` are legal request ids. | ||
| if (notification.params.requestId === undefined) { | ||
| return; | ||
| } | ||
| // Handle request cancellation |
There was a problem hiding this comment.
🟣 Pre-existing spec-conformance issue this PR makes newly observable SDK-to-SDK: the outbound cancel closure in _requestWithSchemaViaCodec sends notifications/cancelled unconditionally for any in-flight request — including initialize, which the spec forbids cancelling ("A client MUST NOT attempt to cancel its initialize request", spec.types.2025-11-25.ts:243). Since initialize is id 0 on the plain legacy connect, the old truthiness guard accidentally swallowed the forbidden cancel on SDK servers; with this (correct, spec-required) fix, an SDK server now aborts the in-flight initialize handler and suppresses its response. Follow-up fix belongs in the untouched send path: skip sending notifications/cancelled when the originating request method is initialize (still rejecting locally with the timeout/abort error).
Extended reasoning...
The bug. The vendored spec is explicit on CancelledNotification (packages/core-internal/src/types/spec.types.2025-11-25.ts:243, also wire/rev2025-11-25/buildSchemas.ts:163): "A client MUST NOT attempt to cancel its initialize request." But the cancel() closure inside _requestWithSchemaViaCodec (packages/core-internal/src/shared/protocol.ts:1449-1470 area) POSTs notifications/cancelled { requestId: messageId, reason } unconditionally on the legacy/single-channel path — there is no request.method === 'initialize' guard anywhere in the request funnel or its abort/timeout paths.
The code path that triggers it — no misbehaving user code required. Client._legacyHandshake (packages/client/src/client/client.ts:1048-1058) issues initialize through this.request(...), the standard funnel, with DEFAULT_REQUEST_TIMEOUT_MSEC (60s). During the handshake no era is negotiated yet, so streamCloseCancels is false, requestAbort is undefined, and the notifications/cancelled POST branch runs. On a fresh client the zero-based _requestMessageId counter makes initialize id 0.
Step-by-step proof. (1) Client connects over the plain legacy path; _legacyHandshake sends initialize with id: 0. (2) The server is slow or hung; after 60s the timeoutHandler fires cancel(new SdkError(RequestTimeout, ...)) (or a caller-threaded abort signal fires cancel() earlier). (3) requestAbort === undefined, so the client POSTs notifications/cancelled { requestId: 0, reason: 'Request timed out' } — a wire-level MUST NOT violation, visible to any conformance-checking server. (4) Before this PR, an SDK server's _oncancel hit !notification.params.requestId with requestId = 0 and returned — the truthiness bug accidentally shielded exactly the one client→server request the spec forbids cancelling. (5) After this PR, _oncancel looks up the AbortController registered for id 0 in _onrequest and aborts it; the completion path sees abortController.signal.aborted and suppresses the InitializeResult (and any error response), leaving the initialize exchange unanswered.
Why this is pre-existing, and why the PR's change is still right. The offending code — the unconditional cancel send — is untouched by this diff and violated the spec on the wire before this PR too (any non-SDK or conformance-checking server always saw it; only SDK-to-SDK traffic was accidentally shielded). The receive-side === undefined fix here is itself correct and spec-required: every other id-0 request (the first sampling/createMessage, elicitation/create, or roots/list a server sends, per the changeset) must be cancellable, and a server honoring a received cancel is not a spec violation — the MUST NOT binds the client. Practical harm from the newly-live consequence is also minimal: the client has already rejected the handshake locally and abandoned the connection, so the suppressed response goes to a peer that stopped listening.
How to fix (follow-up, not this PR). Guard the send side: in the cancel() closure (or where the request enters the funnel), skip emitting notifications/cancelled when request.method === 'initialize', while still rejecting the local promise with the timeout/abort SdkError so caller behavior is unchanged. Alternatively (or additionally), the receive side could decline to abort an in-flight initialize handler. Either single change resolves both the wire-level MUST NOT violation and the newly-observable server-side abort, and a small vitest case in protocol.test.ts asserting no notifications/cancelled is sent for a timed-out initialize would pin it.
| } | ||
|
|
||
| private async _oncancel(notification: CancelledNotification): Promise<void> { | ||
| if (!notification.params.requestId) { | ||
| // `requestId` is optional on the 2025-era wire schema. Absent is the | ||
| // only thing that means "no id": `0` and `''` are legal request ids. | ||
| if (notification.params.requestId === undefined) { | ||
| return; | ||
| } | ||
| // Handle request cancellation |
There was a problem hiding this comment.
🟣 Pre-existing (untouched by this PR, but it completes the invariant this PR's changeset states): _onresponse and _onprogress correlate inbound messages via Number(response.id) / Number(progressToken), and Number('') === 0 — so a response or progress notification carrying the legal string id '' passes schema validation and silently settles (or resets the timeout / fires onprogress of) the pending request with numeric id 0, which is the first request every peer sends. Fix by matching ids strictly: coerce only non-empty numeric strings, letting '' fall through to the unknown-message-ID onerror path.
Extended reasoning...
The bug. This PR fixes two truthiness guards and its changeset states the invariant plainly: "0 and '' are legal request ids; absent is the only value that means 'no id'". But the same file still conflates '' with 0 at the response/progress correlation seam, via Number() coercion:
// _onresponse (protocol.ts ~1200)
const messageId = Number(response.id);
const handler = this._responseHandlers.get(messageId);
// _onprogress (protocol.ts ~1166)
const messageId = Number(progressToken);In JavaScript, Number('') === 0 (also Number(' ') === 0 and Number('0x0') === 0). So the empty-string id — one of the two ids this PR's changeset singles out as legal-but-mistreated — maps onto numeric id 0 at exactly the seam that decides which pending request a response belongs to.
The code path. RequestIdSchema = z.union([z.string(), z.number().int()]) (packages/core/src/schemas.ts:131) accepts '', and both response schemas key id on it, so {jsonrpc:'2.0', id:'', result:{...}} passes the isJSONRPCResultResponse/isJSONRPCErrorResponse guards in Protocol.connect and reaches _onresponse. Nothing between the guard and the Number() coercion does a strict-type match on the id. On the client, the _onresponse override (client.ts ~2224) only consumes string ids found in its listen state and its own comment documents the expectation: the base _responseHandlers map is "keyed by NUMBER", so string-id responses are supposed to fall through to super and surface via onerror as an unknown message ID. '' is the one string id that silently doesn't.
Why id 0 is not a corner case is this PR's own argument: _requestMessageId is zero-based, so 0 is the first id every peer assigns — on the client leg that is initialize itself; on the server→client leg it is the first sampling/createMessage / elicitation/create / roots/list.
Step-by-step proof. (1) Client connects; request() assigns messageId = 0 to initialize and registers _responseHandlers.set(0, ...). (2) A buggy or nonconforming peer sends {jsonrpc:'2.0', id:'', result:{...}} while request 0 is in flight. (3) The message passes isJSONRPCResultResponse (RequestIdSchema admits ''). (4) _onresponse computes Number('') === 0, finds the handler for request 0, deletes it, and settles the initialize promise with the foreign payload — or, if the payload fails the result schema, poisons request 0 with an InvalidResult rejection. Per JSON-RPC, id '' was never issued and should have surfaced via onerror as Received a response for an unknown message ID. The same walk applies to _onprogress with progressToken: '': it resets request 0's timeout and/or fires its onprogress callback.
Why this is pre-existing and non-blocking. The PR does not touch _onresponse or _onprogress, and the trigger requires a misbehaving or malicious peer (a conforming peer echoes the numeric id verbatim). It is flagged here because it is the remaining sibling site of the exact pattern this PR eradicates — the repo's Completeness convention ("partial migrations leave sibling code paths with the very bug the PR claims to fix") — and because it leaves the changeset's "absent is the only value that means 'no id'" claim incomplete at the correlation seam. JSON-RPC response correlation should be exact-match at a trust boundary.
How to fix. Coerce only non-empty numeric strings, so '' (and whitespace/hex strings) fall through as unknown, in both sites:
const messageId =
typeof response.id === 'number'
? response.id
: response.id.trim() !== '' && Number.isInteger(Number(response.id))
? Number(response.id)
: NaN;NaN never matches a map key, so an id-'' response then correctly reaches the Received a response for an unknown message ID onerror path, and a progressToken: '' progress notification reaches the unknown-token path.
2 participants
Summary
requestId: 0as a valid JSON-RPC request ID in cancellation notifications0are aborted correctlyRoot cause
The cancellation handler used a truthiness check for
requestId, causing the valid numeric ID0to be treated as missing.Testing
pnpm --filter @modelcontextprotocol/core-internal exec vitest run test/shared/protocol.test.tspnpm --filter @modelcontextprotocol/core-internal checkpnpm lint:allpnpm test:all— 1 transient SSE timeout; the failed E2E scenario passed when rerun independentlyFixes #2283
Maintainer edit:
Fixes #2115, fixes #2117, fixes #2283
Supersedes #2116, #2135, #2141, #2479, #2496, #2498, #2502, #2504, #2529, #2658