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
This commit is contained in:
Chris Lu
2026-03-29 13:46:54 -07:00
committed by GitHub
parent 937a168d34
commit 0761be58d3
3 changed files with 103 additions and 3 deletions

View File

@@ -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)
}
}
})
}
}