Skip to content

fix(mcp): a connection completing after close() is never reaped — abort mid-connectAll orphans a stdio child #341

Description

@OGtwelve

Problem

McpClientManager.connectAll() and close() are not serialized, so an abort landing mid-connect can leave a live stdio child that nothing will ever reap.

connectAll appends its results after the await:

const outcomes = await Promise.allSettled(... this.connectOne(...) ...);
for (const outcome of outcomes) {
  if (outcome.status === 'fulfilled' && outcome.value) this.servers.push(outcome.value);
}

close() snapshots and clears:

const toClose = this.servers;
this.servers = [];
// ... close each, then this.processes.terminate()

Interleaved on abort:

  1. abort fires → the adapter's reaper drains its Disposerclose() runs. this.servers is still empty (nothing has been appended yet), so it closes nothing; terminate() kills whatever pids connectOne had already registered.
  2. connectAll() then resolves and pushes the fulfilled connections into this.servers.
  3. The disposer is already drained, so the finally finds no MCP disposer left. Those clients are never closed, and any pid registered after step 1's terminate() survives the run.

Where it bites

Both flows, identically — this is not specific to either adapter:

  • runLoopAdapter registers disposer.add(() => mcp.close()) before connectAll(), deliberately, with a comment noting that "a run aborted mid-connect can already have live children". That reasoning covers pids registered before the abort; it does not cover a connection that completes after it.
  • mergeFlowAdapter copies the same pattern (feat(mcp): give the merge-pr take-over flow an MCP surface #339).

Reported by CodeRabbit on #340 and verified against the source. Filed separately rather than folded into #339 because it is pre-existing, lives in mcp/mcp-client.ts rather than in either adapter, and changes shutdown behaviour for aitm start as much as for merge-pr — that deserves its own review and its own regression test.

What

Make the manager refuse to acquire after shutdown starts. Roughly: a closing flag set at the top of close(); connectAll's append loop closes-and-terminates any late arrival instead of pushing it. Both need to hold when close() is still in flight, not only after it resolves.

Worth checking while in there: connectOne registers its child pid at a point that may also be after terminate(), so the flag has to gate registration too, or the pid is tracked by a registry nobody will drain again.

Constraints (per CLAUDE.md)

Portable ESM, strict TS, paired *.test.ts.

Acceptance criteria

  • Aborting during a delayed connectAll leaves no open client and no tracked pid: a regression test with a createClient that resolves on a timer, aborted mid-flight, asserts the late client's close() ran.
  • close() remains idempotent and a normal connect → close cycle is unchanged.
  • bun test + node --test + biome + tsc green in both packages.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions