Skip to content

[v2] e2e protocol:timeout:reset-on-progress leaves unhandled rejections on its known-failing SSE variants, so vitest exits 1 on macOS #3170

Open

Description

@JQ100

What happens

On macOS, pnpm test:all reports every test as passed or expected-fail, but vitest still exits 1 because of unhandled rejections:

Unhandled Rejection
SdkError: Request timed out
 ❯ timeoutHandler ../../packages/core-internal/src/shared/protocol.ts:1583:49
 ...
 ❯ FakeTimers.advanceTimersByTime
 ❯ checkCallback          (vi.waitFor)
Serialized Error: { code: 'REQUEST_TIMEOUT', data: { timeout: 200 } }

Upstream CI on Linux runs the same suite with the same expected failures and reports no unhandled errors, so this only shows up locally.

Where it comes from

protocol:timeout:reset-on-progress lists sse under knownFailures in test/e2e/requirements.ts (the SSE batching race shared with protocol:progress:callback). On those two variants the test fails as expected at:

await vi.waitFor(() => expect(received.length).toBeGreaterThanOrEqual(i));

While vi.waitFor polls, it advances the fake clock, so the 200ms request timeout fires and call rejects. The test has already left the path that reaches await call, and nothing else handles call, so the rejection is reported as unhandled.

Isolating by transport (-t in test/e2e, main + #3130, macOS, Node 24.16.0, 3 runs each):

Variants Result
reset-on-progress [sse ...] only 2 expected fail, 1–2 unhandled errors every run
reset-on-progress, all other transports 10 passed, no errors

Possible fix

Attach a handler right after call is created, as protocol:timeout:max-total already does:

call.catch(() => {});

The passing variants still await call and assert on the result, so nothing is lost. I tried it locally: reset-on-progress still gives 10 passed and 2 expected fail with no unhandled errors (3 runs), and the full e2e suite exits 0 (3126 passed, 141 expected fail).

I haven't looked into why Linux doesn't report these rejections. Happy to open a one-line test-only PR once #3130 is settled.

Context

Found while working on #3045 / #3130. That issue attributed an unhandled Request timed out to max-total; it was this test, as the #3130 description notes.

Activity

  1. added
    v2Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes
    v1Issues / PRs related to v1.x
    on Nov 10, 2026
  2. media-ctrl commented on Nov 11, 2026

    @media-ctrl

    The handler belongs at creation, not in the failure branch, and the timing is the reason. The known-failure branch is entered after the first poll iteration, and the 200ms timer can fire before the poll that would have reached it. Anything attached later competes with the rejection it is meant to catch.

    Two refinements on the fix itself, since it will live in the suite for a long time.

    Record the rejection instead of discarding it. A handler that swallows whatever arrives, catch(() => {}), also swallows a real protocol failure that happens to surface on that promise, and the suite loses the only signal it had. Capturing the reason and asserting on it in the known-failure branch keeps the test honest: it still proves the expected outcome is a timeout, and a different rejection fails the run with a useful message rather than being absorbed. The variant list already says which cases are expected to fail, so the assertion is adding no new knowledge, only keeping it checkable.

    On the macOS and Linux difference, I would not guess without one measurement, because there are at least two candidates that look identical from outside. The first is harness configuration: pool mode and the settings that govern unhandled errors differ between runs, and a local config that relaxes them would hide the rejection locally rather than creating it locally, which is the opposite of what you see. The second is attribution: whether the rejection is stamped to the test that owns the timer depends on when the timer callback runs relative to the test boundary, and a slower or faster platform can move a rejection from inside a test to after it. Printing process.platform and the owning test name from an unhandledRejection hook, then comparing local and CI, separates them in one run and costs less than reasoning about which is likelier.

    The broader point deserves a line in the PR description. A suite that exits nonzero for environmental reasons trains reviewers to ignore the exit code, and the next real unhandled rejection arrives in silence. Fixing this one is what restores the signal.

    Your read of #3045 is worth stating plainly in the PR: the earlier attribution to max-total was wrong, and the evidence is that isolating the variants by transport reproduces the failures only on the SSE pair that the requirements file already marks as known failures. That is a stronger justification for a test-only change than the one-line diff suggests.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    v1Issues / PRs related to v1.xv2Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions