diff --git a/api/cache.go b/api/cache.go index 1288de74e1..ef818ea759 100644 --- a/api/cache.go +++ b/api/cache.go @@ -69,6 +69,9 @@ type CacheEntryCreateResp struct { Multipart bool `json:"multipart"` UploadInstructions []string `json:"upload_instructions"` Message string `json:"message"` + // RetentionDays is how long the backing blob should be kept, matching the + // registry entry's TTL. + RetentionDays int64 `json:"retention_days"` } // CacheEntryRetrieveReq is the request body for retrieving a cache entry. @@ -83,6 +86,7 @@ type CacheEntryRetrieveResp struct { CacheKey []CacheKeyPart `json:"cache_key"` Blobs []CacheBlob `json:"blobs"` ExpiresAt time.Time `json:"expires_at"` + RetentionDays int64 `json:"retention_days"` Store string `json:"store"` Fallback bool `json:"fallback"` Multipart bool `json:"multipart"` diff --git a/internal/cache/restore.go b/internal/cache/restore.go index 9fc10e6f52..d9e9f2e430 100644 --- a/internal/cache/restore.go +++ b/internal/cache/restore.go @@ -537,7 +537,7 @@ func (c *client) downloadCache(ctx context.Context, retrieveResp api.CacheEntryR } // Extend the blob's retention now that it's confirmed good. - maybeRefreshRetention(ctx, blobStore, retrieveResp.Fallback, storeObjectName) + maybeRefreshRetention(ctx, blobStore, retrieveResp.Fallback, storeObjectName, time.Duration(retrieveResp.RetentionDays)*24*time.Hour) span.SetStatus(codes.Ok, "download completed") @@ -549,12 +549,12 @@ func (c *client) downloadCache(ctx context.Context, retrieveResp api.CacheEntryR // 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) { +func maybeRefreshRetention(ctx context.Context, blobStore store.Blob, fallback bool, key string, retention time.Duration) { refresher, ok := blobStore.(store.RetentionRefresher) if !ok || fallback { return } - refresher.RefreshRetention(ctx, key) + refresher.RefreshRetention(ctx, key, retention) } // extractCache extracts files from a cache archive diff --git a/internal/cache/restore_test.go b/internal/cache/restore_test.go index 8090e5cfba..336c2e21fc 100644 --- a/internal/cache/restore_test.go +++ b/internal/cache/restore_test.go @@ -448,10 +448,11 @@ func TestCleanPathWindowsUNCShareRoot(t *testing.T) { // fakeRefreshingBlob is a minimal store.Blob that also implements // store.RetentionRefresher, mirroring NscStore/S3Blob. type fakeRefreshingBlob struct { - refreshCalls []string + refreshCalls []string + refreshRetentions []time.Duration } -func (f *fakeRefreshingBlob) Upload(_ context.Context, _, _ string) (*store.TransferInfo, error) { +func (f *fakeRefreshingBlob) Upload(_ context.Context, _, _ string, _ time.Duration) (*store.TransferInfo, error) { return nil, nil } @@ -459,15 +460,16 @@ func (f *fakeRefreshingBlob) Download(_ context.Context, _, _ string) (*store.Tr return nil, nil } -func (f *fakeRefreshingBlob) RefreshRetention(_ context.Context, key string) { +func (f *fakeRefreshingBlob) RefreshRetention(_ context.Context, key string, retention time.Duration) { f.refreshCalls = append(f.refreshCalls, key) + f.refreshRetentions = append(f.refreshRetentions, retention) } // 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) { +func (f *fakeNonRefreshingBlob) Upload(_ context.Context, _, _ string, _ time.Duration) (*store.TransferInfo, error) { return nil, nil } @@ -476,14 +478,17 @@ func (f *fakeNonRefreshingBlob) Download(_ context.Context, _, _ string) (*store } func TestMaybeRefreshRetention(t *testing.T) { - t.Run("refreshes on an exact match", func(t *testing.T) { + t.Run("refreshes on an exact match with the given retention", func(t *testing.T) { blob := &fakeRefreshingBlob{} - maybeRefreshRetention(t.Context(), blob, false, "key") + maybeRefreshRetention(t.Context(), blob, false, "key", 48*time.Hour) if len(blob.refreshCalls) != 1 { t.Errorf("refresh calls = %d, want 1", len(blob.refreshCalls)) } + if len(blob.refreshRetentions) != 1 || blob.refreshRetentions[0] != 48*time.Hour { + t.Errorf("refresh retentions = %v, want [48h]", blob.refreshRetentions) + } }) // Mirrors the backend's `unless entry_result.fallback_used?` TTL-bump @@ -491,7 +496,7 @@ func TestMaybeRefreshRetention(t *testing.T) { t.Run("skips the refresh on a fallback match", func(t *testing.T) { blob := &fakeRefreshingBlob{} - maybeRefreshRetention(t.Context(), blob, true, "key") + maybeRefreshRetention(t.Context(), blob, true, "key", 48*time.Hour) if len(blob.refreshCalls) != 0 { t.Errorf("refresh calls = %d, want 0 for a fallback match", len(blob.refreshCalls)) @@ -501,6 +506,6 @@ func TestMaybeRefreshRetention(t *testing.T) { 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 + maybeRefreshRetention(t.Context(), blob, false, "key", 48*time.Hour) // must not panic }) } diff --git a/internal/cache/save.go b/internal/cache/save.go index 3633f49588..c35e3208f7 100644 --- a/internal/cache/save.go +++ b/internal/cache/save.go @@ -275,7 +275,7 @@ func (c *client) Save(ctx context.Context, cacheID string) (SaveResult, error) { return result, fmt.Errorf("failed to create blob store: %w", err) } - transferInfo, err := blobStore.Upload(ctx, archiveInfo.ArchivePath, storeObjectName) + transferInfo, err := blobStore.Upload(ctx, archiveInfo.ArchivePath, storeObjectName, time.Duration(createResp.RetentionDays)*24*time.Hour) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, "failed to upload cache") diff --git a/internal/cache/store/blob.go b/internal/cache/store/blob.go index 24cbed8100..6b455c434b 100644 --- a/internal/cache/store/blob.go +++ b/internal/cache/store/blob.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "strings" + "time" ) // ErrBlobNotFound is returned by a Blob's Download when the requested object @@ -13,8 +14,9 @@ var ErrBlobNotFound = errors.New("blob not found") // Blob interface defines the operations for blob storage type Blob interface { - // Upload uploads a file to blob storage - Upload(ctx context.Context, filePath, key string) (*TransferInfo, error) + // Upload uploads a file to blob storage. retention is how long the blob + // should be kept. + Upload(ctx context.Context, filePath, key string, retention time.Duration) (*TransferInfo, error) // Download downloads a file from blob storage Download(ctx context.Context, key, destPath string) (*TransferInfo, error) @@ -25,7 +27,7 @@ type Blob interface { // because not every store has a retention concept to refresh (LocalFileBlob // does not implement it). type RetentionRefresher interface { - RefreshRetention(ctx context.Context, key string) + RefreshRetention(ctx context.Context, key string, retention time.Duration) } 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..28211ecb7e 100644 --- a/internal/cache/store/file.go +++ b/internal/cache/store/file.go @@ -127,7 +127,7 @@ func NewLocalFileBlob(ctx context.Context, fileURL string) (*LocalFileBlob, erro // semantics for concurrent uploads to the same key. // // Returns TransferInfo with bytes transferred, transfer speed, and duration. -func (b *LocalFileBlob) Upload(ctx context.Context, srcPath, key string) (*TransferInfo, error) { +func (b *LocalFileBlob) Upload(ctx context.Context, srcPath, key string, _ time.Duration) (*TransferInfo, error) { _, span := trace.Start(ctx, "LocalFileBlob.Upload") defer span.End() diff --git a/internal/cache/store/file_test.go b/internal/cache/store/file_test.go index fc79588c67..64114ce79f 100644 --- a/internal/cache/store/file_test.go +++ b/internal/cache/store/file_test.go @@ -169,7 +169,7 @@ func TestLocalFileBlobUpload(t *testing.T) { key := "test/cache/artifact.txt" - info, err := blob.Upload(ctx, srcFile, key) + info, err := blob.Upload(ctx, srcFile, key, 0) if err != nil { t.Fatalf("Upload: %v", err) } @@ -231,7 +231,7 @@ func TestLocalFileBlobDownload(t *testing.T) { key := "test/cache/artifact.txt" // Upload first - _, err = blob.Upload(ctx, srcFile, key) + _, err = blob.Upload(ctx, srcFile, key, 0) if err != nil { t.Fatalf("Upload: %v", err) } @@ -285,7 +285,7 @@ func TestLocalFileBlobUploadOverwrite(t *testing.T) { if err := os.WriteFile(srcFile1, content1, 0o600); err != nil { t.Fatalf("WriteFile: %v", err) } - _, err = blob.Upload(ctx, srcFile1, key) + _, err = blob.Upload(ctx, srcFile1, key, 0) if err != nil { t.Fatalf("Upload: %v", err) } @@ -297,7 +297,7 @@ func TestLocalFileBlobUploadOverwrite(t *testing.T) { t.Fatalf("WriteFile: %v", err) } - info, err := blob.Upload(ctx, srcFile2, key) + info, err := blob.Upload(ctx, srcFile2, key, 0) if err != nil { t.Fatalf("Upload: %v", err) } @@ -363,7 +363,7 @@ func TestLocalFileBlobUploadInvalidKey(t *testing.T) { t.Fatalf("WriteFile: %v", err) } - _, err = blob.Upload(ctx, srcFile, "../../../etc/passwd") + _, err = blob.Upload(ctx, srcFile, "../../../etc/passwd", 0) if err == nil { t.Fatal("expected error, got nil") } @@ -518,12 +518,12 @@ func TestLocalFileBlobConcurrentUpload(t *testing.T) { done := make(chan error, 2) go func() { - _, err := blob.Upload(ctx, srcFile1, key) + _, err := blob.Upload(ctx, srcFile1, key, 0) done <- err }() go func() { - _, err := blob.Upload(ctx, srcFile2, key) + _, err := blob.Upload(ctx, srcFile2, key, 0) done <- err }() diff --git a/internal/cache/store/nsc.go b/internal/cache/store/nsc.go index 7524a56495..0debee9ddc 100644 --- a/internal/cache/store/nsc.go +++ b/internal/cache/store/nsc.go @@ -21,15 +21,20 @@ import ( // nscScheme is the URL scheme that routes an agent-managed cache store to NSC. const nscScheme = "nsc" -// nscDefaultExpiry is the artifact lifetime used both when uploading a cache -// entry (--expires_in) and when refreshing it on access (--ensure_minimum). -// Keep this aligned with CacheRegistry::Entry::DEFAULT_EXPIRES in the Buildkite -// backend. -// Cache entries are content-addressed and short-lived, so we cap storage growth -// rather than relying on NSC's no-expiry default. Every restore pushes the -// expiry back out to this duration from now, keeping hot caches alive while -// letting cold ones expire. -const nscDefaultExpiry = "72h" +// nscDefaultRetention is the fallback artifact lifetime used for --expires_in +// (upload) and --ensure_minimum (refresh on access) when the server does not +// supply a retention (older server). +const nscDefaultRetention = "72h" + +// nscRetentionArg formats a retention duration for nsc's --expires_in / --ensure_minimum +// flags, rounding up to whole hours. A zero or negative duration falls back to nscDefaultRetention. +func nscRetentionArg(retention time.Duration) string { + if retention <= 0 { + return nscDefaultRetention + } + hours := int64((retention + time.Hour - 1) / time.Hour) + return fmt.Sprintf("%dh", hours) +} // commandRunner executes an external command. It is a seam so tests can assert // the arguments passed to the nsc CLI without invoking the real binary. @@ -135,7 +140,7 @@ func validateKey(key string) error { return nil } -func (n *NscStore) Upload(ctx context.Context, filePath, key string) (*TransferInfo, error) { +func (n *NscStore) Upload(ctx context.Context, filePath, key string, retention time.Duration) (*TransferInfo, error) { _, span := trace.Start(ctx, "NscStore.Upload") defer span.End() @@ -150,7 +155,7 @@ func (n *NscStore) Upload(ctx context.Context, filePath, key string) (*TransferI start := time.Now() // Execute nsc artifact upload command - result, err := n.run(ctx, "", n.artifactArgs("upload", filePath, key, "--expires_in", nscDefaultExpiry)...) + result, err := n.run(ctx, "", n.artifactArgs("upload", filePath, key, "--expires_in", nscRetentionArg(retention))...) if err != nil { return nil, fmt.Errorf("failed to execute nsc upload command: %w", err) } @@ -238,17 +243,16 @@ func (n *NscStore) Download(ctx context.Context, key, filePath string) (*Transfe }, nil } -// 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. +// RefreshRetention pushes the artifact's expiry out to at least retention from +// now (falling back to nscDefaultRetention when unset) via `nsc artifact extend +// --ensure_minimum`. Using --ensure_minimum 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. 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) { +func (n *NscStore) RefreshRetention(ctx context.Context, key string, retention time.Duration) { 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. @@ -260,7 +264,7 @@ func (n *NscStore) RefreshRetention(ctx context.Context, key string) { } } - result, err := n.run(ctx, "", n.artifactArgs("extend", key, "--ensure_minimum", nscDefaultExpiry)...) + result, err := n.run(ctx, "", n.artifactArgs("extend", key, "--ensure_minimum", nscRetentionArg(retention))...) switch { case err != nil: slog.Warn("failed to refresh cache TTL, continuing (non-fatal)", "key", key, "error", err) diff --git a/internal/cache/store/nsc_test.go b/internal/cache/store/nsc_test.go index 368bb3a53c..c595284351 100644 --- a/internal/cache/store/nsc_test.go +++ b/internal/cache/store/nsc_test.go @@ -7,10 +7,33 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/google/go-cmp/cmp" ) +func TestNscRetentionArg(t *testing.T) { + tests := []struct { + name string + retention time.Duration + want string + }{ + {name: "zero falls back to default", retention: 0, want: nscDefaultRetention}, + {name: "negative falls back to default", retention: -time.Hour, want: nscDefaultRetention}, + {name: "whole hours", retention: 72 * time.Hour, want: "72h"}, + {name: "multi-day", retention: 7 * 24 * time.Hour, want: "168h"}, + {name: "rounds partial hours up", retention: 90 * time.Minute, want: "2h"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := nscRetentionArg(tt.retention); got != tt.want { + t.Errorf("nscRetentionArg(%v) = %q, want %q", tt.retention, got, tt.want) + } + }) + } +} + func TestNscStore_Interface(t *testing.T) { // This test ensures that NscStore properly implements the Blob interface var _ Blob = (*NscStore)(nil) @@ -261,11 +284,11 @@ func TestNscStore_PassesNamespace(t *testing.T) { var captured []string store := &NscStore{namespace: "my-namespace", run: fakeRunner(&captured)} - if _, err := store.Upload(ctx, testFile, "key"); err != nil { + if _, err := store.Upload(ctx, testFile, "key", 7*24*time.Hour); err != nil { t.Fatalf("Upload: %v", err) } - wantArgs := []string{"nsc", "artifact", "upload", testFile, "key", "--expires_in", "72h", "--namespace", "my-namespace"} + wantArgs := []string{"nsc", "artifact", "upload", testFile, "key", "--expires_in", "168h", "--namespace", "my-namespace"} if diff := cmp.Diff(wantArgs, captured); diff != "" { t.Errorf("upload args mismatch (-want +got):\n%s", diff) } @@ -333,9 +356,9 @@ func TestNscStore_RefreshRetention(t *testing.T) { var calls [][]string store := &NscStore{namespace: "my-namespace", run: recordingRunner(&calls, nil)} - store.RefreshRetention(ctx, "key") + store.RefreshRetention(ctx, "key", 7*24*time.Hour) - wantExtend := []string{"nsc", "artifact", "extend", "key", "--ensure_minimum", "72h", "--namespace", "my-namespace"} + wantExtend := []string{"nsc", "artifact", "extend", "key", "--ensure_minimum", "168h", "--namespace", "my-namespace"} var gotExtend []string for _, c := range calls { if isCommand(c, "nsc", "artifact", "extend") && !isCommand(c, "nsc", "artifact", "extend", "--help") { @@ -360,7 +383,7 @@ func TestNscStore_RefreshRetention_UpdatesCLIWhenExtendUnsupported(t *testing.T) } store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - store.RefreshRetention(ctx, "key") + store.RefreshRetention(ctx, "key", 0) var updated, extended bool for _, c := range calls { @@ -398,7 +421,7 @@ func TestNscStore_RefreshRetention_UpdatesCLIWhenExtendHelpLacksEnsureMinimum(t } store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - store.RefreshRetention(ctx, "key") + store.RefreshRetention(ctx, "key", 0) var updated, extended bool for _, c := range calls { @@ -432,7 +455,7 @@ func TestNscStore_RefreshRetention_SwallowsFailure(t *testing.T) { var calls [][]string store := &NscStore{namespace: "ns", run: recordingRunner(&calls, respond)} - store.RefreshRetention(ctx, "key") // must not panic + store.RefreshRetention(ctx, "key", 0) // must not panic } func TestNewNscStore_RequiresNamespace(t *testing.T) { @@ -451,7 +474,7 @@ func TestNscStore_ValidationShortCircuits(t *testing.T) { return &CommandResult{}, nil }} - if _, err := store.Upload(ctx, "invalid;path", "valid-key"); err == nil { + if _, err := store.Upload(ctx, "invalid;path", "valid-key", 0); 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 { @@ -509,7 +532,7 @@ func TestNscStore_Integration(t *testing.T) { // Test upload key := "integration-test/test-file.txt" - transferInfo, err := store.Upload(ctx, testFile, key) + transferInfo, err := store.Upload(ctx, testFile, key, 0) if err != nil { t.Fatalf("Upload should succeed with valid NSC setup: %v", err) } diff --git a/internal/cache/store/s3.go b/internal/cache/store/s3.go index 19d8196716..29e26162e9 100644 --- a/internal/cache/store/s3.go +++ b/internal/cache/store/s3.go @@ -300,8 +300,10 @@ func resolveTransferSettings(opts *Options) transferSettings { } } -// Upload uploads a file to S3 using multipart upload for parallel transfers -func (b *S3Blob) Upload(ctx context.Context, filePath, key string) (*TransferInfo, error) { +// Upload uploads a file to S3 using multipart upload for parallel transfers. +// retention is ignored: S3 object lifetime is governed by the bucket's +// lifecycle policy, which the agent does not set per object. +func (b *S3Blob) Upload(ctx context.Context, filePath, key string, _ time.Duration) (*TransferInfo, error) { ctx, span := trace.Start(ctx, "S3Blob.Upload") defer span.End() @@ -487,7 +489,11 @@ func (b *S3Blob) Download(ctx context.Context, key, destPath string) (*TransferI // // 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) { +// +// retention is ignored: the self-copy only refreshes LastModified, and the +// effective lifetime is whatever window the bucket's lifecycle policy applies +// from that timestamp — the agent cannot lengthen it per object. +func (b *S3Blob) RefreshRetention(ctx context.Context, key string, _ time.Duration) { fullKey := b.getFullKey(key) refreshObjectExpiry(ctx, b.client, b.bucketName, fullKey) } diff --git a/internal/cache/store/s3_test.go b/internal/cache/store/s3_test.go index bdd66f159c..cbe1728f82 100644 --- a/internal/cache/store/s3_test.go +++ b/internal/cache/store/s3_test.go @@ -639,7 +639,7 @@ func TestS3Blob_RefreshRetention(t *testing.T) { copier := &fakeCopier{} b := &S3Blob{client: copier, bucketName: "my-bucket", prefix: "prefix"} - b.RefreshRetention(t.Context(), "key") + b.RefreshRetention(t.Context(), "key", 0) if len(copier.calls) != 1 { t.Fatalf("CopyObject calls = %d, want 1", len(copier.calls))