From 40aaf477051cc088b663f0d46abc6fd2e2b80b41 Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Fri, 24 Jul 2026 12:20:54 -0400 Subject: [PATCH] test: deterministic remote-server teardown to kill #1667 afterEach hang (#1763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: deterministic remote-server teardown to kill #1667 afterEach hang The remote integration tests close their Hono HTTP server in afterEach with `server.close(cb)`, which waits for all keep-alive sockets to go idle before firing the callback. undici's global-fetch connection pool holds sockets open, and under the full parallel/instrumented `npm run coverage` load they may not go idle before the 30s hook timeout — intermittently failing the dead-transport tests in remote-auth-branches.test.ts. The crash-on-startup / dead-session tests are the worst case: they never disconnect, so the session and its dead subprocess transport linger when the server closes. Force the sockets closed with `server.closeAllConnections()` alongside close() so the callback fires deterministically regardless of parallel load, in both remote-auth-branches.test.ts and its sibling connect-crash.test.ts. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * server: unref the abandoned-session sweep timer The transport-onclose path arms a 30s setTimeout to GC a session whose client never opens the events stream. That sweep is best-effort cleanup, not work the process must stay alive for — unref it so a pending timer can't keep the event loop (or a test worker) alive for 30s after everything else has finished. Also narrow closeAllConnections() behind an `in` guard in the two remote teardown helpers: ServerType is Server | Http2Server and the method is http1-only, so the direct call failed tsc. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test: extract shared closeHarnessServer teardown helper Address review feedback on #1763: the deterministic close()+closeAllConnections teardown (and its multi-line rationale) was duplicated across remote-auth-branches.test.ts and connect-crash.test.ts. Extract it to a shared closeHarnessServer(server) helper so the logic and its comment live in one place. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../mcp/remote/connect-crash.test.ts | 3 ++- .../integration/mcp/remote/harnessTeardown.ts | 25 +++++++++++++++++++ .../mcp/remote/remote-auth-branches.test.ts | 3 ++- core/mcp/remote/node/server.ts | 6 ++++- 4 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 clients/web/src/test/integration/mcp/remote/harnessTeardown.ts diff --git a/clients/web/src/test/integration/mcp/remote/connect-crash.test.ts b/clients/web/src/test/integration/mcp/remote/connect-crash.test.ts index 15e6589a..5e7488c6 100644 --- a/clients/web/src/test/integration/mcp/remote/connect-crash.test.ts +++ b/clients/web/src/test/integration/mcp/remote/connect-crash.test.ts @@ -10,6 +10,7 @@ import { serve } from "@hono/node-server"; import type { ServerType } from "@hono/node-server"; import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; +import { closeHarnessServer } from "./harnessTeardown.js"; interface Harness { baseUrl: string; @@ -37,7 +38,7 @@ async function startServer(): Promise { } async function teardown(h: Harness): Promise { - await new Promise((resolve) => h.server.close(() => resolve())); + await closeHarnessServer(h.server); } interface ParsedSseEvent { diff --git a/clients/web/src/test/integration/mcp/remote/harnessTeardown.ts b/clients/web/src/test/integration/mcp/remote/harnessTeardown.ts new file mode 100644 index 00000000..114276c3 --- /dev/null +++ b/clients/web/src/test/integration/mcp/remote/harnessTeardown.ts @@ -0,0 +1,25 @@ +import type { ServerType } from "@hono/node-server"; + +/** + * Deterministically close a Hono-served HTTP server in test teardown. + * + * Node's `server.close(cb)` stops accepting new connections and then waits for + * every existing keep-alive socket to go idle before firing its callback. + * undici's global-`fetch` connection pool holds sockets open, and under the + * full parallel/instrumented `npm run coverage` load they may not go idle + * before the 30s `afterEach` timeout — the #1667 teardown hang. Some of these + * suites' sessions (crash-on-startup / dead-transport) never disconnect, so the + * session and its dead subprocess transport also linger at close time. + * + * Force the pooled sockets closed with `closeAllConnections()` alongside + * `close()` so the callback fires regardless of load. `closeAllConnections` is + * declared only on http1's `Server` (absent on the `Http2Server` arm of + * `ServerType`); `serve()` returns an http1 server here, so the `in` narrow is + * always taken at runtime while keeping the union type-safe without a cast. + */ +export async function closeHarnessServer(server: ServerType): Promise { + await new Promise((resolve) => { + server.close(() => resolve()); + if ("closeAllConnections" in server) server.closeAllConnections(); + }); +} diff --git a/clients/web/src/test/integration/mcp/remote/remote-auth-branches.test.ts b/clients/web/src/test/integration/mcp/remote/remote-auth-branches.test.ts index 3a2100e5..5f992d45 100644 --- a/clients/web/src/test/integration/mcp/remote/remote-auth-branches.test.ts +++ b/clients/web/src/test/integration/mcp/remote/remote-auth-branches.test.ts @@ -18,6 +18,7 @@ import type { ServerType } from "@hono/node-server"; import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-server"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; +import { closeHarnessServer } from "./harnessTeardown.js"; interface Harness { baseUrl: string; @@ -45,7 +46,7 @@ async function start(): Promise { } async function stop(h: Harness): Promise { - await new Promise((resolve) => h.server.close(() => resolve())); + await closeHarnessServer(h.server); } async function connect( diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index ce7f085d..4f5c2a2d 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -633,12 +633,16 @@ export function createRemoteApp( // the user. The endpoint cleans up on stream close; this TTL // sweeps sessions whose client never connects at all. if (!session.hasEventConsumer()) { + // This sweep is a best-effort GC of an abandoned session, not work + // the process must stay alive for — unref it so a pending timer + // can't keep the event loop (or a test worker) alive for 30s after + // everything else has finished. setTimeout(() => { const stale = sessions.get(sessionId); if (stale && !stale.hasEventConsumer()) { sessions.delete(sessionId); } - }, 30_000); + }, 30_000).unref(); } } else { // Session not created yet - failed during start