test(state): remove the wall-clock race from sync legacy-marker recovery - #8948
Conversation
The synchronous legacy short-token recovery case raced the real 1-second acquisition budget: options() sets timeoutMs to 1000 and the sync path defaults to performance.now(), so marker recovery on a loaded CI runner can exceed the deadline and fail with "Timed out waiting for sandbox mutation lock for 'alpha'" (PR NVIDIA#8900, CLI shard 11). Drive the deadline from a stepping clock instead. Recovery success no longer depends on wall-clock load, and a genuine strand regression still exhausts the budget after 1,000 clock reads, so it fails with the real timeout error instead of blocking the synchronous worker the way a fully pinned clock would. Signed-off-by: Kushagar Garg <dreamstick909@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLifecycle lock recovery tests now use deterministic, incrementing monotonic clocks. The clocks allow filesystem recovery work and bound simulated timeout progression. ChangesLifecycle lock recovery
Estimated code review effort: 2 (Simple) | ~5 minutes Mergeability Score: ⚪ Minimal · up to This change makes a flaky recovery test deterministic without changing product behavior; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
…nt observation CLI shard 4 on this PR's own run failed "preserves a replacement main lock published during stale recovery": the case runs acquisition against a real 50 ms budget, and on a loaded runner the deadline can expire before stale recovery reaches its first rename, so the injected replacement is never published and the final assertion observes the planted stale token. Drive the deadline from the file's existing stepping-clock idiom so the recovery sequence is load-independent while the expected timeout still fires once the budget's clock reads are exhausted. Signed-off-by: Kushagar Garg <dreamstick909@gmail.com>
3 participants
Summary
The CLI test
does not strand synchronous recovery behind an expired legacy short-token markerraces a real 1-second clock:options()setstimeoutMs: 1000and the synchronous acquisition path defaultsmonotonicNowtoperformance.now(), so legacy-marker recovery doing real filesystem work can exceed the budget on a loaded CI runner. That produced the shard-11 failureTimed out waiting for sandbox mutation lock for 'alpha'on #8900 after its transient-failure retry was exhausted. This change drives the deadline from a stepping clock, so recovery success no longer depends on wall-clock load while a genuine strand regression still exhausts the budget after 1,000 clock reads and fails with the real timeout error.Changes
src/lib/state/mcp-lifecycle-lock-acquisition.test.ts: inject a steppingmonotonicNowinto the synchronous legacy short-token recovery case, with a comment recording the CI evidence. The asynchronous sibling pins its clock to0; the synchronous case steps instead because its sleep isAtomics.wait, and a fully pinned clock would let a future strand regression block the vitest worker where a stepping clock keeps that failure bounded at 1,000 clock reads with the real timeout error.test/mcp-lifecycle-lock.test.ts: apply the same stepping-clock idiom (already used elsewhere in this file) topreserves a replacement main lock published during stale recovery, which raced a real 50 ms budget the same way and failed on this PR's own CI shard 4 withobserved-stale-tokenwhen the deadline expired before stale recovery reached its first rename.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededVerification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/state/mcp-lifecycle-lock-acquisition.test.ts→ 28 passed, three consecutive runs;npx vitest run --project integration test/mcp-lifecycle-lock.test.ts→ 44 passed, five consecutive runs plus one run on the merged treenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable; one-case test edit with no runtime or repo-wide validation change.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Kushagar Garg dreamstick909@gmail.com