fix(storage): stop a panic poisoning the connection mutex for the whole session
`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.
This commit is contained in:
@@ -7,6 +7,7 @@
|
|||||||
//! - Test with different database backends
|
//! - Test with different database backends
|
||||||
//! - Migrate to other database systems in the future
|
//! - Migrate to other database systems in the future
|
||||||
|
|
||||||
|
use crate::utils::lock::MutexSafe;
|
||||||
use async_trait::async_trait;
|
use async_trait::async_trait;
|
||||||
use rusqlite::{params_from_iter, Connection, Result as SqliteResult, Row};
|
use rusqlite::{params_from_iter, Connection, Result as SqliteResult, Row};
|
||||||
use std::sync::{Arc, Mutex};
|
use std::sync::{Arc, Mutex};
|
||||||
@@ -128,9 +129,10 @@ impl DatabaseService for RusqliteService {
|
|||||||
async fn execute(&self, query: Query) -> DbResult<usize> {
|
async fn execute(&self, query: Query) -> DbResult<usize> {
|
||||||
let conn = Arc::clone(&self.conn);
|
let conn = Arc::clone(&self.conn);
|
||||||
tokio::task::spawn_blocking(move || {
|
tokio::task::spawn_blocking(move || {
|
||||||
let conn = conn
|
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
|
||||||
.lock()
|
// panic under the guard would otherwise poison it, failing every
|
||||||
.map_err(|e| format!("Failed to lock connection: {}", e))?;
|
// later query with "poisoned lock" until the process restarts.
|
||||||
|
let conn = conn.lock_safe();
|
||||||
execute_query(&conn, query)
|
execute_query(&conn, query)
|
||||||
})
|
})
|
||||||
.await
|
.await
|
||||||
@@ -141,9 +143,10 @@ impl DatabaseService for RusqliteService {
|
|||||||
let conn = Arc::clone(&self.conn);
|
let conn = Arc::clone(&self.conn);
|
||||||
let sql = sql.to_string();
|
let sql = sql.to_string();
|
||||||
tokio::task::spawn_blocking(move || {
|
tokio::task::spawn_blocking(move || {
|
||||||
let conn = conn
|
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
|
||||||
.lock()
|
// panic under the guard would otherwise poison it, failing every
|
||||||
.map_err(|e| format!("Failed to lock connection: {}", e))?;
|
// later query with "poisoned lock" until the process restarts.
|
||||||
|
let conn = conn.lock_safe();
|
||||||
conn.execute_batch(&sql)
|
conn.execute_batch(&sql)
|
||||||
.map_err(|e| format!("Execute batch failed: {}", e))
|
.map_err(|e| format!("Execute batch failed: {}", e))
|
||||||
})
|
})
|
||||||
@@ -158,9 +161,10 @@ impl DatabaseService for RusqliteService {
|
|||||||
{
|
{
|
||||||
let conn = Arc::clone(&self.conn);
|
let conn = Arc::clone(&self.conn);
|
||||||
tokio::task::spawn_blocking(move || {
|
tokio::task::spawn_blocking(move || {
|
||||||
let conn = conn
|
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
|
||||||
.lock()
|
// panic under the guard would otherwise poison it, failing every
|
||||||
.map_err(|e| format!("Failed to lock connection: {}", e))?;
|
// later query with "poisoned lock" until the process restarts.
|
||||||
|
let conn = conn.lock_safe();
|
||||||
query_one(&conn, query, mapper)
|
query_one(&conn, query, mapper)
|
||||||
})
|
})
|
||||||
.await
|
.await
|
||||||
@@ -174,9 +178,10 @@ impl DatabaseService for RusqliteService {
|
|||||||
{
|
{
|
||||||
let conn = Arc::clone(&self.conn);
|
let conn = Arc::clone(&self.conn);
|
||||||
tokio::task::spawn_blocking(move || {
|
tokio::task::spawn_blocking(move || {
|
||||||
let conn = conn
|
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
|
||||||
.lock()
|
// panic under the guard would otherwise poison it, failing every
|
||||||
.map_err(|e| format!("Failed to lock connection: {}", e))?;
|
// later query with "poisoned lock" until the process restarts.
|
||||||
|
let conn = conn.lock_safe();
|
||||||
query_optional(&conn, query, mapper)
|
query_optional(&conn, query, mapper)
|
||||||
})
|
})
|
||||||
.await
|
.await
|
||||||
@@ -190,9 +195,10 @@ impl DatabaseService for RusqliteService {
|
|||||||
{
|
{
|
||||||
let conn = Arc::clone(&self.conn);
|
let conn = Arc::clone(&self.conn);
|
||||||
tokio::task::spawn_blocking(move || {
|
tokio::task::spawn_blocking(move || {
|
||||||
let conn = conn
|
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
|
||||||
.lock()
|
// panic under the guard would otherwise poison it, failing every
|
||||||
.map_err(|e| format!("Failed to lock connection: {}", e))?;
|
// later query with "poisoned lock" until the process restarts.
|
||||||
|
let conn = conn.lock_safe();
|
||||||
query_many(&conn, query, mapper)
|
query_many(&conn, query, mapper)
|
||||||
})
|
})
|
||||||
.await
|
.await
|
||||||
@@ -206,9 +212,10 @@ impl DatabaseService for RusqliteService {
|
|||||||
{
|
{
|
||||||
let conn = Arc::clone(&self.conn);
|
let conn = Arc::clone(&self.conn);
|
||||||
tokio::task::spawn_blocking(move || {
|
tokio::task::spawn_blocking(move || {
|
||||||
let conn = conn
|
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
|
||||||
.lock()
|
// panic under the guard would otherwise poison it, failing every
|
||||||
.map_err(|e| format!("Failed to lock connection: {}", e))?;
|
// later query with "poisoned lock" until the process restarts.
|
||||||
|
let conn = conn.lock_safe();
|
||||||
|
|
||||||
conn.execute("BEGIN TRANSACTION", [])
|
conn.execute("BEGIN TRANSACTION", [])
|
||||||
.map_err(|e| format!("Failed to begin transaction: {}", e))?;
|
.map_err(|e| format!("Failed to begin transaction: {}", e))?;
|
||||||
@@ -236,9 +243,10 @@ impl DatabaseService for RusqliteService {
|
|||||||
async fn last_insert_rowid(&self) -> DbResult<i64> {
|
async fn last_insert_rowid(&self) -> DbResult<i64> {
|
||||||
let conn = Arc::clone(&self.conn);
|
let conn = Arc::clone(&self.conn);
|
||||||
tokio::task::spawn_blocking(move || {
|
tokio::task::spawn_blocking(move || {
|
||||||
let conn = conn
|
// `lock_safe`, not `lock`: this is the busiest lock in the app and a
|
||||||
.lock()
|
// panic under the guard would otherwise poison it, failing every
|
||||||
.map_err(|e| format!("Failed to lock connection: {}", e))?;
|
// later query with "poisoned lock" until the process restarts.
|
||||||
|
let conn = conn.lock_safe();
|
||||||
Ok(conn.last_insert_rowid())
|
Ok(conn.last_insert_rowid())
|
||||||
})
|
})
|
||||||
.await
|
.await
|
||||||
@@ -410,4 +418,48 @@ mod tests {
|
|||||||
let count: i32 = service.query_one(query, |row| row.get(0)).await.unwrap();
|
let count: i32 = service.query_one(query, |row| row.get(0)).await.unwrap();
|
||||||
assert_eq!(count, 2);
|
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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user