fix(client): recover from connection close during the version-negotiation probe#2514
fix(client): recover from connection close during the version-negotiation probe#2514felixweinberger wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: bc67606 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}
c8698d1 to
f011bd6
Compare
There was a problem hiding this comment.
Beyond the inline findings, one additional candidate was examined and ruled out this run: a caller-side transport.close() landing in the window after the stdio close verdict resolves but before the respawn. At that point the child has already exited, so StdioClientTransport.close() is a no-op (_process is already undefined) — the respawn is the deliberate legacy fallback, not a resurrection of a caller-terminated process; the caller-close-during-probe case itself is covered by the local-close marker and its test.
Extended reasoning...
This run's inline comments cover the new findings on commit f011bd6 (which addressed the prior round: end:false piping, raw-value close restoration, stale-close guard, migration-doc entries). The only extra item examined and refuted was the post-verdict/pre-respawn local-close race in versionNegotiation.ts: once the stdio child has exited on the probe, a caller close() in that gap has no process to kill and no state to corrupt, and the genuine caller-close-mid-probe path already yields the typed EraProbeConnectionClosed with no respawn. Recording it here so a later review pass need not re-derive it; this is informational, not a correctness guarantee.
| }); | ||
|
|
||
| if (this._stderrStream && this._process.stderr) { | ||
| this._process.stderr.pipe(this._stderrStream); | ||
| if (this._stderrStream && proc.stderr) { | ||
| // end: false — the shared PassThrough must survive a child exit | ||
| // so a restarted transport can pipe the next child's stderr into | ||
| // it (writing to an ended stream raises ERR_STREAM_WRITE_AFTER_END). | ||
| proc.stderr.pipe(this._stderrStream, { end: false }); |
There was a problem hiding this comment.
🔴 The { end: false } pipe change means the stderr PassThrough is now never ended on any code path — not on child exit and not even on final transport close(), which never touches _stderrStream — so every stderr: 'pipe' consumer (including plain mode: 'legacy' clients that never opt into version negotiation) loses the end-of-stream signal: code awaiting 'end'/stream.finished(), for-await-of loops, and .pipe(dest) log destinations now hang or never close where pre-PR they terminated on child exit. At minimum close() (deliberate final teardown — the SDK's own restart flow never goes through it) should end/destroy _stderrStream, and this consumer-facing behavior change on the default path needs a docs/migration entry (it is currently documented only in the changeset and JSDoc).
Extended reasoning...
What changed. Pre-PR, start() piped the child's stderr into the shared PassThrough with the Node default end: true: when the child exited, its stderr readable emitted 'end' and pipe() ended the PassThrough. Consumers holding transport.stderr saw 'end' fire, for await loops terminated, stream.finished() resolved, and transport.stderr.pipe(dest) closed dest (flushing log files). Post-PR, the pipe is unconditionally { end: false } (stdio.ts:189-195) — necessary for the restart path, since piping a respawned child into an already-ended PassThrough raises ERR_STREAM_WRITE_AFTER_END — but nothing was added to end the stream anywhere else. close() (stdio.ts:246-283) ends stdin, escalates SIGTERM→SIGKILL, and clears _readBuffer, but never touches _stderrStream; the field appears only in the constructor, start(), and the stderr getter. So after this PR there is no code path that ever ends the stream, including deliberate final teardown via client.close().\n\nWho is affected. The { end: false } pipe is unconditional, so this hits every stderr: 'pipe' consumer — including plain mode: 'legacy' clients that never touch version negotiation. That contradicts the PR description's "Breaking Changes: None / mode: 'legacy' (the default) is byte-untouched" — true of wire bytes, but not of the transport's observable stream semantics on the default path, shipped as a patch release.\n\nStep-by-step proof. (1) new StdioClientTransport({ command, stderr: 'pipe' }) — no versionNegotiation option at all. (2) Consumer collects a crashed server's stderr the natural way: const chunks = []; transport.stderr.on('data', c => chunks.push(c)); transport.stderr.on('end', () => report(chunks)); — or equivalently for await (const c of transport.stderr), or transport.stderr.pipe(fs.createWriteStream(log)). (3) The server crashes, or the session ends and the caller runs await client.close(). (4) Pre-PR: child exit → pipe() (default end: true) ends the PassThrough → 'end' fires, the loop terminates, the log file is closed. (5) Post-PR: the child exits and close() returns, but the PassThrough is still open — 'end' never fires, the for await never returns, the write stream is never closed. The consumer hangs indefinitely.\n\nWhy the documented substitute doesn't work. The new JSDoc says to "key teardown on onclose instead", but onclose is not a stream-level signal — for await, stream.finished(), and .pipe(dest) cannot consume it — and in auto mode it is not even a reliable end signal: the PR's own test asserts presetCloses === 1 mid-connect() when the probe-killed child exits, while the stream goes on carrying the respawned child's output. So there is no reliable end-of-stream signal left at all.\n\nHow to fix. End (or destroy) _stderrStream in close(): a local close() is the caller-initiated, terminal teardown — the SDK's own restart flow never goes through it (the child exits on its own during the probe, and the classifier treats a local mid-probe close as a typed error with no respawn), so ending the stream there cannot break the restart feature. One fix-shape caveat: a caller who manually does close() → connect() again would then hit write-after-end on the reused PassThrough, so the robust variant is to also recreate the PassThrough per start() when the previous one was ended (or expose a fresh one via the stderr getter). Either way, final teardown regains its 'end' signal.\n\nDocs gap. This consumer-facing behavior change on the default path is documented only in the changeset and JSDoc. Per the repo's own convention (CLAUDE.md § Breaking Changes; REVIEW.md checklist: behavior changes need a docs/migration/ entry), it needs an entry in docs/migration/upgrade-to-v2.md — it affects legacy-mode v1-style usage, not only the 2026 negotiation feature. A grep of docs/migration/ finds no mention of the stderr end-semantics change.
There was a problem hiding this comment.
The code half of this is now addressed well: close() ends _stderrStream exactly once (with an unpipe guard against a SIGKILLed child's late flush), start() recreates the PassThrough when the previous life's stream was ended by close() — so a manual close() → connect() cannot hit write-after-end — and the new stdioRestart.test.ts "stderr: 'pipe' contract" block pins all of it. Final teardown regains its 'end' signal.
One item from the original comment is still open: the docs/migration entry. The end-of-stream semantics on the default stderr: 'pipe' path did change for every consumer (including plain mode: 'legacy' clients): pre-PR the stream ended on child exit; now it ends only on close(), so code keying on 'end'/stream.finished()/.pipe(dest) to detect a crashed server must now go through transport.close() (e.g. from onclose) to get the signal. That behavior change is currently documented only in the changeset and the stderr getter JSDoc — a grep of docs/migration/ finds no mention (the only StdioClientTransport entries in upgrade-to-v2.md are the import-path move, maxBufferSize, and windowsHide). Per CLAUDE.md § Breaking Changes, a short note in the stdio-transport section of docs/migration/upgrade-to-v2.md — what changed, why (the restart seam), and the onclose → close() migration for crash-detection consumers — would close this out.
| /** | ||
| * Detach for the stdio close-fallback: the probe consumed the connection | ||
| * (the child exited), so the legacy handshake will run on a restarted | ||
| * transport, with `start` left live so `Protocol.connect()`'s | ||
| * unconditional `start()` respawns the process. The close-delivery guard | ||
| * is reset before detaching: the probe-phase close was delivered and is | ||
| * spent — the restarted process's eventual close is a new event and must | ||
| * reach the restored handler. | ||
| */ | ||
| releaseForRestart(): void { | ||
| this._closeDelivered = false; | ||
| this.detach(); | ||
| } | ||
|
|
||
| /** Detach the handlers and arm the one-shot `start()` pass-through for the `Protocol.connect()` handover. */ |
There was a problem hiding this comment.
🟡 release() restores transport.start as a fresh bound copy (transport.start.bind(transport)) rather than the pre-connect property value, so after every successful probing connect transport.start loses its original identity — and each reconnect on the same transport re-binds the previous copy, accreting one wrapper layer per connect. This is the exact pattern this PR fixes for close (raw property value saved in _originalClose, restored by identity, with a test pinning it); mirror that fix here: save the raw property value for restoration and use the bound copy only for the wrapper'''s internal call.
Extended reasoning...
What the bug is. ProbeWindow.release() (packages/client/src/client/versionNegotiation.ts:311-324) arms the one-shot start() pass-through like this:
release(): void {
this.detach();
const transport = this._transport;
const originalStart = transport.start.bind(transport); // fresh function object
let armed = true;
transport.start = async (): Promise<void> => {
if (armed) {
armed = false;
transport.start = originalStart; // restores the BOUND COPY
return;
}
return originalStart();
};
}Function.prototype.bind returns a new function object, so when Protocol.connect()'''s unconditional start() consumes the one-shot, the restoration installs a bound copy as an own property — not the pre-connect value of transport.start.
The code path. release() runs on every SUCCESSFUL probing connect (mode: '\''auto'\'' or a pin against a server that answers — the common path; only the new releaseForRestart() and the failure path skip it). Reusing a transport across connects is a supported pattern — the era-scope-discipline tests in the same file reconnect the same client — and each subsequent connect'''s release() computes bind() of the previous bound copy.
Step-by-step proof. (1) Caller creates a transport; transport.start is the prototype method S0. (2) First auto connect against a modern or legacy-answering server: release() computes B1 = S0.bind(transport) and arms the pass-through; Protocol.connect() calls start(), the one-shot fires, and transport.start = B1. Identity check: transport.start === S0 is now false, permanently. (3) Second connect on the same transport: release() computes B2 = B1.bind(transport); after the handover, transport.start = B2 — a call now chains B2 → B1 → S0. (4) After N successful probing connects, transport.start is an N-deep chain of bound copies, growing without bound in a long-lived reconnect loop.
Why it'''s inconsistent with the PR'''s own contract. This PR fixes exactly this flaw for the sibling seam in the same class: open() now saves the raw property value (window._originalClose = transport.close) and calls through a separate bound copy, detach() restores by identity, and the new test in versionNegotiation.test.ts asserts expect(transport.close).toBe(originalClose) after a successful connect with the comment '''close() is restored by identity, so repeat connects never stack wrappers'''. The renamed pin-mode test ('''…start() and close() untouched … identity restored''') pins start identity only on the failed path — where release() never runs. The parallel assertion expect(transport.start).toBe(originalStart) after a successful connect would fail today. This matches the repo'''s Completeness convention: when a PR replaces a pattern, surviving sibling instances of the old form should be migrated too.
Impact. Functionally harmless — bind preserves this, so behavior through the chain is identical, and the wrapper self-replaces so there is no double-arm. The cost is (a) a caller that captured const original = transport.start before connect (a test spy, instrumentation detection) sees a different function afterward, and (b) one cheap closure accreted per reconnect, unbounded over a long-lived reconnect loop. Same class of impact as the close finding this PR fixed.
How to fix. Mirror the close fix in two lines: save the raw property value for the restoration and keep the bound copy only for the wrapper'''s internal call:
const savedStart = transport.start; // raw property value — restore this
const boundStart = transport.start.bind(transport); // internal call only
let armed = true;
transport.start = async (): Promise<void> => {
if (armed) {
armed = false;
transport.start = savedStart;
return;
}
return boundStart();
};Note: the release() body itself predates this PR unchanged — but the PR establishes and tests the identity-restoration contract for this exact seam pair in this module, which is what makes the surviving start sibling worth fixing here rather than leaving it inconsistent with the module'''s newly documented guarantee.
…robe A stdio server that exits on the unrecognized server/discover probe (the rmcp shape: terminate on any pre-initialize request) connected fine under mode 'legacy' but hard-failed under mode 'auto'. Classify that close as a legacy signal, symmetric with the stdio probe-timeout rule. StdioClientTransport owns the restart: an internal capability respawns the server after the child died, with generation-scoped child event handlers, the read buffer cleared on child exit, and refusal after a local close(); with stderr 'pipe' the aggregate stream spans restarts and ends exactly once, in close(). The probe layer checks for the capability and asks the transport to restart before running the plain initialize fallback; a transport without it, or whose restart refuses or fails, rejects with the existing typed negotiation error. On HTTP a mid-probe close keeps surfacing as the same typed connect error as any probe transport failure.
f011bd6 to
bc67606
Compare
There was a problem hiding this comment.
Beyond the inline nits, two stderr-contract concerns from the earlier review rounds were re-examined against this revision and ruled out: the "'end' never fires" hang is resolved (close() now ends the aggregate PassThrough exactly once, unpiping first — test-pinned in stdioRestart.test.ts), and stream recreation across restart lives is correctly handled for the SDK's own TRANSPORT_RESTART flow (the swap in start() only triggers after a local close(), which that flow never takes). Only the narrower manual close()→start() pre-start attach window survived as a finding.
Extended reasoning...
This revision substantively reworked the areas my prior runs flagged: child event handlers are now generation-scoped, the stderr pipe is { end: false } with a single close()-time end(), negotiation no longer wraps transport.close() (identity pinned by test), and the HTTP mid-probe close reverted to the generic EraNegotiationFailed with docs/changeset aligned. Verifiers this run specifically re-checked the two stderr regressions the earlier round raised — end-of-stream signaling on the default legacy path, and cross-restart stream recreation — and refuted both against the shipped code; the surviving stderr issue is only the documented attach-before-start guarantee in the manual close()-then-start() window, which is posted inline as a nit.
| resolve(); | ||
| }); | ||
|
|
||
| this._process.on('close', _code => { | ||
| proc.on('close', _code => { | ||
| if (this._generation !== generation) { | ||
| return; | ||
| } | ||
| this._process = undefined; | ||
| // Drop any partial trailing line so a restarted transport starts with a clean stream. | ||
| this._readBuffer.clear(); | ||
| this.onclose?.(); | ||
| }); |
There was a problem hiding this comment.
🟡 In the window between close() resolving and the next start(), a SIGKILLed child's late stdout flush still passes the generation guard (close() never bumps _generation, and the SIGKILL path returns without awaiting the child's 'close' event) and repopulates the shared _readBuffer; since start() bumps _generation without clearing the buffer, a stale trailing partial line survives into the new life while the old child's now-stale 'close' handler is generation-blocked from running the compensating clear. The restarted child's first JSON-RPC message then concatenates onto the stale partial and is silently skipped by the framing layer (e.g. a hung initialize). One-line fix: clear _readBuffer at the top of start() (symmetric with the _stderrStream recreation in the same block), or bump _generation in close().
Extended reasoning...
What the bug is. The generation-scoping this PR adds covers events landing after the next start(), but not the window between close() resolving and that start(). close() clears _readBuffer at its end but never bumps _generation, and its SIGKILL branch returns without awaiting closePromise a third time — so against a child that ignores stdin-EOF and SIGTERM, close() resolves while the dead child's pipe-buffered stdout is still unflushed. When that flush lands, the stdout 'data' handler's guard (this._generation !== generation) passes — no new start() has run, so the generation still matches — and the dead child's bytes are appended to the just-cleared shared buffer (complete lines are even delivered via onmessage after close() resolved); a trailing partial line stays.
Why the new guard makes the residue sticky. start() bumps _generation but does not clear _readBuffer — asymmetric with the _stderrStream recreation the same block performs for the new life. When the old child's 'close' event finally fires after the restart, the new stale-generation guard (packages/client/src/client/stdio.ts:169-171) returns early, deliberately skipping the _readBuffer.clear() at line 174 that would otherwise have wiped the stale partial. Pre-PR, that unguarded handler would have performed the compensating clear (at the cost of other, worse mutations); post-PR nothing does.
Impact. The restarted child's first stdout chunk is appended after the stale partial line, so its first JSON-RPC message parses as <stale-partial><new-json> — invalid JSON. ReadBuffer.readMessage() consumes the line and continues on SyntaxError (packages/core-internal/src/shared/stdio.ts:42-44, the deliberate skip for non-JSON debug output), so the corrupted line is silently dropped: no onerror, no teardown — just the restarted session's first message (e.g. the initialize response) vanishing, hanging the reconnect until timeout.
Step-by-step proof.
start()spawns child A — a server that ignores stdin-EOF and SIGTERM and has an unterminated stdout line buffered in the kernel pipe.await transport.close(): both 2s races elapse, SIGKILL is sent,close()clears_readBufferand returns — child A's stdout flush and 'close' event are still pending._generationis unchanged.- Node flushes A's dying stdout. The 'data' handler's generation check passes (still the same generation), so A's bytes — including the trailing partial line — land in the shared
_readBuffer. - The caller restarts (the manual
close()-then-start()contract the newstart()JSDoc documents and the restart-provenance test pins as 'a fresh caller start() after close() is a new life').start()bumps_generation; the stale partial survives. - Child A's 'close' event fires; the stale-generation guard returns early — the compensating
_readBuffer.clear()never runs. - Child B's first message concatenates onto the partial, fails
JSON.parse, and is silently skipped — e.g. theinitializeresponse is lost andconnect()hangs until timeout.
The ordering in steps 3–5 (data flush before start(), 'close' after it) is exactly the lag shape the PR's own lingering-predecessor test constructs — its fixture just never writes stdout in the window.
Why the SDK's own flows are unaffected — and why this is a nit. The version-negotiation TRANSPORT_RESTART flow is safe: the child exits on its own, so the same-generation 'close' handler runs and clears the buffer before the restart. A back-to-back await close(); await start() is also safe (the generation bumps before any pending I/O macrotask fires). The exposure needs a child that survives stdin-EOF and SIGTERM through the ~4s escalation to SIGKILL, with a pending partial stdout line, on the manual reuse path with an async gap before the restart — narrow, but the PR newly documents and test-pins that path as supported, which is what makes it worth flagging.
Fix (one line). Clear _readBuffer at the top of start() — symmetric with the _stderrStream recreation in the same block — or bump _generation in close() so a locally-closed child's late data/close events are inert (close() already performed the cleanup itself).
Relation to the earlier review comments. The prior comments at this file targeted the then-unguarded data/error/close handlers injecting after start(); this revision fixes that with the generation checks. This finding is the residual pre-start() window those checks cannot cover, plus the guard now suppressing the old compensating late clear — a distinct mechanism with a distinct fix.
| --- | ||
| '@modelcontextprotocol/core-internal': patch | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
|
|
||
| Handle a connection that closes during the `server/discover` version-negotiation probe. | ||
|
|
||
| On stdio, a server process that exits on the unrecognized probe is a legacy signal — symmetric with the stdio probe-timeout rule. Servers built on SDKs that terminate on any pre-`initialize` request (the official Rust SDK, rmcp, is the prominent example) previously connected fine under `mode: 'legacy'` but hard-failed under `mode: 'auto'`; `StdioClientTransport` now restarts itself (respawning the server from its retained parameters) and the client completes the plain `initialize` handshake there — the respawned process sees bytes identical to a plain legacy connect. A transport that was closed locally, or that cannot restart, rejects with a typed negotiation error instead: a deliberately terminated server is never resurrected. On HTTP-class transports a mid-probe close keeps surfacing as the same typed connect error as any other probe transport failure — an ambiguous drop is never era evidence. | ||
|
|
||
| `StdioClientTransport` restart mechanics: child events are generation-scoped (a slow-dying child's late events cannot touch its successor), the read buffer is cleared on child exit, and with `stderr: 'pipe'` the stream spans restarts — it ends exactly once, on `close()`, not on a child's exit. |
There was a problem hiding this comment.
🟡 The PR description matches an earlier iteration of this change, not the shipped code, in four consumer-visible ways — most importantly, the Breaking Changes section tells callers to match error.code === SdkErrorCode.EraProbeConnectionClosed, an enum member that exists nowhere in the codebase (the shipped HTTP mid-probe close rejects with the generic SdkError(EraNegotiationFailed)). The changeset and docs are correct; please refresh the PR description before merge since it will likely become the merge-commit body and reviewers/consumers will follow its (currently unusable) migration advice.
Extended reasoning...
What is stale. The PR description describes a superseded revision of this change and now contradicts the shipped implementation in four ways:
-
Nonexistent error code in the migration advice. The Breaking Changes section says callers "should switch to
error.code === SdkErrorCode.EraProbeConnectionClosed(HTTP)". A grep over the entire checkout finds zero hits forEraProbeConnectionClosed— theSdkErrorCodeenum's negotiation code isEraNegotiationFailed, and the shipped HTTP mid-probe close routes throughclassifyProbeOutcome'sclosedrow →classifyNetworkError→SdkError(EraNegotiationFailed)(packages/client/src/client/probeClassifier.ts, pinned by the newprobeClassifier.test.tsrows and documented in bothdocs/protocol-versions.mdanddocs/migration/support-2026-07-28.md). A caller following the description's advice writes a predicate that cannot even compile. Earlier bot review comments in the timeline confirm a prior iteration did add that enum member — it was dropped in the rework without the description being updated. -
Wrong restart mechanism. The description says
negotiateEra"skips arming the one-shotstart()pass-through so theProtocol.connect()handover's unconditionalstart()respawns the server". The shipped code does the opposite: on theverdict.restartbranch,negotiateEraawaitstransport[TRANSPORT_RESTART]()itself (packages/client/src/client/versionNegotiation.ts), andwindow.release()— which does arm the one-shot pass-through that absorbsProtocol.connect()'sstart()— runs on every success path. Relatedly, the "Additional context" claim that "no new option or transport method is needed" contradicts the diff, which adds theTRANSPORT_RESTARTsymbol andRestartableTransportinterface topackages/core-internal/src/shared/transport.ts. -
Inverted stderr limitation. The description's "Known limitation: with
stderr: 'pipe', thestderrstream ends at the first child's exit and does not carry a restarted process's output" is the exact opposite of the shipped behavior:start()pipes with{ end: false }(packages/client/src/client/stdio.ts), the getter JSDoc says the stream spans restarts and ends exactly once inclose(), and bothstdioRestart.test.tsand the integration test pin that both children's banners reach the shared stream. -
Wrong custom-transport contract. The description says "custom stdio-shaped transports get the same behavior if their
start()supports being called again after close()". The shipped design instead gates restart on theTRANSPORT_RESTARTcapability symbol — a stdio-shaped transport with a perfectly re-callablestart()but no capability gets the typed 'cannot restart' error, pinned by the "stdio-shaped transport WITHOUT the restart capability: typed error" test inversionNegotiation.test.ts.
Step-by-step proof for the most harmful claim (#1). (a) A caller on mode: 'auto' over HTTP wants to handle a mid-probe drop; per the description they write if (error instanceof SdkError && error.code === SdkErrorCode.EraProbeConnectionClosed) retry(). (b) TypeScript rejects the code outright (EraProbeConnectionClosed is not a member of SdkErrorCode); if they hard-code a string instead, the predicate never matches. (c) The actual mid-probe HTTP close rejects with SdkError(EraNegotiationFailed) and the message "Version negotiation probe failed: Connection closed during the version negotiation probe" — exactly what probeClassifier.test.ts's HTTP-close rows pin. The migration advice is unusable as written.
Why this is a nit, not blocking. Everything that actually ships is internally consistent: the changeset (.changeset/probe-close-handling.md), both docs pages, the JSDoc, and the tests all match the code. The only stale artifact is the PR description itself — likely a leftover merge-commit body from the earlier iteration. Per this repo's own Documentation & Changesets convention, in-diff prose is what must match the implementation, and it does. No runtime behavior is wrong and no published artifact misleads consumers; the harm is confined to PR metadata (which will likely become the merge-commit body and the reference reviewers/consumers read).
How to fix. Rewrite the PR description to match the shipped design before merge: drop the EraProbeConnectionClosed migration advice (HTTP closes stay EraNegotiationFailed), describe the TRANSPORT_RESTART capability + in-negotiateEra restart mechanism, replace the stderr "known limitation" with the actual spans-restarts { end: false } behavior, and state that custom transports opt in via the capability symbol rather than a re-callable start().
| if (this._stderrStream?.writableEnded) { | ||
| // close() ended the previous life's aggregate stream; a caller | ||
| // start() after close() begins a new life with a fresh stream. | ||
| this._stderrStream = new PassThrough(); | ||
| } |
There was a problem hiding this comment.
🟡 The stderr getter's JSDoc still promises callers can attach listeners before start() to avoid losing early output, but for the close() → start() second life this PR documents as supported, start() swaps in a fresh PassThrough (stdio.ts:136-140) after close() ended the old one — so listeners attached in the documented pre-start window bind to the dead stream and the new child's stderr (including exactly the promised early output) flows to the fresh, unobserved stream. Fix by recreating the fresh PassThrough at the end of close() (right after ending the old one) so the getter hands out the next life's stream between close() and start(), or scope the attach-before-start guarantee to the first life in the getter/start() JSDoc.
Extended reasoning...
What the bug is. close() now ends the aggregate stderr PassThrough (stdio.ts:304-310) but leaves the ended stream in _stderrStream; the replacement is only created inside the next start() via the writableEnded swap (stdio.ts:136-140). Meanwhile the stderr getter's JSDoc (unchanged in intent by this PR) still promises: "a PassThrough stream is returned immediately, allowing callers to attach listeners before the start method is invoked. This prevents loss of any early error output emitted by the child process." For the close()-then-start() second life that this PR itself establishes as supported (the new start() JSDoc: "After the child process exits, start() may be called again"; the _closedLocally reset; the stdioRestart.test.ts second-life tests), that promise is false: in the window between close() and start() — exactly where the getter tells callers to attach — the getter returns the dead, ended previous-life stream.\n\nThe code path. Every exit path funnels through the same three lines: close() runs this._stderrStream.end(), start() checks this._stderrStream?.writableEnded and assigns this._stderrStream = new PassThrough(), and the getter returns this._stderrStream as-is. Nothing forwards data from the old stream to the new one, and nothing warns the caller that the reference they captured is stale.\n\nStep-by-step proof.\n1. new StdioClientTransport({ command, stderr: 'pipe' }) → constructor creates PassThrough P1.\n2. First life: start(), session runs, await transport.close() → close() unpipes and calls P1.end(); _stderrStream still points at P1.\n3. Caller begins a second life and follows the getter's documented pattern: reads transport.stderr (gets P1, now ended) and attaches 'data' listeners before calling start().\n4. start() sees P1.writableEnded === true and swaps in fresh PassThrough P2; the new child's stderr pipes into P2.\n5. Every byte of the new child's stderr — including exactly the "early error output" the getter promises to preserve — goes to the unobserved P2. The caller's listeners on P1 see nothing, silently. (Verifiers reproduced this deterministically: the stream grabbed before start() is not the current stream, and its listener receives zero data.)\n\nWhy the PR's own tests don't catch it. The second-life stderr test (\"a caller start() after close() begins a fresh stream\" in stdioRestart.test.ts) attaches its second-life listener only after await transport.start() — the documented attach-before-start ordering is never exercised for a second life, because the current API makes it impossible to satisfy. The getter's other new claim ("the stream spans restarts … it ends exactly once, when close is called") is likewise false for the second life, whose stream is never the one close() ended.\n\nWhy nit, not blocking. (a) The SDK's own version-negotiation restart never hits the swap: TRANSPORT_RESTART fires after a child exit, which never ends the stream, so writableEnded is false and no swap occurs — only the manual close()-then-start() path is affected. (b) The data is delayed, not destroyed: the fresh PassThrough buffers (with pipe backpressure) until read, so re-fetching transport.stderr after start() recovers every byte. (c) Pre-PR this exact scenario was strictly worse (the default end: true pipe would write into an already-ended stream), so the PR is a net improvement with a residual doc/contract inconsistency.\n\nHow to fix. Either recreate the fresh PassThrough at the end of close(), immediately after ending the old one — then the getter hands out the next life's stream during the close()→start() window and the attach-before-start contract holds for every life (the writableEnded check in start() can stay as a belt-and-suspenders guard) — or amend the stderr getter/start() JSDoc to state that after close() listeners must be (re)attached only after the next start(), explicitly forfeiting the early-output guarantee for restarted lives.
1 participant
Classify a stdio close during the
server/discoverprobe as a legacy signal: the transport respawns the server and the client falls back toinitializethere.Motivation and Context
With
versionNegotiation: { mode: 'auto' }, the probe classifier treats the "server didn't understand the probe" outcomes inconsistently. A JSON-RPC error reply falls back toinitialize; a stdio timeout falls back toinitialize; but a connection that closes during the probe was a hard connect failure.Closing on a pre-
initializerequest is not an edge case: stdio servers built on SDKs that terminate the process on any pre-initializerequest behave exactly this way (the official Rust SDK, rmcp, is the prominent example — its servers exit(1) with no JSON-RPC reply). Those servers negotiate 2025-era versions correctly and connect fine undermode: 'legacy', soautoturned a working server into a hard connect failure.The fix:
StdioClientTransportcan respawn its server after the child died out from under a connection: child event handlers are generation-scoped (a slow-dying predecessor's late events cannot touch its successor), the read buffer is cleared on child exit, and a transport that was closed locally — caller shutdown or the transport's own error recovery — refuses to respawn. The capability is keyed by an internal symbol incore-internal(isRestartableTransport); nothing is added to any public surface, and nothing outside the transport touches its lifecycle.negotiateEraasks the transport to restart itself and then runs the plaininitializehandshake there — byte-identical to what amode: 'legacy'connect would have sent. A transport without the capability, or whose restart refuses or fails, rejects with the existing typedSdkError(EraNegotiationFailed)carrying the underlying error asdata.cause.stderr: 'pipe'contract. The aggregate stderr stream spans restarts — a child's exit never ends it; it ends exactly once, whenclose()is called.How Has This Been Tested?
send()/events to the new child; a slow-dying predecessor's late close event cannot touch the successor; a localclose()(including read-buffer-overflow recovery) refuses to restart; a dead child's partial trailing line never leaks into the successor's stream; stderr spans restarts and ends exactly once, inclose().{ kind: 'legacy', restart: true }(fallback-agnostic, like the timeout row); HTTP close → typed error, also under a browser environment (the CORS leniency covers opaque fetchTypeErrors, not closes).mode: 'legacy'connect; pin mode and modern-only clients fail loudly with no restart and noinitialize; a stdio-shaped transport without the capability, and one whose restart refuses, reject typed with the cause attached; negotiation never reassignsstartorcloseon a caller's transport.initializerequest and answersinitializewith2025-03-26):mode: 'auto'connects through the transport's respawn and drivestools/listover the respawned pipe;stderr: 'pipe'carries both children's output; a locally closed transport mid-probe rejects typed with no respawn; explicitmode: 'legacy'against the same fixture is untouched.Breaking Changes
None.
mode: 'legacy'(the default) is byte-untouched;mode: 'auto'connects where it previously failed. Withstderr: 'pipe', the stderr stream now ends onclose()rather than on the child's exit.Types of changes
Checklist
Additional context
Restartability is a capability of the transport, not a property of being stdio-shaped: the probe layer checks for it and asks; it never re-arms
start(), wrapsclose(), or reaches into transport state.docs/protocol-versions.md(probe section) documents the close-handling split next to the timeout split.