feat(miner,ingester,server): snapshot restore v2 — per-tenant restore + startup recovery driver - #187
Merged
Merged
Conversation
…re_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>
…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>
…overy + 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>
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>
📝 WalkthroughWalkthroughThe PR implements RFC 0008/0001 WAL recovery and snapshot restore across ingester and miner: pipeline durable-offset tracking, per-tenant snapshot persistence, miner restore APIs, a recovery driver with suppression horizons and stale-gap detection, server startup/shutdown integration, and extensive tests. ChangesWAL Recovery and Snapshot Restore Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 snapshot-restore v2 end-to-end: the miner can now restore per-tenant state from snapshots, and the ingester/server run a startup recovery driver that restores snapshots and replays only the WAL tail above each tenant’s high-water mark before binding listeners (RFC0008.10 / RFC0001 §6.9 v2).
Changes:
- Add ingester recovery driver + snapshot store (
<wal_root>/snapshots/<tenant>.snap) with atomic write semantics, plus server wiring to run recovery before serving and to write snapshots post-recovery and at shutdown. - Upgrade miner snapshot recovery dispatch so known-version snapshots are returned for restore (instead of discarded), and add
MinerCluster::restore_tenantto rebuild per-tenant state from snapshot payloads. - Add/adjust RFC-gated tests covering restore+tail-replay equivalence, stale-gap detection, and served-binary end-to-end recovery behavior.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-server/tests/rfc0008_10_recovery_driver.rs | New end-to-end served-binary test asserting recovery-before-serving + coherent shutdown snapshots. |
| crates/ourios-server/src/receiver.rs | Run recovery before binding; write snapshots post-recovery and on shutdown; plumb snapshot root and pipeline handle. |
| crates/ourios-miner/tests/invariants.rs | Update §3.5.2 invariant test to new recover() signature and v2 semantics. |
| crates/ourios-miner/src/snapshot.rs | Switch recovery dispatch to return SnapshotState on known version; update outcomes/docs/tests accordingly. |
| crates/ourios-miner/src/mask.rs | Expose mask-tag strings for restore-time path wildcard reconstruction (tag_str_for). |
| crates/ourios-miner/src/cluster.rs | Add tenant_ids() and restore_tenant() to rebuild tenant tree + maps from snapshot state; add restore-focused unit tests. |
| crates/ourios-ingester/tests/rfc0001_3_5_snapshot_restore.rs | New driver-level tests for restore equivalence, corrupt-version fallback, stale-gap detection, and cold start. |
| crates/ourios-ingester/tests/ingest_support/mod.rs | Update Journal::sync test doubles to return an optional durable WAL offset. |
| crates/ourios-ingester/src/snapshot_store.rs | New on-disk per-tenant snapshot store with percent-encoded tenant filenames and atomic writes. |
| crates/ourios-ingester/src/recovery.rs | New startup recovery driver: per-tenant snapshot restore + WAL replay routing with per-tenant suppression horizons and stale-gap detection. |
| crates/ourios-ingester/src/receiver/pipeline.rs | Journal::sync returns durable offset; pipeline tracks last_durable for snapshot stamping. |
| crates/ourios-ingester/src/lib.rs | Export new recovery + snapshot_store modules and related error/report types. |
| crates/ourios-ingester/Cargo.toml | Add uuid dependency (for parsing snapshot-recorded WAL segment IDs). |
| Cargo.lock | Lockfile update for uuid. |
💡 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: 5
🤖 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-ingester/src/recovery.rs`:
- Around line 213-217: write_snapshots is overwriting every tenant's
state.wal_high_water with a single global high_water; instead preserve
per-tenant replay horizons by retaining the tenant's existing wal_high_water
when appropriate. In the block that sets state.wal_high_water after calling
miner.snapshot_state(&tenant_id), check the tenant's current
snapshot/wal_high_water and whether the tenant is marked stale_gap (or compare
to the original S stored for that tenant) and only update to the new high_water
when the tenant is not stale_gap or when the new offset is strictly ahead of the
tenant’s existing horizon; otherwise leave the tenant’s original horizon (or S)
unchanged so serve() and report.max_delivered do not get flattened to a single
global offset. Ensure this logic references write_snapshots,
miner.snapshot_state, state.wal_high_water, stale_gap and report.max_delivered
so reviewers can find and verify the change.
- Around line 113-305: Add a colocated #[cfg(test)] mod with focused unit tests
next to the recovery code: 1) tests for parse_high_water covering None, valid
UUID → Ok(Some(WalOffset)), and invalid UUID → Err(()) by calling
parse_high_water directly; 2) tests that exercise the stale-gap classification
branch in recover by creating a controlled test scenario (use an in-memory/temp
snapshots_root, a test Wal that returns a chosen parquet_horizon via
last_checkpoint(), and a MinerCluster test double with one restored tenant and a
matching horizon) and assert tenant.stale_gap true/false for the checkpoint vs
segment visibility cases; 3) tests for DriverSink::consume/reject by
constructing a DriverSink with a test MinerCluster and TenantRule and calling
consume with payloads that cause ExportLogsServiceRequest::decode or fan_out to
fail, asserting the returned RecoveryError (SinkRejected) and that
frames/records counters update as expected; keep tests small, use test-only
helpers/doubles in the same module so no public API changes are needed.
In `@crates/ourios-ingester/src/snapshot_store.rs`:
- Around line 115-127: In load_all(), the directory iteration treats any path
with EXTENSION (".snap") as a file and calls std::fs::read(&path), which fails
if the entry is a directory like "tenant.snap/"; add a guard that skips
non-files (e.g., path.is_file() or fs::metadata(&path)?.is_file()) before
attempting to read or process the file (place this check in the entries loop
before calling std::fs::read and before percent_decode_tenant), so only real
snapshot files are read and non-file artefacts are silently skipped.
In `@crates/ourios-miner/src/cluster.rs`:
- Around line 1896-1994: The restore loop must reject duplicate
structured-template keys and duplicate template_ids instead of silently
overwriting and double-counting: when iterating state.structured_templates,
check if tenant.structured_templates already contains the key (severity_number,
scope_name) and return Err(RestoreError::Inconsistent { detail: ... }) on
duplicates; also build a set of seen template_id values while processing
state.leaves and state.structured_templates and return
Err(RestoreError::Inconsistent { detail: ... }) if any template_id is
encountered twice (including collisions between leaves and
structured_templates); finally compute tenant.template_count from the actual
lengths of tenant.leaves and tenant.structured_templates (or the seen set) and
keep the next_template_id bump logic unchanged.
In `@crates/ourios-server/src/receiver.rs`:
- Around line 130-131: IngestPipeline::new currently initializes last_durable to
None, discarding the recovered durable mark; after constructing the pipeline
(the SharedPipeline Arc<Mutex<IngestPipeline>> stored in variable pipeline) you
must seed its last_durable with the recovered high-water value produced by
recovery so shutdown() doesn't overwrite it with None. Fix by setting the
recovered durable mark into the pipeline right after creation (either via an
existing setter on IngestPipeline or by adding a small method like
set_last_durable or a constructor parameter) so the pipeline's last_durable
reflects the recovered durable mark instead of None.
🪄 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: 9b811a5f-0322-4298-b824-401494782b6c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/ourios-ingester/Cargo.tomlcrates/ourios-ingester/src/lib.rscrates/ourios-ingester/src/receiver/pipeline.rscrates/ourios-ingester/src/recovery.rscrates/ourios-ingester/src/snapshot_store.rscrates/ourios-ingester/tests/ingest_support/mod.rscrates/ourios-ingester/tests/rfc0001_3_5_snapshot_restore.rscrates/ourios-miner/src/cluster.rscrates/ourios-miner/src/mask.rscrates/ourios-miner/src/snapshot.rscrates/ourios-miner/tests/invariants.rscrates/ourios-server/src/receiver.rscrates/ourios-server/tests/rfc0008_10_recovery_driver.rs
… 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>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-ingester/tests/rfc0001_3_5_snapshot_restore.rs (1)
207-216: 💤 Low valueConsider adding
stale_gapassertion for consistency with the corrupt-version test.The analogous
rfc0001_3_5_2_corrupt_version_discards_and_full_replaystest asserts!report.tenants[0].stale_gapat line 166. Adding the same assertion here would maintain consistency across discard-scenario tests.assert_eq!( report.tenants[0].outcome, RecoveryOutcome::UnknownOrCorruptDiscarded, ); + assert!(!report.tenants[0].stale_gap); assert_eq!(report.records_suppressed_for_miner, 0);🤖 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-ingester/tests/rfc0001_3_5_snapshot_restore.rs` around lines 207 - 216, Add the missing assertion to check that the tenant's stale_gap is false in this discard-scenario test: locate the assertions block in the rfc0001_3_5_snapshot_restore test (where report.tenants.len(), outcome, records_suppressed_for_miner, records_fed_to_miner, and assert_equivalent are asserted) and insert an assertion like assert!(!report.tenants[0].stale_gap) immediately after the outcome check to mirror the behavior asserted in rfc0001_3_5_2_corrupt_version_discards_and_full_replays.
🤖 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.
Nitpick comments:
In `@crates/ourios-ingester/tests/rfc0001_3_5_snapshot_restore.rs`:
- Around line 207-216: Add the missing assertion to check that the tenant's
stale_gap is false in this discard-scenario test: locate the assertions block in
the rfc0001_3_5_snapshot_restore test (where report.tenants.len(), outcome,
records_suppressed_for_miner, records_fed_to_miner, and assert_equivalent are
asserted) and insert an assertion like assert!(!report.tenants[0].stale_gap)
immediately after the outcome check to mirror the behavior asserted in
rfc0001_3_5_2_corrupt_version_discards_and_full_replays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5aeb01d9-8ad8-4073-9691-62b2190f3b11
📒 Files selected for processing (7)
crates/ourios-ingester/src/receiver/pipeline.rscrates/ourios-ingester/src/recovery.rscrates/ourios-ingester/src/snapshot_store.rscrates/ourios-ingester/tests/rfc0001_3_5_snapshot_restore.rscrates/ourios-miner/src/cluster.rscrates/ourios-server/src/receiver.rscrates/ourios-server/tests/rfc0008_10_recovery_driver.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/ourios-server/src/receiver.rs
- crates/ourios-ingester/src/snapshot_store.rs
- crates/ourios-ingester/src/recovery.rs
- crates/ourios-miner/src/cluster.rs
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 3 — the final slice of the snapshot-restore workstream (spec #185, WAL mechanism #186). The ingester now restores miner state from per-tenant snapshots and replays only the WAL tail, end to end in the served binary.
ourios-miner):recover()returns the parsedSnapshotStateon a known version (RecoveryOutcome::Restored) instead of discarding it;MinerCluster::restore_tenantrebuilds a tenant tree from a snapshot — 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 tree candidates share their firstwalk_depthmasked tokens by construction, so this is total), structured-template map andtemplate_countrebuilt, and the cluster-widetemplate_idallocator bumps past every restored id. Semantically inconsistent snapshots areRestoreError::Inconsistent→ discarded as corrupt (§6.9).ourios-ingester::recovery): restore per tenant →Wal::replaythrough a sink that decodes, fans out, and feeds the miner only frames above that tenant's high-water mark (per-tenant horizons — RFC 0001 §6.9 is per-tenant scope, so the singleSof RFC0008.10 generalises per tenant). The Parquet horizon (last_checkpoint) is read and reported. Stale-gap detection per §3.5.4:S < checkpoint ∧ S.segment ∉ replayed segments— internally unreachable under the §6.7 retain floor, so a hit means external mutation; surfaced loudly, never silent.ourios-ingester::snapshot_store):<wal_root>/snapshots/<tenant>.snap, atomic tmp → fsync → rename → parent-fsync (theCHECKPOINTwriter's pattern).serve()): open WAL → recover → warn stale gaps → write post-recovery snapshots → then construct the pipeline and bind listeners (RFC0008.10 ordering: recovery completes before serving).shutdown()writes snapshots again after both listeners stop; a write failure degrades to a warning — the snapshot is a rebuildable cache, never durable state.RFC-gated contract change (explicit, per CLAUDE.md §6.2)
The miner test asserting the v1 "known version is still discarded" contract is retired and replaced by the restore contract — this is exactly the contract change the RFC 0001 §6.9 v2 amendment (#185) approved. The §3.5.2 invariant test is updated to the new
recoversignature with its assertions strengthened (addsrestored == None), not weakened.Tests
Swith a checkpoint above it → loud per-tenant report, survivors still fold.serve()→ live HTTP export → shutdown artefacts equal a control).Invariants / hazards
Journal::syncnow returns the offsetWal::syncalready produced (no new fsync). The WAL remains the truth — a lost/corrupt/inconsistent snapshot degrades to full replay in every branch.Honest deferrals (not faked)
checkpointin production yet (no atomic-publish callback), so the timer would be dead machinery.Checks run
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --all-features(648 tests, zero failures),cargo doc --no-deps --all-features— all green locally.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests