atomic save: preserve mode, fsync the dir, retry temp collisions (F-006)

Audit F-006. `save_atomic` wrote a temp sibling, synced it, and renamed
over the target — but dropped three durability/correctness properties:

1. Mode not preserved: the temp was created with default perms, so saving
   over an existing file replaced its mode — a 0755 script silently
   dropped to 0644. Now snapshot the target's permissions and apply them
   to the temp before the rename (new files still get the default).
2. Parent dir not fsync'd: the file bytes were synced but the rename (a
   directory operation) wasn't durable, so a crash right after rename
   could lose it. Now fsync the parent directory after rename on Unix,
   best-effort (the rename already succeeded; some FSes reject dir fsync).
3. Temp-name collision failed the save: the pid+subsec-nanos name relied
   on `create_new` erroring, with no retry, so a stale temp from a crashed
   run (recurring pid+nanos) surfaced as a spurious save failure. Now a
   process-global atomic sequence makes same-process names unique, and the
   open retries a bounded number of times on collision.

Tests: 0755 mode survives a save (unix); temp names disambiguate by
sequence; 50 back-to-back saves never collide.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014TXbAwk27agwhrNNrhLi2U

src/file_io.rs

@@ -21,8 +21,18 @@ use std::ffi::OsString;
 use std::fs::{self, File, OpenOptions};
 use std::io::{self, Read, Write};
 use std::path::{Path, PathBuf};
+use std::sync::atomic::{AtomicU64, Ordering};
 use std::time::SystemTime;
 
+/// Attempts to find a free temp-file name before giving up (F-006). The
+/// atomic sequence makes same-process names unique, so retries only cover
+/// the rare stale-temp-from-a-crashed-run collision.
+const MAX_TEMP_ATTEMPTS: u32 = 8;
+
+/// Process-global disambiguator for temp names — guarantees two saves in
+/// the same process never collide, even within one nanosecond (F-006).
+static TEMP_SEQ: AtomicU64 = AtomicU64::new(0);
+
 // ---------------------------------------------------------------------------
 // File metadata
 // ---------------------------------------------------------------------------
