fix(queryeviction): deregister evicted queries to prevent double accounting

Evicted victims were never removed from the QueryRegistry by the evictor;
the only cleanup is the deferred Deregister in trackedQuery.Exec, which
runs when the query goroutine eventually unwinds. Until then the
already-cancelled victim remained in the evictable candidate set, so a
later eviction cycle could re-pick it: cortex_query_evictions_total was
double-counted for a single query, a MaxEvictionsPerCycle slot was burned
on a no-op Cancel instead of relieving pressure, and in tests the second
Cancel callback panicked on a double channel close.

Deregister the victim before cancelling it, keeping all bookkeeping
(metric, registry) ahead of the externally observable cancellation
signal. Deregister is an idempotent locked delete, so the deferred
Deregister in trackedQuery.Exec remains a safe no-op when the query
finishes unwinding.

Note the registry semantic shift: registry.Len() (and the
registered_queries debug log field) now means "evictable candidates" -
it no longer includes cancelled-but-still-unwinding victims.

Add TestEvictedVictim_RemovedFromRegistry, which fails deterministically
without this fix (expected: 0, actual: 1): the victim stayed registered
after its eviction was observed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>

Author: sandy2008

pkg/util/queryeviction/evictor.go

@@ -103,6 +103,11 @@ func (e *QueryEvictor) running(ctx context.Context) error {
 				// externally observable commit point, so observers synchronized on
 				// the cancellation must already see it in evictionsTotal.
 				e.evictionsTotal.WithLabelValues(string(breachedResource)).Inc()
+				// Retire the victim before cancelling it so later cycles can never
+				// re-pick (and double-count) an already-cancelled query that is
+				// still unwinding. trackedQuery.Exec's own deferred Deregister
+				// remains a safe no-op.
+				e.registry.Deregister(victim.QueryID)
 				victim.Cancel()
 
 				level.Warn(e.logger).Log(

pkg/util/queryeviction/evictor_test.go

@@ -242,6 +242,35 @@ func TestEvictionsTotal_IncrementedBeforeCancelObserved(t *testing.T) {
 	assert.Equal(t, float64(1), counterAtCancel)
 }
 
+func TestEvictedVictim_RemovedFromRegistry(t *testing.T) {
+	reg := NewQueryRegistry(testMetricFunc)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	stats := &querier_stats.QueryStats{}
+	stats.AddFetchedSamples(1000)
+
+	// The sync.Once keeps the callback idempotent: if the evictor wrongly
+	// re-picked the still-registered victim on a later cycle, the second
+	// Cancel would otherwise panic on the double close.
+	var once sync.Once
+	evicted := make(chan struct{})
+	reg.Register(func() {
+		once.Do(func() {
+			cancel()
+			close(evicted)
+		})
+	}, stats, "up", "user1", "")
+	_ = ctx
+
+	startEvictor(t, newMockMonitor(0.95, 0.0), reg, testEvictorConfig(0.9, 0, 0))
+	waitEvicted(t, evicted)
+
+	// The evictor deregisters the victim before cancelling it, so once the
+	// eviction is observable the victim must already have left the registry
+	// and can never be re-picked (and double-counted) while still unwinding.
+	assert.Equal(t, 0, reg.Len())
+}
+
 func TestNewQueryEvictor_ReturnsNilWhenDisabled(t *testing.T) {
 	cfg := configs.EvictionConfig{CheckInterval: time.Second, EvictionMetric: "fetched_samples", MaxEvictionsPerCycle: 1}
 	evictor := NewQueryEvictor(newMockMonitor(0, 0), NewQueryRegistry(testMetricFunc), cfg, log.NewNopLogger(), prometheus.NewPedanticRegistry(), "test")