Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fcb6223 to
cdcecd8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup can report success after failed removal because a false existence check does not prove absence.
1 open issue
What changed in this PR
Hardens file-cache cleanup against concurrent deletion without changing the cache format or introducing atomic writes.
Changes:
- Shares file-removal handling across cleanup and
delete(). - Retains cache directories and closes directory handles on failure.
- Adds cleanup regression tests and documents directory retention.
| File | Description |
|---|---|
| tests/TestCase.php | Adds shared cleanup and fixture helpers. |
| tests/FileCacheTest.php | Uses the shared directory-removal helper. |
| tests/CleanupRaceTest.php | Tests deletion races, directory retention, and handle closure. |
| src/FileCache.php | Revises removal handling and preserves directories. |
| README.md | Documents retained cache directories. |
| CHANGELOG.md | Records the cleanup fix. |
Review effort: Balanced
| if (!file_exists($file)) { | ||
| return true; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fix-gc-filemtime-failure #114 +/- ##
==============================================================
+ Coverage 94.15% 98.78% +4.63%
+ Complexity 71 70 -1
==============================================================
Files 1 1
Lines 171 165 -6
==============================================================
+ Hits 161 163 +2
+ Misses 10 2 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|

clear()retains cache directories.GC and
clear()can throw when another process removes a file first.clear()can also remove a directory needed by a concurrent writer, or fail when a writer fills a directory after it has been scanned.Treat files already removed as successfully deleted, retain cache directories, and close directory handles even when cleanup fails. Reuse the file-removal handling in
delete(), while continuing to report failures when a file remains.Stacked on #113 to keep each extracted fix in a separate review. This change uses the existing cache format and does not introduce publication locks or atomic writes.