diff --git a/apps/photocraft/src/services.rs b/apps/photocraft/src/services.rs index d74dcfa74d..ffb7db3c50 100644 --- a/apps/photocraft/src/services.rs +++ b/apps/photocraft/src/services.rs @@ -67,14 +67,11 @@ fn recovery_dir() -> Option { config_dir().map(|d| d.join("Recovery")) } -/// Write `bytes` atomically (temp file + rename) so a crash never leaves half a preferences file. +/// Write `bytes` crash-safely (temp file beside the target, fsync, rename, directory fsync; see +/// [`photocraft_format::atomic`]). Every document write (Save, Save As, Export, Save for Web) and +/// the preferences go through here, so a failed or interrupted save never destroys the old file. fn write_atomic(path: &Path, bytes: &[u8]) -> Result<(), String> { - if let Some(dir) = path.parent() { - std::fs::create_dir_all(dir).map_err(|e| e.to_string())?; - } - let tmp = path.with_extension("tmp"); - std::fs::write(&tmp, bytes).map_err(|e| e.to_string())?; - std::fs::rename(&tmp, path).map_err(|e| e.to_string()) + photocraft_format::atomic_write(path, bytes).map_err(|e| e.to_string()) } pub fn native(automation: Option) -> Services { @@ -123,7 +120,7 @@ pub fn native(automation: Option) -> } Some(d.save_file()?.to_string_lossy().to_string()) })), - write: Some(Box::new(|path: &str, bytes: &[u8]| std::fs::write(path, bytes).map_err(|e| e.to_string()))), + write: Some(Box::new(|path: &str, bytes: &[u8]| write_atomic(Path::new(path), bytes))), automation_read, automation_write, automation_command, diff --git a/crates/automation/src/files.rs b/crates/automation/src/files.rs index 424da5ccd9..16788982e9 100644 --- a/crates/automation/src/files.rs +++ b/crates/automation/src/files.rs @@ -61,10 +61,8 @@ pub fn save( return Ok(Vec::new()); } let r = photocraft_io::export(doc, &ext, opts)?; - if let Some(parent) = path.parent().filter(|p| !p.as_os_str().is_empty()) { - std::fs::create_dir_all(parent).map_err(|e| AutomationError::Io(e.to_string()))?; - } - std::fs::write(path, &r.bytes).map_err(|e| AutomationError::Io(format!("{}: {e}", path.display())))?; + // Crash-safe: a failed write never destroys the previous file. + photocraft_format::atomic_write(path, &r.bytes).map_err(|e| AutomationError::Io(e.to_string()))?; Ok(r.warnings) } diff --git a/crates/automation/src/headless.rs b/crates/automation/src/headless.rs index 995529deba..1c5e3baed2 100644 --- a/crates/automation/src/headless.rs +++ b/crates/automation/src/headless.rs @@ -127,7 +127,7 @@ impl Headless { let requested = path.to_str().ok_or_else(|| AutomationError::BadRequest("automation paths must be valid UTF-8".into()))?; match &self.filesystem { Filesystem::Denied => Err(AutomationError::BadRequest("automation filesystem access is not granted: write authority is absent".into())), - Filesystem::TrustedLocal => std::fs::write(path, bytes).map_err(|error| AutomationError::Io(format!("{}: {error}", path.display()))), + Filesystem::TrustedLocal => photocraft_format::atomic_write(path, bytes).map_err(|error| AutomationError::Io(error.to_string())), Filesystem::Workspace(workspace) => workspace.write(requested, bytes), } } diff --git a/crates/automation/src/workspace.rs b/crates/automation/src/workspace.rs index 7ca2b38f2e..c03f5f04c9 100644 --- a/crates/automation/src/workspace.rs +++ b/crates/automation/src/workspace.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use cap_std::ambient_authority; use cap_std::fs::{Dir, OpenOptions}; +use photocraft_format::atomic::RenameRetry; use serde_json::Value; use crate::AutomationError; @@ -50,7 +51,10 @@ impl AuthorizedWorkspace { Ok(bytes) } - /// Create or replace one file below the configured write root. + /// Create or replace one file below the configured write root, crash-safely: the bytes go to + /// a temporary file beside the target, which is synced and renamed over it (the same steps + /// as [`photocraft_format::atomic_write`], through the directory capability). On failure + /// the previous file is untouched and the temporary file is removed. /// /// The parent directory must already exist. `cap-std` performs path /// resolution and file creation relative to the held directory handle, so @@ -58,11 +62,29 @@ impl AuthorizedWorkspace { pub fn write(&self, path: &str, bytes: &[u8]) -> Result<(), AutomationError> { let relative = relative_path(path)?; let root = self.write.as_ref().ok_or_else(|| AutomationError::BadRequest(format!("{DENIED}: write authority is absent")))?; + let leaf = relative.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); + let parent = relative.parent().map(Path::to_path_buf).unwrap_or_default(); + let tmp = parent.join(photocraft_format::atomic::temp_name(&leaf)); let mut options = OpenOptions::new(); - options.write(true).create(true).truncate(true); - let mut file = root.dir.open_with(&relative, &options).map_err(|e| file_error("write", path, e))?; - file.write_all(bytes).map_err(|e| file_error("write", path, e))?; - file.flush().map_err(|e| file_error("write", path, e)) + options.write(true).create_new(true); + let written = root.dir.open_with(&tmp, &options).and_then(|mut file| { + file.write_all(bytes)?; + file.sync_all() + }); + let renamed = written.and_then(|()| photocraft_format::atomic::retry_rename(RenameRetry::platform(), || root.dir.rename(&tmp, &root.dir, &relative))); + if let Err(e) = renamed { + let _ = root.dir.remove_file(&tmp); + return Err(file_error("write", path, e)); + } + // Flush the directory entry (Unix); best effort, the new bytes are already in place. + #[cfg(unix)] + { + let dir = if parent.as_os_str().is_empty() { PathBuf::from(".") } else { parent }; + if let Ok(d) = root.dir.open(&dir) { + let _ = d.sync_all(); + } + } + Ok(()) } } @@ -256,6 +278,43 @@ mod tests { assert_eq!(std::fs::read(inside.join("new-output.txt")).unwrap(), b"created"); } + fn temp_files(dir: &Path) -> Vec { + std::fs::read_dir(dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().into_owned()).filter(|n| n.ends_with(".tmp")).collect() + } + + #[test] + fn write_replaces_atomically_and_leaves_no_temp_file() { + let (inside, _, workspace) = roots("atomic"); + std::fs::create_dir_all(inside.join("sub")).unwrap(); + workspace.write("sub/out.psd", b"original").unwrap(); + workspace.write("sub/out.psd", b"replacement").unwrap(); + assert_eq!(std::fs::read(inside.join("sub/out.psd")).unwrap(), b"replacement"); + assert!(temp_files(&inside.join("sub")).is_empty()); + // Renaming over a directory fails: nothing is left behind and the directory survives. + std::fs::create_dir_all(inside.join("adir")).unwrap(); + assert!(workspace.write("adir", b"x").is_err()); + assert!(inside.join("adir").is_dir()); + assert!(temp_files(&inside).is_empty()); + } + + #[cfg(unix)] + #[test] + fn write_into_read_only_folder_keeps_the_original() { + use std::os::unix::fs::PermissionsExt; + let (inside, _, workspace) = roots("readonly"); + let ro = inside.join("ro"); + std::fs::create_dir_all(&ro).unwrap(); + std::fs::write(ro.join("doc.psd"), b"original").unwrap(); + std::fs::set_permissions(&ro, std::fs::Permissions::from_mode(0o555)).unwrap(); + let root_user = std::fs::File::create(ro.join("probe")).is_ok(); + if !root_user { + assert!(workspace.write("ro/doc.psd", b"new").is_err()); + assert_eq!(std::fs::read(ro.join("doc.psd")).unwrap(), b"original"); + assert!(temp_files(&ro).is_empty()); + } + std::fs::set_permissions(&ro, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + #[test] fn rejects_absolute_traversal_mixed_prefix_and_malformed_paths_without_panicking() { let (_, _, workspace) = roots("reject"); diff --git a/crates/engine/src/automate_cmds.rs b/crates/engine/src/automate_cmds.rs index b15c9ed17c..1a862d4f79 100644 --- a/crates/engine/src/automate_cmds.rs +++ b/crates/engine/src/automate_cmds.rs @@ -307,7 +307,7 @@ fn write_shim(droplet: &str) -> Result { "#!/bin/sh\n# PhotoCraft droplet: runs the action on the files given (or dropped).\nexec \"${{PHOTOCRAFT_CLI:-photocraft-cli}}\" droplet \"{}\" \"$@\"\n", abs.replace('"', "\\\"") ); - std::fs::write(&shim, body).map_err(|e| EngineError::Other(format!("{shim}: {e}")))?; + crate::file_cmds::write_file(&shim, body.as_bytes())?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; diff --git a/crates/engine/src/file_cmds.rs b/crates/engine/src/file_cmds.rs index e0cf8caa56..674619b326 100644 --- a/crates/engine/src/file_cmds.rs +++ b/crates/engine/src/file_cmds.rs @@ -84,11 +84,10 @@ pub(crate) fn read_file(path: &str) -> Result> { } #[cfg(not(target_arch = "wasm32"))] +/// Every file the engine writes goes through here: crash-safe (temp file + fsync + rename, see +/// [`photocraft_format::atomic`]), so a failed save never destroys the previous file. pub(crate) fn write_file(path: &str, bytes: &[u8]) -> Result<()> { - if let Some(dir) = std::path::Path::new(path).parent().filter(|d| !d.as_os_str().is_empty()) { - std::fs::create_dir_all(dir).map_err(|e| EngineError::Other(format!("{}: {e}", dir.display())))?; - } - std::fs::write(path, bytes).map_err(|e| EngineError::Other(format!("{path}: {e}"))) + photocraft_format::atomic_write(std::path::Path::new(path), bytes).map_err(|e| EngineError::Other(e.to_string())) } #[cfg(target_arch = "wasm32")] pub(crate) fn write_file(path: &str, _bytes: &[u8]) -> Result<()> { diff --git a/crates/engine/src/layer_menu_cmds.rs b/crates/engine/src/layer_menu_cmds.rs index 20869913e0..d3276f3b79 100644 --- a/crates/engine/src/layer_menu_cmds.rs +++ b/crates/engine/src/layer_menu_cmds.rs @@ -709,7 +709,7 @@ fn export_layer(s: &mut Session, p: &Value, cmd: &str, png_only: bool) -> Result #[cfg(not(target_arch = "wasm32"))] fn write_file(path: &str, bytes: &[u8]) -> Result<()> { - std::fs::write(path, bytes).map_err(|e| other(format!("{path}: {e}"))) + crate::file_cmds::write_file(path, bytes) } #[cfg(target_arch = "wasm32")] diff --git a/crates/engine/src/pattern_cmds.rs b/crates/engine/src/pattern_cmds.rs index 1149889361..30c86cda53 100644 --- a/crates/engine/src/pattern_cmds.rs +++ b/crates/engine/src/pattern_cmds.rs @@ -288,7 +288,7 @@ fn export(s: &mut Session, p: &Value) -> Result { None => s.patterns.items.clone(), }; let bytes = photocraft_io::pattern_map::write_pat(&pats).map_err(EngineError::Other)?; - std::fs::write(path, &bytes).map_err(|e| EngineError::Other(format!("{path}: {e}")))?; + crate::file_cmds::write_file(path, &bytes)?; Ok(json!({"path": path, "count": pats.len(), "bytes": bytes.len()})) } diff --git a/crates/engine/src/preset_store.rs b/crates/engine/src/preset_store.rs index 272cb34f2f..d3bd3c0206 100644 --- a/crates/engine/src/preset_store.rs +++ b/crates/engine/src/preset_store.rs @@ -183,13 +183,7 @@ impl PresetBackend for DirBackend { let path = self.path(name)?; let dir = path.parent().ok_or_else(|| format!("{name}: no parent directory"))?; std::fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?; - let leaf = path.file_name().map(|f| f.to_string_lossy().to_string()).unwrap_or_default(); - let tmp = dir.join(format!(".{leaf}.tmp")); - std::fs::write(&tmp, bytes).map_err(|e| format!("{name}: {e}"))?; - std::fs::rename(&tmp, &path).map_err(|e| { - let _ = std::fs::remove_file(&tmp); - format!("{name}: {e}") - }) + photocraft_format::atomic_write(&path, bytes).map_err(|e| format!("{name}: {e}")) } fn remove(&self, name: &str) -> Result<(), String> { match std::fs::remove_file(self.path(name)?) { diff --git a/crates/engine/src/smart_cmds.rs b/crates/engine/src/smart_cmds.rs index 97ee923982..a09184081e 100644 --- a/crates/engine/src/smart_cmds.rs +++ b/crates/engine/src/smart_cmds.rs @@ -605,7 +605,7 @@ fn export_contents(s: &mut Session, p: &Value) -> Result { let id = layer_param(s, p)?; let st = s.active().ok_or(EngineError::NoDocument)?; let (name, bytes) = source_bytes(&st.doc.metadata, &smart(&st.doc, id)?.source).ok_or_else(|| other("the smart object's contents are unavailable"))?; - std::fs::write(path, &*bytes).map_err(|e| other(format!("can't write {path}: {e}")))?; + crate::file_cmds::write_file(path, &bytes)?; Ok(json!({"path": path, "fileName": name, "bytes": bytes.len()})) } @@ -874,7 +874,7 @@ pub fn specs() -> Vec { let path = path_param("layer.smartObjects.convertToLinked", p)?.to_string(); set_source(s, p, "Convert to Linked", false, |meta, src| { let (_, bytes) = source_bytes(meta, src).ok_or_else(|| other("the smart object's contents are unavailable"))?; - std::fs::write(&path, &*bytes).map_err(|e| other(format!("can't write {path}: {e}")))?; + crate::file_cmds::write_file(&path, &bytes)?; Ok(SmartSource::Linked { path }) }) } diff --git a/crates/engine/src/variables_cmds.rs b/crates/engine/src/variables_cmds.rs index 71d5ae6067..f1cc252163 100644 --- a/crates/engine/src/variables_cmds.rs +++ b/crates/engine/src/variables_cmds.rs @@ -353,7 +353,7 @@ fn export_as_files(s: &mut Session, p: &Value) -> Result { let path = format!("{dir}/{stem}.{format}"); let opts = photocraft_io::ExportOptions::default(); let bytes = photocraft_io::export(&doc, format, &opts).map(|r| r.bytes).map_err(|e| EngineError::Other(format!("export `{}`: {e}", set.name)))?; - std::fs::write(&path, &bytes).map_err(|e| EngineError::Other(format!("write `{path}`: {e}")))?; + crate::file_cmds::write_file(&path, &bytes)?; files.push(path); } Ok(json!({"files": files, "count": files.len()})) diff --git a/crates/engine/src/video_cmds.rs b/crates/engine/src/video_cmds.rs index 43bbe22084..cefe37ca39 100644 --- a/crates/engine/src/video_cmds.rs +++ b/crates/engine/src/video_cmds.rs @@ -326,7 +326,7 @@ fn render_video(s: &mut Session, p: &Value) -> Result { } let gif = photocraft_codecs::web::encode_gif_animated(w as u32, h as u32, &gframes, true).map_err(|e| EngineError::Other(e.to_string()))?; let path = format!("{dir}/{stem}.gif"); - std::fs::write(&path, &gif).map_err(|e| EngineError::Other(format!("write `{path}`: {e}")))?; + crate::file_cmds::write_file(&path, &gif)?; return Ok(json!({"frames": frames, "file": path})); } @@ -341,7 +341,7 @@ fn render_video(s: &mut Session, p: &Value) -> Result { sync(&mut doc); let path = format!("{dir}/{stem}_{f:04}.{format}"); let bytes = photocraft_io::export(&doc, format, &opts).map(|r| r.bytes).map_err(|e| EngineError::Other(format!("render frame {f}: {e}")))?; - std::fs::write(&path, &bytes).map_err(|e| EngineError::Other(format!("write `{path}`: {e}")))?; + crate::file_cmds::write_file(&path, &bytes)?; files.push(path); } Ok(json!({"frames": files.len(), "dir": dir})) diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs new file mode 100644 index 0000000000..c1c5a8d2b8 --- /dev/null +++ b/crates/format/src/atomic.rs @@ -0,0 +1,221 @@ +//! Crash-safe file replacement: the one way PhotoCraft writes a document to disk. +//! +//! [`atomic_write`] never truncates the destination. It writes a temporary file in the **same +//! directory**, flushes it to stable storage (`sync_all`), renames it over the destination and, +//! on Unix, fsyncs the directory so the rename itself survives a power loss. If anything fails, +//! the destination keeps its previous bytes and the temporary file is removed. +//! +//! - **Windows:** antivirus scanners and the search indexer briefly hold files open, which makes +//! the replacing rename fail with `PermissionDenied`. The rename is retried with exponential +//! backoff before giving up. (`std::fs::rename` uses `MoveFileExW(MOVEFILE_REPLACE_EXISTING)`, +//! i.e. replace semantics.) +//! - **Cross-device (`EXDEV`):** cannot happen while the temporary file sits beside the target; +//! if it does anyway, the save fails with a clear error. It never falls back to writing the +//! destination in place. +//! - **Unwritable folder:** the save fails with a clear error instead of writing in place. +//! +//! On the web there is no file system: writes go through the platform services (downloads), so +//! this module is never reached there; on `wasm32` every call returns the `std` "unsupported" +//! error. +//! +//! [`atomic_write_with`] takes an [`AtomicFs`] so tests can inject failures at each step. + +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +/// The file-system steps [`atomic_write_with`] performs. [`RealFs`] is the real thing; tests +/// wrap it to inject failures. +pub trait AtomicFs { + /// Create `tmp` (it must not exist), write all of `bytes` and `sync_all` it. + fn write_new(&self, tmp: &Path, bytes: &[u8]) -> io::Result<()>; + /// Rename `from` over `to`, replacing it. + fn rename(&self, from: &Path, to: &Path) -> io::Result<()>; + /// Remove a (temporary) file. + fn remove(&self, path: &Path) -> io::Result<()>; + /// Flush a directory entry change (Unix); a no-op elsewhere. + fn sync_dir(&self, dir: &Path) -> io::Result<()>; + /// Copy the destination's permissions onto the temporary file, so replacing a file keeps its + /// mode. Best effort. + fn copy_permissions(&self, from: &Path, to: &Path) -> io::Result<()>; +} + +/// The real file system. +#[derive(Debug, Clone, Copy, Default)] +pub struct RealFs; + +impl AtomicFs for RealFs { + fn write_new(&self, tmp: &Path, bytes: &[u8]) -> io::Result<()> { + let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(tmp)?; + f.write_all(bytes)?; + f.sync_all() + } + fn rename(&self, from: &Path, to: &Path) -> io::Result<()> { + std::fs::rename(from, to) + } + fn remove(&self, path: &Path) -> io::Result<()> { + std::fs::remove_file(path) + } + fn sync_dir(&self, dir: &Path) -> io::Result<()> { + #[cfg(unix)] + { + std::fs::File::open(dir)?.sync_all() + } + #[cfg(not(unix))] + { + let _ = dir; + Ok(()) + } + } + fn copy_permissions(&self, from: &Path, to: &Path) -> io::Result<()> { + let perms = std::fs::metadata(from)?.permissions(); + std::fs::set_permissions(to, perms) + } +} + +/// How hard to retry a rename that fails with `PermissionDenied`. +#[derive(Debug, Clone, Copy)] +pub struct RenameRetry { + /// Retries after the first attempt (0 = no retry). + pub attempts: u32, + /// Delay before the first retry; doubles each time. + pub first_delay_ms: u64, +} + +impl RenameRetry { + /// The platform default: retry on Windows (about 1.3 s in total), never elsewhere, where + /// `PermissionDenied` is a real permission problem that waiting won't fix. + pub fn platform() -> Self { + if cfg!(windows) { Self { attempts: 7, first_delay_ms: 10 } } else { Self { attempts: 0, first_delay_ms: 0 } } + } +} + +/// Atomically replace (or create) `path` with `bytes`. See the module docs. +/// +/// Errors keep their [`io::ErrorKind`] and say what happened and that the original file is +/// unchanged. +pub fn atomic_write(path: &Path, bytes: &[u8]) -> io::Result<()> { + atomic_write_with(&RealFs, RenameRetry::platform(), path, bytes) +} + +/// [`atomic_write`] through an explicit file system and retry policy (the failure-injection seam). +pub fn atomic_write_with(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, bytes: &[u8]) -> io::Result<()> { + let target = resolve_symlink(path); + let name = target.file_name().ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, format!("{}: not a file path", path.display())))?; + let dir = match target.parent() { + Some(d) if !d.as_os_str().is_empty() => d.to_path_buf(), + _ => PathBuf::from("."), + }; + std::fs::create_dir_all(&dir).map_err(|e| context(e, format!("cannot create the folder {}", dir.display())))?; + + // Create a fresh temporary file next to the target (same directory, so the rename never + // crosses a device). The name is unique per process and call; retry a few on collisions. + let mut tmp = None; + for _ in 0..8 { + let candidate = dir.join(temp_name(&name.to_string_lossy())); + match fs.write_new(&candidate, bytes) { + Ok(()) => { + tmp = Some(candidate); + break; + } + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => { + // A partial temp file may exist; the destination was never touched. + let _ = fs.remove(&candidate); + return Err(write_error(e, path, &dir)); + } + } + } + let tmp = tmp.ok_or_else(|| io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: could not create a temporary file", dir.display())))?; + + // Keep the existing file's mode (e.g. 0600) instead of the process default. + if target.exists() { + let _ = fs.copy_permissions(&target, &tmp); + } + + if let Err(e) = retry_rename(retry, || fs.rename(&tmp, &target)) { + let _ = fs.remove(&tmp); + return Err(rename_error(e, path)); + } + // The new bytes are in place; a failed directory fsync can't be undone and must not report + // the save as failed, so it is best effort. + let _ = fs.sync_dir(&dir); + Ok(()) +} + +/// Run `rename`, retrying with backoff while it fails with `PermissionDenied` (see +/// [`RenameRetry`]). For callers that rename through their own handles (capability directories). +pub fn retry_rename(retry: RenameRetry, mut rename: impl FnMut() -> io::Result<()>) -> io::Result<()> { + let mut delay = retry.first_delay_ms; + let mut left = retry.attempts; + loop { + match rename() { + Err(e) if e.kind() == io::ErrorKind::PermissionDenied && left > 0 => { + left -= 1; + sleep_ms(delay); + delay = delay.saturating_mul(2); + } + r => return r, + } + } +} + +#[cfg(not(target_arch = "wasm32"))] +fn sleep_ms(ms: u64) { + std::thread::sleep(std::time::Duration::from_millis(ms)); +} +#[cfg(target_arch = "wasm32")] +fn sleep_ms(_ms: u64) {} + +/// Write through a symlink to the file it points at, instead of replacing the link. +fn resolve_symlink(path: &Path) -> PathBuf { + match std::fs::symlink_metadata(path) { + Ok(m) if m.file_type().is_symlink() => std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()), + _ => path.to_path_buf(), + } +} + +/// A unique hidden temporary file name to write beside `file` before renaming over it. +pub fn temp_name(file: &str) -> String { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + // Keep the name short: a long file name plus the suffix must stay under NAME_MAX. + let stem: String = file.chars().take(64).collect(); + format!(".{stem}.{}-{n}.pcsave.tmp", std::process::id()) +} + +fn context(e: io::Error, msg: String) -> io::Error { + io::Error::new(e.kind(), format!("{msg}: {e}")) +} + +const UNCHANGED: &str = "the original file was not changed"; + +fn write_error(e: io::Error, path: &Path, dir: &Path) -> io::Error { + let msg = match e.kind() { + io::ErrorKind::PermissionDenied | io::ErrorKind::ReadOnlyFilesystem => { + format!("cannot save {}: the folder {} is not writable ({e}); {UNCHANGED}", path.display(), dir.display()) + } + io::ErrorKind::StorageFull => format!("cannot save {}: the disk is full ({e}); {UNCHANGED}", path.display()), + _ => format!("cannot save {}: {e}; {UNCHANGED}", path.display()), + }; + io::Error::new(e.kind(), msg) +} + +fn rename_error(e: io::Error, path: &Path) -> io::Error { + let msg = if is_cross_device(&e) { + format!("cannot save {}: the temporary file is on a different volume than the destination ({e}); {UNCHANGED}", path.display()) + } else if e.kind() == io::ErrorKind::PermissionDenied { + format!("cannot save {}: the file is locked or read-only ({e}); {UNCHANGED}", path.display()) + } else { + format!("cannot save {}: {e}; {UNCHANGED}", path.display()) + }; + io::Error::new(e.kind(), msg) +} + +fn is_cross_device(e: &io::Error) -> bool { + // EXDEV is 18 on Linux, macOS and the BSDs; ERROR_NOT_SAME_DEVICE is 17 on Windows. + e.kind() == io::ErrorKind::CrossesDevices || (cfg!(unix) && e.raw_os_error() == Some(18)) || (cfg!(windows) && e.raw_os_error() == Some(17)) +} + +#[cfg(test)] +mod tests; diff --git a/crates/format/src/atomic/tests.rs b/crates/format/src/atomic/tests.rs new file mode 100644 index 0000000000..be89dbe137 --- /dev/null +++ b/crates/format/src/atomic/tests.rs @@ -0,0 +1,212 @@ +use super::*; +use std::cell::Cell; + +struct TempDir(PathBuf); +impl TempDir { + fn new(tag: &str) -> Self { + static N: AtomicU64 = AtomicU64::new(0); + let d = std::env::temp_dir().join(format!("pc-atomic-{tag}-{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed))); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + Self(d) + } +} +impl Drop for TempDir { + fn drop(&mut self) { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(&self.0, std::fs::Permissions::from_mode(0o755)); + } + let _ = std::fs::remove_dir_all(&self.0); + } +} + +/// Only the target remains: no temporary file was left behind. +fn assert_no_temp(dir: &Path) { + let leftovers: Vec<_> = + std::fs::read_dir(dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().into_owned()).filter(|n| n.ends_with(".tmp")).collect(); + assert!(leftovers.is_empty(), "temporary files left behind: {leftovers:?}"); +} + +const ORIGINAL: &[u8] = b"the user's precious original document"; + +/// Real file system with failures injected at chosen steps. +#[derive(Default)] +struct Faulty { + /// Write this many bytes, then fail like a full disk. + fail_write_after: Option, + /// Fail every rename with this kind. + fail_rename: Option, + /// Fail the first N renames with PermissionDenied (a transient lock), then succeed. + locked_renames: Cell, + renames: Cell, +} + +impl AtomicFs for Faulty { + fn write_new(&self, tmp: &Path, bytes: &[u8]) -> io::Result<()> { + if let Some(n) = self.fail_write_after { + let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(tmp)?; + f.write_all(&bytes[..n.min(bytes.len())])?; + return Err(io::Error::new(io::ErrorKind::StorageFull, "injected: no space left on device")); + } + RealFs.write_new(tmp, bytes) + } + fn rename(&self, from: &Path, to: &Path) -> io::Result<()> { + self.renames.set(self.renames.get() + 1); + if let Some(kind) = self.fail_rename { + return Err(io::Error::new(kind, "injected rename failure")); + } + if self.locked_renames.get() > 0 { + self.locked_renames.set(self.locked_renames.get() - 1); + return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: sharing violation")); + } + RealFs.rename(from, to) + } + fn remove(&self, path: &Path) -> io::Result<()> { + RealFs.remove(path) + } + fn sync_dir(&self, dir: &Path) -> io::Result<()> { + RealFs.sync_dir(dir) + } + fn copy_permissions(&self, from: &Path, to: &Path) -> io::Result<()> { + RealFs.copy_permissions(from, to) + } +} + +const NO_RETRY: RenameRetry = RenameRetry { attempts: 0, first_delay_ms: 0 }; + +#[test] +fn replaces_and_creates() { + let d = TempDir::new("ok"); + let p = d.0.join("doc.psd"); + atomic_write(&p, b"first").unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"first"); + atomic_write(&p, b"second, longer").unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"second, longer"); + assert_no_temp(&d.0); +} + +#[test] +fn creates_missing_parent_folders() { + let d = TempDir::new("parent"); + let p = d.0.join("a/b/out.png"); + atomic_write(&p, b"x").unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"x"); +} + +#[test] +fn write_failure_halfway_keeps_original() { + let d = TempDir::new("write"); + let p = d.0.join("doc.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = Faulty { fail_write_after: Some(5), ..Default::default() }; + let e = atomic_write_with(&fs, NO_RETRY, &p, &[7u8; 4096]).unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::StorageFull); + assert!(e.to_string().contains("not changed"), "{e}"); + assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); + assert_eq!(fs.renames.get(), 0, "never renames after a failed write"); + assert_no_temp(&d.0); +} + +#[test] +fn rename_failure_keeps_original() { + let d = TempDir::new("rename"); + let p = d.0.join("doc.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = Faulty { fail_rename: Some(io::ErrorKind::Other), ..Default::default() }; + assert!(atomic_write_with(&fs, NO_RETRY, &p, b"new").is_err()); + assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); + assert_no_temp(&d.0); +} + +#[test] +fn cross_device_rename_errors_clearly_and_never_writes_in_place() { + let d = TempDir::new("exdev"); + let p = d.0.join("doc.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = Faulty { fail_rename: Some(io::ErrorKind::CrossesDevices), ..Default::default() }; + let e = atomic_write_with(&fs, NO_RETRY, &p, b"new").unwrap_err(); + assert!(e.to_string().contains("different volume"), "{e}"); + assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); + assert_no_temp(&d.0); +} + +#[test] +fn locked_target_rename_is_retried() { + let d = TempDir::new("locked"); + let p = d.0.join("doc.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = Faulty { locked_renames: Cell::new(2), ..Default::default() }; + atomic_write_with(&fs, RenameRetry { attempts: 3, first_delay_ms: 1 }, &p, b"new").unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"new"); + assert_eq!(fs.renames.get(), 3); + assert_no_temp(&d.0); +} + +#[test] +fn lock_that_outlasts_the_retries_keeps_original() { + let d = TempDir::new("locked2"); + let p = d.0.join("doc.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = Faulty { locked_renames: Cell::new(10), ..Default::default() }; + let e = atomic_write_with(&fs, RenameRetry { attempts: 2, first_delay_ms: 1 }, &p, b"new").unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::PermissionDenied); + assert!(e.to_string().contains("locked"), "{e}"); + assert_eq!(fs.renames.get(), 3); + assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); + assert_no_temp(&d.0); +} + +#[cfg(unix)] +#[test] +fn read_only_folder_errors_clearly_and_never_writes_in_place() { + use std::os::unix::fs::PermissionsExt; + let d = TempDir::new("ro"); + let p = d.0.join("doc.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + // The file itself stays writable: an in-place fallback would succeed and must not happen. + std::fs::set_permissions(&d.0, std::fs::Permissions::from_mode(0o555)).unwrap(); + if std::fs::File::create(d.0.join("probe")).is_ok() { + return; // running as root: permissions aren't enforced + } + let e = atomic_write(&p, b"new").unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::PermissionDenied); + assert!(e.to_string().contains("not writable"), "{e}"); + assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); + std::fs::set_permissions(&d.0, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert_no_temp(&d.0); +} + +#[cfg(unix)] +#[test] +fn keeps_the_original_file_mode() { + use std::os::unix::fs::PermissionsExt; + let d = TempDir::new("mode"); + let p = d.0.join("doc.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o600)).unwrap(); + atomic_write(&p, b"new").unwrap(); + assert_eq!(std::fs::metadata(&p).unwrap().permissions().mode() & 0o777, 0o600); +} + +#[cfg(unix)] +#[test] +fn writes_through_a_symlink() { + let d = TempDir::new("link"); + let real = d.0.join("real.psd"); + let link = d.0.join("link.psd"); + std::fs::write(&real, ORIGINAL).unwrap(); + std::os::unix::fs::symlink(&real, &link).unwrap(); + atomic_write(&link, b"new").unwrap(); + assert!(std::fs::symlink_metadata(&link).unwrap().file_type().is_symlink()); + assert_eq!(std::fs::read(&real).unwrap(), b"new"); +} + +#[test] +fn directory_target_or_bare_root_is_an_error_not_a_panic() { + let d = TempDir::new("dir"); + assert!(atomic_write(&d.0, b"x").is_err()); + assert!(atomic_write(Path::new("/"), b"x").is_err()); + assert!(atomic_write(Path::new(""), b"x").is_err()); +} diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 8fd60f401b..5a40f32994 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -20,6 +20,7 @@ #![forbid(unsafe_code)] #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] +pub mod atomic; pub mod autosave; mod convert; pub mod manifest; @@ -32,6 +33,7 @@ use std::path::Path; use photocraft_doc::Document; use photocraft_raster::Rgba8Image; +pub use atomic::atomic_write; pub use autosave::{Autosaver, RecoveryEntry, discard_recovery, list_recovery, recover}; pub use manifest::{FORMAT_VERSION, Manifest}; pub use store::{PcraftWriter, SaveStats}; diff --git a/crates/format/src/store.rs b/crates/format/src/store.rs index d0e19a60c9..22b9e4b82a 100644 --- a/crates/format/src/store.rs +++ b/crates/format/src/store.rs @@ -448,12 +448,8 @@ fn list_objects(dir: &Path) -> Result> { Ok(out) } +/// Crash-safe replace of one file (see [`crate::atomic`]). pub(crate) fn write_atomic(path: &Path, data: &[u8]) -> Result<()> { - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent)?; - } - let tmp = path.with_extension(format!("{}tmp", path.extension().map(|e| format!("{}.", e.to_string_lossy())).unwrap_or_default())); - std::fs::write(&tmp, data)?; - std::fs::rename(&tmp, path)?; + crate::atomic::atomic_write(path, data)?; Ok(()) } diff --git a/crates/ui-egui/examples/snapshot.rs b/crates/ui-egui/examples/snapshot.rs index bfd4460872..d759c7c688 100644 --- a/crates/ui-egui/examples/snapshot.rs +++ b/crates/ui-egui/examples/snapshot.rs @@ -36,7 +36,7 @@ fn main() { } photocraft_io::export(doc, path, &opts).map(|r| (r.bytes, r.warnings)).map_err(|e| e.to_string()) })), - write: Some(Box::new(|path: &str, bytes: &[u8]| std::fs::write(path, bytes).map_err(|e| e.to_string()))), + write: Some(Box::new(|path: &str, bytes: &[u8]| photocraft_format::atomic_write(std::path::Path::new(path), bytes).map_err(|e| e.to_string()))), ..Default::default() }; let open = arg(&args, "--open");