test: fix WS test port race; narrow to single smoke test covering both transport ends - #2267
Merged
maxisbey merged 5 commits intoMar 18, 2026
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
maxisbey
marked this pull request as draft
March 10, 2026 14:42
test_ws.py used the subprocess + TCP port pattern that races under pytest-xdist: a worker allocates a port with socket.bind(0), releases it, then spawns a uvicorn subprocess hoping to rebind. Between release and rebind, another worker can claim the port, causing the WS client to connect to an unrelated server (observed: HTTP 403 Forbidden on the WebSocket upgrade). Three of the four tests here verify transport-agnostic MCP semantics (read_resource happy path, MCPError propagation, session recovery after client-side timeout). These now use the in-memory Client transport — no network, no subprocess, no race. The fourth test (test_ws_client_basic_connection) is kept as a smoke test running the real WS stack end-to-end. It uses a new run_uvicorn_in_thread helper that binds port=0 atomically and reads the actual port back from the server's socket — the OS holds the port from bind to shutdown, eliminating the race window entirely. This test alone provides 100% coverage of src/mcp/client/websocket.py. Also removed dead handler code (list_tools/call_tool were never exercised) and the no-longer-needed pragma: no cover annotations on the read_resource handler (it now runs in-process).
The three tests converted to in-memory transport in the previous commit weren't testing WebSocket behavior anymore — they were sitting in test_ws.py with misleading names. - test_ws_client_happy_request_and_response: deleted. Duplicates tests/client/test_client.py::test_read_resource. - test_ws_client_timeout: deleted. tests/issues/test_88_random_error.py covers the same session-recovery-after-timeout scenario more thoroughly (uses read_timeout_seconds and an anyio.Event to release the slow handler cleanly). - test_ws_client_exception_handling: moved to test_client.py as test_read_resource_error_propagates. This was the only unique behavior — nothing else asserts that a handler-raised MCPError reaches the client with its error code intact. test_ws.py now contains only what it says on the tin.
The previous version polled server.started with time.sleep(0.001) until uvicorn finished binding. We were waiting for uvicorn to tell us the port — but we can just bind the socket ourselves and hand it to uvicorn via server.run(sockets=[sock]). Once sock.listen() returns, the kernel queues incoming connections (up to the backlog). If a client connects before uvicorn's event loop reaches accept(), the connection sits in the accept queue and is picked up as soon as uvicorn is ready. The kernel is the synchronizer — no cross-thread flag needed. The port is now known before the thread starts. No polling, no sleep, no wait at all.
The function-level pragma existed because the server ran in a subprocess where coverage couldn't instrument it. The in-thread conversion makes it reachable, so strict-no-cover (#2305) flags it as stale. Narrow the pragma to the three error paths the smoke test doesn't exercise.
maxisbey
force-pushed
the
maxisbey/max-157-flaky-convert-websocket-tests-to-in-memory-transport-remove
branch
from
97ba3fa to
400f541
Compare
March 18, 2026 11:43
uvicorn.Config.load() calls asyncio.iscoroutinefunction during interface autodetection. Python 3.14 deprecates that function; under filterwarnings=error the DeprecationWarning crashes the server thread before it binds, and the test's WS client times out waiting for a handshake that never comes. This only surfaced with the in-thread conversion — the old subprocess runner wasn't subject to pytest's warning filters. Starlette is asgi3, so declare it and skip the autodetect.
maxisbey
marked this pull request as ready for review
March 18, 2026 13:51
jerome3o-anthropic
approved these changes
Mar 18, 2026
maxisbey
deleted the
maxisbey/max-157-flaky-convert-websocket-tests-to-in-memory-transport-remove
branch
March 18, 2026 15:48
maxisbey
added a commit
that referenced
this pull request
Mar 18, 2026
Rebased onto MAX-157 (#2267) which landed run_uvicorn_in_thread with two improvements over the helper this branch had: - Pre-binds socket + listen() before thread start: port is known immediately, no polling loop, kernel queues early connections - interface="asgi3": skips uvicorn's asyncio.iscoroutinefunction autodetect, which Python 3.14 deprecates (cleaner than catch_warnings) Kept this branch's gc.collect() + ResourceWarning suppression in the finally — MAX-157's single WS smoke test didn't need it, but the SSE abrupt-disconnect test patterns here do. Also removed wait_for_server — MAX-155 (#2277) converted its last caller (test_integration.py) to in-memory transport.
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
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.
Problem
tests/shared/test_ws.py had 4 tests sharing a subprocess+uvicorn fixture with a TOCTOU port race: bind port 0 → release → subprocess rebinds. Under pytest-xdist another worker claims the port in the gap, and the WS client connects to the wrong server's HTTP endpoint, yielding a 403 Forbidden on upgrade (job 63414508256).
Disposition of the 4 tests
Only one of the four was actually testing WebSocket. The other three tested MCP session semantics with WS as an incidental pipe — swap in any transport and they pass identically.
~150 of the old 211 lines were subprocess launcher + port-polling + fixture plumbing shared by all four. With one test remaining on a shared helper, the file is 51 lines.
The smoke test
test_ws_client_basic_connection now uses run_uvicorn_in_thread (tests/test_helpers.py), which pre-binds the socket and calls listen() before the server thread starts. The port is known immediately — the kernel's listen queue buffers any early connection until uvicorn's loop reaches accept(). No polling, no sleep, no gap for another xdist worker to steal the port.
Knock-on effects of in-thread
Two subprocess-era assumptions stopped holding once uvicorn runs in the test process:
Coverage now reaches server/websocket.py. The function-level # pragma: no cover existed because the subprocess couldn't be instrumented; strict-no-cover (ci: run strict-no-cover in scripts/test to catch stale pragmas locally #2305) correctly flags it as stale. Narrowed to the three error paths the smoke test doesn't exercise. The one test now covers both client/websocket.py and server/websocket.py at 100%.
pytest's warning filters now apply to uvicorn. uvicorn.Config.load() calls asyncio.iscoroutinefunction() during ASGI autodetection; Python 3.14 deprecates it; filterwarnings=error promotes it; the server thread crashes silently before binding. Fixed by passing interface="asgi3" to skip the autodetect — Starlette is asgi3, the detection was always going to land there.
Test plan
AI Disclaimer