Autosave: replace a recovery sidecar in one step, so a listing never misses or doubles it (#9034)

On Windows the recovery sidecar was written through store::write_atomic, which replaces the
old file with ReplaceFileW: it moves the old sidecar to a backup name first and only then moves
the new one in. A recovery listing in between saw the bundle without its sidecar and listed the
entry twice, or not at all (so a recovery prompt could miss a document). The flaky
autosave::recovered_entries_survive_a_second_crash_until_saved_or_closed on Windows CI was this.

The sidecar now goes through one AtomicFs on every platform whose replace is a plain rename
(one step); Unix keeps its 0600 temp file, Windows still refuses a read-only sidecar up front.
Recovery data is private to the app, so ReplaceFileW's ACL/stream preservation isn't needed.

Test: a_listing_during_an_autosave_sees_the_entry_exactly_once lists the recovery folder while
an autosaver rewrites the entry 200 times. With the old code it failed 3 of 3 runs on Windows
(two entries, once none); with the fix it passes 3 of 3.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

crates/format/src/autosave.rs

@@ -179,38 +179,52 @@ fn worker(rx: Receiver<Job>, bundle: PathBuf, sidecar: PathBuf, last: Arc<Mutex<
     }
 }
 
+/// Publish a recovery sidecar atomically: a flushed temporary file renamed over the old one.
+///
+/// The replacing step is a plain rename on every platform, never `ReplaceFileW` (what
+/// [`crate::atomic::RealFs`] uses on Windows to keep a user file's ACL and named streams). That
+/// call first moves the old file to a backup name and only then moves the new one in, so for a
+/// moment `<key>.json` does not exist: a listing in between took the entry for a sidecar-less
+/// bundle and listed it twice, or not at all (#9034). A rename swaps the name in one step, and
+/// recovery data is private to the app, so there is no metadata to keep.
 fn write_sidecar(path: &Path, bytes: &[u8]) -> Result<()> {
-    #[cfg(unix)]
-    {
-        use std::os::unix::fs::OpenOptionsExt;
-        struct PrivateFs;
-        impl crate::atomic::AtomicFs for PrivateFs {
-            fn write_new(&self, tmp: &Path, bytes: &[u8]) -> std::io::Result<()> {
+    struct SidecarFs;
+    impl crate::atomic::AtomicFs for SidecarFs {
+        // As before: Windows refuses a read-only sidecar up front, Unix replaces it.
+        #[cfg(not(unix))]
+        fn is_readonly(&self, path: &Path) -> bool {
+            crate::atomic::RealFs.is_readonly(path)
+        }
+        fn write_new(&self, tmp: &Path, bytes: &[u8]) -> std::io::Result<()> {
+            #[cfg(unix)]
+            {
                 use std::io::Write;
+                use std::os::unix::fs::OpenOptionsExt;
                 let mut f = std::fs::OpenOptions::new().write(true).create_new(true).mode(0o600).open(tmp)?;
                 f.write_all(bytes)?;
                 crate::atomic::sync_file(&f)
             }
-            fn rename(&self, from: &Path, to: &Path) -> std::io::Result<()> {
-                crate::atomic::RealFs.rename(from, to)
-            }
-            fn remove(&self, path: &Path) -> std::io::Result<()> {
-                crate::atomic::RealFs.remove(path)
-            }
-            fn sync_dir(&self, dir: &Path) -> std::io::Result<()> {
-                crate::atomic::RealFs.sync_dir(dir)
-            }
-            fn copy_metadata(&self, from: &Path, to: &Path) -> std::io::Result<()> {
-                crate::atomic::RealFs.copy_metadata(from, to)
+            #[cfg(not(unix))]
+            {
+                crate::atomic::RealFs.write_new(tmp, bytes)
             }
         }
-        crate::atomic::atomic_write_with(&PrivateFs, crate::atomic::RenameRetry::platform(), path, bytes)?;
-        Ok(())
-    }
-    #[cfg(not(unix))]
-    {
-        crate::store::write_atomic(path, bytes)
+        fn rename(&self, from: &Path, to: &Path) -> std::io::Result<()> {
+            crate::atomic::RealFs.rename(from, to)
+        }
+        // `replace_existing` keeps the trait's default: this same one-step rename.
+        fn remove(&self, path: &Path) -> std::io::Result<()> {
+            crate::atomic::RealFs.remove(path)
+        }
+        fn sync_dir(&self, dir: &Path) -> std::io::Result<()> {
+            crate::atomic::RealFs.sync_dir(dir)
+        }
+        fn copy_metadata(&self, from: &Path, to: &Path) -> std::io::Result<()> {
+            crate::atomic::RealFs.copy_metadata(from, to)
+        }
     }
+    crate::atomic::atomic_write_with(&SidecarFs, crate::atomic::RenameRetry::platform(), path, bytes)?;
+    Ok(())
 }
 
 fn read_sidecar_bounded(path: &Path, max: usize) -> Option<Vec<u8>> {

crates/format/tests/it/autosave.rs

@@ -304,3 +304,41 @@ fn failed_cleanup_after_commit_does_not_fail_the_save() {
     assert_eq!(load_path(&dir).unwrap(), doc);
     std::fs::remove_dir_all(dir.parent().unwrap()).unwrap();
 }
+
+/// #9034: while a document's autosave replaces its entry, a listing (another launch's recovery
+/// prompt, or the launch's own check after closing a second document) sees that entry exactly
+/// once. On Windows the sidecar used to be replaced through `ReplaceFileW`, which moves the old
+/// sidecar to a backup name before moving the new one in: a listing in between found the bundle
+/// without a sidecar and listed it a second time (or, the other way round, not at all).
+#[test]
+fn a_listing_during_an_autosave_sees_the_entry_exactly_once() {
+    let dir = temp_dir("listing-during-autosave");
+    let doc = Document::new("Busy", photocraft_doc::Size { width: 4, height: 4 }, ColorMode::Rgb, SampleType::U8);
+    save_once(&dir, "busy", doc.clone());
+    let done = Arc::new(std::sync::atomic::AtomicBool::new(false));
+    let writer = {
+        let (dir, done) = (dir.clone(), done.clone());
+        std::thread::spawn(move || {
+            let saver = Autosaver::new(&dir, "busy");
+            for revision in 2..=200 {
+                saver.request(Arc::new(doc.clone()), revision, None, SaveOptions::default());
+                // Wait for this write, so every revision replaces the sidecar once.
+                while saver.take_completed().is_empty() {
+                    std::thread::yield_now();
+                }
+            }
+            done.store(true, std::sync::atomic::Ordering::SeqCst);
+            saver.flush()
+        })
+    };
+    let mut listings = 0u32;
+    while !done.load(std::sync::atomic::Ordering::SeqCst) {
+        let entries = list_recovery(&dir);
+        let keys: Vec<&str> = entries.iter().map(|e| e.info.key.as_str()).collect();
+        assert_eq!(keys, ["busy"], "listing {listings} during an autosave: {entries:?}");
+        listings += 1;
+    }
+    assert!(writer.join().unwrap().is_some_and(|r| r.is_ok()));
+    assert!(listings > 0);
+    std::fs::remove_dir_all(dir).unwrap();
+}