Wait for cron to finish before running upgrade command

[MorrisJobke]

• fixes Check before running "upgrade" if cronjob is still running and wait for it to finish #9562

To test this, add this:

diff --git a/lib/private/Authentication/Token/DefaultTokenProvider.php b/lib/private/Authentication/Token/DefaultTokenProvider.php
index 7a43dbb23e..fb2b33f564 100644
--- a/lib/private/Authentication/Token/DefaultTokenProvider.php
+++ b/lib/private/Authentication/Token/DefaultTokenProvider.php
@@ -281,9 +281,11 @@ class DefaultTokenProvider implements IProvider {
 	public function invalidateOldTokens() {
 		$olderThan = $this->time->getTime() - (int) $this->config->getSystemValue('session_lifetime', 60 * 60 * 24);
 		$this->logger->debug('Invalidating session tokens older than ' . date('c', $olderThan), ['app' => 'cron']);
+		sleep(15);
 		$this->mapper->invalidateOld($olderThan, IToken::DO_NOT_REMEMBER);
 		$rememberThreshold = $this->time->getTime() - (int) $this->config->getSystemValue('remember_login_cookie_lifetime', 60 * 60 * 24 * 15);
 		$this->logger->debug('Invalidating remembered session tokens older than ' . date('c', $rememberThreshold), ['app' => 'cron']);
+		sleep(15);
 		$this->mapper->invalidateOld($rememberThreshold, IToken::REMEMBER);
 	}

Then start a cron job cron.php, raise the version number in version.php or lower it in config/config.php and then trigger the upgrade by calling occ upgrade.

It then looks like this:

$ occ upgrade
Nextcloud or one of the apps require upgrade - only a limited number of commands are available
You may use your browser or the occ upgrade command to do the upgrade
Set log level to debug
Turned on maintenance mode
Waiting for cron to finish (checks again in 5 seconds)...
Waiting for cron to finish (checks again in 5 seconds)...
Waiting for cron to finish (checks again in 5 seconds)...
Waiting for cron to finish (checks again in 5 seconds)...
Updating database schema
Updated database
Checking for update of app activity in appstore
Checked for update of app "activity" in appstore 
Checking for update of app bruteforcesettings in appstore
Checked for update of app "bruteforcesettings" in appstore 
Checking for update of app comments in appstore
Checked for update of app "comments" in appstore 
...

When no cron job runs, then it looks like this:

$ occ upgrade
Nextcloud or one of the apps require upgrade - only a limited number of commands are available
You may use your browser or the occ upgrade command to do the upgrade
Set log level to debug
Turned on maintenance mode
Updating database schema
Updated database
Checking for update of app activity in appstore
Checked for update of app "activity" in appstore 
Checking for update of app bruteforcesettings in appstore
Checked for update of app "bruteforcesettings" in appstore 
Checking for update of app comments in appstore
Checked for update of app "comments" in appstore 
...

[MorrisJobke]

The web UI looks like this:

[MorrisJobke]

I'm tempted to backport this, because we think that this could fix the issue #4978

[codecov]

Codecov Report

Merging #9900 into master will decrease coverage by 0.16%.
The diff coverage is 3.33%.

@@             Coverage Diff             @@
##             master   #9900      +/-   ##
===========================================
- Coverage     52.07%   51.9%   -0.17%     
+ Complexity    25901   25813      -88     
===========================================
  Files          1642    1637       -5     
  Lines         95884   95539     -345     
  Branches       1318    1318              
===========================================
- Hits          49929   49593     -336     
+ Misses        45955   45946       -9

Impacted Files	Coverage Δ	Complexity Δ
lib/private/BackgroundJob/JobList.php	72.84% <0%> (-6.29%)	31 <2> (+2)
core/Command/Upgrade.php	0% <0%> (ø)	68 <1> (+1)	⬆️
core/ajax/update.php	0% <0%> (ø)	11 <0> (ø)	⬇️
lib/private/Updater.php	5.31% <11.11%> (+0.2%)	120 <2> (+2)	⬆️
lib/private/Authentication/Token/DefaultToken.php	85.24% <0%> (-6.69%)	20% <0%> (ø)
...B/QueryBuilder/FunctionBuilder/FunctionBuilder.php	77.77% <0%> (-2.23%)	9% <0%> (-1%)
lib/private/User/Database.php	79.47% <0%> (-1.78%)	43% <0%> (+1%)
apps/files_trashbin/lib/Expiration.php	90.32% <0%> (-1.62%)	29% <0%> (ø)
settings/Controller/AuthSettingsController.php	77.5% <0%> (-0.88%)	20% <0%> (+2%)
...vate/Authentication/Token/DefaultTokenProvider.php	98.03% <0%> (-0.02%)	32% <0%> (ø)
... and 9 more

[skjnldsv]

Makes a lot of sense!

[blizzz]

Let's document why this is supposed to be working. The code is clear, but the thought behinds should be expressed.

Strictly speaking, it does only report whether a job should run (it might be running, or it might be due with the next job run). Correct me if I am wrong. It could perhaps lead to an infinite loop when a job reincarnates and registers to run next time again. Though, cron should not be triggered when maintenance mode is enabled, which in the whole scenario is the case, so it would block for 12 hours instead.

Are the 12 hours based on something?

[MorrisJobke]

The 12 hours is the same timeout that is also used to re-schedule an non-terminated job (see a bit above in this file - getNextJob(). The idea here is: give a job enough time to run very long and then after a timeout re-schedule it nevertheless. It's more likely to be crashed than to run that long.

In theory it could lead to an endless loop (as in - at most 12 hours), because the cron will not start when maintenance mode is active and this method is only executed in maintenance mode (see where it is added in the upgrader class. I could add this as comments of course.

[MorrisJobke]

Comment added.

[blizzz]

From a UX perspective it still is problematic if it will take that long, as it'll look that nothing is happening. Of course stating "it might take up to 12 hours" in the GUI would scare people away. OK, let's get it in and collect some experience.

[MorrisJobke]

This will only trigger for 14.0.0.7+, because it would cause #9992 otherwise. See #10010

[MorrisJobke]

Let's not backport this.

[MorrisJobke]

Reverted in #12188