fix(android): return from an app open only after the launched app is readable - #2895
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 0693754. The code looks good. The change makes local Android Smoke Tests is still running with no failure yet, and this diff sits right on the route it exercises: every local Android open now runs a helper snapshot before returning, and smoke opens apps. If it fails, treat the failure as related to this PR until shown otherwise. Not blocking: does the CHANGELOG entry need to spell out the worst-case cost for a content-poor app (up to 3 capture attempts, then a helper runtime reset, plus a first-use helper install) or state that the open budget already covers it, and could I did not reproduce the live Android runs or the claimed mutations myself, so I'm trusting the PR body's counts and reading the tests and lifecycle.ts/snapshot.ts/snapshot-helper-runtime.ts by inspection rather than execution; I also did not verify that the open command's timeout budget covers the worst case of three 5 s captures plus a 30 s helper install. |
thymikee
added
the
ready-for-human
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Thermo-nuclear structural pass. The idea and the reuse are right: reusing the capture's own content verdict + bounded re-capture (rather than reimplementing a separate readiness wait) is the correct call, and the observeAndroidLaunch doc comment is honest about intent. But the wrapper around that one call collapses distinct failure modes into a single benign-looking value — the exact silent-fallback class AGENTS.md says to turn into an explicit boundary — and one of those modes means "our probe is broken", not "the app has no content." One blocker + one follow-on inline.
Two more, non-inline:
- Name the cross-platform union. This PR is the moment
postOpenObservationbecame cross-platform, but the value set is now spelled three ways: inline here, asLaunchObservationinplatform-apple/src/snapshot-observability.ts, and re-derived asNonNullable<OpenApplicationOutcome['timing']['postOpenObservation']>in the android lifecycle. Two hand-written copies of a wire value drift. ExportPostOpenObservationfromcontracts/application-lifecycle-runtime.ts, state what each value means for every owner (the new doc "see each platform owner" delegates a contract field's meaning to platform source), and give the "probe could not run" outcome from finding #1 its typed home here — not in error text. - The probe is inline, unbounded, and not treated as optional. The iOS half put it behind a named, injected
LaunchObservationPortwith its own bounded window (OBSERVATION_POLL_MS, <=5s, never extended); the Android call inlines it with no seam and inherits the whole request signal, so a busy app that blocks accessibility can now time out the open itself — the budget-burnage this PR set out to remove, just relocated. On top, theunobservableroute runsretireAndroidSnapshotHelperAfterContentFailure->resetRuntime+ a device-side force-stop, soopenreturns success having force-stopped the helper the very next read needs, and the tree that proved readability is thrown away. Scope it with its own bounded child signal, reuse it behind a port, and don't discard the evidence you just captured.
| try { | ||
| await interactor.snapshot({ appBundleId, signal: binding.signal }); | ||
| return 'observable'; | ||
| } catch { |
There was a problem hiding this comment.
This bare catch {} (no error binding) files three very different outcomes under one value, unobservable: (a) a genuine content verdict — the 3-attempt re-capture still saw a content-poor window; (b) the helper APK missing / unbuilt (androidSnapshotHelperUnavailableError); (c) a mechanism failure — adb died / helper install rejected / accessibility-timeout (androidSnapshotHelperCaptureError) — or a programming error inside the capture path. After signal.throwIfAborted(), everything in (b) / (c) / bugs becomes unobservable, i.e. "the app has no content yet", and open reports success. The repo already owns the discriminator for exactly this split — isUnreadableCaptureContentError from @agent-device/contracts/android-snapshot-quality, whose documented invariant is the opposite ("helper timeouts, adb failures, and missing artifacts remain fail-fast"), and which wait / replay honor but this open ignores (it isn't even imported here). Can you make the boundary explicit — catch (e) { binding.signal.throwIfAborted(); if (isUnreadableCaptureContentError(e)) return 'unobservable'; throw e; } — or return a distinct typed value carrying why the probe couldn't run? Right now a caller can't tell "app unreadable" from "probe broken" except by opening the daemon log with --debug. And add a test where the capture rejects with a mechanism failure (no content reason, or androidCaptureFailureReason: 'accessibility-timeout') — today it would assert the same 'unobservable' as a real content verdict, which is why none exists.
| input: OpenApplicationInput, | ||
| appBundleId: string | undefined, | ||
| ): Promise<NonNullable<OpenApplicationOutcome['timing']['postOpenObservation']>> { | ||
| if (!appBundleId) return 'not-eligible'; |
There was a problem hiding this comment.
not-eligible now means two things, and one of them silently cancels the whole guarantee this PR adds: a URL / deep-link open (correct), and whenever inferOpenedAppBundleId yields nothing — and that port is explicitly best-effort (platform-runtime-android-application-tools.ts catches and falls back to currentAppBundleId), so a failure to read the foreground package reports the same benign value as "this was never an app". It also diverges from the iOS half already merged: settleAppleOpen leaves postOpenObservation unset when there is no appBundleId and reserves not-eligible for "the device has no bridge to ask". Same shared enum, same missing-identity situation -> undefined on iOS but not-eligible on Android. Can you split "nothing to observe" (URL target -> leave unset, matching iOS) from "the launched package could not be identified" (a real coverage loss that deserves its own value or typed reason), and pin the distinction in a test rather than in prose?
|
Addressed at
|
aabf4e4 to
f11a928
Compare
|
Delta review addressed at
|
|
Reviewed at acb3731 and the code looks correct: the transient settleBy design replaces the old injected observation port, and the fix logic checks out against the tests. Android, Linux, and macOS Smoke passed at acb3731. The Coverage failure traces to this PR: Move the six new transient-capture tests into a separate test file named after the snapshot module they cover, so A few limits on what this covers: the 20-cold-open live rows referenced in the PR are dated to 31cd88b, before acb3731 changed how the window ends a capture, so they don't speak to the current behavior. The only live evidence at head is the passing Android Smoke job, and I didn't check whether it asserts Not blocking: the PR body still describes the removed injected port and the old "bounded by its own 6 s window" wording (worth updating to the transient settleBy design and naming the validated commit), one line in the |
|
The review of acb3731 is still clean, but the branch now conflicts with main. I removed |
thymikee
removed
the
ready-for-human
acb3731 to
a0c4c84
Compare
…readable am start -W returns when the activity draws its first frame, which can be a splash or an empty root while a React Native app still mounts. The first capture after a cold open --relaunch then saw a content-poor tree and spent its own budget on re-captures. The open now captures the launched app through the interactor snapshot path, whose content verdict and bounded re-capture decide readiness, and reports postOpenObservation. An app that stays unreadable, or a failed capture, still opens as unobservable. Refs #1571 (iOS half: #2838).
The open's launch capture now runs behind an injected launch observation port with a fixed 6 s window of its own, separate from the caller's cancellation. Only a content verdict, a system surface over the app, or the window running out reads as unobservable. Any other capture failure is a typed probe-failed result that the open reports and survives. The capture is transient: it borrows a running helper session and stops only a session it started, installs no helper, and does not retire the helper after a content verdict. A URL open reports no observation, and an app open whose package cannot be read reports app-unidentified. PostOpenObservation is one documented union in the lifecycle contract, shared with the Apple owner. Refs #1571
…per work The open's 6 s window reached the snapshot helper as an abort, so a window that closed during a cold helper start or a borrowed capture tore the helper down and left the next read to recover it. The window is now a settle deadline on the transient capture: the content re-capture loop starts no attempt after it, while helper start, capture and teardown keep their own budgets and only the caller's signal cancels them. The transient read replaces the injected port, the borrow session scope and the interactor-side install mapping: the capture itself keeps a session it found, releases one it started, installs no helper and does not retire it after a content verdict. Refs #1571
…covery A transient capture on a device without the current helper refused at the install check, and that refusal went through the capture failure handler, which logged an error and force-stopped the helper runtime on every new device's first open. The refusal now reaches the caller directly. The transient-capture tests move to their own file, so snapshot.test.ts stays under the test-file size ratchet. Refs #1571
a0c4c84 to
78214d2
Compare
|
Rebased on
|
|
This is ready to merge at 78214d2. The acb3731 conflict is resolved, and I found no blocking issues in this pass. Not blocking: in packages/platform-android/src/snapshot-helper-install.ts#L32, the new constant and predicate landed between the The live Android evidence at 78214d2 (idle, 16-burner, and new-device rows) is author-reported with no run artifact, and I could not reproduce it. I also could not confirm the new-device 0.5s row went through the new rethrow rather than the old force-stop route, though the timing fits the new path. I did not run the unit suite or the named mutations, so the regression judgement here comes from reading the old handler route, not from execution. I did not check whether Android Smoke asserts Smoke Tests, Coverage, and Repo Guards at 78214d2 are still pending. Android Smoke opens apps and now exercises the transient readiness capture on every local open, so a failure there should be treated as related until shown otherwise; Coverage runs the moved and new snapshot-transient-capture tests plus the size ratchet this change addresses, and Repo Guards covers the new export and file moves. No failure has been captured yet. Once those three runs finish green, please restore |
thymikee
added
the
ready-for-human
1 participant
Summary
Closes #1571 (iOS half: #2838).
am start -Wreturns at the first frame, while a React Native app may still show an empty root. So the firstsnapshot,waitor replay step after a coldopen --relaunchread an app with no content.A local Android app
opennow takes one transient snapshot of the launched app with{ settleBy: now + 6 s }. It re-captures up to 3 times, but starts none aftersettleBy. The window never cancels helper work; only the caller's signal does.A transient capture:
open_timing.postOpenObservationis a contract union shared with Apple:observableunobservableprobe-failed, with a typed failureapp-unidentifiedThe open always succeeds, and cancellation still rejects. Limrun and WebDriver are unchanged.
Validation
At
78214d2ba8, rebased ondc9ab863ad:snapshotAndroidtest. Examples: the window aborts the capture; a transient read installs, retires or resets the helper.observableopen, and all recovered.probe-failedin 0.5 s.pnpm check:affected --run: exit 0, 3046 tests.