docs(rfc-0008): defer the RFC0008.5 corruption audit event to system-scoped audit - #189
Conversation
…scoped audit The WalRecoveryCorruption audit event needs a system-scoped audit partition (the RFC 0005 audit stream is tenant-partitioned, §3.7, and WAL corruption has no tenant). Deferred to a §9 follow-up gated on an RFC 0005 amendment. No safety lost: replay still returns the structured RecoveryError and halts recovery, so the receiver refuses to start and an operator must intervene; wal_corrupt_frames_total carries the dashboard signal. RFC0008.5 is satisfied by the structured-error + halt-all-segments halves. §5 scenario, §8 test plan, and the RFC0008.3 happy-path note updated to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 49 minutes and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 (1)
📝 WalkthroughWalkthroughRFC 0008's acceptance scenarios and test strategy were updated to defer the durable ChangesWAL Recovery Audit Event Deferral
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
There was a problem hiding this comment.
Pull request overview
Amends RFC 0008 (WAL) documentation to defer RFC0008.5’s corruption audit-event requirement until a system-scoped audit partition exists, while keeping the corruption detection + recovery-halting behavior as the enforceable contract.
Changes:
- Updates RFC0008.5 scenario text to replace the audit-event requirement with a dated deferral amendment and rationale.
- Adjusts the §8 testing strategy to drop the per-arm audit-event assertion and emphasize structured
RecoveryError+ halt-all-segments. - Adds a §9 open question capturing the needed system-scoped audit design (to later host WAL corruption and other infra events).
💡 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/rfcs/0008-wal.md (1)
1248-1250:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest-strategy arm 1 still mentions the deferred audit-event assertion.
Line 1249–1250 describes the CRC-mismatch arm as asserting the audit event, but the summary (L1271–1277) defers that assertion per the 2026-06-13 amendment. Arms 2–5 correctly omit mention of the audit event; arm 1 should match.
Fix: remove audit-event mention from arm 1 description
1. **Payload bit-flip (CRC mismatch)** on a closed segment — flip a random bit in a random frame, assert - `replay` emits the structured corruption error + audit - event and stops scanning *all* segments. + `replay` emits the structured corruption error and + stops scanning *all* segments.🤖 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 `@docs/rfcs/0008-wal.md` around lines 1248 - 1250, The test-strategy arm 1 text incorrectly asserts that `replay` emits both the structured corruption error and an audit event; update the arm 1 description so it only asserts that `replay` emits the structured corruption error and stops scanning all segments (remove the audit-event mention) to match the amended summary and arms 2–5; modify the sentence that references "segment — flip a random bit in a random frame, assert `replay` emits the structured corruption error + audit event and stops scanning *all* segments." to omit the "+ audit event" part.
🤖 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.
Outside diff comments:
In `@docs/rfcs/0008-wal.md`:
- Around line 1248-1250: The test-strategy arm 1 text incorrectly asserts that
`replay` emits both the structured corruption error and an audit event; update
the arm 1 description so it only asserts that `replay` emits the structured
corruption error and stops scanning all segments (remove the audit-event
mention) to match the amended summary and arms 2–5; modify the sentence that
references "segment — flip a random bit in a random frame, assert `replay` emits
the structured corruption error + audit event and stops scanning *all*
segments." to omit the "+ audit event" part.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 676c1c10-2b06-40b1-bab4-5c17e4fe6ee1
📒 Files selected for processing (1)
docs/rfcs/0008-wal.md
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(wal): flip the RFC0008.1/.3/.4/.5/.9 acceptance arms All five assert the already-implemented WAL behavior; no src/ change. - .1 wal-before-ack: the receiver's ack gate (ack iff append AND the covering sync both returned Ok) modelled on a real Wal — happy path flips the flag strictly after sync returns Ok; append-fault arm uses a real AppendError::TooLarge; sync-fault arm drives the gate with a synthesized SyncError (a real fsync fault needs OS injection, out of reach under deny(unsafe_code) — documented; the gate is the unit). - .3 recovery non-amplification: replay over a multi-segment fixture leaves wal_syncs_total unchanged and emits no audit event (the corruption-path event is deferred per #189, so the happy path is permanently zero). - .4 torn writes: partial header / partial payload on the newest segment heal back to the last frame boundary (next append lands there); a torn tail on a closed segment is TornOnClosedSegment. - .5 corruption: all five reasons (CrcMismatch, UnknownKind, NonZeroPad, OversizeLen, TornOnClosedSegment) — each names the segment UUID + byte, halts scanning all segments, and bumps wal_corrupt_frames_total; no audit-event assertion (deferred, #189). - .9 unflushed-bytes bound: a proptest over interleaved small + 16 MiB frames at the minimum segment size keeps wal_unflushed_bytes ≤ 2× the segment size; an undersized segment config is rejected at open. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(bench): recovery wall-time over N in {1,4,16} segments (RFC0008.3 evidence) Supportive measurement for the RFC0008.3 O(N) wall-time claim, mirroring b1/b2's evidence role (criterion measures; the behavioral gate is the rfc0008_3 smoke test). Self-contained: segments minted into a tempdir, group recovery/{1,4,16}. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(wal): trim the .9 proptest cost and the .1 append-fault arm's stray sync Review round 1 (all test-quality): - .9: 24→12 cases, 40→20 steps, and one reusable MAX_FRAME_BYTES buffer sliced per append instead of a fresh ~16 MiB vec per step (cuts the arm's runtime ~4×). - .1 append-fault arm: pair the failed append with a synthesized Ok(WalOffset) instead of a real wal.sync() — the gate's append half is what's under test, no filesystem touch needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(wal): cache .9's per-boundary metrics() call; correct the .5 read-order comment Review round 2 (test-quality): - .9: one metrics() per boundary (it directory-scans), reused in the assert and message instead of two calls per step. - .5: the module comment claimed the kind/pad/len arms need a recomputed CRC to isolate their check; in fact read_frame validates len/kind/pad before the CRC is ever consulted, so the CRC is irrelevant to those three — comment corrected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(wal): correct the Wal::open reopens-greatest comments (not mints-new) Three comments (rfc0008_3, rfc0008_4, the recovery bench) said a fresh Wal::open mints a new newest segment; it reopens the lexicographically-greatest existing one as the append target, so the EARLIER segments are the closed ones (the tests' older-vs-newest reasoning already relied on this — only the comments were wrong). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Spec amendment for the remaining RFC 0008 red arms: defer RFC0008.5's
WalRecoveryCorruptionaudit-event criterion to a system-scoped-audit follow-up.The corruption detection is already implemented (
replayreturns a structuredRecoveryErrornaming the segment + byte + reason and halts scanning all segments;wal_corrupt_frames_totalcounts). The unbuilt half is the durable audit event — and the RFC 0005 audit stream is tenant-partitioned (CLAUDE.md§3.7) while WAL corruption is a system event with no tenant, so the forensic record needs a system-scoped audit partition that doesn't exist yet.Maintainer decision (2026-06-13)
Defer the audit half via amendment (over a reserved-system-tenant or structured-log alternative). Corruption stays loud — recovery halts and the receiver refuses to start — so only the queryable record is postponed, not the failure response.
Changes (docs only)
This unblocks the RFC0008.5 test arms (and .1/.3/.4/.9) to flip against the amended contract, and the .8 batched-fsync feature, in the follow-up code PRs.
Checks
mdbook buildclean.🤖 Generated with Claude Code
Summary by CodeRabbit