Library on an SMB share: fall back to a plain fsync where macOS refuses the full flush (#1407)

AGENTS.md

@@ -39,8 +39,8 @@ of it. Full standard: `../craftrules/standards/never-crash.md`
   fallback (`unwrap_or…`) only where it can't silently corrupt a document. An unfinished feature returns an
   "unsupported" error or is disabled. Sole exception: a provably infallible literal, as `#[allow(clippy::expect_used)]`
   + `.expect("why it can't fail")`.
-- **`unsafe` lives only in `crates/sysmem`** (one FFI call, `malloc_zone_pressure_relief`, that returns freed
-  allocator pages to macOS after raw decodes). Every other production crate root has `#![forbid(unsafe_code)]`. A new
+- **`unsafe` lives only in `crates/sysmem`** (two FFI calls: `malloc_zone_pressure_relief`, that returns freed
+  allocator pages to macOS after raw decodes, and `fsync`, for volumes that refuse std's `F_FULLFSYNC`). Every other production crate root has `#![forbid(unsafe_code)]`. A new
   unsafe need goes in an isolated, well-tested helper crate like it: `// SAFETY:` on every block, a safe API, a safe
   fallback where possible, and a line here naming it.
 - **Input-derived numbers are hostile:** `get()` instead of `[i]`/`[a..b]` for offsets from files, users, agents or

Cargo.lock

@@ -3104,6 +3104,7 @@ dependencies = [
  "lightcraft-geom",
  "lightcraft-meta",
  "lightcraft-raw",
+ "lightcraft-sysmem",
  "log",
  "proptest",
  "serde",

crates/catalog/Cargo.toml

@@ -12,6 +12,7 @@ workspace = true
 lightcraft-develop = { workspace = true }
 lightcraft-meta = { workspace = true }
 lightcraft-raw = { workspace = true }
+lightcraft-sysmem = { workspace = true }
 serde = { workspace = true }
 serde_json = { workspace = true }
 thiserror = { workspace = true }

crates/catalog/src/store.rs

@@ -175,9 +175,8 @@ impl Store for FsStore {
 
 /// Flush a file to disk: `sync_all`, or `sync_data` when `data_only`.
 ///
-/// On macOS both are `F_FULLFSYNC`, which an SMB share refuses with ENOTSUP (issue #1407): the
-/// volume has no such flush, so the write stands as the share keeps it instead of failing. Any
-/// other error still fails the write.
+/// On macOS both are `F_FULLFSYNC`, which an SMB share refuses with ENOTSUP (issue #1407) while a
+/// plain `fsync` works there: that is the fallback. If it fails too, the write fails.
 fn sync_file(f: &std::fs::File, data_only: bool) -> io::Result<()> {
     #[cfg(test)]
     let r = if NO_FULL_SYNC.with(std::cell::Cell::get) {
@@ -190,11 +189,17 @@ fn sync_file(f: &std::fs::File, data_only: bool) -> io::Result<()> {
     #[cfg(not(test))]
     let r = if data_only { f.sync_data() } else { f.sync_all() };
     match r {
-        Err(e) if flush_unsupported(&e) => Ok(()),
+        Err(e) if flush_unsupported(&e) => plain_fsync(f),
         r => r,
     }
 }
 
+fn plain_fsync(f: &std::fs::File) -> io::Result<()> {
+    #[cfg(test)]
+    PLAIN_FSYNCS.with(|c| c.set(c.get() + 1));
+    lightcraft_sysmem::fsync(f)
+}
+
 /// The volume can't flush a file to disk (ENOTSUP on macOS, `Unsupported` elsewhere).
 fn flush_unsupported(e: &io::Error) -> bool {
     const ENOTSUP: i32 = 45;
@@ -204,6 +209,13 @@ fn flush_unsupported(e: &io::Error) -> bool {
 #[cfg(test)]
 thread_local! {
     static NO_FULL_SYNC: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
+    static PLAIN_FSYNCS: std::cell::Cell<u64> = const { std::cell::Cell::new(0) };
+}
+
+/// Tests: how many plain `fsync` fallbacks [`FsStore`] made on the calling thread.
+#[cfg(test)]
+pub(crate) fn plain_fsyncs() -> u64 {
+    PLAIN_FSYNCS.with(std::cell::Cell::get)
 }
 
 /// What a volume without a full flush answers (macOS: ENOTSUP, os error 45, from `F_FULLFSYNC`).

crates/catalog/src/tests_journal.rs

@@ -287,11 +287,13 @@ fn fs_store_roundtrip() {
 }
 
 /// A library on an SMB share (issue #1407): macOS can't make the full flush there (`F_FULLFSYNC`
-/// fails with ENOTSUP), and the new library's snapshot, its log and `prefs.json` are still written.
+/// fails with ENOTSUP), so the files are flushed with a plain `fsync` and the new library's
+/// snapshot, its log and `prefs.json` are still written.
 #[test]
 fn a_volume_without_a_full_flush_still_gets_its_files() {
     let dir = std::env::temp_dir().join(format!("lc-journal-nofullsync-{}", std::process::id()));
     let _ = std::fs::remove_dir_all(&dir);
+    let fsyncs = crate::store::plain_fsyncs();
     let c = {
         let _smb = crate::store::full_sync_unsupported();
         let (mut j, mut c, r) = Journal::open(Box::new(FsStore::open(&dir).unwrap())).unwrap();
@@ -305,6 +307,8 @@ fn a_volume_without_a_full_flush_still_gets_its_files() {
         let mut s = FsStore::open(&dir).unwrap();
         s.write_atomic("prefs.json", b"{}").unwrap();
         s.truncate(LOG, 0).unwrap();
+        // the new library's snapshot, the append, the snapshot and the emptied log, prefs.json, the truncate
+        assert_eq!(crate::store::plain_fsyncs() - fsyncs, 6, "each sync falls back to fsync");
         c
     };
     let mut names: Vec<String> = std::fs::read_dir(&dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).collect();

crates/sysmem/Cargo.toml

@@ -3,9 +3,9 @@ name = "lightcraft-sysmem"
 version.workspace = true
 edition.workspace = true
 license.workspace = true
-description = "LightCraft sysmem (L0): returns freed allocator memory to the OS. The workspace's only crate allowed `unsafe` (one FFI call)."
+description = "LightCraft sysmem (L0): returns freed allocator memory to the OS, and a plain fsync on macOS. The workspace's only crate allowed `unsafe` (two FFI calls)."
 
-# Opts out of the workspace lints only to keep `unsafe_code = "deny"` overridable for the single FFI
+# Opts out of the workspace lints only to keep `unsafe_code = "deny"` overridable for the FFI
 # module below; every other crate root has `#![forbid(unsafe_code)]`. See AGENTS.md and craftrules
 # standards/never-crash.md ("isolated helper library crate").
 [lints.rust]

crates/sysmem/src/lib.rs

@@ -6,8 +6,12 @@
 //! everything ever decoded. `malloc_zone_pressure_relief` (libSystem) returns those pages now.
 //! Elsewhere (glibc, Windows) blocks this large are unmapped when freed: nothing to do.
 //!
-//! This is LightCraft's only crate allowed `unsafe` (one FFI call, in `macos`). Its API is safe,
-//! and on failure or on other platforms it simply releases nothing.
+//! [`fsync`] is the plain POSIX `fsync`, which std doesn't offer on macOS: there `File::sync_all`
+//! and `sync_data` are `F_FULLFSYNC`, which SMB shares refuse with ENOTSUP while `fsync` works
+//! (issue #1407).
+//!
+//! This is LightCraft's only crate allowed `unsafe` (two FFI calls, in `macos`). Its API is safe;
+//! on failure or on other platforms `release_free_memory` simply releases nothing.
 #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
 
 /// Return free allocator pages to the system. Returns the number of bytes released (always 0 on
@@ -24,10 +28,28 @@ pub fn release_free_memory() -> usize {
     }
 }
 
+/// Flush `file` to the storage device with a plain `fsync`. On macOS that is the libSystem call
+/// (std's `sync_all` is `F_FULLFSYNC`, which some volumes refuse); elsewhere it is `sync_all`.
+pub fn fsync(file: &std::fs::File) -> std::io::Result<()> {
+    #[cfg(target_os = "macos")]
+    {
+        macos::fsync(file)
+    }
+    #[cfg(not(target_os = "macos"))]
+    {
+        file.sync_all()
+    }
+}
+
 #[cfg(target_os = "macos")]
 #[allow(unsafe_code)]
 mod macos {
+    use std::os::fd::AsRawFd;
+
     unsafe extern "C" {
+        /// libSystem (unistd.h): flush the open file `fd` to the device. 0, or -1 and `errno`.
+        #[link_name = "fsync"]
+        fn c_fsync(fd: std::ffi::c_int) -> std::ffi::c_int;
         /// libSystem malloc: release free memory of `zone` (all zones when null) back to the
         /// system, up to `goal` bytes (0 = as much as possible). Returns the bytes released.
         fn malloc_zone_pressure_relief(zone: *mut std::ffi::c_void, goal: usize) -> usize;
@@ -41,6 +63,20 @@ mod macos {
         // releasing 0 bytes.
         unsafe { malloc_zone_pressure_relief(std::ptr::null_mut(), 0) }
     }
+
+    pub fn fsync(file: &std::fs::File) -> std::io::Result<()> {
+        loop {
+            // SAFETY: `fsync` (libSystem, unistd.h) takes a file descriptor by value and touches no
+            // memory of ours. The descriptor is open for the whole call: `file` is borrowed.
+            if unsafe { c_fsync(file.as_raw_fd()) } == 0 {
+                return Ok(());
+            }
+            let e = std::io::Error::last_os_error();
+            if e.kind() != std::io::ErrorKind::Interrupted {
+                return Err(e);
+            }
+        }
+    }
 }
 
 #[cfg(test)]
@@ -70,6 +106,21 @@ mod tests {
         assert_eq!(again[again.len() - 1], 7);
     }
 
+    #[test]
+    fn fsync_flushes_an_open_file_and_reports_errors() {
+        use std::io::Write;
+        let p = std::env::temp_dir().join(format!("lc-sysmem-fsync-{}", std::process::id()));
+        let mut f = std::fs::File::create(&p).unwrap();
+        f.write_all(b"data").unwrap();
+        super::fsync(&f).unwrap();
+        assert_eq!(std::fs::read(&p).unwrap(), b"data");
+        let _ = std::fs::remove_file(&p);
+        // a directory opened read-only can be synced too; what fails is an error, not a panic
+        if let Ok(d) = std::fs::File::open(std::env::temp_dir()) {
+            let _ = super::fsync(&d);
+        }
+    }
+
     #[test]
     fn concurrent_calls_are_safe() {
         let handles: Vec<_> = (0..4).map(|_| std::thread::spawn(release_free_memory)).collect();

docs/architecture.md

@@ -13,7 +13,7 @@ L5 depends on egui, eframe, winit or rfd, and that each crate only uses lower la
 
 | Layer | Crates (lines) | Role |
 |---|---|---|
-| L0 | `geom` (1.4k), `color` (0.9k), `raster` (0.8k), `tiff` (2.0k), `fetch` (1.4k), `sysmem` (0.1k) | Matrices and warps, colour spaces and transfer functions, float image buffers, TIFF/IFD read and write, pinned HTTPS model downloads, the one `unsafe` FFI call (`malloc_zone_pressure_relief`) |
+| L0 | `geom` (1.4k), `color` (0.9k), `raster` (0.8k), `tiff` (2.0k), `fetch` (1.4k), `sysmem` (0.1k) | Matrices and warps, colour spaces and transfer functions, float image buffers, TIFF/IFD read and write, pinned HTTPS model downloads, the `unsafe` FFI calls (`malloc_zone_pressure_relief`, `fsync`) |
 | L1 | `raw` (22.3k), `codecs` (7.3k), `meta` (4.3k), `develop` (2.5k), `denoise-core` (2.2k), `denoise` (2.7k), `faces` (5.3k), `scenes` (1.0k), `heif` (2.1k, optional) | Raw decoders and demosaic, DNG colour model; standard codecs and encoders; EXIF/XMP/IPTC; the non-destructive edit model and control specs; AI denoise; face detection and recognition; procedural demo photos; HEIC |
 | L2 | `pipeline` (9.8k) | The develop pipeline: scene-referred float, resolution independent; the CPU reference (oracle) for everything else |
 | L3 | `catalog` (11.0k), `gpu` (5.1k), `preview` (1.3k), `merge` (3.8k), `segment` (2.3k) | Library model and persistence; wgpu compute twin of the pipeline; thumbnail/preview cache and job pool; HDR and panorama merge; SAM 3 segmentation |