From 3fa6e857b6f8835989a615633550f1f0dfc191e8 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 abd8bcc7927..4b95e6ec349 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,7 @@ * [BUGFIX] Compactor: Fix spurious `bucket operation fail after retries` error logs emitted during partial block cleanup. #7749 * [BUGFIX] Alertmanager: Fix panic in `validateAlertmanagerConfig` when receiver config traversal encounters nil interface values. #7751 * [BUGFIX] Parquet Converter: Fix `auto_forget_delay` having no effect. The ring lifecycler was created without the auto-forget delegate, so unhealthy instances were never automatically removed from the ring. #7752 +* [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.1 2026-06-04 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 +}