diff --git a/AGENTS.md b/AGENTS.md index 8cd199ff..5ca244dc 100644 --- a/AGENTS.md +++ b/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 diff --git a/Cargo.lock b/Cargo.lock index f6a5c947..49fcac32 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3104,6 +3104,7 @@ dependencies = [ "lightcraft-geom", "lightcraft-meta", "lightcraft-raw", + "lightcraft-sysmem", "log", "proptest", "serde", diff --git a/crates/catalog/Cargo.toml b/crates/catalog/Cargo.toml index 97d8868d..59960285 100644 --- a/crates/catalog/Cargo.toml +++ b/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 } diff --git a/crates/catalog/src/store.rs b/crates/catalog/src/store.rs index a672d00e..3a7e9f90 100644 --- a/crates/catalog/src/store.rs +++ b/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 = const { std::cell::Cell::new(false) }; + static PLAIN_FSYNCS: std::cell::Cell = 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 { inner: W, diff --git a/crates/catalog/src/tests_journal.rs b/crates/catalog/src/tests_journal.rs index 8570bd07..29f87703 100644 --- a/crates/catalog/src/tests_journal.rs +++ b/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 = 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] diff --git a/crates/sysmem/Cargo.toml b/crates/sysmem/Cargo.toml index 422d8c71..cb11e9e5 100644 --- a/crates/sysmem/Cargo.toml +++ b/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] diff --git a/crates/sysmem/src/lib.rs b/crates/sysmem/src/lib.rs index 2961f537..9ad8ac00 100644 --- a/crates/sysmem/src/lib.rs +++ b/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(); diff --git a/docs/architecture.md b/docs/architecture.md index c50066a1..5b6c2ed9 100644 --- a/docs/architecture.md +++ b/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 |