QA-8627 Don't Write Connect Data After Signing Out Of PersonalID (1/2: gate the write sites)

[OrangeAndGreen]

QA-8627

First of two PRs. This one gates the Connect write sites on the sign-in status; #3860 stacks the storage-layer hardening on top.

Product Description

Signing out of PersonalID could leave the nav drawer showing "Logged out of PersonalID — A problem occurred, please configure your PersonalID account again", instead of the sign-in and register options. The error persisted for 24 hours or until dismissed. Sign-out now leaves the drawer in the correct signed-out state.

Technical Summary

The trigger is a stale write: an in-flight Connect request (notification retrieval, release toggles) lands after signing out has deleted the Connect DB and its passphrase. ConnectDatabaseHelper.getHandle() then can't open the DB, flags dbBroken and raises PERSONALID_GENERIC_ERROR, which the exception handler persists as a GlobalErrorRecord for the drawer to display.

Connect storage is only valid while signed in, so the callers violating that contract now check before they write:

• PushNotificationApiHelper.callPushNotificationApi resumes with an empty list instead of storing retrieved notifications and acknowledging them.
• ConnectReleaseTogglesParser still parses the response and returns it, it just doesn't persist it.
• MessagingChannelsKeySyncWorker read Connect storage with no check at all, and now bails out.

Two supporting changes are what make those checks mean anything:

• forgetUser flips the status to NotIntroduced before deleting the DB rather than after. Previously a callback landing between the delete and the status change would pass its check and hit a deleted DB. Nothing between the two reads the DB, but the ordering is load-bearing.
• personalIdSatus is now volatile, since every one of these checks runs on a worker thread while sign-out writes the field from the UI thread.

Safety Assurance

Safety story

What gives me confidence:

• The change is additive at each call site — early returns on a state that previously produced the bug. No behavior change while signed in.
• New tests cover the release-toggle guard in both directions, and the existing PushNotificationActivityTest suite passes against the new gate.
• The sign-out flow was exercised on device and the drawer shows the sign-in/register state as expected.

Risks to review:

• A status check is check-then-act, so this narrows the window rather than closing it. A caller already past its check when sign-out lands still reaches the deleted DB and still produces the banner. That interleaving is what the follow-up PR addresses; this PR covers the ordinary case, which is the whole in-flight duration of a request rather than the microseconds inside forgetUser.
• The race is not directly reproduced by any test. It needs a response to land inside a narrow window after sign-out. QA's repeated sign-out while notifications are actively arriving is the real check here.
• Responses arriving while registering or recovering are now dropped too, not just after sign-out. The Connect DB does exist in that state, so this discards data that could have been stored. Deliberate — the simpler isloggedIn() gate was preferred over tracking that case — but it is a real behavior change. PushNotificationActivityTest's click-gating test had to be adjusted for it: it previously loaded notifications while registering, and now loads signed in and drops the login only for the click it actually covers.
• This is a caller-by-caller fix, so coverage is only as complete as the list of callers. Guarded here: notification retrieval, release toggles, messaging-channel key sync. The FCM notification raise in NotificationsSyncWorker is guarded in the follow-up. Not guarded anywhere: ConnectOpportunitiesParser (reachable from the same NotificationsSyncWorker via syncOpportunities) and the remaining connectId response parsers. Those can still land post-sign-out. ConnectOpportunitiesParser was left out because its tests assert storeJobs is always called and would all need updating; worth deciding whether to widen this or track it separately. Note that the follow-up PR makes the consequence non-fatal for all of them, guarded or not.

Automated test coverage

ConnectReleaseTogglesParserTest gains two cases: toggles are stored while signed in, and are parsed but not persisted once signed out — the stale-write path this PR fixes.

The race window itself is not covered; the failure needs a response to arrive inside a narrow window after sign-out, which the existing test harness cannot schedule. The follow-up PR covers the interleaving with a latch-based concurrency test.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 49acbaf4-81da-4804-84a7-3e4e84648a4b

📥 Commits

Reviewing files that changed from the base of the PR and between 824c179 and ff30984.

📒 Files selected for processing (7)

• RELEASES.md
• app/src/org/commcare/connect/PersonalIdManager.java
• app/src/org/commcare/connect/network/connect/parser/ConnectReleaseTogglesParser.kt
• app/src/org/commcare/pn/workers/MessagingChannelsKeySyncWorker.kt
• app/src/org/commcare/utils/PushNotificationApiHelper.kt
• app/unit-tests/src/org/commcare/activities/PushNotificationActivityTest.kt
• app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConnectReleaseTogglesParserTest.kt

---

📝 Walkthrough

Walkthrough

PersonalID sign-out now resets login state before clearing stored user data and makes status visibility thread-safe. Release-toggle parsing, messaging key synchronization, and push-notification handling skip database or synchronization work when sign-out occurs during an in-flight operation. Tests and QA scenarios cover signed-in persistence, signed-out behavior, and notification navigation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant PersonalIdManager
  participant PushNotificationApiHelper
  participant ConnectReleaseTogglesParser
  participant MessagingChannelsKeySyncWorker
  User->>PersonalIdManager: Sign out
  PersonalIdManager->>PersonalIdManager: Set status to NotIntroduced
  PushNotificationApiHelper->>PersonalIdManager: Check login state
  ConnectReleaseTogglesParser->>PersonalIdManager: Check login state
  MessagingChannelsKeySyncWorker->>PersonalIdManager: Check login state
  PushNotificationApiHelper-->>PushNotificationApiHelper: Skip response processing when signed out
  ConnectReleaseTogglesParser-->>ConnectReleaseTogglesParser: Skip toggle persistence when signed out
  MessagingChannelsKeySyncWorker-->>MessagingChannelsKeySyncWorker: Skip channel synchronization when signed out

