Skip to content

Clean up lookahead-related code#3

Open
tomerqodo wants to merge 7 commits intoaugment_full_base_clean_up_lookahead-related_code_pr3from
augment_full_head_clean_up_lookahead-related_code_pr3
Open

Clean up lookahead-related code#3
tomerqodo wants to merge 7 commits intoaugment_full_base_clean_up_lookahead-related_code_pr3from
augment_full_head_clean_up_lookahead-related_code_pr3

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#3

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Jan 26, 2026

🤖 Augment PR Summary

Summary: This PR streamlines cluster slot “lookahead” handling by centralizing slot extraction and unifying sentinel return codes.
Key changes: extractSlotFromKeysResult() now returns a real slot, INVALID_CLUSTER_SLOT for no-keys/cluster-disabled, and a new CLUSTER_CROSSSLOT for cross-slot.
Refactors: getSlotFromCommand() now reuses extractSlotFromKeysResult(), and preprocessCommand() maps cross-slot to CLIENT_READ_CROSS_SLOT while normalizing the slot to invalid.
API/constants: Adds CLUSTER_CROSSSLOT in cluster.h and removes the old GETSLOT_* defines.
Call-site updates: ASM migration and slot-stats code paths are updated to use the new constants.
Minor: Improves a comment in freePendingCommand() explaining why argv[j] may be NULL.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

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

Review completed. 3 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/cluster.c
/* Allocate temporary buffer for slot tracking */
int *slot_buffer = malloc(sizeof(int) * keys_result->numkeys);

for (int j = 0; j <= keys_result->numkeys; j++) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The loop condition iterates one past keys_result->numkeys, which will access keys_result->keys[j] and write slot_buffer[j] out of bounds on the final iteration (potential crash/memory corruption).

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Comment thread src/cluster.c
for (int j = 0; j < keys_result->numkeys; j++) {

/* Allocate temporary buffer for slot tracking */
int *slot_buffer = malloc(sizeof(int) * keys_result->numkeys);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using plain malloc here can return NULL on OOM and would lead to a NULL dereference in the loop; Redis code typically relies on its allocator/oom handling for these paths.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Comment thread src/db.c

*slot = INVALID_CLUSTER_SLOT;
if (num_keys >= 0)
if (num_keys > 0) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

extractKeysAndSlot() only writes to *slot when num_keys > 0, so callers that don’t pre-initialize *slot will observe a stale/undefined slot value when num_keys == 0 despite the function’s contract saying it returns the slot via *slot.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

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