Fix bug in integrity reporting #3309
Conversation
…roid Copy of SQLCipher migration (android-database-sqlcipher -> sqlcipher-android)
…nto commcare_2.59
Backmerge Commcare 2.59
Changed app version to 2.60 in manifest
Solved crash while showing drawer in delivery / learning app
…oring-trace Add perf monitoring trace for case list loading time
📝 WalkthroughWalkthroughThe change modifies IntegrityTokenApiRequestHelper.fetchIntegrityToken to ensure observer setup and cleanup run on the Main thread. It introduces coroutine usage by wrapping the observation registration and cancellation within runBlocking and withContext(Dispatchers.Main). No public APIs or signatures are altered. The rest of the logic remains intact, focusing solely on thread-affinity for the provider-state observation and coroutine interaction. Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant H as IntegrityTokenApiRequestHelper
participant M as Main Dispatcher
participant P as Provider State Observer
participant G as Integrity API
C->>H: fetchIntegrityToken()
activate H
note over H: runBlocking
H->>M: withContext(Dispatchers.Main)
activate M
M->>P: Register observer
M-->>G: Initiate token fetch
G-->>M: Token / Status callback
M->>P: Unregister observer
deactivate M
H-->>C: Return token/result
deactivate H
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/android/integrity/IntegrityTokenApiRequestHelper.kt (2)
162-167: Avoid runBlocking inside a suspend; marshal to Main with withContext instead.runBlocking inside suspendCancellableCoroutine blocks the caller thread and is easy to misuse. Prefer switching the enclosing context to Main and registering the observer synchronously without blocking the caller thread.
Suggested shape:
// Replace the body of fetchIntegrityToken with: suspend fun fetchIntegrityToken(requestHash: String): Result<Pair<String, String>> = withContext(Dispatchers.Main.immediate) { val viewModel = CommCareViewModelProvider.getIntegrityTokenViewModel() suspendCancellableCoroutine { cont -> fun requestToken() { try { viewModel.requestIntegrityToken( requestHash, false, object : IntegrityTokenViewModel.IntegrityTokenCallback { override fun onTokenReceived( requestHash: String, integrityTokenResponse: StandardIntegrityManager.StandardIntegrityToken ) { if (cont.isActive) cont.resume(Result.success(integrityTokenResponse.token() to requestHash)) } override fun onTokenFailure(exception: Exception) { if (cont.isActive) cont.resume(Result.failure(exception)) } } ) } catch (e: Exception) { if (cont.isActive) cont.resume(Result.failure(e)) } } val observer = object : Observer<IntegrityTokenViewModel.TokenProviderState> { override fun onChanged(state: IntegrityTokenViewModel.TokenProviderState) { when (state) { is IntegrityTokenViewModel.TokenProviderState.Success -> { viewModel.providerState.removeObserver(this) requestToken() } is IntegrityTokenViewModel.TokenProviderState.Failure -> { viewModel.providerState.removeObserver(this) if (cont.isActive) cont.resume(Result.failure(state.exception)) } } } } viewModel.providerState.observeForever(observer) cont.invokeOnCancellation { viewModel.providerState.removeObserver(observer) } } }Benefits:
- No blocking of the caller thread.
- Observer registration/removal guaranteed on Main (immediate).
- Defensive checks against double resume.
163-163: Minor: prefer Dispatchers.Main.immediate to avoid an extra dispatch hop.This executes inline when already on Main, reducing latency and flakiness in tight races.
- withContext(Dispatchers.Main) { + withContext(Dispatchers.Main.immediate) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt(2 hunks)
🔇 Additional comments (1)
app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt (1)
9-9: LGTM on coroutine imports.The added imports are appropriate for marshaling work to the Main thread.
Also applies to: 17-17
| kotlinx.coroutines.runBlocking { | ||
| withContext(Dispatchers.Main) { | ||
| viewModel.providerState.observeForever(observer) | ||
| cont.invokeOnCancellation { viewModel.providerState.removeObserver(observer) } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix: ensure removeObserver runs on the Main thread in the cancellation path.
LiveData enforces main-thread affinity for observeForever/removeObserver. cont.invokeOnCancellation may execute on a background thread, risking the same IllegalStateException you’re fixing. Post the removal to Main (or gate by Looper) inside the cancellation callback.
Reproduce by cancelling the coroutine immediately after calling fetchIntegrityToken from a background dispatcher; without this change, you can hit the IllegalStateException intermittently.
Apply this diff within the changed block:
- kotlinx.coroutines.runBlocking {
- withContext(Dispatchers.Main) {
- viewModel.providerState.observeForever(observer)
- cont.invokeOnCancellation { viewModel.providerState.removeObserver(observer) }
- }
- }
+ kotlinx.coroutines.runBlocking {
+ withContext(Dispatchers.Main) {
+ viewModel.providerState.observeForever(observer)
+ cont.invokeOnCancellation {
+ if (android.os.Looper.myLooper() == android.os.Looper.getMainLooper()) {
+ viewModel.providerState.removeObserver(observer)
+ } else {
+ android.os.Handler(android.os.Looper.getMainLooper()).post {
+ viewModel.providerState.removeObserver(observer)
+ }
+ }
+ }
+ }
+ }🏁 Script executed:
#!/bin/bash
# Locate the IntegrityTokenApiRequestHelper.kt file
file=$(fd IntegrityTokenApiRequestHelper.kt)
echo "Inspecting: $file"
# Show where suspendCancellableCoroutine is used
grep -n "suspendCancellableCoroutine" "$file"
# Show where cont.invokeOnCancellation is registered
grep -n "cont.invokeOnCancellation" "$file"
# Display lines around the observer registration and cancellation (approx. lines 150-180)
sed -n '150,180p' "$file"Length of output: 1784
Ensure removeObserver Always Runs on the Main Thread in the Cancellation Handler
LiveData’s observeForever/removeObserver require main-thread affinity. Currently, the cancellation callback registered via cont.invokeOnCancellation may execute off-Main, leading to intermittent IllegalStateException.
Affected location:
- File:
app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt - Inside
suspend fun fetchIntegrityToken(around line 165)
Proposed fix: wrap the removal in a main-thread check/post. Don’t forget to add imports for android.os.Handler and android.os.Looper if they’re not already present.
kotlinx.coroutines.runBlocking {
withContext(Dispatchers.Main) {
viewModel.providerState.observeForever(observer)
cont.invokeOnCancellation {
- viewModel.providerState.removeObserver(observer)
+ if (android.os.Looper.myLooper() == android.os.Looper.getMainLooper()) {
+ viewModel.providerState.removeObserver(observer)
+ } else {
+ android.os.Handler(android.os.Looper.getMainLooper()).post {
+ viewModel.providerState.removeObserver(observer)
+ }
+ }
}
}
}How to reproduce: cancel the coroutine immediately after calling fetchIntegrityToken from a non-Main dispatcher. Without this guard, you’ll intermittently hit an IllegalStateException.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kotlinx.coroutines.runBlocking { | |
| withContext(Dispatchers.Main) { | |
| viewModel.providerState.observeForever(observer) | |
| cont.invokeOnCancellation { viewModel.providerState.removeObserver(observer) } | |
| } | |
| } | |
| kotlinx.coroutines.runBlocking { | |
| withContext(Dispatchers.Main) { | |
| viewModel.providerState.observeForever(observer) | |
| cont.invokeOnCancellation { | |
| if (android.os.Looper.myLooper() == android.os.Looper.getMainLooper()) { | |
| viewModel.providerState.removeObserver(observer) | |
| } else { | |
| android.os.Handler(android.os.Looper.getMainLooper()).post { | |
| viewModel.providerState.removeObserver(observer) | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt
around lines 162 to 167, the cancellation handler registered with
cont.invokeOnCancellation calls viewModel.providerState.removeObserver
potentially off the Main thread; change the cancellation callback so it always
runs removeObserver on the Main thread by checking Looper.myLooper() ==
Looper.getMainLooper() and calling removeObserver directly if true, otherwise
post a Runnable to a new Handler(Looper.getMainLooper()) that calls
removeObserver; add imports for android.os.Handler and android.os.Looper if not
present.
| } | ||
| viewModel.providerState.observeForever(observer) | ||
| cont.invokeOnCancellation { viewModel.providerState.removeObserver(observer) } | ||
| kotlinx.coroutines.runBlocking { |
There was a problem hiding this comment.
no need to use runBlocking as we are inside a suspend method
There was a problem hiding this comment.
@OrangeAndGreen I am prettry concerned about this code as it seems that we are trying to force main thread execution from a work manager task and I am not really sure of the repurcussions of doing that. IMO, we should not need rely on observeForever call if that require us to switch the execution to main thread.
There is another Kotlin component called Flow using which you might simplify things here - viewModel.providerState.asFlow()
|
Ah, okay. This is definitely new territory for me with Kotlin so I was going off guidance from AI and the suggested fix indeed fixed the issue. I'm a little hesitant to try the same strategy to solve this using Flow, but it's also new to me so the alternative will be to spend some time learning more about it properly. |
https://dimagi.atlassian.net/browse/CCCT-1658
Product Description
No user visible changes.
Technical Summary
Fixes a crash that was coming out of the integrity reporting code that gets triggered by a flag in the HQ heartbeat.
The code was trying to observer a LiveData object while waiting to retrieve a token.
But calling observeForever from a background thread caused an IllegalStateException.
I wrapped the crashing code to marshal the observer back to the main thread, and confirmed the crash no longer happens.
Feature Flag
HQ Heartbeat, Integrity Reporting
Safety Assurance
Safety story
Forced a test locally first to observe the crash, then to confirm it didn't happen after the fix.
Automated test coverage
None
QA Plan
None