Skip blob retention refresh on a fallback restore - #4321
Merged
Conversation
NscStore and S3Blob refreshed a blob's retention (nsc artifact extend, S3 self-CopyObject to reset LastModified) on every download regardless of whether the restore was an exact match or a fallback -- even though the backend already skips the equivalent DynamoDB metadata TTL bump on a fallback match, since a fallback hit means the entry was saved under a shorter key sequence and refreshing it would reset the clock on a blob the caller didn't explicitly target. Add a fallback parameter to Blob.Download, threaded from retrieveResp.Fallback (already read by restore.go, just never passed down). Gate both stores' refresh on it. LocalFileBlob accepts and ignores the parameter (no retention concept for local files) -- it's still required in the signature since Go interface satisfaction needs an exact method match. Extracted the S3 self-copy into a free function parameterized on a new objectCopier interface, mirroring the existing objectDownloader/ downloadWithRetry pattern in the same file, so the refresh decision and the copy itself are both directly unit-testable.
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
jamiemonserrate
left a comment
jamiemonserrate
left a comment
Contributor
There was a problem hiding this comment.
So I think the PR makes sense - but I have a design qualm. Blob.Download taking a fallback bool doesn't make sense. Its muddling unrelated concepts.
Could we have the restore flow decide whether to call something like RefreshRetention after a successful download? I think that keeps the responsibility clearer and avoids teaching each blob store what a fallback means.
Blob.Download taking a fallback bool muddled two unrelated concerns: downloading bytes, and deciding whether to refresh a blob's retention afterward. It also forced LocalFileBlob to accept a parameter it had no use for -- a store with no retention concept at all. Revert Download to its original signature. Add a RetentionRefresher interface (RefreshRetention(ctx, key)), implemented by NscStore and S3Blob only -- optional, since not every store has a retention concept, rather than forcing every implementer to satisfy it. restore.go now decides whether to call it via a type assertion, gated on !retrieveResp.Fallback, right after digest verification succeeds (a small correctness improvement over the old placement inside Download: retention no longer gets refreshed on a blob that turns out to fail digest verification). Test coverage moves with it: each store's refresh method is tested directly, a guard test proves Download no longer touches retention, and a new maybeRefreshRetention helper in restore.go (with its own focused test) owns the fallback-gating decision that used to be duplicated inside each store.
Contributor
Author
|
thanks @jamiemonserrate , that makes sense especially since LocalFileBlob had not need for the fallback arg. I've made the following changes:
|
|
🤖 Docs Draft Bot: I've created a draft documentation PR based on this change: 📝 Docs PR: https://github.com/buildkite/docs-private/pull/2153 Please review the draft and make any necessary adjustments. |
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
3 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.
Description
The DynamoDB cache-entry metadata already skips its TTL bump on a fallback restore (a request whose exact key doesn't exist, served by an older, looser-matching entry instead) --
unless entry_result.fallback_used?in the backend. The reasoning: a fallback hit means the entry was saved under a shorter key sequence, and refreshing it would reset the clock on an entry the caller didn't explicitly target.The two blob stores didn't follow the same rule.
NscStore.Downloadrannsc artifact extend --ensure_minimum 72hon every download, andS3Blob.Downloadself-CopyObject'd to resetLastModified(the field S3 lifecycle rules key off) on every download -- both unconditionally, regardless of exact-vs-fallback.restore.goalready readsretrieveResp.Fallbackright where it calls into the blob store; it just never got passed down, sinceBlob.Download's signature had no fallback parameter.Alternative considered: none -- this mirrors an already-decided policy (metadata already does this), so the fix is just applying the same rule to the two places that didn't.
Context
https://linear.app/buildkite/issue/A-1777/align-fallback-restore-expiry-refresh-for-cache-metadata-and-blobs
Changes
internal/cache/store/blob.go--Blob.Downloadgains afallback boolparameter.internal/cache/store/nsc.go-- skips the TTL-extend call on a fallback download.internal/cache/store/s3.go-- skips the self-CopyObjectrefresh on a fallback download. Extracted the copy intorefreshObjectExpiry, parameterized on a newobjectCopierinterface mirroring the existingobjectDownloader/downloadWithRetryseam in the same file, so the refresh (and the fallback gate) is directly unit-testable.internal/cache/store/file.go--LocalFileBlob.Downloadaccepts and ignores the parameter (no retention concept for local files; still required for interface conformance).internal/cache/restore.go-- threadsretrieveResp.Fallbackinto the one productionDownloadcall site.TestNscStore_SkipsRefreshOnFallbackDownload,TestRefreshObjectExpiry's fallback case, plus existing call sites updated to preserve exact-match behavior.Testing
go test ./...)go tool gofumpt -extra -w .)Disclosures / Credits
Claude Code implemented this change end-to-end (TDD: failing test first, then the fix), under my direction and review, including a couple of follow-up simplification passes after I flagged unnecessary indirection.