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 <noreply@anthropic.com>

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));
     }
 

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);
+}

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<u64>,
+    /// Current generation; `None` once [`Self::retire`]d (nothing is read or written any more).
+    generation: RwLock<Option<u64>>,
     mem: Mutex<Lru<Hash128, Arc<Rgba8>>>,
     disk: Option<DiskCache>,
 }
@@ -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<Arc<Rgba8>> {
         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<Rgba8>) {
         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<Self>, generation: u64, key: Hash128, img: Arc<Rgba8>) {
         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<u64> {
         self.mem.lock().unwrap_or_else(|e| e.into_inner()).oldest_tick()

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);