Isolate the stdio server's stdin and stdout from handler subprocesses#3117
Isolate the stdio server's stdin and stdout from handler subprocesses#3117maxisbey wants to merge 4 commits into
Conversation
118422b to
1eec799
Compare
📚 Documentation preview
|
1eec799 to
e1d6e64
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/stdio.py">
<violation number="1" location="src/mcp/server/stdio.py:91">
P2: Each completed default `stdio_server()` session leaks its duplicated protocol descriptor. `closefd=False` prevents wrapper cleanup from closing `private_fd`, and restoration duplicates it back onto fd 0 without closing the source. Repeated start/stop sessions can therefore exhaust the process descriptor limit; close the private duplicate after restoration (the task group has already finished the reader before this `finally` runs).</violation>
</file>
<file name="docs_src/troubleshooting/tutorial009.py">
<violation number="1" location="docs_src/troubleshooting/tutorial009.py:15">
P1: This example exposes arbitrary Python execution to every MCP client that can call `run_script`, because the client-controlled `path` is passed directly to the interpreter. The subprocess example can retain the stdin guidance without teaching an unrestricted code-runner; use a server-configured script path or validate against a narrowly scoped allowlist before spawning.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| """Run a Python script and return what it printed.""" | ||
| process = await asyncio.create_subprocess_exec( | ||
| sys.executable, | ||
| path, |
There was a problem hiding this comment.
P1: This example exposes arbitrary Python execution to every MCP client that can call run_script, because the client-controlled path is passed directly to the interpreter. The subprocess example can retain the stdin guidance without teaching an unrestricted code-runner; use a server-configured script path or validate against a narrowly scoped allowlist before spawning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs_src/troubleshooting/tutorial009.py, line 15:
<comment>This example exposes arbitrary Python execution to every MCP client that can call `run_script`, because the client-controlled `path` is passed directly to the interpreter. The subprocess example can retain the stdin guidance without teaching an unrestricted code-runner; use a server-configured script path or validate against a narrowly scoped allowlist before spawning.</comment>
<file context>
@@ -0,0 +1,23 @@
+ """Run a Python script and return what it printed."""
+ process = await asyncio.create_subprocess_exec(
+ sys.executable,
+ path,
+ stdin=subprocess.DEVNULL,
+ stdout=subprocess.PIPE,
</file context>
| if private_fd is not None: | ||
| # A completed dup2 is undone; an untouched fd 0 is re-pointed at | ||
| # the same pipe it already holds, which is harmless. | ||
| _restore_fd(0, private_fd) |
There was a problem hiding this comment.
P2: Each completed default stdio_server() session leaks its duplicated protocol descriptor. closefd=False prevents wrapper cleanup from closing private_fd, and restoration duplicates it back onto fd 0 without closing the source. Repeated start/stop sessions can therefore exhaust the process descriptor limit; close the private duplicate after restoration (the task group has already finished the reader before this finally runs).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/stdio.py, line 91:
<comment>Each completed default `stdio_server()` session leaks its duplicated protocol descriptor. `closefd=False` prevents wrapper cleanup from closing `private_fd`, and restoration duplicates it back onto fd 0 without closing the source. Repeated start/stop sessions can therefore exhaust the process descriptor limit; close the private duplicate after restoration (the task group has already finished the reader before this `finally` runs).</comment>
<file context>
@@ -17,61 +17,159 @@ async def run_server():
+ if private_fd is not None:
+ # A completed dup2 is undone; an untouched fd 0 is re-pointed at
+ # the same pipe it already holds, which is harmless.
+ _restore_fd(0, private_fd)
+ os.close(private_fd)
+ return sys.stdin.buffer, None
</file context>
| _restore_fd(0, private_fd) | |
| _restore_fd(0, private_fd) | |
| with suppress(OSError): | |
| os.close(private_fd) |
e1d6e64 to
bf24e5e
Compare
While serving on the process's real stdin, stdio_server() now reads the protocol from a private duplicate of fd 0 and points fd 0 (and, on Windows, the standard input handle) at the null device, restoring both on exit. Children spawned by handler code then inherit the null device instead of the protocol pipe. A child that inherited the pipe could consume protocol bytes on any platform, and on Windows a Python child hangs inside interpreter startup behind the transport's pending read (CPython gh-78961) until the next request arrives, so any tool that ran a subprocess without stdin=DEVNULL appeared to hang until timeout. Isolation engages only when sys.stdin is backed by the real fd 0, at most once per process, and degrades to reading stdin in place when the descriptor table cannot be rearranged. Fixes #671.
bf24e5e to
42ef19a
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the nested-transport finding already flagged inline, this pass also examined and ruled out two candidates. The retained private_fd after restore() (the descriptor cubic's suggestion proposes closing) is deliberate, not a leak: an abandoned reader thread can still be blocked in read() on that descriptor past the transport's lifetime — the closefd=False comment documents this — so closing it would free the fd number for reuse under a live read; the cost is one retained fd per session, and a process typically runs one. Also checked the migration-guide claim that explicit streams skip the descriptor changes: the claim path is governed solely by the stdin= parameter, so the sentence holds for the both-streams injected usage it describes (an explicit stdout= alone does not skip it — a wording nit at most).
Extended reasoning...
This run's bug hunt surfaced no new findings; the prior inline finding on the nested-transport path stands unaddressed and no commits have landed since. The note above records what was additionally examined and refuted this pass — chiefly the private-fd retention, which two automated reviewers reached opposite conclusions on: closing it after restore would reintroduce the fd-recycling hazard the code's closefd=False comment guards against, since anyio's blocking file reads are abandoned (not cancelled) in worker threads and can outlive the transport. Recording this so the author and any later review pass don't re-litigate it or apply the suggested close.
While serving on the process's real stdout, stdio_server now moves the protocol pipe to a private descriptor and points fd 1 - and, on Windows, the standard output handle - at stderr, restoring it when the transport exits. A stray print() in handler code or a child process writing to its inherited stdout lands in the client's log instead of corrupting the JSON-RPC stream. The null device stands in when stderr is unusable or, on POSIX, is detected as merged into stdout (2>&1). The stdin claim generalizes into the shared _claim_fd mechanism: one lock-guarded sentinel table covers both descriptors, private wire duplicates are forced above the standard descriptor range so a process started with a standard descriptor closed cannot hand the wire out as its "stderr", and a failed claim degrades to serving the sys stream's buffer in place exactly as v1 did. Docs now describe the guarded behavior with its remaining gaps (output flushed before serving begins, injected streams, merged stderr on Windows), and the transport:stdio:stream-purity divergence narrows accordingly.
maxisbey
changed the title
There was a problem hiding this comment.
2 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/handlers/logging.md">
<violation number="1" location="docs/handlers/logging.md:37">
P2: Windows processes launched with stderr merged into stdout can still send `print()` output to the protocol pipe, so this guarantee is misleading. Qualify the claim with the merged-stream caveat (or direct readers to the migration caveats).</violation>
</file>
<file name="docs/migration.md">
<violation number="1" location="docs/migration.md:1862">
P2: Windows processes launched with stderr merged into stdout can still send stray handler or child output onto the protocol pipe, but this migration note says fd 1 is always diverted away from the wire and that capturing child stdout is no longer needed. Please document this Windows merge exception here (and retain the capture workaround for that launch shape), so users do not rely on isolation that the implementation deliberately cannot provide.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| Never `print()` in a stdio server. `print` writes to **stdout**, and stdout *is* the wire: one stray | ||
| line and the client is trying to parse it as JSON-RPC. | ||
| Don't `print()` in a stdio server. `print` writes to **stdout**, and stdout belongs to the protocol: | ||
| while serving, the SDK diverts stray stdout to stderr so it can't corrupt the wire, but that leaves |
There was a problem hiding this comment.
P2: Windows processes launched with stderr merged into stdout can still send print() output to the protocol pipe, so this guarantee is misleading. Qualify the claim with the merged-stream caveat (or direct readers to the migration caveats).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/handlers/logging.md, line 37:
<comment>Windows processes launched with stderr merged into stdout can still send `print()` output to the protocol pipe, so this guarantee is misleading. Qualify the claim with the merged-stream caveat (or direct readers to the migration caveats).</comment>
<file context>
@@ -33,8 +33,9 @@ For a **stdio** server this question matters more than usual. The host launched
- Never `print()` in a stdio server. `print` writes to **stdout**, and stdout *is* the wire: one stray
- line and the client is trying to parse it as JSON-RPC.
+ Don't `print()` in a stdio server. `print` writes to **stdout**, and stdout belongs to the protocol:
+ while serving, the SDK diverts stray stdout to stderr so it can't corrupt the wire, but that leaves
+ your line interleaved raw among the log output -- no level, no logger name, no way to filter it.
</file context>
|
|
||
| While serving on the process's real stdin and stdout, the stdio server transport now | ||
| duplicates each protocol pipe to a private descriptor and points the standard | ||
| descriptors — with their Windows standard handles — away from the wire, restoring |
There was a problem hiding this comment.
P2: Windows processes launched with stderr merged into stdout can still send stray handler or child output onto the protocol pipe, but this migration note says fd 1 is always diverted away from the wire and that capturing child stdout is no longer needed. Please document this Windows merge exception here (and retain the capture workaround for that launch shape), so users do not rely on isolation that the implementation deliberately cannot provide.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/migration.md, line 1862:
<comment>Windows processes launched with stderr merged into stdout can still send stray handler or child output onto the protocol pipe, but this migration note says fd 1 is always diverted away from the wire and that capturing child stdout is no longer needed. Please document this Windows merge exception here (and retain the capture workaround for that launch shape), so users do not rely on isolation that the implementation deliberately cannot provide.</comment>
<file context>
@@ -1855,31 +1855,40 @@ group (spawned with `start_new_session=True`); the `getpgid()` lookup and the
+
+While serving on the process's real stdin and stdout, the stdio server transport now
+duplicates each protocol pipe to a private descriptor and points the standard
+descriptors — with their Windows standard handles — away from the wire, restoring
+both when the transport exits: fd 0 reads the null device, and fd 1 writes to stderr
+(the null device if stderr is unusable). Subprocesses started by handler code
</file context>
There was a problem hiding this comment.
Beyond the inline findings, two candidates were examined and ruled out this run: (1) the per-session private-descriptor "leak" (also raised in an earlier bot review) — restore() deliberately leaves the private fd open because a worker thread can still sit blocked on it past the transport's lifetime, a one-fd-per-stream-per-session tradeoff documented in the closefd=False comment, not an accidental leak; (2) _claim_fd crashing on injected sys streams lacking a .buffer attribute — _is_backed_by_fd catches the AttributeError for the probe, and the subsequent stream.buffer access matches v1's unconditional sys.stdin.buffer access, so no regression.
Extended reasoning...
This run's finders raised the descriptor-leak and missing-.buffer candidates and verifiers refuted both; I confirmed the refutations against the diff (the leak is a deliberate, code-documented design choice tied to blocked worker-thread reads, and the .buffer access shape is identical to v1). Recording them here so the author and later review passes don't re-explore them from scratch — the descriptor-leak in particular was independently raised by another bot review on this PR. This is informational only, not a correctness guarantee; the two inline nits from this run stand on their own.
| # the same pipe it already holds, which is harmless. | ||
| _restore_fd(fd, private_fd) | ||
| os.close(private_fd) | ||
| # fd still holds the protocol pipe, so the sys stream's buffer is | ||
| # target-consistent: serve it in place, exactly as v1 did - shared | ||
| # write ordering, no new descriptors to allocate in a process whose | ||
| # descriptor table is already failing. | ||
| return stream.buffer, None | ||
|
|
||
| def restore() -> None: | ||
| # Flush first: text buffered in the sys stream during the claim (a | ||
| # stray print() while stdout is claimed) drains to the diversion, not | ||
| # to the restored protocol pipe. | ||
| with suppress(OSError, ValueError): |
There was a problem hiding this comment.
🟡 When _claim_fd's descriptor rearrangement fails with OSError, the fallback path discards fd from _claimed even though the transport goes on serving the real fd 0/fd 1 in place for its whole lifetime — so a nested or concurrent stdio_server() entered afterward finds the fd unclaimed, claims it, and moves the wire out from under the outer transport (its reader sees EOF and the client's frames are delivered to the inner transport instead). Keeping fd in _claimed on the fallback path, with an undo that only discards the sentinel, makes a nested transport take the serve-in-place branch as the design intends.
Extended reasoning...
What the bug is
The _claimed sentinel table exists — per the module comment at src/mcp/server/stdio.py:36-41 — so that "a second concurrent stdio_server() must not claim an fd again ... and its restore would clobber the first transport's." The best-effort fallback breaks that invariant: in _claim_fd's except OSError handler (src/mcp/server/stdio.py:147-160), _claimed.discard(fd) runs and stream.buffer — the real fd 0/fd 1, still holding the wire — is returned with restore=None. The transport then serves the standard descriptor in place for its whole lifetime with no sentinel protecting the descriptor it is actively serving. The _claimed check at lines 121-127 is the only guard in the module against a second transport re-claiming; once the sentinel is discarded, nothing prevents it.
Step-by-step proof (reproduced experimentally against the real _claim_fd on this branch)
- Transport A enters
stdio_server(). Its claim of fd 0 fails transiently (e.g. an injectedOSErroron the firstos.dup(0)— descriptor exhaustion that later clears, exactly the environment the fallback exists for). A takes the fallback: it servessys.stdin.buffer(fd 0 = the wire) in place, and_claimedis empty. - Handler or embedder code enters a nested/concurrent
stdio_server()— the exact shape the sentinel is documented and tested to support (test_a_nested_stdio_server_does_not_clobber_the_first_transports_claim). The environment has recovered, so B's_claim_fd(0, ...)finds_is_backed_by_fdtrue and fd 0 unclaimed, and succeeds: the wire moves to B's private descriptor and fd 0 becomes the null device (verified viasameopenfileagainst a devnull probe). - Consequences, all observed: (a) A's in-place reader on fd 0 immediately sees EOF — the outer
stdin_readerends and the outer session dies; (b) a JSON-RPC frame written to the wire for A's session was read by B's private descriptor while A gotb''— B literally steals A's inbound frames; (c) the stdout twin is symmetric: a fallback stdout transport writes fd 1 in place, and a nested claim of fd 1 diverts fd 1 to stderr, so the outer transport's responses land on stderr instead of the wire for the nested transport's lifetime.
Why existing safeguards miss it
The nested-transport tests only cover the claim-succeeded outer shape: the outer transport holds a sentinel, so the inner one serves in place. No test enters a nested transport after a fallback, so the hole is untested. And this is not quite the v1-parity the fallback comment claims: v1 never repointed fd 0 at devnull, so a v1 nested transport raced the outer one for bytes rather than deterministically killing the outer session, moving fd 0 to the null device, and diverting every one of its inbound frames.
Addressing the refutation
One verifier argued this is a non-actionable known limitation: the trigger is a compound rarity, fallback mode is documented v1-parity where nesting was never safe, and the fix would merely have the inner transport open(fd, ...) on the actual wire, racing the outer one — v1's brokenness in another form. The rarity point is fair and is why this is a nit, not a blocker. But the other two points don't hold up: (1) the current behavior is strictly worse than v1 — a race for bytes can still deliver some frames to the right session, whereas the re-claim deterministically EOFs the outer reader and diverts all of its traffic; and (2) the fix does restore the module's stated contract — with the sentinel kept, the inner transport takes the serve-in-place branch (lines 121-127), which is precisely the degraded-but-documented shape the design already prescribes for nesting, and the outer transport's serving arrangement survives, which is the whole invariant the comment at lines 36-41 promises. Nesting on a shared wire is inherently degraded either way; the question is whether the sentinel's documented guarantee holds in the fallback shape, and today it doesn't.
The fix
On the fallback path, keep fd in _claimed for the transport's lifetime, returning a restore callback that only discards the sentinel (no descriptor work — there is nothing to restore). A nested transport then finds the fd claimed and serves in place, exactly as the design intends. The change is a few lines and slots naturally next to the existing restore() closure.
Severity
Nit: the trigger requires a transient OSError during descriptor rearrangement and a nested/concurrent stdio_server() in the same process and a descriptor table that recovers in between — no realistic deployment hits this. But nested/threaded transports are an explicitly supported scenario per this PR's own sentinel design and lock comment, the hole silently kills and hijacks the outer session when it does hit, and the fix is cheap.
There was a problem hiding this comment.
The 52d9409 redesign changes the frame of this finding but doesn't close the hole. Nested/concurrent transports are now refused with RuntimeError instead of served in place, and the fallback block was reordered with the invariant "fd stays claimed for as long as it is diverted" — but the except OSError path still ends with _claimed.discard(fd) while the transport goes on serving the real fd 0/fd 1 in place for its whole lifetime.
The consequence is the same as originally reported, now expressed against the new contract: after a transient claim failure, a concurrent stdio_server() entered once the descriptor table recovers finds the fd unclaimed, passes _is_backed_by_fd and _std_descriptors_open, and claims successfully — the wire moves to its private descriptor, fd 0 becomes the null device, the outer in-place reader EOFs, and the outer session's frames are delivered to the inner transport. That is exactly the contention the new design promises to refuse ("a second concurrent stdio_server() raises RuntimeError instead of contending for the streams"), except in the fallback shape it silently succeeds instead of raising.
The fix is unchanged in spirit and now even simpler: on the fallback path, keep fd in _claimed for the transport's lifetime and return a restore callback that only discards the sentinel (no descriptor work — nothing was diverted). A second transport then gets the same RuntimeError refusal as in the normal shape. Still nit severity: the trigger remains a compound rarity (transient OSError during the claim plus a concurrent transport plus a recovered descriptor table).
The stdin/stdout claim now engages only in a normal process: the sys stream backed by its real descriptor and fds 0-2 all open, which makes it impossible for the private wire duplicates to land in the standard range. Anything else - replaced streams, an incomplete descriptor table, a failed dup - is served in place exactly as v1 was, and a second concurrent stdio_server() raises RuntimeError instead of contending for the streams. This replaces the previous hardening (the dup-above-standard-range loop, stderr-merge detection, and nested transports serving into the diversion) with guards, and removes the migration entry: no working code changes behavior, so there is nothing to migrate. Comments and docstrings trimmed throughout the diff.
There was a problem hiding this comment.
Beyond the inline docstring nit, this run also examined and ruled out two candidate issues: (1) the apparent per-session "leak" of the private wire descriptors (also raised as a P2 in this thread) — closefd=False and the missing os.close(private_fd) on the normal exit path are deliberate per the comment at src/mcp/server/stdio.py:103: a worker thread may still be blocked reading that descriptor after exit, and closing it would let the fd number be recycled under the blocked read; the cost is one bounded fd per completed session, not growth within a serve. (2) The use of pywin32's win32file._get_osfhandle in rebind_std_handle_to_fd — it resolves the CRT fd to the OS handle as intended for repointing the Win32 std-handle slots (stdlib msvcrt.get_osfhandle would be an equivalent spelling, not a correctness difference).
Extended reasoning...
Bugs were found this run (a single documentation-convention nit posted inline), so no approval verdict is offered — the PR is in any case a complex descriptor-level change to the core stdio transport that warrants human review, which the maintainers are already giving it. This note only records what was additionally examined and refuted so later passes need not re-derive it: multiple independent finder agents flagged the un-closed private duplicates as a descriptor leak, and verification against the code showed it is a deliberate, commented, bounded tradeoff protecting a possibly-still-blocked reader thread from fd recycling; the pywin32 private-API concern was likewise verified to be functionally correct.
| async def stdio_server(stdin: anyio.AsyncFile[str] | None = None, stdout: anyio.AsyncFile[str] | None = None): | ||
| """Server transport for stdio: this communicates with an MCP client by reading | ||
| from the current process' stdin and writing to stdout. | ||
| """Serve MCP over the process's stdin and stdout. | ||
|
|
||
| While serving, fd 0 points at the null device and fd 1 at stderr — handlers | ||
| and children read EOF and stray output misses the wire — both restored on exit. | ||
| Explicit streams skip the claim; a second concurrent stdio_server() raises RuntimeError. | ||
| """ |
There was a problem hiding this comment.
🟡 The public stdio_server() now raises RuntimeError when another stdio_server() in this process has already claimed fd 0 or 1, but the docstring only mentions this in prose without a Raises: section — the convention AGENTS.md requires for exceptions a caller would reasonably catch (and which the private _claim_fd helper in this same PR does carry). Moving the prose sentence into a Raises: RuntimeError: block would align the public entry point with the repo convention.
Extended reasoning...
What the issue is
This PR adds a new failure mode to the public stdio_server() API: when the transport goes to claim fd 0 or fd 1 and finds it already claimed by another stdio_server() in the same process, _claim_fd raises RuntimeError("another stdio_server() in this process has already claimed fd ...") (src/mcp/server/stdio.py:74-76), and that exception propagates out of stdio_server() to the caller. The docstring at src/mcp/server/stdio.py:115-121 documents this only in prose: "a second concurrent stdio_server() raises RuntimeError." There is no structured Raises: section.
Why this violates the repo convention
AGENTS.md (Code Quality) states: "Public APIs must have docstrings. When a public API raises exceptions a caller would reasonably catch, document them in a Raises: section." stdio_server is unambiguously public API — it is imported in src/mcp/__init__.py and listed in __all__ (line 141), which AGENTS.md defines as the deliberate public surface.
The "argument validation or programmer error" exemption does not apply here, both on the merits and by the repo's own practice. On the merits: the RuntimeError signals a runtime environmental condition (another transport in the process already owns the standard descriptors), not a malformed argument — an embedder coordinating multiple transports would plausibly catch it to surface a clean error or fall back. By repo practice: comparable misuse-shaped RuntimeErrors get structured Raises: sections throughout the codebase, e.g. ClientSession's "RuntimeError: Called before entering the context manager" (src/mcp/client/session.py) and MCPServer.session_manager's "RuntimeError: If called before streamable_http_app() has been called" (src/mcp/server/mcpserver/server.py).
The internal inconsistency
The strongest signal that this is an oversight rather than a choice: this same PR gives the private _claim_fd helper a proper Raises: section for this exact exception (src/mcp/server/stdio.py:67-68), while the public entry point — the function callers actually catch the error from — gets only prose. The author clearly knows and applies the convention; it just did not make it to the public docstring.
Step-by-step proof
- An embedder enters
stdio_server()(default streams, real fd 0/1) —_claim_fd(0, ...)adds fd 0 to_claimedand the transport serves. - While that transport is live, the same process enters a second
stdio_server()(nested handler, embedding framework, test harness). - The second call reaches
_claim_fd(0, ...), finds0 in _claimed, and raisesRuntimeError("another stdio_server() in this process has already claimed fd 0")— exactly whattest_a_second_stdio_server_on_the_same_process_streams_is_refusedpins withpytest.raises(RuntimeError, match='already claimed fd 0'). - The exception propagates out of
stdio_server().__aenter__to the embedder, who consults the API docs for what to catch — and finds the contract only as a prose aside rather than in the structuredRaises:block the rest of the SDK's public API uses (which doc tooling and IDEs also render specially).
How to fix
Move the prose sentence into a Raises: block, e.g.:
Raises:
RuntimeError: Another stdio_server() in this process has already
claimed fd 0 or fd 1 (only when serving the default streams).Impact and severity
Zero runtime impact — the behavior itself is deliberate, tested, and correct. This is purely a documentation-convention gap on the public API surface, so it should not block merge: nit.
No-Verification-Needed: docstring and docs prose only
There was a problem hiding this comment.
No new issues from this pass, but this is descriptor-level surgery in the core stdio transport and my earlier inline comments still stand unaddressed, so it needs a human review.
What was reviewed this pass:
- The reported per-session leak of the private wire descriptors (overlaps the open cubic P2): ruled out as a bug — leaving them open is a deliberate tradeoff (
closefd=False, per the code comment) so a worker thread still blocked in a read after exit cannot hit a recycled fd; the suggestedos.close(private_fd)after restore would reintroduce exactly that race. - The absence of a docs/migration.md entry: examined — the branch added an entry in
f74f608and deliberately removed it in52d9409, matching the PR description's no-migration rationale (only previously-broken behaviors change). Whether that call is right is a maintainer judgment, not an oversight. - The docs' "can't corrupt the stream" claim vs a
2>&1launch: examined — that launch shape keeps its v1 behavior and is named in the PR as a deliberate residual.
Extended reasoning...
Overview
The PR rewires stdio_server() to serve the protocol from private duplicates of fd 0/1 while diverting the standard descriptors (fd 0 to the null device, fd 1 to stderr), restoring both on exit, plus a Windows SetStdHandle rebind helper, extensive fd-level and end-to-end tests, and doc updates across four pages. It fixes the #671 Windows subprocess hang and the classic stray-print() wire corruption.
Security risks
No conventional security surface (no auth, crypto, or input parsing changes). The risk profile is correctness of process-global descriptor-table manipulation: a bug here can silently corrupt or steal the JSON-RPC wire for every stdio server built on this SDK. The claim/restore paths, the best-effort OSError fallback, and the Windows handle rebinding are all subtle and platform-sensitive.
Level of scrutiny
Maximum. This is the default transport for every local MCP server, the change deliberately alters process-global state (fd table, Win32 standard handle slots), and it introduces a new public failure mode (RuntimeError on concurrent transports). Prior review passes surfaced real design-level issues (a nested-transport wrapper-finalizer bug, a claim-window race) that drove the 52d9409 redesign; two of my findings from earlier today — the fallback path discarding the _claimed sentinel while still serving fd 0/1 in place, and the missing Raises: section on the public docstring — remain unaddressed with no commits since. That alone rules out approval.
Other factors
This run found no new bugs and refuted several candidates. Most usefully, the per-session descriptor 'leak' (raised by cubic as a P2 with a suggested os.close(private_fd) fix) is deliberate: the code comment explains closefd=False exists because a worker thread can still be blocked reading the private descriptor after exit, and closing it would let the fd number be recycled under that read — the suggested fix is unsafe. Realistic servers enter one stdio session per process, so the two retained descriptors are immaterial. The migration-entry omission is likewise deliberate (entry added in f74f608, removed in 52d9409, rationale in the PR description), though whether the changes count as 'breaking' under AGENTS.md is exactly the kind of judgment a maintainer should make. Test coverage is strong: fd-table contracts, failure injection, concurrency refusal, and real-subprocess regressions for both the Windows hang and the noisy-child case.
1 participant
stdio_server()now serves the protocol from private duplicates of stdin and stdout and repoints the standard descriptors away from the wire while it runs, restoring both on exit: fd 0 (and the Windows standard input handle) reads the null device, and fd 1 (and the standard output handle) writes to stderr. Subprocesses spawned by tool code therefore inherit the diversions instead of the protocol pipes, and a strayprint()lands in the client's log instead of corrupting the stream.Fixes #671.
Motivation and Context
stdin. The #671 hang is CPython gh-78961: Windows serializes operations on a synchronous pipe, a stdio server always has a blocking read pending on stdin, and a Python child that inherits that pipe freezes inside interpreter startup, before its first line of user code, until the pending read completes. That happens exactly when the next JSON-RPC message arrives, which is why hung tool calls "complete" once the client times out or sends another request. Skipping redirection entirely doesn't avoid it: Windows hands a console child the parent's standard handles even with
bInheritHandles=FALSE, so a plainsubprocess.run([...])hangs too. With fd 0 on the null device while serving, there is no shared pipe to block on. It also fixes the cross-platform variant: a child that reads its inherited stdin was consuming protocol bytes on any platform.stdout. The same inheritance corrupts the other direction: a child writing to its inherited stdout (or a stray
print()in handler code) writes straight into the JSON-RPC stream (the classic client-side symptom isUnexpected token ... is not valid JSON, e.g. #409). With fd 1 diverted to stderr while serving, that output shows up in the client's logs instead; every stdio client surveyed (this SDK, TypeScript, C#, Inspector, Claude Desktop, VS Code, Zed, ...) captures or passes through server stderr. This closes the documentedtransport:stdio:stream-puritydivergence, and matches where go-sdk is heading for its v2 (modelcontextprotocol/go-sdk#572).Why the descriptor level. Children inherit descriptors, not Python objects, so no
sys.stdout-level redirection can protect the wire from subprocesses. Node fixed its version of this inside libuv (uv_disable_stdio_inheritance()); Go's runtime hands children the null device for unset streams; Python gives a library neither, so the transport rearranges the table itself, using the same dup/dup2/restore mechanism as pytest'scapfd.The design refuses abnormal processes instead of repairing them. The claim engages only when the sys stream is backed by its real descriptor and fds 0-2 are all open (which makes it arithmetically impossible for the private duplicates to land in the standard range). Anything else (replaced streams, incomplete descriptor tables, a failed dup) is served in place exactly as v1 was. A second concurrent
stdio_server()on the same real streams raisesRuntimeErrorrather than contending for the wire.Who is affected. A survey of real public MCP servers found no code that works today and would break: of 16 servers whose tools spawn children, 11 are exposed to the hang/theft today (none redirect stdin) and are fixed by this change, 5 fully redirect and are unaffected, 0 break.
print()in stdio serving code is widespread and corrupts the wire today (e.g. PrefectHQ/fastmcp#3278, #1010); it now lands in the client's log. Realinput()-in-a-tool under stdio does not exist in the wild (it never worked; it now fails fast with EOF instead of hanging). Accordingly there is no migration entry: no user has anything to do. The workaround documentation #3079 added is removed: v2 no longer exhibits the hang (stdin=subprocess.DEVNULLstill works fine where users already pass it).How Has This Been Tested?
windows-latestrunners against currentmainwith an instrumented harness: the child's first line executes only when the next protocol message lands, on Python 3.10 through 3.14. With this change, the same unmodified repro returns in ~0.5 s.main(POSIX and Windows runners) and pass with this change.print(), post-pollution calls: all clean, all noise in the host-visible stderr), a Go SDK client doing the same, and this SDK's client against a TypeScript server.python server.pysessions with a pre-run banner print (drains to stderr, wire clean), post-run prints (stdout restored),python -u, and launches with stderr closed or merged.Breaking Changes
None requiring action. During a stdio session, handler code that read
sys.stdinnow sees EOF instead of racing the transport for protocol bytes, andprint()/sys.stdoutwrites reach stderr instead of the wire; both were broken behaviors before, not working ones. Injected-stream usage (stdio_server(stdin=..., stdout=...)) and environments where the sys streams aren't the real descriptors are served exactly as before.Types of changes
Checklist
Additional context
Known residuals, deliberately at v1 parity: output flushed to stdout before the transport enters still precedes the first frame, and a process launched with stderr merged into stdout (
2>&1) keeps its already-broken merged behavior. No surveyed host launches servers that way.AI disclosure
AI assistance was used to investigate, implement, and validate this change; I reviewed the result and take responsibility for it.
AI Disclaimer