Add tolerance to plugin and theme auto-update failure emails

[desrosj]

From @johnbillion over on ticket:50268#comment:12:

This may be a separate issue, but under what conditions might a plugin automatic update fail and therefore trigger the failure email message? If it does fail and the problem is persistent, when does it next attempt to update? If I don't check my emails for let's say three days, how many failure emails might I have in my inbox?

I ask in case we need to consider some failure tolerance here to avoid too many failure emails being sent out.

Reading through the code that implements these auto-updates, it seems that these are the scenarios where a plugin or theme auto-update could potentially fail.

1. The upgrader cannot connect to the file system.
2. Package could not be unzipped.
3. Old version of plugin could not be deleted.
4. New version cannot be copied into the plugin's folder.
5. An error occurs downloading the update package.
6. Signature verification fails (eventually, not currently enforced).
7. No plugin is found in the update package.
8. The plugin is already at the latest version.

Items 1-4 are likely to re-occur until admin action is taken, 5-7 potentially could re-occur, but most likely indicate a problem upstream, and 8 will most likely not re-occur.

Since auto-updates will be attempted every 12 hours by default, some tolerance should probably be added so that emails are only sent every few days when issues occur.

[desrosj]

Summarizing a conversation that I had with @arpitgshah here for future reference.

As the feature stands now, email notifications are not tracked. One way to approach this would be:

When an update fails, check the plugin is listed in the failure list (as stored in an option). If not, it should be added to an array with a timestamp of the failure as key value pair. Something like $failure_emails[ 'akismet/akismet.php' ] = current_time( 'timestamp' ); for example.

If that plugin is present in the array and it has not been 3 days since the timestamp, an email can be sent and the timestamp should be updated.

The duration between emails should be filterable, but 3 days by default seems fine.

When a plugin is updated successfully, it should be removed from the failure list if present. Any failure after a successful update is a new occurrence and does not need to be throttled.

Trac ticket:

https://core.trac.wordpress.org/ticket/50448

[arpitgshah]

Hey All,

I have added the patch for the checking failure plugins and theme update with store in database and check for the >=3 days to add within the auto update emails with wp_version_check cron.

Result to store values in database,

New option_name - failed_update_plugins_themes

Array Result -

<?php
Array
(
    [plugin] => Array
        (
            [0] => Array
                (
                    [name] => Contact Form 7
                    [failure_time] => 1593519763
                    [file_name] => contact-form-7/wp-contact-form-7.php
                )
            [1] => Array
                (
                    [name] => Akismet Anti-Spam
                    [failure_time] => 1593519906
                    [file_name] => akismet/akismet.php
                )
        )
    [theme] => Array
        (
            [0] => Array
                (
                    [name] => Storefront
                    [failure_time] => 1593520948
                    [file_name] => storefront
                )
        )
)

Please let me know your feedback for the same.

[audrasjb]

Hi @arpitgshah thanks for the patch!
There are few WPCS issues to address in your patch, I added few comments in the GitHub pull request.

[desrosj]

I've been working through this one over on GitHub with @arpitgshah. My ​latest push to that PR has some refined code I think is getting close.

I took the following approach:

• When a failure email is being sent, only send an email if there is a new failure in the list. When there is a new failure in the list, the email needs to go out anyways, so it's OK to include other failures that may have been sent too recently.
• When a mixed email is sent, that means there are new successful updates. An email is going out anyways for those updates, so include the failure info as well (even if it is too recent).

The only scenario that is not addressed in the recent patch is when the user goes manually applies an update, whether through clicking update in the dashboard or by uploading a new version of the plugin or theme manually.

For the first scenario, it would probably be an edge case for someone to update a plugin and then have a failure within 3 days. But if the interval is filtered to a larger number, it could be more common. The new option can probably be checked when plugins upgrade manually as well.

For the second scenario, I don't know that there is a standardized way to tell if a new plugin version was updated manually. but I could be wrong.

[arpitgshah]

Hey @desrosj, I have revised some code in PR. Added some code for the following,

Recent patch is when the user goes manually applies an update, whether through clicking update in the dashboard or by uploading a new version of the plugin or theme manually.

[desrosj]

50448.diff​ is an updated diff that will reset a plugin's failed auto-update email notification tracking when successfully updated by initiating an update within the admin.

I'm not sure how to address updates that are applied by manually uploading the new version plugin.

[desrosj]

50448.2.diff​ is the correct patch, ignore the first one, which was lacking the email tracking invalidation for themes.

Also, 50448.2.diff​ specifies the new option in the schema for addition during the database update. The reason I decided to include this there is to that the option can be added with autoload = 'no'. The information in that option is only needed during a plugin or theme update, which is a very small number of requests.

[pbiron]

Thanx for the updated patch @desrosj.

