From b07052842460f0f6c28437a716555743c5f4432b Mon Sep 17 00:00:00 2001 From: Justin Jung Date: Fri, 4 Jul 2025 10:04:08 -0700 Subject: [PATCH 1/4] Make scanUser always return unique user IDs to avoid concurrent map write Signed-off-by: Justin Jung --- pkg/storegateway/bucket_stores.go | 18 +++++++++ pkg/storegateway/bucket_stores_test.go | 52 ++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/pkg/storegateway/bucket_stores.go b/pkg/storegateway/bucket_stores.go index f49422f3bd..0bcf4aca8c 100644 --- a/pkg/storegateway/bucket_stores.go +++ b/pkg/storegateway/bucket_stores.go @@ -459,9 +459,27 @@ func (u *BucketStores) scanUsers(ctx context.Context) ([]string, error) { users = append(users, activeUsers...) users = append(users, deletingUsers...) + if err = checkDuplicateUsers(users); err != nil { + return nil, err + } + return users, err } +func checkDuplicateUsers(users []string) error { + seen := make(map[string]struct{}, len(users)) + + for _, user := range users { + if _, ok := seen[user]; ok { + return fmt.Errorf("duplicate user scanned: %s", user) + } else { + seen[user] = struct{}{} + } + } + + return nil +} + func (u *BucketStores) getStore(userID string) *store.BucketStore { u.storesMu.RLock() defer u.storesMu.RUnlock() diff --git a/pkg/storegateway/bucket_stores_test.go b/pkg/storegateway/bucket_stores_test.go index 5b56408dba..0be7cc4124 100644 --- a/pkg/storegateway/bucket_stores_test.go +++ b/pkg/storegateway/bucket_stores_test.go @@ -454,6 +454,50 @@ func TestBucketStores_syncUsersBlocks(t *testing.T) { } } +func TestBucketStores_scanUsers(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + scanner *mockScanner + expectedRes []string + expectedErr error + }{ + "should succeed": { + scanner: &mockScanner{ + res: []string{"user-1", "user-2", "user-3"}, + }, + expectedRes: []string{"user-1", "user-2", "user-3"}, + }, + "should return error if duplicate users are returned": { + scanner: &mockScanner{ + res: []string{"user-1", "user-2", "user-1"}, + }, + expectedErr: fmt.Errorf("duplicate user scanned: user-1"), + }, + } + + for testName, testData := range tests { + testData := testData + t.Run(testName, func(t *testing.T) { + t.Parallel() + + stores := &BucketStores{ + userScanner: testData.scanner, + } + + users, err := stores.scanUsers(context.Background()) + + if testData.expectedErr != nil { + assert.ErrorContains(t, err, testData.expectedErr.Error()) + assert.Empty(t, users) + } else { + assert.NoError(t, err) + assert.ElementsMatch(t, testData.expectedRes, users) + } + }) + } +} + func TestBucketStores_Series_ShouldCorrectlyQuerySeriesSpanningMultipleChunks(t *testing.T) { for _, lazyLoadingEnabled := range []bool{true, false} { t.Run(fmt.Sprintf("lazy loading enabled = %v", lazyLoadingEnabled), func(t *testing.T) { @@ -996,3 +1040,11 @@ func (f *failFirstGetBucket) Get(ctx context.Context, name string) (io.ReadClose return f.Bucket.Get(ctx, name) } + +type mockScanner struct { + res []string +} + +func (m *mockScanner) ScanUsers(_ context.Context) (active, deleting, deleted []string, err error) { + return m.res, nil, nil, nil +} From 1171774b710e06f399df2b688159169a41d38840 Mon Sep 17 00:00:00 2001 From: Justin Jung Date: Fri, 4 Jul 2025 10:11:46 -0700 Subject: [PATCH 2/4] Changelog Signed-off-by: Justin Jung --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cfcf56943..df17386449 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ * [BUGFIX] Ruler: Prevent counting 2xx and 4XX responses as failed writes. #6785 * [BUGFIX] Ingester: Allow shipper to skip corrupted blocks. #6786 * [BUGFIX] Compactor: Delete the prefix `blocks_meta` from the metadata fetcher metrics. #6832 +* [BUGFIX] Store Gateway: Avoid race condition by checking for duplicate entries in bucket stores user scan. #6863 ## 1.19.0 2025-02-27 From 1125007f5a1d4050717577e249ed919c7c287a6b Mon Sep 17 00:00:00 2001 From: Justin Jung Date: Thu, 10 Jul 2025 15:37:13 -0700 Subject: [PATCH 3/4] Instead of returning error, just deduplicate Signed-off-by: Justin Jung --- pkg/storegateway/bucket_stores.go | 15 ++++++--------- pkg/storegateway/bucket_stores_test.go | 20 ++++---------------- 2 files changed, 10 insertions(+), 25 deletions(-) diff --git a/pkg/storegateway/bucket_stores.go b/pkg/storegateway/bucket_stores.go index 0bcf4aca8c..b9da057ae2 100644 --- a/pkg/storegateway/bucket_stores.go +++ b/pkg/storegateway/bucket_stores.go @@ -458,26 +458,23 @@ func (u *BucketStores) scanUsers(ctx context.Context) ([]string, error) { users := make([]string, 0, len(activeUsers)+len(deletingUsers)) users = append(users, activeUsers...) users = append(users, deletingUsers...) - - if err = checkDuplicateUsers(users); err != nil { - return nil, err - } + users = deduplicateUsers(users) return users, err } -func checkDuplicateUsers(users []string) error { +func deduplicateUsers(users []string) []string { seen := make(map[string]struct{}, len(users)) + var uniqueUsers []string for _, user := range users { - if _, ok := seen[user]; ok { - return fmt.Errorf("duplicate user scanned: %s", user) - } else { + if _, ok := seen[user]; !ok { seen[user] = struct{}{} + uniqueUsers = append(uniqueUsers, user) } } - return nil + return uniqueUsers } func (u *BucketStores) getStore(userID string) *store.BucketStore { diff --git a/pkg/storegateway/bucket_stores_test.go b/pkg/storegateway/bucket_stores_test.go index 0be7cc4124..69c018ccfa 100644 --- a/pkg/storegateway/bucket_stores_test.go +++ b/pkg/storegateway/bucket_stores_test.go @@ -460,19 +460,12 @@ func TestBucketStores_scanUsers(t *testing.T) { tests := map[string]struct { scanner *mockScanner expectedRes []string - expectedErr error }{ - "should succeed": { - scanner: &mockScanner{ - res: []string{"user-1", "user-2", "user-3"}, - }, - expectedRes: []string{"user-1", "user-2", "user-3"}, - }, - "should return error if duplicate users are returned": { + "should return unique users only": { scanner: &mockScanner{ res: []string{"user-1", "user-2", "user-1"}, }, - expectedErr: fmt.Errorf("duplicate user scanned: user-1"), + expectedRes: []string{"user-1", "user-2"}, }, } @@ -487,13 +480,8 @@ func TestBucketStores_scanUsers(t *testing.T) { users, err := stores.scanUsers(context.Background()) - if testData.expectedErr != nil { - assert.ErrorContains(t, err, testData.expectedErr.Error()) - assert.Empty(t, users) - } else { - assert.NoError(t, err) - assert.ElementsMatch(t, testData.expectedRes, users) - } + assert.NoError(t, err) + assert.ElementsMatch(t, testData.expectedRes, users) }) } } From 0ee1df80abc505ad32dc0ecd43c51d76d1d83802 Mon Sep 17 00:00:00 2001 From: Justin Jung Date: Thu, 10 Jul 2025 15:39:01 -0700 Subject: [PATCH 4/4] changelog Signed-off-by: Justin Jung --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df17386449..d8b0c27c1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,7 +70,7 @@ * [BUGFIX] Ruler: Prevent counting 2xx and 4XX responses as failed writes. #6785 * [BUGFIX] Ingester: Allow shipper to skip corrupted blocks. #6786 * [BUGFIX] Compactor: Delete the prefix `blocks_meta` from the metadata fetcher metrics. #6832 -* [BUGFIX] Store Gateway: Avoid race condition by checking for duplicate entries in bucket stores user scan. #6863 +* [BUGFIX] Store Gateway: Avoid race condition by deduplicating entries in bucket stores user scan. #6863 ## 1.19.0 2025-02-27