fix(memcache): normalize Redis TTL handling and return actual compareSetTTL result

lib/private/Memcache/Redis.php

@@ -32,8 +32,8 @@
 			'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 @@
 		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 @@
 	#[\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,23 +192,20 @@
 
 	#[\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);
 	}
 
 	#[\Override]
 	public function getTTL(string $key): int|false {
 		$ttl = $this->getCache()->ttl($this->getPrefix() . $key);
 		return $ttl > 0 ? (int)$ttl : 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;
 	}

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;
 }

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'));
 	}
 }