feat(client/stdio): opt-in process-tree teardown on close()

[siddheshbandgar]

Fixes #2023.

Problem

StdioClientTransport.close() signals only the direct child. When a server is launched through a wrapper — npx, uvx, python -m — the wrapper dies and the real server is orphaned. On Windows ChildProcess.kill() cannot terminate a tree at all.

Change

Adds an opt-in killProcessTree option to StdioServerParameters (Options A/B from the issue, combined):

• POSIX: the child is spawned with detached: true so it leads its own process group, and close() signals -pid, reaching every descendant.
• Windows: teardown goes through taskkill /pid <pid> /T /F.
• Both paths fall back to the existing proc.kill(signal) if the group/taskkill call fails.

Default is false, so signal propagation and Ctrl-C behaviour are unchanged for anyone who doesn't opt in — this is deliberate, since detached: true by default would turn a crashed host into a permanent orphan factory (the trade-off called out as Option B in the issue).

Test

stdioKillProcessTree.test.ts spawns a wrapper that forks a long-lived grandchild, records its pid, closes the transport, and asserts the grandchild is reaped. Skipped on Windows in CI; the taskkill path was verified manually.

Happy to change the default, or move to Option C (a consumer-supplied cleanup hook), if you'd prefer a different direction.

A changeset is included (client: minor).

[changeset-bot]

🦋 Changeset detected

Latest commit: 8adc629

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@modelcontextprotocol/client	Minor
@modelcontextprotocol/codemod	Minor
@modelcontextprotocol/core	Minor
@modelcontextprotocol/server-legacy	Minor
@modelcontextprotocol/server	Minor
@modelcontextprotocol/core-internal	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2596

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2596

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2596

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2596

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2596

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2596

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2596

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2596

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2596

commit: 8246333

[siddheshbandgar]

Hi maintainers — friendly bump on this one. 👋

This PR addresses #2023 (orphaned server processes when StdioClientTransport.close() is called, especially via npx/uvx wrappers). It's opt-in (killProcessTree, default false) with a test and changeset included, so existing behavior stays unchanged.

No pressure — but if you'd prefer a different direction (e.g. a consumer-supplied cleanup hook, different defaults/naming), happy to iterate. Would appreciate a review whenever you get a chance. Thanks!

[ettalin]

This is the approach that came out best when I measured the alternatives, so
this is a supporting note rather than an objection — with one residual worth
getting into the thread so it doesn't resurface later as a bug report against
the option.

I ran a matrix over spawn shape × cleanup strategy × how the parent process
dies. For the wrapper case this PR targets:

spawned as	teardown	clean exit	SIGTERM	crash	SIGKILL
node → npx shim → server	child.kill() (today)	orphaned	orphaned	orphaned	orphaned
node → npx shim → server	detached + kill(-pid)	clean	clean	clean	orphaned

The first row is #2023: the shim exits, the real server reparents, and the
handle you hold refers to something already dead — so the kill succeeds and
nothing happens. This PR turns that into the second row, which fixes the case
the issue actually reports.

The residual is the last column. close() has to be called, and under
SIGKILL nothing calls it — no handler runs, so process-tree teardown never
executes. For an SDK that is arguably correct scope; a transport can't be
responsible for its host being killed. But killProcessTree reads like a
lifetime guarantee and is in fact a teardown path, so it may be worth one line
in the option's docs. Otherwise someone will eventually file "killProcessTree
doesn't kill the process tree" against a SIGKILLed host, and it'll look like a
regression rather than known scope.

One smaller thing from the same runs, on the documentation rather than the code:
sh -c "<single command>" does not need this option. The shell execs, so
the direct child is the server and child.kill() already reaches it —
measured clean in every death mode except SIGKILL. npx and uvx do need it,
because the shim stays in the chain and then exits. Those two look like the same
"wrapper" case and behave differently, so if the option gets documented by
example it's worth picking npx, not sh -c.

All of the above is macOS / Node 24; I haven't run it on Linux or Windows, and
the Windows path here (taskkill /T /F) is the one I'd least want to guess about.

