fix: use goAsync in runGloballyWithTimeout

This is required to guarantee the OS does not kill the process after `onReceive` completes. See https://developer.android.com/reference/android/content/BroadcastReceiver#goAsync()

A slight reorganization of NotificationService is then needed, as `goAsync` does not work properly in tests. Also, the primary logic in `onReceive` is starting to get bloated, so it's better to move it to a separate, more testable function.

Author: ericli3690

AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt

@@ -18,6 +18,7 @@ package com.ichi2.anki
 
 import android.app.Activity
 import android.app.Dialog
+import android.content.BroadcastReceiver
 import android.content.Context
 import android.content.DialogInterface
 import android.database.sqlite.SQLiteDatabaseCorruptException
@@ -80,6 +81,7 @@ import timber.log.Timber
 import kotlin.coroutines.CoroutineContext
 import kotlin.coroutines.EmptyCoroutineContext
 import kotlin.time.Duration
+import kotlin.time.Duration.Companion.seconds
 
 /** Overridable reference to [Dispatchers.IO]. Useful if tests can't use it */
 // COULD_BE_BETTER: this shouldn't be necessary, but TestClass::runWith needs it
@@ -612,21 +614,44 @@ private fun Activity.showError(
 ) = showError(throwable.toString(), throwable.toCrashReportData(context = this, reportException))
 
 /**
- * Launches a coroutine which is guaranteed to terminate within the [timeout] duration, which means
- * it is safe to call on the global coroutine scope. We handle the global scope carefully here to ensure
- * that the coroutine eventually terminates and does not cause a memory leak.
+ * Since BroadcastReceiver onReceive methods are expected to finish quickly, this helper function
+ * is required to run a suspending function from an onReceive method. [BroadcastReceiver.goAsync]
+ * extends the lifetime of the onReceive method and tells the OS not to kill the process prematurely.
+ *
+ * Do not call [BroadcastReceiver.goAsync] directly before calling this function.
+ *
+ * @param timeout Just in case the block hangs. Cannot exceed 8 seconds, because an ANR may occur if a
+ * BroadcastReceiver's onReceive method runs for longer than 10 seconds.
+ * See [the docs](https://developer.android.com/reference/android/content/BroadcastReceiver#goAsync()).
+ * @param block The suspending function to run.
  */
