refactor(reminders): remove race conditions, simplify database interface by ericli3690 · Pull Request #20755 · ankidroid/Anki-Android

ericli3690 · 2026-04-16T06:48:23Z

Purpose / Description

There are two possible race conditions in the review reminder code which are resolved here. See Approach below.

David raised a good point at #20325 in that lambdas like upsertReminder and toggleReminder should not be left public for consumers to abuse. Rather, I realized the ReviewRemindersDatabase should be clear about what exactly can be done to stored review reminders: they can be upserted, toggled, and deleted. Any other action should not be permitted by default. Additionally, I realized some of the methods were unnecessary: getAllAppWideReminders and getAllDeckSpecificReminders are never called independently anywhere, they are only ever called in conjunction and then merged together. Hence, we should offer a single getAllReminders method to keep things simple and efficient.

Also addressed other minor nits at #20325.

Unit tests modified accordingly.

Fixes

Follow up to:

feat(reminders): database overhaul #20325

Approach

Regarding the race conditions:

editReviewReminder reads from sharedPrefs, does processing, then writes; it is possible for another write to happen during the critical period between read and write, causing overwrites and lost data.

This is resolved by implementing a single limitedParallelism(1) queue, similar to how the Collection does it. However, it means that every invocation of the ReviewRemindersDatabase needs to be able to suspend, which propagates the changes somewhat.

When AlarmManagerService triggers NotificationService's immediate notification functionality, because it takes a while to dispatch something to an onReceive method, there is a small gap between the notification being requested and onReceive running during which shouldImmediatelyFire could run and return true again.

Previously, this was resolved by just setting latestNotifTime redundantly, once in AlarmManagerService and once in NotificationService. However, I realized this was a code smell and have refactored the code to only have a single latestNotifTime update in NotificationService. AlarmManagerService now eagerly performs an immediate fire if directed to do so, primarily by the scheduleAllNotifications flow. For other uses of AlarmManagerService, ex. a recurring reminder scheduling its next firing, we do not need to have AlarmManagerService eagerly perform an immediate fire (technically, it is safe to do so as NotificationService checks the latestNotifTime carefully, but it would clutter up the logs with confusing messages) and hence set a boolean flag enabling the eager firing to false.

Then, I simplified the ReviewRemindersDatabase interface to only expose simple functions like toggleReminder rather than lambda-accepting functions like editAllAppWideReminders.

How Has This Been Tested?

Unit tests.
Creating, editing, toggling, deleting, viewing review reminders all works on a physical Samsung S23, API 34.
Notifications can be scheduled and are sent.
Closing the app during the 10-minute window where a notification should be sending and then opening the app again triggers the notification.

Learning

I still have a lot to learn about concurrent programming!

Checklist

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code

There are two possible race conditions in the review reminder code which are resolved here. 1. editReviewReminder reads from sharedPrefs, does processing, then writes; it is possible for another write to happen during the critical period between read and write, causing overwrites and lost data. This is resolved by implementing a single `limitedParallelism(1)` queue, similar to how the Collection does it. However, it means that every invocation of the ReviewRemindersDatabase needs to be able to suspend, which propagates the changes somewhat. 2. When AlarmManagerService triggers NotificationService's immediate notification functionality, because it takes a while to dispatch something to an `onReceive` method, there is a small gap between the notification being requested and `onReceive` running which during which `shouldImmediatelyFire` could run and return true again. Previously, this was resolved by just setting `latestNotifTime` redundantly, once in AlarmManagerService and once in NotificationService. However, I realized this was a code smell and have refactored the code to only have a single `latestNotifTime` update in NotificationService. AlarmManagerService now eagerly performs an immediate fire if directed to do so, primarily by the `scheduleAllNotifications` flow. For other uses of AlarmManagerService, ex. a recurring reminder scheduling its next firing, we do not need to have AlarmManagerService eagerly perform an immediate fire (technically, it is safe to do so as NotificationService checks the `latestNotifTime` carefully, but it would clutter up the logs with confusing messages) and hence set a boolean flag enabling the eager firing to false. Unit tests modified accordingly

Some minor changes requested at ankidroid#20325 by David and Ashish.

ericli3690 · 2026-04-16T06:48:58Z

+ * Edits to instances of this class are not automatically persisted to SharedPreferences;
+ * that functionality is provided by [ReviewRemindersDatabase].
+ *


Comment requested by David and Ashish

ericli3690 · 2026-04-16T06:49:31Z

+     * Checks if this review reminder has successfully delivered a routine daily (non-snooze) notification in the time between
+     * its latest scheduled firing time and now. If so, this method returns true.
     */
-    fun shouldImmediatelyFire(): Boolean {
+    fun latestNotifDelivered(): Boolean {


I've inverted this guy (latestNotifDelivered() = !shouldImmediatelyFire()) for better readability. Tests updated accordingly.

ericli3690 · 2026-04-16T06:50:32Z

+/**
+ * Convenience typealias for the mutation functions passed to editors of [ReviewReminderGroup].
+ */
+typealias ReviewReminderGroupEditor = (ReviewReminderGroup) -> ReviewReminderGroup


Moved to top of file as requested by David

ericli3690 · 2026-04-16T06:50:49Z

-    constructor(vararg pairs: Pair<ReviewReminderId, ReviewReminder>) : this(
-        buildMap { pairs.forEach { put(it.first, it.second) } },
-    )
+    constructor(vararg pairs: Pair<ReviewReminderId, ReviewReminder>) : this(hashMapOf(*pairs))


Refactor requested by David

ericli3690 · 2026-04-16T06:51:25Z

+    fun forEach(action: (Pair<ReviewReminderId, ReviewReminder>) -> Unit) {
+        underlyingMap.forEach { (id, reminder) -> action(id to reminder) }


So that we can do forEach { (id, reminder) -> ... instead of forEach { id, reminder -> ...

ericli3690 · 2026-04-16T06:51:52Z

+ * These classes cannot be moved directly to the test file because [ReviewReminderSchema] is a sealed interface,
+ * meaning all implementations of it must be within the same module.


Explanation requested by David as to why we can't move this object to a separate test file.

ericli3690 · 2026-04-16T06:52:07Z

+     * - Version 1: [ReviewReminderSchemaV1]: 3 August 2025 -  Initial version
+     * - Version 2: [ReviewReminderSchemaV2]: 25 January 2026 - Added [ReviewReminder.onlyNotifyIfNoReviews]
+     * - Version 3: [ReviewReminder]: 8 February 2026 - Added [ReviewReminder.latestNotifTime]


Annotations requested by David

ericli3690 · 2026-04-16T06:53:35Z

     * @param key
-     * @param editor A lambda that takes the current map and returns the updated map.
-     * @param expectedScope The expected scope of all review reminders in the resulting map.
+     * @param editor A lambda that takes the current [ReviewReminderGroup] and returns the updated [ReviewReminderGroup].


I kept editRemindersForKey even though it's only ever used by editReminder and could technically be merged with it because I like the parallelism with getRemindersForKey

ericli3690 · 2026-04-16T06:56:37Z

-            this[reminder.id] = reminder
-        }
-    }
+            if (errorString.isEmpty()) return


Early return requested by David

ericli3690 · 2026-04-16T06:56:45Z

+            context.showError(
+                message =
+                    "An error occurred while loading your review reminders, corrupted reminders have been deleted. " +
+                        "Details:\n\n${errorString.ellipsize(1000)}",


Truncation requested by David

ericli3690 · 2026-04-16T06:58:06Z

         * triggering [NotificationService.sendReviewReminderNotification], this method is called again to
         * schedule the next upcoming notification. If for some reason the next day's alarm fails to be set by
-         * the current day's notification, we fall back to setting alarms whenever the app is opened: see
+         * the current day's notification, we fall back to setting alarms whenever the application process is started: see


Past me thought that AnkiDroidApp's onCreate method only ran when the app itself opened... whoops!

Might want to document that for others in AnkiDroidApp

ericli3690 · 2026-04-16T06:58:50Z

                set(Calendar.MINUTE, reviewReminder.time.minute)
                set(Calendar.SECOND, 0)
-                if (before(currentTimestamp)) {
+                if (before(currentTimestamp) || this == currentTimestamp) {


This new branch is unit tested in AlarmManagerServiceTest.

ericli3690 · 2026-04-16T06:59:39Z

+            runGloballyWithTimeout(SCHEDULE_NOTIFICATIONS_TIMEOUT) {
+                AlarmManagerService.scheduleAllNotifications(context)
+            }


Needs to goAsync because scheduleAllNotifications is now suspending

ericli3690 · 2026-04-16T07:01:34Z

     */
    @Test
    fun `raw ReviewReminder string can be deserialized without throwing`() {
+        @Language("JSON")


Requested by David

ericli3690 · 2026-04-16T07:02:50Z

+                    ReviewRemindersDatabase
+                        .getAllReminders()
+                        .getRemindersList()
+                        .filter { it.scope is ReviewReminderScope.Global }
+                        .associateBy { it.id }
+                        .let { ReviewReminderGroup(it) }


The only con of eliminating the superfluous getAllAppWideReminders is that this test function becomes a little messier. But again, as I mentioned in the commit message, getAllAppWideReminders is never used in the actual code itself, and hence I don't think the Database interface should provide it. I thought of making a separate helper function here... but it's just done once, so I think it's unnecessary.

ericli3690 · 2026-04-16T07:03:49Z

    @Test
-    fun `notification should immediately fire if there was no scheduled firing`() {
-        shouldImmediatelyFireTest(
+    fun `latest notification not delivered if there was no scheduled firing`() {


The modifications in this test file are essentially just handling the inversion of latestNotifDelivered() = !shouldImmediatelyFire()

ericli3690 · 2026-04-16T07:05:46Z

-    /**
-     * Helper function that tries scheduling multiple reminders and verifies the outcome for each.
-     */
-    private fun scheduleAllNotificationsTest(vararg testCases: ScheduleAllNotificationsTestCase) {


This big bloated test helper function is no longer necessary as the actual logic for checking for immediate firing has been moved to NotificationService and is hence tested by NotificationServiceTest and ReviewReminderTest.

ericli3690 · 2026-04-16T07:06:22Z

+        private val yesterday = MockTime(TimeManager.time.intTimeMS() - 1.days.inWholeMilliseconds)
+        private val today = MockTime(TimeManager.time.intTimeMS())


The step was causing problems. Manually setting time is cleaner.

Copilot

Pull request overview

This PR refactors the review-reminders persistence + scheduling flow to reduce race conditions and constrain the database interface to a small set of explicit operations.

Changes:

Make ReviewRemindersDatabase operations suspend and serialize reads/writes via a single-concurrency queue; replace lambda-based editors with upsertReminder/toggleReminder/deleteReminder and unify reads under getAllReminders.
Rework reminder notification scheduling to optionally attempt immediate sends (attemptImmediateNotification) while relying on latestNotifTime bookkeeping to dedupe redundant sends.
Update and expand unit tests to match the new database + scheduling APIs and updated “missed notification” detection logic.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
AnkiDroid/src/test/java/com/ichi2/testutils/ext/ReviewRemindersDatabase.kt	Removes test-only `storeReminders` helper now that DB has explicit operations.
AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt	Updates tests to use new DB APIs + adds cases around scheduled-time edge conditions.
AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt	Updates signature and adds coverage for immediate-notification flag behavior.
AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt	Reworks DB tests around suspend APIs and new CRUD-style interface.
AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderTest.kt	Updates tests for renamed/repurposed immediate-fire/delivery detection method.
AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt	Moves dedupe logic into NotificationService bookkeeping and updates DB persistence calls.
AnkiDroid/src/main/java/com/ichi2/anki/services/BootService.kt	Runs notification scheduling via `runGloballyWithTimeout` to support suspend scheduling.
AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt	Adds `attemptImmediateNotification` flag and makes `scheduleAllNotifications` suspend.
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleRemindersAdapter.kt	Simplifies toggle callback shape to pass the reminder object.
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt	Updates UI flows to call new DB APIs and updated alarm scheduling signature.
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt	Introduces single-threaded dispatcher queue + new explicit DB operations and `getAllReminders`.
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt	Clarifies testing schema migration constraints/documentation.
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderGroup.kt	Moves `ReviewReminderGroupEditor` typealias + minor API tweaks.
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt	Renames/changes “missed notification” detection helper and updates documentation.
AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.kt	Calls suspend scheduling from `applicationScope.launch`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T07:06:59Z

@@ -257,7 +259,7 @@ data class ReviewReminder private constructor(
            }
        }

-        return latestNotifTime < latestScheduledTimestamp.timeInMillis
+        return latestNotifTime >= latestScheduledTimestamp.timeInMillis
    }


latestNotifDelivered() is documented as checking whether a notification was “successfully delivered”, but latestNotifTime is updated on attempt (see updateLatestNotifTime() and its call site in NotificationService.handleReviewReminderNotification()), including cases where sendReviewReminderNotification() exits early (deck not accessible / onlyNotifyIfNoReviews / threshold). This makes the name/doc misleading and can confuse future changes. Consider either updating the KDoc to reflect attempt/processing semantics, or renaming the method to match what it actually represents (e.g., “latestNotifAttempted” / “latestNotifTimeUpToDate”).

Modified the docstring for this file to make this more clear. Unfortunately it's a little too late to change this, as latestNotifTime was in the schema migration and is stored on-device, so changing this would not just be cosmetic, we'd have to write an entire new migration. That's probably not worth it.

ericli3690 · 2026-04-16T07:07:11Z

-    private fun createAndSaveDummyDeckSpecificReminder(did: DeckId): ReviewReminder {
-        val reviewReminder = createTestReminder(deckId = did, thresholdInt = 1)
-        ReviewRemindersDatabase.storeReminders(reviewReminder)
-        return reviewReminder
-    }
-
-    private fun createAndSaveDummyAppWideReminder(): ReviewReminder {
-        val reviewReminder = createTestReminder(thresholdInt = 1)
-        ReviewRemindersDatabase.storeReminders(reviewReminder)
-        return reviewReminder
-    }


These became unnecessary after I streamlined attemptNotif more.

ericli3690 · 2026-04-16T07:08:21Z

-import com.ichi2.anki.reviewreminders.ReviewReminderScope.Global
-import com.ichi2.anki.reviewreminders.ReviewRemindersDatabase
-
-fun ReviewRemindersDatabase.storeReminders(vararg reminders: ReviewReminder) {


No longer needed! This logic has been cleanly defined in a single place in the Database's editReminder method, meaning we can just do a simple forEach { upsertReminder(it) } instead of using storeReminders in test files.

David raised a good point at ankidroid#20325 in that lambdas like `upsertReminder` and `toggleReminder` should not be left public for consumers to abuse. Rather, I realized the ReviewRemindersDatabase should be clear about what exactly can be done to stored review reminders: they can be upserted, toggled, and deleted. Any other action should not be permitted by default. Additionally, I realized some of the methods were unnecessary: `getAllAppWideReminders` and `getAllDeckSpecificReminders` are never called independently anywhere, they are only ever called in conjunction and then merged together. Hence, we should offer a single `getAllReminders` method to keep things simple and efficient. Also addressed other minor nits at ankidroid#20325. Unit tests modified accordingly.

ericli3690 · 2026-04-16T08:18:13Z

+     * This method is intended to be lightweight and hence will not go out of its way to validate that an
+     * update has not been performed.
+     */
+    suspend fun insertReminder(reminder: ReviewReminder) =


insert rather than create because there's already a ReviewReminder.createReviewReminder method. I want to be clear that this handles persistence, not creation.

ericli3690 · 2026-04-16T08:18:29Z

+     * Important: Do not use this method for editing review reminders, and in particular
+     * do not use this method for changing the [ReviewReminderScope] of a reminder, as review reminders of
+     * different scopes are stored separately and cannot be cleanly updated in a single operation. The old
+     * review reminder must be deleted first, or else a duplicate [ReviewReminderId] will be introduced.
+     * In general, when you want to edit a [ReviewReminder], use [deleteReminder] first, then [insertReminder].
+     * This method is intended to be lightweight and hence will not go out of its way to validate that an
+     * update has not been performed.


In retrospect, ReviewReminderScope is one of my major design regrets with review reminders. It's more like a part of the ReviewReminderId than it is a "customizable" aspect of a review reminder; put another way, it's part of the key for locating / writing a review reminder, not a property of it, and so making it a field of ReviewReminder was a mistake. I think it's a little late to change now though, and "fixing" this would require a massive rethink and refactor of the entire review reminder system. I'd like to prioritize getting the feature out to users first and foremost! Hence I'll just put a big red docstring warning here and hope consumers don't violate this invariant.

ericli3690 added 2 commits April 15, 2026 23:16

style: minor review reminder comments and reorganization

54a3f7d

Some minor changes requested at ankidroid#20325 by David and Ashish.