MAGETWO-84646: Cron jobs incorrect behavior when running job terminated

Author: Oleksandr Iegorov

app/code/Magento/Cron/Model/ResourceModel/Schedule.php

@@ -66,7 +66,14 @@ public function trySetJobUniqueStatusAtomic($scheduleId, $newStatus, $currentSta
     {
         $connection = $this->getConnection();
 
-        $match = $connection->quoteInto('existing.job_code = current.job_code AND existing.status = ?', $newStatus);
+        // this condition added to avoid cron jobs locking after incorrect termination of running job
+        $match = $connection->quoteInto(
+            'existing.job_code = current.job_code ' .
+            'AND (existing.executed_at > UTC_TIMESTAMP() - INTERVAL 1 DAY OR existing.executed_at IS NULL) ' .
+            'AND existing.status = ?',
+            $newStatus
+        );
+
         $selectIfUnlocked = $connection->select()
             ->joinLeft(
                 ['existing' => $this->getTable('cron_schedule')],
@@ -75,8 +82,7 @@ public function trySetJobUniqueStatusAtomic($scheduleId, $newStatus, $currentSta
             )
             ->where('current.schedule_id = ?', $scheduleId)
             ->where('current.status = ?', $currentStatus)
-            ->where('existing.schedule_id IS NULL')
-            ->where('existing.executed_at IS NULL');
+            ->where('existing.schedule_id IS NULL');
 
         $update = $connection->updateFromSelect($selectIfUnlocked, ['current' => $this->getTable('cron_schedule')]);
         $result = $connection->query($update)->rowCount();

dev/tests/integration/testsuite/Magento/Cron/Model/ScheduleTest.php

@@ -54,6 +54,27 @@ public function testTryLockJobAlreadyLockedFails()
         $this->assertFalse($schedule->tryLockJob());
     }
 
+    /**
+     * If the job is already locked but lock time less than 1 day ago, attempting to lock it again should fail
+     */
+    public function testTryLockJobAlreadyLockedSucceeds()
+    {
+        $offsetInThePast = 2*24*60*60;
+
+        $oldSchedule = $this->scheduleFactory->create()
+            ->setCronExpr("* * * * *")
+            ->setJobCode("test_job")
+            ->setStatus(Schedule::STATUS_RUNNING)
+            ->setCreatedAt(strftime('%Y-%m-%d %H:%M:%S', $this->dateTime->gmtTimestamp() - $offsetInThePast))
+            ->setScheduledAt(strftime('%Y-%m-%d %H:%M', $this->dateTime->gmtTimestamp() - $offsetInThePast + 60))
+            ->setExecutedAt(strftime('%Y-%m-%d %H:%M', $this->dateTime->gmtTimestamp() - $offsetInThePast + 61));
+        $oldSchedule->save();
+
+        $schedule = $this->createSchedule("test_job", Schedule::STATUS_PENDING);
+
+        $this->assertTrue($schedule->tryLockJob());
+    }
+
     /**
      * If there's a job already locked, should not be able to lock another job
      */
@@ -82,9 +103,10 @@ public function testTryLockJobDifferentJobLocked()
      * @param string $jobCode
      * @param string $status
      * @param int $timeOffset
+     * @param int $executionTimeOffset
      * @return Schedule
      */
-    private function createSchedule($jobCode, $status, $timeOffset = 0)
+    private function createSchedule($jobCode, $status, $timeOffset = 0, $executionTimeOffset = 0)
     {
         $schedule = $this->scheduleFactory->create()
             ->setCronExpr("* * * * *")