Skip to content

fix(server): close StdioServerTransport when stdin ends or closes - #2494

Open
claude[bot] wants to merge 9 commits into
mainfrom
claude/fix-2002-stdin-eof-close
Open

fix(server): close StdioServerTransport when stdin ends or closes#2494
claude[bot] wants to merge 9 commits into
mainfrom
claude/fix-2002-stdin-eof-close

Conversation

@claude

@claude claude Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

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. StdioServerTransport only listens for data and error on stdin, so stdin EOF is silently ignored: onclose never 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 onclose exactly once. That propagates through the existing close chain — Protocol._onclose for hand-wired Server/McpServer, and wire.onclose teardown for serveStdio — 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 _onstdinclose handler (same arrow-function pattern as _onstdouterror) registered for stdin end and close in start(), removed in close(). It calls this.close(), which is idempotent via the existing _closed guard, so onclose fires exactly once even when end and close both fire or close() is also called explicitly. Class JSDoc documents the behavior.
  • packages/server/test/server/stdio.test.ts — four new unit tests: onclose fires on stdin end (using autoDestroy: false, emitClose: false so the end listener is proven independently of close); onclose fires on stdin destroy (close); onclose fires exactly once when close() follows stdin EOF; messages that arrived before EOF are still delivered.
  • test/integration/test/processCleanup.test.ts + new fixture serverWithKeepAlive.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 in onclose, 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 the sendLoggingMessage rejection 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 onclose handler before letting the process drain; anything more exotic can pass a custom Readable.

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/integration processCleanup.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 clean

Fixes #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 main with unit + end-to-end regression coverage and the fixture cleanup.

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-bot

changeset-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 8a76c9a

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

This PR includes changesets to release 6 packages
Name Type
@modelcontextprotocol/server Patch
@modelcontextprotocol/client Patch
@modelcontextprotocol/codemod Patch
@modelcontextprotocol/core Patch
@modelcontextprotocol/server-legacy Patch
@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

pkg-pr-new Bot commented Jul 14, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

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

@modelcontextprotocol/codemod

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

@modelcontextprotocol/core

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

@modelcontextprotocol/server

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

@modelcontextprotocol/server-legacy

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

@modelcontextprotocol/express

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

@modelcontextprotocol/fastify

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

@modelcontextprotocol/hono

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

@modelcontextprotocol/node

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

commit: 8a76c9a

Comment thread packages/server/src/server/stdio.ts
…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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/server/src/server/stdio.ts Outdated
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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/server/src/server/stdio.ts Outdated
Comment on lines 73 to 88
// with no listener attached, that late 'error' event would crash the
// process with an uncaught exception.
};
_onstdinclose = () => {
// stdin reaching EOF (or being destroyed) means the client has hung up and no
// further input can ever arrive. The MCP stdio binding says servers should exit
// promptly when their standard input closes — this is the primary graceful
// shutdown signal, and on some platforms (e.g. Windows) the only reliable one.
// Close the transport so `onclose` fires and the process can exit naturally.
this.close().catch(() => {
// Ignore errors during close — nothing more can be read anyway
});
};

