From a014363316305cbb1973084f76936ccfb6da2189 Mon Sep 17 00:00:00 2001 From: mrorigo Date: Tue, 2 Dec 2025 15:07:35 +0100 Subject: [PATCH 1/3] fix(streamableHttp): fix infinite retries when maxRetries is set to 0 closes #1179 --- src/client/streamableHttp.test.ts | 58 +++++++++++++++++++++++++++++++ src/client/streamableHttp.ts | 2 +- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/client/streamableHttp.test.ts b/src/client/streamableHttp.test.ts index db836d127..4a8e24633 100644 --- a/src/client/streamableHttp.test.ts +++ b/src/client/streamableHttp.test.ts @@ -1501,6 +1501,64 @@ describe('StreamableHTTPClientTransport', () => { }); }); + describe('Reconnection Logic with maxRetries 0', () => { + let transport: StreamableHTTPClientTransport; + + // Use fake timers to control setTimeout and make the test instant. + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it('should not retry forever with maxRetries 0', async () => { + // ARRANGE + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 0, // This should disable retries completely + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + } + }); + + const errorSpy = vi.fn(); + transport.onerror = errorSpy; + + const failingStream = new ReadableStream({ + start(controller) { + controller.error(new Error('Network failure')); + } + }); + + const fetchMock = global.fetch as Mock; + // Mock the initial GET request, which will fail + fetchMock.mockResolvedValueOnce({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'text/event-stream' }), + body: failingStream + }); + + // ACT + await transport.start(); + await transport['_startOrAuthSse']({}); + + // Advance time to check if any retries were scheduled + await vi.advanceTimersByTimeAsync(100); + + // ASSERT + // THE KEY ASSERTION: Fetch was only called ONCE. No retries with maxRetries: 0 + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock.mock.calls[0][1]?.method).toBe('GET'); + + // Should still report the error but not retry + expect(errorSpy).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('SSE stream disconnected: Error: Network failure') + }) + ); + }); + }); + + describe('prevent infinite recursion when server returns 401 after successful auth', () => { it('should throw error when server returns 401 after successful auth', async () => { const message: JSONRPCMessage = { diff --git a/src/client/streamableHttp.ts b/src/client/streamableHttp.ts index 9cc4887df..8146b627b 100644 --- a/src/client/streamableHttp.ts +++ b/src/client/streamableHttp.ts @@ -279,7 +279,7 @@ export class StreamableHTTPClientTransport implements Transport { const maxRetries = this._reconnectionOptions.maxRetries; // Check if we've exceeded maximum retry attempts - if (maxRetries > 0 && attemptCount >= maxRetries) { + if (attemptCount >= maxRetries) { this.onerror?.(new Error(`Maximum reconnection attempts (${maxRetries}) exceeded.`)); return; } From ba9013c31a615a89522358941ef36354a1bd5dc3 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 2 Dec 2025 14:18:00 +0000 Subject: [PATCH 2/3] fix: format test file with Prettier Fix formatting issues that were causing CI to fail: - Correct indentation on `let transport` declaration - Remove trailing whitespace - Remove extra blank line --- src/client/streamableHttp.test.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/client/streamableHttp.test.ts b/src/client/streamableHttp.test.ts index 4a8e24633..be3e71268 100644 --- a/src/client/streamableHttp.test.ts +++ b/src/client/streamableHttp.test.ts @@ -1502,7 +1502,7 @@ describe('StreamableHTTPClientTransport', () => { }); describe('Reconnection Logic with maxRetries 0', () => { - let transport: StreamableHTTPClientTransport; + let transport: StreamableHTTPClientTransport; // Use fake timers to control setTimeout and make the test instant. beforeEach(() => vi.useFakeTimers()); @@ -1540,7 +1540,7 @@ describe('StreamableHTTPClientTransport', () => { // ACT await transport.start(); await transport['_startOrAuthSse']({}); - + // Advance time to check if any retries were scheduled await vi.advanceTimersByTimeAsync(100); @@ -1548,7 +1548,7 @@ describe('StreamableHTTPClientTransport', () => { // THE KEY ASSERTION: Fetch was only called ONCE. No retries with maxRetries: 0 expect(fetchMock).toHaveBeenCalledTimes(1); expect(fetchMock.mock.calls[0][1]?.method).toBe('GET'); - + // Should still report the error but not retry expect(errorSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -1558,7 +1558,6 @@ describe('StreamableHTTPClientTransport', () => { }); }); - describe('prevent infinite recursion when server returns 401 after successful auth', () => { it('should throw error when server returns 401 after successful auth', async () => { const message: JSONRPCMessage = { From 06e0b594d3fbba46873a59d7bcb2436c9e4884bb Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 2 Dec 2025 14:21:04 +0000 Subject: [PATCH 3/3] test: improve maxRetries=0 test to verify the actual fix The previous test didn't properly exercise _scheduleReconnection. This improved test: - Directly tests _scheduleReconnection with maxRetries=0 - Verifies the "Maximum reconnection attempts (0) exceeded" error - Confirms no setTimeout is scheduled (no retry attempt) - Adds a companion test for maxRetries>0 to verify normal behavior --- src/client/streamableHttp.test.ts | 61 +++++++++++++++++-------------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/src/client/streamableHttp.test.ts b/src/client/streamableHttp.test.ts index be3e71268..0b979eb99 100644 --- a/src/client/streamableHttp.test.ts +++ b/src/client/streamableHttp.test.ts @@ -1508,7 +1508,7 @@ describe('StreamableHTTPClientTransport', () => { beforeEach(() => vi.useFakeTimers()); afterEach(() => vi.useRealTimers()); - it('should not retry forever with maxRetries 0', async () => { + it('should not schedule any reconnection attempts when maxRetries is 0', async () => { // ARRANGE transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { reconnectionOptions: { @@ -1522,39 +1522,44 @@ describe('StreamableHTTPClientTransport', () => { const errorSpy = vi.fn(); transport.onerror = errorSpy; - const failingStream = new ReadableStream({ - start(controller) { - controller.error(new Error('Network failure')); - } - }); + // ACT - directly call _scheduleReconnection which is the code path the fix affects + transport['_scheduleReconnection']({}); - const fetchMock = global.fetch as Mock; - // Mock the initial GET request, which will fail - fetchMock.mockResolvedValueOnce({ - ok: true, - status: 200, - headers: new Headers({ 'content-type': 'text/event-stream' }), - body: failingStream + // ASSERT - should immediately report max retries exceeded, not schedule a retry + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(errorSpy).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'Maximum reconnection attempts (0) exceeded.' + }) + ); + + // Verify no timeout was scheduled (no reconnection attempt) + expect(transport['_reconnectionTimeout']).toBeUndefined(); + }); + + it('should schedule reconnection when maxRetries is greater than 0', async () => { + // ARRANGE + transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 1, // Allow 1 retry + maxReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1 + } }); - // ACT - await transport.start(); - await transport['_startOrAuthSse']({}); + const errorSpy = vi.fn(); + transport.onerror = errorSpy; - // Advance time to check if any retries were scheduled - await vi.advanceTimersByTimeAsync(100); + // ACT - call _scheduleReconnection with attemptCount 0 + transport['_scheduleReconnection']({}); - // ASSERT - // THE KEY ASSERTION: Fetch was only called ONCE. No retries with maxRetries: 0 - expect(fetchMock).toHaveBeenCalledTimes(1); - expect(fetchMock.mock.calls[0][1]?.method).toBe('GET'); + // ASSERT - should schedule a reconnection, not report error yet + expect(errorSpy).not.toHaveBeenCalled(); + expect(transport['_reconnectionTimeout']).toBeDefined(); - // Should still report the error but not retry - expect(errorSpy).toHaveBeenCalledWith( - expect.objectContaining({ - message: expect.stringContaining('SSE stream disconnected: Error: Network failure') - }) - ); + // Clean up the timeout to avoid test pollution + clearTimeout(transport['_reconnectionTimeout']); }); });