Fix/lwk concurrent wollet access v2

[i5hi]

No description provided.

[i5hi]

@claude please review - we had users on ios and android get the app startup error - reproduced the issue and fixed it for an android user via an update to this branch - we dont have an ios user with the issue to test nad prove the ios side - but its likely similar - lwk issue or the issue with the keychain unlock

[claude]

Claude finished @i5hi's task in 5m 5s —— View job

---

Code review

• Gather context (PR diff, surrounding code)
• Review LWK concurrency fix (lwk_dir_guard, lwk_facade, lwk_wallet_datasource)
• Review keychain/pin-code changes (pin_code_repository, app startup)
• Review remaining changes (logger, error screen, main.dart, tests)
• Post review feedback

Summary

This PR attacks the startup error from three angles and the overall approach is sound: (1) serialize all LWK wollet access per cache directory (LwkDirGuard + the withPublicWallet/withPrivateWallet refactor), (2) disable the workmanager background engine as defense-in-depth, and (3) heal the iOS keychain paths (rethrow KeychainLockedException so the splash-retry path works, plus re-add pre-6.10 PIN items under first_unlock_this_device). I verified the supporting claims: storage_locator.dart:80-82 does configure first_unlock_this_device, AppStartupBloc has the typed on KeychainLockedException catch at app_startup_bloc.dart:170, all createPublicWallet/createPrivateWallet call sites were migrated, deleteLwkDb catches the WalletNotFound that LwkFacade.delete now throws, and all new l10n keys exist in app_en.arb. The File(dbPath) → Directory(dbPath) fix in LwkFacade.delete is a genuinely important catch — on main, File(dbPath).exists() was always false, so the UpdateOnDifferentStatus heal path could never actually delete the cache.

Findings

1. Security: PIN heal has a delete→re-add crash window that silently disables the PIN lock — pin_code_repository.dart:99-102. If the process dies between deleteValue(_key) and the saveValue (crash, OOM kill, user force-quit), the PIN item is gone and the app opens unlocked from then on, with no user-visible signal. The window is small and runs once per install, but for a wallet app "PIN silently removed" is the worst failure mode of this heal. A safer sequence: save the PIN to a backup key (e.g. securityKeyHealBackup) before deleting, and have isPinCodeSet/verifyPinCode fall back to that key (restoring _key from it) if _key is missing while the backup exists; delete the backup only after the marker is written. That makes the heal crash-safe instead of best-effort. Fix this →

2. Post-crash stall: a leftover marker can freeze all LWK ops (and the splash screen) for up to 45s — lwk_dir_guard.dart:63-77. If the app crashes while holding the marker, the next launch polls until the marker goes stale (up to 60s from its last refresh) or maxWait (45s) expires. But since both engines live in one process (the whole premise of the marker design), any marker whose owner pid ≠ current pid can only be a dead process's leftover — it can be treated as stale immediately. The owner id already starts with the pid, so _freshHolder just needs to parse and compare it. That eliminates the worst-case 45s hang on the exact screen this PR is trying to fix.

3. The cross-isolate marker acquisition is check-then-write, not atomic — lwk_dir_guard.dart:61-79. Two isolates can both observe no fresh holder and both write the marker, so the guard narrows the cross-isolate race rather than closing it. File.create(exclusive: true) (dart:io) throws if the file exists and would give real atomic acquisition (delete-then-recreate on refresh, or keep the timestamp inside). Since the workmanager task is unregistered in this same PR, only one isolate exists in practice today — but the doc comments present the marker as the cross-isolate story, so either harden it or note the TOCTOU explicitly before the BG task is ever re-enabled.

4. Footgun: LwkDirGuard.run is not reentrant within an isolate — lwk_dir_guard.dart:22. package:synchronized's Lock is non-reentrant, so nesting any facade call inside a withPublicWallet/withPrivateWallet action for the same wallet deadlocks silently (the marker's holder == _isolateOwner check handles cross-call reuse, but the in-isolate Lock doesn't). No current call site nests — _usedAddressesMap was correctly refactored to reuse the open session — but a one-line warning in the class doc would save the next contributor a very confusing hang.

