diff --git a/Cargo.lock b/Cargo.lock index c30e4e31..9e59c573 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4689,6 +4689,7 @@ version = "0.10.0" dependencies = [ "flate2", "proptest", + "rustix 1.1.5", "serde", "serde_json", "thiserror 2.0.21", diff --git a/Cargo.toml b/Cargo.toml index 2d72fbf7..25ca0782 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,6 +65,7 @@ skrifa = "0.47" harfrust = "0.13" linesweeper = "0.5" flate2 = { version = "1", default-features = false, features = ["rust_backend"] } +rustix = { version = "1.1", default-features = false } # Zstandard decoder (pure Rust, builds for wasm32 too): the editing copy an EPS carries of its art. ruzstd = { version = "0.9", default-features = false, features = ["std"] } jpeg-encoder = { version = "0.6", default-features = false, features = ["std"] } diff --git a/crates/engine/src/cmd/fileio/mod.rs b/crates/engine/src/cmd/fileio/mod.rs index 9a0194e3..89acf941 100644 --- a/crates/engine/src/cmd/fileio/mod.rs +++ b/crates/engine/src/cmd/fileio/mod.rs @@ -794,7 +794,7 @@ pub(crate) fn write_new_file(path: &str, bytes: &[u8]) -> Result<()> { may_write(path)?; let err = |e: std::io::Error| EngineError::Other(format!("{path}: {e}")); let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(path).map_err(err)?; - let written = f.write_all(bytes).and_then(|()| f.sync_all()); + let written = f.write_all(bytes).and_then(|()| vectorcraft_format::sync_file(&f)); drop(f); written.map_err(|e| { // Best effort: the partly written file is all there is to clean up. diff --git a/crates/format/Cargo.toml b/crates/format/Cargo.toml index 0dedea5f..6ab9ce58 100644 --- a/crates/format/Cargo.toml +++ b/crates/format/Cargo.toml @@ -15,6 +15,11 @@ serde_json = { workspace = true } thiserror = { workspace = true } flate2 = { workspace = true } +# A plain fsync where a volume refuses the full flush (`sync_file`, atomic.rs). Already in +# Cargo.lock (tempfile), so this adds no new crate. +[target.'cfg(target_vendor = "apple")'.dependencies] +rustix = { workspace = true, features = ["std", "fs"] } + [dev-dependencies] vectorcraft-testkit = { workspace = true } proptest = { workspace = true } diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs index c2948a94..feaa235a 100644 --- a/crates/format/src/atomic.rs +++ b/crates/format/src/atomic.rs @@ -1,6 +1,9 @@ //! Safe saving: a file is written to a temporary file beside it, then renamed over it, so a failed //! or interrupted write never leaves a damaged file behind (the old one stays as it was). //! [`write_new_with`] creates a file the same way without ever replacing one. +//! +//! On macOS `File::sync_all` is a full flush (`F_FULLFSYNC`), which SMB shares refuse with +//! `ENOTSUP` while a plain `fsync` works: [`sync_file`] falls back to the plain one there (#1620). use std::fs::{File, OpenOptions}; use std::io::{self, Write as _}; @@ -19,7 +22,7 @@ pub fn write_atomic_with(path: &Path, fill: impl FnOnce(&mut File) -> io::Result let target = resolve_link(path)?; let tmp = temp_path(&target)?; let mut f = OpenOptions::new().write(true).create_new(true).open(&tmp)?; - let filled = fill(&mut f).and_then(|()| f.sync_all()); + let filled = fill(&mut f).and_then(|()| sync_file(&f)); drop(f); filled.and_then(|()| keep_permissions(&target, &tmp)).and_then(|()| std::fs::rename(&tmp, &target)).inspect_err(|_| { // Best effort: the temporary file is all there is to clean up. @@ -45,7 +48,7 @@ fn write_new_linking( ) -> io::Result<()> { let tmp = temp_path(path)?; let mut f = OpenOptions::new().write(true).create_new(true).open(&tmp)?; - let filled = fill(&mut f).and_then(|()| f.sync_all()); + let filled = fill(&mut f).and_then(|()| sync_file(&f)); drop(f); let placed = filled.and_then(|()| match link(&tmp, path) { Err(e) if e.kind() != io::ErrorKind::AlreadyExists => copy_new(&tmp, path), @@ -60,7 +63,7 @@ fn write_new_linking( /// removes it again. fn copy_new(from: &Path, path: &Path) -> io::Result<()> { let mut to = OpenOptions::new().write(true).create_new(true).open(path)?; - let copied = File::open(from).and_then(|mut f| io::copy(&mut f, &mut to)).and_then(|_| to.sync_all()); + let copied = File::open(from).and_then(|mut f| io::copy(&mut f, &mut to)).and_then(|_| sync_file(&to)); drop(to); if copied.is_err() { // Best effort: the part copied is all there is to clean up. @@ -69,6 +72,63 @@ fn copy_new(from: &Path, path: &Path) -> io::Result<()> { copied } +/// Flush `file` to stable storage (`sync_all`). On Apple platforms that is a full flush +/// (`F_FULLFSYNC`); where the volume refuses it with `ENOTSUP` (SMB shares, #1620) this falls back +/// to a plain `fsync`. Any other error, and a failed `fsync`, is returned as it is. +pub fn sync_file(file: &File) -> io::Result<()> { + match full_sync(file) { + Err(e) if full_flush_unsupported(&e) => plain_fsync(file), + r => r, + } +} + +/// Whether `e` is a volume refusing a full flush. Never off Apple platforms: there `sync_all` is +/// already a plain `fsync`. Checked by its code because std maps `ENOTSUP` to `Uncategorized`. +fn full_flush_unsupported(e: &io::Error) -> bool { + cfg!(target_vendor = "apple") && e.raw_os_error() == Some(ENOTSUP) +} + +#[cfg(target_vendor = "apple")] +fn plain_fsync(file: &File) -> io::Result<()> { + #[cfg(test)] + { + PLAIN_FSYNCS.with(|c| c.set(c.get() + 1)); + if let Some(errno) = PLAIN_FSYNC_FAILS.with(std::cell::Cell::get) { + return Err(io::Error::from_raw_os_error(errno)); + } + } + rustix::fs::fsync(file).map_err(io::Error::from) +} + +#[cfg(not(target_vendor = "apple"))] +fn plain_fsync(file: &File) -> io::Result<()> { + file.sync_all() +} + +fn full_sync(file: &File) -> io::Result<()> { + #[cfg(test)] + if REFUSE_FULL_SYNC.with(std::cell::Cell::get) { + return Err(io::Error::from_raw_os_error(ENOTSUP)); + } + file.sync_all() +} + +/// What macOS answers when a volume can't make a full flush (SMB shares). +#[cfg(target_vendor = "apple")] +const ENOTSUP: i32 = 45; +#[cfg(not(target_vendor = "apple"))] +const ENOTSUP: i32 = 95; + +#[cfg(test)] +thread_local! { + /// Tests: every full flush on this thread fails as on an SMB share mounted by macOS. + static REFUSE_FULL_SYNC: std::cell::Cell = const { std::cell::Cell::new(false) }; + /// Tests: the plain `fsync` fallback fails too, with this OS error. + static PLAIN_FSYNC_FAILS: std::cell::Cell> = const { std::cell::Cell::new(None) }; + /// Tests: how many plain `fsync` fallbacks were made on this thread. + static PLAIN_FSYNCS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + /// A symbolic link keeps pointing at its file: write the file it names. fn resolve_link(path: &Path) -> io::Result { match std::fs::symlink_metadata(path) { @@ -137,6 +197,36 @@ mod tests { let _ = std::fs::remove_dir_all(&d); } + /// A save to an SMB share (#1620): macOS refuses the full flush there with `ENOTSUP`, so the + /// file is flushed with a plain `fsync` and saved. When that fails too the save fails as before, + /// the old file stays and no temporary file is left. Elsewhere `sync_all` is already a plain + /// `fsync`: a refusal is an error like any other. + #[test] + fn a_volume_that_refuses_the_full_flush_still_saves() { + let d = dir("no-full-sync"); + let path = d.join("doc.vectorcraft"); + std::fs::write(&path, b"original").unwrap(); + let apple = cfg!(target_vendor = "apple"); + REFUSE_FULL_SYNC.with(|c| c.set(true)); + let before = PLAIN_FSYNCS.with(std::cell::Cell::get); + let saved = write_atomic(&path, b"new"); + let created = write_new_with(&d.join("new.otf"), |f| f.write_all(b"created")); + let fsyncs = PLAIN_FSYNCS.with(std::cell::Cell::get) - before; + PLAIN_FSYNC_FAILS.with(|c| c.set(Some(5))); + let failed = write_atomic(&path, b"lost"); + PLAIN_FSYNC_FAILS.with(|c| c.set(None)); + REFUSE_FULL_SYNC.with(|c| c.set(false)); + assert_eq!(saved.is_ok(), apple, "{saved:?}"); + assert_eq!(created.is_ok(), apple, "{created:?}"); + assert_eq!(fsyncs, if apple { 2 } else { 0 }, "each refused full flush falls back to fsync"); + assert!(failed.is_err(), "a failed fsync still fails the save"); + assert_eq!(std::fs::read(&path).unwrap(), if apple { &b"new"[..] } else { b"original" }); + let mut left = leftovers(&d, "doc.vectorcraft"); + left.retain(|n| n != "new.otf"); + assert!(left.is_empty(), "no temporary file is left: {left:?}"); + let _ = std::fs::remove_dir_all(&d); + } + #[test] fn write_new_never_replaces_what_is_there() { let d = dir("new"); diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 9ea60bf6..e6891e1e 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -45,7 +45,7 @@ use serde::{Deserialize, Serialize}; use vectorcraft_doc::{Document, ImageBlob}; #[cfg(not(target_arch = "wasm32"))] -pub use atomic::{write_atomic, write_atomic_with, write_new_with}; +pub use atomic::{sync_file, write_atomic, write_atomic_with, write_new_with}; use stream::{Gz, Indent, KeyScan, Out}; /// v4: large data after the JSON (v1 to v3 files still load).