Add rollback for failed plugin/theme updates

[pbiron]

When a core auto-update fails, core has long had the ability to automatically attempt a "rollback" to the latest stable release (in the branch that the site is running). Note: for core auto-updates, "rollback" is not attempted for certain failures (e.g. "disk full", etc).

This capability should be extended to be available for plugin and theme updates.

[note: originally this description mentioned `auto-updates`...that was a mistake on my part. This is about rollback for both `manual` and `auto-updates`]

The current functionality for core rollbacks relies on the availability of a "rollback" package being returned by the Updates API. It is unclear (to me, at this time) whether extending this functionality to plugins/themes will require changes to the API or not.

Important: This ticket is not about backup/restore in case a site admin later determines that a successful plugin/theme update actually causes problem on a site!!!! Rather it is about covering cases such as #51823, where a plugin/theme update failure happens between the time the existing plugin/theme is deleted but before the new version successfully installed.

[afragen]

Just a quick thought. The plugin/theme update transient could have an additional element containing the most recent download package. Then some error during the update process might be able to attempt an update using the alternate download package.

[afragen]

It seems that some additional error reporting is needed. In referencing the core failure data provided by @dd32 in ​https://wordpress.slack.com/archives/CULBN711P/p1606869594134200 it seems that copy_dir() is a focal point of failure.

In digging a little if $wp_filesystem->dirlist( $from ) is false what happens is the contents of the plugin folder is deleted and the folder remains.

I added a patch to test for this and return a WP_Error. At a minimum it can give us more information. Perhaps it can mitigate this type of failure.

[afragen]

In copy_dir() check $wp_filesystem->dirlist() and return WP_Error if false

[afragen]

only test on first pass

[afragen]

Only running of the first pass of copy_dir() allows plugins to empty subfolders.

[audrasjb]

@afragen indeed this would at least prevent the updater to delete plugin without being able to upload the new version.

I tested it by changing writing permissions on my test install and it looks good to me (plugin not deleted). Is it the best way to test this patch?

[afragen]

@audrasjb part of the issue is that I don't even know if this is issue. It does seem to solve this particular problem ( $dirlist returning false ).

I test by settings a breakpoint after $dirlist = $wp_filesystem->dirlist( $from ); in copy_dir() and then setting $dirlist = false; in VSCode.

I'm working on sending this failure data to dot org but I have no way to tell if they get it. I'll make another ticket for that.

[afragen]

See #51928 as possible method of data collection by dot org.

[afragen]

pass slug in WP_Error instead of FS_METHOD

[afragen]

The Automatic Upgrader Skin doesn't really pass any specific data about plugin/theme so I'm passing the slug in WP_Error as something to help.

[afragen]

51857.4.diff​ added rollback of plugin/theme to patch.

To test copy_dir() must return a WP_Error. This would likely happen if $dirlist = false.

[afragen]

add rollback

[afragen]

also update _copy_dir() per @dd32 rec

[poena]

During today's scrub,
@afragen suggested that:

Easiest way to test is install patch, set a breakpoint in copy_dir() after $dirlist assignment and set $dirlist = false; This will return WP_Error and trigger the rollback during a plugin/theme update.

[TimothyBlynJacobs]

I'm not sure if zipping a copy is the right approach here. In my experience, most sites that have issues with a partial update failure ( ie some files are not copied resulting in a crash ) are due to resources timing out during the upgrade process. By taking a zip, this will significantly increase the amount of processing that needs to be done during the upgrade process and thus may increase the likelihood of failed updates. Further, since this is a fatal error issue, copy_dir won't return a result and the rollback code won't execute in that case.

[afragen]

ATM this is not quite meant to be all encompassing. Some of the issue is getting a WP_Error out of copy_dir().

It is meant to function if the $dirlist returns false as this is clearly an issue likely relating to the download, extraction , copy process. There's another ticket #51928 to help provide more data for where and when failures happen.

The zipping is done before the any unpacking of the downloaded package and I'm not sure that it should increase the server resources significantly. If this can be demonstrated that would be great. But since the failure seems to be in copy_dir(), zipping and unzipping the current plugin seems like a better solution than copying the data somewhere and then copying it back.

[afragen]

I did some quick time tests locally and it takes almost no time to create a zip even of something as large as Jetpack.

[afragen]

It would be helpful to test this on slow servers to see if the zipping process introduces any potential timeout issues.

Please post results here.

[audrasjb]

