From ff25eb68e7ab67b0eab102f6324c997a561f0c64 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:37:32 +0000 Subject: [PATCH 1/9] fix(platform-wallet): typed persister errors with bounded transient retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Persistence failures on the wallet rehydration and registration paths were flattened into `PlatformWalletError::WalletCreation(String)`, destroying the transient/fatal classification callers need and severing the `#[source]` chain. Adds typed `PersisterLoad` / `PersisterStore` / `PersisterRestore` variants carrying the `PersistenceError` (boxed for the recursive restore case) and routes every persister boundary through them. On top of that, `retry_transient` (4 attempts, 20 -> 200 ms doubling backoff) now wraps persister `store` / `flush` / `load` on the registration, startup and identity-discovery paths, so a transient `SQLITE_BUSY` no longer aborts wallet registration outright or costs the identity-scan verdict its durability (#4365). Fatal errors still fail fast. The retry re-drives a failed `store` via a bare `flush`, which `PlatformWalletPersistence::store` now documents as a backend contract. Also fixes the persister leak behind #4133: a failed `load_from_persistor` left the wallet-event adapter holding an `Arc

` clone, so re-opening the same path returned a spurious `AlreadyOpen` masking the real error. `load_from_persistor` now shuts the manager down on both failure paths, with a `Drop` backstop cancelling and aborting the adapter task. `record_or_persister_or_log` and `reconcile_sent_payments` stop swallowing permanent read failures as "not found": transient errors still defer to the next sweep, permanent ones propagate as `PersisterLoad` instead of stalling an unbounded poll loop with no explanation. 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- .../src/changeset/traits.rs | 13 + packages/rs-platform-wallet/src/error.rs | 34 ++ .../rs-platform-wallet/src/manager/load.rs | 214 ++++++- .../rs-platform-wallet/src/manager/mod.rs | 32 ++ .../rs-platform-wallet/src/manager/startup.rs | 26 +- .../src/manager/wallet_lifecycle.rs | 523 +++++++++++++++++- .../src/wallet/asset_lock/sync/proof.rs | 79 ++- .../src/wallet/identity/network/discovery.rs | 30 +- .../src/wallet/identity/network/payments.rs | 43 +- 9 files changed, 926 insertions(+), 68 deletions(-) diff --git a/packages/rs-platform-wallet/src/changeset/traits.rs b/packages/rs-platform-wallet/src/changeset/traits.rs index 60d98195ba9..16e8ac8d217 100644 --- a/packages/rs-platform-wallet/src/changeset/traits.rs +++ b/packages/rs-platform-wallet/src/changeset/traits.rs @@ -266,6 +266,19 @@ pub trait PlatformWalletPersistence: Send + Sync { /// wallet accessor (readers and writers) for its duration. Keep the /// per-call work bounded; if the backend does inline I/O (see the type /// doc), size it accordingly. + /// + /// # Transient-failure retry contract + /// + /// An implementation that returns a [`PersistenceError`] classified + /// [`PersistenceErrorKind::Transient`] from `store` **MUST** have already + /// buffered/preserved the changeset so that a subsequent bare + /// [`flush`](Self::flush) — with no re-supplied changeset — completes the + /// write (mirroring `flush`'s own transient contract). This is what lets a + /// caller retry a transient `store` failure via `flush` alone; re-calling + /// `store` with the same changeset would double-merge it. An + /// implementation that cannot preserve the changeset on failure MUST + /// classify that failure [`PersistenceErrorKind::Fatal`] (or + /// [`Constraint`](PersistenceErrorKind::Constraint)), never `Transient`. fn store( &self, wallet_id: WalletId, diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index d24412b3007..a4c24264a2d 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -14,6 +14,40 @@ pub enum PlatformWalletError { #[error("Wallet creation failed: {0}")] WalletCreation(String), + /// The persister failed to load the client start state during + /// rehydration. Carries the typed [`PersistenceError`] so callers keep + /// its retry classification (`is_transient()` / + /// [`PersistenceErrorKind`]) instead of a flattened string — a + /// transient backend hiccup (e.g. `SQLITE_BUSY`) stays distinguishable + /// from a permanent failure and can be retried. + /// + /// [`PersistenceError`]: crate::changeset::PersistenceError + /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind + #[error("failed to load persisted client state: {0}")] + PersisterLoad(#[from] crate::changeset::PersistenceError), + + /// The persister failed to store the wallet-registration changeset. + /// Like [`Self::PersisterLoad`], it carries the typed + /// [`PersistenceError`] so the retry classification (`is_transient()` + /// / [`PersistenceErrorKind`]) survives the boundary — a transient + /// `SQLITE_BUSY` stays distinguishable from a permanent failure. + /// Distinct from [`Self::PersisterLoad`] so callers can tell a failed + /// registration write from a failed rehydration read; not `#[from]` + /// because that conversion is already claimed by [`Self::PersisterLoad`]. + /// + /// [`PersistenceError`]: crate::changeset::PersistenceError + /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind + #[error("failed to persist wallet registration changeset: {0}")] + PersisterStore(#[source] crate::changeset::PersistenceError), + + /// Restoring the persisted platform-address state into the freshly + /// registered wallet failed. Wraps the underlying + /// [`PlatformWalletError`](Self) (boxed to break the recursive type) so + /// its concrete variant and `#[source]` chain survive instead of being + /// flattened into a string. + #[error("failed to restore persisted platform-address state: {0}")] + PersisterRestore(#[source] Box), + #[error("Wallet not found: {0}")] WalletNotFound(String), diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index 3588aef2b54..ed65cdd0283 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -10,7 +10,7 @@ use crate::wallet::identity::IdentityManager; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use crate::wallet::PlatformWallet; -use super::PlatformWalletManager; +use super::{wallet_lifecycle::retry_transient, PlatformWalletManager}; impl PlatformWalletManager

{ /// Load the full [`ClientStartState`] from the configured persister @@ -30,6 +30,22 @@ impl PlatformWalletManager

{ /// /// [`WalletManager`]: key_wallet_manager::WalletManager pub async fn load_from_persistor(&self) -> Result<(), PlatformWalletError> { + let start_state = match retry_transient(|| self.persister.load()).await { + Ok(state) => state, + Err(e) => { + // Preserve the typed source chain (Debug carries the real + // cause — e.g. a bincode decode failure) instead of flattening + // it to a Display string, and release the wallet-event adapter + // so a reconstruct on the same path doesn't hit `AlreadyOpen` + // masking this error. + tracing::debug!(error = ?e, "persister load failed during rehydration"); + let report = self.shutdown().await; + if !report.all_clean() { + tracing::warn!(?report, "wallet workers unclean after aborting rehydration"); + } + return Err(PlatformWalletError::PersisterLoad(e)); + } + }; let ClientStartState { mut platform_addresses, wallets, @@ -37,12 +53,7 @@ impl PlatformWalletManager

{ // not here — drop the snapshot at this entry point. #[cfg(feature = "shielded")] shielded: _, - } = self.persister.load().map_err(|e| { - PlatformWalletError::WalletCreation(format!( - "Failed to load persisted client state: {}", - e - )) - })?; + } = start_state; // Tracked (wallet-independent) masternodes ride the same startup // hydration; a failure logs and starts empty rather than failing @@ -237,6 +248,16 @@ impl PlatformWalletManager

{ } } } + // Release the wallet-event adapter so a reconstruct on the same + // persister path doesn't hit `AlreadyOpen` (see the early-return + // path above). + let report = self.shutdown().await; + if !report.all_clean() { + tracing::warn!( + ?report, + "wallet workers left unclean after rolling back a failed rehydration" + ); + } return Err(err); } @@ -363,3 +384,182 @@ mod idempotent_load_tests { ); } } + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + use super::*; + use crate::changeset::{PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet}; + use crate::events::{EventHandler, PlatformEventHandler}; + + /// Persister whose `load()` always fails — the failure path under test. + struct FailingLoadPersister; + + impl PlatformWalletPersistence for FailingLoadPersister { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + + fn load(&self) -> Result { + Err(PersistenceError::backend("simulated load failure")) + } + } + + struct TransientOnceLoadPersister { + load_calls: AtomicUsize, + } + + impl PlatformWalletPersistence for TransientOnceLoadPersister { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + + fn load(&self) -> Result { + if self.load_calls.fetch_add(1, Ordering::SeqCst) == 0 { + return Err(PersistenceError::backend_with_kind( + PersistenceErrorKind::Transient, + "simulated transient load failure", + )); + } + Ok(ClientStartState::default()) + } + } + + struct NoopEventHandler; + impl EventHandler for NoopEventHandler {} + impl PlatformEventHandler for NoopEventHandler {} + + #[tokio::test] + async fn transient_load_failure_during_startup_rehydration_is_retried() { + let persister = Arc::new(TransientOnceLoadPersister { + load_calls: AtomicUsize::new(0), + }); + let probe = Arc::clone(&persister); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let handler: Arc = Arc::new(NoopEventHandler); + let manager = PlatformWalletManager::new(sdk, persister, handler); + + manager + .load_from_persistor() + .await + .expect("transient startup load failure must be retried"); + + assert_eq!(probe.load_calls.load(Ordering::SeqCst), 2); + } + + /// A failed `load_from_persistor` must (a) surface the typed `PersisterLoad` + /// error preserving the source chain, and (b) release the wallet-event + /// adapter's `Arc` clone so a reconstruct on the same path + /// doesn't hit `WalletStorageError::AlreadyOpen` masking the real error + /// (issue #4133). + /// + /// This is a **manager-side proxy**, not a full end-to-end proof: it asserts + /// the persister's strong count returns to 1 (the test's own probe) after a + /// failed load + teardown — a lingering adapter clone would keep it above 1 + /// — which is the necessary precondition for a clean re-open. It does not + /// itself open a real `SqlitePersister`, fail, and re-open on the same path; + /// the platform-wallet ⇄ platform-wallet-storage dev-dependency cycle + /// precludes using the concrete persister here. That end-to-end + /// open → fail → reopen is covered by the storage crate's own round-trip + /// coverage test. + // Multi-thread: dropping the manager runs upstream's `Drop`, whose + // `ThreadRegistry::shutdown()` asserts a multi-thread runtime. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn failed_load_releases_persister_for_reconstruct() { + let persister = Arc::new(FailingLoadPersister); + let probe = Arc::clone(&persister); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let handler: Arc = Arc::new(NoopEventHandler); + + let manager = PlatformWalletManager::new(sdk, persister, handler); + + let err = manager + .load_from_persistor() + .await + .expect_err("load must fail"); + assert!( + matches!(err, PlatformWalletError::PersisterLoad(_)), + "load failure must surface as the typed PersisterLoad variant, got {err:?}" + ); + + drop(manager); + // Asserted directly, never polled: the failure path awaits the + // adapter's `JoinHandle` inside `shutdown`, so the task's clone is + // already released before `drop` runs. Release on THIS path is + // synchronous, which is the stronger guarantee — a poll loop (or a + // `yield_now`, which cedes nothing to another worker) would only + // hide a regression into eventual release. + assert_eq!( + Arc::strong_count(&probe), + 1, + "after a failed load + teardown nothing may still hold the persister" + ); + } + + /// The `Drop` backstop alone (no `shutdown` first) must *eventually* release + /// the adapter's `Arc` clone. Unlike the graceful path this is + /// not synchronous: `Drop::drop` calls `abort()`, which only *requests* + /// cancellation — the runtime drops the aborted task (and its clone) at its + /// next poll. So the strong count is polled, not asserted immediately, which + /// is exactly the "eventual, not synchronous" contract the `Drop` impl's + /// doc-comment describes. This is the branch the graceful-path test above + /// never exercises (there `shutdown` has already taken the join handle, so + /// `Drop`'s `abort` sees `None`). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn drop_backstop_eventually_releases_persister_without_shutdown() { + let persister = Arc::new(FailingLoadPersister); + let probe = Arc::clone(&persister); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let handler: Arc = Arc::new(NoopEventHandler); + + let manager = PlatformWalletManager::new(sdk, persister, handler); + // The adapter task spawned in `new()` holds a clone, so the count is + // above the probe before any teardown. + assert!( + Arc::strong_count(&probe) > 1, + "the spawned adapter task must hold an Arc clone" + ); + + // Dirty drop: never call `shutdown`, so `Drop`'s `abort` is the only + // thing that can reclaim the adapter's clone. + drop(manager); + + // Release is eventual: poll until the aborted task is dropped by the + // runtime rather than asserting immediately. The wait must be a timed + // sleep, not `yield_now`: the aborted task is reclaimed by whichever + // worker thread owns it, and yielding this thread never forces that + // one to run — the whole budget can burn in microseconds while the + // clone is still live. Breaks on the first observation, so the 2s + // ceiling is only ever paid by a genuine regression. + let mut count = Arc::strong_count(&probe); + for _ in 0..2_000 { + if count == 1 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(1)).await; + count = Arc::strong_count(&probe); + } + assert_eq!( + count, 1, + "the Drop backstop must eventually release the persister after aborting the adapter" + ); + } +} diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index 9192dfae148..cc484031aca 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -11,6 +11,11 @@ pub mod shielded_sync; pub mod startup; mod wallet_lifecycle; +/// Re-exported so the identity-scan verdict publishers under `wallet::` +/// retry on the same policy the registration path uses. The module itself +/// stays private — this is the only item it owes the rest of the crate. +pub(crate) use wallet_lifecycle::retry_transient; + use std::sync::Arc; use std::time::Duration; @@ -1038,6 +1043,33 @@ impl PlatformWalletManager

{ } } +/// Drop backstop for the wallet-event adapter task. +/// +/// The graceful teardown is [`shutdown`](PlatformWalletManager::shutdown) +/// (cancel + await the join). A dirty drop that skips it would otherwise merely +/// detach the `JoinHandle`, leaving the adapter task running and holding its +/// `Arc

` clone — which keeps the persister "open" and turns a later re-open +/// on the same path into a spurious `WalletStorageError::AlreadyOpen` that +/// masks the real error (issue #4133). Cancelling the token and aborting the +/// task here starts that release — but note it is *eventual*, not synchronous: +/// `abort()` only requests cancellation, so the runtime drops the task (and its +/// `Arc

` clone) at the task's next poll, not inside this `drop`. In practice +/// the adapter loop parks on an `.await` almost every iteration, so the clone is +/// reclaimed promptly. Only the graceful +/// [`shutdown`](PlatformWalletManager::shutdown) path *guarantees* the reference +/// is gone before it returns (it awaits the join); this backstop guarantees +/// eventual reclamation, not synchronous. +impl Drop for PlatformWalletManager

{ + fn drop(&mut self) { + self.event_adapter_cancel.cancel(); + // `get_mut` needs no runtime (we hold `&mut self`); `abort` is + // non-blocking. `None` when `shutdown` already took the handle. + if let Some(handle) = self.event_adapter_join.get_mut().take() { + handle.abort(); + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 168512f4d40..187d269d433 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -969,8 +969,10 @@ impl PlatformWalletManager /// Record that a scan was abandoned before it could answer every index. /// - /// Mirrors what `discover` publishes for itself; needed separately because - /// a scan dropped mid-await never reaches its own bookkeeping. + /// Mirrors what `discover` publishes for itself, retry policy included; + /// needed separately because a scan dropped mid-await never reaches its own + /// bookkeeping. This is the verdict least affordable to lose — it is the + /// one that re-opens the identity question on the next launch. async fn record_identity_scan_cut_off(&self, wallet_id: &WalletId) { // Coverage of nothing: the scan was dropped mid-await, so it answered // no index and may not clear one an earlier scan left open. @@ -988,12 +990,24 @@ impl PlatformWalletManager identity_scan_state: Some(recorded), ..Default::default() }; - if let Err(e) = self.persister.store(*wallet_id, changeset) { - tracing::warn!( + // Transient failures are ridden out on the registration path's bounded + // policy; the buffer preserves the changeset, so the retries re-drive + // it through `flush`. The final outcome is still swallowed — an + // abandoned scan must not turn a shutdown into an error. + let mut changeset_slot = Some(changeset); + let outcome = crate::manager::retry_transient(|| match changeset_slot.take() { + Some(cs) => self.persister.store(*wallet_id, cs), + None => self.persister.flush(*wallet_id), + }) + .await; + if let Err(e) = outcome { + tracing::error!( wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), error = %e, - "failed to persist an abandoned scan's verdict; the next launch may take the \ - warm shortcut over an incomplete identity set" + "abandoned scan's verdict could not be persisted after retries; the next \ + launch will take the warm shortcut over an identity set nothing proved \ + complete" ); } } diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 71bef57d723..4cc2dce78da 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -12,7 +12,7 @@ use key_wallet::Network; #[cfg(any(feature = "bls", feature = "eddsa"))] use crate::changeset::ProviderKeyExtendedPubKey; use crate::changeset::{ - AccountAddressPoolEntry, AccountRegistrationEntry, PlatformWalletChangeSet, + AccountAddressPoolEntry, AccountRegistrationEntry, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, ProviderKeyAccountEntry, WalletMetadataEntry, }; use crate::error::PlatformWalletError; @@ -51,6 +51,56 @@ fn parse_mnemonic_any_language(phrase: &str) -> Result { Err("phrase does not match any supported BIP-39 wordlist") } +/// Total attempts (initial + retries) for a transient-classified persister +/// operation on the wallet-registration path. Small on purpose: this runs +/// inline while creating a wallet, not as a background job — a lock blip +/// should be ridden out in well under a second, and a genuinely stuck +/// backend must still surface promptly. +const PERSIST_RETRY_MAX_ATTEMPTS: u32 = 4; + +/// Backoff before the first retry; doubles on each subsequent attempt. +const PERSIST_RETRY_INITIAL_BACKOFF: std::time::Duration = std::time::Duration::from_millis(20); + +/// Ceiling for the doubling backoff so registration latency stays bounded +/// (worst case with the constants above: 20 + 40 + 80 ≈ 140 ms). +const PERSIST_RETRY_MAX_BACKOFF: std::time::Duration = std::time::Duration::from_millis(200); + +/// Retry a synchronous persister operation while it fails *transiently*, +/// using bounded exponential backoff. +/// +/// `op` runs once, then re-runs after a backoff sleep for as long as it +/// returns a [`PersistenceError`] whose +/// [`is_transient()`](PersistenceError::is_transient) is true, up to +/// [`PERSIST_RETRY_MAX_ATTEMPTS`]. A fatal error (or success) returns +/// immediately — a fatal failure never retries. The sleep is async so it +/// yields the Tokio worker instead of spinning the CPU, which is exactly +/// what the storage layer's `FlushRetryable` contract asks callers to do. +pub(crate) async fn retry_transient(mut op: F) -> Result +where + F: FnMut() -> Result, +{ + let mut backoff = PERSIST_RETRY_INITIAL_BACKOFF; + let mut attempt: u32 = 1; + loop { + match op() { + Ok(value) => return Ok(value), + Err(e) if e.is_transient() && attempt < PERSIST_RETRY_MAX_ATTEMPTS => { + tracing::debug!( + attempt, + max_attempts = PERSIST_RETRY_MAX_ATTEMPTS, + backoff_ms = backoff.as_millis() as u64, + error = %e, + "transient persister failure — backing off before retry" + ); + tokio::time::sleep(backoff).await; + backoff = backoff.saturating_mul(2).min(PERSIST_RETRY_MAX_BACKOFF); + attempt += 1; + } + Err(e) => return Err(e), + } + } +} + /// Test-only rendezvous fired inside [`PlatformWalletManager::remove_wallet_with_teardown`], /// between the inner-manager removal and the public-map removal. /// @@ -484,24 +534,38 @@ impl PlatformWalletManager

{ } } - if let Err(e) = self.persister.store(wallet_id, registration_changeset) { + // Persist the registration changeset, riding out a *transient* + // backend blip (e.g. `SQLITE_BUSY`) with bounded exponential backoff + // before giving up. On a transient `store` failure the persister + // restores the buffered changeset (its documented contract), so the + // retries re-drive that same write via `flush` — no re-merge, no + // double-count: the first attempt hands the changeset over, later + // attempts flush what the buffer preserved. A fatal error is not + // retried and fails fast. Either way the typed `PersistenceError` + // (and its transient/fatal classification) is preserved for the + // caller instead of being flattened to a string. + let mut changeset_slot = Some(registration_changeset); + let store_result = retry_transient(|| match changeset_slot.take() { + Some(cs) => self.persister.store(wallet_id, cs), + None => self.persister.flush(wallet_id), + }) + .await; + if let Err(e) = store_result { tracing::error!( wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), error = %e, - "failed to persist wallet registration changeset" + "failed to persist wallet registration changeset after retries" ); let mut wm = self.wallet_manager.write().await; - if let Err(e) = wm.remove_wallet(&wallet_id) { + if let Err(remove_err) = wm.remove_wallet(&wallet_id) { tracing::warn!( wallet_id = %hex::encode(wallet_id), - error = %e, + error = %remove_err, "rollback: remove_wallet failed while unwinding a failed wallet registration" ); } - return Err(PlatformWalletError::WalletCreation(format!( - "Failed to persist wallet registration changeset: {}", - e - ))); + return Err(PlatformWalletError::PersisterStore(e)); } // Build the PlatformWallet handle. @@ -531,26 +595,36 @@ impl PlatformWalletManager

{ // earlier `insert_wallet`, absent from `self.wallets`), // poisoning every retry on `WalletAlreadyExists`. Roll back // before bailing — same shape as `manager::load`. + // Retry a transient load blip the same way as the store above; a + // load is an idempotent read, so re-reading after a lock blip is + // safe. `load_persisted()` returns the typed `PersistenceError` this + // rehydration boundary is built around, routed through the + // dedicated `PersisterLoad` variant so its retry classification + // survives to the caller. + let load_result = retry_transient(|| platform_wallet.load_persisted()).await; let crate::changeset::ClientStartState { mut platform_addresses, wallets: _, #[cfg(feature = "shielded")] shielded: _, - } = match platform_wallet.load_persisted() { + } = match load_result { Ok(state) => state, Err(e) => { + tracing::error!( + wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), + error = %e, + "failed to load persisted wallet state after retries" + ); let mut wm = self.wallet_manager.write().await; - if let Err(e) = wm.remove_wallet(&wallet_id) { + if let Err(remove_err) = wm.remove_wallet(&wallet_id) { tracing::warn!( wallet_id = %hex::encode(wallet_id), - error = %e, + error = %remove_err, "rollback: remove_wallet failed while unwinding a failed wallet setup" ); } - return Err(PlatformWalletError::WalletCreation(format!( - "Failed to load persisted wallet state: {}", - e - ))); + return Err(PlatformWalletError::PersisterLoad(e)); } }; @@ -560,18 +634,23 @@ impl PlatformWalletManager

{ .initialize_from_persisted(persisted) .await { + tracing::error!( + wallet_id = %hex::encode(wallet_id), + error = %e, + "failed to restore persisted platform-address state" + ); let mut wm = self.wallet_manager.write().await; - if let Err(e) = wm.remove_wallet(&wallet_id) { + if let Err(remove_err) = wm.remove_wallet(&wallet_id) { tracing::warn!( wallet_id = %hex::encode(wallet_id), - error = %e, + error = %remove_err, "rollback: remove_wallet failed while unwinding a failed wallet setup" ); } - return Err(PlatformWalletError::WalletCreation(format!( - "Failed to restore persisted platform address state: {}", - e - ))); + // `initialize_from_persisted` already returns a typed + // `PlatformWalletError`; wrap (boxed) rather than stringify so + // its concrete variant and source chain survive. + return Err(PlatformWalletError::PersisterRestore(Box::new(e))); } } else { platform_wallet.platform().initialize().await; @@ -1275,6 +1354,406 @@ mod register_wallet_duplicate_tests { } } +#[cfg(test)] +mod persist_retry_tests { + //! Registration-path persistence: transient-error retry and typed + //! error classification across the boundary. + + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + use key_wallet::mnemonic::{Language, Mnemonic}; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::Network; + + use crate::changeset::{ + ClientStartState, PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet, + PlatformWalletPersistence, + }; + use crate::error::PlatformWalletError; + use crate::events::{EventHandler, PlatformEventHandler}; + use crate::wallet::platform_wallet::WalletId; + use crate::PlatformWalletManager; + + // Canonical all-`abandon` BIP-39 test vector. + const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon \ + abandon abandon abandon abandon abandon about"; + + fn transient() -> PersistenceError { + PersistenceError::backend_with_kind( + PersistenceErrorKind::Transient, + "simulated SQLITE_BUSY", + ) + } + + fn fatal() -> PersistenceError { + PersistenceError::backend_with_kind(PersistenceErrorKind::Fatal, "simulated corruption") + } + + /// Persister whose `store` / `flush` / `load` outcomes are scripted so + /// the registration retry path can be driven deterministically. Models + /// the real contract: a transient `store` failure preserves the + /// changeset in the buffer, so the retry re-drives the write through + /// `flush`. + /// + /// `store` counts registration and identity-scan-verdict writes + /// separately. Registration ends with a best-effort `identity().sync()`, + /// so a successful registration issues a SECOND `store` carrying the scan + /// verdict; a single counter would make every assertion about the + /// registration write depend on unrelated discovery behaviour. The + /// changeset itself is the discriminator. + #[derive(Default)] + struct FaultyPersister { + /// Stores of the registration changeset. + registration_store_calls: AtomicUsize, + /// Stores of the identity-scan verdict published by `identity().sync()`. + scan_verdict_store_calls: AtomicUsize, + flush_calls: AtomicUsize, + load_calls: AtomicUsize, + /// The first registration `store` fails transiently (buffer preserved + /// for retry). + store_transient_first: bool, + /// Every registration `store` fails fatally (must NOT retry). + store_fatal: bool, + /// Number of leading scan-verdict `store` calls that fail transiently. + scan_verdict_store_transient_failures: usize, + /// Number of leading `flush` calls that fail transiently before Ok. + flush_transient_failures: usize, + /// Number of leading `load` calls that fail transiently before Ok. + load_transient_failures: usize, + /// Every `load` fails fatally (must NOT retry). + load_fatal: bool, + } + + impl PlatformWalletPersistence for FaultyPersister { + fn store( + &self, + _wallet_id: WalletId, + changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + // One changeset can carry both: `merge` folds a buffered + // registration write and a scan verdict into a single round. Each + // counter answers only its own question — "was this changeset + // handed over?" — so both increment. Letting the first match win + // would make an assertion about the registration write depend on + // whether discovery happened to be batched with it, which is the + // coupling these separate counters exist to remove. + let registration = changeset + .wallet_metadata + .is_some() + .then(|| self.registration_store_calls.fetch_add(1, Ordering::SeqCst)); + let verdict = changeset + .identity_scan_state + .is_some() + .then(|| self.scan_verdict_store_calls.fetch_add(1, Ordering::SeqCst)); + + // The registration half decides a combined round's outcome: its + // failure aborts the whole registration, while a verdict's is + // swallowed. + if let Some(n) = registration { + if self.store_fatal { + return Err(fatal()); + } + if self.store_transient_first && n == 0 { + return Err(transient()); + } + } + if let Some(n) = verdict { + if n < self.scan_verdict_store_transient_failures { + return Err(transient()); + } + } + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + let n = self.flush_calls.fetch_add(1, Ordering::SeqCst); + if n < self.flush_transient_failures { + Err(transient()) + } else { + Ok(()) + } + } + + fn load(&self) -> Result { + let n = self.load_calls.fetch_add(1, Ordering::SeqCst); + if self.load_fatal { + return Err(fatal()); + } + if n < self.load_transient_failures { + return Err(transient()); + } + Ok(ClientStartState::default()) + } + } + + struct NoopEventHandler; + impl EventHandler for NoopEventHandler {} + impl PlatformEventHandler for NoopEventHandler {} + + fn make_manager( + persister: Arc, + ) -> Arc> { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let event_handler: Arc = Arc::new(NoopEventHandler); + Arc::new(PlatformWalletManager::new(sdk, persister, event_handler)) + } + + fn seed_bytes() -> [u8; 64] { + Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid test mnemonic") + .to_seed("") + } + + /// `Some(0)` skips the SPV-tip birth-height lookup so the test never + /// consults SPV. + async fn register( + manager: &PlatformWalletManager, + ) -> Result<(), PlatformWalletError> { + manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed_bytes(), + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .map(|_| ()) + } + + /// A transient `store` failure is ridden out — the persister + /// buffers the changeset, the retry re-drives it via `flush`, and + /// registration succeeds instead of aborting. + #[tokio::test] + async fn transient_store_failure_is_retried_and_succeeds() { + let persister = Arc::new(FaultyPersister { + store_transient_first: true, + flush_transient_failures: 1, // one transient flush, then Ok + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + register(&manager) + .await + .expect("registration must succeed after retrying the transient store"); + + // store attempted once; flush retried twice (fail, then succeed). + assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 2); + // Registration ends in `identity().sync()`, whose scan publishes its + // verdict — the write that makes a partial scan survive a restart. + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 1, + "a completed registration must publish the identity-scan verdict" + ); + } + + /// A fatal `store` failure fails fast — no retry — and + /// surfaces as the typed `PersisterStore` whose inner classification is + /// non-transient. + #[tokio::test] + async fn fatal_store_failure_fails_fast_without_retry() { + let persister = Arc::new(FaultyPersister { + store_fatal: true, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("a fatal store must abort registration"); + + match err { + PlatformWalletError::PersisterStore(pe) => assert!( + !pe.is_transient(), + "a fatal store must carry non-transient classification" + ), + other => panic!("expected PersisterStore, got {other:?}"), + } + assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); + assert_eq!( + persister.flush_calls.load(Ordering::SeqCst), + 0, + "a fatal store must not be retried via flush" + ); + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 0, + "an aborted registration never reaches the discovery scan" + ); + } + + /// A store that stays transient exhausts the + /// bounded retry budget and returns the typed `PersisterStore` still + /// carrying transient classification (distinguishable from the fatal + /// case above). + #[tokio::test] + async fn persistently_transient_store_exhausts_bounded_retries() { + let persister = Arc::new(FaultyPersister { + store_transient_first: true, + flush_transient_failures: usize::MAX, // never recovers + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("registration must fail once the retry budget is spent"); + + match err { + PlatformWalletError::PersisterStore(pe) => assert!( + pe.is_transient(), + "an exhausted-but-transient store must stay classified transient" + ), + other => panic!("expected PersisterStore, got {other:?}"), + } + // 1 store + 3 flush retries == 4 total attempts (the budget). + assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 3); + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 0, + "an aborted registration never reaches the discovery scan" + ); + } + + /// A transient `load` blip during rehydration is retried (an + /// idempotent read), so registration succeeds. + #[tokio::test] + async fn transient_load_failure_is_retried_and_succeeds() { + let persister = Arc::new(FaultyPersister { + load_transient_failures: 1, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + register(&manager) + .await + .expect("registration must succeed after retrying the transient load"); + + assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 0); + assert_eq!(persister.load_calls.load(Ordering::SeqCst), 2); + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 1, + "a completed registration must publish the identity-scan verdict" + ); + } + + /// A fatal `load` fails fast and surfaces as the typed + /// `PersisterLoad` — never the flattened `WalletCreation(String)`. + #[tokio::test] + async fn fatal_load_failure_surfaces_as_persister_load() { + let persister = Arc::new(FaultyPersister { + load_fatal: true, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("a fatal load must abort registration"); + + match err { + PlatformWalletError::PersisterLoad(pe) => assert!(!pe.is_transient()), + other => panic!("expected PersisterLoad, got {other:?}"), + } + assert_eq!( + persister.load_calls.load(Ordering::SeqCst), + 1, + "a fatal load must not be retried" + ); + } + + /// A transient failure persisting the identity-scan verdict is ridden out + /// on the same bounded policy the registration write uses, so a merely + /// busy backend does not cost the verdict its survival across a restart + /// (dashpay/platform#4365). + #[tokio::test] + async fn should_retry_a_transient_scan_verdict_store() { + let persister = Arc::new(FaultyPersister { + scan_verdict_store_transient_failures: 1, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + register(&manager) + .await + .expect("a retried scan-verdict store must not disturb registration"); + + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 1, + "the verdict is handed over once; the retry re-drives it via flush" + ); + assert_eq!( + persister.flush_calls.load(Ordering::SeqCst), + 1, + "the transient verdict store must be retried through flush" + ); + } + + /// Retrying the verdict never escalates into failing the scan that just + /// succeeded: once the budget is spent the outcome is logged and dropped, + /// and registration still returns Ok. + #[tokio::test] + async fn should_not_fail_registration_when_the_scan_verdict_never_persists() { + let persister = Arc::new(FaultyPersister { + scan_verdict_store_transient_failures: usize::MAX, + flush_transient_failures: usize::MAX, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + register(&manager) + .await + .expect("an unpersistable verdict must never fail wallet registration"); + + // 1 store + 3 flush retries == the shared 4-attempt budget. + assert_eq!(persister.scan_verdict_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 3); + } + + /// The typed persister-phase variants preserve retry + /// classification, enable structural matching, and keep the `#[source]` + /// chain instead of flattening to a string. + #[test] + fn typed_variants_preserve_classification_matching_and_source() { + use std::error::Error; + + let store_err = PlatformWalletError::PersisterStore(transient()); + match &store_err { + PlatformWalletError::PersisterStore(pe) => assert!(pe.is_transient()), + other => panic!("expected PersisterStore, got {other:?}"), + } + assert!( + store_err.source().is_some(), + "PersisterStore must expose its PersistenceError source" + ); + + let load_err = PlatformWalletError::PersisterLoad(fatal()); + match &load_err { + PlatformWalletError::PersisterLoad(pe) => assert!(!pe.is_transient()), + other => panic!("expected PersisterLoad, got {other:?}"), + } + assert!(load_err.source().is_some()); + + // The restore variant wraps a typed inner error; structural matching + // must recover the concrete inner variant, not an opaque string. + let restore_err = + PlatformWalletError::PersisterRestore(Box::new(PlatformWalletError::WalletLocked)); + assert!(restore_err.source().is_some()); + match restore_err { + PlatformWalletError::PersisterRestore(inner) => { + assert!(matches!(*inner, PlatformWalletError::WalletLocked)); + } + other => panic!("expected PersisterRestore, got {other:?}"), + } + } +} + /// Removal versus a same-id re-registration that lands *during* the removal /// (`dashpay/platform#4185` review). /// diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs index 6d4b674d965..49f34530e22 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs @@ -27,9 +27,9 @@ use super::super::manager::AssetLockManager; /// Persister errors are surfaced as `Err(PersistenceError)` so call /// sites can choose their own policy: /// -/// - **Poll loops** (`wait_for_chain_lock`, `wait_for_proof`) typically -/// downgrade to `None` for the current iteration so the next tick -/// retries — see [`record_or_persister_or_log`] for that policy. +/// - **Poll loops** (`wait_for_chain_lock`, `wait_for_proof`) downgrade +/// transient failures to `None` for the current iteration and surface +/// permanent failures — see [`record_or_persister_or_log`]. /// - **One-shot recovery / fast-fail call sites** want the error /// visible so a transient backend failure isn't silently classified /// as "tx not found" — they handle the `Err` arm explicitly. @@ -143,26 +143,27 @@ pub(in crate::wallet::asset_lock) fn record_holds_local_finality( } } -/// Variant of [`record_or_persister`] that swallows persister errors -/// as `None` after a `warn`-level log. Use this from poll loops where -/// the next iteration retries — a hard error from a single tick would -/// abort the whole poll prematurely. +/// Variant of [`record_or_persister`] that retries transient failures as a miss. +/// +/// Use this from poll loops where the next iteration retries. Permanent +/// failures remain errors so an unbounded poll cannot hide them. pub(super) fn record_or_persister_or_log( in_memory: Option, persister: &crate::wallet::persister::WalletPersister, txid: &Txid, -) -> Option { +) -> Result, crate::changeset::PersistenceError> { match record_or_persister(in_memory, persister, txid) { - Ok(opt) => opt, - Err(e) => { + Ok(opt) => Ok(opt), + Err(e) if e.is_transient() => { tracing::warn!( txid = %txid, error = %e, - "Persister fallback for core tx record failed; \ + "Transient persister fallback for core tx record failed; \ treating as miss for this poll iteration" ); - None + Ok(None) } + Err(e) => Err(e), } } @@ -393,7 +394,7 @@ impl AssetLockManager { }) }; if let Some(record) = - record_or_persister_or_log(in_memory, &self.persister, &out_point.txid) + record_or_persister_or_log(in_memory, &self.persister, &out_point.txid)? { if matches!(record.context, TransactionContext::InChainLockedBlock(_)) { if let Some(h) = record.height() { @@ -520,7 +521,7 @@ impl AssetLockManager { }) }; if let Some(record) = - record_or_persister_or_log(in_memory, &self.persister, &out_point.txid) + record_or_persister_or_log(in_memory, &self.persister, &out_point.txid)? { match &record.context { TransactionContext::InstantSend(instant_lock) => { @@ -969,8 +970,7 @@ mod tests { } } - /// Test persister that always errors out on `get_core_tx_record`, - /// to exercise the error-swallowing branch in `record_or_persister`. + /// Test persister that returns a permanent `get_core_tx_record` error. struct ErroringStore; impl PlatformWalletPersistence for ErroringStore { @@ -996,6 +996,34 @@ mod tests { } } + struct TransientErroringStore; + + impl PlatformWalletPersistence for TransientErroringStore { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + fn load(&self) -> Result { + Ok(ClientStartState::default()) + } + fn get_core_tx_record( + &self, + _wallet_id: WalletId, + _txid: &Txid, + ) -> Result, PersistenceError> { + Err(PersistenceError::backend_with_kind( + crate::changeset::PersistenceErrorKind::Transient, + "simulated transient backend failure", + )) + } + } + fn wallet_persister(inner: Arc) -> WalletPersister { WalletPersister::new([0u8; 32], inner) } @@ -1060,9 +1088,7 @@ mod tests { #[test] fn record_or_persister_propagates_backend_errors() { // Backend errors surface as `Err` so call sites can choose - // their own policy (one-shot recovery logs at error and - // degrades; poll loops downgrade to None for one tick via - // `record_or_persister_or_log`). + // their own policy; poll loops only downgrade transient errors. let unknown_txid = Txid::from([0xFF; 32]); let persister = wallet_persister(Arc::new(ErroringStore)); @@ -1071,14 +1097,21 @@ mod tests { } #[test] - fn record_or_persister_or_log_swallows_backend_errors_as_none() { - // The poll-loop variant downgrades errors to `None` (after a - // `warn` log) so a transient backend failure on one tick - // doesn't abort the whole poll. + fn record_or_persister_or_log_surfaces_permanent_backend_errors() { let unknown_txid = Txid::from([0xFF; 32]); let persister = wallet_persister(Arc::new(ErroringStore)); let resolved = record_or_persister_or_log(None, &persister, &unknown_txid); + assert!(resolved.is_err()); + } + + #[test] + fn record_or_persister_or_log_retries_transient_backend_errors() { + let unknown_txid = Txid::from([0xFF; 32]); + let persister = wallet_persister(Arc::new(TransientErroringStore)); + + let resolved = record_or_persister_or_log(None, &persister, &unknown_txid) + .expect("transient poll error must be downgraded for retry"); assert!(resolved.is_none()); } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs index 93dbfe1ff5b..50731a63871 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -640,9 +640,14 @@ impl IdentityWallet { /// Best-effort by design, and on the persist half only: the in-memory /// record always lands, so a second bring-up in this process already sees /// an incomplete scan and rescans. A failed persist costs the verdict its - /// survival across a restart, which is the same exposure a host that has - /// no slot for the field already has — it must not be allowed to fail the - /// scan that just succeeded. + /// survival across a restart, and it must not be allowed to fail the scan + /// that just succeeded. + /// + /// Best-effort is not one-shot, though. A backend that is merely busy + /// would otherwise cost the verdict its durability outright, which is the + /// gap the verdict exists to close (dashpay/platform#4365), so a transient + /// failure is ridden out on the same bounded policy the registration path + /// uses before the outcome is swallowed. async fn publish_scan_verdict( &self, wallet_id: crate::wallet::platform_wallet::WalletId, @@ -672,12 +677,23 @@ impl IdentityWallet { identity_scan_state: Some(recorded), ..Default::default() }; - if let Err(e) = self.persister.store(changeset) { - tracing::warn!( + // On a transient `store` failure the persister keeps the changeset + // buffered (its documented contract), so the retries re-drive that + // same write through `flush` rather than handing it over twice. + let mut changeset_slot = Some(changeset); + let outcome = crate::manager::retry_transient(|| match changeset_slot.take() { + Some(cs) => self.persister.store(cs), + None => self.persister.flush(), + }) + .await; + if let Err(e) = outcome { + tracing::error!( wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), error = %e, - "failed to persist the identity-scan verdict; a partial scan may not be \ - retried after a restart" + "identity-scan verdict could not be persisted after retries; a partial scan \ + will not be retried after a restart, so an identity at an unanswered index \ + stays hidden until a later scan publishes a verdict that lands" ); } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index d59c3250390..659a430ffe3 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -670,6 +670,11 @@ impl DashPayView<'_, B> { /// retried on the next sweep. /// /// Returns the number of entries confirmed this pass. + /// + /// # Errors + /// + /// Transient persistence read failures are deferred to the next sweep; + /// permanent failures return [`PlatformWalletError::PersisterLoad`]. pub async fn reconcile_sent_payments(&self) -> Result { use crate::wallet::identity::types::dashpay::payment::{PaymentDirection, PaymentStatus}; @@ -704,14 +709,16 @@ impl DashPayView<'_, B> { let record = match self.persister.get_core_tx_record(&txid) { Ok(Some(record)) => record, Ok(None) => continue, - Err(e) => { + Err(e) if e.is_transient() => { tracing::warn!( error = %e, txid = %txid_str, - "reconcile_sent_payments: tx-record read failed; will retry next sweep" + "reconcile_sent_payments: transient tx-record read failed; \ + will retry next sweep" ); continue; } + Err(e) => return Err(PlatformWalletError::PersisterLoad(e)), }; // An InstantSend lock is final for DashPay display, same as a // mined block — one definition of "final", shared with the @@ -1648,7 +1655,8 @@ mod tests { use key_wallet::Network; use crate::changeset::{ - ClientStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, + ClientStartState, PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet, + PlatformWalletPersistence, }; use crate::error::PlatformWalletError; use crate::events::{EventHandler, PlatformEventHandler}; @@ -1698,6 +1706,9 @@ mod tests { key_wallet::managed_account::transaction_record::TransactionRecord, >, >, + /// `Some(kind)` makes every `get_core_tx_record` fail with that + /// error class instead of answering from `records`. + read_error_kind: Mutex>, /// Txids the enumeration lists but `get_core_tx_record` answers /// `Ok(None)` for — the FFI shape for "row exists, record not /// available yet" (missing bytes, undecodable, pending InstantSend). @@ -1746,6 +1757,12 @@ mod tests { PersistenceError, > { *self.get_core_tx_record_calls.lock().unwrap() += 1; + if let Some(kind) = *self.read_error_kind.lock().unwrap() { + return Err(PersistenceError::backend_with_kind( + kind, + "simulated tx-record read failure", + )); + } if self.listed_but_unavailable.lock().unwrap().contains(txid) { return Ok(None); } @@ -3584,6 +3601,26 @@ mod tests { 0, "reconcile must be idempotent" ); + + *persister.read_error_kind.lock().unwrap() = Some(PersistenceErrorKind::Transient); + assert_eq!( + iw.dashpay() + .reconcile_sent_payments() + .await + .expect("transient read failure must wait for the next sweep"), + 0 + ); + + *persister.read_error_kind.lock().unwrap() = Some(PersistenceErrorKind::Fatal); + let err = iw + .dashpay() + .reconcile_sent_payments() + .await + .expect_err("permanent read failure must abort the reconcile sweep"); + assert!(matches!( + err, + PlatformWalletError::PersisterLoad(ref source) if !source.is_transient() + )); } #[tokio::test] From 950095527ddbe5b3d30e08cc7c6f47a7cba2841a Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:18:31 +0000 Subject: [PATCH 2/9] docs(platform-wallet): correct the failed-load test's coverage claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `failed_load_releases_persister_for_reconstruct` claimed the end-to-end open -> failed load -> reopen path was "covered by the storage crate's own round-trip coverage test". It is not: `platform-wallet-storage` contains no reference to `PlatformWalletManager` outside README prose, and its `sqlite_second_open_guard` asserts only the storage-side half — that dropping the last `SqlitePersister` handle frees the path claim so a later open succeeds. Nothing composes the two halves. The doc now states what the test actually proves (a strong count back at 1 is the necessary precondition for a clean re-open, not the re-open itself) and why the composed path cannot be driven from this crate: the concrete persister lives in `platform-wallet-storage`, which depends on this one. A TODO marks the real gap on the side that can close it. The stale justification for the omission is also dropped — it cited a dev-dependency cycle, but the operative constraint is simply the direction of the dependency. 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- .../rs-platform-wallet/src/manager/load.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index ed65cdd0283..d7b7c244ba4 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -471,15 +471,16 @@ mod tests { /// doesn't hit `WalletStorageError::AlreadyOpen` masking the real error /// (issue #4133). /// - /// This is a **manager-side proxy**, not a full end-to-end proof: it asserts - /// the persister's strong count returns to 1 (the test's own probe) after a - /// failed load + teardown — a lingering adapter clone would keep it above 1 - /// — which is the necessary precondition for a clean re-open. It does not - /// itself open a real `SqlitePersister`, fail, and re-open on the same path; - /// the platform-wallet ⇄ platform-wallet-storage dev-dependency cycle - /// precludes using the concrete persister here. That end-to-end - /// open → fail → reopen is covered by the storage crate's own round-trip - /// coverage test. + /// This is a **manager-side proxy**, not an end-to-end proof: a strong count + /// back at 1 (the test's own probe) after a failed load + teardown is the + /// necessary precondition for a clean re-open, not the re-open itself. The + /// concrete `SqlitePersister` lives in `platform-wallet-storage`, which + /// depends on this crate, so only that side can drive the composed path — + /// and its `sqlite_second_open_guard` covers just the other half (dropping + /// the last handle frees the path claim), never building a + /// `PlatformWalletManager`. + // TODO: cover the composed open -> failed load -> reopen from + // platform-wallet-storage; neither side asserts it today. // Multi-thread: dropping the manager runs upstream's `Drop`, whose // `ThreadRegistry::shutdown()` asserts a multi-thread runtime. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] From b88fdf49a83d89759ead56005e3d8dcd0290ecec Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:16:32 +0000 Subject: [PATCH 3/9] fix(platform-wallet): keep the manager usable after a failed load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed `load_from_persistor` ran the manager-wide, one-way `shutdown()` on both failure paths. That seals every coordinator's quiesce gate (admission never reopens) and joins the wallet-event adapter, whose persistence receiver is taken exactly once and so cannot be respawned — a second `load_from_persistor` therefore returned `Ok(())` onto a manager that would never sync or persist again, contradicting the crate's own docs and the Kotlin KDoc's "Idempotent". The teardown existed only to release the adapter's `Arc` clone, so that reconstructing on the same store path could not hit a spurious `AlreadyOpen` masking the real error. The adapter now takes a `Weak

` and upgrades it per batch instead: release on drop is synchronous by construction and neither failure path needs to tear anything down. `adapter_holds_no_strong_persister_reference` reads the strong count on a live, idle manager with nothing dropped, cancelled or aborted, so no teardown path and no abort timing can stand in for the property. Mutation check: restoring a strong `Arc

` in `run_wallet_event_adapter` fails it (left: 5, right: 4); restoring the weak reference makes it pass again. `failed_load_releases_persister_for_reconstruct` is kept and re-scoped, with its doc corrected — it is end to end and isolates nothing. `drop_backstop_eventually_releases_persister_without_shutdown` becomes `dropping_manager_releases_persister_synchronously_when_adapter_idle`, and a new adapter test pins the one bound on that synchrony: a commit in flight holds the upgraded reference until its `store()` returns. Refs #4133 Co-Authored-By: Claude Opus 5 --- .../dashsdk/wallet/PlatformWalletManager.kt | 3 + .../rs-platform-wallet-ffi/src/manager.rs | 5 + .../src/changeset/core_bridge.rs | 114 ++++++-- .../src/manager/identity_sync.rs | 7 + .../rs-platform-wallet/src/manager/load.rs | 267 ++++++++++++------ .../rs-platform-wallet/src/manager/mod.rs | 27 +- .../rs-platform-wallet/src/test_support.rs | 4 +- 7 files changed, 297 insertions(+), 130 deletions(-) diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt index d8f0cf26b6f..01bd3d4161c 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt @@ -1081,6 +1081,9 @@ class PlatformWalletManager( * per restorable id to obtain a [ManagedPlatformWallet] handle. * * Idempotent: with no persisted state, leaves [wallets] untouched. + * + * On failure the manager is unchanged and still usable — fix the store + * and call again, or destroy the manager and rebuild it. */ suspend fun loadPersistedWallets(): List = withContext(Dispatchers.IO) { mapNativeErrors { WalletManagerNative.loadFromPersistor(managerHandle) } diff --git a/packages/rs-platform-wallet-ffi/src/manager.rs b/packages/rs-platform-wallet-ffi/src/manager.rs index 4180a774462..261409f5698 100644 --- a/packages/rs-platform-wallet-ffi/src/manager.rs +++ b/packages/rs-platform-wallet-ffi/src/manager.rs @@ -591,6 +591,11 @@ pub unsafe extern "C" fn platform_wallet_manager_create_wallet_from_mnemonic_wit /// produce wallet handles — the caller should follow up with /// [`platform_wallet_manager_get_wallet`] per `wallet_id` it knows /// about. +/// +/// On error the handle stays valid and the manager is unchanged: fix the +/// store and call again, or `platform_wallet_manager_destroy` it and +/// reconstruct. Destroying releases the persister before it returns, which a +/// reconstruct over the same store path needs. #[no_mangle] pub unsafe extern "C" fn platform_wallet_manager_load_from_persistor( manager_handle: Handle, diff --git a/packages/rs-platform-wallet/src/changeset/core_bridge.rs b/packages/rs-platform-wallet/src/changeset/core_bridge.rs index f9b7f491977..44714a13286 100644 --- a/packages/rs-platform-wallet/src/changeset/core_bridge.rs +++ b/packages/rs-platform-wallet/src/changeset/core_bridge.rs @@ -34,7 +34,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, Weak}; use dashcore::blockdata::transaction::{txout::TxOut, OutPoint}; use key_wallet::account::AccountType; @@ -220,12 +220,16 @@ impl std::fmt::Display for BatchDiagnostics { /// than silently re-freezing on the next launch. /// /// Generic over `P` so the spawned task gets static-dispatch on -/// every `persister.store(...)` call. Pass the manager's own -/// `Arc

` (not the `Arc` -/// coercion) to actually realize the static-dispatch win. +/// every `persister.store(...)` call. Pass a `Weak` to the manager's own +/// `Arc

` (not to the `Arc` coercion) to +/// actually realize the static-dispatch win. +/// +/// The reference is **weak**: the task upgrades it for the duration of each +/// batch commit and holds nothing while idle, so the persister is released +/// as soon as its owner drops rather than when this task next polls. pub fn spawn_wallet_event_adapter

( wallet_manager: Arc>>, - persister: Arc

, + persister: Weak

, receiver: mpsc::UnboundedReceiver, sync_fault: Arc, cancel: CancellationToken, @@ -296,7 +300,7 @@ where /// show a hard "verification failed / rescan pending" state. async fn run_wallet_event_adapter

( wallet_manager: Arc>>, - persister: Arc

, + persister: Weak

, mut receiver: mpsc::UnboundedReceiver, sync_fault: Arc, cancel: CancellationToken, @@ -427,7 +431,13 @@ async fn run_wallet_event_adapter

( // accounted for" and "nobody knows". let settled: Arc>> = Arc::new(Mutex::new(Vec::new())); let settled_for_commit = Arc::clone(&settled); - let persister_for_commit = Arc::clone(&persister); + // Upgraded per batch and held only for the commit: an idle adapter + // must not keep the persister open, or a manager whose owner dropped + // it stays "open" until this task next polls (issue #4133). + let Some(persister_for_commit) = persister.upgrade() else { + tracing::debug!("persister released; wallet-event adapter exiting"); + break; + }; let sync_fault_for_commit = Arc::clone(&sync_fault); let fault_for_commit = Arc::clone(&fault); let freeze_for_commit = Arc::clone(&freeze_logged); @@ -3006,7 +3016,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3055,7 +3065,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3106,7 +3116,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3154,7 +3164,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3201,7 +3211,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3253,7 +3263,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3300,7 +3310,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3387,7 +3397,7 @@ mod tests { let handle = runtime.spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3431,6 +3441,72 @@ mod tests { }); } + /// The adapter upgrades its weak persister reference for exactly the span + /// of a batch commit, and holds nothing outside it. + /// + /// That span is the sole bound on the manager's synchronous release: a + /// drop racing a commit reclaims the persister when the parked `store()` + /// returns, not immediately (issue #4133). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn an_in_flight_commit_holds_a_strong_persister_reference() { + use std::time::{Duration, Instant}; + + let wallet_id = [0x44u8; 32]; + let (tx, rx) = unbounded_channel::(); + let (obs_tx, mut obs_rx) = unbounded_channel(); + let persister = Arc::new(ProbePersister::new(obs_tx)); + let (release, blocked) = persister.block_next(); + let sync_fault = Arc::new(AtomicBool::new(false)); + let cancel = CancellationToken::new(); + let handle = tokio::spawn(run_wallet_event_adapter( + test_manager(), + Arc::downgrade(&persister), + rx, + Arc::clone(&sync_fault), + cancel.clone(), + )); + + assert_eq!( + Arc::strong_count(&persister), + 1, + "an idle adapter must hold the persister weakly — only this test's \ + own reference may be strong" + ); + + // Park the commit inside `store()`, and wait until the park is in + // effect so the count below is read during the commit, not before it. + tx.send(block_processed_event(wallet_id, 10)).unwrap(); + let deadline = Instant::now() + Duration::from_secs(5); + while !blocked.load(Ordering::Relaxed) { + assert!( + Instant::now() < deadline, + "the store must actually park before the assertion below means anything" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert_eq!( + Arc::strong_count(&persister), + 2, + "a commit in flight must hold the upgraded reference for the whole \ + of its store()" + ); + + drop(release); + obs_rx + .recv() + .await + .expect("the released store must complete"); + cancel.cancel(); + drop(tx); + handle.await.unwrap(); + + assert_eq!( + Arc::strong_count(&persister), + 1, + "the upgraded reference must be released with the finished commit" + ); + } + /// (i) A commit panic must punish exactly the wallets whose outcome it /// left unknown — no more, no less. /// @@ -3467,7 +3543,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3568,7 +3644,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3702,7 +3778,7 @@ mod tests { let sync_fault = Arc::new(AtomicBool::new(false)); let handle = spawn_wallet_event_adapter( Arc::clone(&wallet_manager), - Arc::clone(&persister), + Arc::downgrade(&persister), event_rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3831,7 +3907,7 @@ mod tests { let sync_fault = Arc::new(AtomicBool::new(false)); let handle = spawn_wallet_event_adapter( Arc::clone(&wallet_manager), - Arc::clone(&persister), + Arc::downgrade(&persister), event_rx, Arc::clone(&sync_fault), cancel.clone(), diff --git a/packages/rs-platform-wallet/src/manager/identity_sync.rs b/packages/rs-platform-wallet/src/manager/identity_sync.rs index e3b3a591dcd..54d2fd81af8 100644 --- a/packages/rs-platform-wallet/src/manager/identity_sync.rs +++ b/packages/rs-platform-wallet/src/manager/identity_sync.rs @@ -527,6 +527,13 @@ where drained } + /// Test-only: whether new sync passes are currently barred — a drain in + /// flight, a latched timeout, or the terminal seal `shutdown` applies. + #[cfg(test)] + pub(crate) fn sync_admission_closed(&self) -> bool { + self.quiescing.is_closed() + } + /// Run one sync pass across every registered identity. /// /// If a pass is already in flight, returns immediately without diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index d7b7c244ba4..95b9c4f8060 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -28,21 +28,29 @@ impl PlatformWalletManager

{ /// wallets missing from that slice get a fresh /// [`PlatformAddressWallet::initialize`](crate::wallet::platform_addresses::PlatformAddressWallet::initialize). /// + /// # Errors + /// + /// Returns [`PersisterLoad`](PlatformWalletError::PersisterLoad) when the + /// persister cannot produce the snapshot, or the per-wallet restore error + /// when a wallet in it cannot be rebuilt. + /// + /// Any `Err` leaves the manager exactly as it was before the call — + /// partial inserts are rolled back — and it stays usable: fix the store + /// and call again, or tear it down and reconstruct. Reconstructing over + /// the same persister path needs the persister released first: + /// [`shutdown`](Self::shutdown) releases it before returning, and a plain + /// drop releases it once the last strong reference goes (the wallet-event + /// adapter holds only a weak one; a batch commit in flight holds a strong + /// one until it finishes). + /// /// [`WalletManager`]: key_wallet_manager::WalletManager pub async fn load_from_persistor(&self) -> Result<(), PlatformWalletError> { let start_state = match retry_transient(|| self.persister.load()).await { Ok(state) => state, Err(e) => { - // Preserve the typed source chain (Debug carries the real - // cause — e.g. a bincode decode failure) instead of flattening - // it to a Display string, and release the wallet-event adapter - // so a reconstruct on the same path doesn't hit `AlreadyOpen` - // masking this error. + // Debug, not Display: it carries the real cause (e.g. a + // bincode decode failure) rather than flattening the chain. tracing::debug!(error = ?e, "persister load failed during rehydration"); - let report = self.shutdown().await; - if !report.all_clean() { - tracing::warn!(?report, "wallet workers unclean after aborting rehydration"); - } return Err(PlatformWalletError::PersisterLoad(e)); } }; @@ -248,16 +256,6 @@ impl PlatformWalletManager

{ } } } - // Release the wallet-event adapter so a reconstruct on the same - // persister path doesn't hit `AlreadyOpen` (see the early-return - // path above). - let report = self.shutdown().await; - if !report.all_clean() { - tracing::warn!( - ?report, - "wallet workers left unclean after rolling back a failed rehydration" - ); - } return Err(err); } @@ -278,7 +276,8 @@ mod idempotent_load_tests { ClientStartState, ClientWalletStartState, IdentityManagerStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, }; - use crate::events::{EventHandler, PlatformEventHandler}; + use crate::events::PlatformEventHandler; + use crate::test_support::NoopTestEventHandler; use crate::wallet::platform_wallet::WalletId; use crate::PlatformWalletManager; @@ -324,15 +323,11 @@ mod idempotent_load_tests { } } - struct NoopEventHandler; - impl EventHandler for NoopEventHandler {} - impl PlatformEventHandler for NoopEventHandler {} - fn make_manager( persister: SingleWalletPersister, ) -> Arc> { let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); - let event_handler: Arc = Arc::new(NoopEventHandler); + let event_handler: Arc = Arc::new(NoopTestEventHandler); Arc::new(PlatformWalletManager::new( sdk, Arc::new(persister), @@ -390,9 +385,19 @@ mod tests { use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; + use dash_async::WorkerStatus; + use super::*; use crate::changeset::{PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet}; - use crate::events::{EventHandler, PlatformEventHandler}; + use crate::events::PlatformEventHandler; + use crate::manager::WalletWorker; + use crate::test_support::NoopTestEventHandler; + + /// Strong `Arc

` clones a freshly built [`PlatformWalletManager`] holds: + /// its own `persister` field, the `DashPayPaymentHandler` on the event + /// fan-out, and the `IdentitySyncManager`. The wallet-event adapter is + /// deliberately absent — it keeps a `Weak

` and upgrades per batch. + const MANAGER_PERSISTER_HOLDERS: usize = 3; /// Persister whose `load()` always fails — the failure path under test. struct FailingLoadPersister; @@ -443,9 +448,41 @@ mod tests { } } - struct NoopEventHandler; - impl EventHandler for NoopEventHandler {} - impl PlatformEventHandler for NoopEventHandler {} + /// `load()` fails permanently once and succeeds from then on — the host + /// path of "surface the error, fix the store, call again". + #[derive(Default)] + struct FatalOnceLoadPersister { + load_calls: AtomicUsize, + } + + impl PlatformWalletPersistence for FatalOnceLoadPersister { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + + fn load(&self) -> Result { + if self.load_calls.fetch_add(1, Ordering::SeqCst) == 0 { + return Err(PersistenceError::backend("simulated fatal load failure")); + } + Ok(ClientStartState::default()) + } + } + + fn make_manager( + persister: Arc

, + ) -> PlatformWalletManager

{ + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let handler: Arc = Arc::new(NoopTestEventHandler); + PlatformWalletManager::new(sdk, persister, handler) + } #[tokio::test] async fn transient_load_failure_during_startup_rehydration_is_retried() { @@ -453,9 +490,7 @@ mod tests { load_calls: AtomicUsize::new(0), }); let probe = Arc::clone(&persister); - let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); - let handler: Arc = Arc::new(NoopEventHandler); - let manager = PlatformWalletManager::new(sdk, persister, handler); + let manager = make_manager(persister); manager .load_from_persistor() @@ -465,20 +500,84 @@ mod tests { assert_eq!(probe.load_calls.load(Ordering::SeqCst), 2); } - /// A failed `load_from_persistor` must (a) surface the typed `PersisterLoad` - /// error preserving the source chain, and (b) release the wallet-event - /// adapter's `Arc` clone so a reconstruct on the same path - /// doesn't hit `WalletStorageError::AlreadyOpen` masking the real error - /// (issue #4133). + /// The wallet-event adapter must keep a `Weak

`, never a strong clone. + /// + /// Isolating by construction: the count is read on a live, idle manager + /// with nothing dropped, cancelled or aborted, so no teardown path and no + /// abort timing can stand in for the property. Restoring a strong `Arc

` + /// in `run_wallet_event_adapter` turns it red. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn adapter_holds_no_strong_persister_reference() { + let persister = Arc::new(FailingLoadPersister); + let probe = Arc::clone(&persister); + let _manager = make_manager(persister); + + assert_eq!( + Arc::strong_count(&probe), + MANAGER_PERSISTER_HOLDERS + 1, + "expected exactly {} strong persister references — the manager's \ + own `persister` field, the DashPayPaymentHandler on the event \ + fan-out, the IdentitySyncManager, and this test's probe. The idle \ + wallet-event adapter must not be among them: it holds a Weak

\ + and upgrades it per batch", + MANAGER_PERSISTER_HOLDERS + 1 + ); + } + + /// A failed `load_from_persistor` must leave the manager usable: the host + /// fixes its store and calls again. + /// + /// Both failure paths used to run the manager-wide, one-way `shutdown()`, + /// which seals every coordinator's admission gate and joins the + /// wallet-event adapter — so the retry returned `Ok(())` onto a manager + /// that could never sync or persist again (issue #4133). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn manager_stays_usable_after_a_failed_load() { + let manager = make_manager(Arc::new(FatalOnceLoadPersister::default())); + + let err = manager + .load_from_persistor() + .await + .expect_err("the first load must fail"); + assert!( + matches!(err, PlatformWalletError::PersisterLoad(_)), + "load failure must surface as the typed PersisterLoad variant, got {err:?}" + ); + + manager + .load_from_persistor() + .await + .expect("a load retried after a failed one must succeed"); + + assert!( + !manager.identity_sync_manager.sync_admission_closed(), + "a failed load must leave sync admission open — a sealed gate \ + makes every later `Ok(())` a lie" + ); + + // The adapter is the only writer of core wallet events to the + // persister and its receiver is taken exactly once, so a joined + // adapter cannot be respawned: `Ok` here means the reused manager + // still persists. + let report = manager.shutdown().await; + assert_eq!( + report.per_worker.get(&WalletWorker::EventAdapter), + Some(&WorkerStatus::Ok), + "the wallet-event adapter must still have been running for \ + shutdown to join it: {report:?}" + ); + } + + /// End to end: a failed `load_from_persistor` surfaces the typed + /// `PersisterLoad` error, and dropping the manager afterwards releases the + /// persister — the precondition for reconstructing on the same path + /// without a spurious `WalletStorageError::AlreadyOpen` masking the real + /// error (issue #4133). /// - /// This is a **manager-side proxy**, not an end-to-end proof: a strong count - /// back at 1 (the test's own probe) after a failed load + teardown is the - /// necessary precondition for a clean re-open, not the re-open itself. The - /// concrete `SqlitePersister` lives in `platform-wallet-storage`, which - /// depends on this crate, so only that side can drive the composed path — - /// and its `sqlite_second_open_guard` covers just the other half (dropping - /// the last handle frees the path claim), never building a - /// `PlatformWalletManager`. + /// Isolates nothing: the final count is the product of the whole teardown, + /// so it stays green while any one participant regresses as long as + /// another still releases. `adapter_holds_no_strong_persister_reference` + /// is the test that pins the weak adapter reference. // TODO: cover the composed open -> failed load -> reopen from // platform-wallet-storage; neither side asserts it today. // Multi-thread: dropping the manager runs upstream's `Drop`, whose @@ -487,10 +586,7 @@ mod tests { async fn failed_load_releases_persister_for_reconstruct() { let persister = Arc::new(FailingLoadPersister); let probe = Arc::clone(&persister); - let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); - let handler: Arc = Arc::new(NoopEventHandler); - - let manager = PlatformWalletManager::new(sdk, persister, handler); + let manager = make_manager(persister); let err = manager .load_from_persistor() @@ -500,67 +596,50 @@ mod tests { matches!(err, PlatformWalletError::PersisterLoad(_)), "load failure must surface as the typed PersisterLoad variant, got {err:?}" ); + assert_eq!( + Arc::strong_count(&probe), + MANAGER_PERSISTER_HOLDERS + 1, + "a failed load tears nothing down, so the manager's own references \ + must be exactly as they were before the call" + ); drop(manager); - // Asserted directly, never polled: the failure path awaits the - // adapter's `JoinHandle` inside `shutdown`, so the task's clone is - // already released before `drop` runs. Release on THIS path is - // synchronous, which is the stronger guarantee — a poll loop (or a - // `yield_now`, which cedes nothing to another worker) would only - // hide a regression into eventual release. assert_eq!( Arc::strong_count(&probe), 1, - "after a failed load + teardown nothing may still hold the persister" + "after a failed load and a drop nothing may still hold the persister" ); } - /// The `Drop` backstop alone (no `shutdown` first) must *eventually* release - /// the adapter's `Arc` clone. Unlike the graceful path this is - /// not synchronous: `Drop::drop` calls `abort()`, which only *requests* - /// cancellation — the runtime drops the aborted task (and its clone) at its - /// next poll. So the strong count is polled, not asserted immediately, which - /// is exactly the "eventual, not synchronous" contract the `Drop` impl's - /// doc-comment describes. This is the branch the graceful-path test above - /// never exercises (there `shutdown` has already taken the join handle, so - /// `Drop`'s `abort` sees `None`). + /// Dropping the manager without `shutdown` releases the persister + /// **synchronously**: every strong clone lives in the manager's own + /// fields, and the wallet-event adapter holds only a `Weak

`. + /// + /// The one bound: a batch commit in flight upgrades that weak reference + /// for the duration of its `store()`, so a drop racing a commit releases + /// when that commit returns (`an_in_flight_commit_holds_a_strong_persister_reference` + /// in `changeset::core_bridge`). The adapter is idle here, so release is + /// immediate. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn drop_backstop_eventually_releases_persister_without_shutdown() { + async fn dropping_manager_releases_persister_synchronously_when_adapter_idle() { let persister = Arc::new(FailingLoadPersister); let probe = Arc::clone(&persister); - let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); - let handler: Arc = Arc::new(NoopEventHandler); - - let manager = PlatformWalletManager::new(sdk, persister, handler); - // The adapter task spawned in `new()` holds a clone, so the count is - // above the probe before any teardown. - assert!( - Arc::strong_count(&probe) > 1, - "the spawned adapter task must hold an Arc clone" + let manager = make_manager(persister); + assert_eq!( + Arc::strong_count(&probe), + MANAGER_PERSISTER_HOLDERS + 1, + "the manager must hold its persister before the drop for this to \ + mean anything" ); - // Dirty drop: never call `shutdown`, so `Drop`'s `abort` is the only - // thing that can reclaim the adapter's clone. + // Dirty drop: `shutdown` is never called, so nothing joins the adapter. drop(manager); - // Release is eventual: poll until the aborted task is dropped by the - // runtime rather than asserting immediately. The wait must be a timed - // sleep, not `yield_now`: the aborted task is reclaimed by whichever - // worker thread owns it, and yielding this thread never forces that - // one to run — the whole budget can burn in microseconds while the - // clone is still live. Breaks on the first observation, so the 2s - // ceiling is only ever paid by a genuine regression. - let mut count = Arc::strong_count(&probe); - for _ in 0..2_000 { - if count == 1 { - break; - } - tokio::time::sleep(std::time::Duration::from_millis(1)).await; - count = Arc::strong_count(&probe); - } assert_eq!( - count, 1, - "the Drop backstop must eventually release the persister after aborting the adapter" + Arc::strong_count(&probe), + 1, + "dropping the manager must release the persister immediately — an \ + idle adapter holds no strong reference to await" ); } } diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index cc484031aca..2d0640c5464 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -492,7 +492,7 @@ impl PlatformWalletManager

{ let event_adapter_cancel = CancellationToken::new(); let event_adapter_join = spawn_wallet_event_adapter( Arc::clone(&wallet_manager), - Arc::clone(&persister), + Arc::downgrade(&persister), event_receiver, Arc::clone(&sync_fault), event_adapter_cancel.clone(), @@ -1043,22 +1043,17 @@ impl PlatformWalletManager

{ } } -/// Drop backstop for the wallet-event adapter task. +/// Drop backstop for the wallet-event adapter task: cancels its token and +/// aborts the task, which a dirty drop would otherwise merely detach. +/// +/// The persister is released here with the manager's own `Arc

` — the +/// adapter holds a `Weak

` — so a reconstruct on the same path cannot hit a +/// spurious `WalletStorageError::AlreadyOpen` (issue #4133). The one bound: a +/// batch commit in flight has upgraded that weak reference and keeps the +/// persister alive until its `store()` returns. /// -/// The graceful teardown is [`shutdown`](PlatformWalletManager::shutdown) -/// (cancel + await the join). A dirty drop that skips it would otherwise merely -/// detach the `JoinHandle`, leaving the adapter task running and holding its -/// `Arc

` clone — which keeps the persister "open" and turns a later re-open -/// on the same path into a spurious `WalletStorageError::AlreadyOpen` that -/// masks the real error (issue #4133). Cancelling the token and aborting the -/// task here starts that release — but note it is *eventual*, not synchronous: -/// `abort()` only requests cancellation, so the runtime drops the task (and its -/// `Arc

` clone) at the task's next poll, not inside this `drop`. In practice -/// the adapter loop parks on an `.await` almost every iteration, so the clone is -/// reclaimed promptly. Only the graceful -/// [`shutdown`](PlatformWalletManager::shutdown) path *guarantees* the reference -/// is gone before it returns (it awaits the join); this backstop guarantees -/// eventual reclamation, not synchronous. +/// Use [`shutdown`](PlatformWalletManager::shutdown) for a release that is +/// joined rather than aborted. impl Drop for PlatformWalletManager

{ fn drop(&mut self) { self.event_adapter_cancel.cancel(); diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index a9dbddba98c..40442c6d93b 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -650,7 +650,9 @@ impl crate::changeset::PlatformWalletPersistence for NoopTestPersister { } } -struct NoopTestEventHandler; +/// Event handler that ignores every event — for tests whose subject is not +/// the event fan-out. +pub(crate) struct NoopTestEventHandler; impl crate::events::EventHandler for NoopTestEventHandler {} impl crate::events::PlatformEventHandler for NoopTestEventHandler {} From 8f42f61d76a5586674bd32268c1a5a47fdd7b4b6 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:22:12 +0000 Subject: [PATCH 4/9] fix(platform-wallet): remove in-crate store retry; caller decides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changeset_slot.take()/flush retry idiom on store is unsafe against an unknown PlatformWalletPersistence implementation: re-issuing store with the same changeset double-merges every Vec-backed field (Merge for Vec is append-only), and a bare flush retry can't tell "committed by a concurrent writer" from "discarded by one" on a buffer shared per wallet id. Delete the idiom at its three call sites (registration store in wallet_lifecycle.rs, publish_scan_verdict in discovery.rs, record_identity_scan_cut_off in startup.rs): each is now a single store attempt that propagates or logs the typed, kind-classified PersistenceError. The two best-effort verdict-persist sites log at warn (not error). Retry survives only for load, an idempotent read the crate owns end to end. Shrink the retry module to manager::persist_retry (load-only, retry_transient_load, LOAD_RETRY_BACKOFF schedule, spawn_blocking per attempt), replacing wallet_lifecycle's retry_transient. Re-exported once from manager::mod; nothing outside manager imports it. Delete the "Transient-failure retry contract" paragraph on PlatformWalletPersistence::store and rewrite PersistenceErrorKind's docs to describe what each kind means to a caller, imposing no buffering obligation on the implementor. Rewrite the store-retry tests to assert a single store call and zero flush calls; add a transient-then-fatal load contract test and a paused-time backoff-schedule test. Refs #4365 — not fixed by this change: a busy database still aborts registration; the caller now receives a PersisterStore classified Transient and can retry itself. Co-Authored-By: Claude Sonnet 5 --- .../src/changeset/traits.rs | 26 +- .../rs-platform-wallet/src/manager/load.rs | 5 +- .../rs-platform-wallet/src/manager/mod.rs | 6 +- .../src/manager/persist_retry.rs | 73 ++++ .../rs-platform-wallet/src/manager/startup.rs | 21 +- .../src/manager/wallet_lifecycle.rs | 349 +++++++++--------- .../rs-platform-wallet/src/test_support.rs | 2 +- .../src/wallet/identity/network/discovery.rs | 28 +- 8 files changed, 279 insertions(+), 231 deletions(-) create mode 100644 packages/rs-platform-wallet/src/manager/persist_retry.rs diff --git a/packages/rs-platform-wallet/src/changeset/traits.rs b/packages/rs-platform-wallet/src/changeset/traits.rs index 16e8ac8d217..c2bce6521a0 100644 --- a/packages/rs-platform-wallet/src/changeset/traits.rs +++ b/packages/rs-platform-wallet/src/changeset/traits.rs @@ -40,20 +40,19 @@ pub struct ListedCoreTxid { /// kind MUST force every consumer match to update explicitly. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum PersistenceErrorKind { - /// The persister reports the write was not committed and the - /// buffered state is preserved (e.g. `SQLITE_BUSY`, `SQLITE_FULL`, - /// `SQLITE_IOERR`, `SQLITE_NOMEM`). Callers MAY retry with - /// exponential backoff. + /// The backend reports a retryable condition (e.g. `SQLITE_BUSY`, + /// `SQLITE_FULL`, `SQLITE_IOERR`, `SQLITE_NOMEM`). Whether and how + /// to retry is the caller's decision — this kind imposes no + /// obligation on the implementor beyond honest classification. Transient, /// The persister reports an unrecoverable failure (schema /// corruption, logic bug, I/O error not covered by the transient - /// class). Callers MUST NOT retry — the buffered changeset is - /// gone and the same call will keep failing. + /// class). Not retryable — the same call will keep failing. Fatal, /// SQL constraint / foreign-key / integrity violation. Distinct /// from `Fatal` so callers can distinguish "your data is wrong" /// (caller bug) from "the storage engine is unhappy" (operator / - /// infrastructure problem). Treated as fatal for retry purposes. + /// infrastructure problem). Not retryable. Constraint, } @@ -266,19 +265,6 @@ pub trait PlatformWalletPersistence: Send + Sync { /// wallet accessor (readers and writers) for its duration. Keep the /// per-call work bounded; if the backend does inline I/O (see the type /// doc), size it accordingly. - /// - /// # Transient-failure retry contract - /// - /// An implementation that returns a [`PersistenceError`] classified - /// [`PersistenceErrorKind::Transient`] from `store` **MUST** have already - /// buffered/preserved the changeset so that a subsequent bare - /// [`flush`](Self::flush) — with no re-supplied changeset — completes the - /// write (mirroring `flush`'s own transient contract). This is what lets a - /// caller retry a transient `store` failure via `flush` alone; re-calling - /// `store` with the same changeset would double-merge it. An - /// implementation that cannot preserve the changeset on failure MUST - /// classify that failure [`PersistenceErrorKind::Fatal`] (or - /// [`Constraint`](PersistenceErrorKind::Constraint)), never `Transient`. fn store( &self, wallet_id: WalletId, diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index d7b7c244ba4..0f9e48bb198 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -10,7 +10,7 @@ use crate::wallet::identity::IdentityManager; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use crate::wallet::PlatformWallet; -use super::{wallet_lifecycle::retry_transient, PlatformWalletManager}; +use super::{retry_transient_load, PlatformWalletManager}; impl PlatformWalletManager

{ /// Load the full [`ClientStartState`] from the configured persister @@ -30,7 +30,8 @@ impl PlatformWalletManager

{ /// /// [`WalletManager`]: key_wallet_manager::WalletManager pub async fn load_from_persistor(&self) -> Result<(), PlatformWalletError> { - let start_state = match retry_transient(|| self.persister.load()).await { + let persister = Arc::clone(&self.persister); + let start_state = match retry_transient_load(move || persister.load()).await { Ok(state) => state, Err(e) => { // Preserve the typed source chain (Debug carries the real diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index cc484031aca..e4e32d05764 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -5,16 +5,14 @@ pub mod dashpay_sync; pub mod dpns_sync; pub mod identity_sync; mod load; +mod persist_retry; pub mod platform_address_sync; #[cfg(feature = "shielded")] pub mod shielded_sync; pub mod startup; mod wallet_lifecycle; -/// Re-exported so the identity-scan verdict publishers under `wallet::` -/// retry on the same policy the registration path uses. The module itself -/// stays private — this is the only item it owes the rest of the crate. -pub(crate) use wallet_lifecycle::retry_transient; +pub(crate) use persist_retry::retry_transient_load; use std::sync::Arc; use std::time::Duration; diff --git a/packages/rs-platform-wallet/src/manager/persist_retry.rs b/packages/rs-platform-wallet/src/manager/persist_retry.rs new file mode 100644 index 00000000000..90f93cee2b4 --- /dev/null +++ b/packages/rs-platform-wallet/src/manager/persist_retry.rs @@ -0,0 +1,73 @@ +//! Bounded retry for transient persister *reads*. +//! +//! Only `load` is retried in-crate: it is idempotent and the crate owns both +//! ends. Writes are never retried here — a failed `store` propagates typed +//! and kind-classified, and the caller decides. +//! +//! Each attempt runs on the blocking pool; worst case per call is +//! `attempts × backend timeout + Σ backoff` (SQLite `busy_timeout` defaults +//! to 5 s). + +use std::sync::Arc; +use std::time::Duration; + +use crate::changeset::PersistenceError; + +/// Backoff before each retry of a transient `load` failure. Four total +/// attempts (the initial call plus one per entry). +pub(crate) const LOAD_RETRY_BACKOFF: [Duration; 3] = [ + Duration::from_millis(20), + Duration::from_millis(40), + Duration::from_millis(80), +]; + +/// Retry a synchronous persister `load` while it fails *transiently*, off +/// the async runtime, on the fixed [`LOAD_RETRY_BACKOFF`] schedule. +/// +/// `op` runs on the blocking pool once per attempt. A fatal error (or +/// success) returns immediately — a fatal failure never retries. A panic +/// inside `op` propagates to the caller; a cancelled attempt (runtime +/// shutting down) surfaces as a backend error instead of panicking. +pub(crate) async fn retry_transient_load(op: F) -> Result +where + F: Fn() -> Result + Send + Sync + 'static, + T: Send + 'static, +{ + let op = Arc::new(op); + for (attempt, backoff) in LOAD_RETRY_BACKOFF + .iter() + .map(Some) + .chain([None]) + .enumerate() + { + let call = Arc::clone(&op); + let result = match tokio::task::spawn_blocking(move || call()).await { + Ok(result) => result, + Err(join_err) if join_err.is_panic() => { + std::panic::resume_unwind(join_err.into_panic()) + } + Err(_cancelled) => { + return Err(PersistenceError::backend( + "runtime shutting down before load retry", + )) + } + }; + match result { + Ok(value) => return Ok(value), + Err(e) if e.is_transient() => { + let Some(backoff) = backoff else { + return Err(e); + }; + tracing::debug!( + attempt, + backoff_ms = backoff.as_millis() as u64, + error = %e, + "transient persister load failure — retrying" + ); + tokio::time::sleep(*backoff).await; + } + Err(e) => return Err(e), + } + } + unreachable!("the None-terminated schedule always returns on its final iteration") +} diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 187d269d433..6e314df5608 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -990,24 +990,15 @@ impl PlatformWalletManager identity_scan_state: Some(recorded), ..Default::default() }; - // Transient failures are ridden out on the registration path's bounded - // policy; the buffer preserves the changeset, so the retries re-drive - // it through `flush`. The final outcome is still swallowed — an - // abandoned scan must not turn a shutdown into an error. - let mut changeset_slot = Some(changeset); - let outcome = crate::manager::retry_transient(|| match changeset_slot.take() { - Some(cs) => self.persister.store(*wallet_id, cs), - None => self.persister.flush(*wallet_id), - }) - .await; - if let Err(e) = outcome { - tracing::error!( + // Single attempt, not retried — the outcome is logged and swallowed + // either way: an abandoned scan must not turn a shutdown into an error. + if let Err(e) = self.persister.store(*wallet_id, changeset) { + tracing::warn!( wallet_id = %hex::encode(wallet_id), transient = e.is_transient(), error = %e, - "abandoned scan's verdict could not be persisted after retries; the next \ - launch will take the warm shortcut over an identity set nothing proved \ - complete" + "abandoned scan's verdict could not be persisted; the next launch will take \ + the warm shortcut over an identity set nothing proved complete" ); } } diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 4cc2dce78da..b72a19d528c 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -12,7 +12,7 @@ use key_wallet::Network; #[cfg(any(feature = "bls", feature = "eddsa"))] use crate::changeset::ProviderKeyExtendedPubKey; use crate::changeset::{ - AccountAddressPoolEntry, AccountRegistrationEntry, PersistenceError, PlatformWalletChangeSet, + AccountAddressPoolEntry, AccountRegistrationEntry, PlatformWalletChangeSet, PlatformWalletPersistence, ProviderKeyAccountEntry, WalletMetadataEntry, }; use crate::error::PlatformWalletError; @@ -51,56 +51,6 @@ fn parse_mnemonic_any_language(phrase: &str) -> Result { Err("phrase does not match any supported BIP-39 wordlist") } -/// Total attempts (initial + retries) for a transient-classified persister -/// operation on the wallet-registration path. Small on purpose: this runs -/// inline while creating a wallet, not as a background job — a lock blip -/// should be ridden out in well under a second, and a genuinely stuck -/// backend must still surface promptly. -const PERSIST_RETRY_MAX_ATTEMPTS: u32 = 4; - -/// Backoff before the first retry; doubles on each subsequent attempt. -const PERSIST_RETRY_INITIAL_BACKOFF: std::time::Duration = std::time::Duration::from_millis(20); - -/// Ceiling for the doubling backoff so registration latency stays bounded -/// (worst case with the constants above: 20 + 40 + 80 ≈ 140 ms). -const PERSIST_RETRY_MAX_BACKOFF: std::time::Duration = std::time::Duration::from_millis(200); - -/// Retry a synchronous persister operation while it fails *transiently*, -/// using bounded exponential backoff. -/// -/// `op` runs once, then re-runs after a backoff sleep for as long as it -/// returns a [`PersistenceError`] whose -/// [`is_transient()`](PersistenceError::is_transient) is true, up to -/// [`PERSIST_RETRY_MAX_ATTEMPTS`]. A fatal error (or success) returns -/// immediately — a fatal failure never retries. The sleep is async so it -/// yields the Tokio worker instead of spinning the CPU, which is exactly -/// what the storage layer's `FlushRetryable` contract asks callers to do. -pub(crate) async fn retry_transient(mut op: F) -> Result -where - F: FnMut() -> Result, -{ - let mut backoff = PERSIST_RETRY_INITIAL_BACKOFF; - let mut attempt: u32 = 1; - loop { - match op() { - Ok(value) => return Ok(value), - Err(e) if e.is_transient() && attempt < PERSIST_RETRY_MAX_ATTEMPTS => { - tracing::debug!( - attempt, - max_attempts = PERSIST_RETRY_MAX_ATTEMPTS, - backoff_ms = backoff.as_millis() as u64, - error = %e, - "transient persister failure — backing off before retry" - ); - tokio::time::sleep(backoff).await; - backoff = backoff.saturating_mul(2).min(PERSIST_RETRY_MAX_BACKOFF); - attempt += 1; - } - Err(e) => return Err(e), - } - } -} - /// Test-only rendezvous fired inside [`PlatformWalletManager::remove_wallet_with_teardown`], /// between the inner-manager removal and the public-map removal. /// @@ -534,28 +484,15 @@ impl PlatformWalletManager

{ } } - // Persist the registration changeset, riding out a *transient* - // backend blip (e.g. `SQLITE_BUSY`) with bounded exponential backoff - // before giving up. On a transient `store` failure the persister - // restores the buffered changeset (its documented contract), so the - // retries re-drive that same write via `flush` — no re-merge, no - // double-count: the first attempt hands the changeset over, later - // attempts flush what the buffer preserved. A fatal error is not - // retried and fails fast. Either way the typed `PersistenceError` - // (and its transient/fatal classification) is preserved for the - // caller instead of being flattened to a string. - let mut changeset_slot = Some(registration_changeset); - let store_result = retry_transient(|| match changeset_slot.take() { - Some(cs) => self.persister.store(wallet_id, cs), - None => self.persister.flush(wallet_id), - }) - .await; - if let Err(e) = store_result { + // Persist the registration changeset. `store` is not retried here — + // the caller receives the typed, kind-classified `PersistenceError` + // (its transient/fatal classification preserved) and decides. + if let Err(e) = self.persister.store(wallet_id, registration_changeset) { tracing::error!( wallet_id = %hex::encode(wallet_id), transient = e.is_transient(), error = %e, - "failed to persist wallet registration changeset after retries" + "failed to persist wallet registration changeset" ); let mut wm = self.wallet_manager.write().await; if let Err(remove_err) = wm.remove_wallet(&wallet_id) { @@ -595,13 +532,12 @@ impl PlatformWalletManager

{ // earlier `insert_wallet`, absent from `self.wallets`), // poisoning every retry on `WalletAlreadyExists`. Roll back // before bailing — same shape as `manager::load`. - // Retry a transient load blip the same way as the store above; a - // load is an idempotent read, so re-reading after a lock blip is - // safe. `load_persisted()` returns the typed `PersistenceError` this - // rehydration boundary is built around, routed through the - // dedicated `PersisterLoad` variant so its retry classification - // survives to the caller. - let load_result = retry_transient(|| platform_wallet.load_persisted()).await; + // `load` is an idempotent read, so a transient blip is retried + // in-crate — unlike `store` above, which the caller decides on. + // Clone the per-wallet persister handle rather than moving + // `platform_wallet` itself, which is still needed below. + let load_persister = platform_wallet.persister().clone(); + let load_result = super::retry_transient_load(move || load_persister.load()).await; let crate::changeset::ClientStartState { mut platform_addresses, wallets: _, @@ -1356,22 +1292,28 @@ mod register_wallet_duplicate_tests { #[cfg(test)] mod persist_retry_tests { - //! Registration-path persistence: transient-error retry and typed - //! error classification across the boundary. + //! Registration-path persistence: single-attempt `store` with typed + //! error propagation, bounded `load` retry, and log-level policy. use std::sync::atomic::{AtomicUsize, Ordering}; - use std::sync::Arc; + use std::sync::{Arc, Mutex}; + use std::time::Duration; use key_wallet::mnemonic::{Language, Mnemonic}; use key_wallet::wallet::initialization::WalletAccountCreationOptions; use key_wallet::Network; + use tracing::field::{Field, Visit}; + use tracing::Level; + use tracing_subscriber::layer::{Context, SubscriberExt}; + use tracing_subscriber::Layer; use crate::changeset::{ ClientStartState, PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet, PlatformWalletPersistence, }; use crate::error::PlatformWalletError; - use crate::events::{EventHandler, PlatformEventHandler}; + use crate::events::PlatformEventHandler; + use crate::test_support::NoopTestEventHandler; use crate::wallet::platform_wallet::WalletId; use crate::PlatformWalletManager; @@ -1390,11 +1332,39 @@ mod persist_retry_tests { PersistenceError::backend_with_kind(PersistenceErrorKind::Fatal, "simulated corruption") } + /// Captures the level and message of every `tracing` event recorded + /// while installed as the default subscriber, so a test can assert a + /// call site's log level without inspecting stdout. + #[derive(Clone, Default)] + struct RecordedEvents(Arc>>); + + impl RecordedEvents { + fn entries(&self) -> Vec<(Level, String)> { + self.0.lock().expect("recorded events mutex").clone() + } + } + + impl Layer for RecordedEvents { + fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) { + struct MessageVisitor(String); + impl Visit for MessageVisitor { + fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { + if field.name() == "message" { + self.0 = format!("{value:?}"); + } + } + } + let mut visitor = MessageVisitor(String::new()); + event.record(&mut visitor); + self.0 + .lock() + .expect("recorded events mutex") + .push((*event.metadata().level(), visitor.0)); + } + } + /// Persister whose `store` / `flush` / `load` outcomes are scripted so - /// the registration retry path can be driven deterministically. Models - /// the real contract: a transient `store` failure preserves the - /// changeset in the buffer, so the retry re-drives the write through - /// `flush`. + /// the registration path can be driven deterministically. /// /// `store` counts registration and identity-scan-verdict writes /// separately. Registration ends with a best-effort `identity().sync()`, @@ -1408,21 +1378,23 @@ mod persist_retry_tests { registration_store_calls: AtomicUsize, /// Stores of the identity-scan verdict published by `identity().sync()`. scan_verdict_store_calls: AtomicUsize, + /// Never scripted to fail — every assertion here expects this to + /// stay 0, since a `store` failure is never retried through it. flush_calls: AtomicUsize, load_calls: AtomicUsize, - /// The first registration `store` fails transiently (buffer preserved - /// for retry). - store_transient_first: bool, - /// Every registration `store` fails fatally (must NOT retry). + /// The registration `store` call fails transiently. + store_transient: bool, + /// The registration `store` call fails fatally. store_fatal: bool, /// Number of leading scan-verdict `store` calls that fail transiently. scan_verdict_store_transient_failures: usize, - /// Number of leading `flush` calls that fail transiently before Ok. - flush_transient_failures: usize, - /// Number of leading `load` calls that fail transiently before Ok. + /// Number of leading `load` calls that fail transiently. load_transient_failures: usize, /// Every `load` fails fatally (must NOT retry). load_fatal: bool, + /// After `load_transient_failures` transient failures, fail fatally + /// instead of succeeding. + load_then_fatal: bool, } impl PlatformWalletPersistence for FaultyPersister { @@ -1450,11 +1422,11 @@ mod persist_retry_tests { // The registration half decides a combined round's outcome: its // failure aborts the whole registration, while a verdict's is // swallowed. - if let Some(n) = registration { + if registration.is_some() { if self.store_fatal { return Err(fatal()); } - if self.store_transient_first && n == 0 { + if self.store_transient { return Err(transient()); } } @@ -1467,12 +1439,8 @@ mod persist_retry_tests { } fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { - let n = self.flush_calls.fetch_add(1, Ordering::SeqCst); - if n < self.flush_transient_failures { - Err(transient()) - } else { - Ok(()) - } + self.flush_calls.fetch_add(1, Ordering::SeqCst); + Ok(()) } fn load(&self) -> Result { @@ -1483,19 +1451,18 @@ mod persist_retry_tests { if n < self.load_transient_failures { return Err(transient()); } + if self.load_then_fatal { + return Err(fatal()); + } Ok(ClientStartState::default()) } } - struct NoopEventHandler; - impl EventHandler for NoopEventHandler {} - impl PlatformEventHandler for NoopEventHandler {} - fn make_manager( persister: Arc, ) -> Arc> { let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); - let event_handler: Arc = Arc::new(NoopEventHandler); + let event_handler: Arc = Arc::new(NoopTestEventHandler); Arc::new(PlatformWalletManager::new(sdk, persister, event_handler)) } @@ -1521,31 +1488,42 @@ mod persist_retry_tests { .map(|_| ()) } - /// A transient `store` failure is ridden out — the persister - /// buffers the changeset, the retry re-drives it via `flush`, and - /// registration succeeds instead of aborting. + /// A transient `store` failure surfaces to the caller on the first + /// attempt — never retried via `flush` — and rolls the in-memory + /// registration back. #[tokio::test] - async fn transient_store_failure_is_retried_and_succeeds() { + async fn transient_store_failure_surfaces_as_persister_store_without_retry() { let persister = Arc::new(FaultyPersister { - store_transient_first: true, - flush_transient_failures: 1, // one transient flush, then Ok + store_transient: true, ..Default::default() }); let manager = make_manager(Arc::clone(&persister)); - register(&manager) + let err = register(&manager) .await - .expect("registration must succeed after retrying the transient store"); + .expect_err("a transient store failure must abort registration, not retry it"); - // store attempted once; flush retried twice (fail, then succeed). + match err { + PlatformWalletError::PersisterStore(pe) => assert!( + pe.is_transient(), + "a transient store failure must keep its transient classification" + ), + other => panic!("expected PersisterStore, got {other:?}"), + } assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); - assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 2); - // Registration ends in `identity().sync()`, whose scan publishes its - // verdict — the write that makes a partial scan survive a restart. + assert_eq!( + persister.flush_calls.load(Ordering::SeqCst), + 0, + "store is never retried via flush" + ); assert_eq!( persister.scan_verdict_store_calls.load(Ordering::SeqCst), - 1, - "a completed registration must publish the identity-scan verdict" + 0, + "an aborted registration never reaches the discovery scan" + ); + assert!( + manager.wallet_ids().await.is_empty(), + "a failed store must roll back the in-memory wallet insert" ); } @@ -1584,40 +1562,6 @@ mod persist_retry_tests { ); } - /// A store that stays transient exhausts the - /// bounded retry budget and returns the typed `PersisterStore` still - /// carrying transient classification (distinguishable from the fatal - /// case above). - #[tokio::test] - async fn persistently_transient_store_exhausts_bounded_retries() { - let persister = Arc::new(FaultyPersister { - store_transient_first: true, - flush_transient_failures: usize::MAX, // never recovers - ..Default::default() - }); - let manager = make_manager(Arc::clone(&persister)); - - let err = register(&manager) - .await - .expect_err("registration must fail once the retry budget is spent"); - - match err { - PlatformWalletError::PersisterStore(pe) => assert!( - pe.is_transient(), - "an exhausted-but-transient store must stay classified transient" - ), - other => panic!("expected PersisterStore, got {other:?}"), - } - // 1 store + 3 flush retries == 4 total attempts (the budget). - assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); - assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 3); - assert_eq!( - persister.scan_verdict_store_calls.load(Ordering::SeqCst), - 0, - "an aborted registration never reaches the discovery scan" - ); - } - /// A transient `load` blip during rehydration is retried (an /// idempotent read), so registration succeeds. #[tokio::test] @@ -1667,42 +1611,108 @@ mod persist_retry_tests { ); } - /// A transient failure persisting the identity-scan verdict is ridden out - /// on the same bounded policy the registration write uses, so a merely - /// busy backend does not cost the verdict its survival across a restart - /// (dashpay/platform#4365). + /// A load that turns fatal after riding out a transient blip surfaces + /// the fatal classification, not the earlier transient one, after + /// exactly the two calls that produced it. #[tokio::test] - async fn should_retry_a_transient_scan_verdict_store() { + async fn transient_then_fatal_load_surfaces_as_persister_load_fatal() { + let persister = Arc::new(FaultyPersister { + load_transient_failures: 1, + load_then_fatal: true, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("a load that turns fatal must abort registration"); + + match err { + PlatformWalletError::PersisterLoad(pe) => { + assert!( + !pe.is_transient(), + "the fatal outcome must win, not the earlier transient one" + ) + } + other => panic!("expected PersisterLoad, got {other:?}"), + } + assert_eq!(persister.load_calls.load(Ordering::SeqCst), 2); + } + + /// The load-retry schedule sleeps `[20, 40, 80]` ms across the 4 total + /// attempts it allows for an always-transient failure — driven with + /// virtual time so the test itself doesn't wait 140 ms. + #[tokio::test(start_paused = true)] + async fn transient_load_retry_follows_the_backoff_schedule() { + let calls = Arc::new(AtomicUsize::new(0)); + let op_calls = Arc::clone(&calls); + let start = tokio::time::Instant::now(); + + let result: Result<(), PersistenceError> = super::super::retry_transient_load(move || { + op_calls.fetch_add(1, Ordering::SeqCst); + Err(transient()) + }) + .await; + + assert!( + result.is_err(), + "an always-transient op exhausts the schedule" + ); + assert_eq!( + calls.load(Ordering::SeqCst), + 1 + super::super::persist_retry::LOAD_RETRY_BACKOFF.len(), + "one initial attempt plus one per scheduled backoff" + ); + let expected: Duration = super::super::persist_retry::LOAD_RETRY_BACKOFF.iter().sum(); + assert_eq!(tokio::time::Instant::now() - start, expected); + } + + /// A transient failure persisting the identity-scan verdict is logged + /// and swallowed on the first attempt — never retried — so a merely busy + /// backend costs the verdict its durability this launch + /// (dashpay/platform#4365) rather than failing the registration that + /// just succeeded. + #[tokio::test] + async fn transient_scan_verdict_store_failure_is_logged_not_retried() { let persister = Arc::new(FaultyPersister { scan_verdict_store_transient_failures: 1, ..Default::default() }); let manager = make_manager(Arc::clone(&persister)); + let recorder = RecordedEvents::default(); + let subscriber = tracing_subscriber::registry().with(recorder.clone()); + let _guard = tracing::subscriber::set_default(subscriber); + register(&manager) .await - .expect("a retried scan-verdict store must not disturb registration"); + .expect("a scan-verdict store failure must not disturb registration"); assert_eq!( persister.scan_verdict_store_calls.load(Ordering::SeqCst), 1, - "the verdict is handed over once; the retry re-drives it via flush" + "the verdict store is attempted once, never retried" ); - assert_eq!( - persister.flush_calls.load(Ordering::SeqCst), - 1, - "the transient verdict store must be retried through flush" + let events = recorder.entries(); + assert!( + events.iter().any(|(level, msg)| *level == Level::WARN + && msg.contains("identity-scan verdict could not be persisted")), + "an unpersisted scan verdict must be logged at warn: {events:?}" + ); + assert!( + !events + .iter() + .any(|(level, msg)| *level == Level::ERROR && msg.contains("identity-scan verdict")), + "a scan-verdict store failure must not log at error: {events:?}" ); } - /// Retrying the verdict never escalates into failing the scan that just - /// succeeded: once the budget is spent the outcome is logged and dropped, - /// and registration still returns Ok. + /// An unpersistable verdict never escalates into failing the registration + /// that just succeeded. #[tokio::test] - async fn should_not_fail_registration_when_the_scan_verdict_never_persists() { + async fn unpersistable_scan_verdict_does_not_fail_registration() { let persister = Arc::new(FaultyPersister { scan_verdict_store_transient_failures: usize::MAX, - flush_transient_failures: usize::MAX, ..Default::default() }); let manager = make_manager(Arc::clone(&persister)); @@ -1711,9 +1721,8 @@ mod persist_retry_tests { .await .expect("an unpersistable verdict must never fail wallet registration"); - // 1 store + 3 flush retries == the shared 4-attempt budget. assert_eq!(persister.scan_verdict_store_calls.load(Ordering::SeqCst), 1); - assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 3); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 0); } /// The typed persister-phase variants preserve retry diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index a9dbddba98c..180dae88c13 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -650,7 +650,7 @@ impl crate::changeset::PlatformWalletPersistence for NoopTestPersister { } } -struct NoopTestEventHandler; +pub(crate) struct NoopTestEventHandler; impl crate::events::EventHandler for NoopTestEventHandler {} impl crate::events::PlatformEventHandler for NoopTestEventHandler {} diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs index 50731a63871..abf4c1eeed1 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -643,11 +643,10 @@ impl IdentityWallet { /// survival across a restart, and it must not be allowed to fail the scan /// that just succeeded. /// - /// Best-effort is not one-shot, though. A backend that is merely busy - /// would otherwise cost the verdict its durability outright, which is the - /// gap the verdict exists to close (dashpay/platform#4365), so a transient - /// failure is ridden out on the same bounded policy the registration path - /// uses before the outcome is swallowed. + /// `store` is a single attempt — not retried here, per the caller-decides + /// persister-error policy — so a merely busy backend (dashpay/platform#4365) + /// costs the verdict its durability this launch; the outcome is logged and + /// swallowed either way. async fn publish_scan_verdict( &self, wallet_id: crate::wallet::platform_wallet::WalletId, @@ -677,23 +676,14 @@ impl IdentityWallet { identity_scan_state: Some(recorded), ..Default::default() }; - // On a transient `store` failure the persister keeps the changeset - // buffered (its documented contract), so the retries re-drive that - // same write through `flush` rather than handing it over twice. - let mut changeset_slot = Some(changeset); - let outcome = crate::manager::retry_transient(|| match changeset_slot.take() { - Some(cs) => self.persister.store(cs), - None => self.persister.flush(), - }) - .await; - if let Err(e) = outcome { - tracing::error!( + if let Err(e) = self.persister.store(changeset) { + tracing::warn!( wallet_id = %hex::encode(wallet_id), transient = e.is_transient(), error = %e, - "identity-scan verdict could not be persisted after retries; a partial scan \ - will not be retried after a restart, so an identity at an unanswered index \ - stays hidden until a later scan publishes a verdict that lands" + "identity-scan verdict could not be persisted; a partial scan will not be \ + retried after a restart, so an identity at an unanswered index stays hidden \ + until a later scan publishes a verdict that lands" ); } } From 45397aa85f2d6767e4ca8f1ff955a54c2bc5d5cd Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:29:25 +0000 Subject: [PATCH 5/9] feat(platform-wallet-ffi): carry the persister retry classification across the C ABI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wallet's PersisterLoad / PersisterStore / PersisterRestore variants each carry a typed PersistenceError whose kind says whether a retry can help. At the C boundary all three flattened to ErrorUnknown (99), so a host learned only "something went wrong" — the classification died exactly where it was needed, since the host is who decides whether to retry. Outbound (Rust -> host): six result codes, operation x kind, so neither half is lost. 49 ErrorPersisterLoadTransient retry later 50 ErrorPersisterLoadFatal do not retry (Fatal/Constraint/poisoned fold here — a read cannot hit a constraint, and none is retryable) 51 ErrorPersisterStoreTransient retry later; nothing was committed 52 ErrorPersisterStoreFatal do not retry 53 ErrorPersisterStoreConstraint fix the data 54 ErrorPersisterRestore wraps a wallet error; no kind to split Claimed from the registry frontier (49 at the time of the claim) and recorded there per its rule 2; the frontier moves to 55. Mirrored into Swift with all three edits rule 5 requires — raw case, init(ffi:) arm, typed case with its init(code:message:) arm — and into Kotlin as typed PlatformWallet errors whose isRetryable is true only for the two transients. Inbound (host -> Rust): PLATFORM_WALLET_PERSIST_RC_TRANSIENT (-2) and PLATFORM_WALLET_PERSIST_RC_CONSTRAINT (-3). A host holds the real storage handle and sees the real SQLITE_BUSY; these let it say so. Every other non-zero value keeps its Fatal reading, so hosts written against the plain 0 / non-zero contract are unaffected — both shipping handlers return only 0 / 1 / -1 today, and opting in is host work. FFIPersister::store previously aggregated its ~20 per-kind callbacks into a bool and reported one hardcoded Fatal, which would have made the inbound direction unreachable for the case that motivates it: a busy database during wallet registration (refs #4365). It now accumulates the most severe kind any callback reported — Fatal > Constraint > Transient, so one host-declared transient can never mask a fatal sibling. A transient verdict invites the caller to re-send the WHOLE changeset, and Merge for Vec appends rather than overwrites, so reporting one for a partially applied round would duplicate rows. A round therefore reports Transient only when PersistenceCapabilities::ATOMIC_CHANGESETS holds — the host's own attestation that "a changeset is committed or rolled back as one unit", which already requires both round brackets to be wired. Without it the verdict is downgraded to Fatal: losing a retry opportunity costs less than duplicating data. Single-call callbacks (loads, flush, the changeset-begin abort) have no such precondition — each either happened or did not. Both mechanisms are mutation-checked: disabling the atomicity gate turns transient_sentinel_is_withheld_when_the_round_is_not_atomic RED and nothing else; flattening persist_rc_kind to Fatal turns the three classification tests RED. Verified: platform-wallet-ffi clippy -D warnings clean and 358 tests green, including 13 new ones. The generated C header was inspected directly to confirm all six enum constants and both sentinels cross with the names and values the Swift mirror uses. Swift and Kotlin could not be compiled in the authoring environment (no toolchain); CI is their first execution, and each new host test carries a TODO saying so. Co-Authored-By: Claude Opus 5 --- .../dashsdk/errors/DashSdkError.kt | 82 +++ .../dashsdk/ffi/NativePersistenceBridge.kt | 12 + .../dashsdk/errors/DashSdkErrorTest.kt | 37 ++ .../ERROR_CODE_REGISTRY.md | 24 +- packages/rs-platform-wallet-ffi/src/error.rs | 281 +++++++++ .../rs-platform-wallet-ffi/src/persistence.rs | 588 +++++++++++++++--- packages/rs-platform-wallet/src/error.rs | 10 + .../PlatformWalletPersistenceHandler.swift | 15 + .../PlatformWallet/PlatformWalletResult.swift | 83 +++ .../ErrorHandlingTests.swift | 85 +++ 10 files changed, 1124 insertions(+), 93 deletions(-) diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt index 4bcb1ae8ea1..213a013be34 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt @@ -478,6 +478,77 @@ sealed class DashSdkError( cause, ) + /** + * `ErrorPersisterLoadTransient` (native code 49). Reading persisted + * wallet state failed on a store that reported the failure as + * retryable (`SQLITE_BUSY` and friends). Nothing was mutated — a + * load is a read — so this is retryable. The Android analog of + * Swift's `PlatformWalletError.persisterLoadTransient`. + */ + class PersisterLoadTransient(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) { + override val isRetryable: Boolean get() = true + } + + /** + * `ErrorPersisterLoadFatal` (native code 50). Reading persisted + * wallet state failed permanently — a corrupt or unreadable store, + * or a decode that will fail identically next time. Do NOT retry; + * the store needs repair or re-provisioning. Constraint-class read + * failures fold in here: a read cannot violate one, and neither is + * retryable. + */ + class PersisterLoadFatal(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) + + /** + * `ErrorPersisterStoreTransient` (native code 51). Writing wallet + * state failed on a busy or momentarily unavailable store. + * + * **Nothing was committed.** The native side only emits this when + * the persister guarantees the failed changeset round was rolled + * back whole, so re-issuing the operation cannot double-apply part + * of it — which is why this, uniquely among the store failures, is + * retryable. A wallet registration against a locked database + * produces it (dashpay/platform#4365); the retry decision is the + * host's, not the wallet's. + */ + class PersisterStoreTransient(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) { + override val isRetryable: Boolean get() = true + } + + /** + * `ErrorPersisterStoreFatal` (native code 52). Writing wallet state + * failed permanently — a full disk, a corrupt schema, an I/O error + * outside the retryable class. Do NOT retry; the wallet rolled its + * in-memory state back, so the operation may be re-attempted once + * the underlying fault is fixed. + */ + class PersisterStoreFatal(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) + + /** + * `ErrorPersisterStoreConstraint` (native code 53). A write violated + * a constraint / foreign key / integrity rule. Deliberately distinct + * from [PersisterStoreFatal]: this is "the data is wrong" (a caller + * or schema-mapping bug) rather than "the storage engine is unhappy" + * (an operator problem), and the two route to different people. Do + * NOT retry unchanged. + */ + class PersisterStoreConstraint(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) + + /** + * `ErrorPersisterRestore` (native code 54). Rehydrating persisted + * platform-address state into a freshly registered wallet failed. + * One code rather than three: it wraps a wallet error, not a store + * error, so it carries no retry classification. The wrapped error's + * rendering is in [message]. + */ + class PersisterRestore(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) + /** * Any other `PlatformWalletFFIResultCode` without a dedicated type. * Carries the platform-wallet [nativeCode] (already de-offset) and @@ -657,6 +728,17 @@ sealed class DashSdkError( // the deferred-token trio sits at 34-36 above. See // PlatformWalletFFIResultCode for the authoritative map.) 31 -> PlatformWallet.SigningKeyUnavailable(message, cause) + // Persister failures, operation x retry classification. These are + // exactly the "retry-semantics-bearing" codes this mapping exists + // for: only the two transients are retryable, and a constraint is + // kept apart from a fatal so hosts can route "your data is wrong" + // differently from "the storage engine is unhappy". + 49 -> PlatformWallet.PersisterLoadTransient(message, cause) + 50 -> PlatformWallet.PersisterLoadFatal(message, cause) + 51 -> PlatformWallet.PersisterStoreTransient(message, cause) + 52 -> PlatformWallet.PersisterStoreFatal(message, cause) + 53 -> PlatformWallet.PersisterStoreConstraint(message, cause) + 54 -> PlatformWallet.PersisterRestore(message, cause) else -> // @Deprecated fallback — see the code-6 arm; code 31 is the // real discriminator. diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt index 65c25e423d0..0135c005da7 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt @@ -32,6 +32,18 @@ package org.dashfoundation.dashsdk.ffi * [onWalletChangesetAccountBegin] / [onWalletChangesetAccountEnd]. * - Persist slots return `Int` (0 = ok, non-zero flips the round's * success flag so [onChangesetEnd] delivers the rollback). + * - A plain non-zero return means "failed, do not retry". A handler that + * can classify its own failure may instead return one of the two + * sentinels `platform-wallet-ffi` defines — + * `PLATFORM_WALLET_PERSIST_RC_TRANSIENT` (-2) for a retryable failure + * after which nothing was applied, or + * `PLATFORM_WALLET_PERSIST_RC_CONSTRAINT` (-3) for an integrity + * violation. The native side forwards the classification to its caller + * (surfacing as `DashSdkError.PlatformWallet.PersisterStoreTransient` + * and friends) and never retries on the handler's behalf. Returning the + * transient sentinel from a ROUND callback additionally asserts that a + * failed round is rolled back whole — see `PersistenceCallbacks` in + * `rs-platform-wallet-ffi/src/persistence.rs` for the exact contract. * - Load slots return flattened representations (`Array<...>` / typed * holder objects) that the trampoline re-packs into Rust-owned FFI * structs; Kotlin never allocates native memory. diff --git a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt index 0889e6ba126..154da2d65f2 100644 --- a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt +++ b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt @@ -217,6 +217,43 @@ class DashSdkErrorTest { ) } + // TODO: not compiled or run locally — no Kotlin/Gradle toolchain in the + // authoring environment. CI is the first execution of this test and of + // the `DashSdkError.PlatformWallet.Persister*` types it covers. + @Test + fun persisterCodes49Through54MapTypedWithCorrectRetryability() { + // The whole point of the persister block: a host must be able to tell + // a busy store from a corrupt one WITHOUT parsing the message. Before + // these codes all three wallet variants flattened to ErrorUnknown and + // the classification died at the boundary. + val cases = listOf( + Triple(49, DashSdkError.PlatformWallet.PersisterLoadTransient::class.java, true), + Triple(50, DashSdkError.PlatformWallet.PersisterLoadFatal::class.java, false), + Triple(51, DashSdkError.PlatformWallet.PersisterStoreTransient::class.java, true), + Triple(52, DashSdkError.PlatformWallet.PersisterStoreFatal::class.java, false), + Triple(53, DashSdkError.PlatformWallet.PersisterStoreConstraint::class.java, false), + Triple(54, DashSdkError.PlatformWallet.PersisterRestore::class.java, false), + ) + + for ((code, type, retryable) in cases) { + val message = "persistence backend error from code $code" + val mapped = DashSdkError.fromNative( + DashSDKException(DashSdkError.PLATFORM_WALLET_CODE_OFFSET + code, message), + ) + + assertTrue( + "code $code must not fall through to Generic", + type.isInstance(mapped), + ) + assertEquals(message, mapped.message) + assertEquals( + "code $code retryability is part of its contract", + retryable, + mapped.isRetryable, + ) + } + } + @Test fun assetLockInputConflictCode47MapsTyped() { // TERMINAL and RESERVED: no native path emits it today (that needs a diff --git a/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md b/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md index 181eefe62f2..f62f42bea4a 100644 --- a/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md +++ b/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md @@ -114,10 +114,11 @@ These are shipped ABI. Do not renumber. | 98 | `NotFound` | Sentinel — `Option` returned as an error | | 99 | `ErrorUnknown` | Sentinel — unmapped/flattened errors | -**Next allocatable integer: 49** — 27–48 are all claimed (27, 29, 31, 34–42 +**Next allocatable integer: 55** — 27–54 are all claimed (27, 29, 31, 34–42 and 46 merged; 43–45 proposed by active #4313 at head `0302b188ab`; 47 and 48 proposed by active #4356 (47 renumbered from 42, 48 from 43 — see their -rows below); 28, 30, +rows below); 49–54 proposed by active #4586 (the persister +operation × kind block); 28, 30, 32 and 33 reserved). **28, 30, 32 and 33 are RESERVED, not free**: 28 and 30 were vacated when the reservation trio moved to 34–36; 32 and 33 lapsed when their in-repo owners @@ -157,6 +158,12 @@ Fork-era numbers remain in the collision history, which is immutable record. | 33 | *(reserved — lapsed)* | — | Owner #4311 (successor of fork-era #4256) closed without merging; RESERVED, not reissuable | | 43 | `ErrorShieldedInviteAlreadyClaimed` | #4313 | In review — **ACTIVE; the former "on hold — holds no number" row is obsolete.** The branch revived and renumbered to the frontier exactly as that row prescribed. Lineage: fork-era #4204's 32 → 37 move, then 37 **taken by merged #4348** (`ErrorDocumentNotForSale = 37`, ABI since 2026-08-09), then 37 → 43 on revival. `ErrorShieldedInviteAlreadyClaimed = 43` at head `0302b188ab`. **Rule 5 is satisfied at that head**: Swift carries all three edits — the raw case, the `init(ffi:)` arm, and the typed `PlatformWalletError.shieldedInviteAlreadyClaimed` case with its arm in `init(code:message:)` (which `init(result:)` delegates to) — plus `errorDescription`; Kotlin has the typed terminal `PlatformWallet.ShieldedInviteAlreadyClaimed`, the `43 ->` arm in `fromPlatformWalletNative`, and a `DashSdkErrorTest` pin on 43. Swift's 43 mirror predates `0302b188ab` on the branch; the raw-value test pin for 43 is Kotlin's (Swift's `ErrorHandlingTests` pins 44 and 45 only) | | 44 | `ErrorShieldedScanBudgetExhausted` | #4313 | In review — claimed from the frontier; carries the #4306 scan-budget semantics (retryable — progress is checkpointed). **Rule 5 is satisfied as of `0302b188ab`, and was not before it.** At that commit's parent Kotlin already mirrored 44 (typed `ShieldedScanBudgetExhausted`, the `fromPlatformWalletNative` arm, a `DashSdkErrorTest` pin) while Swift carried none of rule 5's three edits, so 44 fell to `init(ffi:)`'s `default:` and lost its identity as `.errorUnknown` — one host typed, the other blind, the same failure shape as merged row 29's. `0302b188ab` adds the raw case, the `init(ffi:)` arm, the typed case with its `init(code:message:)` arm and `errorDescription`, and an `ErrorHandlingTests` pin of raw value 44 | +| 49 | `ErrorPersisterLoadTransient` | #4586 | Proposed — claimed from the frontier (48 at the time of the claim). Reading persisted state failed on a store that classified the failure retryable; nothing was mutated. First of a six-code `operation × kind` block: the wallet's `PersisterLoad` / `PersisterStore` / `PersisterRestore` variants each carry a typed `PersistenceError`, and before this block all three flattened to `ErrorUnknown` (99), so the retry classification died at the C boundary while the Rust API had carried it faithfully | +| 50 | `ErrorPersisterLoadFatal` | #4586 | Proposed — permanent read failure. `Fatal`, `Constraint` and `LockPoisoned` all fold here: a read cannot violate a constraint, and none of the three is retryable, so splitting them would spend codes hosts would handle identically | +| 51 | `ErrorPersisterStoreTransient` | #4586 | Proposed — the retryable write failure, and the code a wallet registration against a locked database produces (refs #4365). Emitted ONLY when the round was rolled back whole (host-attested `ATOMIC_CHANGESETS` plus both round brackets wired), because a caller acting on it re-sends the entire changeset and changeset vectors merge by appending | +| 52 | `ErrorPersisterStoreFatal` | #4586 | Proposed — permanent write failure, plus `LockPoisoned` (which carries no kind of its own) | +| 53 | `ErrorPersisterStoreConstraint` | #4586 | Proposed — integrity/foreign-key violation, kept apart from 52 so a host can route "your data is wrong" (caller or schema-mapping bug) differently from "the storage engine is unhappy" (operator/infrastructure). Not retryable either way | +| 54 | `ErrorPersisterRestore` | #4586 | Proposed — rehydrating persisted platform-address state into a freshly registered wallet failed. One code, not three: the variant wraps a `PlatformWalletError` rather than a `PersistenceError`, so there is no kind to split on | | 45 | `ErrorShieldedLifecycleBusy` | #4313 | In review — claimed from the frontier. A shielded lifecycle operation refused because teardown/clear holds the wallet (retryable — nothing consumed); the FFI remove path passes the refusal through as 45 instead of flattening it to `ErrorWalletOperation` (6). Same rule-5 history as 44: Kotlin mirrored 45 at the parent commit already; Swift's three edits and an `ErrorHandlingTests` pin of raw value 45 landed in `0302b188ab`. **Rule 5 is satisfied at that head** | **Code 31 left this table on 2026-08-04.** `ErrorSigningKeyUnavailable` sat here @@ -242,6 +249,15 @@ that was always required was made — onto the wrong integers. | 42 | `ErrorPersisterTransient` | #3968 | Contradicts **merged ABI** — 42 is #4451's `ErrorMasternodeWithdrawalUnconfirmed` (merged 2026-08-22). Not a paper conflict: since the 2026-08-25 base merges, #3968's **own tree** carries both variants — a hard E0081 in `error.rs` (`= 42` at both variants) and a duplicate raw value 42 in Swift's `PlatformWalletResultCode` — so the branch does not compile as-is | | 43 | `ErrorPersisterFatal` | #3968 | Collides with **active #4313**, whose recorded claim is `ErrorShieldedInviteAlreadyClaimed = 43` (see its proposed row). The silent shape: nothing conflicts textually and neither tree carries both variants, so only this file shows it | +**These two claims are now also redundant, not just misnumbered.** #4586's +49–54 block covers the same ground with finer granularity — it splits the +retry classification by *operation* as well as by kind, so +`ErrorPersisterTransient` / `ErrorPersisterFatal` have no meaning left that +49–52 do not already carry. If #3968 still needs codes it should adopt the +existing block rather than take two more integers from the frontier; a +second, coarser pair of persister codes would leave hosts with two ways to +learn the same thing and no rule for which one arrives. + PR `#3954`'s `ErrorShutdownIncomplete = 27` used to sit in this table. It is gone because that claim **won**: #3954 was closed and superseded by **#4268**, which merged 27 into `v4.2-dev` on 2026-08-02. See the collision history below. @@ -259,8 +275,8 @@ been challenged on day one. Both persister codes must now take fresh integers **from the frontier note above, which is the single canonical source; no number is copied here because any copy goes stale the moment another PR merges** (as the original "46+" copy in this paragraph did when #4465 shipped -46 — the frontier note reads 48 as of 2026-08-26, so a pair claimed today -takes 48 and 49, recording the claim there and here in the same PR). 26 and +46, and as a later "48 and 49" copy did when #4586 claimed the 49–54 +persister block — read the frontier note, do not copy it). 26 and 27 need nothing: they are the merged base's own values, correctly inherited, and rule 3 keeps them where they are. diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 63cb50a152c..7afaed863ba 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -1,4 +1,5 @@ use dpp::platform_value::string_encoding::Encoding; +use platform_wallet::changeset::PersistenceErrorKind; use platform_wallet::PlatformWalletError; use std::ffi::CString; use std::os::raw::c_char; @@ -293,6 +294,7 @@ pub enum PlatformWalletFFIResultCode { // 47 ErrorAssetLockInputConflict asset-lock double-spend detection // (terminal; RESERVED, no emitter yet) // 48 ErrorAssetLockInputContested asset-lock double-spend detection (provisional) + // 49-54 the persister operation x kind block below // // 38/39/40 carry a STABLE JSON detail object in the result `message` // instead of the typed `Display` rendering — see each variant's doc for @@ -504,6 +506,87 @@ pub enum PlatformWalletFFIResultCode { /// height, and says the verdict is provisional. ErrorAssetLockInputContested = 48, + // ----------------------------------------------------------------- + // Persister failures, operation x retry classification (49-54). + // + // The wallet's PersisterLoad / PersisterStore / PersisterRestore + // variants each carry a typed `PersistenceError`, whose `kind` says + // whether a retry can help. Before these codes all three flattened to + // ErrorUnknown (99) and the classification died at the boundary. One + // code per (operation, kind) pair keeps both halves: a host can tell a + // failed read from a failed write AND a retryable failure from a + // permanent one, without parsing the message. + // ----------------------------------------------------------------- + /// Maps `PlatformWalletError::PersisterLoad` whose `PersistenceError` + /// is classified [`Transient`](platform_wallet::changeset::PersistenceErrorKind::Transient) — + /// the store reported a retryable condition (`SQLITE_BUSY` and + /// friends) while reading persisted state. + /// + /// Host action: retry the operation later. Nothing was mutated — a + /// load is a read. + ErrorPersisterLoadTransient = 49, + + /// Maps `PlatformWalletError::PersisterLoad` for every other + /// classification: `Fatal`, `Constraint`, and a poisoned persister + /// lock. Reading persisted state failed permanently — a corrupt or + /// unreadable store, or a decode that will fail identically next + /// time. + /// + /// Host action: do NOT retry; inspect the message and repair or + /// re-provision the store. `Constraint` folds in here because a read + /// cannot violate one: if a store reports it on a load, it is a + /// backend defect, not a caller data error, and it is not retryable + /// either way. + ErrorPersisterLoadFatal = 50, + + /// Maps `PlatformWalletError::PersisterStore` whose `PersistenceError` + /// is classified + /// [`Transient`](platform_wallet::changeset::PersistenceErrorKind::Transient) — + /// a busy or momentarily unavailable store rejected the write. + /// + /// **Nothing was committed**: the wallet only reports this when the + /// persister guarantees the failed changeset round was rolled back + /// whole, so re-issuing the operation cannot double-apply part of it. + /// + /// Host action: retry the operation later. This is the code a wallet + /// registration against a locked database produces + /// (`dashpay/platform#4365`) — the operation aborted, and the retry + /// decision is the host's, not the wallet's. + ErrorPersisterStoreTransient = 51, + + /// Maps `PlatformWalletError::PersisterStore` classified `Fatal`, and + /// a poisoned persister lock. The write failed permanently — a full + /// disk, a corrupt schema, an I/O error outside the retryable class. + /// + /// Host action: do NOT retry; inspect the message. The wallet's + /// in-memory state was rolled back to before the operation, so the + /// host may re-attempt once the underlying fault is fixed. + ErrorPersisterStoreFatal = 52, + + /// Maps `PlatformWalletError::PersisterStore` classified + /// [`Constraint`](platform_wallet::changeset::PersistenceErrorKind::Constraint) — + /// a SQL constraint / foreign-key / integrity violation. Distinct + /// from [`Self::ErrorPersisterStoreFatal`] so a host can separate + /// "your data is wrong" from "the storage engine is unhappy": the + /// first is a caller or schema-mapping bug, the second an operator + /// or infrastructure problem, and they route to different people. + /// + /// Host action: do NOT retry unchanged — fix the data (or the + /// host-side schema mapping that produced it). + ErrorPersisterStoreConstraint = 53, + + /// Maps `PlatformWalletError::PersisterRestore`. Rehydrating persisted + /// platform-address state into a freshly registered wallet failed. + /// + /// One code, not three: this variant wraps a `PlatformWalletError` + /// rather than a `PersistenceError`, so it carries no retry + /// classification to split on. The wrapped error's `Display` reaches + /// the host in the message. + /// + /// Host action: inspect the message; the wallet was registered but its + /// persisted address state did not come back. + ErrorPersisterRestore = 54, + /// The named thing does not exist. /// /// Originally (and still mostly) the code for every `Option` returned as an @@ -884,6 +967,30 @@ impl From for PlatformWalletFFIResult { // rides `NotFound` rather than spending a fifth marketplace // code hosts would handle identically. PlatformWalletError::DpnsNameNotFound { .. } => PlatformWalletFFIResultCode::NotFound, + // The persister trio. Each carries the store's own retry + // classification, which is the whole reason these codes exist — + // flattened to ErrorUnknown a host could not tell a busy database + // from a corrupt one. `PersisterRestore` wraps a + // `PlatformWalletError` rather than a `PersistenceError`, so it + // has no kind to split on and takes a single code. + PlatformWalletError::PersisterLoad(source) => match source.kind() { + Some(PersistenceErrorKind::Transient) => { + PlatformWalletFFIResultCode::ErrorPersisterLoadTransient + } + _ => PlatformWalletFFIResultCode::ErrorPersisterLoadFatal, + }, + PlatformWalletError::PersisterStore(source) => match source.kind() { + Some(PersistenceErrorKind::Transient) => { + PlatformWalletFFIResultCode::ErrorPersisterStoreTransient + } + Some(PersistenceErrorKind::Constraint) => { + PlatformWalletFFIResultCode::ErrorPersisterStoreConstraint + } + _ => PlatformWalletFFIResultCode::ErrorPersisterStoreFatal, + }, + PlatformWalletError::PersisterRestore(..) => { + PlatformWalletFFIResultCode::ErrorPersisterRestore + } // NOTE: `MessageSigningFailed` is deliberately NOT matched, so it // falls to the `ErrorUnknown` catch-all below. Its causes are // internal invariant breaks (a public key that does not own the @@ -1970,6 +2077,180 @@ mod tests { assert_eq!(result.code, PlatformWalletFFIResultCode::ErrorUnknown); } + /// Build a `PersistenceError` of a chosen kind, the way a persister + /// backend (or the FFI persister's sentinel classification) would. + fn persistence_error( + kind: PersistenceErrorKind, + ) -> platform_wallet::changeset::PersistenceError { + platform_wallet::changeset::PersistenceError::backend_with_kind(kind, "database is locked") + } + + /// A transient read failure must reach the host as its own code, not + /// as the fatal sibling and not as `ErrorUnknown`: it is the one + /// persister outcome a host may retry unchanged. + #[test] + fn persister_load_transient_maps_to_code_49() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterLoadTransient as i32, + 49 + ); + + let result: PlatformWalletFFIResult = + PlatformWalletError::PersisterLoad(persistence_error(PersistenceErrorKind::Transient)) + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterLoadTransient + ); + assert!( + message_of(&result).contains("database is locked"), + "the typed Display must survive the conversion: {}", + message_of(&result) + ); + } + + /// Fatal, constraint and lock-poisoned reads all fold onto one code: + /// none of them is retryable, and a read cannot violate a constraint. + #[test] + fn persister_load_non_transient_kinds_fold_onto_code_50() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterLoadFatal as i32, + 50 + ); + + for error in [ + persistence_error(PersistenceErrorKind::Fatal), + persistence_error(PersistenceErrorKind::Constraint), + platform_wallet::changeset::PersistenceError::LockPoisoned, + ] { + let rendered = error.to_string(); + let result: PlatformWalletFFIResult = PlatformWalletError::PersisterLoad(error).into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterLoadFatal, + "every non-transient load failure folds onto 50: {rendered}" + ); + } + } + + /// The code the busy-database registration case produces + /// (`dashpay/platform#4365`). The wallet does not retry the write; the + /// host learns it may. + #[test] + fn persister_store_transient_maps_to_code_51() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterStoreTransient as i32, + 51 + ); + + let result: PlatformWalletFFIResult = + PlatformWalletError::PersisterStore(persistence_error(PersistenceErrorKind::Transient)) + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreTransient + ); + } + + /// A permanent write failure, and the lock-poisoned case that has no + /// kind of its own. + #[test] + fn persister_store_fatal_maps_to_code_52() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal as i32, + 52 + ); + + for error in [ + persistence_error(PersistenceErrorKind::Fatal), + platform_wallet::changeset::PersistenceError::LockPoisoned, + ] { + let result: PlatformWalletFFIResult = PlatformWalletError::PersisterStore(error).into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal + ); + } + } + + /// "Your data is wrong" must not arrive as "the storage engine is + /// unhappy": the two route to different people, so the constraint + /// kind keeps its own code rather than folding into 52. + #[test] + fn persister_store_constraint_maps_to_code_53() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterStoreConstraint as i32, + 53 + ); + + let result: PlatformWalletFFIResult = PlatformWalletError::PersisterStore( + persistence_error(PersistenceErrorKind::Constraint), + ) + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreConstraint + ); + assert_ne!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal + ); + } + + /// `PersisterRestore` wraps a `PlatformWalletError`, so it carries no + /// retry classification and takes a single code. The wrapped error's + /// rendering still has to reach the host. + #[test] + fn persister_restore_maps_to_code_54() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterRestore as i32, + 54 + ); + + let result: PlatformWalletFFIResult = PlatformWalletError::PersisterRestore(Box::new( + PlatformWalletError::WalletCreation("no address pool".to_string()), + )) + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterRestore + ); + assert!( + message_of(&result).contains("no address pool"), + "the wrapped error's Display is the only detail channel: {}", + message_of(&result) + ); + } + + /// The six persister codes must stay distinct from each other and from + /// every code already allocated: a host pins these integers, and a + /// collision silently re-labels a shipped meaning. + #[test] + fn persister_codes_occupy_their_own_slots() { + let persister = [ + PlatformWalletFFIResultCode::ErrorPersisterLoadTransient as i32, + PlatformWalletFFIResultCode::ErrorPersisterLoadFatal as i32, + PlatformWalletFFIResultCode::ErrorPersisterStoreTransient as i32, + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal as i32, + PlatformWalletFFIResultCode::ErrorPersisterStoreConstraint as i32, + PlatformWalletFFIResultCode::ErrorPersisterRestore as i32, + ]; + assert_eq!(persister, [49, 50, 51, 52, 53, 54]); + + // The highest code allocated before this block, and the sentinels + // the registry keeps terminal. + for taken in [ + PlatformWalletFFIResultCode::ErrorAssetLockInputContested as i32, + PlatformWalletFFIResultCode::NotFound as i32, + PlatformWalletFFIResultCode::ErrorUnknown as i32, + ] { + assert!( + !persister.contains(&taken), + "persister codes must not collide with {taken}" + ); + } + } + /// Read a result's message back as an owned `String`. Every /// marketplace assertion below inspects the message, and the raw /// `CStr::from_ptr` dance is noise at each site. diff --git a/packages/rs-platform-wallet-ffi/src/persistence.rs b/packages/rs-platform-wallet-ffi/src/persistence.rs index 3b540ffbf8d..de5ce7fdce9 100644 --- a/packages/rs-platform-wallet-ffi/src/persistence.rs +++ b/packages/rs-platform-wallet-ffi/src/persistence.rs @@ -25,9 +25,9 @@ use std::str::FromStr; use crate::types::{FFINetwork, Network}; use platform_wallet::changeset::{ AccountAddressPoolEntry, AccountRegistrationEntry, ClientStartState, ClientWalletStartState, - ListedCoreTxid, Merge, PersistenceCapabilities, PersistenceError, PlatformWalletChangeSet, - PlatformWalletPersistence, ProviderKeyAccountEntry, ProviderKeyExtendedPubKey, - PERSISTENCE_CAPABILITIES_VERSION, + ListedCoreTxid, Merge, PersistenceCapabilities, PersistenceError, PersistenceErrorKind, + PlatformWalletChangeSet, PlatformWalletPersistence, ProviderKeyAccountEntry, + ProviderKeyExtendedPubKey, PERSISTENCE_CAPABILITIES_VERSION, }; use platform_wallet::wallet::platform_wallet::WalletId; use platform_wallet::wallet::{PerAccountPlatformAddressState, PerWalletPlatformAddressState}; @@ -271,6 +271,102 @@ pub struct PersistenceExtensionCallbacks { pub load_tracked_masternodes_free: Option, } +/// Return value by which a persistence callback reports a **retryable** +/// failure after which nothing was applied (the host's own +/// `SQLITE_BUSY` / `SQLITE_FULL` / `SQLITE_IOERR` class). +/// +/// The host holds the real storage handle and is the only party that can +/// see the native status code, so this is the only channel through which +/// a retry classification reaches the Rust side. Failures reported this +/// way surface to the Rust caller as +/// [`PersistenceErrorKind::Transient`]; the caller — never this crate — +/// decides whether to retry. +/// +/// Unrelated to `rs-unified-sdk-jni`'s `RESOLVE_*` mnemonic-resolver +/// codes, which share these integers on a different callback family. +pub const PLATFORM_WALLET_PERSIST_RC_TRANSIENT: i32 = -2; + +/// Return value by which a persistence callback reports a constraint / +/// foreign-key / integrity violation, surfacing as +/// [`PersistenceErrorKind::Constraint`] — "the data is wrong", as +/// opposed to "the storage engine is unhappy". Not retryable. +/// +/// Same caveat about `rs-unified-sdk-jni`'s `RESOLVE_*` codes as +/// [`PLATFORM_WALLET_PERSIST_RC_TRANSIENT`]. +pub const PLATFORM_WALLET_PERSIST_RC_CONSTRAINT: i32 = -3; + +/// Classify a non-zero persistence-callback return value. +/// +/// Only the two documented sentinels carry a classification; every other +/// non-zero value keeps the conservative [`PersistenceErrorKind::Fatal`] +/// reading, so hosts written against the plain `0` / non-zero contract +/// behave exactly as before. +fn persist_rc_kind(rc: i32) -> PersistenceErrorKind { + match rc { + PLATFORM_WALLET_PERSIST_RC_TRANSIENT => PersistenceErrorKind::Transient, + PLATFORM_WALLET_PERSIST_RC_CONSTRAINT => PersistenceErrorKind::Constraint, + _ => PersistenceErrorKind::Fatal, + } +} + +/// Build the error for a non-zero return from a **single-call** callback +/// (a load, a flush, a standalone persist), carrying the host's own +/// classification of `rc`. +/// +/// Round-participating callbacks do not use this: their verdicts are +/// accumulated by [`RoundOutcome`] and classified once for the round. +fn persist_callback_error(rc: i32, message: impl Into) -> PersistenceError { + PersistenceError::backend_with_kind(persist_rc_kind(rc), message.into()) +} + +/// The verdict of one `store` round's callbacks. +/// +/// A round fails if any callback failed, and reports the MOST SEVERE kind +/// any of them returned (`Fatal` > `Constraint` > `Transient`) so one +/// host-declared transient can never mask a fatal sibling. +#[derive(Default)] +struct RoundOutcome { + worst: Option, +} + +impl RoundOutcome { + /// Record a non-zero return `rc` from a round callback. + fn record(&mut self, rc: i32) { + self.escalate(persist_rc_kind(rc)); + } + + /// Record a Rust-side failure to encode a payload. Never transient: + /// the same changeset will not encode on a later attempt. + fn record_fatal(&mut self) { + self.escalate(PersistenceErrorKind::Fatal); + } + + fn escalate(&mut self, kind: PersistenceErrorKind) { + let severity = |kind| match kind { + PersistenceErrorKind::Transient => 0, + PersistenceErrorKind::Constraint => 1, + PersistenceErrorKind::Fatal => 2, + }; + if self + .worst + .is_none_or(|worst| severity(kind) > severity(worst)) + { + self.worst = Some(kind); + } + } + + /// `true` while every callback so far has returned success. This is + /// what `on_changeset_end_fn` receives as its `success` argument. + fn is_success(&self) -> bool { + self.worst.is_none() + } + + /// The kind to report for the round, or `None` if it succeeded. + fn failure_kind(&self) -> Option { + self.worst + } +} + /// C callback vtable for wallet persistence. /// /// General-purpose notifications (`on_store_fn`, `on_flush_fn`) plus @@ -292,6 +388,43 @@ pub struct PersistenceExtensionCallbacks { /// callback returns and the lock is released.) Keep the work bounded; the call /// blocks every other wallet accessor while it runs. Mirrors the Rust-side /// `PlatformWalletPersistence::store` reentrancy contract. +/// +/// # Reporting a failure's retry classification +/// +/// Every callback below returns `0` for success and non-zero for failure. +/// A plain non-zero value means "failed, do not retry" — the conservative +/// reading Rust has always applied, so a host written against the original +/// contract needs no change. +/// +/// A host that can classify its own failure (it holds the storage handle +/// and sees the native status code) may instead return one of two +/// sentinels, which reach the Rust caller as a typed retry classification: +/// +/// * [`PLATFORM_WALLET_PERSIST_RC_TRANSIENT`] — a retryable failure after +/// which **nothing was applied** (`SQLITE_BUSY` and friends). +/// * [`PLATFORM_WALLET_PERSIST_RC_CONSTRAINT`] — a constraint / integrity +/// violation: the data is wrong, and retrying it unchanged will not help. +/// +/// The Rust side never retries on a host's behalf; it forwards the +/// classification and the caller decides. +/// +/// ## What a transient verdict promises, and who must honour it +/// +/// A caller acting on "transient" re-issues the WHOLE changeset, and +/// changeset vectors merge by appending. So a transient verdict is only +/// meaningful when the failed round left nothing applied — which is exactly +/// what `ATOMIC_CHANGESETS` attests ("a changeset is committed or rolled +/// back as one unit"), and what [`Self::on_changeset_end_fn`] with +/// `success = false` exists to drive. +/// +/// A `store` round therefore reports a transient failure ONLY when both +/// round brackets are wired and the host declared `ATOMIC_CHANGESETS`; +/// otherwise Rust downgrades it to fatal, because a partially applied round +/// re-sent in full would duplicate rows rather than replace them. **A host +/// that does not roll a failed round back must not return the transient +/// sentinel from a round callback.** Single-call callbacks (loads, flush, +/// the changeset-begin abort) have no such precondition: each is one +/// operation that either happened or did not. #[repr(C)] #[allow(clippy::type_complexity)] pub struct PersistenceCallbacks { @@ -1138,6 +1271,32 @@ impl FFIPersister { } } + /// Narrow a `store` round's failure kind to what the caller may safely + /// act on. + /// + /// [`PersistenceErrorKind::Transient`] invites the caller to re-send the + /// whole changeset, which is only sound when a failed round left nothing + /// applied — `Merge for Vec` appends, so re-sending a partially + /// applied round doubles its vector fields instead of overwriting them. + /// A round is all-or-nothing exactly when + /// [`PersistenceCapabilities::ATOMIC_CHANGESETS`] holds, which requires + /// both round brackets to be wired AND the host to have attested + /// "committed or rolled back as one unit". Without that attestation a + /// transient verdict is downgraded to `Fatal`: losing a retry + /// opportunity costs less than duplicating data. + /// + /// `Constraint` and `Fatal` pass through unchanged — neither invites a + /// retry, so neither depends on the round being atomic. + fn reportable_round_kind(&self, reported: PersistenceErrorKind) -> PersistenceErrorKind { + let atomic = self + .persistence_capabilities() + .contains(PersistenceCapabilities::ATOMIC_CHANGESETS); + match reported { + PersistenceErrorKind::Transient if !atomic => PersistenceErrorKind::Fatal, + kind => kind, + } + } + /// Compute the callback contracts that are structurally complete in this /// vtable. This mask is only an upper bound: the host must separately attest /// the semantics it actually implements. @@ -1276,9 +1435,10 @@ impl PlatformWalletPersistence for FFIPersister { ) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_persist_tracked_masternodes_fn returned error code {rc}" - ))); + return Err(persist_callback_error( + rc, + format!("on_persist_tracked_masternodes_fn returned error code {rc}"), + )); } Ok(()) } @@ -1315,9 +1475,10 @@ impl PlatformWalletPersistence for FFIPersister { ) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_tracked_masternodes_fn returned error code {rc}" - ))); + return Err(persist_callback_error( + rc, + format!("on_load_tracked_masternodes_fn returned error code {rc}"), + )); } let mut out = Vec::with_capacity(count); if !rows_ptr.is_null() && count > 0 { @@ -1394,19 +1555,21 @@ impl PlatformWalletPersistence for FFIPersister { // A nonzero begin means the client could NOT open its // transaction. Proceeding would run every per-kind // callback against no batch and then fire an unmatched - // `end`. Treat it as fatal: close the Rust-side round - // (so `in_round` doesn't wedge) and fail now, before any - // per-kind write. (Unlike the previous advisory-log - // behavior, the round is aborted so no state advances - // against an unopened batch.) + // `end`. Close the Rust-side round (so `in_round` doesn't + // wedge) and fail now, before any per-kind write — nothing + // was applied, so the host's own classification of `result` + // is reported as-is. let _ = round.end_round(); - return Err(PersistenceError::backend(format!( - "changeset-begin callback returned error code {result}; \ + return Err(persist_callback_error( + result, + format!( + "changeset-begin callback returned error code {result}; \ round aborted before any write" - ))); + ), + )); } } - let mut round_success = true; + let mut outcome = RoundOutcome::default(); // Wallet-registration metadata. Fires at most once per round // (registration emits the entry; subsequent rounds carry @@ -1427,7 +1590,7 @@ impl PlatformWalletPersistence for FFIPersister { "Wallet metadata persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1464,12 +1627,12 @@ impl PlatformWalletPersistence for FFIPersister { "Account registrations persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } Err(e) => { eprintln!("Failed to encode account registration specs: {}", e); - round_success = false; + outcome.record_fatal(); } } } @@ -1501,12 +1664,12 @@ impl PlatformWalletPersistence for FFIPersister { "Account address pools persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } Err(e) => { eprintln!("Failed to encode account address pool entries: {}", e); - round_success = false; + outcome.record_fatal(); } } } @@ -1541,7 +1704,7 @@ impl PlatformWalletPersistence for FFIPersister { "Address balance persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1581,12 +1744,12 @@ impl PlatformWalletPersistence for FFIPersister { "Derived-address persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } Err(e) => { eprintln!("Failed to encode derived address pool entries: {}", e); - round_success = false; + outcome.record_fatal(); } } } @@ -1624,12 +1787,12 @@ impl PlatformWalletPersistence for FFIPersister { "Marked-used address persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } Err(e) => { eprintln!("Failed to encode marked-used address pool entries: {}", e); - round_success = false; + outcome.record_fatal(); } } } @@ -1644,7 +1807,7 @@ impl PlatformWalletPersistence for FFIPersister { "Wallet changeset persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1688,7 +1851,7 @@ impl PlatformWalletPersistence for FFIPersister { "Identity changeset persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1727,7 +1890,7 @@ impl PlatformWalletPersistence for FFIPersister { "DashPay payment persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1773,7 +1936,7 @@ impl PlatformWalletPersistence for FFIPersister { "Identity keys changeset persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1824,7 +1987,7 @@ impl PlatformWalletPersistence for FFIPersister { "Token balance persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1869,7 +2032,7 @@ impl PlatformWalletPersistence for FFIPersister { "Asset lock persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1910,7 +2073,7 @@ impl PlatformWalletPersistence for FFIPersister { "Invitation persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1960,7 +2123,7 @@ impl PlatformWalletPersistence for FFIPersister { "DPNS name state persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2114,7 +2277,7 @@ impl PlatformWalletPersistence for FFIPersister { "Contact persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2141,7 +2304,7 @@ impl PlatformWalletPersistence for FFIPersister { "Sync state persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2194,7 +2357,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded notes persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2226,7 +2389,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded nullifier-spent persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2286,7 +2449,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded outgoing-notes persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2316,7 +2479,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded synced-index persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2362,7 +2525,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded viewing-key persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2476,7 +2639,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded activity persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } // `rows` and `entries` drop here, after the callback // has copied everything it needs. @@ -2485,13 +2648,19 @@ impl PlatformWalletPersistence for FFIPersister { } } - // Close the round. Clients use this to commit (if - // `round_success == true`) or roll back (otherwise) the + // Close the round. Clients use this to commit (if the round + // succeeded) or roll back (otherwise) the // staged writes accumulated across the per-kind callbacks // above, making the whole store() call a single atomic // transaction from their perspective. if let Some(cb) = self.callbacks.on_changeset_end_fn { - let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr(), round_success) }; + let result = unsafe { + cb( + self.callbacks.context, + wallet_id.as_ptr(), + outcome.is_success(), + ) + }; if result != 0 { eprintln!("Changeset-end callback returned error code {}", result); // The end callback is where the client COMMITS the round (e.g. @@ -2503,7 +2672,7 @@ impl PlatformWalletPersistence for FFIPersister { // cleared drain entries, ignored-sender deltas) against data // that was dropped. Otherwise the failure is silent and the // dropped writes resurface or are lost with no signal. - round_success = false; + outcome.record(result); } } @@ -2516,8 +2685,9 @@ impl PlatformWalletPersistence for FFIPersister { // which cannot happen here since `begin_round` succeeded above.) round.end_round()?; - if !round_success { - return Err(PersistenceError::backend( + if let Some(kind) = outcome.failure_kind() { + return Err(PersistenceError::backend_with_kind( + self.reportable_round_kind(kind), "one or more persistence callbacks failed; changeset was rolled back", )); } @@ -2545,9 +2715,14 @@ impl PlatformWalletPersistence for FFIPersister { ignored" ); } else { - return Err(PersistenceError::backend(format!( - "Persistence store callback returned error code {result}" - ))); + // This branch runs only without an end callback, so the + // per-kind writes already landed individually and the + // round is not all-or-nothing — `reportable_round_kind` + // withholds a retryable verdict accordingly. + return Err(PersistenceError::backend_with_kind( + self.reportable_round_kind(persist_rc_kind(result)), + format!("Persistence store callback returned error code {result}"), + )); } } } @@ -2556,19 +2731,16 @@ impl PlatformWalletPersistence for FFIPersister { } fn flush(&self, wallet_id: WalletId) -> Result<(), PersistenceError> { - // TODO: deferred — FFI callback failures are classified as - // `Fatal` (no transient-retry signal across the C ABI), and - // trailing-byte validation on decoded FFI payloads is not yet - // applied here. Both are tracked for a follow-up; no behavior - // change in this change. + // TODO: deferred — trailing-byte validation on decoded FFI + // payloads is not yet applied here. // Notify caller. if let Some(cb) = self.callbacks.on_flush_fn { let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr()) }; if result != 0 { - return Err(PersistenceError::backend(format!( - "Persistence flush callback returned error code {}", - result - ))); + return Err(persist_callback_error( + result, + format!("Persistence flush callback returned error code {}", result), + )); } } @@ -2594,10 +2766,10 @@ impl PlatformWalletPersistence for FFIPersister { let mut count: usize = 0; let rc = unsafe { load_cb(self.callbacks.context, &mut entries_ptr, &mut count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_wallet_list_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!("on_load_wallet_list_fn returned error code {}", rc), + )); } let _guard = LoadGuard { context: self.callbacks.context, @@ -2685,10 +2857,10 @@ impl PlatformWalletPersistence for FFIPersister { let rc = unsafe { load_notes(self.callbacks.context, &mut notes_ptr, &mut notes_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_notes_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!("on_load_shielded_notes_fn returned error code {}", rc), + )); } struct NotesGuard { context: *mut c_void, @@ -2750,10 +2922,13 @@ impl PlatformWalletPersistence for FFIPersister { let rc = unsafe { load_outgoing(self.callbacks.context, &mut out_ptr, &mut out_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_outgoing_notes_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!( + "on_load_shielded_outgoing_notes_fn returned error code {}", + rc + ), + )); } struct OutgoingGuard { context: *mut c_void, @@ -2814,10 +2989,10 @@ impl PlatformWalletPersistence for FFIPersister { load_states(self.callbacks.context, &mut states_ptr, &mut states_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_sync_states_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!("on_load_shielded_sync_states_fn returned error code {}", rc), + )); } struct StatesGuard { context: *mut c_void, @@ -2872,10 +3047,10 @@ impl PlatformWalletPersistence for FFIPersister { let rc = unsafe { load_activity(self.callbacks.context, &mut act_ptr, &mut act_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_activity_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!("on_load_shielded_activity_fn returned error code {}", rc), + )); } struct ActivityGuard { context: *mut c_void, @@ -3035,10 +3210,13 @@ impl PlatformWalletPersistence for FFIPersister { load_viewing_keys(self.callbacks.context, &mut vk_ptr, &mut vk_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_viewing_keys_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!( + "on_load_shielded_viewing_keys_fn returned error code {}", + rc + ), + )); } struct ViewingKeysGuard { context: *mut c_void, @@ -3348,9 +3526,10 @@ impl PlatformWalletPersistence for FFIPersister { // free a buffer the host still owns on the failure path, which is a // double free for any host that cleans up its own failed allocation. if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_list_wallet_core_txids_fn returned non-zero status {rc}" - ))); + return Err(persist_callback_error( + rc, + format!("on_list_wallet_core_txids_fn returned non-zero status {rc}"), + )); } // Success: ownership is ours now, and every return below must release @@ -8211,6 +8390,237 @@ mod tests { unsafe { free_contact_requests_ffi(rows.as_mut_ptr(), rows.len()) }; } + // ── Inbound retry classification from host return codes ── + + /// Metadata callback returning the host's "retryable, nothing applied" + /// sentinel. + extern "C" fn transient_metadata( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _network: FFINetwork, + _wallet_group_id: *const u8, + _birth_height: u32, + ) -> i32 { + PLATFORM_WALLET_PERSIST_RC_TRANSIENT + } + + /// Metadata callback returning the host's constraint sentinel. + extern "C" fn constraint_metadata( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _network: FFINetwork, + _wallet_group_id: *const u8, + _birth_height: u32, + ) -> i32 { + PLATFORM_WALLET_PERSIST_RC_CONSTRAINT + } + + /// Metadata callback returning a plain non-zero value, the way every + /// host written against the original contract does. + extern "C" fn unclassified_metadata( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _network: FFINetwork, + _wallet_group_id: *const u8, + _birth_height: u32, + ) -> i32 { + 7 + } + + extern "C" fn ok_begin(_ctx: *mut TestCVoid, _wallet_id: *const u8) -> i32 { + 0 + } + + extern "C" fn ok_end(_ctx: *mut TestCVoid, _wallet_id: *const u8, _success: bool) -> i32 { + 0 + } + + /// A changeset carrying exactly one payload: the metadata entry, whose + /// callback each test below drives. + fn metadata_changeset() -> PlatformWalletChangeSet { + PlatformWalletChangeSet { + wallet_metadata: Some(platform_wallet::changeset::WalletMetadataEntry { + network: Network::Testnet, + wallet_group_id: [1u8; 32], + birth_height: 1, + }), + ..PlatformWalletChangeSet::default() + } + } + + /// Build a persister whose metadata callback is `metadata`, optionally + /// bracketing rounds and attesting atomicity. + fn store_failing_persister( + metadata: unsafe extern "C" fn( + *mut TestCVoid, + *const u8, + FFINetwork, + *const u8, + u32, + ) -> i32, + bracketed: bool, + capabilities: PersistenceCapabilities, + ) -> FFIPersister { + let callbacks = PersistenceCallbacks { + on_persist_wallet_metadata_fn: Some(metadata), + on_changeset_begin_fn: bracketed.then_some(ok_begin as _), + on_changeset_end_fn: bracketed.then_some(ok_end as _), + ..PersistenceCallbacks::default() + }; + FFIPersister::new_with_persistence_capabilities(callbacks, capabilities) + } + + fn store_error_kind(persister: &FFIPersister) -> Option { + persister + .store([1u8; 32], metadata_changeset()) + .expect_err("the metadata callback fails every round here") + .kind() + } + + /// The point of the whole inbound direction: a host that sees its own + /// `SQLITE_BUSY` can say so, and the caller receives a retryable + /// classification instead of the blanket `Fatal` every FFI failure used + /// to collapse into. + #[test] + fn transient_sentinel_reaches_the_caller_from_an_atomic_round() { + let persister = store_failing_persister( + transient_metadata, + true, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Transient) + ); + } + + /// A transient verdict tells the caller to re-send the WHOLE changeset, + /// and changeset vectors merge by appending. Without an all-or-nothing + /// round the failed round may have applied part of itself, so re-sending + /// would duplicate rows — the verdict is withheld and reported fatal. + #[test] + fn transient_sentinel_is_withheld_when_the_round_is_not_atomic() { + // Brackets wired, but the host never attested atomicity. + let unattested = + store_failing_persister(transient_metadata, true, PersistenceCapabilities::NONE); + assert_eq!( + store_error_kind(&unattested), + Some(PersistenceErrorKind::Fatal), + "an unattested round must not invite a retry" + ); + + // Attested, but with no round brackets to roll anything back — the + // structural half of ATOMIC_CHANGESETS is missing. + let unbracketed = store_failing_persister( + transient_metadata, + false, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&unbracketed), + Some(PersistenceErrorKind::Fatal), + "an attestation without begin/end brackets must not invite a retry" + ); + } + + /// `Constraint` never invites a retry, so it does not depend on the + /// round being atomic and passes through either way. + #[test] + fn constraint_sentinel_survives_whether_or_not_the_round_is_atomic() { + for (bracketed, capabilities) in [ + (true, PersistenceCapabilities::ATOMIC_CHANGESETS), + (false, PersistenceCapabilities::NONE), + ] { + let persister = store_failing_persister(constraint_metadata, bracketed, capabilities); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Constraint) + ); + } + } + + /// Back-compatibility: a host that returns a plain non-zero value keeps + /// the conservative reading it has always had. + #[test] + fn unclassified_non_zero_return_stays_fatal() { + let persister = store_failing_persister( + unclassified_metadata, + true, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Fatal) + ); + } + + /// One transient callback must never soften a fatal sibling: the round + /// reports the most severe kind any callback returned. Here the commit + /// itself fails unclassified after a per-kind callback reported + /// transient — the round is fatal. + #[test] + fn a_fatal_callback_masks_a_transient_sibling() { + extern "C" fn fatal_end( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _success: bool, + ) -> i32 { + 7 + } + + let callbacks = PersistenceCallbacks { + on_persist_wallet_metadata_fn: Some(transient_metadata), + on_changeset_begin_fn: Some(ok_begin), + on_changeset_end_fn: Some(fatal_end), + ..PersistenceCallbacks::default() + }; + let persister = FFIPersister::new_with_persistence_capabilities( + callbacks, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Fatal), + "a transient sibling must not soften the round's fatal verdict" + ); + } + + /// A load is one call that either happened or did not, so it carries the + /// host's classification with no atomicity precondition. + #[test] + fn transient_sentinel_reaches_the_caller_from_a_load() { + extern "C" fn transient_load( + _ctx: *mut TestCVoid, + _out_entries: *mut *const WalletRestoreEntryFFI, + _out_count: *mut usize, + ) -> i32 { + PLATFORM_WALLET_PERSIST_RC_TRANSIENT + } + + let callbacks = PersistenceCallbacks { + on_load_wallet_list_fn: Some(transient_load), + ..PersistenceCallbacks::default() + }; + let err = FFIPersister::new(callbacks) + .load() + .expect_err("the load callback fails"); + assert_eq!(err.kind(), Some(PersistenceErrorKind::Transient)); + } + + /// The two sentinels must stay off the values a host already returns — + /// success, and the plain failure codes the shipping hosts use. + #[test] + fn sentinels_do_not_collide_with_established_return_values() { + for taken in [0, 1, -1] { + assert_ne!(PLATFORM_WALLET_PERSIST_RC_TRANSIENT, taken); + assert_ne!(PLATFORM_WALLET_PERSIST_RC_CONSTRAINT, taken); + } + assert_ne!( + PLATFORM_WALLET_PERSIST_RC_TRANSIENT, + PLATFORM_WALLET_PERSIST_RC_CONSTRAINT + ); + } + // ── Round serialization + defensive state machine (dashpay/platform#4069) ── use std::os::raw::c_void as TestCVoid; diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index a4c24264a2d..5f322dd875e 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -21,6 +21,11 @@ pub enum PlatformWalletError { /// transient backend hiccup (e.g. `SQLITE_BUSY`) stays distinguishable /// from a permanent failure and can be retried. /// + /// FFI hosts receive the classification too: the boundary maps this + /// variant to result code 49 when the kind is `Transient` and 50 + /// otherwise, so the distinction survives the C ABI rather than + /// flattening to "unknown error". + /// /// [`PersistenceError`]: crate::changeset::PersistenceError /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind #[error("failed to load persisted client state: {0}")] @@ -35,6 +40,11 @@ pub enum PlatformWalletError { /// registration write from a failed rehydration read; not `#[from]` /// because that conversion is already claimed by [`Self::PersisterLoad`]. /// + /// FFI hosts receive the classification too: the boundary maps this + /// variant to result code 51 (`Transient`), 53 (`Constraint`) or 52 + /// (everything else), so a host can tell a busy store from a rejected + /// row from a broken one without parsing the message. + /// /// [`PersistenceError`]: crate::changeset::PersistenceError /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind #[error("failed to persist wallet registration changeset: {0}")] diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift index 4c0baa95899..4c7d569807a 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift @@ -32,6 +32,21 @@ struct LiveModelFetcher: ModelFetching { /// Allocated as a class so its pointer can be passed as the opaque `context` /// to the Rust persistence callbacks. Must be retained for the lifetime of /// the `PlatformWalletManager`. +/// +/// Callback return values: `0` succeeds and any non-zero value fails. A +/// plain non-zero failure means "do not retry". A callback that can +/// classify its own failure may instead return +/// `PLATFORM_WALLET_PERSIST_RC_TRANSIENT` (-2) for a retryable failure +/// after which nothing was applied, or +/// `PLATFORM_WALLET_PERSIST_RC_CONSTRAINT` (-3) for an integrity +/// violation; Rust forwards the classification to its caller (as +/// `PlatformWalletError.persisterStoreTransient` and friends) and never +/// retries on this handler's behalf. Returning the transient sentinel from +/// a callback inside a changeset round additionally asserts that a failed +/// round is rolled back whole — which this handler does, via +/// `endChangeset(success: false)`. The handlers below currently return +/// only `0` / `1` / `-1`, so they always read as fatal; opting in is a +/// per-callback change. // All mutable state (`backgroundContext`, caches) is confined to `serialQueue` // — the handler's de-facto actor — so it is safe to hand to a `@Sendable` // closure (e.g. the off-main `serialQueue.async` backfill dispatch). diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift index 5b07dcbda8c..b5a35e02ce1 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift @@ -216,6 +216,36 @@ public enum PlatformWalletResultCode: Int32, Sendable { /// the Rust-side scan cannot see conflicts whose spender was already /// pruned. case errorAssetLockInputContested = 48 + /// Reading persisted wallet state failed on a store that reported the + /// failure as retryable (`SQLITE_BUSY` and friends). Nothing was + /// mutated — a load is a read. Retry later. + case errorPersisterLoadTransient = 49 + /// Reading persisted wallet state failed permanently — a corrupt or + /// unreadable store, or a decode that will fail identically next time. + /// Do NOT retry; inspect the message. Constraint-class failures fold in + /// here too: a read cannot violate one, and neither is retryable. + case errorPersisterLoadFatal = 50 + /// Writing wallet state failed on a busy or momentarily unavailable + /// store. **Nothing was committed** — the SDK only reports this when the + /// persister rolls a failed changeset round back whole, so re-issuing the + /// operation cannot double-apply part of it. Retry later. + case errorPersisterStoreTransient = 51 + /// Writing wallet state failed permanently — a full disk, a corrupt + /// schema, an I/O error outside the retryable class. Do NOT retry; + /// inspect the message. The wallet rolled its in-memory state back, so + /// the operation may be re-attempted once the fault is fixed. + case errorPersisterStoreFatal = 52 + /// A write violated a constraint / foreign key / integrity rule. + /// Deliberately distinct from `errorPersisterStoreFatal`: this is "the + /// data is wrong" (a caller or schema-mapping bug) rather than "the + /// storage engine is unhappy" (an operator problem), and the two route + /// to different people. Do NOT retry unchanged; fix the data. + case errorPersisterStoreConstraint = 53 + /// Rehydrating persisted platform-address state into a freshly + /// registered wallet failed. One code rather than three: it wraps a + /// wallet error, not a store error, so it carries no retry + /// classification. The wrapped error's rendering is in the message. + case errorPersisterRestore = 54 /// The named thing does not exist. Besides the handle/lookup failures this /// has always covered, BOTH deferred-send paths report the /// wallet-was-REMOVED case here. @@ -323,6 +353,18 @@ public enum PlatformWalletResultCode: Int32, Sendable { self = .errorAssetLockInputConflict case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_ASSET_LOCK_INPUT_CONTESTED: self = .errorAssetLockInputContested + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_TRANSIENT: + self = .errorPersisterLoadTransient + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_FATAL: + self = .errorPersisterLoadFatal + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_TRANSIENT: + self = .errorPersisterStoreTransient + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_FATAL: + self = .errorPersisterStoreFatal + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_CONSTRAINT: + self = .errorPersisterStoreConstraint + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_RESTORE: + self = .errorPersisterRestore case PLATFORM_WALLET_FFI_RESULT_CODE_NOT_FOUND: self = .notFound case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_UNKNOWN: @@ -554,6 +596,28 @@ public enum PlatformWalletError: LocalizedError { /// confirmed spender is this wallet's own transaction, so the value /// behind the contested input lives on in it. case assetLockInputContested(String) + /// Reading persisted wallet state failed on a store that classified the + /// failure as retryable. Nothing was mutated — retry later. One of the + /// two retryable persister cases, alongside `persisterStoreTransient`. + case persisterLoadTransient(String) + /// Reading persisted wallet state failed permanently. Do NOT retry; + /// the store needs repair or re-provisioning. + case persisterLoadFatal(String) + /// Writing wallet state failed on a busy store, with the whole changeset + /// round rolled back — nothing was committed, so re-issuing the + /// operation is safe. Retry later. + case persisterStoreTransient(String) + /// Writing wallet state failed permanently. Do NOT retry until the + /// underlying fault is fixed; the wallet rolled its in-memory state back. + case persisterStoreFatal(String) + /// A write violated a constraint / integrity rule — the data is wrong, + /// as opposed to the storage engine being unhappy. Do NOT retry + /// unchanged. + case persisterStoreConstraint(String) + /// Rehydrating persisted platform-address state into a newly registered + /// wallet failed. Carries no retry classification: it wraps a wallet + /// error rather than a store error. + case persisterRestore(String) /// The named thing does not exist. For the deferred payment calls this is /// the wallet-was-REMOVED case: the token's wallet (or the wallet a payment /// was just signed against) is no longer registered in the manager, so there @@ -592,6 +656,9 @@ public enum PlatformWalletError: LocalizedError { .notForSale(let m), .assetLockInputConflict(let m), .assetLockInputContested(let m), + .persisterLoadTransient(let m), .persisterLoadFatal(let m), + .persisterStoreTransient(let m), .persisterStoreFatal(let m), + .persisterStoreConstraint(let m), .persisterRestore(let m), .notFound(let m), .unknown(let m): return m // The three value-carrying marketplace rejections compose their @@ -718,6 +785,22 @@ public enum PlatformWalletError: LocalizedError { self = .assetLockInputConflict(detail) case .errorAssetLockInputContested: self = .assetLockInputContested(detail) + // The persister codes carry the wallet's typed `Display` as the + // message. Which operation failed and whether a retry can help is + // the CODE's meaning, not the string's — branch on the case, never + // on the text. + case .errorPersisterLoadTransient: + self = .persisterLoadTransient(detail) + case .errorPersisterLoadFatal: + self = .persisterLoadFatal(detail) + case .errorPersisterStoreTransient: + self = .persisterStoreTransient(detail) + case .errorPersisterStoreFatal: + self = .persisterStoreFatal(detail) + case .errorPersisterStoreConstraint: + self = .persisterStoreConstraint(detail) + case .errorPersisterRestore: + self = .persisterRestore(detail) case .notFound: self = .notFound(detail) case .errorUnknown: self = .unknown(detail) } diff --git a/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift b/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift index 9b375b852d4..1ce670da97e 100644 --- a/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift +++ b/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift @@ -73,6 +73,91 @@ final class ErrorHandlingTests: XCTestCase { XCTAssertEqual(error.errorDescription, rendered) } + // TODO: not compiled or run locally — no Swift toolchain in the + // authoring environment. CI is the first execution of these two tests + // and of the `PlatformWalletResult.swift` cases they cover. + /// The persister block (49-54). Each code must decode from its + /// generated C constant, keep its own raw value, and reach a typed + /// `PlatformWalletError` case — the three edits a new code needs on + /// this side. Without the `init(ffi:)` arm a code compiles fine and + /// silently degrades to `.errorUnknown`, losing the classification the + /// Rust side went to the trouble of carrying across. + func testPersisterFFIResultMappings() { + let mappings: [(PlatformWalletFFIResultCode, PlatformWalletResultCode, Int32)] = [ + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_TRANSIENT, + .errorPersisterLoadTransient, 49 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_FATAL, + .errorPersisterLoadFatal, 50 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_TRANSIENT, + .errorPersisterStoreTransient, 51 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_FATAL, + .errorPersisterStoreFatal, 52 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_CONSTRAINT, + .errorPersisterStoreConstraint, 53 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_RESTORE, + .errorPersisterRestore, 54 + ), + ] + + for (ffi, expected, rawValue) in mappings { + XCTAssertEqual(PlatformWalletResultCode(ffi: ffi), expected) + XCTAssertNotEqual(PlatformWalletResultCode(ffi: ffi), .errorUnknown) + // Hand-mirrored ABI, not a derived ordinal. + XCTAssertEqual(expected.rawValue, rawValue) + } + } + + /// The two retryable persister codes must arrive as their own typed + /// cases carrying the Rust message, and must not be confused with the + /// non-retryable siblings that share an operation. + func testPersisterTypedErrorCases() { + let busy = "failed to persist wallet registration changeset: " + + "persistence backend error (Transient): database is locked" + guard case .persisterStoreTransient(let storeMessage) = PlatformWalletError( + code: .errorPersisterStoreTransient, + message: busy + ) else { + return XCTFail("expected typed persisterStoreTransient error") + } + XCTAssertEqual(storeMessage, busy) + + guard case .persisterStoreConstraint = PlatformWalletError( + code: .errorPersisterStoreConstraint, + message: "constraint failed" + ) else { + return XCTFail("a constraint violation must not read as a transient or fatal store") + } + + guard case .persisterLoadTransient = PlatformWalletError( + code: .errorPersisterLoadTransient, + message: busy + ) else { + return XCTFail("expected typed persisterLoadTransient error") + } + + guard case .persisterRestore(let restoreMessage) = PlatformWalletError( + code: .errorPersisterRestore, + message: "failed to restore persisted platform-address state: wallet is locked" + ) else { + return XCTFail("expected typed persisterRestore error") + } + XCTAssertEqual( + restoreMessage, + "failed to restore persisted platform-address state: wallet is locked" + ) + } + func testPlatformWalletNotFoundFFIResultMapping() { // Code 98 (the blanket Option→result miss) stays typed inside the // wallet-error family — the mapping Kotlin now converges on From 9bdf7c62df874640f3b72925917d9195e8578701 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:37:59 +0000 Subject: [PATCH 6/9] test(platform-wallet): route scan-verdict log capture through a global once-installed subscriber --- .../src/manager/wallet_lifecycle.rs | 73 +++++++++++++++++-- 1 file changed, 65 insertions(+), 8 deletions(-) diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index b72a19d528c..757e93cde70 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -1295,8 +1295,9 @@ mod persist_retry_tests { //! Registration-path persistence: single-attempt `store` with typed //! error propagation, bounded `load` retry, and log-level policy. + use std::cell::RefCell; use std::sync::atomic::{AtomicUsize, Ordering}; - use std::sync::{Arc, Mutex}; + use std::sync::{Arc, Mutex, OnceLock}; use std::time::Duration; use key_wallet::mnemonic::{Language, Mnemonic}; @@ -1333,8 +1334,8 @@ mod persist_retry_tests { } /// Captures the level and message of every `tracing` event recorded - /// while installed as the default subscriber, so a test can assert a - /// call site's log level without inspecting stdout. + /// while registered as the active recorder for the current thread (see + /// [`RecordingGuard`]). #[derive(Clone, Default)] struct RecordedEvents(Arc>>); @@ -1342,10 +1343,8 @@ mod persist_retry_tests { fn entries(&self) -> Vec<(Level, String)> { self.0.lock().expect("recorded events mutex").clone() } - } - impl Layer for RecordedEvents { - fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) { + fn record(&self, event: &tracing::Event<'_>) { struct MessageVisitor(String); impl Visit for MessageVisitor { fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { @@ -1363,6 +1362,65 @@ mod persist_retry_tests { } } + thread_local! { + /// The [`RecordedEvents`] a test on THIS thread wants routed to it, + /// if any. Set/cleared only by [`RecordingGuard`]. + static ACTIVE_RECORDER: RefCell> = const { RefCell::new(None) }; + } + + /// Routes every event to whichever [`RecordedEvents`] is registered for + /// the emitting thread, via [`ACTIVE_RECORDER`]. Installed as the + /// process-wide default exactly once — never per-test. + /// + /// A per-test `tracing::subscriber::set_default` swap is flaky under + /// `cargo test`'s parallel harness: tracing's per-callsite `Interest` + /// cache is process-global, and a concurrently-running test's own + /// swap/drop can race the interest rebuild your swap triggers, so the + /// event silently never reaches your subscriber even though dispatch + /// itself stays correctly on your own thread (confirmed: the emitting + /// thread ID matched the installing thread ID on a captured failure). + /// Installing the routing subscriber once, before any callsite is ever + /// hit, sidesteps the race — routing then happens through an ordinary + /// thread-local this code owns, not through tracing's default-swap + /// machinery. + struct RecorderRouter; + + impl Layer for RecorderRouter { + fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) { + ACTIVE_RECORDER.with(|slot| { + if let Some(recorder) = slot.borrow().as_ref() { + recorder.record(event); + } + }); + } + } + + static GLOBAL_ROUTER_INIT: OnceLock<()> = OnceLock::new(); + + /// Scopes [`ACTIVE_RECORDER`] to `recorder` for the current thread, for + /// the guard's lifetime. + struct RecordingGuard; + + impl RecordingGuard { + fn install(recorder: RecordedEvents) -> Self { + GLOBAL_ROUTER_INIT.get_or_init(|| { + let subscriber = tracing_subscriber::registry().with(RecorderRouter); + // Another thread may have already won this race; either + // way, the routing subscriber is the process-wide default + // by the time `get_or_init` returns to any caller. + let _ = tracing::subscriber::set_global_default(subscriber); + }); + ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = Some(recorder)); + Self + } + } + + impl Drop for RecordingGuard { + fn drop(&mut self) { + ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = None); + } + } + /// Persister whose `store` / `flush` / `load` outcomes are scripted so /// the registration path can be driven deterministically. /// @@ -1681,8 +1739,7 @@ mod persist_retry_tests { let manager = make_manager(Arc::clone(&persister)); let recorder = RecordedEvents::default(); - let subscriber = tracing_subscriber::registry().with(recorder.clone()); - let _guard = tracing::subscriber::set_default(subscriber); + let _guard = RecordingGuard::install(recorder.clone()); register(&manager) .await From 20f90f9b7338d0742f7241ea8f477582bfc98221 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:58:45 +0000 Subject: [PATCH 7/9] refactor(platform-wallet)!: name the persister operation at construction, degrade reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING: `impl From for PlatformWalletError` no longer exists. Downstream `?` / `.into()` sites choose the variant explicitly. A blanket `From` cannot be correct, because the conversion is undecidable from the value: a `PersistenceError` does not record whether a load, a store or a flush produced it, so the impl has to guess one variant for every operation. It guessed `PersisterLoad`, and the two downstream consumers that used it are both stores — a failed contact-request or dashpay-payment write surfaces to the user as "failed to load persisted client state". That is not a mistake those call sites made; it is the only thing the conversion could have done. Replaced with three named constructors — `from_load_failure`, `from_store_failure`, `from_restore_failure` (which boxes internally, so callers no longer write `Box::new`) — whose shared rustdoc carries the rationale. The operation is named where it is known, which is the call site. Variants and payloads stay `pub`: this is a construction-side seam, and downstream pattern-matching (including the FFI crate's own code mapping) is unaffected. Every in-tree construction site moved to the constructors. Read-path policy, previously inconsistent between three call sites that all want the same thing: `WalletPersister::get_core_tx_record_or_transient_miss` is now the one place that decides what a failed tx-record read means. A transient failure is indistinguishable in outcome from "not readable right now" and every caller already retries a miss on its next pass, so it collapses to `Ok(None)` at debug level. A permanent failure stays an `Err`. Poll loops (`wait_for_chain_lock`, `wait_for_proof`) no longer abort on a permanent read failure. This read is a FALLBACK for records the in-memory map evicted; the live SPV stream can still deliver the record and end the wait, so aborting converted a degraded read path into a failed operation. The failure is reported once per wait rather than once per iteration — a broken backend inside a loop would otherwise flood the log with the same line, and the wait stays bounded by its own finality timeout. The dashpay reconstruction sweep now surfaces permanent read failures instead of folding them into "incomplete, retry next sweep" alongside transient ones. A permanently unreadable store made it re-run the entire sweep on every sync, indefinitely, and never say why. The confirmation sweep already had the right policy; both now share the helper. Both behaviour changes were confirmed RED first: the reconstruction-sweep test failed on its assertion against the old code, and the poll-loop tests could not have passed under the aborting contract. The removal of the blanket conversion was verified by compiling a probe that requires it, rather than inferred from the absence of errors. Verified (`--no-deps` required: an unrelated unused import in rs-drive fails any dependency-wide `-D warnings` run): clippy --no-deps -p platform-wallet -p platform-wallet-ffi --all-targets -D warnings exit 0 test -p platform-wallet -p platform-wallet-ffi exit 0 platform-wallet 952 + 9, platform-wallet-ffi 326 + 26 + 6 + 4, 0 failed Co-Authored-By: Claude Opus 5 --- packages/rs-platform-wallet-ffi/src/error.rs | 26 +-- packages/rs-platform-wallet/src/error.rs | 46 +++++- .../rs-platform-wallet/src/manager/load.rs | 2 +- .../src/manager/wallet_lifecycle.rs | 37 ++++- .../src/wallet/asset_lock/sync/proof.rs | 148 ++++++++++++++---- .../src/wallet/identity/network/payments.rs | 110 ++++++++++--- .../src/wallet/persister.rs | 28 ++++ 7 files changed, 325 insertions(+), 72 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 7afaed863ba..c0e31a84eec 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -2095,9 +2095,10 @@ mod tests { 49 ); - let result: PlatformWalletFFIResult = - PlatformWalletError::PersisterLoad(persistence_error(PersistenceErrorKind::Transient)) - .into(); + let result: PlatformWalletFFIResult = PlatformWalletError::from_load_failure( + persistence_error(PersistenceErrorKind::Transient), + ) + .into(); assert_eq!( result.code, PlatformWalletFFIResultCode::ErrorPersisterLoadTransient @@ -2124,7 +2125,8 @@ mod tests { platform_wallet::changeset::PersistenceError::LockPoisoned, ] { let rendered = error.to_string(); - let result: PlatformWalletFFIResult = PlatformWalletError::PersisterLoad(error).into(); + let result: PlatformWalletFFIResult = + PlatformWalletError::from_load_failure(error).into(); assert_eq!( result.code, PlatformWalletFFIResultCode::ErrorPersisterLoadFatal, @@ -2143,9 +2145,10 @@ mod tests { 51 ); - let result: PlatformWalletFFIResult = - PlatformWalletError::PersisterStore(persistence_error(PersistenceErrorKind::Transient)) - .into(); + let result: PlatformWalletFFIResult = PlatformWalletError::from_store_failure( + persistence_error(PersistenceErrorKind::Transient), + ) + .into(); assert_eq!( result.code, PlatformWalletFFIResultCode::ErrorPersisterStoreTransient @@ -2165,7 +2168,8 @@ mod tests { persistence_error(PersistenceErrorKind::Fatal), platform_wallet::changeset::PersistenceError::LockPoisoned, ] { - let result: PlatformWalletFFIResult = PlatformWalletError::PersisterStore(error).into(); + let result: PlatformWalletFFIResult = + PlatformWalletError::from_store_failure(error).into(); assert_eq!( result.code, PlatformWalletFFIResultCode::ErrorPersisterStoreFatal @@ -2183,7 +2187,7 @@ mod tests { 53 ); - let result: PlatformWalletFFIResult = PlatformWalletError::PersisterStore( + let result: PlatformWalletFFIResult = PlatformWalletError::from_store_failure( persistence_error(PersistenceErrorKind::Constraint), ) .into(); @@ -2207,9 +2211,9 @@ mod tests { 54 ); - let result: PlatformWalletFFIResult = PlatformWalletError::PersisterRestore(Box::new( + let result: PlatformWalletFFIResult = PlatformWalletError::from_restore_failure( PlatformWalletError::WalletCreation("no address pool".to_string()), - )) + ) .into(); assert_eq!( result.code, diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 5f322dd875e..70a3d40f7ce 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -28,8 +28,10 @@ pub enum PlatformWalletError { /// /// [`PersistenceError`]: crate::changeset::PersistenceError /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind + /// + /// Construct with [`Self::from_load_failure`]. #[error("failed to load persisted client state: {0}")] - PersisterLoad(#[from] crate::changeset::PersistenceError), + PersisterLoad(#[source] crate::changeset::PersistenceError), /// The persister failed to store the wallet-registration changeset. /// Like [`Self::PersisterLoad`], it carries the typed @@ -37,8 +39,7 @@ pub enum PlatformWalletError { /// / [`PersistenceErrorKind`]) survives the boundary — a transient /// `SQLITE_BUSY` stays distinguishable from a permanent failure. /// Distinct from [`Self::PersisterLoad`] so callers can tell a failed - /// registration write from a failed rehydration read; not `#[from]` - /// because that conversion is already claimed by [`Self::PersisterLoad`]. + /// registration write from a failed rehydration read. /// /// FFI hosts receive the classification too: the boundary maps this /// variant to result code 51 (`Transient`), 53 (`Constraint`) or 52 @@ -47,6 +48,8 @@ pub enum PlatformWalletError { /// /// [`PersistenceError`]: crate::changeset::PersistenceError /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind + /// + /// Construct with [`Self::from_store_failure`]. #[error("failed to persist wallet registration changeset: {0}")] PersisterStore(#[source] crate::changeset::PersistenceError), @@ -55,6 +58,8 @@ pub enum PlatformWalletError { /// [`PlatformWalletError`](Self) (boxed to break the recursive type) so /// its concrete variant and `#[source]` chain survive instead of being /// flattened into a string. + /// + /// Construct with [`Self::from_restore_failure`], which boxes for you. #[error("failed to restore persisted platform-address state: {0}")] PersisterRestore(#[source] Box), @@ -954,6 +959,41 @@ pub enum PlatformWalletError { ShieldedNotBound, } +impl PlatformWalletError { + /// A persister `load` failed. Wraps the typed cause so its retry + /// classification survives. + /// + /// There is deliberately no blanket `From`: the + /// conversion is undecidable from the value, because a + /// [`PersistenceError`] does not record whether a load, a store or a + /// flush produced it. Pick the constructor naming the operation that + /// actually failed — an inferred one would silently label failed + /// writes as failed reads. Constructing through these rather than the + /// variants also lets the enum's internals change without touching + /// call sites. + /// + /// [`PersistenceError`]: crate::changeset::PersistenceError + pub fn from_load_failure(source: crate::changeset::PersistenceError) -> Self { + Self::PersisterLoad(source) + } + + /// A persister `store` failed. Distinct from + /// [`Self::from_load_failure`] so a failed write is never reported as + /// a failed read. See that constructor for why no blanket conversion + /// exists. + pub fn from_store_failure(source: crate::changeset::PersistenceError) -> Self { + Self::PersisterStore(source) + } + + /// Restoring persisted platform-address state into a freshly + /// registered wallet failed. Boxes `source` internally, so callers + /// never write `Box::new`. See [`Self::from_load_failure`] for why no + /// blanket conversion exists. + pub fn from_restore_failure(source: PlatformWalletError) -> Self { + Self::PersisterRestore(Box::new(source)) + } +} + /// Check whether an SDK error indicates that an InstantSend lock proof was /// rejected by Platform (e.g. the IS lock has expired). /// diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index 91183a81cf1..674270fe0c2 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -52,7 +52,7 @@ impl PlatformWalletManager

{ // Debug, not Display: it carries the real cause (e.g. a // bincode decode failure) rather than flattening the chain. tracing::debug!(error = ?e, "persister load failed during rehydration"); - return Err(PlatformWalletError::PersisterLoad(e)); + return Err(PlatformWalletError::from_load_failure(e)); } }; let ClientStartState { diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 757e93cde70..5aa704c5e02 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -502,7 +502,7 @@ impl PlatformWalletManager

{ "rollback: remove_wallet failed while unwinding a failed wallet registration" ); } - return Err(PlatformWalletError::PersisterStore(e)); + return Err(PlatformWalletError::from_store_failure(e)); } // Build the PlatformWallet handle. @@ -560,7 +560,7 @@ impl PlatformWalletManager

{ "rollback: remove_wallet failed while unwinding a failed wallet setup" ); } - return Err(PlatformWalletError::PersisterLoad(e)); + return Err(PlatformWalletError::from_load_failure(e)); } }; @@ -586,7 +586,7 @@ impl PlatformWalletManager

{ // `initialize_from_persisted` already returns a typed // `PlatformWalletError`; wrap (boxed) rather than stringify so // its concrete variant and source chain survive. - return Err(PlatformWalletError::PersisterRestore(Box::new(e))); + return Err(PlatformWalletError::from_restore_failure(e)); } } else { platform_wallet.platform().initialize().await; @@ -1785,11 +1785,17 @@ mod persist_retry_tests { /// The typed persister-phase variants preserve retry /// classification, enable structural matching, and keep the `#[source]` /// chain instead of flattening to a string. + /// + /// Also pins the named constructors to the operation each is named + /// for. That is the whole reason no blanket `From` + /// exists: the same value can come from a load, a store or a flush, so + /// only the call site knows which variant is truthful, and an inferred + /// conversion reports failed writes as failed reads. #[test] fn typed_variants_preserve_classification_matching_and_source() { use std::error::Error; - let store_err = PlatformWalletError::PersisterStore(transient()); + let store_err = PlatformWalletError::from_store_failure(transient()); match &store_err { PlatformWalletError::PersisterStore(pe) => assert!(pe.is_transient()), other => panic!("expected PersisterStore, got {other:?}"), @@ -1799,7 +1805,7 @@ mod persist_retry_tests { "PersisterStore must expose its PersistenceError source" ); - let load_err = PlatformWalletError::PersisterLoad(fatal()); + let load_err = PlatformWalletError::from_load_failure(fatal()); match &load_err { PlatformWalletError::PersisterLoad(pe) => assert!(!pe.is_transient()), other => panic!("expected PersisterLoad, got {other:?}"), @@ -1809,7 +1815,7 @@ mod persist_retry_tests { // The restore variant wraps a typed inner error; structural matching // must recover the concrete inner variant, not an opaque string. let restore_err = - PlatformWalletError::PersisterRestore(Box::new(PlatformWalletError::WalletLocked)); + PlatformWalletError::from_restore_failure(PlatformWalletError::WalletLocked); assert!(restore_err.source().is_some()); match restore_err { PlatformWalletError::PersisterRestore(inner) => { @@ -1817,6 +1823,25 @@ mod persist_retry_tests { } other => panic!("expected PersisterRestore, got {other:?}"), } + + // The two persister-error constructors take the SAME input type, so + // nothing but the call site distinguishes them — mixing them up is + // silent, and is exactly the defect the removed blanket conversion + // produced downstream. + assert!( + matches!( + PlatformWalletError::from_store_failure(fatal()), + PlatformWalletError::PersisterStore(_) + ), + "a failed store must never be reported as a failed load" + ); + assert!( + matches!( + PlatformWalletError::from_load_failure(fatal()), + PlatformWalletError::PersisterLoad(_) + ), + "a failed load must never be reported as a failed store" + ); } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs index 49f34530e22..d6451b86382 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs @@ -27,9 +27,9 @@ use super::super::manager::AssetLockManager; /// Persister errors are surfaced as `Err(PersistenceError)` so call /// sites can choose their own policy: /// -/// - **Poll loops** (`wait_for_chain_lock`, `wait_for_proof`) downgrade -/// transient failures to `None` for the current iteration and surface -/// permanent failures — see [`record_or_persister_or_log`]. +/// - **Poll loops** (`wait_for_chain_lock`, `wait_for_proof`) read every +/// failure as a miss and keep waiting on the live sync stream — see +/// [`record_or_persister_for_poll`]. /// - **One-shot recovery / fast-fail call sites** want the error /// visible so a transient backend failure isn't silently classified /// as "tx not found" — they handle the `Err` arm explicitly. @@ -143,30 +143,56 @@ pub(in crate::wallet::asset_lock) fn record_holds_local_finality( } } -/// Variant of [`record_or_persister`] that retries transient failures as a miss. +/// Variant of [`record_or_persister`] for poll loops: never aborts the +/// wait, whatever the persister does. /// -/// Use this from poll loops where the next iteration retries. Permanent -/// failures remain errors so an unbounded poll cannot hide them. -pub(super) fn record_or_persister_or_log( +/// This read is a FALLBACK for records the in-memory map evicted; the live +/// SPV stream can still deliver the record and end the wait. So a failure +/// here reads as a miss and the loop keeps waiting, bounded by its own +/// finality timeout — aborting would turn a degraded read path into a +/// failed operation. +/// +/// A transient failure is a miss and nothing more; the next iteration +/// retries it. A permanent one is a miss too, but is reported once per +/// wait via `reported` — per-iteration logging would let a broken backend +/// flood the log from inside a loop, and the condition is the same one +/// every time. +pub(super) fn record_or_persister_for_poll( in_memory: Option, persister: &crate::wallet::persister::WalletPersister, txid: &Txid, -) -> Result, crate::changeset::PersistenceError> { - match record_or_persister(in_memory, persister, txid) { - Ok(opt) => Ok(opt), - Err(e) if e.is_transient() => { - tracing::warn!( - txid = %txid, - error = %e, - "Transient persister fallback for core tx record failed; \ - treating as miss for this poll iteration" - ); - Ok(None) + reported: &mut bool, +) -> Option { + match persister_read_for_poll(in_memory, persister, txid) { + Ok(found) => found, + Err(e) => { + if !*reported { + *reported = true; + tracing::error!( + txid = %txid, + error = %e, + "Core tx-record fallback read is permanently failing; waiting on the \ + live sync stream instead until this wait's timeout" + ); + } + None } - Err(e) => Err(e), } } +/// The transient half of the poll policy, split out so the permanent arm +/// above owns the once-per-wait reporting. +fn persister_read_for_poll( + in_memory: Option, + persister: &crate::wallet::persister::WalletPersister, + txid: &Txid, +) -> Result, crate::changeset::PersistenceError> { + if let Some(record) = in_memory { + return Ok(Some(record)); + } + persister.get_core_tx_record_or_transient_miss(txid) +} + impl AssetLockManager { /// Validate an IS-lock proof and upgrade it to a ChainLock proof if the /// transaction is old enough that the IS-lock may have expired. @@ -366,6 +392,9 @@ impl AssetLockManager { use key_wallet::transaction_checking::TransactionContext; let deadline = timeout.map(|t| tokio::time::Instant::now() + t); + // Once-per-wait guard for the tx-record fallback read (see + // `record_or_persister_for_poll`). + let mut read_failure_reported = false; loop { // Arm the `Notify` future BEFORE the state check, closing @@ -393,9 +422,12 @@ impl AssetLockManager { funding_tx_record(&info.core_wallet.accounts, account_index, &out_point.txid) }) }; - if let Some(record) = - record_or_persister_or_log(in_memory, &self.persister, &out_point.txid)? - { + if let Some(record) = record_or_persister_for_poll( + in_memory, + &self.persister, + &out_point.txid, + &mut read_failure_reported, + ) { if matches!(record.context, TransactionContext::InChainLockedBlock(_)) { if let Some(h) = record.height() { return Ok(h); @@ -455,6 +487,9 @@ impl AssetLockManager { tracing::info!(outpoint = %out_point, ?timeout, "wait_for_proof: entered"); let deadline = timeout.map(|t| tokio::time::Instant::now() + t); let mut iter: u32 = 0; + // Once-per-wait guard for the tx-record fallback read (see + // `record_or_persister_for_poll`). + let mut read_failure_reported = false; // Read account_index and transaction from the tracked lock. let (account_index, tracked_tx) = { @@ -520,9 +555,12 @@ impl AssetLockManager { funding_tx_record(&info.core_wallet.accounts, account_index, &out_point.txid) }) }; - if let Some(record) = - record_or_persister_or_log(in_memory, &self.persister, &out_point.txid)? - { + if let Some(record) = record_or_persister_for_poll( + in_memory, + &self.persister, + &out_point.txid, + &mut read_failure_reported, + ) { match &record.context { TransactionContext::InstantSend(instant_lock) => { return Ok(dpp::prelude::AssetLockProof::Instant( @@ -1096,22 +1134,70 @@ mod tests { assert!(resolved.is_err()); } + /// A poll loop must DEGRADE on a permanent read failure, not abort. + /// + /// The persister read is a fallback for records the in-memory map + /// evicted; the live SPV stream can still deliver the record and end + /// the wait. Aborting turns a degraded read path into a failed + /// operation, and the wait is already bounded by its finality timeout. + /// The failure is reported once per wait rather than once per + /// iteration, so a broken backend cannot flood the log from a loop. #[test] - fn record_or_persister_or_log_surfaces_permanent_backend_errors() { + fn poll_read_degrades_to_a_miss_on_permanent_backend_errors() { let unknown_txid = Txid::from([0xFF; 32]); let persister = wallet_persister(Arc::new(ErroringStore)); + let mut reported = false; - let resolved = record_or_persister_or_log(None, &persister, &unknown_txid); - assert!(resolved.is_err()); + let resolved = record_or_persister_for_poll(None, &persister, &unknown_txid, &mut reported); + assert!( + resolved.is_none(), + "a permanent read failure must read as a miss, not abort the wait" + ); + assert!(reported, "the first permanent failure must be reported"); + + // Subsequent iterations of the SAME wait stay silent. + let mut still_reported = reported; + let resolved = + record_or_persister_for_poll(None, &persister, &unknown_txid, &mut still_reported); + assert!(resolved.is_none()); + assert!(still_reported); } + /// A transient failure is a miss for this iteration and is NOT worth + /// the once-per-wait permanent-failure report — the next iteration + /// retries it. #[test] - fn record_or_persister_or_log_retries_transient_backend_errors() { + fn poll_read_treats_transient_backend_errors_as_a_silent_miss() { let unknown_txid = Txid::from([0xFF; 32]); let persister = wallet_persister(Arc::new(TransientErroringStore)); + let mut reported = false; - let resolved = record_or_persister_or_log(None, &persister, &unknown_txid) - .expect("transient poll error must be downgraded for retry"); + let resolved = record_or_persister_for_poll(None, &persister, &unknown_txid, &mut reported); assert!(resolved.is_none()); + assert!( + !reported, + "a transient failure must not consume the permanent-failure report" + ); + } + + /// The shared read helper collapses a transient failure into a miss so + /// every caller gets one policy, and leaves permanent failures visible. + #[test] + fn transient_miss_read_helper_separates_transient_from_permanent() { + let unknown_txid = Txid::from([0xFF; 32]); + + let transient = wallet_persister(Arc::new(TransientErroringStore)); + assert!(transient + .get_core_tx_record_or_transient_miss(&unknown_txid) + .expect("a transient failure must read as a miss") + .is_none()); + + let permanent = wallet_persister(Arc::new(ErroringStore)); + assert!( + permanent + .get_core_tx_record_or_transient_miss(&unknown_txid) + .is_err(), + "a permanent failure must stay visible to the caller" + ); } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index 659a430ffe3..9e0a9bc3418 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -224,6 +224,14 @@ impl DashPayView<'_, B> { /// /// Local-only and idempotent: an existing payment entry under the /// txid is never overwritten. + /// + /// # Errors + /// + /// Transient tx-record read failures leave the scan incomplete so the + /// guard stays unstamped and the next sweep retries; permanent ones + /// return [`PlatformWalletError::PersisterLoad`]. Retrying a permanent + /// failure every sweep would never succeed and would never be + /// reported. pub async fn reconcile_sent_payments_from_tx_history( &self, ) -> Result { @@ -407,7 +415,7 @@ impl DashPayView<'_, B> { continue; } let txid = entry.txid; - match self.persister.get_core_tx_record(&txid) { + match self.persister.get_core_tx_record_or_transient_miss(&txid) { Ok(Some(record)) => { // Walk the decoded transaction's outputs, NOT // `record.output_details`. Records handed back by @@ -427,6 +435,9 @@ impl DashPayView<'_, B> { .collect(), }); } + // Either the row is genuinely unreadable yet, or a + // transient failure already read as a miss. Both mean the + // same thing here: retry on the next sweep. Ok(None) => { incomplete_scan = true; tracing::debug!( @@ -434,14 +445,10 @@ impl DashPayView<'_, B> { "reconcile_sent_payments_from_tx_history: listed tx record unavailable; will retry next sweep" ); } - Err(e) => { - incomplete_scan = true; - tracing::warn!( - error = %e, - %txid, - "reconcile_sent_payments_from_tx_history: tx-record read failed; will retry next sweep" - ); - } + // A permanent failure will not fix itself, so deferring it + // re-runs the whole sweep on every sync forever and never + // says why. Same policy as the confirmation sweep. + Err(e) => return Err(PlatformWalletError::from_load_failure(e)), } } @@ -706,19 +713,12 @@ impl DashPayView<'_, B> { let Ok(txid) = txid_str.parse::() else { continue; }; - let record = match self.persister.get_core_tx_record(&txid) { + // A transient failure reads as a miss, so both are the same + // "not final yet, look again next sweep" outcome. + let record = match self.persister.get_core_tx_record_or_transient_miss(&txid) { Ok(Some(record)) => record, Ok(None) => continue, - Err(e) if e.is_transient() => { - tracing::warn!( - error = %e, - txid = %txid_str, - "reconcile_sent_payments: transient tx-record read failed; \ - will retry next sweep" - ); - continue; - } - Err(e) => return Err(PlatformWalletError::PersisterLoad(e)), + Err(e) => return Err(PlatformWalletError::from_load_failure(e)), }; // An InstantSend lock is final for DashPay display, same as a // mined block — one definition of "final", shared with the @@ -3701,6 +3701,76 @@ mod tests { ); } + /// A permanent tx-record read failure surfaces from the reconstruction + /// sweep; only a transient one is folded into "incomplete, retry next + /// time". + /// + /// The distinction is what stops a permanently unreadable store from + /// re-running the whole sweep on every dashpay sync, indefinitely and + /// silently. Same policy as the confirmation sweep. + #[tokio::test] + async fn reconcile_sent_payments_from_tx_history_surfaces_permanent_read_failures() { + use dashcore::hashes::Hash; + use dashcore::BlockHash; + use key_wallet::managed_account::transaction_record::OutputRole; + use key_wallet::transaction_checking::{BlockInfo, TransactionContext}; + + let persister = Arc::new(RecordStorePersister::default()); + let (manager, wallet_id) = make_wallet_with(Arc::clone(&persister)).await; + let owner = Identifier::from([0xAA; 32]); + let contact = Identifier::from([0xBB; 32]); + + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet.identity(); + let p = WalletPersister::new(wallet_id, Arc::clone(&persister) as _); + { + let mut wm = iw.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("info"); + info.identity_manager + .add_identity(bare_identity([0xAA; 32]), 0, wallet_id, &p) + .expect("add owner"); + } + let contact_addresses = install_external_account(&manager, wallet_id, owner, contact).await; + let change_address = first_standard_wallet_address(&manager, wallet_id).await; + let record = tx_record_with_outputs( + TransactionContext::InBlock(BlockInfo::new(123, BlockHash::all_zeros(), 0)), + vec![ + (contact_addresses[0].clone(), 25_000, OutputRole::Sent), + (change_address, 90_000, OutputRole::Change), + ], + ); + persister + .records + .lock() + .unwrap() + .insert(record.txid, record); + + // Transient: the sweep defers, exactly as before. + *persister.read_error_kind.lock().unwrap() = Some(PersistenceErrorKind::Transient); + assert_eq!( + iw.dashpay() + .reconcile_sent_payments_from_tx_history() + .await + .expect("a transient read failure must wait for the next sweep"), + 0 + ); + + // Permanent: the sweep reports it as a failed read. + *persister.read_error_kind.lock().unwrap() = Some(PersistenceErrorKind::Fatal); + let err = iw + .dashpay() + .reconcile_sent_payments_from_tx_history() + .await + .expect_err("a permanent read failure must surface, not loop forever"); + assert!( + matches!( + err, + PlatformWalletError::PersisterLoad(ref source) if !source.is_transient() + ), + "expected a permanent PersisterLoad, got {err:?}" + ); + } + #[tokio::test] async fn reconcile_sent_payments_from_tx_history_does_not_overwrite_existing_entry() { use dashcore::hashes::Hash; diff --git a/packages/rs-platform-wallet/src/wallet/persister.rs b/packages/rs-platform-wallet/src/wallet/persister.rs index e6cc78affaa..1fdf396f9f9 100644 --- a/packages/rs-platform-wallet/src/wallet/persister.rs +++ b/packages/rs-platform-wallet/src/wallet/persister.rs @@ -64,6 +64,34 @@ impl WalletPersister { self.inner.get_core_tx_record(self.wallet_id, txid) } + /// [`Self::get_core_tx_record`] with the shared transient-as-miss + /// read policy applied. + /// + /// A transient backend failure (a busy store) is indistinguishable in + /// outcome from "the row is not readable right now", and every caller + /// of this read already handles a miss by retrying on its next pass — + /// so it collapses to `Ok(None)` and is logged at debug. A permanent + /// failure stays an `Err`: it will not fix itself, so a caller that + /// swallowed it would repeat the same doomed work forever with no + /// signal. Callers that need to tell the two apart use + /// [`Self::get_core_tx_record`] directly. + pub(crate) fn get_core_tx_record_or_transient_miss( + &self, + txid: &Txid, + ) -> Result, PersistenceError> { + match self.get_core_tx_record(txid) { + Err(e) if e.is_transient() => { + tracing::debug!( + %txid, + error = %e, + "Core tx-record read hit a transient backend failure; reading as a miss" + ); + Ok(None) + } + other => other, + } + } + /// Enumerate the persisted Core transaction ids scoped to this /// wallet, tagged with the host's wallet-funded verdict. Used by /// DashPay sent-payment reconstruction to fetch the full records From acd1d2c6daa98e0dbeba84c7b77ea1eb07a8d5b7 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:46:32 +0000 Subject: [PATCH 8/9] test(platform-wallet): assert the poll-loop failure report fires once per wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing test pinned the report flag's final state, not the suppression the flag exists to provide. Deleting the `if !*reported` guard so every poll iteration logs still left `reported == true`, so the test passed and the regression would have shipped silently. Log volume from inside a poll loop is the whole point of the guard, and nothing was measuring it. The new test drives three iterations of one wait against a permanently failing persister and asserts EXACTLY ONE error event, counting matching events rather than observing that one exists. An assertion that merely finds a report present is satisfied just as happily by one per iteration. Mutation check, as required: guard deleted -> poll_read_reports_a_permanent_failure_once_per_wait_not_once_per_iteration FAILS: "three iterations of one wait must produce exactly one report, got 3" (left: 3, right: 1) -> 952 passed, 1 failed: the new test is the ONLY one that changes colour, so it isolates the suppression property. Notably poll_read_degrades_to_a_miss_on_permanent_backend_errors stays green under the mutation, which is the direct evidence that the flag-state assertion never covered this. guard restored -> green. Capturing the events needed the recorder harness that already existed in `wallet_lifecycle`'s test module, so it moves to `test_support` and both call sites share it. Moved verbatim: a second harness would have to re-derive the same constraint, and the naive alternative is a trap — a per-test `set_default` swap races tracing's process-global callsite interest cache under the parallel harness. The harness stays `#[cfg(test)]` because `tracing-subscriber` is a dev-dependency. `wallet_lifecycle`'s own test is unchanged and keeps its full strength (warn present AND error absent). No production code changed. Verified (`--no-deps` required: pre-existing unrelated rs-drive import): clippy --no-deps -p platform-wallet -p platform-wallet-ffi --all-targets -D warnings exit 0 test -p platform-wallet -p platform-wallet-ffi, CLAUDIUS_FORCE=1, twice exit 0, 1324 each Co-Authored-By: Claude Opus 5 --- .../src/manager/wallet_lifecycle.rs | 94 +--------------- .../rs-platform-wallet/src/test_support.rs | 106 ++++++++++++++++++ .../src/wallet/asset_lock/sync/proof.rs | 41 +++++++ 3 files changed, 149 insertions(+), 92 deletions(-) diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 5aa704c5e02..1e59cbae507 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -1295,18 +1295,14 @@ mod persist_retry_tests { //! Registration-path persistence: single-attempt `store` with typed //! error propagation, bounded `load` retry, and log-level policy. - use std::cell::RefCell; use std::sync::atomic::{AtomicUsize, Ordering}; - use std::sync::{Arc, Mutex, OnceLock}; + use std::sync::Arc; use std::time::Duration; use key_wallet::mnemonic::{Language, Mnemonic}; use key_wallet::wallet::initialization::WalletAccountCreationOptions; use key_wallet::Network; - use tracing::field::{Field, Visit}; use tracing::Level; - use tracing_subscriber::layer::{Context, SubscriberExt}; - use tracing_subscriber::Layer; use crate::changeset::{ ClientStartState, PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet, @@ -1333,93 +1329,7 @@ mod persist_retry_tests { PersistenceError::backend_with_kind(PersistenceErrorKind::Fatal, "simulated corruption") } - /// Captures the level and message of every `tracing` event recorded - /// while registered as the active recorder for the current thread (see - /// [`RecordingGuard`]). - #[derive(Clone, Default)] - struct RecordedEvents(Arc>>); - - impl RecordedEvents { - fn entries(&self) -> Vec<(Level, String)> { - self.0.lock().expect("recorded events mutex").clone() - } - - fn record(&self, event: &tracing::Event<'_>) { - struct MessageVisitor(String); - impl Visit for MessageVisitor { - fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { - if field.name() == "message" { - self.0 = format!("{value:?}"); - } - } - } - let mut visitor = MessageVisitor(String::new()); - event.record(&mut visitor); - self.0 - .lock() - .expect("recorded events mutex") - .push((*event.metadata().level(), visitor.0)); - } - } - - thread_local! { - /// The [`RecordedEvents`] a test on THIS thread wants routed to it, - /// if any. Set/cleared only by [`RecordingGuard`]. - static ACTIVE_RECORDER: RefCell> = const { RefCell::new(None) }; - } - - /// Routes every event to whichever [`RecordedEvents`] is registered for - /// the emitting thread, via [`ACTIVE_RECORDER`]. Installed as the - /// process-wide default exactly once — never per-test. - /// - /// A per-test `tracing::subscriber::set_default` swap is flaky under - /// `cargo test`'s parallel harness: tracing's per-callsite `Interest` - /// cache is process-global, and a concurrently-running test's own - /// swap/drop can race the interest rebuild your swap triggers, so the - /// event silently never reaches your subscriber even though dispatch - /// itself stays correctly on your own thread (confirmed: the emitting - /// thread ID matched the installing thread ID on a captured failure). - /// Installing the routing subscriber once, before any callsite is ever - /// hit, sidesteps the race — routing then happens through an ordinary - /// thread-local this code owns, not through tracing's default-swap - /// machinery. - struct RecorderRouter; - - impl Layer for RecorderRouter { - fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) { - ACTIVE_RECORDER.with(|slot| { - if let Some(recorder) = slot.borrow().as_ref() { - recorder.record(event); - } - }); - } - } - - static GLOBAL_ROUTER_INIT: OnceLock<()> = OnceLock::new(); - - /// Scopes [`ACTIVE_RECORDER`] to `recorder` for the current thread, for - /// the guard's lifetime. - struct RecordingGuard; - - impl RecordingGuard { - fn install(recorder: RecordedEvents) -> Self { - GLOBAL_ROUTER_INIT.get_or_init(|| { - let subscriber = tracing_subscriber::registry().with(RecorderRouter); - // Another thread may have already won this race; either - // way, the routing subscriber is the process-wide default - // by the time `get_or_init` returns to any caller. - let _ = tracing::subscriber::set_global_default(subscriber); - }); - ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = Some(recorder)); - Self - } - } - - impl Drop for RecordingGuard { - fn drop(&mut self) { - ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = None); - } - } + use crate::test_support::tracing_capture::{RecordedEvents, RecordingGuard}; /// Persister whose `store` / `flush` / `load` outcomes are scripted so /// the registration path can be driven deterministically. diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index 40442c6d93b..70967a37abb 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -784,3 +784,109 @@ pub(crate) async fn mnemonic_wallet_manager( receive_address, ) } + +/// Thread-scoped `tracing` event capture for tests that assert on log +/// output. +/// +/// Shared because the naive approach is a trap: a per-test +/// `tracing::subscriber::set_default` swap is flaky under `cargo test`'s +/// parallel harness, so every capturing test must route through the one +/// globally-installed subscriber here rather than installing its own. +#[cfg(test)] +pub(crate) mod tracing_capture { + use std::cell::RefCell; + use std::sync::{Arc, Mutex, OnceLock}; + + use tracing::field::{Field, Visit}; + use tracing::Level; + use tracing_subscriber::layer::{Context, SubscriberExt}; + use tracing_subscriber::Layer; + + /// Captures the level and message of every `tracing` event recorded + /// while registered as the active recorder for the current thread (see + /// [`RecordingGuard`]). + #[derive(Clone, Default)] + pub(crate) struct RecordedEvents(Arc>>); + + impl RecordedEvents { + pub(crate) fn entries(&self) -> Vec<(Level, String)> { + self.0.lock().expect("recorded events mutex").clone() + } + + fn record(&self, event: &tracing::Event<'_>) { + struct MessageVisitor(String); + impl Visit for MessageVisitor { + fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { + if field.name() == "message" { + self.0 = format!("{value:?}"); + } + } + } + let mut visitor = MessageVisitor(String::new()); + event.record(&mut visitor); + self.0 + .lock() + .expect("recorded events mutex") + .push((*event.metadata().level(), visitor.0)); + } + } + + thread_local! { + /// The [`RecordedEvents`] a test on THIS thread wants routed to it, + /// if any. Set/cleared only by [`RecordingGuard`]. + static ACTIVE_RECORDER: RefCell> = const { RefCell::new(None) }; + } + + /// Routes every event to whichever [`RecordedEvents`] is registered for + /// the emitting thread, via [`ACTIVE_RECORDER`]. Installed as the + /// process-wide default exactly once — never per-test. + /// + /// A per-test `tracing::subscriber::set_default` swap is flaky under + /// `cargo test`'s parallel harness: tracing's per-callsite `Interest` + /// cache is process-global, and a concurrently-running test's own + /// swap/drop can race the interest rebuild your swap triggers, so the + /// event silently never reaches your subscriber even though dispatch + /// itself stays correctly on your own thread (confirmed: the emitting + /// thread ID matched the installing thread ID on a captured failure). + /// Installing the routing subscriber once, before any callsite is ever + /// hit, sidesteps the race — routing then happens through an ordinary + /// thread-local this code owns, not through tracing's default-swap + /// machinery. + struct RecorderRouter; + + impl Layer for RecorderRouter { + fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) { + ACTIVE_RECORDER.with(|slot| { + if let Some(recorder) = slot.borrow().as_ref() { + recorder.record(event); + } + }); + } + } + + static GLOBAL_ROUTER_INIT: OnceLock<()> = OnceLock::new(); + + /// Scopes [`ACTIVE_RECORDER`] to `recorder` for the current thread, for + /// the guard's lifetime. + pub(crate) struct RecordingGuard; + + impl RecordingGuard { + pub(crate) fn install(recorder: RecordedEvents) -> Self { + GLOBAL_ROUTER_INIT.get_or_init(|| { + let subscriber = tracing_subscriber::registry().with(RecorderRouter); + // Another thread may have already won this race; either + // way, the routing subscriber is the process-wide default + // by the time `get_or_init` returns to any caller. + let _ = tracing::subscriber::set_global_default(subscriber); + }); + ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = Some(recorder)); + Self + } + } + + impl Drop for RecordingGuard { + fn drop(&mut self) { + ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = None); + } + } +} diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs index d6451b86382..05b8c8b0acd 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs @@ -1163,6 +1163,47 @@ mod tests { assert!(still_reported); } + /// The permanent-failure report fires ONCE per wait, not once per + /// iteration. + /// + /// A poll loop can spin many times against the same broken backend, so + /// reporting per iteration would bury the log under one repeated line + /// while saying nothing new. Counting the events is the point: an + /// assertion that merely finds a report present passes just as happily + /// when every iteration emits one. + #[test] + fn poll_read_reports_a_permanent_failure_once_per_wait_not_once_per_iteration() { + use crate::test_support::tracing_capture::{RecordedEvents, RecordingGuard}; + use tracing::Level; + + let unknown_txid = Txid::from([0xFF; 32]); + let persister = wallet_persister(Arc::new(ErroringStore)); + let mut reported = false; + + let recorder = RecordedEvents::default(); + let _guard = RecordingGuard::install(recorder.clone()); + + // Three iterations of ONE wait, as a poll loop would. + for _ in 0..3 { + assert!( + record_or_persister_for_poll(None, &persister, &unknown_txid, &mut reported) + .is_none() + ); + } + + let reports = recorder + .entries() + .into_iter() + .filter(|(level, msg)| { + *level == Level::ERROR && msg.contains("Core tx-record fallback read") + }) + .count(); + assert_eq!( + reports, 1, + "three iterations of one wait must produce exactly one report, got {reports}" + ); + } + /// A transient failure is a miss for this iteration and is NOT worth /// the once-per-wait permanent-failure report — the next iteration /// retries it. From 0940cf41f0556ce3d29f6afcad6e9233f1a56bbe Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:35:01 +0000 Subject: [PATCH 9/9] docs(platform-wallet): condense the typed-persister-error commentary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prose this branch added said the same thing several times over. Cut 193 of 649 added comment lines (-29%) without losing a load-bearing sentence. Comments and doc text only — no executable line, signature or test assertion changed. What went: - Sibling repetition. The "carries the typed PersistenceError so the retry classification survives" paragraph was restated on all three Persister* variants; it now sits once on PersisterLoad and the siblings say only what differs. Same treatment for the rs-unified-sdk-jni RESOLVE_* caveat, which was repeated per constant and now sits once in the persistence module header. - Cross-crate constants duplicated into prose. rs-platform-wallet's error.rs hardcoded FFI result codes 49/50/51/52/53 into rustdoc for a mapping that lives in another crate, where they would drift silently. The numbers are gone; the mapping is referenced by name. - Signature restatement ("Construct with [`Self::from_load_failure`]" directly above from_load_failure), and test docs that only re-read their own test name. - Intra-doc link footer blocks, replaced by the inline [`Name`](path) form where a link still earns its keep. What stayed, deliberately: the undecidability argument for having no blanket From (tightened 11 lines to 6, argument intact), why writes are never retried in-crate, why the poll-loop failure report fires once per wait, the FFI round-classification atomicity gate, and every note explaining a race or lock discipline. The C ABI contract in PersistenceCallbacks is the product out-of-tree hosts implement against, so it was tightened rather than trimmed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QhTi3LdR3CfAvHRb34ZB7D --- packages/rs-platform-wallet-ffi/src/error.rs | 132 ++++++-------- .../rs-platform-wallet-ffi/src/persistence.rs | 166 +++++++----------- .../src/changeset/core_bridge.rs | 20 +-- packages/rs-platform-wallet/src/error.rs | 78 +++----- .../rs-platform-wallet/src/manager/load.rs | 75 +++----- .../rs-platform-wallet/src/manager/mod.rs | 17 +- .../src/manager/persist_retry.rs | 19 +- .../rs-platform-wallet/src/manager/startup.rs | 4 +- .../src/manager/wallet_lifecycle.rs | 115 ++++-------- .../rs-platform-wallet/src/test_support.rs | 53 +++--- .../src/wallet/asset_lock/sync/proof.rs | 60 +++---- .../src/wallet/identity/network/discovery.rs | 7 +- .../src/wallet/identity/network/payments.rs | 30 ++-- .../src/wallet/persister.rs | 17 +- 14 files changed, 300 insertions(+), 493 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index c0e31a84eec..5fc2a20b3d6 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -509,79 +509,68 @@ pub enum PlatformWalletFFIResultCode { // ----------------------------------------------------------------- // Persister failures, operation x retry classification (49-54). // - // The wallet's PersisterLoad / PersisterStore / PersisterRestore - // variants each carry a typed `PersistenceError`, whose `kind` says - // whether a retry can help. Before these codes all three flattened to - // ErrorUnknown (99) and the classification died at the boundary. One - // code per (operation, kind) pair keeps both halves: a host can tell a - // failed read from a failed write AND a retryable failure from a + // The wallet's PersisterLoad / PersisterStore / PersisterRestore each + // carry a typed `PersistenceError` whose `kind` says whether a retry can + // help. One code per (operation, kind) pair keeps both halves: a host can + // tell a failed read from a failed write AND a retryable failure from a // permanent one, without parsing the message. // ----------------------------------------------------------------- - /// Maps `PlatformWalletError::PersisterLoad` whose `PersistenceError` - /// is classified [`Transient`](platform_wallet::changeset::PersistenceErrorKind::Transient) — - /// the store reported a retryable condition (`SQLITE_BUSY` and - /// friends) while reading persisted state. + /// Maps `PlatformWalletError::PersisterLoad` classified + /// [`Transient`](platform_wallet::changeset::PersistenceErrorKind::Transient): + /// a retryable condition (`SQLITE_BUSY` and friends) while reading. /// - /// Host action: retry the operation later. Nothing was mutated — a - /// load is a read. + /// Host action: retry later. Nothing was mutated — a load is a read. ErrorPersisterLoadTransient = 49, /// Maps `PlatformWalletError::PersisterLoad` for every other - /// classification: `Fatal`, `Constraint`, and a poisoned persister - /// lock. Reading persisted state failed permanently — a corrupt or - /// unreadable store, or a decode that will fail identically next - /// time. + /// classification — `Fatal`, `Constraint`, and a poisoned persister lock: + /// a corrupt or unreadable store, or a decode that will fail identically + /// next time. /// /// Host action: do NOT retry; inspect the message and repair or /// re-provision the store. `Constraint` folds in here because a read - /// cannot violate one: if a store reports it on a load, it is a - /// backend defect, not a caller data error, and it is not retryable - /// either way. + /// cannot violate one — reported on a load it is a backend defect, not a + /// caller data error, and not retryable either way. ErrorPersisterLoadFatal = 50, - /// Maps `PlatformWalletError::PersisterStore` whose `PersistenceError` - /// is classified - /// [`Transient`](platform_wallet::changeset::PersistenceErrorKind::Transient) — + /// Maps `PlatformWalletError::PersisterStore` classified + /// [`Transient`](platform_wallet::changeset::PersistenceErrorKind::Transient): /// a busy or momentarily unavailable store rejected the write. /// /// **Nothing was committed**: the wallet only reports this when the - /// persister guarantees the failed changeset round was rolled back - /// whole, so re-issuing the operation cannot double-apply part of it. + /// persister guarantees the failed round was rolled back whole, so + /// re-issuing cannot double-apply part of it. /// - /// Host action: retry the operation later. This is the code a wallet - /// registration against a locked database produces - /// (`dashpay/platform#4365`) — the operation aborted, and the retry - /// decision is the host's, not the wallet's. + /// Host action: retry later. This is the code a wallet registration + /// against a locked database produces (`dashpay/platform#4365`) — the + /// retry decision is the host's, not the wallet's. ErrorPersisterStoreTransient = 51, - /// Maps `PlatformWalletError::PersisterStore` classified `Fatal`, and - /// a poisoned persister lock. The write failed permanently — a full - /// disk, a corrupt schema, an I/O error outside the retryable class. + /// Maps `PlatformWalletError::PersisterStore` classified `Fatal`, and a + /// poisoned persister lock: a full disk, a corrupt schema, an I/O error + /// outside the retryable class. /// - /// Host action: do NOT retry; inspect the message. The wallet's - /// in-memory state was rolled back to before the operation, so the - /// host may re-attempt once the underlying fault is fixed. + /// Host action: do NOT retry; inspect the message. The wallet's in-memory + /// state was rolled back to before the operation, so the host may + /// re-attempt once the underlying fault is fixed. ErrorPersisterStoreFatal = 52, /// Maps `PlatformWalletError::PersisterStore` classified - /// [`Constraint`](platform_wallet::changeset::PersistenceErrorKind::Constraint) — - /// a SQL constraint / foreign-key / integrity violation. Distinct - /// from [`Self::ErrorPersisterStoreFatal`] so a host can separate - /// "your data is wrong" from "the storage engine is unhappy": the - /// first is a caller or schema-mapping bug, the second an operator - /// or infrastructure problem, and they route to different people. + /// [`Constraint`](platform_wallet::changeset::PersistenceErrorKind::Constraint): + /// a SQL constraint / foreign-key / integrity violation. Distinct from + /// [`Self::ErrorPersisterStoreFatal`] so a host can separate "your data is + /// wrong" (caller or schema-mapping bug) from "the storage engine is + /// unhappy" (operator problem) — they route to different people. /// - /// Host action: do NOT retry unchanged — fix the data (or the - /// host-side schema mapping that produced it). + /// Host action: do NOT retry unchanged — fix the data, or the host-side + /// schema mapping that produced it. ErrorPersisterStoreConstraint = 53, - /// Maps `PlatformWalletError::PersisterRestore`. Rehydrating persisted - /// platform-address state into a freshly registered wallet failed. - /// - /// One code, not three: this variant wraps a `PlatformWalletError` - /// rather than a `PersistenceError`, so it carries no retry - /// classification to split on. The wrapped error's `Display` reaches - /// the host in the message. + /// Maps `PlatformWalletError::PersisterRestore`: rehydrating persisted + /// platform-address state into a freshly registered wallet failed. One + /// code, not three — it wraps a `PlatformWalletError` rather than a + /// `PersistenceError`, so there is no retry classification to split on, + /// and the wrapped error's `Display` is the only detail channel. /// /// Host action: inspect the message; the wallet was registered but its /// persisted address state did not come back. @@ -967,12 +956,10 @@ impl From for PlatformWalletFFIResult { // rides `NotFound` rather than spending a fifth marketplace // code hosts would handle identically. PlatformWalletError::DpnsNameNotFound { .. } => PlatformWalletFFIResultCode::NotFound, - // The persister trio. Each carries the store's own retry - // classification, which is the whole reason these codes exist — - // flattened to ErrorUnknown a host could not tell a busy database - // from a corrupt one. `PersisterRestore` wraps a - // `PlatformWalletError` rather than a `PersistenceError`, so it - // has no kind to split on and takes a single code. + // The persister trio, split by the store's own retry + // classification — flattened to ErrorUnknown a host could not tell + // a busy database from a corrupt one. `PersisterRestore` carries + // no kind to split on, so it takes a single code. PlatformWalletError::PersisterLoad(source) => match source.kind() { Some(PersistenceErrorKind::Transient) => { PlatformWalletFFIResultCode::ErrorPersisterLoadTransient @@ -2077,17 +2064,14 @@ mod tests { assert_eq!(result.code, PlatformWalletFFIResultCode::ErrorUnknown); } - /// Build a `PersistenceError` of a chosen kind, the way a persister - /// backend (or the FFI persister's sentinel classification) would. + /// A `PersistenceError` of a chosen kind, as a backend would report it. fn persistence_error( kind: PersistenceErrorKind, ) -> platform_wallet::changeset::PersistenceError { platform_wallet::changeset::PersistenceError::backend_with_kind(kind, "database is locked") } - /// A transient read failure must reach the host as its own code, not - /// as the fatal sibling and not as `ErrorUnknown`: it is the one - /// persister outcome a host may retry unchanged. + /// The one persister outcome a host may retry unchanged. #[test] fn persister_load_transient_maps_to_code_49() { assert_eq!( @@ -2110,8 +2094,7 @@ mod tests { ); } - /// Fatal, constraint and lock-poisoned reads all fold onto one code: - /// none of them is retryable, and a read cannot violate a constraint. + /// None is retryable, and a read cannot violate a constraint. #[test] fn persister_load_non_transient_kinds_fold_onto_code_50() { assert_eq!( @@ -2135,9 +2118,8 @@ mod tests { } } - /// The code the busy-database registration case produces - /// (`dashpay/platform#4365`). The wallet does not retry the write; the - /// host learns it may. + /// The busy-database registration case (`dashpay/platform#4365`): the + /// wallet does not retry the write, the host learns it may. #[test] fn persister_store_transient_maps_to_code_51() { assert_eq!( @@ -2155,8 +2137,7 @@ mod tests { ); } - /// A permanent write failure, and the lock-poisoned case that has no - /// kind of its own. + /// Permanent writes, plus the lock-poisoned case that has no kind. #[test] fn persister_store_fatal_maps_to_code_52() { assert_eq!( @@ -2177,9 +2158,7 @@ mod tests { } } - /// "Your data is wrong" must not arrive as "the storage engine is - /// unhappy": the two route to different people, so the constraint - /// kind keeps its own code rather than folding into 52. + /// "Your data is wrong" must not arrive as "the storage engine is unhappy". #[test] fn persister_store_constraint_maps_to_code_53() { assert_eq!( @@ -2201,9 +2180,7 @@ mod tests { ); } - /// `PersisterRestore` wraps a `PlatformWalletError`, so it carries no - /// retry classification and takes a single code. The wrapped error's - /// rendering still has to reach the host. + /// One code, and the wrapped error's rendering still reaches the host. #[test] fn persister_restore_maps_to_code_54() { assert_eq!( @@ -2226,9 +2203,8 @@ mod tests { ); } - /// The six persister codes must stay distinct from each other and from - /// every code already allocated: a host pins these integers, and a - /// collision silently re-labels a shipped meaning. + /// A host pins these integers, so a collision with an already-allocated + /// code silently re-labels a shipped meaning. #[test] fn persister_codes_occupy_their_own_slots() { let persister = [ @@ -2241,8 +2217,8 @@ mod tests { ]; assert_eq!(persister, [49, 50, 51, 52, 53, 54]); - // The highest code allocated before this block, and the sentinels - // the registry keeps terminal. + // The highest code allocated before this block, plus the terminal + // sentinels. for taken in [ PlatformWalletFFIResultCode::ErrorAssetLockInputContested as i32, PlatformWalletFFIResultCode::NotFound as i32, diff --git a/packages/rs-platform-wallet-ffi/src/persistence.rs b/packages/rs-platform-wallet-ffi/src/persistence.rs index de5ce7fdce9..aa7ca32931d 100644 --- a/packages/rs-platform-wallet-ffi/src/persistence.rs +++ b/packages/rs-platform-wallet-ffi/src/persistence.rs @@ -4,6 +4,10 @@ //! data is available (e.g., address balances), it is sent across FFI in //! C-compatible structs so the caller can persist it incrementally (e.g., via //! SwiftData on iOS). +//! +//! The negative callback return codes defined here are unrelated to +//! `rs-unified-sdk-jni`'s `RESOLVE_*` mnemonic-resolver codes, which reuse the +//! same integers on a different callback family. use bincode::config; use key_wallet::account::account_collection::AccountCollection; @@ -272,27 +276,20 @@ pub struct PersistenceExtensionCallbacks { } /// Return value by which a persistence callback reports a **retryable** -/// failure after which nothing was applied (the host's own -/// `SQLITE_BUSY` / `SQLITE_FULL` / `SQLITE_IOERR` class). +/// failure after which nothing was applied (the host's own `SQLITE_BUSY` / +/// `SQLITE_FULL` / `SQLITE_IOERR` class). /// -/// The host holds the real storage handle and is the only party that can -/// see the native status code, so this is the only channel through which -/// a retry classification reaches the Rust side. Failures reported this -/// way surface to the Rust caller as +/// The host holds the storage handle and is the only party that can see the +/// native status code, so this is the only channel through which a retry +/// classification reaches Rust. Surfaces to the caller as /// [`PersistenceErrorKind::Transient`]; the caller — never this crate — /// decides whether to retry. -/// -/// Unrelated to `rs-unified-sdk-jni`'s `RESOLVE_*` mnemonic-resolver -/// codes, which share these integers on a different callback family. pub const PLATFORM_WALLET_PERSIST_RC_TRANSIENT: i32 = -2; /// Return value by which a persistence callback reports a constraint / /// foreign-key / integrity violation, surfacing as -/// [`PersistenceErrorKind::Constraint`] — "the data is wrong", as -/// opposed to "the storage engine is unhappy". Not retryable. -/// -/// Same caveat about `rs-unified-sdk-jni`'s `RESOLVE_*` codes as -/// [`PLATFORM_WALLET_PERSIST_RC_TRANSIENT`]. +/// [`PersistenceErrorKind::Constraint`] — "the data is wrong", as opposed to +/// "the storage engine is unhappy". Not retryable. pub const PLATFORM_WALLET_PERSIST_RC_CONSTRAINT: i32 = -3; /// Classify a non-zero persistence-callback return value. @@ -309,21 +306,18 @@ fn persist_rc_kind(rc: i32) -> PersistenceErrorKind { } } -/// Build the error for a non-zero return from a **single-call** callback -/// (a load, a flush, a standalone persist), carrying the host's own -/// classification of `rc`. -/// -/// Round-participating callbacks do not use this: their verdicts are -/// accumulated by [`RoundOutcome`] and classified once for the round. +/// Build the error for a non-zero return from a **single-call** callback (a +/// load, a flush, a standalone persist), carrying the host's classification of +/// `rc`. Round callbacks instead accumulate into [`RoundOutcome`], which +/// classifies once for the whole round. fn persist_callback_error(rc: i32, message: impl Into) -> PersistenceError { PersistenceError::backend_with_kind(persist_rc_kind(rc), message.into()) } -/// The verdict of one `store` round's callbacks. -/// -/// A round fails if any callback failed, and reports the MOST SEVERE kind -/// any of them returned (`Fatal` > `Constraint` > `Transient`) so one -/// host-declared transient can never mask a fatal sibling. +/// The verdict of one `store` round's callbacks: fails if any callback failed, +/// reporting the MOST SEVERE kind any returned +/// (`Fatal` > `Constraint` > `Transient`) so one host-declared transient can +/// never mask a fatal sibling. #[derive(Default)] struct RoundOutcome { worst: Option, @@ -335,8 +329,8 @@ impl RoundOutcome { self.escalate(persist_rc_kind(rc)); } - /// Record a Rust-side failure to encode a payload. Never transient: - /// the same changeset will not encode on a later attempt. + /// Record a Rust-side encoding failure. Never transient: the same + /// changeset will not encode on a later attempt. fn record_fatal(&mut self) { self.escalate(PersistenceErrorKind::Fatal); } @@ -355,8 +349,8 @@ impl RoundOutcome { } } - /// `true` while every callback so far has returned success. This is - /// what `on_changeset_end_fn` receives as its `success` argument. + /// `true` while every callback so far has succeeded — what + /// `on_changeset_end_fn` receives as its `success` argument. fn is_success(&self) -> bool { self.worst.is_none() } @@ -396,35 +390,32 @@ impl RoundOutcome { /// reading Rust has always applied, so a host written against the original /// contract needs no change. /// -/// A host that can classify its own failure (it holds the storage handle -/// and sees the native status code) may instead return one of two -/// sentinels, which reach the Rust caller as a typed retry classification: +/// A host that can classify its own failure (it holds the storage handle and +/// sees the native status code) may instead return one of two sentinels, +/// which reach the Rust caller as a typed retry classification: /// /// * [`PLATFORM_WALLET_PERSIST_RC_TRANSIENT`] — a retryable failure after /// which **nothing was applied** (`SQLITE_BUSY` and friends). /// * [`PLATFORM_WALLET_PERSIST_RC_CONSTRAINT`] — a constraint / integrity /// violation: the data is wrong, and retrying it unchanged will not help. /// -/// The Rust side never retries on a host's behalf; it forwards the -/// classification and the caller decides. +/// Rust never retries on a host's behalf; it forwards the classification and +/// the caller decides. /// /// ## What a transient verdict promises, and who must honour it /// -/// A caller acting on "transient" re-issues the WHOLE changeset, and -/// changeset vectors merge by appending. So a transient verdict is only -/// meaningful when the failed round left nothing applied — which is exactly -/// what `ATOMIC_CHANGESETS` attests ("a changeset is committed or rolled -/// back as one unit"), and what [`Self::on_changeset_end_fn`] with -/// `success = false` exists to drive. -/// -/// A `store` round therefore reports a transient failure ONLY when both -/// round brackets are wired and the host declared `ATOMIC_CHANGESETS`; -/// otherwise Rust downgrades it to fatal, because a partially applied round -/// re-sent in full would duplicate rows rather than replace them. **A host -/// that does not roll a failed round back must not return the transient -/// sentinel from a round callback.** Single-call callbacks (loads, flush, -/// the changeset-begin abort) have no such precondition: each is one -/// operation that either happened or did not. +/// A caller acting on "transient" re-issues the WHOLE changeset, and changeset +/// vectors merge by appending — so the verdict is only meaningful when the +/// failed round left nothing applied. That is exactly what +/// `ATOMIC_CHANGESETS` attests and what [`Self::on_changeset_end_fn`] with +/// `success = false` exists to drive, so a `store` round reports a transient +/// failure ONLY when both round brackets are wired AND the host declared +/// `ATOMIC_CHANGESETS`; otherwise Rust downgrades it to fatal, because a +/// partially applied round re-sent in full duplicates rows rather than +/// replacing them. **A host that does not roll a failed round back must not +/// return the transient sentinel from a round callback.** Single-call +/// callbacks (loads, flush, the changeset-begin abort) have no such +/// precondition: each is one operation that either happened or did not. #[repr(C)] #[allow(clippy::type_complexity)] pub struct PersistenceCallbacks { @@ -1271,22 +1262,12 @@ impl FFIPersister { } } - /// Narrow a `store` round's failure kind to what the caller may safely - /// act on. - /// - /// [`PersistenceErrorKind::Transient`] invites the caller to re-send the - /// whole changeset, which is only sound when a failed round left nothing - /// applied — `Merge for Vec` appends, so re-sending a partially - /// applied round doubles its vector fields instead of overwriting them. - /// A round is all-or-nothing exactly when - /// [`PersistenceCapabilities::ATOMIC_CHANGESETS`] holds, which requires - /// both round brackets to be wired AND the host to have attested - /// "committed or rolled back as one unit". Without that attestation a - /// transient verdict is downgraded to `Fatal`: losing a retry - /// opportunity costs less than duplicating data. - /// - /// `Constraint` and `Fatal` pass through unchanged — neither invites a - /// retry, so neither depends on the round being atomic. + /// Narrow a round's failure kind to what the caller may safely act on: + /// `Transient` survives only under + /// [`PersistenceCapabilities::ATOMIC_CHANGESETS`] (see + /// [`PersistenceCallbacks`]), since losing a retry opportunity costs less + /// than the rows a re-sent partial round would duplicate. `Constraint` and + /// `Fatal` invite no retry, so they pass through. fn reportable_round_kind(&self, reported: PersistenceErrorKind) -> PersistenceErrorKind { let atomic = self .persistence_capabilities() @@ -2715,10 +2696,9 @@ impl PlatformWalletPersistence for FFIPersister { ignored" ); } else { - // This branch runs only without an end callback, so the - // per-kind writes already landed individually and the - // round is not all-or-nothing — `reportable_round_kind` - // withholds a retryable verdict accordingly. + // No end callback, so the per-kind writes already landed + // individually and the round is not all-or-nothing — + // `reportable_round_kind` withholds a retryable verdict. return Err(PersistenceError::backend_with_kind( self.reportable_round_kind(persist_rc_kind(result)), format!("Persistence store callback returned error code {result}"), @@ -8392,8 +8372,7 @@ mod tests { // ── Inbound retry classification from host return codes ── - /// Metadata callback returning the host's "retryable, nothing applied" - /// sentinel. + /// Returns the "retryable, nothing applied" sentinel. extern "C" fn transient_metadata( _ctx: *mut TestCVoid, _wallet_id: *const u8, @@ -8404,7 +8383,7 @@ mod tests { PLATFORM_WALLET_PERSIST_RC_TRANSIENT } - /// Metadata callback returning the host's constraint sentinel. + /// Returns the constraint sentinel. extern "C" fn constraint_metadata( _ctx: *mut TestCVoid, _wallet_id: *const u8, @@ -8415,8 +8394,7 @@ mod tests { PLATFORM_WALLET_PERSIST_RC_CONSTRAINT } - /// Metadata callback returning a plain non-zero value, the way every - /// host written against the original contract does. + /// Returns a plain non-zero value, as a host on the original contract does. extern "C" fn unclassified_metadata( _ctx: *mut TestCVoid, _wallet_id: *const u8, @@ -8435,8 +8413,7 @@ mod tests { 0 } - /// A changeset carrying exactly one payload: the metadata entry, whose - /// callback each test below drives. + /// One payload: the metadata entry whose callback each test drives. fn metadata_changeset() -> PlatformWalletChangeSet { PlatformWalletChangeSet { wallet_metadata: Some(platform_wallet::changeset::WalletMetadataEntry { @@ -8448,8 +8425,8 @@ mod tests { } } - /// Build a persister whose metadata callback is `metadata`, optionally - /// bracketing rounds and attesting atomicity. + /// Persister with metadata callback `metadata`, optionally bracketing + /// rounds and attesting atomicity. fn store_failing_persister( metadata: unsafe extern "C" fn( *mut TestCVoid, @@ -8477,10 +8454,8 @@ mod tests { .kind() } - /// The point of the whole inbound direction: a host that sees its own - /// `SQLITE_BUSY` can say so, and the caller receives a retryable - /// classification instead of the blanket `Fatal` every FFI failure used - /// to collapse into. + /// The point of the inbound direction: a host that sees its own + /// `SQLITE_BUSY` can say so, and the caller learns it may retry. #[test] fn transient_sentinel_reaches_the_caller_from_an_atomic_round() { let persister = store_failing_persister( @@ -8494,10 +8469,9 @@ mod tests { ); } - /// A transient verdict tells the caller to re-send the WHOLE changeset, - /// and changeset vectors merge by appending. Without an all-or-nothing - /// round the failed round may have applied part of itself, so re-sending - /// would duplicate rows — the verdict is withheld and reported fatal. + /// A transient verdict invites re-sending the WHOLE changeset, and + /// changeset vectors merge by appending — so without an all-or-nothing + /// round the re-send would duplicate rows. The verdict is withheld. #[test] fn transient_sentinel_is_withheld_when_the_round_is_not_atomic() { // Brackets wired, but the host never attested atomicity. @@ -8523,8 +8497,7 @@ mod tests { ); } - /// `Constraint` never invites a retry, so it does not depend on the - /// round being atomic and passes through either way. + /// `Constraint` invites no retry, so it passes through either way. #[test] fn constraint_sentinel_survives_whether_or_not_the_round_is_atomic() { for (bracketed, capabilities) in [ @@ -8539,8 +8512,8 @@ mod tests { } } - /// Back-compatibility: a host that returns a plain non-zero value keeps - /// the conservative reading it has always had. + /// Back-compatibility: a plain non-zero value keeps its conservative + /// reading. #[test] fn unclassified_non_zero_return_stays_fatal() { let persister = store_failing_persister( @@ -8554,10 +8527,8 @@ mod tests { ); } - /// One transient callback must never soften a fatal sibling: the round - /// reports the most severe kind any callback returned. Here the commit - /// itself fails unclassified after a per-kind callback reported - /// transient — the round is fatal. + /// The round reports the most severe kind any callback returned: here the + /// commit fails unclassified after a per-kind callback said transient. #[test] fn a_fatal_callback_masks_a_transient_sibling() { extern "C" fn fatal_end( @@ -8585,8 +8556,8 @@ mod tests { ); } - /// A load is one call that either happened or did not, so it carries the - /// host's classification with no atomicity precondition. + /// A load either happened or did not, so it carries the host's + /// classification with no atomicity precondition. #[test] fn transient_sentinel_reaches_the_caller_from_a_load() { extern "C" fn transient_load( @@ -8607,8 +8578,7 @@ mod tests { assert_eq!(err.kind(), Some(PersistenceErrorKind::Transient)); } - /// The two sentinels must stay off the values a host already returns — - /// success, and the plain failure codes the shipping hosts use. + /// The sentinels must stay off the values a host already returns. #[test] fn sentinels_do_not_collide_with_established_return_values() { for taken in [0, 1, -1] { diff --git a/packages/rs-platform-wallet/src/changeset/core_bridge.rs b/packages/rs-platform-wallet/src/changeset/core_bridge.rs index 44714a13286..96f529b7e7f 100644 --- a/packages/rs-platform-wallet/src/changeset/core_bridge.rs +++ b/packages/rs-platform-wallet/src/changeset/core_bridge.rs @@ -224,9 +224,9 @@ impl std::fmt::Display for BatchDiagnostics { /// `Arc

` (not to the `Arc` coercion) to /// actually realize the static-dispatch win. /// -/// The reference is **weak**: the task upgrades it for the duration of each -/// batch commit and holds nothing while idle, so the persister is released -/// as soon as its owner drops rather than when this task next polls. +/// The reference is **weak**: the task upgrades it for each batch commit and +/// holds nothing while idle, so the persister is released when its owner drops +/// rather than when this task next polls. pub fn spawn_wallet_event_adapter

( wallet_manager: Arc>>, persister: Weak

, @@ -431,9 +431,9 @@ async fn run_wallet_event_adapter

( // accounted for" and "nobody knows". let settled: Arc>> = Arc::new(Mutex::new(Vec::new())); let settled_for_commit = Arc::clone(&settled); - // Upgraded per batch and held only for the commit: an idle adapter - // must not keep the persister open, or a manager whose owner dropped - // it stays "open" until this task next polls (issue #4133). + // Held only for the commit: an idle adapter keeping the persister + // open leaves a dropped manager's store "open" until the next poll + // (issue #4133). let Some(persister_for_commit) = persister.upgrade() else { tracing::debug!("persister released; wallet-event adapter exiting"); break; @@ -3442,11 +3442,9 @@ mod tests { } /// The adapter upgrades its weak persister reference for exactly the span - /// of a batch commit, and holds nothing outside it. - /// - /// That span is the sole bound on the manager's synchronous release: a - /// drop racing a commit reclaims the persister when the parked `store()` - /// returns, not immediately (issue #4133). + /// of a batch commit — the sole bound on the manager's synchronous release, + /// since a drop racing a commit reclaims the persister only when the parked + /// `store()` returns (issue #4133). #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn an_in_flight_commit_holds_a_strong_persister_reference() { use std::time::{Duration, Instant}; diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 70a3d40f7ce..9bec72a72d3 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -14,52 +14,26 @@ pub enum PlatformWalletError { #[error("Wallet creation failed: {0}")] WalletCreation(String), - /// The persister failed to load the client start state during - /// rehydration. Carries the typed [`PersistenceError`] so callers keep - /// its retry classification (`is_transient()` / - /// [`PersistenceErrorKind`]) instead of a flattened string — a - /// transient backend hiccup (e.g. `SQLITE_BUSY`) stays distinguishable - /// from a permanent failure and can be retried. + /// The persister failed to load the client start state during rehydration. /// - /// FFI hosts receive the classification too: the boundary maps this - /// variant to result code 49 when the kind is `Transient` and 50 - /// otherwise, so the distinction survives the C ABI rather than - /// flattening to "unknown error". - /// - /// [`PersistenceError`]: crate::changeset::PersistenceError - /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind - /// - /// Construct with [`Self::from_load_failure`]. + /// This and the sibling `Persister*` variants carry their typed + /// [`PersistenceError`](crate::changeset::PersistenceError) rather than a + /// flattened string, so its retry classification survives — a transient + /// `SQLITE_BUSY` stays distinguishable from a permanent failure, in-crate + /// and across the C ABI (`platform-wallet-ffi` maps each variant and kind + /// to its own `PlatformWalletFFIResultCode`). They are separate variants + /// so a failed write is never reported as a failed read. #[error("failed to load persisted client state: {0}")] PersisterLoad(#[source] crate::changeset::PersistenceError), /// The persister failed to store the wallet-registration changeset. - /// Like [`Self::PersisterLoad`], it carries the typed - /// [`PersistenceError`] so the retry classification (`is_transient()` - /// / [`PersistenceErrorKind`]) survives the boundary — a transient - /// `SQLITE_BUSY` stays distinguishable from a permanent failure. - /// Distinct from [`Self::PersisterLoad`] so callers can tell a failed - /// registration write from a failed rehydration read. - /// - /// FFI hosts receive the classification too: the boundary maps this - /// variant to result code 51 (`Transient`), 53 (`Constraint`) or 52 - /// (everything else), so a host can tell a busy store from a rejected - /// row from a broken one without parsing the message. - /// - /// [`PersistenceError`]: crate::changeset::PersistenceError - /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind - /// - /// Construct with [`Self::from_store_failure`]. + /// See [`Self::PersisterLoad`] for why the typed cause is carried. #[error("failed to persist wallet registration changeset: {0}")] PersisterStore(#[source] crate::changeset::PersistenceError), - /// Restoring the persisted platform-address state into the freshly - /// registered wallet failed. Wraps the underlying - /// [`PlatformWalletError`](Self) (boxed to break the recursive type) so - /// its concrete variant and `#[source]` chain survive instead of being - /// flattened into a string. - /// - /// Construct with [`Self::from_restore_failure`], which boxes for you. + /// Restoring persisted platform-address state into a freshly registered + /// wallet failed. Boxed to break the recursion; the inner variant and its + /// `#[source]` chain survive intact. #[error("failed to restore persisted platform-address state: {0}")] PersisterRestore(#[source] Box), @@ -960,35 +934,25 @@ pub enum PlatformWalletError { } impl PlatformWalletError { - /// A persister `load` failed. Wraps the typed cause so its retry - /// classification survives. + /// A persister `load` failed. /// /// There is deliberately no blanket `From`: the - /// conversion is undecidable from the value, because a - /// [`PersistenceError`] does not record whether a load, a store or a - /// flush produced it. Pick the constructor naming the operation that - /// actually failed — an inferred one would silently label failed - /// writes as failed reads. Constructing through these rather than the - /// variants also lets the enum's internals change without touching - /// call sites. - /// - /// [`PersistenceError`]: crate::changeset::PersistenceError + /// conversion is undecidable from the value, because a `PersistenceError` + /// does not record whether a load, a store or a flush produced it, so an + /// inferred one would silently label failed writes as failed reads. Pick + /// the constructor naming the operation that actually failed. pub fn from_load_failure(source: crate::changeset::PersistenceError) -> Self { Self::PersisterLoad(source) } - /// A persister `store` failed. Distinct from - /// [`Self::from_load_failure`] so a failed write is never reported as - /// a failed read. See that constructor for why no blanket conversion - /// exists. + /// A persister `store` failed. See [`Self::from_load_failure`] for why no + /// blanket conversion exists. pub fn from_store_failure(source: crate::changeset::PersistenceError) -> Self { Self::PersisterStore(source) } - /// Restoring persisted platform-address state into a freshly - /// registered wallet failed. Boxes `source` internally, so callers - /// never write `Box::new`. See [`Self::from_load_failure`] for why no - /// blanket conversion exists. + /// Restoring persisted platform-address state failed. Boxes `source`, so + /// callers never write `Box::new`. pub fn from_restore_failure(source: PlatformWalletError) -> Self { Self::PersisterRestore(Box::new(source)) } diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index 498e6fecbb6..c40578f7c23 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -34,14 +34,11 @@ impl PlatformWalletManager

{ /// persister cannot produce the snapshot, or the per-wallet restore error /// when a wallet in it cannot be rebuilt. /// - /// Any `Err` leaves the manager exactly as it was before the call — - /// partial inserts are rolled back — and it stays usable: fix the store - /// and call again, or tear it down and reconstruct. Reconstructing over - /// the same persister path needs the persister released first: - /// [`shutdown`](Self::shutdown) releases it before returning, and a plain - /// drop releases it once the last strong reference goes (the wallet-event - /// adapter holds only a weak one; a batch commit in flight holds a strong - /// one until it finishes). + /// Any `Err` rolls back partial inserts and leaves the manager usable: fix + /// the store and call again, or reconstruct. Reconstructing over the same + /// path needs the persister released first — [`shutdown`](Self::shutdown) + /// does so before returning, a plain drop once the last strong reference + /// goes (only a batch commit in flight holds one). /// /// [`WalletManager`]: key_wallet_manager::WalletManager pub async fn load_from_persistor(&self) -> Result<(), PlatformWalletError> { @@ -647,12 +644,11 @@ mod tests { use crate::test_support::NoopTestEventHandler; /// Strong `Arc

` clones a freshly built [`PlatformWalletManager`] holds: - /// its own `persister` field, the `DashPayPaymentHandler` on the event - /// fan-out, and the `IdentitySyncManager`. The wallet-event adapter is - /// deliberately absent — it keeps a `Weak

` and upgrades per batch. + /// its `persister` field, the `DashPayPaymentHandler`, and the + /// `IdentitySyncManager` — the wallet-event adapter deliberately excluded. const MANAGER_PERSISTER_HOLDERS: usize = 3; - /// Persister whose `load()` always fails — the failure path under test. + /// Persister whose `load()` always fails. struct FailingLoadPersister; impl PlatformWalletPersistence for FailingLoadPersister { @@ -701,8 +697,7 @@ mod tests { } } - /// `load()` fails permanently once and succeeds from then on — the host - /// path of "surface the error, fix the store, call again". + /// Fails `load()` permanently once, then succeeds. #[derive(Default)] struct FatalOnceLoadPersister { load_calls: AtomicUsize, @@ -753,12 +748,9 @@ mod tests { assert_eq!(probe.load_calls.load(Ordering::SeqCst), 2); } - /// The wallet-event adapter must keep a `Weak

`, never a strong clone. - /// /// Isolating by construction: the count is read on a live, idle manager - /// with nothing dropped, cancelled or aborted, so no teardown path and no - /// abort timing can stand in for the property. Restoring a strong `Arc

` - /// in `run_wallet_event_adapter` turns it red. + /// with nothing dropped or aborted, so no teardown path can stand in for + /// the weak-reference property. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn adapter_holds_no_strong_persister_reference() { let persister = Arc::new(FailingLoadPersister); @@ -777,13 +769,10 @@ mod tests { ); } - /// A failed `load_from_persistor` must leave the manager usable: the host - /// fixes its store and calls again. - /// - /// Both failure paths used to run the manager-wide, one-way `shutdown()`, - /// which seals every coordinator's admission gate and joins the - /// wallet-event adapter — so the retry returned `Ok(())` onto a manager - /// that could never sync or persist again (issue #4133). + /// Running the manager-wide, one-way `shutdown()` on this failure path + /// seals every coordinator's admission gate and joins the wallet-event + /// adapter, so the retry returns `Ok(())` onto a manager that can never + /// sync or persist again (#4133). #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn manager_stays_usable_after_a_failed_load() { let manager = make_manager(Arc::new(FatalOnceLoadPersister::default())); @@ -808,10 +797,8 @@ mod tests { makes every later `Ok(())` a lie" ); - // The adapter is the only writer of core wallet events to the - // persister and its receiver is taken exactly once, so a joined - // adapter cannot be respawned: `Ok` here means the reused manager - // still persists. + // The adapter's receiver is taken exactly once, so a joined adapter + // cannot be respawned: `Ok` means the reused manager still persists. let report = manager.shutdown().await; assert_eq!( report.per_worker.get(&WalletWorker::EventAdapter), @@ -821,16 +808,13 @@ mod tests { ); } - /// End to end: a failed `load_from_persistor` surfaces the typed - /// `PersisterLoad` error, and dropping the manager afterwards releases the - /// persister — the precondition for reconstructing on the same path - /// without a spurious `WalletStorageError::AlreadyOpen` masking the real - /// error (issue #4133). + /// Dropping the manager after a failed load releases the persister — the + /// precondition for reconstructing on the same path without a spurious + /// `WalletStorageError::AlreadyOpen` masking the real error (#4133). /// - /// Isolates nothing: the final count is the product of the whole teardown, - /// so it stays green while any one participant regresses as long as - /// another still releases. `adapter_holds_no_strong_persister_reference` - /// is the test that pins the weak adapter reference. + /// Isolates nothing: the count is the product of the whole teardown, so one + /// participant may regress while another still releases. + /// `adapter_holds_no_strong_persister_reference` pins the weak reference. // TODO: cover the composed open -> failed load -> reopen from // platform-wallet-storage; neither side asserts it today. // Multi-thread: dropping the manager runs upstream's `Drop`, whose @@ -864,15 +848,10 @@ mod tests { ); } - /// Dropping the manager without `shutdown` releases the persister - /// **synchronously**: every strong clone lives in the manager's own - /// fields, and the wallet-event adapter holds only a `Weak

`. - /// - /// The one bound: a batch commit in flight upgrades that weak reference - /// for the duration of its `store()`, so a drop racing a commit releases - /// when that commit returns (`an_in_flight_commit_holds_a_strong_persister_reference` - /// in `changeset::core_bridge`). The adapter is idle here, so release is - /// immediate. + /// A dirty drop releases the persister **synchronously**, bounded only by a + /// batch commit in flight (see + /// `an_in_flight_commit_holds_a_strong_persister_reference` in + /// `changeset::core_bridge`); the adapter is idle here. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn dropping_manager_releases_persister_synchronously_when_adapter_idle() { let persister = Arc::new(FailingLoadPersister); diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index a542fa0eceb..faf1cbf7314 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -1056,17 +1056,14 @@ impl PlatformWalletManager

{ } } -/// Drop backstop for the wallet-event adapter task: cancels its token and -/// aborts the task, which a dirty drop would otherwise merely detach. +/// Drop backstop for the wallet-event adapter task, which a dirty drop would +/// otherwise merely detach. /// -/// The persister is released here with the manager's own `Arc

` — the -/// adapter holds a `Weak

` — so a reconstruct on the same path cannot hit a -/// spurious `WalletStorageError::AlreadyOpen` (issue #4133). The one bound: a -/// batch commit in flight has upgraded that weak reference and keeps the -/// persister alive until its `store()` returns. -/// -/// Use [`shutdown`](PlatformWalletManager::shutdown) for a release that is -/// joined rather than aborted. +/// The persister is released here with the manager's own `Arc

` — the adapter +/// holds only a `Weak

` — so a reconstruct on the same path cannot hit a +/// spurious `WalletStorageError::AlreadyOpen` (issue #4133), bounded only by a +/// batch commit in flight holding its upgrade until `store()` returns. Use +/// [`shutdown`](PlatformWalletManager::shutdown) to join rather than abort. impl Drop for PlatformWalletManager

{ fn drop(&mut self) { self.event_adapter_cancel.cancel(); diff --git a/packages/rs-platform-wallet/src/manager/persist_retry.rs b/packages/rs-platform-wallet/src/manager/persist_retry.rs index 90f93cee2b4..83a8410d609 100644 --- a/packages/rs-platform-wallet/src/manager/persist_retry.rs +++ b/packages/rs-platform-wallet/src/manager/persist_retry.rs @@ -1,12 +1,12 @@ //! Bounded retry for transient persister *reads*. //! //! Only `load` is retried in-crate: it is idempotent and the crate owns both -//! ends. Writes are never retried here — a failed `store` propagates typed -//! and kind-classified, and the caller decides. +//! ends. A failed `store` propagates typed and kind-classified instead, and the +//! caller decides. //! //! Each attempt runs on the blocking pool; worst case per call is -//! `attempts × backend timeout + Σ backoff` (SQLite `busy_timeout` defaults -//! to 5 s). +//! `attempts × backend timeout + Σ backoff` (SQLite `busy_timeout` defaults to +//! 5 s). use std::sync::Arc; use std::time::Duration; @@ -21,13 +21,12 @@ pub(crate) const LOAD_RETRY_BACKOFF: [Duration; 3] = [ Duration::from_millis(80), ]; -/// Retry a synchronous persister `load` while it fails *transiently*, off -/// the async runtime, on the fixed [`LOAD_RETRY_BACKOFF`] schedule. +/// Retry a synchronous persister `load` while it fails *transiently*, off the +/// async runtime, on the fixed [`LOAD_RETRY_BACKOFF`] schedule. /// -/// `op` runs on the blocking pool once per attempt. A fatal error (or -/// success) returns immediately — a fatal failure never retries. A panic -/// inside `op` propagates to the caller; a cancelled attempt (runtime -/// shutting down) surfaces as a backend error instead of panicking. +/// `op` runs on the blocking pool once per attempt; success or a fatal error +/// returns immediately. A panic inside `op` propagates to the caller; a +/// cancelled attempt (runtime shutting down) surfaces as a backend error. pub(crate) async fn retry_transient_load(op: F) -> Result where F: Fn() -> Result + Send + Sync + 'static, diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 6e314df5608..49db0183a5b 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -971,8 +971,8 @@ impl PlatformWalletManager /// /// Mirrors what `discover` publishes for itself, retry policy included; /// needed separately because a scan dropped mid-await never reaches its own - /// bookkeeping. This is the verdict least affordable to lose — it is the - /// one that re-opens the identity question on the next launch. + /// bookkeeping. This is the verdict least affordable to lose — the one that + /// re-opens the identity question on the next launch. async fn record_identity_scan_cut_off(&self, wallet_id: &WalletId) { // Coverage of nothing: the scan was dropped mid-await, so it answered // no index and may not clear one an earlier scan left open. diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 961fe143703..d2c690a2b50 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -483,9 +483,8 @@ impl PlatformWalletManager

{ } } - // Persist the registration changeset. `store` is not retried here — - // the caller receives the typed, kind-classified `PersistenceError` - // (its transient/fatal classification preserved) and decides. + // `store` is not retried here: the caller receives the typed, + // kind-classified `PersistenceError` and decides. if let Err(e) = self.persister.store(wallet_id, registration_changeset) { tracing::error!( wallet_id = %hex::encode(wallet_id), @@ -532,9 +531,8 @@ impl PlatformWalletManager

{ // poisoning every retry on `WalletAlreadyExists`. Roll back // before bailing — same shape as `manager::load`. // `load` is an idempotent read, so a transient blip is retried - // in-crate — unlike `store` above, which the caller decides on. - // Clone the per-wallet persister handle rather than moving - // `platform_wallet` itself, which is still needed below. + // in-crate — unlike `store` above. Clone the persister handle rather + // than moving `platform_wallet`, still needed below. let load_persister = platform_wallet.persister().clone(); let load_result = super::retry_transient_load(move || load_persister.load()).await; let crate::changeset::ClientStartState { @@ -582,8 +580,7 @@ impl PlatformWalletManager

{ "rollback: remove_wallet failed while unwinding a failed wallet setup" ); } - // `initialize_from_persisted` already returns a typed - // `PlatformWalletError`; wrap (boxed) rather than stringify so + // Wrap the already-typed error rather than stringify it, so // its concrete variant and source chain survive. return Err(PlatformWalletError::from_restore_failure(e)); } @@ -1314,8 +1311,8 @@ mod register_wallet_duplicate_tests { #[cfg(test)] mod persist_retry_tests { - //! Registration-path persistence: single-attempt `store` with typed - //! error propagation, bounded `load` retry, and log-level policy. + //! Registration-path persistence: single-attempt `store`, bounded `load` + //! retry, typed error propagation, log-level policy. use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; @@ -1353,37 +1350,30 @@ mod persist_retry_tests { use crate::test_support::tracing_capture::{RecordedEvents, RecordingGuard}; - /// Persister whose `store` / `flush` / `load` outcomes are scripted so - /// the registration path can be driven deterministically. + /// Persister with scripted `store` / `flush` / `load` outcomes. /// - /// `store` counts registration and identity-scan-verdict writes - /// separately. Registration ends with a best-effort `identity().sync()`, - /// so a successful registration issues a SECOND `store` carrying the scan - /// verdict; a single counter would make every assertion about the - /// registration write depend on unrelated discovery behaviour. The - /// changeset itself is the discriminator. + /// `store` counts registration and scan-verdict writes separately, + /// discriminated by the changeset: registration ends with a best-effort + /// `identity().sync()` that issues a SECOND `store`, and a single counter + /// would couple every registration-write assertion to discovery. #[derive(Default)] struct FaultyPersister { /// Stores of the registration changeset. registration_store_calls: AtomicUsize, /// Stores of the identity-scan verdict published by `identity().sync()`. scan_verdict_store_calls: AtomicUsize, - /// Never scripted to fail — every assertion here expects this to - /// stay 0, since a `store` failure is never retried through it. + /// Never scripted to fail: a `store` failure is never retried through + /// it, so every assertion expects 0. flush_calls: AtomicUsize, load_calls: AtomicUsize, - /// The registration `store` call fails transiently. store_transient: bool, - /// The registration `store` call fails fatally. store_fatal: bool, - /// Number of leading scan-verdict `store` calls that fail transiently. + /// Leading scan-verdict `store` calls that fail transiently. scan_verdict_store_transient_failures: usize, - /// Number of leading `load` calls that fail transiently. + /// Leading `load` calls that fail transiently. load_transient_failures: usize, - /// Every `load` fails fatally (must NOT retry). load_fatal: bool, - /// After `load_transient_failures` transient failures, fail fatally - /// instead of succeeding. + /// Fail fatally after `load_transient_failures`, instead of succeeding. load_then_fatal: bool, } @@ -1393,13 +1383,9 @@ mod persist_retry_tests { _wallet_id: WalletId, changeset: PlatformWalletChangeSet, ) -> Result<(), PersistenceError> { - // One changeset can carry both: `merge` folds a buffered - // registration write and a scan verdict into a single round. Each - // counter answers only its own question — "was this changeset - // handed over?" — so both increment. Letting the first match win - // would make an assertion about the registration write depend on - // whether discovery happened to be batched with it, which is the - // coupling these separate counters exist to remove. + // `merge` can fold a registration write and a scan verdict into + // one round, so both counters increment; letting the first match + // win would reintroduce the batching dependency. let registration = changeset .wallet_metadata .is_some() @@ -1409,9 +1395,8 @@ mod persist_retry_tests { .is_some() .then(|| self.scan_verdict_store_calls.fetch_add(1, Ordering::SeqCst)); - // The registration half decides a combined round's outcome: its - // failure aborts the whole registration, while a verdict's is - // swallowed. + // The registration half decides a combined round: its failure + // aborts registration, a verdict's is swallowed. if registration.is_some() { if self.store_fatal { return Err(fatal()); @@ -1462,8 +1447,7 @@ mod persist_retry_tests { .to_seed("") } - /// `Some(0)` skips the SPV-tip birth-height lookup so the test never - /// consults SPV. + /// `Some(0)` skips the SPV-tip birth-height lookup. async fn register( manager: &PlatformWalletManager, ) -> Result<(), PlatformWalletError> { @@ -1478,9 +1462,7 @@ mod persist_retry_tests { .map(|_| ()) } - /// A transient `store` failure surfaces to the caller on the first - /// attempt — never retried via `flush` — and rolls the in-memory - /// registration back. + /// Surfaces on the first attempt, and rolls the in-memory insert back. #[tokio::test] async fn transient_store_failure_surfaces_as_persister_store_without_retry() { let persister = Arc::new(FaultyPersister { @@ -1517,9 +1499,7 @@ mod persist_retry_tests { ); } - /// A fatal `store` failure fails fast — no retry — and - /// surfaces as the typed `PersisterStore` whose inner classification is - /// non-transient. + /// A fatal `store` failure fails fast, keeping its classification. #[tokio::test] async fn fatal_store_failure_fails_fast_without_retry() { let persister = Arc::new(FaultyPersister { @@ -1552,8 +1532,7 @@ mod persist_retry_tests { ); } - /// A transient `load` blip during rehydration is retried (an - /// idempotent read), so registration succeeds. + /// A transient `load` blip is retried — it is an idempotent read. #[tokio::test] async fn transient_load_failure_is_retried_and_succeeds() { let persister = Arc::new(FaultyPersister { @@ -1576,8 +1555,6 @@ mod persist_retry_tests { ); } - /// A fatal `load` fails fast and surfaces as the typed - /// `PersisterLoad` — never the flattened `WalletCreation(String)`. #[tokio::test] async fn fatal_load_failure_surfaces_as_persister_load() { let persister = Arc::new(FaultyPersister { @@ -1601,9 +1578,6 @@ mod persist_retry_tests { ); } - /// A load that turns fatal after riding out a transient blip surfaces - /// the fatal classification, not the earlier transient one, after - /// exactly the two calls that produced it. #[tokio::test] async fn transient_then_fatal_load_surfaces_as_persister_load_fatal() { let persister = Arc::new(FaultyPersister { @@ -1629,9 +1603,7 @@ mod persist_retry_tests { assert_eq!(persister.load_calls.load(Ordering::SeqCst), 2); } - /// The load-retry schedule sleeps `[20, 40, 80]` ms across the 4 total - /// attempts it allows for an always-transient failure — driven with - /// virtual time so the test itself doesn't wait 140 ms. + /// Virtual time, so the test itself doesn't wait the schedule's 140 ms. #[tokio::test(start_paused = true)] async fn transient_load_retry_follows_the_backoff_schedule() { let calls = Arc::new(AtomicUsize::new(0)); @@ -1657,11 +1629,9 @@ mod persist_retry_tests { assert_eq!(tokio::time::Instant::now() - start, expected); } - /// A transient failure persisting the identity-scan verdict is logged - /// and swallowed on the first attempt — never retried — so a merely busy - /// backend costs the verdict its durability this launch - /// (dashpay/platform#4365) rather than failing the registration that - /// just succeeded. + /// A busy backend costs the scan verdict its durability this launch + /// (dashpay/platform#4365) rather than failing the registration that just + /// succeeded: logged and swallowed, never retried. #[tokio::test] async fn transient_scan_verdict_store_failure_is_logged_not_retried() { let persister = Arc::new(FaultyPersister { @@ -1696,8 +1666,6 @@ mod persist_retry_tests { ); } - /// An unpersistable verdict never escalates into failing the registration - /// that just succeeded. #[tokio::test] async fn unpersistable_scan_verdict_does_not_fail_registration() { let persister = Arc::new(FaultyPersister { @@ -1714,15 +1682,13 @@ mod persist_retry_tests { assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 0); } - /// The typed persister-phase variants preserve retry - /// classification, enable structural matching, and keep the `#[source]` - /// chain instead of flattening to a string. + /// The typed variants preserve retry classification, allow structural + /// matching, and keep the `#[source]` chain. /// - /// Also pins the named constructors to the operation each is named - /// for. That is the whole reason no blanket `From` - /// exists: the same value can come from a load, a store or a flush, so - /// only the call site knows which variant is truthful, and an inferred - /// conversion reports failed writes as failed reads. + /// Also pins each constructor to the operation it names — the reason no + /// blanket `From` exists: only the call site knows + /// whether a load, a store or a flush produced the value, so an inferred + /// conversion would report failed writes as failed reads. #[test] fn typed_variants_preserve_classification_matching_and_source() { use std::error::Error; @@ -1744,8 +1710,7 @@ mod persist_retry_tests { } assert!(load_err.source().is_some()); - // The restore variant wraps a typed inner error; structural matching - // must recover the concrete inner variant, not an opaque string. + // Structural matching must recover the concrete inner variant. let restore_err = PlatformWalletError::from_restore_failure(PlatformWalletError::WalletLocked); assert!(restore_err.source().is_some()); @@ -1756,10 +1721,8 @@ mod persist_retry_tests { other => panic!("expected PersisterRestore, got {other:?}"), } - // The two persister-error constructors take the SAME input type, so - // nothing but the call site distinguishes them — mixing them up is - // silent, and is exactly the defect the removed blanket conversion - // produced downstream. + // Both take the SAME input type, so only the call site distinguishes + // them and a mix-up is silent. assert!( matches!( PlatformWalletError::from_store_failure(fatal()), diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index 70967a37abb..f7ed7e27938 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -650,8 +650,7 @@ impl crate::changeset::PlatformWalletPersistence for NoopTestPersister { } } -/// Event handler that ignores every event — for tests whose subject is not -/// the event fan-out. +/// Event handler that ignores every event. pub(crate) struct NoopTestEventHandler; impl crate::events::EventHandler for NoopTestEventHandler {} impl crate::events::PlatformEventHandler for NoopTestEventHandler {} @@ -785,13 +784,10 @@ pub(crate) async fn mnemonic_wallet_manager( ) } -/// Thread-scoped `tracing` event capture for tests that assert on log -/// output. +/// Thread-scoped `tracing` event capture for tests that assert on log output. /// -/// Shared because the naive approach is a trap: a per-test -/// `tracing::subscriber::set_default` swap is flaky under `cargo test`'s -/// parallel harness, so every capturing test must route through the one -/// globally-installed subscriber here rather than installing its own. +/// Every capturing test must route through the one globally-installed +/// subscriber here rather than installing its own — see [`RecorderRouter`]. #[cfg(test)] pub(crate) mod tracing_capture { use std::cell::RefCell; @@ -802,9 +798,8 @@ pub(crate) mod tracing_capture { use tracing_subscriber::layer::{Context, SubscriberExt}; use tracing_subscriber::Layer; - /// Captures the level and message of every `tracing` event recorded - /// while registered as the active recorder for the current thread (see - /// [`RecordingGuard`]). + /// Level and message of every event recorded while registered as the + /// current thread's active recorder (see [`RecordingGuard`]). #[derive(Clone, Default)] pub(crate) struct RecordedEvents(Arc>>); @@ -832,26 +827,23 @@ pub(crate) mod tracing_capture { } thread_local! { - /// The [`RecordedEvents`] a test on THIS thread wants routed to it, - /// if any. Set/cleared only by [`RecordingGuard`]. + /// Where events from THIS thread go. Set only by [`RecordingGuard`]. static ACTIVE_RECORDER: RefCell> = const { RefCell::new(None) }; } - /// Routes every event to whichever [`RecordedEvents`] is registered for - /// the emitting thread, via [`ACTIVE_RECORDER`]. Installed as the - /// process-wide default exactly once — never per-test. + /// Routes every event to whichever [`RecordedEvents`] the emitting thread + /// registered in [`ACTIVE_RECORDER`]. Installed as the process-wide + /// default exactly once — never per-test. /// /// A per-test `tracing::subscriber::set_default` swap is flaky under - /// `cargo test`'s parallel harness: tracing's per-callsite `Interest` - /// cache is process-global, and a concurrently-running test's own - /// swap/drop can race the interest rebuild your swap triggers, so the - /// event silently never reaches your subscriber even though dispatch - /// itself stays correctly on your own thread (confirmed: the emitting - /// thread ID matched the installing thread ID on a captured failure). - /// Installing the routing subscriber once, before any callsite is ever - /// hit, sidesteps the race — routing then happens through an ordinary - /// thread-local this code owns, not through tracing's default-swap - /// machinery. + /// `cargo test`'s parallel harness: tracing's per-callsite `Interest` cache + /// is process-global, so a concurrent test's swap/drop can race the + /// interest rebuild yours triggers and the event silently never reaches + /// your subscriber — even though dispatch stays correctly on your own + /// thread (confirmed: emitting and installing thread IDs matched on a + /// captured failure). Installing once, before any callsite is hit, + /// sidesteps the race: routing then goes through a thread-local this code + /// owns rather than tracing's default-swap machinery. struct RecorderRouter; impl Layer for RecorderRouter { @@ -866,17 +858,16 @@ pub(crate) mod tracing_capture { static GLOBAL_ROUTER_INIT: OnceLock<()> = OnceLock::new(); - /// Scopes [`ACTIVE_RECORDER`] to `recorder` for the current thread, for - /// the guard's lifetime. + /// Scopes [`ACTIVE_RECORDER`] to `recorder` for this thread and lifetime. pub(crate) struct RecordingGuard; impl RecordingGuard { pub(crate) fn install(recorder: RecordedEvents) -> Self { GLOBAL_ROUTER_INIT.get_or_init(|| { let subscriber = tracing_subscriber::registry().with(RecorderRouter); - // Another thread may have already won this race; either - // way, the routing subscriber is the process-wide default - // by the time `get_or_init` returns to any caller. + // Another thread may have won this race; either way the + // routing subscriber is the process-wide default by the time + // `get_or_init` returns to any caller. let _ = tracing::subscriber::set_global_default(subscriber); }); ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = Some(recorder)); diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs index 05b8c8b0acd..43e5252d530 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs @@ -143,20 +143,16 @@ pub(in crate::wallet::asset_lock) fn record_holds_local_finality( } } -/// Variant of [`record_or_persister`] for poll loops: never aborts the -/// wait, whatever the persister does. +/// Variant of [`record_or_persister`] for poll loops: never aborts the wait, +/// whatever the persister does. /// -/// This read is a FALLBACK for records the in-memory map evicted; the live -/// SPV stream can still deliver the record and end the wait. So a failure -/// here reads as a miss and the loop keeps waiting, bounded by its own -/// finality timeout — aborting would turn a degraded read path into a -/// failed operation. +/// This read is a FALLBACK for records the in-memory map evicted — the live +/// SPV stream can still deliver one — so any failure reads as a miss and the +/// loop keeps waiting, bounded by its own finality timeout. /// -/// A transient failure is a miss and nothing more; the next iteration -/// retries it. A permanent one is a miss too, but is reported once per -/// wait via `reported` — per-iteration logging would let a broken backend -/// flood the log from inside a loop, and the condition is the same one -/// every time. +/// A permanent failure is reported once per wait via `reported`: per-iteration +/// logging would let a broken backend flood the log from inside an unbounded +/// poll loop, saying the same thing every time. pub(super) fn record_or_persister_for_poll( in_memory: Option, persister: &crate::wallet::persister::WalletPersister, @@ -180,8 +176,8 @@ pub(super) fn record_or_persister_for_poll( } } -/// The transient half of the poll policy, split out so the permanent arm -/// above owns the once-per-wait reporting. +/// The transient half of the poll policy, split out so the permanent arm owns +/// the once-per-wait reporting. fn persister_read_for_poll( in_memory: Option, persister: &crate::wallet::persister::WalletPersister, @@ -392,8 +388,7 @@ impl AssetLockManager { use key_wallet::transaction_checking::TransactionContext; let deadline = timeout.map(|t| tokio::time::Instant::now() + t); - // Once-per-wait guard for the tx-record fallback read (see - // `record_or_persister_for_poll`). + // Once-per-wait guard; see `record_or_persister_for_poll`. let mut read_failure_reported = false; loop { @@ -487,8 +482,7 @@ impl AssetLockManager { tracing::info!(outpoint = %out_point, ?timeout, "wait_for_proof: entered"); let deadline = timeout.map(|t| tokio::time::Instant::now() + t); let mut iter: u32 = 0; - // Once-per-wait guard for the tx-record fallback read (see - // `record_or_persister_for_poll`). + // Once-per-wait guard; see `record_or_persister_for_poll`. let mut read_failure_reported = false; // Read account_index and transaction from the tracked lock. @@ -1008,7 +1002,7 @@ mod tests { } } - /// Test persister that returns a permanent `get_core_tx_record` error. + /// Persister with a permanent `get_core_tx_record` failure. struct ErroringStore; impl PlatformWalletPersistence for ErroringStore { @@ -1134,14 +1128,8 @@ mod tests { assert!(resolved.is_err()); } - /// A poll loop must DEGRADE on a permanent read failure, not abort. - /// - /// The persister read is a fallback for records the in-memory map - /// evicted; the live SPV stream can still deliver the record and end - /// the wait. Aborting turns a degraded read path into a failed - /// operation, and the wait is already bounded by its finality timeout. - /// The failure is reported once per wait rather than once per - /// iteration, so a broken backend cannot flood the log from a loop. + /// A poll loop degrades on a permanent read failure rather than aborting: + /// the live SPV stream can still end the wait. #[test] fn poll_read_degrades_to_a_miss_on_permanent_backend_errors() { let unknown_txid = Txid::from([0xFF; 32]); @@ -1163,14 +1151,11 @@ mod tests { assert!(still_reported); } - /// The permanent-failure report fires ONCE per wait, not once per - /// iteration. + /// The report fires ONCE per wait: a poll loop spins many times against the + /// same broken backend, and per-iteration reporting buries the log. /// - /// A poll loop can spin many times against the same broken backend, so - /// reporting per iteration would bury the log under one repeated line - /// while saying nothing new. Counting the events is the point: an - /// assertion that merely finds a report present passes just as happily - /// when every iteration emits one. + /// Counting is the point — an assertion that merely finds a report present + /// passes just as happily when every iteration emits one. #[test] fn poll_read_reports_a_permanent_failure_once_per_wait_not_once_per_iteration() { use crate::test_support::tracing_capture::{RecordedEvents, RecordingGuard}; @@ -1204,9 +1189,7 @@ mod tests { ); } - /// A transient failure is a miss for this iteration and is NOT worth - /// the once-per-wait permanent-failure report — the next iteration - /// retries it. + /// A transient failure must not consume the once-per-wait report. #[test] fn poll_read_treats_transient_backend_errors_as_a_silent_miss() { let unknown_txid = Txid::from([0xFF; 32]); @@ -1221,8 +1204,7 @@ mod tests { ); } - /// The shared read helper collapses a transient failure into a miss so - /// every caller gets one policy, and leaves permanent failures visible. + /// The shared helper collapses transient failures, not permanent ones. #[test] fn transient_miss_read_helper_separates_transient_from_permanent() { let unknown_txid = Txid::from([0xFF; 32]); diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs index abf4c1eeed1..367ecd51e73 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -643,10 +643,9 @@ impl IdentityWallet { /// survival across a restart, and it must not be allowed to fail the scan /// that just succeeded. /// - /// `store` is a single attempt — not retried here, per the caller-decides - /// persister-error policy — so a merely busy backend (dashpay/platform#4365) - /// costs the verdict its durability this launch; the outcome is logged and - /// swallowed either way. + /// `store` is a single attempt, per the caller-decides persister-error + /// policy, so a merely busy backend (dashpay/platform#4365) costs the + /// verdict its durability this launch. Logged and swallowed either way. async fn publish_scan_verdict( &self, wallet_id: crate::wallet::platform_wallet::WalletId, diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index 9e0a9bc3418..daf85b6a7ef 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -227,11 +227,10 @@ impl DashPayView<'_, B> { /// /// # Errors /// - /// Transient tx-record read failures leave the scan incomplete so the - /// guard stays unstamped and the next sweep retries; permanent ones - /// return [`PlatformWalletError::PersisterLoad`]. Retrying a permanent - /// failure every sweep would never succeed and would never be - /// reported. + /// Transient tx-record read failures leave the scan incomplete, so the + /// guard stays unstamped and the next sweep retries. Permanent ones return + /// [`PlatformWalletError::PersisterLoad`] rather than deferring: retrying + /// them every sweep would never succeed and never be reported. pub async fn reconcile_sent_payments_from_tx_history( &self, ) -> Result { @@ -435,9 +434,8 @@ impl DashPayView<'_, B> { .collect(), }); } - // Either the row is genuinely unreadable yet, or a - // transient failure already read as a miss. Both mean the - // same thing here: retry on the next sweep. + // Not readable yet, or a transient failure read as a miss. + // Both mean: retry on the next sweep. Ok(None) => { incomplete_scan = true; tracing::debug!( @@ -446,8 +444,7 @@ impl DashPayView<'_, B> { ); } // A permanent failure will not fix itself, so deferring it - // re-runs the whole sweep on every sync forever and never - // says why. Same policy as the confirmation sweep. + // re-runs the whole sweep on every sync forever, silently. Err(e) => return Err(PlatformWalletError::from_load_failure(e)), } } @@ -1706,8 +1703,7 @@ mod tests { key_wallet::managed_account::transaction_record::TransactionRecord, >, >, - /// `Some(kind)` makes every `get_core_tx_record` fail with that - /// error class instead of answering from `records`. + /// `Some(kind)` fails every `get_core_tx_record` with that class. read_error_kind: Mutex>, /// Txids the enumeration lists but `get_core_tx_record` answers /// `Ok(None)` for — the FFI shape for "row exists, record not @@ -3701,13 +3697,9 @@ mod tests { ); } - /// A permanent tx-record read failure surfaces from the reconstruction - /// sweep; only a transient one is folded into "incomplete, retry next - /// time". - /// - /// The distinction is what stops a permanently unreadable store from - /// re-running the whole sweep on every dashpay sync, indefinitely and - /// silently. Same policy as the confirmation sweep. + /// Only a transient failure folds into "incomplete, retry next time" — the + /// distinction stops a permanently unreadable store from silently + /// re-running the whole sweep on every dashpay sync. #[tokio::test] async fn reconcile_sent_payments_from_tx_history_surfaces_permanent_read_failures() { use dashcore::hashes::Hash; diff --git a/packages/rs-platform-wallet/src/wallet/persister.rs b/packages/rs-platform-wallet/src/wallet/persister.rs index 1fdf396f9f9..c66a829b6a9 100644 --- a/packages/rs-platform-wallet/src/wallet/persister.rs +++ b/packages/rs-platform-wallet/src/wallet/persister.rs @@ -64,17 +64,14 @@ impl WalletPersister { self.inner.get_core_tx_record(self.wallet_id, txid) } - /// [`Self::get_core_tx_record`] with the shared transient-as-miss - /// read policy applied. + /// [`Self::get_core_tx_record`] with the shared transient-as-miss policy. /// - /// A transient backend failure (a busy store) is indistinguishable in - /// outcome from "the row is not readable right now", and every caller - /// of this read already handles a miss by retrying on its next pass — - /// so it collapses to `Ok(None)` and is logged at debug. A permanent - /// failure stays an `Err`: it will not fix itself, so a caller that - /// swallowed it would repeat the same doomed work forever with no - /// signal. Callers that need to tell the two apart use - /// [`Self::get_core_tx_record`] directly. + /// A busy store is indistinguishable in outcome from "the row is not + /// readable right now", and every caller here already retries a miss on its + /// next pass, so a transient failure collapses to `Ok(None)`. A permanent + /// one stays an `Err`: it will not fix itself, so swallowing it would + /// repeat the same doomed work forever with no signal. Use + /// [`Self::get_core_tx_record`] directly to tell the two apart. pub(crate) fn get_core_tx_record_or_transient_miss( &self, txid: &Txid,