feat(anomaly): detect orphaned keys in unowned cluster slots (valkey#539) by jamby77 · Pull Request #376 · BetterDB-inc/monitor

jamby77 · 2026-08-11T07:02:10Z

Summary

New detector for the valkey-io/valkey#539 leak: after a node loads an RDB/AOF file whose keyspace includes slots it no longer owns (a persistence file predating a slot migration, or a warm-up reusing a shared RDB), the engine loads every key regardless of ownership. Keys in unowned slots are permanently unreachable — clients get routed to the slots' actual owners — yet they count in dbsize and hold memory. Upstream treats the missing cleanup as effectively a bugfix but has not shipped it, so the hazard is durable.

Deliberate deviation from #367

The issue specifies detection via the per-slot path (keyed slots outside the owned ranges) with dbsize as corroboration. This PR inverts that: the dbsize surplus is the only signal, and there is no per-slot path.

The reason is that CLUSTER SLOT-STATS reports exclusively the slots a node is assigned, so leaked keys are invisible to it and the per-slot path can never fire on a real server. Verified on Valkey 8.1.6 with a deliberately leaked slot (5 keys resident in slot 941 on a node that does not own it):

probe	result
`CLUSTER COUNTKEYSINSLOT 941`	`5` — keys are there
`CLUSTER GETKEYSINSLOT 941 10`	lists all 5
`CLUSTER SLOT-STATS SLOTSRANGE 941 941`	empty
`CLUSTER SLOT-STATS ORDERBY key-count LIMIT 16384`	slot 941 absent
`DBSIZE` vs `sum(SLOT-STATS key-count)`	`8` vs `3` → surplus `5`, exactly the leak

Same outcome whether the slot is unassigned cluster-wide (CLUSTER DELSLOTS) or owned by another node (the realistic post-migration shape). So the surplus is not corroboration for a better signal — it is the signal.

Detection

Pure snapshot detector (orphaned-slot-keys-detector.ts): the surplus of DBSIZE over every slot CLUSTER SLOT-STATS reports, above a 100-key floor.
Churn cannot manufacture a surplus. DBSIZE is read on both sides of the SLOT-STATS read and the low-water value is used. With owned-slot keys O(t) and leaked keys L, the surplus is min(O(t0), O(t2)) + L - O(t1), which is at most L for any monotone change in O, and exactly L on a steady keyspace. A single pre-read (the earlier form) was biased positive by deletes — a key counted by DBSIZE but expired before the stats read reads as leaked — which on a high-churn TTL cache recurs every poll and would sail through the persistence gate.
DBSIZE and SLOT-STATS were confirmed to count pending-expiry keys identically (200 logically-expired-but-uncollected keys with active expiry disabled: both report 250, surplus 0), so there is no systematic surplus from expiry accounting.
Cluster primaries only — replicas mirror their primary's keyspace; standalone is a no-op. Capability-gated on CLUSTER SLOT-STATS (Valkey 8+ / Redis 8.2+).
In-flight resharding excluded: an IMPORTING slot suppresses the signal (arriving keys inflate dbsize before their slot is assigned), and the service gates on 30s persistence so a reshard window whose markers fell between polls cannot false-positive. MIGRATING needs no special case — a slot stays assigned and counted until handoff, and migration's deletions only move the surplus down under the low-water rule.
Probed at most once per 15s, not every poll: the probe costs two DBSIZEs plus a SLOT-STATS reply carrying every slot on the node, while ANOMALY_POLL_INTERVAL_MS defaults to 1s — needless for a leak that is permanent once loaded.
Every skip (import in flight, failed read, throttled poll) is an observation gap: the persistence gate is not touched, so a gap can never read as recovery and flap the alert.
Distinct from replica-slot-state (stuck migration markers on replicas); this watches orphaned data on primaries.

Event

WARNING carrying the surplus as the event value, an explanation that SLOT-STATS cannot name the leaked slots (with CLUSTER COUNTKEYSINSLOT as the way to locate them), and the remediation advice. Surfaced in the anomaly dashboard as "Orphaned Slot Keys".

Test plan

13 detector unit tests: surplus fires, below-floor silent, dbsize unavailable, import suppressed, standalone/replica no-ops, zero-key slots ignored, signature stability — plus a churn group asserting no fire on draining or insert-heavy keyspaces, a genuine leak still reported while the keyspace drains around it, and an exact count on a steady keyspace.
Service-level: bracketed DBSIZE reads, no fire on a draining keyspace whose naive surplus would clear the floor, probe throttling with skips as gaps, plus the existing persistence/gap/re-alert coverage.
Full anomaly-detection suite 534/534 green; tsc --noEmit clean. Whole backend suite green apart from the pre-existing license.service failure on master.

Closes #367

jamby77 · 2026-08-11T09:40:01Z

bugbot run

jamby77 · 2026-08-11T10:06:04Z

bugbot run

cursor

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 8f6595c. Configure here.}

