ADR-0072: Prevent notification storm on Android#4381
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe ADR specifies a client-side strategy to prevent Android notification storms after long offline periods when many FCM background messages arrive. It requires skipping display of local notifications while the app is in the foreground but still updating delivery state via cache refresh. It adds a burst limiter capping generated local notifications to 20 per push burst, prioritizing emails marked with the keyword `needs-action`, then Inbox mailbox messages, then newest-first. It hardens notification cancellation by wrapping cancel calls in error handling. It also references complementary backend use of `collapse_key`. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0072-prevent-notification-storm-android.md`:
- Line 97: Hyphenate the numeric adjective in the sentence "The background push
handler introduces a 30 second debounce window" by changing "30 second" to
"30-second" so it reads "The background push handler introduces a 30-second
debounce window"; locate the exact sentence in the ADR text and update it
accordingly.
- Line 17: Update every fenced code block in the ADR document that currently
uses bare backticks to include a language identifier (e.g., change ``` to
```text). Specifically, replace the unnamed fences that enclose examples like
the block with "100 push events at T0" and the other unnamed examples throughout
the ADR so they become labeled (e.g., ```text) to satisfy markdownlint MD040 and
improve readability.
- Around line 261-272: The code is saving the email cache state
(StateType.email) then incorrectly reusing newState when storing the delivery
cursor to FCM; update the FCMCacheManager.storeStateToRefresh call to pass the
delivery-state token (e.g., deliveryNewState or deliveryCursor) for
TypeName.emailDelivery rather than newState so the delivery stream and email
stream remain separate; locate the calls to stateCacheManager.saveState and
FCMCacheManager.storeStateToRefresh and replace the second argument value for
the TypeName.emailDelivery call with the correct delivery-state variable
returned/produced by your email-delivery processing logic.
- Around line 106-117: The check-then-set in shouldProcessPush allows a race
where multiple handlers read the old 'last_push_processed_time' before any
write, so two handlers can both proceed; fix it by performing a check, writing
the candidate timestamp, then re-reading to confirm you won the race: read
prefs.getInt('last_push_processed_time'), if within debounce return false; else
compute now, await prefs.setInt('last_push_processed_time', now), then read back
prefs.getInt('last_push_processed_time') and only return true if that stored
value equals now (otherwise return false) so only the first writer proceeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9708a519-1aa6-411c-aead-c32446de4d66
📒 Files selected for processing (1)
docs/adr/0072-prevent-notification-storm-android.md
dab246
changed the title
|
|
||
|
|
||
|
|
||
| # 1. Push debounce in background handler (30s window) |
There was a problem hiding this comment.
I propose the following algorithm, not a normal debounce
WHEN receiving FCM call with emailDelivery wait 1 minute, ignore received message during this 1 minute, then and only then fire notifs for the last 1 minute.
There was a problem hiding this comment.
The proposed algorithm is closer to a batch aggregation window rather than a debounce. However, the FCM background handler runs inside a short-lived Flutter isolate and is not designed for long-running tasks. Delaying notification processing for 1 minute would require keeping the isolate alive, which is not guaranteed by the Android system. Therefore the ADR proposes a simpler debounce mechanism that drops repeated push events within a 30s window while still allowing the notification pipeline to run immediately for the first event.
|
|
||
| Rules: | ||
|
|
||
| * maximum **20 notifications** |
|
|
||
|
|
||
|
|
||
| # Action-required Email Detection |
There was a problem hiding this comment.
Good idea but let's not implement this for now IMO
| Selection priority: | ||
|
|
||
| 1. actionable emails (`needs-action`) | ||
| 2. newest emails |
There was a problem hiding this comment.
- Favour INBOX, ignore SPAM / Trash
| } | ||
| ``` | ||
|
|
||
| This ensures notification removal failures do not interrupt the notification pipeline. |
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
Small adjustments but I mostly agree with this document.
And getting the ADR before writing code is truely nice. THANKS for making the effort.
| ```dart | ||
| Future<void> handleFirebaseBackgroundMessage(RemoteMessage message) async { | ||
|
|
||
| if (!await shouldProcessPush()) { |
There was a problem hiding this comment.
please check the posibility of this in isolated in flutter
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0072-prevent-notification-storm-android.md`:
- Line 126: Change the "Background execution constraints" heading from level-3
to level-2 (replace "### Background execution constraints" with "## Background
execution constraints") so it directly follows the document's level-1 section
and ensures headings increment by one; also scan nearby headings in the ADR to
confirm no other headings skip a level and adjust them similarly.
- Around line 199-201: The example uses a hard-coded "inbox-id"; update the
isInboxEmail guidance to explain how to derive the real inbox ID for an account
(e.g., inspect the account's mailbox objects and find the mailbox whose
mailboxRole == MailboxRole.inbox or equivalent) and then check that ID against
email.mailboxIds in isInboxEmail (function isInboxEmail, type PresentationEmail,
property mailboxIds); mention that implementers should resolve the inbox mailbox
ID from the account's mailbox list rather than using a literal string.
- Around line 217-251: selectEmailsForNotification currently collects all
actionEmails uncapped and then trims the combined list only at the end, which
can let action emails monopolize the 20 slots; change the logic so actionEmails
is capped to at most maxNotifications (use actionEmails.take(maxNotifications)),
compute remainingSlots = maxNotifications - cappedActionCount, then fill
remainingSlots from newestInboxEmails (constructed from the heap) and return
[...cappedActionEmails, ...newestInboxEmailsTaken] so the total never exceeds
maxNotifications and inbox slots are preserved when actions are abundant; keep
references to actionEmails, heap, newestInboxEmails, and maxNotifications when
making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b2f0032-7c08-4f07-bade-a1851ff36e05
📒 Files selected for processing (1)
docs/adr/0072-prevent-notification-storm-android.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/adr/0072-prevent-notification-storm-android.md (1)
94-107: Note potential race condition in debounce implementation.The
shouldProcessPush()example uses a check-then-set pattern: readinglastTime(line 97) and writing the new timestamp (line 104) are separate operations. If two FCM background handlers execute concurrently, both may read the old timestamp before either writes, allowing both to proceed and bypass the debounce.This race is low-probability (requires FCM isolates spawning within milliseconds of each other), but the ADR should either:
- Acknowledge this as an acceptable edge case for "best-effort" debouncing, or
- Propose a mitigation (e.g., atomic check-and-set via native code, file locking, or a check-write-verify pattern)
Given that this is an architectural proposal, consider adding a brief note in the ADR clarifying the atomicity guarantees (or lack thereof) of this approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0072-prevent-notification-storm-android.md` around lines 94 - 107, The debounce implementation in shouldProcessPush() has a potential race because it reads and then writes SharedPreferences separately; acknowledge this or propose a mitigation. Update the ADR text near the shouldProcessPush() example to either state that this is a best-effort, non-atomic debounce with possible rare duplicates, or describe a mitigation such as using an atomic check-and-set via native code or file-locking, or implementing a check-write-verify loop that re-reads the stored last_push_processed_time after writing and aborts if another handler has advanced it; reference shouldProcessPush() and the last_push_processed_time key in the note so readers can find the code and understand the atomicity guarantees (or lack thereof).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/adr/0072-prevent-notification-storm-android.md`:
- Around line 94-107: The debounce implementation in shouldProcessPush() has a
potential race because it reads and then writes SharedPreferences separately;
acknowledge this or propose a mitigation. Update the ADR text near the
shouldProcessPush() example to either state that this is a best-effort,
non-atomic debounce with possible rare duplicates, or describe a mitigation such
as using an atomic check-and-set via native code or file-locking, or
implementing a check-write-verify loop that re-reads the stored
last_push_processed_time after writing and aborts if another handler has
advanced it; reference shouldProcessPush() and the last_push_processed_time key
in the note so readers can find the code and understand the atomicity guarantees
(or lack thereof).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8fdb336-27cf-4d0d-89e4-3e21f0cf6cf8
📒 Files selected for processing (1)
docs/adr/0072-prevent-notification-storm-android.md
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
CF linagora/tmail-backend#2302
Let's focus on grouping notification implementation for a single FCM message.
|
|
||
| To prevent notification storms, we introduce **four defensive mechanisms**. | ||
|
|
||
| # 1. Push debounce in background handler (30s window) |
There was a problem hiding this comment.
Is this really needed if we collapse notification on the backend side?
There was a problem hiding this comment.
Edit: likely desirable as FCM do not do debounce, even with collapse key
|
|
||
| # Optimized Notification Selection Algorithm | ||
|
|
||
| To efficiently select notifications from large bursts of emails, the system keeps only the **top 20 most relevant emails**. |
There was a problem hiding this comment.
| To efficiently select notifications from large bursts of emails, the system keeps only the **top 20 most relevant emails**. | |
| To efficiently select notifications from large bursts of emails, the system keeps only the **top 10 most relevant emails**. |
| # 3. Update delivery state during foreground synchronization | ||
|
|
||
| The application already stores the **email delivery state** when push notifications are processed. |
There was a problem hiding this comment.
I think a key problem is that we currently only have email state that is used both for the Email collection sync and the notification through email delivery
Having a single subscription for both sync and notifs means waking up too often, and makes collapsing harder.
How about having 2 FCM subscription?
- One
Email + Mailboxfor resync - One
EmailDeliveryonly for notif
We thus know that each notification will be useful.
|
|
||
| The application already stores the **email delivery state** when push notifications are processed. | ||
|
|
||
| To ensure state consistency, the delivery state will also be updated when the email state cache is updated during **foreground synchronization**. |
There was a problem hiding this comment.
Those measure actually complement the collapse_key that will be specified by backend.
However this sounds like a LOT of work.
My recommendation to move this work forward is to end this with a MOSCOW of the various items for priorisation as well as identifying a strategy
(dedicated sprint? one enhanccement per sprint?)
Action items listed:
- Debounce on app side
- Limit number of notification
- Separate subscription fo
resyncandnotifs - Heuristic for important push (need-action + INBOX)
- Skip notifs when in foregound
- Fix local notification removal error
My proposal at this
| Item | Priorisation | Comment |
|---|---|---|
| Skip notifs when in foregound | MUST | |
| Limit number of notification | MUST | |
| Fix local notification removal error | SHOULD | |
| Heuristic for important push (need-action + INBOX) | SHOULD - needs product validation | |
Separate subscription fo resync and notifs |
WONT | Correlated with app removal |
| Debounce on app side | WONT | Other steps make this a edge case not worth the team time to fix |
Also another question: for resynch is it really useful to use FCM ? Are we using it to leverage an offline cache? If not how about dropping FCM ENTIRELY for resync and replace it by WebSocket when in foreground ?
|
Work to be done to merge this ADR:
|
…tion storm on Android Signed-off-by: dab246 <tdvu@linagora.com>
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/adr/0073-prevent-notification-storm-android.md (1)
179-181:⚠️ Potential issue | 🟡 MinorAvoid literal
"inbox-id"in the helper example.Using a string literal here can be read as implementation guidance. Prefer passing a resolved inbox mailbox ID into the helper to make the flow explicit.
🧩 Proposed clarification
-bool isInboxEmail(PresentationEmail email) { - return email.mailboxIds?.contains("inbox-id") ?? false; +bool isInboxEmail(PresentationEmail email, MailboxId inboxId) { + return email.mailboxIds?.containsKey(inboxId) ?? false; }Based on learnings:
PresentationEmailstores onlymailboxIds; inbox mailbox ID must be resolved upstream from mailbox list and then used forisInboxEmail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0073-prevent-notification-storm-android.md` around lines 179 - 181, The helper is using a hard-coded "inbox-id" literal; change the signature of isInboxEmail to accept the resolved inbox mailbox ID (e.g., inboxMailboxId: String) and use that parameter to check email.mailboxIds?.contains(inboxMailboxId) ?? false; locate and update the isInboxEmail function and any callers to resolve the inbox ID from the mailbox list upstream (using PresentationEmail.mailboxIds) and pass it in rather than relying on the literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0073-prevent-notification-storm-android.md`:
- Around line 105-113: The Decision table includes WON’T rows that should be
moved to Alternatives: remove the two WON’T rows ("Separate subscription for
`resync` and `notifs`" and "Debounce on app side") from the Decision table and
ensure those items remain documented under the Alternatives section (keeping
their text and rationale), then update the Decision table so it only contains
MUST/SHOULD items and confirm the table formatting/row alignment is preserved.
- Around line 103-104: The ADR uses mixed English variants for
"prioritise/prioritize" — decide on a single variant (e.g., American
"prioritize/prioritized" or British "prioritise/prioritised") and update every
occurrence in this document to that chosen form (notably the sentence "Given the
scope of work, items are prioritised using MoSCoW..." and the later
"prioritize"/"prioritized" occurrences around the MoSCoW section) so the
phrasing is consistent throughout; ensure the chosen variant is applied to both
verb and past participle forms.
---
Duplicate comments:
In `@docs/adr/0073-prevent-notification-storm-android.md`:
- Around line 179-181: The helper is using a hard-coded "inbox-id" literal;
change the signature of isInboxEmail to accept the resolved inbox mailbox ID
(e.g., inboxMailboxId: String) and use that parameter to check
email.mailboxIds?.contains(inboxMailboxId) ?? false; locate and update the
isInboxEmail function and any callers to resolve the inbox ID from the mailbox
list upstream (using PresentationEmail.mailboxIds) and pass it in rather than
relying on the literal.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f078a55-b226-4ce9-85ae-a13f14c39285
📒 Files selected for processing (1)
docs/adr/0073-prevent-notification-storm-android.md
…otification storm on Android Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/adr/0073-prevent-notification-storm-android.md (1)
110-110: Use one keyword form consistently:needs-actionvsneed-action.Line 110/362 uses
need-action, while Line 166-173 and selection logic useneeds-action. Keep one canonical token everywhere to avoid implementation drift.Also applies to: 162-167, 362-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0073-prevent-notification-storm-android.md` at line 110, The ADR uses two different keyword forms (`need-action` and `needs-action`) which will cause implementation drift; standardize on the canonical token `needs-action` everywhere by replacing all instances of `need-action` (e.g., the table row at "Heuristic for important push" and the occurrences called out around lines 162-167 and 362) and update any selection/heuristic logic references that parse or match this tag (search for `need-action` in the document and in the selection logic description) so all mentions, examples, and matching rules consistently use `needs-action`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0073-prevent-notification-storm-android.md`:
- Around line 179-181: The helper isInboxEmail currently requires an
inboxMailboxId parameter and uses Map.contains, causing a mismatch where calls
pass no argument and mailboxIds is a Map; change the signature to remove the
second parameter so it becomes bool isInboxEmail(PresentationEmail email) and
inside use email.mailboxIds?.containsKey(inboxMailboxId) ?? false (using the
existing inboxMailboxId from the surrounding scope) to correctly check Map
membership.
---
Nitpick comments:
In `@docs/adr/0073-prevent-notification-storm-android.md`:
- Line 110: The ADR uses two different keyword forms (`need-action` and
`needs-action`) which will cause implementation drift; standardize on the
canonical token `needs-action` everywhere by replacing all instances of
`need-action` (e.g., the table row at "Heuristic for important push" and the
occurrences called out around lines 162-167 and 362) and update any
selection/heuristic logic references that parse or match this tag (search for
`need-action` in the document and in the selection logic description) so all
mentions, examples, and matching rules consistently use `needs-action`.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12f5dcde-8708-4e06-9329-5f19b07967f0
📒 Files selected for processing (1)
docs/adr/0073-prevent-notification-storm-android.md
…event notification storm on Android Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
4 participants
Add ADR-0072: Prevent notification storm on Android
Summary
This PR introduces a new Architecture Decision Record (ADR) that defines how the application prevents notification storms on Android when a device reconnects after a long idle period.
The ADR documents the root causes of the issue and proposes several defensive mechanisms to stabilize the push notification system and limit excessive local notifications.
Related Issue
Fixes #4366
Problem
Some Android users experience a large burst of notifications when their device reconnects to the network after being idle (for example overnight).
When reconnection occurs, Firebase Cloud Messaging (FCM) may deliver many push events within a short time window.
Each push currently triggers the full notification pipeline and may generate multiple local notifications.
Example scenario:
This can lead to:
Decision
To mitigate this issue, the ADR proposes four defensive mechanisms.
1. Push debounce in background handler
A debounce window is introduced in the background push handler to prevent repeated push bursts from triggering the notification pipeline multiple times.
When multiple FCM messages arrive within a short period of time:
This prevents push bursts from repeatedly executing the full notification pipeline.
2. Notification limiter
When a push burst is processed, the application limits the number of generated notifications.
Rules:
needs-action) are prioritizedSelection priority:
Emails delivered to other mailboxes (Sent, Drafts, Spam, Trash, etc.) will not generate notifications.
This significantly reduces unnecessary notifications during large synchronization bursts.
3. Foreground delivery state synchronization
The email delivery state is currently stored when push notifications are processed.
To maintain state consistency, the delivery state will also be updated during foreground synchronization when the email state cache is refreshed.
This ensures the latest delivery state is always persisted even if updates originate outside the push pipeline.
4. Fix local notification removal error
The notification cancellation logic will be improved to safely handle failures.
Sentry reports the following error:
When cancellation fails:
The updated logic introduces defensive cancellation handling and safer notification removal.
Expected Benefits
This design provides several improvements:
Scope
This PR adds documentation only.
The ADR defines the architecture and design decisions.
Implementation will be delivered in follow-up issues and pull requests.
Files Added
Summary by CodeRabbit