Make WordPress Core

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)

#1 @adamgreenwell
7 weeks ago

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.

#2 @robbsie
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

  1. Run the multisite test suite for this method: phpunit -c tests/phpunit/multisite.xml --filter test_delete_expired_transients_does_not_cross_networks
  2. 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 @robbsie
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 @adamgreenwell
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.

#6 @wprashed
6 weeks ago

Thank you @robbsie

Note: See TracTickets for help on using tickets.