KIvanow

Blocker 1 - verify the load-bearing SLOT-STATS assumption. The whole design pivots (commit 2) on "CLUSTER SLOT-STATS only reports slots assigned to the node," which demotes the precise per-slot path to inert code and makes the dbsize surplus the sole real signal. But Valkey 8 stores keys in a per-slot kvstore, so leaked keys live in their slot's dict regardless of ownership - SLOT-STATS may well report key_count for unowned-but-populated slots. Please confirm on a real node with a deliberately-leaked slot. If it does report them, the explicit path is the correct precise signal and should be primary (with dbsize as corroboration, as #367 intended); if it genuinely doesn't, the explicit path and its tests are dead and should go. Right now we don't know which, and it decides the architecture.

Blocker 2 - the dbsize surplus false-positives on delete/expiry-heavy workloads. The DBSIZE-then-SLOT-STATS ordering only biases against inserts; a key present at the DBSIZE read but deleted/expired before the SLOT-STATS read inflates the surplus. On a high-churn TTL cache (continuous background + lazy expiry), the surplus is biased positive every poll, and since the dbsize-delta signature is constant, two polls above the 100-key floor 30s apart fire a false leak WARNING on a healthy cache - the most common Valkey workload. Please gate on the surplus growing across polls rather than merely persisting above a static floor, raise the floor to something workload-relative, or require a second confirming signal. (Also worth checking DBSIZE and SLOT-STATS count pending-expiry keys identically, else there's a permanent systematic surplus.)

3 - Dead code: the explicit slot_stats path is documented as unable to fire on a real server, yet keeps ~10 tests plus the range-compression/sha1 machinery that only it uses - same concern as #375; resolve it via Blocker 1.

Non-blocking:

Design deviation: #367's detection is the per-slot path with dbsize as corroboration; the PR inverts that. Worth calling out explicitly rather than leaving it in a commit message.
Cost: DBSIZE + SLOT-STATS(key-count, 16384) runs unthrottled every cluster-primary poll (and the final commit added a CLUSTER NODES fetch under Raft where there was none) - a cache/throttle would suit a slow-moving leak signal.

…539) - New snapshot detector: keyed slots outside the primary's owned ranges are unreachable data loaded from a persistence file scoped wider than current slot ownership; reports per-slot counts, the total, and a dbsize corroboration delta - Excludes slots with in-flight migrating/importing markers and gates on 30s persistence so a reshard window cannot false-positive - Cluster primaries only (capability-gated on CLUSTER SLOT-STATS); standalone and replicas are no-ops - New orphaned_slot_keys metric type wired into the gossip topology poll and the dashboard labels

CLUSTER SLOT-STATS only reports slots assigned to the node, so leaked slots never appear in its reply and the explicit per-slot path cannot fire on a real server. Promote the dbsize corroboration to a first-class signal: DBSIZE counts every local key while SLOT-STATS sums only assigned slots, so a persistent surplus beyond a 100-key write-race floor is the leak. DBSIZE is read before SLOT-STATS so insert-load races bias the surplus negative. The explicit path stays as a defensive layer with precise slot ids where servers surface them.

Keys arriving into an IMPORTING slot inflate dbsize before the slot is assigned (and reported by SLOT-STATS), so a live reshard moving 100+ keys would read as a leak once it outlasted the persistence gate. Skip the surplus path while any import is in flight; MIGRATING slots stay assigned and counted, so they need no suppression.

Feeding the import-suppressed null into the persistence gate read as recovery: a brief import reset the 30s clock and re-emitted an already-alerted leak as a duplicate WARNING after the import ended. Skip the poll before the gate instead (same contract as a failed SLOT-STATS read), preserving firstSeen/active state across the import.

Coercing a failed getDbSize to null let the poll reach the persistence gate with findings the surplus path could never produce, so the gate read the blind poll as recovery — clearing firstSeen/active and duplicating an already-alerted leak, or resetting the 30s clock under repeated blips. Skip the poll instead, matching the import and SLOT-STATS observation-gap contract.

- compress the orphaned slot set to range notation (injective on sets) - digest with sha1 past 64 chars so thousands of leaked slots cannot produce a multi-KB signature/event id

…evel - regression tests for the failed-DBSIZE gap: no duplicate WARNING after a blip, persistence clock preserved, genuine recovery still re-alerts - prettier reformat of previously unformatted spec sections

