Improved Connect DB error handling #3348
Conversation
…und in global DB. Crashing (with log) when attempting to create Connect DB without encryption (no passphrase)
…enever an error is encountered opening any of the databases.
Removed redundant non-fatal exceptions just before crashing DB with exception.
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna and
shubham1g5
📝 WalkthroughWalkthroughThis PR revises Connect DB error handling and logging. It adds two GlobalErrors constants for PersonalID DB startup/upgrade. ConnectDatabaseHelper removes crashDb() and requires crashDb(int errorCode), updates crash paths to use explicit GlobalErrors, and throws if passphrase is null. ConnectDatabaseUtils returns null when no passphrase record exists. PersonalIdManager passes a specific GlobalErrors code on DB startup failures. App/global/user/connect DatabaseOpenHelpers now log exceptions when opening the DB fails before attempting SQLCipher upgrade and retry. Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant PIM as PersonalIdManager
participant CDH as ConnectDatabaseHelper
participant GErr as GlobalErrors
App->>PIM: init(context)
PIM->>CDH: open/connect DB
alt DB broken at startup
PIM->>CDH: crashDb(GErr.PERSONALID_DB_STARTUP_ERROR)
note right of CDH: Throws RuntimeException with error name
else DB ok
PIM-->>App: continue
end
%% New: explicit error code on crash path
sequenceDiagram
autonumber
participant Helper as ConnectDatabaseHelper
participant Utils as ConnectDatabaseUtils
participant GErr as GlobalErrors
Helper->>Utils: getConnectDbPassphrase()
alt No key record
Utils-->>Helper: null
Helper->>Helper: validate passphrase
Helper-->>Helper: throw IllegalStateException
else Key exists
Utils-->>Helper: decrypted passphrase
Helper-->>Helper: proceed to open DB
end
alt DB upgrade failure
Helper->>Helper: crashDb(GErr.PERSONALID_DB_UPGRADE_ERROR)
else Generic open failure
Helper->>Helper: crashDb(GErr.PERSONALID_GENERIC_ERROR)
end
%% New: null handling + explicit crash codes
sequenceDiagram
autonumber
participant OpenHelper as *Database*OpenHelper (App/User/Global/Connect)
participant Logger as Logger
participant DbUtil as DbUtil
OpenHelper->>OpenHelper: getWritableDatabase()
alt SQLiteException on open
OpenHelper->>Logger: exception("Opening database failed", sqle)
OpenHelper->>DbUtil: trySqlCipherDbUpdate(...)
OpenHelper->>OpenHelper: retry getWritableDatabase()
alt Retry fails
OpenHelper-->>Caller: rethrow exception (with prior log)
else Retry succeeds
OpenHelper-->>Caller: DB handle
end
else Open success
OpenHelper-->>Caller: DB handle
end
%% New: logging added before existing retry flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 2
🧹 Nitpick comments (6)
app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (1)
78-89: Good change: return null instead of noisy crash when no key record.This aligns with PR goals and avoids false alarms. Ensure all callers treat null as “no passphrase configured” (looks covered by the new guard in ConnectDatabaseHelper.getHandle()).
Consider annotating return types as @nullable for getConnectDbPassphrase/getConnectDbEncodedPassphrase and updating Javadoc accordingly.
app/src/org/commcare/models/database/user/DatabaseUserOpenHelper.java (1)
52-55: Make the log more actionable by including DB identity.- Logger.exception("Opening database failed", sqle); + Logger.exception("Opening user database failed: " + getDbName(mUserId), sqle);app/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java (1)
43-46: Clarify which DB failed in logs.- Logger.exception("Opening database failed", sqle); + Logger.exception("Opening global database failed: " + GLOBAL_DB_LOCATOR, sqle);app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
161-169: Clarify log context for easier triage.- Logger.exception("Opening database failed", sqle); + Logger.exception("Opening connect database failed: " + CONNECT_DB_LOCATOR, sqle);app/src/org/commcare/models/database/app/DatabaseAppOpenHelper.java (1)
48-51: Include app DB identity in the failure log.- Logger.exception("Opening database failed", sqle); + Logger.exception("Opening app database failed: " + getDbName(mAppId), sqle);app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
93-97: Log the exception before crashing to retain stack context.- } catch (Exception e) { + } catch (Exception e) { //Flag the DB as broken if we hit an error opening it (usually means corrupted or bad encryption) dbBroken = true; + Logger.exception("Connect DB open failure", e); crashDb(GlobalErrors.PERSONALID_GENERIC_ERROR); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/org/commcare/connect/PersonalIdManager.java(2 hunks)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java(4 hunks)app/src/org/commcare/connect/database/ConnectDatabaseUtils.java(1 hunks)app/src/org/commcare/models/database/app/DatabaseAppOpenHelper.java(2 hunks)app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java(2 hunks)app/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java(2 hunks)app/src/org/commcare/models/database/user/DatabaseUserOpenHelper.java(2 hunks)app/src/org/commcare/utils/GlobalErrors.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
📚 Learning: 2025-06-06T19:52:53.173Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java:163-180
Timestamp: 2025-06-06T19:52:53.173Z
Learning: In the CommCare Android Connect feature, database operations like ConnectJobUtils.upsertJob should be allowed to crash rather than being wrapped in try-catch blocks. The team prefers fail-fast behavior for database errors instead of graceful error handling.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.javaapp/src/org/commcare/connect/PersonalIdManager.javaapp/src/org/commcare/connect/database/ConnectDatabaseHelper.java
🧬 Code graph analysis (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
ConnectDatabaseHelper(37-134)
⏰ 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 (4)
app/src/org/commcare/connect/PersonalIdManager.java (1)
128-129: Explicit startup crash code: LGTM.Switching to PERSONALID_DB_STARTUP_ERROR is consistent with the PR’s explicit error taxonomy.
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (3)
55-56: Upgrade failure path: LGTM.Crashing with PERSONALID_DB_UPGRADE_ERROR makes the source explicit.
75-77: Good guard against opening an unencrypted DB.Preventing null passphrase access closes the “open unencrypted DB” hole.
88-91: Legacy path logging: LGTM.The simplified message is fine and preserves signal without leaking secrets.
| try { | ||
| return super.getWritableDatabase(); | ||
| } catch (SQLiteException sqle) { | ||
| Logger.exception("Opening database failed", sqle); |
There was a problem hiding this comment.
nit: it might be useful to know what database is failing here from the messag like "Opening App database failed"
There was a problem hiding this comment.
I think we'll be able to see that pretty easily from the stack trace, no? Like whether it comes from DatabaseAppOpenHelper, DatabaseConnectOpenHelper, etc.
There was a problem hiding this comment.
I am concerned that Crashlytics will club them together into one single issue or not as it might be valuable to know if this issue is happening for all Dbs or just one by getting the issue count. Although not sure whats the behaviour here on Crashlytics really, so fine to ignore it atm
| //Store the received passphrase as what's in use locally | ||
| ConnectDatabaseUtils.storeConnectDbPassphrase(context, remotePassphrase, true); | ||
| } catch (Exception e) { | ||
| Logger.exception("Handling received DB passphrase", e); |
There was a problem hiding this comment.
curious why are not we logging the underlying exceptions now for crashDb calls.
There was a problem hiding this comment.
Ah, interesting. I removed them because they were essentially just duplicates of the fatal exception that gets logged immediately after when the app crashes, but didn't think about the lost crash-causing exception. I'll pass it through so the crashing exception can wrap it and we can preserve the full stack while avoiding duplicate events.
There was a problem hiding this comment.
Also something worth noting: I checked back 90 days and neither of the two related non-fatal exceptions had been logged a single time
There was a problem hiding this comment.
neither of the two related non-fatal exceptions
Confused exactly which two non-fatals you are mentioning here.
Product Description
No visible user changes
Technical Summary
Removed ambiguous logging about not finding DB passphrase (often false-alarms)
Removed redundant non-fatal exceptions just before DB crashes (which cause fatal exceptions)
Added a new check and crash to prevent unencrypted Connect DB
Added non-fatal exception when error encountered opening any DB (before trySqlCipherDbUpdate)
Improved exception logging when crashing Connect DB to indicate source
Feature Flag
PersonalID
Safety Assurance
Safety story
Should be no noticeable changes to user, but simply reduced/improved logging when errors occur.
The new crash before opening an unencrypted DB is the biggest risk and could cause users to crash on upgrade.
If so, they would simply be prompted to reconfigure PersonalID when restarting the app.
Automated test coverage
None
QA Plan
Basic regression testing around PersonalID configuration and de-configuration, ensure no errors