fix: vss client recovery and reset race

[ovitrif]

Fixes #1256
Fixes #1257

This PR:

1. Makes the VSS backup client recover after a failed setup instead of staying broken until the app restarts
2. Stops the node before the keychain is wiped on Reset, so a wipe no longer races an in-flight node start

Description

• Resets the setup state when setup fails, so a later successful setup actually makes the client usable. Previously the first failure was latched: every following setup reported success without doing anything, and every VSS call threw the stale error. Setup also captures its own gate, so a reset that swaps the gate mid-setup can no longer be completed by the old wallet's client.
• Restructures the wallet wipe into three phases. First it resets the backup clients and stops the node; this is the only step that can fail the reset, and it fails before anything is destroyed, so an in-flight node start is awaited under the lifecycle mutex instead of raced. Then it runs the remote Paykit and Pubky cleanup, which needs the keychain session, as best-effort. Finally it wipes local state, with the LDK storage wipe as the last abort point: if the node directory cannot be removed, nothing else is destroyed and the reset fails with the wallet intact. Every later local step is logged on failure and never aborts the rest, so a reset cannot leave a wallet that still looks alive with its node data gone, and the LDK directory is never wiped after a new mnemonic was saved.
• Skips starting the backup observers while a wipe is in progress, so they never call VSS setup without a mnemonic, and restarts them after a failed node stop when the node is still running.
• Rejects a second Reset while one is in flight, and shows the Reset button in a loading state with back navigation and the Backup button disabled until the wipe completes.
• Makes LightningRepo.stop() really stop a node object that a failed start left alive instead of short-circuiting on the Stopped state, so Reset during the start retry window tears the node down before any cleanup rather than failing later at the storage wipe. The storage wipe now holds the lifecycle lock across the stop and the directory removal, so a start cannot rebuild the node in between.
• Applies the same setup recovery to the LDK VSS client used by Reset network graph and Reset pathfinding scores.
• Motivation: one failed setup (network or auth hiccup at first node start, or the wipe race from #1254) used to silently kill all VSS backups for the session and could send a restore to the RN backup.

Out of Scope

• WalletViewModel.start(): a restore-triggered start dropped by a stale isStarting flag (#1257, remaining part).
• BackupRepo restore picker: falling back to the RN backup when the VSS lookup returns null (#1254).
• Surfacing setup failures in the UI or adding retries beyond the existing setup-with-retry path.
• Cancelling an in-flight node start from Reset; the wipe waits for it as before, now visibly.

Design

ResetAndRestoreScreen gains a loading state on the Reset button while wiping. N/A — no design available.

Preview

N/A

QA Notes

Journeys

N/A — no backup/restore journey exists yet.

Manual Tests

• 1. Onboarding → New Wallet: node starts and all backup categories upload (Backup succeeded in logs).
• 2. Settings → Security → Reset and Restore → Reset → New Wallet: logs show backup reset, node stopped, remote cleanup, LDK storage wiped, then keychain wiped; the new wallet's node starts and backups upload.
• 3. Reset and Restore → Reset → Yes, Reset: Reset button shows a spinner, Backup button and back are disabled until onboarding shows.

The failed-setup path itself could not be reproduced on-device: setup does no network I/O and the app skips node start while offline, so the only real trigger is the wipe race from #1254. That path is proven by the unit tests only.

Automated Checks

• Unit tests added: cover a successful setup and setup-with-retry after a failed attempt leaving the client usable in VssBackupClientTest.kt, the same recovery in VssBackupClientLdkTest.kt, and observers being skipped while wiping in BackupRepoTest.kt.
• Unit tests added: cover the stop of a node left alive by a failed start, and a start being excluded while the storage wipe holds the lifecycle lock, in LightningRepoTest.kt.
• Unit tests added and modified: assert the phase order, the re-entrancy guard, completion despite later local step failures, the no-wipe path when the LDK storage wipe fails, and the no-wipe plus observer restart path when the node stop fails in WipeWalletUseCaseTest.kt.
• Local: just compile, just test, and just lint pass.
• On-device (Pixel 10 emulator, dev build): Reset → new wallet logs the phase order (backup reset, node stopped, remote cleanup, LDK storage wiped, then keychain wiped), the new wallet's node starts, VSS setup succeeds on attempt 1 and every backup category uploads. Reset shows the loading state on the Reset button with Backup and back disabled until onboarding appears.

[greptile-apps]

Confidence Score: 4/5

The PR should not merge until a failure after successful Lightning storage deletion can no longer leave the old wallet active with its node data erased.

Findings

1.  Wipe Can Leave Broken Wallet ▶

Summary

This PR makes VSS setup recoverable after transient failures, prevents backup observers from starting during wallet erasure, and moves Lightning shutdown and storage deletion ahead of keychain cleanup.

• Failed VSS setup now publishes the failure and installs a fresh setup gate for retries.
• Wallet reset stops and deletes Lightning state before clearing protected wallet data.
• Backup observation is suppressed while the wipe flag is active.
• Tests cover VSS recovery, wipe-time observer suppression, and the revised wipe order.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Reset requested] --> B[Mark backups as wiping]
    B --> C[Reset backup clients and observers]
    C --> D[Stop node and delete LDK storage]
    D --> E[Clean Paykit and Pubky state]
    E --> F[Wipe keychain]
    F --> G[Clear databases and app stores]
    G --> H[Reset wallet state]
    H --> I[Switch to onboarding]
    E -->|Failure| J[Return failure while old wallet remains represented]
    D -. LDK data already deleted .-> J

Loading

_{Reviews (2) · Last reviewed commit: "fix: stop node before wiping keychain on..."}

[github-actions]

Regtest APK

Built from effa097 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

[greptile-apps]

Wipe Can Leave Broken Wallet

Moving the LDK storage deletion to the beginning of the wipe makes it possible to erase the node data and then abort on a later cleanup failure, such as removePublishedEndpointsForCleanup. In that case, the keychain and wallet-state cleanup are skipped, and the caller only reports an error. The app can therefore continue to present the wallet as existing even though its Lightning storage has already been removed.

Knowledge Base Used:

• Persistence, migrations, and backups
• Lightning node and channel liquidity

[ovitrif]

Agreed. Fixed in 2b5431e by wiping the keychain right after the LDK storage, so any later cleanup failure leaves the same state as before this PR (no keychain, no node data) instead of a wallet without its node data.

[piotr-iohk]

QA review from the diff and thread only — not run on device.

1. [gap] VssBackupClientLdk.setup() still does isSetup.completeExceptionally(it) without replacing the deferred — the same latch fixed here in VssBackupClient. Callers are clearNetworkGraph and resetPathfindingScores (LightningRepo.kt:665, :1904): one failed LDK-client setup makes every later "Reset network graph" / "Reset pathfinding scores" fail with the stale error and restart the node until the app is restarted. Same two-line fix; arguably in #1256's scope.

2. [+1 greptile] The partial-wipe window (LDK deleted, keychain kept) is real but narrow — I checked the steps between wipeStorage and keychain.wipe() and only privatePaykitAddressReservationRepo.clear() and keychain.wipe() itself can throw. Still, if it happens the app presents a wallet whose channel state is gone. Consider calling resetWalletState() regardless of failure once wipeStorage has succeeded, or treating the remaining local wipes as non-fatal.

3. [gap] Reset is now a long blocking operation with no UI feedback. ResetAndRestoreScreen never clears showDialog on confirm, so the dialog stays up until resetWalletState(), which now runs after stop() — the full node build + initial sync, or minutes in the #1257 stop-hang case. rememberDebouncedClick only debounces the tap; nothing stops a second wipeWallet() mid-flight, and WipeWalletUseCase has no re-entrancy guard, so the second run's finally { setWiping(false) } can clear the flag while the first is still wiping. Before this PR the onboarding switch was near-immediate so the window was tiny. A wiping flag on the dialog/button or a mutex in the use case would close it.

4. [nit] If stop() fails, wipeStorage().getOrThrow() aborts after backupRepo.reset() already stopped the observers and reset the VSS clients. Wallet survives, but backups are silently off until the next Running transition (app restart). Pre-existing ordering, new abort point — worth restarting observers on that failure path.

[ovitrif]

Thanks, all four addressed in 2b5431e:

1. VssBackupClientLdk.setup() now replaces the deferred on failure, same as VssBackupClient; covered by VssBackupClientLdkTest.kt.
2. keychain.wipe() moved right after wipeStorage(), so a later cleanup failure leaves the pre-PR state (no keychain, no node data) rather than a wallet without its node data.
3. WipeWalletUseCase takes a tryLock mutex and returns WipeAlreadyInProgress for a second call. The screen dismisses the dialog on confirm and, while BackupRepo.isWiping, shows the Reset button loading, disables Backup, and blocks back navigation.
4. On wipe failure the use case restarts the backup observers when the node is still Running.

[ovitrif]

An agent is running and will check the Test 3 checkbox if it succeeds.

[piotr-iohk]

Re-reviewed 2b5431e from the diff — not run on device. Items 1, 3 and the Greptile P1 look good; two new issues from the fix-up:

1. [blocker] keychain.wipe() now runs before removePublishedEndpointsForCleanup, removeBitkitPaymentEndpoints and closeAndClear. All three go through PaykitSdkService, whose PaykitSdkSessionProvider.loadSessionAccess() reads PAYKIT_SESSION from the keychain on every call and returns null once it is gone (liveSessionAccess is only returned when its secret matches the keychain value). So syncPublicEndpoints(emptyList()) / removePaykitReceiverMarker() run without a session, fail, and are swallowed — and the contactSharingCleanupPending flag is then erased by settingsStore.reset(). Result: the wiped wallet's public endpoints and receiver marker stay published after Reset; on master they were removed. Every step between wipeStorage and the old keychain.wipe() position returns Result or swallows (only privatePaykitAddressReservationRepo.clear() can throw), so moving keychain.wipe() back after pubkyRepo.wipeLocalState() keeps Greptile's window closed in practice; wrap clear() if you want it airtight.

2. [gap] The observer restart in onFailure is dead code: backupRepo.startObservingBackups() runs while _isWiping is still true (cleared in finally), so the guard added in this PR returns early with "Skipped observing backups while wiping". The unit test only passes because BackupRepo is a mock. Call setWiping(false) before the restart, or move the restart after the finally.

[ovitrif]

Both addressed in 47187b3:

1. keychain.wipe() is back after pubkyRepo.wipeLocalState(), so the Paykit and Pubky cleanup still see the session. privatePaykitAddressReservationRepo.clear() is wrapped and logged, so nothing between wipeStorage() and the keychain wipe can abort anymore; covered by a new case in WipeWalletUseCaseTest.kt.
2. The observer restart now runs after the finally that clears _isWiping, so the guard no longer short-circuits it. The test asserts setWiping(false) precedes startObservingBackups().

[ovitrif]

Handed off to pr-babysit skill which watches over reviews and CI. Warning: first run of this skill.

[ovitrif]

Both addressed in 43449af, which replaces the previous fix-up rather than moving lines around again. The wipe is now phased:

1. Reset backup clients and lightningRepo.stop(). This is the only step that can fail the reset, and it fails before anything is destroyed. On failure the observers restart after the wiping flag is cleared, so the guard no longer short-circuits it (your point 2).
2. Remote Paykit and Pubky cleanup, best-effort, while the keychain session still exists (your point 1).
3. Local wipe: LDK storage, reservations, Pubky local state, keychain, FCM token, Core, Room, stores, repo state. Every step is wrapped and logged and never aborts the rest, so the partial-wipe window is closed by construction rather than by ordering.

WipeWalletUseCaseTest.kt covers the phase order, completion despite local step failures, and the no-wipe plus observer restart path when the stop fails. Verified on the emulator: the log shows reset, node stopped, remote cleanup, LDK wiped, keychain wiped, then onboarding.

[jvsena42]

No blockers. Two LOW observations below, both pre-existing on v2.4.1 and neither introduced by this PR — I'm raising them only because they sit inside the window this PR's title claims to close, and in one case the PR body's wording is stronger than the code.

Neither is a reversal: greptile's guard, piotr's items 1-4 and the phased-wipe design in 43449af all stay as landed.

Checked and clean.

Exits from ResetAndRestoreScreen: system Back while wiping is swallowed by BackHandler(enabled = isWiping) {} (:76); top-bar back is hidden via onBackClick = null (:81); dialog dismiss only flips showDialog; Confirm launches on the activity-scoped WalletViewModel, so navigating away via the still-enabled drawer icon doesn't cancel the wipe; backgrounding pauses collection but the wipe continues on viewModelScope, and ON_STOP's stopDebounced() is a no-op against an already-stopped node; on failure isWiping clears, a toast fires, and the wallet is intact.

Ordering and lifecycle: remote cleanup doesn't depend on the LDK node, so moving it after stop() is safe, and it runs before keychain.wipe() so the Paykit session is still present — the ordering piotr asked for. stop() can't fail merely because the node never started (LightningService.stop() returns early on node == null), so Forgot-PIN before node start and ErrorStarting still reset. Observer restart on stop() failure runs after setWiping(false), so the new _isWiping guard in startObservingBackups doesn't short-circuit it.

Concurrency: wipeMutex.tryLock + finally unlock is correct, and a second concurrent wipe gets WipeAlreadyInProgress. step() uses runSuspendCatching, so a genuine CancellationException propagates out of the wipe. Cancelling during stopNode/cleanupRemote leaves the wallet intact — stop() is NonCancellable inside its lock and nothing destructive has run. An ON_START node restart mid-wipe is self-healing: wipeStorage's own stop() serialises on lifecycleMutex.

Key material: nothing in the touched code logs a mnemonic or passphrase; VssBackupClient logs only the VSS/LNURL URLs. Keychain.wipe() clears the DataStore and resets the keystore key, and the PR adds no seed-derived artifact that survives it — except the LDK-directory case below. Remote VSS state is intentionally kept for restore; unchanged.

Upgrade: no persisted format changed, and a v2.4.1 VSS backup is read by unchanged BackupRepo code.

I also checked the claim that setup() does no network I/O and it holds — VssClient::new_with_lnurl_auth only derives xprivs and builds the header provider; the JWT is fetched lazily on first request. That's what makes finding 1 below a millisecond-wide window rather than a 30-second one.

[jvsena42]

LOW / hardening. setup() reads isSetup as a field at every use rather than capturing it, so a setup() that is mid-flight when reset() swaps the deferred completes the replacement.

The real line is :65 (isSetup.complete(Unit)), which isn't in the diff — anchoring here since this is the guard the PR rewrote.

reset() (:110-117) does synchronized(this) { isSetup.cancel(); isSetup = CompletableDeferred() } and does not take setupMutex, so it doesn't wait for an in-flight setup(). If one is between :43 (mnemonic read) and :65 when that happens, :65 completes the fresh deferred. Then the next wallet's setup() short-circuits at this line and every VSS call uses the Rust client built for the old wallet — vss_new_client_with_lnurl_auth stores into a global and vssStore/vssGet take no client handle, so the stale client really is reachable. putObject/getObject await the field (:123, :138), i.e. the already-completed replacement, so it returns immediately and silently: the new wallet's backups land in the old store encrypted with the old key, and on the restore path getLatestBackupTime() → setup().getOrThrow() (BackupRepo.kt:742) reads the old wallet's store into a wallet restored from a different seed. It persists until process death.

Why it's LOW rather than what that description sounds like: I couldn't construct a realistic trigger. setup() does no network I/O, so the window is milliseconds of key derivation. The _isWiping guard you added at :144 already keeps startObservingBackups from launching a fresh setup() once a wipe starts, setupWithRetry only re-enters on MnemonicNotAvailableException, and the other callers are user-driven. Identical code is on v2.4.1, so this is not a regression — the PR only rewrote the guard, added @Volatile, and added the onFailure reset.

Fix, if you want it closed while you're in here — capture the gate under the mutex:

setupMutex.withLock {
    val gate = isSetup
    if (gate.isCompleted && !gate.isCancelled) return@runCatching
    ...
    gate.complete(Unit)
}

and in onFailure: gate.completeExceptionally(it); if (isSetup === gate) isSetup = CompletableDeferred(). complete() on the cancelled old gate is a no-op, so the replacement stays incomplete and the next setup() rebuilds the global. Three lines here and the same three in VssBackupClientLdk.kt (:49, :66). Equally fine to leave it — it's theoretical hardening.

[ovitrif]

Applied in ee283d4: setup() captures gate = isSetup under the mutex, completes that gate, and on failure only replaces isSetup when it still is that gate. Same change in VssBackupClientLdk.

[jvsena42]

LOW. stopNode() can succeed vacuously while the node object is still alive, and step() then swallows the wipeStorage failure — so the keychain and stores are destroyed with the old seed's LDK directory left behind.

The PR body says an in-flight node start is "awaited under the lifecycle mutex instead of raced", and "closed by construction". That holds when the start succeeds. It doesn't when the start fails into the retry path:

1. LightningRepo.start(): lightningService.setup() succeeds (node assigned, LightningService.kt:209), lightningService.start() throws. The getOrElse at :418 reverts state to initialLifecycleState — Stopped (:427) — without nulling the node, releases lifecycleMutex, then delay(2.seconds) (:439) and retries (:440).
2. Reset confirmed inside that window: stop() waits on lifecycleMutex, gets it, sees isStoppedOrStopping() (LightningRepo.kt:618-621) and returns success without touching the node.
3. This line: wipeStorage → stop() succeeds vacuously again → LightningService.wipeStorage :506 throws NodeStillRunning → step() logs a warning and continues.
4. :91 keychain.wipe(), then core, DB, settings. onSuccess() runs, onboarding shows.
5. 2s later the queued retry start() runs. node != null so setup is skipped, and the old seed's node starts on the retained old directory. Create a new wallet and start() sees "LDK node already running" — the new seed's wallet drives the old seed's node. Next process start builds the new seed on the old LDK dir and fails.

Trigger B is the same ending via awaitNodeRelease() throwing NodeReleaseTimeout after 90s when node.destroy() wedges.

Pre-existing, and head is arguably better: on v2.4.1 keychain.wipe() ran unconditionally before wipeStorage(...).getOrThrow(), with the same vacuous early return, so the same trigger left the same on-disk state — but it surfaced an error and dropped the user on a dead wallet screen instead of onboarding. The PR didn't touch LightningRepo/LightningService at all; the vacuous gate is entirely theirs. What changed is that the failure is now only a Logger.warn.

Fix, if you want the first local step to be the gate it reads as — nothing local is destroyed before it, so it's the one safe place to abort:

lightningRepo.wipeStorage(walletIndex).onFailure { return Result.failure(it) }

with wipeLocal returning Result and :59 becoming stopNode().mapCatching { cleanupRemote(); wipeLocal(...).getOrThrow(); onSuccess() }. ~4 lines, no new abstractions. It does not reverse greptile's P1 (that was aborting after LDK deletion) and is compatible with piotr's "treat the remaining local wipes as non-fatal".

One collateral: it flips invoke should complete the wipe when lightningRepo wipeStorage fails (WipeWalletUseCaseTest.kt:288), which encodes the never-abort choice. If that choice is deliberate, leave the code and soften the PR body — "closed by construction" is what I'd push back on, not the design.

[ovitrif]

Agreed, and taken in ee283d4: wipeLocal() returns a Result and the LDK storage wipe is its abort point, since nothing local is destroyed before it. Later local steps stay best-effort. The test now asserts the no-wipe path when wipeStorage fails, and the PR body no longer claims the window is closed by construction alone.

[jvsena42]

ee283d43f does what I asked, and I walked trigger A end to end at the new head to confirm: the vacuous stop() still returns success, wipeStorage still throws NodeStillRunning, and wipeLocal now returns before :92 keychain.wipe(). Nothing local is destroyed. The flipped test is load-bearing — I reverted the three main-code files to 43449afd6 while keeping the new tests and got exactly one failure, invoke should fail before wiping local state when LDK storage wipe fails, and it asserts verify(keychain, never()).wipe() rather than just a Result.

One consequence of my own request, which I should have called out when I made it. The abort lands after cleanupRemote(), so it is now the first exit that leaves the wallet alive with remote state already torn down. At the previous head the only fatal step was stopNode(), before any cleanup, so "keychain intact + remote torn down" was unreachable except by process death in that window.

What the user is left in on trigger A: mnemonic, passphrase, PAYKIT_SESSION, PUBKY_SECRET_KEY, LDK dir, Room and settings all intact — but private endpoints unpublished with contactSharingCleanupPending(false) so nothing retries, public endpoints synced to empty, the receiver marker removed, PAYKIT_SDK_STATE deleted and the cache store reset. The wallet is briefly "Paykit-dark": payers resolving its endpoints get nothing until the next foreground cycle republishes them (AppViewModel.refreshPublicPaykitEndpoints / refreshPrivatePaykitEndpoints), and the SDK handle rebuilds lazily from the surviving session. Backup observers stay stopped until the next Running transition, which is the same shape as the stop()-fails case piotr already accepted.

This is still the better trade — a recoverable half-torn remote beats a destroyed keychain with the old seed's LDK directory left behind — so I am not asking you to change it. Two things worth knowing:

Reset is not permanently blocked in the realistic trigger. 2s after the failed start the queued retry runs, state leaves Stopped, and the next stop() really stops and nulls the node, so a second Reset succeeds. The one contrived exception is a recovery-mode deeplink inside that 2s window, which makes the retry return RecoveryModeError before touching state and leaves every Recovery-screen wipe aborting until relaunch.

If you want the half-torn state unreachable rather than merely recoverable, the root cause is one condition: LightningRepo.stop() :618 treats Stopped as "nothing to stop" while a failed start leaves lightningService.node alive. Requiring lightningService.node == null there turns the vacuous success into a real stop and trigger A never reaches cleanupRemote(). That is in LightningRepo, which this PR deliberately does not touch, so deferring it to the #1256 follow-up is a fine call.

Fix #1 also checks out: the gate is captured once under setupMutex and used at the guard, at complete, and in onFailure in both clients, and isSetup === gate keeps a stale setup from clobbering a fresh deferred. I traced the setupWithRetry path too — an exceptionally-completed gate is isCancelled, so the guard does not short-circuit and the next setup() gets a fresh one. No poisoned gate. Neither client has a test covering the race in either direction; just an observation, not a request.

[ovitrif]

Thanks for walking trigger A through. Took the root fix in ed82850 rather than leaving the half-torn state reachable: LightningRepo.stop() now only short-circuits when the state is stopped and lightningService.node == null, so a node left alive by a failed start gets a real stop before cleanupRemote() runs. Covered by two new cases in LightningRepoTest.kt.

[jvsena42]

ed82850af closes trigger A properly. At :618 Stopped && node != null is now false, so stop() takes the real path and LightningService.stop() handles the never-started node cleanly — listenerJob is null because start() threw before assigning it, node.stop() throws NotRunning which is swallowed, and node = null. stopNode() returns success with the node genuinely gone, so wipeStorage no longer throws NodeStillRunning and the half-torn "wallet alive, remote already cleaned up" state is unreachable via that trigger rather than merely less likely.

I checked the thing I was most worried about — that widening into LightningRepo would regress the shipped stop() path — and it does not. "Stopped with a live node" is bounded to the 2s retry window plus a recovery-mode edge, because the only writer of a non-null node is setup() under start()'s mutex, and every other initial state (Initializing, ErrorStarting) already took the real path before this commit. stopDebounced's 5s delay outlives the 2s retry, so a normal background cycle is unchanged. LightningService.stop() cannot realistically throw (NonCancellable, node.stop() inside runSuspendCatching, releaseHandle catches destroy failures), so no caller inherits a failure it never used to get. The node read is safe: @Volatile at LightningService.kt:171, and writes and reads are both under lifecycleMutex anyway. In recovery mode the new behaviour is strictly better — that state previously made every Recovery-screen wipe abort.

The tests pin it. Reverting line 618 alone fails exactly stop tears down a node object left alive by a failed start with WantedButNotInvoked: lightningService.stop(); the second case passes either way and pins the no-op branch. 115/115 at head.

One refinement, LOW, and it is a consequence of the option I offered rather than a new problem. Now that the retry no longer finds a retained node, it does a full setup() — and LightningRepo.wipeStorage runs lightningService.wipeStorage at :815 outside the mutex that stop() released at :642. So if the 2s retry lands in that gap (most plausibly while awaitNodeRelease() is waiting on a wedged free_node, which the comment at :471 says takes tens of seconds after a failed start), the retry can take the mutex, rebuild from VSS, and bring up an old-seed node while the wipe deletes the directory and clears the keychain around it. The wallet ends up on onboarding with the old seed's node running behind it.

Genuinely LOW: it needs a start that fails into the retry path from a Stopped initial state, a Reset confirmed and cleanupRemote() completed inside ~2s of a failure the UI never surfaces, and either the destroy wedge or a millisecond-exact landing. In the likelier ordering the retry holds the mutex through its rebuild, wipeStorage's stop() waits, and the wipe then succeeds correctly — just slower. And the underlying race is pre-existing: any start() acquiring lifecycleMutex between wipeStorage's stop() and :815 already does this on v2.4.1 via the ON_START path in ContentView. This commit adds a second trigger to an old race rather than creating one.

If you want it closed here, the narrow version is to hold lifecycleMutex across both halves of wipeStorage: split stop() into a stopLocked() and have wipeStorage do lifecycleMutex.withLock { stopLocked().mapCatching { lightningService.wipeStorage(walletIndex); … } }. Both setup() and wipeStorage already gate on awaitNodeRelease(), so nothing else moves. Equally defensible to leave it for the #1256 follow-up, given it predates this PR — your call, and not a blocker either way.

[ovitrif]

Closed here in effa097: stop() delegates to a private stopLocked(), and wipeStorage() holds lifecycleMutex across stopLocked() and lightningService.wipeStorage(), so a start cannot take the lock between the stop and the directory removal. Pinned by a new LightningRepoTest.kt case that blocks the wipe inside the service call and asserts a concurrent start() does not reach the node until the wipe releases; it fails without the lock.

[jvsena42]

ee283d43f closes both LOW observations from my last pass. No blockers.

I verified the wipe fix by reverting the three main-code files to 43449afd6 while keeping the new tests: exactly one failure, invoke should fail before wiping local state when LDK storage wipe fails. So the test is load-bearing rather than decorative, and it asserts verify(keychain, never()).wipe() and verify(db, never()).clearAllTables() rather than just a Result. At head, WipeWalletUseCaseTest 11/11, VssBackupClientTest 6/6, VssBackupClientLdkTest 1/1 all pass. Run in a throwaway worktree outside the repo; nothing committed, working tree untouched.

One consequence of the change I asked for is worth your attention before merge — the abort now lands after cleanupRemote(), so a recoverable "wallet alive, Paykit endpoints unpublished" state is newly reachable. Details and the recovery path are in my reply on the WipeWalletUseCase.kt:88 thread. I'm not asking you to change it; it's a better trade than the leftover-LDK-directory outcome it replaces.

Also checked on the new control flow:

• runSuspendCatching at :59 replacing stopNode().map {} keeps cancellation propagating, and finally still clears isWiping and unlocks. A side benefit: the non-step calls in wipeLocal (the resetState()s, resetWalletState(), onSuccess()) now surface as a Result failure and a toast instead of an uncaught exception in viewModelScope.
• keychain.wipe() is unreachable on the abort path by construction.
• A second concurrent wipe still returns WipeAlreadyInProgress.
• The new abort path leaves the user on ResetAndRestore with Back and Reset re-enabled and a toast — it doesn't wedge isWiping.
• Re-walked all seven exits from ResetAndRestoreScreen (the file didn't change, but the wipe's control flow did): Back while wiping, top-bar back, dialog dismiss, Confirm, drawer navigation mid-wipe, backgrounding, process death before and after the keychain step. All still correct.
• No seed-derived material reaches a log or a toast on any new path — the new lines add no Logger calls, and NodeStillRunning's message is a fixed string.

On the VSS gate: the capture is right in both clients, and isSetup === gate stops a stale setup clobbering a fresh deferred. I traced the setupWithRetry path specifically for a poisoned-gate regression and there isn't one — an exceptionally-completed deferred reports isCancelled, so the guard doesn't short-circuit and the next setup() gets a fresh gate. Neither client has a test covering that race in either direction; noting it, not asking for one.

[jvsena42]

ed82850af is correct. No blockers, nothing new to raise as its own thread — one LOW refinement is on the WipeWalletUseCase.kt:88 thread, and it's a consequence of the change I asked for rather than a defect you introduced.

This commit reaches into LightningRepo, which the PR had deliberately left alone, so I reviewed it as a change to shipped lifecycle code rather than as a one-line follow-up. The question that mattered was whether making the early return conditional regresses stop() on the hot path. It doesn't:

• "Stopped with a live node" is not a routine state. The only writer of a non-null node is setup() under start()'s mutex, and it's reachable only when initialLifecycleState was Stopped — bounded to the 2s retry window, plus a recovery-mode edge. Initializing and ErrorStarting already took the real path before this commit, so a real stop against a set-up-but-unstarted node was already exercised (restartWithElectrumServer failure → restartWithPreviousConfig).
• stopDebounced's 5s delay outlives the 2s retry, so a normal background cycle sees no new behaviour. In recovery mode the change is strictly better: start() returns early without touching state, so Stopped + live node used to persist and make every Recovery-screen wipe abort.
• No caller inherits a new failure. LightningService.stop() can't realistically throw — it's NonCancellable, node.stop() is inside runSuspendCatching, and releaseHandle catches destroy() failures. I walked all ten stop() call sites anyway; the ones that ignore the result (LightningNodeService ×2, WakeNodeWorker, onProceedWithoutRestore) get the intended outcome, and the ones that handle failure already did.
• The node read is safely published: @Volatile at LightningService.kt:171, and both the write at :209 and the read at :618 are under lifecycleMutex regardless.
• The fall-through emits Stopping → Stopped and replaces the whole LightningState, which the early return didn't. Every real stop already does this and observers tolerate it; the retry re-derives isGeoBlocked. No probe-cache leak, since a node that never ran has emitted no events.

Tests are load-bearing: reverting line 618 alone fails exactly stop tears down a node object left alive by a failed start with WantedButNotInvoked: lightningService.stop(). The second case passes in both states, which is right — it pins the no-op branch. 115/115 at head. Throwaway worktree outside the repo, nothing committed, main checkout untouched.

Also confirmed the previous commit's work is byte-identical — the captured VSS setup gate and the fatal LDK-wipe step are untouched, and git diff --stat ee283d43f ed82850af is just these two files.