Repository: minio/minio (language: go)

Issue:
Data loss from multiple calls to CompleteMultipartUpload object in a short period of time
### Discussed in https://github.com/minio/minio/discussions/17859

<div type='discussions-op-text'>

<sup>Originally posted by **xieys0** August 16, 2023</sup>
## Environment
OS:  Ubuntu22.04.1
Version:  https://dl.min.io/server/minio/release/linux-amd64/archive/minio_20230809233022.0.0_amd64.deb

# df -HT
root@ubuntu:~# df -hT
Filesystem     Type   Size  Used Avail Use% Mounted on
tmpfs          tmpfs  6.3G  2.2M  6.3G   1% /run
/dev/sda2      ext4   479G   12G  443G   3% /
tmpfs          tmpfs   32G     0   32G   0% /dev/shm
tmpfs          tmpfs  5.0M     0  5.0M   0% /run/lock
/dev/sda1      vfat   1.1G  6.1M  1.1G   1% /boot/efi
tmpfs          tmpfs  6.3G  4.0K  6.3G   1% /run/user/0
/dev/sdb       xfs    7.3T   52G  7.3T   1% /mnt/disk1
/dev/sdc       xfs    7.3T   52G  7.3T   1% /mnt/disk2
/dev/sdd       xfs    7.3T   52G  7.3T   1% /mnt/disk3
/dev/sde       xfs    7.3T   52G  7.3T   1% /mnt/disk4

# cat /etc/default/minio
MINIO_ROOT_USER=xxxx
MINIO_ROOT_PASSWORD=xxxx
MINIO_OPTS="--address :9000 --console-address :9001"
MINIO_VOLUMES="/mnt/disk{1...4}"

## Describe
Calling the CompleteMultipartUpload interface multiple times for the same uploadId in a short period of time. The first call returned success, while the other returned uploadId does not exist. The object information can be found, but the object data is missing and cannot be downloaded or restored. There are only xl. meta files in the object folder.

root@ubuntu:/mnt/disk1/test/0CA985D3589645E49ADFAF4391FA754D# ls -la
total 4
drwxr-xr-x 2 minio-user minio-user  29 Aug 15 11:14 .
drwxr-xr-x 3 minio-user minio-user  54 Aug 15 11:14 ..
-rw-r--r-- 1 minio-user minio-user 844 Aug 15 11:14 xl.meta

## Expected Behavior
Object data is not lost

## Current Behavior
Object data is not lost

## Notes
Historical versions and Ubuntu multi node and multi driver environments can still be replicated</div>


Discussion / hints:
(repro in discussion)
So I tried reproducing I can't reproduce this on my end

```
~ go test -v --count=100
=== RUN   TestDemo
NewMultipartUpload end, uploadId= ZmIyM2VkNzctMzExZS00NDkwLTgzOGMtNjg4MzgzNDk5NDBmLjNiNjVhZjY5LTNkMzctNDNiMS1hZWFlLWNjMzYxZTk5NWI4Yw
PutObjectPart end
uploadId2= d9b6fe4b-59f6-4ffe-a60a-5b576d7edd5a
Second {   0 0001-01-01 00:00:00 +0000 UTC   0001-01-01 00:00:00 +0000 UTC     }    One or more of the specified parts could not be found.  The part may not have been uploaded, or the specified entity tag may not match the part's entity tag.
First {test testObject.data 6a7a32f75cec39d7f42e0d826dc636b7-100 0 0001-01-01 00:00:00 +0000 UTC http://localhost:9000/test/testObject.data  0001-01-01 00:00:00 +0000 UTC     }    <nil>
CompleteMultipartUpload end
--- PASS: TestDemo (2.39s)
```

@klauspost 

Fix patch:
diff --git a/cmd/erasure-multipart.go b/cmd/erasure-multipart.go
index ab46dae997b6a..6d6f3f6225d57 100644
--- a/cmd/erasure-multipart.go
+++ b/cmd/erasure-multipart.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2015-2021 MinIO, Inc.
+// Copyright (c) 2015-2023 MinIO, Inc.
 //
 // This file is part of MinIO Object Storage stack
 //
@@ -101,7 +101,6 @@ func (er erasureObjects) checkUploadIDExists(ctx context.Context, bucket, object
 
 	// Pick one from the first valid metadata.
 	fi, err = pickValidFileInfo(ctx, partsMetadata, modTime, etag, quorum)
-
 	return fi, partsMetadata, err
 }
 
@@ -310,7 +309,7 @@ func (er erasureObjects) ListMultipartUploads(ctx context.Context, bucket, objec
 		populatedUploadIds.Add(uploadID)
 		uploads = append(uploads, MultipartInfo{
 			Object:    object,
-			UploadID:  uploadID,
+			UploadID:  base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("%s.%s", globalDeploymentID, uploadID))),
 			Initiated: fi.ModTime,
 		})
 	}
