metastore: Index.Restore leaves stale shard cache entries after a raft snapshot restore, panicking QueryMetadata
- Dominant language
- Go
- Stars
- 11.7k
- Forks
- 802
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 80
Description
## 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.
```go
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
| Location | What it does |
| --- | --- |
| `pkg/metastore/fsm/fsm.go:187-199` | `FSM.Restore` replaces the bbolt DB, then runs the restorers. Nothing invalidates the index caches. |
| `pkg/metastore/index/index.go:104-134` | `Index.Restore` reloads only shards in partitions inside the lookaround window. It never purges `i.shards`, and never touches `i.blocks` at all. |
| `pkg/metastore/index/index_cache.go:102-105` | `Index.Restore` runs under a read transaction (`fsm.go:158`, `boltdb.View`) yet `getForWriteUnsafe` caches with `readOnly: false`, contradicting the cache contract documented at `index_cache.go:26-38`. |
| `pkg/metastore/index/index_cache.go:126` | `if !x.readOnly \|\| version == 0 \|\| x.ShardIndex.Version >= version` - the `!x.readOnly` disjunct skips #5189's version check for exactly those entries. |
| `pkg/block/metadata/metadata_labels.go:301`, `:315`, `:374` | Unchecked indexing into `lm.strings`. `StringTable.Lookup` (`metadata.go:90-95`) is bounds-checked and returns `""`, and `Export` uses it, so the same bad reference degrades silently on one path and panics on another. |
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:
```go
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.
Contributor guide
No contributing guide indexed for this repository
Research direction
Run the supplied TestIndexRestoreStaleShardCache reproduction from pkg/metastore/index, then read Index.Restore in pkg/metastore/index/index.go alongside index_cache.go and the sibling restorers. Use TestIndex_QueryMetadata_StaleReadCacheReloadsShard in query_test.go as a nearby example; done means restore and subsequent queries or writes do not reuse stale cache state, and the regression tests pass without panic or metadata corruption.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 62/100