Code Review Bench PR #14224 - Fix HINCRBYFLOAT removes field expiration on replica

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

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

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