MOB-1836: Return to onboarding after the encrypted store is wiped; retry and quarantine instead of delete (MOB-1565)

[nesence-m]

Fixes MOB-1836 — Exception on Android after updating from an old release and MOB-1565 — Seed destroyed on any transient Keystore/encrypted-prefs exception.

Problem

The wallet seed lives in the encrypted preferences store; the onboarding flag lives in the plain store. When the encrypted store is provably unreadable (for example after a device-to-device transfer, or the AEADBadTagException from MOB-1452), newEncrypted deletes and recreates it (MOB-1452 in 3.7.1, narrowed by MOB-1691 in 3.10.1) but nothing resets the onboarding flag. secretState derived READY from the flag alone, so the app opened Home with no wallet behind it and the balance and transaction list spun forever. That is the state the MOB-1836 user is in on 3.10.2: the reported exception predates their update, and 3.10.2 wiped the store silently.

MOB-1565 additionally asked for a bounded retry before the store is condemned, and for the corrupted file to be set aside rather than deleted.

Fix

MOB-1836 (ui-lib)

• SecretState.READY now requires both the onboarding flag and a stored wallet (resolveSecretState); READY with no wallet resolves to NONE, and RootNavGraph already routes NONE to onboarding. The flow deliberately never maps a store failure to null.
• Startup self-heal in WalletRepositoryImpl.init(): when the flag says READY but no wallet is stored, erase the stale SDK databases and persist the flag back to NONE. This is what recovers users who were already wiped by 3.10.1/3.10.2.
• createNewWallet() persists the wallet before marking onboarding READY, so a crash between the two writes can never leave "flag without wallet".
• "Reset Zodl" also purges the quarantine directory.
• A failed Synchronizer.erase on the self-heal path is reported through GlobalCrashReporter as well as logged: "the erase failed and we carried on anyway" is the half-reset state this ticket was reported for, and a Twig.error never leaves the device.

MOB-1565 (preference-impl-android-lib)

• Opening the encrypted store is retried three times with a short backoff before any failure is classified (the MOB-1452 trace showed AEADBadTagException caused by a Keystore operation error).
• Provable corruption now quarantines shared_prefs/<file>.xml and its .bak sibling under no_backup/encrypted_prefs_quarantine/ instead of deleting them. Retention keeps the oldest copy of a filename unconditionally plus the two newest: only the first quarantine can set aside a store that ever held a wallet, because the recovery that follows recreates it empty. A quarantine that fails mid-move is rethrown with the original files left in place — there is no delete fallback. The Keystore master-key alias is deleted only for key-level failures, so a quarantined file stays decryptable by this device for data-level ones.
• A cause chain carrying AOSP's transient marker (android.security.KeyStoreException) is never classified as corruption or as a master-key failure — one shared veto used by both classifiers — so a wedged Keystore daemon rethrows and leaves the store where it is.
• The persisted recreated-marker, written only after a quarantine actually succeeded, bounds the immediate retry within a run: it is cleared as soon as a store opens successfully, so it does not bound repeat quarantines across launches. Surviving repeat quarantines is what the retention policy above is for.

Deliberately left for follow-up tickets: the :crash process running the full app init, a SecretState.ERROR for a persistently unreadable store (which is also where the erase-after-a-failed-ensureReadable question belongs), finer API 33+ KeyStoreException classification, and the pre-existing Twig.error(e) on a PersistableWallet parse failure, which can log decrypted wallet JSON through JSONTokener.syntaxError.

Test plan

