From db4fc2a83309bf8b65e25deedfac0ff71d67e4b8 Mon Sep 17 00:00:00 2001 From: Yuan Wang Date: Mon, 28 Jul 2025 21:09:46 +0800 Subject: [PATCH] Fix HINCRBYFLOAT removes field expiration on replica (#14224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #14218 Before, we replicate HINCRBYFLOAT as an HSET 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. However, on the replica side, if the field has an expiration time, HSET will remove it, even though the master retains it. This leads to inconsistencies between the master and the replica. To address this, we now use the HSETEX command with the KEEPTTL flag instead of HSET, ensuring that the field’s TTL is preserved. This bug was introduced in version 7.4, but the HSETEX command was only implemented from version 8.0. Therefore, this patch does not fix the issue in the 7.4 branch, a separate commit is needed to address it in 7.4. --- src/server.c | 2 ++ src/server.h | 4 +-- src/t_hash.c | 9 +++--- tests/unit/type/hash-field-expire.tcl | 46 +++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 6 deletions(-) diff --git a/src/server.c b/src/server.c index 00e9c35794..a144001b14 100644 --- a/src/server.c +++ b/src/server.c @@ -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); @@ -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] = diff --git a/src/server.h b/src/server.h index 056b2abd22..2a9a56fda6 100644 --- a/src/server.h +++ b/src/server.h @@ -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], diff --git a/src/t_hash.c b/src/t_hash.c index 86f3129b97..7c0d504388 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -2562,13 +2562,14 @@ void hincrbyfloatCommand(client *c) { 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, 7, shared.hsetex, c->argv[1], shared.keepttl, + shared.fields, shared.integers[1], c->argv[2], newobj); decrRefCount(newobj); } diff --git a/tests/unit/type/hash-field-expire.tcl b/tests/unit/type/hash-field-expire.tcl index 87abe2a46c..a94a6b303f 100644 --- a/tests/unit/type/hash-field-expire.tcl +++ b/tests/unit/type/hash-field-expire.tcl @@ -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} } }