Skip to content

Commit c352648

Browse files
thymikeeApex
andcommitted
refactor(android): encode the monotonic clock in the flush-window mark name
The coordinator's confirmation of cubic's P1 adds the shape the code should carry: the map is now testImeLastRestoreAtPerfMs, so 'AtMs' no longer misreads as a wall-clock epoch, and the comment states the two-clock mechanism (sleep is timed on libuv's monotonic loop clock; a wall-clock-derived remaining time mixes clocks by construction — the skew stable-capture.ts documents). One monotonic source for registration and expiry, no clamp hiding anything. P3 extension: the abort test now pins the NUMBER the design rests on — the mark is seeded 1000 ms old so the first wait must derive ~1500, and after the aborted kill the retry re-derives the same remainder from the surviving mark. A constant sleep, or an abort that consumed the evidence, fails on the number. Co-Authored-By: Apex <noreply@callstack.io>
1 parent 5753cfa commit c352648

3 files changed

Lines changed: 60 additions & 41 deletions

File tree

‎packages/platform-android/src/ime-restore.test.ts‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {
1616
import {
1717
resetAndroidTestImeActivationCacheForTests,
1818
setAndroidTestImeActiveForTests,
19-
testImeLastRestoreAtMs,
19+
testImeLastRestoreAtPerfMs,
2020
} from './ime-state.ts';
2121
import { fakeImeDeviceAdb, type FakeImeDeviceState } from './ime-device.fixtures.ts';
2222

@@ -190,7 +190,7 @@ describe('a close cancelled mid-settle', () => {
190190
// monotonic timestamp keeps the remaining wait derivable for the next kill-bound caller.
191191
expect(result).toMatchObject({ restored: true, reason: 'ok' });
192192
expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]);
193-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([DEVICE.id]);
193+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([DEVICE.id]);
194194
});
195195

196196
test('a second close waits out the remaining window before it may return to the kill', async () => {
@@ -234,7 +234,7 @@ describe('a close cancelled mid-settle', () => {
234234
expect(sleep).toHaveBeenCalledTimes(1);
235235
expect(sleep.mock.calls[0]?.[0]).toBeGreaterThan(0);
236236
expect(sleep.mock.calls[0]?.[1]).toBe(second.signal);
237-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([]);
237+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([]);
238238
expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]);
239239
});
240240

