From c25ae457c15deece08dcee2bc84b5e80994e08ce Mon Sep 17 00:00:00 2001 From: vicrodh <103399283+vicrodh@users.noreply.github.com> Date: Sun, 13 Sep 2026 03:27:56 -0600 Subject: [PATCH] fix(artwork): restore the shared image cache eviction lost with the Slint frontend ImageCacheService::evict had no caller since 69ed61207 retired the Slint app's spawn_evict, so ~/.cache/qbz/images grew without bound in 2.1.0 and 2.1.1. housekeeping() now trims it to the same 200 MB budget once per run, in 64-row batches so the GUI thread is never parked behind the cache mutex. evict is batched and covered by unit tests. cc --- crates/qbz-cache/src/image_cache.rs | 138 +++++++++++++++++++++++++--- crates/qbz-cache/src/lib.rs | 2 +- crates/qbz-qt/src/artwork_qt.rs | 70 ++++++++++---- crates/qbz-qt/src/main.rs | 9 +- 4 files changed, 183 insertions(+), 36 deletions(-) diff --git a/crates/qbz-cache/src/image_cache.rs b/crates/qbz-cache/src/image_cache.rs index 0fe6d1a52..634781dec 100644 --- a/crates/qbz-cache/src/image_cache.rs +++ b/crates/qbz-cache/src/image_cache.rs @@ -3,9 +3,9 @@ //! LRU disk cache for Qobuz album/artist images. //! - Stores images keyed by MD5 hash of URL //! - Tracks last-access time for LRU eviction -//! - Respects a configurable max size -//! - Framework-agnostic: shared by the Tauri app and the Slint shell so -//! both read and write the same `~/.cache/qbz/images` cache. +//! - Respects a configurable max size ([`ImageCacheService::evict`], batched) +//! - Framework-agnostic: the Qt shell owns the policy (`artwork_qt.rs`), this +//! crate owns the store (`~/.cache/qbz/images`). use md5::{Digest, Md5}; use rusqlite::{params, Connection}; @@ -18,6 +18,13 @@ pub struct ImageCacheStats { pub file_count: u64, } +/// Outcome of one [`ImageCacheService::evict`] batch. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct EvictBatch { + pub freed_bytes: u64, + pub evicted_entries: usize, +} + pub struct ImageCacheService { cache_dir: PathBuf, conn: Connection, @@ -29,7 +36,12 @@ impl ImageCacheService { .ok_or_else(|| "Could not find cache directory".to_string())? .join("qbz") .join("images"); + Self::open_at(cache_dir) + } + /// Open (or create) the cache at an explicit directory. `new` is this on + /// `~/.cache/qbz/images`; tests use a temp dir. + pub fn open_at(cache_dir: PathBuf) -> Result { std::fs::create_dir_all(&cache_dir) .map_err(|e| format!("Failed to create image cache dir: {}", e))?; @@ -54,6 +66,18 @@ impl ImageCacheService { Ok(Self { cache_dir, conn }) } + /// Test hook: rewrite an entry's `last_accessed` stamp. + #[cfg(test)] + fn backdate(&self, url: &str, last_accessed: i64) { + let hash = Self::url_hash(url); + self.conn + .execute( + "UPDATE cached_images SET last_accessed = ?1 WHERE hash = ?2", + params![last_accessed, hash], + ) + .expect("backdate"); + } + fn url_hash(url: &str) -> String { let mut hasher = Md5::new(); hasher.update(url.as_bytes()); @@ -115,8 +139,14 @@ impl ImageCacheService { Ok(path) } - /// Evict least-recently-accessed entries until total size is under max_bytes. - pub fn evict(&self, max_bytes: u64) -> Result { + /// Evict least-recently-accessed entries until the total size is under + /// `max_bytes`, deleting at most `max_entries` rows per call. + /// + /// Batched on purpose: the Qt shell holds a process-wide mutex around + /// this service and its GUI thread takes that mutex to resolve covers, + /// so a caller trims in small batches and releases the lock in between. + /// Loop until `evicted_entries == 0`. + pub fn evict(&self, max_bytes: u64, max_entries: usize) -> Result { let total: i64 = self .conn .query_row( @@ -126,21 +156,23 @@ impl ImageCacheService { ) .map_err(|e| format!("Failed to query cache size: {}", e))?; + let mut batch = EvictBatch { + freed_bytes: 0, + evicted_entries: 0, + }; if (total as u64) <= max_bytes { - return Ok(0); + return Ok(batch); } - let mut to_free = (total as u64) - max_bytes; - let mut freed: u64 = 0; - // Get LRU entries (oldest access first) + // LRU entries (oldest access first), bounded to this batch. let mut stmt = self .conn - .prepare("SELECT hash, file_size FROM cached_images ORDER BY last_accessed ASC") + .prepare("SELECT hash, file_size FROM cached_images ORDER BY last_accessed ASC LIMIT ?1") .map_err(|e| format!("Failed to prepare eviction query: {}", e))?; - + let limit = i64::try_from(max_entries).unwrap_or(i64::MAX); let entries: Vec<(String, i64)> = stmt - .query_map([], |row| Ok((row.get(0)?, row.get(1)?))) + .query_map(params![limit], |row| Ok((row.get(0)?, row.get(1)?))) .map_err(|e| format!("Failed to query LRU entries: {}", e))? .filter_map(|r| r.ok()) .collect(); @@ -157,11 +189,12 @@ impl ImageCacheService { .conn .execute("DELETE FROM cached_images WHERE hash = ?1", params![hash]); let size = file_size as u64; - freed += size; + batch.freed_bytes += size; + batch.evicted_entries += 1; to_free = to_free.saturating_sub(size); } - Ok(freed) + Ok(batch) } /// Get cache statistics. @@ -203,3 +236,80 @@ impl ImageCacheService { Ok(stats.total_bytes) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn fresh_dir(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("qbz-image-cache-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + dir + } + + #[test] + fn evict_drops_least_recently_accessed_first_and_unlinks_files() { + let dir = fresh_dir("lru"); + let cache = ImageCacheService::open_at(dir.clone()).unwrap(); + let oldest = cache.store("https://img/oldest", &[0u8; 100]).unwrap(); + let middle = cache.store("https://img/middle", &[0u8; 100]).unwrap(); + let newest = cache.store("https://img/newest", &[0u8; 100]).unwrap(); + cache.backdate("https://img/oldest", 1_000); + cache.backdate("https://img/middle", 2_000); + cache.backdate("https://img/newest", 3_000); + + let batch = cache.evict(150, usize::MAX).unwrap(); + + assert_eq!(batch, EvictBatch { freed_bytes: 200, evicted_entries: 2 }); + assert!(!oldest.exists()); + assert!(!middle.exists()); + assert!(newest.exists()); + let stats = cache.stats().unwrap(); + assert_eq!((stats.total_bytes, stats.file_count), (100, 1)); + assert!(cache.get("https://img/oldest").is_none()); + assert!(cache.get("https://img/newest").is_some()); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn evict_is_a_no_op_at_or_under_budget() { + let dir = fresh_dir("noop"); + let cache = ImageCacheService::open_at(dir.clone()).unwrap(); + let kept = cache.store("https://img/a", &[0u8; 100]).unwrap(); + assert_eq!(cache.evict(100, usize::MAX).unwrap(), EvictBatch { freed_bytes: 0, evicted_entries: 0 }); + assert_eq!(cache.evict(u64::MAX, usize::MAX).unwrap().evicted_entries, 0); + assert!(kept.exists()); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn evict_honours_the_batch_size_so_a_caller_can_release_its_lock_between_batches() { + let dir = fresh_dir("batch"); + let cache = ImageCacheService::open_at(dir.clone()).unwrap(); + for i in 0..3 { + cache.store(&format!("https://img/{i}"), &[0u8; 100]).unwrap(); + cache.backdate(&format!("https://img/{i}"), 1_000 + i); + } + assert_eq!(cache.evict(0, 1).unwrap(), EvictBatch { freed_bytes: 100, evicted_entries: 1 }); + assert_eq!(cache.evict(0, 1).unwrap(), EvictBatch { freed_bytes: 100, evicted_entries: 1 }); + assert_eq!(cache.evict(0, 1).unwrap(), EvictBatch { freed_bytes: 100, evicted_entries: 1 }); + assert_eq!(cache.evict(0, 1).unwrap(), EvictBatch { freed_bytes: 0, evicted_entries: 0 }); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn get_refreshes_last_accessed_so_a_touched_entry_survives_eviction() { + let dir = fresh_dir("touch"); + let cache = ImageCacheService::open_at(dir.clone()).unwrap(); + let touched = cache.store("https://img/touched", &[0u8; 100]).unwrap(); + let untouched = cache.store("https://img/untouched", &[0u8; 100]).unwrap(); + cache.backdate("https://img/touched", 1_000); + cache.backdate("https://img/untouched", 2_000); + // A read is an access: it moves `touched` to the newest slot. + assert!(cache.get("https://img/touched").is_some()); + assert_eq!(cache.evict(100, usize::MAX).unwrap().freed_bytes, 100); + assert!(touched.exists()); + assert!(!untouched.exists()); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/qbz-cache/src/lib.rs b/crates/qbz-cache/src/lib.rs index da326a674..8c97c324c 100644 --- a/crates/qbz-cache/src/lib.rs +++ b/crates/qbz-cache/src/lib.rs @@ -61,5 +61,5 @@ mod image_cache; mod playback_cache; pub use audio_cache::{AudioCache, CacheStats, CachedTrack, CacheAdmission}; -pub use image_cache::{ImageCacheService, ImageCacheStats}; +pub use image_cache::{EvictBatch, ImageCacheService, ImageCacheStats}; pub use playback_cache::{PlaybackCache, PlaybackCacheStats}; diff --git a/crates/qbz-qt/src/artwork_qt.rs b/crates/qbz-qt/src/artwork_qt.rs index 1a567d906..57c2796f6 100644 --- a/crates/qbz-qt/src/artwork_qt.rs +++ b/crates/qbz-qt/src/artwork_qt.rs @@ -46,14 +46,18 @@ //! the documented follow-up. No RGBA decode in Rust: QML `Image` decodes //! `file://` asynchronously and natively. //! -//! ## Cache eviction — scaled derivatives only +//! ## Cache eviction — both directories, once per run //! -//! [`housekeeping`] bounds `~/.cache/qbz/images/scaled` (this module's OWN -//! derivative directory) at [`MAX_SCALED_BYTES`], LRU by mtime, and does the -//! one-time `.jpg` orphan sweep the `.png` key change left behind. The PARENT -//! `~/.cache/qbz/images` is the SHARED cache and keeps its own policy -//! (`qbz_cache::ImageCacheService`, SQLite `last_accessed` + the Slint app's -//! 200 MB `evict`) — do NOT add a second one for it from here. +//! [`housekeeping`] runs on the blocking pool at boot and bounds BOTH caches: +//! the SHARED `~/.cache/qbz/images` (the full-size originals `store` writes; +//! `qbz_cache::ImageCacheService`, SQLite `last_accessed` LRU, budget +//! [`MAX_SHARED_BYTES`], trimmed in [`EVICT_BATCH_ENTRIES`]-row batches so the +//! GUI thread is never parked behind a long unlink loop) and this module's +//! OWN derivative directory `images/scaled` ([`MAX_SCALED_BYTES`], LRU by +//! mtime, plus the one-time `.jpg` orphan sweep). The shared trim was the +//! Slint app's `spawn_evict` (v2.0.2); it was retired with that frontend +//! (69ed61207) and 2.1.0/2.1.1 shipped with NO caller of `evict` at all — the +//! directory grew without bound. use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; @@ -1229,6 +1233,38 @@ pub fn scaled_path(path: &str, w: u32, h: u32) -> Option { /// order of magnitude under the 260 MB the parent image cache already holds. const MAX_SCALED_BYTES: u64 = 64 * 1024 * 1024; +/// Byte budget for the SHARED cache (`~/.cache/qbz/images`): the Slint app's +/// `MAX_CACHE_BYTES` ("matches the Tauri default"), restored unchanged. +const MAX_SHARED_BYTES: u64 = 200 * 1024 * 1024; + +/// Rows deleted per `evict` call; the cache mutex is released between calls. +const EVICT_BATCH_ENTRIES: usize = 64; + +/// Trim the shared cache to `max_bytes`, LRU by `last_accessed`, in batches. +/// Blocking (SQLite + unlinks) — call from `spawn_blocking`. Returns the +/// bytes freed, `None` when the cache could not be opened (already logged). +fn evict_shared(max_bytes: u64) -> Option { + let cache = cache()?; + let mut freed_total = 0u64; + loop { + let batch = { + let guard = cache.lock().ok()?; + match guard.evict(max_bytes, EVICT_BATCH_ENTRIES) { + Ok(batch) => batch, + Err(e) => { + log::warn!("[qbz-qt] image cache eviction failed: {e}"); + return Some(freed_total); + } + } + }; + freed_total += batch.freed_bytes; + if batch.evicted_entries == 0 { + return Some(freed_total); + } + std::thread::yield_now(); + } +} + /// Keep `scaled/` bounded: drop the oldest derivatives past /// [`MAX_SCALED_BYTES`]. Mtime order, oldest first — the same shape as /// `icon_tint_qt::prune`, with one deliberate difference: `prune` caps on a @@ -1302,16 +1338,18 @@ fn sweep_orphan_scaled(dir: &Path) -> (usize, u64) { (n, freed) } -/// Boot-time housekeeping for the derivative cache: the one-time `.jpg` -/// orphan sweep, then the byte cap. Blocking (`read_dir` + unlinks) — call -/// from `spawn_blocking`. -/// -/// SCOPE: `images/scaled` ONLY. The parent `~/.cache/qbz/images` (260 MB) is -/// the SHARED cache owned by `qbz_cache::ImageCacheService` (SQLite -/// `last_accessed` + the Slint app's 200 MB `evict`); two processes evicting -/// the same directory on different policies is exactly what the memo at the -/// top of this file had to be written about. +/// Boot-time housekeeping for both image caches — see the module doc +/// "Cache eviction". Blocking; call from `spawn_blocking`. pub fn housekeeping() { + // 1. The SHARED cache (full-size originals): LRU trim to the budget. + if let Some(freed) = evict_shared(MAX_SHARED_BYTES) { + if freed > 0 { + log::info!( + "[qbz-qt][perf] image cache evicted {freed} B (budget {MAX_SHARED_BYTES} B)" + ); + } + } + // 2. This module's OWN derivative directory: orphan sweep, then byte cap. let Some(dir) = dirs::cache_dir().map(|d| d.join("qbz").join("images").join("scaled")) else { return; }; diff --git a/crates/qbz-qt/src/main.rs b/crates/qbz-qt/src/main.rs index 0f8be119e..d631b9e97 100644 --- a/crates/qbz-qt/src/main.rs +++ b/crates/qbz-qt/src/main.rs @@ -721,11 +721,10 @@ pub(crate) fn on_boot() { }); offline_fwd::start_ui_forwarder(); - // Derivative-cache housekeeping, off the Qt thread. Once per run: the - // `.jpg` orphan sweep is idempotent (FIX 1 moved the scaled derivatives to - // `.png`, so every `.jpg` left in `images/scaled/` is dead weight from a - // pre-fix build) and the byte cap is cheap — one `read_dir`, then unlink - // the oldest until the directory is back under the ceiling. + // Image-cache housekeeping, off the Qt thread, once per run: the shared + // `~/.cache/qbz/images` LRU trim (200 MB, batched under the cache mutex) + // and the `images/scaled` orphan sweep + byte cap. See artwork_qt.rs + // "Cache eviction". spawn(async { let _ = tokio::task::spawn_blocking(artwork_qt::housekeeping).await; });