Fix HINCRBYFLOAT removes field expiration on replica#30
Conversation
WalkthroughThe changes extend Redis hash field expiration infrastructure by introducing a new shared object and modifying the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🤖 Fix all issues with AI agents
In `@src/t_hash.c`:
- Around line 2306-2313: The local argv array is undersized causing a stack
overflow: change the declaration of argv to hold six pointers (e.g., robj
*argv[6];) so writing argv[0]..argv[5] is safe, then keep the call to
alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL); as-is; verify
this fixes the mismatch between the array size and the argc used (symbols: argv,
alsoPropagate, shared.hpexpireat, createStringObjectFromLongLong, c->argv).
- Around line 2242-2244: Remove the unused variable declaration "unused_flag" in
src/t_hash.c (the block with int has_expiration and uint64_t expireat) since it
is never referenced; delete the line "int unused_flag = 0;" and run a build to
confirm no remaining references or related dead code need cleanup.
- Around line 2309-2314: The temporary RedisObject created by
createStringObjectFromLongLong(expireat) and stored in argv[2] is leaked because
alsoPropagate increments its refcount; after calling alsoPropagate(c->db->id,
argv, 6, PROPAGATE_AOF|PROPAGATE_REPL) you must release the caller's reference
by calling decrRefCount(argv[2]) (i.e., decrease the refcount for the object
created by createStringObjectFromLongLong) so the object is not leaked; locate
the block that sets argv[2] and add the decrRefCount(argv[2]) immediately after
the alsoPropagate call.
- Around line 742-743: The expiration boundary in hashTypeGetValue is wrong:
change the condition that currently reads "*expiredAt > (uint64_t)
commandTimeSnapshot()" to use ">=" so the field remains valid when expireAt ==
currentTime (i.e., use "*expiredAt >= (uint64_t) commandTimeSnapshot()") to
match the project's standard expiration semantics used elsewhere; if there is an
intentional reason to treat the boundary differently, add a concise comment in
hashTypeGetValue explaining the special semantics and why it deviates from other
checks instead of changing the comparison silently.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server.csrc/server.hsrc/t_hash.ctests/unit/type/hash-field-expire.tcl
🧰 Additional context used
🧬 Code graph analysis (2)
src/server.c (1)
src/object.c (1)
createStringObject(102-107)
src/t_hash.c (2)
src/server.c (3)
commandTimeSnapshot(221-236)preventCommandPropagation(3349-3351)alsoPropagate(3322-3335)src/object.c (1)
createStringObjectFromLongLong(149-151)
🪛 GitHub Actions: CI
src/t_hash.c
[error] 2244-2244: Unused variable 'unused_flag' treated as error due to -Werror while compiling hincrbyfloatCommand. Remove or use the variable.
⏰ 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). (1)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (6)
src/server.c (1)
1990-1990: LGTM for shared object initialization.The new shared string uses the correct literal and length, consistent with nearby shared object patterns.
src/server.h (1)
1325-1329: Good addition to the shared object pool.Adds a dedicated shared token for
"FIELDS"alongside other protocol strings.tests/unit/type/hash-field-expire.tcl (1)
1269-1320: Nice coverage for HINCRBYFLOAT replication semantics.The test validates both replication stream shape and TTL/value preservation across master/replica.
src/t_hash.c (3)
713-719: LGTM!The function signature extension with
expiredAtparameter is well-designed. Using a local dummy when the caller passes NULL is a clean approach that avoids null pointer dereference issues and simplifies caller code.
802-802: LGTM!The call sites passing
NULLfor the newexpiredAtparameter are appropriate since they don't need to capture or use the expiration time.
2297-2304: Logic for expiration preservation is correct.The approach of preventing default propagation and manually propagating HSET followed by HPEXPIREAT ensures that replicas correctly receive both the updated value and the restored expiration time. This properly fixes the issue where HINCRBYFLOAT was losing field expiration on replicas.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (*expiredAt > (uint64_t) commandTimeSnapshot()) | ||
| return GETF_OK; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all commandTimeSnapshot() comparisons in src/t_hash.c to verify expiration consistency
rg -n 'commandTimeSnapshot\(\)' src/t_hash.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1286
🏁 Script executed:
#!/bin/bash
# Read context around line 742 to understand the function and its semantics
sed -n '725,760p' src/t_hash.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1171
🏁 Script executed:
#!/bin/bash
# Find the function name for line 742
sed -n '700,745p' src/t_hash.c | head -50Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1731
Fix expiration boundary condition inconsistency in hashTypeGetValue.
The condition at line 742 changed from *expiredAt >= commandTimeSnapshot() to *expiredAt > commandTimeSnapshot(), making fields expire AT their expiration time rather than AFTER. This diverges from the majority of expiration checks in the codebase (lines 574, 1424, 2939, 3104, 3397, 3426), which use < to allow fields to remain valid when expireAt >= currentTime. At the boundary (when expireAt equals currentTime), this change causes inconsistent behavior across the hash module. Either align this check with the standard pattern used elsewhere, or document why this function requires different boundary semantics.
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 742 - 743, The expiration boundary in
hashTypeGetValue is wrong: change the condition that currently reads "*expiredAt
> (uint64_t) commandTimeSnapshot()" to use ">=" so the field remains valid when
expireAt == currentTime (i.e., use "*expiredAt >= (uint64_t)
commandTimeSnapshot()") to match the project's standard expiration semantics
used elsewhere; if there is an intentional reason to treat the boundary
differently, add a concise comment in hashTypeGetValue explaining the special
semantics and why it deviates from other checks instead of changing the
comparison silently.
| int has_expiration = 0; | ||
| uint64_t expireat = EB_EXPIRE_TIME_INVALID; | ||
| int unused_flag = 0; |
There was a problem hiding this comment.
Remove unused variable unused_flag.
The pipeline failure indicates this variable is unused. Remove it to fix the build error.
🔧 Proposed fix
int has_expiration = 0;
uint64_t expireat = EB_EXPIRE_TIME_INVALID;
- int unused_flag = 0;📝 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.
| int has_expiration = 0; | |
| uint64_t expireat = EB_EXPIRE_TIME_INVALID; | |
| int unused_flag = 0; | |
| int has_expiration = 0; | |
| uint64_t expireat = EB_EXPIRE_TIME_INVALID; |
🧰 Tools
🪛 GitHub Actions: CI
[error] 2244-2244: Unused variable 'unused_flag' treated as error due to -Werror while compiling hincrbyfloatCommand. Remove or use the variable.
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 2242 - 2244, Remove the unused variable
declaration "unused_flag" in src/t_hash.c (the block with int has_expiration and
uint64_t expireat) since it is never referenced; delete the line "int
unused_flag = 0;" and run a build to confirm no remaining references or related
dead code need cleanup.
| robj *argv[5]; | ||
| argv[0] = shared.hpexpireat; | ||
| argv[1] = c->argv[1]; | ||
| argv[2] = createStringObjectFromLongLong(expireat); | ||
| argv[3] = shared.fields; | ||
| argv[4] = shared.integers[1]; | ||
| argv[5] = c->argv[2]; | ||
| alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL); |
There was a problem hiding this comment.
Critical: Stack buffer overflow in argv array.
The array argv[5] allocates space for 5 elements (indices 0-4), but the code writes to argv[5] (index 5) and calls alsoPropagate with argc=6. This is a stack buffer overflow that will corrupt memory and cause undefined behavior.
🐛 Proposed fix
/* Propagate HPEXPIREAT */
- robj *argv[5];
+ robj *argv[6];
argv[0] = shared.hpexpireat;
argv[1] = c->argv[1];
argv[2] = createStringObjectFromLongLong(expireat);
argv[3] = shared.fields;
argv[4] = shared.integers[1];
argv[5] = c->argv[2];
alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL);📝 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.
| robj *argv[5]; | |
| argv[0] = shared.hpexpireat; | |
| argv[1] = c->argv[1]; | |
| argv[2] = createStringObjectFromLongLong(expireat); | |
| argv[3] = shared.fields; | |
| argv[4] = shared.integers[1]; | |
| argv[5] = c->argv[2]; | |
| alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL); | |
| robj *argv[6]; | |
| argv[0] = shared.hpexpireat; | |
| argv[1] = c->argv[1]; | |
| argv[2] = createStringObjectFromLongLong(expireat); | |
| argv[3] = shared.fields; | |
| argv[4] = shared.integers[1]; | |
| argv[5] = c->argv[2]; | |
| alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL); |
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 2306 - 2313, The local argv array is undersized
causing a stack overflow: change the declaration of argv to hold six pointers
(e.g., robj *argv[6];) so writing argv[0]..argv[5] is safe, then keep the call
to alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL); as-is;
verify this fixes the mismatch between the array size and the argc used
(symbols: argv, alsoPropagate, shared.hpexpireat,
createStringObjectFromLongLong, c->argv).
| argv[2] = createStringObjectFromLongLong(expireat); | ||
| argv[3] = shared.fields; | ||
| argv[4] = shared.integers[1]; | ||
| argv[5] = c->argv[2]; | ||
| alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL); | ||
| } |
There was a problem hiding this comment.
Memory leak: argv[2] is not freed after propagation.
The object created by createStringObjectFromLongLong(expireat) is never released. The alsoPropagate function increments the reference count, but the caller's reference is leaked.
🐛 Proposed fix
argv[3] = shared.fields;
argv[4] = shared.integers[1];
argv[5] = c->argv[2];
alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL);
+ decrRefCount(argv[2]);
}
}📝 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.
| argv[2] = createStringObjectFromLongLong(expireat); | |
| argv[3] = shared.fields; | |
| argv[4] = shared.integers[1]; | |
| argv[5] = c->argv[2]; | |
| alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL); | |
| } | |
| argv[2] = createStringObjectFromLongLong(expireat); | |
| argv[3] = shared.fields; | |
| argv[4] = shared.integers[1]; | |
| argv[5] = c->argv[2]; | |
| alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL); | |
| decrRefCount(argv[2]); | |
| } |
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 2309 - 2314, The temporary RedisObject created by
createStringObjectFromLongLong(expireat) and stored in argv[2] is leaked because
alsoPropagate increments its refcount; after calling alsoPropagate(c->db->id,
argv, 6, PROPAGATE_AOF|PROPAGATE_REPL) you must release the caller's reference
by calling decrRefCount(argv[2]) (i.e., decrease the refcount for the object
created by createStringObjectFromLongLong) so the object is not leaked; locate
the block that sets argv[2] and add the decrRefCount(argv[2]) immediately after
the alsoPropagate call.
2 participants
Benchmark PR from agentic-review-benchmarks#8
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.