Merge branch 'MDL-84442-metadata-task-logs-MOODLE_405_STABLE' of https://github.com/bwalkerl/moodle into MOODLE_405_STABLE

.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

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);
                 }
             }
         }

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);
+    }
 }