Skip to content

chore: log cause of keychain load failures#898

Merged
jvsena42 merged 6 commits intomasterfrom
chore/keychain-error-logs
Apr 21, 2026
Merged

chore: log cause of keychain load failures#898
jvsena42 merged 6 commits intomasterfrom
chore/keychain-error-logs

Conversation

@jvsena42
Copy link
Copy Markdown
Member

@jvsena42 jvsena42 commented Apr 20, 2026

Related to #897

This PR:

  1. Stops swallowing the underlying cause when Keychain.load/save/upsertString/delete catch a failure, so the real exception propagates through KeychainError
  2. Embeds the cause's simple class name (no message, no payload) into the error message so it surfaces automatically at every existing caller's log line
  3. Emits a single sanitized WARN from Keychain.load on the first decrypt failure per key per process, capturing aliasPresent, entryPresent, walletIndex, and the cause chain of class names. Each probe is tri-state — 'true' / 'false' / 'error:ClassName' — so a failure inside the diagnostic itself (keystore subsystem error, DataStore I/O, Room error) is distinguishable from a genuine false
  4. Passes CancellationException through unwrapped from all four mutating paths
  5. Adds AndroidKeyStore.containsAlias() so the diagnostic can distinguish "alias missing" from "alias present but unusable"

Description

The reporter on issue #897 is stuck at launch because keychain.loadString(BIP39_MNEMONIC) throws, and every one of the repeated ERROR lines in their log says the same thing: [AppError='Failed to load BIP39_MNEMONIC from keychain.']. That string tells us the load failed but not why. The catch block in Keychain.load discards the underlying Throwable entirely, so the log cannot distinguish:

  • DataStore / Base64 / Room failures (likely transient, data-side)
  • keyStore.getKey(...) returning null — alias gone (terminal, hardware-side)
  • Cipher.doFinal AEADBadTagException — alias regenerated under the same name (terminal, hardware-side)
  • UnrecoverableKeyException / KeyStoreException — alias present but unusable

Each of these has different remediation and different user messaging. For a Bitcoin wallet we should not guess between them. This PR is diagnostics only — no retries, no routing changes, no destructive changes to stored data. After this ships, the next occurrence of the bug will include the cause class in the existing AppError=… line and a single enriched WARN with the adjacent keystore/datastore state, which is enough to pick the right fix in a follow-up PR.

The new WARN is intentionally leak-safe: it only logs class names (not cause.message), does not pass the Throwable to Logger (which would re-introduce the message via errorLogOf), and is one-shot per key per process so a start-retry loop cannot flood the log.

Preview

N/A — diagnostic logging only, no UI changes.

QA Notes

1. Happy path (no regression)

  1. Launch the app with a normal, working wallet.
  2. Confirm no WARN … Decrypt failed for key=… lines appear in the log.
  3. Confirm no (cause='…') suffix appears on any AppError=… error line.

5. Unit tests

  • ./gradlew testDevDebugUnitTest --tests to.bitkit.data.keychain.KeychainErrorTest passes
  • ./gradlew testDevDebugUnitTest --tests to.bitkit.repositories.WalletRepoTest passes
  • ./gradlew detekt clean
  • ./gradlew compileDevDebugKotlin clean

Out of scope (intentionally deferred)

  • Any remediation UI (auto-routing to RecoveryModeScreen, sticky keychainCorrupted state, seed-restore copy) — to be designed once these logs tell us what class of failure users are actually hitting.
  • Retries inside Keychain — would risk masking the signal we're trying to capture.
  • Auto-wiping or any destructive change to the stored encrypted mnemonic — the blob stays on disk regardless of decrypt outcome.
  • AndroidKeyStore configuration changes.

@jvsena42 jvsena42 self-assigned this Apr 20, 2026
jvsena42 and others added 4 commits April 20, 2026 09:58
Probes for aliasPresent, entryPresent and walletIndex previously
collapsed any exception inside runCatching to the default value,
making a keystore subsystem / DataStore / Room error indistinguishable
from a legitimate 'alias missing'. Render tri-state instead:
'true' / 'false' / 'error:ClassName'.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jvsena42 jvsena42 marked this pull request as ready for review April 20, 2026 13:37
@jvsena42 jvsena42 requested a review from ovitrif April 20, 2026 13:37
Comment thread CHANGELOG.md Outdated
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Comment thread app/src/main/java/to/bitkit/data/keychain/Keychain.kt
@jvsena42
Copy link
Copy Markdown
Member Author

@ovitrif @piotr-iohk The next step would be launch a 2.2.1 version (public or just internally) so could get the logs without requiring the app uninstall

@jvsena42 jvsena42 enabled auto-merge April 20, 2026 17:01
@jvsena42 jvsena42 requested a review from ben-kaufman April 21, 2026 11:03
Copy link
Copy Markdown
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@jvsena42 jvsena42 merged commit cb141bc into master Apr 21, 2026
21 checks passed
@jvsena42 jvsena42 deleted the chore/keychain-error-logs branch April 21, 2026 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants