QA-8627 Don't Write Connect Data After Signing Out Of PersonalID (2/2: make Connect storage safe to reach around sign-out) - #3860
Draft
OrangeAndGreen wants to merge 1 commit into
Conversation
Contributor
Author
Suggested Review Order
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## QA-8627-gate-connect-writes-when-signed-out #3860 +/- ##
==============================================================================
Coverage ? 27.36%
Complexity ? 4793
==============================================================================
Files ? 989
Lines ? 59008
Branches ? 7044
==============================================================================
Hits ? 16145
Misses ? 40909
Partials ? 1954 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
OrangeAndGreen
force-pushed
the
QA-8627-stale-connect-db-write-after-signout
branch
from
August 10, 2026 16:02
3a6a5d2 to
03821a6
Compare
Contributor
📝 WalkthroughWalkthroughPersonalID sign-out now updates account state before cleanup, cancels related work, and clears Connect credentials and database state. Connect database access distinguishes unavailable storage from broken storage. Release toggles, notifications, push handling, and messaging key synchronization now require an active account. Tests cover cleanup, concurrent access, toggle persistence, and notification navigation. Release notes add sign-out and re-sign-in QA checks. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PersonalIdManager
participant ConnectDatabaseHelper
participant PushNotificationApiHelper
participant MessagingChannelsKeySyncWorker
PersonalIdManager->>PersonalIdManager: mark account absent
PersonalIdManager->>ConnectDatabaseHelper: clear Connect data
ConnectDatabaseHelper-->>PersonalIdManager: reset database state
PushNotificationApiHelper->>PersonalIdManager: check account presence
PersonalIdManager-->>PushNotificationApiHelper: account absent or active
PushNotificationApiHelper->>PushNotificationApiHelper: store or discard notification
MessagingChannelsKeySyncWorker->>PersonalIdManager: check account presence
PersonalIdManager-->>MessagingChannelsKeySyncWorker: account absent or active
MessagingChannelsKeySyncWorker->>MessagingChannelsKeySyncWorker: skip or perform key sync
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/org/commcare/connect/database/PersonalIdNotConfiguredException.java`:
- Around line 13-17: Rewrite the new PersonalIdNotConfiguredException class in a
Kotlin source file while preserving its public class name, IllegalStateException
inheritance, and message-taking constructor so existing Java callers can import
and use it unchanged.
In `@app/src/org/commcare/connect/PersonalIdManager.java`:
- Around line 197-208: Introduce one shared lifecycle lock or guarded-storage
operation and use it consistently: in
app/src/main/java/org/commcare/connect/PersonalIdManager.java:197-208, acquire
exclusive access before changing personalIdSatus and deleting the database; in
app/src/main/java/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.kt:20-24,
guard the storage write; in
app/src/main/java/org/commcare/pn/workers/NotificationsSyncWorker.kt:231-239,
keep the account check and notification-state read within one shared operation;
and in app/src/main/java/org/commcare/utils/PushNotificationApiHelper.kt:82-121,
acquire access before getUser and retain it through response storage and
acknowledgment. Add a latch-based test that exercises sign-out between the
account check and storage access.
In `@app/src/org/commcare/utils/PushNotificationApiHelper.kt`:
- Around line 110-121: Update the notification processing block around
acknowledgeNotificationsReceipt so a false acknowledgment result throws or
otherwise propagates as a failure within runCatching. Preserve the existing
savedNotifications return for successful acknowledgments, allowing the
continuation to resume with failure and NotificationsSyncWorker to retry when
acknowledgment is rejected.
In `@RELEASES.md`:
- Line 18: Update the release note wording by replacing “afterwards” with the
American English form “afterward,” preserving the rest of the signing,
notifications, and messaging guidance.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19c05109-2b7b-4054-86a5-e4e33457a24e
📒 Files selected for processing (9)
RELEASES.mdapp/src/org/commcare/CommCareApplication.javaapp/src/org/commcare/connect/PersonalIdManager.javaapp/src/org/commcare/connect/database/ConnectDatabaseHelper.javaapp/src/org/commcare/connect/database/PersonalIdNotConfiguredException.javaapp/src/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.ktapp/src/org/commcare/pn/workers/NotificationsSyncWorker.ktapp/src/org/commcare/utils/PushNotificationApiHelper.ktapp/unit-tests/src/org/commcare/connect/network/connectId/parser/ConnectReleaseTogglesParserTest.kt
Comment on lines
197
to
+208
| personalIdSatus = PersonalIdStatus.NotIntroduced; | ||
|
|
||
| // Cancel periodic push notification retrieval when user logs out | ||
| NotificationsSyncWorkerManager.cancelPeriodicPushNotificationRetrieval(CommCareApplication.instance()); | ||
|
|
||
| ConnectReleaseTogglesWorker.Companion.cancelPeriodicFetch(CommCareApplication.instance()); | ||
|
|
||
| //Drop in-flight API requests, which are held in an app-wide scope and would otherwise | ||
| //write stale data from the previous session | ||
| ConnectRequestManager.INSTANCE.cancelAll(); | ||
|
|
||
| ConnectUserDatabaseUtil.forgetUser(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make account checks and Connect storage deletion atomic.
volatile makes personalIdSatus visible. It does not serialize hasAccount() with forgetUser(). A worker can pass an account guard, then sign-out can set NotIntroduced and delete the database, then the worker can read or write Connect storage. In PushNotificationApiHelper, ConnectUserDatabaseUtil.getUser(context) also occurs before the new guard.
Use one shared lifecycle lock or a central guarded-storage operation. Hold shared access across each Connect storage operation. Hold exclusive access in forgetUser() before deleting storage. Add a latch-based test that forces sign-out between the check and storage access.
app/src/org/commcare/connect/PersonalIdManager.java#L197-L208: acquire exclusive lifecycle access before status change and database deletion.app/src/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.kt#L20-L24: perform the storage write in the guarded lifecycle operation.app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt#L231-L239: protect the account check and notification-state read as one operation.app/src/org/commcare/utils/PushNotificationApiHelper.kt#L82-L121: acquire guarded access before reading the user and hold it through response storage and acknowledgment.
📍 Affects 4 files
app/src/org/commcare/connect/PersonalIdManager.java#L197-L208(this comment)app/src/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.kt#L20-L24app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt#L231-L239app/src/org/commcare/utils/PushNotificationApiHelper.kt#L82-L121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/org/commcare/connect/PersonalIdManager.java` around lines 197 - 208,
Introduce one shared lifecycle lock or guarded-storage operation and use it
consistently: in
app/src/main/java/org/commcare/connect/PersonalIdManager.java:197-208, acquire
exclusive access before changing personalIdSatus and deleting the database; in
app/src/main/java/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.kt:20-24,
guard the storage write; in
app/src/main/java/org/commcare/pn/workers/NotificationsSyncWorker.kt:231-239,
keep the account check and notification-state read within one shared operation;
and in app/src/main/java/org/commcare/utils/PushNotificationApiHelper.kt:82-121,
acquire access before getUser and retain it through response storage and
acknowledgment. Add a latch-based test that exercises sign-out between the
account check and storage access.
Contributor
Author
There was a problem hiding this comment.
The observation is correct — volatile gives visibility, not serialization, and the caller-side isloggedIn() guards are check-then-act. The getUser(context) call in PushNotificationApiHelper is also outside the guard, as noted.
The consequence is addressed at the source rather than by threading a lifecycle lock through the callers (that work is in an incoming push, so it isn't visible in the commit under review):
ConnectDatabaseHelper.clearConnectData()now holdsconnectDbHandleLockacross teardown,deleteDb(), and key removal — the same lockgetConnectStorage().getHandle()acquires. This is the "hold exclusive access inforgetUser()before deleting storage" half of the suggestion.ConnectUserDatabaseUtil.forgetUser()delegates to it instead of deleting the DB itself.- The missing-passphrase path now throws a typed
ConnectDatabaseUnavailableExceptioninstead of settingdbBrokenand callingtriggerGlobalError(PERSONALID_GENERIC_ERROR). Losing the race no longer persists aGlobalErrorRecord, which was the actual bug.
So the check-then-act windows still exist, but they are no longer load-bearing: a storage operation racing with sign-out either completes against the live DB or sees it already gone, and the latter is a caught/typed exception rather than a persisted global error.
Declining the remaining part of the suggestion — acquiring guarded access before getUser and holding it "through response storage and acknowledgment." acknowledgeNotificationsReceipt makes a network round-trip, and forgetUser() is called from the UI thread. Holding a lifecycle lock across that span would block sign-out behind in-flight HTTP, trading a race whose failure mode is now benign for an ANR. Per-handle-acquisition granularity is the right scope here.
One residual item this does surface, which is the strongest surviving form of the point: ConnectDatabaseUnavailableException is a RuntimeException with no production catch site beyond the rethrow. It is contained on the notification path by the runCatching block, but the call sites listed as unguarded in the PR description (ConnectOpportunitiesParser, the remaining connectId parsers) would throw it from a background thread with no handler. Better than the persistent banner, but a crash rather than a no-op — tracking that separately rather than widening this PR.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate database and exception files =='
fd -t f -E .git 'ConnectDatabaseHelper|ConnectUserDatabaseUtil|ConnectDatabaseUnavailableException|PersonalIdNotConfiguredException' app
printf '%s\n' '== Locking, teardown, and unavailable-exception paths =='
rg -n -C 4 \
'connectDbHandleLock|clearConnectData\s*\(|deleteDb\s*\(|ConnectDatabaseUnavailableException|PersonalIdNotConfiguredException|triggerGlobalError' \
app/src app/unit-tests
printf '%s\n' '== Production catch sites for the unavailable exception =='
rg -n -C 3 \
'catch\s*\([^)]*ConnectDatabaseUnavailableException|ConnectDatabaseUnavailableException' \
app/srcLength of output: 12458
@OrangeAndGreen, I agree with the design rationale. A per-handle lock is the correct scope. A caller-spanning lock across acknowledgeNotificationsReceipt could block UI-thread sign-out on network I/O.
I could not verify the described fix in the current review snapshot:
ConnectUserDatabaseUtil.forgetUser()still performsdeleteDb(), key removal, andteardown()as separate operations.ConnectDatabaseHelper.clearConnectData()is not present.ConnectDatabaseUnavailableExceptionis not present.ConnectDatabaseHelper.getHandle()still catches broadException, setsdbBroken, and triggersPERSONALID_GENERIC_ERROR.
Therefore, I will leave this finding unresolved until the incoming change is available on the review branch. Once it lands, the caller-side lock portion of this finding should be withdrawn if teardown and handle acquisition share the same lock and the unavailable-database path cannot persist a global error.
The unhandled ConnectDatabaseUnavailableException paths are a separate follow-up concern, as you described.
🐇🔍
✏️ Learnings added
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3860
File: app/src/org/commcare/connect/PersonalIdManager.java:181-191
Timestamp: 2026-08-11T15:05:42.898Z
Learning: For PersonalID sign-out, do not hold a lifecycle lock across `PushNotificationApiHelper.acknowledgeNotificationsReceipt`, because it performs network I/O and `PersonalIdManager.forgetUser(String)` can run on the UI thread. Prefer synchronization at Connect database-handle acquisition and database teardown granularity.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
OrangeAndGreen
force-pushed
the
QA-8627-stale-connect-db-write-after-signout
branch
from
August 10, 2026 18:11
03821a6 to
2440f84
Compare
OrangeAndGreen
changed the title
OrangeAndGreen
changed the title
OrangeAndGreen
force-pushed
the
QA-8627-stale-connect-db-write-after-signout
branch
3 times, most recently
from
August 11, 2026 15:06
e6fffea to
7a97735
Compare
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
OrangeAndGreen
force-pushed
the
QA-8627-stale-connect-db-write-after-signout
branch
from
August 11, 2026 15:24
7a97735 to
47ccba8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/org/commcare/connect/database/ConnectDatabaseUnavailableException.kt`:
- Around line 15-17: Update the Connect parser and worker flows, especially
ConnectReleaseTogglesParser and MessagingChannelsKeySyncWorker, to catch
ConnectDatabaseUnavailableException around database-dependent operations and
return their existing expected no-op result. Do not rely solely on isloggedIn(),
since sign-out can occur concurrently; preserve normal behavior for successful
operations and other failures.
In
`@app/unit-tests/src/org/commcare/connect/database/ConnectDatabaseHelperTest.kt`:
- Around line 64-96: Update testStorageReadRacingSignOutNeverInvalidatesLogin to
seed a valid Connect user and passphrase, and replace the readerReady start
barrier with a test hook or latch at storage-handle acquisition so forgetUser()
runs only after the reader reaches that point. Await reader completion before
asserting fatal and ConnectDatabaseHelper.isDbBroken(), while preserving the
race assertions and expected unavailable-storage handling.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b094f9b8-d5bb-4fe5-bd05-9a9e2b15ad6e
📒 Files selected for processing (13)
RELEASES.mdapp/src/org/commcare/CommCareApplication.javaapp/src/org/commcare/connect/PersonalIdManager.javaapp/src/org/commcare/connect/database/ConnectDatabaseHelper.javaapp/src/org/commcare/connect/database/ConnectDatabaseUnavailableException.ktapp/src/org/commcare/connect/database/ConnectUserDatabaseUtil.javaapp/src/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.ktapp/src/org/commcare/pn/workers/MessagingChannelsKeySyncWorker.ktapp/src/org/commcare/pn/workers/NotificationsSyncWorker.ktapp/src/org/commcare/utils/PushNotificationApiHelper.ktapp/unit-tests/src/org/commcare/activities/PushNotificationActivityTest.ktapp/unit-tests/src/org/commcare/connect/database/ConnectDatabaseHelperTest.ktapp/unit-tests/src/org/commcare/connect/network/connectId/parser/ConnectReleaseTogglesParserTest.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- RELEASES.md
- app/src/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.kt
- app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt
- app/src/org/commcare/utils/PushNotificationApiHelper.kt
- app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConnectReleaseTogglesParserTest.kt
Comment on lines
+15
to
+17
| class ConnectDatabaseUnavailableException( | ||
| message: String, | ||
| ) : RuntimeException(message) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline app/src/org/commcare/connect --items all --type function
printf '%s\n' '== Connect storage access sites =='
rg -n -C 3 'getConnectStorage\s*\(|getConnectDbOpenHelper\s*\(' app/src
printf '%s\n' '== Explicit unavailable-database handling =='
rg -n -C 3 'ConnectDatabaseUnavailableException' app/srcRepository: dimagi/commcare-android
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Database helper behavior =='
sed -n '45,90p' app/src/org/commcare/connect/database/ConnectDatabaseHelper.java
printf '%s\n' '== All storage call sites with enclosing declarations =='
python3 - <<'PY'
from pathlib import Path
import re
root = Path("app/src")
call = re.compile(r'\b(?:ConnectDatabaseHelper\.)?getConnectStorage\s*\(')
decl = re.compile(
r'\b(?:public|private|protected|internal|override|static|suspend|fun|'
r'final|abstract|open|class|object|void|[A-Za-z_][\w<>,.? ]*)\s+'
r'([A-Za-z_]\w*)\s*\([^;{}]*\)\s*(?::\s*[^{=]+)?\s*(?:\{|=)'
)
for path in sorted(root.rglob("*")):
if path.suffix not in {".java", ".kt"}:
continue
text = path.read_text(errors="replace")
lines = text.splitlines()
starts = []
for i, line in enumerate(lines):
m = decl.search(line)
if m:
starts.append((i, m.group(1)))
for i, line in enumerate(lines):
if call.search(line):
candidates = [x for x in starts if x[0] <= i]
name = candidates[-1][1] if candidates else "<unknown>"
print(f"{path}:{i+1}: {name}: {line.strip()}")
PY
printf '%s\n' '== Unavailable exception references and catches =='
rg -n -C 2 'ConnectDatabaseUnavailableException' app/src --glob '*.{java,kt}'
printf '%s\n' '== Connect worker implementations =='
rg -n -C 8 'class .*Worker|override fun doWork|Result\.(success|retry|failure)|ConnectJobUtils|ConnectAppDatabaseUtil|ConnectTaskUtils|ConnectMessagingDatabaseHelper' app/src/org/commcare/connect/workers app/src/org/commcare/connect --glob '*Worker*.{java,kt}' --glob '*.kt' --glob '*.java' | head -n 500Repository: dimagi/commcare-android
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Parser implementations and storage-related calls =='
for f in $(fd -t f -e kt -e java app/src/org/commcare/connect/network); do
if rg -q 'class .*Parser|object .*Parser|fun parse|ConnectDatabaseHelper|getConnectStorage|ConnectJobUtils|ConnectAppDatabaseUtil|ConnectTaskUtils|ConnectMessagingDatabaseHelper' "$f"; then
echo "---- $f"
rg -n -C 6 'class .*Parser|object .*Parser|fun parse|ConnectDatabaseHelper|getConnectStorage|ConnectJobUtils|ConnectAppDatabaseUtil|ConnectTaskUtils|ConnectMessagingDatabaseHelper' "$f"
fi
done
printf '%s\n' '== Worker source files =='
for f in $(fd -t f -e kt -e java app/src/org/commcare/connect/workers); do
echo "---- $f"
cat -n "$f"
done
printf '%s\n' '== Callers of parser classes and database utility entry points =='
rg -n -C 5 'ConnectOpportunitiesParser|LearningAppProgressResponseParser|DeliveryAppProgressResponseParser|ConnectReleaseTogglesParser|RetrieveHqTokenResponseParser|ConnectTokenResponseParser|LinkHqWorkerResponseParser|RetrieveNotificationsResponseParser|RetrieveWorkHistoryResponseParser|RetrieveChannelEncryptionKeyResponseParser|ConnectJobUtils\.|ConnectAppDatabaseUtil\.|ConnectTaskUtils\.|ConnectMessagingDatabaseHelper\.' app/src --glob '*.{java,kt}' | head -n 1000Repository: dimagi/commcare-android
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Focused parser source excerpts =='
for f in \
app/src/org/commcare/connect/network/connect/parser/ConnectOpportunitiesParser.kt \
app/src/org/commcare/connect/network/connect/parser/LearningAppProgressResponseParser.kt \
app/src/org/commcare/connect/network/connect/parser/DeliveryAppProgressResponseParser.kt \
app/src/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.kt \
app/src/org/commcare/connect/network/connectId/parser/RetrieveHqTokenResponseParser.kt \
app/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParser.kt \
app/src/org/commcare/connect/network/connectId/parser/LinkHqWorkerResponseParser.kt \
app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt \
app/src/org/commcare/connect/network/connectId/parser/RetrieveWorkHistoryResponseParser.kt \
app/src/org/commcare/connect/network/connectId/parser/RetrieveChannelEncryptionKeyResponseParser.kt; do
if [ -f "$f" ]; then
echo "---- $f"
cat -n "$f"
fi
done
printf '%s\n' '== Focused worker source excerpts =='
for f in $(fd -t f -e kt -e java app/src/org/commcare/connect/workers); do
echo "---- $f"
cat -n "$f"
doneRepository: dimagi/commcare-android
Length of output: 26957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Direct storage callers grouped by file =='
python3 - <<'PY'
from pathlib import Path
import re
pat = re.compile(r'\bgetConnectStorage\s*\(')
for path in sorted(Path("app/src").rglob("*")):
if path.suffix not in {".java", ".kt"}:
continue
lines = path.read_text(errors="replace").splitlines()
hits = [i + 1 for i, line in enumerate(lines) if pat.search(line)]
if hits:
print(f"{path}: {','.join(map(str, hits))}")
print("== Exception catch blocks by file ==")
catch = re.compile(r'\bcatch\s*\([^)]*ConnectDatabaseUnavailableException[^)]*\)')
for path in sorted(Path("app/src").rglob("*")):
if path.suffix not in {".java", ".kt"}:
continue
lines = path.read_text(errors="replace").splitlines()
for i, line in enumerate(lines):
if catch.search(line):
print(f"{path}:{i+1}: {line.strip()}")
PY
printf '%s\n' '== Worker and parser names referenced by production code =='
rg -n 'class [A-Za-z0-9_]*(Worker|Parser)|object [A-Za-z0-9_]*(Worker|Parser)|[A-Za-z0-9_]*(Worker|Parser)\(' app/src --glob '*.{java,kt}' | head -n 500Repository: dimagi/commcare-android
Length of output: 24805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("app/src")
call = re.compile(r'\bConnectDatabaseHelper\s*\.\s*getConnectStorage\s*\(')
decl = re.compile(
r'(?:(?:public|private|protected|internal|override|static|final|suspend|open)\s+)*'
r'(?:[\w.<>,?\[\]]+\s+)+([A-Za-z_]\w*)\s*\([^;{}]*\)\s*(?:[:][^{=]+)?(?:\{|=)'
)
def enclosing(lines, line_no):
found = []
for i, line in enumerate(lines[:line_no + 1]):
m = decl.search(line)
if m:
found.append((i + 1, m.group(1)))
return found[-1] if found else (None, "<unknown>")
for path in sorted(root.rglob("*")):
if path.suffix not in {".java", ".kt"}:
continue
lines = path.read_text(errors="replace").splitlines()
for i, line in enumerate(lines):
if call.search(line):
line_no, method = enclosing(lines, i)
print(f"{path}:{i+1}: {method} (declared at {line_no}): {line.strip()}")
print("== Catch sites ==")
catch = re.compile(r'\bcatch\s*\([^)]*ConnectDatabaseUnavailableException[^)]*\)')
for path in sorted(root.rglob("*")):
if path.suffix not in {".java", ".kt"}:
continue
for i, line in enumerate(path.read_text(errors="replace").splitlines()):
if catch.search(line):
print(f"{path}:{i+1}: {line.strip()}")
PYRepository: dimagi/commcare-android
Length of output: 10157
Handle the sign-out race in Connect parsers and workers.
ConnectDatabaseHelper is the only catch site, but parsers and MessagingChannelsKeySyncWorker can still propagate ConnectDatabaseUnavailableException. The isloggedIn() check in ConnectReleaseTogglesParser does not prevent a concurrent sign-out. Return the expected no-op result when this exception occurs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/org/commcare/connect/database/ConnectDatabaseUnavailableException.kt`
around lines 15 - 17, Update the Connect parser and worker flows, especially
ConnectReleaseTogglesParser and MessagingChannelsKeySyncWorker, to catch
ConnectDatabaseUnavailableException around database-dependent operations and
return their existing expected no-op result. Do not rely solely on isloggedIn(),
since sign-out can occur concurrently; preserve normal behavior for successful
operations and other failures.
Comment on lines
+64
to
+96
| fun testStorageReadRacingSignOutNeverInvalidatesLogin() { | ||
| val readerReady = CountDownLatch(1) | ||
| val signOutDone = CountDownLatch(1) | ||
| val fatal = AtomicReference<Throwable?>(null) | ||
|
|
||
| val reader = | ||
| Thread { | ||
| readerReady.countDown() | ||
| // keep hammering storage across the sign-out so at least one read lands on either | ||
| // side of it, and ideally one lands mid-teardown | ||
| repeat(200) { | ||
| try { | ||
| readUserStorage() | ||
| } catch (expected: ConnectDatabaseUnavailableException) { | ||
| // the read can't succeed once the account is gone, it just has to fail | ||
| // survivably | ||
| } catch (t: Throwable) { | ||
| if (t is LoginInvalidatedException) { | ||
| fatal.compareAndSet(null, t) | ||
| } | ||
| } | ||
| } | ||
| signOutDone.countDown() | ||
| } | ||
|
|
||
| reader.start() | ||
| assertTrue(readerReady.await(5, TimeUnit.SECONDS)) | ||
| ConnectUserDatabaseUtil.forgetUser() | ||
| reader.join(TimeUnit.SECONDS.toMillis(30)) | ||
|
|
||
| assertNull("A read racing sign-out invalidated the login", fatal.get()) | ||
| assertFalse("A read racing sign-out flagged the Connect DB as broken", | ||
| ConnectDatabaseHelper.isDbBroken()) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the race test use a live database and a deterministic interleaving.
The test creates no passphrase or user before Line 91. readerReady only confirms that the thread started. It does not confirm that a storage read reached handle acquisition. The test can pass after executing only the expected unavailable-storage path.
Seed a Connect user and passphrase before starting the reader. Add a test hook or latch that pauses the reader at handle acquisition. Trigger forgetUser() only after that latch. Assert that the reader completes before checking the fatal error and dbBroken.
As per coding guidelines, unit tests must “provide comprehensive coverage of all public methods in the tested class.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/unit-tests/src/org/commcare/connect/database/ConnectDatabaseHelperTest.kt`
around lines 64 - 96, Update testStorageReadRacingSignOutNeverInvalidatesLogin
to seed a valid Connect user and passphrase, and replace the readerReady start
barrier with a test hook or latch at storage-handle acquisition so forgetUser()
runs only after the reader reaches that point. Await reader completion before
asserting fatal and ConnectDatabaseHelper.isDbBroken(), while preserving the
race assertions and expected unavailable-storage handling.
Source: Coding guidelines
Follow-up to the sign-in gates on the Connect write sites. Those close the ordinary paths, but a status check is check-then-act: a caller already past its check when sign-out lands still reaches a deleted DB, and the storage layer treated that as a corrupt DB. So sign-out drops the work it can, and what still gets through is no longer fatal. - forgetUser cancels the periodic release-toggles worker and drops in-flight requests via ConnectRequestManager.cancelAll() (which existed for this but was never called) before deleting the DB. - ConnectUserDatabaseUtil.forgetUser was racing connectDbHandleLock on its own: deleteDb() and the passphrase removal ran outside the lock while teardown() took it, so a reader could open a handle mid-teardown and re-flag the DB as broken after forgetUser had just cleared the flag. Moved into ConnectDatabaseHelper.clearConnectData(), which holds the lock across teardown, deletion and passphrase removal. Deleting a local DB touches no network, so the critical section stays short enough to hold on the UI thread that forgetUser already runs on. - An absent passphrase means there's no account to open a DB for, which is simply the state after sign-out, not corruption. getConnectDbOpenHelper now reports it as ConnectDatabaseUnavailableException rather than a bare IllegalStateException, and getHandle rethrows that without setting dbBroken or raising the global error. Only a DB that won't open despite having a passphrase is still treated as broken. Losing the race now degrades to a logged, catchable no-op instead of an account wipe and process restart. - dbBroken is private and volatile; it was package-private and read from threads other than the one writing it. - The notification-processing coroutine resumes its continuation exactly once rather than letting a storage failure escape to the uncaught exception handler, acknowledgeNotificationsReceipt null-checks the user, and no FCM notification is raised while signed out. ConnectDatabaseHelperTest covers storage reached around sign-out, including a latch-based case that reads Connect storage from a background thread across a concurrent forgetUser and asserts the invariant that holds under any interleaving: never LoginInvalidatedException, never dbBroken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OrangeAndGreen
force-pushed
the
QA-8627-stale-connect-db-write-after-signout
branch
from
August 11, 2026 17:26
47ccba8 to
8c6588f
Compare
OrangeAndGreen
changed the title
OrangeAndGreen
changed the base branch from
commcare_2.64
to
QA-8627-gate-connect-writes-when-signed-out
August 11, 2026 17:27
Base automatically changed from
QA-8627-gate-connect-writes-when-signed-out
to
commcare_2.64
August 13, 2026 06:21
4 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
QA-8627
Second of two PRs, stacked on #3863 — review that one first, and merge it first. The diff here is only the follow-up work; retarget to
commcare_2.64once #3863 lands.Product Description
Same user-visible symptom as #3863: signing out of PersonalID could leave the nav drawer showing "Logged out of PersonalID — A problem occurred, please configure your PersonalID account again". In the worst case the error also reached the uncaught exception handler as a
LoginInvalidatedException, which wipes the account and exits the process. This PR removes that outcome entirely.Technical Summary
#3863 gates the Connect write sites on
isloggedIn(). That closes the ordinary paths, but a status check is check-then-act: a caller already past its check when sign-out lands still reaches a deleted DB, and the storage layer treated that as a corrupt DB. So sign-out now drops the work it can, and what still gets through is no longer fatal.Drop more of the in-flight work
forgetUsercancels the periodic release-toggles worker and drops in-flight requests viaConnectRequestManager.cancelAll()— which existed for exactly this but was never called — before deleting the DB.NotificationsSyncWorkerno longer raises an FCM notification while signed out.Stop treating the lost race as corruption
ConnectUserDatabaseUtil.forgetUserwas racingconnectDbHandleLockon its own:deleteDb()and the passphrase removal ran outside the lock whileteardown()took it, so a reader could open a handle mid-teardown and re-flag the DB as broken afterforgetUserhad just cleared the flag. Moved intoConnectDatabaseHelper.clearConnectData(), which holds the lock across teardown, deletion and passphrase removal. Deleting a local DB touches no network, so the critical section stays short enough to hold on the UI threadforgetUseralready runs on.getConnectDbOpenHelpernow throws the typedConnectDatabaseUnavailableExceptioninstead of a bareIllegalStateException, andgetHandlerethrows it without settingdbBrokenor raising the global error. Only a DB that won't open despite having a passphrase is still treated as broken. Losing the race now degrades to a logged, catchable no-op instead of an account wipe and process restart.dbBrokenis private and volatile; it was package-private and read from threads other than the one writing it.Contain the notification path
The storage/broadcast/acknowledge block in
callPushNotificationApiis wrapped inrunCatching: a failure is logged and returned asResult.failurerather than escaping to the uncaught exception handler, and the continuation is resumed exactly once either way (previously a throw there left the caller suspended forever). A failed acknowledgement is logged but not treated as a failure — the notifications are stored and the server resends unacknowledged ones on the next sync.acknowledgeNotificationsReceiptalso returnsfalseon a null user instead of NPEing.Safety Assurance
Safety story
What gives me confidence:
LoginInvalidatedExceptionreaching the uncaught handler and wiping the account — is now unreachable from a missing passphrase, which is the only way sign-out can produce it.clearConnectDatastrictly widens an existing critical section; every operation it performs was already happening, just partly unlocked.ConnectDatabaseHelperTestasserts the invariant directly, including under real thread interleaving.Risks to review:
clearConnectDataholdsconnectDbHandleLockacross the DB deletion on the UI thread. Local file I/O with no network in the critical section, andforgetUseralready ran on that thread, but it is a longer hold than before. Worth a look if anyone knows of a Connect storage call that can block for a long time under that lock.ConnectRequestManager.cancelAll()is now called for the first time. It cancels in-flight deferreds, so a caller awaiting one sees aCancellationExceptionat sign-out. That is what the method was written for, but it has never actually run in production.runCatchingon the notification path swallows more than the sign-out race. Any storage failure inprocessParsedDataIntoDB— not just a deleted DB — now becomes a loggedResult.failureinstead of an uncaught exception. That is the intent (a background notification sync should not take down the process), but it makes a genuine storage bug quieter than it used to be. The failure is also logged and returned after partial work may have happened; there is no rollback.PERSONALID_DB_STARTUP_ERRORon anulluser ininitstill covers the startup case.forgetUserand asserts what must hold under any interleaving — neverLoginInvalidatedException, neverdbBroken— rather than trying to hit one specific ordering. It will not reliably reproduce the original failure if the fix regresses in a timing-dependent way.Automated test coverage
ConnectDatabaseHelperTestadds three cases covering Connect storage reached around sign-out, including the latch-based one described above.Carried over from #3863:
ConnectReleaseTogglesParserTestcovers the release-toggle guard in both directions. TherunCatchinghardening on the notification path is not directly covered.