From 21bcc8cae334593970e25ddae860a1919d0c1fcd Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Thu, 12 Mar 2026 14:47:34 -0700 Subject: [PATCH 01/11] feat: expose sync backup method --- walletkit-core/src/storage/credential_storage.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index ed2d61da8..6f3d8cd98 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -306,6 +306,16 @@ impl CredentialStore { })? = manager; Ok(()) } + + /// Triggers a one-off backup sync without mutating the vault. + /// + /// Call this at initial backup creation to catch up any credentials + /// that were stored before the backup was set up. The registered + /// [`WalletKitBackupManager`] receives the same `on_vault_changed` + /// callback as after a normal vault mutation. + pub fn sync_backup(&self) { + self.notify_vault_changed(); + } } /// Implementation not exposed to foreign bindings From 713f4c8430f16c47b990a6a854f0e5e7d72e25d3 Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Thu, 12 Mar 2026 14:50:33 -0700 Subject: [PATCH 02/11] test: add vault sync tests --- .../src/storage/credential_storage.rs | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index 6f3d8cd98..2fbf161f2 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -1518,4 +1518,57 @@ mod tests { cleanup_test_storage(&root); cleanup_test_storage(&export_dir); } + + #[test] + fn test_sync_backup_triggers_notification_without_mutation() { + use world_id_core::Credential as CoreCredential; + + let (store, manager, root, export_dir) = setup_store_with_backup(); + + // Store a credential so the vault is non-empty. + let cred: Credential = CoreCredential::new() + .issuer_schema_id(100) + .genesis_issued_at(1000) + .into(); + store + .store_credential(&cred, &FieldElement::from(7u64), 9999, None, 1000) + .expect("store credential"); + assert_eq!(manager.call_count(), 1); + + // sync_backup triggers a notification without mutating the vault. + store.sync_backup(); + assert_eq!(manager.call_count(), 2); + assert!( + manager.last_file_existed(), + "exported vault file should exist during the callback" + ); + let path = manager.last_path().unwrap(); + assert!( + !std::path::Path::new(&path).exists(), + "exported vault file should be cleaned up after the callback" + ); + + // Vault contents are unchanged — still one credential. + let credentials = store.list_credentials(None, 1000).expect("list"); + assert_eq!(credentials.len(), 1); + + cleanup_test_storage(&root); + cleanup_test_storage(&export_dir); + } + + #[test] + fn test_sync_backup_on_empty_vault() { + let (store, manager, root, export_dir) = setup_store_with_backup(); + + // sync_backup on an empty vault should still trigger a notification. + store.sync_backup(); + assert_eq!(manager.call_count(), 1); + assert!( + manager.last_file_existed(), + "exported vault file should exist during the callback" + ); + + cleanup_test_storage(&root); + cleanup_test_storage(&export_dir); + } } From f804103c27c496eceeb394187aab18068ae33d68 Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Thu, 12 Mar 2026 14:51:42 -0700 Subject: [PATCH 03/11] test: asset issuer is the same --- walletkit-core/src/storage/credential_storage.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index 2fbf161f2..e1e04c02c 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -1548,9 +1548,10 @@ mod tests { "exported vault file should be cleaned up after the callback" ); - // Vault contents are unchanged — still one credential. + // Vault contents are unchanged — still the same credential. let credentials = store.list_credentials(None, 1000).expect("list"); assert_eq!(credentials.len(), 1); + assert_eq!(credentials[0].issuer_schema_id, 100); cleanup_test_storage(&root); cleanup_test_storage(&export_dir); From 08ba463d849bb00aca7a5df383431bea957ab632 Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Thu, 12 Mar 2026 15:27:52 -0700 Subject: [PATCH 04/11] feat: propagate errors on sync_backup, swallow when best effort --- .../src/storage/credential_storage.rs | 58 +++++++++++-------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index e1e04c02c..446ded713 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -313,8 +313,17 @@ impl CredentialStore { /// that were stored before the backup was set up. The registered /// [`WalletKitBackupManager`] receives the same `on_vault_changed` /// callback as after a normal vault mutation. - pub fn sync_backup(&self) { - self.notify_vault_changed(); + /// + /// Unlike the automatic notifications after vault mutations (which are + /// best-effort), this method propagates errors so the host app can + /// detect and handle failures during the initial backup. + /// + /// # Errors + /// + /// 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<()> { + self.export_and_notify_backup() } } @@ -335,33 +344,38 @@ impl CredentialStore { /// propagated — the vault mutation has already succeeded and callers /// should not see an error from a backup side-effect. fn notify_vault_changed(&self) { + if let Err(e) = self.export_and_notify_backup() { + tracing::error!("Backup sync failed (best-effort): {e}"); + } + } + + /// Exports the vault and notifies the registered backup manager. + /// + /// This is the shared implementation used by both the best-effort + /// [`notify_vault_changed`](Self::notify_vault_changed) (which logs + /// and swallows errors) and [`sync_backup`](Self::sync_backup) (which + /// propagates them). + fn export_and_notify_backup(&self) -> StorageResult<()> { // Hold the backup lock for the entire export+callback path. This // serializes concurrent notifications so backups are delivered in - // mutation order. Recover the guard on poison — the manager is - // still valid after a prior panic. - let guard = self - .backup - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); + // mutation order. + let guard = self.backup.lock().map_err(|_| { + StorageError::Lock("backup config mutex poisoned".to_string()) + })?; let dest_dir = guard.dest_dir(); if dest_dir.is_empty() { - return; // NoopBackupManager — nothing to do. + return Err(StorageError::Keystore( + "no backup manager configured".to_string(), + )); } // Export a plaintext snapshot of the vault. The file is sensitive // (unencrypted), so we wrap it in a guard that deletes it on drop — // no matter how we exit (normal return, early return, or panic). - let vault_path = match self + let vault_path = self .lock_inner() - .and_then(|inner| inner.export_vault_for_backup(&dest_dir)) - { - Ok(path) => path, - Err(e) => { - tracing::error!("Failed to export vault for backup: {e}"); - return; - } - }; + .and_then(|inner| inner.export_vault_for_backup(&dest_dir))?; let _cleanup = { struct CleanupFile(String); @@ -381,9 +395,7 @@ impl CredentialStore { // Hand the path to the host app (e.g. iOS) so it can copy/upload // the vault to Bedrock. The host must finish with the file during // this synchronous call — the guard deletes it on return. - if let Err(e) = guard.on_vault_changed(vault_path) { - tracing::error!("Backup manager on_vault_changed failed: {e}"); - } + guard.on_vault_changed(vault_path) } /// Retrieves a full credential including raw bytes by issuer schema ID. @@ -1536,7 +1548,7 @@ mod tests { assert_eq!(manager.call_count(), 1); // sync_backup triggers a notification without mutating the vault. - store.sync_backup(); + store.sync_backup().expect("sync_backup"); assert_eq!(manager.call_count(), 2); assert!( manager.last_file_existed(), @@ -1562,7 +1574,7 @@ mod tests { let (store, manager, root, export_dir) = setup_store_with_backup(); // sync_backup on an empty vault should still trigger a notification. - store.sync_backup(); + store.sync_backup().expect("sync_backup"); assert_eq!(manager.call_count(), 1); assert!( manager.last_file_existed(), From ea81c5ef6b5fc8e6246a5bd092c1d06713bc9305 Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Thu, 12 Mar 2026 15:42:04 -0700 Subject: [PATCH 05/11] refactor: simplify, propagate errors up --- .../src/storage/credential_storage.rs | 42 +++++-------------- 1 file changed, 11 insertions(+), 31 deletions(-) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index 446ded713..23a9f2d20 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -190,8 +190,7 @@ impl CredentialStore { /// not exist. pub fn delete_credential(&self, credential_id: u64) -> StorageResult<()> { self.lock_inner()?.delete_credential(credential_id)?; - self.notify_vault_changed(); - Ok(()) + self.notify_vault_changed() } /// Stores a credential and optional associated data. @@ -214,7 +213,7 @@ impl CredentialStore { associated_data, now, )?; - self.notify_vault_changed(); + self.notify_vault_changed()?; Ok(id) } @@ -284,15 +283,15 @@ impl CredentialStore { let count = inner.danger_delete_all_credentials()?; drop(inner); if count > 0 { - self.notify_vault_changed(); + self.notify_vault_changed()?; } Ok(count) } /// Registers a backup manager that will be notified after vault mutations /// ([`store_credential`](Self::store_credential), + /// [`delete_credential`](Self::delete_credential), /// [`danger_delete_all_credentials`](Self::danger_delete_all_credentials)). - /// Backup failures are logged but do not affect the mutation result. /// /// # Errors /// @@ -314,16 +313,12 @@ impl CredentialStore { /// [`WalletKitBackupManager`] receives the same `on_vault_changed` /// callback as after a normal vault mutation. /// - /// Unlike the automatic notifications after vault mutations (which are - /// best-effort), this method propagates errors so the host app can - /// detect and handle failures during the initial backup. - /// /// # Errors /// /// 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<()> { - self.export_and_notify_backup() + self.notify_vault_changed() } } @@ -337,25 +332,12 @@ impl CredentialStore { .map_err(|_| StorageError::Lock("storage mutex poisoned".to_string())) } - /// Best-effort export + notification to the backup manager, if one is set. - /// - /// Called after any vault mutation (store, delete) so the host app can - /// sync the updated vault to its backup. Failures are logged but never - /// propagated — the vault mutation has already succeeded and callers - /// should not see an error from a backup side-effect. - fn notify_vault_changed(&self) { - if let Err(e) = self.export_and_notify_backup() { - tracing::error!("Backup sync failed (best-effort): {e}"); - } - } - - /// Exports the vault and notifies the registered backup manager. + /// Exports a plaintext vault snapshot and notifies the registered backup + /// manager via [`on_vault_changed`](WalletKitBackupManager::on_vault_changed). /// - /// This is the shared implementation used by both the best-effort - /// [`notify_vault_changed`](Self::notify_vault_changed) (which logs - /// and swallows errors) and [`sync_backup`](Self::sync_backup) (which - /// propagates them). - fn export_and_notify_backup(&self) -> StorageResult<()> { + /// Called after vault mutations and by [`sync_backup`](Self::sync_backup). + /// Returns `Ok(())` if no backup manager is configured (noop). + fn notify_vault_changed(&self) -> StorageResult<()> { // Hold the backup lock for the entire export+callback path. This // serializes concurrent notifications so backups are delivered in // mutation order. @@ -365,9 +347,7 @@ impl CredentialStore { let dest_dir = guard.dest_dir(); if dest_dir.is_empty() { - return Err(StorageError::Keystore( - "no backup manager configured".to_string(), - )); + return Ok(()); // NoopBackupManager — nothing to do. } // Export a plaintext snapshot of the vault. The file is sensitive From 4ee41f53f97a6f1a73061248a71180f0e9f8317e Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Thu, 12 Mar 2026 16:35:29 -0700 Subject: [PATCH 06/11] docs: clarify comments --- walletkit-core/src/storage/credential_storage.rs | 8 ++++++++ walletkit-core/src/storage/traits.rs | 14 ++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index 23a9f2d20..b3cf841a1 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -337,6 +337,14 @@ impl CredentialStore { /// /// Called after vault mutations and by [`sync_backup`](Self::sync_backup). /// Returns `Ok(())` if no backup manager is configured (noop). + /// + /// **Note:** errors from the backup callback are propagated to the caller. + /// Because this runs *after* the vault mutation has been committed, a + /// returned `Err` does not mean the mutation failed — only the backup + /// notification. Callers should inspect the error and handle it according + /// to its nature (e.g. log it, schedule a backup retry via + /// [`sync_backup`](Self::sync_backup), or surface it to the user) rather + /// than retrying the already-committed mutation. fn notify_vault_changed(&self) -> StorageResult<()> { // Hold the backup lock for the entire export+callback path. This // serializes concurrent notifications so backups are delivered in diff --git a/walletkit-core/src/storage/traits.rs b/walletkit-core/src/storage/traits.rs index a3e5bb075..d7550b21f 100644 --- a/walletkit-core/src/storage/traits.rs +++ b/walletkit-core/src/storage/traits.rs @@ -121,9 +121,15 @@ pub trait WalletKitBackupManager: Send + Sync { /// /// # Errors /// - /// Returning `Err` is treated as best-effort — the error is logged but - /// 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 + /// mutation (e.g. `store_credential`, `delete_credential`). Because the + /// vault mutation has already been committed by the time this callback + /// runs, callers should be aware that an `Err` result does **not** mean + /// the mutation failed — only that the backup notification failed. + /// Callers should inspect the returned error and handle it according to + /// its nature (e.g. log it, schedule a backup retry via + /// [`sync_backup`](crate::storage::CredentialStore::sync_backup), or + /// surface it to the user) rather than retrying the already-committed + /// mutation. fn on_vault_changed(&self, vault_file_path: String) -> StorageResult<()>; } From 0026a143931ca75802682ee8513dc14b284ff259 Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Mon, 16 Mar 2026 12:52:52 -0700 Subject: [PATCH 07/11] refactor: remove mutex poisoning overhead --- Cargo.lock | 1 + walletkit-core/Cargo.toml | 1 + walletkit-core/src/authenticator/mod.rs | 4 +- .../src/storage/credential_storage.rs | 78 +++++++------------ .../tests/authenticator_integration.rs | 2 +- .../tests/proof_generation_integration.rs | 2 +- 6 files changed, 36 insertions(+), 52 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 52e0bf1eb..fcc2cace0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6380,6 +6380,7 @@ dependencies = [ "hkdf", "log", "mockito", + "parking_lot", "rand 0.8.5", "regex", "reqwest 0.12.28", diff --git a/walletkit-core/Cargo.toml b/walletkit-core/Cargo.toml index 6d9600d0a..c076d7c9d 100644 --- a/walletkit-core/Cargo.toml +++ b/walletkit-core/Cargo.toml @@ -46,6 +46,7 @@ sha2 = { version = "0.10", optional = true } strum = { version = "0.27", features = ["derive"] } subtle = "2" thiserror = "2" +parking_lot = "0.12" tokio = { version = "1", features = ["sync"] } tracing = "0.1" tracing-log = "0.2" diff --git a/walletkit-core/src/authenticator/mod.rs b/walletkit-core/src/authenticator/mod.rs index 99c0c7fcf..1328c5f09 100644 --- a/walletkit-core/src/authenticator/mod.rs +++ b/walletkit-core/src/authenticator/mod.rs @@ -566,10 +566,10 @@ mod tests { let provider = InMemoryStorageProvider::new(&root); let store = CredentialStore::from_provider(&provider).expect("store"); store.init(42, 100).expect("init storage"); - cache_embedded_groth16_material(store.storage_paths().expect("paths")) + cache_embedded_groth16_material(store.storage_paths()) .expect("cache material"); - let paths = store.storage_paths().expect("paths"); + let paths = store.storage_paths(); Authenticator::init(&seed, &config, paths, Arc::new(store)) .await .unwrap(); diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index b3cf841a1..f135bc3e1 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -1,6 +1,8 @@ //! Storage facade implementing the credential storage API. -use std::sync::{Arc, Mutex}; +use std::sync::Arc; + +use parking_lot::Mutex; use world_id_core::FieldElement as CoreFieldElement; @@ -148,12 +150,8 @@ impl CredentialStore { } /// Returns the storage paths used by this handle. - /// - /// # Errors - /// - /// Returns an error if the storage mutex is poisoned. - pub fn storage_paths(&self) -> StorageResult> { - self.lock_inner().map(|inner| Arc::new(inner.paths.clone())) + pub fn storage_paths(&self) -> Arc { + Arc::new(self.lock_inner().paths.clone()) } /// Initializes storage and validates the account leaf index. @@ -162,7 +160,7 @@ impl CredentialStore { /// /// Returns an error if initialization fails or the leaf index mismatches. pub fn init(&self, leaf_index: u64, now: u64) -> StorageResult<()> { - let mut inner = self.lock_inner()?; + let mut inner = self.lock_inner(); inner.init(leaf_index, now) } @@ -179,7 +177,7 @@ impl CredentialStore { issuer_schema_id: Option, now: u64, ) -> StorageResult> { - self.lock_inner()?.list_credentials(issuer_schema_id, now) + self.lock_inner().list_credentials(issuer_schema_id, now) } /// Deletes a credential by ID. @@ -189,7 +187,7 @@ impl CredentialStore { /// Returns an error if the delete operation fails or the credential ID does /// not exist. pub fn delete_credential(&self, credential_id: u64) -> StorageResult<()> { - self.lock_inner()?.delete_credential(credential_id)?; + self.lock_inner().delete_credential(credential_id)?; self.notify_vault_changed() } @@ -206,7 +204,7 @@ impl CredentialStore { associated_data: Option>, now: u64, ) -> StorageResult { - let id = self.lock_inner()?.store_credential( + let id = self.lock_inner().store_credential( credential, blinding_factor, expires_at, @@ -223,7 +221,7 @@ impl CredentialStore { /// /// Returns an error if the cache lookup fails. pub fn merkle_cache_get(&self, valid_until: u64) -> StorageResult>> { - self.lock_inner()?.merkle_cache_get(valid_until) + self.lock_inner().merkle_cache_get(valid_until) } /// Inserts a cached Merkle proof with a TTL. @@ -237,7 +235,7 @@ impl CredentialStore { now: u64, ttl_seconds: u64, ) -> StorageResult<()> { - self.lock_inner()? + self.lock_inner() .merkle_cache_put(proof_bytes, now, ttl_seconds) } @@ -259,7 +257,7 @@ impl CredentialStore { reason = "non-owned strings cannot be lifted via UniFFI" )] pub fn import_vault_from_backup(&self, backup_path: String) -> StorageResult<()> { - self.lock_inner()?.import_vault_from_backup(&backup_path) + self.lock_inner().import_vault_from_backup(&backup_path) } /// **Development only.** Permanently deletes all stored credentials and their @@ -279,7 +277,7 @@ impl CredentialStore { /// /// Returns an error if the delete operation fails. pub fn danger_delete_all_credentials(&self) -> StorageResult { - let mut inner = self.lock_inner()?; + let mut inner = self.lock_inner(); let count = inner.danger_delete_all_credentials()?; drop(inner); if count > 0 { @@ -293,17 +291,11 @@ impl CredentialStore { /// [`delete_credential`](Self::delete_credential), /// [`danger_delete_all_credentials`](Self::danger_delete_all_credentials)). /// - /// # Errors - /// - /// Returns an error if the backup mutex is poisoned. pub fn set_backup_manager( &self, manager: Arc, - ) -> StorageResult<()> { - *self.backup.lock().map_err(|_| { - StorageError::Lock("backup config mutex poisoned".to_string()) - })? = manager; - Ok(()) + ) { + *self.backup.lock() = manager; } /// Triggers a one-off backup sync without mutating the vault. @@ -326,10 +318,8 @@ impl CredentialStore { impl CredentialStore { fn lock_inner( &self, - ) -> StorageResult> { - self.inner - .lock() - .map_err(|_| StorageError::Lock("storage mutex poisoned".to_string())) + ) -> parking_lot::MutexGuard<'_, CredentialStoreInner> { + self.inner.lock() } /// Exports a plaintext vault snapshot and notifies the registered backup @@ -349,9 +339,7 @@ impl CredentialStore { // Hold the backup lock for the entire export+callback path. This // serializes concurrent notifications so backups are delivered in // mutation order. - let guard = self.backup.lock().map_err(|_| { - StorageError::Lock("backup config mutex poisoned".to_string()) - })?; + let guard = self.backup.lock(); let dest_dir = guard.dest_dir(); if dest_dir.is_empty() { @@ -363,7 +351,7 @@ impl CredentialStore { // no matter how we exit (normal return, early return, or panic). let vault_path = self .lock_inner() - .and_then(|inner| inner.export_vault_for_backup(&dest_dir))?; + .export_vault_for_backup(&dest_dir)?; let _cleanup = { struct CleanupFile(String); @@ -380,7 +368,7 @@ impl CredentialStore { CleanupFile(vault_path.clone()) }; - // Hand the path to the host app (e.g. iOS) so it can copy/upload + // Hand the path to the hostI' app (e.g. iOS) so it can copy/upload // the vault to Bedrock. The host must finish with the file during // this synchronous call — the guard deletes it on return. guard.on_vault_changed(vault_path) @@ -398,7 +386,7 @@ impl CredentialStore { issuer_schema_id: u64, now: u64, ) -> StorageResult> { - self.lock_inner()?.get_credential(issuer_schema_id, now) + self.lock_inner().get_credential(issuer_schema_id, now) } /// Checks whether a replay guard entry exists for the given nullifier. @@ -414,7 +402,7 @@ impl CredentialStore { nullifier: CoreFieldElement, now: u64, ) -> StorageResult { - self.lock_inner()?.is_nullifier_replay(nullifier, now) + self.lock_inner().is_nullifier_replay(nullifier, now) } /// After a proof has been successfully generated, creates a replay guard entry @@ -428,7 +416,7 @@ impl CredentialStore { nullifier: CoreFieldElement, now: u64, ) -> StorageResult<()> { - self.lock_inner()?.replay_guard_set(nullifier, now) + self.lock_inner().replay_guard_set(nullifier, now) } } @@ -655,12 +643,8 @@ impl CredentialStore { } /// Returns the storage paths used by this handle. - /// - /// # Errors - /// - /// Returns an error if the storage mutex is poisoned. - pub fn paths(&self) -> StorageResult { - self.lock_inner().map(|inner| inner.paths.clone()) + pub fn paths(&self) -> StoragePaths { + self.lock_inner().paths.clone() } } @@ -1339,15 +1323,15 @@ mod tests { } fn call_count(&self) -> usize { - self.calls.lock().unwrap().len() + self.calls.lock().len() } fn last_path(&self) -> Option { - self.calls.lock().unwrap().last().map(|(p, _)| p.clone()) + self.calls.lock().last().map(|(p, _): &(String, bool)| p.clone()) } fn last_file_existed(&self) -> bool { - self.calls.lock().unwrap().last().is_some_and(|(_, e)| *e) + self.calls.lock().last().is_some_and(|(_, e): &(String, bool)| *e) } } @@ -1358,7 +1342,7 @@ mod tests { fn on_vault_changed(&self, vault_file_path: String) -> StorageResult<()> { let existed = std::path::Path::new(&vault_file_path).exists(); - self.calls.lock().unwrap().push((vault_file_path, existed)); + self.calls.lock().push((vault_file_path, existed)); Ok(()) } } @@ -1376,9 +1360,7 @@ mod tests { std::fs::create_dir_all(&export_dir).expect("create export dir"); let manager = MockBackupManager::new(export_dir.to_string_lossy().to_string()); - store - .set_backup_manager(manager.clone()) - .expect("set backup manager"); + store.set_backup_manager(manager.clone()); (store, manager, root, export_dir) } diff --git a/walletkit-core/tests/authenticator_integration.rs b/walletkit-core/tests/authenticator_integration.rs index 3aa91470c..e1cafb558 100644 --- a/walletkit-core/tests/authenticator_integration.rs +++ b/walletkit-core/tests/authenticator_integration.rs @@ -36,7 +36,7 @@ async fn test_authenticator_integration() { let authenticator_seeder = PrivateKeySigner::random(); let store = common::create_test_credential_store(); - let paths = store.storage_paths().unwrap(); + let paths = store.storage_paths(); cache_embedded_groth16_material(paths.clone()).expect("cache groth16 material"); // When account doesn't exist, this should fail diff --git a/walletkit-core/tests/proof_generation_integration.rs b/walletkit-core/tests/proof_generation_integration.rs index 94c5c6b5c..ab32ea911 100644 --- a/walletkit-core/tests/proof_generation_integration.rs +++ b/walletkit-core/tests/proof_generation_integration.rs @@ -148,7 +148,7 @@ async fn e2e_authenticator_generate_proof() -> Result<()> { // Phase 2: Authenticator init with walletkit wrapper // ---------------------------------------------------------------- let store = common::create_test_credential_store(); - let paths = store.storage_paths().wrap_err("storage_paths failed")?; + let paths = store.storage_paths(); cache_embedded_groth16_material(paths.clone()) .wrap_err("cache_embedded_groth16_material failed")?; From c3ac156bbb2f4e8e2f66e593e9dd6f3658a69788 Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Mon, 16 Mar 2026 16:33:03 -0700 Subject: [PATCH 08/11] feat: only log on backup errors --- .../src/storage/credential_storage.rs | 53 ++++++++++--------- walletkit-core/src/storage/traits.rs | 17 +++--- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index f135bc3e1..63fc30648 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -188,7 +188,8 @@ impl CredentialStore { /// not exist. pub fn delete_credential(&self, credential_id: u64) -> StorageResult<()> { self.lock_inner().delete_credential(credential_id)?; - self.notify_vault_changed() + self.notify_vault_changed(); + Ok(()) } /// Stores a credential and optional associated data. @@ -211,7 +212,7 @@ impl CredentialStore { associated_data, now, )?; - self.notify_vault_changed()?; + self.notify_vault_changed(); Ok(id) } @@ -281,7 +282,7 @@ impl CredentialStore { let count = inner.danger_delete_all_credentials()?; drop(inner); if count > 0 { - self.notify_vault_changed()?; + self.notify_vault_changed(); } Ok(count) } @@ -305,12 +306,11 @@ impl CredentialStore { /// [`WalletKitBackupManager`] receives the same `on_vault_changed` /// callback as after a normal vault mutation. /// - /// # Errors - /// - /// 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<()> { - self.notify_vault_changed() + /// Errors from the vault export or backup callback are logged but not + /// propagated — the caller can schedule a retry via another + /// `sync_backup` call if needed. + pub fn sync_backup(&self) { + self.notify_vault_changed(); } } @@ -328,14 +328,11 @@ impl CredentialStore { /// Called after vault mutations and by [`sync_backup`](Self::sync_backup). /// Returns `Ok(())` if no backup manager is configured (noop). /// - /// **Note:** errors from the backup callback are propagated to the caller. - /// Because this runs *after* the vault mutation has been committed, a - /// returned `Err` does not mean the mutation failed — only the backup - /// notification. Callers should inspect the error and handle it according - /// to its nature (e.g. log it, schedule a backup retry via - /// [`sync_backup`](Self::sync_backup), or surface it to the user) rather - /// than retrying the already-committed mutation. - fn notify_vault_changed(&self) -> StorageResult<()> { + /// Backup callback errors are logged but **not** propagated, because the + /// vault mutation has already been committed by the time this runs. + /// Propagating would cause callers to see a failed result for a mutation + /// that actually succeeded, leading to incorrect retries. + fn notify_vault_changed(&self) { // Hold the backup lock for the entire export+callback path. This // serializes concurrent notifications so backups are delivered in // mutation order. @@ -343,15 +340,19 @@ impl CredentialStore { let dest_dir = guard.dest_dir(); if dest_dir.is_empty() { - return Ok(()); // NoopBackupManager — nothing to do. + return; // NoopBackupManager — nothing to do. } // Export a plaintext snapshot of the vault. The file is sensitive // (unencrypted), so we wrap it in a guard that deletes it on drop — // no matter how we exit (normal return, early return, or panic). - let vault_path = self - .lock_inner() - .export_vault_for_backup(&dest_dir)?; + let vault_path = match self.lock_inner().export_vault_for_backup(&dest_dir) { + Ok(path) => path, + Err(e) => { + tracing::error!("Vault export for backup failed: {e}"); + return; + } + }; let _cleanup = { struct CleanupFile(String); @@ -368,10 +369,12 @@ impl CredentialStore { CleanupFile(vault_path.clone()) }; - // Hand the path to the hostI' app (e.g. iOS) so it can copy/upload + // Hand the path to the host app (e.g. iOS) so it can copy/upload // the vault to Bedrock. The host must finish with the file during // this synchronous call — the guard deletes it on return. - guard.on_vault_changed(vault_path) + if let Err(e) = guard.on_vault_changed(vault_path) { + tracing::error!("Backup callback failed (vault mutation already committed): {e}"); + } } /// Retrieves a full credential including raw bytes by issuer schema ID. @@ -1518,7 +1521,7 @@ mod tests { assert_eq!(manager.call_count(), 1); // sync_backup triggers a notification without mutating the vault. - store.sync_backup().expect("sync_backup"); + store.sync_backup(); assert_eq!(manager.call_count(), 2); assert!( manager.last_file_existed(), @@ -1544,7 +1547,7 @@ mod tests { let (store, manager, root, export_dir) = setup_store_with_backup(); // sync_backup on an empty vault should still trigger a notification. - store.sync_backup().expect("sync_backup"); + store.sync_backup(); assert_eq!(manager.call_count(), 1); assert!( manager.last_file_existed(), diff --git a/walletkit-core/src/storage/traits.rs b/walletkit-core/src/storage/traits.rs index d7550b21f..938a2e90d 100644 --- a/walletkit-core/src/storage/traits.rs +++ b/walletkit-core/src/storage/traits.rs @@ -121,15 +121,12 @@ pub trait WalletKitBackupManager: Send + Sync { /// /// # Errors /// - /// Returning `Err` propagates the error to the caller of the vault - /// mutation (e.g. `store_credential`, `delete_credential`). Because the - /// vault mutation has already been committed by the time this callback - /// runs, callers should be aware that an `Err` result does **not** mean - /// the mutation failed — only that the backup notification failed. - /// Callers should inspect the returned error and handle it according to - /// its nature (e.g. log it, schedule a backup retry via - /// [`sync_backup`](crate::storage::CredentialStore::sync_backup), or - /// surface it to the user) rather than retrying the already-committed - /// mutation. + /// Errors returned here are **logged but not propagated** to the caller + /// of the vault mutation. The vault mutation has already been committed + /// by the time this callback runs, so a failure here only means the + /// backup sync failed — the credential data is safe. The host app can + /// schedule a retry via + /// [`sync_backup`](crate::storage::CredentialStore::sync_backup) if + /// needed. fn on_vault_changed(&self, vault_file_path: String) -> StorageResult<()>; } From 65a53ebbe634d30f67d6c57f32f992809cb3d8ba Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Mon, 16 Mar 2026 16:34:42 -0700 Subject: [PATCH 09/11] style: clippy fixes --- walletkit-core/src/authenticator/mod.rs | 3 +-- .../src/storage/credential_storage.rs | 23 +++++++++++-------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/walletkit-core/src/authenticator/mod.rs b/walletkit-core/src/authenticator/mod.rs index 1328c5f09..362850919 100644 --- a/walletkit-core/src/authenticator/mod.rs +++ b/walletkit-core/src/authenticator/mod.rs @@ -566,8 +566,7 @@ mod tests { let provider = InMemoryStorageProvider::new(&root); let store = CredentialStore::from_provider(&provider).expect("store"); store.init(42, 100).expect("init storage"); - cache_embedded_groth16_material(store.storage_paths()) - .expect("cache material"); + cache_embedded_groth16_material(store.storage_paths()).expect("cache material"); let paths = store.storage_paths(); Authenticator::init(&seed, &config, paths, Arc::new(store)) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index 63fc30648..246b993a8 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -292,10 +292,7 @@ impl CredentialStore { /// [`delete_credential`](Self::delete_credential), /// [`danger_delete_all_credentials`](Self::danger_delete_all_credentials)). /// - pub fn set_backup_manager( - &self, - manager: Arc, - ) { + pub fn set_backup_manager(&self, manager: Arc) { *self.backup.lock() = manager; } @@ -316,9 +313,7 @@ impl CredentialStore { /// Implementation not exposed to foreign bindings impl CredentialStore { - fn lock_inner( - &self, - ) -> parking_lot::MutexGuard<'_, CredentialStoreInner> { + fn lock_inner(&self) -> parking_lot::MutexGuard<'_, CredentialStoreInner> { self.inner.lock() } @@ -373,7 +368,9 @@ impl CredentialStore { // the vault to Bedrock. The host must finish with the file during // this synchronous call — the guard deletes it on return. if let Err(e) = guard.on_vault_changed(vault_path) { - tracing::error!("Backup callback failed (vault mutation already committed): {e}"); + tracing::error!( + "Backup callback failed (vault mutation already committed): {e}" + ); } } @@ -1330,11 +1327,17 @@ mod tests { } fn last_path(&self) -> Option { - self.calls.lock().last().map(|(p, _): &(String, bool)| p.clone()) + self.calls + .lock() + .last() + .map(|(p, _): &(String, bool)| p.clone()) } fn last_file_existed(&self) -> bool { - self.calls.lock().last().is_some_and(|(_, e): &(String, bool)| *e) + self.calls + .lock() + .last() + .is_some_and(|(_, e): &(String, bool)| *e) } } From 5d93559d08200b1067f1d4d149202a94a1084061 Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Mon, 16 Mar 2026 16:48:51 -0700 Subject: [PATCH 10/11] style: fix clippy errors --- walletkit-core/src/storage/credential_storage.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/walletkit-core/src/storage/credential_storage.rs b/walletkit-core/src/storage/credential_storage.rs index 246b993a8..507f3f22f 100644 --- a/walletkit-core/src/storage/credential_storage.rs +++ b/walletkit-core/src/storage/credential_storage.rs @@ -341,7 +341,8 @@ impl CredentialStore { // Export a plaintext snapshot of the vault. The file is sensitive // (unencrypted), so we wrap it in a guard that deletes it on drop — // no matter how we exit (normal return, early return, or panic). - let vault_path = match self.lock_inner().export_vault_for_backup(&dest_dir) { + let export_result = self.lock_inner().export_vault_for_backup(&dest_dir); + let vault_path = match export_result { Ok(path) => path, Err(e) => { tracing::error!("Vault export for backup failed: {e}"); From 7b86e91070ed2ca2ecfdac1cce1b136b97ae31c5 Mon Sep 17 00:00:00 2001 From: Tom Waite Date: Mon, 16 Mar 2026 16:59:38 -0700 Subject: [PATCH 11/11] deps: add parking lot under storage feature --- walletkit-core/Cargo.toml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/walletkit-core/Cargo.toml b/walletkit-core/Cargo.toml index c076d7c9d..f971664fd 100644 --- a/walletkit-core/Cargo.toml +++ b/walletkit-core/Cargo.toml @@ -46,7 +46,7 @@ sha2 = { version = "0.10", optional = true } 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" @@ -98,6 +98,7 @@ storage = [ "dep:rand", "dep:sha2", "dep:uuid", + "dep:parking_lot", "dep:walletkit-db", ]