Opened 7 weeks ago
Last modified 6 weeks ago
#65969 new defect (bug)
Options, Meta APIs: delete_expired_transients() deletes site transients across networks
| Reported by: | robbsie | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | Awaiting Review |
| Component: | Options, Meta APIs | Version: | 4.9 |
| Severity: | normal | Keywords: | has-test-info has-patch has-unit-tests |
| Cc: | Focuses: | multisite |
Description
On multisite, delete_expired_transients() cleans site transients out of wp_sitemeta with this statement (wp-includes/option.php, unchanged since it was introduced in 4.9.0 - verified identical on branches 4.9, 5.0, 6.8 and trunk):
$wpdb->query( $wpdb->prepare( "DELETE a, b FROM {$wpdb->sitemeta} a, {$wpdb->sitemeta} b WHERE a.meta_key LIKE %s AND a.meta_key NOT LIKE %s AND b.meta_key = CONCAT( '_site_transient_timeout_', SUBSTRING( a.meta_key, 17 ) ) AND b.meta_value < %d", $wpdb->esc_like( '_site_transient_' ) . '%', $wpdb->esc_like( '_site_transient_timeout_' ) . '%', time() ) );
wp_sitemeta is a global table: every network's rows live in it, separated only by site_id. The statement pairs a and b on meta_key alone. Nothing requires the value row and the timeout row to belong to the same network.
The guard on the branch is is_main_site() && is_main_network(), so the query runs once - but it runs over every network's rows.
The consequence is that an expired site transient on one network deletes a valid, unexpired site transient of the same name on another, and leaves the victim's timeout row behind as an orphan.
Reproduction
A multi-network install, two networks, same transient name on both. Contents of wp_sitemeta before the run:
| site_id | meta_key | meta_value |
| 1 | _site_transient_foo | A
|
| 1 | _site_transient_timeout_foo | time() + 3600 (not expired)
|
| 2 | _site_transient_foo | B
|
| 2 | _site_transient_timeout_foo | time() - 1 (expired)
|
Only one row satisfies b: network 2's expired timeout. Both value rows satisfy a. So the statement matches two pairs - (a = network 1's value, b = network 2's timeout) and (a = network 2's value, b = network 2's timeout) - and DELETE a, b removes three rows.
Afterwards:
| site_id | meta_key | meta_value |
| 1 | _site_transient_timeout_foo | time() + 3600
|
Network 2 is cleaned correctly. Network 1 has lost a site transient that had an hour left to live, and is left holding a timeout row with no value row.
That leftover row is the orphan described in #65863: before that patch, no public API can remove it. delete_site_transient() will not, because its delete_site_option() on the value row returns false; delete_expired_transients() will not, because its own JOIN needs the value row to exist. It sits in wp_sitemeta until someone deletes it by hand.
So the two tickets meet: this statement is one of the ways core manufactures the orphans that #65863 is about.
Suggested fix
Qualify the pairing with the network, the same way the new anti-join in the patch on #65863 does:
"DELETE a, b FROM {$wpdb->sitemeta} a, {$wpdb->sitemeta} b WHERE a.meta_key LIKE %s AND a.meta_key NOT LIKE %s AND b.meta_key = CONCAT( '_site_transient_timeout_', SUBSTRING( a.meta_key, 17 ) ) + AND b.site_id = a.site_id AND b.meta_value < %d",
Note on scope
Raised during review of the patch on #65863 and split off at the suggestion of its author, because the backport decisions differ: #65863 targets 7.2, whereas this affects every release since 4.9.0 and loses data. The two changes touch adjacent lines in the same function, so whichever lands second will need a rebase.
Testing instructions
There is currently no test that reaches this branch at all - delete_expired_transients()'s sitemeta path is uncovered. A regression test needs a second network ($factory->network->create()), the four rows above written with add_network_option(), a call to delete_expired_transients(), and assertions that network 1 still has both of its rows. It fails on the current statement and passes with the site_id condition added.
Change History (6)
#2
@
7 weeks ago
Welcome, @adamgreenwell - glad to have you on it. A few things that will save you time, since the multisite side of the test suite has some sharp edges.
Where the test belongs. tests/phpunit/tests/option/siteTransient.php. It already carries @group ms-required tests, and its set_up() skips when an external object cache is in use - which matters here,
because delete_expired_transients() returns early under one unless you pass $force_db = true. Putting the test in a file without that skip means it fails for anyone running the suite with an object cache.
The multisite run excludes itself by default. phpunit.xml.dist excludes ms-required, so a plain npm run test:php will silently not run your test. Use the multisite config, which sets
WP_TESTS_MULTISITE:
npm run test:php -- -c tests/phpunit/multisite.xml --filter test_delete_expired_transients
The one trap. delete_expired_transients() runs raw SQL and does not clear the option cache, so get_network_option() will happily hand you back a row the query has already deleted. Assert against the
table.
A skeleton following the reproduction in the description:
/** * @ticket 65969 * * @group ms-required * * @covers ::delete_expired_transients */ public function test_delete_expired_transients_does_not_cross_networks() { global $wpdb; $other = self::factory()->network->create(); $value = '\_site\_transient\_cross\_network'; $timeout = '\_site\_transient\_timeout\_cross\_network'; // This network: valid, an hour left to live. add_site_option( $value, 'A' ); add_site_option( $timeout, time() + HOUR_IN_SECONDS ); // The other network: same name, already expired. add_network_option( $other, $value, 'B' ); add_network_option( $other, $timeout, time() - 1 ); delete_expired_transients(); $rows = $wpdb->get_col( $wpdb->prepare( "SELECT meta_key FROM {$wpdb->sitemeta} WHERE site_id = %d AND meta_key IN ( %s, %s )", get_current_network_id(), $value, $timeout ) ); $this->assertContains( $value, $rows, 'An unexpired site transient on this network must not be deleted because one of the same name expired on another network.' ); $this->assertContains( $timeout, $rows, 'Its timeout row must not be left behind either.' ); }
Check it is a real regression test. Run it against unpatched trunk first. It has to fail on the first assertion - the value row on this network is gone - and pass once AND b.site_id = a.site_id is in
place. A multisite test that passes both before and after is the usual way this kind of fix ends up unguarded.
Two optional extras, if you want them: assert that the other network was cleaned, so the fix cannot be mistaken for disabling the cleanup altogether; and note that the second assertion is the
interesting one, because the row it protects is the orphan from #65863 - the current statement manufactures exactly the leftovers that ticket is about.
Once you have a PR, add has-patch and has-unit-tests to the keywords here and drop the link in a comment. Happy to review.
This ticket was mentioned in PR #13288 on WordPress/wordpress-develop by @wprashed.
7 weeks ago
#3
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
### Ticket
### Description
In multisite environments, site transients and their timeout entries are stored in the global wp_sitemeta table, separated by site_id (representing the network ID).
The multisite cleanup query in delete_expired_transients() previously joined the transient value row (a) and timeout row (b) purely by matching b.meta_key with a.meta_key:
DELETE a, b FROM {$wpdb->sitemeta} a, {$wpdb->sitemeta} b WHERE a.meta_key LIKE %s AND a.meta_key NOT LIKE %s AND b.meta_key = CONCAT( '_site_transient_timeout_', SUBSTRING( a.meta_key, 17 ) ) AND b.meta_value < %d
Because b.site_id = a.site_id was not required, an expired site transient on one network would match an unexpired site transient with the same key on a different network and delete it from wp_sitemeta, leaving an orphaned timeout row behind on the victim network.
This PR adds AND b.site_id = a.site_id to ensure the cleanup is properly scoped within the same network.
### Testing Instructions
- Run the multisite test suite for this method:
phpunit -c tests/phpunit/multisite.xml --filter test_delete_expired_transients_does_not_cross_networks - Verify all assertions pass.
## Use of AI Tools
AI assistance: Yes
Tool(s): Google Antigravity
Model(s): Gemini
Used for: Analyzing the cross-network multi-table delete issue, writing the multisite unit test, and applying the network qualification join condition.
#4
@
7 weeks ago
Thanks @wprashed - I've traced this by hand against the pre-patch statement and the fix and test both hold up.
Without the patch: b is filtered only by meta_key and b.meta_value < time(), across all networks. In the test's setup only the other network's expired timeout satisfies that, but both networks' value rows
satisfy a - so DELETE a, b removes three rows: both value rows plus the one timeout. The main network loses a transient with an hour left to live and is left holding its own unexpired timeout row, which is
exactly the orphan #65863 is about. test_delete_expired_transients_does_not_cross_networks() fails on the first assertion there, as a regression test for this should.
With AND b.site_id = a.site_id added: the main network's own timeout (unexpired) no longer qualifies as b for anything, so its value row is never touched, while the other network's expired pair is still
removed correctly. All three assertions pass.
Placement is fine - this statement is a plain comma-join with WHERE, not the LEFT JOIN anti-join added on #65863, so there's no ON-vs-WHERE distinction to get wrong here.
Nothing further from me. has-patch and has-unit-tests are already set - looks ready for a committer.
#5
@
7 weeks ago
Thanks for opening this. I had claimed the Trac ticket and independently implemented and tested the same change. On unpatched trunk, the regression test failed at the first assertion as expected. With the site_id join, it passed with 3 assertions. I also ran the full Tests_Option_SiteTransient class (5 tests, 19 assertions), the multisite Options group (449 tests, 1,180 assertions), and PHPCS on both changed files. The submitted patch matches my local findings. I see that adamgreenwell is included in the proposed props — thank you, I appreciate it very much.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
I'd like to work on this as my first Core patch. I'll add the network-specific join condition and a multisite regression test following the supplied reproduction steps.