On first read it looks pretty good (and autoload = 'no' is appropriate here, I think). I'm in the middle of something else right now, but will give it a test as soon as I finish that.

[arpitgshah]

@desrosj - Have you checked with this logic for manually update or removed by admin?

<?php
// remove from the failed_plugin_theme_update_emails list if plugin or theme version was update manually or removed by admin

if( $successful_plugins || $successful_themes || $failed_plugins || $failed_themes )
{
    $temp_merge_all = array();
    
    if( $successful_plugins ) {
        foreach ( $successful_updates['plugin'] as $item ) {
            // set key for the intersect
            $temp_merge_all[ $item->item->plugin ] = $item->name; 
        }
    }
    if( $successful_themes ) {
        foreach ( $successful_updates['theme'] as $item ) {
            // set key for the intersect
            $temp_merge_all[ $item->item->theme ] = $item->name; 
        }
    }
    if( $failed_plugins ) {
        foreach ( $failed_updates['plugin'] as $item ) {
            // set key for the intersect
            $temp_merge_all[ $item->item->plugin ] = $item->name;  
        }
    }
    if( $failed_themes ) {
        foreach ( $failed_updates['theme'] as $item ) {
            // set key for the intersect
            $temp_merge_all[ $item->item->theme ] = $item->name; 
        }
    }
    
    $failure_emails = array_intersect_key( $failure_emails, $temp_merge_all );
}
                 

If user has been updated manually or removed then one of these 4 variables are going to set and that can be intersect with our database variables and difference gives us the final values that plugins were updated manually or removed by admin.

Does this make sense?

[desrosj]

@arpitgshah Yes, I reviewed that with ​context in the latest commit made on the PR before pushing some changes.

The problem with that code is the send_plugin_theme_email() function is only called when an auto-update is attempted or applied. A user clicking update in the admin would never run that code, so the previous failure tracked within the option would remain. Additionally, it only has knowledge of the updates attempted during the current auto-update process.

The updates included in the latest push to the PR/patch attached here will clear the plugin or theme out of the option if a user clicks update anywhere in the dashboard.

[johnbillion]

This is looking good, but I wonder whether this is over-engineered and could still result in more than one email every three days.

Considering that a failure of a plugin or theme update is relatively rare, could we instead use a transient that's just a flag to say "there was a failure" and set it to expire in 3 days, regardless of which plugin or theme failed to update, and then read that transient to determine whether to send subsequent failure emails?

With the proposed patch, if I've somehow messed up my filesystem and PHP can't write to it and two separate plugins update separately within three days, it looks like that will still result in multiple emails within a three day period. Is that right?

[desrosj]

Replying to johnbillion:

With the proposed patch, if I've somehow messed up my filesystem and PHP can't write to it and two separate plugins update separately within three days, it looks like that will still result in multiple emails within a three day period. Is that right?

If plugin A fails during attempt 1, and then plugin A & B fails during attempt 2 say 12 hours later, then an email needs to be sent to notify the site owner of plugin B failing. So I chose to just include all failure information in that second email. But, if 12 hours later, only plugin A & B fail again (no other failures or successful updates), then no email will be sent.

If the next day, another plugin successfully updates and requires an email, then A&B will be included in that email.

[pbiron]

Here's the way I read it:

1. if there are only failures, then only send an email if
   1. there is a new failure or
   2. it has been 3 days (or whatever the filtered duration is) since the last failure notification was sent
2. if there is a success, then include failures as well, even if the duration has not been exceeded

Number 1 seems very reasonable. Number 2 is questionable, but it seems to me that trying to leave the failures out in that case would, itself, be over engineering it :-)

[arpitgshah]

2.if there is a success, then include failures as well, even if the duration has not been exceeded

Ans. We have unset the variables once plugin comes to success. so it will be removed from the failed list. It will not add into the email lists if it will happen again and again till 3 days. So it looks like following

1) 1st time failed include in email

2) failed passed 3 days of there past failures then it will include in email

[pbiron]

Here's a really picky nit: in wp-admin/includes/class-wp-automatic-updater.php, the variable that holds the auto_plugin_theme_update_emails option is $failure_emails; while in wp-admin/includes/class-{plugin,theme}-upgrader.php, it is $past_failure_emails.

Would it make sense to use the same variable name in all 3 places?

[johnbillion]

Been chatting with Jonathan in Slack, I should clarify my comment above.

If any failure occurs, I don't think there's great benefit in sending any further failure emails within three days, even for a different plugin or theme. The main scenario I would like to avoid is that a user doesn't check their emails for a week and then they come back to an inbox of emails where none of the emails after the first are really telling them anything they don't already know from the first.

[arpitgshah]

@johnbillion - Yes, you are right but if user tried the solutions for the failed plugins and make enable auto updates then also it will be failed then what will be cases? This code help then 1st time and then after 3 days of time, it has stop sending emails again and again for that particular plugin/theme.

