From c080783c00a1863d8a0effd594d93651b7e97e78 Mon Sep 17 00:00:00 2001 From: David Mann Date: Sat, 24 Oct 2026 03:48:25 +0200 Subject: [PATCH] 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) --- crates/format/src/autosave.rs | 60 ++++++++++++++++++------------ crates/format/tests/it/autosave.rs | 38 +++++++++++++++++++ 2 files changed, 75 insertions(+), 23 deletions(-) diff --git a/crates/format/src/autosave.rs b/crates/format/src/autosave.rs index cb09e27cb8..49f5af61a9 100644 --- a/crates/format/src/autosave.rs +++ b/crates/format/src/autosave.rs @@ -179,38 +179,52 @@ fn worker(rx: Receiver, bundle: PathBuf, sidecar: PathBuf, last: Arc.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> { diff --git a/crates/format/tests/it/autosave.rs b/crates/format/tests/it/autosave.rs index 24d61d50d3..f47c4c60d2 100644 --- a/crates/format/tests/it/autosave.rs +++ b/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(); +}