G2: --save runs on one project take turns, so no edit is lost (#2851)

A run reads the project, edits it and writes the whole file back, so two
at once silently dropped the edits of whichever saved first. A run that
saves over the file it opened (--save, or --save-as the same file) now
holds an OS lock on `.<name>.lock` next to the project from before its
read until it exits. The next run says so on stderr, waits, then edits
what the first one saved. Ported from filmcraft-cli
(storytold/filmcraft#3926).

The OS drops the lock however the process ends, so a crash leaves nothing
stale. If the lock can't be taken the run warns and saves as it did
before. A missing project stops the run before it opens anything, with
the same error opening gives, so a project created in between is never
saved without the lock. The desktop app and `mcp` don't take this lock.

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<String> = (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", &params, "--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 <subcommand>: ` 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]