Merge PR #1409

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

@@ -88,10 +88,14 @@ impl Store for FsStore {
             self.appender = None;
         }
         let tmp = self.path(&format!("{name}.tmp"));
-        {
+        let written = (|| {
             let mut f = std::fs::File::create(&tmp)?;
             f.write_all(data)?;
-            f.sync_all()?;
+            sync_file(&f, false)
+        })();
+        if let Err(e) = written {
+            let _ = std::fs::remove_file(&tmp);
+            return Err(e);
         }
         std::fs::rename(&tmp, self.path(name))?;
         self.sync_dir();
@@ -109,7 +113,7 @@ impl Store for FsStore {
             fill(&mut w)?;
             w.flush()?;
             let f = w.inner.into_inner().map_err(|e| e.into_error())?;
-            f.sync_all()?;
+            sync_file(&f, false)?;
             Ok(w.n)
         })();
         let n = match written {
@@ -135,7 +139,7 @@ impl Store for FsStore {
             self.appender = Some((name.to_string(), f));
         }
         let Some((_, f)) = self.appender.as_mut() else { return Err(io::Error::other("appender not open")) };
-        let written = f.write_all(data).and_then(|()| f.sync_data());
+        let written = f.write_all(data).and_then(|()| sync_file(f, true));
         if written.is_err() {
             // The handle may be dead (a network share that reconnected, a removed volume): the next
             // attempt reopens the file. Part of `data` may be in the file; the caller cuts it back
@@ -156,7 +160,7 @@ impl Store for FsStore {
             Err(e) => return Err(e),
         };
         f.set_len(len)?;
-        f.sync_all()
+        sync_file(&f, false)
     }
 
     fn describe(&self) -> String {
@@ -169,6 +173,76 @@ 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) 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) {
+        Err(enotsup())
+    } else if data_only {
+        f.sync_data()
+    } else {
+        f.sync_all()
+    };
+    #[cfg(not(test))]
+    let r = if data_only { f.sync_data() } else { f.sync_all() };
+    match r {
+        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;
+    e.kind() == io::ErrorKind::Unsupported || (cfg!(target_vendor = "apple") && e.raw_os_error() == Some(ENOTSUP))
+}
+
+#[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`).
+#[cfg(test)]
+fn enotsup() -> io::Error {
+    if cfg!(target_vendor = "apple") { io::Error::from_raw_os_error(45) } else { io::Error::from(io::ErrorKind::Unsupported) }
+}
+
+/// Tests: while the guard lives, every file sync [`FsStore`] makes on the calling thread fails
+/// as on an SMB share mounted by macOS (issue #1407).
+#[cfg(test)]
+pub(crate) fn full_sync_unsupported() -> NoFullSync {
+    NO_FULL_SYNC.with(|c| c.set(true));
+    NoFullSync(())
+}
+
+/// Ends a [`full_sync_unsupported`] when dropped.
+#[cfg(test)]
+pub(crate) struct NoFullSync(());
+
+#[cfg(test)]
+impl Drop for NoFullSync {
+    fn drop(&mut self) {
+        NO_FULL_SYNC.with(|c| c.set(false));
+    }
+}
+
 /// Counts the bytes written through it.
 struct Counter<W> {
     inner: W,

crates/catalog/src/tests_journal.rs

@@ -286,6 +286,39 @@ fn fs_store_roundtrip() {
     let _ = std::fs::remove_dir_all(&dir);
 }
 
+/// A library on an SMB share (issue #1407): macOS can't make the full flush there (`F_FULLFSYNC`
+/// 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();
+        assert!(r.created);
+        assert!(dir.join(SNAPSHOT).exists(), "the new library's snapshot is written");
+        let id = c.alloc_photo_id();
+        let add = Op::AddPhoto { photo: Box::new(Photo::new(id, Source::File { path: "/a.jpg".into() }, "a.jpg", "JPEG", 1, 1, "t")) };
+        c.apply(add.clone()).unwrap();
+        j.append(&[add]).unwrap();
+        j.snapshot(&c).unwrap();
+        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();
+    names.sort();
+    assert_eq!(names, vec![LOG, SNAPSHOT, "prefs.json"], "no temp file left behind");
+    let (_, c2, r) = Journal::open(Box::new(FsStore::open(&dir).unwrap())).unwrap();
+    assert_eq!((r.snapshot_seq, c2.to_snapshot()), (1, c.to_snapshot()));
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
 /// The streamed snapshot is byte-for-byte the format written before streaming (issue #37), so
 /// existing libraries load and older builds can read new snapshots; FsStore and MemStore agree.
 #[test]

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 |