Skip to content

Deflake the session-level timeout test with trio's virtual clock #2788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maxisbey merged 1 commit into from
Jun 5, 2026
+16 −8
Merged

Deflake the session-level timeout test with trio's virtual clock #2788

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions tests/interaction/lowlevel/test_timeouts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
"""Request timeouts against the low-level Server, driven through the public Client API.

The handler blocks on an event that is never set, so the awaited response can never arrive and
any positive timeout fires deterministically on the next event-loop pass. The timeout is therefore
set to an effectively-zero duration: the tests add no wall-clock time to the suite. (Zero itself
any positive timeout fires deterministically on the next event-loop pass. Per-request timeouts are
set to an effectively-zero duration; the session-level test runs on trio's virtual clock instead
(see the comment there). Either way the tests add no wall-clock time to the suite. (Zero itself
cannot be used: a falsy read_timeout_seconds is silently treated as "no timeout".)
"""

import anyio
import pytest
from inline_snapshot import snapshot
from trio.testing import MockClock

from mcp import MCPError, types
from mcp.client.client import Client
Expand Down Expand Up @@ -85,8 +87,20 @@
assert result == snapshot(CallToolResult(content=[TextContent(text="still alive")]))


# A session-level timeout cannot use the effectively-zero pattern above: it also governs the
# initialize handshake, which must complete before the blocked tool call can wait the timeout
# out in full. Any real-clock margin is a bet against CI scheduler stalls (a 50ms value lost
# that bet in CI; the in-process handshake tail reaches ~190ms on a loaded windows runner), so
# this test runs on trio's virtual clock instead. With autojump, time advances only when every
# task is blocked: the handshake always has a runnable task and therefore cannot time out no
# matter how slow the runner, and once the tool call blocks on the never-answered request the
# run goes idle and the clock jumps straight to the deadline — deterministic, with no real wait.
@requirement("protocol:timeout:session-default")
@pytest.mark.parametrize(
"anyio_backend",
[pytest.param(("trio", {"clock": MockClock(autojump_threshold=0)}), id="trio-mockclock")],
)
async def test_session_level_timeout_applies_to_every_request() -> None:

Check warning on line 103 in tests/interaction/lowlevel/test_timeouts.py

View check run for this annotation

Claude / Claude Code Review

tests/interaction/README.md still calls these 'real-clock timeout tests' 

Nit: `tests/interaction/README.md` (line 64) still describes these as "the real-clock timeout tests" when explaining why they skip the transport-parametrized `connect` fixture, but after this change the session-level test runs on trio's MockClock virtual clock rather than the real clock. Consider rewording that phrase (e.g. just "the timeout tests") so the README stays consistent with the updated docstring and comments here.
Comment on lines +90 to 103
Copy link
Copy Markdown
Contributor

@claude claude Bot Jun 5, 2026

Choose a reason for hiding this comment

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

🟡 Nit: tests/interaction/README.md (line 64) still describes these as "the real-clock timeout tests" when explaining why they skip the transport-parametrized connect fixture, but after this change the session-level test runs on trio's MockClock virtual clock rather than the real clock. Consider rewording that phrase (e.g. just "the timeout tests") so the README stays consistent with the updated docstring and comments here.

Extended reasoning...

What's stale: tests/interaction/README.md lines 62–66 list the test groups that don't use the transport-parametrized connect fixture, and one of them is described as "the real-clock timeout tests (the timeout machinery is transport-independent and must not race transport latency)". That phrasing was accurate before this PR, when all three tests in test_timeouts.py ran against the real clock with effectively-zero or 50ms timeouts.

Why this PR makes it inaccurate: This change moves test_session_level_timeout_applies_to_every_request onto trio's MockClock(autojump_threshold=0) via the per-test anyio_backend parametrization. Concretely: the test no longer waits any real time at all — the handshake runs on virtual time that only advances when every task is blocked, and the 0.05s deadline is reached by an autojump, not by the real clock. So of the three timeout tests in the file, only the two per-request tests (test_request_timeout_fails_the_pending_call, test_session_serves_requests_after_timeout) still run on the real clock; the README's collective label "real-clock timeout tests" no longer covers the file.

Step-by-step: (1) A reader sees "real-clock timeout tests" in the README and opens tests/interaction/lowlevel/test_timeouts.py. (2) The module docstring now says the session-level test "runs on trio's virtual clock instead", and the comment block above test_session_level_timeout_applies_to_every_request (lines 90–97) explains the MockClock approach in detail. (3) The README and the file now describe the same tests with contradictory clock semantics — exactly the kind of drift the PR was otherwise careful to avoid, since it updated both the module docstring and the inline comments.

Why nothing else catches it: the README is prose; nothing enforces consistency between it and the test file, so the drift will persist until someone notices it manually.

Addressing the counter-argument: one reviewer noted that the README sentence's real purpose — explaining why the timeout tests skip the connect fixture — remains fully valid, and that two of the three tests still use the real clock, so this is a single slightly-imprecise adjective. That's all true, which is why this is filed as a nit rather than a blocking issue: the substantive rationale (transport-independence, not racing transport latency) is unchanged and arguably strengthened. But the PR's own description checks "I have added or updated documentation as needed" as incomplete, and the change deliberately updated every other piece of documentation describing these tests; keeping the suite README in step is a one-word fix.

How to fix: in tests/interaction/README.md line 64, drop or adjust the adjective — e.g. "the timeout tests (the timeout machinery is transport-independent and must not race transport latency; the session-level one runs on trio's virtual clock)" or simply "the timeout tests (...)".

"""A read timeout configured on the client applies to requests that do not set their own."""

async def call_tool(ctx: ServerRequestContext, params: types.CallToolRequestParams) -> CallToolResult:
Expand All @@ -96,12 +110,6 @@

server = Server("blocker", on_call_tool=call_tool)

# The one real wall-clock wait in the suite, and it cannot be made effectively zero like the
# per-request timeouts: a session-level timeout also governs the initialize handshake, so the
# value must be long enough for the in-process handshake to complete before the blocked tool
# call waits it out in full. 50ms buys a ~50x safety margin over the handshake's actual
# latency; lowering it only erodes the margin against CI scheduler jitter without saving
# anything perceptible.
async with Client(server, read_timeout_seconds=0.05) as client:
with pytest.raises(MCPError) as exc_info:
await client.call_tool("block", {})
Expand Down
Loading