Fix HINCRBYFLOAT removes field expiration on replica by tomerqodo · Pull Request #39 · qodo-benchmark/redis-combined-qodo

tomerqodo · 2026-03-10T17:34:46Z

Benchmark PR from agentic-review-benchmarks#8

qodo-code-review · 2026-03-10T17:35:03Z

Review Summary by Qodo

Fix HINCRBYFLOAT removing field expiration on replica

🐞 Bug fix

Walkthroughs

Description

• Fix HINCRBYFLOAT command removing field expiration on replica
• Modify hashTypeGetValue to return field expiration time
• Propagate HINCRBYFLOAT as HSET+HPEXPIREAT when field has expiration
• Add shared object for FIELDS keyword and comprehensive test coverage

Diagram

flowchart LR
  A["HINCRBYFLOAT with expiration"] -->|captures expireat| B["hashTypeGetValue"]
  B -->|returns expiration time| C["hincrbyfloatCommand"]
  C -->|has_expiration=true| D["Propagate HSET + HPEXPIREAT"]
  D -->|maintains expiration| E["Replica field expiration preserved"]

File Changes

1. src/server.c ✨ Enhancement +1/-0

Add shared FIELDS string object

• Add shared.fields string object for FIELDS keyword
• Initialize the shared object in createSharedObjects function

src/server.c

2. src/server.h ✨ Enhancement +1/-1

Declare fields in shared objects struct
• Add *fields pointer to sharedObjectsStruct
• Declare shared FIELDS keyword object in struct definition
src/server.h

3. src/t_hash.c 🐞 Bug fix +39/-11

Preserve field expiration in HINCRBYFLOAT replication

• Modify hashTypeGetValue signature to accept expiredAt output parameter
• Update function to return field expiration time to caller
• Refactor hincrbyfloatCommand to capture and preserve field expiration
• Propagate HINCRBYFLOAT as HSET+HPEXPIREAT commands when field has expiration
• Update all callers of hashTypeGetValue to pass NULL for expiredAt parameter
• Fix expiration check logic from >= to > comparison

src/t_hash.c

View more (1)

4. tests/unit/type/hash-field-expire.tcl 🧪 Tests +52/-0

Test HINCRBYFLOAT field expiration preservation

• Add comprehensive test for HINCRBYFLOAT with field expiration on replica
• Verify replication stream contains HSET+HPEXPIREAT commands
• Test that field expiration is preserved after HINCRBYFLOAT on replica
• Validate field values and TTLs match between primary and replica

tests/unit/type/hash-field-expire.tcl

qodo-code-review · 2026-03-10T17:35:04Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (3) 📎 Requirement gaps (0)

1. unused_flag unused variable 📘 Rule violation ⛯ Reliability

Description

The newly added unused_flag local is never referenced, which will trigger -Wunused-variable and
fail builds when warnings are treated as errors. This violates the requirement to compile cleanly
with -Werror.

Code

src/t_hash.c[2244]
+    int unused_flag = 0;

Evidence

Compliance requires clean compilation under -Werror; the added unused_flag is not used anywhere,
which typically produces an unused-variable warning that becomes an error under -Werror.

AGENTS.md
src/t_hash.c[2242-2245]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`unused_flag` is declared but never used, which can fail compilation when building with `-Werror`.

## Issue Context
The repository requires building cleanly with warnings treated as errors.

## Fix Focus Areas
- src/t_hash.c[2242-2245]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. argv[5] out of bounds 📘 Rule violation ✓ Correctness

Description

The new propagation code declares robj *argv[5] but writes argv[5] and passes 6 arguments,
which is an out-of-bounds write and can also trigger compile-time diagnostics under -Werror. This
violates the requirement to compile cleanly with -Werror.

Code

src/t_hash.c[R2306-2313]

+        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);

Evidence

The compliance rule requires compilation with -Werror; the code writes beyond the declared bounds
of argv, which can trigger -Warray-bounds/similar warnings and is also a concrete correctness
defect.

AGENTS.md
src/t_hash.c[2306-2313]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `argv` array is declared with 5 elements but is populated at index 5 and passed as 6 arguments, causing an out-of-bounds write and potential `-Werror` build failures.

## Issue Context
This occurs in the new manual propagation path added for `HINCRBYFLOAT` when the field has an expiration.

## Fix Focus Areas
- src/t_hash.c[2306-2313]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. hashTypeGetValue not static 📘 Rule violation ⛯ Reliability

Description

The helper function hashTypeGetValue is defined without static, exporting a symbol that appears
intended for file-local use. This violates the requirement that internal helper functions be
declared static to prevent symbol pollution.

Code

src/t_hash.c[R713-715]

GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vstr,
-                             unsigned int *vlen, long long *vll, int hfeFlags) {
-    uint64_t expiredAt;
+             unsigned int *vlen, long long *vll, int hfeFlags, uint64_t *expiredAt)
+{

Evidence

The checklist requires file-local helper functions to be static; the modified function signature
is still defined as a global (non-static) symbol.

AGENTS.md
src/t_hash.c[713-715]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`hashTypeGetValue` is a helper defined without `static`, unnecessarily exporting it as a global symbol.

## Issue Context
The function signature was modified in this PR; if it remains internal to `src/t_hash.c`, it should be declared `static` per the encapsulation requirement.

## Fix Focus Areas
- src/t_hash.c[713-715]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

View more (1)

4. Leaked expire timestamp object 🐞 Bug ⛯ Reliability

Description

hincrbyfloatCommand creates a timestamp object for the propagated HPEXPIREAT
(createStringObjectFromLongLong(expireat)) and never releases it, leaking one robj per call.
Since alsoPropagate increments refcounts, the object will survive after propagation unless
explicitly decremented by the caller.

Code

src/t_hash.c[R2309-2313]

+        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);