I successfully tested the above patch on the worst server I can access to: 1 vCore, RAM 2Gb, 20 Gb SATA SSD. That's one of the cheapest services provided by big hosting companies here in France.
I'll try to find a non-SSD server next week.

But FWIW: rollback worked :-)

[mikeschroder]

Replying to TimothyBlynJacobs:

I'm not sure if zipping a copy is the right approach here. In my experience, most sites that have issues with a partial update failure ( ie some files are not copied resulting in a crash ) are due to resources timing out during the upgrade process. By taking a zip, this will significantly increase the amount of processing that needs to be done during the upgrade process and thus may increase the likelihood of failed updates. Further, since this is a fatal error issue, copy_dir won't return a result and the rollback code won't execute in that case.

This was brought up in the hosting community channel for discussion and testing, but wanted to leave a quick note that this is also my experience.

[afragen]

Mike, are you saying you have seen/experienced this problem or that you agree it's a theoretical concern?

[a2hosting]

I'd just like to add in that we've seen what should be very small zip tasks called from PHP fail if there are resource issues and that should be avoided if at all possible in cases such as these.

[afragen]

Can we please try to obtain actual data? Let's try to test the actual patch.

I understand theoretical issues, but I also understand that one type of zip task is not the same as another.

[afragen]

From the description.

The current functionality for core rollbacks relies on the availability of a "rollback" package being returned by the Updates API. It is unclear (to me, at this time) whether extending this functionality to plugins/themes will require changes to the API or not.

Obtaining the currently installed version package from dot org is possible but attempting to reinstall it in the same manner as the failure doesn't seem like the practical way forward.

This approach also has the issue of not working with any plugin/theme not hosted on dot org. I think this is a hard block.

[afragen]

Lots of discussion between @TimothyBlynJacobs and myself, ​https://wordpress.slack.com/archives/CULBN711P/p1609968242405800

My impression of the conclusion is that if there is a server resource issue that occurs a result of the zip creation process then a simple page reload brings the site back to it's previous state showing a plugin/theme update being available.

This shouldn't be an issue as long as the frequency of this happening is rare. If this is happens with small plugins it's an issue.

If it happens with large plugins like Mailpoet, Jetpack, Yoast SEO, etc. then perhaps it's not ideal to run a resource intensive site on a resource poor shared environment.

[TimothyBlynJacobs]

My impression of the conclusion is that if there is a server resource issue that occurs a result of the zip creation process then a simple page reload brings the site back to it's previous state showing a plugin/theme update being available.

I think this is accurate. But the failure happening _during_ the zip process specifically is not my big worry, but that it consumes resources so that when the file transfer happens, the site no longer has enough resources to complete the upgrade. Which is how you can end up with a fatal erroring site.

[afragen]

bubble up error for extract_rollback

Moving previous patch(es) to GitHub.

• Updates copy_dir() and _copy_dir() to include WP_Error return early if the $dirlist is empty.
• Adds function zip_to_rollback_dir() which creates a zip of the current plugin/theme that is being updated.
• Adds function extract_rollback() which on a WP_Error returned from copy_dir() will extract the above zip back into the appropriate location.

Trac ticket: https://core.trac.wordpress.org/ticket/51857

[mikeschroder]

Replying to afragen:

Mike, are you saying you have seen/experienced this problem or that you agree it's a theoretical concern?

Sorry, I missed this question before. I'm saying I've seen and experienced this problem.
It is not a theoretical concern.

I agree that data would be helpful here, whether from the WordPress project, or hosts.

I'll ask around a bit and see if I can find recent data. It is one of those things that I helped with a lot in tech support, but that was some time ago.

[SergeyBiryukov]

Thanks for the patch!

It looks like there are some concerns about performance implications of building (and then potentially unpacking) a zip file with the current plugin version, especially for large plugins. Instead of building a zip file, could we rename the plugin directory to something reasonably unique like plugin-slug.__rollback__, and then delete it on successful update, or rename back on failure? I think that would not require any additional resources. Creating a zip file might save some disk space, but if the space is already low enough for that to make a difference, then it seems like updates on that site would start failing anyway sooner rather than later.

Personally, I would like the code for plugin/theme upgraders to be as close to core as possible. At a glance, it looks like Core_Upgrader::upgrade() has a lot more than Plugin_Upgrader::upgrade() or Theme_Upgrader::upgrade(), so perhaps unifying these methods and documenting the differences would be a good first step. I would prefer to adapt the core upgrader for plugins and themes as appropriate, and not create a completely different implementation.

[afragen]

