Skip to content

Commit fcf968b

Browse files
committed
fix(android): register the flush window on every confirmed restore
The kill that must miss the flush window can arrive from another session: an ordinary close confirmed the restore and dropped the owned flag, and a later close --shutdown on the same device then inspected nothing and killed at once. Registration now belongs to the confirmed restore itself, on every close; only kill-bound closes wait it out. ime-restore also stops re-deriving the kill-bound predicate from device kind, which lifecycle.ts already owns; it gates registration to emulators because only emulators can be killed by the runtime.
1 parent 3bbda23 commit fcf968b

2 files changed

Lines changed: 63 additions & 28 deletions

File tree

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

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ describe('a close cancelled mid-settle', () => {
296296
});
297297
});
298298

299-
test('an ordinary close restores without any flush wait', async () => {
299+
test('an ordinary close restores without any flush wait but registers the window', async () => {
300300
bindAndroidAdbHostStub();
301301
setAndroidTestImeActiveForTests(DEVICE, true);
302302

@@ -308,6 +308,37 @@ test('an ordinary close restores without any flush wait', async () => {
308308

309309
expect(result).toMatchObject({ restored: true, reason: 'ok' });
310310
expect(sleep).not.toHaveBeenCalled();
311+
// Registration is what lets a later kill-bound close — including another session's — see the
312+
// window this restore opened.
313+
expect([...pendingTestImeFlushSettles.keys()]).toEqual([`${STATE_DIR}:${DEVICE.id}`]);
314+
});
315+
316+
test("a kill-bound close after another close's restore waits out the registered window", async () => {
317+
// The reviewer's cross-session hole: session A's ordinary close confirmed the restore and
318+
// dropped ownership; session B's close --shutdown must not kill inside A's flush window even
319+
// though its own call inspects nothing.
320+
const host = bindAndroidAdbHostStub();
321+
await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id);
322+
setAndroidTestImeActiveForTests(DEVICE, true);
323+
const deviceAdb = fakeImeDeviceAdb(stuckDeviceState());
324+
await withAndroidAdbProvider(
325+
{ exec: deviceAdb },
326+
{ serial: DEVICE.id },
327+
async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: false }),
328+
);
329+
330+
sleep.mockClear();
331+
const result = await withAndroidAdbProvider(
332+
{ exec: deviceAdb },
333+
{ serial: DEVICE.id },
334+
async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: true }),
335+
);
336+
337+
expect(result).toEqual({ restored: false, reason: 'not-activated-here' });
338+
expect(sleep).toHaveBeenCalledTimes(1);
339+
expect(sleep.mock.calls[0]?.[0]).toBeGreaterThan(0);
340+
expect([...pendingTestImeFlushSettles.keys()]).toEqual([]);
341+
expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]);
311342
});
312343

313344
test('a shutdown of a physical device restores without any flush wait', async () => {

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

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -53,22 +53,24 @@ function flushSettleKey(stateDir: string, serial: string): string {
5353
return `${stateDir}:${serial}`;
5454
}
5555

56-
// `idle`: nothing was open and this call opened none. `retired`: a deadline — this call's own
57-
// restore write or one an earlier aborted close left behind — was waited out and cleared.
58-
// `aborted`: the wait was cancelled, so the deadline and the pending marker both stay in place.
59-
// An aborted settle is never a completed settle.
56+
// `idle`: no deadline was open, or this call is not kill-bound and leaves it in place.
57+
// `retired`: a deadline — this process's own restore write or one another close left behind —
58+
// was waited out and cleared. `aborted`: the wait was cancelled, so the deadline and the pending
59+
// marker both stay in place. An aborted settle is never a completed settle.
6060
type FlushSettleOutcome = 'idle' | 'retired' | 'aborted';
6161

62-
async function settleFlushWindow(
63-
key: string,
64-
registeredAtMs: number | undefined,
65-
signal?: AbortSignal,
66-
): Promise<FlushSettleOutcome> {
67-
let deadlineAtMs = pendingTestImeFlushSettles.get(key);
68-
if (registeredAtMs !== undefined) {
69-
deadlineAtMs = Math.max(deadlineAtMs ?? 0, registeredAtMs + SETTINGS_PROVIDER_FLUSH_SETTLE_MS);
70-
pendingTestImeFlushSettles.set(key, deadlineAtMs);
71-
}
62+
// A confirmed restore registers its flush window here, not on the close that performed it: the
63+
// kill that must miss the window can arrive from another session's `close --shutdown`, after this
64+
// process has dropped device ownership and taken a no-op `not-activated-here` close. Windows
65+
// coalesce because doWriteState() rewrites the whole file — one later deadline covers every
66+
// pending write, including an older aborted settle's.
67+
function registerFlushDeadline(key: string): void {
68+
const dueAtMs = Date.now() + SETTINGS_PROVIDER_FLUSH_SETTLE_MS;
69+
pendingTestImeFlushSettles.set(key, Math.max(pendingTestImeFlushSettles.get(key) ?? 0, dueAtMs));
70+
}
71+
72+
async function awaitFlushDeadline(key: string, signal?: AbortSignal): Promise<FlushSettleOutcome> {
73+
const deadlineAtMs = pendingTestImeFlushSettles.get(key);
7274
if (deadlineAtMs === undefined) return 'idle';
7375
const remainingMs = deadlineAtMs - Date.now();
7476
if (remainingMs > 0) {
@@ -96,20 +98,22 @@ export async function restoreAndroidTestIme(
9698
options: { stateDir: string; shutdownTarget?: boolean; signal?: AbortSignal },
9799
): Promise<AndroidTestImeRestoreResult> {
98100
return await withAndroidTestImeRecoveryLock(options.stateDir, device.id, async () => {
101+
const settleKey = flushSettleKey(options.stateDir, device.id);
99102
const result = await restoreOwnedAndroidTestIme(device);
100-
// A kill-bound close returns only after every open window is covered — including one an
101-
// earlier aborted close left behind — so the finalizer's kill lands after the flush. The
102-
// restore's own write registers its window here, after the write, coalescing with the old
103-
// deadline instead of stacking behind it: one sleep to max(old, write + cap). An ordinary
104-
// close starts no kill, so it opens nothing and leaves any leftover deadline in place.
105-
const settling = options.shutdownTarget === true && device.kind === 'emulator';
106-
const outcome = settling
107-
? await settleFlushWindow(
108-
flushSettleKey(options.stateDir, device.id),
109-
result.restored ? Date.now() : undefined,
110-
options.signal,
111-
)
112-
: 'idle';
103+
// Every confirmed restore registers its flush window, even on an ordinary close: the kill
104+
// that must miss the window can arrive later from another session's `close --shutdown`, on a
105+
// path where this process has already dropped ownership. Only emulators can be killed by the
106+
// runtime, so only they carry windows; the caller owns the kill-bound predicate and this
107+
// layer does not re-derive it.
108+
if (result.restored && device.kind === 'emulator') {
109+
registerFlushDeadline(settleKey);
110+
}
111+
// A kill-bound close returns only after every open window is retired, so the finalizer's
112+
// kill lands after the flush. An ordinary close never waits and leaves deadlines in place.
113+
const outcome =
114+
options.shutdownTarget === true
115+
? await awaitFlushDeadline(settleKey, options.signal)
116+
: 'idle';
113117
if (isRecoveryMarkerClearEarned(result.reason, outcome)) {
114118
await requireAndroidAdbHost().imeRecoveryMarkers.clear(options.stateDir, device.id);
115119
}

0 commit comments

Comments
 (0)