Skip to content

Commit 8fff871

Browse files
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.
1 parent cce3ac7 commit 8fff871

File tree

3 files changed

+56
-3
lines changed

3 files changed

+56
-3
lines changed
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@modelcontextprotocol/core': patch
3+
---
4+
5+
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.

packages/core/src/shared/protocol.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -800,6 +800,8 @@ export abstract class Protocol<ContextT extends BaseContext> {
800800
): Promise<SchemaOutput<T>> {
801801
const { relatedRequestId, resumptionToken, onresumptiontoken } = options ?? {};
802802

803+
let onAbort: (() => void) | undefined;
804+
803805
// Send the request
804806
return new Promise<SchemaOutput<T>>((resolve, reject) => {
805807
const earlyReject = (error: unknown) => {
@@ -885,9 +887,8 @@ export abstract class Protocol<ContextT extends BaseContext> {
885887
}
886888
});
887889

888-
options?.signal?.addEventListener('abort', () => {
889-
cancel(options?.signal?.reason);
890-
});
890+
onAbort = () => cancel(options?.signal?.reason);
891+
options?.signal?.addEventListener('abort', onAbort, { once: true });
891892

892893
const timeout = options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC;
893894
const timeoutHandler = () => cancel(new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout }));
@@ -928,6 +929,12 @@ export abstract class Protocol<ContextT extends BaseContext> {
928929
reject(error);
929930
});
930931
}
932+
}).finally(() => {
933+
// Detach the abort listener once the request settles so it doesn't
934+
// accumulate on a caller-supplied signal reused across requests.
935+
if (onAbort) {
936+
options?.signal?.removeEventListener('abort', onAbort);
937+
}
931938
});
932939
}
933940

packages/core/test/shared/protocol.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,47 @@ describe('protocol tests', () => {
247247
expect((abortReason as SdkError).code).toBe(SdkErrorCode.ConnectionClosed);
248248
});
249249

250+
test('should remove abort listener from caller signal when request settles', async () => {
251+
await protocol.connect(transport);
252+
253+
const controller = new AbortController();
254+
const addSpy = vi.spyOn(controller.signal, 'addEventListener');
255+
const removeSpy = vi.spyOn(controller.signal, 'removeEventListener');
256+
257+
const mockSchema = z.object({ result: z.string() });
258+
const reqPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, {
259+
signal: controller.signal
260+
});
261+
262+
expect(addSpy).toHaveBeenCalledTimes(1);
263+
const listener = addSpy.mock.calls[0]![1];
264+
265+
transport.onmessage?.({ jsonrpc: '2.0', id: 0, result: { result: 'ok' } });
266+
await reqPromise;
267+
268+
expect(removeSpy).toHaveBeenCalledWith('abort', listener);
269+
});
270+
271+
test('should not accumulate abort listeners when reusing a signal across requests', async () => {
272+
await protocol.connect(transport);
273+
274+
const controller = new AbortController();
275+
const addSpy = vi.spyOn(controller.signal, 'addEventListener');
276+
const removeSpy = vi.spyOn(controller.signal, 'removeEventListener');
277+
278+
const mockSchema = z.object({ result: z.string() });
279+
for (let i = 0; i < 5; i++) {
280+
const reqPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, {
281+
signal: controller.signal
282+
});
283+
transport.onmessage?.({ jsonrpc: '2.0', id: i, result: { result: 'ok' } });
284+
await reqPromise;
285+
}
286+
287+
expect(addSpy).toHaveBeenCalledTimes(5);
288+
expect(removeSpy).toHaveBeenCalledTimes(5);
289+
});
290+
250291
test('should not overwrite existing hooks when connecting transports', async () => {
251292
const oncloseMock = vi.fn();
252293
const onerrorMock = vi.fn();

0 commit comments

Comments
 (0)