[pwltr]

Fixes #562

This PR prevents background backup checks from surfacing a stale error toast when Bitkit is unlocked.

Description

• Gates backup failure notifications on the active scene and treats an active retry as in progress rather than failed.
• Retries pending, idle app-managed backups on foregrounding before reporting overdue failures, so restored connectivity can recover them without a stale toast.
• Limits foreground alert suppression to one minute without cancelling uploads, so a stalled retry cannot hide other overdue backup failures indefinitely.
• Preserves the original pending timestamp when retrying and the ten-minute notification cooldown, so genuine failures are not repeatedly postponed or over-reported.
• Rechecks outstanding backup state immediately before emitting an alert and prevents foreground recovery from duplicating running or scheduled uploads.
• Retains pending backup requirements when recovery is cancelled, and prevents a cancelled capture from starting an upload.

Out of Scope

• Periodic background retry behavior: unchanged; the additional recovery pass runs on foregrounding.
• LDK-managed Lightning connection backups: remain managed by ldk-node.
• The existing 30-minute failure threshold and ten-minute notification cooldown: unchanged.

Design

N/A — no design available.

Preview

N/A — behavior-only change.

QA Notes

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

• Wallet running → enable Network Link Conditioner at 100% loss → change a backed-up setting → lock the device for over 30 minutes → restore connectivity while locked and unlock → the pending backup recovers without a stale failure toast — device lock/unlock and Network Link Conditioner fault injection not in Capabilities.
• regression: keep Bitkit active with a backup pending for over 30 minutes → the backup failure toast still appears and repeated alerts observe the ten-minute cooldown — Network Link Conditioner fault injection not in Capabilities.
• regression: leave connectivity unavailable with an overdue backup → background and reopen Bitkit → the retry fails, the overdue failure is still reported, and reopening again within ten minutes does not repeat the toast — Network Link Conditioner fault injection not in Capabilities.

Automated Checks

• added BackupFailureNotificationGateTests.swift — covers inactive suppression, active and recovered backup states, foreground recovery suppression, safe wall-clock comparisons and notification cooldown behavior.
• added BackupForegroundRecoveryTests.swift — covers recovery of all app-managed categories without a stale alert, genuine failures retaining their age and cooldown, no duplicate running or scheduled uploads, cancellation retaining pending requirements, suppression during restore/wipe or without observation, stalled retries not hiding other failures, and late completion not releasing a newer recovery window.

[greptile-apps]

Confidence Score: 3/5

[Medium risk] Changes backup failure notification logic and timing.

The PR should not merge until prolonged failures remain visible across short sessions and recovered backups cannot trigger a queued failure toast.

Findings

1.  Short sessions hide backup failures ▶
2.  Recovered backups still trigger toasts ▶

Summary

The PR gates backup-failure toasts on scene activity, adds a foreground grace period and safe timestamp comparison, and tests the notification gate.

• The grace period can suppress alerts indefinitely across short foreground sessions.
• A backup that recovers between failure detection and notification can still produce a stale toast.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Periodic overdue-backup check] --> B{Backup required?}
  B -->|Yes| C[Queue notification work]
  C --> D{Active long enough and cooldown elapsed?}
  D -->|Yes| E[Send failure toast]
  D -->|No| F[Suppress]
  G[Scene becomes active] --> H[Reset active-since time]
  H --> D
  I[Backup succeeds after check] --> J[Clear required status]
  J -. No status recheck .-> E

Loading

_{Reviews (1) · Last reviewed commit: "fix: suppress stale backup error toast (..."}

[pwltr]

Manual verification completed on the iOS Simulator using temporarily shortened failure intervals:

• Forced a backup failure with Network Link Conditioner set to 100% Loss.
• Confirmed an in-flight backup remains shown as Running rather than producing a premature failure alert.
• Restored connectivity before unlocking and confirmed no stale backup failure toast appeared.
• Confirmed the pending backup completed normally after connectivity returned.
• Repeated with connectivity still unavailable and confirmed the genuine failure path remains available after the request finishes.

