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 12 commits into
Conversation
118422b to
1eec799
Compare
📚 Documentation preview
|
1eec799 to
e1d6e64
Compare
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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
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.
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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 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.
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.
A transport that falls back to serving the real stdin or stdout in place (incomplete descriptor table, or a claim that failed with OSError) now keeps its fd in the claimed set for its lifetime, so a second concurrent stdio_server() is refused in every shape rather than only after a successful diversion. The sentinel now means a transport owns the stream, not that the fd is currently diverted.
No-Verification-Needed: docs-only change
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
os.getppid() never changes on Windows, and the polled-fd pattern being migrated is POSIX-only to begin with. No-Verification-Needed: docs-only change
There was a problem hiding this comment.
Beyond the inline finding, this run also examined and ruled out two stdin-buffering concerns: protocol bytes buffered in sys.stdin.buffer being stranded when the claim engages (the transport never reads through the sys stream before serving, and os.dup shares the open file description, so no wire bytes are lost at claim time), and client bytes read ahead into the dead session's private buffer being invisible on the restored fd 0 (v1's own sys.stdin buffering read ahead the same way, so this is not a regression of this change). The new errlog open in test_lifecycle.py omitting encoding='utf-8' was also checked and is harmless — the assertion only looks for an ASCII marker.
Extended reasoning...
Bugs-found run: one nit is being posted inline (silent OSError swallowing on the best-effort claim/restore/diversion fallbacks), so the review verdict itself stays with the inline comments and human review — this PR rearranges the process descriptor table in the core stdio transport and is not approval-eligible. This note only records the additional candidate issues that finder agents raised and verifiers refuted this run, none of which had been discussed in the thread before, so a later pass does not re-explore them from scratch. It is informational, not a guarantee of correctness.
A failed exit-time restore is still swallowed so it never masks what ended the transport, but the fd now stays in the claimed set: a later stdio_server() in that process is refused instead of claiming the diversion and serving the null device as its wire.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
If a mid-claim OSError is followed by a rollback failure, fd is stuck on the diversion but the private duplicate still holds the wire, so the transport now keeps the claim and serves from the duplicate like a completed claim; the restore at exit gets another chance. Previously the claim was released while fd stayed diverted, letting a later stdio_server() claim the diversion as its wire.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The claim registry maps each standard descriptor to at most one owning transport; the wire duplicate is allocated where it cannot land in the standard range (F_DUPFD_CLOEXEC above fd 2 on POSIX) and recorded on the claim before the descriptor is moved; release restores with a single dup2 and deregisters only on success. Every failure, modeled or not, lands on the safe side: the claim is retained and later transports are refused rather than handed a diverted descriptor. Deleted by the same design: the roll-forward path (its premise, a reliably reportable dup2 outcome, does not exist on Windows), and the restore-time flush (user flush() side effects can destroy the wire; unflushed stray output now drains after the session instead). The design was validated through three adversarial verification passes; the write-up with invariants and accepted residues is on the PR.
Design write-up: the stream-claim state machineCommit 41b9faa implements the design below. It was produced by three adversarial verification rounds (each round: independent attacker agents per lens, every attack judged by a strict verifier, several confirmed empirically with runnable probes); each round shrank the machine. The frozen residues section lists every accepted limitation with its reachability argument. |
No-Verification-Needed: coverage pragmas only
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Matches the sibling errlog tests; the platform locale default could mojibake a failing child's stderr diagnostics on Windows. No-Verification-Needed: test-only encoding fix
There was a problem hiding this comment.
No findings on the current revision (the 41b9faa state-machine reshape through 3e5fa68), but this PR rearranges the process descriptor table inside the core stdio transport with platform-divergent semantics, so it warrants a human maintainer's review rather than a bot approval.
What was reviewed this run:
- The reshaped claim/release state machine in
src/mcp/server/stdio.py: acquire/divert/restore ordering, the best-effort fallback branches, and the refusal-over-release failure direction. - Candidates examined and ruled out: an inheritable Windows wire duplicate leaking the protocol pipe to children (the diverted slot no longer reaches the wire); the
_StreamClaim.fdfield being dead (cosmetic at most); a hole in fds 0-2 silently disabling isolation on Windows (_dup_above_stdguards it); and a bufferless replaced sys stream crashing_claim_fd(_is_backed_by_fdcatchesAttributeError). - The new fd-level tests and the end-to-end noisy-child/Windows-hang regression tests were read against the claimed contract.
Extended reasoning...
Overview
This PR makes stdio_server() serve the JSON-RPC wire from private duplicates of fd 0/1 while diverting the real descriptors (fd 0 to the null device, fd 1 to stderr), restoring both on exit — fixing the #671 Windows subprocess hang and the print()-corrupts-the-wire class of bugs at the descriptor level. It touches the core stdio transport (src/mcp/server/stdio.py), adds a Windows standard-handle rebind helper (src/mcp/os/win32/utilities.py), rewrites five docs pages, and adds substantial fd-level and end-to-end test coverage.
Security risks
No injection, auth, or data-exposure surface. The main risk class is availability/correctness: a wrong descriptor move could corrupt or destroy the protocol stream, leak the wire to child processes, or leave the process with broken standard streams after exit. The design addresses this with a single-authority claim registry whose failure direction is refusal (a failed restore keeps the claim and refuses later transports rather than handing them a diversion), and the CLOEXEC private duplicate keeps the wire out of children.
Level of scrutiny
Maximum — this is production-critical transport code executed by every stdio server, with behavior that differs across POSIX/Windows and across degraded descriptor tables. The current run of the bug hunting system found no bugs, and four finder candidates (Windows handle inheritance of the wire duplicate, a supposedly dead _StreamClaim.fd field, fd-hole handling on Windows, and a bufferless replaced sys stream) were adversarially examined and refuted. My prior runs' inline findings (silent degradation logging, the claim-table poisoning via an unprotected os.close, the test-file encoding nit) were all either fixed by the author (8c46858, 8ed5083, 41b9faa, 3e5fa68) or closed with documented frozen-design rationale. Nevertheless, the correctness argument rests on subtle platform semantics (UCRT _dup2 closing the target handle, Win32 standard-handle slots vs the CRT fd table) that only a human maintainer — ideally with the design write-up comment in hand — should sign off on.
Other factors
Test coverage is strong: fd-level tests pin the descriptor contract including failure injection for every fallback branch, and end-to-end tests spawn real servers whose tools spawn real children on both the Windows hang shape and the cross-platform noisy-child shape, each verified to fail on unpatched main. The author has been responsive through several review rounds and documented deliberate tradeoffs (silent best-effort degradation, the no-close invariant on the private duplicate, no flush in release) with reproducible rationale. The remaining open question is a design-level one — whether the SDK should be doing descriptor-table surgery at all — which is exactly the kind of call a human maintainer must make.
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 claim is a small state machine with one safe failure direction. A registry maps each standard descriptor to at most one owning transport; a second concurrent
stdio_server()raisesRuntimeErrorrather than contending for the wire. The wire duplicate is allocated where it cannot land in the standard range (F_DUPFD_CLOEXECabove fd 2 on POSIX) and recorded before the descriptor is ever moved; release restores with a singledup2and deregisters only on success. Every failure, modeled or not, therefore lands on the safe side: the claim is retained and later transports are refused, never handed a diverted descriptor. Failures to duplicate or divert degrade to serving the streams in place exactly as v1. The design was hardened through three adversarial verification rounds (about fifty attack scenarios, several confirmed and fixed, the rest refuted); the full state machine, invariants, and accepted residues are in the design write-up comment on this PR.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