Fix HINCRBYFLOAT removes field expiration on replica#40
Conversation
Review Summary by QodoFix HINCRBYFLOAT field expiration removal on replica
WalkthroughsDescription• Fix HINCRBYFLOAT removing field expiration on replica • Replicate HINCRBYFLOAT as HSETEX with KEEPTTL flag • Add shared objects for HSETEX and FIELDS commands • Add comprehensive test for field expiration preservation Diagramflowchart LR
A["HINCRBYFLOAT command"] -->|Previously replicated as| B["HSET without TTL"]
A -->|Now replicated as| C["HSETEX with KEEPTTL"]
C -->|Preserves| D["Field expiration on replica"]
E["Shared objects"] -->|Added| F["hsetex and fields"]
G["Test coverage"] -->|Added| H["Replica expiration validation"]
File Changes1. src/server.c
|
Code Review by Qodo
1. addHashFieldToReply missing static
|
Author
|
/agentic_review |
|
Persistent review updated to latest commit 355e36b |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| } | ||
|
|
||
| static GetFieldRes addHashFieldToReply(client *c, kvobj *o, sds field, int hfeFlags) { | ||
| GetFieldRes addHashFieldToReply(client *c, kvobj *o, sds field, int hfeFlags) { |
There was a problem hiding this comment.
1. addhashfieldtoreply missing static 📘 Rule violation ⛯ Reliability
addHashFieldToReply is now declared without static even though it is only used within src/t_hash.c, unnecessarily exporting the symbol. This violates the internal-helper encapsulation requirement and may also trigger -Wmissing-prototypes style warnings when building with -Werror.
Agent Prompt
## Issue description
`addHashFieldToReply` was changed from `static` to non-static, which exports an internal helper symbol.
## Issue Context
Per compliance requirements, helper functions only used within a single `.c` file must be declared `static` to avoid symbol pollution and potential build warnings.
## Fix Focus Areas
- src/t_hash.c[2576-2576]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2571
to
+2572
| rewriteClientCommandVector(c, 6, shared.hsetex, c->argv[1], shared.keepttl, | ||
| shared.fields, shared.integers[1], c->argv[2], newobj); |
There was a problem hiding this comment.
2. Malformed replicated hsetex 🐞 Bug ✓ Correctness
hincrbyfloatCommand rewrites propagation to HSETEX but calls rewriteClientCommandVector with argc=6 while providing 7 arguments, so the final value argument is not included in the rewritten command. Replicas/AOF will receive an invalid HSETEX (missing the value) and fail with an arity error when parsing FIELDS.
Agent Prompt
### Issue description
`hincrbyfloatCommand` rewrites replication/AOF to `HSETEX ... KEEPTTL FIELDS 1 <field> <value>`, but currently calls `rewriteClientCommandVector` with `argc=6` while supplying 7 `robj*` arguments. Since `rewriteClientCommandVector` only copies `argc` objects, the last argument (the value) is dropped, producing a malformed `HSETEX` that fails parsing on replicas/AOF.
### Issue Context
`rewriteClientCommandVector(client *c, int argc, ...)` consumes exactly `argc` varargs.
`HSETEX` requires `FIELDS <numfields> field value ...`; omitting the value triggers an arity error.
### Fix Focus Areas
- src/t_hash.c[2569-2573]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| int len = ld2string(buf,sizeof(buf),value,LD_STR_HUMAN); | ||
| new = sdsnewlen(buf,len); | ||
| hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL); | ||
| hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE); |
There was a problem hiding this comment.
3. Hincrbyfloat drops field ttl 🐞 Bug ✓ Correctness
hincrbyfloatCommand updates the field via hashTypeSet without HASH_SET_KEEP_TTL, which causes existing per-field expiration metadata to be cleared when overwriting the value. This makes the master’s field TTL behavior diverge (and can remove expirations) despite the intention to preserve TTL via KEEPTTL in propagation.
Agent Prompt
### Issue description
`hincrbyfloatCommand` now calls `hashTypeSet(..., HASH_SET_TAKE_VALUE)` without `HASH_SET_KEEP_TTL`. In `hashTypeSet`, absence of `HASH_SET_KEEP_TTL` actively clears existing per-field TTL metadata. This means HINCRBYFLOAT can remove field expirations on the master during the local write.
### Issue Context
`hashTypeSet` clears attached field TTL when `HASH_SET_KEEP_TTL` is not set (listpack_ex and HT encodings).
`hincrbyCommand` still uses `HASH_SET_KEEP_TTL`, so increment-style updates are expected to preserve field TTL.
### Fix Focus Areas
- src/t_hash.c[2556-2561]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#9