From 8c719efc1b837450de809181c5de94cafbd917f8 Mon Sep 17 00:00:00 2001 From: Huong Nguyen Date: Tue, 19 Dec 2023 10:24:59 +0700 Subject: [PATCH 1/2] MDL-75579 adhoc_task: Implement no-retry flag --- lib/classes/task/manager.php | 11 ++++++++ lib/tests/fixtures/task_fixtures.php | 19 ++++++++++++++ lib/tests/task/adhoc_task_test.php | 39 ++++++++++++++++++++++++++++ lib/upgrade.txt | 2 ++ 4 files changed, 71 insertions(+) diff --git a/lib/classes/task/manager.php b/lib/classes/task/manager.php index 90ea9450205..a59ddbfa0c8 100644 --- a/lib/classes/task/manager.php +++ b/lib/classes/task/manager.php @@ -1088,6 +1088,17 @@ class manager { $delay = $task->get_fail_delay(); + if ($delay > 0) { + // If the task has a fail delay, it's already run at least once. + // We need to check if the task should be retried or not. + $taskcustomdata = $task->get_custom_data(); + if ($taskcustomdata && isset($taskcustomdata->noretry) && $taskcustomdata->noretry) { + // The task has been marked as not retrying, so we can mark it as completed and delete it. + self::adhoc_task_complete($task); + return; + } + } + // Reschedule task with exponential fall off for failing tasks. if (empty($delay)) { $delay = 60; diff --git a/lib/tests/fixtures/task_fixtures.php b/lib/tests/fixtures/task_fixtures.php index efaaa98b54b..c7500ba0cc4 100644 --- a/lib/tests/fixtures/task_fixtures.php +++ b/lib/tests/fixtures/task_fixtures.php @@ -107,6 +107,25 @@ class adhoc_test4_task extends adhoc_test_task { class adhoc_test5_task extends adhoc_test_task { } +/** + * Test adhoc-task with no-retry customdata. + * + * @copyright 2023 Huong Nguyen + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class adhoc_test6_task extends adhoc_test_task { + /** + * Execute. + * + * @return void + */ + public function execute(): void { + // Something wrong happened, do not retry. + $this->set_custom_data((object) ['noretry' => true]); + } + +} + class scheduled_test_task extends \core\task\scheduled_task { public function get_name() { return "Test task"; diff --git a/lib/tests/task/adhoc_task_test.php b/lib/tests/task/adhoc_task_test.php index dbd7033deb8..008c4b880ea 100644 --- a/lib/tests/task/adhoc_task_test.php +++ b/lib/tests/task/adhoc_task_test.php @@ -605,4 +605,43 @@ class adhoc_task_test extends \advanced_testcase { $output ); } + + /** + * Test adhoc task failure without retry. + * + * @covers ::get_next_adhoc_task + * @covers ::get_adhoc_task + * @covers ::adhoc_task_failed + */ + public function test_get_next_adhoc_task_without_fail_retry(): void { + global $DB; + $this->resetAfterTest(); + + // Create an adhoc task. + $task = new adhoc_test6_task(); + manager::queue_adhoc_task($task); + $this->assertCount(1, $DB->get_records('task_adhoc')); + + $now = time(); + + // Get the task from the scheduler, execute it, and mark it as failed. + $task = manager::get_next_adhoc_task($now); + $taskid = $task->get_id(); + $task->execute(); + manager::adhoc_task_failed($task); + $this->assertCount(1, $DB->get_records('task_adhoc')); + + // Get the task from the scheduler (retry after delay). Fail it again. + $task = manager::get_next_adhoc_task($now + 120); + $this->assertInstanceOf('\\core\\task\\adhoc_test6_task', $task); + $this->assertEquals($taskid, $task->get_id()); + $task->execute(); + manager::adhoc_task_failed($task); + + // The task was marked as no-retry, so it was deleted. + $this->assertCount(0, $DB->get_records('task_adhoc')); + $this->expectException(\moodle_exception::class); + $this->expectExceptionMessage('error/invalidtaskid'); + manager::get_adhoc_task($taskid); + } } diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 57274bde415..35000caa2b0 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -4,6 +4,8 @@ information provided here is intended especially for developers. === 4.3.2 === * The current page language is available in new `core/config` language property for Javascript modules +* The customdata of adhoc_task class now accepts a new value called noretry. If set to true, the ad-hoc task will not be retried + if it fails. === 4.3.1 === From 7d4f09b97808f70813ed5fab6fb3a0f5d3b19ced Mon Sep 17 00:00:00 2001 From: Huong Nguyen Date: Tue, 19 Dec 2023 10:27:40 +0700 Subject: [PATCH 2/2] MDL-75579 Backup: Prevent failed course restore task to be retried --- .../task/asynchronous_restore_task.php | 65 ++++++++++++------- 1 file changed, 40 insertions(+), 25 deletions(-) diff --git a/lib/classes/task/asynchronous_restore_task.php b/lib/classes/task/asynchronous_restore_task.php index fcd3b2a5482..97416be813a 100644 --- a/lib/classes/task/asynchronous_restore_task.php +++ b/lib/classes/task/asynchronous_restore_task.php @@ -46,7 +46,8 @@ class asynchronous_restore_task extends adhoc_task { global $DB; $started = time(); - $restoreid = $this->get_custom_data()->backupid; + $customdata = $this->get_custom_data(); + $restoreid = $customdata->backupid; $restorerecord = $DB->get_record('backup_controllers', array('backupid' => $restoreid), 'id, controller', IGNORE_MISSING); // If the record doesn't exist, the backup controller failed to create. Unable to proceed. if (empty($restorerecord)) { @@ -62,37 +63,51 @@ class asynchronous_restore_task extends adhoc_task { return; } $rc = \restore_controller::load_controller($restoreid); - $rc->set_progress(new \core\progress\db_updater($restorerecord->id, 'backup_controllers', 'progress')); + try { + $rc->set_progress(new \core\progress\db_updater($restorerecord->id, 'backup_controllers', 'progress')); - // Do some preflight checks on the restore. - $status = $rc->get_status(); - $execution = $rc->get_execution(); + // Do some preflight checks on the restore. + $status = $rc->get_status(); + $execution = $rc->get_execution(); - // Check that the restore is in the correct status and - // that is set for asynchronous execution. - if ($status == \backup::STATUS_AWAITING && $execution == \backup::EXECUTION_DELAYED) { - // Execute the restore. - $rc->execute_plan(); + // Check that the restore is in the correct status and + // that is set for asynchronous execution. + if ($status == \backup::STATUS_AWAITING && $execution == \backup::EXECUTION_DELAYED) { + // Execute the restore. + $rc->execute_plan(); + + // Send message to user if enabled. + $messageenabled = (bool) get_config('backup', 'backup_async_message_users'); + if ($messageenabled && $rc->get_status() == \backup::STATUS_FINISHED_OK) { + $asynchelper = new async_helper('restore', $restoreid); + $asynchelper->send_message(); + } + + } else { + // If status isn't 700, it means the process has failed. + // Retrying isn't going to fix it, so marked operation as failed. + $rc->set_status(\backup::STATUS_FINISHED_ERR); + mtrace('Bad backup controller status, is: ' . $status . ' should be 700, marking job as failed.'); - // Send message to user if enabled. - $messageenabled = (bool)get_config('backup', 'backup_async_message_users'); - if ($messageenabled && $rc->get_status() == \backup::STATUS_FINISHED_OK) { - $asynchelper = new async_helper('restore', $restoreid); - $asynchelper->send_message(); } - } else { - // If status isn't 700, it means the process has failed. - // Retrying isn't going to fix it, so marked operation as failed. + $duration = time() - $started; + mtrace('Restore completed in: ' . $duration . ' seconds'); + } catch (\Exception $e) { + // If an exception is thrown, mark the restore as failed. $rc->set_status(\backup::STATUS_FINISHED_ERR); - mtrace('Bad backup controller status, is: ' . $status . ' should be 700, marking job as failed.'); + // Retrying isn't going to fix this, so add a no-retry flag to customdata. + // We can cancel the task in the task manager. + $customdata->noretry = true; + $this->set_custom_data($customdata); + + mtrace('Exception thrown during restore execution, marking job as failed.'); + mtrace($e->getMessage()); + } finally { + // Cleanup. + // Always destroy the controller. + $rc->destroy(); } - - // Cleanup. - $rc->destroy(); - - $duration = time() - $started; - mtrace('Restore completed in: ' . $duration . ' seconds'); } }