Cache API: Do not cache a miss while cache addition is suspended.

[jigneshbhavani]

wp_cache_add() returns early when wp_suspend_cache_addition() is on and wp_cache_set() does not, so the swap in r63354 left WP_Post, WP_Term, WP_Comment, WP_Site and WP_Network::get_instance() writing to the object cache while additions are suspended.

r63354 reached for set() because an unusable cached value has to be replaced and add() will not overwrite. That still works: only the suspension check moves to the call site, so a poisoned entry is replaced as before on a normal request, and nothing is written while the flag is on. Simplified per review; the first version also replaced while suspended.

Measured on a running site with the cache cleared and the flag on: post, term and comment are PRESENT on trunk and MISS on this branch, with a plain wp_cache_add() control MISS throughout. Each of the five guards is mutation checked, reverting one file fails exactly one test. Groups post, comment, taxonomy, cache, ms-site and ms-network are green, and the single taxonomy warning is present on clean trunk too. PHPCS and PHPStan clean.

Trac ticket: https://core.trac.wordpress.org/ticket/66006

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Drafting the fix and the regression tests, and running the reproduction, mutation and lint checks. Reviewed and verified by me before opening.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props bejignesh, westonruter, josephscott.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[westonruter]

cc @josephscott

[westonruter]

$unusable_value_cached is derived from the cached value rather than from the $found argument of wp_cache_get(), because a drop-in declaring only three parameters would leave $found undefined and warn on PHP 8.

Are there drop-ins that only declare 3 params? The wp_cache_get() function specifically seems to expect at least 4 as it has a func_num_args() > 4 check.

[westonruter]

The naming of the $unusable_value_cached variable is misleading. Given that we're already in this branch, we already know it is not an unusable cached value since it checked the types of $_network already.

What this seems to be getting at is to check whether there was a cache hit or not. In that case, passing a variable by reference to wp_cache_hit() seems better, per above.

[jigneshbhavani]

@westonruter Fair. $found would also be the more accurate source than the name suggests: a persistent cache holding a literal false is a hit that false !== $_network reads as a miss.

Both are moot if the guard below loses its first condition, which I now think it should. Answered there.

[westonruter]

Why not this?

Suggested change

	if ( $unusable_value_cached || ! wp_suspend_cache_addition() ) {
	if ( ! wp_suspend_cache_addition() ) {

If cache addition is suspended, why would this not always apply to setting as well?

[jigneshbhavani]

@westonruter You are right, and there is a caller that settles it. Health Check & Troubleshooting calls wp_suspend_cache_addition( true ) for the whole request while a troubleshooting session is active, commented "Attempt to avoid cache entries from a troubleshooting session". Replacing an entry during that session still writes to the shared cache, which is the thing that plugin is trying to avoid. Same code in WordPress/site-health-troubleshooting.

What the first condition was protecting is r63354's own stated reason for set(), that "the unusable value survived and every later lookup for that object queried the database again". Dropping it reinstates that inside suspended requests only, so on wordcamp.org's wp_suspend_cache_addition( true ) / switch_to_blog() import loop a poisoned entry would be re-queried per iteration. That does not outweigh honouring the flag.

Taking your version. It removes $unusable_value_cached from all five classes, and test_get_instance_replaces_a_poisoned_cache_value_while_cache_addition_is_suspended in wpPost.php goes with it. The five ..._does_not_cache_a_miss_while_cache_addition_is_suspended tests stay.

[westonruter]

(Please disclose every comment that is generated with AI.)

[jigneshbhavani]

@westonruter Understood, will do. Those four were drafted with Claude Code and reviewed by me before posting.

[jigneshbhavani]

@westonruter Only one that I can find, and it is not a live case: Simple Cache's memcache drop-in, 1,000 installs and last updated in 2021. Everything current declares the fourth parameter, including the memcached drop-in bundled in tests/phpunit/includes/object-cache.php.

The reason was weak anyway. Initialising $found before the call removes the warning, which is what WP_AI_Client_Cache::get() does. Dropping that line from the description.

[westonruter]

Suggested change

	* Not wp_cache_add(), since an unusable cached value may still be present and must be
	* replaced. add() checks wp_suspend_cache_addition() and set() does not, so the check
	* moves to the call site.
	* Not wp_cache_add(), since an unusable cached value may still be present and must be replaced.
	* Note that wp_cache_add() checks wp_suspend_cache_addition() and wp_cache_set() does not.

Could be applied to the other instances below as well.

[josephscott]

Thank you for the ping. I've been thinking through what this could look like and ultimately given the past decisions and where WordPress core is right now I'm leaning towards utilizing $found and doing a conditional delete then add flow.

Using Post as an example, it would be:

$found = false;
$_post = wp_cache_get( $post_id, 'posts', false, $found );

// A cached value that is not usable as a post is treated as a cache miss.
if ( ! ( $_post instanceof stdClass || $_post instanceof WP_Post ) || ! isset( $_post->ID ) ) {
    $_post = sanitize_post( $_post, 'raw' );

    /*
     * A cache result was found, but it was invalid.  Remove it so that the correct value
     * can be added.  This pattern is used instead of a direct wp_cache_set() because
     * wp_cache_add() will respect wp_suspend_cache_addition().
     */
    if ( $found ) {
        wp_cache_delete( $post_id, 'posts' );
    }

    wp_cache_add( (int) $_post->ID, $_post, 'posts' );
} elseif ( empty( $_post->filter ) || 'raw' !== $_post->filter ) {
    $_post = sanitize_post( $_post, 'raw' );
}

If we had something like wp_cache_get_valid() from https://core.trac.wordpress.org/ticket/66005 it could take care of deleting the invalid cache entries, allowing the Post code to skip the $found checks. If we aren't going to have that soon, then we should consider the $found, conditional delete, then add flow.

[westonruter]

@josephscott Perhaps but don't you think incorporating a ! wp_suspend_cache_addition() check would be a more immediate fix to preserve the previous behavior whereby the object cache was expected to not get populated?

[josephscott]

It is an approach that would work. When I looked at the usage of wp_suspend_cache_addition() in WordPress core I found it was only used twice ( outside of the function definition and the add check ). Those two calls were both in display_callback(), and neither of those calls were related to a wp_cache_set() condition.

Then I searched for wp_cache_add() usage, it is used 15 times. Based on those numbers WordPress code is depending much more on wp_cache_add() to check wp_suspend_cache_addition() than to do a separate check directly.

I still think having a wp_cache_get_valid() approach would be even better, but I'm trying to be pragmatic and look at what is currently available.

[westonruter]

We should look into the history of wp_suspend_cache_addition(). My understanding is that it was primarily added for the sake of the WordPress Importer, to avoid filling up a persistent object cache with data unnecessarily. It was also added for the preventing previewed data from being populated in the object cache, and thus inadvertently leaking previewed changes live. This would seem to make it essential to prevent setting or adding any data to the persistent object cache when one is active. So wp_using_ext_object_cache() could be relevant here. But if that is the case, then I think it would be preferable to be consistent across all the wp_cache_*() functions to short-circuit if wp_suspend_cache_addition() is true. I see this is only done for WP_Object_Cache::add(). Ironically, this is for a non-persistent object cache, so here it seems the main point of it is to prevent filling up memory in the PHP process during a WordPress Import.

[josephscott]

I think it would be preferable to be consistent across all the wp_cache_*() functions to short-circuit if wp_suspend_cache_addition() is true.

That was the first thing I had wondered about to - and even considered updating wp_cache_set() to detect and swap to a replace call. But given how long the current situation had been in place, that would potentially open a different can of worms.

Looking at redis-cache - https://plugins.trac.wordpress.org/browser/redis-cache/trunk/includes/object-cache.php - it also only checks on add.