Skip to content

Run SSE and Unicode transport tests in process instead of over sockets#2765

Merged
maxisbey merged 1 commit into
mainfrom
maxisbey/deflake-sse-and-unicode-tests
Jun 2, 2026
Merged

Run SSE and Unicode transport tests in process instead of over sockets#2765
maxisbey merged 1 commit into
mainfrom
maxisbey/deflake-sse-and-unicode-tests

Conversation

@maxisbey
Copy link
Copy Markdown
Contributor

@maxisbey maxisbey commented Jun 2, 2026

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):

  • tests/shared/test_sse.py: sse_client connects through an httpx_client_factory backed by the bridge — the same pattern the interaction suite's SSE leg uses — and the basic/mounted/request-context servers become plain app factories sharing one make_app(server) builder. DNS-rebinding protection is disabled in the in-process apps (it guards against a network attack that can't exist here; the protection behaviour itself is pinned by tests/server/test_sse_security.py).
  • tests/client/test_http_unicode.py: the Unicode echo server runs in process and streamable_http_client speaks to it through the bridge with follow_redirects=True, matching the SDK's own client factory (Starlette's Mount 307-redirects the bare /mcp path; the subprocess version silently relied on redirect-following too).

Assertions are unchanged, with two deliberate exceptions now that the server handlers run as traced in-process code:

  • test_sse_client_timeout is deleted. It was @pytest.mark.skip from the day it was introduced ("highlights a possible bug in SSE read timeout exception handling") and carried pragma: no cover, so no executing coverage is lost. Its premise — a real socket read timeout firing mid-stream — cannot occur for an in-process app, so it could never be unskipped here. The slow:// resource branch and the sse_read_timeout=0.5 fixture plumbing existed only for it. Note for the record: the suspicion it encoded (an expired sse_read_timeout may not surface an error to the in-flight request — the ReadTimeout path in src/mcp/client/sse.py is still pragma: lax no cover) is not tracked anywhere; nearest existing issue is client's read_stream_writer open after SSE disconnection hanging .receive() #1811, which is the same failure family with a different trigger. Happy to file a dedicated issue so deleting the test doesn't delete the suspicion.
  • Handlers no test ever invoked are gone or restructured. The main SSE server's tools handlers had no callers; the context server's unknown-tool fallthrough and defensive if ctx.request: arms were dead. They're removed or replaced with dispatch asserts (assert params.name in (...)), since unreachable branches now fail branch coverage instead of hiding in an untraced subprocess.

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?

  • ./scripts/test green: 1526 passed, 100% line+branch coverage, strict-no-cover clean (the suite's skip count drops from 2 to 1 with the dead test removed)
  • The two files stress-tested heavily: ~100 runs including pytest-xdist at -n 2/4/8/16, sustained CPU load (loadavg >100), GC-canary runs for ResourceWarning leaks under filterwarnings=error, and failure-injection runs to verify clean teardown on assertion failure — zero failures
  • The post-disconnect 404 regression test (SSE: ConnectionClosed exception after session disconnect #1227) is now deterministic by construction: the bridge's drain-on-close means the server-side session teardown provably completes before the follow-up POST (the old socket version actually raced uvicorn's disconnect processing)

Breaking Changes

None — test-only.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

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

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).
@maxisbey maxisbey marked this pull request as ready for review June 2, 2026 20:38
@maxisbey maxisbey requested a review from Kludex June 2, 2026 20:43
@maxisbey maxisbey merged commit ed39e73 into main Jun 2, 2026
54 of 55 checks passed
@maxisbey maxisbey deleted the maxisbey/deflake-sse-and-unicode-tests branch June 2, 2026 20:46
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants