fix: keep rn channels on restore

[piotr-iohk]

Fixes #1258

Twin: synonymdev/bitkit-ios#852
Companion: synonymdev/bitkit-e2e-tests#263

This PR keeps a React Native remote restore from opening the Lightning node before the channel backup arrives, and from throwing that backup away if the restart fails.

Description

• Persists unfinished local transfer and boost markers separately, retaining missing activities or failed writes for later syncs without replaying migration completion.
• Waits past the 30s restore timeout only while a React Native remote restore is still running, so the node starts with the channel backup instead of zero channels.
• Restarts the node with that channel file when migration finishes, and leaves the file in place if the node does not stop or the restart fails. A failed restart shows the existing migration warning.
• Keeps activity tags whose matching activity is not ready yet, and retries only the retained tags on later syncs.
• Separates background metadata retries from the one-time migration completion sweep, so missing old tags do not suppress new payment notices.
• Serializes channel-backup application with wallet startup, install-on-top migration, and restore Retry; completion rechecks pending data after startup consumes it.
• Rebuilds restore Retry with retained channel data through the existing lifecycle-aware restart path, preserves requested background stops during internal retries, and keeps the backup on failure or a deferred restart.

Out of Scope

• WalletViewModel: install-on-top and a normal VSS restore still use the 30s timeout.
• AppViewModel: the channel file is not persisted across process death.
• iOS: no extra wait and no channel-file restart. iOS already fetches the backup before the node starts. The twin only keeps tags that are not ready yet.
• Migration tests: the spending balance check stays at 100000 sats.
• E2E sheet dismissals during RN setup and after relaunch: test: stop dismissing absent rn sheets bitkit-e2e-tests#263.

Design

N/A — no UI changes.

Preview

N/A

Migration runs

Runs of this change:

• Android
• iOS

rn_restore still failed in both. Android stopped on the receive celebration. iOS passed the first balance check and stopped on the QuickPay intro after relaunch. Those two are covered by the companion.

QA Notes

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

• Restore an RN 1.1.6 wallet that already has spending balance → spending stays funded and the channel is not force-closed — React Native app and remote channel backup not in Capabilities

Automated Checks

• added WalletViewModelTest.kt — an RN remote restore still running after 30s does not start the node; restore Retry uses the channel file and retains it on failed or deferred restarts; install-on-top startup holds the migration lock until consumption
• added LightningRepoTest.kt — channel-aware restarts yield to a background stop during a failed first attempt, preserve stops requested during the retry, and keep channel data across bounded startup attempts
• added MigrationServiceTest.kt — activity tags that are not ready yet are kept for a later sync
• added AppViewModelSendFlowTest.kt — a channel file is kept when the node is still running after stop; background retries do not mark new activities seen; completion rechecks channel data after startup releases its lock

[greptile-apps]

Confidence Score: 5/5

[High impact] The PR appears safe to merge; Restore Retry now preserves requested background stops and retained channel data.

Summary

This PR waits for React Native channel backups before starting Lightning and retains unfinished migration data for later syncs.

• React Native restores apply the channel backup before the node starts.
• Migration markers and activity tags stay pending until their records are ready.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Restore Retry] --> B[Lock channel migration]
  B --> C[Read pending backup]
  C --> D[Stop and restart node]
  D --> E{Result}
  E -->|Success| F[Consume pending backup]
  E -->|Failed or deferred| G[Keep pending backup]
  F --> H[Unlock channel migration]
  G --> H
  G --> I[Foreground startup retries retained backup]

Loading

_{Reviews (5) · Last reviewed commit: "fix: preserve background stops during re..." · Reviewed by Greptile}

[github-actions]

Regtest APK

Built from eab911c (run).

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

[talosmachina]

No findings. Waits out an RN remote restore before starting the node, applies the channel file on the post-migration restart instead of discarding it, and retries tags whose activity is not there yet without holding the migration open. Reviewed 824ab61, full tier, from the code and CI: this reviewer does not build Android branches.

What I checked, and 5 candidates I ruled out

Read in full: the changed hunks of MigrationService.kt with restoreFromRNRemoteBackup, reapplyMetadataAfterSync, the cleanup gate and the persistence helpers; handleSyncCompleted, retryPendingMigrationData, completeMigration, completeRNRemoteBackupRestore, applyPendingChannelMigration and the finish paths in AppViewModel.kt; start, waitForRestoreIfNeeded, restoreFromBackup and startNode in WalletViewModel.kt
Call sites traced: peekPendingChannelMigration, consumePendingChannelMigration, lockChannelMigration, canCleanupAfterMigration, needsPostMigrationSync
CI: build and lint still pending on 824ab61 at review time

Ruled out

• An orphan tag keeps needsPostMigrationSync set forever: true at 83c8d60, fixed by 824ab61, which clears the flag after the first pass in every completion path and leaves leftover tags to retryPendingMigrationData. That path skips the sweep and the node restart, as background tag retry does not sweep new activities or restart the node checks.
• Unbounded wait in waitForRestoreIfNeeded: restoreFromBackup always leaves InProgress (Completed or BackupFailed, both outside isOngoing), so the wait ends with the restore.
• Channel file applied twice: startNode consumes it on a successful start under the same mutex, and applyPendingChannelMigration re-reads it after taking the lock, so it returns early. Checked by migration completion rechecks pending channels after startup releases the lock.
• completeMigration drops the file on error: the old unconditional consumePendingChannelMigration() is gone and a failed restart keeps it, as pending channel migration is kept when the node stays running after stop checks.
• Cleanup with a pending channel file: the new peekPendingChannelMigration() check in canCleanupAfterMigration holds cleanup back.

Merge confidence: 4/5, no findings, but CI was still running and the channel-restart path is covered by unit tests only, not by a passing rn_restore migration run.

[piotr-iohk]

@greptileai Please re-review the latest head 824ab61. Migration completion now clears its one-time flag even when metadata remains pending; later syncs retry metadata without sweeping activities or suppressing new receive notices. Wallet startup and channel application share a lock, with pending channel data read after acquiring it. Regression tests, compilation and lint passed locally.

[piotr-iohk]

@greptileai Please re-review latest head 10f1435. The late transfer/boost finding is now fixed with separately persisted marker retries, retention on missing activities or failed writes, and regression tests. Direct reply to the finding includes implementation and validation evidence. The twin iOS PR carries the same fix.

[jvsena42]

One MEDIUM inline at 10f1435, and one LOW as a reply on the double-import thread. Both are reproduced by unit tests.

Checked and clean:

• The wait before node start is unbounded only while an RN remote restore is running, and it terminates: the restore always leaves the in-progress state, the lock is taken after the wait and released in finally.
• A repeated import cannot roll state back: ldk-node rc.66 never overwrites an existing channel manager and only writes a monitor with a higher update id. Read at the pinned tag.
• applyPendingChannelMigration consumes the file only after a successful start, keeps it when the node will not stop, and unlocks in finally.
• The first SyncCompleted after a remote restore takes completeMigration, which clears both flags; later tag retries never mark activities as seen or restart the node.
• The MMKV tag replay is gone and tags still arrive through the persisted metadata.
• No key material or channel bytes in the new log lines.
• For users who never migrated, the per-sync retry check returns before doing any work.
• 10f1435 only changes how unapplied transfer and boost markers are retained in MigrationService.kt; WalletViewModel.kt and AppViewModel.kt are unchanged from what I reviewed.

Not posted: pending tags whose activity never appears are re-persisted on every sync with no cap. The retry itself is what the linked issue asks for; a guard that skips the write when nothing changed would remove the cost.

⚠️ Device gate: not run — a device check needs an RN-era install to upgrade and a seed with an RN remote backup holding channels, which is not available on my setup. ⚠️ perf: not measured — the restore now waits for the whole RN download before starting the node, and every sync reads the migration store; both need the same fixture to measure.

[piotr-iohk]

@greptileai Please review latest head 4b82c1d. New review follow-up fixes the two uncovered Android startup paths: install-on-top migration now holds the shared channel lock through consumption; restore Retry rebuilds with retained channel data under that lock and retains the backup on stop/start failures. Direct replies include evidence. Compilation, all 3644 unit tests, and lint passed.

[piotr-iohk]

@talosmachina Please re-review latest head 4b82c1d. It includes retained transfer/boost markers from 10f1435 and now covers install-on-top startup plus restore Retry under the shared migration lock. Retry passes the channel backup before node startup, and failed stop/start retains it. All 3644 unit tests, compilation, and lint passed. Please confirm any remaining actionable findings on this exact head.

[talosmachina]

Re-review of 4b82c1d after 824ab61. No findings. 0 earlier findings of mine to carry. The two commits since keep unapplied transfer and boost markers in the retry queue until their write succeeds. They also put install-on-top startup and restore Retry under the channel-migration lock, and Retry now builds the node with the pending channel file. Full tier, reasoned from the code and CI: this reviewer does not build Android branches.

What I checked, and 5 candidates I ruled out

Read in full: git diff 824ab61..4b82c1d. In MigrationService.kt: reapplyMetadataAfterSync, applyRemoteTransfers, applyBoostTransactions, the persist and clear helpers, canCleanupAfterMigration. In WalletViewModel.kt: checkAndPerformRNMigration, onRestoreRetry, start, startNode. In AppViewModel.kt: applyPendingChannelMigration, completeMigration, retryPendingMigrationData. In LightningRepo.kt: stop/stopLocked, startNode, restartNode
Call sites traced: lockChannelMigration (WalletViewModel.kt:184, :297, :355 and AppViewModel.kt:1940, nothing else), consumePendingChannelMigration, restartNode, onRestoreRetry (ContentView.kt:422, only from the ErrorStarting error view)
CI on 4b82c1d: build (including Run tests), build-local, lint and detekt green

@jvsena42's two threads

• Install-on-top start outside the lock: fixed. checkAndPerformRNMigration now reads the file and starts the node while it holds the lock (WalletViewModel.kt:184-190), so a SyncCompleted emitted during that start waits. Once it has the lock it sees the file already consumed.
• Retry starts without the channel file: fixed. onRestoreRetry reads the retained file under the lock and runs stop() then startNode(0, channelMigration) instead of restartNode(). The file is consumed only in startNode's onSuccess, so a failed stop or start leaves it pending. Covered by the three restore retry ... tests.

Ruled out

• Deadlock on the new lock sites: the Mutex is not reentrant, but nothing reached from startNode's success path (connectMigrationPeers, cleanupInvalidMigrationTransfers, syncBalances) takes it. LDK events are handled in viewModelScope.launch per event, so a completion waiting on the lock never blocks the start that holds it.
• Retry's stop() on a node that never built: after a failed setup lightningService.node is null, so stopLocked only moves the state to Stopped and the start goes ahead.
• Local markers re-applied after they landed: they are merged into the queue only while needsPostMigrationSync() is true, and re-applying one is idempotent (isTransfer/boostTxIds are set, not appended twice).
• Lost boost on a partial CPFP write: remaining.remove sits after both getOrThrow() calls, so if either write fails the marker stays for the next sync. failed transfer and boost writes remain pending covers this.
• runCatching swallowing cancellation in the boost writes: the switch to runSuspendCatching rethrows it.

Merge confidence: 4/5. No findings and CI green, but the channel-restart path is still covered only by unit tests: the rn_restore migration run linked in the body did not pass.

[piotr-iohk]

@greptileai Please review latest head eab911c. The new background-stop finding is fixed by passing channel migration data through LightningRepo.restartNode, preserving its existing yield-to-stop behavior during bounded retries. Failed or deferred restarts retain the channel backup. Direct reply includes evidence; compilation, all 3647 unit tests, and lint passed.

[piotr-iohk]

@talosmachina Please review latest head eab911c. All prior findings have fixes and direct replies. The latest follow-up routes channel-aware restore Retry through the existing lifecycle-aware restart path so background stops still win; install-on-top remains locked through channel consumption. Compilation, all 3647 unit tests, and lint passed.

[jvsena42]

Approving at eab911c: both points from my last review are fixed and I have no new finding in the two commits since.

