fix(storage): SCAN/KEYS/RANDOMKEY enumerate the cold plane under disk-offload (#364) - #367
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: 42 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 (10)
📝 WalkthroughWalkthroughDisk-offloaded key enumeration now combines hot entries with live cold-only keys, deduplicates overlaps, applies filters, and includes cold keys in ChangesCold-tier enumeration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant KeyCommands
participant Database
participant ColdIndex
Client->>KeyCommands: SCAN or KEYS
KeyCommands->>Database: collect hot keys
KeyCommands->>Database: request cold_only_keys
Database->>ColdIndex: filter live, non-shadowed keys
KeyCommands-->>Client: return filtered union
Possibly related issues
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/command/key.rs (1)
917-921: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not count TTL-expired cold entries as unlinked.
remove_counting_coldremoves any indexed cold entry, soUNLINK expired-cold-keyreturns1even though the key is logically absent. Check cachedttl_msbefore incrementing the count while still reclaiming the stale index entry.🤖 Prompt for 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. In `@src/command/key.rs` around lines 917 - 921, Update the counting branch around remove_counting_cold so the cached ttl_ms is checked before incrementing count: only count the key as removed when it is not TTL-expired, while still calling remove_counting_cold to reclaim stale cold index entries.
🤖 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/command/key.rs`:
- Around line 1015-1029: Replace full keyspace materialization and sorting in
the SCAN implementation around src/command/key.rs:1015-1029 with resumable
per-plane state or bounded shard/index iteration, preserving cursor pagination
without rebuilding all hot and cold keys per page. Apply the same bounded
iteration strategy to readonly SCAN at src/command/key.rs:1255-1264. Update
RANDOMKEY sampling at src/storage/db.rs:1424-1436 to use reservoir sampling or
direct randomized selection without cloning every key, and avoid unnecessary
hot-path allocations and clones at all three sites.
---
Outside diff comments:
In `@src/command/key.rs`:
- Around line 917-921: Update the counting branch around remove_counting_cold so
the cached ttl_ms is checked before incrementing count: only count the key as
removed when it is not TTL-expired, while still calling remove_counting_cold to
reclaim stale cold index entries.
🪄 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: 720ec7cc-de76-4279-9983-988444254055
📒 Files selected for processing (10)
CHANGELOG.mdsrc/command/key.rssrc/persistence/kv_page.rssrc/shard/persistence_tick.rssrc/storage/db.rssrc/storage/eviction.rssrc/storage/tiered/cold_index.rssrc/storage/tiered/kv_spill.rssrc/storage/tiered/spill_thread.rstests/scan_offload_visibility.rs
fc52ec3 to
cd5a672
Compare
|
Pre-merge adversarial review (independent agent): MERGE verdict, no blockers. Partition claim verified by hand (byte-lexicographic CompactKey order is plane-independent; exists() only mutates the hot plane so a lazy-expired shadow correctly reveals the cold entry exactly once). value_type verified as a true on-disk field at every construction site incl. rebuild. Pre-existing follow-ups filed as #368 (SCAN positional cursor lacks the stable-key guarantee; O(hot+cold) re-sort per page at 10×-RAM scale). |
cd5a672 to
823a1bd
Compare
…-offload Closes #364. With disk-offload enabled, cold-only keys (spilled by eviction, no in-RAM Entry) were readable via GET/EXISTS/DBSIZE but invisible to enumeration: a 4-shard instance holding 400 logical keys returned only 116 from `redis-cli --scan`. Any SCAN consumer doing migration/backup silently lost spilled keys — the same operator-trust class as the #355 DBSIZE gap. Fix: enumerate the union of the two planes, partitioned so no dedup pass is needed (hot-visible = live in-RAM entry; cold-visible = alive in the in-RAM ColdIndex AND not shadowed by a live hot entry): - Database::cold_only_keys(now_ms): pure in-RAM iterator over the cold index — TTL judged from the cached ColdLocation::ttl_ms, hot-shadow judged from the DashTable; no disk I/O, no promotion. - SCAN + SCAN readonly-twin: cold keys join the sorted cursor space with a plane tag; TYPE filter judges cold keys from a new ColdLocation::value_type cache (same cached-copy contract as ttl_ms: populated at spill time via SpillRequest→SpillCompletionEntry and the sync eviction path, re-derived from on-disk pages by ColdIndex::rebuild_from_manifest after restart; fits existing struct padding — zero cold-index RAM growth; no on-disk format change). - KEYS + readonly twin: cold loop appended after the hot loop. - RANDOMKEY (both twins via Database::random_key): samples the union, so an all-spilled database no longer answers "empty". Tests: 11 red/green unit tests (plane partition, both-planes exactly once, TTL-expired cold skipped, MATCH/COUNT paging across the plane boundary, TYPE from the index, RANDOMKEY on all-cold) + e2e tests/scan_offload_visibility.rs (real spill at 1.5x maxmemory, full SCAN/KEYS/RANDOMKEY + TYPE sweep, then kill-9 restart proving rebuild_from_manifest re-derives value_type). The e2e uses a kill-on-drop child guard so a failed assert can never orphan the server (a prior leaked test moon spun at 667% CPU once its tmpdir was cleaned — filed as #366). author: Tin Dang <tindang.ht97@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/scan_offload_visibility.rs`:
- Around line 234-241: Update the scan/offload test setup around the HSET loop
so hashes are inserted before additional memory-pressure filler, ensuring at
least one hash becomes cold and spills. Add an explicit assertion that a
persisted cold entry has ValueType::Hash, rather than relying only on heap_files
> 0, while preserving the existing TYPE assertions.
🪄 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: f2837606-85e0-4562-94e9-b3d1d53c9c69
📒 Files selected for processing (10)
CHANGELOG.mdsrc/command/key.rssrc/persistence/kv_page.rssrc/shard/persistence_tick.rssrc/storage/db.rssrc/storage/eviction.rssrc/storage/tiered/cold_index.rssrc/storage/tiered/kv_spill.rssrc/storage/tiered/spill_thread.rstests/scan_offload_visibility.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/shard/persistence_tick.rs
- src/persistence/kv_page.rs
- CHANGELOG.md
- src/storage/tiered/spill_thread.rs
- src/storage/eviction.rs
- src/storage/db.rs
- src/storage/tiered/kv_spill.rs
- src/command/key.rs
| for i in 0..HASH_COUNT { | ||
| let key = format!("hobj:{i}"); | ||
| let reply = redis_cli(port, &["HSET", &key, "f", "v"]); | ||
| assert!( | ||
| reply.as_deref().is_some_and(|r| r.parse::<u64>().is_ok()), | ||
| "HSET {key} must succeed, got: {reply:?}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure the TYPE test actually spills at least one hash.
Hashes are inserted only after all memory-pressure filler, making them the newest and smallest entries. heap_files > 0 proves only that something—likely strings—spilled, so both TYPE assertions can pass without exercising cold value_type or manifest reconstruction.
Write hashes before continued memory pressure and explicitly verify that a persisted cold entry has ValueType::Hash.
🤖 Prompt for 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.
In `@tests/scan_offload_visibility.rs` around lines 234 - 241, Update the
scan/offload test setup around the HSET loop so hashes are inserted before
additional memory-pressure filler, ensuring at least one hash becomes cold and
spills. Add an explicit assertion that a persisted cold entry has
ValueType::Hash, rather than relying only on heap_files > 0, while preserving
the existing TYPE assertions.
…n-free RANDOMKEY Review-response fixes on the #364 branch (verified against current code before applying): - remove_counting_cold: a cold entry already TTL-expired (judged from the cached ColdLocation::ttl_ms, no disk read) is still reclaimed but no longer COUNTED — DEL/UNLINK of a logically-expired key answers 0 (Redis parity). Red/green pinned by del_of_ttl_expired_cold_key_answers_zero_but_reclaims + unlink_of_alive_cold_key_still_counts. - Database::random_key: two passes (count, then walk to the selected position) so only the winning key is cloned — the previous version materialized a Bytes copy of EVERY live key per call. Semantics unchanged (same time-based selection); safe because &self spans both passes on the single-threaded shard. Skipped (with reason) from the same review: replacing SCAN's per-page collect+sort with a resumable per-plane cursor — that is the issue #368 redesign (positional cursors lack the stable-key guarantee regardless); a partial bounded-iteration change here would alter cursor semantics without delivering the guarantee. author: Tin Dang <tindang.ht97@gmail.com>
823a1bd to
130a3b9
Compare
|
Review findings triaged against current code (130a3b9):
Gates re-run after the fixes: full lib suite (4367 ok), clippy default+tokio clean, scan_offload_visibility + dbsize_offload_logical e2e green. |
…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
Closes #364.
Problem
With disk-offload enabled, cold-only keys (spilled by eviction, no in-RAM
Entry) were readable via GET/EXISTS but invisible to enumeration: a 4-shard instance holding 400 logical keys returned only 116 fromredis-cli --scan. Any SCAN consumer doing migration/backup (--scan | xargs MIGRATE) silently lost spilled keys — the same operator-trust class as the #355 DBSIZE gap, arguably worse.Fix
Enumerate the union of the two planes, partitioned so no dedup pass is needed:
ColdIndex(TTL judged from the cachedColdLocation::ttl_ms) AND not shadowed by a live hot entry.A key present in both planes (hot shadow over a stale cold entry, e.g. after AOF replay) is returned exactly once; a hot-expired key above a live cold entry is classified cold.
Database::cold_only_keys(now_ms)— pure in-RAM iterator; no disk I/O, no promotion.TYPEfilter judges cold keys from a newColdLocation::value_typecache: same cached-copy contract asttl_ms(populated at spill viaSpillRequest→SpillCompletionEntryand the sync eviction path; re-derived from the on-disk pages byColdIndex::rebuild_from_manifestafter restart). The field fits existing struct padding — zero cold-index RAM growth, no on-disk format change, and no disk reads on the SCAN path (a TYPE-filtered SCAN over a large offloaded keyspace must never become a disk crawl or promotion storm).Database::random_keysamples the union; an all-spilled database no longer answers "empty".Multi-shard needs no coordinator changes:
coordinate_scan/coordinate_keysdelegate to the per-shard command functions fixed here.Gates
tests/scan_offload_visibility.rs(--ignored): real spill at ~1.5×--maxmemory(12 heap files), full SCAN loop / KEYS / RANDOMKEY /SCAN TYPE hashover 6,050 keys, then kill-9 + restart provingrebuild_from_manifestre-derivesvalue_type(post-restart TYPE-filtered SCAN green). Passed on macOS (monoio, 8.0s) and Linux VM (monoio/io_uring, 7.1s). Uses a kill-on-drop child guard so a failed assert can never orphan the server (a prior leaked test moon spun at 667% CPU once its tmpdir vanished — filed as Data-dir deleted under a running server → per-shard tick error loop burns ~100% CPU per thread, no backoff, server wedges #366).cargo testgreen on monoio (default) and runtime-tokio,jemalloc;storage::tieredunit tests green on Linux VM.-D warnings), unwrap ratchet, unsafe audit: clean.Perf note
SCAN/KEYS/RANDOMKEY are admin-plane O(n) collectors already; the cold union adds one in-RAM HashMap iteration. Nothing on the command hot path changed; the spill path change is copying one existing byte into struct padding.
Summary by CodeRabbit
SCAN,KEYS, andRANDOMKEYwith disk offload to include spilled keys exactly once while skipping TTL-expired cold entries.MATCH,COUNT, andTYPEfiltering so cold-keyTYPEresults reflect the stored value type, including after restart.UNLINKremoved-key counts for cold-only keys.TYPEfiltering, and restart behavior.