From b0fdd5b360d9448724248c92bea3b8bef32c7241 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sun, 1 Nov 2026 23:36:11 +0100 Subject: [PATCH 1/2] perf: pipeline HMSET and EXPIREAT in Redis and Predis cache handlers Send HMSET and EXPIREAT together in a single pipeline round trip rather than issuing two sequential network requests on every cache save. For normal cache usage, behavior and data structures remain identical. In error scenarios where HMSET fails (e.g. key contains a non-hash type), the pipelined EXPIREAT will still execute on the existing key, while save() returns false as expected. --- system/Cache/Handlers/PredisHandler.php | 15 ++++++++++----- system/Cache/Handlers/RedisHandler.php | 19 ++++++++++++++----- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/system/Cache/Handlers/PredisHandler.php b/system/Cache/Handlers/PredisHandler.php index 79e7ae98f916..bc7a092441af 100644 --- a/system/Cache/Handlers/PredisHandler.php +++ b/system/Cache/Handlers/PredisHandler.php @@ -118,15 +118,20 @@ public function save(string $key, mixed $value, int $ttl = 60): bool return false; } - if (! $this->redis->hmset($key, ['__ci_type' => $dataType, '__ci_value' => $value]) instanceof Status) { - return false; - } + $fields = ['__ci_type' => $dataType, '__ci_value' => $value]; if ($ttl !== 0) { - $this->redis->expireat($key, Time::now()->getTimestamp() + $ttl); + // Send HMSET and EXPIREAT in a single round trip. + $expiresAt = Time::now()->getTimestamp() + $ttl; + $results = $this->redis->pipeline(static function ($pipe) use ($key, $fields, $expiresAt): void { + $pipe->hmset($key, $fields); + $pipe->expireat($key, $expiresAt); + }); + + return is_array($results) && ($results[0] ?? null) instanceof Status; } - return true; + return $this->redis->hmset($key, $fields) instanceof Status; } public function delete(string $key): bool diff --git a/system/Cache/Handlers/RedisHandler.php b/system/Cache/Handlers/RedisHandler.php index ccbb29bcd032..3e62f2b1ab83 100644 --- a/system/Cache/Handlers/RedisHandler.php +++ b/system/Cache/Handlers/RedisHandler.php @@ -134,15 +134,24 @@ public function save(string $key, mixed $value, int $ttl = 60): bool return false; } - if (! $this->redis->hMset($key, ['__ci_type' => $dataType, '__ci_value' => $value])) { - return false; - } + $fields = ['__ci_type' => $dataType, '__ci_value' => $value]; if ($ttl !== 0) { - $this->redis->expireAt($key, Time::now()->getTimestamp() + $ttl); + // Send HMSET and EXPIREAT in a single round trip. + $pipeline = $this->redis->multi(Redis::PIPELINE); + + if ($pipeline === false) { + return false; + } + + $pipeline->hMset($key, $fields); + $pipeline->expireAt($key, Time::now()->getTimestamp() + $ttl); + $results = $pipeline->exec(); + + return is_array($results) && (bool) ($results[0] ?? false); } - return true; + return (bool) $this->redis->hMset($key, $fields); } public function delete(string $key): bool From 92844d1aae538131658c1ef79c8d58a512e3db83 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Thu, 5 Nov 2026 20:52:31 +0100 Subject: [PATCH 2/2] fix: make expiration conditional on successful save in Redis and Predis handlers Execute HMSET and EXPIREAT atomically via a Lua script so that EXPIREAT is never executed when HMSET fails (e.g. WRONGTYPE error on non-hash keys). Falls back to sequential execution if EVAL is unsupported or blocked. Adds regression tests verifying non-hash keys preserve value and TTL. --- system/Cache/Handlers/PredisHandler.php | 39 ++++++++++++++++--- system/Cache/Handlers/RedisHandler.php | 38 +++++++++++++----- .../Cache/Handlers/PredisHandlerTest.php | 22 +++++++++++ .../Cache/Handlers/RedisHandlerTest.php | 22 +++++++++++ 4 files changed, 105 insertions(+), 16 deletions(-) diff --git a/system/Cache/Handlers/PredisHandler.php b/system/Cache/Handlers/PredisHandler.php index bc7a092441af..2eae6dda20a6 100644 --- a/system/Cache/Handlers/PredisHandler.php +++ b/system/Cache/Handlers/PredisHandler.php @@ -20,12 +20,26 @@ use Predis\Client; use Predis\Collection\Iterator\Keyspace; use Predis\Response\Status; +use Throwable; /** * Predis cache handler */ class PredisHandler extends BaseHandler { + /** + * Lua script to conditionally execute HMSET and EXPIREAT in a single round trip. + * Prevents EXPIREAT from mutating TTL or expiring keys when HMSET fails (e.g. WRONGTYPE). + */ + private const SAVE_SCRIPT = <<<'LUA' + local res = redis.pcall('HMSET', KEYS[1], '__ci_type', ARGV[2], '__ci_value', ARGV[3]) + if type(res) == 'table' and res.err then + return 0 + end + redis.call('EXPIREAT', KEYS[1], ARGV[1]) + return 1 + LUA; + /** * Default config * @@ -121,14 +135,27 @@ public function save(string $key, mixed $value, int $ttl = 60): bool $fields = ['__ci_type' => $dataType, '__ci_value' => $value]; if ($ttl !== 0) { - // Send HMSET and EXPIREAT in a single round trip. $expiresAt = Time::now()->getTimestamp() + $ttl; - $results = $this->redis->pipeline(static function ($pipe) use ($key, $fields, $expiresAt): void { - $pipe->hmset($key, $fields); - $pipe->expireat($key, $expiresAt); - }); - return is_array($results) && ($results[0] ?? null) instanceof Status; + try { + return (bool) $this->redis->eval( + self::SAVE_SCRIPT, + 1, + $key, + (string) $expiresAt, + $dataType, + (string) $value, + ); + } catch (Throwable) { + // If EVAL is unsupported or blocked, fallback to sequential execution. + if (! $this->redis->hmset($key, $fields) instanceof Status) { + return false; + } + + $this->redis->expireat($key, $expiresAt); + + return true; + } } return $this->redis->hmset($key, $fields) instanceof Status; diff --git a/system/Cache/Handlers/RedisHandler.php b/system/Cache/Handlers/RedisHandler.php index 3e62f2b1ab83..e008a35c59c4 100644 --- a/system/Cache/Handlers/RedisHandler.php +++ b/system/Cache/Handlers/RedisHandler.php @@ -18,12 +18,26 @@ use Config\Cache; use Redis; use RedisException; +use Throwable; /** * Redis cache handler */ class RedisHandler extends BaseHandler { + /** + * Lua script to conditionally execute HMSET and EXPIREAT in a single round trip. + * Prevents EXPIREAT from mutating TTL or expiring keys when HMSET fails (e.g. WRONGTYPE). + */ + private const SAVE_SCRIPT = <<<'LUA' + local res = redis.pcall('HMSET', KEYS[1], '__ci_type', ARGV[2], '__ci_value', ARGV[3]) + if type(res) == 'table' and res.err then + return 0 + end + redis.call('EXPIREAT', KEYS[1], ARGV[1]) + return 1 + LUA; + /** * Default config * @@ -137,18 +151,22 @@ public function save(string $key, mixed $value, int $ttl = 60): bool $fields = ['__ci_type' => $dataType, '__ci_value' => $value]; if ($ttl !== 0) { - // Send HMSET and EXPIREAT in a single round trip. - $pipeline = $this->redis->multi(Redis::PIPELINE); + $expiresAt = Time::now()->getTimestamp() + $ttl; - if ($pipeline === false) { - return false; + try { + return (bool) $this->redis->eval( + self::SAVE_SCRIPT, + [$key, $expiresAt, $dataType, (string) $value], + 1, + ); + } catch (Throwable) { + // If EVAL is unsupported or blocked, fallback to sequential execution. + if (! $this->redis->hMset($key, $fields)) { + return false; + } + + return $this->redis->expireAt($key, $expiresAt); } - - $pipeline->hMset($key, $fields); - $pipeline->expireAt($key, Time::now()->getTimestamp() + $ttl); - $results = $pipeline->exec(); - - return is_array($results) && (bool) ($results[0] ?? false); } return (bool) $this->redis->hMset($key, $fields); diff --git a/tests/system/Cache/Handlers/PredisHandlerTest.php b/tests/system/Cache/Handlers/PredisHandlerTest.php index 91949f53238f..dbe70d112bd7 100644 --- a/tests/system/Cache/Handlers/PredisHandlerTest.php +++ b/tests/system/Cache/Handlers/PredisHandlerTest.php @@ -18,6 +18,7 @@ use CodeIgniter\I18n\Time; use Config\Cache; use PHPUnit\Framework\Attributes\Group; +use Predis\Client; /** * @internal @@ -196,4 +197,25 @@ public function testReconnect(): void $this->assertSame('value', $this->handler->get(self::$key1)); } + + public function testSaveFailsOnNonHashKeyPreservingValueAndTtl(): void + { + /** @var Client $redis */ + $redis = $this->getPrivateProperty($this->handler, 'redis'); + + $prefixedKey = $this->config->prefix . 'non_hash_key'; + $redis->set($prefixedKey, 'raw_string_value'); + $redis->expire($prefixedKey, 300); + + $this->assertFalse($this->handler->save('non_hash_key', 'new_value', 60)); + + $this->assertSame('raw_string_value', $redis->get($prefixedKey)); + $this->assertGreaterThan(250, $redis->ttl($prefixedKey)); + + // Handler remains usable for subsequent operations + $this->assertTrue($this->handler->save(self::$key1, 'usable_value')); + $this->assertSame('usable_value', $this->handler->get(self::$key1)); + + $redis->del([$prefixedKey]); + } } diff --git a/tests/system/Cache/Handlers/RedisHandlerTest.php b/tests/system/Cache/Handlers/RedisHandlerTest.php index 1f85ecc28349..4dbd06452169 100644 --- a/tests/system/Cache/Handlers/RedisHandlerTest.php +++ b/tests/system/Cache/Handlers/RedisHandlerTest.php @@ -19,6 +19,7 @@ use Config\Cache; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; +use Redis; /** * @internal @@ -238,4 +239,25 @@ public function testReconnect(): void $this->assertSame('value', $this->handler->get(self::$key1)); } + + public function testSaveFailsOnNonHashKeyPreservingValueAndTtl(): void + { + /** @var Redis $redis */ + $redis = $this->getPrivateProperty($this->handler, 'redis'); + + $prefixedKey = $this->config->prefix . 'non_hash_key'; + $redis->set($prefixedKey, 'raw_string_value'); + $redis->expire($prefixedKey, 300); + + $this->assertFalse($this->handler->save('non_hash_key', 'new_value', 60)); + + $this->assertSame('raw_string_value', $redis->get($prefixedKey)); + $this->assertGreaterThan(250, $redis->ttl($prefixedKey)); + + // Handler remains usable for subsequent operations + $this->assertTrue($this->handler->save(self::$key1, 'usable_value')); + $this->assertSame('usable_value', $this->handler->get(self::$key1)); + + $redis->del($prefixedKey); + } }