• Restore Retry and the install-on-top start now hold the channel-migration lock from the peek to the consume, and release it in finally.
• Lock order: every holder takes the migration lock before the lifecycle mutex and nothing takes them the other way round, so migration completion waiting on the lock cannot block the start that holds it.
• The retained file is consumed only after a successful start, on both consume sites.
• The background-stop point raised by the other review holds at head: Retry no longer calls start(), a stop requested during the failed first attempt makes the retry yield, and one requested during the second attempt is not cancelled.
• restartNode() without a file behaves as before for its existing callers.
• The new tests pin the fix: the Retry and install-on-top tests fail on the previous head.

Read only for this round; unit tests, lint and e2e are green in CI at this head.

⚠️ Device gate: not run — it needs a wallet upgraded from the React Native app with open channels, which is not available on my setup.

[talosmachina]

Re-review of eab911c after 4b82c1d. No findings. 0 earlier findings of mine to carry. The one commit since routes a restore Retry that has a retained channel file through LightningRepo.restartNode(channelMigration), so the retry yields to a background stop instead of cancelling it. Full tier, reasoned from the code and CI: this reviewer does not build Android branches.

What I checked, and 4 candidates I ruled out

Read in full: git diff 4b82c1d..eab911c. In LightningRepo.kt: restartNode, start, the private startNode including its retry and graph-reset branches. In WalletViewModel.kt: onRestoreRetry, startNode
CI on eab911c: build, build-local, detekt, the eight e2e-tests-local shards and e2e-status green; the migration jobs were skipped

Ruled out

• Retry loses the channel file: restartNode hands channelMigration to the first attempt and the bounded retry passes it again (LightningRepo.kt:484-493), so both attempts build the node with it. Covered by channel migration restart keeps the backup across bounded start attempts.
• Backup consumed on a deferred restart: a yield returns NodeStartYieldedToStopError from restartNode, so WalletViewModel.startNode takes onFailure, skips consumePendingChannelMigration and shows no toast. Covered by restore retry keeps pending channels when restart yields to background stop.
• A stop requested during the retry gets cancelled: the retry runs with shouldCancelPendingStop = !shouldRetryYieldToStop, which is false on this path. Covered by channel migration restart retry does not cancel a stop requested during the retry.
• Failed stop now unreported: the old inline stop() failure toast is gone, but restartNode returns the stop failure as Result.failure, which reaches the same Logger.error and ToastEventBus.send in startNode's onFailure, and the file stays pending.

Merge confidence: 4/5, no findings and the new path is unit-tested, but the rn_restore migration scenario did not run on this head.

[piotr-iohk]

Scheduled the standard migration test matrix, including rn_restore, against this PR branch.

• App branch: fix/rn-migration-restore
• App head: eab911c87c1ef918a19c52a2cca49ee91c851c1e
• E2E tests: main (head at dispatch: 67fd0f355ed9673f1f0d81d9caf9c003786f638c), including merged companion test: stop dismissing absent rn sheets bitkit-e2e-tests#263.
• Migration run

I will inspect the results and investigate any failed scenarios. Slack reporting is disabled.

[piotr-iohk]

Migration matrix passed on current app head eab911c87c1ef918a19c52a2cca49ee91c851c1e: RN v1.1.6 rn_restore and rn_upgrade, and native 2.5.0 native_restore and native_upgrade.

Completed migration run. Job checkout logs confirm E2E main at 67fd0f355ed9673f1f0d81d9caf9c003786f638c, including companion #263. The required migration-result job passed; Slack reporting was skipped as requested. This supplies the previously missing rn_restore CI evidence.

Retry caveat: rn_restore and native_restore each passed on attempt 2 after an attempt-1 failure. I inspected both failed-shard artifacts, screenshots, sampled recordings, logcat and available app logs. RN failed while setting up the old RN wallet on the transfer confirmation screen, before native migration started. Native restore reached Wallet Restored and restored tagged history, then the test read an onchain-only Auto QR while expecting a Lightning invoice (app logs confirm an invoice was created). Both identical checks passed on unchanged app and E2E commits, so these are classified as flakes; the artifacts do not establish a migration defect. This is a passing matrix with recovered failures, not a first-attempt clean run.