fix(alarms): spend a dismissed skip on time, and ring a re-armed handled alarm

The upcoming check now also wakes once a noticed occurrence's fire grace has run out, so a notice cannot outlive a late fire, and once an armed skip's has, so the list stops saying "skipping next" about an occurrence already gone. onFire is back to checking only the armed skip: the resolver deliberately re-arms a handled occurrence that a clock moved backwards put back in the future, and that one must ring.

Fixes #16

app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt

@@ -11,7 +11,6 @@ import de.jeanlucmakiola.clockula.domain.alarm.AlarmRing
 import de.jeanlucmakiola.clockula.domain.alarm.AlarmRingState
 import de.jeanlucmakiola.clockula.domain.alarm.DismissOutcome
 import de.jeanlucmakiola.clockula.domain.alarm.FireOutcome
-import de.jeanlucmakiola.clockula.domain.alarm.FireSource
 import de.jeanlucmakiola.clockula.domain.alarm.RingSession
 import de.jeanlucmakiola.clockula.domain.alarm.ScheduledFire
 import de.jeanlucmakiola.clockula.domain.alarm.SnoozeOutcome
@@ -139,7 +138,7 @@ class AlarmEngine @Inject constructor(
         // a just-passed occurrence through the fire grace: one already due is
         // the fire's, not the notice's.
         val resolution = AlarmResolver.resolve(alarm, states.state(alarmId), now, zones.current())
-        if (resolution.source != FireSource.OCCURRENCE || resolution.nextFire != fireAt || fireAt <= now) {
+        if (resolution.nextFire != fireAt || !UpcomingNotices.isInWindow(fireAt, resolution.source, now)) {
             // Whatever posted the stale notice is put right on the way out.
             rescheduleLocked()
             return@withLock DismissOutcome.Stale
@@ -232,17 +231,19 @@ class AlarmEngine @Inject constructor(
      * instead of syncing them, for a caller with something more urgent to do
      * first (the fire path).
      */
-    private suspend fun rescheduleLocked(deferUpcomingTo: MutableList<UpcomingAlarm>? = null): ScheduledFire? {
+    private suspend fun rescheduleLocked(deferUpcomingTo: MutableList<UpcomingSync>? = null): ScheduledFire? {
         val now = wallClock.now()
         val zone = zones.current()
 
         var earliest: Pair<Alarm, AlarmResolution>? = null
         val resolved = mutableListOf<UpcomingAlarm>()
+        val pendingSkips = mutableListOf<Instant>()
         for (alarm in alarms.alarms().first()) {
             val state = states.state(alarm.id)
             val resolution = AlarmResolver.resolve(alarm, state, now, zone)
             persist(alarm, state, resolution)
             resolved += UpcomingAlarm(alarm, resolution.nextFire, resolution.source)
+            resolution.state.skippedOccurrence?.let { pendingSkips += it }
 
             val fireAt = resolution.nextFire ?: continue
             val best = earliest
@@ -262,7 +263,8 @@ class AlarmEngine @Inject constructor(
             ScheduledFire(winner.first.id, fireAt, winner.second.source, mode)
         }
         // After the real registration, never before it.
-        if (deferUpcomingTo == null) syncUpcomingLocked(resolved, now) else deferUpcomingTo += resolved
+        val upcoming = UpcomingSync(resolved, pendingSkips)
+        if (deferUpcomingTo == null) syncUpcomingLocked(upcoming, now) else deferUpcomingTo += upcoming
         return scheduled
     }
 
@@ -275,9 +277,9 @@ class AlarmEngine @Inject constructor(
      * Nothing here may cost an alarm, so a failure is swallowed rather than
      * thrown into whichever entry point is rescheduling.
      */
-    private fun syncUpcomingLocked(resolved: List<UpcomingAlarm>, now: Instant) {
+    private fun syncUpcomingLocked(upcoming: UpcomingSync, now: Instant) {
         runCatching {
-            val plan = UpcomingNotices.plan(resolved, now)
+            val plan = UpcomingNotices.plan(upcoming.resolved, now, upcoming.pendingSkips)
             notifier.showUpcoming(plan.notices)
             val next = plan.nextCheckAt
             if (next == null) scheduler.cancelUpcomingCheck() else scheduler.scheduleUpcomingCheck(next)
@@ -313,16 +315,15 @@ class AlarmEngine @Inject constructor(
         // delivered late (after a reboot, or by Doze) still rings. Rejecting it
         // here would only re-register the same past instant and fire again.
         val lateBy = if (isSnoozeFire) AlarmRing.AUTO_SILENCE_AFTER else AlarmRing.FIRE_GRACE
-        // An occurrence already accounted for: skipped in advance (#16) — armed or,
-        // once a reschedule spent the skip, recorded as handled — or a duplicate
-        // of one that already rang. A fire already on its way when that was
-        // written must not ring it. Equality only: a watermark ahead of a clock
-        // moved backwards must never silence an earlier occurrence.
-        val accountedFor = !isSnoozeFire &&
-            (scheduledFor == state.skippedOccurrence || scheduledFor == state.handledOccurrence)
-        // Deleted, switched off, accounted for, or a PendingIntent so stale the
-        // moment has gone: re-resolve the slot and stay quiet.
-        if (alarm == null || !alarm.enabled || accountedFor || now - scheduledFor > lateBy) {
+        // A skip armed on this very occurrence after its fire was already on its
+        // way: the user dismissed it in advance (#16), so it must not ring.
+        // Only the armed skip, not the handled-occurrence watermark: the
+        // resolver deliberately re-arms a handled occurrence that a clock moved
+        // backwards has put back in the future, and that one must ring.
+        val skipped = !isSnoozeFire && scheduledFor == state.skippedOccurrence
+        // Deleted, switched off, skipped, or a PendingIntent so stale the moment
+        // has gone: re-resolve the slot and stay quiet.
+        if (alarm == null || !alarm.enabled || skipped || now - scheduledFor > lateBy) {
             rescheduleLocked()
             return FireOutcome.Ignored
         }
@@ -348,7 +349,7 @@ class AlarmEngine @Inject constructor(
         notifier.clearSnoozed(alarmId)
         // The upcoming notices wait until the ring has started: their binder
         // calls must not stand between the fire and the sound.
-        val upcoming = mutableListOf<UpcomingAlarm>()
+        val upcoming = mutableListOf<UpcomingSync>()
         rescheduleLocked(deferUpcomingTo = upcoming)
 
         try {
@@ -356,7 +357,7 @@ class AlarmEngine @Inject constructor(
             ring.startRinging(alarmId)
             scheduler.scheduleAutoSilence(now + AlarmRing.AUTO_SILENCE_AFTER, alarmId)
         } finally {
-            syncUpcomingLocked(upcoming, now)
+            upcoming.forEach { syncUpcomingLocked(it, now) }
         }
         return FireOutcome.Ringing(alarmId)
     }
@@ -450,3 +451,10 @@ class AlarmEngine @Inject constructor(
         rescheduleLocked()
     }
 }
+
+/** What one reschedule pass hands the upcoming-alarm sync (#16). */
+private class UpcomingSync(
+    val resolved: List<UpcomingAlarm>,
+    /** Armed skips' watermarks, so a check can spend each one on time. */
+    val pendingSkips: List<Instant>,
+)

app/src/main/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNotices.kt

@@ -14,7 +14,7 @@ data class UpcomingNotice(
 
 /**
  * What the upcoming-alarm surface should show now, and when it next has to
- * look again. [nextCheckAt] is null when no alarm's window is still to open.
+ * look again. [nextCheckAt] is null when there is nothing left to look at.
  */
 data class UpcomingNoticePlan(
     val notices: List<UpcomingNotice>,
@@ -44,17 +44,37 @@ object UpcomingNotices {
     fun isInWindow(nextFire: Instant?, source: FireSource, now: Instant, lead: Duration = LEAD): Boolean =
         source == FireSource.OCCURRENCE && nextFire != null && nextFire > now && nextFire - now <= lead
 
-    /** Notices ordered by when they ring, then id; the next check is the earliest window still to open. */
-    fun plan(upcoming: List<UpcomingAlarm>, now: Instant, lead: Duration = LEAD): UpcomingNoticePlan {
+    /**
+     * Notices ordered by when they ring, then id. The next check is the
+     * earliest of three instants still to come:
+     *
+     * - a window opening, so its notice goes up;
+     * - a noticed occurrence's fire grace running out, so a notice whose fire
+     *   came late (Doze, no exact alarms) does not outlive its alarm;
+     * - an armed skip's ([pendingSkips]) fire grace running out, so a skip
+     *   dismissed in advance is spent on time and the list stops saying
+     *   "skipping next" about an occurrence that has gone by.
+     *
+     * The last two wait out the grace rather than land on the alarm's own
+     * instant, so they never race its fire.
+     */
+    fun plan(
+        upcoming: List<UpcomingAlarm>,
+        now: Instant,
+        pendingSkips: List<Instant> = emptyList(),
+        lead: Duration = LEAD,
+        fireGrace: Duration = AlarmRing.FIRE_GRACE,
+    ): UpcomingNoticePlan {
         val notices = upcoming
             .filter { isInWindow(it.nextFire, it.source, now, lead) }
             .map { UpcomingNotice(it.alarm, it.nextFire!!) }
             .sortedWith(compareBy({ it.fireAt }, { it.alarm.id }))
-        val nextCheckAt = upcoming
+        val openings = upcoming
             .filter { it.source == FireSource.OCCURRENCE }
             .mapNotNull { entry -> entry.nextFire?.let { it - lead } }
-            .filter { it > now }
-            .minOrNull()
+        val closings = notices.map { it.fireAt + fireGrace }
+        val skipsSpent = pendingSkips.map { it + fireGrace }
+        val nextCheckAt = (openings + closings + skipsSpent).filter { it > now }.minOrNull()
         return UpcomingNoticePlan(notices, nextCheckAt)
     }
 }

app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineUpcomingTest.kt

@@ -2,6 +2,7 @@ package de.jeanlucmakiola.clockula.alarm
 
 import com.google.common.truth.Truth.assertThat
 import de.jeanlucmakiola.clockula.domain.RepeatDays
+import de.jeanlucmakiola.clockula.domain.alarm.AlarmRing
 import de.jeanlucmakiola.clockula.domain.alarm.DismissOutcome
 import de.jeanlucmakiola.clockula.domain.alarm.FireOutcome
 import de.jeanlucmakiola.clockula.domain.alarm.UpcomingNotices
@@ -51,7 +52,8 @@ class AlarmEngineUpcomingTest {
         harness.engine.onUpcomingWindow()
 
         assertThat(harness.notifier.upcoming.map { it.alarm.id }).containsExactly(1L, 2L).inOrder()
-        assertThat(harness.scheduler.upcomingCheck).isNull()
+        // Next: the first notice's alarm, gone by.
+        assertThat(harness.scheduler.upcomingCheck).isEqualTo(sixThirty + AlarmRing.FIRE_GRACE)
     }
 
     @Test
@@ -69,7 +71,12 @@ class AlarmEngineUpcomingTest {
             assertThat(alarm.skipNextOccurrence).isTrue()
             assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirtyTomorrow)
             assertThat(harness.notifier.upcoming).isEmpty()
-            assertThat(harness.scheduler.upcomingCheck).isEqualTo(sixThirtyTomorrow - UpcomingNotices.LEAD)
+            // Back once the skipped occurrence has gone by, to spend the skip.
+            assertThat(harness.scheduler.upcomingCheck).isEqualTo(sixThirty + AlarmRing.FIRE_GRACE)
+            harness.wallClock.instant = sixThirty + AlarmRing.FIRE_GRACE
+            harness.engine.onUpcomingWindow()
+            assertThat(harness.alarms.find(1L)!!.skipNextOccurrence).isFalse()
+            assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirtyTomorrow)
         }
 
     @Test
@@ -210,21 +217,21 @@ class AlarmEngineUpcomingTest {
     }
 
     @Test
-    fun `a fire that lands after the dismissed skip was spent still does not ring`(@TempDir tempDir: Path) =
-        runTest {
-            val harness = alarmEngineHarness(tempDir)
-            harness.givenAlarm(id = 1L, hour = 6, minute = 30, repeatDays = RepeatDays.EVERY_DAY)
-            harness.engine.reschedule()
-            harness.engine.dismissUpcomingOccurrence(1L, sixThirty)
+    fun `a handled occurrence a clock moved backwards re-armed still rings`(@TempDir tempDir: Path) = runTest {
+        val harness = alarmEngineHarness(tempDir)
+        harness.givenAlarm(id = 1L, hour = 6, minute = 30, repeatDays = RepeatDays.EVERY_DAY)
+        harness.engine.reschedule()
+        harness.givenRinging(alarmId = 1L, at = sixThirty)
+        harness.engine.dismiss(1L)
 
-            // Another entry point reschedules first and spends the skip.
-            harness.wallClock.instant = sixThirty
-            harness.engine.reschedule()
-            val outcome = harness.engine.onFire(1L, sixThirty)
+        // The clock is set back under the dismissed alarm; the resolver re-arms it.
+        harness.wallClock.instant = sixThirty - 15.minutes
+        harness.engine.onSystemTimeOrZoneChanged()
+        assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirty)
 
-            assertThat(outcome).isEqualTo(FireOutcome.Ignored)
-            assertThat(harness.ring.events).isEmpty()
-        }
+        harness.wallClock.instant = sixThirty
+        assertThat(harness.engine.onFire(1L, sixThirty)).isEqualTo(FireOutcome.Ringing(1L))
+    }
 
     @Test
     fun `an old skip gone by but not yet spent does not swallow the new dismissal`(@TempDir tempDir: Path) =

app/src/test/java/de/jeanlucmakiola/clockula/domain/alarm/UpcomingNoticesTest.kt

@@ -4,6 +4,7 @@ import com.google.common.truth.Truth.assertThat
 import de.jeanlucmakiola.clockula.testing.offsetInstant
 import de.jeanlucmakiola.clockula.testing.upcomingAlarm
 import org.junit.jupiter.api.Test
+import kotlin.time.Duration.Companion.days
 import kotlin.time.Duration.Companion.hours
 import kotlin.time.Duration.Companion.milliseconds
 import kotlin.time.Duration.Companion.minutes
@@ -66,8 +67,6 @@ class UpcomingNoticesTest {
     fun `the next check is when the earliest window still to open opens`() {
         val plan = UpcomingNotices.plan(
             listOf(
-                // Already open: noticed now, needs no check.
-                upcomingAlarm(id = 1L, nextFire = now + 30.minutes),
                 upcomingAlarm(id = 2L, nextFire = now + 5.hours),
                 upcomingAlarm(id = 3L, nextFire = now + 3.hours),
                 // A snooze's own notification covers it; no window to wait for.
@@ -79,6 +78,34 @@ class UpcomingNoticesTest {
         assertThat(plan.nextCheckAt).isEqualTo(now + 1.hours)
     }
 
+    @Test
+    fun `a noticed occurrence is looked at again once its fire grace has run out`() {
+        val fireAt = now + 30.minutes
+        val plan = UpcomingNotices.plan(
+            listOf(
+                upcomingAlarm(id = 1L, nextFire = fireAt),
+                upcomingAlarm(id = 2L, nextFire = now + 5.hours),
+            ),
+            now,
+        )
+
+        // So a notice whose fire came late does not outlive its alarm.
+        assertThat(plan.nextCheckAt).isEqualTo(fireAt + AlarmRing.FIRE_GRACE)
+    }
+
+    @Test
+    fun `an armed skip is looked at again once its occurrence has gone by`() {
+        val skipped = now + 40.minutes
+        val plan = UpcomingNotices.plan(
+            listOf(upcomingAlarm(id = 1L, nextFire = now + 1.days)),
+            now,
+            pendingSkips = listOf(skipped),
+        )
+
+        // So the skip is spent then, not at tomorrow's window.
+        assertThat(plan.nextCheckAt).isEqualTo(skipped + AlarmRing.FIRE_GRACE)
+    }
+
     @Test
     fun `nothing to come means no notice and no check`() {
         val plan = UpcomingNotices.plan(

docs/ARCHITECTURE.md

@@ -1238,7 +1238,12 @@ the alarm's row for whoever opens the app instead.
 - **Kept true in one place.** Every path that moves an alarm already ends in
   `rescheduleLocked`, so that is where the notices are synced and the third
   slot is armed for the earliest window still to open, *after* the real
-  registration. The sync is wrapped so a notification failure costs nothing,
+  registration. The check also wakes once a noticed occurrence's fire grace has
+  run out (so a notice cannot outlive a fire that came late) and once an armed
+  skip's has (so a skip dismissed in advance is spent on time, and the list
+  stops saying "skipping next" about an occurrence already gone). Both wait out
+  the grace rather than land on the alarm's own instant, so they never race its
+  fire. The sync is wrapped so a notification failure costs nothing,
   and on the fire path it is deferred until the ring has started, so its binder
   calls never stand between the fire and the sound. The check's receiver just
   calls `onUpcomingWindow()`, which is `reschedule()`.
@@ -1250,10 +1255,11 @@ the alarm's row for whoever opens the app instead.
   round its challenge. When it matches, it takes `dismissUpcoming`'s own
   branches: a repeating alarm skips that occurrence, a one-shot is switched off
   (or deleted, if transient). A fire broadcast already in flight when a skip
-  was armed is `Ignored` by `onFire`: a natural fire whose instant *equals*
-  the skip or the handled-occurrence watermark is already accounted for.
-  Equality, not order, so a watermark left ahead of a clock moved backwards
-  never silences an earlier occurrence. A repeating alarm's resolution is
+  was armed is `Ignored` by `onFire`, which checks the armed skip watermark
+  against the instant it carries. Only the skip: the handled-occurrence
+  watermark is left to the resolver, which deliberately re-arms a handled
+  occurrence that a clock moved backwards has put back in the future. A
+  repeating alarm's resolution is
   persisted before the skip is armed, so an old skip that has gone by but was
   never spent cannot absorb the new one.
 - **No dismiss challenge before the ring**, as in Google Clock. The challenge