@@ -954,10 +953,10 @@ func (er erasureObjects) CompleteMultipartUpload(ctx context.Context, bucket str
 	if err != nil {
 		return oi, err
 	}
-	wctx := wlkctx.Context()
+	ctx = wlkctx.Context()
 	defer uploadIDLock.Unlock(wlkctx)
 
-	fi, partsMetadata, err := er.checkUploadIDExists(wctx, bucket, object, uploadID, true)
+	fi, partsMetadata, err := er.checkUploadIDExists(ctx, bucket, object, uploadID, true)
 	if err != nil {
 		return oi, toObjectErr(err, bucket, object, uploadID)
 	}
@@ -1227,12 +1226,6 @@ func (er erasureObjects) CompleteMultipartUpload(ctx context.Context, bucket str
 	ctx = lkctx.Context()
 	defer lk.Unlock(lkctx)
 
-	// Write final `xl.meta` at uploadID location
-	onlineDisks, err = writeUniqueFileInfo(ctx, onlineDisks, minioMetaMultipartBucket, uploadIDPath, partsMetadata, writeQuorum)
-	if err != nil {
-		return oi, toObjectErr(err, minioMetaMultipartBucket, uploadIDPath)
-	}
-
 	// Remove parts that weren't present in CompleteMultipartUpload request.
 	for _, curpart := range currentFI.Parts {
 		if objectPartIndex(fi.Parts, curpart.Number) == -1 {
@@ -1242,13 +1235,13 @@ func (er erasureObjects) CompleteMultipartUpload(ctx context.Context, bucket str
 			// Request 3: PutObjectPart 2
 			// Request 4: CompleteMultipartUpload --part 2
 			// N.B. 1st part is not present. This part should be removed from the storage.
-			er.removeObjectPart(bucket, object, uploadID, fi.DataDir, curpart.Number)
+			er.removeObjectPart(bucket, object, uploadID, currentFI.DataDir, curpart.Number)
 		}
 	}
 
 	// Remove part.meta which is not needed anymore.
-	for _, part := range fi.Parts {
-		er.removePartMeta(bucket, object, uploadID, fi.DataDir, part.Number)
+	for _, part := range currentFI.Parts {
+		er.removePartMeta(bucket, object, uploadID, currentFI.DataDir, part.Number)
 	}
 
 	// Rename the multipart object to final location.
diff --git a/cmd/erasure-server-pool.go b/cmd/erasure-server-pool.go
index cef455ee120b8..864948b9dd4b2 100644
--- a/cmd/erasure-server-pool.go
+++ b/cmd/erasure-server-pool.go
@@ -1488,7 +1488,7 @@ func (z *erasureServerPools) CopyObjectPart(ctx context.Context, srcBucket, srcO
 
 // PutObjectPart - writes part of an object to hashedSet based on the object name.
 func (z *erasureServerPools) PutObjectPart(ctx context.Context, bucket, object, uploadID string, partID int, data *PutObjReader, opts ObjectOptions) (PartInfo, error) {
-	if err := checkPutObjectPartArgs(ctx, bucket, object, z); err != nil {
+	if err := checkPutObjectPartArgs(ctx, bucket, object, uploadID, z); err != nil {
 		return PartInfo{}, err
 	}
 
@@ -1520,7 +1520,7 @@ func (z *erasureServerPools) PutObjectPart(ctx context.Context, bucket, object,
 }
 
 func (z *erasureServerPools) GetMultipartInfo(ctx context.Context, bucket, object, uploadID string, opts ObjectOptions) (MultipartInfo, error) {
-	if err := checkListPartsArgs(ctx, bucket, object, z); err != nil {
+	if err := checkListPartsArgs(ctx, bucket, object, uploadID, z); err != nil {
 		return MultipartInfo{}, err
 	}
 
@@ -1551,7 +1551,7 @@ func (z *erasureServerPools) GetMultipartInfo(ctx context.Context, bucket, objec
 
 // ListObjectParts - lists all uploaded parts to an object in hashedSet.
 func (z *erasureServerPools) ListObjectParts(ctx context.Context, bucket, object, uploadID string, partNumberMarker int, maxParts int, opts ObjectOptions) (ListPartsInfo, error) {
-	if err := checkListPartsArgs(ctx, bucket, object, z); err != nil {
+	if err := checkListPartsArgs(ctx, bucket, object, uploadID, z); err != nil {
 		return ListPartsInfo{}, err
 	}
 
@@ -1580,7 +1580,7 @@ func (z *erasureServerPools) ListObjectParts(ctx context.Context, bucket, object
 
 // Aborts an in-progress multipart operation on hashedSet based on the object name.
 func (z *erasureServerPools) AbortMultipartUpload(ctx context.Context, bucket, object, uploadID string, opts ObjectOptions) error {
-	if err := checkAbortMultipartArgs(ctx, bucket, object, z); err != nil {
+	if err := checkAbortMultipartArgs(ctx, bucket, object, uploadID, z); err != nil {
 		return err
 	}
 
@@ -1611,7 +1611,7 @@ func (z *erasureServerPools) AbortMultipartUpload(ctx context.Context, bucket, o
 
 // CompleteMultipartUpload - completes a pending multipart transaction, on hashedSet based on object name.
 func (z *erasureServerPools) CompleteMultipartUpload(ctx context.Context, bucket, object, uploadID string, uploadedParts []CompletePart, opts ObjectOptions) (objInfo ObjectInfo, err error) {
-	if err = checkCompleteMultipartArgs(ctx, bucket, object, z); err != nil {
+	if err = checkCompleteMultipartArgs(ctx, bucket, object, uploadID, z); err != nil {
 		return objInfo, err
 	}
 
diff --git a/cmd/object-api-input-checks.go b/cmd/object-api-input-checks.go
index 0bb51dfc2ba22..e790c05c9f518 100644
--- a/cmd/object-api-input-checks.go
+++ b/cmd/object-api-input-checks.go
@@ -99,7 +99,6 @@ func checkListMultipartArgs(ctx context.Context, bucket, prefix, keyMarker, uplo
 	}
 	if uploadIDMarker != "" {
 		if HasSuffix(keyMarker, SlashSeparator) {
-
 			logger.LogIf(ctx, InvalidUploadIDKeyCombination{
 				UploadIDMarker: uploadIDMarker,
 				KeyMarker:      keyMarker,
@@ -125,24 +124,34 @@ func checkNewMultipartArgs(ctx context.Context, bucket, object string, obj Objec
 	return checkObjectArgs(ctx, bucket, object, obj)
 }
 
-// Checks for PutObjectPart arguments validity, also validates if bucket exists.
-func checkPutObjectPartArgs(ctx context.Context, bucket, object string, obj ObjectLayer) error {
+func checkMultipartObjectArgs(ctx context.Context, bucket, object, uploadID string, obj ObjectLayer) error {
+	_, err := base64.RawURLEncoding.DecodeString(uploadID)
+	if err != nil {
+		return MalformedUploadID{
+			UploadID: uploadID,
+		}
+	}
 	return checkObjectArgs(ctx, bucket, object, obj)
 }
 
+// Checks for PutObjectPart arguments validity, also validates if bucket exists.
+func checkPutObjectPartArgs(ctx context.Context, bucket, object, uploadID string, obj ObjectLayer) error {
+	return checkMultipartObjectArgs(ctx, bucket, object, uploadID, obj)
+}
+
 // Checks for ListParts arguments validity, also validates if bucket exists.
-func checkListPartsArgs(ctx context.Context, bucket, object string, obj ObjectLayer) error {
-	return checkObjectArgs(ctx, bucket, object, obj)
+func checkListPartsArgs(ctx context.Context, bucket, object, uploadID string, obj ObjectLayer) error {
+	return checkMultipartObjectArgs(ctx, bucket, object, uploadID, obj)
 }
 
 // Checks for CompleteMultipartUpload arguments validity, also validates if bucket exists.
-func checkCompleteMultipartArgs(ctx context.Context, bucket, object string, obj ObjectLayer) error {
-	return checkObjectArgs(ctx, bucket, object, obj)
+func checkCompleteMultipartArgs(ctx context.Context, bucket, object, uploadID string, obj ObjectLayer) error {
+	return checkMultipartObjectArgs(ctx, bucket, object, uploadID, obj)
 }
 
 // Checks for AbortMultipartUpload arguments validity, also validates if bucket exists.
-func checkAbortMultipartArgs(ctx context.Context, bucket, object string, obj ObjectLayer) error {
-	return checkObjectArgs(ctx, bucket, object, obj)
+func checkAbortMultipartArgs(ctx context.Context, bucket, object, uploadID string, obj ObjectLayer) error {
+	return checkMultipartObjectArgs(ctx, bucket, object, uploadID, obj)
 }
 
 // Checks Object arguments validity, also validates if bucket exists.
diff --git a/cmd/object-multipart-handlers.go b/cmd/object-multipart-handlers.go
index 32a480b5f4351..da540ce536a48 100644
--- a/cmd/object-multipart-handlers.go
+++ b/cmd/object-multipart-handlers.go
@@ -951,8 +951,6 @@ func (api objectAPIHandlers) CompleteMultipartUploadHandler(w http.ResponseWrite
 		}
 	}
 
-	setEventStreamHeaders(w)
-
 	opts, err := completeMultipartOpts(ctx, r, bucket, object)
 	if err != nil {
 		writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go
index b14d65211159c..cb1fa6603f2c5 100644
--- a/cmd/xl-storage.go
+++ b/cmd/xl-storage.go
@@ -2528,11 +2528,14 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f
 		}
 	}
 
-	// srcFilePath is always in minioMetaTmpBucket, an attempt to
-	// remove the temporary folder is enough since at this point
-	// ideally all transaction should be complete.
-
-	Remove(pathutil.Dir(srcFilePath))
+	if srcVolume != minioMetaMultipartBucket {
+		// srcFilePath is some-times minioMetaTmpBucket, an attempt to
+		// remove the temporary folder is enough since at this point
+		// ideally all transaction should be complete.
+		Remove(pathutil.Dir(srcFilePath))
+	} else {
+		s.deleteFile(srcVolumeDir, pathutil.Dir(srcFilePath), true, false)
+	}
 	return sign, nil
 }
 


Test patch: