Upgrade/Install: Retry an update check whose result could not be stored.#12596
Open
gunjanjaswal wants to merge 3 commits into
Open
Upgrade/Install: Retry an update check whose result could not be stored.#12596gunjanjaswal wants to merge 3 commits into
gunjanjaswal wants to merge 3 commits into
Conversation
wp_version_check(), wp_update_plugins() and wp_update_themes() record a last_checked timestamp before contacting the API so that concurrent requests do not all issue the same remote request. The write that stores the result was made without looking at its return value, so a failure left that timestamp behind on stale data. Subsequent checks returned early until the timeout expired, then failed and re-armed the lock the same way, leaving the site permanently unable to see updates, including security updates, while reporting itself up to date. Check the return value and reset last_checked when the result was not stored, so the next request retries the check. A false return is not conclusive on its own, since update_site_option() also returns false when the value it is asked to store is already the stored value. That happens routinely here, because the result of a check that found nothing new is identical to the object stored when the lock was taken whenever the API responds within the same second. The two are compared before the lock is rolled back so an unchanged value is not mistaken for a failure. Props madhazelnut, siliconforks, soyebsalar01. Fixes #64550.
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…e stored The two tests that force the result write to fail also trigger the wp_trigger_error() warning the fix emits, which the test suite promotes to an error. Capture the warning with an error handler, following the approach used in tests/phpunit/tests/user.php, and assert it was raised so the developer notice stays covered.
… use set_site_transient() writes through wp_cache_set() when an external object cache is present, so the pre_update_site_option filter these two tests rely on to reject the write never runs and no failure is simulated. Skip them in that configuration, matching tests/phpunit/tests/option/siteTransient.php.
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
1 participant
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.
wp_version_check(),wp_update_plugins()andwp_update_themes()record alast_checkedtimestamp before contacting the API, then store the result without checking whether that write succeeded. When it fails, the timestamp is left behind on top of stale data, so later checks return early until the timeout expires, then fail and re-arm the lock the same way. The site stops being offered updates at all, security updates included, while reporting itself up to date.This checks the return value and resets
last_checkedwhen the result was not stored, so the next request retries instead of trusting a check that never persisted.One wrinkle worth reviewer attention: a
falsereturn is not conclusive on its own.update_site_option()also returnsfalsewhen the value it is asked to store already matches the stored value, and that happens on the happy path whenever the API answers within the same second the lock was taken. Rolling back on everyfalsewould send the site back out to the API on each request, so the intended value is compared against the lock object before anything is rolled back.The other open question is the retry interval. Resetting
last_checkedto 0 means a site whose payload genuinely cannot be stored retries on the next request rather than once every twelve hours. That turns a silent permanent failure into a noisy recoverable one, which seems like the right trade, but if a backoff is preferred over an immediate retry that is an easy change.Tests cover the plugin path: the happy path staying unchanged, the lock resetting when the write is rejected, the next check actually re-running, and the unchanged-value case not being mistaken for a failure. Core and themes share the same code path.
Trac ticket: https://core.trac.wordpress.org/ticket/64550