fix(alarms): never announce an alarm that has since stopped ringing

A notice is now re-checked against the engine before a screen shows it,
so an alarm switched off or deleted by any path - the ring screen, a
notification, an intent, a restore - is never announced as set. A
retract takes the publish lock, so a publish already past its read
cannot put the notice back.

Settling is per alarm: switching one alarm off in the list no longer
cancels the repeat days still settling on another. The identical-words
dedupe is gone, since it could swallow a real confirmation.

The editor's writes that move a fire time, with their reschedule and
announcement, run on the application scope. Pick, OK, back could
otherwise cancel the reschedule half-way and leave a stored time the
system had not been given. upcomingFor() reads alarm and state under
the engine lock, so a fire cannot land between the two reads.

Refs #19

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

@@ -197,13 +197,18 @@ class AlarmEngine @Inject constructor(
 
     /**
      * One alarm's [upcoming] entry, resolved against [now]. Read-only like
-     * [upcoming], and for the same reason it takes no lock: it writes nothing.
-     * Null for an id no alarm has. A caller that measures a countdown passes
-     * the `now` it measures from, so both ends of it are the same instant.
+     * [upcoming]; null for an id no alarm has. A caller that measures a
+     * countdown passes the `now` it measures from, so both ends of it are the
+     * same instant.
+     *
+     * It writes nothing, but it does take [lock]: the alarm and its ring state
+     * are two reads, and a fire landing between them would pair an alarm with
+     * a state that is not its own. `upcoming()` gets the same consistency from
+     * combining the two flows.
      */
-    suspend fun upcomingFor(alarmId: Long, now: Instant = wallClock.now()): UpcomingAlarm? {
-        val alarm = alarms.find(alarmId) ?: return null
-        return preview(alarm, states.state(alarmId), now, zones.current())
+    suspend fun upcomingFor(alarmId: Long, now: Instant = wallClock.now()): UpcomingAlarm? = lock.withLock {
+        val alarm = alarms.find(alarmId) ?: return@withLock null
+        preview(alarm, states.state(alarmId), now, zones.current())
     }
 
     /**

app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModel.kt

@@ -146,7 +146,9 @@ class AlarmEditorViewModel @Inject internal constructor(
     }
 
     fun onSkipNextChanged(skip: Boolean) {
-        viewModelScope.launch {
+        // On the application scope, like every write that moves a fire time:
+        // see [edit].
+        appScope.launch {
             alarms.setSkipNextOccurrence(alarmId, skip)
             engine.reschedule()
             // Skipping moves the next ring a day or more out, which is exactly
@@ -265,9 +267,13 @@ class AlarmEditorViewModel @Inject internal constructor(
     /**
      * Every editor write. [reschedule] is on only for a change that can move a
      * fire time — so fiddling with sound settings costs no AlarmManager traffic
-     * (M5 D12). [announces] is on for the same changes, after [settle] — on
-     * the announcer's application scope, so leaving the editor while the
-     * repeat days settle still has them said, for the list to show (#19).
+     * (M5 D12). [announces] is on for the same changes, after [settle] (#19).
+     *
+     * A change that moves a fire time runs on the application scope. Pick →
+     * OK → back is the usual way to set an alarm, and back clears this
+     * ViewModel: on its own scope the reschedule — and the "Alarm set for …"
+     * the list is meant to pick up — could be cancelled half-way, leaving a
+     * stored time the system had not been given yet.
      */
     private fun edit(
         reschedule: Boolean = false,
@@ -275,7 +281,7 @@ class AlarmEditorViewModel @Inject internal constructor(
         settle: Duration = Duration.ZERO,
         transform: (Alarm) -> Alarm,
     ) {
-        viewModelScope.launch {
+        (if (reschedule) appScope else viewModelScope).launch {
             alarms.edit(alarmId, transform)
             if (reschedule) engine.reschedule()
             if (announces) announcer.announce(alarmId, settle)

app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmSetSnackbar.kt

@@ -74,17 +74,23 @@ internal class AlarmSetAnnouncer @Inject constructor(
      */
     private val lock = Mutex()
 
-    /** The request still settling, and the alarm it is about. Guarded by `this`. */
-    private var pending: Job? = null
-    private var pendingFor: Long? = null
+    /**
+     * Requests still settling, one per alarm: a request about one alarm must
+     * not cancel another's — switching B off in the list would otherwise
+     * swallow the repeat days just set on A. Guarded by `this`.
+     */
+    private val pending = mutableMapOf<Long, Job>()
 
     /**
      * The notice to show, or null. Past [FRESH_FOR] it is dropped unshown: a
      * countdown that surfaced when the user came back to the app a while later
-     * would be wrong, and about something they did long ago.
+     * would be wrong, and about something they did long ago. It is checked
+     * against the engine again on the way out, so an alarm switched off or
+     * deleted by any path — the ring screen, a notification, an intent, a
+     * restore — is never announced as set.
      */
     val notice: Flow<AlarmSetNotice?> = current.map { notice ->
-        notice?.takeIf { elapsed.elapsedRealtime() - it.announcedAt < FRESH_FOR }
+        notice?.takeIf { isFresh(it) && stillRings(it) }
     }
 
     /**
@@ -96,25 +102,26 @@ internal class AlarmSetAnnouncer @Inject constructor(
      */
     fun announce(alarmId: Long, settle: Duration = Duration.ZERO) {
         synchronized(this) {
-            pending?.cancel()
-            pendingFor = alarmId
-            pending = appScope.launch {
+            pending.remove(alarmId)?.cancel()
+            val job = appScope.launch {
                 delay(settle)
                 publish(alarmId)
             }
+            pending[alarmId] = job
+            job.invokeOnCompletion { synchronized(this) { pending.remove(alarmId, job) } }
         }
     }
 
-    /** Takes back anything said or about to be said about [alarmId]: it was deleted or dismissed. */
-    fun retract(alarmId: Long) {
-        synchronized(this) {
-            if (pendingFor == alarmId) {
-                pending?.cancel()
-                pending = null
-                pendingFor = null
-            }
+    /**
+     * Takes back anything said or about to be said about [alarmId]: it was
+     * deleted or dismissed. Under the same lock as [publish], so a publish
+     * already past its read cannot put the notice back afterwards.
+     */
+    suspend fun retract(alarmId: Long) {
+        synchronized(this) { pending.remove(alarmId)?.cancel() }
+        lock.withLock {
+            current.update { notice -> notice?.takeUnless { it.alarmId == alarmId } }
         }
-        current.update { notice -> notice?.takeUnless { it.alarmId == alarmId } }
     }
 
     /** [notice] was shown until it timed out or was swiped away. */
@@ -123,15 +130,9 @@ internal class AlarmSetAnnouncer @Inject constructor(
     }
 
     private suspend fun publish(alarmId: Long) = lock.withLock {
-        // One `now` for both ends of the countdown, or a resolve that lands a
-        // hair past the occurrence would read as a day and an hour.
-        val now = wall.now()
-        val countdown = AlarmSetCountdown.of(engine.upcomingFor(alarmId, now), now)
+        val countdown = countdownOf(alarmId)
         current.update { standing ->
             when {
-                // The same words about the same alarm stay as they are, rather
-                // than taking the snackbar down and putting it straight back.
-                countdown != null && standing?.alarmId == alarmId && standing.countdown == countdown -> standing
                 countdown != null -> AlarmSetNotice(alarmId, countdown, elapsed.elapsedRealtime())
                 standing?.alarmId == alarmId -> null
                 else -> standing
@@ -139,6 +140,18 @@ internal class AlarmSetAnnouncer @Inject constructor(
         }
     }
 
+    private fun isFresh(notice: AlarmSetNotice): Boolean =
+        elapsed.elapsedRealtime() - notice.announcedAt < FRESH_FOR
+
+    private suspend fun stillRings(notice: AlarmSetNotice): Boolean = countdownOf(notice.alarmId) != null
+
+    private suspend fun countdownOf(alarmId: Long): NextFireLabel? {
+        // One `now` for both ends of the countdown, or a resolve that lands a
+        // hair past the occurrence would read as a day and an hour.
+        val now = wall.now()
+        return AlarmSetCountdown.of(engine.upcomingFor(alarmId, now), now)
+    }
+
     private companion object {
         /** Comfortably longer than a short snackbar, so a hand-over between screens fits. */
         val FRESH_FOR = 10.seconds
@@ -169,7 +182,9 @@ internal fun AlarmSetSnackbarEffect(
         lifecycle.repeatOnLifecycle(Lifecycle.State.RESUMED) {
             notice.collectLatest { current ->
                 if (current == null) return@collectLatest
-                host.showSnackbar(alarmSetMessage(resources, current.countdown) ?: return@collectLatest)
+                // A notice with nothing to say is used up all the same, so it
+                // cannot sit pending for ever.
+                alarmSetMessage(resources, current.countdown)?.let { host.showSnackbar(it) }
                 shown(current)
             }
         }

app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmEditorViewModelTest.kt

@@ -39,7 +39,6 @@ import org.junit.jupiter.api.io.TempDir
 import java.nio.file.Path
 import java.time.DayOfWeek
 import kotlin.time.Duration.Companion.hours
-import kotlin.time.Duration.Companion.seconds
 
 /**
  * The editor over the **real** alarm engine: every control writes at once
@@ -475,7 +474,7 @@ class AlarmEditorViewModelTest {
         }
 
     @Test
-    fun `the same countdown again leaves the standing notice alone`(@TempDir tempDir: Path) =
+    fun `an alarm switched off by another path is not announced as set`(@TempDir tempDir: Path) =
         runTest(main.dispatcher) {
             val harness = alarmEngineHarness(tempDir, now = fiveToday)
             harness.givenAlarmWith(hour = 7)
@@ -483,14 +482,11 @@ class AlarmEditorViewModelTest {
             advanceUntilIdle()
             viewModel.onTimePicked(6, 45)
             advanceUntilIdle()
-            val first = viewModel.alarmSetNotice.first()
 
-            // A snackbar taken down and put straight back would flicker.
-            elapsed.advance(1.seconds)
-            viewModel.onTimePicked(6, 45)
-            advanceUntilIdle()
+            // The ring screen, a notification or an intent: none of them retracts.
+            harness.alarms.setEnabled(1L, enabled = false)
 
-            assertThat(viewModel.alarmSetNotice.first()).isSameInstanceAs(first)
+            assertThat(viewModel.alarmSetNotice.first()).isNull()
         }
 
     // --- label ---

app/src/test/java/de/jeanlucmakiola/clockula/ui/alarms/AlarmsViewModelTest.kt

@@ -26,6 +26,7 @@ import org.junit.jupiter.api.extension.RegisterExtension
 import org.junit.jupiter.api.io.TempDir
 import java.nio.file.Path
 import kotlin.time.Duration.Companion.minutes
+import kotlin.time.Duration.Companion.seconds
 
 /**
  * The list over the **real** alarm engine: what the rows say, that the readout
@@ -48,13 +49,16 @@ class AlarmsViewModelTest {
      * `collectAsStateWithLifecycle`; without one there is no subscriber and
      * nothing upstream would run.
      */
-    private fun TestScope.viewModel(harness: AlarmEngineHarness) = AlarmsViewModel(
+    private fun TestScope.viewModel(
+        harness: AlarmEngineHarness,
+        announcer: AlarmSetAnnouncer = AlarmSetAnnouncer(harness.engine, harness.wallClock, FakeElapsedRealtimeClock(), this),
+    ) = AlarmsViewModel(
         alarms = harness.alarms,
         engine = harness.engine,
         wall = harness.wallClock,
         zones = harness.zones,
         source = AlarmListSource(harness.engine, harness.wallClock, harness.ticker),
-        announcer = AlarmSetAnnouncer(harness.engine, harness.wallClock, FakeElapsedRealtimeClock(), this),
+        announcer = announcer,
     ).also { viewModel -> backgroundScope.launch { viewModel.state.collect {} } }
 
     private fun AlarmsUiState.rows(): List<AlarmRowState> = (this as AlarmsUiState.Loaded).rows
@@ -258,6 +262,25 @@ class AlarmsViewModelTest {
             assertThat(viewModel.alarmSetNotice.first()).isNull()
         }
 
+    @Test
+    fun `switching another alarm off keeps one still settling`(@TempDir tempDir: Path) =
+        runTest(main.dispatcher) {
+            val harness = alarmEngineHarness(tempDir)
+            harness.givenAlarm(id = 1L, hour = 7)
+            harness.givenAlarm(id = 2L, hour = 9)
+            val announcer = AlarmSetAnnouncer(harness.engine, harness.wallClock, FakeElapsedRealtimeClock(), this)
+            val viewModel = viewModel(harness, announcer)
+            advanceUntilIdle()
+
+            // The editor's repeat days on alarm 1, still settling...
+            announcer.announce(1L, settle = 1.seconds)
+            // ...when alarm 2 is switched off in the list.
+            viewModel.onToggleEnabled(2L, enabled = false)
+            advanceUntilIdle()
+
+            assertThat(viewModel.alarmSetNotice.first()?.alarmId).isEqualTo(1L)
+        }
+
     @Test
     fun `adding an alarm leaves the announcement to the editor`(@TempDir tempDir: Path) =
         runTest(main.dispatcher) {

docs/ARCHITECTURE.md

@@ -1126,13 +1126,15 @@ at once, but waits for repeat-day taps to settle for a second, so Mon–Fri is
 one snackbar rather than five. `AlarmSetAnnouncer` is an app-wide singleton
 holding the notice as *state* until a screen has shown it to the end: pick →
 OK → back disposes the editor's snackbar a moment after it appears, and the
-list underneath picks the same notice up. The settle runs on the application
-scope, so leaving the editor mid-settle still announces; a screen shows the
-notice only while resumed, so a cross-fade never shows it twice; the editor
-shows only notices about its own alarm; deleting an alarm, or the `AlarmClock`
-chooser dismissing it, takes its notice back. A notice older than ten seconds
-on the monotonic clock is dropped unshown rather than shown with a stale
-countdown.
+list underneath picks the same notice up. The editor's fire-time-moving
+writes, their reschedule and the per-alarm settle all run on the application
+scope, so leaving the editor at once neither loses the announcement nor leaves
+a stored time unscheduled. A screen shows the notice only while resumed, so a
+cross-fade never shows it twice; the editor shows only notices about its own
+alarm; a notice is re-checked against the engine before it is shown, so an
+alarm switched off or deleted by any path is never announced as set. A notice
+older than ten seconds on the monotonic clock is dropped unshown rather than
+shown with a stale countdown.
 
 `upcoming(refresh: Flow<Unit>)` takes its **re-read cadence as a parameter**.
 The resolver needs a `now`, and `upcoming` reads one inside a `combine` of the