fix: retry rn tags after restore

[piotr-iohk]

Related to synonymdev/bitkit-android#1258

Twin: synonymdev/bitkit-android#1396
Companion: synonymdev/bitkit-e2e-tests#263

This PR keeps React Native activity tags until the matching activity exists. It does not change when the node starts. The channel wait and the restart in #1258 are Android-only, in the twin.

Description

• Persists unfinished local transfer and boost markers separately, retaining missing activities or failed writes for later syncs without replaying migration completion.
• Keeps tags whose activity has not synced yet, and applies only the retained tags on later syncs instead of replaying the full local backup.
• Separates background metadata retries from migration completion, so old pending tags neither restore tags the user removed nor suppress new payment notices.
• Serializes sync completion tasks and uses isolated persistence plus a fake activity store in retry regression tests.

Out of Scope

• MigrationsService: no change to when the node starts, and no change to how channel monitors are applied. iOS already waits for the React Native channel backup before start.
• The 30s timeout, the failed-restart warning, and keeping the channel file if the node does not stop: Android only, [Bug]: RN remote restore starts the node after 30 s without the RN channel state; LSP force-closes the channel and the monitors are discarded (RN → native restore only) bitkit-android#1258.
• 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 whose tags arrive after the activity list → the tags show up on a later sync — React Native app not in Capabilities

Automated Checks

• added RNMigrationTagRetentionTests.swift — two sync passes retain and reload missing tags, block cleanup until applied, avoid replaying applied tags, and keep failed tag writes pending

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new blocking issue was found.

Summary

The latest changes keep unfinished transfer and boost markers for later syncs.

• Restored activity tags stay saved until their activity is ready.
• Unfinished transfer and boost markers retry when activities sync.

The three previous, unnumbered findings remain addressed: applied tags are not replayed, background retries do not repeat migration completion, and the tag tests exercise two sync passes.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[First migration sync] --> B[Save local transfer and boost markers]
    B --> C[Try pending tags and markers]
    D[Later sync] --> C
    C --> E{Activity found and writes succeed?}
    E -->|No| F[Keep unfinished work saved]
    F --> D
    E -->|Yes| G[Remove completed work]
    G --> H{Pending work remains?}
    H -->|Yes| F
    H -->|No| I[Allow old migration data cleanup]

Loading

_{Reviews (3) · Last reviewed commit: "fix: retain unfinished migration markers" · Reviewed by Greptile}

[talosmachina]

1 finding (1 P1). Keeps React Native tags whose activity has not synced yet and retries them on later syncs. Reviewed 0423fb3, full tier, from the code and CI: iOS does not build on this reviewer's Linux host.

What I checked, and 3 candidates I ruled out

Read in full: MigrationsService.swift (post-sync path, cleanup gate, remote restore), the syncCompleted handler in AppViewModel.swift, the isShowingMigrationLoading observer in AppScene.swift
Call sites traced: reapplyMetadataAfterSync, canCleanupAfterMigration, needsPostMigrationSync
CI: Run Tests, Run Integration Tests and build-local were still pending at review time; validate green

Ruled out

• MMKV path loses tags: applyAllMetadata still ignores the unapplied set, but MMKV metadata is re-extracted from disk on every pass until cleanup, so nothing is lost.
• lastUsedTags dropped: they are written to UserDefaults before the retained metadata strips them.
• Retained metadata survives a wipe: clearPendingMigrationData still nils pendingMetadata.

Merge confidence: 3/5, the unbounded tag retry above.

[piotr-iohk]

@greptileai Please re-review latest head c99cac1. All three findings have fixes and direct replies with validation evidence. Tag retries use only persisted pendingMetadata, migration completion clears its one-time suppression flag while retained data retries independently, and the two-pass persisted retry plus failed-write tests pass. The iOS test build succeeded; all five RNMigrationTagRetentionTests passed.

[greptile-apps]

This PR is set for a Plus review. TREX isn't supported with Plus or Apex reviews yet, so Greptile reviews this PR at Base while TREX is on.

[piotr-iohk]

@greptileai Please review latest head 96f5ca3. The Android twin review identified a shared late transfer/boost retry gap, so this companion fix now persists unfinished local markers before clearing completion and retains missing activities or failed updates for later syncs. All seven migration regression tests passed, including two-pass persistence/reload for transfer and boost markers and failed writes.

[jvsena42]

Code scan at 0423fb3: nothing new from me. The open points on this PR are already raised by other reviewers and are still unfixed at this head, so I am not approving and not repeating them:

• an orphan tag keeps needsPostMigrationSync set, so every sync reruns the full metadata reapply and markAllUnseenActivitiesAsSeen, and migration cleanup never runs;
• the MMKV tag replay re-adds tags the user removed while cleanup is held.

The Android side, synonymdev/bitkit-android#1396, changed both in its latest commit: it clears the flag after the first pass, retries leftover tags outside the migration gate, and drops the MMKV replay. Porting that split here would close both threads.

Checked and clean:

• Retained tags cannot cross wallets: a wipe removes the persisted pending metadata.
• Resolved ids are removed from the pending set, lastUsedTags is applied once, and a cancelled upsertTags keeps its id for the next pass.
• No channel code is touched, and the new log lines carry ids and counts only.

⚠️ Device gate: not run — a device check needs an RN-era remote backup whose metadata references a missing activity, which is not available on my setup. ⚠️ perf: not measured — while a tag is pending the whole reapply runs on every sync; that cost needs the same fixture to measure.

[piotr-iohk]

Following up on the review that says it scanned 0423fb3: both reported paths changed in c99cac1 and are present in latest head 96f5ca3. AppViewModel.swift:1614 captures the one-time completion flag, :1620 sweeps activities only for that first pass, :1622 passes includeLocalMetadata: completingMigration, and :1623 clears needsPostMigrationSync regardless of the cleanup gate. MigrationsService retries tags only from persisted remaining pendingMetadata; the full MMKV tag replay was removed. Cleanup remains deferred while data is pending, while migration UI and notice-suppression flags are cleared. The new two-pass persistence and failed-write tests pass. Please check the current diff; the original findings were fixed rather than deferred.

[talosmachina]

Re-review of 96f5ca3 after 0423fb3. 1 earlier finding: fixed. 0 new findings. The two new commits split the one-time migration completion from the background retries, keep unresolved transfer and boost markers alongside the tags, and add two-pass persistence tests. Full tier, reasoned from the code and CI: iOS does not build on this reviewer's Linux host.

• Bound the tag retry (P1): fixed in c99cac1c. AppViewModel.swift:1623 now clears needsPostMigrationSync after the first pass whatever canCleanupAfterMigration says, so shouldPresentConfirmedOnlyReceive stops suppressing new receives, markAllUnseenActivitiesAsSeen runs once (completingMigration), and the network-required toast in AppScene.swift:537 no longer fires on later launches. An unresolvable id still holds the RN keychain and file cleanup, which the PR states as the intent.

What I checked, and 4 candidates I ruled out

Read in full: the post-sync path of MigrationsService.swift (reapplyMetadataAfterSync, applyRemoteTransfers, applyBoostTransactions, retryPendingMetadata, applyPendingTags, canCleanupAfterMigration, hasPendingMigrationRetries), the syncCompleted handler in AppViewModel.swift, RNMigrationTagRetentionTests.swift
Call sites traced: needsPostMigrationSync (AppScene.swift:537, AppViewModel.swift:1332, the two writers in MigrationsService), isRestoringFromRNRemoteBackup, isShowingMigrationLoading
Compared with: the Android twin synonymdev/bitkit-android#1396 at 10f1435, whose canCleanupAfterMigration gates on the same pending sets
CI: build-local, Run Integration Tests, validate green; Run Tests and e2e-tests-local still pending at review time

Ruled out

• Overlapping syncCompleted events repeat the sweep: isSyncingMigration is claimed on the main actor before the Task starts and released by its defer; a dropped event is followed by the next background sync.
• Unconditional isRestoringFromRNRemoteBackup = false changes behaviour: nothing in Bitkit/ reads that flag, only restoreFromRNRemoteBackup and this handler write it.
• Local activity metadata no longer replays on later passes: applyOnchainMetadata creates the activity when it is missing, so the single local pass loses nothing; on master cleanup also ended the replay after the first successful pass.
• Lightning sync completes the migration before on-chain history exists: the pinned ldk-node only emits SyncCompleted with SyncType::OnchainWallet.

Merge confidence: 4/5, no open findings, but Run Tests had not finished and the restore path has no device run with an unresolvable marker.

[piotr-iohk]

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

• App branch: fix/rn-migration-restore
• App head: 96f5ca3c19b456da07149fd8172a021b27146619
• 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 96f5ca3c19b456da07149fd8172a021b27146619: RN v1.1.6 rn_restore and rn_upgrade, and native 2.5.0 native_restore and native_upgrade.

Completed migration run. All scenario checkout logs confirm E2E main at 67fd0f355ed9673f1f0d81d9caf9c003786f638c, including companion #263. The required migration-result job passed; Slack reporting was skipped. Regular unit, integration, build and E2E CI also passes on this head.

Retry caveat: native restore passed on attempt 2 and RN restore on attempt 3. I inspected both failed-shard artifacts, all available simulator logs, Appium logs, the RN failure screenshot and sampled recording. Both attempt-1 failures were Appium session-creation timeouts before migration assertions (infra). RN attempt 2 remained on Terms of Use after the automated Continue tap and timed out waiting for SkipIntro; the same scenario passed unchanged on attempt 3 (flake). No migration defect is established, and this is a passing matrix with recovered failures. Attempt-1 screenshots/recordings and separate app-log bundles were not supplied.

@jvsena42 The current-head fixes were explained in the earlier reply to your stale-head findings; the migration and remaining regular CI evidence is now available. Please confirm your review against this exact head.