diff --git a/lib/private/Memcache/LoggerWrapperCache.php b/lib/private/Memcache/LoggerWrapperCache.php index 512a8ed22934a..a724c77cf6664 100644 --- a/lib/private/Memcache/LoggerWrapperCache.php +++ b/lib/private/Memcache/LoggerWrapperCache.php @@ -158,7 +158,7 @@ public function ncad(string $key, mixed $old): bool { FILE_APPEND ); - return $this->wrappedCache->cad($key, $old); + return $this->wrappedCache->ncad($key, $old); } /** @inheritDoc */ diff --git a/lib/private/Memcache/Redis.php b/lib/private/Memcache/Redis.php index edf9a80aad497..b3d41a6717963 100644 --- a/lib/private/Memcache/Redis.php +++ b/lib/private/Memcache/Redis.php @@ -39,6 +39,9 @@ class Redis extends Cache implements IMemcacheTTL { private const MAX_TTL = 30 * 24 * 60 * 60; // 1 month + /** Number of keys to request per SCAN iteration in {@see self::clear()} (only a hint to Redis) */ + private const SCAN_COUNT = 1000; + private \Redis|\RedisCluster|null $cache = null; public function __construct($prefix = '', string $logFile = '') { @@ -92,12 +95,45 @@ public function remove($key) { #[\Override] public function clear($prefix = '') { - // TODO: this is slow and would fail with Redis cluster - $prefix = $this->getPrefix() . $prefix . '*'; - $keys = $this->getCache()->keys($prefix); - $deleted = $this->getCache()->del($keys); + $pattern = $this->getPrefix() . $prefix . '*'; + $cache = $this->getCache(); + + // Iterate with SCAN and remove with UNLINK rather than KEYS + DEL: + // KEYS walks the whole keyspace and blocks the server, while a + // multi-key DEL/UNLINK is not cluster-safe (keys spanning hash slots + // raise a CROSSSLOT error). SCAN is non-blocking and UNLINK reclaims + // memory in the background. + if ($cache instanceof \RedisCluster) { + // On a cluster SCAN must be run against each master node, and keys + // are unlinked one at a time so each command stays within a slot. + foreach ($cache->_masters() as $master) { + $iterator = null; + do { + /** @psalm-suppress NullArgument, PossiblyNullArgument the SCAN cursor must start as null (the phpredis stub types it as int) */ + $keys = $cache->scan($iterator, $master, $pattern, self::SCAN_COUNT); + if ($keys === false) { + break; + } + foreach ($keys as $key) { + $cache->unlink($key); + } + } while ($iterator > 0); + } + } else { + $iterator = null; + do { + /** @psalm-suppress NullArgument, PossiblyNullArgument the SCAN cursor must start as null (the phpredis stub types it as int) */ + $keys = $cache->scan($iterator, $pattern, self::SCAN_COUNT); + if ($keys === false) { + break; + } + if ($keys !== []) { + $cache->unlink($keys); + } + } while ($iterator > 0); + } - return (is_array($keys) && (count($keys) === $deleted)); + return true; } /** diff --git a/tests/lib/Memcache/RedisTest.php b/tests/lib/Memcache/RedisTest.php index d410dac6686ae..a690e02679f71 100644 --- a/tests/lib/Memcache/RedisTest.php +++ b/tests/lib/Memcache/RedisTest.php @@ -84,4 +84,37 @@ public function testCasTtlChanged(): void { // allow for 1s of inaccuracy due to time moving forward $this->assertLessThan(1, 50 - $this->instance->getTTL('foo')); } + + public function testClearWithPrefixOnlyRemovesMatchingKeys(): void { + $this->instance->set('foo1', 'a'); + $this->instance->set('foo2', 'b'); + $this->instance->set('bar1', 'c'); + + $this->assertTrue($this->instance->clear('foo')); + + $this->assertFalse($this->instance->hasKey('foo1')); + $this->assertFalse($this->instance->hasKey('foo2')); + $this->assertTrue($this->instance->hasKey('bar1')); + } + + public function testClearWithoutMatchesReturnsTrue(): void { + // Nothing is stored under this prefix; clearing must not error out + // (regression guard for calling UNLINK/DEL with an empty key list). + $this->assertTrue($this->instance->clear('no-such-prefix')); + } + + public function testClearRemovesEntriesAcrossMultipleScanBatches(): void { + // More keys than a single SCAN batch (self::SCAN_COUNT) to exercise the + // cursor loop and make sure nothing is left behind. + $count = 1500; + for ($i = 0; $i < $count; $i++) { + $this->instance->set('bulk-' . $i, $i); + } + + $this->assertTrue($this->instance->clear('bulk-')); + + $this->assertFalse($this->instance->hasKey('bulk-0')); + $this->assertFalse($this->instance->hasKey('bulk-' . ($count - 1))); + $this->assertFalse($this->instance->hasKey('bulk-' . intdiv($count, 2))); + } }