mirror of
https://github.com/mountain-loop/yaak.git
synced 2026-11-13 12:13:42 +01:00
feat(models): raise the pool ceiling and report its state when acquisition is slow
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
1655aaa507
commit
08733bf74d
7 files changed
+107
-11
No files matched your search
@@ -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};
|
||||
|
||||
@@ -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<SqliteConnectionManager>;
|
||||
pub type SqliteConn = r2d2::PooledConnection<SqliteConnectionManager>;
|
||||
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<Connection>);
|
||||
|
||||
|
||||
@@ -37,7 +37,7 @@ impl BlobManager {
|
||||
}
|
||||
|
||||
pub fn connect(&self) -> Result<BlobContext> {
|
||||
let conn = self.pool.get()?;
|
||||
let conn = crate::pool::acquire(&self.pool, "blob")?;
|
||||
Ok(BlobContext { conn })
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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),
|
||||
|
||||
|
||||
@@ -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:?}");
|
||||
|
||||
@@ -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<SqliteConn> {
|
||||
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,
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -20,7 +20,7 @@ impl QueryManager {
|
||||
}
|
||||
|
||||
pub fn connect(&self) -> crate::error::Result<ClientDb<'_>> {
|
||||
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<crate::error::Error>,
|
||||
{
|
||||
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");
|
||||
|
||||
Reference in new issue
Block a user