feat(wal): checkpoint sidecar, retain-floor housekeeping, offset-carrying sink (RFC0008.7) - #186
Merged
Merged
Conversation
…ying sink (RFC 0008 §6.7) Implements the §6.7 contract as amended 2026-06-12: - Wal::checkpoint persists the 32 B OWCK v1 sidecar atomically (CHECKPOINT.tmp -> fsync -> rename -> parent-dir fsync); advance is monotonic, re-asserting the current value is an idempotent no-op, and the in-memory offset is not advanced on error. - Wal::open reads the sidecar; a present-but-invalid one (wrong magic / version / flags / size) is OpenError::Corrupt and aborts before any recovery — silently treating it as None would drop the Parquet suppression horizon and duplicate published records. - Wal::last_checkpoint exposes the offset as the recovery driver's Parquet-side suppression horizon. - Wal::housekeeping(retain_floor) unlinks whole segments wholly below min(checkpoint, floor) — never the current append segment; segment identity comes from the in-file header, not the filename. The floor is the lagging-miner-snapshot guard (hazard #5). - FrameSink::consume now receives the frame's append-offset and replay delivers every well-formed surviving frame — suppression moved out of replay into the driver, per consumer (an in-replay skip would make floor-retained frames undeliverable). - metrics() implemented: exact counters (appends, syncs, unflushed bytes, corrupt frames) + best-effort disk_bytes / segment_count directory walk + checkpoint fields. - wal_crash_fixture gains a CHECKPOINT op (checkpoint at the last append's offset, echoed to stdout) for the RFC0008.7 SIGKILL arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Normal-flow truncation (wholly-below unlinked, straddler kept, wal_disk_bytes drops by the unlinked bytes); SIGKILL between checkpoint(X) and housekeeping (sidecar survives, replay delivers everything, partitioning on X yields exactly the published prefix); surviving-segments offsets (fresh open + checkpoint(Y > X) with no global counter); retain floor (S < X holds the (S, X] segment back until the floor advances). Multi-segment roots are minted in scratch roots and moved in — rotation (RFC0008.6) is still red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 50 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements WAL checkpoint persistence and offset-driven housekeeping (RFC 0008): adds a 32-byte sidecar codec to durably store checkpoint offsets, updates the ChangesCheckpoint sidecar, offset tracking, and segment truncation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements RFC 0008 §6.7 “checkpoint sidecar + truncation bound + offset-carrying replay” in ourios-wal, enabling checkpoint persistence, retain-floor-aware housekeeping, and per-consumer replay suppression via delivered WalOffsets.
Changes:
- Add durable
CHECKPOINTsidecar codec + atomic persistence and wire it intoWal::open/Wal::checkpoint/Wal::last_checkpoint. - Implement
Wal::housekeeping(retain_floor)truncation logic andWal::metrics()counters + best-effort disk stats. - Update
FrameSinkto receive(WalOffset, FrameKind, payload)and adapt integration tests + crash fixture accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-wal/src/lib.rs | Implements checkpointing, housekeeping, metrics, and offset-carrying replay; updates FrameSink trait. |
| crates/ourios-wal/src/checkpoint.rs | New sidecar codec + atomic read/write implementation with validation. |
| crates/ourios-wal/tests/rfc0008_7_checkpoint.rs | Flips RFC0008.7 arms live (normal flow, crash window, offset semantics, retain floor). |
| crates/ourios-wal/tests/fixtures/wal_crash_fixture.rs | Adds CHECKPOINT operation for SIGKILL crash arm. |
| crates/ourios-wal/tests/rfc0008_2_crash_recovery.rs | Updates sink signature to accept WalOffset. |
| crates/ourios-wal/tests/recovery.rs | Updates sink signature to accept WalOffset. |
| crates/ourios-server/tests/rfc0003_16_served_binary.rs | Updates sink signature to accept WalOffset. |
| crates/ourios-ingester/tests/ingest_support/mod.rs | Updates sink signature to accept WalOffset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ourios-wal/src/lib.rs (1)
849-863:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftClassify invalid replay-time segment headers as corruption, not I/O.
This branch documents bad magic/version on a
*.walfile as corruption, but it wraps those errors inRecoveryError::Io. That changes the public replay contract and also suppressescorrupt_frames_total, becauseWal::replayonly increments that counter onRecoveryError::Corrupt.🤖 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/ourios-wal/src/lib.rs` around lines 849 - 863, The branch that handles invalid segment headers currently returns RecoveryError::Io by wrapping the HeaderError in an std::io::Error; change it to return the corruption variant instead so replay-time header failures are classified as corruption and corrupt_frames_total still increments. Specifically, replace the Err(other) => return Err(RecoveryError::Io { ... source: std::io::Error::new(..., other) }) with returning RecoveryError::Corrupt (use the same op string "validate_header(segment for replay)" and pass the original HeaderError/typed `other` as the structured source) so the HeaderError is preserved in the error chain and the error is reported as corruption rather than I/O.
🤖 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/ourios-wal/src/lib.rs`:
- Around line 490-512: The loop currently skips the active segment only by path
(path == self.current_segment_path), which fails if the active segment was
renamed; instead, after reading each file's header (header.segment_uuid), skip
removal when that UUID equals the active segment's UUID by comparing
header.segment_uuid to the in-memory active segment identity (e.g.
self.current_segment.header.segment_uuid or a stored self.current_segment_uuid).
Update the guard to check UUID equality (using header.segment_uuid) before
removing the file so the live append segment is never unlinked even if renamed.
---
Outside diff comments:
In `@crates/ourios-wal/src/lib.rs`:
- Around line 849-863: The branch that handles invalid segment headers currently
returns RecoveryError::Io by wrapping the HeaderError in an std::io::Error;
change it to return the corruption variant instead so replay-time header
failures are classified as corruption and corrupt_frames_total still increments.
Specifically, replace the Err(other) => return Err(RecoveryError::Io { ...
source: std::io::Error::new(..., other) }) with returning RecoveryError::Corrupt
(use the same op string "validate_header(segment for replay)" and pass the
original HeaderError/typed `other` as the structured source) so the HeaderError
is preserved in the error chain and the error is reported as corruption rather
than I/O.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb1336a0-59ff-4097-98e7-b9981d4b5170
📒 Files selected for processing (8)
crates/ourios-ingester/tests/ingest_support/mod.rscrates/ourios-server/tests/rfc0003_16_served_binary.rscrates/ourios-wal/src/checkpoint.rscrates/ourios-wal/src/lib.rscrates/ourios-wal/tests/fixtures/wal_crash_fixture.rscrates/ourios-wal/tests/recovery.rscrates/ourios-wal/tests/rfc0008_2_crash_recovery.rscrates/ourios-wal/tests/rfc0008_7_checkpoint.rs
Both reviewers caught it: a renamed live append segment slipped the path-based guard, and unlinking it leaves the writer appending into an unlinked inode no later open would see. The identity check now reads the candidate's header first and skips on uuid equality, consistent with the pass's rename-resilient identity rule. Pinned by a test that renames the live segment, checkpoints past its every frame, and asserts housekeeping leaves it alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PR 2 of the snapshot-restore workstream (spec: #185). Implements RFC 0008 §6.7 in
ourios-waland flips all four RFC0008.7 red-gate arms:Wal::checkpoint(durable_to)— persists the 32 BOWCKv1 sidecar atomically (CHECKPOINT.tmp→fsync→rename→ parent-dirfsync). Monotonic advance; idempotent on re-assert; the in-memory offset is not advanced on error (conservatively keeps all segments).open(§6.6 step 1): wrong magic / unknown version / non-zero flags / size ≠ 32 B →OpenError::Corrupt, before any recovery. Silently treating it asNonewould drop the Parquet suppression horizon and duplicate every published record.Wal::last_checkpoint()— the recovery driver's Parquet-side suppression horizon.Wal::housekeeping(retain_floor)— unlinks whole segments wholly below min(checkpoint, floor); never the current append segment; segment identity from the in-file header (a renamed file is judged by its true identity). The floor is the lagging-snapshot guard from the docs(rfc-0008,rfc-0001): specify snapshot restore v2 — offset sink, retain floor, recovery driver #185 amendment.FrameSink::consumecarries the frame's append-offset, andreplaydelivers every well-formed surviving frame — suppression moved out ofreplayinto the driver, per consumer. An in-replayskip would make floor-retained(S, X]frames undeliverable (the inconsistency Copilot caught on docs(rfc-0008,rfc-0001): specify snapshot restore v2 — offset sink, retain floor, recovery driver #185).metrics()implemented: exact counters (appends_total,syncs_total,unflushed_bytes,corrupt_frames_total), best-effortdisk_bytes/segment_countdirectory walk (dashboard read, documented), checkpoint fields.wal_crash_fixturegains aCHECKPOINTop for the SIGKILL arm.Tests
All four RFC0008.7 arms are live (
tests/rfc0008_7_checkpoint.rs), plus sidecar codec unit tests (round-trip, pinned 32 B layout, per-field rejection, absent-vs-invalid read). Multi-segment roots are minted via the public API in scratch roots and moved in (UUIDv7 monotonicity ⇒ chronological order) — rotation (RFC0008.6) is still red and untouched.Arm 2 is a real SIGKILL: the fixture checkpoints, echoes the offset, signals READY, and is killed before housekeeping; the restart asserts
last_checkpoint() == Some(X)and that partitioning the delivered frames onXyields exactly the already-published prefix.Invariants / hazards (§3.4 / H3, hazard #5)
append/syncsemantics unchanged; the new counters are passive. Replay still delivers every surviving frame, so no acknowledged data is dropped; the checkpoint affects only what the driver's Parquet path suppresses (already-durable-on-object-storage records) and what housekeeping may reclaim.Wal::replaydocs updated to the delivers-everything contract.Checks run
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --all-features(80 test binaries, zero failures),cargo doc,cargo bench --no-run— all green locally.Part of the #185 plan; PR 3 (miner restore v2 +
serve()recovery driver, RFC0008.10 / §3.5.3 / §3.5.4) follows.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests