Forward server stderr when errlog has no file descriptor - #3268
Forward server stderr when errlog has no file descriptor#3268maisymylod wants to merge 2 commits into
Conversation
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 modelcontextprotocol#156
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/client/stdio.py">
<violation number="1" location="src/mcp/client/stdio.py:233">
P2: Final server stderr bytes can still be dropped during shutdown on asyncio. The new drain wait happens after `_stop_server_process()` closes the subprocess transport, so delaying transport close until after `stderr_done.wait()` in forward-stderr mode would make this drain reliable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # The server is dead, so its stderr pipe is at EOF with at most a buffer left; | ||
| # let the forwarder finish it before the task group's cancel takes the task out. | ||
| if forward_stderr: | ||
| with anyio.move_on_after(_STDERR_DRAIN_TIMEOUT): |
There was a problem hiding this comment.
P2: Final server stderr bytes can still be dropped during shutdown on asyncio. The new drain wait happens after _stop_server_process() closes the subprocess transport, so delaying transport close until after stderr_done.wait() in forward-stderr mode would make this drain reliable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/stdio.py, line 233:
<comment>Final server stderr bytes can still be dropped during shutdown on asyncio. The new drain wait happens after `_stop_server_process()` closes the subprocess transport, so delaying transport close until after `stderr_done.wait()` in forward-stderr mode would make this drain reliable.</comment>
<file context>
@@ -193,13 +227,20 @@ async def shutdown() -> None:
+ # The server is dead, so its stderr pipe is at EOF with at most a buffer left;
+ # let the forwarder finish it before the task group's cancel takes the task out.
+ if forward_stderr:
+ with anyio.move_on_after(_STDERR_DRAIN_TIMEOUT):
+ await stderr_done.wait()
await _aclose_all(read_stream, write_stream, read_stream_writer, write_stream_reader)
</file context>
The 3.14 jobs failed the 100% gate while every other version passed. Both files were at 100% statement coverage; the shortfall was two partial branches, one per file, on the `async with stdio_client(...)` line nested inside `anyio.fail_after(...)`. On 3.14 the tracer records a self-arc for that line instead of the arcs into the body and out past the enclosing block, so both declared exits read as missing even though the body demonstrably runs. Same code and same tests measure 100% on 3.13. This is the existing convention here: test_lifecycle.py already carries `# pragma: no branch` on an identical `stdio_client` line, and the repo uses the pragma in 232 places, including one that names the same cause.
|
Thanks for the contribution. This repository only keeps pull requests open when they're linked to an issue that a maintainer has assigned to the author — CONTRIBUTING.md explains why and how we work. This PR has been closed for now because you aren't currently assigned to #156. If a maintainer would like this change as a PR from you, they'll assign you to #156 and this PR will reopen automatically — there's nothing more you need to do. (If you opened the issue, this PR already shows up on its timeline.) There's no need to open a new PR — this one will be reopened. While it's closed, please push any updates as new commits rather than force-pushing, since GitHub can't reopen a PR whose branch has been rewritten. Maintainers: reopening this PR, removing the |
Closes #156.
Problem
stdio_clientpasseserrlogstraight toanyio.open_process(stderr=...), which needs a real OS file descriptor for the child to inherit. Writers that have none never work:sys.stderrwith a stream whosefileno()raisesio.UnsupportedOperation, so the server's stderr is lost. This is the case reported in support logging to stderr in Jupyter Notebook Environments. #156.io.StringIO, the obvious way to capture server logs in a test, fails the same way.The spawn either raises
io.UnsupportedOperationor sends diagnostics somewhere the caller never sees.Fix
Check whether
errloghas a usable descriptor. If it does, nothing changes: it is inherited exactly as before. If it does not, spawn withsubprocess.PIPEand forward the pipe intoerrlogfrom a reader task.Two details worth review:
shutdown()waits (bounded, 0.5s) on the reader before the task group cancel takes it out. Without this the last diagnostics, which are exactly the ones you want when a server dies, race the cancel.capture_fd_outputis on, and that path already routes back to the notebook, so inheriting it remains correct.A closed
errlog(a finished notebook cell) is logged at debug and stops forwarding rather than failing the session.FallbackProcessgained astderrattribute so the WindowsSelectorEventLooppath works too.Tests
test_server_stderr_output_reaches_an_errlog_without_a_file_descriptorreproduces support logging to stderr in Jupyter Notebook Environments. #156 with a real subprocess. It fails onmainwith the reportedio.UnsupportedOperation.test_server_stderr_output_reaches_the_errlog_filestill passes unchanged, pinning the inherit path.Full suite passes and
src/mcp/client/stdio.pystays at 100% coverage. (tests/server/test_streamable_http_modern.py::test_moderation...fails on a clean checkout too and is unrelated.)AI disclosure
AI tooling was used to write and push the changes. The diagnosis, the approach, and the findings are my own, and I can speak to any of it.