Make dismiss_alarm safe to call twice for the same dismiss - #305
Open
ScottMorris wants to merge 3 commits into
Open
Make dismiss_alarm safe to call twice for the same dismiss#305ScottMorris wants to merge 3 commits into
ScottMorris wants to merge 3 commits into
Conversation
ScottMorris
added
rust
ScottMorris
changed the title
ScottMorris
marked this pull request as ready for review
August 14, 2026 04:44
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76f3838d57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ScottMorris
force-pushed
the
feat/255-p4c-dismiss-idempotency
branch
from
August 18, 2026 16:56
76f3838 to
088b5f3
Compare
Issue #255 made in-app dismiss on Android reach `AlarmCoordinator::dismiss_alarm` through two independent paths for one user action -- the direct TS `AlarmService.dismiss` command and the newly-uniform native round trip (`alarm-manager:dismiss-requested`, no shared `event_id` to dedup on). Because `dismiss_alarm` recomputed `next_trigger` relative to whatever was *currently* stored on the alarm, a second call used the first call's already-advanced trigger as its new reference, silently skipping a whole scheduled occurrence for a repeating alarm. `dismiss_alarm` now only recomputes/advances `next_trigger` when the alarm's stored trigger is due/overdue or unset (`dismiss_should_advance`, pulled out as a pure function alongside the existing `classify_scheduling_transition`) -- the case where the alarm genuinely just fired and hasn't been advanced past this occurrence yet. A second call sees a trigger already in the future and treats itself as an idempotent replay: no DB write, no revision bump, but it still re-emits `alarm:dismissed` so every dismiss source gets the same confirmation, since every known consumer (Ringing.tsx's `isClosingRef`-guarded close, wear-sync's dismiss mirror) already tolerates a duplicate. For a non-repeating alarm (no active days), `next_trigger` lands on `None` after its one occurrence is dismissed, same as before it ever fires -- so a double dismiss can't be distinguished from a fresh one by that signal alone. This stays safe regardless: recomputing from `None` reproduces `None` no matter how many times it runs, so double-dismissing a one-shot alarm never re-enables or re-schedules it, just costs a redundant (harmless) DB write. `snooze_alarm` doesn't share this structural problem -- its anchor is computed fresh by the caller each time rather than chained off the alarm's own stored state -- so it's left alone here; flagging it is out of scope for this fix. Adds coordinator-level tests (via `tauri::test::mock_app()` and a new `AlarmDatabase::new_in_memory()` test helper) proving: a normal single dismiss is unaffected, a double dismiss on a repeating alarm advances exactly one occurrence (not two), and a double dismiss on a one-shot alarm is safe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WVm6JTTpiSY8KSKJ3p5Qq
…ce and lock Code review caught two real problems with the previous commit's approach (next_trigger-in-the-future implies duplicate): - It broke the pre-existing "dismiss upcoming alarm" notification action (`AlarmManagerService.dismissNextOccurrence` -> `AlarmService.dismiss`), which fires up to ~10 minutes *before* the alarm rings, while `next_trigger` is still legitimately in the future. The old heuristic couldn't tell that apart from a duplicate replay of an already-processed dismiss, so it silently no-opped the early dismiss and let the alarm ring anyway -- contradicting the "dismissing an upcoming alarm skips this occurrence" comment right above it. - There was no lock around the read-decide-write sequence, so two genuinely concurrent calls for the same alarm could both read pre-dismiss state and both pass the guard before either committed -- reproducing the double-advance bug this exists to prevent. Replaces the heuristic with a time-based debounce: `dismiss_alarm` now tracks each alarm's last-dismissed wall-clock time in a `dismiss_debounce` map on `AlarmCoordinator` (`is_duplicate_dismiss`, a pure/testable predicate), and treats a second call for the same id within `DISMISS_DEBOUNCE_WINDOW_MS` (5s) as a replay rather than a new request. 5 seconds comfortably absorbs the async plumbing latency between the TS-invoked command and the native `alarm-manager:dismiss-requested` round trip Ringing.tsx's Dismiss button now also triggers, while being far shorter than any realistic gap between two genuinely separate dismissals (a day, for a repeating alarm's next occurrence) or between an early "dismiss upcoming" action and the alarm's actual due time. A `tokio::sync::Mutex` guards the whole critical section (not just the debounce map), held across the entire read-decide-write-emit sequence, closing the concurrency gap outright rather than relying on timing. It's coarse-grained across every alarm id rather than per-id -- dismiss isn't a hot path, so serializing unrelated alarms' dismisses against each other is an acceptable simplicity trade, mirroring the existing `ImportLock` precedent in `lib.rs`. New/updated tests prove: a normal single dismiss is unaffected; dismissing a still-upcoming (future next_trigger) alarm correctly skips that occurrence (the notification-action regression case); near-simultaneous *and* genuinely concurrent (via `tokio::join!`) duplicate calls don't double-advance; a dismissal outside the debounce window is never treated as a duplicate even with a stale debounce entry present; and double-dismissing a one-shot alarm stays safe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WVm6JTTpiSY8KSKJ3p5Qq
…clock Second code review round found the debounce approach itself was structurally sound but had four real problems: - The debounce marker was recorded *before* the fallible recompute/save/emit work, with no rollback on failure. A transient failure (e.g. a bad schedule computation) would still poison the debounce window, so a legitimate retry within it silently no-op'd and returned `Ok(())` without ever actually dismissing anything -- worse than before this fix, where a redundant retry could self-heal a failed attempt. - The debounce compared wall-clock `chrono::Utc::now()` millis via `saturating_sub`, which goes *negative* (not zero) under a backward clock jump (NTP correction, manual clock change), making every dismiss look like a duplicate until wall-clock time caught back up. - One mutex guarded the whole map for every alarm id, so dismissing two unrelated alarms at nearly the same moment serialized against each other for no reason. - The map had no eviction, unlike `EventDedup` (the codebase's own prior art for this class of problem), so it grew by one entry for every alarm id ever dismissed for the life of the process. Addressed all four without another redesign: - The debounce marker (`last_dismissed_at`) is now written only immediately before the final `Ok(())`, after every fallible step has already succeeded. Any early return via `?` leaves it untouched, so a retry after a failure gets a fresh attempt rather than being swallowed as a duplicate. - Switched from `chrono::Utc::now()` to `std::time::Instant`, a monotonic clock immune to wall-clock adjustments by construction -- this is a purely internal, in-memory mechanism with no need to relate to wall-clock time at all, so this sidesteps the whole class of clock-jump bugs rather than special-casing them. - Replaced the single coarse mutex with a lazily-created per-alarm-id `tokio::sync::Mutex<Option<Instant>>` (`dismiss_lock_for`), holding the *outer* map lock only for the brief synchronous get-or-create, never across an `.await`. The per-id lock is held for that alarm's entire `dismiss_alarm` call, so unrelated alarms never block each other, while a genuinely concurrent second call for the *same* id still can't slip past the duplicate check before the first resolves. - `dismiss_lock_for` opportunistically sweeps other idle (not currently locked -- checked via a non-blocking `try_lock`) and expired entries out of the map on every call, bounding it to alarms with recent or in-flight dismiss activity instead of growing forever. Also, per review: extracted `AlarmInput::from_record` (models.rs) so `toggle_alarm` and `dismiss_alarm` share one field-by-field copy from `AlarmRecord` instead of two, so a future field addition can't be updated in one and silently missed in the other. Documented the debounce mechanism as a deliberate, disclosed short-term tradeoff rather than the final design -- the more correct fix is threading a shared `event_id` through every dismiss delivery path (today only the native listeners in lib.rs carry one) and reusing `EventDedup` for exact-match dedup instead of a time window. That requires changes to `Ringing.tsx` and `AlarmManagerPlugin.kt` already implemented in PR #301, so it's out of scope here; filed as a follow-up: #304 (cross-referencing #303, a related but distinct EventDedup gap). New/updated tests: the per-id concurrent-dismiss test now exercises the real per-id lock; a new test proves a failed dismiss doesn't poison the debounce window for a retry; pure `is_duplicate_dismiss` tests now use `Instant` arithmetic (a dedicated backward-wall-clock-jump test is no longer applicable since `Instant` is monotonic by construction -- documented as such, with a narrower test for the one adjacent edge case `saturating_duration_since` still guards defensively); and a new test exercises `dismiss_lock_for`'s sweep directly (evicts idle+expired, keeps recent and busy entries). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WVm6JTTpiSY8KSKJ3p5Qq
ScottMorris
force-pushed
the
feat/255-p4c-dismiss-idempotency
branch
from
August 18, 2026 17:08
088b5f3 to
7439f95
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A follow-up fix, found and fixed during #301's implementation, not a pre-planned phase of #255. Making in-app dismiss publish onto the native bus uniformly (#301) means it now reaches
AlarmCoordinator::dismiss_alarmtwice for the same user action: once via the direct TSAlarmService.dismisscommand, and once via the nativealarm-manager:dismiss-requestedround trip — with no sharedevent_idbetween the two paths to dedup on.dismiss_alarmwas not idempotent: it recomputednext_triggerrelative to whatever was currently stored, so the second call would silently advance a repeating alarm's next occurrence a second time, skipping a whole day.What's here
next_trigger-shape heuristic — an earlier attempt at this fix used "isnext_triggeralready in the future" as the duplicate signal, but that's indistinguishable from a legitimate case: the "dismiss upcoming alarm" notification action fires ~10 minutes before the alarm rings, whilenext_triggeris still in the future. That heuristic silently broke early dismiss. Time elapsed since the alarm's last successful dismissal is the signal that actually distinguishes "duplicate" from "new."std::time::Instant), not wall-clock, for the debounce comparison — immune to NTP/manual clock adjustments.EventDedup's own capped design.AlarmInput::from_record(was duplicated betweendismiss_alarmandtoggle_alarm).Known, disclosed tradeoff
The time-window debounce is a stopgap for a real plumbing gap (no shared
event_idacross the TS-command and native-round-trip delivery paths). The more correct long-term fix — threading a sharedevent_idthrough both paths and reusing the existing, exact-matchEventDedupmechanism — would require touching already-merged files in #301. Documented in code and filed as #304 rather than expanding this fix's scope.Testing checklist (automated, no device needed)
tokio::join!don't double-advance; a failed dismiss doesn't poison the retry window; one-shot double-dismiss stays safecargo check --workspace/cargo test --workspaceclean, no regressionscargo clippy/cargo fmt --checkcleanReview status
Reviewed via
/code-review high, three rounds given the correctness sensitivity of this path. First pass caught the originalnext_trigger-shape heuristic's regression against early dismiss. Second pass caught: no rollback on failure, a wall-clock backward-jump bug, unnecessary cross-alarm serialization, and unbounded map growth. All fixed; final pass clean.Stacked on #302. Part of #255 (a fix arising from its implementation, not a pre-planned phase).