GC: report errors, but remove erroneous silences and continue (#4724)

Signed-off-by: Guido Trotter <guido@hudson-trading.com>
Co-authored-by: Guido Trotter <guido@hudson-trading.com>

Author: ultrotter

silence/silence.go

@@ -278,6 +278,7 @@ type MaintenanceFunc func() (int64, error)
 
 type metrics struct {
 	gcDuration                            prometheus.Summary
+	gcErrorsTotal                         prometheus.Counter
 	snapshotDuration                      prometheus.Summary
 	snapshotSize                          prometheus.Gauge
 	queriesTotal                          prometheus.Counter
@@ -323,6 +324,10 @@ func newMetrics(r prometheus.Registerer, s *Silences) *metrics {
 		Help:       "Duration of the last silence garbage collection cycle.",
 		Objectives: map[float64]float64{},
 	})
+	m.gcErrorsTotal = promauto.With(r).NewCounter(prometheus.CounterOpts{
+		Name: "alertmanager_silences_gc_errors_total",
+		Help: "How many silence GC errors were encountered.",
+	})
 	m.snapshotDuration = promauto.With(r).NewSummary(prometheus.SummaryOpts{
 		Name:       "alertmanager_silences_snapshot_duration_seconds",
 		Help:       "Duration of the last silence snapshot.",
@@ -540,8 +545,7 @@ Loop:
 			break Loop
 		case <-t.C:
 			if err := runMaintenance(doMaintenance); err != nil {
-				// @tjhop: this should probably log at error level
-				s.logger.Info("Running maintenance failed", "err", err)
+				s.logger.Error("Running maintenance failed", "err", err)
 			}
 		}
 	}
@@ -564,6 +568,7 @@ func (s *Silences) GC() (int, error) {
 
 	now := s.nowUTC()
 	var n int
+	var errs error
 
 	s.mtx.Lock()
 	defer s.mtx.Unlock()
@@ -586,15 +591,21 @@ func (s *Silences) GC() (int, error) {
 	// Iterate state map directly (fast - no extra lookups).
 	for _, sv := range s.vi {
 		sil, ok := s.st[sv.id]
-		// FIXME: in both these cases rather than breaking GC forever
-		// we should increase an error metric, log, and drop the culprit silence.
+		expire := false
 		if !ok {
-			return n, errors.New("silence in index missing from state")
+			// Silence in version index but not in state - remove from version index and count error
+			s.metrics.gcErrorsTotal.Inc()
+			errs = errors.Join(errs, fmt.Errorf("silence %s in version index missing from state", sv.id))
+			// not adding to targetVi effectively removes it
+			continue
 		}
 		if sil.ExpiresAt.IsZero() {
-			return n, errors.New("unexpected zero expiration timestamp")
+			// Invalid expiration timestamp - remove silence and count error
+			s.metrics.gcErrorsTotal.Inc()
+			errs = errors.Join(errs, fmt.Errorf("silence %s has zero expiration timestamp", sil.Silence.Id))
+			expire = true
 		}
-		if !sil.ExpiresAt.After(now) {
+		if expire || !sil.ExpiresAt.After(now) {
 			delete(s.st, sil.Silence.Id)
 			delete(s.mi, sil.Silence.Id)
 			n++
@@ -610,7 +621,7 @@ func (s *Silences) GC() (int, error) {
 	s.vi = targetVi
 	s.updateSizeMetrics()
 
-	return n, nil
+	return n, errs
 }
 
 func validateMatcher(m *pb.Matcher) error {

silence/silence_test.go

@@ -332,6 +332,70 @@ func TestSilenceGCOverTime(t *testing.T) {
 			require.Contains(t, id, "active-")
 		}
 	})
+
+	t.Run("GC continues and removes erroneous silences", func(t *testing.T) {
+		reg := prometheus.NewRegistry()
+		s, err := New(Options{Metrics: reg})
+		require.NoError(t, err)
+		clock := quartz.NewMock(t)
+		s.clock = clock
+		now := clock.Now()
+
+		// Create a valid silence
+		validSil := &pb.Silence{
+			Matchers: []*pb.Matcher{{
+				Type:    pb.Matcher_EQUAL,
+				Name:    "foo",
+				Pattern: "bar",
+			}},
+			StartsAt: now,
+			EndsAt:   now.Add(time.Minute),
+		}
+		require.NoError(t, s.Set(validSil))
+		validID := validSil.Id
+
+		// Manually add an erroneous silence with zero expiration
+		erroneousSil := &pb.MeshSilence{
+			Silence: &pb.Silence{
+				Id: "erroneous",
+				Matchers: []*pb.Matcher{{
+					Type:    pb.Matcher_EQUAL,
+					Name:    "bar",
+					Pattern: "baz",
+				}},
+				StartsAt: now,
+				EndsAt:   now.Add(time.Minute),
+			},
+			ExpiresAt: time.Time{}, // Zero expiration - invalid
+		}
+		s.st["erroneous"] = erroneousSil
+		s.vi.add(s.version+1, "erroneous")
+		s.version++
+
+		// Manually add an entry to version index that doesn't exist in state
+		s.vi.add(s.version+1, "missing")
+		s.version++
+
+		require.Len(t, s.st, 2)
+		require.Len(t, s.vi, 3)
+
+		// Run GC - should continue despite errors
+		n, err := s.GC()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "zero expiration timestamp")
+		require.Contains(t, err.Error(), "missing from state")
+
+		// GC should have removed erroneous silences
+		require.Equal(t, 1, n) // Only the erroneous silence with zero expiration
+		require.Len(t, s.st, 1)
+		require.Len(t, s.vi, 1)
+		require.Contains(t, s.st, validID)
+		require.NotContains(t, s.st, "erroneous")
+
+		// Check that the error metric was incremented
+		metricValue := testutil.ToFloat64(s.metrics.gcErrorsTotal)
+		require.Equal(t, float64(2), metricValue)
+	})
 }
 
 func TestSilencesSnapshot(t *testing.T) {