Merge PR #1780

# Conflicts:
#	docs/whats-new.md

crates/engine/src/cmd/face_recognize.rs

@@ -275,7 +275,7 @@ mod imp {
         }
         let job = s.preview_job(id, EDGE, EDGE, false, &DevelopSettings::default())?;
         let preview = s.scan_preview_of(&photo);
-        Some(Prepared { tag, epoch: s.faces.epoch, id, job, preview, todo, detect, detector })
+        Some(Prepared { tag, epoch: s.faces.epoch, id, content: crate::media::content_key(&photo), job, preview, todo, detect, detector })
     }
 
     /// The faces the detector found in a photo that had none become its face regions: unnamed, marked as detected (so a
@@ -299,13 +299,16 @@ mod imp {
         }
     }
 
-    /// Take a finished photo into the index (ignored when it was made for another model or another library). Returns
-    /// (embedded, failed).
+    /// Take a finished photo into the index (ignored when it was made for another model or another library, or from
+    /// a file the photo no longer has: issue #1524). Returns (embedded, failed).
     fn ingest(s: &mut Session, d: Done) -> (usize, usize) {
         s.faces.in_flight.remove(&d.id);
         if d.epoch != s.faces.epoch || d.tag != s.faces.index.tag {
             return (0, 0);
         }
+        if s.catalog.photo(d.id).is_none_or(|p| crate::media::content_key(p) != d.content) {
+            return (0, 0);
+        }
         if d.detect_wanted {
             match &d.found {
                 Some(found) => {
@@ -830,7 +833,8 @@ mod imp {
         }
 
         fn finished(s: &Session, id: PhotoId, found: Option<Vec<Rect>>, results: Vec<(Rect, Option<Vec<f32>>, Rect)>) -> Done {
-            Done { tag: s.faces.index.tag.clone(), epoch: s.faces.epoch, id, results, detect_wanted: true, found }
+            let content = s.catalog.photo(id).map(|p| crate::media::content_key(p)).unwrap_or_default();
+            Done { tag: s.faces.index.tag.clone(), epoch: s.faces.epoch, id, content, results, detect_wanted: true, found }
         }
 
         #[test]
@@ -915,6 +919,34 @@ mod imp {
             let _ = std::fs::remove_dir_all(&d);
         }
 
+        /// A Reload that finds the file replaced (an outside edit) gives the photo other pixels: what a worker made from
+        /// the old file is dropped when it comes back, the faces already embedded from it are forgotten and the photo is
+        /// searched again (issue #1524).
+        #[test]
+        fn faces_made_from_a_replaced_file_are_dropped() {
+            let (mut s, d) = loaded("replaced");
+            let (a, b) = two_photos(&s);
+            let rect = region(0.2, None).rect;
+            s.faces.index.insert(a.0, &rect, unit(&[(0, 1.0)]));
+            s.faces.scanned.insert(a.0);
+            s.faces.index.insert(b.0, &rect, unit(&[(1, 1.0)]));
+            s.faces.scanned.insert(b.0);
+            let old = finished(&s, a, Some(vec![rect]), vec![(rect, Some(unit(&[(2, 1.0)])), rect)]);
+            let (width, height) = s.catalog.photo(a).map(|p| (p.width, p.height)).unwrap();
+            let replace = Op::SetContent { id: a, width, height, file_size: 1234, content_hash: Some("edited".into()), preview_only: None };
+            s.commit("Reload Changed Files", replace).unwrap();
+            assert!(s.faces.index.get(a.0, &rect).is_none() && !s.faces.scanned.contains(a.0), "the old file's faces are forgotten");
+            assert!(needs_search(&s, s.catalog.photo(a).unwrap()), "and the photo is searched again");
+            assert!(s.faces.index.get(b.0, &rect).is_some() && s.faces.scanned.contains(b.0), "other photos keep theirs");
+            assert_eq!(ingest(&mut s, old), (0, 0), "a result made from the old file is dropped");
+            assert!(s.catalog.photo(a).unwrap().meta.regions.is_empty() && !s.faces.scanned.contains(a.0));
+            // what is made from the new file is taken in
+            let new = finished(&s, a, Some(vec![rect]), vec![(rect, Some(unit(&[(3, 1.0)])), rect)]);
+            assert_eq!(ingest(&mut s, new), (1, 0));
+            assert!(s.faces.scanned.contains(a.0) && s.faces.index.get(a.0, &rect).is_some());
+            let _ = std::fs::remove_dir_all(&d);
+        }
+
         #[test]
         fn the_unnamed_faces_that_look_like_a_person_come_best_first() {
             let (mut s, d) = loaded("similar");

crates/engine/src/faces_index.rs

@@ -55,6 +55,8 @@ pub(crate) struct Index {
     skipped: std::collections::HashSet<Key>,
     /// The cache file on disk belongs to this model, so new records can be appended to it.
     file_matches: bool,
+    /// Records were dropped ([`Index::forget_photo`]): the cache file is written fresh at the next save.
+    rewrite: bool,
     pub path: Option<PathBuf>,
 }
 
@@ -87,6 +89,16 @@ impl Index {
         self.skipped.insert((photo, region_key(rect)));
     }
 
+    /// Drop every face of `photo` (its file was replaced, so they were made from other pixels): they are embedded
+    /// again, and the cache file is rewritten without them at the next save.
+    pub fn forget_photo(&mut self, photo: u64) {
+        let before = self.entries.len();
+        self.entries.retain(|k, _| k.0 != photo);
+        self.unsaved.retain(|(k, _)| k.0 != photo);
+        self.skipped.retain(|k| k.0 != photo);
+        self.rewrite |= self.entries.len() != before;
+    }
+
     #[cfg(test)]
     pub fn insert(&mut self, photo: u64, rect: &Rect, embedding: Vec<f32>) {
         self.insert_with_view(photo, rect, embedding, rect);
@@ -102,7 +114,7 @@ impl Index {
 
     /// Whether there are embeddings not yet written to the cache file.
     pub fn has_unsaved(&self) -> bool {
-        !self.unsaved.is_empty()
+        !self.unsaved.is_empty() || self.rewrite
     }
 
     /// Start over for `tag` (`dim` numbers per face), loading what the cache file at `path` holds for it. What the
@@ -116,6 +128,7 @@ impl Index {
         self.unsaved.clear();
         self.skipped.clear();
         self.file_matches = false;
+        self.rewrite = false;
         if let Some(p) = self.path.clone() {
             self.load(&p);
         }
@@ -175,17 +188,19 @@ impl Index {
         r
     }
 
-    /// Write what was added since the last save: appended when the file already belongs to this model, else the
-    /// whole index is written fresh (atomically). A failure is returned, the records stay pending.
+    /// Write what was added since the last save: appended when the file already belongs to this model, else (or when
+    /// records were dropped) the whole index is written fresh (atomically). A failure is returned, the records stay
+    /// pending.
     pub fn save(&mut self) -> std::io::Result<()> {
         let Some(path) = self.path.clone() else {
             self.unsaved.clear();
+            self.rewrite = false;
             return Ok(());
         };
-        if self.unsaved.is_empty() {
+        if self.unsaved.is_empty() && !self.rewrite {
             return Ok(());
         }
-        if self.file_matches {
+        if self.file_matches && !self.rewrite {
             let mut f = std::fs::OpenOptions::new().append(true).open(&path)?;
             let mut buf = Vec::new();
             for (k, e) in &self.unsaved {
@@ -201,6 +216,7 @@ impl Index {
             std::fs::write(&part, &buf)?;
             std::fs::rename(&part, &path)?;
             self.file_matches = true;
+            self.rewrite = false;
         }
         self.unsaved.clear();
         Ok(())
@@ -228,6 +244,8 @@ pub(crate) struct Scanned {
     unsaved: Vec<u64>,
     path: Option<PathBuf>,
     file_matches: bool,
+    /// A photo was taken out ([`Scanned::remove`]): the record is written fresh at the next save.
+    rewrite: bool,
 }
 
 impl Scanned {
@@ -249,8 +267,16 @@ impl Scanned {
         }
     }
 
+    /// Search `photo` again (its file was replaced).
+    pub fn remove(&mut self, photo: u64) {
+        if self.set.remove(&photo) {
+            self.unsaved.retain(|p| *p != photo);
+            self.rewrite = true;
+        }
+    }
+
     pub fn has_unsaved(&self) -> bool {
-        !self.unsaved.is_empty()
+        !self.unsaved.is_empty() || self.rewrite
     }
 
     /// Start over with the record at `path` (written first: what was not yet saved is not lost).
@@ -259,6 +285,7 @@ impl Scanned {
         self.set.clear();
         self.unsaved.clear();
         self.file_matches = false;
+        self.rewrite = false;
         self.path = path;
         if let Some(p) = self.path.clone() {
             self.load(&p);
@@ -292,12 +319,13 @@ impl Scanned {
     pub fn save(&mut self) -> std::io::Result<()> {
         let Some(path) = self.path.clone() else {
             self.unsaved.clear();
+            self.rewrite = false;
             return Ok(());
         };
-        if self.unsaved.is_empty() {
+        if self.unsaved.is_empty() && !self.rewrite {
             return Ok(());
         }
-        if self.file_matches {
+        if self.file_matches && !self.rewrite {
             let mut f = std::fs::OpenOptions::new().append(true).open(&path)?;
             let buf: Vec<u8> = self.unsaved.iter().flat_map(|id| id.to_le_bytes()).collect();
             f.write_all(&buf)?;
@@ -308,6 +336,7 @@ impl Scanned {
             std::fs::write(&part, &buf)?;
             std::fs::rename(&part, &path)?;
             self.file_matches = true;
+            self.rewrite = false;
         }
         self.unsaved.clear();
         Ok(())
@@ -406,6 +435,43 @@ mod tests {
         assert_eq!(b.len(), 0, "a non-finite vector is not loaded back");
         let _ = std::fs::remove_file(path);
     }
+
+    /// The faces and the search of a photo whose file was replaced are dropped from the files too, not only from memory:
+    /// they don't come back at the next launch (issue #1524).
+    #[test]
+    fn a_forgotten_photo_stays_forgotten_after_reloading() {
+        let dir = std::env::temp_dir().join(format!("lc-faces-forget-{}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&dir);
+        std::fs::create_dir_all(&dir).unwrap();
+        let (emb, scan) = (dir.join("face-embeddings.bin"), dir.join("face-scanned.bin"));
+        let mut a = Index::default();
+        a.reset("m@abc", 2, Some(emb.clone()));
+        a.insert(1, &rect(0.1), vec![1.0, 0.0]);
+        a.insert(2, &rect(0.1), vec![0.0, 1.0]);
+        a.save().unwrap();
+        a.forget_photo(1);
+        assert!(a.has_unsaved());
+        a.save().unwrap();
+        a.insert(3, &rect(0.1), vec![1.0, 1.0]);
+        a.save().unwrap();
+        let mut b = Index::default();
+        b.reset("m@abc", 2, Some(emb));
+        assert!(!b.contains(1, &rect(0.1)) && b.contains(2, &rect(0.1)) && b.contains(3, &rect(0.1)));
+        let mut s = Scanned::default();
+        s.reset(Some(scan.clone()));
+        s.insert(1);
+        s.insert(2);
+        s.save().unwrap();
+        s.remove(1);
+        assert!(s.has_unsaved());
+        s.save().unwrap();
+        s.insert(3);
+        s.save().unwrap();
+        let mut t = Scanned::default();
+        t.reset(Some(scan));
+        assert!(!t.contains(1) && t.contains(2) && t.contains(3));
+        let _ = std::fs::remove_dir_all(&dir);
+    }
 }
 
 /// What the recognition commands keep between calls: the loaded model and the embeddings made with it.
@@ -456,4 +522,12 @@ impl FacesState {
         self.saved_at = None;
         self.epoch = self.epoch.wrapping_add(1);
     }
+
+    /// Photo `id`'s file was replaced by other content: the faces made from the old pixels are dropped and the photo is
+    /// searched again (issue #1524). What a worker is still making from the old file is dropped when it comes back.
+    pub fn content_replaced(&mut self, id: lightcraft_catalog::PhotoId) {
+        self.index.forget_photo(id.0);
+        self.scanned.remove(id.0);
+        self.scan_failed.remove(&id.0);
+    }
 }

crates/engine/src/faces_worker.rs

@@ -134,6 +134,9 @@ pub(crate) struct Prepared {
     /// The session's face epoch when this was prepared: another library has been opened since if it differs.
     pub epoch: u64,
     pub id: PhotoId,
+    /// The photo's content key ([`crate::media::content_key`]) when this was prepared: its file was replaced (Reload
+    /// after an outside edit) since if it differs.
+    pub content: String,
     pub job: RenderJob,
     /// A raw's embedded preview, tried before the render job (much faster than decoding the raw).
     pub preview: Option<(String, PreviewLoader)>,
@@ -150,6 +153,7 @@ pub(crate) struct Done {
     pub tag: String,
     pub epoch: u64,
     pub id: PhotoId,
+    pub content: String,
     /// Each face's region, its embedding (`None` when it could not be made) and the box to show it by.
     pub results: Vec<(Rect, Option<Vec<f32>>, Rect)>,
     /// Faces were to be looked for in this photo.
@@ -177,7 +181,7 @@ fn face_rect(f: &Face) -> Option<Rect> {
 
 /// Do the work for one photo. Never panics: whatever goes wrong, its faces come back as `None`.
 pub(crate) fn process(p: Prepared, embedder: &Embedder) -> Done {
-    let Prepared { tag, epoch, id, job, preview, todo, detect, detector } = p;
+    let Prepared { tag, epoch, id, content, job, preview, todo, detect, detector } = p;
     let fallback = |todo: Vec<Rect>| todo.into_iter().map(|r| (r, None, r)).collect::<Vec<_>>();
     let rects = todo.clone();
     let run = catch_unwind(AssertUnwindSafe(|| {
@@ -238,7 +242,7 @@ pub(crate) fn process(p: Prepared, embedder: &Embedder) -> Done {
         (results, new_faces)
     }));
     let (results, found) = run.unwrap_or_else(|_| (fallback(rects), None));
-    Done { tag, epoch, id, results, detect_wanted: detect, found }
+    Done { tag, epoch, id, content, results, detect_wanted: detect, found }
 }
 
 type Job = (Arc<Embedder>, Prepared);

crates/engine/src/lib.rs

@@ -558,10 +558,16 @@ impl Session {
         // Retain only cache IDs while the catalog consumes the operation. Cloning an import
         // batch here also cloned every photo, although commit already keeps the journal copy.
         let mut invalidated = Vec::new();
+        // photos whose file was replaced: the faces made from the old pixels go too (issue #1524)
+        let mut replaced = Vec::new();
         let mut pending = vec![&op];
         while let Some(op) = pending.pop() {
             match op {
-                Op::SetFile { id, .. } | Op::Relink { id, .. } | Op::SetContent { id, .. } | Op::RemovePhoto { id } => invalidated.push(*id),
+                Op::SetContent { id, .. } => {
+                    invalidated.push(*id);
+                    replaced.push(*id);
+                }
+                Op::SetFile { id, .. } | Op::Relink { id, .. } | Op::RemovePhoto { id } => invalidated.push(*id),
                 Op::AddPhoto { photo } => invalidated.push(photo.id),
                 Op::Batch { ops } => pending.extend(ops),
                 _ => {}
@@ -571,6 +577,12 @@ impl Session {
         for id in invalidated {
             self.media.forget(id);
         }
+        #[cfg(not(target_arch = "wasm32"))]
+        for id in replaced {
+            self.faces.content_replaced(id);
+        }
+        #[cfg(target_arch = "wasm32")]
+        let _ = replaced;
         Ok(inv)
     }
 

docs/whats-new.md

@@ -34,6 +34,11 @@
   name (`catalog.snap.tmp`, `model.onnx.part`): the link is removed and a new file made in its place, so the file it
   pointed to is never changed (issue #1502).
 
+### Reliability
+- A photo whose file was replaced (Reload Changed Files after an outside edit) no longer keeps the faces recognition
+  made from the old pixels: they are made again from the new file, and a scan still busy with the old file when it
+  changed doesn't put its faces on the new one (issue #1524).
+
 ### Hard-edged Heal and Clone spots
 - A spot with Feather at 0 no longer draws black pixels exactly on its radius. This also fixes spots at 0% Opacity
   changing pixels along their edge. The hard edge now leaves pixels on the radius unchanged.