fix(android): give a helper start a share of the caller's own timeout

Author: thymikee

CHANGELOG.md

@@ -20,7 +20,11 @@
   `pidof` happens to be unreadable. A helper start that fails is also retried after a backoff scaled
   to how long it spent failing (10 s to 60 s) instead of on every command, which had roughly doubled
   command time on hosts where the helper never starts, and the wait for a started helper to announce
-  itself now uses the caller's own helper-command budget, so `--timeout` reaches it.
+  itself now takes a share of the caller's own helper-command budget — half of `--timeout`, never
+  less than one session command is worth — so a device that needs longer than a capture to bring the
+  helper up stays on the persistent path when the caller budgeted for it. On a host where the helper
+  took 12 s to announce itself, `--timeout 60000` used to answer with the one-shot transport and now
+  answers from the session.
 
 - Fixed: an iOS snapshot whose XCTest query-sweep tier cannot read the screen no longer ends the
   runner process. On a live React Native feed (Bluesky Home, images re-rendering) the AX server

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

@@ -3,6 +3,7 @@ import { afterEach, beforeEach, test } from 'vitest';
 import { captureAndroidSnapshotWithHelperSession } from '../snapshot-helper-session.ts';
 import {
   resetAndroidSnapshotHelperSessions,
+  resolveAndroidSnapshotHelperStartBudgetMs,
   stopAndroidSnapshotHelperSession,
 } from '../snapshot-helper-session-lifecycle.ts';
 import { recoverAndroidSnapshotHelperRetirement } from '../snapshot-helper-retirement.ts';
@@ -123,6 +124,16 @@ test('a session start waits only as long as the caller budgeted for one helper c
   );
 });
 
+test('a generous caller budget buys a slow start, and never more than the caller allowed', () => {
+  // A capture-sized guess is what pushed the slow hosts of #2553 off the persistent path even when
+  // `--timeout` left plenty of room for the same start in the one-shot transport.
+  assert.equal(resolveAndroidSnapshotHelperStartBudgetMs(60_000), 30_000);
+  assert.equal(resolveAndroidSnapshotHelperStartBudgetMs(30_000), 15_000);
+  // A short budget buys nothing extra, and a tiny one is not answered with a longer wait.
+  assert.equal(resolveAndroidSnapshotHelperStartBudgetMs(6_000), 5_000);
+  assert.equal(resolveAndroidSnapshotHelperStartBudgetMs(1_000), 1_000);
+});
+
 test('a session that reaches ready settles a release the device could not confirm', async () => {
   const calls: string[][] = [];
   const spawnArgs: string[][] = [];

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

@@ -120,15 +120,15 @@ export async function acquireAndroidSnapshotHelperSession(
   if (!isAndroidSnapshotHelperSessionEnabled() || !options.adbProvider?.spawn) {
     return undefined;
   }
-  const resolved = resolvePersistentSessionCaptureOptions(
-    resolveAndroidSnapshotHelperCaptureOptions(options),
-  );
+  const callerResolved = resolveAndroidSnapshotHelperCaptureOptions(options);
+  const resolved = resolvePersistentSessionCaptureOptions(callerResolved);
   const identity = createSessionIdentity(deviceKey, resolved, options);
   const session = await resolveAndroidSnapshotHelperSession({
     deviceKey,
     identity,
     options,
     resolved,
+    startBudgetMs: resolveAndroidSnapshotHelperStartBudgetMs(callerResolved.commandTimeoutMs),
   });
   return session ? { session, resolved, deviceKey } : undefined;
 }
@@ -148,6 +148,7 @@ async function resolveAndroidSnapshotHelperSession(params: {
   identity: string;
   options: AndroidSnapshotHelperCaptureOptions;
   resolved: AndroidSnapshotHelperResolvedCaptureOptions;
+  startBudgetMs: number;
 }): Promise<AndroidSnapshotHelperSession | undefined> {
   if (isAndroidSnapshotHelperStartBackedOff(params.identity)) return undefined;
   await retireUnusableAndroidSnapshotHelperSession(params.deviceKey, params.identity);
@@ -178,6 +179,7 @@ async function tryStartAndroidSnapshotHelperSession(params: {
   identity: string;
   options: AndroidSnapshotHelperCaptureOptions;
   resolved: AndroidSnapshotHelperResolvedCaptureOptions;
+  startBudgetMs: number;
 }): Promise<AndroidSnapshotHelperSession | undefined> {
   const startedAtMs = Date.now();
   try {
@@ -240,6 +242,7 @@ async function startAndroidSnapshotHelperSession(params: {
   identity: string;
   options: AndroidSnapshotHelperCaptureOptions;
   resolved: AndroidSnapshotHelperResolvedCaptureOptions;
+  startBudgetMs: number;
 }): Promise<AndroidSnapshotHelperSession> {
   const port = await allocateAndroidSnapshotHelperSessionPort();
   await params.options.adb(['forward', `tcp:${port}`, `tcp:${port}`], {
@@ -278,12 +281,12 @@ async function startAndroidSnapshotHelperSession(params: {
     capturedCount: 0,
   };
   try {
-    // Starting the session gets the budget the caller already allowed one helper command, which is
-    // how `--timeout` reaches it. A fixed guess below that pushed a slow device out of the persistent
-    // path while the one-shot transport it fell back to had room for the same start.
+    // A helper that announces itself late is a slow `am instrument`, which the one-shot transport it
+    // falls back to pays too. The start gets its share of the caller's command budget instead of a
+    // fixed guess, so `--timeout` decides whether the persistent path is affordable at all.
     await waitForAndroidSnapshotHelperSessionReady(
       childProcess,
-      params.resolved.commandTimeoutMs,
+      params.startBudgetMs,
       params.options.signal,
     );
     sessions.set(params.deviceKey, session);
@@ -369,6 +372,22 @@ function resolvePersistentSessionCaptureOptions(
   };
 }
 
+/**
+ * What a start gets out of the budget the caller allowed one helper command: half of it, so a helper
+ * that announces itself later than a session capture takes is not pushed off the persistent path by
+ * a capture-sized guess, while the one-shot transport that answers a failed start keeps the other
+ * half. Never less than one session command is worth, never more than the caller allowed.
+ */
+export function resolveAndroidSnapshotHelperStartBudgetMs(commandTimeoutMs: number): number {
+  return Math.min(
+    commandTimeoutMs,
+    Math.max(
+      Math.floor(commandTimeoutMs / 2),
+      SESSION_CAPTURE_TIMEOUT_MS + SESSION_REQUEST_OVERHEAD_MS,
+    ),
+  );
+}
+
 function isAndroidSnapshotHelperSessionEnabled(): boolean {
   const value = requireAndroidAdbHost().environment.AGENT_DEVICE_ANDROID_SNAPSHOT_HELPER_SESSION;
   return value === undefined || !/^(0|false|no|off)$/i.test(value);