fix(discovery): prevent the manager from storing stale targetGroups

Signed-off-by: machine424 <[email protected]>

discovery/manager.go

@@ -394,8 +394,16 @@ func (m *Manager) updateGroup(poolKey poolKey, tgs []*targetgroup.Group) {
 		m.targets[poolKey] = make(map[string]*targetgroup.Group)
 	}
 	for _, tg := range tgs {
-		if tg != nil { // Some Discoverers send nil target group so need to check for it to avoid panics.
+		// Some Discoverers send nil target group so need to check for it to avoid panics.
+		if tg == nil {
+			continue
+		}
+		if len(tg.Targets) > 0 {
 			m.targets[poolKey][tg.Source] = tg
+		} else {
+			// The target group is empty, drop the corresponding entry to avoid leaks.
+			// In case the group yielded targets before, allGroups() will take care of making consumers drop them.
+			delete(m.targets[poolKey], tg.Source)
 		}
 	}
 }

discovery/manager_test.go

@@ -1051,8 +1051,8 @@ func TestDiscovererConfigs(t *testing.T) {
 }
 
 // TestTargetSetRecreatesEmptyStaticConfigs ensures that reloading a config file after
-// removing all targets from the static_configs sends an update with empty targetGroups.
-// This is required to signal the receiver that this target set has no current targets.
+// removing all targets from the static_configs cleans the corresponding targetGroups entries to avoid leaks and sends an empty update.
+// The update is required to signal the consumers that the previous targets should be dropped.
 func TestTargetSetRecreatesEmptyStaticConfigs(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -1085,16 +1085,14 @@ func TestTargetSetRecreatesEmptyStaticConfigs(t *testing.T) {
 	discoveryManager.ApplyConfig(c)
 
 	syncedTargets = <-discoveryManager.SyncCh()
+	require.Len(t, discoveryManager.targets, 1)
 	p = pk("static", "prometheus", 1)
 	targetGroups, ok := discoveryManager.targets[p]
-	require.True(t, ok, "'%v' should be present in target groups", p)
-	group, ok := targetGroups[""]
-	require.True(t, ok, "missing '' key in target groups %v", targetGroups)
-
-	require.Empty(t, group.Targets, "Invalid number of targets.")
-	require.Len(t, syncedTargets, 1)
-	require.Len(t, syncedTargets["prometheus"], 1)
-	require.Nil(t, syncedTargets["prometheus"][0].Labels)
+	require.True(t, ok, "'%v' should be present in targets", p)
+	// Otherwise the targetGroups will leak, see https://github.com/prometheus/prometheus/issues/12436.
+	require.Empty(t, targetGroups, 0, "'%v' should no longer have any associated target groups", p)
+	require.Len(t, syncedTargets, 1, "an update with no targetGroups should still be sent.")
+	require.Empty(t, syncedTargets["prometheus"], 0)
 }
 
 func TestIdenticalConfigurationsAreCoalesced(t *testing.T) {

scrape/manager_test.go

@@ -1178,7 +1178,7 @@ scrape_configs:
 	)
 }
 
-// TestOnlyStaleTargetsAreDropped makes sure that when a job has multiple providers, when aone of them should no,
+// TestOnlyStaleTargetsAreDropped makes sure that when a job has multiple providers, when one of them should no
 // longer discover targets, only the stale targets of that provier are dropped.
 func TestOnlyStaleTargetsAreDropped(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())