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

@@ -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.

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

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) {

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;

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<array{0: string, 1: int}> */
 		return $dirs;
 	}
 

lib/Service/RestoreFullService.php

@@ -209,6 +209,8 @@ final class RestoreFullService {
 			try {
 				/** @var array<string, mixed> $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;

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

src/components/Bin.vue

@@ -17,7 +17,7 @@
 			</template>
 			<p class="ngb-muted">{{ t('ng_backup', 'This can take a while: every removed restore point is read from the location.') }}</p>
 		</div>
-		<NcNoteCard v-else-if="empty" type="info">{{ t('ng_backup', 'The bin is empty.') }}</NcNoteCard>
+		<NcNoteCard v-else-if="empty && !error" type="info">{{ t('ng_backup', 'The bin is empty.') }}</NcNoteCard>
 
 		<template v-for="target in status.targets" :key="'trash' + target.id">
 			<div v-if="trash[target.id]?.length" class="ngb-trash">

tests/Unit/Repository/EmptyDirsTest.php

@@ -71,6 +71,32 @@ class EmptyDirsTest extends TestCase {
 		$this->assertSame(array_unique($paths), $paths);
 	}
 
+	public function testANameASnapshotCannotHoldIsReportedAndTheSnapshotStaysReadable(): void {
+		mkdir("$this->src/data/odd\\folder", 0700, true);
+		mkdir("$this->src/data/alice/files/line\nbreak", 0700, true);
+		file_put_contents("$this->src/data/alice/files/back\\slash.txt", 'x');
+		file_put_contents("$this->src/data/alice/files/\xff.txt", 'x');
+		$state = BackupRun::start(['data' => "$this->src/data"]);
+		for ($i = 0; $i < 50 && $state['phase'] !== 'commit'; $i++) {
+			$state = BackupRun::step($this->repo(), $state, microtime(true) + 30);
+		}
+		$this->assertSame('commit', $state['phase']);
+		$this->assertSame(4, $state['stats']['unreadable']);
+		$this->repo()->recordSnapshot($state['snapshot']);
+
+		$this->assertSame(['data/alice/files/Hollow/Nested', 'data/skipped/inner'], array_column($this->repo()->emptyDirs($state['snapshot']), 0));
+		$target = $this->tempDir();
+		$this->assertSame(1, $this->repo()->restore($state['snapshot'], $target));
+		$this->assertFileExists("$target/data/alice/files/Docs/note.txt");
+	}
+
+	public function testAnEmptyFolderThatIsThePrefixItselfIsTheTarget(): void {
+		$snapshot = $this->backup();
+		$target = $this->tempDir();
+		$this->repo()->restore($snapshot, $target, 'data/skipped/inner/', true);
+		$this->assertSame([], glob("$target/*"), 'not a folder "inner" inside the target');
+	}
+
 	public function testRestoreBringsThemBackWithTheirTime(): void {
 		$snapshot = $this->backup();
 		$target = $this->tempDir();