diff --git a/.changeset/cancelled-request-id-zero.md b/.changeset/cancelled-request-id-zero.md new file mode 100644 index 0000000000..13cc84332d --- /dev/null +++ b/.changeset/cancelled-request-id-zero.md @@ -0,0 +1,12 @@ +--- +'@modelcontextprotocol/core-internal': patch +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/server': patch +--- + +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 0a19770082..ee79f5f0fc 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -724,7 +724,9 @@ export abstract class Protocol { } private async _oncancel(notification: CancelledNotification): Promise { - 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 @@ -1611,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 2ecdc40adc..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'] }); @@ -775,50 +796,55 @@ describe('protocol tests', () => { }); describe('notifications/cancelled behavior', () => { - test('should abort request handler when notifications/cancelled is received', 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 = 123; - 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: {} + }); + } + + // 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' - } - }); - } + // 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