fix: sleep past the settle quiet deadline instead of onto it (#1306)#1308
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Exact-head review ( The new quiet-deadline delay is not bounded by the loop’s overall deadline. With an injected exact clock, stable tree, Make the delay global-budget-aware so the epsilon cannot consume the only remaining capture opportunity. Add timeout boundary cases at/just above the quiet deadline. Also strengthen the motivating regression by modeling |
|
Fixed in Budget boundary. Confirmed exactly as described: const lastUsefulWakeMs = params.deadlineMs - params.nowMs - 1;
return lastUsefulWakeMs > 0 ? Math.min(cadenceMs, lastUsefulWakeMs) : cadenceMs;Where the budget is the tighter constraint it wakes just inside the deadline and takes the capture the plain cadence would have taken; the epsilon still applies whenever there's room. The recovery-reset branch shares the helper and passes its post-reset Strengthened regression test. You were right that asserting Each test now catches exactly one defect, verified against all three versions:
Boundary cases on both sides, as asked: 990 tests green across |
|
Exact-head re-review (
Terminate or bound the no-useful-wake branch to the remaining deadline, and assert |
|
Fixed in Bounded the branch. It now sleeps out only what remains of the budget: if (lastUsefulWakeMs > 0) return Math.min(cadenceMs, lastUsefulWakeMs);
// No further capture can land inside the budget, so sleep out what remains of
// it rather than a full cadence past it.
return Math.max(0, params.deadlineMs - params.nowMs);The Test now sees the overrun. You were right that asserting rejection alone missed it. One wrinkle on the suggested Happy to add Each test still catches exactly one defect, verified against every version:
990 tests green across |
|
Exact-head re-review ( Combine the PR’s own 1ms-undershooting clock with Current tests cover undershoot with a roomy budget and the 301ms boundary with an exact clock separately, so they miss the interaction. Make the final-budget branch guarantee time progress under the modeled undershoot and add the combined undershoot + 301ms regression. CI is otherwise green; no |
runStableCaptureLoop derived its whole cadence from pollMs = min(300, max(25, quietMs)), so pollMs === quietMs for every quietMs in [25, 300]. The loop settles once two identical captures span quietMs, and the gap it measures is exactly one sleep(pollMs) plus capture time — so across that entire range "settle at capture 2" rode a 0ms margin. Node decides that margin, not the UI: setTimeout(n) advances Date.now() by only n-1 in 0.13% of calls idle and 0.63% under load, because libuv times the sleep on the monotonic loop clock while now() reads the wall clock. On an undershoot the loop spends a wasted extra capture and poll before settling. The sleep is now deadline-aware: while the quiet deadline is further away than one poll the cadence is unchanged, so changes are still noticed promptly; once it is within reach, the loop sleeps to just past it. The capture that decides settled always spans the window. Effects: --settle-quiet in [25,300] now settles at capture 2 rather than 2-or-3 by coin flip, and the default 500ms window settles at ~502ms instead of ~600ms with the same 3 captures. No behaviour change beyond timing; the quiet-window semantics are identical.
The review is right: the epsilon could spend the very capture it exists to land. With quietMs=300 and timeoutMs=301, capture 1 asked for a 302ms sleep, woke past the 301ms deadline, and the loop exited with one capture — where the old 300ms cadence settled. waitedMs could exceed timeoutMs too, and the recovery-reset branch shared the same helper. stableCaptureDelayMs now takes the deadline and never wakes past it: the loop only runs again while now < deadline, so where the budget is the tighter constraint it wakes just inside it and takes the capture the plain cadence would have taken. The epsilon still applies whenever the budget has room. The motivating regression test is also strengthened per review, from asserting the requested delay is > 300 to modelling the defect itself: an injected clock whose sleep advances now by ms - 1, asserting the observable two-capture settle. Each test now catches exactly one defect: vs main (bd62502): undershoot FAILS, boundary passes vs addbcc3: undershoot passes, boundary FAILS vs this commit: both pass Boundary cases added on both sides of the edge: timeoutMs one millisecond past the quiet window (settles at capture 2, waitedMs within budget), and equal to it (cannot settle — the window has to elapse inside the budget).
…act (P2) The review is right, and it caught my own contract being violated one branch below where I wrote it. stableCaptureDelayMs claimed it "never wakes past the deadline", then exempted the no-useful-wake case: with quietMs=300 and timeoutMs=300, capture 2 cannot settle, the helper returned a full 25ms cadence and the loop exited at waitedMs=324 where the old code exited at 300. That branch now sleeps out only what remains of the budget. The loop is about to exit either way; overrunning just reports a wait longer than the caller asked for. The equal-boundary test asserted rejection only, so it could not see the overrun — it now asserts the injected clock's elapsed time against the budget (the timeout error carries captures and nodeCount, but not waitedMs, so the clock is the honest observable). It fails on 59c9363 with "elapsed 324ms", exactly as the review predicted. Each test still catches exactly one defect: vs main (bd62502): undershoot FAILS vs addbcc3: 301-budget boundary FAILS vs 59c9363: equal-boundary overrun FAILS vs this commit: all pass
…not (P2) The review found the real hazard behind the boundary work: not an overrun but a hot loop. Combine the 1ms-undershooting clock with quietMs=300 and timeoutMs=301 and capture 2 lands at 299ms; lastUsefulWakeMs is then 1, the helper asks for 1ms, and the skew eats all of it. The clock never advances, the deadline never arrives, and the loop captures forever — hammering the device at exactly the skew this PR exists to absorb. A delay that buys no time is the bug. Every delay stableCaptureDelayMs returns is now at least QUIET_DEADLINE_EPSILON_MS, which is what makes the loop terminate under the modelled undershoot. Where the budget cannot afford a delay that both progresses and stays inside it, there is no capture left to place: it returns 0 and the loop stops instead of spinning. That also subsumes the previous no-useful-wake branch, so the equal-boundary case now stops at 299ms rather than sleeping out the remainder. The undershooting test clock now yields to the event loop like a real sleep. Without that the spin starves the runner on microtasks and the regression hangs the whole job instead of failing; with it, the reviewed head fails the new test cleanly with "Test timed out in 6000ms". Rebased onto 9b5f333. vs main: undershoot FAILS vs b6261da: 301-budget boundary FAILS vs 8c963fc: equal-boundary overrun FAILS vs 4f6beb8: combined undershoot+301 spin FAILS (timeout) vs this commit: all pass
thymikee
force-pushed
the
claude/settle-quiet-deadline-sleep
branch
from
38ba4ca to
d14000d
Compare
|
Fixed in Confirmed the trace precisely: capture 2 lands at 299ms, A delay that buys no time is the bug. Every delay const lastUsefulWakeMs = params.deadlineMs - params.nowMs - 1;
if (lastUsefulWakeMs < QUIET_DEADLINE_EPSILON_MS) return 0;
return Math.min(cadenceMs, lastUsefulWakeMs);That subsumes the previous no-useful-wake branch, so the equal-boundary case now stops at 299ms instead of sleeping out the remainder — strictly tighter than both the old code and my last revision. One thing worth flagging about the test. My first version of the combined regression didn't fail on the broken code — it hung the entire runner. The undershooting clock resolved synchronously, so the spin starved the event loop on microtasks and vitest's timeout could never fire; Worth noting the production shape differs: real Each test still catches exactly one defect:
1014 tests green across |
|
Exact-head re-review ( At The new regression currently asserts rejection, cementing the false timeout. Change the final-budget branch to spend the remaining budget in a way that guarantees progress, and make the combined test assert settle under undershoot while the exact-clock equal-boundary case still rejects. No |
|
Fixed in The final skew-sized boundary now spends the full remaining budget instead of stopping early or requesting an ineffective 1ms sleep. At
The exact 300ms boundary still times out within budget, and neither path spins or overruns. The combined public |
thymikee
added
the
ready-for-human
|
1 participant
Closes #1306.
Summary
runStableCaptureLoopderives its whole cadence fromso
pollMs === quietMsfor everyquietMsin [25, 300]. The loop settles once two identical captures spanquietMs, and the gap it measures at capture #2 is exactly onesleep(pollMs)plus capture time. Across that whole range, "settle at capture 2" rides a 0ms margin.Node decides that margin, not the UI.
setTimeout(n)advancesDate.now()by onlyn-1in 0.13% of calls idle and 0.63% under load (measured, 4000 samples, darwin/Node 26) — libuv times the sleep on the cached monotonic loop clock whilenow()reads the wall clock (runtime-common.ts:16-23). On an undershoot the loop just spends a wasted extra capture + poll before settling.Change
The sleep is now deadline-aware. While the quiet deadline is further away than one poll, the cadence is unchanged — changes are still noticed promptly, which matters because detecting a change early is what starts the quiet window early. Once the deadline is within reach, the loop sleeps to just past it instead, so the capture that decides
settledalways spans the window.The
STABLE_MIN_POLL_MSfloor keeps--settle-quiet 0from turning into a 2ms hot capture loop against a changing UI.Effects
--settle-quietin [25, 300] settles at capture 2 deterministically, instead of 2-or-3 by coin flip — one capture cheaper on the 0.13–0.63% of settles that lost the flip.quietMs". Nothing but timing moves.Shared by
--settle(press/fill/click/longpress) andwait stable, so both benefit.Tests
New unit test asserts the invariant directly — with
quietMsequal to the 300ms poll cadence, the loop must sleep strictly past the window:Verified it's a real guard: against unpatched production it fails with
settling capture must clear the window, slept 300ms, and passes with the fix. The existing fake-clock capture-count assertions (3 and 4) are unchanged — those windows are wider than one poll, so only theirwaitedMsimproves.src/commands/interaction/,src/commands/capture/,src/daemon/handlers/__tests__/: 988 tests green. Typecheck, lint, format clean.Provenance
Surfaced while diagnosing the
settle-observation.test.tsCI flake (#1307), where the same skew flipped a test assertion because the fixture scripted an exact capture count. That fix is test-only and already separate; this is the production half, deliberately not bundled with it.