Merge PR #729

crates/engine/src/denoise.rs

@@ -1008,16 +1008,18 @@ impl Session {
     }
 
     /// Make the index of ready photos from the products on disk (a directory listing and a hash per photo: no file is
-    /// opened). Pictures that appeared or vanished drop the decoded sources they affect.
+    /// opened). Pictures that appeared or vanished drop the decoded sources they affect. The photo being made is its
+    /// job's to report (see [`Self::denoise_check_photo`]).
     pub(crate) fn denoise_reindex(&mut self) {
         self.denoise.dirty = false;
         self.denoise.reindexed = Some((self.catalog.revision, web_time::Instant::now()));
+        let running = self.denoise.running.as_ref().map(|r| r.photo);
         let mut now: HashMap<PhotoId, DenoiseSpec> = HashMap::new();
         if let (Some(dir), Some(active)) = (self.denoise_products_dir(), self.denoise.active.as_ref()) {
             let names: HashSet<String> =
                 products_in(&dir).into_iter().filter_map(|(p, _, _)| p.file_stem().and_then(|s| s.to_str()).map(str::to_string)).collect();
             if !names.is_empty() {
-                for p in self.catalog.photos().filter(|p| eligible(p)) {
+                for p in self.catalog.photos().filter(|p| eligible(p) && running != Some(p.id)) {
                     let key = product_key(&active.fingerprint, p);
                     if names.contains(&key) {
                         now.insert(p.id, DenoiseSpec { product: product_path(&dir, &key), key, make: None });
@@ -1046,7 +1048,14 @@ impl Session {
     }
 
     /// A photo whose file changed (its content key is not the one its index entry was made for) has no picture.
+    ///
+    /// The photo whose picture is being made is left to its job: the worker saves the picture before it reports, so the
+    /// file is on disk while the job still counts as running. Finding it here would make the photo Ready while the session
+    /// is busy with it (#719); [`Self::denoise_poll`] makes it Ready in the pump that takes the job's result instead.
     fn denoise_check_photo(&mut self, id: PhotoId) {
+        if self.denoise.running.as_ref().is_some_and(|r| r.photo == id) {
+            return;
+        }
         let Some(p) = self.catalog.photo(id).cloned() else { return };
         let Some((spec, _)) = self.denoise_spec(&p) else { return };
         match self.media.denoise.spec(id) {
@@ -1399,6 +1408,12 @@ impl Session {
         self.denoise.made
     }
 
+    /// The running job's product path and key (tests stand in for the worker's save).
+    #[cfg(test)]
+    pub(crate) fn denoise_running_product(&self) -> Option<(PathBuf, String)> {
+        self.denoise.running.as_ref().map(|r| (r.product.clone(), r.key.clone()))
+    }
+
     /// What an export of photo `id` with `settings` needs for its Denoise amount: its denoised picture, made now when the
     /// queue has not got to it. `Ok(None)` when denoise does not apply (amount 0, not a Bayer raw), `Err` when it should
     /// have and cannot (no model chosen, nowhere to keep the picture, the model failing).

crates/engine/src/tests_denoise.rs

@@ -720,6 +720,52 @@ fn a_photo_waits_for_its_turn_unless_pictures_are_not_made_on_their_own_or_have_
     let _ = std::fs::remove_dir_all(&x.dir);
 }
 
+#[test]
+fn a_picture_that_appears_while_its_job_runs_is_ready_only_when_the_job_reports() {
+    // #719: the worker saves the picture, then tears down its thread pool before it reports. A pump in that window
+    // used to find the file and mark the photo Ready while the session still counted the job as running: Ready and
+    // busy at once, and the activity row stayed up for a photo that said it was done.
+    use std::sync::{Mutex, mpsc};
+    use std::time::Duration;
+    let mut x = setup("ready-while-running", true);
+    let id = photo(&x.s);
+    set_amount(&mut x.s, 80.0);
+    // the job loads the model afresh and waits in the loader until it is released: a worker that is at work
+    *x.s.denoise.active.as_ref().unwrap().model.lock().unwrap() = None;
+    let (entered_tx, entered_rx) = mpsc::channel();
+    let (release_tx, release_rx) = mpsc::channel::<()>();
+    let release_rx = Mutex::new(release_rx);
+    let runs = x.runs.clone();
+    x.s.denoise.loader = Arc::new(move |_, _| {
+        entered_tx.send(()).map_err(|e| e.to_string())?;
+        release_rx.lock().unwrap().recv_timeout(Duration::from_secs(10)).map_err(|e| e.to_string())?;
+        Ok(Arc::new(Dim { runs: runs.clone() }) as Arc<dyn Model>)
+    });
+    x.s.execute("denoise.pump", &json!({"pace": "full"})).unwrap();
+    entered_rx.recv_timeout(Duration::from_secs(10)).unwrap();
+    assert!(matches!(x.s.denoise_photo_state(id), PhotoState::Running { .. }));
+    assert!(x.s.denoise_busy());
+    // the picture lands on disk before the worker says so (as it does between its save and its report)
+    let (product, key) = x.s.denoise_running_product().unwrap();
+    let mut rgb = lightcraft_raster::Rgb32f::new(192, 160);
+    rgb.data.fill([0.25, 0.25, 0.25]);
+    lightcraft_denoise::product::write(&product, &rgb, &key).unwrap();
+    assert!(lightcraft_denoise::product::is_current(&product, &key));
+    x.s.denoise.touch(); // the index is also rebuilt from the folder in this window
+    let r = x.s.execute("denoise.pump", &json!({"pace": "full"})).unwrap();
+    assert!(matches!(x.s.denoise_photo_state(id), PhotoState::Running { .. }), "the job reports its own picture");
+    assert_eq!(r["ready"], 0, "{r}");
+    assert!(x.s.denoise_busy());
+    // the worker reports: ready and idle in the same pump
+    release_tx.send(()).unwrap();
+    let r = pump_until_ready(&mut x.s, 1);
+    assert_eq!(x.s.denoise_photo_state(id), PhotoState::Ready);
+    assert_eq!(r["running"], Value::Null, "{r}");
+    assert!(!x.s.denoise_busy());
+    drop(x.s);
+    let _ = std::fs::remove_dir_all(x.dir);
+}
+
 #[test]
 fn the_denoise_switch_validates_arguments_and_accepts_explicit_targets_without_selection() {
     let mut x = setup("toggle", false);