fix: barrier desktop store unlink ordering where directories cannot be fsynced

crates/desktop-seams/src/fs_util.rs

@@ -48,44 +48,37 @@ 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) => {
             let _ = fs::remove_file(&tmp);
             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(()) => {}
         Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(()),
         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<Vec<String>> {
     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<PathBuf> {
+    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 (`<pid>.<seq>`), 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();

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,

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"[..]));
+        }
     });
 }