[wp-trac] [WordPress Trac] #53705: Plugin upgrade deletes files from other in-progress upgrades

Thu Mar 31 22:10:49 UTC 2022

#53705: Plugin upgrade deletes files from other in-progress upgrades
-----------------------------+---------------------
 Reporter:  bpayton          |       Owner:  (none)
     Type:  defect (bug)     |      Status:  new
 Priority:  normal           |   Milestone:  6.0
Component:  Upgrade/Install  |     Version:  5.8
 Severity:  normal           |  Resolution:
 Keywords:                   |     Focuses:
-----------------------------+---------------------

Comment (by bpayton):

 Replying to [comment:16 peterwilsoncc]:
 > Replying to [comment:6 bpayton]:
 > I've been thinking about this and don't think it's a wise idea.
 >
 > In terms of meeting the website owners expectations: this approach would
 leave unexpected files on the server in an unexpected location.
 >

 @peterwilsoncc, thank you for taking time to think about this.

 I agree that leaving unexpected files on a server is undesirable, but I
 don't believe this code bears primary responsibility for that cleanup.
 Here's my thinking:

 In general, WordPress attempts to cleanup working files after an upgrade
 is done or an error occurs.

 Core updates attempt to clean up working files. (refs:
 [https://github.com/WordPress/wordpress-
 develop/blob/16baa3877991d521cb69df712f1e1be9f61791bb/src/wp-
 admin/includes/class-core-upgrader.php#L157-L158 1],
 [https://github.com/WordPress/wordpress-
 develop/blob/16baa3877991d521cb69df712f1e1be9f61791bb/src/wp-
 admin/includes/update-core.php#L978-L979 2], [https://github.com/WordPress
 /wordpress-develop/blob/16baa3877991d521cb69df712f1e1be9f61791bb/src/wp-
 admin/includes/update-core.php#L992-L993 3], [https://github.com/WordPress
 /wordpress-develop/blob/16baa3877991d521cb69df712f1e1be9f61791bb/src/wp-
 admin/includes/update-core.php#L1027-L1028 4],
 [https://github.com/WordPress/wordpress-
 develop/blob/16baa3877991d521cb69df712f1e1be9f61791bb/src/wp-
 admin/includes/update-core.php#L1201-L1202 5],
 [https://github.com/WordPress/wordpress-
 develop/blob/16baa3877991d521cb69df712f1e1be9f61791bb/src/wp-
 admin/includes/update-core.php#L1392-L1393 6],
 [https://github.com/WordPress/wordpress-
 develop/blob/16baa3877991d521cb69df712f1e1be9f61791bb/src/wp-
 admin/includes/update-core.php#L1433-L1434 7])

 The WP_Upgrader class knows how to clean up after itself after installing
 a package. The WP_Upgrader::install_package() method takes a
 `clear_working` argument, and if that argument is truthy, the working
 files are [https://github.com/WordPress/wordpress-
 develop/blob/16baa3877991d521cb69df712f1e1be9f61791bb/src/wp-
 admin/includes/class-wp-upgrader.php#L604-L607 cleaned up after
 installation]. And it looks like `clear_working` is generally set to true:

 - WP_Upgrader::run() is the only method in core that calls
 WP_Upgrader::install_package(), and WP_Upgrader also takes a
 `clear_working` argument that defaults to true.
 - Plugin_Upgrader calls WP_Upgrader::run() three times and each call
 passes `"clear_working" => true`. (refs: [https://github.com/WordPress
 /wordpress-develop/blob/143df6e76363d59f9bbad1d33747a0ff8d05299e/src/wp-
 admin/includes/class-plugin-upgrader.php#L135 1],
 [https://github.com/WordPress/wordpress-
 develop/blob/143df6e76363d59f9bbad1d33747a0ff8d05299e/src/wp-
 admin/includes/class-plugin-upgrader.php#L222 2],
 [https://github.com/WordPress/wordpress-
 develop/blob/143df6e76363d59f9bbad1d33747a0ff8d05299e/src/wp-
 admin/includes/class-plugin-upgrader.php#L337 3])
 - Theme_Upgrader calls WP_Upgrader::run() four times and each call passes
 `"clear_working" => true`. (refs: [https://github.com/WordPress/wordpress-
 develop/blob/143df6e76363d59f9bbad1d33747a0ff8d05299e/src/wp-
 admin/includes/class-theme-upgrader.php#L173 1],
 [https://github.com/WordPress/wordpress-
 develop/blob/143df6e76363d59f9bbad1d33747a0ff8d05299e/src/wp-
 admin/includes/class-theme-upgrader.php#L248 2],
 [https://github.com/WordPress/wordpress-
 develop/blob/143df6e76363d59f9bbad1d33747a0ff8d05299e/src/wp-
 admin/includes/class-theme-upgrader.php#L324 3],
 [https://github.com/WordPress/wordpress-
 develop/blob/143df6e76363d59f9bbad1d33747a0ff8d05299e/src/wp-
 admin/includes/class-theme-upgrader.php#L324 4])
 - Language_Pack_Upgrader calls WP_Upgrader::run() once and passes
 `"clear_working" => true`. (ref: [https://github.com/WordPress/wordpress-
 develop/blob/143df6e76363d59f9bbad1d33747a0ff8d05299e/src/wp-
 admin/includes/class-language-pack-upgrader.php#L243-L256 1])

 I believe this shows that [https://github.com/WordPress/wordpress-
 develop/blob/trunk/src/wp-admin/includes/class-wp-upgrader.php#L312-L318
 the cleanup code in question here] is a second line of defense, where
 WordPress takes the opportunity to clean up working files that should have
 been cleaned up earlier.

 If that is true, I think it is probably reasonable for this second line of
 defense to only clean up working files that are past a certain age. It
 honestly seems like the safer thing to do since __this code naturally does
 not know why the working files are there to begin with, and that lack of
 knowledge is the source of this bug__.

 Does this make sense, and if so, does it influence your thinking at all on
 [https://core.trac.wordpress.org/attachment/ticket/53705/avoid-corrupting-
 concurrent-plugin-upgrades.diff the only-delete-older-working-files
 patch]?

 In case it helps, I think the patch could be adjusted to clean up files
 that are older than an hour or even a number of minutes. In retrospect,
 requiring files to be over a day old seems excessive.

 >
 > If the file names are predictable then a more targeted delete while
 preparing for the upgrade could be a better solution.

 Since this cleanup code assumes all pre-existing working files are left
 over and need to be cleaned up, I'm not sure how better naming would
 prevent it from removing things that are in the process of being
 installed. Would you be up for explaining further?

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/53705#comment:17>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform