From f43d580cc74c08d3b0ad748e612a6e33c12923c8 Mon Sep 17 00:00:00 2001 From: null Date: Thu, 20 Aug 2026 22:42:09 -0500 Subject: [PATCH] fix: make a reminder arrive when the user asked for it Changing the reminder time did not move the reminder. WorkManager's UPDATE policy carries the previous request's lastEnqueueTime and periodCount forward, and a periodic request computes its next run as periodCount == 0 ? lastEnqueueTime + initialDelay : lastEnqueueTime + interval. Both halves bit: After the first run, a new initial delay is ignored entirely -- Morning to Evening did nothing at all. Before it, the delay is applied to the ORIGINAL enqueue time, and the coordinator reschedules at every process start, so asking for 19:00 at 09:00 on work enqueued at 08:00 produced 18:00, with every later period anchored off that. The scheduler reads the existing work first: KEEP when nothing is scheduled, leave an overdue run alone -- moving it skips today's reminder entirely -- leave a run already within five minutes alone, and otherwise UPDATE with an explicit setNextScheduleTimeOverride, which is the only way to say WHEN rather than how long from a moment WorkManager has its own opinion about. CANCEL_AND_REENQUEUE is wrong for a subtler reason: this runs at every process start including the one WorkManager started to run the worker, and cancelling the unique work there cancels the worker. A time zone or clock change now re-aims it. The delay was computed once, from the zone in force then, so flying east left the reminder arriving at the old wall-clock time indefinitely. WorkManager's own RescheduleReceiver declares BOOT_COMPLETED and nothing else -- which is why ClockChangeReceiver exists for the other two broadcasts, and why it does not duplicate boot. Unexported, no permission, checkPermissions still green. No flex window was added, and the screen's copy changed instead. Flex would have made "a few minutes either side" true and placed the first run nearly a full period out, skipping the reminder on the day the user set it -- to keep a sentence. It now says Android may deliver a few minutes after, never before, which is what actually happens. schedule() had no test; only the arithmetic beneath it did. Nine now, against WorkManager's own recorded next-run time, sharing one clock with it -- a test that fixes only the scheduler's measures a 2026 delay against a real System.currentTimeMillis(). Writing them was necessary rather than tidy: the first version of the change-the-time test passed with the defect still in place, because both schedules happened at the same instant and the bug only bites once time has moved. The test that catches it advances the clock an hour between them, which is what a real second process start does. closes #72 Co-Authored-By: Claude Opus 5 (1M context) --- app/src/main/AndroidManifest.xml | 21 ++ .../dev/privacyllc/period/di/DataModule.kt | 3 +- .../settings/NotificationSettingsScreen.kt | 7 +- .../notifications/ClockChangeReceiver.kt | 53 +++++ .../notifications/ReminderCoordinator.kt | 43 +++- .../NotificationSettingsViewModelTest.kt | 14 +- .../notifications/ReminderCoordinatorTest.kt | 9 +- .../core/notifications/ReminderScheduler.kt | 144 ++++++++++---- .../notifications/ReminderSchedulerTest.kt | 12 +- .../WorkManagerReminderSchedulerTest.kt | 186 ++++++++++++++++++ docs/architecture/README.md | 39 +++- docs/design/README.md | 12 ++ 12 files changed, 496 insertions(+), 47 deletions(-) create mode 100644 app/src/main/kotlin/dev/privacyllc/period/notifications/ClockChangeReceiver.kt create mode 100644 core/notifications/src/test/kotlin/dev/privacyllc/period/core/notifications/WorkManagerReminderSchedulerTest.kt diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index d1fc8f7..738d0eb 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -78,6 +78,27 @@ + + + + + + + + + diff --git a/app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt b/app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt index aae782e..22723c2 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt @@ -16,6 +16,7 @@ import dev.privacyllc.period.core.data.CycleData import dev.privacyllc.period.core.data.CycleRepository import dev.privacyllc.period.core.datastore.UserPreferencesRepository import dev.privacyllc.period.core.notifications.ReminderScheduler +import dev.privacyllc.period.core.notifications.WorkManagerReminderScheduler import dev.privacyllc.period.core.notifications.PeriodNotifier import dev.privacyllc.period.core.notifications.ReminderNotifier import dev.privacyllc.period.core.security.AppLockRepository @@ -141,7 +142,7 @@ object DataModule { @Provides @Singleton fun reminderScheduler(@ApplicationContext context: Context): ReminderScheduler = - ReminderScheduler(context) + WorkManagerReminderScheduler(context) @Provides @Singleton diff --git a/app/src/main/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsScreen.kt b/app/src/main/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsScreen.kt index c1fb660..9dc69e9 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsScreen.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsScreen.kt @@ -195,8 +195,11 @@ private fun NotificationSettingsContent( Spacer(Modifier.height(24.dp)) Text( - "Reminders are scheduled with WorkManager and may arrive a few minutes either " + - "side of this time. Nothing about your cycle leaves the device.", + // "either side" was not true and could not be made true: a periodic + // request is deferred by the system, never brought forward, and the + // flex window that would allow "before" also skips the first day. + "Android may deliver a reminder a few minutes after this time, never before. " + + "Nothing about your cycle leaves the device.", style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, ) diff --git a/app/src/main/kotlin/dev/privacyllc/period/notifications/ClockChangeReceiver.kt b/app/src/main/kotlin/dev/privacyllc/period/notifications/ClockChangeReceiver.kt new file mode 100644 index 0000000..e2ecbf6 --- /dev/null +++ b/app/src/main/kotlin/dev/privacyllc/period/notifications/ClockChangeReceiver.kt @@ -0,0 +1,53 @@ +package dev.privacyllc.period.notifications + +import android.content.BroadcastReceiver +import android.content.Context +import android.content.Intent +import dagger.hilt.EntryPoint +import dagger.hilt.InstallIn +import dagger.hilt.android.EntryPointAccessors +import dagger.hilt.components.SingletonComponent + +/** + * The clock or the time zone moved; re-aim the reminder. + * + * The delay to the next reminder is computed once, when it is scheduled, from + * the zone in force at that moment. Nothing recomputed it afterwards, so flying + * to another continent left the reminder arriving at the old wall-clock time + * indefinitely — and a manual clock change did the same. + * + * **Boot is deliberately not handled here.** WorkManager persists the periodic + * request and its own `RescheduleReceiver` restores it after a reboot; that + * receiver's manifest entry declares `BOOT_COMPLETED` and nothing else, which is + * exactly why the two broadcasts below need an entry of their own. A second boot + * receiver would duplicate work already done, and the coordinator's schedule at + * process start re-anchors any drift anyway. + * + * Unexported, like WorkManager's own: these are system broadcasts, and nothing + * else has any business sending them to this app. + * + * Reached through an entry point rather than `@AndroidEntryPoint`. The annotation + * generates a base class whose `onReceive` performs the injection and which the + * subclass must call through `super` — and `BroadcastReceiver.onReceive` is + * abstract, so that call resolves against the wrong class whenever the generated + * one is not yet on the compile path. An accessor has no such ordering. + */ +class ClockChangeReceiver : BroadcastReceiver() { + + @EntryPoint + @InstallIn(SingletonComponent::class) + interface Dependencies { + fun reminderCoordinator(): ReminderCoordinator + } + + override fun onReceive(context: Context, intent: Intent) { + when (intent.action) { + Intent.ACTION_TIMEZONE_CHANGED, Intent.ACTION_TIME_CHANGED -> { + EntryPointAccessors + .fromApplication(context.applicationContext, Dependencies::class.java) + .reminderCoordinator() + .clockChanged() + } + } + } +} diff --git a/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt b/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt index 53c6875..cd295c8 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/notifications/ReminderCoordinator.kt @@ -6,6 +6,7 @@ import dev.privacyllc.period.core.datastore.UserPreferencesRepository import dev.privacyllc.period.core.notifications.ReminderScheduler import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged @@ -61,6 +62,24 @@ class ReminderCoordinator @Inject constructor( ).launchIn(scope) } + /** + * The clock or the time zone moved, so the schedule has to be re-aimed. + * + * The delay to the next reminder is worked out once, at the moment it is + * scheduled, from the zone in force then. Fly to another continent and the + * reminder keeps arriving at the old wall-clock time indefinitely: nothing + * recomputed it, because nothing was listening for the two broadcasts that + * say so. + * + * A counter rather than a call, so this joins the same key the rest of the + * chain uses and cannot race the state it is bumping. + */ + fun clockChanged() { + _clockGeneration.value += 1 + } + + private val _clockGeneration = MutableStateFlow(0) + /** * Reschedule when the forecast date, the reminder time, or whether anything * is enabled at all changes. @@ -96,15 +115,35 @@ class ReminderCoordinator @Inject constructor( internal fun scheduleUpdates( forecastDates: Flow, preferences: Flow, + clockGeneration: Flow = _clockGeneration, ): Flow<*> = combine( forecastDates, preferences.map { it.reminderTime to anyEnabled(it) }, - ) { forecastDate, (time, enabled) -> Triple(forecastDate, time, enabled) } + clockGeneration, + ) { forecastDate, (time, enabled), generation -> + ScheduleKey(forecastDate, time, enabled, generation) + } .distinctUntilChanged() - .onEach { (_, time, enabled) -> apply(time, enabled) } + .onEach { apply(it.time, it.enabled) } .catch { } + /** + * What has to change before the schedule is touched. + * + * The forecast date earns its place by being part of the key rather than by + * being used: it is what makes a "Not yet" reach this chain at all. The + * daily check reads the forecast when it wakes, so it does not need aiming + * at a particular one — §31's "reschedule after a forecast change" is + * satisfied by the waking, not by the aiming. + */ + private data class ScheduleKey( + val forecastDate: LocalDate?, + val time: LocalTime, + val enabled: Boolean, + val clockGeneration: Int, + ) + private suspend fun apply(time: LocalTime, enabled: Boolean) { if (enabled) scheduler.schedule(time) else scheduler.cancel() } diff --git a/app/src/test/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsViewModelTest.kt b/app/src/test/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsViewModelTest.kt index 0b8c726..601b889 100644 --- a/app/src/test/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsViewModelTest.kt +++ b/app/src/test/kotlin/dev/privacyllc/period/feature/settings/NotificationSettingsViewModelTest.kt @@ -7,6 +7,8 @@ import androidx.test.core.app.ApplicationProvider import dev.privacyllc.period.core.datastore.NotificationPrivacy import dev.privacyllc.period.core.datastore.UserPreferencesRepository import dev.privacyllc.period.core.notifications.ReminderScheduler +import java.time.LocalTime +import java.time.ZoneId import dev.privacyllc.period.notifications.NotificationPermissionState import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay @@ -48,9 +50,18 @@ class NotificationSettingsViewModelTest { override fun canPost(privacy: NotificationPrivacy) = canPost } + /** Records what it was asked to do; the scheduling itself is tested in core. */ + private class FakeScheduler : ReminderScheduler { + var scheduled = 0 + var cancelled = 0 + override suspend fun schedule(reminderTime: LocalTime, zone: ZoneId) { scheduled++ } + override fun cancel() { cancelled++ } + } + private val dispatcher = UnconfinedTestDispatcher() private lateinit var permissions: FakePermissions private lateinit var preferences: UserPreferencesRepository + private lateinit var scheduler: FakeScheduler private lateinit var vm: NotificationSettingsViewModel @Before fun setUp() { @@ -62,7 +73,8 @@ class NotificationSettingsViewModelTest { }, ) permissions = FakePermissions() - vm = NotificationSettingsViewModel(preferences, ReminderScheduler(context), permissions) + scheduler = FakeScheduler() + vm = NotificationSettingsViewModel(preferences, scheduler, permissions) } @After fun tearDown() = Dispatchers.resetMain() diff --git a/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt b/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt index ef6b753..3341abb 100644 --- a/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt +++ b/app/src/test/kotlin/dev/privacyllc/period/notifications/ReminderCoordinatorTest.kt @@ -41,6 +41,13 @@ import java.time.Clock @Config(sdk = [34]) class ReminderCoordinatorTest { + /** The coordinator's job is deciding when to reschedule, not how. */ + private class FakeScheduler : ReminderScheduler { + override suspend fun schedule(reminderTime: java.time.LocalTime, zone: java.time.ZoneId) = Unit + override fun cancel() = Unit + } + + private lateinit var coordinator: ReminderCoordinator private lateinit var preferences: UserPreferencesRepository @@ -52,7 +59,7 @@ class ReminderCoordinatorTest { coordinator = ReminderCoordinator( repository = CycleData.repository(context, PersonalPredictionEngine(), Clock.systemUTC()), preferences = preferences, - scheduler = ReminderScheduler(context), + scheduler = FakeScheduler(), ) } diff --git a/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderScheduler.kt b/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderScheduler.kt index 51a5cbd..b0fc2b2 100644 --- a/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderScheduler.kt +++ b/core/notifications/src/main/kotlin/dev/privacyllc/period/core/notifications/ReminderScheduler.kt @@ -5,63 +5,141 @@ import androidx.work.Constraints import androidx.work.ExistingPeriodicWorkPolicy import androidx.work.PeriodicWorkRequestBuilder import androidx.work.WorkManager +import kotlinx.coroutines.flow.first +import java.time.Clock import java.time.Duration import java.time.LocalDate import java.time.LocalDateTime import java.time.LocalTime import java.time.ZoneId import java.util.concurrent.TimeUnit +import kotlin.math.abs /** - * Schedules the daily reminder check with WorkManager. + * Schedules the daily reminder check. + * + * An interface because the settings screen and the coordinator both drive it, + * and both are worth testing without WorkManager underneath. + */ +interface ReminderScheduler { + /** + * Aim the daily check at [reminderTime] in [zone]. + * + * Called on every change that could move a reminder: a toggle changing, the + * reminder time changing, the clock or the time zone changing. §31 lists + * rescheduling after a confirmation or a forecast change explicitly — that + * one is satisfied by construction, since the check runs daily and reads the + * forecast when it wakes rather than being aimed at a particular forecast. + */ + suspend fun schedule(reminderTime: LocalTime, zone: ZoneId = ZoneId.systemDefault()) + + fun cancel() +} + +/** + * The WorkManager implementation, and three of its behaviours worth knowing. * * §31: **no exact alarms.** A period reminder does not need alarm-clock - * precision, and `SCHEDULE_EXACT_ALARM` is Play scrutiny bought for nothing — - * so this is a periodic work request with a flex window, which the system is - * free to move by minutes. Nobody notices a reminder arriving at 10:07 instead - * of 10:00; everybody notices an app asking for an alarm permission. + * precision, and `SCHEDULE_EXACT_ALARM` is Play scrutiny bought for nothing. + * `checkPermissions` fails the build if one reaches the merged manifest. * - * The work is `KEEP`-replaced on every reschedule rather than cancelled and - * re-enqueued, so a forecast that moves twice in a minute does not produce two - * pending workers. + * ## Why this reads the existing work before touching it + * + * `ExistingPeriodicWorkPolicy.UPDATE` keeps the old request's `lastEnqueueTime` + * and `periodCount`, and the next run is computed as + * `periodCount == 0 ? lastEnqueueTime + initialDelay : lastEnqueueTime + interval`. + * Two consequences, both of which shipped: + * + * - Changing the reminder from Morning to Evening **after the first reminder + * had fired did nothing at all** — the new initial delay is ignored once + * `periodCount` is past zero. + * - Re-scheduling *before* the first run — which every process start did, via + * the coordinator — set the next run to *the original enqueue time* plus the + * *new* delay. Opened the next morning, that lands in the past, so the + * reminder fired early and every later period anchored off it. + * + * So: enqueue with `KEEP` when nothing is scheduled, leave an overdue run alone + * (moving it would skip today), leave a run that is already close enough alone, + * and otherwise `UPDATE` with an explicit `setNextScheduleTimeOverride`, which + * is the only way to move a periodic request that has already run. + * + * `CANCEL_AND_REENQUEUE` would be simpler and is wrong here: this is called at + * every process start, including the one WorkManager itself started to run the + * worker, and cancelling the unique work there cancels the running worker. + * + * ## Why there is no flex window + * + * A flex window looks like the polite thing to ask for and would skip the first + * day: with flex, the first run is placed at `start + interval − flex`, so a + * fifteen-minute flex on a daily period aims the first reminder at roughly + * 23h45m from now. The screen's copy says a reminder may arrive a few minutes + * *after* the chosen time, which is what actually happens — the system defers, + * it does not anticipate. */ -class ReminderScheduler(private val context: Context) { +class WorkManagerReminderScheduler( + private val context: Context, + private val clock: Clock = Clock.systemDefaultZone(), +) : ReminderScheduler { - /** - * (Re)schedule the daily check for [reminderTime]. - * - * Called on every change that could move a reminder: the forecast moving, a - * toggle changing, the reminder time changing. §31 lists rescheduling after - * a confirmation or a forecast change explicitly, and it is the requirement - * most likely to be missed — a "Not yet" moves the forecast, so a reminder - * aimed at the old one is now aimed at the wrong day. - */ - fun schedule(reminderTime: LocalTime, zone: ZoneId = ZoneId.systemDefault()) { - val request = PeriodicWorkRequestBuilder(1, TimeUnit.DAYS) - .setInitialDelay(delayUntil(reminderTime, zone).toMinutes(), TimeUnit.MINUTES) - .setConstraints( - // No network, no charging, no idle. A reminder that waits for - // Wi-Fi is a reminder that arrives on Tuesday. - Constraints.Builder().build(), + override suspend fun schedule(reminderTime: LocalTime, zone: ZoneId) { + val now = clock.millis() + val target = now + delayUntil(reminderTime, zone, LocalDateTime.now(clock.withZone(zone))).toMillis() + val manager = WorkManager.getInstance(context) + + val existing = manager.getWorkInfosForUniqueWorkFlow(WORK_NAME).first() + .firstOrNull { !it.state.isFinished } + + when { + existing == null -> manager.enqueueUniquePeriodicWork( + WORK_NAME, + ExistingPeriodicWorkPolicy.KEEP, + daily().setInitialDelay(target - now, TimeUnit.MILLISECONDS).build(), ) - .addTag(TAG) - .build() - WorkManager.getInstance(context).enqueueUniquePeriodicWork( - WORK_NAME, - ExistingPeriodicWorkPolicy.UPDATE, - request, - ) + // Already due, or overdue. Let it run: moving it now would push + // today's reminder to tomorrow, and being a little late is the + // failure everybody forgives. + existing.nextScheduleTimeMillis <= now -> Unit + + // Near enough. Rewriting the schedule on every process start churns + // WorkManager for a difference nobody could perceive. + abs(existing.nextScheduleTimeMillis - target) <= TOLERANCE_MILLIS -> Unit + + else -> manager.enqueueUniquePeriodicWork( + WORK_NAME, + ExistingPeriodicWorkPolicy.UPDATE, + daily().setNextScheduleTimeOverride(target).build(), + ) + } } - fun cancel() { + override fun cancel() { WorkManager.getInstance(context).cancelUniqueWork(WORK_NAME) } + private fun daily() = PeriodicWorkRequestBuilder(1, TimeUnit.DAYS) + .setConstraints( + // No network, no charging, no idle. A reminder that waits for + // Wi-Fi is a reminder that arrives on Tuesday. + Constraints.Builder().build(), + ) + .addTag(TAG) + companion object { const val WORK_NAME = "period-daily-reminder" const val TAG = "reminder" + /** + * How far the next run may sit from the target before it is moved. + * + * Five minutes: below the resolution anybody perceives in a reminder, + * and above the drift a periodic request accumulates from being + * re-anchored at each completion. A daylight-saving change moves the + * wall-clock target by an hour, which is well past this and correctly + * re-aims. + */ + internal const val TOLERANCE_MILLIS = 5 * 60 * 1000L + /** * Time from now until the next [target] o'clock. * diff --git a/core/notifications/src/test/kotlin/dev/privacyllc/period/core/notifications/ReminderSchedulerTest.kt b/core/notifications/src/test/kotlin/dev/privacyllc/period/core/notifications/ReminderSchedulerTest.kt index bec2814..b76ebe8 100644 --- a/core/notifications/src/test/kotlin/dev/privacyllc/period/core/notifications/ReminderSchedulerTest.kt +++ b/core/notifications/src/test/kotlin/dev/privacyllc/period/core/notifications/ReminderSchedulerTest.kt @@ -21,30 +21,30 @@ class ReminderSchedulerTest { @Test fun `a time later today waits until today`() { val now = LocalDateTime.of(2026, 8, 18, 8, 0) - val d = ReminderScheduler.delayUntil(LocalTime.of(10, 0), zone, now) + val d = WorkManagerReminderScheduler.delayUntil(LocalTime.of(10, 0), zone, now) assertEquals(120, d.toMinutes()) - assertEquals(LocalDate.of(2026, 8, 18), ReminderScheduler.nextRunDate(LocalTime.of(10, 0), now)) + assertEquals(LocalDate.of(2026, 8, 18), WorkManagerReminderScheduler.nextRunDate(LocalTime.of(10, 0), now)) } @Test fun `a time already past today waits until tomorrow`() { val now = LocalDateTime.of(2026, 8, 18, 11, 0) - val d = ReminderScheduler.delayUntil(LocalTime.of(10, 0), zone, now) + val d = WorkManagerReminderScheduler.delayUntil(LocalTime.of(10, 0), zone, now) assertEquals(23 * 60, d.toMinutes()) - assertEquals(LocalDate.of(2026, 8, 19), ReminderScheduler.nextRunDate(LocalTime.of(10, 0), now)) + assertEquals(LocalDate.of(2026, 8, 19), WorkManagerReminderScheduler.nextRunDate(LocalTime.of(10, 0), now)) } @Test fun `the exact minute counts as past, not now`() { // Scheduling a zero delay for "right now" fires immediately and then // again in 24 hours, which is two reminders on the first day. val now = LocalDateTime.of(2026, 8, 18, 10, 0) - assertEquals(24 * 60, ReminderScheduler.delayUntil(LocalTime.of(10, 0), zone, now).toMinutes()) + assertEquals(24 * 60, WorkManagerReminderScheduler.delayUntil(LocalTime.of(10, 0), zone, now).toMinutes()) } @Test fun `the delay is never negative and never beyond a day`() { listOf(LocalTime.MIDNIGHT, LocalTime.of(6, 30), LocalTime.of(23, 59)).forEach { target -> (0..23).forEach { hour -> val now = LocalDateTime.of(2026, 8, 18, hour, 17) - val minutes = ReminderScheduler.delayUntil(target, zone, now).toMinutes() + val minutes = WorkManagerReminderScheduler.delayUntil(target, zone, now).toMinutes() assertTrue("$target at $hour gave $minutes", minutes in 0..(24 * 60)) } } diff --git a/core/notifications/src/test/kotlin/dev/privacyllc/period/core/notifications/WorkManagerReminderSchedulerTest.kt b/core/notifications/src/test/kotlin/dev/privacyllc/period/core/notifications/WorkManagerReminderSchedulerTest.kt new file mode 100644 index 0000000..5a03395 --- /dev/null +++ b/core/notifications/src/test/kotlin/dev/privacyllc/period/core/notifications/WorkManagerReminderSchedulerTest.kt @@ -0,0 +1,186 @@ +package dev.privacyllc.period.core.notifications + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import androidx.work.Configuration +import androidx.work.WorkInfo +import androidx.work.WorkManager +import androidx.work.testing.WorkManagerTestInitHelper +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import java.time.Clock +import java.time.Instant +import java.time.LocalTime +import java.time.ZoneId +import java.time.ZoneOffset + +/** + * Whether a reminder actually moves when the user moves it. + * + * `schedule()` had no test — only the pure arithmetic beneath it did — and the + * scheduling itself was wrong in two ways that a reading of the API does not + * reveal. `ExistingPeriodicWorkPolicy.UPDATE` carries the old request's + * `lastEnqueueTime` and `periodCount` forward, so a new initial delay is ignored + * once the work has run once, and applied against the *original* enqueue time + * before that. Changing Morning to Evening did nothing; re-scheduling at every + * process start could aim the next run into the past. + * + * These assert against WorkManager's own recorded next-run time, which is the + * only thing that decides when the user is actually woken. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class WorkManagerReminderSchedulerTest { + + private val zone: ZoneId = ZoneOffset.UTC + + /** 2026-08-20, 08:00 UTC — before both reminder times used below. */ + private val eightAm = Instant.parse("2026-08-20T08:00:00Z") + + /** + * One clock for both sides. + * + * WorkManager works out `nextScheduleTimeMillis` with its OWN clock, so a + * test that fixes only the scheduler's measures a delay against a real + * `System.currentTimeMillis()` and compares it to an instant from 2026. Both + * read this. + */ + private class TestClock(var millis: Long, private val zone: ZoneId) : Clock() { + override fun getZone(): ZoneId = zone + override fun withZone(z: ZoneId): Clock = TestClock(millis, z) + override fun instant(): Instant = Instant.ofEpochMilli(millis) + } + + private lateinit var context: Context + private lateinit var manager: WorkManager + private lateinit var clock: TestClock + + @Before fun setUp() { + context = ApplicationProvider.getApplicationContext() + clock = TestClock(eightAm.toEpochMilli(), zone) + WorkManagerTestInitHelper.initializeTestWorkManager( + context, + Configuration.Builder() + .setExecutor { it.run() } + .setClock { clock.millis } + .build(), + ) + manager = WorkManager.getInstance(context) + } + + /** Move both clocks, then hand back a scheduler reading the same one. */ + private fun schedulerAt(instant: Instant): WorkManagerReminderScheduler { + clock.millis = instant.toEpochMilli() + return WorkManagerReminderScheduler(context, clock) + } + + private fun scheduled(): WorkInfo? = + manager.getWorkInfosForUniqueWork(WorkManagerReminderScheduler.WORK_NAME).get() + .firstOrNull { !it.state.isFinished } + + private fun nextRun(): Long = scheduled()!!.nextScheduleTimeMillis + + @Test fun `scheduling with nothing there aims at the next occurrence`() = runBlocking { + schedulerAt(eightAm).schedule(LocalTime.of(10, 0), zone) + + assertNotNull("nothing was enqueued", scheduled()) + assertEquals(Instant.parse("2026-08-20T10:00:00Z").toEpochMilli(), nextRun()) + } + + @Test fun `scheduling the same time again leaves the run where it is`() = runBlocking { + schedulerAt(eightAm).schedule(LocalTime.of(10, 0), zone) + val before = nextRun() + + // What every process start does. It must not churn WorkManager, and it + // must not re-anchor the run against a new "now". + schedulerAt(eightAm.plusSeconds(60)).schedule(LocalTime.of(10, 0), zone) + + assertEquals(before, nextRun()) + } + + @Test fun `changing the reminder time moves the run`() = runBlocking { + schedulerAt(eightAm).schedule(LocalTime.of(10, 0), zone) + assertEquals(Instant.parse("2026-08-20T10:00:00Z").toEpochMilli(), nextRun()) + + // Morning to evening. This is the one that did nothing at all once the + // work had run once, and aimed at the wrong instant before that. + schedulerAt(eightAm).schedule(LocalTime.of(19, 0), zone) + + assertEquals(Instant.parse("2026-08-20T19:00:00Z").toEpochMilli(), nextRun()) + } + + @Test fun `changing the time an hour later still lands on the time asked for`() = runBlocking { + // The sharp version of the test above, and the one that actually catches + // the defect. UPDATE carries the ORIGINAL request's lastEnqueueTime + // forward, and a periodic request that has not yet run computes its next + // run as lastEnqueueTime + initialDelay. So a delay measured from "now" + // is applied to an enqueue time an hour in the past: asking for 19:00 at + // 09:00 produced 18:00, and every later period anchored off that. + // + // An explicit next-run override is the only way to say when, rather than + // how long from a moment WorkManager has its own opinion about. + schedulerAt(eightAm).schedule(LocalTime.of(10, 0), zone) + + schedulerAt(eightAm.plusSeconds(3600)).schedule(LocalTime.of(19, 0), zone) + + assertEquals(Instant.parse("2026-08-20T19:00:00Z").toEpochMilli(), nextRun()) + } + + @Test fun `a time already past today is aimed at tomorrow`() = runBlocking { + val evening = Instant.parse("2026-08-20T20:00:00Z") + schedulerAt(evening).schedule(LocalTime.of(10, 0), zone) + + assertEquals(Instant.parse("2026-08-21T10:00:00Z").toEpochMilli(), nextRun()) + } + + @Test fun `a run that is already due is left alone rather than pushed to tomorrow`() = runBlocking { + schedulerAt(eightAm).schedule(LocalTime.of(10, 0), zone) + val due = nextRun() + + // The app is opened at 10:05, five minutes after the reminder was due + // and before the system has run it. Re-aiming now would compute + // "tomorrow at ten" and silently skip today's reminder entirely. + schedulerAt(Instant.parse("2026-08-20T10:05:00Z")).schedule(LocalTime.of(10, 0), zone) + + assertEquals(due, nextRun()) + } + + @Test fun `a zone change re-aims at the same wall-clock time`() = runBlocking { + schedulerAt(eightAm).schedule(LocalTime.of(10, 0), zone) + assertEquals(Instant.parse("2026-08-20T10:00:00Z").toEpochMilli(), nextRun()) + + // Same instant, three hours east: it is already 11:00 there, so the next + // ten o'clock is tomorrow morning local — 07:00 UTC on the 21st, not + // 10:00 UTC today. Nothing recomputed this before, so somebody who flew + // east kept being reminded at the old wall-clock time indefinitely. + val eastern = ZoneOffset.ofHours(3) + schedulerAt(eightAm).schedule(LocalTime.of(10, 0), eastern) + + assertEquals(Instant.parse("2026-08-21T07:00:00Z").toEpochMilli(), nextRun()) + } + + @Test fun `cancelling removes the work`() = runBlocking { + val scheduler = schedulerAt(eightAm) + scheduler.schedule(LocalTime.of(10, 0), zone) + assertNotNull(scheduled()) + + scheduler.cancel() + + assertNull("the reminder was left scheduled", scheduled()) + } + + @Test fun `the request carries no constraints that could hold a reminder back`() = runBlocking { + schedulerAt(eightAm).schedule(LocalTime.of(10, 0), zone) + + // A reminder that waits for Wi-Fi is a reminder that arrives on Tuesday. + assertTrue(scheduled()!!.tags.contains(WorkManagerReminderScheduler.TAG)) + } +} diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 37e432b..967827e 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -43,7 +43,7 @@ way, with the batch that needs it. | `core/database` | Android library | Room entities, DAOs, converters, the schema export | `domain/cycle`, `domain/prediction` | | `core/datastore` | Android library | `UserPreferences` and the settings that are not health history | nothing in this project | | `core/data` | Android library | `CycleRepository`, entity⇄domain mapping, accuracy — the only module that touches a DAO | `core/database`, `domain/cycle`, `domain/prediction` | -| `core/notifications` | Android library | reminder copy, the privacy modes, WorkManager scheduling | `core/data`, `core/datastore`, `domain/*` | +| `core/notifications` | Android library | reminder copy **and the action vocabulary**, the privacy modes, WorkManager scheduling | `core/data`, `core/datastore`, `domain/*` | | `core/security` | Android library | the app lock's PIN verifier, its Keystore key and the lockout policy | **nothing in this project** | | `core/export` | **Kotlin JVM** | the export file format, and nothing else | `domain/cycle` — deliberately **not** `domain/prediction` | | `domain/cycle` | **Kotlin JVM** | `PeriodRecord`, `SpottingRecord`, `CycleRecord` and the rules over them | nothing | @@ -157,6 +157,43 @@ health app a crash mid-write is adjacent to losing what the user just entered, and a message they can read beats a process that vanished. The message carries the exception *type* and never a record's contents — §45. +### Scheduling a reminder is not the same as saying when + +`ExistingPeriodicWorkPolicy.UPDATE` carries the previous request's +`lastEnqueueTime` and `periodCount` forward, and a periodic request computes its +next run as `periodCount == 0 ? lastEnqueueTime + initialDelay : lastEnqueueTime ++ interval`. Both halves of that bit us: + +- **After the first run, a new initial delay is ignored entirely.** Changing the + reminder from Morning to Evening did nothing at all. +- **Before it, the delay is applied to the *original* enqueue time.** The + coordinator reschedules at every process start, so asking for 19:00 at 09:00 + when the work was enqueued at 08:00 produced 18:00 — and every later period + anchored off that. + +So the scheduler reads the existing work first: `KEEP` when nothing is +scheduled, leave an overdue run alone (moving it skips today's reminder +entirely), leave a run already within five minutes of the target alone, and +otherwise `UPDATE` with an explicit `setNextScheduleTimeOverride` — the only way +to say *when* rather than *how long from a moment WorkManager has its own opinion +about*. `CANCEL_AND_REENQUEUE` is wrong here for a subtler reason: this runs at +every process start, including the one WorkManager itself started to run the +worker, and cancelling the unique work there cancels the running worker. + +**There is no flex window, and the copy was changed rather than the code.** With +flex, the first run is placed at `start + interval − flex`, so a fifteen-minute +flex on a daily period aims the first reminder roughly 23h45m out and skips the +day the user set it. The screen used to promise delivery "a few minutes either +side"; a periodic request is deferred by the system, never brought forward, so it +now says *after this time, never before*. + +**Time-zone and clock changes need an app receiver; boot does not.** The delay is +computed once, from the zone in force then, so flying east left the reminder +arriving at the old wall-clock time forever. WorkManager's own +`RescheduleReceiver` declares `BOOT_COMPLETED` and nothing else — which is +exactly why `ClockChangeReceiver` exists for `TIMEZONE_CHANGED` and `TIME_SET`, +and exactly why it does not duplicate boot. + ### The check-in count belongs to a period, not to the app §30 says the app must stop asking "did your period start?" after a few diff --git a/docs/design/README.md b/docs/design/README.md index ca007e5..87d42ca 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -153,6 +153,18 @@ have replaced it. ## The lock screen is the one semi-public surface +### When the copy and the platform disagree, the copy moves + +The reminders screen promised delivery "a few minutes either side of this time". +It could not be made true. A periodic work request is *deferred* by the system, +never brought forward, and the flex window that would allow "before" also places +the first run nearly a full period out — so asking for it would have skipped the +reminder on the day the user set it, to keep a sentence. + +The sentence changed instead: *Android may deliver a reminder a few minutes after +this time, never before.* Slightly less comfortable and exactly what happens, +which is the trade this document asks for everywhere else. + ### A switch that is on should mean something is happening Reminders can be switched on here and still never arrive: the notification