feat: add sync_backup() method to force a vault backup on first backup#296
feat: add sync_backup() method to force a vault backup on first backup#296thomas-waite wants to merge 13 commits intomainfrom
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds an explicit, host-invokable backup trigger to the credential vault storage layer so apps can perform an initial backup upload before any further vault mutations.
Changes:
- Added
CredentialStore::sync_backup()to trigger the backup notification flow without mutating the vault. - Added tests covering
sync_backup()behavior for both non-empty and empty vaults.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@codex review |
![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: ea81c5ef6b
ℹ️ 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".
| /// | ||
| /// Returns an error if no backup manager is configured, if the vault | ||
| /// export fails, or if the backup manager callback fails. | ||
| pub fn sync_backup(&self) -> StorageResult<()> { |
There was a problem hiding this comment.
Hmm this is creating a cyclical pattern of Foreign -> Rust -> Foreign (and potentially more bindings after). Is this initial sync necessary? If there are no credentials there should be nothing to backup right?
There was a problem hiding this comment.
Just to be clear - it's the same case with regular credential store calls - they're also Foregin -> Rust -> Foregin
There was a problem hiding this comment.
Yep, the existing backup solution in Oxide is already doing this. I'm following the same pattern. The initial sync is necessary to backup whatever v4 credentials have already been put into the credential vault when the user enables backups (the callback based backup logic is only triggered when the vault is mutated)
walletkit-core/src/storage/traits.rs
Outdated
| /// does not affect the vault mutation that triggered this call. Returning | ||
| /// `Result` (rather than `()`) ensures that host-side exceptions are | ||
| /// translated into a Rust `Err` by `UniFFI` instead of panicking. | ||
| /// Returning `Err` propagates the error to the caller of the vault |
There was a problem hiding this comment.
It's unrelated to this PR but I'm realizing this could lead to very weird states where a credential has been saved to the vault, backup fails and an error is returned. The native app won't know the credential is saved and retry which will error. We should split the backup failures as error logs and/or return a separate status for this, but not fail the entire original call
There was a problem hiding this comment.
Yeah agree, should have a seperate status for this/error logs. Will do
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dependency not gated behind optional storage feature
- Made parking_lot optional and added it to the storage feature list, matching the pattern of other storage-only dependencies.
Or push these changes by commenting:
@cursor push ecdb2e7875
Preview (ecdb2e7875)
diff --git a/walletkit-core/Cargo.toml b/walletkit-core/Cargo.toml
--- a/walletkit-core/Cargo.toml
+++ b/walletkit-core/Cargo.toml
@@ -46,7 +46,7 @@
strum = { version = "0.27", features = ["derive"] }
subtle = "2"
thiserror = "2"
-parking_lot = "0.12"
+parking_lot = { version = "0.12", optional = true }
tokio = { version = "1", features = ["sync"] }
tracing = "0.1"
tracing-log = "0.2"
@@ -95,6 +95,7 @@
storage = [
"dep:ciborium",
"dep:hkdf",
+ "dep:parking_lot",
"dep:rand",
"dep:sha2",
"dep:uuid",This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Add
sync_backup(), to allow for a first time backup upload before vault mutationsWhen a user first creates a backup in a host app, it's an explicit action taken by a user to make a backup. It is not in response to a vault mutation. So, we need a method that can force a backup of the vault to be made if the host app needs one.
Added
sync_backup()to do this. Also done a slight refactor of the internal logic to propagate errors, similar to how the existing backup system works.Note
Medium Risk
Touches core credential storage synchronization by replacing
std::sync::Mutexwithparking_lot::Mutexand removing poison-error handling, which could affect concurrency behavior. Adds a new backup-trigger path (sync_backup) that exports plaintext vault snapshots on demand, so backup integrations should be revalidated.Overview
Adds
CredentialStore::sync_backup()to explicitly export the current vault and invoke the registeredWalletKitBackupManager::on_vault_changedcallback even when no vault mutation occurred (e.g., first-time backup setup), with new tests covering empty and non-empty vault cases.Refactors storage locking to use
parking_lot::Mutex(new optional dependency under thestoragefeature), simplifying APIs by removingResultfromstorage_paths/pathsand eliminating poisoned-lock error paths; related authenticator tests and integration tests are updated accordingly. Backup export/callback failures remain best-effort and are now more explicitly logged/documented as non-propagated.Written by Cursor Bugbot for commit 7b86e91. This will update automatically on new commits. Configure here.