Commit full-backup snapshots to the catalog under the catalog lock

A full backup opened its repository at the start of a step and wrote the
catalog at the end of it, without the catalog lock. A per-user export or
restore that wrote the catalog in between could lose its entry, or the
snapshot could lose its own.

The final step now only stores the snapshot object and moves the run to a
new 'commit' phase. BackupService then records the snapshot under the
catalog lock, from a repository opened inside that lock; if the lock is
busy, the next step retries the commit. Repository::writeSnapshot() keeps
its old behaviour as putSnapshot() plus recordSnapshot().

lib/Command/BackupUser.php

@@ -51,8 +51,8 @@ HELP);
 			return 1;
 		}
 		try {
-			$this->installer->ensureEnabled($input, $output);
 			$target = $this->targets->get($input->getArgument('target'));
+			$this->installer->ensureEnabled($input, $output);
 			$manifest = $this->userBackup->backupUser($target, $user);
 		} catch (\Throwable $e) {
 			$output->writeln('<error>' . $e->getMessage() . '</error>');

lib/Command/KeyImport.php

@@ -58,7 +58,12 @@ HELP);
 		}
 		$file = $input->getOption('passphrase-file');
 		if ($file !== null) {
-			$pass = rtrim((string)@file_get_contents($file), "\r\n");
+			$raw = @file_get_contents($file);
+			if ($raw === false) {
+				$output->writeln('<error>Cannot read the passphrase file</error>');
+				return 1;
+			}
+			$pass = rtrim($raw, "\r\n");
 		} else {
 			$helper = $this->getHelper('question');
 			$q = (new Question('Passphrase of one of the kit\'s slots: '))->setHidden(true)->setHiddenFallback(false);

lib/Command/RestoreUser.php

@@ -48,9 +48,9 @@ HELP);
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		try {
-			$this->installer->ensureEnabled($input, $output);
 			$output->writeln('<comment>Note: on restore, user_migration only imports the app settings on its own allowlist (e.g. calendar view and reminder settings); other app settings of this user are skipped. Reason: NG Backup uses user_migration\'s importer; widening the list would mean building a settings importer of our own.</comment>');
 			$target = $this->targets->get($input->getArgument('target'));
+			$this->installer->ensureEnabled($input, $output);
 			$user = $this->userRestore->restoreUser($target, (string)$input->getArgument('manifest'), (string)$input->getOption('mode'));
 		} catch (\Throwable $e) {
 			$output->writeln('<error>' . $e->getMessage() . '</error>');

lib/Command/Retention.php

@@ -33,6 +33,10 @@ final class Retention extends Command {
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
+		if ($input->getOption('user-last') !== null && !ctype_digit((string)$input->getOption('user-last'))) {
+			$output->writeln('<error>--user-last must be a whole number</error>');
+			return 1;
+		}
 		$p = $this->prune->policy()->toArray();
 		$changed = false;
 		foreach (['last', 'daily', 'weekly', 'monthly'] as $k) {
@@ -50,13 +54,8 @@ final class Retention extends Command {
 			}
 		}
 		if ($input->getOption('user-last') !== null) {
-			$userLast = (string)$input->getOption('user-last');
-			if (!ctype_digit($userLast)) {
-				$output->writeln('<error>--user-last must be a whole number</error>');
-				return 1;
-			}
 			try {
-				$this->prune->setUserExportRetention((int)$userLast);
+				$this->prune->setUserExportRetention((int)$input->getOption('user-last'));
 			} catch (\InvalidArgumentException $e) {
 				$output->writeln('<error>' . $e->getMessage() . '</error>');
 				return 1;

lib/Job/BackupRun.php

@@ -133,7 +133,7 @@ final class BackupRun {
 
 		if ($state['phase'] === 'finish') {
 			$segments = $state['segments'];
-			$repo->writeSnapshot($state['snapshot'],
+			$repo->putSnapshot($state['snapshot'],
 				['time' => $state['started'], 'parent' => $state['parent'], 'label' => $state['label'],
 					'roots' => array_keys($state['roots']), 'stats' => $state['stats']] + $state['meta'],
 				(function () use ($repo, $segments) {
@@ -141,7 +141,7 @@ final class BackupRun {
 						yield from $repo->readLines($blobs);
 					}
 				})());
-			$state['phase'] = 'done';
+			$state['phase'] = 'commit';
 		}
 		return $state;
 	}

lib/Repository/Repository.php

@@ -288,6 +288,17 @@ final class Repository {
 	 * @param iterable<string> $lines
 	 */
 	public function writeSnapshot(string $snapshotId, array $meta, iterable $lines): void {
+		$this->putSnapshot($snapshotId, $meta, $lines);
+		$this->recordSnapshot($snapshotId);
+	}
+
+	/**
+	 * Store a snapshot object without touching the catalog. Use recordSnapshot() afterwards, from a
+	 * repository opened inside the catalog lock, so the catalog write builds on the latest generation.
+	 *
+	 * @param iterable<string> $lines
+	 */
+	public function putSnapshot(string $snapshotId, array $meta, iterable $lines): void {
 		$tree = fopen('php://temp/maxmemory:' . (4 * 1048576), 'w+b');
 		fwrite($tree, json_encode(['snapshot' => $snapshotId] + $meta, JSON_THROW_ON_ERROR) . "\n");
 		foreach ($lines as $line) {
@@ -301,6 +312,9 @@ final class Repository {
 		fclose($tree);
 		rewind($encrypted);
 		$this->backend->put('snapshots/' . $snapshotId, $encrypted);
+	}
+
+	public function recordSnapshot(string $snapshotId): void {
 		$this->catalog->snapshots[$snapshotId] = true;
 		$this->catalog->write();
 	}
@@ -422,9 +436,9 @@ final class Repository {
 		if (!isset($this->catalog->userExports[$manifestPath])) {
 			throw new RepositoryException("$manifestPath is not a recorded user export");
 		}
-		$this->backend->delete($manifestPath);
 		unset($this->catalog->userExports[$manifestPath]);
 		$this->catalog->write();
+		$this->backend->delete($manifestPath);
 	}
 
 	/** @return array<string, array{forgottenAt:int, by:string, db:?string, dbInTrash:?string, time:?string, label:string}> */
@@ -442,23 +456,26 @@ final class Repository {
 
 	/** Permanently delete trash entries older than $delaySeconds. @return list<string> purged snapshot ids */
 	public function purgeTrash(int $delaySeconds): array {
-		$purged = [];
+		$doomed = [];
 		foreach ($this->trash() as $id => $info) {
 			if (time() - $info['forgottenAt'] < $delaySeconds) {
 				continue;
 			}
+			$doomed[$id] = $info;
+			unset($this->catalog->trash[$id]);
+		}
+		if ($doomed === []) {
+			return [];
+		}
+		$this->catalog->write();
+		foreach ($doomed as $id => $info) {
 			if ($info['dbInTrash'] !== null) {
 				$this->backend->delete($info['dbInTrash']);
 			}
 			$this->backend->delete('trash/snapshots/' . $id);
 			$this->backend->delete('trash/info/' . $id);
-			unset($this->catalog->trash[$id]);
-			$purged[] = $id;
 		}
-		if ($purged !== []) {
-			$this->catalog->write();
-		}
-		return $purged;
+		return array_keys($doomed);
 	}
 
 	private function trashInfo(string $snapshotId): array {

lib/Service/BackupService.php

@@ -147,6 +147,17 @@ final class BackupService {
 				}
 			} else {
 				$state['files'] = BackupRun::step($repo, $state['files'], $deadline);
+				if ($state['files']['phase'] === 'commit') {
+					// Catalog write under the catalog lock, from a repository opened inside it: a per-user
+					// export or restore may have written the catalog since this step opened $repo.
+					try {
+						$this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 30,
+							fn () => $this->targets->repository($target)->recordSnapshot((string)$state['files']['snapshot']));
+						$state['files']['phase'] = 'done';
+					} catch (LockedException) {
+						// Another catalog write is in progress: the next step retries this commit.
+					}
+				}
 				if ($state['files']['phase'] === 'done') {
 					$this->recordSnapshot($run, $state['files']);
 					$this->alerts->storedBytes($target, $repo->storedBytes());

lib/Service/RestoreFullService.php

@@ -110,6 +110,7 @@ final class RestoreFullService {
 			// directory: once applied, 'secret' may differ from what ng_backup's own Target
 			// options were encrypted with (see restoreFull()'s docblock), and every further
 			// repository access through $this->targets would fail to decrypt them.
+			/** @var array<string, mixed> $backupConfig */
 			$backupConfig = $this->fetchBackupConfig($target, $snapshotId);
 
 			$dataDir = rtrim($this->config->getSystemValueString('datadirectory'), '/');

lib/Service/UserBackupService.php

@@ -39,10 +39,10 @@ final class UserBackupService {
 			$dest = new RepositoryExportDestination($repo, $user->getUID());
 			$service->export($dest, $user);
 			// A short exclusive lock around the catalog write only: the shared lock above
-			// allows another concurrent backup (full or per-user) to be reading the same
-			// catalog generation at the same time, and both writing the next one would let
-			// one silently overwrite the other's addition.
-			$this->withCatalogLock($target, fn () => $repo->recordUserExport($dest->manifestPath()));
+			// allows another concurrent backup (full or per-user) to be writing the catalog at
+			// the same time. The repository is reopened inside the lock, so the write builds on
+			// the latest catalog generation instead of the one loaded before the lock was taken.
+			$this->withCatalogLock($target, fn () => $this->targets->repository($target)->recordUserExport($dest->manifestPath()));
 			return $dest->manifestPath();
 		});
 	}

lib/Service/UserRestoreService.php

@@ -83,9 +83,9 @@ final class UserRestoreService {
 				$safety = new RepositoryExportDestination($repo, $originalUid);
 				$service->export($safety, $existing);
 				$safetyManifestPath = $safety->manifestPath();
-				// Same race as UserBackupService::backupUser(): serialize the catalog write
-				// itself against any other concurrent backup/restore for this target.
-				$this->withCatalogLock($target, fn () => $repo->recordUserExport($safetyManifestPath));
+				// Same race as UserBackupService::backupUser(): reopen the repository inside the
+				// catalog lock so the write builds on the latest generation.
+				$this->withCatalogLock($target, fn () => $this->targets->repository($target)->recordUserExport($safetyManifestPath));
 				if (!$existing->delete()) {
 					throw new \RuntimeException("Could not delete the existing account $originalUid before restoring; its safety export is at $safetyManifestPath");
 				}

lib/UserMigration/UserMigrationInstaller.php

@@ -7,7 +7,6 @@ declare(strict_types=1);
 
 namespace OCA\NgBackup\UserMigration;
 
-use OCA\NgBackup\AppInfo\Application;
 use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
 use Symfony\Component\Console\Input\InputInterface;
@@ -43,7 +42,8 @@ final class UserMigrationInstaller {
 	/** Installs (from the app store, if missing) and enables user_migration; throws on failure. */
 	public function install(): void {
 		$args = $this->codeIsPresent() ? ['app:enable', self::APP] : ['app:install', self::APP];
-		$occ = dirname($this->appManager->getAppPath(Application::APP_ID), 2) . '/occ';
+		/** @psalm-suppress UndefinedClass */
+		$occ = \OC::$SERVERROOT . '/occ';
 		$command = implode(' ', array_map('escapeshellarg', [PHP_BINARY, $occ, ...$args])) . ' 2>&1';
 		exec($command, $lines, $code);
 		if ($code !== 0) {
@@ -72,9 +72,6 @@ final class UserMigrationInstaller {
 		}
 		$output->writeln('Installing and enabling user_migration (approved by the administrator)...');
 		$this->install();
-		$this->appManager->clearAppsCache();
-		if (!$this->isEnabled()) {
-			throw new \RuntimeException('user_migration was installed; run this command again (check with "occ app:list" if it stays disabled)');
-		}
+		throw new \RuntimeException('user_migration is installed and enabled; run this command again');
 	}
 }

tests/Unit/Job/BackupRunTest.php

@@ -27,13 +27,14 @@ class BackupRunTest extends TestCase {
 
 		$state = BackupRun::start($src);
 		$steps = 0;
-		while ($state['phase'] !== 'done' && $steps < 500) {
+		while ($state['phase'] !== 'commit' && $steps < 500) {
 			// Fresh repository object per step, state through JSON: as separate processes would.
 			$repo = Repository::openWithKey($backend, $keys);
 			$state = json_decode(json_encode(BackupRun::step($repo, $state, microtime(true) + 0.05)), true);
 			$steps++;
 		}
-		$this->assertSame('done', $state['phase']);
+		$this->assertSame('commit', $state['phase']);
+		Repository::openWithKey($backend, $keys)->recordSnapshot($state['snapshot']);
 		$this->assertGreaterThan(2, $steps, 'the run was actually split into steps');
 
 		$target = $this->tempDir();
@@ -50,7 +51,7 @@ class BackupRunTest extends TestCase {
 
 		$state = BackupRun::start($src);
 		$mutated = false;
-		for ($i = 0; $i < 500 && $state['phase'] !== 'done'; $i++) {
+		for ($i = 0; $i < 500 && $state['phase'] !== 'commit'; $i++) {
 			$state = BackupRun::step(Repository::openWithKey($backend, $keys), $state, microtime(true) + 0.01);
 			if (!$mutated && $state['cur'] !== null && $state['cur']['offset'] > 0) {
 				$fh = fopen("$src/big.bin", 'r+b');
@@ -60,6 +61,8 @@ class BackupRunTest extends TestCase {
 				$mutated = true;
 			}
 		}
+		$this->assertSame('commit', $state['phase']);
+		Repository::openWithKey($backend, $keys)->recordSnapshot($state['snapshot']);
 		$this->assertTrue($mutated);
 		$this->assertGreaterThanOrEqual(1, $state['stats']['retried'] ?? 0);
 		$target = $this->tempDir();
@@ -78,14 +81,17 @@ class BackupRunTest extends TestCase {
 		Repository::initWithKey($backend, $keys, $keys->wrap('pw'));
 		$run = function (?string $parent) use ($backend, $src, $keys): array {
 			$state = BackupRun::start(['data' => $src], $parent);
-			for ($i = 0; $i < 2000 && $state['phase'] !== 'done'; $i++) {
+			for ($i = 0; $i < 2000 && $state['phase'] !== 'commit'; $i++) {
 				$state = json_decode(json_encode(BackupRun::step(Repository::openWithKey($backend, $keys), $state, microtime(true))), true);
 			}
+			if ($state['phase'] === 'commit') {
+				Repository::openWithKey($backend, $keys)->recordSnapshot($state['snapshot']);
+			}
 			return $state;
 		};
 		$first = $run(null);
 		$second = $run($first['snapshot']);
-		$this->assertSame('done', $second['phase']);
+		$this->assertSame('commit', $second['phase']);
 		$this->assertGreaterThan(150, $second["steps"], "one file per step");
 		$this->assertSame(0, $second['stats']['read'], 'merge-join with the parent across steps reuses every file');
 		$this->assertSame(200, $second['stats']['reused']);