From 0761be58d3ae3d21bb6ab388507c915216ec0883 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 29 Mar 2026 13:46:54 -0700 Subject: [PATCH] fix(s3): preserve explicit directory markers during empty folder cleanup (#8831) * fix(s3): preserve explicit directory markers during empty folder cleanup PR #8292 switched empty-folder cleanup from per-folder implicit checks to bucket-level policy, inadvertently dropping the check that preserved explicitly created directories (e.g., PUT /bucket/folder/). This caused user-created folders to be deleted when their last file was removed. Add IsDirectoryKeyObject check in executeCleanup to skip folders that have a MIME type set, matching the canonical pattern used throughout the S3 listing and delete handlers. * fix: handle ErrNotFound in IsDirectoryKeyObject for race safety Entry may be deleted between the emptiness check and the directory marker lookup. Treat not-found as false rather than propagating the error, avoiding unnecessary error logging in the cleanup path. * refactor: consolidate directory marker tests and tidy error handling - Combine two separate test functions into a table-driven test - Nest ErrNotFound check inside the err != nil block --- .../empty_folder_cleaner.go | 11 +++ .../empty_folder_cleaner_test.go | 81 ++++++++++++++++++- weed/filer/filer.go | 14 ++++ 3 files changed, 103 insertions(+), 3 deletions(-) diff --git a/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go b/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go index 42a313204..6af15a464 100644 --- a/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go +++ b/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go @@ -27,6 +27,7 @@ type FilerOperations interface { CountDirectoryEntries(ctx context.Context, dirPath util.FullPath, limit int) (count int, err error) DeleteEntryMetaAndData(ctx context.Context, p util.FullPath, isRecursive, ignoreRecursiveError, shouldDeleteChunks, isFromOtherCluster bool, signatures []int32, ifNotModifiedAfter int64) error GetEntryAttributes(ctx context.Context, p util.FullPath) (attributes map[string][]byte, err error) + IsDirectoryKeyObject(ctx context.Context, p util.FullPath) (bool, error) } // folderState tracks the state of a folder for empty folder cleanup @@ -312,6 +313,16 @@ func (efc *EmptyFolderCleaner) executeCleanup(folder string, triggeredBy string) return } + // Skip explicitly created directory markers (e.g., PUT /bucket/folder/) + // These have a MIME type set and should be preserved even when empty + if isKeyObj, err := efc.filer.IsDirectoryKeyObject(ctx, util.FullPath(folder)); err != nil { + glog.V(2).Infof("EmptyFolderCleaner: error checking directory key object %s: %v", folder, err) + return + } else if isKeyObj { + glog.V(3).Infof("EmptyFolderCleaner: skipping %s (triggered by %s), explicit directory marker", folder, triggeredBy) + return + } + // Delete the empty folder glog.Infof("EmptyFolderCleaner: deleting empty folder %s (triggered by %s)", folder, triggeredBy) if err := efc.deleteFolder(ctx, folder); err != nil { diff --git a/weed/filer/empty_folder_cleanup/empty_folder_cleaner_test.go b/weed/filer/empty_folder_cleanup/empty_folder_cleaner_test.go index 25cf1ec68..645746d55 100644 --- a/weed/filer/empty_folder_cleanup/empty_folder_cleaner_test.go +++ b/weed/filer/empty_folder_cleanup/empty_folder_cleaner_test.go @@ -12,9 +12,10 @@ import ( ) type mockFilerOps struct { - countFn func(path util.FullPath) (int, error) - deleteFn func(path util.FullPath) error - attrsFn func(path util.FullPath) (map[string][]byte, error) + countFn func(path util.FullPath) (int, error) + deleteFn func(path util.FullPath) error + attrsFn func(path util.FullPath) (map[string][]byte, error) + isDirKeyObjFn func(path util.FullPath) (bool, error) } func (m *mockFilerOps) CountDirectoryEntries(_ context.Context, dirPath util.FullPath, _ int) (int, error) { @@ -38,6 +39,13 @@ func (m *mockFilerOps) GetEntryAttributes(_ context.Context, p util.FullPath) (m return m.attrsFn(p) } +func (m *mockFilerOps) IsDirectoryKeyObject(_ context.Context, p util.FullPath) (bool, error) { + if m.isDirKeyObjFn == nil { + return false, nil + } + return m.isDirKeyObjFn(p) +} + func Test_isUnderPath(t *testing.T) { tests := []struct { name string @@ -733,3 +741,70 @@ func TestEmptyFolderCleaner_executeCleanup_bucketPolicyDisabledSkips(t *testing. t.Fatalf("expected folder %s to be skipped, got deletions %v", folder, deleted) } } + +func TestEmptyFolderCleaner_executeCleanup_directoryMarker(t *testing.T) { + testCases := []struct { + name string + isDirKeyObj bool + expectDeletion bool + }{ + { + name: "skips explicit directory marker", + isDirKeyObj: true, + expectDeletion: false, + }, + { + name: "deletes implicit empty folder", + isDirKeyObj: false, + expectDeletion: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + lockRing := lock_manager.NewLockRing(5 * time.Second) + lockRing.SetSnapshot([]pb.ServerAddress{"filer1:8888"}) + + var deleted []string + mock := &mockFilerOps{ + countFn: func(_ util.FullPath) (int, error) { + return 0, nil + }, + deleteFn: func(path util.FullPath) error { + deleted = append(deleted, string(path)) + return nil + }, + isDirKeyObjFn: func(path util.FullPath) (bool, error) { + return tc.isDirKeyObj, nil + }, + } + + cleaner := &EmptyFolderCleaner{ + filer: mock, + lockRing: lockRing, + host: "filer1:8888", + bucketPath: "/buckets", + enabled: true, + folderCounts: make(map[string]*folderState), + cleanupQueue: NewCleanupQueue(1000, time.Minute), + maxCountCheck: 1000, + cacheExpiry: time.Minute, + processorSleep: time.Second, + stopCh: make(chan struct{}), + } + + folder := "/buckets/test/folder" + cleaner.executeCleanup(folder, "triggered_item") + + if tc.expectDeletion { + if len(deleted) != 1 || deleted[0] != folder { + t.Fatalf("expected implicit empty folder %s to be deleted, got deletions %v", folder, deleted) + } + } else { + if len(deleted) != 0 { + t.Fatalf("expected explicit directory marker %s to be preserved, got deletions %v", folder, deleted) + } + } + }) + } +} diff --git a/weed/filer/filer.go b/weed/filer/filer.go index 87ecb28c5..49a37b138 100644 --- a/weed/filer/filer.go +++ b/weed/filer/filer.go @@ -559,3 +559,17 @@ func (f *Filer) GetEntryAttributes(ctx context.Context, p util.FullPath) (map[st } return entry.Extended, nil } + +func (f *Filer) IsDirectoryKeyObject(ctx context.Context, p util.FullPath) (bool, error) { + entry, err := f.FindEntry(ctx, p) + if err != nil { + if errors.Is(err, filer_pb.ErrNotFound) { + return false, nil + } + return false, err + } + if entry == nil { + return false, nil + } + return entry.IsDirectory() && entry.Mime != "", nil +}