Fix the session-build-semaphore timeout test flake

[jahands]

What failed

session-build-semaphore.test.ts › "proceeds without a slot when the queue wait exceeds the timeout":

AssertionError: expected 9 to be greater than or equal to 10
    expect(result.waitMs).toBeGreaterThanOrEqual(10);

The test starts a real 10ms setTimeout, waits for it to fire, and asserts that Date.now() advanced by at least 10ms. Sometimes it only advances 9ms.

Why

Node's setTimeout measures its delay from libuv's cached loop time, which is refreshed once per event-loop iteration, not from the moment setTimeout is called. Sync work done earlier in the same iteration (here, the Promise.all and expect calls before acquireBuildSlot(10)) is counted against the 10ms. So the timer can fire when only 9ms of wall-clock has passed since requestedAt. Reproduced locally on Node 22 at ~5 in 2000 runs. It is a timing flake in the test; the semaphore itself is fine.

Fix

Drive the timeout with vi.useFakeTimers() + vi.advanceTimersByTime(10) so waitMs is exactly 10 every run, and reset to real timers in afterEach.

No changeset: @executor-js/cloud is private and the change is test-only.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: aaf3c110-bcc3-4d08-8a18-048d237c15fa

📥 Commits

Reviewing files that changed from the base of the PR and between caa0391 and 4549f13.

📒 Files selected for processing (1)

• apps/cloud/src/mcp/session-build-semaphore.test.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The semaphore timeout test now uses Vitest fake timers, restores real timers after each test, and asserts an exact 10ms timeout result.

Changes

Semaphore timeout testing

Layer / File(s)	Summary
Deterministic timeout assertion apps/cloud/src/mcp/session-build-semaphore.test.ts	The test imports vi, enables fake timers for the timeout case, advances time by 10ms, and checks the exact timeout result. An afterEach hook restores real timers.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Merge Risk: ⚪ Minimal · up to 4549f

This test-only change makes the semaphore timeout assertion deterministic without changing production behavior; no actionable merge-blocking risk remains.

Suggested reviewers: rhyssullivan

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing the intermittent session-build semaphore timeout test failure.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/semaphore-timeout-test-flake

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}