From 70965dd4bd9954558a66c993e088899a5d17b7be Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 22:38:47 +0200 Subject: [PATCH] Keep leases alive during long restores and exports; PR review fixes Full restore, raw directory restore, per-user export and the safety export held a fixed-TTL lease on the location that nothing refreshed. A restore or export running longer than that TTL let a cleanup take over and delete data still in use. They now refresh the lease while they work (per blob, per row batch, per written chunk), throttled to once a minute. Also: - backup:prune still applies snapshot retention when user-export retention fails for a location. - The recovery kit carries the deletion delay, so a disaster recovery keeps the original protection instead of falling back to 7 days. Older kits still import with the default. - CI checkouts no longer persist the token in the git config. - recoveryKit() has its real return type; the stale baseline entry is gone. --- .github/workflows/ci.yml | 12 +++++++ lib/Command/Prune.php | 3 +- lib/Db/DbDumper.php | 5 ++- lib/Repository/Repository.php | 5 ++- lib/Service/KeyService.php | 7 ++-- lib/Service/LeaseService.php | 11 ++++++ lib/Service/RestoreFullService.php | 12 +++---- lib/Service/RestoreService.php | 13 +++++-- lib/Service/UserBackupService.php | 9 ++--- lib/Service/UserRestoreService.php | 7 ++-- .../RepositoryExportDestination.php | 10 ++++++ psalm-baseline.xml | 18 ---------- tests/Unit/Service/LeaseThrottleTest.php | 35 +++++++++++++++++++ 13 files changed, 106 insertions(+), 41 deletions(-) create mode 100644 tests/Unit/Service/LeaseThrottleTest.php diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c37031..ebfa4a2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,6 +17,8 @@ jobs: php: ['8.2', '8.3', '8.4'] steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: shivammathur/setup-php@v2 with: php-version: ${{ matrix.php }} @@ -29,6 +31,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: shivammathur/setup-php@v2 with: php-version: '8.2' @@ -48,6 +52,8 @@ jobs: database: ['sqlite', 'mariadb', 'pgsql'] steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Round trip on Nextcloud ${{ matrix.nextcloud }} / ${{ matrix.database }} run: tests/integration/matrix.sh "${{ matrix.nextcloud }}" "${{ matrix.database }}" @@ -58,6 +64,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Round trip against S3 and SFTP run: tests/integration/targets-matrix.sh @@ -73,6 +81,8 @@ jobs: nextcloud: ['32', '33', '34', '35'] steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up a fresh Nextcloud instance run: | docker run -d --name ngbu -e SQLITE_DATABASE=nextcloud -e NEXTCLOUD_ADMIN_USER=admin \ @@ -90,5 +100,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - name: Full restore round trip run: tests/integration/full-restore-roundtrip.sh 35-apache diff --git a/lib/Command/Prune.php b/lib/Command/Prune.php index eee2a1e..c60ad20 100644 --- a/lib/Command/Prune.php +++ b/lib/Command/Prune.php @@ -43,9 +43,10 @@ final class Prune extends Command { try { $ru = $this->prune->applyUserExports($t, $dry); } catch (\Throwable $e) { + // Snapshot retention is independent of this: report and carry on with it. $output->writeln('' . $t->getName() . ' (user exports): ' . $e->getMessage() . ''); $rc = 1; - continue; + $ru = ['forgotten' => []]; } if ($ru['forgotten'] !== []) { $affected = count(array_unique(array_map(static fn (string $path): string => explode('/', $path)[1] ?? '', $ru['forgotten']))); diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index d1fb64c..a43ce98 100644 --- a/lib/Db/DbDumper.php +++ b/lib/Db/DbDumper.php @@ -132,7 +132,7 @@ final class DbDumper { * * @param array{tables: array, rows: int}>} $manifest */ - public function restore(Repository $repo, array $manifest, ?array &$report = null): int { + public function restore(Repository $repo, array $manifest, ?array &$report = null, ?callable $heartbeat = null): int { $sm = $this->db->createSchemaManager(); $existing = array_flip($sm->listTableNames()); $total = 0; @@ -175,6 +175,9 @@ final class DbDumper { if (count($batch) >= self::INSERT_ROWS) { $total += $this->insertBatch($table, $columns, $batch, $binary); $batch = []; + if ($heartbeat !== null) { + $heartbeat(); + } } } if ($batch !== []) { diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 5e51c51..5c6ccc3 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -198,7 +198,7 @@ final class Repository { * "config/" directly into the real data/config directory, not a subdirectory * named after the prefix). Ignored when $prefix is '' (nothing to strip). */ - public function restore(string $snapshotId, string $target, string $prefix = '', bool $stripPrefix = false): int { + public function restore(string $snapshotId, string $target, string $prefix = '', bool $stripPrefix = false, ?callable $heartbeat = null): int { if ($prefix !== '' && !self::isSafePath(rtrim($prefix, '/'))) { throw new RepositoryException('Invalid path prefix'); } @@ -227,6 +227,9 @@ final class Repository { } foreach ($entry['b'] as $id) { fwrite($out, $this->loadBlob($id)); + if ($heartbeat !== null) { + $heartbeat(); + } } fclose($out); if (filesize($tmp) !== $entry['s']) { diff --git a/lib/Service/KeyService.php b/lib/Service/KeyService.php index 9538c79..fca1d40 100644 --- a/lib/Service/KeyService.php +++ b/lib/Service/KeyService.php @@ -93,7 +93,9 @@ final class KeyService { $this->cached = $keys; $this->appConfig->setValueString(Application::APP_ID, self::KEY_WRAPPED, json_encode($kit['wrapped_key'], JSON_THROW_ON_ERROR), true, true); $this->appConfig->setValueString(Application::APP_ID, self::KEY_FINGERPRINT, $this->fingerprintOf($keys), true); - $this->appConfig->setValueInt(Application::APP_ID, self::KEY_DELETE_DELAY, self::DEFAULT_DELETE_DELAY, true); + // Kits from before this field existed fall back to the default. + $delay = (int)($kit['delete_delay_days'] ?? self::DEFAULT_DELETE_DELAY); + $this->appConfig->setValueInt(Application::APP_ID, self::KEY_DELETE_DELAY, ($delay >= 1 && $delay <= 365) ? $delay : self::DEFAULT_DELETE_DELAY, true); $this->appConfig->setValueInt(Application::APP_ID, self::KEY_KIT_VERSION, (int)($kit['kit_version'] ?? 1), true); // Having and successfully using a real kit file is stronger proof than the normal // download-and-confirm flow it replaces. @@ -260,13 +262,14 @@ final class KeyService { return $this->appConfig->getValueString(Application::APP_ID, self::KEY_FINGERPRINT, '', true); } - /** @return array{version:int, app:string, fingerprint:string, created:string, wrapped_key:array, instructions:string} */ + /** @return array{version:int, app:string, fingerprint:string, kit_version:int, delete_delay_days:int, confirmation_code:string, created:string, wrapped_key:array, slots:list, instructions:string} */ public function recoveryKit(): array { return [ 'version' => 1, 'app' => 'NG Backup', 'fingerprint' => $this->fingerprint(), 'kit_version' => $this->kitVersion(), + 'delete_delay_days' => $this->deleteDelayDays(), 'confirmation_code' => $this->kitCode(), 'created' => gmdate('c'), 'wrapped_key' => $this->wrappedKey(), diff --git a/lib/Service/LeaseService.php b/lib/Service/LeaseService.php index 874dbcb..43437af 100644 --- a/lib/Service/LeaseService.php +++ b/lib/Service/LeaseService.php @@ -76,6 +76,17 @@ final class LeaseService { $qb->executeStatement(); } + /** Wraps $refresh so calling it often (per file, per row batch) only touches the database every $seconds. */ + public static function throttled(callable $refresh, int $seconds = 60): \Closure { + $last = time(); + return static function () use ($refresh, $seconds, &$last): void { + if (time() - $last >= $seconds) { + $refresh(); + $last = time(); + } + }; + } + /** Run $fn under a lease; $refresh() can be called during long work. */ public function with(string $name, string $mode, int $ttl, callable $fn): mixed { $id = $this->acquire($name, $mode, $ttl); diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index fdcf754..7269caf 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -70,7 +70,8 @@ final class RestoreFullService { if (!$check['ok']) { throw new \RuntimeException($this->describeBlockers($check)); } - return $this->withSharedLock($target, function () use ($target, $snapshotId) { + return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId) { + $beat = LeaseService::throttled($refresh); $repo = $this->targets->repository($target); $meta = $repo->snapshotMeta($snapshotId); @@ -100,7 +101,7 @@ final class RestoreFullService { $ownAppConfig = $this->preserveOwnAppConfig(); $dumper = new DbDumper($this->db->getInner(), $prefix); try { - $dumper->restore($repo, $manifest, $dbReport); + $dumper->restore($repo, $manifest, $dbReport, $beat); } finally { $this->restoreOwnAppConfig($ownAppConfig); } @@ -114,7 +115,7 @@ final class RestoreFullService { $backupConfig = $this->fetchBackupConfig($target, $snapshotId); $dataDir = rtrim($this->config->getSystemValueString('datadirectory'), '/'); - $dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $dataDir, stripPrefix: true); + $dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $dataDir, stripPrefix: true, heartbeat: $beat); // Last step: nothing after this may need $this->targets->repository($target) again. if ($backupConfig !== []) { @@ -215,9 +216,8 @@ final class RestoreFullService { private function withSharedLock(Target $target, callable $fn): mixed { try { - // No progress from DbDumper::restore()/Repository::restore() to refresh this lease - // against; a very large instance could in theory outrun this TTL (same limitation as - // RestoreService::restoreToDirectory(), see also UserBackupService). + // Refreshed during the database and data restore (per row batch, per blob), so a + // cleanup cannot take over the location while a long restore is still reading it. return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 7200, $fn); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); diff --git a/lib/Service/RestoreService.php b/lib/Service/RestoreService.php index d45d7f4..20bee68 100644 --- a/lib/Service/RestoreService.php +++ b/lib/Service/RestoreService.php @@ -156,14 +156,23 @@ final class RestoreService { * Repository::restore()); used to restore "data/" or "config/" straight into the * real data/config directory for occ backup:restore:full. */ - public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory, bool $stripPrefix = false): int { + public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory, bool $stripPrefix = false, ?callable $heartbeat = null): int { if ($prefix !== '' && !Repository::isSafePath($prefix)) { throw new \InvalidArgumentException('Invalid path: ' . $prefix); } if (!is_dir($directory) && !mkdir($directory, 0750, true) && !is_dir($directory)) { throw new \RuntimeException("Cannot create $directory"); } - return $this->withSharedLock($target, fn () => $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/', $stripPrefix), 3600); + return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId, $prefix, $directory, $stripPrefix, $heartbeat): int { + $own = LeaseService::throttled($refresh); + $beat = static function () use ($own, $heartbeat): void { + $own(); + if ($heartbeat !== null) { + $heartbeat(); + } + }; + return $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/', $stripPrefix, $beat); + }, 3600); } private function writeFile(Repository $repo, Folder $dest, string $relPath, array $entry): void { diff --git a/lib/Service/UserBackupService.php b/lib/Service/UserBackupService.php index 23aac20..4c5bf5c 100644 --- a/lib/Service/UserBackupService.php +++ b/lib/Service/UserBackupService.php @@ -34,9 +34,9 @@ final class UserBackupService { /** @throws UserMigrationException when user_migration is not enabled, or the export itself fails */ public function backupUser(Target $target, IUser $user): string { $service = $this->locator->get(); - return $this->withSharedLock($target, function () use ($target, $user, $service) { + return $this->withSharedLock($target, function (callable $refresh) use ($target, $user, $service) { $repo = $this->targets->repository($target); - $dest = new RepositoryExportDestination($repo, $user->getUID()); + $dest = new RepositoryExportDestination($repo, $user->getUID(), LeaseService::throttled($refresh)); $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 writing the catalog at @@ -49,10 +49,7 @@ final class UserBackupService { private function withSharedLock(Target $target, callable $fn): mixed { try { - // UserMigrationService::export() exposes no progress, so unlike - // RestoreService::restoreUserFiles() this lease cannot be refreshed mid-call; a very - // large account could in theory outrun this TTL. Resumable, step-based per-user - // backup (like the main BackupService run) is a possible future improvement. + // Refreshed by RepositoryExportDestination as it writes data, so a long export keeps it. return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 3600, $fn); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); diff --git a/lib/Service/UserRestoreService.php b/lib/Service/UserRestoreService.php index 56e61e4..d4877e1 100644 --- a/lib/Service/UserRestoreService.php +++ b/lib/Service/UserRestoreService.php @@ -45,7 +45,7 @@ final class UserRestoreService { /** @throws UserMigrationException when user_migration is not enabled, or the restore itself fails */ public function restoreUser(Target $target, string $manifestPath, string $mode = self::MODE_REPLACE): IUser { $service = $this->locator->get(); - return $this->withSharedLock($target, function () use ($target, $manifestPath, $mode, $service): IUser { + return $this->withSharedLock($target, function (callable $refresh) use ($target, $manifestPath, $mode, $service): IUser { $repo = $this->targets->repository($target); // Only ever read a manifest the verified catalog actually recorded: a path alone // (even one this app generated before) proves nothing about it still being current @@ -80,7 +80,7 @@ final class UserRestoreService { if ($existing !== null) { // Never import over a live account: export its current state first, so nothing // is lost if the restore turns out to be the wrong choice. - $safety = new RepositoryExportDestination($repo, $originalUid); + $safety = new RepositoryExportDestination($repo, $originalUid, LeaseService::throttled($refresh)); $service->export($safety, $existing); $safetyManifestPath = $safety->manifestPath(); // Same race as UserBackupService::backupUser(): reopen the repository inside the @@ -119,8 +119,7 @@ final class UserRestoreService { private function withSharedLock(Target $target, callable $fn): mixed { try { - // See UserBackupService::withSharedLock(): UserMigrationService exposes no progress, - // so this lease cannot be refreshed mid-call. + // Refreshed while the safety export writes data (see UserBackupService::withSharedLock()). return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, 3600, $fn); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); diff --git a/lib/UserMigration/RepositoryExportDestination.php b/lib/UserMigration/RepositoryExportDestination.php index 8eec0d1..ce78995 100644 --- a/lib/UserMigration/RepositoryExportDestination.php +++ b/lib/UserMigration/RepositoryExportDestination.php @@ -28,6 +28,8 @@ final class RepositoryExportDestination implements IExportDestination { public function __construct( private Repository $repo, private string $uid, + /** Called while data is written (per chunk), to keep the location lease alive. */ + private ?\Closure $heartbeat = null, ) { $this->exportId = gmdate('Ymd\THis\Z') . '-' . bin2hex(random_bytes(4)); } @@ -36,6 +38,7 @@ final class RepositoryExportDestination implements IExportDestination { $writer = $this->repo->blobWriter($this->stats); $writer->write($content); $this->entries[self::norm($path)] = ['t' => 'f', 'b' => $writer->finish(), 's' => strlen($content)]; + $this->beat(); } public function addFileAsStream(string $path, $stream): void { @@ -46,6 +49,7 @@ final class RepositoryExportDestination implements IExportDestination { throw new UserMigrationException('Read error while exporting ' . $path); } $writer->write($chunk); + $this->beat(); } $this->entries[self::norm($path)] = ['t' => 'f', 'b' => $writer->finish(), 's' => $writer->bytes()]; } @@ -92,6 +96,12 @@ final class RepositoryExportDestination implements IExportDestination { return $this->stats; } + private function beat(): void { + if ($this->heartbeat !== null) { + ($this->heartbeat)(); + } + } + private static function norm(string $path): string { return trim($path, '/'); } diff --git a/psalm-baseline.xml b/psalm-baseline.xml index d874a89..ab61ac7 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -414,24 +414,6 @@ - - 1, - 'app' => 'NG Backup', - 'fingerprint' => $this->fingerprint(), - 'kit_version' => $this->kitVersion(), - 'confirmation_code' => $this->kitCode(), - 'created' => gmdate('c'), - 'wrapped_key' => $this->wrappedKey(), - 'slots' => $this->slots(), - 'instructions' => 'Keep this file safe, outside this server. Any one of the passphrases of the slots listed here opens it. ' - . 'To restore on a new server: install NG Backup, add the backup location and enter this kit with one passphrase. ' - . 'Without the kit (or the location) and a passphrase, the backups cannot be decrypted.', - ]]]> - - - - ['slot' => $s['slot'], 'label' => $s['label'], 'created' => $s['created']], $this->wrappedKey()['slots'])]]> diff --git a/tests/Unit/Service/LeaseThrottleTest.php b/tests/Unit/Service/LeaseThrottleTest.php new file mode 100644 index 0000000..a4ef4c0 --- /dev/null +++ b/tests/Unit/Service/LeaseThrottleTest.php @@ -0,0 +1,35 @@ +assertSame(0, $calls, 'no refresh before the interval has passed'); + } + + public function testRefreshesEveryCallWithZeroInterval(): void { + $calls = 0; + $beat = LeaseService::throttled(function () use (&$calls): void { + $calls++; + }, 0); + for ($i = 0; $i < 5; $i++) { + $beat(); + } + $this->assertSame(5, $calls); + } +}