Problem/Motivation
Currently if Composer Stager has runtime error during commit(our apply) we would inform the user because we display exceptions through the batch form process and log errors in cron. But things become unpredictable even after a successful update, see #3291147: Successfully applying an update puts the remainder of the request into an unreliable state, so we can assume unsuccessful update would be even more unpredictable. We couldn't be sure how much of the update actually applied before the error.
If there is error in the committer the site may say it has been updated because Drupal.php which has the version number may have been updated but other files may not have been copied over, say a php file that actually has a security fix.
Further more besides the committer throwing an exception there may be a hard failure in the committer that doesn't return or more likely the logging system might not work in the same request after a partial update(even a full update) so there is no guarantee any exception would be logged.
Steps to reproduce
- Follow #3275810: 8.x-2.x Beta Testing Instructions
- But right before you apply the update via the form make
core/lib/README.txtunwritable - Then try to apply the update
You will get an error but the site will say has been updated to 9.3.16. There will be no indication afters that update did not apply correctly. Even though the error was because of README.txt there is no way of knowing which files that would of been updated/deleted after that file were not updated/deleted
Proposed resolution
Probably multiple step.
- in \Drupal\automatic_updates\BatchProcessor::commit and \Drupal\automatic_updates\CronUpdater::performUpdate check to see if the exception means the update could have been partially apply and log a very strongly worked message the site probably should restored from a back up.
\PhpTuf\ComposerStager\Domain\Core\Committer\CommitterInterface::commitsays* * @throws \PhpTuf\ComposerStager\Domain\Exception\InvalidArgumentException * If $exclusions includes invalid paths. * @throws \PhpTuf\ComposerStager\Domain\Exception\PreconditionException * If the preconditions are unfulfilled. * @throws \PhpTuf\ComposerStager\Domain\Exception\RuntimeException * If the operation fails.If the exception is
InvalidArgumentExceptionorPreconditionExceptionwe can probably safely assume the commit did not even start. With any other exception it is probably safest to assume that the site may be in a half updated state and should restored from back up. - follow up in #3293417: If an update failed to apply don't allow more use of the module
The above solution requires an exception to actually be caught. in the rare case where there is fatal which happens in commit the exception does not come back should also assume the site is half updated. Right now the site would just say there is an existing update that you could delete. it may or may not show the site has been updated depending on which files were copied.
To handle this problem we should write a
PACKAGE_MANAGER_UPDATE_FAILURE.txtto the active directory and delete this file as soon as commit comes back from Composer stager. We should always check for existence of this fail and tell the site admin to restore from backup.In follow up issue we could do the full version of #3278452: Create a manual fallback recovery system if update fails during the Apply phase
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork automatic_updates-3291770
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
tedbowComment #5
tedbowComment #6
tedbowComment #7
tedbowwe can step 2) in another issue #3291770: Inform the site admin if Composer Stager's committer failed, possibly leaving the site in a half-updated state
Comment #8
tedbowComment #9
tedbowComment #10
phenaproximaFew minor points, but otherwise I don't see a problem here.
Comment #11
tedbowComment #12
phenaproximaNo objections here.
Comment #13
phenaproximaComment #14
phenaproximaI think this is good to go when tests pass.
Comment #15
phenaproximaRe-titling for clarity.
Comment #17
tedbowComment #19
effulgentsia commentedQuestion about this: this issue was about the committer, but was a similar solution applied for the beginner? For example, if the PHP script is terminated halfway through copying from active to stage, how do we know that that happened and recover from it?
Also, do we have test coverage for these scenarios (PHP script termination during the copy from active to stage or from stage to active), and if so, where are those tests?