diff --git a/packages/contracts/src/application-lifecycle-runtime.ts b/packages/contracts/src/application-lifecycle-runtime.ts index 2fd7144220..b7ed746547 100644 --- a/packages/contracts/src/application-lifecycle-runtime.ts +++ b/packages/contracts/src/application-lifecycle-runtime.ts @@ -396,7 +396,10 @@ export type AndroidApplicationTools = Readonly<{ applyRuntimeHints(device: DeviceInfo, input: RuntimeHintsApplicationInput): Promise; clearRuntimeHints(device: DeviceInfo, input: RuntimeHintsApplicationInput): Promise; activateTestIme(device: DeviceInfo, input: Readonly<{ stateDir: string }>): Promise; - restoreTestIme(device: DeviceInfo, input: Readonly<{ stateDir: string }>): Promise; + restoreTestIme( + device: DeviceInfo, + input: Readonly<{ stateDir: string; shutdownTarget: boolean; signal: AbortSignal }>, + ): Promise; recoverTestImeStartup(input: Readonly<{ stateDir: string }>): Promise; hasTestImeRecoveryEvidence(stateDir: string): Promise; }>; diff --git a/packages/platform-android/src/ime-device.fixtures.ts b/packages/platform-android/src/ime-device.fixtures.ts index 1426061af6..2753f76f05 100644 --- a/packages/platform-android/src/ime-device.fixtures.ts +++ b/packages/platform-android/src/ime-device.fixtures.ts @@ -1,8 +1,17 @@ import type { AndroidAdbExecutor, AndroidAdbExecutorResult } from './adb-transport.ts'; +import { testImeRestoreMarks } from './ime-state.ts'; // An in-memory device speaking the exact shell surfaces the IME lifecycle touches: the // `settings secure` namespace and `ime enable/disable/set`. Shared by the colocated IME module tests. +// Places a restore mark at an exact age on the register of flush windows. Default provenance +// is confirmed: a registered window normally comes from a confirmed restore, and wait- +// derivation tests read only atPerfMs; tests pinning marker-clear eligibility pass provenance +// explicitly. Lives here so a mark-SHAPE change lands in one edit. +export function seedRestoreMark(serial: string, ageMs: number, confirmed = true): void { + testImeRestoreMarks.set(serial, { atPerfMs: performance.now() - ageMs, confirmed }); +} + export type FakeImeDeviceState = { settings: Map; imeSetFails?: boolean; diff --git a/packages/platform-android/src/ime-restore.test.ts b/packages/platform-android/src/ime-restore.test.ts index 7107ed7916..7de5354028 100644 --- a/packages/platform-android/src/ime-restore.test.ts +++ b/packages/platform-android/src/ime-restore.test.ts @@ -1,5 +1,12 @@ -import { beforeEach, expect, test } from 'vitest'; +import { beforeEach, describe, expect, test, vi } from 'vitest'; import type { DeviceInfo } from '@agent-device/kernel/device'; + +const sleep = vi.hoisted(() => vi.fn(async (_ms: number, _signal?: AbortSignal) => {})); +vi.mock('@agent-device/host-kit/retry', async (importOriginal) => ({ + ...(await importOriginal()), + sleep, +})); + import { bindAndroidAdbHostStub } from './adb-host.fixtures.ts'; import { withAndroidAdbProvider } from './adb-provider-scope.ts'; import { @@ -9,8 +16,14 @@ import { import { resetAndroidTestImeActivationCacheForTests, setAndroidTestImeActiveForTests, + SETTINGS_PROVIDER_FLUSH_SETTLE_MS, + testImeRestoreMarks, } from './ime-state.ts'; -import { fakeImeDeviceAdb, type FakeImeDeviceState } from './ime-device.fixtures.ts'; +import { + fakeImeDeviceAdb, + seedRestoreMark, + type FakeImeDeviceState, +} from './ime-device.fixtures.ts'; const DEVICE: DeviceInfo = { platform: 'android', @@ -31,16 +44,21 @@ function stuckDeviceState(): FakeImeDeviceState { }; } -async function restoreWith(state: FakeImeDeviceState) { +async function restoreWith(state: FakeImeDeviceState, options: { shutdownTarget?: boolean } = {}) { return await withAndroidAdbProvider( { exec: fakeImeDeviceAdb(state) }, { serial: DEVICE.id }, - async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR }), + async () => + await restoreAndroidTestIme(DEVICE, { + stateDir: STATE_DIR, + shutdownTarget: options.shutdownTarget, + }), ); } beforeEach(() => { resetAndroidTestImeActivationCacheForTests(); + sleep.mockClear(); }); test.each([false, true])('close restores the previous IME with displaced=%s', async (displaced) => { @@ -102,12 +120,324 @@ test('a failed restore keeps the record and the marker for a later retry', async expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]); }); -test('devices this process never activated are left alone', async () => { - bindAndroidAdbHostStub(); +test('devices this process never activated are left alone, marker and all', async () => { + // The retained marker is the only record an orphaned helper has after a daemon restart; a + // close that inspected nothing must not consume it. + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); const state = stuckDeviceState(); const result = await restoreWith(state); - expect(result).toEqual({ restored: false, reason: 'no-record' }); + expect(result).toEqual({ restored: false, reason: 'not-activated-here' }); expect(state.settings.get('default_input_method')).toBe(HELPER_SERVICE); + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]); +}); + +// #3318: `ime set` is durable only after SettingsProvider's delayed XML flush (AOSP caps it at +// 2 s), so a kill started while that window is open reboots the emulator onto the helper IME. +test('a restore before an emulator shutdown waits out the settings-provider flush window', async () => { + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); + setAndroidTestImeActiveForTests(DEVICE, true); + const markerDuringSettle: string[][] = []; + sleep.mockImplementationOnce(async () => { + // The marker must survive the settle so a daemon crash mid-window still recovers. + markerDuringSettle.push([...(host.markerStore.get(STATE_DIR) ?? [])]); + }); + const signal = new AbortController().signal; + + const result = await withAndroidAdbProvider( + { exec: fakeImeDeviceAdb(stuckDeviceState()) }, + { serial: DEVICE.id }, + async () => + await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: true, signal }), + ); + + expect(result).toMatchObject({ restored: true, reason: 'ok' }); + expect(sleep).toHaveBeenCalledTimes(1); + // The window had barely opened at the wait, so the derived remainder is nearly the whole + // budget; derive both bounds from the constant so a changed window shifts the bound too. + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThanOrEqual(SETTINGS_PROVIDER_FLUSH_SETTLE_MS * 0.8); + // A cancelled close never reaches the kill, so the settle stops early with it (see the + // abort-mid-settle tests below for what an early stop must preserve). + expect(sleep.mock.calls[0]?.[1]).toBe(signal); + expect(markerDuringSettle).toEqual([[DEVICE.id]]); + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]); +}); + +describe('a close cancelled mid-settle', () => { + // The real sleep resolves early (never rejects) when its signal aborts; the mock has to behave + // the same or the abort branch is never exercised. + function abortAwareSleep(signal?: AbortSignal): Promise { + return signal?.aborted ? Promise.resolve() : new Promise(() => {}); + } + + test('keeps the flush-settle deadline and the pending marker for the next close', async () => { + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); + setAndroidTestImeActiveForTests(DEVICE, true); + const controller = new AbortController(); + sleep.mockImplementationOnce((_ms, signal) => { + controller.abort(); + return abortAwareSleep(signal); + }); + + const result = await withAndroidAdbProvider( + { exec: fakeImeDeviceAdb(stuckDeviceState()) }, + { serial: DEVICE.id }, + async () => + await restoreAndroidTestIme(DEVICE, { + stateDir: STATE_DIR, + shutdownTarget: true, + signal: controller.signal, + }), + ); + + // The keyboard really was restored, but an aborted settle is not a completed settle: the + // marker survives so a crash-recovery path still covers the unfinished flush window, and the + // monotonic timestamp keeps the remaining wait derivable for the next kill-bound caller. + expect(result).toMatchObject({ restored: true, reason: 'ok' }); + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]); + expect([...testImeRestoreMarks.keys()]).toEqual([DEVICE.id]); + }); + + test('a second close waits out the remaining window before it may return to the kill', async () => { + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); + setAndroidTestImeActiveForTests(DEVICE, true); + const first = new AbortController(); + sleep.mockImplementationOnce((_ms, signal) => { + first.abort(); + return abortAwareSleep(signal); + }); + const deviceAdb = fakeImeDeviceAdb(stuckDeviceState()); + await withAndroidAdbProvider( + { exec: deviceAdb }, + { serial: DEVICE.id }, + async () => + await restoreAndroidTestIme(DEVICE, { + stateDir: STATE_DIR, + shutdownTarget: true, + signal: first.signal, + }), + ); + + // The owned flag is gone, so this close inspects nothing — yet the kill it precedes must + // still wait for the window the aborted close left open, and consuming that deadline is what + // earns the marker clear (a nothing-inspected close could never clear it on its own). + sleep.mockClear(); + const second = new AbortController(); + const result = await withAndroidAdbProvider( + { exec: deviceAdb }, + { serial: DEVICE.id }, + async () => + await restoreAndroidTestIme(DEVICE, { + stateDir: STATE_DIR, + shutdownTarget: true, + signal: second.signal, + }), + ); + + expect(result).toEqual({ restored: false, reason: 'not-activated-here' }); + expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThan(0); + expect(sleep.mock.calls[0]?.[1]).toBe(second.signal); + expect([...testImeRestoreMarks.keys()]).toEqual([]); + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]); + }); + + test('a restore while a window is open coalesces into one deadline, not two waits', async () => { + bindAndroidAdbHostStub(); + setAndroidTestImeActiveForTests(DEVICE, true); + // A window from an earlier aborted close still has most of its budget left when this close's + // own restore writes again. SettingsState rewrites the whole file, so one wait to the newer + // write's window persists both writes — the timestamp takes the max, never a second entry. + // The seeded elapsed (one twentieth of the window) and the expected sleep band are both + // derived from the window constant, so a changed window keeps this test's meaning. + const elapsedBeforeCloseMs = Math.round(SETTINGS_PROVIDER_FLUSH_SETTLE_MS / 20); + seedRestoreMark(DEVICE.id, elapsedBeforeCloseMs); + + const result = await withAndroidAdbProvider( + { exec: fakeImeDeviceAdb(stuckDeviceState()) }, + { serial: DEVICE.id }, + async () => + await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: true }), + ); + + expect(result).toMatchObject({ restored: true, reason: 'ok' }); + expect(sleep).toHaveBeenCalledTimes(1); + // This close's OWN restore re-registers the mark at nearly the full window (the earlier + // mark was older), so the sleep is derived from the fresh write, not from the seeded one. + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThan(SETTINGS_PROVIDER_FLUSH_SETTLE_MS * 0.8); + expect([...testImeRestoreMarks.keys()]).toEqual([]); + }); + + test('a second close after the window already elapsed skips the wait', async () => { + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); + // A restore whose whole flush window has already passed (the +1 keeps it elapsed for any + // constant, instead of the number that happened to be window+1 at writing time). + seedRestoreMark(DEVICE.id, SETTINGS_PROVIDER_FLUSH_SETTLE_MS + 1, true); + + const result = await withAndroidAdbProvider( + { exec: fakeImeDeviceAdb(stuckDeviceState()) }, + { serial: DEVICE.id }, + async () => + await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: true }), + ); + + expect(result).toEqual({ restored: false, reason: 'not-activated-here' }); + expect(sleep).not.toHaveBeenCalled(); + expect([...testImeRestoreMarks.keys()]).toEqual([]); + // Load-bearing on this close: it inspected nothing (not-activated-here), and covering the + // window a confirmed restore had opened is the only thing that earns clearing this marker. + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]); + }); + + test('an ordinary close never waits on a pending window it cannot race', async () => { + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); + seedRestoreMark(DEVICE.id, 0); + + const result = await withAndroidAdbProvider( + { exec: fakeImeDeviceAdb(stuckDeviceState()) }, + { serial: DEVICE.id }, + async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR }), + ); + + expect(result).toEqual({ restored: false, reason: 'not-activated-here' }); + expect(sleep).not.toHaveBeenCalled(); + // An ordinary close consumes nothing and inspects nothing: both stay for the next caller. + expect([...testImeRestoreMarks.keys()]).toEqual([DEVICE.id]); + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]); + }); +}); + +test('an ordinary close restores without any flush wait but registers the window', async () => { + bindAndroidAdbHostStub(); + setAndroidTestImeActiveForTests(DEVICE, true); + + const result = await withAndroidAdbProvider( + { exec: fakeImeDeviceAdb(stuckDeviceState()) }, + { serial: DEVICE.id }, + async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: false }), + ); + + expect(result).toMatchObject({ restored: true, reason: 'ok' }); + expect(sleep).not.toHaveBeenCalled(); + // Registration is what lets a later kill-bound close — including another session's — see the + // window this restore opened. + expect([...testImeRestoreMarks.keys()]).toEqual([DEVICE.id]); +}); + +test("a kill-bound close after another close's restore waits out the registered window", async () => { + // The cross-session hole: session A's ordinary close confirmed the restore and + // dropped ownership; session B's close --shutdown must not kill inside A's flush window even + // though its own call inspects nothing. + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); + setAndroidTestImeActiveForTests(DEVICE, true); + const deviceAdb = fakeImeDeviceAdb(stuckDeviceState()); + await withAndroidAdbProvider( + { exec: deviceAdb }, + { serial: DEVICE.id }, + async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: false }), + ); + // Call one's own contribution, asserted before call two so the two calls' effects stay + // separable: a confirmed ordinary restore earns the marker clear on the spot, and its only + // flush-window residue is the registered timestamp. (That the clear is EARNED — an inspected + // 'ok' with no wait owed — is what this asserts; the clear-not-earned branches live in the + // cancelled-mid-settle suite, where the first close leaves the marker in place.) + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]); + expect([...testImeRestoreMarks.keys()]).toEqual([DEVICE.id]); + + sleep.mockClear(); + const result = await withAndroidAdbProvider( + { exec: deviceAdb }, + { serial: DEVICE.id }, + async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: true }), + ); + + // Call two's load-bearing contribution: it owns nothing and inspected nothing, yet the kill + // it precedes waits for the window call one registered, and covering it retires the entry. + expect(result).toEqual({ restored: false, reason: 'not-activated-here' }); + expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThan(0); + expect([...testImeRestoreMarks.keys()]).toEqual([]); +}); + +test('a not-activated shutdown covering an unconfirmed window kills but keeps the marker', async () => { + // A failed restore still owes the kill-bound hold — the provider may have accepted the + // write — but its mark proves only issuance, not recovery. An uninspected kill-bound close + // must therefore wait the window out AND leave the durable marker for the startup scan to + // retry: the helper may still be active and the persisted target intact. + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); + setAndroidTestImeActiveForTests(DEVICE, true); + const failingState = stuckDeviceState(); + failingState.imeSetFails = true; + const deviceAdb = fakeImeDeviceAdb(failingState); + + await withAndroidAdbProvider( + { exec: deviceAdb }, + { serial: DEVICE.id }, + async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: false }), + ); + // Call one: set-failed retained the marker and registered an unconfirmed window. + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]); + expect(testImeRestoreMarks.get(DEVICE.id)?.confirmed).toBe(false); + + sleep.mockClear(); + const result = await withAndroidAdbProvider( + { exec: deviceAdb }, + { serial: DEVICE.id }, + async () => await restoreAndroidTestIme(DEVICE, { stateDir: STATE_DIR, shutdownTarget: true }), + ); + + // Call two: it waited (the hold is owed regardless of provenance) and retired the mark, but + // covering an ISSUED-only window does not earn the clear — the marker survives so startup + // recovery retries the restore a confirmed close would have completed. Call two starts + // sub-ms after call one registered, so the derived remainder is nearly the whole window. + expect(result).toEqual({ restored: false, reason: 'not-activated-here' }); + expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThanOrEqual(SETTINGS_PROVIDER_FLUSH_SETTLE_MS * 0.8); + expect([...testImeRestoreMarks.keys()]).toEqual([]); + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]); +}); + +test('a shutdown of a physical device restores without any flush wait', async () => { + bindAndroidAdbHostStub(); + const device: DeviceInfo = { ...DEVICE, kind: 'device', id: 'R5800ABC1' }; + setAndroidTestImeActiveForTests(device, true); + + const result = await withAndroidAdbProvider( + { exec: fakeImeDeviceAdb(stuckDeviceState()) }, + { serial: device.id }, + async () => await restoreAndroidTestIme(device, { stateDir: STATE_DIR, shutdownTarget: true }), + ); + + expect(result).toMatchObject({ restored: true, reason: 'ok' }); + expect(sleep).not.toHaveBeenCalled(); +}); + +test('a restore that did not switch the IME back still owes the flush wait it may have written', async () => { + // A readback mismatch cannot prove the provider never accepted the write, so the issued + // write owes the kill-bound hold like any other — while the retained record and marker + // still cover the retry. The hold and the retained evidence are pinned together because + // this path is exactly where both mechanisms must agree. + const host = bindAndroidAdbHostStub(); + await host.imeRecoveryMarkers.write(STATE_DIR, DEVICE.id); + setAndroidTestImeActiveForTests(DEVICE, true); + const state = stuckDeviceState(); + state.imeSetFails = true; + + const result = await restoreWith(state, { shutdownTarget: true }); + + expect(result).toMatchObject({ restored: false, reason: 'set-failed' }); + expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThanOrEqual(SETTINGS_PROVIDER_FLUSH_SETTLE_MS * 0.8); + expect([...testImeRestoreMarks.keys()]).toEqual([]); + expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([DEVICE.id]); + expect(state.settings.has('agent_device_ime_helper_previous_ime')).toBe(true); }); test('startup recovery never scans devices without a pending marker, and retains offline markers', async () => { @@ -151,6 +481,11 @@ test.each([false, true])('startup recovers a stuck orphan with displaced=%s', as expect(state.settings.has('agent_device_ime_helper_rebind_displaced')).toBe(false); expect(state.settings.has('agent_device_ime_helper_previous_ime')).toBe(false); expect([...(host.markerStore.get(STATE_DIR) ?? [])]).toEqual([]); + // The startup path writes `ime set` like any other restore, so it owes the same flush window: + // a later kill-bound path (close --shutdown or the shutdown runtime) must not land inside it. + // Registration is synchronous — daemon boot never pays a sleep for the window. + expect([...testImeRestoreMarks.keys()]).toEqual([DEVICE.id]); + expect(sleep).not.toHaveBeenCalled(); expect(host.diagnostics).toContainEqual({ phase: 'android_test_ime_orphan_restored', level: 'warn', diff --git a/packages/platform-android/src/ime-restore.ts b/packages/platform-android/src/ime-restore.ts index f1e2b22951..c077ad8d7a 100644 --- a/packages/platform-android/src/ime-restore.ts +++ b/packages/platform-android/src/ime-restore.ts @@ -14,12 +14,20 @@ import { readAndroidDefaultInputMethod, readAndroidTestImeDeviceRecord, } from './ime-settings-record.ts'; -import { activeTestImeDevices, withAndroidTestImeRecoveryLock } from './ime-state.ts'; +import { + activeTestImeDevices, + awaitTestImeFlushWindow, + beginTestImeRestoreWrite, + registerTestImeRestore, + withAndroidTestImeRecoveryLock, + type TestImeFlushWait, +} from './ime-state.ts'; // Restore and startup orphan recovery: undo the helper switch exactly when it is safe, keep // durable evidence until the device is observed clean. export type AndroidTestImeRestoreReason = + | 'not-activated-here' | 'no-record' | 'record-unreadable' | 'helper-not-active' @@ -33,43 +41,77 @@ export type AndroidTestImeRestoreResult = { reason: AndroidTestImeRestoreReason; }; +// One marker-clear rule, owned here and used by both callers of the inner restore. A reason +// from an inspected device proves recovery complete once this call's flush wait is not +// abandoned: a kill-bound close must reach a covered outcome (an aborted wait is never a +// completed settle), while an ordinary close or the startup scan owe no wait at all — neither +// kills, and the device's window stays registered for the next kill-bound path to wait out. A +// nothing-inspected `not-activated-here` close earns the clear only on 'covered-confirmed': +// it saw no device, so the sole evidence that retrying is safe is that the covered window +// belonged to a write that confirmed. An issued-but-unconfirmed write leaves the helper +// possibly displaced and its persisted target intact, and the marker must survive for the +// startup scan to retry. +function isRecoveryMarkerClearEarned( + reason: AndroidTestImeRestoreReason, + settleOutcome: TestImeFlushWait, +): boolean { + if (isDeviceRecoveryComplete(reason)) return settleOutcome !== 'aborted'; + return reason === 'not-activated-here' && settleOutcome === 'covered-confirmed'; +} + export async function restoreAndroidTestIme( device: DeviceInfo, - options: { stateDir: string }, + options: { stateDir: string; shutdownTarget?: boolean; signal?: AbortSignal }, ): Promise { return await withAndroidTestImeRecoveryLock(options.stateDir, device.id, async () => { - const deviceKey = getAndroidImeHelperDeviceKey(device); - // Skip devices this process never activated (orphans from another process are handled by - // restoreOrphanedAndroidTestImeOnDaemonStartup and the doctor check). - if (!activeTestImeDevices.has(deviceKey)) { - return { restored: false, reason: 'no-record' }; - } - // Drop the owned-flag first so restoreAndroidTestImeFor's "owned by a live session" guard does - // not skip this intentional close-time restore. - activeTestImeDevices.delete(deviceKey); - const adb = resolveAndroidAdbExecutor(device); - const result = await restoreAndroidTestImeFor(adb, device); - if (isDeviceRecoveryComplete(result.reason)) { + const result = await restoreOwnedAndroidTestIme(device); + // Only a kill-bound close waits: it returns after the device's flush window is covered, so + // the finalizer's kill lands after the provider's write. Ordinary closes never sleep. + const settleOutcome: TestImeFlushWait = + options.shutdownTarget === true + ? await awaitTestImeFlushWindow(device.id, options.signal) + : 'idle'; + if (isRecoveryMarkerClearEarned(result.reason, settleOutcome)) { await requireAndroidAdbHost().imeRecoveryMarkers.clear(options.stateDir, device.id); } return result; }); } -// A device no longer needs recovery once the helper is confirmed off it (restored, or already not -// the active IME, or no record). A `set-failed` (still stuck), `record-unreadable` (the device may be -// on Android's fallback IME) or `owned-by-live-session` (a live session will restore it on close) -// keeps its pending marker for a later retry. +// Restore this process's own device, or report that it never activated one. For an unactivated +// device nothing is inspected (orphans from another process are handled by +// restoreOrphanedAndroidTestImeOnDaemonStartup and the doctor check), so recovery stays unknown. +async function restoreOwnedAndroidTestIme( + device: DeviceInfo, +): Promise { + const deviceKey = getAndroidImeHelperDeviceKey(device); + if (!activeTestImeDevices.has(deviceKey)) { + return { restored: false, reason: 'not-activated-here' }; + } + // Drop the owned-flag first so restoreAndroidTestImeFor's "owned by a live session" guard does + // not skip this intentional close-time restore. The IME really is back on the previous keyboard, + // so text routing must stop preferring the helper channel even on an abort. + activeTestImeDevices.delete(deviceKey); + return await restoreAndroidTestImeFor(resolveAndroidAdbExecutor(device), device); +} + +// The device was inspected and the helper is confirmed off it (restored, already not the active +// IME, or no rebind record). A `set-failed` (still stuck), `record-unreadable` (the device may be +// on Android's fallback IME), `owned-by-live-session` (a live session will restore it on close) +// or `not-activated-here` (nothing was inspected) keeps its pending marker for a later retry. function isDeviceRecoveryComplete(reason: AndroidTestImeRestoreReason): boolean { return reason === 'ok' || reason === 'helper-not-active' || reason === 'no-record'; } -// Undo the helper switch on one device. Invariants the review requires: +// Undo the helper switch on one device. Invariants: // - Never restore a device a live session in this process owns (the fire-and-forget startup race). // - Only touch the IME when the helper is STILL the active input method, or the device record // marks an unconfirmed rebind. If the user (or a concurrent session) switched away, leave it. // - Only clear the persisted recovery value AFTER confirming the previous IME is actually // restored (read-back). A failed `ime set` keeps the value so recovery can retry. +// - Every issued emulator restore registers its flush window here, the only site that knows +// the write was made, so no caller — close-time or startup-orphan — and no future call site +// can restore without registering. async function restoreAndroidTestImeFor( adb: AndroidAdbExecutor, device: DeviceInfo, @@ -104,26 +146,51 @@ async function restoreAndroidTestImeFor( }); return { restored: false, previousIme, reason: 'helper-not-active' }; } - await runAdbShell(adb, ['ime', 'set', previousIme], { allowFailure: true, timeoutMs: 10_000 }); - const afterIme = await readAndroidDefaultInputMethod(adb); - if (afterIme !== previousIme) { - // Restore did not take effect. Keep the persisted value so recovery can retry — clearing it - // now would permanently strand the user on the helper IME. + // The provider's window opens when the device accepts this write, so the pending entry + // opens before issuing it: a kill-bound waiter drains in-flight writes before consulting + // marks and never reads the gap as "nothing owed". A `finally` close runs after the try + // body's registration, so a waiter never sees the entry closed while its mark is absent. + const pendingWriteOpens = beginTestImeRestoreWrite(device.id); + let writeConfirmed = false; + try { + await runAdbShell(adb, ['ime', 'set', previousIme], { + allowFailure: true, + timeoutMs: 10_000, + }); + const afterIme = await readAndroidDefaultInputMethod(adb); + if (afterIme !== previousIme) { + // Restore did not take effect. Keep the persisted value so recovery can retry — clearing + // it now would permanently strand the user on the helper IME. + emitAndroidAdbDiagnostic({ + level: 'warn', + phase: 'android_test_ime_restore_failed', + data: { device: deviceLabel, previousIme, afterIme }, + }); + return { restored: false, previousIme, reason: 'set-failed' }; + } + // Confirmed back on the previous IME — the only outcome that earns clearing the persisted + // record and the only mark provenance that may justify clearing another close's marker. + // The flush hold attaches to the ISSUED write regardless (see the `finally`). + writeConfirmed = true; + // Now it is safe to drop the recovery value. + await clearPersistedPreviousIme(adb).catch(() => {}); + await clearPersistedRebindDisplacement(adb).catch(() => {}); emitAndroidAdbDiagnostic({ - level: 'warn', - phase: 'android_test_ime_restore_failed', - data: { device: deviceLabel, previousIme, afterIme }, + phase: 'android_test_ime_restored', + data: { device: deviceLabel, previousIme }, }); - return { restored: false, previousIme, reason: 'set-failed' }; + return { restored: true, previousIme, reason: 'ok' }; + } finally { + // An issued emulator restore owes the kill-bound flush hold whatever the outcome: a + // readback mismatch or post-dispatch throw cannot prove the provider never accepted the + // write. Registering here, before the pending entry closes, keeps "a drained waiter never + // sees closed-but-unregistered" a property of this one site; the mark carries whether + // this write confirmed, which is the only fact that may retire retry evidence. + if (device.kind === 'emulator') { + registerTestImeRestore(device.id, writeConfirmed); + } + pendingWriteOpens(); } - // Confirmed back on the previous IME — now it is safe to drop the recovery value. - await clearPersistedPreviousIme(adb).catch(() => {}); - await clearPersistedRebindDisplacement(adb).catch(() => {}); - emitAndroidAdbDiagnostic({ - phase: 'android_test_ime_restored', - data: { device: deviceLabel, previousIme }, - }); - return { restored: true, previousIme, reason: 'ok' }; } // Best-effort: restore any test IME left active by a crashed daemon run. Gated on the device-scoped @@ -177,7 +244,11 @@ export async function restoreOrphanedAndroidTestImeOnDaemonStartup(params: { data: { device: serial, previousIme: result.previousIme }, }); } - if (isDeviceRecoveryComplete(result.reason)) { + // The inner restore has registered the device's flush window; startup never kills and + // never waits, so its settle outcome is 'idle' — the same single clear rule the close + // path applies, not a second notion of "done". The deadline stays in the map for the + // next kill-bound path (close --shutdown or the shutdown runtime) to wait out. + if (isRecoveryMarkerClearEarned(result.reason, 'idle')) { await markers.clear(params.stateDir, serial); } }); diff --git a/packages/platform-android/src/ime-state.ts b/packages/platform-android/src/ime-state.ts index f4fc58dfb5..468af85f89 100644 --- a/packages/platform-android/src/ime-state.ts +++ b/packages/platform-android/src/ime-state.ts @@ -1,8 +1,10 @@ import type { DeviceInfo } from '@agent-device/kernel/device'; import { withKeyedLock } from '@agent-device/kernel/keyed-lock'; +import { sleep } from '@agent-device/host-kit/retry'; import { getAndroidImeHelperDeviceKey } from './ime-helper.ts'; -// Process-lived test-IME ownership state, shared by activation, restore, and startup recovery. +// Process-lived test-IME ownership state, shared by activation, restore, startup recovery, and +// the emulator kill path. export type AndroidTestImeOwnership = { /** The recovery scope whose lock and marker guard this device's IME. */ @@ -11,6 +13,185 @@ export type AndroidTestImeOwnership = { rebindUnconfirmed: boolean; }; +// Android's SettingsProvider persists a setting change asynchronously: `ime set` answers from +// memory and the XML flush follows up to MAX_WRITE_SETTINGS_DELAY_MILLIS later (AOSP +// SettingsState.java, 2020ms cap). A device killed inside that window reboots from the stale +// file, so a restore followed by an immediate `adb emu kill` loses the keyboard on restart. +// The provider exposes no shell flush; holding the kill past the cap is the owning fix. The +// margin past the cap only has to absorb the AtomicFile rename. Exported so tests derive their +// seeded elapsed times and sleep bounds from the window itself, never from a copied number. +export const SETTINGS_PROVIDER_FLUSH_SETTLE_MS = 2_500; + +// A device's restore mark: when the most recent registered write happened, and whether that +// write confirmed. @internal the map is exported for tests; production touches it only +// through the helpers here. +export type TestImeRestoreMark = Readonly<{ + /** Monotonic process-clock time (performance.now) of the write that earned this mark. */ + atPerfMs: number; + /** True only when a read-back confirmed the write; an issued-but-unconfirmed write is false. */ + confirmed: boolean; +}>; + +// Per-device restore marks, keyed by serial. Timed entirely on the PROCESS MONOTONIC CLOCK +// (performance.now) — never the wall clock, as the field name encodes, because sleep() runs +// on libuv's monotonic loop clock, so a wall-clock-derived remaining time mixes two clocks, +// and a forward NTP/VM-resume step could release the kill inside the window. Marks only move +// forward, so windows coalesce into one wait. A mark's provenance says whether its write +// confirmed by read-back (`confirmed`) or was merely issued. The kill-side wait consumes +// only atPerfMs — every issued emulator write owes the hold. Marker clearing consumes +// `confirmed`: only a covered CONFIRMED window lets an uninspected close retire another +// close's retained retry evidence (see isRecoveryMarkerClearEarned). Every path that may +// `adb emu kill` calls awaitTestImeFlushWindow first, and the kill-bound close consumes the +// outcome so the pending marker clears only under a completed (or never-owed) wait. +// The mark does NOT survive a daemon restart: a dead process's write cannot be re-derived +// here. Restart-durable evidence needs a clock that outlives the process plus an expiry rule +// — the forfeit and its alternatives are tracked as #3346. +export const testImeRestoreMarks = new Map(); + +// In-flight `ime set` writes, keyed by serial. The provider's window opens when the device +// ACCEPTS the write, but the mark exists only after the registrar's finally registers it; a +// kill-bound wait reading marks alone would find none and fire adb mid-write. The registrar +// opens the entry before issuing the write and closes it only after that write's mark +// registered — every issued emulator `ime set` registers one, whatever the outcome. Only a +// non-emulator restore closes without a mark (no flush hazard there). A wait that returned +// just before a write opens cannot be caught by in-process state at all; closing that residue +// needs a lock spanning kill and restore, which the shutdown-runtime contract (commands +// only, no stateDir) cannot form. +// @internal exported for the registrar and for tests. +export const testImePendingRestoreWrites = new Map>>(); + +/** Opens in-flight tracking for one issued `ime set`; returns the closing function. */ +export function beginTestImeRestoreWrite(serial: string): () => void { + let writes = testImePendingRestoreWrites.get(serial); + if (!writes) { + writes = new Set(); + testImePendingRestoreWrites.set(serial, writes); + } + let settle!: () => void; + const settled = new Promise((resolve) => { + settle = resolve; + }); + writes.add(settled); + return () => { + writes.delete(settled); + if (writes.size === 0 && testImePendingRestoreWrites.get(serial) === writes) { + testImePendingRestoreWrites.delete(serial); + } + settle(); + }; +} + +function pendingTestImeRestoreWrite(serial: string): Promise | undefined { + const writes = testImePendingRestoreWrites.get(serial); + if (!writes || writes.size === 0) return undefined; + return Promise.allSettled(writes).then(() => undefined); +} + +async function awaitUntilAborted(pending: Promise, signal?: AbortSignal): Promise { + if (!signal) { + await pending; + return; + } + await new Promise((resolve) => { + const finish = () => { + signal.removeEventListener('abort', finish); + resolve(); + }; + signal.addEventListener('abort', finish, { once: true }); + void pending.then(finish, finish); + }); +} + +export type TestImeFlushWait = + /** No registered window existed to wait for. */ + | 'idle' + /** Cancelled before covering; the mark stays for the next kill-bound caller. */ + | 'aborted' + /** Covered a window whose newest write issued a restore that never confirmed. */ + | 'covered-issued' + /** Covered a window whose newest write confirmed by read-back. */ + | 'covered-confirmed'; + +// Every issued emulator restore registers its mark from the registrar's finally, whatever the +// outcome; `confirmed` carries that write's own provenance. A later write REPLACES the +// provenance rather than inheriting it: an issued-unconfirmed write after a confirmed one can +// have re-displaced the IME, and only the newest write's confirmation justifies clearing +// another close's retry evidence. atPerfMs only moves forward, so windows coalesce. +export function registerTestImeRestore(serial: string, confirmed: boolean): void { + const previousPerfMs = testImeRestoreMarks.get(serial)?.atPerfMs ?? 0; + testImeRestoreMarks.set(serial, { + atPerfMs: Math.max(previousPerfMs, performance.now()), + confirmed, + }); +} + +// Wait until the device's newest restore write is older than the flush window. Returns 'idle' +// when this process has no registered window (nothing to wait for), 'covered-issued' or +// 'covered-confirmed' once the newest mark's window has elapsed — retiring the mark, since +// nothing older can still be unflushed, and reporting that newest mark's provenance — and +// 'aborted' when cancelled first: the caller must then skip the kill, and the mark stays so +// the next kill-bound path derives the same remaining wait. +// Rounds repeat until nothing newer appears: a restore landing (or even ISSUING a write) while +// a round sleeps carries a newer deadline, and this caller owns the kill, so reporting covered +// on an older mark would release it inside the newer window — #3318 again. +export async function awaitTestImeFlushWindow( + serial: string, + signal?: AbortSignal, +): Promise { + if (signal?.aborted) return 'aborted'; + for (;;) { + const outcome = await runTestImeFlushRound(serial, signal); + if (outcome !== 'extended') return outcome; + } +} + +// One round: drain in-flight writes, then sleep out the newest mark's remainder and consume +// exactly that mark. 'extended' means a newer deadline appeared and another round must run. +async function runTestImeFlushRound( + serial: string, + signal?: AbortSignal, +): Promise { + if (!(await quietTestImeRestoreWrites(serial, signal))) return 'aborted'; + const mark = testImeRestoreMarks.get(serial); + if (mark === undefined) return 'idle'; + const remainingMs = mark.atPerfMs + SETTINGS_PROVIDER_FLUSH_SETTLE_MS - performance.now(); + if (remainingMs > 0) { + await sleep(remainingMs, signal); + if (signal?.aborted) return 'aborted'; + } + return consumeTestImeFlushMark(serial, mark); +} + +// True once no `ime set` this process issued is still unregistered. The registrar closes its +// pending entry only after that write's mark registered (non-emulator restores register none), +// so a drained, mark-less state truly means no registered window is owed — including one a +// concurrent waiter just retired, which the caller reports as the conservative 'idle'. +async function quietTestImeRestoreWrites(serial: string, signal?: AbortSignal): Promise { + const pending = pendingTestImeRestoreWrite(serial); + if (!pending) return true; + await awaitUntilAborted(pending, signal); + return !signal?.aborted; +} + +// Consume the mark one round waited for — identity, not timestamp: every registration stores +// a fresh object, so a replaced mark is never mistaken for the one waited on. Unchanged or +// already retired means covered with that mark's provenance; a newly opened write or a +// replaced mark means a newer deadline exists and the caller must loop. With no await between +// the read and the delete, the delete cannot erase a mark the round has not waited for, so an +// extension always survives to its own round. +function consumeTestImeFlushMark( + serial: string, + waited: TestImeRestoreMark, +): TestImeFlushWait | 'extended' { + if (pendingTestImeRestoreWrite(serial)) return 'extended'; + const current = testImeRestoreMarks.get(serial); + if (current === undefined || current === waited) { + testImeRestoreMarks.delete(serial); + return waited.confirmed ? 'covered-confirmed' : 'covered-issued'; + } + return 'extended'; +} + // Per-daemon-process cache of devices with the test IME active; input-actions.ts reads this to // route text entry through the broadcast channel. export const activeTestImeDevices = new Map(); @@ -36,10 +217,12 @@ export function withAndroidTestImeRecoveryLock( } /** - * @internal Test isolation hook for the active test-IME device set. + * @internal Test isolation hook for the active test-IME device set and flush timestamps. */ export function resetAndroidTestImeActivationCacheForTests(): void { activeTestImeDevices.clear(); + testImeRestoreMarks.clear(); + testImePendingRestoreWrites.clear(); } /** diff --git a/packages/platform-android/src/lifecycle.test.ts b/packages/platform-android/src/lifecycle.test.ts index 5cad49f7c9..0a417adbfc 100644 --- a/packages/platform-android/src/lifecycle.test.ts +++ b/packages/platform-android/src/lifecycle.test.ts @@ -1,5 +1,6 @@ import { afterEach, expect, test, vi } from 'vitest'; import type { + CloseApplicationFinalizationInput, LocalApplicationInteractorHost, OpenApplicationInput, } from '@agent-device/contracts/application-lifecycle-runtime'; @@ -32,6 +33,11 @@ function createLifecycle( snapshot?: (options: SnapshotOptions) => Promise; openedAppBundleId?: string; signal?: AbortSignal; + restoreTestIme?: ( + device: DeviceInfo, + input: Readonly<{ stateDir: string; shutdownTarget: boolean; signal: AbortSignal }>, + ) => Promise; + shutdownTarget?: () => Promise; }> = {}, ): LifecycleFixture { const calls: string[] = []; @@ -56,7 +62,9 @@ function createLifecycle( android: { ensureReady: async () => ({ ...device, booted: true }) }, }, deviceShutdown: { - android: { shutdownTarget: async () => undefined }, + android: { + shutdownTarget: async () => await (params.shutdownTarget?.() ?? Promise.resolve(undefined)), + }, }, androidApplications: { resolveOpenTarget: async () => ({}), @@ -66,7 +74,7 @@ function createLifecycle( applyRuntimeHints: async () => {}, clearRuntimeHints: async () => {}, activateTestIme: async () => {}, - restoreTestIme: async () => {}, + restoreTestIme: params.restoreTestIme ?? (async () => {}), recoverTestImeStartup: async () => {}, hasTestImeRecoveryEvidence: async () => false, }, @@ -213,3 +221,69 @@ test('an app open whose launched package cannot be identified reports it unident expect(calls).toEqual(['open:Example']); expect(outcome.timing.postOpenObservation).toBe('app-unidentified'); }); + +function finalizationInput( + overrides: Partial = {}, +): CloseApplicationFinalizationInput { + return { + appBundleId: 'com.example.app', + surface: 'app', + retainRunner: false, + stateDir: '/state', + ...overrides, + }; +} + +test('close --shutdown restores the keyboard and only then kills the emulator (#3318)', async () => { + const sequence: string[] = []; + let finishRestore: () => void = () => {}; + const { lifecycle } = createLifecycle({ + restoreTestIme: async (_device, input) => { + sequence.push(`restore:${input.shutdownTarget}`); + await new Promise((resolve) => { + finishRestore = resolve; + }); + sequence.push('restored'); + }, + shutdownTarget: async () => { + sequence.push('shutdown'); + return { success: true, exitCode: 0, stdout: '', stderr: '' }; + }, + }); + + let settled = false; + const finalizing = lifecycle + .finalizeApplicationClose(finalizationInput({ shutdownTarget: true })) + .then((result) => { + settled = true; + return result ?? {}; + }); + await vi.waitFor(() => expect(sequence).toEqual(['restore:true'])); + await Promise.resolve(); + // The kill must wait on the awaited restore, which absorbs the settings-provider flush window. + expect(settled).toBe(false); + expect(sequence).toEqual(['restore:true']); + finishRestore(); + + const result = await finalizing; + expect(sequence).toEqual(['restore:true', 'restored', 'shutdown']); + expect(result.shutdown).toMatchObject({ success: true }); +}); + +test('an ordinary close tells the restore no emulator kill is coming', async () => { + const sequence: string[] = []; + const { lifecycle } = createLifecycle({ + restoreTestIme: async (_device, input) => { + sequence.push(`restore:${input.shutdownTarget}`); + }, + shutdownTarget: async () => { + sequence.push('shutdown'); + return { success: true, exitCode: 0, stdout: '', stderr: '' }; + }, + }); + + const result = await lifecycle.finalizeApplicationClose(finalizationInput()); + + expect(sequence).toEqual(['restore:false']); + expect(result).toEqual({}); +}); diff --git a/packages/platform-android/src/lifecycle.ts b/packages/platform-android/src/lifecycle.ts index 7e27d6a91b..20d19fb375 100644 --- a/packages/platform-android/src/lifecycle.ts +++ b/packages/platform-android/src/lifecycle.ts @@ -77,8 +77,12 @@ export function bindAndroidApplicationLifecycle( }); }, finalizeApplicationClose: async (input) => { + // The restore absorbs the settings-provider flush window before an emulator kill starts + // (#3318); it must not run concurrently with the shutdown. await host.androidApplications.restoreTestIme(device, { stateDir: input.stateDir, + shutdownTarget: input.shutdownTarget === true && device.kind === 'emulator', + signal, }); const shutdown = input.shutdownTarget && device.kind === 'emulator' diff --git a/packages/platform-android/src/shutdown/runtime.test.ts b/packages/platform-android/src/shutdown/runtime.test.ts index 1ecd21924f..3baf1973b8 100644 --- a/packages/platform-android/src/shutdown/runtime.test.ts +++ b/packages/platform-android/src/shutdown/runtime.test.ts @@ -3,15 +3,44 @@ import type { DeviceInfo } from '@agent-device/kernel/device'; import type { DeviceShutdownRuntimeDependencies } from '@agent-device/contracts/device-shutdown-runtime'; import { canShutdownTarget, createAndroidShutdownRuntime } from './runtime.ts'; +const sleep = vi.hoisted(() => vi.fn(async (_ms: number, _signal?: AbortSignal) => {})); +vi.mock('@agent-device/host-kit/retry', async (importOriginal) => ({ + ...(await importOriginal()), + sleep, +})); + +import { + beginTestImeRestoreWrite, + registerTestImeRestore, + SETTINGS_PROVIDER_FLUSH_SETTLE_MS, + testImePendingRestoreWrites, + testImeRestoreMarks, +} from '../ime-state.ts'; +import { seedRestoreMark } from '../ime-device.fixtures.ts'; + const run = vi.fn(async () => ({ stdout: '', stderr: '', exitCode: 0 })); const commands: DeviceShutdownRuntimeDependencies['commands'] = { which: async () => 'adb', run, }; +// Every expected sleep band is derived from the real window constant (proportional seeds and +// tolerances), so changing SETTINGS_PROVIDER_FLUSH_SETTLE_MS shifts these tests' meaning +// instead of turning them red for a confusing arithmetic reason. +const WINDOW_TOLERANCE_MS = Math.round(SETTINGS_PROVIDER_FLUSH_SETTLE_MS / 10); +// Lower bound for "the window had only just opened" waits. +const nearlyFullWindowFloorMs = SETTINGS_PROVIDER_FLUSH_SETTLE_MS - WINDOW_TOLERANCE_MS; +// The abort/coalescing tests seed a KNOWN elapsed fraction of the window (40%), so the +// expected remainder is a known 60% — a fixed sleep anywhere else fails on the number. +const seededElapsedMs = Math.round(SETTINGS_PROVIDER_FLUSH_SETTLE_MS * 0.4); +const seededRemainderMs = SETTINGS_PROVIDER_FLUSH_SETTLE_MS - seededElapsedMs; + beforeEach(() => { run.mockReset(); run.mockResolvedValue({ stdout: '', stderr: '', exitCode: 0 }); + sleep.mockReset(); + testImeRestoreMarks.clear(); + testImePendingRestoreWrites.clear(); }); test('an already-stopped emulator succeeds without adb', async () => { @@ -45,10 +74,230 @@ test('only Android emulators are available', () => { expect(canShutdownTarget({ ...androidDevice(), platform: 'web' })).toBe(false); }); +// #3318: this runtime is the only executor of `adb emu kill`, so the flush-window hold lives +// here — every kill path (close --shutdown, the standalone shutdown command, any future +// caller) inherits it without knowing about the test IME. +test('a kill waits out a registered test-IME flush window before running adb emu kill', async () => { + const device = androidDevice(); + seedRestoreMark(device.id, 0); + // A deferred sleep, not an instantly-resolving one: the point of this hold is that `adb emu + // kill` does not run WHILE the wait is pending, so the test has to observe the middle of + // the wait, not just its endpoints. + const settling = deferred(); + sleep.mockImplementationOnce(() => settling.promise); + + const pendingKill = createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal()); + + // One macrotask boundary: the wait drains (async) in-flight writes before its first sleep. + await drainMicrotasks(); + + // The runtime suspends inside the flush wait before touching adb. + expect(sleep).toHaveBeenCalledTimes(1); + // The wait must cover the registered window, not merely be positive: the timestamp was set + // milliseconds before the kill, so the remaining settle is the whole budget minus that gap. + // A regression sleeping a fixed unrelated amount would fail here. + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThanOrEqual(nearlyFullWindowFloorMs); + expect(run).not.toHaveBeenCalled(); + + settling.resolve(); + await expect(pendingKill).resolves.toEqual(success()); + + expect(run).toHaveBeenCalledTimes(1); + expect(run).toHaveBeenCalledWith( + expect.objectContaining({ args: ['-s', device.id, 'emu', 'kill'] }), + expect.any(AbortSignal), + ); + expect([...testImeRestoreMarks.keys()]).toEqual([]); +}); + +// The mark only exists after `ime set` returns and the readback confirms it, but the +// provider's window opens when the device ACCEPTS the write. A kill reading the map +// mid-write sees nothing registered and must not treat that as 'idle'. +test('a kill started while a restore write is in flight waits for it to register', async () => { + const device = androidDevice(); + // Exactly what restoreAndroidTestImeFor does between issuing `ime set` and registering. + const closeInFlightWrite = beginTestImeRestoreWrite(device.id); + const settling = deferred(); + sleep.mockImplementationOnce(() => settling.promise); + + const pendingKill = createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal()); + + // Advance one event-loop turn so the kill REALLY reaches the wait: asserting synchronously + // after starting the call would pass even if the wait took 'idle' straight through, because + // the kill would still be suspended at its first microtask. With a full turn drained and the + // write still in flight and unregistered, a mark-only implementation has already taken + // 'idle' and called adb — that is the #3318 mid-write kill this test names. + await drainMicrotasks(); + + // Neither the window wait nor adb has started: the in-flight write holds the kill. + expect(sleep).not.toHaveBeenCalled(); + expect(run).not.toHaveBeenCalled(); + + // The write lands and registers its window, closing the in-flight entry exactly as the + // registrar's finally block does. + registerTestImeRestore(device.id, true); + closeInFlightWrite(); + await drainMicrotasks(); + + expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThanOrEqual(nearlyFullWindowFloorMs); + expect(run).not.toHaveBeenCalled(); + + settling.resolve(); + await expect(pendingKill).resolves.toEqual(success()); + expect(run).toHaveBeenCalledTimes(1); + expect([...testImeRestoreMarks.keys()]).toEqual([]); +}); + +test('a pending write that closes with no registered mark releases the kill as idle', async () => { + // The registrar opens the pending entry for every restore but registers a window only for + // emulators (the flush hazard is the emulator's SettingsProvider), so a physical-device + // restore is the production shape of "closed, never registered". The drained wait must fall + // through to 'idle' — never strand the kill on an entry that will never gain a mark. + const device = androidDevice(); + const closeInFlightWrite = beginTestImeRestoreWrite(device.id); + + const pendingKill = createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal()); + expect(run).not.toHaveBeenCalled(); + + closeInFlightWrite(); + + await expect(pendingKill).resolves.toEqual(success()); + expect(sleep).not.toHaveBeenCalled(); + expect(run).toHaveBeenCalledTimes(1); +}); + +// A restore landing mid-wait must EXTEND this very kill's wait, not just survive for some +// hypothetical next caller — reporting 'covered' on the older mark would fire adb inside the +// newer restore's window, which is #3318 again. +test('a restore landing mid-wait extends the pending kill instead of releasing it', async () => { + const device = androidDevice(); + // Known elapsed: the original mark is 40% of the window old, so the first wait must derive + // the remaining 60% — proportional to the constant, not a fixed number. + seedRestoreMark(device.id, seededElapsedMs); + const firstSettling = deferred(); + const secondSettling = deferred(); + sleep + .mockImplementationOnce(() => firstSettling.promise) + .mockImplementationOnce(() => secondSettling.promise); + + const pendingKill = createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal()); + + // One macrotask boundary: the wait drains (async) in-flight writes before its first sleep. + await drainMicrotasks(); + + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThan(seededRemainderMs - WINDOW_TOLERANCE_MS); + expect(sleep.mock.calls[0]?.[0]).toBeLessThanOrEqual(seededRemainderMs); + + // A second confirmed restore lands while this kill sleeps — a newer write, a newer deadline. + registerTestImeRestore(device.id, true); + firstSettling.resolve(); + await drainMicrotasks(); + + // The wait re-reads the mark and takes a SECOND sleep covering the newer window, still + // refusing adb. A one-read-one-sleep implementation returns 'covered' here and kills early. + expect(sleep).toHaveBeenCalledTimes(2); + expect(sleep.mock.calls[1]?.[0]).toBeGreaterThanOrEqual(nearlyFullWindowFloorMs); + expect(sleep.mock.calls[1]?.[0]).toBeLessThanOrEqual(SETTINGS_PROVIDER_FLUSH_SETTLE_MS); + expect(run).not.toHaveBeenCalled(); + + secondSettling.resolve(); + await expect(pendingKill).resolves.toEqual(success()); + + expect(run).toHaveBeenCalledTimes(1); + expect([...testImeRestoreMarks.keys()]).toEqual([]); +}); + +test('a forward host wall-clock jump cannot shorten a registered flush window', async () => { + // The window is timed on the process monotonic clock, so an NTP/VM-resume step of the wall + // clock must leave the kill-side remaining wait at the full budget. + const device = androidDevice(); + seedRestoreMark(device.id, 0); + const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(Number.MAX_SAFE_INTEGER); + + try { + await expect( + createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal()), + ).resolves.toEqual(success()); + } finally { + nowSpy.mockRestore(); + } + + expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep.mock.calls[0]?.[0]).toBeGreaterThanOrEqual(nearlyFullWindowFloorMs); +}); + +test('a kill skips the wait entirely when no flush window is open', async () => { + const device = androidDevice(); + + await expect( + createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal()), + ).resolves.toEqual(success()); + + expect(sleep).not.toHaveBeenCalled(); + expect(run).toHaveBeenCalledTimes(1); +}); + +test('a kill cancelled inside the flush window never reaches adb emu kill', async () => { + const device = androidDevice(); + // Known elapsed: the mark is 40% of the window old, so the derived remainder must be the + // known 60% — not the full budget, not a fixed constant. That makes both waits provably a + // function of the mark. + seedRestoreMark(device.id, seededElapsedMs); + const controller = new AbortController(); + sleep.mockImplementationOnce((_ms, waitSignal) => { + controller.abort(); + return waitSignal?.aborted ? Promise.resolve() : new Promise(() => {}); + }); + + // Pin the cancellation MECHANISM, not just a rejection: awaitTestImeFlushWindow returns + // 'aborted' and shutdownAndroidTarget cancels via signal.throwIfAborted(), which throws the + // signal's exact reason object. An unrelated rejection (a throw inside the flush wait, a + // normalization bug) must fail this test, not stand in for cancellation. + const failure = await createAndroidShutdownRuntime({ commands }) + .shutdownTarget(device, controller.signal) + .catch((error: unknown) => error); + expect(failure).toBe(controller.signal.reason); + expect((failure as Error).name).toBe('AbortError'); + + expect(run).not.toHaveBeenCalled(); + const firstRemainingMs = sleep.mock.calls[0]?.[0] as number; + expect(firstRemainingMs).toBeGreaterThan(seededRemainderMs - WINDOW_TOLERANCE_MS); + expect(firstRemainingMs).toBeLessThanOrEqual(seededRemainderMs); + // The window stays registered so a retry still waits out the remainder — and the retry + // re-derives the SAME remainder from the surviving mark: the monotonic-mark property the + // abort design rests on, pinned by number, not by shape. + expect([...testImeRestoreMarks.keys()]).toEqual([device.id]); + + const retry = await createAndroidShutdownRuntime({ commands }).shutdownTarget(device, signal()); + + expect(retry).toEqual(success()); + expect(sleep).toHaveBeenCalledTimes(2); + const secondRemainingMs = sleep.mock.calls[1]?.[0] as number; + expect(secondRemainingMs).toBeGreaterThan(seededRemainderMs - WINDOW_TOLERANCE_MS); + expect(secondRemainingMs).toBeLessThanOrEqual(firstRemainingMs); +}); + function signal(): AbortSignal { return new AbortController().signal; } +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +// Let the awaited-sleep continuations run: the kill path resumes through promise callbacks, so +// assertions about the middle of the wait need one macrotask boundary, not one microtask tick. +function drainMicrotasks(): Promise { + return new Promise((resolve) => { + setTimeout(resolve, 0); + }); +} + function success() { return { success: true, exitCode: 0, stdout: '', stderr: '' }; } diff --git a/packages/platform-android/src/shutdown/runtime.ts b/packages/platform-android/src/shutdown/runtime.ts index 04826a0a48..bf0a8acf60 100644 --- a/packages/platform-android/src/shutdown/runtime.ts +++ b/packages/platform-android/src/shutdown/runtime.ts @@ -2,6 +2,7 @@ import type { DeviceShutdownRuntimeDependencies } from '@agent-device/contracts/ import type { TargetShutdownResult } from '@agent-device/contracts/device'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { normalizeError } from '@agent-device/kernel/errors'; +import { awaitTestImeFlushWindow } from '../ime-state.ts'; const SHUTDOWN_TIMEOUT_MS = 15_000; @@ -31,6 +32,13 @@ async function shutdownAndroidTarget( ): Promise { if (device.booted === false) return stoppedTargetSuccess(); + signal.throwIfAborted(); + // Every kill of an emulator goes through here, so this is where the settings-provider flush + // window is enforced for ALL kill paths — close --shutdown, the standalone shutdown command, + // and any future caller. A restore this process registered postpones the kill until the + // provider has rewritten its file; an abort cancels the kill rather than letting it land + // inside the window, and the timestamp stays for the retry to wait out the remainder. + await awaitTestImeFlushWindow(device.id, signal); signal.throwIfAborted(); try { const result = await commands.run( diff --git a/src/platform-runtime-android-application-tools.ts b/src/platform-runtime-android-application-tools.ts index 58df264dc1..167be21c8f 100644 --- a/src/platform-runtime-android-application-tools.ts +++ b/src/platform-runtime-android-application-tools.ts @@ -78,7 +78,11 @@ export function createAndroidApplicationTools(): AndroidApplicationTools { }, restoreTestIme: async (device, input) => { const { restoreAndroidTestIme } = await loadAndroidMechanics(); - const result = await restoreAndroidTestIme(device, { stateDir: input.stateDir }); + const result = await restoreAndroidTestIme(device, { + stateDir: input.stateDir, + shutdownTarget: input.shutdownTarget, + signal: input.signal, + }); if (result.reason !== 'set-failed' && result.reason !== 'record-unreadable') return; throw new AppError( 'COMMAND_FAILED',