Keep leases alive during long restores and exports; PR review fixes
CI / unit (8.2) (push) Has been cancelled
CI / unit (8.3) (push) Has been cancelled
CI / unit (8.4) (push) Has been cancelled
CI / psalm (push) Has been cancelled
CI / integration (mariadb, 32) (push) Has been cancelled
CI / integration (mariadb, 33) (push) Has been cancelled
CI / integration (mariadb, 34) (push) Has been cancelled
CI / integration (mariadb, 35) (push) Has been cancelled
CI / integration (pgsql, 32) (push) Has been cancelled
CI / integration (pgsql, 33) (push) Has been cancelled
CI / integration (pgsql, 34) (push) Has been cancelled
CI / integration (pgsql, 35) (push) Has been cancelled
CI / integration (sqlite, 32) (push) Has been cancelled
CI / integration (sqlite, 33) (push) Has been cancelled
CI / integration (sqlite, 34) (push) Has been cancelled
CI / integration (sqlite, 35) (push) Has been cancelled
CI / integration-targets (push) Has been cancelled
CI / integration-user (32) (push) Has been cancelled
CI / integration-user (33) (push) Has been cancelled
CI / integration-user (34) (push) Has been cancelled
CI / integration-user (35) (push) Has been cancelled
CI / integration-full-restore (push) Has been cancelled
CI / unit (8.2) (push) Has been cancelled
CI / unit (8.3) (push) Has been cancelled
CI / unit (8.4) (push) Has been cancelled
CI / psalm (push) Has been cancelled
CI / integration (mariadb, 32) (push) Has been cancelled
CI / integration (mariadb, 33) (push) Has been cancelled
CI / integration (mariadb, 34) (push) Has been cancelled
CI / integration (mariadb, 35) (push) Has been cancelled
CI / integration (pgsql, 32) (push) Has been cancelled
CI / integration (pgsql, 33) (push) Has been cancelled
CI / integration (pgsql, 34) (push) Has been cancelled
CI / integration (pgsql, 35) (push) Has been cancelled
CI / integration (sqlite, 32) (push) Has been cancelled
CI / integration (sqlite, 33) (push) Has been cancelled
CI / integration (sqlite, 34) (push) Has been cancelled
CI / integration (sqlite, 35) (push) Has been cancelled
CI / integration-targets (push) Has been cancelled
CI / integration-user (32) (push) Has been cancelled
CI / integration-user (33) (push) Has been cancelled
CI / integration-user (34) (push) Has been cancelled
CI / integration-user (35) (push) Has been cancelled
CI / integration-full-restore (push) Has been cancelled
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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('<error>' . $t->getName() . ' (user exports): ' . $e->getMessage() . '</error>');
|
||||
$rc = 1;
|
||||
continue;
|
||||
$ru = ['forgotten' => []];
|
||||
}
|
||||
if ($ru['forgotten'] !== []) {
|
||||
$affected = count(array_unique(array_map(static fn (string $path): string => explode('/', $path)[1] ?? '', $ru['forgotten'])));
|
||||
|
||||
+4
-1
@@ -132,7 +132,7 @@ final class DbDumper {
|
||||
*
|
||||
* @param array{tables: array<string, array{blobs: list<string>, 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 !== []) {
|
||||
|
||||
@@ -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']) {
|
||||
|
||||
@@ -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<array{slot:int, label:string, created:int}>, 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(),
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -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, '/');
|
||||
}
|
||||
|
||||
@@ -414,24 +414,6 @@
|
||||
<ArgumentTypeCoercion>
|
||||
<code><![CDATA[array_diff(range(1, self::MAX_SLOTS), $used)]]></code>
|
||||
</ArgumentTypeCoercion>
|
||||
<InvalidReturnStatement>
|
||||
<code><![CDATA[[
|
||||
'version' => 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.',
|
||||
]]]></code>
|
||||
</InvalidReturnStatement>
|
||||
<InvalidReturnType>
|
||||
<code><![CDATA[array{version:int, app:string, fingerprint:string, created:string, wrapped_key:array, instructions:string}]]></code>
|
||||
</InvalidReturnType>
|
||||
<LessSpecificReturnStatement>
|
||||
<code><![CDATA[array_map(fn ($s) => ['slot' => $s['slot'], 'label' => $s['label'], 'created' => $s['created']], $this->wrappedKey()['slots'])]]></code>
|
||||
</LessSpecificReturnStatement>
|
||||
|
||||
@@ -0,0 +1,35 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
// SPDX-FileCopyrightText: 2026 Dillard Blom
|
||||
// SPDX-License-Identifier: AGPL-3.0-or-later
|
||||
|
||||
namespace OCA\NgBackup\Tests\Unit\Service;
|
||||
|
||||
use OCA\NgBackup\Service\LeaseService;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
|
||||
final class LeaseThrottleTest extends TestCase {
|
||||
public function testRefreshesOnlyOncePerInterval(): void {
|
||||
$calls = 0;
|
||||
$beat = LeaseService::throttled(function () use (&$calls): void {
|
||||
$calls++;
|
||||
}, 3600);
|
||||
for ($i = 0; $i < 1000; $i++) {
|
||||
$beat();
|
||||
}
|
||||
$this->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);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user