From dd844035ac5790bd1befd0411a1da7910f2aa46a Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 1 Nov 2023 16:42:55 +0000 Subject: [PATCH] MDL-79966 core_task: Scheduled task timing (crontab fields) is wrong --- admin/tool/task/tests/form_test.php | 6 +- lib/classes/task/scheduled_task.php | 171 ++++++++++++++++++------- lib/tests/task/scheduled_task_test.php | 147 +++++++++++++++++++++ 3 files changed, 280 insertions(+), 44 deletions(-) diff --git a/admin/tool/task/tests/form_test.php b/admin/tool/task/tests/form_test.php index 8a0268da9c4..8cbb40d189f 100644 --- a/admin/tool/task/tests/form_test.php +++ b/admin/tool/task/tests/form_test.php @@ -236,6 +236,8 @@ class form_test extends \advanced_testcase { $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)); @@ -247,7 +249,7 @@ class form_test extends \advanced_testcase { $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'); @@ -271,6 +273,8 @@ class form_test extends \advanced_testcase { $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)); } } diff --git a/lib/classes/task/scheduled_task.php b/lib/classes/task/scheduled_task.php index d67fc39797a..05a08e2017b 100644 --- a/lib/classes/task/scheduled_task.php +++ b/lib/classes/task/scheduled_task.php @@ -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. @@ -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 = '*'; @@ -380,13 +387,25 @@ abstract class scheduled_task extends task_base { 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; } /** @@ -506,9 +525,14 @@ abstract class scheduled_task extends task_base { /** * 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(); @@ -518,27 +542,64 @@ abstract class scheduled_task extends task_base { $validdaysofweek = $this->get_valid(self::FIELD_DAYOFWEEK); $validmonths = $this->get_valid(self::FIELD_MONTH); - $nextvalidyear = date('Y'); - - $currentminute = date("i") + 1; - $currenthour = date("H"); - $currentday = date("j"); - $currentmonth = date("n"); - $currentdayofweek = date("w"); - - $nextvalidminute = $this->next_in_list($currentminute, $validminutes); - if ($nextvalidminute < $currentminute) { - $currenthour += 1; + // 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; } - $nextvalidhour = $this->next_in_list($currenthour, $validhours); - if ($nextvalidhour < $currenthour) { - $currentdayofweek += 1; - $currentday += 1; + + $result = self::get_next_scheduled_time_inner($now, $validminutes, $validhours, $validdays, $validdaysofweek, $validmonths); + return $result; + } + + /** + * 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; } - $nextvaliddayofmonth = $this->next_in_list($currentday, $validdays); - $nextvaliddayofweek = $this->next_in_list($currentdayofweek, $validdaysofweek); + $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); + } + + // 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; } @@ -548,41 +609,65 @@ abstract class scheduled_task extends task_base { $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); + } + + // 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); + } + + $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); } - $nextvaliddayofmonth = $currentday + $daysincrement; - if ($nextvaliddayofmonth > $daysinmonth) { - $currentmonth += 1; - $nextvaliddayofmonth -= $daysinmonth; + // 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); } - $nextvalidmonth = $this->next_in_list($currentmonth, $validmonths); - if ($nextvalidmonth < $currentmonth) { - $nextvalidyear += 1; + 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); } - // Work out the next valid time. - $nexttime = mktime($nextvalidhour, - $nextvalidminute, - 0, - $nextvalidmonth, - $nextvaliddayofmonth, - $nextvalidyear); - - 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; } /** diff --git a/lib/tests/task/scheduled_task_test.php b/lib/tests/task/scheduled_task_test.php index d108fd66000..7f7bf513cd6 100644 --- a/lib/tests/task/scheduled_task_test.php +++ b/lib/tests/task/scheduled_task_test.php @@ -165,6 +165,153 @@ class scheduled_task_test extends \advanced_testcase { $this->assertEquals(1, date('j', $nexttime)); } + /** + * Data provider for get_next_scheduled_time_detail. + * + * Note all times in here are in default Australia/Perth time zone. + * + * @return array[] Function parameters for each run + */ + public static function get_next_scheduled_time_detail_provider(): array { + return [ + // Every minute = next minute. + ['2023-11-01 15:15', '*', '*', '*', '*', '*', '2023-11-01 15:16'], + // Specified minute (coming up) = same hour, that minute. + ['2023-11-01 15:15', '18', '*', '*', '*', '*', '2023-11-01 15:18'], + // Specified minute (passed) = next hour, that minute. + ['2023-11-01 15:15', '11', '*', '*', '*', '*', '2023-11-01 16:11'], + // Range of minutes = same hour, next matching value. + ['2023-11-01 15:15', '*/15', '*', '*', '*', '*', '2023-11-01 15:30'], + // Specified hour, any minute = first minute that hour. + ['2023-11-01 15:15', '*', '20', '*', '*', '*', '2023-11-01 20:00'], + // Specified hour, specified minute = that time. + ['2023-11-01 15:15', '13', '20', '*', '*', '*', '2023-11-01 20:13'], + // Any minute, range of hours = next hour in range, 00:00. + ['2023-11-01 15:15', '*', '*/6', '*', '*', '*', '2023-11-01 18:00'], + // Specified minute, range of hours = next hour where minute not passed, that minute. + ['2023-11-01 18:15', '10', '*/6', '*', '*', '*', '2023-11-02 00:10'], + // Specified day, any hour/minute. + ['2023-11-01 15:15', '*', '*', '3', '*', '*', '2023-11-03 00:00'], + // Specified day (next month), any hour/minute. + ['2023-11-05 15:15', '*', '*', '3', '*', '*', '2023-12-03 00:00'], + // Specified day, specified hour. + ['2023-11-01 15:15', '*', '17', '3', '*', '*', '2023-11-03 17:00'], + // Specified day, specified minute. + ['2023-11-01 15:15', '17', '*', '3', '*', '*', '2023-11-03 00:17'], + // 30th of every month, February. + ['2023-01-31 15:15', '15', '10', '30', '*', '*', '2023-03-30 10:15'], + // Friday, any time. 2023-11-01 is a Wednesday, so it will run in 2 days. + ['2023-11-01 15:15', '*', '*', '*', '5', '*', '2023-11-03 00:00'], + // Friday, any time (but it's already Friday). + ['2023-11-03 15:15', '*', '*', '*', '5', '*', '2023-11-03 15:16'], + // Sunday (week rollover). + ['2023-11-01 15:15', '*', '*', '*', '0', '*', '2023-11-05 00:00'], + // Specified days and day of week (days come first). + ['2023-11-01 15:15', '*', '*', '2,4,6', '5', '*', '2023-11-02 00:00'], + // Specified days and day of week (day of week comes first). + ['2023-11-01 15:15', '*', '*', '4,6,8', '5', '*', '2023-11-03 00:00'], + // Specified months. + ['2023-11-01 15:15', '*', '*', '*', '*', '6,8,10,12', '2023-12-01 00:00'], + // Specified months (crossing year). + ['2023-11-01 15:15', '*', '*', '*', '*', '6,8,10', '2024-06-01 00:00'], + // Specified months and day of week (i.e. first Sunday in December). + ['2023-11-01 15:15', '*', '*', '*', '0', '6,8,10,12', '2023-12-03 00:00'], + // It's already December, but the next Friday is not until next month. + ['2023-12-30 15:15', '*', '*', '*', '5', '6,8,10,12', '2024-06-07 00:00'], + // Around end of year. + ['2023-12-31 23:00', '10', '3', '*', '*', '*', '2024-01-01 03:10'], + // Some impossible requirements... + ['2023-12-31 23:00', '*', '*', '30', '*', '2', scheduled_task::NEVER_RUN_TIME], + ['2023-12-31 23:00', '*', '*', '31', '*', '9,4,6,11', scheduled_task::NEVER_RUN_TIME], + // Normal years and leap years. + ['2021-01-01 23:00', '*', '*', '28', '*', '2', '2021-02-28 00:00'], + ['2021-01-01 23:00', '*', '*', '29', '*', '2', '2024-02-29 00:00'], + // Missing leap year over century. Longest possible gap between runs. + ['2096-03-01 00:00', '59', '23', '29', '*', '2', '2104-02-29 23:59'], + ]; + } + + /** + * Tests get_next_scheduled_time using a large number of example scenarios. + * + * @param string $now Current time (strtotime format) + * @param string $minute Minute restriction list for task + * @param string $hour Hour restriction list for task + * @param string $day Day restriction list for task + * @param string $dayofweek Day of week restriction list for task + * @param string $month Month restriction list for task + * @param string|int $expected Expected run time (strtotime format or time int) + * @dataProvider get_next_scheduled_time_detail_provider + * @covers ::get_next_scheduled_time + */ + public function test_get_next_scheduled_time_detail(string $now, string $minute, string $hour, + string $day, string $dayofweek, string $month, string|int $expected): void { + // Create test task with specified times. + $task = new scheduled_test_task(); + $task->set_minute($minute); + $task->set_hour($hour); + $task->set_day($day); + $task->set_day_of_week($dayofweek); + $task->set_month($month); + + // Check function results. + $nowtime = strtotime($now); + if (is_int($expected)) { + $expectedtime = $expected; + } else { + $expectedtime = strtotime($expected); + } + $actualtime = $task->get_next_scheduled_time($nowtime); + $this->assertEquals($expectedtime, $actualtime, 'Expected ' . $expected . ', actual ' . date('Y-m-d H:i', $actualtime)); + } + + /** + * Tests get_next_scheduled_time around DST changes, with regard to the continuity of frequent + * tasks. + * + * We want frequent tasks to keep progressing as normal and not randomly stop for an hour, or + * suddenly decide they need to happen in the past. + * + * @covers ::get_next_scheduled_time + */ + public function test_get_next_scheduled_time_dst_continuity(): void { + $this->resetAfterTest(); + $this->setTimezone('Europe/London'); + + // Test task is set to run every 20 minutes (:00, :20, :40). + $task = new scheduled_test_task(); + $task->set_minute('*/20'); + + // DST change forwards. Check times in GMT to ensure it progresses as normal. + $before = strtotime('2023-03-26 00:59 GMT'); + $this->assertEquals(strtotime('2023-03-26 00:59 Europe/London'), $before); + $one = $task->get_next_scheduled_time($before); + $this->assertEquals(strtotime('2023-03-26 01:00 GMT'), $one); + $this->assertEquals(strtotime('2023-03-26 02:00 Europe/London'), $one); + $two = $task->get_next_scheduled_time($one); + $this->assertEquals(strtotime('2023-03-26 01:20 GMT'), $two); + $three = $task->get_next_scheduled_time($two); + $this->assertEquals(strtotime('2023-03-26 01:40 GMT'), $three); + $four = $task->get_next_scheduled_time($three); + $this->assertEquals(strtotime('2023-03-26 02:00 GMT'), $four); + + // DST change backwards. + $before = strtotime('2023-10-29 00:59 GMT'); + // The 'before' time is 01:59 Europe/London, but we won't explicitly test that because + // there are two 01:59s so it might fail depending on implementation. + $one = $task->get_next_scheduled_time($before); + $this->assertEquals(strtotime('2023-10-29 01:00 GMT'), $one); + // We cannot compare against the Eerope/London time (01:00) because there are two 01:00s. + $two = $task->get_next_scheduled_time($one); + $this->assertEquals(strtotime('2023-10-29 01:20 GMT'), $two); + $three = $task->get_next_scheduled_time($two); + $this->assertEquals(strtotime('2023-10-29 01:40 GMT'), $three); + $four = $task->get_next_scheduled_time($three); + $this->assertEquals(strtotime('2023-10-29 02:00 GMT'), $four); + // This time is now unambiguous in Europe/London. + $this->assertEquals(strtotime('2023-10-29 02:00 Europe/London'), $four); + } + public function test_timezones() { global $CFG, $USER;