fix(stdio_client): tolerate invalid UTF-8 from child stdout - #2456
Open
shaun0927 wants to merge 2 commits into
Open
fix(stdio_client): tolerate invalid UTF-8 from child stdout#2456shaun0927 wants to merge 2 commits into
shaun0927 wants to merge 2 commits into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
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
Copy link
Copy Markdown
Author
|
for additional signal: mcp-claude[bot] independently reproduced this on both main and v1.x and arrived at the same one-line default change (issue #2454 comment). all 25 CI checks are green; happy to address review feedback. |
Sorry, something went wrong.
This was referenced May 18, 2026
Copy link
Copy Markdown
|
Heads up @shaun0927 — this PR has gone conflicting after the stdio transport was refactored on main. I rebuilt the same root-cause fix (defaulting encoding_error_handler to "replace") cleanly against current main with a mid-session regression test in #2873, crediting your original approach here. Happy to defer if you'd rather refresh this one instead. |
Sorry, something went wrong.
Closed
4 tasks
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
2 participants
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 with encoding_error_handler="strict", so a malformed UTF-8 line can raise during TextReceiveStream(...) iteration and crash the transport task group.
This PR makes the client follow the same strategy that stdio_server() already uses after #2302:
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.py that spawns a child process which writes:
Without this patch, the transport fails before the valid line is delivered.
With this patch:
Local verification:
Breaking Changes
None.
Types of changes
Checklist