diff --git a/Cargo.lock b/Cargo.lock index e4ba5385..b6ae1910 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -971,7 +971,7 @@ dependencies = [ [[package]] name = "craft-pdf" version = "0.1.0" -source = "git+https://github.com/storytold/craft-libs?rev=89dcbb3060377a26c99a6d80caa16084366704c4#89dcbb3060377a26c99a6d80caa16084366704c4" +source = "git+https://github.com/storytold/craft-libs?rev=ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d#ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d" dependencies = [ "hayro-syntax", "thiserror 2.0.21", @@ -1158,7 +1158,7 @@ dependencies = [ [[package]] name = "effectcraft" -version = "0.9.2" +version = "0.10.0" dependencies = [ "cpal", "effectcraft-engine", @@ -1183,7 +1183,7 @@ dependencies = [ [[package]] name = "effectcraft-automation" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-engine", "effectcraft-host", @@ -1195,14 +1195,14 @@ dependencies = [ [[package]] name = "effectcraft-av1enc" -version = "0.9.2" +version = "0.10.0" dependencies = [ "filmcraft-av1", ] [[package]] name = "effectcraft-cli" -version = "0.9.2" +version = "0.10.0" dependencies = [ "cpu-time", "effectcraft-automation", @@ -1218,7 +1218,7 @@ dependencies = [ [[package]] name = "effectcraft-color" -version = "0.9.2" +version = "0.10.0" dependencies = [ "proptest", "serde", @@ -1226,7 +1226,7 @@ dependencies = [ [[package]] name = "effectcraft-effects" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-color", "effectcraft-geom", @@ -1243,7 +1243,7 @@ dependencies = [ [[package]] name = "effectcraft-engine" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-color", "effectcraft-effects", @@ -1276,7 +1276,7 @@ dependencies = [ [[package]] name = "effectcraft-export" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-av1enc", "effectcraft-color", @@ -1306,7 +1306,7 @@ dependencies = [ [[package]] name = "effectcraft-expr" -version = "0.9.2" +version = "0.10.0" dependencies = [ "boa_engine", "effectcraft-color", @@ -1323,7 +1323,7 @@ dependencies = [ [[package]] name = "effectcraft-geom" -version = "0.9.2" +version = "0.10.0" dependencies = [ "proptest", "serde", @@ -1331,7 +1331,7 @@ dependencies = [ [[package]] name = "effectcraft-gpu" -version = "0.9.2" +version = "0.10.0" dependencies = [ "bytemuck", "effectcraft-color", @@ -1353,14 +1353,14 @@ dependencies = [ [[package]] name = "effectcraft-hevcenc" -version = "0.9.2" +version = "0.10.0" dependencies = [ "filmcraft-hevc", ] [[package]] name = "effectcraft-host" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-engine", "effectcraft-export", @@ -1385,7 +1385,7 @@ dependencies = [ [[package]] name = "effectcraft-interchange" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-color", "effectcraft-keyframe", @@ -1404,7 +1404,7 @@ dependencies = [ [[package]] name = "effectcraft-keyframe" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-color", "effectcraft-geom", @@ -1416,7 +1416,7 @@ dependencies = [ [[package]] name = "effectcraft-lottie" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-color", "effectcraft-effects", @@ -1433,7 +1433,7 @@ dependencies = [ [[package]] name = "effectcraft-media" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-model", "effectcraft-pdf", @@ -1460,7 +1460,7 @@ dependencies = [ [[package]] name = "effectcraft-model" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-geom", "image", @@ -1472,14 +1472,14 @@ dependencies = [ [[package]] name = "effectcraft-opusenc" -version = "0.9.2" +version = "0.10.0" dependencies = [ "filmcraft-opus", ] [[package]] name = "effectcraft-path" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-geom", "effectcraft-keyframe", @@ -1491,7 +1491,7 @@ dependencies = [ [[package]] name = "effectcraft-pdf" -version = "0.9.2" +version = "0.10.0" dependencies = [ "craft-pdf", "effectcraft-raster", @@ -1507,7 +1507,7 @@ dependencies = [ [[package]] name = "effectcraft-plugin" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-effects", "effectcraft-keyframe", @@ -1519,7 +1519,7 @@ dependencies = [ [[package]] name = "effectcraft-project" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-color", "effectcraft-geom", @@ -1532,14 +1532,14 @@ dependencies = [ [[package]] name = "effectcraft-psd" -version = "0.9.2" +version = "0.10.0" dependencies = [ "miniz_oxide 0.8.9", ] [[package]] name = "effectcraft-raster" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-color", "effectcraft-geom", @@ -1548,7 +1548,7 @@ dependencies = [ [[package]] name = "effectcraft-render" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-color", "effectcraft-effects", @@ -1570,7 +1570,7 @@ dependencies = [ [[package]] name = "effectcraft-script" -version = "0.9.2" +version = "0.10.0" dependencies = [ "boa_engine", "effectcraft-engine", @@ -1587,7 +1587,7 @@ dependencies = [ [[package]] name = "effectcraft-segment" -version = "0.9.2" +version = "0.10.0" dependencies = [ "miniz_oxide 0.8.9", "ndarray", @@ -1596,7 +1596,7 @@ dependencies = [ [[package]] name = "effectcraft-svg" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-geom", "effectcraft-path", @@ -1608,7 +1608,7 @@ dependencies = [ [[package]] name = "effectcraft-text" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-keyframe", "harfrust", @@ -1620,7 +1620,7 @@ dependencies = [ [[package]] name = "effectcraft-time" -version = "0.9.2" +version = "0.10.0" dependencies = [ "proptest", "serde", @@ -1628,7 +1628,7 @@ dependencies = [ [[package]] name = "effectcraft-track" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-raster", "effectcraft-segment", @@ -1640,7 +1640,7 @@ dependencies = [ [[package]] name = "effectcraft-ui-egui" -version = "0.9.2" +version = "0.10.0" dependencies = [ "craft-ui", "effectcraft-color", @@ -1672,11 +1672,11 @@ dependencies = [ [[package]] name = "effectcraft-vp9enc" -version = "0.9.2" +version = "0.10.0" [[package]] name = "effectcraft-web" -version = "0.9.2" +version = "0.10.0" dependencies = [ "effectcraft-engine", "effectcraft-export", @@ -2719,7 +2719,7 @@ checksum = "c75ab947623ef4ccaa7acf0579edf7cbb5a73838e3839a7be73335e522f433a1" [[package]] name = "hayro-syntax" version = "0.7.2" -source = "git+https://github.com/storytold/craft-libs?rev=89dcbb3060377a26c99a6d80caa16084366704c4#89dcbb3060377a26c99a6d80caa16084366704c4" +source = "git+https://github.com/storytold/craft-libs?rev=ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d#ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d" dependencies = [ "hayro-jbig2", "hayro-jpeg2000", @@ -6616,7 +6616,7 @@ checksum = "e450f9b2ed1dff33c94c12589a87338689467b9c4f5d8a5710bd09a847d2c8a7" [[package]] name = "xtask" -version = "0.9.2" +version = "0.10.0" dependencies = [ "serde_json", ] diff --git a/Cargo.toml b/Cargo.toml index 033e8a6e..5bcb4bf1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,14 +4,14 @@ members = ["crates/*", "apps/*", "xtask"] default-members = ["crates/*", "apps/*"] [workspace.package] -version = "0.9.2" +version = "0.10.0" edition = "2024" license = "MIT OR Apache-2.0" rust-version = "1.95" repository = "https://github.com/storytold/effectcraft" [workspace.dependencies] -craft-pdf = { git = "https://github.com/storytold/craft-libs", rev = "89dcbb3060377a26c99a6d80caa16084366704c4", version = "=0.1.0", default-features = false, features = ["read"] } +craft-pdf = { git = "https://github.com/storytold/craft-libs", rev = "ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d", version = "=0.1.0", default-features = false, features = ["read"] } # internal effectcraft-time = { path = "crates/time" } effectcraft-geom = { path = "crates/geom" } @@ -207,4 +207,4 @@ lto = "thin" codegen-units = 1 [patch.crates-io] -hayro-syntax = { git = "https://github.com/storytold/craft-libs", rev = "89dcbb3060377a26c99a6d80caa16084366704c4", version = "=0.7.2" } +hayro-syntax = { git = "https://github.com/storytold/craft-libs", rev = "ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d", version = "=0.7.2" } diff --git a/crates/automation/src/bridge.rs b/crates/automation/src/bridge.rs index eab1c01d..77d69dd0 100644 --- a/crates/automation/src/bridge.rs +++ b/crates/automation/src/bridge.rs @@ -1,5 +1,5 @@ //! Blocking client for the desktop app's JSON-lines control channel (`docs/control-protocol.md`). -//! One persistent loopback connection, reconnected once if the app went away. +//! One persistent loopback connection. Only explicitly read-only queries can be resent. use std::io::{BufRead, BufReader, Write}; use std::net::{TcpStream, ToSocketAddrs}; @@ -50,16 +50,20 @@ impl BridgeClient { /// Call a control method; returns the reply's `result`, or the app's `error` as [`Error::App`]. pub fn call(&mut self, method: &str, params: Value) -> Result { + let retry_read = + matches!(method, "engine.commands" | "ui.menu.list" | "ui.inspect" | "ui.elements" | "ui.viewer.locate" | "ui.viewer.hit" | "ui.timeline.locate") + || (method == "ui.playback" && params["action"] == "status"); + let unknown = |error| Error::Bridge(format!("`{method}` request outcome is unknown: {error}; inspect the app before retrying")); let mut last = Error::Bridge("not attempted".into()); for attempt in 0..2 { if self.conn.is_none() { self.conn = Some(self.connect()?); } let id = self.next_id; - self.next_id += 1; + self.next_id = self.next_id.checked_add(1).ok_or_else(|| Error::Bridge("control request id space exhausted".into()))?; let Some((r, w)) = self.conn.as_mut() else { continue }; let line = format!("{}\n", json!({"id": id, "method": method, "params": params})); - // `Ok(Err(_))`: the connection is dead before a reply (safe to resend once); + // `Ok(Err(_))`: transmission/reply failed (the app may have applied the request); // `Err(_)`: a timeout or bad reply (never resent: the app may have run the request). let res = (|| -> Result> { if let Err(e) = w.write_all(line.as_bytes()) { @@ -80,8 +84,11 @@ impl BridgeClient { Ok(Ok(v)) if v.get("ok").and_then(Value::as_bool) == Some(true) => return Ok(v.get("result").cloned().unwrap_or(Value::Null)), Ok(Ok(v)) => return Err(Error::App(v.get("error").and_then(Value::as_str).unwrap_or("error").to_string())), Ok(Err(e)) => { - // Stale connection (app restarted): reconnect and resend once. self.conn = None; + if !retry_read { + return Err(unknown(e)); + } + // A read-only query can be repeated after a stale connection or lost reply. last = e; if attempt == 1 { break; @@ -90,7 +97,7 @@ impl BridgeClient { Err(e) => { // The stream may still deliver the late reply: start fresh next call. self.conn = None; - return Err(e); + return Err(if retry_read { e } else { unknown(e) }); } } } diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index 6bb6fbc5..6e622b37 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -511,6 +511,128 @@ fn bridge_rejects_non_loopback() { assert!(Backend::bridge("localhost:9877").is_ok()); } +/// The app applies the first request but loses its reply. The next connection can be either +/// an automatic retry or an explicit follow-up; the returned counts distinguish them. +fn lost_reply_server(first_reply: Option<&'static str>) -> (String, std::thread::JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let worker = std::thread::spawn(move || { + let mut session = Session::default(); + let mut mutations = 0; + for attempt in 0..2 { + let (stream, _) = listener.accept().unwrap(); + stream.set_read_timeout(Some(std::time::Duration::from_secs(5))).unwrap(); + let mut output = stream.try_clone().unwrap(); + let mut input = String::new(); + BufReader::new(stream).read_line(&mut input).unwrap(); + let request: Value = serde_json::from_str(&input).unwrap(); + let result = if request["method"] == "engine.execute" { + mutations += 1; + session.execute(request["params"]["command"].as_str().unwrap(), request["params"]["params"].clone()).unwrap() + } else if request["method"] == "ui.inspect" || (request["method"] == "ui.playback" && request["params"]["action"] == "status") { + json!({"mutations": mutations, "requests": attempt + 1, "comps": session.project.comps().count()}) + } else { + mutations += 1; + json!({"mutations": mutations}) + }; + if attempt == 1 { + writeln!(output, "{}", json!({"id": request["id"], "ok": true, "result": result})).unwrap(); + } else if let Some(reply) = first_reply { + writeln!(output, "{reply}").unwrap(); + } + } + }); + (addr, worker) +} + +#[test] +fn bridge_does_not_replay_a_mutation_when_its_reply_is_lost() { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let error = backend.exec("comp.new", json!({"width": 64, "height": 48, "duration": 1})).unwrap_err(); + assert!(error.to_string().contains("outcome is unknown"), "{error}"); + let state = backend.control("ui.inspect", json!({})).unwrap(); + assert_eq!(state["mutations"], 1, "the first command was applied exactly once"); + assert_eq!(state["comps"], 1); + worker.join().unwrap(); +} + +#[test] +fn bridge_retries_read_only_queries_after_a_lost_reply() { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let state = backend.control("ui.inspect", json!({})).unwrap(); + assert_eq!(state["requests"], 2); + assert_eq!(state["mutations"], 0); + worker.join().unwrap(); +} + +#[test] +fn bridge_reports_unknown_outcome_after_a_malformed_mutation_reply() { + let (addr, worker) = lost_reply_server(Some("{invalid reply")); + let mut backend = Backend::bridge(&addr).unwrap(); + let error = backend.exec("comp.new", json!({"width": 64, "height": 48, "duration": 1})).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("outcome is unknown") && message.contains("bad reply"), "{message}"); + assert_eq!(backend.control("ui.inspect", json!({})).unwrap()["comps"], 1); + worker.join().unwrap(); +} + +#[test] +fn bridge_does_not_assume_an_unknown_method_is_read_only() { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let error = backend.control("future.customOperation", json!({})).unwrap_err(); + assert!(error.to_string().contains("outcome is unknown"), "{error}"); + assert_eq!(backend.control("ui.inspect", json!({})).unwrap()["mutations"], 1); + worker.join().unwrap(); +} + +#[test] +fn bridge_playback_edits_and_default_toggle_are_not_replayed() { + for params in [json!({}), json!({"action": "toggle"}), json!({"action": "play"}), json!({"action": "stop"})] { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let error = backend.control("ui.playback", params).unwrap_err(); + assert!(error.to_string().contains("outcome is unknown"), "{error}"); + assert_eq!(backend.control("ui.inspect", json!({})).unwrap()["mutations"], 1); + worker.join().unwrap(); + } +} + +#[test] +fn bridge_playback_status_can_be_retried() { + let (addr, worker) = lost_reply_server(None); + let mut backend = Backend::bridge(&addr).unwrap(); + let state = backend.control("ui.playback", json!({"action": "status"})).unwrap(); + assert_eq!(state["requests"], 2); + assert_eq!(state["mutations"], 0); + worker.join().unwrap(); +} + +#[test] +fn bridge_connect_failure_can_be_retried_when_the_app_starts() { + let reserved = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = reserved.local_addr().unwrap(); + drop(reserved); + let mut backend = Backend::bridge(&addr.to_string()).unwrap(); + let error = backend.control("ui.inspect", json!({})).unwrap_err(); + assert!(error.to_string().contains("cannot connect"), "{error}"); + assert!(!error.to_string().contains("outcome is unknown"), "nothing was sent"); + let listener = TcpListener::bind(addr).unwrap(); + let worker = std::thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + stream.set_read_timeout(Some(std::time::Duration::from_secs(5))).unwrap(); + let mut output = stream.try_clone().unwrap(); + let mut input = String::new(); + BufReader::new(stream).read_line(&mut input).unwrap(); + let request: Value = serde_json::from_str(&input).unwrap(); + writeln!(output, "{}", json!({"id": request["id"], "ok": true, "result": {"ready": true}})).unwrap(); + }); + assert_eq!(backend.control("ui.inspect", json!({})).unwrap()["ready"], true); + worker.join().unwrap(); +} + #[test] fn script_ui_and_history_tools() { let mut sess = Session::default(); diff --git a/crates/engine/src/config.rs b/crates/engine/src/config.rs index c333c973..00e7377b 100644 --- a/crates/engine/src/config.rs +++ b/crates/engine/src/config.rs @@ -164,32 +164,50 @@ impl ConfigStore for DirConfig { } } -/// The temporary file an atomic write of `path` goes through. -pub fn temp_path(path: &Path) -> PathBuf { - let mut name = path.file_name().map(|n| n.to_os_string()).unwrap_or_default(); - name.push(".tmp"); - path.with_file_name(name) +/// A new, empty temporary file next to `path` for an atomic write of it, and its path. The name +/// is unique to this process and this write (`.effectcraft--.tmp`) and the file is created +/// exclusively, so two writers never share one and nothing already at that name (a file, a +/// symbolic link) is opened or followed (#1233). It ends in `.tmp`, so a file left by a hard +/// kill never counts as an auto-save. Its bounded name does not include the destination's name, +/// so a valid long destination name still fits the filesystem's component limit. +fn create_temp(path: &Path) -> std::io::Result<(std::fs::File, PathBuf)> { + use std::sync::atomic::{AtomicU64, Ordering}; + static NEXT: AtomicU64 = AtomicU64::new(0); + let mut last = None; + for _ in 0..16 { + let n = NEXT.fetch_add(1, Ordering::Relaxed); + let tmp = path.with_file_name(format!(".effectcraft-{}-{n}.tmp", std::process::id())); + match std::fs::OpenOptions::new().write(true).create_new(true).open(&tmp) { + Ok(f) => return Ok((f, tmp)), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => last = Some(e), + Err(e) => return Err(e), + } + } + Err(last.unwrap_or_else(|| std::io::Error::other("no free temporary file name"))) } /// Write `data` to `path` atomically: write a temporary file next to it, flush it to disk, then /// rename it over `path`. A crash at any point leaves either the old file or the new one, never a -/// torn file. +/// torn file, and concurrent writers each publish a whole file of their own. pub fn atomic_write(path: &Path, data: &[u8]) -> std::io::Result<()> { - atomic_write_with(path, data, |tmp, data| { + atomic_write_with(path, data, |f, data| { use std::io::Write; - let mut f = std::fs::File::create(tmp)?; f.write_all(data)?; f.sync_all() }) } -/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write). A -/// failed write removes the temporary file and leaves `path` untouched. -pub fn atomic_write_with(path: &Path, data: &[u8], write: impl FnOnce(&Path, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> { - let tmp = temp_path(path); - if let Err(e) = write(&tmp, data) { +/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write); it is +/// given the temporary file, already created. A failed write or rename removes the temporary +/// file and leaves `path` untouched. +pub fn atomic_write_with(path: &Path, data: &[u8], write: impl FnOnce(&mut std::fs::File, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> { + let (mut f, tmp) = create_temp(path)?; + let written = write(&mut f, data); + // Closed before the rename (Windows can't rename an open file). + drop(f); + if let Err(e) = written.and_then(|()| std::fs::rename(&tmp, path)) { let _ = std::fs::remove_file(&tmp); return Err(e); } - std::fs::rename(&tmp, path) + Ok(()) } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 80956687..96e09b57 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -495,6 +495,8 @@ pub struct Session { /// Offloaded jobs that ended, newest last (kind, error): how a caller waiting for one /// (`wait: true` over the control channel in the browser) learns its outcome. pub offload_log: Vec<(offload::JobKind, Option)>, + /// Number of older completion entries evicted from the bounded offload log. + pub offload_log_start: usize, /// Generic background tasks (Content-Aware Fill, Scene Edit Detection), see [`jobs`]. pub tasks: Vec, /// Finished tasks, newest last (Progress panel, `jobs.list`). @@ -598,6 +600,7 @@ impl Default for Session { offload: None, offloaded: vec![], offload_log: vec![], + offload_log_start: 0, browser: None, storage: None, tasks: vec![], diff --git a/crates/engine/src/offload.rs b/crates/engine/src/offload.rs index 561ebe6c..bd1fec8a 100644 --- a/crates/engine/src/offload.rs +++ b/crates/engine/src/offload.rs @@ -171,6 +171,20 @@ pub enum JobKind { } impl JobKind { + /// The corresponding `jobs.list` id, also used by waiting control commands. + pub fn job_id(self) -> &'static str { + match self { + JobKind::Render => "render", + JobKind::Warp => "warp", + JobKind::WarpPlan => "warpPlan", + JobKind::Camera => "camera", + JobKind::RotoFreeze | JobKind::RotoPropagate => "roto", + JobKind::Track => "track", + JobKind::MaskTrack => "maskTrack", + JobKind::ContentFill => "contentFill", + } + } + /// The undo step an analysis result becomes. pub fn label(self) -> &'static str { match self { @@ -699,6 +713,9 @@ pub struct OffloadedJob { /// Segmentations received (Roto Brush propagation). pub segs: u64, started: web_time::Instant, + /// The analysed group as the job started: its result is merged with what changed since + /// ([`merge_group`]). + base: Option>, } /// A Render Queue job running in a worker (see [`crate::render_queue::RenderJob`]). @@ -730,6 +747,7 @@ impl Session { } let id = next_id(); let inbox = Arc::new(Inbox::default()); + let base = self.project.comp(comp).and_then(|c| c.layer(layer)).and_then(|l| l.props.find_group(uid)).cloned().map(Box::new); let req = WorkerRequest { id, project: self.project.to_json(), files: footage_files(&self.project), job }; off.start(req, inbox.clone())?; self.offloaded.push(OffloadedJob { @@ -742,6 +760,7 @@ impl Session { progress: OffloadProgress::default(), segs: 0, started: web_time::Instant::now(), + base, }); self.events.push(Event::ProjectChanged { revision: self.revision }); Ok(()) @@ -779,7 +798,7 @@ impl Session { j.progress.done = done; j.progress.total = total; } - WorkerReply::Group { comp, layer, group, message } => results.push((j.kind, comp, layer, group, message)), + WorkerReply::Group { comp, layer, group, message } => results.push((j.id, j.kind, comp, layer, group, message, j.base.clone())), WorkerReply::Fill { plan, files } => fills.push((plan, files)), WorkerReply::WarpPlan { key, summary } => effectcraft_effects::warp_stab::store_summary(key, *summary), WorkerReply::Segs { segs } => { @@ -802,7 +821,7 @@ impl Session { Err(e) => self.events.push(Event::Toast { message: format!("Content-Aware Fill: {e}"), error: true }), } } - for (kind, comp, layer, group, message) in results { + for (id, kind, comp, layer, group, message, base) in results { let uid = group.uid; let r = self.edit(kind.label(), None, |p, _| { let g = p @@ -810,12 +829,31 @@ impl Session { .and_then(|c| c.layer_mut(layer)) .and_then(|l| l.props.find_group_mut(uid)) .ok_or_else(|| crate::EngineError::Other("the analysed property was removed during the analysis".into()))?; - *g = *group; + // Edits made while the job ran stay (#1249): only what the analysis changed is + // taken from its result. + *g = match base { + Some(base) => merge_group(&base, g, &group).ok_or_else(|| { + crate::EngineError::Other(format!( + "“{}” was changed during the analysis where its result changes it too, so nothing was applied: analyse again", + g.name + )) + })?, + None => *group, + }; Ok(()) }); match r { Ok(()) => self.events.push(Event::Toast { message, error: false }), - Err(e) => self.events.push(Event::Toast { message: format!("{}: {e}", kind.label()), error: true }), + Err(e) => { + // Completion consumers read the job's error, not its toasts. Keep an apply + // rejection on the originating job; Done logs and reports it just like a + // WorkerReply::Failed, without failing another concurrent analysis. + if let Some(job) = self.offloaded.iter_mut().find(|j| j.id == id) { + job.progress.error = Some(e.to_string()); + } else { + self.events.push(Event::Toast { message: format!("{}: {e}", kind.label()), error: true }); + } + } } } let mut done = vec![]; @@ -830,6 +868,7 @@ impl Session { self.offload_log.push((kind, p.error.clone().or_else(|| p.cancelled.then(|| "stopped".to_string())))); if self.offload_log.len() > 32 { self.offload_log.remove(0); + self.offload_log_start = self.offload_log_start.saturating_add(1); } if let Some(e) = p.error { self.events.push(Event::Toast { message: format!("{}: {e}", kind.label()), error: true }); @@ -880,6 +919,76 @@ impl Session { } } +/// `live` if only it changed since `base`, `result` if only that did (or both the same way); +/// `None` when both changed it differently. +fn pick(base: &T, live: &T, result: &T) -> Option { + if live == base { + Some(result.clone()) + } else if result == base || result == live { + Some(live.clone()) + } else { + None + } +} + +fn node_uid(n: &effectcraft_project::Node) -> Uid { + match n { + effectcraft_project::Node::Prop(p) => p.uid, + effectcraft_project::Node::Group(g) => g.uid, + } +} + +/// Merge an analysis `result` into the `live` group (#1249). `base` is the group as the job +/// started. The group's own fields and each property are taken from whichever side changed +/// them, groups are merged child by child (matched by uid), and a child added or removed by one +/// side is added or removed. `None` when both sides changed the same field or property +/// differently, one side removed a child the other changed, or both reordered or added and +/// removed children differently: a conflict. +pub(crate) fn merge_group(base: &PropGroup, live: &PropGroup, result: &PropGroup) -> Option { + use effectcraft_project::Node; + if live == base { + return Some(result.clone()); + } + if result == base || result == live { + return Some(live.clone()); + } + let header = |g: &PropGroup| PropGroup { children: vec![], ..g.clone() }; + let mut out = pick(&header(base), &header(live), &header(result))?; + let ids = |g: &PropGroup| g.children.iter().map(node_uid).collect::>(); + let (bi, li, ri) = (ids(base), ids(live), ids(result)); + // The children's order and set: from the side that changed them. + let order = if li == bi { + ri + } else if ri == bi || ri == li { + li + } else { + return None; + }; + fn find(g: &PropGroup, u: Uid) -> Option<&effectcraft_project::Node> { + g.children.iter().find(|n| node_uid(n) == u) + } + // A child one side removed must be unchanged on the other, or that side's edit would be lost. + for u in bi.iter().filter(|u| !order.contains(u)) { + let b = find(base, *u); + if [find(live, *u), find(result, *u)].into_iter().flatten().any(|n| Some(n) != b) { + return None; + } + } + for u in order { + let node = match (find(base, u), find(live, u), find(result, u)) { + (Some(Node::Group(b)), Some(Node::Group(l)), Some(Node::Group(r))) => Node::Group(merge_group(b, l, r)?), + (Some(b), Some(l), Some(r)) => pick(b, l, r)?, + // Added by one side, or by both alike. + (None, Some(l), None) => l.clone(), + (None, None, Some(r)) => r.clone(), + (None, Some(l), Some(r)) if l == r => l.clone(), + _ => return None, + }; + out.children.push(node); + } + Some(out) +} + #[cfg(test)] mod tests { use super::*; @@ -892,6 +1001,69 @@ mod tests { a } + /// A group with properties `a` (uid 2) and `b` (uid 3) and a sub-group (uid 4) holding `c` + /// (uid 5). + fn group() -> PropGroup { + use effectcraft_keyframe::Value as KV; + use effectcraft_project::{Node, Property}; + let mut sub = PropGroup::new(4, "sub", "Sub"); + sub.children.push(Node::Prop(Property::new(5, "c", "C", KV::Scalar(0.0)))); + let mut g = PropGroup::new(1, "fx", "Effect"); + g.children.push(Node::Prop(Property::new(2, "a", "A", KV::Scalar(0.0)))); + g.children.push(Node::Prop(Property::new(3, "b", "B", KV::Scalar(0.0)))); + g.children.push(Node::Group(sub)); + g + } + + fn set(g: &mut PropGroup, uid: Uid, v: f64) { + g.find_mut(uid).unwrap().value = effectcraft_keyframe::Value::Scalar(v); + } + + fn get(g: &PropGroup, uid: Uid) -> f64 { + match g.find(uid).unwrap().value { + effectcraft_keyframe::Value::Scalar(v) => v, + ref v => panic!("{v:?}"), + } + } + + #[test] + fn analysis_results_merge_with_edits_made_meanwhile() { + use effectcraft_project::Node; + let base = group(); + // The user changes `a` and switches the effect off; the analysis writes `b` and `c`. + let mut live = base.clone(); + set(&mut live, 2, 1.0); + live.enabled = false; + let mut result = base.clone(); + set(&mut result, 3, 2.0); + set(&mut result, 5, 3.0); + let m = merge_group(&base, &live, &result).unwrap(); + assert_eq!((get(&m, 2), get(&m, 3), get(&m, 5), m.enabled), (1.0, 2.0, 3.0, false)); + // Unchanged on either side: the other side wins whole. + assert_eq!(merge_group(&base, &base, &result).unwrap(), result); + assert_eq!(merge_group(&base, &live, &base).unwrap(), live); + // Both changed `b`, differently: a conflict (nothing is applied). The same way: fine. + let mut both = live.clone(); + set(&mut both, 3, 9.0); + assert!(merge_group(&base, &both, &result).is_none()); + set(&mut both, 3, 2.0); + assert_eq!(get(&merge_group(&base, &both, &result).unwrap(), 3), 2.0); + // The analysis adds a child (a result group) while the user edits `a`: both kept. + let mut added = result.clone(); + added.children.push(Node::Group(PropGroup::new(6, "out", "Output"))); + let m = merge_group(&base, &live, &added).unwrap(); + assert!(m.find_group(6).is_some() && get(&m, 2) == 1.0); + // The analysis drops the sub-group the user didn't touch: dropped. + let mut dropped = result.clone(); + dropped.children.retain(|n| !matches!(n, Node::Group(g) if g.uid == 4)); + let m = merge_group(&base, &live, &dropped).unwrap(); + assert!(m.find_group(4).is_none() && get(&m, 2) == 1.0); + // …but if the user changed something in it, that's a conflict. + let mut touched = live.clone(); + set(&mut touched, 5, 7.0); + assert!(merge_group(&base, &touched, &dropped).is_none()); + } + #[test] fn protocol_round_trips() { let mut s = Session::default(); diff --git a/crates/engine/src/tests_mask_warp.rs b/crates/engine/src/tests_mask_warp.rs index e08ce2e6..137feb1c 100644 --- a/crates/engine/src/tests_mask_warp.rs +++ b/crates/engine/src/tests_mask_warp.rs @@ -412,6 +412,162 @@ impl Deferred { } } +/// #1249: a setting changed while the analysis runs in a worker survives its result, which +/// used to replace the whole effect with the copy the job started from. +#[test] +fn an_edit_made_during_an_offloaded_analysis_survives_its_result() { + let (mut s, clip, solid) = setup(shaky); + s.execute("edit.clear", json!({"layers": [solid.0]})).unwrap(); + s.execute("layer.select", json!({"layers": [clip.0]})).unwrap(); + s.execute("effect.apply", json!({"effect": "Warp Stabilizer"})).unwrap(); + set_param(&mut s, clip, "stabilization/smoothness", json!(37.5)); + let off = std::sync::Arc::new(Deferred::default()); + s.offload = Some(off.clone()); + s.execute_checked("warp.analyze", json!({"layer": clip.0})).unwrap(); + // While the job runs: another setting of the same effect. + set_param(&mut s, clip, "stabilization/smoothness", json!(12.5)); + let fx = |s: &Session| s.active_comp().unwrap().layer(clip).unwrap().props.prop("effects/#1/stabilization/smoothness").unwrap().value.clone(); + off.run(s.footage.clone()); + assert!(s.poll_offload()); + assert!(!s.drain_events().iter().any(|e| matches!(e, crate::Event::Toast { error: true, .. })), "applied without a conflict"); + let st = s.execute("warp.status", json!({})).unwrap(); + assert_eq!(st["analyzed"], true, "the analysis landed: {st}"); + assert_eq!(fx(&s), KV::Scalar(12.5), "the edit made during the analysis stays"); + // One undo step takes the analysis back and keeps the edit. + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(fx(&s), KV::Scalar(12.5)); + assert_eq!(s.execute("warp.status", json!({})).unwrap()["analyzed"], false); +} + +fn point_track_pose(f: u32) -> ([f64; 2], f64, f64) { + ([150.0 + 2.0 * f as f64, 110.0 + f as f64], 0.0, 1.0) +} + +/// Hold an actual point-tracking worker request so commands can edit its target before the +/// serialized worker reply is delivered. +fn held_point_track() -> (Session, LayerId, Uid, std::sync::Arc) { + let (mut s, clip, solid) = setup(patch_frames(point_track_pose)); + s.execute("edit.clear", json!({"layers": [solid.0]})).unwrap(); + s.execute("layer.select", json!({"layers": [clip.0]})).unwrap(); + let tracker = s.execute("track.motion", json!({})).unwrap()["tracker"].as_u64().unwrap(); + s.execute("track.setPoint", json!({"point": 1, "center": point_track_pose(0).0, "featureSize": [36, 36], "searchSize": [80, 80]})).unwrap(); + let off = std::sync::Arc::new(Deferred::default()); + s.offload = Some(off.clone()); + s.execute_checked("track.analyze", json!({"direction": "forward"})).unwrap(); + assert_eq!(off.kinds(), [crate::offload::JobKind::Track]); + (s, clip, tracker, off) +} + +fn tracked_center(s: &Session, clip: LayerId, tracker: Uid) -> Uid { + s.active_comp().unwrap().layer(clip).unwrap().props.find_group(tracker).unwrap().track_points().next().unwrap().get("featureCenter").unwrap().uid +} + +#[test] +fn offloaded_conflict_preserves_state_and_reports_completion_failure() { + let (mut s, clip, tracker, off) = held_point_track(); + let center = tracked_center(&s, clip, tracker); + s.execute("prop.set", json!({"layer": clip.0, "prop": center, "value": [180.0, 140.0]})).unwrap(); + s.execute("prop.renameGroup", json!({"layer": clip.0, "prop": tracker, "name": "Redo survives"})).unwrap(); + assert!(s.undo()); + let before = s.project.clone(); + let state = serde_json::to_value(&s.state).unwrap(); + let steps = |s: &Session| { + let entries = + |v: &[(String, std::sync::Arc)]| v.iter().map(|(label, p)| (label.clone(), p.to_json())).collect::>(); + ( + entries(&s.history.undo), + entries(&s.history.redo), + s.history.branches.iter().map(|b| (b.parent.to_json(), entries(&b.steps))).collect::>(), + s.history.merge_key.clone(), + ) + }; + let history = steps(&s); + let inbox = s.offloaded(crate::offload::JobKind::Track).unwrap().inbox.clone(); + off.run(s.footage.clone()); + let (done, replies): (Vec<_>, Vec<_>) = inbox.take().into_iter().partition(|r| matches!(r, crate::offload::WorkerReply::Done)); + assert_eq!(done.len(), 1); + for reply in replies { + inbox.push(reply); + } + s.drain_events(); + assert!(s.poll_offload()); + let pending = s.offloaded(crate::offload::JobKind::Track).unwrap(); + assert!(pending.progress.error.as_ref().is_some_and(|e| e.contains("nothing was applied"))); + assert!(!pending.progress.finished); + assert!(s.offload_log.is_empty(), "the error is kept before completion is logged"); + // The current completion remains identifiable after the bounded log evicts an old entry. + s.offload_log = vec![(crate::offload::JobKind::Warp, None); 32]; + for reply in done { + inbox.push(reply); + } + assert!(s.poll_offload()); + assert!(std::sync::Arc::ptr_eq(&s.project, &before), "a rejected result changes no document content"); + assert_eq!(serde_json::to_value(&s.state).unwrap(), state); + assert_eq!(steps(&s), history); + let errors: Vec<_> = s + .drain_events() + .into_iter() + .filter_map(|e| match e { + crate::Event::Toast { error: true, message } => Some(message), + _ => None, + }) + .collect(); + assert_eq!(errors.len(), 1, "the apply rejection is reported once: {errors:?}"); + assert!(errors[0].contains("nothing was applied")); + assert_eq!(s.offload_log.len(), 32); + assert_eq!(s.offload_log_start, 1); + let (kind, error) = s.offload_log.last().unwrap(); + assert_eq!(*kind, crate::offload::JobKind::Track); + assert!(error.as_ref().is_some_and(|e| e.contains("nothing was applied")), "a rejected result must report failure to JobWaiter"); + assert!(s.redo()); + assert_eq!(s.active_comp().unwrap().layer(clip).unwrap().props.find_group(tracker).unwrap().name, "Redo survives"); +} + +#[test] +fn offloaded_disjoint_edit_reports_success_and_keeps_one_undo_step() { + let (mut s, clip, tracker, off) = held_point_track(); + s.execute("prop.renameGroup", json!({"layer": clip.0, "prop": tracker, "name": "Live track"})).unwrap(); + s.execute("prop.setGroupEnabled", json!({"layer": clip.0, "prop": tracker, "value": false})).unwrap(); + let undo = s.history.undo.len(); + off.run(s.footage.clone()); + s.drain_events(); + assert!(s.poll_offload()); + let layer = s.active_comp().unwrap().layer(clip).unwrap(); + let track = layer.props.find_group(tracker).unwrap(); + assert_eq!(track.name, "Live track"); + assert!(!track.enabled); + assert_eq!(track.track_points().next().unwrap().get("featureCenter").unwrap().keys.len(), 25); + assert_eq!(s.history.undo.len(), undo + 1); + assert_eq!(s.offload_log.last(), Some(&(crate::offload::JobKind::Track, None))); + assert!(!s.drain_events().iter().any(|e| matches!(e, crate::Event::Toast { error: true, .. }))); + let applied = s.project.to_json(); + assert!(s.undo()); + let track = s.active_comp().unwrap().layer(clip).unwrap().props.find_group(tracker).unwrap(); + assert_eq!(track.name, "Live track"); + assert!(!track.enabled); + assert!(track.track_points().next().unwrap().get("featureCenter").unwrap().keys.is_empty()); + assert!(s.redo()); + assert_eq!(s.project.to_json(), applied); +} + +#[test] +fn offloaded_conflict_does_not_fail_a_concurrent_analysis() { + let (mut s, clip, tracker, off) = held_point_track(); + let effect = s.execute("effect.apply", json!({"effect": "Warp Stabilizer", "layers": [clip.0]})).unwrap()["effects"][0].as_u64().unwrap(); + s.execute_checked("warp.analyze", json!({"layer": clip.0, "effect": effect})).unwrap(); + assert_eq!(off.kinds(), [crate::offload::JobKind::Track, crate::offload::JobKind::Warp]); + let center = tracked_center(&s, clip, tracker); + s.execute("prop.set", json!({"layer": clip.0, "prop": center, "value": [180.0, 140.0]})).unwrap(); + off.run(s.footage.clone()); + assert!(s.poll_offload()); + let track = s.offload_log.iter().find(|(kind, _)| *kind == crate::offload::JobKind::Track).unwrap(); + assert!(track.1.as_ref().is_some_and(|e| e.contains("nothing was applied"))); + let warp = s.offload_log.iter().find(|(kind, _)| *kind == crate::offload::JobKind::Warp).unwrap(); + assert!(warp.1.is_none(), "the other analysis completed successfully: {warp:?}"); + assert_eq!(s.execute("warp.status", json!({"layer": clip.0, "effect": effect})).unwrap()["analyzed"], true); + assert!(s.active_comp().unwrap().layer(clip).unwrap().props.find(center).unwrap().keys.is_empty()); +} + /// M13.32: where jobs run elsewhere (the browser), `warp.status` never solves the /// stabilization plan on the page's thread (seconds for a long Subspace Warp clip): the /// analysis job sends the plan's summary with its result, and a settings change has the plan diff --git a/crates/engine/src/tests_settings.rs b/crates/engine/src/tests_settings.rs index 458c886f..65e39b46 100644 --- a/crates/engine/src/tests_settings.rs +++ b/crates/engine/src/tests_settings.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use serde_json::json; -use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with, temp_path}; +use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with}; use crate::prefs::{PREFS_FILE, Prefs}; use crate::shortcuts::{DEFAULT_PRESET, Keymaps, normalize}; use crate::{Session, autosave}; @@ -548,6 +548,66 @@ fn frontend_commands_are_bindable_with_panel_scope() { // ---------------------------------------------------------------- auto-save / recovery +/// The names in `d`, sorted. +fn names(d: &std::path::Path) -> Vec { + let mut v: Vec = std::fs::read_dir(d).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + v.sort(); + v +} + +#[cfg(unix)] +#[test] +fn atomic_write_overwrites_a_long_valid_filename() { + let d = tmp("atomic-long-name"); + // The old `.tmp` fits a 255-byte component limit for this 251-byte project name. + // Including the full name plus a process id and counter made an otherwise valid save fail. + let name = format!("{}.ecproj", "x".repeat(244)); + assert_eq!(name.len(), 251); + let f = d.join(&name); + std::fs::write(&f, b"original project").unwrap(); + let replacement = b"replacement project\0bytes\xff"; + atomic_write(&f, replacement).unwrap(); + assert_eq!(std::fs::read(&f).unwrap(), replacement); + assert_eq!(names(&d), [name], "no temporary file is left after overwriting the project"); + let _ = std::fs::remove_dir_all(d); +} + +#[test] +fn concurrent_atomic_writes_each_publish_a_whole_file() { + // Writers of the same file used to share one `.tmp`: they truncated each other's data and + // the later rename failed once the first had moved it away (#1233). + let d = tmp("atomic-concurrent"); + let f = d.join("shared.ecproj"); + for round in 0..10 { + let contents: Vec> = (0..8u8).map(|w| vec![b'a' + w; 256 * 1024 + usize::from(w) * 1000]).collect(); + std::thread::scope(|sc| { + for c in &contents { + let f = &f; + sc.spawn(move || atomic_write(f, c).expect("every writer succeeds")); + } + }); + let got = std::fs::read(&f).unwrap(); + assert!(contents.contains(&got), "round {round}: the file is one writer's whole data ({} bytes)", got.len()); + assert_eq!(names(&d), ["shared.ecproj"], "round {round}: no temporary file is left"); + } + let _ = std::fs::remove_dir_all(d); +} + +#[cfg(unix)] +#[test] +fn atomic_write_does_not_follow_a_link_at_the_old_temp_name() { + let d = tmp("atomic-link"); + let f = d.join("p.ecproj"); + let victim = d.join("victim.txt"); + std::fs::write(&victim, b"keep").unwrap(); + // The temporary name used to be `.tmp`, opened with File::create (following links). + std::os::unix::fs::symlink(&victim, d.join("p.ecproj.tmp")).unwrap(); + atomic_write(&f, b"new project").unwrap(); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep", "the link's target is untouched"); + assert_eq!(std::fs::read(&f).unwrap(), b"new project"); + let _ = std::fs::remove_dir_all(d); +} + #[test] fn atomic_write_survives_a_crash_mid_write() { let d = tmp("atomic"); @@ -555,12 +615,12 @@ fn atomic_write_survives_a_crash_mid_write() { atomic_write(&f, b"version 1").unwrap(); // The writer dies halfway through the temporary file. let r = atomic_write_with(&f, b"version 2 is much longer", |tmp, data| { - std::fs::write(tmp, &data[..5])?; + std::io::Write::write_all(tmp, &data[..5])?; Err(std::io::Error::other("power cut")) }); assert!(r.is_err()); assert_eq!(std::fs::read(&f).unwrap(), b"version 1"); - assert!(!temp_path(&f).exists(), "the partial temp file is cleaned up"); + assert_eq!(names(&d), ["p.ecproj"], "the partial temp file is cleaned up"); // A temp file left by a hard kill doesn't count as an auto-save. std::fs::write(d.join("p auto-save 1.ecproj.tmp"), b"junk").unwrap(); assert!(autosave::existing(&d, "p").is_empty()); diff --git a/crates/pdf/README.md b/crates/pdf/README.md index 652dd2c6..57603cf9 100644 --- a/crates/pdf/README.md +++ b/crates/pdf/README.md @@ -136,10 +136,13 @@ ffmpeg elsewhere) to check the code tables against an independent implementation ## Shared parser setup and bounds The workspace pins `craft-pdf` to `craft-libs` Git revision -`89dcbb3060377a26c99a6d80caa16084366704c4`, with `default-features = false` and only the explicit `read` +`ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d`, with `default-features = false` and only the explicit `read` feature. This excludes the backend's bundled fonts and Adobe CMaps and preserves EffectCraft's font and asset policies. The shared parser does not change the supported drawing operators, image codecs, continuously rasterized SVG tree, editable shapes, or EPS interpreter. +This revision removes an unchecked lazy-array read and preserves later values and pages after +sign-only numeric entries; the app exercises those inputs through its object adapter, public +page parser and rasterizer on native and WASM targets. The root Cargo patch pins Hayro-syntax to the same `storytold/craft-libs` revision. `cargo xtask pdf-deps` checks this graph and refuses an enabled Hayro interpreter, rasterizer or CMap dependency. The shared revision must be diff --git a/crates/pdf/src/ps.rs b/crates/pdf/src/ps.rs index 65e352cb..4401ace9 100644 --- a/crates/pdf/src/ps.rs +++ b/crates/pdf/src/ps.rs @@ -700,20 +700,10 @@ impl Interp { a / b } } - "idiv" => { - if b as i64 == 0 { - 0.0 - } else { - ((a as i64) / (b as i64)) as f64 - } - } - "mod" => { - if b as i64 == 0 { - 0.0 - } else { - ((a as i64) % (b as i64)) as f64 - } - } + // `checked_*` give 0 both for a zero divisor and for the one overflowing + // case, `i64::MIN / -1` (`/` and `%` panic on it in every build). + "idiv" => (a as i64).checked_div(b as i64).unwrap_or(0) as f64, + "mod" => (a as i64).checked_rem(b as i64).unwrap_or(0) as f64, "atan" => a.atan2(b).to_degrees().rem_euclid(360.0), "exp" => a.powf(b), "max" => a.max(b), diff --git a/crates/pdf/src/read.rs b/crates/pdf/src/read.rs index 44992538..953582fc 100644 --- a/crates/pdf/src/read.rs +++ b/crates/pdf/src/read.rs @@ -303,6 +303,39 @@ impl Reader<'_> { #[cfg(test)] mod limit_tests { + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)] + #[cfg_attr(not(target_arch = "wasm32"), test)] + fn shared_reader_preserves_tolerant_array_numbers_and_later_pages() { + use crate::object::Obj; + + // Array validation accepts these sign/dot-only zero tokens. Previously '+' + // reached an unchecked lazy-iterator read: native loading returned an internal + // parser error and WASM could abort before reaching the following object. + for token in ["+", "-", ".", "+.", "-."] { + for numeric_kid in [false, true] { + let kids = if numeric_kid { format!("{token} 3 0 R") } else { "3 0 R".into() }; + let bytes = crate::write::pdf( + &[ + (1, format!("<< /Type /Catalog /Pages 2 0 R /Probe [{token} 3] >>"), None), + (2, format!("<< /Type /Pages /Kids [{kids}] /Count 1 >>"), None), + (3, "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 40 20] /Resources << >> /Contents 4 0 R >>".into(), None), + (4, "<< >>".into(), Some(b"1 0 0 rg 0 0 40 20 re f\n".to_vec())), + ], + 1, + ); + let file = super::open(&bytes).unwrap(); + assert_eq!(file.catalog().unwrap().get("Probe").and_then(Obj::array), Some([Obj::Num(0.0), Obj::Num(3.0)].as_slice())); + assert_eq!(crate::page_count(&bytes), 1, "token {token}, numeric kid {numeric_kid}"); + let doc = crate::parse(&bytes).unwrap(); + assert_eq!((doc.width, doc.height), (40.0, 20.0)); + assert_eq!(doc.flatten().len(), 1); + assert!(doc.skipped.is_empty(), "{:?}", doc.skipped); + assert_eq!(effectcraft_svg::rasterize(&doc, 40, 20, 1.0).get(20, 10), [1.0, 0.0, 0.0, 1.0]); + assert_eq!(crate::parse_page(&bytes, 1), Err(crate::Error::PageOutOfRange(1))); + } + } + } + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)] #[cfg_attr(not(target_arch = "wasm32"), test)] fn shared_reader_preserves_the_target_stream_budget() { diff --git a/crates/pdf/src/tests_import.rs b/crates/pdf/src/tests_import.rs index 475121ef..e53727ef 100644 --- a/crates/pdf/src/tests_import.rs +++ b/crates/pdf/src/tests_import.rs @@ -382,3 +382,25 @@ fn extreme_page_indices_have_a_printable_error() { let text = Error::PageOutOfRange(usize::MAX).to_string(); assert!(text.starts_with("the PDF has no page ")); } + +#[test] +fn postscript_idiv_and_mod_of_i64_min_by_minus_one_give_zero() { + // `i64::MIN / -1` and `i64::MIN % -1` overflow, and `/` and `%` panic on them in debug and + // release builds. `as i64` turns any dividend at or below -2^63 into `i64::MIN` and turns + // a divisor of -1.5 into -1. + for (prog, want) in [ + ("-9223372036854775808 -1 idiv", 0), + ("-9223372036854775808 -1 mod", 0), + ("-99999999999999999999 -1.5 idiv", 0), + ("2 63 exp neg -1 mod", 0), + // The result is 0 for a divisor that truncates to 0. Other quotients are truncated + // toward zero, and a remainder has the sign of the dividend. + ("7 0 idiv", 0), + ("7 0.5 mod", 0), + ("-7 2 idiv", -3), + ("-7 2 mod", -1), + ] { + let doc = eps(format!("{prog} {want} eq {{ 0 0 10 10 rectfill }} if\n").as_bytes()); + assert_eq!(shape_names(&doc).len(), 1, "{prog}"); + } +} diff --git a/crates/pdf/src/tests_reader.rs b/crates/pdf/src/tests_reader.rs index 57b10d39..999a9900 100644 --- a/crates/pdf/src/tests_reader.rs +++ b/crates/pdf/src/tests_reader.rs @@ -48,7 +48,11 @@ fn a_long_cyclic_reference_graph_does_not_expand_recursively() { #[test] fn excessive_inline_nesting_returns_an_error_instead_of_partial_art() { let extra = format!("/Extra {}0{}", "[".repeat(80), "]".repeat(80)); - assert!(matches!(parse(&document(&extra, &[])), Err(Error::Read(message)) if message.contains("64 nested objects"))); + let bytes = document(&extra, &[]); + // The shared parser rejects the catalog before the adapter's depth limit runs. + // Its Invalid result follows the existing NoPages mapping, with no partial document. + assert_eq!(crate::page_count(&bytes), 0); + assert_eq!(parse(&bytes), Err(Error::NoPages)); } fn object_stream(entries: &[(u32, &str)]) -> String { diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index bb45d110..15493ad7 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -68,8 +68,8 @@ pub struct JobWaiter { pub result: Value, /// Ids of the jobs it started (`jobs.list` ids: `render`, `track`, `roto`…). pub jobs: Vec, - /// Length of [`effectcraft_engine::Session::offload_log`] when it started. - pub log_len: usize, + /// Absolute completion-log position when it started (including evicted entries). + pub log_cursor: usize, } /// Commands that block with `wait` (and its default). @@ -86,7 +86,7 @@ fn execute_engine(app: &mut EffectcraftApp, id: &str, params: Value) -> Result = app.session.jobs().into_iter().map(|j| j.id).collect(); - let log_len = app.session.offload_log.len(); + let log_cursor = app.session.offload_log_start.saturating_add(app.session.offload_log.len()); let mut p = params.clone(); p["wait"] = json!(false); let result = app.session.execute_checked(id, p).map_err(|e| e.to_string())?; @@ -94,7 +94,7 @@ fn execute_engine(app: &mut EffectcraftApp, id: &str, params: Value) -> Result Value { let s = &mut app.session; s.poll_render(); - let errors: Vec = s.offload_log.iter().skip(self.log_len).filter_map(|(_, e)| e.clone()).collect(); - if let Some(e) = errors.first() { + let error = s + .offload_log + .iter() + .skip(self.log_cursor.saturating_sub(s.offload_log_start)) + .filter(|(kind, _)| self.jobs.iter().any(|id| id == kind.job_id())) + .find_map(|(_, e)| e.as_ref()); + if let Some(e) = error { return json!({"ok": false, "error": e}); } let mut r = self.result; diff --git a/crates/ui-egui/tests/ui_web_offload.rs b/crates/ui-egui/tests/ui_web_offload.rs index cc49325f..67ec6a9f 100644 --- a/crates/ui-egui/tests/ui_web_offload.rs +++ b/crates/ui-egui/tests/ui_web_offload.rs @@ -219,3 +219,45 @@ fn waiting_commands_reply_when_their_offloaded_job_ends() { let v = reply.try_recv().expect("immediate reply"); assert_eq!(v["result"]["rendering"], true, "{v}"); } + +/// Completion errors belong to the waiting command's jobs, including when several +/// different analyses complete in the same UI frame. +#[test] +fn waiting_commands_report_only_their_own_completion_errors() { + use effectcraft_engine::offload::JobKind; + use effectcraft_ui_egui::control::JobWaiter; + + for (kind, job_id) in [ + (JobKind::Render, "render"), + (JobKind::Warp, "warp"), + (JobKind::WarpPlan, "warpPlan"), + (JobKind::Camera, "camera"), + (JobKind::RotoFreeze, "roto"), + (JobKind::RotoPropagate, "roto"), + (JobKind::Track, "track"), + (JobKind::MaskTrack, "maskTrack"), + (JobKind::ContentFill, "contentFill"), + ] { + let other = if kind == JobKind::Track { JobKind::Warp } else { JobKind::Track }; + let mut app = EffectcraftApp::new(Session::default()); + app.session.offload_log = vec![(kind, Some("old error".into())), (other, Some("unrelated failure".into())), (kind, None)]; + let waiter = || JobWaiter { command: "analysis".into(), params: json!({}), result: json!({"running": true}), jobs: vec![job_id.into()], log_cursor: 1 }; + let result = waiter().finish(&mut app); + assert_eq!(result, json!({"ok": true, "result": {"running": false}}), "{kind:?}"); + app.session.offload_log.push((kind, Some("nothing was applied".into()))); + let result = waiter().finish(&mut app); + assert_eq!(result, json!({"ok": false, "error": "nothing was applied"}), "{kind:?}"); + } +} + +#[test] +fn waiting_command_sees_its_error_after_the_completion_log_rolls_over() { + use effectcraft_engine::offload::JobKind; + use effectcraft_ui_egui::control::JobWaiter; + let mut app = EffectcraftApp::new(Session::default()); + app.session.offload_log = vec![(JobKind::Warp, None); 31]; + app.session.offload_log.push((JobKind::Track, Some("nothing was applied".into()))); + app.session.offload_log_start = 1; + let waiter = JobWaiter { command: "track.analyze".into(), params: json!({}), result: json!({}), jobs: vec!["track".into()], log_cursor: 32 }; + assert_eq!(waiter.finish(&mut app), json!({"ok": false, "error": "nothing was applied"})); +} diff --git a/docs/agents.md b/docs/agents.md index 1bec71d5..ed0073d4 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -52,6 +52,11 @@ Desktop, Cursor and the like) take the same `command` and `args`. - **Bridge** (`["mcp", "--bridge", "9877"]`): drives a running `effectcraft --control 9877`, so you see every change live. Bridge mode adds `screenshot` and the `ui_*` tools. +After a broken connection, the bridge retries explicitly read-only control queries once. +Engine commands, UI edits and unknown methods whose transmission or reply fails report an +unknown outcome: the app may already have applied them. Inspect the app before explicitly +retrying. A later call reconnects; timeouts and malformed replies are never automatically resent. + ### From an installed release The release packages ship `effectcraft-cli` alongside the desktop app, so no build is needed: diff --git a/xtask/src/pdf_deps.rs b/xtask/src/pdf_deps.rs index dc5a64ef..d7f88079 100644 --- a/xtask/src/pdf_deps.rs +++ b/xtask/src/pdf_deps.rs @@ -5,7 +5,7 @@ use std::process::Command; use serde_json::Value; -const REV: &str = "89dcbb3060377a26c99a6d80caa16084366704c4"; +const REV: &str = "ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d"; const PACKAGES: &[(&str, &str)] = &[("craft-pdf", "0.1.0"), ("hayro-syntax", "0.7.2")]; pub fn run(root: &Path) -> Result<(), String> {