From f097de8f47ca2ec66c2d77929ced853ffe484361 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Mon, 26 Oct 2026 22:05:53 +0000 Subject: [PATCH 1/2] fix(querier): initialise subservicesWatcher in blocksStoreReplicationSet blocksStoreReplicationSet declares a subservicesWatcher field and calls WatchManager on it from starting(), but newBlocksStoreReplicationSet never assigns it. WatchManager registers a manager listener whose callback sends on w.ch, and unlike Chan() it has no nil receiver guard, so the first subservice failure dereferences a nil *FailureWatcher. Manager.AddListener runs each listener on its own goroutine, so the resulting SIGSEGV is not recoverable by the caller and terminates the process: panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation] goroutine 10 [running]: ...services.(*FailureWatcher).WatchManager.func1(...) pkg/util/services/failure_watcher.go:33 ...services.(*Manager).AddListener.func1() pkg/util/services/manager.go:245 created by ...services.(*Manager).AddListener in goroutine 8 pkg/util/services/manager.go:243 The subservices here are the store-gateway ring and its client pool, so the failing path is reachable in practice: when the ring cannot be started, the panic's stack trace buries the error logged for the actual cause on the preceding line, making a routine startup failure look like a crash inside Cortex. The bug is masked at the other end. Chan() does guard a nil receiver and returns a nil channel, so the select in running() blocks forever on it rather than reporting the failure. A subservice failure is therefore either silently dropped or crashes the process, depending on which path observes it first. Assign services.NewFailureWatcher() in the constructor, matching BlocksStoreQueryable and parquetQueryableWithFallback, which already do. Adds a regression test asserting the field is non-nil after construction. Signed-off-by: Ben Ye --- pkg/querier/blocks_store_replicated_set.go | 11 ++++---- .../blocks_store_replicated_set_test.go | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/pkg/querier/blocks_store_replicated_set.go b/pkg/querier/blocks_store_replicated_set.go index d102a522e24..b2c1546a469 100644 --- a/pkg/querier/blocks_store_replicated_set.go +++ b/pkg/querier/blocks_store_replicated_set.go @@ -58,11 +58,12 @@ func newBlocksStoreReplicationSet( zoneStableShuffleSharding bool, ) (*blocksStoreReplicationSet, error) { s := &blocksStoreReplicationSet{ - storesRing: storesRing, - clientsPool: newStoreGatewayClientPool(client.NewRingServiceDiscovery(storesRing), clientConfig, logger, reg), - shardingStrategy: shardingStrategy, - balancingStrategy: balancingStrategy, - limits: limits, + storesRing: storesRing, + clientsPool: newStoreGatewayClientPool(client.NewRingServiceDiscovery(storesRing), clientConfig, logger, reg), + shardingStrategy: shardingStrategy, + balancingStrategy: balancingStrategy, + limits: limits, + subservicesWatcher: services.NewFailureWatcher(), zoneAwarenessEnabled: zoneAwarenessEnabled, zoneStableShuffleSharding: zoneStableShuffleSharding, diff --git a/pkg/querier/blocks_store_replicated_set_test.go b/pkg/querier/blocks_store_replicated_set_test.go index 6ccc65d4a82..9cb18073985 100644 --- a/pkg/querier/blocks_store_replicated_set_test.go +++ b/pkg/querier/blocks_store_replicated_set_test.go @@ -758,3 +758,30 @@ func getStoreGatewayClientAddrs(clients map[BlocksStoreClient][]ulid.ULID) map[s } return addrs } + +// TestBlocksStoreReplicationSet_SubservicesWatcherIsInitialised guards against a nil +// subservicesWatcher. starting() calls WatchManager on it, which registers a callback +// that sends on the watcher's channel, so a nil watcher panics in the goroutine the +// service manager runs the callback on as soon as a subservice fails. +func TestBlocksStoreReplicationSet_SubservicesWatcherIsInitialised(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + ringStore, closer := consul.NewInMemoryClient(ring.GetCodec(), log.NewNopLogger(), nil) + t.Cleanup(func() { assert.NoError(t, closer.Close()) }) + + require.NoError(t, ringStore.CAS(ctx, "test", func(in any) (any, bool, error) { + return ring.NewDesc(), true, nil + })) + + ringCfg := ring.Config{} + flagext.DefaultValues(&ringCfg) + + r, err := ring.NewWithStoreClientAndStrategy(ringCfg, "test", "test", ringStore, ring.NewIgnoreUnhealthyInstancesReplicationStrategy(), nil, nil) + require.NoError(t, err) + + s, err := newBlocksStoreReplicationSet(r, util.ShardingStrategyDefault, noLoadBalancing, &blocksStoreLimitsMock{}, ClientConfig{}, log.NewNopLogger(), prometheus.NewPedanticRegistry(), false, false) + require.NoError(t, err) + require.NotNil(t, s.subservicesWatcher) +} From 313cd37f61947c72282d3ddf0adf520f3e2c25f0 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Mon, 26 Oct 2026 22:06:35 +0000 Subject: [PATCH 2/2] Add CHANGELOG entry for #7896 Signed-off-by trailer added by git commit -s. Signed-off-by: Ben Ye --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 969e6c8f3b5..b53819eb496 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ * [BUGFIX] Ingester: Staleness markers written by the Ruler are no longer reported as discarded samples for the tenant. #7887 * [BUGFIX] Ruler: Stop retrying staleness markers that can never be accepted. A rule moved between rule groups left the original group re-appending staleness markers for its series on every evaluation until the ruler restarted. #7888 * [BUGFIX] Store Gateway: Fix the Parquet bucket store ignoring query sharding, which made every shard return all series. #7892 +* [BUGFIX] Querier: Fix the querier crashing instead of reporting the error when a blocks store set subservice fails. `blocksStoreReplicationSet` never initialised its `FailureWatcher`, so the failure callback dereferenced a nil pointer on the goroutine the service manager runs listeners on, and the resulting panic's stack trace buried the error logged for the actual cause. #7896 ## 1.22.0 in progress