diff --git a/CHANGELOG.md b/CHANGELOG.md index 10dbe8d..5f37627 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,23 @@ instead carried by the app description, the git tag, the release, and this file. - The list of backup runs (web interface and `occ backup:status`) shows the total size of each backup next to what it uploaded: an incremental backup of 1 TB that uploads 800 MB now shows both. `occ backup:status` shows sizes in GiB where that reads better than MiB. +- `occ backup:cancel ` stops a running backup; `occ backup:status` shows the run numbers. + No new step of the run starts, the step in progress finishes (usually within a minute), then + the process working on the run (`occ backup:run` or cron) stops. What it already stored is + removed by the next cleanup. A backup started by an earlier version is stopped as well: after + two minutes it is marked cancelled while it works, and its process stops at the end of the + step. + +### Fixed +- Backups to SMB locations, and to other slow locations with many packs, slowed to a crawl: each + 30-second step opened the location again, and reading the list of index files took longer + than the step itself. Each step then stored a single 4 MiB pack, which made the list longer + still. A first full backup of 0.8 TB to a Hetzner Storage Box over SMB managed 12 GiB in 35 + hours. `occ backup:run` and system cron now open the location once per run, opening no longer + counts towards the step's time, and SMB folders are listed in one request instead of one per + file. A backup to an SMB location that is not getting anywhere keeps running in its old + process after the update: update first, then stop it with `occ backup:cancel ` and start + it again. The new backup reuses the packs the old one stored. ## [0.9.5-beta.6] - 2026-10-12 diff --git a/README.md b/README.md index 3450db9..9bbb986 100644 --- a/README.md +++ b/README.md @@ -195,6 +195,7 @@ On the backlog; none of these block the beta or 1.0.0, but know them before you | `backup:key:slots`, `backup:key:slot:add/replace/remove` | passphrase slots | | `backup:target:backends/add/edit/list/remove/append-only` | backup locations | | `backup:run`, `backup:status`, `backup:list` | back up, status, restore points | +| `backup:cancel ` | stop a running backup | | `backup:browse`, `backup:restore` | browse and restore | | `backup:retention`, `backup:prune`, `backup:trash`, `backup:trash:restore` | retention, cleanup, trash | | `backup:verify` | checksum audit of a restore point (`--deep` downloads and decrypts everything) | diff --git a/appinfo/info.xml b/appinfo/info.xml index 83fc64f..2d54006 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -41,6 +41,7 @@ See the README for known limitations and the recovery kit before relying on it.] OCA\NgBackup\Command\TargetList OCA\NgBackup\Command\TargetRemove OCA\NgBackup\Command\RunBackup + OCA\NgBackup\Command\CancelRun OCA\NgBackup\Command\Status OCA\NgBackup\Command\ListSnapshots OCA\NgBackup\Command\Browse diff --git a/docs/HOWTO.md b/docs/HOWTO.md index 197ffdc..4ad750f 100644 --- a/docs/HOWTO.md +++ b/docs/HOWTO.md @@ -50,6 +50,14 @@ occ backup:target:list occ backup:status ``` +A backup that hangs or takes far too long can be stopped. `backup:status` shows the run number; +the command waits until the step in progress is finished, then the process working on the run +stops, and the next cleanup removes what it stored: + +``` +occ backup:cancel 7 +``` + Changed password, new host name, or moved the repository? Edit the location instead of adding a new one, so its history stays (in the web interface: **Edit** next to the location). The new settings must lead to the same repository; options you do not pass keep their value: diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index 2ad83c4..427b624 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -223,12 +223,23 @@ final class StorageBackend implements IBackend { if (!$this->storage->is_dir($abs)) { return []; // not created yet } + $cut = $this->base === '' ? 0 : strlen($this->base) + 1; + $names = $this->smbNames($abs); + if ($names !== null) { + $result = []; + foreach ($names as $name) { + if (!self::isTemporary($name)) { + $result[] = substr($abs . '/' . $name, $cut); + } + } + sort($result); + return $result; + } $dh = $this->storage->opendir($abs); if ($dh === false) { // An existing folder read as empty would look like a new repository. throw new BackendException('Cannot list ' . $dir); } - $cut = $this->base === '' ? 0 : strlen($this->base) + 1; $result = []; while (($name = readdir($dh)) !== false) { if ($name !== '.' && $name !== '..' && !self::isTemporary($name)) { @@ -240,6 +251,46 @@ final class StorageBackend implements IBackend { return $result; } + /** + * The names in an SMB folder, read in one listing; null for other storages, with access + * control lists on, or when the storage's internals are not as expected. The SMB storage's + * opendir() asks the server for the attributes of each entry to leave out hidden ones: with + * thousands of index files that took minutes. Hidden entries are still left out: names + * starting with a dot, and, after asking, any name a repository does not use (letters, + * digits, - and _), such as desktop.ini or Thumbs.db. The repository's own files have such + * names: they are not checked one by one, which was the cost. + * + * @return list|null + */ + private function smbNames(string $abs): ?array { + if (!$this->storage instanceof \OCA\Files_External\Lib\Storage\SMB) { + return null; + } + try { + /** @var list|null */ + return (function (string $path): ?array { + /** @psalm-suppress UndefinedThisPropertyFetch */ + if ($this->checkAcl) { + return null; + } + $names = []; + /** @psalm-suppress UndefinedThisPropertyFetch, UndefinedMethod */ + foreach ($this->share->dir(ltrim($this->buildPath($path), '/')) as $info) { + $name = (string)$info->getName(); + /** @psalm-suppress UndefinedThisPropertyFetch */ + if (!$this->showHidden && (str_starts_with($name, '.') + || (preg_match('/^[A-Za-z0-9_-]+$/', $name) !== 1 && $info->isHidden()))) { + continue; + } + $names[] = $name; + } + return $names; + })->call($this->storage, $abs); + } catch (\Throwable) { + return null; // another SMB storage version, or an error: the usual way reports it + } + } + /** * Run $op; when it throws, or $failed says its result may come from a dead connection, and * the SFTP connection turns out to be dead, run it once more on a new connection. diff --git a/lib/Command/CancelRun.php b/lib/Command/CancelRun.php new file mode 100644 index 0000000..095ed64 --- /dev/null +++ b/lib/Command/CancelRun.php @@ -0,0 +1,74 @@ +setName('backup:cancel') + ->setDescription('Stop a running backup') + ->addArgument('run', InputArgument::REQUIRED, 'Run number, as shown by backup:status') + ->setHelp("No new step of the run starts; the step in progress finishes first (usually within a +" + . "minute), so a backup that is just being completed is not cut off. The process working on the +" + . "run (occ backup:run, or cron) then stops. A backup started by an earlier version is marked +" + . "cancelled after two minutes and stops at the end of its step. What it already stored is +" + . "removed by the next cleanup."); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $id = (string)$input->getArgument('run'); + if (!ctype_digit($id)) { + $output->writeln('Give the run number, as shown by backup:status.'); + return 1; + } + try { + $run = $this->runs->find((int)$id); + } catch (DoesNotExistException) { + $output->writeln("There is no run $id."); + return 1; + } + if ($run->getKind() !== BackupService::KIND_FULL) { + $output->writeln("Run $id is not a backup; only backups can be cancelled."); + return 1; + } + if ($run->getStatus() === Run::RUNNING) { + $output->writeln("Waiting for the current step of run $id to finish..."); + } + try { + $run = $this->backups->cancel($run); + } catch (\RuntimeException $e) { + $output->writeln('' . $e->getMessage() . ''); + return 1; + } + if ($run->getStatus() !== Run::CANCELLED) { + $output->writeln("Run $id is not running (status: {$run->getStatus()}); nothing to cancel."); + return 0; + } + $output->writeln("Run $id is cancelled."); + return 0; + } +} diff --git a/lib/Service/BackupService.php b/lib/Service/BackupService.php index 094e764..a5fef3b 100644 --- a/lib/Service/BackupService.php +++ b/lib/Service/BackupService.php @@ -14,6 +14,7 @@ use OCA\NgBackup\Db\Snapshot; use OCA\NgBackup\Db\SnapshotMapper; use OCA\NgBackup\Db\Target; use OCA\NgBackup\Job\BackupRun; +use OCA\NgBackup\Repository\Repository; use OCA\NgBackup\Repository\RepositoryException; use OCP\AppFramework\Db\DoesNotExistException; use OCP\App\IAppManager; @@ -34,6 +35,9 @@ use Psr\Log\LoggerInterface; final class BackupService { public const KIND_FULL = 'full'; + /** @var \WeakMap the location settings each kept repository was opened with */ + private \WeakMap $openedWith; + public function __construct( private TargetService $targets, private KeyService $keys, @@ -47,6 +51,9 @@ final class BackupService { private AlertService $alerts, private LeaseService $leases, ) { + /** @var \WeakMap $opened */ + $opened = new \WeakMap(); + $this->openedWith = $opened; } public function start(Target $target, string $label = ''): Run { @@ -144,13 +151,22 @@ final class BackupService { /** * Do one step of a run until $deadline. Safe to call concurrently: a second caller finds the * run locked and returns immediately. + * + * @param Repository|null $repo the location's repository, opened by an earlier step of this run + * in this process (see runToCompletion()); set when this step opens it */ - public function step(Run $run, float $deadline): Run { + public function step(Run $run, float $deadline, ?Repository &$repo = null): Run { // Leases expire on their own: a step killed by a time limit or OOM blocks nothing for long. // A step can run well past its deadline (the commit of a large snapshot writes its tree and // index), so the lease holds at least 15 minutes; should it still expire and another process // take the run over, the checks below keep the two from overwriting each other. $ttl = (int)min(3600, max(900, $deadline - microtime(true) + 600)); + // No new step once a cancel has begun: it waits for the step in progress, if any. + try { + $this->leases->release($this->leases->acquire(self::cancelKey($run->getId()), LeaseService::SHARED, 60)); + } catch (LockedException) { + return $run; + } try { $runLease = $this->leases->acquire('run/' . $run->getId(), LeaseService::EXCLUSIVE, $ttl); } catch (LockedException) { @@ -164,6 +180,7 @@ final class BackupService { return $run; } $loadedAt = null; + $savedBefore = $run->getUpdatedAt(); try { $run = $this->runs->find($run->getId()); // fresh state after acquiring the lock if ($run->getStatus() !== Run::RUNNING || $run->getKind() !== self::KIND_FULL) { @@ -171,7 +188,19 @@ final class BackupService { } $loadedAt = $run->getUpdatedAt(); $target = $this->targets->get((string)$run->getTargetId()); - $repo = $this->targets->repository($target); + // Reopen when another process took a step since ours (its packs are not in our copy of + // the index) or the location was edited (password, folder, size limit). + $settings = json_encode([$target->getBackend(), $target->getAuth(), $target->getOptions(), + $target->getBasePath(), $target->getRepositoryId(), $target->getMaxBytes()], JSON_THROW_ON_ERROR); + if ($repo === null || $loadedAt !== $savedBefore || ($this->openedWith[$repo] ?? null) !== $settings) { + // Opening reads the index listing, which takes minutes on a slow location with many + // packs: that time does not count, or each step would store one blob and stop. + $opening = microtime(true); + $repo = null; + $repo = $this->targets->repository($target); + $this->openedWith[$repo] = $settings; + $deadline += microtime(true) - $opening; + } $state = json_decode($run->getState(), true, 512, JSON_THROW_ON_ERROR); if ($run->getPhase() === 'db') { @@ -234,8 +263,14 @@ final class BackupService { $run->setStats(json_encode(['db' => $state['dbStats'] ?? null, 'files' => $state['files']['stats']], JSON_THROW_ON_ERROR)); // Another process may have taken the run over after this step's lease expired: then its // writes stand and this step's are dropped. - return $this->runs->updateIfUnchanged($run, $loadedAt) ?? $this->runs->find($run->getId()); + $saved = $this->runs->updateIfUnchanged($run, $loadedAt); + if ($saved === null) { + $repo = null; // the run is another process's now: its packs are not in our index + return $this->runs->find($run->getId()); + } + return $saved; } catch (\Throwable $e) { + $repo = null; // may hold blobs of a half-written pack: the next step opens it afresh if ($loadedAt === null) { // The run could not even be loaded: there is nothing fresh to mark as failed, and // the caller's copy may be stale. @@ -266,14 +301,16 @@ final class BackupService { /** Run to completion in this process (occ, system cron), reporting progress. */ public function runToCompletion(Run $run, ?callable $progress = null, float $stepSeconds = 30): Run { + $repo = null; // opened once, by the first step, and kept for the next ones while ($run->getStatus() === Run::RUNNING) { $before = $run->getUpdatedAt(); - $run = $this->step($run, microtime(true) + $stepSeconds); + $run = $this->step($run, microtime(true) + $stepSeconds, $repo); if ($run->getStatus() === Run::RUNNING && $run->getUpdatedAt() === $before) { // Step was not possible (another process holds the run, or a crashed step's lease has // not expired yet): wait instead of spinning. sleep(5); $run = $this->runs->find($run->getId()); + $repo = null; // another process may have stored packs in the meantime } if ($progress !== null) { $progress($run); @@ -282,11 +319,70 @@ final class BackupService { return $run; } - public function cancel(Run $run): Run { - $run->setStatus(Run::CANCELLED); - $run->setFinishedAt(time()); - $run->setUpdatedAt(time()); - return $this->runs->update($run); + /** + * Stop a running backup. First no new step may start, then this waits (up to $wait seconds) + * for the step in progress, so a step that is recording the snapshot finishes first. A process + * running the backup then finds it cancelled and stops. The packs it stored stay until the + * next cleanup. + * + * A process of an earlier version does not hold back its next step and may never leave a gap: + * after $wait the run is marked cancelled while it works, unless it is recording the snapshot. + * Its step then fails to save and the process stops. + */ + public function cancel(Run $run, int $wait = 120): Run { + $id = $run->getId(); + $until = time() + $wait; + $hold = null; + for ($try = 0; $hold === null; $try++) { + try { + $hold = $this->leases->acquire(self::cancelKey($id), LeaseService::EXCLUSIVE, $wait + 300); + } catch (LockedException) { + if ($try >= 20) { + throw new \RuntimeException("Run $id is being cancelled already"); + } + usleep(100000); // a step checking whether it may start + } + } + try { + $lease = null; + while ($lease === null && time() < $until) { + try { + $lease = $this->leases->acquire('run/' . $id, LeaseService::EXCLUSIVE, 60); + } catch (LockedException) { + sleep(2); + } + } + try { + for ($try = 0; $try < 10; $try++) { + $run = $this->runs->find($id); + if ($run->getStatus() !== Run::RUNNING) { + return $run; + } + $state = json_decode($run->getState(), true) ?: []; + if ($lease === null && ($state['files']['phase'] ?? '') === 'commit') { + throw new \RuntimeException("Run $id is recording its snapshot; try again in a minute"); + } + $loadedAt = $run->getUpdatedAt(); + $run->setStatus(Run::CANCELLED); + $run->setFinishedAt(time()); + if (($cancelled = $this->runs->updateIfUnchanged($run, $loadedAt)) !== null) { + return $cancelled; + } + usleep(200000); // a step of another process saved just now + } + throw new \RuntimeException("Run $id keeps changing; try again"); + } finally { + if ($lease !== null) { + $this->leases->release($lease); + } + } + } finally { + $this->leases->release($hold); + } + } + + private static function cancelKey(int $runId): string { + return 'run/' . $runId . '/cancel'; } private function recordSnapshot(Run $run, array $files): void { diff --git a/tests/Unit/Backend/StorageBackendListTest.php b/tests/Unit/Backend/StorageBackendListTest.php index da2b93e..35b3b00 100644 --- a/tests/Unit/Backend/StorageBackendListTest.php +++ b/tests/Unit/Backend/StorageBackendListTest.php @@ -10,6 +10,8 @@ use OCA\NgBackup\Tests\Unit\TempDirTrait; use OCP\Files\Storage\IStorage; use PHPUnit\Framework\TestCase; +require_once __DIR__ . '/../../stubs/SmbStorage.php'; + /** On a remote storage every is_dir() is a round trip: listing index/ must not cost one per pack. */ class StorageBackendListTest extends TestCase { use TempDirTrait; @@ -54,4 +56,58 @@ class StorageBackendListTest extends TestCase { $this->expectException(BackendException::class); (new StorageBackend($storage, 'repo'))->listFiles('catalog'); } + public function testSmbFoldersAreListedInOneRequest(): void { + $calls = new \ArrayObject(); + $share = new class($calls) { + public function __construct(private \ArrayObject $calls) { + } + + public function dir(string $path): array { + $this->calls[] = "dir $path"; + return array_map(fn (string $n) => new class($n, $this->calls) { + public function __construct(private string $n, private \ArrayObject $calls) { + } + + public function getName(): string { + return $this->n; + } + + public function isHidden(): bool { + $this->calls[] = "attributes {$this->n}"; + return $this->n === 'desktop.ini'; + } + }, ['b1', 'a2', '.hidden', 'desktop.ini', 'c3.part-0123abcd']); + } + }; + $storage = $this->getMockBuilder(\OCA\Files_External\Lib\Storage\SMB::class)->disableOriginalConstructor()->getMock(); + (fn () => $this->share = $share)->call($storage); + $storage->method('instanceOfStorage')->willReturn(false); + $storage->method('is_dir')->willReturn(true); + $storage->expects($this->never())->method('opendir'); + + $this->assertSame(['index/a2', 'index/b1'], (new StorageBackend($storage, 'repo'))->listFiles('index')); + $this->assertSame(['dir repo/index', 'attributes desktop.ini', 'attributes c3.part-0123abcd'], $calls->getArrayCopy(), + 'only names a repository does not use cost a request'); + } + + public function testSmbWithAccessControlListsIsListedTheUsualWay(): void { + $dir = $this->tempDir(); + touch("$dir/a1"); + $storage = $this->getMockBuilder(\OCA\Files_External\Lib\Storage\SMB::class)->disableOriginalConstructor()->getMock(); + (fn () => $this->checkAcl = true)->call($storage); + $storage->method('instanceOfStorage')->willReturn(false); + $storage->method('is_dir')->willReturn(true); + $storage->expects($this->once())->method('opendir')->willReturnCallback(fn () => opendir($dir)); + $this->assertSame(['index/a1'], (new StorageBackend($storage, 'repo'))->listFiles('index')); + } + + public function testSmbFallsBackToTheUsualListing(): void { + $dir = $this->tempDir(); + touch("$dir/a1"); + $storage = $this->getMockBuilder(\OCA\Files_External\Lib\Storage\SMB::class)->disableOriginalConstructor()->getMock(); // no share + $storage->method('instanceOfStorage')->willReturn(false); + $storage->method('is_dir')->willReturn(true); + $storage->method('opendir')->willReturnCallback(fn () => opendir($dir)); + $this->assertSame(['index/a1'], (new StorageBackend($storage, 'repo'))->listFiles('index')); + } } diff --git a/tests/stubs/SmbStorage.php b/tests/stubs/SmbStorage.php new file mode 100644 index 0000000..720199b --- /dev/null +++ b/tests/stubs/SmbStorage.php @@ -0,0 +1,22 @@ +