5. Minor: new top-level test directory — test/core/wallet/... for the guard test, while the repo convention (and this PR's other new test) uses test/core_test/wallet/.... Worth unifying so the guard test isn't stranded in a one-off directory.

On the iOS side (from your comment)

The two keychain fixes here cover the plausible iOS causes well: the KeychainLockedException rethrow in isPinCodeSet fixes the pre-warm/-25308 launch class (previously it was swallowed into a generic Err, making the bloc's splash-retry path unreachable → permanent "Startup Error"), and the accessibility heal fixes pre-6.10 items stuck on WhenUnlocked. The heal's mechanism checks out — SecItemUpdate with only kSecValueData indeed never changes an existing item's accessibility class, so delete + SecItemAdd is the correct (and only) way, and the configured first_unlock_this_device will apply on re-add. Without an affected iOS device you can still partially validate on any iOS install: confirm the healed marker gets written on first unlock, the PIN still verifies afterwards, and (via Console/device logs) that a locked-boot launch stays on splash instead of erroring. The LWK race fix itself is platform-independent, so Android reproduction is reasonable evidence for iOS too.

Notes

• Couldn't run the test suite in this environment (no approved Flutter access); CI's "Analyze and Test" run was still in progress at review time.
• The unconditional log.severe in app_startup_bloc.dart plus the share/export-logs buttons on the error screen are a big supportability win — the root cause of the next report like this will actually be in the logs.

[i5hi]

@claude please review again - consider any regression risks and whether the pin migration for ios safe

[claude]

Claude finished @i5hi's task in 7m 49s —— View job

---

Re-review: regression risks & iOS PIN migration safety

• Gather context (diff, changes since last review)
• Deep-dive PIN heal safety (crash windows, iOS keychain semantics, migration ordering)
• Assess LWK guard changes for regressions (locking, timeouts, error paths)
• Assess remaining changes for regressions (startup, workmanager, logger, main.dart)
• Post review

Verdict

The b7260d4 follow-up addresses all five findings from my previous review, and after walking the full state machine of the heal I consider the iOS PIN migration safe to ship. The remaining regression risks are minor; the one to actively watch in the field is the serialization latency (#2 below), which is the intended price of the correctness fix rather than a bug.

Previous findings — all verified fixed

1. PIN heal crash window → now backed by securityKeyHealBackupV1 written before the delete, with restore-on-read (pin_code_repository.dart:101-160), plus a one-shot retry of the re-add. Covered by tests.
2. Leftover marker stalling startup 45s → _freshHolder now treats any marker whose owner pid ≠ current pid as immediately stale (lwk_dir_guard.dart:112), with a test asserting acquisition completes in <2s against a dead-pid marker.
3. Marker TOCTOU → explicitly documented as acceptable only while the workmanager task stays unregistered, with a pointer to File.create(exclusive: true) for re-enablement (lwk_dir_guard.dart:50-56).
4. Non-reentrancy footgun → documented on LwkDirGuard (lwk_dir_guard.dart:19-23).
5. Test directory → guard test now lives in test/core_test/.

Is the iOS PIN migration safe? Yes — here's the state-machine walk

The invariant that matters is: there is no reachable state where a PIN should exist but both securityKey and the backup are absent. I checked every path:

• Every path that deletes securityKey writes the backup first (_healLegacyKeychainAccessibility saves backup → deletes → re-adds). The backup is only ever deleted in three places: (a) after the re-add is confirmed and the healed marker is written, (b) in setPinCode after the new PIN is already saved, (c) in deletePinCode where removal is the intent. So a crash, force-quit, or keychain error at any step leaves either the key or the backup present, and _readPinRestoringFromHealBackupIfNeeded recovers on the next read.
• Concurrency: PinCodeRepository is a lazy singleton (pin_code_locator.dart:16) and _accessibilityHealAttempted is set synchronously before the first await, so the heal is single-flight per session; the persistent marker makes it once per install. A verifyPinCode that races the heal's delete→re-add window falls into the restore path and still returns the correct PIN — so even the unlock screen racing startup is safe.
• Locked-keychain aborts: the heal only runs after a successful read of securityKey, so the device has been unlocked. If iOS locks mid-heal, a -25308 before the delete leaves everything untouched (outer catch); after the delete, the backup restores on the next read. And a fully-locked launch never reaches the heal at all — isPinCodeSet rethrows KeychainLockedException into AppStartupBloc's splash-retry catch (app_startup_bloc.dart:170).
• Accessibility semantics: the re-add and the crash-recovery restore both hit a state where securityKey is absent, so both go through SecItemAdd, which is the only call that applies the configured first_unlock_this_device (storage_locator.dart). The mechanism is sound. Note the heal also cycles post-6.10 users' items once (the plugin can't read an item's existing accessibility class) — harmless and marker-bounded.
• Device-migration consequence: healed PIN items become ThisDeviceOnly, so they no longer follow an iCloud/encrypted backup to a new device. That matches the seeds and everything else in secure storage, so this makes behavior consistent (previously a restored device could have a PIN lock but no wallet).

Residual edge (fine to ignore): if deletePinCode deletes securityKey but its best-effort backup cleanup also fails, a stale backup could resurrect the just-disabled PIN on the next read. It requires a keychain delete to fail while unlocked (essentially never) and it fails toward locking with a PIN the user just knew, not toward unlocking. Not worth more machinery.

Regression risks

1. isPinCodeSet rethrow reaches two blocs with no typed catch — low likelihood, worth a cheap guard. AppUnlockBloc._onStarted (app_unlock_bloc.dart:46) and PinCodeSettingBloc (line 95) call the same repository method. On main a locked keychain produced a generic Err → failure state; now the exception escapes the bloc handler unhandled, leaving the unlock screen stuck in its initial state. These screens only render after startup already succeeded (keychain readable), and post-heal items are readable after first unlock, so this is nearly unreachable — but a one-line on KeychainLockedException in _onStarted mapping to the failure state would restore the old worst-case behavior. Fix this →
2. Serialization latency is the main thing to watch post-release. All ops on one Liquid wallet's cache dir now queue behind each other, and the in-isolate Lock has no timeout (the marker's 45s maxWait is cross-isolate only). A slow or wedged sync_ (bad network, Tor) now blocks getBalance, getTransactions, buildPset, and signTx for that wallet until it returns — previously they ran concurrently, which was exactly the corruption bug, so this is the intended price. But if lwk's sync_ can hang indefinitely on a dead socket, send flows freeze with it. If field reports of "stuck spinner on send during sync" show up, a timeout wrapped around sync_ specifically (not around the lock) is the right knob.
3. Stale comment — check_for_existing_default_wallets_usecase.dart:28-30 says "lwk_facade throws e.msg", but after this PR the facade propagates raw LwkError and the datasource converts to String. Behavior is unaffected (e.toString().contains('UpdateOnDifferentStatus') matches both shapes — I verified every one of the 12 facade call sites still converts in its own catch). Cosmetic.
4. Re-enabling the workmanager task is the long-term footgun, not this PR: doing so re-opens both the marker TOCTOU (Create new wallet using existing mnemonic and new bip39 passphrase #3 above, documented) and the FG-pruning-the-BG-file race (Logger.pruneBackgroundFile is only safe while nothing else holds a sink on that file). Both sites carry pointer comments to each other, which is the right mitigation — just make sure any future re-enable PR reads them.
5. Checked and clean: no remaining callers of the removed LwkFacade.sync/createPublicWallet/createPrivateWallet; the heal-path delete calls run after the guard is released (no self-deadlock); deleteLwkDb/deleteWallet both catch WalletNotFound (wallet_repository.dart:391-405); Workmanager().cancelAll() clears stale schedules from prior versions; the marker file is a sibling of the cache dir, so LwkFacade.delete removing the dir can't delete the guard's own marker mid-hold. The non-atomic truncate-rewrite in _pruneFile predates this PR.

Notes

• The new test suites are good: the heal tests cover the ordering invariant (verifyInOrder backup→delete→re-add→marker→cleanup), every failure injection point, the Android/no-pin no-op paths, and single-flight; the guard tests cover mutual exclusion, per-dir independence, cleanup on throw, and the dead-pid fast path.
• No Flutter toolchain in this environment, so I couldn't run the suites locally; CI's Analyze and Test run was still in progress at review time — worth a glance before merging.
  · Branch