Make WordPress Core

Opened 2 months ago

Closed 8 weeks ago

Last modified 8 weeks ago

#60457 closed defect (bug) (fixed)

Plugin Dependencies: Running update_option within wp-settings can be catastrophic for a high traffic site

Reported by: dd32's profile dd32 Owned by: costdev's profile costdev
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Plugins Keywords: has-patch has-unit-tests has-testing-info commit
Focuses: performance Cc:

Description

In #22316 the following code was added for plugin dependencies:
https://github.com/WordPress/wordpress-develop/blob/27087b71e0c109af68dbc0545893217fc80350db/src/wp-settings.php#L597-L599

Running DB Writes, particularly update_option() on the front-end, especially during the bootstrap of WordPress can have catastrophic consequences when thousands of requests all try to update the database and cache at the same time.

This leads to a much higher chance of the Database and Cache becoming out-of-sync, which can cause snowballing issues. This happened on api.WordPress.org/plugins today, with WordPress getting stuck in a situation where it constantly tried to update the DB and failed (because the data had already been written there, but the cache said otherwise) and as a result never properly updated the cache.

Additionally; Loading a large option of all plugin headers during the WordPress bootstrapping is rather expensive, in the whole scheme of things, especially when only a few fields are actually used.
On WordPress.org/plugins this has become a 75KB blob of data of which exactly 0bytes are used, loaded on every page load and API request, that has to be fetched from either the Database or the Object cache.

Perhaps the dependency checking shouldn't be done at inclusion time, but rather as part of the activation/update process for plugins.
This has the benefit that it moves the processing away from happening on each page load and only on requests that are able to process more data without affecting the end-user, or site performance, and functionalities such as fetching the plugin headers doesn't strictly need to be cached.

Change History (25)

#1 @dd32
2 months ago

To be clear; This isn't against plugin dependencies, just not at the cost of lowering WordPress performance, or causing issues for high traffic WordPress sites.

I can understand fully why the dependency checking is being performed at plugin inclusion time, as the environment may have changed (ie. site re-deployed with a new PHP version), but for 99.999% of WordPress sites, this isn't something that's going to be an issue for them, and for the 0.001% that it is, accessing wp-admin or another admin contextual page is likely going to be an acceptable workaround.

This ticket was mentioned in Slack in #core-upgrade-install by dd32. View the logs.


2 months ago

#3 @costdev
2 months ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 6.5
  • Owner set to costdev
  • Status changed from new to assigned

Thanks for opening the ticket Dion! Your point about DB write during bootstrap is well taken.

In this case, the DB write in wp-settings.php is to update any potentially missing plugin data for active plugins, rather than being a required part of the plugin dependencies feature. It's an extra layer of ensuring no plugin activation goes unnoticed by the feature (such as external database updates to active_plugins, for example).

It can be removed without hindering the dependencies check being performed during the bootstrap, which essentially just checks against the active_plugins option - which will be cached from the call to wp_get_active_and_valid_plugins() already in the bootstrap a few lines above.

I'm happy to write the patch to remove the update_option() and related lines. I'll hold off on doing this until we get thoughts from @afragen and @azaozz.

#4 @costdev
2 months ago

Additionally; Loading a large option of all plugin headers during the WordPress bootstrapping is rather expensive, in the whole scheme of things, especially when only a few fields are actually used.

We could also reduce the size of the database option to just the required fields. Would this be a suitable alternative?

Additionally, we could possibly further reduce this to just the required headers of plugins that had the Requires Plugins header defined when they were installed/first detected.

Last edited 2 months ago by costdev (previous) (diff)

#5 @costdev
2 months ago

Additionally, there is also a removal of invalid plugins from the plugin_data option done in wp_get_active_and_valid_plugins(). As this function runs during the bootstrap, I would also propose removing this cleanup from there. get_plugins() will effectively perform a cleanup anyway on its first call, and the cleanup can wait until that is triggered on a visit to the admin's plugins.php page.

This ticket was mentioned in Slack in #cli by costdev. View the logs.


2 months ago

#7 @costdev
2 months ago

Following up on this, there are several areas I believe we can improve on.

1. Unnecessary logic in the wp_get_active_and_valid_plugins() loop in wp-settings.php

  • Proposal: Remove the part of the dependencies check in the wp_get_active_and_valid_plugins() loop that deals with plugins that require other plugins.
  • Supplementary proposal: Guard the call to WP_Plugin_Dependencies::deactivate_dependents_with_unmet_dependencies() so it only runs on the admin's plugins.php page.

WP_Plugin_Dependencies::initialize() runs during bootstrap, before the wp_get_active_and_valid_plugins() loop. This means that WP_Plugin_Dependencies::deactivate_dependents_with_unmet_dependencies() will have already deactivated said plugins if necessary, and therefore the wp_get_active_and_valid_plugins() loop won't deal with plugins that require other plugins. There's no need to have that part of the logic included in the loop and therefore running twice.

Note, however, that WP_Plugin_Dependencies::deactivate_dependents_with_unmet_dependencies() will result in a call to deactivate_plugins(), which will call update_option(), resulting in a DB write. It may therefore be necessary to defer WP_Plugin_Dependencies::deactivate_dependents_with_unmet_dependencies() to the admin's plugins.php page.


2. Unnecessary data in the plugin_data option

  • Proposal: Reduce the contents of the plugin_data option to just the needed headers.

WP_Plugin_Dependencies only uses the Name, and RequiresPlugins headers. We don't need to store all headers for the Plugin Dependencies feature. We stored all the headers so that they'd be available to Core without looking to the filesystem each time, but the other headers aren't being used at this time.

However, Requires, RequiresPHP would also be necessary for checking WordPress and PHP requirements. They're just not used directly by the WP_Plugin_Dependencies class, which is focused on plugins that require other plugins.


3. Over-cautious updating of the plugin_data option

  • Proposal: Remove the update_option() calls in the wp_get_active_and_valid_plugins() loop

In the wp_get_active_and_valid_plugins(), any plugins that aren't included in the plugin_data are added. This was an over-cautious attempt to avoid plugins going unnoticed, but is unlikely to be a common or even occasional issue.


4. Cleanup operation running too often

  • Proposal: Guard the cleanup operation inside wp_get_active_and_valid_plugins() to only run on the admin's plugins.php page.
  • Alternative Proposal 1: Schedule the cleanup in cron. This isn't always enabled on WordPress sites, so it would produce an inconsistent result across sites.
  • Alternative Proposal 2: Don't clean up. This will by definition result in unnecessary data in the option though.

In wp_get_active_and_valid_plugins(), invalid plugins are removed from the plugin_data option. This is part of a cleanup operation, and while useful for reducing the data stored in the database, isn't necessary when loading any page. We could just do this when the plugins.php (Plugins > Installed Plugins) page is visited, or in cron, or not clean up at all.


@afragen @azaozz @dd32 Let me know thoughts on each of the above.

Last edited 2 months ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


2 months ago

#9 @costdev
2 months ago

Please note that PR 6071 for #60461 was submitted to avoid issues during install scripts. This removes the update_option() calls in wp-settings.php, and guards a call to update_option() in get_plugins() when WordPress is installing.

This tackles the following from the above list:

  • Proposal in 1
  • Proposal in 3
  • Alternative Proposal 2 in 4

While not a complete resolution to the concerns of this ticket, I'm just updating you all on a patch for a specific issue that relates to the areas discussed here.

Last edited 2 months ago by costdev (previous) (diff)

#10 @azaozz
2 months ago

Right, DB writes should never happen on front-end requests. This can cause big problems on busy sites especially with more complex hosting setup, DB replication, etc. They can go down easily.

There is another patch in the works (afaik) for this exact place (where plugin files are included). It's for prevention of loading of known incompatible plugins (fatal error protection). Any solution here would have to be synced with the other patch. This is also the reason it was left unfinished.

Looking a bit deeper: currently there are some problems with loading of plugins with unmet requirements. For example a plugin that requires newer version of WP or PHP cannot be installed from the UI (the plugins repo/API doesn't allow it) but can still be installed from FTP or SSH. These methods were somewhat popular few years ago, especially using version control to deploy changes, so thinking WP should guard against errors there too. Will open a new ticket for that.

#11 follow-up: @costdev
2 months ago

@azaozz What does that mean for automatic deactivation of dependent plugins with unmet requirements? This will write to the active_plugins option to deactivate them.

Should we cease to load them, but only forcibly deactivate them on plugins.php or plugin-install.php?

Or should we stop the forcible deactivation and just not load their files?

Last edited 2 months ago by costdev (previous) (diff)

#12 in reply to: ↑ 11 @azaozz
2 months ago

Replying to costdev:

Should we cease to load them, but only forcibly deactivate them on plugins.php or plugin-install.php?

Or should we stop the forcible deactivation and just not load their files?

Yea, good questions. Don't think there is a big difference between "not loaded" and "deactivated", but a nice nuance to keep in mind.

The more important part imho is to inform the site admins what's happening so they can take action.

Thinking this discussion may be better off in that new ticket I was planning to open. Will link it here when ready.

#13 @azaozz
2 months ago

Related: #60491.

#14 @costdev
2 months ago

In 57592:

Upgrade/Install: Avoid update_option() calls during bootstrap.

[57545] introduced the Plugin Dependencies feature, which contains a new plugin_data option.

Previously, the plugin_data option was being updated during bootstrap and in get_plugins(), causing an error when using the install script as the options database table does not yet exist, and also risked an "out of sync" issue between the database and the cache on websites with heavy traffic.

This removes the calls to update_option() during Core's bootstrap, and guards the call in get_plugins() to ensure that it doesn't run when WordPress is installing.

Follow-up to [57545].

Props desrosj, swisspidy, huzaifaalmesbah, afragen, dd32, azaozz, costdev.
Fixes #60461. See #60457, #60491.

This ticket was mentioned in Slack in #core-upgrade-install by pbiron. View the logs.


2 months ago

#16 @costdev
2 months ago

Auto-deactivation

Auto-deactivation was discussed during the #core-upgrade-install meeting in Slack, and we have decided to remove this from the Plugin Dependencies feature.

  • deactivate_plugins() will cause a database write during bootstrap which is unacceptable.
  • Relocating auto-deactivation elsewhere would make it ineffective in achieving its goal of preventing Fatal Errors during all other requests.

Bootstrapping logic

The bootstrapping logic to prevent loading plugins with unmet WP/PHP requirements must be removed as it conflicts with #60491, which is still being discussed and any actions on that ticket will not happen during the 6.5 cycle.


The plugin_data option

The plugin_data option will be removed as it was only added to avoid filesystem reads of all plugins during bootstrap and will no longer be needed by the Plugin Dependencies feature. The concept of storing plugin data persistently can be discussed as an implementation detail of any actions on #60491.


These changes will resolve all the remaining items in the list in my earlier comment:

  • Supplementary proposal in 1 as the method will no longer exist.
  • Proposal in 2 as the plugin_data option will no longer exist.
  • Proposal and Alternative Proposal 2 in 4 as the plugin_data option will no longer exist.

I'll submit a PR to make the removals, relocate the initialization of Plugin Dependencies to the appropriate places, and update the class and tests as necessary.

This ticket was mentioned in PR #6123 on WordPress/wordpress-develop by @costdev.


2 months ago
#17

  • Keywords has-patch has-unit-tests added

Automatic-deactivation of dependents with unmet dependencies requires a write operation to the database. This was performed in Core's bootstrap, and risked the database and cache becoming out-of-sync on sites with heavy traffic.

No longer loading plugins that have unmet requirements has not had a final approach decided core-wide, and is still in discussion in another ticket that will be handled in the future.

The plugin_data option, used to persistently store plugin data for detecting unmet dependencies in the bootstrap, is no longer needed.

@swissspidy commented on PR #6123:


2 months ago
#18

There were 3 failures:

1) Tests_Multisite_Network::test_active_network_plugins
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 (
-    0 => '/var/www/tests/phpunit/data/plugins/hello.php'
-)
+Array &0 ()

/var/www/tests/phpunit/tests/multisite/network.php:221
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:106
/var/www/vendor/bin/phpunit:118

2) Tests_Multisite_Network::test_duplicate_network_active_plugin
Failed asserting that actual size 0 matches expected size 1.

/var/www/tests/phpunit/tests/multisite/network.php:247
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:106
/var/www/vendor/bin/phpunit:118

3) Tests_Multisite_Network::test_is_plugin_active_for_network_true
Failed asserting that false is true.

/var/www/tests/phpunit/tests/multisite/network.php:261
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:106
/var/www/vendor/bin/phpunit:118

#19 @costdev
2 months ago

  • Keywords needs-testing added

Update: The CI errors are now resolved and PR 6123 is ready for testing.

#20 @costdev
2 months ago

  • Keywords has-testing-info added

Testing Instructions

These steps define how to test the patch, and indicate the expected behavior.

Note: These instructions are to verify that the patch introduces no regressions to the intended behaviour of Plugin Dependencies, and that auto-deactivation does not occur (will be shown by Expected result 19b).

Setup

  1. Apply the PR.
  2. Run npm run build:dev to build the environment.

Steps to Test

  1. Navigate to Plugins > Add New and search for WPSSO Product Metadata.
  2. Look at the plugin card for WPSSO Product Metadata.
    • Expected results:
      1. The "Install Now" button should be disabled.
      2. The card should contain a notice that additional plugins are required, with two plugins listed and a "More details" link for each.
  3. Click the "More details" link below the "Install Now" button.
    • Expected result: You should see that the "Install Now" button at the bottom of the modal is disabled.
  4. Click the "More details" link beside the first required plugin.
    • Expected result: You should see that the "Install Now" button is enabled.
  5. Click the "Install Now" button.
    • Expected results:
      1. The button should change to "Installing..." with a loading circle.
      2. After a few moments, the button should change to "Installed!", then change to an enabled "Activate" button.
  6. Click the "Activate" button.
    • Expected result: It should change briefly to "Activating..." with a loading circle, then to a disabled "Active" button.
  7. Close the modal and repeat steps 4-6 for the second required plugin.
    • Expected results:
      1. You should see that the "Install Now" button is enabled.
      2. The button should change to "Installing..." with a loading circle.
      3. After a few moments, the button should change to "Installed!", then change to an enabled "Activate" button.
      4. It should change briefly to "Activating..." with a loading circle, then to a disabled "Active" button.
  8. Close the modal for the second required plugin.
    • Expected result: The "Install Now" button for the WPSSO Product Metadata plugin should still be disabled.
  9. Click the "More details" link below the "Install Now" button for WPSSO Product Metadata.
    • Expected result: You should see the "Install Now" button is enabled.
  10. Close the modal and refresh the page.
    • Expected result: You should see that the "Install Now" button for WPSSO Product Metadata is now enabled.
  11. Click the "Install Now" button.
    • Expected results:
      1. It should change to "Installing..." with a loading circle.
      2. After a few moments, the button should change to "Installed!", then change to an enabled "Activate" button.
  12. Click the "Activate" button.
    • Expected result: It should change briefly to "Activating..." with a loading circle, then to a disabled "Active" button.
  13. Navigate to Plugins > Installed plugins.
    • Expected results:
      1. You should see that WPSSO Product Metadata and the two required plugins are active.
      2. The checkbox for the two required plugins should be disabled.
      3. The required plugins should have a disabled "Deactivate" action.
      4. The required plugins should have a "Required by:" line in their description with the WPSSO Product Metadata plugin included.
      5. The required plugins should have a "Note:" that says they cannot be deactivated or deleted until the plugins that require them are deactivated and deleted.
      6. The checkbox for WPSSO Product Metadata should be enabled.
      7. The "Deactivate" action for WPSSO Product Metadata should be enabled.
      8. WPSSO Product Metadata should have a "Requires:" line in its description with the two required plugins listed as links.
  14. Click each of the required plugins links.
    • Expected result: The required plugin links should open a modal that shows that plugin's information, and at the bottom should be a disabled "Active" button.
  15. Close the modal.
    • Expected result: In the description for WPSSO Product Metadata, there should also be a "Note:" that says it cannot be activated until the plugins that require it are activated.
  16. Click "Deactivate" for WPSSO Product Metadata.
    • Expected result: When the page reloads, the "Deactivate" action for the two required plugins should now be enabled.
  17. Click "Deactivate" for each of the two required plugins.
    • Expected result: When the page reloads, the "Delete" action for the two required plugins should be disabled.
  18. Open wp-content/plugins and delete the folder for one of the required plugins.
  19. Refresh the page.
    • Expected results:
      1. There should be a notice saying that required plugins are missing.
      2. WPSSO Product Metadata should still be active.
      3. The "Delete" action for the remaining required plugin should still be disabled.
      4. The "Delete" action for WPSSO Product Metadata should be enabled.
  20. Click the "Delete" action for WPSSO Product Metadata.
    • Expected result: WPSSO Product Metadata should delete successfully.
  21. Refresh the page.
    • Expected result: The "Delete" action for the remaining required plugin should be enabled.
  22. Click the "Delete" action for the remaining required plugin.
    • Expected result: The remaining required plugin should delete successfully.
Last edited 2 months ago by costdev (previous) (diff)

#21 @afragen
2 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6123

Environment

  • WordPress: 6.5-beta1-57630-src
  • PHP: 8.2.16
  • Server: nginx/1.25.4
  • Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.16)
  • Browser: Safari 17.2.1
  • OS: macOS
  • Theme: Twenty Twenty-Four 1.0
  • MU Plugins:
    • WP-CLI: Force Auto-Updates 1.0.0
  • Plugins:
    • Test Reports 1.1.0

Actual Results

  1. ✅ Issue resolved with patch.
  2. Test results followed instructions almost exactly.

Additional Notes

When going to modal after WooCommerce was installed/active, there was an PHP warning in the modal from WooCommerce that cleared. I doubt this is related to the PR.

Also, all PHPUnit tests for Tests_Admin_WPPluginDependencies pass.

Last edited 2 months ago by afragen (previous) (diff)

#22 @zunaid321
2 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6123.diff

Environment

  • WordPress: 6.5-beta1-57630-src
  • PHP: 8.2.12
  • Server: nginx/1.25.3
  • Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.12)
  • Browser: Chrome 121.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Four 1.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.1.0
    • WooCommerce 8.6.0
    • WPSSO Core 17.13.0
    • WPSSO Product Metadata for WooCommerce SEO 4.1.1

Actual Results

  • 2a. The "Install Now" button is disabled. ✅
  • 2b. The card contains a notice that additional plugins are required, with two plugins listed and a "More details" link for each. ✅
  • 3. The "Install Now" button at the bottom of the modal is disabled. ✅
  • 4. The "Install Now" button is enabled. ✅
  • 5a. The "Install Now" button changes to "Installing..." with a loading circle. ✅
  • 5b. After a few moments, the button changes to "Installed!", then changes to an enabled "Activate" button. ✅
  • 6. The "Activate" button changes briefly to "Activating..." with a loading circle, then to a disabled "Active" button. ✅
  • 7a. The "Install Now" button is disabled. ✅
  • 7b. The "Install Now" button changes to "Installing..." with a loading circle. ✅
  • 7c. After a few moments, the button changes to "Installed!", then changes to an enabled "Activate" button. ✅
  • 7d. The "Activate" button changes briefly to "Activating..." with a loading circle, then to a disabled "Active" button. ✅
  • 8. The "Install Now" button for the WPSSO Product Metadata plugin is still disabled. ✅
  • 9. The "Install Now" button is enabled. ✅
  • 10. The "Install Now" button for WPSSO Product Metadata is now enabled. ✅
  • 11a. The "Install Now" button changes to "Installing..." with a loading circle. ✅
  • 11b. After a few moments, the button changes to "Installed!", then changes to an enabled "Activate" button. ✅
  • 12. The "Activate" button changes briefly to "Activating..." with a loading circle, then to a disabled "Active" button. ✅
  • 13a. The WPSSO Product Metadata plugin and its required plugins are active. ✅
  • 13b. The checkbox for the two required plugins is disabled. ✅
  • 13c. The required plugins have a disabled "Deactivate" action. ✅
  • 13d. The required plugins have a "Required by:" line in their description with the WPSSO Product Metadata plugin included. ✅
  • 13e. The required plugins have a "Note:" that says they cannot be deactivated or deleted until the plugins that require them are deactivated and deleted. ✅
  • 13f. The checkbox for WPSSO Product Metadata is enabled. ✅
  • 13g. The "Deactivate" action for WPSSO Product Metadata is enabled. ✅
  • 13h. WPSSO Product Metadata has a "Requires:" line in its description with the two required plugins listed as links. ✅
  • 14. The required plugin links open a modal that shows that plugin's information, and at the bottom is a disabled "Active" button. ✅
  • 15. In the description for WPSSO Product Metadata, there is also a "Note:" that says it cannot be activated until the plugins that require it are activated. ✅
  • 16. When the page reloads, the "Deactivate" action for the two required plugins is enabled. ✅
  • 17. When the page reloads, the "Delete" action for the two required plugins is disabled. ✅
  • 19a. There is a notice saying that required plugins are missing. ✅
  • 19b. WPSSO Product Metadata is still active. ✅
  • 19c. The "Delete" action for the remaining required plugin is still disabled. ✅
  • 19d. The "Delete" action for WPSSO Product Metadata is enabled. ✅
  • 20. WPSSO Product Metadata deletes successfully. ✅
  • 21. The "Delete" action for the remaining required plugin is enabled. ✅
  • 22. The remaining required plugin deletes successfully. ✅

#23 @costdev
8 weeks ago

  • Keywords commit added; dev-feedback needs-testing removed

PR 6123 has multiple successful test reports.

Marking for commit.

#24 @costdev
8 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57658:

Plugin Dependencies: Remove auto-deactivation and bootstrapping logic.

Automatic deactivation of dependents with unmet dependencies requires a write operation to the database. This was performed during Core's bootstrap, which risked the database and cache becoming out-of-sync on sites with heavy traffic.

No longer loading plugins that have unmet requirements has not had a final approach decided core-wide, and is still in discussion in #60491 to be handled in a future release.

The plugin_data option, used to persistently store plugin data for detecting unmet dependencies during Core's bootstrap, is no longer needed.

Follow-up to [57545], [57592], [57606], [57617].

Props dd32, azaozz, swissspidy, desrosj, afragen, pbiron, zunaid321, costdev.
Fixes #60457. See #60491, #60510, #60518.

@costdev commented on PR #6123:


8 weeks ago
#25

Committed in r57658.

Note: See TracTickets for help on using tickets.