test: deterministic remote-server teardown to kill #1667 afterEach hang

[cliffhall]

Closes #1667

Problem

The remote integration test remote-auth-branches.test.ts → "short-circuits with a transport_error when the session's transport is already dead" intermittently timed out in its 30s afterEach under the full npm run coverage / npm run ci gate (v8 instrumentation + all projects under parallel load), while passing reliably in isolation.

Root cause (issue option 2)

The afterEach closes the Hono HTTP server with server.close(cb). Node's close() only stops accepting new connections and then waits for all existing keep-alive sockets to go idle before firing its callback. The global fetch (undici) keeps its connection pool sockets open, and under heavy parallel/instrumented load they don't reliably go idle before the 30s hook timeout — so close()'s callback never fires and the hook times out.

The dead-transport tests are the worst case: they connect a session whose stdio subprocess crashes and then never disconnect it, so the session (and its dead transport) lingers on the server at teardown.

Fix

1. Deterministic teardown. Call server.closeAllConnections() alongside close() in the teardown helpers, forcibly destroying the pooled keep-alive sockets so the close() callback fires regardless of parallel load. Applied to both remote-auth-branches.test.ts and its sibling connect-crash.test.ts (same crash-on-startup / never-disconnect pattern). Guarded behind an in narrow because ServerType is Server | Http2Server and closeAllConnections is http1-only. Available since Node 18.2; the repo requires Node ≥22.

2. Unref the abandoned-session sweep timer (core/mcp/remote/node/server.ts). 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 finishes.

Verification

• Affected tests: green in isolation.
• Full integration project: green (49 files / 969 tests).
• Full web test:coverage gate (unit + integration under v8 instrumentation — the exact context the flake appeared in): green (298 files / 4426 tests); server.ts at 97.9/92.6/97.9/98.4, above the ≥90 gate.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 3s —— View job

---

Review: test stability fix for #1667

I read the full diff and traced the changed teardown helpers plus the server.ts timer path. This is a focused, correct fix and I'd ship it. Details below.

✅ What's right

1. closeAllConnections() alongside close() — correct and deterministic.
The ordering is exactly right: close() first (stops accepting, registers the callback), then closeAllConnections() forcibly destroys the pooled keep-alive sockets so the callback can fire. This is the canonical Node pattern for the "close() hangs on idle keep-alive sockets" problem, and undici's global-fetch connection pool is precisely the culprit the root-cause analysis names. Because this runs in afterEach (test body already complete), the abrupt socket destruction can't truncate an in-flight assertion.

2. The "closeAllConnections" in h.server narrow is the right guard.
ServerType is Server | Http2Server, and closeAllConnections is declared only on the http1 Server arm, so the in operator narrows cleanly without a cast — consistent with the "NEVER suppress / avoid casts" TS rules. Since serve() returns an http1 server here the branch is always taken at runtime, but keeping the guard is the correct way to stay type-safe against the union.

3. .unref() on the abandoned-session sweep — right call, right reasoning. (core/mcp/remote/node/server.ts:645)
This timer is best-effort GC of a session whose client never opens the events stream. In production the process stays alive on the HTTP server itself, so unref() changes nothing there; it only matters at the tail, where a dangling 30s timer could otherwise pin a test worker's event loop. Correct semantics, and the inline comment explains the why well. I confirmed the other setTimeout in the file (server.ts:786) is an awaited await 0 yield, so it's not a leak source — this was the only timer worth unref-ing.

🔎 Minor / optional (non-blocking)

• Duplicated teardown helper + comment across the two test files. stop() in remote-auth-branches.test.ts and teardown() in connect-crash.test.ts are now near-identical, including the multi-line rationale comment. A shared closeHarnessServer(server) helper (e.g. under src/test/integration/mcp/remote/) would DRY this and give one place to maintain the reasoning. Given it's two small files, extracting now is a judgment call — fine to leave as-is.
• Consideration only: closeAllConnections() also destroys any active connection, not just idle ones. That's harmless here (teardown), but if this helper is ever lifted into a shared util used mid-test, closeIdleConnections() would be the gentler primitive. Not a concern for the current call sites.

Verdict

Correct root-cause fix, minimal blast radius, faithful comments, and it keeps the server.ts coverage above the ≥90 gate per your notes. No changes required. 👍
· branch v2/test-stability-1667

[cliffhall]

Thanks for the thorough review! Responses to the two optional items:

1. Duplicated teardown helper + comment — Implemented. Extracted closeHarnessServer(server) into src/test/integration/mcp/remote/harnessTeardown.ts, and both stop() (remote-auth-branches) and teardown() (connect-crash) now delegate to it, so the close()+closeAllConnections() logic and its rationale live in one place. (653ce0b)

2. closeAllConnections() vs closeIdleConnections() — Leaving as closeAllConnections(). In teardown we want all sockets gone deterministically, including the dead-transport/crash-on-startup sessions that never disconnect; closeIdleConnections() would leave any still-active socket to block close()'s callback, which is exactly the hang we're fixing. As you noted, it's not a concern for these call sites — and the shared helper's doc comment now spells out that it's teardown-only.

Verified after the refactor: affected tests green (2 files / 11 tests), tsc -b clean.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 36s —— View job

---

Re-review: shared teardown helper refactor (#1667)

I re-read all four changed files, including the new harnessTeardown.ts and both call sites after the DRY refactor (653ce0b). The previous review's substantive analysis of the fix still holds; this pass focuses on the new shared helper.

✅ What's right

1. closeHarnessServer cleanly consolidates the fix. harnessTeardown.ts:20 now owns the close() + closeAllConnections() logic and its full rationale comment, and both stop() (remote-auth-branches.test.ts:48) and teardown() (connect-crash.test.ts:40) delegate to it. Single source of truth, exactly as suggested — and the thin per-file wrappers keep each test's local naming.

2. Ordering is still correct. close() registers the callback first, then closeAllConnections() destroys the pooled keep-alive sockets so the callback can fire regardless of load. Wrapping the whole thing in one Promise<void> and awaiting it preserves the deterministic teardown.

3. The "closeAllConnections" in server narrow is preserved. Still the right type-safe guard against the Server | Http2Server union — no cast, consistent with the TS rules. The doc comment explains why the branch is always taken at runtime (http1 server from serve()).

4. .unref() on the abandoned-session sweep (server.ts:645) is unchanged and correct — best-effort GC that shouldn't pin a test worker's event loop; production still stays alive on the HTTP server itself.

🔎 Minor / optional (non-blocking)

• The closeAllConnections vs closeIdleConnections choice was raised and resolved in the discussion — closeAllConnections() is right for teardown, and the helper's doc comment now documents it as teardown-only. No action needed.
• The Http2Server arm of the in narrow is unreachable at runtime, so its false branch is never exercised — but this is test-harness code (not under the ≥90 coverage gate), so no v8 ignore annotation is warranted. Just noting for completeness.

Verdict

The refactor addresses the one actionable item from the prior review without changing the fix's behavior. Correct root-cause fix, faithful comments, one place to maintain the reasoning. Ship it. 👍
· branch v2/test-stability-1667

[cliffhall]

Thanks for the re-review — no actionable items this pass, so I'll leave the PR as-is. Both minor notes (the closeAllConnections teardown-only choice and the unreachable Http2Server narrow branch in test-harness code) are covered by the discussion/doc comment and need no change.