@SergeyBiryukov there’s a fundamental problem with trying to use the same method for a rollback that is the cause of the failure in the first place. In this case copy_dir().

[afragen]

Replying to SergeyBiryukov:

Personally, I would like the code for plugin/theme upgraders to be as close to core as possible. At a glance, it looks like Core_Upgrader::upgrade() has a lot more than Plugin_Upgrader::upgrade() or Theme_Upgrader::upgrade(), so perhaps unifying these methods and documenting the differences would be a good first step. I would prefer to adapt the core upgrader for plugins and themes as appropriate, and not create a completely different implementation.

Sounds like this should be another ticket.

[afragen]

Replying to SergeyBiryukov:

Thanks for the patch!

It looks like there are some concerns about performance implications of building (and then potentially unpacking) a zip file with the current plugin version, especially for large plugins. Instead of building a zip file, could we rename the plugin directory to something reasonably unique like plugin-slug.__rollback__, and then delete it on successful update, or rename back on failure? I think that would not require any additional resources. Creating a zip file might save some disk space, but if the space is already low enough for that to make a difference, then it seems like updates on that site would start failing anyway sooner rather than later.

Thanks for the feedback.

Unfortunately rename() can be flaky/unreliable and fallback from rename() involves file copy which is the likely cause of the failure in the first place. Without testing the patch are we looking to solve a problem that doesn't exist?

A server timeout issue is mostly fixed in a page reload and the site isn't likley any worse off than it currently would be.

Isn't this what alpha is for? Unless there's some other concern?

[TimothyBlynJacobs]

A server timeout issue is mostly fixed in a page reload and the site isn't likley any worse off than it currently would be.

Calling this out again.

But the failure happening _during_ the zip process specifically is not my big worry, but that it consumes resources so that when the file transfer happens, the site no longer has enough resources to complete the upgrade. Which is how you can end up with a fatal erroring site.

[dd32]

Replying to SergeyBiryukov:

It looks like there are some concerns about performance implications [...] could we rename the plugin directory to something reasonably unique like plugin-slug.__rollback__, and then delete it on successful update, or rename back on failure?

This looks like the best option from a filesystem-io perspective to me.

Replying to afragen:

Unfortunately rename() can be flaky/unreliable and fallback from rename() involves file copy which is the likely cause of the failure in the first place.

I'm not sure what the flaky/unreliable experience is here, but I feel like creating a ZIP backup is solving a problem that doesn't actually exist while adding significant CPU/IO into the process at the same time that's not needed.

Looking at the filesystem handlers move(), it looks like the Direct IO one has some behaviour to use a copy, which _probably_ isn't needed and is probably not useful, [13001] added the usage of rename() into it. A lot has changed in the last decade host IO wise.

Also of note, is that the FTP variants don't check the overwrite functionality.. but that's mostly irrelevant here.

I would probably suggest just cleaning up those move() methods to be consistent and not fallback on a copy() if it's not actually performing a move()/removing the source directory.

IMHO if move() fails, either the delete of the existing directory will also fail, or rollback should just be skipped for those edge-cases.

[dd32]

An alternative option would be to flip this around, and not add a rollback functionality into it, but rather install into a temporary directory first, and then remove existing/rename new into place.

(edit: I guess that's kind of how it works now, except the temporary install directory is wp-content/upgrade/somethinghere. It's the copy from there to the plugins directory that sometimes fails, so avoiding that copy by extracting into the plugins directory in the first place and then performing a move() would mean far less IO is needed and as a result could be faster too)

That would result in less downtime of the plugin not existing on the site, and also not delete the plugin from the site in the event of a failure to create the new files.

eg:

• Plugin update into .hello-dolly.upgrade.1234..
• Once successful:
  • hello-dolly directory is deleted.
  • move( '.hello-dolly.upgrade.1234', 'hello-dolly' );
  • return true;
• Upon failure:
  • Delete .hello-dolly.upgrade.1234
  • return false

[dd32]

Replying to dd32:

I guess that's kind of how it works now, except the temporary install directory is wp-content/upgrade/somethinghere. It's the copy from there to the plugins directory that sometimes fails

I think this might have originally been a copy, rather than a move, as moving items cross-partition isn't possible. Sometimes each site would have it's own wp-content directory, but the wp-content/plugins or themes folder was actually a symlink to another disk or some such.

I could also be mis-remembering it, and it was purely bad planning and development skills that I wrote it like this :) If I was doing it again, I'd do it how I've outlined in comment 56 above

[hellofromTonya]

