fix(alarms): keep the upcoming Dismiss away from the fire

A fire broadcast already in flight when a skip was armed on its occurrence is now ignored, and a Dismiss that lands once the occurrence is due is stale. The notices are synced after the ring starts on the fire path, and a swiped-away notice stays gone across a process restart. A switched-off alarm answers NotFound.

Fixes #16

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

@@ -128,11 +128,14 @@ class AlarmEngine @Inject constructor(
      * already one switch away in the editor, unchallenged.
      */
     suspend fun dismissUpcomingOccurrence(alarmId: Long, fireAt: Instant): DismissOutcome = lock.withLock {
-        val alarm = alarms.find(alarmId) ?: return@withLock DismissOutcome.NotFound
+        val alarm = alarms.find(alarmId)?.takeIf { it.enabled } ?: return@withLock DismissOutcome.NotFound
+        val now = wallClock.now()
         // Read-only: the resolver's state is discarded, and the reschedule below
-        // persists whatever it should.
-        val resolution = AlarmResolver.resolve(alarm, states.state(alarmId), wallClock.now(), zones.current())
-        if (resolution.source != FireSource.OCCURRENCE || resolution.nextFire != fireAt) {
+        // persists whatever it should. `fireAt > now` because the resolver keeps
+        // 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) {
             // Whatever posted the stale notice is put right on the way out.
             rescheduleLocked()
             return@withLock DismissOutcome.Stale
@@ -220,7 +223,12 @@ class AlarmEngine @Inject constructor(
                 )
         }
 
-    private suspend fun rescheduleLocked(): ScheduledFire? {
+    /**
+     * [deferUpcomingTo] collects what the upcoming notices would be synced from
+     * 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? {
         val now = wallClock.now()
         val zone = zones.current()
 
@@ -250,7 +258,7 @@ class AlarmEngine @Inject constructor(
             ScheduledFire(winner.first.id, fireAt, winner.second.source, mode)
         }
         // After the real registration, never before it.
-        syncUpcomingLocked(resolved, now)
+        if (deferUpcomingTo == null) syncUpcomingLocked(resolved, now) else deferUpcomingTo += resolved
         return scheduled
     }
 
@@ -260,8 +268,8 @@ class AlarmEngine @Inject constructor(
      * snooze, a fire, a boot, a clock or zone change — already ends in
      * [rescheduleLocked], so this is the one place they are kept true.
      *
-     * Nothing here may cost an alarm: [onFireLocked] reschedules *before* it
-     * starts the ring, so a failure on the way is swallowed rather than thrown.
+     * 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) {
         runCatching {
@@ -297,9 +305,12 @@ 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
-        // Deleted, switched off, or a PendingIntent so stale the moment has gone:
-        // re-resolve the slot and stay quiet.
-        if (alarm == null || !alarm.enabled || 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.
+        val skipped = !isSnoozeFire && state.skippedOccurrence == scheduledFor
+        // 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
         }
@@ -323,11 +334,15 @@ class AlarmEngine @Inject constructor(
         // it holds this alarm's *next* occurrence rather than nothing at all.
         states.save(opened)
         notifier.clearSnoozed(alarmId)
-        rescheduleLocked()
+        // 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>()
+        rescheduleLocked(deferUpcomingTo = upcoming)
 
         states.save(states.state(alarmId).copy(ringingSince = now))
         ring.startRinging(alarmId)
         scheduler.scheduleAutoSilence(now + AlarmRing.AUTO_SILENCE_AFTER, alarmId)
+        syncUpcomingLocked(upcoming, now)
         return FireOutcome.Ringing(alarmId)
     }
 

app/src/main/java/de/jeanlucmakiola/clockula/alarm/android/AndroidAlarmNotifier.kt

@@ -53,44 +53,60 @@ class AndroidAlarmNotifier @Inject constructor(
     }
 
     /**
-     * What this process last posted, per alarm. The engine syncs on every
-     * reschedule — each app start, each edit — and re-posting an unchanged
-     * notice would bring back one the user swiped away. Only the engine calls
-     * this, under its lock, so the map needs no lock of its own.
+     * The notices already posted for the occurrences still wanted, as
+     * `id@fireAt` keys. Persisted, because every process start reschedules: a
+     * notice the user swiped away must stay gone across a restart, and only a
+     * notice this code cancelled itself (the alarm left its window) may come
+     * back. The engine calls this under its lock, so it needs none of its own.
      */
-    private val postedUpcoming = mutableMapOf<Long, UpcomingNotice>()
+    private val record by lazy { context.getSharedPreferences(RECORD_FILE, Context.MODE_PRIVATE) }
+
+    /** What this process last posted, per alarm, so an unchanged notice is not re-sent. */
+    private val lastPosted = mutableMapOf<Long, UpcomingNotice>()
 
     override fun showUpcoming(notices: List<UpcomingNotice>) {
         runCatching {
-            val wanted = notices.associateBy { it.alarm.id }
-            // What is on screen, including what an earlier process posted and
-            // this one has no memory of.
+            val wantedIds = notices.map { UpcomingNotifications.notificationId(it.alarm.id) }.toSet()
+            // What is on screen, including what an earlier process posted.
             val onScreen = context.getSystemService(NotificationManager::class.java)
                 .activeNotifications
                 .filter { it.tag == UpcomingNotifications.TAG }
                 .map { it.id }
-            val wantedIds = wanted.keys.map(UpcomingNotifications::notificationId).toSet()
-            (onScreen + postedUpcoming.keys.map(UpcomingNotifications::notificationId))
-                .filterNot { it in wantedIds }
-                .forEach { notifications.cancel(UpcomingNotifications.TAG, it) }
-            postedUpcoming.keys.retainAll(wanted.keys)
+                .toSet()
+            onScreen.filterNot { it in wantedIds }.forEach { notifications.cancel(UpcomingNotifications.TAG, it) }
+            lastPosted.keys.retainAll(notices.map { it.alarm.id }.toSet())
 
-            // Denied, nothing is recorded, so a later grant posts on the next sync.
+            val wantedKeys = notices.map(::recordKey).toSet()
+            val posted = record.getStringSet(RECORD_KEY, emptySet()).orEmpty().intersect(wantedKeys).toMutableSet()
+
+            // Denied, nothing new is recorded, so a later grant posts on the next sync.
             if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU &&
                 ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) !=
                 PackageManager.PERMISSION_GRANTED
             ) {
+                record.edit().putStringSet(RECORD_KEY, posted).apply()
                 return@runCatching
             }
             for (notice in notices) {
-                if (postedUpcoming[notice.alarm.id] == notice) continue
-                notifications.notify(
-                    UpcomingNotifications.TAG,
-                    UpcomingNotifications.notificationId(notice.alarm.id),
-                    UpcomingNotifications.build(context, notice),
-                )
-                postedUpcoming[notice.alarm.id] = notice
+                val id = UpcomingNotifications.notificationId(notice.alarm.id)
+                val shown = id in onScreen
+                // Posted before and no longer on screen: the user swiped it away.
+                if (!shown && recordKey(notice) in posted) continue
+                // On screen and unchanged: nothing to send.
+                if (shown && lastPosted[notice.alarm.id] == notice) continue
+                notifications.notify(UpcomingNotifications.TAG, id, UpcomingNotifications.build(context, notice))
+                lastPosted[notice.alarm.id] = notice
+                posted += recordKey(notice)
             }
+            record.edit().putStringSet(RECORD_KEY, posted).apply()
         }
     }
+
+    private fun recordKey(notice: UpcomingNotice): String =
+        "${notice.alarm.id}@${notice.fireAt.toEpochMilliseconds()}"
+
+    private companion object {
+        const val RECORD_FILE = "alarm_upcoming_notices"
+        const val RECORD_KEY = "posted"
+    }
 }

