From 079a7d8ba32ac20bfaffcfcf3394eb9bbeb1c2a8 Mon Sep 17 00:00:00 2001 From: Joel Town Road <13539685+hippi345@users.noreply.github.com> Date: Wed, 7 Oct 2026 08:53:39 -0400 Subject: [PATCH] s3: give each prefix its own hidden-prefix probe budget (#11626) * s3: give each prefix its own hidden-prefix probe budget The shared cursor.probedEntries counter in dirHoldsOnlyHiddenEntries was exhausted by one large all-deleted subtree, causing every later prefix in the same request to be treated as visible. The fix allocates a fresh budget (hiddenProbePerPrefixBudget, default 1000) for each top-level call and threads it down to recursive calls via a pointer, so cross-prefix budget bleed is impossible. Fixes #10847. * s3: keep 10000 probe budget, now per prefix Restore hiddenProbeBudget = 10000 as a package const (not a mutable var) applied per-prefix instead of per-request. The override hook moves to an unexported probeBudget field on ListingCursor; zero means use the package default. Tests set probeBudget: 3 on the cursor, keeping them fast without touching package state. Also revert the unrelated uint32 cast on the ListEntries Limit field. * s3: cap total hidden-prefix probe work per listing request The per-prefix budget resets for every candidate prefix, but deleted prefixes do not spend maxKeys, so a page can walk an unbounded number of them. Keep a request-wide probedEntries ceiling (hiddenProbeTotalBudget, 10x the per-prefix budget) so total probe work stays bounded. * s3api: pin the probe-budget cutoff and the request-wide ceiling --------- Co-authored-by: Chris Lu --- weed/s3api/s3api_object_handlers_list.go | 45 ++++++++--- ...bject_handlers_list_deleted_prefix_test.go | 79 +++++++++++++++++++ 2 files changed, 115 insertions(+), 9 deletions(-) diff --git a/weed/s3api/s3api_object_handlers_list.go b/weed/s3api/s3api_object_handlers_list.go index 991d11f7f..0af321bc5 100644 --- a/weed/s3api/s3api_object_handlers_list.go +++ b/weed/s3api/s3api_object_handlers_list.go @@ -780,6 +780,12 @@ type ListingCursor struct { // something to find once a bucket has version history to leave behind. hideDeletedPrefixes bool probedEntries int + // probeBudget overrides hiddenProbeBudget for the per-prefix probe; 0 means use + // the package default. Tests set this to a small value to stay fast. + probeBudget int + // probeTotalBudget overrides hiddenProbeTotalBudget for the request-wide probe + // ceiling; 0 means use the package default. + probeTotalBudget int // retractEntry undoes the listing of a base-path null object once its .versions // sibling reveals that the current version is a delete marker. retractEntry func(dir, name string) @@ -1161,12 +1167,16 @@ func (s3a *S3ApiServer) doListFilerEntries(ctx context.Context, client filer_pb. } } -// hiddenProbePageSize is the window one probe request asks the filer for, and -// hiddenProbeBudget caps how many entries a single list request may look at while -// deciding which directories still stand for a prefix. +// hiddenProbePageSize is the window one probe request asks the filer for. +// hiddenProbeBudget caps how many entries one prefix's probe may scan; it +// resets for every candidate prefix so one large all-deleted subtree cannot +// exhaust the allowance for later prefixes. hiddenProbeTotalBudget caps the +// scanned entries a single listing request spends on probing overall, since a +// page may still walk an unbounded number of all-deleted prefixes. const ( - hiddenProbePageSize = 64 - hiddenProbeBudget = 10000 + hiddenProbePageSize = 64 + hiddenProbeBudget = 10000 + hiddenProbeTotalBudget = 100000 ) // dirHoldsOnlyHiddenEntries reports whether dir holds entries but none that a @@ -1179,13 +1189,29 @@ const ( // // The scan stops at the first key it finds, so a populated prefix costs one ListEntries // answered by its first entry. A subtree that is entirely delete-marked costs a walk of -// that subtree, bounded by the request's probe budget; once the budget is spent the -// prefix is reported, as it was before this check existed. +// that subtree, bounded by hiddenProbeBudget entries per prefix. The budget resets for +// each candidate prefix, so a large all-deleted subtree cannot exhaust the allowance +// for later prefixes in the same request. func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntries(ctx context.Context, client filer_pb.SeaweedFilerClient, bucket, dir string, cursor *ListingCursor) bool { if !cursor.hideDeletedPrefixes { return false } + budget := hiddenProbeBudget + if cursor.probeBudget > 0 { + budget = cursor.probeBudget + } + totalBudget := hiddenProbeTotalBudget + if cursor.probeTotalBudget > 0 { + totalBudget = cursor.probeTotalBudget + } + return s3a.dirHoldsOnlyHiddenEntriesInner(ctx, client, bucket, dir, &budget, totalBudget, cursor) +} +// dirHoldsOnlyHiddenEntriesInner is the recursive body of dirHoldsOnlyHiddenEntries. +// budget is shared across the recursive descent for one prefix but reset by the +// caller for each top-level prefix, preventing cross-prefix budget exhaustion. +// cursor.probedEntries bounds the total work one listing request spends probing. +func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntriesInner(ctx context.Context, client filer_pb.SeaweedFilerClient, bucket, dir string, budget *int, totalBudget int, cursor *ListingCursor) bool { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -1227,8 +1253,9 @@ func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntries(ctx context.Context, client fi startFrom = entry.Name sawEntry = true + *budget-- cursor.probedEntries++ - if cursor.probedEntries > hiddenProbeBudget { + if *budget < 0 || cursor.probedEntries > totalBudget { return false } @@ -1269,7 +1296,7 @@ func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntries(ctx context.Context, client fi if entry.IsDirectoryKeyObject() { return false } - if !s3a.dirHoldsOnlyHiddenEntries(ctx, client, bucket, dir+"/"+entry.Name, cursor) { + if !s3a.dirHoldsOnlyHiddenEntriesInner(ctx, client, bucket, dir+"/"+entry.Name, budget, totalBudget, cursor) { return false } } diff --git a/weed/s3api/s3api_object_handlers_list_deleted_prefix_test.go b/weed/s3api/s3api_object_handlers_list_deleted_prefix_test.go index e7ca1345f..1a635f21c 100644 --- a/weed/s3api/s3api_object_handlers_list_deleted_prefix_test.go +++ b/weed/s3api/s3api_object_handlers_list_deleted_prefix_test.go @@ -250,3 +250,82 @@ func TestDeletedPrefixesDoNotConsumeMaxKeys(t *testing.T) { assert.False(t, cursor.isTruncated, "stepping over deleted prefixes must not truncate the page") assert.Zero(t, cursor.maxKeys, "only the live prefixes may spend the page budget") } + +// TestPerPrefixBudgetNotShared is a regression test for the bug reported in issue #10847: +// one large all-deleted prefix exhausts the shared probe budget, causing every later +// prefix to be reported as visible even when all its objects are delete-marked. +// +// The fix gives each candidate prefix its own probe budget (hiddenProbeBudget, 10000 by +// default). The test sets cursor.probeBudget = 3 to stay fast without touching the +// package-level constant. +func TestPerPrefixBudgetNotShared(t *testing.T) { + // Build 5 delete-marked objects under "bigdeleted/" – more than the budget of 3. + bigDeletedEntries := make([]*filer_pb.Entry, 0, 5) + for i := 0; i < 5; i++ { + bigDeletedEntries = append(bigDeletedEntries, deleteMarkedVersionsDir( + "obj"+strconv.Itoa(i), + )) + } + + client := &testFilerClient{ + entriesByDir: map[string][]*filer_pb.Entry{ + "/buckets/test": { + newDir("bigdeleted"), + newDir("emptydeleted"), + newDir("live"), + }, + "/buckets/test/bigdeleted": bigDeletedEntries, + "/buckets/test/emptydeleted": {deleteMarkedVersionsDir("obj")}, + "/buckets/test/live": {liveVersionsDir("obj")}, + }, + } + + // probeBudget: 3 simulates the bug scenario with a tiny dataset; real default is + // hiddenProbeBudget (10000). + seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", delimiter: "/", bucket: "test"}, + &ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true, probeBudget: 3}) + + // "bigdeleted" exhausted its own probe budget, so it is reported rather than + // proven all-deleted. + assert.Contains(t, seen, "bigdeleted", "a prefix whose probe budget ran out must be reported") + + // "emptydeleted" must be hidden: all its objects are delete-marked and its probe + // budget was not exhausted by "bigdeleted". + assert.NotContains(t, seen, "emptydeleted", "all-deleted prefix after a budget-exhausting prefix must still be hidden") + + // "live" must always appear. + assert.Contains(t, seen, "live", "prefix with a live current version must be listed") +} + +// TestPerRequestProbeBudgetStillBoundsWork covers the request-wide ceiling: +// deleted prefixes do not spend maxKeys, so a page can walk an unbounded +// number of them. Once probedEntries exceeds the request total, a later +// all-deleted prefix is reported rather than proven. +func TestPerRequestProbeBudgetStillBoundsWork(t *testing.T) { + deleted := func(n int) []*filer_pb.Entry { + entries := make([]*filer_pb.Entry, 0, n) + for i := 0; i < n; i++ { + entries = append(entries, deleteMarkedVersionsDir("obj"+strconv.Itoa(i))) + } + return entries + } + + client := &testFilerClient{ + entriesByDir: map[string][]*filer_pb.Entry{ + "/buckets/test": { + newDir("d1"), + newDir("d2"), + }, + "/buckets/test/d1": deleted(4), + "/buckets/test/d2": deleted(4), + }, + } + + // Per-prefix budget (10) can cover either prefix alone, but the request + // total (5) is spent probing "d1", so "d2" is reported without a full scan. + seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", delimiter: "/", bucket: "test"}, + &ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true, probeBudget: 10, probeTotalBudget: 5}) + + assert.NotContains(t, seen, "d1", "a fully probed all-deleted prefix stays hidden") + assert.Contains(t, seen, "d2", "a prefix probed past the request total is reported") +}