Sort out the mess around writable replicas and lookupKeyRead/Write

src/db.c

@@ -1530,7 +1530,7 @@ int expireIfNeeded(redisDb *db, robj *key, int expire_on_replica) {
      * commands. */
     if (expire_on_replica) {
         client *c = server.in_eval ? server.lua_client : server.current_client;
-        serverAssert(!c || !c->cmd || (c->cmd->flags & CMD_WRITE));
+        serverAssert(!c || !c->cmd || (c->cmd->flags & (CMD_WRITE|CMD_MODULE)));
     }
 
     if (!keyIsExpired(db,key)) return 0;

src/hyperloglog.c

@@ -1257,7 +1257,14 @@ void pfcountCommand(client *c) {
     /* Case 2: cardinality of the single HLL.
      *
      * The user specified a single key. Either return the cached value
-     * or compute one and update the cache. */
+     * or compute one and update the cache.
+     *
+     * Since a HLL is a regular Redis string type value, updating the cache does
+     * modify the value. We do a lookupKeyRead anyway since this is flagged as a
+     * read-only command. The difference is that with lookupKeyWrite, a
+     * logically expired key on a replica is deleted, while with lookupKeyRead
+     * it isn't, but the lookup returns NULL either way if the key is logically
+     * expired, which is what matters here. */
     o = lookupKeyRead(c->db,c->argv[1]);
     if (o == NULL) {
         /* No key? Cardinality is zero since no element was added, otherwise

tests/integration/replication-3.tcl

@@ -57,61 +57,68 @@ start_server {tags {"repl external:skip"}} {
         } {0}
 
         test {Writable replica doesn't return expired keys} {
+            r select 5
+            assert {[r dbsize] == 0}
+            r debug set-active-expire 0
+            r set key1 5 px 10
+            r set key2 5 px 10
             r -1 select 5
-            assert {[r -1 dbsize] == 0}
+            wait_for_condition 50 100 {
+                [r -1 dbsize] == 2
+            } else {
+                fail "Replication timeout."
+            }
+            after 10
             r -1 config set slave-read-only no
-            r -1 debug set-active-expire 0
-            r -1 set key1 5 px 10
-            r -1 set key2 5 px 10
-            after 50
             assert_equal 2 [r -1 dbsize]    ; # active expire is off
             assert_equal 1 [r -1 incr key1] ; # incr expires and re-creates key1
             assert_equal -1 [r -1 ttl key1] ; # incr created key1 without TTL
             assert_equal {} [r -1 get key2] ; # key2 expired but not deleted
             assert_equal 2 [r -1 dbsize]
             # cleanup
+            r debug set-active-expire 1
             r -1 del key1 key2
-            r -1 debug set-active-expire 1
-            r -1 dbsize
-        } {0}
+            r -1 config set slave-read-only yes
+            r del key1 key2
+        }
 
         test {PFCOUNT updates cache on readonly replica} {
+            r select 5
+            assert {[r dbsize] == 0}
+            r pfadd key a b c d e f g h i j k l m n o p q
+            set strval [r get key]
             r -1 select 5
-            assert {[r -1 dbsize] == 0}
-            r -1 config set slave-read-only no
-            r -1 debug set-active-expire 0
-            r -1 pfadd key a b c d e f g h i j k l m n o p q
-            set strval [r -1 get key]
-            r -1 config set slave-read-only yes
+            wait_for_condition 50 100 {
+                [r -1 dbsize] == 1
+            } else {
+                fail "Replication timeout."
+            }
+            assert {$strval == [r -1 get key]}
             assert_equal 17 [r -1 pfcount key]
-            assert {$strval != [r -1 get key]} ; # pfcount updates cache even
-            ;                                    # on readonly replica
+            assert {$strval != [r -1 get key]}; # cache updated
             # cleanup
-            r -1 config set slave-read-only no
-            r -1 del key
-            r -1 config set slave-read-only yes
-            r -1 debug set-active-expire 1
-            r -1 dbsize
-        } {0}
+            r del key
+        }
 
         test {PFCOUNT doesn't use expired key on readonly replica} {
+            r select 5
+            assert {[r dbsize] == 0}
+            r debug set-active-expire 0
+            r pfadd key a b c d e f g h i j k l m n o p q
+            r pexpire key 10
             r -1 select 5
-            assert {[r -1 dbsize] == 0}
-            r -1 config set slave-read-only no
-            r -1 debug set-active-expire 0
-            r -1 pfadd key a b c d e f g h i j k l m n o p q
-            r -1 pexpire key 10
-            r -1 config set slave-read-only yes
-            after 50
+            wait_for_condition 50 100 {
+                [r -1 dbsize] == 1
+            } else {
+                fail "Replication timeout."
+            }
+            after 10
             assert_equal [r -1 pfcount key] 0 ; # expired key not used
             assert_equal [r -1 dbsize] 1      ; # but it's also not deleted
             # cleanup
-            r -1 config set slave-read-only no
-            r -1 del key
-            r -1 config set slave-read-only yes
-            r -1 debug set-active-expire 1
-            r -1 dbsize
-        } {0}
+            r debug set-active-expire 1
+            r del key
+        }
     }
 }