feat(snapshot): restore from a registry, and survive being interrupted - #1176
Merged
Conversation
`bootstrap stelae` refused `oci://` and told the operator to publish to a directory instead. It no longer does, and the other half of ADR-004's Phase 3 lands with it: the progress file, so an interrupted restore costs at most the layer that was in flight rather than the whole thing. The protocol gains `stelae::plan`: the progress file's shape, the resume rule, and remaining-bytes accounting. The rule is content addressing rather than a policy — a layer is done when its `diffId` is recorded, which is a fact about bytes, so an epoch layer completed under an older inscription stays done under a newer one. `Resume::is_done` takes a `Digest` and nothing else, so the wrong rule is unspellable rather than merely undocumented. `SteleReader` gains `compressed_size`, the one number an inscription cannot carry: identity is anchored on uncompressed bytes, so "how much is left to download" comes from the transport. A registry reads it off the manifest; a directory stats the blob file, which is not the double-read it already pays for `blob_index`. The profile supplies the half the protocol cannot — which layers may be skipped. Epoch layers may; state shards never may, because they are the tip and because a shard's scope names no epoch. The checkpoint lands after each epoch layer's own commit, and the progress file is deleted after the live-UTxO rebuild rather than after `set_cursor`, so the window between those two is exactly what `--continue` repairs. `Restoring` and `Target` replace four and three loose arguments that had reached clippy's ceiling; `RepoRef` moves out of `snapshot/publish.rs` so both commands hold repository names to one grammar. `--continue` improves the completeness hazard and does not close it. Whether `set_cursor` should move after the live-UTxO rebuild is ADR-004's ordering and stays with its owner; nothing here reorders the pipeline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds resumable restore checkpoints, compressed-work accounting, validated OCI repository handling, registry point selection, registry restoration, CLI support, shared registry fixtures, and directory and OCI integration tests. ChangesRestore and registry workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Repository
participant Registry
participant restore_registry
participant Checkpoint
participant RestoreStores
CLI->>Repository: parse OCI source and point
CLI->>Registry: open repository
Registry->>restore_registry: pull selected stele
restore_registry->>Checkpoint: load or create progress
restore_registry->>RestoreStores: restore fetched layers and state shards
restore_registry->>Checkpoint: record completed layers
restore_registry->>RestoreStores: rebuild indexes
restore_registry->>Checkpoint: remove completed progress
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
scarmuega
marked this pull request as ready for review
August 7, 2026 01:00
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/snapshot/src/restore.rs (1)
566-642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making "no checkpoint" unrepresentable instead of an empty-path sentinel.
Checkpoint::none()stores an emptyPathBuf, and bothrecordandclearre-checkself.path.as_os_str().is_empty(). A third write path added later must remember the same guard. AnOption<PathBuf>moves the check to the type and removes the duplication.This is optional; the current code is correct.
♻️ Proposed refactor
pub struct Checkpoint { - path: PathBuf, + path: Option<PathBuf>, resume: Resume, progress: RestoreProgress, }pub fn none() -> Self { Self { - path: PathBuf::new(), + path: None, resume: Resume::none(), progress: RestoreProgress::new(Digest::from_bytes([0; 32])), } }fn record(&mut self, diff_id: Digest) -> Result<(), Error> { - if self.path.as_os_str().is_empty() { - return Ok(()); - } - - self.progress.record(diff_id); - self.progress.save(&self.path)?; + let Some(path) = &self.path else { + return Ok(()); + }; + + self.progress.record(diff_id); + self.progress.save(path)?; Ok(()) }fn clear(&self) -> Result<(), Error> { - if self.path.as_os_str().is_empty() { - return Ok(()); - } - - RestoreProgress::remove(&self.path)?; + let Some(path) = &self.path else { + return Ok(()); + }; + + RestoreProgress::remove(path)?; Ok(()) }
Checkpoint::openthen setspath: Some(Self::path_in(storage_path)). Note thatself.progress.recordborrows&mut selfwhilepathis borrowed; split the borrow withlet path = self.path.clone()or reorder if the borrow checker objects.🤖 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/snapshot/src/restore.rs` around lines 566 - 642, Refactor the checkpoint path representation used by Checkpoint::none, Checkpoint::open, record, and clear from an empty PathBuf sentinel to Option<PathBuf>. Store None for checkpoints without persistence and Some(Self::path_in(storage_path)) for opened checkpoints, then guard record and clear by matching on the option while preserving existing progress recording, saving, and removal behavior.
🤖 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/snapshot/tests/restore_registry.rs`:
- Around line 38-64: Add the crate-level OCI feature gate to the publish test
module, matching the existing gate in restore_registry.rs, so publish.rs and its
registry_fixture module are excluded when the oci feature is disabled.
In `@crates/snapshot/tests/restore.rs`:
- Around line 726-745: Correct the doc comment for
a_newer_inscription_keeps_the_epoch_layers_and_redoes_the_tip to match the
implementation: both export_to calls use the same domain and plan, producing
byte-identical steles and state shards. Remove the claims about a one-slot
cursor advance and differing state shards while preserving the explanation of
the resume behavior being tested.
---
Nitpick comments:
In `@crates/snapshot/src/restore.rs`:
- Around line 566-642: Refactor the checkpoint path representation used by
Checkpoint::none, Checkpoint::open, record, and clear from an empty PathBuf
sentinel to Option<PathBuf>. Store None for checkpoints without persistence and
Some(Self::path_in(storage_path)) for opened checkpoints, then guard record and
clear by matching on the option while preserving existing progress recording,
saving, and removal behavior.
🪄 Autofix
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: 6df6397e-081f-4f5f-953b-0ee8d945ee1d
📒 Files selected for processing (19)
crates/snapshot/src/registry.rscrates/snapshot/src/restore.rscrates/snapshot/tests/publish.rscrates/snapshot/tests/registry_fixture/mod.rscrates/snapshot/tests/restore.rscrates/snapshot/tests/restore_registry.rscrates/stelae/src/dir.rscrates/stelae/src/lib.rscrates/stelae/src/oci.rscrates/stelae/src/plan.rscrates/stelae/src/transport.rscrates/stelae/tests/oci.rssrc/bin/dolos/bootstrap/mod.rssrc/bin/dolos/bootstrap/stelae.rssrc/bin/dolos/main.rssrc/bin/dolos/repo.rssrc/bin/dolos/snapshot/publish.rstests/node/mod.rstests/stelae_restore.rs
`bin/dolos/repo.rs` was a 160-line module in the binary root, beside the commands that run a node, for parsing `oci://HOST/PATH`. It should not have existed, and the reason it did was a signature: `Registry::open` took the host and the repository path as two already-split strings. So every caller split the URL itself and handed back pieces the transport immediately glued together again — while the only crate holding the grammar to split it correctly was the one being handed the pieces. The binary went further and parsed the name with `oci-client` through a re-export *and threw the result away*, keeping a hand-rolled split instead. `stelae::oci::Repository` now owns the whole of naming a repository: the `oci://` scheme (which is the ecosystem's, not this project's — Helm and ORAS spell it the same way), the distribution grammar, and the refusals. One of those refusals is not expressible outside this crate: `Reference`'s parser applies registry defaults, so `oci://dolos/mainnet` silently addresses Docker Hub. It is refused by parsing and then checking the registry it reports is the text the operator wrote. With the type in the transport, the binary has nothing left to own. `repo.rs` is deleted, `--repo` and `--source` hold the real type, and the duplicate `Point` in `bootstrap/stelae.rs` goes with it — it existed only because the profile's lived behind a feature. That feature is gone: `dolos` depends on `dolos-snapshot/oci` unconditionally. It was default-off so that `aws-lc-sys` and its `cmake` requirement were a choice a builder made, and keeping the flag meant keeping a non-gated parse outside the transport — the shim that grew into the module. Building the registry client into every `dolos` is the price of not having one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 (2)
crates/snapshot/src/registry.rs (2)
101-203: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject epoch-tag mismatches before restore.
Point::Epoch(epoch)callsRegistry::pull_sequence(p, epoch), which only rendersepoch-N, pulls that tag, and returns the result. If the registry pointsepoch-Nat a valid manifest for another sequence, restore uses the wrong stele. Checkstele.inscription.sequence == epochforPoint::Epoch(...)and return a dedicated mismatch error; keepLatestas a moving point.🤖 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/snapshot/src/registry.rs` around lines 101 - 203, Update Point::pull so Point::Epoch verifies the returned stele’s inscription.sequence matches the requested epoch before returning it. Add and return a dedicated Error variant for mismatches, including the requested and actual sequences; preserve Latest’s moving-tag behavior unchanged.
66-78: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClarify what
Point::Epochmeans before merge.
Point::Epoch(n)passesndirectly as the registry sequence, andPoint::Epoch(0)matchesepoch-0. The export contract says the stele sequence istip_epoch + 1, but this new documentation saysEpochis a Cardano epoch and the profile name, not a protocol sequence. Make the contract match the generated name: either add one before callingpull_sequence(with checked overflow) ifEpochtargets Cardano epochs, or clarify thatepoch-Nnames protocol sequenceN, including the first published epoch asepoch-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/snapshot/src/registry.rs` around lines 66 - 78, Clarify the `Point::Epoch` contract and align it with `restore_registry` and `DolosProfile::tag_for_sequence`: either treat the value as a Cardano epoch and convert `n` to the registry sequence `n + 1` with checked overflow before `pull_sequence`, or explicitly document that `Epoch(n)` denotes protocol sequence `n`, including `Epoch(0)` as `epoch-0`; ensure the export contract and generated tag semantics use the same interpretation.
🤖 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 `@Cargo.toml`:
- Line 47: Update the release Docker image setup associated with the
dolos-snapshot `oci` feature so the Debian 12 slim build environment installs
the native compiler, linker, pkg-config, and OpenSSL development prerequisites
required by stele/oci and oci-client before running `cargo build --locked
--release`; retain CA certificates and clean the package lists afterward.
---
Outside diff comments:
In `@crates/snapshot/src/registry.rs`:
- Around line 101-203: Update Point::pull so Point::Epoch verifies the returned
stele’s inscription.sequence matches the requested epoch before returning it.
Add and return a dedicated Error variant for mismatches, including the requested
and actual sequences; preserve Latest’s moving-tag behavior unchanged.
- Around line 66-78: Clarify the `Point::Epoch` contract and align it with
`restore_registry` and `DolosProfile::tag_for_sequence`: either treat the value
as a Cardano epoch and convert `n` to the registry sequence `n + 1` with checked
overflow before `pull_sequence`, or explicitly document that `Epoch(n)` denotes
protocol sequence `n`, including `Epoch(0)` as `epoch-0`; ensure the export
contract and generated tag semantics use the same interpretation.
🪄 Autofix
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: 81227f4e-5949-408d-b3e7-5ada240a5cae
📒 Files selected for processing (9)
Cargo.tomlcrates/snapshot/src/registry.rscrates/snapshot/tests/registry_fixture/mod.rscrates/stelae/src/lib.rscrates/stelae/src/oci.rscrates/stelae/tests/oci.rssrc/bin/dolos/bootstrap/stelae.rssrc/bin/dolos/snapshot/publish.rstests/stelae_restore.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/stelae/tests/oci.rs
- crates/snapshot/tests/registry_fixture/mod.rs
- tests/stelae_restore.rs
- crates/stelae/src/lib.rs
- src/bin/dolos/bootstrap/stelae.rs
The test named `a_newer_inscription_keeps_the_epoch_layers_and_redoes_the_tip` exported the same stele twice and asserted the two digests were equal. So the property in its name — that a layer completed under an older inscription stays done under a newer one, because a `diffId` names bytes — was never exercised here at all. The doc comment described the geometry it should have had. Both exports now stand at synthetic chain points a slot apart, the way `tests/publish.rs` builds its second publish: one closing epoch 0, one inside epoch 1. That makes the two sides of the rule real bytes rather than a description, and both are asserted before the resume runs — epoch 0's layers are identical across the steles, and no state shard is, because a shard's header names the epoch it is the tip of. The comparison at the end moves from the domain to an uninterrupted restore of the same stele. A synthetic cursor is not one the harness ledger ever reached, so the domain was never the right thing to compare against. Confirmed to bite: with `Resume::is_done` stubbed to `false`, it fails on "every epoch layer the older stele had completed was kept", 0 against 3. Found by CodeRabbit on #1176, which flagged the doc comment. The comment was the visible half. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 12, 2026
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
1 participant
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.
Closes Phase 3 of ADR-004. Plan:
plans/dolos-stelae-registry-restore.mdin the Trellis domain root. Offmainat608307c8— the #1173 merge.src/bin/dolos/bootstrap/stelae.rsrefusedoci://with a message telling the operator to publish to a directory instead. It no longer does. The other half is the part ADR-004 deferred into this phase twice over — the progress file, resume, and download estimation — so an interrupted restore costs at most the layer that was in flight rather than starting again.What changed
crates/stelae/src/plan.rs(new, protocol). The progress file's shape and its atomic write;Resume;Remaining.The resume rule is a consequence of content addressing rather than a policy: a layer is done when its
diffIdis recorded, which is a fact about bytes, so an epoch layer completed under an older inscription is still the layer this restore would fetch.Resume::is_donetakes aDigestand nothing else — there is deliberately no way to ask it about a kind, a scope, a sequence or an inscription, because comparing any of those would be the wrong rule and the cheapest place to rule it out is the signature.Not layer selection and not the preflight, which ADR-004's code-layout sketch also puts here. Both live profile-side already, for the reason
restore.rsstates: a layer'sscopeis opaque to the protocol.SteleReader::compressed_size— the one trait addition. The inscription carries only uncompressed sizes, because identity must not depend on a compressor, so this is the only place "how much is left to download" can come from. A registry reads it off the manifest; a directorystats the blob file. Neither reads a blob, so thefile://double-read is untouched.crates/snapshot/src/restore.rssupplies the half the protocol cannot: which layers may be skipped. Epoch layers may. State shards never may — they are the tip, and independently of that a shard's descriptor scope is{"shard": n}and names no epoch, so nothing in a shard's identity could distinguish one publish's tip from another's. The checkpoint lands after each epoch layer's own commit, which is possible only because the driver commits per layer.The progress file is deleted after the live-UTxO rebuild, not after
set_cursor. Those are two different moments and only the later one means the restore is finished; clearing it at the cursor would take away the resume that repairs the window between them.RestoringandTargetreplace four and three loose arguments. Adding one parameter took four signatures past clippy's ceiling, and these are the groupings that were already implicit — what the node knows about itself, and where the restore writes.Naming a repository moved into
stelae::oci::Repository(second commit).Registry::openused to take the host and path as two already-split strings, so every caller split the URL itself and handed back pieces the transport immediately glued together again — while the only crate with the grammar to split it correctly was the one receiving the pieces. The binary also parsed the name withoci-clientthrough a re-export and threw the result away. The type now owns theoci://scheme, the grammar and the refusals;src/bin/dolos/repo.rsis deleted and the duplicatePointinbootstrap/stelae.rswith it.One refusal is not expressible outside the transport:
Reference's parser applies registry defaults, sooci://dolos/mainnetsilently addresses Docker Hub. It is caught by parsing and checking the registry it reports is the text the operator wrote.The
registryfeature is gone —dolosdepends ondolos-snapshot/ociunconditionally. Keeping it optional meant keeping a non-gated parse outside the transport, which is the shim that grew into the module. See the risk below.Done criteria
a_registry_restore_is_a_directory_restorea_killed_registry_restore_resumes_where_it_stopped, plus both backends intests/restore.rsa_pre_seeded_node_fetches_only_what_it_lacks--continueagainst a newer inscription keeps epoch layers, redoes the tipa_newer_inscription_keeps_the_epoch_layers_and_redoes_the_tip--forceremoves the progress file with the data it describesclearing_storage_removes_a_restore_in_progress#[ignore]d, run, output quotedCriterion 6 — the ignored suite, run
cargo test -p dolos-snapshot --features oci --test restore_registry -- --ignored --nocapture, againstregistry:2,registry:3andzot, all green:The publish suite and
crates/stelae/tests/oci.rswere re-run too (the docker fixture moved, andStele::compressed_sizebecametotal_compressed_size): 5 and 8 ignored tests, all passing.The interruption is a layer boundary, never a moment
Both resume suites interrupt through a
SteleReaderdecorator that refuses adiffIdthe test named, taken from the plan rather than the inscription — the document's layer order is the export's, the driver's is epoch-by-epoch and kind-by-kind, and only the second says what has committed by the time a given layer is asked for. A kill after a wall-clock delay would sometimes interrupt nothing over a loopback registry and pass for the wrong reason.Through the binary, against a live relay
A preview node synced from
relay.cnode-m1.demeter.run:3002to slot 17980, published into a spawnedregistry:2, and restored back — then killed withSIGKILLmid-restore and resumed:3,300 compressed bytes remaining against 413,403 for a fresh restore, and the resumed node lands at the original's tip across all four stores.
--point epoch-1and--point latestboth resolve;--point tipis refused by the parse, before--forceclears anything.Criterion 7 — the gate
cargo test— 159 passed, 0 failedcargo test --features registry— 162 passed, 0 failedcargo clippy --all-targets --all-features -- -D warnings— cleancargo +nightly fmt --all -- --check— cleancargo deny check advisories—advisories okcargo tree -p stelae -e normal --all-features— no^dolos(-|$)packageFindings reported rather than absorbed
blockslayer leaves dead space in the archive flat files. The redb archive appends block bodies to segment files and keeps a slot-keyed table of offsets, so a resume that redoes the layer in flight rewrites the table entries and leaves the superseded bodies with nothing pointing at them. Reads go through the table, so the node is correct; the dead space is bounded by one layer and is the price of not starting over. Stated in the module documentation rather than left for an operator to discover.--continueimproves the completeness hazard and does not close it. A resumed restore always redoes the state tip and the live-UTxO rebuild, so the partial-utxo::*node is now repairable where before it could only be thrown away. Whetherset_cursorshould move after the rebuild is ADR-004's ordering and stays with its owner; the pipeline is not reordered here.Checkpoint::openhonours a progress file only under--continue. Not merely reads it — honours it. A restore that was not asked to resume overwrites the file rather than obeying it, which is what makes--forcesafe even against a progress file that somehow outlived a wipe. Pinned bya_restore_that_is_not_resuming_honours_no_progress_file.cmakeis now required to builddolos, and I could not verify that on every release target. Removing theregistryfeature putsaws-lc-sys(viaoci-client → jsonwebtoken → aws-lc-rs) into the default dependency tree. CI already builds it green in the Linux--all-featuresrun, and my local default build passes — but this machine hascmakeinstalled, so that proves nothing about one without it. Newly exposed and unverified here:Test (windows-latest),Test (macos-14), and the fourdisttargets,x86_64-pc-windows-msvcabove all. Worth watching the first CI run on this PR.oci-clientalso offers anative-tlsfeature that avoidsaws-lc-sysentirely; choosing between the two is [dolos-stelae-tls-without-cmake]'s call, not this PR's, and landing it would remove this risk.No escalation was raised: nothing in the plan required authority this role does not have.
🤖 Generated with Claude Code
Summary by CodeRabbit
--point.