From 0ce74ffaac93abd8cf9b6fbb4974dbe0d8a45d3c Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Sat, 19 Sep 2026 18:21:30 +0100 Subject: [PATCH] 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 | 29 ++++++++++++++++------------- cmd/serve/s3/multipart_test.go | 34 +++++++++++++++++++++++++++++++++- cmd/serve/s3/server.go | 11 ++++++----- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/cmd/serve/s3/multipart.go b/cmd/serve/s3/multipart.go index bcec5df4f..3dac0c3a7 100644 --- a/cmd/serve/s3/multipart.go +++ b/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 }) } diff --git a/cmd/serve/s3/multipart_test.go b/cmd/serve/s3/multipart_test.go index e64a1d6c4..89258f627 100644 --- a/cmd/serve/s3/multipart_test.go +++ b/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 diff --git a/cmd/serve/s3/server.go b/cmd/serve/s3/server.go index 84a51c9b2..640838733 100644 --- a/cmd/serve/s3/server.go +++ b/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()