diff --git a/.upgradenotes/MDL-84442-2025042111220326.yml b/.upgradenotes/MDL-84442-2025042111220326.yml new file mode 100644 index 00000000000..19d18a79c46 --- /dev/null +++ b/.upgradenotes/MDL-84442-2025042111220326.yml @@ -0,0 +1,8 @@ +issueNumber: MDL-84442 +notes: + core: + - message: >- + Added a new optional param to adhoc_task_failed and + scheduled_task_failed to allow skipping log finalisation when called + from a separate task. + type: improved diff --git a/lib/classes/task/manager.php b/lib/classes/task/manager.php index 2c0eb826a39..5dd7a4158b8 100644 --- a/lib/classes/task/manager.php +++ b/lib/classes/task/manager.php @@ -1082,14 +1082,17 @@ class manager { * This function indicates that an adhoc task was not completed successfully and should be retried. * * @param \core\task\adhoc_task $task + * @param bool $finaliselog finalise the log of the current running task */ - public static function adhoc_task_failed(adhoc_task $task) { + public static function adhoc_task_failed(adhoc_task $task, bool $finaliselog = true) { global $DB; $clock = \core\di::get(\core\clock::class); // Finalise the log output. - logmanager::finalise_log(true); + if ($finaliselog) { + logmanager::finalise_log(true); + } $delay = $task->get_fail_delay(); @@ -1192,14 +1195,17 @@ class manager { * This function indicates that a scheduled task was not completed successfully and should be retried. * * @param \core\task\scheduled_task $task + * @param bool $finaliselog finalise the log of the current running task */ - public static function scheduled_task_failed(scheduled_task $task) { + public static function scheduled_task_failed(scheduled_task $task, bool $finaliselog = true) { global $DB; $clock = \core\di::get(\core\clock::class); // Finalise the log output. - logmanager::finalise_log(true); + if ($finaliselog) { + logmanager::finalise_log(true); + } $delay = $task->get_fail_delay(); @@ -1418,7 +1424,10 @@ class manager { $task = self::scheduled_task_from_record($taskrecord); $task->set_lock($lock); - self::scheduled_task_failed($task); + + // We have to skip log finalisation when failing the task as the finalise_log method from + // the log manager is only aware of the current running task (i.e., the cleanup task). + self::scheduled_task_failed($task, false); } else if ($runningtask->type == 'adhoc') { // Ad hoc tasks are removed from the DB if they finish successfully. // If we can't re-get this task, that means it finished and was properly @@ -1430,7 +1439,10 @@ class manager { $task = self::adhoc_task_from_record($taskrecord); $task->set_lock($lock); - self::adhoc_task_failed($task); + + // We have to skip log finalisation when failing the task as the finalise_log method from + // the log manager is only aware of the current running task (i.e., the cleanup task). + self::adhoc_task_failed($task, false); } } } diff --git a/lib/tests/task/running_test.php b/lib/tests/task/running_test.php index d10a5ca1184..e1086b899c4 100644 --- a/lib/tests/task/running_test.php +++ b/lib/tests/task/running_test.php @@ -156,4 +156,110 @@ final class running_test extends \advanced_testcase { $running = manager::get_running_tasks(); $this->assertCount(0, $running); } + + /** + * Test for adhoc task cleanup. + * + * @covers \core\task\manager::cleanup_metadata() + */ + public function test_adhoc_cleanup_metadata(): void { + global $DB; + $this->resetAfterTest(true); + + $clock = $this->mock_clock_with_frozen(); + + // Specify lock factory to avoid previously mentioned issues with Postgres locks. + set_config('lock_factory', '\core\lock\db_record_lock_factory'); + + // Disable all scheduled tasks except the cleanup task. + $classname = 'core\task\task_lock_cleanup_task'; + $DB->set_field_select('task_scheduled', 'disabled', 1, 'classname != ?', ["\\$classname"]); + $DB->set_field('task_scheduled', 'nextruntime', 1, ['classname' => "\\$classname"]); + + // Create an adhoc task. + $task = new adhoc_test_task(); + $task->set_next_run_time($clock->time() - MINSECS); + + // Queue and start the adhoc task. + manager::queue_adhoc_task($task); + $task = manager::get_next_adhoc_task($clock->time()); + manager::adhoc_task_starting($task); + + // Release the lock to simulate an adhoc task that has been destroyed but hasn't been cleaned up. + $task->get_lock()->release(); + $this->assertCount(1, manager::get_running_tasks()); + + // Run the cleanup scheduled task one hour later. + $clock->bump(HOURSECS); + $cleanuptask = manager::get_next_scheduled_task($clock->time()); + manager::scheduled_task_starting($cleanuptask); + logmanager::start_logging($cleanuptask); + $this->assertCount(2, manager::get_running_tasks()); + $cleanuptask->execute(); + + // Confirm the task has been cleaned up. + $this->assertCount(1, manager::get_running_tasks()); + + // Check the task log hasn't been finalised for the cleanup task. + $record = $DB->get_record('task_log', ['classname' => $classname]); + $this->assertEmpty($record); + + // Now complete the task and make sure it was successful (0 = success, 1 = fail). + manager::scheduled_task_complete($cleanuptask); + $record = $DB->get_record('task_log', ['classname' => $classname]); + $this->assertEquals(0, $record->result); + } + + /** + * Test for scheduled task cleanup. + * + * @covers \core\task\manager::cleanup_metadata() + */ + public function test_scheduled_cleanup_metadata(): void { + global $DB; + $this->resetAfterTest(true); + + $clock = $this->mock_clock_with_frozen(); + + // Specify lock factory to avoid previously mentioned issues with Postgres locks. + set_config('lock_factory', '\core\lock\db_record_lock_factory'); + + // Disable all scheduled tasks except the cleanup task. + $classname = 'core\task\task_lock_cleanup_task'; + $DB->set_field_select('task_scheduled', 'disabled', 1, 'classname != ?', ["\\$classname"]); + $DB->set_field('task_scheduled', 'nextruntime', $clock->time() + MINSECS, ['classname' => "\\$classname"]); + + // Create a new scheduled task. + $task = new scheduled_test_task(); + $task->set_next_run_time($clock->time() - MINSECS); + + // Insert and start the test scheduled task. + $DB->insert_record('task_scheduled', manager::record_from_scheduled_task($task)); + $task = manager::get_next_scheduled_task($clock->time()); + manager::scheduled_task_starting($task); + + // Release the lock to simulate a scheduled task that has been destroyed but hasn't been cleaned up. + $task->get_lock()->release(); + $this->assertCount(1, manager::get_running_tasks()); + + // Run the cleanup scheduled task one hour later. + $clock->bump(HOURSECS); + $cleanuptask = manager::get_next_scheduled_task($clock->time()); + manager::scheduled_task_starting($cleanuptask); + logmanager::start_logging($cleanuptask); + $this->assertCount(2, manager::get_running_tasks()); + $cleanuptask->execute(); + + // Confirm the task has been cleaned up. + $this->assertCount(1, manager::get_running_tasks()); + + // Check the task log hasn't been finalised for the cleanup task. + $record = $DB->get_record('task_log', ['classname' => $classname]); + $this->assertEmpty($record); + + // Now complete the task and make sure it was successful (0 = success, 1 = fail). + manager::scheduled_task_complete($cleanuptask); + $record = $DB->get_record('task_log', ['classname' => $classname]); + $this->assertEquals(0, $record->result); + } }