diff --git a/Cargo.lock b/Cargo.lock index 9dd34b93..599dc4d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4173,6 +4173,7 @@ dependencies = [ "photocraft-geom", "photocraft-raster", "proptest", + "rustix 1.1.5", "ruzstd", "serde", "serde_json", diff --git a/crates/automation/src/workspace.rs b/crates/automation/src/workspace.rs index 2d46f3db..3d4cd0bf 100644 --- a/crates/automation/src/workspace.rs +++ b/crates/automation/src/workspace.rs @@ -68,7 +68,8 @@ impl AuthorizedWorkspace { 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() + // Falls back to a plain fsync on shares that refuse a full flush (#1336). + photocraft_format::atomic::sync_file(&file.into_std()) }); let renamed = written.and_then(|()| photocraft_format::atomic::retry_rename(RenameRetry::platform(), || root.dir.rename(&tmp, &root.dir, &relative))); if let Err(e) = renamed { @@ -80,7 +81,7 @@ impl AuthorizedWorkspace { { 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(); + let _ = photocraft_format::atomic::sync_file(&d.into_std()); } } Ok(()) diff --git a/crates/format/Cargo.toml b/crates/format/Cargo.toml index 8198032e..f29ccf86 100644 --- a/crates/format/Cargo.toml +++ b/crates/format/Cargo.toml @@ -22,5 +22,10 @@ flate2 = { workspace = true } blake3 = { version = "1.8", default-features = false, features = ["pure"] } ruzstd = "0.9" +# A plain fsync where a file system refuses the full flush (`sync_file`, atomic.rs). Already in +# Cargo.lock on Apple targets (cap-std), so this adds no new crate. +[target.'cfg(target_vendor = "apple")'.dependencies] +rustix = { version = "1.1", default-features = false, features = ["std", "fs"] } + [dev-dependencies] proptest = { workspace = true } diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs index c1c5a8d2..25d433db 100644 --- a/crates/format/src/atomic.rs +++ b/crates/format/src/atomic.rs @@ -13,6 +13,9 @@ //! 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. +//! - **Network shares (macOS):** `sync_all` asks for a full flush to the disk's platters +//! (`F_FULLFSYNC`), which SMB shares and some virtual file systems refuse while a plain `fsync` +//! works. [`sync_file`] falls back to the plain one there (#1336). //! //! 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" @@ -27,7 +30,7 @@ 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. + /// Create `tmp` (it must not exist), write all of `bytes` and flush it ([`sync_file`]). 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<()>; @@ -48,7 +51,7 @@ 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() + sync_file(&f) } fn rename(&self, from: &Path, to: &Path) -> io::Result<()> { std::fs::rename(from, to) @@ -59,7 +62,7 @@ impl AtomicFs for RealFs { fn sync_dir(&self, dir: &Path) -> io::Result<()> { #[cfg(unix)] { - std::fs::File::open(dir)?.sync_all() + sync_file(&std::fs::File::open(dir)?) } #[cfg(not(unix))] { @@ -73,6 +76,37 @@ impl AtomicFs for RealFs { } } +/// Flush `file` to stable storage (`sync_all`). On Apple platforms that is a full flush +/// (`F_FULLFSYNC`); where the file system can't do one (SMB shares refuse it with `ENOTSUP`, +/// #1336; Tart's shared folders with `ENOTTY`) this falls back to a plain `fsync`, as Apple's own +/// `removefile` does. Any other error is returned as it is. +pub fn sync_file(file: &std::fs::File) -> io::Result<()> { + #[cfg(target_vendor = "apple")] + { + or_plain_fsync(file.sync_all(), || rustix::fs::fsync(file).map_err(io::Error::from)) + } + #[cfg(not(target_vendor = "apple"))] + { + file.sync_all() + } +} + +/// `full`, or `fsync()` when `full` failed only because the file system can't do a full flush. +pub fn or_plain_fsync(full: io::Result<()>, fsync: impl FnOnce() -> io::Result<()>) -> io::Result<()> { + match full { + Err(e) if full_flush_unsupported(&e) => fsync(), + other => other, + } +} + +/// Whether `e` is a file system refusing a full flush (`F_FULLFSYNC`): `ENOTSUP` (45, SMB) or +/// `ENOTTY` (25) on Apple platforms. Never elsewhere: there `sync_all` is a plain `fsync`. +fn full_flush_unsupported(e: &io::Error) -> bool { + const ENOTTY: i32 = 25; + const ENOTSUP: i32 = 45; + cfg!(target_vendor = "apple") && matches!(e.raw_os_error(), Some(ENOTSUP | ENOTTY)) +} + /// How hard to retry a rename that fails with `PermissionDenied`. #[derive(Debug, Clone, Copy)] pub struct RenameRetry { diff --git a/crates/format/src/atomic/tests.rs b/crates/format/src/atomic/tests.rs index be89dbe1..c03c71cc 100644 --- a/crates/format/src/atomic/tests.rs +++ b/crates/format/src/atomic/tests.rs @@ -210,3 +210,40 @@ fn directory_target_or_bare_root_is_an_error_not_a_panic() { assert!(atomic_write(Path::new("/"), b"x").is_err()); assert!(atomic_write(Path::new(""), b"x").is_err()); } + +/// A share that refuses the full flush (`F_FULLFSYNC`, #1336) still saves through a plain fsync on +/// Apple platforms; every other sync error still fails the save. +#[test] +fn a_refused_full_flush_falls_back_to_a_plain_fsync_on_apple_only() { + let refused = |errno| Err(io::Error::from_raw_os_error(errno)); + for (errno, name) in [(45, "ENOTSUP, SMB"), (25, "ENOTTY")] { + let fsynced = Cell::new(false); + let r = or_plain_fsync(refused(errno), || { + fsynced.set(true); + Ok(()) + }); + let apple = cfg!(target_vendor = "apple"); + assert_eq!(r.is_ok(), apple, "{name}"); + assert_eq!(fsynced.get(), apple, "{name}: the plain fsync runs only on Apple platforms"); + } + // The plain fsync's own failure is reported. + if cfg!(target_vendor = "apple") { + let r = or_plain_fsync(refused(45), || Err(io::Error::from_raw_os_error(5))); + assert_eq!(r.map_err(|e| e.raw_os_error()), Err(Some(5))); + } + // Real errors (EIO, ENOSPC, EACCES) never fall back, and success needs no fallback. + for errno in [5, 28, 13] { + let r = or_plain_fsync(refused(errno), || panic!("no fallback for errno {errno}")); + assert_eq!(r.map_err(|e| e.raw_os_error()), Err(Some(errno))); + } + assert!(or_plain_fsync(Ok(()), || panic!("no fallback after a full flush")).is_ok()); +} + +#[test] +fn sync_file_flushes_a_local_file() { + let dir = TempDir::new("sync"); + let path = dir.0.join("f.bin"); + // Opened for writing, as the saves do: Windows can't flush a read-only handle. + let file = std::fs::OpenOptions::new().write(true).create_new(true).open(&path).unwrap(); + sync_file(&file).unwrap(); +}