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