From 615c4cdffbb0ae45837c8073a65c50940d142050 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Sun, 18 Oct 2026 01:37:27 +0200 Subject: [PATCH 1/7] Render Queue: two outputs of one job never write the same file, and default names avoid the queue's other outputs Two queue items that resolve to one output (the same comp added twice, a duplicated item) replaced each other's file inside one job. Nothing is on disk when the job starts, so no check said anything. - A default output name (renderQueue.add, render.preRender, comp.saveFrameAs {queue}, file.createProxy) that another item of the queue writes, in any output module or segment, is numbered like a footage collision: Main.mp4, Main_1.mp4, Main_2.mp4. Sequences are compared by the frames they write, so two stills of one comp at different times keep their names. Names a caller gave and names already stored in items are never changed. - Outputs of one job that still name the same file (explicit names, renderQueue.duplicate, output modules, segments) refuse the whole render before anything is written, naming both items. Sequences clash only on frames both write; a later item with Skip Existing Files leaves the earlier item's frames alone; items refused for footage take no part. Refusing the whole render, not just the later item: which of two items should keep the file is the user's to say, and failing only the later one would leave a half-rendered queue found after they left. renderQueue.duplicate stays a verbatim copy so the clash is reported, not hidden. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01RDvxCS8xcR28VArjzn1x7U --- apps/effectcraft-cli/tests/it/cli.rs | 32 ++ crates/engine/src/output_guard.rs | 407 +++++++++++++++++++- crates/engine/src/render_queue.rs | 5 + crates/engine/src/tests/render_overwrite.rs | 369 +++++++++++++++++- docs/agents.md | 8 + docs/mcp.md | 9 + 6 files changed, 812 insertions(+), 18 deletions(-) diff --git a/apps/effectcraft-cli/tests/it/cli.rs b/apps/effectcraft-cli/tests/it/cli.rs index 7a770fa87..5f78ec2c1 100644 --- a/apps/effectcraft-cli/tests/it/cli.rs +++ b/apps/effectcraft-cli/tests/it/cli.rs @@ -419,6 +419,38 @@ fn render_out_is_relative_to_the_working_directory() { let _ = std::fs::remove_dir_all(&root); } +/// Two queue items on one file are refused by the render before anything is written, with the +/// file as it was. +#[test] +fn render_refuses_two_items_on_one_file() { + let root = tmp("render-clash"); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let out = root.join("clash.mp4"); + std::fs::write(&out, b"EARLIER").unwrap(); + let project = root.join("p.ecproj"); + let p = project.to_str().unwrap(); + let add = json!({"output": out.to_str().unwrap()}).to_string(); + ok_json(&[ + "run", + "comp.new", + r#"{"name":"T","width":64,"height":36,"duration":1}"#, + "renderQueue.add", + &add, + "renderQueue.add", + &add, + "--empty", + "--save-as", + p, + ]); + let (code, v) = run_json(&["render", "--queue", "--project", p]); + assert_eq!(code, 1, "{v}"); + let error = v["error"].as_str().unwrap(); + assert!(error.contains("#1") && error.contains("#2") && error.contains("clash.mp4"), "{v}"); + assert_eq!(std::fs::read(&out).unwrap(), b"EARLIER"); + let _ = std::fs::remove_dir_all(&root); +} + /// A real stdio client: each edit's reply is read before disconnecting or killing the server. struct McpChild { child: std::process::Child, diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index 5d4dc289e..ba6947694 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -5,12 +5,22 @@ //! exporter. Existing aliases are compared by native file identity, including hard links. //! Missing sources remain protected by their normalized path. Checks happen before dispatch; //! filesystem changes between the check and the write require exporter-side revalidation. +//! +//! Nor does a render write one file twice. When two outputs of one job name the same file (two +//! items with one Output To, a duplicated item, output modules or segments that meet), the one +//! rendered later replaces what the earlier one just wrote, and nothing is on disk to say so when +//! the job starts. [`Session::duplicate_outputs`] lists them and the whole render is refused before +//! anything is written: no answer makes a render replacing its own output right. A numbered sequence is compared by the frames +//! it writes: two items of one pattern clash only where their frames do, and an item that skips +//! existing frames leaves the earlier one's alone. A new item's default output name steps aside +//! the same way it does for footage ([`Session::avoid_sources`]). +//! use std::collections::{HashMap, HashSet}; use std::path::{Component, Path, PathBuf}; use std::sync::Arc; -use effectcraft_project::render_queue::{RenderQueueItem, log_path, numbered_template, sequence_frame, sequence_path}; +use effectcraft_project::render_queue::{RenderQueueItem, log_path, numbered_template, sequence_frame, sequence_parts, sequence_path}; use effectcraft_project::{Footage, ItemKind}; use crate::Session; @@ -26,6 +36,10 @@ const PROBE_LIMIT: u64 = 100_000; /// render time. const MAX_NUMBER: u32 = 1_000; +/// Stands for the frame number in the name a numbered sequence is compared by: a character no +/// file is named with. +const FRAME: char = '\u{e000}'; + /// What reads a file the project depends on. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum Source { @@ -148,6 +162,24 @@ fn identity_hash(handle: &same_file::Handle) -> u64 { hash.finish() } +/// What an existing regular file is known by on its volume (links and hard links to one file give one), +/// `None` when it does not exist, is not a regular file, or the volume has no such identity. +fn identity(path: &Path) -> Option { + #[cfg(not(target_arch = "wasm32"))] + { + // Opening a FIFO can wait for a writer: only regular files are looked at. + if !std::fs::metadata(path).ok()?.is_file() { + return None; + } + same_file::Handle::from_path(path).ok().map(|handle| identity_hash(&handle).to_string()) + } + #[cfg(target_arch = "wasm32")] + { + let _ = path; + None + } +} + /// `path` made absolute, with `.` and `..` resolved by name alone (no file is looked at). fn lexical(path: &Path) -> PathBuf { let absolute = std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()); @@ -228,6 +260,105 @@ fn frames_on_disk(path: &str, first: i64, count: u64) -> Result, St .collect()) } +/// What an output writes, as far as a clash with another output goes. +#[derive(Clone, Debug, PartialEq)] +enum Target { + /// One file: known by its name and, when it exists, by what it is on its volume. + File { key: String, identity: Option }, + /// The numbered files `first..end` of a sequence: `key` is its pattern, with the width the + /// frame numbers are padded to. + Frames { key: String, first: i64, end: i64 }, +} + +impl Target { + /// The file at `path`. `by_identity` also looks up what it is on its volume (a link to it + /// is it); that asks the file system, so it is left out where only names matter. + fn file(path: &Path, by_identity: bool) -> Target { + let identity = if by_identity { identity(path) } else { None }; + Target::File { key: key(path), identity } + } + + /// The `count` files of the sequence `path` (its frame-number pattern) from frame `first`. + fn frames(path: &str, first: i64, count: u64) -> Target { + let (before, width, after) = sequence_parts(path); + let key = format!("{width}:{}", key(Path::new(&format!("{before}{FRAME}{after}")))); + Target::Frames { key, first, end: first.saturating_add(i64::try_from(count).unwrap_or(i64::MAX)) } + } +} + +/// One output of a job. +#[derive(Clone, Debug)] +struct Write { + /// The queue item (`0`: one that is not in the queue yet). + item: u64, + /// The output path (a sequence's has its frame-number pattern). + path: String, + target: Target, + /// Skip Existing Files: a frame that is there already is left alone. + skips: bool, +} + +/// What an output rendered later writes over. +struct Clash<'a> { + /// The earlier output that writes it. + earlier: &'a Write, + /// How many files, and the first of them (its frame number, for a sequence). + files: u64, + first: i64, +} + +/// The outputs of a job in the order they render, to find the one that writes what an earlier +/// one does. +#[derive(Default)] +struct Writes { + all: Vec, + /// The first output to write a file, by name. + files: HashMap, + /// ... and by what the file is on its volume. + identities: HashMap, + /// Every output that writes the frames of a sequence pattern. + frames: HashMap>, +} + +impl Writes { + fn is_empty(&self) -> bool { + self.all.is_empty() + } + + /// The earliest output `later`, rendered after all of these, writes over. + fn clash(&self, later: &Write) -> Option> { + match &later.target { + Target::File { key, identity } => { + let at = self.files.get(key).or_else(|| identity.as_ref().and_then(|id| self.identities.get(id)))?; + Some(Clash { earlier: self.all.get(*at)?, files: 1, first: 0 }) + } + // A frame that is there already is left alone, so a later output that skips existing + // frames replaces none of an earlier one's. + Target::Frames { .. } if later.skips => None, + Target::Frames { key, first, end } => self.frames.get(key)?.iter().filter_map(|at| self.all.get(*at)).find_map(|earlier| { + let Target::Frames { first: from, end: to, .. } = &earlier.target else { return None }; + let start = (*first).max(*from); + let files = u64::try_from((*end).min(*to).saturating_sub(start)).ok().filter(|n| *n > 0)?; + Some(Clash { earlier, files, first: start }) + }), + } + } + + fn push(&mut self, write: Write) { + let at = self.all.len(); + match &write.target { + Target::File { key, identity } => { + self.files.entry(key.clone()).or_insert(at); + if let Some(identity) = identity { + self.identities.entry(identity.clone()).or_insert(at); + } + } + Target::Frames { key, .. } => self.frames.entry(key.clone()).or_default().push(at), + } + self.all.push(write); + } +} + /// A queue item refused before any of its output modules are dispatched. #[derive(Clone, Debug, PartialEq)] pub(crate) struct Refused { @@ -235,6 +366,50 @@ pub(crate) struct Refused { pub reason: String, } +/// An output that would write a file an earlier output of the same job writes. +#[derive(Clone, Debug, PartialEq)] +pub(crate) struct DuplicateOutput { + /// The queue item that would write over it (the later one), its place in the queue (1-based) + /// and its composition. + pub item: u64, + pub index: usize, + pub comp: String, + /// The file: a sequence's pattern, or the frame when it is one. + pub path: String, + /// How many files the two outputs have in common. + pub files: u64, + pub sequence: bool, + /// The queue item whose output is written first, and its place and composition. + pub with: u64, + pub with_index: usize, + pub with_comp: String, +} + +impl DuplicateOutput { + /// Why the render is refused, naming the two items. + pub(crate) fn reason(&self) -> String { + let label = |index: usize, comp: &str| format!("#{index} “{comp}”"); + let what = if self.sequence && self.files != 1 { format!("{} frames of {}", self.files, self.path) } else { self.path.clone() }; + let rule = "and a render never writes one file twice; choose another Output To for one of them"; + if self.item == self.with { + format!("{} would write {what} twice (two of its output modules), {rule}", label(self.index, &self.comp)) + } else { + format!("{} and {} would both write {what}, {rule}", label(self.with_index, &self.with_comp), label(self.index, &self.comp)) + } + } +} + +/// Why a render is refused when outputs of the job write one file twice: it would replace its own output. +pub(crate) fn duplicate_refusal(duplicates: &[DuplicateOutput]) -> Option { + let first = duplicates.first()?; + let more = match duplicates.len().saturating_sub(1) { + 0 => String::new(), + 1 => "; 1 more output would write a file twice too".into(), + n => format!("; {n} more outputs would write a file twice too"), + }; + Some(format!("{}. Nothing was rendered{more}", first.reason())) +} + impl Session { /// The files the project reads. pub(crate) fn protected_files(&self) -> Protected { @@ -266,6 +441,51 @@ impl Session { refused } + /// What the job writes more than once. An item in `refused` writes nothing, so it takes no part. + pub(crate) fn duplicate_outputs(&self, items: &[Vec<(RenderQueueItem, String)>], refused: &[Refused]) -> Vec { + if cfg!(target_arch = "wasm32") { + return Vec::new(); + } + let skip: HashSet = refused.iter().map(|r| r.item).collect(); + let mut writes = Writes::default(); + let mut duplicates = Vec::new(); + for (m, path) in items.iter().flatten().filter(|(m, _)| !skip.contains(&m.id)) { + let write = self.write_of(m, path, true); + if let Some(clash) = writes.clash(&write) { + duplicates.push(self.duplicate_of(&write, &clash)); + } + writes.push(write); + } + duplicates + } + + /// What `path`, an output of `m`, writes. `by_identity`: see [`Target::file`]. + fn write_of(&self, m: &RenderQueueItem, path: &str, by_identity: bool) -> Write { + let target = if m.output.format.is_sequence() { + let (first, count) = self.project.comp(m.comp).map_or((0, 0), |c| (m.settings.first_frame(c), m.settings.frame_count(c))); + Target::frames(path, first, count) + } else { + Target::file(Path::new(path), by_identity) + }; + Write { item: m.id, path: path.to_string(), target, skips: m.settings.skip_existing } + } + + /// The report of `later` writing over `clash`. + fn duplicate_of(&self, later: &Write, clash: &Clash) -> DuplicateOutput { + // Where a queue item stands, and what it renders. + let place = |id: u64| { + let index = self.project.render_queue.iter().position(|i| i.id == id); + let comp = index.and_then(|k| self.project.render_queue.get(k)).and_then(|i| self.project.item(i.comp)).map(|i| i.name.clone()); + (index.map_or(0, |k| k + 1), comp.unwrap_or_default()) + }; + let sequence = matches!(later.target, Target::Frames { .. }); + // One shared frame is named by its file, more by the pattern. + let path = if sequence && clash.files == 1 { sequence_path(&later.path, clash.first) } else { later.path.clone() }; + let (index, comp) = place(later.item); + let (with_index, with_comp) = place(clash.earlier.item); + DuplicateOutput { item: later.item, index, comp, path, files: clash.files, sequence, with: clash.earlier.item, with_index, with_comp } + } + fn check_output(&self, protected: &Protected, item: &RenderQueueItem, path: &str) -> Result, String> { if protected.is_empty() { return Ok(None); @@ -290,43 +510,54 @@ impl Session { .find_map(|path| protected.find(path).map(|source| source.refusal(&path.to_string_lossy())))) } - /// A new item's default output must not be a file the project reads: `[compName]_1`, - /// `[compName]_2`… take its place (and avoid the queue's other outputs), as After Effects - /// numbers a second item of the same composition. An output nothing is in the way of keeps - /// the composition's own name, and one the caller chose is not touched: rendering it is - /// refused instead. + /// A new item's default output must not be a file the project reads, nor one another item of + /// the queue writes (any of its output modules, whether or not it is ticked): `[compName]_1`, + /// `[compName]_2`… take its place, as After Effects numbers a second item of the same + /// composition. An output nothing is in the way of keeps the composition's own name; the name + /// is stored in the item, so it stays as it is when the item it avoided goes away. One the + /// caller chose is not touched: rendering it is refused instead ([`Session::duplicate_outputs`]). pub(crate) fn avoid_sources(&self, item: &mut RenderQueueItem) { if cfg!(target_arch = "wasm32") { return; } let protected = self.protected_files(); - if protected.is_empty() { + let queue = self.queue_writes(); + if protected.is_empty() && queue.is_empty() { return; } let template = item.output.output.clone(); - let mut taken: Option> = None; - 'candidate: for n in 0..=MAX_NUMBER { + for n in 0..=MAX_NUMBER { item.output.output = if n == 0 { template.clone() } else { numbered_template(&template, n) }; let Ok(outputs) = self.outputs_of(item, None) else { break }; + let mut in_the_way = false; for (module, path) in &outputs { match self.check_output(&protected, module, path) { - Ok(Some(_)) => continue 'candidate, - Err(_) => break 'candidate, + Ok(Some(_)) => in_the_way = true, + Err(_) => { + item.output.output = template; + return; + } Ok(None) => {} } + in_the_way |= queue.clash(&self.write_of(module, path, false)).is_some(); } - // Only a name that had to move is kept clear of the other items. - let others = if n == 0 { None } else { Some(taken.get_or_insert_with(|| self.queue_output_keys())) }; - if others.is_none_or(|others| !outputs.iter().any(|(_, path)| others.contains(&key(Path::new(path))))) { + if !in_the_way { return; } } item.output.output = template; } - /// [`key`] of every output of the queue's items. - fn queue_output_keys(&self) -> HashSet { - self.project.render_queue.iter().filter_map(|it| self.outputs_of(it, None).ok()).flatten().map(|(_, path)| key(Path::new(&path))).collect() + /// What the items of the queue write (by name: a new item is not compared with what is on + /// disk). + fn queue_writes(&self) -> Writes { + let mut writes = Writes::default(); + for outputs in self.project.render_queue.iter().filter_map(|it| self.outputs_of(it, None).ok()) { + for (m, path) in &outputs { + writes.push(self.write_of(m, path, false)); + } + } + writes } } @@ -334,6 +565,148 @@ impl Session { mod tests { use super::*; + /// An output of queue item `item` that is a file, known by its name alone. + fn movie(item: u64, path: &str) -> Write { + Write { item, path: path.into(), target: Target::file(Path::new(path), false), skips: false } + } + + /// An output of queue item `item` that is `count` frames of a sequence from frame `first`. + fn frames(item: u64, pattern: &str, first: i64, count: u64) -> Write { + Write { item, path: pattern.into(), target: Target::frames(pattern, first, count), skips: false } + } + + fn job_of(writes: impl IntoIterator) -> Writes { + let mut job = Writes::default(); + for write in writes { + job.push(write); + } + job + } + + /// The item an output replaces a file of, how many files, and the first. + fn clash(job: &Writes, later: &Write) -> Option<(u64, u64, i64)> { + job.clash(later).map(|c| (c.earlier.item, c.files, c.first)) + } + + #[test] + fn a_file_is_replaced_by_the_output_that_has_its_name() { + let job = job_of([movie(1, "out/a.mp4"), movie(2, "out/b.mp4"), movie(3, "out/b.mp4")]); + assert_eq!(clash(&job, &movie(9, "out/a.mp4")), Some((1, 1, 0))); + assert_eq!(clash(&job, &movie(9, "out/b.mp4")), Some((2, 1, 0)), "the first to write it is named"); + assert_eq!(clash(&job, &movie(9, "out/x/../a.mp4")), Some((1, 1, 0)), "dot segments are resolved"); + assert_eq!(clash(&job, &movie(9, "out/c.mp4")), None); + assert_eq!(clash(&job, &movie(9, "other/a.mp4")), None); + assert_eq!(clash(&job, &frames(9, "out/a.mp4", 0, 5)), None, "a sequence is not a movie"); + assert!(Writes::default().is_empty() && !job.is_empty()); + } + + #[test] + fn two_names_of_one_file_on_disk_are_one_file() { + let on_disk = |item: u64, name: &str, id: Option<&str>| Write { + item, + path: name.into(), + target: Target::File { key: name.into(), identity: id.map(String::from) }, + skips: false, + }; + let job = job_of([on_disk(1, "/a/link.mp4", Some("7:1")), on_disk(2, "/a/nothing-yet.mp4", None)]); + assert_eq!(clash(&job, &on_disk(9, "/b/real.mp4", Some("7:1"))), Some((1, 1, 0))); + assert_eq!(clash(&job, &on_disk(9, "/b/real.mp4", Some("7:2"))), None); + assert_eq!(clash(&job, &on_disk(9, "/b/real.mp4", None)), None, "a file that is not there has no identity"); + assert_eq!(clash(&job, &on_disk(9, "/a/nothing-yet.mp4", None)), Some((2, 1, 0)), "its name is all there is"); + } + + #[test] + fn sequences_clash_where_their_frames_do() { + let job = job_of([frames(1, "a_[#####].png", 0, 5)]); // frames 0-4 + assert_eq!(clash(&job, &frames(2, "a_[#####].png", 5, 5)), None, "the five after it"); + assert_eq!(clash(&job, &frames(2, "a_[#####].png", -3, 3)), None, "the three before it"); + assert_eq!(clash(&job, &frames(2, "a_[#####].png", 4, 5)), Some((1, 1, 4)), "frame 4 only"); + assert_eq!(clash(&job, &frames(2, "a_[#####].png", 2, 1)), Some((1, 1, 2))); + assert_eq!(clash(&job, &frames(2, "a_[#####].png", 1, 2)), Some((1, 2, 1))); + assert_eq!(clash(&job, &frames(2, "a_[#####].png", -3, 100)), Some((1, 5, 0)), "all five"); + // The first of several that overlap is the one named. + let job = job_of([frames(1, "a_[#####].png", 0, 3), frames(2, "a_[#####].png", 3, 3)]); + assert_eq!(clash(&job, &frames(9, "a_[#####].png", 2, 3)), Some((1, 1, 2))); + assert_eq!(clash(&job, &frames(9, "a_[#####].png", 3, 3)), Some((2, 3, 3))); + } + + #[test] + fn sequences_of_other_names_do_not_clash() { + let job = job_of([frames(1, "out/a_[#####].png", 0, 5)]); + for other in ["out/b_[#####].png", "out/a_[####].png", "out/a_[#####].jpg", "elsewhere/a_[#####].png", "out/a[#####].png", "out/a_[#####]/x.png"] { + assert_eq!(clash(&job, &frames(2, other, 0, 5)), None, "{other}"); + } + // One name, however it is spelled: the run with or without brackets, none at all, dot segments. + for same in ["out/a_[#####].png", "out/a_#####.png", "out/a.png", "out/b/../a_[#####].png"] { + assert_eq!(clash(&job, &frames(2, same, 0, 5)), Some((1, 5, 0)), "{same}"); + } + // A frame number that starts a folder's name is not a file name's. + let job = job_of([frames(1, "out/[##].png", 0, 5)]); + assert_eq!(clash(&job, &frames(2, "out[##].png", 0, 5)), None); + assert_eq!(clash(&job, &frames(2, "out/[##].png", 0, 5)), Some((1, 5, 0))); + } + + /// Skip Existing Files leaves a frame that is there alone: it matters for the output that + /// comes second, and for sequences only. + #[test] + fn an_output_that_skips_existing_frames_replaces_none() { + let skipping = |mut write: Write| { + write.skips = true; + write + }; + let job_of_one = job_of([frames(1, "a_[#####].png", 0, 5)]); + assert_eq!(clash(&job_of_one, &skipping(frames(2, "a_[#####].png", 0, 5))), None); + let job_of_one = job_of([skipping(frames(1, "a_[#####].png", 0, 5))]); + assert_eq!(clash(&job_of_one, &frames(2, "a_[#####].png", 3, 5)), Some((1, 2, 3)), "the earlier one's frames are replaced"); + let job_of_one = job_of([movie(1, "a.mp4")]); + assert_eq!(clash(&job_of_one, &skipping(movie(2, "a.mp4"))), Some((1, 1, 0)), "a movie is written whole"); + } + + #[test] + fn frame_numbers_beyond_what_a_project_holds_do_not_panic() { + let top = i64::MAX - 1; + let job = job_of([frames(1, "a_[#####].png", top, u64::MAX), frames(2, "a_[#####].png", i64::MIN, u64::MAX)]); + assert_eq!(clash(&job, &frames(9, "a_[#####].png", top, u64::MAX)), Some((1, 1, top))); + assert_eq!(clash(&job, &frames(9, "a_[#####].png", -5, 3)), Some((2, 3, -5))); + assert_eq!(clash(&job, &frames(9, "a_[#####].png", 0, 5)), None, "between the two"); + assert_eq!(clash(&job, &frames(9, "a_[#####].png", i64::MAX, 0)), None, "nothing written"); + assert_eq!(clash(&job, &frames(9, "a_[#####].png", i64::MIN, 0)), None); + } + + #[test] + fn the_reason_names_both_items_and_what_they_write() { + let both = DuplicateOutput { + item: 7, + index: 3, + comp: "Main".into(), + path: "/o/Main.mp4".into(), + files: 1, + sequence: false, + with: 4, + with_index: 1, + with_comp: "Intro".into(), + }; + assert_eq!( + both.reason(), + "#1 “Intro” and #3 “Main” would both write /o/Main.mp4, and a render never writes one file twice; choose another Output To for one of them" + ); + let sequence = DuplicateOutput { sequence: true, files: 12, path: "/o/Main_[#####].png".into(), ..both.clone() }; + assert!(sequence.reason().contains("would both write 12 frames of /o/Main_[#####].png,"), "{}", sequence.reason()); + let one = DuplicateOutput { files: 1, path: "/o/Main_00004.png".into(), ..sequence }; + assert!(one.reason().contains("would both write /o/Main_00004.png,"), "{}", one.reason()); + let itself = DuplicateOutput { with: 7, with_index: 3, with_comp: "Main".into(), ..both.clone() }; + assert!(itself.reason().starts_with("#3 “Main” would write /o/Main.mp4 twice (two of its output modules), and a render never"), "{}", itself.reason()); + + let mut found = vec![]; + assert_eq!(duplicate_refusal(&found), None); + found.push(both.clone()); + assert_eq!(duplicate_refusal(&found), Some(format!("{}. Nothing was rendered", both.reason()))); + found.push(both.clone()); + assert!(duplicate_refusal(&found).is_some_and(|r| r.ends_with("Nothing was rendered; 1 more output would write a file twice too"))); + found.push(both); + assert!(duplicate_refusal(&found).is_some_and(|r| r.ends_with("Nothing was rendered; 2 more outputs would write a file twice too"))); + } + #[test] #[cfg(not(target_arch = "wasm32"))] fn unrelated_same_size_frames_are_indexed_once() { diff --git a/crates/engine/src/render_queue.rs b/crates/engine/src/render_queue.rs index 1ff91e8e8..0285a64d2 100644 --- a/crates/engine/src/render_queue.rs +++ b/crates/engine/src/render_queue.rs @@ -493,6 +493,11 @@ impl Session { let formats = exporter.formats(); let (mut items, mut problems) = self.queued_outputs(&formats); let refused = self.check_outputs(&items); + // Two outputs of this job on one file: the render would replace its own output, so it is + // refused before anything is written (the items refused above take no part). + if let Some(refusal) = crate::output_guard::duplicate_refusal(&self.duplicate_outputs(&items, &refused)) { + return Err(refusal); + } items.retain(|modules| modules.first().is_none_or(|(it, _)| !refused.iter().any(|r| r.item == it.id))); problems.extend(refused.iter().map(|r| (r.item, RenderStatus::Failed(r.reason.clone())))); if !problems.is_empty() { diff --git a/crates/engine/src/tests/render_overwrite.rs b/crates/engine/src/tests/render_overwrite.rs index 420763da2..68357acb7 100644 --- a/crates/engine/src/tests/render_overwrite.rs +++ b/crates/engine/src/tests/render_overwrite.rs @@ -1,5 +1,6 @@ //! Rendering never replaces a file the project reads (#1743). Ordinary-file replacement -//! behavior stays with the exporter and its confirmation API. +//! behavior stays with the exporter and its confirmation API. Two items of one job never write +//! the same file either. use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; @@ -604,3 +605,369 @@ fn a_watched_folder_still_renders_a_project_again_over_its_outputs() { assert_eq!(done[0]["state"], "done", "{:?}", done[0]); assert_eq!(f.read("Main.mp4"), RENDERED); } + +// ------------------------------------------------------------ one file, two items of one job +// +// Nothing is on disk when a job starts, so the check for files that exist said nothing about two +// queue items that resolve to one output: the item rendered last replaced the file of the one +// before it, inside the same job. + +impl Fixture { + /// The output path of every queue item, in queue order. + fn outputs(&self) -> Vec { + (0..self.s.project.render_queue.len()).map(|i| self.output(i)).collect() + } + + fn add(&mut self, params: Value) { + self.s.execute("renderQueue.add", params).unwrap(); + } + + /// What the queue would write twice, in the shape the tests read: `{"duplicates": [...]}`. + fn check(&mut self) -> Value { + let formats = self.s.exporter.as_ref().map(|e| e.formats()).unwrap_or_default(); + let (items, _) = self.s.queued_outputs(&formats); + let refused = self.s.check_outputs(&items); + let duplicates: Vec = self + .s + .duplicate_outputs(&items, &refused) + .iter() + .map(|d| { + json!({ + "item": d.item, "index": d.index, "comp": d.comp, "path": d.path, "files": d.files, "sequence": d.sequence, + "with": {"item": d.with, "index": d.with_index, "comp": d.with_comp}, "reason": d.reason(), + }) + }) + .collect(); + json!({"duplicates": duplicates}) + } + + /// What `check` lists under `duplicates`: for each, the place in the queue of the item that + /// would write over a file, the place of the item that wrote it first, and how many files. + fn clashes(&mut self) -> Vec<(u64, u64, u64)> { + let found = self.check(); + let list = found["duplicates"].as_array().unwrap_or_else(|| panic!("checkOutputs lists no duplicates: {found}")); + list.iter().map(|d| (d["index"].as_u64().unwrap_or(0), d["with"]["index"].as_u64().unwrap_or(0), d["files"].as_u64().unwrap_or(0))).collect() + } +} + +/// A PNG sequence of `pattern` for the composition's seconds `start..end`. +fn span(pattern: &str, start: f64, end: f64) -> Value { + json!({"format": "png", "output": pattern, "timeSpan": "custom", "start": start, "end": end}) +} + +/// After Effects numbers the second item of one composition (`Comp 1_1.mov`). A default output +/// name that another item of the queue already has is numbered the same way. +#[test] +fn a_second_default_item_for_one_comp_gets_the_next_free_name() { + let mut f = Fixture::new("second-default"); + f.comp("Main"); + for _ in 0..3 { + f.add(json!({})); + } + assert_eq!(f.outputs(), [f.path("Main.mp4"), f.path("Main_1.mp4"), f.path("Main_2.mp4")]); + // The name is stored in the item: taking the first one out renames nothing, and the name it + // frees is the next default. + f.s.execute("renderQueue.remove", json!({"index": 1})).unwrap(); + assert_eq!(f.outputs(), [f.path("Main_1.mp4"), f.path("Main_2.mp4")]); + f.add(json!({})); + assert_eq!(f.output(2), f.path("Main.mp4")); + assert!(f.clashes().is_empty()); + f.render(json!({})).unwrap(); + for file in ["Main.mp4", "Main_1.mp4", "Main_2.mp4"] { + assert_eq!(f.read(file), RENDERED, "{file}"); + } + assert_eq!(f.exported().len(), 3); +} + +#[test] +fn only_outputs_that_name_one_file_are_numbered() { + let mut f = Fixture::new("same-file-only"); + f.comp("Main"); + f.add(json!({"format": "h264"})); + f.add(json!({"format": "prores"})); + f.add(json!({"format": "hevc"})); // H.264 and HEVC both write .mp4 + f.add(json!({"format": "png"})); + f.add(json!({"format": "png"})); // the same five frames of the same sequence + assert_eq!(f.outputs(), [f.path("Main.mp4"), f.path("Main.mov"), f.path("Main_1.mp4"), f.path("Main_[#####].png"), f.path("Main_1_[#####].png")]); + assert!(f.clashes().is_empty()); +} + +#[test] +fn a_default_name_avoids_every_output_module_of_the_other_items() { + let mut f = Fixture::new("modules"); + f.comp("Main"); + f.add(json!({})); + // The item's second module is named Main_2.mp4, so that it does not clash with its first. + let item = f.s.execute("render.addOutputModule", json!({})).unwrap(); + assert_eq!(Path::new(item["extraOutputs"][0]["outputPath"].as_str().unwrap()), f.path("Main_2.mp4"), "{item}"); + f.add(json!({})); + f.add(json!({})); + assert_eq!(f.output(1), f.path("Main_1.mp4")); + assert_eq!(f.output(2), f.path("Main_3.mp4"), "Main_2.mp4 is the first item's second module"); + assert!(f.clashes().is_empty()); +} + +#[test] +fn a_name_the_caller_gave_is_never_numbered() { + let mut f = Fixture::new("named"); + f.comp("Main"); + let out = f.text("final.mp4"); + f.add(json!({"output": out})); + f.add(json!({"output": out})); + f.add(json!({"path": out})); + assert_eq!(f.outputs(), vec![f.path("final.mp4"); 3]); + assert!(f.s.project.render_queue.iter().all(|i| i.output.output == out), "the names stored in the items are the ones given"); + // Rendering them is what is refused (below): items 2 and 3 each clash with item 1. + assert_eq!(f.clashes(), [(2, 1, 1), (3, 1, 1)]); +} + +#[test] +fn a_pre_render_numbers_its_default_name_too() { + let mut f = Fixture::new("pre-render"); + f.comp("Main"); + f.s.execute("render.preRender", json!({})).unwrap(); + f.s.execute("render.preRender", json!({})).unwrap(); + f.s.execute("render.preRender", json!({"output": f.text("mine.mov")})).unwrap(); + f.s.execute("render.preRender", json!({"output": f.text("mine.mov")})).unwrap(); + assert_eq!(f.outputs(), [f.path("Main_prerender.mov"), f.path("Main_prerender_1.mov"), f.path("mine.mov"), f.path("mine.mov")]); +} + +/// File ▸ Create Proxy queues a render with a default name of its own too. +#[test] +fn create_proxy_numbers_its_default_name_too() { + let mut f = Fixture::new("proxy-names"); + f.comp("Main"); + f.s.execute("file.createProxy", json!({"kind": "movie"})).unwrap(); + f.s.execute("file.createProxy", json!({"kind": "movie"})).unwrap(); + f.s.execute("file.createProxy", json!({"kind": "movie", "output": f.text("mine.mov")})).unwrap(); + f.s.execute("file.createProxy", json!({"kind": "movie", "output": f.text("mine.mov")})).unwrap(); + assert_eq!(f.outputs(), [f.path("Main_proxy.mov"), f.path("Main_proxy_1.mov"), f.path("mine.mov"), f.path("mine.mov")]); +} + +/// Save Frame As ▸ File… with `queue`: every file of a sequence has its frame number in its name, +/// so two frames of one composition share no file, and the same frame twice does. +#[test] +fn queued_frames_number_their_default_names_only_when_they_are_the_same_frame() { + let mut f = Fixture::new("frames"); + f.comp("Main"); + for time in [0.0, 0.2, 0.0] { + f.s.execute("comp.saveFrameAs", json!({"queue": true, "time": time})).unwrap(); + } + assert_eq!(f.outputs(), [f.path("Main_[#####].png"), f.path("Main_[#####].png"), f.path("Main_1_[#####].png")]); + assert!(f.clashes().is_empty()); + f.render(json!({})).unwrap(); + for file in ["Main_00000.png", "Main_00002.png", "Main_1_00000.png"] { + assert_eq!(f.read(file), RENDERED, "{file}"); + } +} + +/// The report: two queue items forced to one path (the same Output To) replaced each other's file +/// inside one job. The render is refused before anything is written, whatever `overwrite` says: +/// that flag is for files that exist, and this is the render replacing its own output. +#[test] +fn two_items_forced_to_one_file_are_reported_and_the_render_is_refused() { + let mut f = Fixture::new("one-file"); + f.comp("Main"); + let out = f.text("render.mp4"); + f.add(json!({"output": out})); + f.add(json!({"output": out})); + let found = f.check(); + let list = found["duplicates"].as_array().unwrap_or_else(|| panic!("checkOutputs lists no duplicates: {found}")); + assert_eq!(list.len(), 1, "{found}"); + let d = &list[0]; + let (first, second) = (f.s.project.render_queue[0].id, f.s.project.render_queue[1].id); + assert_eq!((d["item"].as_u64(), d["index"].as_u64(), d["comp"].as_str()), (Some(second), Some(2), Some("Main")), "{found}"); + assert_eq!((d["with"]["item"].as_u64(), d["with"]["index"].as_u64(), d["with"]["comp"].as_str()), (Some(first), Some(1), Some("Main")), "{found}"); + assert_eq!((d["files"].as_u64(), d["sequence"].as_bool()), (Some(1), Some(false)), "{found}"); + assert_eq!(Path::new(d["path"].as_str().unwrap_or_default()), f.path("render.mp4"), "{found}"); + let why = d["reason"].as_str().unwrap_or_default(); + assert!(why.contains("#1") && why.contains("#2") && why.contains("render.mp4") && why.contains("Output To"), "{found}"); + + let queue = f.s.project.render_queue.clone(); + let e = f.render(json!({})).unwrap_err().to_string(); + assert!(e.contains("#1") && e.contains("#2") && e.contains("render.mp4") && e.contains("Nothing was rendered"), "{e}"); + assert!(!f.path("render.mp4").exists()); + assert!(f.exported().is_empty(), "the exporter was not started"); + assert_eq!(f.s.project.render_queue, queue, "a refused render changes nothing in the queue"); + + // A file that exists as well does not change that: replacing it would not help. + f.write("render.mp4", EARLIER); + let found = f.check(); + assert_eq!(found["duplicates"].as_array().map(Vec::len), Some(1), "{found}"); + let e = f.render(json!({})).unwrap_err().to_string(); + assert!(e.contains("#2") && e.contains("Nothing was rendered"), "{e}"); + assert_eq!(f.read("render.mp4"), EARLIER); + + // Another Output To for one of them, and both render. + f.s.execute("renderQueue.setOutput", json!({"index": 2, "path": f.text("second.mp4")})).unwrap(); + assert!(f.clashes().is_empty()); + f.render(json!({})).unwrap(); + assert_eq!((f.read("render.mp4"), f.read("second.mp4")), (RENDERED.to_vec(), RENDERED.to_vec())); +} + +/// `renderQueue.duplicate` copies the item as it is, output included: it is reported until one of +/// the two gets another Output To (or is turned off). +#[test] +fn a_duplicated_item_is_a_copy_and_clashes_until_one_of_them_changes() { + let mut f = Fixture::new("duplicate"); + f.comp("Main"); + f.add(json!({})); + f.s.execute("renderQueue.duplicate", json!({"index": 1})).unwrap(); + assert_eq!(f.outputs(), [f.path("Main.mp4"), f.path("Main.mp4")], "a duplicate is a copy, its output included"); + assert_eq!(f.clashes(), [(2, 1, 1)]); + let e = f.render(json!({})).unwrap_err().to_string(); + assert!(e.contains("#1") && e.contains("#2") && e.contains("Main.mp4"), "{e}"); + assert!(f.exported().is_empty()); + // Turned off, the copy is not part of the job. + f.s.execute("renderQueue.setRender", json!({"index": 2, "render": false})).unwrap(); + assert!(f.clashes().is_empty()); + f.render(json!({})).unwrap(); + assert_eq!(f.read("Main.mp4"), RENDERED); + assert_eq!(f.exported().len(), 1); +} + +/// Only the items of the job are compared: an earlier job's file is a file that exists, and +/// replacing it is up to `overwrite`. +#[test] +fn an_item_that_is_not_in_the_job_does_not_clash() { + let mut f = Fixture::new("not-in-job"); + f.comp("Main"); + let out = f.text("render.mp4"); + f.add(json!({"output": out})); + f.add(json!({"output": out})); + f.s.execute("renderQueue.setRender", json!({"index": 2, "render": false})).unwrap(); + f.render(json!({})).unwrap(); // item 1 renders alone + f.s.execute("renderQueue.setRender", json!({"index": 2, "render": true})).unwrap(); + assert!(f.clashes().is_empty(), "item 1 is done and is not in this job"); + // item 2 now replaces the file item 1 wrote in the earlier job: an ordinary file, left to the exporter + f.render(json!({})).unwrap(); +} + +#[test] +fn another_spelling_of_one_file_is_one_file_where_the_volume_ignores_case() { + let mut f = Fixture::new("spellings"); + f.comp("Main"); + f.write("probe.txt", b"x"); + if !f.path("PROBE.TXT").exists() { + eprintln!("this volume tells the two names apart: skipped"); + return; + } + f.add(json!({"output": f.text("Render.mp4")})); + f.add(json!({"output": f.text("render.MP4")})); + f.add(json!({"output": f.dir.join("sub").join("..").join("RENDER.mp4").to_string_lossy()})); + f.add(json!({"output": f.text("other.mp4")})); + assert_eq!(f.clashes(), [(2, 1, 1), (3, 1, 1)]); +} + +/// A sequence writes one file per frame: two items of one pattern clash on the frames they share, +/// and not at all when their frames differ. +#[test] +fn sequences_clash_only_on_the_frames_they_both_write() { + let mut f = Fixture::new("sequence-halves"); + f.comp("Main"); + let pattern = f.text("seq_[#####].png"); + f.add(span(&pattern, 0.0, 0.2)); // frames 0-1 + f.add(span(&pattern, 0.2, 0.5)); // frames 2-4 + assert!(f.clashes().is_empty()); + f.render(json!({})).unwrap(); + for n in 0..5 { + assert_eq!(f.read(&format!("seq_{n:05}.png")), RENDERED, "frame {n}"); + } + + let mut f = Fixture::new("sequence-overlap"); + f.comp("Main"); + let pattern = f.text("seq_[#####].png"); + f.add(span(&pattern, 0.0, 0.2)); + f.add(span(&pattern, 0.2, 0.5)); + f.add(span(&pattern, 0.3, 0.5)); // frames 3-4 again + assert_eq!(f.clashes(), [(3, 2, 2)]); + let found = f.check(); + assert_eq!(found["duplicates"][0]["sequence"], true, "{found}"); + let e = f.render(json!({"overwrite": true})).unwrap_err().to_string(); + assert!(e.contains("#2") && e.contains("#3") && e.contains("2 frames") && e.contains("seq_[#####].png"), "{e}"); + assert!(!f.path("seq_00000.png").exists(), "the frames before the clash were not rendered either"); + assert!(f.exported().is_empty()); +} + +/// Skip Existing Files leaves a frame alone when it is there: the item that skips and comes second +/// writes only what the first did not. The item that skips and comes first is written over. +#[test] +fn a_later_item_that_skips_existing_frames_leaves_the_earlier_ones_alone() { + let mut f = Fixture::new("skip-later"); + f.comp("Main"); + let pattern = f.text("seq_[#####].png"); + f.add(json!({"format": "png", "output": pattern})); + f.add(json!({"format": "png", "output": pattern, "skipExisting": true})); + assert!(f.clashes().is_empty()); + f.render(json!({})).unwrap(); + assert_eq!(f.read("seq_00004.png"), RENDERED); + + let mut f = Fixture::new("skip-earlier"); + f.comp("Main"); + let pattern = f.text("seq_[#####].png"); + f.add(json!({"format": "png", "output": pattern, "skipExisting": true})); + f.add(json!({"format": "png", "output": pattern})); + assert_eq!(f.clashes(), [(2, 1, 5)]); +} + +#[test] +fn two_output_modules_of_one_item_cannot_share_a_file() { + let mut f = Fixture::new("two-modules"); + f.comp("Main"); + let out = f.text("render.mp4"); + f.add(json!({"output": out})); + f.s.execute("render.addOutputModule", json!({"output": out})).unwrap(); + assert_eq!(f.clashes(), [(1, 1, 1)]); + let e = f.render(json!({})).unwrap_err().to_string(); + assert!(e.contains("#1") && e.contains("twice"), "{e}"); + assert!(f.exported().is_empty()); +} + +/// Settings ▸ Export ▸ Segment Sequences puts each item's frames in numbered folders: default +/// names keep two items' folders apart, and items given one name clash segment by segment. +#[test] +fn segmented_sequences_clash_segment_by_segment() { + let mut f = Fixture::new("segments"); + f.comp("Main"); + f.s.prefs.export.segment_sequences = true; + f.s.prefs.export.segment_sequence_files = 2; // five frames: segments of 2, 2 and 1 + f.add(json!({"format": "png"})); + f.add(json!({"format": "png"})); + assert_eq!(f.output(1), f.path("Main_1_[#####].png")); + assert!(f.clashes().is_empty()); + f.render(json!({})).unwrap(); + assert_eq!(f.read("Main_001/Main_00000.png"), RENDERED); + assert_eq!(f.read("Main_1_003/Main_1_00004.png"), RENDERED); + + let mut f = Fixture::new("segments-named"); + f.comp("Main"); + f.s.prefs.export.segment_sequences = true; + f.s.prefs.export.segment_sequence_files = 2; + let pattern = f.text("seg_[#####].png"); + f.add(json!({"format": "png", "output": pattern})); + f.add(json!({"format": "png", "output": pattern})); + assert_eq!(f.clashes(), [(2, 1, 2), (2, 1, 2), (2, 1, 1)]); + let e = f.render(json!({})).unwrap_err().to_string(); + assert!(e.contains("2 more"), "the first clash is named, the rest counted: {e}"); + assert!(f.exported().is_empty()); +} + +/// An item that is refused for the file it would replace does not write anything, so it cannot +/// clash with another item. +#[test] +fn items_refused_for_footage_are_not_compared_with_each_other() { + let mut f = Fixture::new("refused-pair"); + f.footage("clip.mov"); + f.comp("Main"); + let clip = f.text("clip.mov"); + f.add(json!({"format": "prores", "output": clip})); + f.add(json!({"format": "prores", "output": clip})); + f.add(json!({"output": f.text("ok.mp4")})); + let found = f.check(); + assert_eq!(found["duplicates"].as_array().map(Vec::len), Some(0), "{found}"); + let r = f.render(json!({})).unwrap(); + let failed = r["items"].as_array().unwrap().iter().filter(|i| i["status"].get("Failed").is_some()).count(); + assert_eq!(failed, 2, "both items on the footage are refused: {r}"); + assert_eq!(f.read("clip.mov"), SOURCE); + assert_eq!(f.read("ok.mp4"), RENDERED); +} diff --git a/docs/agents.md b/docs/agents.md index d801fcec6..882fb13c3 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -319,6 +319,14 @@ build real projects through these interfaces; they are worked examples of everyt proresProfile: 4444, output}` then `renderQueue.render {"wait": true}`. Check a render by importing it (`file.import`, `file.newCompFromSelection`) and `render_frame {transparent: true}` for its alpha (ProRes 4444 and WebM VP9 alpha both read back as straight alpha). + **A render never writes one file twice either:** when two outputs of the job (two items, a + `renderQueue.duplicate` copy, output modules, segments) name the same file, the whole render is + refused before anything is written, naming both items. Sequences clash only on frames both + write, and a later item with Skip Existing Files leaves the earlier one's frames alone. Default + names are numbered like footage collisions when another queue item writes them + (`renderQueue.add`, `render.preRender`, `comp.saveFrameAs {queue}`, `file.createProxy`: a second + `Main` renders to `Main_1.mp4`); a name you give is kept, and `renderQueue.duplicate` copies the + output as it is, so change one with `renderQueue.setOutput` or turn it off. * **Essential Graphics**: controls can be addressed by name (`essential.set {"layer":"#1", "control":"Title","value":"John Smith"}`); a command that targets an explicit `comp` runs even when the active comp would disable it (`essential.exportTemplate {"comp":"Lower Third", …}`). diff --git a/docs/mcp.md b/docs/mcp.md index 686adcb35..59d41552e 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -53,6 +53,15 @@ receive `resultType: complete` and list/read cache hints; document reads are nev {"jsonrpc":"2.0","id":2,"method":"resources/read","params":{"uri":"effectcraft://document"}} ``` +## Two outputs on one file + +Two outputs of one job that write the same file (two items with one Output To, a duplicated item, +output modules or segments that meet) refuse the whole `renderQueue.render` before anything is +written: the reply is `isError: true`, naming both items, and the queue is left as it was. +Sequences clash only on the frames both write, and an item with Skip Existing Files leaves the +earlier one's frames alone. A new item's default output name is numbered (`Main.mp4`, +`Main_1.mp4`) when another item of the queue writes it. + ## Progress and cancellation Headless `command_run` / `execute_command` calls to `renderQueue.render` (unless `wait: false`) From b6c3e6dc39745af79f37a33a27099679b8683bfe Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 08:26:09 +0100 Subject: [PATCH 2/7] #1830: detect sequence padding and individual-file collisions --- crates/engine/src/output_guard.rs | 69 +++++++++++++++++---- crates/engine/src/tests/render_overwrite.rs | 11 ++++ 2 files changed, 68 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index ba6947694..cff17ff5c 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -267,7 +267,7 @@ enum Target { File { key: String, identity: Option }, /// The numbered files `first..end` of a sequence: `key` is its pattern, with the width the /// frame numbers are padded to. - Frames { key: String, first: i64, end: i64 }, + Frames { key: String, width: usize, first: i64, end: i64 }, } impl Target { @@ -281,8 +281,8 @@ impl Target { /// The `count` files of the sequence `path` (its frame-number pattern) from frame `first`. fn frames(path: &str, first: i64, count: u64) -> Target { let (before, width, after) = sequence_parts(path); - let key = format!("{width}:{}", key(Path::new(&format!("{before}{FRAME}{after}")))); - Target::Frames { key, first, end: first.saturating_add(i64::try_from(count).unwrap_or(i64::MAX)) } + let key = key(Path::new(&format!("{before}{FRAME}{after}"))); + Target::Frames { key, width, first, end: first.saturating_add(i64::try_from(count).unwrap_or(i64::MAX)) } } } @@ -316,8 +316,22 @@ struct Writes { files: HashMap, /// ... and by what the file is on its volume. identities: HashMap, - /// Every output that writes the frames of a sequence pattern. - frames: HashMap>, +} + +/// The common range whose formatted numbers agree. Once the number (including a minus sign) +/// fills the larger padding width, different widths produce the same file name. +fn common_frames(first: i64, end: i64, from: i64, to: i64, width: usize, other_width: usize) -> Option<(u64, i64)> { + let (start, end) = (first.max(from), end.min(to)); + let count = |a: i64, b: i64| u64::try_from(b.saturating_sub(a)).unwrap_or(0); + if width == other_width { + return (end > start).then(|| (count(start, end), start)); + } + let width = width.max(other_width); + let power = |digits: usize| u32::try_from(digits).ok().and_then(|digits| 10i64.checked_pow(digits)); + let negative = power(width.saturating_sub(2)).map(|n| end.min((-n).saturating_add(1))).filter(|to| *to > start); + let positive = power(width.saturating_sub(1)).map(|n| start.max(n)).filter(|from| *from < end); + let files = negative.map_or(0, |to| count(start, to)).saturating_add(positive.map_or(0, |from| count(from, end))); + (files > 0).then(|| (files, if negative.is_some() { start } else { positive.unwrap_or(start) })) } impl Writes { @@ -329,16 +343,32 @@ impl Writes { fn clash(&self, later: &Write) -> Option> { match &later.target { Target::File { key, identity } => { - let at = self.files.get(key).or_else(|| identity.as_ref().and_then(|id| self.identities.get(id)))?; - Some(Clash { earlier: self.all.get(*at)?, files: 1, first: 0 }) + if let Some(at) = self.files.get(key).or_else(|| identity.as_ref().and_then(|id| self.identities.get(id))) { + return Some(Clash { earlier: self.all.get(*at)?, files: 1, first: 0 }); + } + self.all.iter().find_map(|earlier| { + let Target::Frames { first, end, .. } = earlier.target else { return None }; + let frame = sequence_frame(&crate::output_guard::key(Path::new(&earlier.path)), key)?; + (first..end).contains(&frame).then_some(Clash { earlier, files: 1, first: frame }) + }) } // A frame that is there already is left alone, so a later output that skips existing // frames replaces none of an earlier one's. Target::Frames { .. } if later.skips => None, - Target::Frames { key, first, end } => self.frames.get(key)?.iter().filter_map(|at| self.all.get(*at)).find_map(|earlier| { - let Target::Frames { first: from, end: to, .. } = &earlier.target else { return None }; - let start = (*first).max(*from); - let files = u64::try_from((*end).min(*to).saturating_sub(start)).ok().filter(|n| *n > 0)?; + Target::Frames { key, width, first, end } => self.all.iter().find_map(|earlier| { + let (files, start) = match &earlier.target { + Target::Frames { key: other, width: other_width, first: from, end: to } if key == other => { + common_frames(*first, *end, *from, *to, *width, *other_width)? + } + Target::File { key, .. } => { + let frame = sequence_frame(&crate::output_guard::key(Path::new(&later.path)), key)?; + if !(*first..*end).contains(&frame) { + return None; + } + (1, frame) + } + _ => return None, + }; Some(Clash { earlier, files, first: start }) }), } @@ -353,7 +383,7 @@ impl Writes { self.identities.entry(identity.clone()).or_insert(at); } } - Target::Frames { key, .. } => self.frames.entry(key.clone()).or_default().push(at), + Target::Frames { .. } => {} } self.all.push(write); } @@ -646,6 +676,21 @@ mod tests { assert_eq!(clash(&job, &frames(2, "out/[##].png", 0, 5)), Some((1, 5, 0))); } + #[test] + fn padding_widths_can_name_the_same_frame_and_files_can_name_sequence_frames() { + let job = job_of([frames(1, "out/a_[####].png", 9999, 3)]); + assert_eq!(clash(&job, &frames(2, "out/a_[#####].png", 9999, 3)), Some((1, 2, 10000))); + let job = job_of([frames(1, "out/a_[##].png", -12, 15)]); + assert_eq!(clash(&job, &frames(2, "out/a_[###].png", -12, 15)), Some((1, 3, -12))); + let job = job_of([frames(1, "out/a_[#####].png", 0, 5)]); + assert_eq!(clash(&job, &movie(2, "out/a_00002.png")), Some((1, 1, 2))); + let job = job_of([movie(1, "out/a_00002.png")]); + assert_eq!(clash(&job, &frames(2, "out/a_[#####].png", 0, 5)), Some((1, 1, 2))); + let mut skipping = frames(2, "out/a_[#####].png", 0, 5); + skipping.skips = true; + assert_eq!(clash(&job, &skipping), None); + } + /// Skip Existing Files leaves a frame that is there alone: it matters for the output that /// comes second, and for sequences only. #[test] diff --git a/crates/engine/src/tests/render_overwrite.rs b/crates/engine/src/tests/render_overwrite.rs index 68357acb7..a94063253 100644 --- a/crates/engine/src/tests/render_overwrite.rs +++ b/crates/engine/src/tests/render_overwrite.rs @@ -889,6 +889,17 @@ fn sequences_clash_only_on_the_frames_they_both_write() { assert!(f.exported().is_empty()); } +#[test] +fn differently_padded_sequences_cannot_replace_the_same_frames() { + let mut f = Fixture::new("padding-width-clash"); + f.s.execute("comp.new", json!({"name": "Main", "width": 16, "height": 16, "frameRate": 10, "duration": 2})).unwrap(); + f.add(span(&f.text("seq_[#].png"), 0.9, 1.2)); + f.add(span(&f.text("seq_[##].png"), 0.9, 1.2)); + assert_eq!(f.clashes(), [(2, 1, 2)]); + assert!(f.render(json!({})).is_err()); + assert!(f.exported().is_empty()); +} + /// Skip Existing Files leaves a frame alone when it is there: the item that skips and comes second /// writes only what the first did not. The item that skips and comes first is written over. #[test] From 37561799f6bb9c061ac4124ef3c7a4f75fc0fe1b Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 08:42:03 +0100 Subject: [PATCH 3/7] #1830: check virtual output paths in web renders --- crates/engine/src/output_guard.rs | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index cff17ff5c..7634313f2 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -473,9 +473,6 @@ impl Session { /// What the job writes more than once. An item in `refused` writes nothing, so it takes no part. pub(crate) fn duplicate_outputs(&self, items: &[Vec<(RenderQueueItem, String)>], refused: &[Refused]) -> Vec { - if cfg!(target_arch = "wasm32") { - return Vec::new(); - } let skip: HashSet = refused.iter().map(|r| r.item).collect(); let mut writes = Writes::default(); let mut duplicates = Vec::new(); @@ -547,10 +544,9 @@ impl Session { /// is stored in the item, so it stays as it is when the item it avoided goes away. One the /// caller chose is not touched: rendering it is refused instead ([`Session::duplicate_outputs`]). pub(crate) fn avoid_sources(&self, item: &mut RenderQueueItem) { - if cfg!(target_arch = "wasm32") { - return; - } - let protected = self.protected_files(); + // Native source protection stays with its filesystem guard; virtual web outputs still + // need distinct queue names because the browser stores completed files by path. + let protected = if cfg!(target_arch = "wasm32") { Protected::default() } else { self.protected_files() }; let queue = self.queue_writes(); if protected.is_empty() && queue.is_empty() { return; @@ -594,6 +590,23 @@ impl Session { #[cfg(test)] mod tests { use super::*; + use serde_json::json; + + /// Portable path checks protect virtual outputs too; they do not need native file identity. + #[test] + fn duplicate_queue_paths_are_reported_without_native_files() { + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "Main", "width": 16, "height": 16})).unwrap(); + for _ in 0..2 { + s.execute("renderQueue.add", json!({"format": "h264", "output": "/virtual/shared.mp4"})).unwrap(); + } + let (outputs, _) = s.queued_outputs(&effectcraft_project::render_queue::OutputFormat::ALL); + let duplicates = s.duplicate_outputs(&outputs, &[]); + assert_eq!(duplicates.len(), 1); + assert!(duplicates[0].path.ends_with("shared.mp4")); + let refused = [Refused { item: duplicates[0].item, reason: "already refused".into() }]; + assert!(s.duplicate_outputs(&outputs, &refused).is_empty()); + } /// An output of queue item `item` that is a file, known by its name alone. fn movie(item: u64, path: &str) -> Write { From c5671f6569efd5817646fde30c250eea4e81643e Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 08:59:53 +0100 Subject: [PATCH 4/7] #1830: require exporter support before skipping duplicate frames --- crates/engine/src/output_guard.rs | 3 ++- crates/engine/src/render_queue.rs | 5 +++++ crates/engine/src/tests/render_overwrite.rs | 3 +++ crates/host/src/lib.rs | 3 +++ crates/host/src/tests.rs | 21 +++++++++++++++++++++ docs/agents.md | 3 ++- docs/mcp.md | 5 +++-- 7 files changed, 39 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index 7634313f2..86c9fa5e6 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -494,7 +494,8 @@ impl Session { } else { Target::file(Path::new(path), by_identity) }; - Write { item: m.id, path: path.to_string(), target, skips: m.settings.skip_existing } + let skips = m.settings.skip_existing && self.exporter.as_ref().is_some_and(|exporter| exporter.supports_skip_existing()); + Write { item: m.id, path: path.to_string(), target, skips } } /// The report of `later` writing over `clash`. diff --git a/crates/engine/src/render_queue.rs b/crates/engine/src/render_queue.rs index 0285a64d2..045b9f782 100644 --- a/crates/engine/src/render_queue.rs +++ b/crates/engine/src/render_queue.rs @@ -57,6 +57,11 @@ pub struct ExportResult { pub trait Exporter: Send + Sync { /// Formats this build can write. fn formats(&self) -> Vec; + /// Whether Skip Existing Files preserves sequence frames written by earlier items. Exporters + /// without an existence check (such as memory sinks) must keep the conservative default. + fn supports_skip_existing(&self) -> bool { + false + } /// Render and write one item (blocking). `progress(done, total)` returns `false` to cancel; /// a cancelled export removes the files it opened for that output and returns `Err("cancelled")`. /// Completed earlier queue items and unrelated files must be preserved. diff --git a/crates/engine/src/tests/render_overwrite.rs b/crates/engine/src/tests/render_overwrite.rs index a94063253..8572a56f8 100644 --- a/crates/engine/src/tests/render_overwrite.rs +++ b/crates/engine/src/tests/render_overwrite.rs @@ -28,6 +28,9 @@ impl Exporter for Writer { fn formats(&self) -> Vec { OutputFormat::ALL.to_vec() } + fn supports_skip_existing(&self) -> bool { + true + } fn export(&self, job: &ExportJob, _: &mut dyn FnMut(u64, u64) -> bool) -> Result { self.jobs.lock().unwrap().push(job.path.to_string()); let comp = job.project.comp(job.item.comp).ok_or("no comp")?; diff --git a/crates/host/src/lib.rs b/crates/host/src/lib.rs index 08e1616a7..1137a8bce 100644 --- a/crates/host/src/lib.rs +++ b/crates/host/src/lib.rs @@ -33,6 +33,9 @@ impl Exporter for FileExporter { fn formats(&self) -> Vec { effectcraft_export::available_formats() } + fn supports_skip_existing(&self) -> bool { + self.sink.is_none() && !cfg!(target_arch = "wasm32") + } fn export(&self, job: &ExportJob, progress: &mut dyn FnMut(u64, u64) -> bool) -> Result { effectcraft_engine::render::passes::block_on(self.export_async(job, progress)) } diff --git a/crates/host/src/tests.rs b/crates/host/src/tests.rs index 3114c2ddb..447411dc7 100644 --- a/crates/host/src/tests.rs +++ b/crates/host/src/tests.rs @@ -1,5 +1,26 @@ use serde_json::json; +#[test] +fn memory_sink_cannot_overwrite_a_previous_output_even_with_skip_existing() { + use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }; + let writes = Arc::new(AtomicUsize::new(0)); + let count = writes.clone(); + let sink: Arc = Arc::new(move |_, _| { + count.fetch_add(1, Ordering::Relaxed); + }); + let mut s = effectcraft_engine::Session { exporter: Some(Arc::new(super::FileExporter { sink: Some(sink) })), ..Default::default() }; + s.execute("comp.new", json!({"name": "Main", "width": 16, "height": 16, "frameRate": 10, "duration": 0.2})).unwrap(); + for _ in 0..2 { + s.execute("renderQueue.add", json!({"format": "png", "output": "/virtual/frames_[#####].png", "skipExisting": true})).unwrap(); + } + let result = s.execute("renderQueue.render", json!({})); + assert!(result.is_err(), "a sink cannot skip previous paths: {result:?}"); + assert_eq!(writes.load(Ordering::Relaxed), 0, "refused before the sink receives bytes"); +} + #[test] fn host_runs_tasks_scheduled_by_panel_handlers() { let mut s = super::session(); diff --git a/docs/agents.md b/docs/agents.md index 882fb13c3..3d1f4dd83 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -322,7 +322,8 @@ build real projects through these interfaces; they are worked examples of everyt **A render never writes one file twice either:** when two outputs of the job (two items, a `renderQueue.duplicate` copy, output modules, segments) name the same file, the whole render is refused before anything is written, naming both items. Sequences clash only on frames both - write, and a later item with Skip Existing Files leaves the earlier one's frames alone. Default + write. A later item with Skip Existing Files leaves the earlier one's frames alone only when + the exporter can check existing files; browser and other memory sinks still report collisions. Default names are numbered like footage collisions when another queue item writes them (`renderQueue.add`, `render.preRender`, `comp.saveFrameAs {queue}`, `file.createProxy`: a second `Main` renders to `Main_1.mp4`); a name you give is kept, and `renderQueue.duplicate` copies the diff --git a/docs/mcp.md b/docs/mcp.md index 59d41552e..116158706 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -58,8 +58,9 @@ receive `resultType: complete` and list/read cache hints; document reads are nev Two outputs of one job that write the same file (two items with one Output To, a duplicated item, output modules or segments that meet) refuse the whole `renderQueue.render` before anything is written: the reply is `isError: true`, naming both items, and the queue is left as it was. -Sequences clash only on the frames both write, and an item with Skip Existing Files leaves the -earlier one's frames alone. A new item's default output name is numbered (`Main.mp4`, +Sequences clash only on the frames both write. Skip Existing Files leaves earlier frames alone +only when the exporter can check existing files; browser and other memory sinks still report +collisions. A new item's default output name is numbered (`Main.mp4`, `Main_1.mp4`) when another item of the queue writes it. ## Progress and cancellation From b7d97c20e9691df17243ab867b46d1064dbab38e Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 09:55:49 +0100 Subject: [PATCH 5/7] #1743: check final storage overflow destinations for queued outputs --- crates/engine/src/output_guard.rs | 100 ++++++++++++++++---- crates/engine/src/render_queue.rs | 5 +- crates/engine/src/tests/render_overwrite.rs | 39 +++++++- crates/host/src/lib.rs | 5 +- crates/host/src/tests.rs | 44 +++++++++ docs/agents.md | 3 +- docs/mcp.md | 5 +- 7 files changed, 175 insertions(+), 26 deletions(-) diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index 86c9fa5e6..fc625c309 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -312,6 +312,7 @@ struct Clash<'a> { #[derive(Default)] struct Writes { all: Vec, + path_bytes: usize, /// The first output to write a file, by name. files: HashMap, /// ... and by what the file is on its volume. @@ -374,7 +375,12 @@ impl Writes { } } - fn push(&mut self, write: Write) { + fn push(&mut self, write: Write) -> Result<(), String> { + let path_bytes = self.path_bytes.saturating_add(write.path.len()); + if self.all.len() >= 65_536 || path_bytes > 16 * 1024 * 1024 { + return Err("render output plan exceeds 65536 paths or 16 MiB of path names; split the queue".into()); + } + self.path_bytes = path_bytes; let at = self.all.len(); match &write.target { Target::File { key, identity } => { @@ -386,6 +392,7 @@ impl Writes { Target::Frames { .. } => {} } self.all.push(write); + Ok(()) } } @@ -459,8 +466,11 @@ impl Session { if seen.contains(&item.id) { continue; } - let reason = match self.check_output(&protected, item, path) { - Ok(reason) => reason, + let reason = match self.write_paths(item, path) { + Ok(paths) => paths.iter().find_map(|path| match self.check_output(&protected, item, path) { + Ok(reason) => reason, + Err(reason) => Some(reason), + }), Err(reason) => Some(reason), }; if let Some(reason) = reason { @@ -472,18 +482,55 @@ impl Session { } /// What the job writes more than once. An item in `refused` writes nothing, so it takes no part. - pub(crate) fn duplicate_outputs(&self, items: &[Vec<(RenderQueueItem, String)>], refused: &[Refused]) -> Vec { + pub(crate) fn duplicate_outputs(&self, items: &[Vec<(RenderQueueItem, String)>], refused: &[Refused]) -> Result, String> { let skip: HashSet = refused.iter().map(|r| r.item).collect(); let mut writes = Writes::default(); let mut duplicates = Vec::new(); for (m, path) in items.iter().flatten().filter(|(m, _)| !skip.contains(&m.id)) { - let write = self.write_of(m, path, true); - if let Some(clash) = writes.clash(&write) { - duplicates.push(self.duplicate_of(&write, &clash)); + let paths = self.write_paths(m, path)?; + // Alternative destinations of one output cannot collide with themselves. + for path in &paths { + let write = self.write_of(m, path, true); + if let Some(clash) = writes.clash(&write) { + duplicates.push(self.duplicate_of(&write, &clash)); + break; + } + } + for path in paths { + writes.push(self.write_of(m, &path, true))?; } - writes.push(write); } - duplicates + Ok(duplicates) + } + + /// Every possible destination, since quota decisions can change during rendering. + fn write_paths<'a>(&self, item: &RenderQueueItem, path: &'a str) -> Result>, String> { + let overflow = &self.project.render_prefs.overflow_folders; + const PATH_BYTES: usize = 1024 * 1024; + if path.len() > PATH_BYTES { + return Err("render output destinations exceed 1 MiB of path names".into()); + } + let mut paths = vec![std::borrow::Cow::Borrowed(path)]; + let mut bytes = path.len(); + if item.settings.storage_overflow && self.storage_quota.is_some() { + if overflow.len() > 256 { + return Err("render output planning supports at most 256 storage overflow folders".into()); + } + if let Some(name) = Path::new(path).file_name() { + for folder in overflow { + if bytes.saturating_add(folder.len()).saturating_add(name.len()).saturating_add(1) > PATH_BYTES { + return Err("render output destinations exceed 1 MiB of path names".into()); + } + let destination = Path::new(folder).join(name).to_string_lossy().into_owned(); + bytes = bytes.saturating_add(destination.len()); + if bytes > PATH_BYTES { + return Err("render output destinations exceed 1 MiB of path names".into()); + } + paths.push(std::borrow::Cow::Owned(destination)); + } + } + } + Ok(paths) } /// What `path`, an output of `m`, writes. `by_identity`: see [`Target::file`]. @@ -494,7 +541,7 @@ impl Session { } else { Target::file(Path::new(path), by_identity) }; - let skips = m.settings.skip_existing && self.exporter.as_ref().is_some_and(|exporter| exporter.supports_skip_existing()); + let skips = m.settings.skip_existing && self.exporter.as_ref().is_some_and(|exporter| exporter.supports_skip_existing(&m.settings)); Write { item: m.id, path: path.to_string(), target, skips } } @@ -548,7 +595,7 @@ impl Session { // Native source protection stays with its filesystem guard; virtual web outputs still // need distinct queue names because the browser stores completed files by path. let protected = if cfg!(target_arch = "wasm32") { Protected::default() } else { self.protected_files() }; - let queue = self.queue_writes(); + let Ok(queue) = self.queue_writes() else { return }; if protected.is_empty() && queue.is_empty() { return; } @@ -566,7 +613,11 @@ impl Session { } Ok(None) => {} } - in_the_way |= queue.clash(&self.write_of(module, path, false)).is_some(); + let Ok(paths) = self.write_paths(module, path) else { + item.output.output = template; + return; + }; + in_the_way |= paths.iter().any(|path| queue.clash(&self.write_of(module, path, false)).is_some()); } if !in_the_way { return; @@ -577,14 +628,16 @@ impl Session { /// What the items of the queue write (by name: a new item is not compared with what is on /// disk). - fn queue_writes(&self) -> Writes { + fn queue_writes(&self) -> Result { let mut writes = Writes::default(); for outputs in self.project.render_queue.iter().filter_map(|it| self.outputs_of(it, None).ok()) { for (m, path) in &outputs { - writes.push(self.write_of(m, path, false)); + for path in self.write_paths(m, path)? { + writes.push(self.write_of(m, &path, false))?; + } } } - writes + Ok(writes) } } @@ -602,11 +655,11 @@ mod tests { s.execute("renderQueue.add", json!({"format": "h264", "output": "/virtual/shared.mp4"})).unwrap(); } let (outputs, _) = s.queued_outputs(&effectcraft_project::render_queue::OutputFormat::ALL); - let duplicates = s.duplicate_outputs(&outputs, &[]); + let duplicates = s.duplicate_outputs(&outputs, &[]).unwrap(); assert_eq!(duplicates.len(), 1); assert!(duplicates[0].path.ends_with("shared.mp4")); let refused = [Refused { item: duplicates[0].item, reason: "already refused".into() }]; - assert!(s.duplicate_outputs(&outputs, &refused).is_empty()); + assert!(s.duplicate_outputs(&outputs, &refused).unwrap().is_empty()); } /// An output of queue item `item` that is a file, known by its name alone. @@ -622,11 +675,22 @@ mod tests { fn job_of(writes: impl IntoIterator) -> Writes { let mut job = Writes::default(); for write in writes { - job.push(write); + job.push(write).unwrap(); } job } + #[test] + fn output_plan_path_budget_returns_an_error() { + let mut job = Writes::default(); + let path = "x".repeat(1024 * 1024); + for _ in 0..16 { + job.push(movie(1, &path)).unwrap(); + } + assert!(job.push(movie(2, "one-too-many")).is_err()); + assert_eq!(job.all.len(), 16); + } + /// The item an output replaces a file of, how many files, and the first. fn clash(job: &Writes, later: &Write) -> Option<(u64, u64, i64)> { job.clash(later).map(|c| (c.earlier.item, c.files, c.first)) diff --git a/crates/engine/src/render_queue.rs b/crates/engine/src/render_queue.rs index 045b9f782..41474ceef 100644 --- a/crates/engine/src/render_queue.rs +++ b/crates/engine/src/render_queue.rs @@ -59,7 +59,8 @@ pub trait Exporter: Send + Sync { fn formats(&self) -> Vec; /// Whether Skip Existing Files preserves sequence frames written by earlier items. Exporters /// without an existence check (such as memory sinks) must keep the conservative default. - fn supports_skip_existing(&self) -> bool { + /// The check must cover the final destination for these settings, including any overflow. + fn supports_skip_existing(&self, _settings: &effectcraft_project::render_queue::RenderSettings) -> bool { false } /// Render and write one item (blocking). `progress(done, total)` returns `false` to cancel; @@ -500,7 +501,7 @@ impl Session { let refused = self.check_outputs(&items); // Two outputs of this job on one file: the render would replace its own output, so it is // refused before anything is written (the items refused above take no part). - if let Some(refusal) = crate::output_guard::duplicate_refusal(&self.duplicate_outputs(&items, &refused)) { + if let Some(refusal) = crate::output_guard::duplicate_refusal(&self.duplicate_outputs(&items, &refused)?) { return Err(refusal); } items.retain(|modules| modules.first().is_none_or(|(it, _)| !refused.iter().any(|r| r.item == it.id))); diff --git a/crates/engine/src/tests/render_overwrite.rs b/crates/engine/src/tests/render_overwrite.rs index 8572a56f8..d1ca6e65a 100644 --- a/crates/engine/src/tests/render_overwrite.rs +++ b/crates/engine/src/tests/render_overwrite.rs @@ -28,7 +28,7 @@ impl Exporter for Writer { fn formats(&self) -> Vec { OutputFormat::ALL.to_vec() } - fn supports_skip_existing(&self) -> bool { + fn supports_skip_existing(&self, _: &effectcraft_project::render_queue::RenderSettings) -> bool { true } fn export(&self, job: &ExportJob, _: &mut dyn FnMut(u64, u64) -> bool) -> Result { @@ -633,6 +633,7 @@ impl Fixture { let duplicates: Vec = self .s .duplicate_outputs(&items, &refused) + .unwrap() .iter() .map(|d| { json!({ @@ -658,6 +659,42 @@ fn span(pattern: &str, start: f64, end: f64) -> Value { json!({"format": "png", "output": pattern, "timeSpan": "custom", "start": start, "end": end}) } +fn with_overflow(f: &mut Fixture) { + struct Room; + impl effectcraft_project::render_queue::StorageQuota for Room { + fn has_room(&self, _: &str, _: u64) -> bool { + true + } + } + f.s.storage_quota = Some(Arc::new(Room)); + std::fs::create_dir_all(f.path("overflow")).unwrap(); + Arc::make_mut(&mut f.s.project).render_prefs.overflow_folders = vec![f.text("overflow")]; +} + +#[test] +fn default_names_avoid_possible_storage_overflow_collisions() { + let mut f = Fixture::new("overflow-default"); + f.comp("Main"); + with_overflow(&mut f); + f.add(json!({"output": f.text("other/Main.mp4"), "storageOverflow": true})); + f.add(json!({"storageOverflow": true})); + assert_eq!(f.output(1), f.path("Main_1.mp4")); + assert!(f.clashes().is_empty()); +} + +#[test] +fn possible_storage_overflow_destinations_cannot_replace_footage() { + let mut f = Fixture::new("overflow-footage"); + f.comp("Main"); + with_overflow(&mut f); + f.footage("overflow/Main.mp4"); + f.add(json!({"output": f.text("other/Main.mp4"), "storageOverflow": true})); + let error = f.render(json!({})).unwrap_err().to_string(); + assert!(error.contains("footage") && error.contains("Main.mp4"), "{error}"); + assert!(f.exported().is_empty()); + assert_eq!(f.read("overflow/Main.mp4"), SOURCE); +} + /// After Effects numbers the second item of one composition (`Comp 1_1.mov`). A default output /// name that another item of the queue already has is numbered the same way. #[test] diff --git a/crates/host/src/lib.rs b/crates/host/src/lib.rs index 1137a8bce..9a6556fdc 100644 --- a/crates/host/src/lib.rs +++ b/crates/host/src/lib.rs @@ -33,8 +33,9 @@ impl Exporter for FileExporter { fn formats(&self) -> Vec { effectcraft_export::available_formats() } - fn supports_skip_existing(&self) -> bool { - self.sink.is_none() && !cfg!(target_arch = "wasm32") + fn supports_skip_existing(&self, settings: &effectcraft_project::render_queue::RenderSettings) -> bool { + // The exporter's existence check precedes storage-overflow redirection. + self.sink.is_none() && !cfg!(target_arch = "wasm32") && !settings.storage_overflow } fn export(&self, job: &ExportJob, progress: &mut dyn FnMut(u64, u64) -> bool) -> Result { effectcraft_engine::render::passes::block_on(self.export_async(job, progress)) diff --git a/crates/host/src/tests.rs b/crates/host/src/tests.rs index 447411dc7..f628ce4e4 100644 --- a/crates/host/src/tests.rs +++ b/crates/host/src/tests.rs @@ -1,5 +1,49 @@ use serde_json::json; +#[test] +#[cfg(not(target_arch = "wasm32"))] +fn storage_overflow_cannot_bypass_duplicate_output_protection() { + for same_folder in [true, false] { + refuse_overflow_collision(same_folder); + } +} + +#[cfg(not(target_arch = "wasm32"))] +fn refuse_overflow_collision(same_folder: bool) { + use std::{path::PathBuf, sync::Arc}; + struct OverflowOnly(PathBuf); + impl effectcraft_project::render_queue::StorageQuota for OverflowOnly { + fn has_room(&self, path: &str, _: u64) -> bool { + std::path::Path::new(path).starts_with(&self.0) + } + } + let dir = std::env::temp_dir().join(format!("ec-duplicate-overflow-{}-{same_folder}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let overflow = dir.join("overflow"); + std::fs::create_dir_all(&overflow).unwrap(); + let mut s = effectcraft_engine::Session { + exporter: Some(Arc::new(super::FileExporter::default())), + storage_quota: Some(Arc::new(OverflowOnly(overflow.clone()))), + ..Default::default() + }; + Arc::make_mut(&mut s.project).render_prefs.overflow_folders = vec![overflow.to_string_lossy().into_owned(); 2]; + s.execute("comp.new", json!({"width": 16, "height": 16, "frameRate": 10, "duration": 0.2})).unwrap(); + for index in 0..2 { + let folder = if same_folder { dir.clone() } else { dir.join(index.to_string()) }; + s.execute("renderQueue.add", json!({"format": "png", "output": folder.join("frames_[#####].png"), "skipExisting": true, "storageOverflow": true})) + .unwrap(); + } + let result = s.execute("renderQueue.render", json!({})); + let written = std::fs::read_dir(&overflow).unwrap().count(); + assert!(result.is_err(), "overflow destinations cannot skip previous frames: {result:?}"); + assert_eq!(written, 0, "refused before any overflow file is written"); + s.execute("renderQueue.remove", json!({"index": 2})).unwrap(); + let result = s.execute("renderQueue.render", json!({})).unwrap(); + assert_eq!(result["items"][0]["status"], "Done", "one output's alternatives are not separate outputs: {result}"); + assert_eq!(std::fs::read_dir(&overflow).unwrap().count(), 2); + std::fs::remove_dir_all(&dir).unwrap(); +} + #[test] fn memory_sink_cannot_overwrite_a_previous_output_even_with_skip_existing() { use std::sync::{ diff --git a/docs/agents.md b/docs/agents.md index 3d1f4dd83..8d411add4 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -323,7 +323,8 @@ build real projects through these interfaces; they are worked examples of everyt `renderQueue.duplicate` copy, output modules, segments) name the same file, the whole render is refused before anything is written, naming both items. Sequences clash only on frames both write. A later item with Skip Existing Files leaves the earlier one's frames alone only when - the exporter can check existing files; browser and other memory sinks still report collisions. Default + the exporter can check the final destination. Browser/memory sinks and storage-overflow renders still report collisions. + Preflight compares every configured overflow destination, since available space can change during a render. Default names are numbered like footage collisions when another queue item writes them (`renderQueue.add`, `render.preRender`, `comp.saveFrameAs {queue}`, `file.createProxy`: a second `Main` renders to `Main_1.mp4`); a name you give is kept, and `renderQueue.duplicate` copies the diff --git a/docs/mcp.md b/docs/mcp.md index 116158706..fb02e79b6 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -59,8 +59,9 @@ Two outputs of one job that write the same file (two items with one Output To, a output modules or segments that meet) refuse the whole `renderQueue.render` before anything is written: the reply is `isError: true`, naming both items, and the queue is left as it was. Sequences clash only on the frames both write. Skip Existing Files leaves earlier frames alone -only when the exporter can check existing files; browser and other memory sinks still report -collisions. A new item's default output name is numbered (`Main.mp4`, +only when the exporter can check the final destination; browser/memory sinks and storage-overflow +renders still report collisions. Preflight checks every configured overflow destination, since free +space can change during a render. A new item's default output name is numbered (`Main.mp4`, `Main_1.mp4`) when another item of the queue writes it. ## Progress and cancellation From 107b00e53888bc8c44ce8bc5b63f32f85fe5b1ac Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 10:07:12 +0100 Subject: [PATCH 6/7] #1743: compare browser output paths using virtual file keys --- crates/engine/src/output_guard.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index fc625c309..4ac67d8cb 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -17,7 +17,9 @@ //! use std::collections::{HashMap, HashSet}; -use std::path::{Component, Path, PathBuf}; +#[cfg(not(target_arch = "wasm32"))] +use std::path::Component; +use std::path::{Path, PathBuf}; use std::sync::Arc; use effectcraft_project::render_queue::{RenderQueueItem, log_path, numbered_template, sequence_frame, sequence_parts, sequence_path}; @@ -181,6 +183,7 @@ fn identity(path: &Path) -> Option { } /// `path` made absolute, with `.` and `..` resolved by name alone (no file is looked at). +#[cfg(not(target_arch = "wasm32"))] fn lexical(path: &Path) -> PathBuf { let absolute = std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()); let mut out = PathBuf::new(); @@ -201,11 +204,24 @@ fn lexical(path: &Path) -> PathBuf { } /// The name a path is compared by: absolute, resolved by name, without case where volumes ignore it. +#[cfg(not(target_arch = "wasm32"))] fn key(path: &Path) -> String { let parts: Vec = lexical(path).components().map(|c| c.as_os_str().to_string_lossy().into_owned()).collect(); fold(&parts.join("/")) } +#[cfg(target_arch = "wasm32")] +fn key(path: &Path) -> String { + browser_key(path) +} + +/// Match the browser file table: one optional leading slash, with literal case and dot segments. +#[cfg(any(target_arch = "wasm32", test))] +fn browser_key(path: &Path) -> String { + let path = path.to_string_lossy(); + if path.starts_with('/') { path.into_owned() } else { format!("/{path}") } +} + fn fold(name: &str) -> String { if IGNORES_CASE { name.to_lowercase() } else { name.to_string() } } @@ -680,6 +696,14 @@ mod tests { job } + #[test] + fn browser_output_names_match_the_virtual_file_table() { + assert_eq!(browser_key(Path::new("renders/a.png")), browser_key(Path::new("/renders/a.png"))); + assert_ne!(browser_key(Path::new("renders/A.png")), browser_key(Path::new("renders/a.png"))); + assert_ne!(browser_key(Path::new("renders/../a.png")), browser_key(Path::new("a.png"))); + assert_ne!(browser_key(Path::new("//a.png")), browser_key(Path::new("/a.png"))); + } + #[test] fn output_plan_path_budget_returns_an_error() { let mut job = Writes::default(); @@ -873,6 +897,7 @@ mod tests { } #[test] + #[cfg(not(target_arch = "wasm32"))] fn paths_compare_by_name_once_resolved() { let here = std::env::current_dir().unwrap(); assert_eq!(key(Path::new("a/b/../c.mp4")), key(Path::new("a/c.mp4"))); From 2112e553ec7e0b723debd7cabcf2d808f950da66 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 10:13:38 +0100 Subject: [PATCH 7/7] #1743: choose default names around protected overflow paths --- crates/engine/src/output_guard.rs | 18 ++++++++++-------- crates/engine/src/tests/render_overwrite.rs | 13 +++++++++++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index 4ac67d8cb..654569558 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -621,18 +621,20 @@ impl Session { let Ok(outputs) = self.outputs_of(item, None) else { break }; let mut in_the_way = false; for (module, path) in &outputs { - match self.check_output(&protected, module, path) { - Ok(Some(_)) => in_the_way = true, - Err(_) => { - item.output.output = template; - return; - } - Ok(None) => {} - } let Ok(paths) = self.write_paths(module, path) else { item.output.output = template; return; }; + for destination in &paths { + match self.check_output(&protected, module, destination) { + Ok(Some(_)) => in_the_way = true, + Err(_) => { + item.output.output = template; + return; + } + Ok(None) => {} + } + } in_the_way |= paths.iter().any(|path| queue.clash(&self.write_of(module, path, false)).is_some()); } if !in_the_way { diff --git a/crates/engine/src/tests/render_overwrite.rs b/crates/engine/src/tests/render_overwrite.rs index d1ca6e65a..6034c15e6 100644 --- a/crates/engine/src/tests/render_overwrite.rs +++ b/crates/engine/src/tests/render_overwrite.rs @@ -695,6 +695,19 @@ fn possible_storage_overflow_destinations_cannot_replace_footage() { assert_eq!(f.read("overflow/Main.mp4"), SOURCE); } +#[test] +fn default_names_avoid_protected_storage_overflow_destinations() { + let mut f = Fixture::new("overflow-protected-default"); + f.comp("Main"); + with_overflow(&mut f); + f.footage("overflow/Main.mp4"); + f.add(json!({"storageOverflow": true})); + assert_eq!(f.output(0), f.path("Main_1.mp4")); + f.render(json!({})).unwrap(); + assert_eq!(f.exported().len(), 1); + assert_eq!(f.read("overflow/Main.mp4"), SOURCE); +} + /// After Effects numbers the second item of one composition (`Comp 1_1.mov`). A default output /// name that another item of the queue already has is numbered the same way. #[test]