Fix HINCRBYFLOAT removes field expiration on replica#8
Conversation
📝 WalkthroughWalkthroughThis pull request introduces field expiration tracking for hash operations. A new shared string object ( 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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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/t_hash.c`:
- Around line 2242-2244: The variable `unused_flag` is declared but never
used—remove its declaration to clean up the function; locate the block where
`int has_expiration = 0; uint64_t expireat = EB_EXPIRE_TIME_INVALID; int
unused_flag = 0;` is defined (in the function in src/t_hash.c) and delete the
`int unused_flag = 0;` part, keeping `has_expiration` and `expireat` intact and
ensuring no other code references `unused_flag`.
- Around line 2306-2314: The argv array is undersized and the dynamically
created string is leaked: change the local array used for propagation to have at
least 6 slots (e.g. robj *argv[6]) so writing argv[5] is safe, keep passing 6 to
alsoPropagate, and after calling alsoPropagate decrement the refcount of the
temporary created object from createStringObjectFromLongLong(expireat) (same
pattern used in propagateHashFieldDeletion) to avoid the memory leak; ensure you
reference argv, createStringObjectFromLongLong, and alsoPropagate when making
the edits.
- Around line 742-743: The expiration boundary at the hash field check is
inconsistent: change the condition in the GETF path from "if (*expiredAt >
(uint64_t) commandTimeSnapshot())" to use ">=" so that fields with expiredAt ==
commandTimeSnapshot() are treated as not expired (consistent with
hashTypeIsExpired()); update the condition to "if (*expiredAt >= (uint64_t)
commandTimeSnapshot()) return GETF_OK;" ensuring consistency across checks that
use commandTimeSnapshot(), including interactions with hashTypeIsExpired() and
other expiration checks.
| if (*expiredAt > (uint64_t) commandTimeSnapshot()) | ||
| return GETF_OK; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the exact code at lines 742-743 and surrounding context
sed -n '735,750p' src/t_hash.cRepository: agentic-review-benchmarks/redis-combined-coderabbit
Length of output: 604
🏁 Script executed:
# Now search for the hashTypeIsExpired function at line 574
sed -n '570,585p' src/t_hash.cRepository: agentic-review-benchmarks/redis-combined-coderabbit
Length of output: 547
🏁 Script executed:
# Check around line 3074 for the other expiration check
sed -n '3070,3080p' src/t_hash.cRepository: agentic-review-benchmarks/redis-combined-coderabbit
Length of output: 444
🏁 Script executed:
# Comprehensive search for all expiration-related comparisons with commandTimeSnapshot
rg "commandTimeSnapshot" src/t_hash.c -B2 -A2 --type cRepository: agentic-review-benchmarks/redis-combined-coderabbit
Length of output: 3147
🏁 Script executed:
# Also search for other expiration comparisons that might not use commandTimeSnapshot
rg -n "expiredAt|expiresAt" src/t_hash.c --type c -C3Repository: agentic-review-benchmarks/redis-combined-coderabbit
Length of output: 2810
Address inconsistent expiration boundary conditions in hash field expiration checks.
The condition at line 742 uses > commandTimeSnapshot() to determine if a field is valid, meaning fields with expiredAt == now are considered expired. However, this is inconsistent with hashTypeIsExpired() at line 574, which uses < commandTimeSnapshot() (treating equal as NOT expired), while other checks at line 3074 and elsewhere use <= or < operators. This inconsistency can lead to subtle bugs where the same expiration time is handled differently depending on which code path processes it. Standardize the boundary condition across all expiration checks.
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 742 - 743, The expiration boundary at the hash
field check is inconsistent: change the condition in the GETF path from "if
(*expiredAt > (uint64_t) commandTimeSnapshot())" to use ">=" so that fields with
expiredAt == commandTimeSnapshot() are treated as not expired (consistent with
hashTypeIsExpired()); update the condition to "if (*expiredAt >= (uint64_t)
commandTimeSnapshot()) return GETF_OK;" ensuring consistency across checks that
use commandTimeSnapshot(), including interactions with hashTypeIsExpired() and
other expiration checks.
| 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 variable unused_flag is declared but never used in this function.
🧹 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; |
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 2242 - 2244, The variable `unused_flag` is
declared but never used—remove its declaration to clean up the function; locate
the block where `int has_expiration = 0; uint64_t expireat =
EB_EXPIRE_TIME_INVALID; int unused_flag = 0;` is defined (in the function in
src/t_hash.c) and delete the `int unused_flag = 0;` part, keeping
`has_expiration` and `expireat` intact and ensuring no other code references
`unused_flag`.
| 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: Buffer overflow and memory leak in HPEXPIREAT propagation.
Two issues in this code block:
-
Buffer overflow:
argvis declared with 5 elements (robj *argv[5]), but the code writes toargv[5](index 5 is out of bounds for a 5-element array) and passesargc=6toalsoPropagate. -
Memory leak: The object created at
argv[2]viacreateStringObjectFromLongLong(expireat)is never freed afteralsoPropagate. Following the pattern inpropagateHashFieldDeletion(lines 2946-2964), dynamically created objects should be decremented after propagation.
🐛 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);
+ 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.
| 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); | |
| decrRefCount(argv[2]); | |
| } |
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 2306 - 2314, The argv array is undersized and the
dynamically created string is leaked: change the local array used for
propagation to have at least 6 slots (e.g. robj *argv[6]) so writing argv[5] is
safe, keep passing 6 to alsoPropagate, and after calling alsoPropagate decrement
the refcount of the temporary created object from
createStringObjectFromLongLong(expireat) (same pattern used in
propagateHashFieldDeletion) to avoid the memory leak; ensure you reference argv,
createStringObjectFromLongLong, and alsoPropagate when making the edits.
3 participants
Benchmark PR from agentic-review-benchmarks#8
Summary by CodeRabbit
Release Notes
New Features
Tests