From c2f4d2db04373d4a2e1661bce9957c6125023d7f Mon Sep 17 00:00:00 2001 From: Punit Shah Date: Wed, 12 Aug 2026 10:24:44 -0700 Subject: [PATCH 1/3] fix: handle cancellation for request id zero --- packages/core-internal/src/shared/protocol.ts | 2 +- packages/core-internal/test/shared/protocol.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 0a19770082..ead715f5bf 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -724,7 +724,7 @@ export abstract class Protocol { } private async _oncancel(notification: CancelledNotification): Promise { - if (!notification.params.requestId) { + if (notification.params.requestId === undefined) { return; } // Handle request cancellation diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 2ecdc40adc..5b979add5d 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -775,7 +775,7 @@ describe('protocol tests', () => { }); describe('notifications/cancelled behavior', () => { - test('should abort request handler when notifications/cancelled is received', async () => { + test('should abort request handler when notifications/cancelled contains requestId 0', async () => { await protocol.connect(transport); // Set up a request handler that checks if it was aborted @@ -788,7 +788,7 @@ describe('protocol tests', () => { }); // Simulate an incoming request - const requestId = 123; + const requestId = 0; if (transport.onmessage) { transport.onmessage({ jsonrpc: '2.0', From 4e11aad37a963d51c6e245bcdf86457f32882709 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Fri, 14 Aug 2026 12:47:24 +0300 Subject: [PATCH 2/3] test(core-internal): cover every legal request id in the cancellation 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. --- .changeset/cancelled-request-id-zero.md | 7 ++ packages/core-internal/src/shared/protocol.ts | 6 +- .../test/shared/protocol.test.ts | 83 ++++++++++--------- 3 files changed, 56 insertions(+), 40 deletions(-) create mode 100644 .changeset/cancelled-request-id-zero.md diff --git a/.changeset/cancelled-request-id-zero.md b/.changeset/cancelled-request-id-zero.md new file mode 100644 index 0000000000..8ab18bf8aa --- /dev/null +++ b/.changeset/cancelled-request-id-zero.md @@ -0,0 +1,7 @@ +--- +'@modelcontextprotocol/core-internal': patch +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/server': patch +--- + +Honor `notifications/cancelled` for request id `0`. The cancellation guard tested `requestId` for truthiness, so a cancel carrying the legal JSON-RPC id `0` — or the empty string — was treated as an absent id and the in-flight handler ran to completion with its `AbortSignal` never fired. Id `0` is not a corner case: the outbound request counter is zero-based, so it is the first id every peer assigns, which on the server→client leg is the first `sampling/createMessage`, `elicitation/create`, or `roots/list` a server sends. Absent is now the only value that means "no id". diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index ead715f5bf..75b3c1edc0 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -724,7 +724,11 @@ export abstract class Protocol { } private async _oncancel(notification: CancelledNotification): Promise { - if (notification.params.requestId === undefined) { + // `requestId` is optional on the wire, and `params` itself is optional + // on a JSON-RPC notification — inbound notifications are not validated + // against the cancelled-specific schema before dispatch. Absent is the + // only thing that means "no id": `0` and `''` are legal request ids. + if (notification.params?.requestId === undefined) { return; } // Handle request cancellation diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 5b979add5d..5f5be2adbc 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -775,50 +775,55 @@ describe('protocol tests', () => { }); describe('notifications/cancelled behavior', () => { - test('should abort request handler when notifications/cancelled contains requestId 0', async () => { - await protocol.connect(transport); - - // Set up a request handler that checks if it was aborted - let wasAborted = false; - protocol.setRequestHandler('ping', async (_request, ctx) => { - // Simulate a long-running operation - await new Promise(resolve => setTimeout(resolve, 100)); - wasAborted = ctx.mcpReq.signal.aborted; - return {}; - }); - - // Simulate an incoming request - const requestId = 0; - if (transport.onmessage) { - transport.onmessage({ - jsonrpc: '2.0', - id: requestId, - method: 'ping', - params: {} + // Every legal JSON-RPC request id must cancel, including the ones a + // truthiness guard swallows: `0` (the first id every peer assigns, + // since the counter is zero-based) and the empty string. + test.each([0, 123, '', 'req-1'])( + 'should abort request handler when notifications/cancelled carries requestId %j', + async requestId => { + await protocol.connect(transport); + + // Set up a request handler that checks if it was aborted + let wasAborted = false; + protocol.setRequestHandler('ping', async (_request, ctx) => { + // Simulate a long-running operation + await new Promise(resolve => setTimeout(resolve, 100)); + wasAborted = ctx.mcpReq.signal.aborted; + return {}; }); - } - // Wait a bit for the handler to start - await new Promise(resolve => setTimeout(resolve, 10)); + // Simulate an incoming request + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: '2.0', + id: requestId, + method: 'ping', + params: {} + }); + } - // Send cancellation notification - if (transport.onmessage) { - transport.onmessage({ - jsonrpc: '2.0', - method: 'notifications/cancelled', - params: { - requestId: requestId, - reason: 'User cancelled' - } - }); - } + // Wait a bit for the handler to start + await new Promise(resolve => setTimeout(resolve, 10)); + + // Send cancellation notification + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: '2.0', + method: 'notifications/cancelled', + params: { + requestId: requestId, + reason: 'User cancelled' + } + }); + } - // Wait for the handler to complete - await new Promise(resolve => setTimeout(resolve, 150)); + // Wait for the handler to complete + await new Promise(resolve => setTimeout(resolve, 150)); - // Verify the request was aborted - expect(wasAborted).toBe(true); - }); + // Verify the request was aborted + expect(wasAborted).toBe(true); + } + ); }); // Spec basic/patterns/cancellation §Transport-Specific (2026-07-28): on a From 26bc4a28142faeac0b54eef17ef6fee3378ca4de Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Fri, 14 Aug 2026 14:56:17 +0300 Subject: [PATCH 3/3] fix(core): treat relatedRequestId 0 as present in the debounce guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (#2117, alongside #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. --- .changeset/cancelled-request-id-zero.md | 7 ++++++- packages/core-internal/src/shared/protocol.ts | 12 ++++++----- .../test/shared/protocol.test.ts | 21 +++++++++++++++++++ 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/.changeset/cancelled-request-id-zero.md b/.changeset/cancelled-request-id-zero.md index 8ab18bf8aa..13cc84332d 100644 --- a/.changeset/cancelled-request-id-zero.md +++ b/.changeset/cancelled-request-id-zero.md @@ -4,4 +4,9 @@ '@modelcontextprotocol/server': patch --- -Honor `notifications/cancelled` for request id `0`. The cancellation guard tested `requestId` for truthiness, so a cancel carrying the legal JSON-RPC id `0` — or the empty string — was treated as an absent id and the in-flight handler ran to completion with its `AbortSignal` never fired. Id `0` is not a corner case: the outbound request counter is zero-based, so it is the first id every peer assigns, which on the server→client leg is the first `sampling/createMessage`, `elicitation/create`, or `roots/list` a server sends. Absent is now the only value that means "no id". +Treat request id `0` as a real id. Two guards tested a `RequestId` for truthiness, so the legal JSON-RPC ids `0` and `''` were read as absent. Id `0` is not a corner case: the outbound request counter is zero-based, so it is the first id every peer assigns, which on the server→client leg is the first `sampling/createMessage`, `elicitation/create`, or `roots/list` a server sends. + +- `notifications/cancelled` carrying id `0` was ignored, and the in-flight handler ran to completion with its `AbortSignal` never fired. +- A notification sent with `relatedRequestId: 0` wrongly passed the debounce gate (for methods opted into `debouncedNotificationMethods`). Because the pending set is keyed by method alone, a second such notification in the same tick was silently dropped rather than sent. + +Absent is now the only value that means "no id". diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 75b3c1edc0..ee79f5f0fc 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -724,11 +724,9 @@ export abstract class Protocol { } private async _oncancel(notification: CancelledNotification): Promise { - // `requestId` is optional on the wire, and `params` itself is optional - // on a JSON-RPC notification — inbound notifications are not validated - // against the cancelled-specific schema before dispatch. Absent is the + // `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) { + if (notification.params.requestId === undefined) { return; } // Handle request cancellation @@ -1615,7 +1613,11 @@ export abstract class Protocol { const debouncedMethods = this._options?.debouncedNotificationMethods ?? []; // A notification can only be debounced if it's in the list AND it's "simple" // (i.e., has no parameters and no related request ID that could be lost). - const canDebounce = debouncedMethods.includes(notification.method) && !notification.params && !options?.relatedRequestId; + // Absent is the only thing that means "no id" here too: `0` and `''` are + // legal request ids, and the pending set is keyed by method alone, so + // treating them as absent lets a related notification be coalesced away. + const canDebounce = + debouncedMethods.includes(notification.method) && !notification.params && options?.relatedRequestId === undefined; if (canDebounce) { // If a notification of this type is already scheduled, do nothing. diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 5f5be2adbc..95d038c2ce 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -656,6 +656,27 @@ describe('protocol tests', () => { expect(sendSpy).toHaveBeenCalledWith(expect.any(Object), { relatedRequestId: 'req-2' }); }); + // Same-tick coverage for every legal request id: the pending set is keyed + // by method alone, so a related notification that wrongly passes the + // debounce gate is coalesced away entirely. Awaiting between the two + // sends would flush the microtask and hide that, so they are fired in one + // tick. `0` and `''` are the ids a truthiness guard swallows. + test.each([0, 123, '', 'req-1'])('should NOT coalesce same-tick notifications related to requestId %j', async relatedRequestId => { + // ARRANGE + protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced_with_options'] }); + await protocol.connect(transport); + + // ACT — two related notifications in the same tick, no await between + void protocol.notification({ method: 'test/debounced_with_options' }, { relatedRequestId }); + void protocol.notification({ method: 'test/debounced_with_options' }, { relatedRequestId }); + await flushMicrotasks(); + + // ASSERT — both go out, each still carrying its related request id + expect(sendSpy).toHaveBeenCalledTimes(2); + expect(sendSpy).toHaveBeenNthCalledWith(1, expect.any(Object), { relatedRequestId }); + expect(sendSpy).toHaveBeenNthCalledWith(2, expect.any(Object), { relatedRequestId }); + }); + it('should clear pending debounced notifications on connection close', async () => { // ARRANGE protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced'] });