Merge pull request #19329 from nextcloud/feature/noid/warn-admins-about-delayed-cron

Warn admins about delayed cron executions

3 files changed, 89 insertions, 25 deletions

diff --git a/apps/settings/lib/Settings/Admin/Server.php b/apps/settings/lib/Settings/Admin/Server.php
index 6216af4aada..d82c9e7c256 100644
--- a/apps/settings/lib/Settings/Admin/Server.php
+++ b/apps/settings/lib/Settings/Admin/Server.php
@@ -28,17 +28,25 @@
 namespace OCA\Settings\Settings\Admin;
 
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IConfig;
+use OCP\IDBConnection;
 use OCP\Settings\ISettings;
 
 class Server implements ISettings {
+
+	/** @var IDBConnection */
+	private $connection;
+	/** @var ITimeFactory */
+	private $timeFactory;
 	/** @var IConfig */
 	private $config;
 
-	/**
-	 * @param IConfig $config
-	 */
-	public function __construct(IConfig $config) {
+	public function __construct(IDBConnection $connection,
+								ITimeFactory $timeFactory,
+								IConfig $config) {
+		$this->connection = $connection;
+		$this->timeFactory = $timeFactory;
 		$this->config = $config;
 	}
 
@@ -50,7 +58,8 @@ class Server implements ISettings {
 			// Background jobs
 			'backgroundjobs_mode' => $this->config->getAppValue('core', 'backgroundjobs_mode', 'ajax'),
 			'lastcron'            => $this->config->getAppValue('core', 'lastcron', false),
-			'cronErrors'		  => $this->config->getAppValue('core', 'cronErrors'),
+			'cronMaxAge'          => $this->cronMaxAge(),
+			'cronErrors'          => $this->config->getAppValue('core', 'cronErrors'),
 			'cli_based_cron_possible' => function_exists('posix_getpwuid'),
 			'cli_based_cron_user' => function_exists('posix_getpwuid') ? posix_getpwuid(fileowner(\OC::$configDir . 'config.php'))['name'] : '',
 		];
@@ -58,6 +67,24 @@ class Server implements ISettings {
 		return new TemplateResponse('settings', 'settings/admin/server', $parameters, '');
 	}
 
+	protected function cronMaxAge(): int {
+		$query = $this->connection->getQueryBuilder();
+		$query->select('last_checked')
+			->from('jobs')
+			->orderBy('last_checked', 'ASC')
+			->setMaxResults(1);
+
+		$result = $query->execute();
+		if ($row = $result->fetch()) {
+			$maxAge = (int) $row['last_checked'];
+		} else {
+			$maxAge = $this->timeFactory->getTime();
+		}
+		$result->closeCursor();
+
+		return $maxAge;
+	}
+
 	/**
 	 * @return string the section ID, e.g. 'sharing'
 	 */
diff --git a/apps/settings/templates/settings/admin/server.php b/apps/settings/templates/settings/admin/server.php
index 92bf433ca6c..513e82ece5a 100644
--- a/apps/settings/templates/settings/admin/server.php
+++ b/apps/settings/templates/settings/admin/server.php
@@ -29,26 +29,40 @@
 <div class="section" id="backgroundjobs">
 	<h2 class="inlineblock"><?php p($l->t('Background jobs'));?></h2>
 	<p class="cronlog inlineblock">
-		<?php if ($_['lastcron'] !== false):
+		<?php if ($_['lastcron'] !== false) {
 			$relative_time = relative_modified_date($_['lastcron']);
+			$maxAgeRelativeTime = relative_modified_date($_['cronMaxAge']);
 
 			$formatter = \OC::$server->getDateTimeFormatter();
 			$absolute_time = $formatter->formatDateTime($_['lastcron'], 'long', 'long');
-			if (time() - $_['lastcron'] <= 600): ?>
-				<span class="status success"></span>
-				<span class="crondate" title="<?php p($absolute_time);?>">
-				<?php p($l->t("Last job ran %s.", [$relative_time]));?>
-			</span>
-			<?php else: ?>
+			$maxAgeAbsoluteTime = $formatter->formatDateTime($_['cronMaxAge'], 'long', 'long');
+			if (time() - $_['lastcron'] > 600) { ?>
 				<span class="status error"></span>
 				<span class="crondate" title="<?php p($absolute_time);?>">
-				<?php p($l->t("Last job execution ran %s. Something seems wrong.", [$relative_time]));?>
-			</span>
-			<?php endif;
-		else: ?>
+					<?php p($l->t("Last job execution ran %s. Something seems wrong.", [$relative_time]));?>
+				</span>
+			<?php } else if (time() - $_['cronMaxAge'] > 12*3600) {
+					if ($_['backgroundjobs_mode'] === 'cron') { ?>
+						<span class="status warning"></span>
+						<span class="crondate" title="<?php p($maxAgeAbsoluteTime);?>">
+							<?php p($l->t("Some jobs haven’t been executed since %s. Please consider increasing the execution frequency.", [$maxAgeRelativeTime]));?>
+						</span>
+					<?php } else { ?>
+						<span class="status error"></span>
+						<span class="crondate" title="<?php p($maxAgeAbsoluteTime);?>">
+							<?php p($l->t("Some jobs didn’t execute since %s. Please consider switching to system cron.", [$maxAgeRelativeTime]));?>
+						</span>
+					<?php }
+			} else { ?>
+				<span class="status success"></span>
+				<span class="crondate" title="<?php p($absolute_time);?>">
+					<?php p($l->t("Last job ran %s.", [$relative_time]));?>
+				</span>
+			<?php }
+		} else { ?>
 			<span class="status error"></span>
 			<?php p($l->t("Background job didn’t run yet!"));
-		endif; ?>
+		} ?>
 	</p>
 	<a target="_blank" rel="noreferrer noopener" class="icon-info"
 	   title="<?php p($l->t('Open documentation'));?>"
diff --git a/apps/settings/tests/Settings/Admin/ServerTest.php b/apps/settings/tests/Settings/Admin/ServerTest.php
index aeb37f8d6cc..9657ec2d2d4 100644
--- a/apps/settings/tests/Settings/Admin/ServerTest.php
+++ b/apps/settings/tests/Settings/Admin/ServerTest.php
@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016 Lukas Reschke <lukas@statuscode.ch>
  *
@@ -31,25 +32,46 @@ namespace OCA\Settings\Tests\Settings\Admin;
 
 use OCA\Settings\Settings\Admin\Server;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IConfig;
+use OCP\IDBConnection;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
+
+/**
+ * @group DB
+ */
 class ServerTest extends TestCase {
 	/** @var Server */
 	private $admin;
-	/** @var IConfig */
+	/** @var IDBConnection */
+	private $connection;
+	/** @var ITimeFactory|MockObject */
+	private $timeFactory;
+	/** @var IConfig|MockObject */
 	private $config;
 
 	protected function setUp(): void {
 		parent::setUp();
+		$this->connection = \OC::$server->getDatabaseConnection();
+		$this->timeFactory = $this->createMock(ITimeFactory::class);
 		$this->config = $this->createMock(IConfig::class);
 
-		$this->admin = new Server(
-			$this->config
-		);
+		$this->admin = $this->getMockBuilder(Server::class)
+			->onlyMethods(['cronMaxAge'])
+			->setConstructorArgs([
+				$this->connection,
+				$this->timeFactory,
+				$this->config,
+			])
+			->getMock();
 	}
 
-	public function testGetForm() {
+	public function testGetForm(): void {
+		$this->admin->expects($this->once())
+			->method('cronMaxAge')
+			->willReturn(1337);
 		$this->config
 			->expects($this->at(0))
 			->method('getAppValue')
@@ -71,7 +93,8 @@ class ServerTest extends TestCase {
 			[
 				'backgroundjobs_mode' => 'ajax',
 				'lastcron'            => false,
-				'cronErrors'		  => '',
+				'cronErrors'          => '',
+				'cronMaxAge'          => 1337,
 				'cli_based_cron_possible' => true,
 				'cli_based_cron_user' => function_exists('posix_getpwuid') ? posix_getpwuid(fileowner(\OC::$configDir . 'config.php'))['name'] : '', // to not explode here because of posix extension not being disabled - which is already checked in the line above
 			],
@@ -81,11 +104,11 @@ class ServerTest extends TestCase {
 		$this->assertEquals($expected, $this->admin->getForm());
 	}
 
-	public function testGetSection() {
+	public function testGetSection(): void {
 		$this->assertSame('server', $this->admin->getSection());
 	}
 
-	public function testGetPriority() {
+	public function testGetPriority(): void {
 		$this->assertSame(0, $this->admin->getPriority());
 	}
 }