Repository navigation
fix: retry rn tags after restore - #852
Conversation
Tags whose activity had not synced were cleared after one attempt. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
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:
applyAllMetadatastill ignores the unapplied set, but MMKV metadata is re-extracted from disk on every pass until cleanup, so nothing is lost. lastUsedTagsdropped: they are written toUserDefaultsbefore the retained metadata strips them.- Retained metadata survives a wipe:
clearPendingMigrationDatastill nilspendingMetadata.
Merge confidence: 3/5, the unbounded tag retry above.
|
@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. |
|
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. |
This comment has been minimized.
This comment has been minimized.
|
@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
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
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
needsPostMigrationSyncset, so every sync reruns the full metadata reapply andmarkAllUnseenActivitiesAsSeen, 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,
lastUsedTagsis applied once, and a cancelledupsertTagskeeps its id for the next pass. - No channel code is touched, and the new log lines carry ids and counts only.
|
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
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
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:1623now clearsneedsPostMigrationSyncafter the first pass whatevercanCleanupAfterMigrationsays, soshouldPresentConfirmedOnlyReceivestops suppressing new receives,markAllUnseenActivitiesAsSeenruns once (completingMigration), and the network-required toast inAppScene.swift:537no 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
syncCompletedevents repeat the sweep:isSyncingMigrationis claimed on the main actor before theTaskstarts and released by itsdefer; a dropped event is followed by the next background sync. - Unconditional
isRestoringFromRNRemoteBackup = falsechanges behaviour: nothing inBitkit/reads that flag, onlyrestoreFromRNRemoteBackupand this handler write it. - Local activity metadata no longer replays on later passes:
applyOnchainMetadatacreates the activity when it is missing, so the single local pass loses nothing; onmastercleanup 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
SyncCompletedwithSyncType::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.
|
Scheduled the standard migration test matrix, including
I will inspect the results and investigate any failed scenarios. Slack reporting is disabled. |
|
Migration matrix passed on current app head Completed migration run. All scenario checkout logs confirm E2E 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. |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@greptileai Please review current head 92551ad. The wallet-wipe race is fixed by invalidating retry generations and draining the active completion task before databases/defaults are cleared. Suspended tag/transfer/boost lookups cannot write or repersist old metadata. Added actual syncCompleted caller tests for overlapping completion, suppression clearing and a later unseen receive. 32 relevant simulator tests passed, 0 failed; 12 existing destructive cleanup cases skipped by design. Both new inline findings have direct replies with evidence. |
|
Scheduled the standard migration matrix, including
Slack reporting is disabled. I will inspect the final results and artifacts for any failed or recovered attempts. |
|
@greptileai Please re-review current head 3c5e869. Both latest findings have fixes and direct replies: failed wipes restore migration retries after the cancelled task drains; the overlap test no longer invokes MMKV metadata replay or host activity writes and snapshots/disables unrelated restore sweep/prune flags. Added a throwing-wipe caller regression. All33 relevant simulator tests passed, 0 failures/skips; formatting and diff checks passed. |
|
Updated migration validation for current head This replaces run 38044721991 for the preceding app head; the workflow concurrency policy cancels the superseded run. The new run covers the failed-wipe and test-isolation follow-ups. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Migration validation passed on current app head All four standard scenarios passed on their first test attempt: RN v1.1.6 Actual E2E checkout in all consumer jobs: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-read at 3c5e869: no HIGH or MEDIUM finding, and nothing new to raise inline.
Follow-up on my last review:
- The orphan-tag loop is fixed in
c99cac1:needsPostMigrationSyncis cleared after the first pass whether or not cleanup can run, andmarkActivitiesSeenruns only on the completing pass, so receive sheets are no longer held and the seen sweep is not repeated. - The MMKV replay is fixed in
c99cac1: tags now come only from the persistedpendingMetadata, applied ids are dropped from it, and retained metadata carrieslastUsedTags: nil.
Checked in the two wipe commits (92551ad, 3c5e869):
AppReset.wiperunswipeLocalinsidewithMigrationSyncPausedForWipe: it setsisResettingMigration, bumpsretryGeneration, cancels the running task and awaits it before any storage is touched. Every write after a suspension in the retry path is behindisRetryCurrent(generation), which also checksTask.isCancelled.- The defaults domain is removed in the wipe, so the next wallet starts with no
rnPending*keys, and the only in-memory state on theMigrationsServicesingleton (retryGeneration) was already bumped. I found no path that applies wallet A's pending tags to wallet B. - A cancelled pass returns the full pre-pass set and returns before
pendingMetadatais rewritten, so cancellation leaves a superset, never a half-cleared marker. Re-applying is safe becauseupsert_tagsisINSERT OR IGNORE. - A failed wipe resets
isResettingMigrationand rethrows, and the nextsyncCompletedresumes the retries without a second seen sweep. isSyncingMigrationis claimed on the main actor before the task is created and released indefer, so overlappingsyncCompletedevents run one pass.
CI is green at this head, including the rn_restore migration scenario.
Device gate (partial): 3c5e869 — not run yet. Pending: the retry rate and cost with one unresolvable marker, measured against master on the simulator (the PR adds a pass on every syncCompleted for as long as a marker is unresolved, with no bound, so it is performance-relevant). I will post the numbers here when the run finishes.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Device result for 3c5e869: one MEDIUM inline. The code scan from my earlier review today stands; nothing else changed.
Device gate: 3c5e869 — journeys: none listed; perf: 6.0 post-migration passes/min with one unresolvable marker (master 0 after one startup pass), each followed by an ACTIVITY backup upload (18 in 3 min, master 0), pass 351 ms median / 996 ms max steady over 6 payments and 98 activities, CPU 1.0 % mean (master 2.8 %), Receive 0.90 s / Send 0.60 s after idle (master 0.57 s / 0.52 s, 0.5 s polling resolution, n=3); no passes in background, two within 0.4 s on return; no Android run (synonymdev/bitkit-android#1396 is merged).
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 5 files.
No new findings; the rest is in the review.
The same persisted minute-to-hour backoff is in synonymdev/bitkit-android#1462. Android already skips background history rebuilds; this delta aligns iOS with that behavior.
QA:
Tests running.
1 not tested because the supplied activity gate times out before the first wallet sync can finish; missing: an activity gate that completes wallet sync; fix: synonymdev/bitkit-e2e-tests#275.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
There was a problem hiding this comment.
Verdict: ✅ Approve
QA for 7029c52:
Tests for the review: Test 1 not tested: our setup stopped it before the app; Tests 2 and 3 pass.
Tested on iOS 26.5 simulator (iPhone 17 Pro), regtest.
1 not tested because the supplied activity gate times out before wallet sync can finish; missing: an activity gate that completes wallet sync; fix: synonymdev/bitkit-e2e-tests#275.
🟢 Tests 2, 3
Test 2
On 7029c52 + test hook: unresolved metadata follows exponential backoff to an hourly cap without rebuilding history or uploading unchanged activity.
2.mp4 |
test hook
Not pushed. Prepares one missing transfer marker, controls the injected migration clock, and logs real retry/history/backup calls.
diff --git a/Bitkit/Services/BackupService.swift b/Bitkit/Services/BackupService.swift
index 864d5e70..2328a7af 100644
--- a/Bitkit/Services/BackupService.swift
+++ b/Bitkit/Services/BackupService.swift
@@ -232,6 +232,9 @@ class BackupService {
} else {
try await getBackupDataBytes(category: category)
}
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("--qa-rn") { Logger.info("QA_RN backup-upload category=\(category.rawValue) bytes=\(data.count)", context: "TestHook") } // test hook
+ #endif // test hook
try await uploadBackup(category.rawValue, data)
updateBackupStatus(category: category) { status in
diff --git a/Bitkit/Services/MigrationsService.swift b/Bitkit/Services/MigrationsService.swift
index 45d6cbe9..e7620e6c 100644
--- a/Bitkit/Services/MigrationsService.swift
+++ b/Bitkit/Services/MigrationsService.swift
@@ -507,7 +507,15 @@ class MigrationsService: ObservableObject {
init(userDefaults: UserDefaults = .standard, now: @escaping () -> Date = Date.init) {
self.userDefaults = userDefaults
+ #if DEBUG // test hook
+ self.now = { // test hook
+ let clock = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0].appendingPathComponent("qa-rn-clock") // test hook
+ if ProcessInfo.processInfo.arguments.contains("--qa-rn"), let raw = try? String(contentsOf: clock, encoding: .utf8), let seconds = Double(raw.trimmingCharacters(in: .whitespacesAndNewlines)) { return Date(timeIntervalSince1970: seconds) } // test hook
+ return now() // test hook
+ } // test hook
+ #else // test hook
self.now = now
+ #endif // test hook
}
// MARK: - UserDefaults Helpers
@@ -829,6 +837,28 @@ extension MigrationsService {
set { setCodable(newValue, forKey: Self.rnRetryScheduleKey) }
}
+ #if DEBUG // test hook
+ @MainActor func qaReadMigrationCommand() { // test hook
+ guard ProcessInfo.processInfo.arguments.contains("--qa-rn") else { return } // test hook
+ let commandURL = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0].appendingPathComponent("qa-rn-command") // test hook
+ guard let command = try? String(contentsOf: commandURL, encoding: .utf8), command != userDefaults.string(forKey: "qaRnLastCommand") else { return } // test hook
+ let parts = command.trimmingCharacters(in: .whitespacesAndNewlines).split(separator: "|").map(String.init) // test hook
+ guard parts.count == 3 else { return } // test hook
+ if parts[0] == "transfer" { pendingRemoteTransfers = [parts[2]: "1"] } // test hook
+ else if parts[0] == "tag" || parts[0] == "tag-delayed" { pendingRemoteTransfers = nil; pendingMetadata = RNMetadata(tags: [parts[2]: ["qa-late-retry"]], lastUsedTags: nil) } // test hook
+ else { return } // test hook
+ retrySchedule = nil // test hook
+ needsPostMigrationSync = false // test hook
+ if parts[0] == "tag-delayed" { recordPendingMigrationRetry() } // test hook
+ userDefaults.set(command, forKey: "qaRnLastCommand") // test hook
+ Logger.info("QA_RN seeded kind=\(parts[0]) id=\(parts[2])", context: "TestHook") // test hook
+ } // test hook
+ func qaMigrationSnapshot(_ stage: String) { // test hook
+ guard ProcessInfo.processInfo.arguments.contains("--qa-rn") else { return } // test hook
+ Logger.info("QA_RN \(stage) now=\(now().timeIntervalSince1970) attempted=\(retrySchedule?.attemptedAt.timeIntervalSince1970 ?? -1) delay=\(retrySchedule?.delay ?? -1) pending=\(hasPendingMigrationRetries) initial=\(needsPostMigrationSync)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
+
var shouldRetryPendingMigrationData: Bool {
hasPendingMigrationRetries && (retrySchedule?.isDue(at: now()) ?? true)
}
diff --git a/Bitkit/ViewModels/AppViewModel.swift b/Bitkit/ViewModels/AppViewModel.swift
index 15953add..1b34bc2d 100644
--- a/Bitkit/ViewModels/AppViewModel.swift
+++ b/Bitkit/ViewModels/AppViewModel.swift
@@ -271,6 +271,10 @@ class AppViewModel: ObservableObject {
self.navigationViewModel = navigationViewModel
self.scanPaymentOperations = scanPaymentOperations ?? .live(lightningService: lightningService)
+ #if DEBUG // test hook
+ migrations.qaReadMigrationCommand() // test hook
+ migrations.qaMigrationSnapshot("startup") // test hook
+ #endif // test hook
setupManualEntryValidationDebounce()
Task {
@@ -1411,6 +1415,10 @@ extension AppViewModel {
}
private func beginPostMigrationSync() {
+ #if DEBUG // test hook
+ migrations.qaReadMigrationCommand() // test hook
+ migrations.qaMigrationSnapshot("sync-event") // test hook
+ #endif // test hook
guard !isResettingMigration, !isSyncingMigration,
migrations.needsPostMigrationSync || migrations.shouldRetryPendingMigrationData
else { return }
@@ -1423,7 +1431,13 @@ extension AppViewModel {
self.postMigrationSyncTask = nil
}
guard migrations.isRetryCurrent(generation) else { return }
+ #if DEBUG // test hook
+ migrations.qaMigrationSnapshot("pass-begin") // test hook
+ #endif // test hook
if completingMigration {
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("--qa-rn") { Logger.info("QA_RN full-history-rebuild", context: "TestHook") } // test hook
+ #endif // test hook
await postMigrationSyncOperations.syncPayments()
guard migrations.isRetryCurrent(generation) else { return }
await postMigrationSyncOperations.markActivitiesSeen()
@@ -1433,6 +1447,9 @@ extension AppViewModel {
guard migrations.isRetryCurrent(generation) else { return }
migrations.needsPostMigrationSync = false
migrations.recordPendingMigrationRetry()
+ #if DEBUG // test hook
+ migrations.qaMigrationSnapshot("pass-end") // test hook
+ #endif // test hook
if migrations.canCleanupAfterMigration {
if migrations.isShowingMigrationLoading {Test 3
On 7029c52 + test hook: the saved retry deadline survives relaunch, and late activity recovers its tag without suppressing the new-payment notification.
3.mp4 |
test hook
Not pushed. Prepares a saved tag and first retry deadline for a real staging transaction while offline, then observes unchanged production recovery.
diff --git a/Bitkit/Services/BackupService.swift b/Bitkit/Services/BackupService.swift
index 864d5e70..2328a7af 100644
--- a/Bitkit/Services/BackupService.swift
+++ b/Bitkit/Services/BackupService.swift
@@ -232,6 +232,9 @@ class BackupService {
} else {
try await getBackupDataBytes(category: category)
}
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("--qa-rn") { Logger.info("QA_RN backup-upload category=\(category.rawValue) bytes=\(data.count)", context: "TestHook") } // test hook
+ #endif // test hook
try await uploadBackup(category.rawValue, data)
updateBackupStatus(category: category) { status in
diff --git a/Bitkit/Services/MigrationsService.swift b/Bitkit/Services/MigrationsService.swift
index 45d6cbe9..e7620e6c 100644
--- a/Bitkit/Services/MigrationsService.swift
+++ b/Bitkit/Services/MigrationsService.swift
@@ -507,7 +507,15 @@ class MigrationsService: ObservableObject {
init(userDefaults: UserDefaults = .standard, now: @escaping () -> Date = Date.init) {
self.userDefaults = userDefaults
+ #if DEBUG // test hook
+ self.now = { // test hook
+ let clock = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0].appendingPathComponent("qa-rn-clock") // test hook
+ if ProcessInfo.processInfo.arguments.contains("--qa-rn"), let raw = try? String(contentsOf: clock, encoding: .utf8), let seconds = Double(raw.trimmingCharacters(in: .whitespacesAndNewlines)) { return Date(timeIntervalSince1970: seconds) } // test hook
+ return now() // test hook
+ } // test hook
+ #else // test hook
self.now = now
+ #endif // test hook
}
// MARK: - UserDefaults Helpers
@@ -829,6 +837,28 @@ extension MigrationsService {
set { setCodable(newValue, forKey: Self.rnRetryScheduleKey) }
}
+ #if DEBUG // test hook
+ @MainActor func qaReadMigrationCommand() { // test hook
+ guard ProcessInfo.processInfo.arguments.contains("--qa-rn") else { return } // test hook
+ let commandURL = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0].appendingPathComponent("qa-rn-command") // test hook
+ guard let command = try? String(contentsOf: commandURL, encoding: .utf8), command != userDefaults.string(forKey: "qaRnLastCommand") else { return } // test hook
+ let parts = command.trimmingCharacters(in: .whitespacesAndNewlines).split(separator: "|").map(String.init) // test hook
+ guard parts.count == 3 else { return } // test hook
+ if parts[0] == "transfer" { pendingRemoteTransfers = [parts[2]: "1"] } // test hook
+ else if parts[0] == "tag" || parts[0] == "tag-delayed" { pendingRemoteTransfers = nil; pendingMetadata = RNMetadata(tags: [parts[2]: ["qa-late-retry"]], lastUsedTags: nil) } // test hook
+ else { return } // test hook
+ retrySchedule = nil // test hook
+ needsPostMigrationSync = false // test hook
+ if parts[0] == "tag-delayed" { recordPendingMigrationRetry() } // test hook
+ userDefaults.set(command, forKey: "qaRnLastCommand") // test hook
+ Logger.info("QA_RN seeded kind=\(parts[0]) id=\(parts[2])", context: "TestHook") // test hook
+ } // test hook
+ func qaMigrationSnapshot(_ stage: String) { // test hook
+ guard ProcessInfo.processInfo.arguments.contains("--qa-rn") else { return } // test hook
+ Logger.info("QA_RN \(stage) now=\(now().timeIntervalSince1970) attempted=\(retrySchedule?.attemptedAt.timeIntervalSince1970 ?? -1) delay=\(retrySchedule?.delay ?? -1) pending=\(hasPendingMigrationRetries) initial=\(needsPostMigrationSync)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
+
var shouldRetryPendingMigrationData: Bool {
hasPendingMigrationRetries && (retrySchedule?.isDue(at: now()) ?? true)
}
diff --git a/Bitkit/ViewModels/AppViewModel.swift b/Bitkit/ViewModels/AppViewModel.swift
index 15953add..1b34bc2d 100644
--- a/Bitkit/ViewModels/AppViewModel.swift
+++ b/Bitkit/ViewModels/AppViewModel.swift
@@ -271,6 +271,10 @@ class AppViewModel: ObservableObject {
self.navigationViewModel = navigationViewModel
self.scanPaymentOperations = scanPaymentOperations ?? .live(lightningService: lightningService)
+ #if DEBUG // test hook
+ migrations.qaReadMigrationCommand() // test hook
+ migrations.qaMigrationSnapshot("startup") // test hook
+ #endif // test hook
setupManualEntryValidationDebounce()
Task {
@@ -1411,6 +1415,10 @@ extension AppViewModel {
}
private func beginPostMigrationSync() {
+ #if DEBUG // test hook
+ migrations.qaReadMigrationCommand() // test hook
+ migrations.qaMigrationSnapshot("sync-event") // test hook
+ #endif // test hook
guard !isResettingMigration, !isSyncingMigration,
migrations.needsPostMigrationSync || migrations.shouldRetryPendingMigrationData
else { return }
@@ -1423,7 +1431,13 @@ extension AppViewModel {
self.postMigrationSyncTask = nil
}
guard migrations.isRetryCurrent(generation) else { return }
+ #if DEBUG // test hook
+ migrations.qaMigrationSnapshot("pass-begin") // test hook
+ #endif // test hook
if completingMigration {
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("--qa-rn") { Logger.info("QA_RN full-history-rebuild", context: "TestHook") } // test hook
+ #endif // test hook
await postMigrationSyncOperations.syncPayments()
guard migrations.isRetryCurrent(generation) else { return }
await postMigrationSyncOperations.markActivitiesSeen()
@@ -1433,6 +1447,9 @@ extension AppViewModel {
guard migrations.isRetryCurrent(generation) else { return }
migrations.needsPostMigrationSync = false
migrations.recordPendingMigrationRetry()
+ #if DEBUG // test hook
+ migrations.qaMigrationSnapshot("pass-end") // test hook
+ #endif // test hook
if migrations.canCleanupAfterMigration {
if migrations.isShowingMigrationLoading {Note
Tests 2 and 3 are checked in the PR description.
Tip
Tests 2, 3 worth a journey
Test 2
- Restore a fresh wallet on staging.
- Receive a payment and wait for its activity backup.
- Prepare an unresolved legacy marker with the debug hook.
- Leave the wallet active and check retries before and at every deadline.
- Verify saved metadata remains and no unchanged activity backup uploads.
Test 3
- Fund a new transaction while the wallet is stopped and offline.
- Seed one pending tag and initial deadline with the debug hook.
- Relaunch before the deadline without reseeding.
- Restore the network and verify the ordinary receive notification while metadata stays pending.
- Advance to the original deadline and verify recovered tag and cleared retry state.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
QA for 7029c52: The run is still going; the final review follows when it ends. This is a progress note, nothing is approved or requested here. This comment is updated as tests finish and removed when the final result posts. 2 passed so far, 1 not tested. 🟠 Test 1: not tested because the supplied activity gate times out before wallet sync can finish; missing: an activity gate that completes wallet sync; fix: https://github.com/synonymdev/bitkit-e2e-tests/pull/275; waits for round 2The RN restore case was not tested because the supplied activity gate cannot complete the wallet-sync request batch. Tests 2, 3 passed at 7029c52. |
There was a problem hiding this comment.
Code scan of 7029c52 (the one commit since my last review): no new findings. Device result follows.
- The guard now requires
needsPostMigrationSync || shouldRetryPendingMigrationData(AppViewModel.swift:1414-1416), and the schedule doubles from 60 s to a 3600 s cap (MigrationsService.swift:315-326), persisted inrnMigrationRetryScheduleand read on every check, so a relaunch keeps the deadline. - A retry-only pass no longer calls
syncPayments()or the seen sweep (AppViewModel.swift:1426-1431). With unresolved markersreapplyMetadataonly reads, so nothing reachesactivitiesChangedSubjectand no ACTIVITY backup should follow a retry. - The schedule is written behind
isRetryCurrent(generation), so a pass cancelled by a wipe writes nothing;wipeLocalremoves the key with the markers, andclearPendingMigrationDataclears it on cleanup. - Clock rollback fires one pass and keeps doubling; out-of-range stored delays are clamped.
- Blocktank order re-fetches now wait for the same schedule (up to an hour at the cap). That follows from the backoff I asked for and was looked at and ruled out by another reviewer on synonymdev/bitkit-android#1462 (the data stays persisted and is still recovered); not a finding.
Device gate (partial): 7029c52 — not run yet; pending the same measurement as before (one unresolvable marker, 3 minutes idle, plus a relaunch before and after the deadline).
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 7029c52. The MEDIUM from my last review is fixed and measured on the device; the code scan of this commit is in my earlier review today.
Device gate: 7029c52 — journeys: none listed; perf: 4 post-migration passes in 8 minutes with one unresolvable marker (18 in 3 minutes at 3c5e869), gaps 67 s / 142 s across a relaunch / 249 s, stored delay 480 s at the end; 0 ACTIVITY backups caused by a retry pass (one per pass before).
Skipped — covered by CI at 7029c52: unit tests, integration tests, e2e (all green).
3 participants
Related to synonymdev/bitkit-android#1258
Twin: synonymdev/bitkit-android#1396
Companion: synonymdev/bitkit-e2e-tests#263
Retry-backoff companion: synonymdev/bitkit-android#1462
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
Pending metadata retries now persist an exponential schedule from one minute to an hourly cap, retaining missing tags, transfers and boosts across restarts. After the first migration completion, retries reapply pending data without rebuilding full payment history, which avoids unchanged ACTIVITY backup uploads.
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.
Invalidates retry snapshots on wallet reset and drains the active completion task before wiping databases/defaults; stale tag, transfer and boost lookups cannot write or repersist data for the next wallet. If the wipe throws, retries resume on the surviving app.
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.Design
N/A — no UI changes.
Preview
N/A
Migration runs
The standard matrix passed on
96f5ca3cin iOS run 37923566716, includingrn_restore. Native restore passed on attempt 2 after an Appium session timeout; RN restore passed on attempt 3 after a session timeout and an onboarding flake. Failed-attempt artifacts were inspected and results posted on the PR. E2E revision:67fd0f355ed9673f1f0d81d9caf9c003786f638c, including merged companion #263.The wallet-wipe follow-up at
3c5e8699passed all four standard scenarios on the first attempt in iOS run 38045396729, includingrn_restore. Actual E2E revision:492dbfcc890a0ab9d97bc801ffbe428f164dfc6e. Slack reporting was disabled.QA Notes
Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
1 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
2 Restore with an unresolved activity marker and leave the app active through repeated wallet syncs → automatic metadata retries follow the persisted 1/2/4/8-minute backoff, capped at hourly; unresolved-only passes do not rebuild payment history or upload an unchanged ACTIVITY backup — fault-injected RN-era metadata and backup inspection not in Capabilities
3 Relaunch before the next retry is due, then make the matching activity available → backoff survives restart, and the saved marker applies on a later eligible sync without suppressing new receives — delayed RN-era activity fixture 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 pendingadded
RNMigrationSyncLifecycleTests.swift— suspended tag/transfer/boost lookups cannot write or resurrect old metadata after reset; reset drains in-flight work; overlappingsyncCompletedevents complete once, clear suppression, and a later background retry leaves a new receive unseenOctober 10 wallet-reset follow-up: 32 relevant simulator tests passed, 0 failed; 12 existing destructive cleanup tests skipped by their opt-in guard. SwiftFormat and
git diff --checkpassed.Failed-wipe and test-isolation follow-up: 33 selected migration/restore/receive-guard simulator tests passed, 0 failed or skipped; the caller test does not read RN files or write the host wallet history.
added
RNMigrationRetryScheduleTests.swift— real UserDefaults reload, sync-frequency suppression, exponential/hourly boundaries, late tag recovery, schedule reset and clock rollbackupdated
RNMigrationSyncLifecycleTests.swift— initial completion bypasses backoff, due background retries do not rebuild history or sweep new receives, and wipe cancellation remains coveredRetry-backoff follow-up: 19 focused migration simulator tests passed on
7029c52d, 0 failed or skipped; SwiftFormat, whitespace checks and independent full local review of the iOS/Android fixes passed. The idle-backup device measurement is proposed, not claimed executed.Ovi QA approval on
7029c52d: Manual Tests 2 and 3 passed on an iOS 26.5 regtest simulator using the disclosed unpushed test hooks. Backoff avoided full-history rebuilds and unchanged ACTIVITY uploads; the saved deadline survived relaunch, and a late activity recovered its tag without suppressing a new-payment notification. Test 1 remains untested: the supplied activity gate cannot complete wallet sync (E2E Force close transactions with BlockTank not being confirmed #275). These are Ovi’s results, not a device run performed in this chat.