From 072d5f3df1b12967fb259880c34ac83bbc904c50 Mon Sep 17 00:00:00 2001 From: Benjamin Liu <107985463+qiushui7@users.noreply.github.com> Date: Thu, 30 Jul 2026 21:59:06 +0800 Subject: [PATCH 1/2] fix: ignore late progress for completed tokens --- .../ignore-late-progress-notifications.md | 5 + src/shared/protocol.ts | 24 +++- test/shared/protocol.test.ts | 111 +++++++++++++++++- 3 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 .changeset/ignore-late-progress-notifications.md diff --git a/.changeset/ignore-late-progress-notifications.md b/.changeset/ignore-late-progress-notifications.md new file mode 100644 index 0000000000..e67f2bfc82 --- /dev/null +++ b/.changeset/ignore-late-progress-notifications.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/sdk': patch +--- + +Ignore late progress notifications for recently completed requests while preserving errors for progress tokens that were never registered. diff --git a/src/shared/protocol.ts b/src/shared/protocol.ts index 2637be65bc..c3ddfd1d0c 100644 --- a/src/shared/protocol.ts +++ b/src/shared/protocol.ts @@ -104,6 +104,7 @@ export type ProtocolOptions = { * The default request timeout, in miliseconds. */ export const DEFAULT_REQUEST_TIMEOUT_MSEC = 60000; +const MAX_RECENTLY_COMPLETED_PROGRESS_TOKENS = 64; /** * Options that can be given per request. @@ -328,6 +329,7 @@ export abstract class Protocol Promise> = new Map(); private _responseHandlers: Map void> = new Map(); private _progressHandlers: Map = new Map(); + private _recentlyCompletedProgressTokens = new Set(); private _timeoutInfo: Map = new Map(); private _pendingDebouncedNotifications = new Set(); @@ -645,6 +647,7 @@ export abstract class Protocol MAX_RECENTLY_COMPLETED_PROGRESS_TOKENS) { + const oldestProgressToken = this._recentlyCompletedProgressTokens.values().next().value; + if (oldestProgressToken !== undefined) { + this._recentlyCompletedProgressTokens.delete(oldestProgressToken); + } + } + } + private _onresponse(response: JSONRPCResponse | JSONRPCErrorResponse): void { const messageId = Number(response.id); @@ -921,7 +941,9 @@ export abstract class Protocol { }); }); + describe('late progress notifications', () => { + const resultSchema = z.object({ + result: z.string() + }); + + const startProgressRequest = (onprogress: ProgressCallback = vi.fn()) => { + const requestPromise = protocol.request({ method: 'example', params: {} }, resultSchema, { onprogress }); + const sentRequest = sendSpy.mock.calls.at(-1)![0] as { + id: number; + params: { _meta: { progressToken: number } }; + }; + return { requestPromise, sentRequest }; + }; + + const completeRequest = async (requestPromise: Promise<{ result: string }>, requestId: number) => { + transport.onmessage?.({ + jsonrpc: '2.0', + id: requestId, + result: { result: 'success' } + }); + await expect(requestPromise).resolves.toEqual({ result: 'success' }); + }; + + const sendProgress = async (progressToken: number, progress: number) => { + transport.onmessage?.({ + jsonrpc: '2.0', + method: 'notifications/progress', + params: { + progressToken, + progress, + total: 100 + } + }); + await Promise.resolve(); + }; + + test('should deliver progress received before the response', async () => { + const onProgressMock = vi.fn(); + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; + await protocol.connect(transport); + + const { requestPromise, sentRequest } = startProgressRequest(onProgressMock); + const progressToken = sentRequest.params._meta.progressToken; + + await sendProgress(progressToken, 50); + await completeRequest(requestPromise, sentRequest.id); + expect(onProgressMock).toHaveBeenCalledWith({ + progress: 50, + total: 100 + }); + expect(onErrorMock).not.toHaveBeenCalled(); + }); + + test('should ignore progress received after the response for the same token', async () => { + const onProgressMock = vi.fn(); + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; + await protocol.connect(transport); + + const { requestPromise, sentRequest } = startProgressRequest(onProgressMock); + const progressToken = sentRequest.params._meta.progressToken; + + await completeRequest(requestPromise, sentRequest.id); + await sendProgress(progressToken, 100); + + expect(onProgressMock).not.toHaveBeenCalled(); + expect(onErrorMock).not.toHaveBeenCalled(); + }); + + test('should report progress for a token that was never registered', async () => { + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; + await protocol.connect(transport); + + const requestPromise = protocol.request({ method: 'example', params: {} }, resultSchema); + const sentRequest = sendSpy.mock.calls.at(-1)![0] as { id: number }; + + await completeRequest(requestPromise, sentRequest.id); + await sendProgress(sentRequest.id, 50); + expect(onErrorMock).toHaveBeenCalledOnce(); + const error = onErrorMock.mock.calls[0][0] as Error; + expect(error).toBeInstanceOf(Error); + expect(error.message).toContain('Received a progress notification for an unknown token'); + expect(error.message).toContain(`"progressToken":${sentRequest.id}`); + }); + + test('should evict the oldest recently completed progress token', async () => { + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; + await protocol.connect(transport); + + const retainedTokenCount = 64; + const completedProgressTokens: number[] = []; + + for (let i = 0; i <= retainedTokenCount; i++) { + const { requestPromise, sentRequest } = startProgressRequest(); + completedProgressTokens.push(sentRequest.params._meta.progressToken); + await completeRequest(requestPromise, sentRequest.id); + } + + await sendProgress(completedProgressTokens[0], 100); + expect(onErrorMock).toHaveBeenCalledOnce(); + + await sendProgress(completedProgressTokens.at(-1)!, 100); + expect(onErrorMock).toHaveBeenCalledOnce(); + }); + }); + describe('progress notification timeout behavior', () => { beforeEach(() => { vi.useFakeTimers(); From 1e7b8b641825c6c7fd570d692eb7703f56cba0ae Mon Sep 17 00:00:00 2001 From: Benjamin Liu <107985463+qiushui7@users.noreply.github.com> Date: Fri, 31 Jul 2026 02:09:21 +0800 Subject: [PATCH 2/2] fix: remember progress tokens on request cleanup --- .../ignore-late-progress-notifications.md | 2 +- src/shared/protocol.ts | 20 +++--- test/shared/protocol.test.ts | 63 ++++++++++++++++++- 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/.changeset/ignore-late-progress-notifications.md b/.changeset/ignore-late-progress-notifications.md index e67f2bfc82..d879916197 100644 --- a/.changeset/ignore-late-progress-notifications.md +++ b/.changeset/ignore-late-progress-notifications.md @@ -2,4 +2,4 @@ '@modelcontextprotocol/sdk': patch --- -Ignore late progress notifications for recently completed requests while preserving errors for progress tokens that were never registered. +Ignore late progress notifications for recently finished requests while preserving errors for progress tokens that were never registered. diff --git a/src/shared/protocol.ts b/src/shared/protocol.ts index c3ddfd1d0c..533f27b586 100644 --- a/src/shared/protocol.ts +++ b/src/shared/protocol.ts @@ -862,8 +862,8 @@ export abstract class Protocol { this._responseHandlers.delete(messageId); - this._progressHandlers.delete(messageId); + this._cleanupProgressHandler(messageId); this._cleanupTimeout(messageId); this._transport @@ -1499,7 +1503,7 @@ export abstract class Protocol { result: z.string() }); - const startProgressRequest = (onprogress: ProgressCallback = vi.fn()) => { - const requestPromise = protocol.request({ method: 'example', params: {} }, resultSchema, { onprogress }); + const startProgressRequest = (onprogress: ProgressCallback = vi.fn(), options: Omit = {}) => { + const requestPromise = protocol.request({ method: 'example', params: {} }, resultSchema, { ...options, onprogress }); const sentRequest = sendSpy.mock.calls.at(-1)![0] as { id: number; params: { _meta: { progressToken: number } }; @@ -421,6 +421,42 @@ describe('protocol tests', () => { expect(onErrorMock).not.toHaveBeenCalled(); }); + test('should ignore progress received after caller abort', async () => { + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; + await protocol.connect(transport); + + const abortController = new AbortController(); + const { requestPromise, sentRequest } = startProgressRequest(vi.fn(), { signal: abortController.signal }); + const requestRejection = requestPromise.catch(() => undefined); + + abortController.abort(new Error('caller aborted')); + await requestRejection; + await sendProgress(sentRequest.params._meta.progressToken, 50); + + expect(onErrorMock).not.toHaveBeenCalled(); + }); + + test('should ignore progress received after request timeout', async () => { + vi.useFakeTimers(); + try { + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; + await protocol.connect(transport); + + const { requestPromise, sentRequest } = startProgressRequest(vi.fn(), { timeout: 10 }); + const requestRejection = requestPromise.catch(() => undefined); + + await vi.advanceTimersByTimeAsync(20); + await requestRejection; + await sendProgress(sentRequest.params._meta.progressToken, 50); + + expect(onErrorMock).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + test('should report progress for a token that was never registered', async () => { const onErrorMock = vi.fn(); protocol.onerror = onErrorMock; @@ -554,6 +590,8 @@ describe('protocol tests', () => { result: z.string() }); const onProgressMock = vi.fn(); + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; const requestPromise = protocol.request(request, mockSchema, { timeout: 1000, maxTotalTimeout: 150, @@ -593,6 +631,22 @@ describe('protocol tests', () => { } await expect(requestPromise).rejects.toThrow('Maximum total timeout exceeded'); expect(onProgressMock).toHaveBeenCalledTimes(1); + + // A progress notification already in flight after maxTotalTimeout cleanup + // should be ignored rather than reported as an unknown token. + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: '2.0', + method: 'notifications/progress', + params: { + progressToken: 0, + progress: 100, + total: 100 + } + }); + } + await Promise.resolve(); + expect(onErrorMock).not.toHaveBeenCalled(); }); test('should timeout if no progress received within timeout period', async () => { @@ -2585,6 +2639,8 @@ describe('Progress notification support for tasks', () => { const transport = new MockTransport(); const sendSpy = vi.spyOn(transport, 'send'); + const onErrorMock = vi.fn(); + protocol.onerror = onErrorMock; await protocol.connect(transport); // Set up a request handler that will complete the task @@ -2706,6 +2762,7 @@ describe('Progress notification support for tasks', () => { // Progress callback should NOT be invoked after task completion expect(progressCallback).not.toHaveBeenCalled(); + expect(onErrorMock).not.toHaveBeenCalled(); }); it('should stop progress notifications when task reaches terminal status (failed)', async () => {