diff --git a/Cargo.lock b/Cargo.lock index c0fa046121..2d00518243 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2504,16 +2504,6 @@ dependencies = [ "futures-core", ] -[[package]] -name = "fs2" -version = "0.4.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213" -dependencies = [ - "libc", - "winapi", -] - [[package]] name = "fs_extra" version = "1.3.0" @@ -4949,7 +4939,7 @@ dependencies = [ [[package]] name = "platform-wallet-storage" -version = "3.1.0-dev.5" +version = "3.1.0-dev.6" dependencies = [ "assert_cmd", "bincode", @@ -4959,7 +4949,6 @@ dependencies = [ "dashcore", "dpp", "filetime", - "fs2", "hex", "humantime", "key-wallet", @@ -4976,6 +4965,7 @@ dependencies = [ "static_assertions", "tempfile", "thiserror 1.0.69", + "tokio", "tracing", "tracing-subscriber", "tracing-test", diff --git a/packages/rs-platform-wallet-ffi/src/persistence.rs b/packages/rs-platform-wallet-ffi/src/persistence.rs index 072a0ea50a..e18440ee58 100644 --- a/packages/rs-platform-wallet-ffi/src/persistence.rs +++ b/packages/rs-platform-wallet-ffi/src/persistence.rs @@ -1233,11 +1233,9 @@ impl PlatformWalletPersistence for FFIPersister { } if !round_success { - return Err( - "one or more persistence callbacks failed; changeset was rolled back" - .to_string() - .into(), - ); + return Err(PersistenceError::backend( + "one or more persistence callbacks failed; changeset was rolled back", + )); } // Merge into pending changesets. @@ -1251,9 +1249,10 @@ impl PlatformWalletPersistence for FFIPersister { if let Some(cb) = self.callbacks.on_store_fn { let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr()) }; if result != 0 { - return Err( - format!("Persistence store callback returned error code {}", result).into(), - ); + return Err(PersistenceError::backend(format!( + "Persistence store callback returned error code {}", + result + ))); } } @@ -1265,9 +1264,10 @@ impl PlatformWalletPersistence for FFIPersister { 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( - format!("Persistence flush callback returned error code {}", result).into(), - ); + return Err(PersistenceError::backend(format!( + "Persistence flush callback returned error code {}", + result + ))); } } @@ -1293,7 +1293,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(format!("on_load_wallet_list_fn returned error code {}", rc).into()); + return Err(PersistenceError::backend(format!( + "on_load_wallet_list_fn returned error code {}", + rc + ))); } let _guard = LoadGuard { context: self.callbacks.context, @@ -2267,14 +2270,17 @@ fn build_wallet_start_state( let xpub_bytes = unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) }; let (account_xpub, _): (ExtendedPubKey, usize) = - bincode::decode_from_slice(xpub_bytes, config::standard()) - .map_err(|e| format!("failed to decode account xpub: {}", e))?; + bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| { + PersistenceError::backend(format!("failed to decode account xpub: {}", e)) + })?; let account = Account::from_xpub(Some(entry.wallet_id), account_type, account_xpub, network) - .map_err(|e| format!("Account::from_xpub failed: {:?}", e))?; - accounts - .insert(account) - .map_err(|e| format!("AccountCollection::insert failed: {}", e))?; + .map_err(|e| { + PersistenceError::backend(format!("Account::from_xpub failed: {:?}", e)) + })?; + accounts.insert(account).map_err(|e| { + PersistenceError::backend(format!("AccountCollection::insert failed: {}", e)) + })?; } // External-signable wallet — the mnemonic / seed lives in the @@ -2915,7 +2921,7 @@ fn build_unused_asset_locks( for spec in specs { // Decode the outpoint: 32-byte raw txid + 4-byte LE vout. let txid = dashcore::Txid::from_slice(&spec.out_point[..32]).map_err(|e| { - PersistenceError::from(format!( + PersistenceError::backend(format!( "tracked asset lock: invalid txid in outpoint: {}", e )) @@ -2928,8 +2934,8 @@ fn build_unused_asset_locks( // Decode the consensus-encoded transaction. if spec.transaction_bytes.is_null() || spec.transaction_bytes_len == 0 { - return Err(PersistenceError::from( - "tracked asset lock: empty transaction bytes".to_string(), + return Err(PersistenceError::backend( + "tracked asset lock: empty transaction bytes", )); } // SAFETY: Swift guarantees the buffer is valid for the @@ -2939,7 +2945,7 @@ fn build_unused_asset_locks( unsafe { slice::from_raw_parts(spec.transaction_bytes, spec.transaction_bytes_len) }; let transaction: dashcore::Transaction = dashcore::consensus::deserialize(tx_bytes) .map_err(|e| { - PersistenceError::from(format!( + PersistenceError::backend(format!( "tracked asset lock: failed to decode transaction: {}", e )) @@ -2959,7 +2965,10 @@ fn build_unused_asset_locks( config::standard(), ) .map_err(|e| { - PersistenceError::from(format!("tracked asset lock: failed to decode proof: {}", e)) + PersistenceError::backend(format!( + "tracked asset lock: failed to decode proof: {}", + e + )) })?; Some(proof) }; @@ -3010,7 +3019,7 @@ fn funding_type_from_u8( 4 => AssetLockFundingType::AssetLockAddressTopUp, 5 => AssetLockFundingType::AssetLockShieldedAddressTopUp, other => { - return Err(PersistenceError::from(format!( + return Err(PersistenceError::backend(format!( "tracked asset lock: unknown funding_type discriminant {}", other ))) @@ -3027,7 +3036,7 @@ fn status_from_u8(b: u8) -> Result AssetLockStatus::ChainLocked, 4 => AssetLockStatus::Consumed, other => { - return Err(PersistenceError::from(format!( + return Err(PersistenceError::backend(format!( "tracked asset lock: unknown status discriminant {}", other ))) @@ -3190,7 +3199,7 @@ fn account_type_from_spec(spec: &AccountSpecFFI) -> Result Result { let standard_tag = StandardAccountTypeTagFFI::try_from_u8(spec.standard_tag) .ok_or_else(|| { - PersistenceError::Backend(format!( + PersistenceError::backend(format!( "AccountSpecFFI(Standard) carries unknown standard_tag byte {}", spec.standard_tag )) @@ -3254,7 +3263,7 @@ fn account_type_from_spec(spec: &AccountSpecFFI) -> Result { - return Err(PersistenceError::Backend(format!( + return Err(PersistenceError::backend(format!( "AccountTypeTagFFI {:?} is no longer mappable to a key-wallet AccountType after the upstream event-bus refactor (TODO(events))", type_tag ))); @@ -3357,10 +3366,10 @@ fn restore_unresolved_asset_lock_tx_records( let context = match rec.context_raw { 2 => { let block_hash = dashcore::BlockHash::from_slice(&rec.block_hash).map_err(|e| { - format!( + PersistenceError::backend(format!( "load: malformed block_hash on unresolved asset-lock tx record: {}", e - ) + )) })?; TransactionContext::InBlock(BlockInfo::new( rec.block_height, @@ -3370,10 +3379,10 @@ fn restore_unresolved_asset_lock_tx_records( } 3 => { let block_hash = dashcore::BlockHash::from_slice(&rec.block_hash).map_err(|e| { - format!( + PersistenceError::backend(format!( "load: malformed block_hash on unresolved asset-lock tx record: {}", e - ) + )) })?; TransactionContext::InChainLockedBlock(BlockInfo::new( rec.block_height, diff --git a/packages/rs-platform-wallet-storage/Cargo.toml b/packages/rs-platform-wallet-storage/Cargo.toml index d704de1d59..14c1bce76b 100644 --- a/packages/rs-platform-wallet-storage/Cargo.toml +++ b/packages/rs-platform-wallet-storage/Cargo.toml @@ -16,19 +16,24 @@ path = "src/bin/platform-wallet-storage.rs" required-features = ["cli"] [dependencies] -# Cross-cutting deps (always on) -platform-wallet = { path = "../rs-platform-wallet", features = ["serde"] } -serde = { version = "1", features = ["derive"] } +# Truly cross-cutting deps (always on regardless of features). thiserror = "1" tracing = "0.1" hex = "0.4" # SQLite-backed persister deps (gated by the `sqlite` feature). +# `platform-wallet` types are reachable through the `sqlite` submodule +# only; without the feature the bare crate ships no items that mention +# them, so the wallet/serde graph stays out of the build (CODE-020). # `dpp` types reach the persister via `IdentityPublicKey` (identity_keys # writer), `AssetLockProof` (asset_locks writer) and `Identifier` # (dashpay writer). `dash-sdk` is here for the `AddressFunds` re-export # in `schema/platform_addrs.rs`. Feature set mirrors sibling # `rs-platform-wallet` so the resolver picks identical hashes. +platform-wallet = { path = "../rs-platform-wallet", features = [ + "serde", +], optional = true } +serde = { version = "1", features = ["derive"], optional = true } key-wallet = { workspace = true, optional = true } dashcore = { workspace = true, optional = true } dpp = { path = "../rs-dpp", optional = true } @@ -50,7 +55,6 @@ refinery = { version = "0.9", default-features = false, features = [ # (which derives bincode 2 `Encode`/`Decode`) and decode # `dpp::AssetLockProof` from the asset-lock blob column. bincode = { version = "2", optional = true } -fs2 = { version = "0.4", optional = true } tempfile = { version = "3", optional = true } chrono = { version = "0.4", default-features = false, features = [ "clock", @@ -74,11 +78,27 @@ filetime = "0.2" tracing-test = { version = "0.2", features = ["no-env-filter"] } serial_test = "3" platform-wallet-storage = { path = ".", features = ["sqlite", "cli", "__test-helpers"] } +# `round_trip_consumer.rs` constructs a real `PlatformWalletManager` +# (consumer) against a real `SqlitePersister` (this crate's impl) so +# every consumer↔persister contract drift becomes a CI failure (CODE-008 +# / T-024). The manager needs `dash-sdk::SdkBuilder::new_mock().build()` +# (gated behind `mocks`) and `platform-wallet` requires `wallet` on the +# SDK transitively. Tokio is needed directly so `#[tokio::test]` +# resolves the macro by name. +dash-sdk = { path = "../rs-sdk", default-features = false, features = [ + "dashpay-contract", + "dpns-contract", + "wallet", + "mocks", +] } +tokio = { version = "1", features = ["macros", "rt-multi-thread"] } [features] default = ["sqlite", "cli"] # SQLite-backed persister (`platform_wallet_storage::sqlite`). sqlite = [ + "dep:platform-wallet", + "dep:serde", "dep:key-wallet", "dep:dashcore", "dep:dpp", @@ -86,7 +106,6 @@ sqlite = [ "dep:rusqlite", "dep:refinery", "dep:bincode", - "dep:fs2", "dep:tempfile", "dep:chrono", "dep:sha2", diff --git a/packages/rs-platform-wallet-storage/README.md b/packages/rs-platform-wallet-storage/README.md index 1f3c4ebde3..097245743e 100644 --- a/packages/rs-platform-wallet-storage/README.md +++ b/packages/rs-platform-wallet-storage/README.md @@ -28,16 +28,20 @@ structured so a future `SecretStore` (currently sketched in transient SQLite failure (`SQLITE_BUSY` / `SQLITE_LOCKED`) the buffered changeset is merged back into the per-wallet buffer (LWW with anything `store()`-d during the failed transaction) and the -call returns a `PersistenceError::Backend(_)` whose payload contains -the marker `flush failed transiently`. **Retry the call** — do not -discard state. Fatal failures (integrity check, encode error, mutex -poison, …) drop the buffer and surface verbatim. +call returns a `PersistenceError::Backend { kind: Transient, source }` +whose source carries the marker `flush failed transiently`. +**Retry the call** — do not discard state. Fatal failures (integrity +check, encode error, mutex poison, …) return `kind: Fatal` (or +`kind: Constraint` for SQL constraint violations) and drop the buffer. The full classification lives on -[`WalletStorageError::is_transient`](src/sqlite/error.rs); the -boundary mapping into `PersistenceError::Backend(String)` flattens -the `Display` chain so operators can grep for variant names + hex -wallet ids in production logs. +[`WalletStorageError::is_transient`](src/sqlite/error.rs) and the +companion [`WalletStorageError::persistence_kind`](src/sqlite/error.rs) +that selects the trait-side kind. The `source` field is a +`Box` over the original `WalletStorageError` +— operators can walk `Error::source()` for the full typed chain; +the outer `Display` carries the variant marker + hex wallet id so +production-log greps still work. ## load() reconstruction @@ -94,14 +98,16 @@ platform-wallet-storage --db backup --out platform-wallet-storage --db restore --from --yes platform-wallet-storage --db prune --in [--keep-last N] [--max-age 30d] platform-wallet-storage --db inspect [--wallet-id ] [--format text|tsv|json] -platform-wallet-storage --db delete-wallet --wallet-id --yes [--no-auto-backup] ``` -Destructive subcommands (`restore`, `delete-wallet`) REQUIRE `--yes` -— invoking them without it exits 2 with a usage error. `--no-auto-backup` -opts out of the pre-migration / pre-delete auto-backup respectively; -the library API has no equivalent opt-out (it routes to -[`SqlitePersister::delete_wallet_skip_backup`] internally). +Destructive subcommands (`restore`) REQUIRE `--yes` — invoking them +without it exits 2 with a usage error. `--no-auto-backup` opts out of +the pre-restore (or pre-migration) auto-backup; it is the only +supported way to disable auto-backup. + +Wallet removal is a library-only API +([`SqlitePersister::delete_wallet`] / `delete_wallet_skip_backup`); +no CLI subcommand exposes it. Logging: `-v` / `-vv` / `-vvv` enable `info` / `debug` / `trace` respectively on stderr; `-q` suppresses non-error output. @@ -111,19 +117,16 @@ validation failure (e.g. corrupt backup source). ## Operational notes -**Restore advisory-lock warning.** `restore` takes an exclusive `flock(2)` -on the destination DB and holds it across the entire restore body, so a -concurrent writer can't race the atomic swap. On filesystems where -advisory locking is unsupported (some NFS / FUSE / network mounts), the -crate emits a `tracing::warn!` on the -`platform_wallet_storage` target — - -> `advisory lock unsupported on this filesystem; concurrent-writer race possible` - -— and proceeds anyway (there's no alternative on such filesystems). -If you see this warning, ensure no other process opens the destination -DB during the restore window, or move the DB to a filesystem with flock -support before restoring. +**Restore exclusion.** `restore` opens a short-lived writer connection +on the destination DB and holds a SQLite-native `BEGIN EXCLUSIVE` +transaction across the entire restore body. This interlocks with every +other SQLite peer — sibling `SqlitePersister` handles, bare +`rusqlite::Connection` instances, the CLI — so concurrent writes back +off via SQLite's `busy_timeout` instead of racing the atomic swap. If a +peer holds the destination busy for longer than the timeout, `restore` +returns `WalletStorageError::RestoreDestinationLocked`. The lock conn is +released BEFORE the rename so SQLite's file handle on the old inode goes +away before the new DB takes its place. **Manual-mode drop diagnostic.** `SqlitePersister` configured with [`FlushMode::Manual`] emits a `tracing::error!` on drop if the buffer @@ -153,9 +156,14 @@ See [`migrations/V001__initial.rs`](./migrations/V001__initial.rs) for the canonical schema. It is hand-written `CREATE TABLE … PRIMARY KEY … FOREIGN KEY …` SQL with native `ON DELETE CASCADE` constraints; INSERT, DELETE-cascade, and UPDATE re-parenting are all enforced by SQLite -itself. Foreign-key enforcement is enabled and read-back-asserted on -every connection open via the `open_conn` choke-point — if the linked -SQLite cannot honor `PRAGMA foreign_keys`, open fails hard. The single -remaining trigger clears `core_utxos.spent_in_txid` to NULL on -transaction delete (a native composite `SET NULL` would null the -NOT-NULL `wallet_id` column too). +itself. Wallet-scoped tables FK directly to `wallet_metadata`; +identity-owned tables (`identity_keys`, `token_balances`, +`dashpay_profiles`, `dashpay_payments_overlay`) are keyed by +`identity_id` only and cascade through `identities` (whose `wallet_id` +is nullable to support identity-only flows). Foreign-key enforcement is +enabled and read-back-asserted on every connection open via the +`open_conn` choke-point — if the linked SQLite cannot honor +`PRAGMA foreign_keys`, open fails hard. The single remaining trigger +clears `core_utxos.spent_in_txid` to NULL on transaction delete (a +native composite `SET NULL` would null the NOT-NULL `wallet_id` column +too). diff --git a/packages/rs-platform-wallet-storage/SECRETS.md b/packages/rs-platform-wallet-storage/SECRETS.md index 8871f0f396..b6287035c2 100644 --- a/packages/rs-platform-wallet-storage/SECRETS.md +++ b/packages/rs-platform-wallet-storage/SECRETS.md @@ -55,7 +55,7 @@ secret-free. exempt by design. - NFR-4 / TC-082 (`tests/sqlite_persist_roundtrip.rs::tc082_no_box_dyn_error_in_src`): all public method signatures use concrete error types - (`SqlitePersisterError`, `PersistenceError`) — never + (`WalletStorageError`, `PersistenceError`) — never `Box` — so a future leak is caught by `grep`. ## Backup retention and secrets diff --git a/packages/rs-platform-wallet-storage/migrations/V001__initial.rs b/packages/rs-platform-wallet-storage/migrations/V001__initial.rs index 863a7b9cd2..f24015d412 100644 --- a/packages/rs-platform-wallet-storage/migrations/V001__initial.rs +++ b/packages/rs-platform-wallet-storage/migrations/V001__initial.rs @@ -5,11 +5,19 @@ //! FK clause must live inside `CREATE TABLE`; that requirement is why //! the schema is emitted as explicit DDL rather than a query-builder. //! -//! Every per-wallet table carries `wallet_id BLOB` in (or as all of) -//! its primary key plus a native `FOREIGN KEY (wallet_id) REFERENCES -//! wallet_metadata(wallet_id) ON DELETE CASCADE`. `identity_keys` and -//! `dashpay_profiles` additionally key into `identities(wallet_id, -//! identity_id)`. The one relationship that stays a trigger is +//! Per-wallet tables carry `wallet_id BLOB` in (or as all of) their +//! primary key plus a native `FOREIGN KEY (wallet_id) REFERENCES +//! wallet_metadata(wallet_id) ON DELETE CASCADE`. Identity-owned +//! tables (`identity_keys`, `dashpay_profiles`, +//! `dashpay_payments_overlay`, `token_balances`) are keyed by +//! `identity_id` only; their FK targets `identities(identity_id)` so +//! cascade flows `wallet_metadata → identities → child` through the +//! nullable `identities.wallet_id` link. `identities.wallet_id` is +//! NULL-allowed so identity-only flows (no parent wallet, e.g. the +//! identity-sync manager populating rows before any wallet is +//! registered) work without a placeholder. +//! +//! The one relationship that stays a trigger is //! `core_utxos.spent_in_txid` clearing to NULL on transaction delete — //! a native composite `ON DELETE SET NULL` would null the NOT-NULL //! `wallet_id` too (SQLite nulls all FK columns), so the single-column @@ -119,29 +127,27 @@ CREATE TABLE core_sync_state ( ); CREATE TABLE identities ( - wallet_id BLOB NOT NULL, + identity_id BLOB NOT NULL PRIMARY KEY, + wallet_id BLOB, wallet_index INTEGER, - identity_id BLOB NOT NULL, entry_blob BLOB NOT NULL, tombstoned INTEGER NOT NULL, - PRIMARY KEY (wallet_id, identity_id), FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE ); +CREATE INDEX idx_identities_wallet ON identities(wallet_id); + CREATE TABLE identity_keys ( - wallet_id BLOB NOT NULL, identity_id BLOB NOT NULL, key_id INTEGER NOT NULL, public_key_blob BLOB NOT NULL, public_key_hash BLOB NOT NULL, derivation_blob BLOB, - PRIMARY KEY (wallet_id, identity_id, key_id), - FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE, - FOREIGN KEY (wallet_id, identity_id) - REFERENCES identities(wallet_id, identity_id) ON DELETE CASCADE + PRIMARY KEY (identity_id, key_id), + FOREIGN KEY (identity_id) REFERENCES identities(identity_id) ON DELETE CASCADE ); -CREATE INDEX idx_identity_keys_wallet_identity ON identity_keys(wallet_id, identity_id); +CREATE INDEX idx_identity_keys_identity ON identity_keys(identity_id); CREATE TABLE contacts_sent ( wallet_id BLOB NOT NULL, @@ -202,32 +208,26 @@ CREATE TABLE asset_locks ( ); CREATE TABLE token_balances ( - wallet_id BLOB NOT NULL, identity_id BLOB NOT NULL, token_id BLOB NOT NULL, balance INTEGER NOT NULL, updated_at INTEGER NOT NULL, - PRIMARY KEY (wallet_id, identity_id, token_id), - FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE + PRIMARY KEY (identity_id, token_id), + FOREIGN KEY (identity_id) REFERENCES identities(identity_id) ON DELETE CASCADE ); CREATE TABLE dashpay_profiles ( - wallet_id BLOB NOT NULL, - identity_id BLOB NOT NULL, + identity_id BLOB NOT NULL PRIMARY KEY, profile_blob BLOB NOT NULL, - PRIMARY KEY (wallet_id, identity_id), - FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE, - FOREIGN KEY (wallet_id, identity_id) - REFERENCES identities(wallet_id, identity_id) ON DELETE CASCADE + FOREIGN KEY (identity_id) REFERENCES identities(identity_id) ON DELETE CASCADE ); CREATE TABLE dashpay_payments_overlay ( - wallet_id BLOB NOT NULL, identity_id BLOB NOT NULL, payment_id TEXT NOT NULL, overlay_blob BLOB NOT NULL, - PRIMARY KEY (wallet_id, identity_id, payment_id), - FOREIGN KEY (wallet_id) REFERENCES wallet_metadata(wallet_id) ON DELETE CASCADE + PRIMARY KEY (identity_id, payment_id), + FOREIGN KEY (identity_id) REFERENCES identities(identity_id) ON DELETE CASCADE ); "; diff --git a/packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs b/packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs index 66b7a8a9a3..799b7a4c4f 100644 --- a/packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs +++ b/packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs @@ -10,8 +10,8 @@ use std::time::Duration; use clap::{Args, Parser, Subcommand}; use platform_wallet_storage::{ - AutoBackupOperation, RetentionPolicy, SqlitePersister, SqlitePersisterConfig, - WalletStorageError, + default_auto_backup_dir, AutoBackupOperation, RetentionPolicy, SqlitePersister, + SqlitePersisterConfig, WalletStorageError, }; #[derive(Debug, Parser)] @@ -24,9 +24,11 @@ struct Cli { /// Path to the SQLite database file. #[arg(long, value_name = "PATH", global = true)] db: Option, - /// Auto-backup directory. Pass empty string to disable. + /// Auto-backup directory. To disable auto-backup, pass the + /// subcommand flag `--no-auto-backup` (supported by `migrate` and + /// `restore`). #[arg(long, value_name = "PATH", global = true)] - auto_backup_dir: Option, + auto_backup_dir: Option, /// Increase log verbosity (stderr). Repeat for more: `-v` enables /// `info`, `-vv` enables `debug`, `-vvv` enables `trace`. #[arg(long, short, global = true, action = clap::ArgAction::Count)] @@ -177,11 +179,7 @@ fn run(cli: Cli) -> Result { let db = cli .db .ok_or_else(|| CliError::runtime("--db is required"))?; - let auto_backup_dir = match cli.auto_backup_dir { - None => None, - Some(s) if s.is_empty() => Some(None), - Some(s) => Some(Some(PathBuf::from(s))), - }; + let auto_backup_dir: Option = cli.auto_backup_dir; // For `prune`, we don't open a persister — pure filesystem op. if let Cmd::Prune(args) = &cli.cmd { @@ -190,7 +188,7 @@ fn run(cli: Cli) -> Result { // `restore` is an associated function; no persister needed beforehand. if let Cmd::Restore(args) = &cli.cmd { - return run_restore(&db, args, auto_backup_dir.as_ref()); + return run_restore(&db, args, auto_backup_dir.as_deref()); } // For `migrate --no-auto-backup`, we must keep `auto_backup_dir = @@ -199,17 +197,10 @@ fn run(cli: Cli) -> Result { // default) in place — the library's safe-by-default semantics // still apply. let mut config = SqlitePersisterConfig::new(&db); - if let Some(dir_opt) = auto_backup_dir.clone() { - config = config.with_auto_backup_dir(dir_opt); + if let Some(dir) = auto_backup_dir.clone() { + config = config.with_auto_backup_dir(Some(dir)); } if let Cmd::Migrate(m) = &cli.cmd { - if matches!(&auto_backup_dir, Some(None)) && !m.no_auto_backup { - return Err(CliError { - message: "auto-backup directory not configured; pass --no-auto-backup to proceed" - .to_string(), - code: ExitCode::from(1), - }); - } if m.no_auto_backup { config = config.with_auto_backup_dir(None); eprintln!("warning: auto-backup skipped (--no-auto-backup)"); @@ -307,7 +298,7 @@ fn run_backup(persister: &SqlitePersister, args: BackupArgs) -> Result>, + auto_backup_dir: Option<&Path>, ) -> Result { if !args.yes { return Err(CliError { @@ -322,11 +313,11 @@ fn run_restore( // CLI default mirrors the persister config default // (`/backups/auto/`). The CLI doesn't open a // persister here, so we compute the default inline. - let resolved_dir: Option = match auto_backup_dir { - None => Some(default_auto_backup_dir_for_cli(db)), - Some(opt) => opt.clone(), + let resolved_dir: PathBuf = match auto_backup_dir { + None => default_auto_backup_dir(db), + Some(p) => p.to_path_buf(), }; - SqlitePersister::restore_from(db, &args.from, resolved_dir.as_deref()) + SqlitePersister::restore_from(db, &args.from, Some(&resolved_dir)) }; match result { Ok(()) => Ok(ExitCode::SUCCESS), @@ -343,18 +334,6 @@ fn run_restore( } } -/// Mirror of `platform_wallet_storage::sqlite::config::default_auto_backup_dir` -/// for the CLI's `restore` path (which doesn't go through a -/// persister). -fn default_auto_backup_dir_for_cli(db_path: &Path) -> PathBuf { - let parent = db_path - .parent() - .filter(|p| !p.as_os_str().is_empty()) - .map(PathBuf::from) - .unwrap_or_else(|| PathBuf::from(".")); - parent.join("backups").join("auto") -} - fn run_prune(args: &PruneArgs) -> Result { if args.keep_last.is_none() && args.max_age.is_none() { return Err(CliError { diff --git a/packages/rs-platform-wallet-storage/src/lib.rs b/packages/rs-platform-wallet-storage/src/lib.rs index c346742b97..e6e9365336 100644 --- a/packages/rs-platform-wallet-storage/src/lib.rs +++ b/packages/rs-platform-wallet-storage/src/lib.rs @@ -30,11 +30,9 @@ pub mod sqlite; // names. Adding to or trimming from this list does NOT count as a // breaking change of the submodule API. #[cfg(feature = "sqlite")] -#[allow(deprecated)] pub use sqlite::{ - AutoBackupOperation, CommitReport, DeleteWalletReport, FlushMode, JournalMode, PruneReport, - RetentionPolicy, SqlitePersister, SqlitePersisterConfig, SqlitePersisterError, Synchronous, - WalletStorageError, + default_auto_backup_dir, AutoBackupOperation, FlushMode, JournalMode, PruneReport, + RetentionPolicy, SqlitePersister, SqlitePersisterConfig, Synchronous, WalletStorageError, }; // Compile-time assertions — `Send + Sync`, `PlatformWalletPersistence` diff --git a/packages/rs-platform-wallet-storage/src/sqlite/backup.rs b/packages/rs-platform-wallet-storage/src/sqlite/backup.rs index a7d838e934..f0965a7599 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/backup.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/backup.rs @@ -4,7 +4,7 @@ use std::path::{Path, PathBuf}; use std::time::{Duration, SystemTime}; use rusqlite::backup::Backup; -use rusqlite::{Connection, OptionalExtension}; +use rusqlite::Connection; use platform_wallet::wallet::platform_wallet::WalletId; @@ -12,6 +12,27 @@ use crate::sqlite::error::WalletStorageError; use crate::sqlite::persister::{PruneReport, RetentionPolicy}; use crate::sqlite::util::permissions::apply_secure_permissions; +/// CODE-014: fsync the parent directory of `path` on Unix so the +/// rename entry that materialised `path` is durable across power loss. +/// `persist` only fsyncs the file inode; on most Unix filesystems the +/// dentry update is journalled separately and can be lost on crash +/// without this step. No-op on non-Unix platforms. +#[cfg(unix)] +fn fsync_parent_dir(path: &Path) -> Result<(), WalletStorageError> { + if let Some(parent) = path.parent() { + if !parent.as_os_str().is_empty() { + let dir = std::fs::File::open(parent)?; + dir.sync_all()?; + } + } + Ok(()) +} + +#[cfg(not(unix))] +fn fsync_parent_dir(_path: &Path) -> Result<(), WalletStorageError> { + Ok(()) +} + /// Normalize an `open_conn` failure on a candidate source/staged file /// to the typed [`WalletStorageError::SourceOpenFailed`]. A raw rusqlite /// open error keeps its `#[source]`; any other variant (e.g. a future @@ -55,26 +76,25 @@ pub fn auto_backup_filename(kind: BackupKind) -> String { /// # Atomicity /// /// The page-stepping copy runs against a `NamedTempFile` staged in -/// `dest`'s parent directory. The temp is `persist`-ed over `dest` -/// only on success — any failure (open, chmod, backup-stream) drops -/// the temp without ever materialising a partial `.db` file at the -/// caller's path. +/// `dest`'s parent directory. The temp is `persist_noclobber`-ed over +/// `dest` only on success — any failure (open, chmod, backup-stream) +/// drops the temp without ever materialising a partial `.db` file at +/// the caller's path. A pre-existing `dest` is rejected atomically by +/// `persist_noclobber` (no TOCTOU window). On Unix, the parent +/// directory is `fsync`-ed after the rename so the dentry update +/// survives power loss; on non-Unix this fsync step is a no-op. pub fn run_to(src: &Connection, dest: &Path) -> Result<(), WalletStorageError> { if let Some(parent) = dest.parent() { if !parent.as_os_str().is_empty() && !parent.exists() { std::fs::create_dir_all(parent)?; } } - // Reject pre-existing destinations BEFORE staging so the temp file - // isn't created (and immediately dropped) on a duplicate path. The - // CLI's `backup_to(file_path)` relies on this typed error; auto- - // backup callers can't trip it because the filename carries a - // unique timestamp suffix. - if dest.exists() { - return Err(WalletStorageError::BackupDestinationExists { - path: dest.to_path_buf(), - }); - } + // CODE-009: pre-existing-destination rejection happens at the + // `persist_noclobber` site below — that's atomic against the rename + // (no TOCTOU window between `dest.exists()` and persist). The + // CLI's `backup_to(file_path)` still gets the typed + // `BackupDestinationExists` error; auto-backup callers can't trip + // it because the filename carries a unique timestamp suffix. // Stage the backup into an unguessable temp file in the same // directory. Same-FS guarantee makes `persist` an atomic rename. @@ -105,8 +125,23 @@ pub fn run_to(src: &Connection, dest: &Path) -> Result<(), WalletStorageError> { // with the rename since `persist` atomically renames the temp file. drop(backup_conn); - tmp.persist(dest) - .map_err(|e| WalletStorageError::Io(e.error))?; + // CODE-009: `persist_noclobber` is the atomic check-and-rename — + // SQLite-free, no TOCTOU window between an `exists()` probe and the + // rename. `AlreadyExists` maps to the typed + // `BackupDestinationExists` for the CLI's overwrite-refusal contract. + tmp.persist_noclobber(dest).map_err(|e| { + if e.error.kind() == std::io::ErrorKind::AlreadyExists { + WalletStorageError::BackupDestinationExists { + path: dest.to_path_buf(), + } + } else { + WalletStorageError::Io(e.error) + } + })?; + // CODE-014: fsync the parent directory so the atomic rename's + // dentry update is durable across power loss. On non-Unix this is + // a no-op. + fsync_parent_dir(dest)?; // SEC-011: re-tighten in case a non-Unix build (or a future // platform-specific tweak) needs to refresh sibling perms after // SQLite materialised them. No-op on Unix where the temp already @@ -122,13 +157,22 @@ pub fn run_to(src: &Connection, dest: &Path) -> Result<(), WalletStorageError> { /// /// # Atomicity /// -/// The restore is staged in two phases bounded by an exclusive -/// advisory file lock on `dest_db_path` (kept across the entire body): +/// The restore is staged in two phases bounded by a SQLite-native +/// `BEGIN EXCLUSIVE` transaction on `dest_db_path` (kept across the +/// entire restore body): /// /// 1. Open the source read-only; run `PRAGMA integrity_check` + /// schema-history + max-version sniffs. Any failure here aborts /// before the live destination is touched. -/// 2. Stream the source into a `NamedTempFile` in `dest_db_path`'s +/// 2. Open a short-lived writer connection on the destination and +/// `BEGIN EXCLUSIVE`. This blocks every other SQLite peer +/// (other `SqlitePersister` handles in this or sibling processes, +/// bare `rusqlite::Connection`s, the CLI) from writing the file +/// until restore completes. Peers waiting for the lock back off +/// via SQLite's own busy_timeout. The lock conn is DROPPED right +/// before `persist` so SQLite releases its file handle on the old +/// inode before the atomic rename takes its place. +/// 3. Stream the source into a `NamedTempFile` in `dest_db_path`'s /// parent directory; re-run integrity + schema gates against the /// STAGED bytes (catches a torn `io::copy`); unlink the existing /// `-wal` / `-shm` siblings; chmod the temp to 0o600; then @@ -136,8 +180,38 @@ pub fn run_to(src: &Connection, dest: &Path) -> Result<(), WalletStorageError> { /// /// Either both the main DB and its WAL/SHM siblings are replaced, or /// — on any pre-persist failure — none of them are touched. The -/// exclusive lock prevents a racing opener from materialising new -/// WAL/SHM siblings against the about-to-be-replaced inode. +/// SQLite-native lock prevents a racing peer from committing rows +/// between the staged validation and the rename, which the prior +/// flock-based approach could not do (flock doesn't see SQLite peers). +/// +/// On Unix, the parent directory is `fsync`-ed after the rename so the +/// dentry update is durable across power loss; on non-Unix this is a +/// no-op. +/// +/// # Lock-release-before-rename trade-off +/// +/// The EXCLUSIVE lock is released BEFORE the atomic rename, on +/// purpose. SQLite keeps a kernel file handle on the destination's +/// (old) inode for as long as the lock conn is alive; holding that +/// handle across the rename would leave it pointing at the unlinked +/// old inode while peers opening the new path would race the rename +/// itself (on some filesystems the rename can outright fail). +/// Releasing the lock first lets SQLite drop its old-inode handle +/// before the rename swaps it. +/// +/// The trade-off: a microsecond window opens between lock release and +/// rename in which a peer can acquire its own SQLite lock on the +/// destination's old inode. Any writes it makes within that window +/// land in the old inode, which the rename immediately unlinks — the +/// peer's writes are effectively dropped on the floor (the peer keeps +/// a handle on an inode that no longer has any directory entry; once +/// it closes, the bytes are reclaimed). That is acceptable for the +/// restore contract: callers serialize their own restore intent at +/// the application layer; the window is too short for a non-malicious +/// peer to land more than a transient miss, and a malicious peer +/// cannot escalate beyond losing its own write. Correct file-handle +/// semantics across the rename matter more than absolute lock +/// coverage. pub fn restore_from(dest_db_path: &Path, src_backup: &Path) -> Result<(), WalletStorageError> { // 1. Confirm the source is openable, then run cheap pre-staging // integrity + schema-history + max-version sniffs against the @@ -151,52 +225,45 @@ pub fn restore_from(dest_db_path: &Path, src_backup: &Path) -> Result<(), Wallet run_integrity_check(&src, |report| WalletStorageError::IntegrityCheckFailed { report, })?; - let src_has_schema = src - .query_row( - "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'refinery_schema_history'", - [], - |_| Ok(()), - ) - .optional()? - .is_some(); - if !src_has_schema { + if !crate::sqlite::migrations::has_schema_history(&src)? { return Err(WalletStorageError::SchemaHistoryMissing); } crate::sqlite::migrations::assert_schema_version_supported(&src)?; drop(src); - // 2. ATOM-005 (A-2): take an exclusive advisory lock on the - // destination and HOLD it across the entire restore body. The - // pre-A-2 code probed the lock, dropped the handle, then ran - // steps 3-7 unlocked — a concurrent process opening - // `dest_db_path` between the probe and `tmp.persist` would race - // the rename and end up holding a fd against the unlinked old - // inode while the new DB takes its place. Keeping the guard - // `_lock` alive in scope closes that window. - // - // On filesystems without flock support (the previous silent-skip - // arm) we emit a structured warn so operators know the safety - // net is bypassed — still proceed because there's no alternative - // on such filesystems, but never silently. - let _lock: Option = if dest_db_path.exists() { - use fs2::FileExt; - let f = std::fs::OpenOptions::new() - .read(true) - .write(true) - .open(dest_db_path)?; - match f.try_lock_exclusive() { - Ok(()) => Some(f), - Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { + // 2. SQLite-native exclusion. `BEGIN EXCLUSIVE` against a short- + // lived writer connection on the destination blocks every other + // SQLite peer (rusqlite Connection, sibling `SqlitePersister`) + // until the tx is committed/rolled-back or the conn drops. The + // prior flock approach was a false promise: advisory locks + // don't interlock with SQLite's own locking, so a peer mid-write + // could race the swap. The lock conn is dropped (`take()` + end + // of scope) BEFORE `tmp.persist` so SQLite releases its file + // handle on the old inode before the atomic rename — otherwise + // we'd leave a dangling handle on the unlinked inode. + let mut dest_lock_conn: Option = if dest_db_path.exists() { + let conn = + crate::sqlite::conn::open_conn(dest_db_path, crate::sqlite::conn::Access::ReadWrite)?; + // Reuse a sensible busy_timeout so peers don't immediately + // surface BUSY without a backoff window. The destination DB + // may not have a persister attached yet (the persister is the + // CALLER), so this conn applies its own. + conn.busy_timeout(std::time::Duration::from_secs(5))?; + // Take EXCLUSIVE up-front by promoting an immediate tx. If a + // peer holds the DB, SQLite waits for busy_timeout then + // returns BUSY — we surface that as `RestoreDestinationLocked` + // so callers keep their existing branch. + match conn.execute_batch("BEGIN EXCLUSIVE") { + Ok(()) => Some(conn), + Err(rusqlite::Error::SqliteFailure(err, _)) + if matches!( + err.code, + rusqlite::ErrorCode::DatabaseBusy | rusqlite::ErrorCode::DatabaseLocked + ) => + { return Err(WalletStorageError::RestoreDestinationLocked); } - Err(_) => { - tracing::warn!( - target: "platform_wallet_storage", - dest = %dest_db_path.display(), - "advisory lock unsupported on this filesystem; concurrent-writer race possible" - ); - None - } + Err(other) => return Err(WalletStorageError::Sqlite(other)), } } else { None @@ -225,15 +292,7 @@ pub fn restore_from(dest_db_path: &Path, src_backup: &Path) -> Result<(), Wallet // Schema-history presence + max-version gate, bound to the // staged bytes (not the first source handle) so a swap during // the restore window can't slip a forward-version DB through. - let has_schema = staged - .query_row( - "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'refinery_schema_history'", - [], - |_| Ok(()), - ) - .optional()? - .is_some(); - if !has_schema { + if !crate::sqlite::migrations::has_schema_history(&staged)? { return Err(WalletStorageError::SchemaHistoryMissing); } crate::sqlite::migrations::assert_schema_version_supported(&staged)?; @@ -244,16 +303,21 @@ pub fn restore_from(dest_db_path: &Path, src_backup: &Path) -> Result<(), Wallet // have left behind. Doing this BEFORE persist ensures that // either both the main DB and its siblings get replaced/cleared, // or — if any earlier check failed — none of them are touched. - for ext in ["-wal", "-shm"] { - let sibling = dest_db_path.with_file_name(format!( - "{}{ext}", - dest_db_path - .file_name() - .map(|s| s.to_string_lossy().to_string()) - .unwrap_or_default() - )); - if sibling.exists() { - std::fs::remove_file(&sibling)?; + // + // CODE-011: build sibling paths via `OsString::push` so non-UTF-8 + // bytes round-trip intact; `remove_file` runs unconditionally and + // `ErrorKind::NotFound` is a silent no-op (closes the `exists()` + // TOCTOU gate). + if let Some(file_name) = dest_db_path.file_name() { + for ext in ["-wal", "-shm"] { + let mut sibling_name = file_name.to_os_string(); + sibling_name.push(ext); + let sibling = dest_db_path.with_file_name(sibling_name); + match std::fs::remove_file(&sibling) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(WalletStorageError::Io(e)), + } } } @@ -269,21 +333,44 @@ pub fn restore_from(dest_db_path: &Path, src_backup: &Path) -> Result<(), Wallet .set_permissions(std::fs::Permissions::from_mode(0o600))?; } - // 7. Persist atomically over the destination. + // 7. Release the SQLite-native EXCLUSIVE lock BEFORE the rename. + // Dropping `dest_lock_conn` causes SQLite to close its file + // handle on the old inode; if we kept it alive across `persist` + // the handle would point at the unlinked old inode while the + // new DB took its place — peers reopening would race the rename + // and (on some filesystems) the rename itself could fail. + if let Some(conn) = dest_lock_conn.take() { + // Best-effort rollback of the empty EXCLUSIVE tx; an error here + // means SQLite is already in trouble and `drop(conn)` covers + // the rest. Silent because the conn is about to drop anyway. + let _ = conn.execute_batch("ROLLBACK"); + drop(conn); + } + + // 8. Persist atomically over the destination. tmp.persist(dest_db_path) .map_err(|e| WalletStorageError::Io(e.error))?; - // 8. Re-tighten siblings (SQLite may materialise -wal/-shm on next - // open; this is idempotent at restore-completion time). + // 9. CODE-014: fsync the destination's parent directory so the + // atomic rename's dentry update is durable across power loss + // (no-op on non-Unix). + fsync_parent_dir(dest_db_path)?; + + // 10. Re-tighten siblings (SQLite may materialise -wal/-shm on next + // open; this is idempotent at restore-completion time). apply_secure_permissions(dest_db_path)?; - // Lock guard is released by `_lock` going out of scope here. Ok(()) } -/// Run `PRAGMA integrity_check` and return `Ok(())` if SQLite returns -/// "ok". Any other returned text becomes a typed `IntegrityCheckFailed` -/// via the caller-supplied builder; an underlying rusqlite error -/// surfaces as `IntegrityCheckRunFailed`. +/// Run `PRAGMA integrity_check` and return `Ok(())` when SQLite reports +/// the single row `"ok"`. Any other result becomes a typed +/// `IntegrityCheckFailed` via the caller-supplied builder; an +/// underlying rusqlite error surfaces as `IntegrityCheckRunFailed`. +/// +/// CODE-016: SQLite returns one row per detected problem (capped at +/// `PRAGMA integrity_check(N)`; default 100). All rows are collected +/// and joined with `\n` so the typed report carries every diagnostic +/// instead of just the first line. /// /// `pub(crate)` so the persister's open-time A-8 probe shares the /// same helper rather than reimplementing the report-rendering rule. @@ -294,12 +381,45 @@ pub(crate) fn run_integrity_check( where F: FnOnce(String) -> WalletStorageError, { - let report: String = conn - .query_row("PRAGMA integrity_check", [], |row| row.get(0)) + let mut stmt = conn + .prepare("PRAGMA integrity_check") + .map_err(|source| WalletStorageError::IntegrityCheckRunFailed { source })?; + let mut rows: Vec = Vec::new(); + let mut trailing_err: Option = None; + let iter = stmt + .query_map([], |row| row.get::<_, String>(0)) .map_err(|source| WalletStorageError::IntegrityCheckRunFailed { source })?; - if report == "ok" { + for item in iter { + match item { + Ok(s) => rows.push(s), + Err(e) => { + // Severe corruption can cause SQLite to surface a + // `DatabaseCorrupt` SqliteFailure partway through the + // integrity_check stream. Treat it as end-of-stream + // when we already have diagnostics (the rows we have + // are still valid); if we have NOTHING, surface the + // typed `IntegrityCheckRunFailed`. + trailing_err = Some(e); + break; + } + } + } + if rows.is_empty() { + if let Some(source) = trailing_err { + return Err(WalletStorageError::IntegrityCheckRunFailed { source }); + } + // Empty result with no error is unexpected but not "ok". + return Err(on_failure(String::new())); + } + if rows.len() == 1 && rows[0] == "ok" && trailing_err.is_none() { Ok(()) } else { + let mut report = rows.join("\n"); + if let Some(e) = trailing_err { + // Preserve the cut-off marker so operators see the stream + // was truncated, not just under-reported. + report.push_str(&format!("\n[integrity_check stream aborted: {e}]")); + } Err(on_failure(report)) } } @@ -352,7 +472,15 @@ pub fn prune(dir: &Path, policy: RetentionPolicy) -> Result removed.push(path), - Err(e) => failed_removals.push((path, e)), + Err(e) => { + // CODE-019: a failed `remove_file` leaves the file + // on disk, so it MUST be counted in `kept`. The + // invariant `kept + removed.len() == total` then + // holds and `failed_removals` is a subset of + // `kept`. + failed_removals.push((path, e)); + kept += 1; + } } } } diff --git a/packages/rs-platform-wallet-storage/src/sqlite/buffer.rs b/packages/rs-platform-wallet-storage/src/sqlite/buffer.rs index 311a2f1741..616c7a8e3f 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/buffer.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/buffer.rs @@ -83,20 +83,6 @@ impl Buffer { Ok(()) } - /// Deprecated alias for [`Self::take_for_flush`]. New call sites - /// MUST use the renamed pair so the take/restore lifecycle is - /// explicit. - #[deprecated( - since = "3.1.0-dev.1", - note = "use take_for_flush + restore for retry-safe semantics; remove in 3.2.0" - )] - pub fn drain( - &self, - wallet_id: &WalletId, - ) -> Result, WalletStorageError> { - self.take_for_flush(wallet_id) - } - /// Every wallet currently holding buffered data, sorted by id for /// deterministic flush ordering. pub fn dirty_wallets(&self) -> Result, WalletStorageError> { diff --git a/packages/rs-platform-wallet-storage/src/sqlite/config.rs b/packages/rs-platform-wallet-storage/src/sqlite/config.rs index ce69361120..1beb7c2c02 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/config.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/config.rs @@ -108,7 +108,12 @@ impl SqlitePersisterConfig { } /// `/backups/auto/` (or `./backups/auto/` if the DB path has no parent). -pub(crate) fn default_auto_backup_dir(db_path: &Path) -> PathBuf { +/// +/// Public so the CLI binary (a separate compilation unit) can share the +/// same resolution as the library's `SqlitePersisterConfig::new`. The +/// preferred narrower visibility would be `pub(super)`, but `pub use` +/// re-exports up to the crate root cannot expose a `pub(super)` item. +pub fn default_auto_backup_dir(db_path: &Path) -> PathBuf { let parent = db_path .parent() .filter(|p| !p.as_os_str().is_empty()) diff --git a/packages/rs-platform-wallet-storage/src/sqlite/error.rs b/packages/rs-platform-wallet-storage/src/sqlite/error.rs index 887be4cf54..183c6c4d8c 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/error.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/error.rs @@ -8,12 +8,15 @@ //! //! At the `PlatformWalletPersistence` trait boundary, this type //! converts into `PersistenceError`: `LockPoisoned` keeps its -//! dedicated variant, everything else flows through -//! `PersistenceError::Backend` with the full `Display` chain. +//! dedicated variant; everything else flows through +//! `PersistenceError::Backend { kind, source }` — `kind` is classified +//! by [`WalletStorageError::persistence_kind`] (Transient / Constraint / +//! Fatal) and `source` carries the boxed typed error so consumers can +//! walk `Error::source()` to the underlying `rusqlite` payload. use std::path::PathBuf; -use platform_wallet::changeset::PersistenceError; +use platform_wallet::changeset::{PersistenceError, PersistenceErrorKind}; use crate::sqlite::util::safe_cast::SafeCastTarget; @@ -30,9 +33,6 @@ pub enum AutoBackupOperation { } /// Errors produced by the wallet-storage SQLite backend. -/// -/// `SqlitePersisterError` is preserved as a deprecated alias for one -/// cycle; new code should use `WalletStorageError`. #[derive(Debug, thiserror::Error)] pub enum WalletStorageError { /// File-system I/O error reaching the database or backup files. @@ -52,6 +52,8 @@ pub enum WalletStorageError { /// `PRAGMA integrity_check` ran successfully but reported a /// non-`ok` result. `report` carries SQLite's own diagnostic /// text — not a user-facing message, not a stringified source. + /// May be multi-line (`\n`-joined): SQLite returns one row per + /// detected problem and the helper preserves every line. #[error("integrity check failed: {report}")] IntegrityCheckFailed { report: String }, @@ -117,8 +119,10 @@ pub enum WalletStorageError { #[error("persister lock poisoned")] LockPoisoned, - /// `restore_from` tried to acquire an exclusive file-lock on the - /// destination and couldn't — another process is holding it open. + /// `restore_from` tried to take a SQLite-native `BEGIN EXCLUSIVE` + /// on the destination and a peer (another `SqlitePersister`, a + /// bare `rusqlite::Connection`, the CLI) is holding it busy + /// beyond `busy_timeout`. #[error("restore destination is locked or in use")] RestoreDestinationLocked, @@ -233,6 +237,26 @@ pub enum WalletStorageError { target: SafeCastTarget, }, + /// A `delete_wallet` cascade detected that a peer mutated the + /// wallet's footprint between the pre-delete auto-backup snapshot + /// and the cascade's `BEGIN EXCLUSIVE` acquisition. The auto-backup + /// is taken OUTSIDE the EXCLUSIVE tx because rusqlite's Backup API + /// can't run inside a write transaction on the source conn; that + /// leaves a small window in which a cross-process peer can write + /// to the wallet — those writes would survive in the live DB but + /// would NOT be in the pre-delete backup (operator rollback path + /// would silently lose them). + /// + /// The cascade aborts on detection so the operator can retry once + /// the peer is quiesced. The backup file (if one was written) is + /// left in place — it captures the pre-mutation state and is still + /// useful for forensics. + #[error( + "delete_wallet aborted: peer mutated wallet {} between auto-backup snapshot and EXCLUSIVE acquire", + hex::encode(wallet_id) + )] + ConcurrentMutationDuringDelete { wallet_id: [u8; 32] }, + /// Flush failed transiently (e.g. `SQLITE_BUSY` / `SQLITE_LOCKED`) /// for `wallet_id`. The buffered changeset has been restored — the /// next `flush(wallet_id)` will retry the same data merged with @@ -258,37 +282,15 @@ pub enum WalletStorageError { }, } -/// Deprecated alias preserved for one cycle. Switch downstream -/// references to [`WalletStorageError`]. -#[deprecated(since = "3.1.0-dev.1", note = "renamed to WalletStorageError")] -pub type SqlitePersisterError = WalletStorageError; - impl From for PersistenceError { fn from(err: WalletStorageError) -> Self { match err { WalletStorageError::LockPoisoned => PersistenceError::LockPoisoned, - other => PersistenceError::Backend(format!("{}", DisplayChain(&other))), - } - } -} - -/// Renders an error and its `#[source]` chain for the -/// `PersistenceError::Backend` (`String`) boundary. The trait can't -/// carry typed sources, so the chain is concatenated for diagnostic -/// purposes — every typed variant is still preserved on the -/// `WalletStorageError` value the trait `From` impl consumes. -struct DisplayChain<'a>(&'a WalletStorageError); - -impl std::fmt::Display for DisplayChain<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - use std::error::Error; - write!(f, "{}", self.0)?; - let mut cur: Option<&dyn Error> = self.0.source(); - while let Some(err) = cur { - write!(f, ": {err}")?; - cur = err.source(); + other => { + let kind = other.persistence_kind(); + PersistenceError::backend_with_kind(kind, other) + } } - Ok(()) } } @@ -367,10 +369,44 @@ impl WalletStorageError { | Self::IdentityKeyEntryMismatch | Self::AssetLockEntryMismatch { .. } | Self::BlobTooLarge { .. } + | Self::ConcurrentMutationDuringDelete { .. } | Self::IntegerOverflow { .. } => false, } } + /// Trait-boundary classification for the + /// [`PersistenceError::Backend`] kind field (CODE-004). Three + /// classes: + /// + /// - [`PersistenceErrorKind::Transient`] — every variant where + /// [`Self::is_transient`] is `true`. Caller MAY retry. + /// - [`PersistenceErrorKind::Constraint`] — SQL constraint / + /// FK / NOT NULL / UNIQUE / PK / CHECK violations. Schema / + /// integrity failure; caller bug, not infra. + /// - [`PersistenceErrorKind::Fatal`] — everything else. + /// + /// [`Self::LockPoisoned`] is handled by the `From` impl directly + /// (it maps to [`PersistenceError::LockPoisoned`] rather than + /// flowing through `Backend`). + pub fn persistence_kind(&self) -> PersistenceErrorKind { + use rusqlite::ErrorCode; + if self.is_transient() { + return PersistenceErrorKind::Transient; + } + match self { + Self::Sqlite(rusqlite::Error::SqliteFailure(e, _)) + if matches!(e.code, ErrorCode::ConstraintViolation) => + { + PersistenceErrorKind::Constraint + } + // Refinery surfaces FK / constraint problems through + // rusqlite; if that path leaks through here the typed + // variant lives in `Self::Migration`, which we leave as + // `Fatal` since a migration failure isn't a caller bug. + _ => PersistenceErrorKind::Fatal, + } + } + /// Short, lowercase, snake-case tag for tracing fields. One tag /// per variant family — readers grep for these in production /// logs. @@ -414,6 +450,7 @@ impl WalletStorageError { Self::AssetLockEntryMismatch { .. } => "asset_lock_entry_mismatch", Self::BlobTooLarge { .. } => "blob_too_large", Self::IntegerOverflow { .. } => "integer_overflow", + Self::ConcurrentMutationDuringDelete { .. } => "concurrent_mutation_during_delete", } } } diff --git a/packages/rs-platform-wallet-storage/src/sqlite/migrations.rs b/packages/rs-platform-wallet-storage/src/sqlite/migrations.rs index c183c19165..6dd6078e89 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/migrations.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/migrations.rs @@ -18,6 +18,25 @@ pub fn run(conn: &mut rusqlite::Connection) -> Result Result { + run(conn).map_err(WalletStorageError::Migration) +} + +/// Return a fresh refinery [`Runner`](refinery::Runner) seeded with the +/// embedded migration list. Used by tests that need to apply a subset +/// of migrations via [`refinery::Runner::set_target`]. +pub fn runner() -> refinery::Runner { + migrations::runner() +} + /// Highest migration version this binary knows how to apply. Used by /// both `SqlitePersister::open` (CMT-005) and `backup::restore_from` /// (CMT-001 / CMT-010) to refuse forward-version databases. @@ -29,6 +48,22 @@ pub fn max_supported_version() -> i64 { .unwrap_or(0) } +/// Returns true if the `refinery_schema_history` table exists on this +/// connection. Used by `open`, `restore_from`, and `count_pending` to +/// distinguish "fresh DB" (no migrations applied yet) from +/// "pre-existing DB" (carries refinery history). +pub(crate) fn has_schema_history(conn: &rusqlite::Connection) -> Result { + let exists = conn + .query_row( + "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'refinery_schema_history'", + [], + |_| Ok(()), + ) + .optional()? + .is_some(); + Ok(exists) +} + /// Refuse to operate on a database whose `refinery_schema_history` /// MAX(version) exceeds [`max_supported_version`]. Returns /// [`WalletStorageError::SchemaVersionUnsupported`] in that case. @@ -39,15 +74,7 @@ pub fn max_supported_version() -> i64 { pub fn assert_schema_version_supported( conn: &rusqlite::Connection, ) -> Result<(), WalletStorageError> { - let has_table = conn - .query_row( - "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'refinery_schema_history'", - [], - |_| Ok(()), - ) - .optional()? - .is_some(); - if !has_table { + if !has_schema_history(conn)? { return Ok(()); } let source_version: Option = conn @@ -96,3 +123,30 @@ pub fn embedded_migrations_fingerprint() -> [u8; 32] { } hasher.finalize().into() } + +#[cfg(test)] +mod tests { + use super::*; + use rusqlite::Connection; + + /// TC-CODE-027-1: helper returns false on a brand-new in-memory DB + /// (no `refinery_schema_history`), and true after the table is + /// created. + #[test] + fn has_schema_history_distinguishes_fresh_vs_migrated() { + let conn = Connection::open_in_memory().unwrap(); + assert!( + !has_schema_history(&conn).unwrap(), + "fresh in-memory DB has no schema-history table" + ); + conn.execute( + "CREATE TABLE refinery_schema_history (version INTEGER PRIMARY KEY)", + [], + ) + .unwrap(); + assert!( + has_schema_history(&conn).unwrap(), + "schema-history table is present after creation" + ); + } +} diff --git a/packages/rs-platform-wallet-storage/src/sqlite/mod.rs b/packages/rs-platform-wallet-storage/src/sqlite/mod.rs index 44ffd9a5e5..5a722dd1bf 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/mod.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/mod.rs @@ -16,9 +16,8 @@ pub mod persister; pub mod schema; pub mod util; -pub use config::{FlushMode, JournalMode, SqlitePersisterConfig, Synchronous}; -#[allow(deprecated)] -pub use error::{AutoBackupOperation, SqlitePersisterError, WalletStorageError}; -pub use persister::{ - CommitReport, DeleteWalletReport, PruneReport, RetentionPolicy, SqlitePersister, +pub use config::{ + default_auto_backup_dir, FlushMode, JournalMode, SqlitePersisterConfig, Synchronous, }; +pub use error::{AutoBackupOperation, WalletStorageError}; +pub use persister::{PruneReport, RetentionPolicy, SqlitePersister}; diff --git a/packages/rs-platform-wallet-storage/src/sqlite/persister.rs b/packages/rs-platform-wallet-storage/src/sqlite/persister.rs index c4381c7a8c..164960ab43 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/persister.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/persister.rs @@ -7,7 +7,8 @@ use std::sync::{Arc, Mutex, MutexGuard}; use rusqlite::{Connection, OptionalExtension}; use platform_wallet::changeset::{ - ClientStartState, Merge, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, + ClientStartState, CommitReport, DeleteWalletReport, Merge, PersistenceError, + PlatformWalletChangeSet, PlatformWalletPersistence, }; use platform_wallet::wallet::platform_wallet::WalletId; @@ -15,7 +16,7 @@ use crate::sqlite::backup::{self, BackupKind}; use crate::sqlite::buffer::Buffer; use crate::sqlite::config::{FlushMode, SqlitePersisterConfig, Synchronous}; use crate::sqlite::error::{AutoBackupOperation, WalletStorageError}; -use crate::sqlite::schema::{self, PER_WALLET_TABLES}; +use crate::sqlite::schema::{self, count_rows_for_wallet_sql, PER_WALLET_TABLES}; use crate::sqlite::util::permissions::apply_secure_permissions; use crate::sqlite::util::safe_cast; @@ -27,12 +28,20 @@ use crate::sqlite::util::safe_cast; pub(crate) const LOAD_UNIMPLEMENTED: &[&str] = &["ClientStartState::wallets"]; /// Outcome of a `prune_backups` call. +/// +/// Invariant: `kept == total_eligible - removed.len()`. A file is +/// counted as `kept` if it survived the policy (retained-by-rule) OR +/// if `remove_file` failed (`failed_removals` is a subset of `kept`). +/// Either way, the file is still on disk after this call. #[derive(Debug)] pub struct PruneReport { /// Paths that were unlinked, sorted oldest-first by filename /// timestamp. pub removed: Vec, - /// Number of files that remain in the directory after pruning. + /// Files still on disk after this call. Equals + /// `total_eligible - removed.len()` and includes every + /// `failed_removals` entry — a file that couldn't be unlinked is + /// still on disk and therefore "kept". pub kept: usize, /// Files we tried to remove but couldn't, paired with the /// underlying `io::Error`. Returned as part of `Ok(report)` so a @@ -42,43 +51,6 @@ pub struct PruneReport { pub failed_removals: Vec<(PathBuf, std::io::Error)>, } -/// Outcome of a [`SqlitePersister::commit_writes`] call. Carries every -/// dirty wallet's per-flush outcome so a single failed wallet doesn't -/// hide the success of its siblings (or vice-versa). The caller can -/// retry `still_pending` directly; `failed` carries the classified -/// error per wallet so transient-vs-fatal decisions stay local. -#[derive(Debug)] -pub struct CommitReport { - /// Wallets that flushed successfully (durable on disk). - pub succeeded: Vec, - /// Wallets whose flush returned an error. The - /// `PersistenceError` carries the classification + source per D-9. - pub failed: Vec<(WalletId, PersistenceError)>, - /// Wallets we never attempted because an earlier per-flush call - /// poisoned a shared resource (today: a `LockPoisoned` short-circuit - /// — the connection mutex is gone). Empty on the happy path. - pub still_pending: Vec, -} - -impl CommitReport { - /// `true` when every dirty wallet flushed cleanly. - pub fn is_ok(&self) -> bool { - self.failed.is_empty() && self.still_pending.is_empty() - } -} - -/// Outcome of a `delete_wallet` / `delete_wallet_skip_backup` call. -#[derive(Debug, Clone)] -pub struct DeleteWalletReport { - pub wallet_id: WalletId, - /// Absolute path of the pre-delete auto-backup written before the - /// cascade. `None` ONLY when the caller went through - /// [`SqlitePersister::delete_wallet_skip_backup`] — every - /// `delete_wallet` success returns `Some(path)`. - pub backup_path: Option, - pub rows_removed_per_table: BTreeMap<&'static str, usize>, -} - /// Retention policy for `prune_backups`. /// /// **AND-semantics**: a file is kept iff it satisfies BOTH rules. A @@ -123,6 +95,18 @@ pub struct SqlitePersister { /// (no public setter outside `#[cfg(any(test, feature = "__test-helpers"))]`). #[cfg(any(test, feature = "__test-helpers"))] primed_flush_error: Mutex>, + /// Test-only one-shot callback fired by `delete_wallet_inner` + /// between the pre-delete backup snapshot and the cascade + /// EXCLUSIVE acquisition. Lets cross-process delete-race tests + /// inject a peer mutation in the otherwise-tiny window left open + /// by rusqlite's Backup-API constraint (no source-side write tx). + #[cfg(any(test, feature = "__test-helpers"))] + post_backup_hook: Mutex>>, + /// Test-only one-shot injection consumed by `delete_wallet`'s + /// pre-flush phase. Lets TC-CODE-006-2 assert the buffer-restore + /// and skip-backup semantics without provoking a real SQL error. + #[cfg(any(test, feature = "__test-helpers"))] + primed_pre_flush_error: Mutex>, } impl SqlitePersister { @@ -178,17 +162,9 @@ impl SqlitePersister { // Determine whether `schema_history` exists *before* we run // migrations — that's the signal for "is this DB pre-existing - // or brand-new?" (FR-15 vs FR-16). `.optional()?` distinguishes - // a genuine "no row" answer from a real SQL error, which we - // propagate. - let had_schema_history = conn - .query_row( - "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'refinery_schema_history'", - [], - |_| Ok(()), - ) - .optional()? - .is_some(); + // or brand-new?" (FR-15 vs FR-16). Errors from the underlying + // query are propagated, not silently treated as "no history". + let had_schema_history = crate::sqlite::migrations::has_schema_history(&conn)?; // ATOM-013 (A-8): run integrity_check on a pre-existing DB // BEFORE migrations alter it. Bit-rot or escaped-WAL corruption // detected here surfaces as the typed `IntegrityCheckFailed` @@ -226,8 +202,8 @@ impl SqlitePersister { )?; } - // Apply migrations. - let _report = crate::sqlite::migrations::run(&mut conn)?; + // Apply migrations through the typed-error chokepoint. + let _report = crate::sqlite::migrations::run_for_open(&mut conn)?; Ok(Self { config, @@ -235,6 +211,10 @@ impl SqlitePersister { buffer: Buffer::new(), #[cfg(any(test, feature = "__test-helpers"))] primed_flush_error: Mutex::new(None), + #[cfg(any(test, feature = "__test-helpers"))] + post_backup_hook: Mutex::new(None), + #[cfg(any(test, feature = "__test-helpers"))] + primed_pre_flush_error: Mutex::new(None), }) } @@ -371,13 +351,11 @@ impl SqlitePersister { wallet_id: WalletId, skip_backup: bool, ) -> Result { - // CMT-008: acquire the connection mutex FIRST and hold it - // across drain → existence-check → backup → delete-transaction - // → post-commit buffer wipe. Concurrent `store()` calls in - // Immediate mode block on this guard (their flush takes conn); - // Manual-mode stores can still buffer, so we re-drain after - // commit to discard any racing writes (the wallet is going - // away — those writes are intentionally void). + // CMT-008: acquire the connection mutex FIRST so concurrent + // in-process `store()` calls block on it. Cross-process peers + // (other rusqlite Connections / sibling `SqlitePersister`s) are + // excluded by `BEGIN EXCLUSIVE` below — the in-process mutex + // alone never gave that guarantee. let mut conn = self.conn()?; // Drain the buffered changeset so a later flush can't @@ -404,9 +382,9 @@ impl SqlitePersister { }; let result: Result = (|| { - // A wallet exists iff it was buffered OR persisted. Refusing - // on a truly-unknown wallet must not waste a backup file. - let exists_in_db = conn + // Pre-flight existence check on the bare conn (no tx) so + // we don't waste a backup file on an unknown wallet. + let exists_pre_flush = conn .query_row( "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1", rusqlite::params![wallet_id.as_slice()], @@ -414,9 +392,77 @@ impl SqlitePersister { ) .optional()? .is_some(); - if !had_buffered && !exists_in_db { + if !had_buffered && !exists_pre_flush { return Err(WalletStorageError::WalletNotFound { wallet_id }); } + + // Test-only injector for TC-CODE-006-2 — force the pre- + // flush below to fail with the primed error without + // depending on a real SQL failure. Keeps the test free of + // FK-poisoning scaffolding. + #[cfg(any(test, feature = "__test-helpers"))] + let primed_pre_flush_error = self.consume_primed_pre_flush_error(); + + // CODE-006: flush the drained buffer to disk BEFORE + // `run_auto_backup` so the pre-delete snapshot includes + // every pending write. Without this the backup captures + // only already-persisted state and rollback-from-backup + // cannot recover the buffered (lost) data. + // + // The flush opens its own EXCLUSIVE tx and commits; + // `run_auto_backup` then runs against the freshly-flushed + // DB. On flush failure we restore the buffer via the outer + // `restore_buffer` helper and abort the delete — mirrors + // CMT-002. + // + // The cascade-side backup runs BEFORE the cascade's + // `BEGIN EXCLUSIVE` because rusqlite's `Backup::new` can't + // establish a backup whose source connection holds an + // active write tx on its own DB — `sqlite3_backup_step` + // would deadlock against the in-flight EXCLUSIVE. The + // post-EXCLUSIVE re-check below handles cross-process + // peers that mutate the wallet between snapshot and lock. + if let Some(cs) = drained_slot.take() { + #[cfg(any(test, feature = "__test-helpers"))] + if let Some(primed) = primed_pre_flush_error { + drained_slot.set(Some(cs)); + return Err(primed); + } + let pre_flush_tx = + conn.transaction_with_behavior(rusqlite::TransactionBehavior::Exclusive)?; + if let Err(e) = apply_changeset_to_tx(&pre_flush_tx, &wallet_id, &cs) { + let _ = pre_flush_tx.rollback(); + drained_slot.set(Some(cs)); + return Err(e); + } + if let Err(e) = pre_flush_tx.commit() { + drained_slot.set(Some(cs)); + return Err(WalletStorageError::Sqlite(e)); + } + } + + // Re-evaluate existence after the pre-flush: a buffered- + // only wallet now has rows on disk. + let exists_in_db = if exists_pre_flush { + true + } else { + conn.query_row( + "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1", + rusqlite::params![wallet_id.as_slice()], + |_| Ok(()), + ) + .optional()? + .is_some() + }; + + // Snapshot the wallet's footprint BEFORE auto_backup so + // the post-EXCLUSIVE re-check has a baseline to compare + // against. `wallet_footprint` queries every PER_WALLET_TABLES + // row count; mismatches between pre-backup and post-lock + // mean a peer mutated the wallet inside the lock-free + // window the rusqlite Backup API forces us to leave open. + let pre_backup_footprint = wallet_footprint(&conn, &wallet_id)?; + let backup_path = if skip_backup { None } else { @@ -427,21 +473,77 @@ impl SqlitePersister { AutoBackupOperation::DeleteWallet, )? }; - let tx = conn.transaction()?; + + // Test-only hook: fires between the backup snapshot and + // the cascade EXCLUSIVE so TC-CODE-006-3 can simulate a + // cross-process peer that mutates `wallet_metadata` in + // the gap rusqlite's Backup API forces us to leave open. + #[cfg(any(test, feature = "__test-helpers"))] + self.consume_post_backup_hook(); + + // SQLite-native EXCLUSIVE for the cascade window. Excludes + // cross-process peers (other rusqlite Connections, sibling + // `SqlitePersister`s) that would otherwise commit rows for + // `wallet_id` between the backup snapshot and the cascade. + // The in-process mutex on `conn` alone never gave that + // guarantee. Peers waiting on the lock back off via + // SQLite's `busy_timeout`. + let tx = conn.transaction_with_behavior(rusqlite::TransactionBehavior::Exclusive)?; + + // Re-snapshot the wallet's footprint under EXCLUSIVE and + // compare against the pre-backup snapshot. Any change means + // a peer mutated the wallet between the backup and the lock + // acquisition — the backup we just took is now inconsistent + // with the live state, so rollback-from-backup would + // silently lose those writes. Abort with the typed + // `ConcurrentMutationDuringDelete` so the operator can + // retry after quiescing the peer. + let post_lock_footprint = wallet_footprint_tx(&tx, &wallet_id)?; + if post_lock_footprint != pre_backup_footprint { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + pre_backup = ?pre_backup_footprint, + post_lock = ?post_lock_footprint, + "delete_wallet aborted: peer mutated wallet between auto-backup and EXCLUSIVE" + ); + // Roll back the empty EXCLUSIVE — no destructive work + // has happened yet inside this tx, so drop is enough, + // but be explicit. + let _ = tx.rollback(); + return Err(WalletStorageError::ConcurrentMutationDuringDelete { wallet_id }); + } + + // Cross-check existence as a defensive log: post_lock + // footprint equality already implies same existence, but + // keep the structured log for ops visibility. + let post_lock_exists = post_lock_footprint + .iter() + .any(|(table, n)| *table == "wallet_metadata" && *n > 0); + if post_lock_exists != exists_in_db { + tracing::info!( + wallet_id = %hex::encode(wallet_id), + pre_lock_exists = exists_in_db, + post_lock_exists, + "wallet_metadata footprint changed across delete_wallet EXCLUSIVE acquisition" + ); + } + let mut rows_removed_per_table = BTreeMap::new(); - for &table in PER_WALLET_TABLES { + for (table, scope) in PER_WALLET_TABLES { // SQL injection note: `table` comes from a `&'static // &'static str` constant compiled into the binary. There - // is no user input on this path. + // is no user input on this path. The SQL flavour + // (direct column vs. JOIN via `identities`) is picked + // by `count_rows_for_wallet_sql`. let n: i64 = tx .query_row( - &format!("SELECT COUNT(*) FROM {table} WHERE wallet_id = ?1"), + &count_rows_for_wallet_sql(table, *scope), rusqlite::params![wallet_id.as_slice()], |row| row.get(0), ) .optional()? .unwrap_or(0); - rows_removed_per_table.insert(table, usize::try_from(n).unwrap_or(usize::MAX)); + rows_removed_per_table.insert(*table, usize::try_from(n).unwrap_or(usize::MAX)); } crate::sqlite::schema::wallet_meta::delete(&tx, &wallet_id)?; tx.commit()?; @@ -469,8 +571,13 @@ impl SqlitePersister { result } - /// In Manual mode: attempt to flush every dirty wallet. In - /// Immediate mode: no-op (returns an empty report). + /// Attempt to flush every dirty wallet, regardless of flush mode. + /// + /// In `Manual` mode this is the only way pending writes become + /// durable. In `Immediate` mode the buffer is normally empty (each + /// `store` flushes inline) but a transient failure during `store` + /// leaves the changeset in the buffer — `commit_writes` is the + /// retry path that drains those leftovers. /// /// Continues past per-wallet failures instead of fails-fast (N-1). /// Each wallet's flush outcome lands on the returned @@ -483,14 +590,21 @@ impl SqlitePersister { /// (e.g. the buffer mutex is poisoned). Once the loop starts, /// every dirty wallet has a slot in the report. pub fn commit_writes(&self) -> Result { + self.commit_writes_inner() + } + + fn commit_writes_inner(&self) -> Result { let mut report = CommitReport { succeeded: Vec::new(), failed: Vec::new(), still_pending: Vec::new(), }; - if matches!(self.config.flush_mode, FlushMode::Immediate) { - return Ok(report); - } + // Even in `FlushMode::Immediate` the buffer can be non-empty: + // a transient failure during `store()` re-merges the changeset + // back into the buffer via `handle_flush_error`. The retry path + // — `commit_writes()` — has to drain that leftover regardless + // of flush mode, otherwise transient-failure data sits there + // until the next per-wallet `store` happens to retry it. let dirty = self .buffer .dirty_wallets() @@ -522,13 +636,15 @@ impl SqlitePersister { ) -> Result, WalletStorageError> { let conn = self.conn()?; let mut out = Vec::with_capacity(PER_WALLET_TABLES.len()); - for &table in PER_WALLET_TABLES { + for (table, scope) in PER_WALLET_TABLES { // `table` is a compile-time constant — no SQL injection - // surface despite the `format!`. + // surface despite the `format!`. Per-wallet predicate uses + // `count_rows_for_wallet_sql` so identity-scoped tables + // join through `identities`. let n: i64 = match wallet_id { Some(id) => conn .query_row( - &format!("SELECT COUNT(*) FROM {table} WHERE wallet_id = ?1"), + &count_rows_for_wallet_sql(table, *scope), rusqlite::params![id.as_slice()], |row| row.get(0), ) @@ -541,7 +657,7 @@ impl SqlitePersister { .optional()? .unwrap_or(0), }; - out.push((table, usize::try_from(n).unwrap_or(usize::MAX))); + out.push((*table, usize::try_from(n).unwrap_or(usize::MAX))); } Ok(out) } @@ -609,44 +725,7 @@ impl SqlitePersister { ) -> Result<(), WalletStorageError> { let mut conn = self.conn()?; let tx = conn.transaction()?; - if let Some(meta) = cs.wallet_metadata.as_ref() { - schema::wallet_meta::upsert(&tx, wallet_id, meta)?; - } - if !cs.account_registrations.is_empty() { - schema::accounts::apply_registrations(&tx, wallet_id, &cs.account_registrations)?; - } - if !cs.account_address_pools.is_empty() { - schema::accounts::apply_pools(&tx, wallet_id, &cs.account_address_pools)?; - } - if let Some(core) = cs.core.as_ref() { - schema::core_state::apply(&tx, wallet_id, core)?; - } - if let Some(identities) = cs.identities.as_ref() { - schema::identities::apply(&tx, wallet_id, identities)?; - } - if let Some(keys) = cs.identity_keys.as_ref() { - schema::identity_keys::apply(&tx, wallet_id, keys)?; - } - if let Some(contacts) = cs.contacts.as_ref() { - schema::contacts::apply(&tx, wallet_id, contacts)?; - } - if let Some(addrs) = cs.platform_addresses.as_ref() { - schema::platform_addrs::apply(&tx, wallet_id, addrs)?; - } - if let Some(locks) = cs.asset_locks.as_ref() { - schema::asset_locks::apply(&tx, wallet_id, locks)?; - } - if let Some(balances) = cs.token_balances.as_ref() { - schema::token_balances::apply(&tx, wallet_id, balances)?; - } - if cs.dashpay_profiles.is_some() || cs.dashpay_payments_overlay.is_some() { - schema::dashpay::apply( - &tx, - wallet_id, - cs.dashpay_profiles.as_ref(), - cs.dashpay_payments_overlay.as_ref(), - )?; - } + apply_changeset_to_tx(&tx, wallet_id, cs)?; tx.commit()?; Ok(()) } @@ -742,6 +821,64 @@ impl SqlitePersister { .expect("primed_flush_error") .take() } + + /// Test-only: arm a one-shot callback fired by `delete_wallet` + /// after the pre-delete backup snapshot completes and before the + /// cascade EXCLUSIVE tx begins. The callback is consumed (taken) + /// on first fire — subsequent deletes see the slot empty. + #[doc(hidden)] + #[cfg(any(test, feature = "__test-helpers"))] + pub fn arm_post_backup_hook(&self, hook: F) + where + F: FnOnce() + Send + 'static, + { + *self.post_backup_hook.lock().expect("post_backup_hook") = Some(Box::new(hook)); + } + + #[cfg(any(test, feature = "__test-helpers"))] + fn consume_post_backup_hook(&self) { + let hook = self + .post_backup_hook + .lock() + .expect("post_backup_hook") + .take(); + if let Some(hook) = hook { + hook(); + } + } + + /// Test-only: arm a one-shot pre-flush failure for the next + /// `delete_wallet` call. The injection fires only when there is + /// a drained buffered changeset to flush — i.e. when `delete_wallet` + /// actually exercises the pre-flush branch. + #[doc(hidden)] + #[cfg(any(test, feature = "__test-helpers"))] + pub fn force_next_pre_flush_to_fail(&self, err: WalletStorageError) { + *self + .primed_pre_flush_error + .lock() + .expect("primed_pre_flush_error") = Some(err); + } + + #[cfg(any(test, feature = "__test-helpers"))] + fn consume_primed_pre_flush_error(&self) -> Option { + self.primed_pre_flush_error + .lock() + .expect("primed_pre_flush_error") + .take() + } + + /// Test-only: probe whether the wallet has a buffered changeset. + /// Used by TC-CODE-006-2 to assert the buffer survives a failed + /// pre-flush without consuming it. + #[doc(hidden)] + #[cfg(any(test, feature = "__test-helpers"))] + pub fn buffer_has_changeset_for_test(&self, wallet_id: &WalletId) -> bool { + self.buffer + .dirty_wallets() + .map(|v| v.iter().any(|w| w == wallet_id)) + .unwrap_or(false) + } } /// ATOM-007 (N-2): when a `Manual`-mode persister is dropped while @@ -755,7 +892,7 @@ impl SqlitePersister { /// persisters are durable on every `store` so they never trip this. impl Drop for SqlitePersister { fn drop(&mut self) { - if !matches!(self.config.flush_mode, FlushMode::Manual) { + if self.config.flush_mode != FlushMode::Manual { return; } // `dirty_wallets` only fails on a poisoned buffer mutex. A @@ -930,6 +1067,22 @@ impl PlatformWalletPersistence for SqlitePersister { let conn = self.conn().map_err(PersistenceError::from)?; schema::core_state::get_tx_record(&conn, &wallet_id, txid).map_err(PersistenceError::from) } + + /// Trait-dispatch entry into the safe-by-default cascade delete. + /// Always takes an auto-backup (`auto_backup_dir` must be set, else + /// returns `WalletStorageError::AutoBackupDisabled` mapped into a + /// fatal `PersistenceError`). The inherent + /// [`SqlitePersister::delete_wallet_skip_backup`] stays available + /// for the CLI's `--no-auto-backup` flag and isn't reachable + /// through the trait by design. + fn delete_wallet(&self, wallet_id: WalletId) -> Result { + self.delete_wallet_inner(wallet_id, false) + .map_err(PersistenceError::from) + } + + fn commit_writes(&self) -> Result { + self.commit_writes_inner() + } } // ----- Helpers ----- @@ -968,6 +1121,33 @@ fn validate_config(config: &SqlitePersisterConfig) -> Result<(), WalletStorageEr reason: "synchronous=Off is rejected (data-loss footgun)", }); } + // `journal_mode=Memory` keeps the rollback journal in RAM and + // `journal_mode=Off` disables it outright. Either turns crash- + // safety into a coin flip for a wallet DB — reject loudly instead + // of silently corrupting on the next power loss. + match config.journal_mode { + crate::sqlite::config::JournalMode::Memory => { + return Err(WalletStorageError::ConfigInvalid { + reason: "journal_mode=Memory is rejected (crash-unsafe)", + }); + } + crate::sqlite::config::JournalMode::Off => { + return Err(WalletStorageError::ConfigInvalid { + reason: "journal_mode=Off is rejected (crash-unsafe)", + }); + } + _ => {} + } + // `busy_timeout=0` makes contended writers fail-fast with BUSY + // instead of waiting — non-fatal, but the operator almost certainly + // didn't mean it. Warn rather than reject because a few tests + // legitimately want the fail-fast behaviour. + if config.busy_timeout.is_zero() { + tracing::warn!( + "SqlitePersisterConfig.busy_timeout=0; contended writers will return BUSY \ + instead of waiting — set a non-zero timeout (default 5s) unless this is intentional" + ); + } Ok(()) } @@ -987,6 +1167,57 @@ fn apply_pragmas( Ok(()) } +/// Apply every populated sub-changeset of `cs` against the supplied +/// SQLite transaction. Does not commit; the caller owns the tx +/// lifecycle. Splitting this out from `write_changeset_in_one_tx` +/// lets `delete_wallet_inner` flush a drained buffer into a bespoke +/// pre-delete tx (CODE-006) without re-opening the connection. +fn apply_changeset_to_tx( + tx: &rusqlite::Transaction<'_>, + wallet_id: &WalletId, + cs: &PlatformWalletChangeSet, +) -> Result<(), WalletStorageError> { + if let Some(meta) = cs.wallet_metadata.as_ref() { + schema::wallet_meta::upsert(tx, wallet_id, meta)?; + } + if !cs.account_registrations.is_empty() { + schema::accounts::apply_registrations(tx, wallet_id, &cs.account_registrations)?; + } + if !cs.account_address_pools.is_empty() { + schema::accounts::apply_pools(tx, wallet_id, &cs.account_address_pools)?; + } + if let Some(core) = cs.core.as_ref() { + schema::core_state::apply(tx, wallet_id, core)?; + } + if let Some(identities) = cs.identities.as_ref() { + schema::identities::apply(tx, wallet_id, identities)?; + } + if let Some(keys) = cs.identity_keys.as_ref() { + schema::identity_keys::apply(tx, wallet_id, keys)?; + } + if let Some(contacts) = cs.contacts.as_ref() { + schema::contacts::apply(tx, wallet_id, contacts)?; + } + if let Some(addrs) = cs.platform_addresses.as_ref() { + schema::platform_addrs::apply(tx, wallet_id, addrs)?; + } + if let Some(locks) = cs.asset_locks.as_ref() { + schema::asset_locks::apply(tx, wallet_id, locks)?; + } + if let Some(balances) = cs.token_balances.as_ref() { + schema::token_balances::apply(tx, wallet_id, balances)?; + } + if cs.dashpay_profiles.is_some() || cs.dashpay_payments_overlay.is_some() { + schema::dashpay::apply( + tx, + wallet_id, + cs.dashpay_profiles.as_ref(), + cs.dashpay_payments_overlay.as_ref(), + )?; + } + Ok(()) +} + /// Take a single auto-backup. Shared code path for open-time /// (pre-migration), pre-restore, and pre-delete invocations. Returns /// the absolute path written, or [`WalletStorageError::AutoBackupDisabled`] @@ -1035,15 +1266,7 @@ fn count_pending( conn: &mut Connection, embedded: &[(i32, String)], ) -> Result { - let table_exists = conn - .query_row( - "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'refinery_schema_history'", - [], - |_| Ok(()), - ) - .optional()? - .is_some(); - if !table_exists { + if !crate::sqlite::migrations::has_schema_history(conn)? { return Ok(embedded.len()); } let applied: std::collections::HashSet = { @@ -1058,6 +1281,50 @@ fn count_pending( .count()) } +/// Per-wallet footprint fingerprint: `(table_name, row_count)` for +/// every entry in `PER_WALLET_TABLES`. Used by `delete_wallet_inner` +/// to detect cross-process mutations between the pre-delete backup +/// snapshot and the cascade's EXCLUSIVE acquisition. +fn wallet_footprint( + conn: &Connection, + wallet_id: &WalletId, +) -> Result, WalletStorageError> { + let mut out = Vec::with_capacity(PER_WALLET_TABLES.len()); + for (table, scope) in PER_WALLET_TABLES { + let n: i64 = conn + .query_row( + &count_rows_for_wallet_sql(table, *scope), + rusqlite::params![wallet_id.as_slice()], + |row| row.get(0), + ) + .optional()? + .unwrap_or(0); + out.push((*table, n)); + } + Ok(out) +} + +/// Same as [`wallet_footprint`] but on an open transaction so the +/// post-EXCLUSIVE re-check sees the locked snapshot. +fn wallet_footprint_tx( + tx: &rusqlite::Transaction<'_>, + wallet_id: &WalletId, +) -> Result, WalletStorageError> { + let mut out = Vec::with_capacity(PER_WALLET_TABLES.len()); + for (table, scope) in PER_WALLET_TABLES { + let n: i64 = tx + .query_row( + &count_rows_for_wallet_sql(table, *scope), + rusqlite::params![wallet_id.as_slice()], + |row| row.get(0), + ) + .optional()? + .unwrap_or(0); + out.push((*table, n)); + } + Ok(out) +} + fn current_schema_version(conn: &Connection) -> Result, WalletStorageError> { let row = conn .query_row( diff --git a/packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs b/packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs index becb60bfe6..0638b2585b 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs @@ -1,4 +1,16 @@ //! `dashpay_profiles` + `dashpay_payments_overlay` writers. +//! +//! # Precondition +//! +//! Every `identity_id` in the supplied profile / payment maps MUST +//! already exist in the `identities` table and belong to the flush's +//! `wallet_id`. The writer relies on the +//! `identities(identity_id, wallet_id)` row produced by +//! [`super::identities::apply`] (in the same transaction or earlier) +//! for parenting; the FK to `identities(identity_id)` enforces the +//! existence half, but not the wallet match. A `debug_assert!` below +//! catches mis-attributed callers in development builds at no +//! production cost. use std::collections::BTreeMap; @@ -11,36 +23,49 @@ use platform_wallet::wallet::platform_wallet::WalletId; use crate::sqlite::error::WalletStorageError; use crate::sqlite::schema::blob; -/// Apply both dashpay overlays. +/// Both dashpay tables are keyed by identity only; their FK targets +/// `identities(identity_id)` so cascade flows through the +/// `wallet_metadata → identities` chain. +/// +/// The `_wallet_id` parameter is kept on the signature for symmetry +/// with the persister's `write_changeset_in_one_tx` dispatch table, +/// and feeds the precondition debug-assert; it does not feed any +/// column. pub fn apply( tx: &Transaction<'_>, wallet_id: &WalletId, profiles: Option<&BTreeMap>>, payments: Option<&BTreeMap>>, ) -> Result<(), WalletStorageError> { + // Precondition check (debug builds only): every identity_id we + // touch must already belong to the flush-scope wallet (or to no + // wallet if scope is the sentinel). Cheap SELECT inside the same + // tx; compiled out in release builds. + if cfg!(debug_assertions) { + let touched: std::collections::BTreeSet = profiles + .iter() + .flat_map(|m| m.keys().copied()) + .chain(payments.iter().flat_map(|m| m.keys().copied())) + .collect(); + super::assert_identities_belong_to_wallet(tx, wallet_id, &touched)?; + } if let Some(profiles) = profiles { if !profiles.is_empty() { - let mut delete_stmt = tx.prepare_cached( - "DELETE FROM dashpay_profiles WHERE wallet_id = ?1 AND identity_id = ?2", - )?; + let mut delete_stmt = + tx.prepare_cached("DELETE FROM dashpay_profiles WHERE identity_id = ?1")?; let mut insert_stmt = tx.prepare_cached( - "INSERT INTO dashpay_profiles (wallet_id, identity_id, profile_blob) \ - VALUES (?1, ?2, ?3) \ - ON CONFLICT(wallet_id, identity_id) DO UPDATE SET profile_blob = excluded.profile_blob", + "INSERT INTO dashpay_profiles (identity_id, profile_blob) \ + VALUES (?1, ?2) \ + ON CONFLICT(identity_id) DO UPDATE SET profile_blob = excluded.profile_blob", )?; for (identity_id, profile) in profiles { match profile { None => { - delete_stmt - .execute(params![wallet_id.as_slice(), identity_id.as_slice()])?; + delete_stmt.execute(params![identity_id.as_slice()])?; } Some(p) => { let payload = blob::encode(p)?; - insert_stmt.execute(params![ - wallet_id.as_slice(), - identity_id.as_slice(), - payload - ])?; + insert_stmt.execute(params![identity_id.as_slice(), payload])?; } } } @@ -50,19 +75,14 @@ pub fn apply( if !payments.is_empty() { let mut stmt = tx.prepare_cached( "INSERT INTO dashpay_payments_overlay \ - (wallet_id, identity_id, payment_id, overlay_blob) \ - VALUES (?1, ?2, ?3, ?4) \ - ON CONFLICT(wallet_id, identity_id, payment_id) DO UPDATE SET overlay_blob = excluded.overlay_blob", + (identity_id, payment_id, overlay_blob) \ + VALUES (?1, ?2, ?3) \ + ON CONFLICT(identity_id, payment_id) DO UPDATE SET overlay_blob = excluded.overlay_blob", )?; for (identity_id, by_tx) in payments { for (tx_id, entry) in by_tx { let payload = blob::encode(entry)?; - stmt.execute(params![ - wallet_id.as_slice(), - identity_id.as_slice(), - tx_id, - payload - ])?; + stmt.execute(params![identity_id.as_slice(), tx_id, payload])?; } } } diff --git a/packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs b/packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs index 006da164a4..307327b9ff 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs @@ -14,35 +14,79 @@ pub fn apply( cs: &IdentityChangeSet, ) -> Result<(), WalletStorageError> { if !cs.identities.is_empty() { + // PK is `identity_id` alone; `wallet_id` is nullable and links + // the identity to its parent wallet for cascade. The all-zero + // wallet id is treated as "no parent wallet known" and stored + // as NULL so the FK to `wallet_metadata` doesn't activate. + // + // COALESCE order — `COALESCE(identities.wallet_id, + // excluded.wallet_id)` — preserves an already-parented row's + // wallet_id on re-upsert; the excluded value only fills when + // the on-disk row is still NULL. This is the orphan → parented + // promotion path; the reverse (mismatched re-parent) is caught + // by the per-entry cross-check below. + let scope_is_sentinel = wallet_id.iter().all(|b| *b == 0); let mut stmt = tx.prepare_cached( - "INSERT INTO identities (wallet_id, wallet_index, identity_id, entry_blob, tombstoned) \ + "INSERT INTO identities (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ VALUES (?1, ?2, ?3, ?4, 0) \ - ON CONFLICT(wallet_id, identity_id) DO UPDATE SET \ + ON CONFLICT(identity_id) DO UPDATE SET \ + wallet_id = COALESCE(identities.wallet_id, excluded.wallet_id), \ wallet_index = excluded.wallet_index, \ entry_blob = excluded.entry_blob, \ tombstoned = 0", )?; + let wallet_id_param = wallet_id_to_param(wallet_id); for (id, entry) in &cs.identities { + // Cross-check: the entry's own wallet_id (when set) must + // agree with the flush scope so the typed columns and the + // serialized blob describe the same parenting. Sentinel + // scope ("no parent wallet known") requires the entry's + // wallet_id to also be `None` — otherwise a real wallet's + // identity would be written under the orphan slot. + if let Some(entry_wallet_id) = entry.wallet_id { + if scope_is_sentinel { + return Err(WalletStorageError::WalletIdMismatch { + expected: [0u8; 32], + found: entry_wallet_id, + }); + } + if entry_wallet_id != *wallet_id { + return Err(WalletStorageError::WalletIdMismatch { + expected: *wallet_id, + found: entry_wallet_id, + }); + } + } let payload = blob::encode(entry)?; stmt.execute(params![ - wallet_id.as_slice(), - entry.identity_index.map(i64::from), id.as_slice(), + wallet_id_param, + entry.identity_index.map(i64::from), payload, ])?; } } if !cs.removed.is_empty() { - let mut stmt = tx.prepare_cached( - "UPDATE identities SET tombstoned = 1 WHERE wallet_id = ?1 AND identity_id = ?2", - )?; + let mut stmt = + tx.prepare_cached("UPDATE identities SET tombstoned = 1 WHERE identity_id = ?1")?; for id in &cs.removed { - stmt.execute(params![wallet_id.as_slice(), id.as_slice()])?; + stmt.execute(params![id.as_slice()])?; } } Ok(()) } +/// Map the caller-supplied `WalletId` (32 bytes) to the nullable +/// `identities.wallet_id` column: the all-zero id is treated as "no +/// parent wallet" and stored as NULL so the FK doesn't activate. +fn wallet_id_to_param(wallet_id: &WalletId) -> Option<&[u8]> { + if wallet_id.iter().all(|b| *b == 0) { + None + } else { + Some(wallet_id.as_slice()) + } +} + /// Decode a single `identities` row back to its [`IdentityEntry`]. /// /// Returns `Ok(None)` if no row matches. This reads only `entry_blob` @@ -56,10 +100,16 @@ pub fn fetch( identity_id: &[u8; 32], ) -> Result, WalletStorageError> { use rusqlite::OptionalExtension; + // Scope the lookup to the caller's wallet so a peer wallet that + // happens to share the identity-id row can never leak through. + // The sentinel WalletId (all zeros) matches orphan rows (NULL + // wallet_id); a real WalletId matches only that wallet's rows. + // `IS` is NULL-safe equality so the NULL branch works uniformly. + let wallet_id_param = wallet_id_to_param(wallet_id); let row: Option> = conn .query_row( - "SELECT entry_blob FROM identities WHERE wallet_id = ?1 AND identity_id = ?2", - params![wallet_id.as_slice(), &identity_id[..]], + "SELECT entry_blob FROM identities WHERE identity_id = ?1 AND wallet_id IS ?2", + params![&identity_id[..], wallet_id_param], |row| row.get(0), ) .optional()?; @@ -84,6 +134,10 @@ pub fn load_state( ) -> Result { use platform_wallet::changeset::IdentityManagerStartState; + // `identities.wallet_id` is nullable; this load path wants only the + // rows belonging to the wallet the caller asked for, so the WHERE + // clause matches by wallet_id (orphan identities — wallet_id NULL — + // are out of scope for this per-wallet loader). let mut stmt = conn.prepare( "SELECT identity_id, entry_blob, tombstoned FROM identities WHERE wallet_id = ?1", )?; @@ -177,11 +231,12 @@ pub fn ensure_exists( dashpay_payments: Default::default(), }; let payload = blob::encode(&stub)?; + let wallet_id_param = wallet_id_to_param(wallet_id); conn.execute( "INSERT OR IGNORE INTO identities \ - (wallet_id, wallet_index, identity_id, entry_blob, tombstoned) \ - VALUES (?1, NULL, ?2, ?3, 0)", - params![wallet_id.as_slice(), &identity_id[..], payload], + (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ + VALUES (?1, ?2, NULL, ?3, 0)", + params![&identity_id[..], wallet_id_param, payload], )?; Ok(()) } diff --git a/packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs b/packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs index 82474c6826..ccc3cda1aa 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs @@ -73,6 +73,10 @@ impl IdentityKeyWire { } } +/// `identity_keys` is keyed by `(identity_id, key_id)`; the parent FK +/// targets `identities(identity_id)`. The caller-supplied [`WalletId`] +/// scopes cross-checks against the entry's own `wallet_id` field so +/// the entry-blob and the typed columns stay aligned. pub fn apply( tx: &Transaction<'_>, wallet_id: &WalletId, @@ -81,30 +85,38 @@ pub fn apply( if !cs.upserts.is_empty() { let mut stmt = tx.prepare_cached( "INSERT INTO identity_keys \ - (wallet_id, identity_id, key_id, public_key_blob, public_key_hash, derivation_blob) \ - VALUES (?1, ?2, ?3, ?4, ?5, NULL) \ - ON CONFLICT(wallet_id, identity_id, key_id) DO UPDATE SET \ + (identity_id, key_id, public_key_blob, public_key_hash, derivation_blob) \ + VALUES (?1, ?2, ?3, ?4, NULL) \ + ON CONFLICT(identity_id, key_id) DO UPDATE SET \ public_key_blob = excluded.public_key_blob, \ public_key_hash = excluded.public_key_hash, \ derivation_blob = NULL", )?; for ((identity_id, key_id), entry) in &cs.upserts { // Reject any disagreement between the map key / outer - // wallet_id (what the typed columns are bound from) and the - // entry fields (what the serialized blob carries) so the two + // wallet_id (informational scope) and the entry fields + // (what the serialized blob carries) so the two // representations of a row can never diverge on disk. if entry.identity_id != *identity_id || entry.key_id != *key_id { return Err(WalletStorageError::IdentityKeyEntryMismatch); } - if let Some(entry_wallet_id) = entry.wallet_id { - if entry_wallet_id != *wallet_id { + // Sentinel scope ("no parent wallet known") requires the + // entry's wallet_id to also be `None`; a real entry + // wallet_id under sentinel scope would silently file the + // key under the wrong parenting. Non-sentinel scope + // requires the entry's wallet_id (when set) to match + // exactly. + let scope_is_sentinel = wallet_id.iter().all(|b| *b == 0); + match (scope_is_sentinel, entry.wallet_id) { + (true, Some(_)) => return Err(WalletStorageError::IdentityKeyEntryMismatch), + (false, Some(entry_wallet_id)) if entry_wallet_id != *wallet_id => { return Err(WalletStorageError::IdentityKeyEntryMismatch); } + _ => {} } let wire = IdentityKeyWire::from_entry(entry)?; let entry_blob = blob::encode(&wire)?; stmt.execute(params![ - wallet_id.as_slice(), identity_id.as_slice(), i64::from(*key_id), entry_blob, @@ -113,16 +125,10 @@ pub fn apply( } } if !cs.removed.is_empty() { - let mut stmt = tx.prepare_cached( - "DELETE FROM identity_keys \ - WHERE wallet_id = ?1 AND identity_id = ?2 AND key_id = ?3", - )?; + let mut stmt = + tx.prepare_cached("DELETE FROM identity_keys WHERE identity_id = ?1 AND key_id = ?2")?; for (identity_id, key_id) in &cs.removed { - stmt.execute(params![ - wallet_id.as_slice(), - identity_id.as_slice(), - i64::from(*key_id), - ])?; + stmt.execute(params![identity_id.as_slice(), i64::from(*key_id)])?; } } Ok(()) diff --git a/packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs b/packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs index 95fc77ac13..954fbc88d0 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs @@ -24,29 +24,125 @@ pub mod platform_addrs; pub mod token_balances; pub mod wallet_meta; +/// How a per-wallet table is row-scoped against a `wallet_id`. +/// Identity-owned tables (`identity_keys`, `token_balances`, +/// `dashpay_profiles`, `dashpay_payments_overlay`) have no direct +/// `wallet_id` column; they reach the parent wallet only via the +/// cascading FK chain `wallet_metadata → identities → …`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum WalletScope { + /// The table carries a `wallet_id` column directly; predicates + /// like `WHERE wallet_id = ?` work as-is. + DirectColumn, + /// The table is keyed by `identity_id`; lookups by wallet must + /// JOIN through `identities` (`SELECT … WHERE identity_id IN + /// (SELECT identity_id FROM identities WHERE wallet_id = ?)`). + ViaIdentity, +} + /// Every per-wallet table — used by `delete_wallet` to count + cascade /// row removal and by `inspect` for the table summary. `wallet_metadata` /// is the parent and listed first; everything after it depends on the /// parent row via the native `ON DELETE CASCADE` foreign keys declared -/// in `V001__initial.rs`. -pub const PER_WALLET_TABLES: &[&str] = &[ - "wallet_metadata", - "account_registrations", - "account_address_pools", - "core_transactions", - "core_utxos", - "core_instant_locks", - "core_derived_addresses", - "core_sync_state", - "identities", - "identity_keys", - "contacts_sent", - "contacts_recv", - "contacts_established", - "platform_addresses", - "platform_address_sync", - "asset_locks", - "token_balances", - "dashpay_profiles", - "dashpay_payments_overlay", +/// in `V001__initial.rs`. Identity-owned children cascade through +/// `identities` (nullable `wallet_id` link) rather than directly off +/// `wallet_metadata`. +pub const PER_WALLET_TABLES: &[(&str, WalletScope)] = &[ + ("wallet_metadata", WalletScope::DirectColumn), + ("account_registrations", WalletScope::DirectColumn), + ("account_address_pools", WalletScope::DirectColumn), + ("core_transactions", WalletScope::DirectColumn), + ("core_utxos", WalletScope::DirectColumn), + ("core_instant_locks", WalletScope::DirectColumn), + ("core_derived_addresses", WalletScope::DirectColumn), + ("core_sync_state", WalletScope::DirectColumn), + ("identities", WalletScope::DirectColumn), + ("identity_keys", WalletScope::ViaIdentity), + ("contacts_sent", WalletScope::DirectColumn), + ("contacts_recv", WalletScope::DirectColumn), + ("contacts_established", WalletScope::DirectColumn), + ("platform_addresses", WalletScope::DirectColumn), + ("platform_address_sync", WalletScope::DirectColumn), + ("asset_locks", WalletScope::DirectColumn), + ("token_balances", WalletScope::ViaIdentity), + ("dashpay_profiles", WalletScope::ViaIdentity), + ("dashpay_payments_overlay", WalletScope::ViaIdentity), ]; + +/// SQL fragment for counting rows of `table` belonging to a single +/// wallet. `scope` selects the predicate flavour. The fragment includes +/// the leading `SELECT COUNT(*) FROM` so the call site can format it +/// directly and bind a single `?1` parameter (the wallet id bytes). +pub fn count_rows_for_wallet_sql(table: &str, scope: WalletScope) -> String { + match scope { + WalletScope::DirectColumn => { + format!("SELECT COUNT(*) FROM {table} WHERE wallet_id = ?1") + } + WalletScope::ViaIdentity => format!( + "SELECT COUNT(*) FROM {table} \ + WHERE identity_id IN (SELECT identity_id FROM identities WHERE wallet_id = ?1)" + ), + } +} + +/// Defensive check that every `identity_id` in `touched` exists in +/// `identities` and belongs to `wallet_id` (or has NULL wallet_id when +/// scope is the all-zero sentinel). Used by identity-owned writers +/// (`dashpay`, `token_balances`) to catch mis-attributed callers in +/// debug builds; release builds skip the call entirely. +/// +/// Returns [`WalletStorageError::WalletIdMismatch`] for the first +/// offending row found. Rows that don't exist in `identities` aren't +/// flagged here — the FK on the child table will reject the write. +pub(crate) fn assert_identities_belong_to_wallet( + tx: &rusqlite::Transaction<'_>, + wallet_id: &platform_wallet::wallet::platform_wallet::WalletId, + touched: &std::collections::BTreeSet, +) -> Result<(), crate::sqlite::error::WalletStorageError> { + use crate::sqlite::error::WalletStorageError; + use rusqlite::OptionalExtension; + let scope_is_sentinel = wallet_id.iter().all(|b| *b == 0); + let mut stmt = tx.prepare_cached("SELECT wallet_id FROM identities WHERE identity_id = ?1")?; + for identity_id in touched { + let row: Option>> = stmt + .query_row(rusqlite::params![identity_id.as_slice()], |row| row.get(0)) + .optional()?; + let Some(found_wallet_id) = row else { + // Row absent — FK on the child table will reject the + // upcoming write with a clearer error than guessing. + continue; + }; + match (scope_is_sentinel, found_wallet_id) { + (true, None) => {} // sentinel scope matches NULL parenting + (true, Some(found)) => { + let mut found_arr = [0u8; 32]; + if found.len() == 32 { + found_arr.copy_from_slice(&found); + } + return Err(WalletStorageError::WalletIdMismatch { + expected: [0u8; 32], + found: found_arr, + }); + } + (false, None) => { + return Err(WalletStorageError::WalletIdMismatch { + expected: *wallet_id, + found: [0u8; 32], + }); + } + (false, Some(found)) => { + if found.as_slice() != wallet_id.as_slice() { + let mut found_arr = [0u8; 32]; + if found.len() == 32 { + found_arr.copy_from_slice(&found); + } + return Err(WalletStorageError::WalletIdMismatch { + expected: *wallet_id, + found: found_arr, + }); + } + } + } + } + Ok(()) +} diff --git a/packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs b/packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs index fd6bdbc31b..0591d652f6 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs @@ -1,4 +1,15 @@ //! `token_balances` table writer. +//! +//! # Precondition +//! +//! Every `identity_id` in the supplied changeset MUST already exist in +//! the `identities` table and belong to the flush's `wallet_id` (or +//! have a NULL `identities.wallet_id` when the scope is the all-zero +//! sentinel). The writer relies on +//! [`super::identities::apply`] for parenting; the FK to +//! `identities(identity_id)` enforces existence but not the wallet +//! match. A `debug_assert!` below catches mis-attributed callers in +//! development builds at no production cost. use rusqlite::{params, Transaction}; @@ -8,24 +19,46 @@ use platform_wallet::wallet::platform_wallet::WalletId; use crate::sqlite::error::WalletStorageError; use crate::sqlite::util::safe_cast; +/// `token_balances` is keyed by `(identity_id, token_id)`. The caller +/// supplies a [`WalletId`] for symmetry with sibling writers and to +/// feed the precondition debug-assert; it does not feed any column, +/// because cascade flows +/// `wallet_metadata → identities → token_balances` through the +/// nullable `identities.wallet_id` FK. +// +// Orphan-row policy: there is no automatic prune API. Cascade flows +// through `identities`; hosts that delete identities out-of-band must +// prune `token_balances` themselves. pub fn apply( tx: &Transaction<'_>, wallet_id: &WalletId, cs: &TokenBalanceChangeSet, ) -> Result<(), WalletStorageError> { + if cfg!(debug_assertions) { + let touched: std::collections::BTreeSet = cs + .balances + .keys() + .map(|(identity_id, _)| *identity_id) + .chain( + cs.removed_balances + .iter() + .map(|(identity_id, _)| *identity_id), + ) + .collect(); + super::assert_identities_belong_to_wallet(tx, wallet_id, &touched)?; + } if !cs.balances.is_empty() { let now = chrono::Utc::now().timestamp(); let mut stmt = tx.prepare_cached( "INSERT INTO token_balances \ - (wallet_id, identity_id, token_id, balance, updated_at) \ - VALUES (?1, ?2, ?3, ?4, ?5) \ - ON CONFLICT(wallet_id, identity_id, token_id) DO UPDATE SET \ + (identity_id, token_id, balance, updated_at) \ + VALUES (?1, ?2, ?3, ?4) \ + ON CONFLICT(identity_id, token_id) DO UPDATE SET \ balance = excluded.balance, \ updated_at = excluded.updated_at", )?; for ((identity_id, token_id), balance) in &cs.balances { stmt.execute(params![ - wallet_id.as_slice(), identity_id.as_slice(), token_id.as_slice(), safe_cast::u64_to_i64("token_balances.balance", *balance)?, @@ -35,15 +68,10 @@ pub fn apply( } if !cs.removed_balances.is_empty() { let mut stmt = tx.prepare_cached( - "DELETE FROM token_balances \ - WHERE wallet_id = ?1 AND identity_id = ?2 AND token_id = ?3", + "DELETE FROM token_balances WHERE identity_id = ?1 AND token_id = ?2", )?; for (identity_id, token_id) in &cs.removed_balances { - stmt.execute(params![ - wallet_id.as_slice(), - identity_id.as_slice(), - token_id.as_slice() - ])?; + stmt.execute(params![identity_id.as_slice(), token_id.as_slice()])?; } } Ok(()) diff --git a/packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs b/packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs index 1299d748f6..b64525586d 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs @@ -24,15 +24,23 @@ pub fn apply_secure_permissions(path: &Path) -> Result<(), WalletStorageError> { // committed pages live in -wal / -shm. Without this // sweep, the sidecars stay at the process umask default — a // local-user info leak on multi-user hosts. + // + // CODE-011: build the sibling path via `OsString::push` so + // non-UTF-8 path bytes survive intact (no `to_string_lossy` + // corruption). `set_permissions` runs unconditionally — a + // missing sibling returns `ErrorKind::NotFound`, which we treat + // as a silent no-op (closes the `exists()` TOCTOU gate). + let Some(file_name) = path.file_name() else { + return Ok(()); + }; for ext in ["-wal", "-shm"] { - let sibling = path.with_file_name(format!( - "{}{ext}", - path.file_name() - .map(|s| s.to_string_lossy().to_string()) - .unwrap_or_default() - )); - if sibling.exists() { - std::fs::set_permissions(&sibling, perms.clone())?; + let mut sibling_name = file_name.to_os_string(); + sibling_name.push(ext); + let sibling = path.with_file_name(sibling_name); + match std::fs::set_permissions(&sibling, perms.clone()) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(WalletStorageError::Io(e)), } } } diff --git a/packages/rs-platform-wallet-storage/tests/common/mod.rs b/packages/rs-platform-wallet-storage/tests/common/mod.rs index 387b7803c7..a9ed9f0ea5 100644 --- a/packages/rs-platform-wallet-storage/tests/common/mod.rs +++ b/packages/rs-platform-wallet-storage/tests/common/mod.rs @@ -55,6 +55,29 @@ pub fn ensure_wallet_meta(persister: &SqlitePersister, wallet_id: &WalletId) { .expect("ensure wallet_metadata"); } +/// Insert a stub `identities` row so identity-owned table writes +/// (`token_balances`, `dashpay_profiles`, `identity_keys`) pass the +/// FK to `identities(identity_id)`. `parent_wallet_id` is +/// optional — when `Some`, the row is linked to that wallet so the +/// cascade chain works; when `None`, the row is an orphan identity +/// (NULL `wallet_id`), still satisfying the identity-owned FKs. +pub fn ensure_identity( + persister: &SqlitePersister, + identity_id: &[u8; 32], + parent_wallet_id: Option<&WalletId>, +) { + use rusqlite::params; + let conn = persister.lock_conn_for_test(); + let wid_param: Option<&[u8]> = parent_wallet_id.map(|w| w.as_slice()); + conn.execute( + "INSERT OR IGNORE INTO identities \ + (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ + VALUES (?1, ?2, NULL, X'00', 0)", + params![&identity_id[..], wid_param], + ) + .expect("ensure identity"); +} + /// Echo a simple `store` + `flush` of an arbitrary changeset. pub fn store_and_flush( persister: &SqlitePersister, diff --git a/packages/rs-platform-wallet-storage/tests/feature_flag_build.rs b/packages/rs-platform-wallet-storage/tests/feature_flag_build.rs new file mode 100644 index 0000000000..de7b310f24 --- /dev/null +++ b/packages/rs-platform-wallet-storage/tests/feature_flag_build.rs @@ -0,0 +1,53 @@ +//! TC-CODE-020-1 — feature-flag boundary check. +//! +//! Asserts that without the `sqlite` feature the crate compiles with a +//! minimal cross-cutting surface only (`WalletStorageError` is NOT +//! reachable; `SqlitePersister` is NOT reachable). The dev-deps for +//! this crate enable `sqlite`, so this file's purpose is to surface a +//! regression at *review time* by reading the manifest and the lib.rs +//! gating layout — it does NOT re-build the crate. +//! +//! The bare-build invariant itself is enforced by `cargo build +//! -p platform-wallet-storage --no-default-features` in CI; this test +//! pins the source-level expectations so the gate stays meaningful. + +#[test] +fn tc_code_020_1_sqlite_items_are_feature_gated() { + let lib_src = include_str!("../src/lib.rs"); + assert!( + lib_src.contains(r#"#[cfg(feature = "sqlite")]"#), + "lib.rs MUST gate sqlite re-exports behind the `sqlite` feature" + ); + assert!( + lib_src.contains( + r#"#[cfg(feature = "sqlite")] +pub mod sqlite;"# + ), + "the `sqlite` module declaration MUST be cfg-gated" + ); +} + +#[test] +fn tc_code_020_1_wallet_and_serde_deps_are_optional() { + let manifest = include_str!("../Cargo.toml"); + // Both must be tagged optional so a bare build does NOT pull + // platform-wallet (which transitively brings dpp / drive / dashcore) + // or serde. + assert!( + manifest.contains("platform-wallet = { path = \"../rs-platform-wallet\", features = [\n \"serde\",\n], optional = true }"), + "platform-wallet MUST be optional (gated by the `sqlite` feature)" + ); + assert!( + manifest.contains("serde = { version = \"1\", features = [\"derive\"], optional = true }"), + "serde MUST be optional (gated by the `sqlite` feature)" + ); + // The sqlite feature MUST pull both back in. + assert!( + manifest.contains("\"dep:platform-wallet\""), + "the sqlite feature MUST activate dep:platform-wallet" + ); + assert!( + manifest.contains("\"dep:serde\""), + "the sqlite feature MUST activate dep:serde" + ); +} diff --git a/packages/rs-platform-wallet-storage/tests/persistence_error_kind_mapping.rs b/packages/rs-platform-wallet-storage/tests/persistence_error_kind_mapping.rs new file mode 100644 index 0000000000..b5a0745baf --- /dev/null +++ b/packages/rs-platform-wallet-storage/tests/persistence_error_kind_mapping.rs @@ -0,0 +1,379 @@ +//! `WalletStorageError -> PersistenceError` kind-classification table +//! (CODE-004). +//! +//! TC-CODE-004-b — every `WalletStorageError` variant carries through +//! the boundary with the right `PersistenceErrorKind` (`Transient` / +//! `Fatal` / `Constraint`). The `From` impl in +//! `sqlite/error.rs` is the single source of truth; this test pins +//! the mapping so changes to it are deliberate. +//! +//! TC-CODE-004-e — `WalletStorageError::is_transient()` and +//! `WalletStorageError::error_kind_str()` must remain wildcard-free so +//! adding a new variant forces an explicit classification update. This +//! test parses the source file and refuses to compile around a `_ =>` +//! arm in either method. + +use std::path::PathBuf; + +use platform_wallet::changeset::{PersistenceError, PersistenceErrorKind}; +use platform_wallet_storage::sqlite::error::{AutoBackupOperation, WalletStorageError}; +use platform_wallet_storage::sqlite::util::safe_cast::SafeCastTarget; +use rusqlite::ErrorCode; + +/// Classify a converted `PersistenceError` to its `PersistenceErrorKind`. +/// Panics if the converted error is `LockPoisoned`, which is its own +/// trait-level variant rather than a `Backend { kind, .. }`. +fn kind_of(err: WalletStorageError) -> PersistenceErrorKind { + match PersistenceError::from(err) { + PersistenceError::Backend { kind, .. } => kind, + PersistenceError::LockPoisoned => { + panic!("LockPoisoned has no Backend.kind — test was given LockPoisoned by accident") + } + } +} + +fn sqlite_failure(code: ErrorCode, extended: i32) -> WalletStorageError { + WalletStorageError::Sqlite(rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error { + code, + extended_code: extended, + }, + Some("simulated".into()), + )) +} + +/// TC-CODE-004-b — `LockPoisoned` keeps its dedicated variant. +#[test] +fn tc_code_004_b_lock_poisoned_maps_to_lock_poisoned() { + let pe: PersistenceError = WalletStorageError::LockPoisoned.into(); + assert!(matches!(pe, PersistenceError::LockPoisoned)); +} + +/// TC-CODE-004-b — every `is_transient() == true` variant maps to +/// `PersistenceErrorKind::Transient` at the trait boundary. +#[test] +fn tc_code_004_b_transient_variants_map_to_transient_kind() { + let transient_cases = [ + ("DatabaseBusy", sqlite_failure(ErrorCode::DatabaseBusy, 5)), + ( + "DatabaseLocked", + sqlite_failure(ErrorCode::DatabaseLocked, 6), + ), + ("DiskFull", sqlite_failure(ErrorCode::DiskFull, 13)), + ( + "SystemIoFailure", + sqlite_failure(ErrorCode::SystemIoFailure, 10), + ), + ("OutOfMemory", sqlite_failure(ErrorCode::OutOfMemory, 7)), + ( + "FlushRetryable", + WalletStorageError::FlushRetryable { + wallet_id: [0xAB; 32], + source: rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error { + code: ErrorCode::DatabaseBusy, + extended_code: 5, + }, + Some("busy".into()), + ), + }, + ), + ]; + + for (label, err) in transient_cases { + assert!( + err.is_transient(), + "{label}: WalletStorageError::is_transient must be true" + ); + assert_eq!( + kind_of(err), + PersistenceErrorKind::Transient, + "{label}: trait-boundary kind must be Transient" + ); + } +} + +/// TC-CODE-004-b — SQLite constraint failures map to +/// `PersistenceErrorKind::Constraint` so consumers can distinguish +/// "your data is wrong" from "the storage engine is unhappy". +#[test] +fn tc_code_004_b_constraint_variants_map_to_constraint_kind() { + let constraint_codes = [ + ErrorCode::ConstraintViolation, + // Specific constraint sub-codes covered by SQLite's extended + // error codes — checked via the outer ErrorCode group. + ]; + for code in constraint_codes { + let err = sqlite_failure(code, 19); + assert!( + !err.is_transient(), + "constraint must not be transient ({code:?})" + ); + assert_eq!( + kind_of(err), + PersistenceErrorKind::Constraint, + "{code:?} must map to Constraint" + ); + } +} + +/// TC-CODE-004-b — every remaining fatal-but-not-constraint variant +/// maps to `Fatal`. Spot-check enough variants to lock the table; the +/// exhaustiveness is guarded by the wildcard-free invariant test. +#[test] +fn tc_code_004_b_fatal_variants_map_to_fatal_kind() { + let fatal_cases: Vec<(&str, WalletStorageError)> = vec![ + ("Io", WalletStorageError::Io(std::io::Error::other("io"))), + ( + "Sqlite-other", + WalletStorageError::Sqlite(rusqlite::Error::InvalidColumnIndex(0)), + ), + ( + "IntegrityCheckFailed", + WalletStorageError::IntegrityCheckFailed { + report: "bad".into(), + }, + ), + ( + "SchemaHistoryMissing", + WalletStorageError::SchemaHistoryMissing, + ), + ( + "SchemaVersionUnsupported", + WalletStorageError::SchemaVersionUnsupported { + found: 9, + max_supported: 2, + }, + ), + ( + "AutoBackupDisabled", + WalletStorageError::AutoBackupDisabled { + operation: AutoBackupOperation::DeleteWallet, + }, + ), + ( + "AutoBackupDirUnwritable", + WalletStorageError::AutoBackupDirUnwritable { + dir: PathBuf::from("/nope"), + source: std::io::Error::other("io"), + }, + ), + ( + "WalletNotFound", + WalletStorageError::WalletNotFound { + wallet_id: [0xCD; 32], + }, + ), + ( + "WalletIdMismatch", + WalletStorageError::WalletIdMismatch { + expected: [0xAA; 32], + found: [0xBB; 32], + }, + ), + ( + "RestoreDestinationLocked", + WalletStorageError::RestoreDestinationLocked, + ), + ( + "InvalidWalletIdLength", + WalletStorageError::InvalidWalletIdLength { actual: 12 }, + ), + ( + "ConfigInvalid", + WalletStorageError::ConfigInvalid { + reason: "synchronous=Off", + }, + ), + ( + "BlobDecode", + WalletStorageError::BlobDecode { reason: "len" }, + ), + ( + "ForeignKeysNotEnforced", + WalletStorageError::ForeignKeysNotEnforced, + ), + ( + "IdentityKeyEntryMismatch", + WalletStorageError::IdentityKeyEntryMismatch, + ), + ( + "BlobTooLarge", + WalletStorageError::BlobTooLarge { + len_bytes: 1, + limit_bytes: 0, + }, + ), + ( + "IntegerOverflow", + WalletStorageError::IntegerOverflow { + field: "x", + value: 1, + target: SafeCastTarget::I64, + }, + ), + ( + "BackupDestinationExists", + WalletStorageError::BackupDestinationExists { + path: PathBuf::from("/tmp/x"), + }, + ), + ]; + + for (label, err) in fatal_cases { + assert!(!err.is_transient(), "{label}: must not be transient"); + assert_eq!( + kind_of(err), + PersistenceErrorKind::Fatal, + "{label}: trait-boundary kind must be Fatal" + ); + } +} + +/// TC-CODE-004-b — the boxed source preserves the typed `Error` +/// chain so consumers can walk it (the trait was the right boundary +/// for `Box` precisely so the rusqlite +/// source is recoverable). The outer `Display` carries the variant +/// marker ops grep for; `.source()` walks to the inner `rusqlite` +/// payload. +#[test] +fn tc_code_004_b_source_preserves_inner_display_chain() { + let err = WalletStorageError::FlushRetryable { + wallet_id: [0xAB; 32], + source: rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error { + code: ErrorCode::DatabaseBusy, + extended_code: 5, + }, + Some("database is locked".into()), + ), + }; + let pe: PersistenceError = err.into(); + match pe { + PersistenceError::Backend { kind, source } => { + assert_eq!(kind, PersistenceErrorKind::Transient); + let outer = source.to_string(); + assert!(outer.contains("FlushRetryable"), "marker missing: {outer}"); + assert!( + outer.contains("flush failed transiently"), + "body missing: {outer}" + ); + assert!(outer.contains("abab"), "wallet_id hex missing: {outer}"); + + // Walk the typed source chain — that's the whole point of + // boxing the typed error rather than stringifying it. + let mut chain_text = String::new(); + let mut cur: Option<&(dyn std::error::Error + 'static)> = source.source(); + while let Some(e) = cur { + chain_text.push_str(&e.to_string()); + chain_text.push('\n'); + cur = e.source(); + } + assert!( + chain_text.contains("database is locked"), + "inner source text missing from chain walk: {chain_text}" + ); + } + other => panic!("expected Backend {{ .. }}, got {other:?}"), + } +} + +/// TC-CODE-004-e — `is_transient()` source must not regress to a +/// wildcard arm on its outer `match self`. The inner match on +/// `rusqlite::ErrorCode` is allowed to use a wildcard since +/// `ErrorCode` is `#[non_exhaustive]` upstream — we only guard the +/// outer `WalletStorageError` match. +#[test] +fn tc_code_004_e_is_transient_outer_match_is_wildcard_free() { + let src = include_str!("../src/sqlite/error.rs"); + let outer = extract_outer_match_self_body(src, "pub fn is_transient(&self) -> bool") + .expect("is_transient outer match body must be present"); + assert_no_wildcard(&outer, "is_transient"); +} + +/// TC-CODE-004-e — same guard for `error_kind_str()`. The outer match +/// over `WalletStorageError` MUST remain wildcard-free; the inner +/// match over `ErrorCode` may have its own wildcard. +#[test] +fn tc_code_004_e_error_kind_str_is_wildcard_free() { + let src = include_str!("../src/sqlite/error.rs"); + let outer = extract_outer_match_self_body(src, "pub fn error_kind_str(&self) -> &'static str") + .expect("error_kind_str outer match body must be present"); + assert_no_wildcard(&outer, "error_kind_str"); +} + +/// Locate the first `match self {` block inside `fn` `signature` and +/// return only its top-level body (arms at brace-depth = 0 relative +/// to the outer match). Nested matches and tuple patterns at deeper +/// depths are excluded so an inner `_ =>` on `ErrorCode` (which is +/// upstream-`#[non_exhaustive]`) doesn't trip the invariant. +fn extract_outer_match_self_body(src: &str, signature: &str) -> Option { + let start = src.find(signature)?; + let after_sig = &src[start..]; + // Find the `match self {` opening *after* the signature. The + // intervening braces from the fn body are handled by depth + // counting below. + let match_kw = after_sig.find("match self")?; + let open = after_sig[match_kw..].find('{')? + match_kw; + let bytes = after_sig.as_bytes(); + let mut depth = 0usize; + let mut top_level_arms = String::new(); + let mut i = open; + while i < bytes.len() { + let b = bytes[i]; + if b == b'{' { + depth += 1; + if depth > 1 { + // Skip past the nested block. + let close = find_matching_close(bytes, i)?; + i = close + 1; + depth -= 1; + continue; + } + } else if b == b'}' { + if depth == 1 { + return Some(top_level_arms); + } + depth -= 1; + } else if depth == 1 { + top_level_arms.push(b as char); + } + i += 1; + } + None +} + +fn find_matching_close(bytes: &[u8], open_idx: usize) -> Option { + debug_assert_eq!(bytes[open_idx], b'{'); + let mut depth = 0usize; + for (j, &b) in bytes.iter().enumerate().skip(open_idx) { + match b { + b'{' => depth += 1, + b'}' => { + depth -= 1; + if depth == 0 { + return Some(j); + } + } + _ => {} + } + } + None +} + +fn assert_no_wildcard(outer_body: &str, fn_name: &str) { + for line in outer_body.lines() { + let t = line.trim_start(); + // A wildcard arm is either bare `_` or `_ if guard` at the + // start of an arm. Catch the canonical forms operators write. + let is_wildcard_arm = t.starts_with("_ =>") + || t.starts_with("_=>") + || t.starts_with("_ if ") + || t == "_," + || t.starts_with("_ |"); + assert!( + !is_wildcard_arm, + "{fn_name}: outer Self match must remain wildcard-free; offending arm: `{line}`" + ); + } +} diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs b/packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs index 6194abec5d..bf434a9f24 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs @@ -236,3 +236,186 @@ fn atom_011_prune_report_carries_failed_removals_field() { }; assert_eq!(report.failed_removals.len(), 1); } + +/// Detect whether the current process can bypass directory permission +/// checks (i.e. is effectively root) by setting a probe dir to `0o500` +/// and trying to create a file inside it. Returns `true` when the +/// write succeeds — only root sees that. +#[cfg(unix)] +fn is_root_via_probe() -> bool { + use std::os::unix::fs::PermissionsExt; + let Ok(tmp) = tempfile::tempdir() else { + return false; + }; + let dir = tmp.path().join("probe"); + if fs::create_dir(&dir).is_err() { + return false; + } + if fs::set_permissions(&dir, fs::Permissions::from_mode(0o500)).is_err() { + return false; + } + let can_write = fs::write(dir.join("x"), b"x").is_ok(); + let _ = fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)); + can_write +} + +/// TC-CODE-009-a: `backup_to(existing-file)` refuses to overwrite and +/// leaves the sentinel content intact. With `persist_noclobber` the +/// check is atomic against the rename — no TOCTOU window between an +/// `exists()` probe and the atomic swap. +#[test] +fn tc_code_009_a_backup_to_refuses_overwrite_atomically() { + let (persister, tmp, _path) = fresh_persister(); + seed_one_row(&persister, &wid(0xC9)); + let target = tmp.path().join("sentinel.db"); + let sentinel = b"DO NOT OVERWRITE"; + fs::write(&target, sentinel).unwrap(); + + let err = persister.backup_to(&target); + assert!( + matches!(err, Err(WalletStorageError::BackupDestinationExists { .. })), + "expected BackupDestinationExists, got {err:?}" + ); + let after = fs::read(&target).unwrap(); + assert_eq!( + after, sentinel, + "destination must be untouched after refusal" + ); +} + +/// TC-CODE-009-b: non-`AlreadyExists` persist failures surface as +/// `WalletStorageError::Io` — the variant taxonomy stays narrow. +/// Unix-only: emulated via a read-only parent directory (which UID 0 +/// bypasses, so the test is skipped under root). +#[cfg(unix)] +#[test] +fn tc_code_009_b_backup_to_non_already_exists_maps_to_io() { + // Skip under root — UID 0 bypasses the directory permission check + // we use to force EACCES. Detected via a probe: create a 0o500 dir + // and try to write into it; a non-root user gets EACCES, root + // doesn't. + if is_root_via_probe() { + eprintln!("skip: read-only-dir permission bypassed by root"); + return; + } + use std::os::unix::fs::PermissionsExt; + let (persister, tmp, _path) = fresh_persister(); + seed_one_row(&persister, &wid(0xCA)); + let dest_dir = tmp.path().join("ro"); + fs::create_dir(&dest_dir).unwrap(); + fs::set_permissions(&dest_dir, fs::Permissions::from_mode(0o500)).unwrap(); + let target = dest_dir.join("new.db"); + + let err = persister.backup_to(&target); + // Restore perms so tempdir cleanup works on systems that need + // write access to the parent dir. + let _ = fs::set_permissions(&dest_dir, fs::Permissions::from_mode(0o700)); + + match err { + Err(WalletStorageError::Io(e)) => { + assert_ne!( + e.kind(), + std::io::ErrorKind::AlreadyExists, + "must NOT map AlreadyExists to plain Io" + ); + } + other => panic!("expected Io variant, got {other:?}"), + } +} + +/// TC-CODE-014-a: `run_to` and `restore_from` call `fsync` on the +/// destination's parent directory after the atomic rename. Functional +/// fsync verification is impractical without a crash harness, so the +/// regression check is source-level: confirm `fsync_parent_dir` is +/// invoked in `backup.rs`. +#[test] +fn tc_code_014_a_backup_calls_parent_fsync() { + let src = include_str!("../src/sqlite/backup.rs"); + let calls = src.matches("fsync_parent_dir(").count(); + assert!( + calls >= 3, + "expected at least 3 occurrences of `fsync_parent_dir(` in backup.rs \ + (def + run_to + restore_from), found {calls}" + ); +} + +/// TC-CODE-014-b: `# Atomicity` rustdoc mentions the parent-dir fsync +/// so callers aren't misled about durability guarantees. +#[test] +fn tc_code_014_b_atomicity_doc_mentions_fsync() { + let src = include_str!("../src/sqlite/backup.rs"); + let lower = src.to_lowercase(); + assert!( + lower.contains("fsync") || lower.contains("sync_all"), + "atomicity rustdoc must mention fsync / sync_all on parent dir" + ); +} + +/// TC-CODE-019-a: a failed `remove_file` is counted in BOTH `kept` and +/// `failed_removals`, preserving `kept + removed == total`. +/// +/// Unix-only: emulated by chmodding the prune directory read-only so +/// `unlink` returns `EACCES`. Skipped under root because UID 0 bypasses +/// the directory permission check. +#[cfg(unix)] +#[test] +fn tc_code_019_a_failed_removal_counts_in_kept() { + // Skip under root — UID 0 bypasses the directory permission check + // we use to force EACCES. Detected via a probe: create a 0o500 dir + // and try to write into it; a non-root user gets EACCES, root + // doesn't. + if is_root_via_probe() { + eprintln!("skip: read-only-dir permission bypassed by root"); + return; + } + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("backups"); + fs::create_dir(&dir).unwrap(); + // Five eligible backups, all old enough to be removed by `max_age`. + let day = std::time::Duration::from_secs(86_400); + let now = std::time::SystemTime::now(); + for age in [30u64, 31, 32, 33, 34] { + let name = format!( + "wallet-{}.db", + chrono::Utc::now() + .checked_sub_signed(chrono::Duration::days(age as i64)) + .unwrap() + .format("%Y%m%dT%H%M%SZ") + ); + let path = dir.join(&name); + fs::write(&path, b"x").unwrap(); + let mtime = now - day * age as u32; + let _ = filetime::set_file_mtime(&path, filetime::FileTime::from_system_time(mtime)); + } + // Lock the directory so `unlink` fails on EVERY file. + fs::set_permissions(&dir, fs::Permissions::from_mode(0o500)).unwrap(); + + let (persister, _tmp_p, _path) = fresh_persister(); + let res = persister.prune_backups( + &dir, + RetentionPolicy { + keep_last_n: None, + max_age: Some(day), + }, + ); + // Restore perms so tempdir cleanup works. + let _ = fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)); + + let report = res.expect("prune_backups must return Ok even on partial removal failures"); + assert!( + !report.failed_removals.is_empty(), + "expected at least one failed removal" + ); + let total = report.removed.len() + report.failed_removals.len(); + assert_eq!( + report.kept, total, + "kept ({}) must equal total ({}) when no files were removed", + report.kept, total + ); + assert_eq!( + report.removed.len() + report.kept, + 5, + "kept + removed must equal total eligible (5)" + ); +} diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs b/packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs index f4b83fc68e..101087aa40 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs @@ -298,6 +298,10 @@ fn tc023_one_flush_is_one_transaction() { let (persister, _tmp, _path) = fresh_persister_with_mode(FlushMode::Manual); let w = wid(0x90); ensure_wallet_meta(&persister, &w); + // token_balances FK targets identities(identity_id); seed the + // identity so the cross-area flush passes that constraint. + let owner = Identifier::from([0xA1u8; 32]); + common::ensure_identity(&persister, owner.as_bytes(), Some(&w)); let mut cs = PlatformWalletChangeSet::default(); cs.core = Some(core_with_height(7, 7)); cs.wallet_metadata = Some(WalletMetadataEntry { @@ -305,7 +309,6 @@ fn tc023_one_flush_is_one_transaction() { birth_height: 1, }); let mut balances = BTreeMap::new(); - let owner = Identifier::from([0xA1u8; 32]); let token = Identifier::from([0xA2u8; 32]); balances.insert((owner, token), 9u64); cs.token_balances = Some(TokenBalanceChangeSet { @@ -429,8 +432,8 @@ fn tc_p2_002_transient_failure_restores_buffer() { persister.force_next_flush_to_fail(make_busy_error()); let err = persister.flush(w).expect_err("first flush must fail"); let msg = match err { - PersistenceError::Backend(s) => s, - other => panic!("expected Backend(_), got {other:?}"), + PersistenceError::Backend { source, .. } => source.to_string(), + other => panic!("expected Backend {{ .. }}, got {other:?}"), }; assert!( msg.contains("flush failed transiently"), @@ -508,8 +511,8 @@ fn tc_p2_006_immediate_surfaces_flush_retryable() { .store(w, changeset(core_with_height(3, 3))) .expect_err("immediate store must surface the error"); let msg = match err { - PersistenceError::Backend(s) => s, - other => panic!("expected Backend(_), got {other:?}"), + PersistenceError::Backend { source, .. } => source.to_string(), + other => panic!("expected Backend {{ .. }}, got {other:?}"), }; assert!( msg.contains("flush failed transiently"), diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs b/packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs index d9836f34e3..16b7450933 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs @@ -196,3 +196,38 @@ fn tc059_backup_dir() { assert!(path.ends_with(".db")); assert!(std::path::Path::new(path).exists()); } + +/// TC-CODE-030-1a: the supported `--no-auto-backup` flag disables the +/// pre-migration auto-backup. `migrate --no-auto-backup` succeeds on a +/// fresh DB without writing the `backups/auto/` sentinel snapshot. +#[test] +fn tc_code_030_1a_no_auto_backup_disables() { + let tmp = tempfile::tempdir().unwrap(); + let db = tmp.path().join("w.db"); + let out = cli() + .args(["--db", db.to_str().unwrap(), "migrate", "--no-auto-backup"]) + .output() + .unwrap(); + assert!( + out.status.success(), + "migrate --no-auto-backup failed: {out:?}" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("auto-backup skipped (--no-auto-backup)"), + "expected `--no-auto-backup` notice on stderr, got: {stderr}" + ); + // No `backups/auto/pre-migration-*.db` written when the flag is set. + let auto_dir = tmp.path().join("backups").join("auto"); + if auto_dir.exists() { + let pre_mig: Vec<_> = std::fs::read_dir(&auto_dir) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| e.file_name().to_string_lossy().starts_with("pre-migration")) + .collect(); + assert!( + pre_mig.is_empty(), + "pre-migration backup written despite --no-auto-backup: {pre_mig:?}" + ); + } +} diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rs b/packages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rs index c976b9061a..1da0c1fa96 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rs @@ -1,13 +1,18 @@ #![allow(clippy::field_reassign_with_default)] -//! CMT-001 — `delete_wallet` must reconcile the in-memory buffer. +//! CMT-001 / CODE-006 — `delete_wallet` must reconcile the in-memory +//! buffer AND fold buffered writes into the pre-delete backup. //! -//! `delete_wallet_inner` drains-and-discards the target wallet's -//! buffered changeset before the existence gate, and treats "buffered -//! OR persisted" as existence. These regression tests pin the three -//! historical failure modes: spurious `WalletNotFound` for a -//! buffered-only wallet, a pre-delete backup that excludes buffered -//! writes, and post-delete resurrection on the next flush. +//! `delete_wallet_inner` drains the target wallet's buffered +//! changeset, flushes it to disk, snapshots the backup, then runs +//! the cascade. These regression tests pin the failure modes: a +//! buffered-only wallet must delete cleanly without spurious +//! `WalletNotFound`; the pre-delete backup must contain buffered-but- +//! unflushed rows; a transient pre-flush failure must restore the +//! buffer and abort the delete without producing a backup; a peer +//! mutating the wallet in the post-snapshot / pre-cascade window +//! aborts the cascade with `ConcurrentMutationDuringDelete`; the +//! flush must not resurrect a deleted wallet. mod common; @@ -78,10 +83,12 @@ fn buffered_only_delete_is_ok_and_no_resurrection() { ); } -/// The pre-delete backup excludes drained-and-discarded buffered -/// writes — the backup must not contain the wallet. +/// TC-CODE-006-1 — the pre-delete backup MUST include buffered +/// writes flushed during `delete_wallet`'s pre-flush phase. Without +/// the pre-flush, rollback-from-backup couldn't recover a wallet +/// whose only state lived in the buffer. #[test] -fn pre_delete_backup_excludes_buffered_writes() { +fn pre_delete_backup_includes_buffered_writes() { let tmp = tempfile::tempdir().unwrap(); let path = tmp.path().join("w.db"); let backup_dir = tmp.path().join("backups"); @@ -100,7 +107,7 @@ fn pre_delete_backup_excludes_buffered_writes() { rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY, ) .unwrap(); - let in_backup: Option = backup + let in_backup_core: Option = backup .query_row( "SELECT COUNT(*) FROM core_sync_state WHERE wallet_id = ?1", rusqlite::params![w.as_slice()], @@ -108,10 +115,158 @@ fn pre_delete_backup_excludes_buffered_writes() { ) .optional() .unwrap(); + let in_backup_meta: Option = backup + .query_row( + "SELECT COUNT(*) FROM wallet_metadata WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .optional() + .unwrap(); + assert_eq!( + in_backup_core, + Some(1), + "pre-delete backup must contain the flushed buffered core_sync_state row" + ); + assert_eq!( + in_backup_meta, + Some(1), + "pre-delete backup must contain the flushed buffered wallet_metadata row" + ); +} + +/// TC-CODE-006-2 — when the pre-flush fails, the buffer is restored, +/// no backup is produced, the wallet stays in the live DB, and +/// `delete_wallet` surfaces the original error. +#[test] +fn pre_flush_failure_preserves_buffer_and_skips_backup() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("w.db"); + let backup_dir = tmp.path().join("backups"); + let cfg = SqlitePersisterConfig::new(&path) + .with_flush_mode(FlushMode::Manual) + .with_auto_backup_dir(Some(backup_dir.clone())); + let persister = SqlitePersister::open(cfg).unwrap(); + let w = wid(0xC1); + + // Seed wallet_metadata so the wallet exists in the live DB. + { + let conn = persister.lock_conn_for_test(); + conn.execute( + "INSERT INTO wallet_metadata (wallet_id, network, birth_height) \ + VALUES (?1, 'testnet', 0)", + rusqlite::params![w.as_slice()], + ) + .unwrap(); + } + + // Buffer a changeset so `delete_wallet` enters the pre-flush + // branch, then prime the pre-flush injector to fail. + persister.store(w, full_changeset(11)).unwrap(); + persister.force_next_pre_flush_to_fail(busy_error()); + + let err = persister + .delete_wallet(w) + .expect_err("pre-flush failure must propagate as Err"); + assert!( + matches!(err, WalletStorageError::Sqlite(_)), + "expected Sqlite error from primed pre-flush failure, got {err:?}" + ); + + // Backup dir holds no PreDelete file (dir may not even exist if + // `run_auto_backup` never ran — both are acceptable). + let entries: Vec<_> = std::fs::read_dir(&backup_dir) + .map(|it| it.filter_map(Result::ok).collect()) + .unwrap_or_default(); + assert!( + entries.is_empty(), + "pre-flush failure must not leave a backup behind: {entries:?}" + ); + + // Wallet still in the live DB, buffer still holds the changeset. + let meta_rows: i64 = { + let conn = persister.lock_conn_for_test(); + conn.query_row( + "SELECT COUNT(*) FROM wallet_metadata WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .unwrap() + }; + assert_eq!(meta_rows, 1, "wallet must remain in the live DB"); + assert!( + persister.buffer_has_changeset_for_test(&w), + "buffer must still hold the changeset after a failed pre-flush" + ); +} + +/// TC-CODE-006-3 — a peer that mutates the wallet in the post-snapshot +/// / pre-cascade window aborts `delete_wallet` with the typed +/// `ConcurrentMutationDuringDelete` so the operator can retry after +/// quiescing the peer. The pre-delete backup file is left in place — +/// it captures the pre-mutation state and is still useful for +/// forensics — but the cascade itself does NOT run. +#[test] +fn peer_mutation_between_backup_and_exclusive_aborts_with_typed_error() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("w.db"); + let backup_dir = tmp.path().join("backups"); + let cfg = SqlitePersisterConfig::new(&path) + .with_flush_mode(FlushMode::Manual) + .with_auto_backup_dir(Some(backup_dir)); + let persister = SqlitePersister::open(cfg).unwrap(); + let w = wid(0xC3); + persister.store(w, full_changeset(13)).unwrap(); + + // Arm a hook that fires after the pre-delete backup snapshot and + // before the cascade `BEGIN EXCLUSIVE`. The hook opens a sibling + // raw connection to the same DB and deletes the wallet's metadata + // row — simulating a cross-process peer mutation that the + // rusqlite-Backup-API lock-free window left open. + let peer_path = path.clone(); + persister.arm_post_backup_hook(move || { + let peer = rusqlite::Connection::open(&peer_path).expect("peer open"); + peer.execute("PRAGMA foreign_keys = ON", []).unwrap(); + peer.execute( + "DELETE FROM wallet_metadata WHERE wallet_id = ?1", + rusqlite::params![&[0xC3u8; 32][..]], + ) + .expect("peer delete"); + }); + + let err = persister + .delete_wallet(w) + .expect_err("delete_wallet must abort when a peer races the cascade"); + let observed = match err { + WalletStorageError::ConcurrentMutationDuringDelete { wallet_id } => wallet_id, + other => panic!("expected ConcurrentMutationDuringDelete, got: {other:?}"), + }; + assert_eq!(observed, *w.as_slice(), "wallet_id mismatch in typed error"); + + // The pre-delete backup must still exist so forensics can recover + // the pre-peer-mutation state. + let backups: Vec<_> = std::fs::read_dir(tmp.path().join("backups")) + .expect("backup dir") + .filter_map(|e| e.ok()) + .filter(|e| e.file_name().to_string_lossy().starts_with("pre-delete-")) + .collect(); + assert_eq!(backups.len(), 1, "exactly one pre-delete backup expected"); + let backup_path = backups[0].path(); + let backup = rusqlite::Connection::open_with_flags( + &backup_path, + rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY, + ) + .unwrap(); + let in_backup_meta: i64 = backup + .query_row( + "SELECT COUNT(*) FROM wallet_metadata WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .unwrap(); assert_eq!( - in_backup, - Some(0), - "pre-delete backup must not contain buffered-but-unflushed rows" + in_backup_meta, 1, + "backup must carry the wallet that existed at snapshot time" ); } diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_delete_cross_process_exclusion.rs b/packages/rs-platform-wallet-storage/tests/sqlite_delete_cross_process_exclusion.rs new file mode 100644 index 0000000000..0fa07cd3e2 --- /dev/null +++ b/packages/rs-platform-wallet-storage/tests/sqlite_delete_cross_process_exclusion.rs @@ -0,0 +1,102 @@ +#![allow(clippy::field_reassign_with_default)] + +//! TC-CODE-007 — `delete_wallet` must hold a SQLite-native EXCLUSIVE +//! across the (backup + cascade-delete) window so a peer rusqlite +//! Connection (a different process equivalent) can't commit rows +//! between the backup snapshot and the cascade. + +mod common; + +use common::{ensure_wallet_meta, fresh_persister, wid}; +use rusqlite::TransactionBehavior; + +/// When a peer holds EXCLUSIVE on the destination, `delete_wallet` +/// must block / fail on busy rather than proceeding through an +/// in-process-mutex-only path that ignores the peer. +#[test] +fn delete_wallet_blocks_when_peer_holds_exclusive() { + let (persister, _tmp, db_path) = fresh_persister(); + let w = wid(0x77); + ensure_wallet_meta(&persister, &w); + + let backup_dir = tempfile::tempdir().expect("backup dir"); + // Wire the persister with auto-backup so delete_wallet exercises + // the backup + cascade path (the canonical path under test). + // Re-open persister using a config that knows about the dir. + drop(persister); + let cfg = platform_wallet_storage::SqlitePersisterConfig::new(&db_path) + .with_auto_backup_dir(Some(backup_dir.path().to_path_buf())); + let persister = platform_wallet_storage::SqlitePersister::open(cfg).expect("re-open"); + ensure_wallet_meta(&persister, &w); + + // Peer opens a writer conn against the same DB file and takes + // EXCLUSIVE — represents "another process holds the DB busy". + let mut peer = rusqlite::Connection::open(&db_path).expect("peer open"); + peer.pragma_update(None, "busy_timeout", 50i64).unwrap(); + let tx = peer + .transaction_with_behavior(TransactionBehavior::Exclusive) + .expect("peer EXCLUSIVE"); + + // Set the persister's busy-timeout low so the test doesn't hang. + { + let conn = persister.lock_conn_for_test(); + conn.pragma_update(None, "busy_timeout", 50i64).unwrap(); + } + + let err = persister + .delete_wallet(w) + .expect_err("delete must conflict with peer EXCLUSIVE"); + // Detect the busy/locked condition either through the SqliteFailure + // error code or the source rusqlite error string (which renders + // "database is locked" etc.). Falls back to a substring check on + // the Display form so the assertion is robust across rusqlite + // versions and the exact path that surfaced the conflict (open vs. + // begin vs. cascade). + let busy = matches!( + &err, + platform_wallet_storage::WalletStorageError::Sqlite( + rusqlite::Error::SqliteFailure(e, _) + ) if matches!( + e.code, + rusqlite::ErrorCode::DatabaseBusy | rusqlite::ErrorCode::DatabaseLocked + ) + ); + let dbg = format!("{err:?}"); + assert!( + busy || dbg.contains("Busy") + || dbg.contains("Locked") + || dbg.contains("database is locked"), + "expected busy/locked SQLite error, got: {err} | dbg: {dbg}" + ); + + drop(tx); + drop(peer); +} + +/// Single-process load (regression for CMT-002 / CMT-008 invariants) +/// must still pass after the EXCLUSIVE refactor. +#[test] +fn delete_wallet_single_process_still_works() { + let (persister, _tmp, db_path) = fresh_persister(); + let backup_dir = tempfile::tempdir().expect("backup dir"); + drop(persister); + let cfg = platform_wallet_storage::SqlitePersisterConfig::new(&db_path) + .with_auto_backup_dir(Some(backup_dir.path().to_path_buf())); + let persister = platform_wallet_storage::SqlitePersister::open(cfg).expect("re-open"); + + let w = wid(0x88); + ensure_wallet_meta(&persister, &w); + + let report = persister.delete_wallet(w).expect("delete succeeds"); + assert!(report.backup_path.is_some(), "auto-backup should fire"); + // wallet_metadata row should be gone. + let conn = persister.lock_conn_for_test(); + let row: Option = conn + .query_row( + "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |r| r.get(0), + ) + .ok(); + assert!(row.is_none(), "wallet_metadata row must be gone"); +} diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs b/packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs index f7527cf269..af9df9e7e7 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs @@ -93,7 +93,7 @@ fn delete_wallet_restores_buffer_on_backup_failure() { /// per-wallet table holds zero rows for that wallet_id. #[test] fn concurrent_store_does_not_resurrect_deleted_wallet() { - use platform_wallet_storage::sqlite::schema::PER_WALLET_TABLES; + use platform_wallet_storage::sqlite::schema::{count_rows_for_wallet_sql, PER_WALLET_TABLES}; use std::sync::atomic::{AtomicBool, Ordering}; use std::thread; @@ -136,7 +136,9 @@ fn concurrent_store_does_not_resurrect_deleted_wallet() { // Give the worker a moment to land some racing stores. std::thread::sleep(std::time::Duration::from_millis(20)); - let _ = persister.delete_wallet(w); + persister + .delete_wallet(w) + .expect("delete_wallet should succeed in concurrent-store regression"); stop.store(true, Ordering::Relaxed); worker.join().unwrap(); @@ -146,14 +148,14 @@ fn concurrent_store_does_not_resurrect_deleted_wallet() { let _ = persister.commit_writes(); let conn = persister.lock_conn_for_test(); - for &table in PER_WALLET_TABLES { + for (table, scope) in PER_WALLET_TABLES { let n: i64 = conn .query_row( - &format!("SELECT COUNT(*) FROM {table} WHERE wallet_id = ?1"), + &count_rows_for_wallet_sql(table, *scope), rusqlite::params![w.as_slice()], |row| row.get(0), ) - .unwrap_or(0); + .unwrap_or_else(|e| panic!("COUNT(*) query failed for table `{table}`: {e}")); assert_eq!( n, 0, "table `{table}` still has rows for deleted wallet — concurrent-store race regression" diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs b/packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs index ba145c4a78..51352c95ab 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs @@ -177,6 +177,9 @@ fn samples() -> Vec { Some("busy".into()), ), }, + WalletStorageError::ConcurrentMutationDuringDelete { + wallet_id: [0xCD; 32], + }, ] } @@ -244,6 +247,9 @@ fn tc_p2_005_is_transient_table() { WalletStorageError::BlobTooLarge { .. } => (false, "blob_too_large"), WalletStorageError::ForeignKeysNotEnforced => (false, "foreign_keys_not_enforced"), WalletStorageError::IntegerOverflow { .. } => (false, "integer_overflow"), + WalletStorageError::ConcurrentMutationDuringDelete { .. } => { + (false, "concurrent_mutation_during_delete") + } } } @@ -265,9 +271,10 @@ fn tc_p2_005_is_transient_table() { } /// TC-P2-010: `FlushRetryable` flowing through the `From` impl into -/// `PersistenceError::Backend(String)` carries the markers ops grep -/// for: variant name, hex-encoded wallet id prefix, and the inner -/// rusqlite source text. +/// `PersistenceError::Backend { kind, source }` (CODE-004): the outer +/// `Display` carries the variant markers ops grep for, and the typed +/// source chain still reaches the inner rusqlite payload (consumers +/// downcast or `Error::source`-walk to get there). #[test] fn tc_p2_010_boundary_error_mapping() { let err = WalletStorageError::FlushRetryable { @@ -281,21 +288,36 @@ fn tc_p2_010_boundary_error_mapping() { ), }; let pe: PersistenceError = err.into(); - let s = match pe { - PersistenceError::Backend(s) => s, - other => panic!("expected Backend(_), got {other:?}"), + let source = match pe { + PersistenceError::Backend { source, .. } => source, + other => panic!("expected Backend {{ .. }}, got {other:?}"), }; + let outer = source.to_string(); + assert!( + outer.contains("FlushRetryable"), + "missing FlushRetryable variant marker: {outer}" + ); assert!( - s.contains("FlushRetryable"), - "missing FlushRetryable variant marker: {s}" + outer.contains("flush failed transiently"), + "missing FlushRetryable display body: {outer}" ); assert!( - s.contains("flush failed transiently"), - "missing FlushRetryable display body: {s}" + outer.contains("abab"), + "missing wallet_id hex prefix: {outer}" ); - assert!(s.contains("abab"), "missing wallet_id hex prefix: {s}"); + + // Walk the typed source chain to the inner rusqlite payload — + // post-CODE-004 the source is `Box` so + // the chain is preserved structurally, not just stringified. + let mut chain = String::new(); + let mut cur: Option<&(dyn std::error::Error + 'static)> = source.source(); + while let Some(e) = cur { + chain.push_str(&e.to_string()); + chain.push('\n'); + cur = e.source(); + } assert!( - s.contains("database is locked"), - "missing inner source text: {s}" + chain.contains("database is locked"), + "inner source text missing from chain walk: {chain}" ); } diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_hardening_3625.rs b/packages/rs-platform-wallet-storage/tests/sqlite_hardening_3625.rs index f0afaa1222..851f138241 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_hardening_3625.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_hardening_3625.rs @@ -40,7 +40,8 @@ fn native_fk_rejects_orphan_child() { } /// CMT-001: an `identity_keys` row whose `identities` parent does not -/// exist is rejected by the composite FK to `identities`. +/// exist is rejected by the FK to `identities(identity_id)` (cascade +/// chain `wallet_metadata → identities → identity_keys`). #[test] fn native_fk_rejects_identity_keys_without_identity() { let (persister, _tmp, _path) = fresh_persister(); @@ -49,9 +50,9 @@ fn native_fk_rejects_identity_keys_without_identity() { let conn = persister.lock_conn_for_test(); let res = conn.execute( "INSERT INTO identity_keys \ - (wallet_id, identity_id, key_id, public_key_blob, public_key_hash, derivation_blob) \ - VALUES (?1, ?2, 0, X'00', X'00', NULL)", - params![w.as_slice(), [3u8; 32].as_slice()], + (identity_id, key_id, public_key_blob, public_key_hash, derivation_blob) \ + VALUES (?1, 0, X'00', X'00', NULL)", + params![[3u8; 32].as_slice()], ); let err = res.unwrap_err().to_string(); assert!( diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs b/packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs index d7ece33eb0..538c062b1e 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs @@ -97,11 +97,10 @@ fn tc043_non_wired_up_persisted_but_not_returned() { let recipient = Identifier::from([0x22; 32]); let token = Identifier::from([0x33; 32]); ensure_wallet_meta(&persister, &w); - // Identity row required for the contacts/dashpay FK triggers if - // any are wired into contacts_*; the contacts_* tables themselves - // only check the wallet_metadata parent today, so we don't need - // an identity row for this test — but we'd add one here if the - // trigger set grew. + // token_balances FK targets identities(identity_id), so the owner + // identity must exist before any token-balance row is written. + // contacts_* is wallet-scoped, so it doesn't need an identity row. + common::ensure_identity(&persister, owner.as_bytes(), Some(&w)); let mut sent_requests = std::collections::BTreeMap::new(); sent_requests.insert( SentContactRequestKey { @@ -163,8 +162,8 @@ fn tc043_non_wired_up_persisted_but_not_returned() { assert_eq!(sent, 1, "contacts_sent row missing after reopen"); let tokens: i64 = conn .query_row( - "SELECT COUNT(*) FROM token_balances WHERE wallet_id = ?1 AND identity_id = ?2 AND token_id = ?3", - rusqlite::params![w.as_slice(), owner.as_slice(), token.as_slice()], + "SELECT COUNT(*) FROM token_balances WHERE identity_id = ?1 AND token_id = ?2", + rusqlite::params![owner.as_slice(), token.as_slice()], |row| row.get(0), ) .unwrap(); diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs b/packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs index ac7544db71..b8da62f5ff 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs @@ -118,8 +118,10 @@ fn tc027_smoke_insert_every_table() { ), ( "identity_keys", - "INSERT INTO identity_keys (wallet_id, identity_id, key_id, public_key_blob, public_key_hash, derivation_blob) VALUES (?1, ?2, 0, X'00', X'00', NULL)", - &[&wallet_id.as_slice(), &identity_id.as_slice()], + // identity_keys is keyed by (identity_id, key_id); the FK + // targets identities(identity_id). + "INSERT INTO identity_keys (identity_id, key_id, public_key_blob, public_key_hash, derivation_blob) VALUES (?1, 0, X'00', X'00', NULL)", + &[&identity_id.as_slice()], ), ( "contacts_sent", @@ -153,25 +155,37 @@ fn tc027_smoke_insert_every_table() { ), ( "token_balances", - "INSERT INTO token_balances (wallet_id, identity_id, token_id, balance, updated_at) VALUES (?1, ?2, ?3, 0, 0)", - &[&wallet_id.as_slice(), &identity_id.as_slice(), &[5u8; 32].as_slice()], + // token_balances PK is (identity_id, token_id); the FK + // cascades through identities. + "INSERT INTO token_balances (identity_id, token_id, balance, updated_at) VALUES (?1, ?2, 0, 0)", + &[&identity_id.as_slice(), &[5u8; 32].as_slice()], ), ( "dashpay_profiles", - "INSERT INTO dashpay_profiles (wallet_id, identity_id, profile_blob) VALUES (?1, ?2, X'00')", - &[&wallet_id.as_slice(), &identity_id.as_slice()], + // dashpay_profiles is keyed by identity_id only. + "INSERT INTO dashpay_profiles (identity_id, profile_blob) VALUES (?1, X'00')", + &[&identity_id.as_slice()], ), ( "dashpay_payments_overlay", - "INSERT INTO dashpay_payments_overlay (wallet_id, identity_id, payment_id, overlay_blob) VALUES (?1, ?2, 'pay1', X'00')", - &[&wallet_id.as_slice(), &identity_id.as_slice()], + // dashpay_payments_overlay is keyed by (identity_id, payment_id). + "INSERT INTO dashpay_payments_overlay (identity_id, payment_id, overlay_blob) VALUES (?1, 'pay1', X'00')", + &[&identity_id.as_slice()], ), ]; + use platform_wallet_storage::sqlite::schema::{count_rows_for_wallet_sql, PER_WALLET_TABLES}; + let scope_for = |name: &str| { + PER_WALLET_TABLES + .iter() + .find(|(t, _)| *t == name) + .map(|(_, s)| *s) + .expect("table is in PER_WALLET_TABLES") + }; for (table, sql, params) in cases { conn.execute(sql, *params).expect(table); let n: i64 = conn .query_row( - &format!("SELECT COUNT(*) FROM {table} WHERE wallet_id = ?1"), + &count_rows_for_wallet_sql(table, scope_for(table)), rusqlite::params![wallet_id.as_slice()], |row| row.get(0), ) diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs b/packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs index 8ff75f7f67..32fd30e91b 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs @@ -25,7 +25,10 @@ fn corrupt_btree_pages(path: &std::path::Path) { .open(path) .expect("open db for corruption"); let len = f.metadata().unwrap().len(); - assert!(len > 4096, "expected at least one full page"); + assert!( + len >= 8192, + "need at least two full pages to corrupt page 2; got {len} bytes" + ); // Read page 2 (bytes 4096..8192), flip every other byte, write back. f.seek(SeekFrom::Start(4096)).unwrap(); let mut buf = vec![0u8; 4096]; @@ -72,3 +75,76 @@ fn atom_013_open_rejects_corrupt_db() { ); drop(tmp); } + +/// Flip bytes inside pages 2 AND 3 so SQLite's `PRAGMA integrity_check` +/// has multiple problems to report. This widens the surface beyond the +/// single-row "ok" case so the multi-row collection path is exercised. +fn corrupt_multiple_pages(path: &std::path::Path) { + let mut f = OpenOptions::new() + .read(true) + .write(true) + .open(path) + .expect("open db for multi-page corruption"); + let len = f.metadata().unwrap().len(); + assert!(len > 8192, "expected at least two full pages"); + for page_start in [4096u64, 8192] { + f.seek(SeekFrom::Start(page_start)).unwrap(); + let mut buf = vec![0u8; 4096]; + f.read_exact(&mut buf).unwrap(); + for b in buf.iter_mut().step_by(2) { + *b ^= 0xFF; + } + f.seek(SeekFrom::Start(page_start)).unwrap(); + f.write_all(&buf).unwrap(); + } + f.sync_all().unwrap(); +} + +/// TC-CODE-016-a: a multi-problem DB surfaces every diagnostic line in +/// `IntegrityCheckFailed::report`. Pre-fix the helper used `query_row` +/// and silently dropped every row past the first. We assert the report +/// is non-empty and not the truncated single-row "ok" sentinel; we +/// don't bind to a fixed line count because SQLite's exact diagnostic +/// shape isn't stable across builds. +#[test] +fn tc_code_016_a_integrity_report_collects_all_rows() { + let (persister, tmp, path) = fresh_persister(); + { + use rusqlite::params; + let conn = persister.lock_conn_for_test(); + for i in 0..40u32 { + conn.execute( + "INSERT INTO wallet_metadata (wallet_id, network, birth_height) VALUES (?1, 'testnet', ?2)", + params![vec![i as u8; 32].as_slice(), i as i64], + ) + .unwrap(); + } + } + drop(persister); + + corrupt_multiple_pages(&path); + + let cfg = SqlitePersisterConfig::new(&path); + let err = match SqlitePersister::open(cfg) { + Ok(_) => panic!("open must reject multi-page corrupt DB"), + Err(e) => e, + }; + let report = match err { + WalletStorageError::IntegrityCheckFailed { report } => report, + other => panic!("expected IntegrityCheckFailed, got {other:?}"), + }; + assert!(!report.is_empty(), "report must be non-empty"); + assert_ne!( + report.trim(), + "ok", + "report must NOT be the healthy sentinel" + ); + // Source-level regression: the helper must use `query_map`, not + // `query_row`, so multi-row reports are preserved. + let helper_src = include_str!("../src/sqlite/backup.rs"); + assert!( + helper_src.contains("PRAGMA integrity_check") && helper_src.contains("query_map"), + "run_integrity_check must use query_map (not query_row) to collect every diagnostic row" + ); + drop(tmp); +} diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_permissions.rs b/packages/rs-platform-wallet-storage/tests/sqlite_permissions.rs index f4dcb8f1fc..d010ee94f0 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_permissions.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_permissions.rs @@ -8,6 +8,8 @@ mod common; +use std::ffi::OsString; +use std::os::unix::ffi::OsStringExt; use std::os::unix::fs::PermissionsExt; use common::{ensure_wallet_meta, wid}; @@ -59,3 +61,98 @@ fn wal_and_shm_sidecars_are_chmodded_0o600() { ); } } + +/// TC-CODE-011-a: `apply_secure_permissions` survives a non-ASCII DB +/// filename whose bytes round-trip through `OsString` (the codepath +/// builds sidecar names via `OsString::push`, not `format!` over a +/// lossy `String`). The chosen prefix `ÿþ` (`U+00FF U+00FE`, UTF-8 +/// bytes `c3 bf c3 be`) is multi-byte non-ASCII that both Linux and +/// macOS APFS accept — APFS rejects raw non-UTF-8 with `EILSEQ`, so +/// the bytes here are deliberately valid UTF-8 while still exercising +/// the `OsString`-end-to-end path the pre-fix `to_string_lossy()` would +/// have mangled into the wrong sibling names. +#[test] +fn tc_code_011_a_non_ascii_db_path_sidecars_chmodded() { + let tmp = tempfile::tempdir().unwrap(); + // Valid-UTF-8 multi-byte prefix `ÿþ` + `.db` / `.db-wal` / `.db-shm`. + // We still go through `OsString::from_vec` to mirror the production + // codepath's `OsStr`/`OsString` API surface end-to-end. + let prefix: &[u8] = &[0xC3, 0xBF, 0xC3, 0xBE]; // "ÿþ" in UTF-8 + debug_assert_eq!(std::str::from_utf8(prefix).unwrap(), "ÿþ"); + let mk = |suffix: &[u8]| -> OsString { + let mut v = prefix.to_vec(); + v.extend_from_slice(suffix); + OsString::from_vec(v) + }; + let db_name = mk(b".db"); + let wal_name = mk(b".db-wal"); + let shm_name = mk(b".db-shm"); + let db_path = tmp.path().join(&db_name); + let wal = tmp.path().join(&wal_name); + let shm = tmp.path().join(&shm_name); + // Plant the trio with permissive perms so the chmod is observable. + for p in [&db_path, &wal, &shm] { + std::fs::write(p, b"x").unwrap(); + std::fs::set_permissions(p, std::fs::Permissions::from_mode(0o666)).unwrap(); + } + + platform_wallet_storage::sqlite::util::permissions::apply_secure_permissions(&db_path) + .expect("apply_secure_permissions"); + + for p in [&db_path, &wal, &shm] { + let mode = std::fs::metadata(p).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, + 0o600, + "expected 0o600 on non-ASCII path {} after apply_secure_permissions, got {:o}", + p.display(), + mode + ); + } +} + +/// TC-CODE-011-b: `apply_secure_permissions` is a no-op (Ok) when the +/// sidecars don't exist. The `set_permissions` call sees +/// `ErrorKind::NotFound` and swallows it — no `exists()` gate, no +/// race window. +#[test] +fn tc_code_011_b_no_sidecars_is_ok() { + let tmp = tempfile::tempdir().unwrap(); + let db_path = tmp.path().join("solo.db"); + std::fs::write(&db_path, b"x").unwrap(); + // No -wal / -shm planted on purpose. + platform_wallet_storage::sqlite::util::permissions::apply_secure_permissions(&db_path) + .expect("apply_secure_permissions on solo DB must be Ok"); + let mode = std::fs::metadata(&db_path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + // Source-level regression: the helper must NOT contain `exists(` + // anywhere in its sibling-chmod path. + let src = include_str!("../src/sqlite/util/permissions.rs"); + assert!( + !src.contains("sibling.exists("), + "permissions.rs must not pre-gate set_permissions on sibling.exists() (TOCTOU)" + ); + assert!( + !src.contains(".to_string_lossy().to_string()"), + "permissions.rs must not build sibling paths via .to_string_lossy().to_string() (loses non-UTF-8 bytes)" + ); +} + +/// TC-CODE-011-c: the same OsString + NotFound-swallow pattern in +/// `backup.rs`'s WAL/SHM-unlink loop (DRY motif). +#[test] +fn tc_code_011_c_backup_wal_shm_unlink_no_lossy_no_exists_gate() { + let src = include_str!("../src/sqlite/backup.rs"); + // The unlink loop now uses OsString::push, not to_string_lossy. + // We can't structurally diff the loop, but the file must not + // contain the lossy pattern on the sidecar build path. + assert!( + !src.contains("s.to_string_lossy().to_string()"), + "backup.rs must not build sibling paths via to_string_lossy().to_string()" + ); + // And remove_file must not be gated on sibling.exists(). + assert!( + !src.contains("sibling.exists()"), + "backup.rs WAL/SHM-unlink must not pre-gate remove_file on sibling.exists() (TOCTOU)" + ); +} diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs b/packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs index 9684a62d0d..680f7a9f59 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs @@ -21,7 +21,7 @@ use platform_wallet::changeset::{ CoreChangeSet, PlatformWalletChangeSet, PlatformWalletPersistence, WalletMetadataEntry, }; use platform_wallet_storage::{ - SqlitePersister, SqlitePersisterConfig, Synchronous, WalletStorageError, + JournalMode, SqlitePersister, SqlitePersisterConfig, Synchronous, WalletStorageError, }; /// TC-005: sync heights round-trip with monotonic-max merge. @@ -82,6 +82,65 @@ fn tc013_wallet_metadata_roundtrip() { assert_eq!(birth_height, 12345); } +/// TC-CODE-029-1: journal_mode=Memory is rejected at open with a typed +/// `ConfigInvalid` error and the DB is not created. +#[test] +fn tc_code_029_1_journal_mode_memory_rejected() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("w.db"); + let mut cfg = SqlitePersisterConfig::new(&path); + cfg.journal_mode = JournalMode::Memory; + let err = SqlitePersister::open(cfg); + let matched = matches!(err.as_ref(), Err(WalletStorageError::ConfigInvalid { .. })); + assert!( + matched, + "expected ConfigInvalid for journal_mode=Memory, got error = {:?}", + err.as_ref().err() + ); + assert!( + !path.exists(), + "DB should not be created when config is invalid" + ); +} + +/// TC-CODE-029-2: journal_mode=Off is rejected at open with a typed +/// `ConfigInvalid` error and the DB is not created. +#[test] +fn tc_code_029_2_journal_mode_off_rejected() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("w.db"); + let mut cfg = SqlitePersisterConfig::new(&path); + cfg.journal_mode = JournalMode::Off; + let err = SqlitePersister::open(cfg); + let matched = matches!(err.as_ref(), Err(WalletStorageError::ConfigInvalid { .. })); + assert!( + matched, + "expected ConfigInvalid for journal_mode=Off, got error = {:?}", + err.as_ref().err() + ); + assert!( + !path.exists(), + "DB should not be created when config is invalid" + ); +} + +/// TC-CODE-029-3: busy_timeout=0 opens successfully but emits a +/// tracing::warn so operators can spot the footgun in logs. +#[test] +#[tracing_test::traced_test] +fn tc_code_029_3_busy_timeout_zero_warns() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("w.db"); + let mut cfg = SqlitePersisterConfig::new(&path); + cfg.busy_timeout = std::time::Duration::ZERO; + let p = SqlitePersister::open(cfg).expect("open should succeed with busy_timeout=0"); + drop(p); + assert!( + logs_contain("busy_timeout=0"), + "expected a busy_timeout=0 warning in captured logs" + ); +} + /// TC-079: synchronous=Off is rejected at open with a typed error. #[test] fn tc079_synchronous_off_rejected() { @@ -188,10 +247,12 @@ fn tc007_identity_key_entry_roundtrip() { let p2 = SqlitePersister::open(SqlitePersisterConfig::new(&path)).unwrap(); let conn = p2.lock_conn_for_test(); + // identity_keys is keyed by (identity_id, key_id); the wallet_id + // column is not part of the schema. let blob_bytes: Vec = conn .query_row( - "SELECT public_key_blob FROM identity_keys WHERE wallet_id = ?1 AND identity_id = ?2 AND key_id = ?3", - rusqlite::params![w.as_slice(), identity_id.as_slice(), 7i64], + "SELECT public_key_blob FROM identity_keys WHERE identity_id = ?1 AND key_id = ?2", + rusqlite::params![identity_id.as_slice(), 7i64], |row| row.get(0), ) .unwrap(); @@ -439,10 +500,11 @@ fn tc012_dashpay_overlay_roundtrip() { let p2 = SqlitePersister::open(SqlitePersisterConfig::new(&path)).unwrap(); let conn = p2.lock_conn_for_test(); + // dashpay_profiles is keyed by identity_id only. let profile_blob: Vec = conn .query_row( - "SELECT profile_blob FROM dashpay_profiles WHERE wallet_id = ?1 AND identity_id = ?2", - rusqlite::params![w.as_slice(), identity_id.as_slice()], + "SELECT profile_blob FROM dashpay_profiles WHERE identity_id = ?1", + rusqlite::params![identity_id.as_slice()], |row| row.get(0), ) .unwrap(); @@ -452,8 +514,8 @@ fn tc012_dashpay_overlay_roundtrip() { let payment_blob: Vec = conn .query_row( - "SELECT overlay_blob FROM dashpay_payments_overlay WHERE wallet_id = ?1 AND identity_id = ?2 AND payment_id = ?3", - rusqlite::params![w.as_slice(), identity_id.as_slice(), "tx-aaaa"], + "SELECT overlay_blob FROM dashpay_payments_overlay WHERE identity_id = ?1 AND payment_id = ?2", + rusqlite::params![identity_id.as_slice(), "tx-aaaa"], |row| row.get(0), ) .unwrap(); diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_restore_cross_process_exclusion.rs b/packages/rs-platform-wallet-storage/tests/sqlite_restore_cross_process_exclusion.rs new file mode 100644 index 0000000000..916232fc9d --- /dev/null +++ b/packages/rs-platform-wallet-storage/tests/sqlite_restore_cross_process_exclusion.rs @@ -0,0 +1,157 @@ +#![allow(clippy::field_reassign_with_default)] + +//! TC-CODE-005 / TC-CODE-010 / TC-CODE-015 — SQLite-native EXCLUSIVE +//! replaces the (false-positive) `flock(2)` advisory lock pattern that +//! pre-T-006 `restore_from` used for cross-process exclusion. The +//! advisory lock did not exclude rusqlite peers; `BEGIN EXCLUSIVE` +//! against the destination file does. + +mod common; + +use std::path::Path; + +use common::{ensure_wallet_meta, fresh_persister, wid}; +use platform_wallet::changeset::{ + CoreChangeSet, PlatformWalletChangeSet, PlatformWalletPersistence, +}; +use platform_wallet_storage::SqlitePersister; +use rusqlite::TransactionBehavior; + +fn seed_one_row(persister: &SqlitePersister, w: &[u8; 32]) { + ensure_wallet_meta(persister, w); + let mut cs = PlatformWalletChangeSet::default(); + cs.core = Some(CoreChangeSet { + synced_height: Some(7), + last_processed_height: Some(7), + ..Default::default() + }); + persister.store(*w, cs).unwrap(); +} + +/// TC-CODE-005-a — `restore_from` must hold a SQLite-native exclusive +/// lock for the full restore body. A peer rusqlite Connection (a +/// different process equivalent) opening the same DB and trying to +/// `BEGIN EXCLUSIVE` while restore is in flight must conflict. +/// +/// We assert the exclusion by reverse: AFTER `restore_from` returns, +/// the peer can again take its own EXCLUSIVE — proving the persister +/// did NOT leave a dangling EXCLUSIVE behind. The positive (peer +/// conflict during the body) is implicitly covered: if the persister +/// failed to take EXCLUSIVE, the peer's EXCLUSIVE held below would +/// have blocked our restore — and busy-timeouts would surface as +/// `Err`. Negative path also covered by TC-CODE-005-b: if a peer +/// HOLDS exclusive across restore, restore returns BUSY. +#[test] +fn restore_takes_and_releases_native_exclusive() { + let (persister, tmp, db_path) = fresh_persister(); + seed_one_row(&persister, &wid(0xA1)); + let backup_dir = tempfile::tempdir().expect("backup dir"); + let backup_path = persister.backup_to(backup_dir.path()).unwrap(); + drop(persister); + + SqlitePersister::restore_from_skip_backup(&db_path, &backup_path) + .expect("restore succeeds without peer contention"); + + // Peer can now grab its own EXCLUSIVE — restore released cleanly. + let mut peer = ro_conn_rw(&db_path); + let tx = peer + .transaction_with_behavior(TransactionBehavior::Exclusive) + .expect("peer EXCLUSIVE post-restore"); + tx.commit().expect("peer commit"); + + // Keep `tmp` and `backup_dir` alive until here. + drop(tmp); + drop(backup_dir); +} + +/// TC-CODE-005-b — when a peer holds EXCLUSIVE on the destination, +/// `restore_from` returns a busy error rather than silently steamrolling +/// the peer's write tx. With the pre-T-006 flock approach this would +/// have proceeded (flock doesn't see SQLite peers); with the +/// SQLite-native EXCLUSIVE it must conflict. +#[test] +fn restore_blocks_when_peer_holds_exclusive() { + let (persister, tmp, db_path) = fresh_persister(); + seed_one_row(&persister, &wid(0xA2)); + let backup_dir = tempfile::tempdir().expect("backup dir"); + let backup_path = persister.backup_to(backup_dir.path()).unwrap(); + drop(persister); + + // Peer opens a writer conn, sets a SHORT busy_timeout so we don't + // wedge the test on a wedge — then takes EXCLUSIVE and holds it. + let mut peer = ro_conn_rw(&db_path); + peer.pragma_update(None, "busy_timeout", 50i64).unwrap(); + let tx = peer + .transaction_with_behavior(TransactionBehavior::Exclusive) + .unwrap(); + + // restore_from should NOT succeed — the destination is locked. + let err = SqlitePersister::restore_from_skip_backup(&db_path, &backup_path) + .expect_err("restore must fail while peer holds EXCLUSIVE"); + let kind = format!("{err}"); + assert!( + kind.contains("locked") || kind.contains("busy") || kind.contains("database is locked"), + "expected a lock/busy error, got: {kind}" + ); + + drop(tx); + drop(peer); + drop(tmp); + drop(backup_dir); +} + +/// TC-CODE-005-b (grep half) + TC-CODE-010-a + TC-CODE-015-a — +/// flock / fs2 / fs4 must be gone from the persister. +#[test] +fn flock_and_fs2_traces_are_gone() { + let backup_rs = + std::fs::read_to_string(Path::new(env!("CARGO_MANIFEST_DIR")).join("src/sqlite/backup.rs")) + .expect("read backup.rs"); + for needle in [ + "fs2::", + "use fs2", + "fs4::", + "use fs4", + "try_lock_exclusive", + "advisory lock unsupported", + ] { + assert!( + !backup_rs.contains(needle), + "backup.rs must not reference `{needle}` after T-006" + ); + } + + let cargo_toml = + std::fs::read_to_string(Path::new(env!("CARGO_MANIFEST_DIR")).join("Cargo.toml")) + .expect("read Cargo.toml"); + for needle in ["fs2 =", "fs4 =", "dep:fs2", "dep:fs4"] { + assert!( + !cargo_toml.contains(needle), + "Cargo.toml must not list `{needle}` after T-006" + ); + } +} + +/// TC-CODE-005-c — README must describe the SQLite-native exclusion, +/// not the false advisory-flock claim. +#[test] +fn readme_describes_sqlite_native_exclusion() { + let readme = std::fs::read_to_string(Path::new(env!("CARGO_MANIFEST_DIR")).join("README.md")) + .expect("read README.md"); + assert!( + !readme.contains("flock(2)") && !readme.contains("advisory lock unsupported"), + "README must drop the false flock(2) claim" + ); + assert!( + readme.contains("BEGIN EXCLUSIVE") || readme.contains("SQLite-native"), + "README must describe the SQLite-native EXCLUSIVE pattern" + ); +} + +/// Helper — open the destination as a read-write rusqlite Connection +/// with a sane busy_timeout, mimicking what a peer process would do. +fn ro_conn_rw(path: &Path) -> rusqlite::Connection { + let conn = rusqlite::Connection::open(path).expect("rw open"); + conn.pragma_update(None, "busy_timeout", 5_000i64).unwrap(); + conn +} diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_trait_dispatch.rs b/packages/rs-platform-wallet-storage/tests/sqlite_trait_dispatch.rs new file mode 100644 index 0000000000..3315c95141 --- /dev/null +++ b/packages/rs-platform-wallet-storage/tests/sqlite_trait_dispatch.rs @@ -0,0 +1,169 @@ +#![allow(clippy::field_reassign_with_default)] + +//! TC-CODE-003 / TC-CODE-026 — `PlatformWalletPersistence::delete_wallet` +//! and `::commit_writes` are reachable through the trait (not just the +//! inherent methods on `SqlitePersister`). Dispatch happens through +//! `Arc` so consumers don't need a +//! concrete backend type at the call site. +//! +//! - TC-CODE-003-default — trait default `delete_wallet` returns an +//! empty report (proven via a NoPlatformPersistence-style stub). +//! - TC-CODE-003-sqlite — trait-dispatched `delete_wallet` on +//! `SqlitePersister` actually cascades the on-disk rows. +//! - TC-CODE-026-1 — trait default `commit_writes` returns an empty +//! report (same stub backend). +//! - TC-CODE-026-2 — trait-dispatched `commit_writes` on +//! `SqlitePersister` matches the inherent behavior (success). + +mod common; + +use std::sync::Arc; + +use common::{ensure_wallet_meta, fresh_persister, fresh_persister_with_mode, ro_conn, wid}; +use platform_wallet::changeset::{ + ClientStartState, CommitReport, CoreChangeSet, DeleteWalletReport, PersistenceError, + PlatformWalletChangeSet, PlatformWalletPersistence, +}; +use platform_wallet::wallet::platform_wallet::WalletId; +use platform_wallet_storage::FlushMode; + +fn core_with_height(synced_height: u32, last_processed_height: u32) -> CoreChangeSet { + CoreChangeSet { + synced_height: Some(synced_height), + last_processed_height: Some(last_processed_height), + ..Default::default() + } +} + +fn changeset(core: CoreChangeSet) -> PlatformWalletChangeSet { + PlatformWalletChangeSet { + core: Some(core), + ..Default::default() + } +} + +/// Stub persister that exercises every trait default — `delete_wallet` +/// and `commit_writes` are inherited from the trait, so an empty impl +/// suffices. +struct DefaultsOnlyPersister; + +impl PlatformWalletPersistence for DefaultsOnlyPersister { + 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()) + } +} + +/// TC-CODE-003-default — `delete_wallet` default impl returns an +/// empty report keyed by the requested wallet id. Backends with no +/// per-wallet disk state inherit this; consumers use the same Ok-arm +/// regardless of backend. +#[test] +fn tc_code_003_default_delete_wallet_returns_empty_report() { + let persister: Arc = Arc::new(DefaultsOnlyPersister); + let wallet_id = wid(0xAB); + let report: DeleteWalletReport = persister + .delete_wallet(wallet_id) + .expect("default delete_wallet must be infallible"); + assert_eq!(report.wallet_id, wallet_id); + assert!(report.backup_path.is_none()); + assert!(report.rows_removed_per_table.is_empty()); +} + +/// TC-CODE-003-sqlite — trait-dispatched `delete_wallet` on +/// `SqlitePersister` cascades the on-disk rows. Without the trait +/// impl this call would resolve to the default and silently leave +/// the rows in place. +#[test] +fn tc_code_003_sqlite_trait_delete_wallet_cascades_rows() { + let (persister, _tmp, path) = fresh_persister(); + let w = wid(0x55); + ensure_wallet_meta(&persister, &w); + // Land a per-wallet row via the trait so we have something to + // cascade. + PlatformWalletPersistence::store(&persister, w, changeset(core_with_height(11, 11))) + .expect("store must succeed in Immediate mode"); + + let count_for = |id: &[u8; 32]| -> i64 { + ro_conn(&path) + .query_row( + "SELECT COUNT(*) FROM core_sync_state WHERE wallet_id = ?1", + rusqlite::params![id.as_slice()], + |row| row.get(0), + ) + .unwrap() + }; + assert_eq!(count_for(&w), 1); + + // Dispatch through the trait — this is the call shape + // `PlatformWalletManager` uses. + let report = PlatformWalletPersistence::delete_wallet(&persister, w) + .expect("trait delete_wallet must succeed"); + assert_eq!(report.wallet_id, w); + assert!( + report.backup_path.is_some(), + "trait-dispatched delete_wallet must take an auto-backup (safe-by-default)" + ); + + assert_eq!(count_for(&w), 0); +} + +/// TC-CODE-026-1 — `commit_writes` default impl returns an empty +/// `CommitReport`. Drives backwards-compat for stubs + +/// `NoPlatformPersistence`-style implementors that don't track dirty +/// state. +#[test] +fn tc_code_026_1_commit_writes_default_returns_empty_report() { + let persister: Arc = Arc::new(DefaultsOnlyPersister); + let report: CommitReport = persister + .commit_writes() + .expect("default commit_writes must be infallible"); + assert!(report.is_ok()); + assert!(report.succeeded.is_empty()); + assert!(report.failed.is_empty()); + assert!(report.still_pending.is_empty()); +} + +/// TC-CODE-026-2 — trait-dispatched `commit_writes` on +/// `SqlitePersister` flushes every dirty wallet just like the +/// inherent method (no behavioral drift across dispatch). +#[test] +fn tc_code_026_2_sqlite_trait_commit_writes_flushes_dirty() { + let (persister, _tmp, path) = fresh_persister_with_mode(FlushMode::Manual); + let a = wid(0x11); + let b = wid(0x22); + ensure_wallet_meta(&persister, &a); + ensure_wallet_meta(&persister, &b); + PlatformWalletPersistence::store(&persister, a, changeset(core_with_height(3, 3))) + .expect("store A"); + PlatformWalletPersistence::store(&persister, b, changeset(core_with_height(4, 4))) + .expect("store B"); + + let report = PlatformWalletPersistence::commit_writes(&persister) + .expect("trait commit_writes must succeed"); + assert!(report.is_ok(), "report={report:?}"); + assert_eq!(report.succeeded.len(), 2); + + let count_for = |id: &[u8; 32]| -> i64 { + ro_conn(&path) + .query_row( + "SELECT COUNT(*) FROM core_sync_state WHERE wallet_id = ?1", + rusqlite::params![id.as_slice()], + |row| row.get(0), + ) + .unwrap() + }; + assert_eq!(count_for(&a), 1); + assert_eq!(count_for(&b), 1); +} diff --git a/packages/rs-platform-wallet/src/changeset/mod.rs b/packages/rs-platform-wallet/src/changeset/mod.rs index 3a50400528..aa0a329bf0 100644 --- a/packages/rs-platform-wallet/src/changeset/mod.rs +++ b/packages/rs-platform-wallet/src/changeset/mod.rs @@ -41,4 +41,7 @@ pub use platform_address_sync_start_state::PlatformAddressSyncStartState; pub use shielded_changeset::ShieldedChangeSet; #[cfg(feature = "shielded")] pub use shielded_sync_start_state::{ShieldedSubwalletStartState, ShieldedSyncStartState}; -pub use traits::{PersistenceError, PlatformWalletPersistence}; +pub use traits::{ + CommitReport, DeleteWalletReport, PersistenceError, PersistenceErrorKind, + PlatformWalletPersistence, +}; diff --git a/packages/rs-platform-wallet/src/changeset/traits.rs b/packages/rs-platform-wallet/src/changeset/traits.rs index 7cddadac2d..68a2b2a573 100644 --- a/packages/rs-platform-wallet/src/changeset/traits.rs +++ b/packages/rs-platform-wallet/src/changeset/traits.rs @@ -3,19 +3,52 @@ //! Implementors choose their own storage engine (SQLite, file, memory, remote). //! The traits guarantee that deltas are persisted atomically. +use std::collections::BTreeMap; +use std::error::Error as StdError; +use std::path::PathBuf; + use crate::changeset::changeset::PlatformWalletChangeSet; use crate::changeset::client_start_state::ClientStartState; use crate::wallet::platform_wallet::WalletId; use dashcore::Txid; use key_wallet::managed_account::transaction_record::TransactionRecord; +/// Retry classification for [`PersistenceError::Backend`]. +/// +/// The kind carries the persistor's `is_transient()` contract across +/// the trait boundary so consumers can decide whether to retry, undo +/// in-memory state, or surface the failure to the user without +/// guessing from a string message. +/// +/// The enum is intentionally NOT `#[non_exhaustive]`: adding a new +/// kind MUST force every consumer match to update explicitly. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum PersistenceErrorKind { + /// The persistor 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. + Transient, + /// The persistor 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. + 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. + Constraint, +} + /// Errors returned by a [`PlatformWalletPersistence`] backend. /// /// Concrete (non-`Box`) so callers and downstream /// traits can compose the result types without erasing the /// error's shape. Backends that don't fit cleanly into -/// [`Self::LockPoisoned`] render their native error via -/// [`Self::backend`] into [`Self::Backend`]. +/// [`Self::LockPoisoned`] route their native error through +/// [`Self::backend_with_kind`] (or [`Self::backend`] when the kind +/// isn't known) into [`Self::Backend`]. #[derive(Debug, thiserror::Error)] pub enum PersistenceError { /// An internal synchronization primitive is poisoned (a @@ -26,35 +59,76 @@ pub enum PersistenceError { LockPoisoned, /// Error bubbled up from the underlying storage engine - /// (SQLite, file I/O, FFI callback, etc.). Carries the - /// backend's error message; the original error type is - /// intentionally erased so the trait stays object-safe - /// without generic error parameters. - #[error("persistence backend error: {0}")] - Backend(String), + /// (SQLite, file I/O, FFI callback, etc.). + /// + /// `kind` carries the retry classification — see + /// [`PersistenceErrorKind`]. `source` is a boxed typed error so + /// callers that need finer detail can downcast (the canonical + /// SQLite backend boxes `WalletStorageError`, which preserves the + /// full typed source chain). + #[error("persistence backend error ({kind:?}): {source}")] + Backend { + kind: PersistenceErrorKind, + source: Box, + }, } impl PersistenceError { - /// Convenience constructor that stringifies any - /// `Display` error into [`PersistenceError::Backend`]. - pub fn backend(err: impl std::fmt::Display) -> Self { - Self::Backend(err.to_string()) + /// Construct a [`Self::Backend`] from any boxable error, + /// classified as [`PersistenceErrorKind::Fatal`]. + /// + /// Use this when the caller does not (or cannot) classify the + /// kind. Defaulting to `Fatal` is the conservative choice: a + /// misclassification reads as "do not retry" rather than + /// spuriously retrying a permanent failure. + pub fn backend(source: E) -> Self + where + E: Into>, + { + Self::Backend { + kind: PersistenceErrorKind::Fatal, + source: source.into(), + } } -} -// Ergonomic conversions so backends can `.into()` a message without -// spelling out the enum variant. The common pattern in FFI-style -// backends is `Err(format!("...").into())`; the `From` impl -// keeps that terse while routing into the typed error. -impl From for PersistenceError { - fn from(msg: String) -> Self { - Self::Backend(msg) + /// Construct a [`Self::Backend`] with an explicit kind. Use this + /// at the persistor boundary where the kind is known (e.g. + /// `From` checks `is_transient()` and the + /// constraint codes before calling this). + pub fn backend_with_kind(kind: PersistenceErrorKind, source: E) -> Self + where + E: Into>, + { + Self::Backend { + kind, + source: source.into(), + } } -} -impl From<&str> for PersistenceError { - fn from(msg: &str) -> Self { - Self::Backend(msg.to_string()) + /// `true` if the error is a `Backend` whose kind is + /// [`PersistenceErrorKind::Transient`]. `LockPoisoned`, `Fatal`, + /// and `Constraint` all read as non-transient. + pub fn is_transient(&self) -> bool { + matches!( + self, + Self::Backend { + kind: PersistenceErrorKind::Transient, + .. + } + ) + } + + /// Retry-policy classification for the error. + /// + /// Returns `None` for [`Self::LockPoisoned`] (which is its own + /// trait-level variant) and `Some(kind)` for [`Self::Backend`]. + /// Callers that always need a kind should treat `None` as + /// [`PersistenceErrorKind::Fatal`]. + pub fn kind(&self) -> Option { + match self { + Self::LockPoisoned => None, + Self::Backend { kind, .. } => Some(*kind), + } } } @@ -135,21 +209,28 @@ pub trait PlatformWalletPersistence: Send + Sync { /// /// # Errors /// - /// Implementations classify failures along a two-axis contract: + /// Implementations classify failures via + /// [`PersistenceErrorKind`] on the returned + /// [`PersistenceError::Backend`] so callers can drive retry policy + /// off [`PersistenceError::is_transient`]: /// - /// - **Transient** (`PersistenceError::backend(..)` whose source - /// carries `is_transient() == true` — for the canonical SQLite - /// backend that's `SQLITE_BUSY` / `SQLITE_LOCKED`, and as of - /// ATOM-008 also the I/O-class codes `SQLITE_FULL` / - /// `SQLITE_IOERR` / `SQLITE_NOMEM`): the buffered changeset is + /// - **[`PersistenceErrorKind::Transient`]** — for the canonical + /// SQLite backend that's `SQLITE_BUSY` / `SQLITE_LOCKED`, and as + /// of ATOM-008 also the I/O-class codes `SQLITE_FULL` / + /// `SQLITE_IOERR` / `SQLITE_NOMEM`: the buffered changeset is /// preserved (re-merged via the buffer's `restore` path so any /// `store` that landed during the failed flush wins on LWW /// fields), and the caller MAY retry with exponential backoff. - /// - **Fatal** (everything else — schema corruption, logic bugs, - /// integrity violations): the buffer is dropped, the staged - /// changeset is gone, and the backend logs a structured - /// `tracing::error!`. The caller MUST NOT retry — the data is - /// not recoverable through this trait. + /// - **[`PersistenceErrorKind::Constraint`]** — SQL + /// constraint / FK / integrity violation. Caller bug; the data + /// is rejected by the schema. MUST NOT retry without changing + /// the data. + /// - **[`PersistenceErrorKind::Fatal`]** — everything else + /// (schema corruption, logic bugs, I/O outside the transient + /// class): the buffer is dropped, the staged changeset is gone, + /// and the backend logs a structured `tracing::error!`. The + /// caller MUST NOT retry — the data is not recoverable through + /// this trait. /// /// [`PersistenceError::LockPoisoned`] is fatal but distinguished /// at the variant level so callers can pattern-match on it. @@ -215,4 +296,103 @@ pub trait PlatformWalletPersistence: Send + Sync { ) -> Result, PersistenceError> { Ok(None) } + + /// Cascade-delete every persisted row owned by `wallet_id`. + /// + /// The default impl is a no-op that returns an empty + /// [`DeleteWalletReport`]. Backends with no per-wallet state + /// on disk (e.g. [`NoPlatformPersistence`](crate::wallet::persister::NoPlatformPersistence)) + /// inherit it. + /// + /// # Errors + /// + /// - [`PersistenceErrorKind::Transient`] (e.g. `SQLITE_BUSY`): + /// callers MAY retry with backoff. + /// - [`PersistenceErrorKind::Constraint`] / [`PersistenceErrorKind::Fatal`] + /// / [`PersistenceError::LockPoisoned`]: callers MUST NOT retry; + /// the disk state may carry orphan rows that an admin tool has + /// to clean up out-of-band. + fn delete_wallet(&self, wallet_id: WalletId) -> Result { + Ok(DeleteWalletReport { + wallet_id, + backup_path: None, + rows_removed_per_table: BTreeMap::new(), + }) + } + + /// Flush every dirty wallet's buffered changeset to durable storage. + /// + /// The default impl is a no-op that returns an empty + /// [`CommitReport`]. Backends that flush inline (e.g. SQLite in + /// [`FlushMode::Immediate`](https://docs.rs/platform-wallet-storage)) + /// or that have nothing to flush ([`NoPlatformPersistence`](crate::wallet::persister::NoPlatformPersistence)) + /// inherit it. + /// + /// # Errors + /// + /// Returns `Err` ONLY when even enumerating the dirty set fails + /// (e.g. the buffer mutex is poisoned). Per-wallet flush failures + /// land on `report.failed` with the classified `PersistenceError` + /// per wallet so a single bad wallet does not hide its siblings' + /// success. `report.still_pending` lists wallets that were never + /// attempted because an earlier per-flush call short-circuited + /// the loop (today: `LockPoisoned`). + /// + /// Atomicity is per-wallet, not cross-wallet: there is no + /// transaction spanning multiple wallets. + fn commit_writes(&self) -> Result { + Ok(CommitReport { + succeeded: Vec::new(), + failed: Vec::new(), + still_pending: Vec::new(), + }) + } +} + +/// Outcome of a [`PlatformWalletPersistence::commit_writes`] call. +/// +/// Each dirty wallet's per-flush result lands in exactly one of the +/// three vectors so a single failed wallet doesn't hide its siblings' +/// success (or vice-versa). Callers can retry `still_pending` directly; +/// `failed` carries the classified `PersistenceError` per wallet so +/// transient-vs-fatal decisions stay local. +#[derive(Debug)] +pub struct CommitReport { + /// Wallets that flushed successfully (durable on disk). + pub succeeded: Vec, + /// Wallets whose flush returned an error. The `PersistenceError` + /// carries the classification and source per + /// [`PersistenceErrorKind`]. + pub failed: Vec<(WalletId, PersistenceError)>, + /// Wallets we never attempted because an earlier per-flush call + /// short-circuited the loop (today: a `LockPoisoned` — the + /// connection mutex is gone). + pub still_pending: Vec, +} + +impl CommitReport { + /// `true` when every dirty wallet flushed cleanly. + pub fn is_ok(&self) -> bool { + self.failed.is_empty() && self.still_pending.is_empty() + } +} + +/// Outcome of a [`PlatformWalletPersistence::delete_wallet`] call. +/// +/// Lives on the trait so consumers can match on the report without +/// pulling in a backend-specific crate. The SQLite backend builds an +/// instance with `rows_removed_per_table` populated; backends that +/// don't track per-table row counts emit an empty map. +#[derive(Debug, Clone)] +pub struct DeleteWalletReport { + /// The wallet that was deleted. + pub wallet_id: WalletId, + /// Absolute path of the pre-delete auto-backup taken before the + /// cascade. `None` when the backend skipped the backup + /// (intentionally — e.g. the SQLite CLI's `--no-auto-backup` — or + /// because the backend has no backup concept). + pub backup_path: Option, + /// Per-table row counts the backend deleted. Empty for backends + /// that don't expose per-table accounting. + pub rows_removed_per_table: BTreeMap<&'static str, usize>, } 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 d47d8cdcef..ae9ab5b6b9 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 @@ -648,9 +648,7 @@ mod tests { _wallet_id: WalletId, _txid: &Txid, ) -> Result, PersistenceError> { - Err(PersistenceError::Backend( - "simulated backend failure".into(), - )) + Err(PersistenceError::backend("simulated backend failure")) } }