fix: reorder type checks after stale validation and fix zset store destination handling#261
Open
guozhihao-224 wants to merge 3 commits intomainfrom
Open
fix: reorder type checks after stale validation and fix zset store destination handling#261guozhihao-224 wants to merge 3 commits intomainfrom
guozhihao-224 wants to merge 3 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughReworks staleness gating across Redis set, string, and zset operations: replaces many Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage/src/redis_zsets.rs (1)
1143-1160:⚠️ Potential issue | 🟠 MajorDon’t return before clearing the destination for empty intersections.
For
ZINTERSTORE, a missing or stale source means the result is empty, but the destination still needs to be overwritten. These earlyreturn Ok(0)paths skip the destination cleanup/write block entirely, so an existing destination can survive even though the command reports0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/src/redis_zsets.rs` around lines 1143 - 1160, The early returns in the ZINTERSTORE handling (the branches checking base_meta_val.is_empty() and self.is_stale(&base_meta_val) that currently do `return Ok(0)` when is_inter is true) skip the destination cleanup/write and leave an old destination intact; change these returns to instead set a flag (e.g., `empty_result = true`) or update the loop state and break out so processing continues to the common destination-write/clear logic, and then ensure the destination is overwritten/cleared and returns 0 at the end; update the branches in the function handling ZINTERSTORE (use the `is_inter` check, the `base_meta_val` empty/stale branches, and the existing destination write/cleanup code) to follow this flow so destination is always cleared when intersection yields empty.
🧹 Nitpick comments (1)
src/storage/src/redis_sets.rs (1)
802-806: Consider simplifying theis_valid()check.After the stale check at line 732, if
dest_existsis true and the destination was stale,initial_meta_value()was called which resets the metadata (including count to 0 and clearing expiration). Theis_valid()check here may be redundant since:
- If dest was stale →
initial_meta_value()resets etime →is_valid()returns true, count is 0 →0 + 1 = 1- If dest was not stale →
is_valid()returns true, count is existing value →count + 1The current code works correctly, but the condition could potentially be simplified to track whether the destination was stale explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/src/redis_sets.rs` around lines 802 - 806, The is_valid() check after handling a stale destination is redundant; instead, track whether you reset the destination metadata (e.g., via a boolean like dest_was_stale) when calling initial_meta_value() and then unconditionally set the new count: if dest_exists { if dest_was_stale { dest_meta.set_count(1) } else { dest_meta.set_count(dest_meta.count() + 1) } } else { dest_meta.set_count(1) } — update the logic around dest_meta, dest_exists, initial_meta_value(), is_valid(), set_count(), and count() so the code no longer relies on is_valid() to distinguish stale vs. fresh state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/storage/src/redis_zsets.rs`:
- Around line 82-93: Parsed zset-meta is being constructed before we verify the
live key type, which causes non-zset live keys to error during meta parsing;
move the call to ParsedZSetsMetaValue::new(...) so it occurs after
check_type(&base_meta_val, DataType::ZSet) and the is_stale check, i.e., first
call self.is_stale(&base_meta_val) and self.check_type(...) to ensure WRONGTYPE
is returned for live non-zset keys, then parse with
ParsedZSetsMetaValue::new(...); apply the same reorder to the zadd code path so
zadd also performs type and staleness checks before invoking
ParsedZSetsMetaValue::new.
In `@src/storage/tests/redis_set_test.rs`:
- Line 170: Formatting mismatch on the call to redis.set(key, b"x").expect("set
failed"); — run rustfmt to fix the whitespace/formatting by running `cargo fmt
--package storage` (or apply rustfmt to the test file) so the line conforms to
project formatting; target the test containing the redis.set call to reformat
and then re-run tests.
In `@src/storage/tests/redis_string_test.rs`:
- Line 350: Formatting mismatch on the `.hset(hash_key, b"field", b"value")`
call in the Redis string tests: run rustfmt to fix it (e.g., run `cargo fmt
--package storage`) or apply rustfmt formatting to the test containing the
`.hset` invocation so the line matches project formatting; ensure the `hset`
call and surrounding test are formatted by rustfmt and commit the updated file.
---
Outside diff comments:
In `@src/storage/src/redis_zsets.rs`:
- Around line 1143-1160: The early returns in the ZINTERSTORE handling (the
branches checking base_meta_val.is_empty() and self.is_stale(&base_meta_val)
that currently do `return Ok(0)` when is_inter is true) skip the destination
cleanup/write and leave an old destination intact; change these returns to
instead set a flag (e.g., `empty_result = true`) or update the loop state and
break out so processing continues to the common destination-write/clear logic,
and then ensure the destination is overwritten/cleared and returns 0 at the end;
update the branches in the function handling ZINTERSTORE (use the `is_inter`
check, the `base_meta_val` empty/stale branches, and the existing destination
write/cleanup code) to follow this flow so destination is always cleared when
intersection yields empty.
---
Nitpick comments:
In `@src/storage/src/redis_sets.rs`:
- Around line 802-806: The is_valid() check after handling a stale destination
is redundant; instead, track whether you reset the destination metadata (e.g.,
via a boolean like dest_was_stale) when calling initial_meta_value() and then
unconditionally set the new count: if dest_exists { if dest_was_stale {
dest_meta.set_count(1) } else { dest_meta.set_count(dest_meta.count() + 1) } }
else { dest_meta.set_count(1) } — update the logic around dest_meta,
dest_exists, initial_meta_value(), is_valid(), set_count(), and count() so the
code no longer relies on is_valid() to distinguish stale vs. fresh state.
🪄 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: a00e8307-6d0e-4a7b-88c0-5c1aeda43102
📒 Files selected for processing (6)
src/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rssrc/storage/tests/redis_set_test.rssrc/storage/tests/redis_string_test.rssrc/storage/tests/redis_zset_test.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/storage/src/redis_zsets.rs (1)
82-93:⚠️ Potential issue | 🟠 Major
zaddstill parses ZSET metadata before stale/type gating.Line 82 constructs
ParsedZSetsMetaValuebefore the Line 87 stale check and the Line 91 type check, so a non-ZSET key can still fail meta parsing before this path returnsWRONGTYPEor treats an expired key as absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/src/redis_zsets.rs` around lines 82 - 93, ParsedZSetsMetaValue is being constructed before gating by is_stale and check_type in the zadd path, which can cause meta parsing to error for non-ZSET or expired keys; change the order in the zadd implementation so you first call self.is_stale(&base_meta_val)? and if not stale call self.check_type(&base_meta_val, DataType::ZSet)? before constructing ParsedZSetsMetaValue, and for the stale branch use ParsedZSetsMetaValue::new only if you need parsed fields (or use parsed_zset_meta.initial_meta_value() only after safe parsing), ensuring any use of parsed_zset_meta happens after the type/stale checks to avoid premature parsing errors (look for ParsedZSetsMetaValue, is_stale, check_type, and the zadd function to update).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/storage/src/redis_zsets.rs`:
- Around line 1154-1159: In the zinterstore/zunionstore scan logic, remove the
early return Ok(0) when encountering a stale or missing source (the branch that
currently returns for is_inter) and instead set a local flag (e.g., result_empty
= true) and fall through to the existing destination-delete/write path so the
destination lock acquisition and cleanup code (the destination lock and final
cleanup block) still run; apply the same change to the "missing-key" branch so
both cases mark the result as empty and continue rather than returning
prematurely, then have the final write/delete logic check result_empty and
perform the delete/empty-write behavior before returning 0.
- Around line 1225-1227: The code currently calls
self.check_type(&dest_meta_val, DataType::ZSet) and rejects non-ZSET destination
keys, but Redis semantics overwrite any existing type; remove the unconditional
check_type call and instead only perform ZSET-specific physical cleanup when the
existing destination is actually a ZSET: check if dest_meta_val is non-empty and
not stale (using self.is_stale), then attempt to parse
ParsedZSetsMetaValue::new(&dest_meta_val[..]) and if parsing succeeds treat it
as an existing ZSET and run the cleanup logic, otherwise skip cleanup and
proceed with the overwrite unconditionally (do not return WRONGTYPE).
---
Duplicate comments:
In `@src/storage/src/redis_zsets.rs`:
- Around line 82-93: ParsedZSetsMetaValue is being constructed before gating by
is_stale and check_type in the zadd path, which can cause meta parsing to error
for non-ZSET or expired keys; change the order in the zadd implementation so you
first call self.is_stale(&base_meta_val)? and if not stale call
self.check_type(&base_meta_val, DataType::ZSet)? before constructing
ParsedZSetsMetaValue, and for the stale branch use ParsedZSetsMetaValue::new
only if you need parsed fields (or use parsed_zset_meta.initial_meta_value()
only after safe parsing), ensuring any use of parsed_zset_meta happens after the
type/stale checks to avoid premature parsing errors (look for
ParsedZSetsMetaValue, is_stale, check_type, and the zadd function to update).
🪄 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: e3a6d039-60e7-49d2-9c95-3e2b7343a685
📒 Files selected for processing (4)
src/storage/src/redis_zsets.rssrc/storage/tests/redis_set_test.rssrc/storage/tests/redis_string_test.rssrc/storage/tests/redis_zset_test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/storage/tests/redis_zset_test.rs
- src/storage/tests/redis_string_test.rs
- Move ParsedZSetsMetaValue::new() after is_stale/check_type in zadd to prevent wrong error for non-ZSet keys - Replace early return Ok(0) in zinterstore/zunionstore with break + flag so destination cleanup always runs - Remove check_type on destination key in zinterstore/zunionstore since Redis overwrites destination regardless of its current type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
guozhihao-224
changed the title
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
1 participant
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
Reorder type checks to occur after stale validation across set, string, and sorted-set operations, ensuring correct
WRONGTYPEresponses for live non-matching keys while treating stale keys as non-existent.Key changes
is_valid()with centralizedself.is_stale()gating, and movecheck_type()to after staleness validation. This ensures stale keys are treated as absent before any type check runs.ParsedZSetsMetaValue::new()afteris_stale()/check_type()to prevent wrong error for non-ZSet keys.return Ok(0)with a flag + break so the destination lock and cleanup logic always executes, preventing stale destination data from surviving.check_typeon destination key — per Redis semantics, store commands overwrite the destination regardless of its current type.Nonewithout attempting UTF-8 decode.Tests added
scardreturnsWRONGTYPEfor a live string keyzcardreturnsWRONGTYPEfor a live string keymgetreturnsNonefor wrong-type and missing keys