MDL-71044 backup: backup_cleanup_task task to remove old files

backup/moodle2/backup_stepslib.php

@@ -44,11 +44,9 @@ class create_and_clean_temp_stuff extends backup_execution_step {
 }
 
 /**
- * Delete the temp dir used by backup/restore (conditionally),
- * delete old directories and drop temp ids table. Note we delete
- * the directory but not the corresponding log file that will be
- * there for, at least, 1 week - only delete_old_backup_dirs() or cron
- * deletes log files (for easier access to them).
+ * Delete the temp dir used by backup/restore (conditionally) and drop temp ids table.
+ * Note we delete the directory but not the corresponding log file that will be
+ * there until cron cleans it up.
  */
 class drop_and_clean_temp_stuff extends backup_execution_step {
 
@@ -58,7 +56,6 @@ class drop_and_clean_temp_stuff extends backup_execution_step {
         global $CFG;
 
         backup_controller_dbops::drop_backup_ids_temp_table($this->get_backupid()); // Drop ids temp table
-        backup_helper::delete_old_backup_dirs(strtotime('-1 week'));                // Delete > 1 week old temp dirs.
         // Delete temp dir conditionally:
         // 1) If $CFG->keeptempdirectoriesonbackup is not enabled
         // 2) If backup temp dir deletion has been marked to be avoided

backup/moodle2/restore_stepslib.php

@@ -57,21 +57,19 @@ class restore_create_and_clean_temp_stuff extends restore_execution_step {
 }
 
 /**
- * delete the temp dir used by backup/restore (conditionally),
- * delete old directories and drop temp ids table
+ * Drop temp ids table and delete the temp dir used by backup/restore (conditionally).
  */
 class restore_drop_and_clean_temp_stuff extends restore_execution_step {
 
     protected function define_execution() {
         global $CFG;
         restore_controller_dbops::drop_restore_temp_tables($this->get_restoreid()); // Drop ids temp table
-        $progress = $this->task->get_progress();
-        $progress->start_progress('Deleting backup dir');
-        backup_helper::delete_old_backup_dirs(strtotime('-1 week'), $progress);      // Delete > 1 week old temp dirs.
         if (empty($CFG->keeptempdirectoriesonbackup)) { // Conditionally
+            $progress = $this->task->get_progress();
+            $progress->start_progress('Deleting backup dir');
             backup_helper::delete_backup_dir($this->task->get_tempdir(), $progress); // Empty restore dir
+            $progress->end_progress();
         }
-        $progress->end_progress();
     }
 }
 

backup/tests/backup_cleanup_task_test.php

@@ -0,0 +1,157 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Tests for the \core\task\backup_cleanup_task scheduled task.
+ *
+ * @package    core_backup
+ * @copyright  2021 Mikhail Golenkov <mikhailgolenkov@catalyst-au.net>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+global $CFG;
+require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php');
+
+/**
+ * Tests for the \core\task\backup_cleanup_task scheduled task.
+ *
+ * @copyright  2021 Mikhail Golenkov <mikhailgolenkov@catalyst-au.net>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class core_backup_cleanup_task_testcase extends advanced_testcase {
+
+    /**
+     * Set up tasks for all tests.
+     */
+    protected function setUp(): void {
+        $this->resetAfterTest(true);
+    }
+
+    /**
+     * Take a backup of the course provided and return backup id.
+     *
+     * @param int $courseid Course id to be backed up.
+     * @return string Backup id.
+     */
+    private function backup_course(int $courseid): string {
+        // Backup the course.
+        $user = get_admin();
+        $controller = new \backup_controller(
+            \backup::TYPE_1COURSE,
+            $courseid,
+            \backup::FORMAT_MOODLE,
+            \backup::INTERACTIVE_NO,
+            \backup::MODE_AUTOMATED,
+            $user->id
+        );
+        $controller->execute_plan();
+        return $controller->get_backupid();
+    }
+
+    /**
+     * Test the task idle run. Nothing should explode.
+     */
+    public function test_backup_cleanup_task_idle() {
+        $task = new \core\task\backup_cleanup_task();
+        $task->execute();
+    }
+
+    /**
+     * Test the task exits when backup | loglifetime setting is not set.
+     */
+    public function test_backup_cleanup_task_exits() {
+        set_config('loglifetime', 0, 'backup');
+        $task = new \core\task\backup_cleanup_task();
+        ob_start();
+        $task->execute();
+        $output = ob_get_contents();
+        ob_end_clean();
+        $this->assertStringContainsString('config is not set', $output);
+    }
+
+    /**
+     * Test the task deletes records from DB.
+     */
+    public function test_backup_cleanup_task_deletes_records() {
+        global $DB;
+
+        // Create a course.
+        $generator = $this->getDataGenerator();
+        $course = $generator->create_course();
+
+        // Take two backups of the course.
+        $backupid1 = $this->backup_course($course->id);
+        $backupid2 = $this->backup_course($course->id);
+
+        // Emulate the first backup to be done 31 days ago.
+        $bcrecord = $DB->get_record('backup_controllers', ['backupid' => $backupid1]);
+        $bcrecord->timecreated -= DAYSECS * 31;
+        $DB->update_record('backup_controllers', $bcrecord);
+
+        // Run the task.
+        $task = new \core\task\backup_cleanup_task();
+        $task->execute();
+
+        // There should be no records related to the first backup.
+        $this->assertEquals(0, $DB->count_records('backup_controllers', ['backupid' => $backupid1]));
+        $this->assertEquals(0, $DB->count_records('backup_logs', ['backupid' => $backupid1]));
+
+        // Records related to the second backup should remain.
+        $this->assertGreaterThan(0, $DB->count_records('backup_controllers', ['backupid' => $backupid2]));
+        $this->assertGreaterThanOrEqual(0, $DB->count_records('backup_logs', ['backupid' => $backupid2]));
+    }
+
+    /**
+     * Test the task deletes files from file system.
+     */
+    public function test_backup_cleanup_task_deletes_files() {
+        global $CFG;
+
+        // Create a course.
+        $generator = $this->getDataGenerator();
+        $course = $generator->create_course();
+
+        // Take two backups of the course and get their logs.
+        $backupid1 = $this->backup_course($course->id);
+        $backupid2 = $this->backup_course($course->id);
+        $filepath1 = $CFG->backuptempdir . '/' . $backupid1 . '.log';
+        $filepath2 = $CFG->backuptempdir . '/' . $backupid2 . '.log';
+
+        // Create a subdirectory.
+        $subdir = $CFG->backuptempdir . '/subdir';
+        make_writable_directory($subdir);
+
+        // Both logs and the dir should exist.
+        $this->assertTrue(file_exists($filepath1));
+        $this->assertTrue(file_exists($filepath2));
+        $this->assertTrue(file_exists($subdir));
+
+        // Change modification time of the first log and the sub dir to be 8 days ago.
+        touch($filepath1, time() - 8 * DAYSECS);
+        touch($subdir, time() - 8 * DAYSECS);
+
+        // Run the task.
+        $task = new \core\task\backup_cleanup_task();
+        $task->execute();
+
+        // Files and directories older than a week are supposed to be removed.
+        $this->assertFalse(file_exists($filepath1));
+        $this->assertFalse(file_exists($subdir));
+        $this->assertTrue(file_exists($filepath2));
+    }
+}

backup/util/helper/backup_helper.class.php

@@ -153,25 +153,27 @@ abstract class backup_helper {
      * If supplied, progress object should be ready to receive indeterminate
      * progress reports.
      *
-     * @param int $deletefrom Time to delete from
+     * @param int $deletebefore Delete files and directories older than this time
      * @param \core\progress\base $progress Optional progress reporting object
      */
-    static public function delete_old_backup_dirs($deletefrom, \core\progress\base $progress = null) {
+    static public function delete_old_backup_dirs($deletebefore, \core\progress\base $progress = null) {
         $status = true;
-        // Get files and directories in the backup temp dir without descend.
+        // Get files and directories in the backup temp dir.
         $backuptempdir = make_backup_temp_directory('');
-        $list = get_directory_list($backuptempdir, '', false, true, true);
-        foreach ($list as $file) {
-            $file_path = $backuptempdir . '/' . $file;
-            $moddate = filemtime($file_path);
-            if ($status && $moddate < $deletefrom) {
-                //If directory, recurse
-                if (is_dir($file_path)) {
-                    // $file is really the backupid
-                    $status = self::delete_backup_dir($file, $progress);
-                //If file
-                } else {
-                    unlink($file_path);
+        $items = new DirectoryIterator($backuptempdir);
+        foreach ($items as $item) {
+            if ($item->isDot()) {
+                continue;
+            }
+            if ($item->getMTime() < $deletebefore) {
+                if ($item->isDir()) {
+                    // The item is a directory for some backup.
+                    if (!self::delete_backup_dir($item->getFilename(), $progress)) {
+                        // Something went wrong. Finish the list of items and then throw an exception.
+                        $status = false;
+                    }
+                } else if ($item->isFile()) {
+                    unlink($item->getPathname());
                 }
             }
         }

lang/en/admin.php

@@ -1305,7 +1305,7 @@ $string['tasktype'] = 'Type';
 $string['taskadmintitle'] = 'Tasks';
 $string['taskanalyticscleanup'] = 'Analytics cleanup';
 $string['taskautomatedbackup'] = 'Automated backups';
-$string['taskbackupcleanup'] = 'Clean backup tables and logs';
+$string['taskbackupcleanup'] = 'Clean backup tables, logs and files';
 $string['taskbadgescron'] = 'Award badges';
 $string['taskbadgesmessagecron'] = 'Background processing for sending badges notifications';
 $string['taskblogcron'] = 'Sync external blogs';

lib/classes/task/backup_cleanup_task.php

@@ -23,6 +23,10 @@
  */
 namespace core\task;
 
+defined('MOODLE_INTERNAL') || die();
+
+require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php');
+
 /**
  * Simple task to delete old backup records.
  */
@@ -47,7 +51,8 @@ class backup_cleanup_task extends scheduled_task {
         $loglifetime = get_config('backup', 'loglifetime');
 
         if (empty($loglifetime)) {
-            throw new coding_exception('The \'loglifetime\' config is not set. Can\'t proceed and delete old backup records.');
+            mtrace('The \'loglifetime\' config is not set. Can\'t proceed and delete old backup records.');
+            return;
         }
 
         // First, get the list of all backupids older than loglifetime.
@@ -65,6 +70,9 @@ class backup_cleanup_task extends scheduled_task {
                 $DB->delete_records('backup_controllers', array('backupid' => $record->backupid));
             }
         }
+
+        // Delete files and dirs older than 1 week.
+        \backup_helper::delete_old_backup_dirs(strtotime('-1 week'));
     }
 
 }