[wp-trac] [WordPress Trac] #62176: No stable error detection after Automatic Updates with more than one update

[WordPress Trac]

#62176: No stable error detection after Automatic Updates with more than one update
-----------------------------+-----------------------------
 Reporter:  georgwordpress   |      Owner:  (none)
     Type:  defect (bug)     |     Status:  new
 Priority:  normal           |  Milestone:  Awaiting Review
Component:  Upgrade/Install  |    Version:  6.6
 Severity:  normal           |   Keywords:  2nd-opinion
  Focuses:                   |
-----------------------------+-----------------------------
 While testing rollback of buggy plugins updates I found some times the
 buggy plugin was not detected.
 But even with the same test setup this happens only sometimes.

 In {{{class-wp-automatic-updater.php}}} a risk of a racing condition is
 already noticed as a comment.
 To lower the risk a sleep(2) was introduced as a quick fix.

 In another discussion @stoyangeorgiev added a comment:
 "... another thing we found is that when you update two plugins that have
 fatal errors, It may be possible that the checks for enabling/disabling
 maintenance mode in load.php may not be properly getting the state of the
 maintenance mode. I've managed to replicate it on my setup ... I'll try to
 test it more so I can find a stable way to replicate it."

 After digging deep into the related functions I think I have found the
 problem.

 The detection of errors after automatic plugin updates is done with a
 loopback request to the homepage.
 While a user request is still answered with the maintenance screen this
 loopback request must bypass the maintenance mode to receive his dedicated
 scraping result produced by another function.

 There are two very ancient WP functions related to the wp maintenance
 mode:


 **1.) the function {{{maintenance_mode()}}} **
 Was introduced in WP over 15 years ago.
 This function creates a new PHP script every time it is called, which is
 saved as the well-known .maintenance file.

 Here the greatly simplifies file operations of this function:

 {{{
 @unlink( '.maintenance' );
 $fp = @fopen( '.maintenance', 'wb' );
 fwrite( $fp, '<?php $upgrading = ' . time() . '; ?>' );
 fclose( $fp );
 }}}

 Nothing complicated - but there is a racing condition:
 If in parallel another loading process accesses the maintencance file
 exactly between fopen and fwrite, the file is still empty.

 This hasn't been a problem for the last 15 years because the other
 maintenance function has been very fault tolerant.

 **2.) the function {{{wp_is_maintenance_mode()}}}**

 {{{
 global $upgrading;
 if (file_exists(ABSPATH . '.maintenance') ) require ABSPATH .
 '.maintenance';
 }}}

 At this point, it can happen that the maintenance file already exists but
 does not yet contain any PHP code - so the value of {{{$upgrading}}} is
 not updated.
 But thanks to {{{$upgrading}}} has already been declared as global the
 next line of code is running without any problems.

 {{{
 if ( ( time() - $upgrading ) >= 10 * MINUTE_IN_SECONDS ) return false;
 }}}

 Even with {{{$upgrading}}} beeing NULL when this edge case happens - no
 php errors - simply return false.

 As result: this edge request at the beginning of a maintence phase is not
 answered with the maintenance screen.

 Does it matter?
 No - because putting wp into maintenance mode is only a preparation before
 any changes and updates will really take place. For more than 15 years
 nobody complains.

 What has changed?
 Some times ago the automatic updates of core/themes etc was introduced.
 In the of loop of Automatic Updates the function {{{maintenance_mode}}} is
 called a lot of times now.
 (Before it enters the plugins section the function {{{maintenance_mode}}}
 is called even twice in every round.)

 And as mentioned above with every call of {{{maintenance_mode}}} the
 sequence of {{{@unlink/@fopen/fwrite/fclose}}} is deleting and writing the
 maintenance file.

 So the risk becomes higher that another request could hit the gap betweens
 fopen and fwrite.

 But still no problems.

 **Until in WP 6.6 the {{{Rollback part 3}}} comes with three related
 changes:
 **
 1.) in the old function {{{wp_is_maintenance_mode()}}} a new, additional
 condition check was introduced.
 The current value of {{{$upgrading}}} is now crucial in this new check.

 A new function {{{has_fatal_error}}} was introduced with
 2.) require ABSPATH . '.maintenance', so the value of {{{$upgrading}}} as
 global was now updated at a second place - and during an update loop every
 time when a plugin was updated.

 3.) the value of {{{$upgrading}}} was used as part of the loopback request

 So - in the first round of an update the loopback request will bypass the
 maintenance screen in every case:
 - the value of {{{$upgrading}}} used in the loopback request is the same
 as expected in the new maintenance bypass condition.
 - Even if the edge case happens the 15 Year old bail out is ensuring the
 maintenance bypass - so the new condition is not checked in this edge
 case.

 But after the first round of an update under some racing conditions it
 could happen that the value of {{{$upgrading}}} used in the loopback
 request does not match the new maintenance bypass check.
 Due to {{{$upgrading}}} to be declared global it is possible, that after
 the first round {{{$upgrading}}} has already a value, even when
 {{{require}}} is loading a empty maintenance file.

 As a result at the long end the function {{{has_fatal_error}}} returns
 false ==> no errors found - without making a full assessment of the
 loopback request.

 In this PR the use of {{{$upgrading}}} in the new functions was
 removed/replaced.
 And the sleep(2) was also removed.

 @afragen, @costdev, @stoyangeorgiev
 Do you think I'm on the right path with this PR?


 Follow-up to [https://core.trac.wordpress.org/changeset/58128]
 See [https://core.trac.wordpress.org/ticket/58281]

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/62176>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform