Skip to content

test(mcp): fire MCP client test deadlines on conditions instead of wall-clock time - #44166

Merged
mateo-berri merged 3 commits into
mainfrom
litellm_deflake_20261002
Oct 2, 2026
Merged

mateo-berri merged 3 commits into
mainfrom
litellm_deflake_20261002

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • Two MCP client unit tests flaked on the mcp-integration shard in the last 24h
  • Both raced a real 0.2s timer against in-memory work, so a stalled runner lost the race

How it solves it:

  • The outer-deadline test now fires the caller's deadline once the tool call is in flight
  • The SSE read-failure test runs on a virtual clock that only jumps when the loop is idle
  • No production code and no assertions changed

User Flow

Before: a contributor's unrelated PR goes red on mcp-integration and they have to re-run it

  1. They push a commit that touches nothing under the MCP client
  2. The mcp-integration / Run tests job fails on test_outer_deadline_delivers_session_termination with assert started.is_set(), or on test_sse_read_failure_is_preserved with MCPError: Request 'tools/list' timed out
  3. They re-run the job with no changes and it passes

After: the same commit gets a green mcp-integration job on the first attempt

  1. They push the same commit
  2. Both tests reach their assertions no matter how slowly the runner schedules the event loop
  3. The job passes on the first attempt

Flake evidence (run date 2026-10-02)

tests/unit/experimental_mcp_client/test_mcp_client.py::test_outer_deadline_delivers_session_termination

Flaked 7 times in the window, always on the mcp-integration / Run tests job, attempt 1, on PRs that do not touch the MCP client

Same SHA flipped: https://github.com/BerriAI/litellm/actions/runs/36972046831 failed [failure-True-True] on attempt 1 and passed on attempt 2 at 3f066ee987

Other failures, attempt 1 each: https://github.com/BerriAI/litellm/actions/runs/36874909254, https://github.com/BerriAI/litellm/actions/runs/36877132039, https://github.com/BerriAI/litellm/actions/runs/36895408264, https://github.com/BerriAI/litellm/actions/runs/36896660820 and https://github.com/BerriAI/litellm/actions/runs/36927552895 (all [failure-True-True]), plus https://github.com/BerriAI/litellm/actions/runs/36895728486 ([ok-True-False])

Mechanism: wall-clock dependence. The caller wrapped the tool call in a real anyio.fail_after(0.2), and the fake server only sets started after initialize and tools/list have both completed. In the failing CI run the test took 0.95s where its siblings took 0.2s, so the runner stalled the loop and the deadline fired during session setup, before the tool request was ever sent. Injecting a 0.25s synchronous stall into the fake server's initialize reply reproduces the same assert started.is_set() failure on every parameter set

Fix: the deadline is still a real fail_after scope, but it starts with no limit and the fake server expires it the moment the slow tool call reaches it. Cancellation now always lands while the request is in flight, which is the case the test exists to cover. The 6.5s cleanup budget is now measured from the moment cancellation fires, so a slow setup no longer eats into it, and a separate 20s outer bound makes a regression where the tool call never reaches the server fail before the client's 30s timeout

tests/unit/experimental_mcp_client/test_mcp_client.py::test_sse_read_failure_is_preserved

Flaked 2 times in the window, attempt 1 each, on PRs that do not touch the MCP client: https://github.com/BerriAI/litellm/actions/runs/36959115506 and https://github.com/BerriAI/litellm/actions/runs/36967968247. It passed on every other mcp-integration run in the window, including runs at neighboring main SHAs

Mechanism: wall-clock dependence. The client's 0.2s read timeout is real time, and it races delivery of the queued ReadError through the SSE reader to the client's stream-error handler. The client only maps the timeout back to the original read error once that handler has run, so when a stalled loop lets the timer win, the raw Request 'tools/list' timed out escapes. The CI log shows the handler running only during cleanup, after the timeout. Injecting a 0.25s stall right after the error is queued reproduces it

Fix: the test runs on a manual-clock event loop whose selector advances the clock to the next timer only when no callback or I/O is ready, the same idea as trio's autojump clock. The read timeout therefore fires only after the stream error has been fully delivered, regardless of runner speed. The 0.25s stall no longer affects it. The original 3s wait_for stays as a virtual-time bound, so a regression that leaves the operation waiting forever fails instead of hanging the job

Mutation checks

Skipping the transport exit that sends the session DELETE makes all 12 outer-deadline cases fail on deleted. Letting call_tool swallow the cancellation makes the raise_on_error=False cases fail on the expected TimeoutError. Disabling the client's stream-error mapping makes the SSE test fail with an MCPError instead of the read error. Raising the client's session cleanup window from 5s to 8s makes every hang case fail on the cleanup budget. Removing the server's in-flight signal makes the outer-deadline test fail in 28s instead of 30s, and queueing no SSE error makes the SSE test fail in 7s instead of hanging. All mutations were reverted

