Skip to content

fix(tests): stop pi-lifetime asserting the ceiling on a racing boundary - #6082

Merged
waleedlatif1 merged 1 commit into
stagingfrom
fix/pi-lifetime-boundary-flake
Jul 30, 2026
Merged

fix(tests): stop pi-lifetime asserting the ceiling on a racing boundary#6082
waleedlatif1 merged 1 commit into
stagingfrom
fix/pi-lifetime-boundary-flake

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

pi-lifetime.test.ts > keeps the ceiling when the run outlives it is flaky. It failed CI tonight as AssertionError: expected 5399999 to be 5400000 — 1 failure out of 16,047 tests.

Root cause

The test passed a deadline of exactly PI_SANDBOX_MAX_LIFETIME_MS (90 min), then asserted the resolved lifetime equalled that ceiling:

const timeout = createTimeoutAbortController(90 * 60 * 1000)
expect(resolvePiRunLifetimeMs(timeout.signal)).toBe(PI_SANDBOX_MAX_LIFETIME_MS)

But the resolved lifetime is Math.min(ceiling, remaining), where remaining is getRemainingExecutionMs() = deadline - Date.now(). With the deadline set equal to the ceiling, remaining starts at the ceiling and decays below it the instant any time elapses — so Math.min returns remaining, not the ceiling. The assertion held only when zero milliseconds passed between constructing the controller and reading it. A coin flip weighted by runner speed.

Demonstrated, not guessed

ceiling = 5400000
OLD (== ceiling)       after 5ms -> 5399994  FAIL (off by 6ms)
NEW (ceiling + 60s)    after 5ms -> 5400000  PASS

The old input drifts by exactly the elapsed time. The new one is exact regardless.

The fix also repairs the test's intent

The name says the run outlives the ceiling — but a deadline equal to the ceiling isn't outliving it. Raising the deadline past the ceiling fixes the assertion and the intent together, and it's the case actually worth covering: a run whose deadline exceeds the ceiling should be clamped to the ceiling.

Derived from PI_SANDBOX_MAX_LIFETIME_MS + 60_000 rather than restating 90 * 60 * 1000, so the test can't silently drift if the async execution ceiling moves — which is exactly how the original hardcoded value came to sit on the boundary.

Also drops the old comment's claim that "the provider cap still wins". The provider cap is 24 h, far above the 90 min async ceiling, so the ceiling is what wins — the comment described the opposite of the mechanism.

Notes

Deliberately not reached for vi.useFakeTimers(). Freezing the clock would mask the boundary rather than fix it, and the assertion would still be testing an input that contradicts the test's name. The sibling assertions in this file that compare against the ceiling (lines 103-104, 145) are safe — they use signals with no deadline, so getRemainingExecutionMs returns undefined and the ceiling is returned unconditionally.

Type of Change

  • Bug fix (test correctness / CI flake)

Testing

Fixed test run 5x, 16/16 passing each time. Mechanism verified directly against the real helpers (output above) rather than inferred from a green run — the old version passed most of the time too, so repetition alone proves nothing here.

`keeps the ceiling when the run outlives it` passed a deadline of exactly
`PI_SANDBOX_MAX_LIFETIME_MS`, then asserted the resolved lifetime equalled
that ceiling. But the lifetime is `Math.min(ceiling, deadline - Date.now())`,
so the remaining budget decays below the ceiling as soon as one millisecond
of test time elapses and `Math.min` returns the budget instead. It passed
only when zero milliseconds happened to pass, which is why it failed CI as
`expected 5399999 to be 5400000` — 1 failure in 16,047.

Demonstrated: with the old input, a 5ms delay before the assertion yields
5399994 (off by 6ms); with a deadline 60s past the ceiling it yields
5400000 exactly, regardless of elapsed time.

An equal deadline was also not the case the test name describes — the run
has to *outlive* the ceiling, not match it — so raising it past the ceiling
fixes the assertion and the intent together. Derives the input from
`PI_SANDBOX_MAX_LIFETIME_MS` rather than restating 90 minutes, so the test
cannot drift if the async execution ceiling moves.

Also drops the old comment's claim that "the provider cap still wins": the
provider cap is 24h, far above the 90m async ceiling, so the ceiling is what
wins here.
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 30, 2026 1:37am

Request Review

@cursor

cursor Bot commented Jul 30, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Test-only change with no production code paths affected.

Overview
Fixes a flaky CI failure in keeps the ceiling when the run outlives it (expected 5399999 to be 5400000).

The test now uses a timeout deadline of PI_SANDBOX_MAX_LIFETIME_MS + 60_000 instead of exactly the ceiling (previously 90 * 60 * 1000). Because resolvePiRunLifetimeMs takes Math.min(ceiling, remaining) and remaining is deadline - Date.now(), a deadline equal to the ceiling decays below the cap as soon as any real time passes, so the assertion only passed when the read happened in the same millisecond. A deadline strictly past the ceiling matches the scenario name and keeps remaining above the cap so the expectation is stable.

Comments were updated to document that boundary behavior and drop the incorrect note about the provider cap winning over the async ceiling.

Reviewed by Cursor Bugbot for commit f40c410. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes a flaky unit test by setting the abort deadline strictly above the PI sandbox lifetime ceiling so Math.min(ceiling, remaining) stably returns the ceiling.

  • In keeps the ceiling when the run outlives it, deadline is now PI_SANDBOX_MAX_LIFETIME_MS + 60_000 instead of exactly the ceiling.
  • Comment updated to describe the race (deadline - Date.now() decay) and correct that the async ceiling wins, not the provider cap.
  • Production code unchanged; test-only flake fix.

Confidence Score: 5/5

Safe to merge; the change only stabilizes a flaky test assertion with no production behavior change.

The deadline is derived from the same constant the assertion checks, so remaining budget stays above the ceiling for the duration of the test and the equality holds without fake timers.

Important Files Changed

Filename Overview
apps/sim/lib/execution/remote-sandbox/pi-lifetime.test.ts Correctly raises the test deadline past the lifetime ceiling so the clamp assertion is stable and matches the test name.

Reviews (1): Last reviewed commit: "fix(tests): stop pi-lifetime asserting t..." | Re-trigger Greptile

@waleedlatif1
waleedlatif1 merged commit 4b239fa into staging Jul 30, 2026
21 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/pi-lifetime-boundary-flake branch July 30, 2026 01:40
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.

1 participant