Run SSE and Unicode transport tests in process instead of over sockets - #2765
Run SSE and Unicode transport tests in process instead of over sockets#2765maxisbey merged 1 commit into
Conversation
Sorry, something went wrong.
Same class of fix as the security-test migration: these files spawned
uvicorn subprocesses on bind-then-close ports and polled for readiness,
which races under pytest-xdist when two workers pick the same port.
tests/shared/test_sse.py now drives the same Starlette apps in process:
sse_client connects through an httpx_client_factory backed by the
in-process streaming bridge, raw-httpx assertions use the same bridge,
and the mounted-app and request-context servers are built as plain app
factories instead of subprocess targets. Assertions are unchanged, with
two deliberate exceptions now that the server handlers run traced
in-process:
- test_sse_client_timeout is deleted. It has been permanently skipped
("highlights a possible bug in SSE read timeout exception handling"),
and its premise — a real network read timeout — cannot exist for an
in-process app, so it could never be unskipped here. The slow://
resource branch and sse_read_timeout plumbing existed only for it.
- Handlers that no test ever invoked (the main server's tools handlers,
the context server's unknown-tool fallthrough) are removed or
replaced with dispatch asserts, since unreachable branches now fail
branch coverage instead of hiding in an untraced subprocess.
tests/client/test_http_unicode.py gets the same treatment: the Unicode
echo server now runs in process and streamable_http_client speaks to it
through the bridge with follow_redirects enabled, matching the SDK's
own client factory (Starlette's Mount 307-redirects the bare /mcp
path).
There was a problem hiding this comment.
LGTM — test-only migration that follows the same in-process harness pattern already merged in #2764.
Extended reasoning...Overview
This PR touches only two test files (tests/shared/test_sse.py and tests/client/test_http_unicode.py), replacing the subprocess + uvicorn + ephemeral-port harness with in-process Starlette apps driven through the existing StreamingASGITransport bridge from tests/interaction/transports. No production code under src/ is modified. The assertions of the surviving tests are preserved; the changes beyond the harness swap are deletion of a permanently-skipped test (test_sse_client_timeout), removal of dead handler branches that were never exercised, and docstring/annotation cleanup.
Security risks
None. The change is test-only. Disabling DNS-rebinding protection in the in-process test apps is appropriate — that protection guards against a network-level attack that cannot occur in-process, and its behaviour remains pinned by tests/server/test_sse_security.py, which I confirmed exists in the repository.
Level of scrutiny
Low-to-moderate. Test refactors carry the risk of silently weakening coverage rather than introducing bugs, and the author addresses the two coverage-relevant deletions explicitly: the deleted timeout test was @pytest.mark.skip since introduction and never executed, and the removed handler branches were unreachable. The bridge pattern (StreamingASGITransport, cancel_on_close=False for SSE drains, httpx_client_factory injection) is the same one already established and merged in #2764 and used by tests/interaction/transports/test_sse.py, so this is following an established repository pattern rather than introducing a new design.
Other factors
The bug hunting system found no bugs. The PR description reports a green full test run with 100% line+branch coverage and extensive stress testing under pytest-xdist. The change is self-contained, mechanical in nature, and consistent with the project's stated direction of migrating remaining socket-based test files in-process.
Sorry, something went wrong.
In `sse_client`, the message-POST coroutine `_send_message` called
`response.raise_for_status()` with no handler. When the server returned a
non-2xx (401/403/404/5xx) or the POST hit a network error, the exception
propagated into the `post_writer` task group and was swallowed by its
`except Exception: logger.exception("Error in post_writer")`. The failure
was never delivered through the read stream, so a caller blocked on
`read_stream.receive()` (e.g. `ClientSession.initialize()`) hung forever.
Catch `httpx.HTTPError` inside `_send_message` and forward it to
`read_stream_writer`, the same pattern stdio.py and websocket.py already
use, and that `streamable_http.py` uses for its >= 400 responses. The
caller now receives the error promptly instead of deadlocking.
Adds an in-process regression test (matching the modelcontextprotocol#2765 harness) whose
message POST returns 503; it asserts the caller receives the
`HTTPStatusError` via the read stream within a bounded timeout, and fails
(times out) against the unpatched client.
Refs modelcontextprotocol#2110
Second installment of the in-process test migration that started in #2764: tests/shared/test_sse.py and tests/client/test_http_unicode.py still used the bind-then-close port pick + uvicorn subprocess + readiness-poll harness, which races under pytest-xdist when two workers grab the same ephemeral port. test_sse_session_cleanup_on_disconnect has flaked exactly this way under parallel load.
Motivation and Context
Same mechanism and fix as #2764. The two files now drive the same Starlette apps in process through StreamingASGITransport (via the sanctioned tests.interaction.transports re-export):
Assertions are unchanged, with two deliberate exceptions now that the server handlers run as traced in-process code:
The migrated test_sse.py also gets the same prose pass the security files got in #2764: behaviour-sentence docstrings, full annotations, and the line-counter loop in the raw-connection test replaced by two anext() assertions, which retires the file's last pragma: no branch.
tests/shared/test_streamable_http.py is the remaining subprocess-based file and will be its own (final) migration, along with pruning the then-unused wait_for_server helper.
How Has This Been Tested?
Breaking Changes
None — test-only.
Types of changes
Checklist
Additional context
Several migrated tests now overlap with tests/interaction/transports/test_sse.py coverage (basic connection, session-created callback, disconnect cleanup, happy/error round trips). Deduplicating is deliberately out of scope here — this PR keeps the harness swap auditable — but it's inventoried for a follow-up.
AI Disclaimer