Loading

Possibly related PRs

• dimagi/commcare-android#3860: Modifies the same PersonalID sign-out race protections and related components.
• dimagi/commcare-android#3114: Modifies PersonalIdManager and PersonalID login-state handling.
• dimagi/commcare-android#3771: Modifies notification-click navigation tests.

Suggested reviewers: conroy-ricketts, jignesh-dimagi

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the ticket and the primary change: gating Connect writes after PersonalID sign-out.
Description check	✅ Passed	The description covers the product impact, technical rationale, safety risks, test coverage, and follow-up limitations; only the Labels and Review checklist is missing.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch QA-8627-gate-connect-writes-when-signed-out

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 27.36%. Comparing base (b83cb5d) to head (c9adc91).
⚠️ Report is 28 commits behind head on commcare_2.64.

Additional details and impacted files

@@                 Coverage Diff                 @@
##             commcare_2.64    #3863      +/-   ##
===================================================
- Coverage            27.37%   27.36%   -0.01%     
- Complexity            4789     4790       +1     
===================================================
  Files                  988      988              
  Lines                58947    58982      +35     
  Branches              7029     7036       +7     
===================================================
+ Hits                 16135    16140       +5     
- Misses               40864    40892      +28     
- Partials              1948     1950       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Jignesh-dimagi]

@OrangeAndGreen

Trigger on Unlock: Every successful biometric unlock enqueues the Notification and Connect Toggle workers.

The PeriodicWorkRequest is built with no initial delay, making it eligible to execute immediately upon enqueueing during the biometric unlock step.

When the user navigates to "Manage Profile" (after biometric unlock) and triggers "Forgot Personal ID", the app attempts to cancel these workers. However, NotificationsSyncWorker is a CoroutineWorker executing blocking I/O calls (withContext(Dispatchers.IO)). Because coroutine cancellation is cooperative, the blocking network/DB operations do not observe cancellation immediately and complete in-flight tasks anyway.

Reproducible: If the user presses "Forgot Personal ID" immediately after biometric unlock (in-flight worker task will cause this issue).

Non-Reproducible: If the user pauses long enough, on the PersonalId Profile screen, to allow in-flight calls to finish before pressing "Forgot Personal ID".

I think changing the ExistingPeriodicWorkPolicy should work here.

WorkManager.getInstance(context).enqueueUniquePeriodicWork(
                PERIODIC_NOTIFICATION_REQUEST_NAME,
                ExistingPeriodicWorkPolicy.REPLACE, --> cancel existing,  re-enqueues every time and immediate call
                retrievalRequest,
            )

to

WorkManager.getInstance(context).enqueueUniquePeriodicWork(
                PERIODIC_NOTIFICATION_REQUEST_NAME,
                ExistingPeriodicWorkPolicy.UPDATE, --> don't cancel but call as per the scheduled time only
                retrievalRequest,
            )

Also, needs to be corrected for Connect's Heartbeat

CC @shubham1g5

[shubham1g5]

@Jignesh-dimagi Agree that seems like a good change to make on this PR. Seems like we should also modify ConnectReleaseTogglesWorker.Companion.schedulePeriodicFetch to use Update instead of keep to be consistent here.

[OrangeAndGreen]

Done in e58f3ce

• NotificationsSyncWorkerManager.schedulePeriodicPushNotificationRetrieval — REPLACE → UPDATE
• PersonalIdManager.scheduleHeartbeat (Connect heartbeat) — REPLACE → UPDATE
• ConnectReleaseTogglesWorker.schedulePeriodicFetch — KEEP → UPDATE

Added a QA note to RELEASES.md covering the scheduling change.

[conroy-ricketts]

@OrangeAndGreen

With these changes, I'm now seeing a new issue where forgetting PersonalID incorrectly drops me on a CC app home page.

Will share a video with you privately to avoid leaking my email address

[OrangeAndGreen]

@OrangeAndGreen

With these changes, I'm now seeing a new issue where forgetting PersonalID incorrectly drops me on a CC app home page.

Will share a video with you privately to avoid leaking my email address

Ah, good find! I think this surfaces a new issue that we'll want to address, although I'm not sure whether to include it in this PR. All in all, I think the issue is that forgetting PersonalID never currently forces any app navigation but sometimes it should.

In the case of App Home, we should probably logout of the app and navigate to Login.
In the case of any other Connect page (Opp List, Messaging, Work History, etc.), we should just navigate back to Login (or Setup if no apps installed yet).

But automatically logging out of the app (the App Home case) could be risky, for instance if the user still has un-synced forms. I wonder if we should consider disabling the Forget button while in an app? Or we could leave it enabled and show the user a message telling them to logout of their app first if they try to forget their account while on App Home (thinking the disabled button could otherwise be confusing). @shubham1g5 I'm thinking you probably have good insight here.

Thinking a bit more, although maybe this gets fancier than we need... If we're on App Home for a non-Connect app at the time the user forgets PersonalID, we don't need to logout of the app.

Finally, flagging that this is a separate issue unrelated to and currently unaffected by this PR, so we could consider merging these changes and tackling the additional issue in a new ticket.

[conroy-ricketts]

Thanks for the context, and agreed, this fix is time-sensitive

[conroy-ricketts]

We may want to provide some of that context in the ticket as well when this gets merged so that QA is not caught off guard

[shubham1g5]

, for instance if the user still has un-synced forms. I wonder if we should consider disabling the Forget button while in an app?

Ahh think that's alright, an app can have un-synced forms even where user is logged out. And technically it'd be possible for user to recover that data if they sign back into PersonalID account on the same app installation. It'd be nice for us to warn user when they click Forget but think that's most we should do here.