From 926b8fb363b24df795d19bd65168fa3a5d098a91 Mon Sep 17 00:00:00 2001
From: Aaron Bockelie <aaronsb@gmail.com>
Date: Wed, 24 Jun 2026 02:00:33 -0500
Subject: [PATCH] =?UTF-8?q?refactor(mcp):=20address=20review=20of=20#244?=
 =?UTF-8?q?=20=E2=80=94=20document=20SSE=20backstop,=20test=20teardown?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Code review refinements (no behavior change):
- Make the SSE socket-exemption trade-off explicit: with setTimeout(0)
  there is no socket-layer idle reap, so a truly-dead SSE socket is
  bounded by the SessionManager's 1h idle eviction (which calls
  transport.close()) and the 32-session cap, not leaked unbounded. Note
  the finite-timeout alternative is deferred to live validation.
- Clarify the comment: the exemption matches any GET, but non-SSE GETs
  are short-lived so it's a harmless no-op for them.
- Add afterEach teardown disposing the SessionManager + connection-pool
  intervals (server.stop() short-circuits when never started), removing
  the test's leaked-timer warning.
- Strengthen the missing-socket test to assert the request still routes
  to the spec §2 400, not just that it doesn't throw.

Claude-Session: https://claude.ai/code/session_011yowKCMFCBp61DRBu5xQm4
---
 src/mcp-server.ts                | 15 +++++++++++++--
 tests/sse-socket-timeout.test.ts | 15 +++++++++++++--
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/mcp-server.ts b/src/mcp-server.ts
index de5df37..0ba69e3 100644
--- a/src/mcp-server.ts
+++ b/src/mcp-server.ts
@@ -443,8 +443,19 @@ export class MCPHttpServer {
       // idle by design (this server pushes no server-initiated notifications).
       // The global 120s socket timeout set in start() would otherwise reap it
       // every ~2 min, surfacing as a "stream terminated" reconnect churn in
-      // bridges and noisy logs (#221). Exempt only this socket from the idle
-      // timeout; POST request sockets keep the server-wide default.
+      // bridges and noisy logs (#221). Disable the idle timeout on this GET
+      // socket; POST request sockets keep the server-wide default. (A non-SSE
+      // GET is short-lived — its response is sent immediately — so the
+      // exemption is a harmless no-op for those.)
+      //
+      // Trade-off: with no socket-layer idle reap, a truly-dead SSE socket
+      // (client vanished without a FIN) is no longer dropped at ~2 min. The
+      // backstop is the SessionManager's 1h idle eviction, which calls
+      // transport.close() and thereby closes the socket — so abandoned streams
+      // are bounded by that timeout and the 32-session cap, not leaked
+      // unbounded. A long finite timeout was weighed against 0; the choice is
+      // deferred to the live-instance validation, since a finite value would
+      // reintroduce some (less frequent) reconnect churn.
       if (req.method === 'GET') {
         req.socket?.setTimeout(0);
       }
diff --git a/tests/sse-socket-timeout.test.ts b/tests/sse-socket-timeout.test.ts
index 9fd1cec..0882b43 100644
--- a/tests/sse-socket-timeout.test.ts
+++ b/tests/sse-socket-timeout.test.ts
@@ -68,6 +68,14 @@ describe('SSE notification-stream socket timeout exemption (#221)', () => {
     server = new MCPHttpServer(mockApp, 3001);
   });
 
+  afterEach(async () => {
+    // The constructor arms the SessionManager + connection-pool intervals;
+    // server.stop() short-circuits when never started, so dispose them directly
+    // to avoid leaking timers across tests.
+    (server as any).sessionManager?.stop?.();
+    await (server as any).connectionPool?.shutdown?.();
+  });
+
   test('GET /mcp (SSE stream) disables its socket idle timeout', async () => {
     const { req, socket } = makeReq('GET', undefined, 'some-session');
     await (server as any).handleMCPRequest(req, makeRes());
@@ -80,9 +88,12 @@ describe('SSE notification-stream socket timeout exemption (#221)', () => {
     expect(socket.setTimeout).not.toHaveBeenCalled();
   });
 
-  test('exemption tolerates a missing socket (no throw)', async () => {
+  test('exemption tolerates a missing socket and still routes the request', async () => {
     const req = { method: 'GET', body: undefined, headers: {} as Record<string, string> };
     const res = makeRes();
-    await expect((server as any).handleMCPRequest(req, res)).resolves.toBeUndefined();
+    await (server as any).handleMCPRequest(req, res);
+    // No socket to exempt → optional chaining no-ops; the GET (no session, not
+    // an initialize) still routes to the spec §2 "session required" 400.
+    expect(res.statusCode).toBe(400);
   });
 });