From af053391661e37bbd736443abc974ff4810ffc2c Mon Sep 17 00:00:00 2001 From: bootjp Date: Thu, 22 Oct 2026 22:34:06 +0900 Subject: [PATCH 1/3] snapshotoffload: reclaim unreferenced snapshots with publication claims --- adapter/s3.go | 115 +- adapter/s3_test.go | 113 + cmd/elastickv-snapshot-offload/main.go | 66 +- ...artial_physical_snapshot_object_offload.md | 65 +- internal/snapshotoffload/claim.go | 155 ++ internal/snapshotoffload/claim_test.go | 132 ++ internal/snapshotoffload/manifest.go | 10 + internal/snapshotoffload/offload_test.go | 88 +- internal/snapshotoffload/publish.go | 178 +- .../restore_corruption_test.go | 191 ++ internal/snapshotoffload/retention.go | 996 ++++++++ internal/snapshotoffload/retention_test.go | 2008 +++++++++++++++++ internal/snapshotoffload/s3_store.go | 331 ++- internal/snapshotoffload/s3_store_test.go | 533 ++++- internal/snapshotoffload/scheduler.go | 11 +- internal/snapshotoffload/scheduler_test.go | 16 +- internal/snapshotoffload/store.go | 785 ++++++- 17 files changed, 5597 insertions(+), 196 deletions(-) create mode 100644 internal/snapshotoffload/claim.go create mode 100644 internal/snapshotoffload/claim_test.go create mode 100644 internal/snapshotoffload/restore_corruption_test.go create mode 100644 internal/snapshotoffload/retention.go create mode 100644 internal/snapshotoffload/retention_test.go diff --git a/adapter/s3.go b/adapter/s3.go index 6020843c0..7113772a6 100644 --- a/adapter/s3.go +++ b/adapter/s3.go @@ -227,6 +227,11 @@ type s3BucketEntry struct { CreationDate string `xml:"CreationDate"` } +type s3VersioningConfiguration struct { + XMLName xml.Name `xml:"VersioningConfiguration"` + XMLNS string `xml:"xmlns,attr,omitempty"` +} + type s3AccessControlPolicy struct { XMLName xml.Name `xml:"AccessControlPolicy"` XMLNS string `xml:"xmlns,attr,omitempty"` @@ -507,6 +512,14 @@ func (s *S3Server) handleRoot(w http.ResponseWriter, r *http.Request) { //nolint:cyclop // handleBucket routes to sub-handlers based on method+query; branching is by design. func (s *S3Server) handleBucket(w http.ResponseWriter, r *http.Request, bucket string) { query := r.URL.Query() + if query.Has("versioning") { + if r.Method == http.MethodGet { + s.getBucketVersioning(w, r, bucket) + } else { + writeS3Error(w, http.StatusMethodNotAllowed, "MethodNotAllowed", "unsupported method", bucket, "") + } + return + } if query.Has("acl") { switch r.Method { case http.MethodGet: @@ -704,6 +717,25 @@ func (s *S3Server) headBucket(w http.ResponseWriter, r *http.Request, bucket str w.WriteHeader(http.StatusOK) } +func (s *S3Server) getBucketVersioning(w http.ResponseWriter, r *http.Request, bucket string) { + readTS := s.readTS() + readPin := s.pinReadTS(readTS) + defer readPin.Release() + + _, exists, err := s.loadBucketMetaAt(r.Context(), bucket, readTS) + if err != nil { + writeS3InternalError(w, err) + return + } + if !exists { + writeS3Error(w, http.StatusNotFound, "NoSuchBucket", "bucket not found", bucket, "") + return + } + // The bundled endpoint does not implement object versions. An empty + // VersioningConfiguration is the S3 response for an unversioned bucket. + writeS3XML(w, http.StatusOK, s3VersioningConfiguration{XMLNS: s3XMLNamespace}) +} + func (s *S3Server) deleteBucket(w http.ResponseWriter, r *http.Request, bucket string) { var deletedGeneration uint64 err := s.retryS3Mutation(r.Context(), func() error { @@ -1144,46 +1176,12 @@ func (s *S3Server) deleteObject(w http.ResponseWriter, r *http.Request, bucket s var cleanupManifest *s3ObjectManifest var generation uint64 err := s.retryS3Mutation(r.Context(), func() error { - readTS := s.readTS() - readTimestamp, err := s.beginTxnReadTimestamp(r.Context(), readTS, "s3 delete object: begin read timestamp") - if err != nil { - return errors.Wrap(err, "s3: allocate startTS for mutation") - } - readTS = readTimestamp.Timestamp() - startTS := readTS - readPin := s.pinReadTS(readTS) - defer readPin.Release() - - meta, exists, err := s.loadBucketMetaAt(r.Context(), bucket, readTS) + manifest, objectGeneration, err := s.deleteObjectAttempt(r, bucket, objectKey) if err != nil { - return errors.WithStack(err) - } - if !exists || meta == nil { - return &s3ResponseError{ - Status: http.StatusNotFound, - Code: "NoSuchBucket", - Message: "bucket not found", - Bucket: bucket, - Key: objectKey, - } - } - - headKey := s3keys.ObjectManifestKey(bucket, meta.Generation, objectKey) - manifest, found, err := s.loadObjectManifestAt(r.Context(), headKey, readTS) - if err != nil { - return errors.WithStack(err) - } - if !found { - cleanupManifest = nil - return validateS3AbsentObjectDelete(r, bucket, objectKey) - } - if err := s.dispatchConditionalObjectDelete( - r, readTimestamp, startTS, headKey, manifest, bucket, objectKey, - ); err != nil { return err } cleanupManifest = manifest - generation = meta.Generation + generation = objectGeneration return nil }) if err != nil { @@ -1196,6 +1194,46 @@ func (s *S3Server) deleteObject(w http.ResponseWriter, r *http.Request, bucket s w.WriteHeader(http.StatusNoContent) } +func (s *S3Server) deleteObjectAttempt( + r *http.Request, bucket string, objectKey string, +) (*s3ObjectManifest, uint64, error) { + readTS := s.readTS() + readTimestamp, err := s.beginTxnReadTimestamp(r.Context(), readTS, "s3 delete object: begin read timestamp") + if err != nil { + return nil, 0, errors.Wrap(err, "s3: allocate startTS for mutation") + } + readTS = readTimestamp.Timestamp() + readPin := s.pinReadTS(readTS) + defer readPin.Release() + + meta, exists, err := s.loadBucketMetaAt(r.Context(), bucket, readTS) + if err != nil { + return nil, 0, errors.WithStack(err) + } + if !exists || meta == nil { + return nil, 0, &s3ResponseError{ + Status: http.StatusNotFound, + Code: "NoSuchBucket", + Message: "bucket not found", + Bucket: bucket, + Key: objectKey, + } + } + + headKey := s3keys.ObjectManifestKey(bucket, meta.Generation, objectKey) + manifest, found, err := s.loadObjectManifestAt(r.Context(), headKey, readTS) + if err != nil { + return nil, 0, errors.WithStack(err) + } + if !found { + return nil, meta.Generation, validateS3AbsentObjectDelete(r, bucket, objectKey) + } + if err := s.dispatchConditionalObjectDelete(r, readTimestamp, readTS, headKey, manifest, bucket, objectKey); err != nil { + return nil, 0, err + } + return manifest, meta.Generation, nil +} + func validateS3AbsentObjectDelete(r *http.Request, bucket, objectKey string) error { if _, conditional := r.Header[http.CanonicalHeaderKey("If-Match")]; conditional { return newS3ResponseError(http.StatusNotFound, "NoSuchKey", @@ -2605,6 +2643,9 @@ func isReadOnlyS3Request(r *http.Request) bool { // isReadOnlyBucketOp returns true for bucket-level read-only operations // (HeadBucket, ListObjectsV2). func isReadOnlyBucketOp(method string, q url.Values) bool { + if q.Has("versioning") { + return false + } switch method { case http.MethodHead: // HeadBucket: no query params (or only "location"). @@ -2656,7 +2697,7 @@ func validateS3PutPreconditions(ifMatch, ifNoneMatch string, previous *s3ObjectM return errors.New("object already exists") } if ifMatch != "" { - if previous == nil || strings.Trim(ifMatch, `"`) != previous.ETag { + if previous == nil || (ifMatch != "*" && strings.Trim(ifMatch, `"`) != previous.ETag) { return errors.New("etag precondition failed") } } diff --git a/adapter/s3_test.go b/adapter/s3_test.go index f2b4ee950..554792d35 100644 --- a/adapter/s3_test.go +++ b/adapter/s3_test.go @@ -92,6 +92,116 @@ func TestS3Server_BucketAndObjectLifecycle(t *testing.T) { require.Equal(t, http.StatusNoContent, rec.Code) } +func TestS3Server_GetBucketVersioningReportsUnversioned(t *testing.T) { + t.Parallel() + + st := store.NewMVCCStore() + server := NewS3Server(nil, "", st, newLocalAdapterCoordinator(st), nil) + + rec := httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodPut, "/bucket-a", nil)) + require.Equal(t, http.StatusOK, rec.Code) + + rec = httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodGet, "/bucket-a?versioning", nil)) + require.Equal(t, http.StatusOK, rec.Code) + require.Contains(t, rec.Body.String(), "") +} + +func TestS3Server_DeleteObjectHonorsIfMatch(t *testing.T) { + t.Parallel() + + st := store.NewMVCCStore() + server := NewS3Server(nil, "", st, newLocalAdapterCoordinator(st), nil) + + rec := httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodPut, "/bucket-a", nil)) + require.Equal(t, http.StatusOK, rec.Code) + + rec = httptest.NewRecorder() + req := newS3TestRequest(http.MethodPut, "/bucket-a/object", strings.NewReader("payload")) + server.handle(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + etag := rec.Header().Get("ETag") + require.NotEmpty(t, etag) + + rec = httptest.NewRecorder() + req = newS3TestRequest(http.MethodDelete, "/bucket-a/object", nil) + req.Header.Set("If-Match", `"stale"`) + server.handle(rec, req) + require.Equal(t, http.StatusPreconditionFailed, rec.Code) + + rec = httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodGet, "/bucket-a/object", nil)) + require.Equal(t, http.StatusOK, rec.Code, "failed delete precondition must preserve the object") + + rec = httptest.NewRecorder() + req = newS3TestRequest(http.MethodDelete, "/bucket-a/object", nil) + req.Header.Set("If-Match", etag) + server.handle(rec, req) + require.Equal(t, http.StatusNoContent, rec.Code) +} + +func TestS3Server_DeleteObjectHonorsWildcardIfMatch(t *testing.T) { + t.Parallel() + + st := store.NewMVCCStore() + server := NewS3Server(nil, "", st, newLocalAdapterCoordinator(st), nil) + + rec := httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodPut, "/bucket-a", nil)) + require.Equal(t, http.StatusOK, rec.Code) + + rec = httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodPut, "/bucket-a/object", strings.NewReader("payload"))) + require.Equal(t, http.StatusOK, rec.Code) + + rec = httptest.NewRecorder() + req := newS3TestRequest(http.MethodDelete, "/bucket-a/object", nil) + req.Header.Set("If-Match", "*") + server.handle(rec, req) + require.Equal(t, http.StatusNoContent, rec.Code) + + rec = httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodGet, "/bucket-a/object", nil)) + require.Equal(t, http.StatusNotFound, rec.Code) + + for _, ifMatch := range []string{"*", `"missing-etag"`} { + rec = httptest.NewRecorder() + req := newS3TestRequest(http.MethodDelete, "/bucket-a/object", nil) + req.Header.Set("If-Match", ifMatch) + server.handle(rec, req) + require.Equal(t, http.StatusNotFound, rec.Code, "If-Match %s", ifMatch) + } +} + +func TestS3Server_PutObjectHonorsWildcardIfMatch(t *testing.T) { + t.Parallel() + + st := store.NewMVCCStore() + server := NewS3Server(nil, "", st, newLocalAdapterCoordinator(st), nil) + + rec := httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodPut, "/bucket-a", nil)) + require.Equal(t, http.StatusOK, rec.Code) + + rec = httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodPut, "/bucket-a/object", strings.NewReader("first"))) + require.Equal(t, http.StatusOK, rec.Code) + + rec = httptest.NewRecorder() + req := newS3TestRequest(http.MethodPut, "/bucket-a/object", strings.NewReader("replacement")) + req.Header.Set("If-Match", "*") + server.handle(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + rec = httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodGet, "/bucket-a/object", nil)) + require.Equal(t, http.StatusOK, rec.Code) + require.Equal(t, "replacement", rec.Body.String()) +} + func TestS3Server_ProxiesFollowerRequests(t *testing.T) { t.Parallel() @@ -2431,6 +2541,9 @@ func TestIsReadOnlyS3Request(t *testing.T) { {"HeadBucket", http.MethodHead, "/bucket", true}, // Allowed: ListObjectsV2. {"ListObjectsV2", http.MethodGet, "/bucket?list-type=2", true}, + // Not allowed: a versioning subresource cannot become anonymous by + // also carrying the ListObjectsV2 discriminator. + {"GetBucketVersioning_WithListType", http.MethodGet, "/bucket?versioning&list-type=2", false}, // Not allowed: bucket GET without list-type=2 (returns NotImplemented later, not via anonymous path). {"GetBucket_NoListType", http.MethodGet, "/bucket", false}, // Not allowed: object GET with extra query params. diff --git a/cmd/elastickv-snapshot-offload/main.go b/cmd/elastickv-snapshot-offload/main.go index efdd1640d..137a1bcbc 100644 --- a/cmd/elastickv-snapshot-offload/main.go +++ b/cmd/elastickv-snapshot-offload/main.go @@ -31,16 +31,17 @@ const ( ) type storeFlags struct { - storeKind string - localRoot string - s3Bucket string - s3Region string - s3Endpoint string - s3Profile string - s3PathStyle bool - s3ServerSideEncryption string - s3KMSKeyID string - s3DisableChecksumHeaders bool + storeKind string + localRoot string + s3Bucket string + s3Region string + s3Endpoint string + s3Profile string + s3PathStyle bool + s3ServerSideEncryption string + s3KMSKeyID string + s3DisableChecksumHeaders bool + s3AllowVersionedLifecycle bool } type publishConfig struct { @@ -186,7 +187,8 @@ func addStoreFlags(fs *flag.FlagSet, cfg *storeFlags) { cfg.s3ServerSideEncryption = string(s3SSEAES256) fs.StringVar(&cfg.storeKind, "store", storeLocal, "Object store backend: local or s3") fs.StringVar(&cfg.localRoot, "local-root", "", "Local object store root when --store=local") - fs.StringVar(&cfg.s3Bucket, "s3-bucket", "", "S3 bucket when --store=s3") + fs.StringVar(&cfg.s3Bucket, "s3-bucket", "", + "S3 bucket when --store=s3; credentials require s3:GetBucketVersioning") fs.StringVar(&cfg.s3Region, "s3-region", cfg.s3Region, "S3 signing region") fs.StringVar(&cfg.s3Endpoint, "s3-endpoint", "", "S3-compatible endpoint URL") fs.StringVar(&cfg.s3Profile, "s3-profile", "", "AWS shared config profile") @@ -194,6 +196,7 @@ func addStoreFlags(fs *flag.FlagSet, cfg *storeFlags) { fs.StringVar(&cfg.s3ServerSideEncryption, "s3-sse", cfg.s3ServerSideEncryption, "Server-side encryption algorithm for uploaded objects: AES256 or aws:kms") fs.StringVar(&cfg.s3KMSKeyID, "s3-kms-key-id", "", "KMS key ARN or bare key ID when --s3-sse=aws:kms; aliases are rejected") fs.BoolVar(&cfg.s3DisableChecksumHeaders, "s3-disable-checksum-headers", false, "Do not send S3 checksum headers; keep metadata and restore-time verification") + fs.BoolVar(&cfg.s3AllowVersionedLifecycle, "s3-allow-versioned-with-lifecycle", false, "Allow an S3 versioned bucket after confirming noncurrent-version expiration is configured") } func validateStoreFlags(cfg storeFlags) error { @@ -215,11 +218,12 @@ func validateStoreFlags(cfg storeFlags) error { return nil } -func runPublish(ctx context.Context, cfg *publishConfig, stdout io.Writer, logger *slog.Logger) error { - store, err := openObjectStore(ctx, cfg.store) +func runPublish(ctx context.Context, cfg *publishConfig, stdout io.Writer, logger *slog.Logger) (retErr error) { + store, closeStore, err := openObjectStore(ctx, cfg.store) if err != nil { return err } + defer func() { retErr = errors.CombineErrors(retErr, closeStore()) }() manifest, err := snapshotoffload.PublishPersistedSnapshot(ctx, snapshotoffload.PublishOptions{ Store: store, DataDir: cfg.dataDir, @@ -249,11 +253,12 @@ func runPublish(ctx context.Context, cfg *publishConfig, stdout io.Writer, logge return nil } -func runRestore(ctx context.Context, cfg *restoreConfig, logger *slog.Logger) error { - store, err := openObjectStore(ctx, cfg.store) +func runRestore(ctx context.Context, cfg *restoreConfig, logger *slog.Logger) (retErr error) { + store, closeStore, err := openObjectStore(ctx, cfg.store) if err != nil { return err } + defer func() { retErr = errors.CombineErrors(retErr, closeStore()) }() peers, err := parsePeers(cfg.peerCSV) if err != nil { return err @@ -279,31 +284,34 @@ func runRestore(ctx context.Context, cfg *restoreConfig, logger *slog.Logger) er return nil } -func openObjectStore(ctx context.Context, cfg storeFlags) (snapshotoffload.ObjectStore, error) { +func openObjectStore( + ctx context.Context, cfg storeFlags, +) (snapshotoffload.PublishStore, func() error, error) { switch cfg.storeKind { case storeLocal: store, err := snapshotoffload.NewLocalStore(cfg.localRoot) if err != nil { - return nil, errors.Wrap(err, "open local object store") + return nil, nil, errors.Wrap(err, "open local object store") } - return store, nil + return store, store.Close, nil case storeS3: store, err := snapshotoffload.NewS3Store(ctx, snapshotoffload.S3StoreConfig{ - Bucket: cfg.s3Bucket, - Region: cfg.s3Region, - Endpoint: cfg.s3Endpoint, - Profile: cfg.s3Profile, - ForcePathStyle: cfg.s3PathStyle, - ServerSideEncryption: cfg.s3ServerSideEncryption, - SSEKMSKeyID: cfg.s3KMSKeyID, - DisableChecksumHeaders: cfg.s3DisableChecksumHeaders, + Bucket: cfg.s3Bucket, + Region: cfg.s3Region, + Endpoint: cfg.s3Endpoint, + Profile: cfg.s3Profile, + ForcePathStyle: cfg.s3PathStyle, + ServerSideEncryption: cfg.s3ServerSideEncryption, + SSEKMSKeyID: cfg.s3KMSKeyID, + DisableChecksumHeaders: cfg.s3DisableChecksumHeaders, + AllowVersionedBucketWithLifecycle: cfg.s3AllowVersionedLifecycle, }) if err != nil { - return nil, errors.Wrap(err, "open s3 object store") + return nil, nil, errors.Wrap(err, "open s3 object store") } - return store, nil + return store, func() error { return nil }, nil default: - return nil, errors.Errorf("unknown --store %q", cfg.storeKind) + return nil, nil, errors.Errorf("unknown --store %q", cfg.storeKind) } } diff --git a/docs/design/2026_07_19_partial_physical_snapshot_object_offload.md b/docs/design/2026_07_19_partial_physical_snapshot_object_offload.md index 2956620aa..bbdba59ed 100644 --- a/docs/design/2026_07_19_partial_physical_snapshot_object_offload.md +++ b/docs/design/2026_07_19_partial_physical_snapshot_object_offload.md @@ -1,6 +1,6 @@ # Physical Snapshot Object Offload -Status: Partial — M0/M1/M2 implemented; M3 pending +Status: Partial — M0/M1/M2 implemented; M3 retention/GC implemented, remaining M3 items pending Author: bootjp Date: 2026-07-19 Updated: 2026-07-23 @@ -48,7 +48,7 @@ The M1 object-store-neutral substrate now adds: - `cmd/elastickv-snapshot-offload publish` and `restore` for local and S3-backed operator workflows. -The runtime scheduler and retention/GC remain pending. +The runtime scheduler remains pending. Retention/GC and corruption tests are implemented; the remaining M3 items (restore drills, multi-node acceptance, operator documentation) are pending. ## 2. Safety boundary @@ -121,6 +121,53 @@ window. GC runs in two phases: 2. after a grace period, rebuild the live payload SHA set from all remaining manifests and delete only payload objects with no live reference. +Payload reclamation is **two-pass mark-and-sweep with storage-visible +claims**. A pass that finds a payload unreferenced and past its grace +*marks* it; only a later pass that finds the same object unchanged, with +the mark aged past `MinMarkAge`, may claim and delete it. + +Claims close the race that an ETag precondition cannot. A publisher +conditionally creates `/v1/claims/sha256//.lock` +before it creates or reuses a payload and holds that claim until the +manifest commit completes. GC conditionally claims every sweepable +payload **before** its authoritative manifest scan, holds all those +claims through deletion, and skips a payload already claimed by a +publisher. Therefore a publish that wins first is either still holding +the claim or is visible in the final scan; a GC that wins first prevents +the publisher from committing until the sweep finishes. The payload +body is not rewritten merely to advance mtime, so content-addressed +deduplication does not re-transfer multi-terabyte snapshots. + +Manifest reuse uses the same claim protocol. Because manifests are +small, a matching reused manifest is refreshed while claimed. GC claims +every initially expired manifest before the authoritative final scan, +holds those claims through deletion, and deletes only manifests whose +size, mtime, and ETag still match the initial scan. A manifest that was +refreshed before GC won its claim, or whose claim is held elsewhere, is +kept; its payload is added back to the live set before payload sweep. The +refresh advances the existing schema-v1 `created_at` value by one nanosecond, +which changes the manifest bytes and ETag even when the caller supplies the +same timestamp. This avoids depending on object-store timestamp precision +without making newly written schema-v1 manifests unreadable by older binaries. + +The mark state is in-memory and per-process. Losing it on restart +delays reclamation by one pass and never advances it. Marks for objects +absent from a pass's (complete) listing are pruned, so external +reclamation cannot leak them. + +Claim objects are released with a conditional delete even when the +calling context is cancelled. A process crash can leave an orphan claim; +that fails safe by delaying publication and reclamation for the hashed +target key. Publication waits at most 30 seconds with bounded exponential +backoff, then returns a retryable `ErrObjectClaimed` so an orphan cannot +occupy the scheduler's upload slot indefinitely. GC never waits for a busy +claim and retries on a later pass. All claims held by one GC pass share one +30-second cancellation-independent cleanup deadline, so shutdown latency is +bounded independently of the number of claimed objects. Operators may remove +an orphan claim only after confirming no publisher or GC owns it. Automatic +lease expiry is not used because expiring a live claim reopens the data-loss +race. + Malformed manifests fail closed: they are reported and excluded from both automatic manifest deletion and payload reclamation. Listing failure, pagination failure, or an incomplete group scan performs no deletes. This @@ -152,11 +199,21 @@ credentials provider, schedule, retention count/window, upload concurrency, and server-side encryption mode. Static secrets must use file or environment providers and must not appear in process arguments or manifests. +**Versioned buckets.** Retention deletes by key, not by version. On a +bucket with S3 versioning enabled a keyed delete only writes a delete +marker, so the bytes persist as a noncurrent version that later +listings cannot see: GC reports successful reclamation while storage +grows without bound. `NewS3Store` therefore checks bucket versioning and +rejects `Enabled` or `Suspended` by default. Operators that have already +configured noncurrent-version expiration must acknowledge that policy +explicitly with `--s3-allow-versioned-with-lifecycle`; the code cannot +prove the lifecycle configuration itself. + Storage-envelope encryption protects values but not all physical keys and metadata. The external bucket therefore requires private ACLs, TLS, and server-side encryption (SSE-S3 or SSE-KMS). Anonymous reads and writes are a deployment failure. Object credentials need only scoped list/get/put/delete -permissions below the configured prefix. +permissions below the configured prefix, plus `s3:GetBucketVersioning`. ## 8. Milestones @@ -165,7 +222,7 @@ permissions below the configured prefix. | M0 | Persisted snapshot export handle, complete-payload restore preparation, focused design | Implemented in the first substrate PR | | M1 | Object client interface, S3-compatible implementation, immutable payload/manifest publication, download verification, operator CLI | Implemented: local and S3 stores, manifest schema, payload-first publish, verified restore, and publish/restore CLI | | M2 | Leader-only per-group scheduler, metrics, jitter, concurrency bounds, cancellation and restart idempotency | Implemented: `internal/snapshotoffload/scheduler.go`. Leadership is checked before the snapshot is opened and re-checked immediately before the manifest commit via `PublishOptions.VerifyLeader`; uploads are bounded (default one per process) with interval jitter; cancellation is treated as shutdown rather than publish failure; restart idempotency comes from the object store, since publish reuses a matching committed manifest. Not yet wired into `main.go` — the runtime flags are M3. | -| M3 | Retention/GC, restore drills, corruption tests, multi-node acceptance, operational documentation | Pending | +| M3 | Retention/GC, restore drills, corruption tests, multi-node acceptance, operational documentation | Partially implemented: the §5 two-phase retention/GC (`retention.go`) with cross-process publish/GC claims and `RetentionStore` list/delete on both the local and S3 stores. Restore corruption drills are implemented (`restore_corruption_test.go`: truncated, over-length, missing and tampered-descriptor payloads, plus a positive restore-into-fresh-dir drill). Versioned buckets are rejected unless the operator explicitly confirms noncurrent-version expiration. Multi-node acceptance and the complete operator runbook remain pending. | The filename and header remain `partial` until M1-M3 complete the central object-offload subsystem. At that point the completion PR must use `git mv` to diff --git a/internal/snapshotoffload/claim.go b/internal/snapshotoffload/claim.go new file mode 100644 index 000000000..953faff9b --- /dev/null +++ b/internal/snapshotoffload/claim.go @@ -0,0 +1,155 @@ +package snapshotoffload + +import ( + "bytes" + "context" + "crypto/rand" + "crypto/sha256" + "encoding/hex" + "path" + "strings" + "time" + + "github.com/cockroachdb/errors" +) + +const ( + objectClaimBytes = 32 + claimAcquireTimeout = 30 * time.Second + claimReleaseTimeout = 30 * time.Second + claimReleaseConcurrency = 8 + objectClaimInitialRetryDelay = 25 * time.Millisecond + objectClaimMaxRetryDelay = time.Second + objectClaimBackoffMultiplier = 2 +) + +type claimBackingStore interface { + ObjectStore + DeleteObjectIfUnmodified(ctx context.Context, key string, cond DeletePrecondition) error +} + +type storeObjectClaim struct { + store claimBackingStore + key string + cond DeletePrecondition +} + +// AcquireObjectClaim acquires a storage-visible claim for a local object key. +func (s *LocalStore) AcquireObjectClaim(ctx context.Context, key string) (ObjectClaim, error) { + if _, err := s.pathForKey(key); err != nil { + return nil, err + } + return acquireStoreObjectClaim(ctx, s, key) +} + +// AcquireObjectClaim acquires a storage-visible claim for an S3 object key. +func (s *S3Store) AcquireObjectClaim(ctx context.Context, key string) (ObjectClaim, error) { + if _, err := validateStoreObjectKey(key); err != nil { + return nil, err + } + return acquireStoreObjectClaim(ctx, s, key) +} + +func acquireStoreObjectClaim(ctx context.Context, store claimBackingStore, targetKey string) (ObjectClaim, error) { + claimKey := objectClaimKey(targetKey) + token := make([]byte, objectClaimBytes) + if _, err := rand.Read(token); err != nil { + return nil, errors.Wrap(err, "generate object claim token") + } + claimSHA := hexSHA256Bytes(token) + info, err := store.PutObject(ctx, claimKey, bytes.NewReader(token), PutOptions{ + Size: int64(len(token)), + SHA256: claimSHA, + ContentType: "application/octet-stream", + }) + if err != nil { + if errors.Is(err, ErrIntegrity) || errors.Is(err, ErrObjectConflict) || errors.Is(err, ErrObjectNotFound) { + return nil, errors.Wrapf(ErrObjectClaimed, "object %s", targetKey) + } + return nil, errors.Wrapf(err, "claim object %s", targetKey) + } + return &storeObjectClaim{ + store: store, + key: claimKey, + cond: DeletePrecondition{ + ETag: info.ETag, + Size: info.Size, + UpdatedAt: info.UpdatedAt, + }, + }, nil +} + +func (c *storeObjectClaim) Release(ctx context.Context) error { + if c == nil || c.store == nil { + return nil + } + if err := c.store.DeleteObjectIfUnmodified(ctx, c.key, c.cond); err != nil { + return errors.Wrap(err, "release object claim") + } + return nil +} + +func objectClaimKey(targetKey string) string { + normalized := normalizeObjectKey(targetKey) + prefix := "." + if idx := strings.LastIndex(normalized, "/v1/"); idx >= 0 { + prefix = normalized[:idx] + } else if strings.HasPrefix(normalized, "v1/") { + prefix = "." + } + sum := sha256.Sum256([]byte(normalized)) + hexSum := hex.EncodeToString(sum[:]) + return path.Join(prefix, "v1", "claims", "sha256", hexSum[:2], hexSum+".lock") +} + +func objectClaimStore(store ObjectStore) (ObjectClaimStore, error) { + claimStore, ok := store.(ObjectClaimStore) + if !ok { + return nil, errors.Wrap(ErrInvalidOptions, + "object store must support cross-process claims when snapshot publication and retention can overlap") + } + return claimStore, nil +} + +func acquireObjectClaimWaiting(ctx context.Context, store ObjectClaimStore, key string) (ObjectClaim, error) { + return acquireObjectClaimWithin(ctx, store, key, claimAcquireTimeout) +} + +func acquireObjectClaimWithin( + ctx context.Context, store ObjectClaimStore, key string, maxWait time.Duration, +) (ObjectClaim, error) { + waitCtx, cancel := context.WithTimeout(ctx, maxWait) + defer cancel() + delay := objectClaimInitialRetryDelay + for { + claim, err := store.AcquireObjectClaim(waitCtx, key) + if err == nil { + return claim, nil + } + if !errors.Is(err, ErrObjectClaimed) { + return nil, errors.Wrap(err, "acquire object claim") + } + timer := time.NewTimer(delay) + select { + case <-waitCtx.Done(): + timer.Stop() + return nil, errors.Wrapf(ErrObjectClaimed, + "timed out waiting for object %s claim: %v", key, waitCtx.Err()) + case <-timer.C: + } + delay = min(delay*objectClaimBackoffMultiplier, objectClaimMaxRetryDelay) + } +} + +func releaseObjectClaim(ctx context.Context, claim ObjectClaim) error { + releaseCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), claimReleaseTimeout) + defer cancel() + return releaseObjectClaimWithContext(releaseCtx, claim) +} + +func releaseObjectClaimWithContext(ctx context.Context, claim ObjectClaim) error { + if err := claim.Release(ctx); err != nil { + return errors.Wrap(err, "release object claim") + } + return nil +} diff --git a/internal/snapshotoffload/claim_test.go b/internal/snapshotoffload/claim_test.go new file mode 100644 index 000000000..1bc8740ba --- /dev/null +++ b/internal/snapshotoffload/claim_test.go @@ -0,0 +1,132 @@ +package snapshotoffload + +import ( + "context" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestObjectClaimsAreExclusiveAndReusable(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + store func(*testing.T) ObjectClaimStore + }{ + { + name: "local", + store: func(t *testing.T) ObjectClaimStore { + return newTestLocalStore(t, filepath.Join(t.TempDir(), "objects")) + }, + }, + { + name: "s3", + store: func(t *testing.T) ObjectClaimStore { + return newTestS3Store(t, newFakeS3Client()) + }, + }, + } + + for _, tc := range tests { + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + store := tc.store(t) + const key = "cluster-a/v1/payloads/sha256/aa/target.fsm" + + first, err := store.AcquireObjectClaim(ctx, key) + require.NoError(t, err) + + _, err = store.AcquireObjectClaim(ctx, key) + require.ErrorIs(t, err, ErrObjectClaimed) + + require.NoError(t, first.Release(ctx)) + second, err := store.AcquireObjectClaim(ctx, key) + require.NoError(t, err) + require.NoError(t, second.Release(ctx)) + }) + } +} + +type alwaysClaimedStore struct { + attempts atomic.Int32 +} + +func (s *alwaysClaimedStore) AcquireObjectClaim(context.Context, string) (ObjectClaim, error) { + s.attempts.Add(1) + return nil, ErrObjectClaimed +} + +func TestAcquireObjectClaimWaitingIsBounded(t *testing.T) { + t.Parallel() + + store := &alwaysClaimedStore{} + started := time.Now() + _, err := acquireObjectClaimWithin(context.Background(), store, "claimed", 40*time.Millisecond) + require.ErrorIs(t, err, ErrObjectClaimed) + require.Less(t, time.Since(started), 500*time.Millisecond) + require.GreaterOrEqual(t, store.attempts.Load(), int32(1)) +} + +type deadlineClaim struct { + releases *atomic.Int32 +} + +func (c deadlineClaim) Release(ctx context.Context) error { + c.releases.Add(1) + <-ctx.Done() + return ctx.Err() +} + +type immediateClaim struct { + releases *atomic.Int32 +} + +func (c immediateClaim) Release(context.Context) error { + c.releases.Add(1) + return nil +} + +func TestRetentionClaimCleanupDoesNotSerializeBehindSlowRelease(t *testing.T) { + t.Parallel() + + var fastReleases atomic.Int32 + var slowReleases atomic.Int32 + plan := retentionPlan{payloadClaims: []claimedPayload{ + {claim: deadlineClaim{releases: &slowReleases}}, + {claim: immediateClaim{releases: &fastReleases}}, + {claim: immediateClaim{releases: &fastReleases}}, + }} + done := make(chan error, 1) + go func() { + done <- releaseRetentionClaimsWithin(context.Background(), plan, 200*time.Millisecond) + }() + require.Eventually(t, func() bool { return fastReleases.Load() == 2 }, 100*time.Millisecond, time.Millisecond) + require.Error(t, <-done) + require.Equal(t, int32(1), slowReleases.Load()) +} + +func TestRetentionClaimCleanupUsesOneDeadline(t *testing.T) { + t.Parallel() + + var releases atomic.Int32 + plan := retentionPlan{ + manifestClaims: map[string]claimedManifest{ + "m1": {claim: deadlineClaim{releases: &releases}}, + "m2": {claim: deadlineClaim{releases: &releases}}, + }, + payloadClaims: []claimedPayload{ + {claim: deadlineClaim{releases: &releases}}, + }, + } + started := time.Now() + err := releaseRetentionClaimsWithin(context.Background(), plan, 40*time.Millisecond) + require.Error(t, err) + require.Less(t, time.Since(started), 500*time.Millisecond) + require.Equal(t, int32(3), releases.Load()) +} diff --git a/internal/snapshotoffload/manifest.go b/internal/snapshotoffload/manifest.go index ca30a7686..a215a3c7a 100644 --- a/internal/snapshotoffload/manifest.go +++ b/internal/snapshotoffload/manifest.go @@ -22,6 +22,14 @@ var ( ErrIntegrity = errors.New("snapshot offload: integrity check failed") ErrObjectConflict = errors.New("snapshot offload: object conflict") ErrObjectNotFound = errors.New("snapshot offload: object not found") + ErrObjectClaimed = errors.New("snapshot offload: object claimed by another operation") + + // ErrObjectModified is returned by DeleteObjectIfUnmodified when + // the object changed after the caller validated it. For retention + // this is not a failure: it means a concurrent publish changed the + // target object, so the correct response is to leave it alone. The + // target may be either a payload or a manifest. + ErrObjectModified = errors.New("snapshot offload: object modified since validation") // ErrNoPersistedSnapshot reports that the LOCAL data dir has no // persisted snapshot yet. It is deliberately distinct from @@ -118,6 +126,8 @@ func validateManifestIdentity(manifest Manifest) error { return errors.Wrapf(ErrInvalidOptions, "unsupported manifest schema version %d", manifest.SchemaVersion) case manifest.GroupID == 0 && strings.TrimSpace(manifest.SourceCluster) == "": return errors.Wrap(ErrInvalidOptions, "source cluster is required for group 0 manifests") + case manifest.CreatedAt.IsZero(): + return errors.Wrap(ErrInvalidOptions, "manifest creation time is required") case manifest.SnapshotIndex == 0: return errors.Wrap(ErrInvalidOptions, "snapshot index must be > 0") case manifest.SnapshotTerm == 0: diff --git a/internal/snapshotoffload/offload_test.go b/internal/snapshotoffload/offload_test.go index a8942cadd..5189fee75 100644 --- a/internal/snapshotoffload/offload_test.go +++ b/internal/snapshotoffload/offload_test.go @@ -11,6 +11,7 @@ import ( "time" etcdraftengine "github.com/bootjp/elastickv/internal/raftengine/etcd" + "github.com/cockroachdb/errors" "github.com/stretchr/testify/require" ) @@ -183,6 +184,15 @@ func TestPublishReusesExistingObjectsWithoutHeadChecksum(t *testing.T) { require.NoError(t, err) require.Equal(t, first.ManifestKey, second.ManifestKey) require.Equal(t, first.Payload.Key, second.Payload.Key) + require.Equal(t, opts.CreatedAt.Add(time.Nanosecond), second.CreatedAt) + require.NotEqual(t, first.ManifestSHA256, second.ManifestSHA256) + + third, err := PublishPersistedSnapshot(ctx, opts) + require.NoError(t, err) + require.Equal(t, first.ManifestKey, third.ManifestKey) + require.Equal(t, first.Payload.Key, third.Payload.Key) + require.Equal(t, opts.CreatedAt.Add(2*time.Nanosecond), third.CreatedAt) + require.NotEqual(t, second.ManifestSHA256, third.ManifestSHA256) } func TestPublishReusesExistingManifestWhenCreatedAtOmitted(t *testing.T) { @@ -201,12 +211,13 @@ func TestPublishReusesExistingManifestWhenCreatedAtOmitted(t *testing.T) { first, err := PublishPersistedSnapshot(ctx, opts) require.NoError(t, err) + firstCreatedAt := first.CreatedAt time.Sleep(time.Millisecond) second, err := PublishPersistedSnapshot(ctx, opts) require.NoError(t, err) require.Equal(t, first.ManifestKey, second.ManifestKey) - require.Equal(t, first.ManifestSHA256, second.ManifestSHA256) - require.Equal(t, first.CreatedAt, second.CreatedAt) + require.Equal(t, firstCreatedAt.Add(time.Nanosecond), second.CreatedAt) + require.NotEqual(t, first.ManifestSHA256, second.ManifestSHA256) } func TestPutManifestReusesExistingManifestAfterCreateConflict(t *testing.T) { @@ -243,9 +254,27 @@ func TestPutManifestReusesExistingManifestAfterCreateConflict(t *testing.T) { candidate := existing candidate.CreatedAt = time.Unix(401, 0).UTC() + lost := errors.New("leadership lost during create conflict") + checks := 0 + racingAfterDemotion := &headMissOnceStore{ObjectStore: store, key: key} + err = putManifest(ctx, racingAfterDemotion, &candidate, true, func(context.Context) error { + checks++ + if checks > 1 { + return lost + } + return nil + }) + require.ErrorIs(t, err, lost) + require.Equal(t, 2, checks) + unchanged, err := LoadManifest(ctx, store, key) + require.NoError(t, err) + require.Equal(t, existing.CreatedAt, unchanged.CreatedAt) + + candidate = existing + candidate.CreatedAt = time.Unix(401, 0).UTC() racingStore := &headMissOnceStore{ObjectStore: store, key: key} require.NoError(t, putManifest(ctx, racingStore, &candidate, true, nil)) - require.Equal(t, existing.CreatedAt, candidate.CreatedAt) + require.Equal(t, existing.CreatedAt.Add(time.Nanosecond), candidate.CreatedAt) require.NotEmpty(t, candidate.ManifestSHA256) } @@ -620,6 +649,7 @@ func newTestLocalStore(t *testing.T, root string) *LocalStore { t.Helper() store, err := NewLocalStore(root) require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, store.Close()) }) return store } @@ -640,6 +670,20 @@ type countingObjectStore struct { getObjectCalls int } +func (s *countingObjectStore) AcquireObjectClaim(ctx context.Context, key string) (ObjectClaim, error) { + claimStore, err := objectClaimStore(s.ObjectStore) + if err != nil { + return nil, err + } + return claimStore.AcquireObjectClaim(ctx, key) +} + +func (s *countingObjectStore) RefreshObject( + ctx context.Context, key string, body io.Reader, opts PutOptions, +) (ObjectInfo, error) { + return refreshWrappedObject(ctx, s.ObjectStore, key, body, opts) +} + func (s *countingObjectStore) GetObject(ctx context.Context, key string) (io.ReadCloser, ObjectInfo, error) { s.getObjectCalls++ return s.ObjectStore.GetObject(ctx, key) @@ -651,6 +695,20 @@ type headMissOnceStore struct { miss bool } +func (s *headMissOnceStore) AcquireObjectClaim(ctx context.Context, key string) (ObjectClaim, error) { + claimStore, err := objectClaimStore(s.ObjectStore) + if err != nil { + return nil, err + } + return claimStore.AcquireObjectClaim(ctx, key) +} + +func (s *headMissOnceStore) RefreshObject( + ctx context.Context, key string, body io.Reader, opts PutOptions, +) (ObjectInfo, error) { + return refreshWrappedObject(ctx, s.ObjectStore, key, body, opts) +} + func (s *headMissOnceStore) HeadObject(ctx context.Context, key string) (ObjectInfo, bool, error) { if !s.miss && normalizeObjectKey(key) == normalizeObjectKey(s.key) { s.miss = true @@ -672,6 +730,30 @@ type headOrderingStore struct { calls []string } +func (s *headOrderingStore) AcquireObjectClaim(ctx context.Context, key string) (ObjectClaim, error) { + claimStore, err := objectClaimStore(s.ObjectStore) + if err != nil { + return nil, err + } + return claimStore.AcquireObjectClaim(ctx, key) +} + +func (s *headOrderingStore) RefreshObject( + ctx context.Context, key string, body io.Reader, opts PutOptions, +) (ObjectInfo, error) { + return refreshWrappedObject(ctx, s.ObjectStore, key, body, opts) +} + +func refreshWrappedObject( + ctx context.Context, store ObjectStore, key string, body io.Reader, opts PutOptions, +) (ObjectInfo, error) { + refresher, ok := store.(ObjectRefresher) + if !ok { + return ObjectInfo{}, errors.Wrap(ErrInvalidOptions, "wrapped object store cannot refresh objects") + } + return refresher.RefreshObject(ctx, key, body, opts) +} + func (s *headOrderingStore) HeadObject(ctx context.Context, key string) (ObjectInfo, bool, error) { if key == s.manifestKey { s.calls = append(s.calls, "head-manifest") diff --git a/internal/snapshotoffload/publish.go b/internal/snapshotoffload/publish.go index d53914c0f..cfede7ffd 100644 --- a/internal/snapshotoffload/publish.go +++ b/internal/snapshotoffload/publish.go @@ -17,7 +17,7 @@ import ( ) type PublishOptions struct { - Store ObjectStore + Store PublishStore DataDir string Prefix string GroupID uint64 @@ -45,7 +45,7 @@ type PublishOptions struct { SkipIfNotNewerThan uint64 } -func PublishPersistedSnapshot(ctx context.Context, opts PublishOptions) (*Manifest, error) { +func PublishPersistedSnapshot(ctx context.Context, opts PublishOptions) (result *Manifest, retErr error) { if err := validatePublishOptions(opts); err != nil { return nil, err } @@ -56,9 +56,8 @@ func PublishPersistedSnapshot(ctx context.Context, opts PublishOptions) (*Manife defer func() { _ = export.Close() }() metadata := export.Metadata() - if opts.SkipIfNotNewerThan > 0 && metadata.Index <= opts.SkipIfNotNewerThan { - return nil, errors.Wrapf(ErrSnapshotNotNewer, - "persisted snapshot index %d is not newer than %d", metadata.Index, opts.SkipIfNotNewerThan) + if err := rejectSnapshotNotNewer(metadata.Index, opts.SkipIfNotNewerThan); err != nil { + return nil, err } payloadFile, payloadSHA, payloadBytes, err := spoolExport(ctx, export, publishSpoolDir(opts)) if err != nil { @@ -75,12 +74,27 @@ func PublishPersistedSnapshot(ctx context.Context, opts PublishOptions) (*Manife if err != nil { return nil, err } + payloadClaim, err := acquireObjectClaimWaiting(ctx, opts.Store, payloadObjectKey) + if err != nil { + return nil, errors.Wrap(err, "claim snapshot payload") + } + defer func() { + retErr = errors.CombineErrors(retErr, releaseObjectClaim(ctx, payloadClaim)) + }() if err := putPayload(ctx, opts.Store, payloadObjectKey, payloadFile, payloadBytes, payloadSHA); err != nil { return nil, err } return commitManifest(ctx, opts, metadata, payloadObjectKey, payloadSHA) } +func rejectSnapshotNotNewer(index uint64, threshold uint64) error { + if threshold > 0 && index <= threshold { + return errors.Wrapf(ErrSnapshotNotNewer, + "persisted snapshot index %d is not newer than %d", index, threshold) + } + return nil +} + // commitManifest builds, validates and commits the manifest once the payload is // durable. Split out of PublishPersistedSnapshot to keep that function inside // the cyclop budget after the leadership re-check landed. @@ -150,21 +164,32 @@ func buildManifest( func putManifest( ctx context.Context, - store ObjectStore, + store PublishStore, manifest *Manifest, reuseExistingCreatedAt bool, verifyLeader func(context.Context) error, -) error { +) (retErr error) { + claim, err := acquireObjectClaimWaiting(ctx, store, manifest.ManifestKey) + if err != nil { + return errors.Wrap(err, "claim snapshot manifest") + } + defer func() { + retErr = errors.CombineErrors(retErr, releaseObjectClaim(ctx, claim)) + }() + data, manifestSHA, err := manifest.MarshalCanonical() if err != nil { return err } size := int64(len(data)) objectSHA := hexSHA256Bytes(data) - if exists, err := verifyExistingManifest(ctx, store, manifest, size, objectSHA, reuseExistingCreatedAt); err != nil { + if exists, err := verifyExistingManifest(ctx, store, manifest, reuseExistingCreatedAt); err != nil { return err } else if exists { - return nil + if err := verifyPublishLeadership(ctx, verifyLeader, "manifest reuse"); err != nil { + return err + } + return refreshExistingManifest(ctx, store, manifest) } // §4: leadership must hold at the instant the manifest is created, // not merely before the absence probe above. That probe is a remote @@ -172,26 +197,65 @@ func putManifest( // so checking before it leaves a window in which a demoted node // still commits a manifest — precisely the guarantee this // scheduler exists to provide. - if verifyLeader != nil { - if err := verifyLeader(ctx); err != nil { - return errors.Wrap(err, "snapshot offload: leadership lost before manifest commit") - } + if err := verifyPublishLeadership(ctx, verifyLeader, "manifest commit"); err != nil { + return err } - if err := createManifestObject(ctx, store, manifest, data, size, objectSHA, reuseExistingCreatedAt); err != nil { + if err := createManifestObject(ctx, store, manifest, data, size, objectSHA, reuseExistingCreatedAt, verifyLeader); err != nil { return err } manifest.ManifestSHA256 = manifestSHA - return verifyCommittedManifest(ctx, store, manifest, size, objectSHA, reuseExistingCreatedAt) + return verifyCommittedManifest(ctx, store, manifest, reuseExistingCreatedAt) +} + +func verifyPublishLeadership(ctx context.Context, verify func(context.Context) error, operation string) error { + if verify == nil { + return nil + } + if err := verify(ctx); err != nil { + return errors.Wrapf(err, "snapshot offload: leadership lost before %s", operation) + } + return nil +} + +// refreshExistingManifest changes the storage-visible state of a reused +// manifest while preserving its canonical bytes. The manifest is small, and +// the refresh makes a retention scan taken before this publish skip the object +// after it acquires the shared claim and revalidates the listed state. +func refreshExistingManifest(ctx context.Context, store PublishStore, manifest *Manifest) error { + // CreatedAt is part of schema v1's canonical self-hash. Advancing the + // stored value produces a distinct object version without adding a field + // that older restore binaries would omit when recomputing that hash. + manifest.CreatedAt = manifest.CreatedAt.Add(time.Nanosecond) + data, manifestSHA, err := manifest.MarshalCanonical() + if err != nil { + return err + } + opts := PutOptions{ + Size: int64(len(data)), + SHA256: hexSHA256Bytes(data), + ContentType: "application/json", + } + info, err := store.RefreshObject(ctx, manifest.ManifestKey, bytes.NewReader(data), opts) + if err != nil { + return errors.Wrap(err, "refresh existing snapshot manifest") + } + if info.Size != opts.Size || (info.SHA256 != "" && info.SHA256 != opts.SHA256) { + return errors.Wrapf(ErrIntegrity, + "manifest object %s remote integrity mismatch after refresh", manifest.ManifestKey) + } + manifest.ManifestSHA256 = manifestSHA + return nil } func createManifestObject( ctx context.Context, - store ObjectStore, + store PublishStore, manifest *Manifest, data []byte, size int64, objectSHA string, reuseExistingCreatedAt bool, + verifyLeader func(context.Context) error, ) error { info, err := store.PutObject(ctx, manifest.ManifestKey, bytes.NewReader(data), PutOptions{ Size: size, @@ -199,7 +263,7 @@ func createManifestObject( ContentType: "application/json", }) if err != nil { - return handleManifestPutError(ctx, store, manifest, size, objectSHA, reuseExistingCreatedAt, err) + return handleManifestPutError(ctx, store, manifest, reuseExistingCreatedAt, verifyLeader, err) } if info.Size != size || (info.SHA256 != "" && info.SHA256 != objectSHA) { return errors.Wrapf(ErrIntegrity, "manifest object %s remote integrity mismatch", manifest.ManifestKey) @@ -209,20 +273,22 @@ func createManifestObject( func handleManifestPutError( ctx context.Context, - store ObjectStore, + store PublishStore, manifest *Manifest, - size int64, - objectSHA string, reuseExistingCreatedAt bool, + verifyLeader func(context.Context) error, err error, ) error { if !errors.Is(err, ErrIntegrity) { return errors.Wrap(err, "put snapshot manifest") } - if exists, verifyErr := verifyExistingManifest(ctx, store, manifest, size, objectSHA, reuseExistingCreatedAt); verifyErr != nil { + if exists, verifyErr := verifyExistingManifest(ctx, store, manifest, reuseExistingCreatedAt); verifyErr != nil { return errors.Wrap(verifyErr, "verify conflicting snapshot manifest") } else if exists { - return nil + if err := verifyPublishLeadership(ctx, verifyLeader, "conflicting manifest reuse"); err != nil { + return err + } + return refreshExistingManifest(ctx, store, manifest) } return errors.Wrap(err, "put snapshot manifest") } @@ -231,11 +297,9 @@ func verifyCommittedManifest( ctx context.Context, store ObjectStore, manifest *Manifest, - size int64, - objectSHA string, reuseExistingCreatedAt bool, ) error { - if exists, err := verifyExistingManifest(ctx, store, manifest, size, objectSHA, reuseExistingCreatedAt); err != nil { + if exists, err := verifyExistingManifest(ctx, store, manifest, reuseExistingCreatedAt); err != nil { return errors.Wrap(err, "verify committed snapshot manifest") } else if !exists { return errors.Wrapf(ErrIntegrity, "manifest object %s missing after put", manifest.ManifestKey) @@ -247,26 +311,19 @@ func verifyExistingManifest( ctx context.Context, store ObjectStore, manifest *Manifest, - size int64, - sha string, reuseExistingCreatedAt bool, ) (bool, error) { - info, ok, err := store.HeadObject(ctx, manifest.ManifestKey) + _, ok, err := store.HeadObject(ctx, manifest.ManifestKey) if err != nil { return false, errors.Wrap(err, "head existing snapshot manifest") } if !ok { return false, nil } - if info.Size != size && !reuseExistingCreatedAt { - return true, errors.Wrapf(ErrIntegrity, "manifest object %s already exists with different size", manifest.ManifestKey) - } - if info.SHA256 != "" && info.SHA256 != sha && !reuseExistingCreatedAt { - return true, errors.Wrapf(ErrIntegrity, "manifest object %s already exists with different sha256", manifest.ManifestKey) - } existing, err := LoadManifest(ctx, store, manifest.ManifestKey) if err != nil { - return true, errors.Wrap(err, "load existing snapshot manifest") + return true, errors.Wrapf(ErrIntegrity, + "load existing snapshot manifest %s: %v", manifest.ManifestKey, err) } if !manifestMatchesCandidate(existing, *manifest, reuseExistingCreatedAt) { return true, errors.Wrapf(ErrIntegrity, "manifest object %s already exists with different content", manifest.ManifestKey) @@ -279,8 +336,19 @@ func manifestMatchesCandidate(existing Manifest, candidate Manifest, reuseExisti if reuseExistingCreatedAt { return sameManifestExceptCreation(existing, candidate) } + originalCreatedAt := candidate.CreatedAt candidate.ManifestSHA256 = existing.ManifestSHA256 - return reflect.DeepEqual(existing, candidate) + if reflect.DeepEqual(existing, candidate) { + return true + } + // Refreshing a reused schema-v1 manifest advances CreatedAt by one + // nanosecond so the canonical bytes and self-hash form a new generation + // without adding a field older restore binaries cannot verify. A later + // retry with the caller's original explicit timestamp must still match + // that refreshed object. Only accept a forward-advanced stored timestamp; + // a candidate attempting to replace the audit timestamp with a newer value + // remains a conflict. + return existing.CreatedAt.After(originalCreatedAt) && sameManifestExceptCreation(existing, candidate) } // sameManifestExceptCreation compares a retry's candidate against the @@ -359,23 +427,41 @@ func spoolExport(ctx context.Context, export *etcdraftengine.PersistedSnapshotEx } func putPayload(ctx context.Context, store ObjectStore, key string, file *os.File, size int64, sha string) error { - if exists, err := verifyExistingStoreObject(ctx, store, key, size, sha); err != nil { + opts := PutOptions{ + Size: size, + SHA256: sha, + ContentType: "application/octet-stream", + } + exists, err := verifyExistingStoreObject(ctx, store, key, size, sha) + if err != nil { return errors.Wrap(err, "verify existing snapshot payload") - } else if exists { + } + if exists { + // The caller holds the payload's storage-visible claim until the + // manifest commits. Rewriting a multi-terabyte content-addressed + // payload merely to move its mtime would defeat deduplication; the + // shared claim is the lightweight coordination primitive instead. return nil } - if _, err := file.Seek(0, io.SeekStart); err != nil { - return errors.WithStack(err) + if err := seekPayloadFile(file); err != nil { + return err } - info, err := store.PutObject(ctx, key, file, PutOptions{ - Size: size, - SHA256: sha, - ContentType: "application/octet-stream", - }) + info, err := store.PutObject(ctx, key, file, opts) if err != nil { return errors.Wrap(err, "put snapshot payload") } - if info.Size != size || (info.SHA256 != "" && info.SHA256 != sha) { + return validatePayloadObjectInfo(key, info, opts) +} + +func seekPayloadFile(file *os.File) error { + if _, err := file.Seek(0, io.SeekStart); err != nil { + return errors.WithStack(err) + } + return nil +} + +func validatePayloadObjectInfo(key string, info ObjectInfo, opts PutOptions) error { + if info.Size != opts.Size || (info.SHA256 != "" && info.SHA256 != opts.SHA256) { return errors.Wrapf(ErrIntegrity, "payload object %s remote integrity mismatch", key) } return nil diff --git a/internal/snapshotoffload/restore_corruption_test.go b/internal/snapshotoffload/restore_corruption_test.go new file mode 100644 index 000000000..e773645e4 --- /dev/null +++ b/internal/snapshotoffload/restore_corruption_test.go @@ -0,0 +1,191 @@ +package snapshotoffload + +import ( + "bytes" + "context" + "os" + "path/filepath" + "testing" + + "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" +) + +// The M3 corruption drills. Each plants one specific defect in a +// published artifact and asserts restore fails closed AND leaves the +// destination data dir absent — a half-restored dir is worse than no +// restore, because the node would come up serving truncated state. + +// publishForRestoreDrill publishes a snapshot and returns the store and +// its manifest. +func publishForRestoreDrill(t *testing.T, root string, payload []byte) (*LocalStore, Manifest) { + t.Helper() + sourceDataDir := seedPhysicalSnapshot(t, root, payload, 21, 5, singlePeer()) + store := newTestLocalStore(t, filepath.Join(root, "objects")) + manifest, err := PublishPersistedSnapshot(context.Background(), PublishOptions{ + Store: store, + DataDir: sourceDataDir, + Prefix: "cluster-a", + GroupID: 1, + SourceCluster: "cluster-a", + }) + require.NoError(t, err) + return store, *manifest +} + +// requireRestoreFailsClosed runs a restore and asserts it failed and +// left no data dir behind. +func requireRestoreFailsClosed(t *testing.T, store *LocalStore, manifestKey, dataDir string) error { + t.Helper() + _, err := RestorePhysicalSnapshot(context.Background(), RestoreOptions{ + Store: store, + ManifestKey: manifestKey, + DataDir: dataDir, + Peers: singlePeer(), + }) + require.Error(t, err) + _, statErr := os.Stat(dataDir) + require.True(t, os.IsNotExist(statErr), + "a failed restore must leave the destination absent, not half-written") + return err +} + +// TestRestoreRejectsTruncatedPayload covers a partial upload or a +// truncating filesystem: the bytes hash differently AND are short. The +// length check must fire before any content is trusted. +func TestRestoreRejectsTruncatedPayload(t *testing.T) { + t.Parallel() + + root := t.TempDir() + payload := []byte("EKVTHLC1a-payload-long-enough-to-truncate-meaningfully") + store, manifest := publishForRestoreDrill(t, root, payload) + + payloadPath, err := store.pathForKey(manifest.Payload.Key) + require.NoError(t, err) + require.NoError(t, os.WriteFile(payloadPath, payload[:len(payload)/2], 0o600)) + + err = requireRestoreFailsClosed(t, store, manifest.ManifestKey, filepath.Join(root, "restored")) + require.True(t, errors.Is(err, ErrIntegrity)) +} + +// TestRestoreRejectsPayloadGrownBeyondItsDeclaredLength is the +// complement: extra bytes appended to the object. +func TestRestoreRejectsPayloadGrownBeyondItsDeclaredLength(t *testing.T) { + t.Parallel() + + root := t.TempDir() + payload := []byte("EKVTHLC1a-payload-to-be-extended") + store, manifest := publishForRestoreDrill(t, root, payload) + + payloadPath, err := store.pathForKey(manifest.Payload.Key) + require.NoError(t, err) + require.NoError(t, os.WriteFile(payloadPath, append(payload, []byte("extra")...), 0o600)) + + err = requireRestoreFailsClosed(t, store, manifest.ManifestKey, filepath.Join(root, "restored")) + require.True(t, errors.Is(err, ErrIntegrity)) +} + +// TestRestoreRejectsAManifestWhosePayloadIsMissing is the dangling +// reference case — precisely the state a retention bug would leave +// behind if it reclaimed a payload a committed manifest still names. +// Restore must fail cleanly rather than produce an empty data dir. +func TestRestoreRejectsAManifestWhosePayloadIsMissing(t *testing.T) { + t.Parallel() + + root := t.TempDir() + payload := []byte("EKVTHLC1payload-that-gets-reclaimed") + store, manifest := publishForRestoreDrill(t, root, payload) + + require.NoError(t, store.DeleteObject(context.Background(), manifest.Payload.Key)) + + err := requireRestoreFailsClosed(t, store, manifest.ManifestKey, filepath.Join(root, "restored")) + require.True(t, errors.Is(err, ErrObjectNotFound), + "a manifest naming an absent payload must report the payload as missing") +} + +// TestRestoreRejectsAManifestWithATamperedPayloadDescriptor covers a +// manifest edited to name a different payload SHA, in both shapes an +// attacker can produce. +// +// Leaving the self-hash stale is caught by the manifest's own +// integrity check. Refreshing the self-hash defeats that check but is +// still caught downstream, because the payload OBJECT KEY encodes the +// content hash: a descriptor claiming a different SHA points at a key +// whose bytes cannot hash to it. Both must fail closed. +func TestRestoreRejectsAManifestWithATamperedPayloadDescriptor(t *testing.T) { + t.Parallel() + + falseSHA := hexSHA256Bytes([]byte("a completely different payload")) + + tests := []struct { + name string + refreshSelfHash bool + }{ + {name: "stale self hash", refreshSelfHash: false}, + {name: "self hash refreshed to match the edit", refreshSelfHash: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + payload := []byte("EKVTHLC1payload-with-an-edited-descriptor") + store, manifest := publishForRestoreDrill(t, root, payload) + + tampered := manifest + tampered.Payload.SHA256 = falseSHA + encoded, freshSum, err := tampered.MarshalCanonical() + require.NoError(t, err) + if !tc.refreshSelfHash { + require.NotEqual(t, manifest.ManifestSHA256, freshSum) + stale := bytes.Replace(encoded, + []byte(freshSum), []byte(manifest.ManifestSHA256), 1) + require.NotEqual(t, encoded, stale) + encoded = stale + } + manifestPath, err := store.pathForKey(manifest.ManifestKey) + require.NoError(t, err) + require.NoError(t, os.WriteFile(manifestPath, encoded, 0o600)) + + err = requireRestoreFailsClosed(t, store, manifest.ManifestKey, + filepath.Join(root, "restored")) + require.True(t, errors.Is(err, ErrIntegrity), + "an edited payload descriptor must fail the integrity contract, got %v", err) + }) + } +} + +// TestRestoreDrillSucceedsIntoAFreshDirectory is the positive drill: +// the artifact published above restores cleanly into an absent data +// dir, which is the operation the corruption cases must not +// half-perform. +func TestRestoreDrillSucceedsIntoAFreshDirectory(t *testing.T) { + t.Parallel() + + root := t.TempDir() + payload := []byte("EKVTHLC1a-healthy-payload-for-the-drill") + store, manifest := publishForRestoreDrill(t, root, payload) + + restoreDataDir := filepath.Join(root, "restored") + result, err := RestorePhysicalSnapshot(context.Background(), RestoreOptions{ + Store: store, + ManifestKey: manifest.ManifestKey, + DataDir: restoreDataDir, + Peers: singlePeer(), + }) + require.NoError(t, err) + require.NotNil(t, result) + require.DirExists(t, restoreDataDir) + + // A second restore into the now-populated dir must refuse rather + // than overwrite: the preflight is what protects an operator who + // re-runs the drill against a live node's data dir. + _, err = RestorePhysicalSnapshot(context.Background(), RestoreOptions{ + Store: store, + ManifestKey: manifest.ManifestKey, + DataDir: restoreDataDir, + Peers: singlePeer(), + }) + require.Error(t, err, "restore must refuse a destination that already exists") +} diff --git a/internal/snapshotoffload/retention.go b/internal/snapshotoffload/retention.go new file mode 100644 index 000000000..3705e0cbb --- /dev/null +++ b/internal/snapshotoffload/retention.go @@ -0,0 +1,996 @@ +package snapshotoffload + +import ( + "context" + "log/slog" + "path" + "sort" + "strings" + "sync" + "time" + + "github.com/cockroachdb/errors" +) + +// Design §5 retention/GC. +// +// GC is deliberately two-phase and asymmetric in what it trusts: +// +// Phase 1 deletes MANIFEST objects that fall outside the policy. +// Phase 2 deletes PAYLOAD objects that no surviving manifest names. +// +// The asymmetry exists because payloads are content-addressed and +// therefore SHARED: two groups (or two generations within a group) +// that snapshot identical bytes converge on one payload object. A +// payload may only be reclaimed once the live set has been rebuilt +// from EVERY surviving manifest in the whole prefix — not just the +// group being trimmed. Getting this wrong deletes live data, so every +// ambiguity below resolves toward keeping the object. +const ( + // DefaultMinGenerations is the floor on manifests kept per group, + // independent of age. Restores are rare and usually urgent, so the + // default keeps a fallback generation when the newest snapshot + // turns out to be unusable. + DefaultMinGenerations = 3 + + // DefaultMaxAge keeps every manifest published inside this window + // even when that exceeds MinGenerations. + DefaultMaxAge = 14 * 24 * time.Hour + + // DefaultMinMarkAge is the default two-pass sweep delay. It + // matches DefaultPayloadGrace because both bound the same thing: + // the longest plausible time between a publisher touching a + // payload and its manifest committing. + DefaultMinMarkAge = 24 * time.Hour + + // DefaultPayloadGrace is the §5 phase-2 delay before an + // unreferenced payload may be reclaimed. It covers the window in + // which a payload has been uploaded (payload-first publish) but + // its manifest has not yet committed: during that window the + // payload is legitimately unreferenced and must NOT be collected. + // The grace period must therefore exceed the longest plausible + // publish duration. + DefaultPayloadGrace = 24 * time.Hour + + payloadKeyRelativeParts = 2 +) + +// RetentionPolicy is the per-prefix retention rule. +// +// A manifest survives phase 1 if it is within MinGenerations newest +// for its group OR younger than MaxAge. The newest valid manifest for +// a group is always retained regardless of both, per §9's acceptance +// criterion that retention never deletes a group's newest manifest. +type RetentionPolicy struct { + MinGenerations int + MaxAge time.Duration + PayloadGrace time.Duration + // MinMarkAge is the §5 two-pass sweep delay: a payload must have + // been marked for at least this long, and must not have changed + // since the mark, before it may be reclaimed. + // + // It exists because no conditional-delete primitive on a + // general-purpose S3 bucket can detect a content-preserving + // refresh: `If-Match` compares a content-derived ETag, which a + // republish of identical bytes leaves untouched, and + // `IfMatchLastModifiedTime` is directory-buckets only. Comparing + // the object's observed state ACROSS passes is what detects that + // refresh, so it must exceed the longest plausible publish. + MinMarkAge time.Duration +} + +func (p RetentionPolicy) withDefaults() RetentionPolicy { + if p.MinGenerations <= 0 { + p.MinGenerations = DefaultMinGenerations + } + if p.MaxAge <= 0 { + p.MaxAge = DefaultMaxAge + } + if p.PayloadGrace <= 0 { + p.PayloadGrace = DefaultPayloadGrace + } + if p.MinMarkAge <= 0 { + p.MinMarkAge = DefaultMinMarkAge + } + return p +} + +// GCResult reports what one RunOnce did and, just as importantly, what +// it refused to do. PayloadPhaseSkipped with a non-empty +// MalformedManifests is the expected shape when the bucket contains a +// manifest this build cannot parse. +type GCResult struct { + GroupsScanned int + ManifestsScanned int + ManifestsDeleted []string + PayloadsDeleted []string + MalformedManifests []string + PayloadPhaseSkipped bool + // SkipReason explains PayloadPhaseSkipped in operator-facing + // terms. Empty when the phase ran. + SkipReason string + // ManifestsClaimedConcurrently counts expired manifests whose + // compare-and-delete lost to a concurrent publish rewriting the + // same key (an idempotent republish of the same index/term). + ManifestsClaimedConcurrently int + // PayloadsAwaitingSweep counts payloads that are eligible for + // reclamation but are still serving the §5 two-pass sweep delay. + // A steady non-zero value on a busy prefix is normal. + PayloadsAwaitingSweep int + // PayloadsClaimedConcurrently counts payloads that were eligible + // for reclamation but whose compare-and-delete lost to a + // concurrent publish reusing them. A steady non-zero value is + // healthy dedup traffic, not an error. + PayloadsClaimedConcurrently int +} + +// GC implements the §5 retention and payload-reclamation passes over +// one object-store prefix. +type GC struct { + store RetentionStore + prefix string + policy RetentionPolicy + now func() time.Time + log *slog.Logger + + // marks is the §5 two-pass sweep state: the first pass that finds + // a payload unreferenced and past grace records what it saw, and + // only a LATER pass that finds the same object unchanged may + // delete it. A publisher that refreshes the payload anywhere + // between the two passes changes its mtime, which invalidates the + // mark and spares the object — the coordination that a + // content-derived ETag precondition cannot provide. + // + // The state is in-memory and per-process. Losing it on restart is + // safe in the only direction that matters: reclamation is delayed + // by one more pass, never advanced. + marksMu sync.Mutex + marks map[string]payloadMark +} + +// payloadMark records the state of a payload when it was first seen +// eligible for reclamation. +type payloadMark struct { + at time.Time + size int64 + updatedAt time.Time +} + +// matches reports whether ref is the same object state that was +// marked. Any difference means the object was rewritten since — the +// signal that a publisher reused it. +func (m payloadMark) matches(ref ObjectRef) bool { + return m.size == ref.Size && m.updatedAt.Equal(ref.UpdatedAt) +} + +// GCOptions configures NewGC. +type GCOptions struct { + Store RetentionStore + Prefix string + Policy RetentionPolicy + // Now defaults to time.Now. Injected so tests can drive the age + // windows deterministically rather than by sleeping. + Now func() time.Time + Logger *slog.Logger +} + +func NewGC(opts GCOptions) (*GC, error) { + if opts.Store == nil { + return nil, errors.Wrap(ErrInvalidOptions, "retention requires an object store") + } + now := opts.Now + if now == nil { + now = time.Now + } + log := opts.Logger + if log == nil { + log = slog.Default() + } + return &GC{ + store: opts.Store, + prefix: cleanObjectPrefix(opts.Prefix), + policy: opts.Policy.withDefaults(), + now: now, + log: log, + marks: make(map[string]payloadMark), + }, nil +} + +// RunOnce executes both phases. It returns an error only when the scan +// itself failed; a scan that completes but declines to reclaim +// payloads returns a nil error and a GCResult with +// PayloadPhaseSkipped set. +func (g *GC) RunOnce(ctx context.Context) (result GCResult, retErr error) { + if g == nil { + return GCResult{}, errors.Wrap(ErrInvalidOptions, "gc is required") + } + + scan, err := g.scanManifests(ctx) + if err != nil { + // §5: listing or pagination failure performs no deletes. + return GCResult{}, err + } + + result = GCResult{ + GroupsScanned: len(scan.byGroup), + ManifestsScanned: scan.scanned, + MalformedManifests: scan.malformed, + } + + var plan retentionPlan + defer func() { + retErr = errors.CombineErrors(retErr, releaseRetentionClaims(ctx, plan)) + }() + plan, err = g.prepareRetentionPlan(ctx, scan) + if err != nil { + return result, err + } + scan = plan.scan + result.GroupsScanned = len(scan.byGroup) + result.ManifestsScanned = scan.scanned + result.MalformedManifests = scan.malformed + result.PayloadsClaimedConcurrently = plan.payloadsClaimedConcurrently + result.PayloadsAwaitingSweep = plan.awaitingSweep + survivors, expired := g.partition(scan) + live := livePayloadKeys(survivors) + result.ManifestsDeleted, result.ManifestsClaimedConcurrently, err = + g.deleteExpiredManifests(ctx, expired, plan.manifestClaims, live) + if err != nil { + return result, err + } + + if plan.skipReason != "" { + result.PayloadPhaseSkipped = true + result.SkipReason = plan.skipReason + g.log.Warn("snapshot offload payload reclamation skipped", + "reason", plan.skipReason, + "malformed_manifests", len(scan.malformed)) + return result, nil + } + + deleted, claimed, err := g.reclaimPayloads(ctx, live, plan.payloadClaims, plan.refs) + result.PayloadsDeleted = deleted + result.PayloadsClaimedConcurrently += claimed + if err != nil { + return result, err + } + return result, nil +} + +type retentionPlan struct { + scan manifestScan + refs []ObjectRef + payloadClaims []claimedPayload + manifestClaims map[string]claimedManifest + payloadsClaimedConcurrently int + awaitingSweep int + skipReason string +} + +type claimedManifest struct { + ref ObjectRef + manifestSHA256 string + claim ObjectClaim +} + +func (g *GC) prepareRetentionPlan(ctx context.Context, scan manifestScan) (retentionPlan, error) { + initialMalformed := append([]string(nil), scan.malformed...) + plan := retentionPlan{ + scan: scan, + skipReason: g.payloadPhaseBlockedBy(scan), + manifestClaims: make(map[string]claimedManifest), + } + if plan.skipReason == "" { + survivors, _ := g.partition(scan) + live := livePayloadKeys(survivors) + refs, err := g.listPayloadObjects(ctx) + if err != nil { + return plan, err + } + plan.refs = refs + plan.payloadClaims, plan.payloadsClaimedConcurrently, plan.awaitingSweep, err = + g.claimSweepablePayloads(ctx, live, refs) + if err != nil { + return plan, err + } + } + + _, expired := g.partition(scan) + manifestClaims, err := g.claimExpiredManifests(ctx, expired) + plan.manifestClaims = manifestClaims + if err != nil { + return plan, err + } + + // This is the final fallible manifest scan for the pass. It must + // complete BEFORE phase-one deletes. Payload and manifest claims remain + // held through the scan and deletion, closing both publish-versus-sweep + // races. A manifest that changed before its claim was acquired is also + // skipped by comparing this scan with the initial ref. + plan.scan, err = g.scanManifests(ctx) + if err != nil { + return plan, err + } + plan.scan.malformed = unionSortedStrings(initialMalformed, plan.scan.malformed) + if finalSkipReason := g.payloadPhaseBlockedBy(plan.scan); finalSkipReason != "" { + plan.skipReason = finalSkipReason + } + return plan, nil +} + +func (g *GC) claimExpiredManifests( + ctx context.Context, expired []scannedManifest, +) (map[string]claimedManifest, error) { + claims := make(map[string]claimedManifest, len(expired)) + for _, entry := range expired { + claim, err := g.store.AcquireObjectClaim(ctx, entry.key) + if err != nil { + if errors.Is(err, ErrObjectClaimed) { + continue + } + return claims, errors.Wrapf(err, "retention: claim manifest %s", entry.key) + } + claims[entry.key] = claimedManifest{ + ref: entry.ref, + manifestSHA256: entry.manifest.ManifestSHA256, + claim: claim, + } + } + return claims, nil +} + +func protectManifestPayload(live map[string]struct{}, entry scannedManifest) { + live[normalizeObjectKey(entry.manifest.Payload.Key)] = struct{}{} +} + +func (g *GC) deleteExpiredManifests( + ctx context.Context, + expired []scannedManifest, + claims map[string]claimedManifest, + live map[string]struct{}, +) ([]string, int, error) { + var deletedKeys []string + claimedConcurrently := 0 + for _, entry := range expired { + claimed, ok := claims[entry.key] + if !ok || !sameObjectState(claimed.ref, entry.ref) || + claimed.manifestSHA256 != entry.manifest.ManifestSHA256 { + protectManifestPayload(live, entry) + claimedConcurrently++ + continue + } + deleted, err := g.deleteClaimedManifest(ctx, entry) + if err != nil { + return deletedKeys, claimedConcurrently, err + } + if !deleted { + protectManifestPayload(live, entry) + claimedConcurrently++ + continue + } + deletedKeys = append(deletedKeys, entry.key) + } + return deletedKeys, claimedConcurrently, nil +} + +// manifestScan is the phase-1 view of the prefix. +type manifestScan struct { + byGroup map[uint64][]scannedManifest + malformed []string + scanned int +} + +type scannedManifest struct { + key string + // ref is the object state observed when the manifest was listed. + // It is the compare-and-delete precondition for phase 1, for the + // same reason phase 2 needs one: an idempotent publish retry can + // rewrite this exact key between the retention decision and the + // delete, and an unconditional delete would then remove a + // manifest the publisher believes it just committed. + ref ObjectRef + manifest Manifest + // createdAt is the manifest's own CreatedAt. The object store's + // mtime is deliberately NOT used here: a bucket copy or a + // lifecycle transition rewrites mtime and would silently reset + // every manifest's apparent age. + createdAt time.Time +} + +type malformedManifestError struct { + err error +} + +func (e *malformedManifestError) Error() string { + return e.err.Error() +} + +func (e *malformedManifestError) Unwrap() error { + return e.err +} + +func malformedManifest(err error) error { + return &malformedManifestError{err: err} +} + +func isMalformedManifest(err error) bool { + var target *malformedManifestError + return errors.As(err, &target) +} + +func unionSortedStrings(left, right []string) []string { + seen := make(map[string]struct{}, len(left)+len(right)) + for _, value := range left { + seen[value] = struct{}{} + } + for _, value := range right { + seen[value] = struct{}{} + } + result := make([]string, 0, len(seen)) + for value := range seen { + result = append(result, value) + } + sort.Strings(result) + return result +} + +func (g *GC) scanManifests(ctx context.Context) (manifestScan, error) { + groupsPrefix := path.Join(g.prefix, "v1", "groups") + refs, err := g.store.ListObjects(ctx, groupsPrefix) + if err != nil { + return manifestScan{}, errors.Wrap(err, "retention: list manifests") + } + + scan := manifestScan{byGroup: make(map[uint64][]scannedManifest)} + seen := make(map[string]struct{}, len(refs)) + for _, ref := range refs { + if !strings.HasSuffix(ref.Key, manifestObjectSuffix) { + continue + } + // A store that returns the same key twice — overlapping pages + // from an S3-compatible endpoint while objects change — would + // otherwise be counted as two generations of the same + // manifest. With MinGenerations 1 one copy lands in survivors + // and the other in expired, so phase 1 would delete the very + // key chosen as the group's newest restore point. + if _, dup := seen[ref.Key]; dup { + continue + } + seen[ref.Key] = struct{}{} + scan.scanned++ + manifest, err := g.loadManifest(ctx, ref) + if err != nil { + if !isMalformedManifest(err) { + return manifestScan{}, errors.Wrapf(err, "retention: load manifest %s", ref.Key) + } + // §5: malformed manifests are reported and excluded from + // deletion. They also block payload reclamation entirely + // (see payloadPhaseBlockedBy) because an unparseable + // manifest may reference a payload we cannot enumerate. + scan.malformed = append(scan.malformed, ref.Key) + g.log.Error("snapshot offload retention found malformed manifest", + "manifest_key", ref.Key, "error", err) + continue + } + scan.byGroup[manifest.GroupID] = append(scan.byGroup[manifest.GroupID], scannedManifest{ + key: ref.Key, + ref: ref, + manifest: manifest, + createdAt: manifest.CreatedAt, + }) + } + return scan, nil +} + +func (g *GC) loadManifest(ctx context.Context, ref ObjectRef) (Manifest, error) { + if ref.Size > maxManifestBytes { + return Manifest{}, malformedManifest(errors.Wrapf(ErrInvalidOptions, + "manifest %s exceeds %d bytes", ref.Key, maxManifestBytes)) + } + body, _, err := g.store.GetObject(ctx, ref.Key) + if err != nil { + return Manifest{}, errors.Wrapf(err, "get manifest %s", ref.Key) + } + defer func() { _ = body.Close() }() + data, err := readLimitedManifest(ctx, body) + if err != nil { + if errors.Is(err, ErrInvalidOptions) { + return Manifest{}, malformedManifest(errors.Wrapf(err, "read manifest %s", ref.Key)) + } + return Manifest{}, errors.Wrapf(err, "read manifest %s", ref.Key) + } + manifest, err := DecodeManifest(data) + if err != nil { + return Manifest{}, malformedManifest(errors.Wrapf(err, "decode manifest %s", ref.Key)) + } + if err := verifyManifestPaths(g.prefix, ref, manifest); err != nil { + return Manifest{}, err + } + return manifest, nil +} + +// verifyManifestPaths rejects a manifest whose recorded keys disagree with the +// canonical paths its own body implies. +// +// Split out of loadManifest so the three checks read as one rule rather than +// accumulating inline branches. +func verifyManifestPaths(prefix string, ref ObjectRef, manifest Manifest) error { + if normalizeObjectKey(ref.Key) != normalizeObjectKey(manifest.ManifestKey) { + return malformedManifest(errors.Wrapf(ErrIntegrity, + "manifest key mismatch: listed %s, body says %s", ref.Key, manifest.ManifestKey)) + } + // Self-consistency is not enough: a body may agree with its own + // ManifestKey while its group/index/term disagree with the path it + // is stored under. Retention groups and orders by the BODY, so a + // high-index body claiming group 2 parked under a group-1 path + // would consume group 2's retained-generation slots and get its + // real newest manifests deleted. Re-derive the canonical key and + // treat any disagreement as malformed. + canonical, err := manifestKey(prefix, manifest.GroupID, manifest.SnapshotIndex, manifest.SnapshotTerm) + if err != nil { + return malformedManifest(errors.Wrapf(err, "derive canonical key for %s", ref.Key)) + } + if normalizeObjectKey(ref.Key) != normalizeObjectKey(canonical) { + return malformedManifest(errors.Wrapf(ErrIntegrity, + "manifest %s is stored off its canonical path %s (group=%d index=%d term=%d)", + ref.Key, canonical, manifest.GroupID, manifest.SnapshotIndex, manifest.SnapshotTerm)) + } + // The payload reference has to be canonical for THIS prefix too. A + // manifest under prefix A naming a payload under prefix B records the + // exact B key in A's live set, but A's scan never lists B -- and B's + // scan cannot see A's manifest, so B eventually reclaims a payload A + // still needs and A's restore breaks. Re-deriving the key makes a + // cross-prefix reference malformed instead of a latent data loss. + canonicalPayload, err := payloadKey(prefix, manifest.Payload.SHA256) + if err != nil { + return malformedManifest(errors.Wrapf(err, "derive canonical payload key for %s", ref.Key)) + } + if normalizeObjectKey(manifest.Payload.Key) != normalizeObjectKey(canonicalPayload) { + return malformedManifest(errors.Wrapf(ErrIntegrity, + "manifest %s references payload %s outside its own prefix (canonical %s)", + ref.Key, manifest.Payload.Key, canonicalPayload)) + } + return nil +} + +// partition splits every scanned manifest into survivors and the keys +// phase 1 may delete. +func (g *GC) partition(scan manifestScan) ([]scannedManifest, []scannedManifest) { + var ( + survivors []scannedManifest + expired []scannedManifest + ) + cutoff := g.now().Add(-g.policy.MaxAge) + + for _, manifests := range scan.byGroup { + // Newest first, so index < MinGenerations is "recent". + sorted := append([]scannedManifest(nil), manifests...) + sort.Slice(sorted, func(i, j int) bool { + if sorted[i].manifest.SnapshotIndex != sorted[j].manifest.SnapshotIndex { + return sorted[i].manifest.SnapshotIndex > sorted[j].manifest.SnapshotIndex + } + return sorted[i].manifest.SnapshotTerm > sorted[j].manifest.SnapshotTerm + }) + for i, entry := range sorted { + if g.retains(i, entry, cutoff) { + survivors = append(survivors, entry) + continue + } + expired = append(expired, entry) + } + } + sort.Slice(expired, func(i, j int) bool { return expired[i].key < expired[j].key }) + return survivors, expired +} + +// retains decides one manifest's fate. Index 0 is the group's newest +// valid manifest and is retained unconditionally. +// +// That first branch is deliberately NOT folded into the +// MinGenerations check even though withDefaults currently clamps +// MinGenerations to >= 1, which makes the two overlap today. §9 makes +// "never delete a group's newest manifest" a standalone safety +// property, and resting it on a policy floor means a later change +// that permits MinGenerations == 0 (say, an age-only policy) would +// silently make the last restore point deletable. Stating the +// invariant here keeps it true regardless of the policy. +func (g *GC) retains(index int, entry scannedManifest, cutoff time.Time) bool { + switch { + case index == 0: + return true + case index < g.policy.MinGenerations: + return true + case entry.createdAt.After(cutoff): + return true + default: + return false + } +} + +// payloadPhaseBlockedBy returns a non-empty reason when phase 2 must +// not run. Every branch here is a case where the live payload set +// cannot be proven complete, and deleting on an incomplete live set +// destroys referenced data. +func (g *GC) payloadPhaseBlockedBy(scan manifestScan) string { + if len(scan.malformed) > 0 { + return "malformed manifests present; live payload set cannot be proven complete" + } + return "" +} + +func (g *GC) listPayloadObjects(ctx context.Context) ([]ObjectRef, error) { + payloadsPrefix := path.Join(g.prefix, "v1", "payloads") + refs, err := g.store.ListObjects(ctx, payloadsPrefix) + if err != nil { + return nil, errors.Wrap(err, "retention: list payloads") + } + return refs, nil +} + +func livePayloadKeys(survivors []scannedManifest) map[string]struct{} { + live := make(map[string]struct{}, len(survivors)) + for _, entry := range survivors { + live[normalizeObjectKey(entry.manifest.Payload.Key)] = struct{}{} + } + return live +} + +type claimedPayload struct { + ref ObjectRef + sha string + claim ObjectClaim +} + +// claimSweepablePayloads takes claims for every payload this pass may delete. +// Claims are acquired before the final manifest scan and held through delete, +// so a publisher cannot complete inside the scan-to-delete window. +func (g *GC) claimSweepablePayloads( + ctx context.Context, + live map[string]struct{}, + refs []ObjectRef, +) ([]claimedPayload, int, int, error) { + graceCutoff := g.now().Add(-g.policy.PayloadGrace) + var ( + claims []claimedPayload + claimed int + marked int + ) + for _, ref := range refs { + sha, ok := payloadSHAFromKey(g.prefix, ref.Key) + if !ok { + g.log.Warn("snapshot offload retention skipped unrecognized payload object", + "object_key", ref.Key) + continue + } + if payloadKeyIsLive(live, ref.Key) || !beforeGraceCutoff(ref.UpdatedAt, graceCutoff) { + g.dropMark(ref.Key) + continue + } + if !g.sweepable(ref) { + marked++ + continue + } + claim, err := g.store.AcquireObjectClaim(ctx, ref.Key) + if err != nil { + if errors.Is(err, ErrObjectClaimed) { + claimed++ + continue + } + return claims, claimed, marked, errors.Wrapf(err, "retention: claim payload %s", ref.Key) + } + claims = append(claims, claimedPayload{ref: ref, sha: sha, claim: claim}) + } + return claims, claimed, marked, nil +} + +// reclaimPayloads is §5 phase 2. Every entry in claims has been locked since +// before the authoritative manifest scan used to build live. +func (g *GC) reclaimPayloads( + ctx context.Context, + live map[string]struct{}, + claims []claimedPayload, + refs []ObjectRef, +) ([]string, int, error) { + graceCutoff := g.now().Add(-g.policy.PayloadGrace) + var ( + deleted []string + claimed int + ) + for _, candidate := range claims { + if payloadKeyIsLive(live, candidate.ref.Key) { + g.dropMark(candidate.ref.Key) + continue + } + key, ok, err := g.reclaimClaimedPayload(ctx, candidate.ref, candidate.sha, graceCutoff) + if err != nil { + if errors.Is(err, errObjectClaimed) { + claimed++ + continue + } + return deleted, claimed, err + } + if !ok { + continue + } + deleted = append(deleted, key) + } + sort.Strings(deleted) + // Drop marks for payloads no longer in the listing. Without this, + // a payload removed by another GC process or a bucket lifecycle + // rule never passes through reclaimPayload again, so no dropMark + // call can reach it and its mark leaks for the process's lifetime. + g.pruneMarks(refs) + return deleted, claimed, nil +} + +func releaseRetentionClaims(ctx context.Context, plan retentionPlan) error { + return releaseRetentionClaimsWithin(ctx, plan, claimReleaseTimeout) +} + +func releaseRetentionClaimsWithin(ctx context.Context, plan retentionPlan, maxWait time.Duration) error { + releaseCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), maxWait) + defer cancel() + + claims := make([]ObjectClaim, 0, len(plan.manifestClaims)+len(plan.payloadClaims)) + for _, candidate := range plan.manifestClaims { + claims = append(claims, candidate.claim) + } + for _, candidate := range plan.payloadClaims { + claims = append(claims, candidate.claim) + } + if len(claims) == 0 { + return nil + } + + jobs := make(chan ObjectClaim, len(claims)) + errs := make(chan error, len(claims)) + for _, claim := range claims { + jobs <- claim + } + close(jobs) + + var workers sync.WaitGroup + for range min(len(claims), claimReleaseConcurrency) { + workers.Add(1) + go func() { + defer workers.Done() + for claim := range jobs { + errs <- releaseObjectClaimWithContext(releaseCtx, claim) + } + }() + } + workers.Wait() + close(errs) + + var releaseErr error + for err := range errs { + releaseErr = errors.CombineErrors(releaseErr, err) + } + if releaseErr != nil { + return errors.Wrap(releaseErr, "release retention claims") + } + return nil +} + +// pruneMarks discards marks whose object was absent from the listing +// this pass. refs is the COMPLETE payload listing (ListObjects is +// all-or-error), so absence is authoritative. +func (g *GC) pruneMarks(refs []ObjectRef) { + present := make(map[string]struct{}, len(refs)) + for _, ref := range refs { + present[ref.Key] = struct{}{} + } + + g.marksMu.Lock() + defer g.marksMu.Unlock() + for key := range g.marks { + if _, ok := present[key]; !ok { + delete(g.marks, key) + } + } +} + +func (g *GC) reclaimClaimedPayload( + ctx context.Context, + ref ObjectRef, + sha string, + graceCutoff time.Time, +) (string, bool, error) { + info, exists, err := g.store.HeadObject(ctx, ref.Key) + if err != nil { + return "", false, errors.Wrapf(err, "retention: head payload %s", ref.Key) + } + if !exists || !beforeGraceCutoff(info.UpdatedAt, graceCutoff) { + g.dropMark(ref.Key) + return "", false, nil + } + if err := g.compareAndDeletePayload(ctx, refreshed(ref, info), sha); err != nil { + return "", false, err + } + g.dropMark(ref.Key) + g.log.Info("snapshot offload retention reclaimed payload", + "object_key", ref.Key, "sha256", sha) + return ref.Key, true, nil +} + +// sweepable implements the §5 two-pass rule. It reports true only when +// this payload was marked on an earlier pass, has not changed since, +// and the mark has aged past MinMarkAge. Otherwise it (re-)marks the +// object and reports false. +// +// The DELAY is the protection: a publisher that refreshes a reused +// payload and then commits its manifest completes well inside one +// inter-pass interval, so the sweep pass sees the refreshed mtime (via +// the grace check) or the new manifest (via the live set) and spares +// the object. A single-pass GC had no such window. +// +// The state comparison is a secondary consistency check rather than +// the primary mechanism: because a refresh sets mtime to now, the +// grace check already rejects a refreshed object on its own. Keeping +// the comparison makes a mark mean "this exact object state has been +// quiet", so a mark can never be honoured for bytes that changed +// underneath it — including a size change that left mtime untouched, +// which the grace check alone would miss. +func (g *GC) sweepable(ref ObjectRef) bool { + now := g.now() + + g.marksMu.Lock() + defer g.marksMu.Unlock() + + mark, marked := g.marks[ref.Key] + if !marked || !mark.matches(ref) { + if marked { + g.log.Info("snapshot offload retention re-marked a payload that changed since the last pass", + "object_key", ref.Key) + } + g.marks[ref.Key] = payloadMark{at: now, size: ref.Size, updatedAt: ref.UpdatedAt} + return false + } + return now.Sub(mark.at) >= g.policy.MinMarkAge +} + +func (g *GC) dropMark(key string) { + g.marksMu.Lock() + defer g.marksMu.Unlock() + delete(g.marks, key) +} + +// MarkedPayloads returns the payload keys currently held under a sweep +// mark. Exposed for operator tooling and tests; the set is per-process +// and advisory. +func (g *GC) MarkedPayloads() []string { + g.marksMu.Lock() + defer g.marksMu.Unlock() + keys := make([]string, 0, len(g.marks)) + for key := range g.marks { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} + +func (g *GC) deleteClaimedManifest(ctx context.Context, entry scannedManifest) (bool, error) { + info, exists, err := g.store.HeadObject(ctx, entry.key) + if err != nil { + return false, errors.Wrapf(err, "retention: head manifest %s", entry.key) + } + if !exists { + return false, nil + } + current := refreshed(entry.ref, info) + if !sameObjectState(entry.ref, current) { + g.log.Info("snapshot offload retention skipped manifest rewritten by a concurrent publish", + "manifest_key", entry.key) + return false, nil + } + err = g.store.DeleteObjectIfUnmodified(ctx, entry.key, PreconditionFor(current)) + switch { + case err == nil: + g.log.Info("snapshot offload retention deleted manifest", + "manifest_key", entry.key) + return true, nil + case errors.Is(err, ErrObjectModified): + g.log.Info("snapshot offload retention skipped manifest rewritten by a concurrent publish", + "manifest_key", entry.key) + return false, nil + default: + return false, errors.Wrapf(err, "retention: delete manifest %s", entry.key) + } +} + +func sameObjectState(want, got ObjectRef) bool { + if want.Size != got.Size { + return false + } + if !want.UpdatedAt.IsZero() && !got.UpdatedAt.Equal(want.UpdatedAt) { + return false + } + if want.ETag != "" && got.ETag != want.ETag { + return false + } + return true +} + +// compareAndDeletePayload deletes the payload only if it still matches +// the state reclaimPayload validated. +// +// An unconditional delete here would still lose the race with a +// publisher that reuses this payload: it can refresh the object and +// commit a manifest between the validating head and this call, leaving +// a committed manifest pointing at deleted bytes. +func (g *GC) compareAndDeletePayload(ctx context.Context, ref ObjectRef, sha string) error { + err := g.store.DeleteObjectIfUnmodified(ctx, ref.Key, PreconditionFor(ref)) + switch { + case err == nil: + return nil + case errors.Is(err, ErrObjectModified): + // A concurrent publish claimed the payload. Not an error: + // leaving it alone is the correct outcome, and the next pass + // reclaims it if it really is garbage. + g.log.Info("snapshot offload retention skipped payload claimed by a concurrent publish", + "object_key", ref.Key, "sha256", sha) + return errObjectClaimed + default: + return errors.Wrapf(err, "retention: delete payload %s", ref.Key) + } +} + +// errObjectClaimed marks the benign "a publisher took this payload +// back" outcome so the caller can count it without treating it as a +// failure. It never escapes reclaimPayloads. +var errObjectClaimed = ErrObjectClaimed + +// refreshed merges the freshly-headed state into the listed ref so the +// delete precondition describes what was actually validated, not the +// possibly-staler listing. +func refreshed(ref ObjectRef, info ObjectInfo) ObjectRef { + ref.Size = info.Size + if !info.UpdatedAt.IsZero() { + ref.UpdatedAt = info.UpdatedAt + } + if info.ETag != "" { + ref.ETag = info.ETag + } + return ref +} + +func payloadKeyIsLive(live map[string]struct{}, key string) bool { + _, ok := live[normalizeObjectKey(key)] + return ok +} + +func beforeGraceCutoff(updatedAt, graceCutoff time.Time) bool { + return !updatedAt.IsZero() && updatedAt.Before(graceCutoff) +} + +// payloadSHAFromKey recovers the content hash from a payload object +// key laid out as /v1/payloads/sha256//. +// It verifies the two-character shard directory matches the hash so a +// hand-placed object cannot masquerade as a payload. +func payloadSHAFromKey(prefix, key string) (string, bool) { + normalized := normalizeObjectKey(key) + if normalized != key { + return "", false + } + payloadsPrefix := path.Join(cleanObjectPrefix(prefix), "v1", "payloads", "sha256") + rel, ok := strings.CutPrefix(normalized, payloadsPrefix+"/") + if !ok { + return "", false + } + parts := strings.Split(rel, "/") + if len(parts) != payloadKeyRelativeParts { + return "", false + } + base := parts[1] + if !strings.HasSuffix(base, payloadObjectSuffix) { + return "", false + } + sha := strings.TrimSuffix(base, payloadObjectSuffix) + if !isSHA256Hex(sha) { + return "", false + } + if shard := parts[0]; shard != sha[:2] { + return "", false + } + return sha, true +} diff --git a/internal/snapshotoffload/retention_test.go b/internal/snapshotoffload/retention_test.go new file mode 100644 index 000000000..778e1a397 --- /dev/null +++ b/internal/snapshotoffload/retention_test.go @@ -0,0 +1,2008 @@ +package snapshotoffload + +import ( + "bytes" + "context" + "fmt" + "io" + "os" + "path" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" +) + +const retentionPrefix = "cluster-a" + +// gcFixture is a LocalStore-backed prefix plus a controllable clock. +type gcFixture struct { + root string + store *LocalStore + now time.Time +} + +func newGCFixture(t *testing.T) *gcFixture { + t.Helper() + root := t.TempDir() + store := newTestLocalStore(t, root) + return &gcFixture{ + root: root, + store: store, + now: time.Unix(1_700_000_000, 0).UTC(), + } +} + +func (f *gcFixture) gc(t *testing.T, policy RetentionPolicy) *GC { + t.Helper() + if policy.MinMarkAge <= 0 { + policy.MinMarkAge = time.Hour + } + gc, err := NewGC(GCOptions{ + Store: f.store, + Prefix: retentionPrefix, + Policy: policy, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + return gc +} + +// sweep runs the two passes the §5 mark-and-sweep requires: the first +// trims manifests and MARKS eligible payloads, then the clock advances +// past MinMarkAge and the second reclaims them. +// +// Manifest trimming is single-pass (phase 1) and payload reclamation +// is two-pass, so both results are returned: assert manifests on the +// first, payloads on the second. +func (f *gcFixture) sweep(t *testing.T, gc *GC) (GCResult, GCResult) { + t.Helper() + first, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.Empty(t, first.PayloadsDeleted, + "the first pass must only mark payloads, never delete them") + + f.now = f.now.Add(2 * time.Hour) + second, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + return first, second +} + +// publishManifest writes a manifest object and (unless the payload is +// shared with an earlier manifest) its payload object, mirroring the +// real payload-first publish layout. +func (f *gcFixture) publishManifest( + t *testing.T, groupID, index uint64, payload []byte, age time.Duration, +) Manifest { + t.Helper() + ctx := context.Background() + // Term is fixed: retention orders by snapshot index and never + // inspects the term beyond tie-breaking equal indexes. + const term = uint64(2) + + payloadSHA := hexSHA256Bytes(payload) + pKey, err := payloadKey(retentionPrefix, payloadSHA) + require.NoError(t, err) + _, err = f.store.PutObject(ctx, pKey, bytes.NewReader(payload), PutOptions{ + Size: int64(len(payload)), + SHA256: payloadSHA, + }) + require.NoError(t, err) + f.setMTime(t, pKey, f.now.Add(-age)) + + mKey, err := manifestKey(retentionPrefix, groupID, index, term) + require.NoError(t, err) + manifest := Manifest{ + SchemaVersion: ManifestSchemaVersion, + CreatedAt: f.now.Add(-age), + SourceCluster: retentionPrefix, + GroupID: groupID, + SnapshotIndex: index, + SnapshotTerm: term, + ConfState: ManifestConfState{Voters: []uint64{1}}, + Payload: PayloadDescriptor{ + Key: pKey, + Bytes: int64(len(payload)), + SHA256: payloadSHA, + }, + ManifestKey: mKey, + } + encoded, sum, err := manifest.MarshalCanonical() + require.NoError(t, err) + manifest.ManifestSHA256 = sum + _, err = f.store.PutObject(ctx, mKey, bytes.NewReader(encoded), PutOptions{ + Size: int64(len(encoded)), + SHA256: hexSHA256Bytes(encoded), + }) + require.NoError(t, err) + f.setMTime(t, mKey, f.now.Add(-age)) + return manifest +} + +func (f *gcFixture) writeManifest(t *testing.T, manifest Manifest, age time.Duration) { + t.Helper() + encoded, sum, err := manifest.MarshalCanonical() + require.NoError(t, err) + manifest.ManifestSHA256 = sum + _, err = f.store.PutObject(context.Background(), manifest.ManifestKey, bytes.NewReader(encoded), PutOptions{ + Size: int64(len(encoded)), + SHA256: hexSHA256Bytes(encoded), + ContentType: "application/json", + }) + require.NoError(t, err) + f.setMTime(t, manifest.ManifestKey, f.now.Add(-age)) +} + +// writeMalformedManifest plants an object that lands in the manifest +// keyspace but does not decode. +func (f *gcFixture) writeMalformedManifest(t *testing.T, groupID, index, term uint64) string { + t.Helper() + key, err := manifestKey(retentionPrefix, groupID, index, term) + require.NoError(t, err) + body := []byte("{ not a manifest") + _, err = f.store.PutObject(context.Background(), key, bytes.NewReader(body), PutOptions{ + Size: int64(len(body)), + SHA256: hexSHA256Bytes(body), + }) + require.NoError(t, err) + return key +} + +func (f *gcFixture) setMTime(t *testing.T, key string, at time.Time) { + t.Helper() + require.NoError(t, os.Chtimes(filepath.Join(f.root, filepath.FromSlash(key)), at, at)) +} + +func (f *gcFixture) exists(t *testing.T, key string) bool { + t.Helper() + _, err := os.Stat(filepath.Join(f.root, filepath.FromSlash(key))) + if err == nil { + return true + } + require.True(t, os.IsNotExist(err), "unexpected stat error: %v", err) + return false +} + +// TestGCAlwaysRetainsNewestManifestPerGroup pins the §9 acceptance +// criterion: even with the most aggressive policy and an ancient +// snapshot, a group's newest manifest is never deleted. Losing it +// would leave the group with no restore point at all. +func TestGCAlwaysRetainsNewestManifestPerGroup(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + old := f.publishManifest(t, 1, 10, []byte("ancient"), 365*24*time.Hour) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.Empty(t, result.ManifestsDeleted) + require.True(t, f.exists(t, old.ManifestKey)) + require.True(t, f.exists(t, old.Payload.Key), + "the payload of a retained manifest must never be reclaimed") +} + +func TestGCRetainsMinGenerationsAndDeletesBeyondBothBounds(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + m1 := f.publishManifest(t, 1, 10, []byte("gen-1"), year) + m2 := f.publishManifest(t, 1, 20, []byte("gen-2"), year) + m3 := f.publishManifest(t, 1, 30, []byte("gen-3"), year) + m4 := f.publishManifest(t, 1, 40, []byte("gen-4"), year) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 2, MaxAge: time.Hour, PayloadGrace: time.Hour}) + trim, reclaim := f.sweep(t, gc) + + // Newest two survive; the two older ones are beyond MinGenerations + // and beyond MaxAge, so both go. + require.True(t, f.exists(t, m4.ManifestKey)) + require.True(t, f.exists(t, m3.ManifestKey)) + require.False(t, f.exists(t, m2.ManifestKey)) + require.False(t, f.exists(t, m1.ManifestKey)) + require.Len(t, trim.ManifestsDeleted, 2) + + // Their payloads are unreferenced and past grace, so phase 2 takes them. + require.False(t, f.exists(t, m1.Payload.Key)) + require.False(t, f.exists(t, m2.Payload.Key)) + require.True(t, f.exists(t, m3.Payload.Key)) + require.True(t, f.exists(t, m4.Payload.Key)) + require.Len(t, reclaim.PayloadsDeleted, 2) +} + +func TestGCRetainsManifestsInsideMaxAgeBeyondMinGenerations(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + recent := 2 * time.Hour + m1 := f.publishManifest(t, 1, 10, []byte("a"), recent) + m2 := f.publishManifest(t, 1, 20, []byte("b"), recent) + m3 := f.publishManifest(t, 1, 30, []byte("c"), recent) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: 24 * time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.Empty(t, result.ManifestsDeleted) + for _, m := range []Manifest{m1, m2, m3} { + require.True(t, f.exists(t, m.ManifestKey)) + } +} + +// TestGCNeverReclaimsPayloadSharedWithAnotherGroup is the test that +// matters most. Payloads are content-addressed, so two groups that +// snapshot identical bytes converge on ONE object. Trimming group 1's +// manifest must not delete a payload group 2 still references — +// rebuilding the live set per-group instead of prefix-wide would +// silently destroy group 2's only restore point. +func TestGCNeverReclaimsPayloadSharedWithAnotherGroup(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + shared := []byte("identical-snapshot-bytes") + + // Group 1: an old generation using the shared payload, plus a + // newer one so the old is eligible for deletion. + g1old := f.publishManifest(t, 1, 10, shared, year) + g1new := f.publishManifest(t, 1, 20, []byte("group-1-newer"), year) + // Group 2 has only one manifest, and it references the SAME payload. + g2 := f.publishManifest(t, 2, 5, shared, year) + + require.Equal(t, g1old.Payload.Key, g2.Payload.Key, "fixture must exercise a shared payload") + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + // Group 1's old manifest is trimmed... + require.False(t, f.exists(t, g1old.ManifestKey)) + require.True(t, f.exists(t, g1new.ManifestKey)) + require.True(t, f.exists(t, g2.ManifestKey)) + // ...but the shared payload survives because group 2 still names it. + require.True(t, f.exists(t, g2.Payload.Key), + "a payload referenced by another group's manifest must never be reclaimed") + require.NotContains(t, result.PayloadsDeleted, g2.Payload.Key) +} + +// TestGCSkipsPayloadPhaseWhenAManifestIsMalformed pins the §5 +// fail-closed rule. An unparseable manifest means the live payload set +// cannot be proven complete, so phase 2 must not run at all — and the +// malformed object itself must not be deleted. +func TestGCSkipsPayloadPhaseWhenAManifestIsMalformed(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + live := f.publishManifest(t, 1, 30, []byte("live"), year) + orphan := f.publishManifest(t, 1, 10, []byte("orphaned-payload"), year) + badKey := f.writeMalformedManifest(t, 2, 7, 1) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err, "a malformed manifest is reported, not an error") + + require.True(t, result.PayloadPhaseSkipped) + require.Contains(t, result.SkipReason, "malformed") + require.Equal(t, []string{badKey}, result.MalformedManifests) + require.Empty(t, result.PayloadsDeleted) + + // The malformed manifest survives for operator inspection. + require.True(t, f.exists(t, badKey)) + // The orphaned payload is NOT reclaimed despite being unreferenced. + require.True(t, f.exists(t, orphan.Payload.Key), + "phase 2 must not run while the live set is unprovable") + require.True(t, f.exists(t, live.Payload.Key)) +} + +type malformedRemovedBeforeFinalScanStore struct { + RetentionStore + triggerKey string + malformedKey string + done bool +} + +func (s *malformedRemovedBeforeFinalScanStore) AcquireObjectClaim( + ctx context.Context, key string, +) (ObjectClaim, error) { + if key == s.triggerKey && !s.done { + s.done = true + if err := s.DeleteObject(ctx, s.malformedKey); err != nil { + return nil, err + } + } + return s.RetentionStore.AcquireObjectClaim(ctx, key) +} + +func TestGCPreservesMalformedManifestEvidenceFromTheInitialScan(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("live"), year) + stale := f.publishManifest(t, 1, 10, []byte("stale"), year) + badKey := f.writeMalformedManifest(t, 2, 7, 1) + store := &malformedRemovedBeforeFinalScanStore{ + RetentionStore: f.store, + triggerKey: stale.ManifestKey, + malformedKey: badKey, + } + gc, err := NewGC(GCOptions{ + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.False(t, f.exists(t, badKey), "the fixture must remove the malformed object before the final scan") + require.True(t, result.PayloadPhaseSkipped) + require.Contains(t, result.SkipReason, "malformed") + require.Equal(t, []string{badKey}, result.MalformedManifests, + "operator evidence from the initial scan must survive a later disappearance") +} + +// TestGCDoesNotReclaimPayloadInsideGracePeriod covers the +// payload-first publish window: the payload lands before its manifest +// commits, so a freshly uploaded unreferenced payload is an in-flight +// publish, not garbage. +func TestGCDoesNotReclaimPayloadInsideGracePeriod(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + keep := f.publishManifest(t, 1, 30, []byte("referenced"), 365*24*time.Hour) + + // An unreferenced payload uploaded one minute ago. + inflight := []byte("payload-of-an-in-flight-publish") + sha := hexSHA256Bytes(inflight) + key, err := payloadKey(retentionPrefix, sha) + require.NoError(t, err) + _, err = f.store.PutObject(context.Background(), key, bytes.NewReader(inflight), PutOptions{ + Size: int64(len(inflight)), + SHA256: sha, + }) + require.NoError(t, err) + f.setMTime(t, key, f.now.Add(-time.Minute)) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.Empty(t, result.PayloadsDeleted) + require.True(t, f.exists(t, key), "a payload inside the grace window is an in-flight publish") + require.True(t, f.exists(t, keep.Payload.Key)) +} + +func TestPutPayloadReusesExistingObjectWithoutReupload(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + ctx := context.Background() + payload := []byte("payload-reused-by-a-new-manifest") + sha := hexSHA256Bytes(payload) + key, err := payloadKey(retentionPrefix, sha) + require.NoError(t, err) + _, err = f.store.PutObject(ctx, key, bytes.NewReader(payload), PutOptions{ + Size: int64(len(payload)), + SHA256: sha, + }) + require.NoError(t, err) + oldMTime := f.now.Add(-365 * 24 * time.Hour) + f.setMTime(t, key, oldMTime) + + file, err := os.CreateTemp(t.TempDir(), "payload-*.fsm") + require.NoError(t, err) + defer func() { require.NoError(t, file.Close()) }() + _, err = file.Write(payload) + require.NoError(t, err) + + require.NoError(t, putPayload(ctx, f.store, key, file, int64(len(payload)), sha)) + info, ok, err := f.store.HeadObject(ctx, key) + require.NoError(t, err) + require.True(t, ok) + require.True(t, info.UpdatedAt.Equal(oldMTime), + "the shared claim, not a multi-terabyte rewrite, coordinates payload reuse with GC") +} + +func TestGCRevalidatesPayloadReferenceCommittedAfterInitialPayloadList(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + ctx := context.Background() + payload := []byte("orphan-that-becomes-referenced") + sha := hexSHA256Bytes(payload) + key, err := payloadKey(retentionPrefix, sha) + require.NoError(t, err) + _, err = f.store.PutObject(ctx, key, bytes.NewReader(payload), PutOptions{ + Size: int64(len(payload)), + SHA256: sha, + }) + require.NoError(t, err) + f.setMTime(t, key, f.now.Add(-365*24*time.Hour)) + + mKey, err := manifestKey(retentionPrefix, 1, 10, 2) + require.NoError(t, err) + injectedManifest := Manifest{ + SchemaVersion: ManifestSchemaVersion, + CreatedAt: f.now, + SourceCluster: retentionPrefix, + GroupID: 1, + SnapshotIndex: 10, + SnapshotTerm: 2, + ConfState: ManifestConfState{Voters: []uint64{1}}, + Payload: PayloadDescriptor{ + Key: key, + Bytes: int64(len(payload)), + SHA256: sha, + }, + ManifestKey: mKey, + } + store := &manifestDuringPayloadListStore{ + RetentionStore: f.store, + inject: func() { + f.writeManifest(t, injectedManifest, 0) + }, + } + gc, err := NewGC(GCOptions{ + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + result, err := gc.RunOnce(ctx) + require.NoError(t, err) + require.Empty(t, result.PayloadsDeleted) + require.True(t, f.exists(t, key), + "a payload referenced by a manifest committed after the initial scan must survive") +} + +func TestGCLiveSetUsesReferencedPayloadKeys(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + ctx := context.Background() + keySHA := hexSHA256Bytes([]byte("key-hash")) + body := []byte("payload-body-with-a-different-hash") + bodySHA := hexSHA256Bytes(body) + key, err := payloadKey(retentionPrefix, keySHA) + require.NoError(t, err) + _, err = f.store.PutObject(ctx, key, bytes.NewReader(body), PutOptions{ + Size: int64(len(body)), + SHA256: bodySHA, + }) + require.NoError(t, err) + f.setMTime(t, key, f.now.Add(-365*24*time.Hour)) + + mKey, err := manifestKey(retentionPrefix, 1, 10, 2) + require.NoError(t, err) + f.writeManifest(t, Manifest{ + SchemaVersion: ManifestSchemaVersion, + CreatedAt: f.now.Add(-365 * 24 * time.Hour), + SourceCluster: retentionPrefix, + GroupID: 1, + SnapshotIndex: 10, + SnapshotTerm: 2, + ConfState: ManifestConfState{Voters: []uint64{1}}, + Payload: PayloadDescriptor{ + Key: key, + Bytes: int64(len(body)), + SHA256: bodySHA, + }, + ManifestKey: mKey, + }, 365*24*time.Hour) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(ctx) + require.NoError(t, err) + require.Empty(t, result.PayloadsDeleted) + require.True(t, f.exists(t, key), + "retention must keep the exact object key a surviving manifest restores from") +} + +// TestGCPerformsNoDeletesWhenListingFails pins §5's +// "listing failure performs no deletes". +func TestGCPerformsNoDeletesWhenListingFails(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + m1 := f.publishManifest(t, 1, 10, []byte("gen-1"), year) + m2 := f.publishManifest(t, 1, 20, []byte("gen-2"), year) + + failing := &failingListStore{RetentionStore: f.store, failOn: "v1/groups"} + gc, err := NewGC(GCOptions{ + Store: failing, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + _, err = gc.RunOnce(context.Background()) + require.Error(t, err) + require.Zero(t, failing.deletes, "a failed scan must delete nothing") + require.True(t, f.exists(t, m1.ManifestKey)) + require.True(t, f.exists(t, m2.ManifestKey)) +} + +func TestGCPerformsNoDeletesWhenPayloadListingFails(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + m1 := f.publishManifest(t, 1, 10, []byte("gen-1"), year) + m2 := f.publishManifest(t, 1, 20, []byte("gen-2"), year) + + failing := &failingListStore{RetentionStore: f.store, failOn: "v1/payloads"} + gc, err := NewGC(GCOptions{ + Store: failing, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + _, err = gc.RunOnce(context.Background()) + require.Error(t, err) + require.Zero(t, failing.deletes, "payload listing must complete before any destructive delete") + require.True(t, f.exists(t, m1.ManifestKey)) + require.True(t, f.exists(t, m2.ManifestKey)) +} + +func TestGCAbortsOnManifestTransportFailure(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + m1 := f.publishManifest(t, 1, 10, []byte("gen-1"), year) + m2 := f.publishManifest(t, 1, 20, []byte("gen-2"), year) + + failing := &failingGetStore{RetentionStore: f.store, failKey: m1.ManifestKey} + gc, err := NewGC(GCOptions{ + Store: failing, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + _, err = gc.RunOnce(context.Background()) + require.Error(t, err) + require.Zero(t, failing.deletes, "manifest transport failure must abort before deletes") + require.True(t, f.exists(t, m1.ManifestKey)) + require.True(t, f.exists(t, m2.ManifestKey)) +} + +func TestGCTreatsManifestKeyMismatchAsMalformed(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + source := f.publishManifest(t, 1, 10, []byte("payload"), 365*24*time.Hour) + body, _, err := f.store.GetObject(context.Background(), source.ManifestKey) + require.NoError(t, err) + raw, err := io.ReadAll(body) + require.NoError(t, err) + require.NoError(t, body.Close()) + + wrongKey, err := manifestKey(retentionPrefix, 2, 99, 2) + require.NoError(t, err) + _, err = f.store.PutObject(context.Background(), wrongKey, bytes.NewReader(raw), PutOptions{ + Size: int64(len(raw)), + SHA256: hexSHA256Bytes(raw), + ContentType: "application/json", + }) + require.NoError(t, err) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.True(t, result.PayloadPhaseSkipped) + require.Equal(t, []string{wrongKey}, result.MalformedManifests) + require.True(t, f.exists(t, wrongKey)) +} + +// TestGCLeavesUnrecognizedObjectsUnderPayloadPrefix guards against +// reclaiming an object this build does not understand — e.g. one +// written by a future layout version. +func TestGCLeavesUnrecognizedObjectsUnderPayloadPrefix(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + f.publishManifest(t, 1, 30, []byte("referenced"), 365*24*time.Hour) + + stray := "cluster-a/v1/payloads/sha256/ab/not-a-content-hash.fsm" + body := []byte("from a future layout version") + _, err := f.store.PutObject(context.Background(), stray, bytes.NewReader(body), PutOptions{ + Size: int64(len(body)), + SHA256: hexSHA256Bytes(body), + }) + require.NoError(t, err) + f.setMTime(t, stray, f.now.Add(-365*24*time.Hour)) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.Empty(t, result.PayloadsDeleted) + require.True(t, f.exists(t, stray)) +} + +// TestPayloadSHAFromKeyRejectsMismatchedShard stops a hand-placed +// object in the wrong shard directory from being treated as a payload. +func TestPayloadSHAFromKeyRejectsMismatchedShard(t *testing.T) { + t.Parallel() + + sha := hexSHA256Bytes([]byte("payload")) + good, err := payloadKey(retentionPrefix, sha) + require.NoError(t, err) + + got, ok := payloadSHAFromKey(retentionPrefix, good) + require.True(t, ok) + require.Equal(t, sha, got) + + wrongShard := strings.Replace(good, "/"+sha[:2]+"/", "/zz/", 1) + _, ok = payloadSHAFromKey(retentionPrefix, wrongShard) + require.False(t, ok, "shard directory must agree with the content hash") + + _, ok = payloadSHAFromKey(retentionPrefix, "cluster-a/v1/payloads/sha256/ab/short.fsm") + require.False(t, ok) + + _, ok = payloadSHAFromKey(retentionPrefix, "cluster-a/v1/payloads/archive/"+sha[:2]+"/"+sha+payloadObjectSuffix) + require.False(t, ok, "only the canonical payloads/sha256 layout is reclaimable") + + _, ok = payloadSHAFromKey(retentionPrefix, "cluster-b/v1/payloads/sha256/"+sha[:2]+"/"+sha+payloadObjectSuffix) + require.False(t, ok, "payload keys from another prefix must not be reclaimed") +} + +func TestGCOverEmptyPrefixIsACleanNoOp(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + gc := f.gc(t, RetentionPolicy{}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.Zero(t, result.ManifestsScanned) + require.Empty(t, result.ManifestsDeleted) + require.Empty(t, result.PayloadsDeleted) + require.False(t, result.PayloadPhaseSkipped) +} + +func TestNewGCRequiresStore(t *testing.T) { + t.Parallel() + + _, err := NewGC(GCOptions{Prefix: retentionPrefix}) + require.Error(t, err) + require.True(t, errors.Is(err, ErrInvalidOptions)) +} + +func TestRetentionPolicyDefaults(t *testing.T) { + t.Parallel() + + got := RetentionPolicy{}.withDefaults() + require.Equal(t, DefaultMinGenerations, got.MinGenerations) + require.Equal(t, DefaultMaxAge, got.MaxAge) + require.Equal(t, DefaultPayloadGrace, got.PayloadGrace) +} + +// failingListStore fails ListObjects for a chosen prefix and counts +// deletes so a test can assert none happened. +type failingListStore struct { + RetentionStore + failOn string + deletes int +} + +func (s *failingListStore) ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) { + if strings.Contains(prefix, s.failOn) { + return nil, errors.New("object store listing unavailable") + } + return s.RetentionStore.ListObjects(ctx, prefix) +} + +func (s *failingListStore) DeleteObject(ctx context.Context, key string) error { + s.deletes++ + return s.RetentionStore.DeleteObject(ctx, key) +} + +type failingGetStore struct { + RetentionStore + failKey string + deletes int +} + +func (s *failingGetStore) GetObject(ctx context.Context, key string) (io.ReadCloser, ObjectInfo, error) { + if normalizeObjectKey(key) == normalizeObjectKey(s.failKey) { + return nil, ObjectInfo{}, errors.New("manifest transport unavailable") + } + return s.RetentionStore.GetObject(ctx, key) +} + +func (s *failingGetStore) DeleteObject(ctx context.Context, key string) error { + s.deletes++ + return s.RetentionStore.DeleteObject(ctx, key) +} + +type manifestDuringPayloadListStore struct { + RetentionStore + inject func() + injected bool +} + +func (s *manifestDuringPayloadListStore) ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) { + refs, err := s.RetentionStore.ListObjects(ctx, prefix) + if err != nil { + return nil, err + } + if !s.injected && strings.Contains(prefix, "v1/payloads") { + s.injected = true + s.inject() + } + return refs, nil +} + +// TestGCRetainsNewestEvenWhenPolicyWouldNotPins the §9 invariant +// independently of the MinGenerations floor. withDefaults clamps +// MinGenerations to >= 1 today, which makes the newest manifest +// survive as a side effect; this drives retains directly with a +// zero-generation, zero-age policy so the guarantee is pinned by the +// rule itself rather than by the clamp. +func TestGCRetainsNewestEvenWhenPolicyWouldNot(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + // Bypass withDefaults: an age-only policy that keeps no generations. + gc.policy.MinGenerations = 0 + + ancient := scannedManifest{ + key: "cluster-a/v1/groups/1/snapshots/x.json", + createdAt: f.now.Add(-365 * 24 * time.Hour), + } + cutoff := f.now.Add(-time.Hour) + + require.True(t, gc.retains(0, ancient, cutoff), + "a group's newest manifest must survive any policy") + require.False(t, gc.retains(1, ancient, cutoff), + "an older manifest outside both bounds is still collectable") +} + +// TestLocalStoreListObjectsSkipsOnlyTheReservedTempNamespace covers the +// crash-during-publish leftover without hiding legitimate object keys whose +// final component starts with ".put-". +func TestLocalStoreListObjectsSkipsOnlyTheReservedTempNamespace(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + f.publishManifest(t, 1, 10, []byte("payload"), time.Hour) + + leftover := filepath.Join(f.root, localStoreTempDir, ".put-abandoned") + require.NoError(t, os.WriteFile(leftover, []byte("partial upload"), 0o600)) + legitimateKey := path.Join(retentionPrefix, ".put-user-object") + legitimateBody := []byte("committed object") + _, err := f.store.PutObject(context.Background(), legitimateKey, bytes.NewReader(legitimateBody), PutOptions{ + Size: int64(len(legitimateBody)), + SHA256: hexSHA256Bytes(legitimateBody), + }) + require.NoError(t, err) + + refs, err := f.store.ListObjects(context.Background(), ".") + require.NoError(t, err) + + keys := make([]string, 0, len(refs)) + for _, ref := range refs { + keys = append(keys, ref.Key) + } + require.Contains(t, keys, legitimateKey, "a committed .put-* key is an ordinary object") + require.NotContains(t, keys, filepath.ToSlash(filepath.Join(localStoreTempDir, ".put-abandoned"))) + require.FileExists(t, leftover, "listing must not delete the leftover either") + + _, err = f.store.ListObjects(context.Background(), localStoreTempDir) + require.ErrorIs(t, err, ErrInvalidOptions) + _, err = f.store.PutObject(context.Background(), path.Join(localStoreTempDir, "object"), bytes.NewReader(nil), PutOptions{ + SHA256: hexSHA256Bytes(nil), + }) + require.ErrorIs(t, err, ErrInvalidOptions) +} + +func TestLocalStoreReadOnlyOpenDoesNotCreateTempNamespace(t *testing.T) { + t.Parallel() + + root := t.TempDir() + const key = "cluster-a/v1/manifests/existing.json" + objectPath := filepath.Join(root, filepath.FromSlash(key)) + require.NoError(t, os.MkdirAll(filepath.Dir(objectPath), 0o755)) + require.NoError(t, os.WriteFile(objectPath, []byte("existing"), 0o600)) + require.NoError(t, os.Chmod(root, 0o555)) + t.Cleanup(func() { require.NoError(t, os.Chmod(root, 0o755)) }) + + store, err := NewLocalStore(root) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, store.Close()) }) + require.NoDirExists(t, filepath.Join(root, localStoreTempDir)) + + body, _, err := store.GetObject(context.Background(), key) + require.NoError(t, err) + got, err := io.ReadAll(body) + require.NoError(t, err) + require.NoError(t, body.Close()) + require.Equal(t, []byte("existing"), got) + require.NoDirExists(t, filepath.Join(root, localStoreTempDir)) +} + +// TestGCDoesNotDeletePayloadRefreshedByAConcurrentPublish is the +// regression test for the concurrent-publication race. +// +// A publisher reusing a content-addressed payload refreshes it — +// rewriting IDENTICAL bytes — and then commits a manifest naming it. +// No conditional-delete primitive on a general-purpose S3 bucket +// detects that: `If-Match` compares a content-derived ETag, which +// identical bytes leave unchanged, and `IfMatchLastModifiedTime` is +// directory-buckets only. +// +// The §5 two-pass rule is what closes it: the refresh moves the +// object's mtime, so the sweep pass sees state that differs from the +// mark and re-marks instead of deleting. +func TestGCDoesNotDeletePayloadRefreshedByAConcurrentPublish(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + orphan := f.publishManifest(t, 1, 10, []byte("reused-by-a-concurrent-publish"), year) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + + // Pass 1: the payload becomes eligible and is marked. + first, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.Empty(t, first.PayloadsDeleted) + require.Equal(t, 1, first.PayloadsAwaitingSweep) + require.Contains(t, gc.MarkedPayloads(), orphan.Payload.Key) + + // Between the passes a publisher reuses the payload: same bytes, + // fresh mtime. This is the event no ETag precondition can see. + f.now = f.now.Add(2 * time.Hour) + f.setMTime(t, orphan.Payload.Key, f.now) + + // Pass 2: the state no longer matches the mark, so the payload is + // re-marked rather than reclaimed. + second, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.Empty(t, second.PayloadsDeleted, + "a payload refreshed between passes must not be reclaimed") + require.True(t, f.exists(t, orphan.Payload.Key)) +} + +type claimAttemptDuringFinalScanStore struct { + RetentionStore + payloadKey string + manifestKey string + groupLists int + payloadClaimErr error + manifestClaimErr error +} + +func (s *claimAttemptDuringFinalScanStore) ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) { + refs, err := s.RetentionStore.ListObjects(ctx, prefix) + if err != nil { + return nil, err + } + if strings.HasSuffix(prefix, "/v1/groups") { + s.afterGroupList(ctx) + } + return refs, nil +} + +func (s *claimAttemptDuringFinalScanStore) afterGroupList(ctx context.Context) { + s.groupLists++ + switch s.groupLists { + case 2: + s.manifestClaimErr = attemptClaim(ctx, s.RetentionStore, s.manifestKey) + case 4: + s.payloadClaimErr = attemptClaim(ctx, s.RetentionStore, s.payloadKey) + } +} + +func attemptClaim(ctx context.Context, store ObjectClaimStore, key string) error { + claim, err := store.AcquireObjectClaim(ctx, key) + if err != nil { + return err + } + return claim.Release(ctx) +} + +// TestGCHoldsPayloadClaimAcrossTheFinalManifestScan pins the ordering that +// closes the same-ETag race. The fourth groups listing is the sweep pass's +// authoritative scan; a publisher trying to claim the payload after that scan +// has started must be excluded until GC finishes. +func TestGCHoldsPayloadClaimAcrossTheFinalManifestScan(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + orphan := f.publishManifest(t, 1, 10, []byte("sweep-race"), year) + store := &claimAttemptDuringFinalScanStore{ + RetentionStore: f.store, + payloadKey: orphan.Payload.Key, + manifestKey: orphan.ManifestKey, + } + gc, err := NewGC(GCOptions{ + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{ + MinGenerations: 1, + MaxAge: time.Hour, + PayloadGrace: time.Hour, + MinMarkAge: time.Hour, + }, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + _, err = gc.RunOnce(context.Background()) + require.NoError(t, err) + f.now = f.now.Add(2 * time.Hour) + _, err = gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.ErrorIs(t, store.payloadClaimErr, ErrObjectClaimed, + "the payload claim must already be held when the final manifest scan completes") + require.ErrorIs(t, store.manifestClaimErr, ErrObjectClaimed, + "the expiring manifest claim must already be held when the final manifest scan completes") +} + +// TestGCReclaimsAfterTwoQuietPasses is the complement: a payload that +// nobody touches across both passes is reclaimed, so the two-pass rule +// delays collection rather than preventing it. +func TestGCReclaimsAfterTwoQuietPasses(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + orphan := f.publishManifest(t, 1, 10, []byte("genuinely-garbage"), year) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + _, reclaim := f.sweep(t, gc) + + require.Contains(t, reclaim.PayloadsDeleted, orphan.Payload.Key) + require.False(t, f.exists(t, orphan.Payload.Key)) + require.NotContains(t, gc.MarkedPayloads(), orphan.Payload.Key, + "a reclaimed payload must not leave its mark behind") +} + +// TestGCDropsTheMarkWhenAPayloadBecomesReferencedAgain covers the +// other invalidation path: a payload that a new manifest starts naming +// must lose its mark, so a later eligibility serves a full fresh delay +// rather than inheriting a stale one. +func TestGCDropsTheMarkWhenAPayloadBecomesReferencedAgain(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + reused := []byte("about-to-be-referenced-again") + orphan := f.publishManifest(t, 1, 10, reused, year) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + _, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.Contains(t, gc.MarkedPayloads(), orphan.Payload.Key) + + // A new manifest in another group now references the same content. + f.now = f.now.Add(2 * time.Hour) + f.publishManifest(t, 2, 5, reused, 0) + + second, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.Empty(t, second.PayloadsDeleted) + require.NotContains(t, gc.MarkedPayloads(), orphan.Payload.Key, + "a payload that is referenced again must lose its mark") + require.True(t, f.exists(t, orphan.Payload.Key)) +} + +// TestLocalStoreConditionalDeleteRejectsChangedObject exercises the +// precondition directly. +func TestLocalStoreConditionalDeleteRejectsChangedObject(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + ctx := context.Background() + key := "cluster-a/v1/payloads/sha256/ab/cond.fsm" + body := []byte("payload bytes") + _, err := f.store.PutObject(ctx, key, bytes.NewReader(body), PutOptions{ + Size: int64(len(body)), + SHA256: hexSHA256Bytes(body), + }) + require.NoError(t, err) + f.setMTime(t, key, f.now.Add(-time.Hour)) + + stale := DeletePrecondition{Size: int64(len(body)), UpdatedAt: f.now.Add(-time.Hour)} + + // The object moves under the caller's feet. + f.setMTime(t, key, f.now) + err = f.store.DeleteObjectIfUnmodified(ctx, key, stale) + require.Error(t, err) + require.True(t, errors.Is(err, ErrObjectModified)) + require.True(t, f.exists(t, key), "a failed precondition must not delete") + + // With the current state it succeeds. + current := DeletePrecondition{Size: int64(len(body)), UpdatedAt: f.now} + require.NoError(t, f.store.DeleteObjectIfUnmodified(ctx, key, current)) + require.False(t, f.exists(t, key)) + + // Already-absent stays a no-op so GC is retry-safe. + require.NoError(t, f.store.DeleteObjectIfUnmodified(ctx, key, current)) +} + +func TestLocalStoreDeleteIsIdempotentWhenParentNeverExisted(t *testing.T) { + t.Parallel() + + store := newTestLocalStore(t, filepath.Join(t.TempDir(), "missing-root")) + ctx := context.Background() + const key = "never/created/object" + + require.NoError(t, store.DeleteObject(ctx, key)) + require.NoError(t, store.DeleteObjectIfUnmodified(ctx, key, DeletePrecondition{Size: 1})) +} + +func TestLocalStoreCloseIsIdempotentAndStopsOperations(t *testing.T) { + t.Parallel() + + store := newTestLocalStore(t, t.TempDir()) + require.NoError(t, store.Close()) + require.NoError(t, store.Close()) + + _, _, err := store.HeadObject(context.Background(), "object") + require.ErrorIs(t, err, ErrInvalidOptions) +} + +// manifestRewritingStore rewrites an expired manifest between the scan +// that decided to delete it and the delete itself, modelling an +// idempotent publish retry that republishes the same index/term. +type manifestRewritingStore struct { + RetentionStore + target string + rewrite func() + done bool +} + +func (s *manifestRewritingStore) DeleteObjectIfUnmodified( + ctx context.Context, key string, cond DeletePrecondition, +) error { + if key == s.target && !s.done { + s.done = true + s.rewrite() + } + return s.RetentionStore.DeleteObjectIfUnmodified(ctx, key, cond) +} + +// TestGCDoesNotDeleteManifestRewrittenByAConcurrentPublish is the +// phase-1 sibling of the payload race. Manifest keys are deterministic +// in (group, index, term), so an idempotent republish rewrites the +// exact key retention is about to remove. Deleting it anyway would +// drop a manifest the publisher believes it just committed. +func TestGCDoesNotDeleteManifestRewrittenByAConcurrentPublish(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + stale := f.publishManifest(t, 1, 10, []byte("republished"), year) + + rewriting := &manifestRewritingStore{ + RetentionStore: f.store, + target: stale.ManifestKey, + rewrite: func() { + // The publisher's idempotent rewrite: same key, fresh mtime. + f.setMTime(t, stale.ManifestKey, f.now) + }, + } + + gc, err := NewGC(GCOptions{ + Store: rewriting, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.True(t, f.exists(t, stale.ManifestKey), + "a manifest rewritten by a concurrent publish must survive GC") + require.NotContains(t, result.ManifestsDeleted, stale.ManifestKey) + require.Equal(t, 1, result.ManifestsClaimedConcurrently) +} + +type manifestRefreshedBeforeClaimStore struct { + RetentionStore + target string + refresh func() + done bool +} + +func (s *manifestRefreshedBeforeClaimStore) AcquireObjectClaim( + ctx context.Context, key string, +) (ObjectClaim, error) { + if key == s.target && !s.done { + s.done = true + s.refresh() + } + return s.RetentionStore.AcquireObjectClaim(ctx, key) +} + +func TestGCKeepsManifestRefreshedBeforeItsClaim(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + stale := f.publishManifest(t, 1, 10, []byte("republished-before-claim"), year) + store := &manifestRefreshedBeforeClaimStore{ + RetentionStore: f.store, + target: stale.ManifestKey, + refresh: func() { + f.setMTime(t, stale.ManifestKey, f.now.Add(time.Minute)) + }, + } + gc, err := NewGC(GCOptions{ + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.True(t, f.exists(t, stale.ManifestKey), + "a manifest refreshed between the initial scan and claim acquisition must survive") + require.NotContains(t, result.ManifestsDeleted, stale.ManifestKey) + require.Equal(t, 1, result.ManifestsClaimedConcurrently) +} + +func TestGCKeepsManifestWhoseContentChangedWithStableFilesystemMetadata(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + stale := f.publishManifest(t, 1, 10, []byte("content-generation"), year) + before, exists, err := f.store.HeadObject(context.Background(), stale.ManifestKey) + require.NoError(t, err) + require.True(t, exists) + + store := &manifestRefreshedBeforeClaimStore{ + RetentionStore: f.store, + target: stale.ManifestKey, + refresh: func() { + manifest, err := LoadManifest(context.Background(), f.store, stale.ManifestKey) + require.NoError(t, err) + manifest.SourceCluster = "cluster-b" + data, _, err := manifest.MarshalCanonical() + require.NoError(t, err) + _, err = f.store.RefreshObject( + context.Background(), + stale.ManifestKey, + bytes.NewReader(data), + PutOptions{Size: int64(len(data)), SHA256: hexSHA256Bytes(data)}, + ) + require.NoError(t, err) + after, exists, err := f.store.HeadObject(context.Background(), stale.ManifestKey) + require.NoError(t, err) + require.True(t, exists) + require.Equal(t, before.Size, after.Size, + "the regression requires a same-size manifest rewrite") + f.setMTime(t, stale.ManifestKey, before.UpdatedAt) + }, + } + gc, err := NewGC(GCOptions{ + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.True(t, f.exists(t, stale.ManifestKey), + "a changed manifest self-hash must survive even when size and mtime are unchanged") + require.NotContains(t, result.ManifestsDeleted, stale.ManifestKey) + require.Equal(t, 1, result.ManifestsClaimedConcurrently) +} + +type manifestClaimBlockingStore struct { + RetentionStore + manifestKey string +} + +func (s *manifestClaimBlockingStore) AcquireObjectClaim( + ctx context.Context, key string, +) (ObjectClaim, error) { + if key == s.manifestKey { + return nil, ErrObjectClaimed + } + return s.RetentionStore.AcquireObjectClaim(ctx, key) +} + +func TestGCKeepsPayloadWhenExpiredManifestClaimIsBusy(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + stale := f.publishManifest(t, 1, 10, []byte("still-referenced"), year) + store := &manifestClaimBlockingStore{ + RetentionStore: f.store, + manifestKey: stale.ManifestKey, + } + gc, err := NewGC(GCOptions{ + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{ + MinGenerations: 1, + MaxAge: time.Hour, + PayloadGrace: time.Hour, + MinMarkAge: time.Hour, + }, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + _, err = gc.RunOnce(context.Background()) + require.NoError(t, err) + f.now = f.now.Add(2 * time.Hour) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.True(t, f.exists(t, stale.ManifestKey)) + require.True(t, f.exists(t, stale.Payload.Key), + "a payload remains live while its expired manifest could not be deleted") + require.NotContains(t, result.PayloadsDeleted, stale.Payload.Key) + require.Equal(t, 1, result.ManifestsClaimedConcurrently) +} + +// countingListStore counts ListObjects calls so a test can assert the +// revalidation pass is per-phase, not per-payload. +type countingListStore struct { + RetentionStore + mu sync.Mutex + lists int +} + +func (s *countingListStore) ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) { + s.mu.Lock() + s.lists++ + s.mu.Unlock() + return s.RetentionStore.ListObjects(ctx, prefix) +} + +// TestGCRevalidatesReferencesOncePerPassNotPerPayload pins the cost +// shape. Revalidating inside the per-payload loop turns a stale-payload +// backlog into N listings and O(N×M) object reads, so the first cleanup +// of a realistically accumulated backlog never finishes. +func TestGCRevalidatesReferencesOncePerPassNotPerPayload(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 100, []byte("current"), year) + // Eight orphaned payloads, all eligible for reclamation. + for i := range uint64(8) { + f.publishManifest(t, 1, 10+i, []byte(fmt.Sprintf("orphan-%d", i)), year) + } + + counting := &countingListStore{RetentionStore: f.store} + gc, err := NewGC(GCOptions{ + Store: counting, + Prefix: retentionPrefix, + Policy: RetentionPolicy{ + MinGenerations: 1, MaxAge: time.Hour, + PayloadGrace: time.Hour, MinMarkAge: time.Hour, + }, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + // Pass 1 marks; pass 2 (after the sweep delay) reclaims. Count + // listings for the reclaiming pass only, so the bound is measured + // against a pass that does the full amount of work. + _, err = gc.RunOnce(context.Background()) + require.NoError(t, err) + f.now = f.now.Add(2 * time.Hour) + + counting.mu.Lock() + counting.lists = 0 + counting.mu.Unlock() + + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.NotEmpty(t, result.PayloadsDeleted, "the fixture must actually reclaim something") + + counting.mu.Lock() + defer counting.mu.Unlock() + // One manifest listing (phase 1) + one payload listing + one + // revalidation listing. The bound is what matters: it must not + // grow with the number of reclaimable payloads. + require.LessOrEqual(t, counting.lists, 4, + "listings must be bounded per pass, not proportional to the payload backlog") +} + +// TestGCRejectsAManifestStoredOffItsCanonicalPath closes a retention +// hijack: retention groups and orders by the manifest BODY, so a +// high-index body claiming group 2 parked under a group-1 path would +// consume group 2's retained-generation slots and get group 2's real +// newest manifests deleted. +func TestGCRejectsAManifestStoredOffItsCanonicalPath(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + ctx := context.Background() + year := 365 * 24 * time.Hour + victim := f.publishManifest(t, 2, 5, []byte("group-2-only-restore-point"), year) + + // A manifest whose body claims group 2 at a very high index, but + // which is stored under group 1's path. Its self-hash is valid and + // its ManifestKey matches where it lives. + hijackKey, err := manifestKey(retentionPrefix, 1, 9000, 2) + require.NoError(t, err) + payloadSHA := hexSHA256Bytes([]byte("hijack")) + pKey, err := payloadKey(retentionPrefix, payloadSHA) + require.NoError(t, err) + hijack := Manifest{ + SchemaVersion: ManifestSchemaVersion, + CreatedAt: f.now, + SourceCluster: retentionPrefix, + GroupID: 2, // body claims group 2 ... + SnapshotIndex: 9000, + SnapshotTerm: 2, + ConfState: ManifestConfState{Voters: []uint64{1}}, + Payload: PayloadDescriptor{Key: pKey, Bytes: 6, SHA256: payloadSHA}, + ManifestKey: hijackKey, // ... but lives under group 1's path + } + encoded, _, err := hijack.MarshalCanonical() + require.NoError(t, err) + _, err = f.store.PutObject(ctx, hijackKey, bytes.NewReader(encoded), PutOptions{ + Size: int64(len(encoded)), + SHA256: hexSHA256Bytes(encoded), + }) + require.NoError(t, err) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.Contains(t, result.MalformedManifests, hijackKey, + "a manifest off its canonical path must be classified malformed") + require.True(t, f.exists(t, victim.ManifestKey), + "group 2's real manifest must not be displaced by the hijack") + require.True(t, result.PayloadPhaseSkipped, + "a malformed manifest blocks reclamation") +} + +// duplicateListingStore returns one manifest key twice, modelling an +// S3-compatible endpoint producing overlapping pages while objects +// change underneath the scan. +type duplicateListingStore struct { + RetentionStore + target string +} + +func (s *duplicateListingStore) ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) { + refs, err := s.RetentionStore.ListObjects(ctx, prefix) + if err != nil { + return nil, err + } + for _, ref := range refs { + if ref.Key == s.target { + refs = append(refs, ref) + break + } + } + return refs, nil +} + +// TestGCDeduplicatesListedManifestKeys is the duplicate-page guard. +// Two copies of the same key would be counted as two generations: with +// MinGenerations 1 one lands in survivors and the other in expired, so +// phase 1 would delete the exact key chosen as the group's newest +// restore point. +func TestGCDeduplicatesListedManifestKeys(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + only := f.publishManifest(t, 1, 10, []byte("sole-restore-point"), 365*24*time.Hour) + + dup := &duplicateListingStore{RetentionStore: f.store, target: only.ManifestKey} + gc, err := NewGC(GCOptions{ + Store: dup, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, + }) + require.NoError(t, err) + + result, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + + require.Empty(t, result.ManifestsDeleted) + require.True(t, f.exists(t, only.ManifestKey), + "a duplicated listing entry must not make a group's newest manifest deletable") + require.Equal(t, 1, result.ManifestsScanned, "duplicates must be collapsed") +} + +// TestGCSweepableRequiresTheMarkedStateToBeUnchanged pins the mark's +// state comparison directly. +// +// It needs its own test because the end-to-end race test does NOT +// exercise it: a refresh sets mtime to now, so the grace check rejects +// the object before the mark is ever consulted. The case the +// comparison uniquely covers is a change that leaves mtime untouched — +// here a size change — which the grace check cannot see. +func TestGCSweepableRequiresTheMarkedStateToBeUnchanged(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + gc := f.gc(t, RetentionPolicy{ + MinGenerations: 1, MaxAge: time.Hour, + PayloadGrace: time.Hour, MinMarkAge: time.Hour, + }) + + ref := ObjectRef{ + Key: "cluster-a/v1/payloads/sha256/ab/x.fsm", + Size: 100, + UpdatedAt: f.now.Add(-365 * 24 * time.Hour), + } + + // First observation marks it. + require.False(t, gc.sweepable(ref), "the first pass must only mark") + require.Equal(t, []string{ref.Key}, gc.MarkedPayloads()) + + // Same state, past the sweep delay: now sweepable. + f.now = f.now.Add(2 * time.Hour) + require.True(t, gc.sweepable(ref)) + + // Size changed while mtime stayed put: the mark must be discarded + // and the delay restarted, even though the object still looks old. + changed := ref + changed.Size = 101 + require.False(t, gc.sweepable(changed), + "a mark must not be honoured for bytes that changed underneath it") + + // And the restarted mark serves a full fresh delay. + require.False(t, gc.sweepable(changed)) + f.now = f.now.Add(2 * time.Hour) + require.True(t, gc.sweepable(changed)) +} + +// TestGCPrunesMarksForPayloadsThatVanished stops the mark set growing +// without bound. A payload removed by another GC process or a bucket +// lifecycle rule never passes through reclaimPayload again, so no +// dropMark call can reach it. +func TestGCPrunesMarksForPayloadsThatVanished(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + year := 365 * 24 * time.Hour + f.publishManifest(t, 1, 30, []byte("current"), year) + orphan := f.publishManifest(t, 1, 10, []byte("removed-externally"), year) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + _, err := gc.RunOnce(context.Background()) + require.NoError(t, err) + require.Contains(t, gc.MarkedPayloads(), orphan.Payload.Key) + + // Something else reclaims the object between passes. + require.NoError(t, f.store.DeleteObject(context.Background(), orphan.Payload.Key)) + + f.now = f.now.Add(2 * time.Hour) + _, err = gc.RunOnce(context.Background()) + require.NoError(t, err) + require.NotContains(t, gc.MarkedPayloads(), orphan.Payload.Key, + "a mark for an object absent from a complete listing must be pruned") + require.Empty(t, gc.MarkedPayloads()) +} + +// TestLocalStoreListObjectsRejectsTraversingPrefixes keeps a listing +// inside the store root. Every other local-store operation rejects the +// equivalent key through pathForKey; listing must not be the one path +// that leaks an ancestor tree's names, sizes and timestamps. +func TestLocalStoreListObjectsRejectsTraversingPrefixes(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + // A file in the parent of the store root, which must stay unseen. + outside := filepath.Join(filepath.Dir(f.root), "outside.txt") + require.NoError(t, os.WriteFile(outside, []byte("not yours"), 0o600)) + + for _, prefix := range []string{"..", "../", "../sibling", " .. "} { + _, err := f.store.ListObjects(context.Background(), prefix) + require.ErrorIs(t, err, ErrInvalidOptions, "prefix %q must be rejected", prefix) + } + + // A normal prefix still works. + refs, err := f.store.ListObjects(context.Background(), retentionPrefix) + require.NoError(t, err) + for _, ref := range refs { + require.NotContains(t, ref.Key, "outside.txt") + } +} + +// nonRefreshingStore exposes only ObjectStore, so a type assertion to the +// storage-visible claim capability fails — the shape a decorator that forwards +// a narrow interface produces. +type nonRefreshingStore struct { + put func(context.Context, string, io.Reader, PutOptions) (ObjectInfo, error) + get func(context.Context, string) (io.ReadCloser, ObjectInfo, error) + head func(context.Context, string) (ObjectInfo, bool, error) +} + +func (s nonRefreshingStore) PutObject( + ctx context.Context, key string, body io.Reader, opts PutOptions, +) (ObjectInfo, error) { + return s.put(ctx, key, body, opts) +} + +func (s nonRefreshingStore) GetObject(ctx context.Context, key string) (io.ReadCloser, ObjectInfo, error) { + return s.get(ctx, key) +} + +func (s nonRefreshingStore) HeadObject(ctx context.Context, key string) (ObjectInfo, bool, error) { + return s.head(ctx, key) +} + +// TestObjectClaimStoreRequiredForPublicationCoordination prevents a decorator +// from silently dropping the capability that closes the publish/GC race. +func TestObjectClaimStoreRequiredForPublicationCoordination(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + narrow := nonRefreshingStore{ + put: f.store.PutObject, + get: f.store.GetObject, + head: f.store.HeadObject, + } + _, err := objectClaimStore(narrow) + require.ErrorIs(t, err, ErrInvalidOptions) + + claimable, err := objectClaimStore(f.store) + require.NoError(t, err) + require.NotNil(t, claimable) +} + +// TestLocalStoreListObjectsRejectsNativeSeparatorTraversal covers the +// Windows form: cleanObjectPrefix uses path (slash) semantics but +// filepath.Join interprets the platform separator, so a backslash +// prefix would survive a slash-only check and then escape the root. +func TestLocalStoreListObjectsRejectsNativeSeparatorTraversal(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + for _, prefix := range []string{ + `..\sibling`, `a\..\..\b`, `\\..\\x`, + // Volume-qualified prefixes escape the root the same way keys do, + // so the shared guard has to refuse them on both paths -- fixing + // one and leaving the other is how the original traversal bug + // got in. + // + // Rooted prefixes are NOT listed: cleanObjectPrefix uses + // strings.Trim("/"), which strips every leading slash, so + // `//sibling` is already the legitimate prefix `sibling`. Only + // normalizeObjectKey has the rooted problem, because it trims a + // single slash with TrimPrefix. + `C:/sibling`, "C:sibling", + } { + _, err := f.store.ListObjects(context.Background(), prefix) + require.ErrorIs(t, err, ErrInvalidOptions, "prefix %q must be rejected", prefix) + } +} + +// TestLocalStoreRejectsTraversalOnEveryObjectOperation is the sibling +// sweep for the prefix fix. The traversal guard was first applied only +// to ListObjects, leaving the direct key path — Get/Head/Put and both +// deletes — able to resolve outside the root on a native separator. +func TestLocalStoreRejectsTraversalOnEveryObjectOperation(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + ctx := context.Background() + + // A file just outside the store root that must stay untouched. + outside := filepath.Join(filepath.Dir(f.root), "victim.txt") + require.NoError(t, os.WriteFile(outside, []byte("not yours"), 0o600)) + + for _, key := range []string{ + "..", `..\victim.txt`, `a\..\..\victim.txt`, "../victim.txt", + // normalizeObjectKey trims ONE leading slash, so these arrive as + // rooted paths that every relative check above accepts. + // filepath.Join against a drive-relative root such as `C:` + // resolves `/victim.txt` to `C:\victim.txt`, outside the store, + // and the delete methods would then remove an arbitrary file. + "//victim.txt", "///victim.txt", "//..//victim.txt", + // Volume-qualified keys escape the same way. + `C:/victim.txt`, "C:victim.txt", + } { + t.Run(key, func(t *testing.T) { + _, err := f.store.PutObject(ctx, key, bytes.NewReader(nil), PutOptions{ + SHA256: hexSHA256Bytes(nil), + }) + require.ErrorIs(t, err, ErrInvalidOptions, "PutObject") + + _, _, err = f.store.GetObject(ctx, key) + require.ErrorIs(t, err, ErrInvalidOptions, "GetObject") + + _, _, err = f.store.HeadObject(ctx, key) + require.ErrorIs(t, err, ErrInvalidOptions, "HeadObject") + + require.ErrorIs(t, f.store.DeleteObject(ctx, key), ErrInvalidOptions, "DeleteObject") + + require.ErrorIs(t, + f.store.DeleteObjectIfUnmodified(ctx, key, DeletePrecondition{Size: 9}), + ErrInvalidOptions, "DeleteObjectIfUnmodified") + }) + } + + require.FileExists(t, outside, "no operation may reach outside the store root") +} + +// TestObjectPathSegmentIsSafeRejectsRootedAndVolumeQualifiedKeys states the +// character-level guard directly, because the escape it closes is +// platform-specific: filepath.Join("C:", "/victim") resolves outside the root +// on Windows but harmlessly inside it on POSIX, so a test that only exercised +// the store through the filesystem would pass on CI while Windows stayed +// exposed. +func TestObjectPathSegmentIsSafeRejectsRootedAndVolumeQualifiedKeys(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + normalized string + safe bool + }{ + // Rooted: what normalizeObjectKey produces from `//victim`. + {"/victim", false}, + {"/", false}, + // Volume-qualified. + {"C:", false}, + {"C:victim", false}, + {"c:/victim", false}, + // A single character before the colon is indistinguishable from a + // drive designator, so it is refused rather than guessed at. + {"a:b", false}, + // A multi-character prefix cannot be a drive, so namespaced keys + // stay legal. + {"ns:key/part", true}, + {"sha256:abc/def", true}, + // Ordinary keys are unaffected. + {"manifests/2026/snap.json", true}, + {"snap.json", true}, + // Still rejected from before. + {"", false}, + {".", false}, + {"..", false}, + {"../victim", false}, + {`..\victim`, false}, + } { + t.Run(tc.normalized, func(t *testing.T) { + t.Parallel() + require.Equal(t, tc.safe, objectPathSegmentIsSafe(tc.normalized)) + }) + } +} + +// TestObjectPathWithinRootIsTheBackstop pins the containment check, which states +// the property the character checks only approximate: whatever the key looked +// like, the joined path has to land under the root. +func TestObjectPathWithinRootIsTheBackstop(t *testing.T) { + t.Parallel() + + root := filepath.Join("/srv", "snapshots") + require.True(t, objectPathWithinRoot(root, filepath.Join(root, "a", "b.json"))) + require.True(t, objectPathWithinRoot(root, filepath.Join(root, "a.json"))) + + // The root itself is not an object path. + require.False(t, objectPathWithinRoot(root, root)) + // A sibling directory sharing a name prefix must not count as inside. + require.False(t, objectPathWithinRoot(root, filepath.Join("/srv", "snapshots-evil", "a.json"))) + require.False(t, objectPathWithinRoot(root, filepath.Join("/srv", "other", "a.json"))) + require.False(t, objectPathWithinRoot(root, filepath.Join("/", "victim.txt"))) +} + +// TestGCRejectsAManifestReferencingAPayloadOutsideItsPrefix closes a +// cross-prefix data-loss path. +// +// Manifest-key verification re-derives the canonical MANIFEST path, but the +// payload reference was taken on trust. A manifest under prefix A naming a +// payload under prefix B records the exact B key in A's live set — and A's scan +// never lists B, while B's scan cannot see A's manifest. B therefore reclaims a +// payload A still references, and A's restore breaks with a missing payload. +func TestGCRejectsAManifestReferencingAPayloadOutsideItsPrefix(t *testing.T) { + t.Parallel() + + f := newGCFixture(t) + ctx := context.Background() + good := f.publishManifest(t, 1, 10, []byte("inside"), time.Minute) + + // A manifest at a fresh but fully canonical path: correct group, index, + // term and manifest key, self-consistent body. Only the payload reference + // points outside this prefix, so every other verification step passes and + // this isolates the payload check. + const ( + crossGroup = uint64(1) + crossIndex = uint64(12) + crossTerm = uint64(2) + ) + mKey, err := manifestKey(retentionPrefix, crossGroup, crossIndex, crossTerm) + require.NoError(t, err) + elsewhere := []byte("elsewhere") + elsewhereSHA := hexSHA256Bytes(elsewhere) + foreign, err := payloadKey("other-cluster", elsewhereSHA) + require.NoError(t, err) + canonical, err := payloadKey(retentionPrefix, elsewhereSHA) + require.NoError(t, err) + require.NotEqual(t, canonical, foreign, "the fixture must actually cross a prefix") + + f.writeManifest(t, Manifest{ + SchemaVersion: ManifestSchemaVersion, + CreatedAt: f.now.Add(-time.Minute), + SourceCluster: retentionPrefix, + GroupID: crossGroup, + SnapshotIndex: crossIndex, + SnapshotTerm: crossTerm, + ConfState: ManifestConfState{Voters: []uint64{1}}, + Payload: PayloadDescriptor{ + Key: foreign, + Bytes: int64(len(elsewhere)), + SHA256: elsewhereSHA, + }, + ManifestKey: mKey, + }, time.Minute) + + gc := f.gc(t, RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}) + result, err := gc.RunOnce(ctx) + require.NoError(t, err) + + require.Contains(t, result.MalformedManifests, mKey, + "a payload reference outside this prefix must be classified malformed") + require.True(t, result.PayloadPhaseSkipped, + "a malformed manifest must block reclamation") + require.True(t, f.exists(t, good.Payload.Key), + "no payload may be reclaimed while a manifest is unreadable") +} + +// TestLocalStoreDeleteDoesNotFollowASymlinkOutOfTheRoot closes the gap the +// lexical containment check cannot. +// +// objectPathWithinRoot compares strings, so it cannot see a symlink: if any +// ancestor under the root is one — or is swapped for one between the listing +// and the delete — os.Remove follows it and unlinks a file outside the +// configured root. The character checks cannot close that, because the path +// they validated is still the path being passed; it is the resolution that +// differs. +func TestLocalStoreDeleteDoesNotFollowASymlinkOutOfTheRoot(t *testing.T) { + t.Parallel() + + ctx := context.Background() + base := t.TempDir() + root := filepath.Join(base, "store") + require.NoError(t, os.MkdirAll(root, 0o750)) + + // A file outside the store that must survive. + outside := filepath.Join(base, "outside") + require.NoError(t, os.MkdirAll(outside, 0o750)) + victim := filepath.Join(outside, "victim.txt") + require.NoError(t, os.WriteFile(victim, []byte("not yours"), 0o600)) + + // An in-root directory that is really a symlink pointing out of the root. + // Every lexical check passes for "escape/victim.txt": no "..", no + // backslash, not rooted, and the joined path is under the root. + require.NoError(t, os.Symlink(outside, filepath.Join(root, "escape"))) + + store := newTestLocalStore(t, root) + + err := store.DeleteObject(ctx, "escape/victim.txt") + require.Error(t, err, "a delete that resolves through a symlink out of the root must fail") + require.FileExists(t, victim, + "the file outside the store root must survive: os.Root refuses to traverse out of it") + + require.Error(t, + store.DeleteObjectIfUnmodified(ctx, "escape/victim.txt", DeletePrecondition{Size: 9}), + "the conditional delete shares the same escape") + require.FileExists(t, victim) +} + +func TestLocalStoreRefreshDoesNotFollowASymlinkOutOfTheRoot(t *testing.T) { + t.Parallel() + + ctx := context.Background() + base := t.TempDir() + root := filepath.Join(base, "store") + require.NoError(t, os.MkdirAll(root, 0o750)) + out := filepath.Join(base, "outside") + require.NoError(t, os.MkdirAll(out, 0o750)) + victim := filepath.Join(out, "manifest.json") + require.NoError(t, os.WriteFile(victim, []byte("original"), 0o600)) + require.NoError(t, os.Symlink(out, filepath.Join(root, "escape"))) + + store := newTestLocalStore(t, root) + replacement := []byte("replacement") + _, err := store.RefreshObject(ctx, "escape/manifest.json", bytes.NewReader(replacement), PutOptions{ + Size: int64(len(replacement)), + SHA256: hexSHA256Bytes(replacement), + ContentType: "application/json", + }) + require.Error(t, err) + contents, err := os.ReadFile(victim) + require.NoError(t, err) + require.Equal(t, []byte("original"), contents) + + _, err = store.PutObject(ctx, "escape/new.json", bytes.NewReader(replacement), PutOptions{ + Size: int64(len(replacement)), + SHA256: hexSHA256Bytes(replacement), + ContentType: "application/json", + }) + require.Error(t, err) + require.NoFileExists(t, filepath.Join(out, "new.json")) + + _, _, err = store.GetObject(ctx, "escape/manifest.json") + require.Error(t, err) + _, _, err = store.HeadObject(ctx, "escape/manifest.json") + require.Error(t, err) +} + +func TestLocalStoreListObjectsFailsClosedOnSymlink(t *testing.T) { + t.Parallel() + + base := t.TempDir() + root := filepath.Join(base, "store") + groups := filepath.Join(root, retentionPrefix, "v1", "groups") + require.NoError(t, os.MkdirAll(filepath.Dir(groups), 0o750)) + out := filepath.Join(base, "outside") + require.NoError(t, os.MkdirAll(out, 0o750)) + require.NoError(t, os.Symlink(out, groups)) + + store := newTestLocalStore(t, root) + _, err := store.ListObjects(context.Background(), path.Join(retentionPrefix, "v1", "groups")) + require.ErrorIs(t, err, ErrIntegrity) +} + +// An ordinary nested key still deletes, so the descriptor-relative path did not +// simply break deletion. +func TestLocalStoreDeleteStillRemovesANestedObject(t *testing.T) { + t.Parallel() + + ctx := context.Background() + root := t.TempDir() + store := newTestLocalStore(t, root) + + _, err := store.PutObject(ctx, "a/b/c.json", bytes.NewReader([]byte("{}")), PutOptions{ + Size: 2, + SHA256: hexSHA256Bytes([]byte("{}")), + }) + require.NoError(t, err) + require.FileExists(t, filepath.Join(root, "a", "b", "c.json")) + + require.NoError(t, store.DeleteObject(ctx, "a/b/c.json")) + require.NoFileExists(t, filepath.Join(root, "a", "b", "c.json")) +} + +// TestLocalStoreDeleteResolvesAgainstThePinnedRoot covers the escape in the +// ROOT ARGUMENT, which os.Root cannot refuse because it happens before any +// resolution the descriptor governs. +// +// Reopening the root per operation re-resolves a mutable pathname: +// os.OpenRoot follows symlinks in its own argument, so a process that can +// write the root's PARENT can rename the root away and drop a symlink in its +// place between two deletes. The next open then anchors the descriptor outside +// the configured directory and a descendant-relative Remove unlinks an +// external file at the corresponding key. Pinning the descriptor on first use +// means later operations resolve against the directory the store was +// configured with, whatever the pathname comes to point at. +func TestLocalStoreDeleteResolvesAgainstThePinnedRoot(t *testing.T) { + t.Parallel() + + ctx := context.Background() + base := t.TempDir() + root := filepath.Join(base, "store") + require.NoError(t, os.MkdirAll(root, 0o750)) + + store := newTestLocalStore(t, root) + + // One real delete, so the descriptor is pinned to the configured + // directory while it is still the configured directory. + require.NoError(t, store.DeleteObject(ctx, "absent.json")) + + // Now the attacker swaps the root for a symlink to a directory it owns. + attacker := filepath.Join(base, "attacker") + require.NoError(t, os.MkdirAll(attacker, 0o750)) + victim := filepath.Join(attacker, "victim.json") + require.NoError(t, os.WriteFile(victim, []byte("not yours"), 0o600)) + require.NoError(t, os.Rename(root, filepath.Join(base, "store-moved"))) + require.NoError(t, os.Symlink(attacker, root)) + + // The same key the attacker planted. A store that re-opened the root here + // would resolve into the attacker's directory and unlink the file. + require.NoError(t, store.DeleteObject(ctx, "victim.json")) + require.FileExists(t, victim, + "the delete must resolve against the pinned root, not the swapped pathname") +} + +func TestLocalStoreAllOperationsResolveAgainstThePinnedRoot(t *testing.T) { + t.Parallel() + + ctx := context.Background() + base := t.TempDir() + root := filepath.Join(base, "store") + store := newTestLocalStore(t, root) + + const key = "nested/object.json" + original := []byte("original-root") + _, err := store.PutObject(ctx, key, bytes.NewReader(original), PutOptions{ + Size: int64(len(original)), + SHA256: hexSHA256Bytes(original), + }) + require.NoError(t, err) + before, exists, err := store.HeadObject(ctx, key) + require.NoError(t, err) + require.True(t, exists) + + attacker := filepath.Join(base, "attacker") + attackerObject := filepath.Join(attacker, filepath.FromSlash(key)) + require.NoError(t, os.MkdirAll(filepath.Dir(attackerObject), 0o750)) + attackerBytes := []byte("attacker-root") + require.Equal(t, len(original), len(attackerBytes), "fixture requires the same size") + require.NoError(t, os.WriteFile(attackerObject, attackerBytes, 0o600)) + require.NoError(t, os.Chtimes(attackerObject, before.UpdatedAt, before.UpdatedAt)) + + moved := filepath.Join(base, "store-moved") + require.NoError(t, os.Rename(root, moved)) + require.NoError(t, os.Symlink(attacker, root)) + + body, info, err := store.GetObject(ctx, key) + require.NoError(t, err) + got, err := io.ReadAll(body) + require.NoError(t, err) + require.NoError(t, body.Close()) + require.Equal(t, original, got) + require.Equal(t, before.Size, info.Size) + + head, exists, err := store.HeadObject(ctx, key) + require.NoError(t, err) + require.True(t, exists) + require.Equal(t, before.Size, head.Size) + + refs, err := store.ListObjects(ctx, "nested") + require.NoError(t, err) + require.Len(t, refs, 1) + require.Equal(t, key, refs[0].Key) + + created := []byte("new-object") + _, err = store.PutObject(ctx, "nested/new.json", bytes.NewReader(created), PutOptions{ + Size: int64(len(created)), + SHA256: hexSHA256Bytes(created), + }) + require.NoError(t, err) + require.FileExists(t, filepath.Join(moved, "nested", "new.json")) + require.NoFileExists(t, filepath.Join(attacker, "nested", "new.json")) + + require.NoError(t, store.DeleteObjectIfUnmodified(ctx, key, DeletePrecondition{ + Size: before.Size, + UpdatedAt: before.UpdatedAt, + })) + require.NoFileExists(t, filepath.Join(moved, filepath.FromSlash(key))) + require.FileExists(t, attackerObject, + "the replacement pathname must not participate in conditional validation or deletion") +} diff --git a/internal/snapshotoffload/s3_store.go b/internal/snapshotoffload/s3_store.go index f8819d8df..148c7342a 100644 --- a/internal/snapshotoffload/s3_store.go +++ b/internal/snapshotoffload/s3_store.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "encoding/hex" "io" + "net/http" "strings" "time" @@ -39,6 +40,9 @@ type S3ObjectClient interface { UploadPart(context.Context, *s3.UploadPartInput, ...func(*s3.Options)) (*s3.UploadPartOutput, error) CompleteMultipartUpload(context.Context, *s3.CompleteMultipartUploadInput, ...func(*s3.Options)) (*s3.CompleteMultipartUploadOutput, error) AbortMultipartUpload(context.Context, *s3.AbortMultipartUploadInput, ...func(*s3.Options)) (*s3.AbortMultipartUploadOutput, error) + ListObjectsV2(context.Context, *s3.ListObjectsV2Input, ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) + DeleteObject(context.Context, *s3.DeleteObjectInput, ...func(*s3.Options)) (*s3.DeleteObjectOutput, error) + GetBucketVersioning(context.Context, *s3.GetBucketVersioningInput, ...func(*s3.Options)) (*s3.GetBucketVersioningOutput, error) } type S3StoreConfig struct { @@ -54,6 +58,12 @@ type S3StoreConfig struct { ServerSideEncryption string SSEKMSKeyID string DisableChecksumHeaders bool + // AllowVersionedBucketWithLifecycle is an explicit operator assertion + // that the bucket has a noncurrent-version expiration lifecycle. The + // store rejects Enabled/Suspended versioning by default because keyed + // retention deletes otherwise create delete markers without reclaiming + // bytes. + AllowVersionedBucketWithLifecycle bool } type S3Store struct { @@ -86,9 +96,13 @@ func NewS3Store(ctx context.Context, cfg S3StoreConfig) (*S3Store, error) { } }) } + bucket := stringsTrim(cfg.Bucket) + if err := validateS3BucketVersioning(ctx, client, bucket, cfg.AllowVersionedBucketWithLifecycle); err != nil { + return nil, err + } return &S3Store{ client: client, - bucket: stringsTrim(cfg.Bucket), + bucket: bucket, serverSideEncryption: stringsTrim(cfg.ServerSideEncryption), sseKMSKeyID: stringsTrim(cfg.SSEKMSKeyID), disableChecksumHeaders: cfg.DisableChecksumHeaders, @@ -97,6 +111,33 @@ func NewS3Store(ctx context.Context, cfg S3StoreConfig) (*S3Store, error) { }, nil } +func validateS3BucketVersioning( + ctx context.Context, + client S3ObjectClient, + bucket string, + allowWithLifecycle bool, +) error { + out, err := client.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{Bucket: aws.String(bucket)}) + if err != nil { + return errors.Wrap(err, "get s3 bucket versioning (requires s3:GetBucketVersioning)") + } + if out == nil { + return errors.Wrap(ErrIntegrity, "get s3 bucket versioning returned no response") + } + switch out.Status { + case "": + return nil + case types.BucketVersioningStatusEnabled, types.BucketVersioningStatusSuspended: + if !allowWithLifecycle { + return errors.Wrapf(ErrInvalidOptions, + "s3 bucket %s has versioning %s; configure noncurrent-version expiration and explicitly allow it", bucket, out.Status) + } + default: + return errors.Wrapf(ErrIntegrity, "s3 bucket %s returned unknown versioning status %q", bucket, out.Status) + } + return nil +} + func loadS3AWSConfig(ctx context.Context, cfg S3StoreConfig) (aws.Config, error) { optFns := make([]func(*config.LoadOptions) error, 0, s3LoadConfigOptionCapHint) if stringsTrim(cfg.Region) != "" { @@ -130,7 +171,7 @@ func (s *S3Store) PutObject(ctx context.Context, key string, body io.Reader, opt if err != nil { return ObjectInfo{}, err } - if err := s.putObjectIfAbsent(ctx, normalized, body, opts); err != nil { + if err := s.putObject(ctx, normalized, body, opts, true); err != nil { if errors.Is(err, ErrObjectConflict) { return ObjectInfo{}, err } @@ -142,20 +183,41 @@ func (s *S3Store) PutObject(ctx context.Context, key string, body io.Reader, opt return s.verifyS3PutObject(ctx, normalized, opts) } -func (s *S3Store) putObjectIfAbsent(ctx context.Context, key string, body io.Reader, opts PutOptions) error { - return s.putObjectWithRetry(key, body, func() error { +func (s *S3Store) RefreshObject(ctx context.Context, key string, body io.Reader, opts PutOptions) (ObjectInfo, error) { + if err := validatePutOptions(opts); err != nil { + return ObjectInfo{}, err + } + normalized, err := validateStoreObjectKey(key) + if err != nil { + return ObjectInfo{}, err + } + if err := s.putObject(ctx, normalized, body, opts, false); err != nil { + return ObjectInfo{}, err + } + return s.verifyS3PutObject(ctx, normalized, opts) +} + +func (s *S3Store) putObject(ctx context.Context, key string, body io.Reader, opts PutOptions, ifAbsent bool) error { + put := func() error { if opts.Size > s.multipartThreshold { - return s.putMultipartIfAbsent(ctx, key, body, opts) + return s.putMultipart(ctx, key, body, opts, ifAbsent) } input, err := s.putObjectInput(key, body, opts) if err != nil { return err } + if !ifAbsent { + input.IfNoneMatch = nil + } if _, err = s.client.PutObject(ctx, input); err != nil { return errors.Wrap(err, "put s3 object") } return nil - }) + } + if !ifAbsent { + return put() + } + return s.putObjectWithRetry(key, body, put) } func (s *S3Store) putObjectWithRetry( @@ -182,11 +244,12 @@ func (s *S3Store) putObjectWithRetry( key, s3ConditionalWriteRetries) } -func (s *S3Store) putMultipartIfAbsent( +func (s *S3Store) putMultipart( ctx context.Context, key string, body io.Reader, opts PutOptions, + ifAbsent bool, ) (retErr error) { partSize, err := multipartPartSize(opts.Size, s.multipartPartSize) if err != nil { @@ -206,7 +269,7 @@ func (s *S3Store) putMultipartIfAbsent( if err != nil { return err } - if err := s.completeMultipartUpload(ctx, key, uploadID, opts.Size, parts); err != nil { + if err := s.completeMultipartUpload(ctx, key, uploadID, opts.Size, parts, ifAbsent); err != nil { return err } completed = true @@ -411,17 +474,21 @@ func (s *S3Store) completeMultipartUpload( uploadID string, size int64, parts []types.CompletedPart, + ifAbsent bool, ) error { - _, err := s.client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + input := &s3.CompleteMultipartUploadInput{ Bucket: aws.String(s.bucket), Key: aws.String(key), UploadId: aws.String(uploadID), - IfNoneMatch: aws.String("*"), MpuObjectSize: aws.Int64(size), MultipartUpload: &types.CompletedMultipartUpload{ Parts: parts, }, - }) + } + if ifAbsent { + input.IfNoneMatch = aws.String("*") + } + _, err := s.client.CompleteMultipartUpload(ctx, input) if err != nil { return errors.Wrap(err, "complete s3 multipart upload") } @@ -511,6 +578,7 @@ func (s *S3Store) GetObject(ctx context.Context, key string) (io.ReadCloser, Obj out.Metadata, out.ChecksumSHA256, out.ChecksumType, + out.LastModified, out.ServerSideEncryption, out.SSEKMSKeyId, ) @@ -522,6 +590,7 @@ func (s *S3Store) GetObject(ctx context.Context, key string) (io.ReadCloser, Obj _ = out.Body.Close() return nil, ObjectInfo{}, err } + info.ETag = aws.ToString(out.ETag) return out.Body, info, nil } @@ -550,6 +619,7 @@ func (s *S3Store) HeadObject(ctx context.Context, key string) (ObjectInfo, bool, out.Metadata, out.ChecksumSHA256, out.ChecksumType, + out.LastModified, out.ServerSideEncryption, out.SSEKMSKeyId, ) @@ -559,6 +629,9 @@ func (s *S3Store) HeadObject(ctx context.Context, key string) (ObjectInfo, bool, if err := s.validateS3ObjectEncryption(normalized, info); err != nil { return ObjectInfo{}, false, err } + // Carried so retention can use it as the compare-and-delete + // precondition on DeleteObjectIfUnmodified. + info.ETag = aws.ToString(out.ETag) return info, true, nil } @@ -678,6 +751,7 @@ func s3ObjectInfo( metadata map[string]string, checksumSHA256 *string, checksumType types.ChecksumType, + updatedAt *time.Time, encryption types.ServerSideEncryption, kmsKeyID *string, ) (ObjectInfo, error) { @@ -691,6 +765,7 @@ func s3ObjectInfo( return ObjectInfo{ Key: key, Size: *contentLength, + UpdatedAt: aws.ToTime(updatedAt), SHA256: sha, ServerSideEncryption: string(encryption), SSEKMSKeyID: aws.ToString(kmsKeyID), @@ -873,3 +948,237 @@ func isS3ConditionalConflict(err error) bool { } return apiErr.ErrorCode() == "ConditionalRequestConflict" } + +var _ RetentionStore = (*S3Store)(nil) +var _ PublishStore = (*S3Store)(nil) + +// listObjectsPageLimit bounds a single ListObjectsV2 page. The AWS +// maximum is 1000; naming it keeps the mnd linter satisfied and the +// intent legible. +const listObjectsPageLimit int32 = 1000 + +// ListObjects pages through every object under prefix. +// +// Per the RetentionStore contract this is all-or-error: any page +// failure returns an error and no partial slice, because §5 makes +// "an incomplete scan performs no deletes" a safety property. A +// truncated listing would make a live payload look unreferenced and +// let phase 2 delete data a committed manifest still points at. +func (s *S3Store) ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) { + if s == nil || s.client == nil { + return nil, errors.Wrap(ErrInvalidOptions, "object store is required") + } + listPrefix := s3ListSubtreePrefix(prefix) + + var ( + refs []ObjectRef + token *string + seenTokens = make(map[string]struct{}) + ) + for { + out, err := s.listObjectsPage(ctx, listPrefix, token) + if err != nil { + return nil, errors.Wrapf(err, "list objects under %q", prefix) + } + refs, err = appendListedObjects(refs, out, listPrefix) + if err != nil { + return nil, err + } + + next, more, err := nextListPageToken(out, prefix) + if err != nil { + return nil, err + } + if !more { + return refs, nil + } + if err := rememberListToken(seenTokens, prefix, next); err != nil { + return nil, err + } + token = next + } +} + +func s3ListSubtreePrefix(prefix string) string { + listPrefix := cleanObjectPrefix(prefix) + if listPrefix == "." { + return "" + } + return listPrefix + "/" +} + +func (s *S3Store) listObjectsPage( + ctx context.Context, + prefix string, + token *string, +) (*s3.ListObjectsV2Output, error) { + if err := ctx.Err(); err != nil { + return nil, errors.WithStack(err) + } + out, err := s.client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{ + Bucket: aws.String(s.bucket), + Prefix: aws.String(prefix), + ContinuationToken: token, + MaxKeys: aws.Int32(listObjectsPageLimit), + }) + if err != nil { + return nil, errors.Wrap(err, "list s3 objects page") + } + if out == nil { + return nil, errors.Wrapf(ErrIntegrity, "list objects under %q returned a nil page", prefix) + } + if len(out.CommonPrefixes) != 0 || out.KeyCount == nil || int64(*out.KeyCount) != int64(len(out.Contents)) { + return nil, errors.Wrapf(ErrIntegrity, "list objects under %q returned an incomplete page", prefix) + } + return out, nil +} + +func rememberListToken(seen map[string]struct{}, prefix string, token *string) error { + if _, ok := seen[*token]; ok { + return errors.Wrapf(ErrIntegrity, + "list objects under %q returned non-advancing continuation token %q", prefix, *token) + } + seen[*token] = struct{}{} + return nil +} + +// appendListedObjects converts one ListObjectsV2 page into ObjectRefs. +func appendListedObjects(refs []ObjectRef, out *s3.ListObjectsV2Output, listPrefix string) ([]ObjectRef, error) { + for _, obj := range out.Contents { + if obj.Key == nil { + return nil, errors.Wrap(ErrIntegrity, "listed object is missing its key") + } + key := *obj.Key + if listPrefix != "" && !strings.HasPrefix(key, listPrefix) { + return nil, errors.Wrapf(ErrIntegrity, "listed object %q outside prefix %q", key, listPrefix) + } + if normalizeObjectKey(key) != key { + return nil, errors.Wrapf(ErrIntegrity, "listed object key %q is not canonical", key) + } + ref := ObjectRef{Key: key, ETag: aws.ToString(obj.ETag)} + if obj.Size != nil { + ref.Size = *obj.Size + } + if obj.LastModified != nil { + ref.UpdatedAt = *obj.LastModified + } + refs = append(refs, ref) + } + return refs, nil +} + +// nextListPageToken reports whether another page follows and returns +// its continuation token. +// +// A truncated page with no continuation token cannot be continued and +// would otherwise loop forever re-reading page one. §5 requires +// pagination failure to fail closed, so it becomes an error rather +// than a silently short listing. +func nextListPageToken(out *s3.ListObjectsV2Output, prefix string) (*string, bool, error) { + if out.IsTruncated == nil || !*out.IsTruncated { + // A page that says "complete" while still handing back a + // continuation token is self-contradictory, and believing the + // flag discards every later page. For a GC live-set scan that is + // not a cosmetic truncation: a manifest missed here leaves the + // payload it references unprotected, and retention reclaims data + // a restore still needs. Fail closed like every other pagination + // failure in §5 rather than returning a short listing. + if out.NextContinuationToken != nil && *out.NextContinuationToken != "" { + return nil, false, errors.Wrapf(ErrIntegrity, + "list objects under %q reported a complete page with a continuation token", prefix) + } + return nil, false, nil + } + if out.NextContinuationToken == nil || *out.NextContinuationToken == "" { + return nil, false, errors.Wrapf(ErrIntegrity, + "list objects under %q returned a truncated page with no continuation token", prefix) + } + return out.NextContinuationToken, true, nil +} + +// DeleteObject removes one object. S3 delete is idempotent, so an +// already-absent key is not an error. +// +// Versioned buckets: a delete without a VersionId only writes a delete +// marker, so the bytes survive as a noncurrent version that later +// ListObjectsV2 scans cannot see. Retention would then report +// successful reclamation while storage kept growing. Reclaiming those +// versions requires enumerating them (ListObjectVersions) and deleting +// each VersionId, which this store deliberately does not do — whether +// to enumerate versions, refuse versioned buckets outright, or require +// a noncurrent-version lifecycle policy is a deployment decision. Until +// that is settled, a versioned backup bucket MUST carry a +// noncurrent-version expiration lifecycle rule. +func (s *S3Store) DeleteObject(ctx context.Context, key string) error { + if s == nil || s.client == nil { + return errors.Wrap(ErrInvalidOptions, "object store is required") + } + normalized, err := validateStoreObjectKey(key) + if err != nil { + return err + } + if _, err := s.client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: aws.String(s.bucket), + Key: aws.String(normalized), + }); err != nil { + return errors.Wrapf(err, "delete object %s", key) + } + return nil +} + +// DeleteObjectIfUnmodified deletes key only when it still matches +// cond, mapping S3's 412 precondition failure to ErrObjectModified. +// +// When cond carries an ETag this is an exact compare-and-delete via +// If-Match. Without one it falls back to If-Match-Last-Modified-Time +// plus If-Match-Size, which is the same contract at coarser +// resolution. A store that honours neither would silently degrade to +// an unconditional delete, so an empty condition is refused instead. +func (s *S3Store) DeleteObjectIfUnmodified(ctx context.Context, key string, cond DeletePrecondition) error { + if s == nil || s.client == nil { + return errors.Wrap(ErrInvalidOptions, "object store is required") + } + normalized, err := validateStoreObjectKey(key) + if err != nil { + return err + } + input := &s3.DeleteObjectInput{ + Bucket: aws.String(s.bucket), + Key: aws.String(normalized), + } + switch { + case strings.TrimSpace(cond.ETag) != "": + input.IfMatch = aws.String(cond.ETag) + case !cond.UpdatedAt.IsZero(): + input.IfMatchLastModifiedTime = aws.Time(cond.UpdatedAt) + input.IfMatchSize = aws.Int64(cond.Size) + default: + return errors.Wrapf(ErrInvalidOptions, + "conditional delete of %s requires an etag or a last-modified time", key) + } + if _, err := s.client.DeleteObject(ctx, input); err != nil { + if isS3NotFound(err) { + return nil + } + if isPreconditionFailed(err) { + return errors.Wrapf(ErrObjectModified, + "object %s changed since it was validated for deletion", key) + } + return errors.Wrapf(err, "conditional delete object %s", key) + } + return nil +} + +// isPreconditionFailed reports whether err is S3's 412 response to a +// failed If-Match on delete. +func isPreconditionFailed(err error) bool { + var responseErr interface{ HTTPStatusCode() int } + if errors.As(err, &responseErr) && responseErr.HTTPStatusCode() == http.StatusPreconditionFailed { + return true + } + var apiErr smithy.APIError + if errors.As(err, &apiErr) { + return apiErr.ErrorCode() == "PreconditionFailed" + } + return false +} diff --git a/internal/snapshotoffload/s3_store_test.go b/internal/snapshotoffload/s3_store_test.go index 010f65981..5543b71f1 100644 --- a/internal/snapshotoffload/s3_store_test.go +++ b/internal/snapshotoffload/s3_store_test.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "io" + "sort" "strings" "sync" "testing" @@ -15,6 +16,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/aws/smithy-go" etcdraftengine "github.com/bootjp/elastickv/internal/raftengine/etcd" + "github.com/cockroachdb/errors" "github.com/stretchr/testify/require" ) @@ -72,12 +74,14 @@ func TestS3StorePutHeadGetPreservesIntegrityMetadata(t *testing.T) { require.True(t, ok) require.Equal(t, int64(len(body)), head.Size) require.Equal(t, sha, head.SHA256) + require.False(t, head.UpdatedAt.IsZero()) require.Equal(t, types.ChecksumModeEnabled, fake.lastHeadObjectChecksumMode()) reader, gotInfo, err := store.GetObject(ctx, "snapshots/body.fsm") require.NoError(t, err) defer func() { require.NoError(t, reader.Close()) }() require.Equal(t, sha, gotInfo.SHA256) + require.Equal(t, head.UpdatedAt, gotInfo.UpdatedAt) require.Equal(t, types.ChecksumModeEnabled, fake.lastGetObjectChecksumMode()) gotBody, err := io.ReadAll(reader) require.NoError(t, err) @@ -131,7 +135,7 @@ func TestS3StoreConflictHashesExistingObjectWithoutIntegrityMetadata(t *testing. key := "snapshots/body.fsm" body := []byte("same-body") sha := hexSHA256Bytes(body) - fake.putRawObject("backup-bucket", key, body, nil, nil) + fake.putRawObject(key, body, nil, nil) info, err := store.PutObject(ctx, key, strings.NewReader("not-read-on-conflict"), PutOptions{ Size: int64(len(body)), @@ -149,7 +153,7 @@ func TestS3StoreConflictRejectsExistingObjectWithoutIntegrityMetadataMismatch(t key := "snapshots/body.fsm" existing := []byte("aaaa") candidate := []byte("bbbb") - fake.putRawObject("backup-bucket", key, existing, nil, nil) + fake.putRawObject(key, existing, nil, nil) _, err := store.PutObject(ctx, key, strings.NewReader("not-read-on-conflict"), PutOptions{ Size: int64(len(candidate)), @@ -296,7 +300,7 @@ func TestS3StoreHeadAndGetRequestChecksumModeForFullObjectValidation(t *testing. metadataSHA := hexSHA256Bytes([]byte("metadata-body")) fullObjectChecksum, err := sha256HexToBase64(hexSHA256Bytes([]byte("full-object-body"))) require.NoError(t, err) - fake.putRawObject("backup-bucket", key, []byte("body"), map[string]string{ + fake.putRawObject(key, []byte("body"), map[string]string{ s3MetadataSHA256: metadataSHA, }, aws.String(fullObjectChecksum), types.ChecksumTypeFullObject) @@ -333,6 +337,60 @@ func TestS3StoreRejectsInvalidKMSConfig(t *testing.T) { require.ErrorContains(t, err, "aliases are not supported") } +func TestS3StoreRejectsVersionedBucketsWithoutLifecycleAssertion(t *testing.T) { + t.Parallel() + + for _, status := range []types.BucketVersioningStatus{ + types.BucketVersioningStatusEnabled, + types.BucketVersioningStatusSuspended, + } { + + t.Run(string(status), func(t *testing.T) { + t.Parallel() + client := newFakeS3Client() + client.versioningStatus = status + _, err := NewS3Store(context.Background(), S3StoreConfig{ + Client: client, + Bucket: "backup-bucket", + ServerSideEncryption: string(types.ServerSideEncryptionAes256), + }) + require.ErrorIs(t, err, ErrInvalidOptions) + require.ErrorContains(t, err, "noncurrent-version expiration") + }) + } +} + +func TestS3StoreAllowsVersionedBucketWithLifecycleAssertion(t *testing.T) { + t.Parallel() + + client := newFakeS3Client() + client.versioningStatus = types.BucketVersioningStatusEnabled + store, err := NewS3Store(context.Background(), S3StoreConfig{ + Client: client, + Bucket: "backup-bucket", + ServerSideEncryption: string(types.ServerSideEncryptionAes256), + AllowVersionedBucketWithLifecycle: true, + }) + require.NoError(t, err) + require.NotNil(t, store) +} + +func TestS3StoreRejectsUnknownVersioningStatus(t *testing.T) { + t.Parallel() + for _, allow := range []bool{false, true} { + client := newFakeS3Client() + client.versioningStatus = "UnknownVersionedMode" + _, err := NewS3Store(context.Background(), S3StoreConfig{ + Client: client, + Bucket: "backup-bucket", + ServerSideEncryption: string(types.ServerSideEncryptionAes256), + AllowVersionedBucketWithLifecycle: allow, + }) + require.ErrorIs(t, err, ErrIntegrity) + require.ErrorContains(t, err, "unknown versioning status") + } +} + func TestS3StoreRejectsParentDirectoryKeys(t *testing.T) { ctx := context.Background() store := newTestS3Store(t, newFakeS3Client()) @@ -375,6 +433,19 @@ type fakeS3Client struct { nextUploadID int uploadedParts int multipartCompletes int + // Retention/GC listing controls. + listPageSize int + listOmitContinuationToken bool + listRepeatContinuationToken bool + listErr error + listNilOutput bool + listBadKeyCount bool + listCommonPrefixes bool + listModTime time.Time + listPrefixes []string + deletes []string + versioningStatus types.BucketVersioningStatus + versioningErr error } type fakeS3Object struct { @@ -384,6 +455,7 @@ type fakeS3Object struct { checksumType types.ChecksumType serverSideEncryption types.ServerSideEncryption kmsKeyID *string + modifiedAt time.Time } type fakeMultipartUpload struct { @@ -485,6 +557,7 @@ func (c *fakeS3Client) PutObject(_ context.Context, input *s3.PutObjectInput, _ checksumType: s3ChecksumTypeForSHA(input.ChecksumSHA256), serverSideEncryption: input.ServerSideEncryption, kmsKeyID: input.SSEKMSKeyId, + modifiedAt: time.Now().UTC(), } return &s3.PutObjectOutput{}, nil } @@ -507,8 +580,10 @@ func (c *fakeS3Client) HeadObject(_ context.Context, input *s3.HeadObjectInput, Metadata: metadata, ChecksumSHA256: checksum, ChecksumType: checksumType, + LastModified: aws.Time(obj.modifiedAt), ServerSideEncryption: obj.serverSideEncryption, SSEKMSKeyId: obj.kmsKeyID, + ETag: aws.String(fakeS3ETag(obj.body)), }, nil } @@ -532,6 +607,7 @@ func (c *fakeS3Client) GetObject(_ context.Context, input *s3.GetObjectInput, _ Metadata: metadata, ChecksumSHA256: checksum, ChecksumType: checksumType, + LastModified: aws.Time(obj.modifiedAt), ServerSideEncryption: obj.serverSideEncryption, SSEKMSKeyId: obj.kmsKeyID, }, nil @@ -615,6 +691,7 @@ func (c *fakeS3Client) CompleteMultipartUpload( checksumType: s3ChecksumTypeForSHA(upload.checksum), serverSideEncryption: upload.serverSideEncryption, kmsKeyID: upload.kmsKeyID, + modifiedAt: time.Now().UTC(), } c.multipartCompletes++ delete(c.multipart, uploadID) @@ -692,7 +769,7 @@ func (c *fakeS3Client) getAttempts() int { return c.gets } -func (c *fakeS3Client) putRawObject(bucket string, key string, body []byte, metadata map[string]string, checksum *string, checksumType ...types.ChecksumType) { +func (c *fakeS3Client) putRawObject(key string, body []byte, metadata map[string]string, checksum *string, checksumType ...types.ChecksumType) { c.mu.Lock() defer c.mu.Unlock() clonedMetadata := make(map[string]string, len(metadata)) @@ -703,12 +780,13 @@ func (c *fakeS3Client) putRawObject(bucket string, key string, body []byte, meta if len(checksumType) > 0 { storedChecksumType = checksumType[0] } - c.objects[bucket+"/"+key] = fakeS3Object{ + c.objects["backup-bucket/"+key] = fakeS3Object{ body: append([]byte(nil), body...), metadata: clonedMetadata, checksum: checksum, checksumType: storedChecksumType, serverSideEncryption: types.ServerSideEncryptionAes256, + modifiedAt: time.Now().UTC(), } } @@ -726,6 +804,160 @@ func fakeS3ChecksumForMode(mode types.ChecksumMode, obj fakeS3Object) (*string, return obj.checksum, obj.checksumType } +// listMaxKeys lets a test force pagination without uploading 1000 +// objects; zero means "use the request's MaxKeys". +func (c *fakeS3Client) ListObjectsV2( + _ context.Context, input *s3.ListObjectsV2Input, _ ...func(*s3.Options), +) (*s3.ListObjectsV2Output, error) { + c.mu.Lock() + defer c.mu.Unlock() + if c.listErr != nil { + return nil, c.listErr + } + if c.listNilOutput { + return nil, nil + } + + bucketPrefix := aws.ToString(input.Bucket) + "/" + wantPrefix := aws.ToString(input.Prefix) + c.listPrefixes = append(c.listPrefixes, wantPrefix) + keys := c.listObjectKeysLocked(bucketPrefix, wantPrefix) + start, end := c.listPageBounds(keys, input.ContinuationToken) + out := &s3.ListObjectsV2Output{ + Contents: c.listPageContentsLocked(bucketPrefix, keys[start:end]), + KeyCount: aws.Int32(int32(end - start)), + } + if c.listBadKeyCount { + out.KeyCount = aws.Int32(*out.KeyCount + 1) + } + if c.listCommonPrefixes { + out.CommonPrefixes = []types.CommonPrefix{{Prefix: aws.String(wantPrefix + "hidden/")}} + } + c.setListContinuation(out, input, keys, start, end) + return out, nil +} + +func (c *fakeS3Client) listObjectKeysLocked(bucketPrefix, wantPrefix string) []string { + keys := make([]string, 0, len(c.objects)) + for stored := range c.objects { + key, ok := strings.CutPrefix(stored, bucketPrefix) + if !ok || !strings.HasPrefix(key, wantPrefix) { + continue + } + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} + +func (c *fakeS3Client) listPageBounds(keys []string, token *string) (int, int) { + start := 0 + if value := aws.ToString(token); value != "" { + start = sort.SearchStrings(keys, value) + } + pageSize := c.listPageSize + if pageSize <= 0 { + pageSize = len(keys) + } + return start, min(start+pageSize, len(keys)) +} + +func (c *fakeS3Client) listPageContentsLocked(bucketPrefix string, keys []string) []types.Object { + contents := make([]types.Object, 0, len(keys)) + for _, key := range keys { + obj := c.objects[bucketPrefix+key] + modifiedAt := obj.modifiedAt + if !c.listModTime.IsZero() { + modifiedAt = c.listModTime + } + contents = append(contents, types.Object{ + Key: aws.String(key), + Size: aws.Int64(int64(len(obj.body))), + LastModified: aws.Time(modifiedAt), + ETag: aws.String(fakeS3ETag(obj.body)), + }) + } + return contents +} + +func (c *fakeS3Client) setListContinuation( + out *s3.ListObjectsV2Output, + input *s3.ListObjectsV2Input, + keys []string, + start int, + end int, +) { + if end >= len(keys) { + return + } + out.IsTruncated = aws.Bool(true) + if c.listOmitContinuationToken { + return + } + out.NextContinuationToken = aws.String(c.nextListContinuationToken(input, keys, start, end)) +} + +func (c *fakeS3Client) nextListContinuationToken( + input *s3.ListObjectsV2Input, + keys []string, + start int, + end int, +) string { + if !c.listRepeatContinuationToken { + return keys[end] + } + if token := aws.ToString(input.ContinuationToken); token != "" { + return token + } + return keys[start] +} + +func (c *fakeS3Client) listedPrefixes() []string { + c.mu.Lock() + defer c.mu.Unlock() + return append([]string(nil), c.listPrefixes...) +} + +func (c *fakeS3Client) DeleteObject( + _ context.Context, input *s3.DeleteObjectInput, _ ...func(*s3.Options), +) (*s3.DeleteObjectOutput, error) { + c.mu.Lock() + defer c.mu.Unlock() + c.deletes = append(c.deletes, aws.ToString(input.Key)) + storeKey := fakeS3ClientKey(input.Bucket, input.Key) + if want := aws.ToString(input.IfMatch); want != "" { + obj, ok := c.objects[storeKey] + if !ok { + return nil, &types.NotFound{} + } + if fakeS3ETag(obj.body) != want { + return nil, &smithy.GenericAPIError{ + Code: "PreconditionFailed", + Message: "At least one of the pre-conditions you specified did not hold", + } + } + } + delete(c.objects, storeKey) + return &s3.DeleteObjectOutput{}, nil +} + +func (c *fakeS3Client) GetBucketVersioning( + _ context.Context, _ *s3.GetBucketVersioningInput, _ ...func(*s3.Options), +) (*s3.GetBucketVersioningOutput, error) { + c.mu.Lock() + defer c.mu.Unlock() + if c.versioningErr != nil { + return nil, c.versioningErr + } + return &s3.GetBucketVersioningOutput{Status: c.versioningStatus}, nil +} + +// fakeS3ETag derives a stable ETag from the object bytes, matching +// S3's single-part semantics closely enough for the If-Match tests. +func fakeS3ETag(body []byte) string { + return `"` + hexSHA256Bytes(body)[:32] + `"` +} + func fakeS3ClientKey(bucket *string, key *string) string { return aws.ToString(bucket) + "/" + aws.ToString(key) } @@ -737,3 +969,294 @@ func cloneStringPtr(value *string) *string { cloned := *value return &cloned } + +// TestS3StoreListObjectsPagesThroughEveryPage proves the lister +// honours its all-or-error contract across pages. Retention's phase-2 +// safety depends on a complete listing: a payload missed by a +// truncated page would look unreferenced and be reclaimed while a +// committed manifest still points at it. +func TestS3StoreListObjectsPagesThroughEveryPage(t *testing.T) { + ctx := context.Background() + client := newFakeS3Client() + client.listPageSize = 2 + client.listModTime = time.Unix(1_700_000_000, 0).UTC() + store := newTestS3Store(t, client) + + want := make([]string, 0, 5) + for i := range 5 { + key := fmt.Sprintf("cluster-a/v1/payloads/obj-%d.fsm", i) + body := []byte(key) + _, err := store.PutObject(ctx, key, bytes.NewReader(body), PutOptions{ + Size: int64(len(body)), + SHA256: hexSHA256Bytes(body), + }) + require.NoError(t, err) + want = append(want, key) + } + + refs, err := store.ListObjects(ctx, "cluster-a/v1/payloads") + require.NoError(t, err) + + got := make([]string, 0, len(refs)) + for _, ref := range refs { + got = append(got, ref.Key) + require.Equal(t, client.listModTime, ref.UpdatedAt) + require.NotEmpty(t, ref.ETag) + } + sort.Strings(got) + require.Equal(t, want, got, "every page must be returned") +} + +func TestS3StoreListObjectsDelimitsSubtreePrefix(t *testing.T) { + ctx := context.Background() + client := newFakeS3Client() + store := newTestS3Store(t, client) + realKey := "cluster-a/v1/payloads/obj.fsm" + siblingKey := "cluster-a/v1/payloads-archive/obj.fsm" + client.putRawObject(realKey, []byte("real"), nil, nil) + client.putRawObject(siblingKey, []byte("sibling"), nil, nil) + + refs, err := store.ListObjects(ctx, "cluster-a/v1/payloads") + require.NoError(t, err) + + require.Len(t, refs, 1) + require.Equal(t, realKey, refs[0].Key) + require.Equal(t, int64(len("real")), refs[0].Size) + require.False(t, refs[0].UpdatedAt.IsZero()) + require.Equal(t, []string{"cluster-a/v1/payloads/"}, client.listedPrefixes()) +} + +// TestS3StoreListObjectsFailsClosedOnTruncatedPageWithoutToken pins +// the pagination-failure branch: a truncated response with no +// continuation token cannot be continued, so the lister must error +// rather than return the partial set it has. +func TestS3StoreListObjectsFailsClosedOnTruncatedPageWithoutToken(t *testing.T) { + ctx := context.Background() + client := newFakeS3Client() + client.listPageSize = 1 + client.listOmitContinuationToken = true + store := newTestS3Store(t, client) + + for i := range 3 { + key := fmt.Sprintf("cluster-a/v1/payloads/obj-%d.fsm", i) + body := []byte(key) + _, err := store.PutObject(ctx, key, bytes.NewReader(body), PutOptions{ + Size: int64(len(body)), + SHA256: hexSHA256Bytes(body), + }) + require.NoError(t, err) + } + + refs, err := store.ListObjects(ctx, "cluster-a/v1/payloads") + require.Error(t, err) + require.True(t, errors.Is(err, ErrIntegrity)) + require.Nil(t, refs, "a partial listing must never be returned") +} + +func TestS3StoreListObjectsRejectsNonAdvancingContinuationToken(t *testing.T) { + ctx := context.Background() + client := newFakeS3Client() + client.listPageSize = 1 + client.listRepeatContinuationToken = true + store := newTestS3Store(t, client) + + for i := range 3 { + key := fmt.Sprintf("cluster-a/v1/payloads/obj-%d.fsm", i) + client.putRawObject(key, []byte(key), nil, nil) + } + + refs, err := store.ListObjects(ctx, "cluster-a/v1/payloads") + require.Error(t, err) + require.True(t, errors.Is(err, ErrIntegrity)) + require.Nil(t, refs, "a non-advancing token must fail closed") +} + +func TestS3StoreListObjectsFailsClosedOnNilPage(t *testing.T) { + t.Parallel() + + client := newFakeS3Client() + client.listNilOutput = true + store := newTestS3Store(t, client) + + refs, err := store.ListObjects(context.Background(), "cluster-a/v1/payloads") + require.ErrorIs(t, err, ErrIntegrity) + require.Nil(t, refs, "a nil page must never be treated as a complete empty listing") +} + +func TestS3StoreListObjectsRejectsIncompletePage(t *testing.T) { + t.Parallel() + for _, test := range []struct { + name string + configure func(*fakeS3Client) + }{ + {name: "key count mismatch", configure: func(c *fakeS3Client) { c.listBadKeyCount = true }}, + {name: "unexpected common prefixes", configure: func(c *fakeS3Client) { c.listCommonPrefixes = true }}, + } { + t.Run(test.name, func(t *testing.T) { + client := newFakeS3Client() + test.configure(client) + store := newTestS3Store(t, client) + client.putRawObject("cluster-a/v1/groups/1/manifest.json", []byte("manifest"), nil, nil) + refs, err := store.ListObjects(context.Background(), "cluster-a/v1/groups") + require.ErrorIs(t, err, ErrIntegrity) + require.Nil(t, refs, "an incomplete page must not produce a GC live set") + }) + } +} + +func TestS3StoreListObjectsRejectsNoncanonicalKeys(t *testing.T) { + ctx := context.Background() + client := newFakeS3Client() + store := newTestS3Store(t, client) + client.putRawObject("cluster-a/v1/payloads//obj.fsm", []byte("body"), nil, nil) + + refs, err := store.ListObjects(ctx, "cluster-a/v1/payloads") + require.Error(t, err) + require.True(t, errors.Is(err, ErrIntegrity)) + require.Nil(t, refs, "a canonicalized alias must not be returned with another key's identity") +} + +func TestAppendListedObjectsRejectsEntryWithoutKey(t *testing.T) { + t.Parallel() + + refs, err := appendListedObjects(nil, &s3.ListObjectsV2Output{ + Contents: []types.Object{{Key: nil}}, + }, "cluster-a/v1/payloads/") + require.ErrorIs(t, err, ErrIntegrity) + require.Nil(t, refs) +} + +func TestS3StoreDeleteObjectIsIdempotentAndValidatesKeys(t *testing.T) { + ctx := context.Background() + client := newFakeS3Client() + store := newTestS3Store(t, client) + + body := []byte("payload") + key := "cluster-a/v1/payloads/gone.fsm" + _, err := store.PutObject(ctx, key, bytes.NewReader(body), PutOptions{ + Size: int64(len(body)), + SHA256: hexSHA256Bytes(body), + }) + require.NoError(t, err) + + require.NoError(t, store.DeleteObject(ctx, key)) + // Deleting an absent key must stay a no-op so GC is retry-safe. + require.NoError(t, store.DeleteObject(ctx, key)) + require.Equal(t, []string{key, key}, client.deletes) + + require.ErrorIs(t, store.DeleteObject(ctx, "../escape"), ErrInvalidOptions) +} + +// TestS3StoreConditionalDeleteUsesIfMatchAndMapsPreconditionFailure +// covers the exact compare-and-delete the retention race fix depends +// on: S3 must be asked to delete only the version GC validated, and a +// 412 must surface as ErrObjectModified so GC treats it as a skip +// rather than a failure. +func TestS3StoreConditionalDeleteUsesIfMatchAndMapsPreconditionFailure(t *testing.T) { + ctx := context.Background() + client := newFakeS3Client() + store := newTestS3Store(t, client) + + key := "cluster-a/v1/payloads/sha256/ab/cond.fsm" + body := []byte("payload bytes") + _, err := store.PutObject(ctx, key, bytes.NewReader(body), PutOptions{ + Size: int64(len(body)), + SHA256: hexSHA256Bytes(body), + }) + require.NoError(t, err) + + info, ok, err := store.HeadObject(ctx, key) + require.NoError(t, err) + require.True(t, ok) + require.NotEmpty(t, info.ETag, "the store must carry the version token forward") + + // A stale precondition must be refused, and must not delete. + err = store.DeleteObjectIfUnmodified(ctx, key, DeletePrecondition{ETag: `"not-the-current-version"`}) + require.Error(t, err) + require.True(t, errors.Is(err, ErrObjectModified)) + _, stillThere, err := store.HeadObject(ctx, key) + require.NoError(t, err) + require.True(t, stillThere, "a failed precondition must leave the object in place") + + // The observed version deletes. + require.NoError(t, store.DeleteObjectIfUnmodified(ctx, key, DeletePrecondition{ETag: info.ETag})) + _, gone, err := store.HeadObject(ctx, key) + require.NoError(t, err) + require.False(t, gone) + // A concurrent deleter already satisfying the operation is success, + // even when the stale condition no longer has an object to compare. + require.NoError(t, store.DeleteObjectIfUnmodified(ctx, key, DeletePrecondition{ETag: info.ETag})) +} + +// TestS3StoreConditionalDeleteRefusesAnEmptyPrecondition stops a +// silent downgrade to an unconditional delete when neither an etag nor +// a last-modified time is available. +func TestS3StoreConditionalDeleteRefusesAnEmptyPrecondition(t *testing.T) { + ctx := context.Background() + client := newFakeS3Client() + store := newTestS3Store(t, client) + + err := store.DeleteObjectIfUnmodified(ctx, "cluster-a/v1/payloads/sha256/ab/x.fsm", DeletePrecondition{}) + require.ErrorIs(t, err, ErrInvalidOptions) + require.Empty(t, client.deletes, "an unusable precondition must not reach the store") +} + +// TestNextListPageTokenRejectsACompletePageCarryingAToken pins the fail-closed +// handling of a self-contradictory S3 listing response. +// +// An endpoint that reports IsTruncated=false while still returning a +// continuation token has more pages. Believing the flag drops them, and for the +// GC live-set scan a dropped page means a manifest is never seen — so the +// payload it references is absent from the live set and retention reclaims data +// a restore still needs. +func TestNextListPageTokenRejectsACompletePageCarryingAToken(t *testing.T) { + t.Parallel() + + token := "page-2" + truthy := true + falsy := false + + t.Run("complete with a token is a contradiction", func(t *testing.T) { + t.Parallel() + _, more, err := nextListPageToken(&s3.ListObjectsV2Output{ + IsTruncated: &falsy, + NextContinuationToken: &token, + }, "p") + require.ErrorIs(t, err, ErrIntegrity) + require.False(t, more) + }) + + t.Run("omitted IsTruncated with a token is the same contradiction", func(t *testing.T) { + t.Parallel() + _, more, err := nextListPageToken(&s3.ListObjectsV2Output{ + NextContinuationToken: &token, + }, "p") + require.ErrorIs(t, err, ErrIntegrity) + require.False(t, more) + }) + + t.Run("genuinely complete stays complete", func(t *testing.T) { + t.Parallel() + next, more, err := nextListPageToken(&s3.ListObjectsV2Output{IsTruncated: &falsy}, "p") + require.NoError(t, err) + require.False(t, more) + require.Nil(t, next) + }) + + t.Run("truncated with a token continues", func(t *testing.T) { + t.Parallel() + next, more, err := nextListPageToken(&s3.ListObjectsV2Output{ + IsTruncated: &truthy, + NextContinuationToken: &token, + }, "p") + require.NoError(t, err) + require.True(t, more) + require.Equal(t, token, *next) + }) + + t.Run("truncated without a token still fails closed", func(t *testing.T) { + t.Parallel() + _, _, err := nextListPageToken(&s3.ListObjectsV2Output{IsTruncated: &truthy}, "p") + require.ErrorIs(t, err, ErrIntegrity) + }) +} diff --git a/internal/snapshotoffload/scheduler.go b/internal/snapshotoffload/scheduler.go index 2db05eee1..cb165ce0a 100644 --- a/internal/snapshotoffload/scheduler.go +++ b/internal/snapshotoffload/scheduler.go @@ -21,7 +21,7 @@ import ( // compaction behaviour, which §10 lists as a non-goal. type Scheduler struct { groups []OffloadGroup - store ObjectStore + store PublishStore prefix string sourceName string binVersion string @@ -145,7 +145,7 @@ func WithSchedulerSpoolDir(dir string) SchedulerOption { // must supply both leadership callbacks: SyncOnce is exported and does // not re-validate, so a nil callback that survived construction would // be a follower publishing a manifest. -func NewScheduler(store ObjectStore, groups []OffloadGroup, prefix, sourceCluster, binaryVersion string, opts ...SchedulerOption) (*Scheduler, error) { +func NewScheduler(store PublishStore, groups []OffloadGroup, prefix, sourceCluster, binaryVersion string, opts ...SchedulerOption) (*Scheduler, error) { s := &Scheduler{ groups: groups, store: store, @@ -420,6 +420,13 @@ func (s *Scheduler) publishGroup(ctx context.Context, group OffloadGroup) { s.observer.ObserveSnapshotOffloadSkipped(group.GroupID, "no_persisted_snapshot") return } + if errors.Is(err, ErrObjectClaimed) { + // A live peer or an orphaned storage-visible claim owns this + // target. The bounded wait has already elapsed; release the global + // upload slot and retry on a later scheduler tick. + s.observer.ObserveSnapshotOffloadSkipped(group.GroupID, "object_claimed") + return + } s.observer.ObserveSnapshotOffloadFailed(group.GroupID, err) s.logger.WarnContext(ctx, "snapshot offload publish failed", slog.Uint64("group_id", group.GroupID), slog.String("error", err.Error())) diff --git a/internal/snapshotoffload/scheduler_test.go b/internal/snapshotoffload/scheduler_test.go index b788a65ef..3ea109b23 100644 --- a/internal/snapshotoffload/scheduler_test.go +++ b/internal/snapshotoffload/scheduler_test.go @@ -226,7 +226,7 @@ func TestSchedulerRunRequiresStoreAndSourceCluster(t *testing.T) { // newTestScheduler builds a valid scheduler and fails the test if the // configuration is rejected. func newTestScheduler( - t *testing.T, store ObjectStore, groups []OffloadGroup, opts ...SchedulerOption, + t *testing.T, store PublishStore, groups []OffloadGroup, opts ...SchedulerOption, ) *Scheduler { t.Helper() s, err := NewScheduler(store, groups, "cluster-a", "cluster-a", "test", opts...) @@ -473,6 +473,20 @@ type objectLosingStore struct { ObjectStore } +func (s *objectLosingStore) AcquireObjectClaim(ctx context.Context, key string) (ObjectClaim, error) { + claimStore, err := objectClaimStore(s.ObjectStore) + if err != nil { + return nil, err + } + return claimStore.AcquireObjectClaim(ctx, key) +} + +func (s *objectLosingStore) RefreshObject( + ctx context.Context, key string, body io.Reader, opts PutOptions, +) (ObjectInfo, error) { + return refreshWrappedObject(ctx, s.ObjectStore, key, body, opts) +} + func (s *objectLosingStore) PutObject( ctx context.Context, key string, body io.Reader, opts PutOptions, ) (ObjectInfo, error) { diff --git a/internal/snapshotoffload/store.go b/internal/snapshotoffload/store.go index 7049149b6..8dbd61d8a 100644 --- a/internal/snapshotoffload/store.go +++ b/internal/snapshotoffload/store.go @@ -2,13 +2,17 @@ package snapshotoffload import ( "context" + "crypto/rand" "crypto/sha256" "encoding/hex" "io" + iofs "io/fs" "os" "path" "path/filepath" "strings" + "sync" + "time" "github.com/cockroachdb/errors" ) @@ -19,6 +23,302 @@ type ObjectStore interface { HeadObject(ctx context.Context, key string) (ObjectInfo, bool, error) } +// ObjectRefresher rewrites an already-verified object while preserving its +// canonical content. Publish uses this to make reuse of a small manifest +// visible to retention; content-addressed payloads are coordinated by claims +// and are not re-uploaded merely to advance metadata. +type ObjectRefresher interface { + RefreshObject(ctx context.Context, key string, body io.Reader, opts PutOptions) (ObjectInfo, error) +} + +// ObjectClaim is an exclusive claim over one object key. Publishers hold a +// payload claim until its manifest is committed; retention holds the same +// claim while it validates and deletes that payload. The shared claim closes +// the final same-content race that an ETag precondition cannot detect. +type ObjectClaim interface { + Release(ctx context.Context) error +} + +// ObjectClaimStore can acquire a cross-process claim for one object key. +// Implementations must use storage-visible conditional operations rather than +// a process-local mutex: publication and GC may run on different nodes. +type ObjectClaimStore interface { + AcquireObjectClaim(ctx context.Context, key string) (ObjectClaim, error) +} + +// PublishStore is the capability set publication requires. Claims and refresh +// are part of the type instead of runtime assertions because every publish must +// coordinate with retention and remain idempotent when a manifest already +// exists. +type PublishStore interface { + ObjectStore + ObjectClaimStore + ObjectRefresher +} + +// ObjectRef is one object seen by ListObjects. +// +// UpdatedAt is the object store's own last-modified time, not a value +// carried inside the object. Retention uses it only for the phase-2 +// payload grace period, where the question is "has this object been +// sitting unreferenced long enough", which is a property of the store +// rather than of the snapshot. +type ObjectRef struct { + Key string + Size int64 + UpdatedAt time.Time + // ETag is the store's opaque version token for the exact bytes + // observed. It is the precondition DeleteObjectIfUnmodified uses + // to make reclamation a compare-and-delete rather than a + // check-then-delete. Empty when the store cannot supply one, in + // which case the size + mtime pair is the fallback precondition. + ETag string +} + +// DeletePrecondition is the state a caller observed for an object, +// used to make deletion conditional on the object not having changed +// since. +type DeletePrecondition struct { + ETag string + Size int64 + UpdatedAt time.Time +} + +// PreconditionFor returns the precondition describing ref. +func PreconditionFor(ref ObjectRef) DeletePrecondition { + return DeletePrecondition{ETag: ref.ETag, Size: ref.Size, UpdatedAt: ref.UpdatedAt} +} + +// RetentionStore is an ObjectStore that also supports the listing and +// deletion that retention/GC needs (design §5). +// +// It is separate from ObjectStore and PublishStore so restore can use a +// read-only object client, publication requires claims at compile time, and GC +// additionally requires listing and deletion. A silently-no-op GC would be far +// worse: retention would appear configured while the bucket grew without bound. +// +// ListObjects is all-or-error by contract: it MUST return every object +// under prefix or a non-nil error. §5 makes no-deletes-on-partial-scan +// a safety property, and a lister that silently truncated a page would +// make live payloads look unreferenced. +type RetentionStore interface { + ObjectStore + ObjectClaimStore + ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) + DeleteObject(ctx context.Context, key string) error + + // DeleteObjectIfUnmodified deletes key only if it still matches + // cond, and returns ErrObjectModified otherwise. + // + // This is what makes payload reclamation safe against a + // concurrent publish. The grace window alone is not enough: a + // publisher that reuses a content-addressed payload refreshes the + // object to restart its grace, and an unconditional delete can + // still land between GC observing the old state and the publisher + // committing its manifest — leaving a committed manifest pointing + // at bytes that no longer exist. Making the delete conditional on + // the exact state GC validated turns that race into a skip. + DeleteObjectIfUnmodified(ctx context.Context, key string, cond DeletePrecondition) error +} + +var _ RetentionStore = (*LocalStore)(nil) +var _ PublishStore = (*LocalStore)(nil) + +// ListObjects walks the pinned local root below prefix. Directories and +// irregular files are skipped, while symlinks fail the all-or-error listing +// contract closed. Returned keys are slash-separated and relative to the +// store root, matching the keys PutObject accepts. +func (s *LocalStore) ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) { + if s == nil { + return nil, errors.Wrap(ErrInvalidOptions, "object store is required") + } + if err := ctx.Err(); err != nil { + return nil, errors.WithStack(err) + } + listRoot, err := s.listRootForPrefix(prefix) + if err != nil { + return nil, err + } + root, err := s.pinnedRoot() + if err != nil { + return nil, err + } + if err := rejectSymlinkComponents(root, listRoot); err != nil { + return nil, err + } + return s.listObjectsFromRoot(ctx, root, listRoot, prefix) +} + +func (s *LocalStore) listObjectsFromRoot( + ctx context.Context, root *os.Root, listRoot, prefix string, +) ([]ObjectRef, error) { + var refs []ObjectRef + walk := func(walkPath string, entry os.DirEntry, err error) error { + if err != nil { + return s.walkEntryError(walkPath, listRoot, err) + } + if ctxErr := ctx.Err(); ctxErr != nil { + return errors.WithStack(ctxErr) + } + if walkPath == localStoreTempDir && entry.IsDir() { + return iofs.SkipDir + } + ref, ok, refErr := s.objectRefForWalkEntry(walkPath, entry) + if refErr != nil { + return refErr + } + if ok { + refs = append(refs, ref) + } + return nil + } + if err := iofs.WalkDir(root.FS(), listRoot, walk); err != nil { + return nil, errors.Wrapf(err, "list objects under %q", prefix) + } + return refs, nil +} + +// walkEntryError translates a WalkDir error. A missing root is an +// empty listing, not a failure: a bucket that has never been published +// to has no group tree yet, and GC over it must be a clean no-op. +func (s *LocalStore) walkEntryError(walkPath, root string, err error) error { + if os.IsNotExist(err) && walkPath == root { + return iofs.SkipAll + } + return errors.WithStack(err) +} + +// objectRefForWalkEntry converts one walk entry into an ObjectRef, +// reporting ok=false for entries that are not objects (directories and +// sockets). In-progress writes live in the reserved localStoreTempDir subtree, +// which listObjectsFromRoot skips before reaching this helper. +// Symlinks fail the complete-listing contract closed instead of disappearing +// from GC's live-set scan. +func (s *LocalStore) objectRefForWalkEntry(walkPath string, entry os.DirEntry) (ObjectRef, bool, error) { + if entry.Type()&os.ModeSymlink != 0 { + return ObjectRef{}, false, errors.Wrapf(ErrIntegrity, + "object listing encountered symlink %s", walkPath) + } + if entry.IsDir() || !entry.Type().IsRegular() { + return ObjectRef{}, false, nil + } + info, err := entry.Info() + if err != nil { + return ObjectRef{}, false, errors.WithStack(err) + } + return ObjectRef{ + Key: path.Clean(filepath.ToSlash(walkPath)), + Size: info.Size(), + UpdatedAt: info.ModTime(), + }, true, nil +} + +// listRootForPrefix resolves the directory a listing should walk. +// +// cleanObjectPrefix preserves ".." segments, so passing it blindly to the +// root-relative walker would attempt to escape the store. Every other +// local-store operation rejects the equivalent key through pathForKey. +func (s *LocalStore) listRootForPrefix(prefix string) (string, error) { + cleaned := cleanObjectPrefix(prefix) + if cleaned == "." { + return ".", nil + } + // cleanObjectPrefix uses path (slash) semantics, while os.Root interprets + // the platform separator too. On Windows a prefix like `..\sibling` + // would survive a slash-only check, so reject the native form as well. + if !objectPathSegmentIsSafe(cleaned) { + return "", errors.Wrapf(ErrInvalidOptions, "invalid object prefix %q", prefix) + } + return cleaned, nil +} + +// DeleteObjectIfUnmodified removes key only when it still matches +// cond. See the RetentionStore contract for why the condition matters. +func (s *LocalStore) DeleteObjectIfUnmodified(ctx context.Context, key string, cond DeletePrecondition) error { + if err := ctx.Err(); err != nil { + return errors.WithStack(err) + } + relPath, err := s.relPathForKey(key) + if err != nil { + return err + } + root, err := s.pinnedRoot() + if err != nil { + return err + } + if err := rejectSymlinkComponents(root, relPath); err != nil { + return err + } + + s.deleteMu.Lock() + defer s.deleteMu.Unlock() + + exists, err := validateDeletePreconditionWithinRoot(root, key, relPath, cond) + if err != nil || !exists { + return err + } + removed, err := removeWithinRoot(root, relPath) + if err != nil { + return errors.Wrapf(err, "delete object %s", key) + } + if !removed { + return nil + } + // Persist the unlink before reporting success. Without the + // directory sync a crash can resurrect an object GC already + // counted as reclaimed. + return syncDirWithinRoot(root, filepath.Dir(relPath)) +} + +func validateDeletePreconditionWithinRoot( + root *os.Root, key, relPath string, cond DeletePrecondition, +) (bool, error) { + stat, err := root.Stat(relPath) + if err != nil { + if os.IsNotExist(err) { + // Already gone: the caller's intent is satisfied. + return false, nil + } + return false, errors.Wrapf(err, "stat object %s", key) + } + if stat.Size() != cond.Size || !stat.ModTime().Equal(cond.UpdatedAt) { + return false, errors.Wrapf(ErrObjectModified, + "object %s changed since it was validated for deletion", key) + } + return true, nil +} + +// DeleteObject removes one object. A already-absent object is not an +// error: GC must be idempotent across retries and a concurrent +// reclamation of the same key is a benign race. +func (s *LocalStore) DeleteObject(ctx context.Context, key string) error { + if err := ctx.Err(); err != nil { + return errors.WithStack(err) + } + relPath, err := s.relPathForKey(key) + if err != nil { + return err + } + root, err := s.pinnedRoot() + if err != nil { + return err + } + if err := rejectSymlinkComponents(root, relPath); err != nil { + return err + } + removed, err := removeWithinRoot(root, relPath) + if err != nil { + return errors.Wrapf(err, "delete object %s", key) + } + if !removed { + return nil + } + // Persist the unlink before reporting success. Without the + // directory sync a crash can resurrect an object GC already + // counted as reclaimed. + return syncDirWithinRoot(root, filepath.Dir(relPath)) +} + type PutOptions struct { Size int64 SHA256 string @@ -26,66 +326,162 @@ type PutOptions struct { } type ObjectInfo struct { - Key string - Size int64 + Key string + Size int64 + UpdatedAt time.Time // SHA256 is optional for metadata-only Head/Get paths; PutObject returns it // when the writer verified the committed content. SHA256 string ServerSideEncryption string SSEKMSKeyID string + // ETag is the store's opaque version token, when it supplies one. + // Retention uses it as the compare-and-delete precondition. + ETag string } type LocalStore struct { root string + // deleteMu serialises conditional deletes against refreshes made + // through this same store value. Within one process that makes + // compare-and-delete atomic. It cannot coordinate two processes + // sharing a directory — POSIX has no compare-and-unlink — so a + // cross-process local deployment keeps the residual race that the + // S3 store closes with If-Match. LocalStore is the dev/test and + // single-writer store; production offload targets S3. + deleteMu sync.Mutex + + // rootMu protects the pinned descriptor's ownership. Close is only valid + // after callers have stopped issuing operations. + rootMu sync.RWMutex + // rootDir is the configured directory, opened by NewLocalStore and kept + // for the life of the store. See pinnedRoot. + rootDir *os.Root } -const localStoreDirPerm = 0o755 +const ( + localStoreDirPerm = 0o755 + localStoreFilePerm = 0o600 + localStoreTempTokenBytes = 16 + localStoreTempCreateAttempts = 10 + localStoreTempDir = ".snapshotoffload-tmp" + localStoreTempDirPerm = 0o700 +) func NewLocalStore(root string) (*LocalStore, error) { if strings.TrimSpace(root) == "" { return nil, errors.Wrap(ErrInvalidOptions, "local store root is required") } - return &LocalStore{root: filepath.Clean(root)}, nil + cleaned := filepath.Clean(root) + if err := os.MkdirAll(cleaned, localStoreDirPerm); err != nil { + return nil, errors.Wrapf(err, "create local store root %s", cleaned) + } + rootDir, err := os.OpenRoot(cleaned) + if err != nil { + return nil, errors.Wrapf(err, "open local store root %s", cleaned) + } + return &LocalStore{root: cleaned, rootDir: rootDir}, nil +} + +// Close releases the descriptor pinned by NewLocalStore. Callers must stop all +// store operations before closing it. Close is idempotent. +func (s *LocalStore) Close() error { + if s == nil { + return nil + } + s.rootMu.Lock() + defer s.rootMu.Unlock() + if s.rootDir == nil { + return nil + } + err := s.rootDir.Close() + s.rootDir = nil + if err != nil { + return errors.WithStack(err) + } + return nil } func (s *LocalStore) PutObject(ctx context.Context, key string, body io.Reader, opts PutOptions) (ObjectInfo, error) { if err := validatePutOptions(opts); err != nil { return ObjectInfo{}, err } - finalPath, err := s.pathForKey(key) + relPath, err := s.relPathForKey(key) + if err != nil { + return ObjectInfo{}, err + } + root, err := s.pinnedRoot() if err != nil { return ObjectInfo{}, err } - if err := os.MkdirAll(filepath.Dir(finalPath), localStoreDirPerm); err != nil { + if err := rejectSymlinkComponents(root, relPath); err != nil { + return ObjectInfo{}, err + } + if err := root.MkdirAll(filepath.Dir(relPath), localStoreDirPerm); err != nil { return ObjectInfo{}, errors.WithStack(err) } - tmpPath, info, err := writeLocalObjectTemp(ctx, filepath.Dir(finalPath), key, body, opts) + tmpPath, info, err := writeLocalObjectTempWithinRoot(ctx, root, key, body, opts) if err != nil { return ObjectInfo{}, err } - defer func() { _ = os.Remove(tmpPath) }() - return s.commitTempObject(key, tmpPath, finalPath, info) + defer func() { _ = root.Remove(tmpPath) }() + return commitTempObjectWithinRoot(root, key, tmpPath, relPath, info) +} + +func (s *LocalStore) RefreshObject(ctx context.Context, key string, body io.Reader, opts PutOptions) (ObjectInfo, error) { + if err := validatePutOptions(opts); err != nil { + return ObjectInfo{}, err + } + relPath, err := s.relPathForKey(key) + if err != nil { + return ObjectInfo{}, err + } + root, err := s.pinnedRoot() + if err != nil { + return ObjectInfo{}, err + } + if err := rejectSymlinkComponents(root, relPath); err != nil { + return ObjectInfo{}, err + } + tmpPath, info, err := writeLocalObjectTempWithinRoot(ctx, root, key, body, opts) + if err != nil { + return ObjectInfo{}, err + } + defer func() { _ = root.Remove(tmpPath) }() + // Held across the replace so an in-process reclamation of this + // same payload cannot land between a validating stat and the + // unlink. See DeleteObjectIfUnmodified. + s.deleteMu.Lock() + defer s.deleteMu.Unlock() + return replaceObjectWithinRoot(root, key, tmpPath, relPath, info) } func (s *LocalStore) GetObject(ctx context.Context, key string) (io.ReadCloser, ObjectInfo, error) { if err := ctx.Err(); err != nil { return nil, ObjectInfo{}, errors.WithStack(err) } - objectPath, err := s.pathForKey(key) + relPath, err := s.relPathForKey(key) if err != nil { return nil, ObjectInfo{}, err } - info, err := s.objectInfoForPath(key, objectPath) + root, err := s.pinnedRoot() if err != nil { return nil, ObjectInfo{}, err } - file, err := os.Open(objectPath) + if err := rejectSymlinkComponents(root, relPath); err != nil { + return nil, ObjectInfo{}, err + } + file, err := root.Open(relPath) if err != nil { if os.IsNotExist(err) { return nil, ObjectInfo{}, errors.Wrapf(ErrObjectNotFound, "object %s", key) } return nil, ObjectInfo{}, errors.WithStack(err) } + info, err := objectInfoFromFile(key, file) + if err != nil { + _ = file.Close() + return nil, ObjectInfo{}, err + } return file, info, nil } @@ -93,18 +489,26 @@ func (s *LocalStore) HeadObject(ctx context.Context, key string) (ObjectInfo, bo if err := ctx.Err(); err != nil { return ObjectInfo{}, false, errors.WithStack(err) } - objectPath, err := s.pathForKey(key) + relPath, err := s.relPathForKey(key) + if err != nil { + return ObjectInfo{}, false, err + } + root, err := s.pinnedRoot() if err != nil { return ObjectInfo{}, false, err } - info, err := s.objectInfoForPath(key, objectPath) + if err := rejectSymlinkComponents(root, relPath); err != nil { + return ObjectInfo{}, false, err + } + stat, err := root.Stat(relPath) if err != nil { - if errors.Is(err, ErrObjectNotFound) { + if os.IsNotExist(err) { return ObjectInfo{}, false, nil } - return ObjectInfo{}, false, err + return ObjectInfo{}, false, errors.WithStack(err) } - return info, true, nil + info, err := objectInfoFromStat(key, stat) + return info, err == nil, err } func (s *LocalStore) pathForKey(key string) (string, error) { @@ -112,38 +516,213 @@ func (s *LocalStore) pathForKey(key string) (string, error) { return "", errors.Wrap(ErrInvalidOptions, "object store is required") } normalized := normalizeObjectKey(key) - if normalized == "" || normalized == "." || normalized == ".." || strings.HasPrefix(normalized, "../") { + if !objectPathSegmentIsSafe(normalized) { return "", errors.Wrapf(ErrInvalidOptions, "invalid object key %q", key) } - return filepath.Join(s.root, filepath.FromSlash(normalized)), nil + joined := filepath.Join(s.root, filepath.FromSlash(normalized)) + if !objectPathWithinRoot(s.root, joined) { + return "", errors.Wrapf(ErrInvalidOptions, "object key %q resolves outside the store root", key) + } + return joined, nil } -func (s *LocalStore) objectInfoForPath(key, objectPath string) (ObjectInfo, error) { - stat, err := os.Stat(objectPath) - if err != nil { +// objectPathSegmentIsSafe reports whether a normalized key or prefix +// stays inside the store root once joined. +// +// The backslash check is not redundant on the slash-only forms: +// normalizeObjectKey uses path (slash) semantics, so `..\victim` +// survives every "/"-based test, and filepath.Join then interprets the +// backslash on Windows and resolves outside the root. Both the key +// path (Get/Head/Put/Delete) and the prefix path (ListObjects) route +// through this, so a fix here cannot be applied to one and missed on +// the other. +func objectPathSegmentIsSafe(normalized string) bool { + switch { + case normalized == "", normalized == ".", normalized == "..": + return false + case normalized == localStoreTempDir, strings.HasPrefix(normalized, localStoreTempDir+"/"): + return false + case strings.HasPrefix(normalized, "../"): + return false + case strings.ContainsRune(normalized, '\\'): + return false + case strings.HasPrefix(normalized, "/"): + // normalizeObjectKey trims ONE leading slash, so `//victim` + // arrives here as `/victim`: relative by none of the checks + // above, but rooted. filepath.Join("C:", "/victim") resolves to + // `C:\victim` against a drive-relative root, outside the store. + // A rooted key is never legitimate -- object keys are relative + // to the root by definition -- so reject rather than re-trim. + return false + case volumeQualified(normalized): + return false + default: + return true + } +} + +// volumeQualified reports whether the first path segment carries a +// Windows volume, as in `C:` or `C:foo`. +// +// filepath.VolumeName is deliberately not used: it returns "" on +// non-Windows, so a test running on Linux or macOS would pass against a +// key that escapes on Windows. The check is spelled out so it behaves +// identically on every platform. +func volumeQualified(normalized string) bool { + first := normalized + if idx := strings.IndexByte(first, '/'); idx >= 0 { + first = first[:idx] + } + idx := strings.IndexByte(first, ':') + if idx < 0 { + return false + } + // A single letter before the colon is a drive designator. Anything + // else containing a colon is still refused below by the caller's + // containment check, but keys like `a:b` are not volume-qualified. + return idx == 1 && isASCIILetter(first[0]) +} + +func isASCIILetter(c byte) bool { + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') +} + +// removeWithinRoot unlinks relPath RELATIVE TO the store root, through a +// descriptor that cannot escape it. +// +// objectPathWithinRoot is lexical, so it cannot see a symlink: if any ancestor +// under the root is a symlink -- or is swapped for one between the listing and +// the delete -- os.Remove follows it and unlinks a file outside the configured +// root. The character checks cannot close that, because the path they +// validated is still the path being passed; it is the RESOLUTION that differs. +// +// os.Root resolves every component against the opened root descriptor and +// refuses to traverse out of it, so the check and the operation can no longer +// disagree. +func removeWithinRoot(root *os.Root, relPath string) (bool, error) { + if err := root.Remove(relPath); err != nil { if os.IsNotExist(err) { - return ObjectInfo{}, errors.Wrapf(ErrObjectNotFound, "object %s", key) + return false, nil } - return ObjectInfo{}, errors.WithStack(err) + return false, errors.Wrapf(err, "remove %s within store root", relPath) } + return true, nil +} + +// pinnedRoot returns the descriptor NewLocalStore opened for the configured +// directory. Opening it during construction is essential: delaying the open +// until the first operation would let a rename-and-symlink swap redirect that +// first operation before the descriptor was pinned. +// +// Reopening per operation re-resolves a MUTABLE PATHNAME. os.OpenRoot follows +// symlinks in its own argument, so a process that can write the root's PARENT +// can rename the root away and leave a symlink in its place between two +// operations; the next open then anchors the descriptor outside the configured +// directory, and a descendant-relative Remove unlinks an external file at the +// corresponding key. That is a different hole from a descendant symlink, which +// os.Root already refuses to traverse -- here the escape is in the root +// argument itself, before any resolution the descriptor governs. +// +// Holding the descriptor means every later operation resolves against the +// directory this store was configured with, whatever the pathname comes to +// point at afterwards. The descriptor lives until Close; callers must not race +// Close with an operation that has already obtained the descriptor. +func (s *LocalStore) pinnedRoot() (*os.Root, error) { + if s == nil { + return nil, errors.Wrap(ErrInvalidOptions, "object store root is required") + } + s.rootMu.RLock() + defer s.rootMu.RUnlock() + if s.rootDir == nil { + return nil, errors.Wrap(ErrInvalidOptions, "object store root is required") + } + return s.rootDir, nil +} + +// relPathForKey is pathForKey's root-relative half, for the +// descriptor-relative operations that resolve against the root themselves. +func (s *LocalStore) relPathForKey(key string) (string, error) { + full, err := s.pathForKey(key) + if err != nil { + return "", err + } + rel, err := filepath.Rel(filepath.Clean(s.root), full) + if err != nil { + return "", errors.Wrapf(ErrInvalidOptions, "object key %q is not under the store root", key) + } + return rel, nil +} + +func rejectSymlinkComponents(root *os.Root, relPath string) error { + cleaned := filepath.Clean(relPath) + if cleaned == "." { + return nil + } + current := "" + for _, component := range strings.Split(cleaned, string(filepath.Separator)) { + current = filepath.Join(current, component) + info, err := root.Lstat(current) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return errors.Wrapf(err, "inspect object path component %s", current) + } + if info.Mode()&os.ModeSymlink != 0 { + return errors.Wrapf(ErrIntegrity, "object path component %s is a symlink", current) + } + } + return nil +} + +// objectPathWithinRoot reports whether joined actually resolves inside +// root. +// +// This is the backstop for the character-level checks above: those +// enumerate the escapes we know about, and this one states the property +// we actually need. filepath.Rel answers it using the platform's own +// separator and volume rules, so an escape neither check anticipated +// still fails here instead of reaching the filesystem. +// +// It is deliberately unreachable today: every escape currently known is +// refused earlier by objectPathSegmentIsSafe, so no input reaches the +// store and trips this instead. That is the intended relationship +// between the two layers, not a missing case -- a test that exercised +// this through pathForKey would mean the character checks had a hole. +// TestObjectPathWithinRootIsTheBackstop therefore covers the function +// directly; if it ever starts firing in production, the character +// checks need a new case rather than this one being relaxed. +func objectPathWithinRoot(root, joined string) bool { + rel, err := filepath.Rel(filepath.Clean(root), filepath.Clean(joined)) + if err != nil { + return false + } + if rel == "." || rel == ".." { + return false + } + return !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + +func objectInfoFromStat(key string, stat os.FileInfo) (ObjectInfo, error) { if !stat.Mode().IsRegular() { return ObjectInfo{}, errors.Wrapf(ErrInvalidOptions, "object %s is not a regular file", key) } return ObjectInfo{ - Key: normalizeObjectKey(key), - Size: stat.Size(), + Key: normalizeObjectKey(key), + Size: stat.Size(), + UpdatedAt: stat.ModTime(), }, nil } -func (s *LocalStore) hashedObjectInfoForPath(key, objectPath string) (ObjectInfo, error) { - file, err := os.Open(objectPath) +func objectInfoFromFile(key string, file *os.File) (ObjectInfo, error) { + stat, err := file.Stat() if err != nil { - if os.IsNotExist(err) { - return ObjectInfo{}, errors.Wrapf(ErrObjectNotFound, "object %s", key) - } return ObjectInfo{}, errors.WithStack(err) } - defer func() { _ = file.Close() }() + return objectInfoFromStat(key, stat) +} + +func hashedObjectInfoFromFile(key string, file *os.File) (ObjectInfo, error) { stat, err := file.Stat() if err != nil { return ObjectInfo{}, errors.WithStack(err) @@ -156,72 +735,162 @@ func (s *LocalStore) hashedObjectInfoForPath(key, objectPath string) (ObjectInfo return ObjectInfo{}, errors.WithStack(err) } return ObjectInfo{ - Key: normalizeObjectKey(key), - Size: stat.Size(), - SHA256: hex.EncodeToString(sum.Sum(nil)), + Key: normalizeObjectKey(key), + Size: stat.Size(), + UpdatedAt: stat.ModTime(), + SHA256: hex.EncodeToString(sum.Sum(nil)), }, nil } -func writeLocalObjectTemp( +func writeLocalObjectTempWithinRoot( ctx context.Context, - dir string, + root *os.Root, key string, body io.Reader, opts PutOptions, ) (string, ObjectInfo, error) { - tmp, err := os.CreateTemp(dir, ".put-*") + tmpPath, tmp, err := createLocalTempWithinRoot(root) if err != nil { - return "", ObjectInfo{}, errors.WithStack(err) + return "", ObjectInfo{}, err } - tmpPath := tmp.Name() keep := false defer func() { if !keep { _ = tmp.Close() - _ = os.Remove(tmpPath) + _ = root.Remove(tmpPath) } }() + info, err := writeLocalObjectContents(ctx, tmp, key, body, opts) + if err != nil { + return "", ObjectInfo{}, err + } + if err := tmp.Close(); err != nil { + return "", ObjectInfo{}, errors.WithStack(err) + } + keep = true + return tmpPath, info, nil +} + +func createLocalTempWithinRoot(root *os.Root) (string, *os.File, error) { + if err := root.MkdirAll(localStoreTempDir, localStoreTempDirPerm); err != nil { + return "", nil, errors.Wrap(err, "create local store temp directory") + } + for range localStoreTempCreateAttempts { + var token [localStoreTempTokenBytes]byte + if _, err := rand.Read(token[:]); err != nil { + return "", nil, errors.Wrap(err, "generate local object temp name") + } + name := filepath.Join(localStoreTempDir, ".put-"+hex.EncodeToString(token[:])) + file, err := root.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, localStoreFilePerm) + if err == nil { + return name, file, nil + } + if !os.IsExist(err) { + return "", nil, errors.Wrap(err, "create local object temp file") + } + } + return "", nil, errors.Wrap(ErrObjectConflict, "exhausted local object temp names") +} + +func writeLocalObjectContents( + ctx context.Context, + tmp *os.File, + key string, + body io.Reader, + opts PutOptions, +) (ObjectInfo, error) { sum := sha256.New() n, err := io.Copy(io.MultiWriter(tmp, sum), contextReader{ctx: ctx, reader: body}) if err != nil { - return "", ObjectInfo{}, errors.WithStack(err) + return ObjectInfo{}, errors.WithStack(err) } gotSHA := hex.EncodeToString(sum.Sum(nil)) if n != opts.Size { - return "", ObjectInfo{}, errors.Wrapf(ErrIntegrity, "object %s wrote %d bytes, expected %d", key, n, opts.Size) + return ObjectInfo{}, errors.Wrapf(ErrIntegrity, "object %s wrote %d bytes, expected %d", key, n, opts.Size) } if gotSHA != opts.SHA256 { - return "", ObjectInfo{}, errors.Wrapf(ErrIntegrity, "object %s wrote sha256 %s, expected %s", key, gotSHA, opts.SHA256) + return ObjectInfo{}, errors.Wrapf(ErrIntegrity, "object %s wrote sha256 %s, expected %s", key, gotSHA, opts.SHA256) } if err := tmp.Sync(); err != nil { - return "", ObjectInfo{}, errors.WithStack(err) - } - if err := tmp.Close(); err != nil { - return "", ObjectInfo{}, errors.WithStack(err) + return ObjectInfo{}, errors.WithStack(err) } - keep = true - return tmpPath, ObjectInfo{Key: normalizeObjectKey(key), Size: opts.Size, SHA256: opts.SHA256}, nil + return ObjectInfo{Key: normalizeObjectKey(key), Size: opts.Size, SHA256: opts.SHA256}, nil } -func (s *LocalStore) commitTempObject(key, tmpPath, finalPath string, expected ObjectInfo) (ObjectInfo, error) { - if err := os.Link(tmpPath, finalPath); err != nil { +func commitTempObjectWithinRoot( + root *os.Root, + key string, + tmpPath string, + finalPath string, + expected ObjectInfo, +) (ObjectInfo, error) { + if err := root.Link(tmpPath, finalPath); err != nil { if !os.IsExist(err) { return ObjectInfo{}, errors.WithStack(err) } - return s.verifyExistingObject(key, finalPath, expected) + return verifyExistingObjectWithinRoot(root, key, finalPath, expected) } finalDir := filepath.Dir(finalPath) - if err := syncDir(finalDir); err != nil { - if removeErr := os.Remove(finalPath); removeErr != nil && !os.IsNotExist(removeErr) { + if err := syncDirWithinRoot(root, finalDir); err != nil { + if removeErr := root.Remove(finalPath); removeErr != nil && !os.IsNotExist(removeErr) { err = errors.CombineErrors(err, errors.WithStack(removeErr)) } return ObjectInfo{}, errors.WithStack(err) } - return expected, nil + return verifyExistingObjectWithinRoot(root, key, finalPath, expected) +} + +func replaceObjectWithinRoot( + root *os.Root, + key string, + tmpPath string, + finalPath string, + expected ObjectInfo, +) (ObjectInfo, error) { + if err := root.Rename(tmpPath, finalPath); err != nil { + return ObjectInfo{}, errors.WithStack(err) + } + if err := syncDirWithinRoot(root, filepath.Dir(finalPath)); err != nil { + return ObjectInfo{}, err + } + info, err := hashedObjectInfoWithinRoot(root, key, finalPath) + if err != nil { + return ObjectInfo{}, err + } + if info.Size == expected.Size && info.SHA256 == expected.SHA256 { + return info, nil + } + return ObjectInfo{}, errors.Wrapf(ErrIntegrity, "object %s changed during refresh", key) +} + +func syncDirWithinRoot(root *os.Root, dir string) error { + file, err := root.Open(dir) + if err != nil { + return errors.WithStack(err) + } + defer func() { _ = file.Close() }() + if err := file.Sync(); err != nil { + return errors.WithStack(err) + } + return nil +} + +func hashedObjectInfoWithinRoot(root *os.Root, key, objectPath string) (ObjectInfo, error) { + file, err := root.Open(objectPath) + if err != nil { + if os.IsNotExist(err) { + return ObjectInfo{}, errors.Wrapf(ErrObjectNotFound, "object %s", key) + } + return ObjectInfo{}, errors.WithStack(err) + } + defer func() { _ = file.Close() }() + return hashedObjectInfoFromFile(key, file) } -func (s *LocalStore) verifyExistingObject(key, finalPath string, expected ObjectInfo) (ObjectInfo, error) { - info, err := s.hashedObjectInfoForPath(key, finalPath) +func verifyExistingObjectWithinRoot( + root *os.Root, key, finalPath string, expected ObjectInfo, +) (ObjectInfo, error) { + info, err := hashedObjectInfoWithinRoot(root, key, finalPath) if err != nil { return ObjectInfo{}, err } From 4082ec12c61544e7cb5c85d6426e1e3912822338 Mon Sep 17 00:00:00 2001 From: bootjp Date: Thu, 22 Oct 2026 22:40:01 +0900 Subject: [PATCH 2/3] test: bound fake S3 list page counts before conversion --- internal/snapshotoffload/s3_store_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/snapshotoffload/s3_store_test.go b/internal/snapshotoffload/s3_store_test.go index 5543b71f1..337313641 100644 --- a/internal/snapshotoffload/s3_store_test.go +++ b/internal/snapshotoffload/s3_store_test.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "io" + "math" "sort" "strings" "sync" @@ -823,9 +824,13 @@ func (c *fakeS3Client) ListObjectsV2( c.listPrefixes = append(c.listPrefixes, wantPrefix) keys := c.listObjectKeysLocked(bucketPrefix, wantPrefix) start, end := c.listPageBounds(keys, input.ContinuationToken) + pageSize := end - start + if pageSize < 0 || pageSize >= math.MaxInt32 { + return nil, errors.New("fake s3 list page size exceeds key count range") + } out := &s3.ListObjectsV2Output{ Contents: c.listPageContentsLocked(bucketPrefix, keys[start:end]), - KeyCount: aws.Int32(int32(end - start)), + KeyCount: aws.Int32(int32(pageSize)), } if c.listBadKeyCount { out.KeyCount = aws.Int32(*out.KeyCount + 1) From 8671973fbeeca4c7cb3f14d8995d03eb447e89d4 Mon Sep 17 00:00:00 2001 From: bootjp Date: Thu, 22 Oct 2026 22:55:55 +0900 Subject: [PATCH 3/3] snapshotoffload: gate reclamation on publisher rollout and bucket policy --- cmd/elastickv-snapshot-offload/main.go | 4 +- ...artial_physical_snapshot_object_offload.md | 23 ++- internal/snapshotoffload/retention.go | 44 +++-- internal/snapshotoffload/retention_test.go | 185 +++++++++++++----- internal/snapshotoffload/s3_store.go | 41 ++-- internal/snapshotoffload/s3_store_test.go | 57 +++++- internal/snapshotoffload/store.go | 8 + 7 files changed, 277 insertions(+), 85 deletions(-) diff --git a/cmd/elastickv-snapshot-offload/main.go b/cmd/elastickv-snapshot-offload/main.go index 137a1bcbc..ffbf34caf 100644 --- a/cmd/elastickv-snapshot-offload/main.go +++ b/cmd/elastickv-snapshot-offload/main.go @@ -188,7 +188,7 @@ func addStoreFlags(fs *flag.FlagSet, cfg *storeFlags) { fs.StringVar(&cfg.storeKind, "store", storeLocal, "Object store backend: local or s3") fs.StringVar(&cfg.localRoot, "local-root", "", "Local object store root when --store=local") fs.StringVar(&cfg.s3Bucket, "s3-bucket", "", - "S3 bucket when --store=s3; credentials require s3:GetBucketVersioning") + "S3 bucket when --store=s3") fs.StringVar(&cfg.s3Region, "s3-region", cfg.s3Region, "S3 signing region") fs.StringVar(&cfg.s3Endpoint, "s3-endpoint", "", "S3-compatible endpoint URL") fs.StringVar(&cfg.s3Profile, "s3-profile", "", "AWS shared config profile") @@ -196,7 +196,7 @@ func addStoreFlags(fs *flag.FlagSet, cfg *storeFlags) { fs.StringVar(&cfg.s3ServerSideEncryption, "s3-sse", cfg.s3ServerSideEncryption, "Server-side encryption algorithm for uploaded objects: AES256 or aws:kms") fs.StringVar(&cfg.s3KMSKeyID, "s3-kms-key-id", "", "KMS key ARN or bare key ID when --s3-sse=aws:kms; aliases are rejected") fs.BoolVar(&cfg.s3DisableChecksumHeaders, "s3-disable-checksum-headers", false, "Do not send S3 checksum headers; keep metadata and restore-time verification") - fs.BoolVar(&cfg.s3AllowVersionedLifecycle, "s3-allow-versioned-with-lifecycle", false, "Allow an S3 versioned bucket after confirming noncurrent-version expiration is configured") + fs.BoolVar(&cfg.s3AllowVersionedLifecycle, "s3-allow-versioned-with-lifecycle", false, "Allow retention on an S3 versioned bucket after confirming noncurrent-version expiration is configured; not required for restore") } func validateStoreFlags(cfg storeFlags) error { diff --git a/docs/design/2026_07_19_partial_physical_snapshot_object_offload.md b/docs/design/2026_07_19_partial_physical_snapshot_object_offload.md index bbdba59ed..a3d43143a 100644 --- a/docs/design/2026_07_19_partial_physical_snapshot_object_offload.md +++ b/docs/design/2026_07_19_partial_physical_snapshot_object_offload.md @@ -203,17 +203,34 @@ providers and must not appear in process arguments or manifests. bucket with S3 versioning enabled a keyed delete only writes a delete marker, so the bytes persist as a noncurrent version that later listings cannot see: GC reports successful reclamation while storage -grows without bound. `NewS3Store` therefore checks bucket versioning and +grows without bound. Each GC pass calls `RetentionStore.ValidateRetention` +to check bucket versioning before scanning or deleting anything, and rejects `Enabled` or `Suspended` by default. Operators that have already configured noncurrent-version expiration must acknowledge that policy explicitly with `--s3-allow-versioned-with-lifecycle`; the code cannot -prove the lifecycle configuration itself. +prove the lifecycle configuration itself. Store construction and read-only +restore do not query versioning or require this acknowledgement. Unknown +versioning modes and versioning-query failures abort GC. + +**Claims rollout gate.** GC is disabled unless its caller supplies +`GCOptions.VerifyPublishersUseClaims`. Before every pass, this callback must +confirm that all publishers sharing the prefix, including standalone CLI jobs, +use the claims protocol and that legacy in-flight publications have drained. +Upgrade publishers first while GC is disabled, drain/stop all legacy jobs, then +enable the verifier. Before rollback or restarting any legacy publisher, make +the verifier fail, stop and drain any active GC pass, and only then resume the +legacy publisher. Claims cannot protect a publisher that ignores them; a +process-local build version or successful claim acquisition is not evidence +that the rollout is complete. Runtime verifier wiring is part of the separate +runtime integration work; `NewGC` refuses a missing verifier. Storage-envelope encryption protects values but not all physical keys and metadata. The external bucket therefore requires private ACLs, TLS, and server-side encryption (SSE-S3 or SSE-KMS). Anonymous reads and writes are a deployment failure. Object credentials need only scoped list/get/put/delete -permissions below the configured prefix, plus `s3:GetBucketVersioning`. +permissions below the configured prefix. GC additionally requires +`s3:GetBucketVersioning`; a restore role needs only object reads (and any +applicable KMS decrypt permission). ## 8. Milestones diff --git a/internal/snapshotoffload/retention.go b/internal/snapshotoffload/retention.go index 3705e0cbb..9c29084d9 100644 --- a/internal/snapshotoffload/retention.go +++ b/internal/snapshotoffload/retention.go @@ -127,11 +127,12 @@ type GCResult struct { // GC implements the §5 retention and payload-reclamation passes over // one object-store prefix. type GC struct { - store RetentionStore - prefix string - policy RetentionPolicy - now func() time.Time - log *slog.Logger + store RetentionStore + prefix string + policy RetentionPolicy + now func() time.Time + log *slog.Logger + verifyPublishersUseClaims func(context.Context) error // marks is the §5 two-pass sweep state: the first pass that finds // a payload unreferenced and past grace records what it saw, and @@ -168,6 +169,11 @@ type GCOptions struct { Store RetentionStore Prefix string Policy RetentionPolicy + // VerifyPublishersUseClaims must confirm that every publisher sharing + // this prefix uses object claims and all legacy in-flight publishes have + // drained. It runs before each pass; fail it before allowing a downgrade + // or a legacy publisher to resume. A missing verifier disables GC. + VerifyPublishersUseClaims func(context.Context) error // Now defaults to time.Now. Injected so tests can drive the age // windows deterministically rather than by sleeping. Now func() time.Time @@ -178,6 +184,9 @@ func NewGC(opts GCOptions) (*GC, error) { if opts.Store == nil { return nil, errors.Wrap(ErrInvalidOptions, "retention requires an object store") } + if opts.VerifyPublishersUseClaims == nil { + return nil, errors.Wrap(ErrInvalidOptions, "retention requires a publisher claims rollout verifier") + } now := opts.Now if now == nil { now = time.Now @@ -187,23 +196,30 @@ func NewGC(opts GCOptions) (*GC, error) { log = slog.Default() } return &GC{ - store: opts.Store, - prefix: cleanObjectPrefix(opts.Prefix), - policy: opts.Policy.withDefaults(), - now: now, - log: log, - marks: make(map[string]payloadMark), + store: opts.Store, + prefix: cleanObjectPrefix(opts.Prefix), + policy: opts.Policy.withDefaults(), + now: now, + log: log, + verifyPublishersUseClaims: opts.VerifyPublishersUseClaims, + marks: make(map[string]payloadMark), }, nil } -// RunOnce executes both phases. It returns an error only when the scan -// itself failed; a scan that completes but declines to reclaim -// payloads returns a nil error and a GCResult with +// RunOnce verifies rollout and backend prerequisites, then executes both +// phases. A prerequisite or scan failure returns an error; a completed scan +// that declines to reclaim payloads returns a nil error and a GCResult with // PayloadPhaseSkipped set. func (g *GC) RunOnce(ctx context.Context) (result GCResult, retErr error) { if g == nil { return GCResult{}, errors.Wrap(ErrInvalidOptions, "gc is required") } + if err := g.verifyPublishersUseClaims(ctx); err != nil { + return GCResult{}, errors.Wrap(err, "retention: publishers have not completed the claims rollout") + } + if err := g.store.ValidateRetention(ctx); err != nil { + return GCResult{}, errors.Wrap(err, "retention: validate object store") + } scan, err := g.scanManifests(ctx) if err != nil { diff --git a/internal/snapshotoffload/retention_test.go b/internal/snapshotoffload/retention_test.go index 778e1a397..0e054167f 100644 --- a/internal/snapshotoffload/retention_test.go +++ b/internal/snapshotoffload/retention_test.go @@ -19,6 +19,8 @@ import ( const retentionPrefix = "cluster-a" +func verifyTestPublishersUseClaims(context.Context) error { return nil } + // gcFixture is a LocalStore-backed prefix plus a controllable clock. type gcFixture struct { root string @@ -43,10 +45,11 @@ func (f *gcFixture) gc(t *testing.T, policy RetentionPolicy) *GC { policy.MinMarkAge = time.Hour } gc, err := NewGC(GCOptions{ - Store: f.store, - Prefix: retentionPrefix, - Policy: policy, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: f.store, + Prefix: retentionPrefix, + Policy: policy, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) return gc @@ -334,10 +337,11 @@ func TestGCPreservesMalformedManifestEvidenceFromTheInitialScan(t *testing.T) { malformedKey: badKey, } gc, err := NewGC(GCOptions{ - Store: store, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) @@ -452,10 +456,11 @@ func TestGCRevalidatesPayloadReferenceCommittedAfterInitialPayloadList(t *testin }, } gc, err := NewGC(GCOptions{ - Store: store, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) @@ -521,10 +526,11 @@ func TestGCPerformsNoDeletesWhenListingFails(t *testing.T) { failing := &failingListStore{RetentionStore: f.store, failOn: "v1/groups"} gc, err := NewGC(GCOptions{ - Store: failing, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: failing, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) @@ -545,10 +551,11 @@ func TestGCPerformsNoDeletesWhenPayloadListingFails(t *testing.T) { failing := &failingListStore{RetentionStore: f.store, failOn: "v1/payloads"} gc, err := NewGC(GCOptions{ - Store: failing, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: failing, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) @@ -569,10 +576,11 @@ func TestGCAbortsOnManifestTransportFailure(t *testing.T) { failing := &failingGetStore{RetentionStore: f.store, failKey: m1.ManifestKey} gc, err := NewGC(GCOptions{ - Store: failing, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: failing, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) @@ -685,6 +693,84 @@ func TestNewGCRequiresStore(t *testing.T) { require.True(t, errors.Is(err, ErrInvalidOptions)) } +func TestGCRequiresCompletedClaimsRolloutBeforeEveryPass(t *testing.T) { + t.Parallel() + f := newGCFixture(t) + stale := f.publishManifest(t, 1, 10, []byte("old generation"), 365*24*time.Hour) + f.publishManifest(t, 1, 20, []byte("live generation"), 365*24*time.Hour) + opts := GCOptions{ + Store: f.store, Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour, MinMarkAge: time.Hour}, + Now: func() time.Time { return f.now }, + } + gc, err := NewGC(opts) + require.ErrorIs(t, err, ErrInvalidOptions) + require.Nil(t, gc, "GC must not default to allowing legacy publishers") + + legacyActive := true + legacyErr := errors.New("legacy publisher active") + opts.VerifyPublishersUseClaims = func(context.Context) error { + if legacyActive { + return legacyErr + } + return nil + } + gc, err = NewGC(opts) + require.NoError(t, err) + _, err = gc.RunOnce(context.Background()) + require.ErrorIs(t, err, legacyErr) + require.True(t, f.exists(t, stale.ManifestKey)) + require.True(t, f.exists(t, stale.Payload.Key)) + + legacyActive = false + _, err = gc.RunOnce(context.Background()) + require.NoError(t, err) + f.now = f.now.Add(2 * time.Hour) + legacyActive = true + _, err = gc.RunOnce(context.Background()) + require.ErrorIs(t, err, legacyErr) + require.True(t, f.exists(t, stale.Payload.Key), "a sweepable payload must survive when legacy publishing resumes") + + legacyActive = false + _, err = gc.RunOnce(context.Background()) + require.NoError(t, err) + require.False(t, f.exists(t, stale.Payload.Key)) +} + +type invalidRetentionStore struct { + RetentionStore + err error +} + +func (s *invalidRetentionStore) ValidateRetention(context.Context) error { return s.err } + +func TestGCValidatesRetentionStoreBeforeEveryPass(t *testing.T) { + t.Parallel() + f := newGCFixture(t) + stale := f.publishManifest(t, 1, 10, []byte("stale"), 365*24*time.Hour) + f.publishManifest(t, 1, 20, []byte("live"), 365*24*time.Hour) + validationErr := errors.New("bucket policy unavailable") + backend := &invalidRetentionStore{RetentionStore: f.store, err: validationErr} + gc, err := NewGC(GCOptions{ + Store: backend, Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour, MinMarkAge: time.Hour}, + Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + }) + require.NoError(t, err) + _, err = gc.RunOnce(context.Background()) + require.ErrorIs(t, err, validationErr) + require.True(t, f.exists(t, stale.ManifestKey)) + backend.err = nil + _, err = gc.RunOnce(context.Background()) + require.NoError(t, err) + f.now = f.now.Add(2 * time.Hour) + backend.err = validationErr + _, err = gc.RunOnce(context.Background()) + require.ErrorIs(t, err, validationErr) + require.True(t, f.exists(t, stale.Payload.Key)) +} + func TestRetentionPolicyDefaults(t *testing.T) { t.Parallel() @@ -938,8 +1024,9 @@ func TestGCHoldsPayloadClaimAcrossTheFinalManifestScan(t *testing.T) { manifestKey: orphan.ManifestKey, } gc, err := NewGC(GCOptions{ - Store: store, - Prefix: retentionPrefix, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: store, + Prefix: retentionPrefix, Policy: RetentionPolicy{ MinGenerations: 1, MaxAge: time.Hour, @@ -1111,10 +1198,11 @@ func TestGCDoesNotDeleteManifestRewrittenByAConcurrentPublish(t *testing.T) { } gc, err := NewGC(GCOptions{ - Store: rewriting, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: rewriting, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) @@ -1159,10 +1247,11 @@ func TestGCKeepsManifestRefreshedBeforeItsClaim(t *testing.T) { }, } gc, err := NewGC(GCOptions{ - Store: store, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) @@ -1210,10 +1299,11 @@ func TestGCKeepsManifestWhoseContentChangedWithStableFilesystemMetadata(t *testi }, } gc, err := NewGC(GCOptions{ - Store: store, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: store, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) @@ -1251,8 +1341,9 @@ func TestGCKeepsPayloadWhenExpiredManifestClaimIsBusy(t *testing.T) { manifestKey: stale.ManifestKey, } gc, err := NewGC(GCOptions{ - Store: store, - Prefix: retentionPrefix, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: store, + Prefix: retentionPrefix, Policy: RetentionPolicy{ MinGenerations: 1, MaxAge: time.Hour, @@ -1308,8 +1399,9 @@ func TestGCRevalidatesReferencesOncePerPassNotPerPayload(t *testing.T) { counting := &countingListStore{RetentionStore: f.store} gc, err := NewGC(GCOptions{ - Store: counting, - Prefix: retentionPrefix, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: counting, + Prefix: retentionPrefix, Policy: RetentionPolicy{ MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour, MinMarkAge: time.Hour, @@ -1429,10 +1521,11 @@ func TestGCDeduplicatesListedManifestKeys(t *testing.T) { dup := &duplicateListingStore{RetentionStore: f.store, target: only.ManifestKey} gc, err := NewGC(GCOptions{ - Store: dup, - Prefix: retentionPrefix, - Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, - Now: func() time.Time { return f.now }, + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + Store: dup, + Prefix: retentionPrefix, + Policy: RetentionPolicy{MinGenerations: 1, MaxAge: time.Hour, PayloadGrace: time.Hour}, + Now: func() time.Time { return f.now }, }) require.NoError(t, err) diff --git a/internal/snapshotoffload/s3_store.go b/internal/snapshotoffload/s3_store.go index 148c7342a..98234f642 100644 --- a/internal/snapshotoffload/s3_store.go +++ b/internal/snapshotoffload/s3_store.go @@ -60,20 +60,21 @@ type S3StoreConfig struct { DisableChecksumHeaders bool // AllowVersionedBucketWithLifecycle is an explicit operator assertion // that the bucket has a noncurrent-version expiration lifecycle. The - // store rejects Enabled/Suspended versioning by default because keyed + // GC rejects Enabled/Suspended versioning by default because keyed // retention deletes otherwise create delete markers without reclaiming // bytes. AllowVersionedBucketWithLifecycle bool } type S3Store struct { - client S3ObjectClient - bucket string - serverSideEncryption string - sseKMSKeyID string - disableChecksumHeaders bool - multipartThreshold int64 - multipartPartSize int64 + client S3ObjectClient + bucket string + serverSideEncryption string + sseKMSKeyID string + disableChecksumHeaders bool + multipartThreshold int64 + multipartPartSize int64 + allowVersionedBucketWithLifecycle bool } func NewS3Store(ctx context.Context, cfg S3StoreConfig) (*S3Store, error) { @@ -97,20 +98,24 @@ func NewS3Store(ctx context.Context, cfg S3StoreConfig) (*S3Store, error) { }) } bucket := stringsTrim(cfg.Bucket) - if err := validateS3BucketVersioning(ctx, client, bucket, cfg.AllowVersionedBucketWithLifecycle); err != nil { - return nil, err - } return &S3Store{ - client: client, - bucket: bucket, - serverSideEncryption: stringsTrim(cfg.ServerSideEncryption), - sseKMSKeyID: stringsTrim(cfg.SSEKMSKeyID), - disableChecksumHeaders: cfg.DisableChecksumHeaders, - multipartThreshold: s3MaxSinglePutBytes, - multipartPartSize: s3DefaultMultipartPart, + client: client, + bucket: bucket, + serverSideEncryption: stringsTrim(cfg.ServerSideEncryption), + sseKMSKeyID: stringsTrim(cfg.SSEKMSKeyID), + disableChecksumHeaders: cfg.DisableChecksumHeaders, + multipartThreshold: s3MaxSinglePutBytes, + multipartPartSize: s3DefaultMultipartPart, + allowVersionedBucketWithLifecycle: cfg.AllowVersionedBucketWithLifecycle, }, nil } +// ValidateRetention checks versioning immediately before GC, without imposing +// bucket-policy permissions or lifecycle requirements on read-only restores. +func (s *S3Store) ValidateRetention(ctx context.Context) error { + return validateS3BucketVersioning(ctx, s.client, s.bucket, s.allowVersionedBucketWithLifecycle) +} + func validateS3BucketVersioning( ctx context.Context, client S3ObjectClient, diff --git a/internal/snapshotoffload/s3_store_test.go b/internal/snapshotoffload/s3_store_test.go index 337313641..f1a0d1754 100644 --- a/internal/snapshotoffload/s3_store_test.go +++ b/internal/snapshotoffload/s3_store_test.go @@ -54,6 +54,52 @@ func TestPublishAndRestorePhysicalSnapshotRoundTripWithS3Store(t *testing.T) { require.Equal(t, manifest.Payload.SHA256, result.PayloadSHA256) } +func TestS3RestoreDoesNotRequireBucketVersioningPermission(t *testing.T) { + t.Parallel() + for _, status := range []types.BucketVersioningStatus{"", types.BucketVersioningStatusEnabled, "FutureVersioningMode"} { + t.Run(string(status), func(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + dataDir := seedPhysicalSnapshot(t, root, []byte("EKVTHLC1read-only-restore"), 50, 8, singlePeer()) + client := newFakeS3Client() + manifest, err := PublishPersistedSnapshot(ctx, PublishOptions{ + Store: newTestS3Store(t, client), DataDir: dataDir, + Prefix: "cluster-s3", GroupID: 3, SourceCluster: "cluster-s3", + }) + require.NoError(t, err) + client.versioningStatus = status + client.versioningErr = errors.New("AccessDenied: s3:GetBucketVersioning") + putsBefore, deletesBefore := client.attempts, len(client.deletes) + readerStore := newTestS3Store(t, client) + result, err := RestorePhysicalSnapshot(ctx, RestoreOptions{ + Store: readerStore, ManifestKey: manifest.ManifestKey, DataDir: root + "/restored", + Peers: []etcdraftengine.Peer{{NodeID: 2, ID: "n2", Address: "127.0.0.1:12002"}}, + }) + require.NoError(t, err) + require.Equal(t, manifest.Payload.SHA256, result.PayloadSHA256) + require.Zero(t, client.versioningCalls) + require.Equal(t, putsBefore, client.attempts) + require.Len(t, client.deletes, deletesBefore) + }) + } +} + +func TestS3GCRequiresBucketVersioningPermissionBeforeScanning(t *testing.T) { + t.Parallel() + client := newFakeS3Client() + client.versioningErr = errors.New("AccessDenied: s3:GetBucketVersioning") + gc, err := NewGC(GCOptions{ + Store: newTestS3Store(t, client), Prefix: "cluster-s3", + VerifyPublishersUseClaims: verifyTestPublishersUseClaims, + }) + require.NoError(t, err) + _, err = gc.RunOnce(context.Background()) + require.ErrorIs(t, err, client.versioningErr) + require.Equal(t, 1, client.versioningCalls) + require.Empty(t, client.listPrefixes) + require.Empty(t, client.deletes) +} + func TestS3StorePutHeadGetPreservesIntegrityMetadata(t *testing.T) { ctx := context.Background() fake := newFakeS3Client() @@ -350,11 +396,13 @@ func TestS3StoreRejectsVersionedBucketsWithoutLifecycleAssertion(t *testing.T) { t.Parallel() client := newFakeS3Client() client.versioningStatus = status - _, err := NewS3Store(context.Background(), S3StoreConfig{ + store, err := NewS3Store(context.Background(), S3StoreConfig{ Client: client, Bucket: "backup-bucket", ServerSideEncryption: string(types.ServerSideEncryptionAes256), }) + require.NoError(t, err) + err = store.ValidateRetention(context.Background()) require.ErrorIs(t, err, ErrInvalidOptions) require.ErrorContains(t, err, "noncurrent-version expiration") }) @@ -374,6 +422,7 @@ func TestS3StoreAllowsVersionedBucketWithLifecycleAssertion(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, store) + require.NoError(t, store.ValidateRetention(context.Background())) } func TestS3StoreRejectsUnknownVersioningStatus(t *testing.T) { @@ -381,12 +430,14 @@ func TestS3StoreRejectsUnknownVersioningStatus(t *testing.T) { for _, allow := range []bool{false, true} { client := newFakeS3Client() client.versioningStatus = "UnknownVersionedMode" - _, err := NewS3Store(context.Background(), S3StoreConfig{ + store, err := NewS3Store(context.Background(), S3StoreConfig{ Client: client, Bucket: "backup-bucket", ServerSideEncryption: string(types.ServerSideEncryptionAes256), AllowVersionedBucketWithLifecycle: allow, }) + require.NoError(t, err) + err = store.ValidateRetention(context.Background()) require.ErrorIs(t, err, ErrIntegrity) require.ErrorContains(t, err, "unknown versioning status") } @@ -447,6 +498,7 @@ type fakeS3Client struct { deletes []string versioningStatus types.BucketVersioningStatus versioningErr error + versioningCalls int } type fakeS3Object struct { @@ -951,6 +1003,7 @@ func (c *fakeS3Client) GetBucketVersioning( ) (*s3.GetBucketVersioningOutput, error) { c.mu.Lock() defer c.mu.Unlock() + c.versioningCalls++ if c.versioningErr != nil { return nil, c.versioningErr } diff --git a/internal/snapshotoffload/store.go b/internal/snapshotoffload/store.go index 8dbd61d8a..1a0128162 100644 --- a/internal/snapshotoffload/store.go +++ b/internal/snapshotoffload/store.go @@ -104,6 +104,9 @@ func PreconditionFor(ref ObjectRef) DeletePrecondition { type RetentionStore interface { ObjectStore ObjectClaimStore + // ValidateRetention checks backend-specific reclamation prerequisites + // before each pass. Read-only restore must not require these permissions. + ValidateRetention(ctx context.Context) error ListObjects(ctx context.Context, prefix string) ([]ObjectRef, error) DeleteObject(ctx context.Context, key string) error @@ -124,6 +127,11 @@ type RetentionStore interface { var _ RetentionStore = (*LocalStore)(nil) var _ PublishStore = (*LocalStore)(nil) +// ValidateRetention checks the pass context; local stores have no bucket policy. +func (s *LocalStore) ValidateRetention(ctx context.Context) error { + return errors.WithStack(ctx.Err()) +} + // ListObjects walks the pinned local root below prefix. Directories and // irregular files are skipped, while symlinks fail the all-or-error listing // contract closed. Returned keys are slash-separated and relative to the