fix(command): SCAN stable-key guarantee — hash-ordered cursor replaces the positional re-sort (#368) - #379
Merged
Merged
Conversation
…s the positional re-sort (#368) 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>
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? |
📝 WalkthroughWalkthroughSCAN now uses a numeric cursor representing stable 48-bit key-hash space. Shared paging logic scans live hot and cold-only keys with bounded selection, applies filters, and is reused by read-only scans. Tests cover churn, draining, duplicates, filtering, and COUNT behavior. ChangesSCAN cursor correctness
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SCAN
participant Database
Client->>SCAN: SCAN cursor with COUNT, TYPE, MATCH
SCAN->>Database: Read live hot keys and cold-only keys
Database-->>SCAN: Candidate keys
SCAN-->>Client: Next hash cursor and filtered keys
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
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)
993-999: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBound the client-controlled heap capacity.
A request such as
SCAN 0 COUNT 9223372036854775807reachesBinaryHeap::with_capacity(count + 1), allowing an unauthenticated client to trigger an enormous allocation and potentially abort the process. Reject or clamp unreasonable COUNT hints before allocating.Proposed guard
if let Some(c) = parse_int(&args[i]) { if c > 0 { - count = c as usize; + count = (c as usize).min(MAX_SCAN_COUNT); } }Also applies to: 1062-1064
🤖 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 993 - 999, Bound the positive COUNT value parsed in the COUNT option handling and the corresponding logic around the additional referenced location before it reaches BinaryHeap::with_capacity(count + 1). Reject or clamp values above a reasonable maximum while preserving valid COUNT behavior, ensuring client-controlled input cannot request an enormous heap allocation.
🤖 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 `@CHANGELOG.md`:
- Around line 20-25: Update the CHANGELOG cost description to remove the
unconditional “one walk” claim and accurately state that full COUNT 1 pages
currently perform two complete walks due to collision-boundary handling; retain
the existing complexity and Redis-parity details.
In `@src/command/key.rs`:
- Line 1101: Update the hot-path logic around the result vectors at the
referenced locations to preallocate capacity instead of using Vec::new, sizing
each vector from the expected result count. Replace cursor to_string formatting
with the project’s allocation-conscious integer-buffer approach, preserving the
existing output and behavior.
- Around line 1090-1119: Update the paging logic around heap selection and the
full_page equal-hash branch in src/command/key.rs:1090-1119 to retain one extra
candidate or equivalent boundary evidence, so COUNT 1 pages do not rescan both
key planes unless a genuine hash-boundary collision is detected; preserve
complete equal-hash-group emission when a collision exists. Update the
complexity claim in CHANGELOG.md:20-25 to match the corrected bounded
single-walk behavior, or retain it only if the implementation now guarantees it.
- Around line 1118-1119: Update the cursor assignment after extending selected
results so the terminal hash value 0xFFFF_FFFF_FFFF produces cursor 0 instead of
incrementing to 1 << 48. Preserve the existing h_last + 1 behavior for all
smaller hashes, keeping the composite cursor within 48 bits.
---
Outside diff comments:
In `@src/command/key.rs`:
- Around line 993-999: Bound the positive COUNT value parsed in the COUNT option
handling and the corresponding logic around the additional referenced location
before it reaches BinaryHeap::with_capacity(count + 1). Reject or clamp values
above a reasonable maximum while preserving valid COUNT behavior, ensuring
client-controlled input cannot request an enormous heap allocation.
🪄 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: 5c898c5a-08b4-46c3-b529-ead7799327b9
📒 Files selected for processing (3)
CHANGELOG.mdsrc/command/key.rssrc/storage/db.rs
Comment on lines
+20
to
+25
| unchanged) and remain client-compatible. Page cost drops from | ||
| `collect + sort O(n log n)` plus a second full-table lookup pass (and, | ||
| on the write path, a full-keyspace lazy-expiry probe per page) to one | ||
| walk with a bounded COUNT-min selection heap. COUNT stays a hint (Redis | ||
| parity): a full page may defer a trailing equal-hash group to the next | ||
| page. Follow-up tracked in #368: O(COUNT)-per-page via a DashTable |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the cost claim with the implemented collision path.
The implementation currently performs two complete walks for every full COUNT 1 page, so the unconditional “one walk” claim is inaccurate until the boundary detection is fixed.
🤖 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 `@CHANGELOG.md` around lines 20 - 25, Update the CHANGELOG cost description to
remove the unconditional “one walk” claim and accurately state that full COUNT 1
pages currently perform two complete walks due to collision-boundary handling;
retain the existing complexity and Redis-parity details.
Comment on lines
+1090
to
+1119
| let full_page = heap.len() == count; | ||
| let mut selected = heap.into_sorted_vec(); | ||
| let mut next_cursor: u64 = 0; | ||
| if full_page { | ||
| #[allow(clippy::unwrap_used)] // full_page ⇒ count ≥ 1 entries | ||
| let h_last = selected.last().unwrap().0; | ||
| let h_first = selected.first().map(|e| e.0).unwrap_or(h_last); | ||
| if h_first == h_last { | ||
| // Whole page one hash value: emit the ENTIRE equal-hash group | ||
| // (second filtered pass; unreachable in practice at 48 bits) | ||
| // so the cursor may step past it. | ||
| let mut extra: Vec<(u64, CompactKey, bool)> = Vec::new(); | ||
| for key in db.iter_live_keys(now_ms) { | ||
| if scan_hash48(key.as_bytes()) == h_last | ||
| && !selected.iter().any(|(_, k, _)| k == key) | ||
| { | ||
| extra.push((h_last, key.clone(), false)); | ||
| } | ||
| } | ||
| for key in db.cold_only_keys(now_ms) { | ||
| if scan_hash48(key.as_ref()) == h_last | ||
| && !selected | ||
| .iter() | ||
| .any(|(_, k, _)| k.as_bytes() == key.as_ref()) | ||
| { | ||
| extra.push((h_last, CompactKey::from(key.as_ref()), true)); | ||
| } | ||
| } | ||
| selected.extend(extra); | ||
| next_cursor = h_last + 1; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
COUNT 1 defeats the claimed bounded single-walk paging behavior.
Because a one-entry page always has h_first == h_last, every full COUNT 1 page rescans both planes, making a complete drain quadratic.
src/command/key.rs#L1090-L1119: distinguish a genuine boundary collision without automatically rescanning every one-entry page, for example by retaining one extra candidate.CHANGELOG.md#L20-L25: update the complexity claim, or retain it only after fixing the implementation.
📍 Affects 2 files
src/command/key.rs#L1090-L1119(this comment)CHANGELOG.md#L20-L25
🤖 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 1090 - 1119, Update the paging logic around
heap selection and the full_page equal-hash branch in
src/command/key.rs:1090-1119 to retain one extra candidate or equivalent
boundary evidence, so COUNT 1 pages do not rescan both key planes unless a
genuine hash-boundary collision is detected; preserve complete equal-hash-group
emission when a collision exists. Update the complexity claim in
CHANGELOG.md:20-25 to match the corrected bounded single-walk behavior, or
retain it only if the implementation now guarantees it.
| // Whole page one hash value: emit the ENTIRE equal-hash group | ||
| // (second filtered pass; unreachable in practice at 48 bits) | ||
| // so the cursor may step past it. | ||
| let mut extra: Vec<(u64, CompactKey, bool)> = Vec::new(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Remove the newly introduced hot-path allocation patterns.
Preallocate the result vectors and format the cursor with an allocation-conscious integer buffer rather than to_string().
As per coding guidelines, src/{command,protocol,shard,io}/**/*.rs must avoid Vec::new and to_string() on hot paths.
Also applies to: 1128-1128, 1150-1154
🤖 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` at line 1101, Update the hot-path logic around the result
vectors at the referenced locations to preallocate capacity instead of using
Vec::new, sizing each vector from the expected result count. Replace cursor
to_string formatting with the project’s allocation-conscious integer-buffer
approach, preserving the existing output and behavior.
Source: Coding guidelines
Comment on lines
+1118
to
+1119
| selected.extend(extra); | ||
| next_cursor = h_last + 1; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the terminal cursor within 48 bits.
When h_last == 0xFFFF_FFFF_FFFF, adding one returns 1 << 48, violating the composite cursor contract. Since no greater 48-bit hash exists after emitting the entire group, return cursor 0.
Proposed fix
selected.extend(extra);
- next_cursor = h_last + 1;
+ next_cursor = if h_last == 0x0000_FFFF_FFFF_FFFF {
+ 0
+ } else {
+ h_last + 1
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| selected.extend(extra); | |
| next_cursor = h_last + 1; | |
| selected.extend(extra); | |
| next_cursor = if h_last == 0x0000_FFFF_FFFF_FFFF { | |
| 0 | |
| } else { | |
| h_last + 1 | |
| }; |
🤖 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 1118 - 1119, Update the cursor assignment
after extending selected results so the terminal hash value 0xFFFF_FFFF_FFFF
produces cursor 0 instead of incrementing to 1 << 48. Preserve the existing
h_last + 1 behavior for all smaller hashes, keeping the composite cursor within
48 bits.
TinDang97
added a commit
that referenced
this pull request
Jul 17, 2026
…th invariant Adversarial-review follow-ups on the O(COUNT) page walk: - SCAN/SCAN-readonly now mask the parsed cursor to the low 48 bits. Legitimate resumed cursors always fit (multi-shard composites are unpacked by coordinate_scan before dispatch), but at --shards 1 a client-supplied out-of-range cursor (e.g. a composite cursor replayed from a multi-shard server, or garbage) previously filtered out every key — h48 is always < 2^48 — and returned an empty page with cursor 0: a false "scan complete" on a non-empty keyspace. Pre-existing behavior (same comparison since PR #379), closed here because this PR owns the cursor-semantics surface. Red/green: scan_out_of_range_cursor_clamps_to_hash_space. - debug_assert in Database::scan_hot_page that the DashTable directory depth is <= 48 (new DashTable::directory_depth accessor). The h64→h48 bridge and the "equal-h48 group never straddles a page" guarantee are load-bearing on this; depth > 48 needs a 2^48-entry directory — unreachable in practice, but now asserted rather than assumed. author: Tin Dang <tindang.ht97@gmail.com>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 17, 2026
…gment walk (#368) (#380) * perf(storage): SCAN hot-plane pages are true O(COUNT) — hash-range segment walk (#368) Completes the second half of #368. PR #379 made the SCAN cursor a position in stable hash space (fixing the stable-key guarantee); this change makes each page's hot-plane cost independent of keyspace size. The cursor hash becomes the DashTable's own fixed-seed xxh64 key hash truncated to its top 48 bits. Because the extendible-hashing directory is indexed by the hash's top `global_depth` bits, ascending directory order IS ascending hash order and directory entry d covers exactly the hash range [d << (64-D), (d+1) << (64-D)) — segments are range-partitioned in hash space. The new `DashTable::hash_page` starts at the segment covering the cursor and walks segments in ascending range order, stopping at the first segment boundary once COUNT entries are collected: later segments can only hold larger hashes, so the page is complete without touching the rest of the table. A segment with local_depth < global_depth occupies a contiguous directory run, so alias-dedup is a consecutive store-index comparison. Split/merge/directory-doubling between pages stays safe by construction: the cursor is a position in hash space, and structural churn only changes WHICH segment covers that position, never the set of keys at or above it. Equal-hash groups can never straddle a page (equal top-48 bits => same segment; pages are whole-segment granular), which preserves the PR #379 collision boundary rule unchanged. `Database::scan_hot_page` wraps the walk (maps 64-bit table hashes to the 48-bit cursor space via >> 16, judges liveness from the entry during iteration), and `scan_core` feeds its selection heap from that pruned page instead of a full `iter_live_keys` walk. The pathological all-one-hash second pass now rescans only the already-fetched page. The cold plane keeps its filtered in-RAM index walk (bounded by spilled-key count); ordered cold-side paging remains a follow-up. Red/green coverage: new DashTable unit tests (empty-table terminal page, alive-filter + more-flag, 4000-key drain in (hash, key) order under forced split churn between pages) plus the existing six SCAN semantics tests (churn stability, exact duplicate-free drain, MATCH paging, cold-plane enumeration x2, COUNT-as-hint) all green on the new path. Verified on Linux VM (4 shards, real server): 500/500 keys drained in 12 pages, MATCH key:1* = 111/111; scaling proof — 500 SCAN pages at COUNT 100 take 0.10s on a 10k-key DB and 0.11s on a 1,000,000-key DB (flat per-page cost across a 100x keyspace; the previous full-walk design re-hashed every live key on every page). SCAN cursors from before this change are invalidated (cursor hash function changed); cursors are documented as ephemeral — restart scans at 0. Multi-shard composite packing (upper 16 shard | lower 48 per-shard) is unchanged. Refs #368 author: Tin Dang <tindang.ht97@gmail.com> * fix(command): clamp SCAN cursor to 48-bit hash space + assert the depth invariant Adversarial-review follow-ups on the O(COUNT) page walk: - SCAN/SCAN-readonly now mask the parsed cursor to the low 48 bits. Legitimate resumed cursors always fit (multi-shard composites are unpacked by coordinate_scan before dispatch), but at --shards 1 a client-supplied out-of-range cursor (e.g. a composite cursor replayed from a multi-shard server, or garbage) previously filtered out every key — h48 is always < 2^48 — and returned an empty page with cursor 0: a false "scan complete" on a non-empty keyspace. Pre-existing behavior (same comparison since PR #379), closed here because this PR owns the cursor-semantics surface. Red/green: scan_out_of_range_cursor_clamps_to_hash_space. - debug_assert in Database::scan_hot_page that the DashTable directory depth is <= 48 (new DashTable::directory_depth accessor). The h64→h48 bridge and the "equal-h48 group never straddles a page" guarantee are load-bearing on this; depth > 48 needs a 2^48-entry directory — unreachable in practice, but now asserted rather than assumed. author: Tin Dang <tindang.ht97@gmail.com> --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the semantics half of #368: SCAN's cursor was a positional index into a per-page re-collected+re-sorted snapshot, so churn between pages could skip keys that existed for the entire scan — violating Redis's documented contract on the exact backup/migration path #364/#367 serve.
Pages now iterate in stable 48-bit key-hash order; the cursor is a position in hash space. A key's hash never changes, so churn cannot displace it — a key present throughout the scan is returned exactly once.
Key properties:
coordinate_scan) and the adminscan_fanoutopaque-cursor plane work unchanged.O(n log n)sort, no second per-key lookup pass, no full-keyspace lazy-expiry probe per page (newDatabase::iter_live_keysjudges liveness during iteration). Reclamation is active-expiry's job (100ms cadence), not SCAN's.Follow-up left open in #368: true O(COUNT) pages via a DashTable bucket-order cursor + shared cold live-count primitive for
logical_len.Testing
scan_offload_visibility(cold plane + restart) green; 4381 lib tests; fmt + clippy both feature sets.redis-cli --scandrains 500/500 unique;--pattern "key:1*"returns exactly 111.refs #368
Summary by CodeRabbit
Bug Fixes
SCANreliability during key insertions, deletions, and expiration activity.COUNTand filtering options.Documentation
SCANcorrectness improvements to the unreleased changelog.