docs(rfc-0008,rfc-0001): specify snapshot restore v2 — offset sink, retain floor, recovery driver - #185
Merged
Conversation
…etain floor, driver RFC 0008 §6.1/§6.6/§6.7 amendment: FrameSink::consume carries the frame's WalOffset (per-consumer replay horizons — Parquet consumes frames above the checkpoint X, the miner above its snapshot's high-water mark S); housekeeping becomes an explicit API taking an optional retain floor so truncation reclaims only segments wholly below min(X, S) — the CHECKPOINT sidecar always records the true Parquet horizon, since capping it would re-feed published records. New RFC0008.7 retain-floor arm + scenario RFC0008.10 (startup recovery driver: replay before listeners, routing, snapshot at rotation). The §9 miner-snapshot open question is RESOLVED. RFC 0001 §6.9 v2 amendment: the known-version branch now restores the tree and replays only the WAL tail above S (the v1 full-replay carve-out existed solely because Wal::checkpoint was a stub); the v1 double-apply hazard is resolved by offset routing rather than by refusing to restore. Stale-snapshot fallback: a WAL externally truncated past S restores + replays survivors + emits a structured warning naming the gap (hazard #5 surfaced via the RFC 0010 drift query, never silent). New criteria §3.5.3 (restore-equivalence, field-by-field via the snapshot payload of both trees) and §3.5.4 (loud degradation); §9 resume entry RESOLVED. No snapshot format change — the high-water mark has been in the payload since v1. Design decisions (maintainer, 2026-06-12): retain floor at housekeeping over snapshot-before-checkpoint coupling; full production driver in scope (serve() replays before listening). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 33 minutes and 56 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)
📝 WalkthroughWalkthroughTwo RFCs are amended to align snapshot recovery: RFC 0008 introduces WAL retain-floor truncation and per-consumer replay horizons via new ChangesSnapshot Recovery Semantics Alignment
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![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
🧹 Nitpick comments (2)
docs/rfcs/0008-wal.md (1)
835-839: 💤 Low valueMinor: Inconsistent terminology—use
Xinstead of "checkpoint" in the per-consumer horizon explanation.Line 835–839 explains per-consumer routing using "checkpoint" and "(checkpoint, S]", while the rest of §6.6 and §6.7 use variable names
X(Parquet checkpoint offset) andS(miner snapshot high-water mark). Aligning the terminology—e.g., "frames in(X, S]are already folded into the snapshot"—would improve clarity and reduce the cognitive load of mapping variable names.🤖 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 835 - 839, Replace the informal term "checkpoint" with the established variable name `X` in the per-consumer horizon explanation so it matches §6.6/§6.7; specifically, update the sentence that reads "frames in `(checkpoint, S]` are already folded into the snapshot" to use `(X, S]` and ensure surrounding text consistently uses `X` as the Parquet checkpoint offset alongside `S` (miner snapshot high-water mark).docs/rfcs/0001-template-miner.md (1)
2477-2488: 💤 Low valueEnumerate the snapshot payload fields compared in the restore-equivalence test.
The test description says the recovered state is "compared field-by-field via the §6.9 snapshot payload" but does not enumerate which fields. The snapshot format (lines 2272–2276) lists: tree leaves,
template_id,template_version,(severity_number, scope_name)key,slot_types, structured-template-id map, and high-water mark. Adding a brief parenthetical here — e.g., "(leaves, template versions, slot types, structured-template map)" — would make the test intent clearer without requiring the reader to cross-reference §6.9.🤖 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/0001-template-miner.md` around lines 2477 - 2488, Update the "Restore-equivalence test" description to explicitly list the §6.9 snapshot payload fields that are compared; add a brief parenthetical after "compared field-by-field via the §6.9 snapshot payload" naming the fields (e.g., leaves, template_id/template_version, (severity_number, scope_name) keys, slot_types, structured-template-id map, and high‑water mark) so the test intent is clear without cross-referencing §6.9.
🤖 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 `@docs/rfcs/0001-template-miner.md`:
- Around line 485-497: The paragraph claiming "the data side is complete
(everything truncated was ≤ `X`, hence in Parquet)" in the stale-snapshot
fallback needs an explicit ordering clarification between the miner snapshot
high-water mark `S` and the Parquet checkpoint `X`: either (a) state the design
invariant "S ≤ X" is enforced by the downstream consumer layer (so the claim
holds), or (b) change the sentence to a conditional that only asserts
completeness when "X ≤ S" and explain the fallback behavior/risk if "X > S"
(i.e., frames in (S, X] may be missing). Update the amendment text around the
stale-snapshot fallback and the sentence referencing `S` and `X` accordingly.
---
Nitpick comments:
In `@docs/rfcs/0001-template-miner.md`:
- Around line 2477-2488: Update the "Restore-equivalence test" description to
explicitly list the §6.9 snapshot payload fields that are compared; add a brief
parenthetical after "compared field-by-field via the §6.9 snapshot payload"
naming the fields (e.g., leaves, template_id/template_version, (severity_number,
scope_name) keys, slot_types, structured-template-id map, and high‑water mark)
so the test intent is clear without cross-referencing §6.9.
In `@docs/rfcs/0008-wal.md`:
- Around line 835-839: Replace the informal term "checkpoint" with the
established variable name `X` in the per-consumer horizon explanation so it
matches §6.6/§6.7; specifically, update the sentence that reads "frames in
`(checkpoint, S]` are already folded into the snapshot" to use `(X, S]` and
ensure surrounding text consistently uses `X` as the Parquet checkpoint offset
alongside `S` (miner snapshot high-water mark).
🪄 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: a9b3908e-38a4-4cf2-a0a5-d848120d8a84
📒 Files selected for processing (2)
docs/rfcs/0001-template-miner.mddocs/rfcs/0008-wal.md
Contributor
There was a problem hiding this comment.
Pull request overview
This PR amends RFC 0008 (WAL) and RFC 0001 (template miner) to specify snapshot restore v2 with per-consumer replay horizons, a housekeeping(retain_floor) API to prevent WAL truncation from outrunning miner snapshots, and a production startup recovery driver contract.
Changes:
- RFC 0008: specify offset-carrying
FrameSink::consume, explicitWal::housekeeping(retain_floor), and a new startup recovery driver scenario (RFC0008.10). - RFC 0001: switch the known-version recovery branch to restore + tail replay, add stale-snapshot loud-degradation behavior, and add acceptance criteria (§3.5.3/§3.5.4).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| docs/rfcs/0008-wal.md | Updates WAL API/spec to support snapshot-restore v2 (offset sink, retain floor, driver scenario). |
| docs/rfcs/0001-template-miner.md | Updates miner persistence/recovery spec to enable restore+tail replay v2 and adds new acceptance criteria. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…er-consumer in the driver Review round 1 — Copilot caught a real inconsistency: the in-replay checkpoint skip made the retain floor useless (a lagging snapshot's (S, X] frames were retained on disk but never deliverable, so the miner could not close its state gap). Suppression moves out of Wal::replay into the recovery driver: replay delivers every well-formed surviving frame with its offset; the driver suppresses per consumer (Parquet > X via the new last_checkpoint accessor, miner > S), handling both orderings of S and X. RFC0008.7 arm 2 reworded accordingly (sidecar survives; the driver's Parquet-side suppression uses it); RFC0008.10 gains a lagging-snapshot catch-up arm. Also: the stale-fallback example is corrected to manual WAL segment deletion (deleting snapshots triggers full-replay fallback, not a truncated WAL), and §3.5.4 introduces X explicitly, scoping 'data side complete' to truncation that never exceeded the checkpoint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ording in §6.7 Copilot round 2: a present-but-invalid CHECKPOINT (bad magic, unknown version, non-zero flags, wrong size) is a structured corruption error that aborts recovery — silently treating it as None would drop the Parquet suppression horizon and duplicate every already-published record; removal by the operator is an explicit acceptance of at-least-once re-publish. The §6.7 durability paragraph now speaks in driver-suppression terms instead of the retired skip-inside-replay model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jun 12, 2026
…ver (#187) * feat(miner): snapshot restore v2 — recover() returns the state, restore_tenant rebuilds the tree RFC 0001 §6.9 v2 (amended 2026-06-12, PR #185): the known-version branch now restores instead of discarding. recover() returns (Option<SnapshotState>, RecoveryOutcome) — Restored replaces KnownVersionDiscarded; the v1 discard-contract test is retired per the RFC-gated amendment and replaced by the restore contract. MinerCluster::restore_tenant rebuilds a tenant tree from a SnapshotState: leaves re-descend by their creation-time masked path (a path-position wildcard resolves to its singleton mask tag — widening/type-expansion are impossible at path positions because candidates share their first walk_depth masked tokens by construction); structured-template map and template_count rebuilt; the cluster-wide template_id allocator bumps past every restored id. Semantically inconsistent snapshots (empty template, slot-count mismatch, non-mask path slot) are RestoreError::Inconsistent — the driver treats them as corrupt (discard, full replay) per §6.9. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(ingester): startup recovery driver + per-tenant snapshot store (RFC 0008 §6.6 / RFC0008.10) recovery::recover restores each tenant's snapshot into the miner (uuid-parse failures and restore_tenant rejections discard the artefact as corrupt — §6.9), then replays the WAL through a sink that decodes, fans out, and feeds the miner only frames above that tenant's high-water mark; the Parquet horizon (last_checkpoint) is read and reported — its consumer joins when the Parquet write path does. Stale-gap detection per §3.5.4: S below the checkpoint with S's segment absent from the replayed set (internally unreachable under the §6.7 retain floor; a hit means external mutation and is surfaced, never silent — hazard #5). A frame that fails decode or fan-out stops replay loudly: it was valid when acked, so this is corruption-adjacent. snapshot_store: <wal_root>/snapshots/<tenant>.snap artefacts, atomic tmp -> fsync -> rename -> parent fsync; load_all returns raw bytes (version dispatch stays in recover). Journal::sync now returns the durable offset so the pipeline can track last_durable for the shutdown-cadence snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(server): recovery before listeners; snapshot cadence at post-recovery + shutdown serve() now runs the RFC0008.10 sequence: open WAL, restore + replay through the recovery driver, warn per stale-gap tenant (stderr — the documented stopgap until structured logging lands), write fresh snapshots at the replayed high-water, and only then construct the pipeline and bind both listeners. ReceiverHandle::shutdown writes snapshots again after both listeners stop; a write failure degrades to a warning — the snapshot is a rebuildable cache (§6.9), never durable state. Per-rotation cadence stays blocked on RFC0008.6. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: land RFC0001 §3.5.3/§3.5.4 and the RFC0008.10 driver scenario Restore-equivalence (restored + tail-replayed miner equals a from-scratch control, with the suppression counter proving no frame at or below S reached it), corrupt-version full-replay fallback, the stale-gap arm (external segment deletion past S with a checkpoint above it -> loud report, survivors still fold), cold start, and the served-binary end-to-end (pre-populated WAL + snapshot -> serve -> live export -> shutdown artefacts equal a control). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ingester,miner,server): restore requires a concrete horizon; seed last_durable from recovery Review round 1: - a known-version snapshot whose wal_high_water is absent or unparseable is discarded (full replay) instead of restored — restoring without a horizon cannot suppress, which is exactly the v1 double-apply hazard (§6.9 maps it to the discard class) - IngestPipeline::with_last_durable seeds the recovered high-water, so a zero-traffic shutdown no longer overwrites the post-recovery snapshots with a horizonless artefact that would force full replay - restore_tenant rejects duplicate template_ids (across leaves + structured) and duplicate structured (severity, scope) keys as Inconsistent - load_all skips non-file *.snap entries instead of aborting recovery - colocated unit tests for recovery.rs (parse_high_water, the extracted stale_gap classification helper, sink rejection) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(server): shutdown doc reflects the handle retaining the pipeline Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- 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
Spec-first amendment for the WAL checkpoint → snapshot restore workstream (the §6.9 v2 restore that RFC 0001 explicitly deferred until
Wal::checkpointexisted). Two RFCs amended together because they are two halves of one design:RFC 0008 (§6.1, §6.6, §6.7, §5, §8, §9):
FrameSink::consumenow carries the frame'sWalOffset— the recovery driver routes one delivered frame to consumers with different replay horizons: the Parquet path consumes every frame above the checkpointX; the miner consumes only frames above its snapshot's high-water markS.Wal::housekeeping(retain_floor: Option<WalOffset>)becomes an explicit API; truncation reclaims only segments wholly below min(X, S). TheCHECKPOINTsidecar always records the true Parquet horizon (capping it at the floor would re-feed already-published records → data-side duplicates).replaysignature is&mut self(the landed §6.6 step-4 heal mutates the newest segment).RFC 0001 (§6.9, §5, §8, §9):
S— the v1 full-replay carve-out existed solely because the offset-resume API was a stub; the double-apply hazard is resolved by routing, not by refusing to restore. No format change (the high-water mark has been in the payload since v1, as planned).Srestores + replays survivors + emits a structured warning naming the gap — hazard docs(rfc-0001): fill in drafted-bar content for the template miner #5 surfaced (observable via the RFC 0010 drift query), never silent.Sreached the miner) and §3.5.4 (loud degradation); §8 test-plan entry added; §9 resume entry RESOLVED.Why this shape
The checkpoint
X(Parquet durability) and the snapshot markS(miner-state coverage) are independent horizons. Truncating up toXwhileS < Xdestroys the only remaining source of miner state in(S, X]— the data is safe in Parquet, but the restored tree re-mintstemplate_ids for templates first seen in the gap (hazard #5 template drift). The retain floor closes that hole at the cost of at most the segments appended since the last successful snapshot; in steady state (snapshot per rotation) the floor leads the checkpoint and the rule is vacuous.Maintainer decisions encoded (2026-06-12)
serve()currently opens the WAL but never replays; the driver (replay before listeners, routing, snapshot-at-rotation) is part of this workstream (RFC0008.10).Invariants / hazards touched
CHECKPOINTsidecar still records exactly the Parquet horizon.Implementation plan (follow-up PRs)
ourios-wal: checkpoint sidecar + replay skip + housekeeping(retain_floor) + offset-carrying sink → flips RFC0008.7 (4 arms).serve()→ §3.5.3/§3.5.4 + RFC0008.10.Checks run
mdbook buildclean (no new diagrams).Summary by CodeRabbit