Clean up lookahead-related code#25
Conversation
Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
WalkthroughThis pull request refactors cluster slot extraction logic by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@src/cluster_slot_stats.c`:
- Around line 139-141: The guard in canAddNetworkBytesOut only excludes
INVALID_CLUSTER_SLOT and can still allow CLUSTER_CROSSSLOT (a negative
sentinel), which can trigger serverAssert(c->slot >= 0...) — change the
condition to explicitly reject negative slots (e.g. require c->slot >= 0) or
check both sentinels (c->slot != INVALID_CLUSTER_SLOT && c->slot !=
CLUSTER_CROSSSLOT) in canAddNetworkBytesOut and update the other similar guards
in this file that currently only compare against INVALID_CLUSTER_SLOT so they
also reject negative/CLUSTER_CROSSSLOT values before doing slot-based
accounting.
In `@src/cluster.c`:
- Around line 1112-1115: The for-loop iterating over keys_result uses j <=
keys_result->numkeys which causes an off-by-one and out-of-bounds access; update
the loop condition to iterate j from 0 to j < keys_result->numkeys so you only
access keys_result->keys[j], argv[keys_result->keys[j].pos], and slot_buffer[j];
ensure slot_buffer is sized for keys_result->numkeys and keep use of
keyHashSlot((char*)this_key->ptr, sdslen(this_key->ptr)) and robj *this_key
unchanged.
- Around line 1110-1124: Replace use of deprecated malloc/free for slot_buffer
with Redis allocation APIs: allocate slot_buffer with zmalloc(sizeof(int) *
keys_result->numkeys) and free it with zfree(slot_buffer); ensure you still
handle allocation failure (check for NULL and return an error or cleanup) in the
same block that references slot_buffer, preserving the existing logic around
keyHashSlot, keys_result, first_slot, INVALID_CLUSTER_SLOT and the
CLUSTER_CROSSSLOT return path.
🧹 Nitpick comments (2)
src/cluster.h (1)
26-26: Add an inline comment for the new sentinel.
CLUSTER_CROSSSLOTis public API but lacks a brief semantic note likeINVALID_CLUSTER_SLOT, which helps readers and downstream usage.📝 Suggested tweak
-#define CLUSTER_CROSSSLOT (-2) +#define CLUSTER_CROSSSLOT (-2) /* Keys hash to different slots (cross-slot). */src/cluster.c (1)
1108-1124:slot_bufferis allocated but never read — consider removing it.The
slot_bufferarray is populated with slot values (slot_buffer[j] = this_slot) but these values are never used. The cross-slot detection logic only usesfirst_slotandthis_slotdirectly. This buffer appears to be leftover from a previous implementation or intended for future use.If the buffer is not needed, removing it would simplify the code and avoid unnecessary allocations.
♻️ Simplified implementation without unused buffer
int extractSlotFromKeysResult(robj **argv, getKeysResult *keys_result) { if (keys_result->numkeys == 0 || !server.cluster_enabled) return INVALID_CLUSTER_SLOT; int first_slot = INVALID_CLUSTER_SLOT; - /* Allocate temporary buffer for slot tracking */ - int *slot_buffer = zmalloc(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) { - zfree(slot_buffer); return CLUSTER_CROSSSLOT; } } - zfree(slot_buffer); return first_slot; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/cluster.csrc/cluster.hsrc/cluster_asm.csrc/cluster_slot_stats.csrc/db.csrc/networking.csrc/server.csrc/server.h
🧰 Additional context used
🧬 Code graph analysis (3)
src/cluster_slot_stats.c (1)
src/server.h (1)
clusterSlotStatsEnabled(3741-3741)
src/db.c (1)
src/cluster.c (1)
extractSlotFromKeysResult(1103-1126)
src/cluster.c (2)
src/cluster.h (1)
keyHashSlot(66-85)src/sds.h (1)
sdslen(73-86)
🪛 GitHub Actions: Codecov
src/cluster.c
[error] 1110-1124: lcov: 'malloc' is deprecated and 'free' is deprecated when building with -Werror. Update code to use non-deprecated allocation/free patterns.
🪛 GitHub Actions: External Server Tests
src/cluster.c
[error] 1110-1124: Werror: 'malloc' and 'free' are deprecated declarations. Build failed due to treating deprecated declarations as errors while compiling cluster.c (lines 1110-1124). Command causing the failure: make REDIS_CFLAGS=-Werror (via 'make -C src all'), triggered by -Werror in the build.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (cpp)
- GitHub Check: build-old-chain-jemalloc
- GitHub Check: code-coverage
- GitHub Check: build-centos-jemalloc
- GitHub Check: build-centos-jemalloc
🔇 Additional comments (7)
src/networking.c (1)
5099-5100: LGTM!Good documentation addition. The comment accurately explains the scenario where
argv[j]can be NULL: whenreclaimPendingCommandhandles IO thread commands, it decrements the refcount and setsargv[j] = NULLfor shared objects before deferring the free operation. This clarifies the purpose of the existing NULL guard.src/cluster_asm.c (2)
754-778: Cross-slot handling in migration feed looks correct.Deferring cancellation via
cross_slot_during_propagatingavoids recursion while preventing further propagation in the same cycle.
3455-3467: Module propagation slot checks are consistent with new sentinels.Rejecting
CLUSTER_CROSSSLOTand allowingINVALID_CLUSTER_SLOT(no-keys) matches the updated semantics.src/server.h (1)
2410-2413: LGTM: flag addition is clean and non-overlapping.src/server.c (1)
4135-4146: LGTM — cross-slot precheck looks correct.Setting
CLIENT_READ_CROSS_SLOTand resetting toINVALID_CLUSTER_SLOTkeeps downstream routing from using a sentinel slot.src/db.c (2)
429-441: LGTM!The refactoring of
getSlotFromCommandto delegate slot extraction toextractSlotFromKeysResultsimplifies the code and centralizes the slot extraction logic. The early return for no command or disabled cluster is appropriate.
3200-3203: LGTM!The conditional now correctly guards the slot extraction call, which is appropriate since
extractSlotFromKeysResulthandles the zero-key case internally by returningINVALID_CLUSTER_SLOT.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| static int canAddNetworkBytesOut(client *c) { | ||
| return clusterSlotStatsEnabled() && c->slot != -1; | ||
| return clusterSlotStatsEnabled() && c->slot != INVALID_CLUSTER_SLOT; | ||
| } |
There was a problem hiding this comment.
Guard against CLUSTER_CROSSSLOT to avoid assertion crashes.
If c->slot can be CLUSTER_CROSSSLOT (new sentinel), the current checks only exclude INVALID_CLUSTER_SLOT, so serverAssert(c->slot >= 0...) can fire on cross-slot commands. Recommend excluding any negative slot (or explicitly checking both sentinels) before accounting.
🔧 Suggested fix
static int canAddNetworkBytesOut(client *c) {
- return clusterSlotStatsEnabled() && c->slot != INVALID_CLUSTER_SLOT;
+ return clusterSlotStatsEnabled() && c->slot >= 0;
}
static int canAddCpuDuration(client *c) {
return server.cluster_slot_stats_enabled && /* Config should be enabled. */
server.cluster_enabled && /* Cluster mode should be enabled. */
- c->slot != INVALID_CLUSTER_SLOT && /* Command should be slot specific. */
+ c->slot >= 0 && /* Command should be slot specific. */
(!server.execution_nesting || /* Either command should not be nested, */
(c->realcmd->flags & CMD_BLOCKING)); /* or it must be due to unblocking. */
}
static int canAddNetworkBytesIn(client *c) {
- return clusterSlotStatsEnabled() && c->slot != INVALID_CLUSTER_SLOT &&
+ return clusterSlotStatsEnabled() && c->slot >= 0 &&
!(c->flags & CLIENT_BLOCKED) && !server.in_exec;
}Also applies to: 223-227, 246-253
🤖 Prompt for AI Agents
In `@src/cluster_slot_stats.c` around lines 139 - 141, The guard in
canAddNetworkBytesOut only excludes INVALID_CLUSTER_SLOT and can still allow
CLUSTER_CROSSSLOT (a negative sentinel), which can trigger serverAssert(c->slot
>= 0...) — change the condition to explicitly reject negative slots (e.g.
require c->slot >= 0) or check both sentinels (c->slot != INVALID_CLUSTER_SLOT
&& c->slot != CLUSTER_CROSSSLOT) in canAddNetworkBytesOut and update the other
similar guards in this file that currently only compare against
INVALID_CLUSTER_SLOT so they also reject negative/CLUSTER_CROSSSLOT values
before doing slot-based accounting.
| 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); |
There was a problem hiding this comment.
Build failure: Use zmalloc/zfree instead of deprecated malloc/free.
The pipeline failures indicate that malloc and free are deprecated in this codebase. Redis/Valkey uses its own memory allocation functions (zmalloc/zfree) for consistency and memory tracking.
🔧 Proposed fix
/* Allocate temporary buffer for slot tracking */
- int *slot_buffer = malloc(sizeof(int) * keys_result->numkeys);
+ int *slot_buffer = zmalloc(sizeof(int) * keys_result->numkeys);
- for (int j = 0; j <= keys_result->numkeys; j++) {
+ 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) {
- free(slot_buffer);
+ zfree(slot_buffer);
return CLUSTER_CROSSSLOT;
}
}
- free(slot_buffer);
+ zfree(slot_buffer);
return first_slot;🧰 Tools
🪛 GitHub Actions: Codecov
[error] 1110-1124: lcov: 'malloc' is deprecated and 'free' is deprecated when building with -Werror. Update code to use non-deprecated allocation/free patterns.
🪛 GitHub Actions: External Server Tests
[error] 1110-1124: Werror: 'malloc' and 'free' are deprecated declarations. Build failed due to treating deprecated declarations as errors while compiling cluster.c (lines 1110-1124). Command causing the failure: make REDIS_CFLAGS=-Werror (via 'make -C src all'), triggered by -Werror in the build.
🤖 Prompt for AI Agents
In `@src/cluster.c` around lines 1110 - 1124, Replace use of deprecated
malloc/free for slot_buffer with Redis allocation APIs: allocate slot_buffer
with zmalloc(sizeof(int) * keys_result->numkeys) and free it with
zfree(slot_buffer); ensure you still handle allocation failure (check for NULL
and return an error or cleanup) in the same block that references slot_buffer,
preserving the existing logic around keyHashSlot, keys_result, first_slot,
INVALID_CLUSTER_SLOT and the CLUSTER_CROSSSLOT return path.
| 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; |
There was a problem hiding this comment.
Critical: Off-by-one error causes out-of-bounds array access.
The loop condition j <= keys_result->numkeys iterates one element past the valid range. Array indices are 0 to numkeys - 1, so accessing keys_result->keys[numkeys] is undefined behavior.
🐛 Proposed fix
- for (int j = 0; j <= keys_result->numkeys; j++) {
+ for (int j = 0; j < keys_result->numkeys; j++) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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; | |
| 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; |
🤖 Prompt for AI Agents
In `@src/cluster.c` around lines 1112 - 1115, The for-loop iterating over
keys_result uses j <= keys_result->numkeys which causes an off-by-one and
out-of-bounds access; update the loop condition to iterate j from 0 to j <
keys_result->numkeys so you only access keys_result->keys[j],
argv[keys_result->keys[j].pos], and slot_buffer[j]; ensure slot_buffer is sized
for keys_result->numkeys and keep use of keyHashSlot((char*)this_key->ptr,
sdslen(this_key->ptr)) and robj *this_key unchanged.
2 participants
Benchmark PR from agentic-review-benchmarks#3
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.