Skip to content

Commit 59c9363

Browse files
committed
fix: bound the quiet-deadline sleep by the loop's own budget (P2 review)
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).
1 parent addbcc3 commit 59c9363

2 files changed

Lines changed: 86 additions & 18 deletions

File tree

src/commands/interaction/runtime/selector-read-stable.test.ts

Lines changed: 67 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -43,23 +43,34 @@ test('runtime wait stable settles after two unchanged captures', async () => {
4343
assert.equal(captures, 3);
4444
});
4545

46-
test('runtime wait stable wakes past the quiet deadline instead of onto it', async () => {
47-
const snapshot = selectorReadSnapshot();
48-
const sleeps: number[] = [];
46+
// A clock whose sleep lands `undershootMs` short of what was asked, modelling
47+
// the real defect: `setTimeout(n)` can advance `Date.now()` by only n-1,
48+
// because libuv times the sleep on the monotonic loop clock while `now()` reads
49+
// the wall clock.
50+
function createUndershootingClock(undershootMs = 1): {
51+
now: () => number;
52+
sleep: (ms: number) => Promise<void>;
53+
} {
4954
let elapsed = 0;
50-
const clock = {
55+
return {
5156
now: () => elapsed,
5257
sleep: async (ms: number) => {
53-
sleeps.push(ms);
54-
elapsed += ms;
58+
elapsed += Math.max(0, ms - undershootMs);
5559
},
5660
};
57-
let captures = 0;
61+
}
62+
63+
function createStableCaptureDevice(clock: {
64+
now: () => number;
65+
sleep: (ms: number) => Promise<void>;
66+
}) {
67+
const snapshot = selectorReadSnapshot();
68+
const counter = { captures: 0 };
5869
const device = createAgentDevice({
5970
backend: {
6071
platform: 'ios',
6172
captureSnapshot: async () => {
62-
captures += 1;
73+
counter.captures += 1;
6374
return { snapshot };
6475
},
6576
} satisfies AgentDeviceBackend,
@@ -68,21 +79,63 @@ test('runtime wait stable wakes past the quiet deadline instead of onto it', asy
6879
policy: localCommandPolicy(),
6980
clock,
7081
});
82+
return { device, counter };
83+
}
7184

85+
test('runtime wait stable settles at the second capture even when the sleep undershoots', async () => {
7286
// A 300ms quiet window equals the poll cadence, so the second capture is the
73-
// one that decides `settled`.
87+
// one that decides `settled` — and a sleep that lands 1ms short must not
88+
// change the verdict (#1306).
89+
const { device, counter } = createStableCaptureDevice(createUndershootingClock());
90+
7491
const result = await device.selectors.wait({
7592
session: 'default',
7693
target: { kind: 'stable', quietMs: 300, timeoutMs: 10_000 },
7794
});
7895

7996
assert.equal(result.kind, 'stable');
8097
if (result.kind === 'stable') assert.equal(result.captures, 2);
81-
// Strictly past the window, never onto it: sleeping exactly 300 would leave
82-
// the verdict to ~1ms of setTimeout/Date.now() skew (#1306).
83-
assert.equal(sleeps.length, 1);
84-
assert.ok(sleeps[0]! > 300, `settling capture must clear the window, slept ${sleeps[0]}ms`);
85-
assert.equal(captures, 2);
98+
assert.equal(counter.captures, 2);
99+
});
100+
101+
test('runtime wait stable still takes its settling capture when the budget barely clears the window', async () => {
102+
// One millisecond of budget past the quiet window is enough to settle, and
103+
// the deadline-aware sleep must not spend it on the epsilon: overshooting the
104+
// deadline would drop the settling capture the plain cadence would have taken.
105+
const { device, counter } = createStableCaptureDevice(createFakeClock());
106+
107+
const result = await device.selectors.wait({
108+
session: 'default',
109+
target: { kind: 'stable', quietMs: 300, timeoutMs: 301 },
110+
});
111+
112+
assert.equal(result.kind, 'stable');
113+
if (result.kind === 'stable') {
114+
assert.equal(result.captures, 2);
115+
assert.ok(
116+
result.waitedMs <= 301,
117+
`waitedMs must stay within the budget, got ${result.waitedMs}`,
118+
);
119+
}
120+
assert.equal(counter.captures, 2);
121+
});
122+
123+
test('runtime wait stable cannot settle when the budget only reaches the quiet deadline', async () => {
124+
// The other side of the boundary: the window has to ELAPSE inside the budget,
125+
// so a budget equal to it leaves no instant where two captures span it. The
126+
// deadline-aware sleep must not manufacture a settle here.
127+
const { device } = createStableCaptureDevice(createFakeClock());
128+
129+
await assert.rejects(
130+
() =>
131+
device.selectors.wait({
132+
session: 'default',
133+
target: { kind: 'stable', quietMs: 300, timeoutMs: 300 },
134+
}),
135+
(error: unknown) =>
136+
error instanceof Error &&
137+
(error as { details?: { reason?: string } }).details?.reason === 'wait_stable_timeout',
138+
);
86139
});
87140

88141
test('runtime wait stable hints when it settles on a nearly-empty tree', async () => {

src/commands/interaction/runtime/stable-capture.ts

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,10 @@ export async function runStableCaptureLoop(
9595
quietSinceMs = nowMs;
9696
lastDigest = digest;
9797
lastNodeCount = capture.snapshot.nodes.length;
98-
await sleep(runtime, stableCaptureDelayMs({ nowMs, quietSinceMs, quietMs, pollMs }));
98+
await sleep(
99+
runtime,
100+
stableCaptureDelayMs({ nowMs, quietSinceMs, quietMs, pollMs, deadlineMs }),
101+
);
99102
continue;
100103
}
101104
if (digest !== lastDigest) {
@@ -112,7 +115,10 @@ export async function runStableCaptureLoop(
112115
lastCapture,
113116
};
114117
}
115-
await sleep(runtime, stableCaptureDelayMs({ nowMs, quietSinceMs, quietMs, pollMs }));
118+
await sleep(
119+
runtime,
120+
stableCaptureDelayMs({ nowMs, quietSinceMs, quietMs, pollMs, deadlineMs }),
121+
);
116122
}
117123
return {
118124
settled: false,
@@ -134,16 +140,25 @@ function isPrivateAxRecovery(verdict: SnapshotQualityVerdict | undefined): boole
134140
* Once it is within reach, sleep to just past it instead — the capture that
135141
* decides `settled` then always spans the window, rather than landing on the
136142
* boundary where clock skew picks the answer (#1306).
143+
*
144+
* Bounded by the loop's own budget: it only runs again while `now < deadline`,
145+
* so a wake-up past the deadline spends the very capture the epsilon exists to
146+
* land. Where the budget is the tighter constraint, wake just inside it.
137147
*/
138148
function stableCaptureDelayMs(params: {
139149
nowMs: number;
140150
quietSinceMs: number;
141151
quietMs: number;
142152
pollMs: number;
153+
deadlineMs: number;
143154
}): number {
144155
const remainingQuietMs = params.quietSinceMs + params.quietMs - params.nowMs;
145-
if (remainingQuietMs > params.pollMs) return params.pollMs;
146-
return Math.max(STABLE_MIN_POLL_MS, remainingQuietMs + QUIET_DEADLINE_EPSILON_MS);
156+
const cadenceMs =
157+
remainingQuietMs > params.pollMs
158+
? params.pollMs
159+
: Math.max(STABLE_MIN_POLL_MS, remainingQuietMs + QUIET_DEADLINE_EPSILON_MS);
160+
const lastUsefulWakeMs = params.deadlineMs - params.nowMs - 1;
161+
return lastUsefulWakeMs > 0 ? Math.min(cadenceMs, lastUsefulWakeMs) : cadenceMs;
147162
}
148163

149164
// Intentionally does not update the session snapshot: the stable loop captures

0 commit comments

Comments
 (0)