The production failure intervals were restored after verification.

[jvsena42]

The immediate check on foreground adds a new stale-toast path at cold launch, the same class this PR removes. Details inline; the second comment is a smaller, optional point.

Checked and clean:

• The gate does not drop notifications permanently: shouldNotify returns before consuming the cooldown when inactive, and every setAppActive(true) re-runs the check. A Task that lands after backgrounding is suppressed without consuming the cooldown.
• The recovered/running status is re-read before emitting, under statusUpdateQueue, matching the writes in updateBackupStatus.
• No deadlock: stateQueue and statusUpdateQueue never sync back onto the backup queue.
• A persistent failure while active still toasts at +30 min and then every 10 min, as on master.
• The wall-clock guards replace a UInt64 underflow trap on master with a skip, which is the right outcome when the clock is behind required.
• Wipe clears backupStatuses, so the launch check during onboarding finds nothing.

No Android twin: BackupRepo.checkForFailedBackups has neither the scene gate nor the in-progress exemption.

[piotr-iohk]

QA reviewed on 27367a5.

QA review

No new actionable code findings.

QA review

Reviewed the full PR diff against its merge base 521ce0f, at 27367a5. Full review of the current diff, with no inherited baseline.

No new actionable code findings.

The pass covered scene-phase gating in AppScene, the failure gate, the status recheck on statusUpdateQueue, the in-progress exclusion, cooldown handling, and BackupFailureNotificationGateTests, plus the launch order of startObservingBackups and scheduleFullBackup.

Earlier threads were checked on this revision. The foreground grace timer is gone, so a short active session no longer postpones an overdue alert (short sessions). The queued notification re-reads backup status before it consumes the cooldown (recovered backups). The immediate foreground check runs only when backup observation has already started and the scene is still active, so a cold or offline launch does not toast from the first .active callback (cold launch). A running upload still withholds this toast until that attempt finishes; that limitation stays in the existing thread.

Android parity was not part of this review.

Validation: Source was inspected at 27367a5. The new gate tests were read and were not executed in this review. Run Tests and Run Integration Tests succeeded on this commit. The local e2e job was still queued and is not evidence for this gate. The author reports a simulator pass with shortened failure intervals (comment); that report is not a test performed in this review. The PR's two manual cases still apply: recovery while locked, and an active overdue failure that honors the ten-minute cooldown. No new Appium spec is indicated; the path depends on a 30-minute failure age and network-loss injection, and this PR does not change journeys or accessibility identifiers.

Additional test cases

• Persisted overdue failure (required, not running, older than 30 minutes). Relaunch online. The splash and PIN screen stay free of a backup-failure toast while scheduleFullBackup retries. If that retry fails and the backup is still required and not running after observation has started, the toast comes from the periodic check.
• Same persisted failure with the device offline. Launch Bitkit. The backup-failure toast stays absent while the node never starts.
• Scene active, one backup running with required older than 30 minutes. The failure toast stays absent until that attempt ends in failure. A success ends with no toast.
• Observation already started, backup overdue and not running. Foreground for under a minute, background, and repeat. One toast appears on an active scene. Backgrounding leaves the ten-minute cooldown in place, so a second toast waits until that cooldown has elapsed.

Device testing: not performed in this review.

Ready for device testing.

[jvsena42]

27367a5: fixed. The foreground check now runs only once isObserving is set, which happens on node .running, before scheduleFullBackup(). A cold launch therefore leaves the first check to the periodic loop, after the retry. Later foregrounds still get the immediate recheck, and the gate re-checks isActive after the hop to the backup queue. No new findings.

Addressed in 27367a5.

[jvsena42]

No finding at 70b6ae5; the tree is the one I read at 27367a5.

Checked: the gate never consumes the cooldown when it suppresses (inactive scene, throttled, or the re-read under statusUpdateQueue finding no failure); a failure that is still overdue is re-checked on every .active and by the periodic loop; the cold-launch .active is skipped until isObserving; the >= guards replace the UInt64 underflow on master; wipe and restore reset the statuses the check reads.

build-local is red at this head because the run was cancelled during package resolution (run 36606127919, conclusion cancelled), and e2e-status failed for lack of its artifact. Run Tests and the integration tests passed. The e2e workflow needs a re-run before merge.

⚠️ Device gate: not run — the two Manual Tests need a backup upload held failing for 30 minutes (Network Link Conditioner at 100 % loss), which on the simulator cuts the host's network as well, so it is not available on my setup. The approval rests on the code scan, the unit tests and CI. perf: not relevant.

[piotr-iohk]

@pwltr please resolve the merge conflicts. QA review has not been performed for this request.

[piotr-iohk]

QA review

Scope: Full reassessment of the entire PR diff against merge base b45b22a, at c58ffc3. The saved assessment had no usable coverage baseline, so nothing was inherited.

No new actionable code findings.

The scene-activity gate, the observation guard on the foreground check, the status re-read on statusUpdateQueue before the cooldown is consumed, the in-progress exclusion, and the ten-minute cooldown match the behavior described for #562. A cold or offline launch does not emit from the first .active callback, because that check waits until backup observation has started. Short returns to the foreground do not reset the cooldown. Earlier comments on short sessions, recovered backups, and cold launch do not describe this revision. A backup left running by a stalled upload still withholds this toast; that limitation remains the accepted note in the existing thread.

Android BackupRepo.checkForFailedBackups at 31b05e7 still emits from the periodic loop with no foreground or in-progress gate. That difference is the iOS lock/unlock fix, not a defect in this PR.

Validation: Source was inspected at c58ffc3. BackupFailureNotificationGateTests was read and was not executed in this review. Run Tests and Run Integration Tests succeeded on this commit. The local e2e job was still queued and is not evidence for this gate. The author reports a simulator pass with shortened failure intervals (comment); that report is not a test performed in this review.

Device testing: not performed in this review.

Ready for device testing.

Suggested additional test cases

• iOS. Persisted overdue failure (required, not running, older than 30 minutes). Relaunch online. The splash and PIN screen stay free of a backup-failure toast while scheduleFullBackup retries. If that retry fails and the backup is still required and not running, the toast appears after that attempt finishes.
• iOS. Same persisted failure, device offline. Launch Bitkit. The backup-failure toast stays absent while the node never starts.
• iOS. Scene active, one backup running with required older than 30 minutes. The failure toast stays absent until that attempt ends. A success ends with no toast. A failure ends with the toast.

[jvsena42]

Code scan at c58ffc3: no issues found. The rebase replays the same patches against current master (only context and offsets changed), so I checked how they behave against the new master code.

Checked and clean:

• Cold-launch fix still holds: the foreground check waits for isObserving (BackupService.swift:192-211), the scene-phase observer keeps initial: true (AppScene.swift:535), and nothing returns early before setAppActive (:1254).
• A real failure is never silenced. Every new master path that sets running: true (scheduleBackup, markBackupRequired(schedule:), the start of runBackup) ends with running: false on success, failure, cancellation or loop exit. A .failed outcome stops the loop and leaves the backup required, and both the periodic check and the failure catch report it.
• Paykit wait: PaykitPaymentActivity.waitUntilIdle() keeps running true, but that means the attempt hasn't run yet, not that it failed.
• Deadlock: none in the emit-time re-read under statusUpdateQueue.
• Reset paths: wipe clears the statuses, restore marks them synced, init clears running, and stopObservingBackups clears isObserving.
• The hung JWT refresh stays the accepted limitation from the earlier thread.

Perf: not relevant. l10n: no UI text changed.

Device gate (partial): c58ffc3 — not run yet. Pending: the two Manual Tests (a backup held failing past the 30-minute window, then foreground and cold launch). Device results will follow in a separate review.

[jvsena42]

Device gate (partial): c58ffc3 on iPhone 17 (simulator), Manual Test 2 (app active, backup failing for more than 30 minutes): passed.

