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 5239aee8..3a7e9f90 100644 --- a/crates/catalog/src/store.rs +++ b/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 = 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`). diff --git a/crates/catalog/src/tests_journal.rs b/crates/catalog/src/tests_journal.rs index 81cdb35b..29f87703 100644 --- a/crates/catalog/src/tests_journal.rs +++ b/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 = std::fs::read_dir(&dir).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).collect(); 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 |