feat: implement the fuse content write path over sealed spill files - #1156
Conversation
WalkthroughThe FUSE crate now supports encrypted per-handle spill files and staged content writes. Pending writes are read locally and committed through one durable ChangesFUSE content write path
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FUSEClient
participant OperationCore
participant SpillFile
participant Engine
participant StagingStore
FUSEClient->>OperationCore: write(handle, offset, data)
OperationCore->>SpillFile: store encrypted pending blocks
FUSEClient->>OperationCore: flush or release(handle)
OperationCore->>Engine: commit updateContent
Engine->>StagingStore: journal durable operation
StagingStore-->>OperationCore: acknowledge durable journal
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Kernel writes land in a per-handle sealed spill file — XChaCha20-Poly1305 under a random key minted from the injected entropy seam, held only in process memory and gone when the handle closes. A crash leaves ciphertext nothing can open, replacing v1's plaintext cb-write-* exposure class. flush, fsync and release turn what a handle holds into exactly one updateContent op, assembled block by block from the spill, the base version, and the zeros a hole reads as, so peak plaintext is one block however large the file. The kernel is acked only once that op is durable in the StagingStore — a queue that cannot journal refuses the write instead of losing it. truncate on an open writable handle is a spill-file operation folded into that handle's op; with no handle it becomes its own op, so a bare truncate(2) is never silently lost. Writes on a read-only handle are refused rather than accepted and dropped, and a partial write over a version whose length is unprojected fails closed rather than guessing zero and dropping the file's untouched tail. Closes #1110
Review findings from this PR's own gates. A shrink recorded no floor on the base version, so any range at or past the new length that a later write or regrow re-covered was served from the version being replaced instead of reading as the hole it is. Bytes a member truncated away were re-sealed into the next version and published. Pending now carries base_len, clamped by every truncate and consulted by the assembler, which also collapses the Base enum away. Also: a zero-length write no longer extends the file, a spill closes its file handle before unlinking so the Windows leg really leaves nothing behind, spill files are named from entropy rather than a per-area counter so two areas over one directory cannot unlink each other's live spill, a slot is claimed before its bytes land so a half-written slot fails closed rather than falling back to the base version, and a zero block size is refused at creation.
90bc157 to
d312a25
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/fuse/src/spill.rs (1)
252-270: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDocument why the non-unix arms carry no permission enforcement.
On Windows
restrict_dirreturnsOk(())andopen_privatesets no ACL, so the spill directory and file inherit the parent's permissions. The confidentiality argument still holds, because the file holds ciphertext under a memory-only key. State that argument here. The unix arms are self-explanatory from the mode bits; these arms are not, and the blueprint lists WinFsp as a shipping backend.♻️ Proposed comment
#[cfg(not(unix))] +/// No mode bits outside unix. The spill holds ciphertext under a key this +/// process never writes down, so confidentiality does not rest on the file +/// permissions; the directory only inherits the account root's ACL. fn open_private(path: &Path) -> io::Result<File> { OpenOptions::new() .read(true) .write(true) .create_new(true) .open(path) }🤖 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/fuse/src/spill.rs` around lines 252 - 270, Add comments to the non-unix `open_private` and `restrict_dir` implementations explaining that inherited Windows permissions are intentional because spill files contain ciphertext encrypted with a memory-only key; note that WinFsp is a supported backend. Leave the Unix implementations and behavior unchanged.crates/fuse/src/ops.rs (1)
589-615: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
version_blockbypasses the chunk cache, so base blocks are re-fetched on every read and again on commit.
readconsultsChunkCacheand inserts each fetched block.version_blockcallsengine.read_streamdirectly and never reads or fills the cache. Two consequences follow:
- Once a handle holds pending writes,
readroutes toread_pending, so every repeated read of an untouched region re-fetches that region from the engine.push_versioncallsversion_blockfor every block of the file, so committing a one-byte patch to a large file re-fetches every untouched block from the engine.The cache is keyed by
(StreamHandle, index)andversion_blockalready resolves the stream throughstream_for, so the same key is available here. Consider routing the base-version read through the cache.🤖 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/fuse/src/ops.rs` around lines 589 - 615, Update version_block to route base-block reads through ChunkCache using the resolved StreamHandle and block index, reusing cached blocks and inserting fetched blocks when absent. Preserve the existing handle validation, spill overlay, truncation clamping, zero-filled output, and block-length behavior while replacing the direct engine.read_stream path.
🤖 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.
Inline comments:
In `@crates/fuse/src/ops.rs`:
- Around line 643-646: Update the documentation comment for the read stream
pinning handle to remove the claim that writes replacing every block avoid the
resolve and stream slot. State only the accurate behavior supported by the
current begin_pending, base_len, and stream_for flow, preserving the rest of the
comment.
- Around line 536-541: Update the successful commit_write flow around the
pending dirty reset and Invalidation::Attributes call to first issue
Invalidation::Data { ino } for the same inode. Preserve the existing attribute
invalidation and return behavior.
In `@crates/fuse/tests/fuse_op_core.rs`:
- Around line 918-926: Narrow the never-block test comment and name to cover
only created handles, then add a test for a reopened existing file with a
projected size. Use the existing mount/open and projection setup helpers, poll
the first write on that reopened handle, and assert it is immediately
Poll::Ready without invoking resolution.
---
Nitpick comments:
In `@crates/fuse/src/ops.rs`:
- Around line 589-615: Update version_block to route base-block reads through
ChunkCache using the resolved StreamHandle and block index, reusing cached
blocks and inserting fetched blocks when absent. Preserve the existing handle
validation, spill overlay, truncation clamping, zero-filled output, and
block-length behavior while replacing the direct engine.read_stream path.
In `@crates/fuse/src/spill.rs`:
- Around line 252-270: Add comments to the non-unix `open_private` and
`restrict_dir` implementations explaining that inherited Windows permissions are
intentional because spill files contain ciphertext encrypted with a memory-only
key; note that WinFsp is a supported backend. Leave the Unix implementations and
behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d450906e-4f3b-49b0-b746-6712332a3fd7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
blueprint/desktop.mdcrates/desktop-seams/src/lib.rscrates/desktop-seams/src/paths.rscrates/fuse/Cargo.tomlcrates/fuse/src/cache.rscrates/fuse/src/lib.rscrates/fuse/src/ops.rscrates/fuse/src/spill.rscrates/fuse/tests/fuse_op_core.rs
A successful commit_write pushed only an attributes invalidation, so the kernel kept serving the pre-commit pages it already held for that inode — nothing re-binds a stream to notice the new version. Push a data invalidation first, so the pages are gone before the kernel learns the new size and could serve them as the new version. Narrow the stream_for doc: a writer skips the resolve only over an already-projected size, since an unprojected one is what the resolve yields. Narrow the never-block write test to created handles, which is all it proved, and add the reopened-handle case over a projected size. State the confidentiality argument on spill.rs's non-unix arms, where the mode bits do not make it for themselves.
|
The two nitpicks from the review body, which have no thread to reply on.
The re-fetch is confirmed: The staleness worry does not apply — What stopped me from doing it in this PR is two other things:
So the fix is a semantics change on the clamp this branch's head commit had just corrected, plus a new cache mode — not a small one, and not one I want landing untested alongside the invalidation fix. #1168 carries both constraints and the acceptance criteria. |
1 participant
What this is
The fuse content write path:
write,flush,fsync,truncateand areleasethat journals. Before this,releaseclosed a handle and emitted zero ops, and nothing incrates/fuseever reachedbegin_write/push_chunk/commit_write.The spill file
Bytes land in a per-handle sealed spill file in the engine data dir, exactly as
blueprint/desktop.md"Reads, writes, and the never-block law" specifies:SpillArea::open(dir, entropy)) — the projection never calls an RNG.nonce || ciphertext||tag, with the block index in the AAD so a slot transplanted to another offset fails to open.cb-write-*class is gone structurally, so the spill is deleted rather than zero-overwritten.SpillArea::opensweeps a previous run's debris.Sealing uses
crates/core's AEAD; the fuse crate implements no crypto of its own.Ops and the ack
flush/fsync/releaseturn what a handle holds into exactly oneupdateContentop. The version is assembled block by block — the spill's block if it took one, else the base version's bytes, else the zeros a hole reads as — and fed throughpush_chunk, so peak plaintext is one block however large the file. A block a write replaces whole never fetches the version it replaces.The kernel is acked only once
commit_writehas journaled the op durably in the StagingStore. A queue that cannot journal makesflushandreleasefail, and the op queue is left exactly as it was — no half-formed op, no false ack.Decisions worth reviewing
truncateon an open writable handle is a spill-file operation, folded into that handle's one op; with no handle it becomes its own op, so a baretruncate(2)is never silently lost.O_TRUNCis the first form: the adapter opens the handle, then truncates it to zero. Truncating to zero resolves nothing, so> fileneeds no network. A shrink also lowers the floor the base version may contribute from, so bytes it removed never come back if the file grows again.updateContentis staged but not yet published is refused until it publishes, because the engine's content read plane resolves published heads only.lookup/getattrreport the length those writes leave, so a program that stats or re-reads what it just wrote does not see the old file.Access::writablepreviously had no caller to do.Coverage
fuse-op-coregains the write path end to end, plus spill unit tests incrates/fuse/src/spill.rs(the gate runscargo test -p cipherbox-fuse, so both are in it):updateContentfail_enqueue_after(0)outage refuses the write rather than acking it, leaving the queue untouchedWhat the review gates changed
The three mandatory gates ran on this diff and found one real defect plus a set of hardening items, all folded in:
ftruncate(fd, 4), any range at or past 4 that a later write or regrow re-covered was served from the version being replaced, so bytes a member truncated away were re-sealed into the next version and published.Pendingnow carries abase_lenclamped by every truncate, which the assembler consults — and which collapses the oldBaseenum away. Two regression tests over a real published version cover it.Rejected with reason: binding a slot generation into the AAD. It defends only against an attacker who can write the
0600spill file, who by the same access can read the plaintext out of the mount or the process — the local uid is not a boundary this system defends.blueprint/desktop.mdsaidcrates/fusedepends on the engine facade alone, which its own writes section contradicts by requiring the FS core to seal spill files. The narrower statement is amended to carve out core's AEAD.Not verified here
The FUSE mount itself cannot be exercised in this environment — macFUSE-versus-FUSE-T linking breaks a local mount — so no kernel drove these paths. What needs a real mount: that a host adapter maps
O_TRUNCto open-then-truncate, thatclose(2)surfaces the flush error, and the spill's behaviour under a real 1 MiB framing block rather than CI's 16-byte one.SpillAreaalso has no production construction site yet — there is no host adapter implementation either, so the mount is not wired at the Tauri shell. Whoever lands that must construct the spill area from the same productiongetrandomsource the engine uses; the counter-nonce argument rests on it.Closes #1110
Note
Implement write path for FUSE content using sealed per-handle spill files
SpillAreaandSpillFileto manage a per-account spill directory with per-handle XChaCha20-Poly1305 AEAD encryption; keys are in-memory only and files are swept on mount open.OperationCorewithwrite,truncate,flush, andfsyncoperations that buffer writes to spill files and commit a singleupdateContentop per handle on flush or release.attributes()reports the projected size for unjournaled writes.spill_dirpath utility todesktop-seamsand promotescipherbox-coreto a runtime dependency of the fuse crate.releaseis now async; callers must be updated accordingly.Macroscope summarized 2ace6f6.
Summary by CodeRabbit
New Features
Bug Fixes