From c2a9a6e526390808ef195759056fda0b9a55d8a3 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Fri, 11 Sep 2026 02:44:17 +0200 Subject: [PATCH] Windows atomic replace waits out a transient reader A concurrent reader of the store's destination file (another process opening it, an indexer, an antivirus scan) makes MoveFileExW fail with a sharing or access error for as long as it holds the handle. That is transient by construction; treating the first refusal as the outcome killed Muninn serve on 2026-09-11 the moment a status CLI overlapped a write. The replace retries such errors for up to half a second. The test holds the destination open from another thread and expects the replace to outlast it. Co-Authored-By: Claude Fable 5.1 --- packages/cultcache-rs/src/lib.rs | 84 ++++++++++++++++++++++++++------ 1 file changed, 69 insertions(+), 15 deletions(-) diff --git a/packages/cultcache-rs/src/lib.rs b/packages/cultcache-rs/src/lib.rs index ba7088e5..36953620 100644 --- a/packages/cultcache-rs/src/lib.rs +++ b/packages/cultcache-rs/src/lib.rs @@ -2662,23 +2662,77 @@ fn replace_file_atomically(staged: &Path, destination: &Path) -> Result<()> { .encode_wide() .chain(std::iter::once(0)) .collect::>(); - let success = unsafe { - MoveFileExW( - staged_wide.as_ptr(), - destination_wide.as_ptr(), - MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH, - ) - }; - if success == 0 { - return Err(std::io::Error::last_os_error()).with_context(|| { - format!( - "failed to atomically replace {} with {}", - destination.display(), - staged.display() + // A concurrent reader of the destination (another process opening the + // store, an indexer, an antivirus scan) makes the replace fail with a + // sharing or access error for as long as it holds the handle. That is + // transient by construction; a daemon that treats it as fatal dies on + // the first overlap (Muninn serve did, 2026-09-11). Retry briefly. + const ERROR_ACCESS_DENIED: i32 = 5; + const ERROR_SHARING_VIOLATION: i32 = 32; + const ERROR_LOCK_VIOLATION: i32 = 33; + const ATTEMPTS: u32 = 25; + const BACKOFF: std::time::Duration = std::time::Duration::from_millis(20); + let mut attempt = 0; + loop { + let success = unsafe { + MoveFileExW( + staged_wide.as_ptr(), + destination_wide.as_ptr(), + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH, ) - }); + }; + if success != 0 { + return Ok(()); + } + let error = std::io::Error::last_os_error(); + attempt += 1; + let transient = matches!( + error.raw_os_error(), + Some(ERROR_ACCESS_DENIED | ERROR_SHARING_VIOLATION | ERROR_LOCK_VIOLATION) + ); + if !transient || attempt >= ATTEMPTS { + return Err(error).with_context(|| { + format!( + "failed to atomically replace {} with {} after {attempt} attempt(s)", + destination.display(), + staged.display() + ) + }); + } + std::thread::sleep(BACKOFF); + } +} + +#[cfg(all(test, windows))] +mod windows_replace_tests { + use super::replace_file_atomically; + use std::os::windows::fs::OpenOptionsExt; + + /// A reader holding the destination open with no sharing makes the + /// replace fail for as long as it holds on; it must succeed once the + /// reader lets go, not report the first refusal as the outcome. + #[test] + fn a_replace_waits_out_a_reader_that_holds_the_destination() { + let dir = std::env::temp_dir().join(format!("cultcache-replace-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let destination = dir.join("store.cc"); + let staged = dir.join("store.cc.tmp"); + std::fs::write(&destination, b"old").unwrap(); + std::fs::write(&staged, b"new").unwrap(); + let held = std::fs::OpenOptions::new() + .read(true) + .share_mode(0) + .open(&destination) + .unwrap(); + let releaser = std::thread::spawn(move || { + std::thread::sleep(std::time::Duration::from_millis(120)); + drop(held); + }); + replace_file_atomically(&staged, &destination).expect("the replace outlasts the reader"); + releaser.join().unwrap(); + assert_eq!(std::fs::read(&destination).unwrap(), b"new"); + let _ = std::fs::remove_dir_all(&dir); } - Ok(()) } #[cfg(unix)]