From 62325bd45ed5cb0a16835e72be14e55a5248cbcc Mon Sep 17 00:00:00 2001 From: radialmonster Date: Wed, 7 Oct 2026 20:14:40 -0400 Subject: [PATCH] Thumbnails: retire the replaced preview cache so its old jobs can't write Reopening a library (or opening another one, or set_disk_cache_bytes falling back to attaching) replaced MediaCache::rendered with a new PreviewCache, but render jobs still in flight kept the old object. Each PreviewCache checked only its own generation, so after a clear on the new cache the old one's generation was still current: a finishing thumbnail job wrote its pixels through it into the directory the clear had just emptied. The UI rejected the result, but the JPEG stayed. attach_disk_cache now retires the cache it replaces before swapping it: PreviewCache::retire takes the generation write lock (so it waits for a write in progress, like clear) and marks the object retired; every get/put/put_deferred through it is then refused and its clear() is a no-op, as its directory may now belong to the replacement. Files on disk are left alone, so a plain reopen still finds its thumbnails, and resizing the budget in place keeps the same cache object as before. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/media.rs | 5 ++++- crates/engine/src/tests_prefs.rs | 29 +++++++++++++++++++++++++ crates/preview/src/lib.rs | 36 +++++++++++++++++++++++--------- crates/preview/src/tests.rs | 24 +++++++++++++++++++++ 4 files changed, 83 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index e45cc92e..76a9bf1a 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -244,8 +244,11 @@ fn source_bytes(img: &Rgb32f) -> usize { } impl MediaCache { - /// Keep rendered thumbnails on disk in `dir` as well. + /// Keep rendered thumbnails on disk in `dir` as well. The cache this replaces is retired + /// (its files stay): render jobs still holding it can't write through it any more, even + /// into a directory a later clear emptied (`dir` may be the same one). pub fn attach_disk_cache(&mut self, dir: &std::path::Path, disk_bytes: u64) { + self.rendered.retire(); self.rendered = Arc::new(PreviewCache::with_disk(rendered_budget(self.budget), dir, disk_bytes)); } diff --git a/crates/engine/src/tests_prefs.rs b/crates/engine/src/tests_prefs.rs index 9b343bfd..3534064f 100644 --- a/crates/engine/src/tests_prefs.rs +++ b/crates/engine/src/tests_prefs.rs @@ -288,3 +288,32 @@ fn resizing_the_disk_cache_keeps_the_cache_and_its_thumbnails() { drop(s); let _ = std::fs::remove_dir_all(&lib); } + +#[test] +fn reopening_a_library_retires_the_old_cache_so_its_jobs_cannot_write_after_a_clear() { + let lib = temp_dir("reopen-writer"); + let mut s = Session::new().with_fs(); + s.open_library(&lib, true).unwrap(); + let id = s.catalog.photos().next().unwrap().id; + // a thumbnail job still holding the first open's cache object + let job = s.thumb_job(id, 128).unwrap(); + let (old, key) = job.cache.clone().unwrap(); + // a thumbnail written before the reopen is still valid afterwards + let kept = lightcraft_preview::hash_bytes(b"kept thumbnail"); + let img = lightcraft_raster::Rgba8 { width: 4, height: 4, data: vec![[10, 20, 30, 255]; 16] }; + old.put(kept, Arc::new(img.clone())); + s.open_library(&lib, false).unwrap(); + assert!(!Arc::ptr_eq(&old, &s.media.rendered)); + assert!(s.media.rendered.get(kept).is_some(), "a plain reopen keeps the disk cache"); + s.execute("library.clearPreviews", &json!({})).unwrap(); + // the old job finishes: its render must not land in the cleared directory + let r = job.run(); + assert!(r.rendered.is_ok()); + old.put(kept, Arc::new(img)); + let fresh = lightcraft_preview::PreviewCache::with_disk(1 << 20, &lib.join("thumbs"), 1 << 30); + assert!(fresh.get(key).is_none(), "stale thumbnail written through the replaced cache"); + assert!(fresh.get(kept).is_none(), "stale put through the replaced cache"); + assert!(s.media.rendered.get(key).is_none()); + drop(s); + let _ = std::fs::remove_dir_all(&lib); +} diff --git a/crates/preview/src/lib.rs b/crates/preview/src/lib.rs index 50dc68e5..a571ecda 100644 --- a/crates/preview/src/lib.rs +++ b/crates/preview/src/lib.rs @@ -28,7 +28,8 @@ pub use pool::JobPool; /// Rendered-thumbnail cache: memory LRU over an optional disk cache. pub struct PreviewCache { - generation: RwLock, + /// Current generation; `None` once [`Self::retire`]d (nothing is read or written any more). + generation: RwLock>, mem: Mutex>>, disk: Option, } @@ -36,12 +37,12 @@ pub struct PreviewCache { impl PreviewCache { /// Memory-only cache of at most `mem_bytes`. pub fn memory(mem_bytes: usize) -> PreviewCache { - PreviewCache { generation: RwLock::new(0), mem: Mutex::new(Lru::new(mem_bytes)), disk: None } + PreviewCache { generation: RwLock::new(Some(0)), mem: Mutex::new(Lru::new(mem_bytes)), disk: None } } /// Memory LRU + disk cache in `dir` (at most `disk_bytes`). pub fn with_disk(mem_bytes: usize, dir: &Path, disk_bytes: u64) -> PreviewCache { - PreviewCache { generation: RwLock::new(0), mem: Mutex::new(Lru::new(mem_bytes)), disk: Some(DiskCache::new(dir, disk_bytes)) } + PreviewCache { generation: RwLock::new(Some(0)), mem: Mutex::new(Lru::new(mem_bytes)), disk: Some(DiskCache::new(dir, disk_bytes)) } } pub fn disk(&self) -> Option<&DiskCache> { @@ -53,13 +54,14 @@ impl PreviewCache { } /// Changes when explicitly cleared; jobs captured before that clear cannot repopulate it. + /// A retired cache reports `u64::MAX`, which no job's generation matches. pub fn generation(&self) -> u64 { - *self.generation.read().unwrap_or_else(std::sync::PoisonError::into_inner) + self.generation.read().unwrap_or_else(std::sync::PoisonError::into_inner).unwrap_or(u64::MAX) } pub fn get_at(&self, generation: u64, key: Hash128) -> Option> { let current = self.generation.read().unwrap_or_else(std::sync::PoisonError::into_inner); - if *current != generation { + if *current != Some(generation) { return None; } if let Some(v) = self.mem.lock().unwrap_or_else(|e| e.into_inner()).get(&key) { @@ -76,7 +78,7 @@ impl PreviewCache { pub fn put_at(&self, generation: u64, key: Hash128, img: Arc) { let current = self.generation.read().unwrap_or_else(std::sync::PoisonError::into_inner); - if *current != generation { + if *current != Some(generation) { return; } if let Some(d) = &self.disk { @@ -94,7 +96,7 @@ impl PreviewCache { pub fn put_deferred_at(self: &Arc, generation: u64, key: Hash128, img: Arc) { let current = self.generation.read().unwrap_or_else(std::sync::PoisonError::into_inner); - if *current != generation { + if *current != Some(generation) { return; } let c = cost(&img); @@ -106,7 +108,7 @@ impl PreviewCache { let me = self.clone(); let write = move || { let current = me.generation.read().unwrap_or_else(std::sync::PoisonError::into_inner); - if *current != generation { + if *current != Some(generation) { return; } if let Some(d) = &me.disk { @@ -121,16 +123,30 @@ impl PreviewCache { write(); } - /// Drop everything (memory and disk). + /// Drop everything (memory and disk). Does nothing once [`Self::retire`]d. pub fn clear(&self) { let mut generation = self.generation.write().unwrap_or_else(std::sync::PoisonError::into_inner); - *generation = generation.wrapping_add(1); + // a retired cache no longer owns its directory (its replacement may) + let Some(g) = generation.as_mut() else { return }; + // (never reaches u64::MAX, which stands for "retired") + *g = g.wrapping_add(1) % u64::MAX; self.mem.lock().unwrap_or_else(|e| e.into_inner()).clear(); if let Some(d) = &self.disk { d.clear(); } } + /// Stop this cache object for good, leaving its disk files alone: called when another cache + /// object replaces it (a library reopened or another one opened). Jobs still holding it then + /// can neither read nor write through it, so a later clear of the replacement's directory + /// (possibly the same one) can't be repopulated by them. Waits for writes in progress, like + /// [`Self::clear`]. + pub fn retire(&self) { + let mut generation = self.generation.write().unwrap_or_else(std::sync::PoisonError::into_inner); + *generation = None; + self.mem.lock().unwrap_or_else(|e| e.into_inner()).clear(); + } + /// Use tick ([`next_tick`]) of the least recently used image in memory. pub fn oldest_tick(&self) -> Option { self.mem.lock().unwrap_or_else(|e| e.into_inner()).oldest_tick() diff --git a/crates/preview/src/tests.rs b/crates/preview/src/tests.rs index ca4d39f8..3d634003 100644 --- a/crates/preview/src/tests.rs +++ b/crates/preview/src/tests.rs @@ -26,6 +26,30 @@ fn cleared_generation_cannot_read_or_repopulate_memory_or_disk() { cache.clear(); } +#[test] +fn retired_cache_keeps_its_files_but_refuses_every_read_and_write() { + let dir = temp_dir("retire"); + let old = Arc::new(PreviewCache::with_disk(1 << 20, &dir, 1 << 20)); + let (kept, late) = (hash_bytes(b"kept"), hash_bytes(b"late")); + let img = Arc::new(gradient(8, 8, 7)); + let generation = old.generation(); + old.put_at(generation, kept, img.clone()); + old.retire(); + assert_ne!(old.generation(), generation); + assert!(old.get_at(generation, kept).is_none()); + assert!(old.get(kept).is_none()); + old.put_at(generation, late, img.clone()); + old.put_deferred_at(generation, late, img.clone()); + old.put(late, img.clone()); + old.put_deferred(late, img.clone()); + old.clear(); + // a replacement for the same directory still finds the valid file, and only that + let new = PreviewCache::with_disk(1 << 20, &dir, 1 << 20); + assert_eq!(new.get(kept).map(|i| (i.width, i.height)), Some((8, 8)), "the disk file stays (JPEG: lossy)"); + assert!(new.get(late).is_none()); + new.clear(); +} + fn temp_dir(tag: &str) -> std::path::PathBuf { let d = std::env::temp_dir().join(format!("lc-preview-{tag}-{}", std::process::id())); let _ = std::fs::remove_dir_all(&d);