fix(atlasd): kill orphan MCP probe subprocesses on client abort (#344)#410
Open
LissaGreense wants to merge 12 commits into
Open
fix(atlasd): kill orphan MCP probe subprocesses on client abort (#344)#410LissaGreense wants to merge 12 commits into
LissaGreense wants to merge 12 commits into
Conversation
withTimeout was a fire-and-forget Promise.race: when the timer won, the inner work kept running, leaking stdio MCP children whose clients were never closed. Add an optional `signal` so callers can also race an abort, and ensure the timer + abort listener are cleared on every exit path. This is foundational for threading c.req.raw.signal through the connect chain (task #3) — once the helper races the signal correctly, the attemptStdio abort listener can be wired up on top. ## Progress - Task: #2 — Make withTimeout signal-aware in packages/mcp/src/create-mcp-tools.ts - Decisions: Exported withTimeout so it can be unit-tested directly; the AC explicitly calls out timer-cleared assertions that are awkward to observe through the public createMCPTools API. - Key Learnings: Vitest fake-timer tests must attach the `expect(p).rejects` assertion *before* advancing timers — otherwise the synchronous timeout rejection fires with no handler attached and surfaces as an unhandled rejection (`Vitest caught 1 unhandled error`), even though all `it()` cases pass. `vi.getTimerCount()` is the clean way to assert the timer-cleared invariant. - Files: packages/mcp/src/create-mcp-tools.ts, packages/mcp/src/create-mcp-tools.test.ts
The `GET /api/mcp-registry/:id/tools` handler now passes `c.req.raw.signal` into `probeAndExtract` and integrates it into the in-flight prewarm race so client disconnects stop the route's wait immediately. `probeAndExtract` composes the parent signal with its internal timeout via `AbortSignal.any`, letting whichever aborts first win. The race against an in-flight prewarm adds a third sentinel (`ABORTED`) so client abort cancels the *wait on* the prewarm without canceling the prewarm itself — prewarms retain their own 60s lifecycle so the next caller can still hit the warm cache. Partial fix for #344: the route returns earlier on client abort and any successfully-connected MCP clients get disposed by `createMCPTools`' existing post-settle dispose path. Subprocess teardown (so stdio children actually die on abort/timeout instead of orphaning for ~20s) lands in the follow-up PR covering tasks 3+4. ## Progress - Task: #1 — Tracer Bullet: thread c.req.raw.signal from MCP probe route to createMCPTools - Decisions: - Kept `probeAndExtract` signature positional (`signal?: AbortSignal` last) per the AC interface contract. - For the in-flight prewarm race, used a third `ABORTED` sentinel + one-shot abort listener (removed on race resolution) instead of canceling the underlying prewarm promise — design doc § "What does NOT change" explicitly preserves prewarm's independent lifecycle. - Throwing `clientSignal.reason` on the abort branch matches `createMCPTools`' existing post-settle dispose behavior; Hono silently drops the response write when the socket is already closed (per design doc risk analysis). - Key Learnings: - `AbortSignal.any([parent, AbortSignal.timeout(ms)])` is the established composition pattern in this repo (`packages/core/src/delegate/index.ts`, `tools/agent-playground/src/lib/server/routes/export.ts`). - `createMCPTools`' existing signal contract has TWO checkpoints: a sync pre-check (throws synchronously on already-aborted signal) and a post-settle dispose-then-throw. Tests must mirror this behavior in mocks to validate the composed-signal path end-to-end. - Files: - apps/atlasd/routes/mcp-tool-cache.ts — `probeAndExtract` gains optional `signal`; composes with internal timeout. - apps/atlasd/routes/mcp-registry.ts — `GET /:id/tools` captures `c.req.raw.signal`, passes to `probeAndExtract`, and races it against the in-flight prewarm with an `ABORTED` sentinel. - apps/atlasd/routes/mcp-tool-cache.test.ts (new) — unit tests for pre-aborted, mid-flight abort, timeout-fallthrough, and happy paths.
Adds two route-level tests to lock in the prewarm-race lifecycle invariants the lead flagged after task #1's plan approval: - "client abort wins the race" — asserts the route does NOT return tools (Hono catches the throw and surfaces a 500), the prewarm continues running independently, and exactly one createMCPTools call was issued. - "prewarm wins the race" — asserts the happy path still returns tools and that a post-resolution abort on the same signal is inert (no leaked listener; relies on the `{once: true}` + explicit removeEventListener belt-and-suspenders cleanup in the route). ## Progress - Task: #1 follow-up — listener-cleanup invariant tests requested by lead - Decisions: - Used `mcpRegistryRouter.request(url, { signal })` to wire AbortController into Hono's `c.req.raw.signal`. Confirmed Hono's `app.request` does NOT propagate the signal into a rejected fetch — it lets the handler throw and surfaces a 500. Adjusted the assertion accordingly. - Cannot introspect AbortSignal listeners via public API, so the "no leak" assertion is indirect: a leaked listener would cause an unhandled rejection in vitest. Test passes cleanly = no leak. - Key Learnings: - `app.request(url, { signal })` in Hono test mode: aborting the signal mid-handler does NOT reject the request promise. The handler's thrown error becomes a 500 response instead. Tests asserting `rejects` on the request promise will fail. - Files: apps/atlasd/routes/mcp-registry.test.ts
Thread the optional AbortSignal from createMCPTools through the connect
chain (connectServerWithTimeout → connectServer → connectStdio →
attemptStdio, and the parallel HTTP branch). attemptStdio now (a) pre-
checks signal.aborted before spawning, (b) re-checks immediately after
createMCPClient resolves to close the race window — calling client.close()
on the just-spawned child and throwing, and (c) registers a one-shot
signal.addEventListener('abort', ...) that survives attemptStdio's return
so a later abort still closes the client. close() propagates to
StdioMCPTransport.abortController.abort() → Node's spawn signal option →
SIGTERM, so the spawned child actually dies instead of orphaning for the
full ~20s LIST_TOOLS_TIMEOUT_MS.
For the timeout-only path, connectServerWithTimeout derives an internal
AbortController and combines it with the external signal via
AbortSignal.any. When the timer fires, the controller is aborted, which
fires the same downstream listener — so both abort and timeout paths
collapse the orphan subprocess window to <2s.
connectHttp's sharedMCPProcesses.acquire is deliberately NOT given the
signal: the daemon-scoped process registry intentionally outlives any
individual probe. Signal only affects the MCP-client construction layer.
withTimeout gains a defensive no-op promise.catch(() => {}) on the inner
promise: once attemptStdio throws on the abort path, the orphan
connectServer rejection (after the race already settled) would otherwise
surface as an unhandled rejection.
This closes #344's subprocess-leak side. The route-level signal plumbing
was already in place from task #1.
## Progress
- Task: #3 — Thread signal through connect chain and register abort listener in attemptStdio
- Decisions: (1) Did NOT remove the abort listener in attemptStdio's
finally — making it `{ once: true }` and leaving it attached lets a
late signal abort still close the spawned client after attemptStdio
has returned ok:true. The listener is request-scoped via the route
signal, so unfired listeners are GC'd with the request. (2) Used
AbortSignal.any in connectServerWithTimeout to fold the timeout into
the downstream signal, so both abort and timeout paths converge on the
same kill path. (3) Updated the existing "disposes all connected
clients when signal aborts mid-connect" test: with the new early
attemptStdio checks, the second mapper short-circuits before calling
createMCPClient — old expectation was 2 calls, new behavior is 1 call.
The spirit (client gets disposed on abort) is preserved.
- Key Learnings: (1) `Promise.race([p.finally(cleanup), timeoutPromise])`
treats the inner promise as orphan after the race settles, but the
inner's *eventual* rejection still has to be observed somewhere or
Node/Vitest report unhandled rejection. A bare `promise.catch(() => {})`
next to the race is enough — it doesn't change race semantics because
it's a sibling subscriber, not a replacement. (2) AI SDK's
StdioMCPTransport.close() abort propagation is the only sanctioned way
to SIGTERM the spawned child — the private `process?` field is not
reachable directly. (3) Synchronous signal.aborted right after
`await createMCPClient` covers the race where abort happened during
the handshake; without that check, the listener attaches *after*
abort and never fires.
- Files: packages/mcp/src/create-mcp-tools.ts, packages/mcp/src/create-mcp-tools.test.ts
Adds real-subprocess tests that capture the spawned PID via `pgrep -P` and assert `process.kill(pid, 0)` throws ESRCH within 2s of (a) external signal abort and (b) `withTimeout` firing through `connectServerWithTimeout`'s `AbortSignal.any` wiring. These pin the AI SDK contract dependency (transport .close → abortController.abort → SIGTERM) as a tripwire so it breaks loudly on an `@ai-sdk/mcp` version bump instead of silently regressing into the #344 orphan-leak. Surfaced a gap in task #3's listener placement and fixed it: the abort listener was registered AFTER `await createMCPClient(...)`. Against the exact #344 failure mode (server hangs at the MCP `initialize` handshake) that await never resolves, the listener never attaches, and the orphan window collapses back to ~20s. Hoisted the `StdioMCPTransport` to a local so the listener can call `transport.close()` BEFORE the await — which SIGTERMs the child and tears down the in-flight handshake via the transport's onclose handler. The catch and post-await `signal?.aborted` checks are preserved so abort still surfaces as `signal.reason` instead of "Connection closed". Test file deliberately does NOT mock `@ai-sdk/mcp` — only the real spawn → real SIGTERM path proves the leak is gone. PID capture via `pgrep -P` process-tree diff because Vitest's ESM namespace is frozen and `vi.spyOn(childProcess, "spawn")` fails with "Cannot redefine property". Skipped on Windows since `ESRCH` is POSIX. Existing `create-mcp-tools.test.ts` needed one mock fix: default `MockStdioTransport` now sets `close: () => Promise.resolve()` so the new pre-await listener doesn't throw in tests that mock the transport as a bare `vi.fn()`. ## Progress - Task: #4 — Assert subprocess PID is gone after abort/timeout in attemptStdio - Decisions: (1) Fixed the gap in task #3's listener placement rather than testing around it. The AC explicitly specifies a slow server that hangs at the handshake (`setInterval`/`stdin.resume`) — Patina's post-await listener can't observe abort during the handshake, so the AC was unsatisfiable without the fix. (2) Added `export` to `attemptStdio` so tests can drive it directly with a fabricated AbortController instead of threading through `createMCPTools`. Minimum surface change — one keyword. (3) `pgrep -P` over `vi.spyOn(childProcess, "spawn")` because ESM module namespace is frozen in Vitest. (4) Test B inlines `connectServerWithTimeout`'s wiring with a 200ms timeout via the already- exported `withTimeout`, so no need to mutate `LIST_TOOLS_TIMEOUT_MS`. - Key Learnings: (1) `@ai-sdk/mcp`'s `createMCPClient` does not accept a signal — `MCPClientConfig` only has transport/onUncaughtError/clientName/ capabilities. Any signal-observing teardown must hook the transport instance, not the createMCPClient call. (2) `StdioMCPTransport.close()` fires the transport's internal `abortController.abort()`, which is what the constructor passed to `spawn(..., { signal })` — that's the only sanctioned SIGTERM path, the `process?` field is private. Same mechanism works whether the transport is mid-handshake or fully connected. (3) `vi.spyOn(childProcess, "spawn")` throws "Cannot redefine property" in ESM Vitest — module namespace is frozen. For real-spawn tests, use OS process-tree inspection (`pgrep -P $$` diff) instead. (4) JS class constructors via `vi.fn()` return an empty `this` — any production code that calls a method on the constructed instance needs the test to set that method in `mockImplementation(function() { this.foo = ... })`, otherwise the call throws `is not a function` from inside async event handlers (uncaught). - Files: packages/mcp/src/create-mcp-tools.ts, packages/mcp/src/create-mcp-tools.test.ts, packages/mcp/src/create-mcp-tools.subprocess-kill.test.ts
Test B inlines `connectServerWithTimeout`'s signal-composition (`AbortController` + `AbortSignal.any` + `timeoutController.abort()` in `makeError`) at a 200ms timeout. If the production wiring changes, the test would silently keep passing while no longer testing the real path. Comment now names `connectServerWithTimeout` explicitly and instructs future editors to mirror any composition changes. Per lead note on task #4. ## Progress - Task: #4 — follow-up comment per lead approval - Decisions: None — pure comment edit, no behavior change. - Key Learnings: None - Files: packages/mcp/src/create-mcp-tools.subprocess-kill.test.ts
End-to-end test that demonstrates #344 is closed. Real `Deno.serve` hosts the `mcpRegistryRouter`, real Node `fetch` with `AbortController` hits `GET /:id/tools`, real `createMCPTools` spawns a slow stdio MCP server (`node -e 'setInterval(...); process.stdin.resume()'` — the same fixture as `packages/mcp/src/create-mcp-tools.subprocess-kill.test.ts`). Aborting the client after the spawn lands must result in `process.kill(pid, 0)` throwing ESRCH within ~2s — anything else means the orphan-leak regression is back. Lives in its own file (not extending `mcp-registry.test.ts`) because that file does `vi.mock("@atlas/mcp", ...)` globally and the whole point of this test is to NOT mock the spawn path. Per-file `vi.mock` scope keeps the two suites cleanly isolated. Server entries are seeded directly via `getMCPRegistryAdapter().add(...)` instead of through `POST /` — the POST handler kicks off `prewarmTools` which would race the foreground probe and possibly observe the abort first. Direct adapter seeding keeps the GET handler on its no-cache / no-in-flight / foreground `probeAndExtract` branch, which is the branch #344 is about. Verified deterministic with 10 sequential runs (~175ms test runtime each) and alongside the existing 101-test `mcp-registry.test.ts` (102/102 pass). ## Progress - Task: #5 — Route-level integration test: client abort kills MCP subprocess (closes #344) - Decisions: (1) Real `Deno.serve` on `:0` + real Node `fetch` over the in-process `mcpRegistryRouter.request(...)` pattern from `mcp-registry.test.ts`, because only a real socket close fires `c.req.raw.signal` — the in-process helper synthesises a Request with no underlying transport, so `signal` never aborts even when the caller intends it to. (2) Seeded the dynamic registry adapter directly instead of POSTing through the registry route, so the GET hits the foreground probe branch (not the in-flight-prewarm wait branch). The shared JetStream KV bucket from `vitest.setup.ts:initMCPRegistryAdapter(nc)` is already initialized — random `crypto.randomUUID()` suffix in the server ID avoids cross-run collisions, and `afterEach` calls `adapter.delete` to keep the bucket clean. (3) Reused Ellie's `pgrep -P` baseline-then- diff pattern verbatim for PID capture rather than inventing a new signal-route — the constraint that motivated it (Vitest ESM namespaces are frozen, `vi.spyOn(child_process, "spawn")` throws "Cannot redefine property") still applies in this file. (4) Mounted the router at "/" instead of the production "/api/mcp-registry" prefix — the prefix is irrelevant to what we're testing (signal propagation through the handler) and a flat mount keeps URLs short. - Key Learnings: (1) `mcpRegistryRouter.request(path, init)` (the in-process Hono helper used pervasively in `mcp-registry.test.ts`) synthesises a Request whose `signal` is permanently un-aborted — socket-close behavior is not modelled. Any route test that needs to exercise `c.req.raw.signal` MUST go through real `Deno.serve` + real `fetch` + real `AbortController`. (2) `vitest.setup.ts` calls `initMCPRegistryAdapter(nc)` against the shared NATS test server, so `getMCPRegistryAdapter()` returns a real JetStream-backed adapter in every test file by default — `mcp-registry.test.ts` only swaps in a `LocalMCPRegistryAdapter`-on-Deno.Kv via `vi.mock` because it needs isolation between many tests; a single-test file with a unique server ID can use the JetStream adapter directly and just delete the entry in `afterEach`. (3) `c.req.raw.signal` aborts via Deno's `Deno.serve` when the client closes its socket — confirmed end-to-end here. The same mechanism backs `chat.ts` and `workspaces/chat.ts`'s existing abort handling, but until this test no daemon test exercised it through a real socket. - Files: apps/atlasd/routes/mcp-registry.subprocess-kill.test.ts
biome's auto-formatter collapsed three single-arg object literals onto one line each. No behavior change. Caught by `deno task fmt` during team-lead end-of-session verification.
…ests `pgrep -P $$` diffing alone picks up *any* new child of the test runner, so two real-spawn tests running in the same parent worker could claim each other's PIDs. Add a `cmdlineFor(pid)` helper (`ps -o command=`, portable across macOS and Linux) and require new candidates to match the test's slow-server script before claiming the PID. Verified 10/10 sequential `deno task test` runs pass for both files. ## Progress - Task: #2 Tighten pgrep -P PID capture in subprocess-kill tests with a cmdline filter - Decisions: Used `ps -o command=` instead of /proc/<pid>/cmdline so the helper is identical on macOS and Linux. Filter substring is `SLOW_SERVER_SCRIPT` itself — already unique to each spawn and already imported at file scope, so no new constant needed. `cmdlineFor` returns "" on ESRCH so a racing exit just falls through to "not a match" without bubbling up an error. - Key Learnings: `ps -o command= -p <pid>` is a clean POSIX way to read a single pid's command line; portable cross-platform without resorting to /proc on Linux. Useful pattern any time a test needs to disambiguate a spawned child by cmdline. - Files: packages/mcp/src/create-mcp-tools.subprocess-kill.test.ts, apps/atlasd/routes/mcp-registry.subprocess-kill.test.ts
…MCPProcesses.acquire Locks in the HTTP-path safety invariant from issue #344's design: the daemon-scoped process registry intentionally outlives any individual probe, so the client's AbortSignal must stop at the `connectHttp` → `createMCPClient` boundary and never reach `sharedMCPProcesses.acquire`. The invariant was previously protected only by an inline comment in `create-mcp-tools.ts:845-851`; a refactor could leak the signal into `acquire` without breaking any test. Verified the test catches a deliberate signal-leak injection at the call site, then reverts cleanly. ## Progress - Task: #1 — Lock in HTTP-path safety: assert sharedMCPProcesses.acquire never receives the request signal - Decisions: Used the existing `create-mcp-tools-startup.test.ts` boundary (real `connectHttp` + mocked `@ai-sdk/mcp` + spied `sharedMCPProcesses.acquire`) instead of `apps/atlasd/routes/mcp-registry.test.ts` — that file mocks `@atlas/mcp` wholesale, so the real `acquire` call site is unobservable there. The startup test already exercises `connectHttp` with real `process-registry`, making it the natural boundary. - Key Learnings: `@atlas/mcp` only exposes `.` and `./testing` via its workspace exports — there's no subpath for `process-registry`, so `vi.mock("@atlas/mcp/process-registry", ...)` doesn't work. Spy directly on the imported `sharedMCPProcesses` singleton instead. - Files: packages/mcp/src/create-mcp-tools-startup.test.ts
…meout The "timer → SIGTERM" chain that closes #344 depends on `@ai-sdk/mcp`'s internal `StdioMCPTransport.close()` calling `abortController.abort()` on the controller passed to Node's `spawn()`. A future SDK version bump could regress this silently. The dependency was previously called out only in `attemptStdio`'s inline comment; this commit propagates it up to `connectServerWithTimeout`'s docstring with explicit pointers to the subprocess-kill tests as the tripwire. No behavior change. ## Progress - Task: #4 — Document AI SDK contract dependency in connectServerWithTimeout docstring - Decisions: Cite both subprocess-kill test files by full path so a future maintainer doing an `@ai-sdk/mcp` bump has a direct breadcrumb to the suites that gate the bump. - Key Learnings: None — pure documentation. - Files: packages/mcp/src/create-mcp-tools.ts
Existing abort-path tests assert vi.getTimerCount() === 0 for timer cleanup but not that the abort listener registered on the signal in withTimeout is removed when the signal wins the race. The cleanup code does both; this locks in the listener half symmetrically with the happy-path test.
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.
Summary
Closes #344. Mashing Retry 5–10× on a flaky MCP server used to silently wedge the daemon — each click spawned a fresh stdio MCP child while the previous one was still running, and after ~10 retries the daemon ran out of file descriptors and stopped accepting connections. PR #343 fixed the client-side stacking; this fixes the daemon-side leak that the client-side abort exposed.
Two compounding bugs:
GET /api/mcp-registry/:id/toolsdidn't observec.req.raw.signal. Client disconnect was ignored.withTimeoutwas a fire-and-forgetPromise.race— the inner spawn kept running until it eventually responded (or didn't). The MCP client object never had.close()called, so the spawned child never received SIGTERM. Worst-case orphan window: ~20s per probe.Fix:
c.req.raw.signalfrom the route throughprobeAndExtract→createMCPTools→connectServerWithTimeout→attemptStdio(and the parallel HTTP branch).withTimeoutsignal-aware: timer fires → internalAbortController.abort()→ same downstream listener fires → SIGTERM.attemptStdio, hoistStdioMCPTransportto a local and register the abort listener beforeawait createMCPClient(...). The AI SDK'screateMCPClientdoesn't accept a signal, so a server hanging at the MCPinitializehandshake (the exact Daemon MCP tools probe ignores client AbortSignal #344 failure mode) is otherwise uninterruptible.transport.close()propagates through the SDK's internalAbortController→ Node's spawnsignaloption → SIGTERM.ABORTEDsentinel + one-shot listener: client abort cancels the wait on the in-flight prewarm without canceling the prewarm itself (it keeps running for the next caller).connectHttp'ssharedMCPProcesses.acquiredeliberately does not receive the signal — that daemon-scoped registry is supposed to outlive any individual probe.Orphan window collapses from ~20s to <2s (SIGTERM → exit → kernel reap).
Test plan
packages/mcp/src/create-mcp-tools.subprocess-kill.test.ts— real spawn, real SIGTERM,process.kill(pid, 0)ESRCH within 2s of (a) external abort and (b)withTimeoutfiring through theAbortSignal.anyplumbing. PID captured viapgrep -P $$baseline-then-diff (Vitest's frozen ESM namespace makesvi.spyOn(child_process, "spawn")throw).apps/atlasd/routes/mcp-registry.subprocess-kill.test.ts— end-to-end: realDeno.serve, real NodefetchwithAbortController, same ESRCH assertion through the actual HTTP path. Verified deterministic with 10 sequential runs.apps/atlasd/routes/mcp-tool-cache.test.ts(new) —probeAndExtractsignal composition: pre-aborted, mid-flight abort, timeout fallthrough, happy path.apps/atlasd/routes/mcp-registry.test.ts— added two route-level race tests covering both prewarm-race winners (client abort wins / prewarm wins) to lock in the listener-cleanup invariant.packages/mcp/src/create-mcp-tools.test.ts— extended with 6withTimeouttests (pre-aborted, mid-flight abort, timer cleared on abort and on resolve, backward-compat timeout-only, listener removal on settle); updated one existing test's assertions to reflect new short-circuit behavior.deno checkclean; 354/354 tests pass across PR-touched files; lint warnings on PR-touched files are pre-existing (noNonNullAssertion,noTemplateCurlyInString).describe.skipIf(process.platform === "win32")— daemon doesn't run on Windows;process.kill(pid, 0)ESRCH semantics aren't portable.Notes for reviewers
/mcp/<flaky-server>/tools→ mash Retry 10× → watchpgrep -P <daemon-pid>go to zero within 2s of release, instead of accumulating 10 orphans for ~20s each.attemptStdioandwithTimeoutare now exported. Test-only exports — one keyword each, no signature changes for production callers.createMCPClientawait) was the subtle bug — the first attempt had it after, which never observes signals during the hung handshake. Pattern documented inline.