Not flakes in the window

Real breakages, failing on every attempt: tests/proxy_migration_tests/test_prisma_toolchain.py::test_db_push_timeout_hint_names_the_per_command_budget (Lens data safety check refuses prisma db push), the two anthropic event-loop-stall tests in tests/unit/proxy/pass_through_endpoints/test_streaming_handler.py, and the docs check flagging LITELLM_DISABLE_LAZY_ROUTES as undocumented. Infra: one dependency install timed out at 8 minutes. None of these were touched

Other open deflake PRs

#43437, #42936 and #41219 each carry an approval, so they are left untouched under the human-reviewed exception and this PR is the new rolling PR

Pre-Submission checklist

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. uv run pytest tests/unit/<your_test_file>.py -v. Leave the suites (make test-unit-*, make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Type

✅ Test

Caveats (if any)

Low

  • Cleanup budget still runs on the real clock, unavoidably
    • A stall over about 1.5s inside the 5s cleanup window would fail it
    • Setup, where every CI failure stalled, no longer counts toward it
  • The 20s hang guard counts setup, so a 14s+ setup stall trips it
  • Optional-discovery MCP candidate passed 20/20 locally, left unchanged as unreproduced

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

Link to Devin session: https://app.devin.ai/sessions/26bdd1e8bf8149a48dfe897164ee8b33
Open in Devin Desktop: https://app.devin.ai/desktop/session/26bdd1e8bf8149a48dfe897164ee8b33?variant=devin


Note

Low Risk
Test-only changes to timing harnesses; no production MCP client behavior is modified.

Overview
Deflakes two MCP client unit tests that were racing real ~0.2s timers against mocked I/O on slow CI runners. Production code and assertions are unchanged.

Adds _AutojumpSelector / _AutojumpClockLoop: a manual asyncio clock that advances only when the loop is idle (trio-style autojump). test_sse_read_failure_is_preserved now runs off pytest-asyncio on this loop so the client read timeout cannot beat delivery of the queued ReadError.

test_outer_deadline_delivers_session_termination no longer uses a fixed fail_after(0.2) from the start. The mock server sets the caller’s anyio deadline once the slow tools/call is in flight, so cancellation always happens during the in-flight request. Cleanup timing is measured from that fire point, with a separate 20s outer bound if the server never signals.

_ManualClockLoop now accepts an optional custom selector for the autojump wiring.

Reviewed by Cursor Bugbot for commit 03e4f67. Bugbot is set up for automated code reviews on this repo. Configure here.

…ll-clock time

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
devin-ai-integration Bot requested a review from a team October 2, 2026 10:47
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Low risk] Test infrastructure changes to control timing in unit tests.

The PR appears safe to merge based on the reviewed changes

Findings

  1. P2 Unit test uses clock ▶

Summary

The PR makes two MCP client tests less dependent on runner scheduling.

  • The outer-deadline test triggers cancellation when the tool call is in flight and retains a separate failure bound.
  • The SSE read-failure test uses a virtual clock while retaining a bounded wait.

Reviews (3) · Last reviewed commit: "test(mcp): measure outer-deadline cleanu..."

Comment thread tests/unit/experimental_mcp_client/test_mcp_client.py
Comment thread tests/unit/experimental_mcp_client/test_mcp_client.py
@codecov

codecov Bot commented Oct 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…riggered deadline

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai

Comment thread tests/unit/experimental_mcp_client/test_mcp_client.py Outdated
def select(self, timeout: float | None = None) -> list[tuple[selectors.SelectorKey, int]]:
ready: Final = super().select(0)
if ready or timeout == 0:
return ready

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Unit test uses clock This selector advances a manual clock, which the SSE test uses to drive its deadline. The tests/unit guidance requires unit tests to use no clock, so this repository requirement must be satisfied before merging

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The guidance bans real wall-clock dependence. This loop is an injected virtual clock, the same fake _ManualClockLoop this file already uses, so no real time is read

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're right. This does not introduce real wall-clock dependence: _AutojumpSelector only advances the injected _ManualClockLoop's virtual time when no I/O or callback is ready, and no wall-clock value is read. That is consistent with the existing fake-clock pattern in this file. My earlier comment was incorrect; I withdraw it.

…ot setup

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 03e4f67. Configure here.

@mateo-berri mateo-berri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mateo-berri
mateo-berri merged commit 615ed79 into main Oct 2, 2026
100 of 102 checks passed
@mateo-berri
mateo-berri deleted the litellm_deflake_20261002 branch October 2, 2026 12:44
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