Make cache save/restore fail-open by default

[ss1909]

Description

A cache is an optimization, not a build input — but today a buildkite-agent cache save or cache restore failure aborts the whole cache batch and exits non-zero, failing the build. That means a transient cache-backend blip fails otherwise-green builds, and for save it fails a build whose real work has already completed (the cache upload happens after the build step's work).

This changes the default to fail-open: a per-cache save/restore error is logged as a warning and skipped, so the build proceeds (cold, but green). It extends the philosophy already in restore.go — "a bad cache must never block a build", currently applied to corrupt/mismatched restore blobs — to transient API errors and to save failures.

A new --cache-fail-on-error flag (BUILDKITE_AGENT_CACHE_FAIL_ON_ERROR) opts back into the previous strict/fatal behaviour.

Key decision for reviewers: this flips the default (fatal → fail-open). The alternative is to keep fatal-by-default and gate fail-open behind an opt-in flag — but that wouldn't fix the reported build failures for anyone who doesn't know to set it. Happy to invert if the team prefers.

Not changed: setup errors (missing token, bad cache config, unresolved config file) and context cancellation (e.g. a cancelled build) remain fatal — fail-open applies only to per-cache save/restore errors inside the dispatch loop.

Context

Prompted by a build failure where cache save for a ~2.3 GB Go build cache returned repeated 500s and failed the step, even though the build + image push had already succeeded. (The server-side cause — a 4-byte file_size overflow — is being fixed separately in buildkite/buildkite#33851; this PR addresses the orthogonal question of whether a cache error should ever fail a build.)

Changes

• internal/cache: add Config.FailOnError; thread it into saveWithClient/restoreWithClient. On a per-cache error, when FailOnError is false (default) log a warning and continue to the next cache instead of cancelling the batch. Context-cancellation still stops the loop quietly.
• clicommand: add the --cache-fail-on-error flag (env BUILDKITE_AGENT_CACHE_FAIL_ON_ERROR, default false) to the shared cache flags, wired into both cache save and cache restore.
• Tests: existing error-propagation tests now assert the strict path (failOnError=true); new tests assert fail-open skips a failing cache and still processes the rest.

cache save --help gains:

--cache-fail-on-error  Fail the command (non-zero exit) when a cache save or restore fails.
                       By default a cache is best-effort: failures are logged and skipped so
                       they never fail the build [$BUILDKITE_AGENT_CACHE_FAIL_ON_ERROR]

Public documentation

• Ready for public documentation - document and publish
• Not ready for public docs - document and hold
• Not applicable - docs not needed

Testing

• Tests have run locally (with go test ./...)
• Code is formatted (with go tool gofumpt -extra -w .)

Ran locally in a clean worktree: go build ./internal/cache/... ./clicommand/..., go vet on both, and go test ./internal/cache/... (pass, including the new fail-open tests). Did not run the full go test ./... suite locally — relying on CI for the remainder.

Disclosures / Credits

Authored with Claude Code (Opus 4.8): it investigated the originating build failure, proposed the fail-open design, and wrote the implementation and tests. Reviewed by the PR author before submission.

[buildsworth-bk-app]

The restore path has one blocking correctness issue; details are in the inline comment.

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 26300, 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. Unless repository policy already allows L2 approval, grant it by mentioning @buildsworth-bk (see approval ceiling and 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 when repository policy or a verified grant allows it; otherwise comment-only.
• L3 — High risk (auth, migrations, payments, secrets, perf-critical pipeline paths). Human review always required.

Previous concerns have been addressed.

[buildsworth-bk-app]

The previous restore-path concern is fixed. The default failure-semantics change affects job execution, so it is above the L1 approval ceiling; I found no further issues.

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 26315, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[buildsworth-bk-app]

The merge from main doesn't introduce any new issues in the cache failure handling. This still changes cache command failures from fatal to fail-open during job execution, so it remains above the L1 approval ceiling.

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 26480, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.