Harness is public if it's useful for the PR's tests — it includes no-cleanup
control scenarios that must always orphan, so a broken detector aborts the run
rather than silently reporting everything clean:
https://github.com/ettalin/subreap

[siddheshbandgar]

Thanks for the detailed matrix — really useful, and I appreciate you sharing the subreap harness. Your read matches my intent: this is a teardown path, not a lifetime guarantee.

Agreed on the SIGKILL residual. I'll add a line to the killProcessTree option docs clarifying that it only runs when close() is actually invoked, and that a SIGKILLed host can't trigger it — so it's not mistaken for a regression later.

Also good catch on the sh -c vs npx/uvx distinction. I'll update the doc example to use npx (where the shim stays in the chain) rather than sh -c, since that's the case that actually needs the option.

I'll push both doc tweaks to the PR. Thanks again for the thorough review.

[ettalin]

Read through the implementation properly this time rather than just the option
docs. Three things, one of which I was able to measure.

The short version: the POSIX path does what it says, but detached: true has a
second effect that works against the option in one common path, and the Windows
branch has an error-handling gap that could take the host process down.

None of this is a blocker on an opt-in flag. Details inline.

[ettalin]

detached: true does two things on POSIX, and only one of them is wanted here.
It creates the process group close() later signals — that's the intent. It
also calls setsid(), which moves the child out of the terminal's foreground
process group.

That second effect reverses the option's intent when the host dies from a
terminal signal without reaching close(). Measured on macOS / Node 24, host
with no SIGINT handler, SIGINT sent to the foreground process group:

spawn	after Ctrl+C
detached: false (today)	host dead, child died
detached: true (killProcessTree)	host dead, child survived as an orphan

Today the child shares the host's process group, so the terminal's SIGINT
reaches both and nothing is orphaned. With the option enabled the child no
longer receives it, so anything that kills the host without running close()
now leaks the very process the option exists to reap.

Hosts that trap SIGINT and await close() are unaffected — which is probably
most of them. But it is a real trade rather than a pure win, and it belongs
next to the SIGKILL note you just added, since it is the same class of caveat:
the option only helps when close() actually runs, and enabling it makes the
paths where close() doesn't run slightly worse than the status quo.

[ettalin]

The signal parameter is unused on this branch, so both phases of close()
are the same call. close() deliberately escalates — stdin.end(), 2s, then
SIGTERM, 2s, then SIGKILL — but on Windows the SIGTERM phase already runs
taskkill /T /F, which is the forceful one.

The effect is that killProcessTree: true removes graceful shutdown on Windows:
a server that would have flushed state or released a lock on SIGTERM gets
terminated instead, and the 2s grace window is spent waiting on a tree that was
already force-killed.

/T without /F for the SIGTERM phase and /T /F only for SIGKILL would keep
the escalation intact:

const args = signal === 'SIGKILL'
    ? ['/pid', String(pid), '/T', '/F']
    : ['/pid', String(pid), '/T'];

Worth a second opinion from someone who runs this on Windows — /T alone
declines rather than force-terminates when a child has no window to close, so
the SIGKILL phase still has to do the real work.

[ettalin]

Summary of the review above, since the inline comments sit in Files changed and are easy to miss:

1. detached: true also calls setsid() (line 157) — which moves the child out of the terminal's foreground group. Measured on macOS/Node 24: with a host that has no SIGINT handler, Ctrl+C kills the child today but leaves it orphaned once the option is on. Worth a line next to the SIGKILL caveat, since it's the same shape — the option helps when close() runs, and slightly hurts when it doesn't.
2. The taskkill spawn has no 'error' listener (line 238) — launch failures arrive asynchronously, so the catch never fires, the documented fall-through never runs, and an unhandled 'error' event can take the host process down.
3. signal is unused on the win32 branch (line 236) — both phases run taskkill /T /F, so close()'s graceful step isn't graceful on Windows.

Only (1) is measured. (2) and (3) are read-not-run — I have no Windows machine. Worth noting the 14 green checks don't cover them either: every job in main.yml is ubuntu-latest.

None of it blocks an opt-in flag.