fix(alarms): keep clock changes and setting writes inside one window

Third round of review fixes for the silence-after setting.

A backstop that fires while the clock is still behind the ring's start
now closes the ring instead of re-arming in a loop until the wall clock
catches up. A clock change re-measures a ring in progress before it
reschedules: a clock moved back re-arms the backstop at most one window
from the moved clock, and one moved forward past the window closes the
ring.

The engine now owns the alarm setting's write. setAlarmSilenceAfter
stores it and re-measures the ring under one lock and without
cancellation, so a write can no longer land without the re-measure.

A snooze the resolver drops takes its "Snoozed until" notification with
it. SNOOZE_GRACE is derived from the default window, and the silence-after
picker moves to ui/settings, since it is not a per-alarm override.

Refs #17

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

@@ -21,6 +21,7 @@ import de.jeanlucmakiola.clockula.domain.resolveSettings
 import de.jeanlucmakiola.clockula.domain.time.WallClock
 import de.jeanlucmakiola.clockula.domain.time.ZoneProvider
 import de.jeanlucmakiola.clockula.system.RebootRepair
+import kotlinx.coroutines.NonCancellable
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.combine
 import kotlinx.coroutines.flow.distinctUntilChanged
@@ -29,6 +30,8 @@ import kotlinx.coroutines.flow.flowOf
 import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
+import kotlinx.coroutines.withContext
+import kotlin.time.Duration
 import kotlin.time.Duration.Companion.minutes
 import kotlin.time.Instant
 import javax.inject.Inject
