From cf073e1c5c7a20459fad5220080228d50d368b58 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Wed, 3 Jun 2026 19:09:10 -0700 Subject: [PATCH 01/11] perf(api): bound per-request DB query fan-out to prevent pool exhaustion List and detail handlers assembled their DTOs by fanning out many related-table queries through tokio::join! (up to 14 for the full series response). Each arm acquired its own pool connection simultaneously, so a single request could hold most of the pool at once. Under concurrent load (e.g. many open tabs) this saturated the connection pool and made acquire() block for seconds, especially on SQLite where the pool is small. Add a shared-semaphore gate (db_batch::with_permit / fan_out_limiter) and wrap each join arm so at most a fixed number of queries hold a connection at a time; the rest park on the semaphore without a connection. A semaphore is used rather than a homogeneous stream because the arms return distinct types and must keep their positional bindings. Applied to the series, books, and bulk handlers' fan-out sites. The bound is currently a module constant; making it per-backend configurable is left for a follow-up. Includes unit tests for bound enforcement and result ordering. --- crates/codex-api/src/db_batch.rs | 102 ++++++++++ crates/codex-api/src/lib.rs | 1 + .../codex-api/src/routes/v1/handlers/books.rs | 50 +++-- .../codex-api/src/routes/v1/handlers/bulk.rs | 45 ++++- .../src/routes/v1/handlers/series.rs | 189 ++++++++++++++---- 5 files changed, 326 insertions(+), 61 deletions(-) create mode 100644 crates/codex-api/src/db_batch.rs diff --git a/crates/codex-api/src/db_batch.rs b/crates/codex-api/src/db_batch.rs new file mode 100644 index 00000000..16428463 --- /dev/null +++ b/crates/codex-api/src/db_batch.rs @@ -0,0 +1,102 @@ +//! Bounded fan-out for per-request batched reads. +//! +//! List endpoints enrich their rows from many related tables. Running those +//! queries with an unbounded `tokio::join!` lets a single request hold one +//! pool connection *per query* simultaneously (14 for the full series DTO). +//! Under concurrent load that exhausts the connection pool — acutely on SQLite, +//! whose pool is small — so requests block for seconds on `acquire()`. +//! +//! Gating each query on a shared [`Semaphore`] caps how many run, and therefore +//! how many connections one request holds, at once. The concurrency benefit is +//! preserved up to the bound while the pathological amplification is removed. +//! +//! Each `tokio::join!` arm keeps its own return type — this gates heterogeneous +//! futures (the arms return different map types) without the boxing/type-erasure +//! a homogeneous `buffer_unordered` stream would require. + +use std::future::Future; +use tokio::sync::Semaphore; + +/// Default per-request fan-out bound. +/// +/// Phase 3 of the DB-resilience work replaces this constant with a per-backend +/// configurable value (SQLite low, PostgreSQL higher). Until then a single +/// conservative default applies to every backend. +pub const DEFAULT_BATCH_FAN_OUT: usize = 4; + +/// Build a [`Semaphore`] that bounds concurrent batched reads to `bound`. +/// +/// `bound` is clamped to at least 1 so a misconfigured `0` cannot deadlock the +/// request (which would otherwise never acquire a permit). +pub fn fan_out_limiter(bound: usize) -> Semaphore { + Semaphore::new(bound.max(1)) +} + +/// Run `fut` once a permit is available, holding the permit for the whole query. +/// +/// Used to wrap each arm of a `tokio::join!` so the number of arms actively +/// executing (and thus pool connections held) never exceeds the `limiter`'s +/// bound. Arms beyond the bound are parked on `acquire()` holding no connection. +pub async fn with_permit(limiter: &Semaphore, fut: F) -> F::Output { + let _permit = limiter + .acquire() + .await + .expect("fan-out limiter semaphore is never closed"); + fut.await +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering::SeqCst}; + + #[test] + fn fan_out_limiter_clamps_zero_to_one() { + assert_eq!(fan_out_limiter(0).available_permits(), 1); + assert_eq!(fan_out_limiter(4).available_permits(), 4); + } + + /// `with_permit` must (a) return each arm's value in order and (b) never let + /// more than `bound` arms execute concurrently. + #[tokio::test] + async fn with_permit_bounds_concurrency_and_preserves_results() { + let limiter = fan_out_limiter(3); + let current = Arc::new(AtomicUsize::new(0)); + let max_seen = Arc::new(AtomicUsize::new(0)); + + let task = |i: usize| { + let current = current.clone(); + let max_seen = max_seen.clone(); + async move { + let now = current.fetch_add(1, SeqCst) + 1; + max_seen.fetch_max(now, SeqCst); + // Yield repeatedly so other arms get a chance to run; this is + // what would let concurrency exceed the bound if it were unbounded. + for _ in 0..8 { + tokio::task::yield_now().await; + } + current.fetch_sub(1, SeqCst); + i + } + }; + + let results = tokio::join!( + with_permit(&limiter, task(0)), + with_permit(&limiter, task(1)), + with_permit(&limiter, task(2)), + with_permit(&limiter, task(3)), + with_permit(&limiter, task(4)), + with_permit(&limiter, task(5)), + with_permit(&limiter, task(6)), + with_permit(&limiter, task(7)), + ); + + assert_eq!(results, (0, 1, 2, 3, 4, 5, 6, 7)); + assert!( + max_seen.load(SeqCst) <= 3, + "observed {} concurrent arms, expected <= 3", + max_seen.load(SeqCst) + ); + } +} diff --git a/crates/codex-api/src/lib.rs b/crates/codex-api/src/lib.rs index cde6c22e..5a01bbfd 100644 --- a/crates/codex-api/src/lib.rs +++ b/crates/codex-api/src/lib.rs @@ -1,3 +1,4 @@ +pub mod db_batch; pub mod docs; pub mod error; pub mod extractors; diff --git a/crates/codex-api/src/routes/v1/handlers/books.rs b/crates/codex-api/src/routes/v1/handlers/books.rs index ee30fc4f..9bda7ff5 100644 --- a/crates/codex-api/src/routes/v1/handlers/books.rs +++ b/crates/codex-api/src/routes/v1/handlers/books.rs @@ -206,11 +206,22 @@ pub async fn books_to_dtos( // Fan out one batched query per related table. Previous per-row loops // produced O(books) sequential DB roundtrips here. + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + use crate::db_batch::with_permit; let (series_metadata_result, book_metadata_result, libraries_result, progress_result) = tokio::join!( - SeriesMetadataRepository::get_by_series_ids(db, &series_ids), - BookMetadataRepository::get_by_book_ids(db, &book_ids), - LibraryRepository::get_by_ids(db, &library_ids), - ReadProgressRepository::get_for_user_books(db, user_id, &book_ids), + with_permit( + &limiter, + SeriesMetadataRepository::get_by_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + BookMetadataRepository::get_by_book_ids(db, &book_ids) + ), + with_permit(&limiter, LibraryRepository::get_by_ids(db, &library_ids)), + with_permit( + &limiter, + ReadProgressRepository::get_for_user_books(db, user_id, &book_ids) + ), ); let series_metadata_map = series_metadata_result @@ -337,12 +348,21 @@ pub async fn books_to_full_dtos_batched( .into_iter() .collect(); - // Fetch all related data in parallel + // Fetch all related data in parallel, bounded so a single request cannot + // hold one pool connection per query and exhaust the pool. + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + use crate::db_batch::with_permit; let (metadata_map, series_metadata_map, library_map, progress_map, genres_map, tags_map) = tokio::join!( - BookMetadataRepository::get_by_book_ids(db, &book_ids), - SeriesMetadataRepository::get_by_series_ids(db, &series_ids), - LibraryRepository::get_by_ids(db, &library_ids), - async { + with_permit( + &limiter, + BookMetadataRepository::get_by_book_ids(db, &book_ids) + ), + with_permit( + &limiter, + SeriesMetadataRepository::get_by_series_ids(db, &series_ids) + ), + with_permit(&limiter, LibraryRepository::get_by_ids(db, &library_ids)), + with_permit(&limiter, async { // Fetch read progress for all books let mut map = HashMap::new(); for book_id in &book_ids { @@ -353,9 +373,15 @@ pub async fn books_to_full_dtos_batched( } } Ok::<_, anyhow::Error>(map) - }, - GenreRepository::get_genres_for_book_ids(db, &book_ids), - TagRepository::get_tags_for_book_ids(db, &book_ids), + }), + with_permit( + &limiter, + GenreRepository::get_genres_for_book_ids(db, &book_ids) + ), + with_permit( + &limiter, + TagRepository::get_tags_for_book_ids(db, &book_ids) + ), ); // Handle errors diff --git a/crates/codex-api/src/routes/v1/handlers/bulk.rs b/crates/codex-api/src/routes/v1/handlers/bulk.rs index 976f7269..42edda7b 100644 --- a/crates/codex-api/src/routes/v1/handlers/bulk.rs +++ b/crates/codex-api/src/routes/v1/handlers/bulk.rs @@ -847,7 +847,10 @@ pub async fn bulk_reset_series_metadata( _ => continue, }; - // Clear all associated data + // Clear all associated data, bounded so this does not hold one pool + // connection per query at once. + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + use crate::db_batch::with_permit; let ( genres_res, tags_res, @@ -858,14 +861,38 @@ pub async fn bulk_reset_series_metadata( covers_res, sharing_res, ) = tokio::join!( - GenreRepository::set_genres_for_series(&state.db, *series_id, vec![]), - TagRepository::set_tags_for_series(&state.db, *series_id, vec![]), - AlternateTitleRepository::delete_all_for_series(&state.db, *series_id, None), - SeriesExternalIdRepository::delete_all_for_series(&state.db, *series_id), - ExternalRatingRepository::delete_all_for_series(&state.db, *series_id), - ExternalLinkRepository::delete_all_for_series(&state.db, *series_id), - SeriesCoversRepository::delete_by_series(&state.db, *series_id), - SharingTagRepository::set_tags_for_series(&state.db, *series_id, vec![]), + with_permit( + &limiter, + GenreRepository::set_genres_for_series(&state.db, *series_id, vec![]) + ), + with_permit( + &limiter, + TagRepository::set_tags_for_series(&state.db, *series_id, vec![]) + ), + with_permit( + &limiter, + AlternateTitleRepository::delete_all_for_series(&state.db, *series_id, None) + ), + with_permit( + &limiter, + SeriesExternalIdRepository::delete_all_for_series(&state.db, *series_id) + ), + with_permit( + &limiter, + ExternalRatingRepository::delete_all_for_series(&state.db, *series_id) + ), + with_permit( + &limiter, + ExternalLinkRepository::delete_all_for_series(&state.db, *series_id) + ), + with_permit( + &limiter, + SeriesCoversRepository::delete_by_series(&state.db, *series_id) + ), + with_permit( + &limiter, + SharingTagRepository::set_tags_for_series(&state.db, *series_id, vec![]) + ), ); // Log errors but continue with other series diff --git a/crates/codex-api/src/routes/v1/handlers/series.rs b/crates/codex-api/src/routes/v1/handlers/series.rs index e2c39f2c..95c5b3b9 100644 --- a/crates/codex-api/src/routes/v1/handlers/series.rs +++ b/crates/codex-api/src/routes/v1/handlers/series.rs @@ -259,6 +259,10 @@ async fn series_to_dtos_batched( .into_iter() .collect(); + // Bound how many of these related-table queries run at once so a single + // request cannot hold one pool connection per query and exhaust the pool. + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + use crate::db_batch::with_permit; let ( metadata_map, book_counts_map, @@ -270,21 +274,42 @@ async fn series_to_dtos_batched( tracking_map, ext_ids_map, ) = tokio::join!( - SeriesMetadataRepository::get_by_series_ids(db, &series_ids), - SeriesRepository::get_book_counts_for_series_ids(db, &series_ids), - SeriesRepository::get_book_classification_aggregates_for_series_ids(db, &series_ids), - async { + with_permit( + &limiter, + SeriesMetadataRepository::get_by_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesRepository::get_book_counts_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesRepository::get_book_classification_aggregates_for_series_ids(db, &series_ids) + ), + with_permit(&limiter, async { if let Some(uid) = user_id { BookRepository::count_unread_in_series_ids(db, &series_ids, uid).await } else { Ok(HashMap::new()) } - }, - SeriesCoversRepository::get_selected_for_series_ids(db, &series_ids), - SeriesCoversRepository::has_custom_cover_for_series_ids(db, &series_ids), - LibraryRepository::get_by_ids(db, &library_ids), - SeriesTrackingRepository::get_for_series_ids(db, &series_ids), - SeriesExternalIdRepository::get_for_series_ids(db, &series_ids), + }), + with_permit( + &limiter, + SeriesCoversRepository::get_selected_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesCoversRepository::has_custom_cover_for_series_ids(db, &series_ids) + ), + with_permit(&limiter, LibraryRepository::get_by_ids(db, &library_ids)), + with_permit( + &limiter, + SeriesTrackingRepository::get_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesExternalIdRepository::get_for_series_ids(db, &series_ids) + ), ); let metadata_map = @@ -408,7 +433,11 @@ async fn series_to_full_dtos_batched( .into_iter() .collect(); - // Fetch all related data in parallel using batched queries + // Fetch all related data using batched queries, but bound how many run at + // once so a single request cannot hold one pool connection per query and + // exhaust the pool under concurrent load. + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + use crate::db_batch::with_permit; let ( metadata_map, book_counts_map, @@ -425,26 +454,62 @@ async fn series_to_full_dtos_batched( ext_ids_map, tracking_map, ) = tokio::join!( - SeriesMetadataRepository::get_by_series_ids(db, &series_ids), - SeriesRepository::get_book_counts_for_series_ids(db, &series_ids), - SeriesRepository::get_book_classification_aggregates_for_series_ids(db, &series_ids), - async { + with_permit( + &limiter, + SeriesMetadataRepository::get_by_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesRepository::get_book_counts_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesRepository::get_book_classification_aggregates_for_series_ids(db, &series_ids) + ), + with_permit(&limiter, async { if let Some(uid) = user_id { BookRepository::count_unread_in_series_ids(db, &series_ids, uid).await } else { Ok(HashMap::new()) } - }, - SeriesCoversRepository::get_selected_for_series_ids(db, &series_ids), - SeriesCoversRepository::has_custom_cover_for_series_ids(db, &series_ids), - LibraryRepository::get_by_ids(db, &library_ids), - GenreRepository::get_genres_for_series_ids(db, &series_ids), - TagRepository::get_tags_for_series_ids(db, &series_ids), - AlternateTitleRepository::get_for_series_ids(db, &series_ids), - ExternalRatingRepository::get_for_series_ids(db, &series_ids), - ExternalLinkRepository::get_for_series_ids(db, &series_ids), - SeriesExternalIdRepository::get_for_series_ids(db, &series_ids), - SeriesTrackingRepository::get_for_series_ids(db, &series_ids), + }), + with_permit( + &limiter, + SeriesCoversRepository::get_selected_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesCoversRepository::has_custom_cover_for_series_ids(db, &series_ids) + ), + with_permit(&limiter, LibraryRepository::get_by_ids(db, &library_ids)), + with_permit( + &limiter, + GenreRepository::get_genres_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + TagRepository::get_tags_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + AlternateTitleRepository::get_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + ExternalRatingRepository::get_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + ExternalLinkRepository::get_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesExternalIdRepository::get_for_series_ids(db, &series_ids) + ), + with_permit( + &limiter, + SeriesTrackingRepository::get_for_series_ids(db, &series_ids) + ), ); // Handle errors @@ -3044,7 +3109,10 @@ pub async fn reset_series_metadata( .map_err(|e| ApiError::Internal(format!("Failed to fetch series: {}", e)))? .ok_or_else(|| ApiError::NotFound("Series not found".to_string()))?; - // Clear all associated data in parallel + // Clear all associated data in parallel, bounded so this does not hold one + // pool connection per query at once. + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + use crate::db_batch::with_permit; let ( genres_res, tags_res, @@ -3055,14 +3123,38 @@ pub async fn reset_series_metadata( covers_res, sharing_tags_res, ) = tokio::join!( - GenreRepository::set_genres_for_series(&state.db, series_id, vec![]), - TagRepository::set_tags_for_series(&state.db, series_id, vec![]), - AlternateTitleRepository::delete_all_for_series(&state.db, series_id, None), - SeriesExternalIdRepository::delete_all_for_series(&state.db, series_id), - ExternalRatingRepository::delete_all_for_series(&state.db, series_id), - ExternalLinkRepository::delete_all_for_series(&state.db, series_id), - SeriesCoversRepository::delete_by_series(&state.db, series_id), - SharingTagRepository::set_tags_for_series(&state.db, series_id, vec![]), + with_permit( + &limiter, + GenreRepository::set_genres_for_series(&state.db, series_id, vec![]) + ), + with_permit( + &limiter, + TagRepository::set_tags_for_series(&state.db, series_id, vec![]) + ), + with_permit( + &limiter, + AlternateTitleRepository::delete_all_for_series(&state.db, series_id, None) + ), + with_permit( + &limiter, + SeriesExternalIdRepository::delete_all_for_series(&state.db, series_id) + ), + with_permit( + &limiter, + ExternalRatingRepository::delete_all_for_series(&state.db, series_id) + ), + with_permit( + &limiter, + ExternalLinkRepository::delete_all_for_series(&state.db, series_id) + ), + with_permit( + &limiter, + SeriesCoversRepository::delete_by_series(&state.db, series_id) + ), + with_permit( + &limiter, + SharingTagRepository::set_tags_for_series(&state.db, series_id, vec![]) + ), ); // Check for errors @@ -3384,12 +3476,29 @@ pub async fn get_series_metadata( .ok_or_else(|| ApiError::Internal("Series metadata not found".to_string()))?; // Fetch all related data in parallel + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + use crate::db_batch::with_permit; let (genres_result, tags_result, alt_titles_result, ext_ratings_result, ext_links_result) = tokio::join!( - GenreRepository::get_genres_for_series(&state.db, series_id), - TagRepository::get_tags_for_series(&state.db, series_id), - AlternateTitleRepository::get_for_series(&state.db, series_id), - ExternalRatingRepository::get_for_series(&state.db, series_id), - ExternalLinkRepository::get_for_series(&state.db, series_id), + with_permit( + &limiter, + GenreRepository::get_genres_for_series(&state.db, series_id) + ), + with_permit( + &limiter, + TagRepository::get_tags_for_series(&state.db, series_id) + ), + with_permit( + &limiter, + AlternateTitleRepository::get_for_series(&state.db, series_id) + ), + with_permit( + &limiter, + ExternalRatingRepository::get_for_series(&state.db, series_id) + ), + with_permit( + &limiter, + ExternalLinkRepository::get_for_series(&state.db, series_id) + ), ); let genres = From 8a8ee7af3bf443da72eb58f0462177ed5886bfa3 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Wed, 3 Jun 2026 19:24:35 -0700 Subject: [PATCH 02/11] perf(db): isolate background-task DB pool from the API pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task workers, the scheduler, and the inventory poller shared the API's connection pool, so heavy or bursty background work (scans, analysis, thumbnail generation) could hold every connection and starve interactive requests — acute on SQLite, whose pool is small. Add Database::new_background(), which builds a separate, smaller pool over the same database (cloning the config with an overridden max_connections, reusing the existing connect/pragma setup, and running no migrations). In serve, route the in-process background subsystems to this pool while the API and request-path services keep the primary pool. The background pool is only created when workers run in-process; web-only deployments and the dedicated worker process keep a single pool. On Postgres the pool is additive to the API pool, so the server's max_connections must accommodate the total (documented; startup validation to follow). SQLite's busy_timeout default already makes writers wait rather than error. Includes a test verifying the background pool is independent but targets the same database. --- crates/codex-db/src/connection.rs | 90 +++++++++++++++++++++++++++++++ src/commands/serve.rs | 44 ++++++++++++--- 2 files changed, 126 insertions(+), 8 deletions(-) diff --git a/crates/codex-db/src/connection.rs b/crates/codex-db/src/connection.rs index 34844416..f64fb658 100644 --- a/crates/codex-db/src/connection.rs +++ b/crates/codex-db/src/connection.rs @@ -174,6 +174,38 @@ impl Database { Ok(Self { conn }) } + /// Create a separate connection pool for background work. + /// + /// Task workers, the scheduler, and pollers do heavy or bursty database + /// work that, on a shared pool, can hold every connection and starve the + /// interactive API requests (acute on SQLite, whose pool is small). Giving + /// background work its own, smaller pool keeps a scan storm from draining + /// the pool that serves HTTP requests. + /// + /// The pool targets the *same* database (SQLite file / Postgres server) as + /// [`Database::new`], reusing identical connect and pragma setup, but with + /// an independent pool capped at `max_connections`. It deliberately does + /// **not** run migrations — the primary connection owns schema setup. + pub async fn new_background(config: &DatabaseConfig, max_connections: u32) -> Result { + let mut cfg = config.clone(); + match cfg.db_type { + DatabaseType::SQLite => { + if let Some(sqlite) = cfg.sqlite.as_mut() { + sqlite.max_connections = max_connections; + sqlite.min_connections = sqlite.min_connections.min(max_connections); + } + } + DatabaseType::Postgres => { + if let Some(postgres) = cfg.postgres.as_mut() { + postgres.max_connections = max_connections; + postgres.min_connections = postgres.min_connections.min(max_connections); + } + } + } + info!("Initializing separate background connection pool..."); + Self::new(&cfg).await + } + /// Run database migrations /// /// This will apply all pending migrations. Migrator::up() is idempotent, @@ -494,6 +526,64 @@ mod tests { db.close().await; } + #[tokio::test] + async fn test_new_background_is_independent_pool_over_same_db() { + let temp_dir = TempDir::new().unwrap(); + let db_path = temp_dir.path().join("test.db"); + + let config = DatabaseConfig { + db_type: DatabaseType::SQLite, + postgres: None, + sqlite: Some(SQLiteConfig { + path: db_path.to_str().unwrap().to_string(), + pragmas: None, + max_connections: 16, + ..SQLiteConfig::default() + }), + }; + + // Primary pool owns schema setup. + let primary = Database::new(&config).await.unwrap(); + primary.run_migrations().await.unwrap(); + + // Background pool: separate, smaller pool over the same file. No + // migrations run here; it must still see the primary's schema. + let background = Database::new_background(&config, 4).await.unwrap(); + assert!(background.health_check().await.is_ok()); + + // A write through the primary pool is visible through the background + // pool — proving they target the same database, not two pools over two + // independent `:memory:` databases. + let backend = primary.sea_orm_connection().get_database_backend(); + primary + .sea_orm_connection() + .execute(sea_orm::Statement::from_string( + backend, + "CREATE TABLE pool_probe (id INTEGER PRIMARY KEY)".to_string(), + )) + .await + .unwrap(); + background + .sea_orm_connection() + .execute(sea_orm::Statement::from_string( + backend, + "INSERT INTO pool_probe (id) VALUES (1)".to_string(), + )) + .await + .unwrap(); + let row = primary + .sea_orm_connection() + .query_one(sea_orm::Statement::from_string( + backend, + "SELECT COUNT(*) AS n FROM pool_probe".to_string(), + )) + .await + .unwrap() + .unwrap(); + let count: i64 = row.try_get("", "n").unwrap(); + assert_eq!(count, 1); + } + #[tokio::test] #[ignore] // Requires PostgreSQL server async fn test_database_new_postgres() { diff --git a/src/commands/serve.rs b/src/commands/serve.rs index 98587990..858b4ec3 100644 --- a/src/commands/serve.rs +++ b/src/commands/serve.rs @@ -99,6 +99,36 @@ pub async fn serve_command(config_path: PathBuf) -> anyhow::Result<()> { .map(|v| v.eq_ignore_ascii_case("true") || v == "1") .unwrap_or(false); + // Background connection pool isolation. + // + // Task workers, the scheduler, and pollers do heavy/bursty DB work that, on + // a shared pool, can hold every connection and starve interactive API + // requests (acute on SQLite, whose pool is small). When workers run in this + // process, give that background work its own smaller pool so a scan storm + // cannot drain the pool serving HTTP requests. Web-only deployments + // (workers disabled) keep a single pool — there is no in-process background + // work to isolate. + // + // NOTE: on Postgres this pool is additive to the API pool, so the server's + // max_connections must accommodate the total. The common multi-pod Postgres + // deployment runs serve with CODEX_DISABLE_WORKERS=true, so no extra pool is + // created there. Phase 3 makes the size configurable and validates the total. + let background_db = if disable_workers { + None + } else { + let background_max = match config.database.db_type { + DatabaseType::SQLite => 4, + DatabaseType::Postgres => 16, + }; + Some(codex_db::Database::new_background(&config.database, background_max).await?) + }; + // Connection handed to background subsystems: the dedicated pool when one + // exists, otherwise the primary pool (single-pool/web-only mode). + let background_conn = background_db + .as_ref() + .map(|d| d.sea_orm_connection().clone()) + .unwrap_or_else(|| db.sea_orm_connection().clone()); + // Initialize thumbnail service (needed for both workers, API handlers, and scheduler) let thumbnail_service = Arc::new(codex_services::ThumbnailService::new(config.files.clone())); info!( @@ -110,11 +140,8 @@ pub async fn serve_command(config_path: PathBuf) -> anyhow::Result<()> { info!("Initializing job scheduler..."); let scheduler: Arc> = Arc::new(tokio::sync::Mutex::new( - codex_scheduler::Scheduler::new( - db.sea_orm_connection().clone(), - &config.scheduler.timezone, - ) - .await?, + codex_scheduler::Scheduler::new(background_conn.clone(), &config.scheduler.timezone) + .await?, )); scheduler.lock().await.start().await?; info!("Job scheduler started successfully"); @@ -141,7 +168,7 @@ pub async fn serve_command(config_path: PathBuf) -> anyhow::Result<()> { // gauges have current values. Cheap: five `COUNT(*)` queries. The poller // exits as soon as the cancellation token fires. let inventory_poller_handle = codex_api::observability::inventory::spawn_poller( - Arc::new(db.sea_orm_connection().clone()), + Arc::new(background_conn.clone()), std::time::Duration::from_secs(30), background_task_cancel.clone(), ); @@ -405,9 +432,10 @@ pub async fn serve_command(config_path: PathBuf) -> anyhow::Result<()> { info!("Starting {} task queue worker(s)...", worker_count); - // Spawn multiple workers for parallel task processing + // Spawn multiple workers for parallel task processing on the isolated + // background pool so heavy task DB work cannot starve API requests. let (handles, channels) = spawn_workers( - db.sea_orm_connection(), + &background_conn, worker_count, event_broadcaster.clone(), settings_service.clone(), From 74690a4f54b033a833f3004571a9f210f47ead67 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Wed, 3 Jun 2026 19:42:33 -0700 Subject: [PATCH 03/11] feat(config): make DB fan-out bound and background pool size configurable Expose per-backend batch_fan_out and background_max_connections settings (env-overridable) so the per-request query fan-out cap and the background work pool can be tuned without recompiling. Bump the SQLite max_connections default to 64 for read headroom under WAL. The fan-out bound is resolved once at startup and read by the list and detail handlers (replacing the hardcoded default); serve reads the background pool size from config. On Postgres, warn at startup if the API and background pools together exceed the server's max_connections, since the background pool is additive when workers run in-process. Update the sample SQLite and Docker configs and the configuration and performance docs, including the API-vs-background pool split and the SQLite write-serialization caveat. --- config/config.docker.yaml | 10 ++ config/config.sqlite.yaml | 9 +- crates/codex-api/src/db_batch.rs | 38 ++++- .../codex-api/src/routes/v1/handlers/books.rs | 4 +- .../codex-api/src/routes/v1/handlers/bulk.rs | 2 +- .../src/routes/v1/handlers/series.rs | 8 +- crates/codex-config/src/types.rs | 132 +++++++++++++++++- docs/docs/configuration.md | 18 ++- docs/docs/deployment/performance.md | 15 +- src/commands/serve.rs | 70 +++++++++- 10 files changed, 282 insertions(+), 24 deletions(-) diff --git a/config/config.docker.yaml b/config/config.docker.yaml index dcb034ae..c7d07473 100644 --- a/config/config.docker.yaml +++ b/config/config.docker.yaml @@ -17,6 +17,16 @@ database: # Connection Pool Settings (optional - defaults shown) # max_connections: 100 # min_connections: 5 + # Per-request query fan-out bound: how many related-table queries a single + # list/detail request runs at once (caps connections held per request). + # Higher than SQLite since Postgres runs queries truly concurrently. + # batch_fan_out: 8 + # Separate pool for in-process background work (task workers, scheduler, + # pollers). NOTE: additive to max_connections; ensure the Postgres server's + # max_connections covers the total when workers run in the same process. + # Multi-pod deployments run serve with CODEX_DISABLE_WORKERS=true, so no + # extra pool is created there. + # background_max_connections: 16 # acquire_timeout_seconds: 30 # idle_timeout_seconds: 600 # max_lifetime_seconds: 3600 diff --git a/config/config.sqlite.yaml b/config/config.sqlite.yaml index 6aaaa146..5d7eff3c 100644 --- a/config/config.sqlite.yaml +++ b/config/config.sqlite.yaml @@ -15,8 +15,15 @@ database: synchronous: NORMAL # Note: foreign_keys is ALWAYS ON for data integrity (cannot be disabled) # Connection Pool Settings (optional - defaults shown) - # max_connections: 16 + # max_connections: 64 # min_connections: 2 + # Per-request query fan-out bound: how many related-table queries a single + # list/detail request runs at once (caps connections held per request). + # Kept low for SQLite (writes serialize; queries are cheap lookups). + # batch_fan_out: 4 + # Separate pool for in-process background work (task workers, scheduler, + # pollers) so heavy background queries cannot drain the API pool. + # background_max_connections: 4 # acquire_timeout_seconds: 30 # idle_timeout_seconds: 300 # max_lifetime_seconds: 1800 diff --git a/crates/codex-api/src/db_batch.rs b/crates/codex-api/src/db_batch.rs index 16428463..10d3d147 100644 --- a/crates/codex-api/src/db_batch.rs +++ b/crates/codex-api/src/db_batch.rs @@ -15,15 +15,34 @@ //! a homogeneous `buffer_unordered` stream would require. use std::future::Future; +use std::sync::OnceLock; use tokio::sync::Semaphore; -/// Default per-request fan-out bound. -/// -/// Phase 3 of the DB-resilience work replaces this constant with a per-backend -/// configurable value (SQLite low, PostgreSQL higher). Until then a single -/// conservative default applies to every backend. +/// Fallback per-request fan-out bound used until [`set_fan_out`] is called +/// (e.g. in tests that exercise handlers without going through `serve`). pub const DEFAULT_BATCH_FAN_OUT: usize = 4; +/// Process-wide resolved fan-out bound, set once at startup from the per-backend +/// database config. It is a process constant (the backend never changes at +/// runtime), so a global avoids threading the value through every converter and +/// call site. Unset → [`DEFAULT_BATCH_FAN_OUT`]. +static CONFIGURED_FAN_OUT: OnceLock = OnceLock::new(); + +/// Set the process-wide fan-out bound from configuration. Called once during +/// startup. Clamped to at least 1. Subsequent calls are ignored (the first +/// value wins), which keeps it stable across the process lifetime. +pub fn set_fan_out(bound: usize) { + let _ = CONFIGURED_FAN_OUT.set(bound.max(1)); +} + +/// The configured fan-out bound, or [`DEFAULT_BATCH_FAN_OUT`] if unset. +pub fn configured_fan_out() -> usize { + CONFIGURED_FAN_OUT + .get() + .copied() + .unwrap_or(DEFAULT_BATCH_FAN_OUT) +} + /// Build a [`Semaphore`] that bounds concurrent batched reads to `bound`. /// /// `bound` is clamped to at least 1 so a misconfigured `0` cannot deadlock the @@ -57,6 +76,15 @@ mod tests { assert_eq!(fan_out_limiter(4).available_permits(), 4); } + #[test] + fn configured_fan_out_is_always_positive() { + // Whether or not set_fan_out has run in this process, the bound must be + // >= 1 so a request can always acquire a permit. (We avoid calling + // set_fan_out here: CONFIGURED_FAN_OUT is process-global and a write + // would leak into other tests in this binary.) + assert!(configured_fan_out() >= 1); + } + /// `with_permit` must (a) return each arm's value in order and (b) never let /// more than `bound` arms execute concurrently. #[tokio::test] diff --git a/crates/codex-api/src/routes/v1/handlers/books.rs b/crates/codex-api/src/routes/v1/handlers/books.rs index 9bda7ff5..95c5381e 100644 --- a/crates/codex-api/src/routes/v1/handlers/books.rs +++ b/crates/codex-api/src/routes/v1/handlers/books.rs @@ -206,7 +206,7 @@ pub async fn books_to_dtos( // Fan out one batched query per related table. Previous per-row loops // produced O(books) sequential DB roundtrips here. - let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::configured_fan_out()); use crate::db_batch::with_permit; let (series_metadata_result, book_metadata_result, libraries_result, progress_result) = tokio::join!( with_permit( @@ -350,7 +350,7 @@ pub async fn books_to_full_dtos_batched( // Fetch all related data in parallel, bounded so a single request cannot // hold one pool connection per query and exhaust the pool. - let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::configured_fan_out()); use crate::db_batch::with_permit; let (metadata_map, series_metadata_map, library_map, progress_map, genres_map, tags_map) = tokio::join!( with_permit( diff --git a/crates/codex-api/src/routes/v1/handlers/bulk.rs b/crates/codex-api/src/routes/v1/handlers/bulk.rs index 42edda7b..73334fe1 100644 --- a/crates/codex-api/src/routes/v1/handlers/bulk.rs +++ b/crates/codex-api/src/routes/v1/handlers/bulk.rs @@ -849,7 +849,7 @@ pub async fn bulk_reset_series_metadata( // Clear all associated data, bounded so this does not hold one pool // connection per query at once. - let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::configured_fan_out()); use crate::db_batch::with_permit; let ( genres_res, diff --git a/crates/codex-api/src/routes/v1/handlers/series.rs b/crates/codex-api/src/routes/v1/handlers/series.rs index 95c5b3b9..666280f1 100644 --- a/crates/codex-api/src/routes/v1/handlers/series.rs +++ b/crates/codex-api/src/routes/v1/handlers/series.rs @@ -261,7 +261,7 @@ async fn series_to_dtos_batched( // Bound how many of these related-table queries run at once so a single // request cannot hold one pool connection per query and exhaust the pool. - let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::configured_fan_out()); use crate::db_batch::with_permit; let ( metadata_map, @@ -436,7 +436,7 @@ async fn series_to_full_dtos_batched( // Fetch all related data using batched queries, but bound how many run at // once so a single request cannot hold one pool connection per query and // exhaust the pool under concurrent load. - let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::configured_fan_out()); use crate::db_batch::with_permit; let ( metadata_map, @@ -3111,7 +3111,7 @@ pub async fn reset_series_metadata( // Clear all associated data in parallel, bounded so this does not hold one // pool connection per query at once. - let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::configured_fan_out()); use crate::db_batch::with_permit; let ( genres_res, @@ -3476,7 +3476,7 @@ pub async fn get_series_metadata( .ok_or_else(|| ApiError::Internal("Series metadata not found".to_string()))?; // Fetch all related data in parallel - let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::DEFAULT_BATCH_FAN_OUT); + let limiter = crate::db_batch::fan_out_limiter(crate::db_batch::configured_fan_out()); use crate::db_batch::with_permit; let (genres_result, tags_result, alt_titles_result, ext_ratings_result, ext_links_result) = tokio::join!( with_permit( diff --git a/crates/codex-config/src/types.rs b/crates/codex-config/src/types.rs index 7a2dcadd..a61d3ad7 100644 --- a/crates/codex-config/src/types.rs +++ b/crates/codex-config/src/types.rs @@ -501,6 +501,50 @@ impl DatabaseConfig { .unwrap_or(30), } } + + /// Per-request query fan-out bound for the current database type. + /// + /// Bounds how many related-table queries a single request runs at once, and + /// therefore how many pool connections it can hold. Clamped to at least 1. + pub fn batch_fan_out(&self) -> usize { + let raw = match self.db_type { + DatabaseType::Postgres => self.postgres.as_ref().map(|c| c.batch_fan_out).unwrap_or(8), + DatabaseType::SQLite => self.sqlite.as_ref().map(|c| c.batch_fan_out).unwrap_or(4), + }; + raw.max(1) + } + + /// Maximum connections for the separate background-work pool, by database type. + pub fn background_max_connections(&self) -> u32 { + match self.db_type { + DatabaseType::Postgres => self + .postgres + .as_ref() + .map(|c| c.background_max_connections) + .unwrap_or(16), + DatabaseType::SQLite => self + .sqlite + .as_ref() + .map(|c| c.background_max_connections) + .unwrap_or(4), + } + } + + /// Maximum connections for the primary (API) pool, by database type. + pub fn max_connections(&self) -> u32 { + match self.db_type { + DatabaseType::Postgres => self + .postgres + .as_ref() + .map(|c| c.max_connections) + .unwrap_or(100), + DatabaseType::SQLite => self + .sqlite + .as_ref() + .map(|c| c.max_connections) + .unwrap_or(64), + } + } } impl Default for DatabaseConfig { @@ -578,6 +622,19 @@ pub struct PostgresConfig { /// Maximum time a database operation can hold a connection before timing out. /// Prevents indefinite connection holds from slow queries or stuck operations. pub operation_deadline_seconds: u64, + + /// Maximum number of related-table queries a single request runs at once + /// (default: 8). Bounds how many pool connections one request can hold while + /// enriching list/detail rows. Higher than SQLite because PostgreSQL + /// executes queries truly concurrently, so the parallelism is worth keeping. + pub batch_fan_out: usize, + + /// Maximum connections for the separate background-work pool (default: 16) + /// used by in-process task workers, the scheduler, and pollers. + /// NOTE: this is additive to `max_connections`; ensure the PostgreSQL + /// server's `max_connections` accommodates the total when workers run in + /// the same process (validated with a warning at startup). + pub background_max_connections: u32, } impl Default for PostgresConfig { @@ -599,6 +656,11 @@ impl Default for PostgresConfig { idle_timeout_seconds: env_or("CODEX_DATABASE_POSTGRES_IDLE_TIMEOUT", 600), max_lifetime_seconds: env_or("CODEX_DATABASE_POSTGRES_MAX_LIFETIME", 3600), operation_deadline_seconds: env_or("CODEX_DATABASE_POSTGRES_OPERATION_DEADLINE", 30), + batch_fan_out: env_or("CODEX_DATABASE_POSTGRES_BATCH_FAN_OUT", 8), + background_max_connections: env_or( + "CODEX_DATABASE_POSTGRES_BACKGROUND_MAX_CONNECTIONS", + 16, + ), } } } @@ -610,15 +672,30 @@ pub struct SQLiteConfig { pub pragmas: Option>, // Connection Pool Settings - /// Maximum number of connections in the pool (default: 16) + /// Maximum number of connections in the pool (default: 64) /// SQLite with WAL mode handles concurrent reads well, but writes are serialized. - /// 16 connections is enough for most workloads without overwhelming the single-writer lock. + /// Connections are cheap file handles under WAL; a larger pool gives headroom + /// for concurrent readers (e.g. many browser tabs) without overwhelming the + /// single-writer lock. pub max_connections: u32, /// Minimum number of connections to maintain in the pool (default: 2) /// Keep a couple warm connections ready pub min_connections: u32, + /// Maximum number of related-table queries a single request runs at once + /// (default: 4). List/detail handlers enrich rows from many tables; this + /// bounds how many of those run concurrently, and therefore how many pool + /// connections one request can hold, so a few concurrent requests cannot + /// exhaust the pool. Kept low for SQLite (writes serialize; queries are + /// cheap indexed lookups, so parallelism gains are marginal). + pub batch_fan_out: usize, + + /// Maximum connections for the separate background-work pool (default: 4) + /// used by task workers, the scheduler, and pollers when they run in the + /// same process, so heavy background work cannot drain the API pool. + pub background_max_connections: u32, + /// Connection acquire timeout in seconds (default: 30) /// How long to wait for a connection before failing pub acquire_timeout_seconds: u64, @@ -650,12 +727,17 @@ impl Default for SQLiteConfig { .unwrap_or_else(|| "data/codex.db".to_string()), pragmas: Some(pragmas), // Pool settings - SQLite is more conservative due to single-writer lock - max_connections: env_or("CODEX_DATABASE_SQLITE_MAX_CONNECTIONS", 16), + max_connections: env_or("CODEX_DATABASE_SQLITE_MAX_CONNECTIONS", 64), min_connections: env_or("CODEX_DATABASE_SQLITE_MIN_CONNECTIONS", 2), acquire_timeout_seconds: env_or("CODEX_DATABASE_SQLITE_ACQUIRE_TIMEOUT", 30), idle_timeout_seconds: env_or("CODEX_DATABASE_SQLITE_IDLE_TIMEOUT", 300), max_lifetime_seconds: env_or("CODEX_DATABASE_SQLITE_MAX_LIFETIME", 1800), operation_deadline_seconds: env_or("CODEX_DATABASE_SQLITE_OPERATION_DEADLINE", 30), + batch_fan_out: env_or("CODEX_DATABASE_SQLITE_BATCH_FAN_OUT", 4), + background_max_connections: env_or( + "CODEX_DATABASE_SQLITE_BACKGROUND_MAX_CONNECTIONS", + 4, + ), } } } @@ -1407,6 +1489,50 @@ verification_url_base: https://codex.example.com assert_eq!(config.operation_deadline_seconds(), 30); } + #[test] + fn test_fan_out_and_background_pool_accessors() { + // SQLite: explicit values flow through the accessors. + let sqlite = DatabaseConfig { + db_type: DatabaseType::SQLite, + postgres: None, + sqlite: Some(SQLiteConfig { + batch_fan_out: 6, + background_max_connections: 3, + max_connections: 64, + ..SQLiteConfig::default() + }), + }; + assert_eq!(sqlite.batch_fan_out(), 6); + assert_eq!(sqlite.background_max_connections(), 3); + assert_eq!(sqlite.max_connections(), 64); + + // batch_fan_out is clamped to at least 1 (a 0 would deadlock a request). + let zero = DatabaseConfig { + db_type: DatabaseType::SQLite, + postgres: None, + sqlite: Some(SQLiteConfig { + batch_fan_out: 0, + ..SQLiteConfig::default() + }), + }; + assert_eq!(zero.batch_fan_out(), 1); + + // Postgres: explicit values flow through the accessors. + let postgres = DatabaseConfig { + db_type: DatabaseType::Postgres, + postgres: Some(PostgresConfig { + batch_fan_out: 12, + background_max_connections: 20, + max_connections: 100, + ..PostgresConfig::default() + }), + sqlite: None, + }; + assert_eq!(postgres.batch_fan_out(), 12); + assert_eq!(postgres.background_max_connections(), 20); + assert_eq!(postgres.max_connections(), 100); + } + #[test] fn test_full_config() { let config = Config { diff --git a/docs/docs/configuration.md b/docs/docs/configuration.md index ea9cb4f2..ad074f03 100644 --- a/docs/docs/configuration.md +++ b/docs/docs/configuration.md @@ -73,23 +73,29 @@ database: sqlite: path: ./data/codex.db # Connection pool settings - max_connections: 16 # Maximum pool size (default: 16) + max_connections: 64 # Maximum pool size (default: 64) min_connections: 2 # Minimum warm connections (default: 2) acquire_timeout_seconds: 30 # Wait time for connection (default: 30) idle_timeout_seconds: 300 # Idle connection timeout (default: 300 = 5 min) max_lifetime_seconds: 1800 # Max connection lifetime (default: 1800 = 30 min) + batch_fan_out: 4 # Per-request query fan-out bound (default: 4) + background_max_connections: 4 # Background-work pool size (default: 4) ``` | Setting | Default | Description | |---------|---------|-------------| -| `max_connections` | `16` | Maximum connections in pool | +| `max_connections` | `64` | Maximum connections in pool | | `min_connections` | `2` | Minimum warm connections | | `acquire_timeout_seconds` | `30` | How long to wait for a connection | | `idle_timeout_seconds` | `300` | Idle connection timeout (5 min) | | `max_lifetime_seconds` | `1800` | Maximum connection lifetime (30 min) | +| `batch_fan_out` | `4` | Max related-table queries one request runs at once | +| `background_max_connections` | `4` | Connections for the in-process background pool | :::tip SQLite Pool Sizing -SQLite with WAL mode handles concurrent reads well, but writes are serialized. The default of 16 connections works well for most workloads. Increase if you see "connection pool timeout" errors during heavy load. +SQLite with WAL mode handles concurrent reads well, but writes are serialized. Connections are cheap file handles under WAL, so the default of 64 gives headroom for many concurrent readers (e.g. multiple browser tabs). + +`batch_fan_out` caps how many related-table queries a single list/detail request runs concurrently, so a few simultaneous requests cannot each grab a connection per query and exhaust the pool. `background_max_connections` gives in-process task workers, the scheduler, and pollers a **separate** pool, so a scan or analysis burst cannot starve interactive API requests. Increase `max_connections` if you still see "connection pool timeout" errors under heavy load. ::: ### PostgreSQL (Recommended for Production) @@ -112,8 +118,14 @@ database: acquire_timeout_seconds: 30 # Wait time for connection (default: 30) idle_timeout_seconds: 600 # Idle connection timeout (default: 600 = 10 min) max_lifetime_seconds: 3600 # Max connection lifetime (default: 3600 = 1 hour) + batch_fan_out: 8 # Per-request query fan-out bound (default: 8) + background_max_connections: 16 # Background-work pool size (default: 16) ``` +:::warning PostgreSQL connection budget +`background_max_connections` is **additive** to `max_connections` whenever task workers run in the same process as the web server. Ensure the PostgreSQL server's own `max_connections` covers the total (API pool + background pool). Multi-pod deployments run the web server with `CODEX_DISABLE_WORKERS=true`, so no background pool is created there. Codex logs a warning at startup if the configured pools exceed the server limit. +::: + #### PostgreSQL SSL Modes | Mode | Description | diff --git a/docs/docs/deployment/performance.md b/docs/docs/deployment/performance.md index c725dfca..eb62fe83 100644 --- a/docs/docs/deployment/performance.md +++ b/docs/docs/deployment/performance.md @@ -39,9 +39,22 @@ database: WAL mode allows readers while a write is in progress, dramatically improving responsiveness during library scans. ::: +#### Connection Pool Behaviour Under Load + +Two settings keep a single SQLite instance from stalling when many requests arrive at once (for example, several open browser tabs): + +- **`batch_fan_out`** (default `4`) bounds how many related-table queries a single list/detail request runs concurrently. Without this bound, one request could hold a connection per query and a few simultaneous requests would exhaust the pool, causing multi-second `connection pool timeout` waits. +- **`background_max_connections`** (default `4`) gives in-process task workers, the scheduler, and pollers a **separate** connection pool. A library scan or analysis burst therefore cannot drain the pool that serves interactive API requests. This pool is only created when workers run in the same process (the default single-container `serve`); web-only deployments keep a single pool. + +If you still hit pool-timeout warnings under heavy concurrent browsing, raise `max_connections` (cheap under WAL — connections are just file handles). + +:::note +These settings reduce connection contention, but **writes still serialize** on SQLite: a heavy scan slows other writes regardless of pool sizing. That is inherent to SQLite. For write-heavy or many-user workloads, use PostgreSQL. +::: + #### SQLite Limitations -- **Single writer**: Only one process can write at a time +- **Single writer**: Only one process can write at a time (heavy scans slow all writes) - **No horizontal scaling**: Cannot run multiple Codex instances - **Concurrent users**: Best for 5-10 simultaneous users - **File locking**: Network filesystems (NFS, SMB) may cause issues diff --git a/src/commands/serve.rs b/src/commands/serve.rs index 858b4ec3..8cd97da7 100644 --- a/src/commands/serve.rs +++ b/src/commands/serve.rs @@ -49,6 +49,10 @@ pub async fn serve_command(config_path: PathBuf) -> anyhow::Result<()> { // Initialize database connection let db = init_database(&config).await?; + // Resolve the per-request query fan-out bound for this backend once, so the + // list/detail handlers cap how many pool connections a single request holds. + codex_api::db_batch::set_fan_out(config.database.batch_fan_out()); + // Create cancellation token for graceful shutdown of background tasks let background_task_cancel = CancellationToken::new(); @@ -116,10 +120,17 @@ pub async fn serve_command(config_path: PathBuf) -> anyhow::Result<()> { let background_db = if disable_workers { None } else { - let background_max = match config.database.db_type { - DatabaseType::SQLite => 4, - DatabaseType::Postgres => 16, - }; + let background_max = config.database.background_max_connections(); + // On Postgres the background pool is additive to the API pool; warn (not + // fatal) if the combined demand likely exceeds the server's limit. + if config.database.db_type == DatabaseType::Postgres { + warn_if_pg_budget_exceeded( + db.sea_orm_connection(), + config.database.max_connections(), + background_max, + ) + .await; + } Some(codex_db::Database::new_background(&config.database, background_max).await?) }; // Connection handed to background subsystems: the dedicated pool when one @@ -675,6 +686,57 @@ pub async fn serve_command(config_path: PathBuf) -> anyhow::Result<()> { Ok(()) } +/// Warn (non-fatal) when the API pool plus the additive background pool exceed +/// the PostgreSQL server's `max_connections`. Best-effort: if the value can't be +/// read, the check is skipped quietly. SQLite has no server-side connection +/// limit, so this only runs for Postgres. +async fn warn_if_pg_budget_exceeded( + conn: &sea_orm::DatabaseConnection, + api_max: u32, + background_max: u32, +) { + use sea_orm::{ConnectionTrait, Statement}; + + let requested = api_max + background_max; + let backend = conn.get_database_backend(); + match conn + .query_one(Statement::from_string( + backend, + "SHOW max_connections".to_string(), + )) + .await + { + Ok(Some(row)) => { + // Postgres returns max_connections as a text column. + let server_max = row + .try_get::("", "max_connections") + .ok() + .and_then(|s| s.parse::().ok()); + match server_max { + Some(server_max) if requested > server_max => { + tracing::warn!( + "Configured connection pools (API {api_max} + background {background_max} \ + = {requested}) exceed PostgreSQL server max_connections ({server_max}). \ + Reduce database.postgres.max_connections or background_max_connections, \ + or raise the server limit, to avoid connection failures." + ); + } + Some(server_max) => { + info!( + "PostgreSQL connection budget OK: API {api_max} + background \ + {background_max} = {requested} <= server max {server_max}" + ); + } + None => {} + } + } + Ok(None) => {} + Err(e) => { + tracing::warn!("Could not read PostgreSQL max_connections for budget check: {e}"); + } + } +} + /// Wait for shutdown signal (SIGTERM or SIGINT/Ctrl+C) async fn shutdown_signal() { let ctrl_c = async { From 791874863a86c3422a6460b76f17cf9cc5645e64 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Wed, 3 Jun 2026 19:54:49 -0700 Subject: [PATCH 04/11] test(api): add connection-pool contention regression test Fire many concurrent series-list requests against a deliberately small SQLite pool with a short acquire timeout and assert they all succeed. This guards the per-request query fan-out bound: if it regressed to an unbounded tokio::join!, the cross-request demand would starve the pool and the requests would fail with acquire timeouts instead of 200s. Includes an ignored PostgreSQL parity variant that runs the same workload when a test server is available. The deterministic proof that concurrency stays within the bound lives in the db_batch unit test; this test verifies the bound is wired through the real HTTP endpoints under load. --- tests/api/mod.rs | 1 + tests/api/pool_contention.rs | 173 +++++++++++++++++++++++++++++++++++ 2 files changed, 174 insertions(+) create mode 100644 tests/api/pool_contention.rs diff --git a/tests/api/mod.rs b/tests/api/mod.rs index d68964b6..7ed91284 100644 --- a/tests/api/mod.rs +++ b/tests/api/mod.rs @@ -37,6 +37,7 @@ mod pages; mod pdf_cache; mod plugin_metrics; mod plugins; +mod pool_contention; mod rate_limit; mod read_progress; mod recommendations; diff --git a/tests/api/pool_contention.rs b/tests/api/pool_contention.rs new file mode 100644 index 00000000..715979f6 --- /dev/null +++ b/tests/api/pool_contention.rs @@ -0,0 +1,173 @@ +//! Connection-pool contention regression tests. +//! +//! These guard the per-request query fan-out bound (the `db_batch` helper). +//! A list request enriches its rows from many related tables; without a bound +//! each query grabs its own pool connection, so a handful of concurrent +//! requests exhaust a small pool and `acquire()` blocks for seconds. With the +//! bound, a single request holds at most `batch_fan_out` connections, so many +//! concurrent requests complete promptly instead of timing out. +//! +//! The SQLite test runs against a deliberately small pool with a short acquire +//! timeout: if the fan-out bound regressed (back to unbounded `tokio::join!`), +//! the cross-request demand on the tiny pool would push `acquire()` past the +//! timeout and the requests would fail with 500s rather than 200s. + +#[path = "../common/mod.rs"] +mod common; + +use std::collections::HashMap; +use std::time::Duration; + +use codex::db::repositories::{LibraryRepository, SeriesRepository, UserRepository}; +use codex::utils::password; +use common::*; +use hyper::StatusCode; +use tempfile::TempDir; + +/// Number of concurrent `GET /api/v1/series` requests to fire. +const CONCURRENT_REQUESTS: usize = 24; +/// Series to seed so the list endpoint does real per-row enrichment work. +const SEEDED_SERIES: usize = 25; + +/// Build a SQLite database with a small connection pool and a short acquire +/// timeout, so connection starvation surfaces as a fast failure rather than a +/// 30s hang. Returns the connection plus the TempDir (kept alive by the caller). +async fn small_pool_sqlite() -> (sea_orm::DatabaseConnection, TempDir) { + let temp_dir = TempDir::new().unwrap(); + let db_path = temp_dir.path().join("contention.db"); + + let mut pragmas = HashMap::new(); + pragmas.insert("foreign_keys".to_string(), "ON".to_string()); + pragmas.insert("journal_mode".to_string(), "WAL".to_string()); + + let config = DatabaseConfig { + db_type: DatabaseType::SQLite, + postgres: None, + sqlite: Some(SQLiteConfig { + path: db_path.to_str().unwrap().to_string(), + pragmas: Some(pragmas), + // Smaller than the full series DTO fan-out (14) so an unbounded + // regression would saturate the pool under concurrency. + max_connections: 4, + min_connections: 1, + // Fail fast instead of hanging 30s if the pool is genuinely starved. + acquire_timeout_seconds: 5, + ..SQLiteConfig::default() + }), + }; + + let database = Database::new(&config).await.unwrap(); + database.run_migrations().await.unwrap(); + (database.sea_orm_connection().clone(), temp_dir) +} + +/// Seed a library with `SEEDED_SERIES` series and return an admin JWT for the +/// given auth state. +async fn seed_and_token( + db: &sea_orm::DatabaseConnection, + state: &codex::api::extractors::AuthState, +) -> String { + let library = + LibraryRepository::create(db, "Library", "/lib", codex::db::ScanningStrategy::Default) + .await + .unwrap(); + for i in 0..SEEDED_SERIES { + SeriesRepository::create(db, library.id, &format!("Series {i}"), None) + .await + .unwrap(); + } + + let password_hash = password::hash_password("admin123").unwrap(); + let user = create_test_user("admin", "admin@example.com", &password_hash, true); + let created = UserRepository::create(db, &user).await.unwrap(); + state + .jwt_service + .generate_token(created.id, created.username.clone(), created.get_role()) + .unwrap() +} + +/// Fire `CONCURRENT_REQUESTS` simultaneous list requests against a 4-connection +/// SQLite pool. With the fan-out bound in place they all succeed; an unbounded +/// regression would starve the pool and return 500s (acquire timeout). +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn concurrent_series_list_does_not_starve_small_pool() { + let (db, _temp_dir) = small_pool_sqlite().await; + let state = create_test_auth_state(db.clone()).await; + let token = seed_and_token(&db, &state).await; + let app = create_test_router(state).await; + + let mut handles = Vec::with_capacity(CONCURRENT_REQUESTS); + for _ in 0..CONCURRENT_REQUESTS { + let app = app.clone(); + let token = token.clone(); + handles.push(tokio::spawn(async move { + let request = get_request_with_auth("/api/v1/series", &token); + make_request(app, request).await.0 + })); + } + + for handle in handles { + // The hard timeout is a deadlock guard, not a perf assertion: the + // acquire timeout (5s) would already turn starvation into a 500. + let status = tokio::time::timeout(Duration::from_secs(30), handle) + .await + .expect("a request hung well past the acquire timeout") + .expect("request task panicked"); + assert_eq!( + status, + StatusCode::OK, + "a concurrent list request failed — the connection pool was likely starved" + ); + } +} + +/// PostgreSQL parity: the same concurrent-list workload must not regress on +/// Postgres. Ignored by default; runs when a test PostgreSQL server is +/// available (see `setup_test_db_postgres`). Postgres uses a larger default +/// pool and true parallel execution, so this is a no-regression smoke rather +/// than a tight small-pool contention test. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[ignore] // Requires PostgreSQL server +async fn concurrent_series_list_postgres_parity() { + let Some(db) = setup_test_db_postgres().await else { + return; // PostgreSQL not available; skip. + }; + + let library = + LibraryRepository::create(&db, "Library", "/lib", codex::db::ScanningStrategy::Default) + .await + .unwrap(); + for i in 0..SEEDED_SERIES { + SeriesRepository::create(&db, library.id, &format!("Series {i}"), None) + .await + .unwrap(); + } + + let state = create_test_auth_state(db.clone()).await; + let password_hash = password::hash_password("admin123").unwrap(); + let user = create_test_user("admin", "admin@example.com", &password_hash, true); + let created = UserRepository::create(&db, &user).await.unwrap(); + let token = state + .jwt_service + .generate_token(created.id, created.username.clone(), created.get_role()) + .unwrap(); + let app = create_test_router(state).await; + + let mut handles = Vec::with_capacity(CONCURRENT_REQUESTS); + for _ in 0..CONCURRENT_REQUESTS { + let app = app.clone(); + let token = token.clone(); + handles.push(tokio::spawn(async move { + let request = get_request_with_auth("/api/v1/series", &token); + make_request(app, request).await.0 + })); + } + + for handle in handles { + let status = tokio::time::timeout(Duration::from_secs(30), handle) + .await + .expect("a request hung") + .expect("request task panicked"); + assert_eq!(status, StatusCode::OK); + } +} From d15f45ef438219194f12a8dfc0b442b77ed4cde6 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Wed, 3 Jun 2026 20:30:07 -0700 Subject: [PATCH 05/11] perf(web): relax query refetch defaults to avoid multi-tab refetch storms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Freshness is already driven by SSE entity events, which invalidate the relevant query caches when data actually changes. The global staleTime of 5s combined with refetchOnWindowFocus meant switching between many open tabs re-ran every active query, including the heavy series list — a needless load amplifier. Raise staleTime to 30s and disable refetch-on-focus, relying on the SSE stream for real-time updates. Keep refetch-on-reconnect so the client recovers any events missed while offline. --- web/src/main.tsx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/web/src/main.tsx b/web/src/main.tsx index 218ca2dd..293db210 100644 --- a/web/src/main.tsx +++ b/web/src/main.tsx @@ -20,10 +20,16 @@ import "./index.css"; const queryClient = new QueryClient({ defaultOptions: { queries: { - staleTime: 5000, // 5 seconds - reduced for real-time updates - refetchOnWindowFocus: true, // Refetch when switching tabs - refetchOnMount: true, // Refetch when component mounts - refetchOnReconnect: true, // Refetch when network reconnects + // Freshness is driven by SSE entity events (useEntityEvents), which + // invalidate the relevant query keys when data actually changes. So we + // don't need aggressive time-based refetching. A longer staleTime plus + // no refetch-on-focus avoids a refetch storm when the user has many tabs + // open and switches between them (each switch previously refetched every + // active query, e.g. the heavy series list). + staleTime: 30_000, // 30 seconds; SSE invalidation handles real-time changes + refetchOnWindowFocus: false, // Rely on SSE, not tab-focus, for freshness + refetchOnMount: true, // Refetch on mount only if data is stale + refetchOnReconnect: true, // Refetch after network loss (may have missed SSE events) retry: (failureCount, error) => { // Don't retry on client errors (4xx) - axios handles 429 retries internally const apiError = error as { error?: string }; From 0ed4bed19f5511204281c2b84276b23de42f11cb Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Thu, 4 Jun 2026 16:50:57 -0700 Subject: [PATCH 06/11] fix(series): eliminate list N+1 and SSE-driven refetch storm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default (non-full) /series/list built its DTOs with an unbounded futures::future::join_all over a per-series helper that ran ~6 sequential queries each, including a per-series library lookup. For a 200-series page that fanned out to ~1000 concurrent, unbounded connection acquisitions from a single request — enough to exhaust the pool on its own. Swap every such list call site to the existing batched converter, which issues a fixed handful of queries regardless of page size. On the client, every cover_updated event force-refetched the entire series/books list. During a library scan or bulk thumbnail regeneration — which emit a cover/book event per item — this produced hundreds of heavy list refetches that piled up and cancelled each other, hammering the API pool. Cover changes only affect an image URL (already cache-busted via the cover-timestamp store), so drop the list refetch on cover events entirely, and throttle the legitimate data-change invalidations so a burst coalesces into a handful of refetches instead of one per event. Updates the affected event-handler tests and strengthens the pool contention test to cover the non-full list path. --- .../src/routes/v1/handlers/series.rs | 135 +++++++----------- tests/api/pool_contention.rs | 8 +- web/src/hooks/useEntityEvents.test.ts | 98 +++++++------ web/src/hooks/useEntityEvents.tsx | 91 ++++++------ 4 files changed, 160 insertions(+), 172 deletions(-) diff --git a/crates/codex-api/src/routes/v1/handlers/series.rs b/crates/codex-api/src/routes/v1/handlers/series.rs index 666280f1..200f276f 100644 --- a/crates/codex-api/src/routes/v1/handlers/series.rs +++ b/crates/codex-api/src/routes/v1/handlers/series.rs @@ -900,15 +900,12 @@ pub async fn list_series( Ok(paginated_response(response, &link_builder)) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; let response = SeriesListResponse::with_builder(dtos, page, page_size, total, &link_builder); @@ -1550,15 +1547,12 @@ pub async fn list_series_filtered( Ok(paginated_response(response, &link_builder)) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; let response = SeriesListResponse::with_builder(dtos, page, page_size, total, &link_builder); @@ -2240,15 +2234,12 @@ pub async fn list_in_progress_series( Ok(Json(full_dtos).into_response()) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; Ok(Json(dtos).into_response()) } @@ -2319,15 +2310,12 @@ pub async fn list_recently_added_series( Ok(Json(full_dtos).into_response()) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; Ok(Json(dtos).into_response()) } @@ -2380,15 +2368,12 @@ pub async fn list_library_recently_added_series( Ok(Json(full_dtos).into_response()) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; Ok(Json(dtos).into_response()) } @@ -2436,15 +2421,12 @@ pub async fn list_recently_updated_series( Ok(Json(full_dtos).into_response()) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; Ok(Json(dtos).into_response()) } @@ -2496,15 +2478,12 @@ pub async fn list_library_recently_updated_series( Ok(Json(full_dtos).into_response()) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; Ok(Json(dtos).into_response()) } @@ -2599,15 +2578,12 @@ pub async fn list_library_series( Ok(paginated_response(response, &link_builder)) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; let response = SeriesListResponse::with_builder(dtos, page, page_size, total, &link_builder); @@ -2672,15 +2648,12 @@ pub async fn list_library_in_progress_series( Ok(Json(full_dtos).into_response()) } else { let user_id = Some(auth.user_id); - let dtos: Vec = futures::future::join_all( - series_list - .into_iter() - .map(|series| series_to_dto(&state.db, series, user_id)), - ) - .await - .into_iter() - .collect::, _>>() - .map_err(|e| ApiError::Internal(format!("Failed to build series DTOs: {:?}", e)))?; + // Batched assembly: one bounded set of queries per related table instead + // of an unbounded per-series fan-out. The per-series path (join_all over + // the singular series_to_dto) is an N+1 that, on a large page, fires + // hundreds of concurrent connection acquisitions from a single request + // and can exhaust the pool. + let dtos = series_to_dtos_batched(&state.db, series_list, user_id).await?; Ok(Json(dtos).into_response()) } diff --git a/tests/api/pool_contention.rs b/tests/api/pool_contention.rs index 715979f6..90e04d23 100644 --- a/tests/api/pool_contention.rs +++ b/tests/api/pool_contention.rs @@ -27,7 +27,13 @@ use tempfile::TempDir; /// Number of concurrent `GET /api/v1/series` requests to fire. const CONCURRENT_REQUESTS: usize = 24; /// Series to seed so the list endpoint does real per-row enrichment work. -const SEEDED_SERIES: usize = 25; +/// +/// Sized large enough that a regression to the old per-series N+1 fan-out +/// (`join_all` over the singular `series_to_dto`, ~6 queries per series, all +/// unbounded) would demand far more than the 4-connection pool can supply and +/// blow the acquire timeout — whereas the batched converter issues a fixed +/// handful of queries regardless of page size. +const SEEDED_SERIES: usize = 60; /// Build a SQLite database with a small connection pool and a short acquire /// timeout, so connection starvation surfaces as a fast failure rather than a diff --git a/web/src/hooks/useEntityEvents.test.ts b/web/src/hooks/useEntityEvents.test.ts index 6d3c8e2e..d75bed88 100644 --- a/web/src/hooks/useEntityEvents.test.ts +++ b/web/src/hooks/useEntityEvents.test.ts @@ -100,7 +100,7 @@ describe("useEntityEvents", () => { }); }); - it("should invalidate and refetch queries and record cover update on CoverUpdated event", async () => { + it("should record cover update and invalidate only the specific entity on CoverUpdated event (no list refetch)", async () => { let capturedCallback: ((event: EntityChangeEvent) => void) | undefined; vi.spyOn(eventsApi.eventsApi, "subscribeToEntityEvents").mockImplementation( @@ -136,19 +136,21 @@ describe("useEntityEvents", () => { } await waitFor(() => { - // Invalidate the specific series + // Only the specific series detail query is invalidated (cheap, targeted) expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ["series", "series-123"], }); - // Invalidate all series list queries - expect(invalidateSpy).toHaveBeenCalledWith({ - queryKey: ["series"], - }); - // Refetch all active series queries to trigger component re-render - expect(refetchSpy).toHaveBeenCalledWith({ - queryKey: ["series"], - type: "active", - }); + }); + + // A cover change must NOT refetch the heavy list — the image refreshes via + // the cover-timestamp store. Re-pulling the list per cover event was the + // refetch storm we removed. + expect(invalidateSpy).not.toHaveBeenCalledWith({ + queryKey: ["series"], + }); + expect(refetchSpy).not.toHaveBeenCalledWith({ + queryKey: ["series"], + type: "active", }); // Verify cover update was recorded in the store for cache-busting @@ -293,7 +295,7 @@ describe("useEntityEvents", () => { }); }); - it("should invalidate Recommended section queries on cover_updated event", async () => { + it("should NOT invalidate list/Recommended queries on cover_updated (image refresh is store-driven)", async () => { let capturedCallback: ((event: EntityChangeEvent) => void) | undefined; vi.spyOn(eventsApi.eventsApi, "subscribeToEntityEvents").mockImplementation( @@ -303,6 +305,9 @@ describe("useEntityEvents", () => { }, ); + // Reset cover updates store for isolation + useCoverUpdatesStore.setState({ updates: {} }); + // Set up queries that match the Recommended section's query keys queryClient.setQueryData(["series", "recently-added", "lib-1"], []); queryClient.setQueryData(["series", "recently-updated", "lib-1"], []); @@ -328,30 +333,30 @@ describe("useEntityEvents", () => { capturedCallback(seriesEvent); } - // All series queries should be invalidated (stale) + // Confirm the handler ran by checking it recorded the cover timestamp + // (the image-refresh mechanism). await waitFor(() => { - const recentlyAddedState = queryClient.getQueryState([ - "series", - "recently-added", - "lib-1", - ]); - const recentlyUpdatedState = queryClient.getQueryState([ - "series", - "recently-updated", - "lib-1", - ]); - - expect(recentlyAddedState?.isInvalidated).toBe(true); - expect(recentlyUpdatedState?.isInvalidated).toBe(true); + expect( + useCoverUpdatesStore.getState().getCoverTimestamp("series-123"), + ).toBeDefined(); }); - // Books queries should NOT be invalidated by series cover update - const booksRecentlyAddedState = queryClient.getQueryState([ - "books", - "recently-added", - "lib-1", - ]); - expect(booksRecentlyAddedState?.isInvalidated).toBe(false); + // A series cover update must NOT invalidate the series list/Recommended + // queries — only the image refreshes via the store. This is what removes + // the per-cover refetch storm. + expect( + queryClient.getQueryState(["series", "recently-added", "lib-1"]) + ?.isInvalidated, + ).toBe(false); + expect( + queryClient.getQueryState(["series", "recently-updated", "lib-1"]) + ?.isInvalidated, + ).toBe(false); + // And it does not touch books either. + expect( + queryClient.getQueryState(["books", "recently-added", "lib-1"]) + ?.isInvalidated, + ).toBe(false); // Simulate receiving a cover_updated event for a book const bookEvent: EntityChangeEvent = { @@ -366,22 +371,21 @@ describe("useEntityEvents", () => { capturedCallback(bookEvent); } - // All book queries should now be invalidated (stale) + // Likewise a book cover update records its timestamp but does not + // invalidate the book list/Recommended queries. await waitFor(() => { - const booksRecentlyAddedState = queryClient.getQueryState([ - "books", - "recently-added", - "lib-1", - ]); - const booksInProgressState = queryClient.getQueryState([ - "books", - "in-progress", - "lib-1", - ]); - - expect(booksRecentlyAddedState?.isInvalidated).toBe(true); - expect(booksInProgressState?.isInvalidated).toBe(true); + expect( + useCoverUpdatesStore.getState().getCoverTimestamp("book-456"), + ).toBeDefined(); }); + expect( + queryClient.getQueryState(["books", "recently-added", "lib-1"]) + ?.isInvalidated, + ).toBe(false); + expect( + queryClient.getQueryState(["books", "in-progress", "lib-1"]) + ?.isInvalidated, + ).toBe(false); }); it("should invalidate and refetch plugin queries on plugin events", async () => { diff --git a/web/src/hooks/useEntityEvents.tsx b/web/src/hooks/useEntityEvents.tsx index 382c4eea..8b19e258 100644 --- a/web/src/hooks/useEntityEvents.tsx +++ b/web/src/hooks/useEntityEvents.tsx @@ -1,6 +1,7 @@ import { Anchor } from "@mantine/core"; import { notifications } from "@mantine/notifications"; import { useQueryClient } from "@tanstack/react-query"; +import { throttle } from "es-toolkit"; import { useEffect, useState } from "react"; import { eventsApi } from "@/api/events"; import { navigationService } from "@/services/navigation"; @@ -134,9 +135,25 @@ export function useEntityEvents() { return; } + // Throttle the heavy list invalidations: a burst of entity events (e.g. a + // scan or bulk thumbnail regen) collapses into at most one series/books + // list refetch per interval instead of one per event. + const invalidateSeriesList = throttle( + () => queryClient.invalidateQueries({ queryKey: ["series"] }), + 1500, + ); + const invalidateBooksList = throttle( + () => queryClient.invalidateQueries({ queryKey: ["books"] }), + 1500, + ); + const listInvalidate: ListInvalidators = { + series: invalidateSeriesList, + books: invalidateBooksList, + }; + const unsubscribe = eventsApi.subscribeToEntityEvents( (event: EntityChangeEvent) => { - handleEntityEvent(event, queryClient); + handleEntityEvent(event, queryClient, listInvalidate); }, (error: Error) => { console.error("[SSE] Connection error:", error); @@ -149,6 +166,9 @@ export function useEntityEvents() { return () => { unsubscribe(); + // Drop any pending trailing refetch so we don't fire after teardown/logout. + invalidateSeriesList.cancel(); + invalidateBooksList.cancel(); }; }, [queryClient, isAuthenticated]); @@ -160,9 +180,17 @@ export function useEntityEvents() { /** * Handle entity change events and invalidate appropriate query caches */ +/** Throttled invalidators for the heavy list queries. A scan/analyze can emit + * hundreds of entity events; invalidating the (large) series/books list on each + * one would refetch it hundreds of times. Throttling coalesces a burst into at + * most one refetch per interval (leading + trailing), so a single event still + * updates promptly while a sustained stream refreshes ~once/interval. */ +type ListInvalidators = { series: () => void; books: () => void }; + function handleEntityEvent( event: EntityChangeEvent, queryClient: ReturnType, + listInvalidate: ListInvalidators, ) { log("Received entity event:", event.type, event); @@ -171,13 +199,11 @@ function handleEntityEvent( case "book_created": case "book_updated": case "book_deleted": { - // Invalidate book queries - use "all" to ensure Recommended section updates - // even when user switches between tabs - queryClient.invalidateQueries({ - queryKey: ["books"], - }); + // Invalidate the (heavy) book list, throttled so a scan adding many + // books coalesces into a handful of refetches instead of one per book. + listInvalidate.books(); - // Invalidate specific book if it's an update + // Invalidate specific book if it's an update (targeted + cheap) if (event.type === "book_updated") { queryClient.invalidateQueries({ queryKey: ["books", event.bookId], @@ -190,10 +216,8 @@ function handleEntityEvent( queryKey: ["libraries", event.libraryId], }); - // Invalidate series in this library - queryClient.invalidateQueries({ - queryKey: ["series"], - }); + // Series in this library may have changed (book counts, etc.) + listInvalidate.series(); } break; } @@ -203,10 +227,9 @@ function handleEntityEvent( case "series_deleted": case "series_bulk_purged": case "series_metadata_updated": { - // Invalidate series queries - use default to ensure Recommended section updates - queryClient.invalidateQueries({ - queryKey: ["series"], - }); + // Invalidate the (heavy) series list, throttled so a bulk operation + // emitting many series events coalesces into a handful of refetches. + listInvalidate.series(); // Invalidate specific series if it's an update if ( @@ -247,36 +270,22 @@ function handleEntityEvent( `Cover updated for ${event.entityType} ${event.entityId}, cache-bust timestamp: ${timestamp}`, ); + // A cover change only affects the entity's IMAGE, not its list data. + // MediaCard / detail views read the cache-bust timestamp recorded above + // from the cover store and re-render the image on their own — no list + // refetch is needed. Re-pulling the whole series/books list on every + // cover event caused a refetch storm during bulk thumbnail regeneration + // (one heavy refetch per series, hundreds of them). So invalidate only + // the specific entity's detail query (cheap, keeps cover-source fields + // current); never the list. if (event.entityType === "book") { - // Invalidate the specific book query queryClient.invalidateQueries({ queryKey: ["books", event.entityId], }); - // Invalidate all book list queries (marks them as stale) - queryClient.invalidateQueries({ - queryKey: ["books"], - }); - // Force immediate refetch of active queries to trigger component re-render - // This ensures MediaCard components pick up the new cache-busting timestamp - queryClient.refetchQueries({ - queryKey: ["books"], - type: "active", - }); } else if (event.entityType === "series") { - // Invalidate the specific series query queryClient.invalidateQueries({ queryKey: ["series", event.entityId], }); - // Invalidate all series list queries (marks them as stale) - queryClient.invalidateQueries({ - queryKey: ["series"], - }); - // Force immediate refetch of active queries to trigger component re-render - // This ensures MediaCard components pick up the new cache-busting timestamp - queryClient.refetchQueries({ - queryKey: ["series"], - type: "active", - }); } break; } @@ -297,12 +306,8 @@ function handleEntityEvent( // When a library is deleted, also invalidate all books and series queries // since they may contain data from the deleted library if (event.type === "library_deleted") { - queryClient.invalidateQueries({ - queryKey: ["books"], - }); - queryClient.invalidateQueries({ - queryKey: ["series"], - }); + listInvalidate.books(); + listInvalidate.series(); } break; } From ce601b3fed219e6d3fad21affd55327805c6672c Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Thu, 4 Jun 2026 17:36:56 -0700 Subject: [PATCH 07/11] perf(on-deck): paginate by series instead of loading all unread books MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit list_on_deck fetched every unread book across all of a user's eligible series into memory, then picked the first book per series, sorted, and paginated in memory — discarding ~99% of the rows and scaling with the size of the library. On large libraries this made the home page's On Deck section slow. Rework it: prune eligible series by visibility and library in Rust, then SELECT DISTINCT series_id to find which eligible series actually have an on-deck book (the total), order and paginate that series list, and load unread books only for the page's series before picking the first per series. Rows loaded now scale with page size, not library size. Also fix on-deck pagination past the first page: callers pass a 0-indexed row offset, but the repository recomputed start = offset * page_size, so later pages skipped too far and returned nothing. The two callers also disagreed — the native handler passed an offset while the Komga handler passed a 0-indexed page index. Standardize both on an offset (Komga now passes page * size) and slice with start = offset. Adds a pagination test covering multiple on-deck series. --- .../src/routes/komga/handlers/books.rs | 4 +- crates/codex-db/src/repositories/book.rs | 140 ++++++++++++------ tests/api/books.rs | 90 +++++++++++ 3 files changed, 187 insertions(+), 47 deletions(-) diff --git a/crates/codex-api/src/routes/komga/handlers/books.rs b/crates/codex-api/src/routes/komga/handlers/books.rs index d77ed357..be1b498f 100644 --- a/crates/codex-api/src/routes/komga/handlers/books.rs +++ b/crates/codex-api/src/routes/komga/handlers/books.rs @@ -276,8 +276,10 @@ pub async fn get_books_ondeck( require_permission!(auth, Permission::BooksRead)?; let user_id = auth.user_id; + // Komga pagination is 0-indexed; convert to a 0-indexed row offset. let page = query.page.max(0) as u64; let size = query.size.clamp(1, 500) as u64; + let offset = page * size; let content_filter = ContentFilter::for_user(&state.db, user_id) .await @@ -290,7 +292,7 @@ pub async fn get_books_ondeck( &state.db, user_id, query.library_id, - page, + offset, size, visibility.as_ref(), ) diff --git a/crates/codex-db/src/repositories/book.rs b/crates/codex-db/src/repositories/book.rs index 5fcaf208..1f81cb03 100644 --- a/crates/codex-db/src/repositories/book.rs +++ b/crates/codex-db/src/repositories/book.rs @@ -1618,7 +1618,8 @@ impl BookRepository { db: &DatabaseConnection, user_id: Uuid, library_id: Option, - page: u64, + // 0-indexed row offset (callers pass `(page - 1) * page_size`). + offset: u64, page_size: u64, visibility: Option<&SeriesVisibility>, ) -> Result<(Vec, u64)> { @@ -1674,17 +1675,52 @@ impl BookRepository { .await .context("Failed to get in-progress series")?; - // Step 3: Calculate eligible series (completed - in_progress) - let eligible_series: Vec = completed_series + // Step 3: Calculate eligible series (completed - in_progress). + let in_progress_set: std::collections::HashSet = + in_progress_series.into_iter().collect(); + let mut eligible_series: Vec = completed_series .into_iter() - .filter(|s| !in_progress_series.contains(s)) + .filter(|s| !in_progress_set.contains(s)) .collect(); + // Apply series-level visibility by pruning the eligible set: a book is + // visible iff its series is, so we never need to push the visibility + // predicate into the (heavier) book queries below. Mirrors + // `apply_book_visibility` semantics (deny wins; `Some(allow)` = whitelist). + if let Some(vis) = visibility + && !vis.is_unrestricted() + { + let excluded: std::collections::HashSet = + vis.excluded_series_ids.iter().copied().collect(); + let allowed: Option> = vis + .allowed_series_ids + .as_ref() + .map(|ids| ids.iter().copied().collect()); + eligible_series.retain(|s| { + !excluded.contains(s) && allowed.as_ref().is_none_or(|a| a.contains(s)) + }); + } + + // Restrict to a single library if requested (series belong to one library). + if let Some(lib_id) = library_id + && !eligible_series.is_empty() + { + eligible_series = series::Entity::find() + .select_only() + .column(series::Column::Id) + .filter(series::Column::Id.is_in(eligible_series.clone())) + .filter(series::Column::LibraryId.eq(lib_id)) + .into_tuple::() + .all(db) + .await + .context("Failed to filter eligible series by library")?; + } + if eligible_series.is_empty() { return Ok((vec![], 0)); } - // Step 4: Get all book IDs that have progress for this user (to exclude from unread) + // Step 4: Book IDs that have progress for this user (to exclude from unread). let books_with_progress: Vec = read_progress::Entity::find() .select_only() .column(read_progress::Column::BookId) @@ -1694,30 +1730,58 @@ impl BookRepository { .await .context("Failed to get books with progress")?; - // Step 5: Get all books in eligible series that are unread - let mut unread_query = Books::find() + // Step 5: Of the eligible series, find which ones actually have an unread + // book (an eligible series whose books are all read has no on-deck book). + // This is the total, and it returns only series IDs — not the full row set + // the old implementation loaded into memory. + let mut on_deck_series_query = Books::find() + .select_only() + .column(books::Column::SeriesId) + .distinct() .filter(books::Column::SeriesId.is_in(eligible_series.clone())) .filter(books::Column::Deleted.eq(false)); - - // Exclude books that have progress if !books_with_progress.is_empty() { - unread_query = unread_query.filter(books::Column::Id.is_not_in(books_with_progress)); + on_deck_series_query = on_deck_series_query + .filter(books::Column::Id.is_not_in(books_with_progress.clone())); } + let on_deck_series: std::collections::HashSet = on_deck_series_query + .into_tuple::() + .all(db) + .await + .context("Failed to find on-deck series")? + .into_iter() + .collect(); - // Filter by library if specified - if let Some(lib_id) = library_id { - unread_query = unread_query - .join(JoinType::InnerJoin, books::Relation::Series.def()) - .filter(series::Column::LibraryId.eq(lib_id)); + // Step 6: Order the on-deck series by most-recent completed read (desc) and + // paginate the *series* list before touching any book rows. + let mut ordered_series: Vec = eligible_series + .into_iter() + .filter(|s| on_deck_series.contains(s)) + .collect(); + ordered_series.sort_by(|a, b| series_last_read.get(b).cmp(&series_last_read.get(a))); + + let total = ordered_series.len() as u64; + // `offset` is already a 0-indexed row offset. (The previous + // implementation multiplied it by page_size again, so on-deck + // pagination past page 1 returned the wrong rows / nothing.) + let start = offset as usize; + if start >= ordered_series.len() { + return Ok((vec![], total)); } + let end = (start + page_size as usize).min(ordered_series.len()); + let page_series = &ordered_series[start..end]; - // Hide books in series the caller cannot see. - unread_query = apply_book_visibility(unread_query, visibility); - - // Order by series, then by book number/title/filename (from metadata) + // Step 7: Load unread books only for the page's series, then pick the first + // per series. Bounded by page_size, not by the user's library size. use crate::entities::book_metadata; - let all_unread_books = unread_query + let mut page_query = Books::find() + .filter(books::Column::SeriesId.is_in(page_series.to_vec())) + .filter(books::Column::Deleted.eq(false)); + if !books_with_progress.is_empty() { + page_query = page_query.filter(books::Column::Id.is_not_in(books_with_progress)); + } + let page_unread = page_query .join(JoinType::LeftJoin, books::Relation::BookMetadata.def()) .order_by_asc(books::Column::SeriesId) .order_by_asc(book_metadata::Column::Number) @@ -1726,35 +1790,19 @@ impl BookRepository { .order_by_asc(books::Column::FileName) .all(db) .await - .context("Failed to get unread books")?; + .context("Failed to get unread books for page")?; - // Step 6: Pick the first book from each series - let mut seen_series: std::collections::HashSet = std::collections::HashSet::new(); - let mut on_deck_books: Vec = Vec::new(); - - for book in all_unread_books { - if !seen_series.contains(&book.series_id) { - seen_series.insert(book.series_id); - on_deck_books.push(book); - } + let mut first_by_series: std::collections::HashMap = + std::collections::HashMap::new(); + for book in page_unread { + first_by_series.entry(book.series_id).or_insert(book); } - // Sort by most recently read series first (descending activity timestamp) - on_deck_books.sort_by(|a, b| { - let a_activity = series_last_read.get(&a.series_id); - let b_activity = series_last_read.get(&b.series_id); - b_activity.cmp(&a_activity) - }); - - let total = on_deck_books.len() as u64; - - // Apply pagination - let start = (page * page_size) as usize; - if start >= on_deck_books.len() { - return Ok((vec![], total)); - } - let end = (start + page_size as usize).min(on_deck_books.len()); - let paginated_books = on_deck_books[start..end].to_vec(); + // Emit in the (recency-ordered) page sequence. + let paginated_books: Vec = page_series + .iter() + .filter_map(|s| first_by_series.remove(s)) + .collect(); Ok((paginated_books, total)) } diff --git a/tests/api/books.rs b/tests/api/books.rs index 707fbfae..d821addf 100644 --- a/tests/api/books.rs +++ b/tests/api/books.rs @@ -806,6 +806,96 @@ async fn test_list_on_deck_empty_when_no_completed_books() { assert_eq!(book_list.total, 0); } +#[tokio::test] +async fn test_list_on_deck_paginates_across_series() { + use codex::db::repositories::{BookMetadataRepository, ReadProgressRepository}; + + let (db, _temp_dir) = setup_test_db().await; + let library = + LibraryRepository::create(&db, "Test Library", "/test", ScanningStrategy::Default) + .await + .unwrap(); + + let state = create_test_auth_state(db.clone()).await; + let password_hash = password::hash_password("admin123").unwrap(); + let admin = create_test_user("admin", "admin@example.com", &password_hash, true); + let admin_user = UserRepository::create(&db, &admin).await.unwrap(); + let token = state + .jwt_service + .generate_token( + admin_user.id, + admin_user.username.clone(), + admin_user.get_role(), + ) + .unwrap(); + + // 3 series, each with 1 completed book + several unread books. The first + // unread (book 2) of each series is the expected on-deck pick. + let mut expected_first_unread = std::collections::HashSet::new(); + for s in 0..3 { + let series = SeriesRepository::create(&db, library.id, &format!("Series {s}"), None) + .await + .unwrap(); + let mut ids = Vec::new(); + // 6 books per series: proves we return only the first unread, not all of them. + for i in 1..=6 { + let book = create_test_book_model( + series.id, + library.id, + &format!("/test/s{s}-book{i}.cbz"), + &format!("s{s}-book{i}.cbz"), + Some(format!("Book {i}")), + ); + let created = BookRepository::create(&db, &book, None).await.unwrap(); + BookMetadataRepository::create_with_title_and_number( + &db, + created.id, + Some(format!("Book {i}")), + Some(sea_orm::prelude::Decimal::from(i)), + ) + .await + .unwrap(); + ids.push(created.id); + } + // Complete book 1 → first unread is book 2. + ReadProgressRepository::upsert(&db, admin_user.id, ids[0], 10, true) + .await + .unwrap(); + expected_first_unread.insert(ids[1]); + } + + let app = create_test_router(state).await; + + // Page 1 (pageSize 2): 2 of 3 on-deck series. + let req = get_request_with_auth("/api/v1/books/on-deck?page=1&pageSize=2", &token); + let (status, resp): (StatusCode, Option) = + make_json_request(app.clone(), req).await; + assert_eq!(status, StatusCode::OK); + let page1 = resp.unwrap(); + assert_eq!(page1.total, 3, "three series have an on-deck book"); + assert_eq!(page1.data.len(), 2); + + // Page 2 (pageSize 2): the remaining series. This also guards the offset + // fix — the old code multiplied the offset by page_size and returned []. + let req2 = get_request_with_auth("/api/v1/books/on-deck?page=2&pageSize=2", &token); + let (status2, resp2): (StatusCode, Option) = + make_json_request(app, req2).await; + assert_eq!(status2, StatusCode::OK); + let page2 = resp2.unwrap(); + assert_eq!(page2.total, 3); + assert_eq!(page2.data.len(), 1, "page 2 returns the third series"); + + // Union across pages == the first-unread book of each series (one per series), + // with no duplicates — proving series-level pagination and first-per-series. + let returned: std::collections::HashSet<_> = page1 + .data + .iter() + .chain(page2.data.iter()) + .map(|b| b.id) + .collect(); + assert_eq!(returned, expected_first_unread); +} + // ============================================================================ // Recently Added Books Tests // ============================================================================ From f4737ecab1a70f844a9aa40590ed7831d6acb4eb Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Thu, 4 Jun 2026 17:56:43 -0700 Subject: [PATCH 08/11] perf(web): eliminate residual SSE refetch and task-poll storms Heavy background task processing still produced two frontend request storms after the earlier refetch-storm work. Both are client-side only. - release_announced: the SSE handler invalidated the releases inbox, per-series ledger, tracking, and the heavy series "full" query on every event, unthrottled. A release-source poll (or a source reset, which re-emits every release as announced) lands a burst of events, turning one detail-page refresh into a full/tracking/releases refetch flood. Now throttled through the same coalescing path the list uses, and the "full" invalidation is dropped entirely: a release advances tracking.latestKnownChapter, not local book counts or the upstream gap, so the Behind-by badge recomputes from the cheap tracking refetch alone. - List invalidation over-match: the throttled series/books list invalidators fired the bare ["series"] / ["books"] keys, which prefix-match every open detail query (full, tracking, aliases, releases, covers). With detail tabs open during a scan, one list refetch fanned out across all of them. Target the specific list/grid/home-section keys instead; genuinely-changed details are still refreshed by their targeted per-entity invalidations. - Task-progress poller fan-out: useTaskProgress is mounted in many places at once, and each instance ran its own poll loop plus immediate fetches on mount, multiplying the tasks/stats and processing-tasks requests. Rewrite it as a useSyncExternalStore subscriber over a single reference-counted manager that owns one poll loop and one SSE subscription, so the server sees one poller regardless of mount count. Public API and merge semantics are unchanged. Tests added for the new throttle/no-full release contract, the narrowed list invalidation, and shared-poller deduplication across mounts. --- web/src/hooks/useEntityEvents.test.ts | 70 +++- web/src/hooks/useEntityEvents.tsx | 113 ++++-- web/src/hooks/useTaskProgress.test.ts | 27 ++ web/src/hooks/useTaskProgress.ts | 540 +++++++++++++------------- 4 files changed, 450 insertions(+), 300 deletions(-) diff --git a/web/src/hooks/useEntityEvents.test.ts b/web/src/hooks/useEntityEvents.test.ts index d75bed88..71acfc98 100644 --- a/web/src/hooks/useEntityEvents.test.ts +++ b/web/src/hooks/useEntityEvents.test.ts @@ -194,13 +194,21 @@ describe("useEntityEvents", () => { } await waitFor(() => { + // The series LIST is refreshed via its section keys, not the bare + // ["series"] root (which would also nuke every open detail tab's + // full/tracking/aliases/releases queries — the refetch storm). expect(invalidateSpy).toHaveBeenCalledWith({ - queryKey: ["series"], + queryKey: ["series", "search"], }); expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ["libraries", "lib-2"], }); }); + + // Must NOT invalidate the bare root — that over-matches detail queries. + expect(invalidateSpy).not.toHaveBeenCalledWith({ + queryKey: ["series"], + }); }); it("should track connection state", async () => { @@ -285,12 +293,13 @@ describe("useEntityEvents", () => { expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ["library", "lib-123"], }); - // Should also invalidate books and series queries + // Should also refresh the books + series LISTS via their section keys + // (not the bare roots, which would also refetch open detail tabs). expect(invalidateSpy).toHaveBeenCalledWith({ - queryKey: ["books"], + queryKey: ["books", "search"], }); expect(invalidateSpy).toHaveBeenCalledWith({ - queryKey: ["series"], + queryKey: ["series", "search"], }); }); }); @@ -484,6 +493,59 @@ describe("useEntityEvents", () => { }); }); + it("refreshes release/tracking views on release_announced but NOT the heavy series 'full' query", async () => { + let capturedCallback: ((event: EntityChangeEvent) => void) | undefined; + + vi.spyOn(eventsApi.eventsApi, "subscribeToEntityEvents").mockImplementation( + (onEvent) => { + capturedCallback = onEvent; + return mockUnsubscribe; + }, + ); + + const invalidateSpy = vi.spyOn(queryClient, "invalidateQueries"); + + renderHook(() => useEntityEvents(), { wrapper }); + + await waitFor(() => { + expect(capturedCallback).toBeDefined(); + }); + + const event = { + type: "release_announced", + seriesId: "series-789", + seriesTitle: "Test Series", + pluginId: "release-nyaa", + language: "en", + chapter: 42, + volume: null, + ledgerId: "ledger-1", + sourceId: "source-1", + timestamp: "2026-01-07T12:00:00Z", + } as unknown as EntityChangeEvent; + + capturedCallback?.(event); + + await waitFor(() => { + // Shared inbox/facets + this series' ledger and tracking row refresh. + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ["releases"] }); + expect(invalidateSpy).toHaveBeenCalledWith({ + queryKey: ["series", "series-789", "releases"], + }); + expect(invalidateSpy).toHaveBeenCalledWith({ + queryKey: ["series", "series-789", "tracking"], + }); + }); + + // The heavy series detail assembly must NOT be refetched on a release: a + // release advances tracking.latestKnownChapter, not localMaxChapter or the + // upstream gap, so the Behind-by badge recomputes from the tracking refetch + // alone. Refetching "full" per event was the ?full=true storm. + expect(invalidateSpy).not.toHaveBeenCalledWith({ + queryKey: ["series", "series-789", "full"], + }); + }); + it("should handle errors gracefully", async () => { const consoleError = vi .spyOn(console, "error") diff --git a/web/src/hooks/useEntityEvents.tsx b/web/src/hooks/useEntityEvents.tsx index 8b19e258..f9831bd4 100644 --- a/web/src/hooks/useEntityEvents.tsx +++ b/web/src/hooks/useEntityEvents.tsx @@ -16,6 +16,28 @@ type ConnectionState = "connecting" | "connected" | "disconnected" | "failed"; const log = createDevLog("[SSE]"); +/** Second key segment of every series LIST/grid/home-section query (see + * SeriesSection / SeriesSection home rows). Detail queries are keyed + * ["series", , ...] instead, so invalidating these prefixes refreshes the + * lists without touching open detail tabs. Keep in sync with the query keys in + * the components that own these lists. */ +const SERIES_LIST_SECTIONS = [ + "search", + "alphabetical-groups", + "recently-added", + "recently-updated", +] as const; + +/** Second key segment of every book LIST/home-section query (see Recommended + * section rows + the books grid). Detail queries are ["books", , ...]. */ +const BOOKS_LIST_SECTIONS = [ + "search", + "in-progress", + "on-deck", + "recently-added", + "recently-read", +] as const; + /** Per-series state for the aggregated release toast. Lives at module scope * so a burst of `release_announced` events for the same series collapses * into a single toast that updates in place rather than spawning N toasts. @@ -138,17 +160,59 @@ export function useEntityEvents() { // Throttle the heavy list invalidations: a burst of entity events (e.g. a // scan or bulk thumbnail regen) collapses into at most one series/books // list refetch per interval instead of one per event. - const invalidateSeriesList = throttle( - () => queryClient.invalidateQueries({ queryKey: ["series"] }), - 1500, - ); - const invalidateBooksList = throttle( - () => queryClient.invalidateQueries({ queryKey: ["books"] }), - 1500, - ); + // + // IMPORTANT: invalidate the specific list/grid/home-section query keys, NOT + // the bare ["series"] / ["books"] roots. React Query matches by prefix, so + // ["series"] also matches every open detail query — ["series", id, "full"], + // "tracking", "aliases", "releases", "covers" — and ["books"] matches + // ["books", id, ...]. With several detail tabs open during a scan, the bare + // root turned one list refetch into a full+tracking+aliases+releases refetch + // wave on every tab. Targeting the section prefixes refreshes the lists + // (counts, membership) while leaving open detail tabs untouched; a detail + // that genuinely changed is refreshed by the targeted ["series", id] / + // ["books", id] invalidation in the per-event branches below. + const invalidateSeriesList = throttle(() => { + for (const section of SERIES_LIST_SECTIONS) { + queryClient.invalidateQueries({ queryKey: ["series", section] }); + } + }, 1500); + const invalidateBooksList = throttle(() => { + for (const section of BOOKS_LIST_SECTIONS) { + queryClient.invalidateQueries({ queryKey: ["books", section] }); + } + }, 1500); + + // Coalesce a burst of `release_announced` events (a poll or a source reset + // can announce hundreds of releases in one wave) into a single refetch + // wave — the release-tracking equivalent of the list throttle above. + // Affected series IDs accumulate between flushes; the throttled flush + // refetches the shared inbox/facets once plus each touched series' ledger + // and tracking row, then clears the set. + const pendingReleaseSeriesIds = new Set(); + const flushReleaseInvalidations = throttle(() => { + // Inbox + facets (keyed ["releases", ...]). One refetch per wave instead + // of one per event. + queryClient.invalidateQueries({ queryKey: ["releases"] }); + for (const id of pendingReleaseSeriesIds) { + // Per-series ledger view + the tracking row that drives the Behind-by + // badge's moving value (latest_known_*). The heavy ["series", id, + // "full"] query is intentionally NOT invalidated: a release + // announcement advances tracking.latestKnownChapter, not the series' + // localMaxChapter (local books) or upstream gap (metadata), so the + // badge recomputes from the tracking refetch alone. + queryClient.invalidateQueries({ queryKey: ["series", id, "releases"] }); + queryClient.invalidateQueries({ queryKey: ["series", id, "tracking"] }); + } + pendingReleaseSeriesIds.clear(); + }, 1500); + const listInvalidate: ListInvalidators = { series: invalidateSeriesList, books: invalidateBooksList, + release: (seriesId: string) => { + pendingReleaseSeriesIds.add(seriesId); + flushReleaseInvalidations(); + }, }; const unsubscribe = eventsApi.subscribeToEntityEvents( @@ -169,6 +233,7 @@ export function useEntityEvents() { // Drop any pending trailing refetch so we don't fire after teardown/logout. invalidateSeriesList.cancel(); invalidateBooksList.cancel(); + flushReleaseInvalidations.cancel(); }; }, [queryClient, isAuthenticated]); @@ -185,7 +250,14 @@ export function useEntityEvents() { * one would refetch it hundreds of times. Throttling coalesces a burst into at * most one refetch per interval (leading + trailing), so a single event still * updates promptly while a sustained stream refreshes ~once/interval. */ -type ListInvalidators = { series: () => void; books: () => void }; +type ListInvalidators = { + series: () => void; + books: () => void; + /** Queue a throttled refetch wave for release views touched by a + * `release_announced` burst (shared inbox/facets + this series' ledger and + * tracking row). Coalesces a poll/reset storm into ~one wave per interval. */ + release: (seriesId: string) => void; +}; function handleEntityEvent( event: EntityChangeEvent, @@ -363,22 +435,13 @@ function handleEntityEvent( } useReleaseAnnouncementsStore.getState().bump(); - // Refresh inbox + per-series ledger views in case the user is - // watching them. - queryClient.invalidateQueries({ queryKey: ["releases"] }); - queryClient.invalidateQueries({ - queryKey: ["series", event.seriesId, "releases"], - }); - // Refresh the series tracking row so the Behind-by-N badge can - // pick up the latest_known_* high-water mark advance. - queryClient.invalidateQueries({ - queryKey: ["series", event.seriesId, "tracking"], - }); - // Refresh the full series so localMaxChapter / upstream gap props - // recompute against the latest state. - queryClient.invalidateQueries({ - queryKey: ["series", event.seriesId, "full"], - }); + // Refresh inbox + per-series ledger + the tracking row (drives the + // Behind-by-N badge's latest_known_* high-water mark) — but throttled, + // so a poll/reset announcing hundreds of releases coalesces into ~one + // refetch wave instead of one heavy cascade per event. The series' + // heavy ["...","full"] query is deliberately not refetched here (see the + // flush comment): a release advances tracking, not local book counts. + listInvalidate.release(event.seriesId); // Surface a low-priority toast. To avoid spamming the user when a // single poll lands a dozen releases for one series, we aggregate by diff --git a/web/src/hooks/useTaskProgress.test.ts b/web/src/hooks/useTaskProgress.test.ts index 32f1f0d7..8cae3c90 100644 --- a/web/src/hooks/useTaskProgress.test.ts +++ b/web/src/hooks/useTaskProgress.test.ts @@ -140,6 +140,33 @@ describe("useTaskProgress", () => { expect(mockUnsubscribe).toHaveBeenCalled(); }); + it("shares one poller + SSE subscription across many mounted instances", () => { + const mockSubscribe = vi + .spyOn(tasksApi, "subscribeToTaskProgress") + .mockReturnValue(mockUnsubscribe); + + // Three components reading task progress at once (e.g. the global + // indicator + badge + a settings page). + const a = renderHook(() => useTaskProgress()); + const b = renderHook(() => useTaskProgress()); + const c = renderHook(() => useTaskProgress()); + + // Exactly one SSE subscription and one initial poll regardless of mounts — + // this is the whole point of the shared manager (no per-instance request + // storm). + expect(mockSubscribe).toHaveBeenCalledTimes(1); + expect(tasksApi.fetchPendingTaskCounts).toHaveBeenCalledTimes(1); + expect(tasksApi.fetchTasksByStatus).toHaveBeenCalledTimes(1); + + // The shared subscription stays alive until the LAST consumer unmounts. + a.unmount(); + b.unmount(); + expect(mockUnsubscribe).not.toHaveBeenCalled(); + + c.unmount(); + expect(mockUnsubscribe).toHaveBeenCalledTimes(1); + }); + it("should track active tasks", () => { let capturedCallback: ((event: TaskProgressEvent) => void) | undefined; diff --git a/web/src/hooks/useTaskProgress.ts b/web/src/hooks/useTaskProgress.ts index d7693e44..3422afda 100644 --- a/web/src/hooks/useTaskProgress.ts +++ b/web/src/hooks/useTaskProgress.ts @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useCallback, useSyncExternalStore } from "react"; import { fetchPendingTaskCounts, fetchTasksByStatus, @@ -12,303 +12,301 @@ import { PERMISSIONS } from "@/types/permissions"; type ConnectionState = "connecting" | "connected" | "disconnected" | "failed"; +/** Poll cadence for the processing-tasks / pending-counts backstop. SSE pushes + * live progress; this catches anything the event stream missed. */ +const POLL_INTERVAL_MS = 10_000; +/** How long a completed/failed task lingers in the active list so the UI can + * show its terminal state before it disappears. */ +const COMPLETED_TASK_LINGER_MS = 5_000; + +type TaskProgressSnapshot = { + activeTasks: ActiveTask[]; + connectionState: ConnectionState; + pendingCounts: PendingTaskCounts; +}; + +const EMPTY_SNAPSHOT: TaskProgressSnapshot = { + activeTasks: [], + connectionState: "disconnected", + pendingCounts: {}, +}; + +const sortTasks = (tasks: ActiveTask[]): ActiveTask[] => + [...tasks].sort((a, b) => a.taskType.localeCompare(b.taskType)); + +const is401 = (error: unknown): boolean => + error instanceof Error && error.message.includes("401"); + +/** Convert a `GET /api/v1/tasks` row into the frontend ActiveTask shape. + * Titles come from the polling snapshot; SSE events do not carry them. */ +function convertTaskToEvent(task: { + id: string; + taskType: string; + status: string; + libraryId?: string | null; + seriesId?: string | null; + bookId?: string | null; + startedAt?: string | null; + bookTitle?: string | null; + seriesTitle?: string | null; + libraryName?: string | null; +}): ActiveTask { + // Map "processing" status to "running" for UI consistency. + const status: TaskStatus = + task.status === "processing" ? "running" : (task.status as TaskStatus); + + return { + taskId: task.id, + taskType: task.taskType, + status, + progress: undefined, + error: undefined, + startedAt: task.startedAt ?? new Date().toISOString(), + completedAt: undefined, + libraryId: task.libraryId ?? undefined, + seriesId: task.seriesId ?? undefined, + bookId: task.bookId ?? undefined, + bookTitle: task.bookTitle ?? undefined, + seriesTitle: task.seriesTitle ?? undefined, + libraryName: task.libraryName ?? undefined, + }; +} + /** - * Hook to subscribe to task progress events and track active tasks - * - * This hook maintains a map of active tasks and their current progress, - * automatically subscribing to the task progress SSE stream. - * - * Features: - * - Automatic subscription/cleanup - * - Connection state tracking - * - Active task tracking - * - Task completion/failure cleanup - * - Permission-aware: skips task API calls if user lacks TASKS_READ permission + * Single shared source for task-progress state. * - * @returns Object with active tasks and connection state + * `useTaskProgress` is mounted in many places at once (the global progress + * indicator + notification badge, the library page, release hooks, several + * settings pages, …). If every instance ran its own poll + SSE handler, a busy + * server produced N copies of the same `GET /tasks?status=processing` + + * `/tasks/stats` requests every interval — a self-inflicted request storm + * visible in the network panel. This manager owns exactly one poll loop and one + * SSE subscription, reference-counted across all hook subscribers (mirroring the + * SSE manager in `@/api/tasks`), and broadcasts an immutable snapshot via + * `useSyncExternalStore`. No matter how many components read task progress, the + * server sees one poller. */ -export function useTaskProgress() { - const { isAuthenticated } = useAuthStore(); - const { hasPermission } = usePermissions(); - const canReadTasks = hasPermission(PERMISSIONS.TASKS_READ); - const [activeTasks, setActiveTasks] = useState>( - new Map(), - ); - const [connectionState, setConnectionState] = - useState("disconnected"); - const [pendingCounts, setPendingCounts] = useState({}); +class TaskProgressManager { + private tasks = new Map(); + private pendingCounts: PendingTaskCounts = {}; + private connectionState: ConnectionState = "disconnected"; + private readonly listeners = new Set<() => void>(); + private pollTimer: ReturnType | null = null; + private sseUnsubscribe: (() => void) | null = null; + /** Per-task deletion timers for terminal tasks (the 5s linger). Tracked so + * teardown can clear them and avoid firing after the last unsubscribe. */ + private readonly lingerTimers = new Map< + string, + ReturnType + >(); + /** Cached immutable snapshot. `useSyncExternalStore` requires a stable + * reference between changes, so this is only rebuilt inside `commit`. */ + private snapshot: TaskProgressSnapshot = EMPTY_SNAPSHOT; - // Track if we've already subscribed to prevent duplicate subscriptions - // when isAuthenticated briefly flips during Zustand hydration - const hasSubscribedRef = useRef(false); - - useEffect(() => { - if (!isAuthenticated) { - console.debug("Not authenticated, skipping task progress subscription"); - hasSubscribedRef.current = false; - return; + subscribe = (listener: () => void): (() => void) => { + this.listeners.add(listener); + if (this.listeners.size === 1) { + this.start(); } + return () => { + this.listeners.delete(listener); + if (this.listeners.size === 0) { + this.stop(); + } + }; + }; - if (!canReadTasks) { - console.debug( - "User lacks TASKS_READ permission, skipping task progress subscription", - ); - hasSubscribedRef.current = false; - return; - } + getSnapshot = (): TaskProgressSnapshot => this.snapshot; - // Prevent duplicate subscriptions from rapid effect re-runs - if (hasSubscribedRef.current) { - console.debug("Already subscribed, skipping duplicate subscription"); - return; + /** Rebuild the cached snapshot from current state and notify subscribers. */ + private commit() { + this.snapshot = { + activeTasks: sortTasks(Array.from(this.tasks.values())), + connectionState: this.connectionState, + pendingCounts: this.pendingCounts, + }; + for (const listener of this.listeners) { + listener(); } - hasSubscribedRef.current = true; - - // Convert API task response to ActiveTask format. Titles come from the - // polling snapshot (`GET /api/v1/tasks`); SSE events do not carry them. - const convertTaskToEvent = (task: { - id: string; - taskType: string; - status: string; - libraryId?: string | null; - seriesId?: string | null; - bookId?: string | null; - startedAt?: string | null; - bookTitle?: string | null; - seriesTitle?: string | null; - libraryName?: string | null; - }): ActiveTask => { - // Map "processing" status to "running" for UI consistency - const status: TaskStatus = - task.status === "processing" ? "running" : (task.status as TaskStatus); + } - return { - taskId: task.id, - taskType: task.taskType, - status, - progress: undefined, - error: undefined, - startedAt: task.startedAt ?? new Date().toISOString(), - completedAt: undefined, - libraryId: task.libraryId ?? undefined, - seriesId: task.seriesId ?? undefined, - bookId: task.bookId ?? undefined, - bookTitle: task.bookTitle ?? undefined, - seriesTitle: task.seriesTitle ?? undefined, - libraryName: task.libraryName ?? undefined, - }; - }; + private start() { + // Prime immediately, then poll as a backstop to the SSE stream. + void this.refreshPendingCounts(); + void this.refreshProcessingTasks("replace"); + this.pollTimer = setInterval(() => { + void this.refreshPendingCounts(); + void this.refreshProcessingTasks("preserve"); + }, POLL_INTERVAL_MS); + this.sseUnsubscribe = subscribeToTaskProgress( + this.handleEvent, + this.handleError, + this.handleConnectionStateChange, + ); + } - // Fetch initial pending task counts - fetchPendingTaskCounts() - .then((counts) => { - console.debug("Initial pending task counts:", counts); - setPendingCounts(counts); - }) - .catch((error) => { - // Only log non-401 errors - if (!(error instanceof Error && error.message.includes("401"))) { - console.error("Failed to fetch pending task counts:", error); - } - }); + private stop() { + if (this.pollTimer) { + clearInterval(this.pollTimer); + this.pollTimer = null; + } + this.sseUnsubscribe?.(); + this.sseUnsubscribe = null; + for (const timer of this.lingerTimers.values()) { + clearTimeout(timer); + } + this.lingerTimers.clear(); + this.tasks.clear(); + this.pendingCounts = {}; + this.connectionState = "disconnected"; + // No subscribers remain — reset the cached snapshot so the next subscriber + // starts clean without an extra notify. + this.snapshot = EMPTY_SNAPSHOT; + } - // Fetch initial processing tasks and add them to activeTasks - fetchTasksByStatus("processing", 100) - .then((tasks) => { - console.debug("Initial processing tasks:", tasks); - setActiveTasks((prev) => { - const next = new Map(prev); - // Create a set of current processing task IDs - const currentProcessingIds = new Set(tasks.map((task) => task.id)); + private refreshPendingCounts = async () => { + try { + this.pendingCounts = await fetchPendingTaskCounts(); + this.commit(); + } catch (error) { + if (!is401(error)) { + console.error("Failed to fetch pending task counts:", error); + } + } + }; - // Remove tasks that were previously "running" (from processing) - // but are no longer in the processing list - // Preserve tasks with "completed" or "failed" status (from SSE) - for (const [taskId, task] of prev.entries()) { - if ( - task.status === "running" && - !currentProcessingIds.has(taskId) - ) { - next.delete(taskId); - } - } + /** + * Poll the processing list and reconcile it into `tasks`. + * + * - `"replace"` (initial load): trust the snapshot fully, overwriting rows. + * - `"preserve"` (subsequent polls): SSE is authoritative for live progress, + * so only fill in tasks the stream hasn't already enriched (new tasks, or + * stale `running` rows with no progress yet). Either way, drop `running` + * tasks that have fallen out of the processing list; keep terminal + * (completed/failed) tasks so their linger timer removes them. + */ + private refreshProcessingTasks = async (mode: "replace" | "preserve") => { + try { + const tasks = await fetchTasksByStatus("processing", 100); + const currentIds = new Set(tasks.map((task) => task.id)); - // Add or update tasks that are currently processing - for (const task of tasks) { - const event = convertTaskToEvent(task); - next.set(event.taskId, event); - } - return next; - }); - }) - .catch((error) => { - // Only log non-401 errors - if (!(error instanceof Error && error.message.includes("401"))) { - console.error("Failed to fetch processing tasks:", error); + for (const [taskId, task] of this.tasks) { + if (task.status === "running" && !currentIds.has(taskId)) { + this.tasks.delete(taskId); } - }); + } - // Poll for pending counts every 10 seconds - const pollInterval = setInterval(() => { - fetchPendingTaskCounts() - .then((counts) => { - setPendingCounts(counts); - }) - .catch((error) => { - // Only log non-401 errors - if (!(error instanceof Error && error.message.includes("401"))) { - console.error("Failed to fetch pending task counts:", error); - } - }); - - // Also poll for processing tasks to catch any that weren't sent via SSE - fetchTasksByStatus("processing", 100) - .then((tasks) => { - setActiveTasks((prev) => { - const next = new Map(prev); - // Create a set of current processing task IDs - const currentProcessingIds = new Set(tasks.map((task) => task.id)); - - // Remove tasks that were previously "running" (from processing) - // but are no longer in the processing list - // Preserve tasks with "completed" or "failed" status (from SSE) - // These are kept for 5 seconds to show completion state - for (const [taskId, task] of prev.entries()) { - if ( - task.status === "running" && - !currentProcessingIds.has(taskId) - ) { - next.delete(taskId); - } - // Explicitly preserve completed/failed tasks (they're removed by setTimeout in handleEvent) - // Don't remove them here even if they're not in the processing list - } + for (const task of tasks) { + const event = convertTaskToEvent(task); + if (mode === "replace") { + this.tasks.set(event.taskId, event); + continue; + } + const existing = this.tasks.get(event.taskId); + if ( + !existing || + (existing.status === "running" && + !existing.progress && + !existing.completedAt) + ) { + this.tasks.set(event.taskId, event); + } + } - // Add or update tasks that are currently processing - // SSE events take precedence, so we only update if: - // - Task doesn't exist yet, OR - // - Task exists with "running" status and no progress (from previous poll) - // Don't overwrite if SSE has updated it (has progress or different status) - for (const task of tasks) { - const event = convertTaskToEvent(task); - const existing = next.get(event.taskId); - if ( - !existing || - (existing.status === "running" && - !existing.progress && - !existing.completedAt) - ) { - next.set(event.taskId, event); - } - } - return next; - }); - }) - .catch((error) => { - // Only log non-401 errors - if (!(error instanceof Error && error.message.includes("401"))) { - console.error("Failed to fetch processing tasks:", error); - } - }); - }, 10000); + this.commit(); + } catch (error) { + if (!is401(error)) { + console.error("Failed to fetch processing tasks:", error); + } + } + }; - const handleEvent = (event: TaskProgressEvent) => { - setActiveTasks((prev) => { - const next = new Map(prev); + private handleEvent = (event: TaskProgressEvent) => { + if (event.status === "completed" || event.status === "failed") { + const pending = this.lingerTimers.get(event.taskId); + if (pending) { + clearTimeout(pending); + } + this.lingerTimers.set( + event.taskId, + setTimeout(() => { + this.tasks.delete(event.taskId); + this.lingerTimers.delete(event.taskId); + this.commit(); + }, COMPLETED_TASK_LINGER_MS), + ); + } - // Remove completed or failed tasks after a delay - if (event.status === "completed" || event.status === "failed") { - // Keep the event for 5 seconds so UI can show completion - setTimeout(() => { - setActiveTasks((current) => { - const updated = new Map(current); - updated.delete(event.taskId); - return updated; - }); - }, 5000); - } + // SSE events do not carry resolved target titles. Preserve any titles + // stashed from the most recent polling snapshot so the UI keeps showing the + // human-readable label across progress updates. + const existing = this.tasks.get(event.taskId); + this.tasks.set(event.taskId, { + ...event, + bookTitle: existing?.bookTitle, + seriesTitle: existing?.seriesTitle, + libraryName: existing?.libraryName, + }); + this.commit(); + }; - // SSE events do not carry resolved target titles. Preserve any titles - // already stashed on this task from the most recent polling snapshot - // so the UI keeps showing the human-readable label across progress - // updates. - const existing = prev.get(event.taskId); - next.set(event.taskId, { - ...event, - bookTitle: existing?.bookTitle, - seriesTitle: existing?.seriesTitle, - libraryName: existing?.libraryName, - }); - return next; - }); - }; + private handleError = (error: Error) => { + console.error("Task progress subscription error:", error); + }; - const handleError = (error: Error) => { - console.error("Task progress subscription error:", error); - }; + private handleConnectionStateChange = (state: ConnectionState) => { + this.connectionState = state; + this.commit(); + }; +} - const handleConnectionStateChange = (state: ConnectionState) => { - console.debug("Task progress connection state:", state); - setConnectionState(state); - }; +const taskProgressManager = new TaskProgressManager(); - console.debug("Subscribing to task progress events..."); - const unsubscribe = subscribeToTaskProgress( - handleEvent, - handleError, - handleConnectionStateChange, - ); +/** + * Hook to read task-progress state (active tasks, pending counts, connection + * state). All instances share one poll loop and one SSE subscription via + * {@link TaskProgressManager}, so mounting this hook in many components costs no + * extra network traffic. + * + * Permission-aware: a user without `TASKS_READ` (or while unauthenticated) never + * subscribes, so the shared poller/SSE never starts for them. + */ +export function useTaskProgress() { + const { isAuthenticated } = useAuthStore(); + const { hasPermission } = usePermissions(); + const enabled = isAuthenticated && hasPermission(PERMISSIONS.TASKS_READ); - return () => { - console.debug("Unsubscribing from task progress events"); - hasSubscribedRef.current = false; - clearInterval(pollInterval); - unsubscribe(); - }; - }, [isAuthenticated, canReadTasks]); + const subscribe = useCallback( + (listener: () => void) => + enabled ? taskProgressManager.subscribe(listener) : () => {}, + [enabled], + ); + const getSnapshot = useCallback( + () => (enabled ? taskProgressManager.getSnapshot() : EMPTY_SNAPSHOT), + [enabled], + ); - // Sort helper for consistent ordering (by task_type alphabetically) - const sortTasks = (tasks: ActiveTask[]): ActiveTask[] => - tasks.sort((a, b) => a.taskType.localeCompare(b.taskType)); + const { activeTasks, connectionState, pendingCounts } = useSyncExternalStore( + subscribe, + getSnapshot, + getSnapshot, + ); return { - /** - * Array of active tasks (sorted by task_type for consistent UI ordering) - */ - activeTasks: sortTasks(Array.from(activeTasks.values())), - /** - * Current SSE connection state - */ + /** Active tasks, sorted by task_type for consistent UI ordering. */ + activeTasks, + /** Current SSE connection state. */ connectionState, - /** - * Pending task counts by type - */ + /** Pending task counts by type. */ pendingCounts, - /** - * Get all tasks with a specific status (sorted by task_type) - */ - getTasksByStatus: (status: TaskStatus): ActiveTask[] => { - return sortTasks( - Array.from(activeTasks.values()).filter( - (task) => task.status === status, - ), - ); - }, - /** - * Get all tasks for a specific library (sorted by task_type) - */ - getTasksByLibrary: (libraryId: string): ActiveTask[] => { - return sortTasks( - Array.from(activeTasks.values()).filter( - (task) => task.libraryId === libraryId, - ), - ); - }, - /** - * Get a specific task by ID - */ - getTask: (taskId: string): ActiveTask | undefined => { - return activeTasks.get(taskId); - }, + /** All tasks with a specific status (already sorted by task_type). */ + getTasksByStatus: (status: TaskStatus): ActiveTask[] => + activeTasks.filter((task) => task.status === status), + /** All tasks for a specific library (already sorted by task_type). */ + getTasksByLibrary: (libraryId: string): ActiveTask[] => + activeTasks.filter((task) => task.libraryId === libraryId), + /** A specific task by ID. */ + getTask: (taskId: string): ActiveTask | undefined => + activeTasks.find((task) => task.taskId === taskId), }; } From d3f97e75e8ae5ed2dabae99fd1a47538633723ec Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Thu, 4 Jun 2026 20:41:48 -0700 Subject: [PATCH 09/11] perf(web): narrow per-event series/cover invalidations to the detail DTO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SSE handler invalidated the bare ["series", id] key on series update/metadata events and ["series"/"books", id] on cover events. React Query matches by prefix, so each of these also invalidated the entity's independent sub-resources — tracking config, aliases, and the release ledger for a series; genres, tags, and external links for a book — none of which a content/metadata change or a cover regeneration actually touches (they have their own event types). With several detail tabs open during an analyze run or a bulk thumbnail regeneration, that turned every per-entity event into a tracking+aliases+releases (or genres+tags) refetch burst for each affected entity. Target the specific detail queries instead: series content/metadata invalidates the full DTO plus the metadata view; a cover change invalidates only the detail DTO that carries the cover-source fields. There is no bare two-segment detail query, so nothing relied on the broad prefix. The book_updated branch keeps its broad invalidation deliberately: an analyze pass does rewrite a book's genres, tags, and metadata. Updates the cover-event tests to assert the narrowed keys. --- web/src/hooks/useEntityEvents.test.ts | 10 +++++-- web/src/hooks/useEntityEvents.tsx | 40 +++++++++++++++++---------- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/web/src/hooks/useEntityEvents.test.ts b/web/src/hooks/useEntityEvents.test.ts index 71acfc98..02edfc53 100644 --- a/web/src/hooks/useEntityEvents.test.ts +++ b/web/src/hooks/useEntityEvents.test.ts @@ -136,9 +136,9 @@ describe("useEntityEvents", () => { } await waitFor(() => { - // Only the specific series detail query is invalidated (cheap, targeted) + // Only the series detail DTO is invalidated (carries cover-source fields). expect(invalidateSpy).toHaveBeenCalledWith({ - queryKey: ["series", "series-123"], + queryKey: ["series", "series-123", "full"], }); }); @@ -148,6 +148,12 @@ describe("useEntityEvents", () => { expect(invalidateSpy).not.toHaveBeenCalledWith({ queryKey: ["series"], }); + // ...nor the bare ["series", id] prefix, which would also drag in the + // series' tracking/aliases/releases queries that a cover change never + // touches (a thumbnail-regen burst amplifier). + expect(invalidateSpy).not.toHaveBeenCalledWith({ + queryKey: ["series", "series-123"], + }); expect(refetchSpy).not.toHaveBeenCalledWith({ queryKey: ["series"], type: "active", diff --git a/web/src/hooks/useEntityEvents.tsx b/web/src/hooks/useEntityEvents.tsx index f9831bd4..06a0da5e 100644 --- a/web/src/hooks/useEntityEvents.tsx +++ b/web/src/hooks/useEntityEvents.tsx @@ -303,20 +303,29 @@ function handleEntityEvent( // emitting many series events coalesces into a handful of refetches. listInvalidate.series(); - // Invalidate specific series if it's an update + // Invalidate specific series if it's an update. if ( event.type === "series_updated" || event.type === "series_metadata_updated" ) { - queryClient.invalidateQueries({ - queryKey: ["series", event.seriesId], - }); - // For metadata updates, also refetch active queries to immediately update the UI - if (event.type === "series_metadata_updated") { - queryClient.refetchQueries({ - queryKey: ["series", event.seriesId], - type: "active", - }); + // A content/metadata change affects the detail DTO and the metadata + // view — NOT the independent sub-resources (tracking config, aliases, + // release ledger), which change only via their own event types + // (release_announced, etc.). Invalidating the bare ["series", id] + // prefix would refetch all of them: an analyze run over many series + // with detail tabs open turned that into a needless + // tracking+aliases+releases refetch burst per analyzed series. + const detailKeys = [ + ["series", event.seriesId, "full"], + ["series", event.seriesId, "metadata"], + ] as const; + for (const queryKey of detailKeys) { + queryClient.invalidateQueries({ queryKey }); + // For metadata updates, refetch the active detail views immediately + // so the open page reflects the change without waiting for staleness. + if (event.type === "series_metadata_updated") { + queryClient.refetchQueries({ queryKey, type: "active" }); + } } } @@ -348,15 +357,18 @@ function handleEntityEvent( // refetch is needed. Re-pulling the whole series/books list on every // cover event caused a refetch storm during bulk thumbnail regeneration // (one heavy refetch per series, hundreds of them). So invalidate only - // the specific entity's detail query (cheap, keeps cover-source fields - // current); never the list. + // the entity's DETAIL DTO (which carries the cover-source fields), not + // the bare ["series"/"books", id] prefix — that prefix also matches the + // independent sub-resources (tracking/aliases/releases for series, + // genres/tags/external-* for books) that a cover change never touches, + // and bulk thumbnail regen turned that into a per-entity refetch burst. if (event.entityType === "book") { queryClient.invalidateQueries({ - queryKey: ["books", event.entityId], + queryKey: ["books", event.entityId, "detail"], }); } else if (event.entityType === "series") { queryClient.invalidateQueries({ - queryKey: ["series", event.entityId], + queryKey: ["series", event.entityId, "full"], }); } break; From 6998161377b18604f182ce44f4ba5ca496e7a210 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Thu, 4 Jun 2026 21:00:44 -0700 Subject: [PATCH 10/11] perf(web): narrow over-broad query invalidations and throttle task-page refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audited every frontend invalidation against the actual read-query keys and fixed the remaining cases that refetch far more than they change, plus the one burst-driven refresh that wasn't throttled. - Release mutations (dismiss/acquire/patch/delete/bulk/reset) invalidated the bare ["series"] key. The series DTO carries no release-derived fields, so the only series query a release change affects is the per-series ledger panel — but the bare key, via prefix matching, also refetched full/metadata/covers/tracking/aliases/lists across every open tab. Replace with a predicate targeting only the release ledger panels (and the tracking row for ledger-wiping resets). - MediaCard analyze actions invalidated the whole ["series"]/["books"] namespace on enqueue, but analysis is async: nothing has changed yet, and the real refresh arrives via the completion SSE event. Narrow to the single entity's detail query. - The tasks settings page refetched the task list (four status fetches under the "all" filter) plus stats on every task-completion SSE event, unthrottled — a request storm when a bulk analyze or scan completes thousands of tasks. Throttle the refresh and keep the existing periodic poll as the backstop; the side effect also moves out of the state updater to keep it pure. Left untouched: per-entity invalidations that fire once per save on the entity being edited (one-shot, single entity, no burst), and the many mutation onSuccess invalidations that are already correctly scoped. --- web/src/components/library/MediaCard.tsx | 27 ++++++++++++-- web/src/hooks/useReleases.ts | 46 +++++++++++++++++++----- web/src/pages/settings/TasksSettings.tsx | 27 +++++++++++--- 3 files changed, 84 insertions(+), 16 deletions(-) diff --git a/web/src/components/library/MediaCard.tsx b/web/src/components/library/MediaCard.tsx index 07f802df..cde42d96 100644 --- a/web/src/components/library/MediaCard.tsx +++ b/web/src/components/library/MediaCard.tsx @@ -180,7 +180,15 @@ export const MediaCard = memo(function MediaCard({ message: "Book analysis has been queued", color: "blue", }); - queryClient.invalidateQueries({ queryKey: ["books"] }); + // Analysis is async: only a task is queued here, nothing has changed yet. + // The real refresh arrives via the book_updated SSE event on completion, + // so nudge just this book's detail (cheap) rather than the whole + // ["books"] namespace, which would refetch every open list/detail tab. + if (book) { + queryClient.invalidateQueries({ + queryKey: ["books", book.id, "detail"], + }); + } }, onError: (error: Error) => { notifications.show({ @@ -203,7 +211,14 @@ export const MediaCard = memo(function MediaCard({ message: "All books in series queued for analysis", color: "blue", }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + // Async enqueue — refresh just this series' detail; the series_metadata + // /book_updated SSE events refresh the rest on completion (was ["series"], + // which refetched every open detail tab + list for nothing). + if (series) { + queryClient.invalidateQueries({ + queryKey: ["series", series.id, "full"], + }); + } }, onError: (error: Error) => { notifications.show({ @@ -225,7 +240,13 @@ export const MediaCard = memo(function MediaCard({ message: "Unanalyzed books queued for analysis", color: "blue", }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + // Async enqueue — refresh just this series' detail; SSE refreshes the + // rest on completion (was the whole ["series"] namespace). + if (series) { + queryClient.invalidateQueries({ + queryKey: ["series", series.id, "full"], + }); + } }, onError: (error: Error) => { notifications.show({ diff --git a/web/src/hooks/useReleases.ts b/web/src/hooks/useReleases.ts index fe39148b..e0d0ab42 100644 --- a/web/src/hooks/useReleases.ts +++ b/web/src/hooks/useReleases.ts @@ -1,5 +1,10 @@ import { notifications } from "@mantine/notifications"; -import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { + type QueryClient, + useMutation, + useQuery, + useQueryClient, +} from "@tanstack/react-query"; import { useEffect, useRef } from "react"; import { type BulkReleaseActionRequest, @@ -34,6 +39,31 @@ export const releasesKeys = { sourcesRoot: ["release-sources"] as const, }; +/** + * Refresh the per-series release views touched by a release-state change. + * + * Release ledger panels are keyed `["series", id, "releases", …]`, which do NOT + * start with `["releases"]`, so the inbox/facets invalidation misses them. The + * previous code reached them with a bare `["series"]` invalidation — but that + * prefix-matches *every* series query (full, metadata, covers, tracking, + * aliases, and all the lists/home sections), none of which a release-state + * change touches (the series DTO carries no release-derived fields). With + * several detail tabs open, one dismiss/acquire became a full-namespace + * refetch. This targets only the release ledger panels (and, for ledger-wiping + * resets, the tracking row whose latest_known high-water mark can move). + */ +function invalidateSeriesReleaseViews( + queryClient: QueryClient, + opts?: { tracking?: boolean }, +) { + queryClient.invalidateQueries({ + predicate: (query) => + query.queryKey[0] === "series" && + (query.queryKey[2] === "releases" || + (opts?.tracking === true && query.queryKey[2] === "tracking")), + }); +} + export function useReleaseFacets(params: ReleaseFacetsParams = {}) { return useQuery({ queryKey: releasesKeys.facets(params), @@ -49,7 +79,7 @@ export function useDeleteRelease() { // Delete touches the ledger and (server-side) the source's etag. // Invalidate both so the table and the source-admin row refresh. queryClient.invalidateQueries({ queryKey: ["releases"] }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + invalidateSeriesReleaseViews(queryClient); queryClient.invalidateQueries({ queryKey: releasesKeys.sourcesRoot }); }, onError: notifyError("Failed to delete release"), @@ -88,7 +118,7 @@ export function useBulkReleaseAction() { color: action === "delete" ? "orange" : "blue", }); queryClient.invalidateQueries({ queryKey: ["releases"] }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + invalidateSeriesReleaseViews(queryClient); if (action === "delete") { queryClient.invalidateQueries({ queryKey: releasesKeys.sourcesRoot }); } @@ -132,7 +162,7 @@ export function useDismissRelease() { mutationFn: (releaseId) => releasesApi.dismiss(releaseId), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["releases"] }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + invalidateSeriesReleaseViews(queryClient); }, onError: notifyError("Failed to dismiss release"), }); @@ -144,7 +174,7 @@ export function useMarkReleaseAcquired() { mutationFn: (releaseId) => releasesApi.markAcquired(releaseId), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["releases"] }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + invalidateSeriesReleaseViews(queryClient); }, onError: notifyError("Failed to mark release acquired"), }); @@ -161,7 +191,7 @@ export function usePatchRelease() { releasesApi.patchEntry(releaseId, update), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["releases"] }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + invalidateSeriesReleaseViews(queryClient); }, onError: notifyError("Failed to update release"), }); @@ -292,7 +322,7 @@ export function useResetReleaseSource() { // Reset wipes ledger rows, so invalidate everything that reads them. queryClient.invalidateQueries({ queryKey: releasesKeys.sourcesRoot }); queryClient.invalidateQueries({ queryKey: releasesKeys.inboxRoot }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + invalidateSeriesReleaseViews(queryClient, { tracking: true }); }, onError: notifyError("Failed to reset source"), }); @@ -323,7 +353,7 @@ export function useResetAllReleaseSources() { // covers everything that reads ledger rows. queryClient.invalidateQueries({ queryKey: releasesKeys.sourcesRoot }); queryClient.invalidateQueries({ queryKey: releasesKeys.inboxRoot }); - queryClient.invalidateQueries({ queryKey: ["series"] }); + invalidateSeriesReleaseViews(queryClient, { tracking: true }); }, onError: notifyError("Failed to reset sources"), }); diff --git a/web/src/pages/settings/TasksSettings.tsx b/web/src/pages/settings/TasksSettings.tsx index 747669ce..0521c3b8 100644 --- a/web/src/pages/settings/TasksSettings.tsx +++ b/web/src/pages/settings/TasksSettings.tsx @@ -26,6 +26,7 @@ import { IconX, } from "@tabler/icons-react"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { throttle } from "es-toolkit"; import { useEffect, useState } from "react"; import { api } from "@/api/client"; import { @@ -213,27 +214,43 @@ export function TasksSettings() { // Subscribe to real-time task progress useEffect(() => { + // A bulk analyze/scan can complete thousands of tasks, each firing a + // terminal SSE event. Refetching the task list (4 status fetches when the + // filter is "all") + stats on every one is a request storm. Throttle the + // refresh so a completion flood coalesces into ≤1 refetch per interval; the + // 5s refetchInterval on both queries is the backstop for anything dropped. + const refreshTaskViews = throttle(() => { + refetchTasks(); + queryClient.invalidateQueries({ queryKey: ["task-stats"] }); + }, 1500); + const unsubscribe = subscribeToTaskProgress( (event) => { + const terminal = + event.status === "completed" || event.status === "failed"; setActiveProgress((prev) => { const next = new Map(prev); - if (event.status === "completed" || event.status === "failed") { + if (terminal) { next.delete(event.taskId); - // Refetch tasks when a task completes - refetchTasks(); - queryClient.invalidateQueries({ queryKey: ["task-stats"] }); } else { next.set(event.taskId, event); } return next; }); + // Side-effect kept out of the state updater (updaters must stay pure). + if (terminal) { + refreshTaskViews(); + } }, (error) => { console.error("Task progress error:", error); }, ); - return () => unsubscribe(); + return () => { + unsubscribe(); + refreshTaskViews.cancel(); + }; }, [refetchTasks, queryClient]); // Mutations From be6dd5836da89ad1a00742c3a183b68226258346 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Thu, 4 Jun 2026 21:45:16 -0700 Subject: [PATCH 11/11] perf: cut idle background fetching across SSE, task polling, and reference data Reduce the steady stream of background requests an open tab makes when nothing is happening. - SSE anti-buffering: the entity and task-progress streams now send X-Accel-Buffering: no so reverse proxies don't buffer the response and hold back the 15s keep-alive. Buffered keep-alives let an idle-timeout close the connection, forcing the client to reconnect every minute, which in turn re-runs refetch-on-reconnect across the whole app. - Adaptive task polling: the shared task-progress poller now backs off to a slow cadence when no task is running and only polls quickly while work is in flight, instead of polling at the fast cadence forever. SSE still announces new work instantly, which flips it back to the fast cadence. Polling survives fetch errors and re-arms correctly. - Reference data caching: all tags / genres "fetch every page" lookups in the filter panels and metadata editors move to a shared hook with a long staleTime. These lists change only on an explicit edit (which already invalidates them), so they no longer re-run the multi-page sweep on every remount or reconnect. Note: tags/genres remain keyed as before so existing invalidations and all consumers still dedupe to one query. --- .../src/routes/v1/handlers/events.rs | 30 ++++--- .../books/BookMetadataEditModal.tsx | 17 +--- .../components/library/BookFilterPanel.tsx | 21 ++--- .../library/BulkMetadataEditModal.tsx | 21 ++--- .../components/library/SeriesFilterPanel.tsx | 20 ++--- .../series/SeriesMetadataEditModal.tsx | 17 +--- web/src/hooks/useReferenceData.ts | 39 +++++++++ web/src/hooks/useTaskProgress.ts | 86 ++++++++++++++++--- 8 files changed, 158 insertions(+), 93 deletions(-) create mode 100644 web/src/hooks/useReferenceData.ts diff --git a/crates/codex-api/src/routes/v1/handlers/events.rs b/crates/codex-api/src/routes/v1/handlers/events.rs index 98a5dea8..356cb874 100644 --- a/crates/codex-api/src/routes/v1/handlers/events.rs +++ b/crates/codex-api/src/routes/v1/handlers/events.rs @@ -1,9 +1,11 @@ use crate::{AppState, error::ApiError, extractors::AuthContext, permissions::Permission}; use axum::{ extract::State, - response::sse::{Event, KeepAlive, Sse}, + response::{ + IntoResponse, + sse::{Event, KeepAlive, Sse}, + }, }; -use futures::stream::Stream; use std::{convert::Infallible, sync::Arc, time::Duration}; use tokio::time::timeout; use tracing::{debug, warn}; @@ -48,7 +50,7 @@ use tracing::{debug, warn}; pub async fn entity_events_stream( State(state): State>, auth: AuthContext, -) -> Result>>, ApiError> { +) -> Result { // Require read access to libraries auth.require_permission(&Permission::LibrariesRead)?; @@ -86,7 +88,7 @@ pub async fn entity_events_stream( match serde_json::to_string(&event) { Ok(json) => { debug!("Sending entity event to client: {:?}", event.event); - yield Ok(Event::default().data(json)); + yield Ok::(Event::default().data(json)); } Err(e) => { warn!("Failed to serialize entity event: {}", e); @@ -118,11 +120,16 @@ pub async fn entity_events_stream( debug!("Entity events stream ended for user {}", auth.user_id); }; - Ok(Sse::new(stream).keep_alive( + let sse = Sse::new(stream).keep_alive( KeepAlive::new() .interval(Duration::from_secs(15)) .text("keep-alive"), - )) + ); + // Disable proxy/CDN response buffering so the 15s keep-alive (and events) + // reach the client promptly. Without this, nginx-style proxies buffer the + // stream and an idle-timeout closes the connection, forcing the client to + // reconnect every ~minute (which then re-runs refetch-on-reconnect). + Ok(([("X-Accel-Buffering", "no")], sse)) } /// Subscribe to real-time task progress events via SSE @@ -169,7 +176,7 @@ pub async fn entity_events_stream( pub async fn task_progress_stream( State(state): State>, auth: AuthContext, -) -> Result>>, ApiError> { +) -> Result { // Require read access to libraries auth.require_permission(&Permission::LibrariesRead)?; @@ -210,7 +217,7 @@ pub async fn task_progress_stream( "Sending task progress event to client: task_id={}, type={}, status={:?}", event.task_id, event.task_type, event.status ); - yield Ok(Event::default().data(json)); + yield Ok::(Event::default().data(json)); } Err(e) => { warn!("Failed to serialize task progress event: {}", e); @@ -242,11 +249,14 @@ pub async fn task_progress_stream( debug!("Task progress stream ended for user {}", auth.user_id); }; - Ok(Sse::new(stream).keep_alive( + let sse = Sse::new(stream).keep_alive( KeepAlive::new() .interval(Duration::from_secs(15)) .text("keep-alive"), - )) + ); + // See entity_events_stream: disable proxy buffering so keep-alives are not + // held back and the connection isn't idle-timed-out into a reconnect loop. + Ok(([("X-Accel-Buffering", "no")], sse)) } #[cfg(test)] diff --git a/web/src/components/books/BookMetadataEditModal.tsx b/web/src/components/books/BookMetadataEditModal.tsx index 0e65bd41..416fe09d 100644 --- a/web/src/components/books/BookMetadataEditModal.tsx +++ b/web/src/components/books/BookMetadataEditModal.tsx @@ -49,6 +49,7 @@ import { LockableTextarea, } from "@/components/forms/lockable"; import { extractSourceFromUrl } from "@/components/series/ExternalLinks"; +import { useAllGenres, useAllTags } from "@/hooks/useReferenceData"; import type { BookTypeDto } from "@/types"; import type { BookAuthor, BookAuthorRole } from "@/types/book-metadata"; import { AUTHOR_ROLE_DISPLAY } from "@/types/book-metadata"; @@ -307,19 +308,9 @@ export function BookMetadataEditModal({ enabled: opened, }); - // Fetch all genres for suggestions - const { data: allGenres } = useQuery({ - queryKey: ["genres"], - queryFn: () => genresApi.getAll(), - enabled: opened, - }); - - // Fetch all tags for suggestions - const { data: allTags } = useQuery({ - queryKey: ["tags"], - queryFn: () => tagsApi.getAll(), - enabled: opened, - }); + // Fetch all genres + tags for suggestions (shared, long-cached reference data) + const { data: allGenres } = useAllGenres(opened); + const { data: allTags } = useAllTags(opened); const isLoading = isLoadingBook || isLoadingLocks; diff --git a/web/src/components/library/BookFilterPanel.tsx b/web/src/components/library/BookFilterPanel.tsx index 38467bc6..a004a69c 100644 --- a/web/src/components/library/BookFilterPanel.tsx +++ b/web/src/components/library/BookFilterPanel.tsx @@ -25,14 +25,12 @@ import { IconX, type TablerIcon, } from "@tabler/icons-react"; -import { useQuery } from "@tanstack/react-query"; import { type ReactNode, useMemo } from "react"; import { useSearchParams } from "react-router-dom"; import type { FilterPresetDto } from "@/api/filterPresets"; -import { genresApi } from "@/api/genres"; -import { tagsApi } from "@/api/tags"; import { useBookFilterState } from "@/hooks/useBookFilterState"; import { useDraftBookFilterState } from "@/hooks/useDraftBookFilterState"; +import { useAllGenres, useAllTags } from "@/hooks/useReferenceData"; import { useUserPreferencesStore } from "@/store/userPreferencesStore"; import { BOOK_FILTER_PARAM_KEYS, @@ -172,19 +170,10 @@ export function BookFilterPanel({ libraryId }: BookFilterPanelProps = {}) { close(); }; - // Fetch available genres (global, not library-specific) - const { data: genres = [], isLoading: genresLoading } = useQuery({ - queryKey: ["genres"], - queryFn: () => genresApi.getAll(), - staleTime: 60000, // Cache for 1 minute - }); - - // Fetch available tags (global, not library-specific) - const { data: tags = [], isLoading: tagsLoading } = useQuery({ - queryKey: ["tags"], - queryFn: () => tagsApi.getAll(), - staleTime: 60000, - }); + // Fetch available genres + tags (global reference data, shared + long-cached + // so the multi-page sweep doesn't re-run on every remount / reconnect). + const { data: genres = [], isLoading: genresLoading } = useAllGenres(); + const { data: tags = [], isLoading: tagsLoading } = useAllTags(); const isLoading = genresLoading || tagsLoading; diff --git a/web/src/components/library/BulkMetadataEditModal.tsx b/web/src/components/library/BulkMetadataEditModal.tsx index 14bff7f4..8a7a310f 100644 --- a/web/src/components/library/BulkMetadataEditModal.tsx +++ b/web/src/components/library/BulkMetadataEditModal.tsx @@ -26,12 +26,11 @@ import { IconTag, IconTrash, } from "@tabler/icons-react"; -import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; import { useCallback, useEffect, useMemo, useState } from "react"; import { bulkMetadataApi } from "@/api/bulkMetadata"; -import { genresApi } from "@/api/genres"; -import { tagsApi } from "@/api/tags"; import { CustomMetadataEditor } from "@/components/forms/CustomMetadataEditor"; +import { useAllGenres, useAllTags } from "@/hooks/useReferenceData"; import { AUTHOR_ROLE_DISPLAY, BOOK_TYPE_DISPLAY, @@ -248,19 +247,9 @@ export function BulkMetadataEditModal({ const [customMetadataLocked, setCustomMetadataLocked] = useState(false); const [customMetadataTouched, setCustomMetadataTouched] = useState(false); - // Fetch all genres for autocomplete - const { data: allGenres } = useQuery({ - queryKey: ["genres"], - queryFn: () => genresApi.getAll(), - enabled: opened, - }); - - // Fetch all tags for autocomplete - const { data: allTags } = useQuery({ - queryKey: ["tags"], - queryFn: () => tagsApi.getAll(), - enabled: opened, - }); + // Fetch all genres + tags for autocomplete (shared, long-cached reference data) + const { data: allGenres } = useAllGenres(opened); + const { data: allTags } = useAllTags(opened); const genreNames = useMemo( () => allGenres?.map((g) => g.name) ?? [], diff --git a/web/src/components/library/SeriesFilterPanel.tsx b/web/src/components/library/SeriesFilterPanel.tsx index 4a13c8b0..0d74008d 100644 --- a/web/src/components/library/SeriesFilterPanel.tsx +++ b/web/src/components/library/SeriesFilterPanel.tsx @@ -28,10 +28,9 @@ import { useQuery } from "@tanstack/react-query"; import { type ReactNode, useMemo } from "react"; import { useSearchParams } from "react-router-dom"; import type { FilterPresetDto } from "@/api/filterPresets"; -import { genresApi } from "@/api/genres"; import { sharingTagsApi } from "@/api/sharingTags"; -import { tagsApi } from "@/api/tags"; import { useDraftSeriesFilterState } from "@/hooks/useDraftSeriesFilterState"; +import { useAllGenres, useAllTags } from "@/hooks/useReferenceData"; import { useSeriesFilterState } from "@/hooks/useSeriesFilterState"; import { useAuthStore } from "@/store/authStore"; import { @@ -180,19 +179,10 @@ export function SeriesFilterPanel({ libraryId }: SeriesFilterPanelProps = {}) { close(); }; - // Fetch available genres (global, not library-specific) - const { data: genres = [], isLoading: genresLoading } = useQuery({ - queryKey: ["genres"], - queryFn: () => genresApi.getAll(), - staleTime: 60000, // Cache for 1 minute - }); - - // Fetch available tags (global, not library-specific) - const { data: tags = [], isLoading: tagsLoading } = useQuery({ - queryKey: ["tags"], - queryFn: () => tagsApi.getAll(), - staleTime: 60000, - }); + // Fetch available genres + tags (global reference data, shared + long-cached + // so the multi-page sweep doesn't re-run on every remount / reconnect). + const { data: genres = [], isLoading: genresLoading } = useAllGenres(); + const { data: tags = [], isLoading: tagsLoading } = useAllTags(); // Fetch sharing tags (admin only) const { data: sharingTags = [], isLoading: sharingTagsLoading } = useQuery({ diff --git a/web/src/components/series/SeriesMetadataEditModal.tsx b/web/src/components/series/SeriesMetadataEditModal.tsx index eba19270..64555fe5 100644 --- a/web/src/components/series/SeriesMetadataEditModal.tsx +++ b/web/src/components/series/SeriesMetadataEditModal.tsx @@ -55,6 +55,7 @@ import { } from "@/components/forms/lockable"; import { extractSourceFromUrl } from "@/components/series/ExternalLinks"; import { usePermissions } from "@/hooks/usePermissions"; +import { useAllGenres, useAllTags } from "@/hooks/useReferenceData"; import type { BookAuthor, BookAuthorRole } from "@/types/book-metadata"; import { AUTHOR_ROLE_DISPLAY } from "@/types/book-metadata"; @@ -220,19 +221,9 @@ export function SeriesMetadataEditModal({ enabled: opened, }); - // Fetch all genres for suggestions - const { data: allGenres } = useQuery({ - queryKey: ["genres"], - queryFn: () => genresApi.getAll(), - enabled: opened, - }); - - // Fetch all tags for suggestions - const { data: allTags } = useQuery({ - queryKey: ["tags"], - queryFn: () => tagsApi.getAll(), - enabled: opened, - }); + // Fetch all genres + tags for suggestions (shared, long-cached reference data) + const { data: allGenres } = useAllGenres(opened); + const { data: allTags } = useAllTags(opened); // Fetch existing covers for this series const { data: existingCovers, refetch: refetchCovers } = useQuery({ diff --git a/web/src/hooks/useReferenceData.ts b/web/src/hooks/useReferenceData.ts new file mode 100644 index 00000000..efb0dbbe --- /dev/null +++ b/web/src/hooks/useReferenceData.ts @@ -0,0 +1,39 @@ +import { useQuery } from "@tanstack/react-query"; +import { type Genre, genresApi } from "@/api/genres"; +import { type Tag, tagsApi } from "@/api/tags"; + +/** + * Reference-data lists (all tags, all genres) shared by filter panels and + * metadata editors. + * + * Both `getAll`s paginate the whole table (≈one request per 500 rows), so a + * large library makes each fetch several round-trips. These lists change only + * on an explicit tag/genre edit — which already invalidates the cache — so they + * are given a long `staleTime`. That stops the expensive multi-page sweep from + * re-running on every component remount or `refetchOnReconnect` (a flapping SSE + * connection on a remote deployment otherwise re-fetched the full list every + * time the socket bounced). Freshness after an edit still works: the metadata + * editors invalidate `["tags"]` / `["genres"]`, which overrides `staleTime`. + * + * Keyed `["tags"]` / `["genres"]` (unchanged) so all consumers and the existing + * invalidations dedupe to one shared query. + */ +const REFERENCE_DATA_STALE_TIME = 10 * 60 * 1000; // 10 minutes + +export function useAllTags(enabled = true) { + return useQuery({ + queryKey: ["tags"], + queryFn: () => tagsApi.getAll(), + staleTime: REFERENCE_DATA_STALE_TIME, + enabled, + }); +} + +export function useAllGenres(enabled = true) { + return useQuery({ + queryKey: ["genres"], + queryFn: () => genresApi.getAll(), + staleTime: REFERENCE_DATA_STALE_TIME, + enabled, + }); +} diff --git a/web/src/hooks/useTaskProgress.ts b/web/src/hooks/useTaskProgress.ts index 3422afda..b285bdb7 100644 --- a/web/src/hooks/useTaskProgress.ts +++ b/web/src/hooks/useTaskProgress.ts @@ -13,8 +13,13 @@ import { PERMISSIONS } from "@/types/permissions"; type ConnectionState = "connecting" | "connected" | "disconnected" | "failed"; /** Poll cadence for the processing-tasks / pending-counts backstop. SSE pushes - * live progress; this catches anything the event stream missed. */ -const POLL_INTERVAL_MS = 10_000; + * live progress, so polling only catches what the event stream missed — and is + * adaptive: fast while work is in flight, slow when idle. With nothing running + * there is nothing to reconcile, so a long idle interval keeps an open tab from + * hammering `/tasks` + `/tasks/stats` forever (SSE still announces new work the + * instant it starts, which flips polling back to the active cadence). */ +const POLL_ACTIVE_MS = 10_000; +const POLL_IDLE_MS = 60_000; /** How long a completed/failed task lingers in the active list so the UI can * show its terminal state before it disappears. */ const COMPLETED_TASK_LINGER_MS = 5_000; @@ -91,7 +96,11 @@ class TaskProgressManager { private pendingCounts: PendingTaskCounts = {}; private connectionState: ConnectionState = "disconnected"; private readonly listeners = new Set<() => void>(); - private pollTimer: ReturnType | null = null; + private pollTimer: ReturnType | null = null; + /** Current cadence of the armed poll timer, so re-evaluation can skip + * re-arming when the cadence hasn't changed (avoids timer thrash while a + * stream of SSE progress events flows). `null` means no timer is armed. */ + private pollMode: "active" | "idle" | null = null; private sseUnsubscribe: (() => void) | null = null; /** Per-task deletion timers for terminal tasks (the 5s linger). Tracked so * teardown can clear them and avoid firing after the last unsubscribe. */ @@ -118,7 +127,9 @@ class TaskProgressManager { getSnapshot = (): TaskProgressSnapshot => this.snapshot; - /** Rebuild the cached snapshot from current state and notify subscribers. */ + /** Rebuild the cached snapshot from current state and notify subscribers. + * Also re-evaluates the poll cadence: any state change (poll result, SSE + * event, terminal-task removal) can flip the active/idle decision. */ private commit() { this.snapshot = { activeTasks: sortTasks(Array.from(this.tasks.values())), @@ -128,28 +139,81 @@ class TaskProgressManager { for (const listener of this.listeners) { listener(); } + this.scheduleNextPoll(); } + /** Is there work worth polling for? Running/pending tasks need progress + * reconciliation; a non-zero pending count means work is about to start. + * When neither holds, there is nothing for the backstop poll to catch, so it + * drops to the idle cadence (SSE still announces new work instantly). */ + private hasActiveWork(): boolean { + for (const task of this.tasks.values()) { + if (task.status === "running" || task.status === "pending") { + return true; + } + } + for (const count of Object.values(this.pendingCounts)) { + if (count > 0) { + return true; + } + } + return false; + } + + /** (Re)arm the single backstop poll timer at the cadence matching current + * activity. No-op when the correct cadence is already armed, so a burst of + * SSE events doesn't keep resetting (and starving) the timer. */ + private scheduleNextPoll() { + if (this.listeners.size === 0) { + return; // stopped — don't arm a timer with no subscribers + } + const desired: "active" | "idle" = this.hasActiveWork() ? "active" : "idle"; + if (this.pollTimer !== null && this.pollMode === desired) { + return; + } + if (this.pollTimer !== null) { + clearTimeout(this.pollTimer); + } + this.pollMode = desired; + this.pollTimer = setTimeout( + () => { + this.pollTimer = null; + this.pollMode = null; + void this.poll(); + }, + desired === "active" ? POLL_ACTIVE_MS : POLL_IDLE_MS, + ); + } + + /** One backstop poll cycle. Each refresh commits, which re-arms the next + * poll; the trailing schedule guarantees the loop survives even if both + * fetches error (and thus skip their commit). */ + private poll = async () => { + await this.refreshPendingCounts(); + await this.refreshProcessingTasks("preserve"); + this.scheduleNextPoll(); + }; + private start() { - // Prime immediately, then poll as a backstop to the SSE stream. + // Prime immediately; the commit from each refresh arms the backstop poll at + // the right cadence. Arm an idle timer up front too, so the loop runs even + // if the initial fetches never resolve. void this.refreshPendingCounts(); void this.refreshProcessingTasks("replace"); - this.pollTimer = setInterval(() => { - void this.refreshPendingCounts(); - void this.refreshProcessingTasks("preserve"); - }, POLL_INTERVAL_MS); this.sseUnsubscribe = subscribeToTaskProgress( this.handleEvent, this.handleError, this.handleConnectionStateChange, ); + this.scheduleNextPoll(); } private stop() { if (this.pollTimer) { - clearInterval(this.pollTimer); + clearTimeout(this.pollTimer); this.pollTimer = null; } + this.pollMode = null; this.sseUnsubscribe?.(); this.sseUnsubscribe = null; for (const timer of this.lingerTimers.values()) { @@ -172,6 +236,8 @@ class TaskProgressManager { if (!is401(error)) { console.error("Failed to fetch pending task counts:", error); } + // Keep the backstop loop alive even when a poll fails (no commit ran). + this.scheduleNextPoll(); } };