From c37b26ea38639094a7988adcb98fd1eec2facca6 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 21:53:05 +0200 Subject: [PATCH 01/19] Full restore: an empty listing never empties the index; read a failed pack once more; recognise and replace SFTP connections a Storage Box dropped quietly; version 0.9.11 --- CHANGELOG.md | 16 ++ appinfo/info.xml | 8 +- lib/Backend/SftpRangeFetcher.php | 33 +++- lib/Backend/StorageBackend.php | 21 ++- lib/Repository/BlobIndex.php | 39 +++++ lib/Repository/Repository.php | 32 ++-- tests/Unit/Backend/SftpRangeFetcherTest.php | 32 ++++ .../Backend/StorageBackendReconnectTest.php | 23 +++ tests/Unit/Repository/IndexListingTest.php | 148 ++++++++++++++++++ 9 files changed, 324 insertions(+), 28 deletions(-) create mode 100644 tests/Unit/Repository/IndexListingTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 35ed537..03c1094 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ the app store accepts a `-beta` suffix, but Nextcloud servers then only offer th beta/daily update channel, which would hide the app from most admins. The stability stage is instead carried by the app description, the git tag, the release, and this file. +## [1.0.0-rc.5] - 2026-10-20 + +Fifth release candidate for 1.0.0; the app version is 0.9.11. A full restore of 1 TB over SFTP +stopped after 13 hours with "Blob not in index"; the backup itself was fine. + +### Fixed +- A full restore no longer stops with "Blob not in index" after a pack could not be read for a + moment. Reading the index again then got an empty listing (from a connection the location had + dropped) and emptied the index. Reading the index again now only adds, an empty listing of a + location that has an index is an error instead of an empty index, and a pack that could not be + read is read once more before the restore gives up. +- An SFTP connection that a Hetzner Storage Box dropped without phpseclib noticing is now + recognised and replaced: the check asks the server and counts its answer. +- The extra connection a full restore reads ahead on is replaced when it breaks (up to 20 times), + instead of reading the rest of the restore the slow way. + ## [1.0.0-rc.4] - 2026-10-19 Fourth release candidate for 1.0.0; the app version is 0.9.10. 1.0.0-rc.3 (0.9.9) was not diff --git a/appinfo/info.xml b/appinfo/info.xml index 210a3ce..536aa46 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -41,7 +41,7 @@ welcome: use the "Share your experience" link on the admin page. Developed and sponsored by [Ensembia](https://www.ensembia.com). We can offer (paid) assistance if you need help with restoring a backup after a crash.]]> - 0.9.10 + 0.9.11 agpl Dillard Blom NgBackup @@ -52,9 +52,9 @@ you need help with restoring a backup after a crash.]]> https://github.com/dillardblom/ng_backup https://github.com/dillardblom/ng_backup/issues https://github.com/dillardblom/ng_backup - https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.4/screenshots/1-backups.png - https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.4/screenshots/2-restore.png - https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.4/screenshots/3-settings.png + https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.5/screenshots/1-backups.png + https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.5/screenshots/2-restore.png + https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.5/screenshots/3-settings.png sodium diff --git a/lib/Backend/SftpRangeFetcher.php b/lib/Backend/SftpRangeFetcher.php index def71af..3f45f99 100644 --- a/lib/Backend/SftpRangeFetcher.php +++ b/lib/Backend/SftpRangeFetcher.php @@ -54,6 +54,9 @@ final class SftpRangeFetcher { private const FXF_READ = 1; private const FX_OK = 0; + /** Connections that broke and are replaced, at most, in one restore. */ + public const MAX_RECONNECTS = 20; + private const OPEN = 0; private const READ = 1; private const CLOSE = 2; @@ -65,6 +68,8 @@ final class SftpRangeFetcher { /** @var array ranges read (or failed) and not yet given to the caller */ private array $done = []; private int $requestId = 0x4e520000; + /** Connections replaced so far (see MAX_RECONNECTS). */ + public int $reconnects = 0; /** * @param array $connections phpseclib 2 connections with their SFTP channel open @@ -75,6 +80,7 @@ final class SftpRangeFetcher { private string $root, private array $ranges, private string $spool, + private ?\Closure $connect = null, ) { foreach (array_keys($connections) as $c) { $this->chunks[$c] = self::CHUNK; @@ -86,10 +92,12 @@ final class SftpRangeFetcher { * * @param list $connections * @param list $ranges + * @param (\Closure():?object)|null $connect a new connection in place of one that broke + * (null when it cannot be made) * @return \Generator */ - public static function fetch(array $connections, string $root, array $ranges, string $spool): \Generator { - $fetcher = new self($connections, rtrim($root, '/'), $ranges, $spool); + public static function fetch(array $connections, string $root, array $ranges, string $spool, ?\Closure $connect = null): \Generator { + $fetcher = new self($connections, rtrim($root, '/'), $ranges, $spool, $connect); try { yield from $fetcher->run(); } finally { @@ -319,6 +327,27 @@ final class SftpRangeFetcher { } self::disconnect($this->connections[$c]); unset($this->connections[$c]); + // A long restore outlives connections (a Storage Box drops one now and then): without a + // new one, the rest would be read the slow way. + if ($this->connect === null || $this->reconnects >= self::MAX_RECONNECTS) { + return; + } + $this->reconnects++; + try { + $new = ($this->connect)(); + } catch (\Throwable) { + $new = null; + } + if (function_exists('OCP\Log\logger')) { + \OCP\Log\logger('ng_backup')->info($new === null + ? 'Reading ahead: an SFTP connection broke and no new one could be made' + : 'Reading ahead: an SFTP connection broke; reading on with a new one'); + } + if ($new !== null) { + $key = max(array_merge(array_keys($this->chunks), [-1])) + 1; + $this->connections[$key] = $new; + $this->chunks[$key] = self::CHUNK; + } } /** @return resource|null a new file in the spool, already unlinked: nothing is left behind if the process dies */ diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index ec5897d..984ecc6 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -282,10 +282,19 @@ final class StorageBackend implements IBackend, IReadsAhead { if ($connections === []) { return null; } - return (static function () use ($storages, $connections, $root, $abs, $spool): \Generator { + // in place of a connection that broke: a new storage, kept like the others + $connect = static function () use ($reconnect, &$storages): ?object { + $storage = $reconnect(); + $sftp = SftpReadStream::connection($storage); + if ($sftp !== null) { + $storages[] = $storage; + } + return $sftp; + }; + return (static function () use (&$storages, $connections, $root, $abs, $spool, $connect): \Generator { // the storages hold their connections: kept until the fetcher is done try { - yield from SftpRangeFetcher::fetch($connections, $root, $abs, $spool); + yield from SftpRangeFetcher::fetch($connections, $root, $abs, $spool, $connect); } finally { unset($storages); } @@ -478,8 +487,9 @@ final class StorageBackend implements IBackend, IReadsAhead { $this->lastUsed = microtime(true); } + /** After a failed request: ask the server (one round trip), as phpseclib may not know it is gone. */ private function reopenIfDead(): bool { - if ($this->reconnect === null || !$this->sftpReads || $this->alive(false) !== false) { + if ($this->reconnect === null || !$this->sftpReads || $this->alive(true) !== false) { return false; } $this->reopen(); @@ -508,8 +518,9 @@ final class StorageBackend implements IBackend, IReadsAhead { if (is_callable([$sftp, 'clearStatCache'])) { $sftp->clearStatCache(); // or stat() answers from memory } - $sftp->stat('.'); - return (bool)$sftp->isConnected(); + // A Storage Box drops a quiet connection without phpseclib noticing: it still says + // connected, and every request fails. So the answer counts, not what it says after. + return $sftp->stat('.') !== false && (bool)$sftp->isConnected(); } catch (\Throwable) { return false; } diff --git a/lib/Repository/BlobIndex.php b/lib/Repository/BlobIndex.php index 1fcfc3d..fac0fa2 100644 --- a/lib/Repository/BlobIndex.php +++ b/lib/Repository/BlobIndex.php @@ -7,6 +7,7 @@ declare(strict_types=1); namespace OCA\NgBackup\Repository; +use OCA\NgBackup\Backend\BackendException; use OCA\NgBackup\Backend\IBackend; use OCA\NgBackup\Crypto\StreamCipher; @@ -104,6 +105,11 @@ final class BlobIndex { $fetch[] = $path; } } + if ($present === [] && $cached !== []) { + // A repository never loses all its index files: an empty listing is a connection that + // failed quietly. Taken as is, it would empty the index and the cache. + throw new BackendException('The location listed no index files, while ' . count($cached) . ' are known: not trusting that listing'); + } // The rest is downloaded several at a time where the location allows it: on a large // repository these are tens of thousands of small files. getMany() keeps their order, so // the cached packs listed before each one are added first. @@ -192,6 +198,39 @@ final class BlobIndex { } /** @return array> packId => blobs currently located in that pack */ + /** + * Add the index files that appeared on the location since load(), such as those of packs + * another server's cleanup wrote while a restore runs. Only adds and updates: a blob is + * never dropped, as a listing that misses files (a connection that failed) must not make a + * restore lose blobs it knows. A blob in a new pack takes that pack's place. + */ + public function refresh(): void { + $known = array_flip(array_map(fn (array $b): string => $b[0], $this->blobs)); + $fetch = []; + $listed = 0; + foreach ($this->backend->listFiles('index') as $path) { + $listed++; + if (!isset($known[basename($path)])) { + $fetch[] = $path; + } + } + if ($listed === 0 && $this->blobs !== []) { + throw new BackendException('The location listed no index files, while ' . count($known) . ' are known: not trusting that listing'); + } + $toCache = []; + foreach ($this->backend->getMany($fetch) as $path => $data) { + $packId = basename($path); + /** @var list $entries authenticated: written by NG Backup */ + $entries = json_decode($this->cipher->decryptString($data, 'index:' . $packId), true, 512, JSON_THROW_ON_ERROR); + $this->downloaded++; + $this->add($packId, $entries); + $toCache[$packId] = $entries; + } + if ($toCache !== []) { + $this->cache?->putMany($this->repositoryId, $toCache); + } + } + public function packs(): array { $packs = []; foreach ($this->blobs as $id => [$pack, $offset, $length]) { diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index bc3b6f7..b761dcf 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -841,9 +841,9 @@ final class Repository { $done++; } } - } catch (BackendException $e) { - // A pack moved (see loadBlob()): the rest one blob at a time, from the fresh index. - $this->refreshIndex($e); + } catch (BackendException) { + // A pack moved, or the read failed for a while (see loadBlob()): the rest one blob at a + // time, each read once more after a failure. foreach (array_slice($ids, $done) as $id) { yield $this->loadBlob($id); } @@ -872,27 +872,25 @@ final class Repository { $encrypted = $this->packReader->read($pack, $offset, $length); } catch (BackendException $e) { // Another server's cleanup may have repacked this blob into a new pack since the index - // was loaded (a disaster recovery test while the original server still runs). Reload - // the index from the location; read again only if the blob has moved. - $this->refreshIndex($e); + // was loaded (a disaster recovery test while the original server still runs): add the + // new index files, and read the blob where it is now. Or the read failed for a while + // (a connection that died; the backend has replaced it by now): then read it once + // more where it was. A second failure is the real one. + try { + $this->index->refresh(); + } catch (\Throwable) { + // the location could not be listed either: read again where the blob was + } [$moved, $offset, $length] = $this->index->get($id); - if ($moved === $pack) { + try { + $encrypted = $this->packReader->read($moved, $offset, $length); + } catch (BackendException) { throw $e; } - $encrypted = $this->packReader->read($moved, $offset, $length); } return $this->decodeBlob($id, $encrypted); } - /** Reload the index from the location after a failed pack read; rethrows that failure if it cannot. */ - private function refreshIndex(BackendException $cause): void { - try { - $this->index->load(); - } catch (\Throwable) { - throw $cause; - } - } - /** Decrypts and checks a blob read from its pack; throws when it is corrupted or tampered with. */ private function decodeBlob(string $id, string $encrypted): string { [, , , $raw, $flags] = $this->index->get($id); diff --git a/tests/Unit/Backend/SftpRangeFetcherTest.php b/tests/Unit/Backend/SftpRangeFetcherTest.php index 5544d76..b969743 100644 --- a/tests/Unit/Backend/SftpRangeFetcherTest.php +++ b/tests/Unit/Backend/SftpRangeFetcherTest.php @@ -184,6 +184,38 @@ class SftpRangeFetcherTest extends TestCase { $this->assertSame(array_fill(0, 10, null), $result); } + public function testABrokenConnectionIsReplacedAndTheRestIsReadAhead(): void { + $connections = $this->connections(1, [['loseAt' => 1048576]]); + $made = []; + $connect = function () use (&$made): object { + $made[] = $c = new FakeSftpConnection($this->files); + return SftpReadStream::connection(new FakeSftpStorage($c)); + }; + $ranges = $this->ranges(); + + $result = self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', $ranges, $this->tempDir(), $connect)); + + $expected = $this->expected($ranges); + $expected[0] = null; // the range it was reading when it broke: read the usual way + $this->assertSame($expected, $result); + $this->assertCount(1, $made); + $this->assertTrue($made[0]->disconnected, 'the new one ends with the fetcher too'); + } + + public function testConnectionsThatKeepBreakingAreReplacedOnlySoOften(): void { + $made = 0; + $connect = function () use (&$made): object { + $made++; + return SftpReadStream::connection(new FakeSftpStorage(new FakeSftpConnection($this->files, loseAt: 0))); + }; + $ranges = array_merge($this->ranges(), $this->ranges(), $this->ranges()); + + $result = self::collect(SftpRangeFetcher::fetch(self::opened($this->connections(1, [['loseAt' => 0]])), '/home/', $ranges, $this->tempDir(), $connect)); + + $this->assertSame(array_fill(0, count($ranges), null), $result); + $this->assertSame(SftpRangeFetcher::MAX_RECONNECTS, $made); + } + public function testStoppingHalfwayEndsTheConnectionsAndLeavesNothingBehind(): void { $connections = $this->connections(2); $spool = $this->tempDir(); diff --git a/tests/Unit/Backend/StorageBackendReconnectTest.php b/tests/Unit/Backend/StorageBackendReconnectTest.php index 161fa0a..8c4aea0 100644 --- a/tests/Unit/Backend/StorageBackendReconnectTest.php +++ b/tests/Unit/Backend/StorageBackendReconnectTest.php @@ -77,6 +77,24 @@ class StorageBackendReconnectTest extends TestCase { $this->assertSame(1, $backend->reconnects); } + public function testASilentlyDroppedConnectionIsReplacedAfterAPause(): void { + $backend = $this->backend(); + $this->connections[0]->dropped = true; + $this->connections[0]->silent = true; // phpseclib still says connected + self::idle($backend); + $this->assertTrue($backend->exists('config')); + $this->assertSame(1, $backend->reconnects, 'the check failed, so the connection was replaced'); + } + + public function testAFailureOnASilentlyDroppedConnectionIsRepeated(): void { + $backend = $this->backend(); + $this->connections[0]->dropped = true; + $this->connections[0]->silent = true; + $fh = $backend->get('config'); + $this->assertSame('config', stream_get_contents($fh)); + $this->assertSame(1, $backend->reconnects); + } + public function testAMissingFileOnALiveConnectionIsJustMissing(): void { $backend = $this->backend(); $this->assertFalse($backend->exists('nothing')); @@ -121,6 +139,8 @@ interface SftpLikeStorage extends IStorage { /** The public phpseclib calls the backend uses to tell a dead connection. */ final class ReconnectFakeSftp { public bool $dropped = false; + /** Dropped without phpseclib noticing (as a Hetzner Storage Box does): it still says connected. */ + public bool $silent = false; private bool $connected = true; public int $requests = 0; @@ -139,6 +159,9 @@ final class ReconnectFakeSftp { public function request(): bool { $this->requests++; if ($this->dropped) { + if ($this->silent) { + return false; + } $this->connected = false; } return $this->connected; diff --git a/tests/Unit/Repository/IndexListingTest.php b/tests/Unit/Repository/IndexListingTest.php new file mode 100644 index 0000000..d1eab6b --- /dev/null +++ b/tests/Unit/Repository/IndexListingTest.php @@ -0,0 +1,148 @@ +root = $this->tempDir(); + $this->keys = KeyRing::generate(); + $this->src = $this->tempDir(); + for ($i = 0; $i < 12; $i++) { + file_put_contents("$this->src/f$i", random_bytes(3 * 1048576)); + } + $repo = Repository::initWithKey(new LocalBackend($this->root), $this->keys, $this->keys->wrap('pw')); + $this->snapshot = $repo->backupDirectory($this->src)['snapshot']; + } + + public function testARestoreGoesOnAfterAPackReadFailedAndTheListingCameBackEmpty(): void { + $backend = new FlakyBackend(new LocalBackend($this->root)); + $repo = Repository::openWithKey($backend, $this->keys); + $backend->failPackReads = 1; // the read of a pack fails once + $backend->emptyIndexListings = 1; // and listing the index right after comes back empty + $target = $this->tempDir(); + + $this->assertSame(12, $repo->restore($this->snapshot, $target)); + + $this->assertSame(self::hashTree($this->src), self::hashTree($target)); + $this->assertSame(0, $backend->failPackReads, 'the failure happened'); + $this->assertSame(0, $backend->emptyIndexListings, 'the empty listing was seen'); + } + + public function testAPackThatStaysUnreadableIsStillReportedAsSuch(): void { + $backend = new FlakyBackend(new LocalBackend($this->root)); + $repo = Repository::openWithKey($backend, $this->keys); + $backend->failPackReads = 1000; + + try { + $repo->restore($this->snapshot, $this->tempDir()); + $this->fail('the packs cannot be read'); + } catch (BackendException $e) { + $this->assertStringStartsWith('Not found: packs/', $e->getMessage()); + } + } + + public function testOpeningWithAnEmptyListingKeepsTheCachedIndex(): void { + $cache = new class implements IIndexCache { + public array $data = []; + public function all(string $r): array { return $this->data[$r] ?? []; } + public function put(string $r, string $p, array $e): void { $this->data[$r][$p] = $e; } + public function remove(string $r, array $ps): void { foreach ($ps as $p) { unset($this->data[$r][$p]); } } + public function putMany(string $r, array $packs): void { foreach ($packs as $p => $e) { $this->put($r, (string)$p, $e); } } + public function replaceAll(string $r, array $packs): void { $this->data[$r] = []; $this->putMany($r, $packs); } + }; + $backend = new FlakyBackend(new LocalBackend($this->root)); + Repository::openWithKey($backend, $this->keys, $cache); + $cached = $cache->data; + $this->assertNotSame([], $cached); + + $backend->emptyIndexListings = 1; + try { + Repository::openWithKey($backend, $this->keys, $cache); + $this->fail('an empty listing of a repository with an index must not be taken as is'); + } catch (BackendException $e) { + $this->assertStringContainsString('listed no index files', $e->getMessage()); + } + $this->assertSame($cached, $cache->data, 'the cache is untouched'); + } +} + +/** A location whose pack reads fail, and whose index listings come back empty, a set number of times. */ +final class FlakyBackend implements IBackend { + use \OCA\NgBackup\Backend\GetsOneByOne; + + public int $failPackReads = 0; + public int $emptyIndexListings = 0; + + public function __construct( + private IBackend $inner, + ) { + } + + public function put(string $path, $stream): void { + $this->inner->put($path, $stream); + } + + public function get(string $path) { + if (str_starts_with($path, 'packs/') && $this->failPackReads > 0) { + $this->failPackReads--; + throw new BackendException('Not found: ' . $path); + } + return $this->inner->get($path); + } + + public function getRange(string $path, int $offset, int $length): string { + if (str_starts_with($path, 'packs/') && $this->failPackReads > 0) { + $this->failPackReads--; + throw new BackendException('Not found: ' . $path); + } + return $this->inner->getRange($path, $offset, $length); + } + + public function exists(string $path): bool { + return $this->inner->exists($path); + } + + public function move(string $from, string $to): void { + $this->inner->move($from, $to); + } + + public function list(string $prefix): array { + return $this->inner->list($prefix); + } + + public function listFiles(string $dir): array { + if ($dir === 'index' && $this->emptyIndexListings > 0) { + $this->emptyIndexListings--; + return []; + } + return $this->inner->listFiles($dir); + } + + public function delete(string $path): void { + $this->inner->delete($path); + } +} From d504067cd89ad4f5884b1bb1818cd8127fe29db1 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 22:13:26 +0200 Subject: [PATCH 02/19] Index: an empty listing is an error in checks and cleanups too; refresh() fetches only index files it has not loaded; full-restore CI on Nextcloud 35.0.0 --- .github/workflows/ci.yml | 14 ++++++------ lib/Repository/BlobIndex.php | 25 ++++++++++++++-------- tests/Unit/Repository/IndexListingTest.php | 21 ++++++++++++++++++ 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 549c2a5..3546369 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -93,6 +93,8 @@ jobs: - name: Per-user backup/restore round trip on Nextcloud ${{ matrix.nextcloud }} run: tests/integration/user-roundtrip.sh "docker exec -u www-data ngbu" /var/www/html/data + # 35.0.0, not 35: the app store lists apps for a new point release only some time after it + # is out, and this test installs Mail and Deck from it. integration-full-restore: # Disaster recovery (occ backup:restore:full) on a SECOND, fresh Nextcloud instance that # never saw the first one, sharing only the backup location. Does not depend on the database @@ -103,14 +105,14 @@ jobs: with: persist-credentials: false - name: Full restore round trip - run: tests/integration/full-restore-roundtrip.sh 35-apache + run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache - name: Full restore round trip on PostgreSQL, with Mail (foreign keys) and a disabled app - run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql + run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache pgsql - name: Full restore round trip on MariaDB, with Mail (foreign keys) and a disabled app - run: tests/integration/full-restore-roundtrip.sh 35-apache mysql + run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache mysql - name: Full restore of a backup made by 1.0.0-rc.1 (no table structure) on PostgreSQL - run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql legacy + run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache pgsql legacy - name: Full restore of a backup made by 1.0.0-rc.1 (no table structure) on MariaDB - run: tests/integration/full-restore-roundtrip.sh 35-apache mysql legacy + run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache mysql legacy - name: Full restore over SFTP on PostgreSQL, reading packs ahead on several connections - run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql sftp + run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache pgsql sftp diff --git a/lib/Repository/BlobIndex.php b/lib/Repository/BlobIndex.php index fac0fa2..32ca142 100644 --- a/lib/Repository/BlobIndex.php +++ b/lib/Repository/BlobIndex.php @@ -33,6 +33,8 @@ final class BlobIndex { private const CACHE_LOT = 256; /** @var int index files downloaded by the last load() (for tests and diagnostics) */ public int $downloaded = 0; + /** @var array packs whose index file is loaded (refresh() fetches only others) */ + private array $loaded = []; public function __construct( private IBackend $backend, @@ -91,6 +93,7 @@ final class BlobIndex { * longer exist on the backend are dropped from the cache. */ public function load(bool $verify = false, ?callable $progress = null): void { + $hadIndex = $this->blobs !== []; $this->blobs = []; $cached = $verify ? [] : ($this->cache?->all($this->repositoryId) ?? []); $present = []; @@ -105,11 +108,13 @@ final class BlobIndex { $fetch[] = $path; } } - if ($present === [] && $cached !== []) { - // A repository never loses all its index files: an empty listing is a connection that - // failed quietly. Taken as is, it would empty the index and the cache. - throw new BackendException('The location listed no index files, while ' . count($cached) . ' are known: not trusting that listing'); + // A repository never loses all its index files: an empty listing is a connection that + // failed quietly. Taken as is, it would empty the index and the cache (or make a check + // report every blob missing, or a cleanup wipe the cache). + if ($present === [] && ($hadIndex || $cached !== [] || ($verify && ($this->cache?->all($this->repositoryId) ?? []) !== []))) { + throw new BackendException('The location listed no index files, while some are known: not trusting that listing'); } + $this->loaded = $present; // The rest is downloaded several at a time where the location allows it: on a large // repository these are tens of thousands of small files. getMany() keeps their order, so // the cached packs listed before each one are added first. @@ -191,13 +196,13 @@ final class BlobIndex { rewind($stream); $this->backend->put('index/' . $packId, $stream); $this->cachePut($packId, $entries); + $this->loaded[$packId] = true; foreach ($entries as $e) { $this->blobs[$e['id']] = [$packId, $e['offset'], $e['length'], $e['raw'], $e['flags']]; unset($this->pending[$e['id']]); } } - /** @return array> packId => blobs currently located in that pack */ /** * Add the index files that appeared on the location since load(), such as those of packs * another server's cleanup wrote while a restore runs. Only adds and updates: a blob is @@ -205,17 +210,16 @@ final class BlobIndex { * restore lose blobs it knows. A blob in a new pack takes that pack's place. */ public function refresh(): void { - $known = array_flip(array_map(fn (array $b): string => $b[0], $this->blobs)); $fetch = []; $listed = 0; foreach ($this->backend->listFiles('index') as $path) { $listed++; - if (!isset($known[basename($path)])) { + if (!isset($this->loaded[basename($path)])) { $fetch[] = $path; } } - if ($listed === 0 && $this->blobs !== []) { - throw new BackendException('The location listed no index files, while ' . count($known) . ' are known: not trusting that listing'); + if ($listed === 0 && $this->loaded !== []) { + throw new BackendException('The location listed no index files, while ' . count($this->loaded) . ' are known: not trusting that listing'); } $toCache = []; foreach ($this->backend->getMany($fetch) as $path => $data) { @@ -224,6 +228,7 @@ final class BlobIndex { $entries = json_decode($this->cipher->decryptString($data, 'index:' . $packId), true, 512, JSON_THROW_ON_ERROR); $this->downloaded++; $this->add($packId, $entries); + $this->loaded[$packId] = true; $toCache[$packId] = $entries; } if ($toCache !== []) { @@ -231,6 +236,7 @@ final class BlobIndex { } } + /** @return array> packId => blobs currently located in that pack */ public function packs(): array { $packs = []; foreach ($this->blobs as $id => [$pack, $offset, $length]) { @@ -243,6 +249,7 @@ final class BlobIndex { public function removePack(string $packId): void { $this->backend->delete('index/' . $packId); $this->cacheRemove([$packId]); + unset($this->loaded[$packId]); foreach ($this->blobs as $id => $loc) { if ($loc[0] === $packId) { unset($this->blobs[$id]); diff --git a/tests/Unit/Repository/IndexListingTest.php b/tests/Unit/Repository/IndexListingTest.php index d1eab6b..b014fb4 100644 --- a/tests/Unit/Repository/IndexListingTest.php +++ b/tests/Unit/Repository/IndexListingTest.php @@ -65,6 +65,27 @@ class IndexListingTest extends TestCase { } } + public function testACheckWithAnEmptyListingIsAnErrorNotEveryBlobMissing(): void { + $backend = new FlakyBackend(new LocalBackend($this->root)); + $repo = Repository::openWithKey($backend, $this->keys); + $backend->emptyIndexListings = 1; + + $this->expectException(BackendException::class); + $this->expectExceptionMessage('listed no index files'); + $repo->verify($this->snapshot); + } + + public function testRefreshingFetchesOnlyIndexFilesItHasNotLoaded(): void { + $backend = new FlakyBackend(new LocalBackend($this->root)); + $repo = Repository::openWithKey($backend, $this->keys); + $index = (new \ReflectionProperty($repo, 'index'))->getValue($repo); + $index->downloaded = 0; + + $index->refresh(); + + $this->assertSame(0, $index->downloaded, 'every index file is loaded already, also those whose blobs are elsewhere too'); + } + public function testOpeningWithAnEmptyListingKeepsTheCachedIndex(): void { $cache = new class implements IIndexCache { public array $data = []; From e461c480569b22d7b47fac811c1f1d6dc8fe817a Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 22:18:33 +0200 Subject: [PATCH 03/19] CI: back to Nextcloud 35 for the full-restore test (the failures were a slow app store, not the version) --- .github/workflows/ci.yml | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3546369..549c2a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -93,8 +93,6 @@ jobs: - name: Per-user backup/restore round trip on Nextcloud ${{ matrix.nextcloud }} run: tests/integration/user-roundtrip.sh "docker exec -u www-data ngbu" /var/www/html/data - # 35.0.0, not 35: the app store lists apps for a new point release only some time after it - # is out, and this test installs Mail and Deck from it. integration-full-restore: # Disaster recovery (occ backup:restore:full) on a SECOND, fresh Nextcloud instance that # never saw the first one, sharing only the backup location. Does not depend on the database @@ -105,14 +103,14 @@ jobs: with: persist-credentials: false - name: Full restore round trip - run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache + run: tests/integration/full-restore-roundtrip.sh 35-apache - name: Full restore round trip on PostgreSQL, with Mail (foreign keys) and a disabled app - run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache pgsql + run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql - name: Full restore round trip on MariaDB, with Mail (foreign keys) and a disabled app - run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache mysql + run: tests/integration/full-restore-roundtrip.sh 35-apache mysql - name: Full restore of a backup made by 1.0.0-rc.1 (no table structure) on PostgreSQL - run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache pgsql legacy + run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql legacy - name: Full restore of a backup made by 1.0.0-rc.1 (no table structure) on MariaDB - run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache mysql legacy + run: tests/integration/full-restore-roundtrip.sh 35-apache mysql legacy - name: Full restore over SFTP on PostgreSQL, reading packs ahead on several connections - run: tests/integration/full-restore-roundtrip.sh 35.0.0-apache pgsql sftp + run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql sftp From ac57e31c9300d9369d0061d328f358bd50b19121 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 20 Oct 2026 23:04:53 +0200 Subject: [PATCH 04/19] Full restore onto PostgreSQL: counters of restored tables go after their data when the backup records none (MySQL, SQLite); test MariaDB onto PostgreSQL --- .github/workflows/ci.yml | 2 ++ CHANGELOG.md | 5 ++++ lib/Db/DbDumper.php | 27 ++++++++++++++++++--- tests/integration/full-restore-roundtrip.sh | 17 +++++++++++-- 4 files changed, 46 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 549c2a5..3f701f1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -114,3 +114,5 @@ jobs: run: tests/integration/full-restore-roundtrip.sh 35-apache mysql legacy - name: Full restore over SFTP on PostgreSQL, reading packs ahead on several connections run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql sftp + - name: Full restore of a MariaDB backup onto PostgreSQL (ID counters after the data) + run: tests/integration/full-restore-roundtrip.sh 35-apache mysql to-pgsql diff --git a/CHANGELOG.md b/CHANGELOG.md index 03c1094..650cbfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,11 @@ stopped after 13 hours with "Blob not in index"; the backup itself was fine. dropped) and emptied the index. Reading the index again now only adds, an empty listing of a location that has an index is an error instead of an empty index, and a pack that could not be read is read once more before the restore gives up. +- After a full restore of a MySQL/MariaDB or SQLite backup onto PostgreSQL, new files, users or + activities could not be added ("duplicate key", "File entry could not be inserted"): such + backups record no ID counters, and they stayed at 1. Every counter of a restored table now goes + after its data. Found in a disaster recovery simulation of 138 GB from MariaDB to PostgreSQL, + where all 15,772 files and every count were identical. - An SFTP connection that a Hetzner Storage Box dropped without phpseclib noticing is now recognised and replaced: the check asks the server and counts its answer. - The extra connection a full restore reads ahead on is replaced when it breaks (up to 20 times), diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index 605fb8e..f99da8f 100644 --- a/lib/Db/DbDumper.php +++ b/lib/Db/DbDumper.php @@ -216,7 +216,7 @@ final class DbDumper { } $report['restored'][] = $table; } - $this->restoreSequences($manifest['sequences'] ?? [], $report['missing']); + $this->restoreSequences($manifest['sequences'] ?? [], $report['missing'], $report['restored']); $this->resumeForeignKeys($foreignKeys); $this->db->commit(); // Foreign keys of the tables created above, now their rows are in. Not inside the @@ -573,16 +573,21 @@ final class DbDumper { return $name; } - /** @param list $leftOut tables not restored (--skip-missing-tables): they do not exist here */ - private function restoreSequences(array $sequences, array $leftOut = []): void { + /** + * @param list $leftOut tables not restored (--skip-missing-tables): they do not exist here + * @param list $restored tables restored: their other counters go after their data + */ + private function restoreSequences(array $sequences, array $leftOut = [], array $restored = []): void { if (!$this->db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { return; } $leftOut = array_flip($leftOut); + $set = []; foreach ($sequences as $info) { if (isset($leftOut[$info['table']])) { continue; } + $set[$info['table'] . '.' . $info['column']] = true; $seq = $this->db->fetchOne('SELECT pg_get_serial_sequence(quote_ident(?), ?)', [$info['table'], $info['column']]); if ($seq === false || $seq === null) { continue; @@ -597,5 +602,21 @@ final class DbDumper { [\Doctrine\DBAL\ParameterType::STRING, \Doctrine\DBAL\ParameterType::INTEGER, \Doctrine\DBAL\ParameterType::BOOLEAN]); } } + // A backup from MySQL/MariaDB or SQLite records no counters (those derive them from the + // data): every other counter of a restored table goes after its data, or new rows (a new + // file, an activity) would collide with restored ones. + $restored = array_flip($restored); + $rows = $this->db->fetchAllAssociative( + "SELECT c.table_name, c.column_name, pg_get_serial_sequence(quote_ident(c.table_name), c.column_name) AS seq + FROM information_schema.columns c + WHERE c.table_schema = current_schema() AND pg_get_serial_sequence(quote_ident(c.table_name), c.column_name) IS NOT NULL"); + foreach ($rows as $r) { + if (!isset($restored[$r['table_name']]) || isset($set[$r['table_name'] . '.' . $r['column_name']])) { + continue; + } + $max = (int)$this->db->fetchOne(sprintf('SELECT COALESCE(MAX(%s), 0) FROM %s', + $this->db->quoteIdentifier($r['column_name']), $this->db->quoteIdentifier($r['table_name']))); + $this->db->executeQuery('SELECT setval(CAST(? AS regclass), ?, false)', [$r['seq'], $max + 1]); + } } } diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index 7b65ed7..2b3061f 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -2,10 +2,12 @@ # Disaster recovery: occ backup:key:import + backup:target:add + backup:restore:full on a # SECOND, fresh Nextcloud instance that never saw the first one, sharing only the backup # location (a local directory, bind-mounted into both containers) -- see PLAN.md 3.7. -# Usage: full-restore-roundtrip.sh [sqlite|pgsql|mysql] [legacy|sftp] +# Usage: full-restore-roundtrip.sh [sqlite|pgsql|mysql] [legacy|sftp|to-pgsql] # e.g. full-restore-roundtrip.sh 35-apache pgsql # With "sftp" the location is an SFTP server instead of a shared directory, and the origin has a # file spread over several packs: the restore reads them ahead on connections of its own. +# With "to-pgsql" the restore instance runs PostgreSQL whatever the origin runs (MySQL and SQLite +# backups record no ID counters; after the restore new rows must not collide with restored ones). # With "legacy" the origin runs NG Backup 1.0.0-rc.1 (0.9.7), whose backups do not record the # structure of their tables: the restore must then leave the tables of the disabled app out # with --skip-missing-tables, as for a real backup made before 1.0.0-rc.2. @@ -127,6 +129,8 @@ ok "test data backed up, snapshot $SNAP" docker cp ngb-frt-origin:/tmp/kit.json /tmp/ngb-frt-kit.json docker exec ngb-frt-origin sh -c "cat /tmp/pass" > /tmp/ngb-frt-pass +ORIGIN_DBTYPE=$DB +[ "$MODE" = to-pgsql ] && DB=pgsql RESTORE_DB=$(db_env restore) # shellcheck disable=SC2086 docker run -d --name ngb-frt-restore --network "$NET" $RESTORE_DB \ @@ -170,6 +174,15 @@ run_on ngb-frt-restore user:list | grep -q frtest || fail "the backed-up user is docker exec ngb-frt-restore test -f /var/www/html/data/frtest/files/Docs/note.txt || fail "restored file is missing" [ "$(docker exec ngb-frt-restore cat /var/www/html/data/frtest/files/Docs/note.txt)" = "hello disaster recovery" ] || fail "restored file content differs" run_on ngb-frt-restore files:scan frtest >/dev/null || fail "files:scan fails after restore" +# New rows after the restore: a new file (file cache) and a new user must not collide with +# restored rows (ID counters after the data). +docker exec ngb-frt-restore sh -c "echo new > /var/www/html/data/frtest/files/Docs/after-restore.txt && chown www-data /var/www/html/data/frtest/files/Docs/after-restore.txt" +run_on ngb-frt-restore files:scan frtest >/dev/null || fail "a new file cannot be added after the restore" +if [ "$DB" != sqlite ]; then + [ "$(sql restore "select count(*) from oc_filecache where path = 'files/Docs/after-restore.txt'")" = 1 ] \ + || fail "a new file did not make it into the file cache after the restore (ID counters?)" +fi +docker exec -u www-data -e OC_PASS="$NC_PASS" ngb-frt-restore php occ user:add --password-from-env frnew >/dev/null || fail "a new user cannot be added after the restore" ok "database, files and ng_backup's own state are all intact and usable after the restore" if [ "$DB" != sqlite ] && [ "$LEGACY" = legacy ]; then @@ -197,4 +210,4 @@ fi run_on ngb-frt-restore backup:run offsite -l post-restore-check >/dev/null || fail "a new backup after the restore fails" ok "the restored instance can run a new backup of its own" -echo "FULL RESTORE ROUNDTRIP PASS ($DB${MODE:+, $MODE})" +echo "FULL RESTORE ROUNDTRIP PASS ($ORIGIN_DBTYPE${MODE:+, $MODE})" From 85ea0acc50fe87927db0ee93aa89bb7e9296686e Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 20 Oct 2026 23:10:25 +0200 Subject: [PATCH 05/19] Full-restore test: restore onto another database (to-pgsql, to-mysql, to-sqlite), every crossing in CI --- .github/workflows/ci.yml | 8 ++++++++ tests/integration/full-restore-roundtrip.sh | 9 +++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3f701f1..d3a807b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -116,3 +116,11 @@ jobs: run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql sftp - name: Full restore of a MariaDB backup onto PostgreSQL (ID counters after the data) run: tests/integration/full-restore-roundtrip.sh 35-apache mysql to-pgsql + - name: Full restore of a PostgreSQL backup onto MariaDB + run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql to-mysql + - name: Full restore of a PostgreSQL backup onto SQLite + run: tests/integration/full-restore-roundtrip.sh 35-apache pgsql to-sqlite + - name: Full restore of a MariaDB backup onto SQLite + run: tests/integration/full-restore-roundtrip.sh 35-apache mysql to-sqlite + - name: Full restore of a SQLite backup onto PostgreSQL + run: tests/integration/full-restore-roundtrip.sh 35-apache sqlite to-pgsql diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index 2b3061f..fe6c2cf 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -2,12 +2,13 @@ # Disaster recovery: occ backup:key:import + backup:target:add + backup:restore:full on a # SECOND, fresh Nextcloud instance that never saw the first one, sharing only the backup # location (a local directory, bind-mounted into both containers) -- see PLAN.md 3.7. -# Usage: full-restore-roundtrip.sh [sqlite|pgsql|mysql] [legacy|sftp|to-pgsql] +# Usage: full-restore-roundtrip.sh [sqlite|pgsql|mysql] [legacy|sftp|to-pgsql|to-mysql|to-sqlite] # e.g. full-restore-roundtrip.sh 35-apache pgsql # With "sftp" the location is an SFTP server instead of a shared directory, and the origin has a # file spread over several packs: the restore reads them ahead on connections of its own. -# With "to-pgsql" the restore instance runs PostgreSQL whatever the origin runs (MySQL and SQLite -# backups record no ID counters; after the restore new rows must not collide with restored ones). +# With "to-" the restore instance runs that database whatever the origin runs (a MySQL or +# SQLite backup records no ID counters; after the restore new rows must not collide with restored +# ones, whatever the database on either side). # With "legacy" the origin runs NG Backup 1.0.0-rc.1 (0.9.7), whose backups do not record the # structure of their tables: the restore must then leave the tables of the disabled app out # with --skip-missing-tables, as for a real backup made before 1.0.0-rc.2. @@ -130,7 +131,7 @@ ok "test data backed up, snapshot $SNAP" docker cp ngb-frt-origin:/tmp/kit.json /tmp/ngb-frt-kit.json docker exec ngb-frt-origin sh -c "cat /tmp/pass" > /tmp/ngb-frt-pass ORIGIN_DBTYPE=$DB -[ "$MODE" = to-pgsql ] && DB=pgsql +case "$MODE" in to-*) DB=${MODE#to-} ;; esac RESTORE_DB=$(db_env restore) # shellcheck disable=SC2086 docker run -d --name ngb-frt-restore --network "$NET" $RESTORE_DB \ From 177361acaeaeeea49a79aa764cba0ebaebe5d80a Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 09:08:50 +0200 Subject: [PATCH 06/19] Index load keeps the index when it refuses an empty listing; counters of unchanged tables are repaired too, never moved backwards --- lib/Db/DbDumper.php | 12 +++++++++--- lib/Repository/BlobIndex.php | 3 ++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index f99da8f..b0918f7 100644 --- a/lib/Db/DbDumper.php +++ b/lib/Db/DbDumper.php @@ -216,7 +216,8 @@ final class DbDumper { } $report['restored'][] = $table; } - $this->restoreSequences($manifest['sequences'] ?? [], $report['missing'], $report['restored']); + // unchanged tables too: a restore that stopped earlier may have left their counters at 1 + $this->restoreSequences($manifest['sequences'] ?? [], $report['missing'], array_merge($report['restored'], $report['skipped'])); $this->resumeForeignKeys($foreignKeys); $this->db->commit(); // Foreign keys of the tables created above, now their rows are in. Not inside the @@ -575,7 +576,7 @@ final class DbDumper { /** * @param list $leftOut tables not restored (--skip-missing-tables): they do not exist here - * @param list $restored tables restored: their other counters go after their data + * @param list $restored tables restored or unchanged: their other counters go after their data */ private function restoreSequences(array $sequences, array $leftOut = [], array $restored = []): void { if (!$this->db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { @@ -616,7 +617,12 @@ final class DbDumper { } $max = (int)$this->db->fetchOne(sprintf('SELECT COALESCE(MAX(%s), 0) FROM %s', $this->db->quoteIdentifier($r['column_name']), $this->db->quoteIdentifier($r['table_name']))); - $this->db->executeQuery('SELECT setval(CAST(? AS regclass), ?, false)', [$r['seq'], $max + 1]); + // never backwards: a counter already past the data stays where it is + $state = $this->db->fetchAssociative('SELECT last_value, is_called FROM ' . self::sequenceIdentifier($r['seq'])); + $next = $state === false ? 1 : (int)$state['last_value'] + ($state['is_called'] ? 1 : 0); + if ($next <= $max) { + $this->db->executeQuery('SELECT setval(CAST(? AS regclass), ?, false)', [$r['seq'], $max + 1]); + } } } } diff --git a/lib/Repository/BlobIndex.php b/lib/Repository/BlobIndex.php index 32ca142..fc8e844 100644 --- a/lib/Repository/BlobIndex.php +++ b/lib/Repository/BlobIndex.php @@ -94,7 +94,6 @@ final class BlobIndex { */ public function load(bool $verify = false, ?callable $progress = null): void { $hadIndex = $this->blobs !== []; - $this->blobs = []; $cached = $verify ? [] : ($this->cache?->all($this->repositoryId) ?? []); $present = []; $this->downloaded = 0; @@ -114,6 +113,8 @@ final class BlobIndex { if ($present === [] && ($hadIndex || $cached !== [] || ($verify && ($this->cache?->all($this->repositoryId) ?? []) !== []))) { throw new BackendException('The location listed no index files, while some are known: not trusting that listing'); } + // only now: an empty listing that was refused above leaves the index as it was + $this->blobs = []; $this->loaded = $present; // The rest is downloaded several at a time where the location allows it: on a large // repository these are tens of thousands of small files. getMany() keeps their order, so From 2fcd38f478ade8f52ff11af063b4f1e15ab510fe Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 13:09:54 +0200 Subject: [PATCH 07/19] Full-restore test: Mail and Deck checks only when the origin had them (not on a SQLite origin) --- tests/integration/full-restore-roundtrip.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index fe6c2cf..10a1751 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -186,11 +186,12 @@ fi docker exec -u www-data -e OC_PASS="$NC_PASS" ngb-frt-restore php occ user:add --password-from-env frnew >/dev/null || fail "a new user cannot be added after the restore" ok "database, files and ng_backup's own state are all intact and usable after the restore" -if [ "$DB" != sqlite ] && [ "$LEGACY" = legacy ]; then +# Mail and Deck were only set up on an origin with a database server (see above) +if [ "$ORIGIN_DBTYPE" != sqlite ] && [ "$DB" != sqlite ] && [ "$LEGACY" = legacy ]; then [ "$(sql restore "select count(*) from information_schema.tables where table_name = 'oc_deck_boards'")" = 0 ] || fail "a table of unknown structure was created" [ "$(sql restore "select count(*) from oc_mail_accounts where drafts_mailbox_id is not null")" = 1 ] || fail "the mail account was not restored" ok "older backup: tables of the disabled app left out, Mail restored" -elif [ "$DB" != sqlite ]; then +elif [ "$ORIGIN_DBTYPE" != sqlite ] && [ "$DB" != sqlite ]; then [ "$(sql restore "select count(*) from information_schema.tables where table_name = 'oc_deck_boards'")" = 1 ] || fail "the tables of the disabled app were not created" [ "$(sql restore "select count(*) from oc_mail_accounts where drafts_mailbox_id is not null")" = 1 ] || fail "the mail account was not restored" [ "$(sql restore "select count(*) from information_schema.table_constraints where constraint_type = 'FOREIGN KEY' and table_name like 'oc_mail_%'")" -gt 0 ] \ From 9b6bd7d2abf4d0624c8ca2be2192b86c0907b993 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 13:10:03 +0200 Subject: [PATCH 08/19] Release 1.0.0-rc.5 (app version 0.9.11) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 650cbfd..e1cd1c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ the app store accepts a `-beta` suffix, but Nextcloud servers then only offer th beta/daily update channel, which would hide the app from most admins. The stability stage is instead carried by the app description, the git tag, the release, and this file. -## [1.0.0-rc.5] - 2026-10-20 +## [1.0.0-rc.5] - 2026-10-21 Fifth release candidate for 1.0.0; the app version is 0.9.11. A full restore of 1 TB over SFTP stopped after 13 hours with "Blob not in index"; the backup itself was fine. From 95184dadc75f9a7d9183c44426de2fa6da0b32b0 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 19:51:46 +0200 Subject: [PATCH 09/19] Index: an empty listing is checked (config present, listed once more) before it is taken, so a repository another server emptied is accepted and a hiccup recovers --- lib/Repository/BlobIndex.php | 38 +++++++---- tests/Unit/Repository/IndexListingTest.php | 76 ++++++++++++++++++---- 2 files changed, 91 insertions(+), 23 deletions(-) diff --git a/lib/Repository/BlobIndex.php b/lib/Repository/BlobIndex.php index fc8e844..e0d9c87 100644 --- a/lib/Repository/BlobIndex.php +++ b/lib/Repository/BlobIndex.php @@ -99,7 +99,8 @@ final class BlobIndex { $this->downloaded = 0; $order = []; // pack ids in listing order: where a blob is in two packs, the last listed wins $fetch = []; - foreach ($this->backend->listFiles('index') as $path) { + $known = $hadIndex || $cached !== [] || ($verify && ($this->cache?->all($this->repositoryId) ?? []) !== []); + foreach ($this->listIndex($known) as $path) { $packId = basename($path); $present[$packId] = true; $order[] = $packId; @@ -107,13 +108,7 @@ final class BlobIndex { $fetch[] = $path; } } - // A repository never loses all its index files: an empty listing is a connection that - // failed quietly. Taken as is, it would empty the index and the cache (or make a check - // report every blob missing, or a cleanup wipe the cache). - if ($present === [] && ($hadIndex || $cached !== [] || ($verify && ($this->cache?->all($this->repositoryId) ?? []) !== []))) { - throw new BackendException('The location listed no index files, while some are known: not trusting that listing'); - } - // only now: an empty listing that was refused above leaves the index as it was + // only now: an empty listing that listIndex() refused leaves the index as it was $this->blobs = []; $this->loaded = $present; // The rest is downloaded several at a time where the location allows it: on a large @@ -213,15 +208,12 @@ final class BlobIndex { public function refresh(): void { $fetch = []; $listed = 0; - foreach ($this->backend->listFiles('index') as $path) { + foreach ($this->listIndex($this->loaded !== []) as $path) { $listed++; if (!isset($this->loaded[basename($path)])) { $fetch[] = $path; } } - if ($listed === 0 && $this->loaded !== []) { - throw new BackendException('The location listed no index files, while ' . count($this->loaded) . ' are known: not trusting that listing'); - } $toCache = []; foreach ($this->backend->getMany($fetch) as $path => $data) { $packId = basename($path); @@ -237,6 +229,28 @@ final class BlobIndex { } } + /** + * The index files on the location. An empty listing while index files are known ($known) is + * usually a connection that failed quietly; taken as is, it would empty the index and the + * cache, make a check report every blob missing, or make a cleanup wipe the cache. So the + * connection is checked (the repository's config must be there) and the folder listed once + * more: empty again on a working connection, the repository really is empty (another server + * removed its last snapshot and cleaned up). + * + * @return list + * @throws BackendException when the location does not answer as it should + */ + private function listIndex(bool $known): array { + $paths = $this->backend->listFiles('index'); + if ($paths !== [] || !$known) { + return $paths; + } + if (!$this->backend->exists('config')) { + throw new BackendException('The location listed no index files and has no repository config: not trusting that listing'); + } + return $this->backend->listFiles('index'); + } + /** @return array> packId => blobs currently located in that pack */ public function packs(): array { $packs = []; diff --git a/tests/Unit/Repository/IndexListingTest.php b/tests/Unit/Repository/IndexListingTest.php index b014fb4..a0f3f97 100644 --- a/tests/Unit/Repository/IndexListingTest.php +++ b/tests/Unit/Repository/IndexListingTest.php @@ -38,6 +38,10 @@ class IndexListingTest extends TestCase { $this->snapshot = $repo->backupDirectory($this->src)['snapshot']; } + private static function cache(): IIndexCache { + return indexCache(); + } + public function testARestoreGoesOnAfterAPackReadFailedAndTheListingCameBackEmpty(): void { $backend = new FlakyBackend(new LocalBackend($this->root)); $repo = Repository::openWithKey($backend, $this->keys); @@ -68,7 +72,7 @@ class IndexListingTest extends TestCase { public function testACheckWithAnEmptyListingIsAnErrorNotEveryBlobMissing(): void { $backend = new FlakyBackend(new LocalBackend($this->root)); $repo = Repository::openWithKey($backend, $this->keys); - $backend->emptyIndexListings = 1; + $backend->dead = true; $this->expectException(BackendException::class); $this->expectExceptionMessage('listed no index files'); @@ -86,21 +90,54 @@ class IndexListingTest extends TestCase { $this->assertSame(0, $index->downloaded, 'every index file is loaded already, also those whose blobs are elsewhere too'); } + public function testARestoreGoesOnWhenTheLocationCannotBeListedAfterAFailedRead(): void { + $backend = new FlakyBackend(new LocalBackend($this->root)); + $repo = Repository::openWithKey($backend, $this->keys); + $backend->failPackReads = 1; + $backend->dead = true; // listing and config look gone; reading packs works again + $target = $this->tempDir(); + + $this->assertSame(12, $repo->restore($this->snapshot, $target)); + + $this->assertSame(self::hashTree($this->src), self::hashTree($target)); + } + + public function testAnEmptyListingThatWasAHiccupIsListedAgain(): void { + $cache = self::cache(); + $backend = new FlakyBackend(new LocalBackend($this->root)); + Repository::openWithKey($backend, $this->keys, $cache); + $cached = $cache->data; + $backend->emptyIndexListings = 1; + + $again = Repository::openWithKey($backend, $this->keys, $cache); + + $this->assertSame($cached, $cache->data); + $this->assertSame(12, $again->restore($this->snapshot, $this->tempDir())); + } + + public function testARepositoryThatReallyBecameEmptyIsTakenAsEmpty(): void { + $cache = self::cache(); + $backend = new FlakyBackend(new LocalBackend($this->root)); + Repository::openWithKey($backend, $this->keys, $cache); + $this->assertNotSame([], array_filter($cache->data)); + // another server forgot the last snapshot and cleaned up: no packs and index files left + foreach (array_merge($backend->list('index'), $backend->list('packs')) as $path) { + $backend->delete($path); + } + + Repository::openWithKey($backend, $this->keys, $cache); + + $this->assertSame([], array_filter($cache->data), 'the stale cache rows are gone'); + } + public function testOpeningWithAnEmptyListingKeepsTheCachedIndex(): void { - $cache = new class implements IIndexCache { - public array $data = []; - public function all(string $r): array { return $this->data[$r] ?? []; } - public function put(string $r, string $p, array $e): void { $this->data[$r][$p] = $e; } - public function remove(string $r, array $ps): void { foreach ($ps as $p) { unset($this->data[$r][$p]); } } - public function putMany(string $r, array $packs): void { foreach ($packs as $p => $e) { $this->put($r, (string)$p, $e); } } - public function replaceAll(string $r, array $packs): void { $this->data[$r] = []; $this->putMany($r, $packs); } - }; + $cache = self::cache(); $backend = new FlakyBackend(new LocalBackend($this->root)); Repository::openWithKey($backend, $this->keys, $cache); $cached = $cache->data; $this->assertNotSame([], $cached); - $backend->emptyIndexListings = 1; + $backend->dead = true; try { Repository::openWithKey($backend, $this->keys, $cache); $this->fail('an empty listing of a repository with an index must not be taken as is'); @@ -111,12 +148,26 @@ class IndexListingTest extends TestCase { } } +/** @internal */ +function indexCache(): IIndexCache { + return new class implements IIndexCache { + public array $data = []; + public function all(string $r): array { return $this->data[$r] ?? []; } + public function put(string $r, string $p, array $e): void { $this->data[$r][$p] = $e; } + public function remove(string $r, array $ps): void { foreach ($ps as $p) { unset($this->data[$r][$p]); } } + public function putMany(string $r, array $packs): void { foreach ($packs as $p => $e) { $this->put($r, (string)$p, $e); } } + public function replaceAll(string $r, array $packs): void { $this->data[$r] = []; $this->putMany($r, $packs); } + }; +} + /** A location whose pack reads fail, and whose index listings come back empty, a set number of times. */ final class FlakyBackend implements IBackend { use \OCA\NgBackup\Backend\GetsOneByOne; public int $failPackReads = 0; public int $emptyIndexListings = 0; + /** The connection is gone: also the repository config looks missing. */ + public bool $dead = false; public function __construct( private IBackend $inner, @@ -144,7 +195,7 @@ final class FlakyBackend implements IBackend { } public function exists(string $path): bool { - return $this->inner->exists($path); + return !$this->dead && $this->inner->exists($path); } public function move(string $from, string $to): void { @@ -156,6 +207,9 @@ final class FlakyBackend implements IBackend { } public function listFiles(string $dir): array { + if ($dir === 'index' && $this->dead) { + return []; + } if ($dir === 'index' && $this->emptyIndexListings > 0) { $this->emptyIndexListings--; return []; From d6234163069e4ad44f6885234ac0fd36edcd292b Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 20:49:32 +0200 Subject: [PATCH 10/19] Index: an empty listing is taken only when known index files are gone too --- lib/Repository/BlobIndex.php | 33 ++++++++++++++-------- tests/Unit/Repository/IndexListingTest.php | 16 +++++++++++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/lib/Repository/BlobIndex.php b/lib/Repository/BlobIndex.php index e0d9c87..a14dd2c 100644 --- a/lib/Repository/BlobIndex.php +++ b/lib/Repository/BlobIndex.php @@ -99,7 +99,7 @@ final class BlobIndex { $this->downloaded = 0; $order = []; // pack ids in listing order: where a blob is in two packs, the last listed wins $fetch = []; - $known = $hadIndex || $cached !== [] || ($verify && ($this->cache?->all($this->repositoryId) ?? []) !== []); + $known = array_keys($this->loaded + $cached + ($verify ? ($this->cache?->all($this->repositoryId) ?? []) : [])); foreach ($this->listIndex($known) as $path) { $packId = basename($path); $present[$packId] = true; @@ -208,7 +208,7 @@ final class BlobIndex { public function refresh(): void { $fetch = []; $listed = 0; - foreach ($this->listIndex($this->loaded !== []) as $path) { + foreach ($this->listIndex(array_keys($this->loaded)) as $path) { $listed++; if (!isset($this->loaded[basename($path)])) { $fetch[] = $path; @@ -230,25 +230,36 @@ final class BlobIndex { } /** - * The index files on the location. An empty listing while index files are known ($known) is - * usually a connection that failed quietly; taken as is, it would empty the index and the - * cache, make a check report every blob missing, or make a cleanup wipe the cache. So the - * connection is checked (the repository's config must be there) and the folder listed once - * more: empty again on a working connection, the repository really is empty (another server - * removed its last snapshot and cleaned up). + * The index files on the location. An empty listing while index files are known ($known, + * pack ids) is usually a connection that failed quietly; taken as is, it would empty the + * index and the cache, make a check report every blob missing, or make a cleanup wipe the + * cache. So the connection is checked (the repository's config must be there) and the folder + * listed once more. Empty again, a few of the known index files are looked up one by one: + * only when they are gone too is the repository really empty (another server removed its + * last snapshot and cleaned up). * + * @param list $known * @return list * @throws BackendException when the location does not answer as it should */ - private function listIndex(bool $known): array { + private function listIndex(array $known): array { $paths = $this->backend->listFiles('index'); - if ($paths !== [] || !$known) { + if ($paths !== [] || $known === []) { return $paths; } if (!$this->backend->exists('config')) { throw new BackendException('The location listed no index files and has no repository config: not trusting that listing'); } - return $this->backend->listFiles('index'); + $paths = $this->backend->listFiles('index'); + if ($paths !== []) { + return $paths; + } + foreach (array_slice($known, 0, 3) as $packId) { + if ($this->backend->exists('index/' . $packId)) { + throw new BackendException('The location listed no index files, while index/' . $packId . ' is there: not trusting that listing'); + } + } + return []; } /** @return array> packId => blobs currently located in that pack */ diff --git a/tests/Unit/Repository/IndexListingTest.php b/tests/Unit/Repository/IndexListingTest.php index a0f3f97..1fade9a 100644 --- a/tests/Unit/Repository/IndexListingTest.php +++ b/tests/Unit/Repository/IndexListingTest.php @@ -115,6 +115,22 @@ class IndexListingTest extends TestCase { $this->assertSame(12, $again->restore($this->snapshot, $this->tempDir())); } + public function testAListingThatStaysEmptyWhileIndexFilesAreThereIsNotTaken(): void { + $cache = self::cache(); + $backend = new FlakyBackend(new LocalBackend($this->root)); + Repository::openWithKey($backend, $this->keys, $cache); + $cached = $cache->data; + $backend->emptyIndexListings = 5; // the config answers, the listing keeps lying + + try { + Repository::openWithKey($backend, $this->keys, $cache); + $this->fail('index files are there'); + } catch (BackendException $e) { + $this->assertStringContainsString('is there', $e->getMessage()); + } + $this->assertSame($cached, $cache->data); + } + public function testARepositoryThatReallyBecameEmptyIsTakenAsEmpty(): void { $cache = self::cache(); $backend = new FlakyBackend(new LocalBackend($this->root)); From 8eb6c4c0334d409d444e3e49dc0e15d1e81f0bb1 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 20:56:45 +0200 Subject: [PATCH 11/19] Index: check known index files spread over the list --- lib/Repository/BlobIndex.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/Repository/BlobIndex.php b/lib/Repository/BlobIndex.php index a14dd2c..6d7e7e2 100644 --- a/lib/Repository/BlobIndex.php +++ b/lib/Repository/BlobIndex.php @@ -234,9 +234,10 @@ final class BlobIndex { * pack ids) is usually a connection that failed quietly; taken as is, it would empty the * index and the cache, make a check report every blob missing, or make a cleanup wipe the * cache. So the connection is checked (the repository's config must be there) and the folder - * listed once more. Empty again, a few of the known index files are looked up one by one: - * only when they are gone too is the repository really empty (another server removed its - * last snapshot and cleaned up). + * listed once more. Empty again, three of the known index files are looked up one by one: + * only when they are gone too is the repository taken as empty (another server removed its + * last snapshot and cleaned up). Even when that is wrong, nothing on the location is lost: + * only this index and the cache are empty until the next load. * * @param list $known * @return list @@ -254,7 +255,10 @@ final class BlobIndex { if ($paths !== []) { return $paths; } - foreach (array_slice($known, 0, 3) as $packId) { + // spread over the list: first, middle and last (another server's cleanup removes packs + // here and there, not in this order) + $n = count($known); + foreach (array_unique([$known[0], $known[intdiv($n, 2)], $known[$n - 1]]) as $packId) { if ($this->backend->exists('index/' . $packId)) { throw new BackendException('The location listed no index files, while index/' . $packId . ' is there: not trusting that listing'); } From 827e46381e3ec6478c5b0bf4489c46195a25f83a Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 22:08:14 +0200 Subject: [PATCH 12/19] Log a snapshot that cannot be adopted when a location is added; full-restore test prints what target:add and backup:list said when the snapshot is missing --- lib/Service/TargetService.php | 3 ++- tests/integration/full-restore-roundtrip.sh | 10 ++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index abb8989..4fd4922 100644 --- a/lib/Service/TargetService.php +++ b/lib/Service/TargetService.php @@ -228,7 +228,8 @@ final class TargetService { $s->setFiles((int)($meta['stats']['files'] ?? 0)); $s->setBytes((int)($meta['stats']['bytes'] ?? 0)); $this->snapshots->insert($s); - } catch (\Throwable) { + } catch (\Throwable $e) { + Server::get(\Psr\Log\LoggerInterface::class)->warning('NG Backup: snapshot ' . $snapshotId . ' on ' . $target->getName() . ' not listed: ' . $e->getMessage(), ['exception' => $e]); } } } diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index 10a1751..13bcb28 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -146,8 +146,14 @@ docker cp /tmp/ngb-frt-pass ngb-frt-restore:/tmp/pass ok "fresh restore instance up" run_on ngb-frt-restore backup:key:import /tmp/kit.json --passphrase-file=/tmp/pass >/dev/null || fail "key import" -run_on ngb-frt-restore backup:target:add offsite "${LOCATION[@]}" --path=repo >/dev/null || fail "target add on the restore instance" -run_on ngb-frt-restore backup:list offsite | grep -q "$SNAP" || fail "the origin's snapshot is not listed after connecting the location" +ADD_OUT=$(run_on ngb-frt-restore backup:target:add offsite "${LOCATION[@]}" --path=repo 2>&1) || { printf '%s\n' "$ADD_OUT"; fail "target add on the restore instance"; } +LIST_OUT=$(run_on ngb-frt-restore backup:list offsite 2>&1) +printf '%s\n' "$LIST_OUT" | grep -q "$SNAP" || { + printf 'target:add said:\n%s\nbackup:list said:\n%s\n' "$ADD_OUT" "$LIST_OUT" + docker exec ngb-frt-restore sh -c 'grep -h ng_backup /var/www/html/data/nextcloud.log | tail -5' || true + docker exec ngb-frt-restore sh -c 'ls -la /mnt/shared/repo /mnt/shared/repo/snapshots' || true + fail "the origin's snapshot is not listed after connecting the location" +} ok "recovery kit imported, existing repository adopted, snapshot visible" RESTORE_OPTS=(--no-interaction --install-apps) From 9bc4e4a4ab83465c65c674af796e4a1fff580f7b Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 22:25:48 +0200 Subject: [PATCH 13/19] Never create a repository where backup data lies but no config could be read --- CHANGELOG.md | 3 +++ lib/Repository/Repository.php | 8 ++++++++ tests/Unit/Repository/RepositoryTest.php | 14 ++++++++++++++ 3 files changed, 25 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e1cd1c5..b454a3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,9 @@ stopped after 13 hours with "Blob not in index"; the backup itself was fine. backups record no ID counters, and they stayed at 1. Every counter of a restored table now goes after its data. Found in a disaster recovery simulation of 138 GB from MariaDB to PostgreSQL, where all 15,772 files and every count were identical. +- Adding a location whose repository config could not be read, while backup data is there, + no longer creates a new repository over it: it stops with an explanation. A snapshot that + cannot be taken over when a location is added is now logged instead of left out silently. - An SFTP connection that a Hetzner Storage Box dropped without phpseclib noticing is now recognised and replaced: the check asks the server and counts its answer. - The extra connection a full restore reads ahead on is replaced when it breaks (up to 20 times), diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index b761dcf..8408e38 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -72,6 +72,14 @@ final class Repository { if ($backend->exists('config')) { throw new RepositoryException('A repository already exists at this location'); } + // No config, but backup data: the config is missing (or could not be seen just now, a + // connection that failed). A new repository here would write a config over the one that + // opens this data; refuse instead. + foreach (['index', 'snapshots'] as $dir) { + if ($backend->listFiles($dir) !== []) { + throw new RepositoryException("This location holds backup data ($dir/) but no repository config could be read: not creating a new repository over it. Check the connection and the folder; a missing config can be restored from the recovery kit."); + } + } $id = bin2hex(random_bytes(16)); $config = ['format' => self::FORMAT, 'id' => $id, 'created' => gmdate('c'), 'key' => $wrappedKey, 'check' => base64_encode((new StreamCipher($keys))->encryptString($id, 'config-check'))]; diff --git a/tests/Unit/Repository/RepositoryTest.php b/tests/Unit/Repository/RepositoryTest.php index b8710d6..49ee92f 100644 --- a/tests/Unit/Repository/RepositoryTest.php +++ b/tests/Unit/Repository/RepositoryTest.php @@ -99,6 +99,20 @@ class RepositoryTest extends TestCase { Repository::init($this->backend, 'pw'); } + public function testInitRefusesALocationWithBackupDataButNoConfig(): void { + $repo = Repository::init($this->backend, 'pw'); + $repo->backupDirectory($this->src); + $this->backend->delete('config'); // missing, or not seen for a moment + + try { + Repository::init($this->backend, 'pw'); + $this->fail('a new repository over existing backup data'); + } catch (RepositoryException $e) { + $this->assertStringContainsString('holds backup data', $e->getMessage()); + } + $this->assertFalse($this->backend->exists('config'), 'no new config written'); + } + public function testTamperedPackIsDetected(): void { $repo = Repository::init($this->backend, 'pw'); $r = $repo->backupDirectory($this->src); From 2616b6e5438251922fef1dde5de83b57351a0796 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 21 Oct 2026 22:42:43 +0200 Subject: [PATCH 14/19] Init guard also counts snapshots in the trash and the catalog as backup data --- lib/Repository/Repository.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 8408e38..2146e20 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -75,7 +75,7 @@ final class Repository { // No config, but backup data: the config is missing (or could not be seen just now, a // connection that failed). A new repository here would write a config over the one that // opens this data; refuse instead. - foreach (['index', 'snapshots'] as $dir) { + foreach (['index', 'snapshots', 'trash/snapshots', 'catalog'] as $dir) { if ($backend->listFiles($dir) !== []) { throw new RepositoryException("This location holds backup data ($dir/) but no repository config could be read: not creating a new repository over it. Check the connection and the folder; a missing config can be restored from the recovery kit."); } From bdcb4f0caec53753e32eb67476a99523fcb74c5f Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Thu, 22 Oct 2026 00:31:34 +0200 Subject: [PATCH 15/19] Full restore runs in maintenance mode, switched off again at the end (also after an error) --- CHANGELOG.md | 3 +++ docs/DISASTER-RECOVERY.md | 3 ++- lib/Command/RestoreFull.php | 1 + lib/Service/RestoreFullService.php | 21 +++++++++++++++++++++ tests/integration/full-restore-roundtrip.sh | 4 +++- 5 files changed, 30 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b454a3c..84ccb29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,9 @@ stopped after 13 hours with "Blob not in index"; the backup itself was fine. backups record no ID counters, and they stayed at 1. Every counter of a restored table now goes after its data. Found in a disaster recovery simulation of 138 GB from MariaDB to PostgreSQL, where all 15,772 files and every count were identical. +- A full restore now runs in maintenance mode, switched off again at the end (also after an + error): a background job, a page visit or a user writing into the instance meanwhile could + collide with restored rows ("duplicate key") or be lost. - Adding a location whose repository config could not be read, while backup data is there, no longer creates a new repository over it: it stops with an explanation. A snapshot that cannot be taken over when a location is added is now logged instead of left out silently. diff --git a/docs/DISASTER-RECOVERY.md b/docs/DISASTER-RECOVERY.md index d1f48a1..acef877 100644 --- a/docs/DISASTER-RECOVERY.md +++ b/docs/DISASTER-RECOVERY.md @@ -94,7 +94,8 @@ tables that do not exist here, install the apps they belong to and run it again, out with `--skip-missing-tables`. **8. Let the restore run.** It restores the database, then the data directory, and takes hours for -a large backup. If it stops halfway, fix the cause and run the same command again with `--force`. +a large backup. Nextcloud is in maintenance mode meanwhile, so nobody and no background job +writes into it; the restore switches it off again at the end, also after an error. If it stops halfway, fix the cause and run the same command again with `--force`. If occ then says that Nextcloud or one of the apps requires an upgrade, the database is already the old server's: run `occ upgrade` first, then the same command with `--force`. diff --git a/lib/Command/RestoreFull.php b/lib/Command/RestoreFull.php index 9774e7a..e8867c1 100644 --- a/lib/Command/RestoreFull.php +++ b/lib/Command/RestoreFull.php @@ -88,6 +88,7 @@ HELP); $this->installApp($app, $output); } } + $output->writeln('Nextcloud is in maintenance mode while the restore runs (switched off again at the end, also after an error).'); $result = $this->restoreFull->restoreFull($target, $snapshot, $skipMissingTables); } catch (\Throwable $e) { $output->writeln('' . $e->getMessage() . ''); diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index 9a0e279..4b036dd 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -30,6 +30,8 @@ final class RestoreFullService { private const CONNECTION_KEYS = [ 'dbtype', 'dbhost', 'dbport', 'dbname', 'dbuser', 'dbpassword', 'dbpersistent', 'dbtableprefix', 'dbdriveroptions', 'datadirectory', + // set by restoreFull() itself for the length of the restore + 'maintenance', ]; public function __construct( @@ -91,6 +93,25 @@ final class RestoreFullService { if (!$check['ok']) { throw new \RuntimeException($this->describeBlockers($check)); } + // Maintenance mode for the whole restore: background jobs (cron.php stops in maintenance + // mode), page visits (AJAX cron) and users must not write into tables or the data + // directory while they are being replaced; a row written in between would collide with a + // restored one or be silently lost. Switched off again at the end, also after an error: + // occ only offers app commands outside maintenance mode, and the guide's next step is + // to run the same restore again with --force. + $wasInMaintenance = $this->config->getSystemValueBool('maintenance', false); + $this->config->setSystemValue('maintenance', true); + try { + return $this->restoreInMaintenance($target, $snapshotId, $skipMissingTables, $check); + } finally { + if (!$wasInMaintenance) { + $this->config->setSystemValue('maintenance', false); + } + } + } + + /** @return array{dbReport:array, dataFiles:int, configKeys:int, dbConsistent:bool, missingApps:list} */ + private function restoreInMaintenance(Target $target, string $snapshotId, bool $skipMissingTables, array $check): array { return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId, $skipMissingTables, $check) { $beat = LeaseService::throttled($refresh); $repo = $this->targets->repository($target); diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index 13bcb28..f15e311 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -172,7 +172,9 @@ OUT=$(run_on ngb-frt-restore backup:restore:full offsite "$SNAP" "${RESTORE_OPTS docker exec ngb-frt-restore sh -c 'tail -c 6000 /var/www/html/data/nextcloud.log' || true fail "restore:full" } -ok "backup:restore:full completed" +printf '%s\n' "$OUT" | grep -q "maintenance mode while the restore runs" || fail "the restore did not say it runs in maintenance mode" +run_on ngb-frt-restore status 2>&1 | grep -q "maintenance: false" || fail "maintenance mode is still on after the restore" +ok "backup:restore:full completed, in maintenance mode and out of it again" # Everything below must work in a FRESH occ process (a new PHP process per docker exec), since # earlier bugs here only showed up after config.php's secret was rewritten mid-restore. From 7ed53ad0eb49cf05c39a772b9700600cdcbc47c6 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Thu, 22 Oct 2026 00:36:58 +0200 Subject: [PATCH 16/19] Full restore: after an error maintenance mode stays on, with the one line to switch it off and run again; maintenance that was on before stays on --- CHANGELOG.md | 8 +++++--- docs/DISASTER-RECOVERY.md | 7 ++++++- lib/Command/RestoreFull.php | 13 +++++++++++-- lib/Service/RestoreFullService.php | 19 +++++++++---------- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 84ccb29..4d3997c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,9 +23,11 @@ stopped after 13 hours with "Blob not in index"; the backup itself was fine. backups record no ID counters, and they stayed at 1. Every counter of a restored table now goes after its data. Found in a disaster recovery simulation of 138 GB from MariaDB to PostgreSQL, where all 15,772 files and every count were identical. -- A full restore now runs in maintenance mode, switched off again at the end (also after an - error): a background job, a page visit or a user writing into the instance meanwhile could - collide with restored rows ("duplicate key") or be lost. +- A full restore now runs in maintenance mode: a background job, a page visit or a user writing + into the instance meanwhile could collide with restored rows ("duplicate key") or be lost. It + is switched off when the restore completes; after an error it stays on, and the message gives + the one line to switch it off and run the restore again. Maintenance mode that was on before + the restore stays on. - Adding a location whose repository config could not be read, while backup data is there, no longer creates a new repository over it: it stops with an explanation. A snapshot that cannot be taken over when a location is added is now logged instead of left out silently. diff --git a/docs/DISASTER-RECOVERY.md b/docs/DISASTER-RECOVERY.md index acef877..99db900 100644 --- a/docs/DISASTER-RECOVERY.md +++ b/docs/DISASTER-RECOVERY.md @@ -95,7 +95,12 @@ out with `--skip-missing-tables`. **8. Let the restore run.** It restores the database, then the data directory, and takes hours for a large backup. Nextcloud is in maintenance mode meanwhile, so nobody and no background job -writes into it; the restore switches it off again at the end, also after an error. If it stops halfway, fix the cause and run the same command again with `--force`. +writes into it; the restore switches it off when it completes. (If you had switched maintenance +mode on yourself before, it stays on: switch it off with `occ maintenance:mode --off` when ready.) + +If it stops halfway, maintenance mode stays on (the instance may be partly restored). Fix the cause, +then switch maintenance mode off and run the same command again with `--force`, in one line: +`occ maintenance:mode --off && occ backup:restore:full --install-apps --force`. If occ then says that Nextcloud or one of the apps requires an upgrade, the database is already the old server's: run `occ upgrade` first, then the same command with `--force`. diff --git a/lib/Command/RestoreFull.php b/lib/Command/RestoreFull.php index e8867c1..3cf0b8c 100644 --- a/lib/Command/RestoreFull.php +++ b/lib/Command/RestoreFull.php @@ -88,11 +88,17 @@ HELP); $this->installApp($app, $output); } } - $output->writeln('Nextcloud is in maintenance mode while the restore runs (switched off again at the end, also after an error).'); + $output->writeln('Nextcloud is in maintenance mode while the restore runs; it is switched off when the restore completes.'); $result = $this->restoreFull->restoreFull($target, $snapshot, $skipMissingTables); } catch (\Throwable $e) { $output->writeln('' . $e->getMessage() . ''); - $output->writeln('If this happened after the restore had started, the instance may be partly restored: fix the cause and run the same command again with --force.'); + if (\OCP\Server::get(\OCP\IConfig::class)->getSystemValueBool('maintenance', false)) { + $output->writeln('The restore had started: the instance may be partly restored, and maintenance mode stays on so that nobody uses it like that.'); + $output->writeln('Fix the cause, then switch maintenance mode off and run the same command again with --force, in one line:'); + $output->writeln(' occ maintenance:mode --off && occ backup:restore:full ' . (string)$input->getArgument('target') . ' ' . (string)$input->getArgument('snapshot') . ' --force ...'); + } else { + $output->writeln('If this happened after the restore had started, the instance may be partly restored: fix the cause and run the same command again with --force.'); + } $output->writeln('If occ then says that Nextcloud needs an upgrade (the database is already the old server\'s), run occ upgrade first.'); return 1; } @@ -122,6 +128,9 @@ HELP); if (!$result['dbConsistent']) { $output->writeln('Warning: this snapshot\'s database dump was made over several steps (web cron), so it is not one consistent point in time. Check the instance carefully; prefer a snapshot made with system cron.'); } + if ($result['maintenanceLeftOn']) { + $output->writeln('Maintenance mode was on before the restore and stays on: switch it off with occ maintenance:mode --off when the instance is ready.'); + } $output->writeln('Next: check trusted_domains and overwritehost/overwriteprotocol match this server, then occ maintenance:repair.'); return 0; } diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index 4b036dd..cd3a1fb 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -87,7 +87,7 @@ final class RestoreFullService { ]; } - /** @return array{dbReport:array, dataFiles:int, configKeys:int, dbConsistent:bool, missingApps:list} */ + /** @return array{dbReport:array, dataFiles:int, configKeys:int, dbConsistent:bool, missingApps:list, maintenanceLeftOn:bool} */ public function restoreFull(Target $target, string $snapshotId, bool $skipMissingTables = false): array { $check = $this->checkCompatibility($target, $snapshotId, $skipMissingTables); if (!$check['ok']) { @@ -96,18 +96,17 @@ final class RestoreFullService { // Maintenance mode for the whole restore: background jobs (cron.php stops in maintenance // mode), page visits (AJAX cron) and users must not write into tables or the data // directory while they are being replaced; a row written in between would collide with a - // restored one or be silently lost. Switched off again at the end, also after an error: - // occ only offers app commands outside maintenance mode, and the guide's next step is - // to run the same restore again with --force. + // restored one or be silently lost. Switched off after a restore that completed. After an + // error it stays on: the instance may be partly restored, and must not be used like that. + // occ only offers app commands outside maintenance mode, so the admin switches it off + // right before running the restore again (see RestoreFull). Already on before: left on. $wasInMaintenance = $this->config->getSystemValueBool('maintenance', false); $this->config->setSystemValue('maintenance', true); - try { - return $this->restoreInMaintenance($target, $snapshotId, $skipMissingTables, $check); - } finally { - if (!$wasInMaintenance) { - $this->config->setSystemValue('maintenance', false); - } + $result = $this->restoreInMaintenance($target, $snapshotId, $skipMissingTables, $check); + if (!$wasInMaintenance) { + $this->config->setSystemValue('maintenance', false); } + return $result + ['maintenanceLeftOn' => $wasInMaintenance]; } /** @return array{dbReport:array, dataFiles:int, configKeys:int, dbConsistent:bool, missingApps:list} */ From 61e2a28b670ee83102e1ad05dd2f06839bc1364a Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Thu, 22 Oct 2026 00:53:08 +0200 Subject: [PATCH 17/19] Full restore: maintenance mode on before the checks (off again when they refuse); the printed retry command is complete --- docs/DISASTER-RECOVERY.md | 6 ++++-- lib/Command/RestoreFull.php | 5 ++++- lib/Service/RestoreFullService.php | 20 ++++++++++++++++---- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/docs/DISASTER-RECOVERY.md b/docs/DISASTER-RECOVERY.md index 99db900..9b5d179 100644 --- a/docs/DISASTER-RECOVERY.md +++ b/docs/DISASTER-RECOVERY.md @@ -99,8 +99,10 @@ writes into it; the restore switches it off when it completes. (If you had switc mode on yourself before, it stays on: switch it off with `occ maintenance:mode --off` when ready.) If it stops halfway, maintenance mode stays on (the instance may be partly restored). Fix the cause, -then switch maintenance mode off and run the same command again with `--force`, in one line: -`occ maintenance:mode --off && occ backup:restore:full --install-apps --force`. +then switch maintenance mode off and run the restore again with `--force`, in one line (the message +prints it): `occ maintenance:mode --off && occ backup:restore:full --force`. +Leave out `--install-apps` this time: the apps are installed already, and the instance is then open +only for the seconds until the restore has switched maintenance mode on again. If occ then says that Nextcloud or one of the apps requires an upgrade, the database is already the old server's: run `occ upgrade` first, then the same command with `--force`. diff --git a/lib/Command/RestoreFull.php b/lib/Command/RestoreFull.php index 3cf0b8c..05d63ed 100644 --- a/lib/Command/RestoreFull.php +++ b/lib/Command/RestoreFull.php @@ -95,7 +95,10 @@ HELP); if (\OCP\Server::get(\OCP\IConfig::class)->getSystemValueBool('maintenance', false)) { $output->writeln('The restore had started: the instance may be partly restored, and maintenance mode stays on so that nobody uses it like that.'); $output->writeln('Fix the cause, then switch maintenance mode off and run the same command again with --force, in one line:'); - $output->writeln(' occ maintenance:mode --off && occ backup:restore:full ' . (string)$input->getArgument('target') . ' ' . (string)$input->getArgument('snapshot') . ' --force ...'); + $again = 'occ backup:restore:full ' . escapeshellarg((string)$input->getArgument('target')) . ' ' . escapeshellarg((string)$input->getArgument('snapshot')) + . ($input->getOption('skip-missing-tables') ? ' --skip-missing-tables' : '') . ' --force --no-interaction'; + $output->writeln(' occ maintenance:mode --off && ' . $again); + $output->writeln('(Without --install-apps: the apps are installed already, and the instance is open again only for the seconds until the restore starts.)'); } else { $output->writeln('If this happened after the restore had started, the instance may be partly restored: fix the cause and run the same command again with --force.'); } diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index cd3a1fb..27e8edd 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -89,10 +89,6 @@ final class RestoreFullService { /** @return array{dbReport:array, dataFiles:int, configKeys:int, dbConsistent:bool, missingApps:list, maintenanceLeftOn:bool} */ public function restoreFull(Target $target, string $snapshotId, bool $skipMissingTables = false): array { - $check = $this->checkCompatibility($target, $snapshotId, $skipMissingTables); - if (!$check['ok']) { - throw new \RuntimeException($this->describeBlockers($check)); - } // Maintenance mode for the whole restore: background jobs (cron.php stops in maintenance // mode), page visits (AJAX cron) and users must not write into tables or the data // directory while they are being replaced; a row written in between would collide with a @@ -100,8 +96,24 @@ final class RestoreFullService { // error it stays on: the instance may be partly restored, and must not be used like that. // occ only offers app commands outside maintenance mode, so the admin switches it off // right before running the restore again (see RestoreFull). Already on before: left on. + // On before the checks below, so that a run again after an error leaves the partly + // restored instance open for seconds at most; off again when they refuse (nothing changed). $wasInMaintenance = $this->config->getSystemValueBool('maintenance', false); $this->config->setSystemValue('maintenance', true); + try { + $check = $this->checkCompatibility($target, $snapshotId, $skipMissingTables); + } catch (\Throwable $e) { + if (!$wasInMaintenance) { + $this->config->setSystemValue('maintenance', false); + } + throw $e; + } + if (!$check['ok']) { + if (!$wasInMaintenance) { + $this->config->setSystemValue('maintenance', false); + } + throw new \RuntimeException($this->describeBlockers($check)); + } $result = $this->restoreInMaintenance($target, $snapshotId, $skipMissingTables, $check); if (!$wasInMaintenance) { $this->config->setSystemValue('maintenance', false); From 2eabdf44d517bd82c86b5d7a77d6660ab097d634 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Thu, 22 Oct 2026 01:03:52 +0200 Subject: [PATCH 18/19] README and store description: background jobs must run on system cron, with how to set it up; rc5 date --- CHANGELOG.md | 6 +++++- README.md | 20 ++++++++++++++++++++ appinfo/info.xml | 1 + 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d3997c..e4db1a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,15 @@ the app store accepts a `-beta` suffix, but Nextcloud servers then only offer th beta/daily update channel, which would hide the app from most admins. The stability stage is instead carried by the app description, the git tag, the release, and this file. -## [1.0.0-rc.5] - 2026-10-21 +## [1.0.0-rc.5] - 2026-10-22 Fifth release candidate for 1.0.0; the app version is 0.9.11. A full restore of 1 TB over SFTP stopped after 13 hours with "Blob not in index"; the backup itself was fine. +### Changed +- README and app store description: NG Backup needs background jobs on system cron, with how to + set that up (issues #13, #17). + ### Fixed - A full restore no longer stops with "Blob not in index" after a pack could not be read for a moment. Reading the index again then got an empty listing (from a connection the location had diff --git a/README.md b/README.md index db2595c..47ef3c1 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,26 @@ Need help restoring a backup after a crash? Ensembia offers paid restore assista - Restores go through Nextcloud, so overwritten files keep their old content as a version and removed files go to the trash. +## Requirements + +- **Background jobs on system cron.** NG Backup runs backups in steps from Nextcloud's + background job, so a backup only moves forward when that job runs. Choosing "Cron" under + **Administration settings → Basic settings → Background jobs** is not enough on its own: it + tells Nextcloud that the system will call it, so you also need a cron line that does. Without + it a backup stays at "running (db)", and so does one started with "Back up now". + `occ backup:run` in a terminal works without cron (it runs in the foreground). + - Regular server: `crontab -u www-data -e` and add + `*/5 * * * * php -f /var/www/nextcloud/cron.php` (your Nextcloud path). + - Docker (the official `nextcloud` image): on the host, `crontab -e` as root and add + `*/5 * * * * docker exec -u www-data php -f /var/www/html/cron.php`, or add a + service with the same image and `entrypoint: /cron.sh` to your `docker-compose.yml`. + - Nextcloud All-in-One already runs cron. + + After a few minutes, the Background jobs section shows that the last job ran just now. See the + [Nextcloud documentation on background jobs](https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/background_jobs_configuration.html). +- A storage location: S3 or compatible, SFTP, WebDAV, SMB (needs the PHP `smbclient` extension, + see below), FTP or a local path. + ## Quick start ``` diff --git a/appinfo/info.xml b/appinfo/info.xml index 536aa46..ba68b03 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -36,6 +36,7 @@ All-in-One and hosted Nextcloud. - One user (account, files, calendars, contacts) via user_migration. - The whole server onto a fresh installation (disaster recovery). +NG Backup needs Nextcloud's background jobs on system cron (a real cron line, not only the setting). Read "Known limitations" in the README before you rely on it, and test a restore. Feedback is very welcome: use the "Share your experience" link on the admin page. From fd0f786f5ffbab67db45c1395271f03211360272 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Thu, 22 Oct 2026 01:59:01 +0200 Subject: [PATCH 19/19] Full restore: a run again after an interrupted restore keeps maintenance mode on when its check refuses; retry text about apps --- docs/DISASTER-RECOVERY.md | 5 +++-- lib/Command/RestoreFull.php | 2 +- lib/Service/RestoreFullService.php | 23 +++++++++++++++------ tests/integration/full-restore-roundtrip.sh | 1 + 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/docs/DISASTER-RECOVERY.md b/docs/DISASTER-RECOVERY.md index 9b5d179..06bbb47 100644 --- a/docs/DISASTER-RECOVERY.md +++ b/docs/DISASTER-RECOVERY.md @@ -101,8 +101,9 @@ mode on yourself before, it stays on: switch it off with `occ maintenance:mode - If it stops halfway, maintenance mode stays on (the instance may be partly restored). Fix the cause, then switch maintenance mode off and run the restore again with `--force`, in one line (the message prints it): `occ maintenance:mode --off && occ backup:restore:full --force`. -Leave out `--install-apps` this time: the apps are installed already, and the instance is then open -only for the seconds until the restore has switched maintenance mode on again. +Leave out `--install-apps` this time, so that the instance is open only for the seconds until the +restore has switched maintenance mode on again; apps the first run could not install are listed at +the end of the restore, install those afterwards. If occ then says that Nextcloud or one of the apps requires an upgrade, the database is already the old server's: run `occ upgrade` first, then the same command with `--force`. diff --git a/lib/Command/RestoreFull.php b/lib/Command/RestoreFull.php index 05d63ed..6c804c6 100644 --- a/lib/Command/RestoreFull.php +++ b/lib/Command/RestoreFull.php @@ -98,7 +98,7 @@ HELP); $again = 'occ backup:restore:full ' . escapeshellarg((string)$input->getArgument('target')) . ' ' . escapeshellarg((string)$input->getArgument('snapshot')) . ($input->getOption('skip-missing-tables') ? ' --skip-missing-tables' : '') . ' --force --no-interaction'; $output->writeln(' occ maintenance:mode --off && ' . $again); - $output->writeln('(Without --install-apps: the apps are installed already, and the instance is open again only for the seconds until the restore starts.)'); + $output->writeln('(Without --install-apps, so the instance is open only for the seconds until the restore starts again. Apps the first run could not install are listed at the end: install those afterwards.)'); } else { $output->writeln('If this happened after the restore had started, the instance may be partly restored: fix the cause and run the same command again with --force.'); } diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index 27e8edd..2a1978e 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -27,6 +27,9 @@ final class RestoreFullService { * Config keys that must keep pointing at THIS installation's actual, already-working * database and data directory (set by occ maintenance:install), never at the old server's. */ + /** App setting: when a full restore started that has not completed (see restoreFull()). */ + private const INTERRUPTED_KEY = 'restore_full_started'; + private const CONNECTION_KEYS = [ 'dbtype', 'dbhost', 'dbport', 'dbname', 'dbuser', 'dbpassword', 'dbpersistent', 'dbtableprefix', 'dbdriveroptions', 'datadirectory', @@ -98,23 +101,31 @@ final class RestoreFullService { // right before running the restore again (see RestoreFull). Already on before: left on. // On before the checks below, so that a run again after an error leaves the partly // restored instance open for seconds at most; off again when they refuse (nothing changed). + // A marker in ng_backup's own app settings (which the restore keeps) says a restore started + // and did not complete: then the instance may be partly restored, and a check that refuses + // a run again must not open it. + $appConfig = \OCP\Server::get(\OCP\IAppConfig::class); + $interrupted = $appConfig->getValueInt(\OCA\NgBackup\AppInfo\Application::APP_ID, self::INTERRUPTED_KEY, 0) > 0; $wasInMaintenance = $this->config->getSystemValueBool('maintenance', false); $this->config->setSystemValue('maintenance', true); + $reopen = function () use ($wasInMaintenance, $interrupted): void { + if (!$wasInMaintenance && !$interrupted) { + $this->config->setSystemValue('maintenance', false); + } + }; try { $check = $this->checkCompatibility($target, $snapshotId, $skipMissingTables); } catch (\Throwable $e) { - if (!$wasInMaintenance) { - $this->config->setSystemValue('maintenance', false); - } + $reopen(); throw $e; } if (!$check['ok']) { - if (!$wasInMaintenance) { - $this->config->setSystemValue('maintenance', false); - } + $reopen(); throw new \RuntimeException($this->describeBlockers($check)); } + $appConfig->setValueInt(\OCA\NgBackup\AppInfo\Application::APP_ID, self::INTERRUPTED_KEY, time()); $result = $this->restoreInMaintenance($target, $snapshotId, $skipMissingTables, $check); + $appConfig->deleteKey(\OCA\NgBackup\AppInfo\Application::APP_ID, self::INTERRUPTED_KEY); if (!$wasInMaintenance) { $this->config->setSystemValue('maintenance', false); } diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index f15e311..ab79807 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -174,6 +174,7 @@ OUT=$(run_on ngb-frt-restore backup:restore:full offsite "$SNAP" "${RESTORE_OPTS } printf '%s\n' "$OUT" | grep -q "maintenance mode while the restore runs" || fail "the restore did not say it runs in maintenance mode" run_on ngb-frt-restore status 2>&1 | grep -q "maintenance: false" || fail "maintenance mode is still on after the restore" +run_on ngb-frt-restore config:app:get ng_backup restore_full_started >/dev/null 2>&1 && fail "the restore left its 'started' marker behind" ok "backup:restore:full completed, in maintenance mode and out of it again" # Everything below must work in a FRESH occ process (a new PHP process per docker exec), since