feat(cache): cache v2 in the agent - #4237
Conversation
2874146 to
1cb0be1
Compare
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
The dogfood path cannot currently reach its first cache operation; I also found that the active smoke definition does not exercise fallback as described. Details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14333, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
977c3e9 to
8acdb7d
Compare
There was a problem hiding this comment.
Both prior concerns remain open on the current revision; I’ve kept the details in their existing threads.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14350, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
bf83819 to
45276eb
Compare
There was a problem hiding this comment.
The binary-mount blocker is fixed. The fallback coverage concern remains, and I found two issues in the skipped Go-cache path; details are inline. This changes live CI/cache behavior, so I’m leaving a comment-only review.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14364, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
Activating the Go-cache step makes the bind-mounted restore path a blocker; the fallback and branch-key concerns also remain open in their existing threads.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14394, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
968138b to
5b4ca3e
Compare
There was a problem hiding this comment.
The branch-key concern is fixed. The bind-mounted restore blocker and fallback coverage concern remain open in their existing threads.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14402, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
One prior concern is now addressed. The fallback/scenarios concern remains open in its existing thread because the new job cannot start.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14677, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
531130e to
54ff2d1
Compare
There was a problem hiding this comment.
The fallback/scenarios blocker remains open on the current revision; I’ve kept the detail in its existing thread. I found no new issues this pass.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14695, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
54ff2d1 to
3c8ce01
Compare
There was a problem hiding this comment.
The fallback/scenarios blocker remains open in its existing thread; I found no new issues this pass.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14702, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous concerns have been addressed.
There was a problem hiding this comment.
The scenarios step can now start, and I found no new issues in this revision. This changes live CI/cache behavior, so I’m leaving a comment-only review.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 14706, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
a9ed28b to
1c396ba
Compare
1c396ba to
a50ff2c
Compare
b1a96f8 to
fcf362f
Compare
Previous concerns have been addressed.
There was a problem hiding this comment.
The earlier dogfood concerns are resolved by the current integration rewrite. I found two blockers in the active cache path and two non-blocking cache coverage/effectiveness regressions; details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 15703, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| cache_key: | ||
| - v2 | ||
| - { agent: os } | ||
| - { agent: arch } |
There was a problem hiding this comment.
Non-blocking: This exact address never changes, and cache save explicitly skips an address that already exists. After the first successful Linux/AMD64 save, gocache is frozen at that snapshot: new dependency or toolchain outputs are rebuilt on every job but can never refresh the shared entry. A changing exact suffix after a fallback boundary would preserve cross-branch reuse while allowing refreshed entries.
There was a problem hiding this comment.
Still open — gocache still has a fixed exact key, so an existing entry cannot be refreshed.
There was a problem hiding this comment.
Still open — the gocache key is still fixed, so once the first entry exists, later runs cannot refresh it.
There was a problem hiding this comment.
Still open — the new platform writers do not change the fixed key, so once each platform’s first entry exists, later runs still cannot refresh it.
There was a problem hiding this comment.
Still open — both gocache and the new race cache have fixed exact addresses, so successful entries cannot be refreshed.
There was a problem hiding this comment.
Still open — both build-cache definitions retain fixed exact addresses, so existing entries still cannot be refreshed.
0c08cc5 to
5265b8c
Compare
There was a problem hiding this comment.
Moving the caches out of the checkout fixes the gofumpt blocker. The per-platform writer, frozen gocache key, and Binary-build cache regression remain open in their existing threads, and the new parent-relative targets introduce another save blocker.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 15715, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
The home-anchored targets address the latest save blocker. The per-platform writer remains blocking, and the cache-refresh and Binary-build regressions remain open in their existing threads; I found no new issues this pass.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 16010, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous blocking concerns have been addressed.
There was a problem hiding this comment.
The platform writers and Binary-build restore address the prior blocker and regression. The fixed gocache key still cannot refresh, as noted in its existing thread. This also changes live CI cache and OIDC credential handling, so it still needs a human sanity-check.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 16586, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous concerns have been addressed.
There was a problem hiding this comment.
The Windows cache path now follows the native agent's USERPROFILE home, and I found no new code issue in this revision. The fixed gocache address remains open in its existing thread. Build #13997 is currently failing, but the available GitHub status does not expose which job failed; the live cache/OIDC path still needs human verification.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 16597, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
The prior cache-refresh concern remains open in its existing thread, and this revision adds a new blocker in the shared cache config; details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 16737, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
bbb46e8 to
67d4919
Compare
There was a problem hiding this comment.
The race-cache blocker remains open in its existing thread, and the non-refreshing cache-key concern is unchanged. I found no new issues this pass.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 16742, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
67d4919 to
cbe3873
Compare
There was a problem hiding this comment.
The race-cache naming blocker is fixed. The cache-refresh concern remains open in its existing thread; I found no new issues in this revision. Build #14000 is still pending, and the live cache/OIDC credential path needs human verification.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 16744, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
1 participant
Description
Integrates Cache™ v2 (Cache Registries) into this repo's real CI:
Lint,Check protobuf generation, and every step inTests and Coverage(Linux AMD64/ARM64, Windows, race) now restore the Go module cache (gomodcache) and build cache (gocache) viabuildkite-agent cache restore/save, in place of the old host-mounted~/gocache/~/gomodcachedirectories.Context
Started as an isolated dogfood step to validate cache v2's wiring (auth, S3 storage, registry policy) against production without risking the real gating CI steps. That surfaced two confirmed bugs (below) and validated fallback matching and policy enforcement (scopes + CEL rules) end-to-end. With those understood, the standalone step was redundant — it duplicated a full
go mod download/go build ./...purely to exercise cache v2, while Lint/Tests kept building from the host mount and got no benefit from it. This PR replaces that step with real integration instead.Changes
docker-compose.yml:lint/agentservices no longer bind-mount~/gocache/~/gomodcachefrom the host.GOCACHE/GOMODCACHEnow point at.gocache/.gomodcacheinside/work(the checkout) — a plain subdirectory is never a mount point itself, which also sidesteps finding Unresolved TODO in /buildbox/script.go : 51 #2 below for these two paths.cache.yml:target_pathsupdated to match (.gocache/.gomodcache, relative) — one definition works identically on Linux and Windows.gocache'scache_keyno longer includesbranch: Go's build cache is already content-addressed internally, so scoping it by branch only fragmented it across feature branches for no correctness benefit.aws-assume-role-with-web-identityOIDC role via a YAML anchor, no static credentials):Lint(check-code-committed.sh): restores at the top, saves at the end — the sole writer.Check protobuf generation: restores only.Tests and Coverage(Linux AMD64/ARM64, Windows, race): restore only, addedmount-buildkite-agent: truewhere missing sobuildkite-agentis available in-container.cache-v2-dogfoodstep and script — superseded by the above.Why only
Lintsaves — the parallel-race problem this avoidsTests and Coverageruns up to ~9 concurrent job instances across its parallelism. If every shard also saved, they'd all resolve to the samecache_key(no per-shard distinction) and redundantly re-download/rebuild just to have something to save — up to 9x the work for nothing. Saves do race safely in that scenario (peek-then-create, last-committer-wins on a narrow TOCTOU window, never a torn or corrupt entry — Go's build cache is itself content-addressed so mixed content isn't wrong either), but it's still pure waste. Restoring is read-only and safely idempotent in every shard; onlyLint(unparallelized, and already populating most ofgocacheas a side effect ofgo generate/gofumpt/golangci-lint) needs to write.Known issues
gocachesave fails once the archive exceeds ~2GiB (server-side500):CacheRegistry::Entry::Blob#file_sizeis a 4-byte:integercolumn with nolimit:override inbuildkite/buildkite. Root cause confirmed by reading the source; fix is a one-linelimit: 8, not yet applied. Until it lands,Lintwill fail red on its cache save — notsoft_fail, so the gap stays visible rather than passing silently.mount-buildkite-agentequivalent is needed (the agent binary is already on the host), but theaws-assume-role-with-web-identityplugin's behavior on that queue specifically hasn't been verified end-to-end yet.Testing
Not applicable in the usual sense — this PR only changes pipeline/cache/docker-compose config and shell scripts, no Go code. Verified instead by repeatedly triggering real builds against isolated test cache registries and confirming actual restore/save/policy behavior (cold miss → save → warm hit, scope isolation, CEL rule enforcement, default-deny) against production.
Disclosures / Credits
Claude Code was used throughout this work: designing the cache definitions and pipeline wiring, tracing the
file_sizeoverflow and mount-pointcleanPathbugs to confirmed root causes across both repos, validating cache v2's policy behavior (scopes, CEL rules, fallback matching) against production, and designing/implementing this integration pass (in-checkout cache paths, single-writer save to avoid the parallel-shard race). I reviewed and directed all of it.