Fix stale reads in Cache/Backend/Redis preload_keys - #41132
Fix stale reads in Cache/Backend/Redis preload_keys#41132EclipseEternal wants to merge 2 commits into
Conversation
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.
|
Hi @EclipseEternal. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
@magento run all tests |
- 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.
|
@magento run Unit Tests, Static Tests |
|
@magento run all tests |
|
@magento run Functional Tests EE, Functional Tests CE |
|
@magento run Unit Tests, Sample Data Tests CE, Sample Data Tests EE, Sample Data Tests B2B |
|
@magento run Sample Data Tests CE, Sample Data Tests EE, Sample Data Tests B2B |
|
Failed to run the builds. Please try to re-run them later. |
|
@magento run Sample Data Tests CE, Sample Data Tests EE, Sample Data Tests B2B |
|
Failed to run the builds. Please try to re-run them later. |
|
@magento run Unit Tests |
1 participant
Description (*)
Magento\Framework\Cache\Backend\Redis::load()has two defects in itspreload_keyshandling.1. The preload pipeline re-fires on every
load()after a total miss.The guard is:
$this->preloadedDatais built witharray_filter(array_combine($this->preloadKeys, $redisResponse)), which strips every key that missed. If every preloaded key misses in a given batch,$preloadedDataends up empty - indistinguishable from "the pipeline has never run". The full N-key pipeline then re-fires on every subsequentload()call for the lifetime of the object, instead of running once.2.
save()andremove()never invalidate the preloaded snapshot.Both methods only wrap
parent::save()/parent::remove()in a try/catch; neither touches$preloadedData. If an id was preloaded and is then written or removed within the same request,load()keeps returning the stale pre-write value from$preloadedDatafor the rest of that request, becauseisset($this->preloadedData[$id])is still true.Fix: a
private bool $preloadedflag now drives theload()guard (set once the pipeline has actually executed, independent of hit/miss), andsave()/remove()unconditionallyunset($this->preloadedData[$id])before delegating to the parent implementation.Adds
RedisTest.php, which does not currently exist for this backend (onlyDatabaseTest,MongoDbTest, andRemoteSynchronizedCacheTestdo). It instantiates the real class with a fake minimal Redis client (Credis_Client connects lazily, so no live server is needed) and covers: the pipeline firing exactly once across repeated total-missload()calls, a preloaded hit being reused without a second pipeline,save()dropping the stale entry so the nextload()re-reads, andremove()doing the same.Related Pull Requests
None.
Fixed Issues (if relevant)
Not previously reported. A related but distinct edge case in the same method (
array_combine()raising whenexec()returnsfalse) was already fixed via #37510; this PR does not touch that path.Manual testing scenarios (*)
preload_keyson adefaultfrontend pointed at an empty/flushed Redis namespace (so every preloaded key misses).load()calls). Before this fix, each request re-runs the full preload pipeline; after, only the first does (observable viaMONITORor a call counter on the Redis client).save()a new value for that id andload()it again. Before this fix, the stale pre-save value is returned; after, the new value is returned.remove()instead ofsave(): before this fix the removed id still resolves via the stale snapshot; after, it does not.vendor/bin/phpunit lib/internal/Magento/Framework/Cache/Test/Unit/Backend/RedisTest.phpcovers all four scenarios above in isolation.