diff --git a/crates/catalog/src/store.rs b/crates/catalog/src/store.rs index a672d00e..5239aee8 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,64 @@ 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. +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) => Ok(()), + r => r, + } +} + +/// 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) }; +} + +/// 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..81cdb35b 100644 --- a/crates/catalog/src/tests_journal.rs +++ b/crates/catalog/src/tests_journal.rs @@ -286,6 +286,35 @@ 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), 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 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(); + 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]