Skip to content

Fix HINCRBYFLOAT removes field expiration on replica #9

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 2 commits into
base: claude_claude_vs_qodo_base_fix_hincrbyfloat_removes_field_expiration_on_replica_pr9
Choose a base branch
from
Open

Fix HINCRBYFLOAT removes field expiration on replica #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d86ac67
Fix HINCRBYFLOAT removes field expiration on replica
ShooterIT Jul 28, 2025
355e36b
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
2 changes: 2 additions & 0 deletions src/server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2127,6 +2127,7 @@ void createSharedObjects(void) {
shared.hpexpireat = createStringObject("HPEXPIREAT",10);
shared.hpersist = createStringObject("HPERSIST",8);
shared.hdel = createStringObject("HDEL",4);
shared.hsetex = createStringObject("HSETEX",6);

/* Shared command argument */
shared.left = createStringObject("left",4);
Expand All @@ -2149,6 +2150,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
4 changes: 2 additions & 2 deletions src/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1526,9 +1526,9 @@ struct sharedObjectsStruct {
*rpop, *lpop, *lpush, *rpoplpush, *lmove, *blmove, *zpopmin, *zpopmax,
*emptyscan, *multi, *exec, *left, *right, *hset, *srem, *xgroup, *xclaim,
*script, *replconf, *eval, *persist, *set, *pexpireat, *pexpire,
*hdel, *hpexpireat, *hpersist,
*hdel, *hpexpireat, *hpersist, *hsetex,
*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
13 changes: 7 additions & 6 deletions src/t_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2556,23 +2556,24 @@ void hincrbyfloatCommand(client *c) {
char buf[MAX_LONG_DOUBLE_CHARS];
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);
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

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

🔴 Bug: HASH_SET_KEEP_TTL was removed from the hashTypeSet call in hincrbyfloatCommand (line 2559), causing field TTL to be cleared on the primary. Since replication now uses HSETEX KEEPTTL which preserves TTL on replicas, this creates primary-replica divergence. The fix is to restore the flag: HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL.

Extended reasoning...

What the bug is

The PR changes the hashTypeSet call in hincrbyfloatCommand from:

hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL);

to:

hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE);

Without HASH_SET_KEEP_TTL, hashTypeSet actively removes any existing field TTL. For listpack encoding (lines 959-964), it calls listpackExUpdateExpiry with HASH_LP_NO_TTL, removing the TTL. For hashtable encoding (lines 989-992), it calls hfieldPersist(o, oldField) which explicitly removes the TTL from the field.

How primary-replica divergence occurs

On the primary, HINCRBYFLOAT now clears the field TTL via hashTypeSet (because HASH_SET_KEEP_TTL is missing). However, the replication is rewritten as HSETEX h1 KEEPTTL FIELDS 1 f1 <value>, which preserves the TTL on replicas. This means after HINCRBYFLOAT on a field with TTL:

  • Primary: field TTL is removed (field becomes persistent)
  • Replica: field TTL is preserved

This is a data consistency violation between primary and replica.

Step-by-step proof

  1. Set a hash field with TTL: HSETEX h1 EX 100 FIELDS 1 f1 10
  2. Run HINCRBYFLOAT h1 f1 1.5 on the primary
  3. On the primary, hashTypeSet is called without HASH_SET_KEEP_TTL → field TTL is cleared → HPTTL h1 FIELDS 1 f1 returns -1
  4. The command is replicated as HSETEX h1 KEEPTTL FIELDS 1 f1 11.5 → on the replica, TTL is preserved → HPTTL h1 FIELDS 1 f1 returns a value in [1, 100000]
  5. The primary and replica now disagree on the field's expiration state

Evidence from the codebase

The analogous hincrbyCommand at line 2506 correctly retains HASH_SET_KEEP_TTL:

hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL);

The existing test {HINCRBYFLOAT - preserve expiration time of the field} would fail with this change, as it sets a 20ms HPEXPIRE, runs HINCRBYFLOAT, then asserts HPTTL is in range [1, 20] — but without HASH_SET_KEEP_TTL, the TTL is cleared and HPTTL returns -1.

Fix

Restore HASH_SET_KEEP_TTL in the hashTypeSet call while keeping the new HSETEX KEEPTTL replication:

hashTypeSet(c->db, o,c->argv[2]->ptr,new,HASH_SET_TAKE_VALUE | HASH_SET_KEEP_TTL);

addReplyBulkCBuffer(c,buf,len);
signalModifiedKey(c,c->db,c->argv[1]);
notifyKeyspaceEvent(NOTIFY_HASH,"hincrbyfloat",c->argv[1],c->db->id);
server.dirty++;

