fix(server): exit stdio server when the client closes stdin - #61
Open
majkelooo wants to merge 2 commits into
Open
fix(server): exit stdio server when the client closes stdin#61majkelooo wants to merge 2 commits into
majkelooo wants to merge 2 commits into
Conversation
The MCP stdio lifecycle has the client shut the server down by closing our stdin, but nothing observed that. StdioServerTransport subscribes only to 'data' and 'error', so EOF raised no event, and the scheduler tick, the hooks rate-limit timer and IMAP IDLE sockets kept the event loop from draining. Any client death that skipped SIGTERM — closed terminal, SIGKILL, crash, MCP reconnect — left an immortal process reparented to launchd/init. 232 such orphans accumulated on one workstation in 7 days, holding 5.2 GB RSS and pushing the machine into 13.3 GB of swap. Shut down on stdin 'end'/'close' and on transport.onclose, add SIGHUP beside the existing signals, and make shutdown idempotent because those triggers routinely fire together. A grace timer forces the exit if a goodbye round trip hangs, so a stuck QUIT/LOGOUT cannot resurrect the orphan. Also guard the post-handshake block: it sits behind two awaits, so a client disconnecting mid-startup could arm the scheduler tick after shutdown had already run, leaving a ref'd handle nothing clears. The regression test spawns a real child process, since the defect is the event loop failing to drain and that is unobservable in-process. Refs codefuturist#60 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review of the previous commit surfaced a second await boundary. A watcher that finishes starting after shutdown has already run holds IMAP IDLE sockets the completed stop() never saw, so the loop stayed pinned and the grace timer forced exit 1 instead of a clean stop. With the watcher enabled that is not an edge case: reconnect storms hit exactly that window. stop() is idempotent, so the guard re-runs it and bails. Also replace the paraphrase of the shutdown contract with the specification's own wording, which is stronger than the paraphrase implied — the client SHOULD initiate shutdown by "first, closing the input stream to the child process (the server)", then by "waiting for the server to exit" before escalating to SIGTERM. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Description
Fixes #60 — the stdio server never exited when the client closed its stdin, so every client death that skipped
SIGTERM(closed terminal,SIGKILL, crash, MCP reconnect) left an immortal process reparented tolaunchd/init. I found 232 of them on one workstation after 7 days of uptime, holding 5.2 GB RSS and pushing the machine into 13.3 GB of swap.The spec is explicit about whose job this is. From
basic/lifecycle, Shutdown:"Waiting for the server to exit" is the part this package could not honour. Three things had to coincide, and all three did:
StdioServerTransportsubscribes to'data'and'error'only, so EOF on stdin raises no event andtransport.onclosenever fires in stdio mode.runServerlistened for it either —transport.onclosewas wired on the HTTP path only.This PR shuts down on stdin
'end'/'close'and ontransport.onclose, addsSIGHUPbeside the existing signals, and makesshutdownidempotent because those triggers routinely arrive together. A grace timer forces the exit if a goodbye round trip hangs, so a stuckQUIT/LOGOUTcannot resurrect the orphan.It also guards the post-handshake block, at both of its
awaitboundaries. That block sits behind twoawaits, so a client disconnecting mid-startup could arm the scheduler tick aftershutdownhad already run — a ref'd handle nothing clears. The new test caught this; without the guard it fails with exit code 1 from the grace timer instead of hanging. It is pre-existing (aSIGTERMduring startup did the same) but only becomes observable once the server can exit at all.A second review pass found the same shape one
awaitearlier, atwatcherService.start(): a watcher finishing its start aftershutdownhas run holds IMAP IDLE sockets the completedstop()never saw, so the loop stays pinned and the grace timer forces exit 1 rather than a clean stop. With the watcher enabled that is not an edge case — reconnect storms hit precisely that window.stop()is idempotent, so the guard re-runs it and bails.Reproduction
Minimal, no account needed — this isolates the mechanism:
printf 'x\n' | node a.mjsexits on its own;printf 'x\n' | node b.mjsnever does.For a control:
chrome-devtools-mcpruns through the samenpxlauncher and the same SDK on the same machine and left zero orphans over those 7 days. The launcher and the SDK are not the differentiator; the event-loop-pinning handle is.Decisions worth a second opinion
.unref(). Unref'ing the scheduler interval alone fixes nothing, becausehooks.service.ts(rateResetTimer) and IMAP IDLE sockets still pin the loop. Unref'ing all of them would touch three services and still leave the loop pinned by any handle added later.shutdown()already stops every one of them, so the missing piece was only ever the trigger. Happy to add.unref()as belt-and-braces if you prefer it.process.exitin the grace timer needs ann/no-process-exitdisable.process.exitCodeis the house style, but it only takes effect once the loop drains — and a hung shutdown is exactly the case where it will not. The reasoning is in a comment at the call site.'data'listener on stdin, deliberately: it would switch the stream to flowing mode and consume protocol bytes out from under the SDK.'end'/'close'leave the mode untouched.transport.oncloseis redundant today and wired anyway, so this becomes the idiomatic path if the SDK ever emits it. Worth knowing: Bug: StdioServerTransport doesn't handle stdin close/end — causes zombie process accumulation modelcontextprotocol/typescript-sdk#2002 reports the SDK-side gap and has had three competing PRs open since May with none merged. Fixing the SDK alone would not have fixed this package anyway, given point 2 above.Test
src/main.lifecycle.test.tsspawns the server as a real child process — the defect is the event loop failing to drain, which is unobservable in-process. It completes the handshake, closes stdin, and asserts a clean exit. Everything is confined to a temp dir viaXDG_*, so it cannot see a developer's real config or touch a real scheduled-mail queue, andafterEachkills the child — a test about leaked processes should not leak one.Verified red before / green after by stashing the
src/main.tschange:Type of change
Checklist
pnpm checkand it passespnpm typecheckand it passespnpm test: 151 passed / 16 filesOne behavioural consequence worth stating explicitly: a server spawned with
stdio: 'ignore'gets/dev/nullas stdin and therefore an immediate EOF, so it now exits at once. For a stdio transport that seems correct — such a server has no client and can never receive a request — but it is a change from today's silent hang.