From c8fae0f8a59c5b3326e5b0691814519be58f7336 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 18:59:58 +0200 Subject: [PATCH 01/12] fix(android): read device ownership off the device, not off adb The snapshot helper retirement treated the outcome of an `am force-stop` call as the fact it was supposed to measure. On a loaded host the adb round trip exceeds its budget while the helper process is already gone, so a completed interaction failed its own teardown, and the quarantine that followed refused the next command with "could not confirm release of device automation ownership" on a device that had nothing holding UiAutomation. Ownership is a device fact, so it is now read as one: `adb shell pidof` answers released, occupied, or unknown, and only a device that names a live helper process may refuse a command. Teardown records what it could not prove for the next acquire and never decides what the command it is finishing reports, which is what let a settled `press` turn into a stale-coordinate failure in #2553. A start that only ran out of time or lost its transport no longer excludes the helper identity for the rest of the daemon's life; it retries after a cooldown. Only a helper that ran and exited before announcing readiness proves the identity unusable. --- .../__tests__/snapshot-helper-capture.test.ts | 52 +++-- .../snapshot-helper-retirement.test.ts | 123 +++++++---- .../snapshot-helper-session-lifecycle.test.ts | 57 +++-- .../snapshot-helper-session.fixtures.ts | 32 ++- .../__tests__/snapshot-helper-session.test.ts | 47 ++-- .../src/__tests__/snapshot.test.ts | 24 +-- .../__tests__/touch-helper-session.test.ts | 80 ++++--- packages/platform-android/src/mechanics.ts | 2 +- .../src/snapshot-helper-retirement.ts | 201 ++++++++++++------ .../src/snapshot-helper-session-lifecycle.ts | 162 ++++++++------ .../src/snapshot-helper-session-protocol.ts | 14 ++ .../platform-android/src/snapshot-helper.ts | 2 +- packages/platform-android/src/snapshot.ts | 4 +- 13 files changed, 541 insertions(+), 259 deletions(-) diff --git a/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts index f6927689c5..000b940eb6 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts @@ -31,14 +31,17 @@ test('one-shot capture that resolves during cancellation retires before rejectin if (options?.signal?.aborted) onAbort(); }); } + if (args.join(' ').includes('pidof')) { + return { exitCode: 1, stdout: '', stderr: '' }; + } assert.deepEqual(args, [ 'shell', 'am', 'force-stop', 'com.callstack.agentdevice.snapshothelper', ]); - assert.notEqual(options?.signal, controller.signal); - assert.equal(options?.signal?.aborted, false); + // The retirement stop must not inherit the aborted command signal; it bounds itself. + assert.ok(!options?.signal?.aborted); events.push('retirement-started'); await retirementCanFinish; events.push('retirement-finished'); @@ -70,22 +73,26 @@ test('one-shot capture that resolves during cancellation retires before rejectin ]); }); -test('uncertain one-shot retirement is quarantined until the next capture recovers it', async () => { +test('canceled one-shot capture reports the cancellation and the next capture recovers the device', async () => { const controller = new AbortController(); + const cancellation = new Error('wait deadline exceeded'); const events: string[] = []; - let forceStopCount = 0; + let stopCount = 0; const adb: AndroidAdbExecutor = async (args, options) => { if (args.join(' ').includes('am force-stop')) { - forceStopCount += 1; - events.push(`force-stop-${forceStopCount}`); - return { - exitCode: forceStopCount === 1 ? 1 : 0, - stdout: '', - stderr: forceStopCount === 1 ? 'runtime still busy' : '', - }; + stopCount += 1; + events.push(`force-stop-${stopCount}`); + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (args.join(' ').includes('pidof')) { + // The first read happens while Android still runs the helper; the next says it is gone. + events.push('pidof'); + return stopCount === 1 + ? { exitCode: 0, stdout: '4211\n', stderr: '' } + : { exitCode: 1, stdout: '', stderr: '' }; } - events.push(`instrument-${forceStopCount}`); - if (forceStopCount === 0) { + events.push(`instrument-${stopCount}`); + if (stopCount === 0) { return await new Promise((_resolve, reject) => { const onAbort = () => reject(options?.signal?.reason); options?.signal?.addEventListener('abort', onAbort, { once: true }); @@ -103,21 +110,24 @@ test('uncertain one-shot retirement is quarantined until the next capture recove deviceKey: 'android:emulator-5554', signal: controller.signal, }); - controller.abort(new Error('wait deadline exceeded')); + controller.abort(cancellation); - await assert.rejects( - canceledCapture, - (error: unknown) => - (error as { details?: { reason?: string } }).details?.reason === - 'android_snapshot_helper_retirement_unconfirmed', - ); + // The ownership question never replaces the reason this command stopped. + await assert.rejects(canceledCapture, cancellation); const recovered = await captureAndroidSnapshotWithHelper({ adb, deviceKey: 'android:emulator-5554', }); assert.match(recovered.xml, /recovered/); - assert.deepEqual(events, ['instrument-0', 'force-stop-1', 'force-stop-2', 'instrument-2']); + assert.deepEqual(events, [ + 'instrument-0', + 'force-stop-1', + 'pidof', + 'force-stop-2', + 'pidof', + 'instrument-2', + ]); }); function helperOutput(xml: string): string { diff --git a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts index 3d5b6b8de1..9d5702e68e 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts @@ -3,7 +3,9 @@ import { beforeEach, test } from 'vitest'; import { EventEmitter } from 'node:events'; import { PassThrough } from 'node:stream'; import { + isAndroidSnapshotHelperRuntimeOccupiedError, recoverAndroidSnapshotHelperRetirement, + recordAndroidSnapshotHelperRelease, resetAndroidSnapshotHelperRetirements, retireCanceledAndroidSnapshotHelperCapture, settleAndroidSnapshotHelperSessionCleanup, @@ -11,69 +13,97 @@ import { import type { AndroidAdbProcess } from '../adb-executor.ts'; import type { AndroidAdbExecutor } from '../snapshot-helper-types.ts'; +const PACKAGE_NAME = 'com.callstack.agentdevice.snapshothelper'; +const DEVICE_KEY = 'android:emulator-5554'; + beforeEach(() => { resetAndroidSnapshotHelperRetirements(); }); -test('requires positive recovery evidence after uncertain runtime retirement', async () => { +test('canceled capture answers for the device, not for the force-stop call that served it', async () => { const calls: string[][] = []; - let forceStopCount = 0; const adb: AndroidAdbExecutor = async (args) => { calls.push(args); - forceStopCount += 1; - return { - exitCode: forceStopCount === 1 ? 1 : 0, - stdout: '', - stderr: forceStopCount === 1 ? 'runtime still busy' : '', - }; + if (args.includes('force-stop')) throw new Error('adb round trip exceeded its budget'); + return { exitCode: 1, stdout: '', stderr: '' }; }; - await assert.rejects( - retireCanceledAndroidSnapshotHelperCapture({ - deviceKey: 'android:emulator-5554', - packageName: 'com.callstack.agentdevice.snapshothelper', - adb, - cause: new Error('capture canceled'), - }), - (error: unknown) => - (error as { details?: { reason?: string } }).details?.reason === - 'android_snapshot_helper_retirement_unconfirmed', - ); - await recoverAndroidSnapshotHelperRetirement({ - deviceKey: 'android:emulator-5554', + // A loaded host makes the stop call time out while the helper process is already gone. Ownership + // is decided by the process Android reports, so this retirement resolves. + await retireCanceledAndroidSnapshotHelperCapture({ + deviceKey: DEVICE_KEY, + packageName: PACKAGE_NAME, adb, + cause: new Error('capture canceled'), }); - await recoverAndroidSnapshotHelperRetirement({ - deviceKey: 'android:emulator-5554', + + assert.deepEqual(calls, [ + ['shell', 'am', 'force-stop', PACKAGE_NAME], + ['shell', 'pidof', PACKAGE_NAME], + ]); + await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); + assert.equal(calls.length, 2); +}); + +test('unproven release stays pending until an acquire reads the device', async () => { + let helperAlive = true; + const adb: AndroidAdbExecutor = async (args) => { + if (args.includes('pidof')) { + return helperAlive + ? { exitCode: 0, stdout: '4211\n', stderr: '' } + : { exitCode: 1, stdout: '', stderr: '' }; + } + return { exitCode: 0, stdout: '', stderr: '' }; + }; + + await retireCanceledAndroidSnapshotHelperCapture({ + deviceKey: DEVICE_KEY, + packageName: PACKAGE_NAME, adb, + cause: new Error('capture canceled'), }); - assert.equal(forceStopCount, 2); - assert.deepEqual( - calls, - Array.from({ length: 2 }, () => [ - 'shell', - 'am', - 'force-stop', - 'com.callstack.agentdevice.snapshothelper', - ]), + await assert.rejects( + recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }), + isAndroidSnapshotHelperRuntimeOccupiedError, ); + + helperAlive = false; + await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); + await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); +}); + +test('a device that cannot be read leaves the retirement pending without failing the command', async () => { + const adb: AndroidAdbExecutor = async (args) => { + if (args.includes('pidof')) throw new Error('device offline'); + return { exitCode: 0, stdout: '', stderr: '' }; + }; + + const release = await recordAndroidSnapshotHelperRelease({ + deviceKey: DEVICE_KEY, + packageName: PACKAGE_NAME, + adb, + cause: new Error('quit timed out'), + }); + assert.equal(release, 'unknown'); + await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); }); -test('session cleanup force-stops the runtime when release was not confirmed', async () => { +test('session cleanup stops the runtime even when the transport refuses the stop', async () => { const calls: string[][] = []; const cleanup = await settleAndroidSnapshotHelperSessionCleanup({ - adb: recordingAdb(calls), + adb: recordingAdb(calls, () => ({ exitCode: 1, stdout: '', stderr: 'device offline' })), process: new StubAndroidProcess(), port: 41234, - packageName: 'com.callstack.agentdevice.snapshothelper', + packageName: PACKAGE_NAME, timeoutMs: 2_000, forceStopRuntime: true, }); - assert.equal(cleanup.runtimeForceStopped, true); + // The stop is an action, never the release evidence; a refused call must not fail the teardown. + assert.equal(cleanup.timedOut, false); assert.deepEqual(calls, [ - ['shell', 'am', 'force-stop', 'com.callstack.agentdevice.snapshothelper'], + ['shell', 'am', 'force-stop', PACKAGE_NAME], ['forward', '--remove', 'tcp:41234'], ]); }); @@ -84,21 +114,28 @@ test('session cleanup skips the force-stop round trip once release is confirmed' adb: recordingAdb(calls), process: new StubAndroidProcess(), port: 41234, - packageName: 'com.callstack.agentdevice.snapshothelper', + packageName: PACKAGE_NAME, timeoutMs: 2_000, forceStopRuntime: false, }); - // The helper already released UiAutomation, so nothing was force-stopped and nothing may claim - // it was: the caller reads this flag to decide whether the retirement needs quarantining. - assert.equal(cleanup.runtimeForceStopped, false); + // The helper already released UiAutomation, so the only device call is the forward removal the + // next session on this port would otherwise collide with. + assert.equal(cleanup.timedOut, false); assert.deepEqual(calls, [['forward', '--remove', 'tcp:41234']]); }); -function recordingAdb(calls: string[][]): AndroidAdbExecutor { +function recordingAdb( + calls: string[][], + result: () => { exitCode: number; stdout: string; stderr: string } = () => ({ + exitCode: 0, + stdout: '', + stderr: '', + }), +): AndroidAdbExecutor { return async (args) => { calls.push(args); - return { exitCode: 0, stdout: '', stderr: '' }; + return result(); }; } diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts index 46067dd462..36d041a505 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { afterEach, beforeEach, test } from 'vitest'; +import { afterEach, beforeEach, test, vi } from 'vitest'; import { captureAndroidSnapshotWithHelperSession } from '../snapshot-helper-session.ts'; import { resetAndroidSnapshotHelperSessions, @@ -84,6 +84,37 @@ test('disables repeated persistent session attempts after startup failure', asyn assert.equal(calls.filter((args) => args[0] === 'forward').length, 2); }); +test('a start that failed for a transient reason is retried instead of ending the persistent path', async () => { + vi.useFakeTimers({ toFake: ['Date'] }); + try { + const calls: string[][] = []; + const spawnArgs: string[][] = []; + const provider = createSessionProvider({ calls, spawnArgs }); + let transportHealthy = false; + const adb: AndroidAdbExecutor = async (args, options) => { + if (args[0] === 'forward' && !transportHealthy) throw new Error('adb server is restarting'); + return await provider.exec!(args, options); + }; + const capture = () => + captureAndroidSnapshotWithHelperSession({ + adb, + adbProvider: { ...provider, exec: adb }, + deviceKey: 'android:emulator-5554', + }); + + assert.equal(await capture(), undefined); + assert.equal(await capture(), undefined); + assert.equal(spawnArgs.length, 0, 'the failed start is not retried inside its cooldown'); + + vi.advanceTimersByTime(61_000); + transportHealthy = true; + assert.match((await capture())?.xml ?? '', /snapshot 1/); + assert.equal(spawnArgs.length, 1); + } finally { + vi.useRealTimers(); + } +}); + test('starts and reuses a persistent Android snapshot helper session', async () => { const calls: string[][] = []; const spawnArgs: string[][] = []; @@ -273,11 +304,11 @@ test('probes the adb transport once per device instead of once per teardown', as assert.equal(calls.some(isHelperRuntimeForceStop), false); }); -test('failed whole-module reset preserves quarantine until recovery is confirmed', async () => { +test('whole-module reset clears a release the last teardown left pending', async () => { const options: SessionProviderOptions = { calls: [], quitResponseMode: 'malformed', - recoveryFailure: true, + runtimeRelease: 'occupied', }; const provider = createSessionProvider(options); const deviceKey = 'android:emulator-5554'; @@ -287,26 +318,22 @@ test('failed whole-module reset preserves quarantine until recovery is confirmed adbProvider: provider, deviceKey, }); - await assert.rejects( - resetAndroidSnapshotHelperSessions(), - /Failed to retire every Android snapshot helper session/, - ); - const forceStopsBeforeRecovery = options.calls.filter((args) => - args.join(' ').includes('am force-stop'), - ).length; + await resetAndroidSnapshotHelperSessions(); + const forceStopsAfterReset = countForceStops(options); - options.recoveryFailure = false; await recoverAndroidSnapshotHelperRetirement({ deviceKey, adb: provider.exec, }); - assert.equal( - options.calls.filter((args) => args.join(' ').includes('am force-stop')).length, - forceStopsBeforeRecovery + 1, - ); + assert.ok(forceStopsAfterReset > 0, 'the teardown stopped the runtime'); + assert.equal(countForceStops(options), forceStopsAfterReset); }); +function countForceStops(options: SessionProviderOptions): number { + return options.calls.filter((args) => isHelperRuntimeForceStop(args)).length; +} + function isHelperRuntimeForceStop(args: string[]): boolean { return args.join(' ') === 'shell am force-stop com.callstack.agentdevice.snapshothelper'; } diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts index cb86e9265c..5b7077906a 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts @@ -54,6 +54,8 @@ export type PersistentSnapshotHelperProviderOptions = { stalledSessionCleanup?: boolean; oneShotAttempts?: string[][]; oneShotXml?: string; + /** Make the device-side stop fail the way an unhealthy transport answers. */ + runtimeStopFailure?: boolean; }; export function createPersistentSnapshotHelperProvider( @@ -153,6 +155,8 @@ export type SessionProviderOptions = { shellProtocolV2?: boolean; /** Make the `adb features` probe fail the way an adb too old to know the command does. */ featureProbeFailure?: boolean; + /** What the device answers when the teardown reads back whether the helper still runs. */ + runtimeRelease?: 'released' | 'occupied' | 'unreadable'; }; export function createSessionProvider(options: SessionProviderOptions): AndroidAdbProvider { @@ -262,6 +266,7 @@ function createSessionExec(options: SessionProviderOptions): AndroidAdbExecutor return async (args, execOptions) => { options.calls.push(args); if (args[0] === 'features') return adbFeaturesResult(options); + if (isAndroidHelperRuntimeProbe(args)) return adbRuntimeProbeResult(options); const forceStopsRuntime = args.join(' ').includes('am force-stop'); await stallSessionCleanupIfConfigured(options, args, execOptions?.signal, forceStopsRuntime); if (options.recoveryFailure && forceStopsRuntime) { @@ -272,6 +277,21 @@ function createSessionExec(options: SessionProviderOptions): AndroidAdbExecutor }; } +function isAndroidHelperRuntimeProbe(args: readonly string[]): boolean { + return args[0] === 'shell' && args[1] === 'pidof'; +} + +function adbRuntimeProbeResult(options: SessionProviderOptions): { + exitCode: number; + stdout: string; + stderr: string; +} { + if (options.runtimeRelease === 'unreadable') throw new Error('device offline'); + return options.runtimeRelease === 'occupied' + ? { exitCode: 0, stdout: '4211\n', stderr: '' } + : { exitCode: 1, stdout: '', stderr: '' }; +} + function adbFeaturesResult(options: SessionProviderOptions): { exitCode: number; stdout: string; @@ -405,9 +425,19 @@ function persistentSnapshotExecResult( if (args[0] === 'features') { return Promise.resolve({ exitCode: 0, stdout: 'cmd\nstat_v2\nshell_v2\n', stderr: '' }); } - if (args[0] === 'forward' || isAndroidHelperRuntimeForceStop(args)) { + if (isAndroidHelperRuntimeForceStop(args)) { + return Promise.resolve( + options.runtimeStopFailure + ? { exitCode: 1, stdout: '', stderr: 'error: device offline' } + : { exitCode: 0, stdout: '', stderr: '' }, + ); + } + if (args[0] === 'forward') { return Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }); } + if (isAndroidHelperRuntimeProbe(args)) { + return Promise.resolve({ exitCode: 1, stdout: '', stderr: '' }); + } if (args.includes('instrument')) { options.oneShotAttempts?.push(args); if (options.oneShotXml) { diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-session.test.ts index 1b9e3fb0e6..002c48860c 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session.test.ts @@ -2,6 +2,7 @@ import assert from 'node:assert/strict'; import { afterEach, beforeEach, test } from 'vitest'; import { captureAndroidSnapshotWithHelperSession } from '../snapshot-helper-session.ts'; import { resetAndroidSnapshotHelperSessions } from '../snapshot-helper-session-lifecycle.ts'; +import { isAndroidSnapshotHelperRuntimeOccupiedError } from '../snapshot-helper-retirement.ts'; import { resolveAndroidSnapshotHelperSessionRequestTimeoutMs } from '../snapshot-helper-session-protocol.ts'; import { createSessionProvider, @@ -124,39 +125,57 @@ test('canceled capture joins canceled external cleanup before returning', async assert.equal(cleanupAborts.length, 2); }); -test('failed capture does not fall back when device runtime retirement is unconfirmed', async () => { +test('release the transport could not confirm falls back instead of failing the capture', async () => { const calls: string[][] = []; const processes: FakeAndroidProcess[] = []; + // An adb that cannot forward the device exit status, and a device that cannot be read back: + // neither says anything about who owns UiAutomation, so neither may answer for this capture. const provider = createSessionProvider({ calls, processes, recoveryFailure: true, responseMode: 'malformed', - stalledCleanup: true, + shellProtocolV2: false, + runtimeRelease: 'unreadable', }); - await assert.rejects( - captureAndroidSnapshotWithHelperSession({ + for (const attempt of [1, 2]) { + const output = await captureAndroidSnapshotWithHelperSession({ adb: provider.exec, adbProvider: provider, deviceKey: 'android:emulator-5554', - }), - (error: unknown) => - (error as { details?: { reason?: string } }).details?.reason === - 'android_snapshot_helper_retirement_unconfirmed', - ); + }); + assert.equal(output, undefined, `attempt ${attempt}`); + } + assert.equal(processes.length, 2); +}); + +test('capture refuses a device the previous teardown found the helper still running', async () => { + const calls: string[][] = []; + const processes: FakeAndroidProcess[] = []; + const provider = createSessionProvider({ + calls, + processes, + responseMode: 'malformed', + shellProtocolV2: false, + runtimeRelease: 'occupied', + }); + + const failed = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider: provider, + deviceKey: 'android:emulator-5554', + }); + assert.equal(failed, undefined); await assert.rejects( captureAndroidSnapshotWithHelperSession({ adb: provider.exec, - adbProvider: { exec: provider.exec }, + adbProvider: provider, deviceKey: 'android:emulator-5554', }), - (error: unknown) => - (error as { details?: { reason?: string } }).details?.reason === - 'android_snapshot_helper_retirement_unconfirmed', + isAndroidSnapshotHelperRuntimeOccupiedError, ); - assert.equal(processes.length, 1); }); test('allows device retirement beyond host-process grace before falling back', async () => { diff --git a/packages/platform-android/src/__tests__/snapshot.test.ts b/packages/platform-android/src/__tests__/snapshot.test.ts index 510e17379e..5a0e5ce28b 100644 --- a/packages/platform-android/src/__tests__/snapshot.test.ts +++ b/packages/platform-android/src/__tests__/snapshot.test.ts @@ -661,29 +661,29 @@ test('snapshotAndroid falls back to one-shot capture after retiring a failed ses ); }); -test('snapshotAndroid does not start one-shot capture when session retirement is unconfirmed', async () => { +test('snapshotAndroid answers from one-shot capture when the session stop could not run', async () => { const adbCalls: string[][] = []; const oneShotAttempts: string[][] = []; + // The stop call failing says the transport is unhealthy, not that UiAutomation is still held. + // ADR 0002 keeps the one-shot transport as the fallback for a session failure either way. const provider = createPersistentSnapshotHelperProvider({ calls: adbCalls, spawnArgs: [], processes: [], sessionResponseMode: 'malformed', - stalledSessionCleanup: true, + runtimeStopFailure: true, oneShotAttempts, - oneShotXml: '', + oneShotXml: '', }); - await assert.rejects( - snapshotAndroid(device, { - helperAdb: provider, - helperArtifact, - helperSessionScope: 'daemon-session', - }), - /could not confirm release of device automation ownership/, - ); + const result = await snapshotAndroid(device, { + helperAdb: provider, + helperArtifact, + helperSessionScope: 'daemon-session', + }); - assert.equal(oneShotAttempts.length, 0); + assert.equal(result.nodes[0]?.label, 'one-shot fallback'); + assert.equal(oneShotAttempts.length, 1); }); test('snapshotAndroid fails closed when the helper fails', async () => { diff --git a/packages/platform-android/src/__tests__/touch-helper-session.test.ts b/packages/platform-android/src/__tests__/touch-helper-session.test.ts index 29b71f2485..5e06ad3a7f 100644 --- a/packages/platform-android/src/__tests__/touch-helper-session.test.ts +++ b/packages/platform-android/src/__tests__/touch-helper-session.test.ts @@ -20,7 +20,10 @@ import { } from '../adb-executor.ts'; import { captureAndroidSnapshotWithHelperSession } from '../snapshot-helper-session.ts'; import { resetAndroidSnapshotHelperSessions } from '../snapshot-helper-session-lifecycle.ts'; -import { getAndroidSnapshotHelperSessionDeviceKey } from '../snapshot-helper-retirement.ts'; +import { + getAndroidSnapshotHelperSessionDeviceKey, + isAndroidSnapshotHelperRuntimeOccupiedError, +} from '../snapshot-helper-retirement.ts'; import { lowerAndroidTouchPlan } from '../touch-plan-lowering.ts'; import { executeAndroidTouchHelperPlan, @@ -108,24 +111,48 @@ function snapshotSessionResponse(requestId: string): string { .join('\n')}\n\n${body}`; } +function isTouchCleanupCommand(args: string[]): boolean { + return ( + (args[0] === 'forward' && args[1] === '--remove') || + (args[0] === 'shell' && args[1] === 'am' && args[2] === 'force-stop') + ); +} + +function neverResolvingAfterAbort(signal: AbortSignal): Promise { + return new Promise((_resolve, reject) => { + const onAbort = () => reject(signal.reason); + signal.addEventListener('abort', onAbort, { once: true }); + if (signal.aborted) onAbort(); + }); +} + +function readRuntimePidProbeResult( + args: string[], + runtimePid: string | undefined, +): { exitCode: number; stdout: string; stderr: string } | undefined { + if (args[0] !== 'shell' || args[1] !== 'pidof') return undefined; + return runtimePid + ? { exitCode: 0, stdout: `${runtimePid}\n`, stderr: '' } + : { exitCode: 1, stdout: '', stderr: '' }; +} + function createFakeTouchHelperSessionProvider( handleCommand: TouchSessionCommandHandler, - options: { stallCleanup?: boolean } = {}, + options: { stallCleanup?: boolean; runtimePid?: string } = {}, ): AndroidAdbProvider { return { exec: async (args, execOptions) => { - const cleanupCommand = - (args[0] === 'forward' && args[1] === '--remove') || - (args[0] === 'shell' && args[1] === 'am' && args[2] === 'force-stop'); const signal = execOptions?.signal; - if (options.stallCleanup && cleanupCommand && signal) { - return await new Promise((_resolve, reject) => { - const onAbort = () => reject(signal.reason); - signal.addEventListener('abort', onAbort, { once: true }); - if (signal.aborted) onAbort(); - }); + if (options.stallCleanup && signal && isTouchCleanupCommand(args)) { + return await neverResolvingAfterAbort(signal); } - return { exitCode: 0, stdout: '', stderr: '' }; + return ( + readRuntimePidProbeResult(args, options.runtimePid) ?? { + exitCode: 0, + stdout: '', + stderr: '', + } + ); }, spawn: (args) => { const port = readSessionPort(args); @@ -166,23 +193,21 @@ function createFakeTouchHelperSessionProvider( }; } -test('touch helper does not run one-shot while snapshot retirement is unconfirmed', async () => { +test('touch helper does not run one-shot while the device still runs the helper', async () => { const device = makeIsolatedDevice(); const deviceKey = getAndroidSnapshotHelperSessionDeviceKey(device); const provider = createFakeTouchHelperSessionProvider(() => 'malformed snapshot response', { - stallCleanup: true, + runtimePid: '4211', }); - await assert.rejects( - captureAndroidSnapshotWithHelperSession({ - adb: provider.exec, - adbProvider: provider, - deviceKey, - }), - (error: unknown) => - (error as { details?: { reason?: string } }).details?.reason === - 'android_snapshot_helper_retirement_unconfirmed', - ); + // A malformed response retires the session; the device then answers that the helper process is + // still running, which is the only fact that may hold the next command. + const failed = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider: provider, + deviceKey, + }); + assert.equal(failed, undefined); let oneShotCalled = false; await assert.rejects( @@ -192,6 +217,9 @@ test('touch helper does not run one-shot while snapshot retirement is unconfirme if (args[0] === 'shell' && args[1] === 'am' && args[2] === 'force-stop') { return { exitCode: 1, stdout: '', stderr: 'runtime still busy' }; } + if (args[0] === 'shell' && args[1] === 'pidof') { + return { exitCode: 0, stdout: '4211\n', stderr: '' }; + } if (args.includes('instrument')) oneShotCalled = true; return { exitCode: 0, stdout: '', stderr: '' }; }), @@ -199,9 +227,7 @@ test('touch helper does not run one-shot while snapshot retirement is unconfirme { serial: device.id }, async () => await executeAndroidTouchHelperPlan(device, lowerAndroidTouchPlan(flingPlan())), ), - (error: unknown) => - (error as { details?: { reason?: string } }).details?.reason === - 'android_snapshot_helper_retirement_unconfirmed', + isAndroidSnapshotHelperRuntimeOccupiedError, ); assert.equal(oneShotCalled, false); diff --git a/packages/platform-android/src/mechanics.ts b/packages/platform-android/src/mechanics.ts index 87ec15971b..b026b14094 100644 --- a/packages/platform-android/src/mechanics.ts +++ b/packages/platform-android/src/mechanics.ts @@ -268,7 +268,7 @@ export { ensureAndroidSnapshotHelper, forgetAndroidSnapshotHelperInstall, getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRetirementUnconfirmedError, + isAndroidSnapshotHelperRuntimeOccupiedError, parseAndroidSnapshotHelperManifest as parseAndroidHelperManifest, resetAndroidSnapshotHelperSessions, stopAndroidSnapshotHelperSession, diff --git a/packages/platform-android/src/snapshot-helper-retirement.ts b/packages/platform-android/src/snapshot-helper-retirement.ts index 6d3ef1f39b..cf04c3ed62 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -1,5 +1,6 @@ import { AppError } from '@agent-device/kernel/errors'; import type { DeviceInfo } from '@agent-device/kernel/device'; +import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; import type { AndroidAdbProcess } from './adb-executor.ts'; import type { AndroidAdbExecutor } from './snapshot-helper-types.ts'; @@ -9,14 +10,22 @@ const RETIREMENT_RECOVERY_TIMEOUT_MS = 5_000; // time out before Android could confirm UiAutomation release. export const ANDROID_SNAPSHOT_HELPER_HOST_PROCESS_EXIT_GRACE_MS = 250; export const ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS = 2_000; -const RETIREMENT_UNCONFIRMED_REASON = 'android_snapshot_helper_retirement_unconfirmed'; +const RUNTIME_OCCUPIED_REASON = 'android_snapshot_helper_runtime_occupied'; -type UnconfirmedRetirement = { +/** + * Whether anything on the device still owns UiAutomation through the helper runtime. `unknown` is + * reserved for a device that could not be read, never for one that answered slowly or whose + * `am force-stop` call failed: those say nothing about ownership. + */ +export type AndroidSnapshotHelperRuntimeRelease = 'released' | 'occupied' | 'unknown'; + +/** A retirement whose release the last teardown could not prove; recovered at the next acquire. */ +type PendingRetirement = { packageName: string; cause: string; }; -const unconfirmedRetirements = new Map(); +const pendingRetirements = new Map(); export function getAndroidSnapshotHelperSessionDeviceKey( device: Pick, @@ -24,74 +33,145 @@ export function getAndroidSnapshotHelperSessionDeviceKey( return `${device.platform}:${device.id}`; } +/** + * Stops the runtime a canceled one-shot capture left behind and records what could not be proven. + * The caller's own outcome — usually a cancellation — is what the caller reports; ownership is a + * device fact recovered before the next command starts work, not a reason to fail this one. + */ export async function retireCanceledAndroidSnapshotHelperCapture(params: { deviceKey: string; packageName: string; adb: AndroidAdbExecutor; cause: unknown; }): Promise { - const runtimeForceStopped = await forceStopAndroidSnapshotHelperRuntime({ + await forceStopAndroidSnapshotHelperRuntime({ adb: params.adb, packageName: params.packageName, timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, }); - if (!runtimeForceStopped) { - quarantineAndroidSnapshotHelperRetirement(params); - } + await recordAndroidSnapshotHelperRelease({ + deviceKey: params.deviceKey, + packageName: params.packageName, + adb: params.adb, + cause: params.cause, + }); } +/** + * Clears the pending release a previous teardown could not prove, before the next command acquires + * the device. Only a device that says a helper process is still running may block a command here: + * an `adb` call that failed or ran out of budget is recorded and the command proceeds, because the + * transport is exactly what such a call measures badly. + */ export async function recoverAndroidSnapshotHelperRetirement(params: { deviceKey: string; adb: AndroidAdbExecutor; signal?: AbortSignal; }): Promise { - const retirement = unconfirmedRetirements.get(params.deviceKey); + const retirement = pendingRetirements.get(params.deviceKey); if (!retirement) return; - try { - const result = await params.adb(['shell', 'am', 'force-stop', retirement.packageName], { - allowFailure: true, - timeoutMs: RETIREMENT_RECOVERY_TIMEOUT_MS, - signal: params.signal, - }); - params.signal?.throwIfAborted(); - if (result.exitCode === 0) { - unconfirmedRetirements.delete(params.deviceKey); - return; - } - } catch { - params.signal?.throwIfAborted(); - } - quarantineAndroidSnapshotHelperRetirement({ - deviceKey: params.deviceKey, + await forceStopAndroidSnapshotHelperRuntime({ + adb: params.adb, + packageName: retirement.packageName, + timeoutMs: RETIREMENT_RECOVERY_TIMEOUT_MS, + ...(params.signal ? { signal: params.signal } : {}), + }); + params.signal?.throwIfAborted(); + const release = await readAndroidSnapshotHelperRuntimeRelease({ + adb: params.adb, packageName: retirement.packageName, - cause: retirement.cause, }); + if (release === 'occupied') { + throw createAndroidSnapshotHelperRuntimeOccupiedError({ + deviceKey: params.deviceKey, + packageName: retirement.packageName, + cause: retirement.cause, + }); + } + // A device that could not be read leaves the retirement pending: the next acquire asks again, and + // this command answers for its own transport instead of for ownership. + if (release === 'unknown') return; + pendingRetirements.delete(params.deviceKey); } -export function quarantineAndroidSnapshotHelperRetirement(params: { +/** + * Records what one teardown proved about device automation ownership: nothing a command reports + * through. A release the device confirmed clears the pending retirement; anything else keeps it for + * the next acquire, which is where a command may be refused for it. + */ +export async function recordAndroidSnapshotHelperRelease(params: { deviceKey: string; packageName: string; + adb: AndroidAdbExecutor; cause: unknown; -}): never { + /** Release the caller already proved, for example by an acknowledged and clean helper quit. */ + release?: AndroidSnapshotHelperRuntimeRelease; +}): Promise { + const release = + params.release ?? + (await readAndroidSnapshotHelperRuntimeRelease({ + adb: params.adb, + packageName: params.packageName, + })); + if (release === 'released') { + pendingRetirements.delete(params.deviceKey); + return release; + } const causeMessage = params.cause instanceof Error ? params.cause.message : String(params.cause); - unconfirmedRetirements.set(params.deviceKey, { + pendingRetirements.set(params.deviceKey, { packageName: params.packageName, cause: causeMessage, }); - throw new AppError( + emitDiagnostic({ + level: 'warn', + phase: 'android_snapshot_helper_retirement_pending', + data: { deviceKey: params.deviceKey, packageName: params.packageName, release }, + }); + return release; +} + +export function isAndroidSnapshotHelperRuntimeOccupiedError(error: unknown): boolean { + return error instanceof AppError && error.details?.reason === RUNTIME_OCCUPIED_REASON; +} + +function createAndroidSnapshotHelperRuntimeOccupiedError(params: { + deviceKey: string; + packageName: string; + cause: unknown; +}): AppError { + const causeMessage = params.cause instanceof Error ? params.cause.message : String(params.cause); + return new AppError( 'COMMAND_FAILED', - 'Android snapshot helper could not confirm release of device automation ownership', + 'Android automation helper is still holding device automation ownership', { - reason: RETIREMENT_UNCONFIRMED_REASON, + reason: RUNTIME_OCCUPIED_REASON, deviceKey: params.deviceKey, + packageName: params.packageName, cause: causeMessage, - hint: 'Retry after the helper process exits, or restart the device if Android still reports automation as busy.', + hint: 'Retry after the helper process exits, or restart the device if Android keeps reporting the helper as running.', }, ); } -export function isAndroidSnapshotHelperRetirementUnconfirmedError(error: unknown): boolean { - return error instanceof AppError && error.details?.reason === RETIREMENT_UNCONFIRMED_REASON; +/** + * Asks the device who owns UiAutomation. Android drops the connection with the process that opened + * it, so a helper package with no process cannot be holding the device. + */ +async function readAndroidSnapshotHelperRuntimeRelease(params: { + adb: AndroidAdbExecutor; + packageName: string; +}): Promise { + try { + const result = await params.adb(['shell', 'pidof', params.packageName], { + allowFailure: true, + timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, + }); + // `pidof` exits non-zero and prints nothing when no process matches. + if (result.exitCode !== 0 || !/\d/.test(result.stdout)) return 'released'; + return 'occupied'; + } catch { + return 'unknown'; + } } export async function settleAndroidSnapshotHelperSessionCleanup(params: { @@ -108,25 +188,22 @@ export async function settleAndroidSnapshotHelperSessionCleanup(params: { * paths that distrust the helper's output require the stop regardless of that evidence. */ forceStopRuntime: boolean; -}): Promise<{ timedOut: boolean; runtimeForceStopped: boolean }> { +}): Promise<{ timedOut: boolean }> { const signal = AbortSignal.timeout(params.timeoutMs); - const results = await Promise.allSettled([ - params.forceStopRuntime - ? forceStopAndroidSnapshotHelperRuntime({ - adb: params.adb, - packageName: params.packageName, - timeoutMs: params.timeoutMs, - signal, - }) - : Promise.resolve(false), + await Promise.all([ + ...(params.forceStopRuntime + ? [ + forceStopAndroidSnapshotHelperRuntime({ + adb: params.adb, + packageName: params.packageName, + timeoutMs: params.timeoutMs, + signal, + }), + ] + : []), removeAndroidSnapshotHelperSessionForward({ ...params, signal }), - ] as const); - const [runtimeStopResult] = results; - return { - timedOut: signal.aborted, - runtimeForceStopped: - runtimeStopResult?.status === 'fulfilled' ? runtimeStopResult.value : false, - }; + ]); + return { timedOut: signal.aborted }; } /** @@ -211,26 +288,26 @@ export async function stopAndroidSnapshotHelperHostProcess(params: { } export function resetAndroidSnapshotHelperRetirements(): void { - unconfirmedRetirements.clear(); + pendingRetirements.clear(); } +/** + * Best-effort device-side stop. Its outcome is never release evidence: whoever needs that reads the + * device with `readAndroidSnapshotHelperRuntimeRelease`. + */ async function forceStopAndroidSnapshotHelperRuntime(params: { adb: AndroidAdbExecutor; packageName: string; timeoutMs: number; signal?: AbortSignal; -}): Promise { - const signal = params.signal ?? AbortSignal.timeout(params.timeoutMs); - try { - const result = await params.adb(['shell', 'am', 'force-stop', params.packageName], { +}): Promise { + await params + .adb(['shell', 'am', 'force-stop', params.packageName], { allowFailure: true, timeoutMs: params.timeoutMs, - signal, - }); - return result.exitCode === 0; - } catch { - return false; - } + ...(params.signal ? { signal: params.signal } : {}), + }) + .catch(() => {}); } async function removeAndroidSnapshotHelperSessionForward(params: { diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index 0dd4fb62df..caa125923b 100644 --- a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts +++ b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts @@ -27,6 +27,7 @@ import { import { allocateAndroidSnapshotHelperSessionPort, isAndroidSnapshotHelperSessionCommandAcknowledged, + provesAndroidSnapshotHelperSessionUnavailable, sendAndroidSnapshotHelperSessionCommand, waitForAndroidSnapshotHelperSessionReady, } from './snapshot-helper-session-protocol.ts'; @@ -35,10 +36,9 @@ import { ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, ANDROID_SNAPSHOT_HELPER_HOST_PROCESS_EXIT_GRACE_MS, getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRetirementUnconfirmedError, observeAndroidSnapshotHelperProcessExit, - quarantineAndroidSnapshotHelperRetirement, recoverAndroidSnapshotHelperRetirement, + recordAndroidSnapshotHelperRelease, resetAndroidSnapshotHelperRetirements, settleAndroidSnapshotHelperSessionCleanup, stopAndroidSnapshotHelperHostProcess, @@ -46,6 +46,8 @@ import { } from './snapshot-helper-retirement.ts'; const SESSION_READY_TIMEOUT_MS = 10_000; +// How long a device that merely started too slowly stays excluded from the persistent path. +const SESSION_START_RETRY_AFTER_MS = 60_000; const SESSION_STOP_TIMEOUT_MS = 1_000; // SnapshotInstrumentation.finishSafely can spend up to 10 seconds waiting for Android to finish // connecting UiAutomation. Let an acknowledged quit complete that release before force-killing adb. @@ -84,7 +86,18 @@ export type AndroidSnapshotHelperSessionAcquisition = { }; const sessions = new Map(); -const disabledSessionIdentities = new Map(); + +type DisabledAndroidSnapshotHelperSession = { + identity: string; + /** + * When set, the start failed for a reason that says nothing about this identity — the device was + * slow, the transport was slow — so a later command tries again instead of paying one-shot + * instrumentation for the rest of the daemon's life. + */ + retryAfterMs?: number; +}; + +const disabledSessionIdentities = new Map(); /** * Starts (or reuses) the session without capturing, so a helper-backed read that is not a snapshot @@ -140,42 +153,73 @@ async function resolveAndroidSnapshotHelperSession(params: { resolved: AndroidSnapshotHelperResolvedCaptureOptions; }): Promise { const { deviceKey, identity, options, resolved } = params; - if (disabledSessionIdentities.get(deviceKey) === identity) { - return undefined; - } let session = sessions.get(deviceKey); if (session && session.identity !== identity) { await stopAndroidSnapshotHelperSession(deviceKey); session = undefined; } - if (!session) { - try { - session = await startAndroidSnapshotHelperSession({ - deviceKey, - identity, - options, - resolved, - }); - } catch (error) { - options.signal?.throwIfAborted(); - disabledSessionIdentities.set(deviceKey, identity); - emitDiagnostic({ - level: 'warn', - phase: 'android_snapshot_helper_session_disabled', - data: { - deviceKey, - reason: error instanceof Error ? error.message : String(error), - }, - }); - if (isAndroidSnapshotHelperRetirementUnconfirmedError(error)) { - throw error; - } - return undefined; - } + if (!session && !isAndroidSnapshotHelperSessionIdentityDisabled(deviceKey, identity)) { + session = await startAndroidSnapshotHelperSessionOrDisable({ + deviceKey, + identity, + options, + resolved, + }); } return session; } +/** + * A start that failed is not a command that failed: the caller answers with the one-shot transport. + * What it does decide is how soon this identity is worth starting again. + */ +async function startAndroidSnapshotHelperSessionOrDisable(params: { + deviceKey: string; + identity: string; + options: AndroidSnapshotHelperCaptureOptions; + resolved: AndroidSnapshotHelperResolvedCaptureOptions; +}): Promise { + try { + return await startAndroidSnapshotHelperSession(params); + } catch (error) { + params.options.signal?.throwIfAborted(); + disableAndroidSnapshotHelperSessionIdentity(params.deviceKey, params.identity, error); + return undefined; + } +} + +function disableAndroidSnapshotHelperSessionIdentity( + deviceKey: string, + identity: string, + error: unknown, +): void { + // Only a helper that ran and exited before announcing readiness proves this identity unusable. A + // start that ran out of time or lost its transport says nothing, so the exclusion expires. + const unavailable = provesAndroidSnapshotHelperSessionUnavailable(error); + disabledSessionIdentities.set(deviceKey, { + identity, + ...(unavailable ? {} : { retryAfterMs: Date.now() + SESSION_START_RETRY_AFTER_MS }), + }); + emitDiagnostic({ + level: 'warn', + phase: 'android_snapshot_helper_session_disabled', + data: { + deviceKey, + reason: error instanceof Error ? error.message : String(error), + ...(unavailable ? {} : { retryAfterMs: SESSION_START_RETRY_AFTER_MS }), + }, + }); +} + +function isAndroidSnapshotHelperSessionIdentityDisabled(deviceKey: string, identity: string) { + const disabled = disabledSessionIdentities.get(deviceKey); + if (!disabled || disabled.identity !== identity) return false; + if (disabled.retryAfterMs === undefined) return true; + if (Date.now() < disabled.retryAfterMs) return true; + disabledSessionIdentities.delete(deviceKey); + return false; +} + async function startAndroidSnapshotHelperSession(params: { deviceKey: string; identity: string; @@ -225,6 +269,7 @@ async function startAndroidSnapshotHelperSession(params: { params.options.signal, ); sessions.set(params.deviceKey, session); + disabledSessionIdentities.delete(params.deviceKey); emitDiagnostic({ phase: 'android_snapshot_helper_session_ready', data: { @@ -242,7 +287,7 @@ async function startAndroidSnapshotHelperSession(params: { } catch { // Best effort after startup failure. } - const [, cleanup] = await Promise.all([ + await Promise.all([ waitForAndroidSnapshotHelperProcessExit( processExit.ended, ANDROID_SNAPSHOT_HELPER_HOST_PROCESS_EXIT_GRACE_MS, @@ -258,13 +303,14 @@ async function startAndroidSnapshotHelperSession(params: { forceStopRuntime: true, }), ]); - if (!cleanup.runtimeForceStopped) { - quarantineAndroidSnapshotHelperRetirement({ - deviceKey: params.deviceKey, - packageName: session.helper.packageName, - cause: error, - }); - } + // What this command reports is the failed start, which the caller answers with the one-shot + // transport. Whether the device is still owned is a fact the next acquire reads. + await recordAndroidSnapshotHelperRelease({ + deviceKey: params.deviceKey, + packageName: session.helper.packageName, + adb: session.adb, + cause: error, + }); throw error; } } @@ -335,7 +381,7 @@ export async function stopAndroidSnapshotHelperSession( // exit status adb forwarded from the device from one adb invented for a closed connection. // Anything less is not evidence, and the device-side stop runs. const deviceExitObserved = graceful.acknowledged && graceful.exited; - const runtimeReleaseConfirmed = + const releaseProvenByQuit = deviceExitObserved && (await androidAdbForwardsDeviceExitStatus({ adb: session.adb, @@ -362,9 +408,18 @@ export async function stopAndroidSnapshotHelperSession( port: session.port, packageName: session.helper.packageName, timeoutMs: cleanupTimeoutMs, - forceStopRuntime: options.resetRuntime === true || !runtimeReleaseConfirmed, + forceStopRuntime: options.resetRuntime === true || !releaseProvenByQuit, }), ]); + // Teardown never decides what the command reports: what the device said about ownership is + // recorded for the next acquire, and a command that already answered stays answered. + const release = await recordAndroidSnapshotHelperRelease({ + deviceKey, + packageName: session.helper.packageName, + adb: session.adb, + cause: options.cause, + ...(releaseProvenByQuit ? { release: 'released' as const } : {}), + }); emitDiagnostic({ phase: 'android_snapshot_helper_session_stop', data: { @@ -373,24 +428,15 @@ export async function stopAndroidSnapshotHelperSession( capturedCount: session.capturedCount, lifetimeMs: Date.now() - session.startedAtMs, quitAcknowledged: graceful.acknowledged, - // With the exit observed but the release unconfirmed, the transport is what failed to prove it. + // With the exit observed but the release unproven, the transport is what failed to prove it. quitExitObserved: deviceExitObserved, - runtimeReleaseConfirmed, + releaseProvenByQuit, + release, forceKilled: !hostProcessEnded && processStopped, forced: force || options.signal?.aborted === true, - runtimeForceStopped: cleanup.runtimeForceStopped, externalCleanupTimedOut: cleanup.timedOut, }, }); - // Either the release was proven or the device-side stop confirmed it. An unproven quit whose - // stop also failed leaves ownership unknown, which is what quarantine exists to report. - if (!runtimeReleaseConfirmed && !cleanup.runtimeForceStopped) { - quarantineAndroidSnapshotHelperRetirement({ - deviceKey, - packageName: session.helper.packageName, - cause: options.cause, - }); - } return true; } @@ -435,16 +481,12 @@ export async function stopAndroidSnapshotHelperSessionForDevice( } export async function resetAndroidSnapshotHelperSessions(): Promise { - const retirements = await Promise.allSettled( - [...sessions.keys()].map((deviceKey) => stopAndroidSnapshotHelperSession(deviceKey)), + await Promise.all( + [...sessions.keys()].map(async (deviceKey) => { + await stopAndroidSnapshotHelperSession(deviceKey); + }), ); disabledSessionIdentities.clear(); - const failures = retirements - .filter((result): result is PromiseRejectedResult => result.status === 'rejected') - .map((result) => result.reason); - if (failures.length > 0) { - throw new AggregateError(failures, 'Failed to retire every Android snapshot helper session'); - } resetAndroidSnapshotHelperRetirements(); resetAndroidAdbShellProtocolProbes(); } diff --git a/packages/platform-android/src/snapshot-helper-session-protocol.ts b/packages/platform-android/src/snapshot-helper-session-protocol.ts index 8d2d9ce36b..489120e88f 100644 --- a/packages/platform-android/src/snapshot-helper-session-protocol.ts +++ b/packages/platform-android/src/snapshot-helper-session-protocol.ts @@ -108,6 +108,18 @@ export function resolveAndroidSnapshotHelperSessionRequestTimeoutMs(params: { ); } +const SESSION_READY_TIMEOUT_REASON = 'android_snapshot_helper_session_ready_timeout'; +const SESSION_EXITED_BEFORE_READY_REASON = 'android_snapshot_helper_session_exited_before_ready'; + +/** + * Whether a failed start proves this helper identity cannot serve this device at all. A helper that + * ran and ended before announcing readiness says so; a start that only ran out of time or lost its + * transport says nothing about the identity, and must not exclude it for the daemon's whole life. + */ +export function provesAndroidSnapshotHelperSessionUnavailable(error: unknown): boolean { + return error instanceof AppError && error.details?.reason === SESSION_EXITED_BEFORE_READY_REASON; +} + export function waitForAndroidSnapshotHelperSessionReady( childProcess: AndroidAdbProcess, timeoutMs: number, @@ -121,6 +133,7 @@ export function waitForAndroidSnapshotHelperSessionReady( new AppError('COMMAND_FAILED', 'Android snapshot helper session did not become ready', { output, timeoutMs, + reason: SESSION_READY_TIMEOUT_REASON, }), ); }, timeoutMs); @@ -154,6 +167,7 @@ export function waitForAndroidSnapshotHelperSessionReady( output, exitCode: code, signal: exitSignal, + reason: SESSION_EXITED_BEFORE_READY_REASON, }), ); }); diff --git a/packages/platform-android/src/snapshot-helper.ts b/packages/platform-android/src/snapshot-helper.ts index b7e365914e..5eed360b76 100644 --- a/packages/platform-android/src/snapshot-helper.ts +++ b/packages/platform-android/src/snapshot-helper.ts @@ -8,7 +8,7 @@ export { } from './snapshot-helper-session-lifecycle.ts'; export { getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRetirementUnconfirmedError, + isAndroidSnapshotHelperRuntimeOccupiedError, } from './snapshot-helper-retirement.ts'; export { ensureAndroidSnapshotHelper, diff --git a/packages/platform-android/src/snapshot.ts b/packages/platform-android/src/snapshot.ts index 2e73c36844..d0c21c52f5 100644 --- a/packages/platform-android/src/snapshot.ts +++ b/packages/platform-android/src/snapshot.ts @@ -37,7 +37,7 @@ import { ensureAndroidSnapshotHelper, forgetAndroidSnapshotHelperInstall, getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRetirementUnconfirmedError, + isAndroidSnapshotHelperRuntimeOccupiedError, stopAndroidSnapshotHelperSession, type AndroidAdbExecutor, type AndroidSnapshotHelperArtifact, @@ -377,7 +377,7 @@ async function captureAndroidUiHierarchyFromHelper(params: { if (sessionCapture) return sessionCapture; } catch (error) { signal?.throwIfAborted(); - if (isAndroidSnapshotHelperRetirementUnconfirmedError(error)) { + if (isAndroidSnapshotHelperRuntimeOccupiedError(error)) { throw error; } emitDiagnostic({ From b93eb847e4889e0bc88e267fcdbba1f5799674c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 19:31:13 +0200 Subject: [PATCH 02/12] refactor(android): share the runtime stop and the device's process answer resetAndroidSnapshotHelperRuntime carried a second best-effort `am force-stop` with the same 2s budget the retirement path already owned, and two session fakes answered `pidof` between them. One stop serves both callers, one fake answers the probe for every test that steers it, and the release read now looks for a pid token rather than any digit. --- .../__tests__/snapshot-helper-capture.test.ts | 14 ++++---- .../snapshot-helper-retirement.test.ts | 12 ++++--- .../snapshot-helper-session.fixtures.ts | 32 ++++++++++++------- .../__tests__/touch-helper-session.test.ts | 30 +++++++---------- .../src/snapshot-helper-retirement.ts | 18 ++++------- .../src/snapshot-helper-runtime.ts | 28 +++++----------- 6 files changed, 61 insertions(+), 73 deletions(-) diff --git a/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts index 000b940eb6..1c6bccb8b0 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts @@ -3,6 +3,10 @@ import { beforeEach, test } from 'vitest'; import { captureAndroidSnapshotWithHelper } from '../snapshot-helper-capture.ts'; import { resetAndroidSnapshotHelperRetirements } from '../snapshot-helper-retirement.ts'; import type { AndroidAdbExecutor } from '../snapshot-helper-types.ts'; +import { + androidHelperRuntimeProbeResult, + isAndroidHelperRuntimeProbe, +} from './snapshot-helper-session.fixtures.ts'; beforeEach(() => { resetAndroidSnapshotHelperRetirements(); @@ -31,9 +35,7 @@ test('one-shot capture that resolves during cancellation retires before rejectin if (options?.signal?.aborted) onAbort(); }); } - if (args.join(' ').includes('pidof')) { - return { exitCode: 1, stdout: '', stderr: '' }; - } + if (isAndroidHelperRuntimeProbe(args)) return androidHelperRuntimeProbeResult(); assert.deepEqual(args, [ 'shell', 'am', @@ -84,12 +86,10 @@ test('canceled one-shot capture reports the cancellation and the next capture re events.push(`force-stop-${stopCount}`); return { exitCode: 0, stdout: '', stderr: '' }; } - if (args.join(' ').includes('pidof')) { + if (isAndroidHelperRuntimeProbe(args)) { // The first read happens while Android still runs the helper; the next says it is gone. events.push('pidof'); - return stopCount === 1 - ? { exitCode: 0, stdout: '4211\n', stderr: '' } - : { exitCode: 1, stdout: '', stderr: '' }; + return androidHelperRuntimeProbeResult(stopCount === 1 ? 'occupied' : 'released'); } events.push(`instrument-${stopCount}`); if (stopCount === 0) { diff --git a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts index 9d5702e68e..c8e9dc2bed 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts @@ -12,6 +12,10 @@ import { } from '../snapshot-helper-retirement.ts'; import type { AndroidAdbProcess } from '../adb-executor.ts'; import type { AndroidAdbExecutor } from '../snapshot-helper-types.ts'; +import { + androidHelperRuntimeProbeResult, + isAndroidHelperRuntimeProbe, +} from './snapshot-helper-session.fixtures.ts'; const PACKAGE_NAME = 'com.callstack.agentdevice.snapshothelper'; const DEVICE_KEY = 'android:emulator-5554'; @@ -48,10 +52,8 @@ test('canceled capture answers for the device, not for the force-stop call that test('unproven release stays pending until an acquire reads the device', async () => { let helperAlive = true; const adb: AndroidAdbExecutor = async (args) => { - if (args.includes('pidof')) { - return helperAlive - ? { exitCode: 0, stdout: '4211\n', stderr: '' } - : { exitCode: 1, stdout: '', stderr: '' }; + if (isAndroidHelperRuntimeProbe(args)) { + return androidHelperRuntimeProbeResult(helperAlive ? 'occupied' : 'released'); } return { exitCode: 0, stdout: '', stderr: '' }; }; @@ -75,7 +77,7 @@ test('unproven release stays pending until an acquire reads the device', async ( test('a device that cannot be read leaves the retirement pending without failing the command', async () => { const adb: AndroidAdbExecutor = async (args) => { - if (args.includes('pidof')) throw new Error('device offline'); + if (isAndroidHelperRuntimeProbe(args)) return androidHelperRuntimeProbeResult('unreadable'); return { exitCode: 0, stdout: '', stderr: '' }; }; diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts index 5b7077906a..355a15b334 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts @@ -11,7 +11,12 @@ import assert from 'node:assert/strict'; import { EventEmitter } from 'node:events'; import net from 'node:net'; import { PassThrough } from 'node:stream'; -import type { AndroidAdbProcess, AndroidAdbProvider } from '../adb-executor.ts'; +import type { + AndroidAdbExecutorResult, + AndroidAdbProcess, + AndroidAdbProvider, +} from '../adb-executor.ts'; +import type { AndroidSnapshotHelperRuntimeRelease } from '../snapshot-helper-retirement.ts'; import type { AndroidAdbExecutor } from '../snapshot-helper-types.ts'; import { bindAndroidAdbTestHost } from './test-utils/android-host-test-setup.ts'; @@ -156,9 +161,14 @@ export type SessionProviderOptions = { /** Make the `adb features` probe fail the way an adb too old to know the command does. */ featureProbeFailure?: boolean; /** What the device answers when the teardown reads back whether the helper still runs. */ - runtimeRelease?: 'released' | 'occupied' | 'unreadable'; + runtimeRelease?: FakeAndroidHelperRuntimeRelease; }; +/** What a fake device says about the helper process, including a device that cannot be read. */ +export type FakeAndroidHelperRuntimeRelease = + | Exclude + | 'unreadable'; + export function createSessionProvider(options: SessionProviderOptions): AndroidAdbProvider { bindAndroidAdbTestHost(); let stalledSnapshots = options.stalledSnapshots ?? 0; @@ -266,7 +276,8 @@ function createSessionExec(options: SessionProviderOptions): AndroidAdbExecutor return async (args, execOptions) => { options.calls.push(args); if (args[0] === 'features') return adbFeaturesResult(options); - if (isAndroidHelperRuntimeProbe(args)) return adbRuntimeProbeResult(options); + if (isAndroidHelperRuntimeProbe(args)) + return androidHelperRuntimeProbeResult(options.runtimeRelease); const forceStopsRuntime = args.join(' ').includes('am force-stop'); await stallSessionCleanupIfConfigured(options, args, execOptions?.signal, forceStopsRuntime); if (options.recoveryFailure && forceStopsRuntime) { @@ -277,17 +288,16 @@ function createSessionExec(options: SessionProviderOptions): AndroidAdbExecutor }; } -function isAndroidHelperRuntimeProbe(args: readonly string[]): boolean { +/** Whether an adb call is the helper-process read that decides device automation ownership. */ +export function isAndroidHelperRuntimeProbe(args: readonly string[]): boolean { return args[0] === 'shell' && args[1] === 'pidof'; } -function adbRuntimeProbeResult(options: SessionProviderOptions): { - exitCode: number; - stdout: string; - stderr: string; -} { - if (options.runtimeRelease === 'unreadable') throw new Error('device offline'); - return options.runtimeRelease === 'occupied' +export function androidHelperRuntimeProbeResult( + release: FakeAndroidHelperRuntimeRelease = 'released', +): AndroidAdbExecutorResult { + if (release === 'unreadable') throw new Error('device offline'); + return release === 'occupied' ? { exitCode: 0, stdout: '4211\n', stderr: '' } : { exitCode: 1, stdout: '', stderr: '' }; } diff --git a/packages/platform-android/src/__tests__/touch-helper-session.test.ts b/packages/platform-android/src/__tests__/touch-helper-session.test.ts index 5e06ad3a7f..1cac2cd246 100644 --- a/packages/platform-android/src/__tests__/touch-helper-session.test.ts +++ b/packages/platform-android/src/__tests__/touch-helper-session.test.ts @@ -18,6 +18,11 @@ import { type AndroidAdbProcess, type AndroidAdbProvider, } from '../adb-executor.ts'; +import { + androidHelperRuntimeProbeResult, + isAndroidHelperRuntimeProbe, + type FakeAndroidHelperRuntimeRelease, +} from './snapshot-helper-session.fixtures.ts'; import { captureAndroidSnapshotWithHelperSession } from '../snapshot-helper-session.ts'; import { resetAndroidSnapshotHelperSessions } from '../snapshot-helper-session-lifecycle.ts'; import { @@ -126,19 +131,9 @@ function neverResolvingAfterAbort(signal: AbortSignal): Promise { }); } -function readRuntimePidProbeResult( - args: string[], - runtimePid: string | undefined, -): { exitCode: number; stdout: string; stderr: string } | undefined { - if (args[0] !== 'shell' || args[1] !== 'pidof') return undefined; - return runtimePid - ? { exitCode: 0, stdout: `${runtimePid}\n`, stderr: '' } - : { exitCode: 1, stdout: '', stderr: '' }; -} - function createFakeTouchHelperSessionProvider( handleCommand: TouchSessionCommandHandler, - options: { stallCleanup?: boolean; runtimePid?: string } = {}, + options: { stallCleanup?: boolean; runtimeRelease?: FakeAndroidHelperRuntimeRelease } = {}, ): AndroidAdbProvider { return { exec: async (args, execOptions) => { @@ -146,13 +141,10 @@ function createFakeTouchHelperSessionProvider( if (options.stallCleanup && signal && isTouchCleanupCommand(args)) { return await neverResolvingAfterAbort(signal); } - return ( - readRuntimePidProbeResult(args, options.runtimePid) ?? { - exitCode: 0, - stdout: '', - stderr: '', - } - ); + if (isAndroidHelperRuntimeProbe(args)) { + return androidHelperRuntimeProbeResult(options.runtimeRelease); + } + return { exitCode: 0, stdout: '', stderr: '' }; }, spawn: (args) => { const port = readSessionPort(args); @@ -197,7 +189,7 @@ test('touch helper does not run one-shot while the device still runs the helper' const device = makeIsolatedDevice(); const deviceKey = getAndroidSnapshotHelperSessionDeviceKey(device); const provider = createFakeTouchHelperSessionProvider(() => 'malformed snapshot response', { - runtimePid: '4211', + runtimeRelease: 'occupied', }); // A malformed response retires the session; the device then answers that the helper process is diff --git a/packages/platform-android/src/snapshot-helper-retirement.ts b/packages/platform-android/src/snapshot-helper-retirement.ts index cf04c3ed62..b86647887b 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -44,11 +44,7 @@ export async function retireCanceledAndroidSnapshotHelperCapture(params: { adb: AndroidAdbExecutor; cause: unknown; }): Promise { - await forceStopAndroidSnapshotHelperRuntime({ - adb: params.adb, - packageName: params.packageName, - timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, - }); + await stopAndroidSnapshotHelperRuntime({ adb: params.adb, packageName: params.packageName }); await recordAndroidSnapshotHelperRelease({ deviceKey: params.deviceKey, packageName: params.packageName, @@ -70,7 +66,7 @@ export async function recoverAndroidSnapshotHelperRetirement(params: { }): Promise { const retirement = pendingRetirements.get(params.deviceKey); if (!retirement) return; - await forceStopAndroidSnapshotHelperRuntime({ + await stopAndroidSnapshotHelperRuntime({ adb: params.adb, packageName: retirement.packageName, timeoutMs: RETIREMENT_RECOVERY_TIMEOUT_MS, @@ -167,7 +163,7 @@ async function readAndroidSnapshotHelperRuntimeRelease(params: { timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, }); // `pidof` exits non-zero and prints nothing when no process matches. - if (result.exitCode !== 0 || !/\d/.test(result.stdout)) return 'released'; + if (result.exitCode !== 0 || !/\b\d+\b/.test(result.stdout)) return 'released'; return 'occupied'; } catch { return 'unknown'; @@ -193,7 +189,7 @@ export async function settleAndroidSnapshotHelperSessionCleanup(params: { await Promise.all([ ...(params.forceStopRuntime ? [ - forceStopAndroidSnapshotHelperRuntime({ + stopAndroidSnapshotHelperRuntime({ adb: params.adb, packageName: params.packageName, timeoutMs: params.timeoutMs, @@ -295,16 +291,16 @@ export function resetAndroidSnapshotHelperRetirements(): void { * Best-effort device-side stop. Its outcome is never release evidence: whoever needs that reads the * device with `readAndroidSnapshotHelperRuntimeRelease`. */ -async function forceStopAndroidSnapshotHelperRuntime(params: { +export async function stopAndroidSnapshotHelperRuntime(params: { adb: AndroidAdbExecutor; packageName: string; - timeoutMs: number; + timeoutMs?: number; signal?: AbortSignal; }): Promise { await params .adb(['shell', 'am', 'force-stop', params.packageName], { allowFailure: true, - timeoutMs: params.timeoutMs, + timeoutMs: params.timeoutMs ?? ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, ...(params.signal ? { signal: params.signal } : {}), }) .catch(() => {}); diff --git a/packages/platform-android/src/snapshot-helper-runtime.ts b/packages/platform-android/src/snapshot-helper-runtime.ts index e34e47f506..f8615a5821 100644 --- a/packages/platform-android/src/snapshot-helper-runtime.ts +++ b/packages/platform-android/src/snapshot-helper-runtime.ts @@ -1,11 +1,10 @@ -import { normalizeError } from '@agent-device/kernel/errors'; import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; import { sleep } from './adb.ts'; import type { AndroidAdbExecutor } from './adb-executor.ts'; +import { stopAndroidSnapshotHelperRuntime } from './snapshot-helper-retirement.ts'; import { stopAndroidSnapshotHelperSession } from './snapshot-helper-session-lifecycle.ts'; const HELPER_RUNTIME_RESET_DELAY_MS = 150; -const HELPER_RUNTIME_RESET_TIMEOUT_MS = 2_000; export async function retireAndroidSnapshotHelperAfterContentFailure(params: { adb: AndroidAdbExecutor; @@ -33,22 +32,11 @@ export async function resetAndroidSnapshotHelperRuntime( adb: AndroidAdbExecutor, packageName: string, ): Promise { - try { - await adb(['shell', 'am', 'force-stop', packageName], { - allowFailure: true, - timeoutMs: HELPER_RUNTIME_RESET_TIMEOUT_MS, - }); - await sleep(HELPER_RUNTIME_RESET_DELAY_MS); - emitDiagnostic({ - level: 'debug', - phase: 'android_snapshot_helper_runtime_reset', - data: { packageName }, - }); - } catch (error) { - emitDiagnostic({ - level: 'warn', - phase: 'android_snapshot_helper_runtime_reset_failed', - data: { packageName, error: normalizeError(error).message }, - }); - } + await stopAndroidSnapshotHelperRuntime({ adb, packageName }); + await sleep(HELPER_RUNTIME_RESET_DELAY_MS); + emitDiagnostic({ + level: 'debug', + phase: 'android_snapshot_helper_runtime_reset', + data: { packageName }, + }); } From 50e09e6c0aecf330a27f409bf2e336561453b115 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 19:59:36 +0200 Subject: [PATCH 03/12] refactor(android): name the retry window a timestamp and share it A disabled session identity stored an epoch time under a duration's name, and the pending retirement next to it asked the same question with its own shape. One retry state now answers both: a value plus when trying again is worth it, standing until then or until the device settles it. The helper device key and the occupied-device predicate stay module-owned: nothing outside platform-android reads them, so they leave the mechanics boundary. --- .../snapshot-helper-retry-state.test.ts | 15 ++++++++++++ packages/platform-android/src/mechanics.ts | 2 -- .../src/snapshot-helper-retirement.ts | 18 +++++++------- .../src/snapshot-helper-retry-state.ts | 17 +++++++++++++ .../src/snapshot-helper-session-lifecycle.ts | 24 ++++++++----------- .../platform-android/src/snapshot-helper.ts | 4 ---- packages/platform-android/src/snapshot.ts | 6 +++-- 7 files changed, 55 insertions(+), 31 deletions(-) create mode 100644 packages/platform-android/src/__tests__/snapshot-helper-retry-state.test.ts create mode 100644 packages/platform-android/src/snapshot-helper-retry-state.ts diff --git a/packages/platform-android/src/__tests__/snapshot-helper-retry-state.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-retry-state.test.ts new file mode 100644 index 0000000000..fbe168f5c1 --- /dev/null +++ b/packages/platform-android/src/__tests__/snapshot-helper-retry-state.test.ts @@ -0,0 +1,15 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { isAndroidSnapshotHelperRetryStateStanding } from '../snapshot-helper-retry-state.ts'; + +const NOW_MS = 10_000_000; + +test('a state with no retry time stands until something settles it', () => { + assert.equal(isAndroidSnapshotHelperRetryStateStanding({ value: 'x' }, NOW_MS), true); +}); + +test('a state with a retry time stands only until that time', () => { + const state = { value: 'x', retryAtMs: NOW_MS + 60_000 }; + assert.equal(isAndroidSnapshotHelperRetryStateStanding(state, NOW_MS), true); + assert.equal(isAndroidSnapshotHelperRetryStateStanding(state, NOW_MS + 60_000), false); +}); diff --git a/packages/platform-android/src/mechanics.ts b/packages/platform-android/src/mechanics.ts index b026b14094..a7173e79f3 100644 --- a/packages/platform-android/src/mechanics.ts +++ b/packages/platform-android/src/mechanics.ts @@ -267,8 +267,6 @@ export { export { ensureAndroidSnapshotHelper, forgetAndroidSnapshotHelperInstall, - getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRuntimeOccupiedError, parseAndroidSnapshotHelperManifest as parseAndroidHelperManifest, resetAndroidSnapshotHelperSessions, stopAndroidSnapshotHelperSession, diff --git a/packages/platform-android/src/snapshot-helper-retirement.ts b/packages/platform-android/src/snapshot-helper-retirement.ts index b86647887b..d8bfe5980b 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -2,6 +2,7 @@ import { AppError } from '@agent-device/kernel/errors'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; import type { AndroidAdbProcess } from './adb-executor.ts'; +import type { AndroidSnapshotHelperRetryState } from './snapshot-helper-retry-state.ts'; import type { AndroidAdbExecutor } from './snapshot-helper-types.ts'; const RETIREMENT_RECOVERY_TIMEOUT_MS = 5_000; @@ -19,11 +20,11 @@ const RUNTIME_OCCUPIED_REASON = 'android_snapshot_helper_runtime_occupied'; */ export type AndroidSnapshotHelperRuntimeRelease = 'released' | 'occupied' | 'unknown'; -/** A retirement whose release the last teardown could not prove; recovered at the next acquire. */ -type PendingRetirement = { +/** A release the last teardown could not prove; settled by the next acquire that reads the device. */ +type PendingRetirement = AndroidSnapshotHelperRetryState<{ packageName: string; cause: string; -}; +}>; const pendingRetirements = new Map(); @@ -68,20 +69,20 @@ export async function recoverAndroidSnapshotHelperRetirement(params: { if (!retirement) return; await stopAndroidSnapshotHelperRuntime({ adb: params.adb, - packageName: retirement.packageName, + packageName: retirement.value.packageName, timeoutMs: RETIREMENT_RECOVERY_TIMEOUT_MS, ...(params.signal ? { signal: params.signal } : {}), }); params.signal?.throwIfAborted(); const release = await readAndroidSnapshotHelperRuntimeRelease({ adb: params.adb, - packageName: retirement.packageName, + packageName: retirement.value.packageName, }); if (release === 'occupied') { throw createAndroidSnapshotHelperRuntimeOccupiedError({ deviceKey: params.deviceKey, - packageName: retirement.packageName, - cause: retirement.cause, + packageName: retirement.value.packageName, + cause: retirement.value.cause, }); } // A device that could not be read leaves the retirement pending: the next acquire asks again, and @@ -115,8 +116,7 @@ export async function recordAndroidSnapshotHelperRelease(params: { } const causeMessage = params.cause instanceof Error ? params.cause.message : String(params.cause); pendingRetirements.set(params.deviceKey, { - packageName: params.packageName, - cause: causeMessage, + value: { packageName: params.packageName, cause: causeMessage }, }); emitDiagnostic({ level: 'warn', diff --git a/packages/platform-android/src/snapshot-helper-retry-state.ts b/packages/platform-android/src/snapshot-helper-retry-state.ts new file mode 100644 index 0000000000..1433543d48 --- /dev/null +++ b/packages/platform-android/src/snapshot-helper-retry-state.ts @@ -0,0 +1,17 @@ +/** + * What a helper teardown leaves standing for one device until an acquire looks again: a session + * identity that should not start yet, or a release that was never proven. `retryAtMs` is the epoch + * time when trying again is worth it, which is what a slow device or transport asks for; a state + * without one stands until the device, or an acquire that reads it, settles it. + */ +export type AndroidSnapshotHelperRetryState = Readonly<{ + value: Value; + retryAtMs?: number; +}>; + +export function isAndroidSnapshotHelperRetryStateStanding( + state: AndroidSnapshotHelperRetryState, + nowMs = Date.now(), +): boolean { + return state.retryAtMs === undefined || nowMs < state.retryAtMs; +} diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index caa125923b..017373ab55 100644 --- a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts +++ b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts @@ -24,6 +24,10 @@ import { resolveAndroidSnapshotHelperCaptureOptions, type AndroidSnapshotHelperResolvedCaptureOptions, } from './snapshot-helper-capture.ts'; +import { + isAndroidSnapshotHelperRetryStateStanding, + type AndroidSnapshotHelperRetryState, +} from './snapshot-helper-retry-state.ts'; import { allocateAndroidSnapshotHelperSessionPort, isAndroidSnapshotHelperSessionCommandAcknowledged, @@ -87,15 +91,8 @@ export type AndroidSnapshotHelperSessionAcquisition = { const sessions = new Map(); -type DisabledAndroidSnapshotHelperSession = { - identity: string; - /** - * When set, the start failed for a reason that says nothing about this identity — the device was - * slow, the transport was slow — so a later command tries again instead of paying one-shot - * instrumentation for the rest of the daemon's life. - */ - retryAfterMs?: number; -}; +/** A start that failed for this identity, standing until it is worth starting again. */ +type DisabledAndroidSnapshotHelperSession = AndroidSnapshotHelperRetryState; const disabledSessionIdentities = new Map(); @@ -197,8 +194,8 @@ function disableAndroidSnapshotHelperSessionIdentity( // start that ran out of time or lost its transport says nothing, so the exclusion expires. const unavailable = provesAndroidSnapshotHelperSessionUnavailable(error); disabledSessionIdentities.set(deviceKey, { - identity, - ...(unavailable ? {} : { retryAfterMs: Date.now() + SESSION_START_RETRY_AFTER_MS }), + value: identity, + ...(unavailable ? {} : { retryAtMs: Date.now() + SESSION_START_RETRY_AFTER_MS }), }); emitDiagnostic({ level: 'warn', @@ -213,9 +210,8 @@ function disableAndroidSnapshotHelperSessionIdentity( function isAndroidSnapshotHelperSessionIdentityDisabled(deviceKey: string, identity: string) { const disabled = disabledSessionIdentities.get(deviceKey); - if (!disabled || disabled.identity !== identity) return false; - if (disabled.retryAfterMs === undefined) return true; - if (Date.now() < disabled.retryAfterMs) return true; + if (!disabled || disabled.value !== identity) return false; + if (isAndroidSnapshotHelperRetryStateStanding(disabled)) return true; disabledSessionIdentities.delete(deviceKey); return false; } diff --git a/packages/platform-android/src/snapshot-helper.ts b/packages/platform-android/src/snapshot-helper.ts index 5eed360b76..33f4e359e2 100644 --- a/packages/platform-android/src/snapshot-helper.ts +++ b/packages/platform-android/src/snapshot-helper.ts @@ -6,10 +6,6 @@ export { stopAndroidSnapshotHelperSession, stopAndroidSnapshotHelperSessionForDevice, } from './snapshot-helper-session-lifecycle.ts'; -export { - getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRuntimeOccupiedError, -} from './snapshot-helper-retirement.ts'; export { ensureAndroidSnapshotHelper, forgetAndroidSnapshotHelperInstall, diff --git a/packages/platform-android/src/snapshot.ts b/packages/platform-android/src/snapshot.ts index d0c21c52f5..8189de96c3 100644 --- a/packages/platform-android/src/snapshot.ts +++ b/packages/platform-android/src/snapshot.ts @@ -36,8 +36,6 @@ import { captureAndroidSnapshotWithHelperSession, ensureAndroidSnapshotHelper, forgetAndroidSnapshotHelperInstall, - getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRuntimeOccupiedError, stopAndroidSnapshotHelperSession, type AndroidAdbExecutor, type AndroidSnapshotHelperArtifact, @@ -45,6 +43,10 @@ import { type AndroidSnapshotHelperInstallResult, type AndroidSnapshotHelperOutput, } from './snapshot-helper.ts'; +import { + getAndroidSnapshotHelperSessionDeviceKey, + isAndroidSnapshotHelperRuntimeOccupiedError, +} from './snapshot-helper-retirement.ts'; import { requireAndroidAdbHost } from './adb-host.ts'; import { parseAndroidSnapshotHelperManifest } from './snapshot-helper-artifact.ts'; import type { AndroidSnapshotBackendMetadata } from './snapshot-types.ts'; From 879af6f664aace33c105808b7e8ecb6433593859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 07:36:43 +0200 Subject: [PATCH 04/12] refactor(android): answer a failed helper start once and budget it as the caller does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A helper start that failed was answered twice: the caller fell back to the one-shot transport, and the identity was excluded anyway — for the daemon's whole life in the worst case, which is how a transient slow start becomes a permanent state. The fallback covers the outcome, so the next command starts again and nothing stands in front of it. The wait for readiness also stops being a guess at how long Android needs. It gets the budget the caller allowed one helper command, which is how `--timeout` reaches it, so a slow device is not pushed out of the persistent path while the transport it fell back to had room for the same start. A helper capture's content branches answer one question — is this worth another call — so they return once, disclosing a system-surface tree instead of giving it its own branch. --- .../snapshot-helper-retry-state.test.ts | 15 --- .../snapshot-helper-session-lifecycle.test.ts | 72 +++++++------- .../src/snapshot-helper-retirement.ts | 16 +-- .../src/snapshot-helper-retry-state.ts | 17 ---- .../src/snapshot-helper-session-lifecycle.ts | 98 +++++-------------- .../src/snapshot-helper-session-protocol.ts | 9 -- packages/platform-android/src/snapshot.ts | 39 ++++---- 7 files changed, 87 insertions(+), 179 deletions(-) delete mode 100644 packages/platform-android/src/__tests__/snapshot-helper-retry-state.test.ts delete mode 100644 packages/platform-android/src/snapshot-helper-retry-state.ts diff --git a/packages/platform-android/src/__tests__/snapshot-helper-retry-state.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-retry-state.test.ts deleted file mode 100644 index fbe168f5c1..0000000000 --- a/packages/platform-android/src/__tests__/snapshot-helper-retry-state.test.ts +++ /dev/null @@ -1,15 +0,0 @@ -import assert from 'node:assert/strict'; -import { test } from 'vitest'; -import { isAndroidSnapshotHelperRetryStateStanding } from '../snapshot-helper-retry-state.ts'; - -const NOW_MS = 10_000_000; - -test('a state with no retry time stands until something settles it', () => { - assert.equal(isAndroidSnapshotHelperRetryStateStanding({ value: 'x' }, NOW_MS), true); -}); - -test('a state with a retry time stands only until that time', () => { - const state = { value: 'x', retryAtMs: NOW_MS + 60_000 }; - assert.equal(isAndroidSnapshotHelperRetryStateStanding(state, NOW_MS), true); - assert.equal(isAndroidSnapshotHelperRetryStateStanding(state, NOW_MS + 60_000), false); -}); diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts index 36d041a505..f1436dda77 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { afterEach, beforeEach, test, vi } from 'vitest'; +import { afterEach, beforeEach, test } from 'vitest'; import { captureAndroidSnapshotWithHelperSession } from '../snapshot-helper-session.ts'; import { resetAndroidSnapshotHelperSessions, @@ -50,7 +50,7 @@ test('returns undefined when the adb provider cannot spawn a helper process', as assert.deepEqual(calls, []); }); -test('disables repeated persistent session attempts after startup failure', async () => { +test('a failed start answers with the one-shot transport and the next command starts again', async () => { const calls: string[][] = []; const spawnArgs: string[][] = []; const provider: AndroidAdbProvider = { @@ -79,40 +79,39 @@ test('disables repeated persistent session attempts after startup failure', asyn assert.equal(first, undefined); assert.equal(second, undefined); - assert.equal(spawnArgs.length, 1); - assert.equal(readSessionArgument(spawnArgs[0]!, 'timeoutMs'), '2000'); - assert.equal(calls.filter((args) => args[0] === 'forward').length, 2); + assert.equal(spawnArgs.length, 2, 'a failed start does not end the persistent path'); + assert.equal( + calls.filter((args) => args[0] === 'forward' && args[1]?.startsWith('tcp:')).length, + 2, + ); }); -test('a start that failed for a transient reason is retried instead of ending the persistent path', async () => { - vi.useFakeTimers({ toFake: ['Date'] }); - try { - const calls: string[][] = []; - const spawnArgs: string[][] = []; - const provider = createSessionProvider({ calls, spawnArgs }); - let transportHealthy = false; - const adb: AndroidAdbExecutor = async (args, options) => { - if (args[0] === 'forward' && !transportHealthy) throw new Error('adb server is restarting'); - return await provider.exec!(args, options); - }; - const capture = () => - captureAndroidSnapshotWithHelperSession({ - adb, - adbProvider: { ...provider, exec: adb }, - deviceKey: 'android:emulator-5554', - }); - - assert.equal(await capture(), undefined); - assert.equal(await capture(), undefined); - assert.equal(spawnArgs.length, 0, 'the failed start is not retried inside its cooldown'); - - vi.advanceTimersByTime(61_000); - transportHealthy = true; - assert.match((await capture())?.xml ?? '', /snapshot 1/); - assert.equal(spawnArgs.length, 1); - } finally { - vi.useRealTimers(); - } +test('a session start waits only as long as the caller budgeted for one helper command', async () => { + const spawnArgs: string[][] = []; + const provider = createSessionProvider({ calls: [], spawnArgs }); + const startedAtMs = Date.now(); + + // The spawned instrumentation never announces readiness, so only the caller's own command budget + // ends the wait. A fixed floor above it would starve the one-shot transport this call falls back to. + const output = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider: { + ...provider, + spawn: (args) => { + spawnArgs.push(args); + return new FakeAndroidProcess(); + }, + }, + deviceKey: 'android:emulator-5554', + commandTimeoutMs: 50, + }); + + assert.equal(output, undefined); + assert.equal(spawnArgs.length, 1); + assert.ok( + Date.now() - startedAtMs < 3_000, + 'the start obeys the caller budget, not a fixed floor', + ); }); test('starts and reuses a persistent Android snapshot helper session', async () => { @@ -337,8 +336,3 @@ function countForceStops(options: SessionProviderOptions): number { function isHelperRuntimeForceStop(args: string[]): boolean { return args.join(' ') === 'shell am force-stop com.callstack.agentdevice.snapshothelper'; } - -function readSessionArgument(args: string[], name: string): string | undefined { - const index = args.indexOf(name); - return index < 0 ? undefined : args[index + 1]; -} diff --git a/packages/platform-android/src/snapshot-helper-retirement.ts b/packages/platform-android/src/snapshot-helper-retirement.ts index d8bfe5980b..cf64830101 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -2,7 +2,6 @@ import { AppError } from '@agent-device/kernel/errors'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; import type { AndroidAdbProcess } from './adb-executor.ts'; -import type { AndroidSnapshotHelperRetryState } from './snapshot-helper-retry-state.ts'; import type { AndroidAdbExecutor } from './snapshot-helper-types.ts'; const RETIREMENT_RECOVERY_TIMEOUT_MS = 5_000; @@ -21,10 +20,10 @@ const RUNTIME_OCCUPIED_REASON = 'android_snapshot_helper_runtime_occupied'; export type AndroidSnapshotHelperRuntimeRelease = 'released' | 'occupied' | 'unknown'; /** A release the last teardown could not prove; settled by the next acquire that reads the device. */ -type PendingRetirement = AndroidSnapshotHelperRetryState<{ +type PendingRetirement = { packageName: string; cause: string; -}>; +}; const pendingRetirements = new Map(); @@ -69,20 +68,20 @@ export async function recoverAndroidSnapshotHelperRetirement(params: { if (!retirement) return; await stopAndroidSnapshotHelperRuntime({ adb: params.adb, - packageName: retirement.value.packageName, + packageName: retirement.packageName, timeoutMs: RETIREMENT_RECOVERY_TIMEOUT_MS, ...(params.signal ? { signal: params.signal } : {}), }); params.signal?.throwIfAborted(); const release = await readAndroidSnapshotHelperRuntimeRelease({ adb: params.adb, - packageName: retirement.value.packageName, + packageName: retirement.packageName, }); if (release === 'occupied') { throw createAndroidSnapshotHelperRuntimeOccupiedError({ deviceKey: params.deviceKey, - packageName: retirement.value.packageName, - cause: retirement.value.cause, + packageName: retirement.packageName, + cause: retirement.cause, }); } // A device that could not be read leaves the retirement pending: the next acquire asks again, and @@ -116,7 +115,8 @@ export async function recordAndroidSnapshotHelperRelease(params: { } const causeMessage = params.cause instanceof Error ? params.cause.message : String(params.cause); pendingRetirements.set(params.deviceKey, { - value: { packageName: params.packageName, cause: causeMessage }, + packageName: params.packageName, + cause: causeMessage, }); emitDiagnostic({ level: 'warn', diff --git a/packages/platform-android/src/snapshot-helper-retry-state.ts b/packages/platform-android/src/snapshot-helper-retry-state.ts deleted file mode 100644 index 1433543d48..0000000000 --- a/packages/platform-android/src/snapshot-helper-retry-state.ts +++ /dev/null @@ -1,17 +0,0 @@ -/** - * What a helper teardown leaves standing for one device until an acquire looks again: a session - * identity that should not start yet, or a release that was never proven. `retryAtMs` is the epoch - * time when trying again is worth it, which is what a slow device or transport asks for; a state - * without one stands until the device, or an acquire that reads it, settles it. - */ -export type AndroidSnapshotHelperRetryState = Readonly<{ - value: Value; - retryAtMs?: number; -}>; - -export function isAndroidSnapshotHelperRetryStateStanding( - state: AndroidSnapshotHelperRetryState, - nowMs = Date.now(), -): boolean { - return state.retryAtMs === undefined || nowMs < state.retryAtMs; -} diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index 017373ab55..1c4f84dcd2 100644 --- a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts +++ b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts @@ -24,14 +24,9 @@ import { resolveAndroidSnapshotHelperCaptureOptions, type AndroidSnapshotHelperResolvedCaptureOptions, } from './snapshot-helper-capture.ts'; -import { - isAndroidSnapshotHelperRetryStateStanding, - type AndroidSnapshotHelperRetryState, -} from './snapshot-helper-retry-state.ts'; import { allocateAndroidSnapshotHelperSessionPort, isAndroidSnapshotHelperSessionCommandAcknowledged, - provesAndroidSnapshotHelperSessionUnavailable, sendAndroidSnapshotHelperSessionCommand, waitForAndroidSnapshotHelperSessionReady, } from './snapshot-helper-session-protocol.ts'; @@ -49,9 +44,6 @@ import { waitForAndroidSnapshotHelperProcessExit, } from './snapshot-helper-retirement.ts'; -const SESSION_READY_TIMEOUT_MS = 10_000; -// How long a device that merely started too slowly stays excluded from the persistent path. -const SESSION_START_RETRY_AFTER_MS = 60_000; const SESSION_STOP_TIMEOUT_MS = 1_000; // SnapshotInstrumentation.finishSafely can spend up to 10 seconds waiting for Android to finish // connecting UiAutomation. Let an acknowledged quit complete that release before force-killing adb. @@ -91,11 +83,6 @@ export type AndroidSnapshotHelperSessionAcquisition = { const sessions = new Map(); -/** A start that failed for this identity, standing until it is worth starting again. */ -type DisabledAndroidSnapshotHelperSession = AndroidSnapshotHelperRetryState; - -const disabledSessionIdentities = new Map(); - /** * Starts (or reuses) the session without capturing, so a helper-backed read that is not a snapshot * — the gesture viewport — can leave a warm session behind for the gesture that follows instead of @@ -155,67 +142,31 @@ async function resolveAndroidSnapshotHelperSession(params: { await stopAndroidSnapshotHelperSession(deviceKey); session = undefined; } - if (!session && !isAndroidSnapshotHelperSessionIdentityDisabled(deviceKey, identity)) { - session = await startAndroidSnapshotHelperSessionOrDisable({ - deviceKey, - identity, - options, - resolved, - }); + if (!session) { + try { + session = await startAndroidSnapshotHelperSession({ + deviceKey, + identity, + options, + resolved, + }); + } catch (error) { + // A start that failed is not a command that failed: the caller answers with the one-shot + // transport and the next command tries the persistent path again. + options.signal?.throwIfAborted(); + emitDiagnostic({ + level: 'warn', + phase: 'android_snapshot_helper_session_start_failed', + data: { + deviceKey, + reason: error instanceof Error ? error.message : String(error), + }, + }); + } } return session; } -/** - * A start that failed is not a command that failed: the caller answers with the one-shot transport. - * What it does decide is how soon this identity is worth starting again. - */ -async function startAndroidSnapshotHelperSessionOrDisable(params: { - deviceKey: string; - identity: string; - options: AndroidSnapshotHelperCaptureOptions; - resolved: AndroidSnapshotHelperResolvedCaptureOptions; -}): Promise { - try { - return await startAndroidSnapshotHelperSession(params); - } catch (error) { - params.options.signal?.throwIfAborted(); - disableAndroidSnapshotHelperSessionIdentity(params.deviceKey, params.identity, error); - return undefined; - } -} - -function disableAndroidSnapshotHelperSessionIdentity( - deviceKey: string, - identity: string, - error: unknown, -): void { - // Only a helper that ran and exited before announcing readiness proves this identity unusable. A - // start that ran out of time or lost its transport says nothing, so the exclusion expires. - const unavailable = provesAndroidSnapshotHelperSessionUnavailable(error); - disabledSessionIdentities.set(deviceKey, { - value: identity, - ...(unavailable ? {} : { retryAtMs: Date.now() + SESSION_START_RETRY_AFTER_MS }), - }); - emitDiagnostic({ - level: 'warn', - phase: 'android_snapshot_helper_session_disabled', - data: { - deviceKey, - reason: error instanceof Error ? error.message : String(error), - ...(unavailable ? {} : { retryAfterMs: SESSION_START_RETRY_AFTER_MS }), - }, - }); -} - -function isAndroidSnapshotHelperSessionIdentityDisabled(deviceKey: string, identity: string) { - const disabled = disabledSessionIdentities.get(deviceKey); - if (!disabled || disabled.value !== identity) return false; - if (isAndroidSnapshotHelperRetryStateStanding(disabled)) return true; - disabledSessionIdentities.delete(deviceKey); - return false; -} - async function startAndroidSnapshotHelperSession(params: { deviceKey: string; identity: string; @@ -259,13 +210,15 @@ async function startAndroidSnapshotHelperSession(params: { capturedCount: 0, }; try { + // Starting the session gets the budget the caller already allowed one helper command, which is + // how `--timeout` reaches it. A fixed guess below that pushed a slow device out of the persistent + // path while the one-shot transport it fell back to had room for the same start. await waitForAndroidSnapshotHelperSessionReady( childProcess, - SESSION_READY_TIMEOUT_MS, + params.resolved.commandTimeoutMs, params.options.signal, ); sessions.set(params.deviceKey, session); - disabledSessionIdentities.delete(params.deviceKey); emitDiagnostic({ phase: 'android_snapshot_helper_session_ready', data: { @@ -482,7 +435,6 @@ export async function resetAndroidSnapshotHelperSessions(): Promise { await stopAndroidSnapshotHelperSession(deviceKey); }), ); - disabledSessionIdentities.clear(); resetAndroidSnapshotHelperRetirements(); resetAndroidAdbShellProtocolProbes(); } diff --git a/packages/platform-android/src/snapshot-helper-session-protocol.ts b/packages/platform-android/src/snapshot-helper-session-protocol.ts index 489120e88f..05fb4ab5f3 100644 --- a/packages/platform-android/src/snapshot-helper-session-protocol.ts +++ b/packages/platform-android/src/snapshot-helper-session-protocol.ts @@ -111,15 +111,6 @@ export function resolveAndroidSnapshotHelperSessionRequestTimeoutMs(params: { const SESSION_READY_TIMEOUT_REASON = 'android_snapshot_helper_session_ready_timeout'; const SESSION_EXITED_BEFORE_READY_REASON = 'android_snapshot_helper_session_exited_before_ready'; -/** - * Whether a failed start proves this helper identity cannot serve this device at all. A helper that - * ran and ended before announcing readiness says so; a start that only ran out of time or lost its - * transport says nothing about the identity, and must not exclude it for the daemon's whole life. - */ -export function provesAndroidSnapshotHelperSessionUnavailable(error: unknown): boolean { - return error instanceof AppError && error.details?.reason === SESSION_EXITED_BEFORE_READY_REASON; -} - export function waitForAndroidSnapshotHelperSessionReady( childProcess: AndroidAdbProcess, timeoutMs: number, diff --git a/packages/platform-android/src/snapshot.ts b/packages/platform-android/src/snapshot.ts index 8189de96c3..becae49968 100644 --- a/packages/platform-android/src/snapshot.ts +++ b/packages/platform-android/src/snapshot.ts @@ -480,29 +480,32 @@ async function captureAndroidHelperContentAttempt(params: { const content = classifyAndroidHelperContent(helperCapture.xml, helperCapture.metadata, { foregroundAppPackage: options.appBundleId, }); - if (content.outcome === 'system-surface-only') { + if (content.outcome === 'unusable') { + return { outcome: 'unusable', decision: content.decision }; + } + // Only content the helper cannot answer with is worth another call. A tree holding just the + // system surface is an answer, so it is disclosed rather than recaptured. + const systemSurfaceOnly = content.outcome === 'system-surface-only'; + if (systemSurfaceOnly) { emitDiagnostic({ phase: 'android_snapshot_helper_system_surface', data: { foregroundAppPackage: options.appBundleId }, }); - return { - outcome: 'captured', - capture: { - xml: helperCapture.xml, - metadata: { ...helperCapture.metadata, systemSurfaceOnly: true }, - }, - }; - } - if (content.outcome === 'ok') { - if (attempt > 0) { - emitDiagnostic({ - phase: 'android_snapshot_helper_content_recaptured', - data: { attempts: attempt + 1, recoveredFromReason: params.previousContentReason }, - }); - } - return { outcome: 'captured', capture: helperCapture }; + } else if (attempt > 0) { + emitDiagnostic({ + phase: 'android_snapshot_helper_content_recaptured', + data: { attempts: attempt + 1, recoveredFromReason: params.previousContentReason }, + }); } - return { outcome: 'unusable', decision: content.decision }; + return { + outcome: 'captured', + capture: { + xml: helperCapture.xml, + metadata: systemSurfaceOnly + ? { ...helperCapture.metadata, systemSurfaceOnly: true } + : helperCapture.metadata, + }, + }; } async function delayBeforeContentRecapture(signal?: AbortSignal): Promise { From e404936ec15a364de3b7e123e82d190f24936769 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 14:44:09 +0200 Subject: [PATCH 05/12] refactor(android): keep the system-surface disclosure in the capture metadata --- packages/platform-android/src/snapshot.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/platform-android/src/snapshot.ts b/packages/platform-android/src/snapshot.ts index becae49968..008e30913a 100644 --- a/packages/platform-android/src/snapshot.ts +++ b/packages/platform-android/src/snapshot.ts @@ -484,14 +484,10 @@ async function captureAndroidHelperContentAttempt(params: { return { outcome: 'unusable', decision: content.decision }; } // Only content the helper cannot answer with is worth another call. A tree holding just the - // system surface is an answer, so it is disclosed rather than recaptured. + // system surface is an answer, and `systemSurfaceOnly` on the capture is where it is recorded: + // it travels with the response and becomes the disclosure the caller reads. const systemSurfaceOnly = content.outcome === 'system-surface-only'; - if (systemSurfaceOnly) { - emitDiagnostic({ - phase: 'android_snapshot_helper_system_surface', - data: { foregroundAppPackage: options.appBundleId }, - }); - } else if (attempt > 0) { + if (!systemSurfaceOnly && attempt > 0) { emitDiagnostic({ phase: 'android_snapshot_helper_content_recaptured', data: { attempts: attempt + 1, recoveredFromReason: params.previousContentReason }, From 5ae5964c30e1f0afb03612a58e2b0daa2fb0f6b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 14:44:09 +0200 Subject: [PATCH 06/12] fix(android): start a helper instead of writing to a session that already died --- .../snapshot-helper-session-lifecycle.test.ts | 27 +++++++++++++++++++ .../src/snapshot-helper-retirement.ts | 3 ++- .../src/snapshot-helper-session-lifecycle.ts | 27 ++++++++++++++++--- 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts index f1436dda77..c084731345 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts @@ -174,6 +174,33 @@ test('restarts the helper session when capture options change', async () => { ); }); +test('a session whose helper process died is not written to again', async () => { + const calls: string[][] = []; + const spawnArgs: string[][] = []; + const processes: FakeAndroidProcess[] = []; + const provider = createSessionProvider({ calls, spawnArgs, processes }); + + await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider: provider, + deviceKey: 'android:emulator-5554', + }); + processes[0]!.emitExit(137, null); + + const restarted = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider: provider, + deviceKey: 'android:emulator-5554', + }); + + assert.equal(restarted?.metadata.sessionReused, false); + assert.equal(spawnArgs.length, 2); + assert.equal( + calls.some((args) => args[0] === 'forward' && args[1] === '--remove'), + true, + ); +}); + test('a quit acknowledged and followed by process exit skips the force-stop round trip', async () => { const calls: string[][] = []; const processes: FakeAndroidProcess[] = []; diff --git a/packages/platform-android/src/snapshot-helper-retirement.ts b/packages/platform-android/src/snapshot-helper-retirement.ts index cf64830101..9b1f3e68b2 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -238,7 +238,8 @@ export function observeAndroidSnapshotHelperProcessExit( }; } -function hasAndroidSnapshotHelperProcessEnded(childProcess: AndroidAdbProcess): boolean { +/** Whether the host side of an instrumentation session is already gone. */ +export function hasAndroidSnapshotHelperProcessEnded(childProcess: AndroidAdbProcess): boolean { return childProcess.exitCode != null || childProcess.signalCode != null; } diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index 1c4f84dcd2..4c2f81ca4e 100644 --- a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts +++ b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts @@ -35,6 +35,7 @@ import { ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, ANDROID_SNAPSHOT_HELPER_HOST_PROCESS_EXIT_GRACE_MS, getAndroidSnapshotHelperSessionDeviceKey, + hasAndroidSnapshotHelperProcessEnded, observeAndroidSnapshotHelperProcessExit, recoverAndroidSnapshotHelperRetirement, recordAndroidSnapshotHelperRelease, @@ -137,11 +138,16 @@ async function resolveAndroidSnapshotHelperSession(params: { resolved: AndroidSnapshotHelperResolvedCaptureOptions; }): Promise { const { deviceKey, identity, options, resolved } = params; - let session = sessions.get(deviceKey); - if (session && session.identity !== identity) { - await stopAndroidSnapshotHelperSession(deviceKey); - session = undefined; + const cached = sessions.get(deviceKey); + const reusable = cached !== undefined && isReusableAndroidSnapshotHelperSession(cached, identity); + if (cached && !reusable) { + await stopAndroidSnapshotHelperSession(deviceKey, { + // A process that already exited cannot answer the forwarded port: the forward is all that is + // left of it, so there is nothing to quit gracefully. + force: hasAndroidSnapshotHelperProcessEnded(cached.process), + }); } + let session = reusable ? cached : undefined; if (!session) { try { session = await startAndroidSnapshotHelperSession({ @@ -167,6 +173,19 @@ async function resolveAndroidSnapshotHelperSession(params: { return session; } +/** + * A cached session is worth writing to only while it belongs to this helper build and its + * instrumentation process is still running. The helper binds its session socket inside that + * process, so a process that has exited has nobody left to accept on the forwarded port: the + * command would die on a dead socket and fall back, instead of starting a helper that can answer. + */ +function isReusableAndroidSnapshotHelperSession( + session: AndroidSnapshotHelperSession, + identity: string, +): boolean { + return session.identity === identity && !hasAndroidSnapshotHelperProcessEnded(session.process); +} + async function startAndroidSnapshotHelperSession(params: { deviceKey: string; identity: string; From f8800064c7d5c930d612480b5734567fdce5dd58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 14:58:06 +0200 Subject: [PATCH 07/12] fix(android): tell a device that answered from an adb that could not carry the call --- .../snapshot-helper-retirement.test.ts | 55 +++++++++++++++++++ .../snapshot-helper-session.fixtures.ts | 5 +- .../src/snapshot-helper-retirement.ts | 40 ++++++++++++-- 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts index c8e9dc2bed..163c0473ea 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts @@ -51,7 +51,9 @@ test('canceled capture answers for the device, not for the force-stop call that test('unproven release stays pending until an acquire reads the device', async () => { let helperAlive = true; + const calls: string[][] = []; const adb: AndroidAdbExecutor = async (args) => { + calls.push(args); if (isAndroidHelperRuntimeProbe(args)) { return androidHelperRuntimeProbeResult(helperAlive ? 'occupied' : 'released'); } @@ -64,19 +66,39 @@ test('unproven release stays pending until an acquire reads the device', async ( adb, cause: new Error('capture canceled'), }); + assert.deepEqual(calls, [ + ['shell', 'am', 'force-stop', PACKAGE_NAME], + ['shell', 'pidof', PACKAGE_NAME], + ]); await assert.rejects( recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }), isAndroidSnapshotHelperRuntimeOccupiedError, ); + // A refusal is the strongest thing an acquire does with this read, so it is earned by a force-stop + // and two reads that both name the process. + assert.deepEqual(calls.slice(2), [ + ['shell', 'am', 'force-stop', PACKAGE_NAME], + ['shell', 'pidof', PACKAGE_NAME], + ['shell', 'pidof', PACKAGE_NAME], + ]); helperAlive = false; await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); + assert.deepEqual(calls.slice(5), [ + ['shell', 'am', 'force-stop', PACKAGE_NAME], + ['shell', 'pidof', PACKAGE_NAME], + ]); + + // The release is proven, so the entry is gone and a further acquire has nothing to settle. await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); + assert.equal(calls.length, 7); }); test('a device that cannot be read leaves the retirement pending without failing the command', async () => { + const calls: string[][] = []; const adb: AndroidAdbExecutor = async (args) => { + calls.push(args); if (isAndroidHelperRuntimeProbe(args)) return androidHelperRuntimeProbeResult('unreadable'); return { exitCode: 0, stdout: '', stderr: '' }; }; @@ -88,7 +110,40 @@ test('a device that cannot be read leaves the retirement pending without failing cause: new Error('quit timed out'), }); assert.equal(release, 'unknown'); + + // `adb` answered with its own transport fault, which says nothing about the helper process. The + // acquire stops the runtime and asks again, and keeps doing that on every command until the device + // can be read — unless a session reaches ready first, which settles it from the other end. + await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); + assert.deepEqual(calls.slice(1), [ + ['shell', 'am', 'force-stop', PACKAGE_NAME], + ['shell', 'pidof', PACKAGE_NAME], + ]); await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); + assert.deepEqual(calls.slice(3), [ + ['shell', 'am', 'force-stop', PACKAGE_NAME], + ['shell', 'pidof', PACKAGE_NAME], + ]); +}); + +test('a shell that has no pidof still answers for its own processes', async () => { + // An older device image reports the missing command on stderr and exits non-zero. That is the + // device answering, not a transport fault, and reading it as `unknown` would keep every later + // acquire force-stopping a runtime that was never held. + const adb: AndroidAdbExecutor = async () => ({ + exitCode: 1, + stdout: '', + stderr: '/system/bin/sh: pidof: not found', + }); + + const release = await recordAndroidSnapshotHelperRelease({ + deviceKey: DEVICE_KEY, + packageName: PACKAGE_NAME, + adb, + cause: new Error('quit timed out'), + }); + + assert.equal(release, 'released'); }); test('session cleanup stops the runtime even when the transport refuses the stop', async () => { diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts index 355a15b334..81d3fffc17 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts @@ -296,7 +296,10 @@ export function isAndroidHelperRuntimeProbe(args: readonly string[]): boolean { export function androidHelperRuntimeProbeResult( release: FakeAndroidHelperRuntimeRelease = 'released', ): AndroidAdbExecutorResult { - if (release === 'unreadable') throw new Error('device offline'); + // A host whose adb cannot carry the call answers the way the executor really answers it: a non-zero + // exit with a transport fault on stderr, which is a different shape from a device that says "no + // such process" only by what it prints. + if (release === 'unreadable') return { exitCode: 1, stdout: '', stderr: 'error: device offline' }; return release === 'occupied' ? { exitCode: 0, stdout: '4211\n', stderr: '' } : { exitCode: 1, stdout: '', stderr: '' }; diff --git a/packages/platform-android/src/snapshot-helper-retirement.ts b/packages/platform-android/src/snapshot-helper-retirement.ts index 9b1f3e68b2..98a66d2692 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -1,10 +1,17 @@ import { AppError } from '@agent-device/kernel/errors'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; +import { sleep } from '@agent-device/host-kit/retry'; +import { classifyAndroidAdbFailure } from './adb-failure.ts'; +import { findPidToken } from './perf-native-process.ts'; import type { AndroidAdbProcess } from './adb-executor.ts'; import type { AndroidAdbExecutor } from './snapshot-helper-types.ts'; const RETIREMENT_RECOVERY_TIMEOUT_MS = 5_000; +// A force-stopped helper can still be inside Android's process-teardown path when the next acquire +// asks who owns the runtime. Refusing a command is a stronger claim than one read supports, so the +// refusal reads the same fact again after this long. +const RUNTIME_OCCUPANCY_RECHECK_MS = 250; // Host-process termination is local and should be nearly immediate. Device-side force-stop is an // adb round trip and needs its own budget; sharing the host grace caused healthy CI force-stops to // time out before Android could confirm UiAutomation release. @@ -73,10 +80,20 @@ export async function recoverAndroidSnapshotHelperRetirement(params: { ...(params.signal ? { signal: params.signal } : {}), }); params.signal?.throwIfAborted(); - const release = await readAndroidSnapshotHelperRuntimeRelease({ + let release = await readAndroidSnapshotHelperRuntimeRelease({ adb: params.adb, packageName: retirement.packageName, }); + if (release === 'occupied') { + // A helper that was force-stopped a moment ago can still be inside Android's exit path while + // `pidof` answers. Refusing a command is the strongest claim this function makes, so it is the + // one that asks the device twice. + await sleep(RUNTIME_OCCUPANCY_RECHECK_MS); + release = await readAndroidSnapshotHelperRuntimeRelease({ + adb: params.adb, + packageName: retirement.packageName, + }); + } if (release === 'occupied') { throw createAndroidSnapshotHelperRuntimeOccupiedError({ deviceKey: params.deviceKey, @@ -162,9 +179,13 @@ async function readAndroidSnapshotHelperRuntimeRelease(params: { allowFailure: true, timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, }); - // `pidof` exits non-zero and prints nothing when no process matches. - if (result.exitCode !== 0 || !/\b\d+\b/.test(result.stdout)) return 'released'; - return 'occupied'; + // A process id for the helper package is the device naming whoever owns the runtime. + if (findPidToken(result.stdout)) return 'occupied'; + // `pidof` prints nothing for "no such process", and adb prints nothing useful on stdout when the + // call never reached a device. On the transport this probe exists for, that shape is common: a + // stderr the adb failure classifier recognises as a device or transport fault is no answer at + // all, while an unclassified one (an older shell without `pidof`) is the device's own. + return classifyAndroidAdbFailure(result.stderr, result.stdout) ? 'unknown' : 'released'; } catch { return 'unknown'; } @@ -284,6 +305,17 @@ export async function stopAndroidSnapshotHelperHostProcess(params: { return await waitForAndroidSnapshotHelperProcessExit(params.processExit.ended, params.timeoutMs); } +/** + * Settles a release the last teardown could not prove, from the other end of the device. Android + * hands UiAutomation to one connection at a time, so a helper that has just reported itself ready + * owns it now and whatever held it before no longer does. The session lifecycle calls this on the + * way to ready: an unreadable `pidof` must not leave a pending entry that force-stops a live helper + * on the next acquire. + */ +export function settleAndroidSnapshotHelperRetirement(deviceKey: string): void { + pendingRetirements.delete(deviceKey); +} + export function resetAndroidSnapshotHelperRetirements(): void { pendingRetirements.clear(); } From 4ae33adefed735294866353b48a6f7338ff6a444 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 14:58:06 +0200 Subject: [PATCH 08/12] fix(android): settle an unproven release at ready and back off a failed start --- CHANGELOG.md | 20 +++ .../snapshot-helper-session-lifecycle.test.ts | 101 ++++++++++--- .../src/snapshot-helper-session-lifecycle.ts | 137 +++++++++++++----- 3 files changed, 197 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4307468822..2cd081b854 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,26 @@ - Fixed (android): a chunked `record stop` (recordings over 170 s) no longer warns that screenrecord stopped before record stop at the 180 s limit. Rotation always ends every earlier chunk before stop, so the warning now fires only when the last chunk's recorder had already exited. +- Fixed (android): a snapshot helper that could not prove it released device automation no longer + refuses the next command on the strength of an `adb` call. The old code read the outcome of + `am force-stop` as the fact it was supposed to measure, and on a loaded host that round trip can + outlive its budget while the device is healthy — the shape of #2553 — so the helper process was + already gone and the command still failed with `Android automation helper is still holding device + automation ownership`. Ownership is read off the device now: `adb shell pidof + com.callstack.agentdevice.snapshothelper` says `occupied` only while it names a process, `released` + when the device answers that nothing is running, and `unknown` when adb could not carry the call, + so a `device offline` stderr no longer counts as a release either. A refusal requires two reads that + both name the process, which keeps a helper still inside Android's exit path from costing a + command. Scripts that match the failure reason see `android_snapshot_helper_runtime_occupied`, + which replaces `android_snapshot_helper_retirement_unconfirmed`. +- Changed (android): a snapshot helper session that reaches ready settles a release the previous + teardown could not prove, because Android hands UiAutomation to one connection at a time and that + helper owns it now; the next command no longer force-stops the session it has just started while + `pidof` happens to be unreadable. A helper start that fails is also retried after a backoff scaled + to how long it spent failing (10 s to 60 s) instead of on every command, which had roughly doubled + command time on hosts where the helper never starts, and the wait for a started helper to announce + itself now uses the caller's own helper-command budget, so `--timeout` reaches it. + - Fixed: an iOS snapshot whose XCTest query-sweep tier cannot read the screen no longer ends the runner process. On a live React Native feed (Bluesky Home, images re-rendering) the AX server rejects each of the sweep's 19 element-type queries with `kAXErrorIllegalArgument`, and XCTest diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts index c084731345..89b9be989e 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts @@ -9,6 +9,7 @@ import { recoverAndroidSnapshotHelperRetirement } from '../snapshot-helper-retir import { createSessionProvider, FakeAndroidProcess, + isAndroidHelperRuntimeForceStop, type SessionProviderOptions, } from './snapshot-helper-session.fixtures.ts'; import type { AndroidAdbExecutor, AndroidAdbProvider } from '../adb-executor.ts'; @@ -50,40 +51,48 @@ test('returns undefined when the adb provider cannot spawn a helper process', as assert.deepEqual(calls, []); }); -test('a failed start answers with the one-shot transport and the next command starts again', async () => { +test('a helper that never starts is not spawned again on every command', async () => { const calls: string[][] = []; const spawnArgs: string[][] = []; - const provider: AndroidAdbProvider = { - exec: async (args) => { - calls.push(args); - return { exitCode: 0, stdout: '', stderr: '' }; - }, + const provider = createSessionProvider({ calls }); + // The spawned instrumentation never announces readiness, so every start spends the caller's whole + // command budget failing before the one-shot transport answers. Paying for that on every command + // is what made the commands of #2553 take roughly twice as long. + const adbProvider: AndroidAdbProvider = { + ...provider, spawn: (args) => { spawnArgs.push(args); - const process = new FakeAndroidProcess(); - queueMicrotask(() => process.emitExit(0, null)); - return process; + return new FakeAndroidProcess(); }, }; - const first = await captureAndroidSnapshotWithHelperSession({ - adb: provider.exec, - adbProvider: provider, - deviceKey: 'android:emulator-5554', - }); - const second = await captureAndroidSnapshotWithHelperSession({ - adb: provider.exec, - adbProvider: provider, - deviceKey: 'android:emulator-5554', - }); + for (let command = 0; command < 3; command += 1) { + const output = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider, + deviceKey: 'android:emulator-5554', + commandTimeoutMs: 50, + }); + assert.equal(output, undefined, 'the one-shot transport answers every command'); + } - assert.equal(first, undefined); - assert.equal(second, undefined); - assert.equal(spawnArgs.length, 2, 'a failed start does not end the persistent path'); + assert.equal(spawnArgs.length, 1, 'a failed start earns a backoff, not another spawn'); assert.equal( calls.filter((args) => args[0] === 'forward' && args[1]?.startsWith('tcp:')).length, - 2, + 1, ); + + // The backoff belongs to one helper build under one set of budgets, not to the device forever. + const otherBuild = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider, + deviceKey: 'android:emulator-5554', + commandTimeoutMs: 50, + waitForIdleTimeoutMs: 40, + }); + + assert.equal(otherBuild, undefined); + assert.equal(spawnArgs.length, 2, 'a different capture identity is not covered by the backoff'); }); test('a session start waits only as long as the caller budgeted for one helper command', async () => { @@ -114,6 +123,52 @@ test('a session start waits only as long as the caller budgeted for one helper c ); }); +test('a session that reaches ready settles a release the device could not confirm', async () => { + const calls: string[][] = []; + const spawnArgs: string[][] = []; + // The device cannot be read at all, and the first command's session stalls, so that command's + // teardown records a release nothing could prove. + const provider = createSessionProvider({ + calls, + spawnArgs, + stalledSnapshots: 1, + runtimeRelease: 'unreadable', + }); + + const stalled = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider: provider, + deviceKey: 'android:emulator-5554', + commandTimeoutMs: 400, + }); + assert.equal(stalled, undefined); + + const started = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider: provider, + deviceKey: 'android:emulator-5554', + commandTimeoutMs: 400, + }); + assert.equal(started?.metadata.sessionReused, false); + const forceStopsWhilePending = calls.filter(isAndroidHelperRuntimeForceStop).length; + + // Android hands UiAutomation to one connection, so the helper that just reported itself ready owns + // the runtime and the unreadable device has nothing left to hold the next command with. + const reused = await captureAndroidSnapshotWithHelperSession({ + adb: provider.exec, + adbProvider: provider, + deviceKey: 'android:emulator-5554', + commandTimeoutMs: 400, + }); + + assert.equal(reused?.metadata.sessionReused, true); + assert.equal( + calls.filter(isAndroidHelperRuntimeForceStop).length, + forceStopsWhilePending, + 'a live session is not force-stopped for a release its own readiness settled', + ); +}); + test('starts and reuses a persistent Android snapshot helper session', async () => { const calls: string[][] = []; const spawnArgs: string[][] = []; diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index 4c2f81ca4e..a567b75ea8 100644 --- a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts +++ b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts @@ -40,6 +40,7 @@ import { recoverAndroidSnapshotHelperRetirement, recordAndroidSnapshotHelperRelease, resetAndroidSnapshotHelperRetirements, + settleAndroidSnapshotHelperRetirement, settleAndroidSnapshotHelperSessionCleanup, stopAndroidSnapshotHelperHostProcess, waitForAndroidSnapshotHelperProcessExit, @@ -55,6 +56,15 @@ const SESSION_PROCESS_EXIT_TIMEOUT_MS = 2_000; const SESSION_CAPTURE_TIMEOUT_MS = 2_000; const SESSION_REQUEST_OVERHEAD_MS = 3_000; const FORWARD_TIMEOUT_MS = 5_000; +// A helper that cannot start spends its whole start budget failing, and the one-shot transport that +// answers afterwards still has to run. Retrying that on the very next command is what made commands +// on the slow hosts of #2553 take roughly twice as long, so a failed start keeps the persistent path +// away for at least this long. +const SESSION_START_RETRY_FLOOR_MS = 10_000; +// …and for no longer than this, however long the start took. The floor keeps a burst of commands +// from re-paying an instant failure; the ceiling keeps a host whose helper is simply broken from +// being written off for longer than a working session would have lasted. +const SESSION_START_RETRY_CEILING_MS = 60_000; export type AndroidSnapshotHelperSessionHelperIdentity = { packageName: string; @@ -83,6 +93,8 @@ export type AndroidSnapshotHelperSessionAcquisition = { }; const sessions = new Map(); +/** Capture identity → when this process may spawn that helper build again after a failed start. */ +const failedStarts = new Map(); /** * Starts (or reuses) the session without capturing, so a helper-backed read that is not a snapshot @@ -137,40 +149,77 @@ async function resolveAndroidSnapshotHelperSession(params: { options: AndroidSnapshotHelperCaptureOptions; resolved: AndroidSnapshotHelperResolvedCaptureOptions; }): Promise { - const { deviceKey, identity, options, resolved } = params; + if (isAndroidSnapshotHelperStartBackedOff(params.identity)) return undefined; + await retireUnusableAndroidSnapshotHelperSession(params.deviceKey, params.identity); + return sessions.get(params.deviceKey) ?? (await tryStartAndroidSnapshotHelperSession(params)); +} + +/** Drops a cached session this command cannot write to, so only its forward is left behind. */ +async function retireUnusableAndroidSnapshotHelperSession( + deviceKey: string, + identity: string, +): Promise { const cached = sessions.get(deviceKey); - const reusable = cached !== undefined && isReusableAndroidSnapshotHelperSession(cached, identity); - if (cached && !reusable) { - await stopAndroidSnapshotHelperSession(deviceKey, { - // A process that already exited cannot answer the forwarded port: the forward is all that is - // left of it, so there is nothing to quit gracefully. - force: hasAndroidSnapshotHelperProcessEnded(cached.process), + if (!cached || isReusableAndroidSnapshotHelperSession(cached, identity)) return; + // A process that already exited cannot answer the forwarded port, so there is nothing left to ask + // it to quit gracefully. + await stopAndroidSnapshotHelperSession(deviceKey, { + force: hasAndroidSnapshotHelperProcessEnded(cached.process), + }); +} + +/** + * Starts the helper, or answers `undefined` for a start that failed. A start that failed is not a + * command that failed — the caller answers with the one-shot transport — and this helper build is + * not spawned again until the backoff it just earned is over. + */ +async function tryStartAndroidSnapshotHelperSession(params: { + deviceKey: string; + identity: string; + options: AndroidSnapshotHelperCaptureOptions; + resolved: AndroidSnapshotHelperResolvedCaptureOptions; +}): Promise { + const startedAtMs = Date.now(); + try { + return await startAndroidSnapshotHelperSession(params); + } catch (error) { + params.options.signal?.throwIfAborted(); + failedStarts.set( + params.identity, + Date.now() + androidSnapshotHelperStartRetryAfterMs(Date.now() - startedAtMs), + ); + emitDiagnostic({ + level: 'warn', + phase: 'android_snapshot_helper_session_start_failed', + data: { + deviceKey: params.deviceKey, + reason: error instanceof AppError ? error.details?.reason : undefined, + detail: error instanceof Error ? error.message : String(error), + }, }); + return undefined; } - let session = reusable ? cached : undefined; - if (!session) { - try { - session = await startAndroidSnapshotHelperSession({ - deviceKey, - identity, - options, - resolved, - }); - } catch (error) { - // A start that failed is not a command that failed: the caller answers with the one-shot - // transport and the next command tries the persistent path again. - options.signal?.throwIfAborted(); - emitDiagnostic({ - level: 'warn', - phase: 'android_snapshot_helper_session_start_failed', - data: { - deviceKey, - reason: error instanceof Error ? error.message : String(error), - }, - }); - } - } - return session; +} + +/** A helper build whose last start failed is left alone until that start's backoff has run out. */ +function isAndroidSnapshotHelperStartBackedOff(identity: string): boolean { + const retryAtMs = failedStarts.get(identity); + if (retryAtMs === undefined) return false; + if (retryAtMs > Date.now()) return true; + failedStarts.delete(identity); + return false; +} + +/** + * How long a failed start earns: as long as it spent failing, because a start that burned half a + * minute on a wedged device would burn another half minute on the next command, bounded so a burst + * of commands neither re-pays an instant failure nor writes a device off for the rest of the run. + */ +function androidSnapshotHelperStartRetryAfterMs(startDurationMs: number): number { + return Math.min( + Math.max(startDurationMs, SESSION_START_RETRY_FLOOR_MS), + SESSION_START_RETRY_CEILING_MS, + ); } /** @@ -238,6 +287,11 @@ async function startAndroidSnapshotHelperSession(params: { params.options.signal, ); sessions.set(params.deviceKey, session); + failedStarts.delete(params.identity); + // This helper holds the device's one UiAutomation connection now, so a release the previous + // teardown could not prove is settled by the device itself. Leaving it pending would have the + // next acquire force-stop the session that just started. + settleAndroidSnapshotHelperRetirement(params.deviceKey); emitDiagnostic({ phase: 'android_snapshot_helper_session_ready', data: { @@ -449,11 +503,18 @@ export async function stopAndroidSnapshotHelperSessionForDevice( } export async function resetAndroidSnapshotHelperSessions(): Promise { - await Promise.all( - [...sessions.keys()].map(async (deviceKey) => { - await stopAndroidSnapshotHelperSession(deviceKey); - }), - ); - resetAndroidSnapshotHelperRetirements(); - resetAndroidAdbShellProtocolProbes(); + try { + await Promise.allSettled( + [...sessions.keys()].map(async (deviceKey) => { + await stopAndroidSnapshotHelperSession(deviceKey); + }), + ); + } finally { + // One teardown that throws must not leave the next caller believing a session, a pending + // retirement, or a failed start is still standing. + sessions.clear(); + failedStarts.clear(); + resetAndroidSnapshotHelperRetirements(); + resetAndroidAdbShellProtocolProbes(); + } } From a4a888073c0d591859a8b08c1bf5f242944781d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 15:08:55 +0200 Subject: [PATCH 09/12] fix(android): give a helper start a share of the caller's own timeout --- CHANGELOG.md | 6 +++- .../snapshot-helper-session-lifecycle.test.ts | 11 +++++++ .../src/snapshot-helper-session-lifecycle.ts | 33 +++++++++++++++---- 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cd081b854..e30b4cfa5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,11 @@ `pidof` happens to be unreadable. A helper start that fails is also retried after a backoff scaled to how long it spent failing (10 s to 60 s) instead of on every command, which had roughly doubled command time on hosts where the helper never starts, and the wait for a started helper to announce - itself now uses the caller's own helper-command budget, so `--timeout` reaches it. + itself now takes a share of the caller's own helper-command budget — half of `--timeout`, never + less than one session command is worth — so a device that needs longer than a capture to bring the + helper up stays on the persistent path when the caller budgeted for it. On a host where the helper + took 12 s to announce itself, `--timeout 60000` used to answer with the one-shot transport and now + answers from the session. - Fixed: an iOS snapshot whose XCTest query-sweep tier cannot read the screen no longer ends the runner process. On a live React Native feed (Bluesky Home, images re-rendering) the AX server diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts index 89b9be989e..c2a33c8e6d 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts @@ -3,6 +3,7 @@ import { afterEach, beforeEach, test } from 'vitest'; import { captureAndroidSnapshotWithHelperSession } from '../snapshot-helper-session.ts'; import { resetAndroidSnapshotHelperSessions, + resolveAndroidSnapshotHelperStartBudgetMs, stopAndroidSnapshotHelperSession, } from '../snapshot-helper-session-lifecycle.ts'; import { recoverAndroidSnapshotHelperRetirement } from '../snapshot-helper-retirement.ts'; @@ -123,6 +124,16 @@ test('a session start waits only as long as the caller budgeted for one helper c ); }); +test('a generous caller budget buys a slow start, and never more than the caller allowed', () => { + // A capture-sized guess is what pushed the slow hosts of #2553 off the persistent path even when + // `--timeout` left plenty of room for the same start in the one-shot transport. + assert.equal(resolveAndroidSnapshotHelperStartBudgetMs(60_000), 30_000); + assert.equal(resolveAndroidSnapshotHelperStartBudgetMs(30_000), 15_000); + // A short budget buys nothing extra, and a tiny one is not answered with a longer wait. + assert.equal(resolveAndroidSnapshotHelperStartBudgetMs(6_000), 5_000); + assert.equal(resolveAndroidSnapshotHelperStartBudgetMs(1_000), 1_000); +}); + test('a session that reaches ready settles a release the device could not confirm', async () => { const calls: string[][] = []; const spawnArgs: string[][] = []; diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index a567b75ea8..4664e3b180 100644 --- a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts +++ b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts @@ -120,15 +120,15 @@ export async function acquireAndroidSnapshotHelperSession( if (!isAndroidSnapshotHelperSessionEnabled() || !options.adbProvider?.spawn) { return undefined; } - const resolved = resolvePersistentSessionCaptureOptions( - resolveAndroidSnapshotHelperCaptureOptions(options), - ); + const callerResolved = resolveAndroidSnapshotHelperCaptureOptions(options); + const resolved = resolvePersistentSessionCaptureOptions(callerResolved); const identity = createSessionIdentity(deviceKey, resolved, options); const session = await resolveAndroidSnapshotHelperSession({ deviceKey, identity, options, resolved, + startBudgetMs: resolveAndroidSnapshotHelperStartBudgetMs(callerResolved.commandTimeoutMs), }); return session ? { session, resolved, deviceKey } : undefined; } @@ -148,6 +148,7 @@ async function resolveAndroidSnapshotHelperSession(params: { identity: string; options: AndroidSnapshotHelperCaptureOptions; resolved: AndroidSnapshotHelperResolvedCaptureOptions; + startBudgetMs: number; }): Promise { if (isAndroidSnapshotHelperStartBackedOff(params.identity)) return undefined; await retireUnusableAndroidSnapshotHelperSession(params.deviceKey, params.identity); @@ -178,6 +179,7 @@ async function tryStartAndroidSnapshotHelperSession(params: { identity: string; options: AndroidSnapshotHelperCaptureOptions; resolved: AndroidSnapshotHelperResolvedCaptureOptions; + startBudgetMs: number; }): Promise { const startedAtMs = Date.now(); try { @@ -240,6 +242,7 @@ async function startAndroidSnapshotHelperSession(params: { identity: string; options: AndroidSnapshotHelperCaptureOptions; resolved: AndroidSnapshotHelperResolvedCaptureOptions; + startBudgetMs: number; }): Promise { const port = await allocateAndroidSnapshotHelperSessionPort(); await params.options.adb(['forward', `tcp:${port}`, `tcp:${port}`], { @@ -278,12 +281,12 @@ async function startAndroidSnapshotHelperSession(params: { capturedCount: 0, }; try { - // Starting the session gets the budget the caller already allowed one helper command, which is - // how `--timeout` reaches it. A fixed guess below that pushed a slow device out of the persistent - // path while the one-shot transport it fell back to had room for the same start. + // A helper that announces itself late is a slow `am instrument`, which the one-shot transport it + // falls back to pays too. The start gets its share of the caller's command budget instead of a + // fixed guess, so `--timeout` decides whether the persistent path is affordable at all. await waitForAndroidSnapshotHelperSessionReady( childProcess, - params.resolved.commandTimeoutMs, + params.startBudgetMs, params.options.signal, ); sessions.set(params.deviceKey, session); @@ -369,6 +372,22 @@ function resolvePersistentSessionCaptureOptions( }; } +/** + * What a start gets out of the budget the caller allowed one helper command: half of it, so a helper + * that announces itself later than a session capture takes is not pushed off the persistent path by + * a capture-sized guess, while the one-shot transport that answers a failed start keeps the other + * half. Never less than one session command is worth, never more than the caller allowed. + */ +export function resolveAndroidSnapshotHelperStartBudgetMs(commandTimeoutMs: number): number { + return Math.min( + commandTimeoutMs, + Math.max( + Math.floor(commandTimeoutMs / 2), + SESSION_CAPTURE_TIMEOUT_MS + SESSION_REQUEST_OVERHEAD_MS, + ), + ); +} + function isAndroidSnapshotHelperSessionEnabled(): boolean { const value = requireAndroidAdbHost().environment.AGENT_DEVICE_ANDROID_SNAPSHOT_HELPER_SESSION; return value === undefined || !/^(0|false|no|off)$/i.test(value); From a516dfcd30b78c01cdef694fbd78b65a8f992346 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 15:45:56 +0200 Subject: [PATCH 10/12] fix(android): read a release only from an answer, not from an adb that complained --- CHANGELOG.md | 35 +++++++++------- .../snapshot-helper-retirement.test.ts | 42 ++++++++++++++----- .../snapshot-helper-session-lifecycle.test.ts | 11 ++--- .../snapshot-helper-session.fixtures.ts | 13 ++++-- .../src/snapshot-helper-retirement.ts | 33 +++++++++------ .../src/snapshot-helper-session-lifecycle.ts | 22 ++++++---- 6 files changed, 100 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e30b4cfa5e..c5ad95e1f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,23 +11,26 @@ outlive its budget while the device is healthy — the shape of #2553 — so the helper process was already gone and the command still failed with `Android automation helper is still holding device automation ownership`. Ownership is read off the device now: `adb shell pidof - com.callstack.agentdevice.snapshothelper` says `occupied` only while it names a process, `released` - when the device answers that nothing is running, and `unknown` when adb could not carry the call, - so a `device offline` stderr no longer counts as a release either. A refusal requires two reads that - both name the process, which keeps a helper still inside Android's exit path from costing a - command. Scripts that match the failure reason see `android_snapshot_helper_runtime_occupied`, - which replaces `android_snapshot_helper_retirement_unconfirmed`. + com.callstack.agentdevice.snapshothelper` says `occupied` while it names a process, and says + `released` only on the shell's own no-process answer — a non-zero exit with nothing on either + stream. Anything else, including `error: closed`, `cannot connect to daemon` and `device offline`, + is `unknown`: those describe the transport, not who holds the runtime, and a release is never + cleared on a description of the transport. A refusal requires two reads that both name the process, + which keeps a helper still inside Android's exit path from costing a command. Scripts that match the + failure reason see `android_snapshot_helper_runtime_occupied`, which replaces + `android_snapshot_helper_retirement_unconfirmed`. - Changed (android): a snapshot helper session that reaches ready settles a release the previous - teardown could not prove, because Android hands UiAutomation to one connection at a time and that - helper owns it now; the next command no longer force-stops the session it has just started while - `pidof` happens to be unreadable. A helper start that fails is also retried after a backoff scaled - to how long it spent failing (10 s to 60 s) instead of on every command, which had roughly doubled - command time on hosts where the helper never starts, and the wait for a started helper to announce - itself now takes a share of the caller's own helper-command budget — half of `--timeout`, never - less than one session command is worth — so a device that needs longer than a capture to bring the - helper up stays on the persistent path when the caller budgeted for it. On a host where the helper - took 12 s to announce itself, `--timeout 60000` used to answer with the one-shot transport and now - answers from the session. + teardown could not prove. `am instrument` force-stops whatever is already instrumenting the helper + package, so a session that reported itself ready is the only helper process the device has left, + and the unproven release went away with the process that owed it; the next command no longer + force-stops the session it has just started because `pidof` happens to be unreadable. A helper start + that fails is also retried after a backoff scaled to how long it spent failing (10 s to 60 s) + instead of on every command, which had roughly doubled command time on hosts where the helper never + starts. And the wait for a started helper to announce itself no longer uses a fixed 10 s: it takes + half of the helper-command budget the capture was built with, 15 s today, which is what had been + pushing devices slower than a capture off the persistent path. On a host where the helper took 12 s + to announce itself, the command used to answer with the one-shot transport and now answers from the + session. The CLI's `--timeout` reaches that wait as its deadline aborting it, not as the number. - Fixed: an iOS snapshot whose XCTest query-sweep tier cannot read the screen no longer ends the runner process. On a live React Native feed (Bluesky Home, images re-rendering) the AX server diff --git a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts index 163c0473ea..75515f64a7 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts @@ -10,7 +10,7 @@ import { retireCanceledAndroidSnapshotHelperCapture, settleAndroidSnapshotHelperSessionCleanup, } from '../snapshot-helper-retirement.ts'; -import type { AndroidAdbProcess } from '../adb-executor.ts'; +import type { AndroidAdbExecutorResult, AndroidAdbProcess } from '../adb-executor.ts'; import type { AndroidAdbExecutor } from '../snapshot-helper-types.ts'; import { androidHelperRuntimeProbeResult, @@ -126,15 +126,37 @@ test('a device that cannot be read leaves the retirement pending without failing ]); }); -test('a shell that has no pidof still answers for its own processes', async () => { - // An older device image reports the missing command on stderr and exits non-zero. That is the - // device answering, not a transport fault, and reading it as `unknown` would keep every later - // acquire force-stopping a runtime that was never held. - const adb: AndroidAdbExecutor = async () => ({ - exitCode: 1, - stdout: '', - stderr: '/system/bin/sh: pidof: not found', - }); +test('a read that names no process is released only when the shell itself said so', async () => { + // `pidof` answers "no such process" with a non-zero exit and nothing on either stream. Every other + // shape is adb or the shell describing itself, and a description of the transport cannot clear a + // pending release. Enumerating the ways a transport fails is not a fix either: that list is long, + // version-dependent, and includes plain `error: closed` and `cannot connect to daemon`. + const nonAnswers: AndroidAdbExecutorResult[] = [ + { exitCode: 1, stdout: '', stderr: 'error: closed' }, + { exitCode: 1, stdout: '', stderr: 'error: device offline' }, + { exitCode: 1, stdout: '', stderr: 'adb: cannot connect to daemon' }, + { exitCode: 1, stdout: '', stderr: 'failed to get feature set: device offline' }, + { exitCode: 1, stdout: '/system/bin/sh: pidof: not found', stderr: '' }, + { exitCode: 0, stdout: '', stderr: '' }, + ]; + + for (const answer of nonAnswers) { + resetAndroidSnapshotHelperRetirements(); + const adb: AndroidAdbExecutor = async () => answer; + + const release = await recordAndroidSnapshotHelperRelease({ + deviceKey: DEVICE_KEY, + packageName: PACKAGE_NAME, + adb, + cause: new Error('quit timed out'), + }); + + assert.equal(release, 'unknown', `answered ${JSON.stringify(answer)}`); + } +}); + +test('a device that answers with nothing at all is read as released', async () => { + const adb: AndroidAdbExecutor = async () => ({ exitCode: 1, stdout: '', stderr: '' }); const release = await recordAndroidSnapshotHelperRelease({ deviceKey: DEVICE_KEY, diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts index c2a33c8e6d..17794b8b3c 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session-lifecycle.test.ts @@ -137,13 +137,13 @@ test('a generous caller budget buys a slow start, and never more than the caller test('a session that reaches ready settles a release the device could not confirm', async () => { const calls: string[][] = []; const spawnArgs: string[][] = []; - // The device cannot be read at all, and the first command's session stalls, so that command's - // teardown records a release nothing could prove. + // The device answers every process read with an adb error no classifier lists, and the first + // command's session stalls, so that command's teardown records a release nothing could prove. const provider = createSessionProvider({ calls, spawnArgs, stalledSnapshots: 1, - runtimeRelease: 'unreadable', + runtimeRelease: 'closed', }); const stalled = await captureAndroidSnapshotWithHelperSession({ @@ -163,8 +163,9 @@ test('a session that reaches ready settles a release the device could not confir assert.equal(started?.metadata.sessionReused, false); const forceStopsWhilePending = calls.filter(isAndroidHelperRuntimeForceStop).length; - // Android hands UiAutomation to one connection, so the helper that just reported itself ready owns - // the runtime and the unreadable device has nothing left to hold the next command with. + // `am instrument` force-stops whatever is already instrumenting the helper package, so the helper + // that just reported itself ready is the only one the device has, and the pending release went + // away with the process that owed it. const reused = await captureAndroidSnapshotWithHelperSession({ adb: provider.exec, adbProvider: provider, diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts index 81d3fffc17..3029099432 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts @@ -164,10 +164,14 @@ export type SessionProviderOptions = { runtimeRelease?: FakeAndroidHelperRuntimeRelease; }; -/** What a fake device says about the helper process, including a device that cannot be read. */ +/** + * What a fake device answers about the helper process. `unreadable` is a transport fault adb's own + * failure classifier recognises and `closed` is one it does not; the probe has to fail closed on both. + */ export type FakeAndroidHelperRuntimeRelease = | Exclude - | 'unreadable'; + | 'unreadable' + | 'closed'; export function createSessionProvider(options: SessionProviderOptions): AndroidAdbProvider { bindAndroidAdbTestHost(); @@ -297,9 +301,10 @@ export function androidHelperRuntimeProbeResult( release: FakeAndroidHelperRuntimeRelease = 'released', ): AndroidAdbExecutorResult { // A host whose adb cannot carry the call answers the way the executor really answers it: a non-zero - // exit with a transport fault on stderr, which is a different shape from a device that says "no - // such process" only by what it prints. + // exit, empty stdout and a fault on stderr. The shell's own "no such process" is that same shape + // with nothing at all on stderr, which is the only non-pid answer that means released. if (release === 'unreadable') return { exitCode: 1, stdout: '', stderr: 'error: device offline' }; + if (release === 'closed') return { exitCode: 1, stdout: '', stderr: 'error: closed' }; return release === 'occupied' ? { exitCode: 0, stdout: '4211\n', stderr: '' } : { exitCode: 1, stdout: '', stderr: '' }; diff --git a/packages/platform-android/src/snapshot-helper-retirement.ts b/packages/platform-android/src/snapshot-helper-retirement.ts index 98a66d2692..72d22ce314 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -2,7 +2,6 @@ import { AppError } from '@agent-device/kernel/errors'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { emitDiagnostic } from '@agent-device/host-kit/diagnostics'; import { sleep } from '@agent-device/host-kit/retry'; -import { classifyAndroidAdbFailure } from './adb-failure.ts'; import { findPidToken } from './perf-native-process.ts'; import type { AndroidAdbProcess } from './adb-executor.ts'; import type { AndroidAdbExecutor } from './snapshot-helper-types.ts'; @@ -179,13 +178,19 @@ async function readAndroidSnapshotHelperRuntimeRelease(params: { allowFailure: true, timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, }); + const stdout = result.stdout.trim(); + const stderr = result.stderr.trim(); // A process id for the helper package is the device naming whoever owns the runtime. - if (findPidToken(result.stdout)) return 'occupied'; - // `pidof` prints nothing for "no such process", and adb prints nothing useful on stdout when the - // call never reached a device. On the transport this probe exists for, that shape is common: a - // stderr the adb failure classifier recognises as a device or transport fault is no answer at - // all, while an unclassified one (an older shell without `pidof`) is the device's own. - return classifyAndroidAdbFailure(result.stderr, result.stdout) ? 'unknown' : 'released'; + if (findPidToken(stdout)) return 'occupied'; + // `pidof` answers "no process" by exiting non-zero with nothing on either stream. Every other + // shape — a line of stderr, a zero exit that names nobody, output without a pid — is adb or the + // shell describing itself, and a transport describing itself says nothing about the runtime. + // Enumerating adb's failure texts is not an option either: the list is long, version-dependent + // and includes plain `error: closed`, and every missed entry would clear a pending release that + // was never proven. + return result.exitCode !== 0 && stdout.length === 0 && stderr.length === 0 + ? 'released' + : 'unknown'; } catch { return 'unknown'; } @@ -306,11 +311,15 @@ export async function stopAndroidSnapshotHelperHostProcess(params: { } /** - * Settles a release the last teardown could not prove, from the other end of the device. Android - * hands UiAutomation to one connection at a time, so a helper that has just reported itself ready - * owns it now and whatever held it before no longer does. The session lifecycle calls this on the - * way to ready: an unreadable `pidof` must not leave a pending entry that force-stops a live helper - * on the next acquire. + * Settles a release the last teardown could not prove, from the other end of the device. The fact + * that settles it is Android's, not ours: `am instrument` for a package that is already instrumenting + * force-stops that process first, so a helper that reached ready — which it reports straight after + * binding its session socket, before it asks for UiAutomation — is the only helper process the device + * still has. Whatever the old process held is gone with it. The session lifecycle calls this on the + * way to ready; an unreadable `pidof` must not leave a pending entry that force-stops a live helper + * on the next acquire. Should the helper ever start sharing its package with another instrumentation + * target, or report readiness after acquiring UiAutomation instead of before, this settles on a + * process that may not be the only one, and the pending entry has to stay. */ export function settleAndroidSnapshotHelperRetirement(deviceKey: string): void { pendingRetirements.delete(deviceKey); diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index 4664e3b180..e7f562907f 100644 --- a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts +++ b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts @@ -282,8 +282,9 @@ async function startAndroidSnapshotHelperSession(params: { }; try { // A helper that announces itself late is a slow `am instrument`, which the one-shot transport it - // falls back to pays too. The start gets its share of the caller's command budget instead of a - // fixed guess, so `--timeout` decides whether the persistent path is affordable at all. + // falls back to pays too, so the wait gets a share of the helper-command budget rather than a + // smaller guess. The caller's own deadline reaches it as an abort on `options.signal`, which is + // what bounds this below the budget when the command itself is short. await waitForAndroidSnapshotHelperSessionReady( childProcess, params.startBudgetMs, @@ -291,9 +292,10 @@ async function startAndroidSnapshotHelperSession(params: { ); sessions.set(params.deviceKey, session); failedStarts.delete(params.identity); - // This helper holds the device's one UiAutomation connection now, so a release the previous - // teardown could not prove is settled by the device itself. Leaving it pending would have the - // next acquire force-stop the session that just started. + // `am instrument` force-stops whatever is already instrumenting this package, so a helper that + // reported itself ready is the only helper process the device has left, and the release the + // previous teardown could not prove went away with the process that owed it. Leaving the entry + // pending would have the next acquire force-stop the session that just started. settleAndroidSnapshotHelperRetirement(params.deviceKey); emitDiagnostic({ phase: 'android_snapshot_helper_session_ready', @@ -373,10 +375,12 @@ function resolvePersistentSessionCaptureOptions( } /** - * What a start gets out of the budget the caller allowed one helper command: half of it, so a helper - * that announces itself later than a session capture takes is not pushed off the persistent path by - * a capture-sized guess, while the one-shot transport that answers a failed start keeps the other - * half. Never less than one session command is worth, never more than the caller allowed. + * What a start gets out of the helper-command budget it was built with: half of it, so a helper that + * announces itself later than a session capture takes is not pushed off the persistent path by a + * capture-sized guess, while the one-shot transport that answers a failed start keeps the other half. + * Never less than one session command is worth, never more than the budget. Production builds that + * budget from `ANDROID_SNAPSHOT_HELPER_COMMAND_TIMEOUT_MS` (30 s today, so 15 s here) rather than + * from the CLI's `--timeout`, whose deadline reaches this wait as an abort instead. */ export function resolveAndroidSnapshotHelperStartBudgetMs(commandTimeoutMs: number): number { return Math.min( From 7113e97204b3b3854deb012e61053269daf0bc41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 17:36:51 +0200 Subject: [PATCH 11/12] fix(android): let the device echo the release it is asked about --- CHANGELOG.md | 18 +++-- .../snapshot-helper-retirement.test.ts | 79 +++++++++++++------ .../snapshot-helper-session.fixtures.ts | 32 +++++--- .../src/snapshot-helper-retirement.ts | 37 ++++++--- 4 files changed, 111 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5ad95e1f5..18122b929c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,14 +10,16 @@ `am force-stop` as the fact it was supposed to measure, and on a loaded host that round trip can outlive its budget while the device is healthy — the shape of #2553 — so the helper process was already gone and the command still failed with `Android automation helper is still holding device - automation ownership`. Ownership is read off the device now: `adb shell pidof - com.callstack.agentdevice.snapshothelper` says `occupied` while it names a process, and says - `released` only on the shell's own no-process answer — a non-zero exit with nothing on either - stream. Anything else, including `error: closed`, `cannot connect to daemon` and `device offline`, - is `unknown`: those describe the transport, not who holds the runtime, and a release is never - cleared on a description of the transport. A refusal requires two reads that both name the process, - which keeps a helper still inside Android's exit path from costing a command. Scripts that match the - failure reason see `android_snapshot_helper_runtime_occupied`, which replaces + automation ownership`. Ownership is read off the device now: the probe asks the device shell for + `pidof com.callstack.agentdevice.snapshothelper` and tells it to echo a marker when nothing matched. + A process id is `occupied`, the bare marker is `released`, and everything else is `unknown` — + `error: closed`, `cannot connect to daemon`, `device offline`, or an adb client killed by a signal + before it wrote anything. A release is therefore claimed only by an answer the transport cannot + produce about itself, and no exit status is trusted: `adb shell` answers 0 for a device command that + failed, and an adb that dies by signal leaves the executor inventing an exit code it never saw. A + refusal requires two reads that both name the process, which keeps a helper still inside Android's + exit path from costing a command. Scripts that match the failure reason see + `android_snapshot_helper_runtime_occupied`, which replaces `android_snapshot_helper_retirement_unconfirmed`. - Changed (android): a snapshot helper session that reaches ready settles a release the previous teardown could not prove. `am instrument` force-stops whatever is already instrumenting the helper diff --git a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts index 75515f64a7..d7f9528825 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-retirement.test.ts @@ -3,6 +3,7 @@ import { beforeEach, test } from 'vitest'; import { EventEmitter } from 'node:events'; import { PassThrough } from 'node:stream'; import { + ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER, isAndroidSnapshotHelperRuntimeOccupiedError, recoverAndroidSnapshotHelperRetirement, recordAndroidSnapshotHelperRelease, @@ -19,6 +20,16 @@ import { const PACKAGE_NAME = 'com.callstack.agentdevice.snapshothelper'; const DEVICE_KEY = 'android:emulator-5554'; +// The device is asked for the helper process and told to echo a marker when there is none, so a +// release can only come from a shell that ran the command. +const RUNTIME_PROBE_CALL = [ + 'shell', + 'pidof', + PACKAGE_NAME, + '||', + 'echo', + ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER, +]; beforeEach(() => { resetAndroidSnapshotHelperRetirements(); @@ -29,7 +40,8 @@ test('canceled capture answers for the device, not for the force-stop call that const adb: AndroidAdbExecutor = async (args) => { calls.push(args); if (args.includes('force-stop')) throw new Error('adb round trip exceeded its budget'); - return { exitCode: 1, stdout: '', stderr: '' }; + if (isAndroidHelperRuntimeProbe(args)) return androidHelperRuntimeProbeResult('released'); + return { exitCode: 0, stdout: '', stderr: '' }; }; // A loaded host makes the stop call time out while the helper process is already gone. Ownership @@ -41,10 +53,7 @@ test('canceled capture answers for the device, not for the force-stop call that cause: new Error('capture canceled'), }); - assert.deepEqual(calls, [ - ['shell', 'am', 'force-stop', PACKAGE_NAME], - ['shell', 'pidof', PACKAGE_NAME], - ]); + assert.deepEqual(calls, [['shell', 'am', 'force-stop', PACKAGE_NAME], RUNTIME_PROBE_CALL]); await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); assert.equal(calls.length, 2); }); @@ -66,10 +75,7 @@ test('unproven release stays pending until an acquire reads the device', async ( adb, cause: new Error('capture canceled'), }); - assert.deepEqual(calls, [ - ['shell', 'am', 'force-stop', PACKAGE_NAME], - ['shell', 'pidof', PACKAGE_NAME], - ]); + assert.deepEqual(calls, [['shell', 'am', 'force-stop', PACKAGE_NAME], RUNTIME_PROBE_CALL]); await assert.rejects( recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }), @@ -79,15 +85,15 @@ test('unproven release stays pending until an acquire reads the device', async ( // and two reads that both name the process. assert.deepEqual(calls.slice(2), [ ['shell', 'am', 'force-stop', PACKAGE_NAME], - ['shell', 'pidof', PACKAGE_NAME], - ['shell', 'pidof', PACKAGE_NAME], + RUNTIME_PROBE_CALL, + RUNTIME_PROBE_CALL, ]); helperAlive = false; await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); assert.deepEqual(calls.slice(5), [ ['shell', 'am', 'force-stop', PACKAGE_NAME], - ['shell', 'pidof', PACKAGE_NAME], + RUNTIME_PROBE_CALL, ]); // The release is proven, so the entry is gone and a further acquire has nothing to settle. @@ -117,20 +123,20 @@ test('a device that cannot be read leaves the retirement pending without failing await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); assert.deepEqual(calls.slice(1), [ ['shell', 'am', 'force-stop', PACKAGE_NAME], - ['shell', 'pidof', PACKAGE_NAME], + RUNTIME_PROBE_CALL, ]); await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); assert.deepEqual(calls.slice(3), [ ['shell', 'am', 'force-stop', PACKAGE_NAME], - ['shell', 'pidof', PACKAGE_NAME], + RUNTIME_PROBE_CALL, ]); }); -test('a read that names no process is released only when the shell itself said so', async () => { - // `pidof` answers "no such process" with a non-zero exit and nothing on either stream. Every other - // shape is adb or the shell describing itself, and a description of the transport cannot clear a - // pending release. Enumerating the ways a transport fails is not a fix either: that list is long, - // version-dependent, and includes plain `error: closed` and `cannot connect to daemon`. +test('a read that names no process is released only when the device shell echoed the answer', async () => { + // A release is the device shell echoing the marker and nothing else. Every shape below is adb or the + // shell describing itself, and a description of the transport cannot clear a pending release. + // Enumerating the ways a transport fails is not a fix either: that list is long, version-dependent, + // and includes plain `error: closed` and `cannot connect to daemon`. const nonAnswers: AndroidAdbExecutorResult[] = [ { exitCode: 1, stdout: '', stderr: 'error: closed' }, { exitCode: 1, stdout: '', stderr: 'error: device offline' }, @@ -138,6 +144,12 @@ test('a read that names no process is released only when the shell itself said s { exitCode: 1, stdout: '', stderr: 'failed to get feature set: device offline' }, { exitCode: 1, stdout: '/system/bin/sh: pidof: not found', stderr: '' }, { exitCode: 0, stdout: '', stderr: '' }, + { + exitCode: 0, + stdout: ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER, + stderr: '/system/bin/sh: pidof: not found', + }, + { exitCode: 0, stdout: `${ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER} trailing`, stderr: '' }, ]; for (const answer of nonAnswers) { @@ -155,17 +167,40 @@ test('a read that names no process is released only when the shell itself said s } }); -test('a device that answers with nothing at all is read as released', async () => { - const adb: AndroidAdbExecutor = async () => ({ exitCode: 1, stdout: '', stderr: '' }); +test('an adb killed before it answers is not a release, whatever exit code it left behind', async () => { + // A client killed by a signal — `pkill adb`, a host OOM kill, a concurrent `adb kill-server` — dies + // without writing anything, and the executor reports an exit code it invented for the missing one. + // Nothing on a stream is the transport saying it never reached the device, which is not evidence + // that the helper is gone; clearing here would let the next acquire skip its force-stop while the + // old helper still runs, and start a second instrumentation beside it. + const release = await recordAndroidSnapshotHelperRelease({ + deviceKey: DEVICE_KEY, + packageName: PACKAGE_NAME, + adb: async () => androidHelperRuntimeProbeResult('signalled'), + cause: new Error('quit timed out'), + }); + + assert.equal(release, 'unknown'); +}); +test('a device that echoes the answer is read as released', async () => { + const calls: string[][] = []; const release = await recordAndroidSnapshotHelperRelease({ deviceKey: DEVICE_KEY, packageName: PACKAGE_NAME, - adb, + adb: async (args) => { + calls.push(args); + return androidHelperRuntimeProbeResult('released'); + }, cause: new Error('quit timed out'), }); assert.equal(release, 'released'); + await recoverAndroidSnapshotHelperRetirement({ + deviceKey: DEVICE_KEY, + adb: async () => ({ exitCode: 0, stdout: '', stderr: '' }), + }); + assert.deepEqual(calls, [RUNTIME_PROBE_CALL]); }); test('session cleanup stops the runtime even when the transport refuses the stop', async () => { diff --git a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts index 3029099432..f483e56c6a 100644 --- a/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts +++ b/packages/platform-android/src/__tests__/snapshot-helper-session.fixtures.ts @@ -16,6 +16,7 @@ import type { AndroidAdbProcess, AndroidAdbProvider, } from '../adb-executor.ts'; +import { ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER } from '../snapshot-helper-retirement.ts'; import type { AndroidSnapshotHelperRuntimeRelease } from '../snapshot-helper-retirement.ts'; import type { AndroidAdbExecutor } from '../snapshot-helper-types.ts'; import { bindAndroidAdbTestHost } from './test-utils/android-host-test-setup.ts'; @@ -165,13 +166,16 @@ export type SessionProviderOptions = { }; /** - * What a fake device answers about the helper process. `unreadable` is a transport fault adb's own - * failure classifier recognises and `closed` is one it does not; the probe has to fail closed on both. + * What a fake device answers about the helper process. `unreadable` and `closed` are transport faults + * adb puts on stderr, one its own failure classifier recognises and one it does not; `signalled` is + * an adb client killed before it wrote anything, which the executor reports as an invented exit code + * and empty streams. The probe has to fail closed on all three. */ export type FakeAndroidHelperRuntimeRelease = | Exclude | 'unreadable' - | 'closed'; + | 'closed' + | 'signalled'; export function createSessionProvider(options: SessionProviderOptions): AndroidAdbProvider { bindAndroidAdbTestHost(); @@ -300,14 +304,18 @@ export function isAndroidHelperRuntimeProbe(args: readonly string[]): boolean { export function androidHelperRuntimeProbeResult( release: FakeAndroidHelperRuntimeRelease = 'released', ): AndroidAdbExecutorResult { - // A host whose adb cannot carry the call answers the way the executor really answers it: a non-zero - // exit, empty stdout and a fault on stderr. The shell's own "no such process" is that same shape - // with nothing at all on stderr, which is the only non-pid answer that means released. - if (release === 'unreadable') return { exitCode: 1, stdout: '', stderr: 'error: device offline' }; - if (release === 'closed') return { exitCode: 1, stdout: '', stderr: 'error: closed' }; - return release === 'occupied' - ? { exitCode: 0, stdout: '4211\n', stderr: '' } - : { exitCode: 1, stdout: '', stderr: '' }; + switch (release) { + case 'occupied': + return { exitCode: 0, stdout: '4211\n', stderr: '' }; + case 'released': + return { exitCode: 0, stdout: `${ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER}\n`, stderr: '' }; + case 'unreadable': + return { exitCode: 1, stdout: '', stderr: 'error: device offline' }; + case 'closed': + return { exitCode: 1, stdout: '', stderr: 'error: closed' }; + case 'signalled': + return { exitCode: 1, stdout: '', stderr: '' }; + } } function adbFeaturesResult(options: SessionProviderOptions): { @@ -454,7 +462,7 @@ function persistentSnapshotExecResult( return Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }); } if (isAndroidHelperRuntimeProbe(args)) { - return Promise.resolve({ exitCode: 1, stdout: '', stderr: '' }); + return Promise.resolve(androidHelperRuntimeProbeResult('released')); } if (args.includes('instrument')) { options.oneShotAttempts?.push(args); diff --git a/packages/platform-android/src/snapshot-helper-retirement.ts b/packages/platform-android/src/snapshot-helper-retirement.ts index 72d22ce314..55c8159370 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -17,6 +17,11 @@ const RUNTIME_OCCUPANCY_RECHECK_MS = 250; export const ANDROID_SNAPSHOT_HELPER_HOST_PROCESS_EXIT_GRACE_MS = 250; export const ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS = 2_000; const RUNTIME_OCCUPIED_REASON = 'android_snapshot_helper_runtime_occupied'; +/** + * Printed by the device's own shell, and only by it, when the helper process is not running. + * Exported so a fake device can answer with the real thing. + */ +export const ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER = 'AGENT_DEVICE_NO_HELPER'; /** * Whether anything on the device still owns UiAutomation through the helper runtime. `unknown` is @@ -174,21 +179,27 @@ async function readAndroidSnapshotHelperRuntimeRelease(params: { packageName: string; }): Promise { try { - const result = await params.adb(['shell', 'pidof', params.packageName], { - allowFailure: true, - timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, - }); + // The marker comes from the device shell, and only from it, so a release is claimed by an answer + // the transport cannot forge: an adb client that ran out of budget, was killed by a signal, or + // lost the connection prints nothing at all. No exit status is consulted either, because `adb + // shell` answers 0 for a device command that failed and the executor has to invent one when the + // client dies before reporting one. + const result = await params.adb( + [ + 'shell', + 'pidof', + params.packageName, + '||', + 'echo', + ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER, + ], + { allowFailure: true, timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS }, + ); const stdout = result.stdout.trim(); - const stderr = result.stderr.trim(); - // A process id for the helper package is the device naming whoever owns the runtime. if (findPidToken(stdout)) return 'occupied'; - // `pidof` answers "no process" by exiting non-zero with nothing on either stream. Every other - // shape — a line of stderr, a zero exit that names nobody, output without a pid — is adb or the - // shell describing itself, and a transport describing itself says nothing about the runtime. - // Enumerating adb's failure texts is not an option either: the list is long, version-dependent - // and includes plain `error: closed`, and every missed entry would clear a pending release that - // was never proven. - return result.exitCode !== 0 && stdout.length === 0 && stderr.length === 0 + // A shell with no `pidof` prints the marker too, after it complains, so the answer counts only + // from a shell that had nothing to say about the command it ran. + return stdout === ANDROID_SNAPSHOT_HELPER_NO_HELPER_ANSWER && result.stderr.trim().length === 0 ? 'released' : 'unknown'; } catch { From 788944f2456b6dd6c606ac5143ac67a111803c4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 17:36:52 +0200 Subject: [PATCH 12/12] docs(android): name the instrumentation takeover that retires the previous helper --- .../src/__tests__/fill-verification.test.ts | 9 +++++---- .../src/snapshot-helper-session-lifecycle.ts | 8 ++++---- .../platform-android/src/snapshot-helper-session.ts | 5 +++-- .../platform-android/src/snapshot-helper-types.ts | 12 ++++++------ 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/packages/platform-android/src/__tests__/fill-verification.test.ts b/packages/platform-android/src/__tests__/fill-verification.test.ts index d832569277..1771815fe7 100644 --- a/packages/platform-android/src/__tests__/fill-verification.test.ts +++ b/packages/platform-android/src/__tests__/fill-verification.test.ts @@ -1,8 +1,9 @@ // Fill reads the live hierarchy four times per attempt (one pre-action target read plus the -// 0/150/350 ms settling samples). Android permits ONE UiAutomation owner, so a command-scoped -// capture stops the automation-helper session after every one of those reads and the next read -// pays a fresh `am instrument` start. These tests pin who owns the helper session across the -// samples — not what the samples conclude, which fill-diagnostics/input-actions-fill own. +// 0/150/350 ms settling samples). `am instrument` force-stops whatever is already instrumenting the +// helper package, so a command-scoped capture stops the automation-helper session after every one of +// those reads and the next read pays a fresh `am instrument` start. These tests pin who owns the +// helper session across the samples — not what the samples conclude, which +// fill-diagnostics/input-actions-fill own. import { afterEach, beforeEach, test } from 'vitest'; import './test-utils/android-host-test-setup.ts'; diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index e7f562907f..6a70ac1ebe 100644 --- a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts +++ b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts @@ -1,10 +1,10 @@ /** * Who owns the device's UiAutomation right now, and how that ownership starts and ends. * - * Android permits ONE UiAutomation owner, so a live helper session is device-exclusive state: this - * module is the only place that starts one, hands it out, and retires it. Commands run OVER a - * session (snapshot capture, gestures) live in `snapshot-helper-session.ts`; they acquire through - * here and never reach the registry themselves. + * `am instrument` force-stops whatever is already instrumenting the helper package, so a live helper + * session is device-exclusive state: this module is the only place that starts one, hands it out, and + * retires it. Commands run OVER a session (snapshot capture, gestures) live in + * `snapshot-helper-session.ts`; they acquire through here and never reach the registry themselves. */ import type { AndroidAdbProcess } from './adb-executor.ts'; import { requireAndroidAdbHost } from './adb-host.ts'; diff --git a/packages/platform-android/src/snapshot-helper-session.ts b/packages/platform-android/src/snapshot-helper-session.ts index 4014835d5d..3b05cb5176 100644 --- a/packages/platform-android/src/snapshot-helper-session.ts +++ b/packages/platform-android/src/snapshot-helper-session.ts @@ -86,8 +86,9 @@ async function captureFromAndroidSnapshotHelperSession(params: { } // Touch commands piggyback on a live snapshot session so gestures do not restart instrumentation -// (Android permits one UiAutomation owner). They never start a session: without one, callers use -// the same helper APK through a one-shot `am instrument` run instead. +// (a second instrumentation for the helper package would force-stop the session that has it). They +// never start a session: without one, callers use the same helper APK through a one-shot +// `am instrument` run instead. export async function runAndroidSnapshotHelperSessionTouchCommand(params: { deviceKey: string; action: 'gesture' | 'viewport'; diff --git a/packages/platform-android/src/snapshot-helper-types.ts b/packages/platform-android/src/snapshot-helper-types.ts index 7ca8b74558..cee1b6bf6d 100644 --- a/packages/platform-android/src/snapshot-helper-types.ts +++ b/packages/platform-android/src/snapshot-helper-types.ts @@ -26,12 +26,12 @@ export const ANDROID_SNAPSHOT_HELPER_COMMAND_TIMEOUT_MS = 30_000; /** * Who releases the helper's persistent instrumentation session. * - * Android permits ONE UiAutomation owner, so a `command`-scoped call stops the session when it - * finishes and the next helper call pays a fresh `am instrument` start plus the UiAutomation - * connect wait. `daemon-session` hands that release to session teardown - * (`stopSessionAndroidSnapshotHelper`), which every Android session runs, so consecutive commands - * in one session share one warm helper. Device-scoped work stays `command` so nothing squats - * UiAutomation once the command returns. + * `am instrument` force-stops whatever is already instrumenting the helper package, so a + * `command`-scoped call stops the session when it finishes and the next helper call pays a fresh + * `am instrument` start plus the UiAutomation connect wait. `daemon-session` hands that release to + * session teardown (`stopSessionAndroidSnapshotHelper`), which every Android session runs, so + * consecutive commands in one session share one warm helper. Device-scoped work stays `command` so + * nothing squats UiAutomation once the command returns. */ export type AndroidHelperSessionScope = 'command' | 'daemon-session';