fix(android): read a release only from an answer, not from an adb that complained

Author: thymikee

CHANGELOG.md

@@ -8,23 +8,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

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,

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,

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<AndroidSnapshotHelperRuntimeRelease, 'unknown'>
-  | '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: '' };

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);

packages/platform-android/src/snapshot-helper-session-lifecycle.ts

@@ -282,18 +282,20 @@ 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,
       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.
+    // `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(