diff --git a/lib/internal/Magento/Framework/Cache/Backend/Redis.php b/lib/internal/Magento/Framework/Cache/Backend/Redis.php index 33429e59c67e4..6bf005e3f9117 100644 --- a/lib/internal/Magento/Framework/Cache/Backend/Redis.php +++ b/lib/internal/Magento/Framework/Cache/Backend/Redis.php @@ -25,6 +25,14 @@ class Redis extends \Cm_Cache_Backend_Redis */ private $preloadKeys = []; + /** + * Whether the preload pipeline has already run. Cannot be inferred from $preloadedData: a batch + * in which every key missed leaves it empty, so the pipeline would re-fire on every load(). + * + * @var bool + */ + private bool $preloaded = false; + /** * Whether to use lua on garbage collection * @@ -51,7 +59,7 @@ public function __construct($options = []) */ public function load($id, $doNotTestCacheValidity = false) { - if (!empty($this->preloadKeys) && empty($this->preloadedData)) { + if (!empty($this->preloadKeys) && !$this->preloaded) { $redis = $this->_slave ?? $this->_redis; $redis = $redis->pipeline(); @@ -60,6 +68,7 @@ public function load($id, $doNotTestCacheValidity = false) } $redisResponse = $redis->exec(); + $this->preloaded = true; $this->preloadedData = is_array($redisResponse) ? array_filter(array_combine($this->preloadKeys, $redisResponse)) : []; @@ -83,6 +92,10 @@ public function load($id, $doNotTestCacheValidity = false) */ public function save($data, $id, $tags = [], $specificLifetime = 86_400_000) { + // The preloaded copy is a snapshot from the first load(), so a write makes it stale. Dropped + // unconditionally, including when the write below fails, so the next load() re-reads Redis. + unset($this->preloadedData[$id]); + // @todo add special handling of MAGE tag, save clenup try { $result = parent::save($data, $id, $tags, $specificLifetime); @@ -98,6 +111,9 @@ public function save($data, $id, $tags = [], $specificLifetime = 86_400_000) */ public function remove($id) { + // Same as save(): drop the snapshot so the next load() reports the removal. + unset($this->preloadedData[$id]); + try { $result = parent::remove($id); } catch (\Throwable $exception) { diff --git a/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php b/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php new file mode 100644 index 0000000000000..21e0ad727e339 --- /dev/null +++ b/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php @@ -0,0 +1,121 @@ + 'localhost', 'preload_keys' => $preloadKeys]); + $client = new RedisTestFakeClient(); + + $bind = \Closure::bind(function ($instance, $redisClient) { + $instance->_redis = $redisClient; + }, null, Redis::class); + $bind($backend, $client); + + return [$backend, $client]; + } + + /** + * A batch in which every preload key misses must not cause the pipeline to re-fire on the + * next load(): the guard must track "already ran", not "found something". + * + * @return void + */ + public function testPreloadPipelineDoesNotRefireAfterTotalMiss(): void + { + [$backend, $client] = $this->createBackend(['a', 'b']); + $client->queueExecResult([false, false]); + $client->setDirectResult('a', 'fetched-a'); + + $backend->load('a'); + $backend->load('a'); + + $this->assertSame( + 1, + $client->pipelineCount, + 'The preload pipeline must run at most once, even when every key misses.' + ); + } + + /** + * A preloaded hit is served from the batch and does not trigger a second pipeline on a later + * load() of a different id from the same batch. + * + * @return void + */ + public function testPreloadedHitIsReusedWithoutRefiring(): void + { + [$backend, $client] = $this->createBackend(['a', 'b']); + $client->queueExecResult(['payload-a', false]); + $client->setDirectResult('b', 'fetched-b'); + + $this->assertSame('payload-a', $backend->load('a')); + $this->assertSame('fetched-b', $backend->load('b')); + $this->assertSame(1, $client->pipelineCount); + } + + /** + * save() must drop the preloaded snapshot for the id it writes, even when the underlying + * write itself fails, so the next load() re-reads Redis instead of serving stale data. + * + * @return void + */ + public function testSaveDropsStalePreloadedValue(): void + { + [$backend, $client] = $this->createBackend(['a']); + $client->queueExecResult(['stale-a']); + $client->setDirectResult('a', 'fresh-a'); + + $this->assertSame('stale-a', $backend->load('a')); + + // The fake client has no hMSet/multi support, so parent::save() throws; save() itself + // catches that and returns false - the snapshot must still have been dropped beforehand. + $this->assertFalse($backend->save('new-a', 'a')); + + $this->assertSame( + 'fresh-a', + $backend->load('a'), + 'load() must not keep serving the pre-write value after save() for the same id.' + ); + $this->assertSame(1, $client->pipelineCount); + } + + /** + * remove() must drop the preloaded snapshot for the id it removes, so the next load() + * reports the removal instead of the pre-removal value. + * + * @return void + */ + public function testRemoveDropsStalePreloadedValue(): void + { + [$backend, $client] = $this->createBackend(['a']); + $client->queueExecResult(['stale-a']); + $client->setDirectResult('a', false); + + $this->assertSame('stale-a', $backend->load('a')); + + $backend->remove('a'); + + $this->assertFalse( + $backend->load('a'), + 'load() must not resurrect a removed id from the stale preloaded snapshot.' + ); + } +} diff --git a/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTestFakeClient.php b/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTestFakeClient.php new file mode 100644 index 0000000000000..287e7a71736cd --- /dev/null +++ b/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTestFakeClient.php @@ -0,0 +1,105 @@ +> + */ + private array $queuedExecResults = []; + + /** + * @var array + */ + private array $directResults = []; + + /** + * @var bool + */ + private bool $inPipeline = false; + + /** + * @param array $result + * @return void + */ + public function queueExecResult(array $result): void + { + $this->queuedExecResults[] = $result; + } + + /** + * @param string $id + * @param mixed $value + * @return void + */ + public function setDirectResult(string $id, $value): void + { + $this->directResults[$id] = $value; + } + + /** + * @return self + */ + public function pipeline(): self + { + $this->pipelineCount++; + $this->inPipeline = true; + + return $this; + } + + /** + * @param string $key + * @return self|mixed + */ + public function hGet(string $key) + { + if ($this->inPipeline) { + // Queued: the actual value is returned positionally by exec(), not here. + return $this; + } + + $id = $this->stripKeyPrefix($key); + + return $this->directResults[$id] ?? false; + } + + /** + * @return array + */ + public function exec(): array + { + $this->inPipeline = false; + + return array_shift($this->queuedExecResults) ?? []; + } + + /** + * @param string $key + * @return string + */ + private function stripKeyPrefix(string $key): string + { + return substr($key, strlen(Redis::PREFIX_KEY)); + } +}