From 5cef0eaa2f83cd0ce4d2301c2cf438adf3c29e3f Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Tue, 3 Feb 2026 10:50:18 +0000 Subject: [PATCH 1/4] fix: clear _timeoutInfo in _onclose() and scope .finally() abort controller cleanup Fixes two related cleanup gaps in Protocol._onclose(): 1. _timeoutInfo not cleared: pending setTimeout callbacks from old requests survive close() and can fire cancel() against a new transport after close()+connect(), sending spurious notifications/cancelled for request IDs the new client never sent. Fix: iterate _timeoutInfo entries calling clearTimeout(), then clear. 2. Stale .finally() can delete new connection's abort controller: the .finally() in _onrequest deletes from _requestHandlerAbortControllers by request.id without checking if the stored controller matches. After reconnect, if a new client reuses the same request ID, the old handler's .finally() deletes the new connection's abort controller. Fix: only delete if the stored value matches the captured controller. Fixes #1459 --- package-lock.json | 11 ----------- src/shared/protocol.ts | 11 ++++++++++- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/package-lock.json b/package-lock.json index c563717c0..b40a1b73c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1333,7 +1333,6 @@ "integrity": "sha512-PC0PDZfJg8sP7cmKe6L3QIL8GZwU5aRvUFedqSIpw3B+QjRSUZeeITC2M5XKeMXEzL6wccN196iy3JLwKNvDVA==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.48.1", "@typescript-eslint/types": "8.48.1", @@ -1765,7 +1764,6 @@ "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "dev": true, "license": "MIT", - "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -2308,7 +2306,6 @@ "integrity": "sha512-BhHmn2yNOFA9H9JmmIVKJmd288g9hrVRDkdoIgRCRuSySRUHH7r/DI6aAXW9T1WwUuY3DFgrcaqB+deURBLR5g==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.8.0", "@eslint-community/regexpp": "^4.12.1", @@ -2627,7 +2624,6 @@ "resolved": "https://registry.npmjs.org/express/-/express-5.2.1.tgz", "integrity": "sha512-hIS4idWWai69NezIdRt2xFVofaF4j+6INOpJlVOLDO8zXGpUVEVzIYk12UUi2JzjEzWL3IOAxcTubgz9Po0yXw==", "license": "MIT", - "peer": true, "dependencies": { "accepts": "^2.0.0", "body-parser": "^2.2.1", @@ -3049,7 +3045,6 @@ "resolved": "https://registry.npmjs.org/hono/-/hono-4.11.4.tgz", "integrity": "sha512-U7tt8JsyrxSRKspfhtLET79pU8K+tInj5QZXs1jSugO1Vq5dFj3kmZsRldo29mTBfcjDRVRXrEZ6LS63Cog9ZA==", "license": "MIT", - "peer": true, "engines": { "node": ">=16.9.0" } @@ -4107,7 +4102,6 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" }, @@ -4188,7 +4182,6 @@ "integrity": "sha512-4H8vUNGNjQ4V2EOoGw005+c+dGuPSnhpPBPHBtsZdGZBk/iJb4kguGlPWaZTZ3q5nMtFOEsY0nRDlh9PJyd6SQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "~0.25.0", "get-tsconfig": "^4.7.5" @@ -4234,7 +4227,6 @@ "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.6.3.tgz", "integrity": "sha512-hjcS1mhfuyi4WW8IWtjP7brDrG2cuDZukyrYrSauoXGNgx0S7zceP07adYkJycEr56BOUTNPzbInooiN3fn1qw==", "dev": true, - "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -4429,7 +4421,6 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" }, @@ -4443,7 +4434,6 @@ "integrity": "sha512-BxAKBWmIbrDgrokdGZH1IgkIk/5mMHDreLDmCJ0qpyJaAteP8NvMhkwr/ZCQNqNH97bw/dANTE9PDzqwJghfMQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.5.0", @@ -4596,7 +4586,6 @@ "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", "license": "MIT", - "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } diff --git a/src/shared/protocol.ts b/src/shared/protocol.ts index aa242a647..7bc7714d7 100644 --- a/src/shared/protocol.ts +++ b/src/shared/protocol.ts @@ -642,6 +642,11 @@ export abstract class Protocol this._onerror(new Error(`Failed to send response: ${error}`))) .finally(() => { - this._requestHandlerAbortControllers.delete(request.id); + // Only delete if the stored controller is still ours; after close()+connect(), + // a new connection may have reused the same request ID with a different controller. + if (this._requestHandlerAbortControllers.get(request.id) === abortController) { + this._requestHandlerAbortControllers.delete(request.id); + } }); } From 14eded434b13b70d343d5d3d74a3b5a890e3c069 Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Tue, 3 Feb 2026 10:56:57 +0000 Subject: [PATCH 2/4] test: add tests for _onclose timeout and abort controller cleanup Tests that _onclose() properly clears _timeoutInfo to prevent spurious cancellation notifications after reconnect, and that the .finally() abort controller cleanup is scoped to avoid deleting a new connection's controller when request IDs overlap. --- package-lock.json | 11 +++ test/shared/protocol.test.ts | 151 +++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+) diff --git a/package-lock.json b/package-lock.json index b40a1b73c..c563717c0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1333,6 +1333,7 @@ "integrity": "sha512-PC0PDZfJg8sP7cmKe6L3QIL8GZwU5aRvUFedqSIpw3B+QjRSUZeeITC2M5XKeMXEzL6wccN196iy3JLwKNvDVA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.48.1", "@typescript-eslint/types": "8.48.1", @@ -1764,6 +1765,7 @@ "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "dev": true, "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -2306,6 +2308,7 @@ "integrity": "sha512-BhHmn2yNOFA9H9JmmIVKJmd288g9hrVRDkdoIgRCRuSySRUHH7r/DI6aAXW9T1WwUuY3DFgrcaqB+deURBLR5g==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.8.0", "@eslint-community/regexpp": "^4.12.1", @@ -2624,6 +2627,7 @@ "resolved": "https://registry.npmjs.org/express/-/express-5.2.1.tgz", "integrity": "sha512-hIS4idWWai69NezIdRt2xFVofaF4j+6INOpJlVOLDO8zXGpUVEVzIYk12UUi2JzjEzWL3IOAxcTubgz9Po0yXw==", "license": "MIT", + "peer": true, "dependencies": { "accepts": "^2.0.0", "body-parser": "^2.2.1", @@ -3045,6 +3049,7 @@ "resolved": "https://registry.npmjs.org/hono/-/hono-4.11.4.tgz", "integrity": "sha512-U7tt8JsyrxSRKspfhtLET79pU8K+tInj5QZXs1jSugO1Vq5dFj3kmZsRldo29mTBfcjDRVRXrEZ6LS63Cog9ZA==", "license": "MIT", + "peer": true, "engines": { "node": ">=16.9.0" } @@ -4102,6 +4107,7 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -4182,6 +4188,7 @@ "integrity": "sha512-4H8vUNGNjQ4V2EOoGw005+c+dGuPSnhpPBPHBtsZdGZBk/iJb4kguGlPWaZTZ3q5nMtFOEsY0nRDlh9PJyd6SQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "~0.25.0", "get-tsconfig": "^4.7.5" @@ -4227,6 +4234,7 @@ "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.6.3.tgz", "integrity": "sha512-hjcS1mhfuyi4WW8IWtjP7brDrG2cuDZukyrYrSauoXGNgx0S7zceP07adYkJycEr56BOUTNPzbInooiN3fn1qw==", "dev": true, + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -4421,6 +4429,7 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -4434,6 +4443,7 @@ "integrity": "sha512-BxAKBWmIbrDgrokdGZH1IgkIk/5mMHDreLDmCJ0qpyJaAteP8NvMhkwr/ZCQNqNH97bw/dANTE9PDzqwJghfMQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.5.0", @@ -4586,6 +4596,7 @@ "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", "license": "MIT", + "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } diff --git a/test/shared/protocol.test.ts b/test/shared/protocol.test.ts index 886dcbb21..3f4f07e9a 100644 --- a/test/shared/protocol.test.ts +++ b/test/shared/protocol.test.ts @@ -5556,3 +5556,154 @@ describe('Error handling for missing resolvers', () => { }); }); }); + +describe('_onclose cleanup', () => { + let protocol: Protocol; + let transport: MockTransport; + let sendSpy: MockInstance; + + beforeEach(() => { + vi.useFakeTimers(); + transport = new MockTransport(); + sendSpy = vi.spyOn(transport, 'send'); + protocol = new (class extends Protocol { + protected assertCapabilityForMethod(): void {} + protected assertNotificationCapability(): void {} + protected assertRequestHandlerCapability(): void {} + protected assertTaskCapability(): void {} + protected assertTaskHandlerCapability(): void {} + })(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + test('should clear pending timeouts in _onclose to prevent spurious cancellation after reconnect', async () => { + await protocol.connect(transport); + + // Start a request with a long timeout + const request = { method: 'example', params: {} }; + const mockSchema = z.object({ result: z.string() }); + + const requestPromise = protocol.request(request, mockSchema, { + timeout: 60000 + }).catch(() => {/* expected ConnectionClosed error */}); + + // Verify the request was sent + expect(sendSpy).toHaveBeenCalled(); + + // Spy on clearTimeout to verify it gets called during close + const clearTimeoutSpy = vi.spyOn(global, 'clearTimeout'); + + // Close the transport - this should clear timeouts + await transport.close(); + + // Verify clearTimeout was called (at least once for our timeout) + expect(clearTimeoutSpy).toHaveBeenCalled(); + + // Now reconnect with a new transport + const transport2 = new MockTransport(); + const sendSpy2 = vi.spyOn(transport2, 'send'); + await protocol.connect(transport2); + + // Advance past the original timeout - if not cleared, this would fire the callback + await vi.advanceTimersByTimeAsync(60000); + + // Verify no spurious cancellation notification was sent to the new transport + const cancellationCalls = sendSpy2.mock.calls.filter(call => { + const msg = call[0] as Record; + return msg.method === 'notifications/cancelled'; + }); + expect(cancellationCalls).toHaveLength(0); + + await transport2.close(); + await requestPromise; + clearTimeoutSpy.mockRestore(); + }); + + test('should not let stale .finally() delete a new connections abort controller after reconnect', async () => { + await protocol.connect(transport); + + const TestRequestSchema = z.object({ + method: z.literal('test/longRunning'), + params: z.optional(z.record(z.unknown())) + }); + + // Set up a handler with a deferred resolution we control + let resolveHandler!: () => void; + const handlerStarted = new Promise(resolve => { + protocol.setRequestHandler(TestRequestSchema, async () => { + resolve(); // signal that handler has started + // Wait for explicit resolution + await new Promise(r => { resolveHandler = r; }); + return { _meta: {} } as Result; + }); + }); + + // Simulate an incoming request with id=1 on the first connection + const requestId = 1; + transport.onmessage!({ + jsonrpc: '2.0', + id: requestId, + method: 'test/longRunning', + params: {} + }); + + // Wait for handler to start + await handlerStarted; + + // Close the connection (aborts the controller and clears the map) + await transport.close(); + + // Reconnect with a new transport + const transport2 = new MockTransport(); + await protocol.connect(transport2); + + // Set up a new handler for the second connection that we can verify cancellation on + let wasAborted = false; + let resolveHandler2!: () => void; + const handler2Started = new Promise(resolve => { + protocol.setRequestHandler(TestRequestSchema, async (_request, extra) => { + resolve(); + await new Promise(r => { resolveHandler2 = r; }); + wasAborted = extra.signal.aborted; + return { _meta: {} } as Result; + }); + }); + + // Simulate same request id=1 on the new connection + transport2.onmessage!({ + jsonrpc: '2.0', + id: requestId, + method: 'test/longRunning', + params: {} + }); + + await handler2Started; + + // Resolve the OLD handler so its .finally() runs + resolveHandler(); + // Flush microtasks so .finally() executes + await vi.advanceTimersByTimeAsync(0); + + // Send cancellation for request id=1 on the new connection. + // If the old .finally() incorrectly deleted the new controller, this won't work. + transport2.onmessage!({ + jsonrpc: '2.0', + method: 'notifications/cancelled', + params: { + requestId: requestId, + reason: 'test cancel' + } + }); + + // Resolve handler2 so it can check the abort signal + resolveHandler2(); + await vi.advanceTimersByTimeAsync(0); + + expect(wasAborted).toBe(true); + + await transport2.close(); + }); +}); From 1f94a34b60c4c156ce46dba736aa6cc32330cb25 Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Tue, 3 Feb 2026 10:57:07 +0000 Subject: [PATCH 3/4] docs: add fake timers preference to CLAUDE.md --- CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6e768e559..2cbb850df 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,7 +22,7 @@ npm run typecheck # Type-check without emitting - **Files**: Lowercase with hyphens, test files with `.test.ts` suffix - **Imports**: ES module style, include `.js` extension, group imports logically - **Formatting**: 2-space indentation, semicolons required, single quotes preferred -- **Testing**: Co-locate tests with source files, use descriptive test names +- **Testing**: Co-locate tests with source files, use descriptive test names. Use `vi.useFakeTimers()` instead of real `setTimeout`/`await` delays in tests - **Comments**: JSDoc for public APIs, inline comments for complex logic ## Architecture Overview From a20c180111e470d60225f0ed50cd083a945fb07d Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Tue, 3 Feb 2026 13:03:05 +0000 Subject: [PATCH 4/4] style: fix prettier formatting in protocol test --- test/shared/protocol.test.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/shared/protocol.test.ts b/test/shared/protocol.test.ts index 3f4f07e9a..733146f29 100644 --- a/test/shared/protocol.test.ts +++ b/test/shared/protocol.test.ts @@ -5586,9 +5586,13 @@ describe('_onclose cleanup', () => { const request = { method: 'example', params: {} }; const mockSchema = z.object({ result: z.string() }); - const requestPromise = protocol.request(request, mockSchema, { - timeout: 60000 - }).catch(() => {/* expected ConnectionClosed error */}); + const requestPromise = protocol + .request(request, mockSchema, { + timeout: 60000 + }) + .catch(() => { + /* expected ConnectionClosed error */ + }); // Verify the request was sent expect(sendSpy).toHaveBeenCalled(); @@ -5636,7 +5640,9 @@ describe('_onclose cleanup', () => { protocol.setRequestHandler(TestRequestSchema, async () => { resolve(); // signal that handler has started // Wait for explicit resolution - await new Promise(r => { resolveHandler = r; }); + await new Promise(r => { + resolveHandler = r; + }); return { _meta: {} } as Result; }); }); @@ -5666,7 +5672,9 @@ describe('_onclose cleanup', () => { const handler2Started = new Promise(resolve => { protocol.setRequestHandler(TestRequestSchema, async (_request, extra) => { resolve(); - await new Promise(r => { resolveHandler2 = r; }); + await new Promise(r => { + resolveHandler2 = r; + }); wasAborted = extra.signal.aborted; return { _meta: {} } as Result; });