[release-12.3.2] Alerting: Fix a race condition panic in ResetStateByRuleUID (#115680)

Alerting: Fix a race condition panic in ResetStateByRuleUID (#115662)

(cherry picked from commit 096208202e)

Co-authored-by: Alexander Akhmetov <me@alx.cx>

pkg/services/ngalert/schedule/registry.go

@@ -101,13 +101,13 @@ func (e *Evaluation) Fingerprint() fingerprint {
 type alertRulesRegistry struct {
 	rules        map[models.AlertRuleKey]*models.AlertRule
 	folderTitles map[models.FolderKey]string
-	mu           sync.Mutex
+	mu           sync.RWMutex
 }
 
 // all returns all rules in the registry.
 func (r *alertRulesRegistry) all() ([]*models.AlertRule, map[models.FolderKey]string) {
-	r.mu.Lock()
-	defer r.mu.Unlock()
+	r.mu.RLock()
+	defer r.mu.RUnlock()
 	result := make([]*models.AlertRule, 0, len(r.rules))
 	for _, rule := range r.rules {
 		result = append(result, rule)
@@ -116,8 +116,8 @@ func (r *alertRulesRegistry) all() ([]*models.AlertRule, map[models.FolderKey]st
 }
 
 func (r *alertRulesRegistry) get(k models.AlertRuleKey) *models.AlertRule {
-	r.mu.Lock()
-	defer r.mu.Unlock()
+	r.mu.RLock()
+	defer r.mu.RUnlock()
 	return r.rules[k]
 }
 
@@ -157,12 +157,14 @@ func (r *alertRulesRegistry) del(k models.AlertRuleKey) (*models.AlertRule, bool
 }
 
 func (r *alertRulesRegistry) isEmpty() bool {
-	r.mu.Lock()
-	defer r.mu.Unlock()
+	r.mu.RLock()
+	defer r.mu.RUnlock()
 	return len(r.rules) == 0
 }
 
 func (r *alertRulesRegistry) needsUpdate(keys []models.AlertRuleKeyWithVersion) bool {
+	r.mu.RLock()
+	defer r.mu.RUnlock()
 	if len(r.rules) != len(keys) {
 		return true
 	}

pkg/services/ngalert/state/manager.go

@@ -229,9 +229,7 @@ func (st *Manager) Get(orgID int64, alertRuleUID string, stateId data.Fingerprin
 	return st.cache.get(orgID, alertRuleUID, stateId)
 }
 
-// DeleteStateByRuleUID removes the rule instances from cache and instanceStore. A closed channel is returned to be able
-// to gracefully handle the clear state step in scheduler in case we do not need to use the historian to save state
-// history.
+// DeleteStateByRuleUID removes the rule instances from cache and instanceStore.
 func (st *Manager) DeleteStateByRuleUID(ctx context.Context, ruleKey ngModels.AlertRuleKeyWithGroup, reason string) []StateTransition {
 	logger := st.log.FromContext(ctx)
 	logger.Debug("Resetting state of the rule")
@@ -289,10 +287,14 @@ func (st *Manager) ForgetStateByRuleUID(ctx context.Context, ruleKey ngModels.Al
 // ResetStateByRuleUID removes the rule instances from cache and instanceStore and saves state history. If the state
 // history has to be saved, rule must not be nil.
 func (st *Manager) ResetStateByRuleUID(ctx context.Context, rule *ngModels.AlertRule, reason string) []StateTransition {
+	if rule == nil {
+		return nil
+	}
+
 	ruleKey := rule.GetKeyWithGroup()
 	transitions := st.DeleteStateByRuleUID(ctx, ruleKey, reason)
 
-	if rule == nil || st.historian == nil || len(transitions) == 0 {
+	if st.historian == nil || len(transitions) == 0 {
 		return transitions
 	}
 

pkg/services/ngalert/state/manager_test.go

@@ -2044,6 +2044,39 @@ func TestIntegrationResetStateByRuleUID(t *testing.T) {
 	}
 }
 
+func TestResetStateByRuleUID(t *testing.T) {
+	ctx := context.Background()
+
+	setupManager := func(historian state.Historian) *state.Manager {
+		cfg := state.ManagerCfg{
+			Metrics:       metrics.NewNGAlert(prometheus.NewPedanticRegistry()).GetStateMetrics(),
+			ExternalURL:   nil,
+			InstanceStore: &state.FakeInstanceStore{},
+			Images:        &state.NoopImageService{},
+			Clock:         clock.NewMock(),
+			Historian:     historian,
+			Tracer:        tracing.InitializeTracerForTest(),
+			Log:           log.New("ngalert.state.manager"),
+		}
+
+		return state.NewManager(cfg, state.NewNoopPersister())
+	}
+
+	t.Run("with nil historian", func(t *testing.T) {
+		manager := setupManager(nil)
+
+		transitions := manager.ResetStateByRuleUID(ctx, nil, "test reason")
+		require.Empty(t, transitions)
+	})
+
+	t.Run("with historian", func(t *testing.T) {
+		manager := setupManager(&state.FakeHistorian{})
+
+		transitions := manager.ResetStateByRuleUID(ctx, nil, "test reason")
+		require.Empty(t, transitions)
+	})
+}
+
 func setCacheID(s *state.State) *state.State {
 	if s.CacheID != 0 {
 		return s