Skip to content

Isolate the stdio server's stdin and stdout from handler subprocesses#3117

Open
maxisbey wants to merge 12 commits into
mainfrom
stdio-stdin-isolation
Open

Isolate the stdio server's stdin and stdout from handler subprocesses#3117
maxisbey wants to merge 12 commits into
mainfrom
stdio-stdin-isolation

Conversation

@maxisbey

@maxisbey maxisbey commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

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 stray print() 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 plain subprocess.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 is Unexpected 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 documented transport:stdio:stream-purity divergence, 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's capfd.

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() raises RuntimeError rather than contending for the wire. The wire duplicate is allocated where it cannot land in the standard range (F_DUPFD_CLOEXEC above fd 2 on POSIX) and recorded before the descriptor is ever moved; release restores with a single dup2 and 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. Real input()-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.DEVNULL still works fine where users already pass it).

How Has This Been Tested?

  • Reproduced MCP Tool execution hangs indefinitely in stdio mode when calling external Python scripts #671 on windows-latest runners against current main with 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.
  • fd-level tests pin the descriptor-table contract: the wire moves to private descriptors, fd 0 reads EOF, fd 1 diverts to stderr (asserted by reading a planted stderr pipe), frames stay pure on the planted wire pipes, both descriptors restore on exit; plus the in-place path for incomplete descriptor tables, failure injection for the best-effort fallback and restore paths, and the concurrent-claim refusal.
  • End-to-end regression tests spawn a real server whose tool spawns a real child: the Windows stdin-hang shapes (piped and bare) and a cross-platform noisy child whose junk line must arrive in the server's stderr, never the wire. Each verified to fail on unpatched main (POSIX and Windows runners) and pass with this change.
  • Live interop against other SDKs: a TypeScript SDK (1.29.0) client driving this server through hostile traffic (noisy child, direct 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.
  • Shell-level probes: piped python server.py sessions 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.
  • Full suite + 100% coverage gate, pyright, ruff, and the strict docs build pass on ubuntu and windows runners.

Breaking Changes

None requiring action. During a stdio session, handler code that read sys.stdin now sees EOF instead of racing the transport for protocol bytes, and print()/sys.stdout writes 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

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

@maxisbey
maxisbey marked this pull request as ready for review July 16, 2026 19:25
@maxisbey
maxisbey force-pushed the stdio-stdin-isolation branch from 118422b to 1eec799 Compare July 16, 2026 19:26
@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

📚 Documentation preview

Preview https://pr-3117.mcp-python-docs.pages.dev
Deployment https://d193db9b.mcp-python-docs.pages.dev
Commit 3e5fa68
Triggered by @maxisbey
Updated 2026-07-21 21:35:02 UTC

@maxisbey
maxisbey force-pushed the stdio-stdin-isolation branch from 1eec799 to e1d6e64 Compare July 16, 2026 19:29

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread docs_src/troubleshooting/tutorial009.py Outdated
Comment thread docs/migration.md Outdated
Comment thread docs/run/index.md Outdated
Comment thread src/mcp/server/stdio.py Outdated
@maxisbey
maxisbey force-pushed the stdio-stdin-isolation branch from e1d6e64 to bf24e5e Compare July 16, 2026 19:40
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.
@maxisbey
maxisbey force-pushed the stdio-stdin-isolation branch from bf24e5e to 42ef19a Compare July 16, 2026 19:44
Comment thread src/mcp/server/stdio.py Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 maxisbey changed the title Isolate the stdio server's stdin from handler subprocesses Isolate the stdio server's stdin and stdout from handler subprocesses Jul 16, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 12 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/mcp/server/stdio.py Outdated
Comment thread docs/handlers/logging.md
Comment thread docs/run/index.md Outdated
Comment thread docs/migration.md Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/mcp/server/stdio.py
No-Verification-Needed: docstring and docs prose only

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 suggested os.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 f74f608 and deliberately removed it in 52d9409, 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>&1 launch: 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.

maxisbey added 2 commits July 21, 2026 14:16
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

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread docs/migration.md Outdated
Comment thread src/mcp/server/stdio.py Outdated
Comment thread src/mcp/server/stdio.py Outdated
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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/mcp/server/stdio.py
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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

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

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread src/mcp/server/stdio.py Outdated
Comment thread src/mcp/server/stdio.py Outdated
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.
@maxisbey

Copy link
Copy Markdown
Contributor Author

Design write-up: the stream-claim state machine

Commit 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.

# Stdio stream claims (FINAL: v3 + pass-3 corrections)
# Arc: v1 (9 attacks: scattered guards) -> v2 (9 attacks: inspection mechanisms)
# -> v3 (8 attacks: over-broad universal claims). The machine held in all three
# passes; the final corrections shrink claims, not add mechanism.

## Problem and domain

On stdio transport, fd 0/1 are the protocol. While serving: (R1) keep handlers and
children off the wire; (R2) restore on exit; (R3) an abnormal process degrades, never
corrupts; (R4) refuse a second concurrent transport.

**Domain.** The machine governs the process's standard descriptors, fd 0 and fd 1.
A sys stream not backed by its standard descriptor is user-owned: served as v1 did,
no claims made about it, including R4.

**Domain axiom (pass 3).** The process does not close or reallocate fds 0-2 outside
the machine while a claim exists. User space has no atomic fd transactions: no
conditional restore, no slot reservation. A process that mutates its own standard
descriptors under a live claim defeats every fd-managing library the same way
(pytest capfd, wurlitzer, daemon(3)) and defeated v1 worse: there, the same events
put protocol frames straight into recycled app files mid-session. Under such
mutation the machine still guarantees only: it closes nothing it did not create,
and claims hold (successors refused).

## The two core facts

**F1.** `dup2(p, fd)` is the correct restore action in every reachable fd state:
fd on the wire (no-op), fd on a diversion (restores), fd closed (reopens; POSIX and
UCRT both permit a closed target). Therefore no transition ever needs to know
whether a previous dup2 landed, half-landed, or closed its target. There is no
rollback logic, no roll-forward logic, and no "did it land" record.

**F2 (corrected in pass 3).** Once p exists, p carries the wire and is the restore
source. But when the divert did NOT land, the transport must serve `stream.buffer`,
not p: fd still carries the wire, and two independent buffered writers on one pipe
tear frames mid-line above PIPE_BUF (measured: 26/8000 at 6 KiB), while the shared
buffer serializes exactly as v1 (0/30000). Serve-from-p is correct only when the
divert landed; p is kept regardless, as the exit restore source.

## States and registry

    _claims: dict[int, _StreamClaim]      # single authority; guarded by _claims_lock
    _StreamClaim: fd, private_fd: int | None

| State | Representation | Serve from | Meaning |
|---|---|---|---|
| FREE | not registered | - | no owner |
| OWNED, no wire dup | claim, `private_fd = None` | `stream.buffer` | owned; no descriptor was created; fd untouched |
| OWNED, wire dup | claim, `private_fd = p` | `fdopen(p)` | owned; p carries the wire (F2); fd is wherever the best-effort diversion left it |

`private_fd` is recorded on the claim at the instant p is created, before fd is
ever touched — so "a descriptor was moved" implies "p is recorded" at every
interruption point.

## Transitions

acquire(fd, stream):

    1. stream not backed by fd        -> serve stream.buffer, no claim   (outside domain)
    2. lock: registered? raise RuntimeError; else insert claim(p=None)
    3. p := dup_above_std(fd); claim.private_fd = p
         on OSError: serve stream.buffer                                  (owned, no wire dup)
    4. divert: d := diversion(); dup2(d, fd)
         OSError before dup2 lands: serve stream.buffer (fd still carries the wire)
    5. dup2 landed: close(d) and rebind(fd) best-effort (residue 1; they cannot
       un-land the divert); serve fdopen(p, closefd=False)

release(claim):

    if claim.private_fd is None: deregister                               (zero syscalls)
    elif dup2(p, fd) succeeds:   best-effort rebind; deregister
    else:                        stay registered                          (still owned)

`dup_above_std`: on POSIX, `fcntl(fd, F_DUPFD_CLOEXEC, 3)` — atomically cannot land
in the standard range, closing the low-landing window entirely. On Windows, `os.dup`
plus release-and-fail if the result is <= 2 (residue 6). The diversion descriptor may
transit a low slot: its content is the null device or a stderr duplicate, so a
concurrent writer hitting that slot cannot corrupt the wire; it is closed in step 4.

## Invariants (all inductive over the interruption points)

- **I1** `_claims` is mutated only by acquire step 2 and release's deregister, locked.
- **I2** If fd was moved by this module, a claim with `private_fd = p` is registered
  (step 4 runs only after p is recorded in step 3).
- **I3** While `private_fd = p` is registered, p carries the wire and is never closed:
  the module contains no `close(p)` on any path.
- **I4** acquire on a registered fd raises: at most one owner per standard descriptor.
- **I5** release executes no user code and at most two syscalls; failure leaves state
  unchanged; deregistration follows a successful restore only.

## One-direction principle

Under-claiming (fd moved or wire served while FREE) is silent corruption;
over-claiming is loud refusal. Every interruption point in acquire and release —
including async KeyboardInterrupt at any bytecode boundary — lands over-claim:
insertion precedes every mutation, deregistration follows the successful restore,
and p is recorded before fd is moved. The strongest stranding (abandoned generator,
KI mid-release) leaves: claim registered, wire at p or at fd, successors refused.
Nothing in any stranded state is corrupted or destroyed.

## Proof obligations

- **P1** Every claim returned by acquire reaches release at most once (single
  `finally`; asyncgen frames cannot re-run); zero-run paths land over-claim (above).
- **P2** No interleaving serves or moves a standard descriptor while FREE: steps 3-5
  run only after step 2's atomic insert; a concurrent acquire hits step 2's raise.
- **P3** The wire is always carried by fd or p: p is a duplicate made before fd is
  moved (I2), never closed while registered (I3), and release only deregisters after
  dup2(p, fd) re-points fd at the wire (F1). No path destroys both carriers.
- **P4** Wire duplicates never occupy the standard range: atomic on POSIX (F_DUPFD
  minfd), checked-and-released on Windows (residue 6). Diversion transients are
  harmless by content.

## Frozen residues

1. Close/fdopen failures on fresh valid duplicates: unmodeled; land over-claim.
2. Pre-serve stdout output; stderr merged at launch: v1 parity.
3. One or two descriptors retained per session; stranded-reader close hazard.
4. No logging from claim paths.
5. Output buffered in sys streams at exit drains to the restored wire after the
   session (release executes no user code, so it cannot flush user wrappers; the
   in-session diversion covers flushed output).
6. Windows lacks an atomic minfd dup: with a standard slot closed at launch, the
   wire duplicate transits it for the allocate-inspect-release window, and an async
   KeyboardInterrupt inside that one-bytecode window can strand it there. POSIX is
   immune (F_DUPFD).
7. A failed divert or failed dup serves in place exactly as v1 (fd exposure = v1).
8. RuntimeError on concurrent claim is programmer error; stated in prose in the
   public docstring, not as a caller-facing `Raises:` contract.
9. Epoch overlap through retained descriptors: a session torn down with a reader or
   writer abandoned mid-kernel-I/O (second Ctrl-C class) leaves that I/O armed on p;
   a successor session on the same pipes can lose its first inbound message to the
   stale reader or receive a stale frame splice. The alternative (closing p) is the
   recycled-fd corruption residue 3 exists to prevent; no user-space mechanism can
   cancel blocked pipe I/O without closing. Successor-after-abandoned-teardown is
   accepted as degraded.

## Review contract

An attack names a concrete executable event sequence, in-domain, ending in a state
violating I1-I5 or a transition's stated result, or a proof obligation that does not
discharge. Restating a frozen residue, wording feedback, and suggestions are invalid.

AI Disclaimer

No-Verification-Needed: coverage pragmas only

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread src/mcp/server/stdio.py
Comment thread src/mcp/server/stdio.py
Comment thread src/mcp/server/stdio.py
Comment thread tests/transports/stdio/test_lifecycle.py
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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.fd field being dead (cosmetic at most); a hole in fds 0-2 silently disabling isolation on Windows (_dup_above_std guards it); and a bufferless replaced sys stream crashing _claim_fd (_is_backed_by_fd catches AttributeError).
  • 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP Tool execution hangs indefinitely in stdio mode when calling external Python scripts

1 participant