From c2b47967bde49e2cce3021a980329e03a3e2b264 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 5 Aug 2026 14:31:50 -0700 Subject: [PATCH] s3: retire the suspended null marker only once the PUT has committed (#10589) The suspended PUT dropped the null delete marker before writing, so a failed write left the .versions pointer naming a marker that was gone. The read path heals a dangling pointer by promoting the newest survivor, so a key the caller had deleted came back serving an older version, and the heal persisted that pointer. Move the retire into afterCreate via the shared finalize, which also brings the ownership check the copy and multipart paths already have. --- .../s3_suspended_versioning_test.go | 40 +++++++++++++++++++ weed/s3api/s3api_object_handlers_put.go | 23 +++++------ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/test/s3/versioning/s3_suspended_versioning_test.go b/test/s3/versioning/s3_suspended_versioning_test.go index c1e8c7277..2a039c330 100644 --- a/test/s3/versioning/s3_suspended_versioning_test.go +++ b/test/s3/versioning/s3_suspended_versioning_test.go @@ -8,6 +8,8 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestSuspendedVersioningNullOverwrite tests the scenario where: @@ -255,3 +257,41 @@ func TestEnabledVersioningReturnsVersionId(t *testing.T) { t.Logf(" Version %d: VersionId=%s, Size=%d, IsLatest=%v", i, *v.VersionId, v.Size, v.IsLatest) } } + +// The suspended PUT retires the null delete marker a preceding DELETE left, the same +// as the copy and multipart paths. While the regular-path object owns the null slot a +// leftover marker is shadowed, so it only shows once that null version goes away. +func TestSuspendedPutRetiresDeleteMarker(t *testing.T) { + client := getS3Client(t) + bucketName := getNewBucketName() + + createBucket(t, client, bucketName) + defer deleteBucket(t, client, bucketName) + + objectKey := "suspended-put-after-delete.txt" + + enableVersioning(t, client, bucketName) + putObject(t, client, bucketName, objectKey, "pre-suspension-content") + suspendVersioning(t, client, bucketName) + + putObject(t, client, bucketName, objectKey, "null-version-content") + deleteKey(t, client, bucketName, objectKey) + putObject(t, client, bucketName, objectKey, "replacement-content") + + headObject(t, client, bucketName, objectKey) + + // Drop the null version just written; a retired marker leaves nothing behind. + _, err := client.DeleteObject(context.TODO(), &s3.DeleteObjectInput{ + Bucket: aws.String(bucketName), + Key: aws.String(objectKey), + VersionId: aws.String("null"), + }) + require.NoError(t, err) + + listResp, err := client.ListObjectVersions(context.TODO(), &s3.ListObjectVersionsInput{ + Bucket: aws.String(bucketName), + Prefix: aws.String(objectKey), + }) + require.NoError(t, err) + assert.Empty(t, listResp.DeleteMarkers, "the suspended PUT should have retired the null delete marker") +} diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 5055f61b6..3e30b6774 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -1293,10 +1293,6 @@ func (s3a *S3ApiServer) putSuspendedVersioningObject(r *http.Request, bucket, ob glog.V(3).Infof("putSuspendedVersioningObject: START bucket=%s, object=%s, normalized=%s", bucket, object, normalizedObject) - // The null version now lives at the regular path, so any null version recorded in - // .versions is stale (S3 has the suspended write overwrite it, not accumulate). - s3a.removeNullVersionFile(bucket, normalizedObject) - filePath := s3a.toFilerPath(bucket, normalizedObject) body := dataReader @@ -1360,15 +1356,18 @@ func (s3a *S3ApiServer) putSuspendedVersioningObject(r *http.Request, bucket, ob } // Versioned/suspended bucket → resolver returns 0; pass it directly. - // afterCreate clears the prior latest pointer and stamps the displaced version - // with NoncurrentSinceNs — off-ring under the write lock, routed off-lock after - // the PUT. Best-effort either way: a stale flag self-heals on the next list. + // afterCreate retires the null delete marker a preceding DELETE left, clears the + // prior latest pointer and stamps the displaced version with NoncurrentSinceNs — + // off-ring under the write lock, routed off-lock after the PUT. Only once the write + // has committed: retiring the marker for a write that then fails leaves the pointer + // naming a marker that is gone, and the read path heals that by promoting an older + // version, republishing the deleted key. Best-effort, as a stale flag self-heals on + // the next list. etag, errCode, sseMetadata = s3a.putToFiler(r, filePath, body, bucket, normalizedObject, 1, 0, &putFinalize{ - afterCreate: func(_ *filer_pb.Entry) s3err.ErrorCode { - if err := s3a.updateIsLatestFlagsForSuspendedVersioning(bucket, normalizedObject); err != nil { - // Best-effort: a stale IsLatest flag is recoverable on the - // next list-versions resync, so don't fail the PUT. - glog.Warningf("putSuspendedVersioningObject: failed to update IsLatest flags: %v", err) + afterCreate: func(entry *filer_pb.Entry) s3err.ErrorCode { + writtenETag := string(entry.Extended[s3_constants.ExtETagKey]) + if err := s3a.finalizeSuspendedNullWrite("", bucket, normalizedObject, s3_constants.ExtETagKey, writtenETag); err != nil { + glog.Warningf("putSuspendedVersioningObject: failed to retire the null delete marker: %v", err) } return s3err.ErrNone },