ouyangbo/EvoScientist-Multi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(mcp): give stdio subprocess a real stderr fd under redirected streams (#423)

Browse Source 
* fix(mcp): give stdio subprocess a real stderr fd under redirected streams (#418)

On Windows the Textual TUI redirects sys.stderr to an in-memory capture
(textual.app._PrintCapture) whose fileno() returns -1. The MCP SDK forwards
that stderr to stdio server subprocesses via subprocess.Popen(stderr=...),
and Popen rejects the invalid handle with OSError: [Errno 9] Bad file
descriptor — so only stdio servers fail to load (HTTP/SSE are unaffected).

Wrap mcp.client.stdio.stdio_client so that, whenever the configured errlog
has no usable fileno, it falls back to sys.__stderr__ (or os.devnull in GUI
hosts). Idempotent, no-op when the SDK is absent, warns if the SDK renames
stdio_client. Adds 9 regression tests and a troubleshooting note.

* fix(mcp): validate live fd and close fallback errlog after stdio session

Address CodeRabbit review on #423:
- _stdio_errlog_is_usable now os.fstat()s the fd to reject closed streams
  that still report their former positive fileno (prevents a deferred
  [Errno 9] from subprocess.Popen).
- The stdio_client wrapper owns the devnull fallback it allocates and
  closes it once the session exits, so repeated MCP reloads no longer leak
  file descriptors. Caller-provided usable errlogs pass through untouched.
- Tests cover the closed-fd case, the fd-leak/closure invariant, and
  confirm langchain-mcp-adapters binds the patched stdio_client.

* fix(mcp): rebind adapter stdio_client, forward errlog by kw, harden tests

Address CodeRabbit round-2 review on #423:
- The patch now also rebinds langchain_mcp_adapters.sessions.stdio_client,
  which the adapter captures via a 'from' import at module load — so the
  wrapped function reaches the adapter regardless of import order.
- errlog is forwarded to the SDK by keyword (original(server, *args,
  errlog=errlog, **kwargs)) so a future SDK inserting a positional
  parameter before errlog can't mis-bind the fallback.
- The fallback stream is now allocated inside the async context manager,
  so it is closed on session exit even if the CM is constructed but never
  entered (narrower fd-leak path).
- test_closed_fd_rejected now reaches the os.fstat branch (stale positive
  fd stub) instead of the ValueError path; test_adapter_binds_patched_stdio_client
  documents and asserts the import-order-independent rebind.

* fix(mcp): close fallback errlog when stdio_client construction fails

Address CodeRabbit round-3 review on #423: move the original(server, *args,
errlog=errlog, **kwargs) construction inside the try block so a failure
during subprocess/client setup still reaches the finally and closes the
wrapper-owned os.devnull stream. Added test_fallback_closed_when_construction_fails
covering the path.

* refactor(mcp): track fallback ownership via (stream, opened_by_us)

Address din0s review on #423:
- _safe_stdio_errlog() now returns (stream, opened_by_us); the wrapper closes
  the fallback only when opened_by_us is True, instead of inferring ownership
  from needs_fallback + an identity check against sys.__stderr__. Simpler and
  less likely to regress.
- Removed dead try/finally in test_closed_fd_rejected.
- Added test_wrapped_stdio_client_swaps_explicit_bad_errlog covering the
  'not _stdio_errlog_is_usable(errlog)' branch (explicit bad errlog, not the
  default sentinel).
- Updated test_safe_errlog_returns_usable_stream for the tuple return.

---------

Co-authored-by: Xi Zhang <106144707+X-iZhang@users.noreply.github.com>
This commit is contained in:
houren Antony
2026-08-14 01:39:45 +08:00
committed by GitHub
parent 1f3e8f57a3
commit 932c934485
3 changed files with 524 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
EvoScientist/mcp/README.md
+9
View File
@@ -319,6 +319,15 @@ stdio server fails to start — install Node.js and `npx`, or replace `npx` with
</details>
<details>
<summary><strong>Windows: stdio server fails with <code>[Errno 9] Bad file descriptor</code></strong></summary>
On Windows, the TUI redirects `sys.stderr` to an in-memory capture whose `fileno()` is not a real OS handle. The MCP SDK forwards that `stderr` to the stdio server subprocess, and `subprocess.Popen` rejects the invalid handle with `OSError: [Errno 9] Bad file descriptor` — so only stdio servers fail to load (HTTP/SSE servers are unaffected).
EvoScientist wraps the SDK's stdio client so that, whenever the configured `stderr` has no usable file descriptor, it falls back to the original console handle (`sys.__stderr__`, or `os.devnull` in GUI hosts). If you still see this error, run from a real console (not `pythonw.exe`) and check the server's own startup output.
</details>
<details>
<summary><strong><code>--env-ref</code> or <code>${VAR}</code> not resolving</strong></summary>
EvoScientist/mcp/client.py
+174
View File
@@ -14,6 +14,8 @@ import re
import shutil
import sys
from collections.abc import Callable
from contextlib import asynccontextmanager
from functools import wraps
from pathlib import Path
from typing import Any
@@ -98,6 +100,178 @@ def _patch_mcp_windows_command_resolver() -> None:
_patch_mcp_windows_command_resolver()
# =============================================================================
# Windows MCP SDK patch — give stdio subprocesses a real stderr file descriptor
# =============================================================================
#
# ``mcp.client.stdio.stdio_client`` forwards the parent process's ``stderr``
# (``errlog``, defaulting to ``sys.stderr``) to the MCP server subprocess via
# ``anyio.open_process`` → ``subprocess.Popen(stderr=...)``. ``Popen`` resolves
# that to an OS handle by calling ``errlog.fileno()``.
#
# Under the Textual TUI (and any non-console host), ``sys.stderr`` is
# redirected to ``textual.app._PrintCapture``, whose ``fileno()`` returns
# ``-1``. ``subprocess.Popen`` dutifully converts fd ``-1`` into an invalid
# Windows handle and the child inherits a broken stderr pipe — every stdio
# MCP server then fails to spawn with ``OSError: [Errno 9] Bad file
# descriptor``. HTTP/SSE servers are unaffected (no subprocess), which is why
# only the stdio server (e.g. arxiv) shows up as failed in the loader. See
# issue #418; the same class of bug is tracked upstream as
# modelcontextprotocol/python-sdk#1103.
#
# We can't pass ``errlog`` through ``langchain-mcp-adapters`` (it calls
# ``stdio_client(server_params)`` with no ``errlog``), and the SDK's default
# is bound once at import time — which may itself already capture a
# redirected ``sys.stderr``. So we wrap ``stdio_client`` to swap in a safe
# ``errlog`` at call time whenever the configured one has no usable fileno.
# The substitute is ``sys.__stderr__`` (the real console handle) when
# available, otherwise a discarded ``os.devnull`` handle.
def _stdio_errlog_is_usable(errlog: object) -> bool:
"""Return ``True`` if *errlog* can back a subprocess ``stderr`` pipe.
A usable errlog exposes a ``fileno()`` that resolves to a live OS file
descriptor. Textual's ``_PrintCapture`` and similar redirected streams
return ``-1`` (or raise), so they are rejected here. A closed stream may
still report its former (positive) fd, so we additionally ``os.fstat``
the descriptor to confirm it is still open.
"""
fileno = getattr(errlog, "fileno", None)
if not callable(fileno):
return False
try:
fd = fileno()
except Exception:
return False
if not isinstance(fd, int) or fd < 0:
return False
try:
os.fstat(fd)
except (OSError, OverflowError):
return False
return True
def _safe_stdio_errlog() -> tuple[Any, bool]:
"""Return a ``(stream, opened_by_us)`` pair for a usable stderr.
Prefers ``sys.__stderr__`` (the original console handle, so the server's
diagnostic output still lands where the user expects — note
``sys.__stderr__`` is the process's original handle and stays usable even
while the Textual TUI controls the screen, since Textual only redirects
``sys.stderr``). Falls back to an ``os.devnull`` handle when even
``__stderr__`` is unavailable (e.g. in a GUI/pythonw host with no console).
The second element is ``True`` when *we* allocated the stream (the
``os.devnull`` case) and therefore own its lifecycle; it is ``False`` for
``sys.__stderr__``, which is process-owned and must never be closed here.
Callers use that flag to decide whether to close the stream after the
stdio session exits.
"""
dunder = getattr(sys, "__stderr__", None)
if dunder is not None and _stdio_errlog_is_usable(dunder):
return dunder, False
# Last resort: discard the server's stderr so the spawn still succeeds.
return open(os.devnull, "w", encoding="utf-8", errors="replace"), True
def _patch_mcp_stdio_errlog_safe() -> None:
"""Wrap the SDK's ``stdio_client`` to guarantee a usable ``errlog``.
Idempotent. A no-op when the MCP SDK is absent (optional dependency).
When the caller already supplied a usable ``errlog`` it is forwarded
unchanged; only the unsafe default (redirected ``sys.stderr``) is
replaced. This keeps the patch transparent for embedders that pass their
own ``errlog`` explicitly.
The wrapper is installed on both ``mcp.client.stdio.stdio_client`` and
``langchain_mcp_adapters.sessions.stdio_client``: the adapter binds the
name via a ``from … import`` at its module load, so updating only the
SDK module would leave an already-imported adapter pointing at the
unwrapped function.
"""
try:
import mcp.client.stdio as _stdio_mod
except ImportError:
return # MCP SDK not installed — nothing to patch.
original = getattr(_stdio_mod, "stdio_client", None)
if original is None:
# The SDK renamed/removed stdio_client — nothing to wrap. Log so a
# future SDK refactor doesn't silently drop this guard.
logger.warning(
"MCP SDK layout changed: mcp.client.stdio.stdio_client is missing; "
"the Windows stdio errlog safety patch was NOT applied. MCP stdio "
"tool loading may fail with [Errno 9] under a redirected stderr."
)
return
if getattr(original, "_evosci_errlog_safe", False):
return # Already patched.
@wraps(original)
def _stdio_client_safe(server: Any, errlog: Any = ..., *args: Any, **kwargs: Any):
# When the caller didn't supply a usable errlog we allocate a fallback
# stream (sys.__stderr__ or os.devnull). The SDK never closes a
# caller-provided errlog, so a devnull fallback would leak its fd on
# every MCP reload. We allocate the fallback inside the async context
# manager below so it is closed on exit — and, if the CM is discarded
# before being entered, Python finalises the async generator and runs
# the same ``finally``. ``errlog`` is forwarded by keyword so a future
# SDK that inserts a positional parameter before it can't mis-bind it.
caller_errlog = errlog
needs_fallback = errlog is ... or not _stdio_errlog_is_usable(errlog)
@asynccontextmanager
async def _close_owned_errlog():
if needs_fallback:
# ``opened_by_us`` is True only for the os.devnull case;
# sys.__stderr__ is process-owned and must not be closed.
errlog, opened_by_us = _safe_stdio_errlog()
else:
errlog, opened_by_us = caller_errlog, False
try:
# Construct inside the try so a failure here still reaches the
# finally and closes a wrapper-owned fallback stream.
# Forward by keyword: robust against future SDK signature changes
# that insert a positional parameter before ``errlog``.
cm = original(server, *args, errlog=errlog, **kwargs)
async with cm as streams:
yield streams
finally:
if opened_by_us:
close = getattr(errlog, "close", None)
if callable(close):
try:
close()
except Exception:
logger.debug(
"Failed to close fallback stdio errlog", exc_info=True
)
return _close_owned_errlog()
_stdio_client_safe._evosci_errlog_safe = True # type: ignore[attr-defined]
_stdio_mod.stdio_client = _stdio_client_safe
# langchain-mcp-adapters binds stdio_client via a ``from`` import at its
# module load, so a pre-imported adapter keeps the unwrapped reference.
# Re-bind it too (best-effort; ignore if the layout differs).
try:
import langchain_mcp_adapters.sessions as _adapter_sessions
if getattr(_adapter_sessions, "stdio_client", None) is original:
_adapter_sessions.stdio_client = _stdio_client_safe
except ImportError:
pass # Adapter not installed — nothing extra to rebind.
logger.debug("Applied MCP stdio errlog safety patch")
_patch_mcp_stdio_errlog_safe()
# =============================================================================
# Constants
# =============================================================================