From 1736005648c582117cff2f2996272315c849eb7a Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Fri, 6 Nov 2026 19:44:52 +0100 Subject: [PATCH] Review of 1.0.3: names a snapshot cannot hold, restore-directory errors under salvage, browse speed - Leave out and report a file or folder whose name a snapshot cannot hold - A staging write failure stops a full restore under --salvage - Browse reuses its repository; empty folders read along with the entries - Keep leases alive while empty folders are created; exact-prefix empty folder is the target - Clean up tables that were partly created or before the transaction started - Bin: no empty message after a read error; longer pages --- CHANGELOG.md | 3 +++ lib/Controller/AdminApiController.php | 2 +- lib/Db/DbDumper.php | 9 +++++++-- lib/Job/BackupRun.php | 16 +++++++++------ lib/Repository/Repository.php | 17 ++++++++++------ lib/Service/RestoreFullService.php | 2 ++ lib/Service/RestoreService.php | 10 +++++++--- src/components/Bin.vue | 2 +- tests/Unit/Repository/EmptyDirsTest.php | 26 +++++++++++++++++++++++++ 9 files changed, 68 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3547364..bf1ce07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,9 @@ shown under 1.0.0. The git tag of a release candidate goes after the date. - A full disk during a restore no longer looks like a damaged backup. The restore now stops with a message about the disk, also with `--salvage`, and does not point at `--salvage` any more. The same goes for other trouble writing to the restore directory, such as permissions. +- A file or folder whose name a snapshot cannot hold (a backslash, a control character, not UTF-8) is + left out and reported as unreadable. Before, a backslash or control character could make the whole + restore point unreadable, and a name that is not UTF-8 made the backup fail. - A restore that goes on after a stop also removes the temporary files in folders where every file is already in place, and checks that those folders are inside the restore directory before it leaves their files alone. diff --git a/lib/Controller/AdminApiController.php b/lib/Controller/AdminApiController.php index 72fda95..ae1f6a4 100644 --- a/lib/Controller/AdminApiController.php +++ b/lib/Controller/AdminApiController.php @@ -83,7 +83,7 @@ final class AdminApiController extends Controller { /** Seconds of backup work one tick() does: well within a normal web request. */ public const TICK_SECONDS = 10.0; /** Seconds one request for the Bin reads, before the page shows how far it is and asks for more. */ - private const TRASH_PAGE_SECONDS = 2.0; + private const TRASH_PAGE_SECONDS = 5.0; /** * Move running backups forward from the open admin page, when background jobs do not run on a diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index 2a532d2..e8eff0c 100644 --- a/lib/Db/DbDumper.php +++ b/lib/Db/DbDumper.php @@ -160,8 +160,8 @@ final class DbDumper { try { foreach ($missing as $table) { if (isset($manifest['tables'][$table]['schema'])) { + $report['created'][] = $table; // first: creating it can stop halfway (its indexes) $this->db->createSchemaManager()->createTable(self::tableFromStructure($table, $manifest['tables'][$table]['schema'], $this->tableOptions())); - $report['created'][] = $table; } else { $report['missing'][] = $table; } @@ -172,7 +172,12 @@ final class DbDumper { } $total = 0; $foreignKeys = []; - $this->db->beginTransaction(); + try { + $this->db->beginTransaction(); + } catch (\Throwable $e) { + $this->dropCreatedTables($report); + throw $e; + } try { $foreignKeys = $this->suspendForeignKeys(); // first: on SQLite it also takes the write lock foreach ($manifest['tables'] as $table => $info) { diff --git a/lib/Job/BackupRun.php b/lib/Job/BackupRun.php index f15fd4f..5b42579 100644 --- a/lib/Job/BackupRun.php +++ b/lib/Job/BackupRun.php @@ -59,17 +59,21 @@ final class BackupRun { $finishedWalk = true; foreach ($walker->walk($state['after']) as $logical => $path) { + if (!mb_check_encoding($logical, 'UTF-8') || !Repository::isSafePath($logical)) { + // A name a snapshot cannot hold (a backslash, a control character, not UTF-8). One + // such path must not make a backup fail, or the whole snapshot unreadable later. + self::recordUnreadable($state, $stats, mb_scrub($logical, 'UTF-8')); + $state['cur'] = null; + $state['after'] = $logical; + continue; + } if (is_dir($path)) { $mtime = @filemtime($path); if ($mtime === false) { continue; // removed while walking } - if (mb_check_encoding($logical, 'UTF-8')) { - $emptyDirs[] = json_encode([$logical, $mtime], JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR); - $stats['emptyDirs'] = ($stats['emptyDirs'] ?? 0) + 1; - } else { - self::recordUnreadable($state, $stats, $logical); // a snapshot only holds UTF-8 - } + $emptyDirs[] = json_encode([$logical, $mtime], JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR); + $stats['emptyDirs'] = ($stats['emptyDirs'] ?? 0) + 1; $state['after'] = $logical; if (microtime(true) >= $deadline) { $finishedWalk = false; diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index e900b25..398ae20 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -384,7 +384,12 @@ final class Repository { // read along with the files, so this asks nothing more of the location. foreach ($this->emptyDirs($snapshotId) as [$path, $mtime]) { if (self::underPrefix($path, $prefix)) { - self::restoreEmptyDir($target, self::destination($target, $path, $prefix, $stripPrefix), $path, $mtime); + if ($heartbeat !== null) { + $heartbeat(); + } + // The prefix itself, stripped, is the target (for a file it is the file's name there). + $dest = $stripPrefix && $path === rtrim($prefix, '/') ? rtrim($target, '/') : self::destination($target, $path, $prefix, $stripPrefix); + self::restoreEmptyDir($target, $dest, $path, $mtime); } } return $count; @@ -397,13 +402,13 @@ final class Repository { */ public function emptyDirs(string $snapshotId): array { $meta = $this->lastMeta[$snapshotId] ?? $this->snapshotMeta($snapshotId); - $dirs = $meta['emptyDirs'] ?? []; - foreach (is_array($dirs) ? $dirs : [null] as $dir) { - if (!is_array($dir) || !is_string($dir[0] ?? null) || !self::isSafePath($dir[0]) || !is_int($dir[1] ?? null)) { - throw new RepositoryException('Invalid empty folder in snapshot ' . $snapshotId); + $dirs = []; + foreach (is_array($meta['emptyDirs'] ?? null) ? $meta['emptyDirs'] : [] as $dir) { + // One folder that makes no sense is no reason to give up on the files of a restore. + if (is_array($dir) && is_string($dir[0] ?? null) && self::isSafePath($dir[0]) && is_int($dir[1] ?? null)) { + $dirs[] = [$dir[0], $dir[1]]; } } - /** @var list */ return $dirs; } diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index b463251..4c2cc68 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -209,6 +209,8 @@ final class RestoreFullService { try { /** @var array $backupConfig */ $backupConfig = $this->fetchBackupConfig($target, $snapshotId, $salvage); + } catch (\OCA\NgBackup\Repository\RestoreTargetException $e) { + throw $e; // nowhere to put it: not a config.php that cannot be read } catch (\Throwable $e) { if (!$salvage) { throw $e; diff --git a/lib/Service/RestoreService.php b/lib/Service/RestoreService.php index bcd2d3b..83d3153 100644 --- a/lib/Service/RestoreService.php +++ b/lib/Service/RestoreService.php @@ -61,7 +61,8 @@ final class RestoreService { $prefix = trim($path, '/'); $prefix = $prefix === '' ? '' : $prefix . '/'; $children = []; - foreach ($this->targets->repository($target)->entries($snapshotId) as $e) { + $repo = $this->targets->repository($target); + foreach ($repo->entries($snapshotId) as $e) { if ($prefix !== '' && !str_starts_with($e['p'], $prefix)) { continue; } @@ -75,7 +76,7 @@ final class RestoreService { } $children[$name] = $c; } - foreach ($this->targets->repository($target)->emptyDirs($snapshotId) as [$dir]) { + foreach ($repo->emptyDirs($snapshotId) as [$dir]) { // read along with the entries if ($prefix === '' || str_starts_with($dir, $prefix)) { $name = explode('/', substr($dir, strlen($prefix)), 2)[0]; $children[$name] ??= ['name' => $name, 'type' => 'dir', 'size' => 0, 'mtime' => null, 'files' => 0]; @@ -180,6 +181,9 @@ final class RestoreService { if (!$existed) { $folder->touch($mtime); } + if ($progress !== null) { + $progress($restored, count($names), $dir); // keeps the lease alive too + } } $trashed = 0; @@ -204,7 +208,7 @@ final class RestoreService { throw new \InvalidArgumentException('Invalid path: ' . $prefix); } if (!is_dir($directory) && !mkdir($directory, 0750, true) && !is_dir($directory)) { - throw new \RuntimeException("Cannot create $directory"); + throw new \OCA\NgBackup\Repository\RestoreTargetException("Cannot create $directory (disk full, or no permission?)"); } return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId, $prefix, $directory, $stripPrefix, $heartbeat, &$skipped, $keepUnchanged): int { $own = LeaseService::throttled($refresh); diff --git a/src/components/Bin.vue b/src/components/Bin.vue index b3d8abb..7bdb49b 100644 --- a/src/components/Bin.vue +++ b/src/components/Bin.vue @@ -17,7 +17,7 @@

{{ t('ng_backup', 'This can take a while: every removed restore point is read from the location.') }}

- {{ t('ng_backup', 'The bin is empty.') }} + {{ t('ng_backup', 'The bin is empty.') }}