From 77694d3a9393404d3d6b1edff1184776e5340162 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 2 Jul 2026 13:25:04 -0700 Subject: [PATCH] s3: surface transient store errors during multipart resume/copy instead of masking them (#10207) * s3: surface multipart-list store errors instead of masking them listMultipartUploads masked a filer/metadata-store list error as an empty 200 response, and listObjectParts masked it as NoSuchUpload. A client resuming a multipart upload (the Docker Registry S3 driver resolves an in-progress upload via ListMultipartUploads, then ListParts) reads either as "upload gone" and fails the upload permanently with a non-retryable error. Return ErrInternalError so a transient store error stays retryable, keeping ErrNotFound as an empty list / NoSuchUpload respectively. * s3: return retryable error for transient CopyObject source lookups Resolving the copy source is reported as InvalidArgument ("Copy Source must mention the source bucket and key") whenever the lookup returns any error, including a transient store error. Clients don't retry a 400, so a resumable blob commit fails permanently. Keep the client error for a missing, invalid, directory, or delete-marker source; map a transient store error to ErrInternalError. * s3: mark versioned lookup miss with the not-found sentinel recoverLatestVersionWithoutPointer's terminal miss (a .versions directory with no pointer, no versions, and no null object) returned a plain error, so errors.Is-based callers could not tell this designed NoSuchKey state from a store failure and reported it as an internal error. * s3: reject a delete-marker copy source with NoSuchKey getLatestObjectVersion returns the delete-marker entry when the latest version is a delete marker, and the copy handlers copied its empty stub into the destination. Every other handler detects ExtDeleteMarkerKey and answers NoSuchKey; do the same for CopyObject and UploadPartCopy. * s3: align copy-source not-found detection with the grpc status idiom The lookup path canonicalizes text-form not-found into the sentinel in filer_pb.LookupEntry and wraps with %w after that, so matching sentinel text here was unreachable -- and risky, since a store error whose message merely mentions the sentinel would be downgraded to a terminal 400. Match the raw grpc NotFound code instead, like the other version-lookup sites, and give transient filer errors the 503 that the upload-entry lookup in this file already returns. * s3: keep source-bucket versioning lookup errors retryable in copy CopyObject and UploadPartCopy mapped any source-bucket versioning-state lookup error to a terminal InvalidCopySource, including transient store errors; getVersioningState signals a missing bucket with the not-found sentinel, so split on that and let real store errors surface as 500, the same mapping the destination-bucket lookup already uses. * s3: match grpc-transported not-found in multipart list error paths The list client path returns raw grpc status errors without the sentinel reconstruction that lookups get in filer_pb.LookupEntry, so errors.Is on the not-found sentinel never matched a store-reported missing directory; match the sentinel text as well via a shared helper. The common missing- directory case still lists as empty with no error and is unaffected. * s3: complete and abort multipart surface store errors prepareMultipartCompletionState mapped any upload-directory list or lookup error to NoSuchUpload, so a transient store error at completion time made the client discard a fully-uploaded object as gone. Split on not-found like the sibling listing paths. abortMultipartUpload did the same on s3a.exists, whose errors are never not-found (filer_pb.Exists reports that as false with no error) -- a store failure there answered NoSuchUpload and silently leaked the uploaded parts. * s3: reject part numbers below 1 in UploadPartCopy Only the upper bound was checked, unlike PutObjectPart; partNumber=0 passed the route regex and validation and wrote an out-of-range 0000_copy.part into the upload directory. --- weed/s3api/filer_multipart.go | 36 ++++++++--- weed/s3api/s3_error_utils.go | 12 ++++ weed/s3api/s3api_copy_source_classify_test.go | 48 +++++++++++++++ weed/s3api/s3api_object_handlers_copy.go | 59 ++++++++++++++++--- weed/s3api/s3api_object_versioning.go | 2 +- 5 files changed, 140 insertions(+), 17 deletions(-) create mode 100644 weed/s3api/s3api_copy_source_classify_test.go diff --git a/weed/s3api/filer_multipart.go b/weed/s3api/filer_multipart.go index 5010bce3d1..4bf4f39dc8 100644 --- a/weed/s3api/filer_multipart.go +++ b/weed/s3api/filer_multipart.go @@ -361,8 +361,12 @@ func (s3a *S3ApiServer) prepareMultipartCompletionState(r *http.Request, input * entries, _, err := s3a.list(uploadDirectory, "", "", false, s3_constants.MaxS3MultipartParts+1) if err != nil { glog.Errorf("completeMultipartUpload %s %s error: %v", *input.Bucket, *input.UploadId, err) - stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() - return nil, nil, s3err.ErrNoSuchUpload + if isFilerListNotFound(err) { + stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() + return nil, nil, s3err.ErrNoSuchUpload + } + // a store error must not read as "upload gone": the client would drop a fully-uploaded object + return nil, nil, s3err.ErrInternalError } if len(entries) == 0 { stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() @@ -372,8 +376,11 @@ func (s3a *S3ApiServer) prepareMultipartCompletionState(r *http.Request, input * pentry, err := s3a.getEntry(s3a.genUploadsFolder(*input.Bucket), *input.UploadId) if err != nil { glog.Errorf("completeMultipartUpload %s %s error: %v", *input.Bucket, *input.UploadId, err) - stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() - return nil, nil, s3err.ErrNoSuchUpload + if errors.Is(err, filer_pb.ErrNotFound) { + stats.S3HandlerCounter.WithLabelValues(stats.ErrorCompletedNoSuchUpload).Inc() + return nil, nil, s3err.ErrNoSuchUpload + } + return nil, nil, s3err.ErrInternalError } deleteEntries := make([]*filer_pb.Entry, 0) @@ -882,8 +889,10 @@ func (s3a *S3ApiServer) abortMultipartUpload(input *s3.AbortMultipartUploadInput exists, err := s3a.exists(s3a.genUploadsFolder(*input.Bucket), *input.UploadId, true) if err != nil { - glog.V(1).Infof("bucket %s abort upload %s: %v", *input.Bucket, *input.UploadId, err) - return nil, s3err.ErrNoSuchUpload + // filer_pb.Exists reports not-found as (false, nil), so an error here is + // always a store failure; answering NoSuchUpload would leak the parts. + glog.Errorf("bucket %s abort upload %s: %v", *input.Bucket, *input.UploadId, err) + return nil, s3err.ErrInternalError } if exists { err = s3a.rm(s3a.genUploadsFolder(*input.Bucket), *input.UploadId, true, true) @@ -930,8 +939,14 @@ func (s3a *S3ApiServer) listMultipartUploads(input *s3.ListMultipartUploadsInput entries, _, err := s3a.list(s3a.genUploadsFolder(*input.Bucket), "", *input.UploadIdMarker, false, math.MaxInt32) if err != nil { + // A missing .uploads folder normally lists as empty with no error; a + // store that reports it as not-found still means an empty list. + if isFilerListNotFound(err) { + return output, s3err.ErrNone + } + // surface a real store error; a masked empty 200 makes a resuming client treat the upload as gone glog.Errorf("listMultipartUploads %s error: %v", *input.Bucket, err) - return + return output, s3err.ErrInternalError } uploadsCount := int64(0) @@ -991,8 +1006,13 @@ func (s3a *S3ApiServer) listObjectParts(input *s3.ListPartsInput) (output *ListP entries, isLast, err := s3a.list(s3a.genUploadsFolder(*input.Bucket)+"/"+*input.UploadId, "", fmt.Sprintf("%04d%s", *input.PartNumberMarker, multipartExt), false, uint32(*input.MaxParts)) if err != nil { + // A store that reports the missing upload directory as not-found means + // the upload is gone (completed or aborted), not a store error. + if isFilerListNotFound(err) { + return nil, s3err.ErrNoSuchUpload + } glog.Errorf("listObjectParts %s %s error: %v", *input.Bucket, *input.UploadId, err) - return nil, s3err.ErrNoSuchUpload + return nil, s3err.ErrInternalError } // Note: The upload directory is sort of a marker of the existence of an multipart upload request. diff --git a/weed/s3api/s3_error_utils.go b/weed/s3api/s3_error_utils.go index 0f7b99fdbe..bd38e50b79 100644 --- a/weed/s3api/s3_error_utils.go +++ b/weed/s3api/s3_error_utils.go @@ -1,10 +1,22 @@ package s3api import ( + "errors" + "strings" + "github.com/seaweedfs/seaweedfs/weed/glog" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" ) +// isFilerListNotFound reports whether a filer list error is a not-found. +// Unlike lookups (normalized in filer_pb.LookupEntry), list errors cross gRPC +// as raw status errors, so the sentinel survives only as text. Callers must +// pass errors from paths without user-controlled segments that could spoof it. +func isFilerListNotFound(err error) bool { + return errors.Is(err, filer_pb.ErrNotFound) || strings.Contains(err.Error(), filer_pb.ErrNotFound.Error()) +} + // ErrorHandlers provide common error handling patterns for S3 API operations // handleMultipartError logs an error and returns the standard multipart error format diff --git a/weed/s3api/s3api_copy_source_classify_test.go b/weed/s3api/s3api_copy_source_classify_test.go new file mode 100644 index 0000000000..7a40a90510 --- /dev/null +++ b/weed/s3api/s3api_copy_source_classify_test.go @@ -0,0 +1,48 @@ +package s3api + +import ( + "errors" + "fmt" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func TestClassifyCopySourceError(t *testing.T) { + deleteMarker := &filer_pb.Entry{ + Extended: map[string][]byte{s3_constants.ExtDeleteMarkerKey: []byte("true")}, + } + + tests := []struct { + name string + entry *filer_pb.Entry + err error + want s3err.ErrorCode + }{ + {"regular entry", &filer_pb.Entry{Name: "o"}, nil, s3err.ErrNone}, + {"nil entry", nil, nil, s3err.ErrInvalidCopySource}, + {"directory entry", &filer_pb.Entry{IsDirectory: true}, nil, s3err.ErrInvalidCopySource}, + {"delete marker entry", deleteMarker, nil, s3err.ErrNoSuchKey}, + {"not found sentinel", nil, filer_pb.ErrNotFound, s3err.ErrInvalidCopySource}, + {"wrapped not found", nil, fmt.Errorf("read %s: %w", "o", filer_pb.ErrNotFound), s3err.ErrInvalidCopySource}, + {"grpc not found", nil, status.Error(codes.NotFound, "no entry"), s3err.ErrInvalidCopySource}, + {"invalid version id", nil, errInvalidVersionID, s3err.ErrInvalidCopySource}, + {"delete marker error", nil, ErrDeleteMarker, s3err.ErrInvalidCopySource}, + {"transient unavailable", nil, status.Error(codes.Unavailable, "filer down"), s3err.ErrServiceUnavailable}, + {"store error", nil, errors.New("connection reset"), s3err.ErrInternalError}, + // A message merely mentioning the sentinel text must not downgrade a store error. + {"sentinel text in message", nil, fmt.Errorf("peer said: %s", filer_pb.ErrNotFound.Error()), s3err.ErrInternalError}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := classifyCopySourceError(tt.entry, tt.err); got != tt.want { + t.Errorf("classifyCopySourceError() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 3da0370de6..e15a3098b0 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -24,6 +24,8 @@ import ( "github.com/seaweedfs/seaweedfs/weed/security" "github.com/seaweedfs/seaweedfs/weed/util" util_http "github.com/seaweedfs/seaweedfs/weed/util/http" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -67,6 +69,31 @@ func hasPrefixFold(s, prefix string) bool { return len(s) >= len(prefix) && strings.EqualFold(s[:len(prefix)], prefix) } +// classifyCopySourceError maps a copy-source lookup to an S3 error: a missing, +// invalid, or directory source stays a client error, but a transient store +// error becomes a retryable 5xx so a resumable copy/commit survives a blip. +func classifyCopySourceError(entry *filer_pb.Entry, err error) s3err.ErrorCode { + if err == nil { + if entry == nil || entry.IsDirectory { + return s3err.ErrInvalidCopySource + } + // The latest-version pointer tracks delete markers too; copying one + // must be NoSuchKey like every other handler, not a copy of the stub. + if deleteMarker, ok := entry.Extended[s3_constants.ExtDeleteMarkerKey]; ok && string(deleteMarker) == "true" { + return s3err.ErrNoSuchKey + } + return s3err.ErrNone + } + if errors.Is(err, filer_pb.ErrNotFound) || status.Code(err) == codes.NotFound || + errors.Is(err, errInvalidVersionID) || errors.Is(err, ErrDeleteMarker) { + return s3err.ErrInvalidCopySource + } + if isTransientFilerError(err) { + return s3err.ErrServiceUnavailable + } + return s3err.ErrInternalError +} + func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request) { t := time.Now().UTC().Truncate(time.Millisecond) @@ -144,13 +171,19 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request srcVersioningState, err := s3a.getVersioningState(srcBucket) if err != nil { glog.Errorf("Error checking versioning state for source bucket %s: %v", srcBucket, err) - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + // Only a missing bucket is the client's fault; a store error must stay + // retryable, matching the destination-bucket lookup. + if errors.Is(err, filer_pb.ErrNotFound) { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + return + } + s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) return } entry, err := s3a.resolveCopySourceEntry(srcBucket, srcObject, srcVersionId, srcVersioningState) - if err != nil || entry.IsDirectory { - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + if errCode := classifyCopySourceError(entry, err); errCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, errCode) return } @@ -216,8 +249,8 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request routeInPlace := owner != "" && dstVersioningState == "" && !mimeChanged selfCopyBody := func() s3err.ErrorCode { currentEntry, currentErr := s3a.resolveCopySourceEntry(srcBucket, srcObject, srcVersionId, srcVersioningState) - if currentErr != nil || currentEntry.IsDirectory { - return s3err.ErrInvalidCopySource + if errCode := classifyCopySourceError(currentEntry, currentErr); errCode != s3err.ErrNone { + return errCode } if errCode := s3a.validateConditionalCopyHeaders(r, currentEntry); errCode != s3err.ErrNone { return errCode @@ -828,12 +861,22 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req s3err.WriteErrorResponse(w, r, s3err.ErrInvalidPart) return } + if partID < 1 { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidPart) + return + } // Get detailed versioning state for source bucket srcVersioningState, err := s3a.getVersioningState(srcBucket) if err != nil { glog.Errorf("Error checking versioning state for source bucket %s: %v", srcBucket, err) - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + // Only a missing bucket is the client's fault; a store error must stay + // retryable, matching the destination-bucket lookup. + if errors.Is(err, filer_pb.ErrNotFound) { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + return + } + s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) return } @@ -856,8 +899,8 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req entry, err = s3a.getEntry(dir, name) } - if err != nil || entry.IsDirectory { - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + if errCode := classifyCopySourceError(entry, err); errCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, errCode) return } diff --git a/weed/s3api/s3api_object_versioning.go b/weed/s3api/s3api_object_versioning.go index fce90a8250..bea7dfb48d 100644 --- a/weed/s3api/s3api_object_versioning.go +++ b/weed/s3api/s3api_object_versioning.go @@ -1849,7 +1849,7 @@ func (s3a *S3ApiServer) recoverLatestVersionWithoutPointer(bucket, normalizedObj return nil, healErr } - return nil, fmt.Errorf("no version metadata in .versions directory and no regular object found for %s/%s", bucket, normalizedObject) + return nil, fmt.Errorf("no version metadata in .versions directory and no regular object found for %s/%s: %w", bucket, normalizedObject, filer_pb.ErrNotFound) } // versionIdFromEntry returns a .versions child entry's version id, preferring