From 75a9c1f7992b5e292853918fd273a3f67c481386 Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 12 Jul 2026 14:27:10 -0400 Subject: [PATCH 1/4] fix(memcache): return EXPIRE result and centralize TTL normalization TTL handling was duplicated across multiple methods, while `compareSetTTL()` used slightly different behavior and always reported success when the compare matched, even though the Lua script ignored the actual EXPIRE result. This change makes the behavior more consistent and makes the return value of `compareSetTTL()` more truthful. Note: Negative TTL values are still passed through intentionally. Signed-off-by: Josh --- lib/private/Memcache/Redis.php | 36 ++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/lib/private/Memcache/Redis.php b/lib/private/Memcache/Redis.php index edf9a80aad497..9b1b9b1338685 100644 --- a/lib/private/Memcache/Redis.php +++ b/lib/private/Memcache/Redis.php @@ -32,8 +32,8 @@ class Redis extends Cache implements IMemcacheTTL { '75526f8048b13ce94a41b58eee59c664b4990ab2', ], 'caSetTtl' => [ - 'if redis.call("get", KEYS[1]) == ARGV[1] then redis.call("expire", KEYS[1], ARGV[2]) return 1 else return 0 end', - 'fa4acbc946d23ef41d7d3910880b60e6e4972d72', + 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("expire", KEYS[1], ARGV[2]) else return 0 end' + '94eac401502554c02b811e3199baddde62d976d4', ], ]; @@ -65,14 +65,23 @@ public function get($key) { return self::decodeValue($result); } - #[\Override] - public function set($key, $value, $ttl = 0) { - $value = self::encodeValue($value); + private static function normalizeTtl(int $ttl): int { if ($ttl === 0) { // having infinite TTL can lead to leaked keys as the prefix changes with version upgrades - $ttl = self::DEFAULT_TTL; + return self::DEFAULT_TTL; } - $ttl = min($ttl, self::MAX_TTL); + + if ($ttl > 0) { + return min($ttl, self::MAX_TTL); + } + + return $ttl; + } + + #[\Override] + public function set($key, $value, $ttl = 0) { + $value = self::encodeValue($value); + $ttl = self::normalizeTtl((int)$ttl); return $this->getCache()->setex($this->getPrefix() . $key, $ttl, $value); } @@ -111,11 +120,7 @@ public function clear($prefix = '') { #[\Override] public function add($key, $value, $ttl = 0) { $value = self::encodeValue($value); - if ($ttl === 0) { - // having infinite TTL can lead to leaked keys as the prefix changes with version upgrades - $ttl = self::DEFAULT_TTL; - } - $ttl = min($ttl, self::MAX_TTL); + $ttl = self::normalizeTtl((int)$ttl); $args = ['nx']; $args['ex'] = $ttl; @@ -187,11 +192,7 @@ public function ncad(string $key, mixed $old): bool { #[\Override] public function setTTL($key, $ttl) { - if ($ttl === 0) { - // having infinite TTL can lead to leaked keys as the prefix changes with version upgrades - $ttl = self::DEFAULT_TTL; - } - $ttl = min($ttl, self::MAX_TTL); + $ttl = self::normalizeTtl((int)$ttl); $this->getCache()->expire($this->getPrefix() . $key, $ttl); } @@ -204,6 +205,7 @@ public function getTTL(string $key): int|false { #[\Override] public function compareSetTTL(string $key, mixed $value, int $ttl): bool { $value = self::encodeValue($value); + $ttl = self::normalizeTtl($ttl); return $this->evalLua('caSetTtl', [$key], [$value, $ttl]) > 0; } From 5695a09a111fb753683eb3d86a81234fa8eb57db Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 13 Jul 2026 11:25:30 -0400 Subject: [PATCH 2/4] test(memcache): expand Redis compareSetTTL coverage and clarify test names Assisted-by: Copilot:gpt-5.4 Signed-off-by: Josh --- tests/lib/Memcache/RedisTest.php | 52 ++++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/tests/lib/Memcache/RedisTest.php b/tests/lib/Memcache/RedisTest.php index d410dac6686ae..f355af1ad95c0 100644 --- a/tests/lib/Memcache/RedisTest.php +++ b/tests/lib/Memcache/RedisTest.php @@ -65,23 +65,63 @@ protected function setUp(): void { $this->instance = new Redis($this->getUniqueID()); } + private function assertTtlInRange(int $expected, int|false $actual): void { + $this->assertNotFalse($actual); + // allow for 1s of inaccuracy due to time moving forward + $this->assertGreaterThanOrEqual($expected - 1, $actual); + $this->assertLessThanOrEqual($expected, $actual); + } + public function testScriptHashes(): void { foreach (Redis::LUA_SCRIPTS as $script) { $this->assertEquals(sha1($script[0]), $script[1]); } } - public function testCasTtlNotChanged(): void { + public function testCompareSetTtlUpdatesTtlWhenValueMatches(): void { $this->instance->set('foo', 'bar', 50); + $this->assertTrue($this->instance->compareSetTTL('foo', 'bar', 100)); - // allow for 1s of inaccuracy due to time moving forward - $this->assertLessThan(1, 100 - $this->instance->getTTL('foo')); + + $ttl = $this->instance->getTTL('foo'); + $this->assertTtlInRange(100, $ttl); } - public function testCasTtlChanged(): void { + public function testCompareSetTtlDoesNotUpdateTtlWhenValueDiffers(): void { $this->instance->set('foo', 'bar1', 50); + $this->assertFalse($this->instance->compareSetTTL('foo', 'bar', 100)); - // allow for 1s of inaccuracy due to time moving forward - $this->assertLessThan(1, 50 - $this->instance->getTTL('foo')); + + $ttl = $this->instance->getTTL('foo'); + $this->assertTtlInRange(50, $ttl); + } + + public function testCompareSetTtlZeroUsesDefaultTtl(): void { + $this->instance->set('foo', 'bar', 50); + + $this->assertTrue($this->instance->compareSetTTL('foo', 'bar', 0)); + + $ttl = $this->instance->getTTL('foo'); + $this->assertTtlInRange(Redis::DEFAULT_TTL, $ttl); + } + + public function testCompareSetTtlIsClampedToMaxTtl(): void { + $expectedMaxTtl = 30 * 24 * 60 * 60; + + $this->instance->set('foo', 'bar', 50); + + $this->assertTrue($this->instance->compareSetTTL('foo', 'bar', $expectedMaxTtl + 1000)); + + $ttl = $this->instance->getTTL('foo'); + $this->assertTtlInRange($expectedMaxTtl, $ttl); + } + + public function testCompareSetTtlNegativeExpiresKeyImmediately(): void { + $this->instance->set('foo', 'bar', 50); + + $this->assertTrue($this->instance->compareSetTTL('foo', 'bar', -1)); + $this->assertFalse($this->instance->hasKey('foo')); + $this->assertNull($this->instance->get('foo')); + $this->assertFalse($this->instance->getTTL('foo')); } } From 03efc36ab301aeb3711ed21c2c2e5bfa62d0fb3f Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 13 Jul 2026 11:51:28 -0400 Subject: [PATCH 3/4] docs(memcache): match IMemcacheTTL docs to implementation Signed-off-by: Josh --- lib/public/IMemcacheTTL.php | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/lib/public/IMemcacheTTL.php b/lib/public/IMemcacheTTL.php index 5f70bc3b8ebb2..737d618291462 100644 --- a/lib/public/IMemcacheTTL.php +++ b/lib/public/IMemcacheTTL.php @@ -9,34 +9,45 @@ namespace OCP; /** - * Interface for memcache backends that support setting ttl after the value is set + * Interface for memcache backends that can update and retrieve the TTL of an + * existing cache entry. * * @since 8.2.2 */ interface IMemcacheTTL extends IMemcache { /** - * Set the ttl for an existing value + * Update the TTL of an existing cache entry. * - * @param string $key - * @param int $ttl time to live in seconds + * Implementations may treat a non-positive TTL as immediate expiry. + * + * @param string $key Cache key + * @param int $ttl TTL in seconds. 0 uses the backend default TTL, negative + * values expire the entry immediately. * @since 8.2.2 */ public function setTTL(string $key, int $ttl); /** - * Get the ttl for an existing value, in seconds till expiry + * Get the remaining TTL of an existing cache entry. * - * @return int|false - * @since 27 + * @param string $key Cache key + * @return int Remaining TTL in whole seconds, or false if the key has no + * positive remaining TTL or does not exist + * @since 27.0.0 */ public function getTTL(string $key): int|false; + /** - * Set the ttl for an existing value if the value matches + * Atomically update the TTL if the existing value matches. + * + * Implementations may treat a non-positive TTL as immediate expiry. * - * @param string $key - * @param mixed $value - * @param int $ttl time to live in seconds - * @since 27 + * @param string $key Cache key + * @param mixed $value Expected current value + * @param int $ttl TTL in seconds. uses the backend default TTL, negative + * values expire the entry immediately. + * @return bool True if the value matched and the TTL was updated, false otherwise + * @since 27.0.0 */ - public function compareSetTTL(string $key, $value, int $ttl): bool; + public function compareSetTTL(string $key, mixed $value, int $ttl): bool; } From 518cf9471527c712a3548c211fc981ae73a223d1 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 13 Jul 2026 12:01:47 -0400 Subject: [PATCH 4/4] chore(memcache): fix typo in Redis refactor Signed-off-by: Josh --- lib/private/Memcache/Redis.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Memcache/Redis.php b/lib/private/Memcache/Redis.php index 9b1b9b1338685..e29bbcf141283 100644 --- a/lib/private/Memcache/Redis.php +++ b/lib/private/Memcache/Redis.php @@ -32,7 +32,7 @@ class Redis extends Cache implements IMemcacheTTL { '75526f8048b13ce94a41b58eee59c664b4990ab2', ], 'caSetTtl' => [ - 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("expire", KEYS[1], ARGV[2]) else return 0 end' + 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("expire", KEYS[1], ARGV[2]) else return 0 end', '94eac401502554c02b811e3199baddde62d976d4', ], ];