/**
* Starts listening for messages on `stdin`.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Closing the transport immediately on stdin end aborts every in-flight request handler (via Protocol._onclose) and flips _closed so their responses can never be written — but on a half-closed pipe the client is still reading stdout, so requests whose bytes arrived before EOF (the classic cat requests.jsonl | mcp-server > responses.jsonl batch idiom, or any client that pipelines its final request and half-closes) now silently lose their responses. This is a regression: an A/B run at PR head shows the final response is delivered with these two listeners detached and dropped with them attached, deterministically for any handler that awaits anything. Consider a bounded drain on end — stop reading, let requests delivered before EOF settle (the pending-request tracking in serveStdio's channel, whenRequestsAnswered, is exactly this shape), then close.

Extended reasoning...

The bug

stdin end/close now trigger _onstdinclosethis.close() synchronously (packages/server/src/server/stdio.ts:76-85, registered at :100-101). close() flips _closed — so every subsequent send() rejects with StdioServerTransport is closed — and fires onclose, which Protocol._onclose (packages/core-internal/src/shared/protocol.ts:825-854) turns into an abort of every in-flight request handler's AbortController with SdkError: Connection closed. A handler that completes after abort also returns without sending its response (the post-abort check around protocol.ts:1102-1105).

But stdin EOF only closes the read direction. On a half-closed pipe the client is still reading the server's stdout, which remains fully writable — and responses to requests whose bytes arrived before EOF are still owed. The PR hard-closes the write side the instant the read side ends.

Step-by-step proof

  1. A client pipelines its final request and half-closes — e.g. cat requests.jsonl | mcp-server > responses.jsonl, a one-shot smoke test, or any scripted client that writes tools/call (id 2), calls child.stdin.end(), and keeps reading stdout to EOF.
  2. Node emits 'data' with the final line; _ondataonmessageProtocol._onrequest starts the tool handler. The handler hits its first real await and suspends; its continuation is a promise microtask.
  3. The stream emits 'end' from the process.nextTick queue right behind that last 'data' — and nextTick drains before promise microtasks. So _onstdincloseclose() runs while the handler is still in flight. This ordering is deterministic, not a rare race: any handler that awaits anything (a timer, I/O, an LLM call — i.e. every real tool) loses.
  4. Protocol._onclose aborts the handler's signal (ConnectionClosed). Whether the handler bails on the abort or runs to completion, its response is never written: the post-abort guard suppresses it, and even without that, transport.send() now rejects because _closed is set.
  5. The client, reading stdout until EOF per normal half-close semantics, never receives the id:2 response for a request the server accepted.

This was verified empirically at PR head (049610c) with real spawned pipes and the real McpServer + StdioServerTransport: with the PR's listeners attached, a pipelined tools/call whose handler awaits 150ms observes ctx.mcpReq.signal.aborted === true and its response never reaches the parent; a control run identical except with the two 'end'/'close' registrations detached (pre-PR behavior) delivers the response. So this is a regression opened by this PR for a pattern that previously worked, not a pre-existing limitation.

Why the PR's own reasoning and test don't cover this

  • The PR's no-opt-out rationale — 'once stdin has ended an open transport has nothing left to transport' — overlooks the write direction: it still has the responses to already-delivered requests to transport. The suggested escape hatch ('servers can finish in-flight work in their onclose handler') cannot work for responses, because by the time user onclose code runs, send() already rejects and the handlers have already been aborted.
  • The unit test 'should still deliver messages that arrived before stdin ended' asserts delivery to onmessage only — the response round-trip is exactly the part that breaks. Arguably the current behavior is the worst combination: the pre-EOF request's side effects run (or start running), while its response is guaranteed to be dropped.
  • Notably, this PR itself holds the opposite invariant one layer up: serveStdio's probe-discard path is documented as 'a probe request the entry accepted must never go silently unanswered' and implements bounded drain machinery for it (StdioConnectionChannel._pendingRequests / whenRequestsAnswered / DISCARD_ANSWER_TIMEOUT_MS in packages/server/src/server/serveStdio.ts). The transport-level EOF close violates that same invariant for every pre-EOF request.

Addressing the refutation (spec + cross-SDK)

One verifier argued this is intentional, spec-anchored behavior. Two responses:

  • Spec: the stdio binding says servers 'SHOULD exit promptly when their standard input is closed', and the lifecycle doc has the client initiate shutdown by closing stdin. Neither licenses discarding responses to requests the server already accepted — a bounded drain-then-exit (flush answers to pre-EOF requests, then close) is equally 'prompt' and is the universal Unix filter/half-close convention. The shutdown sequence ('close stdin, then wait for the server to exit') is in fact more compatible with drain-then-exit than with abort-and-drop.
  • python-sdk parity: the refutation claims python-sdk also cancels in-flight handlers on stdin EOF; another verifier traced the opposite (anyio task group waits for in-flight handler tasks while the stdout writer stays open). The two readings conflict and I could not independently resolve them here, so I don't rest the finding on parity in either direction. What is not in dispute is the TS-side A/B result: within this SDK, this PR converts a working response-delivery path into a silent drop.

Impact and fix

Any batch/one-shot stdio usage — shell pipelines, CI smoke tests, scripted clients that pipeline their last request before half-closing — silently loses the final response(s) while the request's side effects still execute. The failure is silent on both sides: the server sees only an aborted signal and a swallowed send() rejection; the client just sees stdout EOF without the answer.

Fix shape: on stdin 'end', stop reading and begin a graceful drain instead of an immediate hard close — defer close() (and hence onclose/handler abort) until requests delivered before EOF have been answered, bounded by a timeout as a backstop, mirroring what whenRequestsAnswered already does in this PR's serveStdio channel. Alternatively, at minimum document that half-closed batch usage is unsupported — but given the pattern worked before this PR, draining is the better outcome.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping immediate close. Stdin EOF means the client has hung up: the spec's shutdown sequence closes stdin and then only waits for exit, and python-sdk's dispatcher cancels in-flight handlers at EOF for the same reason (jsonrpc_dispatcher.py, run(), the finally cancelling the task group), which settles the parity question above. The batch idiom of piping a request file and half-closing was never a specified client flow. The semantics are now stated in the changeset, the stdio guide, and the class JSDoc.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to the parity claim above. After running both SDKs on the same pipeline (initialize, one in-flight tools/call, close stdin, read stdout to EOF): python-sdk main cancels the handler at EOF but answers it with {"error":{"code":-32000,"message":"Connection closed"}} before exiting (_handle_request, cancelled-scope branch; CONNECTION_CLOSED = -32000 in mcp_types/jsonrpc.py). This PR currently writes nothing for it. So the mechanism matches (cancel, no drain, prompt exit) but the wire outcome does not. We'll align in this PR: requests still in flight at close will be answered with a -32000 Connection closed error before the transport closes.

Comment thread packages/server/src/server/stdio.ts Outdated
…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
Comment thread packages/server/src/server/stdio.ts
Comment thread packages/server/src/server/stdio.ts
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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 both await tryServeListen sites in serveStdio.ts, plus the new race test — correct and matching the function's existing convention.
  • The post-close stdout 'error' swallow (_onstdouterror no-op once _closed, kept attached) and the symbol-tagged sweep in start() — 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 next start()).
  • The readableEnded || destroyed pre-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.onclose behavior — 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 claude Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 \n and hangs up gets no answer (python-sdk's async for line in stdin does 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 _buffer and return null. With this PR, stdin 'end' now triggers _onstdinclose (stdio.ts:87-96) -> close(), which at line 178 calls this._readBuffer.clear() and fires onclose — 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 before stdin.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…

Comment thread packages/server/src/server/stdio.ts Outdated
Comment thread examples/guides/serving/stdio.examples.ts Outdated
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.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed the latest two commits and found no new bugs; both of my earlier nits (the queueMicrotasksetImmediate 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/close listener registration and removal, the readableEnded || destroyed synthetic hangup via setImmediate, and the tagged swallow-only stdout 'error' listener plus its sweep in start() — idempotent close() still fires onclose once.
  • packages/server/src/server/serveStdio.ts: both post-tryServeListen isTornDown() guards.
  • New unit tests (including the onclose-assigned-after-start regression test) and the synced docs snippet, which matches its //#region byte-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.

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

Labels

v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: StdioServerTransport doesn't handle stdin close/end — causes zombie process accumulation

3 participants