Report a delete the filer rejected instead of answering success (#11003)

* s3tables: report a delete the filer rejected

deleteDirectory discarded DeleteEntryResponse and checked only the
transport error, so DeleteTable, DeleteNamespace, DeleteView and
DeleteTableBucket answered 200 for a delete the filer refused. Call
filer_pb.DoRemove, which reads resp.Error and still treats a missing
entry as success.

Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU

* admin: report a delete the filer rejected

The bucket delete, the file browser handlers and the topic retention
purger all discarded DeleteEntryResponse, so a delete the filer refused
came back as success. Call filer_pb.DoRemove, which reads resp.Error.

Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU

* credential: report a delete the filer rejected

DeleteUser, DeletePolicy and the full-sync cleanup loops discarded
DeleteEntryResponse, so a rejected delete answered success and left the
credential file in place. The service account path in the same store
already read resp.Error; the rest now do too, via filer_pb.DoRemove
where not-found is already tolerated.

Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU

* shell: report a delete the filer rejected

remote.configure -delete, remote.cache and the remote metadata sync
discarded DeleteEntryResponse, so a rejected delete printed as removed.
Call filer_pb.DoRemove, which reads resp.Error.

Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU

* mq: report a delete the filer rejected

The consumer offset group purge and the coordinator assignment delete
discarded DeleteEntryResponse. Call filer_pb.DoRemove, which reads
resp.Error.

Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU

* iam: count only the revocation entries the filer actually deleted

The expiry sweep discarded DeleteEntryResponse, so a rejected delete was
counted as purged and the entry stayed. Call filer_pb.DoRemove, which
reads resp.Error, matching the role and provider stores beside it.

Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU

* mount: fail rmdir when the unary fallback delete was rejected

The streaming branch turns DeleteEntryResponse.Error into an error, the
unary fallback dropped it, so rmdir of a non-empty directory answered OK
off the stream and ENOTEMPTY on it. Surface it in both.

Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU

* s3tables: fail DeleteTableBucket when the directory delete is refused

The handler only failed when both the leaf entry and the directory
delete failed, so a refused bucket directory delete still answered 200
with the bucket in place. The directory is the bucket, so it decides;
the leaf entry stays best-effort.

Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU
This commit is contained in:
Chris Lu
2026-08-27 22:29:48 -07:00
committed by GitHub
parent 742b2f5896
commit 9e06e1d0f9
16 changed files with 153 additions and 126 deletions
+2 -8
View File
@@ -1233,14 +1233,8 @@ func (s *AdminServer) DeleteS3Bucket(bucketName string) error {
// Then delete bucket directory recursively from filer
// Use same parameters as s3.bucket.delete shell command and S3 API
return s.WithFilerClient(func(client filer_pb.SeaweedFilerClient) error {
_, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: filerConfig.BucketsPath,
Name: bucketName,
IsDeleteData: false, // Collection already deleted, just remove metadata
IsRecursive: true,
IgnoreRecursiveError: true, // Same as S3 API and shell command
})
if err != nil {
// The collection is already gone, so this only has to drop the metadata.
if err := filer_pb.DoRemove(ctx, client, filerConfig.BucketsPath, bucketName, false, true, true, false, nil); err != nil {
return fmt.Errorf("failed to delete bucket: %w", err)
}
+2 -10
View File
@@ -277,11 +277,7 @@ func (p *TopicRetentionPurger) deleteDirectoryRecursively(client filer_pb.Seawee
}
} else {
// Delete file
_, err = client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{
Directory: dirPath,
Name: resp.Entry.Name,
})
if err != nil {
if err := filer_pb.DoRemove(context.Background(), client, dirPath, resp.Entry.Name, false, false, false, false, nil); err != nil {
return fmt.Errorf("failed to delete file %s: %v", entryPath, err)
}
}
@@ -291,11 +287,7 @@ func (p *TopicRetentionPurger) deleteDirectoryRecursively(client filer_pb.Seawee
parentDir := path.Dir(dirPath)
dirName := path.Base(dirPath)
_, err = client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{
Directory: parentDir,
Name: dirName,
})
if err != nil {
if err := filer_pb.DoRemove(context.Background(), client, parentDir, dirName, false, false, false, false, nil); err != nil {
return fmt.Errorf("failed to delete directory %s: %v", dirPath, err)
}
+2 -16
View File
@@ -109,14 +109,7 @@ func (h *FileBrowserHandlers) DeleteFile(w http.ResponseWriter, r *http.Request)
// Delete file via filer
err := h.adminServer.WithFilerClient(func(client filer_pb.SeaweedFilerClient) error {
_, err := client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{
Directory: path.Dir(request.Path),
Name: path.Base(request.Path),
IsDeleteData: true,
IsRecursive: true,
IgnoreRecursiveError: false,
})
return err
return filer_pb.DoRemove(context.Background(), client, path.Dir(request.Path), path.Base(request.Path), true, true, false, false, nil)
})
if err != nil {
writeJSONError(w, http.StatusInternalServerError, "Failed to delete file: "+err.Error())
@@ -156,14 +149,7 @@ func (h *FileBrowserHandlers) DeleteMultipleFiles(w http.ResponseWriter, r *http
// Delete each file/folder
for _, p := range request.Paths {
err := h.adminServer.WithFilerClient(func(client filer_pb.SeaweedFilerClient) error {
_, err := client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{
Directory: path.Dir(p),
Name: path.Base(p),
IsDeleteData: true,
IsRecursive: true,
IgnoreRecursiveError: false,
})
return err
return filer_pb.DoRemove(context.Background(), client, path.Dir(p), path.Base(p), true, true, false, false, nil)
})
if err != nil {
+10 -16
View File
@@ -209,10 +209,7 @@ func (store *FilerEtcStore) SaveConfiguration(ctx context.Context, config *iam_p
for _, entry := range entries {
if !entry.IsDirectory && !validNames[entry.Name] {
// Delete obsolete identity file
if _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: dir,
Name: entry.Name,
}); err != nil {
if err := filer_pb.DoRemove(ctx, client, dir, entry.Name, false, false, false, false, nil); err != nil {
glog.Warningf("Failed to delete obsolete identity file %s: %v", entry.Name, err)
}
}
@@ -240,10 +237,7 @@ func (store *FilerEtcStore) SaveConfiguration(ctx context.Context, config *iam_p
for _, entry := range entries {
if !entry.IsDirectory && !validNames[entry.Name] {
if _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: dir,
Name: entry.Name,
}); err != nil {
if err := filer_pb.DoRemove(ctx, client, dir, entry.Name, false, false, false, false, nil); err != nil {
glog.Warningf("Failed to delete obsolete service account file %s: %v", entry.Name, err)
}
}
@@ -271,14 +265,8 @@ func (store *FilerEtcStore) SaveConfiguration(ctx context.Context, config *iam_p
for _, entry := range entries {
if !entry.IsDirectory && !validNames[entry.Name] {
resp, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: dir,
Name: entry.Name,
})
if err != nil {
if err := filer_pb.DoRemove(ctx, client, dir, entry.Name, false, false, false, false, nil); err != nil {
glog.Warningf("Failed to delete obsolete group file %s: %v", entry.Name, err)
} else if resp != nil && resp.Error != "" {
glog.Warningf("Failed to delete obsolete group file %s: %s", entry.Name, resp.Error)
}
}
}
@@ -341,7 +329,7 @@ func (store *FilerEtcStore) DeleteUser(ctx context.Context, username string) err
}
return store.withFilerClient(func(client filer_pb.SeaweedFilerClient) error {
_, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
resp, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: filer.IamConfigDirectory + "/" + IamIdentitiesDirectory,
Name: username + ".json",
})
@@ -351,6 +339,12 @@ func (store *FilerEtcStore) DeleteUser(ctx context.Context, username string) err
}
return err
}
if resp != nil && resp.Error != "" {
if strings.Contains(resp.Error, filer_pb.ErrNotFound.Error()) {
return credential.ErrUserNotFound
}
return fmt.Errorf("delete user %s: %s", username, resp.Error)
}
return nil
})
}
@@ -263,14 +263,7 @@ func (store *FilerEtcStore) DeletePolicy(ctx context.Context, name string) error
}
if err := store.withFilerClient(func(client filer_pb.SeaweedFilerClient) error {
_, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: filer.IamConfigDirectory + "/" + IamPoliciesDirectory,
Name: name + ".json",
})
if err != nil && !strings.Contains(err.Error(), filer_pb.ErrNotFound.Error()) {
return err
}
return nil
return filer_pb.DoRemove(ctx, client, filer.IamConfigDirectory+"/"+IamPoliciesDirectory, name+".json", false, false, false, false, nil)
}); err != nil {
return err
}
+1 -5
View File
@@ -235,11 +235,7 @@ func (f *FilerSessionRevocationStore) Purge(ctx context.Context, filerAddress st
if entry.ExpiresAt.IsZero() || entry.ExpiresAt.After(before) {
continue
}
if _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: f.basePath,
Name: resp.Entry.Name,
IsDeleteData: true,
}); err == nil {
if err := filer_pb.DoRemove(ctx, client, f.basePath, resp.Entry.Name, true, false, false, false, nil); err == nil {
count++
}
}
+13 -2
View File
@@ -3,6 +3,7 @@ package mount
import (
"context"
"errors"
"syscall"
"github.com/seaweedfs/seaweedfs/weed/glog"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
@@ -60,7 +61,17 @@ func (wfs *WFS) streamDeleteEntry(ctx context.Context, req *filer_pb.DeleteEntry
err := wfs.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
var err error
resp, err = client.DeleteEntry(ctx, req)
return err
if err != nil {
return err
}
// A refused delete arrives in the response, as the stream branch above.
if resp.Error != "" {
return &streamMutateError{msg: resp.Error, errno: syscall.EIO}
}
return nil
})
return resp, err
if err != nil {
return nil, err
}
return resp, nil
}
@@ -296,13 +296,6 @@ func (f *FilerStorage) deleteDirectory(path string) error {
dir, name := fullPath.DirAndName()
return f.fca.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
_, err := client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{
Directory: dir,
Name: name,
IsDeleteData: true,
IsRecursive: true,
IgnoreRecursiveError: true,
})
return err
return filer_pb.DoRemove(context.Background(), client, dir, name, true, true, true, false, nil)
})
}
@@ -751,12 +751,7 @@ func (cr *CoordinatorRegistry) deleteCoordinatorAssignment(consumerGroup string)
fileName := fmt.Sprintf("%s_assignments.json", consumerGroup)
filePath := fmt.Sprintf("%s/%s", CoordinatorAssignmentsDir, fileName)
_, err := client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{
Directory: CoordinatorAssignmentsDir,
Name: fileName,
})
if err != nil {
if err := filer_pb.DoRemove(context.Background(), client, CoordinatorAssignmentsDir, fileName, false, false, false, false, nil); err != nil {
return fmt.Errorf("failed to delete assignment file %s: %w", filePath, err)
}
+1 -9
View File
@@ -295,17 +295,9 @@ func (h *S3TablesHandler) deleteExtendedAttribute(ctx context.Context, client fi
}
// deleteDirectory deletes a directory and all its contents
// Note: DeleteEntry RPC response doesn't have an Error field, so we only check the RPC err
func (h *S3TablesHandler) deleteDirectory(ctx context.Context, client filer_pb.SeaweedFilerClient, path string) error {
dir, name := splitPath(path)
_, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: dir,
Name: name,
IsDeleteData: true,
IsRecursive: true,
IgnoreRecursiveError: true,
})
return err
return filer_pb.DoRemove(ctx, client, dir, name, true, true, true, false, nil)
}
// entryExists checks if an entry exists at the given path
+102
View File
@@ -0,0 +1,102 @@
package s3tables
import (
"context"
"encoding/json"
"errors"
"testing"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
// deleteEntryStub answers DeleteEntry the way the filer does: a rejected delete
// comes back as a nil transport error with the reason in the response.
type deleteEntryStub struct {
filer_pb.SeaweedFilerClient
resp *filer_pb.DeleteEntryResponse
err error
req *filer_pb.DeleteEntryRequest
}
func (s *deleteEntryStub) DeleteEntry(_ context.Context, req *filer_pb.DeleteEntryRequest, _ ...grpc.CallOption) (*filer_pb.DeleteEntryResponse, error) {
s.req = req
return s.resp, s.err
}
func TestDeleteDirectoryReportsRejectedDelete(t *testing.T) {
stub := &deleteEntryStub{resp: &filer_pb.DeleteEntryResponse{Error: "fail to delete non-empty folder"}}
err := (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t")
require.Error(t, err, "a delete the filer rejected must not be reported as success")
assert.Contains(t, err.Error(), "fail to delete non-empty folder")
}
func TestDeleteDirectoryToleratesMissingEntry(t *testing.T) {
t.Run("reported in the response", func(t *testing.T) {
stub := &deleteEntryStub{resp: &filer_pb.DeleteEntryResponse{Error: filer_pb.ErrNotFound.Error()}}
assert.NoError(t, (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t"))
})
t.Run("reported as a transport error", func(t *testing.T) {
stub := &deleteEntryStub{err: status.Error(codes.NotFound, filer_pb.ErrNotFound.Error())}
assert.NoError(t, (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t"))
})
}
func TestDeleteDirectoryPropagatesTransportError(t *testing.T) {
stub := &deleteEntryStub{err: errors.New("filer unreachable")}
err := (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t")
require.Error(t, err)
assert.Contains(t, err.Error(), "filer unreachable")
}
// deleteDirectory drops a whole subtree, so the delete has to stay recursive and
// take the data with it.
func TestDeleteDirectoryDeletesTheSubtree(t *testing.T) {
stub := &deleteEntryStub{resp: &filer_pb.DeleteEntryResponse{}}
require.NoError(t, (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t"))
require.NotNil(t, stub.req)
assert.Equal(t, "/buckets/b/ns", stub.req.Directory)
assert.Equal(t, "t", stub.req.Name)
assert.True(t, stub.req.IsDeleteData)
assert.True(t, stub.req.IsRecursive)
assert.True(t, stub.req.IgnoreRecursiveError)
}
// The bucket is the directory, so DeleteTableBucket must not answer success
// when the filer refused to remove it.
func TestDeleteTableBucketReportsRejectedDirectoryDelete(t *testing.T) {
fs := s3tablestest.Start(t)
bucketMeta, err := json.Marshal(tableBucketMetadata{Name: renameTestBucket, OwnerAccountID: DefaultAccountID})
require.NoError(t, err)
fs.Put(TablesPath, renameTestBucket, map[string][]byte{
ExtendedKeyTableBucket: []byte("{}"),
ExtendedKeyMetadata: bucketMeta,
})
bucketPath := GetTableBucketPath(renameTestBucket)
fs.RejectDelete = func(dir, name string) string {
if dir+"/"+name == bucketPath {
return "fail to delete non-empty folder"
}
return ""
}
m := NewManager()
m.SetTrusted(true)
err = m.Execute(context.Background(), NewManagerClient(fs.Client), "DeleteTableBucket",
&DeleteTableBucketRequest{TableBucketARN: mustBucketARN(t)}, nil, "")
require.Error(t, err, "a bucket the filer refused to delete must not be reported as deleted")
assert.NotNil(t, fs.Get(TablesPath, renameTestBucket), "the bucket is still there")
}
@@ -347,22 +347,13 @@ func (h *S3TablesHandler) handleDeleteTableBucket(w http.ResponseWriter, r *http
// Delete the bucket
err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
// Delete table object entry first, then directory
// This ensures we clean up the leaf entry even if directory deletion fails
tableObjErr := h.deleteEntryIfExists(r.Context(), client, GetTableObjectBucketPath(bucketName))
dirErr := h.deleteDirectory(r.Context(), client, bucketPath)
// Log any errors but don't fail if one succeeds
if tableObjErr != nil && dirErr != nil {
return fmt.Errorf("delete table object failed: %w, delete directory failed: %w", tableObjErr, dirErr)
}
if tableObjErr != nil {
// Drop the leaf entry first, so it does not outlive the directory.
if tableObjErr := h.deleteEntryIfExists(r.Context(), client, GetTableObjectBucketPath(bucketName)); tableObjErr != nil {
glog.V(1).Infof("failed to delete table object for %s: %v", bucketName, tableObjErr)
}
if dirErr != nil {
glog.V(1).Infof("failed to delete table bucket dir for %s: %v", bucketName, dirErr)
}
return nil
// The bucket is the directory, so a refused delete leaves it in place
// and the caller must hear about it.
return h.deleteDirectory(r.Context(), client, bucketPath)
})
if err != nil {
@@ -34,6 +34,9 @@ type MemFiler struct {
// BeforeUpdate runs once, at the start of the next UpdateEntry, so a test
// can land a competing write in a handler's read-to-write window.
BeforeUpdate func()
// RejectDelete returns the reason DeleteEntry refuses a path, answered the
// way the filer answers one: no transport error, the reason in the response.
RejectDelete func(dir, name string) string
}
func newMemFiler() *MemFiler {
@@ -167,6 +170,11 @@ func (f *MemFiler) UpdateEntry(_ context.Context, req *filer_pb.UpdateEntryReque
}
func (f *MemFiler) DeleteEntry(_ context.Context, req *filer_pb.DeleteEntryRequest) (*filer_pb.DeleteEntryResponse, error) {
if reject := f.RejectDelete; reject != nil {
if reason := reject(req.Directory, req.Name); reason != "" {
return &filer_pb.DeleteEntryResponse{Error: reason}, nil
}
}
f.mu.Lock()
defer f.mu.Unlock()
if d, ok := f.entries[req.Directory]; ok {
+1 -8
View File
@@ -230,14 +230,7 @@ func (c *commandRemoteCache) doComprehensiveSync(commandEnv *CommandEnv, writer
fmt.Fprintf(writer, "Deleting %s... ", pathToDelete)
dir, name := util.FullPath(pathToDelete).DirAndName()
_, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: dir,
Name: name,
IgnoreRecursiveError: false,
IsDeleteData: true,
IsRecursive: false,
IsFromOtherCluster: false,
})
err := filer_pb.DoRemove(ctx, client, dir, name, true, false, false, false, nil)
if err != nil {
fmt.Fprintf(writer, "failed: %v\n", err)
return err
+2 -10
View File
@@ -179,16 +179,8 @@ func (c *commandRemoteConfigure) deleteRemoteStorage(commandEnv *CommandEnv, wri
return commandEnv.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
request := &filer_pb.DeleteEntryRequest{
Directory: filer.DirectoryEtcRemote,
Name: storageName + filer.REMOTE_STORAGE_CONF_SUFFIX,
IgnoreRecursiveError: false,
IsDeleteData: true,
IsRecursive: true,
IsFromOtherCluster: false,
Signatures: nil,
}
_, err := client.DeleteEntry(context.Background(), request)
name := storageName + filer.REMOTE_STORAGE_CONF_SUFFIX
err := filer_pb.DoRemove(context.Background(), client, filer.DirectoryEtcRemote, name, true, true, false, false, nil)
if err == nil {
fmt.Fprintf(writer, "removed: %s\n", storageName)
+1 -6
View File
@@ -326,12 +326,7 @@ func deleteRemoteBackedEntry(ctx context.Context, client filer_pb.SeaweedFilerCl
}
func deleteLocalEntry(ctx context.Context, client filer_pb.SeaweedFilerClient, localDir util.FullPath, name string) error {
_, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{
Directory: string(localDir),
Name: name,
IsDeleteData: true,
})
return err
return filer_pb.DoRemove(ctx, client, string(localDir), name, true, false, false, false, nil)
}
func isLocalDirectoryEmpty(ctx context.Context, client filer_pb.SeaweedFilerClient, dir util.FullPath) (bool, error) {