diff --git a/CHANGELOG.md b/CHANGELOG.md index 4307468822..18122b929c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,35 @@ - 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: 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 + 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 rejects each of the sweep's 19 element-type queries with `kAXErrorIllegalArgument`, and XCTest 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/__tests__/snapshot-helper-capture.test.ts b/packages/platform-android/src/__tests__/snapshot-helper-capture.test.ts index f6927689c5..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,14 +35,15 @@ test('one-shot capture that resolves during cancellation retires before rejectin if (options?.signal?.aborted) onAbort(); }); } + if (isAndroidHelperRuntimeProbe(args)) return androidHelperRuntimeProbeResult(); 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 +75,24 @@ 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 (isAndroidHelperRuntimeProbe(args)) { + // The first read happens while Android still runs the helper; the next says it is gone. + events.push('pidof'); + return androidHelperRuntimeProbeResult(stopCount === 1 ? 'occupied' : 'released'); } - 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..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,77 +3,221 @@ 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, resetAndroidSnapshotHelperRetirements, 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, + isAndroidHelperRuntimeProbe, +} from './snapshot-helper-session.fixtures.ts'; + +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(); }); -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'); + if (isAndroidHelperRuntimeProbe(args)) return androidHelperRuntimeProbeResult('released'); + return { exitCode: 0, 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], RUNTIME_PROBE_CALL]); + 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 calls: string[][] = []; + const adb: AndroidAdbExecutor = async (args) => { + calls.push(args); + if (isAndroidHelperRuntimeProbe(args)) { + return androidHelperRuntimeProbeResult(helperAlive ? 'occupied' : 'released'); + } + return { exitCode: 0, stdout: '', stderr: '' }; + }; + + await retireCanceledAndroidSnapshotHelperCapture({ + deviceKey: DEVICE_KEY, + packageName: PACKAGE_NAME, adb, + cause: new Error('capture canceled'), }); + assert.deepEqual(calls, [['shell', 'am', 'force-stop', PACKAGE_NAME], RUNTIME_PROBE_CALL]); - 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, ); + // 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], + 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], + RUNTIME_PROBE_CALL, + ]); + + // 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('session cleanup force-stops the runtime when release was not confirmed', async () => { +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: '' }; + }; + + const release = await recordAndroidSnapshotHelperRelease({ + deviceKey: DEVICE_KEY, + packageName: PACKAGE_NAME, + adb, + 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], + RUNTIME_PROBE_CALL, + ]); + await recoverAndroidSnapshotHelperRetirement({ deviceKey: DEVICE_KEY, adb }); + assert.deepEqual(calls.slice(3), [ + ['shell', 'am', 'force-stop', PACKAGE_NAME], + RUNTIME_PROBE_CALL, + ]); +}); + +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' }, + { 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: '' }, + { + 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) { + 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('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: 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 () => { 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 +228,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..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 @@ -3,12 +3,14 @@ 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'; import { createSessionProvider, FakeAndroidProcess, + isAndroidHelperRuntimeForceStop, type SessionProviderOptions, } from './snapshot-helper-session.fixtures.ts'; import type { AndroidAdbExecutor, AndroidAdbProvider } from '../adb-executor.ts'; @@ -50,38 +52,133 @@ 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 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({ + 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(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, + 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 () => { + 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('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[][] = []; + // 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: 'closed', + }); + + const stalled = await captureAndroidSnapshotWithHelperSession({ adb: provider.exec, adbProvider: provider, deviceKey: 'android:emulator-5554', + commandTimeoutMs: 400, }); - const second = await captureAndroidSnapshotWithHelperSession({ + 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; - 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); + // `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, + 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 () => { @@ -144,6 +241,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[] = []; @@ -273,11 +397,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,31 +411,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 isHelperRuntimeForceStop(args: string[]): boolean { - return args.join(' ') === 'shell am force-stop com.callstack.agentdevice.snapshothelper'; +function countForceStops(options: SessionProviderOptions): number { + return options.calls.filter((args) => isHelperRuntimeForceStop(args)).length; } -function readSessionArgument(args: string[], name: string): string | undefined { - const index = args.indexOf(name); - return index < 0 ? undefined : args[index + 1]; +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..f483e56c6a 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,13 @@ 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 { 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'; @@ -54,6 +60,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,8 +161,22 @@ 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?: FakeAndroidHelperRuntimeRelease; }; +/** + * 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' + | 'signalled'; + export function createSessionProvider(options: SessionProviderOptions): AndroidAdbProvider { bindAndroidAdbTestHost(); let stalledSnapshots = options.stalledSnapshots ?? 0; @@ -262,6 +284,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 androidHelperRuntimeProbeResult(options.runtimeRelease); const forceStopsRuntime = args.join(' ').includes('am force-stop'); await stallSessionCleanupIfConfigured(options, args, execOptions?.signal, forceStopsRuntime); if (options.recoveryFailure && forceStopsRuntime) { @@ -272,6 +296,28 @@ function createSessionExec(options: SessionProviderOptions): AndroidAdbExecutor }; } +/** 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'; +} + +export function androidHelperRuntimeProbeResult( + release: FakeAndroidHelperRuntimeRelease = 'released', +): AndroidAdbExecutorResult { + 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): { exitCode: number; stdout: string; @@ -405,9 +451,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(androidHelperRuntimeProbeResult('released')); + } 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..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,9 +18,17 @@ 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 { getAndroidSnapshotHelperSessionDeviceKey } from '../snapshot-helper-retirement.ts'; +import { + getAndroidSnapshotHelperSessionDeviceKey, + isAndroidSnapshotHelperRuntimeOccupiedError, +} from '../snapshot-helper-retirement.ts'; import { lowerAndroidTouchPlan } from '../touch-plan-lowering.ts'; import { executeAndroidTouchHelperPlan, @@ -108,22 +116,33 @@ 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 createFakeTouchHelperSessionProvider( handleCommand: TouchSessionCommandHandler, - options: { stallCleanup?: boolean } = {}, + options: { stallCleanup?: boolean; runtimeRelease?: FakeAndroidHelperRuntimeRelease } = {}, ): 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); + } + if (isAndroidHelperRuntimeProbe(args)) { + return androidHelperRuntimeProbeResult(options.runtimeRelease); } return { exitCode: 0, stdout: '', stderr: '' }; }, @@ -166,23 +185,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, + runtimeRelease: 'occupied', }); - 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 +209,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 +219,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..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, - isAndroidSnapshotHelperRetirementUnconfirmedError, 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..55c8159370 100644 --- a/packages/platform-android/src/snapshot-helper-retirement.ts +++ b/packages/platform-android/src/snapshot-helper-retirement.ts @@ -1,22 +1,42 @@ 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 { 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. 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'; +/** + * 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 + * 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'; -type UnconfirmedRetirement = { +/** A release the last teardown could not prove; settled by the next acquire that reads the device. */ +type PendingRetirement = { packageName: string; cause: string; }; -const unconfirmedRetirements = new Map(); +const pendingRetirements = new Map(); export function getAndroidSnapshotHelperSessionDeviceKey( device: Pick, @@ -24,74 +44,167 @@ 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({ - adb: params.adb, + await stopAndroidSnapshotHelperRuntime({ adb: params.adb, packageName: params.packageName }); + await recordAndroidSnapshotHelperRelease({ + deviceKey: params.deviceKey, packageName: params.packageName, - timeoutMs: ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, + adb: params.adb, + cause: params.cause, }); - if (!runtimeForceStopped) { - quarantineAndroidSnapshotHelperRetirement(params); - } } +/** + * 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 stopAndroidSnapshotHelperRuntime({ + adb: params.adb, packageName: retirement.packageName, - cause: retirement.cause, + timeoutMs: RETIREMENT_RECOVERY_TIMEOUT_MS, + ...(params.signal ? { signal: params.signal } : {}), }); + params.signal?.throwIfAborted(); + 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, + 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 { + // 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(); + if (findPidToken(stdout)) return 'occupied'; + // 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 { + return 'unknown'; + } } export async function settleAndroidSnapshotHelperSessionCleanup(params: { @@ -108,25 +221,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 + ? [ + stopAndroidSnapshotHelperRuntime({ + 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 }; } /** @@ -165,7 +275,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; } @@ -210,27 +321,42 @@ 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. 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); +} + export function resetAndroidSnapshotHelperRetirements(): void { - unconfirmedRetirements.clear(); + pendingRetirements.clear(); } -async function forceStopAndroidSnapshotHelperRuntime(params: { +/** + * Best-effort device-side stop. Its outcome is never release evidence: whoever needs that reads the + * device with `readAndroidSnapshotHelperRuntimeRelease`. + */ +export async function stopAndroidSnapshotHelperRuntime(params: { adb: AndroidAdbExecutor; packageName: string; - timeoutMs: number; + 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; - } + timeoutMs: params.timeoutMs ?? ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, + ...(params.signal ? { signal: params.signal } : {}), + }) + .catch(() => {}); } async function removeAndroidSnapshotHelperSessionForward(params: { 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 }, + }); } diff --git a/packages/platform-android/src/snapshot-helper-session-lifecycle.ts b/packages/platform-android/src/snapshot-helper-session-lifecycle.ts index 0dd4fb62df..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'; @@ -35,17 +35,17 @@ import { ANDROID_SNAPSHOT_HELPER_DEVICE_RETIREMENT_TIMEOUT_MS, ANDROID_SNAPSHOT_HELPER_HOST_PROCESS_EXIT_GRACE_MS, getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRetirementUnconfirmedError, + hasAndroidSnapshotHelperProcessEnded, observeAndroidSnapshotHelperProcessExit, - quarantineAndroidSnapshotHelperRetirement, recoverAndroidSnapshotHelperRetirement, + recordAndroidSnapshotHelperRelease, resetAndroidSnapshotHelperRetirements, + settleAndroidSnapshotHelperRetirement, settleAndroidSnapshotHelperSessionCleanup, stopAndroidSnapshotHelperHostProcess, waitForAndroidSnapshotHelperProcessExit, } from './snapshot-helper-retirement.ts'; -const SESSION_READY_TIMEOUT_MS = 10_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. @@ -56,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; @@ -84,7 +93,8 @@ export type AndroidSnapshotHelperSessionAcquisition = { }; const sessions = new Map(); -const disabledSessionIdentities = 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 @@ -110,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; } @@ -138,42 +148,93 @@ 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); + 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); + 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; + startBudgetMs: number; }): Promise { - const { deviceKey, identity, options, resolved } = params; - if (disabledSessionIdentities.get(deviceKey) === identity) { + 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 = 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; - } - } - 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, + ); +} + +/** + * 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: { @@ -181,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}`], { @@ -219,12 +281,22 @@ async function startAndroidSnapshotHelperSession(params: { capturedCount: 0, }; try { + // A helper that announces itself late is a slow `am instrument`, which the one-shot transport it + // 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, - SESSION_READY_TIMEOUT_MS, + params.startBudgetMs, params.options.signal, ); sessions.set(params.deviceKey, session); + failedStarts.delete(params.identity); + // `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', data: { @@ -242,7 +314,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 +330,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; } } @@ -301,6 +374,24 @@ function resolvePersistentSessionCaptureOptions( }; } +/** + * 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( + 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); @@ -335,7 +426,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 +453,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 +473,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 +526,18 @@ export async function stopAndroidSnapshotHelperSessionForDevice( } export async function resetAndroidSnapshotHelperSessions(): Promise { - const retirements = await Promise.allSettled( - [...sessions.keys()].map((deviceKey) => 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'); + 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(); } - 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..05fb4ab5f3 100644 --- a/packages/platform-android/src/snapshot-helper-session-protocol.ts +++ b/packages/platform-android/src/snapshot-helper-session-protocol.ts @@ -108,6 +108,9 @@ 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'; + export function waitForAndroidSnapshotHelperSessionReady( childProcess: AndroidAdbProcess, timeoutMs: number, @@ -121,6 +124,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 +158,7 @@ export function waitForAndroidSnapshotHelperSessionReady( output, exitCode: code, signal: exitSignal, + reason: SESSION_EXITED_BEFORE_READY_REASON, }), ); }); 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'; diff --git a/packages/platform-android/src/snapshot-helper.ts b/packages/platform-android/src/snapshot-helper.ts index b7e365914e..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, - isAndroidSnapshotHelperRetirementUnconfirmedError, -} 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 2e73c36844..008e30913a 100644 --- a/packages/platform-android/src/snapshot.ts +++ b/packages/platform-android/src/snapshot.ts @@ -36,8 +36,6 @@ import { captureAndroidSnapshotWithHelperSession, ensureAndroidSnapshotHelper, forgetAndroidSnapshotHelperInstall, - getAndroidSnapshotHelperSessionDeviceKey, - isAndroidSnapshotHelperRetirementUnconfirmedError, 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'; @@ -377,7 +379,7 @@ async function captureAndroidUiHierarchyFromHelper(params: { if (sessionCapture) return sessionCapture; } catch (error) { signal?.throwIfAborted(); - if (isAndroidSnapshotHelperRetirementUnconfirmedError(error)) { + if (isAndroidSnapshotHelperRuntimeOccupiedError(error)) { throw error; } emitDiagnostic({ @@ -478,29 +480,28 @@ 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, 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 && attempt > 0) { emitDiagnostic({ - phase: 'android_snapshot_helper_system_surface', - data: { foregroundAppPackage: options.appBundleId }, + phase: 'android_snapshot_helper_content_recaptured', + data: { attempts: attempt + 1, recoveredFromReason: params.previousContentReason }, }); - 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 }; } - 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 {