Skip blob retention refresh on a fallback restore

internal/cache/restore.go

@@ -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")

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
+	})
+}

internal/cache/store/blob.go

@@ -20,6 +20,14 @@ type Blob interface {
 	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) {
 	switch store {
 	case AgentManaged:

internal/cache/store/nsc.go

@@ -230,12 +230,6 @@ 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)
-
 	return &TransferInfo{
 		BytesTransferred: bytesTransferred,
 		TransferSpeed:    averageSpeed,
@@ -244,14 +238,17 @@ func (n *NscStore) Download(ctx context.Context, key, filePath string) (*Transfe
 	}, 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.

internal/cache/store/nsc_test.go

@@ -305,7 +305,11 @@ 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")
 
@@ -316,6 +320,21 @@ func TestNscStore_RefreshesTTLOnDownload(t *testing.T) {
 		t.Fatalf("Download: %v", err)
 	}
 
+	for _, c := range calls {
+		if isCommand(c, "nsc", "artifact", "extend") {
+			t.Errorf("unexpected extend command from Download: %v", c)
+		}
+	}
+}
+
+func TestNscStore_RefreshRetention(t *testing.T) {
+	ctx := t.Context()
+
+	var calls [][]string
+	store := &NscStore{namespace: "my-namespace", run: recordingRunner(&calls, nil)}
+
+	store.RefreshRetention(ctx, "key")
+
 	wantExtend := []string{"nsc", "artifact", "extend", "key", "--ensure_minimum", "72h", "--namespace", "my-namespace"}
 	var gotExtend []string
 	for _, c := range calls {
@@ -328,9 +347,8 @@ func TestNscStore_RefreshesTTLOnDownload(t *testing.T) {
 	}
 }
 
-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) {
@@ -342,9 +360,7 @@ func TestNscStore_UpdatesCLIWhenExtendUnsupported(t *testing.T) {
 	}
 	store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)}
 
-	if _, err := store.Download(ctx, "key", dest); err != nil {
-		t.Fatalf("Download: %v", err)
-	}
+	store.RefreshRetention(ctx, "key")
 
 	var updated, extended bool
 	for _, c := range calls {
@@ -363,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) {
@@ -383,9 +398,7 @@ func TestNscStore_UpdatesCLIWhenExtendHelpLacksEnsureMinimum(t *testing.T) {
 	}
 	store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)}
 
-	if _, err := store.Download(ctx, "key", dest); err != nil {
-		t.Fatalf("Download: %v", err)
-	}
+	store.RefreshRetention(ctx, "key")
 
 	var updated, extended bool
 	for _, c := range calls {
@@ -404,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") {
@@ -417,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); 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) {

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
@@ -458,42 +464,53 @@ 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),
+	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
+}
+
+// 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.
+//
+// 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
+// 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 (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),
 		Key:                         aws.String(fullKey),
 		CopySource:                  aws.String(copySource),
 		MetadataDirective:           "REPLACE",
 		CopySourceIfUnmodifiedSince: aws.Time(time.Now().Add(-restoreRefreshMinInterval)),
 	})
 	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