From cc09d7494211d5d1ed65290785fc24a64e8cce2f Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 17:58:21 +0200 Subject: [PATCH] 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 | 4 ++-- lib/Repository/Repository.php | 14 ++++++++++++++ lib/Service/BackupService.php | 11 +++++++++++ tests/Unit/Job/BackupRunTest.php | 16 +++++++++++----- 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/lib/Job/BackupRun.php b/lib/Job/BackupRun.php index ca928b5..86ef1e5 100644 --- a/lib/Job/BackupRun.php +++ b/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; } diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 51e0999..5e51c51 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -288,6 +288,17 @@ final class Repository { * @param iterable $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 $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(); } diff --git a/lib/Service/BackupService.php b/lib/Service/BackupService.php index 3d2ae34..3649383 100644 --- a/lib/Service/BackupService.php +++ b/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()); diff --git a/tests/Unit/Job/BackupRunTest.php b/tests/Unit/Job/BackupRunTest.php index 9df3667..6ad73c4 100644 --- a/tests/Unit/Job/BackupRunTest.php +++ b/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']);