feat(stelae): refuse a run that cannot fit its staging, before it starts - #1192
Merged
Conversation
A publish holds all sixteen shard sinks open across one walk of the store; a restore stages one layer at a time. Neither asked whether the volume could hold it, so the shortfall arrived as `No space left on device` hours in. One policy for both directions, in `dolos_snapshot::preflight`: a measured shortfall refuses, naming the volume and how far short it is, and what cannot be measured warns and proceeds. Needs that land on one volume are summed rather than each compared against the whole pool — which is the ordinary case, since `--scratch-dir` defaults to `<storage.path>/scratch`. Restore: `Plan::preflight` takes the staging directory alongside the destination. The need is the largest layer the run will actually pull, taken as a `max` in the pass `Remaining::of` already makes to sum the download, so a resumed run sizes what it still has to stage rather than what a fresh one would. Publish: sized off the predecessor's manifest — sixteen state-shard compressed sizes plus its largest other layer — with no per-layer HEAD. A first publish has no predecessor, cannot be sized, and warns. There is no override flag: `--scratch-dir` pointed at a bigger volume is the escape hatch. Co-Authored-By: Claude Opus 5 <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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe snapshot crate adds shared filesystem preflight checks and a ChangesSnapshot staging preflight
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PublishCommand
participant RegistryPreflight
participant Filesystem
PublishCommand->>RegistryPreflight: Preflight repository staging capacity
RegistryPreflight->>Filesystem: Check scratch-volume requirement
Filesystem-->>RegistryPreflight: Return available space
RegistryPreflight-->>PublishCommand: Continue or report NotEnoughSpace
sequenceDiagram
participant RestoreOperation
participant restore_stele
participant PlanPreflight
participant Filesystem
RestoreOperation->>restore_stele: Pass scratch directory and target
restore_stele->>PlanPreflight: Submit destination and staging needs
PlanPreflight->>Filesystem: Check required volumes
Filesystem-->>PlanPreflight: Return available space
PlanPreflight-->>restore_stele: Allow or refuse restore
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/src/preflight.rs`:
- Around line 254-262: Update the non-Unix same_volume function to compare
platform-specific volume identities rather than ancestor relationships, so
sibling paths on the same Windows drive or share are grouped together while
paths on different volumes remain separate. Add coverage in the relevant
preflight tests for sibling paths sharing one volume and verify check combines
their requirements.
In `@crates/snapshot/src/registry.rs`:
- Around line 551-558: Update the STATE branch in the loop over
inscription.layers to accumulate state_bytes with saturating addition instead of
allowing integer overflow. Preserve the existing handling for unsized layers and
largest_other_bytes, while ensuring StagingPeak::bytes() receives a non-wrapped
state total.
In `@crates/snapshot/src/restore.rs`:
- Around line 857-860: Update the staging construction around Staging and
outlook.remaining to retain the unsized layer count alongside
largest_compressed, while continuing to use the known lower-bound estimate.
Ensure staging_need emits the established warning when unsized layers remain,
matching the behavior in registry::staging_need.
- Around line 282-298: Update Plan::preflight to accept and use the resume-aware
remaining destination size calculated by restore_stele instead of
self.uncompressed_size(). Thread that value through the preflight call while
preserving staging checks, and add coverage for a resumed restore where
completed epoch layers exceed available space collectively but the remaining
layers fit.
🪄 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: e5773626-8617-478c-83b2-372c8b7de674
📒 Files selected for processing (10)
crates/snapshot/src/lib.rscrates/snapshot/src/preflight.rscrates/snapshot/src/registry.rscrates/snapshot/src/restore.rscrates/snapshot/tests/publish.rscrates/snapshot/tests/registry_fixture/mod.rscrates/snapshot/tests/restore.rscrates/stelae/src/oci.rscrates/stelae/src/plan.rssrc/bin/dolos/snapshot/publish.rs
Comment on lines
+254
to
+262
| #[cfg(not(unix))] | ||
| fn same_volume(a: &Path, b: &Path) -> bool { | ||
| let (a, b) = match (a.canonicalize(), b.canonicalize()) { | ||
| (Ok(a), Ok(b)) => (a, b), | ||
| _ => return false, | ||
| }; | ||
|
|
||
| a.starts_with(&b) || b.starts_with(&a) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Group sibling paths on non-Unix volumes.
On non-Unix platforms, two sibling paths on the same Windows drive or share are not ancestors of each other. same_volume returns false, so check compares both requirements against the same free-space pool separately. The combined requirement can exceed free space and still pass.
Use a platform-specific volume identity for non-Unix paths. Add a test with sibling paths on one volume.
🤖 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/preflight.rs` around lines 254 - 262, Update the non-Unix
same_volume function to compare platform-specific volume identities rather than
ancestor relationships, so sibling paths on the same Windows drive or share are
grouped together while paths on different volumes remain separate. Add coverage
in the relevant preflight tests for sibling paths sharing one volume and verify
check combines their requirements.
Comment on lines
+282
to
+298
| pub fn preflight(&self, path: &Path, staging: Option<Staging<'_>>) -> Result<(), Error> { | ||
| let mut needs = vec![preflight::Need::of( | ||
| "restoring it", | ||
| path, | ||
| self.uncompressed_size(), | ||
| )]; | ||
|
|
||
| if let Some(staging) = staging { | ||
| needs.push(preflight::Need::or_unsized( | ||
| "staging the layers it pulls", | ||
| staging.dir, | ||
| staging.largest_layer, | ||
| "this stele's transport states no compressed size for the layers it would pull", | ||
| )); | ||
| } | ||
|
|
||
| Ok(()) | ||
| preflight::check(&needs) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the destination requirement resume-aware.
restore_stele calculates resume-aware remaining layers at Line 846. Plan::preflight still uses self.uncompressed_size(), which includes immutable layers that the checkpoint already committed.
Completed layers already reduce current free space. Counting them again can refuse a resumed restore that has enough space to finish. Pass a resume-aware destination requirement into preflight.
Add a test where completed epoch layers make the full plan exceed free space but the remaining layers fit.
🤖 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 282 - 298, Update
Plan::preflight to accept and use the resume-aware remaining destination size
calculated by restore_stele instead of self.uncompressed_size(). Thread that
value through the preflight call while preserving staging checks, and add
coverage for a resumed restore where completed epoch layers exceed available
space collectively but the remaining layers fit.
`staging_peak` summed sixteen manifest-stated sizes with `+`. They are another document's numbers, and a wrapped total is a *small* need, which is the one direction a refusal must never be wrong in. The non-Unix `same_volume` tested containment, which answers `<storage.path>/scratch` and misses two siblings on one drive. It now compares the canonical path's prefix — the drive letter or UNC share — and the summing test covers both shapes, with the sibling directory created so it is measured as itself rather than through a shared ancestor. A restore staging need where some layers are sized and others are not took the known maximum and said nothing. It is a floor, and now says so, the way `registry::staging_need` already did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 12, 2026
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.
Plan:
plans/dolos-stelae-publisher-operability-preflight.md— the second piece of thestelae-operabilityfamily, behind #1191. Base:mainat7286c48d, exactly as the plan's Approach names it.What was wrong
A publish holds sixteen staged shards at once — all sixteen sink writers stay open across a single walk of the store, because the alternative is sixteen full scans — plus whatever epoch layer is in flight. A restore holds one layer at a time. Neither command asked whether the volume could hold that, so the operator found out as
No space left on devicehours in.The restore side was half built:
Plan::preflightchecked free space atstorage.pathagainst the selected layers' uncompressed size, but never asked about the scratch volume that #1191 had just given it. The publish side had no check at all.What changed
One policy, in one place —
crates/snapshot/src/preflight.rs, new. A measured shortfall refuses, naming the volume and how far short it is; free space that will not read, or a need nothing could size, warns and proceeds. There is no override flag —--scratch-dirpointed at a bigger volume is the escape hatch, and a better one than a flag that turns the check off. (org/founder, 2026-08-11, the umbrella's fourth escalation.)Needs that land on one volume are summed rather than each compared against the whole pool. That is the ordinary case, not the exotic one:
--scratch-dirdefaults to<storage.path>/scratch, so a restore's destination and its staging are two claims on the same free bytes. Same-volume is the filesystem device id on Unix — exact in both directions, so a dedicated mount under the storage path correctly reads as separate, and--scratch-direlsewhere on the same disk correctly reads as shared. Stable Rust exposes no device id off Unix, so there the test is containment, which is right for the default and over-states rather than under-states everywhere else.Restore —
Plan::preflightextended, not duplicated. It now takes an optionalStaging { dir, largest_layer }beside the destination and hands both to the shared check. The need is the largest layer the run will actually pull, taken as amaxin the passRemaining::ofalready makes to sum the download (Remaining::largest_compressed), so a resumed run sizes what it still has to stage rather than what a fresh one would have.The plan flagged the ordering:
preflightran above the sizes. It now runs below them, afterCheckpoint::openandblob_index— both of which only read — so ADR-004 step 2's promise, refuse before anything is written, is intact while the check can see the resume.A directory restore stages nothing (a
SteleDirreads its blobs where they are) and passesNone. Which directory a registry restore stages in comes from the transport — a newstelae::oci::Registry::scratch_dir()— not from the caller re-deriving the path it handed toopen. Two derivations of one directory is one more than can be kept in step.Publish —
registry::staging_peaksizes the sixteen-shard-plus-largest-other peak off the predecessor's manifest: no per-layerHEAD, keepingregistry::preview's promise.registry::preflightapplies the same policy to it, and runs besidestanding— before the dry run too, for the reason already written there: a publisher rehearsing with--dry-runshould get the same answer the publish gives. A first publish has no predecessor, cannot be sized, and warns.The peak is a proxy — last epoch's layer sizes, not this publish's — and the refuse/warn split is what keeps it honest: only what was actually measured can refuse.
Done criterion
cargo test, clippy, fmt,cargo deny check advisories, and thestelaedependency boundary.Verification
Run, not asserted. Both CI test legs:
The
#[ignore]d registry suites were executed against a spawnedregistry:2— this change editspublish.rsand the shared fixture, so an unrun ignored test would prove nothing:The new suite's own output, from the fixture's stele:
— which is the point the check exists for: what a publish holds at once is not what the repository holds.
Mutation-checked, because a preflight that never fires looks exactly like one that always passes:
Remaining::largest_compressedstuck atNone→the_remaining_download_excludes_what_is_already_donefails (NonevsSome(1490)).needs_sharing_a_volume_are_summedfails, and only that one.staging_peaksumming the non-state layers instead of taking the max → the registry test fails (3390vs1493).Review pass (
debbccbb)Three of CodeRabbit's four applied, all in the same commit:
staging_peaksummed sixteen manifest-stated sizes with+. They are another document's numbers, and a wrapped total is a small need — the one direction a refusal must never be wrong in. Saturating now, like the policy's own sum already was.same_volumetested containment, which answers<storage.path>/scratchand misses two siblings on one drive. It compares the canonical path's prefix — the drive letter or UNC share — and the summing test now runs both shapes, with the sibling directory created so it is measured as itself rather than through a shared ancestor. The Windows CI leg is what validates it.registry::staging_needalready did.The fourth is right about the defect and is deliberately not fixed here. The destination need is
Plan::uncompressed_size(), which is not resume-aware, so a resumed restore is charged again for layers its own checkpoint already wrote. That predates this change — the comparison has read the whole plan since it was written — and what this PR did was make it visible, by moving the check below the resume where the two needs sit side by side and disagree. Narrowing it is not arithmetic: a redone layer is rewritten rather than appended, except in the redb archive, which leaves dead space bounded by one layer, so the honest need carries slack whose size is a decision the plan did not make. Filed as its own plan rather than widened into this one.What is covered structurally rather than end to end
A test cannot shrink a filesystem, so no test drives a real registry restore into a real refusal. The chain is covered in pieces instead, and each piece is a real assertion:
Registry::scratch_dir()returns whatopenwas given (asserted against the fixture's own directory, in the registry suite);Remaining::largest_compressedis the max over the layers that remain;Plan::preflightrefuses a staging need it cannot fit and warns at one it cannot size; and the policy's summing and refusal are unit-tested against the volume the test is running on. What no test covers isrestore_stelepassing the transport's directory into that preflight — one line, and the accessor assertion is what stands behind it.Scope held
Reporting only, never repairing: a preflight that says a first publish cannot be sized is in scope; making a publish cheap is [dolos-stelae-publish-cost]. No resumption, no progress reporting.
snapshot verifyandsnapshot inspectalso stage through the scratch directory and are not preflighted here — neither is a publish or a restore, and widening to them is not this plan's.Escalations
None.
On the test level, given #1191's review
#1191 landed with
69f8007d, "drop the CLI staging suite" — a container-spawning suite and its CI leg traded away for a unit test onstele_scratch_dirplus the transport's own staging test, on the grounds that what it guarded was aPathBufargument on two commands. This change follows that line rather than re-opening it: the new coverage is unit tests on the policy and on each side's need, plus one assertion inside the existing registry suite where a real manifest is the only thing that can state the sizes. No new suite, no new CI leg.🤖 Generated with Claude Code