Repository navigation
test: tolerate the stdio transport's close race on teardown - #9
Conversation
The new stdio probe fails on the Windows lane (run 42595057330, job test (windows-latest, 3.11)) with BrokenResourceError raised out of stdio_client's TaskGroup. mcp 1.26's stdout_reader catches ClosedResourceError only: when the child writes its last line while the client closes the read stream, the send raises BrokenResourceError and the group surfaces through the caller, masking a clean shutdown. All assertions run inside a live session, so only that closed-stream leaf is treated as benign, mirroring server._reraise_unless_stdio_disconnect; any real error, cancellation, KeyboardInterrupt or SystemExit still fails the test. The probe is split into two sessions so the second one (selection must not leak between clients) still runs after the first one's teardown. New test pins the filter: a group of only BrokenResourceError is tolerated and a mixed group is re-raised.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e312c0ba3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(exc, BaseExceptionGroup): | ||
| _, remaining = exc.split(_is_benign_stream_close) | ||
| if remaining is not None: | ||
| raise | ||
| return |
There was a problem hiding this comment.
If a stream closes during session initialization or a tool request, this wrapper also swallows the resulting BrokenResourceError/ClosedResourceError, even though the remaining assertions never execute. The exception type alone does not establish that the failure happened during teardown: the try covers the entire probe, and neither client records that its assertions completed. Consequently, a transport regression can make the integration test pass without verifying project selection or client isolation. Record successful completion inside each session after its final assertion, and tolerate these exceptions only when that marker is set.
Useful? React with 👍 / 👎.
What fails
cirun 42595057330 on0c573a69is red only on test (windows-latest, 3.11) (ubuntu 3.11/3.13 and lint pass). The failure is at teardown, after every assertion:Why
mcp1.26.0'sstdio_client.stdout_readercatchesanyio.ClosedResourceErroronly. When the child writes its last line while the client is closing the read stream, thesendraisesBrokenResourceErrorinstead, and the transport's TaskGroup raises that group into the caller — masking a clean shutdown. The Windows lane loses the race because process exit and pipe teardown are slower there. This is not a server bug: the child logged only its three startup lines and no traceback, and every assertion had already passed.Change
_is_benign_stream_close/_run_stdio_probeintests/test_project_catalog.py, mirroringserver._reraise_unless_stdio_disconnect: a group made only ofBrokenResourceError/ClosedResourceErrorleaves is tolerated, any other leaf (real error, cancellation,KeyboardInterrupt,SystemExit) is re-raised.first_client/second_clientso the second session — selection must not leak between clients — still runs on Windows after the first session's teardown.test_stdio_probe_teardown_close_is_tolerated_but_real_errors_are_notpins the filter: a BrokenResourceError-only group is swallowed, a group that also carriesValueErroris re-raised.Evidence
Local, Linux, Python 3.11.2,
uv sync --frozen+ embedded model:pytest -q tests/test_project_catalog.py— 15 passed (14 before, +1 new).test_stdio_binding_selects_only_current_client_context— passed 3/3 separate runs._is_benign_stream_closeforced toFalse, the new test fails with the BrokenResourceError group; restored, it passes.ruff check tests/test_project_catalog.pyclean;mypy tests/test_project_catalog.pyreports the same 4 pre-existingunion-attrfindings onstale.content[0].text, none new.The Windows lane in this PR's own
cirun is the proof for the failure that could not be reproduced on Linux.