Fix repeated crash at app startup on corrupt Connect DB. #3411
Conversation
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
… when the app restarts (to avoid crashing again repeatedly at app startup). Also added an empty check to the passphrase when opening handle to Connect DB.
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThe changes relocate user forget logic from GlobalErrorUtil to ConnectDatabaseHelper. Previously, Global error handling would conditionally invoke PersonalIdManager.forgetUser() on certain errors. Now, the forgetUser call is triggered in ConnectDatabaseHelper when a crash occurs on the Connect database. Additionally, the passphrase validation in ConnectDatabaseHelper is tightened to treat empty passphrases (length 0) the same as null, throwing an IllegalStateException if Connect DB access is attempted without a valid passphrase. Sequence DiagramsequenceDiagram
participant App as App/Caller
participant CDB as ConnectDatabaseHelper
participant PIM as PersonalIdManager
participant GEU as GlobalErrorUtil
rect rgb(240, 248, 255)
Note over CDB,PIM: New Flow (Post-Change)
App->>CDB: DB operation / crash
alt Passphrase is null or empty
CDB->>CDB: Throw IllegalStateException
else Crash with error
CDB->>PIM: forgetUser(PERSONAL_ID_FORGOT_USER_DB_ERROR)
PIM->>PIM: Clear user data
CDB->>CDB: Throw RuntimeException
end
end
rect rgb(255, 240, 240)
Note over GEU: Old Flow (Pre-Change)
App->>GEU: Global error handling
GEU->>GEU: Conditionally check deleteConnectStorage flag
opt deleteConnectStorage true
GEU->>PIM: forgetUser(...)
end
GEU->>GEU: Handle error (no-op for forgetUser now)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-06-06T19:52:53.173ZApplied to files:
📚 Learning: 2025-03-10T08:16:29.416ZApplied to files:
📚 Learning: 2025-05-08T11:08:18.530ZApplied to files:
📚 Learning: 2025-01-21T17:28:09.007ZApplied to files:
📚 Learning: 2025-06-06T19:54:26.428ZApplied to files:
📚 Learning: 2025-06-13T15:53:12.951ZApplied to files:
🧬 Code graph analysis (1)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (2)
⏰ 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)
🔇 Additional comments (3)
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 |
| @@ -15,7 +13,6 @@ public static void addError(GlobalErrorRecord error) { | |||
|
|
|||
| public static String handleGlobalErrors() { | |||
Contributor
There was a problem hiding this comment.
@OrangeAndGreen Changes look good, only minor nit: Rename this to getGlobalErrorsIfAny as it's not handling now anything?
Contributor
Author
There was a problem hiding this comment.
Ah, makes sense. I went with just getGlobalErrors: be2aeb7
Jignesh-dimagi
approved these changes
Nov 11, 2025
Jignesh-dimagi
approved these changes
Nov 11, 2025
Contributor
Author
|
@damagatchi Retest this please |
Contributor
Author
|
@damagatchi Retest this please |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://dimagi.atlassian.net/browse/CCCT-1868
Product Description
Fixes a repeated crash at app startup after an initial crash due to a corrupt Connect DB.
Note this PR does not fix the original crash (i.e. why the DB got corrupted).
It just makes sure the app only crashes once when that happens.
Technical Summary
Deleting Connect DB immediately when crash DB is called, so it's gone when the app restarts (to avoid crashing again repeatedly at app startup).
Also added an empty check to the passphrase when opening handle to Connect DB. If that were the case it would explain a corrupted DB error, so this could help determine how the app gets into this state to begin with.
Feature Flag
PersonalID
Safety Assurance
Safety story
I tested by starting with a clean PersonalID configuration, then temporarily overriding the passphrase to null in the code in order to cause the app to detect the DB as corrupt. Before applying the fix, I could observe the app crashing repeatedly on startup. After the fix, the app only crashes once, and when restarted the user sees PersonalID de-configured and a message explaining that something went wrong and they will need to re-configure PersonalID.
Automated test coverage
None
QA Plan
This will be difficult/impossible for QA to test since we're not sure how to cause the corrupt DB scenario that opens up this second bug.