valkey#539 is a persistence-load leak, not a gossip race: the engine loads persistence files the same way under Raft (Cluster V2) and CLUSTER NODES / SLOT-STATS remain available. Hoist the topology fetch above the gossip gate (SHARDS stays gossip-only) and call the orphan detector for both modes. Raft-mode tests now assert the real invariant (no SHARDS fetch, no gossip events) instead of using the topology fetch as a proxy.

…dead path - Remove the explicit per-slot detection path: CLUSTER SLOT-STATS reports only slots assigned to the node, verified on Valkey 8.1.6, so the path could never fire on a real server. Drops the slot-set signature, its range compression and sha1 digest, and the tests covering it - Read DBSIZE either side of SLOT-STATS and use the low-water value, so keys deleted or expired between the two round trips can no longer inflate the surplus — the previous single pre-read was biased positive by deletes and would confirm a leak on a healthy TTL cache - Cover the churn cases the bias produced, at both detector and service level, including the exact-leak and draining-keyspace shapes

- Probe at most once per 15s instead of every poll: the probe costs two DBSIZEs plus a SLOT-STATS reply carrying every slot on the node, and ANOMALY_POLL_INTERVAL_MS defaults to 1s, for a leak that is permanent once loaded - Treat a skipped poll as an observation gap like the other early returns, so polls between probes cannot read as recovery

jamby77 · 2026-08-13T06:54:23Z

Both blockers resolved. Blocker 1 went the way you suspected it might not — I ran it on a real node rather than reasoning about the kvstore.

Blocker 1 — the load-bearing assumption holds; the explicit path is dead and is gone.

Valkey 8.1.6, 3-node cluster, 5 keys deliberately leaked into slot 941 on a node that does not own it:

probe	result
`CLUSTER COUNTKEYSINSLOT 941`	`5` — keys are resident
`CLUSTER GETKEYSINSLOT 941 10`	lists all 5
`CLUSTER SLOT-STATS SLOTSRANGE 941 941`	empty
`CLUSTER SLOT-STATS ORDERBY key-count LIMIT 16384`	slot 941 absent
`DBSIZE` vs `sum(SLOT-STATS key-count)`	`8` vs `3` → surplus `5`

Tested both shapes: slot unassigned cluster-wide (CLUSTER DELSLOTS), and slot owned by another node while the keys stay here (the realistic post-migration leak). Identical outcome — so the per-slot kvstore does hold the keys, but SLOT-STATS won't report a slot the node isn't assigned. The explicit path, its ~10 tests, the range compression and the sha1 digest are all removed; the detector is 205 → 129 lines.

Blocker 2 — fixed at the source rather than by raising the floor.

DBSIZE is now read on both sides of the SLOT-STATS read and the low-water value is used. With owned-slot keys O(t) and leaked keys L the surplus becomes min(O(t0), O(t2)) + L - O(t1), which is at most L for any monotone change in O, and exactly L on a steady keyspace. So deletes and expiries can no longer inflate it — and neither can inserts, which the old read ordering only happened to protect against.

I went this way rather than your first suggestion because gating on the surplus growing would break the true positive: a leak is constant, not growing, so a growth gate would never fire on the real thing. The low-water read kills the false positive without touching the true one. The 100-key floor stays, now only absorbing non-monotone dips between the bracketing reads rather than the whole churn rate.

On your parenthetical — checked, and they do count identically. 200 keys expired with DEBUG SET-ACTIVE-EXPIRE 0 and never accessed: DBSIZE 250, sum(SLOT-STATS) 250, surplus 0. No systematic surplus from pending-expiry accounting.

One caveat worth stating: I could not reproduce the read-skew race in a shell harness — the window is milliseconds and the Lua-driven churn kept finishing between my two valkey-cli round trips. The mechanism is arithmetic and your analysis of it was right; it's covered deterministically in tests at both detector and service level instead, including the case where the naive surplus clears the floor but the bracketed one doesn't.

Non-blocking, both taken:

Design deviation is now an explicit "Deliberate deviation from Detector: orphaned keys in unowned cluster slots after persistence load #367" section in the PR body with the evidence table, rather than buried in a commit message.
Cost: the probe now runs at most once per 15s instead of every poll. Worth flagging that this was worse than the review said — ANOMALY_POLL_INTERVAL_MS defaults to 1s, so a 16384-entry SLOT-STATS reply was going out every second per cluster primary. Throttled skips are observation gaps like the others, so they can't read as recovery.

534/534 green in the anomaly suite; whole backend suite green apart from the pre-existing license.service failure on master.