From beb38a4c44b48267f752410dfcc7d231200a2dc4 Mon Sep 17 00:00:00 2001 From: EclipseEternal <18310675+EclipseEternal@users.noreply.github.com> Date: Wed, 19 Aug 2026 11:27:12 +0100 Subject: [PATCH 1/2] Fix stale reads in Cache/Backend/Redis preload_keys Two defects in the preload_keys pipeline in Cache/Backend/Redis: 1. load() guards the preload pipeline on `empty($this->preloadedData)`. array_filter() strips missed keys from that array, so a batch where every key misses is indistinguishable from "never preloaded", and the full pipeline re-fires on every subsequent load() instead of running once. A private bool $preloaded flag now drives the guard instead, set once the pipeline has actually run regardless of hit or miss. 2. save() and remove() never invalidate the preloaded snapshot, so an id that was preloaded and then written or removed within the same request keeps serving its pre-write value from $preloadedData for the rest of that request. Both methods now unset the entry unconditionally before delegating to the parent implementation, so the next load() re-reads Redis. Adds RedisTest.php, which did not previously exist for this backend. --- .../Magento/Framework/Cache/Backend/Redis.php | 18 +- .../Cache/Test/Unit/Backend/RedisTest.php | 183 ++++++++++++++++++ 2 files changed, 200 insertions(+), 1 deletion(-) create mode 100644 lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php 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..c1cd9f1602058 --- /dev/null +++ b/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php @@ -0,0 +1,183 @@ + '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.' + ); + } +} + +/** + * Minimal stand-in for Credis_Client covering only what Redis::load()/save()/remove() call + * directly: a pipelined batch of hGet() calls flushed by exec(), and a standalone hGet() for the + * non-preloaded path. Anything save()/remove() need beyond that (hMSet, multi, del, ...) is left + * unimplemented on purpose, so those calls surface as an Error that the caller's own try/catch + * around parent::save()/parent::remove() is expected to swallow. + */ +class RedisTestFakeClient +{ + public int $pipelineCount = 0; + + /** @var array> */ + private array $queuedExecResults = []; + + /** @var array */ + private array $directResults = []; + + private bool $inPipeline = false; + + public function queueExecResult(array $result): void + { + $this->queuedExecResults[] = $result; + } + + public function setDirectResult(string $id, $value): void + { + $this->directResults[$id] = $value; + } + + public function pipeline(): self + { + $this->pipelineCount++; + $this->inPipeline = true; + + return $this; + } + + public function hGet(string $key, string $field) + { + 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; + } + + public function exec(): array + { + $this->inPipeline = false; + + return array_shift($this->queuedExecResults) ?? []; + } + + private function stripKeyPrefix(string $key): string + { + return substr($key, strlen(Redis::PREFIX_KEY)); + } +} From 5e001c4ed3146ce757ab5e4ec8477dd25fdb8d89 Mon Sep 17 00:00:00 2001 From: EclipseEternal <18310675+EclipseEternal@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:52:29 +0100 Subject: [PATCH 2/2] Fix static analysis findings in RedisTest - Move the fake Credis_Client stand-in into its own file (RedisTestFakeClient.php): the coding standard requires one class per file. - Add missing @var docblocks on two of its properties. - Drop the unused $field parameter from its hGet() - the fake never needed it, since results are keyed by id alone. --- .../Cache/Test/Unit/Backend/RedisTest.php | 62 ----------- .../Test/Unit/Backend/RedisTestFakeClient.php | 105 ++++++++++++++++++ 2 files changed, 105 insertions(+), 62 deletions(-) create mode 100644 lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTestFakeClient.php diff --git a/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php b/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php index c1cd9f1602058..21e0ad727e339 100644 --- a/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php +++ b/lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.php @@ -119,65 +119,3 @@ public function testRemoveDropsStalePreloadedValue(): void ); } } - -/** - * Minimal stand-in for Credis_Client covering only what Redis::load()/save()/remove() call - * directly: a pipelined batch of hGet() calls flushed by exec(), and a standalone hGet() for the - * non-preloaded path. Anything save()/remove() need beyond that (hMSet, multi, del, ...) is left - * unimplemented on purpose, so those calls surface as an Error that the caller's own try/catch - * around parent::save()/parent::remove() is expected to swallow. - */ -class RedisTestFakeClient -{ - public int $pipelineCount = 0; - - /** @var array> */ - private array $queuedExecResults = []; - - /** @var array */ - private array $directResults = []; - - private bool $inPipeline = false; - - public function queueExecResult(array $result): void - { - $this->queuedExecResults[] = $result; - } - - public function setDirectResult(string $id, $value): void - { - $this->directResults[$id] = $value; - } - - public function pipeline(): self - { - $this->pipelineCount++; - $this->inPipeline = true; - - return $this; - } - - public function hGet(string $key, string $field) - { - 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; - } - - public function exec(): array - { - $this->inPipeline = false; - - return array_shift($this->queuedExecResults) ?? []; - } - - private function stripKeyPrefix(string $key): string - { - return substr($key, strlen(Redis::PREFIX_KEY)); - } -} 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)); + } +}