From c72ca868659ce10fb04ae6f76207279644efecc3 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Mon, 7 Sep 2026 20:18:46 +0200 Subject: [PATCH] fix(storage): stop a panic poisoning the connection mutex for the whole session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RusqliteService` is the path every async database operation in the app takes, and all seven of its lock sites used a raw `.lock()`. A single panic while that guard is held poisons the mutex, after which every database call for the rest of the process returns "poisoned lock" — for a database-backed app, the entire UI stops working until restart. `utils::lock` exists to stop exactly this cascade, and `storage::Database` already used `lock_safe()`. The busiest lock in the app was the one that did not. The test poisons the connection the way a panicking row mapper would and asserts queries still serve. The same raw-lock pattern remains at ~121 command-layer sites on the `DatabaseWrapper`/`CredentialsWrapper` mutexes. Those degrade to a failed command rather than a panic, and converting them is a mechanical sweep better reviewed on its own. --- src-tauri/src/storage/db_service.rs | 94 ++++++++++++++++++++++------- 1 file changed, 73 insertions(+), 21 deletions(-) diff --git a/src-tauri/src/storage/db_service.rs b/src-tauri/src/storage/db_service.rs index dbda6c07e..8fdf96799 100644 --- a/src-tauri/src/storage/db_service.rs +++ b/src-tauri/src/storage/db_service.rs @@ -7,6 +7,7 @@ //! - Test with different database backends //! - Migrate to other database systems in the future +use crate::utils::lock::MutexSafe; use async_trait::async_trait; use rusqlite::{params_from_iter, Connection, Result as SqliteResult, Row}; use std::sync::{Arc, Mutex}; @@ -128,9 +129,10 @@ impl DatabaseService for RusqliteService { async fn execute(&self, query: Query) -> DbResult { let conn = Arc::clone(&self.conn); tokio::task::spawn_blocking(move || { - let conn = conn - .lock() - .map_err(|e| format!("Failed to lock connection: {}", e))?; + // `lock_safe`, not `lock`: this is the busiest lock in the app and a + // panic under the guard would otherwise poison it, failing every + // later query with "poisoned lock" until the process restarts. + let conn = conn.lock_safe(); execute_query(&conn, query) }) .await @@ -141,9 +143,10 @@ impl DatabaseService for RusqliteService { let conn = Arc::clone(&self.conn); let sql = sql.to_string(); tokio::task::spawn_blocking(move || { - let conn = conn - .lock() - .map_err(|e| format!("Failed to lock connection: {}", e))?; + // `lock_safe`, not `lock`: this is the busiest lock in the app and a + // panic under the guard would otherwise poison it, failing every + // later query with "poisoned lock" until the process restarts. + let conn = conn.lock_safe(); conn.execute_batch(&sql) .map_err(|e| format!("Execute batch failed: {}", e)) }) @@ -158,9 +161,10 @@ impl DatabaseService for RusqliteService { { let conn = Arc::clone(&self.conn); tokio::task::spawn_blocking(move || { - let conn = conn - .lock() - .map_err(|e| format!("Failed to lock connection: {}", e))?; + // `lock_safe`, not `lock`: this is the busiest lock in the app and a + // panic under the guard would otherwise poison it, failing every + // later query with "poisoned lock" until the process restarts. + let conn = conn.lock_safe(); query_one(&conn, query, mapper) }) .await @@ -174,9 +178,10 @@ impl DatabaseService for RusqliteService { { let conn = Arc::clone(&self.conn); tokio::task::spawn_blocking(move || { - let conn = conn - .lock() - .map_err(|e| format!("Failed to lock connection: {}", e))?; + // `lock_safe`, not `lock`: this is the busiest lock in the app and a + // panic under the guard would otherwise poison it, failing every + // later query with "poisoned lock" until the process restarts. + let conn = conn.lock_safe(); query_optional(&conn, query, mapper) }) .await @@ -190,9 +195,10 @@ impl DatabaseService for RusqliteService { { let conn = Arc::clone(&self.conn); tokio::task::spawn_blocking(move || { - let conn = conn - .lock() - .map_err(|e| format!("Failed to lock connection: {}", e))?; + // `lock_safe`, not `lock`: this is the busiest lock in the app and a + // panic under the guard would otherwise poison it, failing every + // later query with "poisoned lock" until the process restarts. + let conn = conn.lock_safe(); query_many(&conn, query, mapper) }) .await @@ -206,9 +212,10 @@ impl DatabaseService for RusqliteService { { let conn = Arc::clone(&self.conn); tokio::task::spawn_blocking(move || { - let conn = conn - .lock() - .map_err(|e| format!("Failed to lock connection: {}", e))?; + // `lock_safe`, not `lock`: this is the busiest lock in the app and a + // panic under the guard would otherwise poison it, failing every + // later query with "poisoned lock" until the process restarts. + let conn = conn.lock_safe(); conn.execute("BEGIN TRANSACTION", []) .map_err(|e| format!("Failed to begin transaction: {}", e))?; @@ -236,9 +243,10 @@ impl DatabaseService for RusqliteService { async fn last_insert_rowid(&self) -> DbResult { let conn = Arc::clone(&self.conn); tokio::task::spawn_blocking(move || { - let conn = conn - .lock() - .map_err(|e| format!("Failed to lock connection: {}", e))?; + // `lock_safe`, not `lock`: this is the busiest lock in the app and a + // panic under the guard would otherwise poison it, failing every + // later query with "poisoned lock" until the process restarts. + let conn = conn.lock_safe(); Ok(conn.last_insert_rowid()) }) .await @@ -410,4 +418,48 @@ mod tests { let count: i32 = service.query_one(query, |row| row.get(0)).await.unwrap(); assert_eq!(count, 2); } + /// A panic while the connection guard is held must not brick every later + /// query. + /// + /// This is the single busiest lock in the app — every async DB operation + /// goes through it. With a raw `.lock()`, one panic under the guard poisons + /// the mutex and every subsequent call returns "poisoned lock" until the + /// process restarts, which for a database-backed app means the whole UI + /// stops working. `utils::lock` exists precisely to stop that cascade, and + /// `storage::Database` already used it; this path did not. + /// + /// TRACES: UR-002 | DR-012 | UT-014 + #[tokio::test] + async fn a_poisoned_connection_still_serves_queries() { + let conn = Arc::new(Mutex::new(Connection::open_in_memory().unwrap())); + { + let c = conn.lock_safe(); + c.execute_batch("CREATE TABLE test (id INTEGER PRIMARY KEY);") + .unwrap(); + } + + // Poison the mutex the way a panicking row mapper would. + let poisoner = Arc::clone(&conn); + let hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(|_| {})); + let _ = std::thread::spawn(move || { + let _guard = poisoner.lock().unwrap(); + panic!("a row mapper blew up while holding the connection"); + }) + .join(); + std::panic::set_hook(hook); + assert!(conn.lock().is_err(), "the mutex should now be poisoned"); + + // Every operation must still work. + let service = RusqliteService::new(Arc::clone(&conn)); + service + .execute(Query::new("INSERT INTO test (id) VALUES (1)")) + .await + .expect("execute must survive a poisoned connection"); + let count: i32 = service + .query_one(Query::new("SELECT COUNT(*) FROM test"), |row| row.get(0)) + .await + .expect("query_one must survive a poisoned connection"); + assert_eq!(count, 1); + } }