fix: harden the desktop floor store read-modify-write and intent length prefix

[FSM1]

Closes #704

Retro /security-review hardening on crates/desktop-seams/src/floor_store.rs. Both items are LOW / defence-in-depth. I re-verified the reachability claims rather than taking them on trust — both hold — so this makes the store's monotonic-floor invariant structural rather than incidental, without changing observable behaviour.

1 — Unserialized read-modify-write in raise_floor

raise_floor read the stored floor, took max, then wrote, with nothing serializing the two halves. Two concurrent raises on one key could interleave read/read/write-high/write-low and durably regress the floor. That loss is unhealable: both commits return Ok and clear their own intent records, so replay has nothing left to roll forward.

Fix: a Mutex<()> on FileFloorStore, held across the whole RMW in raise_floor — floor_store.rs:38-42 and :88-92. A poisoned lock fails closed with a SeamError.

Per-key serialization rather than per-batch is deliberate and sufficient. Monotonic-max is a per-key invariant, and the seam contract makes the desktop store roll-forward rather than transactional across keys, so an interleave between the keys of two batches heals idempotently on replay. apply_raises and replay_intents inherit the lock through raise_floor; there is one acquisition site, so no nesting and no deadlock edge. The guard never spans an .await — raise_floor is a sync fn.

Scope boundary: the lock is in-process, per handle. Two FileFloorStore handles over the same directory, or two processes, are not serialized by it. The engine constructs one store per session, and cross-process file locking is well beyond this issue.

2 — 32-bit intent overflow in intent_slice

intent_slice computed start + len on a len taken from the corrupt-controlled 4-byte length prefix, which can wrap usize on a 32-bit target and panic in debug rather than returning the clean corrupt_intent() error. Fix: start.checked_add(len).and_then(|end| bytes.get(start..end)) — floor_store.rs:223-231.

While there, the encode side had the mirroring gap that AGENTS.md rule 8 requires closing: encode_intent wrote raise.key.len() as u32, a silently truncating cast. A truncated prefix does not merely produce a record the decoder rejects — it reframes the record, so the decoder reads a short key and then parses key bytes as the next raise's value. encode_intent now returns SeamResult<Vec<u8>> and rejects through intent_key_len — floor_store.rs:190-197. Release-active Err, no debug_assert!.

Reachability — verified, neither is live

• Item 1: Scheduler spawns via tokio::task::spawn_local (scheduler.rs:19, :49), which requires a LocalSet on a current-thread runtime, so engine tasks are single-threaded. raise_floor is a sync fn with no .await between read and write, so a task cannot yield mid-RMW even under a multi-threaded executor. FileFloorStore is also not yet wired into a shell — the only in-tree constructions are the lib.rs re-export and the conformance suite.
• Item 2: 32-bit is not a supported desktop target, and even the panic is fail-closed.
• The encode-side truncation needs a 4 GiB key; keys are scope ids and ipnsName bytes.

Tests

• concurrent_raises_never_regress_a_floor — 32 keys, two barrier-synced threads per key raising 2 and 1. Negative control: with the mutex removed this fails immediately with a durable Some(1) where Some(2) is required, reproducing the exact regression the issue describes. It passes deterministically with the fix.
• intent_slice_fails_closed_on_an_overflowing_end — the usize boundary, on any pointer width.
• decode_rejects_an_oversized_key_length — a record claiming u32::MAX key bytes.
• encode_rejects_a_key_longer_than_its_length_prefix — the u32::MAX encode boundary. intent_key_len takes a u64 precisely so this is testable without a 4 GiB allocation, and so the test is pointer-width independent.

All four are ordinary #[test]s, so they fire in a release build.

Gates

All exit 0, rebased onto main at 20d03b8, with a per-worktree CARGO_TARGET_DIR:

• cargo fmt --all --check
• cargo clippy --workspace --all-targets -- -D warnings
• cargo check --workspace --all-targets
• cargo test -p cipherbox-engine -p cipherbox-core -p cipherbox-desktop-seams — all green; cipherbox-desktop-seams 19 lib + 13 conformance passed
• cargo check -p cipherbox-wasm --target wasm32-unknown-unknown

Parallel work

Confined to floor_store.rs. It does not touch fs_util.rs, staging_store.rs, or tests/conformance.rs, so it does not collide with #952.

Summary by CodeRabbit

• Bug Fixes

  • Prevented concurrent floor updates from overwriting newer persisted values.
  • Added validation for malformed or oversized intent data.
  • Improved handling of overflow conditions by failing safely instead of attempting invalid reads.

• Tests

  • Added coverage for concurrent updates, oversized lengths, and arithmetic overflow scenarios.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 41 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

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: 1797b746-f3b8-4234-b94f-c4cc8ca2df06

📥 Commits

Reviewing files that changed from the base of the PR and between c39e455 and 5a097f2.

📒 Files selected for processing (1)

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

Walkthrough

The floor store now serializes concurrent floor raises. Intent encoding rejects oversized keys, and intent decoding checks for slice-bound overflow. Tests cover concurrency, oversized lengths, overflow, round trips, and recovery setup.

Changes

Floor store hardening

Layer / File(s)	Summary
Intent encoding and decoding bounds crates/desktop-seams/src/floor_store.rs	encode_intent now returns errors for unrepresentable key lengths. Intent decoding uses checked slice bounds. Round-trip, corruption, and recovery tests handle fallible encoding and invalid lengths.
Serialized floor updates crates/desktop-seams/src/floor_store.rs	A per-store mutex now serializes raise_floor read-modify-write operations. Tests verify that concurrent raises preserve the higher floor.

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

