Skip to content

Fix HINCRBYFLOAT removes field expiration on replica #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomerqodo wants to merge 5 commits into
base: qodo_claude_vs_qodo_base_fix_hincrbyfloat_removes_field_expiration_on_replica_pr8
Choose a base branch
from
Open

Fix HINCRBYFLOAT removes field expiration on replica #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b77a28d
Fix HINCRBYFLOAT removes field expiration on replica
ShooterIT Jul 29, 2025
1cd8499
use shared object
ShooterIT Jul 29, 2025
2ba9b3c
fix typo
ShooterIT Jul 29, 2025
9796dc2
remove unnecessary file
ShooterIT Jul 30, 2025
01bf0c0
update pr
tomerqodo Jan 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1987,6 +1987,7 @@ void createSharedObjects(void) {
shared.special_asterick = createStringObject("*",1);
shared.special_equals = createStringObject("=",1);
shared.redacted = makeObjectShared(createStringObject("(redacted)",10));
shared.fields = createStringObject("FIELDS",6);

for (j = 0; j < OBJ_SHARED_INTEGERS; j++) {
shared.integers[j] =
Expand Down
2 changes: 1 addition & 1 deletion src/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,7 +1325,7 @@ struct sharedObjectsStruct {
*script, *replconf, *eval, *persist, *set, *pexpireat, *pexpire,
*hdel, *hpexpireat,
*time, *pxat, *absttl, *retrycount, *force, *justid, *entriesread,
*lastid, *ping, *setid, *keepttl, *load, *createconsumer,
*lastid, *ping, *setid, *keepttl, *load, *createconsumer, *fields,
*getack, *special_asterick, *special_equals, *default_username, *redacted,
*ssubscribebulk,*sunsubscribebulk, *smessagebulk,
*select[PROTO_SHARED_SELECT_CMDS],
Expand Down
50 changes: 39 additions & 11 deletions src/t_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,24 +706,29 @@ GetFieldRes hashTypeGetFromHashTable(robj *o, sds field, sds *value, uint64_t *e
* If *vll is populated *vstr is set to NULL, so the caller can
* always check the function return by checking the return value
* for GETF_OK and checking if vll (or vstr) is NULL.
* expiredAt - if the field has an expiration time, it will be set to the expiration
* time of the field. Otherwise, will be set to EB_EXPIRE_TIME_INVALID.
*
*/
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)
{
Comment on lines 713 to +715

@qodo-code-review qodo-code-review Bot Mar 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

sds key;
GetFieldRes res;
uint64_t dummy;
if (expiredAt == NULL) expiredAt = &dummy;

if (o->encoding == OBJ_ENCODING_LISTPACK ||
o->encoding == OBJ_ENCODING_LISTPACK_EX) {
*vstr = NULL;
res = hashTypeGetFromListpack(o, field, vstr, vlen, vll, &expiredAt);
res = hashTypeGetFromListpack(o, field, vstr, vlen, vll, expiredAt);

if (res == GETF_NOT_FOUND)
return GETF_NOT_FOUND;

} else if (o->encoding == OBJ_ENCODING_HT) {
sds value = NULL;
res = hashTypeGetFromHashTable(o, field, &value, &expiredAt);
res = hashTypeGetFromHashTable(o, field, &value, expiredAt);

if (res == GETF_NOT_FOUND)
return GETF_NOT_FOUND;
Expand All @@ -734,7 +739,7 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs
serverPanic("Unknown hash encoding");
}

if (expiredAt >= (uint64_t) commandTimeSnapshot())
if (*expiredAt > (uint64_t) commandTimeSnapshot())
return GETF_OK;

if (server.masterhost) {
Expand Down Expand Up @@ -794,7 +799,7 @@ robj *hashTypeGetValueObject(redisDb *db, robj *o, sds field, int hfeFlags, int
long long vll;

if (isHashDeleted) *isHashDeleted = 0;
GetFieldRes res = hashTypeGetValue(db,o,field,&vstr,&vlen,&vll, hfeFlags);
GetFieldRes res = hashTypeGetValue(db,o,field,&vstr,&vlen,&vll, hfeFlags, NULL);

if (res == GETF_OK) {
if (vstr) return createStringObject((char*)vstr,vlen);
Expand Down Expand Up @@ -823,7 +828,7 @@ int hashTypeExists(redisDb *db, robj *o, sds field, int hfeFlags, int *isHashDel
unsigned int vlen = UINT_MAX;
long long vll = LLONG_MAX;

GetFieldRes res = hashTypeGetValue(db, o, field, &vstr, &vlen, &vll, hfeFlags);
GetFieldRes res = hashTypeGetValue(db, o, field, &vstr, &vlen, &vll, hfeFlags, NULL);
if (isHashDeleted)
*isHashDeleted = (res == GETF_EXPIRED_HASH) ? 1 : 0;
return (res == GETF_OK) ? 1 : 0;
Expand Down Expand Up @@ -2195,7 +2200,7 @@ void hincrbyCommand(client *c) {
if ((o = hashTypeLookupWriteOrCreate(c,c->argv[1])) == NULL) return;

GetFieldRes res = hashTypeGetValue(c->db,o,c->argv[2]->ptr,&vstr,&vlen,&value,
HFE_LAZY_EXPIRE);
HFE_LAZY_EXPIRE, NULL);
if (res == GETF_OK) {
if (vstr) {
if (string2ll((char*)vstr,vlen,&value) == 0) {
Expand Down Expand Up @@ -2234,6 +2239,9 @@ void hincrbyfloatCommand(client *c) {
sds new;
unsigned char *vstr;
unsigned int vlen;
int has_expiration = 0;
uint64_t expireat = EB_EXPIRE_TIME_INVALID;
int unused_flag = 0;

@qodo-code-review qodo-code-review Bot Mar 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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


if (getLongDoubleFromObjectOrReply(c,c->argv[3],&incr,NULL) != C_OK) return;
if (isnan(incr) || isinf(incr)) {
Expand All @@ -2242,7 +2250,7 @@ void hincrbyfloatCommand(client *c) {
}
if ((o = hashTypeLookupWriteOrCreate(c,c->argv[1])) == NULL) return;
GetFieldRes res = hashTypeGetValue(c->db, o,c->argv[2]->ptr,&vstr,&vlen,&ll,
HFE_LAZY_EXPIRE);
HFE_LAZY_EXPIRE, &expireat);
if (res == GETF_OK) {
if (vstr) {
if (string2ld((char*)vstr,vlen,&value) == 0) {
Expand All @@ -2252,6 +2260,8 @@ void hincrbyfloatCommand(client *c) {
} else {
value = (long double)ll;
}
/* Field has expiration time. */
if (expireat != EB_EXPIRE_TIME_INVALID) has_expiration = 1;
} else if ((res == GETF_NOT_FOUND) || (res == GETF_EXPIRED)) {
value = 0;
} else {
Expand Down Expand Up @@ -2284,6 +2294,24 @@ void hincrbyfloatCommand(client *c) {
rewriteClientCommandArgument(c,0,shared.hset);
rewriteClientCommandArgument(c,3,newobj);
decrRefCount(newobj);

if (has_expiration) {
/* To make sure that the HSET command is propagated before the HPEXPIREAT,
* we need to prevent the HSET command from being propagated, and then
* propagate both commands manually in the correct order. */
preventCommandPropagation(c);
/* Propagate HSET */
alsoPropagate(c->db->id, c->argv, c->argc, PROPAGATE_AOF|PROPAGATE_REPL);
/* Propagate HPEXPIREAT */
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);
Comment on lines +2306 to +2313

@qodo-code-review qodo-code-review Bot Mar 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Comment on lines +2309 to +2313

@qodo-code-review qodo-code-review Bot Mar 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

}
}

static GetFieldRes addHashFieldToReply(client *c, robj *o, sds field, int hfeFlags) {
Expand All @@ -2296,7 +2324,7 @@ static GetFieldRes addHashFieldToReply(client *c, robj *o, sds field, int hfeFla
unsigned int vlen = UINT_MAX;
long long vll = LLONG_MAX;

GetFieldRes res = hashTypeGetValue(c->db, o, field, &vstr, &vlen, &vll, hfeFlags);
GetFieldRes res = hashTypeGetValue(c->db, o, field, &vstr, &vlen, &vll, hfeFlags, NULL);
if (res == GETF_OK) {
if (vstr) {
addReplyBulkCBuffer(c, vstr, vlen);
Expand Down Expand Up @@ -2408,7 +2436,7 @@ void hstrlenCommand(client *c) {
checkType(c,o,OBJ_HASH)) return;

GetFieldRes res = hashTypeGetValue(c->db, o, c->argv[2]->ptr, &vstr, &vlen, &vll,
HFE_LAZY_EXPIRE);
HFE_LAZY_EXPIRE, NULL);

if (res == GETF_NOT_FOUND || res == GETF_EXPIRED || res == GETF_EXPIRED_HASH) {
addReply(c, shared.czero);
Expand Down
52 changes: 52 additions & 0 deletions tests/unit/type/hash-field-expire.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,5 +1266,57 @@ start_server {tags {"external:skip needs:debug"}} {
assert_equal [dumpAllHashes $primary] [dumpAllHashes $replica]
}
}

test "HINCRBYFLOAT command won't remove field expiration on replica ($type)" {
r flushall
set repl [attach_to_replication_stream]

r hset h1 f1 1
r hset h1 f2 1
r hexpire h1 100 FIELDS 1 f1
r hincrbyfloat h1 f1 1.1
r hincrbyfloat h1 f2 1.1

# HINCRBYFLOAT will be replicated as HSET if no expiration time is set.
# Otherwise it will be replicated as HSET+HPEXPIREAT multi command.
assert_replication_stream $repl {
{select *}
{hset h1 f1 1}
{hset h1 f2 1}
{hpexpireat h1 * FIELDS 1 f1}
{multi}
{hset h1 f1 *}
{hpexpireat h1 * FIELDS 1 f1}
{exec}
{hset h1 f2 *}
}
close_replication_stream $repl

start_server {tags {external:skip}} {
r -1 flushall
r slaveof [srv -1 host] [srv -1 port]
wait_for_sync r

r -1 hset h1 f1 1
r -1 hset h1 f2 1
r -1 hexpire h1 100 FIELDS 1 f1
wait_for_ofs_sync [srv -1 client] [srv 0 client]
assert_range [r httl h1 FIELDS 1 f1] 90 100
assert_equal {-1} [r httl h1 FIELDS 1 f2]

r -1 hincrbyfloat h1 f1 1.1
r -1 hincrbyfloat h1 f2 1.1

# Expiration time should not be removed on replica and the value
# should be equal to the master.
wait_for_ofs_sync [srv -1 client] [srv 0 client]
assert_range [r httl h1 FIELDS 1 f1] 90 100
assert_equal [r -1 hget h1 f1] [r hget h1 f1]

# The field f2 should not have any expiration time on replica either.
assert_equal {-1} [r httl h1 FIELDS 1 f2]
assert_equal [r -1 hget h1 f2] [r hget h1 f2]
}
} {} {needs:repl external:skip}
}
}
Loading