Repository navigation
fix(rcache): an index hit whose artifact is gone is a miss, not a failure - #2624
Merged
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Contributor
|
Approved: no findings at Nothing to do. Clobber: none seen. |
…lure An index hit followed by a 404 on the artifact object failed the resolve with a hard `Backend(...404...)`, retried as if transient. It is a normal state of this architecture: a snapshot index keeps every record forever, and the store's retention (or a redistribution fix) deletes objects out from under them. `fetch_verified` now returns `Error::NotFound` on a 404, after one attempt, with a `cache_dangling_hit` warning naming the object, so a caller treats it as the miss it is: the patched-build path already does; the orchestrator's hydrate is the other half, filed separately per Norrie's call on inbox#535. Refs: gominimal/inbox#535 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review on #2624. `NotFound` displayed as its Debug name, so a dangling hit reached the session fetch and `cache_hydrate` as the bare word `NotFound`, worse than the 404 and URL it replaced. The variant carries the object now (the spec hash for an index miss, the artifact name for a gone object) and reads "the remote cache has no artifact for <object>"; the session package fetch and the orchestrator's hydrate prefix the package name, so the user sees which package and what. The dangling-hit warning states only the fact, names the object, and carries `kind = "dangling_hit"` as a field for the COGS counter rather than in the words. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bryan-minimal
force-pushed
the
fix/535-dangling-index-hit
branch
from
October 26, 2026 18:00
735ae79 to
ef9e10c
Compare
From review (#2624). `Error::NotFound` carried a spec hash from the index lookup and an artifact file name from the store fetch under one wording; it now says which ("spec hash", "artifact file") before the identifier. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Found by the lab (min-regress scenario 800): with the index retry no longer repeating a 404 (#2623), a prefetch of a package whose artifact the store no longer has failed the session spawn with "the remote cache has no artifact for artifact file ...". On main the same case only passed because the 404 was retried and the stub served the artifact the second time. The prefetch now treats `NotFound` as the miss it is, commits nothing for it, and lets the build make the package, as the op path already does; rcache has logged the dangling hit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… failing it The planner trusts the remote index: a spec the index lists is scheduled as a cache fill, which has no build behind it. When the store no longer has the artifact (a dangling hit, inbox#535) the fill failed the whole build with "the remote cache has no artifact for ...", which the lab's scenario 800 caught on the session-spawn path after the prefetch path was fixed. The fill now reports `Error::DanglingCacheHit(bsr)`; the build driver masks that spec out of every cache provider (the same `MaskingBinProvider` that `--rebuild` uses) and plans again, so the spec is built like any other miss. Artifacts completed before the re-plan are committed first, and each pass masks one more spec, so the loop is bounded by the graph. The re-plan is logged as `dangling_replan` next to rcache's `dangling_hit`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Member
Author
|
Rounds 4 and 5 both come from the lab rather than a review thread. min-regress scenario 800 (a reverse proxy to the real cache that injects one failure per mode) runs this branch against main on debian-12, with #2623's round 5 merged in:
The last row is the gap this stack opened: once #2623 stopped retrying a 404, every consumer of That one is different: the planner trusts the index, so a listed spec is scheduled as a cache fill with no build behind it. Round 5 makes the fill report |
…build for it A dangling cache fill failed the pass at once: `abort_all` killed every in-flight build, the driver learned one spec, and a retention sweep that left N dangling specs cost N+1 passes (review on #2624 round 5). Now a dangling fill fails only itself and what depends on it: the pass runs everything else to completion, then hands the whole set back as `Error::DanglingCacheHit(Vec<_>)`, and the driver masks them all for the next pass. Test with a fake backend: two listed leaves gone from the store are both reported in one pass, and the masked pass builds all three specs. Also: the prefetch's comment on a dangling hit says what a prefetch-only caller (mip add/update/check) gets, which is the rcache warning and no cached package, not a build. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Member
Author
|
Lab evidence for round 5 (7f37698), as Lab evidence (min-regress batch Head: fix/535-dangling-index-hit at 7f37698 (lab name
Fixed since base: 1. Still reproducing on both: 0. Regressed: 0. Passing on both: 0. A verdict links to the run's trace in Cloud Trace (project access needed); the scenario link is its source with the The same scenario is queued against round 6 (cbb5920); its block follows when it lands. |
Member
Author
|
Lab evidence for round 6 (cbb5920), written by Lab evidence (min-regress batch Head: fix/535-dangling-index-hit at cbb5920 (lab name
Fixed since base: 1. Still reproducing on both: 0. Regressed: 0. Passing on both: 0. A verdict links to the run's trace in Cloud Trace (project access needed); the scenario link is its source with the 800-remote-index-retry on debian-12: base fail (trace 813e1fff)800-remote-index-retry on debian-12: head pass (trace 79cc2c9f) |
norrietaylor
approved these changes
Oct 28, 2026
yash-marathe
pushed a commit
to yash-marathe/minimal
that referenced
this pull request
Oct 29, 2026
…gominimal#2623) * fix(rcache): retry transient 5xx on the index fetch One transient 5xx from the cache server on the first index/closure fetch aborted the whole session spawn, even though artifact downloads already rode out such failures with bounded retries. Give the index fetch the same treatment as materialize: retry transient failures (transport errors, error statuses) up to DEFAULT_FETCH_RETRIES times with the shared 0.5s/1s/2s exponential backoff. 404s and SnapshotMissing stay non-transient so the snapshot-to-root fallback is untouched. When retries run out, the error names the object and HTTP status (Error::Unavailable) instead of surfacing the raw reqwest debug dump. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 6b8ad3d) * chore(worker-iterate): address CodeRabbit findings on PR gominimal#2496 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 855e0b1) * fix(rcache): retry only a 5xx or 429 on the index fetch, never a 4xx `is_transient` counted every `Error::Fetch { .. }` as transient, and the index path now maps every non-success status to it, so a 401, 403 or 404 on a snapshot object was retried three times with backoff and then reported as "try again shortly": the wrong advice for a refusal, and the whole backoff added to every such failure. A `Fetch { status }` is transient only for a 5xx or 429; transport errors and hash mismatches are retried as before. A scripted 403 pins it: one ACB9 call, `Fetch { status: 403 }`. The artifact path still classifies through `error_for_status`; its doc says so. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(rcache): never retry an unbuildable request; classify artifact statuses as the index does Review on gominimal#2623. `backend.get` can fail before anything is sent (a URL the backend cannot represent); that came back as `Backend`, was retried three times with backoff, and then read "the cache server was unreachable". It is `Error::Request` now, deterministic and never retried. The artifact path classified a refused status through `error_for_status` into `Backend`, so a 404 or 403 on an object was retried where the index path's was not; both paths now return `Fetch { status }`, and `is_transient` has one rule, in one doc paragraph: transport, 5xx/429 and a hash mismatch retry; a 4xx, a request that could not be built, local I/O and a missing spec do not. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(rcache): box the index retry loop so min's dispatch stays under rustc's query depth Inlined, the retry loop added one layer to the future of every command that opens the cache, and the layout of `run_command`'s future then overran rustc's query depth limit (128) at the `Box::pin` in cmd/mod.rs, on 1.99.0 and 1.99.1 alike: `queries overflow the depth limit!`. Pinned on the heap, the loop's state is a pointer in the caller's state and the depth stops there. No behaviour change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * build(minimal): raise the recursion limit where the in-process daemon is compiled The depth overflow moved: boxed in the cache, it reappeared in the test crates (`bug`, `git_remote`) while computing the layout of `minimald::env::BoxChannel::handle`'s async body, the daemon these tests run in-process. rustc's help says the fix: `#![recursion_limit = "256"]`, which minimald, mctx, op and mip already declare and the `min` crate roots did not. Added to the bin, the lib and every integration test root. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(rcache): retry an index body cut off mid-record From review (gominimal#2623). A 200 whose body is not a whole number of records is a cut-off response, the same class of failure as a transport error, so it is `UnexpectedEof` and the retry loop treats it as transient; a malformed index stays `InvalidData` and is not retried. When every attempt comes back cut off the index is reported unavailable, never accepted as a shorter one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * refactor(rcache): the retry policy is one function, used by the index and the artifact fetch Review on round 5: the retry loop was mixed into the business logic twice, once around the index fetch and once around an artifact's. It is `retrying_transient` now: try `once` until it succeeds, fails in a way `is_transient` does not cover, or the retry budget is spent, with the same bounded exponential backoff and log lines; the caller keeps the meaning of a spent budget (the index reports `Unavailable`, an artifact surfaces the error). The index's `Box::pin` stays for the query-depth reason it carries. 40 rcache tests unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(rcache): an index hit whose artifact is gone is a miss, not a failure (gominimal#2624) * fix(rcache): an index hit whose artifact is gone is a miss, not a failure An index hit followed by a 404 on the artifact object failed the resolve with a hard `Backend(...404...)`, retried as if transient. It is a normal state of this architecture: a snapshot index keeps every record forever, and the store's retention (or a redistribution fix) deletes objects out from under them. `fetch_verified` now returns `Error::NotFound` on a 404, after one attempt, with a `cache_dangling_hit` warning naming the object, so a caller treats it as the miss it is: the patched-build path already does; the orchestrator's hydrate is the other half, filed separately per Norrie's call on inbox#535. Refs: gominimal/inbox#535 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(rcache): a miss names its object, and the callers name the package Review on gominimal#2624. `NotFound` displayed as its Debug name, so a dangling hit reached the session fetch and `cache_hydrate` as the bare word `NotFound`, worse than the 404 and URL it replaced. The variant carries the object now (the spec hash for an index miss, the artifact name for a gone object) and reads "the remote cache has no artifact for <object>"; the session package fetch and the orchestrator's hydrate prefix the package name, so the user sees which package and what. The dangling-hit warning states only the fact, names the object, and carries `kind = "dangling_hit"` as a field for the COGS counter rather than in the words. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(rcache): say which identifier a not-found names From review (gominimal#2624). `Error::NotFound` carried a spec hash from the index lookup and an artifact file name from the store fetch under one wording; it now says which ("spec hash", "artifact file") before the identifier. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(mctx): a dangling index hit is a miss in the session's prefetch too Found by the lab (min-regress scenario 800): with the index retry no longer repeating a 404 (gominimal#2623), a prefetch of a package whose artifact the store no longer has failed the session spawn with "the remote cache has no artifact for artifact file ...". On main the same case only passed because the 404 was retried and the stub served the artifact the second time. The prefetch now treats `NotFound` as the miss it is, commits nothing for it, and lets the build make the package, as the op path already does; rcache has logged the dangling hit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(orchestrator): a dangling index hit re-plans the build instead of failing it The planner trusts the remote index: a spec the index lists is scheduled as a cache fill, which has no build behind it. When the store no longer has the artifact (a dangling hit, inbox#535) the fill failed the whole build with "the remote cache has no artifact for ...", which the lab's scenario 800 caught on the session-spawn path after the prefetch path was fixed. The fill now reports `Error::DanglingCacheHit(bsr)`; the build driver masks that spec out of every cache provider (the same `MaskingBinProvider` that `--rebuild` uses) and plans again, so the spec is built like any other miss. Artifacts completed before the re-plan are committed first, and each pass masks one more spec, so the loop is bounded by the graph. The re-plan is logged as `dangling_replan` next to rcache's `dangling_hit`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(orchestrator): one pass learns every dangling spec and aborts no build for it A dangling cache fill failed the pass at once: `abort_all` killed every in-flight build, the driver learned one spec, and a retention sweep that left N dangling specs cost N+1 passes (review on gominimal#2624 round 5). Now a dangling fill fails only itself and what depends on it: the pass runs everything else to completion, then hands the whole set back as `Error::DanglingCacheHit(Vec<_>)`, and the driver masks them all for the next pass. Test with a fake backend: two listed leaves gone from the store are both reported in one pass, and the masked pass builds all three specs. Also: the prefetch's comment on a dangling hit says what a prefetch-only caller (mip add/update/check) gets, which is the rcache warning and no cached package, not a build. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <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.
Summary
The rcache half of gominimal/inbox#535, per Norrie's split there. Stacked on the inbox#1162 PR (
fix/1162-index-retry), which this sharesremote.rswith; GitHub retargets it tomainwhen that merges.An index hit followed by a 404 on the artifact object failed the resolve with a hard
Backend(...404...), and after #1162's retries would have retried it as transient. It is a normal state of this architecture, not an outage: a snapshot index keeps every record forever, and the store's retention (or a redistribution fix, which is how the August incident happened) deletes objects out from under them.fetch_verifiednow returnsError::NotFoundon a 404 after one attempt, with acache_dangling_hitwarning naming the object, so a caller treats it as the miss it is. The patched-build path already does (a dangling dependency is now "could not find", not "could not fetch"); the session package fetch surfaces aNotFoundinstead of a reqwest dump.Not in this PR: the orchestrator half.
cache_hydratecannot fall back tobuild()becauseCacheFilldeliverables carry no dependency edges; that is a graph change and gets its own issue, as Norrie asked on inbox#535.Testing
materialize_dangling_index_hit_is_a_miss(rcache): index hit, artifact 404 →NotFoundafter one artifact attempt.op::patched:reports_remote_fetch_failure_distinctly_from_a_missnow serves a corrupt artifact (a real fetch failure); newa_dangling_index_hit_is_reported_as_a_misscovers the 404 case.cargo nextest run -p rcache -p op: all green;just clippyclean; workspace tests through anvil's CI mirror.🤖 Generated with Claude Code