From 8ae19de7a85126c150d5cceb83cbee5823d8240c Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Tue, 3 Nov 2026 21:39:36 +0800 Subject: [PATCH] fix(restore): avoid duplicate completion locks for empty upload IDs Keep the storage layer error precedence for missing buckets and uploads while retaining every real upload lock. Cover the alias with short deadlines and race-check the original complete-multipart suite in CI. Signed-off-by: Feng Ruohang --- .github/workflows/go.yml | 2 +- cmd/erasure-server-pool.go | 16 ++++++++++------ cmd/restore-native_test.go | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 2bf27c930..44afacf74 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -99,7 +99,7 @@ jobs: - name: Run multipart listing, cancellation and cleanup tests under race detector env: MINIO_API_REQUESTS_MAX: "10000" - run: go test -race ./cmd -run '^Test(Multipart|PaginateMultipartUploads|ListMultipartUploads|CleanupStaleUploads|ExtractModTime)' -count=1 -timeout=5m + run: go test -race ./cmd -run '^Test(ObjectCompleteMultipartUpload|Multipart|PaginateMultipartUploads|ListMultipartUploads|CleanupStaleUploads|ExtractModTime)' -count=1 -timeout=5m - name: Run listing metadata recovery, pagination and cancellation tests under race detector run: go test -race ./cmd -run '^Test(ListFallback|ListingCache|ListingMerge|ListingRecovery|ListingPending|ListingEntry|ListingSecondLock|StreamMetadataParts|LocalLockerListing|DistributedListing|MergeXLV2LatestNullCrossParity|CountXLV2MixedECGroups|ListCrossParity|ListObjectVersionsCrossParityNull|MigrationCrossParity)' -count=1 -timeout=5m diff --git a/cmd/erasure-server-pool.go b/cmd/erasure-server-pool.go index 23661c413..db2973717 100644 --- a/cmd/erasure-server-pool.go +++ b/cmd/erasure-server-pool.go @@ -2391,13 +2391,17 @@ func (z *erasureServerPools) CompleteMultipartUpload(ctx context.Context, bucket // Hold write locks to verify uploaded parts, also disallows any // parallel PutObjectPart() requests. - uploadIDLock := z.NewNSLock(bucket, pathJoin(object, uploadID)) - wlkctx, err := uploadIDLock.GetLock(ctx, globalOperationTimeout) - if err != nil { - return objInfo, err + // An empty upload ID aliases the object lock already held above. + // Let the storage layer return its normal missing bucket/upload error. + if uploadID != "" { + uploadIDLock := z.NewNSLock(bucket, pathJoin(object, uploadID)) + wlkctx, err := uploadIDLock.GetLock(ctx, globalOperationTimeout) + if err != nil { + return objInfo, err + } + ctx = wlkctx.Context() + defer uploadIDLock.Unlock(wlkctx) } - ctx = wlkctx.Context() - defer uploadIDLock.Unlock(wlkctx) if z.SinglePool() { return z.serverPools[0].CompleteMultipartUpload(ctx, bucket, object, uploadID, uploadedParts, opts) diff --git a/cmd/restore-native_test.go b/cmd/restore-native_test.go index 577607ced..706dbc8a4 100644 --- a/cmd/restore-native_test.go +++ b/cmd/restore-native_test.go @@ -707,3 +707,35 @@ func (r *nativeRestorePartGateReader) Read(p []byte) (int, error) { } return r.ReadCloser.Read(p) } + +// Invalid upload paths must retain the backend's normal error precedence, +// rather than reacquiring the object lock as the upload lock. +func TestMultipartCompletionLockAlias(t *testing.T) { + for _, pools := range []int{1, 2} { + for _, id := range []string{"", ".", "./"} { + t.Run(fmt.Sprintf("pools=%d/id=%q", pools, id), func(t *testing.T) { + r := newRestoreSafetyTest(t, pools, versioning.Enabled) + for _, missingBucket := range []bool{false, true} { + bucket := r.bucket + if missingBucket { + bucket = "missing-bucket" + } + ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + _, err := r.z.CompleteMultipartUpload(ctx, bucket, r.object, id, nil, ObjectOptions{}) + cancel() + if id != "" { + if !errors.As(err, &MalformedUploadID{}) { + t.Errorf("malformed upload: %v", err) + } + } else if missingBucket { + if !isErrBucketNotFound(err) { + t.Errorf("missing bucket: %v", err) + } + } else if !errors.As(err, &InvalidUploadID{}) { + t.Errorf("invalid upload: %v", err) + } + } + }) + } + } +}