Skip to content

Fix adjacent slot range behavior in ASM operations#2

Open
tomerqodo wants to merge 8 commits into
sentry_full_base_fix_adjacent_slot_range_behavior_in_asm_operations_pr2from
sentry_full_head_fix_adjacent_slot_range_behavior_in_asm_operations_pr2
Open

Fix adjacent slot range behavior in ASM operations#2
tomerqodo wants to merge 8 commits into
sentry_full_base_fix_adjacent_slot_range_behavior_in_asm_operations_pr2from
sentry_full_head_fix_adjacent_slot_range_behavior_in_asm_operations_pr2

Conversation

@tomerqodo

Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#2

Comment thread src/cluster.c
Comment on lines +2015 to +2016
if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) {
sdsfree(err);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The function parseSlotRangesOrReply no longer sends an error reply to the client on validation failure; it only frees the error string, causing silent failures.
Severity: HIGH

Suggested Fix

Restore the error reporting mechanism. The line sdsfree(err); should be replaced with addReplyErrorSds(c, err); to ensure validation errors are sent to the client.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/cluster.c#L2015-L2016

Potential issue: In the `parseSlotRangesOrReply` function, the call to
`addReplyErrorSds(c, err)` was replaced with `sdsfree(err)`. The original
`addReplyErrorSds` function was responsible for sending the error message to the client
before freeing the memory. The new code now silently discards the validation error
without notifying the client. This affects commands like `CLUSTER MIGRATION IMPORT`,
which will fail without explanation when provided with invalid slot ranges, making
debugging difficult for operators.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment thread src/cluster.c
Comment on lines +1879 to +1880
slotRangeArraySortAndMerge(slots1);
slotRangeArraySortAndMerge(slots2);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The comparison function slotRangeArrayIsEqual now mutates its input arrays by merging slot ranges, which is an unexpected and potentially breaking side effect.
Severity: MEDIUM

Suggested Fix

The slotRangeArrayIsEqual function should not modify its inputs. It should either perform the comparison on copies of the arrays or use a non-mutating comparison logic to check for equality.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/cluster.c#L1879-L1880

Potential issue: The comparison function `slotRangeArrayIsEqual` now has a side effect
of modifying its input arrays. It calls `slotRangeArraySortAndMerge`, which sorts and
merges slot ranges, altering the `num_ranges` field and the contents of the arrays. This
violates the principle that comparison functions should not mutate their inputs. Callers
that reuse an array after passing it to `slotRangeArrayIsEqual` may operate on
unexpectedly modified data, leading to subtle bugs.

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants