feat(rcache): read the index from per-commit snapshots (auto/pinned/root) - #912
Conversation
📝 WalkthroughWalkthroughThe PR adds root and per-commit remote-cache index selection, resolves it from file settings or ChangesRemote cache index selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Mctx
participant Mfile
participant Rcache
participant Storage
Mctx->>Mfile: resolve cache configuration
Mctx->>Rcache: initialize selected index source
Rcache->>Storage: fetch root or snapshot object
Storage-->>Rcache: index bytes or missing object
Rcache-->>Mctx: parsed remote cache reader
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rcache/src/remote.rs (1)
285-316: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-404 error statuses fall through to body-parsing instead of erroring.
The match only special-cases
404per source; any other non-2xx status (500, 403, 429, …) hits the_arm and attempts to parse the response body as anIndexFile. Since the explicit 404 branches are reachable at all,execute()clearly doesn't convert error statuses toErritself, so this is a real gap: a transient backend error could either surface as a confusingError::IO(parse failure) instead of a clear backend error, or worse, if the error body happens to be empty/parseable, silently be treated as an empty index — masking an outage rather than surfacing it.
FetchResponsealready exposesis_success()/error_for_status()(see theMockResponseimpl in this file's tests), so this is fixable without new API surface.🛡️ Proposed fix to surface non-404 backend errors explicitly
let index = match (index_resp.status_code(), &source) { (404, IndexSource::Root) => IndexFile::default(), (404, IndexSource::Snapshot { object }) => { return Err(Error::SnapshotMissing { object: object.clone(), }); } + (_, _) if !index_resp.is_success() => { + return Err(Error::Backend(index_resp.error_for_status().unwrap_err())); + } _ => {🤖 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/rcache/src/remote.rs` around lines 285 - 316, Update the index response handling around the match on index_resp.status_code() so every non-404 unsuccessful response is returned as a backend error before body parsing. Preserve the existing IndexSource::Root 404 default and IndexSource::Snapshot 404 SnapshotMissing behavior, and use FetchResponse’s existing error_for_status() or equivalent success check before the parsing arm.
🧹 Nitpick comments (2)
crates/rcache/src/remote.rs (2)
104-112: 🚀 Performance & Scalability | 🔵 TrivialUnbounded local snapshot cache growth.
Each distinct commit gets its own permanently-cached
<commit>.shishafile with no expiry (by design, since snapshots are immutable). Worth confirming there's a pruning/GC mechanism forindex_direlsewhere, since over the life of a long-lived repo this could accumulate one file per built commit indefinitely.🤖 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/rcache/src/remote.rs` around lines 104 - 112, Inspect the index_dir cache lifecycle around IndexSource::local_filename and add or reuse a pruning/GC mechanism that removes obsolete snapshot index files while preserving files still referenced by active snapshots. Ensure cleanup is invoked as part of the existing cache maintenance flow so immutable snapshots do not accumulate indefinitely.
239-266: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSynchronous filesystem calls inside an async fn.
std::fs::metadata,std::fs::File::open, andstd::fs::File::createall run directly on the async task inRemoteCache::new. Based on learnings, this repo avoids blocking filesystem work in async contexts and preferstokio::task::spawn_blockingso a wedged/slow disk (NFS home dir, degraded disk, etc.) can't stall the async worker running this daemon-side call.Also applies to: 305-313
🤖 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/rcache/src/remote.rs` around lines 239 - 266, Update RemoteCache::new to move all synchronous filesystem operations, including metadata/modified checks, File::open, and the File::create logic referenced around the additional location, into tokio::task::spawn_blocking closures. Await those operations and propagate their existing Error::IO failures while preserving the current cache freshness and file-handling behavior.Source: Learnings
🤖 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.
Outside diff comments:
In `@crates/rcache/src/remote.rs`:
- Around line 285-316: Update the index response handling around the match on
index_resp.status_code() so every non-404 unsuccessful response is returned as a
backend error before body parsing. Preserve the existing IndexSource::Root 404
default and IndexSource::Snapshot 404 SnapshotMissing behavior, and use
FetchResponse’s existing error_for_status() or equivalent success check before
the parsing arm.
---
Nitpick comments:
In `@crates/rcache/src/remote.rs`:
- Around line 104-112: Inspect the index_dir cache lifecycle around
IndexSource::local_filename and add or reuse a pruning/GC mechanism that removes
obsolete snapshot index files while preserving files still referenced by active
snapshots. Ensure cleanup is invoked as part of the existing cache maintenance
flow so immutable snapshots do not accumulate indefinitely.
- Around line 239-266: Update RemoteCache::new to move all synchronous
filesystem operations, including metadata/modified checks, File::open, and the
File::create logic referenced around the additional location, into
tokio::task::spawn_blocking closures. Await those operations and propagate their
existing Error::IO failures while preserving the current cache freshness and
file-handling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa7a5ab8-1ee8-4e7f-b9d2-4299ffd13f00
📒 Files selected for processing (4)
crates/mctx/src/lib.rscrates/mfile/src/lib.rscrates/rcache/src/lib.rscrates/rcache/src/remote.rs
|
Both review findings addressed in 32c8a14:
New regression test covers the corrupt-copy refetch; the existing expiry test covers the flattened naming. |
|
CodeRabbit's outside-diff Major is valid and now fixed in 78e333a: non-404 error statuses on the index fetch no longer fall through to body parsing. The empty-5xx-body case was the sharp edge: it parses as an empty index, and with this PR's local caching would also have been written to disk. Now surfaced as a backend error before parsing, with a regression test (500 + empty body => Error::Backend). Same bug class as the artifact-fetch path tracked in #879. |
|
Post-review sweep of the couplings around the changed code — three checks, all clean, documenting for reviewers:
|
|
Macroscope's follow-up finding (remote.rs:347) is valid and fixed in e16695d: the whole-record check now applies to freshly fetched index bodies, not just local copies. It was right about the laundering: a truncated download parses as a shorter index, and the local write would re-serialize it as well-formed whole records — permanently passing the local check. Now rejected before parsing, with a regression test (complete response, mid-record body => InvalidData). Transport-level truncation usually fails the Content-Length check, but a mirror returning complete-but-wrong bodies is a failure mode we have observed, so the format-level check earns its keep. |
…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>
9557c36 to
fa086ec
Compare
| let res = | ||
| RemoteCache::new_any(url, gcs_storage, index_dir, self.daemon.config.ot.clone()).await; | ||
|
|
||
| let override_mode = match std::env::var("MINIMAL_INDEX_SOURCE") { |
There was a problem hiding this comment.
Do we plan to turn this env-var down once things are settled?
There was a problem hiding this comment.
yeah, that was my idea
There was a problem hiding this comment.
Yes — it's the rollout lever: pinned proves the snapshot path end-to-end and root is the kill-switch while this beds in. Marked it as such in 394dc17 (comment says retire the env var first, then likely the [cache] index_source file setting too, once snapshot reads are the settled default). Happy to file the retirement as a tracked follow-up once it's merged.
There was a problem hiding this comment.
Sweet, just checking. Defs want to keep this logic pretty tight long term
| { | ||
| mfile::CacheConfig::GlobalIndex => (IndexSource::Root, false), | ||
| mfile::CacheConfig::CommitIndex { object } => (IndexSource::Snapshot { object }, true), | ||
| mfile::CacheConfig::CommitIndexOnly { object } => { |
There was a problem hiding this comment.
Can RemoteCache::new or RemoteCache::new_any consume mfile::CacheConfig and do this logic in there? Feels like a cleaner separation than in mctx as not everything goes through mctx.
There was a problem hiding this comment.
Done in 394dc17: RemoteCache::new_any_configured consumes mfile::CacheConfig directly and owns the translation, the auto-mode fallback, and the provenance log lines — so any caller gets identical behavior. mctx shrinks to resolving the env override and handing over the config. Dependency-wise it's clean: rcache -> mfile adds nothing transitively (graph already depends on mfile). The primitive new_any(..., IndexSource) stays for callers that need explicit control (and the writer-adjacent constructors remain root-only). Two new tests cover the configured path: auto falls back when the snapshot is missing; pinned errors rather than falling back even when root could serve.
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>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mctx/src/lib.rs`:
- Around line 531-537: Update the MINIMAL_INDEX_SOURCE environment-variable
handling around override_mode so only an unset variable returns None; propagate
VarError::NotUnicode as a RemoteError::Config instead of silently falling back.
Preserve the existing parsing and error formatting for valid Unicode values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b86e09cb-c38a-4d15-b670-ae1d38750dc0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/mctx/src/lib.rscrates/mfile/src/lib.rscrates/rcache/Cargo.tomlcrates/rcache/src/index_file.rscrates/rcache/src/lib.rscrates/rcache/src/remote.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/rcache/src/lib.rs
- crates/mfile/src/lib.rs
- crates/rcache/src/remote.rs
Phase 1 of #870, shaped per the config-enum suggestion: the policy (which index object to read) is resolved centrally and handed to the fetch layer as concrete instructions.
What
CacheConfigenum (GlobalIndex/CommitIndex/CommitIndexOnly), resolved byFile::cache_config()from[cache] index_source(auto|pinned|root), the upstream pin (repo+locked_commit-><host>/<owner>/<repo>/<commit>.shisha, matching the publisher's keying), and an optional override.autois the default and degrades to the root index when there's no pin.IndexSource { Root, Snapshot }threaded through the reader constructors; no policy in this layer.Error::SnapshotMissing(the caller decides on fallback) — never an empty index, and corrupt-but-present snapshots still error rather than fall back.into_writer— writers always refetch and compare-and-swap against the root object.new_over_https/new_with_gcs_bucketare unchanged and always read root (writer-adjacent paths, where root is authoritative).CacheConfig->IndexSource, honours theMINIMAL_INDEX_SOURCEenv override, implements auto-mode fallback, and logs provenance (debugfor the chosen snapshot,infowhen falling back).Modes
rootindex.shishaauto(default)pinnedTesting
mfile: slug normalization across URL forms (https,.git, credentials,ssh://, scp-like), mode x pin resolution matrix,[cache]parsing + override precedence.rcache: snapshot fetch over real HTTP transport, snapshot-404 =>SnapshotMissing(root present, proving no silent root read), snapshot-cache-never-expires vs root-expiry behavior.autofalls back identically to today's read path.🤖 Generated with Claude Code
Note
Read remote cache index from per-commit snapshots in
RemoteCacheIndexSourceenum (RootorSnapshot { object }) torcachesoRemoteCache::newcan load either the global root index or a per-commit snapshot index object.CacheSettingsandIndexSourceMode(auto,pinned,root) tomfile, plus acache_configresolver that maps the project'sminimal.toml[cache]config and upstream pin to the appropriateIndexSource.mctxreads an optionalMINIMAL_INDEX_SOURCEenv-var override and callscache_configto select the index source, passing it toRemoteCache::new_any; returnsSnapshotMissingerrors and optionally falls back to the root index.RemoteCachenow returnsSnapshotMissinginstead of an empty index when a per-commit snapshot object is absent, and non-2xx responses are now hard errors rather than parsed.Changes since #912 opened
rcache[394dc17]mctxwith configuration-based approach [394dc17]mfiledependency torcachecrate [394dc17]MINIMAL_INDEX_SOURCEenvironment variable parsing inmctx::Contextcache configuration to return configuration error for invalid values [beb0969]Macroscope summarized fa086ec.
Summary by CodeRabbit