/* Always replicate HINCRBYFLOAT as an HSET command with the final value
/* Always replicate HINCRBYFLOAT as an HSETEX command with the final value
* in order to make sure that differences in float precision or formatting
* will not create differences in replicas or after an AOF restart. */
* will not create differences in replicas or after an AOF restart.
* The KEEPTTL flag is used to make sure the field TTL is preserved. */
robj *newobj;
newobj = createRawStringObject(buf,len);
rewriteClientCommandArgument(c,0,shared.hset);
rewriteClientCommandArgument(c,3,newobj);
rewriteClientCommandVector(c, 6, shared.hsetex, c->argv[1], shared.keepttl,
shared.fields, shared.integers[1], c->argv[2], newobj);
Comment on lines +2571 to +2572
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

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

🔴 rewriteClientCommandVector(c, 6, ...) at line 2571 passes 7 robj* arguments but argc=6, so the function only consumes the first 6 via va_arg — silently dropping newobj (the field value). The replicated command sent to replicas and AOF becomes HSETEX key KEEPTTL FIELDS 1 field (missing the value), which will cause parse errors on replicas and corrupt data on AOF reload. Change 6 to 7.

Extended reasoning...

What the bug is

The rewriteClientCommandVector call in hincrbyfloatCommand (src/t_hash.c:2571-2572) specifies argc=6 but passes 7 robj* variadic arguments:

rewriteClientCommandVector(c, 6, shared.hsetex, c->argv[1], shared.keepttl,
                    shared.fields, shared.integers[1], c->argv[2], newobj);

How rewriteClientCommandVector works

Looking at rewriteClientCommandVector in networking.c:4158-4174, the function allocates an array of exactly argc entries (line 4163) and then loops exactly argc times (line 4165) consuming arguments from the va_list via va_arg. With argc=6, only the first 6 of the 7 provided arguments are consumed. The 7th argument (newobj, the computed float value) is never read.

Step-by-step proof

When a user runs HINCRBYFLOAT h1 f1 1.1, the command computes the new value and rewrites the client command for replication. The 7 arguments passed are:

  1. shared.hsetex → HSETEX
  2. c->argv[1] → h1 (key)
  3. shared.keepttl → KEEPTTL
  4. shared.fields → FIELDS
  5. shared.integers[1] → 1 (field count)
  6. c->argv[2] → f1 (field name)
  7. newobj → 2.1 (computed value) ← DROPPED

Since argc=6, the loop stops after consuming argument #6 (c->argv[2]). The resulting replicated command is: HSETEX h1 KEEPTTL FIELDS 1 f1 — a malformed command missing the value.

Impact

The HSETEX command requires field-value pairs after the FIELDS <count> argument. A command with a field name but no value will be rejected by the HSETEX parser on replicas, causing replication errors. Similarly, AOF files containing this malformed command will fail on reload, potentially causing data loss. Notably, the PR's own test expects the value in the replication stream ({hsetex h1 KEEPTTL FIELDS 1 f1 *}), which would fail with the current argc=6.

Fix

Change argc from 6 to 7 on line 2571:

rewriteClientCommandVector(c, 7, shared.hsetex, c->argv[1], shared.keepttl,
                    shared.fields, shared.integers[1], c->argv[2], newobj);

For reference, the analogous HINCRBY replication path uses rewriteClientCommandArgument (which modifies individual args in-place) and does not have this problem.

decrRefCount(newobj);
}

static GetFieldRes addHashFieldToReply(client *c, kvobj *o, sds field, int hfeFlags) {
GetFieldRes addHashFieldToReply(client *c, kvobj *o, sds field, int hfeFlags) {
if (o == NULL) {
addReplyNull(c);
return GETF_NOT_FOUND;
Expand Down
46 changes: 46 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 @@ -1956,5 +1956,51 @@ start_server {tags {"external:skip needs:debug"}} {
}
close_replication_stream $repl
} {} {needs:repl}

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

r hsetex h1 EX 100 FIELDS 1 f1 1
r hset h1 f2 1
r hincrbyfloat h1 f1 1.1
r hincrbyfloat h1 f2 1.1

# HINCRBYFLOAT will be replicated as HSETEX with KEEPTTL flag
assert_replication_stream $repl {
{select *}
{hsetex h1 PXAT * FIELDS 1 f1 1}
{hset h1 f2 1}
{hsetex h1 KEEPTTL FIELDS 1 f1 *}
{hsetex h1 KEEPTTL FIELDS 1 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 hsetex h1 EX 100 FIELDS 1 f1 1
r -1 hset h1 f2 1
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 on replica either even
# though it was set using HSET with KEEPTTL flag.
assert_equal {-1} [r httl h1 FIELDS 1 f2]
assert_equal [r -1 hget h1 f2] [r hget h1 f2]
}
} {} {needs:repl external:skip}
}
}