Fix HINCRBYFLOAT removes field expiration on replica (redis#14224)

Fixes redis#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.

Author: ShooterIT

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] =

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],

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

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}
     }
 }