engine: atomic writes use an exclusive per-call temporary, so parallel writers cannot mix or follow a planted symlink (#1233)

crates/engine/src/config.rs

@@ -164,10 +164,15 @@ impl ConfigStore for DirConfig {
     }
 }
 
-/// The temporary file an atomic write of `path` goes through.
+/// The temporary file an atomic write of `path` goes through. Unique per call (process id plus a
+/// counter), so two writers saving the same destination never share a temporary — one cannot
+/// truncate, rename or clean up another's — and there is no predictable name for a pre-existing
+/// file or symlink to sit at.
 pub fn temp_path(path: &Path) -> PathBuf {
+    use std::sync::atomic::{AtomicU64, Ordering};
+    static NEXT: AtomicU64 = AtomicU64::new(0);
     let mut name = path.file_name().map(|n| n.to_os_string()).unwrap_or_default();
-    name.push(".tmp");
+    name.push(format!(".{}-{}.tmp", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed)));
     path.with_file_name(name)
 }
 
@@ -177,19 +182,30 @@ pub fn temp_path(path: &Path) -> PathBuf {
 pub fn atomic_write(path: &Path, data: &[u8]) -> std::io::Result<()> {
     atomic_write_with(path, data, |tmp, data| {
         use std::io::Write;
-        let mut f = std::fs::File::create(tmp)?;
+        // Exclusive creation: an entry already at the temporary's name (a leftover from a crashed
+        // writer, or something planted there) is never opened, truncated or written through.
+        let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(tmp)?;
         f.write_all(data)?;
         f.sync_all()
     })
 }
 
-/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write). A
-/// failed write removes the temporary file and leaves `path` untouched.
-pub fn atomic_write_with(path: &Path, data: &[u8], write: impl FnOnce(&Path, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> {
-    let tmp = temp_path(path);
-    if let Err(e) = write(&tmp, data) {
-        let _ = std::fs::remove_file(&tmp);
-        return Err(e);
+/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write). The
+/// temporary belongs to this call alone: the writer must create it exclusively, and a failed
+/// write removes only its own temporary and leaves `path` untouched. An `AlreadyExists` from the
+/// writer means the name was taken (never opened by us): that entry is left alone and another
+/// unique name is tried.
+pub fn atomic_write_with(path: &Path, data: &[u8], mut write: impl FnMut(&Path, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> {
+    for _ in 0..16 {
+        let tmp = temp_path(path);
+        match write(&tmp, data) {
+            Ok(()) => return std::fs::rename(&tmp, path),
+            Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue,
+            Err(e) => {
+                let _ = std::fs::remove_file(&tmp);
+                return Err(e);
+            }
+        }
     }
-    std::fs::rename(&tmp, path)
+    Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, format!("{}: no free temporary name", path.display())))
 }

crates/engine/src/tests_settings.rs

@@ -5,7 +5,7 @@ use std::sync::Arc;
 
 use serde_json::json;
 
-use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with, temp_path};
+use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with};
 use crate::prefs::{PREFS_FILE, Prefs};
 use crate::shortcuts::{DEFAULT_PRESET, Keymaps, normalize};
 use crate::{Session, autosave};
@@ -560,7 +560,9 @@ fn atomic_write_survives_a_crash_mid_write() {
     });
     assert!(r.is_err());
     assert_eq!(std::fs::read(&f).unwrap(), b"version 1");
-    assert!(!temp_path(&f).exists(), "the partial temp file is cleaned up");
+    let leftovers: Vec<String> =
+        std::fs::read_dir(&d).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).filter(|n| n != "p.ecproj").collect();
+    assert!(leftovers.is_empty(), "the partial temporary is cleaned up: {leftovers:?}");
     // A temp file left by a hard kill doesn't count as an auto-save.
     std::fs::write(d.join("p auto-save 1.ecproj.tmp"), b"junk").unwrap();
     assert!(autosave::existing(&d, "p").is_empty());
@@ -569,6 +571,54 @@ fn atomic_write_survives_a_crash_mid_write() {
     let _ = std::fs::remove_dir_all(d);
 }
 
+#[cfg(unix)]
+#[test]
+fn atomic_write_does_not_follow_a_planted_temporary_symlink() {
+    let d = tmp("atomic-link");
+    let f = d.join("p.ecproj");
+    let sentinel = d.join("sentinel.txt");
+    std::fs::write(&sentinel, b"precious").unwrap();
+    // Someone plants a symlink at the temporary name the write used to go through.
+    std::os::unix::fs::symlink(&sentinel, d.join("p.ecproj.tmp")).unwrap();
+    atomic_write(&f, b"new").unwrap();
+    assert_eq!(std::fs::read(&sentinel).unwrap(), b"precious", "a symlink at the temporary name is never followed");
+    assert_eq!(std::fs::read(&f).unwrap(), b"new");
+    let _ = std::fs::remove_dir_all(d);
+}
+
+#[cfg(unix)]
+#[test]
+fn atomic_writes_in_parallel_never_publish_a_mixture() {
+    use std::sync::{Arc, Barrier};
+    let d = tmp("atomic-race");
+    let f = d.join("p.ecproj");
+    let a = vec![b'A'; 64];
+    let b = vec![b'B'; 64];
+    // Writer A writes the first half of its temporary, then waits while writer B runs against
+    // the same destination, and only then finishes.
+    let (half, published) = (Arc::new(Barrier::new(2)), Arc::new(Barrier::new(2)));
+    let (f2, a2) = (f.clone(), a.clone());
+    let (half2, published2) = (half.clone(), published.clone());
+    let t = std::thread::spawn(move || {
+        atomic_write_with(&f2, &a2, |tmp, data| {
+            use std::io::Write;
+            let mut fh = std::fs::File::create(tmp)?;
+            fh.write_all(&data[..32])?;
+            half2.wait();
+            published2.wait();
+            fh.write_all(&data[32..])?;
+            fh.sync_all()
+        })
+    });
+    half.wait();
+    atomic_write(&f, &b).unwrap();
+    published.wait();
+    let _ = t.join();
+    let got = std::fs::read(&f).unwrap();
+    assert!(got == a || got == b, "the destination holds one complete write, never a mixture: {}", String::from_utf8_lossy(&got));
+    let _ = std::fs::remove_dir_all(d);
+}
+
 #[test]
 fn autosave_rotates_through_max_versions() {
     let d = tmp("rotate");