fix leaking of matcher cache entries

silence/silence.go

@@ -49,13 +49,13 @@ var ErrNotFound = errors.New("silence not found")
 // ErrInvalidState is returned if the state isn't valid.
 var ErrInvalidState = errors.New("invalid state")
 
-type matcherCache map[*pb.Silence]labels.Matchers
+type matcherCache map[string]labels.Matchers
 
 // Get retrieves the matchers for a given silence. If it is a missed cache
 // access, it compiles and adds the matchers of the requested silence to the
 // cache.
 func (c matcherCache) Get(s *pb.Silence) (labels.Matchers, error) {
-	if m, ok := c[s]; ok {
+	if m, ok := c[s.Id]; ok {
 		return m, nil
 	}
 	return c.add(s)
@@ -88,7 +88,7 @@ func (c matcherCache) add(s *pb.Silence) (labels.Matchers, error) {
 		ms[i] = matcher
 	}
 
-	c[s] = ms
+	c[s.Id] = ms
 	return ms, nil
 }
 
@@ -478,7 +478,7 @@ func (s *Silences) GC() (int, error) {
 		}
 		if !sil.ExpiresAt.After(now) {
 			delete(s.st, id)
-			delete(s.mc, sil.Silence)
+			delete(s.mc, sil.Silence.Id)
 			n++
 		}
 	}

silence/silence_test.go

@@ -91,16 +91,16 @@ func TestSilencesGC(t *testing.T) {
 	s.clock = clock.NewMock()
 	now := s.nowUTC()
 
-	newSilence := func(exp time.Time) *pb.MeshSilence {
-		return &pb.MeshSilence{ExpiresAt: exp}
+	newSilence := func(id string, exp time.Time) *pb.MeshSilence {
+		return &pb.MeshSilence{Silence: &pb.Silence{Id: id}, ExpiresAt: exp}
 	}
 	s.st = state{
-		"1": newSilence(now),
-		"2": newSilence(now.Add(-time.Second)),
-		"3": newSilence(now.Add(time.Second)),
+		"1": newSilence("1", now),
+		"2": newSilence("2", now.Add(-time.Second)),
+		"3": newSilence("3", now.Add(time.Second)),
 	}
 	want := state{
-		"3": newSilence(now.Add(time.Second)),
+		"3": newSilence("3", now.Add(time.Second)),
 	}
 
 	n, err := s.GC()
@@ -109,6 +109,119 @@ func TestSilencesGC(t *testing.T) {
 	require.Equal(t, want, s.st)
 }
 
+func TestSilenceGCOverTime(t *testing.T) {
+	type silenceEntry struct {
+		s                    *pb.Silence
+		expectPresentAfterGc bool
+	}
+	type testCase struct {
+		initialState    []silenceEntry
+		updates         []silenceEntry
+		expectedGCCount int
+	}
+
+	c := clock.NewMock()
+	now := c.Now().UTC()
+
+	newSilence := func(id string, exp time.Time) *pb.Silence {
+		return &pb.Silence{
+			Id:       id,
+			StartsAt: now.Add(-time.Second),
+			EndsAt:   exp,
+			Matchers: []*pb.Matcher{
+				{Name: "foo", Type: pb.Matcher_REGEXP, Pattern: "bar"},
+			}}
+	}
+
+	cases := map[string]testCase{
+		"gc does not clean active silences": {
+			initialState: []silenceEntry{
+				{s: newSilence("1", now), expectPresentAfterGc: false},
+				{s: newSilence("2", now.Add(-time.Second)), expectPresentAfterGc: false},
+				{s: newSilence("3", now.Add(time.Second)), expectPresentAfterGc: true},
+			},
+		},
+		"silences added with Set are handled correctly": {
+			initialState: []silenceEntry{
+				{s: newSilence("1", now), expectPresentAfterGc: false},
+			},
+			updates: []silenceEntry{
+				{s: newSilence("", now.Add(time.Second)), expectPresentAfterGc: true},
+				{s: newSilence("", now.Add(-time.Second)), expectPresentAfterGc: false},
+			},
+		},
+		"silence update does not leak state": {
+			initialState: []silenceEntry{
+				{s: newSilence("1", now), expectPresentAfterGc: false},
+			},
+			updates: []silenceEntry{
+				{s: newSilence("1", now.Add(time.Second)), expectPresentAfterGc: true},
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			silences, err := New(Options{})
+			silClock := clock.NewMock()
+			silences.clock = silClock
+			require.NoError(t, err)
+
+			expectedRemaining := []string{}
+			expectedGCCount := 0
+			for _, sil := range tc.initialState {
+				silences.st[sil.s.Id] = &pb.MeshSilence{
+					Silence:   sil.s,
+					ExpiresAt: sil.s.EndsAt,
+				}
+				if sil.expectPresentAfterGc {
+					expectedRemaining = append(expectedRemaining, sil.s.Id)
+				} else {
+					expectedGCCount += 1
+				}
+				// simulate this silences being seen in a query
+				silences.mc.Get(silences.st[sil.s.Id].Silence)
+			}
+			silClock.Add(-time.Second)
+			for _, sil := range tc.updates {
+				if sil.s.Id != "" {
+					// we're replacing a silence which now will not get GC'd
+					expectedGCCount -= 1
+				}
+				err := silences.Set(sil.s)
+				require.NoError(t, err)
+				if sil.expectPresentAfterGc {
+					expectedRemaining = append(expectedRemaining, sil.s.Id)
+				} else {
+					expectedGCCount += 1
+				}
+				// simulate this silences being seen in a query
+				silences.mc.Get(silences.st[sil.s.Id].Silence)
+			}
+
+			silClock.Add(time.Second)
+
+			n, err := silences.GC()
+			require.NoError(t, err)
+			require.Equal(t, expectedGCCount, n)
+
+			for _, id := range expectedRemaining {
+				foundSil, inState := silences.st[id]
+				if !inState {
+					require.Failf(t, "silence %s was missing from final state", id)
+				}
+				if _, ok := silences.mc[foundSil.Silence.Id]; !ok {
+					require.Failf(t, "silence %s's matchers were missing from the matcherCache", id)
+				}
+			}
+			require.Equalf(t, len(expectedRemaining), len(silences.st), "there are extra silences in the final state")
+			require.Equalf(t, len(expectedRemaining), len(silences.mc), "there are extra entries in the matcher cache")
+
+		})
+	}
+
+}
+
 func TestSilencesSnapshot(t *testing.T) {
 	// Check whether storing and loading the snapshot is symmetric.
 	now := clock.NewMock().Now().UTC()