From edb19c039aa7291d3ea9f992a60495a4c2104b79 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 12:59:46 -0400 Subject: [PATCH 1/7] fix(android): report the process lifecycle, not the activity's 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. --- shared/android/app/build.gradle | 2 + .../keybase/ossifrage/AppLifecycleReporter.kt | 109 ++++++++++ .../keybase/ossifrage/KeybaseLifecycleBind.kt | 17 ++ .../java/io/keybase/ossifrage/MainActivity.kt | 14 +- .../io/keybase/ossifrage/MainApplication.kt | 6 + .../ossifrage/AppLifecycleReporterTest.kt | 202 ++++++++++++++++++ 6 files changed, 337 insertions(+), 13 deletions(-) create mode 100644 shared/android/app/src/main/java/io/keybase/ossifrage/AppLifecycleReporter.kt create mode 100644 shared/android/app/src/main/java/io/keybase/ossifrage/KeybaseLifecycleBind.kt create mode 100644 shared/android/app/src/test/java/io/keybase/ossifrage/AppLifecycleReporterTest.kt diff --git a/shared/android/app/build.gradle b/shared/android/app/build.gradle index 9327e574c2cb..e205353b3d61 100644 --- a/shared/android/app/build.gradle +++ b/shared/android/app/build.gradle @@ -171,6 +171,8 @@ dependencies { implementation 'com.android.installreferrer:installreferrer:2.2' implementation "androidx.lifecycle:lifecycle-common-java8:2.10.0" implementation "androidx.lifecycle:lifecycle-process:2.10.0" + + testImplementation "junit:junit:4.13.2" } // This requires a google-services.json file locally. Drop it in diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/AppLifecycleReporter.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/AppLifecycleReporter.kt new file mode 100644 index 000000000000..f644bf3e399e --- /dev/null +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/AppLifecycleReporter.kt @@ -0,0 +1,109 @@ +package io.keybase.ossifrage + +import androidx.lifecycle.DefaultLifecycleObserver +import androidx.lifecycle.LifecycleOwner +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit + +// The Go lifecycle entry points. Kept free of Android and gomobile types so +// the event mapping runs in JVM tests. +internal interface LifecycleBind { + fun uiActive() + fun uiInactive() + fun uiBackground() + fun willExit() +} + +// Reports the app's process lifecycle to Go as events; Go decides the state. +// +// Events reach Go on the calling thread, before the callback returns: every Go +// lifecycle call returns at once, except willExit, whose warning about +// messages still sending reads the outbox. +// +// Only the process lifecycle counts. Activity pauses (dialogs, permission +// prompts, choosers, the photo picker sheet) report nothing, not even +// UIInactive: only the process lifecycle decides what Go sees. A full-screen +// picker or camera stops the process like any other exit. +// +// A process started without UI (a push, a quick reply) reports nothing: Go +// starts in the background. +internal class AppLifecycleReporter( + private val bind: LifecycleBind, + private val log: (String) -> Unit, +) : DefaultLifecycleObserver { + override fun onStart(owner: LifecycleOwner) { + report("uiInactive") { bind.uiInactive() } + } + + override fun onResume(owner: LifecycleOwner) { + report("uiActive") { bind.uiActive() } + } + + override fun onStop(owner: LifecycleOwner) { + report("uiBackground") { bind.uiBackground() } + } + + // Activity recreation and a task moved to the back are not an exit. + fun onMainActivityDestroy(isFinishing: Boolean, isChangingConfigurations: Boolean) { + if (!isFinishing || isChangingConfigurations) { + return + } + report("willExit") { bind.willExit() } + } + + private fun report(event: String, call: () -> Unit) { + log("AppLifecycleReporter: $event") + try { + call() + } catch (e: Exception) { + log("AppLifecycleReporter: $event failed: $e") + } + } +} + +// Sends a notification quick reply. Returns the text for the replied +// notification. +internal fun sendQuickReply(error: (String, Throwable) -> Unit, send: () -> Unit): String = + try { + send() + QUICK_REPLY_SENT + } catch (e: Exception) { + error("Failed to send quick reply", e) + QUICK_REPLY_FAILED + } + +// Runs a receiver's work off the main thread and calls finish exactly once: +// when the work ends or when budgetMs runs out, whichever is first, so the +// broadcast never outlives its limit. Work that overruns keeps going. An +// exception from work is logged, since it would otherwise kill the process. +internal fun runReceiverWork( + budgetMs: Long, + start: (Runnable) -> Unit, + warn: (String) -> Unit, + error: (String, Throwable) -> Unit, + finish: () -> Unit, + work: () -> Unit, +) { + val done = CountDownLatch(1) + start(Runnable { + try { + work() + } catch (e: Exception) { + error("runReceiverWork: work failed", e) + } finally { + done.countDown() + } + }) + start(Runnable { + try { + if (!done.await(budgetMs, TimeUnit.MILLISECONDS)) { + warn("runReceiverWork: still running after ${budgetMs}ms, finishing the broadcast") + } + } finally { + finish() + } + }) +} + +internal const val QUICK_REPLY_SENT = "Replied" +internal const val QUICK_REPLY_FAILED = "Couldn't send reply" diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/KeybaseLifecycleBind.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/KeybaseLifecycleBind.kt new file mode 100644 index 000000000000..34f0a34e8ca1 --- /dev/null +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/KeybaseLifecycleBind.kt @@ -0,0 +1,17 @@ +package io.keybase.ossifrage + +import android.content.Context +import android.os.Bundle +import keybase.Keybase + +internal class KeybaseLifecycleBind(private val context: Context) : LifecycleBind { + override fun uiActive() = Keybase.appUIActive() + + override fun uiInactive() = Keybase.appUIInactive() + + override fun uiBackground() { + Keybase.appUIBackground(KBPushNotifier(context, Bundle())) + } + + override fun willExit() = Keybase.appWillExit(KBPushNotifier(context, Bundle())) +} diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt index c93a7982e1e0..510722697005 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt @@ -149,33 +149,21 @@ class MainActivity : ReactActivity() { return filePath } - // Native reports only what the UI is doing; Go derives the app state from - // these reports and the holds background work opens (go/libkb/lifecycle). override fun onResume() { NativeLogger.info("Activity onResume") super.onResume() - Keybase.appUIActive() handleIntent() } override fun onStart() { NativeLogger.info("Activity onStart") super.onStart() - Keybase.appUIInactive() - } - - override fun onStop() { - NativeLogger.info("Activity onStop") - super.onStop() - // The token is for iOS, which has to wait out Go's background task - // before the OS suspends it; Android keeps the process running. - Keybase.appUIBackground(KBPushNotifier(this, Bundle())) } override fun onDestroy() { NativeLogger.info("Activity onDestroy") super.onDestroy() - Keybase.appWillExit(KBPushNotifier(this, Bundle())) + (application as MainApplication).lifecycleReporter.onMainActivityDestroy(isFinishing, isChangingConfigurations) } private var cachedIntent: Intent? = null diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/MainApplication.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/MainApplication.kt index 65de3e828c1c..6c0fd5f9ccfa 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/MainApplication.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/MainApplication.kt @@ -52,9 +52,15 @@ class MainApplication : Application(), ReactApplication { } + internal val lifecycleReporter by lazy { + AppLifecycleReporter(KeybaseLifecycleBind(this)) { NativeLogger.info(it) } + } + override fun onCreate() { NativeLogger.info("MainApplication created") super.onCreate() + // Before any activity or service starts, so no process event is missed. + ProcessLifecycleOwner.get().lifecycle.addObserver(lifecycleReporter) try { DefaultNewArchitectureEntryPoint.releaseLevel = ReleaseLevel.valueOf(BuildConfig.REACT_NATIVE_RELEASE_LEVEL.uppercase()) } catch (e: IllegalArgumentException) { diff --git a/shared/android/app/src/test/java/io/keybase/ossifrage/AppLifecycleReporterTest.kt b/shared/android/app/src/test/java/io/keybase/ossifrage/AppLifecycleReporterTest.kt new file mode 100644 index 000000000000..0bc3013d0490 --- /dev/null +++ b/shared/android/app/src/test/java/io/keybase/ossifrage/AppLifecycleReporterTest.kt @@ -0,0 +1,202 @@ +package io.keybase.ossifrage + +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import java.util.Collections +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicReference +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +private class FakeBind : LifecycleBind { + val calls: MutableList = Collections.synchronizedList(mutableListOf()) + val threads: MutableSet = Collections.synchronizedSet(mutableSetOf()) + + private fun record(call: String) { + threads.add(Thread.currentThread()) + calls.add(call) + } + + override fun uiActive() = record("uiActive") + + override fun uiInactive() = record("uiInactive") + + override fun uiBackground() = record("uiBackground") + + override fun willExit() = record("willExit") +} + +private object Owner : LifecycleOwner { + override val lifecycle: Lifecycle + get() = throw UnsupportedOperationException() +} + +class AppLifecycleReporterTest { + private val bind = FakeBind() + private val reporter = AppLifecycleReporter(bind) {} + + private fun launch() { + reporter.onCreate(Owner) + reporter.onStart(Owner) + reporter.onResume(Owner) + } + + private fun stop() { + reporter.onPause(Owner) + reporter.onStop(Owner) + } + + private fun calls(): List = bind.calls.toList() + + @Test + fun processStartAndStopReportEventsInOrder() { + launch() + stop() + reporter.onStart(Owner) + reporter.onResume(Owner) + assertEquals( + listOf( + "uiInactive", "uiActive", + "uiBackground", + "uiInactive", "uiActive", + ), + calls(), + ) + } + + @Test + fun eventsReachGoOnTheCallingThreadBeforeTheCallbackReturns() { + reporter.onStart(Owner) + assertEquals(listOf("uiInactive"), calls()) + reporter.onResume(Owner) + assertEquals(listOf("uiInactive", "uiActive"), calls()) + reporter.onStop(Owner) + assertEquals(listOf("uiInactive", "uiActive", "uiBackground"), calls()) + assertEquals(setOf(Thread.currentThread()), bind.threads.toSet()) + } + + @Test + fun dialogOrPermissionPromptPauseNeverBackgrounds() { + launch() + reporter.onPause(Owner) + reporter.onResume(Owner) + reporter.onPause(Owner) + assertEquals(listOf("uiInactive", "uiActive", "uiActive"), calls()) + } + + // A full-screen picker or camera stops the process like any other exit. + @Test + fun fullScreenPickerBackgroundsAndReturningForegrounds() { + launch() + stop() + reporter.onStart(Owner) + reporter.onResume(Owner) + assertEquals( + listOf( + "uiInactive", "uiActive", + "uiBackground", + "uiInactive", "uiActive", + ), + calls(), + ) + } + + @Test + fun onlyAFinishingActivityExits() { + launch() + reporter.onMainActivityDestroy(isFinishing = false, isChangingConfigurations = false) + reporter.onMainActivityDestroy(isFinishing = true, isChangingConfigurations = true) + assertEquals(listOf("uiInactive", "uiActive"), calls()) + reporter.onMainActivityDestroy(isFinishing = true, isChangingConfigurations = false) + stop() + reporter.onStart(Owner) + reporter.onResume(Owner) + assertEquals( + listOf( + "uiInactive", "uiActive", + "willExit", "uiBackground", + "uiInactive", "uiActive", + ), + calls(), + ) + } +} + +class SendQuickReplyTest { + private val errors = mutableListOf>() + + private fun send(send: () -> Unit) = sendQuickReply({ msg, e -> errors.add(msg to e) }, send) + + @Test + fun replySends() { + var sent = false + assertEquals(QUICK_REPLY_SENT, send { sent = true }) + assertTrue(sent) + assertTrue(errors.isEmpty()) + } + + @Test + fun failedReplyIsNotReportedAsRepliedAndLogsTheException() { + val failure = IllegalStateException("outbox full") + assertEquals(QUICK_REPLY_FAILED, send { throw failure }) + assertEquals(listOf("Failed to send quick reply" to failure), errors.toList()) + } +} + +class RunReceiverWorkTest { + private val finishes = AtomicInteger() + private val finished = CountDownLatch(1) + private val warnings = Collections.synchronizedList(mutableListOf()) + private val errors = Collections.synchronizedList(mutableListOf()) + + private fun run(budgetMs: Long, work: () -> Unit) = runReceiverWork( + budgetMs, + { r -> Thread(r).start() }, + { warnings.add(it) }, + { _, e -> errors.add(e) }, + { + finishes.incrementAndGet() + finished.countDown() + }, + work, + ) + + @Test(timeout = 10_000) + fun finishesAfterTheWorkOffTheCallingThread() { + val ranOn = AtomicReference() + run(10_000) { ranOn.set(Thread.currentThread()) } + assertTrue(finished.await(5, TimeUnit.SECONDS)) + assertTrue(ranOn.get() != Thread.currentThread()) + Thread.sleep(50) + assertEquals(1, finishes.get()) + assertTrue(warnings.isEmpty()) + } + + @Test(timeout = 10_000) + fun finishesAndLogsWhenTheWorkThrows() { + val failure = IllegalStateException("boom") + run(10_000) { throw failure } + assertTrue(finished.await(5, TimeUnit.SECONDS)) + assertEquals(listOf(failure), errors.toList()) + assertEquals(1, finishes.get()) + } + + @Test(timeout = 10_000) + fun finishesAtTheBudgetWhileTheWorkIsStillRunning() { + val release = CountDownLatch(1) + val workDone = CountDownLatch(1) + run(100) { + release.await(5, TimeUnit.SECONDS) + workDone.countDown() + } + assertTrue(finished.await(5, TimeUnit.SECONDS)) + assertEquals(1, warnings.size) + release.countDown() + assertTrue(workDone.await(5, TimeUnit.SECONDS)) + Thread.sleep(50) + assertEquals("finishes once", 1, finishes.get()) + } +} From d6327241784f27c179d715d1de460088a0096ed2 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 13:00:08 -0400 Subject: [PATCH 2/7] fix(android): send quick replies off the main thread and keep one sync 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. --- go/bind/notifications.go | 36 ++++--- go/bind/notifications_test.go | 100 ++++++++++++++++++ .../ossifrage/ChatBroadcastReceiver.kt | 58 ++++++---- .../io/keybase/ossifrage/MainApplication.kt | 72 +++++++++++-- .../modules/BackgroundSyncSchedule.kt | 29 +++++ .../modules/BackgroundSyncScheduleTest.kt | 64 +++++++++++ 6 files changed, 313 insertions(+), 46 deletions(-) create mode 100644 go/bind/notifications_test.go create mode 100644 shared/android/app/src/main/java/io/keybase/ossifrage/modules/BackgroundSyncSchedule.kt create mode 100644 shared/android/app/src/test/java/io/keybase/ossifrage/modules/BackgroundSyncScheduleTest.kt diff --git a/go/bind/notifications.go b/go/bind/notifications.go index f3ca13136805..3298c263805a 100644 --- a/go/bind/notifications.go +++ b/go/bind/notifications.go @@ -118,34 +118,40 @@ func HandlePostTextReply(strConvID, tlfName string, intMessageID int, body strin ctx := context.Background() defer kbCtx.CTrace(ctx, "HandlePostTextReply", &err)() defer func() { err = flattenError(err) }() - outboxID, err := storage.NewOutboxID() + return postTextReply(ctx, globals.NewContext(kbCtx, kbChatCtx), strConvID, tlfName, intMessageID, body) +} + +// postTextReply sends a notification quick reply and marks the conversation +// read. The send is nonblocking: an error means the message couldn't be +// queued, not that delivery failed. +func postTextReply(ctx context.Context, gc *globals.Context, strConvID, tlfName string, intMessageID int, + body string, +) error { + convID, err := chat1.MakeConvID(strConvID) if err != nil { return err } - convID, err := chat1.MakeConvID(strConvID) + if intMessageID < 0 { + return fmt.Errorf("invalid message ID: %d", intMessageID) + } + uid, err := utils.AssertLoggedInUID(ctx, gc) if err != nil { return err } - _, err = kbCtx.ChatHelper.SendTextByIDNonblock(context.Background(), convID, tlfName, body, &outboxID, nil) - - kbCtx.Log.CDebugf(ctx, "Marking as read from QuickReply: convID: %s", strConvID) - gc := globals.NewContext(kbCtx, kbChatCtx) - uid, err := utils.AssertLoggedInUID(ctx, gc) + outboxID, err := storage.NewOutboxID() if err != nil { return err } - - if intMessageID < 0 { - return fmt.Errorf("invalid message ID: %d", intMessageID) + if _, err := gc.ChatHelper.SendTextByIDNonblock(ctx, convID, tlfName, body, &outboxID, nil); err != nil { + return err } + gc.Log.CDebugf(ctx, "Marking as read from QuickReply: convID: %s", strConvID) msgID := chat1.MessageID(intMessageID) - if err = kbChatCtx.InboxSource.MarkAsRead(context.Background(), convID, uid, &msgID, false /* forceUnread */); err != nil { - kbCtx.Log.CDebugf(ctx, "Failed to mark as read from QuickReply: convID: %s. Err: %s", strConvID, err) - // We don't want to fail this method call just because we couldn't mark it as aread - err = nil + if err := gc.InboxSource.MarkAsRead(ctx, convID, uid, &msgID, false /* forceUnread */); err != nil { + // The reply went out; failing to mark it read doesn't fail the reply. + gc.Log.CDebugf(ctx, "Failed to mark as read from QuickReply: convID: %s. Err: %s", strConvID, err) } - return nil } diff --git a/go/bind/notifications_test.go b/go/bind/notifications_test.go new file mode 100644 index 000000000000..83b5a07679e9 --- /dev/null +++ b/go/bind/notifications_test.go @@ -0,0 +1,100 @@ +package keybase + +import ( + "context" + "errors" + "testing" + + "github.com/keybase/client/go/chat/globals" + "github.com/keybase/client/go/chat/types" + "github.com/keybase/client/go/libkb" + "github.com/keybase/client/go/protocol/chat1" + "github.com/keybase/client/go/protocol/gregor1" + "github.com/keybase/client/go/protocol/keybase1" + "github.com/stretchr/testify/require" +) + +type replyChatHelper struct { + libkb.ChatHelper + sendErr error + sent []string +} + +func (h *replyChatHelper) SendTextByIDNonblock(_ context.Context, _ chat1.ConversationID, _ string, text string, + _ *chat1.OutboxID, _ *chat1.MessageID, +) (chat1.OutboxID, error) { + if h.sendErr != nil { + return nil, h.sendErr + } + h.sent = append(h.sent, text) + return nil, nil +} + +type replyInboxSource struct { + types.InboxSource + markErr error + marked []chat1.MessageID +} + +func (s *replyInboxSource) MarkAsRead(_ context.Context, _ chat1.ConversationID, _ gregor1.UID, + msgID *chat1.MessageID, _ bool, +) error { + s.marked = append(s.marked, *msgID) + return s.markErr +} + +func TestPostTextReply(t *testing.T) { + const convID = "0000bbbbccccddddeeeeffff0000aaaabbbbccccddddeeeeffff0000aaaabbbb" + setup := func(t *testing.T, loggedIn bool) (*globals.Context, *replyChatHelper, *replyInboxSource) { + tc := libkb.SetupTest(t, "PostTextReply", 0) + t.Cleanup(tc.Cleanup) + helper := &replyChatHelper{} + inbox := &replyInboxSource{} + tc.G.ChatHelper = helper + if loggedIn { + uid := keybase1.MakeTestUID(1) + deviceID := keybase1.DeviceID("00000000000000000000000000000018") + sigKey, err := libkb.GenerateNaclSigningKeyPair() + require.NoError(t, err) + encKey, err := libkb.GenerateNaclDHKeyPair() + require.NoError(t, err) + require.NoError(t, tc.G.ActiveDevice.Set(libkb.NewMetaContextForTest(tc), + keybase1.UserVersion{Uid: uid, EldestSeqno: 1}, deviceID, + sigKey, encKey, "testuser-device", 0, libkb.KeychainModeNone)) + require.NoError(t, tc.G.Env.GetConfigWriter().SetUserConfig( + libkb.NewUserConfig(uid, "testuser", nil, deviceID), true)) + require.NoError(t, tc.G.Env.GetConfigWriter().SwitchUser("testuser")) + } + return globals.NewContext(tc.G, &globals.ChatContext{InboxSource: inbox}), helper, inbox + } + ctx := context.Background() + + t.Run("sends and marks read", func(t *testing.T) { + gc, helper, inbox := setup(t, true) + require.NoError(t, postTextReply(ctx, gc, convID, "testuser", 5, "hi")) + require.Equal(t, []string{"hi"}, helper.sent) + require.Equal(t, []chat1.MessageID{5}, inbox.marked) + }) + t.Run("send error is returned", func(t *testing.T) { + gc, helper, inbox := setup(t, true) + helper.sendErr = errors.New("outbox full") + require.EqualError(t, postTextReply(ctx, gc, convID, "testuser", 5, "hi"), "outbox full") + require.Empty(t, inbox.marked) + }) + t.Run("mark read failure doesn't fail a sent reply", func(t *testing.T) { + gc, helper, inbox := setup(t, true) + inbox.markErr = errors.New("offline") + require.NoError(t, postTextReply(ctx, gc, convID, "testuser", 5, "hi")) + require.Equal(t, []string{"hi"}, helper.sent) + }) + t.Run("logged out doesn't send", func(t *testing.T) { + gc, helper, _ := setup(t, false) + require.ErrorAs(t, postTextReply(ctx, gc, convID, "testuser", 5, "hi"), &libkb.LoginRequiredError{}) + require.Empty(t, helper.sent) + }) + t.Run("invalid message ID doesn't send", func(t *testing.T) { + gc, helper, _ := setup(t, true) + require.Error(t, postTextReply(ctx, gc, convID, "testuser", -1, "hi")) + require.Empty(t, helper.sent) + }) +} diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/ChatBroadcastReceiver.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/ChatBroadcastReceiver.kt index afb2f3a7dd03..7b3a85490e79 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/ChatBroadcastReceiver.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/ChatBroadcastReceiver.kt @@ -19,37 +19,53 @@ class ChatBroadcastReceiver : BroadcastReceiver() { } override fun onReceive(context: Context, intent: Intent) { - setupKBRuntime(context, false) val convData = ConvData.fromIntent(intent) val openConv = intent.getParcelableExtra("openConvPendingIntent") - val repliedNotification = NotificationCompat.Builder(context, KeybasePushNotificationListenerService.CHAT_CHANNEL_ID) - .setContentIntent(openConv) - .setTimeoutAfter(1000) - .setSmallIcon(R.drawable.ic_notif) - val notificationManager = NotificationManagerCompat.from(context) val messageBody = getMessageText(intent) - if (messageBody != null) { - try { - val withBackgroundActive: WithBackgroundActive = object : WithBackgroundActive { - override fun task() { - Keybase.handlePostTextReply(convData.convID, convData.tlfName, convData.lastMsgId, messageBody) - } + val pendingResult = goAsync() + runReceiverWork(RECEIVER_BUDGET_MS, { Thread(it).start() }, { NativeLogger.warn(it) }, { msg, e -> NativeLogger.error(msg, e) }, + { pendingResult.finish() }) { + val status = if (messageBody == null) { + NativeLogger.error("Message Body in quick reply was null") + "Couldn't send reply - Failed to read input." + } else { + setupKBRuntime(context, false) + sendQuickReply({ msg, e -> NativeLogger.error(msg, e) }) { + postTextReplyInPushWindow(context, convData, messageBody) } - withBackgroundActive.whileActive(context) - repliedNotification.setContentText("Replied") - } catch (e: Exception) { - repliedNotification.setContentText("Couldn't send reply") - NativeLogger.error("Failed to send quick reply", e) } - } else { - repliedNotification.setContentText("Couldn't send reply - Failed to read input.") - NativeLogger.error("Message Body in quick reply was null") + val repliedNotification = NotificationCompat.Builder(context, KeybasePushNotificationListenerService.CHAT_CHANNEL_ID) + .setContentIntent(openConv) + .setTimeoutAfter(1000) + .setSmallIcon(R.drawable.ic_notif) + .setContentText(status) + NotificationManagerCompat.from(context).notify(convData.convID, 0, repliedNotification.build()) + } + } + + // Transitional: the push window is opened here, for the same reason as + // WithBackgroundActive -- it goes away once the bind layer wraps the reply in the + // window itself. Unlike WithBackgroundActive this never skips the send while the app + // is foreground; a reply typed in the notification shade must go out either way. + private fun postTextReplyInPushWindow(context: Context, convData: ConvData, messageBody: String) { + // 0 when the app is active and nothing needs holding up. + val token = Keybase.appPushWindowBegin() + try { + Keybase.handlePostTextReply(convData.convID, convData.tlfName, convData.lastMsgId, messageBody) + } finally { + if (token > 0) { + // Hands over to a background task if the UI is still in the background and + // work must keep going. + Keybase.appPushWindowEnd(token, KBPushNotifier(context, Bundle())) + } } - notificationManager.notify(convData.convID, 0, repliedNotification.build()) } companion object { const val KEY_TEXT_REPLY = "key_text_reply" + + // goAsync gives a broadcast 10s; leave margin. + private const val RECEIVER_BUDGET_MS = 9_000L } } diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/MainApplication.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/MainApplication.kt index 6c0fd5f9ccfa..f5398a9449e8 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/MainApplication.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/MainApplication.kt @@ -6,9 +6,11 @@ import android.content.res.Configuration import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.ProcessLifecycleOwner +import androidx.work.ExistingPeriodicWorkPolicy +import androidx.work.Operation import androidx.work.PeriodicWorkRequest import androidx.work.WorkManager -import androidx.work.WorkRequest +import androidx.work.await import com.bumptech.glide.Glide import com.facebook.react.PackageList import com.facebook.react.ReactApplication @@ -22,9 +24,15 @@ import com.reactnativekb.IncomingShareCache import expo.modules.ApplicationLifecycleDispatcher.onApplicationCreate import expo.modules.ApplicationLifecycleDispatcher.onConfigurationChanged import expo.modules.ExpoReactHostFactory +import io.keybase.ossifrage.modules.BackgroundSyncJobs import io.keybase.ossifrage.modules.BackgroundSyncWorker +import io.keybase.ossifrage.modules.LegacyJobsCleanupFlag import io.keybase.ossifrage.modules.NativeLogger +import io.keybase.ossifrage.modules.scheduleBackgroundSync import keybase.Keybase +import kotlinx.coroutines.TimeoutCancellationException +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import java.util.concurrent.TimeUnit internal class AppLifecycleListener(private val context: Context?) : @@ -79,15 +87,13 @@ class MainApplication : Application(), ReactApplication { } }.start() - val backgroundSyncRequest: WorkRequest = PeriodicWorkRequest.Builder( - BackgroundSyncWorker::class.java, - 1, TimeUnit.HOURS, - 15, TimeUnit.MINUTES - ) - .build() - WorkManager - .getInstance(this) - .enqueue(backgroundSyncRequest) + Thread { + try { + scheduleBackgroundSync(WorkManagerBackgroundSyncJobs(this), SharedPrefsCleanupFlag(this)) + } catch (e: Exception) { + NativeLogger.warn("MainApplication: error scheduling background sync", e) + } + }.start() } fun onReactContextInitialized(context: ReactContext?) { @@ -106,3 +112,49 @@ class MainApplication : Application(), ReactApplication { super.onLowMemory() } } + +private class WorkManagerBackgroundSyncJobs(context: Context) : BackgroundSyncJobs { + private val workManager = WorkManager.getInstance(context) + + // WorkManager tags every request with its worker's class name. + override fun cancelAll() { + workManager.cancelAllWorkByTag(BackgroundSyncWorker::class.java.name).awaitDone("cancel") + } + + override fun enqueueUnique() { + val request = PeriodicWorkRequest.Builder( + BackgroundSyncWorker::class.java, + 1, TimeUnit.HOURS, + 15, TimeUnit.MINUTES + ).build() + workManager.enqueueUniquePeriodicWork("background_sync", ExistingPeriodicWorkPolicy.KEEP, request).awaitDone("enqueue") + } + + // A stalled WorkManager must not park the scheduling thread forever. + private fun Operation.awaitDone(what: String) { + try { + runBlocking { withTimeout(OPERATION_TIMEOUT_MS) { await() } } + } catch (e: TimeoutCancellationException) { + NativeLogger.warn("MainApplication: background sync $what timed out after ${OPERATION_TIMEOUT_MS}ms") + throw e + } + } + + companion object { + private const val OPERATION_TIMEOUT_MS = 30_000L + } +} + +private class SharedPrefsCleanupFlag(context: Context) : LegacyJobsCleanupFlag { + private val prefs = context.getSharedPreferences("background_sync", Context.MODE_PRIVATE) + + override fun isDone() = prefs.getBoolean(KEY, false) + + override fun markDone() { + prefs.edit().putBoolean(KEY, true).commit() + } + + companion object { + private const val KEY = "legacy_jobs_cancelled" + } +} diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/modules/BackgroundSyncSchedule.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/modules/BackgroundSyncSchedule.kt new file mode 100644 index 000000000000..930c305053f6 --- /dev/null +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/modules/BackgroundSyncSchedule.kt @@ -0,0 +1,29 @@ +package io.keybase.ossifrage.modules + +// WorkManager and the persisted flag, behind interfaces so the scheduling +// decision runs in JVM tests. Each call returns once its operation is done and +// throws if it failed. +internal interface BackgroundSyncJobs { + // Cancels every BackgroundSyncWorker job, including ones enqueued without + // a unique name by older versions. + fun cancelAll() + fun enqueueUnique() +} + +internal interface LegacyJobsCleanupFlag { + fun isDone(): Boolean + fun markDone() +} + +// Older versions enqueued a new periodic job on every process start, so +// existing installs can carry many. Clear them once, then keep one unique job +// whose period isn't reset on each launch. +internal fun scheduleBackgroundSync(jobs: BackgroundSyncJobs, cleanup: LegacyJobsCleanupFlag) { + if (cleanup.isDone()) { + jobs.enqueueUnique() + return + } + jobs.cancelAll() + jobs.enqueueUnique() + cleanup.markDone() +} diff --git a/shared/android/app/src/test/java/io/keybase/ossifrage/modules/BackgroundSyncScheduleTest.kt b/shared/android/app/src/test/java/io/keybase/ossifrage/modules/BackgroundSyncScheduleTest.kt new file mode 100644 index 000000000000..42e7f560956f --- /dev/null +++ b/shared/android/app/src/test/java/io/keybase/ossifrage/modules/BackgroundSyncScheduleTest.kt @@ -0,0 +1,64 @@ +package io.keybase.ossifrage.modules + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Test + +class BackgroundSyncScheduleTest { + private val calls = mutableListOf() + private var enqueueFails = false + private var done = false + + private val jobs = object : BackgroundSyncJobs { + override fun cancelAll() { + calls.add("cancelAll") + } + + override fun enqueueUnique() { + if (enqueueFails) throw IllegalStateException("enqueue failed") + calls.add("enqueueUnique") + } + } + + private val flag = object : LegacyJobsCleanupFlag { + override fun isDone() = done + + override fun markDone() { + done = true + } + } + + @Test + fun firstRunCancelsLegacyJobsThenEnqueues() { + scheduleBackgroundSync(jobs, flag) + assertEquals(listOf("cancelAll", "enqueueUnique"), calls) + assertTrue(done) + } + + @Test + fun laterRunsOnlyEnqueue() { + scheduleBackgroundSync(jobs, flag) + calls.clear() + scheduleBackgroundSync(jobs, flag) + scheduleBackgroundSync(jobs, flag) + assertEquals(listOf("enqueueUnique", "enqueueUnique"), calls) + } + + @Test + fun failedEnqueueRetriesTheCleanupNextRun() { + enqueueFails = true + try { + scheduleBackgroundSync(jobs, flag) + fail("the failure propagates") + } catch (e: IllegalStateException) { + } + assertFalse(done) + enqueueFails = false + calls.clear() + scheduleBackgroundSync(jobs, flag) + assertEquals(listOf("cancelAll", "enqueueUnique"), calls) + assertTrue(done) + } +} From 27b1c5ef4794d691aea34609fb1cb3ff9ea69727 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 13:00:16 -0400 Subject: [PATCH 3/7] refactor(android): flush a share intent when JS asks, not on a poll 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). --- .../java/io/keybase/ossifrage/MainActivity.kt | 94 ++++++------------- 1 file changed, 29 insertions(+), 65 deletions(-) diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt index 510722697005..932562e5e086 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt @@ -10,7 +10,6 @@ import android.os.Bundle import android.os.Handler import android.os.Looper import android.provider.MediaStore -import android.provider.Settings import android.util.Log import android.view.KeyEvent import androidx.core.content.IntentCompat @@ -19,10 +18,8 @@ import com.facebook.react.ReactActivity import com.facebook.react.ReactActivityDelegate import com.facebook.react.ReactApplication import com.facebook.react.bridge.Arguments -import com.facebook.react.bridge.ReactContext import com.facebook.react.defaults.DefaultNewArchitectureEntryPoint.fabricEnabled import com.facebook.react.defaults.DefaultReactActivityDelegate -import com.facebook.react.modules.core.PermissionListener import com.reactnativekb.DarkModePreference import com.reactnativekb.IncomingShareCache import com.reactnativekb.KbModule @@ -40,7 +37,6 @@ import java.security.cert.CertificateException import java.util.UUID class MainActivity : ReactActivity() { - private val listener: PermissionListener? = null private var isUsingHardwareKeyboard = false override fun invokeDefaultOnBackPressed() { @@ -75,8 +71,6 @@ class MainActivity : ReactActivity() { super.onCreate(null) KeybasePushNotificationListenerService.createNotificationChannel(this) updateIsUsingHardwareKeyboard() - - scheduleHandleIntent() } override fun onKeyUp(keyCode: Int, event: KeyEvent): Boolean { @@ -85,11 +79,6 @@ class MainActivity : ReactActivity() { } else super.onKeyUp(keyCode, event) } - override fun onRequestPermissionsResult(requestCode: Int, permissions: Array, grantResults: IntArray) { - listener?.onRequestPermissionsResult(requestCode, permissions, grantResults) - super.onRequestPermissionsResult(requestCode, permissions, grantResults) - } - override fun onPause() { NativeLogger.info("Activity onPause") super.onPause() @@ -111,10 +100,10 @@ class MainActivity : ReactActivity() { return filename } - private fun saveFileToCache(reactContext: ReactContext?, uri: Uri, filename: String): File { - val file = IncomingShareCache.file(reactContext!!, filename) + private fun saveFileToCache(context: Context, uri: Uri, filename: String): File { + val file = IncomingShareCache.file(context, filename) try { - reactContext.contentResolver.openInputStream(uri).use { istream -> + context.contentResolver.openInputStream(uri).use { istream -> FileOutputStream(file).use { ostream -> val buf = ByteArray(64 * 1024) var len: Int @@ -129,11 +118,11 @@ class MainActivity : ReactActivity() { return file } - private fun readFileFromUri(reactContext: ReactContext?, uri: Uri?): String? { + private fun readFileFromUri(context: Context, uri: Uri?): String? { if (uri == null) return null var filePath: String? filePath = if (uri.scheme == "content") { - val resolver = reactContext!!.contentResolver + val resolver = context.contentResolver val mimeType = resolver.getType(uri) val extension = MimeTypeMap.getSingleton().getExtensionFromMimeType(mimeType) @@ -141,7 +130,7 @@ class MainActivity : ReactActivity() { val filename = getFileNameFromResolver(resolver, uri, extension) // Now load the file itself. - val file = saveFileToCache(reactContext, uri, filename) + val file = saveFileToCache(context, uri, filename) file.path } else { uri.path @@ -166,26 +155,33 @@ class MainActivity : ReactActivity() { (application as MainApplication).lifecycleReporter.onMainActivityDestroy(isFinishing, isChangingConfigurations) } + // A share or notification intent parks here until JS asks for it. Nothing else is parked: + // deep links go through super.onNewIntent -> RCTLinkingManager, so a plain launch leaves + // this null. private var cachedIntent: Intent? = null private var pendingShareUris: List? = null private var pendingShareSubject: String? = null private var pendingShareText: String? = null - // Snapshot share/notification data out of the intent right away: share URI - // permission grants and clip data are tied to the delivered intent, and JS may - // not be ready to consume them until much later (see tryHandleIntentWithRetry). + // Snapshot share data out of the intent right away: share URI permission grants and clip + // data are tied to the delivered intent, and JS may not be ready to route them until much + // later (see shareListenersRegistered). private fun captureIntent(intent: Intent) { + val bundleFromNotification = intent.getBundleExtra("notification") + if (bundleFromNotification != null) { + KbModule.setInitialNotification(bundleFromNotification.clone() as Bundle) + } + val isShare = Intent.ACTION_SEND == intent.action || Intent.ACTION_SEND_MULTIPLE == intent.action + if (!isShare && bundleFromNotification == null) { + return + } cachedIntent = intent - if (Intent.ACTION_SEND == intent.action || Intent.ACTION_SEND_MULTIPLE == intent.action) { + if (isShare) { pendingShareUris = extractSharedUris(intent) pendingShareSubject = intent.getStringExtra(Intent.EXTRA_SUBJECT) pendingShareText = intent.getStringExtra(Intent.EXTRA_TEXT) } - val bundleFromNotification = intent.getBundleExtra("notification") - if (bundleFromNotification != null) { - KbModule.setInitialNotification(bundleFromNotification.clone() as Bundle) - } } override fun onNewIntent(intent: Intent) { @@ -197,9 +193,11 @@ class MainActivity : ReactActivity() { private var jsIsListening = false + // JS calls this once it is ready to route a share. That is the only signal the parked + // intent waits on, so it replaces any native-side polling for a live JS runtime. public fun shareListenersRegistered() { jsIsListening = true - tryHandleIntentWithRetry() + handleIntent() } private var handledIntentHash: String? = null @@ -235,37 +233,9 @@ class MainActivity : ReactActivity() { return uris.distinct() } - private var handleIntentRetryCount = 0 - private val maxHandleIntentRetries = 20 // 20 * 500ms = 10s max - - private fun scheduleHandleIntent() { - if (cachedIntent == null) return - handleIntentRetryCount = 0 - tryHandleIntentWithRetry() - } - - private fun tryHandleIntentWithRetry() { - if (cachedIntent == null) return - if (handleIntent()) return - handleIntentRetryCount++ - if (handleIntentRetryCount >= maxHandleIntentRetries) { - NativeLogger.info("MainActivity: giving up on handleIntent after $maxHandleIntentRetries retries") - return - } - NativeLogger.info("MainActivity: scheduling handleIntent retry #$handleIntentRetryCount") - Handler(Looper.getMainLooper()).postDelayed({ tryHandleIntentWithRetry() }, 500) - } - - private fun handleIntent(): Boolean { - val intent = cachedIntent ?: return true - val rc = reactActivityDelegate?.getCurrentReactContext() ?: run { - NativeLogger.info("MainActivity.handleIntent: no react context, will retry") - return false - } - if (!jsIsListening) { - NativeLogger.info("MainActivity.handleIntent: JS not listening yet, will retry") - return false - } + private fun handleIntent() { + val intent = cachedIntent ?: return + if (!jsIsListening) return NativeLogger.info("MainActivity.handleIntent: processing intent action=${intent.action}") // Here we are just reading from the notification bundle. @@ -315,10 +285,11 @@ class MainActivity : ReactActivity() { } else { // Copying out of the content providers can be slow for big files; don't // block the main thread on it. + val context: Context = this Thread { val filePaths = uris.mapNotNull { uri -> try { - readFileFromUri(rc, uri) + readFileFromUri(context, uri) } catch (e: SecurityException) { null } @@ -336,7 +307,6 @@ class MainActivity : ReactActivity() { } cachedIntent = null - return true } private fun emitShareText(text: String) { @@ -432,12 +402,6 @@ class MainActivity : ReactActivity() { } } - // Is this a robot controlled test device? (i.e. pre-launch report?) - fun isTestDevice(context: Context): Boolean { - val testLabSetting = Settings.System.getString(context.contentResolver, "firebase.test.lab") - return "true" == testLabSetting - } - @JvmStatic fun setupKBRuntime(context: Context, shouldCreateDummyFile: Boolean) { try { From 6f7e43743144c71d456f288885662408444444d2 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 13:28:25 -0400 Subject: [PATCH 4/7] fix(mobile): Go opens the push window and skips only the display while 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. --- go/bind/keybase.go | 41 ++++++- go/bind/notifications.go | 79 +++++++++---- go/bind/notifications_test.go | 111 ++++++++++++++++++ .../ossifrage/ChatBroadcastReceiver.kt | 21 +--- .../KeybasePushNotificationListenerService.kt | 44 ++++--- shared/ios/Keybase/AppDelegate.swift | 2 +- 6 files changed, 226 insertions(+), 72 deletions(-) diff --git a/go/bind/keybase.go b/go/bind/keybase.go index f8beb533fac4..ac421634d7b5 100644 --- a/go/bind/keybase.go +++ b/go/bind/keybase.go @@ -1005,14 +1005,33 @@ func AppUIBackground(pusher PushNotifier) int64 { return kbCtx.MobileLifecycle.UIBackground(backgroundTaskDeps(pusher)) } -// AppWaitBackgroundTask returns once the background task whose token -// AppUIBackground returned no longer needs any time in the background. -func AppWaitBackgroundTask(token int64) { +// inPushWindow runs work, which handles a push or a notification action. On +// Android it holds a backgrounded app up while work runs, and work learns +// whether the UI is active, in which case nothing is held. pusher warns about +// messages that won't send if the window hands over to a background task. +func inPushWindow(pusher PushNotifier, work func(uiActive bool) error) error { if !isInited() { - return + return work(false) } - defer kbCtx.Trace("AppWaitBackgroundTask", nil)() - kbCtx.MobileLifecycle.WaitBackgroundTask(token) + return runPushWindow(kbCtx.MobileLifecycle, runtime.GOOS, backgroundTaskDeps(pusher), work) +} + +func runPushWindow(lc *lifecycle.Controller, goos string, deps lifecycle.BackgroundTaskDeps, + work func(uiActive bool) error, +) error { + if goos != "android" { + // iOS handles a push within the time it grants for it and suspends the + // app at the completion handler, so nothing needs holding up; a window + // would only take the app through BACKGROUNDACTIVE and back. work only + // uses uiActive on Android. + return work(false) + } + token := lc.PushWindowBegin() + if token == 0 { + return work(true) + } + defer lc.PushWindowEnd(token, deps) + return work(false) } // AppPushWindowBegin holds a backgrounded app up while native handles a push or @@ -1043,6 +1062,16 @@ func AppPushWindowEnd(token int64, pusher PushNotifier) { kbCtx.MobileLifecycle.PushWindowEnd(token, backgroundTaskDeps(pusher)) } +// AppWaitBackgroundTask returns once the background task whose token +// AppUIBackground returned no longer needs any time in the background. +func AppWaitBackgroundTask(token int64) { + if !isInited() { + return + } + defer kbCtx.Trace("AppWaitBackgroundTask", nil)() + kbCtx.MobileLifecycle.WaitBackgroundTask(token) +} + func backgroundTaskDeps(pusher PushNotifier) lifecycle.BackgroundTaskDeps { return lifecycle.BackgroundTaskDeps{ Stay: shouldStayRunningInBackground, diff --git a/go/bind/notifications.go b/go/bind/notifications.go index 3298c263805a..93eef9f7e9ec 100644 --- a/go/bind/notifications.go +++ b/go/bind/notifications.go @@ -114,11 +114,15 @@ type ChatNotification struct { Uid string } -func HandlePostTextReply(strConvID, tlfName string, intMessageID int, body string) (err error) { +// HandlePostTextReply sends a notification quick reply, in the foreground too. +// pusher warns about the reply if it won't send. +func HandlePostTextReply(strConvID, tlfName string, intMessageID int, body string, pusher PushNotifier) (err error) { ctx := context.Background() defer kbCtx.CTrace(ctx, "HandlePostTextReply", &err)() defer func() { err = flattenError(err) }() - return postTextReply(ctx, globals.NewContext(kbCtx, kbChatCtx), strConvID, tlfName, intMessageID, body) + return inPushWindow(pusher, func(bool) error { + return postTextReply(ctx, globals.NewContext(kbCtx, kbChatCtx), strConvID, tlfName, intMessageID, body) + }) } // postTextReply sends a notification quick reply and marks the conversation @@ -157,10 +161,16 @@ func postTextReply(ctx context.Context, gc *globals.Context, strConvID, tlfName var spoileRegexp = regexp.MustCompile(`!>(.*?) 0 || len(chatNotification.Message.ServerMessage) > 0) { - // Lock and check if we've already processed this notification. - seenNotificationsMtx.Lock() - defer seenNotificationsMtx.Unlock() - if _, ok := getSeenNotificationsCache().Get(dupKey); ok { - // Cancel any duplicate visible notifications + ackPush := func() { if ack != nil { ack.Ack(ctx, []string{pushID}) } - kbCtx.Log.CDebugf(ctx, "HandleBackgroundNotification: duplicate notification convID=%s msgID=%d", strConvID, intMessageID) - // Return nil (not an error) so Android does not treat this as failure and show a fallback notification. - return nil } - // Add to cache before displaying so that any concurrent goroutine that - // reaches the second check while DisplayChatNotification is running will - // see the entry and bail out rather than displaying a duplicate. - getSeenNotificationsCache().Add(dupKey, struct{}{}) - pusher.DisplayChatNotification(&chatNotification) - if ack != nil { - ack.Ack(ctx, []string{pushID}) + if displayOnce(dupKey, &chatNotification, pusher, runtime.GOOS, uiActive, ackPush) { + kbCtx.Log.CDebugf(ctx, "HandleBackgroundNotification: duplicate notification convID=%s msgID=%d", strConvID, intMessageID) } } return nil } + +// displayOnce displays n unless its push was already handled, then acks the +// push. On Android, while the UI is active it only acks: the app already shows +// the message. iOS always displays, because its display also removes the +// server's generic notification for this message, which can land while the +// push is being handled; a local notification never shows while active. +func displayOnce(dupKey string, n *ChatNotification, pusher PushNotifier, goos string, uiActive bool, + ack func(), +) (dup bool) { + seenNotificationsMtx.Lock() + defer seenNotificationsMtx.Unlock() + if _, ok := getSeenNotificationsCache().Get(dupKey); ok { + // Cancel any duplicate visible notifications + ack() + return true + } + // Add to cache before displaying so that any concurrent goroutine that + // reaches the check while DisplayChatNotification is running sees the + // entry and bails out rather than displaying a duplicate. + getSeenNotificationsCache().Add(dupKey, struct{}{}) + if !uiActive || goos != "android" { + pusher.DisplayChatNotification(n) + } + ack() + return false +} diff --git a/go/bind/notifications_test.go b/go/bind/notifications_test.go index 83b5a07679e9..dea2bf766df5 100644 --- a/go/bind/notifications_test.go +++ b/go/bind/notifications_test.go @@ -3,11 +3,15 @@ package keybase import ( "context" "errors" + "fmt" "testing" + "time" "github.com/keybase/client/go/chat/globals" "github.com/keybase/client/go/chat/types" "github.com/keybase/client/go/libkb" + "github.com/keybase/client/go/libkb/lifecycle" + "github.com/keybase/client/go/libkb/lifecycle/lifecycletest" "github.com/keybase/client/go/protocol/chat1" "github.com/keybase/client/go/protocol/gregor1" "github.com/keybase/client/go/protocol/keybase1" @@ -98,3 +102,110 @@ func TestPostTextReply(t *testing.T) { require.Empty(t, helper.sent) }) } + +// pendingDeliveryDeps reports a message still sending, so a push window that +// may hand over to a background task does. +func pendingDeliveryDeps() lifecycle.BackgroundTaskDeps { + return lifecycle.BackgroundTaskDeps{ + Stay: func() bool { return true }, + ActiveDeliveries: func(context.Context) ([]chat1.OutboxRecord, error) { + return make([]chat1.OutboxRecord, 1), nil + }, + NextFailure: func() (chan []chat1.OutboxRecord, func()) { return make(chan []chat1.OutboxRecord), func() {} }, + NotifyFailure: func([]chat1.OutboxRecord) {}, + } +} + +func TestBackgroundNotificationOpensAndClosesPushWindow(t *testing.T) { + const ( + fg = keybase1.MobileAppState_FOREGROUND + bg = keybase1.MobileAppState_BACKGROUND + bga = keybase1.MobileAppState_BACKGROUNDACTIVE + ) + for _, platform := range []lifecycletest.Platform{lifecycletest.IOS, lifecycletest.Android} { + t.Run(platform.String(), func(t *testing.T) { + tc := libkb.SetupTest(t, "PushWindow", 0) + defer tc.Cleanup() + h := lifecycletest.NewHarness(t, libkb.NewMobileAppState(tc.G), platform) + defer h.Close() + h.Controller.UIInactive() + lifecycletest.ToBackground(h.Controller) + require.Equal(t, bg, h.AppState.State()) + seen := len(h.Recorder.States()) + + unboxFailed := errors.New("unbox failed") + var during keybase1.MobileAppState + err := runPushWindow(h.Controller, platform.String(), pendingDeliveryDeps(), func(uiActive bool) error { + require.False(t, uiActive) + during = h.AppState.State() + return unboxFailed + }) + require.ErrorIs(t, err, unboxFailed) + if platform == lifecycletest.IOS { + require.Equal(t, bg, during, "iOS handles the push without holding the app up") + require.Equal(t, bg, h.AppState.State()) + h.Recorder.Sync(t) + require.Len(t, h.Recorder.States(), seen, "the push never reached the controller") + } else { + require.Equal(t, bga, during, "the push is handled in BACKGROUNDACTIVE") + require.Equal(t, bga, h.AppState.State(), "a background task keeps sending") + h.Controller.BackgroundTaskExpired(func() {}) + require.Equal(t, bg, h.AppState.State(), "the background task held the app, not the push window") + } + + h.Controller.UIActive() + ran := false + require.NoError(t, runPushWindow(h.Controller, platform.String(), pendingDeliveryDeps(), func(uiActive bool) error { + require.Equal(t, platform == lifecycletest.Android, uiActive, "only Android's work asks") + ran = true + return nil + })) + require.True(t, ran, "the work runs while the UI is active") + require.Equal(t, fg, h.AppState.State()) + }) + } +} + +type recordingPusher struct { + PushNotifier + displayed []string +} + +func (p *recordingPusher) DisplayChatNotification(n *ChatNotification) { + p.displayed = append(p.displayed, n.ConvID) +} + +func TestBackgroundNotificationActiveSkipsDisplayButAcks(t *testing.T) { + for _, goos := range []string{"android", "ios"} { + t.Run(goos, func(t *testing.T) { + pusher := &recordingPusher{} + acks := 0 + ack := func() { acks++ } + show := func(convID string, uiActive bool) bool { + return displayOnce(convID+"||1", &ChatNotification{ConvID: convID}, pusher, goos, uiActive, ack) + } + // The seen cache is global, so each run needs its own push ids. + run := fmt.Sprintf("%s/%d/", t.Name(), time.Now().UnixNano()) + active := run + "active" + require.False(t, show(active, true)) + if goos == "android" { + require.Empty(t, pusher.displayed, "the app already shows the message") + } else { + require.Equal(t, []string{active}, pusher.displayed, + "iOS displays to remove the server's generic notification; the local one never shows while active") + } + require.Equal(t, 1, acks, "the push is acked so the server's fallback doesn't show it") + displayed := len(pusher.displayed) + + require.True(t, show(active, false), "a push handled while active isn't shown later") + require.Len(t, pusher.displayed, displayed) + require.Equal(t, 2, acks) + + background := run + "background" + require.False(t, show(background, false)) + require.Equal(t, background, pusher.displayed[len(pusher.displayed)-1]) + require.Len(t, pusher.displayed, displayed+1) + require.Equal(t, 3, acks) + }) + } +} diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/ChatBroadcastReceiver.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/ChatBroadcastReceiver.kt index 7b3a85490e79..8a2bd9bdc198 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/ChatBroadcastReceiver.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/ChatBroadcastReceiver.kt @@ -31,7 +31,8 @@ class ChatBroadcastReceiver : BroadcastReceiver() { } else { setupKBRuntime(context, false) sendQuickReply({ msg, e -> NativeLogger.error(msg, e) }) { - postTextReplyInPushWindow(context, convData, messageBody) + Keybase.handlePostTextReply(convData.convID, convData.tlfName, convData.lastMsgId, messageBody, + KBPushNotifier(context, Bundle())) } } val repliedNotification = NotificationCompat.Builder(context, KeybasePushNotificationListenerService.CHAT_CHANNEL_ID) @@ -43,24 +44,6 @@ class ChatBroadcastReceiver : BroadcastReceiver() { } } - // Transitional: the push window is opened here, for the same reason as - // WithBackgroundActive -- it goes away once the bind layer wraps the reply in the - // window itself. Unlike WithBackgroundActive this never skips the send while the app - // is foreground; a reply typed in the notification shade must go out either way. - private fun postTextReplyInPushWindow(context: Context, convData: ConvData, messageBody: String) { - // 0 when the app is active and nothing needs holding up. - val token = Keybase.appPushWindowBegin() - try { - Keybase.handlePostTextReply(convData.convID, convData.tlfName, convData.lastMsgId, messageBody) - } finally { - if (token > 0) { - // Hands over to a background task if the UI is still in the background and - // work must keep going. - Keybase.appPushWindowEnd(token, KBPushNotifier(context, Bundle())) - } - } - } - companion object { const val KEY_TEXT_REPLY = "key_text_reply" diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/KeybasePushNotificationListenerService.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/KeybasePushNotificationListenerService.kt index 6bdce186b21d..a50758c0f382 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/KeybasePushNotificationListenerService.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/KeybasePushNotificationListenerService.kt @@ -23,8 +23,12 @@ class KeybasePushNotificationListenerService : FirebaseMessagingService() { // was notified about to give context to future notifications. private val msgCache = HashMap() - // Avoid ever showing doubles - private val seenChatNotifications = HashSet() + // Go's seen cache dedupes what Go displays, but not the fallback below: a + // redelivered push that Go fails on again would show the fallback twice, + // and each display adds the message to msgCache's history again. + private val seenChatNotifications = object : LinkedHashMap(16, 0.75f, false) { + override fun removeEldestEntry(eldest: MutableMap.MutableEntry?) = size > SEEN_CHAT_NOTIFICATIONS_MAX + } private fun chatNotificationKey(convID: String?, messageId: Int, targetUID: String): String { return "$targetUID|$convID|$messageId" } @@ -112,12 +116,12 @@ class KeybasePushNotificationListenerService : FirebaseMessagingService() { // Key includes UID so two signed-in accounts in the same chat are not treated as duplicates. if (!dontNotify) { val notificationKey = chatNotificationKey(n.convID, n.messageId, targetUID) - if (seenChatNotifications.contains(notificationKey)) { + if (seenChatNotifications.containsKey(notificationKey)) { NativeLogger.info("KeybasePushNotificationListenerService skipping duplicate notification: $notificationKey") return } // Mark as seen immediately to prevent duplicate processing - seenChatNotifications.add(notificationKey) + seenChatNotifications[notificationKey] = Unit NativeLogger.info("KeybasePushNotificationListenerService marked notification as seen: $notificationKey") } @@ -129,27 +133,18 @@ class KeybasePushNotificationListenerService : FirebaseMessagingService() { } notifier.setMsgCache(msgCache[n.convID]) try { - val withBackgroundActive: WithBackgroundActive = object : WithBackgroundActive { - override fun task() { - try { - Keybase.handleBackgroundNotification(n.convID, payload, n.serverMessageBody, n.sender, - n.membersType.toLong(), n.displayPlaintext, n.messageId.toLong(), n.pushId, - n.badgeCount.toLong(), n.unixTime, n.soundName, if (dontNotify) null else notifier, true, - targetUID) - goProcessingSucceeded = true - if (!dontNotify) { - seenChatNotifications.add(chatNotificationKey(n.convID, n.messageId, targetUID)) - } - } catch (ex: Exception) { - NativeLogger.error("Go Couldn't handle background notification2: " + ex.message) - throw ex - } - } + // Go holds the app up while it handles the push, and in the + // foreground acks it without displaying it. + Keybase.handleBackgroundNotification(n.convID, payload, n.serverMessageBody, n.sender, + n.membersType.toLong(), n.displayPlaintext, n.messageId.toLong(), n.pushId, + n.badgeCount.toLong(), n.unixTime, n.soundName, if (dontNotify) null else notifier, true, + targetUID, KBPushNotifier(applicationContext, Bundle())) + goProcessingSucceeded = true + if (!dontNotify) { + seenChatNotifications[chatNotificationKey(n.convID, n.messageId, targetUID)] = Unit } - withBackgroundActive.whileActive(applicationContext) } catch (ex: Exception) { - NativeLogger.error("Failed to process notification (app may not be running): " + ex.message) - goProcessingSucceeded = false + NativeLogger.error("Go couldn't handle background notification: " + ex.message) } } @@ -203,7 +198,7 @@ class KeybasePushNotificationListenerService : FirebaseMessagingService() { chatNotif.uid = targetUID notifier.displayChatNotification(chatNotif) - seenChatNotifications.add(chatNotificationKey(n.convID, n.messageId, targetUID)) + seenChatNotifications[chatNotificationKey(n.convID, n.messageId, targetUID)] = Unit NativeLogger.info("KeybasePushNotificationListenerService fallback notification displayed successfully") } catch (e: Exception) { NativeLogger.error("Failed to display notification fallback: " + e.message) @@ -281,6 +276,7 @@ class KeybasePushNotificationListenerService : FirebaseMessagingService() { } companion object { + private const val SEEN_CHAT_NOTIFICATIONS_MAX = 100 const val CHAT_CHANNEL_ID = "kb_chat_channel" const val FOLLOW_CHANNEL_ID = "kb_follow_channel" const val DEVICE_CHANNEL_ID = "kb_device_channel" diff --git a/shared/ios/Keybase/AppDelegate.swift b/shared/ios/Keybase/AppDelegate.swift index 2dd6f45c3b72..42cac6f2eeae 100644 --- a/shared/ios/Keybase/AppDelegate.swift +++ b/shared/ios/Keybase/AppDelegate.swift @@ -337,7 +337,7 @@ class AppDelegate: ExpoAppDelegate, ExpoReactNativeFactoryProvider, UNUserNotifi var err: NSError? Keybasego.KeybaseHandleBackgroundNotification( convID, body, "", sender, membersType, displayPlaintext, messageID, pushID, badgeCount, - unixTime, soundName, pusher, false, targetUID, &err) + unixTime, soundName, pusher, false, targetUID, pusher, &err) if let err { log.error("Failed to handle in engine: \(err.localizedDescription, privacy: .public)") } completionHandler(.newData) log.info("Remote notification handle finished...") From 8834186d6b836e0160fa4ffde4a6a142173a964f Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 13:28:35 -0400 Subject: [PATCH 5/7] refactor(mobile): drop the transitional Kotlin push window 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. --- go/bind/keybase.go | 28 ------------- .../KeybasePushNotificationListenerService.kt | 39 ------------------- 2 files changed, 67 deletions(-) diff --git a/go/bind/keybase.go b/go/bind/keybase.go index ac421634d7b5..3c8aece0fa7c 100644 --- a/go/bind/keybase.go +++ b/go/bind/keybase.go @@ -1034,34 +1034,6 @@ func runPushWindow(lc *lifecycle.Controller, goos string, deps lifecycle.Backgro return work(false) } -// AppPushWindowBegin holds a backgrounded app up while native handles a push or -// a notification action. It returns the window's token, or 0 when the app is -// active and nothing needs holding. -// -// Transitional: it exists only because Android still opens the push window from -// Kotlin. It goes away once the bind layer wraps push handling in the window -// itself, which is where the decision belongs -- iOS needs no window at all, -// since it suspends the app at the push's completion handler. -func AppPushWindowBegin() int64 { - if !isInited() { - return 0 - } - defer kbCtx.Trace("AppPushWindowBegin", nil)() - return kbCtx.MobileLifecycle.PushWindowBegin() -} - -// AppPushWindowEnd ends the window AppPushWindowBegin opened. If the UI is -// still in the background it first hands over to a background task, which keeps -// the app up while work must keep going; pusher warns about messages that -// won't send. Transitional, for the same reason as AppPushWindowBegin. -func AppPushWindowEnd(token int64, pusher PushNotifier) { - if !isInited() { - return - } - defer kbCtx.Trace("AppPushWindowEnd", nil)() - kbCtx.MobileLifecycle.PushWindowEnd(token, backgroundTaskDeps(pusher)) -} - // AppWaitBackgroundTask returns once the background task whose token // AppUIBackground returned no longer needs any time in the background. func AppWaitBackgroundTask(token int64) { diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/KeybasePushNotificationListenerService.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/KeybasePushNotificationListenerService.kt index a50758c0f382..15d05055970a 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/KeybasePushNotificationListenerService.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/KeybasePushNotificationListenerService.kt @@ -391,42 +391,3 @@ internal class NotificationData(type: String, bundle: Bundle) { } } -// Runs some task inside a push window, which keeps a backgrounded app running -// while the task does. If already foreground, ignore. -// -// Transitional: it exists only because Android still opens the push window from -// here. It goes away, with Keybase.appPushWindow*, once the bind layer wraps -// push handling in the window itself. -internal interface WithBackgroundActive { - @Throws(Exception::class) - fun task() - - @Throws(Exception::class) - fun whileActive(context: Context) { - try { - // We are foreground don't show anything - val isForeground = Keybase.isAppStateForeground() - NativeLogger.info("WithBackgroundActive.whileActive isForeground: $isForeground") - if (isForeground) { - NativeLogger.info("WithBackgroundActive.whileActive app is foreground, returning early") - return - } - // 0 when the app is active and nothing needs holding up. - val token = Keybase.appPushWindowBegin() - NativeLogger.info("WithBackgroundActive.whileActive push window $token, calling task") - try { - task() - NativeLogger.info("WithBackgroundActive.whileActive task completed") - } finally { - if (token > 0) { - // Hands over to a background task if the UI is still in the - // background and work must keep going. - Keybase.appPushWindowEnd(token, KBPushNotifier(context, Bundle())) - } - } - } catch (ex: Exception) { - NativeLogger.error("WithBackgroundActive.whileActive exception: " + ex.message) - throw ex - } - } -} From 0de89d515eed6d50c4b88a48620a07a043d09bd0 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 13:40:47 -0400 Subject: [PATCH 6/7] test(bind): settle the recorder before the push window's baseline 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. --- go/bind/notifications_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/go/bind/notifications_test.go b/go/bind/notifications_test.go index dea2bf766df5..470fde9223d4 100644 --- a/go/bind/notifications_test.go +++ b/go/bind/notifications_test.go @@ -131,6 +131,10 @@ func TestBackgroundNotificationOpensAndClosesPushWindow(t *testing.T) { h.Controller.UIInactive() lifecycletest.ToBackground(h.Controller) require.Equal(t, bg, h.AppState.State()) + // The recorder observes the setup transitions from its own + // goroutine; without this the baseline can be short by one and the + // iOS assertion below reads a late setup state as a push. + h.Recorder.Sync(t) seen := len(h.Recorder.States()) unboxFailed := errors.New("unbox failed") From 1a50b6f42dad6dc875ba9f0dc3fcd3c9a56a0a1f Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Tue, 22 Sep 2026 16:35:54 -0400 Subject: [PATCH 7/7] chore(android): drop a comment describing removed polling --- .../app/src/main/java/io/keybase/ossifrage/MainActivity.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt b/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt index 932562e5e086..f1aaabaf42b4 100644 --- a/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt +++ b/shared/android/app/src/main/java/io/keybase/ossifrage/MainActivity.kt @@ -194,7 +194,7 @@ class MainActivity : ReactActivity() { private var jsIsListening = false // JS calls this once it is ready to route a share. That is the only signal the parked - // intent waits on, so it replaces any native-side polling for a live JS runtime. + // intent waits on. public fun shareListenersRegistered() { jsIsListening = true handleIntent()