@@ -84,8 +87,11 @@ class AlarmEngine @Inject constructor(
 
         // A backstop armed for a shorter window than the one now chosen ("Silence
         // after" was raised mid-ring): the ring still has time, so the backstop
-        // moves rather than ending it early.
-        if (rearmIfInsideWindowLocked(alarmId, since)) return@withLock
+        // moves rather than ending it early. A negative age is a clock moved back
+        // behind the ring's start; the backstop was then armed a window from that
+        // moved clock, so the ring has had its window and closes.
+        val age = wallClock.now() - since
+        if (age >= Duration.ZERO && rearmIfInsideWindowLocked(alarmId, since)) return@withLock
 
         // Missed, not snoozed: the ring stops, the cycle closes, the next
         // occurrence is scheduled. There is no missed-alarm notification in v1.
@@ -93,15 +99,25 @@ class AlarmEngine @Inject constructor(
     }
 
     /**
-     * "Silence after" changed. A ring in progress is measured against the new
+     * Writes the alarms' "Silence after" — the engine is its only writer outside
+     * a backup restore — and measures a ring in progress against the new
      * window at once: one already past it closes as missed, exactly as the
      * backstop would close it, and one still inside it has its backstop moved
      * to the new end. Measured from when the ring started, so changing the
      * setting back and forth cannot buy a ring a fresh window.
      */
-    suspend fun onSilenceAfterChanged(): Unit = lock.withLock {
-        val state = states.ringing() ?: return@withLock
-        val since = state.ringingSince ?: return@withLock
+    suspend fun setAlarmSilenceAfter(minutes: Int): Unit = withContext(NonCancellable) {
+        // Not cancellable: a write that lands without the re-measure would leave
+        // a ring in progress on the old window.
+        lock.withLock {
+            settings.setAlarmSilenceAfterMinutes(minutes)
+            remeasureRingLocked()
+        }
+    }
+
+    private suspend fun remeasureRingLocked() {
+        val state = states.ringing() ?: return
+        val since = state.ringingSince ?: return
         if (!rearmIfInsideWindowLocked(state.alarmId, since)) closeCycleLocked(state.alarmId, state)
     }
 
@@ -155,7 +171,15 @@ class AlarmEngine @Inject constructor(
         rescheduleLocked()
     }
 
-    suspend fun onSystemTimeOrZoneChanged(): ScheduledFire? = reschedule()
+    /**
+     * The backstop is a wall-clock registration, so a clock change moves it. A
+     * ring in progress is re-measured first: a clock moved forward past its
+     * window closes it, one moved back re-arms it a window from now at most.
+     */
+    suspend fun onSystemTimeOrZoneChanged(): ScheduledFire? = lock.withLock {
+        remeasureRingLocked()
+        rescheduleLocked()
+    }
 
     suspend fun onPackageReplaced(): ScheduledFire? = reschedule()
 
@@ -369,6 +393,9 @@ class AlarmEngine @Inject constructor(
      */
     private suspend fun persist(alarm: Alarm, before: AlarmRingState, resolution: AlarmResolution) {
         if (resolution.state != before) states.save(resolution.state)
+        // A snooze the resolver dropped (it never fired, or the alarm was
+        // switched off) takes its "Snoozed until" notification with it.
+        if (before.snoozedUntil != null && resolution.state.snoozedUntil == null) notifier.clearSnoozed(alarm.id)
         // The resolver closed a ring: its window ran out before the backstop
         // fired ("Silence after" was shortened mid-ring, or the clock jumped),
         // or the alarm was switched off while ringing. Storage now says it is

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

@@ -1,5 +1,6 @@
 package de.jeanlucmakiola.clockula.domain.alarm
 
+import de.jeanlucmakiola.clockula.domain.ring.SilenceAfter
 import kotlin.time.Duration
 import kotlin.time.Duration.Companion.minutes
 
@@ -23,7 +24,7 @@ object AlarmRing {
      * window or this, whichever is longer, so a short window never makes a
      * promised snooze easier to lose.
      */
-    val SNOOZE_GRACE: Duration = 10.minutes
+    val SNOOZE_GRACE: Duration = SilenceAfter.DEFAULT_MINUTES.minutes
 
     /** How late a snooze may come due and still ring, given the ring window. */
     fun snoozeGrace(silenceAfter: Duration): Duration = maxOf(silenceAfter, SNOOZE_GRACE)

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

@@ -5,7 +5,6 @@ import androidx.compose.ui.res.pluralStringResource
 import androidx.compose.ui.res.stringResource
 import de.jeanlucmakiola.clockula.R
 import de.jeanlucmakiola.clockula.domain.DismissChallenge
-import de.jeanlucmakiola.clockula.domain.ring.SilenceAfter
 import de.jeanlucmakiola.floret.components.OptionPicker
 
 /**
@@ -68,37 +67,6 @@ internal fun challengeText(challenge: DismissChallenge): String = stringResource
     },
 )
 
-/** [SilenceAfter.NEVER] is "Never" in words, never "0 minutes". */
-@Composable
-internal fun silenceAfterText(minutes: Int): String = if (minutes == SilenceAfter.NEVER) {
-    stringResource(R.string.silence_after_never)
-} else {
-    pluralStringResource(R.plurals.silence_after_minutes, minutes, minutes)
-}
-
-/**
- * "Silence after" is app-wide only (#17): no alarm or timer overrides it, so
- * this picker has no "App default" rung. A stored value no choice matches (a
- * backup can carry any minute up to the hard stop) is offered too, in its
- * place, so the picker always shows what is selected and can put it back.
- */
-@Composable
-internal fun SilenceAfterPicker(
-    selected: Int,
-    onSelect: (Int) -> Unit,
-    onDismiss: () -> Unit,
-) {
-    val finite = (SilenceAfter.CHOICES + selected).filter { it != SilenceAfter.NEVER }.distinct().sorted()
-    OptionPicker(
-        title = stringResource(R.string.settings_silence_after),
-        options = finite + SilenceAfter.NEVER,
-        selected = selected,
-        label = { silenceAfterText(it) },
-        onSelect = onSelect,
-        onDismiss = onDismiss,
-    )
-}
-
 @Composable
 internal fun VibratePicker(
     stored: Boolean?,

app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/AlarmDefaultsScreen.kt

@@ -12,13 +12,11 @@ import androidx.compose.ui.res.stringResource
 import androidx.compose.ui.unit.dp
 import de.jeanlucmakiola.clockula.R
 import de.jeanlucmakiola.clockula.ui.alarms.DismissChallengePicker
-import de.jeanlucmakiola.clockula.ui.alarms.SilenceAfterPicker
 import de.jeanlucmakiola.clockula.ui.alarms.SnoozeLengthPicker
 import de.jeanlucmakiola.clockula.ui.alarms.SnoozeLimitPicker
 import de.jeanlucmakiola.clockula.ui.alarms.VibratePicker
 import de.jeanlucmakiola.clockula.ui.alarms.VolumeRampPicker
 import de.jeanlucmakiola.clockula.ui.alarms.challengeText
-import de.jeanlucmakiola.clockula.ui.alarms.silenceAfterText
 import de.jeanlucmakiola.clockula.ui.alarms.snoozeLimitText
 import de.jeanlucmakiola.clockula.ui.alarms.snoozeMinutesText
 import de.jeanlucmakiola.clockula.ui.alarms.vibrateText

app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/SettingsViewModel.kt

@@ -146,8 +146,7 @@ class SettingsViewModel @Inject internal constructor(
 
     /** The engine re-measures a ring in progress against the new window at once. */
     fun onAlarmSilenceAfterChanged(minutes: Int) = viewModelScope.launch {
-        settings.setAlarmSilenceAfterMinutes(minutes)
-        alarmEngine.onSilenceAfterChanged()
+        alarmEngine.setAlarmSilenceAfter(minutes)
     }
 
     fun onTimerSilenceAfterChanged(minutes: Int) =

app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/SilenceAfterPicker.kt

@@ -0,0 +1,39 @@
+package de.jeanlucmakiola.clockula.ui.settings
+
+import androidx.compose.runtime.Composable
+import androidx.compose.ui.res.pluralStringResource
+import androidx.compose.ui.res.stringResource
+import de.jeanlucmakiola.clockula.R
+import de.jeanlucmakiola.clockula.domain.ring.SilenceAfter
+import de.jeanlucmakiola.floret.components.OptionPicker
+
+/** [SilenceAfter.NEVER] is "Never" in words, never "0 minutes". */
+@Composable
+internal fun silenceAfterText(minutes: Int): String = if (minutes == SilenceAfter.NEVER) {
+    stringResource(R.string.silence_after_never)
+} else {
+    pluralStringResource(R.plurals.silence_after_minutes, minutes, minutes)
+}
+
+/**
+ * "Silence after" is app-wide only (#17): no alarm or timer overrides it, so
+ * this picker has no "App default" rung. A stored value no choice matches (a
+ * backup can carry any minute up to the hard stop) is offered too, in its
+ * place, so the picker always shows what is selected and can put it back.
+ */
+@Composable
+internal fun SilenceAfterPicker(
+    selected: Int,
+    onSelect: (Int) -> Unit,
+    onDismiss: () -> Unit,
+) {
+    val finite = (SilenceAfter.CHOICES + selected).filter { it != SilenceAfter.NEVER }.distinct().sorted()
+    OptionPicker(
+        title = stringResource(R.string.settings_silence_after),
+        options = finite + SilenceAfter.NEVER,
+        selected = selected,
+        label = { silenceAfterText(it) },
+        onSelect = onSelect,
+        onDismiss = onDismiss,
+    )
+}

app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/TimerDefaultsScreen.kt

@@ -11,8 +11,6 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.res.stringResource
 import androidx.compose.ui.unit.dp
 import de.jeanlucmakiola.clockula.R
-import de.jeanlucmakiola.clockula.ui.alarms.SilenceAfterPicker
-import de.jeanlucmakiola.clockula.ui.alarms.silenceAfterText
 import de.jeanlucmakiola.floret.components.CollapsingScaffold
 import de.jeanlucmakiola.floret.components.GroupedRow
 import de.jeanlucmakiola.floret.components.Position

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

@@ -50,6 +50,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.hours
 import kotlin.time.Duration.Companion.minutes
 import kotlin.time.Duration.Companion.seconds
 import kotlin.time.Instant
@@ -937,8 +938,7 @@ class AlarmEngineTest {
         engine.onFire(alarmId = 1L, scheduledFor = sevenToday)
         wallClock.instant = sevenToday + 3.minutes
 
-        settings.setAlarmSilenceAfterMinutes(SilenceAfter.NEVER)
-        engine.onSilenceAfterChanged()
+        engine.setAlarmSilenceAfter(SilenceAfter.NEVER)
 
         assertThat(ring.ringing.value).isEqualTo(1L)
         assertThat(scheduler.autoSilence).isEqualTo(Registration(sevenToday + SilenceAfter.HARD_STOP, 1L))
@@ -953,8 +953,7 @@ class AlarmEngineTest {
         engine.onFire(alarmId = 1L, scheduledFor = sevenToday)
         wallClock.instant = sevenToday + 5.minutes
 
-        settings.setAlarmSilenceAfterMinutes(1)
-        engine.onSilenceAfterChanged()
+        engine.setAlarmSilenceAfter(1)
 
         assertThat(ring.ringing.value).isNull()
         assertThat(states.state(1L).ringingSince).isNull()
@@ -969,8 +968,7 @@ class AlarmEngineTest {
         val settings = settings(dir)
         val engine = engineOver(settings)
 
-        settings.setAlarmSilenceAfterMinutes(1)
-        engine.onSilenceAfterChanged()
+        engine.setAlarmSilenceAfter(1)
 
         assertThat(ring.events).isEmpty()
         assertThat(scheduler.autoSilence).isNull()
@@ -1030,12 +1028,70 @@ class AlarmEngineTest {
         engine.onFire(alarmId = 1L, scheduledFor = sevenToday)
         wallClock.instant = sevenToday - 30.minutes
 
-        settings.setAlarmSilenceAfterMinutes(5)
-        engine.onSilenceAfterChanged()
+        engine.setAlarmSilenceAfter(5)
 
         assertThat(scheduler.autoSilence).isEqualTo(Registration(sevenToday - 30.minutes + 5.minutes, 1L))
     }
 
+    @Test
+    fun `a backstop reached with the clock still behind the ring's start closes it`(@TempDir dir: Path) =
+        runTest {
+            givenAlarm(repeatDays = RepeatDays.EVERY_DAY)
+            wallClock.instant = sevenToday
+            val engine = engine(dir)
+            engine.onFire(alarmId = 1L, scheduledFor = sevenToday)
+            wallClock.instant = sevenToday - 20.minutes
+
+            engine.onAutoSilence(1L)
+
+            assertThat(ring.ringing.value).isNull()
+            assertThat(states.state(1L).ringingSince).isNull()
+        }
+
+    @Test
+    fun `a clock moved back mid-ring re-arms the backstop a window from now`(@TempDir dir: Path) = runTest {
+        givenAlarm(repeatDays = RepeatDays.EVERY_DAY)
+        wallClock.instant = sevenToday
+        val engine = engine(dir)
+        engine.onFire(alarmId = 1L, scheduledFor = sevenToday)
+        wallClock.instant = sevenToday - 1.hours
+
+        engine.onSystemTimeOrZoneChanged()
+
+        assertThat(ring.ringing.value).isEqualTo(1L)
+        assertThat(scheduler.autoSilence)
+            .isEqualTo(Registration(sevenToday - 1.hours + ClockDefaults().alarmSilenceAfter, 1L))
+    }
+
+    @Test
+    fun `a clock moved forward past the window closes the ring`(@TempDir dir: Path) = runTest {
+        givenAlarm(repeatDays = RepeatDays.EVERY_DAY)
+        wallClock.instant = sevenToday
+        val engine = engine(dir)
+        engine.onFire(alarmId = 1L, scheduledFor = sevenToday)
+        wallClock.instant = sevenToday + 1.hours
+
+        engine.onSystemTimeOrZoneChanged()
+
+        assertThat(ring.ringing.value).isNull()
+        assertThat(states.state(1L).ringingSince).isNull()
+        assertThat(scheduler.autoSilence).isNull()
+    }
+
+    @Test
+    fun `a snooze the resolver drops takes its notification with it`(@TempDir dir: Path) = runTest {
+        givenAlarm(repeatDays = RepeatDays.EVERY_DAY)
+        states.save(
+            ringState(snoozedUntil = sevenToday + 9.minutes, snoozeCount = 1, handledOccurrence = sevenToday),
+        )
+        wallClock.instant = sevenToday + 2.hours
+        val engine = engine(dir)
+
+        engine.reschedule()
+
+        assertThat(notifier.cleared).contains(1L)
+    }
+
     // --- upcoming() ---
 
     @Test

docs/ARCHITECTURE.md

@@ -1179,13 +1179,17 @@ go through `SilenceAfter.sanitise` on read, on write and from a backup: over 60
 is 60, so no value can make a ring outlast the hard stop, and a negative one is
 the default rather than the clamp's floor, which would be "Never".
 
-Changing the setting mid-ring applies at once. The settings screen calls
-`AlarmEngine.onSilenceAfterChanged()`, which measures the ring in progress
-against the new window from when it *started*: past it, the cycle closes as
-missed through `closeCycleLocked`; inside it, the backstop moves to the new
-end. As a second line, `onAutoSilence` re-arms rather than closes a ring that
-is still inside the current window, so a backstop armed for a shorter one can
-never end a ring early.
+Changing the setting mid-ring applies at once. The settings screen writes it
+through `AlarmEngine.setAlarmSilenceAfter()`, which stores it and, under the
+same lock and uncancellably, measures the ring in progress against the new
+window from when it *started*: past it, the cycle closes as missed through
+`closeCycleLocked`; inside it, the backstop moves to the new end. As a second
+line, `onAutoSilence` re-arms rather than closes a ring that is still inside
+the current window, so a backstop armed for a shorter one can never end a ring
+early. A clock change re-measures the ring the same way before rescheduling:
+the backstop is a wall-clock registration, so a clock moved back re-arms it at
+most one window from the moved clock (and the backstop then closes the ring,
+its age still negative), and one moved forward past the window closes it.
 
 Whenever the resolver closes a ring (a clock jumped forward, a ringing alarm
 switched off, a reboot outlasting the window), `persist` stops the ring service