diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index 6ba352247..551ff8362 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -47,6 +47,7 @@ use std::io::Write; +mod project_lock; mod template; use effectcraft_automation::tools::{self, Reply}; @@ -508,6 +509,7 @@ fn script_cmd(args: &Args, json_out: bool) -> Result<(), Failure> { (None, Some(f)) => (std::fs::read_to_string(f).map_err(|e| Failure::Error(format!("cannot read {f}: {e}")))?, f.clone()), (None, None) => return usage_err("script needs a .jsx/.js file or --eval CODE"), }; + let _lock = project_lock::for_save(args)?; let mut b = backend(args, false)?; let r = b.exec("script.run", json!({"code": code, "name": name}))?; let saved = maybe_save(&mut b, args)?; @@ -611,6 +613,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { (Some(p), _) | (None, Some(p)) => serde_json::from_str(p).map_err(|e| Failure::Usage(format!("params are not valid JSON: {e}")))?, (None, None) => json!({}), }; + let _lock = project_lock::for_save(args)?; let mut b = backend(args, true)?; let r = b.exec(&id, params)?; if let Some(e) = failed_render(&id, &r) { @@ -654,6 +657,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { if steps.is_empty() { return Err(Failure::Usage(format!("--commands {}: no commands", sources.join(", ")))); } + let _lock = project_lock::for_save(args)?; let mut b = backend(args, true)?; let mut results = vec![]; // The step's index closes the message: a generated list repeats ids, and callers match @@ -708,6 +712,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { } let (comp, layer, path) = (args.pos[0].clone(), args.pos[1].clone(), args.pos[2].clone()); let value = args.pos.get(3).map(|v| value_arg(v)); + let _lock = project_lock::for_save(args)?; let mut b = backend(args, true)?; let a = json!({"comp": reference(&comp), "layer": reference(&layer), "path": path, "value": value, "time": args.num("--time")?, "expression": expr}); diff --git a/apps/effectcraft-cli/src/project_lock.rs b/apps/effectcraft-cli/src/project_lock.rs new file mode 100644 index 000000000..234bbfd33 --- /dev/null +++ b/apps/effectcraft-cli/src/project_lock.rs @@ -0,0 +1,150 @@ +//! One saving run at a time per project file. +//! +//! A `--save` run reads the project, edits it and writes the whole file back, so two at once would +//! silently drop the edits of whichever saved first (#2851). Each run holds an OS lock from before +//! its read until it exits; the next one waits, then reads what the first one saved. This is how +//! filmcraft-cli does it (storytold/filmcraft#3926). +//! +//! The lock is on a file next to the project, not on the project: a save replaces the project's +//! inode, and on Windows a locked file can't be read by other processes. The OS drops the lock when +//! the process ends, however it ends, so a crash leaves nothing stale. The file itself stays, since +//! removing it would let one run wait on the old file while another locks a new one. + +use crate::{Args, Failure}; +use std::fs::{File, TryLockError}; +use std::io::Write; +use std::path::{Path, PathBuf}; + +/// Most bytes of the project's file name kept in the lock's name; the rest of the 255-byte limit +/// is left for the leading dot and `.lock`. +const MAX_NAME_BYTES: usize = 200; + +fn lock_path(project: &Path) -> PathBuf { + let name = project.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); + project.with_file_name(format!(".{}.lock", &name[..name.floor_char_boundary(MAX_NAME_BYTES)])) +} + +/// Do two paths name one existing file? +fn same_file(a: &str, b: &str) -> bool { + a == b || matches!((std::fs::canonicalize(a), std::fs::canonicalize(b)), (Ok(a), Ok(b)) if a == b) +} + +/// The path this run saves over, when that is the file it opens. `--save-as` somewhere else replaces +/// that file whatever it held, so there is no edit of its to lose. +/// +/// The lock goes by this path as written, not by where a symlink points: runs that name the +/// project the same way share one lock. +/// +/// A missing project is an error here, before anything else, the same one opening it would give. +/// If the run went on, another one could create the project before it is read, and this run would +/// edit and save it with no lock. Checking first also means the paths compared below exist, so +/// `canonicalize` tells exactly whether they are one file, however they are spelt. A project that +/// exists but isn't a file gets no lock file beside it, and opening it reports why. +fn saved_in_place(a: &Args) -> Result, String> { + // `--bridge` with a project is a usage error, which `backend` reports + let Some(project) = a.project.as_deref().filter(|_| a.opt("--bridge").is_none()) else { return Ok(None) }; + match Path::new(project).metadata() { + Ok(meta) if meta.is_file() => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Err(format!("cannot read {project}: {e}")), + _ => return Ok(None), + } + match a.opt("--save-as") { + Some(target) if same_file(target, project) => Ok(Some(Path::new(target))), + None if a.flag("--save") => Ok(Some(Path::new(project))), + _ => Ok(None), + } +} + +/// Lock `project`, calling `waiting` first if another run holds the lock. +fn lock(project: &Path, waiting: impl FnOnce()) -> std::io::Result { + let file = File::options().create(true).truncate(false).read(true).write(true).open(lock_path(project))?; + match file.try_lock() { + Ok(()) => {} + Err(TryLockError::WouldBlock) => { + waiting(); + file.lock()?; + } + Err(TryLockError::Error(e)) => return Err(e), + } + Ok(file) +} + +/// Take the project's lock if this run saves over the file it opens, or fail if the project is +/// missing. Call it before opening the project and keep the result until the save is done. Blocks +/// while another run holds the lock, which is fine here: nothing else is running yet. A lock that +/// can't be had (a read-only folder, a file system without locks) is a warning and not an error, so +/// that saving works where it did. +pub(crate) fn for_save(a: &Args) -> Result, Failure> { + let Some(project) = saved_in_place(a).map_err(Failure::Error)? else { return Ok(None) }; + let shown = project.display(); + let waiting = || { + let _ = writeln!(std::io::stderr(), "effectcraft-cli: waiting for another run saving {shown}"); + }; + match lock(project, waiting) { + Ok(file) => Ok(Some(file)), + Err(e) => { + let _ = writeln!(std::io::stderr(), "effectcraft-cli: warning: can't lock {shown} ({e}); another run saving it now could overwrite these edits"); + Ok(None) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn args(list: &[&str]) -> Args { + Args::parse(list.iter().map(|s| s.to_string()).collect()).unwrap() + } + + #[test] + fn the_lock_sits_next_to_the_project() { + assert_eq!(lock_path(Path::new("/a/b/p.ecproj")), Path::new("/a/b/.p.ecproj.lock")); + assert_eq!(lock_path(Path::new("p.ecproj")), Path::new(".p.ecproj.lock")); + } + + #[test] + fn a_long_project_name_still_gets_a_lock_name_the_file_system_takes() { + for name in ["a".repeat(255), "é".repeat(127)] { + let lock = lock_path(Path::new(&name)); + let lock = lock.to_str().unwrap(); + assert!(lock.len() <= 255, "{} bytes", lock.len()); + assert!(lock.ends_with(".lock")); + } + } + + /// The moment between this check and opening the project can't be hit on demand, so this pins + /// the decision that closes it: a run never goes on to open a project that wasn't there, however + /// its paths are spelt and whatever it saves. + #[test] + fn a_missing_project_stops_the_run_before_anything_opens_it() { + let name = format!("ec-cli-gone-{}.ecproj", std::process::id()); + let gone = std::env::temp_dir().join(&name); + let gone = gone.to_str().unwrap(); + let alias = std::env::temp_dir().join(".").join(&name); + let alias = alias.to_str().unwrap(); + let dotted = format!("./{name}"); + // in a folder that doesn't exist either, so no part of either spelling can be resolved + let new_dir = std::env::temp_dir().join(format!("ec-cli-new-dir-{}", std::process::id())); + let in_new_dir = new_dir.join("p.ecproj"); + let in_new_dir = in_new_dir.to_str().unwrap(); + let in_new_dir_alias = new_dir.join(".").join("p.ecproj"); + let in_new_dir_alias = in_new_dir_alias.to_str().unwrap(); + for (project, list) in [ + (gone, &["exec", "--project", gone, "--save"][..]), + (gone, &["exec", gone, "--save"]), + (gone, &["exec", "--project", gone, "--save-as", gone]), + (gone, &["exec", "--project", gone, "--save-as", alias]), + (name.as_str(), &["exec", "--project", &name, "--save-as", &dotted]), + (in_new_dir, &["exec", "--project", in_new_dir, "--save-as", in_new_dir_alias]), + (gone, &["exec", "--project", gone, "--save-as", "other.ecproj"]), + (gone, &["exec", "--project", gone]), + ] { + let error = saved_in_place(&args(list)).unwrap_err(); + assert_eq!(error.split_once(": ").map(|(head, _)| head), Some(format!("cannot read {project}").as_str()), "{list:?}: {error}"); + } + // a project with --bridge is a usage error, which `backend` reports; no project, no lock + assert_eq!(saved_in_place(&args(&["exec", "--bridge", "9877", "--project", gone, "--save"])), Ok(None)); + assert_eq!(saved_in_place(&args(&["exec", "--demo", "--save-as", gone])), Ok(None)); + } +} diff --git a/apps/effectcraft-cli/tests/it/cli.rs b/apps/effectcraft-cli/tests/it/cli.rs index e9a96dd7e..0623f37fe 100644 --- a/apps/effectcraft-cli/tests/it/cli.rs +++ b/apps/effectcraft-cli/tests/it/cli.rs @@ -110,6 +110,167 @@ fn exec_set_get_with_saved_project() { assert_eq!(v[1]["result"]["time"], v[0]["result"]["time"], "{v}"); } +/// A project in a new folder of its own, for the save lock tests (#2851). +fn lock_test_project(tag: &str) -> (PathBuf, String) { + let dir = tmp(&format!("lock-{tag}")); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let proj = dir.join("p.ecproj").to_str().unwrap().to_string(); + ok_json(&["exec", "project.newFolder", r#"{"name":"start"}"#, "--empty", "--save-as", &proj]); + (dir, proj) +} + +/// The lock of the project file `name`, held by this process. +fn hold_lock(dir: &std::path::Path, name: &str) -> std::fs::File { + let lock = std::fs::File::options().create(true).truncate(false).read(true).write(true).open(dir.join(format!(".{name}.lock"))).unwrap(); + lock.lock().unwrap(); + lock +} + +/// Start a run that has to wait for a lock this process holds; returns once it says it is waiting. +fn start_waiting_run(args: &[&str]) -> std::process::Child { + let mut child = bin().args(args).stdout(Stdio::null()).stderr(Stdio::piped()).spawn().unwrap(); + let mut stderr = BufReader::new(child.stderr.take().unwrap()); + let mut line = String::new(); + while !line.contains("waiting for another run saving") { + line.clear(); + assert_ne!(stderr.read_line(&mut line).unwrap(), 0, "{args:?} ended without waiting"); + } + assert!(child.try_wait().unwrap().is_none(), "{args:?} did not wait"); + child +} + +/// Folder names in a saved project. +fn has_folder(proj: &str, name: &str) -> bool { + std::fs::read_to_string(proj).unwrap().contains(&format!("\"{name}\"")) +} + +#[test] +fn parallel_saves_keep_every_edit() { + let (dir, proj) = lock_test_project("parallel"); + let names = |writer: usize| (0..300).map(move |i| format!("edit-{writer}-{i}")); + let mut writers: Vec<_> = (0..4) + .map(|_| { + bin() + .args(["run", "--project", &proj, "--commands", "-", "--save"]) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .unwrap() + }) + .collect(); + // Each writer reads its whole script before it opens the project, so closing the stdins together + // starts all the edits at once, and long scripts keep each read-to-save span over the others. + for (n, writer) in writers.iter_mut().enumerate() { + let script: String = names(n).map(|name| format!("{}\n", json!({"command": "project.newFolder", "params": {"name": name}}))).collect(); + writer.stdin.take().unwrap().write_all(script.as_bytes()).unwrap(); + } + for mut writer in writers { + assert!(writer.wait().unwrap().success()); + } + let lost: Vec = (0..4).flat_map(names).filter(|name| !has_folder(&proj, name)).collect(); + assert!(lost.is_empty(), "lost {} edits: {lost:?}", lost.len()); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn a_save_in_place_waits_for_the_lock_then_edits_the_latest_project() { + let (dir, proj) = lock_test_project("wait"); + for save in [&["--save"][..], &["--save-as", proj.as_str()]] { + let held = hold_lock(&dir, "p.ecproj"); + let before = std::fs::read(&proj).unwrap(); + let params = json!({"name": format!("waited-{}", save.len())}).to_string(); + let args = [&["exec", "project.newFolder", ¶ms, "--project", &proj][..], save].concat(); + let mut child = start_waiting_run(&args); + assert_eq!(std::fs::read(&proj).unwrap(), before, "{save:?} wrote while another run held the lock"); + // an edit made meanwhile by the holder must survive: the waiter reads only after the lock is free + let held_params = json!({"name": format!("held-{}", save.len())}).to_string(); + ok_json(&["exec", "project.newFolder", &held_params, "--project", &proj, "--save-as", &format!("{proj}.tmp")]); + std::fs::rename(format!("{proj}.tmp"), &proj).unwrap(); + drop(held); + assert!(child.wait().unwrap().success(), "{save:?}"); + for want in [format!("waited-{}", save.len()), format!("held-{}", save.len())] { + assert!(has_folder(&proj, &want), "{save:?} lost {want}"); + } + } + let _ = std::fs::remove_dir_all(&dir); +} + +/// Runs through a symlink take the same lock before and after a save, or a run still waiting on +/// the old one could overwrite a later run's save. +#[cfg(unix)] +#[test] +fn a_symlinked_project_keeps_its_lock_after_a_save() { + let (dir, proj) = lock_test_project("symlink"); + let link = dir.join("link.ecproj"); + std::os::unix::fs::symlink(&proj, &link).unwrap(); + for name in [r#"{"name":"first"}"#, r#"{"name":"second"}"#] { + let held = hold_lock(&dir, "link.ecproj"); + let mut child = start_waiting_run(&["exec", "project.newFolder", name, "--project", link.to_str().unwrap(), "--save"]); + drop(held); + assert!(child.wait().unwrap().success(), "{name}"); + } + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn runs_that_do_not_save_in_place_ignore_the_lock() { + let (dir, proj) = lock_test_project("ignore"); + let other = dir.join("other.ecproj").to_str().unwrap().to_string(); + let _held = hold_lock(&dir, "p.ecproj"); + let params = r#"{"name":"copy-only"}"#; + let cases: [&[&str]; 3] = [ + &["info", "--project", &proj], + &["exec", "project.newFolder", params, "--project", &proj], + &["exec", "project.newFolder", params, "--project", &proj, "--save-as", &other], + ]; + for args in cases { + let mut child = bin().args(args).stdout(Stdio::null()).stderr(Stdio::null()).spawn().unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(60); + let status = loop { + if let Some(status) = child.try_wait().unwrap() { + break status; + } + if std::time::Instant::now() > deadline { + let _ = child.kill(); + panic!("{args:?} is waiting for a lock it doesn't need"); + } + std::thread::sleep(std::time::Duration::from_millis(10)); + }; + assert!(status.success(), "{args:?}"); + } + assert!(!has_folder(&proj, "copy-only")); + assert!(has_folder(&other, "copy-only")); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn a_missing_project_gets_no_lock_file_and_keeps_its_error() { + let (dir, _) = lock_test_project("missing"); + let gone = dir.join("gone.ecproj").to_str().unwrap().to_string(); + let other = dir.join("other.ecproj").to_str().unwrap().to_string(); + // what opening it says, after the `effectcraft-cli : ` prefix + let error = |args: &[&str]| { + let out = bin().args(args).output().unwrap(); + assert_eq!(out.status.code(), Some(1), "{args:?}"); + let stderr = String::from_utf8_lossy(&out.stderr).into_owned(); + stderr.split_once(": ").map(|(_, e)| e.to_string()).unwrap_or(stderr) + }; + let plain = error(&["info", "--project", &gone]); + assert!(plain.contains("gone.ecproj"), "{plain}"); + // saving in place, saving elsewhere, or not saving: the error is the one opening gives + for save in [&["--save"][..], &["--save-as", gone.as_str()], &["--save-as", other.as_str()], &[]] { + let args = [&["exec", "project.newFolder", r#"{"name":"copy-only"}"#, "--project", &gone][..], save].concat(); + assert_eq!(error(&args), plain, "{save:?}"); + } + assert!(!dir.join(".gone.ecproj.lock").exists()); + assert!(!dir.join("other.ecproj").exists()); + // a project with --bridge stays a usage error + assert_eq!(bin().args(["exec", "project.newFolder", "--bridge", "9877", "--project", &gone, "--save"]).output().unwrap().status.code(), Some(2)); + let _ = std::fs::remove_dir_all(&dir); +} + /// `run --args-file` reads its command / params pairs from a file, one per line: a batch longer /// than a Windows command line (32 767 characters, os error 206) still runs. #[test] diff --git a/docs/agents.md b/docs/agents.md index 34162aacd..38ed16dca 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -751,6 +751,14 @@ effectcraft-cli exec file.importLottie '{"path":"anim.json"}' main.ecproj --save effectcraft-cli render main.ecproj --comp Main --out main.mov --format prores --progress # progress lines on stderr for a program that reads them ``` +Runs that save over the project they opened (`--save`, or `--save-as` the same file) take turns, so +parallel scripts don't lose edits. Each holds an OS lock on `..lock` next to the project from +before it reads the file until it exits; the next one says so on stderr, waits, and then edits what +the first one saved. A crash releases the lock, and the empty file left behind is harmless. Only these +CLI runs take the lock: the desktop app, `mcp`, and a `file.save` step in a run without `--save` +don't. OS locks can also be unreliable over network shares and synced folders. If the lock can't be +taken, the run warns on stderr and saves anyway. + A long `run` doesn't fit on a command line (Windows caps it at 32 767 characters, Linux one argument at 128 KiB). `run --args-file F` reads the same ` ` arguments one per line; `run --commands FILE` reads steps as a JSON array of `{"command": ID, "params": {…}}` objects, or one