fix(alarms): let the backstop always close the ring it was armed for
Fourth round of review fixes for the silence-after setting. onAutoSilence no longer re-arms a ring it measures as inside the window. After a clock moved back, that check could push the backstop out again and stretch the ring well past its window. Every writer of the window and every clock change already re-arms the backstop under the engine lock, so a backstop that fires has had its window, and it closes the ring as in v1. A one-shot alarm the resolver switches off as missed also clears a pending skip flag, as the resolver's other one-shot disable does. SNOOZE_GRACE is its own ten-minute constant again, not tied to the setting's default. Refs #17
This commit is contained in:
1 parent
4cb4b6b6eb
commit
0b7af0d57b
5 files changed
+16
-36
No files matched your search
@@ -31,7 +31,6 @@ 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
|
||||
@@ -83,15 +82,12 @@ class AlarmEngine @Inject constructor(
|
||||
|
||||
suspend fun onAutoSilence(alarmId: Long): Unit = lock.withLock {
|
||||
val state = states.state(alarmId)
|
||||
val since = state.ringingSince ?: return@withLock
|
||||
if (state.ringingSince == null) return@withLock
|
||||
|
||||
// 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. 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
|
||||
// The backstop is always armed for the window in force: every writer of
|
||||
// "Silence after" and every clock change re-arms it under this lock. So
|
||||
// when it fires, the ring has had its window, whatever the wall clock
|
||||
// now says about its age.
|
||||
|
||||
// Missed, not snoozed: the ring stops, the cycle closes, the next
|
||||
// occurrence is scheduled. There is no missed-alarm notification in v1.
|
||||
@@ -377,7 +373,7 @@ class AlarmEngine @Inject constructor(
|
||||
* run out ⇒ false, and the caller closes or drops it. Measured from when the
|
||||
* ring started, so neither a reboot nor a setting change buys it a fresh
|
||||
* window — or from now, if the clock has since moved back behind that
|
||||
* start, so a clock change cannot stretch a ring past one window either.
|
||||
* start, so a clock moved back adds at most the time already rung.
|
||||
*/
|
||||
private suspend fun rearmIfInsideWindowLocked(alarmId: Long, since: Instant): Boolean {
|
||||
val now = wallClock.now()
|
||||
|
||||
@@ -107,6 +107,7 @@ object AlarmResolver {
|
||||
nextFire = null,
|
||||
source = FireSource.NONE,
|
||||
state = working.copy(skippedOccurrence = null),
|
||||
clearSkipFlag = alarm.skipNextOccurrence,
|
||||
disableAlarm = true,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
package de.jeanlucmakiola.clockula.domain.alarm
|
||||
|
||||
import de.jeanlucmakiola.clockula.domain.ring.SilenceAfter
|
||||
import kotlin.time.Duration
|
||||
import kotlin.time.Duration.Companion.minutes
|
||||
|
||||
@@ -24,7 +23,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 = SilenceAfter.DEFAULT_MINUTES.minutes
|
||||
val SNOOZE_GRACE: Duration = 10.minutes
|
||||
|
||||
/** How late a snooze may come due and still ring, given the ring window. */
|
||||
fun snoozeGrace(silenceAfter: Duration): Duration = maxOf(silenceAfter, SNOOZE_GRACE)
|
||||
|
||||
@@ -912,23 +912,6 @@ class AlarmEngineTest {
|
||||
assertThat(entry.source).isEqualTo(FireSource.RESUMED_RING)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a backstop armed for a shorter window moves rather than firing early`(@TempDir dir: Path) = runTest {
|
||||
givenAlarm(repeatDays = RepeatDays.EVERY_DAY)
|
||||
wallClock.instant = sevenToday
|
||||
val settings = settings(dir)
|
||||
val engine = engineOver(settings)
|
||||
engine.onFire(alarmId = 1L, scheduledFor = sevenToday)
|
||||
settings.setAlarmSilenceAfterMinutes(25)
|
||||
wallClock.instant = sevenToday + 10.minutes
|
||||
|
||||
engine.onAutoSilence(1L)
|
||||
|
||||
assertThat(ring.ringing.value).isEqualTo(1L)
|
||||
assertThat(states.state(1L).ringingSince).isEqualTo(sevenToday)
|
||||
assertThat(scheduler.autoSilence).isEqualTo(Registration(sevenToday + 25.minutes, 1L))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `raising silence-after mid-ring moves the backstop at once`(@TempDir dir: Path) = runTest {
|
||||
givenAlarm(repeatDays = RepeatDays.EVERY_DAY)
|
||||
|
||||
@@ -1183,13 +1183,14 @@ 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.
|
||||
`closeCycleLocked`; inside it, the backstop moves to the new end. 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 one moved forward past the window closes it. When the
|
||||
backstop fires it always closes the ring, since every writer of the window and
|
||||
every clock change has already re-armed it for the window in force. A clock
|
||||
moved back mid-ring can still add up to the time already rung, which v1's fixed
|
||||
backstop did not bound at all.
|
||||
|
||||
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
|
||||
|
||||
Reference in new issue
Block a user