Skip to content

test(e2e): wait for the call to reach the server in protocol:timeout:max-total - #3130

Open
JQ100 wants to merge 1 commit into
modelcontextprotocol:mainfrom
JQ100:fix/max-total-sse-race
Open

JQ100 wants to merge 1 commit into
modelcontextprotocol:mainfrom
JQ100:fix/max-total-sse-race

Conversation

@JQ100

@JQ100 JQ100 commented Oct 29, 2026

Copy link
Copy Markdown

Fixes #3045.

Before: protocol:timeout:max-total switched to fake timers, started tools/call, and advanced the clock straight away. Over SSE the request is a real HTTP POST that fake timers don't wait for, so when it landed late the 500ms per-request timeout fired before any progress was sent. The SSE variants then failed intermittently with expected 0 to be greater than or equal to 3, and when they did, the test never exercised maxTotalTimeout.

After: the tool handler resolves a promise as soon as it starts, and the test awaits it before advancing the clock. Awaiting a promise doesn't move the fake clock, unlike vi.waitFor, which advances fake timers while it polls. Test-only change, 9 lines, no changeset.

Motivation and Context

On a clean clone on macOS, pnpm test:all intermittently fails on this test, while CI on main is green. Details and the original investigation are in #3045.

One correction to that issue: it says the failure is followed by an unhandled SdkError: Request timed out. max-total attaches call.catch(() => {}), so that rejection isn't from this test. It comes from protocol:timeout:reset-on-progress (data: { timeout: 200 }), which leaves 1–2 unhandled rejections on every run on macOS, with or without this change. That's a separate problem and I've left it out of this PR; happy to open an issue for it.

How Has This Been Tested?

On macOS (Darwin 27), Node 24.16.0, from main @ 39ae8e61:

  • Measured: pnpm vitest run scenarios/protocol.test.ts -t "timeout:basic|timeout:max-total" in test/e2e, repeated. Before: 5 of 20 SSE variants failed (10 runs). After: 0 of 40 (20 runs).
  • Forced: a temporary patch delayed only the tools/call POST by 50ms of real time (via a setTimeout captured before vi.useFakeTimers()). Without this change both SSE variants failed on 3 of 3 runs; with it all 12 variants passed on 3 of 3 runs. The patch isn't part of this PR.
  • pnpm lint:all and pnpm --filter @modelcontextprotocol/test-e2e typecheck pass. In pnpm test:all every test passes; vitest still exits 1 because of the reset-on-progress unhandled rejections described above.

…max-total

The test switched to fake timers and advanced the clock immediately
after starting tools/call. Over SSE the request is a real HTTP POST
that fake timers do not wait for, so when it was slow to land the
500ms per-request timeout fired before any progress was sent, and the
test failed without ever exercising maxTotalTimeout.

Resolve a promise when the tool handler starts and await it before
advancing. Awaiting a promise does not move the fake clock, unlike
vi.waitFor, which advances fake timers while it polls.

On macOS, -t "timeout:basic|timeout:max-total" failed 5 of 20 SSE
variants before this change and 0 of 40 after. Delaying only the
tools/call POST by 50ms of real time fails both SSE variants on every
run without the change and passes all 12 variants with it.

Fixes modelcontextprotocol#3045
@JQ100
JQ100 requested a review from a team as a code owner October 29, 2026 20:32
@changeset-bot

changeset-bot Bot commented Oct 29, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 61396ea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Oct 29, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3130

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3130

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3130

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3130

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3130

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3130

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3130

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3130

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3130

commit: 61396ea

@arhancanli

Copy link
Copy Markdown

Read 61396ea (not run). The reasoning holds: the client-side timers are armed while the clock is frozen, so awaiting callReceived can only let the POST land, and unlike vi.waitFor it does not advance fake time.

One failure mode: callReceived has no timeout and no reject path. If the call never reaches the handler (schema rejection of delayMs/steps, a transport bug, a session that fails to open), the test hangs on a promise that never settles with fake timers on, so it dies on the suite-level timeout with no hint of why. A cheap guard is await Promise.race([callReceived, call.then(() => { throw new Error('call settled before the handler ran'); })]), which turns that into an immediate named failure (call already has a catch attached, so use a separate handle for the race).

Question: markCallReceived is also invoked on the stdio and in-memory variants where the ordering was never a problem. Do you want to keep it unconditional so all variants share one code path (my preference), or gate it to SSE?

@claude claude Bot added the v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes label Oct 30, 2026

This branch has not been deployed

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

Labels

v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v2] e2e protocol:timeout:max-total is flaky over SSE: fake timers advance before the request reaches the server

2 participants