CCCT-1877 Phone Verification Crash (Empty Session Data)

[conroy-ricketts]

CCCT-1877

Technical Summary

This crash is happening because the user's session data is null when trying to get the phone number.

Here's why the session data is null for these users: They are putting the app in the background, and the Android OS, under memory pressure, decides to terminate the app's process to reclaim resources. Then, the entire PersonalIdSessionDataViewModel instance gets wiped from memory. When the users return to the app and the PersonalIdPhoneVerificationFragment is recreated, a new empty PersonalIdSessionDataViewModel is created because the old one was destroyed during process death and the PersonalIdSessionData inside the view model was never re-initialized.

I assume that the reason users are backgrounding the app in the first place is to read the OTP from the text message that gets sent to them.

Here is the easiest way I found to reproduce the crash:

1. Enable the "Developer options" in your phone's settings if you have not already.
2. In the "Developer options" settings, find the "Don't keep activities" setting under the "Apps" section and enable it.
3. In the CommCare app, go through the Personal ID signup flow until you get to the OTP verification screen, but do NOT enter the OTP - stay on that page.
4. Background the app.
5. Foreground the app.
6. Observe that the app crashes.

Here's a video demonstrating those reproduction steps above:

Screen_Recording_20251126_160510_CommCare.Debug.mp4

I think the best way to fix this crash is to makePersonalIdSessionDataViewModel lifecycle-aware.

I considered kicking the user back to the start of the Personal ID signup flow (or even the activity they were on just before they started the Personal ID signup flow) to avoid the crash, but if you really think about this, the user will likely see the crash again because they are likely to background the app again to read the OTP text message.

We could use the phone number that is saved to the savedInstanceState bundle in PersonalIdPhoneVerificationFragment, but the app will still crash because we also need the session data for getting/setting otpAttempts, getting the otpFallback, and getting the smsMethod.

Note that since the user is coming back to the same screen after process death, we also need to tweak some logic in the PersonalIdPhoneVerificationFragment because of our OTP fallback implementation or else we will have more logical errors.

Let me know what y'alls thoughts are on this!

Safety Assurance

Safety story

I verified that, with these changes, the app no longer crashes.

I also verified that I'm still able to successfully complete the entire Personal ID signup flow with these changes - the app behaves as expected, especially when the OTP fallback is used.

QA Plan

QA should follow the reproduction steps I listed above and ensure that the app does not crash. QA should also background and freground the app at various states of otp-recreations (before sending code, after sending code, after verifying otp , after resending code once and twice) to check that we restore successfully and able to verify otp in each of those scenarios.

Also, QA should follow the test notes that I outlined here to ensure that there are no regressions.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR migrates PersonalIdSessionData persistence from in-memory ViewModel fields to AndroidX SavedStateHandle to survive process death. It adds Serializable support and a new otpFallbackUsedBeforeProcessDeath flag to the PersonalIdSessionData model. PersonalIdPhoneVerificationFragment is updated to conditionally initialize OTP state on first creation only and leverage the persisted fallback flag when applying OTP fallback logic.

Sequence Diagram

sequenceDiagram
    actor User
    participant Fragment as PersonalIdPhoneVerificationFragment
    participant ViewModel as PersonalIdSessionDataViewModel
    participant SavedState as SavedStateHandle
    participant OTPMgr as OTP Manager

    User->>Fragment: Initial load / OTP attempt
    Fragment->>ViewModel: Check otpAttempts
    alt First creation (otpAttempts == 0)
        Fragment->>Fragment: setupInitialState()
    else Process death recovery
        Fragment->>Fragment: Skip setupInitialState
        Note over Fragment: Preserve OTP state from before death
    end

    User->>Fragment: OTP attempt fails
    Fragment->>ViewModel: getOtpFallbackUsedBeforeProcessDeath()
    SavedState->>ViewModel: Retrieve persisted flag
    alt Attempt 2 OR flag already set
        Fragment->>OTPMgr: Create with fallback enabled
        Fragment->>ViewModel: setOtpFallbackUsedBeforeProcessDeath(true)
        SavedState->>SavedState: Persist flag
    else First attempt, no prior fallback
        Fragment->>OTPMgr: Create without fallback
        Fragment->>ViewModel: setOtpFallbackUsedBeforeProcessDeath(false)
        SavedState->>SavedState: Persist flag
    end

    rect rgb(200, 220, 240)
        Note over Fragment,SavedState: Process death scenario
        Fragment--xFragment: Process killed
        Fragment->>ViewModel: onCreate (reconstructed)
        SavedState->>ViewModel: Restore PersonalIdSessionData + flags
        Fragment->>ViewModel: Read restored state & flag
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• SavedStateHandle integration: Verify correct initialization and get/set patterns in ViewModel
• Serialization: Ensure PersonalIdSessionData can serialize/deserialize correctly across process death
• Conditional initialization logic: Review onCreateView's otpAttempts == 0 check to confirm it prevents unintended state resets
• OTP fallback state transitions: Trace the flag state through setupOtpManager to ensure proper persistence and reset sequencing

Possibly related PRs

• Fallback to PersonalID from Firebase Otp auth in case of Errors #3334: Directly modifies PersonalIdSessionData and PersonalIdPhoneVerificationFragment to persist OTP fallback state and alter fallback logic
• fixed wrong backup code bug #3159: Related to SavedStateHandle-backed ViewModel and PersonalIdSessionData storage/access patterns
• Twilio fallback Copy for master #3232: Adds persistent session fields to PersonalIdSessionData model using similar serialization approach

Suggested labels

skip-integration-tests

Suggested reviewers

• OrangeAndGreen
• shubham1g5
• Jignesh-dimagi
• jaypanchal-13

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main bug being fixed: a crash on the phone verification screen caused by empty session data after process death.
Description check	✅ Passed	PR description is comprehensive with technical summary, clear problem explanation, reproduction steps, safety assurance details, and QA plan, though some template sections are missing.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch bug/CCCT-1877-no-session-data-for-phone-verification-crash

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (1)

167-172: Avoid re‑requesting OTP on recreation, but consider restoring UI state separately

Gating setupInitialState() on personalIdSessionData.getOtpAttempts() == 0 is a good way to prevent sending a new “initial” OTP when the fragment is recreated after process death or configuration changes.

One side effect is that, when otpAttempts > 0, you now skip updateVerificationMessage() and the initial button state setup driven by setupInitialState(), so the recreated UI may not show the “last 4 digits” text or initial disabled state until the user interacts again.

If that matters UX‑wise, consider splitting setupInitialState() into something like:

• setupInitialUiState() (updates label, button state, clears errors), and
• requestInitialOtp(),

and call only the UI part unconditionally while keeping requestInitialOtp() behind the otpAttempts == 0 check.

Based on learnings, this fragment already cleans up view binding in onDestroyView(), so factoring out UI initialization shouldn’t pose leak risks.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4574b29 and 8684236.

📒 Files selected for processing (4)

• app/build.gradle (1 hunks)
• app/src/org/commcare/activities/connect/viewmodel/PersonalIdSessionDataViewModel.java (1 hunks)
• app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (2 hunks)
• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (3 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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-04-21T15:02:17.492Z

Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3042
File: app/src/org/commcare/fragments/BreadcrumbBarViewModel.java:50-55
Timestamp: 2025-04-21T15:02:17.492Z
Learning: ViewModels should not store View or Activity references as this can cause memory leaks. Unlike Fragments with setRetainInstance(true), ViewModels don't have automatic view detachment mechanisms. When migrating from Fragments to ViewModels, view references should be replaced with data-only state in the ViewModel.

Applied to files:

• app/src/org/commcare/activities/connect/viewmodel/PersonalIdSessionDataViewModel.java

📚 Learning: 2025-03-10T08:16:59.436Z

Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.

Applied to files:

• app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java

🧬 Code graph analysis (1)

app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (1)

app/src/org/commcare/utils/PersonalIDAuthService.kt (1)

• activity (10-44)

⏰ 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 (5)

app/build.gradle (1)

119-123: SavedStateHandle dependency addition is appropriate

Adding androidx.lifecycle:lifecycle-viewmodel-savedstate is consistent with the new SavedStateHandle‑based PersonalIdSessionDataViewModel and is the right artifact for process‑death persistence. No issues from this change alone.

Please confirm that $lifecycle_version matches the rest of your lifecycle components and that your existing CI/instrumentation suite covers the Personal ID flow with this new dependency enabled.

app/src/org/commcare/activities/connect/viewmodel/PersonalIdSessionDataViewModel.java (1)

5-30: SavedStateHandle integration is good; verify ViewModel factory wiring

Using SavedStateHandle with a dedicated key here is a good fit for persisting PersonalIdSessionData, and making the model Serializable supports this usage.

One thing to double‑check: PersonalIdSessionDataViewModel now only has a SavedStateHandle constructor. Unless the hosting Activity overrides getDefaultViewModelProviderFactory() to return a SavedStateViewModelFactory/AbstractSavedStateViewModelFactory (or similar), a plain new ViewModelProvider(requireActivity()) call (as in PersonalIdPhoneVerificationFragment) will fail at runtime because there is no no‑arg constructor.

If that default factory is already in place, this looks solid; if not, you’ll need to switch to a SavedState‑aware factory at the call site or via the Activity’s default factory.

Based on learnings, this ViewModel correctly keeps only data (no View/Activity references); please just confirm that the default ViewModelProvider.Factory for the Personal ID flow is a SavedState‑aware one so the SavedStateHandle constructor is actually used.

app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)

4-46: Serializable + process‑death flag on PersonalIdSessionData are appropriate

Making PersonalIdSessionData Serializable and adding otpFallbackUsedBeforeProcessDeath cleanly support persisting OTP session state through SavedStateHandle. All fields are primitives/Strings, so they’re safe for Serializable, and the default value keeps existing callers backward‑compatible.

Please confirm that this object is only serialized for in‑memory/saved‑state purposes (and not for long‑term disk/network persistence); if it ever becomes a persisted schema, we may want to be more deliberate about versioning.

app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (2)

143-145: Fallback decision now correctly accounts for prior fallback usage

Including personalIdSessionData.getOtpFallbackUsedBeforeProcessDeath() in the useOtpFallback calculation ensures that, after a process death, you continue using the Twilio/Personal ID fallback if it was already in use, rather than silently reverting to Firebase. This matches the intended behavior around OTP retries and looks correct.

---

373-393: Persisting otpFallbackUsedBeforeProcessDeath inside setupOtpManager is well‑placed

Updating personalIdSessionData.setOtpFallbackUsedBeforeProcessDeath(true/false) in the same method that chooses the OTP backend keeps the persisted flag tightly coupled to the actual runtime choice. This ensures initOtpManager() can reliably restore the correct backend after process death, without needing extra bookkeeping elsewhere.

[Jignesh-dimagi]

@conroy-ricketts Now we are storing the personalIdSessionData in SavedStateHandle of ViewModel, only checking the
personalIdSessionData.getOtpAttempts() > 1 should work?

[conroy-ricketts]

Solely checking for the number of otpAttempts would lead to a logical error in our code currently. More details here

[conroy-ricketts]

On second thought, we may actually be able to simplify this a lot because this function is only called in onCreate(). I'm going to tweak this and do more testing

[Jignesh-dimagi]

@conroy-ricketts Is this required as we have started storing PersonalIdSessionData with persistance data? I think we should use ViewModel only for persistence storage instead using both ViewModel and Fragment's savedInstance to have clear code?

[shubham1g5]

Think PersonalIdSessionData is a shared view model across multiple fragments and it will be confusing for it to have temporary fragment UI state from multiple fragments. Instead it would be more concrete for it to only have key data points that define the UI state and business logic itself on these signup fragments. We can potentially have a view model per fragment as well in addition to PersonalIdSessionDataViewModel to do this.

[Jignesh-dimagi]

Yeah that is correct if storing any fragment UI state but I am here proposing to use only otpAttempts, which is already present, to calculate the PersonalIdPhoneVerificationFragment UI

[conroy-ricketts]

That would lead us back to the issue discussed in this thread

[conroy-ricketts]

@shubham1g5 @Jignesh-dimagi In PersonalIdPhoneVerificationFragment.java, I simplified the useOtpFallback check inside initOtpManager() since that function is only called in onCreate(). I tested the code using both the fallback and firebase and it works as expected. Let me know what y'all think!

[shubham1g5]

Looks good to go and great work on troubleshooting and implementing the fix here, my sense is we might need to do this kind of background/re-creation testing with all our screens as I think we are not really storing state in Connect side of code anywhere, but it does feel specially important for OTP screen and I am glad to have this fixed.