fix(android): report the process lifecycle, quick replies and share intents - #29654
Draft
chrisnojima wants to merge 7 commits into
Draft
chrisnojima wants to merge 7 commits into
chrisnojima wants to merge 7 commits into
Conversation
chrisnojima
added this pull request to stack #29658
September 21, 2026 18:56
chrisnojima
changed the title
This was referenced Sep 21, 2026
MainActivity reported appUIActive/appUIInactive/appUIBackground from its own onResume/onStart/onStop, so any second activity -- or the activity being recreated -- looked to Go like the app leaving and re-entering the screen, and appWillExit fired on every destroy, configuration change included. AppLifecycleReporter observes ProcessLifecycleOwner instead, registered in Application.onCreate before any activity or service starts, so what Go sees is the process: an activity pause (a dialog, a permission prompt, a chooser) reports nothing. Only a finishing, non-recreating MainActivity reports willExit, and ProcessLifecycleOwner's debounced stop then lands after it, which is the order lifecycle.UIBackground's doc already describes. The event mapping is free of Android and gomobile types -- KeybaseLifecycleBind holds the Keybase calls -- so it runs in JVM tests. The file also carries sendQuickReply and runReceiverWork, which the quick reply path takes next.
…c job A quick reply ran on the broadcast's main thread and inside whileActive, which returns early when the app is foreground, so a reply typed in the shade while the app was on screen was silently dropped -- and a slow one blocked the main thread until the broadcast's 10s ran out. runReceiverWork runs the send on its own thread and finishes the broadcast when the work ends or the budget runs out, whichever is first, so the reply outlives neither. The push window is still opened here, transitionally, but without the foreground skip. postTextReply is the seam the send now goes through: it validates before it sends rather than after, returns a send failure instead of swallowing it, and treats a failed mark-as-read as what it is -- the reply went out. The notification then says what happened instead of always saying "Replied". MainApplication enqueued a new periodic BackgroundSyncWorker on every process start, so an existing install can carry many; scheduleBackgroundSync cancels them once and then keeps a single unique job whose period no launch resets.
tryHandleIntentWithRetry reposted handleIntent every 500ms for up to 10s and then gave up silently, but every path it was waiting on ends in JS calling shareListenersRegistered, which already re-ran the flush itself. The retry loop was therefore pure duplication of its own success condition; a share now parks in the activity until JS says it is ready to route one. captureIntent no longer parks an intent with nothing to flush, so a plain launch parks nothing. The file copy takes the activity's own Context, so the flush no longer waits on a live ReactContext either. Also drops the always-null permission listener and the dead isTestDevice (KbModule has the live copy).
…e active Native opened the push window in Kotlin, and skipped the whole push when Keybase.isAppStateForeground() said foreground. That check was a proxy for "the app already shows this message", but it also decided whether the push was unboxed and acked at all, so anything it called foreground was dropped -- never unboxed, never acked, no notification, nothing until the server's own fallback. Now that the process lifecycle is reported from ProcessLifecycleOwner's 700ms-debounced stop, that window covers the first 700ms after the user leaves the app, where a push would have been lost outright. inPushWindow moves the decision into the bind layer, which is the only place that can tell the two questions apart. The push is always handled; the window is an Android concern, since iOS suspends the app at the push's completion handler and needs no hold; and the work learns whether the UI is active, which is now used for one thing only -- displayOnce skips the display on Android, because the app is showing the message itself, and still acks so the server does not send its own. iOS displays either way: that display also removes the server's generic notification, and a local notification never shows while active. HandlePostTextReply takes the same window, so a quick reply typed in the shade holds the app up while it sends whatever the UI is doing. The service's Kotlin-side seen cache now guards only the fallback, and is bounded rather than growing for the life of the process.
AppPushWindowBegin/AppPushWindowEnd and WithBackgroundActive existed only to span the gap between native reporting the lifecycle and the bind layer wrapping push handling itself. inPushWindow closed that gap, so both sides go, and appPushWindow* leaves the gomobile surface entirely.
TestBackgroundNotificationOpensAndClosesPushWindow read the observed-state count straight after the setup transitions, but the recorder observes them from its own goroutine, so the baseline could be short by one. The iOS branch then read that late setup state as a state the push had caused: 8 failures in 12 runs of the two tests together, and none when the test ran alone.
chrisnojima
force-pushed
the
nojima/HOTPOT-as-06-android
branch
from
September 22, 2026 23:08
b634a1e to
82989ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 6/9, splitting #29637. Parent: #29653.
Why
Android reported the activity's lifecycle as if it were the app's. An activity is not the process: a configuration change destroys and recreates it, a second activity changes which one is on top, and a finishing activity's
onStopis not the process going away. So Go's notion of foreground/background was wrong in exactly the situations that matter for deciding whether background work may run.Master bugs this fixes
whileActiveearly-returns when foreground and nothing sends.What this changes
AppLifecycleReporterreports the process lifecycle viaProcessLifecycleOwner, replacing the activity-scoped stopgap #29651 used to keep the build working. Quick reply moves off the main thread and reports failures; a failed mark-as-read no longer fails the reply; duplicate sync jobs are cleared once and one is scheduled. Share intents are flushed when JS asks rather than on a poll — the retry loop, its scheduler and the ReactContext gate are gone.The push window also moves into Go here, and the transitional
AppPushWindowBegin/AppPushWindowEndshim #29651 added is deleted. Confirmed gone at the gomobile boundary:javapon the rebuilt AAR shows zeroappPushWindowsymbols.Why the push window is in this PR rather than #29656
Moving to
ProcessLifecycleOwnerintroduces a 700ms debounce on ON_STOP. During that windowisAppStateForeground()still returns true, andwhileActiveearly-returned while foreground — so a push arriving within ~700ms of pressing Home was never unboxed, never displayed and never acked. That hole is created by splitting these two changes apart; #29637 never had it, because there the push window is already in Go by then. Bringing the Go side forward closes it: the push is always handled and only the display is skipped while the UI is active.Behaviour worth knowing: such a push is unboxed into the inbox and acked, so nothing is lost and the server sends no duplicate, but no heads-up notification is shown. That is upstream behaviour, unchanged here. A related edge case: Go decides whether to display when the push window opens, so a push that arrives while the UI is active is not displayed even if the user leaves the app while Go is still handling it. Master drops that push too; the window between the two foreground checks is just shorter there. Left as is.
Merged with master's #29648
Master's #29648 ("tag notifications by uid too") conflicted with this PR's
KeybasePushNotificationListenerService.kt. Resolution:One deliberate deviation
captureIntentin #29637 early-returns for anything that is not a share. Taken literally here it would stop parking notification intents and break notification taps, because the notification path stays inhandleIntentuntil #29656. It parks on "share or notification bundle" instead, and collapses to the upstream form once #29656 removes that path.Verification
Android builds (
assembleDebug) and 13 unit tests pass —gobuildIfNeededrebuilds the AAR from this branch's Go, so the Kotlin is compiler-checked against the real bind surface.go build,go vet,gofmt,golangci-lint0 issues.yarn lint:allclean, unit suite green.The
UIBackgrounddoc comment claims Android reports it after a finishing activity'swillExit. Verified mechanically rather than argued:ProcessLifecycleOwner.TIMEOUT_MSis 700ms, so ON_STOP lands afteronDestroy. It was false under #29651's stopgap and is true now.Includes a fix for a flake that also exists on #29637 —
TestBackgroundNotificationOpensAndClosesPushWindowread its recorder baseline without settling first (8 failures in 12 runs alongside its sibling; passes alone). Worth backporting.