diff --git a/crates/engine/src/project.rs b/crates/engine/src/project.rs index 67537b9e..8ca7ec43 100644 --- a/crates/engine/src/project.rs +++ b/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> = (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] diff --git a/crates/platform/src/fs.rs b/crates/platform/src/fs.rs index 26e6bb61..af7d4add 100644 --- a/crates/platform/src/fs.rs +++ b/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 _; diff --git a/docs/pitfalls.md b/docs/pitfalls.md index 60947cb7..8edb488f 100644 --- a/docs/pitfalls.md +++ b/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