From af77ababd36f36b1731d5e520ee2773ae0dc16e9 Mon Sep 17 00:00:00 2001 From: Rajat Khare Date: Tue, 21 Jul 2026 18:17:17 +0530 Subject: [PATCH] Fix falsy-zero requestId bugs in cancel and debounce paths JSON-RPC id 0 is legal and is the first id Protocol assigns; truthiness guards were dropping cancels and incorrectly debounce-coalescing related notifications. Add regression tests and a patch changeset. Fixes #2283 Fixes #2117 Co-authored-by: Cursor --- .changeset/fix-falsy-zero-request-id.md | 5 ++ packages/core-internal/src/shared/protocol.ts | 11 +++- .../test/shared/protocol.test.ts | 59 +++++++++++++++++++ 3 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 .changeset/fix-falsy-zero-request-id.md diff --git a/.changeset/fix-falsy-zero-request-id.md b/.changeset/fix-falsy-zero-request-id.md new file mode 100644 index 0000000000..7570041d35 --- /dev/null +++ b/.changeset/fix-falsy-zero-request-id.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/core-internal": patch +--- + +Fix falsy-zero requestId bugs in debounce guard and cancellation handling diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 0a19770082..a35081fafc 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -724,11 +724,14 @@ export abstract class Protocol { } private async _oncancel(notification: CancelledNotification): Promise { - if (!notification.params.requestId) { + // RequestId 0 is legal JSON-RPC (and is the first id Protocol assigns). + // Use an explicit absence check — truthiness would drop id 0 (#2283). + const requestId = notification.params.requestId; + if (requestId === undefined || requestId === null) { return; } // Handle request cancellation - const controller = this._requestHandlerAbortControllers.get(notification.params.requestId); + const controller = this._requestHandlerAbortControllers.get(requestId); controller?.abort(notification.params.reason); } @@ -1611,7 +1614,9 @@ 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; + // relatedRequestId 0 is a valid id — check === undefined, not truthiness (#2117). + 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..9956443e3d 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -656,6 +656,25 @@ describe('protocol tests', () => { expect(sendSpy).toHaveBeenCalledWith(expect.any(Object), { relatedRequestId: 'req-2' }); }); + // Regression for #2117: numeric 0 is a legal JSON-RPC RequestId and must + // not be treated as "absent" by the debounce guard's truthiness check. + // Call synchronously (no await between) so coalescing can be observed — + // awaiting would flush the microtask and hide the bug. + it('should NOT debounce a notification that has relatedRequestId 0', async () => { + // ARRANGE + protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced_with_options'] }); + await protocol.connect(transport); + + // ACT — fire both in the same tick + void protocol.notification({ method: 'test/debounced_with_options' }, { relatedRequestId: 0 }); + void protocol.notification({ method: 'test/debounced_with_options' }, { relatedRequestId: 0 }); + await flushMicrotasks(); + + // ASSERT — related notifications must never coalesce (sent immediately, twice) + expect(sendSpy).toHaveBeenCalledTimes(2); + expect(sendSpy).toHaveBeenCalledWith(expect.any(Object), { relatedRequestId: 0 }); + }); + it('should clear pending debounced notifications on connection close', async () => { // ARRANGE protocol = new TestProtocolImpl({ debouncedNotificationMethods: ['test/debounced'] }); @@ -819,6 +838,46 @@ describe('protocol tests', () => { // Verify the request was aborted expect(wasAborted).toBe(true); }); + + // Regression for #2283 / #2115: requestId 0 is legal JSON-RPC and is the + // first id assigned by Protocol (_requestMessageId starts at 0). + test('should abort request handler when notifications/cancelled uses requestId 0', async () => { + await protocol.connect(transport); + + let wasAborted = false; + protocol.setRequestHandler('ping', async (_request, ctx) => { + await new Promise(resolve => setTimeout(resolve, 100)); + wasAborted = ctx.mcpReq.signal.aborted; + return {}; + }); + + const requestId = 0; + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: '2.0', + id: requestId, + method: 'ping', + params: {} + }); + } + + await new Promise(resolve => setTimeout(resolve, 10)); + + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: '2.0', + method: 'notifications/cancelled', + params: { + requestId: requestId, + reason: 'User cancelled' + } + }); + } + + await new Promise(resolve => setTimeout(resolve, 150)); + + expect(wasAborted).toBe(true); + }); }); // Spec basic/patterns/cancellation §Transport-Specific (2026-07-28): on a