Repository navigation
fix: retry a backup that fails on a busy paykit lock - #1552
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] The change affects the wallet-backup workflow in the app. The PR appears safe to merge, with a non-blocking issue in the overdue-backup warning. Findings
SummaryAdds automatic retries for backups that fail while Paykit is temporarily unavailable.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Backup fails] --> B{Temporary Paykit error?}
B -->|No| C[Keep existing error handling]
B -->|Yes| D[Wait with capped backoff]
D --> E{Still observing and failure matches?}
E -->|No| F[Stop retry]
E -->|Yes| G{Backup still eligible?}
G -->|Yes| H[Schedule backup]
H --> I[Advance required time]
I --> J{Upload succeeds?}
J -->|Yes| K[Clear retry count]
J -->|No| A
Reviews (1) · Last reviewed commit: "fix: retry a backup that fails on a busy..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5b2a1182e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
|
Pushed 1630e0f, answering the Greptile and Codex findings:
|
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Retries a WALLET backup that failed on a temporary Paykit error (SharedStateBusy, ConcurrentUpdate, Transport) with a 10 s backoff doubling to 5 min, instead of waiting for the next change or restart. Reviewed 1630e0f, full tier, reasoned from the code and CI (Bitkit Android branches are not built here).
What I checked, and 4 candidates I ruled out
Read in full: BackupRepo.kt (observers, scheduleBackup, triggerBackup, markBackupFailed, checkForFailedBackups, start/stop observing), PaykitExceptionExt.kt, the three new BackupRepoTest.kt cases
Call sites traced: stopObservingBackups (BackupsViewModel, reset() from WipeWalletUseCase.stopNode), markBackupRequired
CI: build (unit tests), lint, detekt green; e2e send and onchain_boost_receive_widgets still running at review time
Ruled out
- Retry fires after a wipe or a pause: the delayed job checks
isObservingafter removing thefailedBackupRequiredmarker, so a paused repo drops the retry and the status observer reschedules onstartObservingBackupsbecauseshouldBackupno longer sees the marker;reset()stops observing before a wipe. - Retry races a newer change:
failedBackupRequired.remove(category, backupRequired)only matches the exact failedrequiredvalue, andmarkBackupRequiredclears the marker first, so a newer change schedules through the observer and the stale retry is a no-op. - 30-minute warning suppressed by retries:
checkForFailedBackupstakes the earlier oftemporaryFailureSinceandstatus.required, so repeated retries bumpingrequireddo not reset the window; covered by the newstays pending past the failure windowtest. - Shift overflow: the doubling is capped at
MAX_RETRY_SHIFT = 6and coerced to 5 min, so the attempt counter growing does not overflow the delay.
Merge confidence: 4/5, the change is covered by BackupRepoTest.kt but two e2e jobs were still running when I read it.
ben-kaufman
left a comment
ben-kaufman
left a comment
There was a problem hiding this comment.
Reviewed 1630e0f. The retry, warning-age, and paused-observation fixes look good, with no blocking findings. All 72 BackupRepo tests pass. I inspected the complete Detekt report: its 78 findings in changed files map to unchanged base code, with no introduced finding identified. Current hosted checks are green.
3 participants
Closes #1550
Refs:
This PR retries a backup that fails because the Paykit shared state is busy, instead of leaving it until the next change or app start.
Description
shared_state_busy,concurrent_update, a transport error) staying unbacked for the rest of the session. The WALLET snapshot reads Paykit state under its shared-state lock, and a failed backup was only retried once the data changed again or the app restarted. It is now retried after 10 s, doubling up to 5 minutes, which outlasts the lock's write cooldown, while the same required snapshot is still pending.Backup failedat ERROR. Other failures keep the current handling and log.Out of Scope
Design
N/A — no UI changes.
Preview
N/A — no user-visible changes.
QA Notes
Journeys
N/A — no user-visible behaviour change.
Manual Tests
N/A
Automated Checks
BackupRepoTest.kt— a wallet backup that fails on a busy Paykit shared state is retried and uploaded without a new change, also across a pause in observation, and still warns once it stays pending past 30 minutes