Merge pull request #12188 from nextcloud/revert/9900/revert-wait-for-cron

Revert "Wait for cron to finish before running upgrade command"

core/Command/Upgrade.php

@@ -99,8 +99,7 @@ class Upgrade extends Command {
 					$this->config,
 					\OC::$server->getIntegrityCodeChecker(),
 					$this->logger,
-					$this->installer,
-					\OC::$server->getJobList()
+					$this->installer
 			);
 
 			$dispatcher = \OC::$server->getEventDispatcher();
@@ -192,9 +191,6 @@ class Upgrade extends Command {
 			$updater->listen('\OC\Updater', 'maintenanceActive', function () use($output) {
 				$output->writeln('<info>Maintenance mode is kept active</info>');
 			});
-			$updater->listen('\OC\Updater', 'waitForCronToFinish', function () use($output) {
-				$output->writeln('<info>Waiting for cron to finish (checks again in 5 seconds) …</info>');
-			});
 			$updater->listen('\OC\Updater', 'updateEnd',
 				function ($success) use($output, $self) {
 					if ($success) {

core/ajax/update.php

@@ -119,8 +119,7 @@ if (\OCP\Util::needUpgrade()) {
 			$config,
 			\OC::$server->getIntegrityCodeChecker(),
 			$logger,
-			\OC::$server->query(\OC\Installer::class),
-			\OC::$server->getJobList()
+			\OC::$server->query(\OC\Installer::class)
 	);
 	$incompatibleApps = [];
 
@@ -153,9 +152,6 @@ if (\OCP\Util::needUpgrade()) {
 	$updater->listen('\OC\Updater', 'maintenanceActive', function () use ($eventSource, $l) {
 		$eventSource->send('success', (string)$l->t('Maintenance mode is kept active'));
 	});
-	$updater->listen('\OC\Updater', 'waitForCronToFinish', function () use ($eventSource, $l) {
-		$eventSource->send('success', (string)$l->t('Waiting for cron to finish (checks again in 5 seconds) …'));
-	});
 	$updater->listen('\OC\Updater', 'dbUpgradeBefore', function () use($eventSource, $l) {
 		$eventSource->send('success', (string)$l->t('Updating database schema'));
 	});

lib/private/BackgroundJob/JobList.php

@@ -48,9 +48,6 @@ class JobList implements IJobList {
 	/**@var ITimeFactory */
 	protected $timeFactory;
 
-	/** @var int - 12 hours * 3600 seconds*/
-	private $jobTimeOut = 43200;
-
 	/**
 	 * @param IDBConnection $connection
 	 * @param IConfig $config
@@ -186,7 +183,7 @@ class JobList implements IJobList {
 		$query = $this->connection->getQueryBuilder();
 		$query->select('*')
 			->from('jobs')
-			->where($query->expr()->lte('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - $this->jobTimeOut, IQueryBuilder::PARAM_INT)))
+			->where($query->expr()->lte('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - 12 * 3600, IQueryBuilder::PARAM_INT)))
 			->andWhere($query->expr()->lte('last_checked', $query->createNamedParameter($this->timeFactory->getTime(), IQueryBuilder::PARAM_INT)))
 			->orderBy('last_checked', 'ASC')
 			->setMaxResults(1);
@@ -346,39 +343,4 @@ class JobList implements IJobList {
 			->where($query->expr()->eq('id', $query->createNamedParameter($job->getId(), IQueryBuilder::PARAM_INT)));
 		$query->execute();
 	}
-
-	/**
-	 * checks if a job is still running (reserved_at time is smaller than 12 hours ago)
-	 *
-	 * Background information:
-	 *
-	 * The 12 hours is the same timeout that is also used to re-schedule an non-terminated
-	 * job (see getNext()). The idea here is to give a job enough time to run very
-	 * long but still be able to recognize that it maybe crashed and re-schedule it
-	 * after the timeout. It's more likely to be crashed at that time than it ran
-	 * that long.
-	 *
-	 * In theory it could lead to an nearly endless loop (as in - at most 12 hours).
-	 * The cron command will not start new jobs when maintenance mode is active and
-	 * this method is only executed in maintenance mode (see where it is called in
-	 * the upgrader class. So this means in the worst case we wait 12 hours when a
-	 * job has crashed. On the other hand: then the instance should be fixed anyways.
-	 *
-	 * @return bool
-	 */
-	public function isAnyJobRunning(): bool {
-		$query = $this->connection->getQueryBuilder();
-		$query->select('*')
-			->from('jobs')
-			->where($query->expr()->gt('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - $this->jobTimeOut, IQueryBuilder::PARAM_INT)))
-			->setMaxResults(1);
-		$result = $query->execute();
-		$row = $result->fetch();
-		$result->closeCursor();
-
-		if ($row) {
-			return true;
-		}
-		return false;
-	}
 }

lib/private/Updater.php

@@ -38,7 +38,6 @@ use OC\DB\MigrationService;
 use OC\Hooks\BasicEmitter;
 use OC\IntegrityCheck\Checker;
 use OC_App;
-use OCP\BackgroundJob\IJobList;
 use OCP\IConfig;
 use OCP\ILogger;
 use OCP\Util;
@@ -67,9 +66,6 @@ class Updater extends BasicEmitter {
 	/** @var Installer */
 	private $installer;
 
-	/** @var IJobList */
-	private $jobList;
-
 	private $logLevelNames = [
 		0 => 'Debug',
 		1 => 'Info',
@@ -78,16 +74,20 @@ class Updater extends BasicEmitter {
 		4 => 'Fatal',
 	];
 
+	/**
+	 * @param IConfig $config
+	 * @param Checker $checker
+	 * @param ILogger $log
+	 * @param Installer $installer
+	 */
 	public function __construct(IConfig $config,
 								Checker $checker,
-								ILogger $log,
-								Installer $installer,
-								IJobList $jobList) {
+								ILogger $log = null,
+								Installer $installer) {
 		$this->log = $log;
 		$this->config = $config;
 		$this->checker = $checker;
 		$this->installer = $installer;
-		$this->jobList = $jobList;
 	}
 
 	/**
@@ -114,11 +114,6 @@ class Updater extends BasicEmitter {
 		$installedVersion = $this->config->getSystemValue('version', '0.0.0');
 		$currentVersion = implode('.', \OCP\Util::getVersion());
 
-		// see https://github.com/nextcloud/server/issues/9992 for potential problem
-		if (version_compare($installedVersion, '14.0.0.9', '>=')) {
-			$this->waitForCronToFinish();
-		}
-
 		$this->log->debug('starting upgrade from ' . $installedVersion . ' to ' . $currentVersion, array('app' => 'core'));
 
 		$success = true;
@@ -617,12 +612,6 @@ class Updater extends BasicEmitter {
 		});
 
 	}
-	private function waitForCronToFinish() {
-		while ($this->jobList->isAnyJobRunning()) {
-			$this->emit('\OC\Updater', 'waitForCronToFinish');
-			sleep(5);
-		}
-	}
 
 }
 

tests/lib/UpdaterTest.php

@@ -24,7 +24,6 @@ namespace Test;
 
 use OC\Installer;
 use OC\Updater;
-use OCP\BackgroundJob\IJobList;
 use OCP\IConfig;
 use OCP\ILogger;
 use OC\IntegrityCheck\Checker;
@@ -40,8 +39,6 @@ class UpdaterTest extends TestCase {
 	private $checker;
 	/** @var Installer|\PHPUnit_Framework_MockObject_MockObject */
 	private $installer;
-	/** @var IJobList|\PHPUnit_Framework_MockObject_MockObject */
-	private $jobList;
 
 	public function setUp() {
 		parent::setUp();
@@ -57,16 +54,12 @@ class UpdaterTest extends TestCase {
 		$this->installer = $this->getMockBuilder(Installer::class)
 			->disableOriginalConstructor()
 			->getMock();
-		$this->jobList = $this->getMockBuilder(IJobList::class)
-			->disableOriginalConstructor()
-			->getMock();
 
 		$this->updater = new Updater(
 			$this->config,
 			$this->checker,
 			$this->logger,
-			$this->installer,
-			$this->jobList
+			$this->installer
 		);
 	}