[earnjam]

Haven't tested it yet, but on a read through it looks good.

Only thing I noticed is in send_plugin_theme_email(), the $unique_failures variable gets set to false immediately before the conditional, so you don't need to set it again.

<?php
                $unique_failures = false;
                $failure_emails  = get_option( 'auto_plugin_theme_update_emails', array() );

                // When only failures have occurred, an email should only be sent if there are either new failures,
                // or failures that have not sent out an email recently.
                if ( 'fail' === $type ) {
                        $unique_failures = false;

Do we want to add some unit tests for this too?

[arpitgshah]

@earnjam - Yes, you are right and I just have revised the PR now.

[desrosj]

I looked over how Core handles failure emails.

When a Core auto-update attempt fails, a new failure email is not sent unless either the site admin email changes or the site attempts to update to a new version. For example, if the site fails to update to 5.4.1, and then two weeks later the site fails to update to 5.4.2, a new email will be sent.

Instead of using time intervals, we could replace the timestamps with the plugin/theme versions attempting targeted in the update. This would require the option to remain, though, as the amount of time before the next version is released is unknown.

Another interesting idea that @johnbillion had was to not send a failure email the first time the plugin fails to update, just in case it was a hiccup. I do like this option.

Items 1-4 are the only scenarios likely to reoccur that require user intervention. Item 4 above (New version cannot be copied into the plugin's folder) would be the only scenario where there could be problems on the site as a result of a failed update and you would need to know right away, but this scenario would also trigger a WSOD protection email.

[arpitgshah]

@desrosj - Yes, as previous failure tracked within the option would remain, but if we will get the same plugin/theme within the success or not in lists (either option) in next email then we can removed from our lists form that we can know this plugin are updated manually or remove by admin.

Following code does the same.

Replying to desrosj:

@arpitgshah Yes, I reviewed that with ​context in the latest commit made on the PR before pushing some changes.

The problem with that code is the send_plugin_theme_email() function is only called when an auto-update is attempted or applied. A user clicking update in the admin would never run that code, so the previous failure tracked within the option would remain. Additionally, it only has knowledge of the updates attempted during the current auto-update process.

The updates included in the latest push to the PR/patch attached here will clear the plugin or theme out of the option if a user clicks update anywhere in the dashboard.

[desrosj]

I rethought this today and have made the following decisions that @johnbillion agreed with in our Slack conversation. 50448.3.diff​ reflects the changes below.

• Instead of tracking the time of a failure, let's mirror the approach core takes to tracking when emails go out. Now, only one failure email will go out per plugin per version. If a failure occurs trying to update to 2.0, a new failure email will not be generated for this plugin until the version is different (2.1, for example).
• If other update events occur, then all information will be included, even if the failure had already been communicated in an email.

I think this is the best approach for 5.5, and we can iterate as we find edge cases. Failures should be relatively rare. The only scenario I see this being a problem is when a plugin or theme pushes out frequent updates (daily or multiple times weekly). But I'm not sure that is a common practice.

[arpitgshah]

It looks good with new patch.

[desrosj]

In 48397:

Administration: Prevent repeat emails for identical plugin or theme auto-update attempt failures.

This change adds a throttle mechanism to plugin and theme auto-update failure emails using similar logic to the email sent for a Core auto-update.

The first time a plugin or theme auto-update fails, the package and new_version will be tracked in the auto_plugin_theme_update_emails option. An email for this specific update attempt will not be resent.

However, if this update fails again and non-repeat failures or successful updates are also present, then the failure information will be included in that email (an email needs to be sent for the new events regardless).

Props johnbillion, arpitgshah, desrosj, audrasjb, pbiron, earnjam.
Fixes #50448.

[desrosj]

This should be noted in the auto-updates dev note.

[desrosj]

In 48401:

Administration: Fix failing tests as a result of [48397].

Because of changes to how PHP handles arrays used in foreach() loops in PHP >= 7.0, [48397] resulted in a failing test for PHP 5.6.

This calls reset() after using the $results array in the foreach() to ensure the array is treated the same and as expected.

Props azaozz, desrosj, SergeyBiryukov, xknown.
See #50448.

[SergeyBiryukov]

In 48445:

Administration: Handle the result of Plugin_Upgrader::bulk_upgrade() for a plugin that is already at the latest version in the same way it is handled for themes.

This corrects a fragile check of the result in wp_ajax_update_plugin() that depended on the internal array pointer, and brings some consistency with wp_ajax_update_theme().

Follow-up to [37714], [48401].
See #50448.

[audrasjb]

Dev note published: ​https://make.wordpress.org/core/2020/07/30/controlling-plugin-and-theme-auto-update-email-notifications-and-site-health-infos-in-wp-5-5/