fix(client): probe stdio servers on a disposable sibling process#2514
Conversation
🦋 Changeset detectedLatest commit: b05977e 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.
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.
bc67606 to
b139ad9
Compare
b139ad9 to
9bb567b
Compare
9bb567b to
760ef81
Compare
dd2fcc7 to
9964407
Compare
9964407 to
2741549
Compare
There was a problem hiding this comment.
Beyond the inline findings, this run also examined and ruled out two candidates: (1) the transport's internal error-recovery close() (the stdout data handler's this.close() on a read error) stamping CLOSED_BY_CALLER = true — that close genuinely is local, so the "true means locally closed" invariant holds and it cannot wrongly suppress the exit-on-probe respawn (the child's exit path never routes through that handler); (2) the respawn's own transport.close() being re-delivered to a pre-set onclose as a spurious duplicate — on the real StdioClientTransport the child is already dead (_process unset) so that close() fires no onclose, and the ProbeWindow's spent-close guard covers the window itself.
Extended reasoning...
Bugs were found this run (the dropped stale-close guard on the per-start ChildProcess 'close' handler, plus the caller-close-mid-probe invariant nit), so the inline comments already signal that human review is needed; this note only records what else was examined and refuted, since this PR has gone through several review rounds and the same candidate areas keep being re-explored. Both ruled-out items were checked against the shipped code: the error-recovery close in the stdout handler is a genuinely local close (stamping true is correct provenance), and a respawn-issued close on a dead child performs no onclose delivery on the real transport, so no duplicate reaches a pre-set observer.
2741549 to
50232f7
Compare
50232f7 to
7c38fba
Compare
felixweinberger
changed the title
7c38fba to
73686a2
Compare
There was a problem hiding this comment.
Beyond the inline findings, one further candidate was examined and ruled out: a session-transport start() failure at the unguarded super.connect(transport) after a sibling-path verdict "escaping all cleanup" — by that point the sibling has already been reaped in negotiateStdioViaSibling's finally, and a failed session start() leaves no child spawned, so nothing leaks; the propagated raw spawn error matches what a plain legacy connect would produce.
Extended reasoning...
This run's finder agents raised a candidate that the unguarded await super.connect(transport) at client.ts:1151 lets a session start() failure escape the cleanup path (no transport.close(), no disarmSpentCloseGuard). Verifiers refuted it: on the sibling path the disposable sibling is disposed inside negotiateStdioViaSibling's finally before the verdict is returned, so it cannot leak; and a session StdioClientTransport.start() that throws (e.g. spawn ENOENT) leaves _process unset, so there is no child to reap and no armed spent-close guard on that path (the guard only arms via the in-place probe window). The escaping error is the same raw spawn error a non-negotiating connect surfaces. Recording this so later review passes need not re-explore it.
| if (callerClosed) { | ||
| throw callerCloseAbortError(); | ||
| } | ||
| return result; | ||
| } finally { | ||
| sessionTransport.close = originalClose; | ||
| await disposeSibling(sibling); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The finally block in negotiateStdioViaSibling restores the caller's close() watch before 'await disposeSibling(sibling)' settles, leaving an unsynchronized window (tens of ms up to ~1-2s of SIGTERM/SIGKILL grace for a live sibling) in which the documented caller-close invariant breaks in both directions: a caller close() landing during disposal is silently lost (the session child then spawns after the caller's shutdown resolved, contradicting the changeset/docs promise that 'the session child is never spawned'), and conversely connect()'s deferred cleanup close() in Client._connectNegotiated's catch (packages/client/src/client/client.ts:1141) can SIGTERM a child the caller legitimately restarted after their own mid-probe close() already resolved. Fix: re-check callerClosed (and restore the close wrapper) only after disposeSibling settles, and skip the cleanup close in client.ts when the failure came from the sibling path, where the session transport is known-unstarted.
Extended reasoning...
What the bug is. The finally in negotiateStdioViaSibling (packages/client/src/client/versionNegotiation.ts:613-616) runs sessionTransport.close = originalClose first and only then await disposeSibling(sibling). For any sibling still alive at verdict time — which is the normal modern-verdict path (the sibling answered server/discover and must be reaped) and the silent-timeout legacy path — StdioClientTransport._dispose is a genuine macrotask window: stdin.end() → SIGTERM → up to 1s grace → SIGKILL → await exited. For that whole window the caller-close watch is uninstalled, and the if (callerClosed) throw at :609 has already run. Nothing re-checks afterwards. This one unsynchronized window breaks the PR's documented caller-close guarantee in both directions.\n\nDirection 1 — a caller close() during disposal is lost; the session child spawns post-shutdown. StdioClientTransport.close() on the never-started session transport (_process === undefined) is a pure no-op: it clears the read buffer and resolves. It sets no flag and cannot reject the pending connect(). So a caller await transport.close() landing during disposeSibling resolves silently, the verdict is returned, and Client._connectNegotiated proceeds straight to super.connect(transport) → transport.start(), which has no closed-check and spawns the session child unconditionally — after the caller's shutdown already resolved, with connect() resolving connected and the child leaked. This contradicts the invariant this PR states verbatim in the changeset, docs/protocol-versions.md, and docs/migration/support-2026-07-28.md: "Closing the caller's transport during the probe aborts connect() with a typed SdkError(EraNegotiationFailed) and the session child is never spawned." From the caller's perspective connect() is still pending, so this is indistinguishable from "during the probe".\n\nDirection 2 — the deferred cleanup close() kills a caller-restarted child. The inverse race at a second code site: a caller transport.close() mid-probe resolves immediately (the wrapper sets callerClosed, fires the abort, and delegates to the no-op original close), but connect()'s rejection is deferred behind the same await disposeSibling(sibling). Only after disposal settles does Client._connectNegotiated's catch run await transport.close().catch(() => {}) (packages/client/src/client/client.ts:1141) against the session transport. Per the close() → start() restart contract this PR itself documents and tests (the integration test "a pre-set onclose observer survives a failed pin negotiation" restarts exactly this way), a caller whose close() has resolved may legitimately start() again; if that restart lands in the dispose window, start() succeeds (_process undefined) and spawns a fresh child — which the pending connect()'s cleanup close then stdin-ends and SIGTERM/SIGKILLs, firing the caller's pre-set onclose. Their brand-new session is torn down by a connect() they already aborted. Note the cleanup close was written for the in-place probe; on the sibling path the session transport was never started by connect(), so in every legitimate sibling-path failure it is a guaranteed no-op — its only observable effect is this harmful race.\n\nStep-by-step proof (direction 1). (1) mode: 'auto' against a modern stdio server: the sibling answers server/discover, the verdict settles, the sibling child is still alive. (2) The finally restores sessionTransport.close to the original, then begins await disposeSibling(sibling) — _dispose ends stdin, sends SIGTERM, and awaits process exit with a 1s grace before SIGKILL. (3) During that await, a supervisor shutdown runs await transport.close() on the session transport: _process === undefined, so it clears the read buffer and resolves — the shutdown believes the client is stopped, and the docs promise the pending connect() will reject typed with no session child. (4) Disposal settles; negotiateStdioViaSibling returns the verdict (the callerClosed check at :609 ran back at step 2 and cannot see the step-3 close). (5) _connectNegotiated calls super.connect(transport) → transport.start() — the session child spawns and connect() resolves connected. A live OS process now exists that no later teardown will reap.\n\nWhy existing tests miss it, and why this is the repo's recurring class. Both the unit caller-close test and the integration "caller close() mid-probe" test land the close while the probe race is still pending — the wrapper catches it and everything works; neither test lands a close (or a restart) during the disposal await, so the window is untested. This is exactly the repository REVIEW.md Async/Lifecycle recurring catch: "Deferred callbacks … must check closed/aborted state before … starting I/O — a callback scheduled pre-close can fire after close/reconnect and corrupt the new connection's state" (#1735, #1763).\n\nHow to fix. For direction 1: in the finally, restore the close wrapper (or at least re-check callerClosed) only after await disposeSibling(sibling) settles — e.g. move the restore below the await and add a post-disposal if (callerClosed) throw callerCloseAbortError(). After that point only microtasks separate the check from start(), so no macrotask caller close can slip through. For direction 2: skip the cleanup close() in client.ts when the failure came from the sibling path (the session transport is known-unstarted there and either untouched or already closed by the caller), or reject before awaiting disposal so the abort settles before a caller can plausibly restart.\n\nSeverity. Filed as nit rather than normal: both directions require a caller lifecycle action (shutdown close, or a restart that does not await the pending connect() rejection) to land inside a tens-of-ms-to-~1-2s window per stdio connect — a probabilistic race whose worst outcome is one leaked (or one killed) child process and a connect settling inconsistently with the caller's view. Every documented/tested flow that awaits the typed rejection first is unaffected. Still worth fixing before or shortly after merge, since it punches a hole in a guarantee the PR advertises unconditionally, and the fix is small at both sites.
There was a problem hiding this comment.
Direction 1 is fixed: the finally now disposes the sibling FIRST with the close watch still armed, restores sessionTransport.close only after disposeSibling settles, and re-checks callerClosed after the finally — so a caller close() landing during the disposal window now aborts typed with the session transport never started. The new unit test ("a caller close() landing during sibling DISPOSAL still aborts") pins exactly this. 👍
Direction 2 is still open, though. The rejection is still deferred behind await disposeSibling(sibling) (the callerClosed throw sits after the finally), and the cleanup close in Client._connectNegotiated's catch (packages/client/src/client/client.ts:1147) still runs unconditionally — including for sibling-path failures, where the session transport is known-unstarted and the close is a guaranteed no-op in every legitimate flow. The harmful race therefore remains: a caller whose mid-probe close() already resolved can legitimately start() again during the disposal window (up to ~1–2s of SIGTERM/SIGKILL grace for a live sibling); the deferred cleanup close then SIGTERMs the freshly restarted child and fires the caller's pre-set onclose for a session the aborted connect() never owned. Either of the originally suggested fixes still applies: skip the cleanup close when the failure came from the sibling path, or reject before awaiting disposal so the abort settles before a restart can plausibly land.
Given the fixed direction was the one breaking the documented invariant and this remainder is a narrow probabilistic race, fine to defer — but flagging it so the partial fix isn't mistaken for full coverage.
73686a2 to
ec76435
Compare
ec76435 to
03aa2c0
Compare
| try { | ||
| proc?.stdout?.destroy(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| try { | ||
| proc?.stdin?.destroy(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| try { | ||
| proc?.stderr?.destroy(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| this._readBuffer.clear(); |
There was a problem hiding this comment.
🟣 Pre-existing issue (close() is untouched by this diff): the session-path close() has the exact parent-side pipe-handle leak that this PR's new _dispose fixes one method above — in the helper-holding server shape this PR documents and ships a fixture for (rmcp-holding), the child exits promptly but its 'close' event is deferred by the helper's grip, so close() burns the 2s race, skips both kill branches (exitCode !== null), and returns leaving the flowing stdout read handle ref'd, pinning the host event loop until the helper exits (unbounded for a daemon helper). Since the PR both fixes the identical bug in _dispose and elevates the triggering shape to a documented, fixture-backed scenario, completing the migration here is natural: mirror the three guarded destroy() calls (processToClose.stdout/stdin/stderr) in close(), after the child is confirmed exited.
Extended reasoning...
What the bug is. This PR's new _dispose (packages/client/src/client/stdio.ts:229-271) ends with three guarded destroy() calls on the parent-side pipe handles, with a comment naming exactly why: "A helper process holding the inherited write ends would otherwise keep them (and with them the host's event loop: stdout carries a flowing 'data' listener from start()) alive until the helper exits." The ordinary close() immediately below (stdio.ts:273-312) — the teardown every SESSION transport goes through via client.close() — has the identical anatomy (end stdin, race the ChildProcess 'close' event against 2s, escalate SIGTERM/SIGKILL keyed on exitCode) but never destroys processToClose.stdout/stdin/stderr. The fix was applied only to the sibling-reaper path.\n\nThe code path that triggers it. The triggering server shape is the one this PR itself documents as supported (docs/protocol-versions.md: "an exit hidden behind a helper process holding them open falls to the probe-timeout path") and ships as the rmcp-holding fixture in test/integration/test/client/versionNegotiation.test.ts: a stdio server that spawns a helper with stdio: 'inherit'. Per Node semantics, a ChildProcess's 'close' event fires only after all of its stdio streams close — and the helper holds the inherited write ends open past the child's own exit.\n\nStep-by-step proof. (1) A CLI connects under versionNegotiation: { mode: 'auto' } to an rmcp-holding-shaped server; the sibling probe lands the legacy verdict, the session child spawns, work completes. (2) The caller runs await client.close() → StdioClientTransport.close(). (3) close() calls processToClose.stdin?.end(); the child exits promptly (exitCode set) — but its 'close' event does not fire, because the helper still holds the pipe write ends, so closePromise never resolves. (4) The first Promise.race burns the full 2s and falls through to the timeout arm. (5) Both kill branches are skipped: processToClose.exitCode !== null. (6) close() returns — but the parent-side stdout Socket still carries the flowing 'data' listener installed by start() (stdio.ts:160), a ref'd libuv handle that keeps Node's event loop alive until it sees EOF, which arrives only when the helper releases the write end. For a daemon helper that is never: the host process cannot exit after client.close() resolves. This is exactly the mechanism three independent verifiers reproduced empirically for _dispose on this checkout (~40ms vs ~4s host-exit delta, unbounded for a daemon), which the author accepted and fixed — for the sibling path only.\n\nWhy existing code and tests don't catch it. The PR's own rmcp-holding integration test drives this exact flow (its finally calls client.close() on the session transport, whose fixture child also spawned the holding helper), but the vitest worker process keeps running regardless of dangling handles, so the host-pinning effect is invisible to the suite — the test passes while the leak occurs.\n\nWhy this is pre-existing rather than a regression. close() is untouched by this diff, and the leak was reachable before the PR on any stderr-inheriting-helper scenario, including plain mode: 'legacy' connects — merging this PR causes no new failure. But the repo's Completeness recurring catch applies squarely: "partial migrations leave sibling code paths with the very bug the PR claims to fix — flag every leftover site." This PR fixes the identical bug in _dispose and newly elevates the helper-holding server shape from an obscure corner to a documented, fixture-backed supported scenario, so it is the natural PR to complete the migration.\n\nHow to fix. Mirror _dispose's tail in close(): after the child is confirmed exited (and on the path where this._process was already undefined), run the same three guarded calls — processToClose.stdout?.destroy(), processToClose.stdin?.destroy(), processToClose.stderr?.destroy(), each in try/catch. A test analogous to the new _dispose unit test (packages/client/test/client/stdio.test.ts, asserting proc.stdout?.destroyed/proc.stdin?.destroyed after close() against a holding child) would pin it.
Some stdio servers exit on any pre-initialize request, so the server/discover version-negotiation probe killed the server and connect() hard-failed under mode 'auto' while mode 'legacy' worked. The probe now runs on a short-lived sibling spawned from the same parameters (stderr discarded, reaped once the era is known — awaiting process exit, never held-open pipes) and the caller's transport starts exactly once, afterwards: a legacy verdict connects with the plain initialize handshake, byte-identical to mode 'legacy'; a modern verdict adopts the sibling's DiscoverResult directly, so the session wire never carries server/discover. Closing the caller's transport during the probe aborts promptly with the typed error and the session child is never spawned. On HTTP, and on custom stdio-shaped transports (which probe in place), a mid-probe close keeps rejecting with the typed connect error, now naming the close in pin/modern-only diagnostics.
03aa2c0 to
b05977e
Compare
| await client.connect(transport); | ||
| const inbound = recordInbound(transport); | ||
|
|
||
| // 2026 negotiated via discover on the pipe — no initialize was ever written. | ||
| // 2026 negotiated via the disposable sibling probe — the session | ||
| // pipe carries neither initialize nor server/discover. | ||
| expect(client.getNegotiatedProtocolVersion()).toBe(MODERN); | ||
| expect(outbound.some(message => (message as { method?: string }).method === 'initialize')).toBe(false); |
There was a problem hiding this comment.
🟡 Three test files this PR edits keep pre-sibling header/comment wording that now contradicts the updated prose and assertions one screenful below: (1) this file's header JSDoc (lines ~9-11) still says the auto-mode client does server/discover "on the pipe" while the updated assertion at line ~145 requires the session pipe to carry NO server/discover; (2) test/e2e/scenarios/stdio-dual-era.test.ts header (lines ~9-13) still says the auto client reaches 2026-07-28 "over server/discover" opening the connection, contradicting the flipped assertion at line 73; (3) packages/client/test/client/probeFixtureCorpus.test.ts lines 7 and 90 keep "initialize on the SAME connection" directly above the T9 test name this PR already reworded to "initialize fallback" at line 92. Reword all three the way the PR reworded the matching inline comments (e.g. "via the disposable sibling probe — the session pipe carries neither initialize nor server/discover").
Extended reasoning...
What is stale. This PR moves the SDK stdio probe onto a disposable sibling process and systematically reworded the pre-sibling "server/discover on the pipe" / "initialize on the same connection/stream/pipe" phrasing across the docs (docs/migration/support-2026-07-28.md, docs/protocol-versions.md, docs/troubleshooting.md), the VersionNegotiationMode / timeoutMs / ClientOptions.versionNegotiation JSDoc, the probeClassifier.ts source comments, and — after prior review rounds — the classifier test names and the versionNegotiation.test.ts prose. Three straggler sites of the exact replaced pattern survive in test files this same diff edits, each contradicting text the diff updated one screenful away:
-
test/integration/test/server/dualEraStdio.test.tsfile-header JSDoc (lines ~3-15, untouched by the diff). The header still enumerates the auto-mode leg as "the negotiating client in auto mode (the 2026-07-28 vertical:server/discoveron the pipe, then list → call with the per-request envelope...)" — while the very test that bullet describes was rewritten by this PR: its name now says "via the sibling probe, the session pipe pins modern from its first enveloped request", its inline comment says "the session pipe carries neither initialize nor server/discover", and the assertion flipped fromexpect((outbound[0]).method).toBe('server/discover')toexpect(outbound.some(m => m.method === 'server/discover')).toBe(false). The header promises a wire the test explicitly asserts does not exist. Secondarily, the header's "spawned as a real child process — once per connection" is now misleading for the auto leg: a singleclient.connect()undermode: 'auto'spawns two fixture children (the disposable sibling plus the session child), so it is worth touching in the same reword — though note each spawned process still serves exactly one connection, so this part alone would not warrant a comment. -
test/e2e/scenarios/stdio-dual-era.test.tsheader (lines ~9-13, untouched). Still reads "the spec-version axis selects which client opens the connection — a plain 2025 client overinitialize, or the auto-negotiating client reaching 2026-07-28 overserver/discover— and the entry pins that connection's instance to the era the client opened with." Post-PR the session connection is never opened withserver/discover: the probe rides the disposable sibling (a separate child), andserveStdiopins the session connection modern from its first enveloped request — exactly the wording this PR's own updated inline comment in the same file uses at lines 56-60, right above the flipped assertionexpect(sentMethods).not.toContain('server/discover')at line 73. -
packages/client/test/client/probeFixtureCorpus.test.tslines 7 and 90. The file-header bullet ("probe-success-then-no-overlap → initialize on the SAME connection") and the T9 section comment ("// --- T9 edge 3: probe success but no version overlap → initialize on the SAME connection.") both survive, while this PR reworded the test name one line below (line 92) from "initialize on the same connection" to "initialize fallback" (addressing a prior review round) and reworded the identical phrase inprobeClassifier.ts(classifyResultcomment,legacy-verdict JSDoc). The line-90 comment now contradicts the corrected test name in the same screenful.
Why the old wording is wrong on the shipped code. For the base StdioClientTransport — the population this PR targets — Client._connectNegotiated routes through readStdioServerParams → negotiateStdioViaSibling: the probe (and any timeout or no-overlap legacy verdict) is measured on the disposable sibling, the sibling is reaped via _dispose, and only then does super.connect(transport) start the session transport — spawning a second child — with _legacyHandshake sending initialize on that child's fresh pipe, or a modern verdict adopted with the session wire carrying neither initialize nor server/discover. No pipe is shared between the probe and the session; "on the pipe" / "the SAME connection" describes a wire that no longer exists for this transport. "Same connection" remains accurate only for in-place probes (HTTP, custom stdio-shaped transports, StdioClientTransport subclasses) — which is why the PR's own rewordings qualify rather than delete the phrase, and why the third dualEraStdio.test.ts header bullet ("initialize on the same pipe" for the raw hand-driven probe-then-fallback exchange, which uses no Client at all) is still correct and should stay.
Step-by-step proof for site 1. (1) Run the modern-opening test in dualEraStdio.test.ts under the shipped code: client.connect(transport) with mode: 'auto' detects the base StdioClientTransport, spawns a sibling from the same _serverParams, and sends server/discover on the sibling's pipe. (2) The sibling's verdict is modern; the sibling is reaped. (3) The session transport starts (its first and only spawn) and the recorded outbound on the session pipe contains no server/discover — the assertion at line ~145 passes precisely because the header's described exchange never touches this pipe. (4) A reader who trusts the header and looks at the wire to debug will search for a server/discover frame that is never written.
Why nothing catches it. These are comments and JSDoc — no assertion pins them, so the suite stays green while the prose describes the superseded design. This matches the repo's Completeness recurring catch ("when a PR replaces a pattern, grep for surviving instances of the old form") and the Documentation catch (prose contradicting the implementation in the same diff). None of the three sites was named in the prior review rounds (those covered the probeClassifier.ts comments, the classifier test names including probeFixtureCorpus.test.ts:92 itself, and versionNegotiation.test.ts — all since fixed).
How to fix. Reword the three sites the same way the PR reworded the adjacent text: in dualEraStdio.test.ts, change the auto-mode header bullet to something like "the negotiating client in auto mode (2026-07-28 negotiated via the disposable sibling probe — the session pipe carries neither initialize nor server/discover, and pins modern from its first enveloped request)" and qualify the spawn-count sentence for the auto leg; in stdio-dual-era.test.ts, reword the header bullet to "the auto-negotiating client reaching 2026-07-28 via the disposable sibling probe, the session connection pinned from its first enveloped request"; in probeFixtureCorpus.test.ts, change lines 7 and 90 to "→ the legacy initialize fallback", matching the line-92 test name. Comment-only changes; nothing behavioral.
| caller's one child process. A child that exits on the probe is simply a legacy server; | ||
| the caller's transport spawns exactly once, after the era is known, and its wire never | ||
| carries `server/discover`. Closing the caller's transport during the probe aborts | ||
| `connect()` with a typed `SdkError(EraNegotiationFailed)` and the session child is | ||
| never spawned. On HTTP — and on custom stdio-shaped transports, which probe in | ||
| place — a mid-probe connection close rejects with the same typed error as any probe | ||
| transport failure. | ||
|
|
There was a problem hiding this comment.
🟡 Two paragraphs in the "Who should not default to 'auto'" section still describe the pre-sibling probe behavior: lines ~114-119 claim "the probe round trip changes recorded transcripts/raw logs" on stdio, but this PR's sibling design makes the session wire byte-identical to a plain mode: 'legacy' connect (pinned by this PR's own unit test) — this same diff already reworded the exact parallel ::: warning in docs/protocol-versions.md to drop the transcript claim and name the real remaining cost (one extra short-lived spawn per connect). Similarly, lines ~121-126 ("the legacy-fallback path also begins with one enveloped probe") are now true only for HTTP and in-place stdio-shaped transports; on the base StdioClientTransport the enveloped probe rides the disposable sibling and never touches the session wire. Reword both to match the protocol-versions.md rewording.
Extended reasoning...
What's stale. This PR moves the SDK stdio probe onto a disposable sibling process, and it carefully reworded the affected prose almost everywhere — including the exact parallel ::: warning block in docs/protocol-versions.md, which the diff changed from "the extra round trip changes recorded transcripts" to "the probe spawns an extra short-lived server process per connect." But the sibling paragraph in docs/migration/support-2026-07-28.md (in the "Who should not default to 'auto'" section, immediately below the Probe policy section this PR rewrites) survived untouched:
…and the probe round trip changes recorded transcripts/raw logs, which matters for tools whose value is byte-stable observation.
Why it's now wrong for exactly its stated audience. The paragraph is explicitly scoped to spawn-per-invocation CLI tools on stdio. Post-PR, on the base StdioClientTransport (the transport such a tool uses), the server/discover probe rides the disposable sibling and the caller's session wire is byte-identical to a plain mode: 'legacy' connect. This is pinned by the PR's own tests: the unit test "the session's traffic is byte-identical to a plain mode:'legacy' connect" (packages/client/test/client/versionNegotiation.test.ts) asserts JSON.stringify(autoSession.sent) === JSON.stringify(plainSession.sent), and the integration test asserts the session pid's wire log contains initialize but never server/discover.
Step-by-step proof. (1) A spawn-per-invocation CLI tool defaults to 'auto' and records its own transport transcript (the population the paragraph warns). (2) Client._connectNegotiated calls readStdioServerParams(transport); the base class's prototype owns _dispose and retains _serverParams, so the sibling path is taken. (3) negotiateStdioViaSibling spawns the sibling, runs the probe there, reaps it. (4) Only then does the session transport start, and _legacyHandshake sends a plain initialize — the recorded transcript is identical to a mode: 'legacy' run. The "changes recorded transcripts" rationale is therefore false for this transport, while the paragraph omits the cost that IS new and real: one extra short-lived server spawn per connect (which is exactly what the reworded protocol-versions.md warning and the PR description's Breaking Changes section both name).
Secondary staleness two paragraphs down (~lines 121-126): "Tooling that classifies traffic must not treat 'saw an envelope' as 'modern era negotiated': the legacy-fallback path also begins with one enveloped probe." On the base stdio transport the enveloped probe went to the sibling; the session wire's legacy fallback begins with a plain un-enveloped initialize. The claim remains true only for HTTP and for in-place stdio-shaped transports/subclasses, so it needs the same scoping the rest of the diff applies (e.g. "on HTTP and in-place stdio probes, the legacy-fallback path also begins with one enveloped probe; on the SDK's base stdio transport the enveloped probe rides the sibling and never touches the session wire").
Why nothing catches it. Docs prose isn't pinned by any test, and the file's other stale sites (lines 59 and 79-81, flagged in an earlier round) were fixed by this PR — these two paragraphs sit just below the rewritten section and were simply missed. This matches the repo's Completeness recurring catch (surviving instances of a replaced phrasing pattern in a file the same diff edits) and the Documentation checklist item ("flag prose that now contradicts the implementation").
How to fix. Mirror the protocol-versions.md rewording: in the "Who should not default to 'auto'" paragraph, replace the transcript sentence with the extra-spawn cost ("and the probe spawns an extra short-lived server process per connect"), and scope the "saw an envelope" caveat to HTTP and in-place stdio probes. Docs-only; nothing breaks at runtime.
1 participant
Probe stdio servers on a disposable sibling process, so a server that exits on the unrecognized probe still connects on the legacy era — and the caller's transport spawns exactly once, after the era is known.
Motivation and Context
Some stdio servers exit on any pre-
initializerequest — servers built on the official Rust SDK, rmcp, behave exactly this way. UnderversionNegotiation: { mode: 'auto' }theserver/discoverprobe killed such a server andconnect()hard-failed with a connection-closed error, whilemode: 'legacy'connected fine. The specification's stdio backward-compatibility guidance prescribes probing and falling back toinitializewhen the server errors, closes, or stays silent past a reasonable timeout; it does not require the probe and the session to share one process.Three sentences of design: the probe measures the server on a disposable sibling process spawned from the same
StdioServerParameters— its stderr is discarded and it is reaped (signal escalation awaiting process exit, so a helper holding its pipes can never block disposal) once the era is known. The caller's transport then starts exactly once with the verdict: a legacy verdict connects with the plaininitializehandshake, byte-identical to amode: 'legacy'connect, and a modern verdict adopts the sibling'sDiscoverResultdirectly — the session wire never carriesserver/discover. Closing the caller's transport during the probe abortsconnect()promptly with the typedSdkError(EraNegotiationFailed)and the session child is never spawned.A child's exit registers as a close once it closes the child's stdio pipes; an exit hidden behind a helper process holding them open falls to the probe-timeout path, which lands the same legacy verdict. The sibling path requires exactly the SDK's
StdioClientTransport: subclasses cannot be faithfully cloned by re-invoking their constructor with the retained parameters alone, so they — like custom stdio-shaped transports without readable spawn parameters — probe in place. On HTTP, and on those in-place transports, a mid-probe connection close keeps rejecting with the typed connect error (naming the in-place scope), and pin-mode and modern-only diagnostics name the close instead of implying aserver/discoververdict that never happened.How Has This Been Tested?
'auto'— the sibling spends itself and is reaped (asserted by pid), the session child servestools/callon its only spawn, and per-child wire logs prove the session sentinitializebut neverserver/discover; a modern fixture connects with a wire that carries neitherserver/discovernorinitialize; a callerclose()mid-probe rejects typed with the session child never spawned; a probe child whose helper holds its pipes falls to the timeout row and the session still connects._serverParams): sibling spawn/reap for rmcp, modern, silent, and spawn-failure shapes; session traffic byte-identical tomode: 'legacy'; prompt caller-close abort withclose()identity restored; pin-mode and modern-only rejections naming the close; in-place typed error for stdio-shaped transports without spawn parameters; HTTP close never a legacy verdict.transport.startregains its original identity after the handover; a pre-setoncloseobserver keeps receiving closes after both failed AND succeeded negotiations that saw a mid-window close (the spent-close guard is disarmed on every exit path); a callerclose()landing during sibling disposal still aborts with the session never started; a subclass ofStdioClientTransportgets the in-place probe with a typed error naming the base-class requirement; and the sibling reaper releases the parent-side pipe handles, so a helper process holding the probe child's stdio cannot pin the host's event loop — each pinned red-first.Breaking Changes
None.
mode: 'legacy'(the default) is byte-untouched. Under'auto'or a pin, each stdio connect now spawns one extra short-lived server process for the probe.Types of changes
Checklist
Additional context
The sibling seam is internal: the spawn parameters are read structurally from the SDK's own transport and the sibling is constructed from the transport's own class — no new public API, no new error code, and the transport's lifecycle code is untouched apart from an internal disposal helper. The probe timeout continues to inherit the connect timeout (
probe.timeoutMsoverrides); the sibling design would make a lower stdio default safe as a one-line follow-up.