Sync notifications with a Worker Task #3376
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a periodic push notification retrieval system integrated with the existing sync workflow. A new singleton manager (NotificationsRetrievalWorkerManager) schedules recurring notification fetches every hour using Android WorkManager, with constraints requiring network connectivity and non-low battery. A new worker (NotificationsRetrievalWorker) handles background notification retrieval with retry logic. The system is wired into PersonalIdManager to initiate scheduling on sign-in and cancellation on user removal, and integrated into PNApiSyncWorkerManager to chain notification retrieval after sync work completion. Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdManager
participant NotificationsRetrievalWorkerManager
participant WorkManager
participant NotificationsRetrievalWorker
participant API as PushNotificationAPI
User->>PersonalIdManager: Sign In
PersonalIdManager->>NotificationsRetrievalWorkerManager: schedulePeriodicPushNotificationRetrieval()
NotificationsRetrievalWorkerManager->>WorkManager: enqueueUniquePeriodicWork<br/>(1-hour interval)
Note over WorkManager: Periodic task scheduled<br/>with network constraint
loop Every hour
WorkManager->>NotificationsRetrievalWorker: doWork()
NotificationsRetrievalWorker->>API: fetchLatestNotifications()
alt Success
API-->>NotificationsRetrievalWorker: Notifications received
NotificationsRetrievalWorker->>WorkManager: Result.success()
else Failure & Retries Remaining
API-->>NotificationsRetrievalWorker: Error
NotificationsRetrievalWorker->>WorkManager: Result.retry()
else Failure & Max Retries Exceeded
NotificationsRetrievalWorker->>WorkManager: Result.failure()
end
end
User->>PersonalIdManager: Forget User
PersonalIdManager->>NotificationsRetrievalWorkerManager: cancelPeriodicPushNotificationRetrieval()
NotificationsRetrievalWorkerManager->>WorkManager: cancelUniqueWork()
Note over WorkManager: Periodic task cancelled
sequenceDiagram
participant WorkManager
participant PNApiSyncWorkerManager
participant SyncWorker as PN API SyncWorker
participant NotificationsRetrievalWorker
participant PushAPI as PushNotificationAPI
Note over WorkManager: Sync triggered
WorkManager->>PNApiSyncWorkerManager: Enqueue unique work
PNApiSyncWorkerManager->>WorkManager: beginUniqueWork(syncWorkRequest)<br/>.then(notificationRetrievalRequest)
WorkManager->>SyncWorker: doWork()
SyncWorker->>PushAPI: Sync data
PushAPI-->>SyncWorker: Success
SyncWorker-->>WorkManager: Result.success()
Note over WorkManager: Chain continues on success
WorkManager->>NotificationsRetrievalWorker: doWork()
NotificationsRetrievalWorker->>PushAPI: Fetch notifications
PushAPI-->>NotificationsRetrievalWorker: Notifications
NotificationsRetrievalWorker-->>WorkManager: Result.success()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple files with a mix of new implementations and existing flow modifications. New worker logic and manager code require careful review for correctness of retry semantics and WorkManager constraint configuration. The chaining in PNApiSyncWorkerManager represents a behavioral change that needs validation. However, the changes follow a coherent pattern and remain within a focused feature scope. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 1
🧹 Nitpick comments (2)
app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt (1)
166-168: Consider adding backoff criteria for consistency.The
notificationRetrievalRequestlacks backoff criteria, unlike thesyncWorkRequest(lines 160-164). While the worker has internal retry logic, adding WorkManager-level backoff would better handle transient failures.Apply this diff to add backoff criteria:
val notificationRetrievalRequest = OneTimeWorkRequestBuilder<NotificationsRetrievalWorker>() .setConstraints(Constraints.Builder().setRequiredNetworkType(NetworkType.CONNECTED).build()) + .setBackoffCriteria( + androidx.work.BackoffPolicy.EXPONENTIAL, + PN_SYNC_BACKOFF_DELAY_IN_MILLIS, + TimeUnit.MILLISECONDS + ) .build()app/src/org/commcare/pn/workers/NotificationsRetrievalWorker.kt (1)
39-42: Consider consistent retry logic for exception handling.The catch block returns
Result.failure()immediately, while API failures check retry count (lines 33-37). For consistency and resilience, consider checkingrunAttemptCountin the catch block as well.Apply this diff to add consistent retry handling:
} catch (e: Exception) { Logger.exception("PushNotificationRetrievalWorker encountered unexpected error", e) - Result.failure() + if (runAttemptCount < MAX_RETRIES) { + Result.retry() + } else { + Result.failure() + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/org/commcare/connect/PersonalIdManager.java(3 hunks)app/src/org/commcare/pn/workermanager/NotificationsRetrievalWorkerManager.kt(1 hunks)app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt(3 hunks)app/src/org/commcare/pn/workers/NotificationsRetrievalWorker.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/org/commcare/connect/PersonalIdManager.java (2)
app/src/org/commcare/pn/workermanager/NotificationsRetrievalWorkerManager.kt (2)
schedulePeriodicPushNotificationRetrieval(27-52)cancelPeriodicPushNotificationRetrieval(58-60)app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(157-1279)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
app/src/org/commcare/connect/PersonalIdManager.java (2)
206-211: LGTM! Notification retrieval scheduling integrated correctly.The periodic notification retrieval is appropriately scheduled after successful sign-in, consistent with the existing heartbeat scheduling pattern.
221-230: LGTM! Cleanup logic correctly implemented.The periodic notification retrieval is properly cancelled when the user is forgotten, ensuring no background work continues after user removal.
app/src/org/commcare/pn/workermanager/NotificationsRetrievalWorkerManager.kt (1)
1-61: LGTM! Well-structured manager implementation.The manager correctly implements scheduling and cancellation of periodic notification retrieval with appropriate constraints (network connectivity, battery not low) and exponential backoff policy.
| WorkManager.getInstance(context) | ||
| .beginUniqueWork(uniqueWorkName, ExistingWorkPolicy.KEEP, syncWorkRequest) | ||
| .then(notificationRetrievalRequest) | ||
| .enqueue() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify notification retrieval frequency to avoid redundancy.
Notification retrieval is chained after every sync action (which can be triggered multiple times per push notification) AND scheduled periodically every hour. If multiple sync actions occur in quick succession, this could result in multiple concurrent notification retrievals, potentially causing unnecessary API load.
Consider whether notification retrieval should:
- Only chain after the first/last sync in a batch, or
- Use a throttling mechanism to prevent multiple retrievals within a short time window
Run the following script to verify how frequently sync actions are triggered:
🏁 Script executed:
#!/bin/bash
# Check how many unique sync actions can be triggered per notification
ast-grep --pattern $'when (pn[REDIRECT_ACTION]) {
$$$
}'Length of output: 3115
🏁 Script executed:
#!/bin/bash
# View the startWorkRequest function and surrounding context to understand chaining logic
rg -n -A 30 'fun startWorkRequest' app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.ktLength of output: 1471
🏁 Script executed:
#!/bin/bash
# Check how the multiple work requests from a single action are being handled
rg -n -B 5 -A 15 'createOpportunitiesSyncWorkRequest' app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.kt | head -80Length of output: 2260
🏁 Script executed:
#!/bin/bash
# View the full chaining logic around lines 170-174
sed -n '150,180p' app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.ktLength of output: 1158
🏁 Script executed:
#!/bin/bash
# Search for where startWorkRequest is called to understand frequency
rg -n 'startWorkRequest' app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.ktLength of output: 442
🏁 Script executed:
#!/bin/bash
# Check if there's any batching or throttling logic around notification retrieval
rg -n 'notificationRetrieval\|throttle\|batch' app/src/org/commcare/pn/workermanager/Length of output: 0
🏁 Script executed:
#!/bin/bash
# View NotificationsRetrievalWorkerManager to understand the periodic retrieval
find app/src -name '*NotificationsRetrievalWorkerManager*' -type fLength of output: 76
🏁 Script executed:
#!/bin/bash
# View NotificationsRetrievalWorker to check for deduplication or throttling
cat app/src/org/commcare/pn/workermanager/NotificationsRetrievalWorker.ktLength of output: 101
🏁 Script executed:
#!/bin/bash
# View NotificationsRetrievalWorkerManager to understand periodic setup
cat app/src/org/commcare/pn/workermanager/NotificationsRetrievalWorkerManager.ktLength of output: 2268
🏁 Script executed:
#!/bin/bash
# Check if there's any logic that deduplicates work requests across the codebase
rg -n 'NotificationsRetrievalWorker' app/src/org/commcare/pn/workermanager/ -A 3 -B 3Length of output: 3985
🏁 Script executed:
#!/bin/bash
# Find NotificationsRetrievalWorker in the workers package
find app/src -name 'NotificationsRetrievalWorker.kt' -type fLength of output: 63
🏁 Script executed:
#!/bin/bash
# If found, view the implementation
find app/src -name 'NotificationsRetrievalWorker.kt' -type f -exec cat {} \;Length of output: 1593
🏁 Script executed:
#!/bin/bash
# Check the full flow: see how many times startWorkRequest is called for a single PN action
sed -n '74,110p' app/src/org/commcare/pn/workermanager/PNApiSyncWorkerManager.ktLength of output: 1007
Multiple concurrent notification retrievals will be triggered for multi-sync actions.
When a push notification triggers multiple sync actions (e.g., CCC_DEST_PAYMENTS triggers both opportunities and delivery sync), each sync work independently chains a notification retrieval request. These chains execute concurrently with different work names, bypassing the KEEP deduplication policy. For example:
CCC_DEST_PAYMENTS→ 2 notification retrievals run in parallelCCC_DEST_LEARN_PROGRESS→ 2 notification retrievals run in parallel
Combined with the hourly periodic retrieval, this results in redundant and potentially concurrent API calls with no deduplication mechanism.
Recommended fix: Use a single, uniquely-named notification retrieval work that executes after all sync operations complete, rather than chaining a retrieval after each individual sync.
There was a problem hiding this comment.
tackled it by not chaining but scheduling the worker from another worker with replace work policy - ed40a8c
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the commit diff to see how the issue was addressed
cd /tmp
git clone --quiet https://github.com/dimagi/commcare-android.git repo 2>&1 | head -5
cd repo
git fetch origin pull/3376/head:pr-3376 2>&1 | head -5
git checkout pr-3376 2>&1 | head -5
git show ed40a8c1a7bc1fa544a6ca9053e6558c1e9f01e9
Length of output: 6681
@shubham1g5, excellent solution! 👍
The fix effectively addresses the concurrent notification retrieval issue. By scheduling the one-time notification retrieval from within PNApiSyncWorker.processAfterSuccessfulSync() and using ExistingWorkPolicy.REPLACE with a unique work name, you've created a natural deduplication mechanism.
Key benefits of this approach:
- Deduplication: Multiple sync workers completing concurrently will all attempt to schedule notification retrieval, but the REPLACE policy ensures only one runs
- Clean separation: The periodic retrieval (hourly) and one-time retrieval (post-sync) use different unique work names, so they don't interfere with each other
- Simpler code flow: Removes the chaining complexity from the manager class
This is much cleaner than chaining and naturally handles the edge cases where multiple sync actions are triggered by a single push notification.
🧬 Code Graph Analysis Results
[PNApiSyncWorkerManager.kt: startWorkRequest method, approx. lines 154-197]
Full snippet:
fun startWorkRequest(pn: Map<String,String>, action: SYNC_ACTION, uniqueWorkName:String) {
val pnJsonString = Gson().toJson(pn)
val syncActionString = Gson().toJson(action)
val syncTypeString = Gson().toJson(syncType)
val inputData = Data.Builder()
.putString(PN_DATA, pnJsonString)
.putString(ACTION, syncActionString)
.putString(PNApiSyncWorker.SYNC_TYPE, syncTypeString)
.build()
val syncWorkRequest = OneTimeWorkRequestBuilder<PNApiSyncWorker>()
.setInputData(inputData)
.setConstraints(Constraints.Builder().setRequiredNetworkType(NetworkType.CONNECTED).build())
.setBackoffCriteria(
androidx.work.BackoffPolicy.EXPONENTIAL,
PN_SYNC_BACKOFF_DELAY_IN_MILLIS,
TimeUnit.MILLISECONDS
).build()
WorkManager.getInstance(context).enqueueUniqueWork(
uniqueWorkName,
ExistingWorkPolicy.KEEP,
syncWorkRequest
)
signaling=true
}
shubham1g5
force-pushed
the
syncNotifications
branch
from
c92fc33 to
ed40a8c
Compare
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
conroy-ricketts
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
These changes look good to me. But, I think it would be good to have a second set of eyes on this for approval
There was a problem hiding this comment.
The comments and documentation here are great!
| public void completeSignin() { | ||
| personalIdSatus = PersonalIdStatus.LoggedIn; | ||
| scheduleHeartbeat(); | ||
| NotificationsRetrievalWorkerManager.INSTANCE.schedulePeriodicPushNotificationRetrieval(CommCareApplication.instance()); |
There was a problem hiding this comment.
Wondering if we want this to start on app startup when PersonalID is configured? That way the user could potentially learn about new messages/notifications before starting some other activity, via the indicators on the related menu items.
From a security perspective, making the call before unlock will only result in the user seeing that new data is available, but they'll still need to unlock to view that data (whether a notification or message). Also, once the user unlocks once we'll continue doing this periodic background check even if they return to the Login page later (or somehow perform a similar "logout" that then requires them to unlock again).
|
|
||
| private const val PUSH_NOTIFICATION_RETRIEVAL_WORKER_TAG = "push_notification_retrieval_worker" | ||
| private const val PERIODICITY_FOR_RETRIEVAL_IN_HOURS = 1L | ||
| private const val BACKOFF_DELAY_FOR_RETRIEVAL_RETRY = 10 * 60 * 1000L // 10 minutes |
There was a problem hiding this comment.
Nit: Curious why specify retrieval in hours but backoff in milliseconds?
I'd advocate for:
BACKOFF_DELAY_FOR_RETRIEVAL_RETRY_IN_MINUTES = 10
Then we avoid the math and comment
| return@withContext try { | ||
| val result = PushNotificationApiHelper.retrieveLatestPushNotifications(appContext) | ||
|
|
||
| if (result.isSuccess) { | ||
| val notifications = result.getOrNull() | ||
| Logger.log("PushNotificationRetrievalWorker", | ||
| "Successfully retrieved ${notifications?.size ?: 0} push notifications") | ||
| Result.success() | ||
| } else { | ||
| val exception = result.exceptionOrNull() | ||
| Logger.exception("PushNotificationRetrievalWorker failed", exception ?: Exception("Unknown error")) | ||
|
|
||
| if (runAttemptCount < MAX_RETRIES) { | ||
| Result.retry() | ||
| } else { | ||
| Result.failure() | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Logger.exception("PushNotificationRetrievalWorker encountered unexpected error", e) | ||
| Result.failure() | ||
| } |
There was a problem hiding this comment.
@shubham1g5 Can we replace with ?
result.onSuccess { }.onFailure{ }
There was a problem hiding this comment.
Do you see any benefits of that other than stylistic improvement, also these functions don't return the Result values - they return the original result object instead, so we will need to do additional result retention in a variable if we want to use that.
There was a problem hiding this comment.
@shubham1g5 Ok, I see your point. I was just confused with try - catch, do we need that?
There was a problem hiding this comment.
I added it to not trigger a app crash for something that's happening on background, I would imagine it would be pretty bad behaviour given this task happens every hour and if every hour user get a message saying "CommCare stopped"
|
|
||
| private fun processAfterSuccessfulSync(){ | ||
| raiseFCMPushNotificationIfApplicable() | ||
| NotificationsRetrievalWorkerManager.scheduleImmediateNotificationRetrieval(appContext) |
There was a problem hiding this comment.
@shubham1g5 This will schedule multiple worker when having multiple Sync workers from notification api. Also, it's not required to start the thread when calling from notification api as personalId login will have already started this.
Instead new PNApiSyncWorker with new SYNC_ACTION as SYNC_NOTIFICATIONS should be started for from PNApiSyncWorkerManager when it's SYNC_TYPE is FCM . This should be in addition to other type of sync i.e. always present if FCM is for signalling.
There was a problem hiding this comment.
I am not sure I fully understand that. Are you asking to do this after starting the PNApiSyncWorker ? That was my original approach as well but the multiple workers issue was still there (look at this code rabbit comment in response to which I moved it here)
this will schedule multiple worker when having multiple Sync workers from notification api.
this should be taken care by ExistingWorkPolicy on the NotificationsRetrievalWorker as well.
There was a problem hiding this comment.
@shubham1g5 I mean here to start the notification sync worker always in parallel with other sync(s) as generated by current code. That being said PNApiSyncWorkerManager will always start sync worker with new SYNC_ACTION as SYNC_NOTIFICATIONS if SYNC_TYPE is FCM
Note: This same PNApiSyncWorkerManager and thus PNApiSyncWorker is called whenever app receives PNs through notification API also and we don't want to start NotificationRetrievalWorker in this case.
There was a problem hiding this comment.
I mean here to start the notification sync worker always in parallel with other sync(s) as generated by current code.
Curious what benefit would that have in comparison to scheduling notification fetch after fcm sync ? I don't want to start a lot of parallel threads (considering low end devices) unless it solves a specific use case.
There was a problem hiding this comment.
@shubham1g5 I was thinking 2 points
- Short time to live before application gets killed
- As mentioned in above point:
Note: This same PNApiSyncWorkerManager and thus PNApiSyncWorker is called whenever app receives PNs through notification API also and we don't want to start NotificationRetrievalWorker in this case.
| * Schedules immediate notification retrieval as a one time task | ||
| * @param context Application context | ||
| */ | ||
| fun scheduleImmediateNotificationRetrieval(context: Context) { |
There was a problem hiding this comment.
@shubham1g5 I think this should be called on every app start if personalId login?
|
|
||
| @Config(application = CommCareTestApplication::class) | ||
| @RunWith(AndroidJUnit4::class) | ||
| class NotificationsRetrievalWorkerTest { |
|
@shubham1g5 Wrongly linked ticket, should be https://dimagi.atlassian.net/browse/CCCT-1805 |
…e-android into syncNotifications
…g multiple notiifcations fetches
…rkers Periodic Notifications Sync
Product Description
https://dimagi.atlassian.net/browse/CCCT-1805
Technical Summary
Adds a worker task to fetch notifications from
retrieve_notificationsendpoint and schedule it a periodic frequency of an hour after personal ID sign in and once after the fcm sync.Safety Assurance
Safety story
Will be QA'ed + should only affect background processing
QA Plan
Part of https://dimagi.atlassian.net/browse/QA-8180
Labels and Review