fix(kvblock): avoid full keyspace scan in Redis Clear

[yankay]

Summary

Optimize Redis/Valkey Clear by maintaining a per-pod reverse index.

Clear(ctx, podIdentifier) now scans kvblock:pod:<podIdentifier>:entries and removes entries with pipelined HDEL + SREM, instead of scanning the full Redis keyspace.

Notes

• Add records reverse-index members.
• Evict removes reverse-index members.
• Clear drops malformed reverse-index members and continues.
• Existing Redis data written before this change does not have reverse-index members. Existing deployments should flush the dedicated Redis/Valkey index DB during upgrade, or run a one-time full-scan migration, before relying on Clear for pre-upgrade entries.
• Trade-off: Add now writes both the request-key hash and the per-pod reverse-index set, and the set member stores <requestKey>\0<encodedPodField>. This adds write and memory overhead in exchange for avoiding full keyspace scans during Clear.

Benchmark

Real Redis redis:7.4.1, persistence disabled:

scenario	legacy full scan	reverse index	speedup
512 target / 512 unrelated / 8 pods	42.70 ms/op	1.56 ms/op	27.41x
2,048 target / 2,048 unrelated / 8 pods	167.45 ms/op	3.20 ms/op	52.28x
32,768 target / 32,768 unrelated / 8 pods	2,789.52 ms/op	66.07 ms/op	42.22x
2,048 target / 32,768 unrelated / 8 pods	907.41 ms/op	4.14 ms/op	219.18x

Testing

• make lint
• go test ./pkg/kvcache/kvblock
• go test ./pkg/kvevents
• go test ./tests/profiling/kv_cache_index -run '^$'
• go test -tags redis_real_bench ./tests/profiling/kv_cache_index -run '^$'
• go test -race ./pkg/kvcache/kvblock -run 'TestRedis|TestValkeyIndexBehavior/(ClearBasic|ClearIsolatesOtherPods|ClearAllTiers|ClearThenReAdd|StressHighCardinality)' -count=1

Fixes #672

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR refactors RedisIndex.Clear to avoid scanning the entire Redis keyspace by maintaining a per-pod reverse index, and adds targeted tests to validate the new behavior.

Changes:

• Add a per-pod Redis SET reverse index updated from Add and Evict.
• Rewrite Clear to operate via SSCAN over the per-pod reverse index rather than SCAN over all keys.
• Add tests covering “clear doesn’t delete unrelated keys” and “evict removes reverse-index entries”.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
pkg/kvcache/kvblock/redis.go	Introduces reverse-index helpers; updates Add/eviction to maintain it; rewrites Clear to use it.
pkg/kvcache/kvblock/redis_test.go	Adds inspection helper and new tests validating Clear isolation and reverse-index pruning on eviction.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[yankay]

Hi @vMaroon,

If you have bandwidth, would you mind taking a look at the Redis Clear reverse-index approach when convenient?

Thanks a lot!

[gkneighb]

Following up on my +1 from #672 with an actual review -- I had mapped the same per-pod reverse-index design against the code before finding this PR, so this is the shape I believe in. Verified beyond reading:

• The branch applies cleanly on current main, builds, and pkg/kvcache/kvblock tests pass locally.
• Independently reproduced the benchmark (miniredis mode): legacy full-scan Clear ~945ms vs ~19ms reverse-index at 2048 target / 32768 unrelated / 8 pods, and reverse-index time stays flat as the unrelated keyspace grows (26ms at 0 unrelated, 19ms at 32k) -- which is the real point: Clear cost now scales with the pod's own entries, not the keyspace.

Correctness walkthrough, all confirmed against the code:

• There is exactly one HSet site writing pod fields, so the reverse index covers every write path. Evict SRems with the byte-identical field string it HDels, so the set can only miss where the HDel would have missed anyway -- no new divergence risk.
• Dropping pruneRequestKeyScript from Clear is safe: HDEL of a hash's last field auto-removes the key in Redis (the new test asserts EXISTS == 0), and the script only guarded an empty-hash state that can't persist.
• The SScan-then-SCard re-loop is right: SScan doesn't guarantee full coverage of a set shrinking under it, and termination holds because every observed member -- including malformed ones -- is SRem'd, so the set strictly shrinks absent new Adds for the pod, and Clear targets pods that are gone.

The one design point worth a maintainer decision is the back-compat position (the question I'd raised on #672): index-authoritative plus a documented flush is defensible, and I see why fallback-to-SCAN is unattractive -- an empty reverse set is indistinguishable from "pre-upgrade entries exist unindexed", so a naive fallback would full-scan on every Clear of an already-clean pod and defeat the fix. But the failure mode when an operator misses the upgrade note is silent: a restarted pod that reuses its identity keeps its pre-upgrade fields, so Lookup keeps reporting cache hits for a now-cold pod. A cheap middle ground would be a one-time schema marker (SETNX kvblock:schema:v2 on first Add; Clear logs a warning -- or falls back exactly once -- when the marker is absent but the keyspace is non-empty). Fine as a follow-up or an explicit "documented flush is enough", raising it because the miss is invisible when it happens.

One non-blocking observation: engine:* mappings are not pruned on Clear -- same as the old code, so not a regression, just noting it was considered.

LGTM from my side (not an approver).