From core scrub today, @afragen noted that he'll open a new ticket to address the copy_dir changes Dion mentioned above.

For this ticket though, just want to make sure we all understand. Replying to dd32:

An alternative option would be to flip this around, and not add a rollback functionality into it, but rather install into a temporary directory first, and then remove existing/rename new into place.

Hey @dd32 > Is the proposed implementation how it currently works?

If yes, then nothing else needs to be done in this ticket.

[hellofromTonya]

Marking as a close candidate. Why? Our understanding is that Dion's proposed solution does not require any additional work in this ticket. And it resolves the server resources concerns.

[SergeyBiryukov]

It's my understanding that comment:56 is not exactly how it currently works and still needs to be implemented, so I don't think the ticket should be closed yet.

[hellofromTonya]

Closing this ticket for now as a maybelater. Why?

• #52342 is the new direction proposed by Dion.
• #52381 proposes a new filter to enable moving this ticket into a feature plugin.

[dd32]

Just noting, that while this has been closed as maybelater, #52342 is not the suggestion as I mentioned above, and #52381 is a suggestion for a filter to make this ticket possible as a feature plugin, which could've just been kept here.

[SergeyBiryukov]

Reopening per comment:62 and comment:67.

[SergeyBiryukov]

I think comment:56 is the preferred direction here.

[KZeni]

Question... does this also accommodate a plugin/theme update that's fully completed (no issue during copying of files or anything like the example stated), but then actually has a bug within the code of the latest release that causes a fatal server error?

I know WP has precautions in place when trying to activate a plugin that'll cause a server error so I'm thinking this might be the case for what's being proposed here (just applying to when updating a plugin with it then rolling back if an error is encountered.)

If that isn't being accommodated, currently, is there anything preventing that situation from being included in this functionality as well?

[afragen]

The intent of this is currently accommodating update failures, not complete updates where the new version causes a fatal.

[KZeni]

Replying to afragen:

The intent of this is currently accommodating update failures, not complete updates where the new version causes a fatal.

I wanted to check since I'm not certain on what it might entail to expand this to include that scenario as well if it didn't already (seems like it could be a natural expansion on the idea.)

For what it's worth, I'm thinking it could be an incredibly helpful thing that could greatly improve ongoing reliability of WordPress sites if it's a reasonable thing to consider for inclusion as part/expansion of this. I mean, unfortunately, there are still plugins out there that have a release buggy release from time to time (ex. I encountered 2 in the last month with them being widely used [one having 100,000+ active installs] plugins where the issue really did live in the plugin release itself), and this is more important now that auto-updates are becoming more of a thing since a buggy release that auto-updates (or even is encountered during a manual update) means everyone encounters the issue rather than the sporadic cases where a copy didn't complete properly due to a server shortcoming/hiccup like this is currently set to address.

[pbiron]

@KZeni thanx for the comment.

Personally, I would love to be able to detect whether a plugin/theme update will cause a fatal. However, that would be very hard to do so, since in many cases, such fatals won't occur until the plugin is actually used (e.g., they often don't happen until an admin- or end-user actually does something with the plugin/theme).

Since WP 5.1 was release, there is Fatal Error Protection built into core, where a plugin will be "suspended" if a fatal occurs at run-time and admins will be notified (see the "Fatal Error Protection" section of ​PHP Site Health Mechanisms in 5.1 for more info).

[KZeni]

@pbiron Gotcha. I was just hoping for this since, in my recent case, the Simple History plugin had a syntax error in a recent release where it took down all sites using it for me per the sites being set to auto-update. The other case was the hCaptcha for WordPress plugin where they left in development code where it was trying to require a file that didn't exist.

Both cases were involving a very straightforward & immediate server error simply by way of simply having errors when active without any further action needed (errors in the code itself), and WordPress (running 5.6.2) simply applied the update & had things break site-wide as a result (when it'd be ideal to have it prevent activation or even rollback to the previous version, if encountered.)

Meanwhile, it seems like the kind of situation that the error-checking upon attempting to activate a plugin that'd cause a server error would prevent, but then that protection isn't happening on update & is only on activation (seems like partway towards having plugins not cause server errors where this ticket focuses on issues during installing the new version while it could/should also ideally include the actual plugin being at fault during update just like it does upon the attempted first activation.)

I guess I was hoping that type of error prevention could be inserted during the update process as well somehow while I was uncertain if this particular ticket would take care of that situation (with this ticket's rollback behavior being ideal, if an issue is encountered), it'd be possible to expand this ticket to include that scenario, and/or this ticket is using a different mechanism from that where it'd be a wholly separate request/implementation (with it sounding like the latter, currently & unfortunately, unless these details have helped clarify things.)

[SergeyBiryukov]

Replying to SergeyBiryukov:

I think comment:56 is the preferred direction here.

Related: #52832

[galbaras]

How about treating old versions like trashed items and deleting them later?

This can be easily done by a background job, as well as manual links, e.g. "remove previous version backup" in the plugin list and a "remove previous version backups" action for selected plugins?

[SergeyBiryukov]

#52832 was marked as a duplicate.

[galbaras]

I think comment:56 is the preferred direction here.

This great approach can be combined with avoiding copy_dir() altogether and using move() again.

1. Unpack the zip file to hello.update.1234
2. If unsuccessful, return false
3. If hello.old exists, remove it
4. Move hello to hello.old
5. Move hello.update.1234 to hello
6. Later on (manually and/or scheduled), remove hello.old.

move() should fail a lot less often than copy_dir(), especially since it uses delete & copy anyway when direct renaming fails.

This should greatly simplify the entire upgrade process, make it faster and provide a way to recover from problems by quickly restoring hello.old to whence it came, also by deleting and renaming.

[desrosj]

As far as I can tell, this effort has been moved to a feature plugin. With 5.8 feature freeze fast approaching (a few short weeks away) and no published merge proposal, I think the reasonable early cutoff has passed. I'm going to punt this to Future Release for now.

If that's an incorrect assessment, please do move back and add the needed context.

[richards1052]

I am a victim of this auto update failure. Two plug-ins, Yoast and Mailpoet, when they update have failed to do so multiple times AND disappeared from my directory.

So when I discover they're missing (long after they've been deleted) I have to manually reinstall them.

I'm surprised a roll back hasn't been implemented for failed plugin upgrades. Also disappointed the auto updater doesn't notify the user when such failures occur. It would permit me to discover the failure, notice the disappearance and reinstall immediately, rather than waiting till I notice the plugin is gone.

[afragen]

Replying to richards1052:

I am a victim of this auto update failure. Two plug-ins, Yoast and Mailpoet, when they update have failed to do so multiple times AND disappeared from my directory.

So when I discover they're missing (long after they've been deleted) I have to manually reinstall them.

I'm surprised a roll back hasn't been implemented for failed plugin upgrades. Also disappointed the auto updater doesn't notify the user when such failures occur. It would permit me to discover the failure, notice the disappearance and reinstall immediately, rather than waiting till I notice the plugin is gone.

Have you tried installing the feature plugin Rollback Update Failure??

[richards1052]

I probably will. But I have so many plug-ins that I try to restrain myself from adding another.

[pbiron]

Richard, please do install and test/use ​Rollback Update Failure.

It is basically a proposal for how core could do rollbacks and we need real world usage of the proposed solution before we're comfortable incorporating it into core.

[galbaras]

Replying to SergeyBiryukov:

#52832 was marked as a duplicate.

It's related, but not a duplicate, unless my suggestions are integrated into this change. So far, the feature plugin isn't covering them, though, because it's somewhat of an alternative approach to the same thing.

Does anyone disagree that moving is quicker and more flexible than copying?

[SergeyBiryukov]

Replying to galbaras:

Replying to SergeyBiryukov:

#52832 was marked as a duplicate.

It's related, but not a duplicate, unless my suggestions are integrated into this change. So far, the feature plugin isn't covering them, though, because it's somewhat of an alternative approach to the same thing.

Does anyone disagree that moving is quicker and more flexible than copying?

I think moving instead of copying is exactly what was proposed in comment:56, and is the preferred direction for this ticket, as previously noted in comment:69.

The feature plugin indeed offers an alternative approach that could be added later, if still necessary after the approach from comment:56 is implemented.

[afragen]

comment:56 is not a rollback provision per se but a new method in the course of plugin updating.

[galbaras]

Looks like this ticket is getting so long that we just look at the tail end of it. Thank you @SergeyBiryukov for clarifying.

@afragen Given the preferred direction, the only failure scenarios I see are when WP fails to delete the existing directory or move the temporary directory to the live one, likely due to insufficient permissions, which should almost never happen.

An even rarer case is when part of the directory is deleted/moved, and some fails to be deleted/moved.

In the entire deletion fails, WP should return false (with some meaningful message), but at least the old plugin is still there. If only some of the deletion fails, or the move fails (partly or completely), WP may be crippled, but rolling back may suffer the same fate :(