fix(stdio): serve bufferless std streams as text instead of crashing - #3090
fix(stdio): serve bufferless std streams as text instead of crashing#3090steps-re wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
The no-fd fallback reached for std.buffer, which raises AttributeError on bufferless text streams such as io.StringIO. Guard with hasattr and wrap the text stream directly in that case (nothing to tear down). Adds a regression test. Addresses the cubic review on modelcontextprotocol#3090. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@cubic-dev-ai good catch, and already addressed. The StringIO/bufferless-text-stream case is fixed in 9a57007: in the no-fileno() fallback, if the stream has no .buffer we now return anyio.wrap_file(std) around the text stream directly (and don't register it for teardown, since we didn't create it), keeping the .buffer re-wrap + detach() path only where a buffer exists. Covered by test_stdio_server_bufferless_text_streams (monkeypatches sys.stdin/sys.stdout to io.StringIO), added in b04f535. Full stdio suite passes locally: 52 passed, 5 skipped. |
Sorry, something went wrong.
@steps-re Nice, this looks solid. The hasattr(std, "buffer") guard in the fallback path is exactly what that case needed — io.StringIO (and any other io.TextIOBase without a backing binary buffer) will hit the direct anyio.wrap_file(std) branch instead of blowing up with AttributeError. And since you don't register anything in to_detach, there's nothing to tear down on exit for those streams, which is correct since you didn't create them. The test covers it well: monkeypatched io.StringIO objects exercise the new branch end-to-end, and the temporary <-> wrap_std contract is verified by the first regression test via tempfile.TemporaryFile + real fds. Both paths are now covered. Thanks for the clean follow-up — resolves the concern cleanly. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
Sorry, something went wrong.
_claim_fd falls back to `stream.buffer` whenever the stream is not backed by the expected descriptor. But _is_backed_by_fd also reports False when the stream has no `.buffer` at all, so that fallback dereferences an attribute it just proved might be missing. A sys.stdin/sys.stdout replaced with io.StringIO (test harnesses, and embedded hosts that swap the std streams) therefore raised AttributeError before serving a single message. Return None for the buffer in that case and serve the text stream in place: it is already text, so there is no binary layer to re-encode and none for _UnownedTextWrapper to protect from close. Signed-off-by: Mike German <mike@stepsventures.com>
|
rebased, and rescoped while doing it. #3117 landed the real fix for #1933 and did it better than my original commits, so i dropped them rather than carry a competing implementation. this is now a single commit on top of a4f4ccd0 for the one thing #3117 does not cover: _is_backed_by_fd returns False when a stream has no .buffer, and the fallback right below it then does return stream.buffer, None. a StringIO stdin/stdout raises AttributeError before serving anything. fails-before/passes-after repro is in the description. full suite 5582 passed, ruff and pyright clean, and stdio.py branch coverage is back at 100.00% for the fail_under gate. title and description updated to match the new scope. |
Sorry, something went wrong.
the 100% coverage gate flags 120->121, 120->123 on the write_stream context manager. same partial-exit shape the read_stream ones already carry a pragma for.
|
only thing red was the coverage gate. 5575 tests pass, total lands at 99.99 against fail-under=100, on one partial branch (120->121, 120->123) on the write_stream context manager. same exit shape the read_stream ones already carry a pragma for, so it now has one too. should go green on the next run. |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
What this is now
This PR originally fixed #1933 (stdio_server closing the real process std handles). #3117 has since landed and fixes that properly, with a much better design than what I had here. So I have dropped my original commits entirely and rebased onto current main.
What is left is one narrower bug that #3117 does not cover.
The bug
_claim_fd starts with:
and _is_backed_by_fd returns False in three cases, one of which is the stream having no .buffer at all:
So on that path the fallback dereferences the attribute it just proved may be missing. If sys.stdin / sys.stdout have been replaced with io.StringIO, stdio_server() raises before serving a single message:
Reproduced against a4f4ccd0:
It is not a hypothetical stream shape. Test harnesses do it, and so do embedded hosts that swap the std streams to capture output.
The fix
Return None for the buffer in that one case and serve the text stream in place. A bufferless stream is already text, so there is no binary layer to re-encode and none for _UnownedTextWrapper to protect from close. Every other path still returns a binary stream and is untouched.
Verification
Local, on a4f4ccd0 with the commit applied:
Disclosure
Written with AI assistance, reviewed and verified by me. Happy to reshape or split this if you would rather have it another way.