Skip to content

Commit

Permalink
MDL-79966 core_task: Scheduled task timing (crontab fields) is wrong
Browse files Browse the repository at this point in the history
  • Loading branch information
@sammarshallou
sammarshallou committed Nov 20, 2023
1 parent b58d1fd commit 1c4d433
Show file tree
Hide file tree
Showing 3 changed files with 278 additions and 42 deletions.
6 changes: 5 additions & 1 deletion admin/tool/task/tests/form_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ public function test_validate_fields_dayofweek() {
$checker->set_day_of_week('6');
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('7');
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('8');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('20');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
Expand All @@ -247,7 +249,7 @@ public function test_validate_fields_dayofweek() {
$checker->set_day_of_week('*/6');
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('*/7');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('*/13');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('*/35');
Expand All @@ -271,6 +273,8 @@ public function test_validate_fields_dayofweek() {
$checker->set_day_of_week('65-2');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('3-7');
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('3-8');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
}
}
167 changes: 126 additions & 41 deletions lib/classes/task/scheduled_task.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ abstract class scheduled_task extends task_base {
const DAYOFWEEKMIN = 0;
/** Maximum dayofweek value. */
const DAYOFWEEKMAX = 6;
/** Maximum dayofweek value allowed in input (7 = 0). */
const DAYOFWEEKMAXINPUT = 7;

/**
* Minute field identifier.
Expand All @@ -77,6 +79,11 @@ abstract class scheduled_task extends task_base {
*/
const FIELD_DAYOFWEEK = 'dayofweek';

/**
* Time used for the next scheduled time when a task should never run. This is 3000-01-01 00:00 GMT.
*/
const NEVER_RUN_TIME = 32503680000;

/** @var string $hour - Pattern to work out the valid hours */
private $hour = '*';

Expand Down Expand Up @@ -380,13 +387,25 @@ private function get_valid(string $field): array {
break;
case self::FIELD_DAYOFWEEK:
$min = self::DAYOFWEEKMIN;
$max = self::DAYOFWEEKMAX;
$max = self::DAYOFWEEKMAXINPUT;
break;
default:
throw new \coding_exception("Field '$field' is not a valid crontab identifier.");
}

return $this->eval_cron_field($this->{$field}, $min, $max);
$result = $this->eval_cron_field($this->{$field}, $min, $max);
if ($field === self::FIELD_DAYOFWEEK) {
// For day of week, 0 and 7 both mean Sunday; if there is a 7 we set 0. The result array is sorted.
if (end($result) === 7) {
// Remove last element.
array_pop($result);
// Insert 0 as first element if it's not there already.
if (reset($result) !== 0) {
array_unshift($result, 0);
}
}
}
return $result;
}

/**
Expand Down Expand Up @@ -506,9 +525,14 @@ private function next_in_list($current, $list) {
/**
* Calculate when this task should next be run based on the schedule.
*
* @param int $now Current time, for testing (leave 0 to use default time)
* @return int $nextruntime.
*/
public function get_next_scheduled_time() {
public function get_next_scheduled_time(int $now = 0): int {
if (!$now) {
$now = time();
}

// We need to change to the server timezone before using php date() functions.
\core_date::set_default_server_timezone();

Expand All @@ -518,27 +542,64 @@ public function get_next_scheduled_time() {
$validdaysofweek = $this->get_valid(self::FIELD_DAYOFWEEK);
$validmonths = $this->get_valid(self::FIELD_MONTH);

$nextvalidyear = date('Y');
// If any of the fields contain no valid data then the task will never run.
if (!$validminutes || !$validhours || !$validdays || !$validdaysofweek || !$validmonths) {
return self::NEVER_RUN_TIME;
}

$currentminute = date("i") + 1;
$currenthour = date("H");
$currentday = date("j");
$currentmonth = date("n");
$currentdayofweek = date("w");
$result = self::get_next_scheduled_time_inner($now, $validminutes, $validhours, $validdays, $validdaysofweek, $validmonths);
return $result;
}

$nextvalidminute = $this->next_in_list($currentminute, $validminutes);
if ($nextvalidminute < $currentminute) {
$currenthour += 1;
/**
* Recursively calculate the next valid time for this task.
*
* @param int $now Start time
* @param array $validminutes Valid minutes
* @param array $validhours Valid hours
* @param array $validdays Valid days
* @param array $validdaysofweek Valid days of week
* @param array $validmonths Valid months
* @param int $originalyear Zero for first call, original year for recursive calls
* @return int Next run time
*/
protected function get_next_scheduled_time_inner(int $now, array $validminutes, array $validhours,
array $validdays, array $validdaysofweek, array $validmonths, int $originalyear = 0) {
$currentyear = (int)date('Y', $now);
if ($originalyear) {
// In recursive calls, check we didn't go more than 8 years ahead, that indicates the
// user has chosen an impossible date. 8 years is the maximum time, considering a task
// set to run on 29 February over a century boundary when a leap year is skipped.
if ($currentyear - $originalyear > 8) {
// Use this time if it's never going to happen.
return self::NEVER_RUN_TIME;
}
$firstyear = $originalyear;
} else {
$firstyear = $currentyear;
}
$nextvalidhour = $this->next_in_list($currenthour, $validhours);
if ($nextvalidhour < $currenthour) {
$currentdayofweek += 1;
$currentday += 1;
$currentmonth = (int)date('n', $now);

// Evaluate month first.
$nextvalidmonth = $this->next_in_list($currentmonth, $validmonths);
if ($nextvalidmonth < $currentmonth) {
$currentyear += 1;
}
// If we moved to another month, set the current time to start of month, and restart calculations.
if ($nextvalidmonth !== $currentmonth) {
$newtime = strtotime($currentyear . '-' . $nextvalidmonth . '-01 00:00');
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}
$nextvaliddayofmonth = $this->next_in_list($currentday, $validdays);
$nextvaliddayofweek = $this->next_in_list($currentdayofweek, $validdaysofweek);

// Special handling for dayofmonth vs dayofweek (see man 5 cron). If both are specified, then
// it is ok to continue when either matches. If only one is specified then it must match.
$currentday = (int)date("j", $now);
$currentdayofweek = (int)date("w", $now);
$nextvaliddayofmonth = self::next_in_list($currentday, $validdays);
$nextvaliddayofweek = self::next_in_list($currentdayofweek, $validdaysofweek);
$daysincrementbymonth = $nextvaliddayofmonth - $currentday;
$daysinmonth = date('t');
$daysinmonth = (int)date('t', $now);
if ($nextvaliddayofmonth < $currentday) {
$daysincrementbymonth += $daysinmonth;
}
Expand All @@ -548,41 +609,65 @@ public function get_next_scheduled_time() {
$daysincrementbyweek += 7;
}

// Special handling for dayofmonth vs dayofweek:
// if either field is * - use the other field
// otherwise - choose the soonest (see man 5 cron).
if ($this->dayofweek == '*') {
$daysincrement = $daysincrementbymonth;
} else if ($this->day == '*') {
$daysincrement = $daysincrementbyweek;
} else {
// Take the smaller increment of days by month or week.
$daysincrement = $daysincrementbymonth;
if ($daysincrementbyweek < $daysincrementbymonth) {
$daysincrement = $daysincrementbyweek;
}
$daysincrement = min($daysincrementbymonth, $daysincrementbyweek);
}

$nextvaliddayofmonth = $currentday + $daysincrement;
if ($nextvaliddayofmonth > $daysinmonth) {
$currentmonth += 1;
$nextvaliddayofmonth -= $daysinmonth;
// If we moved day, recurse using new start time.
if ($daysincrement != 0) {
$newtime = strtotime($currentyear . '-' . $currentmonth . '-' . $currentday .
' 00:00 +' . $daysincrement . ' days');
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}

$nextvalidmonth = $this->next_in_list($currentmonth, $validmonths);
if ($nextvalidmonth < $currentmonth) {
$nextvalidyear += 1;
$currenthour = (int)date('H', $now);
$nextvalidhour = $this->next_in_list($currenthour, $validhours);
if ($nextvalidhour != $currenthour) {
if ($nextvalidhour < $currenthour) {
$offset = ' +1 day';
} else {
$offset = '';
}
$newtime = strtotime($currentyear . '-' . $currentmonth . '-' . $currentday . ' ' . $nextvalidhour .
':00' . $offset);
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}

// Work out the next valid time.
$nexttime = mktime($nextvalidhour,
$nextvalidminute,
0,
$nextvalidmonth,
$nextvaliddayofmonth,
$nextvalidyear);
// Round time down to an exact minute because we need to use numeric calculations on it now.
// If we construct times based on all the components, it will mess up around DST changes
// (because there are two times with the same representation).
$now = intdiv($now, 60) * 60;

$currentminute = (int)date('i', $now);
$nextvalidminute = $this->next_in_list($currentminute, $validminutes);
if ($nextvalidminute == $currentminute && !$originalyear) {
// This is not a recursive call so time has not moved on at all yet. We can't use the
// same minute as now because it has already happened, it has to be at least one minute
// later, so update time and retry.
$newtime = $now + 60;
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}

if ($nextvalidminute < $currentminute) {
// The time is in the next hour so we need to recurse. Don't use strtotime at this
// point because it will mess up around DST changes.
$minutesforward = $nextvalidminute + 60 - $currentminute;
$newtime = $now + $minutesforward * 60;
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}

return $nexttime;
// The next valid minute is in the same hour so it must be valid according to all other
// checks and we can finally return it.
return $now + ($nextvalidminute - $currentminute) * 60;
}

/**
Expand Down
Loading

0 comments on commit 1c4d433

Please sign in to comment.