To switch the failure on and off live, I used a scratch-only fault hook that is not PR code: VssBackupClient.putObject throws notConnectedToInternet while a flag file exists. LDK's own VSS path is unaffected. The same hook is on a master build for the baseline.

The app stayed active on Home throughout:

• 11:51:45: the ACTIVITY and METADATA backups fail. SETTINGS fails at 11:52:52, and BLOCKTANK retries every 2 min and keeps failing.
• 12:21:49: WARN: Backup failed for more than 30 minutes. The "Data Backup Failure" toast is on screen at 12:21:51.
• 12:31:51 and 12:41:53: repeats, about 10 min 01 s apart, so the cooldown holds.

So a real, ongoing failure is still reported while the app is active.

Seen on master's code too, not from this PR: only BLOCKTANK retries on a timer. A failed ACTIVITY, METADATA or SETTINGS backup stays failed until retried from Settings → Data backups.

Still running: Manual Test 1 (locked for more than 30 minutes, connectivity restored while locked, then unlock and cold launch).

[jvsena42]

Device gate (partial 2): c58ffc3, Manual Test 1 (locked for more than 30 minutes, connectivity restored while locked, then unlock). The unlock step fails; the cold launch passes. Same build and fault hook as partial 1, starting with every backup category succeeded.

• 12:44:59: Backup failed for: 'SETTINGS'. Locked at 12:45:04.
• 13:17:11: uploads unblocked while still locked, 32 min after the failure. Unlocked at 13:17:19.
• On unlock: "Data Backup Failure — Bitkit failed to back up wallet data. Retrying in 1 minute." appears 0.7 s after the scene turns active. Nothing retries SETTINGS on foreground; only BLOCKTANK's periodic job runs. SETTINGS stays failed until the next cold launch.
• Cold launch (13:20:16): no toast. The startup backup retries SETTINGS, METADATA and ACTIVITY, and all succeed by 13:20:31.

[13:17:19.937 UTC] INFO: Scene phase changed: inactive - AppScene
[13:17:20.670 UTC] INFO: Scene phase changed: active - AppScene
[13:17:20.671 UTC] WARN: Backup failed for more than 30 minutes - BackupService [BackupService.swift: showBackupFailureNotification() line: 774]
[13:17:26.275 UTC] DEBUG: Backup starting for: 'BLOCKTANK' - BackupService
[13:17:27.088 UTC] INFO: Backup succeeded for: 'BLOCKTANK' - BackupService
(no 'SETTINGS' line between 13:17:20 and 13:20:08)

Reading of the code: setAppActive(true) runs checkForFailedBackups() immediately. A category whose last upload failed is still isRequired && !running with required over 30 min old, so it counts as failed, and no retry starts before the check. The foreground recheck therefore still reports the stale failure.

Still running: the same sequence on master with the same hook, to tell whether this changed. The finding and its severity will follow once that baseline lands.

[jvsena42]

One MEDIUM from the device inline: Manual Test 1 (unlock after connectivity returns) still shows the stale backup-failure toast, the same as master.

Device gate: c58ffc3 — manual tests: 1 passed, 1 failed (Manual Test 1, unlock step; the cold-launch part passes); perf: not relevant; Figma: n/a; l10n: no UI text changed.

• Manual Test 2 passes. A backup failing for more than 30 minutes while active still shows the toast (12:21:51), and the repeats keep the 10-minute cooldown (12:31:51, 12:41:53).
• The code scan from earlier today stands: no real failure is silenced, and the reset paths are clean.
• Seen on master's code too, not from this PR: only BLOCKTANK retries on a timer, so a failed ACTIVITY, METADATA or SETTINGS backup waits for a manual retry or a cold launch.

[jvsena42]

Delta at 06824ec: the MEDIUM is fixed in code (trace on the thread). One new LOW is inline, introduced by the recovery window.

Checked and clean:

• One retry per foreground, guarded by isRecovering, and suppression doesn't use up the cooldown.
• No duplicate concurrent backups for a category: retry registration and triggerBackup share the serial backup queue.
• A failed retry keeps the original required age, so a real failure is still reported.
• Cold launch is unchanged.
• No lock-order inversion between stateQueue and statusUpdateQueue.
• Cancelling now keeps required.

