From 2a1afadcd079148d8f14ea5f37145e7a3e290cbc Mon Sep 17 00:00:00 2001 From: Ping Qiu Date: Thu, 29 Oct 2026 19:02:23 -0700 Subject: [PATCH] s3api: return NoSuchUpload when aborting an upload that was already completed (#12227) s3: AbortMultipartUpload answers NoSuchUpload for an upload that no longer exists Complete and abort both remove .uploads/. abortMultipartUpload answered 204 whenever that record was missing, so aborting an upload that had already completed reported success while the object stayed in place, and a client could read it as the object being discarded. AWS returns 404 NoSuchUpload ("The specified multipart upload does not exist") for that case, and ListParts here already does. Answer ErrNoSuchUpload when the record is not found. checkUploadStillOpen reuses abort for cleanup after the upload is gone; NoSuchUpload is not a failed cleanup there. The leftover-directory path from #11385 is unchanged: a .uploads/ that outlived its completed object is still removed metadata-only and answered. Co-authored-by: Claude Opus 5.5 --- weed/s3api/filer_multipart.go | 6 ++-- weed/s3api/filer_multipart_abort_test.go | 32 ++++++++++++++++--- weed/s3api/s3api_object_handlers_multipart.go | 2 +- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/weed/s3api/filer_multipart.go b/weed/s3api/filer_multipart.go index 6be70b8adf..9a66b6f879 100644 --- a/weed/s3api/filer_multipart.go +++ b/weed/s3api/filer_multipart.go @@ -1156,15 +1156,17 @@ func (s3a *S3ApiServer) abortMultipartUpload(input *s3.AbortMultipartUploadInput uploadsFolder := s3a.genUploadsFolder(*input.Bucket) uploadEntry, err := s3a.getEntry(uploadsFolder, *input.UploadId) + // complete and abort both remove .uploads/; a missing record is an upload + // that does not exist (NoSuchUpload on AWS), not one this call discarded if err != nil { if isFilerNotFound(err) { - return &s3.AbortMultipartUploadOutput{}, s3err.ErrNone + return nil, s3err.ErrNoSuchUpload } glog.Errorf("bucket %s abort upload %s: %v", *input.Bucket, *input.UploadId, err) return nil, s3err.ErrInternalError } if uploadEntry == nil { - return &s3.AbortMultipartUploadOutput{}, s3err.ErrNone + return nil, s3err.ErrNoSuchUpload } object := string(uploadEntry.Extended[s3_constants.ExtMultipartObjectKey]) diff --git a/weed/s3api/filer_multipart_abort_test.go b/weed/s3api/filer_multipart_abort_test.go index 37f3945afd..fc1af1d300 100644 --- a/weed/s3api/filer_multipart_abort_test.go +++ b/weed/s3api/filer_multipart_abort_test.go @@ -189,14 +189,36 @@ func TestAbortUndecidableCheckRefuses(t *testing.T) { } } -// No upload directory at all answers success, the same as before. -func TestAbortGoneUploadAnswersSuccess(t *testing.T) { - s3a := newAbortTestServer(t, &fakeAbortFiler{}) +// No upload directory at all is an upload that does not exist: NoSuchUpload +// (404), as on AWS, not a successful abort. +func TestAbortGoneUploadAnswersNoSuchUpload(t *testing.T) { + f := &fakeAbortFiler{} + s3a := newAbortTestServer(t, f) _, code := s3a.abortMultipartUpload(abortInput("gone")) - if code != s3err.ErrNone { - t.Fatalf("code = %v, want ErrNone", code) + if code != s3err.ErrNoSuchUpload { + t.Fatalf("code = %v, want ErrNoSuchUpload", code) + } + if f.deleteReq != nil { + t.Fatalf("delete issued for a missing upload: %+v", f.deleteReq) + } +} + +// Completion removes .uploads/; aborting the completed upload afterwards +// must say NoSuchUpload, not 204, so the client cannot read it as the object +// being discarded, and must not touch the object. +func TestAbortAfterCompleteAnswersNoSuchUpload(t *testing.T) { + f := &fakeAbortFiler{objectEntry: versionEntry("a.bin", "up1")} + s3a := newAbortTestServer(t, f) + + _, code := s3a.abortMultipartUpload(abortInput("up1")) + + if code != s3err.ErrNoSuchUpload { + t.Fatalf("code = %v, want ErrNoSuchUpload", code) + } + if f.deleteReq != nil { + t.Fatalf("delete issued after completion: %+v", f.deleteReq) } } diff --git a/weed/s3api/s3api_object_handlers_multipart.go b/weed/s3api/s3api_object_handlers_multipart.go index c28778a594..c817027f60 100644 --- a/weed/s3api/s3api_object_handlers_multipart.go +++ b/weed/s3api/s3api_object_handlers_multipart.go @@ -546,7 +546,7 @@ func (s3a *S3ApiServer) checkUploadStillOpen(w http.ResponseWriter, r *http.Requ Bucket: aws.String(bucket), Key: objectKey(aws.String(object)), UploadId: aws.String(uploadID), - }); code != s3err.ErrNone { + }); code != s3err.ErrNone && code != s3err.ErrNoSuchUpload { glog.Warningf("checkUploadStillOpen %s/%s: part left behind, cleanup failed", bucket, uploadID) } s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchUpload)