refactor(store): improve SQL connections

CHANGELOG.md

@@ -22,11 +22,13 @@
 * [BREAKING][removal][cli] Removed the `account --show --with-code` flag. Use `account --inspect <ID> --verbose` to view procedure disassembly. ([#2312](https://github.com/0xMiden/rust-sdk/issues/2312)).
 * [BREAKING][rename][cli] Renamed the `token_symbol_map.toml` Bech32 field from `id` to `address` ([#2377](https://github.com/0xMiden/rust-sdk/pull/2377)).
 * [BREAKING][type][rust] Added the `NoteFilter::ScriptRoots` variant, so exhaustive matches on `NoteFilter` in `Store` implementations must handle it ([#2335](https://github.com/0xMiden/rust-sdk/pull/2335)).
+* [BREAKING][behavior][store] `SqliteStore::new` rejects a database path that is not valid UTF-8 ([#2363](https://github.com/0xMiden/rust-sdk/pull/2363)).
 * [BREAKING][behavior][rpc] The `SyncNotes` response now carries a reduced note metadata message: instead of the note's attachments commitment it carries one entry per attachment, with single-word attachments sent verbatim and larger ones sent as commitments. The client reconstructs the protocol-level `NoteMetadata` from those entries, so it requires a node that speaks this format.
 * [BREAKING][behavior][store] The SQLite base schema now declares an index on `input_notes(script_root)`. This changes the schema fingerprint, so opening a database created before this change fails with `SchemaHashMismatch` and existing stores must be recreated ([#2335](https://github.com/0xMiden/rust-sdk/pull/2335)).
 
 ### Enhancements
 
+* [store] Added `SqliteStore::database_filepath`, which returns the backing database path losslessly as a `&Path` ([#2363](https://github.com/0xMiden/rust-sdk/pull/2363)).
 * [FEATURE][rust] A client that only watches a public account now recovers notes the account consumed authenticated, even when it never tracked them by tag. During sync it reads the note references the node attaches to the account's transactions, fetches each note body by id, and surfaces it through `InputNoteReader`. Requires node `0.15.1` ([#2300](https://github.com/0xMiden/rust-sdk/pull/2300)).
 * [FEATURE][cli] Added a `--payback-note-type` option to `swap` so the payback note can be created as public or private (defaults to private). Public payback works without any off-band advice now that SWAP derives the payback recipient deterministically ([#2190](https://github.com/0xMiden/rust-sdk/pull/2190)).
 * [FEATURE][cli] `init` now also writes the non-fungible faucet, guarded multisig auth and network account auth component packages ([#2356](https://github.com/0xMiden/rust-sdk/pull/2356)).

crates/sqlite-store/Cargo.toml

@@ -22,6 +22,7 @@ anyhow      = { workspace = true }
 async-trait = { workspace = true }
 chrono      = { workspace = true }
 thiserror   = { workspace = true }
+tracing     = { workspace = true }
 
 # External dependencies
 deadpool           = { default-features = false, features = ["managed", "rt_tokio_1"], version = "0.12" }
@@ -37,3 +38,5 @@ workspace = true
 # Enable client testing utilities only for tests
 miden-client    = { features = ["testing"], workspace = true }
 miden-standards = { features = ["testing"], workspace = true }
+# `time` is only needed to bound tests that would otherwise hang on a pool regression
+tokio = { features = ["time"], workspace = true }

crates/sqlite-store/README.md

@@ -1,12 +1,11 @@
 # SQLite Store
 
 SQLite-backed `Store` implementation for the Miden client. This crate provides a production‑ready
-persistence layer for std environments using SQLite (via `rusqlite`) with a small in‑memory
-`AccountSmtForest` cache for fast proof queries.
-
+persistence layer for std environments using SQLite (via `rusqlite`) with an in‑memory account SMT
+forest for fast proof queries.
 - Persists accounts, notes, transactions, block headers, and MMR nodes
 - Atomic updates on transaction and state sync paths
-- Connection pooling (Deadpool) and bundled SQLite for reproducible builds
+- WAL journaling and bundled SQLite for reproducible builds
 
 ## Quick Start
 

crates/sqlite-store/src/db_management/pool_manager.rs

@@ -1,7 +1,8 @@
 use std::path::PathBuf;
+use std::time::Duration;
 
 use deadpool::Runtime;
-use deadpool::managed::{Manager, Metrics, RecycleResult};
+use deadpool::managed::{Manager, Metrics, RecycleError, RecycleResult};
 use rusqlite::Connection;
 use rusqlite::vtab::array;
 
@@ -47,7 +48,11 @@ impl SqlitePoolManager {
                 if path.exists()
                     && let Err(e) = std::fs::set_permissions(&path, perms.clone())
                 {
-                    eprintln!("Warning: failed to set permissions on {}: {e}", path.display());
+                    tracing::warn!(
+                        path = %path.display(),
+                        error = %e,
+                        "failed to restrict permissions on the database file"
+                    );
                 }
             }
         }
@@ -57,6 +62,21 @@ impl SqlitePoolManager {
         // queries we want to run
         array::load_module(&conn)?;
 
+        conn.busy_timeout(Duration::from_secs(5))?;
+
+        let journal_mode: String =
+            conn.pragma_update_and_check(None, "journal_mode", "WAL", |row| row.get(0))?;
+        if !journal_mode.eq_ignore_ascii_case("wal") {
+            tracing::warn!(
+                journal_mode = %journal_mode,
+                path = %self.database_path.display(),
+                "database does not support WAL journal mode; commits will be slower and readers \
+                 will block on writes"
+            );
+        }
+
+        conn.pragma_update(None, "synchronous", "NORMAL")?;
+
         // Enable foreign key checks.
         conn.pragma_update(None, "foreign_keys", "ON")?;
 
@@ -77,7 +97,211 @@ impl Manager for SqlitePoolManager {
         deadpool_sync::SyncWrapper::new(RUNTIME, move || conn).await
     }
 
-    async fn recycle(&self, _: &mut Self::Type, _: &Metrics) -> RecycleResult<Self::Error> {
+    async fn recycle(&self, conn: &mut Self::Type, _: &Metrics) -> RecycleResult<Self::Error> {
+        if conn.is_mutex_poisoned() {
+            return Err(RecycleError::message("sqlite connection mutex is poisoned"));
+        }
+
+        // A closure that issued a bare `BEGIN` and returned early leaves the transaction open on
+        // the connection, holding its locks and hiding its writes from the next caller. `rusqlite`
+        // only rolls back for a dropped `Transaction` guard, so undo it here.
+        conn.interact(|conn| {
+            if conn.is_autocommit() {
+                Ok(())
+            } else {
+                conn.execute_batch("ROLLBACK")
+            }
+        })
+        .await
+        .map_err(|_| RecycleError::message("failed to reset the sqlite connection"))??;
+
+        Ok(())
+    }
+}
+
+// TESTS
+// ================================================================================================
+
+#[cfg(test)]
+mod tests {
+    use std::time::Duration;
+
+    use miden_client::EMPTY_WORD;
+    use miden_client::account::component::{AccountComponent, BasicWallet};
+    use miden_client::account::{
+        Account,
+        AccountBuilder,
+        AccountBuilderSchemaCommitmentExt,
+        AccountType,
+        Address,
+    };
+    use miden_client::auth::{AuthSchemeId, AuthSingleSig, PublicKeyCommitment};
+    use miden_client::store::{ClientAccountType, Store};
+    use miden_client::testing::common::create_test_store_path;
+    use miden_protocol::account::AccountComponentMetadata;
+    use miden_standards::account::auth::Approver;
+
+    use crate::SqliteStore;
+    use crate::sql_error::SqlResultExt;
+    use crate::tests::create_test_store;
+
+    /// A construction that hangs means the pool is being acquired twice, so bound it rather than
+    /// let it burn the whole nextest slow-timeout budget.
+    const OPEN_TIMEOUT: Duration = Duration::from_secs(30);
+
+    /// Builds a minimal account that can be inserted into a store.
+    fn test_account(init_seed: [u8; 32]) -> anyhow::Result<Account> {
+        let component = AccountComponent::new(
+            BasicWallet::code().clone(),
+            vec![],
+            AccountComponentMetadata::new("miden::testing::dummy_component"),
+        )?;
+
+        // The auth component has to be added first, because `AccountCode` takes its first
+        // component as the authentication one.
+        Ok(AccountBuilder::new(init_seed)
+            .account_type(AccountType::Private)
+            .with_component(AuthSingleSig::new(Approver::new(
+                PublicKeyCommitment::from(EMPTY_WORD),
+                AuthSchemeId::Falcon512Poseidon2,
+            )))
+            .with_component(component)
+            .build_with_schema_commitment()?)
+    }
+
+    #[tokio::test]
+    async fn connection_pragmas_are_applied() -> anyhow::Result<()> {
+        let store = create_test_store().await;
+
+        let (journal_mode, synchronous, busy_timeout, foreign_keys) = store
+            .interact_with_connection(|conn| {
+                let journal_mode: String = conn
+                    .pragma_query_value(None, "journal_mode", |row| row.get(0))
+                    .into_store_error()?;
+                let synchronous: i32 = conn
+                    .pragma_query_value(None, "synchronous", |row| row.get(0))
+                    .into_store_error()?;
+                let busy_timeout: i32 = conn
+                    .pragma_query_value(None, "busy_timeout", |row| row.get(0))
+                    .into_store_error()?;
+                let foreign_keys: i32 = conn
+                    .pragma_query_value(None, "foreign_keys", |row| row.get(0))
+                    .into_store_error()?;
+
+                Ok((journal_mode, synchronous, busy_timeout, foreign_keys))
+            })
+            .await?;
+
+        // Asserted on the connection rather than on the `PRAGMA` return value, because
+        // `pragma_update` reports success even when SQLite kept the previous journal mode.
+        assert_eq!(journal_mode.to_lowercase(), "wal");
+        // 1 is `NORMAL`.
+        assert_eq!(synchronous, 1);
+        assert_eq!(busy_timeout, 5_000);
+        assert_eq!(foreign_keys, 1);
+
         Ok(())
     }
+
+    /// `SqliteStore::new` acquires a connection twice on a database that already holds accounts:
+    /// once for the migrations and again for the SMT forest initialization. It has to release the
+    /// first before taking the second, so that construction never needs two connections at once
+    /// and cannot exhaust the pool.
+    #[tokio::test]
+    async fn new_does_not_deadlock_on_a_populated_database() -> anyhow::Result<()> {
+        let path = create_test_store_path();
+
+        let account = test_account([0; 32])?;
+        {
+            let store =
+                tokio::time::timeout(OPEN_TIMEOUT, SqliteStore::new(path.clone())).await??;
+            store
+                .insert_account(&account, Address::new(account.id()), ClientAccountType::Native)
+                .await?;
+        }
+
+        let store = tokio::time::timeout(OPEN_TIMEOUT, SqliteStore::new(path)).await??;
+        assert_eq!(store.get_account_ids().await?, vec![account.id()]);
+
+        Ok(())
+    }
+
+    /// A panic inside `interact` poisons the connection's mutex. `recycle` has to drop that
+    /// connection, otherwise the pool keeps handing the poisoned one back to callers.
+    #[tokio::test]
+    async fn poisoned_connection_is_replaced() -> anyhow::Result<()> {
+        let store = create_test_store().await;
+
+        let panicked = store
+            .interact_with_connection(|_| -> Result<(), miden_client::store::StoreError> {
+                panic!("poisoning the connection on purpose")
+            })
+            .await;
+        assert!(panicked.is_err());
+
+        store.set_setting("after-panic".to_string(), b"value".to_vec()).await?;
+        assert_eq!(store.get_setting("after-panic".to_string()).await?, Some(b"value".to_vec()));
+
+        Ok(())
+    }
+
+    /// A bare `BEGIN` that is never committed holds the connection's locks and hides its writes
+    /// from the next caller, so `recycle` has to roll it back.
+    #[tokio::test]
+    async fn leaked_transaction_is_rolled_back() -> anyhow::Result<()> {
+        let store = create_test_store().await;
+
+        store
+            .interact_with_connection(|conn| {
+                conn.execute_batch("BEGIN").into_store_error()?;
+                conn.execute_batch("INSERT INTO settings (name, value) VALUES ('leaked', X'00')")
+                    .into_store_error()?;
+                Ok(())
+            })
+            .await?;
+
+        let autocommit = store.interact_with_connection(|conn| Ok(conn.is_autocommit())).await?;
+        assert!(autocommit, "the leaked transaction was not rolled back");
+        assert_eq!(store.get_setting("leaked".to_string()).await?, None);
+
+        Ok(())
+    }
+
+    /// Overlapping accessors are unsupported (see [`SqliteStore`]), but they must degrade to
+    /// waiting on each other's write locks rather than failing with `SQLITE_BUSY`.
+    #[tokio::test]
+    async fn overlapping_accessors_wait_instead_of_failing() -> anyhow::Result<()> {
+        let path = create_test_store_path();
+        let first = SqliteStore::new(path.clone()).await?;
+        let second = SqliteStore::new(path).await?;
+
+        first.set_setting("from-first".to_string(), b"1".to_vec()).await?;
+        second.set_setting("from-second".to_string(), b"2".to_vec()).await?;
+
+        assert_eq!(second.get_setting("from-first".to_string()).await?, Some(b"1".to_vec()));
+        assert_eq!(first.get_setting("from-second".to_string()).await?, Some(b"2".to_vec()));
+
+        Ok(())
+    }
+
+    /// `Store::identifier` returns `&str`, so a path it cannot represent has to be refused at
+    /// construction rather than reported as a placeholder every such path would share.
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn non_utf8_database_path_is_rejected() {
+        use std::ffi::OsStr;
+        use std::os::unix::ffi::OsStrExt;
+
+        let mut path = create_test_store_path();
+        let mut file_name =
+            path.file_name().expect("test path has a file name").as_bytes().to_vec();
+        file_name.push(0xff);
+        path.set_file_name(OsStr::from_bytes(&file_name));
+
+        // `SqliteStore` is not `Debug`, so this cannot go through `expect_err`.
+        let Err(error) = SqliteStore::new(path).await else {
+            panic!("a non-UTF-8 path must be rejected");
+        };
+        assert!(error.to_string().contains("not valid UTF-8"), "unexpected error: {error}");
+    }
 }