fix(stdio_client): tolerate invalid UTF-8 from child stdout#2456
Open
shaun0927 wants to merge 2 commits intomodelcontextprotocol:mainfrom
Open
fix(stdio_client): tolerate invalid UTF-8 from child stdout#2456shaun0927 wants to merge 2 commits intomodelcontextprotocol:mainfrom
shaun0927 wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
The stdio server path already degrades invalid UTF-8 into parse errors instead of killing the transport, but the client side still defaulted to strict decoding and could crash the whole task group on a single bad line from a child process. This changes the default decode strategy to replacement, broadens shutdown-time exception handling for abrupt child exits, and adds a regression test that proves malformed output becomes an in-stream error while the next valid JSON-RPC message still arrives. Constraint: Must preserve stdio_client cleanup behavior when subprocesses exit early or reset pipes Rejected: Add a new configuration flag only for the regression test | leaves the default transport behavior asymmetric and crash-prone Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep client and server stdio malformed-UTF-8 handling symmetric unless the protocol deliberately diverges Tested: uv run --frozen pytest tests/client/test_stdio.py; uv run --frozen ruff check src/mcp/client/stdio.py tests/client/test_stdio.py; uv run --frozen pyright src/mcp/client/stdio.py Not-tested: Full multi-platform matrix outside local macOS / Python 3.10 run
The malformed-UTF8 regression test now executes the JSON-parse failure branch in stdio_client, so the old pragma no longer reflects reality and breaks CI's strict-no-cover gate. This follow-up removes the stale pragma without changing behavior. Constraint: Must not widen the PR scope beyond the already-proposed stdio robustness fix Rejected: Suppress strict-no-cover or weaken the regression test | hides real coverage drift instead of correcting it Confidence: high Scope-risk: narrow Reversibility: clean Directive: When adding malformed-input regression tests, re-audit nearby no-cover pragmas immediately Tested: coverage run of tests/client/test_stdio.py plus strict-no-cover locally up to the updated branch execution path Not-tested: Full upstream CI rerun after push
Author
|
for additional signal: |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #2454
Summary
stdio_client()currently decodes child stdout withencoding_error_handler="strict", so a malformed UTF-8 line can raise duringTextReceiveStream(...)iteration and crash the transport task group.This PR makes the client follow the same strategy that
stdio_server()already uses after #2302:errors="replace"It also treats abrupt child shutdowns (
BrokenResourceError/ConnectionResetError) as normal shutdown-time conditions in the background stdio tasks.Motivation and Context
PR #2302 fixed the server side so invalid UTF-8 on stdin becomes a parse error instead of a transport crash. The client side still behaved asymmetrically: a buggy or non-compliant child process could kill the Python client with a single malformed stdout line even if the next line was valid JSON-RPC.
That makes the client less robust than the server for the same class of malformed stdio input.
How Has This Been Tested?
Added a focused regression test in
tests/client/test_stdio.pythat spawns a child process which writes:b"\xff\xfe\n")Without this patch, the transport fails before the valid line is delivered.
With this patch:
ExceptionSessionMessageLocal verification:
uv run --frozen pytest tests/client/test_stdio.pyuv run --frozen ruff check src/mcp/client/stdio.py tests/client/test_stdio.pyuv run --frozen pyright src/mcp/client/stdio.pyBreaking Changes
None.
Types of changes
Checklist