Code Review Bench PR #14637 - Fix adjacent slot range behavior in ASM operations #2

ketkarameya · 2026-02-10T22:04:42Z

Code Review Bench PR redis#14637 - Fix adjacent slot range behavior in ASM operations

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#2

Fix adjacent slot range behavior in ASM operations

Summary by Gitar

Slot range normalization:
- Added slotRangeArraySortAndMerge() to merge adjacent ranges (e.g., 1000-2000 + 2001-3000 → 1000-3000)
Memory management fix:
- Replaced addReplyErrorSds() with sdsfree() in parseSlotRangesOrReply() to prevent leak
Assignment bug fix:
- Changed += to = in asmActiveTrimStart() to prevent key count accumulation
Enhanced logging:
- Added key counts to import/migrate completion messages via asmCountKeysInSlots()

_{This will update automatically on new commits.}

Co-authored-by: Yuan Wang <yuan.wang@redis.com>

gitar-bot-staging · 2026-02-10T22:13:27Z

Code Review ⚠️ Changes requested 0 resolved / 3 findings

Good fix for adjacent slot range handling, but slotRangeArrayIsEqual has a logic bug (compares num_ranges before merging) and parseSlotRangesOrReply silently drops validation errors instead of replying to the client.

⚠️

Bug: slotRangeArrayIsEqual checks count before merging

📄 src/cluster.c:1876-1880

The num_ranges comparison at line 1877 happens before slotRangeArraySortAndMerge() is called at lines 1879-1880. This means two slot range arrays that represent the same set of slots but have different pre-merge num_ranges will incorrectly be reported as not equal.

For example:

Array A: {7000-7001, 7002-7003} → num_ranges = 2, merges to {7000-7003} (num_ranges = 1)
Array B: {7000-7003} → num_ranges = 1

slotRangeArrayIsEqual(A, B) would return 0 at line 1877 before ever merging, even though both arrays represent slots 7000-7003.

The fix is to move the num_ranges comparison after both slotRangeArraySortAndMerge() calls.

Suggested fix

