fix(commands): DBSIZE + INFO keyspace count logical keys under disk-offload (#355) - #362
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughDBSIZE and INFO keyspace reporting now count logical keys across hot RAM and disk-offloaded cold storage, avoiding hot/cold double-counting. Command, coordinator, and shard paths use the new count, with unit and end-to-end tests covering spill, restart, clearing, and multi-shard aggregation. ChangesLogical key counting
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/storage/db.rs`:
- Around line 1394-1405: Replace the per-call cold_index scan in logical_len
with maintained per-shard counters for cold entries and hot/cold overlap, so
DBSIZE and INFO remain O(1). Encapsulate all cold_index mutations behind methods
that update these counters whenever entries are added, removed, or hot keys
change, and have logical_len compute the result from the counters without
iterating the index.
- Around line 1377-1406: Split the oversized Rust modules without changing
behavior: in src/storage/db.rs lines 1377-1406, move tiered-counting and storage
responsibilities, including logical_len, into focused modules; in
src/command/key.rs lines 1373-1454, divide command groups into directory modules
while retaining tests in mod.rs; in src/shard/coordinator.rs lines 1807-1809,
extract keyspace/DBSIZE coordination; and in src/shard/spsc_handler.rs lines
2027-2038, extract message-family handlers from the dispatch function. Ensure
every resulting Rust file remains at or below 1500 lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee81fae5-ad94-486b-a2cd-52d5d3636b67
📒 Files selected for processing (7)
CHANGELOG.mdsrc/command/connection.rssrc/command/key.rssrc/shard/coordinator.rssrc/shard/spsc_handler.rssrc/storage/db.rstests/dbsize_offload_logical.rs
…rops The restart leg of dbsize_counts_spilled_keys_and_survives_restart asserted exact equality between the pre-kill and post-recovery logical counts. CI failed it consistently (recovered=399 vs live=400, 3 retries): the instance sits at its maxmemory cap, so the background eviction tick may legally plain-drop a victim between the live snapshot and the kill -9 — Wave A records the drop as a reason-DEL in the AOF, so it survives replay as deleted. Exact equality is unattainable by design. Relax to a bounded band: recovered <= live (restart must never invent keys) and recovered >= live - 10 (only a handful of tick drops are legal). The existing floor assertion (size > N_KEYS/2) still guards against vacuous majority-drop runs. refs #362 author: Tin Dang <tindang.ht97@gmail.com>
…ffload (#355) The 2026-07-16 G2 re-run wrote ~164K distinct keys and DBSIZE answered 24,275 — the resident set only. A spilled-but-readable key is still a key (Redis parity); under-reporting by ~86% breaks operator capacity math and any tooling that trusts DBSIZE. Fix: `Database::logical_len()` = hot + cold, overlap counted once. The hot∩cold overlap is transient but real — a fresh SET over a cold-only key lands on `set()`'s `Inserted` arm, which deliberately leaves the cold shadow (removing it there would defeat restart-as-cold: every first replayed write is `Inserted` — see the AOF-replay ambiguity proof in `Database::set`). The overlap is subtracted with an O(cold) probe pass instead of a maintained counter: `cold_index` is a `pub` field mutated directly by the spill-completion path (counter drift risk), and INFO already tolerates the same-order `expires_count` scan per call. Wired through all five count sites: - `key::dbsize` / `key::dbsize_readonly` (single-shard dispatch) - INFO fallback `# Keyspace` section (`connection.rs`) - `KeyspaceStats` scatter handler + `coordinate_keyspace_info` local leg - `coordinate_dbsize` LOCAL leg — this one inlined a resident-only `db.len()` while its remote legs dispatched real DBSIZE commands, so a multi-shard DBSIZE mixed two definitions in one reply (found by the new e2e: INFO said 400 while DBSIZE said 339 in the same instant). Tests (red/green): - 3 unit tests (`command::key::tests::test_dbsize_*`): cold counting, no-double-count for a cold-shadowed hot key, clear() zeroes both planes. - New wire-level e2e `tests/dbsize_offload_logical.rs` (pattern: cold_collection_visibility): 400 keys × 4KiB through a 512KiB cap with real spill; DBSIZE must converge to a stabilized EXISTS ground truth (EXISTS is cold-aware and non-promoting; both counters legitimately under-read while spill batches are in flight), agree with INFO `db0:keys=`, and survive a kill-9 restart; single-shard + 4-shard legs. Known remaining parity gap (out of scope, noted on #355): SCAN / KEYS / RANDOMKEY enumerate the hot plane only (probe: 116 of 400 logical keys). Closes #355 author: Tin Dang <tindang.ht97@gmail.com>
…rops The restart leg of dbsize_counts_spilled_keys_and_survives_restart asserted exact equality between the pre-kill and post-recovery logical counts. CI failed it consistently (recovered=399 vs live=400, 3 retries): the instance sits at its maxmemory cap, so the background eviction tick may legally plain-drop a victim between the live snapshot and the kill -9 — Wave A records the drop as a reason-DEL in the AOF, so it survives replay as deleted. Exact equality is unattainable by design. Relax to a bounded band: recovered <= live (restart must never invent keys) and recovered >= live - 10 (only a handful of tick drops are legal). The existing floor assertion (size > N_KEYS/2) still guards against vacuous majority-drop runs. refs #362 author: Tin Dang <tindang.ht97@gmail.com>
…yspace Pre-merge review sweep found the embedded/non-sharded connection handler's INFO `# Keyspace` vector still used hot-only `len()`; the sharded binary (any --shards value) routes through `coordinate_keyspace_info` and was already converted, so this is reachable only via the embedded server — fixed for consistency so DBSIZE and INFO can never disagree on any code path. CHANGELOG site count updated 5 → 6. author: Tin Dang <tindang.ht97@gmail.com>
4aeaa64 to
39966c6
Compare
|
Pre-merge adversarial review (independent agent + manual verification): core fix verified sound — overlap accounting correct, coordinate_dbsize LOCAL/remote legs agree, tolerance band is directional (can only mask the documented tick-drop under-count, never an over-count). One review finding fixed in 4aeaa64: the embedded/non-sharded |
…s the positional re-sort (#368) (#379) SCAN's cursor was a positional index into a keyspace snapshot that every page re-collected and re-sorted: any insert/delete (or cold-plane spill/ promotion/TTL churn) between pages shifted positions, so a key present for the ENTIRE scan could be skipped — a violation of Redis's documented contract, hitting exactly the backup/migration-via-SCAN use case #364/ #367 exist to serve. On top of that, every page paid collect + sort O(hot+cold log) plus a second get_if_alive() lookup pass over every key, and the write path additionally probed exists() on the full keyspace per page. SCAN pages now iterate in (hash48(key), key) order and the cursor is a position in hash space (`scan_core` in src/command/key.rs): - Guarantee: a key's hash never changes, so churn cannot displace another key's position. A key present throughout the scan is returned exactly once. New red/green unit test deletes already-returned keys and inserts new ones between every page and asserts all 100 stable keys are returned (the old positional cursor fails this). - Cursor stays NUMERIC and 48-bit — the multi-shard composite cursor (upper 16 bits shard / lower 48 per-shard, coordinate_scan) and the admin scan_fanout opaque-cursor plane both work unchanged, and integer- parsing clients (redis-rs, redis-py, redis-cli --scan) stay compatible. - Cost per page: ONE walk over both planes with a bounded COUNT-min selection heap (BinaryHeap, capacity COUNT+1) — no full sort, no second lookup pass, no full-keyspace lazy-expiry probe. New Database::iter_live_keys judges liveness from the entry during iteration instead of a per-key hash lookup. - Hash-collision safety: a full page never advances the cursor past a hash whose key group might be partially selected — the trailing equal-hash group defers to the next page (COUNT is a hint, Redis parity); the all-one-hash pathological page emits the entire group and steps past it. - Hash: FNV-1a 64 truncated to 48 bits, fixed seed — deliberately NOT the tables' randomized hashers (the cursor must be stable across pages). The old exists()-per-key reclamation side effect on the SCAN write path is gone by design: active expiry (100ms cadence) owns reclamation; SCAN no longer walks the whole keyspace to do it as a side job. Follow-up (tracked in #368, open): O(COUNT) pages via a DashTable bucket-order cursor; shared cold-only live-count primitive for logical_len (#355/#362 class). Gates: - New unit tests: churn stable-key guarantee (red vs old impl), duplicate-free exact full drain, MATCH paging termination; legacy exact-COUNT assertion corrected to the documented COUNT-is-a-hint contract. 4381 lib tests green. - scan_offload_visibility (cold plane + restart) green. - Multi-shard e2e (Linux VM, 4 shards, 500 keys): redis-cli --scan drains 500/500 unique keys; --pattern "key:1*" returns exactly 111. - fmt, clippy (default + tokio,jemalloc) green. refs #368 author: Tin Dang <tindang.ht97@gmail.com> Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
2 participants
Summary
Closes #355. DBSIZE (and INFO
# Keyspace) reported the resident set only under disk-offload — the 2026-07-16 G2 re-run wrote ~164K distinct keys and DBSIZE answered 24,275 (~86% under-report). A spilled-but-readable key is still a key (Redis parity); the gap breaks operator capacity math and any tooling that trusts DBSIZE.Fix
Database::logical_len()= hot + cold, overlap counted once.set()'sInsertedarm, which deliberately leaves the cold shadow (removing it there would defeat restart-as-cold — every first replayed write isInserted; see the AOF-replay ambiguity proof inDatabase::set).cold_indexis apubfield mutated directly by the spill-completion path, so an incremental counter has unfenceable drift risk; INFO already tolerates the same-orderexpires_count()scan per call. Zero hot-path cost.key::dbsize,key::dbsize_readonly, the INFO fallback keyspace section, theKeyspaceStatsscatter handler, andcoordinate_dbsize's local leg — which inlined a resident-onlydb.len()while its remote legs dispatched real DBSIZE commands, so a multi-shard DBSIZE mixed two definitions in a single reply (the new e2e caught INFO saying 400 while DBSIZE said 339 in the same instant).Tests (red/green)
clear()zeroes both planes.tests/dbsize_offload_logical.rs(pattern:cold_collection_visibility): 400 keys × 4KiB through a 512KiB cap with real spill. DBSIZE must converge to a stabilized EXISTS ground truth (EXISTS is cold-aware and non-promoting; both counters legitimately under-read while spill batches are in flight — a key lives in neither plane between evict-time hot removal and completion-apply), agree withINFO db0:keys=, and survive a kill-9 restart. Single-shard + 4-shard legs.Gates
cargo clippy -- -D warningsboth configs (incl.--tests),cargo fmt --check, 4342 lib tests greenKnown remaining parity gap (out of scope)
SCAN / KEYS / RANDOMKEY still enumerate the hot plane only (probe: 116 of 400 logical keys visible to SCAN). Needs cold-aware cursor semantics — noted on #355 for follow-up tracking.
Summary by CodeRabbit
DBSIZEandINFO # Keyspacecounts when data is offloaded to disk.SCAN,KEYS, andRANDOMKEYcontinue to enumerate only in-memory keys.