Files
seaweedfs/weed/s3api/s3api_object_handlers_put_ambiguous_test.go
T
Chris Lu ce1e0dc30a s3api: don't delete chunks when CreateEntry outcome is ambiguous (#11376)
* s3api: map ambiguous filer transport errors to retryable 503

Canceled, DeadlineExceeded and Unavailable can be returned after the
filer applied the write, so the outcome is ambiguous. Reporting them as
a 4xx tells the client not to retry; report ServiceUnavailable instead.

* s3api: verify entry existence before deleting orphaned chunks

A failed CreateEntry can still have landed on the filer when the error
is a transport failure, and entryCreated=false would tombstone chunks a
live entry references, leaving a dangling pointer that survives only
because reads pass readDeleted=true until vacuum reclaims the needle.

Before deleting, look the entry up: if it is stored with the same
chunks, the write succeeded; if the lookup cannot be answered, keep the
chunks for vacuum to reclaim; only a confirmed absence still cleans up.

* s3api: regression tests for ambiguous CreateEntry outcomes

Covers the three post-create-failure cases in putToFiler: the entry
landed despite the error (treat as success, keep chunks), the entry is
confirmed absent (delete orphans), and the outcome is unverifiable
(keep chunks, return error).

* volume: count reads served from deleted needles

A readDeleted read succeeding on a tombstoned needle is the signal that
metadata still points at deleted data. Count it under a
readDeletedNeedle handler label in both the Go and Rust volume servers
so the condition is visible before vacuum turns it into a 404.

* s3api: never delete chunks on an ambiguous create error

Review feedback on the first fix showed verification could still go
wrong in both directions: a stale or lagged lookup could report
not-found for a committed entry, a prefix object stores its chunks on a
directory entry, and filer-side manifestization rewrites the top-level
chunk ids the comparison relied on.

Rework the rule so the outcome classes are asymmetric:

- A transport-level error (anything filerErrorToS3Error maps to a
  retryable 503) is ambiguous and never deletes chunks; the lookup can
  only upgrade the write to success.
- Any other error is a definitive filer refusal and still cleans up.

confirmCreateLanded asks the write owner first, resolves the stored
entry through chunk manifests, requires an exact match of the uploaded
file ids, and on success runs the finalize callback the failed create
skipped (under the object write lock, with the same rmObject undo the
create path uses). Zero-chunk writes stay ambiguous since they cannot
be told apart by chunks.

* s3api: cover definitive refusals and stale entries in put tests

The confirmed-failure case now uses a definitive refusal so it still
exercises orphan cleanup, and a new case keeps chunks when the stored
entry belongs to an older object rather than this PUT.

* volume: count deleted-needle reads once per request

Streamed Go reads ran the deleted check in readNeedle and again in
readNeedleDataInto, and non-streamed Rust reads in stream_info and the
full-read fallback, double-counting one request. Count at the single
entry probe each implementation takes per GET: readNeedle in Go,
read_needle_stream_info in Rust.

* s3api: run recovered-write rollback under the object lock

Two follow-ups from review: ResolveChunkManifest returns traversed
manifest blobs in its manifestChunks output, so requiring it empty
rejected every manifestized landing; and the rmObject undo ran after
the object write lock was released, so a concurrent newer write could
be deleted between finalize failure and rollback. Compare only the
resolved data chunks and keep the undo inside the lock.

* s3api: verify, finalize and roll back recovered creates in one lock

A lookup done before the object write lock let a concurrent PUT replace
the entry between the chunk comparison and the finalize/rollback
section, so a failed afterCreate could rmObject a newer write. Run the
owner lookup, manifest resolution, chunk comparison, afterCreate and
the conditional undo inside a single withObjectWriteLock section.
2026-09-17 19:58:49 -07:00

266 lines
8.7 KiB
Go

package s3api
import (
"context"
"fmt"
"io"
"net"
"net/http"
"net/http/httptest"
"strings"
"sync"
"testing"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/proto"
"github.com/seaweedfs/seaweedfs/weed/pb"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
"github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
"github.com/seaweedfs/seaweedfs/weed/wdclient"
)
// fakeVolumeServer serves the two volume-server calls putToFiler makes: chunk
// uploads over HTTP and BatchDelete over gRPC. Deleted fids are recorded so a
// test can tell whether chunk cleanup ran.
type fakeVolumeServer struct {
volume_server_pb.UnimplementedVolumeServerServer
httpAddr string
grpcPort uint32
mu sync.Mutex
deletedFids []string
}
func (f *fakeVolumeServer) BatchDelete(_ context.Context, req *volume_server_pb.BatchDeleteRequest) (*volume_server_pb.BatchDeleteResponse, error) {
f.mu.Lock()
defer f.mu.Unlock()
resp := &volume_server_pb.BatchDeleteResponse{}
for _, fid := range req.FileIds {
f.deletedFids = append(f.deletedFids, fid)
resp.Results = append(resp.Results, &volume_server_pb.DeleteResult{FileId: fid, Status: http.StatusAccepted})
}
return resp, nil
}
func (f *fakeVolumeServer) deleted() []string {
f.mu.Lock()
defer f.mu.Unlock()
return append([]string(nil), f.deletedFids...)
}
func startFakeVolumeServer(t *testing.T) *fakeVolumeServer {
t.Helper()
v := &fakeVolumeServer{}
upload := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
io.Copy(io.Discard, r.Body)
w.Header().Set("Content-MD5", r.Header.Get("Content-MD5"))
w.WriteHeader(http.StatusCreated)
io.WriteString(w, `{"size":1}`)
}))
t.Cleanup(upload.Close)
v.httpAddr = strings.TrimPrefix(upload.URL, "http://")
lis, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
t.Fatal(err)
}
grpcSrv := grpc.NewServer()
volume_server_pb.RegisterVolumeServerServer(grpcSrv, v)
go grpcSrv.Serve(lis)
t.Cleanup(grpcSrv.Stop)
v.grpcPort = uint32(lis.Addr().(*net.TCPAddr).Port)
return v
}
// ambiguousPutFiler fakes the filer calls putToFiler makes. CreateEntry can
// apply the write and still return an error — the ambiguous outcome a
// restarting owner filer produces for issue 11366.
type ambiguousPutFiler struct {
filer_pb.UnimplementedSeaweedFilerServer
volume *fakeVolumeServer
mu sync.Mutex
entries map[string]*filer_pb.Entry
apply bool
createErr error
lookupErr error
lookupFailKey string
nextKey uint64
}
func (f *ambiguousPutFiler) AssignVolume(context.Context, *filer_pb.AssignVolumeRequest) (*filer_pb.AssignVolumeResponse, error) {
f.mu.Lock()
defer f.mu.Unlock()
f.nextKey++
return &filer_pb.AssignVolumeResponse{
FileId: fmt.Sprintf("3,%016x%08x", f.nextKey, uint32(f.nextKey)),
Count: 1,
Location: &filer_pb.Location{
Url: f.volume.httpAddr,
PublicUrl: f.volume.httpAddr,
GrpcPort: f.volume.grpcPort,
},
}, nil
}
func (f *ambiguousPutFiler) CreateEntry(_ context.Context, req *filer_pb.CreateEntryRequest) (*filer_pb.CreateEntryResponse, error) {
f.mu.Lock()
defer f.mu.Unlock()
if f.apply {
entry := proto.Clone(req.Entry).(*filer_pb.Entry)
filer_pb.BeforeEntrySerialization(entry.Chunks)
f.entries[req.Directory+"/"+req.Entry.Name] = entry
}
if f.createErr != nil {
return nil, f.createErr
}
return &filer_pb.CreateEntryResponse{}, nil
}
func (f *ambiguousPutFiler) LookupDirectoryEntry(_ context.Context, req *filer_pb.LookupDirectoryEntryRequest) (*filer_pb.LookupDirectoryEntryResponse, error) {
f.mu.Lock()
defer f.mu.Unlock()
if f.lookupErr != nil && req.Directory+"/"+req.Name == f.lookupFailKey {
return nil, f.lookupErr
}
if entry, ok := f.entries[req.Directory+"/"+req.Name]; ok {
out := proto.Clone(entry).(*filer_pb.Entry)
filer_pb.AfterEntryDeserialization(out.Chunks)
return &filer_pb.LookupDirectoryEntryResponse{Entry: out}, nil
}
return &filer_pb.LookupDirectoryEntryResponse{}, nil
}
func (f *ambiguousPutFiler) LookupVolume(_ context.Context, req *filer_pb.LookupVolumeRequest) (*filer_pb.LookupVolumeResponse, error) {
resp := &filer_pb.LookupVolumeResponse{LocationsMap: map[string]*filer_pb.Locations{}}
for _, vid := range req.VolumeIds {
resp.LocationsMap[vid] = &filer_pb.Locations{Locations: []*filer_pb.Location{{
Url: f.volume.httpAddr,
PublicUrl: f.volume.httpAddr,
GrpcPort: f.volume.grpcPort,
}}}
}
return resp, nil
}
func newPutTestServer(t *testing.T, filerAddr pb.ServerAddress) *S3ApiServer {
t.Helper()
dialOption := grpc.WithTransportCredentials(insecure.NewCredentials())
return &S3ApiServer{
option: &S3ApiServerOption{
Filers: []pb.ServerAddress{filerAddr},
GrpcDialOption: dialOption,
BucketsPath: "/buckets",
},
filerClient: wdclient.NewFilerClient([]pb.ServerAddress{filerAddr}, dialOption, ""),
}
}
func putTestObject(t *testing.T, s3a *S3ApiServer) (string, s3err.ErrorCode) {
t.Helper()
r := httptest.NewRequest(http.MethodPut, "/b/o", nil)
etag, code, _ := s3a.putToFiler(r, "/buckets/b/o", strings.NewReader("hello world"), "b", "o", 1, 0, nil, false, "")
return etag, code
}
// Issue 11366: CreateEntry applied on the filer but the response was lost
// (owner restarting). Once the entry is confirmed, the write is successful —
// deleting the chunks would leave the entry pointing at tombstoned needles.
func TestPutToFilerAmbiguousCreateKeepsChunks(t *testing.T) {
volume := startFakeVolumeServer(t)
filerImpl := &ambiguousPutFiler{
volume: volume,
entries: map[string]*filer_pb.Entry{},
apply: true,
createErr: status.Error(codes.Unavailable, "connect: connection refused"),
}
s3a := newPutTestServer(t, startFakeFiler(t, filerImpl))
etag, code := putTestObject(t, s3a)
if code != s3err.ErrNone {
t.Fatalf("putToFiler returned %v, want success once the entry is confirmed on the filer", code)
}
if etag == "" {
t.Fatal("expected an etag")
}
if deleted := volume.deleted(); len(deleted) != 0 {
t.Fatalf("chunks under a live entry were deleted: %v", deleted)
}
}
// A create the filer definitively refused still cleans up the uploaded chunks.
func TestPutToFilerConfirmedFailureDeletesOrphans(t *testing.T) {
volume := startFakeVolumeServer(t)
filerImpl := &ambiguousPutFiler{
volume: volume,
entries: map[string]*filer_pb.Entry{},
apply: false,
createErr: status.Error(codes.Unknown, "create refused"),
}
s3a := newPutTestServer(t, startFakeFiler(t, filerImpl))
_, code := putTestObject(t, s3a)
if code == s3err.ErrNone {
t.Fatal("expected an error when the entry was not created")
}
if deleted := volume.deleted(); len(deleted) == 0 {
t.Fatal("orphaned chunks were not deleted")
}
}
// A stale entry from an earlier object does not prove this PUT landed: the
// outcome stays unknown, so the new chunks are kept and an error returned.
func TestPutToFilerAmbiguousCreateWithStaleEntryKeepsChunks(t *testing.T) {
volume := startFakeVolumeServer(t)
stale := &filer_pb.Entry{
Name: "o",
Attributes: &filer_pb.FuseAttributes{FileSize: 5},
Chunks: []*filer_pb.FileChunk{{FileId: "3,000000000000009900000099", Size: 5}},
}
filerImpl := &ambiguousPutFiler{
volume: volume,
entries: map[string]*filer_pb.Entry{"/buckets/b/o": stale},
apply: false,
createErr: status.Error(codes.Unavailable, "connect: connection refused"),
}
s3a := newPutTestServer(t, startFakeFiler(t, filerImpl))
_, code := putTestObject(t, s3a)
if code == s3err.ErrNone {
t.Fatal("expected an error when the create outcome is unknown")
}
if deleted := volume.deleted(); len(deleted) != 0 {
t.Fatalf("chunks were deleted while the create outcome was unverifiable: %v", deleted)
}
}
// When neither the create nor the lookup can be answered, the outcome stays
// unknown: keep the chunks (vacuum reclaims orphans) rather than risk deleting
// chunks a live entry references.
func TestPutToFilerUnverifiableCreateKeepsChunks(t *testing.T) {
volume := startFakeVolumeServer(t)
unavailable := status.Error(codes.Unavailable, "connect: connection refused")
filerImpl := &ambiguousPutFiler{
volume: volume,
entries: map[string]*filer_pb.Entry{},
apply: false,
createErr: unavailable,
lookupErr: unavailable,
lookupFailKey: "/buckets/b/o",
}
s3a := newPutTestServer(t, startFakeFiler(t, filerImpl))
_, code := putTestObject(t, s3a)
if code == s3err.ErrNone {
t.Fatal("expected an error when the create outcome is unknown")
}
if deleted := volume.deleted(); len(deleted) != 0 {
t.Fatalf("chunks were deleted while the create outcome was unverifiable: %v", deleted)
}
}