From e620364aeae356abf0d762f8d4d0be97e56f3294 Mon Sep 17 00:00:00 2001
From: Alexander Ebert <ebert@woltlab.com>
Date: Mon, 30 Jul 2018 15:34:36 +0200
Subject: [PATCH] Reset previously failed cronjobs before executing them
 (#2726)

* Reset previously failed cronjobs before executing them

* Missing method call to reset the failed cronjobs
---
 .../system/cronjob/CronjobScheduler.class.php | 100 ++++++++++++------
 1 file changed, 65 insertions(+), 35 deletions(-)

diff --git a/wcfsetup/install/files/lib/system/cronjob/CronjobScheduler.class.php b/wcfsetup/install/files/lib/system/cronjob/CronjobScheduler.class.php
index a29eaffb28..f6c3ff0db6 100644
--- a/wcfsetup/install/files/lib/system/cronjob/CronjobScheduler.class.php
+++ b/wcfsetup/install/files/lib/system/cronjob/CronjobScheduler.class.php
@@ -46,6 +46,8 @@ class CronjobScheduler extends SingletonFactory {
 			return;
 		}
 		
+		$this->resetFailedCronjobs();
+		
 		// get outstanding cronjobs
 		$this->loadCronjobs();
 		
@@ -105,6 +107,56 @@ class CronjobScheduler extends SingletonFactory {
 		return $this->cache['nextExec'];
 	}
 	
+	/**
+	 * Resets any cronjobs that have previously failed to execute. Cronjobs that have failed too often will
+	 * be disabled automatically.
+	 */
+	protected function resetFailedCronjobs() {
+		WCF::getDB()->beginTransaction();
+		/** @noinspection PhpUnusedLocalVariableInspection */
+		$committed = false;
+		try {
+			$sql = "SELECT          *
+				FROM            wcf" . WCF_N . "_cronjob
+				WHERE           isDisabled = ?
+						AND failCount < ?
+						AND afterNextExec <= ?
+				FOR UPDATE";
+			$statement = WCF::getDB()->prepareStatement($sql);
+			$statement->execute([
+				0,
+				Cronjob::MAX_FAIL_COUNT,
+				TIME_NOW
+			]);
+			/** @var Cronjob $cronjob */
+			while ($cronjob = $statement->fetchObject(Cronjob::class)) {
+				$failCount = $cronjob->failCount + 1;
+				$data['failCount'] = $failCount;
+				
+				if ($failCount == Cronjob::MAX_FAIL_COUNT) {
+					$data['isDisabled'] = 1;
+					$data['state'] = Cronjob::READY;
+				}
+				
+				// Schedule the cronjob for execution at the next regular execution date. The previous
+				// implementation was executing the cronjob immediately, which may be undesirable if
+				// the cronjob is expected to be executed in a specific time window only. 
+				$data['nextExec'] = $cronjob->getNextExec(TIME_NOW);
+				$data['afterNextExec'] = $cronjob->getNextExec($data['nextExec']);
+				
+				(new CronjobEditor($cronjob))->update($data);
+			}
+			
+			WCF::getDB()->commitTransaction();
+			$committed = true;
+		}
+		finally {
+			if (!$committed) {
+				WCF::getDB()->rollBackTransaction();
+			}
+		}
+	}
+	
 	/**
 	 * Loads outstanding cronjobs.
 	 */
@@ -114,46 +166,24 @@ class CronjobScheduler extends SingletonFactory {
 		$committed = false;
 		try {
 			$sql = "SELECT	        *
-				FROM	        wcf" . WCF_N . "_cronjob cronjob
-				WHERE	        (cronjob.nextExec <= ? OR cronjob.afterNextExec <= ?)
-						AND cronjob.isDisabled = ?
-						AND cronjob.failCount < ?
+				FROM	        wcf" . WCF_N . "_cronjob
+				WHERE	        isDisabled = ?
+						AND state = ?
+						AND nextExec <= ?
 				FOR UPDATE";
 			$statement = WCF::getDB()->prepareStatement($sql);
-			$statement->execute([TIME_NOW, TIME_NOW, 0, Cronjob::MAX_FAIL_COUNT]);
-			while ($row = $statement->fetchArray()) {
-				$cronjob = new Cronjob(null, $row);
+			$statement->execute([
+				0,
+				Cronjob::READY,
+				TIME_NOW
+			]);
+			while ($cronjob = $statement->fetchObject(Cronjob::class)) {
 				$cronjobEditor = new CronjobEditor($cronjob);
-				$executeCronjob = true;
 				
-				$data = ['state' => Cronjob::PENDING];
+				// Mark the cronjob as pending to prevent concurrent requests from executing it.
+				$cronjobEditor->update(['state' => Cronjob::PENDING]);
 				
-				// reset cronjob if it got stuck before and afterNextExec is in the past
-				if ($cronjobEditor->afterNextExec <= TIME_NOW) {
-					if ($cronjobEditor->state == Cronjob::EXECUTING) {
-						$failCount = $cronjobEditor->failCount + 1;
-						$data['failCount'] = $failCount;
-						
-						// disable cronjob
-						if ($failCount == Cronjob::MAX_FAIL_COUNT) {
-							$data['isDisabled'] = 1;
-							$data['state'] = 0;
-							$executeCronjob = false;
-						}
-					}
-				} // ignore cronjobs which seem to be running
-				else {
-					if ($cronjobEditor->nextExec <= TIME_NOW && $cronjobEditor->state != Cronjob::READY) {
-						$executeCronjob = false;
-					}
-				}
-				
-				// mark cronjob as pending, preventing parallel execution
-				$cronjobEditor->update($data);
-				
-				if ($executeCronjob) {
-					$this->cronjobEditors[] = $cronjobEditor;
-				}
+				$this->cronjobEditors[] = $cronjobEditor;
 			}
 			WCF::getDB()->commitTransaction();
 			$committed = true;
-- 
2.20.1