fix(android): read device ownership off the device, not off adb - #2604
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 595f562. Reading ownership off the device is the right direction, but two paths can still clear or break ownership without proof. A transport failure reads as An The tests "unproven release stays pending until an acquire reads the device" and "unreadable device leaves the retirement pending" ( Two smaller points. A design question before more state goes in: The changed teardown and acquire path also needs a live Android run under a slow or offline adb, including one session reused across two commands after an Coverage fails in |
|
Reviewed at 595f562. Reading ownership from the device is the right fact to use, but a few cases still look wrong.
The error reason changes from Would a smaller change do? Keep the old two-map shape and change only the evidence source: a three-state The emulator run in the description covers the slow-transport repro. Could you also run a The Coverage failure is from this change, not pre-existing: the new module adds the extra module to the |
|
Reviewed at eed1d0c. The Coverage failure from 595f562 is gone because the delta deletes snapshot-helper-retry-state.ts, but the main problem from the last review is still open. snapshot-helper-retirement.ts#L166: the snapshot-helper-session-lifecycle.ts#L220: after an snapshot-helper-session-lifecycle.ts#L145: the delta removes the start backoff that main and 595f562 had. When the helper never starts, each command now spawns, waits up to its timeout, force-stops and probes before it falls back to one-shot, which on the slow hosts from #2553 could roughly double command time. Could a short in-memory backoff per identity come back, with a test that counts spawns across several commands on a device that always times out? The retirement tests at snapshot-helper-retirement.test.ts#L76 still assert only "no adb calls and no state change", as noted at 595f562, so they pass even if recover silently consumes the entry. Recording the calls and asserting that a second recover still sends force-stop and A smaller shape may cover all of this: keep main's two maps and change only the evidence source, so This device-facing change still needs a live Android run on the fixed head covering a |
eed1d0c to
ffae528
Compare
|
Fixed at 18c6651 (four commits on top of
A pending Failed-start backoff. The retirement tests assert calls now. Smaller shape. Mostly taken: the retry-state module and its test are gone, Smaller points. Two cleanups found on the way: the system-surface case no longer emits a diagnostic duplicating the typed Found while verifying on device, and fixed: the readiness wait was still capped at 5 s. Live runs (
Gate: |
|
Reviewed at 18c6651, as a follow-up to the review at eed1d0c. The ready-settle and the start backoff are back, but the snapshot-helper-retirement.ts#L188 reads every stderr that snapshot-helper-session-lifecycle.ts#L381: snapshot-helper-session-lifecycle.ts#L294 says a ready helper holds the device's one UiAutomation connection. A smaller point: All 17 checks pass and there are no conflicts. The live runs in the PR are author-reported and use Next: make |
18c6651 to
f4a514b
Compare
|
Fixed at
The The settle's mechanism is named where it can break. The diagnostic removal is named in the PR body as one of two cleanups that are not ownership fixes, with why: nothing consumed Live run with an unrecognized adb error while the helper runs (Pixel 7 CI emulator, API 35): an adb shim answers
Gate: |
|
Reviewed at f4a514b, as a follow-up to the review at 18c6651. Replacing the classifier with one positive rule makes this smaller, but a signal-killed The rule at snapshot-helper-retirement.ts#L191 is a non-zero exit with empty stdout and stderr. The local executor sets Can the exec result carry the signal, or keep All 17 checks pass and there are no conflicts. Next: decide the signal case and pin it with a fixture. |
f4a514b to
3de2027
Compare
|
Fixed at Neither signal plumbing nor an accepted gap: the probe stopped reading exit status. I did not add a signal to The stderr clause stays for a different reason than before: a device shell with no Pinned with fixtures, as asked. Same scenario on the emulator (Pixel 7, API 35): an adb shim answers
Holistic pass over the claim you corrected. Four other comments still repeated "Android permits ONE UiAutomation owner" as the reason for device-exclusivity: the Gate: |
|
Reviewed at 3de2027, as a follow-up to the review at f4a514b. The signal case is fixed. A release is now read only from the marker with an empty stderr, so a signal-killed read with empty streams is One question: remote or cloud Android executors were not checked. If one quotes All 5 checks pass. The PR now conflicts with main. Next: rebase onto main and let CI run on the new head. |
The snapshot helper retirement treated the outcome of an `am force-stop` call as the fact it was supposed to measure. On a loaded host the adb round trip exceeds its budget while the helper process is already gone, so a completed interaction failed its own teardown, and the quarantine that followed refused the next command with "could not confirm release of device automation ownership" on a device that had nothing holding UiAutomation. Ownership is a device fact, so it is now read as one: `adb shell pidof` answers released, occupied, or unknown, and only a device that names a live helper process may refuse a command. Teardown records what it could not prove for the next acquire and never decides what the command it is finishing reports, which is what let a settled `press` turn into a stale-coordinate failure in #2553. A start that only ran out of time or lost its transport no longer excludes the helper identity for the rest of the daemon's life; it retries after a cooldown. Only a helper that ran and exited before announcing readiness proves the identity unusable.
…swer resetAndroidSnapshotHelperRuntime carried a second best-effort `am force-stop` with the same 2s budget the retirement path already owned, and two session fakes answered `pidof` between them. One stop serves both callers, one fake answers the probe for every test that steers it, and the release read now looks for a pid token rather than any digit.
A disabled session identity stored an epoch time under a duration's name, and the pending retirement next to it asked the same question with its own shape. One retry state now answers both: a value plus when trying again is worth it, standing until then or until the device settles it. The helper device key and the occupied-device predicate stay module-owned: nothing outside platform-android reads them, so they leave the mechanics boundary.
… the caller does A helper start that failed was answered twice: the caller fell back to the one-shot transport, and the identity was excluded anyway — for the daemon's whole life in the worst case, which is how a transient slow start becomes a permanent state. The fallback covers the outcome, so the next command starts again and nothing stands in front of it. The wait for readiness also stops being a guess at how long Android needs. It gets the budget the caller allowed one helper command, which is how `--timeout` reaches it, so a slow device is not pushed out of the persistent path while the transport it fell back to had room for the same start. A helper capture's content branches answer one question — is this worth another call — so they return once, disclosing a system-surface tree instead of giving it its own branch.
3de2027 to
788944f
Compare
|
Rebased onto main as 788944f. The only conflict was CHANGELOG.md, and both sides are kept. The range-diff shows every commit unchanged apart from CHANGELOG context. The 731 platform-android tests and |
thymikee
added
the
ready-for-human
|
|
Checked at 788944f. The rebase onto main is identical to 3de2027 apart from CHANGELOG.md, where both entries are kept, so the code verdict from the 3de2027 review still holds and the live runs in the PR body still cover the changed path. All checks pass on 788944f, and there are no conflicts. The earlier question about remote or cloud Android executors that quote |
1 participant
Summary
Snapshot helper retirement treated the outcome of an
am force-stopcall as the fact it was supposed to measure. On a loaded host that adb round trip exceeds its budget while the helper process is already gone, so a completed interaction failed in its own teardown and the following quarantine refused the next command with "could not confirm release of device automation ownership" on a device holding nothing. That is also how a settledpress --settleturned into the stale-coordinate failures reported in the issue.Ownership is a device fact, so it is read as one now: the probe asks the device shell for
pidof <helperPackage>and tells it to echo a marker when nothing matched. A process id isoccupied, the bare marker with a silent stderr isreleased, and everything else —error: closed,cannot connect to daemon,device offline, an adb client killed by a signal before it wrote anything — isunknown. A release is then claimable only through an answer the transport cannot produce about itself: a client that ran out of budget, was SIGKILLed, or lost the connection prints nothing at all. No exit status is consulted either, becauseadb shellanswers 0 for a device command that failed (measured on API 35) and an adb that dies by signal leaves the executor inventing an exit code it never saw. Listing adb's failure texts was not the fix either: the list is long, version-dependent, and every missed entry fails open. Only a device that keeps naming a live helper process may refuse a command (android_snapshot_helper_runtime_occupied), and it refuses after a second read, so a helper still inside Android's exit path costs nothing. Teardown records what it could not prove for the next acquire and never decides what the command it is finishing reports.A session that reaches ready settles a pending release, because
am instrumentfor the helper package force-stops whatever is already instrumenting that package — the ready helper is the only helper process the device has. That mechanism is named in the lifecycle comment, thesettleAndroidSnapshotHelperRetirementdocblock, the CHANGELOG and the test, so a change to the helper's ready order or to its package breaks the settle loudly instead of silently. A helper start that fails is answered for by the one-shot transport and not spawned again until a backoff scaled to how long it spent failing has run out (10–60 s per capture identity), which is what had roughly doubled command time on hosts where the helper never starts. The readiness wait takes half the helper-command budget the capture was built with — 15 s today — instead of a fixed 10 s; the CLI's--timeoutreaches that wait as its deadline aborting it, not as the number.Two cleanups ride along, named because they are not ownership fixes: the system-surface capture case no longer emits an
android_snapshot_helper_system_surfacediagnostic (no consumer read it, and the typedsystemSurfaceOnlymetadata already travels with the response and becomes the user-facing disclosure), and a cached session whose instrumentation process has already exited is dropped rather than written to. Four comments also repeated the "Android permits one UiAutomation owner" story this review rejected; they name the instrument takeover now, in the lifecycle module docstring, the session-scope type, the touch piggyback comment and the fill-verification header. The runtime reset also shares the one best-effortam force-stopthe retirement path owns, one fake answerspidoffor every session test, and the helper device key and the occupied-device predicate stay module-owned.Closes #2553
Validation
Tested at
3de2027997.pnpm check:affected --run: all runnable checks passed, incl.check:fallowandcheck:layering; typecheck, lint and format clean; 718 platform-android tests. All 17 GitHub checks pass at18c6651.am instrument12s andam force-stop6s madesnapshot --force-fullfail with the ownership error before the change whileadb shell pidofshowed no helper process; after it, consecutive commands each return 37 nodes. Healthy-hostsnapshot(p50 296ms) andpress --settleunaffected.pidofread killed by a signal while the helper ran: the shim answersshell pidofby SIGKILLing itself before writing anything,am force-stopas a no-op, and refuses the one-shot fallback so the request log keeps the diagnostic. Request log2ed92c31c6c3c924.ndjson:{"ts":"2026-09-15T15:36:28.151Z","level":"warn","phase":"android_snapshot_helper_retirement_pending","requestId":"2ed92c31c6c3c924","command":"snapshot","data":{"deviceKey":"android:emulator-5554","packageName":"com.callstack.agentdevice.snapshothelper","release":"unknown"}}with helper pid 27363 read directly afterwards by real
adb shell pidof. Under the exit-code rule that shape wasreleasedand the entry was cleared. The next command's calls aream force-stop→pidof … || echo AGENT_DEVICE_NO_HELPER→am instrument: the pending retirement was retried, an unreadable read refused nothing, and the command answered from a fresh session (helper pid 27409; the instrument takeover ended 27363, the same mechanism the settle rests on).shell pidofwitherror: closed(a fault no classifier lists) andam force-stopas a no-op, with the session socket pulled mid-command. Request log5477a2983b05082a.ndjson:{"ts":"2026-09-15T13:41:55.173Z","level":"warn","phase":"android_snapshot_helper_retirement_pending","requestId":"5477a2983b05082a","command":"snapshot","data":{"deviceKey":"android:emulator-5554","packageName":"com.callstack.agentdevice.snapshothelper","release":"unknown"}}adb shell pidofread directly afterwards still returned the helper (pid 25334), so theunknowncame from a live helper and an unrecognized adb error — the shape that used to readreleasedand clear the entry. The next command's shim log order isam force-stop→pidof→am instrument -e sessionPort: the entry stayed pending, the stop attempt precedes any spawn, and the command answered from the session with one spawn and a clean quit, no second process coexisting.am instrument -e sessionPortdelayed 45s and--timeout 40000three times, commands took 19 s, 0 s, 1 s with onesessionPortspawn across all three; before the backoff every command paid the failed start.helperTransport: "instrumentation") and now answers from the session (15 s,helperTransport: "persistent-session").pidofreturns a pid while the instrumentation runs, empty afterquit.No provider-integration or coverage obligations selected locally; GitHub stays authoritative.