Skip to content

fix(android): refuse close when stop fences expire - #3562

Merged
thymikee merged 8 commits into
mainfrom
t3/android-close-fences-3077
Oct 15, 2026
Merged

thymikee merged 8 commits into
mainfrom
t3/android-close-fences-3077

Conversation

@thymikee

@thymikee thymikee commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Summary

ADB close could report success after either 2s foreground/process fence expired. Both now throw typed COMMAND_FAILED / android-app-close-timeout with phase, packageName and timeoutMs. Relaunch stops before open or observation, both when closing a recorded owner and, since #3600, when force-stopping a package before launch. A probe that completes after the deadline counts as expired, including the 150ms stability recheck. Fences use Deadline; budgets unchanged (2s per phase, 50ms polling, 150ms stable absence).

The shared stop wait reports which fence expired; close and relaunch turn that into the refusal, while killApp (#2749) runs both waits as on main and keeps its own android-kill-process-survived classification. Apps Android restarts at once, such as the launcher, now fail close. help close and commands.md state this. Close tests moved to app-lifecycle-close.test.ts. Related to #3077.

Validation

Head bedf0a066, rebased on main fc224bc1e. pnpm check:affected --base origin/main --run: pass (2,803 tests).

main source, new tests Head
10 failed / 35 passed all pass

Live, API 35 emulator, built CLI through the daemon at 61091d578:

Command Result
close com.android.settings success, 2.6s
open com.android.settings --relaunch success, PID 3845 → 4159
close com.google.android.apps.nexuslauncher (from mCurrentFocus) android-app-close-timeout, phase foreground, timeoutMs 2000, 3.1s; launcher restarted

🤖 Generated with Claude Code

@thymikee
thymikee added this pull request to stack #3556 October 13, 2026 03:59
@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.25 MB 5.25 MB +747 B
Package (unpacked) 5.25 MB 5.25 MB +747 B
Package (download) 1.58 MB 1.58 MB +250 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 23.0 ms 23.0 ms +0.0 ms
CLI --help 61.2 ms 60.8 ms -0.4 ms

@thymikee
thymikee marked this pull request as ready for review October 13, 2026 04:20

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread packages/platform-android/src/app-lifecycle.ts Outdated
@thymikee

Copy link
Copy Markdown
Member Author

Reviewed 5d05dc2. I found no problems in the change: an expired close fence is now a typed refusal, and closeAndroidApp in the Android app lifecycle module is the right owner for it.

The open cubic P2 thread on the deadline at app-lifecycle.ts lines 552 and 578-580 does not apply, so please resolve it. Those returns fire only on a positive stop observation; every other outcome reaches the throw once the loop ends. A late probe that sees the app stopped has still confirmed the close, so refusing it would turn a real close into a false failure. #3562 (comment)

Not blocking, take or leave: close <app> also goes through closeAndroidApp, so on ADB-backed Android (local and Limrun) a lingering process now makes close fail after about 4 s with android-app-close-timeout. One line in the close and open --relaunch docs, and "ADB-backed Android" in the PR body, would cover it. The test at app-lifecycle-open.test.ts:207 asserts exactly 40 pidof calls, which ties it to the poll interval; asserting at least one call for the process phase and none for the foreground phase would be sturdier.

CI is green and there are no conflicts. The exact-head Android live job covers the confirmed-close relaunch path; the refusal branch is covered by unit tests. The PR can merge after its stacked base #3560.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 13, 2026
@thymikee
thymikee force-pushed the t3/settings-replay-corpus-3077 branch from 30942ef to 78213e2 Compare October 13, 2026 09:38
@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from 5d05dc2 to 3800fea Compare October 13, 2026 09:42
@thymikee

Copy link
Copy Markdown
Member Author

The code in 3800fea looks good. The close-timeout refusal is now fixed, and I found no problems in the diff. I did not run tests locally. I judged the regression from the pre-change code path and the base-vs-fixed table in the PR body. I also did not check whether the new refusal on plain close needs a CHANGELOG or docs entry.

The cubic-dev-ai thread on the early returns in app-lifecycle.ts does not apply, so you can resolve it: the early returns fire only on a positive stop observation, and every other outcome still reaches the throw (#3562 (comment)).

The Coverage failure looks unrelated to this PR. The eager-closure budget test fails because mechanics.ts reaches intent-arguments.ts through app-lifecycle.ts. That import is already in app-lifecycle.ts at the merge base 78213e2, and this PR adds no import. The extra edge comes from the stacked base. Smoke Tests is still queued, and this change only touches Android close, so it cannot reach iOS smoke. I did not re-run either job. I read the Coverage failure from the log excerpt and an import grep.

Before merge, the Coverage budget (mechanics.ts at 176 modules against a limit of 175) needs a fix in the stacked base (the #3555 intent-arguments edge) or a rebase. Smoke Tests also needs to finish green. There are no conflicts.

@thymikee
thymikee force-pushed the t3/settings-replay-corpus-3077 branch from 78213e2 to 964e0a0 Compare October 13, 2026 10:32
@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from 3800fea to 9f428e7 Compare October 13, 2026 10:39
@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready from the code side. At 9f428e7, the earlier clean review still holds. The only change since 3800fea is the switch of the intent-arguments import to a dynamic import, which does not change close behavior. The cubic-dev-ai thread on the early returns in app-lifecycle.ts does not apply, so you can resolve it: those returns fire only on a positive stop observation after the deadline, and every other outcome reaches the throw (#3562 (comment)).

Not blocking, and you can take or leave both: plain close on ADB Android now rejects with COMMAND_FAILED / android-app-close-timeout when the app is still foreground or running after 2s (https://github.com/callstack/agent-device/blob/9f428e7/packages/platform-android/src/app-lifecycle.ts#L546), and no help or docs line says so, so one line in the close/open --relaunch help would cover it. The dynamic import at https://github.com/callstack/agent-device/blob/9f428e7/packages/platform-android/src/app-lifecycle.ts#L292 is an eager-import fix carried over from #3555 and unrelated to close fences, so drop it when #3555 merges or keep it as disclosed.

Coverage is failing on this head, and I could not see its logs. The diff touches code that Coverage exercises, so the failure may be related, and I could not confirm whether the dynamic import fixes it. Smoke Tests, Repo Guards, Compatibility & Provenance and Typecheck & Package are still pending, and I could not confirm they will pass. Before merge, Coverage must go green or be explained, and those four jobs must finish. I did not run tests or check:affected, and there is no live run of the timeout refusal on this head. The earlier live run was at 5d05dc2, which has the same implementation. There are no conflicts.

@thymikee
thymikee force-pushed the t3/settings-replay-corpus-3077 branch from 964e0a0 to 169cee2 Compare October 13, 2026 11:29
@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from 9f428e7 to d06caa8 Compare October 13, 2026 11:31
@thymikee
thymikee force-pushed the t3/settings-replay-corpus-3077 branch from 169cee2 to 1ea2ab0 Compare October 13, 2026 11:48
@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from d06caa8 to 5a63e97 Compare October 13, 2026 11:52
@thymikee

Copy link
Copy Markdown
Member Author

Reviewed 5a63e97: the code looks good, and the close fences now refuse relaunch when they expire. The earlier review of 9f428e7 (#3562 (comment)) was also clean, and nothing new needs changing in this diff, which touches only the Android close fences in app-lifecycle.ts and their unit tests.

The one earlier inline thread, on the success-after-deadline path (#3562 (comment)), is resolved without a code change. It does not apply: the returns at app-lifecycle.ts:547 and :572-575 happen only after a positive stop observation, and every other path reaches the throw, so you can leave it resolved.

CI on this head is still running: 9 of 18 checks are queued or in progress, including Analyze, Smoke Tests, Repo Guards, Compatibility & Provenance and Integration Tests, and none has failed so far. The PR body says the earlier Coverage failure was an unchanged Apple helper recovery test (bridge-request-deadline), which this diff does not touch. I did not re-run the tests locally, and the base-versus-fix counts come from the PR body. The refusal path is covered by the unit tests with a fake ADB provider and fake timers. I did not see a live Android device run for this head, since the cited run 38379787613 is on 3800fea.

The only thing left before merge is for the hosted checks on 5a63e97 to finish green, with no code changes needed.

@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from 5a63e97 to 4dd17b4 Compare October 13, 2026 13:19
@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready. At 4dd17b4 the code still looks right, and the patch is unchanged since the earlier clean review at 5a63e97 (#3562 (comment)). The rebase only brought in iOS simulator e2e and replay changes from the base, which this PR does not touch.

Not blocking: Android close and open --relaunch now fail with COMMAND_FAILED (reason android-app-close-timeout) when force-stop does not take effect within 2s, where they used to report success, so if the close or open --relaunch help topic describes how close completes, add one line saying Android close fails when the app stays in the foreground or its process keeps running after force-stop. You can take this or leave it.

The earlier thread at #3562 (comment) does not apply. It was resolved without a code change, because the success returns at app-lifecycle.ts:547 and :572-575 already cover it, and the patch has not changed since.

The Smoke Tests job is still queued, so I could not read its result. The PR diff touches only Android lifecycle code and its unit tests, so it does not overlap that job's iOS route. I did not run the Android tests locally. I relied on the identical range-diff and the earlier review. No device run was captured, since this is a typed error on an ADB timeout path and the unit tests use a fake ADB provider with fake timers. There are no conflicts. The only thing left before merge is for the queued Smoke Tests job to finish.

@thymikee
thymikee force-pushed the t3/settings-replay-corpus-3077 branch 2 times, most recently from 912fd23 to b98bfd2 Compare October 13, 2026 17:40
@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from 4dd17b4 to d3b7385 Compare October 13, 2026 18:20
@thymikee
thymikee force-pushed the t3/settings-replay-corpus-3077 branch from b98bfd2 to cbae29e Compare October 13, 2026 18:20
@thymikee

Copy link
Copy Markdown
Member Author

The fixes since the earlier clean review (4dd17b4) look right at 554123e, and all 18 checks pass with no conflicts. The earlier cubic-dev-ai P2 thread (#3562 (comment)) is fixed at this head: both fence loops now go to the typed timeout when a probe returns "not foreground" or "no process" at or after the deadline, so you can resolve it.

One design question before I mark this ready. The deadline rule now lives in four hand-written Date.now() >= deadline guards in https://github.com/callstack/agent-device/blob/554123e/packages/platform-android/src/app-lifecycle.ts#L574, and the one at line 574 changes nothing, because the line 577 check or the loop condition gives the same result. Could one Deadline.fromTimeoutMs(...) per phase, plus one helper that returns a probe result only if !deadline.isExpired() after the await, replace all four guards? If you kept the separate guards on purpose, please say why.

I did not run the focused tests on d3b7385 or on the head. I traced the rows by hand, and they match your 7 failing / 29 passing split. No live device run covers these timing boundaries; fake timers are the realistic proof for the exact-deadline edges, and the device route was covered by the earlier review.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 13, 2026
@thymikee
thymikee force-pushed the t3/settings-replay-corpus-3077 branch from cbae29e to 36aae30 Compare October 14, 2026 06:21
@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from 554123e to c6f525a Compare October 14, 2026 06:24
@thymikee thymikee changed the title fix(android): refuse relaunch when close fences expire fix(android): refuse close when stop fences expire Oct 14, 2026
@thymikee
thymikee removed this pull request from stack #3556 October 14, 2026 06:25
@thymikee
thymikee changed the base branch from t3/settings-replay-corpus-3077 to main October 14, 2026 06:25
@thymikee thymikee closed this Oct 14, 2026
@thymikee thymikee reopened this Oct 14, 2026
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-3562/

Built to branch gh-pages at 2026-10-15 10:47 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@thymikee

Copy link
Copy Markdown
Member Author

The Android close and relaunch fix at c6f525a looks correct, and the earlier timing concern from the 554123e review is fixed. The probe now drops any result that arrives at or after expiry, and the stability recheck runs after the 150ms sleep, so both paths reach the typed refusal. All 23 checks pass, including Android Smoke Tests (run 38524025687). That run covers the accepted close and relaunch path only.

The evidence gap from the 554123e review is still open. website/docs/docs/commands.md:93 says that closing the launcher fails with a typed refusal, but no run on a real emulator has reached that branch. The refusal is proven only with a fake ADB and fake timers. If the emulator's mCurrentFocus briefly shows a non-launcher window after force-stop, the foreground fence would accept the close, and the docs line would be wrong. Could you run close <launcher package> on an Android emulator at c6f525a through the repo CLI and daemon, reading the package from dumpsys window mCurrentFocus? Please paste the output. It should show COMMAND_FAILED with details.reason android-app-close-timeout, plus phase, packageName and timeoutMs 2000, after about 2-4s. If the close succeeds instead, please drop the launcher sentence from commands.md:93 and the PR body.

The cubic-dev-ai thread on the expiry probe (#3562 (comment)) no longer applies, so please resolve it. No other threads need action.

Before merge, we need that emulator run showing the typed refusal, or the launcher claim removed from the docs.

@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from c6f525a to 0c24cfd Compare October 14, 2026 16:44
@thymikee

Copy link
Copy Markdown
Member Author

Rebased onto main 063b1b86f at 0c24cfdd7 to clear the conflict with #3600:

Gate: pnpm check:affected --base origin/main --run passed (377 files / 2,800 tests).

@thymikee

Copy link
Copy Markdown
Member Author

Thanks for the rebase. The conflict from the earlier review (c6f525a) is fixed, and the cubic-dev-ai P2 thread on the stop fences is fixed at 0c24cfd: both fences now go through probeBeforeDeadline, and the early exits reach the typed throw.

One item is still open. The earlier review asked for one real-emulator run through the repo CLI and daemon of close <launcher package>, with the package read from dumpsys window mCurrentFocus. The run should show COMMAND_FAILED, details.reason android-app-close-timeout, phase, packageName and timeoutMs 2000, after about 2-4s. The rebase comment adds no such output, and the PR body still says "Current-head native verification pending". website/docs/docs/commands.md:94 still says apps that Android restarts at once, such as the launcher, fail this way. Close used to succeed here and now returns a typed refusal. So far the refusal has only been seen with a fake ADB and fake timers. If mCurrentFocus briefly shows a non-launcher window after force-stop, the foreground fence accepts the close and that docs line is false. Could you paste real-emulator CLI output at 0c24cfd for close <launcher package> through the daemon, plus one ordinary-app close that succeeds and one open --relaunch that completes? Or remove the launcher sentence from commands.md:94 and from the PR body.

The cubic-dev-ai P2 thread at #3562 (comment) is fixed at 0c24cfd, so you can resolve it.

Smoke Tests (iOS) fails at live-snapshot-depth-frontier.ts:169 with a sparse-tree snapshot on the tester app. This diff only touches Android app lifecycle code, its tests, the close help string and commands.md, so I think this failure is unrelated. I did not check whether it also fails on main. The other jobs were still queued or running when I looked, and Coverage has no failed-step log. I did not run any tests myself.

Before merge, the emulator output or the docs change is needed, and the remaining CI jobs must finish.

@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from 0c24cfd to 61091d5 Compare October 15, 2026 09:14
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the review of 0c24cfd at 61091d578 (rebased on main 8c01a3787).

Real-emulator evidence (ReactNative_API_35, API 35, emulator-5554; built CLI through the daemon at 61091d578, isolated state dir):

Command Result
close com.android.settings success, 2.6s
open com.android.settings --relaunch success, appBundleId: com.android.settings, PID 3845 → 4159
close com.google.android.apps.nexuslauncher COMMAND_FAILED, reason: android-app-close-timeout, phase: foreground, packageName: com.google.android.apps.nexuslauncher, timeoutMs: 2000, after 3.1s

The launcher package came from dumpsys window (mCurrentFocus=Window{79c5600 u0 com.google.android.apps.nexuslauncher/...NexusLauncherActivity}). After the refused close, mCurrentFocus showed a new launcher window (c3edad5): Android restarted it, and no non-launcher window was accepted in between. So the commands.md sentence stays. The daemon stopped gracefully and the emulator is shut down.

Rebase findings:

  • Main's killApp (feat(maestro): support killApp via close mode #2749) shares the stop wait. The fence error was pre-empting its android-kill-process-survived classification, and main's killAndroidApp fails when the process survives the kill failed. The wait now reports which fence expired: close and relaunch throw android-app-close-timeout, and kill keeps main's behavior.
  • Coverage failed on the test-file size ratchet (app-lifecycle-open.test.ts at 1,050 lines). The close and stop-fence tests moved unchanged to app-lifecycle-close.test.ts.
  • Fences use main's readAndroidPackageProcessState; unknown still counts as not running, as on main.

iOS Smoke previously failed at live-snapshot-depth-frontier.ts:169 (sparse depth-1 snapshot). This diff has no iOS code; the new head reruns it.

Gate: pnpm check:affected --base origin/main --run passed (378 files / 2,802 tests).

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread packages/platform-android/src/app-lifecycle.ts
Comment thread packages/platform-android/src/app-lifecycle.ts
Comment thread website/docs/docs/commands.md
@thymikee

Copy link
Copy Markdown
Member Author

Thanks, the earlier review at 0c24cfd is now answered at 0f9cf63. The stop fences refuse close with a typed reason when they expire, and the live emulator table covers the launcher refusal in the foreground phase. Production code at 61091d5, where that table was taken, is the same as at this head. All 22 checks pass, and there are no conflicts.

One item is still open. The cubic-dev-ai P2 thread on the foreground fence (#3562 (comment)) still applies. At app-lifecycle.ts:618 the wait returns 'foreground' before the process fence runs, so kill then samples pidof only once at :595-596. On main, kill fell through to the 2s process wait. The impact is narrow, because kill already refuses a resumed target at :575. Could killAndroidPackage call waitForAndroidPackageProcessGone directly? The foreground fence adds nothing for a background-only kill.

The other two cubic-dev-ai threads do not apply, so you can resolve them. The P2 on unknown mapping to not-running (#3562 (comment)) matches main's !== 'running' at 8c01a37, so this PR does not change it; it is a fair follow-up for probe failures. The P3 on the commands.md wording (#3562 (comment)) is grammatical as written.

Not blocking: the last commit retires a stale APPROVED_OVER_CEILING row for the Maestro replay backend at eager-closure-budgets.ts:135, which belongs to #3623 on main; keep it only if the gate needs it on this base.

I did not run the tests. The process-phase refusal has no live run yet, only unit tests with fake ADB, since the launcher run stops in the foreground phase.

thymikee and others added 8 commits October 15, 2026 12:41
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#3600 routed package relaunch through the same stop helper as close, so the
fence refusal now also guards relaunch before any launch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stop wait now reports which fence expired instead of throwing. Close and
relaunch turn that into android-app-close-timeout; killApp keeps its own
android-kill-process-survived classification, as on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… file

app-lifecycle-open.test.ts crossed the test-file size tripwire; the close and
stop-fence cases move unchanged to app-lifecycle-close.test.ts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stop wait returned at an expired foreground fence, so killApp skipped the
process wait and sampled pidof once. Kill now runs both waits as on main and
classifies a survivor itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@thymikee
thymikee force-pushed the t3/android-close-fences-3077 branch from 0f9cf63 to bedf0a0 Compare October 15, 2026 10:46
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the review of 0f9cf63 at bedf0a066 (rebased on main fc224bc1e):

  • Foreground-fence thread (fixed): killApp no longer uses the shared stop wait. It runs both waits as on main, foreground then process, and then classifies a survivor with android-kill-process-survived. I kept main's foreground wait rather than dropping it, so feat(maestro): support killApp via close mode #2749's adb-sequence tests stay unchanged. New test killAndroidApp waits out a slow exit even while stale focus still names the target: on the previous head it fails with am kill did not stop (pidof sampled once after the foreground fence expired), and it passes now.
  • unknown → not running: unchanged from main (!== 'running'); resolved, with a note that it belongs in a follow-up.
  • commands.md wording: kept; resolved.
  • Stale approval row: the rebase dropped my commit, because refactor(interaction): move the moving-target decision policy into the runtime gate #3631 already removed the row on main.

Gate: pnpm check:affected --base origin/main --run passed (2,803 tests). The live emulator table from 61091d578 still applies: close and relaunch production code is unchanged since then, and only killApp's wait sequence moved back to main's.

@thymikee

Copy link
Copy Markdown
Member Author

The 0f9cf63 finding is fixed at bedf0a0, and I found no new problems in the code. CI is green: 22 checks, none failing. There are no conflicts, and nothing else stands between this PR and merge.

Evidence limits: I did not run the tests. The claim that the kill regression test fails on the previous head comes from tracing the code, not from running it. No live run covers the process-phase close refusal or the relaunch refusal, so both rest on the fake-ADB unit tests, as in the earlier review. The live table was taken at 61091d5. It still applies because the close and relaunch code is byte-identical at head. killApp has no live run, but its wait sequence matches main again.

On the bot threads: two P2 threads are fixed at head. #3562 (comment) (late probe result) is fixed because the late probe result is now discarded and the stable recheck checks the deadline. #3562 (comment) (process-gone wait) is fixed because killAndroidPackage now waits for the process to be gone after the foreground wait, and the new kill test covers it. You can resolve both. #3562 (comment) (unknown as not-running) can be resolved: it matches the existing !== 'running' check on main, so it fits a follow-up. #3562 (comment) (commands.md wording) can be resolved too: the wording is fine as written.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 15, 2026
@thymikee
thymikee merged commit f9aef84 into main Oct 15, 2026
22 checks passed
@thymikee
thymikee deleted the t3/android-close-fences-3077 branch October 15, 2026 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant