Fix the echo liveness check when in sentinel mode.

The current echo liveness check was doing one big complex conditional
trying to incorporate both sentinel's expected ERR no such command
response and non-sentinel's actual bulk reply to ECHO.

This commit refactors the logic to check the echo response into a little
helper with different logic depending on whether or not we're connected
to a sentinel.

Additionally, we add a test to verify that we are in fact reusing
persistent connections when the user requests a persistent connection
with `RedisSentinel`.

Fixes #2148

library.c

@@ -2946,10 +2946,30 @@ static int redis_stream_detect_dirty(php_stream *stream) {
     return rv == 0 ? SUCCESS : FAILURE;
 }
 
+static inline zend_bool
+redis_check_echo_response(RedisSock *redis_sock, char *hdr, const char *id,
+                          size_t idlen)
+{
+    char buf[256] = {0};
+    size_t len;
+
+    /* Sentinel doesn't have ECHO but it will contain the ID in the error
+     * message so just check that */
+    if (redis_sock->sentinel) {
+        return redis_strncmp(hdr, ZEND_STRL("-ERR unknown command")) == 0 &&
+               strstr(hdr, id) != NULL;
+    }
+
+    /* Non-sentinel: Read and verify the ID */
+    return *hdr == TYPE_BULK && atoi(hdr + 1) == idlen &&
+           redis_sock_gets(redis_sock, buf, sizeof(buf) - 1, &len) == 0 &&
+           redis_strncmp(buf, id, idlen) == 0;
+}
+
 static int
 redis_sock_check_liveness(RedisSock *redis_sock)
 {
-    char id[64], inbuf[4096];
+    char id[64], inbuf[256] = {0};
     int idlen, auth;
     smart_string cmd = {0};
     size_t len;
@@ -3015,18 +3035,10 @@ redis_sock_check_liveness(RedisSock *redis_sock)
         }
     }
 
-    /* check echo response */
-    if ((redis_sock->sentinel && (
-        redis_strncmp(inbuf, ZEND_STRL("-ERR unknown command")) != 0 ||
-        strstr(inbuf, id) == NULL
-    )) || *inbuf != TYPE_BULK || atoi(inbuf + 1) != idlen ||
-        redis_sock_gets(redis_sock, inbuf, sizeof(inbuf) - 1, &len) < 0 ||
-        redis_strncmp(inbuf, id, idlen) != 0
-    ) {
-        goto failure;
+    if (redis_check_echo_response(redis_sock, inbuf, id, idlen)) {
+        return SUCCESS;
     }
 
-    return SUCCESS;
 failure:
     redis_sock->status = REDIS_SOCK_STATUS_DISCONNECTED;
     if (redis_sock->stream) {

tests/RedisSentinelTest.php

@@ -116,4 +116,45 @@ class Redis_Sentinel_Test extends TestSuite
             $this->checkFields($slave);
         }
     }
+
+    protected function getClients(Redis $redis, string $cmd)
+    {
+        $result = [];
+
+        foreach ($redis->client('list') as $client) {
+            if ($client['cmd'] !== $cmd)
+                continue;
+
+            $result[] = $client['id'];
+        }
+
+        return $result;
+    }
+
+    public function testPersistent() {
+        /* I think the tests just use the default port */
+        $redis = new Redis;
+        $redis->connect($this->getHost(), 26379);
+
+        $id = null;
+
+        for ($i = 0; $i < 3; $i++) {
+            $sentinel = new RedisSentinel([
+                'host' => $this->getHost(),
+                'persistent' => 'sentinel',
+            ]);
+
+            $this->assertTrue($sentinel->ping());
+
+            $clients = $this->getClients($redis, 'ping');
+
+            /* Capture the ping client */
+            $id ??= $clients[0];
+
+            unset($sentinel);
+        }
+
+        /* The same client should have been reused */
+        $this->assertEquals($id, $clients[0]);
+    }
 }