Conversation
The pool kept its state in two parallel maps keyed by server id plus a hash of the caller's bearer token, and that split bookkeeping hid four defects: a stdio child was spawned per token although the token never reaches the child process; nothing ever removed an entry, so every token rotation leaked one (and, for stdio, a process) for the life of the middleware; closing an in-flight connect left the client it later resolved to unowned; and no route or shutdown path invalidated anything, so a deleted, reconfigured or disconnected server kept being served with its old command, env, headers and token. One entry map replaces the two, keyed by exactly what each transport consumes, with a lazily-swept idle TTL and explicit invalidation from the three mutating operator routes and from shutdownBuilder. close(serverId) is widened to drop every token-scoped entry of that server — the scope rule is exported as mcpPoolScopeMatches so the no-collateral guarantee lives in one place and can be tested. The subsystem had no pooling coverage at all, which is why the defects survived; the new suites assert on real spawned child processes rather than on the internal map.
npm test runs one OS process per test *file*, all in parallel. The pool work landed two new files, and this pair was the only place in the suite spawning grandchild processes and booting loopback listeners, so it added the largest per-file footprint of the branch to a run that is already CPU-saturated. Fold the route-invalidation assertions into test/mcpPool.test.ts so the branch adds one scheduled process instead of two; serve all three route assertions from a single express listener instead of one per test (the store stubs are stateless, so sharing is safe); and let AC4's "closeAll kills the child" ride on AC1's already-pooled child instead of spawning its own. Both suites now state concurrency: 1 explicitly — it is the default, but it is load-bearing here because these tests count child processes. Coverage is unchanged: same eight assertions, same acceptance criteria, all still asserted on process identity and at the route boundary.
Resolves five conflicts against main (#550 MCP waves 0-6, #624, #613): - mcpClient.ts: keep BOTH the new pool-lifetime members (entries map, idleTtlMs, MCP_POOL_IDLE_TTL_MS, mcpPoolScopeMatches) and main's structuredSink / pendingInput options and outputSchemas cache. - src/index.ts: keep the runtimeMcpManager handle alongside main's structured-sink wiring and the W2-1 (#544) input replayer. - routes/agentBuilder.ts: keep main's W0-1 ownership check on DELETE /mcp-servers/:id/token (404/403 fail-closed) and invalidate the pooled connection after the token row is deleted. - docs/adr: main landed 0007-mcp-client-id-metadata-documents (2026-07-30) first, so this ADR is renumbered 0007 -> 0008 and every reference updated. - CHANGELOG: both Unreleased entries retained. Also fixes the mcpPool fixture: serverRow() predates #550 and omitted the required `delegation` field, so resolveMcpUserKey failed closed and the token-revocation test got 403 instead of 204. Verified on the merge result: lint, typecheck and the core-decoupling ratchet pass, and `npm test` is green 3/3 (6075 pass, 0 fail, ~35-49s) -- the parallelism failures documented in the PR body are resolved by #613, now on main. Both new behaviours mutation-checked: disabling onMcpServerChanged turns 3 tests red, weakening the '#' pool-key separator turns 2 red.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes #563. Gives pooled MCP connections an explicit lifetime.
The MCP pool kept its state in two parallel maps keyed by server id plus a hash of
the caller's bearer token. Since a stdio child process never sees that token, N callers
with N tokens spawned N identical child processes for the same server. This replaces the
two maps with one entry map and one set of rules:
http/sse by server id + token hash.
close(serverId), idle-TTL expiry, andcloseAll().getOrConnect, never a timer, so nothing keeps theprocess or
node --testalive).SIGTERM/SIGINT no longer leaves stdio children behind.
Rationale and rejected alternatives:
docs/adr/0008-mcp-connection-lifetime.md.Status — the previously-documented blocker is resolved
An earlier revision of this description reported that
cd middleware && npm testfailed4 of 4 runs on this branch, in unrelated suites, whenever both new test files were
present. That was the parallelism/resource-exhaustion problem tracked as #564.
#613 fixed it and is now on main (
71079cac). This branch has been merged up to mainand re-verified end to end:
npm test(middleware)npm run lintnpm run typechecknode scripts/check-core-decoupling.mjsMerging main required five conflict resolutions
Main moved a long way (#550 MCP waves 0-6, #624, #613), so this is not a trivial
fast-forward:
mcpClient.ts— kept both this PR's pool-lifetime members and main'sstructuredSink/pendingInputoptions andoutputSchemascache.src/index.ts— kept theruntimeMcpManagerhandle alongside main's structured-sinkwiring and the W2-1 (Support MRTR (resultType: input_required) — mid-call user input for MCP tools #544) input replayer.
routes/agentBuilder.ts— kept main's W0-1 ownership check onDELETE /mcp-servers/:id/token(404/403, fail-closed) and added the poolinvalidation after the token row is deleted.
0007-mcp-client-id-metadata-documents.md(dated 2026-07-30) first, so this ADR was renumbered 0007 → 0008 and every
reference updated.
CHANGELOG.md— both Unreleased entries retained.One real test fix fell out of the merge:
mcpPool.test.ts'sserverRow()fixture predates#550 and omitted the now-required
delegationfield, soresolveMcpUserKeyfailed closedand the token-revocation test saw 403 instead of 204.
The new tests are load-bearing (mutation-checked)
Green runs alone do not prove coverage, so both new behaviours were mutated:
options.onMcpServerChanged?.(serverId)→ 3 tests fail.#pool-key separator to a barestartsWith(id)→ 2 tests fail.(Note: these tests import the built package, so a
src/mutation only bites afterrebuilding
harness-orchestrator— the first attempt silently passed against staledist/.)Behaviour changes worth a reviewer's attention
McpManager.close(serverId)semantics widened. It now closes every token-scopedconnection of that server rather than one exact pool key. Passing a full pool key still
matches only itself, and a server id never matches a different server whose id shares its
prefix (
mcpPoolScopeMatches).makeTransportthe stdio branch consumes onlycommand/args/env, andenvcomesfrom
getConfigEnv(cfg), which is keyed by server config with no caller identity. Nocaller-specific material reaches the child, so de-duplication is safe. If stdio ever
gains token-derived env,
poolKeymust change back in lockstep.idleTtlMs), so a rarely-used stdioserver pays a respawn on next use.
Known follow-up (not a blocker)
MCP_POOL_IDLE_TTL_MS(300 s) is a new member of this codebase's timeout hierarchy but isnot registered in
test/orchestrator/timeoutHierarchy.test.ts, which exists preciselyto catch inversions of this kind. With defaults it is safe — the absolute call ceiling is
180 s and the dispatch deadline 240 s, both below the TTL — but nothing pins that ordering.
Lowering
idleTtlMsbelow the call ceiling (or raisingOMADIA_MCP_CALL_MAX_TOTAL_TIMEOUT_MSabove the TTL) would letevictIdleclose aconnection out from under an in-flight call, since
lastUsedAtis refreshed ongetOrConnectand not for the duration of the call. Worth a one-line invariant assertionin a follow-up.
Issue: #563