From e4c36749ef044a662bd4764f95d3a6889ac58189 Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Sun, 11 Oct 2026 16:26:04 -0500 Subject: [PATCH] feat(engine): #1046 typed error kinds for cancelled work and missing models EngineError gains Cancelled and ModelMissing { model: AiModel } and a kind() -> ErrorKind, so frontends decide on what an error is rather than on its words. The face detector, SAM 3 model checks and the Lightroom job produce them at the source; the error text is unchanged, so control channel and MCP clients see the same `error` strings. Background results that cross the wire as JSON get an additive flag instead: SAM 3 download status `cancelled`, Apple Photos import outcome `cancelled: true` next to `skipped`. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/apple_photos.rs | 20 +++++--- crates/engine/src/cmd/face_detect.rs | 44 +++++++++++++---- crates/engine/src/cmd/masks.rs | 35 +++++++------ crates/engine/src/errors.rs | 68 ++++++++++++++++++++++++++ crates/engine/src/lib.rs | 9 ++++ crates/engine/src/lightroom_job.rs | 35 ++++++++++--- crates/engine/src/segment/download.rs | 13 ++++- crates/engine/src/segment/mod.rs | 43 +++++++++++----- crates/engine/src/tests_face_models.rs | 20 ++++++-- crates/engine/src/tests_segment.rs | 29 ++++++++++- 10 files changed, 263 insertions(+), 53 deletions(-) create mode 100644 crates/engine/src/errors.rs diff --git a/crates/engine/src/apple_photos.rs b/crates/engine/src/apple_photos.rs index fd1c6f24b..01394cd5f 100644 --- a/crates/engine/src/apple_photos.rs +++ b/crates/engine/src/apple_photos.rs @@ -308,9 +308,12 @@ pub enum Outcome { Imported(Imported), /// Photos (or osascript) failed: why, for the user. Failed(String), - /// Photos wasn't asked: the export that reserved the import was cancelled, wrote no files or - /// stopped early. - Skipped(String), + /// Photos wasn't asked: the export that reserved the import was cancelled (`cancelled`), wrote + /// no files or stopped early. `why` is for people; code decides on `cancelled`. + Skipped { + why: String, + cancelled: bool, + }, } /// One import: reserved by an export still writing its files, running, or finished. @@ -402,9 +405,12 @@ impl ImportJob { v["running"] = json!(false); v["error"] = json!(e); } - Some(Outcome::Skipped(why)) => { + Some(Outcome::Skipped { why, cancelled }) => { v["running"] = json!(false); v["skipped"] = json!(why); + if cancelled { + v["cancelled"] = json!(true); + } } } v @@ -660,7 +666,7 @@ impl Reservation { None }; if let Some(why) = skipped { - job.finish(Outcome::Skipped(why.into())); + job.finish(Outcome::Skipped { why: why.into(), cancelled }); return Err(job.json()); } if let Err(e) = check(&paths, &self.album) { @@ -700,7 +706,7 @@ impl Reservation { impl Drop for Reservation { fn drop(&mut self) { if let Some(job) = self.job.take() { - job.finish(Outcome::Skipped("the export ended before writing its files".into())); + job.finish(Outcome::Skipped { why: "the export ended before writing its files".into(), cancelled: false }); } } } @@ -1009,8 +1015,10 @@ mod tests { // nothing written, or cancelled: Photos isn't asked, and the slot is free again let out = imports.reserve(runner.clone(), "").unwrap().run(&[json!({"skipped": "x"})], false); assert!(out["skipped"].as_str().unwrap().contains("wrote no files"), "{out}"); + assert_eq!(out.get("cancelled"), None, "{out}"); let out = imports.reserve(runner.clone(), "").unwrap().run(&exported, true); assert!(out["skipped"].as_str().unwrap().contains("cancelled"), "{out}"); + assert_eq!(out["cancelled"], true, "the app decides on the flag, not on the words: {out}"); assert_eq!(calls.lock().unwrap().len(), 1); assert!(imports.running().is_none()); let _ = std::fs::remove_dir_all(dir); diff --git a/crates/engine/src/cmd/face_detect.rs b/crates/engine/src/cmd/face_detect.rs index ffdedbef1..4676d1c65 100644 --- a/crates/engine/src/cmd/face_detect.rs +++ b/crates/engine/src/cmd/face_detect.rs @@ -18,7 +18,7 @@ use lightcraft_meta::{Region, RegionKind}; use serde_json::{Value, json}; use super::{CommandSpec, bad, cmd, has_selection}; -use crate::{Result, Session}; +use crate::{AiModel, EngineError, Result, Session}; const C: &str = "faces.detect"; /// Regions made by this command say so in their description; only those are replaced on a new run. @@ -34,15 +34,39 @@ const EXISTING_IOU: f64 = 0.3; /// What to tell someone who has not downloaded the detector yet. const NOT_INSTALLED: &str = "finding faces needs the YuNet face detector (232 KB, MIT): download it in Settings > Faces"; +/// Why [`detector`] has no detector to give. +#[derive(Debug)] +pub(crate) enum NoDetector { + /// Not downloaded yet: a frontend offers the download ([`crate::ErrorKind::ModelMissing`]). + NotInstalled, + /// Nowhere to keep it, or there but unusable: says what to do. + Unusable(String), +} + +impl NoDetector { + /// Command `cmd`'s error for it. The message reads as before kinds existed (an invalid-parameters + /// error), so control-channel and MCP clients see the same `error` text. + pub(crate) fn into_error(self, cmd: &str) -> EngineError { + match self { + Self::NotInstalled => EngineError::ModelMissing { model: AiModel::FaceDetector, msg: bad(cmd, NOT_INSTALLED).to_string() }, + Self::Unusable(why) => bad(cmd, why), + } + } +} + /// The installed YuNet, loaded once and kept (a new one is loaded when the model folder changes). /// (also used by the background scan, which aligns faces by the detector's landmarks and finds faces in photos that have none) -pub(crate) fn detector(s: &Session) -> std::result::Result, String> { +pub(crate) fn detector(s: &Session) -> std::result::Result, NoDetector> { static LOADED: Mutex)>> = Mutex::new(None); - let home = s.face_models_dir.as_deref().ok_or("this build has nowhere to keep face models (the desktop app does)")?.join(YUNET_ID); + let home = s + .face_models_dir + .as_deref() + .ok_or_else(|| NoDetector::Unusable("this build has nowhere to keep face models (the desktop app does)".into()))? + .join(YUNET_ID); let path = home.join("model.onnx"); // installing writes the manifest last, so a folder without one is not (yet) a model if !path.is_file() || !home.join("face-model.json").is_file() { - return Err(NOT_INSTALLED.into()); + return Err(NoDetector::NotInstalled); } let mut loaded = LOADED.lock().unwrap_or_else(PoisonError::into_inner); if let Some((p, d)) = loaded.as_ref() @@ -50,12 +74,14 @@ pub(crate) fn detector(s: &Session) -> std::result::Result, String { return Ok(d.clone()); } - let len = std::fs::metadata(&path).map_err(|e| format!("could not read the detector: {e}"))?.len(); + let len = std::fs::metadata(&path).map_err(|e| NoDetector::Unusable(format!("could not read the detector: {e}")))?.len(); if len > MAX_MODEL_BYTES { - return Err("the detector's file is far bigger than YuNet: remove it in Settings > Faces and download it again".into()); + return Err(NoDetector::Unusable("the detector's file is far bigger than YuNet: remove it in Settings > Faces and download it again".into())); } - let bytes = std::fs::read(&path).map_err(|e| format!("could not read the detector: {e}"))?; - let d = Arc::new(Detector::new(&bytes).map_err(|e| format!("{e}: remove the detector in Settings > Faces and download it again"))?); + let bytes = std::fs::read(&path).map_err(|e| NoDetector::Unusable(format!("could not read the detector: {e}")))?; + let d = Arc::new( + Detector::new(&bytes).map_err(|e| NoDetector::Unusable(format!("{e}: remove the detector in Settings > Faces and download it again")))?, + ); *loaded = Some((path, d.clone())); Ok(d) } @@ -103,7 +129,7 @@ fn detect(s: &mut Session, p: &Value) -> Result { nms_iou: p.get("nmsIou").and_then(Value::as_f64).map_or(defaults.nms_iou, |v| v as f32), max_faces: p.get("maxFaces").and_then(Value::as_u64).map_or(defaults.max_faces, |v| usize::try_from(v).unwrap_or(defaults.max_faces)), }; - let detector = detector(s).map_err(|e| bad(C, e))?; + let detector = detector(s).map_err(|e| e.into_error(C))?; let started = Instant::now(); let (mut results, mut ops) = (Vec::new(), Vec::new()); for id in s.targets(p) { diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index ae8b862b6..aa23e93cf 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -84,29 +84,29 @@ fn resolve_ai(s: &mut Session, shape: &mut MaskShape, c: &str) -> Result match shape { MaskShape::Object { seg: Some(_), .. } | MaskShape::Prompt { seg: Some(_), .. } => {} MaskShape::Object { hint, exclude, .. } if !hint.is_empty() && background => { - s.segmenter.model_dir().map_err(ai_err)?; + s.segmenter.require_model()?; return Ok(Later::Clicks(std::mem::take(hint), std::mem::take(exclude))); } MaskShape::Object { hint, exclude, seg, .. } if !hint.is_empty() => { - *seg = Some(s.segment_clicks(id, &clicks_of(hint, exclude)).map_err(ai_err)?); + *seg = Some(segment(s, |s| s.segment_clicks(id, &clicks_of(hint, exclude)))?); } MaskShape::Object { .. } => { // the clicks come later: refuse an Object mask that could never be computed (no // model) rather than leave an empty one; in the app, analyze the photo meanwhile - s.segmenter.model_dir().map_err(ai_err)?; + s.segmenter.require_model()?; if background { - s.segment_prepare(id).map_err(ai_err)?; + segment(s, |s| s.segment_prepare(id))?; } } MaskShape::Prompt { text, .. } if background => { - s.segmenter.model_dir().map_err(ai_err)?; + s.segmenter.require_model()?; if text.trim().is_empty() { return Err(ai_err("describe what to select")); } return Ok(Later::Text(text.clone())); } MaskShape::Prompt { text, seg, .. } => { - let found = s.segment_text(id, text).map_err(ai_err)?; + let found = segment(s, |s| s.segment_text(id, text))?; *seg = Some(found.ok_or_else(|| ai_err(format!("nothing matching “{text}” was found in this photo")))?); } _ => {} @@ -118,7 +118,7 @@ fn resolve_ai(s: &mut Session, shape: &mut MaskShape, c: &str) -> Result fn queue_later(s: &mut Session, later: Later, mask: u32, comp: usize, mut out: Value) -> Result { if let Later::Clicks(hint, exclude) = later { let id = s.active().ok_or_else(|| ai_err("no active photo"))?; - s.segment_clicks_later(id, mask, comp, hint, exclude).map_err(ai_err)?; + segment(s, |s| s.segment_clicks_later(id, mask, comp, hint, exclude))?; out["pending"] = json!(true); } Ok(out) @@ -129,6 +129,13 @@ fn ai_err(msg: impl Into) -> crate::EngineError { crate::EngineError::Other(msg.into()) } +/// A segmentation call: the model is checked first, so a missing one is [`crate::EngineError::ModelMissing`] +/// (the segmenter's own errors are text). +fn segment(s: &mut Session, f: impl FnOnce(&mut Session) -> std::result::Result) -> Result { + s.segmenter.require_model()?; + f(s).map_err(ai_err) +} + pub(crate) fn clicks_of(include: &[Point], exclude: &[Point]) -> Vec { include.iter().map(|p| Click { at: *p, include: true }).chain(exclude.iter().map(|p| Click { at: *p, include: false })).collect() } @@ -167,7 +174,7 @@ pub fn specs() -> Vec { let name = str_param(p, "name").map(str::to_string); if let Later::Text(text) = &later { let id = s.active().ok_or_else(|| bad("mask.add", "no active photo"))?; - s.segment_text_later(id, None, "add", name, text).map_err(ai_err)?; + segment(s, |s| s.segment_text_later(id, None, "add", name, text))?; return Ok(json!({"pending": true})); } let next = s.active().and_then(|id| s.develop_of(id)).map(|d| d.next_mask_id()).unwrap_or(1); @@ -205,7 +212,7 @@ pub fn specs() -> Vec { let later = resolve_ai(s, &mut shape, "mask.addComponent")?; if let Later::Text(text) = &later { let op = p.get("op").and_then(Value::as_str).unwrap_or("add"); - s.segment_text_later(id, Some(mid), op, None, text).map_err(ai_err)?; + segment(s, |s| s.segment_text_later(id, Some(mid), op, None, text))?; return Ok(json!({"pending": true})); } let mut comp = 0; @@ -365,10 +372,10 @@ pub fn specs() -> Vec { } if s.segmenter.background && !hint.is_empty() { let out = json!({"include": hint.len(), "exclude": ex.len(), "pending": true}); - s.segment_clicks_later(id, mid, k, hint, ex).map_err(ai_err)?; + segment(s, |s| s.segment_clicks_later(id, mid, k, hint, ex))?; return Ok(out); } - let seg = if hint.is_empty() { None } else { Some(s.segment_clicks(id, &clicks_of(&hint, &ex)).map_err(ai_err)?) }; + let seg = if hint.is_empty() { None } else { Some(segment(s, |s| s.segment_clicks(id, &clicks_of(&hint, &ex)))?) }; let out = json!({"include": hint.len(), "exclude": ex.len()}); masks_edit(s, c, "Object Mask", |masks, _| { let i = find(masks, mid, c)?; @@ -401,7 +408,7 @@ pub fn specs() -> Vec { .rposition(|x| matches!(x.shape, MaskShape::Object { .. } | MaskShape::Prompt { .. })) .ok_or_else(|| bad(c, "the mask has no Object or Describe selection"))?, }; - let started = s.segment_detail(id, mid, k).map_err(ai_err)?; + let started = segment(s, |s| s.segment_detail(id, mid, k))?; Ok(json!({"started": started})) } ), @@ -415,7 +422,7 @@ pub fn specs() -> Vec { |s, _| { let c = "segment.prepare"; let id = s.active().ok_or_else(|| bad(c, "no active photo"))?; - s.segment_prepare(id).map_err(ai_err)?; + segment(s, |s| s.segment_prepare(id))?; Ok(json!({"busy": s.segmenter.busy()})) } ), @@ -700,7 +707,7 @@ pub fn specs() -> Vec { |s, p| { let c = "spot.findBlemishes"; let id = s.active().ok_or_else(|| bad(c, "no active photo"))?; - let detector = super::face_detect::detector(s).map_err(|e| bad(c, e))?; + let detector = super::face_detect::detector(s).map_err(|e| e.into_error(c))?; // the uncropped photo, so positions are the spots' own coordinates let job = s.render_job(id, 1600, 1600, false, false).ok_or_else(|| bad(c, "no photo"))?; let img = job.run().rendered.map_err(|e| bad(c, e))?.image; diff --git a/crates/engine/src/errors.rs b/crates/engine/src/errors.rs new file mode 100644 index 000000000..beb4cefb4 --- /dev/null +++ b/crates/engine/src/errors.rs @@ -0,0 +1,68 @@ +//! What kind of error a command returned, for code that decides on it (#1046). +//! +//! A frontend that reacts to an error (offers a model download, says "cancelled" in its own words +//! instead of showing a failure) matches [`EngineError::kind`], never the message: messages are +//! for people, get reworded, and will be translated. + +use crate::EngineError; + +/// A model a feature needs and LightCraft downloads only when the person agrees. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +pub enum AiModel { + /// YuNet, which finds faces (`faces.detect`; installed in Settings ▸ Faces). + FaceDetector, + /// SAM 3, behind Object and Describe masks (`segment.model.download`). + Sam3, +} + +/// The kind of an [`EngineError`] (or of an error a frontend raised itself: [`ErrorKind::Other`]). +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +pub enum ErrorKind { + UnknownCommand, + Disabled, + BadParams, + Catalog, + NotSaved, + LibraryInUse, + LightroomCatalog, + /// The work was cancelled: not a failure. + Cancelled, + /// The feature needs this model, which is not installed (or still downloading). + ModelMissing(AiModel), + Other, +} + +impl EngineError { + /// What kind of error this is: decide on this, not on the message. + pub fn kind(&self) -> ErrorKind { + match self { + Self::UnknownCommand(_) => ErrorKind::UnknownCommand, + Self::Disabled(..) => ErrorKind::Disabled, + Self::BadParams { .. } => ErrorKind::BadParams, + Self::Catalog(_) => ErrorKind::Catalog, + Self::NotSaved(_) => ErrorKind::NotSaved, + Self::LibraryInUse(_) => ErrorKind::LibraryInUse, + Self::LightroomCatalog { .. } => ErrorKind::LightroomCatalog, + Self::Cancelled(_) => ErrorKind::Cancelled, + Self::ModelMissing { model, .. } => ErrorKind::ModelMissing(*model), + Self::Other(_) => ErrorKind::Other, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_kind_does_not_depend_on_the_message() { + let cancelled = EngineError::Cancelled("Lightroom-Import abgebrochen".into()); + assert_eq!(cancelled.kind(), ErrorKind::Cancelled); + assert_eq!(cancelled.to_string(), "Lightroom-Import abgebrochen", "the message is shown as is"); + let missing = EngineError::ModelMissing { model: AiModel::Sam3, msg: "any wording".into() }; + assert_eq!(missing.kind(), ErrorKind::ModelMissing(AiModel::Sam3)); + assert_eq!(missing.to_string(), "any wording"); + // an `Other` whose text reads like a cancellation or a missing model is still `Other` + assert_eq!(EngineError::Other("The SAM 3 model is not installed; cancelled".into()).kind(), ErrorKind::Other); + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 3bc6c431e..cdd914583 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -29,6 +29,7 @@ pub mod demo; pub mod denoise; pub mod devices; pub mod display; +mod errors; pub mod export; pub mod face_download; #[cfg(not(target_arch = "wasm32"))] @@ -68,6 +69,7 @@ pub mod walk; use std::sync::Arc; pub use cmd::{CommandInfo, CommandSpec, command_specs, find_command}; +pub use errors::{AiModel, ErrorKind}; pub use fonts::{CRAFT_FONTS, CraftFont}; use lightcraft_catalog::{Catalog, Filter, Op, PhotoId, Sort}; use lightcraft_develop::DevelopSettings; @@ -98,6 +100,13 @@ pub enum EngineError { /// to import with `library.importLightroom`, when one was found. #[error("{} is a Lightroom Classic catalog, not a LightCraft library; import it into a LightCraft library with File ▸ Import Lightroom Catalog…{}", path.display(), catalog.as_ref().map_or(String::new(), |c| format!(" ({})", c.display())))] LightroomCatalog { path: std::path::PathBuf, catalog: Option }, + /// The work was cancelled (by the person, an agent or a newer request): not a failure. + #[error("{0}")] + Cancelled(String), + /// The feature needs `model`, which is not installed (or still downloading): a frontend + /// offers its download (terms first) on [`ErrorKind::ModelMissing`], not on the message. + #[error("{msg}")] + ModelMissing { model: AiModel, msg: String }, #[error("{0}")] Other(String), } diff --git a/crates/engine/src/lightroom_job.rs b/crates/engine/src/lightroom_job.rs index 33ed64a2d..317d81ef1 100644 --- a/crates/engine/src/lightroom_job.rs +++ b/crates/engine/src/lightroom_job.rs @@ -24,6 +24,12 @@ fn error(message: impl Into) -> crate::EngineError { crate::EngineError::BadParams { cmd: COMMAND.into(), msg: message.into() } } +/// The import was cancelled ([`crate::ErrorKind::Cancelled`]: not a failure). The message is the +/// one it always had, so control-channel and MCP clients see the same `error` text. +fn cancelled() -> crate::EngineError { + crate::EngineError::Cancelled(error("Lightroom import cancelled").to_string()) +} + fn stage_archive(archive_dir: Option<&Path>, data: &mut CatalogImport) -> crate::Result> { let archive_path = archive_dir.map(|dir| crate::lightroom_archive::store_best_effort(dir, data).map_err(error)).transpose()?.flatten(); if archive_dir.is_some() && archive_path.is_none() { @@ -180,14 +186,16 @@ impl LightroomJob { /// Read/probe/stage all source data off the owner thread. pub fn prepare(&mut self, cancel: &AtomicBool) -> crate::Result { if cancel.load(Ordering::Relaxed) { - return Err(error("Lightroom import cancelled")); + return Err(cancelled()); } self.import.total.store(1, Ordering::Relaxed); self.import.done.store(0, Ordering::Relaxed); - let mut data = lightroom_catalog::read_with_progress(&self.path, cancel, &self.import.total, &self.import.done).map_err(error)?; + // (reading stops with a message when `cancel` is set: the flag says it was cancelled) + let mut data = lightroom_catalog::read_with_progress(&self.path, cancel, &self.import.total, &self.import.done) + .map_err(|e| if cancel.load(Ordering::Relaxed) { cancelled() } else { error(e) })?; lightroom_catalog::validate_for_job(&data).map_err(error)?; if cancel.load(Ordering::Relaxed) { - return Err(error("Lightroom import cancelled")); + return Err(cancelled()); } self.import.done.fetch_add(1, Ordering::Relaxed); let paths: Vec = data @@ -206,7 +214,7 @@ impl LightroomJob { let normal = self.import.prepare(&paths, cancel); if cancel.load(Ordering::Relaxed) { normal.rollback(); - return Err(error("Lightroom import cancelled")); + return Err(cancelled()); } let archive_path = stage_archive(self.archive_dir.as_deref(), &mut data)?; let index_path = self.archive_dir.as_ref().map(|dir| dir.join("lightroom-index.json")); @@ -229,7 +237,7 @@ impl LightroomJob { pub fn commit_prepared(s: &mut crate::Session, mut prepared: PreparedLightroom) -> crate::Result { if prepared.cancelled { prepared.normal.rollback(); - return Err(error("Lightroom import cancelled")); + return Err(cancelled()); } if !prepared.token.matches(s) { prepared.normal.rollback(); @@ -340,11 +348,26 @@ mod tests { let mut session = crate::Session::new(); let before = session.catalog.to_snapshot(); let prepared = cancelled_prepared(&session); - assert!(commit_prepared(&mut session, prepared).is_err()); + let Err(e) = commit_prepared(&mut session, prepared) else { panic!("a cancelled import commits nothing") }; + assert_eq!(e.kind(), crate::ErrorKind::Cancelled, "{e}"); assert_eq!(session.catalog.to_snapshot(), before); assert!(session.undo.is_empty()); } + /// Cancelling while the catalog is read is [`crate::ErrorKind::Cancelled`] whatever the message + /// says, and a failure that is not a cancellation is not one, even when its text says "cancelled". + #[test] + fn a_cancelled_read_is_cancelled_by_kind() { + let mut session = crate::Session::new(); + let mut job = LightroomJob::new(&mut session, PathBuf::from("/nonexistent/cancelled.lrcat"), false).unwrap(); + let Err(e) = job.prepare(&AtomicBool::new(true)) else { panic!("cancelled before it started") }; + assert_eq!(e.kind(), crate::ErrorKind::Cancelled, "{e}"); + // the `error` text clients see is unchanged + assert_eq!(e.to_string(), "invalid parameters for `library.importLightroom`: Lightroom import cancelled"); + let Err(e) = job.prepare(&AtomicBool::new(false)) else { panic!("no such catalog") }; + assert_ne!(e.kind(), crate::ErrorKind::Cancelled, "{e}"); + } + #[test] fn library_identity_rejects_late_completion_after_reopen() { let mut session = crate::Session::new(); diff --git a/crates/engine/src/segment/download.rs b/crates/engine/src/segment/download.rs index 7147db09d..57ba4f341 100644 --- a/crates/engine/src/segment/download.rs +++ b/crates/engine/src/segment/download.rs @@ -20,6 +20,8 @@ pub struct Status { pub file: String, /// The last download's error (cleared when a new one starts). pub error: Option, + /// The last download was cancelled (its `error` says so in words): decide on this, not on the text. + pub cancelled: bool, /// The last download finished and the files are in place. pub finished: bool, } @@ -28,6 +30,7 @@ pub struct Status { struct Text { file: String, error: Option, + cancelled: bool, } #[derive(Default)] @@ -68,15 +71,17 @@ impl Downloader { { last.file.clone_from(&t.file); last.error.clone_from(&t.error); + last.cancelled = t.cancelled; } // (only the session's thread locks `last`) - let (file, error) = self.last.try_lock().map(|l| (l.file.clone(), l.error.clone())).unwrap_or_default(); + let (file, error, cancelled) = self.last.try_lock().map(|l| (l.file.clone(), l.error.clone(), l.cancelled)).unwrap_or_default(); Status { running: s.running.load(Ordering::SeqCst), done: s.done.load(Ordering::SeqCst), total: s.total.load(Ordering::SeqCst), file, error, + cancelled, finished: s.finished.load(Ordering::SeqCst), } } @@ -135,6 +140,7 @@ impl Downloader { }; let r = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| fetch::download(files, &mirrors, &dir, &opts, &s.cancel, &mut on_progress))); + let cancelled = matches!(r, Ok(Err(fetch::DownloadError::Cancelled))); let error = match r { Ok(Ok(())) => { s.finished.store(true, Ordering::SeqCst); @@ -147,7 +153,10 @@ impl Downloader { if let Some(e) = &error { log::warn!("SAM 3 download: {e}"); } - lock(&s.text).error = error; + let mut text = lock(&s.text); + text.cancelled = cancelled; + text.error = error; + drop(text); drop(guard); }); match spawned { diff --git a/crates/engine/src/segment/mod.rs b/crates/engine/src/segment/mod.rs index ec2c4ba85..c955dcc01 100644 --- a/crates/engine/src/segment/mod.rs +++ b/crates/engine/src/segment/mod.rs @@ -35,7 +35,7 @@ pub const MODEL_BYTES: u64 = 3_439_938_512; /// The model's licence (not LightCraft's): shown before downloading. pub const LICENSE_NAME: &str = "SAM License (Meta)"; pub const LICENSE_URL: &str = "https://github.com/facebookresearch/sam3/blob/main/LICENSE"; -/// How errors about a missing model start (the UI offers the download on it). +/// How errors about a missing model start (frontends decide on [`crate::ErrorKind::ModelMissing`], not on this text). pub const NOT_INSTALLED: &str = "The SAM 3 model is not installed"; /// Longest a waiting command (CLI, MCP) waits for the model (loading on a CPU can take minutes). #[cfg_attr(not(feature = "sam"), allow(dead_code))] @@ -111,6 +111,8 @@ pub struct DownloadStatus { pub total: u64, pub file: String, pub error: Option, + /// The last download was cancelled (it resumes next time): decide on this, not on `error`'s text. + pub cancelled: bool, pub finished: bool, } @@ -215,7 +217,15 @@ impl Segmenter { #[cfg(feature = "sam")] { let s = self.download.status(); - DownloadStatus { running: s.running, done: s.done, total: s.total, file: s.file, error: s.error, finished: s.finished } + DownloadStatus { + running: s.running, + done: s.done, + total: s.total, + file: s.file, + error: s.error, + cancelled: s.cancelled, + finished: s.finished, + } } #[cfg(not(feature = "sam"))] DownloadStatus::default() @@ -234,27 +244,36 @@ impl Segmenter { /// The model folder, if the model is there; otherwise an error that says what to do. pub fn model_dir(&self) -> Result { + self.require_model().map_err(|e| e.to_string()) + } + + /// [`Segmenter::model_dir`] for commands: a model that is not installed (or still downloading) is + /// [`crate::EngineError::ModelMissing`], so a frontend offers the download without reading the message. + pub fn require_model(&self) -> crate::Result { + let other = |why: &str| crate::EngineError::Other(why.into()); if !Self::AVAILABLE { - return Err("AI masks are not available in this build".into()); + return Err(other("AI masks are not available in this build")); } #[cfg(feature = "sam")] if lightcraft_segment::remote::configured().is_some() { return Ok(PathBuf::new()); } - let dir = self.dir.clone().ok_or("no folder is set for the SAM 3 model")?; + let dir = self.dir.clone().ok_or_else(|| other("no folder is set for the SAM 3 model"))?; if self.installed() { return Ok(dir); } let d = self.download_status(); - if d.running { + let msg = if d.running { let pct = d.done.saturating_mul(100).checked_div(d.total).unwrap_or(0); - return Err(format!("{NOT_INSTALLED} yet: it is downloading ({pct} %).")); - } - Err(format!( - "{NOT_INSTALLED}. Download it (about {:.1} GB, {LICENSE_NAME}) when LightCraft offers it, with `segment.model.download {{\"acknowledged\": true}}`, or put model.safetensors, vocab.json and merges.txt in {}.", - MODEL_BYTES as f64 / 1e9, - dir.display() - )) + format!("{NOT_INSTALLED} yet: it is downloading ({pct} %).") + } else { + format!( + "{NOT_INSTALLED}. Download it (about {:.1} GB, {LICENSE_NAME}) when LightCraft offers it, with `segment.model.download {{\"acknowledged\": true}}`, or put model.safetensors, vocab.json and merges.txt in {}.", + MODEL_BYTES as f64 / 1e9, + dir.display() + ) + }; + Err(crate::EngineError::ModelMissing { model: crate::AiModel::Sam3, msg }) } /// Start downloading the model (on a background thread; see `segment.model.download`). diff --git a/crates/engine/src/tests_face_models.rs b/crates/engine/src/tests_face_models.rs index f1c27c849..a308f20fb 100644 --- a/crates/engine/src/tests_face_models.rs +++ b/crates/engine/src/tests_face_models.rs @@ -205,10 +205,24 @@ fn detect_without_the_model_says_where_to_get_it() { let d = temp("nodetector"); let mut s = Session::with_demo(); // no folder for models at all, then a folder that has none yet - assert!(s.execute("faces.detect", &json!({})).is_err()); + let e = s.execute("faces.detect", &json!({})).unwrap_err(); + assert_ne!(e.kind(), crate::ErrorKind::ModelMissing(crate::AiModel::FaceDetector), "nothing to download into: {e}"); s.face_models_dir = Some(d.join("models")); - let e = s.execute("faces.detect", &json!({})).unwrap_err().to_string(); - assert!(e.contains("Settings > Faces"), "{e}"); + let e = s.execute("faces.detect", &json!({})).unwrap_err(); + // the app offers the download on the kind (#1046), the person reads the message + assert_eq!(e.kind(), crate::ErrorKind::ModelMissing(crate::AiModel::FaceDetector), "{e}"); + assert!( + e.to_string().starts_with("invalid parameters for `faces.detect`: ") && e.to_string().contains("Settings > Faces"), + "the `error` text clients see is unchanged: {e}" + ); + // a detector that is there but unusable also points to Settings > Faces, and is not a missing one + let home = d.join("models").join(lightcraft_faces::known::YUNET_ID); + std::fs::create_dir_all(&home).unwrap(); + std::fs::write(home.join("model.onnx"), b"not a model").unwrap(); + std::fs::write(home.join("face-model.json"), b"{}").unwrap(); + let e = s.execute("faces.detect", &json!({})).unwrap_err(); + assert!(e.to_string().contains("Settings > Faces"), "{e}"); + assert_eq!(e.kind(), crate::ErrorKind::BadParams, "{e}"); let _ = std::fs::remove_dir_all(&d); } diff --git a/crates/engine/src/tests_segment.rs b/crates/engine/src/tests_segment.rs index ccca1cb23..f4b9f4f48 100644 --- a/crates/engine/src/tests_segment.rs +++ b/crates/engine/src/tests_segment.rs @@ -115,6 +115,7 @@ fn a_failing_download_ends_with_an_error_and_never_blocks() { } let st = s.segmenter.download_status(); assert!(!st.finished); + assert!(!st.cancelled, "a failure is not a cancel"); let e = st.error.unwrap(); assert!(e.contains("not found"), "{e}"); assert!(!s.segmenter.installed()); @@ -272,9 +273,35 @@ fn a_running_model_download_shows_in_activity_and_cancels() { assert!(t.elapsed() < Duration::from_secs(10), "the download never stopped"); std::thread::sleep(Duration::from_millis(10)); } - let e = s.segmenter.download_status().error.unwrap_or_default(); + let st = s.segmenter.download_status(); + assert!(st.cancelled, "the app tells a cancel by the flag (#1046): {st:?}"); + let e = st.error.unwrap_or_default(); assert!(e.contains("cancelled"), "{e}"); assert!(!s.segmenter.installed()); let _ = std::fs::remove_dir_all(&dir); let _ = std::fs::remove_file(&mirrors); } + +/// Frontends decide on the error's kind, not its words (#1046): a missing model is +/// `ModelMissing(Sam3)` whether the app queues requests (background) or a command waits (CLI, MCP); +/// a build without AI masks is not a missing model (there is nothing to download). +#[test] +fn a_missing_model_is_model_missing_by_kind() { + use crate::{AiModel, ErrorKind}; + for background in [true, false] { + let mut s = Session::with_demo(); + s.segmenter.background = background; + s.segmenter.dir = Some(tmp(&format!("kind-{background}"))); + for (cmd, p) in + [("mask.add", json!({"kind": "object"})), ("mask.add", json!({"kind": "prompt", "text": "sky"})), ("segment.prepare", json!({}))] + { + let e = s.execute(cmd, &p).unwrap_err(); + if Segmenter::AVAILABLE { + assert_eq!(e.kind(), ErrorKind::ModelMissing(AiModel::Sam3), "{cmd} (background {background}): {e}"); + assert!(e.to_string().starts_with(NOT_INSTALLED), "the message is unchanged: {e}"); + } else { + assert_eq!(e.kind(), ErrorKind::Other, "{cmd}: {e}"); + } + } + } +}