@@ -129,31 +139,74 @@ pub fn save_atomic(path: &Path, content: &[u8]) -> Result<FileMeta, SaveError> {
     if path.parent().is_none() {
         return Err(SaveError::NoParent(path.to_path_buf()));
     }
-    let tmp_path = temp_sibling(path);
+
+    // Snapshot the target's current permissions so an existing file keeps
+    // its mode across the replace (F-006) — e.g. a `0755` script stays
+    // executable. `None` for a new file, which then gets the default mode.
+    let existing_perms = fs::metadata(path).ok().map(|m| m.permissions());
+
+    // Open a fresh temp, retrying on the rare name collision (a stale temp
+    // left by a crashed prior run whose pid+nanos recurs) instead of
+    // failing the save (F-006). `TEMP_SEQ` makes same-process names unique.
+    let (mut tmp, tmp_path) = {
+        let mut opened = None;
+        for _ in 0..MAX_TEMP_ATTEMPTS {
+            let seq = TEMP_SEQ.fetch_add(1, Ordering::Relaxed);
+            let candidate = temp_sibling(path, seq);
+            match OpenOptions::new()
+                .write(true)
+                .create_new(true)
+                .open(&candidate)
+            {
+                Ok(file) => {
+                    opened = Some((file, candidate));
+                    break;
+                }
+                // Name taken (a stale temp): fall through to the next seq.
+                Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {}
+                Err(e) => return Err(SaveError::Io(e)),
+            }
+        }
+        opened.ok_or_else(|| {
+            SaveError::Io(io::Error::new(
+                io::ErrorKind::AlreadyExists,
+                "save_atomic: temp name still colliding after retries",
+            ))
+        })?
+    };
 
     let mut guard = TempCleanup {
         tmp: Some(tmp_path.clone()),
     };
 
-    {
-        let mut tmp = OpenOptions::new()
-            .write(true)
-            .create_new(true)
-            .open(&tmp_path)
-            .map_err(SaveError::Io)?;
-        tmp.write_all(content).map_err(SaveError::Io)?;
-        tmp.sync_all().map_err(SaveError::Io)?;
+    // Carry the target's mode onto the temp so the saved file preserves it
+    // (F-006). Done *before* the write so a sensitive (e.g. 0600) file's
+    // content is never briefly world-readable in a default-perms temp; the
+    // already-open write handle keeps write access regardless of the new
+    // mode (Unix checks permissions at open, not per write).
+    if let Some(perms) = existing_perms {
+        fs::set_permissions(&tmp_path, perms).map_err(SaveError::Io)?;
     }
+    tmp.write_all(content).map_err(SaveError::Io)?;
+    tmp.sync_all().map_err(SaveError::Io)?;
+    drop(tmp); // close before rename (Windows can't rename an open file)
 
     fs::rename(&tmp_path, path).map_err(SaveError::Io)?;
     // Rename succeeded: temp no longer exists at tmp_path; defuse cleanup.
     guard.tmp = None;
 
+    // fsync the parent directory so the rename (a directory operation) is
+    // durable across a crash, not just the file bytes `sync_all` covered
+    // (F-006). Best-effort: the rename already succeeded, and some
+    // filesystems reject directory fsync. Directory fsync is a Unix concept.
+    #[cfg(unix)]
+    sync_parent_dir(path);
+
     let meta = current_meta(path)?;
     Ok(meta)
 }
 
-fn temp_sibling(target: &Path) -> PathBuf {
+fn temp_sibling(target: &Path, seq: u64) -> PathBuf {
     // `parent()` may be empty (target is a bare filename in cwd) or
     // non-empty (target lives under some directory). `Path::join` handles
     // both correctly: empty parent + name == name; non-empty parent + name
@@ -165,15 +218,34 @@ fn temp_sibling(target: &Path) -> PathBuf {
     name.push(".");
     name.push(format!("{:x}", std::process::id()));
     name.push(".");
-    // A coarse per-call disambiguator. Collisions are vanishingly unlikely
-    // and `create_new` would error if hit.
     let nanos = SystemTime::now()
         .duration_since(SystemTime::UNIX_EPOCH)
         .map_or(0, |d| d.subsec_nanos());
-    name.push(format!("{nanos:x}"));
+    // pid + subsecond nanos + a process-global sequence: the sequence
+    // guarantees same-process uniqueness, the rest disambiguates across
+    // processes/runs.
+    name.push(format!("{nanos:x}.{seq:x}"));
     parent.join(name)
 }
 
+/// fsync the directory containing `path` so a rename into it is durable
+/// (F-006). Best-effort — errors are ignored (see the call site).
+#[cfg(unix)]
+fn sync_parent_dir(path: &Path) {
+    let Some(parent) = path.parent() else {
+        return;
+    };
+    // An empty parent means the current directory.
+    let dir = if parent.as_os_str().is_empty() {
+        Path::new(".")
+    } else {
+        parent
+    };
+    if let Ok(dir) = File::open(dir) {
+        let _ = dir.sync_all();
+    }
+}
+
 // ---------------------------------------------------------------------------
 // Tests
 // ---------------------------------------------------------------------------
@@ -228,6 +300,43 @@ mod tests {
         assert_eq!(bytes, content);
     }
 
+    #[cfg(unix)]
+    #[test]
+    fn save_preserves_existing_file_mode() {
+        // F-006 — an atomic save over an existing file keeps its mode, so
+        // a `0755` script stays executable instead of dropping to `0644`.
+        use std::os::unix::fs::PermissionsExt;
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("script.sh");
+        save_atomic(&path, b"#!/bin/sh\necho hi\n").unwrap();
+        fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).unwrap();
+
+        save_atomic(&path, b"#!/bin/sh\necho bye\n").unwrap();
+        let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777;
+        assert_eq!(mode, 0o755, "the executable bit must survive the save");
+    }
+
+    #[test]
+    fn temp_sibling_disambiguates_by_sequence() {
+        // F-006 — the process-global sequence makes temp names unique even
+        // for the same target within one nanosecond.
+        let p = Path::new("/tmp/foo.txt");
+        assert_ne!(temp_sibling(p, 1), temp_sibling(p, 2));
+    }
+
+    #[test]
+    fn rapid_saves_in_one_process_do_not_collide() {
+        // F-006 — back-to-back saves must never spuriously fail on a temp
+        // name collision (the sequence guarantees uniqueness).
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("hot.txt");
+        for i in 0..50u32 {
+            save_atomic(&path, format!("write {i}").as_bytes()).unwrap();
+        }
+        let (bytes, _) = load_file(&path).unwrap();
+        assert_eq!(bytes, b"write 49");
+    }
+
     #[test]
     fn save_overwrites_existing() {
         let dir = TempDir::new().unwrap();