Boost: fully remove boost-cache directory on uninstall and make cleanup resilient on very large caches#49546
Draft
kraftbj wants to merge 4 commits into
Draft
Conversation
Fixes two Page Cache uninstall issues (#41982, #41983): - The uninstall cleanup used Filesystem_Utils::iterate_directory() with a Simple_Delete action, which deliberately preserves index.html placeholder files and only removes directories it considers empty, and logs every deletion via Logger::debug(). The logging could recreate the log file in boost-cache/logs/ while the cleanup was still running, leaving the logs/ and cache/ directories, index.html placeholders, and the boost-cache/ directory itself behind after uninstall. - Add a dedicated Filesystem_Utils::delete_directory() (pre-WordPress safe) which streams over the tree with RecursiveDirectoryIterator/CHILD_FIRST, deletes every file (including index.html) and directory as it is visited without building in-memory file lists, lifts the PHP time limit when allowed, and suppresses per-entry errors so a single failure does not abort the cleanup. This keeps uninstall from hanging or timing out on very large caches (e.g. hundreds of MB of cache files). - Page_Cache_Setup::uninstall() now uses delete_directory(), and still refuses to touch anything outside wp-content/boost-cache. - Add unit tests covering full tree removal (cache/, logs/, static/, nested dirs, index.html placeholders), a deep/many-file tree, path validation, and an already-missing directory. Also define a fallback WP_CONTENT_DIR in the test so it can run standalone with --filter. https://claude.ai/code/session_01PgpTrtTCH4hpz6Krh3ssho
kraftbj
added
[Status] In Progress
[Plugin] Boost
Claude
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Jetpack Boost Page Cache uninstall cleanup by replacing the previous GC-oriented deletion path with a purpose-built directory remover that fully deletes wp-content/boost-cache/ (including index.html placeholders and subdirectories) and is designed to remain performant on very large caches. It also adds PHPUnit coverage and a changelog entry.
Changes:
- Add
Filesystem_Utils::delete_directory()to delete the entire cache tree efficiently during uninstall. - Update
Page_Cache_Setup::uninstall()to call the new deletion helper. - Add unit tests covering full-tree removal, deep/many-file trees, outside-path refusal, and missing-directory handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| projects/plugins/boost/app/modules/optimizations/page-cache/pre-wordpress/class-filesystem-utils.php | Adds a new deletion helper intended for full-tree uninstall cleanup. |
| projects/plugins/boost/app/modules/optimizations/page-cache/class-page-cache-setup.php | Switches uninstall from iterate_directory + Simple_Delete to delete_directory. |
| projects/plugins/boost/tests/php/modules/optimizations/page-cache/Filesystem_Utils_Test.php | Adds unit tests for the new delete behavior and edge cases. |
| projects/plugins/boost/changelog/fix-page-cache-uninstall-cleanup | Adds a patch changelog entry describing the uninstall cleanup fix. |
- delete_directory() now validates the resolved path against the real boost-cache root itself instead of relying on validate_path(), whose is_boost_cache_directory() substring match would also accept sibling paths like boost-cache-old. Only the cache root or paths inside it are accepted, compared on realpath()ed values. - Wrap the recursive iteration in try/catch so an unreadable subdirectory surfaces as a Boost_Cache_Error instead of an uncaught exception during uninstall. - Add a regression test covering the boost-cache-old sibling case, and reword the changelog entry in imperative mood. https://claude.ai/code/session_01PgpTrtTCH4hpz6Krh3ssho
…ubdirectory The unreadable-subdirectory test exercises the Throwable catch added in review; it skips under root, where permission bits are not enforced. https://claude.ai/code/session_01PgpTrtTCH4hpz6Krh3ssho
This was referenced Jun 11, 2026
Draft
Code review (ce-code-review + two Codex adversarial passes) found a symlink-escape: if WP_CONTENT_DIR/boost-cache were a symlink, realpath() resolved both the input path and the cache-root guard to the same external target, so the containment check passed and uninstall would recursively delete files OUTSIDE the cache tree. Reject a symlinked cache root up front with is_link() on the literal path, before realpath() follows it. Symlinks encountered inside the tree are already handled correctly (unlinked, never followed) by the deletion loop. Add tests: - a symlink inside the tree is unlinked without deleting its target - a symlinked cache root is refused with invalid-directory - the post-iteration rmdir() failure path returns could-not-delete-directory distinctly from the iterator-throw path (read-only parent)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #41982
Fixes #41983
Proposed changes
wp-content/boost-cache/completely — all cache files,index.htmlplaceholder files, thecache/,logs/andstatic/subdirectories, and theboost-cache/directory itself. Root cause:Page_Cache_Setup::uninstall()reused the garbage-collection-orientedSimple_Deletepath action, which deliberately skipsindex.htmlplaceholders and callsLogger::debug()per entry — when debug logging is enabled those writes recreateboost-cache/logs/log-<date>.log.phpwhile cleanup is running, sologs/(and thereforeboost-cache/) are never empty and never removed.Filesystem_Utils::delete_directory()streams the tree withRecursiveDirectoryIterator+CHILD_FIRST, deleting entries as it goes (constant memory, no in-memory file lists), lifts the PHP execution time limit where permitted, skips per-file logging, and suppresses per-entry errors so a single locked file doesn't abort the whole cleanup.realpath+is_boost_cache_directory()and refuses to delete anything outsidewp-content/boost-cache; an already-missing directory is treated as success. The helper lives in the pre-WordPress layer and uses SPL only (no WP APIs, no new files torequire_once).Related product discussion/links
document.write) and a Critical CSS SVG-stripping fix (Boost: Inline SVGs stripped out of Critical CSS #42321).Does this pull request change what data or activity we track or use?
No.
Testing instructions
wp-content/boost-cache/cache/. Optionally enable cache debug logging soboost-cache/logs/contains log files.wp-content/boost-cache/containscache/,logs/,index.htmlplaceholders in each directory, and cached.htmlfiles.wp-content/boost-cache/no longer exists at all (no leftovercache/,logs/,static/, orindex.html).wp-content/advanced-cache.phpis removed and Boost'sWP_CACHEdefine is gone fromwp-config.php.composer install --working-dir=projects/plugins/boost && cd projects/plugins/boost && vendor/bin/phpunit-select-config phpunit.#.xml.dist --bootstrap tests/bootstrap.php --testsuite unit --filter Filesystem_Utils_Test(full Boostunitsuite passing: 118 tests, 337 assertions).https://claude.ai/code/session_01PgpTrtTCH4hpz6Krh3ssho
Generated by Claude Code