diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 1f01a3ed3..68363f899 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -565,7 +565,7 @@ func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader s3a.deleteOrphanedChunks(chunkResult.FileChunks) } - return "", mapChunkedUploadErrorToS3Error(err), SSEResponseMetadata{} + return "", mapChunkedUploadErrorToS3Error(r.Context(), err), SSEResponseMetadata{} } // Step 3: Calculate MD5 hash and add SSE metadata to chunks @@ -1204,11 +1204,27 @@ func filerErrorToS3Error(err error) s3err.ErrorCode { // IncompleteBody (400) rather than a 500 a reverse proxy would relay as a confusing // 502. Only the source read is tagged, so a volume-server upload fault still maps to // InternalError. -func mapChunkedUploadErrorToS3Error(err error) s3err.ErrorCode { +// +// reqCtx is the request context, which separates the two ways a body ends early: a +// peer that went away, and a body that arrived short while the peer was still there. +// Both surface as the same read error, so without this they are indistinguishable in +// logs even though they point at opposite causes — a network path versus a client. +// The upload itself deliberately runs on a background context, so cancellation of +// reqCtx races the read error; a missed signal degrades to IncompleteBody as before. +// +// This reads cancellation as "the peer is gone", which is what net/http means by it +// today: nothing on the S3 request path cancels reqCtx for its own reasons. Anything +// added later that does — a request budget, an auth deadline, shutdown draining — +// would have to cancel with its own cause and be excluded here, otherwise a body +// truncated at that instant gets attributed to the peer. +func mapChunkedUploadErrorToS3Error(reqCtx context.Context, err error) s3err.ErrorCode { switch { case strings.Contains(err.Error(), s3err.ErrMsgPayloadChecksumMismatch): return s3err.ErrInvalidDigest case errors.Is(err, operation.ErrTruncatedBody): + if errors.Is(reqCtx.Err(), context.Canceled) { + return s3err.ErrClientDisconnected + } return s3err.ErrIncompleteBody default: return s3err.ErrInternalError diff --git a/weed/s3api/s3api_object_handlers_put_error_test.go b/weed/s3api/s3api_object_handlers_put_error_test.go index ff70b12fd..7fda9dbbb 100644 --- a/weed/s3api/s3api_object_handlers_put_error_test.go +++ b/weed/s3api/s3api_object_handlers_put_error_test.go @@ -1,10 +1,12 @@ package s3api import ( + "context" "errors" "fmt" "io" "testing" + "time" "github.com/seaweedfs/seaweedfs/weed/operation" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" @@ -43,9 +45,78 @@ func TestMapChunkedUploadErrorToS3Error(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := mapChunkedUploadErrorToS3Error(tt.err); got != tt.want { + if got := mapChunkedUploadErrorToS3Error(context.Background(), tt.err); got != tt.want { t.Errorf("mapChunkedUploadErrorToS3Error(%v) = %v, want %v", tt.err, got, tt.want) } }) } } + +// A body that ends early because the peer vanished and one that ends early while the +// peer is still connected arrive as the same read error. Only the request context +// tells them apart, and they point at opposite causes, so they must not share a code. +func TestMapChunkedUploadErrorToS3ErrorClientDisconnect(t *testing.T) { + truncated := fmt.Errorf("%w: read chunk at offset %d (got %d bytes): %w", operation.ErrTruncatedBody, 0, 0, io.ErrUnexpectedEOF) + + canceled, cancel := context.WithCancel(context.Background()) + cancel() + + deadline, cancelDeadline := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancelDeadline() + + tests := []struct { + name string + ctx context.Context + err error + want s3err.ErrorCode + }{ + { + name: "peer gone maps to ClientDisconnected", + ctx: canceled, + err: truncated, + want: s3err.ErrClientDisconnected, + }, + { + name: "peer still connected stays IncompleteBody", + ctx: context.Background(), + err: truncated, + want: s3err.ErrIncompleteBody, + }, + { + // A deadline is the server giving up, not the peer leaving, so it must + // not be laundered into a client-side code. + name: "expired deadline stays IncompleteBody", + ctx: deadline, + err: truncated, + want: s3err.ErrIncompleteBody, + }, + { + // Cancellation must not reclassify faults that are not truncations. + name: "server fault under a canceled context stays InternalError", + ctx: canceled, + err: errors.New("assign volume: no free volumes"), + want: s3err.ErrInternalError, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := mapChunkedUploadErrorToS3Error(tt.ctx, tt.err); got != tt.want { + t.Errorf("mapChunkedUploadErrorToS3Error() = %v, want %v", got, tt.want) + } + }) + } +} + +// 499 must stay distinguishable from the 400 it was split out of. +func TestClientDisconnectedAPIError(t *testing.T) { + got := s3err.GetAPIError(s3err.ErrClientDisconnected) + if got.HTTPStatusCode != 499 { + t.Errorf("ClientDisconnected status = %d, want 499", got.HTTPStatusCode) + } + if got.Code != "ClientDisconnected" { + t.Errorf("ClientDisconnected code = %q, want %q", got.Code, "ClientDisconnected") + } + if s3err.GetAPIError(s3err.ErrIncompleteBody).HTTPStatusCode != 400 { + t.Error("IncompleteBody must remain a 400") + } +} diff --git a/weed/s3api/s3err/s3api_errors.go b/weed/s3api/s3err/s3api_errors.go index 6174ef61f..754d1caff 100644 --- a/weed/s3api/s3err/s3api_errors.go +++ b/weed/s3api/s3err/s3api_errors.go @@ -160,6 +160,9 @@ const ( // Truncated request body (fewer bytes than Content-Length) ErrIncompleteBody + + // Peer went away before the request body was fully received + ErrClientDisconnected ) // Error message constants for checksum validation @@ -317,6 +320,13 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "You did not provide the number of bytes specified by the Content-Length HTTP header.", HTTPStatusCode: http.StatusBadRequest, }, + // 499 has no RFC; it is nginx's code for a client that went away, and is what + // log pipelines already recognise for this case. + ErrClientDisconnected: { + Code: "ClientDisconnected", + Description: "The client disconnected before the request body was fully received.", + HTTPStatusCode: 499, + }, ErrInvalidPart: { Code: "InvalidPart",