Close the in-app dismiss gap and propagate stop signals natively - #301
Open
ScottMorris wants to merge 5 commits into
Open
Close the in-app dismiss gap and propagate stop signals natively#301ScottMorris wants to merge 5 commits into
ScottMorris wants to merge 5 commits into
Conversation
ScottMorris
changed the title
ScottMorris
added
architecture
This was referenced Aug 14, 2026
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: a12fdd6241
ℹ️ 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
added a commit
that referenced
this pull request
Aug 18, 2026
…gle lines Fixes the remainder of the AGENTS.md Markdown convention violation flagged by automated review on PR #301, beyond the one blockquote already fixed in the previous commit -- covers the new WatchStopInitProvider.kt/WatchStopListener.kt files, the id-threading additions to AlarmManagerPlugin.kt, the Rust-side StopRingingRequest/stop_ringing_for docs, and the TS/test comments across Ringing.tsx, AlarmManagerService.ts, guest-js/index.ts, and their test files. Comment-only, no behavioural change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ScottMorris
force-pushed
the
feat/255-p4a-alarm-manager-stop
branch
from
August 18, 2026 16:56
a12fdd6 to
601a7da
Compare
ScottMorris
added a commit
that referenced
this pull request
Aug 18, 2026
…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
…nd publish it uniformly In-app dismiss (Ringing.tsx's Stop Alarm button -> AlarmManagerService.stopRinging() -> Kotlin's stopRinging command) built the ACTION_DISMISS intent with no ALARM_ID extra at all, so AlarmRingingService defaulted alarmId to -1 and AlarmManagerPlugin's if (alarmId <= 0) return guard silently dropped the event -- in-app dismiss produced no native dismiss event whatsoever, not just a degraded one. Thread the real alarm id explicitly from Ringing.tsx (it already has it in scope) all the way through guest-js/mobile.rs/AlarmManagerPlugin.kt, so every dismiss origin now produces a real id and notifyAlarmDismissed's guard passes uniformly, without an origin-based special case. In-app snooze deliberately keeps passing no id: stopRinging's single Kotlin command is shared between dismiss and snooze (it always sends ACTION_DISMISS), so threading an id through there too would misattribute a snooze as a dismiss and, via Rust's dismiss_alarm recomputing next_trigger off whatever is currently stored, risk clobbering the snooze just requested. notifyAlarmDismissed/notifySnoozeRequested also now publish onto NativeEventBus (in addition to the existing DurableEventQueue/Channel path to Rust), letting wear-sync's own listener (a sibling worktree, issue #255 Phase 4B) tell the watch to stop instantly without waiting for Rust to boot -- this only works for in-app dismiss now that it carries a real id. Verified (but did not fix, out of scope for this plugin) that Rust's dismiss_alarm is not cleanly idempotent under a sequential double-invocation for repeating alarms, which this change's uniform publish can now trigger for in-app dismiss specifically (the direct AlarmService.dismiss() TS command and the newly-uniform native dismiss-requested channel event both reach it); documented in event-architecture.md and flagged for follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WVm6JTTpiSY8KSKJ3p5Qq
…ginated dismiss/snooze Adds WatchStopInitProvider (mirrors wear-sync's WearRingInitProvider from Phase 3B) so WatchStopListener is subscribed to NativeEventBus before any other component can run, even on a cold multi-plugin process start. It subscribes to wear-sync's wear:alarm:dismiss/wear:alarm:snooze topics (published by wear-sync's own listener, a sibling worktree, issue #255 Phase 4B) and, when the payload's alarmId matches AlarmRingingService.currentlyRingingAlarmId, stops the service directly via Context.stopService -- silencing audio/vibration/the notification without waiting for Rust to boot. This is the phone-side half of issue #255's symmetric stop signals: the watch can now silence the phone's ringing as fast as the phone can silence the watch's. Deliberately does not re-publish through notifyAlarmDismissed/notifySnoozeRequested or enqueue anything of its own -- Rust's catch-up for the watch-originated command is already durably queued by wear-sync's existing offline-write path independent of this listener, so doing so would just be a redundant, second dismiss/snooze event. Snooze's re-arm stays entirely Rust-side and queued, unchanged -- this only stops the local ringing. The payload key is "alarmId" (matching wear-sync's WatchDismissAlarm/WatchSnoozeAlarm and the watch's own pre-existing message shape), not "id" like this plugin's own alarm-manager:dismiss-requested/snooze-requested topics -- the two topic pairs are not on the same wire-shape convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WVm6JTTpiSY8KSKJ3p5Qq
Ringing.tsx's Test Alarm (SPECIAL_ALARM_IDS.TEST_ALARM = 999) sound-preview flow isn't a real DB-backed alarm, but handleDismiss's new stopRinging(alarmId) call from the previous commit didn't special-case it the way closeRingingWindow already does. Threading id 999 through durably enqueues and drains a real alarm-manager:dismiss-requested event that Rust's listener can never resolve (get_by_id(999) always fails), logging a fresh error and writing to the durable queue on every sound preview -- noise that didn't exist before alarm ids were threaded through stopRinging. handleDismiss now follows the same Test Alarm special-case already established in closeRingingWindow: stopRinging() gets no id for the sentinel, keeping sound preview a pure local stop with no native round-trip, matching its pre-existing behaviour. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WVm6JTTpiSY8KSKJ3p5Qq
…gle lines Fixes the remainder of the AGENTS.md Markdown convention violation flagged by automated review on PR #301, beyond the one blockquote already fixed in the previous commit -- covers the new WatchStopInitProvider.kt/WatchStopListener.kt files, the id-threading additions to AlarmManagerPlugin.kt, the Rust-side StopRingingRequest/stop_ringing_for docs, and the TS/test comments across Ringing.tsx, AlarmManagerService.ts, guest-js/index.ts, and their test files. Comment-only, no behavioural change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ScottMorris
force-pushed
the
feat/255-p4a-alarm-manager-stop
branch
from
August 18, 2026 17:08
601a7da to
78f0c42
Compare
ScottMorris
added a commit
that referenced
this pull request
Aug 18, 2026
…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
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
Phase 4A of #255's implementation plan — alarm-manager's side of "symmetric stop signals" (dismiss/snooze propagating natively in both directions without waiting for Rust to boot, the same pattern Phase 3 established for the fired→ring path). No dedup tags needed here (per decision 4: a double-delivered stop is benign, only the ring needed tags).
Also closes a real, independently-verified gap: in-app dismiss previously sent no native dismiss event at all.
What's here
Ringing.tsx→guest-js→ Rust → Kotlin), explicit id-passing from the TS layer rather than a Kotlin-side fallback — the fallback would have corrupted in-app snooze, sincestopRinging's single Kotlin command always sendsACTION_DISMISSregardless of dismiss vs. snooze intent (documented in code).alarm-manager:dismiss-requesteduniformly (the old "only notification-action dismisses publish" special case is gone, since every origin now produces a real id).ContentProvider(WatchStopInitProvider/WatchStopListener) subscribes towear:alarm:dismiss/wear:alarm:snoozeand stopsAlarmRingingServicenatively on a watch-originated stop, mirroring wear-sync's own Phase 3B pattern.Making dismiss publish uniformly surfaced a real bug this PR does not fix:
AlarmCoordinator::dismiss_alarm(Rust) is not idempotent — it recomputesnext_triggerrelative to whatever's currently stored, with no guard. In-app dismiss now reaches it twice (the direct TS command, plus the newly-uniform native round trip), which for a repeating alarm can silently skip an entire occurrence. This was caught during implementation, verified by code review, and is being fixed in a follow-up PR in this same stack (Rust-core change, out of this PR's scope) rather than patched around here.Testing checklist (automated, no device needed)
cargo check --workspace,cargo test -p tauri-plugin-alarm-manager5/5,cargo test -p threshold --lib alarm::44/44pnpm -r run typecheck,pnpm --filter threshold test86/86Review status
Reviewed via
/code-review high. Confirmed the dismiss_alarm idempotency finding (see above) and found the Test Alarm preview regression (fixed). Both are documented; the idempotency issue is tracked as a follow-up in this stack rather than silently worked around.Stacked on #300. Part of #255.