• :preference-impl-android-lib:testDebugUnitTest — 35/35 (retry ladder, classification incl. the transient-marker veto for both classifiers, quarantine/prune retention, quarantine-failure rethrow).
• :ui-lib:testZcashmainnetStoreDebugUnitTest — full suite 469/469, incl. new WalletRepositoryImplTest (resolver, real combine, create-wallet ordering, init self-heal).
• :ui-lib:testZcashmainnetInternalDebugUnitTest --tests '*WalletRepository*' — 11/11 after the review fixes, incl. the fail-safe hinge (a wallet read that throws writes no onboarding state and erases nothing) and the failed-erase crash report.
• :preference-impl-android-lib:connectedDebugAndroidTest on Pixel 9 Pro XL (API 35) — 12/12; the two recovery tests now assert the quarantine file, no leftover .bak, and alias retention.
• ktlint + detektAll clean.
• Manual, debug build against the pinned SDK on the emulator:
  • corrupted Tink keyset → three failed attempts logged → quarantine → onboarding, flag reset, SDK DB erased; creating a new wallet from there reaches Home;
  • encrypted store deleted with the flag still READY (the reported user's state) → self-heal → onboarding.

Author

• Self-review your own code in GitHub's web interface
• Add automated tests as appropriate
• Update the manual tests as appropriate
• Check the code coverage report for the automated tests
• Update documentation as appropriate (CHANGELOG.md)
• Run the app and try the changes
• Pull in the latest changes from the main branch and squash your commits before assigning a reviewer

Reviewer

• Check the code with the Code Review Guidelines checklist
• Perform an ad hoc review
• Review the automated tests
• Review the manual tests
• Review the documentation
• Run the app and try the changes

🤖 Generated with Claude Code

https://claude.ai/code/session_016rqq1yYyS5XofPcqE7bZ4r

[linear]

MOB-1836

MOB-1565

[nesence-m]

Pushed 3 review-response commits that were sitting in the local worktree:

• 8a0a3ef9d MOB-1565: harden encrypted store recovery per review
• baf2073b3 MOB-1836: repair the SDK secret store and order the self-heal writes per review
• e01e3d358 MOB-1836: keep wallet startup work out of the :crash process (skip full app init in the secondary :crash process, since WalletRepositoryImpl.init() now erases SDK data and quarantines encrypted stores as part of self-heal)

ktlintFormat/detektAll are clean and preference-impl-android-lib:testDebugUnitTest + ui-lib:testZcashmainnetInternalDebugUnitTest --tests WalletRepositoryImplTest pass. Marked ready for review.

[LukasKorba]

@zodl-review

[zodl-review]

🤖 Claude Review

Coverage

Full diff and file contents reviewed.

Summary

This PR fixes two related bugs: MOB-1836, where a wiped encrypted store left the onboarding flag as READY causing a wallet-less home screen; and MOB-1565, which replaces immediate-delete-on-failure with bounded retry + quarantine-instead-of-delete. It also adds self-heal startup logic in WalletRepositoryImpl.init(), repairs the SDK's shared encrypted store via SdkEncryptedPreferenceRecoveryProvider, and gates full app initialisation away from the :crash secondary process.

Risk Assessment

Medium. The logic is well-reasoned and well-tested, but the recovery path permanently alters user state (erases SDK databases, quarantines secrets). Two correctness concerns below deserve attention before merging.

Key Observations

Correctness

1. TOCTOU in resetOnboardingIfWalletMissing (WalletRepository.kt)

The function receives wallet (already read by init() via getPersistableWallet()) but then re-subscribes to the cold onboardingState flow with onboardingState.first(). If persistOnboardingStateInternal(NONE) from a concurrent restore completes between the getPersistableWallet() call in init() and this first() call, the erase runs against a wallet that's already being restored. The consequence — erasing a live wallet's SDK databases — is severe even if the window is small.

Suggestion: Pass the already-collected onboardingState value into the function (or have init() read it once), instead of re-calling onboardingState.first().

2. ensureEncryptedReadable opens without caching (AndroidPreferenceProvider.kt)

override suspend fun ensureEncryptedReadable(context: Context, filename: String) calls withContext(Dispatchers.IO) { openEncrypted(context, filename) } and discards the returned SharedPreferences. If the SDK subsequently opens the same file via its own EncryptedSharedPreferences.create call, Android may hand it a separate in-memory instance — potentially violating the one-instance-per-file invariant the factory KDoc describes. This is acceptable only if the SDK always opens after ensureEncryptedReadable completes and Android's own in-memory cache is cleared first, but that ordering is not enforced. Worth a comment confirming the intent.

3. isMasterKeyFailure and transient java.security.KeyStoreException (AndroidPreferenceProvider.kt)

if (chain.any { it is KeyStoreException }) return true unconditionally treats java.security.KeyStoreException anywhere in the chain as a master-key failure. The docstring says this comes from Tink's validateAead(), but MasterKey.Builder.build() can also throw java.security.KeyStoreException transiently. If that happens, isMasterKeyFailure returns true and the shared master key is deleted, permanently orphaning both the quarantined file and the SDK store. The retry loop before classification mitigates this, but the assumption that java.security.KeyStoreException is never transient is fragile. Consider adding a comment acknowledging this edge case.

4. quarantineCorruptedEncryptedPreferences in-memory cache "clear" creates a new file (AndroidPreferenceProvider.kt)

After the move, context.getSharedPreferences(filename, Context.MODE_PRIVATE).edit().clear().commit() is called. At this point the original path no longer exists (it was moved), so getSharedPreferences creates a new empty raw SharedPreferences XML file at the original path. This is arguably intentional (warming Android's cache with a blank instance), but if the subsequent create() call in createEncryptedPreferencesWithRecovery fails, an empty <filename>.xml will be left on disk. On the next launch, quarantineEncryptedPreferencesFiles's early-return !xmlFile.exists() && !bakFile.exists() won't fire, and the empty file will be quarantined instead of the marker guard triggering the rethrow path. Low-probability, but worth a comment.

Design

5. resolveSecretState silently maps NEEDS_WARN/NEEDS_BACKUP + wallet → NONE (WalletRepository.kt)

onboardingState == OnboardingState.READY && hasWallet -> SecretState.READY
else -> SecretState.NONE

A user mid-backup (NEEDS_BACKUP) with a wallet present gets NONE, routing to onboarding. The test assertEquals(SecretState.NONE, resolveSecretState(true, OnboardingState.NEEDS_BACKUP, true)) confirms this without explaining why it is correct. If the intent is that NEEDS_BACKUP/NEEDS_WARN only ever appear after READY was first set and thus imply a present wallet, that assumption should be captured in a KDoc comment.

6. ensureEncryptedReadable on AndroidPreferenceFactory is a non-default interface method

AndroidPreferenceFactory gains suspend fun ensureEncryptedReadable(context: Context, filename: String) with no default implementation. Any external implementation of this interface will fail to compile. In practice AndroidPreferenceFactoryImpl appears to be the only implementation, but if AndroidPreferenceFactory is part of the public API surface of preference-impl-android-lib, this is a breaking change.

7. Step-numbers in quarantineCorruptedEncryptedPreferences KDoc

The KDoc uses numbered implementation steps (1. Move…, 2. Create…, 3. Clear…, 4. Delete…). Per the project rubric, step-numbers in comments are reserved for tracing a published spec. These steps describe internal implementation order — prose or inline comments would be more appropriate.

Minor / Non-blocking

• Scope leak in tests: WalletRepositoryImpl defines both private val sharingScope and internal var scope, neither with a cancel() path. In tests, repository.scope is replaced but the original scope instance and sharingScope are both abandoned. Not a production issue (singleton), but each test method leaks two uncancelled scopes.
• Quarantine pruning test coverage: ResetZashiUseCase.purgeQuarantine() is called on reset, but there is no test verifying the quarantine directory is correctly pruned to prevent unbounded growth. Non-blocking.

Areas of Disagreement

None between the two reviewers on substance — both flagged Medium risk. Reviewer A raised more implementation-level detail; Reviewer B's observations were all confirmed against the file contents and are reflected above.

---

_{Automated review by zodl-review bot · Models: us.anthropic.claude-sonnet-4-6 + us.meta.llama4-maverick-17b-instruct-v1:0 (synthesis) · [Silent pilot — feedback welcome]}

[nesence-m]

Thanks — addressed the @zodl-review findings in 7bd859c (preference lib) and 247d4c6 (ui-lib).

1. TOCTOU in resetOnboardingIfWalletMissing — fixed three ways. init() now reads the wallet and the onboarding flag once and passes both in (no second onboardingState.first()); init(), createNewWallet() and restoreWallet() all run under one new walletMutation mutex, so the self-heal and a create/restore can no longer interleave at all; and the wallet is re-read immediately before Synchronizer.erase, so a wallet stored while the SDK secret store was being repaired keeps its databases. New test initSkipsEraseWhenAWalletIsStoredWhileSelfHealing drives the race shape through a provider that reports no wallet on the first read and a stored wallet afterwards — it fails without the re-check (verified) and asserts the erase never runs.

2. ensureEncryptedReadable caching — ordering is enforced by the call site, now documented in the interface KDoc: the opened store is never handed out, so no second AndroidPreferenceProvider (and no second serializing dispatcher) exists for that file; the app's only caller is the wallet-less self-heal, which awaits it before Synchronizer.erase and only runs on the path where no wallet is stored, so no synchronizer can be holding the SDK store open.

3. isMasterKeyFailure — the unconditional java.security.KeyStoreException short-circuit is gone. An AOSP transient-HAL marker (android.security.KeyStoreException, matched by simple name) anywhere in the chain now vetoes the whole classification, not just the InvalidKeyException branch, so that shape can no longer cost the shared master key. KDoc records that the classification only ever runs after the retry ladder is exhausted, plus the residual assumption: Tink's validateAead() folds its inner cause into the message rather than the cause chain, so a transient HAL error surfacing through it stays indistinguishable by type — the retry ladder is the only mitigation there. New test covers the wrapped-marker case.

4. Empty XML left by the cache clear — the file the clear commits back to the vacated path is deleted right after, via a shared deleteEncryptedPreferencesFiles helper (which also replaces the two ad-hoc deletes in the quarantine-failed fallback), so a failed recreate leaves nothing for the next launch to quarantine instead of the marker guard rethrowing. Covered by the empty file left by clearing the cache is not quarantined on the next launch.

5. resolveSecretState and NEEDS_WARN/NEEDS_BACKUP — mapping is intentional and now stated in KDoc: nothing writes those ordinals any more (WalletRepositoryImpl only ever persists NONE or READY; they are leftovers of an earlier onboarding flow) and both mean onboarding never completed, so a wallet found under them belongs in onboarding. Added the NEEDS_WARN case to the test alongside NEEDS_BACKUP.

6. Non-default ensureEncryptedReadable — left as is. AndroidPreferenceFactoryImpl is the only implementation in the repo (it is private, reached solely through the AndroidPreferenceProvider.Factory companion delegate), and preference-impl-android-lib is not published, so there is no external implementor to break.

7. Step-numbered KDoc — rewritten as prose.

Minor: WalletRepositoryImplTest now tracks every scope it creates or installs and cancels them in teardown. sharingScope is still not cancelled — it is private production state with no shutdown path, and its only jobs are WhileSubscribed sharing coroutines that idle out. Quarantine pruning: added purging removes every quarantined copy and every marker, covering what ResetZashiUseCase.purgeQuarantine() calls (purgeEncryptedPreferencesQuarantine now takes the directory so it is reachable from a JVM test).

Gates: ktlintFormat detektAll clean, :preference-impl-android-lib:testDebugUnitTest green (quarantine 6, recovery 18), full :ui-lib:testZcashmainnetInternalDebugUnitTest green (472 tests, 0 failures).

[zodl-review]

🤖 Claude Review

Coverage

Full diff and file contents reviewed at head_sha=247d4c66.

Summary

This PR fixes two bugs: MOB-1836 (app opens a wallet-less home screen when the encrypted store was silently wiped) and MOB-1565 (transient Keystore errors deleted the seed instead of retrying). It adds resolveSecretState (READY requires both flag + stored wallet), a startup self-heal in WalletRepositoryImpl.init(), bounded retry + quarantine-instead-of-delete for the encrypted prefs store, SDK store repair via SdkEncryptedPreferenceRecoveryProvider, and a guard that skips full app init in the :crash secondary process. Test coverage is solid across all new paths.

Risk Assessment

Medium. Core correctness issues are well-addressed. Two narrower items below deserve attention before merge.

Key Observations

Correctness / Bugs

1. AndroidPreferenceFactory.ensureEncryptedReadable — breaking interface addition without default implementation

AndroidPreferenceProvider.kt adds to the existing public AndroidPreferenceFactory interface:

suspend fun ensureEncryptedReadable(context: Context, filename: String)

No default body is provided. Any external code implementing AndroidPreferenceFactory (e.g. test doubles outside this repo) will now fail to compile. Per standard API evolution rules, adding a method to an existing interface without a default is a breaking change. The AndroidPreferenceProvider.Factory companion delegates to the private AndroidPreferenceFactoryImpl, so in-repo call sites compile fine — the risk is external implementors.

2. quarantineCorruptedEncryptedPreferences: marker-write failure is silently swallowed

In AndroidPreferenceProvider.kt:

runCatching {
    quarantineDir.mkdirs()
    recreatedMarkerFile(quarantineDir, filename).createNewFile()
}

createNewFile() returns false on failure (e.g. storage full) and that result is discarded inside runCatching. If the marker is never written, the once-per-filename quarantine guard never fires, and a second quarantine will run on the next launch. This is a pre-existing fragility but the new quarantine path makes it worth a Twig.error log when createNewFile() returns false.

Resources / Concurrency

3. sharingScope is never cancelled and not tracked in test teardown

WalletRepository.kt:

private val sharingScope = CoroutineScope(Dispatchers.IO + SupervisorJob())

sharingScope is a private val with no cancel() call. The secretState, isIronwoodAnnouncementShown, fastestEndpoints, and walletRestoringState flows all stateIn on sharingScope. In production this is a singleton so it doesn't leak. However, WalletRepositoryImplTest cancels repository.scope (the mutable one) via scopes += repository.scope, but never tracks sharingScope. Tests that collect secretState hold a StateFlow backed by a scope that is never cancelled, potentially leaking coroutines across test cases.

4. scope replacement ordering in tests is fragile but not currently broken

WalletRepository.kt:

@set:RestrictTo(RestrictTo.Scope.TESTS)
@VisibleForTesting
internal var scope: CoroutineScope = CoroutineScope(Dispatchers.IO + SupervisorJob())

In WalletRepositoryImplTest, useTestScope replaces scope after newRepository already does scopes += repository.scope (capturing the old production scope). If any test called init() or createNewWallet() before useTestScope, those launches would run on the production Dispatchers.IO scope and advanceUntilIdle would be ineffective. All current tests call useTestScope before any triggering call, so this is not currently broken, but it is an unenforced ordering dependency.

Design / Refactoring

5. walletMutation mutex correctly serializes self-heal against wallet creation/restore

WalletRepository.kt:

private val walletMutation = Mutex()

Confirmed: init(), createNewWallet(), and restoreWallet() all acquire walletMutation before reading/writing wallet state, preventing the self-heal erase from racing with an in-progress wallet store. The secondary re-read of the wallet immediately before Synchronizer.erase in resetOnboardingIfWalletMissing provides an additional guard for the narrow window between sdkEncryptedPreferenceRecoveryProvider.ensureReadable() and the erase.

6. resolveSecretState correctly combines onboarding flag and wallet presence

WalletRepository.kt:

internal fun resolveSecretState(
    isConfigurationLoaded: Boolean,
    onboardingState: OnboardingState,
    hasWallet: Boolean,
): SecretState =
    when {
        !isConfigurationLoaded -> SecretState.LOADING
        onboardingState == OnboardingState.READY && hasWallet -> SecretState.READY
        else -> SecretState.NONE
    }

Logic is correct and well-tested. The onboardingWithWallet flow correctly short-circuits the encrypted wallet store read when the flag is not READY, preventing startup latency on fresh installs.

Suggestions

• Observation 1 (blocking if external implementors exist): Add a default implementation to AndroidPreferenceFactory.ensureEncryptedReadable, e.g. suspend fun ensureEncryptedReadable(context: Context, filename: String): Unit = Unit or move it to a separate interface, so existing implementations of AndroidPreferenceFactory don't fail to compile.

• Observation 2 (non-blocking): Replace the silent runCatching around recreatedMarkerFile(...).createNewFile() with an explicit check: log Twig.error when it returns false so storage-full marker-write failures are observable in crash reports.

• Observation 3 (non-blocking): Track sharingScope in the test scopes list the same way scope is tracked (scopes += repository.sharingScope), or expose it via the same @VisibleForTesting annotation, so test teardown cancels all coroutines the repository owns.

Areas of Disagreement

None — both reviewers were in agreement on risk level (Medium) and the non-blocking nature of the concurrency observations. Reviewer A raised the AndroidPreferenceFactory breaking-interface concern (Observation 1); Reviewer B did not address it. Both reviewers confirmed the TOCTOU fix, mutex serialization, and resolveSecretState logic are correct.

---

_{Automated review by zodl-review bot · Models: us.anthropic.claude-sonnet-4-6 + us.meta.llama4-maverick-17b-instruct-v1:0 (synthesis) · [Silent pilot — feedback welcome]}

[nesence-m]

Addressed 2 of the 6 items from the latest @zodl-review pass in 9379b807b:

• Observation 1 (breaking interface addition): AndroidPreferenceFactory.ensureEncryptedReadable now defaults to a no-op, so out-of-repo implementors don't fail to compile.
• Observation 3 (sharingScope leak in tests): sharingScope is now @VisibleForTesting internal val, and WalletRepositoryImplTest.newRepository registers it alongside scope for teardown cancellation.

Not touched in this round — flagging for a maintainer/reviewer call since they touch the quarantine/erase paths directly:

• Observation 2: quarantineCorruptedEncryptedPreferences's marker-write createNewFile() result is discarded inside runCatching; suggestion was to log Twig.error on a false return.
• Observation 4: scope replacement ordering in tests is an unenforced dependency (not currently broken, no concrete fix suggested).

ktlintFormat/detektAll clean, preference-impl-android-lib:testDebugUnitTest + ui-lib:testZcashmainnetInternalDebugUnitTest --tests WalletRepositoryImplTest pass.

[zodl-review]

🤖 Claude Review

Coverage

Full diff and file contents reviewed.

Summary

This PR fixes two bugs: MOB-1836 (app opened a wallet-less home screen after the encrypted store was silently wiped) and MOB-1565 (transient Keystore errors immediately deleted the seed). It adds resolveSecretState requiring both flag + wallet for READY, a startup self-heal in WalletRepositoryImpl.init(), bounded retry + quarantine-instead-of-delete for the encrypted prefs store, SDK store repair via SdkEncryptedPreferenceRecoveryProvider, and a :crash process guard. The prior round's blocking issues are resolved; what remains is a narrow silent-failure mode in the quarantine path, one design-level known limitation, and a few nits.

Risk Assessment

Low-Medium. The changes touch sensitive recovery paths (encrypted secrets, seed storage, SDK databases). The core logic is sound and well-tested; the residual items below are non-blocking.

Key Observations

Correctness / Bugs

1. quarantineCorruptedEncryptedPreferences: createNewFile() false-return is silently swallowed

In AndroidPreferenceProvider.kt, the marker write block reads:

runCatching {
    quarantineDir.mkdirs()
    recreatedMarkerFile(quarantineDir, filename).createNewFile()
}

createNewFile() returns false if the file already exists or if the filesystem refuses creation (e.g. storage full), and that boolean is discarded inside runCatching with no logging. If the marker is never written on a fresh quarantine, the once-per-filename guard never fires and the store can be re-quarantined on the next launch. A Twig.error on !createNewFile() && !recreatedMarkerFile(quarantineDir, filename).exists() (the .exists() branch handles the already-exists case benignly) would surface the storage-full scenario without false noise.

2. resetOnboardingIfWalletMissing: proceeds to erase even when ensureReadable fails

In WalletRepository.kt:

runCatching { sdkEncryptedPreferenceRecoveryProvider.ensureReadable() }
    .onFailure { Twig.error(it) { "Repairing the SDK secret store failed; continuing" } }

if (persistableWalletProvider.getPersistableWallet() != null) { ... }

runCatching { Synchronizer.erase(application, ZcashNetwork.fromResources(application)) }
    .onFailure { Twig.error(it) { "Erasing stale wallet data failed; continuing" } }

If ensureReadable fails, the code continues to Synchronizer.erase. Since Synchronizer.erase also opens the SDK store, a persistently unreadable store could cause this path to crash-loop. This is acknowledged in the KDoc as a known limitation (SecretState.ERROR for a persistently unreadable store is deliberately deferred). Non-blocking as-is given the runCatching around erase, but worth a follow-up ticket.

Design

3. SdkEncryptedPreferenceRecoveryProvider interface — ensureReadable() has no doc comment

SdkEncryptedPreferenceRecoveryProvider.kt:

interface SdkEncryptedPreferenceRecoveryProvider {
    suspend fun ensureReadable()
}

The KDoc explaining intent lives on SdkEncryptedPreferenceRecoveryProviderImpl, not on the interface method. The interface's contract (repair-in-place, no provider cached, caller must complete before SDK opens the file) is non-obvious from the method signature alone. Moving or duplicating a one-liner to ensureReadable() on the interface would help future implementors.

4. RecordingPreferenceProvider visibility

In WalletRepositoryImplTest.kt:

internal class RecordingPreferenceProvider(
    private val events: MutableList<String>
) : PreferenceProvider {

This class is only referenced within the same file. Its visibility can be private without affecting anything.

Confirmed Resolved (from prior review)

• sharingScope test teardown: both repository.scope and repository.sharingScope are now cancelled in teardown (scopes += repository.scope / scopes += repository.sharingScope). ✓
• createNewWallet now writes wallet + restoring state before setting onboarding to READY, confirmed by the test asserting listOf("wallet", "restoring=INITIATING", "onboarding=3"). ✓
• walletMutation Mutex serializes init against createNewWallet/restoreWallet. ✓

Suggestions

1. Address the createNewFile() false-return silently: add a conditional Twig.error when !createNewFile() && !recreatedMarkerFile(quarantineDir, filename).exists() in quarantineCorruptedEncryptedPreferences. This catches storage-full without false positives on the already-exists case.

2. Nit: Move or duplicate the intent KDoc from SdkEncryptedPreferenceRecoveryProviderImpl to the ensureReadable() method on the SdkEncryptedPreferenceRecoveryProvider interface.

3. Non-blocking: Track the ensureReadable-failed-but-erase-proceeds scenario in a follow-up ticket alongside the SecretState.ERROR item already noted in the PR description.

4. Nit: RecordingPreferenceProvider in WalletRepositoryImplTest.kt can be private since it has no callers outside that file.

---

_{Automated review by zodl-review bot · Models: us.anthropic.claude-sonnet-4-6 + us.meta.llama4-maverick-17b-instruct-v1:0 (synthesis) · [Silent pilot — feedback welcome]}

[nesence-m]

Follow-up in 32782b4ca: Observation 2 addressed — quarantineCorruptedEncryptedPreferences now logs via Twig.error when the recreated-marker's createNewFile() returns false, no other behaviour change. ktlint/detekt clean, preference-impl-android-lib:testDebugUnitTest passes.

[nesence-m]

Follow-up in 533443e15, addressing the remaining non-blocking nits from the latest @zodl-review pass (which predates the 32782b4ca marker-write fix, already covering its Observation 1):

• Moved the intent KDoc from SdkEncryptedPreferenceRecoveryProviderImpl onto SdkEncryptedPreferenceRecoveryProvider.ensureReadable() so the contract is visible on the interface.
• Narrowed RecordingPreferenceProvider in WalletRepositoryImplTest.kt from internal to private (no external callers).

Left for a maintainer call, since it touches the erase path directly: the bot's Observation 2 — resetOnboardingIfWalletMissing proceeds to Synchronizer.erase even when ensureReadable() fails, which the bot suggests tracking in a follow-up ticket alongside the already-noted SecretState.ERROR deferral.

ktlint/detekt clean, ui-lib:testZcashmainnetInternalDebugUnitTest --tests WalletRepositoryImplTest passes.

[zodl-review]

🤖 Claude Review

Coverage

Full diff and file contents reviewed at head_sha=533443e1.

Summary

This PR fixes MOB-1836 (wallet-less home screen when encrypted store is silently wiped) and MOB-1565 (transient Keystore errors immediately deleted the seed). It adds resolveSecretState requiring both flag + stored wallet for READY, a startup self-heal in WalletRepositoryImpl.init(), bounded retry + quarantine-instead-of-delete for the encrypted prefs store, SDK store repair via SdkEncryptedPreferenceRecoveryProvider, and a :crash process guard. All blocking concerns from prior rounds have been resolved.

Risk Assessment

Low. The remaining observations are narrow, non-blocking, or design-level. Both reviewers agreed the core logic is sound and well-tested.

Key Observations

Correctness / Bugs

1. moveFile — copy-then-delete fallback is silent on failure

In EncryptedPreferenceQuarantine.kt:

private fun moveFile(source: File, destination: File) {
    if (!source.renameTo(destination)) {
        source.copyTo(destination, overwrite = true)
        source.delete()
    }
}

source.copyTo(...) can throw (out-of-space, I/O error), and source.delete() returns a boolean that is silently discarded. If the copy succeeds but the delete fails, the original corrupted file remains alongside the quarantined copy. The outer quarantineCorruptedEncryptedPreferences onFailure fallback only catches exceptions thrown by quarantineEncryptedPreferencesFiles itself; an exception inside moveFile propagates out through it, but a false delete-return does not. Wrapping the copy+delete in runCatching with a Twig.error on failure would close the gap.

2. createEncryptedPreferencesWithRecovery — maxAttempts = 0 triggers checkNotNull crash mid-execution

val cause = checkNotNull(lastFailure) { "maxAttempts must be at least 1" }

If maxAttempts = 0, the loop body never runs and lastFailure remains null, crashing here with an IllegalStateException. A require(maxAttempts >= 1) at function entry would catch this at the call site. Non-blocking given all callers are internal, but cleaner as a precondition.

3. resetOnboardingIfWalletMissing — proceeds to erase even when ensureReadable fails

runCatching { sdkEncryptedPreferenceRecoveryProvider.ensureReadable() }
    .onFailure { Twig.error(it) { "Repairing the SDK secret store failed; continuing" } }

If ensureReadable fails, execution continues to Synchronizer.erase. This is documented as a known limitation; consider a follow-up ticket to track it. Non-blocking.

Design / Refactoring

4. SdkEncryptedPreferenceRecoveryProviderImpl is public without internal

In SdkEncryptedPreferenceRecoveryProvider.kt:

class SdkEncryptedPreferenceRecoveryProviderImpl(
    private val context: Context
) : SdkEncryptedPreferenceRecoveryProvider {

The Impl class has no internal modifier. The interface is intentionally public for the Koin binding, but the concrete implementation leaking as a public type allows external code to instantiate it directly. Marking it internal would be a clean nit.

5. clearSharedPrefs — call ordering of purgeQuarantine is implicit

In ResetZashiUseCase.kt:

private suspend fun clearSharedPrefs() {
    standardPreferenceProvider().clearPreferences()
    encryptedPreferenceProvider().clearPreferences()
    encryptedPreferenceProvider.purgeQuarantine()
}

The ordering is correct (clear first, then purge markers), but there is no comment explaining why purgeQuarantine must follow clearPreferences. A single-line comment would guard against future reordering.

Suggestions

• moveFile: Wrap copyTo/delete in runCatching, log Twig.error on failure, and let the exception propagate to the outer onFailure path that falls back to deleteEncryptedPreferencesFiles. Non-blocking.
• createEncryptedPreferencesWithRecovery: Add require(maxAttempts >= 1) { "maxAttempts must be at least 1" } at function entry. Nit.
• resetOnboardingIfWalletMissing: Track the known limitation (continuing to erase after ensureReadable failure) in a follow-up ticket. Non-blocking.
• SdkEncryptedPreferenceRecoveryProviderImpl: Mark internal. Nit.
• clearSharedPrefs: Add a comment explaining why purgeQuarantine must follow clearPreferences. Nit.

---

_{Automated review by zodl-review bot · Models: us.anthropic.claude-sonnet-4-6 + us.meta.llama4-maverick-17b-instruct-v1:0 (synthesis) · [Silent pilot — feedback welcome]}

[nesence-m]

Closing the loop on the erase-after-failed-ensureReadable() item raised in the earlier @zodl-review passes: this is intentionally left as-is for this PR and is tracked by the SecretState.ERROR follow-up already noted on MOB-1836, no code change here.

The latest @zodl-review pass (Low risk, nits only, incl. the moveFile copy-then-delete note) needs no further round.

[zodl-review]

🤖 Claude Review

Coverage

Full diff and file contents reviewed at head_sha=533443e1.

Summary

This PR fixes two bugs: MOB-1836 (wallet-less home screen after the encrypted store is silently wiped) by requiring both the onboarding flag and a stored wallet for SecretState.READY, and MOB-1565 (seed destroyed on transient Keystore errors) by adding bounded retry + quarantine-instead-of-delete. It also adds a startup self-heal in WalletRepositoryImpl.init(), repairs the SDK's shared encrypted store via SdkEncryptedPreferenceRecoveryProvider, and skips full app init in the :crash secondary process.

Risk Assessment

Low. Core logic is sound and well-tested. Remaining items are narrow correctness edge-cases and design nits.

Key Observations

Correctness / Bugs

1. moveFile copy-then-delete: source.delete() false-return is silently discarded

In EncryptedPreferenceQuarantine.kt, the moveFile function reads:

private fun moveFile(source: File, destination: File) {
    if (!source.renameTo(destination)) {
        source.copyTo(destination, overwrite = true)
        source.delete()
    }
}

After a cross-filesystem copy, if source.delete() returns false (e.g., storage-full), the corrupted source file silently remains. The marker guard prevents re-quarantine on next launch, but the stale file persists. Adding a Twig.error on !source.delete() would surface real storage-full scenarios. Non-blocking given the marker guard.

2. createEncryptedPreferencesWithRecovery — maxAttempts = 0 precondition checked post-loop

In AndroidPreferenceProvider.kt, the function uses:

val cause = checkNotNull(lastFailure) { "maxAttempts must be at least 1" }

This is a post-loop checkNotNull rather than a require at entry. A caller passing maxAttempts = 0 gets a confusing IllegalStateException with a misleading message at the wrong site. All callers are internal so this cannot be triggered externally, but a require(maxAttempts >= 1) at function entry would be cleaner. Nit.

3. SdkEncryptedPreferenceRecoveryProviderImpl missing internal modifier

SdkEncryptedPreferenceRecoveryProvider.kt reads:

class SdkEncryptedPreferenceRecoveryProviderImpl(
    private val context: Context
) : SdkEncryptedPreferenceRecoveryProvider {

No internal modifier. The Koin registration in ProviderModule.kt uses singleOf(::SdkEncryptedPreferenceRecoveryProviderImpl), so the Impl being public allows callers to construct it directly, bypassing the singleton. If cross-module test doubles aren't needed, adding internal removes unintended public API surface. Non-blocking.

4. SdkEncryptedPreferenceRecoveryProvider.ensureReadable() missing doc comment on interface

The interface declares ensureReadable() but the doc comment lives only on the interface-level KDoc block above it. The method itself has no @param/@throws contract documentation at the member level. Minor, but the contract (callers must await before anything opens the SDK store) is subtle enough to warrant an inline doc. Nit.

Design / Refactoring

5. isCrashProcess() literal duplication — sync comment is the right fix

ZcashApplication.kt has:

private fun isCrashProcess() = ProcessNameCompat.getProcessName(this).endsWith(CRASH_PROCESS_NAME_SUFFIX)

and at the bottom:

private const val CRASH_PROCESS_NAME_SUFFIX = ":crash"

The KDoc explains why the literal is deliberately repeated. Adding a // keep in sync with GlobalCrashReporter.CRASH_PROCESS_NAME_SUFFIX inline note next to the constant would make a future rename visible via grep. Nit.

Suggestions

1. Observation 1 (moveFile): Wrap source.delete() — if (!source.delete()) Twig.error { "Failed to delete source file after copy: $source" }. The marker guard already prevents double-quarantine; the log would surface storage-full scenarios.

2. Observation 2 (maxAttempts): Add require(maxAttempts >= 1) at the top of createEncryptedPreferencesWithRecovery, replacing the post-loop checkNotNull. Nit.

3. Observation 3 (SdkEncryptedPreferenceRecoveryProviderImpl): Add internal to the class declaration. One-character change, removes unintended public API surface. Non-blocking.

4. Observation 4 (ensureReadable): Add a doc comment to SdkEncryptedPreferenceRecoveryProvider.ensureReadable() describing its ordering contract. Nit.

5. Observation 5 (CRASH_PROCESS_NAME_SUFFIX): Add // keep in sync with GlobalCrashReporter.CRASH_PROCESS_NAME_SUFFIX next to the constant. Nit.

---

_{Automated review by zodl-review bot · Models: us.anthropic.claude-sonnet-4-6 + us.meta.llama4-maverick-17b-instruct-v1:0 (synthesis) · [Silent pilot — feedback welcome]}

[nesence-m]

Addressed all items from the latest @zodl-review pass in 71559314b:

1. moveFile copy-then-delete: now logs via Twig.error when source.delete() returns false, naming the file.
2. createEncryptedPreferencesWithRecovery: added require(maxAttempts >= 1) at entry; dropped the misleading post-loop checkNotNull message (kept checkNotNull(lastFailure) for the smart cast).
3. SdkEncryptedPreferenceRecoveryProviderImpl: marked internal (Koin singleOf binding is same-module, still compiles).
4. ensureReadable() interface doc: already covered by the prior round's KDoc move onto the method itself; no further change needed.
5. CRASH_PROCESS_NAME_SUFFIX: added a KDoc note (not a // comment, per this repo's style) that it must stay in sync with GlobalCrashReporter's own copy.

Gates: ktlintFormat/detektAll clean, preference-impl-android-lib:testDebugUnitTest and ui-lib:compileZcashmainnetInternalDebugKotlin both pass.

[LukasKorba]

Reviewed all three passes and the deferred items. Answers to your three maintainer calls at the bottom — but first, two things the bot missed across all three passes that I do want fixed here. Both are inside code this PR writes, both on the "can this orphan the only copy of a spending key" axis. Neither is a regression against maint/v3.10.x (which deletes the store outright), so this is still strictly safer than what's on the branch — but both fixes are small.

Blocking

1. isUnrecoverableCorruption doesn't apply the transient-HAL veto that isMasterKeyFailure does

AndroidPreferenceProvider.kt:365-374 vs :397-403.

internal fun isUnrecoverableCorruption(exception: Exception): Boolean =
    generateSequence<Throwable>(exception) { it.cause }
        .take(CAUSE_CHAIN_LIMIT)
        .any { ... || it is InvalidKeyException || ... }        // :372 — no veto

internal fun isMasterKeyFailure(exception: Exception): Boolean {
    val chain = ...
    val hasAndroidKeyStoreFailure =
        chain.any { it !is KeyStoreException && it.javaClass.simpleName == "KeyStoreException" }
    if (hasAndroidKeyStoreFailure) return false                  // :401 — veto present
    ...
}

Your own KDoc at :376-395 says AOSP wraps a transient HAL error as InvalidKeyException("Keystore operation failed") carrying android.security.KeyStoreException. The code acts on that for key deletion and ignores it for the quarantine decision.

Sequence — Keystore daemon wedged >600ms after an OS update or a StrongBox hiccup, i.e. the MOB-1565 scenario:

1. create() throws that shape on all three attempts (:431-445, 200+400ms total).
2. isUnrecoverableCorruption → true (chain contains InvalidKeyException).
3. :451 quarantine(cause) — the seed blob leaves shared_prefs.
4. isMasterKeyFailure → false, so the key survives. Good, but too late for reachability.
5. getPersistableWallet() → null, onboarding is READY → WalletRepository.kt:299 writes NONE, :309 erases SDK databases.
6. User lands on onboarding. Seed is in no_backup/encrypted_prefs_quarantine/, unreachable from any UI, excluded from Auto Backup, and one Reset Zodl away from purgeQuarantine().

Failing safe here means rethrowing and leaving the store in place: the user gets the (already-deferred) splash hang, recoverable by relaunching once the Keystore settles. Quarantine is not user-recoverable. Please extract the veto into one shared hasTransientAndroidKeyStoreMarker(chain) so the two classifiers can't drift again. EncryptedPreferenceRecoveryTest.kt:83-90 pins the AOSP shape for isMasterKeyFailure and has no counterpart for isUnrecoverableCorruption — that's why this survived three passes.

2. pruneQuarantine deletes oldest-first, i.e. the only copy that has a wallet in it

EncryptedPreferenceQuarantine.kt:110-116, QUARANTINE_KEEP_NEWEST = 3:

xmlFiles.sortedByDescending { it.name }.drop(keepNewest).forEach { ... delete() }

After the first quarantine the live store is a fresh empty one, so every subsequent quarantine of that filename sets aside an empty or newly-created store — while the first entry, the one holding the seed, is the oldest and therefore the first thing drop(3) deletes.

Flaky hardware Keystore: launch 1 quarantines the real seed (T1). The recreated-marker is cleared the same run (:260 deletes it on any successful open, and the post-quarantine create() on an empty store basically always succeeds), so nothing prevents recurrence. Launches 2-4 quarantine T2/T3/T4. On the fourth, sortedByDescending keeps T4/T3/T2 and deletes T1. Four app launches to total loss.

Cheapest defensible fix: keep the oldest entry for a filename unconditionally, plus the newest N-1. A few KB of XML against total loss.

Related — the PR description says "at most one recreate per filename per process", but because :260 clears the marker on any successful open, it bounds the immediate retry, not repeat quarantine across launches. Worth correcting the description.

3. The quarantine-failure fallback deletes the user's ciphertext

AndroidPreferenceProvider.kt:316-319:

.onFailure { failure ->
    Twig.error(failure) { "Quarantining encrypted preferences $filename failed; deleting instead" }
    runCatching { deleteEncryptedPreferencesFiles(sharedPrefsDir, filename) }
}

moveFile (EncryptedPreferenceQuarantine.kt:87-90) falls back to copyTo(overwrite = true) + delete() when renameTo fails. If copyTo throws mid-write — low disk being the obvious trigger, and low disk is also a plausible reason renameTo failed — the exception unwinds into this onFailure, which then deletes the original, leaving a truncated partial in quarantine. Seed gone, log line cheerful about it.

Three lines: when quarantine fails, don't delete — rethrow so createEncryptedPreferencesWithRecovery propagates and the store stays put for a later attempt. "Recovery still completes" (KDoc at :286) isn't worth an unrecoverable seed. If you want to keep a fallback, gate it on quarantinedXml.exists() && quarantinedXml.length() == originalLength.

(The bot flagged moveFile in pass 3 but chased the delete()-returns-false hazard, which is harmless since :335 deletes that path anyway.)

Your three deferred items

Erase after a failed ensureReadable — agreed, follow-up ticket. By WalletRepository.kt:301 we've established twice (:296, :304) that no wallet is stored, and Synchronizer.erase destroys SDK databases — block cache and derived note data, all reconstructible from the seed, which isn't in there. Skipping the erase would be no safer, just the crash-loop MOB-1836 was chasing. File it with the SecretState.ERROR ticket, since the honest fix is surfacing "your secret store is unreadable, do not reset" rather than routing silently to onboarding. One thing before merge though: the erase's .onFailure should log at a level that reaches crash reporting — "erase failed and we continued anyway" is the state that produces the next MOB-1836.

Test scope replacement ordering — genuinely fine, close it. A test calling init() before useTestScope would run the self-heal on real Dispatchers.IO, advanceUntilIdle() would no-op, and it'd pass vacuously. Test-integrity risk only, all current tests comply, and the bot offered no concrete fix. Not refactoring a passing test file on a maint branch.

sharingScope never cancelled in production — fine. singleOf + init() called once from ZcashApplication.onCreate. Process-lifetime scope on a process-lifetime singleton. Your reasoning is right.

What I checked and found safe

The retry-then-rethrow default is genuinely fail-safe (:449 requires both isRecreateAllowed and isUnrecoverableCorruption, else :455 throw cause), and PreferenceHolder/PreferenceProviderCache cache nothing on throw so a later launch retries cleanly. A read failure cannot surface as hasWallet = false — there's no try/runCatching anywhere in getPersistableWallet() → BaseNullableStorageProvider.get() → PersistableWalletPreferenceDefault.getValue, so null always means "key absent". That's the most important property in init() and it rests entirely on the absence of a catch three frames down — one coEvery { … } throws test would pin it against a future well-meaning runCatching. Device-locked/UserNotAuthenticatedException isn't reachable (MasterKey.Builder:267-271 never sets setUserAuthenticationRequired). Quarantine atomicity is sound: same filesystem so renameTo is an atomic rename, the .bak sibling is taken along (:76-78, correct and non-obvious — leaving it would let loadFromDisk resurrect the corrupt data), and I walked both crash windows (die after move before marker; die after marker before recreate) — neither loops and neither erases a usable store. Master-key deletion is correctly narrowed by the veto at :399-401, which is what makes quarantine meaningfully non-destructive at all.

TOCTOU (finding 1 pass 1) is over-delivered — single read at :257-260, walletMutation over init/create/restore, re-read before erase at :304-307, pinned by initSkipsEraseWhenAWalletIsStoredWhileSelfHealing. Findings 2, 4, 5 and both minors check out; I verified the resolveSecretState one against the pre-image and the old code mapped both ordinals to NONE already, so there's genuinely no behaviour change.

No log statement this PR adds leaks anything — :317, :325, :440, :450, :454 take only filename and counters. Separate pre-existing ticket though: WalletRepository.kt:262-264 does Twig.error(e) on a PersistableWallet parse failure, and Android's JSONTokener.syntaxError appends the entire input to the message — which at that point is the decrypted wallet JSON. Same catch existed before this PR; flagging it because this PR makes malformed-decrypt a routine path rather than an anomaly.

Test gaps

Three, and they map exactly onto the above: no isUnrecoverableCorruption assertion for the AOSP-wrapped transient shape; pruneQuarantine is tested for "keeps the newest" but nothing asserts which copy holds a wallet, so the retention policy's actual purpose is untested; and nothing tests the fail-safe hinge (throw → no onboarding write, no erase). The WalletRepositoryImplTest decision table is otherwise complete — all four quadrants, create-ordering, the race shape, the not-READY short-circuit.

One process note

The bot's three passes converged on nits while the two highest-consequence issues in the diff went unraised in all three. Both are only visible by reading the two classifiers against each other and by asking what pruneQuarantine is retaining rather than whether it retains correctly. Worth calibrating how much weight its "Risk: Low" carries on this kind of change.

Also: the user is told nothing (:298 is a Twig.warn) before landing on onboarding. Not a blocker — but a user who assumes a reinstall and taps Reset Zodl hits ResetZashiUseCase.kt:94 → purgeQuarantine() → deleteRecursively(), which is the last mile of the loss chain in items 1 and 2.

[zodl-review]

🤖 Claude Review

Coverage

Full diff and file contents reviewed at head_sha=71559314.

Summary

This PR fixes two bugs: MOB-1836 (wiped encrypted store left the onboarding flag as READY, causing a wallet-less home screen) and MOB-1565 (transient Keystore errors immediately destroyed the seed). It adds resolveSecretState requiring both flag and stored wallet for READY, a startup self-heal in WalletRepositoryImpl.init(), bounded retry + quarantine-instead-of-delete for the encrypted prefs store, SDK store repair, and a :crash process guard. Well-tested throughout.

Risk Assessment

Low. All prior blocking items resolved. Remaining items are narrow edge cases and documentation nits.

Key Observations

Correctness / Bugs

1. Marker written even when quarantine falls back to delete

In AndroidPreferenceProvider.kt, quarantineCorruptedEncryptedPreferences runs in three independent runCatching blocks. If quarantineEncryptedPreferencesFiles throws and the fallback deleteEncryptedPreferencesFiles runs, the marker-write block immediately after still executes:

runCatching {
    quarantineDir.mkdirs()
    val markerCreated = recreatedMarkerFile(quarantineDir, filename).createNewFile()

This creates the marker even though no quarantined file exists. On the next launch marker.exists() is true, so isRecreateAllowed = false, and a persistently corrupt store is rethrown without recovery. The delete fallback also wipes the quarantine dir, so in practice these race, but the ordering is fragile. The marker write should be conditional on the quarantine succeeding.

2. isMasterKeyFailure and isUnrecoverableCorruption both match bare InvalidKeyException

isUnrecoverableCorruption returns true for InvalidKeyException (confirmed in the any { ... } block). isMasterKeyFailure also returns true for bare InvalidKeyException without an android KeyStoreException cause (confirmed in the function body and pinned by tests). The KDoc for quarantineCorruptedEncryptedPreferences states: "a data-level failure keeps the key and the quarantined file stays decryptable by this device if the classification was ever wrong." But InvalidKeyException is also a master-key-failure path, so the master key is deleted and the quarantined D2D file becomes permanently unrecoverable on this device too. This is a latent pre-existing issue that the new quarantine semantics make worth documenting explicitly — the current KDoc's decryptability guarantee does not carve out this case.

3. moveFile silently drops a failed delete after copy

In EncryptedPreferenceQuarantine.kt:

if (!source.delete()) {
    Twig.error { "Failed to delete source file after copy: $source" }
}

The error is logged but the original file remains alongside the quarantined copy. On a truly storage-full device the original corrupted file survives, and the next launch's quarantine would see it again (though the marker guard would prevent a second quarantine attempt). Non-blocking.

Concurrency

4. walletMutation held across ensureReadable()'s retry ladder and Synchronizer.erase()

init() holds walletMutation.withLock { ... resetOnboardingIfWalletMissing(...) }. resetOnboardingIfWalletMissing calls sdkEncryptedPreferenceRecoveryProvider.ensureReadable() (up to 3× with 200/400/800 ms backoff ≈ 1.4 s) and then Synchronizer.erase(). createNewWallet() and restoreWallet() both contend on the same mutex. The KDoc on walletMutation describes the intent, but the "this can block for ~1–2 s on first launch after a corrupt-store recovery" consequence is not called out. Nit.

Design / Misc

5. CRASH_PROCESS_NAME_SUFFIX duplication is documented but un-enforced

In ZcashApplication.kt:

private const val CRASH_PROCESS_NAME_SUFFIX = ":crash"

The KDoc says "Kept in sync with GlobalCrashReporter's own copy of this suffix." There is no compile-time or test-time enforcement. If GlobalCrashReporter changes its suffix, isCrashProcess() silently diverges. Nit.

6. SdkEncryptedPreferenceRecoveryProviderImpl is correctly internal — verified in the full file. No issue.

7. resolveSecretState is internal for testability — deliberate, correct given same-module test access.

Suggestions

• Obs 1 (marker ordering): Move the marker-write runCatching block inside the success branch of the quarantine runCatching, or track a boolean (quarantineSucceeded) and guard the marker write on it. Non-blocking given current delete-fallback behavior, but worth tidying to prevent the confusing stuck-rethrow case.

• Obs 2 (InvalidKeyException / master key on D2D path): Add a note to quarantineCorruptedEncryptedPreferences's KDoc explicitly stating that InvalidKeyException is also a master-key failure path and that the quarantined D2D file will not be locally recoverable after it fires — the current "stays decryptable if classification was wrong" guarantee does not apply here. Non-blocking.

• Obs 3 (moveFile silent delete failure): Add a Twig.warn (already present) — already done. Consider whether the copy-then-failed-delete case should rethrow so the outer runCatching falls through to the delete fallback, rather than leaving two copies. Non-blocking.

• Obs 4 (mutex contention window): Add one sentence to walletMutation's KDoc: "The self-heal path holds this lock across ensureReadable's retry ladder (up to ~1.4 s) and Synchronizer.erase; createNewWallet and restoreWallet block for that window on first launch after corrupt-store recovery." Nit.

• Obs 5 (CRASH_PROCESS_NAME_SUFFIX): Add a cross-reference comment on the GlobalCrashReporter side, or add a test asserting the constant value. Nit.

---

_{Automated review by zodl-review bot · Models: us.anthropic.claude-sonnet-4-6 + us.meta.llama4-maverick-17b-instruct-v1:0 (synthesis) · [Silent pilot — feedback welcome]}

[nesence-m]

Thanks Lukas — both of the blocking findings are exactly the ones three bot passes walked past, and they are the two that matter here. Reading the two classifiers against each other, and asking what pruneQuarantine is retaining rather than whether it retains correctly, is the kind of pass this diff needed. All four items are addressed in 0a0c332 (preference-impl-android-lib) and 0b77aed (ui-lib).

1. Shared transient-HAL veto — 0a0c332
The veto is now one function, hasTransientAndroidKeyStoreMarker(chain) in AndroidPreferenceProvider.kt, used by both isUnrecoverableCorruption and isMasterKeyFailure (both also share a causeChain(exception) helper, so they cannot drift on chain depth either). A chain carrying android.security.KeyStoreException is now neither corruption nor a master-key failure, so your wedged-daemon sequence stops at step 2: the retry ladder exhausts, nothing is unrecoverable, createEncryptedPreferencesWithRecovery rethrows and the store stays in shared_prefs for the next launch. Test added: a failure wrapping an android Keystore failure is not corruption, covering the AOSP shape, a java.security.KeyStoreException wrapping the marker, and the marker one level deeper — the mirror of the isMasterKeyFailure case you pointed at.

2. Quarantine retention — 0a0c332
pruneQuarantine now keeps the oldest entry per filename unconditionally plus the newest keepNewest - 1, deleting only what is between them; total kept is unchanged at 3. Your four-launch loss chain is now a test: repeated quarantines keep the first copy, the only one holding a wallet quarantines a store holding "seed ciphertext", recreates it empty, and repeats five times, then asserts the first entry both survives and still reads back the seed content. The existing prune test was updated to the new policy (it now expects target-1, target-4, target-5).

I corrected the PR description too: it now says the recreated-marker bounds the immediate retry within a run — it is cleared on any successful open — and explicitly not repeat quarantines across launches, which is what the retention policy is there for.

3. No delete fallback — 0a0c332
The onFailure that deleted the original is gone. quarantineEncryptedPreferencesFilesAndMarkRecreated runs the move and then writes the recreated-marker only if the move succeeded; a throwing move propagates out of quarantineCorruptedEncryptedPreferences and out of createEncryptedPreferencesWithRecovery, so the master-key deletion and the cache-clear-and-delete below it are skipped too and the store is left exactly where it was. Test: a failed quarantine keeps the original files and writes no marker — both original files still readable with their original content, no marker, so the next launch is still allowed to recover. a successful quarantine writes the marker pins the other side.

4. Erase failure visibility — 0b77aed
The line was already Twig.error(it) { ... }, but Twig.error is Log.e and nothing routes it off the device — the only path that reaches crash reporting in this repo is GlobalCrashReporter.reportCaughtException (NearSwapDataSource is the existing user). So the .onFailure now does both, and configureAnalytics() registers the reporters before walletRepository.init() in ZcashApplication.onCreate, so it is live by then. Test: initReportsAFailedEraseToCrashReporting.

Third test gap — the fail-safe hinge — 0b77aed
initNeitherResetsOnboardingNorErasesWhenTheWalletReadFails: the wallet read throws, and the test asserts no onboarding write happened, the flag is still 3 (READY), and Synchronizer.erase was never called. That pins the property you identified as resting on the absence of a catch three frames down.

Your three deferred items, as agreed

• Erase after a failed ensureReadable — agreed, no code here; it goes on the SecretState.ERROR ticket, since the honest fix is telling the user their secret store is unreadable rather than routing silently to onboarding. Noted in the PR description's follow-up list.
• Test scope replacement ordering and sharingScope — closed, no change, per your reasoning.
• Twig.error(e) on a PersistableWallet parse failure logging decrypted wallet JSON via JSONTokener.syntaxError — agreed this is not this PR; a separate ticket will be filed for it, and it is listed in the PR description's follow-ups so it doesn't get lost.

Gates: ktlintFormat + detektAll clean, :preference-impl-android-lib:testDebugUnitTest 35/35, :ui-lib:testZcashmainnetInternalDebugUnitTest --tests '*WalletRepository*' 11/11, :preference-impl-android-lib:compileDebugAndroidTestKotlin clean. One @file:Suppress("TooManyFunctions") was needed on EncryptedPreferenceQuarantine.kt — the extracted function is its 11th, one over the threshold; same idiom as FileExt.kt.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RXV241FzSZi6wy2PXPNz3S

[nesence-m]

CI regression fixed in 2de9a76.

What failed

test_android_modules_emulator (libs) failed one instrumented test in preference-impl-android-lib:

co.electriccoin.zcash.preference.EncryptedPreferenceProviderTest > graceful_recovery_when_master_key_is_lost_in_migration[emulator-5554 - 15] FAILED
    javax.crypto.AEADBadTagException
    at android.security.keystore2.AndroidKeyStoreCipherSpiBase.engineDoFinal

The store was never quarantined, so the recovery under test never happened and the failure was rethrown instead.

The chain that actually occurs

I reproduced the scenario on an emulator and dumped the cause chain. It is the same on API 35 and API 31:

javax.crypto.AEADBadTagException
  caused by android.security.KeyStoreException: Signature/MAC verification failed
      (internal Keystore code: -30 message: system/security/keystore2/src/operation.rs:847 ...)

with, on API 35: getNumericErrorCode() = 10 (ERROR_KEYMINT_FAILURE), isTransientFailure() = false, isSystemError() = false, getRetryPolicy() = 1 (RETRY_NEVER). On API 31 the API-33 methods do not exist at all; only the message is available.

MasterKey.Builder.build() reuses the alias, so once the original key is gone it silently mints a replacement under the same alias and the stored ciphertext can never authenticate again. That is a permanent condition, and it carries an android.security.KeyStoreException.

hasTransientAndroidKeyStoreMarker (introduced in 0a0c332) vetoed on the mere presence of that class anywhere in the chain, so both classifiers read this permanent failure as transient: isUnrecoverableCorruption returned false, the retry ladder exhausted, and the exception was rethrown. The class is not a transient marker — AOSP raises it for every Keystore/KeyMint failure, permanent ones included.

How the veto was made precise

The veto is unchanged in shape and is still shared by both classifiers, so they cannot drift. It now holds only while the failure is not positively permanent:

• isTransientFailure() short-circuits it — whenever the platform itself says the failure will heal (wedged daemon, KM_ERROR_SECURE_HW_BUSY, KM_ERROR_SECURE_HW_COMMUNICATION_FAILED, all of which AOSP flags IS_TRANSIENT_ERROR), the store is never touched.
• Permanence then needs positive evidence: either AOSP's hard-coded wording for a key that is gone, unparseable or unauthenticating (Signature/MAC verification failed, Invalid key blob, Key blob corrupted, Key not found, Key permanently invalidated), or getNumericErrorCode() reporting ERROR_KEY_DOES_NOT_EXIST / ERROR_KEY_CORRUPTED (AOSP maps both ResponseCode.KEY_NOT_FOUND and ResponseCode.KEY_PERMANENTLY_INVALIDATED onto the former).
• Anything neither signal classifies stays vetoed and is rethrown for a later launch, never quarantined.

The typed classification is API 33+, so it is read reflectively; the message strings carry devices below that. I checked the AOSP sources for API 30, 31 and 36: the strings come from KeymasterDefs.sErrorCodeToString and the ResponseCode switch in KeyStore2.getKeyStoreException, they are hard-coded and never localized, and the legacy KeyStore.getKeyStoreException used below API 31 emits the identical set. This also repairs API 31/32, where the previous veto would have stranded the same case with no API-33 classification available to contradict it.

Tests

The instrumented test is unchanged — it was correct, and it is what caught this. EncryptedPreferenceRecoveryTest gained pins for the permanent shapes (key not found, key permanently invalidated, ciphertext that no longer authenticates, the numeric code standing alone, and the pre-API-33 message-only path) alongside the existing transient ones, plus one asserting that a transient verdict outranks a permanent-sounding message. The Keystore stand-in gained the API 33 classification, with a second stand-in for the pre-33 shape that carries none.

Verified locally

• :preference-impl-android-lib:connectedDebugAndroidTest — 12/12 green on a Pixel 9 Pro XL AVD (API 35), including graceful_recovery_when_master_key_is_lost_in_migration.
• :preference-impl-android-lib:testDebugUnitTest — 26/26 green in EncryptedPreferenceRecoveryTest.
• ktlintFormat detektAll — clean.
• :ui-lib:testZcashmainnetInternalDebugUnitTest --tests '*WalletRepository*' — green.

[LukasKorba]

All three blocking items are properly closed, and (2) plus the new tests are better than the minimum ask. One thing needs to be on the record rather than riding in on a fix round — see the predicate note below. Good to merge once that's acknowledged.

The predicate change — my call, recorded

2de9a76 narrowed the veto I asked for. I asked for the bare it !is java.security.KeyStoreException && simpleName == "KeyStoreException"; head has a third conjunct, !isPermanentAndroidKeyStoreFailure(it) (AndroidPreferenceProvider.kt:457-462).

You were right to push back and I'm accepting it. My predicate would have stranded the device-to-device transfer case — the thing this recovery exists for — permanently: the real chain is AEADBadTagException ← android.security.KeyStoreException: Signature/MAC verification failed, MasterKey.Builder.build() mints a replacement under the old alias so the ciphertext can never authenticate again, and a bare marker veto reads that as transient forever. Ladder exhausts, rethrow, splash with no recourse. I'd have traded a loss path for a brick, which is not obviously the better trade. Reproducing it on API 31 and 35 and pinning it with graceful_recovery_when_master_key_is_lost_in_migration is the right way to have made that argument.

The structure is also what I asked for: one hasTransientAndroidKeyStoreMarker at :457, called as the first statement of both classifiers (:486, :516), with causeChain() shared too, so CAUSE_CHAIN_LIMIT can't drift either. And the ordering inside isPermanentAndroidKeyStoreFailure is the safe one — isTransientPerAndroidKeyStore first at :430, so when AOSP says transient that outranks whatever the message string says, and anything unclassifiable stays vetoed.

I checked the veto didn't over-apply, which was my worry: BadPaddingException, AEADBadTagException, CharConversionException and InvalidProtocolBufferException with no marker still classify as corruption and still recover. Genuine corruption isn't stuck.

The residual I'm accepting with open eyes, so it's written down: on API 27-32 both reflective signals are absent (isTransientFailure() and the numeric code are API 33+), so PERMANENT_ANDROID_KEY_STORE_MESSAGES (:383-390) is the only guard there. A transient failure that happened to carry "Key not found" — a keystore daemon whose DB isn't up yet, say — would release the veto, and then InvalidKeyException in the chain makes both classifiers true: quarantine the seed and delete the shared master key. I think that's unlikely (AOSP returns SYSTEM_ERROR, not KEY_NOT_FOUND, for daemon-level trouble) and the ~1.4s ladder covers the common case, but it's the one place the narrowing costs loss-axis margin, and it's the thing to revisit first if a pre-33 report ever comes in.

The rest

(2) retention — fixed, and the shape is right. EncryptedPreferenceQuarantine.kt:150-170, drop(keepNewest - 1).filterNot { it == oldest }. I walked it out to five quarantines: Q1 survives every round, steady state is exactly 3 entries, no unbounded growth. keepNewest = 1 and even 0 still keep the oldest, so there's no argument for which prune deletes the last copy. .bak pairing is correct — the filter requires .xml so .bak files are never candidates themselves, and EncryptedPreferenceQuarantineTest.kt:83-94 asserts the three survivors' .bak siblings all remain.

(3) — fixed. The deleting instead block is gone; :318-322 calls quarantine unguarded, so a throw propagates with the originals in place. The bonus is the better half: the marker now writes only after the move succeeded (EncryptedPreferenceQuarantine.kt:99-121), so a store that was never set aside can still recover next launch. a failed quarantine keeps the original files and writes no marker pins both.

Crash reporting — fixed, WalletRepository.kt:314-318, pinned with verify(exactly = 1) on the instance.

All three test gaps genuinely pinned, not adjacent: EncryptedPreferenceQuarantineTest.kt:103-133 writes "seed ciphertext" then asserts by content that target-1700000000001.xml still holds it after five quarantines — that's the property, not a file count. WalletRepositoryImplTest.kt:198-221 asserts the fail-safe hinge with an empty event list, the onboarding pref unchanged at READY, and coVerify(exactly = 0) on erase. And EncryptedPreferenceRecoveryTest.kt:71-201 now covers both API tiers with stand-ins, including transient-outranks-message.

Nits, not blocking

• EncryptedPreferenceQuarantine.kt:150-158 — if a quarantine ever moves only a .bak (xml absent mid-commit), the resulting <filename>-<millis>.xml.bak has no .xml sibling, so the endsWith(".xml") filter never sees it and it's never pruned. Genuinely unbounded, though it's a few KB and purgeQuarantine() clears it.
• Worth naming in the KDoc: on an unclassifiable permanent failure the user now sits on the splash with no in-app recourse, since Reset Zodl is behind it. That's the deliberate price of failing safe and it's the right default — but it should be written down next to WalletRepository.kt:146-153 rather than discovered later.

[nesence-m]

Follow-up tickets from Lukas's second pass:

• MOB-1865 [Security] PersistableWallet parse failure logs the decrypted wallet JSON via JSONTokener.syntaxError — https://linear.app/zodl/issue/MOB-1865 (pre-existing, High)
• MOB-1866 Surface SecretState.ERROR when the encrypted store is unreadable instead of routing silently to onboarding — https://linear.app/zodl/issue/MOB-1866 (also covers the erase-after-failed-ensureReadable case)

[nesence-m]

Acknowledged on the record, per your 19:35Z review.

API 27-32 residual risk — accepted with open eyes, exactly as framed: with isTransientFailure() and the numeric error code both API 33+, PERMANENT_ANDROID_KEY_STORE_MESSAGES is the only guard on those API levels. A transient failure that happens to carry the "Key not found" wording would release the veto there, and InvalidKeyException in the chain would then make both classifiers true — quarantining the seed and deleting the shared master key. Judged unlikely (AOSP returns SYSTEM_ERROR, not KEY_NOT_FOUND, for daemon-level trouble) and the ~1.4s retry ladder covers the common case, but it is the one place the narrowing costs loss-axis margin, and it's the first thing to revisit if a pre-33 report ever comes in.

Both nits pushed in 5c8a9f941:

1. pruneQuarantine now identifies an entry by its shared <filename>-<millis> base name instead of by its .xml file alone, so a .bak-only entry (.xml move failed mid-commit) is counted and pruned like any other instead of growing the quarantine directory unbounded. New test: prune counts and prunes an entry that has only a bak file. The oldest-survives rule is unchanged.
2. Added a KDoc paragraph to isPermanentAndroidKeyStoreFailure naming the deliberate trade-off: an unclassifiable permanent failure is rethrown, leaving the user on the splash with no in-app recourse (Reset Zodl sits behind a normal boot the wedged store can't reach) — the price of never quarantining or wiping a store that might still turn out to be readable.

Filed separately by the orchestrator, not in this PR: the JSONTokener decrypted-JSON log leak in WalletRepository.kt and the SecretState.ERROR in-app recourse item. Ticket IDs to follow in a separate comment.

Gates: ktlintFormat/detektAll clean, preference-impl-android-lib:testDebugUnitTest passes.

[LukasKorba]

Both nits done, and the orphan fix is the right shape — EncryptedPreferenceQuarantine.kt:160-165 collects base names from .xml and .xml.bak into a set and deletes both siblings per entry, so a .bak with no .xml is now reachable by the pruner while the oldest entry stays pinned. Retention semantics are unchanged.

The KDoc is better than what I asked for. I wanted the no-in-app-recourse consequence written down; you also supplied the fact that makes it defensible — that the crashed sharing coroutine is the same terminal outcome the app already reaches today via SynchronizerProvider collecting the same underlying store. That reframes it: the splash hang isn't a new failure mode this PR introduces, it's the existing one, and the alternative would be misrouting an intact-but-unreadable store to onboarding. That's the stronger argument and I hadn't had it in front of me.

Nothing further from me on this one. Signed off on the API 27-32 residual as discussed — the pre-33 message list is the thing to revisit first if a report ever comes in from that range.

[LukasKorba]

Approving.

Summary of what this sign-off covers, for the record: the three blocking items I raised are closed in code — the transient-Keystore veto is shared by both classifiers through one helper so they can't drift, pruneQuarantine pins the oldest (seed-bearing) entry indefinitely with no unbounded growth, and the quarantine-failure path no longer deletes the original. The erase failure reaches crash reporting, and all three test gaps are pinned by tests that assert the property rather than something adjacent — the quarantine one asserts by content that the seed survives five rounds.

Two things I'm explicitly signing off rather than silently accepting:

1. The narrowed veto (hasTransientAndroidKeyStoreMarker + isPermanentAndroidKeyStoreFailure). This is not the predicate I originally asked for. You were right that the bare version would have stranded the device-to-device transfer case on the splash forever, and reproducing it on API 31 and 35 was the right way to make that argument.
2. The API 27-32 residual. Below API 33 both reflective signals are absent, so the five-string message list is the only guard, and a transient failure carrying one of those messages would release the veto and both quarantine the seed and delete the master key. I judge this unlikely and the ~1.4s ladder covers the common case. If a report ever comes in from that API range, this is the first thing to look at.