WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#34878 closed defect (bug) (fixed)

Core Auto Updater Needs to be Aware of Manual Updates

Reported by: aaroncampbell Owned by: dd32
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: Cc:
PR Number:

Description

Some of the "critical failures" that we get for automatic core updates of major releases are actually a race condition between a manual update and an auto update.

The auto updater uses an option called auto_updater.lock to avoid race conditions with itself, but that needs to be extended to account for manual updates as well.

Example:

  • A user clicks upgrade and a manual upgrade starts
  • WordPress checks and sees that an update is needed and starts and auto update
  • The zip is downloaded and extracted by the manual upgrade and it begins to copy files
  • The zip is downloaded and extracted to the same location by the auto-update upgrade and it begins to copy files
  • The manual update finishes and removes the files it was copying from
  • The auto-updater is still trying to copy files, but they no longer exist because the manual updater removed them.

This can happen in either order (manual update can fail because the auto updater finished first).

These two upgrade processes need to be aware of each other.

Attachments (1)

34878.diff (15.4 KB) - added by dd32 4 years ago.

Download all attachments as: .zip

Change History (9)

#1 @dd32
4 years ago

  • Milestone changed from Future Release to 4.5
  • The zip is downloaded and extracted to the same location by the auto-update upgrade and it begins to copy files
  • The manual update finishes and removes the files it was copying from

For what it's worth, this is no longer the case in 4.4+, both updates will now succeed as they're copying from unique directories, but we should still attempt to prevent the two updates occurring at the same time to improve the chances that the one which is running will succeed.

Another scenario is where two site admins see the update alert, and hit update at roughly the same time.

@dd32
4 years ago

#2 follow-up: @dd32
4 years ago

  • Keywords has-patch needs-testing added

34878.diff is a first pass at this.

To test, I suggest adding a sleep(120) after locking and attempting to start another manual update in another window.

One thing worth noting in this patch, was I introduced wp_create_lock() and wp_release_lock(), these may be moved internally to the upgraders.

#3 in reply to: ↑ 2 @ocean90
4 years ago

Replying to dd32:

One thing worth noting in this patch, was I introduced wp_create_lock() and wp_release_lock(), these may be moved internally to the upgraders.

#34330 is about introducing a locks API.

#4 @dd32
4 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

In 36349:

Core Upgrader: Add a locking mechanism to avoid two concurrent updates of WordPress occuring.

Fixes #34878

#5 @danielbachhuber
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

WP_Automatic_Updater::run() calls $this->create_lock() and $this->release_lock(), both methods of WP_Upgrader, which causes a fatal: https://travis-ci.org/wp-cli/wp-cli/jobs/103627421

#6 @dd32
4 years ago

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

In 36370:

Upgrade: Switch the locking mechanism to using static methods so that it can be accessed from other upgrade-classes.

Fixes #34878

#7 @DrewAPicture
4 years ago

In 36821:

Docs: Improve documentation for WP_Upgrader::create_lock(), introduced in [36349].

See #34878. See #35986.

#8 @DrewAPicture
4 years ago

In 36822:

Docs: Improve documentation for WP_Upgrader::release_lock(), introduced in [36349].

See #34878. See #35986.

Note: See TracTickets for help on using tickets.