mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-08-16 04:06:44 +00:00
s3: report a peer that went away as ClientDisconnected, not IncompleteBody (#10511)
* s3: report a peer that went away as ClientDisconnected, not IncompleteBody A streaming PUT whose body ends early is always reported as IncompleteBody (400). That collapses two cases with opposite causes: the peer vanished mid-upload, and the peer sent fewer bytes than it promised while still connected. The first points at the network path, the second at the client, and once merged they cannot be told apart from the logs. Split out ClientDisconnected (499) and select it when the request context shows the peer is gone. The upload itself keeps running on a background context so chunks still finish, which means cancellation races the read error; a missed signal degrades to IncompleteBody exactly as before. * s3: note what request-context cancellation is taken to mean
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user