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

[FSM1]

Problem

FileStagingStore documents removal ordering as a correctness property (desktop-seams hard constraint 5): the op record goes first, its staged-ciphertext sidecar second, so an interruption can only orphan a sidecar — never leave an op record naming a sidecar that is already gone. remove_file_durable enforces that by fsyncing the parent directory after each unlink.

fsync_dir was Ok(()) on non-Unix. Windows has no directory fsync, so on that platform the ordering was never barriered at all and the store silently reported a durability it had not obtained.

The over-claim is wider than the issue recorded. #665 assumed atomic_write was still covered on Windows by MOVEFILE_WRITE_THROUGH, per the comment in fs_util.rs. That comment is wrong. Rust std's Windows rename is MoveFileExW(old, new, MOVEFILE_REPLACE_EXISTING) — write-through is not among the flags (library/std/src/sys/fs/windows.rs, fn rename, checked against the 1.88 sources this workspace builds with). So the rename was unbarriered too, and the fix has to cover both barriers, not just unlinks.

Change

fsync_dir on non-Unix now calls metadata_log_barrier: write a byte to a fresh temp file in the same directory and sync_all it. NTFS journals metadata to a per-volume write-ahead log flushed as an LSN-ordered prefix, so flushing a transaction issued after a directory-entry change also persists that change. The temp carries the existing .cbtmp. prefix, so it is invisible to list_file_names and a crash before its removal leaves debris ensure_dir already sweeps — the harmless orphan, in line with the same principle the store's ordering rule serves.

The barrier is plain std::fs; the crate is #![forbid(unsafe_code)] and no Win32 call was added. It is compiled under cfg(any(not(unix), test)), so the Windows algorithm is exercised by the Unix CI legs too rather than only by the Windows one.

Also in the diff:

