fix: resolve race conditions in Redis TTL and prevent cache test state leakage#10374
fix: resolve race conditions in Redis TTL and prevent cache test state leakage#10374gr8man wants to merge 1 commit into
Conversation
|
IMPORTANT Note to reviewers and developers: Please ensure that tests are run regularly with the --order-by=random flag during development. Identifying test pollution and global state leakage bugs at a later stage is extremely difficult and time-consuming, as these failures only manifest under very specific test suite execution orders. |
7827c6e to
260ac10
Compare
michalsn
left a comment
There was a problem hiding this comment.
Please uncomment Cache here: https://github.com/codeigniter4/CodeIgniter4/blob/develop/.github/scripts/random-tests-config.txt#L14
Or run it manually locally first:
.github/scripts/run-random-tests.sh --component Cache --repeat 100
I'm not a fan of some of the changes here, but let's see if they really solve the problem.
740154d to
20159bc
Compare
| if (! isset($this->handler)) { | ||
| parent::tearDown(); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $this->handler->clean(); | ||
|
|
||
| parent::tearDown(); |
There was a problem hiding this comment.
Please refactor to call parent::tearDown() only once. Please review all the methods.
There was a problem hiding this comment.
Please do not add unrelated changes.
0905016 to
ee143cd
Compare
Description
This PR fixes persistent and random failures when running the cache test suite (
tests/system/Cache) repeatedly or in a random order (--order-by=random).Ref: #9968
Changes included:
PredisHandlerandRedisHandlerto use theexpire()command directly with a relative TTL (in seconds) instead of calculating an absolute timestamp forexpireAt()via PHP'sTime::now(). Using a PHP-generated timestamp makes the cache vulnerable to clock drifts between the PHP server and the Redis server. This drift resulted in random test failures (e.g.testGetandtestRememberfailing to assertnullafter waiting for 3 seconds).FileHandlerTest,RedisHandlerTest,PredisHandlerTest,ApcuHandlerTest,MemcachedHandlerTest) inheritedtestDeleteMatchingwhich temporarily created 50-100 cache entries. ThetearDown()methods previously only deleted three predefined default keys (key1,key2,key3), leaving dozens of dummy keys behind. This leaked state caused randomDirectory not emptysystem errors forFileHandlerand false-positive assertion failures in tests liketestDeleteMatchingNothing.tearDown()now properly calls$this->handler->clean()to guarantee a perfectly clean state for the next random test.ApcuHandlerTest'ssetUp()to verifyapcu_enabled(). This correctly skips the test when APCu CLI mode is disabled instead of falling back to theDummyHandlerand producing cascaded test failures.Checklist: