Persist filesystem WAL writer epochs - #715
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
|
@codex please review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughChangesThe filesystem WAL now persists checksummed writer-epoch ledgers, acquires OS-backed writer leases, validates monotonic epoch chains, and reconciles recovery evidence. Trusted runtime integration derives store-specific epochs and uses in-memory rollback snapshots. Tests cover corruption, overlap, recovery, and external-action replay across processes. Filesystem WAL hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HostProcess
participant FilesystemWalStore
participant WriterEpochLedger
participant WAL
HostProcess->>FilesystemWalStore: acquire fresh writer epoch
FilesystemWalStore->>WriterEpochLedger: reload and reconcile evidence
FilesystemWalStore->>FilesystemWalStore: acquire writer lease
FilesystemWalStore->>WriterEpochLedger: persist successor epoch
HostProcess->>FilesystemWalStore: record request, claim, or settlement
FilesystemWalStore->>WAL: append and commit evidence
FilesystemWalStore->>WriterEpochLedger: persist final LSN and digest
HostProcess->>FilesystemWalStore: recover and replay settled result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/warp-core/src/causal_wal.rs (2)
6062-6064: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
truncate_tail_aftermutates segments without the writer lease.Every other mutating filesystem path in this PR gained
ensure_writer_lock()—append_frame(5977),flush_commit_with_capabilities(5848),rotate_segment(5910),publish_manifest(6071),close_epoch(6100).truncate_tail_aftercallsrewrite_filesystem_segments_after_truncation, which rewrites on-disk segment content, and is unguarded. A second process that opens the same root without acquiring the lease can therefore truncate committed tail data underneath the live epoch holder, and the epoch ledger'sfinal_lsn/final_commit_digestevidence for the active epoch silently stops matching the segments.🔒 Guard the truncation path
fn truncate_tail_after(&mut self, after_lsn: Lsn) -> Result<(), WalStoreError> { + self.ensure_writer_lock()?; rewrite_filesystem_segments_after_truncation(&self.root, after_lsn) }If recovery legitimately needs to truncate before an epoch is acquired, make that an explicit lease-acquiring recovery entrypoint rather than an unguarded port method.
🤖 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/warp-core/src/causal_wal.rs` around lines 6062 - 6064, Update truncate_tail_after to require the writer lease before calling rewrite_filesystem_segments_after_truncation, consistent with append_frame, flush_commit_with_capabilities, rotate_segment, publish_manifest, and close_epoch. If recovery needs pre-epoch truncation, route it through an explicit lease-acquiring recovery entrypoint instead of leaving this mutating method unguarded.
5872-5879: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPer-commit ledger rewrite triples fsyncs on the durable ACK path, and it's redundant.
Every
flush_commit_with_capabilitiesnow does: segment append +sync_all, thenwrite_writer_epoch_ledger_atomic→ tempsync_all+ rename +sync_directory_store. That's three durability barriers per committed transaction instead of one, plus a full re-encode of the entire closed-epoch history (which grows with every process restart).
reconcile_writer_epoch_closures(Line 8375) already reconstructsfinal_lsn/final_commit_digestfrom scanned commits at open, and the tests only depend on reopen-visible state. The closure evidence for the active epoch therefore does not need a synchronous ledger write per commit — persisting on epoch close (Line 6112) plus reconcile-at-open yields the same recovered ledger.Also note the in-memory
epoch_closuresinsert at Line 5872 is not rolled back when the persist at Line 5879 fails, unlike the careful restore inclose_epoch. If you keep the write, make the failure path symmetric.As per path instructions for
crates/warp-core/**: "Performance matters — flag unnecessary allocations in hot paths."🤖 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/warp-core/src/causal_wal.rs` around lines 5872 - 5879, The per-commit update in flush_commit_with_capabilities should stop calling persist_writer_epoch_ledger, avoiding redundant synchronous ledger rewrites; retain the in-memory epoch_closures update and rely on epoch-close persistence plus reconcile_writer_epoch_closures during reopen. If persistence is intentionally retained, make its error path restore the prior epoch_closures entry symmetrically with close_epoch.Source: Path instructions
🤖 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/warp-core/src/causal_wal.rs`:
- Around line 8236-8254: Bound the retained history in WriterEpochLedger during
the decode/append flow around closed_epochs, preserving only the latest closed
epoch plus a bounded window while folding older entries into a chain digest.
Update validate_writer_epoch_request and ledger serialization/reopen logic to
validate successors from the retained state and digest without scanning the full
lifetime history. Ensure compaction occurs under the existing lease and
preserves deterministic reopen validation.
- Around line 5740-5791: Update the writer-epoch creation flow around
derive_filesystem_writer_epoch_evidence so process_identity and host_identity
incorporate explicit caller-supplied host/process identity, while preserving
deterministic derivation and existing chain-position inputs. Ensure
storage_fencing_token and lease_or_lock_evidence are likewise bound to the
supplied identity where they represent takeover evidence, and thread the new
identity input through the relevant caller APIs instead of reading global state,
randomness, or system time. Update the associated WAL documentation and
CHANGELOG wording only if the implementation instead remains purely
chain-derived.
- Around line 5669-5692: Update persist_writer_epoch_ledger and the write/encode
path it invokes to borrow the existing epoch ledger data instead of constructing
an owned WriterEpochLedger via writer_epoch_ledger; preserve writer_epoch_ledger
unchanged for rollback snapshots, and ensure encoding accepts read-only
references while retaining the current persisted output.
In `@crates/warp-core/src/trusted_runtime_host.rs`:
- Around line 2135-2138: In the rollback path around runtime_wal_before, do not
use ? when the in-memory snapshot is unavailable, because returning there skips
the remaining rollback actions. Restore or record the WAL rollback failure while
continuing through the rollback of self.runtime, self.provenance,
echo_operation_action_outcomes, and admitted_echo_operation_actions, then return
the appropriate TrustedRuntimeWalError after all rollback state has been
processed.
In `@crates/warp-core/tests/causal_wal_hardening_tests.rs`:
- Around line 1126-1147: Extract a shared run_child_phase helper for
crates/warp-core/tests/causal_wal_hardening_tests.rs:1126-1147 and
crates/warp-core/tests/external_action_protocol_tests.rs:987-1018 that spawns
each child with --test-threads=1, polls try_wait() until the supplied deadline,
kills timed-out children, cleans up the root, and panics with the phase name;
use it for the filesystem writer phases in
filesystem_writer_epoch_chain_crosses_independent_processes and all request,
claim, settlement, and replay phases, replacing every blocking status() call.
---
Outside diff comments:
In `@crates/warp-core/src/causal_wal.rs`:
- Around line 6062-6064: Update truncate_tail_after to require the writer lease
before calling rewrite_filesystem_segments_after_truncation, consistent with
append_frame, flush_commit_with_capabilities, rotate_segment, publish_manifest,
and close_epoch. If recovery needs pre-epoch truncation, route it through an
explicit lease-acquiring recovery entrypoint instead of leaving this mutating
method unguarded.
- Around line 5872-5879: The per-commit update in flush_commit_with_capabilities
should stop calling persist_writer_epoch_ledger, avoiding redundant synchronous
ledger rewrites; retain the in-memory epoch_closures update and rely on
epoch-close persistence plus reconcile_writer_epoch_closures during reopen. If
persistence is intentionally retained, make its error path restore the prior
epoch_closures entry symmetrically with close_epoch.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 45ccccd8-0886-4adf-b038-077c66643bad
📒 Files selected for processing (7)
CHANGELOG.mdcrates/warp-core/src/causal_wal.rscrates/warp-core/src/trusted_runtime_host.rscrates/warp-core/tests/causal_wal_hardening_tests.rscrates/warp-core/tests/external_action_protocol_tests.rscrates/warp-core/tests/trusted_runtime_host_loop_tests.rsdocs/topics/WAL.md
1 participant
Persists single-owner filesystem WAL writer epochs and exact predecessor evidence across host restarts. Proves fenced external-action recovery and replay across independent processes.
Closes #714.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation