feat(miner): template-tree snapshot format + v1 full-replay recovery (RFC0001 §3.5) - #170
Conversation
…(RFC0001 §3.5) Implements the RFC 0001 §6.9 per-tenant snapshot format and v1 recovery, closing the last miner-owned red gate (§3.5.1 / §3.5.2). The snapshot artefact is `[SNAPSHOT_VERSION][payload]`: byte 0 is the format version, the payload is a serde_json-encoded `SnapshotState` capturing the per-tenant leaves (template tokens, template_id, template_version, the (severity_number, scope_name) key, slot_types), the structured-template-id map, and the WAL high-water mark. The codec is a v1 detail behind the version byte; the reader dispatches on byte 0. `load_snapshot` reads byte 0 and either deserialises the payload (known version) or returns a typed `SnapshotError` (UnknownVersion / Corrupt / Empty) — enum-carried states, no panics on the recovery path. `recover` is v1: it ALWAYS rebuilds the tree from a full `Wal::replay()` in BOTH branches and never restores from the snapshot. The known-version restore-then-replay-the-tail path needs RFC 0008 §6.7's offset-resume API (`Wal::checkpoint`, still a red-gate stub); restoring + full replay would double-apply every captured frame and corrupt the tree. So v1 lands the format + version-dispatch + WAL-fallback only; the restore path switches on with no format change once §6.7 lands. The miner does not depend on ourios-wal — the OtlpBatch-decode + ingest pipeline the replay drives lives in ourios-ingester — so the full replay is passed in as a closure, keeping the crate boundary clean. §3.5 invariant addressed: snapshot format evolution is gated on the leading version byte (additive changes tolerated, breaking changes bump the byte and discard old snapshots); v1 recovery rebuilds from the WAL, so a stale/unknown/corrupt snapshot is never a data-loss event. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 52 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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)
📝 WalkthroughWalkthroughThis PR adds a versioned snapshot wire format and serde-backed SnapshotState, implements snapshot serialization/deserialization and recover() (v1 always rebuilds), exposes a snapshot module, adds MinerCluster::snapshot_state that emits SnapshotState, and enables tests validating framing and WAL-replay recovery. ChangesSnapshot and Recovery Subsystem
Sequence Diagram(s)sequenceDiagram
participant MinerCluster
participant snapshot_mod as snapshot::*
participant serde_json
MinerCluster->>snapshot_mod: snapshot_state(tenant_id)
snapshot_mod->>snapshot_mod: build SnapshotState
snapshot_mod->>serde_json: serialize SnapshotState
serde_json-->>snapshot_mod: JSON bytes
snapshot_mod-->>MinerCluster: [SNAPSHOT_VERSION][payload_json]
MinerCluster->>snapshot_mod: load_snapshot(bytes)
snapshot_mod->>snapshot_mod: inspect version byte
alt version == SNAPSHOT_VERSION
snapshot_mod->>serde_json: deserialize payload
serde_json-->>snapshot_mod: SnapshotState
else
snapshot_mod-->>MinerCluster: SnapshotError::UnknownVersion / Corrupt
end
MinerCluster->>snapshot_mod: recover(snapshot_bytes, rebuild_fn)
snapshot_mod->>snapshot_mod: call rebuild_fn() and return (rebuilt, RecoveryOutcome)
🎯 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 |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Implements RFC 0001 §6.9 snapshot format framing (leading version byte + JSON payload) and introduces v1 recovery semantics that always fall back to a full WAL replay while still exercising version dispatch. It also unblocks the remaining RFC 0001 §3.5 red-gate tests by replacing the ignored stubs with assertions over the new snapshot API.
Changes:
- Add
ourios_miner::snapshotmodule implementing[version byte][payload]snapshot encoding, typed snapshot load errors, and v1 “always rebuild” recovery outcome reporting. - Add
MinerCluster::snapshot_state()to capture a tenant’s serializable snapshot state. - Turn RFC 0001 §3.5.1/§3.5.2 invariant tests from ignored stubs into passing tests that validate versioning behavior and WAL-replay recovery fallback.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-miner/tests/invariants.rs | Replaces §3.5 ignored stubs with assertions for snapshot version byte, loader rejection on unknown version, and v1 recovery fallback behavior. |
| crates/ourios-miner/src/snapshot.rs | New snapshot format + loader + v1 recovery API, plus helper codecs and unit tests. |
| crates/ourios-miner/src/lib.rs | Exposes the new snapshot module publicly. |
| crates/ourios-miner/src/cluster.rs | Adds MinerCluster::snapshot_state() to materialize per-tenant SnapshotState from in-memory miner state. |
| crates/ourios-miner/Cargo.toml | Adds serde/serde_json dependencies for snapshot payload encoding/decoding. |
| Cargo.lock | Updates lockfile to include new serde dependencies for ourios-miner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…c enums Sort snapshot leaves + structured templates by the cluster-unique template_id (HashMap iteration order varies across runs, causing spurious snapshot churn); add a sorted-order test. Mark SnapshotError and RecoveryOutcome non_exhaustive (forward-compatible public enums). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…store inverse slot_types_vec_to_record is pub(crate) (only cluster.rs uses it). The restore-side inverse (slot_types[_vec]_from_record) was unused in v1 — recover never reconstructs a tree, it full-replays — so remove it (it returns with the RFC 0008 §6.7 restore path) and pin the forward encoding with a direct assertion instead of a round-trip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ring snapshot() now returns Result<Vec<u8>, SnapshotError> (Serialize variant) rather than silently emitting a truncated [version]-only artefact every reader would reject as corrupt. Extend the determinism test with structured ingests + assert structured_templates is sorted too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-miner/src/snapshot.rs`:
- Around line 499-510: The test slot_types_to_record_captures_every_member
currently only asserts membership which won't catch order changes; update it to
assert the exact encoded vector order returned by slot_types_to_record (called
with SlotTypes::singleton(ParamType::Num).insert(ParamType::Str)) instead of
using contains/len checks—compare the result to the exact Vec<[ParamTypeRecord]>
sequence (e.g. [ParamTypeRecord::Num, ParamTypeRecord::Str]) so the snapshot
determinism is verified.
- Around line 245-258: The v1 JSON snapshot decoder (load_snapshot) currently
deserializes into SnapshotState and its mirror structs without denying unknown
fields, so same-version schema drift can be silently ignored; update the
Deserialize mirror types used by load_snapshot (including SnapshotState and any
nested mirror structs referenced during v1 deserialization) to include
#[serde(deny_unknown_fields)] so serde_json::from_slice will fail on unknown
fields and surface incompatible v1 payloads immediately. Ensure you add the
attribute to every mirror struct involved in v1 decoding, rebuild, and run tests
that exercise load_snapshot.
🪄 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: d0bb78f1-715b-4937-b6a4-e08b3f09bb62
📒 Files selected for processing (3)
crates/ourios-miner/src/cluster.rscrates/ourios-miner/src/snapshot.rscrates/ourios-miner/tests/invariants.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/ourios-miner/tests/invariants.rs
- crates/ourios-miner/src/cluster.rs
The payload is the state a future restore would rebuild from; v1 recovery full-replays the WAL and never restores. Mark the rebuild as the RFC 0008 §6.7 restore-path behavior, not current. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assert the exact encoded vector (the snapshot byte-determinism rests on SlotTypes::iter canonical order), not just membership. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
10c6765 to
e810d8e
Compare
Inline code spans cannot contain a newline in rustdoc markdown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implements RFC 0001 §6.9 — the per-tenant miner template-tree snapshot format + v1 recovery — closing the last RFC 0001 miner-owned red gate (§3.5.1 / §3.5.2).
What lands
crates/ourios-miner/src/snapshot.rs(new).pub const SNAPSHOT_VERSION: u8 = 1.snapshot(&SnapshotState) -> Vec<u8>emits[SNAPSHOT_VERSION][payload]. The payload is aserde_json-encodedSnapshotStatecapturing the per-tenant state the §6.9 contract names: the tree leaves (template tokens,template_id,template_version, the(severity_number, scope_name)template key, per-slotslot_types), the structured-template-id map, and the WAL high-water mark. The codec is a v1 implementation detail behind the version byte; the reader dispatches on byte 0.load_snapshot(&[u8]) -> Result<SnapshotState, SnapshotError>. Reads byte 0:== SNAPSHOT_VERSIONdeserialises the payload; otherwiseErr(SnapshotError::UnknownVersion(byte0)). Empty / too-short / undeserialisable inputs are typed variants (Empty/Corrupt) — enum-carried states, no.unwrap()/.expect()on the recovery path.recover(snapshot_bytes, rebuild) -> (tree, RecoveryOutcome). v1 behaviour: always rebuilds from a full WAL replay in both branches; never restores from the snapshot. It still callsload_snapshotto exercise/observe the version-dispatch (known →KnownVersionDiscarded, unknown/corrupt/empty →UnknownOrCorruptDiscarded), but the result is not used to skip the replay.MinerCluster::snapshot_state(&tenant)— the producer side that captures a live tenant'sSnapshotState.#[ignore]+todo!()removed): §3.5.1 assertsbytes[0] == SNAPSHOT_VERSION; §3.5.2 asserts unknown version →Err(UnknownVersion(0xFF)), then drivesrecoverwith a stale (unrelated-template) snapshot + a full-replay closure and asserts the recovered tree equals the WAL-only replay and is not the stale snapshot's content.Invariant §3.5 (schema/format evolution)
Snapshot format evolution is gated on the leading version byte: additive changes are read-compatible, breaking changes bump the byte and old snapshots are discarded and rebuilt from the WAL. Because the snapshot is a rebuildable recovery cache, not durable state (the WAL is the truth, §3.4), a stale / unknown / corrupt snapshot is never a data-loss event — it degrades to a full
Wal::replay().Why v1 does NOT restore (correctness constraint)
The known-version restore-then-replay-the-tail path of §6.9 step (2) needs RFC 0008 §6.7's offset-resume API (
Wal::checkpoint/CHECKPOINT), which is still a red-gate stub. Restoring a tree from a snapshot and then replaying the full WAL — the only replay available without offset support — would double-apply every captured frame and corrupt the tree. So v1 lands the snapshot format + version-dispatch + WAL-fallback only; the restore path switches on, with no format change, once §6.7 lands. This v1 fully satisfies §3.5.1 (leading version byte) and §3.5.2 (unknown version → full WAL replay).Notes
ourios-wal: theOtlpBatch-decode + tenant-fan-out + miner-ingest pipeline the full replay drives lives inourios-ingester. The full replay is passed intorecoveras a closure, keeping the crate boundary clean. Restore + offset-resume are deferred to RFC 0008 §6.7 and are not wired.RecoveryOutcomeenum (named in prose in §6.9), not exported as an OTel instrument — so no weaver-registry change in this slice.Verification
cargo fmt --all --check— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo test --all-features— 598 passed; 0 failed; 24 ignored (both §3.5 tests now run and pass; the invariants binary has 0 ignored)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests