From 0c0ee59213527d469d4071a82dbad2009b02f893 Mon Sep 17 00:00:00 2001 From: Sneha Date: Thu, 3 Sep 2026 11:16:20 +1000 Subject: [PATCH 1/2] Skip blob retention refresh on a fallback restore 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. --- internal/cache/restore.go | 2 +- internal/cache/store/blob.go | 5 ++- internal/cache/store/file.go | 2 +- internal/cache/store/file_test.go | 6 +-- internal/cache/store/nsc.go | 14 +++--- internal/cache/store/nsc_test.go | 39 +++++++++++++---- internal/cache/store/s3.go | 71 ++++++++++++++++++++----------- internal/cache/store/s3_test.go | 62 +++++++++++++++++++++++++++ 8 files changed, 153 insertions(+), 48 deletions(-) diff --git a/internal/cache/restore.go b/internal/cache/restore.go index bdab6b3bdd..0e24ac1d92 100644 --- a/internal/cache/restore.go +++ b/internal/cache/restore.go @@ -440,7 +440,7 @@ func (c *client) downloadCache(ctx context.Context, retrieveResp api.CacheEntryR archiveFile = filepath.Join(tmpDir, storeObjectName) // Download archive - transferInfo, err = blobStore.Download(ctx, storeObjectName, archiveFile) + transferInfo, err = blobStore.Download(ctx, storeObjectName, archiveFile, retrieveResp.Fallback) if err != nil { // Clean up temporary directory on failure _ = os.RemoveAll(tmpDir) diff --git a/internal/cache/store/blob.go b/internal/cache/store/blob.go index 46c3f77d37..8c58179c26 100644 --- a/internal/cache/store/blob.go +++ b/internal/cache/store/blob.go @@ -16,8 +16,9 @@ type Blob interface { // Upload uploads a file to blob storage Upload(ctx context.Context, filePath, key string) (*TransferInfo, error) - // Download downloads a file from blob storage - Download(ctx context.Context, key, destPath string) (*TransferInfo, error) + // Download downloads a file from blob storage. fallback reports whether the + // restore that resolved this key was a fallback match or an exact one. + Download(ctx context.Context, key, destPath string, fallback bool) (*TransferInfo, error) } func NewBlobStore(ctx context.Context, store, bucketURL string) (Blob, error) { diff --git a/internal/cache/store/file.go b/internal/cache/store/file.go index 6bcc085ec2..9abc281ad1 100644 --- a/internal/cache/store/file.go +++ b/internal/cache/store/file.go @@ -287,7 +287,7 @@ func (b *LocalFileBlob) Upload(ctx context.Context, srcPath, key string) (*Trans // // Returns TransferInfo with bytes transferred, transfer speed, and duration. // Returns an error if the cache key doesn't exist or file operations fail. -func (b *LocalFileBlob) Download(ctx context.Context, key, destPath string) (*TransferInfo, error) { +func (b *LocalFileBlob) Download(ctx context.Context, key, destPath string, fallback bool) (*TransferInfo, error) { _, span := trace.Start(ctx, "LocalFileBlob.Download") defer span.End() diff --git a/internal/cache/store/file_test.go b/internal/cache/store/file_test.go index fc79588c67..27ca47af76 100644 --- a/internal/cache/store/file_test.go +++ b/internal/cache/store/file_test.go @@ -238,7 +238,7 @@ func TestLocalFileBlobDownload(t *testing.T) { // Then download destFile := filepath.Join(destDir, "downloaded.txt") - info, err := blob.Download(ctx, key, destFile) + info, err := blob.Download(ctx, key, destFile, false) if err != nil { t.Fatalf("Download: %v", err) } @@ -333,7 +333,7 @@ func TestLocalFileBlobDownloadNonExistent(t *testing.T) { } destFile := filepath.Join(destDir, "nonexistent.txt") - _, err = blob.Download(ctx, "nonexistent/key", destFile) + _, err = blob.Download(ctx, "nonexistent/key", destFile, false) if err == nil { t.Fatal("expected error, got nil") } @@ -389,7 +389,7 @@ func TestLocalFileBlobDownloadInvalidKey(t *testing.T) { } destFile := filepath.Join(destDir, "test.txt") - _, err = blob.Download(ctx, "cache//invalid", destFile) + _, err = blob.Download(ctx, "cache//invalid", destFile, false) if err == nil { t.Fatal("expected error, got nil") } diff --git a/internal/cache/store/nsc.go b/internal/cache/store/nsc.go index f273f452f9..0f959f7df9 100644 --- a/internal/cache/store/nsc.go +++ b/internal/cache/store/nsc.go @@ -183,7 +183,7 @@ func (n *NscStore) Upload(ctx context.Context, filePath, key string) (*TransferI }, nil } -func (n *NscStore) Download(ctx context.Context, key, filePath string) (*TransferInfo, error) { +func (n *NscStore) Download(ctx context.Context, key, filePath string, fallback bool) (*TransferInfo, error) { _, span := trace.Start(ctx, "NscStore.Download") defer span.End() @@ -230,11 +230,13 @@ func (n *NscStore) Download(ctx context.Context, key, filePath string) (*Transfe attribute.String("nsc_key", key), ) - // Refresh the artifact's TTL on access so hot caches stay alive, mirroring - // the S3 store's self-CopyObject refresh. Unlike CopyObject this is cheap, - // so we refresh on every restore rather than gating it behind a minimum - // interval. - n.refreshExpiry(ctx, key) + // Refresh TTL on exact-match access to keep hot caches alive; cheap enough + // to run every time, unlike S3's gated CopyObject refresh. Skipped on + // fallback (mirrors the backend's TTL-bump guard) since the blob wasn't + // explicitly targeted. + if !fallback { + n.refreshExpiry(ctx, key) + } return &TransferInfo{ BytesTransferred: bytesTransferred, diff --git a/internal/cache/store/nsc_test.go b/internal/cache/store/nsc_test.go index 2f45da0c52..7c26aa423b 100644 --- a/internal/cache/store/nsc_test.go +++ b/internal/cache/store/nsc_test.go @@ -312,7 +312,7 @@ func TestNscStore_RefreshesTTLOnDownload(t *testing.T) { var calls [][]string store := &NscStore{namespace: "my-namespace", run: recordingRunner(&calls, nil)} - if _, err := store.Download(ctx, "key", dest); err != nil { + if _, err := store.Download(ctx, "key", dest, false); err != nil { t.Fatalf("Download: %v", err) } @@ -328,6 +328,27 @@ func TestNscStore_RefreshesTTLOnDownload(t *testing.T) { } } +// TestNscStore_SkipsRefreshOnFallbackDownload mirrors the backend's +// `unless entry_result.fallback_used?` TTL-bump guard: a fallback match must +// not refresh the blob's retention. +func TestNscStore_SkipsRefreshOnFallbackDownload(t *testing.T) { + ctx := t.Context() + dest := filepath.Join(t.TempDir(), "out.txt") + + var calls [][]string + store := &NscStore{namespace: "my-namespace", run: recordingRunner(&calls, nil)} + + if _, err := store.Download(ctx, "key", dest, true); err != nil { + t.Fatalf("Download: %v", err) + } + + for _, c := range calls { + if isCommand(c, "nsc", "artifact", "extend") { + t.Errorf("unexpected extend command on fallback download: %v", c) + } + } +} + func TestNscStore_UpdatesCLIWhenExtendUnsupported(t *testing.T) { ctx := t.Context() dest := filepath.Join(t.TempDir(), "out.txt") @@ -342,7 +363,7 @@ func TestNscStore_UpdatesCLIWhenExtendUnsupported(t *testing.T) { } store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - if _, err := store.Download(ctx, "key", dest); err != nil { + if _, err := store.Download(ctx, "key", dest, false); err != nil { t.Fatalf("Download: %v", err) } @@ -383,7 +404,7 @@ func TestNscStore_UpdatesCLIWhenExtendHelpLacksEnsureMinimum(t *testing.T) { } store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - if _, err := store.Download(ctx, "key", dest); err != nil { + if _, err := store.Download(ctx, "key", dest, false); err != nil { t.Fatalf("Download: %v", err) } @@ -417,7 +438,7 @@ func TestNscStore_DownloadSucceedsWhenRefreshFails(t *testing.T) { var calls [][]string store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - if _, err := store.Download(ctx, "key", dest); err != nil { + if _, err := store.Download(ctx, "key", dest, false); err != nil { t.Fatalf("Download should succeed despite a failed TTL refresh: %v", err) } } @@ -441,7 +462,7 @@ func TestNscStore_ValidationShortCircuits(t *testing.T) { if _, err := store.Upload(ctx, "invalid;path", "valid-key"); err == nil { t.Error("Upload with unsafe path: expected error, got nil") } - if _, err := store.Download(ctx, "invalid key with spaces", "dest.txt"); err == nil { + if _, err := store.Download(ctx, "invalid key with spaces", "dest.txt", false); err == nil { t.Error("Download with unsafe key: expected error, got nil") } if ran { @@ -513,7 +534,7 @@ func TestNscStore_Integration(t *testing.T) { // Test download downloadFile := filepath.Join(tmpDir, "test-download.txt") - transferInfo, err = store.Download(ctx, key, downloadFile) + transferInfo, err = store.Download(ctx, key, downloadFile, false) if err != nil { t.Fatalf("Download should succeed: %v", err) } @@ -546,7 +567,7 @@ func TestNscStore_DownloadNotFound(t *testing.T) { store := &NscStore{namespace: "ns", run: func(context.Context, string, ...string) (*CommandResult, error) { return &CommandResult{ExitCode: 1, Stderr: "Error: artifact not found"}, nil }} - _, err := store.Download(ctx, "valid-key", dest) + _, err := store.Download(ctx, "valid-key", dest, false) if !errors.Is(err, ErrBlobNotFound) { t.Fatalf("Download err = %v, want ErrBlobNotFound", err) } @@ -559,7 +580,7 @@ func TestNscStore_DownloadNotFound(t *testing.T) { Stderr: "Failed: the artifact has expired at 2026-07-14T02:06:24Z (request id: cbdorlqepas5e10ldvfvdpog40).", }, nil }} - _, err := store.Download(ctx, "valid-key", dest) + _, err := store.Download(ctx, "valid-key", dest, false) if !errors.Is(err, ErrBlobNotFound) { t.Fatalf("Download err = %v, want ErrBlobNotFound", err) } @@ -569,7 +590,7 @@ func TestNscStore_DownloadNotFound(t *testing.T) { store := &NscStore{namespace: "ns", run: func(context.Context, string, ...string) (*CommandResult, error) { return &CommandResult{ExitCode: 1, Stderr: "connection refused"}, nil }} - _, err := store.Download(ctx, "valid-key", dest) + _, err := store.Download(ctx, "valid-key", dest, false) if err == nil { t.Fatal("Download: expected error, got nil") } diff --git a/internal/cache/store/s3.go b/internal/cache/store/s3.go index c3e2041b23..0e5f7e2a5e 100644 --- a/internal/cache/store/s3.go +++ b/internal/cache/store/s3.go @@ -112,6 +112,12 @@ type objectDownloader interface { Download(ctx context.Context, w io.WriterAt, input *s3.GetObjectInput, options ...func(*manager.Downloader)) (int64, error) //nolint:staticcheck // SA1019: pending migration to transfermanager } +// objectCopier is the subset of *s3.Client used for the TTL-refresh self-copy, +// declared so the refresh decision can be tested with a fake. +type objectCopier interface { + CopyObject(ctx context.Context, input *s3.CopyObjectInput, opts ...func(*s3.Options)) (*s3.CopyObjectOutput, error) +} + // isPreconditionFailed returns true when an error is an S3 412 PreconditionFailed. // This happens when: // @@ -172,7 +178,7 @@ func downloadWithRetry(ctx context.Context, r *roko.Retrier, d objectDownloader, // S3Blob implements the Blob interface using AWS S3 type S3Blob struct { - client *s3.Client + client objectCopier uploader *manager.Uploader //nolint:staticcheck // SA1019: pending migration to transfermanager downloader *manager.Downloader //nolint:staticcheck // SA1019: pending migration to transfermanager bucketName string @@ -387,7 +393,7 @@ func (b *S3Blob) Upload(ctx context.Context, filePath, key string) (*TransferInf const restoreRefreshMinInterval = 12 * time.Hour // Download downloads a file from S3 using parallel range requests for large files -func (b *S3Blob) Download(ctx context.Context, key, destPath string) (*TransferInfo, error) { +func (b *S3Blob) Download(ctx context.Context, key, destPath string, fallback bool) (*TransferInfo, error) { ctx, span := trace.Start(ctx, "S3Blob.Download") defer span.End() @@ -458,18 +464,40 @@ func (b *S3Blob) Download(ctx context.Context, key, destPath string) (*TransferI attribute.Int("concurrency", b.downloadConcurrency), ) - // Extend an object's effective TTL by performing CopyObject on itself to - // refresh its LastModified timestamp. - // - // The CopySourceIfUnmodifiedSince precondition aborts the operation with an - // HTTP status code 412 if the object's LastModified timestamp falls within - // restoreRefreshMinInterval. - // - // This refresh is best-effort only: any failure (incl. objects exceeding - // S3's 5GB CopyObject limit) must not cause the overall restore operation to fail. - copySource := fmt.Sprintf("%s/%s", b.bucketName, fullKey) - _, err = b.client.CopyObject(ctx, &s3.CopyObjectInput{ - Bucket: aws.String(b.bucketName), + refreshObjectExpiry(ctx, b.client, b.bucketName, fullKey, fallback) + + return &TransferInfo{ + BytesTransferred: bytesWritten, + TransferSpeed: averageSpeed, + RequestID: "", // Download doesn't return a single request ID for parallel downloads + Duration: duration, + PartCount: actualPartCount, + Concurrency: b.downloadConcurrency, + }, nil +} + +// refreshObjectExpiry extends an object's effective TTL by performing +// CopyObject on itself to refresh its LastModified timestamp. +// +// Skipped on a fallback match — mirrors the backend's +// `unless entry_result.fallback_used?` TTL-bump guard: 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. +// +// The CopySourceIfUnmodifiedSince precondition aborts the operation with an +// HTTP status code 412 if the object's LastModified timestamp falls within +// restoreRefreshMinInterval. +// +// This refresh is best-effort only: any failure (incl. objects exceeding +// S3's 5GB CopyObject limit) must not cause the overall restore operation to fail. +func refreshObjectExpiry(ctx context.Context, copier objectCopier, bucket, fullKey string, fallback bool) { + if fallback { + return + } + + copySource := fmt.Sprintf("%s/%s", bucket, fullKey) + _, err := copier.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: aws.String(bucket), Key: aws.String(fullKey), CopySource: aws.String(copySource), MetadataDirective: "REPLACE", @@ -477,23 +505,14 @@ func (b *S3Blob) Download(ctx context.Context, key, destPath string) (*TransferI }) switch { case err == nil: - slog.Debug("refreshed object expiration", "key", fullKey, "bucket", b.bucketName) + slog.Debug("refreshed object expiration", "key", fullKey, "bucket", bucket) case isPreconditionFailed(err): slog.Debug("skipping cache TTL refresh, blob modified recently", - "key", fullKey, "bucket", b.bucketName) + "key", fullKey, "bucket", bucket) default: slog.Warn("failed to refresh object expiration, continuing (non-fatal)", - "key", fullKey, "bucket", b.bucketName, "error", err) + "key", fullKey, "bucket", bucket, "error", err) } - - return &TransferInfo{ - BytesTransferred: bytesWritten, - TransferSpeed: averageSpeed, - RequestID: "", // Download doesn't return a single request ID for parallel downloads - Duration: duration, - PartCount: actualPartCount, - Concurrency: b.downloadConcurrency, - }, nil } // getFullKey combines the prefix with the key diff --git a/internal/cache/store/s3_test.go b/internal/cache/store/s3_test.go index 1c91216601..5e8b4aa678 100644 --- a/internal/cache/store/s3_test.go +++ b/internal/cache/store/s3_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go-v2/aws" awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http" "github.com/aws/aws-sdk-go-v2/feature/s3/manager" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -582,3 +583,64 @@ func TestIsNotFound(t *testing.T) { }) } } + +// fakeCopier is a test double for objectCopier, recording the input of every +// CopyObject call and returning the configured result. +type fakeCopier struct { + calls []*s3.CopyObjectInput + err error +} + +func (f *fakeCopier) CopyObject(_ context.Context, input *s3.CopyObjectInput, _ ...func(*s3.Options)) (*s3.CopyObjectOutput, error) { + f.calls = append(f.calls, input) + if f.err != nil { + return nil, f.err + } + return &s3.CopyObjectOutput{}, nil +} + +func TestRefreshObjectExpiry(t *testing.T) { + t.Run("self-copies the object to refresh LastModified", func(t *testing.T) { + copier := &fakeCopier{} + + refreshObjectExpiry(t.Context(), copier, "my-bucket", "prefix/key", false) + + if len(copier.calls) != 1 { + t.Fatalf("CopyObject calls = %d, want 1", len(copier.calls)) + } + got := copier.calls[0] + if aws.ToString(got.Bucket) != "my-bucket" || aws.ToString(got.Key) != "prefix/key" { + t.Errorf("CopyObjectInput bucket/key = %q/%q, want %q/%q", aws.ToString(got.Bucket), aws.ToString(got.Key), "my-bucket", "prefix/key") + } + if aws.ToString(got.CopySource) != "my-bucket/prefix/key" { + t.Errorf("CopySource = %q, want %q", aws.ToString(got.CopySource), "my-bucket/prefix/key") + } + if got.MetadataDirective != types.MetadataDirectiveReplace { + t.Errorf("MetadataDirective = %v, want REPLACE", got.MetadataDirective) + } + }) + + // Mirrors the backend's `unless entry_result.fallback_used?` TTL-bump + // guard: a fallback match must not refresh the blob's retention. + t.Run("skips the refresh on a fallback match", func(t *testing.T) { + copier := &fakeCopier{} + + refreshObjectExpiry(t.Context(), copier, "my-bucket", "key", true) + + if len(copier.calls) != 0 { + t.Errorf("CopyObject calls = %d, want 0 for a fallback match", len(copier.calls)) + } + }) + + t.Run("swallows a precondition-failed error (recently refreshed)", func(t *testing.T) { + copier := &fakeCopier{err: responseErrorWithStatus(http.StatusPreconditionFailed)} + + refreshObjectExpiry(t.Context(), copier, "my-bucket", "key", false) // must not panic + }) + + t.Run("swallows any other error (best-effort)", func(t *testing.T) { + copier := &fakeCopier{err: errors.New("boom")} + + refreshObjectExpiry(t.Context(), copier, "my-bucket", "key", false) // must not panic + }) +} From 300c207489f2d90ead0dc5fb2681e80513ed977d Mon Sep 17 00:00:00 2001 From: Sneha Date: Thu, 3 Sep 2026 13:46:48 +1000 Subject: [PATCH 2/2] Move retention refresh out of Blob.Download 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. --- internal/cache/restore.go | 18 +++++++- internal/cache/restore_test.go | 61 +++++++++++++++++++++++++++ internal/cache/store/blob.go | 13 ++++-- internal/cache/store/file.go | 2 +- internal/cache/store/file_test.go | 6 +-- internal/cache/store/nsc.go | 25 +++++------ internal/cache/store/nsc_test.go | 70 ++++++++++++++----------------- internal/cache/store/s3.go | 24 +++++------ internal/cache/store/s3_test.go | 34 ++++++++------- 9 files changed, 163 insertions(+), 90 deletions(-) diff --git a/internal/cache/restore.go b/internal/cache/restore.go index 0e24ac1d92..cee218cb9e 100644 --- a/internal/cache/restore.go +++ b/internal/cache/restore.go @@ -440,7 +440,7 @@ func (c *client) downloadCache(ctx context.Context, retrieveResp api.CacheEntryR archiveFile = filepath.Join(tmpDir, storeObjectName) // Download archive - transferInfo, err = blobStore.Download(ctx, storeObjectName, archiveFile, retrieveResp.Fallback) + transferInfo, err = blobStore.Download(ctx, storeObjectName, archiveFile) if err != nil { // Clean up temporary directory on failure _ = os.RemoveAll(tmpDir) @@ -467,11 +467,27 @@ func (c *client) downloadCache(ctx context.Context, retrieveResp api.CacheEntryR return "", "", nil, err } + // Extend the blob's retention now that it's confirmed good. + maybeRefreshRetention(ctx, blobStore, retrieveResp.Fallback, storeObjectName) + span.SetStatus(codes.Ok, "download completed") return tmpDir, archiveFile, transferInfo, nil } +// maybeRefreshRetention extends key's retention via blobStore, unless +// fallback is true — 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 (mirrors the backend's +// `unless entry_result.fallback_used?` TTL-bump guard). +func maybeRefreshRetention(ctx context.Context, blobStore store.Blob, fallback bool, key string) { + refresher, ok := blobStore.(store.RetentionRefresher) + if !ok || fallback { + return + } + refresher.RefreshRetention(ctx, key) +} + // extractCache extracts files from a cache archive func (c *client) extractCache(ctx context.Context, archiveFile string, archiveSize int64, paths []string) (*archive.ArchiveInfo, error) { tracer := otel.Tracer("github.com/buildkite/agent/v4/internal/cache") diff --git a/internal/cache/restore_test.go b/internal/cache/restore_test.go index 7ad16de02c..00fdc731de 100644 --- a/internal/cache/restore_test.go +++ b/internal/cache/restore_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/buildkite/agent/v4/api" + "github.com/buildkite/agent/v4/internal/cache/store" ) func TestInvalidateStaleEntry_EchoesScopesFromRetrieve(t *testing.T) { @@ -342,3 +343,63 @@ func TestCleanPathWindowsUNCShareRoot(t *testing.T) { t.Errorf("error %q should contain %q", err.Error(), "refusing to remove") } } + +// fakeRefreshingBlob is a minimal store.Blob that also implements +// store.RetentionRefresher, mirroring NscStore/S3Blob. +type fakeRefreshingBlob struct { + refreshCalls []string +} + +func (f *fakeRefreshingBlob) Upload(_ context.Context, _, _ string) (*store.TransferInfo, error) { + return nil, nil +} + +func (f *fakeRefreshingBlob) Download(_ context.Context, _, _ string) (*store.TransferInfo, error) { + return nil, nil +} + +func (f *fakeRefreshingBlob) RefreshRetention(_ context.Context, key string) { + f.refreshCalls = append(f.refreshCalls, key) +} + +// fakeNonRefreshingBlob is a store.Blob with no RefreshRetention method, +// mirroring LocalFileBlob (no retention concept to refresh). +type fakeNonRefreshingBlob struct{} + +func (f *fakeNonRefreshingBlob) Upload(_ context.Context, _, _ string) (*store.TransferInfo, error) { + return nil, nil +} + +func (f *fakeNonRefreshingBlob) Download(_ context.Context, _, _ string) (*store.TransferInfo, error) { + return nil, nil +} + +func TestMaybeRefreshRetention(t *testing.T) { + t.Run("refreshes on an exact match", func(t *testing.T) { + blob := &fakeRefreshingBlob{} + + maybeRefreshRetention(t.Context(), blob, false, "key") + + if len(blob.refreshCalls) != 1 { + t.Errorf("refresh calls = %d, want 1", len(blob.refreshCalls)) + } + }) + + // Mirrors the backend's `unless entry_result.fallback_used?` TTL-bump + // guard: a fallback match must not refresh the blob's retention. + t.Run("skips the refresh on a fallback match", func(t *testing.T) { + blob := &fakeRefreshingBlob{} + + maybeRefreshRetention(t.Context(), blob, true, "key") + + if len(blob.refreshCalls) != 0 { + t.Errorf("refresh calls = %d, want 0 for a fallback match", len(blob.refreshCalls)) + } + }) + + t.Run("does nothing for a store with no retention concept", func(t *testing.T) { + blob := &fakeNonRefreshingBlob{} + + maybeRefreshRetention(t.Context(), blob, false, "key") // must not panic + }) +} diff --git a/internal/cache/store/blob.go b/internal/cache/store/blob.go index 8c58179c26..24cbed8100 100644 --- a/internal/cache/store/blob.go +++ b/internal/cache/store/blob.go @@ -16,9 +16,16 @@ type Blob interface { // Upload uploads a file to blob storage Upload(ctx context.Context, filePath, key string) (*TransferInfo, error) - // Download downloads a file from blob storage. fallback reports whether the - // restore that resolved this key was a fallback match or an exact one. - Download(ctx context.Context, key, destPath string, fallback bool) (*TransferInfo, error) + // Download downloads a file from blob storage + Download(ctx context.Context, key, destPath string) (*TransferInfo, error) +} + +// RetentionRefresher is implemented by Blob stores that support extending a +// blob's effective retention/TTL on access (NscStore, S3Blob). Optional +// because not every store has a retention concept to refresh (LocalFileBlob +// does not implement it). +type RetentionRefresher interface { + RefreshRetention(ctx context.Context, key string) } func NewBlobStore(ctx context.Context, store, bucketURL string) (Blob, error) { diff --git a/internal/cache/store/file.go b/internal/cache/store/file.go index 9abc281ad1..6bcc085ec2 100644 --- a/internal/cache/store/file.go +++ b/internal/cache/store/file.go @@ -287,7 +287,7 @@ func (b *LocalFileBlob) Upload(ctx context.Context, srcPath, key string) (*Trans // // Returns TransferInfo with bytes transferred, transfer speed, and duration. // Returns an error if the cache key doesn't exist or file operations fail. -func (b *LocalFileBlob) Download(ctx context.Context, key, destPath string, fallback bool) (*TransferInfo, error) { +func (b *LocalFileBlob) Download(ctx context.Context, key, destPath string) (*TransferInfo, error) { _, span := trace.Start(ctx, "LocalFileBlob.Download") defer span.End() diff --git a/internal/cache/store/file_test.go b/internal/cache/store/file_test.go index 27ca47af76..fc79588c67 100644 --- a/internal/cache/store/file_test.go +++ b/internal/cache/store/file_test.go @@ -238,7 +238,7 @@ func TestLocalFileBlobDownload(t *testing.T) { // Then download destFile := filepath.Join(destDir, "downloaded.txt") - info, err := blob.Download(ctx, key, destFile, false) + info, err := blob.Download(ctx, key, destFile) if err != nil { t.Fatalf("Download: %v", err) } @@ -333,7 +333,7 @@ func TestLocalFileBlobDownloadNonExistent(t *testing.T) { } destFile := filepath.Join(destDir, "nonexistent.txt") - _, err = blob.Download(ctx, "nonexistent/key", destFile, false) + _, err = blob.Download(ctx, "nonexistent/key", destFile) if err == nil { t.Fatal("expected error, got nil") } @@ -389,7 +389,7 @@ func TestLocalFileBlobDownloadInvalidKey(t *testing.T) { } destFile := filepath.Join(destDir, "test.txt") - _, err = blob.Download(ctx, "cache//invalid", destFile, false) + _, err = blob.Download(ctx, "cache//invalid", destFile) if err == nil { t.Fatal("expected error, got nil") } diff --git a/internal/cache/store/nsc.go b/internal/cache/store/nsc.go index 0f959f7df9..7524a56495 100644 --- a/internal/cache/store/nsc.go +++ b/internal/cache/store/nsc.go @@ -183,7 +183,7 @@ func (n *NscStore) Upload(ctx context.Context, filePath, key string) (*TransferI }, nil } -func (n *NscStore) Download(ctx context.Context, key, filePath string, fallback bool) (*TransferInfo, error) { +func (n *NscStore) Download(ctx context.Context, key, filePath string) (*TransferInfo, error) { _, span := trace.Start(ctx, "NscStore.Download") defer span.End() @@ -230,14 +230,6 @@ func (n *NscStore) Download(ctx context.Context, key, filePath string, fallback attribute.String("nsc_key", key), ) - // Refresh TTL on exact-match access to keep hot caches alive; cheap enough - // to run every time, unlike S3's gated CopyObject refresh. Skipped on - // fallback (mirrors the backend's TTL-bump guard) since the blob wasn't - // explicitly targeted. - if !fallback { - n.refreshExpiry(ctx, key) - } - return &TransferInfo{ BytesTransferred: bytesTransferred, TransferSpeed: averageSpeed, @@ -246,14 +238,17 @@ func (n *NscStore) Download(ctx context.Context, key, filePath string, fallback }, nil } -// refreshExpiry pushes the artifact's expiry out to at least nscDefaultExpiry -// from now via `nsc artifact extend --ensure_minimum`. Using --ensure_minimum -// (rather than the additive --by) makes the refresh idempotent, so calling it -// on every restore keeps a hot cache alive without growing its expiry unbounded. +// RefreshRetention pushes the artifact's expiry out to at least +// nscDefaultExpiry from now via `nsc artifact extend --ensure_minimum`. Using +// --ensure_minimum (rather than the additive --by) makes the refresh +// idempotent, so calling it on every restore keeps a hot cache alive without +// growing its expiry unbounded. // // This is best-effort: any failure is logged and swallowed so a restore never -// fails because its TTL could not be refreshed. -func (n *NscStore) refreshExpiry(ctx context.Context, key string) { +// fails because its TTL could not be refreshed. Whether to call this at all +// (e.g. skipping it on a fallback match) is the restore flow's decision, not +// this store's — see store.RetentionRefresher. +func (n *NscStore) RefreshRetention(ctx context.Context, key string) { if !n.extendSupported(ctx) { // `nsc artifact extend` is still being rolled out by Namespace. Update // the nsc CLI as a contingency so the command becomes available. diff --git a/internal/cache/store/nsc_test.go b/internal/cache/store/nsc_test.go index 7c26aa423b..368bb3a53c 100644 --- a/internal/cache/store/nsc_test.go +++ b/internal/cache/store/nsc_test.go @@ -305,53 +305,50 @@ func recordingRunner(calls *[][]string, respond func(args []string) (*CommandRes } } -func TestNscStore_RefreshesTTLOnDownload(t *testing.T) { +// TestNscStore_Download_DoesNotRefreshRetention guards against Download +// reaching back into RefreshRetention itself — that decision belongs to the +// restore flow (which knows about fallback), not this store. See +// store.RetentionRefresher. +func TestNscStore_Download_DoesNotRefreshRetention(t *testing.T) { ctx := t.Context() dest := filepath.Join(t.TempDir(), "out.txt") var calls [][]string store := &NscStore{namespace: "my-namespace", run: recordingRunner(&calls, nil)} - if _, err := store.Download(ctx, "key", dest, false); err != nil { + if _, err := store.Download(ctx, "key", dest); err != nil { t.Fatalf("Download: %v", err) } - wantExtend := []string{"nsc", "artifact", "extend", "key", "--ensure_minimum", "72h", "--namespace", "my-namespace"} - var gotExtend []string for _, c := range calls { - if isCommand(c, "nsc", "artifact", "extend") && !isCommand(c, "nsc", "artifact", "extend", "--help") { - gotExtend = c + if isCommand(c, "nsc", "artifact", "extend") { + t.Errorf("unexpected extend command from Download: %v", c) } } - if diff := cmp.Diff(wantExtend, gotExtend); diff != "" { - t.Errorf("extend args mismatch (-want +got):\n%s", diff) - } } -// TestNscStore_SkipsRefreshOnFallbackDownload mirrors the backend's -// `unless entry_result.fallback_used?` TTL-bump guard: a fallback match must -// not refresh the blob's retention. -func TestNscStore_SkipsRefreshOnFallbackDownload(t *testing.T) { +func TestNscStore_RefreshRetention(t *testing.T) { ctx := t.Context() - dest := filepath.Join(t.TempDir(), "out.txt") var calls [][]string store := &NscStore{namespace: "my-namespace", run: recordingRunner(&calls, nil)} - if _, err := store.Download(ctx, "key", dest, true); err != nil { - t.Fatalf("Download: %v", err) - } + store.RefreshRetention(ctx, "key") + wantExtend := []string{"nsc", "artifact", "extend", "key", "--ensure_minimum", "72h", "--namespace", "my-namespace"} + var gotExtend []string for _, c := range calls { - if isCommand(c, "nsc", "artifact", "extend") { - t.Errorf("unexpected extend command on fallback download: %v", c) + if isCommand(c, "nsc", "artifact", "extend") && !isCommand(c, "nsc", "artifact", "extend", "--help") { + gotExtend = c } } + if diff := cmp.Diff(wantExtend, gotExtend); diff != "" { + t.Errorf("extend args mismatch (-want +got):\n%s", diff) + } } -func TestNscStore_UpdatesCLIWhenExtendUnsupported(t *testing.T) { +func TestNscStore_RefreshRetention_UpdatesCLIWhenExtendUnsupported(t *testing.T) { ctx := t.Context() - dest := filepath.Join(t.TempDir(), "out.txt") var calls [][]string respond := func(args []string) (*CommandResult, error) { @@ -363,9 +360,7 @@ func TestNscStore_UpdatesCLIWhenExtendUnsupported(t *testing.T) { } store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - if _, err := store.Download(ctx, "key", dest, false); err != nil { - t.Fatalf("Download: %v", err) - } + store.RefreshRetention(ctx, "key") var updated, extended bool for _, c := range calls { @@ -384,9 +379,8 @@ func TestNscStore_UpdatesCLIWhenExtendUnsupported(t *testing.T) { } } -func TestNscStore_UpdatesCLIWhenExtendHelpLacksEnsureMinimum(t *testing.T) { +func TestNscStore_RefreshRetention_UpdatesCLIWhenExtendHelpLacksEnsureMinimum(t *testing.T) { ctx := t.Context() - dest := filepath.Join(t.TempDir(), "out.txt") var calls [][]string respond := func(args []string) (*CommandResult, error) { @@ -404,9 +398,7 @@ func TestNscStore_UpdatesCLIWhenExtendHelpLacksEnsureMinimum(t *testing.T) { } store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - if _, err := store.Download(ctx, "key", dest, false); err != nil { - t.Fatalf("Download: %v", err) - } + store.RefreshRetention(ctx, "key") var updated, extended bool for _, c := range calls { @@ -425,9 +417,11 @@ func TestNscStore_UpdatesCLIWhenExtendHelpLacksEnsureMinimum(t *testing.T) { } } -func TestNscStore_DownloadSucceedsWhenRefreshFails(t *testing.T) { +// TestNscStore_RefreshRetention_SwallowsFailure guards the best-effort +// contract: a failed refresh must not panic or otherwise propagate, since +// RefreshRetention has no error return for a caller to check. +func TestNscStore_RefreshRetention_SwallowsFailure(t *testing.T) { ctx := t.Context() - dest := filepath.Join(t.TempDir(), "out.txt") respond := func(args []string) (*CommandResult, error) { if isCommand(args, "nsc", "artifact", "extend", "key") { @@ -438,9 +432,7 @@ func TestNscStore_DownloadSucceedsWhenRefreshFails(t *testing.T) { var calls [][]string store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - if _, err := store.Download(ctx, "key", dest, false); err != nil { - t.Fatalf("Download should succeed despite a failed TTL refresh: %v", err) - } + store.RefreshRetention(ctx, "key") // must not panic } func TestNewNscStore_RequiresNamespace(t *testing.T) { @@ -462,7 +454,7 @@ func TestNscStore_ValidationShortCircuits(t *testing.T) { if _, err := store.Upload(ctx, "invalid;path", "valid-key"); err == nil { t.Error("Upload with unsafe path: expected error, got nil") } - if _, err := store.Download(ctx, "invalid key with spaces", "dest.txt", false); err == nil { + if _, err := store.Download(ctx, "invalid key with spaces", "dest.txt"); err == nil { t.Error("Download with unsafe key: expected error, got nil") } if ran { @@ -534,7 +526,7 @@ func TestNscStore_Integration(t *testing.T) { // Test download downloadFile := filepath.Join(tmpDir, "test-download.txt") - transferInfo, err = store.Download(ctx, key, downloadFile, false) + transferInfo, err = store.Download(ctx, key, downloadFile) if err != nil { t.Fatalf("Download should succeed: %v", err) } @@ -567,7 +559,7 @@ func TestNscStore_DownloadNotFound(t *testing.T) { store := &NscStore{namespace: "ns", run: func(context.Context, string, ...string) (*CommandResult, error) { return &CommandResult{ExitCode: 1, Stderr: "Error: artifact not found"}, nil }} - _, err := store.Download(ctx, "valid-key", dest, false) + _, err := store.Download(ctx, "valid-key", dest) if !errors.Is(err, ErrBlobNotFound) { t.Fatalf("Download err = %v, want ErrBlobNotFound", err) } @@ -580,7 +572,7 @@ func TestNscStore_DownloadNotFound(t *testing.T) { Stderr: "Failed: the artifact has expired at 2026-07-14T02:06:24Z (request id: cbdorlqepas5e10ldvfvdpog40).", }, nil }} - _, err := store.Download(ctx, "valid-key", dest, false) + _, err := store.Download(ctx, "valid-key", dest) if !errors.Is(err, ErrBlobNotFound) { t.Fatalf("Download err = %v, want ErrBlobNotFound", err) } @@ -590,7 +582,7 @@ func TestNscStore_DownloadNotFound(t *testing.T) { store := &NscStore{namespace: "ns", run: func(context.Context, string, ...string) (*CommandResult, error) { return &CommandResult{ExitCode: 1, Stderr: "connection refused"}, nil }} - _, err := store.Download(ctx, "valid-key", dest, false) + _, err := store.Download(ctx, "valid-key", dest) if err == nil { t.Fatal("Download: expected error, got nil") } diff --git a/internal/cache/store/s3.go b/internal/cache/store/s3.go index 0e5f7e2a5e..19d8196716 100644 --- a/internal/cache/store/s3.go +++ b/internal/cache/store/s3.go @@ -393,7 +393,7 @@ func (b *S3Blob) Upload(ctx context.Context, filePath, key string) (*TransferInf const restoreRefreshMinInterval = 12 * time.Hour // Download downloads a file from S3 using parallel range requests for large files -func (b *S3Blob) Download(ctx context.Context, key, destPath string, fallback bool) (*TransferInfo, error) { +func (b *S3Blob) Download(ctx context.Context, key, destPath string) (*TransferInfo, error) { ctx, span := trace.Start(ctx, "S3Blob.Download") defer span.End() @@ -464,8 +464,6 @@ func (b *S3Blob) Download(ctx context.Context, key, destPath string, fallback bo attribute.Int("concurrency", b.downloadConcurrency), ) - refreshObjectExpiry(ctx, b.client, b.bucketName, fullKey, fallback) - return &TransferInfo{ BytesTransferred: bytesWritten, TransferSpeed: averageSpeed, @@ -476,13 +474,12 @@ func (b *S3Blob) Download(ctx context.Context, key, destPath string, fallback bo }, nil } -// refreshObjectExpiry extends an object's effective TTL by performing -// CopyObject on itself to refresh its LastModified timestamp. +// RefreshRetention extends an object's effective TTL by performing CopyObject +// on itself to refresh its LastModified timestamp — S3 has no native +// touch/extend-TTL API, and lifecycle expiration rules key off LastModified. // -// Skipped on a fallback match — mirrors the backend's -// `unless entry_result.fallback_used?` TTL-bump guard: 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. +// Whether to call this at all (e.g. skipping it on a fallback match) is the +// restore flow's decision, not this store's — see store.RetentionRefresher. // // The CopySourceIfUnmodifiedSince precondition aborts the operation with an // HTTP status code 412 if the object's LastModified timestamp falls within @@ -490,11 +487,12 @@ func (b *S3Blob) Download(ctx context.Context, key, destPath string, fallback bo // // This refresh is best-effort only: any failure (incl. objects exceeding // S3's 5GB CopyObject limit) must not cause the overall restore operation to fail. -func refreshObjectExpiry(ctx context.Context, copier objectCopier, bucket, fullKey string, fallback bool) { - if fallback { - return - } +func (b *S3Blob) RefreshRetention(ctx context.Context, key string) { + fullKey := b.getFullKey(key) + refreshObjectExpiry(ctx, b.client, b.bucketName, fullKey) +} +func refreshObjectExpiry(ctx context.Context, copier objectCopier, bucket, fullKey string) { copySource := fmt.Sprintf("%s/%s", bucket, fullKey) _, err := copier.CopyObject(ctx, &s3.CopyObjectInput{ Bucket: aws.String(bucket), diff --git a/internal/cache/store/s3_test.go b/internal/cache/store/s3_test.go index 5e8b4aa678..bdd66f159c 100644 --- a/internal/cache/store/s3_test.go +++ b/internal/cache/store/s3_test.go @@ -603,7 +603,7 @@ func TestRefreshObjectExpiry(t *testing.T) { t.Run("self-copies the object to refresh LastModified", func(t *testing.T) { copier := &fakeCopier{} - refreshObjectExpiry(t.Context(), copier, "my-bucket", "prefix/key", false) + refreshObjectExpiry(t.Context(), copier, "my-bucket", "prefix/key") if len(copier.calls) != 1 { t.Fatalf("CopyObject calls = %d, want 1", len(copier.calls)) @@ -620,27 +620,31 @@ func TestRefreshObjectExpiry(t *testing.T) { } }) - // Mirrors the backend's `unless entry_result.fallback_used?` TTL-bump - // guard: a fallback match must not refresh the blob's retention. - t.Run("skips the refresh on a fallback match", func(t *testing.T) { - copier := &fakeCopier{} - - refreshObjectExpiry(t.Context(), copier, "my-bucket", "key", true) - - if len(copier.calls) != 0 { - t.Errorf("CopyObject calls = %d, want 0 for a fallback match", len(copier.calls)) - } - }) - t.Run("swallows a precondition-failed error (recently refreshed)", func(t *testing.T) { copier := &fakeCopier{err: responseErrorWithStatus(http.StatusPreconditionFailed)} - refreshObjectExpiry(t.Context(), copier, "my-bucket", "key", false) // must not panic + refreshObjectExpiry(t.Context(), copier, "my-bucket", "key") // must not panic }) t.Run("swallows any other error (best-effort)", func(t *testing.T) { copier := &fakeCopier{err: errors.New("boom")} - refreshObjectExpiry(t.Context(), copier, "my-bucket", "key", false) // must not panic + refreshObjectExpiry(t.Context(), copier, "my-bucket", "key") // must not panic }) } + +// TestS3Blob_RefreshRetention covers the method wrapper — that it resolves +// the prefixed full key and delegates to refreshObjectExpiry. +func TestS3Blob_RefreshRetention(t *testing.T) { + copier := &fakeCopier{} + b := &S3Blob{client: copier, bucketName: "my-bucket", prefix: "prefix"} + + b.RefreshRetention(t.Context(), "key") + + if len(copier.calls) != 1 { + t.Fatalf("CopyObject calls = %d, want 1", len(copier.calls)) + } + if got := aws.ToString(copier.calls[0].Key); got != "prefix/key" { + t.Errorf("CopyObjectInput.Key = %q, want %q", got, "prefix/key") + } +}