Clean up lookahead-related code

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#3

[qodo-code-review]

Review Summary by Qodo

Refactor cluster slot handling and unify slot extraction logic

✨ Enhancement 🐞 Bug fix

Walkthroughs

Description

• Rename GETSLOT_CROSSSLOT constant to CLUSTER_CROSSSLOT for consistency
• Unify slot extraction logic by refactoring getSlotFromCommand to use extractSlotFromKeysResult
• Fix cluster slot status check by replacing magic number -1 with INVALID_CLUSTER_SLOT
• Improve extractSlotFromKeysResult to handle cluster disabled state and return proper error codes

Diagram

flowchart LR
  A["getSlotFromCommand"] -->|"now uses"| B["extractSlotFromKeysResult"]
  B -->|"returns"| C["CLUSTER_CROSSSLOT or INVALID_CLUSTER_SLOT"]
  D["Magic number -1"] -->|"replaced with"| E["INVALID_CLUSTER_SLOT constant"]
  F["GETSLOT_CROSSSLOT"] -->|"renamed to"| G["CLUSTER_CROSSSLOT"]

Loading

File Changes

1. src/cluster.h ✨ Enhancement +1/-0

Define CLUSTER_CROSSSLOT constant in cluster header

• Added CLUSTER_CROSSSLOT constant definition with value -2
• Removed GETSLOT_NOKEYS and GETSLOT_CROSSSLOT definitions from server.h
• Centralized cluster slot error codes in cluster.h header

src/cluster.h

---

2. src/cluster.c ✨ Enhancement +14/-9

Improve slot extraction with proper error codes

• Refactored extractSlotFromKeysResult to check cluster_enabled status upfront
• Changed return value from INVALID_CLUSTER_SLOT to CLUSTER_CROSSSLOT for cross-slot errors
• Added temporary slot_buffer allocation for slot tracking
• Updated function documentation to clarify return values

src/cluster.c

---

3. src/cluster_asm.c ✨ Enhancement +4/-4

Update slot constant references in cluster assembly

• Replaced GETSLOT_NOKEYS with INVALID_CLUSTER_SLOT constant
• Replaced GETSLOT_CROSSSLOT with CLUSTER_CROSSSLOT constant
• Updated two locations in asmFeedMigrationClient and asmModulePropagateBeforeSlotSnapshot

src/cluster_asm.c

---

View more (5)

4. src/cluster_slot_stats.c ✨ Enhancement +3/-3

Replace magic numbers with INVALID_CLUSTER_SLOT constant

• Replaced magic number -1 with INVALID_CLUSTER_SLOT in three functions
• Updated canAddNetworkBytesOut, canAddCpuDuration, and canAddNetworkBytesIn checks
• Improved code readability with semantic constant names

src/cluster_slot_stats.c

---

5. src/db.c ✨ Enhancement +8/-20

Unify slot extraction by reusing extractSlotFromKeysResult

• Refactored getSlotFromCommand to use extractSlotFromKeysResult instead of duplicating logic
• Removed manual slot extraction loop and key iteration
• Updated function documentation to reference new constant names
• Simplified return value handling

src/db.c

---

6. src/server.c 🐞 Bug fix +5/-4

Fix cross-slot error detection and slot reset logic

• Updated preprocessCommand to check for CLUSTER_CROSSSLOT instead of INVALID_CLUSTER_SLOT
• Added explicit slot reset to INVALID_CLUSTER_SLOT when cross-slot error detected
• Changed condition from num_keys > 0 to num_keys >= 0 for proper error handling

src/server.c

---

7. src/server.h ✨ Enhancement +1/-4

Remove slot constants from server header

• Removed GETSLOT_NOKEYS and GETSLOT_CROSSSLOT macro definitions
• Fixed alignment of PENDING_CMD_KEYS_RESULT_VALID flag comment
• Moved slot constant definitions to cluster.h

src/server.h

---

8. src/networking.c 📝 Documentation +1/-1

Clarify NULL pointer handling in pending command cleanup

• Improved TODO comment in freePendingCommand to clarify argv[j] NULL handling
• Enhanced code documentation for better maintainability

src/networking.c

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. Direct malloc/free in extractSlotFromKeysResult 📘 Rule violation ⛯ Reliability

Description

The new code allocates and frees memory using malloc()/free() in src/, which bypasses Redis
memory tracking. This violates the requirement to use the zmalloc/zfree family for all dynamic
allocations in src/.

Code

src/cluster.c[R1110-1124]

+    int *slot_buffer = malloc(sizeof(int) * keys_result->numkeys);
+
+    for (int j = 0; j <= keys_result->numkeys; j++) {
        robj *this_key = argv[keys_result->keys[j].pos];
        int this_slot = (int)keyHashSlot((char*)this_key->ptr, sdslen(this_key->ptr));
+        slot_buffer[j] = this_slot;

        if (first_slot == INVALID_CLUSTER_SLOT)
            first_slot = this_slot;
        else if (first_slot != this_slot) {
-            return INVALID_CLUSTER_SLOT;
+            free(slot_buffer);
+            return CLUSTER_CROSSSLOT;
        }
    }
+    free(slot_buffer);

Evidence

