fix(notify): make the fire-once dedupe guard cross-isolate safe#145
Conversation
Derivation runs in two isolates — the long-lived foreground pass (kept alive for BLE) and the WorkManager background pass — and both call NotificationCenter.emit() → FiredKeyStore. The guard was defeated across them, so a same-day insight (e.g. "Low readiness today") re-fired repeatedly and re-alerted after every dismissal: - Stale read cache: legacy SharedPreferences loads a snapshot once and never re-reads disk, so the foreground isolate never saw a key the background isolate had recorded, and hasFired() re-reported "not fired". - Lost-update clobber: keys lived in one shared list mutated via read-modify-write; with no cross-process lock, the last writer rewrote the whole list from its stale copy and dropped the other isolate's keys. FiredKeyStore now reload()s before every read and stores one independent bool flag per key instead of a shared list, so neither isolate can mask or clobber the other's record. Growth is bounded by pruning date-prefixed flags older than a 14-day window (replacing the old FIFO cap). The in-isolate emit lock is unchanged; its comment now scopes it correctly. Tests can't span isolates, so the previous suite passed while the bug shipped; the dedupe tests are updated to the per-key model and cover retention pruning and undated-key retention.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesNotification deduplication
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant NotificationCenter
participant FiredKeyStore
participant LocalDb
participant presentSink
NotificationCenter->>FiredKeyStore: claim(dedupeKey)
FiredKeyStore->>LocalDb: atomic claim
LocalDb-->>FiredKeyStore: winner or duplicate
FiredKeyStore-->>NotificationCenter: claim result
NotificationCenter->>presentSink: present notification
presentSink-->>NotificationCenter: shown or failed
NotificationCenter->>FiredKeyStore: release when not shown
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/notify/fired_keys.dart`:
- Around line 55-70: Make FiredKeyStore’s same-key claim atomic across isolates
by replacing the separate hasFired/recordFired protocol around hasFired and
recordFired with a transactional or locked storage operation that checks and
records the key as one unit. In lib/notify/notification_center.dart lines 30-35,
update the integration so it relies on the atomic claim rather than assuming the
existing protocol provides cross-isolate fire-once behavior. In
test/notification_dedupe_test.dart lines 217-234, add a barrier-controlled
multi-isolate regression test asserting that concurrent claimants for the same
key result in only one presentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9adf8c29-764b-464e-9b16-3995a732ef74
📒 Files selected for processing (3)
lib/notify/fired_keys.dartlib/notify/notification_center.darttest/notification_dedupe_test.dart
| Future<bool> hasFired(String dedupeKey) async { | ||
| final p = await SharedPreferences.getInstance(); | ||
| final keys = p.getStringList(_prefsKey); | ||
| return keys != null && keys.contains(dedupeKey); | ||
| await _reload(p); | ||
| return p.getBool('$_prefix$dedupeKey') ?? false; | ||
| } | ||
|
|
||
| /// Record [dedupeKey] as fired. Bounded FIFO: the newest key is appended and | ||
| /// the oldest evicted once past [maxKeys]. A key already present is left | ||
| /// untouched — a repeated hit must NOT re-fire and must NOT refresh its | ||
| /// recency (otherwise a hot duplicate could keep a stale key pinned forever). | ||
| /// Record [dedupeKey] as fired — one independent flag, so a concurrent record | ||
| /// of a different key from the other isolate can't clobber it. Recording the | ||
| /// same key again is a harmless idempotent no-op. Prunes stale flags after. | ||
| Future<void> recordFired(String dedupeKey) async { | ||
| final p = await SharedPreferences.getInstance(); | ||
| final keys = List<String>.of(p.getStringList(_prefsKey) ?? const <String>[]); | ||
| if (keys.contains(dedupeKey)) return; | ||
| keys.add(dedupeKey); | ||
| if (keys.length > maxKeys) { | ||
| keys.removeRange(0, keys.length - maxKeys); | ||
| } | ||
| await p.setStringList(_prefsKey, keys); | ||
| // Reload first so both the write and the prune below act on the latest | ||
| // cross-isolate state (and so _prune enumerates the other isolate's keys). | ||
| await _reload(p); | ||
| await p.setBool('$_prefix$dedupeKey', true); | ||
| await _prune(p); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the fired-key claim atomic across isolates.
Two isolates can both reload and read false, then both call presentSink before either reaches setBool. Per-key flags prevent different-key clobbers, but do not enforce fire-once for the same key.
lib/notify/fired_keys.dart#L55-L70: replace the separate read/record protocol with an atomic cross-isolate claim operation backed by transactional/locked storage.lib/notify/notification_center.dart#L30-L35: do not claimFiredKeyStoreprovides cross-isolate fire-once until the atomic claim exists.test/notification_dedupe_test.dart#L217-L234: add a barrier-controlled, multi-isolate regression test proving only one same-key claimant can present.
📍 Affects 3 files
lib/notify/fired_keys.dart#L55-L70(this comment)lib/notify/notification_center.dart#L30-L35test/notification_dedupe_test.dart#L217-L234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/notify/fired_keys.dart` around lines 55 - 70, Make FiredKeyStore’s
same-key claim atomic across isolates by replacing the separate
hasFired/recordFired protocol around hasFired and recordFired with a
transactional or locked storage operation that checks and records the key as one
unit. In lib/notify/notification_center.dart lines 30-35, update the integration
so it relies on the atomic claim rather than assuming the existing protocol
provides cross-isolate fire-once behavior. In test/notification_dedupe_test.dart
lines 217-234, add a barrier-controlled multi-isolate regression test asserting
that concurrent claimants for the same key result in only one presentation.
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Follow-up to the review on OpenStrap#145. Two gaps remained. 1. TOCTOU — the guard was still check-then-record Reload-before-read and per-key flags fix a stale read and a lost update, but neither makes the guard atomic: hasFired() and recordFired() are two separate operations, so the foreground derive isolate and the WorkManager one can both read "not fired" before either writes, and both present. Fresher reads shrink that window; they cannot close it. SharedPreferences has no primitive that can. SQLite does. `notif_fired(key PRIMARY KEY)` turns the guard into one atomic INSERT OR IGNORE: for a given key exactly one caller in the process ever sees changes() == 1, and only that caller presents. emit() now claims instead of checking, and releases the claim on every path where the present did not actually happen (permission denied, OS error, a throw) — preserving the existing "record only after a real present" semantics, which a claim-first protocol would otherwise break. The in-isolate emit lock stays: it still serialises overlapping emits here. It is just no longer what enforces fire-once, and its comment says so. If the DB is unusable (torn down mid-background pass, absent in a unit test), claim falls back to the per-key prefs flags — the OpenStrap#145 behaviour, narrow window and all. The fallback deliberately does NOT assume "already fired": a broken DB must never silently mute every notification on the device. 2. No migration off the legacy shared list OpenStrap#145 changed the on-disk shape without carrying the old `notif_fired_keys` list over, so every key that had already fired on the day of the upgrade would have fired a second time, and the dead list would have sat in prefs forever. It is now drained into the claim table on first use and deleted; its absence is the already-migrated marker, so the pass is idempotent and costs one cached read. Tests notification_claim_atomic_test.dart runs the real LocalDb via sqflite ffi: concurrent same-key claimants (no in-isolate lock — the cross-isolate shape) yield exactly one winner, release makes a key claimable again, a denied or throwing present does not consume it, legacy keys do not re-fire, and retention prunes dated claims while leaving undated ones. Verified to have teeth: swapping claim() back to check-then-record fails the concurrency test. notification_dedupe_test.dart is unchanged in intent and now documents that it covers the degraded fallback (no sqlite factory registered). Its date helper uses dayLabelOf, matching the store's own cutoff and the local-day convention. DB schema 25 -> 26, purely additive. No analytics output changes, so kAlgoVersion is untouched.
|
Pushed 1. The TOCTOU race (CodeRabbit 🟠 / PR-Agent "TOCTOU race remains") — fixed properly. Both reviews landed on the same real thing: reload-before-read and per-key flags fix a stale read and a lost update, but SQLite does. New The one thing an atomic claim breaks if you're not careful is the existing "record only after a real present" rule (a permission-denied no-op must not consume the day's only chance to fire). So a claim we don't spend is released — on denial, on OS error, and on a throwing present. The in-isolate lock stays; it still serialises overlapping emits here. It's just no longer what enforces fire-once, and the comment now says that instead of claiming FiredKeyStore's reload+per-key flags do it. Degraded mode. If the DB is unusable (torn down mid-background pass, absent in a plain unit test) claim falls back to this PR's per-key prefs flags — narrow window and all. It deliberately does not assume "already fired": an unusable DB must never silently mute every notification on the device. 2. Missing migration off the legacy list — fixed. Not flagged by either bot: the PR changed the on-disk shape without carrying On the multi-isolate regression test CodeRabbit asked for. A unit test can't spawn the WorkManager isolate — but it doesn't need to. Both isolates hit the same SQLite file, and the claim's correctness is a property of the statement, not of who calls it. Calling Also covered there: release makes a key claimable again, a denied and a throwing present don't consume it, legacy keys don't re-fire, retention prunes dated claims and leaves undated ones. DB schema 25 → 26, purely additive. No analytics outputs change, so Full suite: 605 pass. The 3 failures in |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/db.dart (1)
90-126: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a SQLite busy timeout for cross-isolate writes
claimNotifFired()can still hitSQLITE_BUSYimmediately when the foreground and WorkManager isolates contend for the WAL file._guardedWrite()only retries closed-handle failures, so lock contention drops straight into the degraded SharedPreferences path. AddingPRAGMA busy_timeouthere would let the second writer wait briefly instead of bypassing the atomic DB claim.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/data/db.dart` around lines 90 - 126, Add a non-fatal PRAGMA busy_timeout configuration in the _open() onConfigure callback, alongside the existing synchronous and cache_size pragmas, so competing isolate writes wait briefly for the WAL lock. Execute it through the same guarded performance-pragma handling and preserve database opening even if the setting fails; do not modify claimNotifFired() or _guardedWrite().
🧹 Nitpick comments (1)
test/notification_claim_atomic_test.dart (1)
62-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded near-term dates will start colliding with the 14-day retention prune.
_dayOffset()(Lines 38-40) exists for exactly this, but most tests hardcode'2026-07-25'instead. Since every successfulclaim()triggersLocalDb.pruneNotifFired(_cutoffLabel()), once "today" is more thanretentionDayspast that literal (mid-August 2026), these keys become prunable mid-test — some assertions would then only keep passing by coincidence (via the prefs-mirror fallback / reconciliation branch), rather than actually testing the intended behavior.Suggest swapping the hardcoded literals for
'${_dayOffset(0)}:...'(or similar) throughout this file so the suite stays meaningful regardless of when it runs.Also applies to: 156-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/notification_claim_atomic_test.dart` around lines 62 - 110, Replace hardcoded date prefixes such as '2026-07-25' throughout the notification claim tests, including the atomic claim tests and the additional cases around lines 156-187, with the existing _dayOffset(0) helper while preserving each key suffix. Ensure all claim and persistence assertions use current-day keys so retention pruning cannot affect the intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/notify/fired_keys.dart`:
- Around line 113-130: Update the reconciliation branch in the claim method
around the p.getBool('$_prefix$dedupeKey') check so it relinquishes only the
newly acquired DB claim without removing the existing prefs marker. Do not call
the public release(dedupeKey), since it clears both stores; use a DB-only
release mechanism or equivalent targeted deletion, while preserving the false
return and keeping the degraded-mode evidence intact.
---
Outside diff comments:
In `@lib/data/db.dart`:
- Around line 90-126: Add a non-fatal PRAGMA busy_timeout configuration in the
_open() onConfigure callback, alongside the existing synchronous and cache_size
pragmas, so competing isolate writes wait briefly for the WAL lock. Execute it
through the same guarded performance-pragma handling and preserve database
opening even if the setting fails; do not modify claimNotifFired() or
_guardedWrite().
---
Nitpick comments:
In `@test/notification_claim_atomic_test.dart`:
- Around line 62-110: Replace hardcoded date prefixes such as '2026-07-25'
throughout the notification claim tests, including the atomic claim tests and
the additional cases around lines 156-187, with the existing _dayOffset(0)
helper while preserving each key suffix. Ensure all claim and persistence
assertions use current-day keys so retention pruning cannot affect the intended
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61704945-a8e0-4616-a13e-6c78b7ab1772
📒 Files selected for processing (5)
lib/data/db.dartlib/notify/fired_keys.dartlib/notify/notification_center.darttest/notification_claim_atomic_test.darttest/notification_dedupe_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- test/notification_dedupe_test.dart
…claim Real bug, caught in review of the previous commit. When a DB claim won but the prefs mirror already read true — the state a fallback claim leaves behind, i.e. "the DB was down when this key fired" — the reconciliation branch called the public release(), which clears BOTH stores. That erased the mirror: the only evidence of the degraded-mode fire. Both stores then read "not fired", so the next derive pass won the claim cleanly and re-alerted, reopening this PR's bug in exactly the recovered-from-outage path the comments call out as expected. A mirror that already reads true means the key HAS fired, so the row we just inserted is the truth arriving late. Keep it and back off. Undoing the insert (releasing only the DB row) also stops the immediate re-fire, but repeats the insert/delete dance on every subsequent pass and leaves the claim table permanently ignorant of a fire it should own. Reconciling forward converges after one pass: from then on the DB alone loses the claim. release() is unchanged and still clears both — that is correct for its actual job, "the present did not happen, forget it everywhere". The bug was reusing it for reconciliation, which is the opposite intent. Regression test drives the on-disk state a degraded claim leaves behind and asserts three consecutive passes all lose, then asserts the claim table has absorbed the fire. Verified to fail on the previous commit at pass 1.
2 participants
Problem
A same-day insight notification — reproduced with "Low readiness today" — re-fires repeatedly and re-alerts every time it's dismissed. The
dedupeKey(\$date:readiness) is stable, so the fire-once guard added in #137 (issue #136) is being defeated across isolates, not bypassed by a changing key.Derivation runs in two isolates, both of which call
NotificationCenter.emit()→FiredKeyStore:AppState._afterDrainkicks a light derive on every BLE drain, and the foreground service keeps this isolate alive for days while backgrounded.derivationDispatcherrunsengine.run(heavy: true)(→_runNotifications) every ~15 min, even app-closed.NotificationCenter's_synchronizedlock only orders emits within one isolate. The store defeated its own guard two ways across them:SharedPreferencesloads a snapshot once and never re-reads disk. The long-lived foreground isolate loaded prefs before today's key existed, so after the background isolate recorded it,hasFired()still read the stale snapshot as "not fired" → re-alert.getStringList→ add →setStringList). With no cross-process lock, whichever isolate wrote last rewrote the whole list from its own stale copy, dropping the other's keys — including a just-recorded one, which then re-fired.The existing tests are single-isolate against one in-memory mock, so they stayed green while this shipped.
Fix
FiredKeyStoreis now cross-isolate safe:reload()before every read so a record from the other isolate is seen.boolflag per key (notif_fired:\$dedupeKey) instead of a shared list. Native SharedPreferences merges single-key writes, so concurrent records of different keys can't clobber each other and a repeat record is idempotent._prune()— date-prefixed flags older than a 14-day window are dropped (replacing the old FIFO cap). Undated keys (e.g.alarm_fired:<epoch>) are rare and left alone.The in-isolate emit lock is unchanged; its comment is corrected to scope it to a single isolate.
Tests
test/notification_dedupe_test.dartis updated to the per-key model and adds coverage for retention pruning and undated-key retention. Behavioural dedupe/gating/concurrency tests are unchanged in intent.Summary by CodeRabbit