macOS: save to SMB shares that refuse a full flush (#1639)

sync_all on Apple platforms asks for F_FULLFSYNC, which SMB shares (ENOTSUP) and some virtual
file systems (ENOTTY) refuse, so every save there failed. atomic::sync_file falls back to a
plain fsync for exactly those errors, as Apple's removefile does; the automation workspace
writes use it too. rustix is already in the lock on Apple targets. Fixes #1336.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Brennan Stehling 2026-10-09 06:36:03 -07:00 committed by GitHub
parent f338454f36
commit 9fee51c308
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 83 additions and 5 deletions

1
Cargo.lock generated
View File

@ -4173,6 +4173,7 @@ dependencies = [
"photocraft-geom", "photocraft-geom",
"photocraft-raster", "photocraft-raster",
"proptest", "proptest",
"rustix 1.1.5",
"ruzstd", "ruzstd",
"serde", "serde",
"serde_json", "serde_json",

View File

@ -68,7 +68,8 @@ impl AuthorizedWorkspace {
options.write(true).create_new(true); options.write(true).create_new(true);
let written = root.dir.open_with(&tmp, &options).and_then(|mut file| { let written = root.dir.open_with(&tmp, &options).and_then(|mut file| {
file.write_all(bytes)?; 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))); let renamed = written.and_then(|()| photocraft_format::atomic::retry_rename(RenameRetry::platform(), || root.dir.rename(&tmp, &root.dir, &relative)));
if let Err(e) = renamed { if let Err(e) = renamed {
@ -80,7 +81,7 @@ impl AuthorizedWorkspace {
{ {
let dir = if parent.as_os_str().is_empty() { PathBuf::from(".") } else { parent }; let dir = if parent.as_os_str().is_empty() { PathBuf::from(".") } else { parent };
if let Ok(d) = root.dir.open(&dir) { if let Ok(d) = root.dir.open(&dir) {
let _ = d.sync_all(); let _ = photocraft_format::atomic::sync_file(&d.into_std());
} }
} }
Ok(()) Ok(())

View File

@ -22,5 +22,10 @@ flate2 = { workspace = true }
blake3 = { version = "1.8", default-features = false, features = ["pure"] } blake3 = { version = "1.8", default-features = false, features = ["pure"] }
ruzstd = "0.9" 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] [dev-dependencies]
proptest = { workspace = true } proptest = { workspace = true }

View File

@ -13,6 +13,9 @@
//! if it does anyway, the save fails with a clear error. It never falls back to writing the //! if it does anyway, the save fails with a clear error. It never falls back to writing the
//! destination in place. //! destination in place.
//! - **Unwritable folder:** the save fails with a clear error instead of writing 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 //! 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" //! 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 /// The file-system steps [`atomic_write_with`] performs. [`RealFs`] is the real thing; tests
/// wrap it to inject failures. /// wrap it to inject failures.
pub trait AtomicFs { 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<()>; fn write_new(&self, tmp: &Path, bytes: &[u8]) -> io::Result<()>;
/// Rename `from` over `to`, replacing it. /// Rename `from` over `to`, replacing it.
fn rename(&self, from: &Path, to: &Path) -> io::Result<()>; 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<()> { fn write_new(&self, tmp: &Path, bytes: &[u8]) -> io::Result<()> {
let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(tmp)?; let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(tmp)?;
f.write_all(bytes)?; f.write_all(bytes)?;
f.sync_all() sync_file(&f)
} }
fn rename(&self, from: &Path, to: &Path) -> io::Result<()> { fn rename(&self, from: &Path, to: &Path) -> io::Result<()> {
std::fs::rename(from, to) std::fs::rename(from, to)
@ -59,7 +62,7 @@ impl AtomicFs for RealFs {
fn sync_dir(&self, dir: &Path) -> io::Result<()> { fn sync_dir(&self, dir: &Path) -> io::Result<()> {
#[cfg(unix)] #[cfg(unix)]
{ {
std::fs::File::open(dir)?.sync_all() sync_file(&std::fs::File::open(dir)?)
} }
#[cfg(not(unix))] #[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`. /// How hard to retry a rename that fails with `PermissionDenied`.
#[derive(Debug, Clone, Copy)] #[derive(Debug, Clone, Copy)]
pub struct RenameRetry { pub struct RenameRetry {

View File

@ -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());
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();
}