Backport more hfe fixes

src/rdb.c

@@ -2237,7 +2237,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) {
                 return NULL;
             }
 
-            if (rdbtype == RDB_TYPE_HASH_2 && itemexpiry > 0) {
+            if (rdbtype == RDB_TYPE_HASH_2 && itemexpiry != EXPIRY_NONE) {
                 hashTypeTrackEntry(o, entry);
             }
         }

src/t_hash.c

@@ -335,7 +335,7 @@ int hashTypeSet(robj *o, sds field, sds value, long long expiry, int flags, bool
      * This is needed for HINCRBY* case since in other commands this is handled early by
      * hashTypeTryConversion, so this check will be a NOP. */
     if (o->encoding == OBJ_ENCODING_LISTPACK) {
-        if (expiry > 0 || sdslen(field) > server.hash_max_listpack_value || sdslen(value) > server.hash_max_listpack_value)
+        if (expiry != EXPIRY_NONE || sdslen(field) > server.hash_max_listpack_value || sdslen(value) > server.hash_max_listpack_value)
             hashTypeConvert(o, OBJ_ENCODING_HASHTABLE);
     }
 
@@ -1326,6 +1326,7 @@ void hsetexCommand(client *c) {
             return;
 
         if (checkAlreadyExpired(when)) {
+            need_rewrite_argv = 1;
             set_expired = 1;
         }
     }
@@ -1396,13 +1397,15 @@ void hsetexCommand(client *c) {
 
     for (i = fields_index; i < c->argc; i += 2) {
         if (set_expired) {
+            hashTypeIgnoreTTL(o, true);
             if (hashTypeDelete(o, c->argv[i]->ptr)) {
                 new_argv[new_argc++] = c->argv[i];
                 incrRefCount(c->argv[i]);
-                /* we treat this case exactly as active expiration. */
-                server.stat_expiredfields++;
                 changes++;
             }
+            /* we treat this case exactly as active expiration. */
+            server.stat_expiredfields++;
+            hashTypeIgnoreTTL(o, false);
         } else {
             bool expired;
             hashTypeSet(o, c->argv[i]->ptr, c->argv[i + 1]->ptr, when, set_flags, &expired);
@@ -1435,40 +1438,31 @@ void hsetexCommand(client *c) {
         if (has_volatile_fields != hashTypeHasVolatileFields(o)) {
             dbUpdateObjectWithVolatileItemsTracking(c->db, o);
         }
-        if (set_expired) {
-            replaceClientCommandVector(c, new_argc, new_argv);
-            /* We would like to reduce the number of hexpired events in case there are potential many expired fields. */
+
+        /* In case we overwritten fields which were expired we need to act as if we actively expired them */
+        if (expired_overwritten > 0) {
+            server.stat_expiredfields += expired_overwritten;
             notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id);
-        } else {
-            /* In case we overwritten fields which were expired we need to act as if we actively expired them */
-            if (expired_overwritten > 0) {
-                server.stat_expiredfields += expired_overwritten;
-                notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id);
-                /* Propagate deletions for expired/non-existent fields in batches.
-                 * When KEEPTTL is used the replica has noway telling if, at the time the primary was executing the command,
-                 * the fields were expired or not. When the replica executes the command it will ALWAYS overwrite the field, so
-                 * we need to propagate hdel explicitly to prevent the replica from keeping the TTL on it's side. */
-                if (keepttl_fields != NULL) {
-                    /* Propagate individual fields deletions */
-                    int idx = 0;
-                    while (idx < expired_overwritten) {
-                        idx += propagateFieldsDeletion(c->db, o, expired_overwritten - idx,
-                                                       &keepttl_fields[idx], c->slot);
-                    }
-                    zfree(keepttl_fields);
-                    keepttl_fields = NULL;
+            /* Propagate deletions for expired/non-existent fields in batches.
+             * When KEEPTTL is used the replica has noway telling if, at the time the primary was executing the command,
+             * the fields were expired or not. When the replica executes the command it will ALWAYS overwrite the field, so
+             * we need to propagate hdel explicitly to prevent the replica from keeping the TTL on it's side. */
+            if (keepttl_fields != NULL) {
+                /* Propagate individual fields deletions */
+                int idx = 0;
+                while (idx < expired_overwritten) {
+                    idx += propagateFieldsDeletion(c->db, o, expired_overwritten - idx,
+                                                   &keepttl_fields[idx], c->slot);
                 }
+                zfree(keepttl_fields);
+                keepttl_fields = NULL;
             }
+        }
 
-            notifyKeyspaceEvent(NOTIFY_HASH, "hset", c->argv[1], c->db->id);
-
-            if (need_rewrite_argv) {
-                replaceClientCommandVector(c, new_argc, new_argv);
-            }
-            if (expire) {
-                notifyKeyspaceEvent(NOTIFY_HASH, "hexpire", c->argv[1], c->db->id);
-            }
+        if (need_rewrite_argv) {
+            replaceClientCommandVector(c, new_argc, new_argv);
         }
+
         signalModifiedKey(c, c->db, c->argv[1]);
         server.dirty += changes;
     } else {
@@ -1478,14 +1472,27 @@ void hsetexCommand(client *c) {
         if (new_argv) zfree(new_argv);
     }
 
+    /* Handle keyspace notifications and object delete if needed.
+     * since setting fields in hash object should always work in case all validations pass,
+     * it is safe to assume that in case we reach this point events should be issues */
+    notifyKeyspaceEvent(NOTIFY_HASH, "hset", c->argv[1], c->db->id);
+    if (expire) {
+        notifyKeyspaceEvent(NOTIFY_HASH, "hexpire", c->argv[1], c->db->id);
+    }
+    if (set_expired) {
+        /* We would like to reduce the number of hexpired events in case there are potential many expired fields. */
+        notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id);
+    }
     /* Delete the object in case it was left empty or created with all expired items. */
     if (hashTypeLength(o) == 0) {
         dbDelete(c->db, c->argv[1]);
         notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id);
     }
     /* make sure that if we ever allocated this it was freed */
     serverAssert(keepttl_fields == NULL);
-    addReplyLongLong(c, changes == num_fields ? 1 : 0);
+    /* In case we reached here we know that we operated on ALL the fields,
+     * even in case we end up in the same original state, we still need to reflect as the operation was done on all the fields. */
+    addReply(c, shared.cone);
 }
 
 /* High-Level Algorithm of HGETEX Command:

tests/unit/hashexpire.tcl

@@ -555,7 +555,7 @@ start_server {tags {"hashexpire"}} {
     } {ERR *}
 
     foreach command {EX PX EXAT PXAT} {
-        test "HSETEX $command 0/past time works correctly with 1 field" {
+        test "HSETEX $command 0/past time works correctly with 2 fields" {
             r FLUSHALL
             r config resetstat
             # Create hash with field
@@ -566,16 +566,16 @@ start_server {tags {"hashexpire"}} {
             set rd [setup_single_keyspace_notification r]
 
             # Set field to expire immediately
-            assert_equal {1} [r HSETEX myhash $command [get_past_zero_expire_value $command] FIELDS 1 f1 v1]
+            assert_equal {1} [r HSETEX myhash $command [get_past_zero_expire_value $command] FIELDS 2 f1 v1 f2 v2]
 
             # Verify field and keys are deleted
-            assert_keyevent_patterns $rd myhash hexpired del
+            assert_keyevent_patterns $rd myhash hset hexpire hexpired del
             assert_equal -2 [r HTTL myhash FIELDS 1 f1]
             assert_equal 0 [r HLEN myhash]
             assert_equal 0 [r EXISTS myhash]
             assert_equal 0 [get_keys r]
             assert_equal 0 [get_keys_with_volatile_items r]
-            assert_equal 1 [info_field [r info stats] expired_fields]
+            assert_equal 2 [info_field [r info stats] expired_fields]
             $rd close
         }
     }
@@ -632,7 +632,6 @@ start_server {tags {"hashexpire"}} {
          assert_error {ERR numfields should be greater than 0 and match the provided number of fields} {r HSETEX myhash PX 100 FIELDS 1 field1 val1 extra}
     } 
 
-
     ## FNX/FXX
 
     # hsetex throws ERR *, it shouldn't
@@ -835,7 +834,7 @@ start_server {tags {"hashexpire"}} {
         r FLUSHALL
         r HSET myhash f1 v1
         set rd [setup_single_keyspace_notification r]
-        
+
         r HEXPIRE myhash 1000 FIELDS 1 f2
         r HEXPIRE myhash 0 FIELDS 1 f2
         # Verify no notification (getting hset and not hexpire)
@@ -844,7 +843,7 @@ start_server {tags {"hashexpire"}} {
         assert_equal 0 [get_keys_with_volatile_items r]
         $rd close
     }
-
+    
     # Error Cases
     test {HEXPIRE - conflicting conditions error} {
         r FLUSHALL
@@ -4662,4 +4661,23 @@ start_server {tags {"hash"}} {
        assert_match {*keys_with_volatile_items=0*} [r INFO keyspace]
        assert_equal {string} [r TYPE myhash]
    }
+
+   test {Zero is a valid ttl in HFE} {
+       r flushall
+       r hset myhash f1 v1
+       assert_equal [r OBJECT ENCODING myhash] "listpack"
+       assert_equal [r hsetex myhash exat 0 fields 2 f2 v2 f3 v3] 1
+       assert_equal [r hlen myhash] 1
+       assert_equal [r OBJECT ENCODING myhash] "listpack"
+       r config set import-mode yes
+       assert_equal [r hsetex myhash exat 0 fields 2 f2 v2 f3 v3] 1
+       assert_equal [r hlen myhash] 3
+       assert_equal [r OBJECT ENCODING myhash] "hashtable"
+       r config set import-mode no
+       wait_for_condition 30 100 {
+           [r hlen myhash] == 1
+       } else {
+           fail "field wasn't expired"
+       }
+   }
 }