From f68c86add07db5cfae2abb49ae9687e91e9110ba Mon Sep 17 00:00:00 2001 From: Srinivas Chippagiri Date: Wed, 21 Oct 2026 03:48:34 -0700 Subject: [PATCH 1/3] compactor: delete orphaned global block deletion marks When a block no longer exists but its deletion mark is still in the tenant's markers/ location, the bucket index updater skipped the mark at every run and never removed it, so the tenant location stayed in the bucket forever. Delete such a mark when the block location is empty. Fixes #7885 Signed-off-by: Srinivas Chippagiri --- CHANGELOG.md | 2 +- pkg/storage/tsdb/bucketindex/updater.go | 30 ++++++++++++++ pkg/storage/tsdb/bucketindex/updater_test.go | 42 ++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b53819eb496..738e2474a06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ * [BUGFIX] Ruler: Stop retrying staleness markers that can never be accepted. A rule moved between rule groups left the original group re-appending staleness markers for its series on every evaluation until the ruler restarted. #7888 * [BUGFIX] Store Gateway: Fix the Parquet bucket store ignoring query sharding, which made every shard return all series. #7892 * [BUGFIX] Querier: Fix the querier crashing instead of reporting the error when a blocks store set subservice fails. `blocksStoreReplicationSet` never initialised its `FailureWatcher`, so the failure callback dereferenced a nil pointer on the goroutine the service manager runs listeners on, and the resulting panic's stack trace buried the error logged for the actual cause. #7896 - +* [BUGFIX] Compactor: Delete block deletion markers left in the global `markers/` location of a tenant after the block they refer to was removed. They were skipped at every bucket index update and kept the tenant location in the bucket forever. #7885 ## 1.22.0 in progress * [CHANGE] Ruler: Remove the deprecated `-ruler.evaluation-delay-duration` flag and its `ruler_evaluation_delay_duration` per-tenant limit. Use `-ruler.query-offset` / `ruler_query_offset`, which no longer takes the higher of the two values. Cortex decodes the runtime config strictly, so a leftover `ruler_evaluation_delay_duration` override makes the runtime config fail to load: Cortex **exits at startup** (`module failed`, `module=runtime-config`), and on an already-running process every reload fails, pinning the last good overrides and dropping `cortex_runtime_config_last_reload_successful` to 0. Run `grep -r ruler_evaluation_delay_duration` over your runtime configs before upgrading. #7792 diff --git a/pkg/storage/tsdb/bucketindex/updater.go b/pkg/storage/tsdb/bucketindex/updater.go index e917a53fb25..40c56d80f32 100644 --- a/pkg/storage/tsdb/bucketindex/updater.go +++ b/pkg/storage/tsdb/bucketindex/updater.go @@ -254,6 +254,7 @@ func (w *Updater) updateBlockMarks(ctx context.Context, old []*BlockDeletionMark if errors.Is(err, ErrBlockDeletionMarkNotFound) { // This could happen if the block is permanently deleted between the "list objects" and now. level.Warn(w.logger).Log("msg", "skipped missing block deletion mark when updating bucket index", "block", id.String()) + w.deleteOrphanedGlobalDeletionMark(ctx, id) continue } if errors.Is(err, ErrBlockDeletionMarkCorrupted) { @@ -270,6 +271,35 @@ func (w *Updater) updateBlockMarks(ctx context.Context, old []*BlockDeletionMark return out, deletedBlocks, totalBlocksBlocksMarkedForNoCompaction, nil } +// errStopIteration is used to stop a bucket iteration as soon as the first object is found. +var errStopIteration = errors.New("stop iteration") + +// deleteOrphanedGlobalDeletionMark removes the block deletion mark from the global markers location +// when the block it refers to no longer exists in the storage. Without this, the stale marker would +// be listed (and skipped) at every update of the bucket index, and would keep the tenant's location in +// the storage forever. The marker is removed only if the block location is empty, so that a block +// which is being marked for deletion is never affected. This is best effort: failures are only logged. +func (w *Updater) deleteOrphanedGlobalDeletionMark(ctx context.Context, id ulid.ULID) { + hasObjects := false + err := w.bkt.Iter(ctx, id.String()+objstore.DirDelim, func(string) error { + hasObjects = true + return errStopIteration + }, objstore.WithRecursiveIter()) + if err != nil && !errors.Is(err, errStopIteration) { + level.Warn(w.logger).Log("msg", "failed to check whether the block of an orphaned deletion mark exists", "block", id.String(), "err", err) + return + } + if hasObjects { + return + } + + if err := w.bkt.Delete(ctx, BlockDeletionMarkFilepath(id)); err != nil && !w.bkt.IsObjNotFoundErr(err) { + level.Warn(w.logger).Log("msg", "failed to delete orphaned block deletion mark", "block", id.String(), "err", err) + return + } + level.Info(w.logger).Log("msg", "deleted orphaned block deletion mark of a block which no longer exists", "block", id.String()) +} + func (w *Updater) updateBlockDeletionMarkIndexEntry(ctx context.Context, id ulid.ULID) (*BlockDeletionMark, error) { m := metadata.DeletionMark{} diff --git a/pkg/storage/tsdb/bucketindex/updater_test.go b/pkg/storage/tsdb/bucketindex/updater_test.go index 1b1116cf32d..b57423abfa6 100644 --- a/pkg/storage/tsdb/bucketindex/updater_test.go +++ b/pkg/storage/tsdb/bucketindex/updater_test.go @@ -318,6 +318,48 @@ func TestUpdater_UpdateIndex_ShouldSkipBlockMarkedForDeletionWithMissingGlobalMa assert.Empty(t, nonCompactBlocks) } +func TestUpdater_UpdateIndex_ShouldDeleteOrphanedGlobalDeletionMarks(t *testing.T) { + const userID = "user-1" + + bkt, _ := testutil.PrepareFilesystemBucket(t) + + ctx := context.Background() + logger := log.NewNopLogger() + + bkt = BucketWithGlobalMarkers(bkt) + block1 := testutil.MockStorageBlock(t, bkt, userID, 10, 20) + block2 := testutil.MockStorageBlock(t, bkt, userID, 20, 30) + testutil.MockStorageDeletionMark(t, bkt, userID, block2) + + // Orphaned marker: the block does not exist at all. + orphanID := ulid.MustNew(ulid.Now()+1, nil) + mark, err := json.Marshal(metadata.DeletionMark{ID: orphanID, Version: metadata.DeletionMarkVersion1, DeletionTime: time.Now().Add(-24 * time.Hour).Unix()}) + require.NoError(t, err) + orphanPath := path.Join(userID, BlockDeletionMarkFilepath(orphanID)) + require.NoError(t, bkt.Upload(ctx, orphanPath, bytes.NewReader(mark))) + + // Global marker of a block which still has files but no local marker is left untouched. + partialID := ulid.MustNew(ulid.Now()+2, nil) + partialPath := path.Join(userID, BlockDeletionMarkFilepath(partialID)) + require.NoError(t, bkt.Upload(ctx, partialPath, bytes.NewReader(mark))) + require.NoError(t, bkt.Upload(ctx, path.Join(userID, partialID.String(), "index"), strings.NewReader("x"))) + + w := NewUpdater(bkt, userID, nil, logger) + idx, _, _, err := w.UpdateIndex(ctx, nil) + require.NoError(t, err) + assertBucketIndexEqual(t, idx, bkt, userID, + []tsdb.BlockMeta{block1, block2}, + []*metadata.DeletionMark{{ID: block2.ULID, Version: metadata.DeletionMarkVersion1, DeletionTime: idx.BlockDeletionMarks[0].DeletionTime}}) + + exists, err := bkt.Exists(ctx, orphanPath) + require.NoError(t, err) + assert.False(t, exists) + + exists, err = bkt.Exists(ctx, partialPath) + require.NoError(t, err) + assert.True(t, exists) +} + func TestUpdater_UpdateIndex_NoTenantInTheBucket(t *testing.T) { const userID = "user-1" From c030ce65423f2c830e57c67a30c9eb20a9e7db62 Mon Sep 17 00:00:00 2001 From: Srinivas Chippagiri Date: Wed, 21 Oct 2026 19:17:56 -0700 Subject: [PATCH 2/3] Use the PR number in the changelog entry Signed-off-by: Srinivas Chippagiri --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 738e2474a06..8ba051a8fce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ * [BUGFIX] Store Gateway: Fix the Parquet bucket store ignoring query sharding, which made every shard return all series. #7892 * [BUGFIX] Querier: Fix the querier crashing instead of reporting the error when a blocks store set subservice fails. `blocksStoreReplicationSet` never initialised its `FailureWatcher`, so the failure callback dereferenced a nil pointer on the goroutine the service manager runs listeners on, and the resulting panic's stack trace buried the error logged for the actual cause. #7896 * [BUGFIX] Compactor: Delete block deletion markers left in the global `markers/` location of a tenant after the block they refer to was removed. They were skipped at every bucket index update and kept the tenant location in the bucket forever. #7885 +* [BUGFIX] Compactor: Delete block deletion markers left in the global `markers/` location of a tenant after the block they refer to was removed. They were skipped at every bucket index update and kept the tenant location in the bucket forever. #7890 ## 1.22.0 in progress * [CHANGE] Ruler: Remove the deprecated `-ruler.evaluation-delay-duration` flag and its `ruler_evaluation_delay_duration` per-tenant limit. Use `-ruler.query-offset` / `ruler_query_offset`, which no longer takes the higher of the two values. Cortex decodes the runtime config strictly, so a leftover `ruler_evaluation_delay_duration` override makes the runtime config fail to load: Cortex **exits at startup** (`module failed`, `module=runtime-config`), and on an already-running process every reload fails, pinning the last good overrides and dropping `cortex_runtime_config_last_reload_successful` to 0. Run `grep -r ruler_evaluation_delay_duration` over your runtime configs before upgrading. #7792 From e67337a0af1cbe2e78760812a048b14e0eb0a5b0 Mon Sep 17 00:00:00 2001 From: Srinivas Chippagiri Date: Mon, 26 Oct 2026 21:17:00 -0700 Subject: [PATCH 3/3] Let the block listing finish instead of stopping it with an error Returning an error from the Iter callback leaves the S3 listing undrained, and BucketWithRetries and the metrics wrapper treat it as a failed operation. A block location holds only a few objects, so list them all. Signed-off-by: Srinivas Chippagiri --- CHANGELOG.md | 1 - pkg/storage/tsdb/bucketindex/updater.go | 10 +++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ba051a8fce..9daa87d2f2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,6 @@ * [BUGFIX] Ruler: Stop retrying staleness markers that can never be accepted. A rule moved between rule groups left the original group re-appending staleness markers for its series on every evaluation until the ruler restarted. #7888 * [BUGFIX] Store Gateway: Fix the Parquet bucket store ignoring query sharding, which made every shard return all series. #7892 * [BUGFIX] Querier: Fix the querier crashing instead of reporting the error when a blocks store set subservice fails. `blocksStoreReplicationSet` never initialised its `FailureWatcher`, so the failure callback dereferenced a nil pointer on the goroutine the service manager runs listeners on, and the resulting panic's stack trace buried the error logged for the actual cause. #7896 -* [BUGFIX] Compactor: Delete block deletion markers left in the global `markers/` location of a tenant after the block they refer to was removed. They were skipped at every bucket index update and kept the tenant location in the bucket forever. #7885 * [BUGFIX] Compactor: Delete block deletion markers left in the global `markers/` location of a tenant after the block they refer to was removed. They were skipped at every bucket index update and kept the tenant location in the bucket forever. #7890 ## 1.22.0 in progress diff --git a/pkg/storage/tsdb/bucketindex/updater.go b/pkg/storage/tsdb/bucketindex/updater.go index 40c56d80f32..85abd909689 100644 --- a/pkg/storage/tsdb/bucketindex/updater.go +++ b/pkg/storage/tsdb/bucketindex/updater.go @@ -271,21 +271,21 @@ func (w *Updater) updateBlockMarks(ctx context.Context, old []*BlockDeletionMark return out, deletedBlocks, totalBlocksBlocksMarkedForNoCompaction, nil } -// errStopIteration is used to stop a bucket iteration as soon as the first object is found. -var errStopIteration = errors.New("stop iteration") - // deleteOrphanedGlobalDeletionMark removes the block deletion mark from the global markers location // when the block it refers to no longer exists in the storage. Without this, the stale marker would // be listed (and skipped) at every update of the bucket index, and would keep the tenant's location in // the storage forever. The marker is removed only if the block location is empty, so that a block // which is being marked for deletion is never affected. This is best effort: failures are only logged. func (w *Updater) deleteOrphanedGlobalDeletionMark(ctx context.Context, id ulid.ULID) { + // The listing is not stopped at the first object: returning an error from the callback makes some + // clients (e.g. S3) leave the listing undrained, and is counted and retried as a failed operation. + // A block location holds only a handful of objects. hasObjects := false err := w.bkt.Iter(ctx, id.String()+objstore.DirDelim, func(string) error { hasObjects = true - return errStopIteration + return nil }, objstore.WithRecursiveIter()) - if err != nil && !errors.Is(err, errStopIteration) { + if err != nil { level.Warn(w.logger).Log("msg", "failed to check whether the block of an orphaned deletion mark exists", "block", id.String(), "err", err) return }