Uh oh!
There was an error while loading. Please reload this page.
Forward server stderr when errlog has no file descriptor - #3268
Open
maisymylod wants to merge 1 commit into
Open
Forward server stderr when errlog has no file descriptor#3268maisymylod wants to merge 1 commit into
maisymylod wants to merge 1 commit 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. Fixesmodelcontextprotocol#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.)