Detect event-loop blocking in tests - #3510
Conversation
Codex Review SummaryThis 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: dcc8afae72
ℹ️ 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".
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked two things in validate_tool_result and ruled them out: a concurrent list_tools during the off-loop compile cannot poison the cache (the _same_schema guard skips caching, and the in-flight call still validates against the schema it read before the await), and moving the structured_content is None check ahead of compilation only changes which RuntimeError wins when a tool has both an invalid schema and no structured content — same exception type, so callers catching it are unaffected.
Extended reasoning...
Findings are being posted inline, so approval is not on the table. This note records two concrete concerns examined from the diff at src/mcp/client/session.py:1137-1144 and ruled out: the stale-validator cache race is closed by the _same_schema check after the thread hop, and the reordering of the structured-content check versus schema compilation only affects error precedence in the double-failure case, with the same exception type either way. The removed in-function cache hit in _output_schema_validator also means the earlier coverage concern about an unreachable branch is moot.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 2 findings from earlier reviews are still open above.
Still open from earlier reviews (2):
- 🔴
src/mcp/client/session.py:1142—If the client runs on a platform without worker threads (Pyodide/emscripten, which pyproject.toml:142 deliberately keep… - 🔴
src/mcp/client/session.py:1142—Servers whose sync handlers call a downstream call_tool from their worker thread can hang forever under a burst of requ…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
LGTM — since the last review the scope shrank to the BlockBuster fixture plus the Windows command-resolution offload, and the session.py validator offload that the earlier open threads were about is no longer in the diff.
What was reviewed:
_get_executable_commandoffload: cancellation atstdio.py:124happens before the spawn, so no cleanup path runs against the monkeypatchedsys;get_windows_executable_commandalready swallowsOSError, so the thread hop changes no observablestdio_clientbehaviour.- conftest allowlist: checked the
os.scandir/os.listdirconcern for thejsonschema_specificationslazy import on Python 3.10-3.12 — the final commit adds theos.scandirentry and bumps the floor toblockbuster>=1.5.27, so both walkers are covered; CI on those cells is the remaining confirmation since the suite could not be executed here. - New cancellation test: the resolver signals via
anyio.from_thread.run_syncand is released/awaited only in worker threads, so it neither blocks the loop nor leaks the abandoned thread past the test.
Extended reasoning...
Overview
The diff against the base now touches five files: pyproject.toml/uv.lock add blockbuster>=1.5.27 to the dev group, tests/conftest.py adds an autouse BlockBuster(["mcp", "mcp_types"]) fixture with six targeted can_block_in exemptions (coverage source reads, jsonschema_specifications._schemas discovery, and the intentionally synchronous to_image_content/to_audio_content reads) plus an eager httpcore2 import, src/mcp/client/stdio.py makes _get_executable_command async and offloads the Windows shutil.which probing via anyio.to_thread.run_sync(..., abandon_on_cancel=True), and tests/client/test_stdio.py stubs the resolver in install_fake_process and adds a cancellation test. Commits 36fc996 and 4cd5f41 since the previous review removed the src/mcp/client/session.py validator-compilation offload entirely (reverting it to the base behaviour and dropping its promotion tests) in favour of allowlisting the jsonschema lazy import in conftest, and added the os.scandir exemption with the matching version floor.
Security risks
None. The runtime change is confined to moving an existing shutil.which lookup onto a worker thread on Windows; it does not alter which command is resolved, how the subprocess is spawned, or the environment passed to it. Everything else is test-only tooling and a dev-group dependency.
Level of scrutiny
Moderate. The runtime change is small and I traced its failure modes: get_windows_executable_command already catches OSError and returns the bare command, so exceptions crossing the thread boundary are unchanged; cancellation at the new await raises before _create_platform_compatible_process is called, so nothing is leaked and none of the later sys.platform checks in stdio.py run under the test's SimpleNamespace patch. The abandon_on_cancel=True thread is short-lived in production (shutil.which on a handful of candidates). For the conftest, the main risk is a BlockingError or KeyError on platforms I cannot exercise here (Windows, Python 3.10-3.12); the final commit's os.scandir entry addresses the version-dependent importlib.resources walker, and any remaining miss would surface as an immediately visible red CI cell rather than a latent production defect.
Other factors
The two session.py threads I left open earlier (thread-less runtimes, shared worker-pool starvation) are moot because that code is no longer in the diff. No third-party CHANGES_REQUESTED review is outstanding; the other reviewer's reviews are recorded as dismissed, consistent with superseded-by-push. The PR is authored by a maintainer, the dependency is dev-only per DEPENDENCY_POLICY.md, the conftest follows the Starlette fixture pattern with brief comments rather than a wall of suppressions, and the new test uses anyio.Event/threading.Event coordination under anyio.fail_after(5) per the repository's test conventions. I could not execute the suite in this environment (no venv, no permission to install), so CI is the final gate for the allowlist coverage across the matrix.
2 participants
Summary
mcpandmcp_typesjsonschemalazy imports, and synchronous media conversionTests
./scripts/testuv run --frozen ruff format --check .uv run --frozen ruff check .uv run --frozen pyrightAI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.