test: deflake the timing-based daemon and control-plane suites - #1090
Conversation
… suites Every daemon test that failed on CI or under full-suite load and passed on rerun was waiting a fixed real interval for something the code already announces. - `shim-dial-in` "waits for the replacement connection" (#938) waited for the redial inside a second `connect(…, 8_000)`, timing the supervised reconnect against that call's real binding deadline. It now collects the dialer's own `onConnection` announcements and waits for the second one; the `connect()` that follows resolves from the dial's current connection. - `daemon-duty-fence` "closes it on a retry" waited out the fence's real one-second convergence backoff under a 10s `vi.waitFor` inside a 5s test budget it could never spend. - `daemon-duty-drain` slept out the `poolShutdownDrainMs` budget, the release-ack backoff ladder and the loop's "still busy" re-check — and asserted a bound on `Date.now()` elapsed. The daemon already takes a `clock`, so those suites inject one. New `test/fakes/virtual-clock.ts` adds `runVirtual`, which fires the deadlines a promise parks on, and `settle`, which drains the macrotask queue without moving time — so a "nothing further happened" assertion proves nothing CAN happen rather than betting that nothing did in 150ms. `runVirtual` takes a horizon so only the waits a test means to skip are skippable: the daemon arms hour-scale sweeps on the same clock. - `dream-scheduler` "fires onFire" asserted a once-per-second cron had fired behind a fixed 1400ms window — a positive assertion a slow runner failed outright. Now a bounded `vi.waitFor` on the fire itself. Its paired negative test keeps its real window deliberately. - `daemon-serial-gate` "the queue drains on shutdown" had the file's one positive assertion behind a fixed wait; the rest are negative assertions, where slowness only widens the window, and are left alone. Behaviour under test is unchanged and no timeout is merely raised. Per-test wall time: 2413→474, 2615→949, 1960→511, 1512→577, 1431→491, 1412→481, 1400→111 ms. Closes #938
Four concurrency tests in `slack-platform-install.route` set up a row-lock race by sleeping 150ms and hoping the request had reached the lock by then. Too short a sleep does not fail them — it silently runs a different interleaving than the one the test names, so the race the test exists for goes uncovered. They now poll `pg_stat_activity` for a backend genuinely queued on a lock, which is the probe `members.route`, `org-invite-links` and `session-visibility` already use for the same setup. `social-identity-mutation-gate` and `session-visibility.route` already polled the right signal but under tight bounds — a one-second ceiling, and a 25ms interval. Widened and tightened respectively: the poll ends the instant the waiter appears, so a larger bound only buys not failing a correct run on a slow scheduler.
There was a problem hiding this comment.
Approved — I found no blocking regressions in the test-only changes at 62c776891aafb3a7ab577db112a07ea0ced728ae. The database races now wait for actual lock contention, and the daemon timing cases use existing lifecycle signals or bounded virtual time without changing production behavior.
Verification: the modified daemon cases passed locally; daemon and control-plane typechecks, focused ESLint, and Prettier checks passed. Two unchanged Git-dependent serial-gate cases could not run in this sandbox because opening /dev/null is denied, and the control-plane integration suite could not start locally without a container runtime. On the exact reviewed head, both integration shards plus Unit Test, Build, Check, Daemon Store, and Sandbox CI jobs are green.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
Summary
Tests that failed on CI or under full-suite load in the last two days and passed on rerun were all
waiting on the wall clock. This replaces those waits with the signal each one is actually about — a
hook the code already emits, a lock the database can be asked about, or a deadline moved on an
injected clock — so the assertions are about ordering and outcome rather than about how fast the
runner happened to be. No timeout is merely raised, and no behaviour under test changes.
Two of the reported failures needed nothing:
daemon-agent-mention-routinganddaemon-platform-authorshiptimed out becauseDaemon.start()built a real@slack/web-apiclientand dialled slack.com, and #1083 already gave both suites the injected fake.
duty-recompute's"stop() cancels a pending kick" already lost its ~20 ms wait in #1084, which replaced it with
clock.pendingTimers()plussettle(). Both are re-verified here rather than re-fixed.Closes #938.
Per test
daemon/test/shim-dial-in.test.ts— "waits for the replacement connection while a bound channelis reconnecting" (#938). It waited for the redial inside a second
dialer.connect(…, 8_000),which times the supervised reconnect against that call's real binding deadline — the reported
binding timed out after 8000mson a loaded runner. The dialer already announces every boundconnection through
onConnection, so the test now collects them and waits for the second to arrive;the
connect()that follows resolves from the dial's current connection instead of racing anything.daemon/test/daemon-duty-fence.test.ts— "closes it on a retry when the reconcile pass carryingthe fence throws". The fence's convergence retry backs off a real second
(
DUTY_CONVERGE_RETRY_BASE_MS), and the test waited it out under a 10 svi.waitForinside a 5 stest budget it could never spend. The daemon takes a
clock, so the suite injects one and elapsesthe backoff in virtual time; what is asserted is that a retry happens. 1431 ms → 491 ms.
daemon/test/daemon-duty-drain.test.ts— five tests. Every deadline the shutdown drain measures— the
poolShutdownDrainMsbudget, the release-ack backoff ladder, the "still busy, look again"sleep — runs on the daemon's injected clock, so
boot()now supplies one:backoff inside the 5 s budget and asserted
Date.now()elapsed under 6 s — a wall-clock bound ona loaded runner. Both the ladder and the bound are now the injected clock's. 2413 ms → 474 ms.
1960 ms → 511 ms.
2615 ms → 949 ms.
still waiting on a busy group each slept out the loop's 1 s re-check plus a fixed 50/150 ms
"nothing else happened" window. 1512/1412 ms → 577/481 ms.
The negative assertions ("nothing further was acknowledged") no longer sleep a guess: the loop is
parked on a virtual deadline nobody fired, so draining the macrotask queue proves nothing can
happen rather than betting that nothing did in 150 ms.
daemon/test/fakes/virtual-clock.ts(new) is the seam: aClockwhose time moves only onrequest,
runVirtualto fire the deadlines a promise parks on, andsettleto drain the macrotaskqueue without moving time.
runVirtualtakes a horizon so only the waits a test means to skip areskippable — a daemon also arms hour-scale sweeps on the same clock, and one of those firing because
it was the only thing armed would jump virtual time past everything the test is about.
daemon/test/dream-scheduler.test.ts— "fires onFire for the scheduled agent". Slept 1400 ms andthen asserted a once-per-second cron had fired — a positive assertion behind a fixed window, so a
slow runner failed it outright. Now a bounded
vi.waitForon the fire itself. 1400 ms → 111 ms. Thepaired "stops firing once stopped" keeps its real 1200 ms: proving nothing fires needs a window in
which a live job would have, and slowness only widens it.
daemon/test/daemon-serial-gate.test.ts— "the queue drains on shutdown".expect(settled).toBe(true)behind a fixed 10 ms wait — the one positive assertion of that shape in the file. Now a bounded
vi.waitFor. The file's other fixed waits are all negative assertions ("the second prompt must NOThave started"), where a slow runner only widens the window, and are left alone deliberately.
control-plane/test/integration/slack-platform-install.route.test.ts— four concurrency tests.Each set up a row-lock race by sleeping 150 ms and hoping the request had reached the lock. Too short
a sleep does not fail the test; it silently runs a different interleaving than the one the test
names, so the race goes uncovered. They now poll
pg_stat_activityfor a backend genuinely queued ona lock, which is the probe
members.route,org-invite-linksandsession-visibilityalready use.control-plane/test/social-identity-mutation-gate.test.tsandtest/integration/session-visibility.route.test.ts. Both already polled the right signal but withtight ceilings (1 s, and a 25 ms interval). Ceilings widened and the interval tightened — the poll
ends the instant the waiter appears, so a larger bound only buys not failing a correct run.
Verification
Each touched suite three times, plus the whole daemon unit suite and the control-plane unit and
integration suites, plus
typecheck,lint, andformat:check.shim-workspace-files,shim-exec-handlerandworkspace-git-runner-seamfail identically on aclean
origin/mainin this sandbox (9 tests, filesystem/git environment), and are untouched here.