BLA-4200: LRU observation capture (producer 1/3)#8
Merged
shreyas-blacksmith merged 5 commits intoJul 9, 2026
Merged
Conversation
Introduce the producer<->retention-sweep contract for LRU artifacts with no runtime behavior yet: - LRUObserver interface (RecordACAccess) plus a nil-safe, panic-swallowing ObserveACAccess helper, mirroring the existing OperationObserver pattern. - ACClosure/LRUObject types and LRUArtifactHeader matching the design doc artifact schema (short h/s keys, sizeOnDisk unit, per-entry ts_ms). - JSONL encode/decode (single source of truth FA must reuse) and a chronologically-sortable object-key helper (zero-padded window-end ms). Golden-byte, round-trip, key-sort, and observer nil/panic tests included. Co-authored-by: Cursor <cursoragent@cursor.com>
Produce one ACClosure per touched AC to a (possibly nil) LRUObserver, with sizes gathered at existing validation touch points (zero extra round trips): - LeafSizeSink (cache pkg, context-threaded) records leaf sizeOnDisk from the local LRU index (findMissingLocalCAS) and from the proxy StatObject.Size that s3proxy.Contains currently discards for CAS/v2 (returned value unchanged). - GetValidatedActionResult assembles the hit closure (output files, expanded Tree files, the Tree blobs themselves, stdout/stderr) under complete-or-drop; capture is gated on an observer so the default path is unchanged. - AC writes emit only when the closure is fully resolvable now: writes with output directories or inlined leaves emit nothing and defer to the read path (D10), so no partial closure ever reaches an artifact. - WithLRUObserver option; leaf-size-source and dropped-observation counters. Tests: hit closure + dedup + tree leaves, local-vs-proxy leaf size source, no extra proxy stat under capture, write with/without directories, complete-or-drop on unresolved leaf, nil-observer no-op, and an s3proxy StatObject->sink test backed by gofakes3. Co-authored-by: Cursor <cursoragent@cursor.com>
Record output_directories Tree blobs as closure leaves using a
local-index size lookup instead of re-adding them to pendingValidations,
so capture can never turn a proven AC hit into a miss. Emit the
lru_leaf_size_source{missing} counter on complete-or-drop so the
zero-extra-round-trip size assumption (D11) is observable in the field.
Add capture-overhead microbenchmarks and widen the put helpers to
testing.TB so benches can reuse the existing test seam.
Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 82ddb3c. Configure here.
lookupSizeOnDisk resolved leaf sizes via SizedLRU.Get, which moves each entry to the MRU side. The AC write capture path calls it for every referenced CAS digest without reading those blobs, so enabling an LRU observer perturbed on-disk eviction order. Add SizedLRU.Peek (no MoveToFront) and use it, with a regression test on eviction order. Co-authored-by: Codesmith Staging <codesmith-bot@users.noreply.github.com>
ajwerner
approved these changes
Jul 9, 2026
| // blobs are small and this only runs when an observer is configured. | ||
| var acForObserver *pb.ActionResult | ||
| if kind == cache.AC && c.lruObserver != nil { | ||
| acBytes, rErr := io.ReadAll(r) |
Contributor
There was a problem hiding this comment.
do we want to bound the size here? Like if somebody for whatever reason sends a very large action result should we just fail? Maybe use a limit reader at 512k or something?
Contributor
Author
There was a problem hiding this comment.
Landed in a816d00, with two bounds instead of one:
- Per-request: 4 MiB cap on the declared size. Went above 512k because 4 MiB is the gRPC max-message default — every AC arriving via
UpdateActionResultfits under it by construction, so the gate only bites the HTTP path, where no transport limit exists. Oversized ACs are still cached normally via the streaming path (constant memory); they just skip observation. Capture is advisory, so it shouldn't gate the write. - Aggregate: 64 MiB process-wide semaphore across concurrent captures — the per-request cap alone leaves a worst case of (concurrent AC writes x 4 MiB), which scales with stream count. Non-blocking
TryAcquire: no budget means skip the observation, never delay the write. Both skips are counted inbazel_remote_lru_observations_dropped_total(ac_too_large/capture_budget).
On "should we just fail": the declared-size gate alone bounds nothing if the client lies, since ReadAll trusted the body. The read now goes through LimitReader(size+1), and a body longer than declared fails with a 400 — that request is malformed, so failing it is correct. Honest-but-large just isn't observed.
The capture path buffered AC writes with io.ReadAll on a client-declared size. Bound it two ways: a 4 MiB per-request cap (gRPC max-message default, so only the HTTP path can exceed it) and a 64 MiB process-wide semaphore across concurrent captures. Either bound skips the advisory observation only - the write proceeds on the streaming path. Bodies longer than the declared size are now rejected as malformed instead of buffered unboundedly. Co-authored-by: Cursor <cursoragent@cursor.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
2 participants
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.
BLA-4200: LRU observation capture (producer, part 1 of 3)
Repo: useblacksmith/bazel-remote, branch
shreyaskalyan/bla-4200-bazel-cache-lru-artifacts-> merge FIRST, then re-pin fa (FastActions/fa#4428). Full feature narrative (flows, decisions, E2E) lives in the web PR: useblacksmith/web#8767.Changes in this repo
cache/lru_observer.go- theLRUObservercontract: a nil/panic-safe observation seam mirroring the existingOperationObserverpattern, plus the JSONL artifact schema (ACClosure/LRUObject/LRUArtifactHeader, schema_version 1) and the zero-padded chronological object-key layout. This file is the single source of truth shared with fa (serialization) and the web sweep (consumption).diskCache- observations fire on validated AC hits (viaGetValidatedActionResult, where the closure is already expanded) and on AC writes viaPutunder complete-or-drop: writes withoutput_directoriesor inlined leaves defer to the first read, and a closure with any unresolvable leaf size is dropped whole - no partial record is ever emitted. Plain CAS traffic is never observed on its own.LeafSizeSinksize harvesting - leafsizeOnDiskis recorded at the existing validation touch points (local index lookups + the proxyStatObject.Sizethat was previously discarded), context-threaded per request: zero extra round trips on the request path (D5/D11). Tree blobs are emitted as closure leaves (the sweep must keep a kept AC's Tree blob or the entry breaks).ac_too_large/capture_budgetdrops); the write proceeds on the streaming path. Bodies longer than the declared size are rejected as malformed (400) instead of buffered unboundedly.bazel_remote_lru_leaf_size_source_total{source}(the field validator for the zero-round-trip assumption: a non-trivialmissingrate means the design must be revisited) andbazel_remote_lru_observations_dropped_total{reason}.lru_capture_bench_test.go).Design doc
docs/bazel-cache-lru-artifacts-plan.md- decisions D1-D17 (Slices 1+2 of section 9).Validation
go test ./cache/...green.Reviewer note
No production observer is wired here - fa injects it (part 2/3); with a nil observer the cache path is unchanged.