From f60ab97b2ed290c8769c47dd27830b183af50c33 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sat, 21 Mar 2026 14:08:08 -0700 Subject: [PATCH] fix: use fail-open for directory marker check in empty-folder cleaner The isDirectoryMarker check used fail-closed behavior: any transient error from GetEntryAttributes would permanently cache the folder as a directory marker, blocking cleanup forever. Change to fail-open: on transient errors, allow deletion to proceed and don't cache the result so it can be retried on the next cycle. This is safe because directory markers are intentional objects that users can recreate, while blocked cleanup causes silent data leaks. Also increase the Spark test cleanup timeout from 35s to 60s to accommodate cascading multi-level directory cleanup with the additional store lookups. --- test/s3/spark/issue_8285_repro_test.go | 2 +- .../empty_folder_cleaner.go | 43 +++++++++++-------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/test/s3/spark/issue_8285_repro_test.go b/test/s3/spark/issue_8285_repro_test.go index 66b46439d..b715bfaf4 100644 --- a/test/s3/spark/issue_8285_repro_test.go +++ b/test/s3/spark/issue_8285_repro_test.go @@ -70,7 +70,7 @@ print("WRITE_COUNT=" + str(count)) "issue-8285/output/_temporary/0/", "issue-8285/output/_temporary/0/_temporary/", } - lingering := waitForObjectsToDisappear(t, env, "test", temporaryCandidates, 35*time.Second) + lingering := waitForObjectsToDisappear(t, env, "test", temporaryCandidates, 60*time.Second) if len(lingering) > 0 { t.Fatalf("issue #8285 regression detected: lingering temporary directories: %v", lingering) } diff --git a/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go b/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go index 1103d1d56..7c7d110f7 100644 --- a/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go +++ b/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go @@ -324,11 +324,14 @@ func (efc *EmptyFolderCleaner) executeCleanup(folder string, triggeredBy string) isMarker = *state.isDirectoryMarker } else { efc.mu.Unlock() - isMarker = efc.isDirectoryMarker(ctx, folder) - // Cache the result + var cacheable bool + isMarker, cacheable = efc.isDirectoryMarker(ctx, folder) + // Only cache definitive results; transient errors should be retried efc.mu.Lock() - if state, exists := efc.folderCounts[folder]; exists && state != nil { - state.isDirectoryMarker = &isMarker + if cacheable { + if state, exists := efc.folderCounts[folder]; exists && state != nil { + state.isDirectoryMarker = &isMarker + } } } efc.mu.Unlock() @@ -350,9 +353,13 @@ func (efc *EmptyFolderCleaner) executeCleanup(folder string, triggeredBy string) delete(efc.folderCounts, folder) efc.mu.Unlock() - // Note: No need to recursively check parent folder here. - // The deletion of this folder will generate a metadata event, - // which will trigger OnDeleteEvent for the parent folder. + // Directory deletions don't fire NotifyUpdateEvent (filer_delete_entry.go), + // so we must explicitly notify the parent to trigger cascading cleanup. + parentDir := folder[:strings.LastIndex(folder, "/")] + if parentDir != "" && parentDir != folder { + entryName := folder[strings.LastIndex(folder, "/")+1:] + efc.OnDeleteEvent(parentDir, entryName, true, time.Now()) + } } // countItems counts items in a folder (up to maxCountCheck) @@ -367,31 +374,31 @@ func (efc *EmptyFolderCleaner) deleteFolder(ctx context.Context, folder string) // isDirectoryMarker checks if a folder is a directory marker (explicitly created S3 directory key object) // Directory markers have a MIME type set in extended attributes (ExtMimeType). -// Returns true (fail-closed) on transient errors to prevent accidental deletion, -// but returns false for ErrNotFound (entry doesn't exist, e.g. implicit directories). -func (efc *EmptyFolderCleaner) isDirectoryMarker(ctx context.Context, folder string) bool { +// Returns (result, ok) where ok=false means the check failed due to a transient error +// and the result should NOT be cached. +func (efc *EmptyFolderCleaner) isDirectoryMarker(ctx context.Context, folder string) (bool, bool) { attrs, err := efc.filer.GetEntryAttributes(ctx, util.FullPath(folder)) if err != nil { if errors.Is(err, filer_pb.ErrNotFound) { // Entry doesn't exist in filer (implicit directory) — not a directory marker - return false + return false, true } - // Fail-closed on transient errors: assume it's a directory marker - // to prevent accidental deletion due to temporary attribute read failures. - glog.V(2).Infof("EmptyFolderCleaner: error reading attributes for %s, skipping deletion (fail-closed): %v", folder, err) - return true + // Transient error — return false to allow cleanup to proceed, + // but signal that the result should not be cached so it can be retried. + glog.V(2).Infof("EmptyFolderCleaner: error reading attributes for %s, allowing deletion (will retry next cycle): %v", folder, err) + return false, false } if attrs == nil { - return false + return false, true } // Check if MIME type is set in extended attributes (where we store it for directory markers) if _, hasMime := attrs[s3_constants.ExtMimeType]; hasMime { - return true + return true, true } - return false + return false, true } func (efc *EmptyFolderCleaner) getBucketCleanupPolicy(ctx context.Context, folder string) (bucketPath string, autoRemove bool, source string, attrValue string, err error) {