Fix HINCRBYFLOAT removes field expiration on replica#21
Conversation
WalkthroughAdds infrastructure for tracking per-field expiration times in hash operations. Introduces a shared object "FIELDS", extends Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Master as Master Server
participant HashOps as Hash Operations
participant Replication as Replication Layer
participant Replica as Replica Server
Client->>Master: HINCRBYFLOAT with field
Master->>HashOps: hashTypeGetValue(field, &expiredAt)
HashOps->>HashOps: Retrieve field value & expiration
HashOps-->>Master: Return value + expiredAt
alt Field has expiration
Master->>Replication: Propagate HSET
Master->>Replication: Propagate HPEXPIREAT
Replication->>Replica: HSET command
Replication->>Replica: HPEXPIREAT command
Replica-->>Replica: Update field with expiration
else No expiration
Master->>Replication: Propagate HSET only
Replication->>Replica: HSET command
Replica-->>Replica: Update field
end
Master-->>Client: OK
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: 2
🤖 Fix all issues with AI agents
In `@src/t_hash.c`:
- Around line 2242-2244: The variable unused_flag is declared but never used in
t_hash.c (see the block with has_expiration, expireat, unused_flag), causing
-Werror=unused-variable; remove the unused_flag declaration (or if it was meant
to be used, implement its usage) so only used variables remain (e.g., keep
has_expiration and expireat, delete the unused_flag line) to resolve the build
failure.
- Around line 2306-2314: The argv buffer overflow occurs because argv is
declared as robj *argv[5] but indices 0..5 are written and passed to
alsoPropagate; change the declaration to robj *argv[6] so it can hold six
elements and keep alsoPropagate(c->db->id, argv, 6, ...) unchanged; additionally
the temporary object created by createStringObjectFromLongLong(expireat)
(currently assigned to argv[2]) must be released after propagation to avoid a
leak—call the appropriate reference decrement (e.g., decrRefCount) on that
object once alsoPropagate returns (or avoid creating an extra ref by reusing an
existing object) ensuring correct ownership semantics.
📜 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 (1)
src/server.c (1)
src/object.c (1)
createStringObject(102-107)
🪛 GitHub Actions: CI
src/t_hash.c
[error] 2244-2244: Step failed: 'cd src && make 32bit' -> t_hash.c: unused variable 'unused_flag' [-Werror=unused-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 (7)
src/server.h (1)
1328-1328: LGTM!The addition of the
fieldsshared object follows the established pattern for pre-allocated constant strings. This will be used in hash field expiration operations, avoiding repeated allocations for the "FIELDS" string.src/server.c (1)
1988-1991: Looks good: shared FIELDS object initialized consistently.tests/unit/type/hash-field-expire.tcl (1)
1269-1320: LGTM! Well-structured test for HINCRBYFLOAT replication with field expiration.The test comprehensively covers:
- Replication stream validation ensuring HSET+HPEXPIREAT multi-sequence for fields with TTL
- Actual replica state verification confirming TTL preservation and value consistency
src/t_hash.c (4)
713-742: LGTM! Clean implementation of the expiredAt out-parameter.The NULL-safe pattern using a local dummy variable is appropriate, and the comparison change from
>=to>correctly treats fields expiring at exactly the current timestamp as expired.
2202-2203: LGTM!Correctly passes
NULLfor expiredAt since HINCRBY doesn't need to preserve field expiration in this code path (it usesHASH_SET_KEEP_TTLflag instead).
2252-2264: LGTM!Correctly captures the field's expiration time before modification and sets the
has_expirationflag when the field has a valid TTL.
2297-2305: LGTM!The approach of preventing automatic propagation and manually propagating HSET followed by HPEXPIREAT ensures correct replication order for fields with expiration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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 causing build failure.
The pipeline fails due to -Werror=unused-variable for unused_flag which is declared but never used.
🐛 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: Step failed: 'cd src && make 32bit' -> t_hash.c: unused variable 'unused_flag' [-Werror=unused-variable].
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 2242 - 2244, The variable unused_flag is declared
but never used in t_hash.c (see the block with has_expiration, expireat,
unused_flag), causing -Werror=unused-variable; remove the unused_flag
declaration (or if it was meant to be used, implement its usage) so only used
variables remain (e.g., keep has_expiration and expireat, delete the unused_flag
line) to resolve the build failure.
| 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 in argv array declaration.
The array is declared with 5 elements (robj *argv[5]) but code writes to index 5, which is out of bounds. Additionally, argv[2] is created with createStringObjectFromLongLong but never freed, causing a memory leak.
🐛 Proposed fix
- 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]);
}🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 2306 - 2314, The argv buffer overflow occurs
because argv is declared as robj *argv[5] but indices 0..5 are written and
passed to alsoPropagate; change the declaration to robj *argv[6] so it can hold
six elements and keep alsoPropagate(c->db->id, argv, 6, ...) unchanged;
additionally the temporary object created by
createStringObjectFromLongLong(expireat) (currently assigned to argv[2]) must be
released after propagation to avoid a leak—call the appropriate reference
decrement (e.g., decrRefCount) on that object once alsoPropagate returns (or
avoid creating an extra ref by reusing an existing object) ensuring correct
ownership semantics.
2 participants
Benchmark PR from qodo-benchmark#104
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.