Perf-relevant: each switch back to active starts one upload per required idle category. I'll count Backup starting for: lines per Scene phase changed: active on the device.

Device gate (partial): 06824ec — not run yet. Pending: Manual Test 1 (recovery on unlock), the real-failure-still-reported case, and Manual Test 2 (active regression), with the same fault hook as before. Device results will follow in a separate review.

[ovitrif]

@pwltr the red SwiftFormat check here comes from master's own files, which #1040 fixed (merged). Syncing this branch with master turns it green; if it still fails after that, run SwiftFormat 0.64.0 on this PR's changed files.

[piotr-iohk]

QA review

Scope: Follow-up review of changes since c58ffc3, including the affected backup paths, at 317736d. Inherited baseline: QA review, covering AppScene scene-phase gating, the cold-launch order and offline skip, and Android BackupRepo.checkForFailedBackups at 31b05e7.

No new actionable code findings.

Foregrounding retries required, idle, app-managed backups before the overdue check. The check still runs only when the scene is active and backup observation has started. Alert suppression ends when those retries finish or after 60 seconds, and the uploads keep running. A backup that is still running stays out of the failure predicate, so another overdue failure can be reported once the window ends. Retries keep the original pending timestamp. The ten-minute cooldown is left unconsumed while the window is open. Cancelling recovery keeps the pending requirement and does not upload.

The stale unlock toast reported on c58ffc3 in the device thread is the path this recovery targets. That walkthrough has not been repeated on this commit. The open-window hang described in the later thread is bounded by waitForForegroundRetries.

Android checkForFailedBackups at 31b05e7 still reports an overdue required backup from its periodic check, with no scene gate, running exclusion, or foreground retry. That difference is the iOS fix.

Validation: Source was inspected at 317736d. BackupForegroundRecoveryTests and BackupFailureNotificationGateTests were read and were not executed in this review. Run Tests succeeded on this commit. Run Integration Tests was canceled at its one-hour limit after unrelated UTXO and Blocktank failures, so it is not evidence for this gate. The author reports the focused backup tests passed on a simulator (comment); that report is not a test performed in this review.

Device testing: not performed in this review. The failed Manual Test 1 result belongs to c58ffc3, not this commit.

Ready for device testing.

Suggested additional test cases

• iOS. Wallet running, with overdue settings and metadata backups. Settings upload fails immediately and metadata upload stays open for more than one minute. Foreground Bitkit. The settings failure toast appears while metadata is still uploading, and the metadata upload is not cancelled. When metadata later succeeds, the settings backup remains outstanding.
• iOS. Kill Bitkit with an overdue idle app-managed backup, then relaunch online. The splash and PIN screen stay free of a backup-failure toast before the launch retry. If that retry succeeds, no toast appears. If it fails and the backup is still required and not running, the toast appears after that attempt.

[piotr-iohk]

Backup recovery and notification simulator checks passed at 317736d on iPhone 17 / iOS 26.0:

• Kept a failed SETTINGS backup pending while locked for 31 minutes. Restored upload connectivity while still locked, then unlocked: foreground retry succeeded in 0.325 seconds with no stale backup-failure toast.
• Kept an app-managed backup failing while active for over 30 minutes: Data Backup Failure appeared, then repeated approximately ten minutes later (599.966 seconds), with no intermediate alert.
• Backgrounded and reopened the still-failing wallet twice during cooldown: one SETTINGS retry per foreground, no duplicate alert, and the original pending timestamp was preserved. Restoring uploads afterward recovered the backup.

These were controlled simulator tests using a scratch-only DEBUG upload-failure hook at VssBackupClient.putObject (URLError.notConnectedToInternet), rather than host-wide Network Link Conditioner. BackupService and AppScene were unchanged from this head; production timing intervals were retained with no timestamp backdating. LDK-managed VSS was untouched.

fixed