• atomic_write and metadata_log_barrier share a write_synced_temp helper instead of two copies of create/write/sync_all.
• A barrier failure in remove_file_durable is labelled unlink barrier: …, so the new Windows-only failure mode (the barrier's File::create hitting ENOSPC or a read-only directory after the unlink already succeeded) is diagnosable rather than surfacing as a bare staging_store remove_op error.
• The over-claiming comments in fs_util.rs and on FileStagingStore are corrected; the store's doc now cross-references fs_util::fsync_dir rather than restating the platform mechanism.

Dependencies

The issue carries no dependency statement. Established from its body and the surrounding desktop slices:

• No blocking prerequisite. crates/desktop-seams is a leaf adapter crate. The change is confined to its private fs_util module and touches no seam trait, no engine code, and no wire format. It is independently mergeable.
• Depends on desktop: implement the desktop seam set against the conformance kits #646 only historically — the seam set whose ship review surfaced this. That work is landed.
• Not fixable a layer down. FileStagingStore stores op bytes verbatim and never parses them, so it cannot know which sidecar an op references and cannot repair the dangerous state at reopen. An ordering barrier is the only enforcement available at this layer.

Relationship to #941 and #950

Both are open against the engine-side staged-block lifecycle; this is the desktop seam underneath them. No overlap in files, and the rules do not contradict:

• fix: mark a leaf uploaded before releasing its staged bytes #941 (issue engine: a crash between releasing a staged leaf and marking it destroys a successful upload #924, crates/engine/sync/drain.rs) reorders mark-before-release for a leaf so an interruption leaves a marked-and-staged leaf rather than a released-but-unmarked one, and cites this store's hard constraint 5 by name as the rule it is applying. This PR makes that same rule actually hold on Windows, where the barrier it depends on was absent. Strictly reinforcing.
• feat: cancel an upload and collect orphaned staged blocks #950 (issue engine: cancel an upload and collect orphaned staged blocks #869) adds cancel plus orphan-staged-block GC. Its GC is the reclaimer for exactly the orphan this store's ordering is designed to leave behind, and it decides which blocks are collectible from the queued op records — which this change makes more trustworthy on Windows. No shared file; the staging_store_removal_ordering_leaves_only_a_reclaimable_orphan test name fix: mark a leaf uploaded before releasing its staged bytes #941's body references is preserved.

Tests

crates/desktop-seams/src/fs_util.rs:

• metadata_log_barrier_fails_closed_when_it_cannot_be_established — the revert guard. Restoring the old Ok(()) body makes it fail on every platform, verified locally. It pins that the barrier is a real operation with real failure modes, so an unbarriered removal is an error rather than a silent fast path.
• metadata_log_barrier_leaves_the_directory_as_it_found_it — successive barriers neither collide on a temp name nor accumulate debris, and do not disturb neighbouring files. This one is a residue guard, not a revert guard: it passes against the old no-op too.

crates/desktop-seams/tests/conformance.rs:

• staging_store_removal_ordering_leaves_only_a_reclaimable_orphan now walks all four interruption points of one op's life — bytes staged before the op is journaled, op journaled, op record removed before the sidecar, sidecar reclaimed — reopening the store at each and asserting exactly what survived. A Survivors precondition rejects any kill point that expects an op record without its sidecar, so the forbidden state cannot be encoded into the table.

Verification

All exit 0 on macOS (aarch64, Rust 1.88): cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, cargo check --workspace --all-targets, cargo check -p cipherbox-wasm --target wasm32-unknown-unknown --all-targets, cargo test -p cipherbox-desktop-seams in both debug and --release, pnpm -r --if-present run typecheck, pnpm -r --if-present run test, eslint ..

Beyond that, the non-Unix path was forced on locally — fsync_dir temporarily rewired so metadata_log_barrier was the only implementation — and the whole cipherbox-desktop-seams suite passed against it. That proves the Windows code path compiles and is semantically correct as a barrier-shaped operation.

What only the Windows CI leg can confirm, stated plainly:

• That the #[cfg(not(unix))] arm compiles and links on x86_64-pc-windows-msvc. Cross-checking from macOS is not possible here: cargo check --target x86_64-pc-windows-msvc fails in blake3's build script for want of ml64.exe. The authoritative check is the Cargo Check & Test (Windows) job in ci.yml, which runs cargo test --workspace on windows-latest and therefore executes both new tests against the real production path.
• That the barrier behaves on a real NTFS volume — file creation in the store directories, and the temp's removal, under Windows sharing semantics and any AV or indexer holding handles.

What no CI leg can confirm, and I am not claiming: that the NTFS LSN-ordered-log-prefix premise actually delivers the ordering. Proving it needs power-loss or crash-injection testing on real Windows hardware, which is out of reach of this repo's suites. The tests here assert the barrier's shape and its fail-closed behaviour, not the durability guarantee itself. This is a strict improvement over the previous no-op regardless — the old code obtained no ordering under any model.

Desktop E2E is not required for this change; no FUSE or shell surface is touched.

Windows cost, accepted deliberately. Every atomic_write and every remove_file_durable on Windows now pays an extra file create + write + FlushFileBuffers + unlink. Scoping the barrier to unlinks only was considered and rejected: with MOVEFILE_WRITE_THROUGH absent from std's rename, the write path needs it too. Batching the barrier across the delete loops in snapshot_cache and floor_store, and skipping it in ensure_dir when the sweep removed nothing, are real wins but sit outside this diff.

Review gates

• /security-review — no HIGH or MEDIUM findings. The diff adds no untrusted input, no crypto, no network surface and no secret handling; the barrier file carries a single NUL byte and never sees key or sealed material. Failure propagates fail-closed, so a failed barrier fails the removal instead of letting the caller proceed to the sidecar.
• /simplify — findings folded in: the duplicated temp-write block extracted to write_synced_temp; the barrier doc cut from 13 lines to 6; the platform mechanism no longer restated on FileStagingStore; a tautological assertion in the test replaced with a real precondition on the kill-point table; positional bools replaced with a named Survivors struct; absent-versus-corrupted sidecar states no longer conflated. One finding was rejected on evidence — scoping the barrier away from atomic_write rests on the MOVEFILE_WRITE_THROUGH claim, which the std sources disprove. One accepted change was then reverted: barriering the already-absent remove_file_durable path closes a retry-after-barrier-failure window, but costs a directory fsync per already-removed leaf on the per-block GC path, which is not a trade worth making here.
• /crypto-privacy-review — not applicable; the diff touches no key material and no sealed bytes.

Closes #665

Summary by CodeRabbit

• Bug Fixes

  • Improved durability when saving, updating, and removing staged data, helping ensure changes persist correctly across interruptions or system failures.
  • Strengthened cleanup behavior so incomplete operations and leftover temporary data are handled safely.
  • Added safeguards to detect and report situations where durable filesystem updates cannot be completed.

• Tests

  • Expanded coverage for interrupted staging operations, recovery scenarios, orphaned data, and final cleanup states.

[coderabbitai]

Warning

Review limit reached

@FSM1, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 48 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b5d238d9-f5c7-4dc1-8c65-38397100fcf8

📥 Commits

Reviewing files that changed from the base of the PR and between 649df0b and 8798038.

📒 Files selected for processing (1)

• crates/desktop-seams/src/fs_util.rs

Walkthrough

The change adds durable metadata barriers for non-Unix filesystems, shares synced temporary-file logic, documents StagingStore removal ordering, and tests recovery across four interruption points.

Changes

Staging durability

Layer / File(s)	Summary
Filesystem barrier implementation crates/desktop-seams/src/fs_util.rs	atomic_write uses shared synced temporary-file logic. Non-Unix fsync_dir writes and removes a synced marker. Unlink barrier errors include context. Tests verify cleanup and failure handling.
Staging recovery ordering validation crates/desktop-seams/src/staging_store.rs, crates/desktop-seams/tests/conformance.rs	Documentation defines directory-entry barriers for mutating methods. Conformance tests cover four crash points and verify that operation records never reference missing sidecars.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant StagingStore
  participant fsync_dir
  participant Filesystem
  participant RecoveryTest
  StagingStore->>fsync_dir: barrier directory entry
  fsync_dir->>Filesystem: write and sync temporary marker
  fsync_dir->>Filesystem: remove temporary marker
  StagingStore->>Filesystem: remove operation record and sidecar
  RecoveryTest->>StagingStore: reopen after interruption
  StagingStore-->>RecoveryTest: surviving operations and sidecars

Loading

Possibly related PRs

• FSM1/cipher-box#666: Introduces the implementation and tests that this change further hardens.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: adding a durability barrier for desktop store unlink ordering when directories cannot be fsynced.
Linked Issues check	✅ Passed	The changes implement issue #665 by adding a non-Unix metadata barrier, preserving unlink ordering, updating documentation, and expanding interruption tests.
Out of Scope Changes check	✅ Passed	All changes support issue #665 through shared durability logic, documentation updates, failure tests, cleanup tests, and staging-ordering coverage.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/665-windows-unlink-barrier

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/desktop-seams/src/fs_util.rs (1)

70-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider labeling the write-side barrier failure for the same diagnostic consistency.

remove_file_durable now wraps a fsync_dir failure with "unlink barrier: {err}" context, so an operator can tell that a durability barrier failed on removal. atomic_write (Line 66) calls fsync_dir(dir) too, but that failure surfaces with no equivalent context. After this change, a barrier failure on write/rename is harder to diagnose than one on unlink, even though both barriers protect the same crash-consistency guarantee.

♻️ Proposed fix for labeling consistency

     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}")))
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/desktop-seams/src/fs_util.rs` around lines 70 - 84, Update the
fsync_dir error handling in atomic_write to wrap failures with descriptive
write-side barrier context, matching the "unlink barrier: {err}" labeling used
by remove_file_durable. Preserve atomic_write’s existing behavior for successful
writes and other errors.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/desktop-seams/src/fs_util.rs`:
- Around line 70-84: Update the fsync_dir error handling in atomic_write to wrap
failures with descriptive write-side barrier context, matching the "unlink
barrier: {err}" labeling used by remove_file_durable. Preserve atomic_write’s
existing behavior for successful writes and other errors.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e6a1671b-674d-413b-a416-8d4b64dbcb57

📥 Commits

Reviewing files that changed from the base of the PR and between afb3887 and 649df0b.

📒 Files selected for processing (3)

• crates/desktop-seams/src/fs_util.rs
• crates/desktop-seams/src/staging_store.rs
• crates/desktop-seams/tests/conformance.rs

[FSM1]

Review of 649df0b9d triaged — one nitpick, body-only, no threads to resolve.

fs_util.rs:70-84 — label the write-side barrier failure

Applied in 879803837.

The asymmetry is real and this PR introduced it: adding "unlink barrier: {err}" to remove_file_durable left atomic_write's identical fsync_dir(dir) failure unlabelled, so after this change a barrier failure on write was harder to diagnose than one on unlink — for two barriers protecting the same crash-consistency guarantee.

fsync_dir(dir).map_err(|err| io::Error::new(err.kind(), format!("write barrier: {err}")))

Gates on the amended branch: cargo fmt --all --check clean, cargo clippy -p cipherbox-desktop-seams --all-targets -- -D warnings clean, cargo test -p cipherbox-desktop-seams 15 passed / 13 passed / 0 failed, 2 pre-existing real_keyring_* ignored.