Crons are not being executed.

[GlobalExperts]

It took me some time to debug this issue.

We are using your script to schedule raw commands using CronExpressions like ->at("0 3 * * *"). Some of our commands are with doNotOverlap() others are not. We have a total of 20 commands scheduled.

I have observed that when doNotOverlap() is set you, always set the job to run in foreground even if I haven't set the explicitly on the job.

The problem is that the CronExpression isDue() calculates the execution time always using strtotime("now"). Because the execution could take more than one minute and it runs in foreground, the next job picked from the list won't be executed because the isDue will return false, it's not on time anymore because isDue will use now() and not the time the job was scheduled.

Solutions:
1.) I don't understand why you force nonOverlapping jobs to run in foreground. You should remove that line because it doesn't make much sense to me.
2.) You should rewrite your isDue function to use the time when the job was scheduled and not current time which could be several minutes later because of the non-overlapping jobs.

  /**
   * Check if the job is due to run
   *
   * @return bool
   */
  public function isDue()
  {
    return $this->execution->isDue(date("c", $this->time)) && $this->truthTest === true;
  }

I think both 1.) and 2.) solutions should be applied on your script.

[peppeocchi]

Hi @GlobalExperts

I'm forcing the job to run in foreground because I am creating a lock file and deleting it after the scheduled script is being executed. If I run the job in background I won't have a way to remove the lock file and the next cron job will never run until you manually delete the lock file.

That was the solution I found when I developed that feature, but if you have a better idea we can discuss it.

About the second issue you're absolutely right and I'm happy to merge the changes.

Thanks!

[csaba-meszaros]

Hello @peppeocchi,

I have posted the issue by mistake from the company account.

It doesn't matter if you are running the script in foreground or background because in both cases you are removing the lock file with rm after execution. Because every job has a unique lock file it doesn't really make sense to make them wait after each other, the lock file will be removed in both cases.
Because I am not a unix guru I have made a test on our CentOS server to confirm the statement above:

[coderoot@server test]$ touch test.lock
[coderoot@server test]$ php -v;rm test.lock > /dev/null 2>&1
PHP 5.6.30 (cli) (built: Jan 19 2017 07:57:06)
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
[coderoot@server test]$ ls -l
total 0
[coderoot@server test]$ touch test.lock
[coderoot@server test]$ ls -l
total 0
-rw-rw-r-- 1 coderoot coderoot 0 Feb 17 22:00 test.lock
[coderoot@server test]$ php -v;rm test.lock > /dev/null 2>&1 &
PHP 5.6.30 (cli) (built: Jan 19 2017 07:57:06)
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
[1] 11725
[coderoot@server test]$ ls -l
total 0
[1]+  Done                    rm test.lock > /dev/null 2>&1
[coderoot@server test]$ ls -l
total 0
[coderoot@server test]$

The test above confirms that it's safe to modify your $job->run function to this:

public function run()
  {
    $output = [];

    // First reorder the cronjobs
    $this->jobsInBackgroundFirst();

    foreach ($this->jobs as $job) {
      $job->setup($this->config);

      // Check if job is due and if it should prevent overlapping
      if ($job->isDue()) {

        if ($job->preventOverlap() !== false) {

          // If overlapping, skip job execution
          if (! $this->jobCanRun($job)) {
            continue;
          }

          // Create lock file for this job to prevent overlap
          $lockFile = implode('/', [$this->getTempDir(), md5($job->getCommand()) . '.lock']);
          Filesystem::write('', $lockFile);
          // Sets the file to remove after the execution
          $job->removeLockAfterExec($lockFile);
        }

        $output[] = $job->exec();
        $this->executed[] = $job;
      }
    }

    return $output;
  }

[peppeocchi]

@csaba-meszaros you're right, there is no need to force the job to run in foreground.

I'll remove that line, thanks!