From 08733bf74d40fab3620ddfa81b39281584816542 Mon Sep 17 00:00:00 2001 From: Gregory Schier Date: Thu, 10 Sep 2026 17:04:50 -0700 Subject: [PATCH] feat(models): raise the pool ceiling and report its state when acquisition is slow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes that go together, because the first is untestable without the second. `max_size` goes 20 → 50. The comment justifying 20 was out of date: it cited the 256 open-file soft limit macOS gives GUI apps, but `run()` has since called `rlimit::increase_nofile_limit(10240)` before any pool is opened, so the ceiling it was sized against is gone. 50 WAL connections is roughly 150 descriptors against 10240, and only `min_idle` is opened up front — r2d2 creates the rest on demand — so the idle cost is unchanged. The comment now says what actually bounds the number: commands nest a couple of connections each, and since #542 every frontend command dispatches as its own task, so they arrive together. On its own that is a guess with no way to check itself. A pool that cannot hand out a connection fails with `Error(None)` and nothing else, whether every connection is checked out or none could be opened — unrelated problems with identical symptoms, and raising the ceiling only helps the first. So both pools now acquire through one helper that times the wait, warns past a second, and folds the pool's state into `Error::PoolTimeout`. The error a command returns now says which failure it was, which is what the next report needs to be worth anything. Blob pool sizing is untouched; nothing points at it. Co-Authored-By: Claude Opus 5 --- crates/common/yaak-database/src/lib.rs | 2 +- crates/common/yaak-database/src/pool.rs | 27 ++++++++++++++ crates/yaak-models/src/blob_manager.rs | 2 +- crates/yaak-models/src/error.rs | 12 +++++++ crates/yaak-models/src/lib.rs | 13 ++++--- crates/yaak-models/src/pool.rs | 48 +++++++++++++++++++++++++ crates/yaak-models/src/query_manager.rs | 14 +++++--- 7 files changed, 107 insertions(+), 11 deletions(-) create mode 100644 crates/yaak-models/src/pool.rs diff --git a/crates/common/yaak-database/src/lib.rs b/crates/common/yaak-database/src/lib.rs index da45ac08c..cd96f9595 100644 --- a/crates/common/yaak-database/src/lib.rs +++ b/crates/common/yaak-database/src/lib.rs @@ -12,7 +12,7 @@ pub use connection_or_tx::ConnectionOrTx; pub use db_context::DbContext; pub use error::{Error, Result}; pub use migrate::run_migrations; -pub use pool::{PoolError, SqliteConn, SqlitePool}; +pub use pool::{PoolError, PoolStatus, SqliteConn, SqlitePool, status as pool_status}; pub use traits::{UpsertModelInfo, upsert_date}; pub use update_source::{ModelChangeEvent, UpdateSource}; pub use util::{generate_id, generate_id_of_length, generate_prefixed_id}; diff --git a/crates/common/yaak-database/src/pool.rs b/crates/common/yaak-database/src/pool.rs index 863087da8..77cdfd12e 100644 --- a/crates/common/yaak-database/src/pool.rs +++ b/crates/common/yaak-database/src/pool.rs @@ -23,17 +23,39 @@ //! differently (here SQLite refuses the inner `BEGIN`; natively the inner //! connection blocks on `busy_timeout` and then fails). +/// What a pool looks like from outside at one moment. Enough to tell "everything is +/// checked out" apart from "nothing could be opened", which are unrelated problems that +/// both surface as an acquire timeout. +#[derive(Debug, Clone, Copy)] +pub struct PoolStatus { + pub connections: u32, + pub idle: u32, +} + +impl std::fmt::Display for PoolStatus { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{} open, {} idle", self.connections, self.idle) + } +} + #[cfg(not(target_arch = "wasm32"))] mod imp { + use super::PoolStatus; use r2d2_sqlite::SqliteConnectionManager; pub type SqlitePool = r2d2::Pool; pub type SqliteConn = r2d2::PooledConnection; pub type PoolError = r2d2::Error; + + pub fn status(pool: &SqlitePool) -> PoolStatus { + let state = pool.state(); + PoolStatus { connections: state.connections, idle: state.idle_connections } + } } #[cfg(target_arch = "wasm32")] mod imp { + use super::PoolStatus; use rusqlite::Connection; use std::ops::Deref; use std::rc::Rc; @@ -66,6 +88,11 @@ mod imp { #[derive(Debug, thiserror::Error)] pub enum PoolError {} + /// The one connection, which is never checked out and never unavailable. + pub fn status(_pool: &SqlitePool) -> PoolStatus { + PoolStatus { connections: 1, idle: 1 } + } + #[derive(Debug)] pub struct SqliteConn(Rc); diff --git a/crates/yaak-models/src/blob_manager.rs b/crates/yaak-models/src/blob_manager.rs index 29f607d53..7cbcbd341 100644 --- a/crates/yaak-models/src/blob_manager.rs +++ b/crates/yaak-models/src/blob_manager.rs @@ -37,7 +37,7 @@ impl BlobManager { } pub fn connect(&self) -> Result { - let conn = self.pool.get()?; + let conn = crate::pool::acquire(&self.pool, "blob")?; Ok(BlobContext { conn }) } } diff --git a/crates/yaak-models/src/error.rs b/crates/yaak-models/src/error.rs index 78c861f97..b035cdcee 100644 --- a/crates/yaak-models/src/error.rs +++ b/crates/yaak-models/src/error.rs @@ -9,6 +9,18 @@ pub enum Error { #[error("SQL Pool error: {0}")] SqlPoolError(#[from] yaak_database::PoolError), + #[error( + "Timed out after {waited_ms}ms waiting for a {what} database connection ({connections} open, {idle} idle)" + )] + PoolTimeout { + what: &'static str, + waited_ms: u128, + connections: u32, + idle: u32, + #[source] + source: yaak_database::PoolError, + }, + #[error("Database error: {0}")] Database(String), diff --git a/crates/yaak-models/src/lib.rs b/crates/yaak-models/src/lib.rs index ee6c79296..82572631b 100644 --- a/crates/yaak-models/src/lib.rs +++ b/crates/yaak-models/src/lib.rs @@ -17,6 +17,7 @@ pub mod migrate; pub mod models; pub mod models_ops; pub mod path_placeholders; +mod pool; pub mod queries; pub mod query_manager; pub mod render; @@ -108,12 +109,14 @@ pub fn init_standalone( let db_path = db_path.as_ref(); let blob_path = blob_path.as_ref(); - // Main database pool. Sized for concurrent in-flight queries, not concurrent app - // features — connections are held per-statement, so even heavy fan-out (e.g. many - // gRPC streams) only needs a handful at once. Keep max_size modest: WAL connections - // hold ~3 file descriptors each, and macOS GUI apps get a 256 fd soft limit. + // Main database pool. Sized for concurrent in-flight commands, not concurrent app + // features: connections are held per-statement, but one command can nest a couple of + // them, and since every frontend command dispatches as its own task they all land at + // once. Only `min_idle` is paid up front — r2d2 opens the rest on demand — and the + // app raises its open-file limit to 10240 at startup, so the old 256 fd ceiling that + // kept this small no longer applies. info!("Initializing app database {db_path:?}"); - let pool = open::file_pool(db_path, 20, 2)?; + let pool = open::file_pool(db_path, 50, 2)?; migrate_db(&pool)?; info!("Initializing blobs database {blob_path:?}"); diff --git a/crates/yaak-models/src/pool.rs b/crates/yaak-models/src/pool.rs new file mode 100644 index 000000000..68219574c --- /dev/null +++ b/crates/yaak-models/src/pool.rs @@ -0,0 +1,48 @@ +//! One way in and out of a connection pool, so every acquisition is timed the same way. +//! +//! A pool that cannot hand out a connection fails with a timeout and nothing else: r2d2 +//! reports `Error(None)` whether all the connections are checked out or none could be +//! opened in the first place. Those have nothing to do with each other, so the state of +//! the pool at the moment it gave up rides along in the error and in the log line. + +use crate::error::{Error, Result}; +use log::warn; +use std::time::{Duration, Instant}; +use yaak_database::{SqliteConn, SqlitePool, pool_status}; + +/// Long enough that a healthy app never logs, short enough to catch a pool going bad +/// well before the acquire timeout turns it into an error. +const SLOW_ACQUIRE: Duration = Duration::from_secs(1); + +pub(crate) fn acquire(pool: &SqlitePool, what: &'static str) -> Result { + let started = Instant::now(); + let result = pool.get(); + let waited = started.elapsed(); + + match result { + Ok(conn) => { + if waited >= SLOW_ACQUIRE { + warn!( + "Waited {}ms for a {what} connection ({})", + waited.as_millis(), + pool_status(pool) + ); + } + Ok(conn) + } + Err(source) => { + let status = pool_status(pool); + warn!( + "Gave up after {}ms waiting for a {what} connection ({status})", + waited.as_millis() + ); + Err(Error::PoolTimeout { + what, + waited_ms: waited.as_millis(), + connections: status.connections, + idle: status.idle, + source, + }) + } + } +} diff --git a/crates/yaak-models/src/query_manager.rs b/crates/yaak-models/src/query_manager.rs index fb6e7b242..d1842fa01 100644 --- a/crates/yaak-models/src/query_manager.rs +++ b/crates/yaak-models/src/query_manager.rs @@ -20,7 +20,7 @@ impl QueryManager { } pub fn connect(&self) -> crate::error::Result> { - let conn = self.pool.get()?; + let conn = crate::pool::acquire(&self.pool, "model")?; let ctx = DbContext::new(ConnectionOrTx::Connection(conn)); Ok(ClientDb::new(ctx, self.events_tx.clone())) } @@ -32,7 +32,7 @@ impl QueryManager { where E: From, { - let conn = self.pool.get().map_err(crate::error::Error::from)?; + let conn = crate::pool::acquire(&self.pool, "model")?; // `new_unchecked` takes `&Connection`; see yaak_database::pool for why // the pool never hands out `&mut`. let tx = Transaction::new_unchecked(&conn, TransactionBehavior::Immediate) @@ -95,7 +95,13 @@ mod tests { match qm.connect() { Ok(_) => panic!("pool should be exhausted"), - Err(e) => assert!(matches!(e, Error::SqlPoolError(_)), "got {e:?}"), + Err(e) => { + // The state of the pool rides along, so a report says which failure it was + assert!( + matches!(e, Error::PoolTimeout { connections: 1, idle: 0, .. }), + "got {e:?}" + ); + } } let err = qm @@ -104,7 +110,7 @@ mod tests { Ok(()) }) .expect_err("pool is exhausted"); - assert!(matches!(err, Error::SqlPoolError(_)), "got {err:?}"); + assert!(matches!(err, Error::PoolTimeout { .. }), "got {err:?}"); drop(held); qm.connect().expect("pool recovers once the connection is back");