diff --git a/crates/desktop-seams/src/fs_util.rs b/crates/desktop-seams/src/fs_util.rs index e0c1057ce..baf9c7bac 100644 --- a/crates/desktop-seams/src/fs_util.rs +++ b/crates/desktop-seams/src/fs_util.rs @@ -48,22 +48,14 @@ pub(crate) fn ensure_dir(dir: &Path) -> io::Result<()> { /// Durably writes `bytes` to `path`, replacing any existing file atomically. /// /// Barrier order: write a fresh temp file in the same directory, `sync_all` -/// its contents, `rename` it over the target (atomic replace on Unix and on -/// Windows — Rust's `fs::rename` maps to `MoveFileExW` with -/// `MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH`), then fsync the -/// directory so the rename itself is durable. A crash can only ever leave -/// the old value or the new value — never a torn one. +/// its contents, `rename` it over the target (an atomic replace on both Unix +/// and Windows), then [`fsync_dir`] so the rename itself is durable. A crash +/// can only ever leave the old value or the new value — never a torn one. pub(crate) fn atomic_write(path: &Path, bytes: &[u8]) -> io::Result<()> { let dir = path .parent() .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no parent"))?; - let tmp = temp_path(dir); - // Scope the handle so it is closed before the rename. - { - let mut file = File::create(&tmp)?; - file.write_all(bytes)?; - file.sync_all()?; - } + let tmp = write_synced_temp(dir, bytes)?; match fs::rename(&tmp, path) { Ok(()) => {} Err(err) => { @@ -71,13 +63,13 @@ pub(crate) fn atomic_write(path: &Path, bytes: &[u8]) -> io::Result<()> { return Err(err); } } - fsync_dir(dir) + fsync_dir(dir).map_err(|err| io::Error::new(err.kind(), format!("write barrier: {err}"))) } /// Durably removes a file. Idempotent: a missing file is success. The -/// removal is barriered with a directory fsync so an ordered caller (e.g. -/// the StagingStore's op-before-sidecar removal) can rely on it having hit -/// the platter before the next removal begins. +/// removal is followed by [`fsync_dir`] so an ordered caller (e.g. the +/// StagingStore's op-before-sidecar removal) can rely on it having hit the +/// platter before the next removal begins. pub(crate) fn remove_file_durable(path: &Path) -> io::Result<()> { match fs::remove_file(path) { Ok(()) => {} @@ -85,7 +77,8 @@ pub(crate) fn remove_file_durable(path: &Path) -> io::Result<()> { Err(err) => return Err(err), } if let Some(dir) = path.parent() { - fsync_dir(dir)?; + fsync_dir(dir) + .map_err(|err| io::Error::new(err.kind(), format!("unlink barrier: {err}")))?; } Ok(()) } @@ -121,21 +114,45 @@ pub(crate) fn list_file_names(dir: &Path) -> io::Result> { Ok(names) } -/// Fsyncs a directory so a create/rename/unlink inside it is durable. -/// -/// Unix opens the directory and `sync_all`s it. Windows has no directory -/// fsync; `fs::rename`'s `MOVEFILE_WRITE_THROUGH` and NTFS metadata -/// journaling cover the same guarantee, so this is a no-op there. +/// Barriers a directory so a create/rename/unlink inside it is durable +/// before the next one is issued. #[cfg(unix)] fn fsync_dir(dir: &Path) -> io::Result<()> { File::open(dir)?.sync_all() } #[cfg(not(unix))] -fn fsync_dir(_dir: &Path) -> io::Result<()> { +fn fsync_dir(dir: &Path) -> io::Result<()> { + metadata_log_barrier(dir) +} + +/// The [`fsync_dir`] barrier where a directory handle cannot be fsynced. +/// +/// NTFS journals metadata to a per-volume log flushed as an LSN-ordered +/// prefix, so `sync_all`ing a file created *after* an unlink or rename also +/// persists it. This covers both barriers, not just unlinks: std's Windows +/// `rename` passes `MOVEFILE_REPLACE_EXISTING` alone, never +/// `MOVEFILE_WRITE_THROUGH` (#665). The temp carries [`TEMP_PREFIX`], so a +/// crash before its removal leaves debris [`ensure_dir`] sweeps on reopen. +#[cfg(any(not(unix), test))] +fn metadata_log_barrier(dir: &Path) -> io::Result<()> { + // A byte of content makes the flush a data-and-metadata transaction + // rather than a flush of an untouched handle. + let path = write_synced_temp(dir, b"\0")?; + let _ = fs::remove_file(&path); Ok(()) } +/// Writes `bytes` to a fresh temp file in `dir` and `sync_all`s it, returning +/// its path with the handle already closed. +fn write_synced_temp(dir: &Path, bytes: &[u8]) -> io::Result { + let path = temp_path(dir); + let mut file = File::create(&path)?; + file.write_all(bytes)?; + file.sync_all()?; + Ok(path) +} + /// A process-unique filename component (`.`), so two in-flight /// records never collide on a name even across concurrent writers. pub(crate) fn unique_component() -> String { @@ -220,6 +237,32 @@ mod tests { assert_eq!(read_file_opt(&path).unwrap(), None); } + #[test] + fn metadata_log_barrier_leaves_the_directory_as_it_found_it() { + let dir = tempfile::tempdir().unwrap(); + atomic_write(&dir.path().join("value"), b"x").unwrap(); + metadata_log_barrier(dir.path()).unwrap(); + metadata_log_barrier(dir.path()).unwrap(); + assert_eq!( + fs::read_dir(dir.path()).unwrap().count(), + 1, + "successive barriers must neither collide on a temp name nor accumulate debris" + ); + assert_eq!( + read_file_opt(&dir.path().join("value")).unwrap(), + Some(b"x".to_vec()) + ); + } + + /// The barrier reports a failure rather than returning success without + /// having established one — an unbarriered removal is a fail-closed error, + /// never a fast path, because callers order two removals against it. + #[test] + fn metadata_log_barrier_fails_closed_when_it_cannot_be_established() { + let dir = tempfile::tempdir().unwrap(); + assert!(metadata_log_barrier(&dir.path().join("absent")).is_err()); + } + #[test] fn ensure_dir_sweeps_temp_debris() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/desktop-seams/src/staging_store.rs b/crates/desktop-seams/src/staging_store.rs index 91e6f5a62..3bc9f065d 100644 --- a/crates/desktop-seams/src/staging_store.rs +++ b/crates/desktop-seams/src/staging_store.rs @@ -34,12 +34,13 @@ const COUNTER_FILE: &str = "next_op_id"; /// reopens. /// /// **Removal ordering is a correctness property** (hard constraint 5): every -/// mutating method fsyncs before returning, so when the engine completes an -/// op by removing the op record *before* its sidecar, that ordering actually -/// reaches the platter in order. A crash can then only ever leave an orphan -/// sidecar (harmless, reclaimed by orphan-sidecar GC via [`staged_keys`] + -/// [`remove_staged_bytes`]) — never an op record pointing at a sidecar that -/// is already gone. +/// mutating method barriers its directory entry before returning (see +/// `fs_util::fsync_dir`), so when a caller completes an op by removing the +/// op record *before* its sidecar, that +/// ordering actually reaches the platter in order. A crash can then only ever +/// leave an orphan sidecar (harmless, reclaimed by orphan-sidecar GC via +/// [`staged_keys`] + [`remove_staged_bytes`]) — never an op record pointing +/// at a sidecar that is already gone. pub struct FileStagingStore { ops_dir: PathBuf, staged_dir: PathBuf, diff --git a/crates/desktop-seams/tests/conformance.rs b/crates/desktop-seams/tests/conformance.rs index 4922af596..7cf00653a 100644 --- a/crates/desktop-seams/tests/conformance.rs +++ b/crates/desktop-seams/tests/conformance.rs @@ -68,52 +68,105 @@ fn file_credential_store_passes_the_credential_store_kit() { // StagingStore desktop-specific durability — beyond what the kit asserts. // --------------------------------------------------------------------------- -/// The json-record-before-bin-sidecar removal ordering (hard constraint 5). +/// The json-record-before-bin-sidecar removal ordering (hard constraint 5), +/// asserted at **every** interruption point in one op's life rather than only +/// on the happy path. /// -/// Simulates a crash at the kill point *between* `remove_op` (the op record, -/// the "json" of the v1 journal) and `remove_staged_bytes` (the sidecar): -/// after reopen the op is durably gone while the sidecar survives as a -/// harmless orphan — never the dangerous inverse (an op record referencing a -/// sidecar that is already gone). Orphan-sidecar GC then reclaims it. +/// After each kill point the store is dropped and reopened, and the surviving +/// state must never be the dangerous inverse — an op record whose staged +/// sidecar is already gone. What it may leave is an orphan sidecar, which +/// orphan-sidecar GC ([`StagingStore::staged_keys`] + `remove_staged_bytes`) +/// reclaims. #[test] fn staging_store_removal_ordering_leaves_only_a_reclaimable_orphan() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("staging"); + // Kill point 1: bytes staged, op not yet journaled. block_on(async { let store = FileStagingStore::open(&path).unwrap(); - let op_id = store.enqueue_op(b"update-content-op").await.unwrap(); store .put_staged_bytes(b"chunk-key", b"sealed-ciphertext") .await .unwrap(); + }); + assert_survivors( + &path, + Survivors { + op: false, + sidecar: true, + }, + ); + + // Kill point 2: op journaled, nothing removed yet. + let op_id = block_on(async { + let store = FileStagingStore::open(&path).unwrap(); + store.enqueue_op(b"update-content-op").await.unwrap() + }); + assert_survivors( + &path, + Survivors { + op: true, + sidecar: true, + }, + ); - // Engine completes the op: op record removed durably FIRST... + // Kill point 3: op record removed, sidecar not yet — the orphan. + block_on(async { + let store = FileStagingStore::open(&path).unwrap(); store.remove_op(op_id).await.unwrap(); - // ...and the process dies here, before remove_staged_bytes. }); + assert_survivors( + &path, + Survivors { + op: false, + sidecar: true, + }, + ); - // Reopen (post-"crash"): the op is gone, the sidecar is an orphan. + // Kill point 4: GC reclaims the orphan and the budget goes back to zero. block_on(async { - let reopened = FileStagingStore::open(&path).unwrap(); - assert!( - reopened.queued_ops().await.unwrap().is_empty(), - "the op record must be durably gone after remove_op" - ); + let store = FileStagingStore::open(&path).unwrap(); assert_eq!( - reopened.staged_bytes(b"chunk-key").await.unwrap(), - Some(b"sealed-ciphertext".to_vec()), - "the sidecar must survive as a harmless orphan, never a dangling op" + store.staged_keys().await.unwrap(), + vec![b"chunk-key".to_vec()] ); + store.remove_staged_bytes(b"chunk-key").await.unwrap(); + assert_eq!(store.staged_bytes_total().await.unwrap(), 0); + }); + assert_survivors( + &path, + Survivors { + op: false, + sidecar: false, + }, + ); +} - // Orphan-sidecar GC reclaims it. +/// What one kill point expects to find after the store is reopened. +struct Survivors { + op: bool, + sidecar: bool, +} + +/// Reopens the staging store and asserts exactly what survived the kill point. +fn assert_survivors(path: &std::path::Path, expected: Survivors) { + assert!( + !expected.op || expected.sidecar, + "no kill point may expect an op record without the sidecar it references" + ); + block_on(async { + let store = FileStagingStore::open(path).unwrap(); assert_eq!( - reopened.staged_keys().await.unwrap(), - vec![b"chunk-key".to_vec()] + !store.queued_ops().await.unwrap().is_empty(), + expected.op, + "op record survival" ); - reopened.remove_staged_bytes(b"chunk-key").await.unwrap(); - assert!(reopened.staged_keys().await.unwrap().is_empty()); - assert_eq!(reopened.staged_bytes_total().await.unwrap(), 0); + let staged = store.staged_bytes(b"chunk-key").await.unwrap(); + assert_eq!(staged.is_some(), expected.sidecar, "sidecar survival"); + if expected.sidecar { + assert_eq!(staged.as_deref(), Some(&b"sealed-ciphertext"[..])); + } }); }