fix(cache): verify cache checksum on restore - #4169
Conversation
71fb0a0 to
0709d21
Compare
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two issues in the verifier need addressing before merge; details 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 11438, 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.
0709d21 to
598b75d
Compare
598b75d to
27f4eab
Compare
There was a problem hiding this comment.
The algorithm guard and cancellation handling are addressed. The remaining latency question is re-raised in its existing thread; I found no new issues in 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 11499, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| func verifyBlobDigest(ctx context.Context, archiveFile string, digest api.CacheDigest) error { | ||
| if digest.Algorithm != "sha256" { | ||
| return nil | ||
| } | ||
| f, err := os.Open(archiveFile) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open archive for digest check: %w", err) | ||
| } | ||
| defer func() { _ = f.Close() }() | ||
|
|
||
| // Hash in chunks, checking ctx between reads, so a cancelled restore aborts | ||
| // promptly instead of reading the whole archive. | ||
| sum := archive.NewChecksumSHA256(io.Discard) | ||
| buf := make([]byte, 1024*1024) |
There was a problem hiding this comment.
The linear ticket has a note but it wasn't clear enough, my bad. But as I commented in the other PR, it'd be best if we avoid reading the same archive file twice from the disk.
zhming0
left a comment
zhming0
left a comment
There was a problem hiding this comment.
I left one blocking comment.
2 participants
Description
When we download a cache, its object name in storage is the SHA-256 of its contents (content-addressed storage). Until now we trusted that the bytes we downloaded matched that name but never checked — so a corrupt or half-written upload would sail through, we'd wipe the target folders, and the failure would surface later as a confusing "not a valid zip file" error, after we'd already destroyed any good existing state.
This adds a verification step on restore: re-hash the downloaded archive and confirm it matches the name it was fetched under, before we touch any target folder.
The entry is deliberately not invalidated on a mismatch: the stored bytes are wrong, not the entry, and this is a read-side safety check only.
Context
https://linear.app/buildkite/issue/A-1494/verify-cache-checksum-on-restore
Changes
verifyBlobDigestininternal/cache/restore.go, reusing the existing streamingarchive.ChecksumSHA256calculator that save already uses. Unknown digest algorithms are skipped rather than failed, so a future algorithm can't turn every verifiable cache into a false mismatch.downloadCacheafter the download completes and before returning to the caller (i.e. beforecleanPath); on failure it cleans up the temp dir, recordscache.digest_mismatchon the span, and returns a newErrDigestMismatchsentinel.ErrDigestMismatchinRestoreas a clean miss:CacheHit=false,CacheRestored=false, no error returned,slog.Warn+ span attributes for observability, no entry invalidation.TestCacheIntegration_RestoreDigestMismatchIsMiss: corrupts a stored blob's bytes (keeping its content-addressed name), then asserts the restore is a clean miss, a pre-existing sentinel file in the target folder survives, and no expire/invalidate call fires.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Affiliation (optional, external contributors)
Disclosures / Credits