Possibly related PRs

• FSM1/cipher-box#690: Introduced the floor-store intent mechanism that this change further hardens.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes implement both requirements from issue #704: serialized floor RMW operations and checked intent length arithmetic.
Out of Scope Changes check	✅ Passed	All reported changes are limited to floor_store.rs and directly support the objectives in issue #704.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main changes: hardened floor-store read-modify-write operations and intent length-prefix validation.

✨ 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/704-floor-store-rmw-hardening

---

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/floor_store.rs (1)

298-322: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift

Make the concurrency regression deterministic.

The barrier only synchronizes the start of both raises. Without write_lock, either raise can complete its full read-modify-write before the other starts its read. The test can then pass without the lock.

Add a test-only hook after read_floor. Pause the low raise there, start the high raise, and verify that the high raise cannot complete until the low raise proceeds. Then assert that the persisted floor is Some(2). This fails when the critical section does not cover both the read and the write.

🤖 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/floor_store.rs` around lines 298 - 322, Make
concurrent_raises_never_regress_a_floor deterministic by adding a test-only hook
immediately after read_floor that pauses the low raise. Coordinate the test to
release the low raise only after starting the high raise, and assert the high
raise cannot complete while the low raise is paused; then release it and verify
the persisted floor is Some(2), ensuring the read-modify-write critical section
is locked.

🤖 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/floor_store.rs`:
- Around line 298-322: Make concurrent_raises_never_regress_a_floor
deterministic by adding a test-only hook immediately after read_floor that
pauses the low raise. Coordinate the test to release the low raise only after
starting the high raise, and assert the high raise cannot complete while the low
raise is paused; then release it and verify the persisted floor is Some(2),
ensuring the read-modify-write critical section is locked.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fa92db3b-2ba8-48a2-bce0-dfa631c9146e

📥 Commits

Reviewing files that changed from the base of the PR and between 7a17c9f and c39e455.

📒 Files selected for processing (1)

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

[FSM1]

Review sweep — rebased onto main, one body-only finding, declined with measurements

Rebased onto main at 20d03b8 (8 commits). No textual conflict, and no semantic one either: the diff is confined to crates/desktop-seams/src/floor_store.rs, which main did not touch in that range — #952 landed in desktop-seams but in fs_util.rs/staging_store.rs. The branch constructs no preserved-unknowns field and implements no seam trait whose members moved, so neither the PreservedFields change (#957) nor the HttpSeam surface reaches it.

Gates after the rebase, all exit 0:

• 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
• cargo test -p cipherbox-engine -p cipherbox-core -p cipherbox-desktop-seams — 1133 passed, 0 failed (cipherbox-desktop-seams 19 lib + 13 conformance)

Sweep covered reviewThreads (zero inline threads), the submitted review bodies, and the issue comments. One reviewer (CodeRabbit); no Greptile review. One finding, in the review body only.

Declined — "Make the concurrency regression deterministic" — floor_store.rs:298-322, CodeRabbit nitpick, body-only

The claim is that concurrent_raises_never_regress_a_floor "can pass without the lock" because the barrier only synchronizes the start of the two raises, and the fix is a test-only hook after read_floor to pause the low raise.

I measured it rather than reasoning about it. Removing only the write_lock acquisition from raise_floor and running the test 40 times:

• lock removed: 0 passed, 40 failed — every run trips left: Some(1), right: Some(2) at floor_store.rs:313
• lock present: 40 passed, 0 failed

The test discriminates on the lock perfectly, so the premise does not hold in practice. The reason it is not a coin flip is asymmetry in the two halves of the race: the barrier releases both threads into read_floor, a single small open-read-close, while the half that has to win for the test to pass falsely is atomic_write — temp file, write, sync_all, rename, parent-dir fsync. The read/read interleave is not a narrow window, it is the overwhelmingly likely schedule, and the assertion only needs 1 of the 32 independent keys to lose.

The more important property: this test cannot go flaky-red. With the lock in place the assertion is guaranteed by the mutex under every schedule, so no CI run can fail on it. The only thing at stake is its strength as a regression detector, and that is measured above at 40/40.

Against that, the proposed fix costs a #[cfg(test)] callback field on FileFloorStore and a branch inside the production raise_floor whose sole consumer is one test — the speculative-machinery-in-production shape AGENTS.md and /simplify reject. It would also weaken what the test proves: a forced pause asserts that the hook fires where it was planted, not that the critical section spans the read and the write. CodeRabbit tags the item Trivial / Heavy lift itself.

No code change. Happy to revisit if the test is ever observed green on a build without the lock.

Also considered and left alone

apply_raises takes the mutex once per key rather than once per batch, so a batch is N acquisitions and two concurrent commit_floors can interleave between keys. Hoisting the lock to the batch would need an inner non-locking raise_floor to avoid self-deadlock on the non-reentrant std::sync::Mutex, and it buys no correctness: floors are monotonic-max per key, so any interleaving of two batches converges on the same per-key maxima, a concurrent reader can only observe an earlier valid floor, and crash recovery is roll-forward. The acquisitions are uncontended nanoseconds against a millisecond fsync, so it is not the cost centre either. The extra indirection would be the change /simplify pushes back on.