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 mutex. 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

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: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.

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.

It'd be best to add a document in /docs/ with your thoughts.

Longterm we're going to have a lot more dev capacity for refactoring.

Good idea. In fact, Arthur's told me before to add longer-form docs explaining how review reminders are designed so other developers will be able to pick up work on the system. I might as well do that now too.

@david-allison Where do you want me to put them? A new folder of markdown files at https://github.com/ankidroid/Anki-Android/tree/main/docs? Or maybe the wiki (https://github.com/ankidroid/Anki-Android/wiki)?

https://github.com/ankidroid/Anki-Android/tree/main/docs/ankidroid

david-allison

I've been through all but the first commit and I'm initially happy.

I think we're taking on a lot of complexity with the queue, and I don't want to review heavily if moving to a Mutex/lock simplifies the code.

david-allison

Excellent!

david-allison · 2026-04-23T11:37:22Z

+     * 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.


It'd be best to add a document in /docs/ with your thoughts.

Longterm we're going to have a lot more dev capacity for refactoring.

david-allison · 2026-04-23T11:50:37Z

+            // Record this latest routine notification-firing attempt's timestamp
+            reminder.updateLatestNotifTime()
+            editReminder(reminder) { reminders: ReviewReminderGroup ->
+                reminders.apply { this[reminder.id] = reminder }


I don't think this is fully safe - if there are concurrent invocations, then the provided reminder replaces the database value, rather than being read from the database and updated.

Ah poop, good point. Thanks for pointing this out, I think I've fixed this now.

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 via a mutex. 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. 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.

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

Follow-up for ankidroid#20747 Also a small comment change requested by David!

ericli3690 · 2026-04-27T05:37:17Z

@david-allison

Responded to / resolved all your comments! Ready for review. Thanks again!

Changes:

Renamed editReminder to editReminderForScope to better reflect what it's doing
Refactored getReminderForDeck to now be getReminderForScope, because it's a cheap fix that streamlines tests and keeps it more in line with editReminderForScope.
Refactored prepareForNotifAttempt as per David's comment above.
Modified tests accordingly.

david-allison

LGTM, I feel the docs could be split into a separate PR