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

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

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