app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/RingNotifications.kt

@@ -216,7 +216,8 @@ object RingNotifications {
     private fun formatTime(context: Context, hour: Int, minute: Int): String =
         format(context, LocalTime.of(hour, minute))
 
-    private fun formatInstant(context: Context, instant: Instant): String = format(
+    /** Also the upcoming notice's (#16): one formatter for every alarm notification. */
+    internal fun formatInstant(context: Context, instant: Instant): String = format(
         context,
         JavaInstant.ofEpochMilli(instant.toEpochMilliseconds()).atZone(ZoneId.systemDefault()).toLocalTime(),
     )

app/src/main/java/de/jeanlucmakiola/clockula/alarm/ring/UpcomingNotifications.kt

@@ -12,10 +12,6 @@ import de.jeanlucmakiola.clockula.R
 import de.jeanlucmakiola.clockula.alarm.AlarmIntents
 import de.jeanlucmakiola.clockula.alarm.receiver.AlarmActionReceiver
 import de.jeanlucmakiola.clockula.domain.alarm.UpcomingNotice
-import java.time.Instant as JavaInstant
-import java.time.ZoneId
-import java.time.format.DateTimeFormatter
-import java.time.format.FormatStyle
 
 /**
  * "Upcoming alarm" with a Dismiss, posted while an alarm is inside its window
@@ -46,7 +42,7 @@ object UpcomingNotifications {
 
     fun build(context: Context, notice: UpcomingNotice): Notification {
         val alarm = notice.alarm
-        val time = formatInstant(context, notice.fireAt)
+        val time = RingNotifications.formatInstant(context, notice.fireAt)
         val text = if (alarm.label.isBlank()) {
             time
         } else {
@@ -98,17 +94,6 @@ object UpcomingNotifications {
             PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE,
         )
 
-    /** The device's 12/24-hour choice, then the locale's own short form. */
-    private fun formatInstant(context: Context, instant: kotlin.time.Instant): String {
-        val time = JavaInstant.ofEpochMilli(instant.toEpochMilliseconds()).atZone(ZoneId.systemDefault()).toLocalTime()
-        val pattern = if (android.text.format.DateFormat.is24HourFormat(context)) {
-            DateTimeFormatter.ofPattern("HH:mm")
-        } else {
-            DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT)
-        }
-        return pattern.format(time)
-    }
-
     /** Matches `RingNotifications`: keeps a derived request code clear of the small ones. */
     private const val REQUEST_CODE_STRIDE = 100_000
 

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

@@ -12,6 +12,7 @@ import kotlinx.coroutines.test.runTest
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.io.TempDir
 import java.nio.file.Path
+import kotlin.time.Duration.Companion.minutes
 
 /**
  * The upcoming-alarm notification (#16). The harness starts at 05:00 Berlin, so
@@ -164,4 +165,46 @@ class AlarmEngineUpcomingTest {
         assertThat(harness.ring.events).containsExactly(RingEvent.Started(1L))
         assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirtyTomorrow)
     }
+
+    @Test
+    fun `a fire already on its way when the notice was dismissed 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)
+
+            // The broadcast for 06:30 was sent before the skip was armed.
+            harness.wallClock.instant = sixThirty
+            val outcome = harness.engine.onFire(1L, sixThirty)
+
+            assertThat(outcome).isEqualTo(FireOutcome.Ignored)
+            assertThat(harness.ring.events).isEmpty()
+            assertThat(harness.scheduler.next?.fireAt).isEqualTo(sixThirtyTomorrow)
+            // The skip is spent on the one occurrence, not on tomorrow's.
+            assertThat(harness.alarms.find(1L)!!.skipNextOccurrence).isFalse()
+        }
+
+    @Test
+    fun `a Dismiss once the occurrence is due belongs to the fire, not the notice`(@TempDir tempDir: Path) =
+        runTest {
+            val harness = alarmEngineHarness(tempDir)
+            harness.givenAlarm(id = 1L, hour = 6, minute = 30, repeatDays = RepeatDays.EVERY_DAY)
+            harness.engine.reschedule()
+
+            // Inside the fire grace, with the fire itself running late.
+            harness.wallClock.instant = sixThirty + 1.minutes
+            val outcome = harness.engine.dismissUpcomingOccurrence(1L, sixThirty)
+
+            assertThat(outcome).isEqualTo(DismissOutcome.Stale)
+            assertThat(harness.alarms.find(1L)!!.skipNextOccurrence).isFalse()
+        }
+
+    @Test
+    fun `a switched-off alarm has nothing to dismiss`(@TempDir tempDir: Path) = runTest {
+        val harness = alarmEngineHarness(tempDir)
+        harness.givenAlarm(id = 1L, hour = 6, minute = 30, enabled = false)
+
+        assertThat(harness.engine.dismissUpcomingOccurrence(1L, sixThirty)).isEqualTo(DismissOutcome.NotFound)
+    }
 }

docs/ARCHITECTURE.md

@@ -1237,22 +1237,28 @@ 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: `onFire` reschedules before it starts the
-  ring, and a notification failure must not cost the ring. The check's
-  receiver just calls `onUpcomingWindow()`, which is `reschedule()`.
+  registration. 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()`.
 - **Dismiss is `dismissUpcoming`, pinned to one occurrence.**
   `dismissUpcomingOccurrence(id, fireAt)` re-resolves under the lock and acts
-  only if the alarm's next fire is still that natural occurrence; otherwise it
-  is `DismissOutcome.Stale` and writes nothing. Without the pin a tap landing
-  as the alarm starts would close the ring and get 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).
+  only if the alarm's next fire is still that natural occurrence *and still in
+  the future*; otherwise it is `DismissOutcome.Stale` and dismisses nothing.
+  Without the pin a tap landing as the alarm starts would close the ring and get
+  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`, which checks the skip watermark against
+  the instant it carries.
 - **No dismiss challenge before the ring**, as in Google Clock. The challenge
   proves you are awake while an alarm sounds; before it, skipping is already one
   unchallenged switch away in the editor.
 - **Denied notifications** cost the notice and nothing else; the row's chip
-  still works. The notifier remembers what it posted in this process, so a
-  reschedule does not bring back a notice the user swiped away.
+  still works. The notifier keeps a small SharedPreferences record of the
+  `id@fireAt` notices it has posted, so a reschedule, even in a new process,
+  does not bring back a notice the user swiped away; one the app cancelled
+  itself (the alarm left its window) is dropped from the record and may return.
 
 **The `showIntent` was wrong until M9.** It is what the system opens when the
 user taps the status-bar alarm icon or the lockscreen's next-alarm line, and it