PR Compliance ID 2 forbids direct malloc()/free() calls in src/ and requires using
zmalloc/zfree. The added lines in src/cluster.c allocate slot_buffer with malloc() and
release it with free().

AGENTS.md
src/cluster.c[1110-1110]
src/cluster.c[1120-1124]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/cluster.c` introduces a new temporary buffer allocated with `malloc()` and released with `free()`, which violates the requirement to use Redis's `zmalloc`/`zfree` family in `src/` for memory tracking.

## Issue Context
Redis requires `zmalloc`-family allocators in `src/` to ensure memory accounting and consistent allocation behavior.

## Fix Focus Areas
- src/cluster.c[1110-1124]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Slot loop buffer overflow 🐞 Bug ⛨ Security

Description

extractSlotFromKeysResult allocates slot_buffer for numkeys elements but loops with j <= numkeys,
reading keys_result->keys[numkeys] and writing slot_buffer[numkeys], causing out-of-bounds memory
access and potential heap corruption/crash during command processing. It also uses plain malloc
without Redis’s OOM handler, so malloc failure would lead to NULL dereference.

Code

src/cluster.c[R1109-1116]

+    /* Allocate temporary buffer for slot tracking */
+    int *slot_buffer = malloc(sizeof(int) * keys_result->numkeys);
+
+    for (int j = 0; j <= keys_result->numkeys; j++) {
        robj *this_key = argv[keys_result->keys[j].pos];
        int this_slot = (int)keyHashSlot((char*)this_key->ptr, sdslen(this_key->ptr));
+        slot_buffer[j] = this_slot;

Evidence

The loop iterates one past keys_result->numkeys, but getKeysResult->keys is an array of key
references with valid indices [0..numkeys-1], so j==numkeys is out of bounds; the code also writes
to slot_buffer[j] with the same invalid index. Additionally, Redis’s allocator (zmalloc) aborts via
an OOM handler, while plain malloc does not, and the new code does not check malloc’s return value.

src/cluster.c[1098-1125]
src/server.h[2390-2405]
src/zmalloc.c[111-116]
src/server.c[4135-4147]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`extractSlotFromKeysResult()` currently performs out-of-bounds reads/writes due to an off-by-one loop (`j <= numkeys`) combined with a buffer sized for exactly `numkeys`. This can corrupt the heap or crash Redis on common multi-key commands in cluster mode; it also introduces an unchecked `malloc()` in a hot path.

### Issue Context
`getKeysResult->keys` is indexed by `numkeys` count, so valid indices are `[0..numkeys-1]`. The current code also allocates `slot_buffer` but never uses it for logic, so it can be removed entirely.

### Fix Focus Areas
- src/cluster.c[1103-1125]

### Expected fix
- Change the loop to `for (int j = 0; j < keys_result->numkeys; j++)`.
- Remove `slot_buffer` completely (since it is not used) to avoid per-call heap allocation and eliminate NULL/OOM handling concerns.
- If a buffer is truly needed, use `zmalloc` and check sizes/indices carefully, but prefer no allocation here.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-code-review]

1. Direct malloc/free in extractslotfromkeysresult 📘 Rule violation ⛯ Reliability

The new code allocates and frees memory using malloc()/free() in src/, which bypasses Redis
memory tracking. This violates the requirement to use the zmalloc/zfree family for all dynamic
allocations in src/.

Agent Prompt

## Issue description
`src/cluster.c` introduces a new temporary buffer allocated with `malloc()` and released with `free()`, which violates the requirement to use Redis's `zmalloc`/`zfree` family in `src/` for memory tracking.

## Issue Context
Redis requires `zmalloc`-family allocators in `src/` to ensure memory accounting and consistent allocation behavior.

## Fix Focus Areas
- src/cluster.c[1110-1124]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

2. Slot loop buffer overflow 🐞 Bug ⛨ Security

extractSlotFromKeysResult allocates slot_buffer for numkeys elements but loops with j <= numkeys,
reading keys_result->keys[numkeys] and writing slot_buffer[numkeys], causing out-of-bounds memory
access and potential heap corruption/crash during command processing. It also uses plain malloc
without Redis’s OOM handler, so malloc failure would lead to NULL dereference.

Agent Prompt

### Issue description
`extractSlotFromKeysResult()` currently performs out-of-bounds reads/writes due to an off-by-one loop (`j <= numkeys`) combined with a buffer sized for exactly `numkeys`. This can corrupt the heap or crash Redis on common multi-key commands in cluster mode; it also introduces an unchecked `malloc()` in a hot path.

### Issue Context
`getKeysResult->keys` is indexed by `numkeys` count, so valid indices are `[0..numkeys-1]`. The current code also allocates `slot_buffer` but never uses it for logic, so it can be removed entirely.

### Fix Focus Areas
- src/cluster.c[1103-1125]

### Expected fix
- Change the loop to `for (int j = 0; j < keys_result->numkeys; j++)`.
- Remove `slot_buffer` completely (since it is not used) to avoid per-call heap allocation and eliminate NULL/OOM handling concerns.
- If a buffer is truly needed, use `zmalloc` and check sizes/indices carefully, but prefer no allocation here.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools