Saves from two threads at once never collide or mix a project file

Fixes #273

crates/engine/src/project.rs

@@ -212,41 +212,37 @@ impl Project {
     }
 }
 
-/// Write `bytes` to `path` atomically: a sibling temp file, fsync, then rename.
-pub fn write_atomic(path: &Path, bytes: &[u8]) -> Result<(), ToolError> {
-    use std::io::Write as _;
-    let dir = path.parent().ok_or_else(|| ToolError::invalid(format!("{} has no parent folder", path.display())))?;
-    std::fs::create_dir_all(dir).map_err(|e| ToolError::io(dir.display(), e))?;
-    let name = path.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default();
-    let tmp = dir.join(format!(".{name}.tmp-{}", std::process::id()));
-    {
-        let mut f = std::fs::File::create(&tmp).map_err(|e| ToolError::io(tmp.display(), e))?;
-        f.write_all(bytes).map_err(|e| ToolError::io(tmp.display(), e))?;
-        f.sync_all().map_err(|e| ToolError::io(tmp.display(), e))?;
-    }
-    // On Windows a file another program has open for a moment (an antivirus scan, the search
-    // indexer) can't be replaced: try again briefly before giving up.
-    let mut tries = 0;
-    loop {
-        match std::fs::rename(&tmp, path) {
-            Ok(()) => return Ok(()),
-            Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied && tries < 20 => {
-                tries += 1;
-                std::thread::sleep(std::time::Duration::from_millis(100));
-            }
-            Err(e) => {
-                let _ = std::fs::remove_file(&tmp);
-                return Err(ToolError::io(path.display(), e));
-            }
-        }
-    }
-}
+/// Write `bytes` to `path` atomically: a temp file of its own, fsync, then rename (retried on
+/// Windows while another program holds the file for a moment).
+pub use craft_anything_platform::fs::write_atomic;
 
 #[cfg(test)]
 mod tests {
     use super::*;
     use craft_anything_evidence::{Environment, EvidenceBuilder, Subject};
 
+    /// #273: threads saving one file at once shared a temp file, so saves failed with "not found"
+    /// and the file could be left half one version, half another.
+    #[test]
+    fn saves_from_several_threads_never_mix() {
+        let dir = tempfile::tempdir().unwrap();
+        let p = dir.path().join("craft.json");
+        let versions: Vec<Vec<u8>> = (0..4u8).map(|i| vec![b'a' + i; 4096 * (usize::from(i) + 1)]).collect();
+        std::thread::scope(|s| {
+            for v in &versions {
+                let (p, versions) = (&p, &versions);
+                s.spawn(move || {
+                    for _ in 0..50 {
+                        write_atomic(p, v).unwrap();
+                        let now = std::fs::read(p).unwrap();
+                        assert!(versions.contains(&now), "a mixed file of {} bytes", now.len());
+                    }
+                });
+            }
+        });
+        assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1, "temp files left behind");
+    }
+
     /// A file another program holds open for a moment (as a virus scan does) is still replaced.
     #[cfg(windows)]
     #[test]

crates/platform/src/fs.rs

@@ -39,7 +39,7 @@ fn write_impl(path: &Path, bytes: &[u8], private: bool) -> Result<(), ToolError>
         f.write_all(bytes).map_err(|e| ToolError::io(tmp.display(), e))?;
         f.sync_all().map_err(|e| ToolError::io(tmp.display(), e))?;
         drop(f);
-        std::fs::rename(&tmp, path).map_err(|e| ToolError::io(path.display(), e))
+        rename_retrying(&tmp, path).map_err(|e| ToolError::io(path.display(), e))
     })();
     if result.is_err() {
         let _ = std::fs::remove_file(&tmp);
@@ -47,6 +47,21 @@ fn write_impl(path: &Path, bytes: &[u8], private: bool) -> Result<(), ToolError>
     result
 }
 
+/// On Windows a file another program has open for a moment (an antivirus scan, the search
+/// indexer) can't be replaced: try again briefly before giving up.
+fn rename_retrying(from: &Path, to: &Path) -> std::io::Result<()> {
+    let mut tries = 0;
+    loop {
+        match std::fs::rename(from, to) {
+            Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied && tries < 20 => {
+                tries += 1;
+                std::thread::sleep(std::time::Duration::from_millis(100));
+            }
+            r => return r,
+        }
+    }
+}
+
 #[cfg(unix)]
 fn set_private(path: &Path) -> Result<(), ToolError> {
     use std::os::unix::fs::PermissionsExt as _;

docs/pitfalls.md

@@ -229,6 +229,10 @@ going), merge what's on disk before saving instead of overwriting it.
 The same happened in `craft.json`: a run saved from the copy it started with and undid a budget
 the person raised meanwhile (and any "always allow" answer). Fields the person owns change only
 through `Workspace::set_person`; `save` keeps them as they are on disk.
+And again inside one process: the engine had its own `write_atomic` whose temp file was named
+by process only, so two threads saving `craft.json` wrote the same temp file and one rename found
+it gone (#273). There is one atomic writer, `craft_anything_platform::fs::write_atomic`; don't
+write another.
 
 **Keywords that only some code paths honour.** "…in Rust" picked Rust for apps but not for games, so
 a headless game rebuild stopped at once for want of Unity. It was found only by a live run. Run the