Evidence

The command creates a new object for argv[2] and passes it into alsoPropagate. alsoPropagate
increments refcounts, so without a matching decrRefCount by the caller (as done elsewhere in the
codebase), the newly created object remains leaked.

src/t_hash.c[2306-2313]
src/server.c[3322-3334]
src/t_set.c[831-835]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A temporary `robj` is created for the `HPEXPIREAT` timestamp and passed to `alsoPropagate`, but it is never decremented afterward. Because `alsoPropagate` increments refcounts, this leads to a per-call memory leak.

### Issue Context
Other Redis codepaths that create temporary argv objects for `alsoPropagate` explicitly `decrRefCount` them after queuing propagation.

### Fix Focus Areas
- src/t_hash.c[2309-2313]
- src/server.c[3322-3334]
- src/t_set.c[831-835]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

qodo-code-review · 2026-03-10T17:39:37Z

    unsigned int vlen;
+    int has_expiration = 0;
+    uint64_t expireat = EB_EXPIRE_TIME_INVALID;
+    int unused_flag = 0;


1. unused_flag unused variable 📘 Rule violation ⛯ Reliability

The newly added unused_flag local is never referenced, which will trigger -Wunused-variable and fail builds when warnings are treated as errors. This violates the requirement to compile cleanly with -Werror.

Agent Prompt

## Issue description `unused_flag` is declared but never used, which can fail compilation when building with `-Werror`. ## Issue Context The repository requires building cleanly with warnings treated as errors. ## Fix Focus Areas - src/t_hash.c[2242-2245]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-03-10T17:39:37Z

+        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);


2. argv[5] out of bounds 📘 Rule violation ✓ Correctness

The new propagation code declares robj *argv[5] but writes argv[5] and passes 6 arguments, which is an out-of-bounds write and can also trigger compile-time diagnostics under -Werror. This violates the requirement to compile cleanly with -Werror.

Agent Prompt

## Issue description The `argv` array is declared with 5 elements but is populated at index 5 and passed as 6 arguments, causing an out-of-bounds write and potential `-Werror` build failures. ## Issue Context This occurs in the new manual propagation path added for `HINCRBYFLOAT` when the field has an expiration. ## Fix Focus Areas - src/t_hash.c[2306-2313]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-03-10T17:39:38Z

 GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vstr,
-                             unsigned int *vlen, long long *vll, int hfeFlags) {
-    uint64_t expiredAt;
+             unsigned int *vlen, long long *vll, int hfeFlags, uint64_t *expiredAt)
+{


3. hashtypegetvalue not static 📘 Rule violation ⛯ Reliability

The helper function hashTypeGetValue is defined without static, exporting a symbol that appears intended for file-local use. This violates the requirement that internal helper functions be declared static to prevent symbol pollution.

Agent Prompt

## Issue description `hashTypeGetValue` is a helper defined without `static`, unnecessarily exporting it as a global symbol. ## Issue Context The function signature was modified in this PR; if it remains internal to `src/t_hash.c`, it should be declared `static` per the encapsulation requirement. ## Fix Focus Areas - src/t_hash.c[713-715]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-03-10T17:39:38Z

+        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);


4. Leaked expire timestamp object 🐞 Bug ⛯ Reliability

hincrbyfloatCommand creates a timestamp object for the propagated HPEXPIREAT (createStringObjectFromLongLong(expireat)) and never releases it, leaking one robj per call. Since alsoPropagate increments refcounts, the object will survive after propagation unless explicitly decremented by the caller.

Agent Prompt

### Issue description A temporary `robj` is created for the `HPEXPIREAT` timestamp and passed to `alsoPropagate`, but it is never decremented afterward. Because `alsoPropagate` increments refcounts, this leads to a per-call memory leak. ### Issue Context Other Redis codepaths that create temporary argv objects for `alsoPropagate` explicitly `decrRefCount` them after queuing propagation. ### Fix Focus Areas - src/t_hash.c[2309-2313] - src/server.c[3322-3334] - src/t_set.c[831-835]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ShooterIT and others added 5 commits January 25, 2026 12:12

Fix HINCRBYFLOAT removes field expiration on replica

b77a28d

use shared object

1cd8499

fix typo

2ba9b3c

remove unnecessary file

9796dc2

update pr

01bf0c0

qodo-code-review Bot reviewed Mar 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix HINCRBYFLOAT removes field expiration on replica#39

Fix HINCRBYFLOAT removes field expiration on replica#39
tomerqodo wants to merge 5 commits into
qodo_claude_vs_qodo_base_fix_hincrbyfloat_removes_field_expiration_on_replica_pr8from
qodo_claude_vs_qodo_head_fix_hincrbyfloat_removes_field_expiration_on_replica_pr8

tomerqodo commented Mar 10, 2026

Uh oh!

qodo-code-review Bot commented Mar 10, 2026

Uh oh!

qodo-code-review Bot commented Mar 10, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot Mar 10, 2026

Uh oh!

qodo-code-review Bot Mar 10, 2026

Uh oh!

qodo-code-review Bot Mar 10, 2026

Uh oh!

qodo-code-review Bot Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tomerqodo commented Mar 10, 2026

Uh oh!

qodo-code-review Bot commented Mar 10, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review Bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

qodo-code-review Bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qodo-code-review Bot commented Mar 10, 2026 •

edited

Loading