Repository navigation
fix(android): refuse close when stop fences expire - #3562
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
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
|
Reviewed 5d05dc2. I found no problems in the change: an expired close fence is now a typed refusal, and The open cubic P2 thread on the deadline at Not blocking, take or leave: 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. |
30942ef to
78213e2
Compare
5d05dc2 to
3800fea
Compare
|
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 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. |
78213e2 to
964e0a0
Compare
3800fea to
9f428e7
Compare
|
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 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. |
964e0a0 to
169cee2
Compare
9f428e7 to
d06caa8
Compare
169cee2 to
1ea2ab0
Compare
d06caa8 to
5a63e97
Compare
|
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. |
5a63e97 to
4dd17b4
Compare
|
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 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. |
912fd23 to
b98bfd2
Compare
4dd17b4 to
d3b7385
Compare
b98bfd2 to
cbae29e
Compare
|
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 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. |
cbae29e to
36aae30
Compare
554123e to
c6f525a
Compare
|
|
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 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. |
c6f525a to
0c24cfd
Compare
|
Rebased onto main
Gate: |
|
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 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. |
0c24cfd to
61091d5
Compare
|
Addressed the review of 0c24cfd at Real-emulator evidence (ReactNative_API_35, API 35,
The launcher package came from Rebase findings:
iOS Smoke previously failed at Gate: |
There was a problem hiding this comment.
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
|
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 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. |
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>
0f9cf63 to
bedf0a0
Compare
|
Addressed the review of 0f9cf63 at
Gate: |
|
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 |
Summary
ADB close could report success after either 2s foreground/process fence expired. Both now throw typed
COMMAND_FAILED/android-app-close-timeoutwithphase,packageNameandtimeoutMs. 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 useDeadline; 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 ownandroid-kill-process-survivedclassification. Apps Android restarts at once, such as the launcher, now fail close.help closeandcommands.mdstate this. Close tests moved toapp-lifecycle-close.test.ts. Related to #3077.Validation
Head
bedf0a066, rebased on mainfc224bc1e.pnpm check:affected --base origin/main --run: pass (2,803 tests).Live, API 35 emulator, built CLI through the daemon at
61091d578:close com.android.settingsopen com.android.settings --relaunchclose com.google.android.apps.nexuslauncher(frommCurrentFocus)android-app-close-timeout, phaseforeground,timeoutMs2000, 3.1s; launcher restarted🤖 Generated with Claude Code