From ff550a838cda9ee76656dfd5df2ef63fbda57907 Mon Sep 17 00:00:00 2001 From: Steeve Amonles Date: Wed, 14 Oct 2026 10:16:22 -0400 Subject: [PATCH 1/5] fix: deliver progress notifications that share a read with their response _onnotification deferred handlers via Promise.resolve(), while _onresponse runs synchronously and deletes the progress handler. When both messages arrive in one turn, the response won the race and the progress was dropped. Dispatch notification handlers synchronously (async rejections still go to _onerror) and add a regression test for the same-turn case. Fixes #3018 --- packages/core-internal/src/shared/protocol.ts | 16 +++++-- .../test/shared/protocol.test.ts | 42 +++++++++++++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 9d60be8efe..c0edd73222 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -929,10 +929,18 @@ export abstract class Protocol { return; } - // Starting with Promise.resolve() puts any synchronous errors into the monad as well. - Promise.resolve() - .then(() => (handler === undefined ? fallback!(notification) : handler(notification, codec))) - .catch(error => this._onerror(new Error(`Uncaught error in notification handler: ${error}`))); + // Dispatch synchronously so a notification that shares a read with + // its request's response is not deferred past _onresponse, which + // deletes the matching progress handler (see #3018). Async handler + // rejections and synchronous throws still route to _onerror. + try { + const result = handler === undefined ? fallback!(notification) : handler(notification, codec); + if (result !== undefined && typeof (result as PromiseLike).then === 'function') { + Promise.resolve(result).catch(error => this._onerror(new Error(`Uncaught error in notification handler: ${error}`))); + } + } catch (error) { + this._onerror(new Error(`Uncaught error in notification handler: ${error}`)); + } } private _onrequest(rawRequest: JSONRPCRequest, extra?: MessageExtraInfo): void { diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index 2fb0f64813..fcb1923ffc 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -617,6 +617,48 @@ describe('protocol tests', () => { await Promise.resolve(); await expect(requestPromise).resolves.toEqual({ result: 'success' }); }); + + test('should deliver progress when notification and response arrive in the same turn', async () => { + // Repro for #3018: progress was deferred via Promise.resolve(), so + // a same-turn response deleted the progress handler first. + await protocol.connect(transport); + const request = { method: 'example', params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string() + }); + const onProgressMock = vi.fn(); + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; + + const requestPromise = testRequest(protocol, request, mockSchema, { + timeout: 1000, + onprogress: onProgressMock + }); + + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: '2.0', + method: 'notifications/progress', + params: { + progressToken: 0, + progress: 50, + total: 100 + } + }); + transport.onmessage({ + jsonrpc: '2.0', + id: 0, + result: { result: 'success' } + }); + } + + await expect(requestPromise).resolves.toEqual({ result: 'success' }); + expect(onProgressMock).toHaveBeenCalledWith({ + progress: 50, + total: 100 + }); + expect(onErrorMock).not.toHaveBeenCalled(); + }); }); describe('Debounced Notifications', () => { From 9af4566b1010d1496cd8c3633e622bc27d0cc7d1 Mon Sep 17 00:00:00 2001 From: Steeve-Crypto Date: Wed, 14 Oct 2026 10:43:59 -0400 Subject: [PATCH 2/5] test(e2e): drop SSE knownFailures for fixed progress delivery The sync notification dispatch in #3023 delivers progress that shares a read with its response, so the SSE progress scenarios should pass. --- test/e2e/requirements.ts | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/test/e2e/requirements.ts b/test/e2e/requirements.ts index 36da4fc523..13b4e47fb4 100644 --- a/test/e2e/requirements.ts +++ b/test/e2e/requirements.ts @@ -214,23 +214,11 @@ export const REQUIREMENTS: Record = { 'protocol:progress:callback': { source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/utilities/progress#progress-flow', behavior: - "Progress notifications emitted by a handler during a request are delivered to the caller's progress callback, in order, with their progress, total, and message.", - knownFailures: [ - { - transport: 'sse', - note: "Real-socket SSE delivers a handler's progress notifications and its response in one batch; the response is processed first, so the progress notifications never reach the caller's progress callback." - } - ] + "Progress notifications emitted by a handler during a request are delivered to the caller's progress callback, in order, with their progress, total, and message." }, 'typescript:protocol:progress:token-injected': { source: 'sdk', - behavior: 'Passing onprogress causes a progressToken to be injected into request _meta, preserving existing _meta fields.', - knownFailures: [ - { - transport: 'sse', - note: "Real-socket SSE delivers a handler's progress notifications and its response in one batch; the response is processed first, so the progress notifications never reach the caller's progress callback." - } - ] + behavior: 'Passing onprogress causes a progressToken to be injected into request _meta, preserving existing _meta fields.' }, 'protocol:progress:token-unique': { source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/utilities/progress#progress-flow', @@ -246,13 +234,7 @@ export const REQUIREMENTS: Record = { }, 'protocol:timeout:reset-on-progress': { source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/lifecycle#timeouts', - behavior: "When configured to do so, each progress notification resets the request's read timeout.", - knownFailures: [ - { - transport: 'sse', - note: 'Same real-socket SSE batching race as protocol:progress:callback: the progress notifications are dropped before they can reset the timeout, so the request times out.' - } - ] + behavior: "When configured to do so, each progress notification resets the request's read timeout." }, 'protocol:timeout:sends-cancellation': { source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/lifecycle#timeouts', @@ -391,13 +373,7 @@ export const REQUIREMENTS: Record = { }, 'tools:call:progress': { source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/utilities/progress#progress-flow', - behavior: "Progress notifications emitted by a tool handler reach the caller's progress callback before the tool result returns.", - knownFailures: [ - { - transport: 'sse', - note: "Real-socket SSE delivers a handler's progress notifications and its response in one batch; the response is processed first, so the progress notifications never reach the caller's progress callback." - } - ] + behavior: "Progress notifications emitted by a tool handler reach the caller's progress callback before the tool result returns." }, 'tools:call:sampling-roundtrip': { transports: STATEFUL_TRANSPORTS, From aa734a86a38e8192661f349276fb96b6e9454a38 Mon Sep 17 00:00:00 2001 From: Steeve-Crypto Date: Sat, 31 Oct 2026 07:40:35 -0400 Subject: [PATCH 3/5] fix(core): limit synchronous dispatch to progress notifications Keep microtask dispatch for every other notification and the fallback handler so their callbacks do not run inside the transport's onmessage stack. Adds regression coverage for an extension notification handler that closes the connection. --- packages/core-internal/src/shared/protocol.ts | 25 +++++++++---- .../test/shared/protocol.test.ts | 37 +++++++++++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index c0edd73222..24f60d4f52 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -929,17 +929,28 @@ export abstract class Protocol { return; } - // Dispatch synchronously so a notification that shares a read with - // its request's response is not deferred past _onresponse, which - // deletes the matching progress handler (see #3018). Async handler - // rejections and synchronous throws still route to _onerror. + const dispatch = () => (handler === undefined ? fallback!(notification) : handler(notification, codec)); + const reportError = (error: unknown) => this._onerror(new Error(`Uncaught error in notification handler: ${error}`)); + + if (notification.method !== 'notifications/progress') { + // Other notifications keep microtask dispatch so their handlers do + // not run inside the transport's onmessage stack. + // Starting with Promise.resolve() puts any synchronous errors into the monad as well. + Promise.resolve().then(dispatch).catch(reportError); + return; + } + + // Progress is dispatched synchronously so a progress notification that + // shares a read with its request's response is not deferred past + // _onresponse, which deletes the matching progress handler (see #3018). + // Async handler rejections and synchronous throws still route to _onerror. try { - const result = handler === undefined ? fallback!(notification) : handler(notification, codec); + const result = dispatch(); if (result !== undefined && typeof (result as PromiseLike).then === 'function') { - Promise.resolve(result).catch(error => this._onerror(new Error(`Uncaught error in notification handler: ${error}`))); + Promise.resolve(result).catch(reportError); } } catch (error) { - this._onerror(new Error(`Uncaught error in notification handler: ${error}`)); + reportError(error); } } diff --git a/packages/core-internal/test/shared/protocol.test.ts b/packages/core-internal/test/shared/protocol.test.ts index fcb1923ffc..47649a4bff 100644 --- a/packages/core-internal/test/shared/protocol.test.ts +++ b/packages/core-internal/test/shared/protocol.test.ts @@ -661,6 +661,43 @@ describe('protocol tests', () => { }); }); + describe('Notification dispatch ordering', () => { + test('keeps non-progress notification handlers on microtask dispatch', async () => { + await protocol.connect(transport); + const order: string[] = []; + protocol.fallbackNotificationHandler = async () => { + order.push('handler'); + }; + + transport.onmessage?.({ jsonrpc: '2.0', method: 'notifications/custom-extension', params: {} }); + order.push('after-onmessage'); + + await Promise.resolve(); + await Promise.resolve(); + expect(order).toEqual(['after-onmessage', 'handler']); + }); + + test('a non-progress handler that closes the connection does not run inside onmessage', async () => { + await protocol.connect(transport); + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; + let closedDuringOnmessage = false; + let inOnmessage = false; + protocol.fallbackNotificationHandler = async () => { + closedDuringOnmessage = inOnmessage; + await protocol.close(); + }; + + inOnmessage = true; + transport.onmessage?.({ jsonrpc: '2.0', method: 'notifications/custom-extension', params: {} }); + inOnmessage = false; + + await vi.waitFor(() => expect(protocol.transport).toBeUndefined()); + expect(closedDuringOnmessage).toBe(false); + expect(onErrorMock).not.toHaveBeenCalled(); + }); + }); + describe('Debounced Notifications', () => { // We need to flush the microtask queue to test the debouncing logic. // This helper function does that. From b9e44064c77738452d62ce2b80971e7308ec9b3e Mon Sep 17 00:00:00 2001 From: Steeve-Crypto Date: Sat, 31 Oct 2026 08:11:08 -0400 Subject: [PATCH 4/5] fix(core): satisfy consistent-function-scoping lint --- packages/core-internal/src/shared/protocol.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 24f60d4f52..0f6443d32a 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -930,13 +930,14 @@ export abstract class Protocol { } const dispatch = () => (handler === undefined ? fallback!(notification) : handler(notification, codec)); - const reportError = (error: unknown) => this._onerror(new Error(`Uncaught error in notification handler: ${error}`)); if (notification.method !== 'notifications/progress') { // Other notifications keep microtask dispatch so their handlers do // not run inside the transport's onmessage stack. // Starting with Promise.resolve() puts any synchronous errors into the monad as well. - Promise.resolve().then(dispatch).catch(reportError); + Promise.resolve() + .then(dispatch) + .catch(error => this._reportNotificationHandlerError(error)); return; } @@ -947,13 +948,17 @@ export abstract class Protocol { try { const result = dispatch(); if (result !== undefined && typeof (result as PromiseLike).then === 'function') { - Promise.resolve(result).catch(reportError); + Promise.resolve(result).catch(error => this._reportNotificationHandlerError(error)); } } catch (error) { - reportError(error); + this._reportNotificationHandlerError(error); } } + private _reportNotificationHandlerError(error: unknown): void { + this._onerror(new Error(`Uncaught error in notification handler: ${error}`)); + } + private _onrequest(rawRequest: JSONRPCRequest, extra?: MessageExtraInfo): void { // Lift wire-only material before dispatch: handlers (including the // fallback handler and the per-method schema parse) see exactly the From c6543cf2533a301e1b8be14c3758cf2dd33d5b1d Mon Sep 17 00:00:00 2001 From: Steeve-Crypto Date: Sat, 31 Oct 2026 08:51:00 -0400 Subject: [PATCH 5/5] fix(core): keep progress sync dispatch inside the inbound instrumentation scope --- packages/core-internal/src/shared/protocol.ts | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 737fd35cd9..c9074ea819 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -961,25 +961,17 @@ export abstract class Protocol { const dispatch = () => (handler === undefined ? fallback!(notification) : handler(notification, codec)); - let settled: Promise; - if (notification.method === 'notifications/progress') { - // Progress is dispatched synchronously so a progress notification that - // shares a read with its request's response is not deferred past - // _onresponse, which deletes the matching progress handler (see #3018). - // Async handler rejections and synchronous throws still route to _onerror. - settled = runInScope(scope, () => { - try { - return Promise.resolve(dispatch()); - } catch (error) { - return Promise.reject(error); - } - }); - } else { - // Other notifications keep microtask dispatch so their handlers do - // not run inside the transport's onmessage stack. - // Starting with Promise.resolve() puts any synchronous errors into the monad as well. - settled = runInScope(scope, () => Promise.resolve().then(dispatch)); - } + // Progress is dispatched synchronously so a progress notification that + // shares a read with its request's response is not deferred past + // _onresponse, which deletes the matching progress handler (see #3018). + // Other notifications keep microtask dispatch so their handlers do not + // run inside the transport's onmessage stack. Either way, synchronous + // throws and async rejections both settle the promise below and route to _onerror. + const settled = runInScope(scope, () => + notification.method === 'notifications/progress' + ? new Promise(resolve => resolve(dispatch())) + : Promise.resolve().then(dispatch) + ); settled.then( () => scope?.end({ type: 'delivered' }),