From 89e69b87070040a3e58b5f94e94a25753efaadfb Mon Sep 17 00:00:00 2001 From: Anas Khan <83116240+anxkhn@users.noreply.github.com> Date: Fri, 3 Jul 2026 12:43:12 +0530 Subject: [PATCH] Close per-query querier in the remote read handler The remote read handler creates a storage.Querier for each sub-query in its own goroutine but never calls Close() on it. It is the only storage.Querier call site in the codebase that does not close the querier, so it does not honor the interface contract. The concrete queriers wired into the read path today all have a Close() that returns nil, so this is not an observed leak in the current code. It is a correctness and consistency fix: any querier that does release resources in Close() (a future implementation, a wrapper, or an upstream Prometheus change) is then released deterministically instead of relying on GC. Defer querier.Close() inside the goroutine so it runs on every path, matching every other querier call site. Add a regression test asserting that each per-query querier is closed. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com> --- CHANGELOG.md | 1 + pkg/querier/remote_read.go | 1 + pkg/querier/remote_read_test.go | 46 +++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 563d7aabf9b..bc5d74ddfa1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,7 @@ * [BUGFIX] Ingester: Fix panic (`HistogramProtoToHistogram called with a float histogram`) when ingesting a float native histogram with a zero count (e.g. a staleness marker or empty histogram). The decoder is now selected by histogram type via `IsFloatHistogram()` instead of by count value. #7645 * [BUGFIX] Querier: Fix parquet queryable fallback returning a nil error instead of the actual query error in `LabelValues` and `LabelNames`. #7638 * [BUGFIX] Store Gateway: Fix misleading "no index cache backend addresses" validation error being reported for chunks-cache, metadata-cache, and parquet caches when their memcached or redis backend is configured without addresses. The message is now the cache-type-agnostic "no cache backend addresses". #7675 +* [BUGFIX] Querier: Close the per-query `storage.Querier` in the remote read handler once each sub-query completes. The handler was the only `storage.Querier` call site that did not call `Close()`, so it did not honor the interface contract and would leak any resources a querier releases in `Close()`. #7672 ## 1.21.0 2026-04-24 diff --git a/pkg/querier/remote_read.go b/pkg/querier/remote_read.go index 5067f1a8f51..cfa8efab155 100644 --- a/pkg/querier/remote_read.go +++ b/pkg/querier/remote_read.go @@ -46,6 +46,7 @@ func RemoteReadHandler(q storage.Queryable, logger log.Logger) http.Handler { errors <- err return } + defer querier.Close() params := &storage.SelectHints{ Start: int64(from), diff --git a/pkg/querier/remote_read_test.go b/pkg/querier/remote_read_test.go index b3e7c0c28b4..4aac75ed0f7 100644 --- a/pkg/querier/remote_read_test.go +++ b/pkg/querier/remote_read_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/go-kit/log" "github.com/gogo/protobuf/proto" @@ -17,10 +18,12 @@ import ( "github.com/prometheus/prometheus/storage" "github.com/prometheus/prometheus/util/annotations" "github.com/stretchr/testify/require" + "go.uber.org/atomic" "github.com/cortexproject/cortex/pkg/cortexpb" "github.com/cortexproject/cortex/pkg/ingester/client" "github.com/cortexproject/cortex/pkg/querier/series" + "github.com/cortexproject/cortex/pkg/util/test" ) func TestRemoteReadHandler(t *testing.T) { @@ -88,6 +91,37 @@ func TestRemoteReadHandler(t *testing.T) { require.Equal(t, expected, response) } +func TestRemoteReadHandler_Closes_Querier(t *testing.T) { + t.Parallel() + var closed atomic.Int64 + q := storage.QueryableFunc(func(mint, maxt int64) (storage.Querier, error) { + return &closeCountingQuerier{closed: &closed}, nil + }) + handler := RemoteReadHandler(q, log.NewNopLogger()) + + const numQueries = 3 + queries := make([]*client.QueryRequest, numQueries) + for i := range queries { + queries[i] = &client.QueryRequest{StartTimestampMs: 0, EndTimestampMs: 10} + } + requestBody, err := proto.Marshal(&client.ReadRequest{Queries: queries}) + require.NoError(t, err) + requestBody = snappy.Encode(nil, requestBody) + request, err := http.NewRequest("GET", "/query", bytes.NewReader(requestBody)) + require.NoError(t, err) + request.Header.Set("X-Prometheus-Remote-Read-Version", "0.1.0") + + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, request) + + require.Equal(t, 200, recorder.Result().StatusCode) + // Each per-query querier is closed by a deferred call in its own goroutine, + // which may run after ServeHTTP returns, so poll until every querier is closed. + test.Poll(t, time.Second, int64(numQueries), func() any { + return closed.Load() + }) +} + type mockQuerier struct { matrix model.Matrix } @@ -110,3 +144,15 @@ func (m mockQuerier) LabelNames(ctx context.Context, _ *storage.LabelHints, matc func (mockQuerier) Close() error { return nil } + +// closeCountingQuerier records how many times Close is called so a test can +// assert the remote read handler releases every querier it creates. +type closeCountingQuerier struct { + mockQuerier + closed *atomic.Int64 +} + +func (q *closeCountingQuerier) Close() error { + q.closed.Add(1) + return nil +}