Reset previously failed cronjobs before executing them (#2726)

* Reset previously failed cronjobs before executing them

* Missing method call to reset the failed cronjobs

diff --git a/wcfsetup/install/files/lib/system/cronjob/CronjobScheduler.class.php b/wcfsetup/install/files/lib/system/cronjob/CronjobScheduler.class.php
index a29eaffb28eb0d5aa4c00ba391a19d83f7dbb921..f6c3ff0db6b6223251df4ab3ab9f62c9a3a8e75d 100644 (file)
--- 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;