serve s3: forget expired multipart uploads completely

When an abandoned multipart upload expired after --multipart-expiry,
gofakes3's record of it was left behind, so it stayed in memory and in
ListMultipartUploads until the server was restarted.

That record is now removed too.

cmd/serve/s3/multipart.go

@@ -839,20 +839,19 @@ func (b *s3Backend) CompleteMultipartUpload(ctx context.Context, bucketName, obj
 	up.startActivity()
 	defer up.endActivity()
 
+	// gofakes3 keeps its record of an upload whose completion fails, so
+	// forgetUpload removes that too, as the upload can't be retried.
 	if err := up.validate(input); err != nil {
-		b.deleteUpload(uploadID)
-		_ = up.abort()
-		b.discardUpload(up)
+		b.forgetUpload(uploadID, up)
 		return "", "", err
 	}
 
 	// close commits the upload, failing with the upload left open if the
-	// streamed parts don't form the complete object; abort then tears it
-	// down. (After a successful or failed commit the abort is a no-op.)
+	// streamed parts don't form the complete object; forgetUpload then
+	// tears it down. (After a successful or failed commit the abort it
+	// does is a no-op.)
 	if err := up.close(); err != nil {
-		b.deleteUpload(uploadID)
-		_ = up.abort()
-		b.discardUpload(up)
+		b.forgetUpload(uploadID, up)
 		return "", "", err
 	}
 
@@ -905,11 +904,19 @@ func (b *s3Backend) AbortMultipartUpload(ctx context.Context, bucketName, object
 // accepted.
 func (b *s3Backend) failUpload(uploadID gofakes3.UploadID, up *multipartUpload) {
 	fs.Errorf(up.fp, "failing multipart upload %s: %v", uploadID, errMultipartPoisoned)
+	b.forgetUpload(uploadID, up)
+}
+
+// forgetUpload aborts up, discards what it wrote and removes every record
+// of it, ours and gofakes3's.
+func (b *s3Backend) forgetUpload(uploadID gofakes3.UploadID, up *multipartUpload) {
 	b.deleteUpload(uploadID)
 	if err := up.abort(); err != nil {
 		fs.Errorf(up.fp, "aborting multipart upload: %v", err)
 	}
 	b.discardUpload(up)
+	// gofakes3 doesn't know the upload has gone, so tell it
+	_ = b.s.faker.ForgetMultipartUpload(up.bucket, up.key, uploadID)
 }
 
 // startReaper starts a goroutine which aborts incomplete multipart
@@ -950,11 +957,7 @@ func (b *s3Backend) reapExpiredUploads(now time.Time, expiry time.Duration) {
 			return true
 		}
 		fs.Logf(up.fp, "aborting multipart upload %s idle for more than %v", uploadID, expiry)
-		b.deleteUpload(uploadID)
-		if err := up.abort(); err != nil {
-			fs.Errorf(up.fp, "aborting abandoned multipart upload: %v", err)
-		}
-		b.discardUpload(up)
+		b.forgetUpload(uploadID, up)
 		return true
 	})
 }

cmd/serve/s3/multipart_test.go

@@ -647,9 +647,17 @@ func TestMultipartReaper(t *testing.T) {
 	_, err = core.PutObjectPart(ctx, bucket, object, uploadID, 1, bytes.NewReader(data), int64(len(data)), minio.PutObjectPartOptions{})
 	require.NoError(t, err)
 
+	// FIXME gofakes3 lists nothing when the delimiter is empty
+	uploads, err := core.ListMultipartUploads(ctx, bucket, "", "", "", "/", 1000)
+	require.NoError(t, err)
+	require.Len(t, uploads.Uploads, 1)
+
 	// Wait for well over the expiry and the reaper interval, then the
-	// upload must be gone.
+	// upload must be gone, from listings too.
 	time.Sleep(time.Second)
+	uploads, err = core.ListMultipartUploads(ctx, bucket, "", "", "", "/", 1000)
+	require.NoError(t, err)
+	assert.Empty(t, uploads.Uploads)
 	_, err = core.PutObjectPart(ctx, bucket, object, uploadID, 2, bytes.NewReader(data), int64(len(data)), minio.PutObjectPartOptions{})
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "NoSuchUpload")
@@ -663,6 +671,30 @@ func TestMultipartReaper(t *testing.T) {
 	requireOnly(t, f, bucket)
 }
 
+// TestMultipartCompleteFailureForgotten checks that a completion which
+// tears the upload down removes it from gofakes3's records too, so it
+// isn't left in listings or memory.
+func TestMultipartCompleteFailureForgotten(t *testing.T) {
+	core, f, bucket := newMultipartTestServer(t, false)
+	ctx := context.Background()
+	const object = "bad-complete.bin"
+
+	uploadID, err := core.NewMultipartUpload(ctx, bucket, object, minio.PutObjectOptions{})
+	require.NoError(t, err)
+	data := []byte(random.String(50 * 1024))
+	_, err = core.PutObjectPart(ctx, bucket, object, uploadID, 1, bytes.NewReader(data), int64(len(data)), minio.PutObjectPartOptions{})
+	require.NoError(t, err)
+
+	_, err = core.CompleteMultipartUpload(ctx, bucket, object, uploadID, []minio.CompletePart{{PartNumber: 1, ETag: `"00000000000000000000000000000000"`}}, minio.PutObjectOptions{})
+	require.Error(t, err)
+
+	// FIXME gofakes3 lists nothing when the delimiter is empty
+	uploads, err := core.ListMultipartUploads(ctx, bucket, "", "", "", "/", 1000)
+	require.NoError(t, err)
+	assert.Empty(t, uploads.Uploads)
+	requireOnly(t, f, bucket)
+}
+
 // TestMultipartReapExpiredUploads checks the reaper's rules directly: an
 // idle upload past the expiry is aborted and cleaned up, one with a request
 // in flight is left alone however stale its idle time, and a fresh one is

cmd/serve/s3/server.go

@@ -86,11 +86,6 @@ func newServer(ctx context.Context, f fs.Fs, opt *Options, vfsOpt *vfscommon.Opt
 	}
 
 	w.backend = newBackend(w)
-	if w.opt.MultipartExpiry > 0 {
-		w.backend.startReaper(time.Duration(w.opt.MultipartExpiry))
-	} else if w.opt.MultipartMaxUploads > 0 {
-		fs.Logf("serve s3", "--multipart-expiry 0 means abandoned multipart uploads are never cleaned up and go on counting towards --multipart-max-uploads %d", w.opt.MultipartMaxUploads)
-	}
 
 	var newLogger logger
 	w.faker = gofakes3.New(
@@ -102,6 +97,12 @@ func newServer(ctx context.Context, f fs.Fs, opt *Options, vfsOpt *vfscommon.Opt
 		gofakes3.WithV4Auth(authList),
 		gofakes3.WithIntegrityCheck(true), // Check Content-MD5 if supplied
 	)
+	// The reaper uses w.faker so must start after it is set
+	if w.opt.MultipartExpiry > 0 {
+		w.backend.startReaper(time.Duration(w.opt.MultipartExpiry))
+	} else if w.opt.MultipartMaxUploads > 0 {
+		fs.Logf("serve s3", "--multipart-expiry 0 means abandoned multipart uploads are never cleaned up and go on counting towards --multipart-max-uploads %d", w.opt.MultipartMaxUploads)
+	}
 
 	w.handler = w.faker.Server()