From 4cb4b6b6eb8f4a60028a0f2cb1a351769b475f44 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Wed, 21 Oct 2026 19:20:25 +0200 Subject: [PATCH] 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 --- .../clockula/alarm/AlarmEngine.kt | 41 +++++++++-- .../clockula/domain/alarm/AlarmRing.kt | 3 +- .../clockula/ui/alarms/OverridePickers.kt | 32 --------- .../ui/settings/AlarmDefaultsScreen.kt | 2 - .../clockula/ui/settings/SettingsViewModel.kt | 3 +- .../ui/settings/SilenceAfterPicker.kt | 39 ++++++++++ .../ui/settings/TimerDefaultsScreen.kt | 2 - .../clockula/alarm/AlarmEngineTest.kt | 72 ++++++++++++++++--- docs/ARCHITECTURE.md | 18 +++-- 9 files changed, 151 insertions(+), 61 deletions(-) create mode 100644 app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/SilenceAfterPicker.kt diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt b/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt index ea5ce1d..f04f7ca 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt +++ b/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 diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/domain/alarm/AlarmRing.kt b/app/src/main/java/de/jeanlucmakiola/clockula/domain/alarm/AlarmRing.kt index 6d11d99..a062c0e 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/domain/alarm/AlarmRing.kt +++ b/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) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/OverridePickers.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/OverridePickers.kt index 6046147..4ba9867 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/alarms/OverridePickers.kt +++ b/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?, diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/AlarmDefaultsScreen.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/AlarmDefaultsScreen.kt index 4942a8e..0a56420 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/AlarmDefaultsScreen.kt +++ b/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 diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/SettingsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/SettingsViewModel.kt index 213ac9c..f8697fa 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/SettingsViewModel.kt +++ b/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) = diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/SilenceAfterPicker.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/SilenceAfterPicker.kt new file mode 100644 index 0000000..e926f70 --- /dev/null +++ b/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, + ) +} diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/TimerDefaultsScreen.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/TimerDefaultsScreen.kt index 90ffbb2..a1396ed 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/TimerDefaultsScreen.kt +++ b/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 diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineTest.kt index b8540c4..b469608 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/alarm/AlarmEngineTest.kt +++ b/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 diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4344b71..9beeda0 100644 --- a/docs/ARCHITECTURE.md +++ b/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