|
| 1 | +# Design — #237: Android instrumentation-slot handoff (L2 interaction → L3 flow) |
| 2 | + |
| 3 | +- **Issue:** GH #237 (`kano:must-be`, `effort:m`). Android analog of the iOS `ensureSingleRunner` / `RN_DEVICE_KILL_LEGACY` work (#202 Phase 1). |
| 4 | +- **Date:** 2026-06-09 |
| 5 | +- **Status:** Approved design (brainstorming) → pending implementation plan |
| 6 | +- **Refs:** #202 Phase 2a (`runFlowParked` / L2-park-for-flow precedent), #202 Phase 1 (`ensure-single-runner.ts`), #210 (one-coherent-path / self-heal precedent), #165 (in-tree `rn-android-runner`), three-layer device-control contract (CLAUDE.md), D-arbiter (`device-arbiter.ts`). |
| 7 | +- **Reviewed by:** Antigravity (`agy` CLI 1.0.6) + an independent source-verified review — both verdicts **APPROVE-WITH-CHANGES**, strongly convergent. Amendments folded in (see §9). |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## 1. Problem |
| 12 | + |
| 13 | +On Android, after any `device_*` call, `maestro_run` fails: |
| 14 | + |
| 15 | +``` |
| 16 | +failed to create driver: start UIAutomator2: UIAutomator2 server not ready after 30s |
| 17 | +``` |
| 18 | + |
| 19 | +**Root cause.** Android permits **one active `UiAutomation` connection at a time.** Our L2 interaction runner holds it, then maestro-runner (L3) cannot bind its own UIAutomator2 server. Two distinct slot-holders exist: |
| 20 | + |
| 21 | +1. **The in-tree `rn-android-runner`** (shipped #165, default-on via `RN_ANDROID_RUNNER !== '0'`) — started via `adb shell am instrument` of `dev.lykhoyda.rndevagent.androidrunner.test` (`rn-android-runner-client.ts:18,150`). This is the modern default holder. |
| 22 | +2. **The legacy `agent-device` daemon** — holds its own UIAutomator2 connection. Still reached for Android verbs NOT served by the in-tree runner (`device_deeplink`, `device_permission`, `device_reset_state` — `agent-device-wrapper.ts:760-788`), and for **all** interaction when a user sets `RN_ANDROID_RUNNER=0`. Host-side, so it survives `adb reboot` (the issue's "survives reboot" observation). |
| 23 | + |
| 24 | +**The defect.** `runFlowParked()` (`maestro-run.ts:34-43`) — the wrapper all three flow tools execute inside — **parks only the iOS runner** (`stopFastRunner()`). It has no Android counterpart, even though `stopAndroidRunner()` exists (`rn-android-runner-client.ts:222`) but **is never called outside its own file**. So on Android, the L2 slot-holder is never released before an L3 flow. |
| 25 | + |
| 26 | +**Why the obvious workaround is wrong.** The reporter's `pkill -f agent-device` freed the slot but **also killed the MCP server** — a broad pattern-kill matches too much. The fix must release the slot *surgically* (specific packages/PIDs, never `pkill`). |
| 27 | + |
| 28 | +### Key correctness finding (both reviewers, independently) |
| 29 | + |
| 30 | +`stopAndroidRunner()`'s `runnerProcess?.kill('SIGTERM')` (`rn-android-runner-client.ts:224`) kills only the **host-side** `adb shell am instrument` child. Because `am instrument` is launched by Android's `system_server` (ActivityManager), the **device-side** instrumentation process keeps running and keeps holding the `UiAutomation` slot after the local adb pipe is severed. **`adb shell am force-stop <pkg>` is the decisive slot-release**, not the SIGTERM. This reframes the fix: force-stop is the primary mechanism, the host-side `stopAndroidRunner()` is the secondary (process-handle cleanup + adb-forward removal). |
| 31 | + |
| 32 | +--- |
| 33 | + |
| 34 | +## 2. Scope & decision |
| 35 | + |
| 36 | +**Release BOTH slot-holders on the L2→L3 handoff**, surgically: |
| 37 | + |
| 38 | +- **Our own runner** — always (it is *our* resource; never gated). |
| 39 | +- **Legacy `agent-device` daemon** — gated, because it may belong to another project (see §4). |
| 40 | + |
| 41 | +**In scope:** per-flow slot release inside `runFlowParked` for all three flow tools (`maestro_run`, `maestro_test_all`, `cdp_auto_login`), plus `cdp_run_action` which composes `maestro_run`. |
| 42 | + |
| 43 | +**Out of scope (this PR):** |
| 44 | +- Session-open Android cleanup (the "survives-reboot" stale daemon at `device_snapshot action=open`) — possible follow-up; the per-flow park is where the L2→L3 conflict actually lives. |
| 45 | +- The issue's *secondary* observations: `maestro_run` `repeat`-command allowlist; multi-device CDP routing (#60); MCP-server self-disconnects. |
| 46 | +- iOS behavior — untouched. |
| 47 | + |
| 48 | +--- |
| 49 | + |
| 50 | +## 3. Architecture |
| 51 | + |
| 52 | +### New module: `src/runners/release-android-slot.ts` |
| 53 | + |
| 54 | +The Android analog of `ensure-single-runner.ts`: a pure-core orchestrator with dependency-injected `deps` for unit testing (no device required in unit tests). |
| 55 | + |
| 56 | +```ts |
| 57 | +export interface ReleaseAndroidSlotResult { |
| 58 | + stoppedOwnRunner: boolean; |
| 59 | + forceStoppedPackages: string[]; |
| 60 | + killedDaemonPids: number[]; |
| 61 | + removedFiles: string[]; |
| 62 | + warnings: string[]; |
| 63 | + meta: { timings_ms: Record<string, number> }; |
| 64 | +} |
| 65 | + |
| 66 | +export interface ReleaseAndroidSlotDeps { |
| 67 | + stopOwnRunner: (deviceId?: string) => Promise<void>; // ← stopAndroidRunner |
| 68 | + adbForceStop: (pkg: string, serial: string[]) => Promise<void>; |
| 69 | + readDaemonPid: () => number | null; |
| 70 | + isAlive: (pid: number) => boolean; |
| 71 | + isSelfOrAncestor: (pid: number) => boolean; // ← self-kill guard |
| 72 | + kill: (pid: number, sig: NodeJS.Signals) => void; |
| 73 | + fileExists: (p: string) => boolean; |
| 74 | + removeFile: (p: string) => void; |
| 75 | + delay: (ms: number) => Promise<void>; |
| 76 | + adbSerial: () => string[]; // ← getAdbSerial |
| 77 | + killLegacy: () => boolean; // ← RN_DEVICE_KILL_LEGACY !== '0' |
| 78 | +} |
| 79 | + |
| 80 | +export async function releaseAndroidInteractionSlot( |
| 81 | + opts: { deviceId?: string }, |
| 82 | + deps: ReleaseAndroidSlotDeps = defaultDeps(), |
| 83 | +): Promise<ReleaseAndroidSlotResult>; |
| 84 | +``` |
| 85 | + |
| 86 | +**Owned identifiers** (force-stop targets — OUR packages only): |
| 87 | +- `dev.lykhoyda.rndevagent.androidrunner.test` (the instrumentation/test package) |
| 88 | +- `dev.lykhoyda.rndevagent.androidrunner` (the runner app package) |
| 89 | + |
| 90 | +### Modified: `runFlowParked` |
| 91 | + |
| 92 | +```ts |
| 93 | +// before: runFlowParked<T>(run, deps) |
| 94 | +// after: runFlowParked<T>(run, opts: { platform?: 'ios' | 'android'; deviceId?: string } = {}, deps) |
| 95 | +``` |
| 96 | + |
| 97 | +On `opts.platform === 'android'`, `await releaseAndroidInteractionSlot({ deviceId: opts.deviceId })` **before** `run()`. iOS path unchanged (`stopFastRunner()`). `markCdpStale()` still runs in `finally` for both platforms. |
| 98 | + |
| 99 | +### Call sites (all already resolve `platform`; pass `deviceId` from the active session) |
| 100 | +- `maestro-run.ts:193` — `platform` in scope (line 103). |
| 101 | +- `maestro-test-all.ts:143` — `platform` in scope (line 66). |
| 102 | +- `auto-login.ts:223` — `platform` in scope (line 130). |
| 103 | + |
| 104 | +--- |
| 105 | + |
| 106 | +## 4. Release algorithm |
| 107 | + |
| 108 | +Sequential, each step **best-effort** (failures push to `warnings[]`, never throw — a flow must not fail because cleanup hiccuped). All `adb`/process operations carry a **hard ~5s timeout** so a hung adb daemon can't block the MCP server. |
| 109 | + |
| 110 | +| Step | Action | Gated by `RN_DEVICE_KILL_LEGACY`? | Why | |
| 111 | +|---|---|---|---| |
| 112 | +| **1. Our runner** | `stopAndroidRunner(deviceId)` — kill our `am instrument` handle + `adb forward --remove` | **No** — always | Our resource; secondary cleanup (the host handle + port) | |
| 113 | +| **2. Our instrumentation** | `adb shell am force-stop` of **both** owned packages, scoped via `getAdbSerial()` | **No** — always | **The decisive slot-release** (device-side process the SIGTERM left alive) | |
| 114 | +| **3. Legacy daemon** | Read PID from `~/.agent-device/daemon.json`; if alive **AND not self/ancestor** → SIGTERM → grace → SIGKILL; clear orphaned `daemon.{json,lock}` | **Yes** | May belong to another project; mirrors the iOS gate | |
| 115 | + |
| 116 | +**Flag rationale (synthesis of the two reviews).** Reuse the existing `RN_DEVICE_KILL_LEGACY` flag (one mental model; the daemon-file path is genuinely shared with iOS), **but gate ONLY step 3 behind it.** Steps 1 & 2 are unconditionally safe (they touch only *our* runner/instrumentation) and *are* the core #237 fix — so `RN_DEVICE_KILL_LEGACY=0` disables the legacy-daemon cleanup without ever breaking the primary fix. |
| 117 | + |
| 118 | +**Foreign runners are NOT force-stopped.** Force-stopping a *competing* tool's UIAutomator2 package (a foreign maestro-mcp, Appium) is the same overreach that killed the MCP server. Foreign competitors keep the existing detect-and-warn path (`detectAndroidExternalRunner` → `ANDROID_UIAUTOMATOR_COMPETITOR`); we never kill what we don't own. |
| 119 | + |
| 120 | +**Idempotency requirement.** `releaseAndroidInteractionSlot` MUST be safe to call when the runner is already stopped / the daemon is already gone (no throw, no error result). This is load-bearing for the auto-repair re-entrancy path (§6). |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +## 5. Self-heal after the flow |
| 125 | + |
| 126 | +No explicit Android restart. `runAndroid()` calls `startAndroidRunner()` at the top of **every** command (`rn-android-runner-client.ts:293`), idempotent via `isAndroidRunnerAvailable() && shouldReuseAndroidRunner()` — so the next `device_*` cold-starts a fresh runner. Mirrors iOS's lazy fast-runner restart. `markCdpStale()` (already in `finally`) forces the next CDP read to reconnect to post-flow state. |
| 127 | + |
| 128 | +--- |
| 129 | + |
| 130 | +## 6. Failure handling & edge cases |
| 131 | + |
| 132 | +- **iOS untouched** — `platform !== 'android'` keeps the exact current path; no Android module imported on the iOS branch. |
| 133 | +- **No emulator / adb missing** — every step degrades to a warning; the flow still proceeds (maestro's own error stays authoritative). |
| 134 | +- **`RN_DEVICE_KILL_LEGACY=0`** — skips step 3 only; steps 1 & 2 still run (the core fix). |
| 135 | +- **Self-kill guard** — never SIGTERM/SIGKILL `process.pid` or an ancestor (`isSelfOrAncestor`). The specific defense against the reporter's "it dropped the MCP server" — a stale, OS-recycled PID in `daemon.json` could otherwise match our own tree. |
| 136 | +- **Auto-repair re-entrancy** (`run-action.ts`): inside `cdp_run_action`, an auto-repair snapshot restarts the runner, then `maestro_run` fires again → `runFlowParked` re-runs the release. Safe because the exclusive `flow` arbiter lease is held continuously across the whole sequence (see §7), and the release is idempotent (§4). |
| 137 | +- **Hard adb timeout** — a disconnected emulator / hanging adb server cannot block the MCP server. |
| 138 | + |
| 139 | +--- |
| 140 | + |
| 141 | +## 7. Concurrency — why no re-acquire race |
| 142 | + |
| 143 | +`arbiterWrap` (`device-arbiter.ts:155`) acquires the **exclusive `flow` lease** before any flow handler runs; the arbiter refuses `flow` while any op is in flight and refuses `interaction` while the flow lease is held (`device-arbiter.ts:31-49`). Therefore `releaseAndroidInteractionSlot` runs **inside** the held flow lease — no concurrent `device_*` can re-grab the slot between our release and maestro's bind. **This is load-bearing: the release MUST stay inside `runFlowParked` (where the lease is held); do not move it earlier/outer.** The new module documents this. (The only residual re-bind risk is a *foreign* process, which §4 deliberately does not fight.) |
| 144 | + |
| 145 | +The arbiter itself stays **pure in-memory** — no device I/O is added to it (CLAUDE.md: persisting/side-effecting a lease recreates the #202 orphaned-lock bug). All side effects live in the new module, invoked from `runFlowParked`. |
| 146 | + |
| 147 | +--- |
| 148 | + |
| 149 | +## 8. Testing strategy (TDD) |
| 150 | + |
| 151 | +**Unit (DI, no device):** |
| 152 | +- `releaseAndroidInteractionSlot` orchestration order: step 1 → 2 → 3; asserts both owned packages are force-stopped; asserts step 3 skipped when `killLegacy()` is false while 1 & 2 still run. |
| 153 | +- Self-kill guard: daemon PID == self/ancestor → NOT killed (warning emitted). |
| 154 | +- Idempotency: stub `stopOwnRunner` resolving when nothing is running → no throw, clean result. |
| 155 | +- `runFlowParked` branch dispatch: release fn called on `android`, NOT on `ios`; `stopFastRunner` still called on `ios`; `markCdpStale` runs even when `run()` throws (both platforms). |
| 156 | +- Existing `gh-202-maestro-flow-parks-l2.test.js` updated for the new `runFlowParked(run, opts, deps)` signature. |
| 157 | + |
| 158 | +**Live emulator (workflow step 5) — a gate, not a nice-to-have:** |
| 159 | +- Booted Pixel emulator: `device_snapshot` (starts our runner) → `maestro_run` → assert UIAutomator2 binds and the flow passes. |
| 160 | +- **Decisive-step experiment:** prove the slot is free after **step 2 alone** (force-stop) vs **step 1 alone** (SIGTERM) — confirms force-stop is the lever both reviews predict, and that the device-side instrumentation indeed survives the SIGTERM. |
| 161 | +- Repeat with `RN_ANDROID_RUNNER=0` + a live legacy daemon to exercise step 3. |
| 162 | + |
| 163 | +--- |
| 164 | + |
| 165 | +## 9. Amendments folded in from the multi-LLM review (Antigravity + source-verified) |
| 166 | + |
| 167 | +1. **Force-stop is the decisive mechanism** (device-side instrumentation survives the host SIGTERM) — promoted step 2 from belt-and-suspenders to primary; live test gates it. |
| 168 | +2. **Force-stop OUR OWN packages only** — both `…androidrunner.test` and `…androidrunner`; foreign competitors keep the warn-only path. |
| 169 | +3. **`runFlowParked` signature** → `(run, { platform, deviceId }, deps)`. |
| 170 | +4. **Hard ~5s timeout** on all `adb` calls in the release path. |
| 171 | +5. **Keep step 3** (legacy daemon kill) — Antigravity evidence: `device_deeplink`/`permission`/`reset_state` + `RN_ANDROID_RUNNER=0` route to the legacy daemon (`agent-device-wrapper.ts:760-788`). |
| 172 | +6. **Reuse `RN_DEVICE_KILL_LEGACY`, gate only step 3 behind it** — steps 1 & 2 always run. |
| 173 | +7. **Idempotency** of `releaseAndroidInteractionSlot` (auto-repair re-entrancy) — explicit requirement + unit test. |
0 commit comments