int slotRangeArrayIsEqual(slotRangeArray *slots1, slotRangeArray *slots2) {
    slotRangeArraySortAndMerge(slots1);
    slotRangeArraySortAndMerge(slots2);

    if (slots1->num_ranges != slots2->num_ranges) return 0;

    for (int i = 0; i < slots1->num_ranges; i++) {

⚠️

Bug: Validation error silently dropped, no reply sent to client

📄 src/cluster.c:2014-2018

In parseSlotRangesOrReply, the validation error message from slotRangeArrayNormalizeAndValidate() is silently freed at line 2016 without sending any error reply to the client. The old code was:

addReplyErrorSds(c, err);  // sends error AND takes ownership of err

The new code is:

sdsfree(err);  // silently discards error message

Since the function name convention is *OrReply (meaning it replies on error), and callers like clusterMigrationCommandImport simply return when parseSlotRangesOrReply returns NULL (trusting that an error was already sent), the client will receive no response at all. This will cause the client connection to hang or timeout.

The fix should restore the error reply to the client. Note that addReplyErrorSds takes ownership of the sds string, so sdsfree(err) should be removed when using it.

Suggested fix

    if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) {
        addReplyErrorSds(c, err);
        slotRangeArrayFree(slots);
        return NULL;
    }

💡 Quality: Unused variable total_ranges in asmCountKeysInSlots

📄 src/cluster_asm.c:1016

The variable total_ranges is assigned at line 1016 (int total_ranges = slots->num_ranges;) but never used anywhere in the function. This will generate a compiler warning with -Wunused-variable (which Redis builds with). It should be removed.

Suggested fix

    unsigned long long key_count = 0;
    for (int i = 0; i < slots->num_ranges; i++) {

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Appended technical summary with 4 bullet points covering slot range normalization, memory management fix, assignment bug fix, and enhanced logging

_{5 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

gitar-bot-staging · 2026-02-10T22:15:15Z

src/cluster.c

 int slotRangeArrayIsEqual(slotRangeArray *slots1, slotRangeArray *slots2) {
    if (slots1->num_ranges != slots2->num_ranges) return 0;

-    /* Sort slot ranges first */
-    qsort(slots1->ranges, slots1->num_ranges, sizeof(slotRange), compareSlotRange);
-    qsort(slots2->ranges, slots2->num_ranges, sizeof(slotRange), compareSlotRange);
+    slotRangeArraySortAndMerge(slots1);
+    slotRangeArraySortAndMerge(slots2);


⚠️ Bug: slotRangeArrayIsEqual checks count before merging

The num_ranges comparison at line 1877 happens before slotRangeArraySortAndMerge() is called at lines 1879-1880. This means two slot range arrays that represent the same set of slots but have different pre-merge num_ranges will incorrectly be reported as not equal.

For example:

Array A: {7000-7001, 7002-7003} → num_ranges = 2, merges to {7000-7003} (num_ranges = 1)

Array B: {7000-7003} → num_ranges = 1

slotRangeArrayIsEqual(A, B) would return 0 at line 1877 before ever merging, even though both arrays represent slots 7000-7003.

The fix is to move the num_ranges comparison after both slotRangeArraySortAndMerge() calls.

_{Was this helpful? React with 👍 / 👎}

Suggested change

int slotRangeArrayIsEqual(slotRangeArray *slots1, slotRangeArray *slots2) {

if (slots1->num_ranges != slots2->num_ranges) return 0;

/* Sort slot ranges first */

qsort(slots1->ranges, slots1->num_ranges, sizeof(slotRange), compareSlotRange);

qsort(slots2->ranges, slots2->num_ranges, sizeof(slotRange), compareSlotRange);

slotRangeArraySortAndMerge(slots1);

slotRangeArraySortAndMerge(slots2);

int slotRangeArrayIsEqual(slotRangeArray *slots1, slotRangeArray *slots2) {

slotRangeArraySortAndMerge(slots1);

slotRangeArraySortAndMerge(slots2);

if (slots1->num_ranges != slots2->num_ranges) return 0;

for (int i = 0; i < slots1->num_ranges; i++) {

Apply suggested fix

gitar-bot-staging · 2026-02-10T22:15:16Z

src/cluster.c

    sds err = NULL;
-    if (validateSlotRanges(slots, &err) != C_OK) {
-        addReplyErrorSds(c, err);
+    if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) {
+        sdsfree(err);
        slotRangeArrayFree(slots);
        return NULL;


⚠️ Bug: Validation error silently dropped, no reply sent to client

In parseSlotRangesOrReply, the validation error message from slotRangeArrayNormalizeAndValidate() is silently freed at line 2016 without sending any error reply to the client. The old code was:

addReplyErrorSds(c, err); // sends error AND takes ownership of err

The new code is:

sdsfree(err); // silently discards error message

Since the function name convention is *OrReply (meaning it replies on error), and callers like clusterMigrationCommandImport simply return when parseSlotRangesOrReply returns NULL (trusting that an error was already sent), the client will receive no response at all. This will cause the client connection to hang or timeout.

The fix should restore the error reply to the client. Note that addReplyErrorSds takes ownership of the sds string, so sdsfree(err) should be removed when using it.

_{Was this helpful? React with 👍 / 👎}

Suggested change

sds err = NULL;

if (validateSlotRanges(slots, &err) != C_OK) {

addReplyErrorSds(c, err);

if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) {

sdsfree(err);

slotRangeArrayFree(slots);

return NULL;

if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) {

addReplyErrorSds(c, err);

slotRangeArrayFree(slots);

return NULL;

}

Apply suggested fix

tezc and others added 8 commits January 25, 2026 12:11

Fix adjacent slot range behavior in ASM operations

deabeea

minor

0bb1647

minor

98943f5

minor

a6db339

Update src/cluster_asm.c

3129018

Co-authored-by: Yuan Wang <yuan.wang@redis.com>

minor

55e9331

minor

f32efc8

update pr

0443e9f

gitar-bot-staging bot reviewed Feb 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Review Bench PR #14637 - Fix adjacent slot range behavior in ASM operations #2

Code Review Bench PR #14637 - Fix adjacent slot range behavior in ASM operations #2

Uh oh!

ketkarameya commented Feb 10, 2026 •

edited by gitar-bot-staging bot

Loading

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026 •

edited

Loading

Gitar Rules

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Code Review Bench PR #14637 - Fix adjacent slot range behavior in ASM operations #2

Are you sure you want to change the base?

Code Review Bench PR #14637 - Fix adjacent slot range behavior in ASM operations #2

Uh oh!

Conversation

ketkarameya commented Feb 10, 2026 • edited by gitar-bot-staging bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Bench PR redis#14637 - Fix adjacent slot range behavior in ASM operations

Summary by Gitar

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Gitar Rules

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ketkarameya commented Feb 10, 2026 •

edited by gitar-bot-staging bot

Loading

gitar-bot-staging bot commented Feb 10, 2026 •

edited

Loading