Skip to content

Commit c900b53

Browse files
committed
Forward server stderr when errlog has no file descriptor
stdio_client hands errlog to the child as an inherited file descriptor. Writers that have none (Jupyter's ipykernel stream, io.StringIO) cannot be inherited, so spawning either raised io.UnsupportedOperation or silently sent the server's diagnostics somewhere the caller never sees. Detect that case and spawn with a stderr pipe instead, forwarding it into errlog from a reader task. Shutdown drains the forwarder after the server dies so the last diagnostics still land. Writers that do own a descriptor keep the existing inherit path untouched, including ipykernel once fd capture is on. Fixes #156
1 parent a4f4ccd commit c900b53

4 files changed

Lines changed: 200 additions & 7 deletions

File tree

src/mcp/client/stdio.py

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
import logging
1212
import os
13+
import subprocess
1314
import sys
1415
from collections.abc import AsyncGenerator
1516
from contextlib import asynccontextmanager, suppress
@@ -68,6 +69,9 @@
6869
# Time for the writer to flush accepted messages before stdin closes.
6970
_WRITER_FLUSH_TIMEOUT = 0.5
7071

72+
# Time for the forwarder to drain the dead server's remaining stderr into errlog.
73+
_STDERR_DRAIN_TIMEOUT = 0.5
74+
7175
# How often to poll returncode while waiting for the process to die.
7276
_EXIT_POLL_INTERVAL = 0.01
7377

@@ -122,11 +126,16 @@ async def stdio_client(
122126
"""
123127
command = _get_executable_command(server.command)
124128

129+
# A child process inherits stderr as an OS file descriptor. Writer objects that
130+
# have none (Jupyter's ipykernel stream, io.StringIO) cannot be inherited, so the
131+
# server's stderr is piped back and forwarded into errlog by a reader task instead.
132+
forward_stderr = _lacks_file_descriptor(errlog)
133+
125134
process = await _create_platform_compatible_process(
126135
command=command,
127136
args=server.args,
128137
env=get_default_environment() | (server.env or {}),
129-
errlog=errlog,
138+
errlog=subprocess.PIPE if forward_stderr else errlog,
130139
cwd=server.cwd,
131140
)
132141

@@ -137,6 +146,7 @@ async def stdio_client(
137146

138147
shutting_down = False
139148
writer_done = anyio.Event()
149+
stderr_done = anyio.Event()
140150

141151
async def stdout_reader() -> None:
142152
assert process.stdout, "Opened process is missing stdout"
@@ -165,6 +175,30 @@ async def stdout_reader() -> None:
165175
if not shutting_down:
166176
logger.exception("Reading from the MCP server's stdout failed mid-session")
167177

178+
async def stderr_reader() -> None:
179+
"""Forwards the server's piped stderr into errlog.
180+
181+
Only runs when errlog could not be inherited by the child; keeps the
182+
server's diagnostics visible where a bare fd hand-off would have lost them.
183+
"""
184+
assert process.stderr, "Piped stderr is missing from the opened process"
185+
186+
stderr = TextReceiveStream(process.stderr, encoding=server.encoding, errors="replace")
187+
try:
188+
async for chunk in stderr:
189+
errlog.write(chunk)
190+
errlog.flush()
191+
except (anyio.ClosedResourceError, anyio.BrokenResourceError, ConnectionError):
192+
pass # the pipe went away with the process; nothing left to forward
193+
except ValueError:
194+
# errlog was closed under us (a notebook cell finishing, a StringIO
195+
# released). The server's own traffic must not fail over a lost log sink.
196+
logger.debug("Stopped forwarding the MCP server's stderr: the log stream was closed")
197+
finally:
198+
# Reaching EOF means the server's last diagnostics are in errlog, which
199+
# is what shutdown waits on before cancelling this task.
200+
stderr_done.set()
201+
168202
async def stdin_writer() -> None:
169203
assert process.stdin, "Opened process is missing stdin"
170204

@@ -193,13 +227,20 @@ async def shutdown() -> None:
193227
if flush_scope.cancelled_caught:
194228
await anyio.lowlevel.cancel_shielded_checkpoint() # resync coverage on 3.11 (gh-106749)
195229
await _stop_server_process(process)
230+
# The server is dead, so its stderr pipe is at EOF with at most a buffer left;
231+
# let the forwarder finish it before the task group's cancel takes the task out.
232+
if forward_stderr:
233+
with anyio.move_on_after(_STDERR_DRAIN_TIMEOUT):
234+
await stderr_done.wait()
196235
await _aclose_all(read_stream, write_stream, read_stream_writer, write_stream_reader)
197236
# One pass so unblocked tasks exit via their except paths before the cancel.
198237
await anyio.lowlevel.checkpoint()
199238

200239
async with anyio.create_task_group() as tg:
201240
tg.start_soon(stdout_reader)
202241
tg.start_soon(stdin_writer)
242+
if forward_stderr:
243+
tg.start_soon(stderr_reader)
203244
try:
204245
yield read_stream, write_stream
205246
finally:
@@ -317,6 +358,22 @@ def _close_subprocess_transport(process: ServerProcess) -> None:
317358
close()
318359

319360

361+
def _lacks_file_descriptor(errlog: TextIO) -> bool:
362+
"""Reports whether errlog has no OS file descriptor for a child to inherit.
363+
364+
Jupyter's ipykernel stream and io.StringIO answer fileno() with an error;
365+
real files, pipes and terminals return one. ipykernel does expose a
366+
descriptor once fd capture is on, and that path already reaches the
367+
notebook, so inheriting it stays correct.
368+
"""
369+
try:
370+
errlog.fileno()
371+
except (AttributeError, OSError, ValueError):
372+
# io.UnsupportedOperation derives from OSError and ValueError.
373+
return True
374+
return False
375+
376+
320377
def _get_executable_command(command: str) -> str:
321378
"""Normalizes the command for the current platform."""
322379
if sys.platform == "win32": # pragma: no cover
@@ -329,7 +386,7 @@ async def _create_platform_compatible_process(
329386
command: str,
330387
args: list[str],
331388
env: dict[str, str] | None = None,
332-
errlog: TextIO = sys.stderr,
389+
errlog: TextIO | int = sys.stderr,
333390
cwd: Path | str | None = None,
334391
) -> ServerProcess:
335392
"""Spawns the server in its own kill scope.

src/mcp/os/win32/utilities.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,12 @@ def __init__(self, popen_obj: subprocess.Popen[bytes]) -> None:
9292
self.popen: subprocess.Popen[bytes] = popen_obj
9393
stdin = popen_obj.stdin
9494
stdout = popen_obj.stdout
95+
stderr = popen_obj.stderr
9596

9697
self.stdin = FileWriteStream(cast(BinaryIO, stdin)) if stdin else None
9798
self.stdout = FileReadStream(cast(BinaryIO, stdout)) if stdout else None
99+
# Only set when the spawn asked for a stderr pipe; inherited stderr leaves it None.
100+
self.stderr = FileReadStream(cast(BinaryIO, stderr)) if stderr else None
98101

99102
async def wait(self) -> int:
100103
"""Waits for exit by polling the Popen.
@@ -137,7 +140,7 @@ async def create_windows_process(
137140
command: str,
138141
args: list[str],
139142
env: dict[str, str] | None = None,
140-
errlog: TextIO | None = sys.stderr,
143+
errlog: TextIO | int | None = sys.stderr,
141144
cwd: Path | str | None = None,
142145
) -> Process | FallbackProcess:
143146
"""Creates a subprocess with Job Object support for tree termination.
@@ -177,7 +180,7 @@ async def _create_windows_fallback_process(
177180
command: str,
178181
args: list[str],
179182
env: dict[str, str] | None = None,
180-
errlog: TextIO | None = sys.stderr,
183+
errlog: TextIO | int | None = sys.stderr,
181184
cwd: Path | str | None = None,
182185
) -> FallbackProcess:
183186
"""Spawns via subprocess.Popen and wraps it in FallbackProcess."""

tests/client/test_stdio.py

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import sys
1717
from collections.abc import Callable
1818
from contextlib import AsyncExitStack, suppress
19+
from io import StringIO
1920
from pathlib import Path
2021
from typing import TextIO, cast
2122

@@ -127,6 +128,10 @@ async def aclose(self) -> None:
127128
# Real async closes yield; keeps the fake honest and shutdown scheduling realistic.
128129
await anyio.lowlevel.checkpoint()
129130

131+
def close(self) -> None:
132+
"""Release the read end, as the kernel does when the process's pipe goes away."""
133+
self._inner.close()
134+
130135

131136
class FakeProcess:
132137
"""In-memory stand-in for the spawned server process.
@@ -145,7 +150,15 @@ def __init__(
145150
stdout_eof_error: Exception | None = None,
146151
stdout_aclose_error: Exception | None = None,
147152
on_stdout_receive: Callable[[], None] | None = None,
153+
stderr_eof_error: Exception | None = None,
148154
) -> None:
155+
self._stderr_send, stderr_receive = anyio.create_memory_object_stream[bytes](math.inf)
156+
# Only read when errlog has no descriptor to inherit, so the client pipes stderr.
157+
self.stderr = _FakeStdout(
158+
stderr_receive,
159+
eof_error=stderr_eof_error,
160+
on_receive=lambda: None,
161+
)
149162
self._stdout_send, stdout_receive = anyio.create_memory_object_stream[bytes](math.inf)
150163
self.stdout = _FakeStdout(
151164
stdout_receive,
@@ -178,10 +191,24 @@ def close_stdout(self) -> None:
178191
"""End the fake process's stdout, as the kernel does when it dies."""
179192
self._stdout_send.close()
180193

194+
async def feed_stderr(self, data: bytes) -> None:
195+
"""Make `data` readable on the fake process's stderr."""
196+
await self._stderr_send.send(data)
197+
198+
def close_stderr(self) -> None:
199+
"""End the fake process's stderr, as the kernel does when it dies.
200+
201+
Closes both ends: unlike stdout, stderr goes unread whenever errlog owns a
202+
descriptor, so nothing else would ever release the read end.
203+
"""
204+
self._stderr_send.close()
205+
self.stderr.close()
206+
181207
def exit(self, code: int = 0) -> None:
182-
"""Die: set the exit code and EOF stdout, as the kernel does."""
208+
"""Die: set the exit code and EOF both output pipes, as the kernel does."""
183209
self.returncode = code
184210
self.close_stdout()
211+
self.close_stderr()
185212

186213
def pending_stdout_chunks(self) -> int:
187214
"""How many fed chunks the client has not yet pulled off the fake stdout."""
@@ -975,6 +1002,71 @@ async def stubborn_terminate(proc: FakeProcess) -> None:
9751002
# The fake "survived", so nothing ever EOF'd its stdout pipe; release it here
9761003
# or its GC-time ResourceWarning would fail a later test.
9771004
process.close_stdout()
1005+
process.close_stderr()
1006+
1007+
1008+
class _UnwritableErrlog(StringIO):
1009+
"""A log sink that rejects writes, as a closed notebook cell's stream does."""
1010+
1011+
def __init__(self) -> None:
1012+
super().__init__()
1013+
self.attempted = anyio.Event()
1014+
1015+
def write(self, s: str, /) -> int:
1016+
self.attempted.set()
1017+
raise ValueError("I/O operation on closed file")
1018+
1019+
1020+
@pytest.mark.anyio
1021+
async def test_a_closed_errlog_stops_stderr_forwarding_without_failing_the_session(
1022+
monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture
1023+
) -> None:
1024+
"""Losing the log sink mid-session costs the diagnostics, never the session.
1025+
1026+
A notebook cell that finishes closes the stream underneath the forwarder; the
1027+
server's own traffic has to survive that.
1028+
"""
1029+
errlog = _UnwritableErrlog()
1030+
process = FakeProcess(on_stdin_close=lambda: process.exit(0))
1031+
install_fake_process(monkeypatch, process)
1032+
ping = JSONRPCRequest(jsonrpc="2.0", id=1, method="ping")
1033+
1034+
with caplog.at_level(logging.DEBUG, logger="mcp.client.stdio"):
1035+
with anyio.fail_after(5):
1036+
async with stdio_client(FAKE_PARAMS, errlog=cast(TextIO, errlog)) as (read_stream, _):
1037+
await process.feed_stderr(b"server diagnostics no one will read\n")
1038+
await errlog.attempted.wait()
1039+
1040+
# The failed write must not have disturbed the message path.
1041+
await process.feed(_line(ping))
1042+
assert await _next_message(read_stream) == ping
1043+
1044+
assert "the log stream was closed" in caplog.text
1045+
assert process.returncode == 0
1046+
1047+
1048+
@pytest.mark.anyio
1049+
async def test_a_stderr_pipe_dying_with_the_server_ends_forwarding_quietly(
1050+
monkeypatch: pytest.MonkeyPatch,
1051+
) -> None:
1052+
"""A stderr pipe torn down with the process is shutdown noise, not an error.
1053+
1054+
The proactor loop reports a hard-killed pipe as a reset rather than EOF, which
1055+
must not propagate out of the transport.
1056+
"""
1057+
errlog = StringIO()
1058+
process = FakeProcess(
1059+
on_stdin_close=lambda: process.exit(0),
1060+
stderr_eof_error=anyio.BrokenResourceError(),
1061+
)
1062+
install_fake_process(monkeypatch, process)
1063+
1064+
with anyio.fail_after(5):
1065+
async with stdio_client(FAKE_PARAMS, errlog=cast(TextIO, errlog)) as (_, _write):
1066+
await process.feed_stderr(b"last words\n")
1067+
1068+
assert errlog.getvalue() == "last words\n"
1069+
assert process.returncode == 0
9781070

9791071

9801072
# ---------------------------------------------------------------------------

tests/transports/stdio/test_lifecycle.py

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import sys
1515
import threading
1616
from contextlib import AsyncExitStack
17+
from io import StringIO
1718
from pathlib import Path
1819
from textwrap import dedent
1920

@@ -170,8 +171,9 @@ async def test_server_stderr_output_reaches_the_errlog_file(
170171
) -> None:
171172
"""What the server writes to stderr lands in the file passed as `errlog`.
172173
173-
The spawn hands over errlog's file descriptor as the child's stderr, so it must
174-
be a real file -- an in-memory StringIO has no fileno.
174+
A real file has a descriptor, so the spawn hands it straight to the child and no
175+
forwarding task is involved. The descriptor-less case is covered by
176+
test_server_stderr_output_reaches_an_errlog_without_a_file_descriptor.
175177
"""
176178
marker = "stdio-lifecycle stderr marker 4242"
177179

@@ -206,6 +208,45 @@ async def test_server_stderr_output_reaches_the_errlog_file(
206208
assert spawned_processes[0].returncode == 0
207209

208210

211+
@pytest.mark.anyio
212+
async def test_server_stderr_output_reaches_an_errlog_without_a_file_descriptor(
213+
spawned_processes: list[anyio.abc.Process | FallbackProcess],
214+
) -> None:
215+
"""Server stderr reaches an `errlog` that a child process cannot inherit.
216+
217+
Jupyter replaces sys.stderr with an ipykernel stream that has no descriptor to
218+
hand over, which used to drop the server's diagnostics entirely (#156). StringIO
219+
stands in for it here: same missing fileno, no notebook needed.
220+
"""
221+
marker = "stdio-lifecycle fd-less stderr marker 4243"
222+
errlog = StringIO()
223+
224+
async with AsyncExitStack() as stack:
225+
sock, port = await open_liveness_listener()
226+
stack.push_async_callback(sock.aclose)
227+
228+
server = (
229+
f"import socket, sys\n"
230+
f"s = socket.create_connection(('127.0.0.1', {port}))\n"
231+
f"s.sendall(b'alive')\n"
232+
f"sys.stderr.write({marker!r} + '\\n')\n"
233+
f"sys.stderr.flush()\n"
234+
f"sys.stdin.read()\n"
235+
)
236+
params = StdioServerParameters(command=sys.executable, args=["-c", server])
237+
238+
# The bound covers one interpreter cold start on a loaded runner; a
239+
# healthy run takes well under a second.
240+
with anyio.fail_after(10.0):
241+
async with stdio_client(params, errlog=errlog):
242+
stream = await accept_alive(sock)
243+
stack.push_async_callback(stream.aclose)
244+
245+
# Shutdown drains the forwarder after the server dies, so the write has landed.
246+
assert marker in errlog.getvalue()
247+
assert spawned_processes[0].returncode == 0
248+
249+
209250
@pytest.mark.skipif(
210251
not hasattr(os, "waitid"), reason="needs os.waitid(WNOWAIT); absent on Windows and macOS before 3.13"
211252
)

0 commit comments

Comments
 (0)