From 8fff8713811163cd9bbca703505935aeadccc831 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Fri, 27 Mar 2026 17:44:48 +0000 Subject: [PATCH 1/3] fix(core): remove abort listener from caller signal when request settles When a caller reuses a single AbortSignal across multiple requests (common for session-scoped cancellation), the previous implementation attached a new abort listener per request without ever removing it, leaking one closure per completed request onto the caller's signal. The listener is now named and detached via .finally() on the returned promise, so cleanup runs regardless of which exit path the request takes. This is structurally robust against future refactors of the promise body. Supersedes #1672. --- .changeset/fix-abort-listener-leak.md | 5 +++ packages/core/src/shared/protocol.ts | 13 +++++-- packages/core/test/shared/protocol.test.ts | 41 ++++++++++++++++++++++ 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 .changeset/fix-abort-listener-leak.md diff --git a/.changeset/fix-abort-listener-leak.md b/.changeset/fix-abort-listener-leak.md new file mode 100644 index 000000000..3a7f70a48 --- /dev/null +++ b/.changeset/fix-abort-listener-leak.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/core': patch +--- + +Fix abort signal listener leak in outbound requests. When a caller reuses a single `AbortSignal` across multiple requests (common for session-scoped cancellation), the SDK previously attached a new listener per request without ever removing it. The listener is now detached when the request settles. diff --git a/packages/core/src/shared/protocol.ts b/packages/core/src/shared/protocol.ts index d6daf0172..f62284961 100644 --- a/packages/core/src/shared/protocol.ts +++ b/packages/core/src/shared/protocol.ts @@ -800,6 +800,8 @@ export abstract class Protocol { ): Promise> { const { relatedRequestId, resumptionToken, onresumptiontoken } = options ?? {}; + let onAbort: (() => void) | undefined; + // Send the request return new Promise>((resolve, reject) => { const earlyReject = (error: unknown) => { @@ -885,9 +887,8 @@ export abstract class Protocol { } }); - options?.signal?.addEventListener('abort', () => { - cancel(options?.signal?.reason); - }); + onAbort = () => cancel(options?.signal?.reason); + options?.signal?.addEventListener('abort', onAbort, { once: true }); const timeout = options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC; const timeoutHandler = () => cancel(new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout })); @@ -928,6 +929,12 @@ export abstract class Protocol { reject(error); }); } + }).finally(() => { + // Detach the abort listener once the request settles so it doesn't + // accumulate on a caller-supplied signal reused across requests. + if (onAbort) { + options?.signal?.removeEventListener('abort', onAbort); + } }); } diff --git a/packages/core/test/shared/protocol.test.ts b/packages/core/test/shared/protocol.test.ts index 69735bc3a..8305c6e49 100644 --- a/packages/core/test/shared/protocol.test.ts +++ b/packages/core/test/shared/protocol.test.ts @@ -247,6 +247,47 @@ describe('protocol tests', () => { expect((abortReason as SdkError).code).toBe(SdkErrorCode.ConnectionClosed); }); + test('should remove abort listener from caller signal when request settles', async () => { + await protocol.connect(transport); + + const controller = new AbortController(); + const addSpy = vi.spyOn(controller.signal, 'addEventListener'); + const removeSpy = vi.spyOn(controller.signal, 'removeEventListener'); + + const mockSchema = z.object({ result: z.string() }); + const reqPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, { + signal: controller.signal + }); + + expect(addSpy).toHaveBeenCalledTimes(1); + const listener = addSpy.mock.calls[0]![1]; + + transport.onmessage?.({ jsonrpc: '2.0', id: 0, result: { result: 'ok' } }); + await reqPromise; + + expect(removeSpy).toHaveBeenCalledWith('abort', listener); + }); + + test('should not accumulate abort listeners when reusing a signal across requests', async () => { + await protocol.connect(transport); + + const controller = new AbortController(); + const addSpy = vi.spyOn(controller.signal, 'addEventListener'); + const removeSpy = vi.spyOn(controller.signal, 'removeEventListener'); + + const mockSchema = z.object({ result: z.string() }); + for (let i = 0; i < 5; i++) { + const reqPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, { + signal: controller.signal + }); + transport.onmessage?.({ jsonrpc: '2.0', id: i, result: { result: 'ok' } }); + await reqPromise; + } + + expect(addSpy).toHaveBeenCalledTimes(5); + expect(removeSpy).toHaveBeenCalledTimes(5); + }); + test('should not overwrite existing hooks when connecting transports', async () => { const oncloseMock = vi.fn(); const onerrorMock = vi.fn(); From 214e8a08a0d53c5adfec2fab6265095c8d63b9ff Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Fri, 27 Mar 2026 18:02:23 +0000 Subject: [PATCH 2/3] refactor(core): consolidate per-request cleanup into .finally() Moves _responseHandlers.delete and _cleanupTimeout from four scattered exit paths into the .finally() block alongside the abort listener removal. This also fixes two latent bugs where the taskManager error callback and transport.send().catch() paths were only calling _cleanupTimeout, leaking entries in _responseHandlers. _progressHandlers.delete stays at the error-path call sites because _onresponse deletes it conditionally (preserveProgress for task flows) and putting it in .finally() would override that. --- .changeset/fix-abort-listener-leak.md | 2 +- packages/core/src/shared/protocol.ts | 21 +++++++++++++-------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/.changeset/fix-abort-listener-leak.md b/.changeset/fix-abort-listener-leak.md index 3a7f70a48..f1dd3163b 100644 --- a/.changeset/fix-abort-listener-leak.md +++ b/.changeset/fix-abort-listener-leak.md @@ -2,4 +2,4 @@ '@modelcontextprotocol/core': patch --- -Fix abort signal listener leak in outbound requests. When a caller reuses a single `AbortSignal` across multiple requests (common for session-scoped cancellation), the SDK previously attached a new listener per request without ever removing it. The listener is now detached when the request settles. +Consolidate per-request cleanup in `_requestWithSchema` into a single `.finally()` block. This fixes an abort signal listener leak (listeners accumulated when a caller reused one `AbortSignal` across requests) and two cases where `_responseHandlers` entries leaked on send-failure paths. diff --git a/packages/core/src/shared/protocol.ts b/packages/core/src/shared/protocol.ts index f62284961..ffa642998 100644 --- a/packages/core/src/shared/protocol.ts +++ b/packages/core/src/shared/protocol.ts @@ -801,6 +801,7 @@ export abstract class Protocol { const { relatedRequestId, resumptionToken, onresumptiontoken } = options ?? {}; let onAbort: (() => void) | undefined; + let cleanupMessageId: number | undefined; // Send the request return new Promise>((resolve, reject) => { @@ -825,6 +826,7 @@ export abstract class Protocol { options?.signal?.throwIfAborted(); const messageId = this._requestMessageId++; + cleanupMessageId = messageId; const jsonrpcRequest: JSONRPCRequest = { ...request, jsonrpc: '2.0', @@ -843,9 +845,7 @@ export abstract class Protocol { } const cancel = (reason: unknown) => { - this._responseHandlers.delete(messageId); this._progressHandlers.delete(messageId); - this._cleanupTimeout(messageId); this._transport ?.send( @@ -908,16 +908,14 @@ export abstract class Protocol { let outboundQueued = false; try { const taskResult = this._taskManager.processOutboundRequest(jsonrpcRequest, options, messageId, responseHandler, error => { - this._cleanupTimeout(messageId); + this._progressHandlers.delete(messageId); reject(error); }); if (taskResult.queued) { outboundQueued = true; } } catch (error) { - this._responseHandlers.delete(messageId); this._progressHandlers.delete(messageId); - this._cleanupTimeout(messageId); reject(error); return; } @@ -925,16 +923,23 @@ export abstract class Protocol { if (!outboundQueued) { // No related task or no module - send through transport normally this._transport.send(jsonrpcRequest, { relatedRequestId, resumptionToken, onresumptiontoken }).catch(error => { - this._cleanupTimeout(messageId); + this._progressHandlers.delete(messageId); reject(error); }); } }).finally(() => { - // Detach the abort listener once the request settles so it doesn't - // accumulate on a caller-supplied signal reused across requests. + // Per-request cleanup that must run on every exit path. Consolidated + // here so new exit paths added to the promise body can't forget it. + // _progressHandlers is NOT cleaned up here: _onresponse deletes it + // conditionally (preserveProgress for task flows), and error paths + // above delete it inline since no task exists in those cases. if (onAbort) { options?.signal?.removeEventListener('abort', onAbort); } + if (cleanupMessageId !== undefined) { + this._responseHandlers.delete(cleanupMessageId); + this._cleanupTimeout(cleanupMessageId); + } }); } From 9fa785848b96f4639d9a7f48025ecaf4ee3ea6d6 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Fri, 27 Mar 2026 18:11:49 +0000 Subject: [PATCH 3/3] test: add reject-path coverage for abort listener cleanup Verifies .finally() runs removeEventListener when the request rejects (via timeout), not just on resolve. Adapted from #1672's test suite. --- packages/core/test/shared/protocol.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/core/test/shared/protocol.test.ts b/packages/core/test/shared/protocol.test.ts index 8305c6e49..619e09376 100644 --- a/packages/core/test/shared/protocol.test.ts +++ b/packages/core/test/shared/protocol.test.ts @@ -288,6 +288,23 @@ describe('protocol tests', () => { expect(removeSpy).toHaveBeenCalledTimes(5); }); + test('should remove abort listener when request rejects', async () => { + await protocol.connect(transport); + + const controller = new AbortController(); + const removeSpy = vi.spyOn(controller.signal, 'removeEventListener'); + + const mockSchema = z.object({ result: z.string() }); + await expect( + testRequest(protocol, { method: 'example', params: {} }, mockSchema, { + signal: controller.signal, + timeout: 0 + }) + ).rejects.toThrow(); + + expect(removeSpy).toHaveBeenCalledWith('abort', expect.any(Function)); + }); + test('should not overwrite existing hooks when connecting transports', async () => { const oncloseMock = vi.fn(); const onerrorMock = vi.fn();