@@ -244,7 +244,7 @@ describe('a close cancelled mid-settle', () => {
244244
// A window from an earlier aborted close still has most of its budget left when this close's
245245
// own restore writes again. SettingsState rewrites the whole file, so one wait to the newer
246246
// write's window persists both writes — the timestamp takes the max, never a second entry.
247-
testImeLastRestoreAtMs.set(DEVICE.id, performance.now() - 100);
247+
testImeLastRestoreAtPerfMs.set(DEVICE.id, performance.now() - 100);
248248

249249
const result = await withAndroidAdbProvider(
250250
{ exec: fakeImeDeviceAdb(stuckDeviceState()) },
@@ -256,14 +256,14 @@ describe('a close cancelled mid-settle', () => {
256256
expect(result).toMatchObject({ restored: true, reason: 'ok' });
257257
expect(sleep).toHaveBeenCalledTimes(1);
258258
expect(sleep.mock.calls[0]?.[0]).toBeGreaterThan(2_000);
259-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([]);
259+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([]);
260260
});
261261

262262
test('a second close after the window already elapsed skips the wait', async () => {
263263
const host = bindAndroidAdbHostStub();
264264
await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id);
265265
// A restore whose whole flush window has already passed.
266-
testImeLastRestoreAtMs.set(DEVICE.id, performance.now() - 2_501);
266+
testImeLastRestoreAtPerfMs.set(DEVICE.id, performance.now() - 2_501);
267267

268268
const result = await withAndroidAdbProvider(
269269
{ exec: fakeImeDeviceAdb(stuckDeviceState()) },
@@ -274,7 +274,7 @@ describe('a close cancelled mid-settle', () => {
274274

275275
expect(result).toEqual({ restored: false, reason: 'not-activated-here' });
276276
expect(sleep).not.toHaveBeenCalled();
277-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([]);
277+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([]);
278278
// Load-bearing on this close: it inspected nothing (not-activated-here), and covering the
279279
// window a confirmed restore had opened is the only thing that earns clearing this marker.
280280
expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]);
@@ -283,7 +283,7 @@ describe('a close cancelled mid-settle', () => {
283283
test('an ordinary close never waits on a pending window it cannot race', async () => {
284284
const host = bindAndroidAdbHostStub();
285285
await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id);
286-
testImeLastRestoreAtMs.set(DEVICE.id, performance.now());
286+
testImeLastRestoreAtPerfMs.set(DEVICE.id, performance.now());
287287

288288
const result = await withAndroidAdbProvider(
289289
{ exec: fakeImeDeviceAdb(stuckDeviceState()) },
@@ -294,7 +294,7 @@ describe('a close cancelled mid-settle', () => {
294294
expect(result).toEqual({ restored: false, reason: 'not-activated-here' });
295295
expect(sleep).not.toHaveBeenCalled();
296296
// An ordinary close consumes nothing and inspects nothing: both stay for the next caller.
297-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([DEVICE.id]);
297+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([DEVICE.id]);
298298
expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]);
299299
});
300300
});
@@ -313,7 +313,7 @@ test('an ordinary close restores without any flush wait but registers the window
313313
expect(sleep).not.toHaveBeenCalled();
314314
// Registration is what lets a later kill-bound close — including another session's — see the
315315
// window this restore opened.
316-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([DEVICE.id]);
316+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([DEVICE.id]);
317317
});
318318

319319
test("a kill-bound close after another close's restore waits out the registered window", async () => {
@@ -335,7 +335,7 @@ test("a kill-bound close after another close's restore waits out the registered
335335
// 'ok' with no wait owed — is what this asserts; the clear-not-earned branches live in the
336336
// cancelled-mid-settle suite, where the first close leaves the marker in place.)
337337
expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]);
338-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([DEVICE.id]);
338+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([DEVICE.id]);
339339

340340
sleep.mockClear();
341341
const result = await withAndroidAdbProvider(
@@ -349,7 +349,7 @@ test("a kill-bound close after another close's restore waits out the registered
349349
expect(result).toEqual({ restored: false, reason: 'not-activated-here' });
350350
expect(sleep).toHaveBeenCalledTimes(1);
351351
expect(sleep.mock.calls[0]?.[0]).toBeGreaterThan(0);
352-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([]);
352+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([]);
353353
});
354354

355355
test('a shutdown of a physical device restores without any flush wait', async () => {
@@ -423,7 +423,7 @@ test.each([false, true])('startup recovers a stuck orphan with displaced=%s', as
423423
// The startup path writes `ime set` like any other restore, so it owes the same flush window:
424424
// a later kill-bound path (close --shutdown or the shutdown runtime) must not land inside it.
425425
// Registration is synchronous — daemon boot never pays a sleep for the window.
426-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([DEVICE.id]);
426+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([DEVICE.id]);
427427
expect(sleep).not.toHaveBeenCalled();
428428
expect(host.diagnostics).toContainEqual({
429429
phase: 'android_test_ime_orphan_restored',

‎packages/platform-android/src/ime-state.ts‎

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,26 +21,30 @@ export type AndroidTestImeOwnership = {
2121
// margin past the cap only has to absorb the AtomicFile rename.
2222
const SETTINGS_PROVIDER_FLUSH_SETTLE_MS = 2_500;
2323

24-
// Per-device monotonic last-restore timestamps, keyed by serial and read from the process
25-
// monotonic clock (performance.now), never the wall clock: the flush window is a property of the
26-
// device's SettingsProvider, not of any host state dir or of the close that performed the
27-
// restore, and a wall-clock step (VM resume, NTP correction) must never be able to shrink the
28-
// remaining wait. Every restore only moves the timestamp forward, so it can never consume
29-
// evidence a later kill needs — the wait is always derived from the newest write, which is also
30-
// why windows coalesce into one wait rather than stacking. Every path that may `adb emu kill`
31-
// calls awaitTestImeFlushWindow first; the kill-bound close finalization also consumes the
32-
// outcome so the pending marker clears only under a completed (or never-owed) wait.
24+
// Per-device last-restore marks, keyed by serial, timed entirely on the PROCESS MONOTONIC
25+
// CLOCK (performance.now) — never the wall clock, as the name suffix encodes. sleep() is
26+
// timed on libuv's monotonic loop clock, so a wall-clock-derived remaining time would mix two
27+
// clocks by construction (the skew stable-capture.ts documents in prose), and a forward step
28+
// from NTP or VM resume could make remainingMs negative and release the kill inside the
29+
// window. One source for both registration and expiry means neither can drift against the
30+
// other. The flush window is a property of the device's SettingsProvider, not of any host
31+
// state dir or of the close that performed the restore. Every restore only moves the mark
32+
// forward, so it can never consume evidence a later kill needs — the wait is always derived
33+
// from the newest write, which is also why windows coalesce into one wait rather than
34+
// stacking. Every path that may `adb emu kill` calls awaitTestImeFlushWindow first; the
35+
// kill-bound close finalization also consumes the outcome so the pending marker clears only
36+
// under a completed (or never-owed) wait.
3337
// @internal the map is exported for tests; production touches it only through the helpers here.
34-
export const testImeLastRestoreAtMs = new Map<string, number>();
38+
export const testImeLastRestoreAtPerfMs = new Map<string, number>();
3539

3640
export type TestImeFlushWait = 'idle' | 'covered' | 'aborted';
3741

3842
// The write that earned the window calls this, on every confirmed emulator restore — close-time,
3943
// cross-session, or startup-orphan recovery.
4044
export function registerTestImeRestore(serial: string): void {
41-
testImeLastRestoreAtMs.set(
45+
testImeLastRestoreAtPerfMs.set(
4246
serial,
43-
Math.max(testImeLastRestoreAtMs.get(serial) ?? 0, performance.now()),
47+
Math.max(testImeLastRestoreAtPerfMs.get(serial) ?? 0, performance.now()),
4448
);
4549
}
4650

@@ -58,20 +62,20 @@ export async function awaitTestImeFlushWindow(
5862
signal?: AbortSignal,
5963
): Promise<TestImeFlushWait> {
6064
if (signal?.aborted) return 'aborted';
61-
const restoredAtMs = testImeLastRestoreAtMs.get(serial);
62-
if (restoredAtMs === undefined) return 'idle';
63-
const remainingMs = restoredAtMs + SETTINGS_PROVIDER_FLUSH_SETTLE_MS - performance.now();
65+
const restoredAtPerfMs = testImeLastRestoreAtPerfMs.get(serial);
66+
if (restoredAtPerfMs === undefined) return 'idle';
67+
const remainingMs = restoredAtPerfMs + SETTINGS_PROVIDER_FLUSH_SETTLE_MS - performance.now();
6468
if (remainingMs <= 0) {
65-
testImeLastRestoreAtMs.delete(serial);
69+
testImeLastRestoreAtPerfMs.delete(serial);
6670
return 'covered';
6771
}
6872
await sleep(remainingMs, signal);
6973
if (signal?.aborted) return 'aborted';
70-
// Retire only the timestamp this wait actually covered: a restore that landed mid-wait (the
74+
// Retire only the mark this wait actually covered: a restore that landed mid-wait (the
7175
// unlocked shutdown-command path racing a close finalization) must keep its evidence for the
7276
// next kill-bound caller rather than have its younger window erased by this older wait.
73-
if (testImeLastRestoreAtMs.get(serial) === restoredAtMs) {
74-
testImeLastRestoreAtMs.delete(serial);
77+
if (testImeLastRestoreAtPerfMs.get(serial) === restoredAtPerfMs) {
78+
testImeLastRestoreAtPerfMs.delete(serial);
7579
}
7680
return 'covered';
7781
}
@@ -105,7 +109,7 @@ export function withAndroidTestImeRecoveryLock<T>(
105109
*/
106110
export function resetAndroidTestImeActivationCacheForTests(): void {
107111
activeTestImeDevices.clear();
108-
testImeLastRestoreAtMs.clear();
112+
testImeLastRestoreAtPerfMs.clear();
109113
}
110114

111115
/**

‎packages/platform-android/src/shutdown/runtime.test.ts‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ vi.mock('@agent-device/host-kit/retry', async (importOriginal) => ({
99
sleep,
1010
}));
1111

12-
import { testImeLastRestoreAtMs } from '../ime-state.ts';
12+
import { testImeLastRestoreAtPerfMs } from '../ime-state.ts';
1313

1414
const run = vi.fn(async () => ({ stdout: '', stderr: '', exitCode: 0 }));
1515
const commands: DeviceShutdownRuntimeDependencies['commands'] = {
@@ -21,7 +21,7 @@ beforeEach(() => {
2121
run.mockReset();
2222
run.mockResolvedValue({ stdout: '', stderr: '', exitCode: 0 });
2323
sleep.mockClear();
24-
testImeLastRestoreAtMs.clear();
24+
testImeLastRestoreAtPerfMs.clear();
2525
});
2626

2727
test('an already-stopped emulator succeeds without adb', async () => {
@@ -60,7 +60,7 @@ test('only Android emulators are available', () => {
6060
// caller) inherits it without knowing about the test IME.
6161
test('a kill waits out a registered test-IME flush window before running adb emu kill', async () => {
6262
const device = androidDevice();
63-
testImeLastRestoreAtMs.set(device.id, performance.now());
63+
testImeLastRestoreAtPerfMs.set(device.id, performance.now());
6464

6565
await expect(
6666
createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal()),
@@ -72,14 +72,14 @@ test('a kill waits out a registered test-IME flush window before running adb emu
7272
// gap. A regression sleeping a fixed unrelated amount would fail here.
7373
expect(sleep.mock.calls[0]?.[0]).toBeGreaterThanOrEqual(2_000);
7474
expect(run).toHaveBeenCalledTimes(1);
75-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([]);
75+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([]);
7676
});
7777

7878
test('a forward host wall-clock jump cannot shorten a registered flush window', async () => {
7979
// The window is timed on the process monotonic clock, so an NTP/VM-resume step of the wall
8080
// clock must leave the kill-side remaining wait at the full budget.
8181
const device = androidDevice();
82-
testImeLastRestoreAtMs.set(device.id, performance.now());
82+
testImeLastRestoreAtPerfMs.set(device.id, performance.now());
8383
const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(Number.MAX_SAFE_INTEGER);
8484

8585
try {
@@ -107,7 +107,9 @@ test('a kill skips the wait entirely when no flush window is open', async () =>
107107

108108
test('a kill cancelled inside the flush window never reaches adb emu kill', async () => {
109109
const device = androidDevice();
110-
testImeLastRestoreAtMs.set(device.id, performance.now());
110+
// Known elapsed: the mark is 1000 ms old, so the derived remainder must be ~1500 — not the
111+
// full budget, not a fixed constant. That makes both waits provably a function of the mark.
112+
testImeLastRestoreAtPerfMs.set(device.id, performance.now() - 1_000);
111113
const controller = new AbortController();
112114
sleep.mockImplementationOnce((_ms, waitSignal) => {
113115
controller.abort();
@@ -119,8 +121,21 @@ test('a kill cancelled inside the flush window never reaches adb emu kill', asyn
119121
).rejects.toBeDefined();
120122

121123
expect(run).not.toHaveBeenCalled();
122-
// The window stays registered so a retry still waits out the remainder.
123-
expect([...testImeLastRestoreAtMs.keys()]).toEqual([device.id]);
124+
const firstRemainingMs = sleep.mock.calls[0]?.[0] as number;
125+
expect(firstRemainingMs).toBeGreaterThan(1_400);
126+
expect(firstRemainingMs).toBeLessThanOrEqual(1_500);
127+
// The window stays registered so a retry still waits out the remainder — and the retry
128+
// re-derives the SAME remainder from the surviving mark: the monotonic-mark property the
129+
// abort design rests on, pinned by number, not by shape.
130+
expect([...testImeLastRestoreAtPerfMs.keys()]).toEqual([device.id]);
131+
132+
const retry = await createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal());
133+
134+
expect(retry).toEqual(success());
135+
expect(sleep).toHaveBeenCalledTimes(2);
136+
const secondRemainingMs = sleep.mock.calls[1]?.[0] as number;
137+
expect(secondRemainingMs).toBeGreaterThan(1_400);
138+
expect(secondRemainingMs).toBeLessThanOrEqual(firstRemainingMs);
124139
});
125140

126141
function signal(): AbortSignal {

0 commit comments

Comments
 (0)