Review summary: buildkite/agent PR #4237

Change reviewed

This PR dogfoods Cache v2 in the agent repository’s real CI rather than in an isolated smoke step. It adds .buildkite/cache.yml definitions for the Go module cache, normal build cache, and race build cache; removes the old host cache mounts from the lint and agent Docker Compose services; and wires cache restore/save calls into lint, protobuf generation, tests, binary builds, and the bk CLI test.

The pipeline obtains short-lived AWS credentials through a shared aws-assume-role-with-web-identity plugin definition and passes those credentials, the cache store URL, and—where required—the mounted agent binary into Docker Compose jobs. Lint writes the shared module cache and Linux/AMD64 build cache. Shard 0 writes the ARM64, Windows, and race build caches, while the other jobs restore only. The Linear issue, A-1638, asks the team to dogfood Buildkite cache in the agent repository and report findings.

What I examined

I read the complete eight-file PR diff and the full current contents of the changed pipeline, Docker Compose, cache configuration, and shell scripts. I also traced the relevant cache implementation paths:

• cache configuration validation and name filtering in internal/cache/client.go and internal/cache/configuration/cache.go;
• key resolution in internal/cache/configuration/key_part.go;
• fixed-address save behavior in internal/cache/save.go;
• home-relative target resolution, cleanup, and extraction in internal/cache/archive and internal/cache/restore.go;
• S3 credential and content-addressed blob behavior in internal/cache/store/s3.go;
• default .buildkite/cache.yml discovery in clicommand/cache_shared.go.

I compared the current revision (cbe3873c) with the previously reviewed revision (67d4919). The only new code change is the race-cache naming correction in .buildkite/steps/tests.sh: gocache-race / .gocache-race now use gocache_race / .gocache_race, matching .buildkite/cache.yml.

I also read the full PR review conversation and rechecked the two open buildsworth threads. There was no L2 approval grant in the PR description or comments.

Checks performed

• bash -n and sh -n passed for all five changed shell scripts.
• The installed Buildkite Agent successfully parsed and validated .buildkite/cache.yml; it reached the expected sentinel error for a deliberately nonexistent cache name rather than reporting a configuration error.
• buildkite-agent pipeline upload --dry-run --format json --no-interpolation --apply-if-changed=false parsed .buildkite/pipeline.yml successfully. The resulting pipeline expanded the YAML anchor into the expected cache-role plugin instances and retained the required Docker environment forwarding and agent mounts.
• GitHub reported the category-label and Socket checks passing. The main buildkite/agent build #14000 was still pending when the review was submitted.

I could not run Go tests because the Go toolchain is not installed in this review environment. The available Buildkite API credentials also could not access the organization’s build details, so I could not inspect build #14000’s jobs or logs and relied on its GitHub commit status. I did not independently execute the live S3/OIDC cache path or the native Windows job.

Findings and disposition

The prior blocking race-cache issue is fixed. Cache configuration validates every definition before selecting --name, and the script now uses the same underscore-only cache name and target directory as the shared config. I acknowledged that fix and resolved its GitHub thread.

The earlier non-blocking cache-refresh concern remains. Both gocache and gocache_race still have fixed exact addresses consisting only of v2, OS, and architecture. internal/cache/save.go returns without uploading whenever that address already exists, so the first successful snapshot for each platform cannot be refreshed; newly produced outputs will be rebuilt on later jobs but never added to the shared entry. I kept this in its existing thread rather than opening a duplicate.

I found no new code issue in the current revision. The PR description separately acknowledges that large gocache saves currently hit an external server-side size limit and that the Windows OIDC plugin path has not yet been verified end-to-end.

Submitted verdict

I submitted a COMMENT review with no new inline findings. The code-level blocker from the previous pass is resolved, but the cache-refresh concern remains open, the primary CI build was still pending, and the change affects live cache behavior plus OIDC-provided AWS credentials, so it still warrants human verification.

Review: #4237 (review)

Trigger source: automatic.