metastore: Index.Restore leaves stale shard cache entries after a raft snapshot restore, panicking QueryMetadata

Summary

Index.Restore does not invalidate the metastore index's in-memory shardCache/blockCache. When the raft FSM installs a snapshot it replaces the whole bbolt database, but cache entries loaded from the previous database survive. A surviving entry pairs an old shard string table with block metadata read from the new database, and because dataset labels are int32 indices into that table, LabelMatcher.checkMatches indexes past the end:

panic: runtime error: index out of range [N] with length N
  pkg/block/metadata.(*LabelMatcher).checkMatches        metadata_labels.go:301
  pkg/block/metadata.(*LabelMatcher).MatchesPairs        metadata_labels.go:281
  pkg/block/metadata.(*LabelMatcher).CollectMatches      metadata_labels.go:235
  pkg/metastore/index.(*blockMetadataQuerier).collectMatched  query.go:174
  pkg/metastore/index.(*Index).QueryMetadata                  index.go:259
  pkg/metastore.(*QueryService).queryMetadata            query_service.go:89

This is a gap in the guard added by #5189, not a new regression. That fix versions read-cached shards; entries cached by Index.Restore are marked readOnly: false and take the !x.readOnly short-circuit that skips the version check entirely.

Impact

QueryMetadata panics on affected shards. The panic is recovered by the gRPC interceptor (pkg/util/recovery.go:41), so the process survives and the client gets codes.Internal, but the read fails and pyroscope_panic_total increments. QueryService uses followerRead (pkg/metastore/metastore.go:180), so any replica can serve the query, and followers are exactly the nodes that install snapshots.

It clears on its own once the stale entries age out of the shard LRU, which makes it easy to misread as a transient blip.

Reproduction

Fails on main. Drop into pkg/metastore/index/ and run. The blocks are deliberately dated well in the past, because Index.Restore only reloads partitions overlapping now +/- queryLookaroundPeriod (index.go:107-112); the shard cache entry for an older partition is the residue that survives.

package index

import (
	"context"
	"testing"
	"time"

	"github.com/stretchr/testify/require"
	"go.etcd.io/bbolt"

	metastorev1 "github.com/grafana/pyroscope/api/gen/proto/go/metastore/v1"
	"github.com/grafana/pyroscope/v2/pkg/test"
	"github.com/grafana/pyroscope/v2/pkg/util"
)

var (
	minT = test.UnixMilli("2024-09-23T08:00:00.000Z")
	maxT = test.UnixMilli("2024-09-23T09:00:00.000Z")
	idA  = test.ULID("2024-09-23T08:00:00.001Z")
	idB  = test.ULID("2024-09-23T08:30:00.002Z")
)

func block(id, svc string) *metastorev1.BlockMeta {
	return &metastorev1.BlockMeta{
		Id: id, Tenant: 1, Shard: 1, MinTime: minT, MaxTime: maxT,
		Datasets: []*metastorev1.Dataset{{
			Tenant: 1, MinTime: minT, MaxTime: maxT, Labels: []int32{1, 2, 3},
		}},
		StringTable: []string{"", "tenant-a", "service_name", svc},
	}
}

func TestIndexRestoreStaleShardCache(t *testing.T) {
	// Local pre-snapshot state: block A only. Populates the shard cache.
	idx := NewIndex(util.Logger, NewStore(), DefaultConfig, nil)
	local := test.BoltDB(t)
	require.NoError(t, local.Update(idx.Init))
	require.NoError(t, local.Update(func(tx *bbolt.Tx) error {
		return idx.InsertBlock(tx, block(idA, "svc-a"))
	}))

	// Snapshot state, ahead of local: blocks A and B, so a longer string table.
	snap := test.BoltDB(t)
	writer := NewIndex(util.Logger, NewStore(), DefaultConfig, nil)
	require.NoError(t, snap.Update(writer.Init))
	require.NoError(t, snap.Update(func(tx *bbolt.Tx) error {
		if err := writer.InsertBlock(tx, block(idA, "svc-a")); err != nil {
			return err
		}
		return writer.InsertBlock(tx, block(idB, "svc-b"))
	}))

	// What FSM.Restore does: swap the database, then run restorers under a read tx.
	require.NoError(t, snap.View(idx.Restore))

	// Panics: the cached shard still holds the pre-snapshot string table.
	require.NoError(t, snap.View(func(tx *bbolt.Tx) error {
		_, err := idx.QueryMetadata(tx, context.Background(), MetadataQuery{
			Expr:      `{service_name=~".+"}`,
			StartTime: time.UnixMilli(minT),
			EndTime:   time.UnixMilli(maxT),
			Tenant:    []string{"tenant-a"},
			Labels:    []string{"service_name"},
		})
		return err
	}))
}

--- FAIL: TestIndexRestoreStaleShardCache
panic: runtime error: index out of range [4] with length 4
  pkg/block/metadata/metadata_labels.go:301
  pkg/metastore/index/query.go:174
  pkg/metastore/index/index.go:259

The same query against the same database from an index with a cold cache succeeds, so the cache entry is at fault, not the data.

Where it breaks

For comparison, the three sibling restorers all reset in-memory state first: compactor.go:219-221, scheduler.go:243-247, tombstones.go:170-174, each with the comment "Reset in-memory state before loading entries from the store." The index restorer is the only one that does not.

Second-order consequence: silent metadata corruption

Lower frequency, higher severity, so worth fixing together.

Shard.Store keys the new string chunk by the pre-import table length (store/shard.go:62-73). A write routed through a stale cached shard overwrites the chunk already at that key with a shorter payload and rolls the version forward. After that the on-disk table is durably wrong, and once it regrows past the collision point the indices resolve to the wrong strings: label values are silently mislabeled instead of panicking.

Same helpers as above, plus idC = test.ULID("2024-09-23T08:45:00.003Z"). Run it separately from the test above, whose panic aborts the test binary:

func TestIndexStaleShardWriteCorruptsStringTable(t *testing.T) {
	idx := NewIndex(util.Logger, NewStore(), DefaultConfig, nil)
	local := test.BoltDB(t)
	require.NoError(t, local.Update(idx.Init))
	require.NoError(t, local.Update(func(tx *bbolt.Tx) error {
		return idx.InsertBlock(tx, block(idA, "svc-a"))
	}))

	snap := test.BoltDB(t)
	writer := NewIndex(util.Logger, NewStore(), DefaultConfig, nil)
	require.NoError(t, snap.Update(writer.Init))
	require.NoError(t, snap.Update(func(tx *bbolt.Tx) error {
		if err := writer.InsertBlock(tx, block(idA, "svc-a")); err != nil {
			return err
		}
		return writer.InsertBlock(tx, block(idB, "svc-b"))
	}))

	require.NoError(t, snap.View(idx.Restore))

	// A later raft command writes a block into the same (stale-cached) shard.
	require.NoError(t, snap.Update(func(tx *bbolt.Tx) error {
		return idx.InsertBlock(tx, block(idC, "svc-c"))
	}))

	// Read back from disk with a cold index. Block B must still be "svc-b".
	fresh := NewIndex(util.Logger, NewStore(), DefaultConfig, nil)
	require.NoError(t, snap.View(func(tx *bbolt.Tx) error {
		metas, err := fresh.GetBlocks(tx, &metastorev1.BlockList{
			Tenant: "tenant-a", Shard: 1, Blocks: []string{idA, idB, idC},
		})
		require.NoError(t, err)
		for _, m := range metas {
			svc := m.StringTable[m.Datasets[0].Labels[2]]
			t.Logf("block %s service_name=%q", m.Id, svc)
			if m.Id == idB {
				require.Equal(t, "svc-b", svc, "block B service_name was silently rewritten")
			}
		}
		return nil
	}))
}

block 01J8EYA0019Y4RY8TJ33V8JZZZ service_name="svc-a"
block 01J8F00XT20FCHC2GNR1BHFXF9 service_name="svc-c"
    Error:    Not equal:
              expected: "svc-b"
              actual  : "svc-c"
    Messages: block B service_name was silently rewritten
--- FAIL: TestIndexStaleShardWriteCorruptsStringTable

Block B is read back as a different service. No panic, no error, no metric.

Reaching it needs a write to a stale-cached shard, which after a snapshot install means a write into a partition outside the restore window. Routine ingest and compaction produce fresh ULIDs and land in reloaded partitions, so the realistic triggers are DLQ recovery of an older segment, and shards absent from the snapshot entirely.

Related

• #5189 (fix(metastore/index): version shard cache reads, merged 2026-05-28) - fixed the same panic signature for read-cached shards and named these exact symptoms. Its code comment states the assumption this bug violates: "Write-cached shards are always safe to reuse because writes are serialized and observe the latest shard state." That holds only while the underlying database is continuous; FSM.Restore swaps it. The regression test added there, TestIndex_QueryMetadata_StaleReadCacheReloadsShard (query_test.go:328), is the natural neighbour for a new one.
• #4079 (feat(v2): non-blocking metadata queries) - introduced index_cache.go and the readOnly design.
• #3744 (feat(v2): metadata string interning) - made block metadata reference a shard-level table by index, creating this failure mode.
• #5414 (feat(metastore): online BoltDB compaction, open) - hot-swaps the bbolt file via fsm.db.openPath on an interval, and likewise runs neither fsm.init()/fsm.restore() nor any cache invalidation. It is a second path where these caches can end up describing a database that is no longer there. The divergence runs the opposite way (the caches end up ahead of the swapped-in copy rather than behind it), so it would not produce this panic, but whatever invalidation hook fixes this issue is likely a prerequisite there. Details in a comment on that PR.
• #5432 and #5443 report metastore leadership churn requiring manual restarts, which is the kind of event that drives a follower to install a snapshot.

No existing issue covers this panic.

Suggested fix

1. Invalidate shards and blocks on restore. Either purge in Index.Restore to match the sibling restorers, or add an explicit hook in FSM.Restore after db.restore and before init()/restore().
2. Do not cache Index.Restore-loaded shards as writable. They come from a read transaction, so readOnly: true is correct and re-enables the version guard for them on its own.
3. Optionally close the remaining guard holes at index_cache.go:126: apply the version check to write-cached entries, and treat version == 0 as "unknown, reload" rather than "safe".
4. Defense in depth: use the bounds-checked StringTable.Lookup at metadata_labels.go:301, :315, :374, so a future cache-coherence bug degrades a query instead of panicking a request.

Environment

Reproduced on main (v2.3.0, commit e58d3b3bf). Present since the shard cache was introduced in #4079 (weekly-f114); #5189 (weekly-f173) narrowed it to the write-cached path but did not close it. Also observed in production on a v2 deployment, where a rolling restart of the metastore StatefulSet produced a burst of these panics on one replica that stopped once the stale entries aged out.