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 3 commits 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. |
gr8man
changed the title
gr8man
force-pushed
the
fix/cache-tests-race-conditions
branch
from
7827c6e to
260ac10
Compare
michalsn
left a comment
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.
|
|
||
| if ($ttl !== 0) { | ||
| $this->redis->expireat($key, Time::now()->getTimestamp() + $ttl); | ||
| $this->redis->expire($key, $ttl); |
There was a problem hiding this comment.
This is a BC break, please revert.
|
|
||
| if ($ttl !== 0) { | ||
| $this->redis->expireAt($key, Time::now()->getTimestamp() + $ttl); | ||
| $this->redis->expire($key, $ttl); |
There was a problem hiding this comment.
This is a BC break, please revert.
| protected function tearDown(): void | ||
| { | ||
| parent::tearDown(); | ||
| clearstatcache(); |
| if (! isset($this->handler)) { | ||
| return; | ||
| } | ||
|
|
||
| $this->handler->clean(); |
There was a problem hiding this comment.
Should we also call parent::tearDown()?
2 participants
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: