Repository navigation
fix(server): close StdioServerTransport when stdin ends or closes - #2494
Conversation
Per the MCP stdio binding, servers should exit promptly when their standard input is closed - stdin EOF is the primary graceful-shutdown signal, and on Windows (where no signal is delivered when the host goes away) the only reliable one. StdioServerTransport previously listened only for 'data' and 'error' on stdin, so when an MCP client hung up its end of the pipe (window closed, session restarted, host crashed) the transport never noticed: onclose never fired, nothing tore down, and server processes accumulated as zombies until killed by hand. The transport now attaches 'end' and 'close' listeners on stdin that close the transport (idempotently - onclose still fires exactly once if close() is also called), so Server/McpServer and serveStdio tear down through the existing onclose chain and a well-behaved server process exits naturally. The serverThatHangs fixture now swallows the sendLoggingMessage rejection its signal handler hits once the transport closes itself on stdin EOF; the fixture keeps hanging on purpose either way. Fixes #2002
🦋 Changeset detectedLatest commit: 5c84662 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…rocessMessage Review nit on #2494: with the transport now closing on stdin EOF, the two await tryServeListen(message) sites in processMessage were the only awaits not followed by the isTornDown() re-check, so a buffered final message racing stdin EOF on a modern-pinned connection dereferenced state.instance.channel after the state had been reassigned to closed, surfacing TypeError: Cannot read properties of undefined (reading 'channel') through options.onerror. Guard both sites like every other await in the function, and add a deterministic regression test (final 'data' chunk with 'end' on the next tick, emitted from a macrotask so the EOF handler beats the pump's microtask continuation, as it does with real stream events). Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gag9FFr6udcsF8qvMvMV8
There was a problem hiding this comment.
The tryServeListen race I flagged last run is fixed in f98dd15 (both isTornDown() re-checks + regression test) — thanks. Beyond the new inline EPIPE finding, this run also examined and ruled out two candidates: that the stdin-EOF close aborts in-flight requests before their responses are written, and that responses to requests received before EOF are silently dropped — verifiers refuted both (pre-EOF messages are fully delivered and answered; the existing unit test covers this).
Extended reasoning...
A new bug was found this run (post-close EPIPE crash via the removed stdout error listener), so approval is off the table and the inline comment carries that finding. This note only records progress and coverage: the previous run's serveStdio race was addressed in f98dd15 with the exact fix suggested plus a regression test, and two finder candidates about dropped/aborted in-flight responses at stdin EOF were adversarially examined and refuted, so they need not be re-explored.
close() removed the transport-level stdout 'error' listener, but a write accepted before close (write() returned true, per-send listener already detached) can still fail asynchronously at the OS level — e.g. EPIPE when the client that hung up also stops reading our stdout. With the new stdin-EOF close, that left stdout listener-less at exactly the moment a late EPIPE is most likely, turning it into an unhandled 'error' event that crashes the process with exit code 1 instead of the clean exit this change promises. Leave a swallow-only 'error' listener attached during close() so a post-close stream error is absorbed, and add a regression test that fails without the fix. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gag9FFr6udcsF8qvMvMV8
There was a problem hiding this comment.
Beyond the new inline findings, I confirmed that both issues flagged in my earlier review rounds are fixed at head 049610c: processMessage now re-checks isTornDown() after both await tryServeListen(message) sites (with a regression test for the buffered-final-message/EOF race), and close() now leaves a swallow-only stdout 'error' listener so a late EPIPE from a pre-close pending write can no longer crash the process (also covered by a new test).
Extended reasoning...
Bugs were found this run and are posted as inline comments, so this is not an approval — the response-drop finding on stdin EOF is substantive enough to warrant human judgment on drain-vs-immediate-close semantics. This note only records what was re-examined and ruled out: the two defects flagged in prior review passes (the serveStdio teardown race after tryServeListen, and the unhandled post-close EPIPE on stdout) were verified as fixed in the current diff at 049610c, each with accompanying regression tests, so those threads need not be re-explored.
…t lifecycles Review nit on #2494: the swallow-only stdout 'error' listener attached in close() was per-instance and never removed, and _stdout defaults to the process-global process.stdout — so repeated create->close transport cycles in one process piled up listeners (MaxListenersExceededWarning after 11 cycles). Drop the extra listener entirely: make _onstdouterror a no-op once the transport is closed and keep it attached through close(), so a late write failure (e.g. EPIPE from a write accepted before close) is still swallowed with no second listener needed. Tag the kept listener so the next transport to start on the same stream sweeps stale ones away — the listener count on a shared stream stays bounded at one per lifecycle instead of growing without limit. Add a regression test covering repeated cycles on shared streams. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gag9FFr6udcsF8qvMvMV8
start() registered stdin 'end'/'close' listeners but never checked whether the stream had already ended or been destroyed before start() ran. Node emits those events once, so a custom stream (e.g. a net.Socket the peer reset during async setup) handed the transport a dead stream that never fired onclose — a zombie connection. Now start() detects the already-dead stream via readableEnded/destroyed and defers _onstdinclose in a microtask. Also document in the stdio serving guide that stdin EOF tears the connection down automatically (no signal handler needed) and that servers holding their own keep-alive handles should release them on close, with a synced snippet showing the onclose-release pattern. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gag9FFr6udcsF8qvMvMV8
There was a problem hiding this comment.
Warning
This review may be incomplete: some analysis steps could not run due to a temporary API capacity limit.
The four follow-up commits check out — I verified each addresses its corresponding finding from the earlier rounds, and I found no new issues at the current head (f9d3eed). Deferring to a human because the half-close question from the earlier round is still open in the code: _onstdinclose still closes the transport immediately on stdin end, so the in-flight-response drain tradeoff flagged inline remains a design call for a maintainer.
What was reviewed on this revision:
- The
isTornDown()re-checks after bothawait tryServeListensites inserveStdio.ts, plus the new race test — correct and matching the function's existing convention. - The post-close stdout
'error'swallow (_onstdouterrorno-op once_closed, kept attached) and the symbol-tagged sweep instart()— checked the cross-instance edge (a still-open transport's untagged listener is never swept; a stale tagged listener left behind is a harmless no-op until the nextstart()). - The
readableEnded || destroyedpre-dead-stream check with its microtask deferral, and the two new unit tests covering ended-not-destroyed and destroyed-before-start shapes. - The docs/serving/stdio.md keep-alive release section against the actual
server.server.onclosebehavior — consistent.
Extended reasoning...
Overview
This PR makes StdioServerTransport close itself (firing onclose) when stdin ends or closes, so stdio servers stop lingering as zombies after a client hangup. It touches the transport (packages/server/src/server/stdio.ts), the serveStdio entry's message pump, docs, examples, unit tests, and an end-to-end integration test with a new fixture. Since the last review body, four commits landed (f98dd15, 049610c, a8b799d, f9d3eed), each addressing one finding from the earlier review rounds; I verified each fix in the current head and found no new issues in this run.
Security risks
None identified. The change is transport lifecycle logic on local stdio pipes — no auth, parsing of untrusted structure, or network surface is affected. The main risk class is availability (process crash / premature teardown), which is exactly what the earlier rounds probed: the unhandled-EPIPE crash is now fixed by the retained no-op error listener, and the listener-accumulation leak is fixed by the tagged sweep.
Level of scrutiny
High. This is a behavior change to the default lifecycle of every stdio MCP server built on the SDK — after this PR, every client hangup triggers transport close and Protocol._onclose, a path that previously only ran on explicit close() or stdout error. The earlier rounds showed this exposes real ordering hazards (nextTick 'end' vs. microtask continuations), and one flagged consequence remains unaddressed in code: closing immediately on stdin end aborts in-flight handlers and drops responses to requests that arrived before EOF, regressing the half-close batch idiom (cat requests.jsonl | server > responses.jsonl). Whether to drain pending responses before closing (the inline comment points at whenRequestsAnswered as an existing shape for this) versus closing promptly per the spec's SHOULD is a design tradeoff the maintainers should decide — it is not something I can rule correct either way, so approval is not appropriate even though the implementation of what the PR does attempt is now solid.
Other factors
Test coverage is strong: each prior finding gained a dedicated regression test (the serveStdio race test, the post-close EPIPE swallow test, the create→close cycle listener-count test, the two pre-dead-stream tests), plus the e2e zombie test that was verified to fail pre-fix. The docs gap flagged earlier is closed with the keep-alive release section synced from a typechecked example. The remaining open item is the unresolved inline 🔴 on the half-close/drain question, with no author response yet — that outstanding thread is the primary reason to keep a human in the loop.
claude
Bot
added
the
v2
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
packages/server/src/server/stdio.ts— nit, pre-existing: a final message that arrives without a trailing newline right before stdin EOF is silently discarded —_onstdinclose->close()clears the ReadBuffer at line 178 without draining the unterminated remainder, so a client that writes its last request with no\nand hangs up gets no answer (python-sdk'sasync for line in stdindoes deliver that last line). Fix: on stdin 'end', before closing, flush any non-empty buffered remainder through the same parse path (e.g. append '\n' and run processReadBuffer) so the trailing message is delivered like every earlier one; the base already drops it, so this only makes the new EOF path complete.Extended reasoning...
ReadBuffer.readMessage (core-internal/src/shared/stdio.ts:26-49) only parses up to a '\n'; bytes after the last newline stay in
_bufferand return null. With this PR, stdin 'end' now triggers_onstdinclose(stdio.ts:87-96) ->close(), which at line 178 callsthis._readBuffer.clear()and firesonclose— the pending partial line is thrown away rather than parsed. Trigger:printf '{"jsonrpc":"2.0","id":1,"method":"ping"}' | node server.js(no trailing newline, common in hand-written smoke tests and one-shot scripts), or any client whose final write omits the delimiter beforestdin.end(). Sequence: 'data' chunk buffered (no newline -> readMessage returns null), EOF 'end' -> close() -> buffer cleared -> request never reaches onmessage, no response, no error reported. On the base branch the message also sits unparsed forever (no 'end' handling at all), so this is pre-existing and not a regression; but the PR is the change that adds an EOF handler at exactly the point where a final flush belongs, and python-sdk's stdio_server iterates the text stream line by line and does…Verification: pre-existing — triggered when a client writes its final request with no trailing '\n' and then closes the pipe (e.g.
printf '{"jsonrpc":"2.0","id":1,"method":"ping"}' | node server.js). Mechanism verified:ReadBuffer.readMessage()(packages/core-internal/src/shared/stdio.ts:26-31) returns null whenever_buffer.indexOf('\n') === -1, leaving the unterminated remainder in…
For a stdin that is already ended or destroyed at start(), the synthetic hangup was queued as a microtask, which runs before the caller's `await connect()` continuation: an onclose assigned right after connect() never fired and keep-alive handles were not released. Use setImmediate so it is observable exactly like a real 'end'. Adds a regression test that assigns onclose after start(). Also declare an explicit return type on the stdio guide's notesServerWithKeepAlive() example, matching its sibling snippet files.
There was a problem hiding this comment.
I re-reviewed the latest two commits and found no new bugs; both of my earlier nits (the queueMicrotask → setImmediate ordering for a pre-dead stdin, and the missing return type on notesServerWithKeepAlive) are addressed. A maintainer look is still worthwhile because the one remaining open thread — aborting in-flight requests on stdin EOF — is now a documented, deliberate semantic (changeset, docs/serving/stdio.md, class JSDoc) rather than a fix, and that tradeoff should be consciously accepted by a human.
What was reviewed:
packages/server/src/server/stdio.ts:end/closelistener registration and removal, thereadableEnded || destroyedsynthetic hangup viasetImmediate, and the tagged swallow-only stdout'error'listener plus its sweep instart()— idempotentclose()still firesoncloseonce.packages/server/src/server/serveStdio.ts: both post-tryServeListenisTornDown()guards.- New unit tests (including the onclose-assigned-after-start regression test) and the synced docs snippet, which matches its
//#regionbyte-for-byte.
Extended reasoning...
Overview
The PR makes StdioServerTransport close itself on stdin end/close (and on a stdin that is already ended/destroyed at start(), deferred one event-loop turn via setImmediate), keeps a no-op stdout 'error' listener attached after close to absorb late EPIPE, and sweeps previously tagged listeners off the shared stream in start(). serveStdio.ts gains two isTornDown() re-checks after the async tryServeListen calls. Supporting changes: a patch changeset, a new "pipe hangup" paragraph plus keep-alive snippet in docs/serving/stdio.md (synced from examples/guides/serving/stdio.examples.ts), seven new unit tests, a new integration fixture and e2e case, and a fixture tweak to swallow a sendLoggingMessage rejection.
Since my last review, commits e4fddef and 8a76c9a landed: the former documents the in-flight-request semantics at EOF in the changeset, docs page, and class JSDoc; the latter switches the synthetic hangup from queueMicrotask to setImmediate (with a regression test assigning onclose after start()) and adds the explicit : void return type on the guide example. Both of my prior nits are resolved by the code.
Security risks
None identified. The change is confined to Node stream lifecycle handling in a Node-only subpath (./stdio); no auth, crypto, network input parsing, or new exports are involved. The added 'error' swallow is scoped to the post-close state and only suppresses events that would otherwise crash the process.
Level of scrutiny
Moderate-to-high: this is transport lifecycle code that went through six fix commits addressing lifecycle races (torn-down state in serveStdio, late EPIPE crash, listener accumulation, pre-dead streams, microtask ordering). The current code is coherent and each fix has a targeted unit test. The remaining open item is not a bug but a deliberate semantic — requests in flight at EOF are aborted and their responses dropped — which the author has now documented and justified against the spec's "SHOULD exit promptly" language and python-sdk's behavior. That is a maintainer-level product decision, which is why I deferred rather than approved.
Other factors
The docs fence in docs/serving/stdio.md matches the serveStdio_releaseKeepAlive region byte-for-byte, so pnpm sync:snippets --check should be clean. No new symbols are exported from any package index, and stdio.ts already lives at the Node-only subpath, so the runtime-neutral root barrel is unaffected. The bug hunt exited on a dry streak with no candidate findings, and I have nothing beyond the already-posted inline threads to add.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
4 participants
Requested by Felix Weinberger
Before / After
Before: when an MCP client hangs up its end of the stdio pipe — window closed, session restarted, host crashed — the server never notices.
StdioServerTransportonly listens fordataanderroron stdin, so stdin EOF is silently ignored:onclosenever fires, nothing tears down, and any server holding a keep-alive handle (timer, connection pool, watcher) lingers forever. In practice this means zombie MCP server processes accumulating with every client restart until someone kills them by hand.After: stdin reaching EOF (or being destroyed) closes the transport and fires
oncloseexactly once. That propagates through the existing close chain —Protocol._onclosefor hand-wiredServer/McpServer, andwire.oncloseteardown forserveStdio— so a well-behaved server releases its handles and the process exits naturally. This is what the MCP stdio binding asks for: servers "SHOULD exit promptly when their standard input is closed"; stdin EOF is the primary graceful-shutdown signal, and on Windows (where no signal is delivered when the parent goes away) the only reliable one.How
packages/server/src/server/stdio.ts— new_onstdinclosehandler (same arrow-function pattern as_onstdouterror) registered for stdinendandcloseinstart(), removed inclose(). It callsthis.close(), which is idempotent via the existing_closedguard, soonclosefires exactly once even whenendandcloseboth fire orclose()is also called explicitly. Class JSDoc documents the behavior.packages/server/test/server/stdio.test.ts— four new unit tests:onclosefires on stdinend(usingautoDestroy: false, emitClose: falseso theendlistener is proven independently ofclose);onclosefires on stdin destroy (close);onclosefires exactly once whenclose()follows stdin EOF; messages that arrived before EOF are still delivered.test/integration/test/processCleanup.test.ts+ new fixtureserverWithKeepAlive.ts— end-to-end regression test for the zombie itself: spawns a real SDK server child that holds a keep-alive interval and releases it inonclose, completes an initialize round-trip, closes the child's stdin (no signals), and asserts the process exits with code 0. Verified this test fails against the unfixed transport (child stays alive past the bound and has to be SIGKILLed) and passes with the fix.test/integration/test/__fixtures__/serverThatHangs.ts— the fixture's signal handler now swallows thesendLoggingMessagerejection it hits once the transport closes itself on stdin EOF (previously an unhandled rejection); the fixture intentionally keeps hanging either way, preserving what the signal-escalation test exercises..changeset/stdio-server-stdin-eof-close.md— patch changeset for@modelcontextprotocol/server.No opt-out flag. Considered and deliberately omitted: once stdin has ended no further input can ever arrive, so an open transport has nothing left to transport — there is no coherent use case for opting out, the spec words this as server behavior that SHOULD happen, and the community PR #2003 proposing the same two listeners drew no maintainer request for a flag. Servers that need to finish in-flight work can still do so in their
onclosehandler before letting the process drain; anything more exotic can pass a customReadable.Out of scope: the parent-PID watchdog for hosts that die without closing the pipe — that is PR #1712's lane and the broader remainder of #208.
Tests
pnpm --filter @modelcontextprotocol/server test— 450 passed (40 files)test/integrationprocessCleanup.test.ts— 4 passed (including the new e2e; confirmed it fails without the fix)pnpm --filter @modelcontextprotocol/server check(typecheck + eslint + prettier) — clean; touched integration files are eslint/prettier cleanFixes #2002. Also addresses the stdin-EOF part of #208 (self-termination when the host closes the pipe); the periodic parent-PID liveness check discussed there remains open and is intentionally not duplicated here. Credit to #2003, which proposed the same two-listener fix — this PR lands it against current
mainwith unit + end-to-end regression coverage and the fixture cleanup.