feat(rcache): retry transient artifact fetches with exponential backoff - #931
Conversation
…oot) The cache index is currently a single mutable root object. Builds publish immutable per-commit copies alongside it; this teaches the reader to use them (#870, phase 1): - mfile: CacheConfig resolved centrally from [cache] index_source, the upstream pin, and an override (auto = snapshot with root fallback; pinned = snapshot only, missing is an error; root = today's behavior). The fetch layer follows instructions and holds no policy. - rcache: IndexSource {Root, Snapshot} threaded through the reader constructors. Snapshot 404 is Error::SnapshotMissing (never an empty index); local copies of snapshots never expire (immutable); a snapshot's GCS generation never seeds into_writer, so writers always compare-and-swap against the root. - mctx: MINIMAL_INDEX_SOURCE env override; auto-mode fallback with a provenance log line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iles Review findings on the snapshot fast path: - Local copies are written via temp file + rename, so a crash or concurrent reader never observes a partial file; the write is best-effort (logged, not fatal). - Load-time check that a local copy is a whole number of wire records: the parser reads till EOF and would otherwise silently accept a mid-record truncation as a shorter index — permanently, since snapshot copies never expire. Any bad copy now falls through to a refetch. - Snapshot local filenames flatten the full object key, so distinct snapshots never share a file even when different repos pin the same commit hash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the body An error status fell through to body parsing; an empty 5xx body parses as an empty index, masking the outage (and now getting cached locally). Surface it as a backend error before parsing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A body truncated mid-record but delivered as a complete response parses as a shorter index, and the local write would launder it into a well-formed permanent copy. Reject it before parsing, as the local-copy load already does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A single mirror blip failed whole builds, and an error status fell through to the hasher, reporting corruption where there was none. - materialize() checks the response status before hashing; error statuses surface as backend/status errors, never HashMismatch. - Transient failures (transport errors, error statuses, hash mismatches from complete-but-wrong bodies) retry with exponential backoff, 500ms base. Local I/O errors and absent specs do not. - Retry count: [cache] fetch_retries in minimal.toml or MINIMAL_FETCH_RETRIES env, default 3; 0 disables. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
| fn is_transient(e: &Error<<B::Response as FetchResponse>::Error>) -> bool { | ||
| matches!( | ||
| e, | ||
| Error::Backend(_) | Error::Fetch { .. } | Error::HashMismatch { .. } |
There was a problem hiding this comment.
If the status code was 200, do we want to consider HashMismatch a transient error?
There was a problem hiding this comment.
Yes, deliberately — a 200 carrying wrong bytes is exactly the observed incident that motivated this issue: the mirror served a complete response whose body hashed to garbage, and the same key served correct bytes minutes later. Transport-level truncation usually fails the Content-Length check, so complete-but-wrong is precisely the shape the mismatch retry is for.
The asymmetry that makes it safe: if the object is persistently corrupt, retrying costs ~3.5s of backoff and then surfaces the same loud HashMismatch (covered by the exhausts-retries test); if it's the transient mirror class, the retry saves the whole build. Sharpened the is_transient doc comment to record this reasoning in-code.
Review: the translation from mfile::CacheConfig to IndexSource, and the auto-mode fallback, lived in mctx — but not everything goes through mctx. RemoteCache::new_any_configured now consumes the config directly (rcache -> mfile is dependency-clean; graph already depends on mfile), so every caller gets identical fallback and provenance-logging behavior. mctx shrinks to resolving the env override. Also marks MINIMAL_INDEX_SOURCE as a rollout lever to retire once snapshot reads are the settled default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ries # Conflicts: # crates/mctx/src/lib.rs
# Conflicts: # crates/mctx/src/lib.rs # crates/mfile/src/lib.rs # crates/rcache/src/lib.rs # crates/rcache/src/remote.rs
Fixes #879. Stacked on #912 (same
Errorenum and[cache]config section) — shows only its own commit once #912 merges and this retargets.What
The incident class: a single transient mirror error failed a whole build, presenting as
HashMismatchbecause the error body was fed to the hasher — indistinguishable from real corruption.materialize()now rejects error statuses up front (Error::Backendviaerror_for_status,Error::Fetch { status }fallback). An error page never reaches the hasher, soHashMismatchagain means what it says. The index fetch's status fallback switches to the sameFetchvariant for consistency.NotFound(spec absent from the index) and local I/O errors are not retried. Extraction runs once, after a verified fetch.[cache] fetch_retries = Nin minimal.toml,MINIMAL_FETCH_RETRIESenv override (resolved centrally in the same place asindex_source),RemoteCache::with_fetch_retries()on the API.0disables retries. Each retry logs a warn line with attempt count and delay.Testing
Mock backend gained scripted per-URL response sequences (status + body) and a call counter; retry tests run under tokio's paused clock so backoff costs no wall time:
fetch_retries = 1→HashMismatchafter exactly 2 attempts (real corruption still fails loudly)fetch_retries = 0→ fails fast as a status error, notHashMismatch, proving the error page never reached the hasherNotFoundwith zero fetch attempts🤖 Generated with Claude Code
Note
Retry transient artifact fetches in
RemoteCachewith exponential backoffRemoteCache::materializenow retries failed artifact downloads on transient errors (transport failures, HTTP error status, hash mismatches) using exponential backoff with a 500ms base, up to a configurable retry count (default: 3).RemoteCache::fetch_verifiedto encapsulate per-attempt fetching, status checking, streaming to a temp file, and SHA-256 verification.IndexSourceenum;new_any_configuredresolves the index source frommfile::CacheConfig, with automatic fallback to the root index when a snapshot is missing.MINIMAL_FETCH_RETRIESenv var, the[cache] fetch_retriesfield inminimal.toml, orRemoteCache::with_fetch_retries.Error::Fetchinstead of silently producing an invalid or empty index.Changes since #931 opened
ScriptedResponsestype alias and applying it toMockBackend::scriptedfield [483b242]Macroscope summarized ed52664.