Release the lock when loading of cache gen number fails. (#3182)

* Release the lock when loading of cache gen number fails.

Signed-off-by: Peter Štibraný <[email protected]>

* Release the lock when loading of cache gen number fails.

Signed-off-by: Peter Štibraný <[email protected]>

* CHANGELOG.md

Signed-off-by: Peter Štibraný <[email protected]>

* Fixed linter

Signed-off-by: Marco Pracucci <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>

Author: pstibrany

CHANGELOG.md

@@ -69,6 +69,7 @@
 * [BUGFIX] Cassandra: fixed consistency setting in the CQL session when creating the keyspace. #3105
 * [BUGFIX] Ruler: Config API would return both the `record` and `alert` in `YAML` response keys even when one of them must be empty. #3120
 * [BUGFIX] Index page now uses configured HTTP path prefix when creating links. #3126
+* [BUGFIX] Purger: fixed deadlock when reloading of tombstones failed. #3182
 * [BUGFIX] Fixed panic in flusher job, when error writing chunks to the store would cause "idle" chunks to be flushed, which triggered panic. #3140
 * [BUGFIX] Index page no longer shows links that are not valid for running Cortex instance. #3133
 * [BUGFIX] Configs: prevent validation of templates to fail when using template functions. #3157

pkg/chunk/purger/tombstones.go

@@ -47,6 +47,12 @@ type TombstonesSet struct {
 	oldestTombstoneStart, newestTombstoneEnd model.Time // Used as optimization to find whether we want to iterate over tombstones or not
 }
 
+// Used for easier injection of mocks.
+type DeleteStoreAPI interface {
+	getCacheGenerationNumbers(ctx context.Context, user string) (*cacheGenNumbers, error)
+	GetPendingDeleteRequestsForUser(ctx context.Context, id string) ([]DeleteRequest, error)
+}
+
 // TombstonesLoader loads delete requests and gen numbers from store and keeps checking for updates.
 // It keeps checking for changes in gen numbers, which also means changes in delete requests and reloads specific users delete requests.
 type TombstonesLoader struct {
@@ -56,13 +62,13 @@ type TombstonesLoader struct {
 	cacheGenNumbers    map[string]*cacheGenNumbers
 	cacheGenNumbersMtx sync.RWMutex
 
-	deleteStore *DeleteStore
+	deleteStore DeleteStoreAPI
 	metrics     *tombstonesLoaderMetrics
 	quit        chan struct{}
 }
 
 // NewTombstonesLoader creates a TombstonesLoader
-func NewTombstonesLoader(deleteStore *DeleteStore, registerer prometheus.Registerer) *TombstonesLoader {
+func NewTombstonesLoader(deleteStore DeleteStoreAPI, registerer prometheus.Registerer) *TombstonesLoader {
 	tl := TombstonesLoader{
 		tombstones:      map[string]*TombstonesSet{},
 		cacheGenNumbers: map[string]*cacheGenNumbers{},
@@ -106,6 +112,7 @@ func (tl *TombstonesLoader) reloadTombstones() error {
 	for userID, oldGenNumbers := range tl.cacheGenNumbers {
 		newGenNumbers, err := tl.deleteStore.getCacheGenerationNumbers(context.Background(), userID)
 		if err != nil {
+			tl.cacheGenNumbersMtx.RUnlock()
 			return err
 		}
 

pkg/chunk/purger/tombstones_test.go

@@ -2,6 +2,7 @@ package purger
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -131,3 +132,27 @@ func TestTombstonesLoader(t *testing.T) {
 		})
 	}
 }
+
+func TestTombstonesReloadDoesntDeadlockOnFailure(t *testing.T) {
+	s := &store{}
+	tombstonesLoader := NewTombstonesLoader(s, nil)
+	tombstonesLoader.getCacheGenNumbers("test")
+
+	s.err = errors.New("error")
+	require.NotNil(t, tombstonesLoader.reloadTombstones())
+
+	s.err = nil
+	require.NotNil(t, tombstonesLoader.getCacheGenNumbers("test2"))
+}
+
+type store struct {
+	err error
+}
+
+func (f *store) getCacheGenerationNumbers(ctx context.Context, user string) (*cacheGenNumbers, error) {
+	return &cacheGenNumbers{}, f.err
+}
+
+func (f *store) GetPendingDeleteRequestsForUser(ctx context.Context, id string) ([]DeleteRequest, error) {
+	return nil, nil
+}