-fun runGloballyWithTimeout(
+fun BroadcastReceiver.runGloballyWithTimeout(
     timeout: Duration,
     block: suspend () -> Unit,
 ) {
+    val pendingResult = goAsync()
+    if (pendingResult == null) {
+        // pendingResult should never be null, so this should never happen.
+        // According to the implementation of goAsync, if it is, that indicates goAsync was called twice for the same onReceive.
+        Timber.w("goAsync returned null, cannot run block")
+        CrashReportService.sendExceptionReport(
+            message =
+                "goAsync returned null for BroadcastReceiver: " +
+                    "This should never happen and indicates goAsync was called twice for the same onReceive",
+            origin = "CoroutineHelpers:BroadcastReceiver.runGloballyWithTimeout",
+        )
+        return
+    }
+
     AnkiDroidApp.applicationScope.launch {
         try {
-            withTimeout(timeout) {
+            withTimeout(minOf(timeout, 8.seconds)) {
                 block()
             }
         } catch (e: TimeoutCancellationException) {
             Timber.w(e, "runGloballyWithTimeout timed out after $timeout")
+        } finally {
+            pendingResult.finish()
         }
     }
 }

AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt

@@ -47,6 +47,7 @@ import com.ichi2.anki.utils.remainingTime
 import com.ichi2.widget.WidgetStatus
 import net.ankiweb.rsdroid.BackendException
 import timber.log.Timber
+import kotlin.coroutines.cancellation.CancellationException
 import kotlin.time.Duration
 import kotlin.time.Duration.Companion.days
 import kotlin.time.Duration.Companion.hours
@@ -79,8 +80,49 @@ class NotificationService : BroadcastReceiver() {
 
         /**
          * Timeout for the process of sending a review reminder notification.
+         * Should be below 10 seconds as BroadcastReceivers may ANR when onReceive takes longer than 10 seconds.
+         * See [the docs](https://developer.android.com/reference/android/content/BroadcastReceiver#goAsync()).
          */
-        private val SEND_REVIEW_REMINDER_TIMEOUT = 10.seconds
+        private val SEND_REVIEW_REMINDER_TIMEOUT = 8.seconds
+
+        /**
+         * Triggered by [onReceive]. Does some bookkeeping if the triggered notification is of the recurring type
+         * or does nothing if it is a snoozed one, and then begins the process of sending the notification.
+         */
+        @VisibleForTesting
+        suspend fun handleReviewReminderNotification(
+            context: Context,
+            reviewReminder: ReviewReminder,
+            isRecurringNotification: Boolean,
+        ) {
+            if (isRecurringNotification) {
+                // Record this latest routine notification-firing attempt's timestamp
+                reviewReminder.updateLatestNotifTime()
+                when (val scope = reviewReminder.scope) {
+                    is ReviewReminderScope.DeckSpecific ->
+                        ReviewRemindersDatabase.editRemindersForDeck(
+                            scope.did,
+                            upsertReminder(reviewReminder),
+                        )
+                    is ReviewReminderScope.Global -> ReviewRemindersDatabase.editAllAppWideReminders(upsertReminder(reviewReminder))
+                }
+
+                // Schedule the next routine notification-firing
+                Timber.d("Scheduling next review reminder notification")
+                AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminder)
+            }
+
+            // Send the notification
+            try {
+                sendReviewReminderNotification(context, reviewReminder)
+            } catch (e: BackendException) {
+                // This may occur if the collection is blocked, in which case we should fail gracefully
+                Timber.w(e, "Aborted review reminder notification due to backend exception")
+            } catch (e: CancellationException) {
+                Timber.w(e, "Aborted review reminder notification due to cancellation exception")
+                throw e // Rethrow to propagate to parent coroutine
+            }
+        }
 
         /**
          * Sends a notification for a review reminder.
@@ -421,35 +463,16 @@ class NotificationService : BroadcastReceiver() {
                 ) ?: return
             Timber.d("onReceive: ${reviewReminder.id}")
 
-            // If this is a recurring notification...
-            if (action == NotificationServiceAction.ScheduleRecurringNotifications.actionString) {
-                // Record this latest routine notification-firing attempt's timestamp
-                reviewReminder.updateLatestNotifTime()
-                when (val scope = reviewReminder.scope) {
-                    is ReviewReminderScope.DeckSpecific ->
-                        ReviewRemindersDatabase.editRemindersForDeck(
-                            scope.did,
-                            upsertReminder(reviewReminder),
-                        )
-                    is ReviewReminderScope.Global -> ReviewRemindersDatabase.editAllAppWideReminders(upsertReminder(reviewReminder))
-                }
-
-                // Schedule the next routine notification-firing
-                Timber.d("Scheduling next review reminder notification")
-                AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminder)
-            }
-
+            // We must run some suspending functions. Hence we mark this onReceive function as long-running
+            // and use the global scope for simplicity's sake. Theoretically, we could also use an expedited Worker,
+            // but AnkiDroid is only allotted a fixed number of expedited Worker calls per day
+            // and those expedited calls are also used by the sync service, so it's best to conserve them.
             runGloballyWithTimeout(SEND_REVIEW_REMINDER_TIMEOUT) {
-                // We run this on the global scope for simplicity's sake, as BroadcastReceivers do not have CoroutineScopes.
-                // Theoretically we could also use an expedited Worker, but AnkiDroid is only allotted a fixed number
-                // of expedited Worker calls per day, and these expedited calls are also used by the sync service,
-                // so it's best to conserve them.
-                try {
-                    sendReviewReminderNotification(context, reviewReminder)
-                } catch (e: BackendException) {
-                    // This may occur if the collection is blocked, in which case we should fail gracefully
-                    Timber.w(e, "Aborted review reminder notification due to backend exception")
-                }
+                handleReviewReminderNotification(
+                    context,
+                    reviewReminder,
+                    isRecurringNotification = (action == NotificationServiceAction.ScheduleRecurringNotifications.actionString),
+                )
             }
         } else {
             triggerNotificationFor(context)

AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt

@@ -85,7 +85,7 @@ class NotificationServiceTest : RobolectricTest() {
     }
 
     @Test
-    fun `onReceive with less cards than card threshold should not fire notification but schedule next`() =
+    fun `triggering with less cards than card threshold should not fire notification but schedule next`() =
         runTest {
             val did1 = addDeck("Deck", setAsSelected = true).withNotes(count = 2)
             val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 3)
@@ -105,7 +105,7 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with happy path for single deck should fire notification and schedule next`() =
+    fun `triggering with happy path for single deck should fire notification and schedule next`() =
         runTest {
             val did1 = addDeck("Deck", setAsSelected = true).withNotes(count = 2)
             val reviewReminder = createAndSaveDummyDeckSpecificReminder(did1)
@@ -119,7 +119,7 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with happy path for global reminder should fire notification and schedule next`() =
+    fun `triggering with happy path for global reminder should fire notification and schedule next`() =
         runTest {
             addDeck("Deck1").withNotes(count = 2)
             addDeck("Deck2").withNotes(count = 2)
@@ -134,9 +134,10 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with non-existent deck should not fire notification but schedule next`() =
+    fun `triggering with non-existent deck should not fire notification but schedule next`() =
         runTest {
-            val reviewReminder = createTestReminder(deckId = 9999)
+            val did1 = addDeck("Deck")
+            val reviewReminder = createTestReminder(deckId = did1 + 1)
             val creationTime = reviewReminder.latestNotifTime
 
             triggerDummyReminderNotification(reviewReminder)
@@ -147,7 +148,7 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with reviews today and onlyNotifyIfNoReviews is true should not fire notification`() =
+    fun `triggering with reviews today and onlyNotifyIfNoReviews is true should not fire notification`() =
         runTest {
             val did1 = addDeck("Deck", setAsSelected = true).withNote()
             col.sched.answerCard(col.sched.card!!, CardAnswer.Rating.GOOD)
@@ -162,7 +163,7 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with no reviews ever and onlyNotifyIfNoReviews is true should fire notification`() =
+    fun `triggering with no reviews ever and onlyNotifyIfNoReviews is true should fire notification`() =
         runTest {
             val did1 = addDeck("Deck", setAsSelected = true).withNote()
             val reviewReminderDeckSpecific = createTestReminder(deckId = did1, thresholdInt = 1, onlyNotifyIfNoReviews = true)
@@ -177,7 +178,7 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with review yesterday but none today and onlyNotifyIfNoReviews is true should fire notification`() =
+    fun `triggering with review yesterday but none today and onlyNotifyIfNoReviews is true should fire notification`() =
         runTest {
             TimeManager.resetWith(yesterday) // Wind back time and perform the review
             val did1 = addDeck("Deck", setAsSelected = true).withNote()
@@ -201,7 +202,7 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with onlyNotifyIfNoReviews is false should always fire notification`() =
+    fun `triggering with onlyNotifyIfNoReviews is false should always fire notification`() =
         runTest {
             val did1 = addDeck("Deck", setAsSelected = true).withNote()
             val reviewReminderDeckSpecific = createAndSaveDummyDeckSpecificReminder(did1)
@@ -218,7 +219,7 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with blocked collection should not fire notification but schedule next`() =
+    fun `triggering with blocked collection should not fire notification but schedule next`() =
         runTest {
             val did1 = addDeck("Deck", setAsSelected = true).withNotes(count = 2)
             val reviewReminderDeckSpecific = createAndSaveDummyDeckSpecificReminder(did1)
@@ -238,18 +239,16 @@ class NotificationServiceTest : RobolectricTest() {
         }
 
     @Test
-    fun `onReceive with snoozed notification should fire notification but not schedule next`() =
+    fun `triggering with snoozed notification should fire notification but not schedule next`() =
         runTest {
             val did1 = addDeck("Deck", setAsSelected = true).withNotes(count = 2)
             val reviewReminderDeckSpecific = createAndSaveDummyDeckSpecificReminder(did1)
             val reviewReminderAppWide = createAndSaveDummyAppWideReminder()
             val deckSpecificCreationTime = reviewReminderDeckSpecific.latestNotifTime
             val appWideCreationTime = reviewReminderAppWide.latestNotifTime
 
-            val intentDeckSpecific = reviewReminderDeckSpecific.getNotifIntent(NotifIntent.SNOOZE)
-            val intentAppWide = reviewReminderAppWide.getNotifIntent(NotifIntent.SNOOZE)
-            NotificationService().onReceive(context, intentDeckSpecific)
-            NotificationService().onReceive(context, intentAppWide)
+            triggerDummyReminderNotification(reviewReminderDeckSpecific, isRecurring = false)
+            triggerDummyReminderNotification(reviewReminderAppWide, isRecurring = false)
 
             verifyNotifSent(reviewReminderDeckSpecific)
             verifyNotifSent(reviewReminderAppWide)
@@ -270,11 +269,8 @@ class NotificationServiceTest : RobolectricTest() {
             val slotOne = slot<Notification>()
             val slotTwo = slot<Notification>()
 
-            val intentOne = reviewReminderOne.getNotifIntent(NotifIntent.RECURRING)
-            NotificationService().onReceive(context, intentOne)
-
-            val intentTwo = reviewReminderTwo.getNotifIntent(NotifIntent.RECURRING)
-            NotificationService().onReceive(context, intentTwo)
+            triggerDummyReminderNotification(reviewReminderOne)
+            triggerDummyReminderNotification(reviewReminderTwo)
 
             verifyNotifSent(reviewReminderOne, slot = slotOne)
             verifyNotifSent(reviewReminderTwo, slot = slotTwo)
@@ -301,9 +297,15 @@ class NotificationServiceTest : RobolectricTest() {
         return reviewReminder
     }
 
-    private fun triggerDummyReminderNotification(reviewReminder: ReviewReminder) {
-        val intent = reviewReminder.getNotifIntent(NotifIntent.RECURRING)
-        NotificationService().onReceive(context, intent)
+    private suspend fun triggerDummyReminderNotification(
+        reviewReminder: ReviewReminder,
+        isRecurring: Boolean = true,
+    ) {
+        NotificationService.handleReviewReminderNotification(
+            context,
+            reviewReminder,
+            isRecurringNotification = isRecurring,
+        )
     }
 
     /**
@@ -367,20 +369,4 @@ class NotificationServiceTest : RobolectricTest() {
             assertThat(previousTime, equalTo(storedReminder.latestNotifTime))
         }
     }
-
-    /**
-     * Convenience enum class to minimize verbosity in test methods when using [getNotifIntent].
-     */
-    private enum class NotifIntent {
-        RECURRING,
-        SNOOZE,
-    }
-
-    private fun ReviewReminder.getNotifIntent(action: NotifIntent) =
-        when (action) {
-            NotifIntent.RECURRING -> NotificationService.NotificationServiceAction.ScheduleRecurringNotifications
-            NotifIntent.SNOOZE -> NotificationService.NotificationServiceAction.SnoozeNotification
-        }.let { action ->
-            NotificationService.getIntent(context, this, action)
-        }
 }