From e0b79939f9b0bdb7ef549b4f654b76ec99360344 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Sun, 18 Oct 2026 23:19:18 +0200 Subject: [PATCH 01/12] PLAN: 1.0.0 only reliability; 1.0.1 restore speed, cron warning, DR test with table structure --- PLAN.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/PLAN.md b/PLAN.md index d9bec8e..60a0198 100644 --- a/PLAN.md +++ b/PLAN.md @@ -383,6 +383,17 @@ in README "Known limitations": de gebruiker, wachtwoordzin min. 16 tekens). Geen nieuwe functies meer. - **Vóór 1.0.0 (verplicht):** volledige restore met kit op een tweede server (PVE02), alleen lezend of op een kopie van de locatie; plus een user-restore. Zonder geslaagde test geen 1.0.0. +- **1.0.0 (besluit 2026-10-18):** alleen betrouwbaarheid. rc2 uitgebracht; slaagt de grote + DR-test op VM 980 (snapshot van 12-10, met `--skip-missing-tables`), dan 1.0.0. Zo niet: fix, + rc3, en rc3 pas uitbrengen na een geslaagde volledige DR-test. +- **1.0.1:** + 1. Herstelsnelheid: bij een restore meerdere pakketten tegelijk lezen (SFTP: meerdere + verbindingen; S3 heeft al 16 tegelijk). Raakt het leespad, daarom niet in 1.0.0. + 2. Waarschuwing in de beheerpagina en `backup:status` als achtergrondjobs niet op systeem-cron + staan, en "waiting for background jobs" als een lopende backup lang geen stap zette (issue + #13). Cron zelf aanzetten kan de app niet: zonder echte cron-regel stopt dan alles. + 3. Volledige DR-test met een backup die al de tabelstructuur bevat (gemaakt met rc2 of later), + zonder `--skip-missing-tables`: dat pad is op 1 TB nog niet getest. - **1.1.0:** 1. `backup:key:import` zonder kit: de sleutel-slots van de locatie (`config`) lezen, met de kit als reserve als `config` ontbreekt of niet opent. Vergt een andere volgorde van From 8e5c8ccda45296c8ce9a13ed4367bce9088cc151 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Sun, 18 Oct 2026 23:44:00 +0200 Subject: [PATCH 02/12] Full restore: skip the ID counters of tables left out with --skip-missing-tables; test a backup made by rc.1; version 0.9.9 --- .github/workflows/ci.yml | 4 +++ CHANGELOG.md | 6 ++++ appinfo/info.xml | 2 +- lib/Db/DbDumper.php | 9 +++-- tests/integration/full-restore-roundtrip.sh | 38 +++++++++++++++++---- 5 files changed, 50 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e8df09c..dbc09a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -108,3 +108,7 @@ jobs: 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-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 + - 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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 3409abf..0244488 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ instead carried by the app description, the git tag, the release, and this file. ## [Unreleased] +### Fixed +- A full restore of a backup made before 1.0.0-rc.2 with `--skip-missing-tables` failed on + PostgreSQL with "relation ... does not exist": the left-out tables were still looked up to set + their ID counters. It now skips them. A new test restores a backup made by 1.0.0-rc.1 on + PostgreSQL and MariaDB. + ## [1.0.0-rc.2] - 2026-10-18 Second release candidate for 1.0.0; the app version is 0.9.8. Fixes from a full disaster recovery diff --git a/appinfo/info.xml b/appinfo/info.xml index efe6cc8..8763946 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), which also offers paid help with restoring a backup after a crash.]]> - 0.9.8 + 0.9.9 agpl Dillard Blom NgBackup diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index 716b63e..605fb8e 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'] ?? []); + $this->restoreSequences($manifest['sequences'] ?? [], $report['missing']); $this->resumeForeignKeys($foreignKeys); $this->db->commit(); // Foreign keys of the tables created above, now their rows are in. Not inside the @@ -573,11 +573,16 @@ final class DbDumper { return $name; } - private function restoreSequences(array $sequences): void { + /** @param list $leftOut tables not restored (--skip-missing-tables): they do not exist here */ + private function restoreSequences(array $sequences, array $leftOut = []): void { if (!$this->db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { return; } + $leftOut = array_flip($leftOut); foreach ($sequences as $info) { + if (isset($leftOut[$info['table']])) { + continue; + } $seq = $this->db->fetchOne('SELECT pg_get_serial_sequence(quote_ident(?), ?)', [$info['table'], $info['column']]); if ($seq === false || $seq === null) { continue; diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index e9c3657..a26e755 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -2,15 +2,20 @@ # 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] +# Usage: full-restore-roundtrip.sh [sqlite|pgsql|mysql] [legacy] # e.g. full-restore-roundtrip.sh 35-apache pgsql +# 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. # With pgsql or mysql the origin also has what broke real disaster recoveries: the Mail app with a synced # account (foreign keys between its tables) and Deck installed and then switched off (tables of # an app that is not enabled). The restore instance has neither; --install-apps adds Mail. set -euo pipefail IMAGE="nextcloud:${1:-35-apache}" DB="${2:-sqlite}" +LEGACY="${3:-}" APP=$(cd "$(dirname "$0")/../.." && pwd) +ORIGIN_APP="$APP" NET=ngb-frt-net SHARED=$(mktemp -d) chmod 777 "$SHARED" @@ -23,9 +28,17 @@ cleanup() { docker run --rm -v "$SHARED:/target" alpine sh -c 'rm -rf /target/*' >/dev/null 2>&1 || true rmdir "$SHARED" 2>/dev/null || true rm -f /tmp/ngb-frt-kit.json /tmp/ngb-frt-pass + [ -n "${LEGACY_DIR:-}" ] && rm -rf "$LEGACY_DIR" + return 0 } trap cleanup EXIT +if [ "$LEGACY" = legacy ]; then + LEGACY_DIR=$(mktemp -d) + curl -fsSL https://github.com/dillardblom/ng_backup/releases/download/v1.0.0-rc.1/ng_backup-0.9.7.tar.gz | tar -xz -C "$LEGACY_DIR" + ORIGIN_APP="$LEGACY_DIR/ng_backup" +fi + run_on() { docker exec -u www-data "$1" php occ "${@:2}"; } # Database settings for an instance: SQLite inside the container, or its own PostgreSQL container @@ -50,11 +63,11 @@ ORIGIN_DB=$(db_env origin) # shellcheck disable=SC2086 docker run -d --name ngb-frt-origin --network "$NET" $ORIGIN_DB \ -e NEXTCLOUD_ADMIN_USER=admin -e NEXTCLOUD_ADMIN_PASSWORD=admin \ - -v "$APP:/var/www/html/custom_apps/ng_backup:ro" -v "$SHARED:/mnt/shared" "$IMAGE" >/dev/null + -v "$ORIGIN_APP:/var/www/html/custom_apps/ng_backup:ro" -v "$SHARED:/mnt/shared" "$IMAGE" >/dev/null for i in $(seq 1 90); do run_on ngb-frt-origin status 2>/dev/null | grep -q "installed: true" && break; sleep 3; done docker exec ngb-frt-origin chown www-data:www-data /var/www/html/custom_apps run_on ngb-frt-origin app:enable ng_backup >/dev/null -ok "origin instance up ($DB)" +ok "origin instance up ($DB${LEGACY:+, NG Backup 0.9.7})" docker exec ngb-frt-origin sh -c "printf 'full restore roundtrip passphrase 2026\n' > /tmp/pass" run_on ngb-frt-origin backup:key:init --passphrase-file=/tmp/pass --label=CI >/dev/null || fail "key init" @@ -118,7 +131,16 @@ run_on ngb-frt-restore backup:target:add offsite local -a null::null -o datadir= run_on ngb-frt-restore backup:list offsite | grep -q "$SNAP" || fail "the origin's snapshot is not listed after connecting the location" ok "recovery kit imported, existing repository adopted, snapshot visible" -OUT=$(run_on ngb-frt-restore backup:restore:full offsite "$SNAP" --no-interaction --install-apps 2>&1) || { +RESTORE_OPTS=(--no-interaction --install-apps) +if [ "$LEGACY" = legacy ] && [ "$DB" != sqlite ]; then + # Without --skip-missing-tables an older backup must be refused, naming the app's tables + REFUSED=$(run_on ngb-frt-restore backup:restore:full offsite "$SNAP" "${RESTORE_OPTS[@]}" 2>&1) \ + && fail "an older backup with tables of unknown structure was restored without --skip-missing-tables" + printf '%s\n' "$REFUSED" | grep -q "oc_deck_boards" || { printf '%s\n' "$REFUSED" | tail -20; fail "the refusal does not name the missing tables"; } + ok "older backup refused without --skip-missing-tables, missing tables named" + RESTORE_OPTS+=(--skip-missing-tables) +fi +OUT=$(run_on ngb-frt-restore backup:restore:full offsite "$SNAP" "${RESTORE_OPTS[@]}" 2>&1) || { printf '%s\n' "$OUT" | tail -40 docker exec ngb-frt-restore sh -c 'tail -c 6000 /var/www/html/data/nextcloud.log' || true fail "restore:full" @@ -134,7 +156,11 @@ docker exec ngb-frt-restore test -f /var/www/html/data/frtest/files/Docs/note.tx run_on ngb-frt-restore files:scan frtest >/dev/null || fail "files:scan fails after restore" ok "database, files and ng_backup's own state are all intact and usable after the restore" -if [ "$DB" != sqlite ]; then +if [ "$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 [ "$(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 ] \ @@ -146,4 +172,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)" +echo "FULL RESTORE ROUNDTRIP PASS ($DB${LEGACY:+, legacy})" From b789d17636681d32447b7e3f6004b49386b9eb01 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Sun, 18 Oct 2026 23:47:32 +0200 Subject: [PATCH 03/12] Screenshot URLs per release tag, so the app store fetches new images --- CHANGELOG.md | 2 ++ appinfo/info.xml | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0244488..0ac6881 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ instead carried by the app description, the git tag, the release, and this file. PostgreSQL with "relation ... does not exist": the left-out tables were still looked up to set their ID counters. It now skips them. A new test restores a backup made by 1.0.0-rc.1 on PostgreSQL and MariaDB. +- The app store showed old screenshots: it keeps a copy per URL, and the URLs always pointed at + `main`. They now point at the release's tag. ## [1.0.0-rc.2] - 2026-10-18 diff --git a/appinfo/info.xml b/appinfo/info.xml index 8763946..81849a4 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -52,9 +52,9 @@ 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/main/screenshots/1-backups.png - https://raw.githubusercontent.com/dillardblom/ng_backup/main/screenshots/2-restore.png - https://raw.githubusercontent.com/dillardblom/ng_backup/main/screenshots/3-settings.png + https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.3/screenshots/1-backups.png + https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.3/screenshots/2-restore.png + https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.3/screenshots/3-settings.png sodium From ccbbf01c54988023123829f10fe0fb40f5718ea4 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 01:40:49 +0200 Subject: [PATCH 04/12] Full restore over SFTP: read packs ahead on four connections at once A restore read one pack at a time over one SFTP connection, which over a long line is bound by the round trip and by what one TCP connection gets: about 2-3 MiB/s from a Hetzner Storage Box to Canada, with PHP mostly waiting. Repository::restore() now first lists the ranges of packs the restore needs, in order (ReadAheadPlan), and the location reads them ahead into unlinked files in a spool folder in the target (IReadsAhead). Over SFTP, SftpRangeFetcher does that on four connections of its own, 16 requests in flight each, taking the answers of whichever connection has one. A range that fails, a damaged copy, or a blob that moved since the plan is read the usual way, which reports what is wrong. Other locations restore as before. On a test line with 120 ms RTT and 0.2% loss, a 1.6 GiB restore went from 5.1 to 17.8 MiB/s, all files identical. --- lib/Backend/IReadsAhead.php | 30 ++ lib/Backend/SftpRangeFetcher.php | 344 ++++++++++++++++++ lib/Backend/SftpRangeJob.php | 40 ++ lib/Backend/SftpReadStream.php | 4 +- lib/Backend/StorageBackend.php | 47 ++- lib/Repository/PackReaderGuard.php | 12 +- lib/Repository/ReadAheadPlan.php | 123 +++++++ lib/Repository/Repository.php | 66 +++- tests/Unit/Backend/SftpRangeFetcherTest.php | 189 ++++++++++ tests/Unit/Backend/SftpReadStreamTest.php | 3 + .../Backend/StorageBackendReadAheadTest.php | 103 ++++++ tests/Unit/Repository/ReadAheadTest.php | 272 ++++++++++++++ 12 files changed, 1228 insertions(+), 5 deletions(-) create mode 100644 lib/Backend/IReadsAhead.php create mode 100644 lib/Backend/SftpRangeFetcher.php create mode 100644 lib/Backend/SftpRangeJob.php create mode 100644 lib/Repository/ReadAheadPlan.php create mode 100644 tests/Unit/Backend/SftpRangeFetcherTest.php create mode 100644 tests/Unit/Backend/StorageBackendReadAheadTest.php create mode 100644 tests/Unit/Repository/ReadAheadTest.php diff --git a/lib/Backend/IReadsAhead.php b/lib/Backend/IReadsAhead.php new file mode 100644 index 0000000..873577e --- /dev/null +++ b/lib/Backend/IReadsAhead.php @@ -0,0 +1,30 @@ + $ranges [path, offset, length] each + * @param string $spool an existing local directory for the files that hold the ranges + * @return \Generator|null by position in $ranges; null when this backend + * cannot read ahead now + */ + public function readAhead(array $ranges, string $spool): ?\Generator; +} diff --git a/lib/Backend/SftpRangeFetcher.php b/lib/Backend/SftpRangeFetcher.php new file mode 100644 index 0000000..cb059b8 --- /dev/null +++ b/lib/Backend/SftpRangeFetcher.php @@ -0,0 +1,344 @@ + READ size per connection, lowered when a server answers less than asked */ + private array $chunks = []; + /** @var array the range each busy connection reads */ + private array $jobs = []; + /** @var array ranges read (or failed) and not yet given to the caller */ + private array $done = []; + private int $requestId = 0x4e520000; + + /** + * @param array $connections phpseclib 2 connections with their SFTP channel open + * @param list $ranges paths below $root + */ + private function __construct( + private array $connections, + private string $root, + private array $ranges, + private string $spool, + ) { + foreach (array_keys($connections) as $c) { + $this->chunks[$c] = self::CHUNK; + } + } + + /** + * See IReadsAhead::readAhead(). The connections are ended when the generator ends. + * + * @param list $connections + * @param list $ranges + * @return \Generator + */ + public static function fetch(array $connections, string $root, array $ranges, string $spool): \Generator { + $fetcher = new self($connections, rtrim($root, '/'), $ranges, $spool); + try { + yield from $fetcher->run(); + } finally { + $fetcher->end(); + } + } + + /** @return \Generator */ + private function run(): \Generator { + $count = count($this->ranges); + $next = 0; + for ($want = 0; $want < $count; $want++) { + while (!array_key_exists($want, $this->done)) { + foreach (array_keys($this->connections) as $c) { + if (!isset($this->jobs[$c]) && $next < $count && $next < $want + self::AHEAD) { + $this->start($c, $next++); + } + } + if ($this->jobs === [] && !array_key_exists($want, $this->done)) { + // no connection left: the rest the usual way + $this->done[$want] = null; + $next = max($next, $want + 1); + break; + } + foreach ($this->ready() as $c) { + $this->step($c); + } + } + $fh = $this->done[$want]; + unset($this->done[$want]); + try { + yield $want => $fh; + } finally { + if (is_resource($fh)) { + fclose($fh); + } + } + } + } + + /** + * The busy connections that have an answer waiting, so that a connection that waits for a + * lost packet to be sent again does not hold up the others; all of them when the connections + * do not tell (then they are taken in turn). + * + * @return list + */ + private function ready(): array { + $sockets = []; + $ready = []; + foreach ($this->jobs as $c => $job) { + $sftp = $this->connections[$c]; + $vars = get_object_vars($sftp); // only what is public: phpseclib 2's are + $buffered = $vars['requestBuffer'] ?? null; + if (is_array($buffered) && isset($job->queue[0]) && array_key_exists($job->queue[0][0], $buffered)) { + $ready[] = $c; // read already, while waiting for another answer + continue; + } + $socket = $vars['fsock'] ?? null; + if (!is_resource($socket)) { + return array_keys($this->jobs); + } + $sockets[$c] = $socket; + } + if ($ready !== [] || $sockets === []) { + return $ready; + } + $read = $sockets; + $write = null; + $except = null; + // Data in PHP's own buffer of a socket counts as readable. Nothing within the time: + // each in turn, where phpseclib's own timeout applies. + $n = @stream_select($read, $write, $except, self::SELECT_TIMEOUT); + return $n === false || $n === 0 ? array_keys($this->jobs) : array_keys($read); + } + + private function start(int $c, int $i): void { + [$path, $offset, $length] = $this->ranges[$i]; + $fh = $this->spoolFile(); + if ($fh === null) { + $this->done[$i] = null; + return; + } + if ($length <= 0) { + $this->done[$i] = $fh; + return; + } + $this->jobs[$c] = new SftpRangeJob($i, $fh, $offset, $offset + $length); + try { + $remote = $this->connections[$c]->_realpath($this->root . '/' . ltrim($path, '/')); + if (!is_string($remote)) { + $this->finish($c); + return; + } + $this->send($c, self::FXP_OPEN, pack('Na*N2', strlen($remote), $remote, self::FXF_READ, 0), self::OPEN); + } catch (\Throwable) { + $this->drop($c); + } + } + + /** Take the oldest answer on connection $c, then keep its requests in flight. */ + private function step(int $c): void { + $job = $this->jobs[$c]; + [$id, $kind, $offset, $length] = array_shift($job->queue) ?? throw new \LogicException('no request in flight'); + $sftp = $this->connections[$c]; + try { + $response = $sftp->_get_sftp_packet($id); + if (!is_string($response)) { + throw new BackendException('SFTP connection lost'); + } + $type = $sftp->packet_type; + if ($kind === self::OPEN) { + if ($type === self::FXP_HANDLE) { + $job->handle = substr($response, 4); + } else { + $job->failed = true; // missing, or no permission + } + } elseif ($kind === self::READ) { + if (!$job->failed) { + $this->received($c, $type, $response, $offset, $length); + } + } else { + $status = $type === self::FXP_STATUS && strlen($response) >= 4 ? unpack('N', $response) : false; + $closed = is_array($status) && $status[1] === self::FX_OK; + $this->finish($c); + if (!$closed) { + // handles might pile up on the server: not used again + $this->drop($c); + } + return; + } + $this->refill($c); + } catch (\Throwable) { + $this->drop($c); + } + } + + private function received(int $c, int $type, string $response, int $offset, int $length): void { + $job = $this->jobs[$c]; + $declared = strlen($response) >= 4 ? unpack('N', $response) : false; + $n = is_array($declared) ? $declared[1] : 0; + if ($type !== self::FXP_DATA || $n <= 0 || $n !== strlen($response) - 4 || $n > $length) { + // end of file before the end of the range, a server error or a malformed answer + $job->failed = true; + return; + } + if (fseek($job->fh, $offset - $job->start) !== 0 || fwrite($job->fh, substr($response, 4)) !== $n) { + $job->failed = true; + return; + } + $job->got += $n; + if ($n < $length) { + // a server that caps a READ: ask for the rest, and no more than it gives from now on + $job->redo[] = [$offset + $n, $length - $n]; + $this->chunks[$c] = max(self::MIN_CHUNK, min($this->chunks[$c], $n)); + } + } + + /** Send READs until DEPTH are in flight; when nothing is left to ask for, close the file. */ + private function refill(int $c): void { + $job = $this->jobs[$c]; + if ($job->handle !== '' && !$job->failed) { + while (count($job->queue) < self::DEPTH) { + if ($job->redo !== []) { + [$offset, $length] = array_shift($job->redo); + } elseif ($job->next < $job->end) { + $offset = $job->next; + $length = min($this->chunks[$c], $job->end - $offset); + $job->next += $length; + } else { + break; + } + $packet = pack('Na*N3', strlen($job->handle), $job->handle, intdiv($offset, 4294967296), $offset % 4294967296, $length); + $this->send($c, self::FXP_READ, $packet, self::READ, $offset, $length); + } + } + if ($job->queue !== []) { + return; + } + if ($job->handle === '') { + $this->finish($c); // not opened: nothing to close + return; + } + $this->send($c, self::FXP_CLOSE, pack('Na*', strlen($job->handle), $job->handle), self::CLOSE); + } + + private function send(int $c, int $type, string $packet, int $kind, int $offset = 0, int $length = 0): void { + $this->requestId = $this->requestId >= 0x7fffffff ? 0x4e520000 : $this->requestId + 1; + if (!$this->connections[$c]->_send_sftp_packet($type, $packet, $this->requestId)) { + throw new BackendException('SFTP connection failed while sending a request'); + } + $this->jobs[$c]->queue[] = [$this->requestId, $kind, $offset, $length]; + } + + /** The range of connection $c is done: its stream for the caller, or null when it failed. */ + private function finish(int $c): void { + $job = $this->jobs[$c]; + unset($this->jobs[$c]); + if ($job->failed || $job->got !== $job->end - $job->start || !rewind($job->fh)) { + self::closeFile($job->fh); + $this->done[$job->i] = null; + return; + } + $this->done[$job->i] = $job->fh; + } + + /** A connection in an unknown state: its range fails, and it is not used again. */ + private function drop(int $c): void { + if (isset($this->jobs[$c])) { + $job = $this->jobs[$c]; + unset($this->jobs[$c]); + self::closeFile($job->fh); + $this->done[$job->i] = null; + } + self::disconnect($this->connections[$c]); + unset($this->connections[$c]); + } + + /** @return resource|null a new file in the spool, already unlinked: nothing is left behind if the process dies */ + private function spoolFile() { + $path = $this->spool . '/' . bin2hex(random_bytes(8)); + $fh = @fopen($path, 'x+b'); + if ($fh === false) { + return null; + } + @unlink($path); + return $fh; + } + + private function end(): void { + foreach ($this->jobs as $job) { + self::closeFile($job->fh); + } + foreach ($this->done as $fh) { + if (is_resource($fh)) { + fclose($fh); + } + } + $this->jobs = []; + $this->done = []; + foreach ($this->connections as $sftp) { + self::disconnect($sftp); + } + $this->connections = []; + } + + private static function closeFile(mixed $fh): void { + if (is_resource($fh)) { + fclose($fh); + } + } + + private static function disconnect(object $sftp): void { + try { + if (is_callable([$sftp, 'disconnect'])) { + $sftp->disconnect(); + } + } catch (\Throwable) { + } + } +} diff --git a/lib/Backend/SftpRangeJob.php b/lib/Backend/SftpRangeJob.php new file mode 100644 index 0000000..1e36d4d --- /dev/null +++ b/lib/Backend/SftpRangeJob.php @@ -0,0 +1,40 @@ + offset and length of parts to ask for again (after a short answer) */ + public array $redo = []; + /** @var list request id, kind, offset and length, in the order sent */ + public array $queue = []; + /** Bytes received. */ + public int $got = 0; + public bool $failed = false; + + /** @var resource the spool file that holds the range */ + public $fh; + + /** @param resource $fh */ + public function __construct( + public int $i, + $fh, + public int $start, + public int $end, + ) { + $this->fh = $fh; + $this->next = $start; + } +} diff --git a/lib/Backend/SftpReadStream.php b/lib/Backend/SftpReadStream.php index cd91855..1be1ee3 100644 --- a/lib/Backend/SftpReadStream.php +++ b/lib/Backend/SftpReadStream.php @@ -144,9 +144,9 @@ final class SftpReadStream { /** * The storage's phpseclib connection, with its SFTP channel open, when it offers what these - * streams need; null otherwise. + * streams (and SftpRangeFetcher) need; null otherwise. */ - private static function connection(object $storage): ?object { + public static function connection(object $storage): ?object { if (!is_callable([$storage, 'getConnection']) || !is_callable([$storage, 'getRoot'])) { return null; } diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index c9c0ab3..8d59e6c 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -14,7 +14,7 @@ use OCP\Files\Storage\IWriteStreamStorage; * IBackend on top of a Nextcloud storage object (from files_external or core), below a base * directory on that storage. */ -final class StorageBackend implements IBackend { +final class StorageBackend implements IBackend, IReadsAhead { /** Object stores write whole objects atomically; other storages get a temp name + rename. */ private bool $atomicPut; /** SFTP: read through SftpReadStream, which keeps several requests in flight. */ @@ -243,6 +243,51 @@ final class StorageBackend implements IBackend { } } + /** + * Over SFTP: SftpRangeFetcher, on CONNECTIONS new connections of its own (built with the + * reconnect function), so the storage's connection stays free for other requests. Elsewhere + * not (yet): null. + */ + public function canReadAhead(): bool { + return $this->sftpReads && $this->reconnect !== null; + } + + public function readAhead(array $ranges, string $spool): ?\Generator { + $reconnect = $this->reconnect; + if (!$this->sftpReads || $reconnect === null || $ranges === []) { + return null; + } + $storages = []; + $connections = []; + $root = ''; + for ($n = 0; $n < SftpRangeFetcher::CONNECTIONS; $n++) { + try { + $storage = $reconnect(); + $sftp = SftpReadStream::connection($storage); + if ($sftp === null || !is_callable([$storage, 'getRoot'])) { + break; + } + $root = (string)$storage->getRoot(); + } catch (\Throwable) { + break; // the server may allow fewer connections: go on with those that work + } + $storages[] = $storage; + $connections[] = $sftp; + } + if ($connections === []) { + return null; + } + $abs = array_map(fn (array $r): array => [$this->abs($r[0]), $r[1], $r[2]], $ranges); + return (static function () use ($storages, $connections, $root, $abs, $spool): \Generator { + // the storages hold their connections: kept until the fetcher is done + try { + yield from SftpRangeFetcher::fetch($connections, $root, $abs, $spool); + } finally { + unset($storages); + } + })(); + } + public function getRange(string $path, int $offset, int $length): string { return $this->call(fn () => $this->readRange($path, $offset, $length)); } diff --git a/lib/Repository/PackReaderGuard.php b/lib/Repository/PackReaderGuard.php index 99577e6..24ca5c7 100644 --- a/lib/Repository/PackReaderGuard.php +++ b/lib/Repository/PackReaderGuard.php @@ -8,9 +8,10 @@ declare(strict_types=1); namespace OCA\NgBackup\Repository; use OCA\NgBackup\Backend\IBackend; +use OCA\NgBackup\Backend\IReadsAhead; /** The location as Repository sees it: closes the PackReader's open stream before every other request. */ -final class PackReaderGuard implements IBackend { +final class PackReaderGuard implements IBackend, IReadsAhead { public function __construct( private IBackend $inner, private PackReader $reader, @@ -43,6 +44,15 @@ final class PackReaderGuard implements IBackend { return $this->inner->getRanges($ranges); } + public function canReadAhead(): bool { + return $this->inner instanceof IReadsAhead && $this->inner->canReadAhead(); + } + + /** Over connections of the location's own: the open stream may stay open. */ + public function readAhead(array $ranges, string $spool): ?\Generator { + return $this->inner instanceof IReadsAhead ? $this->inner->readAhead($ranges, $spool) : null; + } + public function exists(string $path): bool { $this->reader->close(); return $this->inner->exists($path); diff --git a/lib/Repository/ReadAheadPlan.php b/lib/Repository/ReadAheadPlan.php new file mode 100644 index 0000000..dc74395 --- /dev/null +++ b/lib/Repository/ReadAheadPlan.php @@ -0,0 +1,123 @@ + [path, offset, length] */ + private array $ranges = []; + /** @var \Generator|null */ + private ?\Generator $streams = null; + private int $current = -1; + /** @var resource|null */ + private $stream = null; + + /** @param string $spool local folder for the ranges read ahead (see IReadsAhead::readAhead()) */ + public function __construct( + public readonly string $spool, + ) { + } + + public function add(string $path, int $offset, int $length): void { + $last = array_pop($this->ranges); + if ($last !== null) { + [$p, $start, $len] = $last; + if ($p === $path && $offset >= $start && $offset <= $start + $len + self::GAP + && $offset + $length - $start <= self::MAX_RANGE) { + $this->ranges[] = [$p, $start, max($len, $offset + $length - $start)]; + return; + } + $this->ranges[] = $last; + } + $this->ranges[] = [$path, $offset, $length]; + } + + /** @return list */ + public function ranges(): array { + return $this->ranges; + } + + /** @param \Generator $streams what IReadsAhead::readAhead() gave for ranges() */ + public function readFrom(\Generator $streams): void { + $this->streams = $streams; + } + + /** The bytes of a blob, or null when its range could not be read ahead (read it the usual way). */ + public function read(string $path, int $offset, int $length): ?string { + if ($this->streams === null) { + return null; + } + if (!$this->contains($this->current, $path, $offset, $length)) { + // Normally the next range. Further on when blobs before it were read elsewhere: the + // index was reloaded after another server's cleanup moved them to new packs. + $to = null; + for ($i = $this->current + 1; $i <= $this->current + self::SEARCH && $to === null; $i++) { + if ($this->contains($i, $path, $offset, $length)) { + $to = $i; + } + } + if ($to === null) { + return null; // not where the plan has it + } + if ($this->current < 0) { + $this->streams->current(); + $this->current = 0; + } + while ($this->current < $to) { + $this->streams->next(); + $this->current++; + } + $stream = $this->streams->valid() ? $this->streams->current() : null; + $this->stream = is_resource($stream) ? $stream : null; + } + $fh = $this->stream; + if ($fh === null || fseek($fh, $offset - $this->ranges[$this->current][1]) !== 0) { + return null; + } + $data = ''; + while (strlen($data) < $length) { + $chunk = fread($fh, $length - strlen($data)); + if ($chunk === false || $chunk === '') { + return null; + } + $data .= $chunk; + } + return $data; + } + + /** Ends the reads ahead, also halfway: closes their streams and connections. */ + public function close(): void { + $this->stream = null; + $streams = $this->streams; + $this->streams = null; + unset($streams); // the generator's finally blocks run when it is destroyed + } + + private function contains(int $i, string $path, int $offset, int $length): bool { + if ($i < 0 || !isset($this->ranges[$i])) { + return false; + } + [$p, $start, $len] = $this->ranges[$i]; + return $p === $path && $offset >= $start && $offset + $length <= $start + $len; + } +} diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 83b4abc..ad71021 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -9,6 +9,7 @@ namespace OCA\NgBackup\Repository; use OCA\NgBackup\Backend\BackendException; use OCA\NgBackup\Backend\IBackend; +use OCA\NgBackup\Backend\IReadsAhead; use OCA\NgBackup\Crypto\KeyRing; use OCA\NgBackup\Crypto\StreamCipher; @@ -216,6 +217,53 @@ final class Repository { if ($prefix !== '' && !self::isSafePath(rtrim($prefix, '/'))) { throw new RepositoryException('Invalid path prefix'); } + $plan = $this->readAhead($snapshotId, $prefix, $target); + try { + return $this->restoreEntries($snapshotId, $target, $prefix, $stripPrefix, $heartbeat, $plan); + } finally { + $plan?->close(); + if ($plan !== null) { + @rmdir($plan->spool); + } + } + } + + /** + * Start reading the packs of a restore ahead, when the location can (IReadsAhead): a first + * pass over the snapshot lists the ranges of packs it needs, in order. The spool is a + * folder in the target, which has room for the restore anyway. + */ + private function readAhead(string $snapshotId, string $prefix, string $target): ?ReadAheadPlan { + if (!$this->backend instanceof IReadsAhead || !$this->backend->canReadAhead()) { + return null; + } + $plan = new ReadAheadPlan(rtrim($target, '/') . '/.ngb-restore-spool-' . bin2hex(random_bytes(4))); + foreach ($this->streamEntries($snapshotId) as $entry) { + if ($prefix !== '' && !str_starts_with($entry['p'], $prefix)) { + continue; + } + foreach ($entry['b'] as $id) { + try { + [$pack, $offset, $length] = $this->index->get($id); + } catch (RepositoryException) { + continue; // the restore reports it when it gets there + } + $plan->add('packs/' . substr($pack, 0, 2) . '/' . $pack, $offset, $length); + } + } + if (count($plan->ranges()) < 2 || (!is_dir($plan->spool) && !@mkdir($plan->spool, 0700, true))) { + return null; + } + $streams = $this->backend->readAhead($plan->ranges(), $plan->spool); + if ($streams === null) { + @rmdir($plan->spool); + return null; + } + $plan->readFrom($streams); + return $plan; + } + + private function restoreEntries(string $snapshotId, string $target, string $prefix, bool $stripPrefix, ?callable $heartbeat, ?ReadAheadPlan $plan): int { $count = 0; foreach ($this->streamEntries($snapshotId) as $entry) { if ($prefix !== '' && !str_starts_with($entry['p'], $prefix)) { @@ -240,7 +288,7 @@ final class Repository { throw new RepositoryException('Cannot create a temporary file for ' . $entry['p']); } foreach ($entry['b'] as $id) { - fwrite($out, $this->loadBlob($id)); + fwrite($out, $this->restoreBlob($id, $plan)); if ($heartbeat !== null) { $heartbeat(); } @@ -789,6 +837,22 @@ final class Repository { } } + /** A blob for restore(): read ahead when it could be, otherwise (or when that copy is bad) the usual way. */ + private function restoreBlob(string $id, ?ReadAheadPlan $plan): string { + if ($plan !== null) { + [$pack, $offset, $length] = $this->index->get($id); + $encrypted = $plan->read('packs/' . substr($pack, 0, 2) . '/' . $pack, $offset, $length); + if ($encrypted !== null) { + try { + return $this->decodeBlob($id, $encrypted); + } catch (\Exception) { + // read again from the location below, which reports what is wrong + } + } + } + return $this->loadBlob($id); + } + private function loadBlob(string $id): string { [$pack, $offset, $length] = $this->index->get($id); try { diff --git a/tests/Unit/Backend/SftpRangeFetcherTest.php b/tests/Unit/Backend/SftpRangeFetcherTest.php new file mode 100644 index 0000000..57036f4 --- /dev/null +++ b/tests/Unit/Backend/SftpRangeFetcherTest.php @@ -0,0 +1,189 @@ + */ + private array $files = []; + + protected function setUp(): void { + SftpReadStream::reset(); + for ($i = 0; $i < 10; $i++) { + $this->files["/home/repo/packs/p$i"] = random_bytes(3 * 1048576 + $i * 1000); + } + } + + /** @return list */ + private function connections(int $n, array $options = []): array { + $result = []; + for ($c = 0; $c < $n; $c++) { + $result[] = new FakeSftpConnection($this->files, ...($options[$c] ?? [])); + } + return $result; + } + + /** @return list what StorageBackend passes on: each with its SFTP channel open */ + private static function opened(array $connections): array { + return array_map(fn (FakeSftpConnection $c): object => SftpReadStream::connection(new FakeSftpStorage($c)), $connections); + } + + /** @return list */ + private function ranges(): array { + $ranges = []; + for ($i = 0; $i < 10; $i++) { + $ranges[] = ["repo/packs/p$i", 1000 * $i, 2 * 1048576 + 333]; + } + return $ranges; + } + + /** @return array sha1 of the contents by position (a failing comparison of MiBs would not fit in memory) */ + private static function collect(\Generator $streams): array { + $result = []; + foreach ($streams as $i => $fh) { + $result[$i] = $fh === null ? null : sha1(stream_get_contents($fh)); + } + return $result; + } + + private function expected(array $ranges): array { + return array_map(fn (array $r): string => sha1(substr($this->files['/home/' . $r[0]], $r[1], $r[2])), $ranges); + } + + public function testReadsEveryRangeInOrderOverAllConnections(): void { + $connections = $this->connections(4); + $ranges = $this->ranges(); + $spool = $this->tempDir(); + + $this->assertSame($this->expected($ranges), self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', $ranges, $spool))); + + foreach ($connections as $c) { + $this->assertGreaterThan(0, count($c->reads), 'every connection reads'); + $this->assertLessThanOrEqual(SftpRangeFetcher::DEPTH, $c->maxInFlight); + $this->assertSame([], $c->openHandles); + $this->assertSame(0, $c->pending()); + $this->assertTrue($c->disconnected, 'its own connections end with it'); + } + $this->assertSame([], glob("$spool/*"), 'nothing left in the spool'); + } + + public function testKeepsSeveralRequestsInFlightOnEachConnection(): void { + $this->files['/home/repo/packs/big'] = random_bytes(6 * 1048576); + $connections = $this->connections(2); + self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', [['repo/packs/big', 0, 6 * 1048576], ['repo/packs/big', 5, 6 * 1048576 - 5]], $this->tempDir())); + + $this->assertSame(SftpRangeFetcher::DEPTH, $connections[0]->maxInFlight); + $this->assertSame(SftpRangeFetcher::DEPTH, $connections[1]->maxInFlight); + } + + public function testAServerThatCapsAReadStillGivesEveryByte(): void { + $connections = $this->connections(3, array_fill(0, 3, ['cap' => 100000])); + $ranges = $this->ranges(); + + $this->assertSame($this->expected($ranges), self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', $ranges, $this->tempDir()))); + // learned: after the first short answers (and asking for their rest) it asks for no more than the server gives + foreach ($connections as $c) { + $this->assertLessThanOrEqual(2 * SftpRangeFetcher::DEPTH, count(array_filter($c->reads, fn (int $n): bool => $n > 100000))); + } + } + + public function testAMissingFileIsLeftToTheCaller(): void { + $connections = $this->connections(2); + $ranges = $this->ranges(); + $ranges[3][0] = 'repo/packs/gone'; + + $result = self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', $ranges, $this->tempDir())); + + $expected = $this->expected($this->ranges()); + $expected[3] = null; + $this->assertSame($expected, $result); + $this->assertSame([], $connections[0]->openHandles); + } + + public function testARangePastTheEndOfTheFileIsLeftToTheCaller(): void { + $ranges = $this->ranges(); + $ranges[5][2] = 10 * 1048576; + + $result = self::collect(SftpRangeFetcher::fetch(self::opened($this->connections(2)), '/home/', $ranges, $this->tempDir())); + + $this->assertNull($result[5]); + $this->assertSame($this->expected($this->ranges())[6], $result[6]); + } + + public function testAServerErrorFailsOnlyThatRange(): void { + // every read from 1 MiB on fails on connection 0: its ranges, not those of the others + $connections = $this->connections(2, [['failAt' => 1048576]]); + $ranges = $this->ranges(); + + $result = self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', $ranges, $this->tempDir())); + + $expected = $this->expected($ranges); + $failed = array_keys(array_filter($result, fn (?string $r): bool => $r === null)); + $this->assertNotSame([], $failed); + foreach ($result as $i => $data) { + if ($data !== null) { + $this->assertSame($expected[$i], $data); + } + } + $this->assertLessThan(10, count($failed), 'the other connection goes on'); + $this->assertSame([], $connections[0]->openHandles, 'a failed range is closed too'); + } + + public function testALostConnectionIsNotUsedAgainAndTheOthersGoOn(): void { + $connections = $this->connections(3, [['loseAt' => 1048576]]); + $ranges = $this->ranges(); + + $result = self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', $ranges, $this->tempDir())); + + $expected = $this->expected($ranges); + $this->assertSame(1, count(array_filter($result, fn (?string $r): bool => $r === null)), 'only the range it was reading'); + foreach ($result as $i => $data) { + if ($data !== null) { + $this->assertSame($expected[$i], $data); + } + } + $this->assertTrue($connections[0]->disconnected); + $this->assertLessThanOrEqual(1048576 / 261120 + SftpRangeFetcher::DEPTH + 1, count($connections[0]->reads), 'not used after it broke'); + } + + public function testWhenEveryConnectionIsLostTheRestIsLeftToTheCaller(): void { + $connections = $this->connections(2, [['throwAt' => 1048576], ['loseAt' => 1048576]]); + + $result = self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', $this->ranges(), $this->tempDir())); + + $this->assertSame(array_fill(0, 10, null), $result); + } + + public function testStoppingHalfwayEndsTheConnectionsAndLeavesNothingBehind(): void { + $connections = $this->connections(2); + $spool = $this->tempDir(); + $streams = SftpRangeFetcher::fetch(self::opened($connections), '/home/', $this->ranges(), $spool); + $this->assertIsResource($streams->current()); + + unset($streams); + + $this->assertTrue($connections[0]->disconnected); + $this->assertTrue($connections[1]->disconnected); + $this->assertSame([], glob("$spool/*")); + } + + public function testReadsNoMoreThanAheadOfTheCaller(): void { + $connections = $this->connections(4); + $streams = SftpRangeFetcher::fetch(self::opened($connections), '/home/', $this->ranges(), $this->tempDir()); + $streams->current(); + + $opened = array_sum(array_map(fn (FakeSftpConnection $c): int => count(array_unique(array_values($c->opened))), $connections)); + $this->assertLessThanOrEqual(SftpRangeFetcher::AHEAD, $opened); + } +} diff --git a/tests/Unit/Backend/SftpReadStreamTest.php b/tests/Unit/Backend/SftpReadStreamTest.php index e152baa..0718d0e 100644 --- a/tests/Unit/Backend/SftpReadStreamTest.php +++ b/tests/Unit/Backend/SftpReadStreamTest.php @@ -466,6 +466,8 @@ final class FakeSftpConnection { public array $reads = []; /** @var array */ public array $openHandles = []; + /** @var list paths of every OPEN request */ + public array $opened = []; /** Whether the SFTP channel is open: phpseclib 2 opens it on the first SFTP call. */ public bool $initialised = false; /** Packets sent */ @@ -532,6 +534,7 @@ final class FakeSftpConnection { return false; } $path = substr($data, 4, unpack('N', $data)[1]); + $this->opened[] = $path; if (!isset($this->files[$path])) { $this->answers[$id] = [101, pack('N', 2)]; break; diff --git a/tests/Unit/Backend/StorageBackendReadAheadTest.php b/tests/Unit/Backend/StorageBackendReadAheadTest.php new file mode 100644 index 0000000..5bf7b7c --- /dev/null +++ b/tests/Unit/Backend/StorageBackendReadAheadTest.php @@ -0,0 +1,103 @@ + */ + private array $connections = []; + /** @var array */ + private array $files = []; + /** New storages after this many fail to connect. */ + private ?int $connectLimit = null; + + protected function setUp(): void { + SftpReadStream::reset(); + for ($i = 0; $i < 6; $i++) { + $this->files["/home/base/repo/packs/p$i"] = random_bytes(1048576 + $i); + } + } + + private function storage(): IStorage { + if ($this->connectLimit !== null && count($this->connections) >= $this->connectLimit) { + throw new \RuntimeException('Too many connections'); + } + $sftp = new FakeSftpConnection($this->files); + $this->connections[] = $sftp; + $storage = $this->createMock(SftpLikeRootStorage::class); + $storage->method('instanceOfStorage')->willReturnCallback(fn (string $class): bool => $class === \OCA\Files_External\Lib\Storage\SFTP::class); + $storage->method('getConnection')->willReturn($sftp); + $storage->method('getRoot')->willReturn('/home/'); + $storage->method('is_dir')->willReturn(true); + return $storage; + } + + /** @return list */ + private static function ranges(): array { + return array_map(fn (int $i): array => ["packs/p$i", 7, 1048576 - 7], range(0, 5)); + } + + public function testReadsAheadOnNewConnectionsOnly(): void { + $backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage()); + $this->assertTrue($backend->canReadAhead()); + + $result = []; + foreach ($backend->readAhead(self::ranges(), $this->tempDir()) as $i => $fh) { + $result[$i] = sha1(stream_get_contents($fh)); + } + + $expected = array_map(fn (int $i): string => sha1(substr($this->files["/home/base/repo/packs/p$i"], 7, 1048576 - 7)), range(0, 5)); + $this->assertSame($expected, $result); + $this->assertCount(1 + SftpRangeFetcher::CONNECTIONS, $this->connections); + $this->assertSame(0, $this->connections[0]->sent, 'the storage\'s own connection stays free'); + foreach (array_slice($this->connections, 1) as $c) { + $this->assertTrue($c->disconnected); + } + } + + public function testGoesOnWithTheConnectionsTheServerAllows(): void { + $this->connectLimit = 3; + $backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage()); + + $count = 0; + foreach ($backend->readAhead(self::ranges(), $this->tempDir()) as $fh) { + $this->assertIsResource($fh); + $count++; + } + + $this->assertSame(6, $count); + $this->assertCount(3, $this->connections); + } + + public function testNoNewConnectionMeansNoReadingAhead(): void { + $this->connectLimit = 1; + $backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage()); + + $this->assertNull($backend->readAhead(self::ranges(), $this->tempDir())); + } + + public function testWithoutAWayToConnectAgainItDoesNotReadAhead(): void { + $backend = new StorageBackend($this->storage(), 'base/repo', false, null); + + $this->assertFalse($backend->canReadAhead()); + $this->assertNull($backend->readAhead(self::ranges(), $this->tempDir())); + } +} + +interface SftpLikeRootStorage extends SftpLikeStorage { + public function getRoot(): string; +} diff --git a/tests/Unit/Repository/ReadAheadTest.php b/tests/Unit/Repository/ReadAheadTest.php new file mode 100644 index 0000000..276639f --- /dev/null +++ b/tests/Unit/Repository/ReadAheadTest.php @@ -0,0 +1,272 @@ +root = $this->tempDir(); + $this->keys = KeyRing::generate(); + $this->src = $this->tempDir(); + // three packs of 32 MiB, and small files in between + for ($i = 0; $i < 4; $i++) { + file_put_contents(sprintf('%s/big%d', $this->src, $i), random_bytes(20 * 1048576)); + for ($j = 0; $j < 20; $j++) { + file_put_contents(sprintf('%s/small%d-%02d', $this->src, $i, $j), random_bytes(30000)); + } + } + file_put_contents("$this->src/same", file_get_contents("$this->src/small0-00")); // the same blob again, later + $repo = Repository::initWithKey(new LocalBackend($this->root), $this->keys, $this->keys->wrap('pw')); + $this->snapshot = $repo->backupDirectory($this->src)['snapshot']; + } + + private function restore(ReadAheadBackend $backend, string $prefix = ''): string { + $target = $this->tempDir(); + $repo = Repository::openWithKey($backend, $this->keys, null, null); + $repo->restore($this->snapshot, $target, $prefix); + return $target; + } + + private function assertRestored(string $target, string $prefix = ''): void { + $expected = array_filter(self::hashTree($this->src), fn (string $p): bool => str_starts_with($p, $prefix), ARRAY_FILTER_USE_KEY); + $this->assertSame($expected, self::hashTree($target)); + $this->assertSame([], glob("$target/.ngb-restore-spool-*"), 'the spool is removed'); + } + + public function testARestoreReadsEveryPackAhead(): void { + $backend = new ReadAheadBackend(new LocalBackend($this->root)); + + $this->assertRestored($this->restore($backend)); + + $this->assertSame(0, $backend->counter->packOpens(), 'nothing read the usual way'); + $this->assertGreaterThanOrEqual(3, count($backend->asked)); + // in the order of the files, one range per pack the restore goes through (the same blob + // again later is a range of its own) + $this->assertLessThanOrEqual(count($backend->inner->list('packs')) + 1, count($backend->asked)); + } + + public function testARangeThatCouldNotBeReadAheadIsReadTheUsualWay(): void { + $backend = new ReadAheadBackend(new LocalBackend($this->root), fail: [1]); + + $this->assertRestored($this->restore($backend)); + + $this->assertGreaterThan(0, $backend->counter->packOpens()); + } + + public function testADamagedCopyIsReadAgainTheUsualWay(): void { + $backend = new ReadAheadBackend(new LocalBackend($this->root), damage: [0, 2]); + + $this->assertRestored($this->restore($backend)); + + $this->assertGreaterThan(0, $backend->counter->packOpens()); + } + + public function testAPartialRestoreReadsOnlyWhatItNeeds(): void { + $backend = new ReadAheadBackend(new LocalBackend($this->root)); + + $this->assertRestored($this->restore($backend, 'big1'), 'big1'); + + $this->assertSame(0, $backend->counter->packOpens()); + $this->assertCount(2, $backend->asked, 'the end of one pack and the start of the next'); + $total = array_sum(array_map(fn (array $r): int => $r[2], $backend->asked)); + $this->assertLessThan(21 * 1048576, $total, 'not the whole packs'); + } + + public function testABackendThatCannotReadAheadNowRestoresAsBefore(): void { + $backend = new ReadAheadBackend(new LocalBackend($this->root), unavailable: true); + + $this->assertRestored($this->restore($backend)); + + $this->assertGreaterThan(0, $backend->counter->packOpens()); + } + + public function testAFailingRestoreStillEndsTheReadsAheadAndRemovesTheSpool(): void { + // the second range fails, and so does reading it the usual way + $backend = new ReadAheadBackend(new LocalBackend($this->root), fail: [1], packsGone: true); + $target = $this->tempDir(); + $repo = Repository::openWithKey($backend, $this->keys, null, null); + + try { + $repo->restore($this->snapshot, $target); + $this->fail('restore should fail'); + } catch (BackendException $e) { + $this->assertStringStartsWith('Not found', $e->getMessage()); + } + + $this->assertTrue($backend->ended, 'the generator was ended'); + $this->assertSame([], glob("$target/.ngb-restore-spool-*")); + } + + public function testPlanJoinsBlobsThatFollowEachOtherInAPack(): void { + $plan = new ReadAheadPlan('/nowhere'); + $plan->add('packs/aa/a', 0, 100); + $plan->add('packs/aa/a', 100, 100); + $plan->add('packs/aa/a', 50, 10); // the same blob again: inside the range + $plan->add('packs/aa/a', 200 + ReadAheadPlan::GAP, 100); // a gap it reads along + $plan->add('packs/aa/a', 300 + 3 * ReadAheadPlan::GAP, 100); // too far: a new range + $plan->add('packs/bb/b', 0, 100); + $plan->add('packs/bb/b', 0, 100); // backwards is fine as long as it is inside + $plan->add('packs/aa/a', 0, 100); // back to an earlier pack: a new range + + $this->assertSame([ + ['packs/aa/a', 0, 300 + ReadAheadPlan::GAP], + ['packs/aa/a', 300 + 3 * ReadAheadPlan::GAP, 100], + ['packs/bb/b', 0, 100], + ['packs/aa/a', 0, 100], + ], $plan->ranges()); + } + + public function testPlanSkipsRangesWhoseBlobsWereReadElsewhere(): void { + $plan = new ReadAheadPlan('/nowhere'); + foreach (range(0, 4) as $p) { + $plan->add("packs/0$p/p$p", 0, 10); + } + $plan->readFrom((function (): \Generator { + foreach (range(0, 4) as $p) { + $fh = fopen('php://temp', 'w+b'); + fwrite($fh, str_repeat((string)$p, 10)); + rewind($fh); + yield $p => $fh; + } + })()); + + $this->assertSame('00000', $plan->read('packs/00/p0', 0, 5)); + $this->assertNull($plan->read('packs/77/moved', 0, 5), 'moved: not in the plan'); + $this->assertSame('333', $plan->read('packs/03/p3', 2, 3), 'the ranges in between were read elsewhere'); + $this->assertNull($plan->read('packs/01/p1', 0, 5), 'never backwards'); + $this->assertSame('44', $plan->read('packs/04/p4', 0, 2)); + } + + public function testPlanKeepsARangeShorterThanTheMaximum(): void { + $plan = new ReadAheadPlan('/nowhere'); + for ($o = 0; $o < 3 * ReadAheadPlan::MAX_RANGE; $o += 4 * 1048576) { + $plan->add('packs/aa/a', $o, 4 * 1048576); + } + $this->assertCount(3, $plan->ranges()); + foreach ($plan->ranges() as $r) { + $this->assertLessThanOrEqual(ReadAheadPlan::MAX_RANGE, $r[2]); + } + } +} + +/** Reads ahead from a local repository, one range after the other; can fail or damage ranges. */ +final class ReadAheadBackend implements IBackend, IReadsAhead { + public PackOpenCountingBackend $counter; + /** @var list */ + public array $asked = []; + public bool $ended = false; + + /** + * @param list $fail ranges given as null + * @param list $damage ranges given with a flipped byte in every blob + */ + public function __construct( + public LocalBackend $inner, + private array $fail = [], + private array $damage = [], + private bool $unavailable = false, + /** reading a pack the usual way fails */ + private bool $packsGone = false, + ) { + $this->counter = new PackOpenCountingBackend($inner); + } + + public function canReadAhead(): bool { + return true; + } + + public function readAhead(array $ranges, string $spool): ?\Generator { + if ($this->unavailable) { + return null; + } + \PHPUnit\Framework\Assert::assertDirectoryExists($spool); + $this->asked = $ranges; + return (function () use ($ranges): \Generator { + try { + foreach ($ranges as $i => [$path, $offset, $length]) { + if (in_array($i, $this->fail, true)) { + yield $i => null; + continue; + } + $data = $this->inner->getRange($path, $offset, $length); + if (in_array($i, $this->damage, true)) { + for ($k = 0; $k < strlen($data); $k += 4096) { + $data[$k] = chr(ord($data[$k]) ^ 1); + } + } + $fh = fopen('php://temp', 'w+b'); + fwrite($fh, $data); + rewind($fh); + yield $i => $fh; + fclose($fh); + } + } finally { + $this->ended = true; + } + })(); + } + + public function put(string $path, $stream): void { + $this->counter->put($path, $stream); + } + + public function get(string $path) { + if ($this->packsGone && str_starts_with($path, 'packs/')) { + throw new BackendException('Not found: ' . $path); + } + return $this->counter->get($path); + } + + public function getMany(array $paths): \Generator { + return $this->counter->getMany($paths); + } + + public function getRange(string $path, int $offset, int $length): string { + return $this->counter->getRange($path, $offset, $length); + } + + public function getRanges(array $ranges): \Generator { + return $this->counter->getRanges($ranges); + } + + public function exists(string $path): bool { + return $this->counter->exists($path); + } + + public function move(string $from, string $to): void { + $this->counter->move($from, $to); + } + + public function list(string $prefix): array { + return $this->counter->list($prefix); + } + + public function listFiles(string $dir): array { + return $this->counter->listFiles($dir); + } + + public function delete(string $path): void { + $this->counter->delete($path); + } +} From 1d7f7e6423763240460632f3b388a092f8ac8c43 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 01:52:58 +0200 Subject: [PATCH 05/12] Read ahead: resolve paths before opening connections; a failing readAhead() leaves no spool folder --- lib/Backend/StorageBackend.php | 3 ++- lib/Repository/Repository.php | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index 8d59e6c..24ee719 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -257,6 +257,8 @@ final class StorageBackend implements IBackend, IReadsAhead { if (!$this->sftpReads || $reconnect === null || $ranges === []) { return null; } + // before any connection is opened: an invalid path throws + $abs = array_map(fn (array $r): array => [$this->abs($r[0]), $r[1], $r[2]], $ranges); $storages = []; $connections = []; $root = ''; @@ -277,7 +279,6 @@ final class StorageBackend implements IBackend, IReadsAhead { if ($connections === []) { return null; } - $abs = array_map(fn (array $r): array => [$this->abs($r[0]), $r[1], $r[2]], $ranges); return (static function () use ($storages, $connections, $root, $abs, $spool): \Generator { // the storages hold their connections: kept until the fetcher is done try { diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index ad71021..284a300 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -254,7 +254,11 @@ final class Repository { if (count($plan->ranges()) < 2 || (!is_dir($plan->spool) && !@mkdir($plan->spool, 0700, true))) { return null; } - $streams = $this->backend->readAhead($plan->ranges(), $plan->spool); + try { + $streams = $this->backend->readAhead($plan->ranges(), $plan->spool); + } catch (\Throwable) { + $streams = null; // the usual reads report what is wrong + } if ($streams === null) { @rmdir($plan->spool); return null; From 88b719879d98b9f465f70f582383bc244321eb3e Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 03:51:21 +0200 Subject: [PATCH 06/12] Release candidate 4 (app version 0.9.10): full restore over SFTP reads ahead; SFTP leg in the full-restore test --- .github/workflows/ci.yml | 2 ++ CHANGELOG.md | 7 ++++ PLAN.md | 10 ++++-- README.md | 3 ++ appinfo/info.xml | 8 ++--- tests/integration/full-restore-roundtrip.sh | 37 +++++++++++++++++---- 6 files changed, 55 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dbc09a7..549c2a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -112,3 +112,5 @@ jobs: 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-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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac6881..b8b0102 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,13 @@ instead carried by the app description, the git tag, the release, and this file. ## [Unreleased] +### Changed +- A full restore over SFTP reads the packs ahead on 4 connections of its own, many requests in + flight on each, into temporary files in the restore folder. From a Hetzner Storage Box to a + server in Canada (122 ms) packs now come in at about 7.6 MiB/s instead of 2.8; on a test line + with 120 ms and some packet loss, a 1.6 GiB restore went from 5.1 to 17.8 MiB/s. A part that + cannot be read this way is read as before. Restores from other locations work as before. + ### Fixed - A full restore of a backup made before 1.0.0-rc.2 with `--skip-missing-tables` failed on PostgreSQL with "relation ... does not exist": the left-out tables were still looked up to set diff --git a/PLAN.md b/PLAN.md index 60a0198..065a39b 100644 --- a/PLAN.md +++ b/PLAN.md @@ -386,9 +386,15 @@ in README "Known limitations": - **1.0.0 (besluit 2026-10-18):** alleen betrouwbaarheid. rc2 uitgebracht; slaagt de grote DR-test op VM 980 (snapshot van 12-10, met `--skip-missing-tables`), dan 1.0.0. Zo niet: fix, rc3, en rc3 pas uitbrengen na een geslaagde volledige DR-test. +- **rc4 (besluit 2026-10-19):** rc3 is niet uitgebracht. De DR-test met rc3 liep ~2,8 MiB/s + (~3,5 dag); parallel lezen over SFTP haalde vanaf VM 980 ~7,6 MiB/s. Daarom rc4 = rc3 + + vooruitlezen over SFTP (4 eigen verbindingen). Volgorde: volledige DR-test op VM 980 volgens de + gids (SFTP, Storage Box) → rc4 uitbrengen en om tests vragen → zelf een volledige DR van een + productie-instantie van ~250 GB via S3 en via SMB. Drie volledige DR-restores vóór 1.0.0. + Problemen of meldingen van gebruikers: rc5. - **1.0.1:** - 1. Herstelsnelheid: bij een restore meerdere pakketten tegelijk lezen (SFTP: meerdere - verbindingen; S3 heeft al 16 tegelijk). Raakt het leespad, daarom niet in 1.0.0. + 1. (Naar rc4: herstelsnelheid over SFTP.) S3 leest al 16 tegelijk bij index en tree, maar een + restore leest de packs nog één voor één: ook vooruitlezen via de S3-client. 2. Waarschuwing in de beheerpagina en `backup:status` als achtergrondjobs niet op systeem-cron staan, en "waiting for background jobs" als een lopende backup lang geen stap zette (issue #13). Cron zelf aanzetten kan de app niet: zonder echte cron-regel stopt dan alles. diff --git a/README.md b/README.md index f2a3a45..79e8258 100644 --- a/README.md +++ b/README.md @@ -181,6 +181,9 @@ Contributions welcome via PR — add what you ran into with your own provider. server (or the VM/container host where TCP ends) to BBR: `sysctl -w net.core.default_qdisc=fq net.ipv4.tcp_congestion_control=bbr` (`modprobe tcp_bbr` first; make it permanent in `/etc/sysctl.d/`). + A full restore over SFTP reads the packs ahead on 4 connections of its own, next to the one + NG Backup already has open: the SFTP server must allow 5 connections at once for that user (a + Hetzner Storage Box allows 10). With fewer it uses the connections it gets. - **NFS is not a files_external backend at all.** Mount the NFS export on the server yourself (e.g. via `/etc/fstab` or a systemd `.mount` unit) and point ng_backup's `local` backend at the diff --git a/appinfo/info.xml b/appinfo/info.xml index 81849a4..3b0fe43 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), which also offers paid help with restoring a backup after a crash.]]> - 0.9.9 + 0.9.10 agpl Dillard Blom NgBackup @@ -52,9 +52,9 @@ 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.3/screenshots/1-backups.png - https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.3/screenshots/2-restore.png - https://raw.githubusercontent.com/dillardblom/ng_backup/v1.0.0-rc.3/screenshots/3-settings.png + 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 sodium diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index a26e755..0d5139e 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -2,8 +2,10 @@ # 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] +# Usage: full-restore-roundtrip.sh [sqlite|pgsql|mysql] [legacy|sftp] # 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 "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. @@ -13,7 +15,9 @@ set -euo pipefail IMAGE="nextcloud:${1:-35-apache}" DB="${2:-sqlite}" -LEGACY="${3:-}" +MODE="${3:-}" +LEGACY="" +[ "$MODE" = legacy ] && LEGACY=legacy APP=$(cd "$(dirname "$0")/../.." && pwd) ORIGIN_APP="$APP" NET=ngb-frt-net @@ -23,7 +27,7 @@ fail() { echo "FAIL: $*"; exit 1; } ok() { echo "ok $*"; } cleanup() { - docker rm -fv ngb-frt-origin ngb-frt-restore ngb-frt-db-origin ngb-frt-db-restore ngb-frt-imap >/dev/null 2>&1 || true + docker rm -fv ngb-frt-origin ngb-frt-restore ngb-frt-db-origin ngb-frt-db-restore ngb-frt-imap ngb-frt-sftp >/dev/null 2>&1 || true docker network rm "$NET" >/dev/null 2>&1 || true docker run --rm -v "$SHARED:/target" alpine sh -c 'rm -rf /target/*' >/dev/null 2>&1 || true rmdir "$SHARED" 2>/dev/null || true @@ -59,6 +63,11 @@ db_env() { } docker network create "$NET" >/dev/null +LOCATION=(local -a null::null -o datadir=/mnt/shared) +if [ "$MODE" = sftp ]; then + docker run -d --name ngb-frt-sftp --network "$NET" atmoz/sftp:alpine ngbackup:ngbackuppw:::upload >/dev/null + LOCATION=(sftp -a password::password -o host=ngb-frt-sftp -o root=/upload -o user=ngbackup -o password=ngbackuppw) +fi ORIGIN_DB=$(db_env origin) # shellcheck disable=SC2086 docker run -d --name ngb-frt-origin --network "$NET" $ORIGIN_DB \ @@ -74,12 +83,17 @@ run_on ngb-frt-origin backup:key:init --passphrase-file=/tmp/pass --label=CI >/d run_on ngb-frt-origin backup:key:kit --output=/tmp/kit.json >/dev/null CODE=$(run_on ngb-frt-origin backup:key:kit | php -r 'echo json_decode(stream_get_contents(STDIN), true)["confirmation_code"];') run_on ngb-frt-origin backup:key:confirm -u admin --code="$CODE" --confirm="Yes, I confirm" >/dev/null || fail "confirm" -run_on ngb-frt-origin backup:target:add offsite local -a null::null -o datadir=/mnt/shared --path=repo >/dev/null || fail "target add" +run_on ngb-frt-origin backup:target:add offsite "${LOCATION[@]}" --path=repo >/dev/null || fail "target add" ok "key, kit and location on the origin" NC_PASS=$(openssl rand -hex 16) docker exec -u www-data -e OC_PASS="$NC_PASS" ngb-frt-origin php occ user:add --password-from-env frtest >/dev/null docker exec ngb-frt-origin sh -c "mkdir -p /var/www/html/data/frtest/files/Docs && head -c 200000 /dev/urandom > /var/www/html/data/frtest/files/Docs/report.bin && echo 'hello disaster recovery' > /var/www/html/data/frtest/files/Docs/note.txt && chown -R www-data:www-data /var/www/html/data/frtest" +if [ "$MODE" = sftp ]; then + # about three packs of 32 MiB + docker exec ngb-frt-origin sh -c "head -c 90000000 /dev/urandom > /var/www/html/data/frtest/files/Docs/large.bin && chown www-data:www-data /var/www/html/data/frtest/files/Docs/large.bin" + LARGE_SUM=$(docker exec ngb-frt-origin sha256sum /var/www/html/data/frtest/files/Docs/large.bin | cut -d' ' -f1) +fi run_on ngb-frt-origin files:scan frtest >/dev/null # SQL on an instance's own database (pgsql/mysql only) @@ -127,7 +141,7 @@ 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 local -a null::null -o datadir=/mnt/shared --path=repo >/dev/null || fail "target add on the restore instance" +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" ok "recovery kit imported, existing repository adopted, snapshot visible" @@ -140,6 +154,8 @@ if [ "$LEGACY" = legacy ] && [ "$DB" != sqlite ]; then ok "older backup refused without --skip-missing-tables, missing tables named" RESTORE_OPTS+=(--skip-missing-tables) fi +sftp_logins() { docker logs ngb-frt-sftp 2>&1 | grep -c "Accepted password" || true; } +[ "$MODE" = sftp ] && LOGINS_BEFORE=$(sftp_logins) OUT=$(run_on ngb-frt-restore backup:restore:full offsite "$SNAP" "${RESTORE_OPTS[@]}" 2>&1) || { printf '%s\n' "$OUT" | tail -40 docker exec ngb-frt-restore sh -c 'tail -c 6000 /var/www/html/data/nextcloud.log' || true @@ -169,7 +185,16 @@ elif [ "$DB" != sqlite ]; then ok "tables of a disabled app created, Mail restored with its foreign keys" fi +if [ "$MODE" = sftp ]; then + [ "$(docker exec ngb-frt-restore sha256sum /var/www/html/data/frtest/files/Docs/large.bin | cut -d' ' -f1)" = "$LARGE_SUM" ] \ + || fail "the large file differs after the restore" + LOGINS=$(( $(sftp_logins) - LOGINS_BEFORE )) + [ "$LOGINS" -gt 1 ] || fail "the restore did not read ahead on connections of its own ($LOGINS SFTP logins)" + docker exec ngb-frt-restore sh -c 'ls -a /var/www/html/data' | grep -q ngb-restore-spool && fail "the spool folder was left behind" + ok "over SFTP: read ahead on $LOGINS connections, large file identical, no spool left" +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${LEGACY:+, legacy})" +echo "FULL RESTORE ROUNDTRIP PASS ($DB${MODE:+, $MODE})" From 43a71684f9231bdd89ff29a32eed0bd01d3256de Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 05:28:26 +0200 Subject: [PATCH 07/12] Read ahead over SFTP: take answers phpseclib has read already instead of waiting up to 10 s on the socket --- lib/Backend/SftpRangeFetcher.php | 18 ++++++++++++++---- tests/Unit/Backend/SftpRangeFetcherTest.php | 19 +++++++++++++++++++ tests/Unit/Backend/SftpReadStreamTest.php | 4 ++++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/lib/Backend/SftpRangeFetcher.php b/lib/Backend/SftpRangeFetcher.php index cb059b8..153b13e 100644 --- a/lib/Backend/SftpRangeFetcher.php +++ b/lib/Backend/SftpRangeFetcher.php @@ -30,8 +30,11 @@ final class SftpRangeFetcher { public const DEPTH = 16; /** Ranges read ahead of the one the caller is at, in flight or waiting in the spool. */ public const AHEAD = 8; - /** Seconds ready() waits for any connection to answer before it waits for each in turn. */ - private const SELECT_TIMEOUT = 10; + /** + * Seconds ready() waits for any connection to answer before it waits for each in turn: short, + * in case a connection holds an answer where ready() does not look. + */ + private const SELECT_TIMEOUT = 1; /** See SftpReadStream::CHUNK and MIN_CHUNK. */ private const CHUNK = 261120; private const MIN_CHUNK = 32768; @@ -134,9 +137,16 @@ final class SftpRangeFetcher { foreach ($this->jobs as $c => $job) { $sftp = $this->connections[$c]; $vars = get_object_vars($sftp); // only what is public: phpseclib 2's are + // Answers phpseclib has read already: the socket does not show them. Its request + // buffer holds whole answers read while it waited for another; its packet buffer and + // channel buffers hold what came in with the last packet it read (small answers, such + // as a CLOSE and the next OPEN, often arrive together). $buffered = $vars['requestBuffer'] ?? null; - if (is_array($buffered) && isset($job->queue[0]) && array_key_exists($job->queue[0][0], $buffered)) { - $ready[] = $c; // read already, while waiting for another answer + $channels = $vars['channel_buffers'] ?? null; + if ((is_array($buffered) && isset($job->queue[0]) && array_key_exists($job->queue[0][0], $buffered)) + || (is_string($vars['packet_buffer'] ?? null) && $vars['packet_buffer'] !== '') + || (is_array($channels) && array_filter($channels) !== [])) { + $ready[] = $c; continue; } $socket = $vars['fsock'] ?? null; diff --git a/tests/Unit/Backend/SftpRangeFetcherTest.php b/tests/Unit/Backend/SftpRangeFetcherTest.php index 57036f4..b27e8ff 100644 --- a/tests/Unit/Backend/SftpRangeFetcherTest.php +++ b/tests/Unit/Backend/SftpRangeFetcherTest.php @@ -87,6 +87,25 @@ class SftpRangeFetcherTest extends TestCase { $this->assertSame(SftpRangeFetcher::DEPTH, $connections[1]->maxInFlight); } + public function testAnAnswerPhpseclibHasReadAlreadyIsTakenWithoutWaitingOnTheSocket(): void { + // The fake's answers are all "read already": its socket never becomes readable, and + // phpseclib's packet buffer holds data. Waiting on the socket would take a second a step. + $connections = $this->connections(2); + $sockets = []; + foreach ($connections as $c) { + $pair = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP); + $sockets[] = $pair; + $c->fsock = $pair[0]; + $c->packet_buffer = 'more'; + } + $ranges = array_slice($this->ranges(), 0, 3); + $start = microtime(true); + + $this->assertSame($this->expected($ranges), self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', $ranges, $this->tempDir()))); + + $this->assertLessThan(1.0, microtime(true) - $start); + } + public function testAServerThatCapsAReadStillGivesEveryByte(): void { $connections = $this->connections(3, array_fill(0, 3, ['cap' => 100000])); $ranges = $this->ranges(); diff --git a/tests/Unit/Backend/SftpReadStreamTest.php b/tests/Unit/Backend/SftpReadStreamTest.php index 0718d0e..969f921 100644 --- a/tests/Unit/Backend/SftpReadStreamTest.php +++ b/tests/Unit/Backend/SftpReadStreamTest.php @@ -468,6 +468,10 @@ final class FakeSftpConnection { public array $openHandles = []; /** @var list paths of every OPEN request */ public array $opened = []; + /** @var resource|null like phpseclib's socket (SftpRangeFetcher waits on it) */ + public $fsock = null; + /** like phpseclib's: what came in with the last packet and is not read yet */ + public string $packet_buffer = ''; /** Whether the SFTP channel is open: phpseclib 2 opens it on the first SFTP call. */ public bool $initialised = false; /** Packets sent */ From 6121f684e9fac69235513903a0f504e38765d352 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 05:34:54 +0200 Subject: [PATCH 08/12] Full restore: say to run occ upgrade before rerunning a restore that stopped after the database; remove empty spool folders of a stopped restore --- CHANGELOG.md | 3 +++ docs/DISASTER-RECOVERY.md | 2 ++ lib/Command/RestoreFull.php | 1 + lib/Repository/Repository.php | 5 +++++ tests/Unit/Repository/ReadAheadTest.php | 11 +++++++++-- 5 files changed, 20 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8b0102..5496a03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ instead carried by the app description, the git tag, the release, and this file. cannot be read this way is read as before. Restores from other locations work as before. ### Fixed +- After a full restore stopped halfway, running it again with `--force` was refused when the + database was already restored: Nextcloud then first wants `occ upgrade`. The guide and the + error message now say so. - A full restore of a backup made before 1.0.0-rc.2 with `--skip-missing-tables` failed on PostgreSQL with "relation ... does not exist": the left-out tables were still looked up to set their ID counters. It now skips them. A new test restores a backup made by 1.0.0-rc.1 on diff --git a/docs/DISASTER-RECOVERY.md b/docs/DISASTER-RECOVERY.md index fd7bdf1..d1f48a1 100644 --- a/docs/DISASTER-RECOVERY.md +++ b/docs/DISASTER-RECOVERY.md @@ -95,6 +95,8 @@ 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`. +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`. **9. After the restore.** These worked in our tests: diff --git a/lib/Command/RestoreFull.php b/lib/Command/RestoreFull.php index 1516391..9774e7a 100644 --- a/lib/Command/RestoreFull.php +++ b/lib/Command/RestoreFull.php @@ -92,6 +92,7 @@ HELP); } 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.'); + $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; } $output->writeln(sprintf( diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 284a300..3c836b0 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -237,6 +237,11 @@ final class Repository { if (!$this->backend instanceof IReadsAhead || !$this->backend->canReadAhead()) { return null; } + // what a restore that was stopped left behind: empty, its files were unlinked when opened + $stale = glob(rtrim($target, '/') . '/.ngb-restore-spool-*', GLOB_ONLYDIR); + foreach ($stale === false ? [] : $stale as $dir) { + @rmdir($dir); + } $plan = new ReadAheadPlan(rtrim($target, '/') . '/.ngb-restore-spool-' . bin2hex(random_bytes(4))); foreach ($this->streamEntries($snapshotId) as $entry) { if ($prefix !== '' && !str_starts_with($entry['p'], $prefix)) { diff --git a/tests/Unit/Repository/ReadAheadTest.php b/tests/Unit/Repository/ReadAheadTest.php index 276639f..7d97709 100644 --- a/tests/Unit/Repository/ReadAheadTest.php +++ b/tests/Unit/Repository/ReadAheadTest.php @@ -41,8 +41,8 @@ class ReadAheadTest extends TestCase { $this->snapshot = $repo->backupDirectory($this->src)['snapshot']; } - private function restore(ReadAheadBackend $backend, string $prefix = ''): string { - $target = $this->tempDir(); + private function restore(ReadAheadBackend $backend, string $prefix = '', ?string $target = null): string { + $target ??= $this->tempDir(); $repo = Repository::openWithKey($backend, $this->keys, null, null); $repo->restore($this->snapshot, $target, $prefix); return $target; @@ -66,6 +66,13 @@ class ReadAheadTest extends TestCase { $this->assertLessThanOrEqual(count($backend->inner->list('packs')) + 1, count($backend->asked)); } + public function testTheEmptySpoolOfAStoppedRestoreIsRemoved(): void { + $target = $this->tempDir(); + mkdir("$target/.ngb-restore-spool-0badc0de", 0700); // a restore that was killed + + $this->assertRestored($this->restore(new ReadAheadBackend(new LocalBackend($this->root)), '', $target)); + } + public function testARangeThatCouldNotBeReadAheadIsReadTheUsualWay(): void { $backend = new ReadAheadBackend(new LocalBackend($this->root), fail: [1]); From c95f552cc6a321ecd00fa187d3360ebfb3675f8d Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 07:46:12 +0200 Subject: [PATCH 09/12] Read ahead over SFTP: one connection with 32 requests in flight by default, more with restore_sftp_connections; a Storage Box gets slower with parallel readers --- CHANGELOG.md | 10 +++++----- README.md | 7 ++++--- lib/Backend/SftpRangeFetcher.php | 20 ++++++++++++------- lib/Backend/StorageBackend.php | 7 +++++-- lib/Service/TargetService.php | 5 ++++- tests/Unit/Backend/SftpRangeFetcherTest.php | 4 ++-- .../Backend/StorageBackendReadAheadTest.php | 14 ++++++++++++- tests/integration/full-restore-roundtrip.sh | 2 +- 8 files changed, 47 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5496a03..a3fdc86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,11 +10,11 @@ instead carried by the app description, the git tag, the release, and this file. ## [Unreleased] ### Changed -- A full restore over SFTP reads the packs ahead on 4 connections of its own, many requests in - flight on each, into temporary files in the restore folder. From a Hetzner Storage Box to a - server in Canada (122 ms) packs now come in at about 7.6 MiB/s instead of 2.8; on a test line - with 120 ms and some packet loss, a 1.6 GiB restore went from 5.1 to 17.8 MiB/s. A part that - cannot be read this way is read as before. Restores from other locations work as before. +- A full restore over SFTP reads the packs ahead on a connection of its own, 32 requests in + flight, into temporary files in the restore folder. From a Hetzner Storage Box to a server in + Canada (122 ms) packs now come in at about 14 MiB/s instead of 2.8. More connections can be set + with `restore_sftp_connections` (README). A part that cannot be read this way is read as + before. Restores from other locations work as before. ### Fixed - After a full restore stopped halfway, running it again with `--force` was refused when the diff --git a/README.md b/README.md index 79e8258..db2595c 100644 --- a/README.md +++ b/README.md @@ -181,9 +181,10 @@ Contributions welcome via PR — add what you ran into with your own provider. server (or the VM/container host where TCP ends) to BBR: `sysctl -w net.core.default_qdisc=fq net.ipv4.tcp_congestion_control=bbr` (`modprobe tcp_bbr` first; make it permanent in `/etc/sysctl.d/`). - A full restore over SFTP reads the packs ahead on 4 connections of its own, next to the one - NG Backup already has open: the SFTP server must allow 5 connections at once for that user (a - Hetzner Storage Box allows 10). With fewer it uses the connections it gets. + A full restore over SFTP reads the packs ahead on a second connection, with many requests in + flight. A server that handles parallel reads well (not a Hetzner Storage Box, which gets slower) + can be read on more connections: `occ config:app:set ng_backup restore_sftp_connections + --value=4` (at most 8; the server must allow that many plus one at once for the user). - **NFS is not a files_external backend at all.** Mount the NFS export on the server yourself (e.g. via `/etc/fstab` or a systemd `.mount` unit) and point ng_backup's `local` backend at the diff --git a/lib/Backend/SftpRangeFetcher.php b/lib/Backend/SftpRangeFetcher.php index 153b13e..74651b5 100644 --- a/lib/Backend/SftpRangeFetcher.php +++ b/lib/Backend/SftpRangeFetcher.php @@ -11,10 +11,9 @@ namespace OCA\NgBackup\Backend; * Reads byte ranges of files on an SFTP location ahead of use, over several connections at once, * into local files (StorageBackend::readAhead(), for a full restore). * - * One connection is bound by the round trip and by what a single TCP connection gets over a long - * line: a restore from a Hetzner Storage Box to Canada (RTT 122 ms) reached about 2 MiB/s, with - * the PHP process mostly waiting. Here every connection reads its own range with DEPTH requests - * in flight. PHP runs one thing at a time: while it waits for an answer on one connection, the + * A restore that reads its packs as it goes is bound by the round trip: from a Hetzner Storage + * Box to Canada (RTT 122 ms) it reached about 2.8 MiB/s, with the PHP process mostly waiting. + * Here every connection reads its own range with DEPTH requests in flight, ahead of the restore. PHP runs one thing at a time: while it waits for an answer on one connection, the * answers of the others arrive in the kernel's buffers, so the connections still transfer at the * same time. * @@ -24,10 +23,17 @@ namespace OCA\NgBackup\Backend; * for the caller to read the usual way; a broken connection is not used again. */ final class SftpRangeFetcher { - /** Connections used at once by a full restore. A Hetzner Storage Box allows 10 per account. */ - public const CONNECTIONS = 4; + /** + * Connections a full restore uses, unless the app setting restore_sftp_connections says + * otherwise. One: a Hetzner Storage Box gave about 14 MiB/s on one connection with DEPTH + * requests in flight, and 4-6 MiB/s on 2 or 4 (it reads from disks, and parallel readers make + * them seek). On a line with packet loss to a server that handles parallel reads well, 4 + * connections did better (21 against 14 MiB/s). + */ + public const DEFAULT_CONNECTIONS = 1; + public const MAX_CONNECTIONS = 8; /** READ requests in flight per connection. */ - public const DEPTH = 16; + public const DEPTH = 32; /** Ranges read ahead of the one the caller is at, in flight or waiting in the spool. */ public const AHEAD = 8; /** diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index 24ee719..ec5897d 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -29,6 +29,8 @@ final class StorageBackend implements IBackend, IReadsAhead { /** Seconds without a request after which an SFTP connection is checked before it is used. */ public const IDLE_CHECK = 60; private float $lastUsed; + /** Connections readAhead() opens (see SftpRangeFetcher::DEFAULT_CONNECTIONS). */ + public int $readAheadConnections = SftpRangeFetcher::DEFAULT_CONNECTIONS; /** Times the storage was built anew after its connection died (for tests and diagnostics). */ public int $reconnects = 0; @@ -244,7 +246,7 @@ final class StorageBackend implements IBackend, IReadsAhead { } /** - * Over SFTP: SftpRangeFetcher, on CONNECTIONS new connections of its own (built with the + * Over SFTP: SftpRangeFetcher, on readAheadConnections new connections of its own (built with the * reconnect function), so the storage's connection stays free for other requests. Elsewhere * not (yet): null. */ @@ -262,7 +264,8 @@ final class StorageBackend implements IBackend, IReadsAhead { $storages = []; $connections = []; $root = ''; - for ($n = 0; $n < SftpRangeFetcher::CONNECTIONS; $n++) { + $wanted = max(1, min(SftpRangeFetcher::MAX_CONNECTIONS, $this->readAheadConnections)); + for ($n = 0; $n < $wanted; $n++) { try { $storage = $reconnect(); $sftp = SftpReadStream::connection($storage); diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index b0e914e..abb8989 100644 --- a/lib/Service/TargetService.php +++ b/lib/Service/TargetService.php @@ -287,7 +287,10 @@ final class TargetService { private function buildBackend(string $backend, string $auth, array $options, string $basePath, bool $createBase = true): StorageBackend { $create = fn (): IStorage => $this->factory()->create($backend, $auth, $options); - return new StorageBackend($create(), $basePath, $createBase, $create); + $storage = new StorageBackend($create(), $basePath, $createBase, $create); + $storage->readAheadConnections = Server::get(\OCP\IAppConfig::class) + ->getValueInt('ng_backup', 'restore_sftp_connections', \OCA\NgBackup\Backend\SftpRangeFetcher::DEFAULT_CONNECTIONS); + return $storage; } private ?ExternalStorageFactory $factory = null; diff --git a/tests/Unit/Backend/SftpRangeFetcherTest.php b/tests/Unit/Backend/SftpRangeFetcherTest.php index b27e8ff..51dedc1 100644 --- a/tests/Unit/Backend/SftpRangeFetcherTest.php +++ b/tests/Unit/Backend/SftpRangeFetcherTest.php @@ -79,9 +79,9 @@ class SftpRangeFetcherTest extends TestCase { } public function testKeepsSeveralRequestsInFlightOnEachConnection(): void { - $this->files['/home/repo/packs/big'] = random_bytes(6 * 1048576); + $this->files['/home/repo/packs/big'] = random_bytes(10 * 1048576); $connections = $this->connections(2); - self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', [['repo/packs/big', 0, 6 * 1048576], ['repo/packs/big', 5, 6 * 1048576 - 5]], $this->tempDir())); + self::collect(SftpRangeFetcher::fetch(self::opened($connections), '/home/', [['repo/packs/big', 0, 10 * 1048576], ['repo/packs/big', 5, 10 * 1048576 - 5]], $this->tempDir())); $this->assertSame(SftpRangeFetcher::DEPTH, $connections[0]->maxInFlight); $this->assertSame(SftpRangeFetcher::DEPTH, $connections[1]->maxInFlight); diff --git a/tests/Unit/Backend/StorageBackendReadAheadTest.php b/tests/Unit/Backend/StorageBackendReadAheadTest.php index 5bf7b7c..87818b2 100644 --- a/tests/Unit/Backend/StorageBackendReadAheadTest.php +++ b/tests/Unit/Backend/StorageBackendReadAheadTest.php @@ -53,6 +53,7 @@ class StorageBackendReadAheadTest extends TestCase { public function testReadsAheadOnNewConnectionsOnly(): void { $backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage()); + $backend->readAheadConnections = 4; $this->assertTrue($backend->canReadAhead()); $result = []; @@ -62,7 +63,7 @@ class StorageBackendReadAheadTest extends TestCase { $expected = array_map(fn (int $i): string => sha1(substr($this->files["/home/base/repo/packs/p$i"], 7, 1048576 - 7)), range(0, 5)); $this->assertSame($expected, $result); - $this->assertCount(1 + SftpRangeFetcher::CONNECTIONS, $this->connections); + $this->assertCount(1 + 4, $this->connections); $this->assertSame(0, $this->connections[0]->sent, 'the storage\'s own connection stays free'); foreach (array_slice($this->connections, 1) as $c) { $this->assertTrue($c->disconnected); @@ -72,6 +73,7 @@ class StorageBackendReadAheadTest extends TestCase { public function testGoesOnWithTheConnectionsTheServerAllows(): void { $this->connectLimit = 3; $backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage()); + $backend->readAheadConnections = 4; $count = 0; foreach ($backend->readAhead(self::ranges(), $this->tempDir()) as $fh) { @@ -83,6 +85,16 @@ class StorageBackendReadAheadTest extends TestCase { $this->assertCount(3, $this->connections); } + public function testByDefaultOneConnectionNextToTheStoragesOwn(): void { + $backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage()); + foreach ($backend->readAhead(self::ranges(), $this->tempDir()) as $fh) { + $this->assertIsResource($fh); + } + + $this->assertSame(1, SftpRangeFetcher::DEFAULT_CONNECTIONS); + $this->assertCount(2, $this->connections); + } + public function testNoNewConnectionMeansNoReadingAhead(): void { $this->connectLimit = 1; $backend = new StorageBackend($this->storage(), 'base/repo', false, fn (): IStorage => $this->storage()); diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index 0d5139e..7b65ed7 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -191,7 +191,7 @@ if [ "$MODE" = sftp ]; then LOGINS=$(( $(sftp_logins) - LOGINS_BEFORE )) [ "$LOGINS" -gt 1 ] || fail "the restore did not read ahead on connections of its own ($LOGINS SFTP logins)" docker exec ngb-frt-restore sh -c 'ls -a /var/www/html/data' | grep -q ngb-restore-spool && fail "the spool folder was left behind" - ok "over SFTP: read ahead on $LOGINS connections, large file identical, no spool left" + ok "over SFTP: read ahead on connections of its own ($LOGINS SFTP logins during the restore), large file identical, no spool left" fi run_on ngb-frt-restore backup:run offsite -l post-restore-check >/dev/null || fail "a new backup after the restore fails" From 9b85221e50abc5f52c40f8d4aa4dbd2774d4bfc8 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 08:54:13 +0200 Subject: [PATCH 10/12] Read ahead: a plan that cannot be made leaves the restore to the usual reads; only a whole buffered SFTP packet counts as an answer waiting --- lib/Backend/SftpRangeFetcher.php | 8 ++++++- lib/Repository/Repository.php | 24 ++++++++++++--------- tests/Unit/Backend/SftpRangeFetcherTest.php | 2 +- tests/Unit/Repository/ReadAheadTest.php | 19 ++++++++++++++++ 4 files changed, 41 insertions(+), 12 deletions(-) diff --git a/lib/Backend/SftpRangeFetcher.php b/lib/Backend/SftpRangeFetcher.php index 74651b5..def71af 100644 --- a/lib/Backend/SftpRangeFetcher.php +++ b/lib/Backend/SftpRangeFetcher.php @@ -150,7 +150,7 @@ final class SftpRangeFetcher { $buffered = $vars['requestBuffer'] ?? null; $channels = $vars['channel_buffers'] ?? null; if ((is_array($buffered) && isset($job->queue[0]) && array_key_exists($job->queue[0][0], $buffered)) - || (is_string($vars['packet_buffer'] ?? null) && $vars['packet_buffer'] !== '') + || (is_string($vars['packet_buffer'] ?? null) && self::wholePacket($vars['packet_buffer'])) || (is_array($channels) && array_filter($channels) !== [])) { $ready[] = $c; continue; @@ -173,6 +173,12 @@ final class SftpRangeFetcher { return $n === false || $n === 0 ? array_keys($this->jobs) : array_keys($read); } + /** Whether $buffer starts with a whole SFTP packet (length, then that many bytes). */ + private static function wholePacket(string $buffer): bool { + $length = strlen($buffer) >= 4 ? unpack('N', $buffer) : false; + return is_array($length) && strlen($buffer) >= 4 + $length[1]; + } + private function start(int $c, int $i): void { [$path, $offset, $length] = $this->ranges[$i]; $fh = $this->spoolFile(); diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 3c836b0..bc3b6f7 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -243,18 +243,22 @@ final class Repository { @rmdir($dir); } $plan = new ReadAheadPlan(rtrim($target, '/') . '/.ngb-restore-spool-' . bin2hex(random_bytes(4))); - foreach ($this->streamEntries($snapshotId) as $entry) { - if ($prefix !== '' && !str_starts_with($entry['p'], $prefix)) { - continue; - } - foreach ($entry['b'] as $id) { - try { - [$pack, $offset, $length] = $this->index->get($id); - } catch (RepositoryException) { - continue; // the restore reports it when it gets there + try { + foreach ($this->streamEntries($snapshotId) as $entry) { + if ($prefix !== '' && !str_starts_with($entry['p'], $prefix)) { + continue; + } + foreach ($entry['b'] as $id) { + try { + [$pack, $offset, $length] = $this->index->get($id); + } catch (RepositoryException) { + continue; // the restore reports it when it gets there + } + $plan->add('packs/' . substr($pack, 0, 2) . '/' . $pack, $offset, $length); } - $plan->add('packs/' . substr($pack, 0, 2) . '/' . $pack, $offset, $length); } + } catch (\Throwable) { + return null; // reading ahead is only faster: the restore itself reads the snapshot again and reports what is wrong } if (count($plan->ranges()) < 2 || (!is_dir($plan->spool) && !@mkdir($plan->spool, 0700, true))) { return null; diff --git a/tests/Unit/Backend/SftpRangeFetcherTest.php b/tests/Unit/Backend/SftpRangeFetcherTest.php index 51dedc1..5544d76 100644 --- a/tests/Unit/Backend/SftpRangeFetcherTest.php +++ b/tests/Unit/Backend/SftpRangeFetcherTest.php @@ -96,7 +96,7 @@ class SftpRangeFetcherTest extends TestCase { $pair = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP); $sockets[] = $pair; $c->fsock = $pair[0]; - $c->packet_buffer = 'more'; + $c->packet_buffer = pack('N', 4) . 'more'; // a whole packet } $ranges = array_slice($this->ranges(), 0, 3); $start = microtime(true); diff --git a/tests/Unit/Repository/ReadAheadTest.php b/tests/Unit/Repository/ReadAheadTest.php index 7d97709..2e9ef34 100644 --- a/tests/Unit/Repository/ReadAheadTest.php +++ b/tests/Unit/Repository/ReadAheadTest.php @@ -66,6 +66,15 @@ class ReadAheadTest extends TestCase { $this->assertLessThanOrEqual(count($backend->inner->list('packs')) + 1, count($backend->asked)); } + public function testAPlanThatCannotBeMadeLeavesTheRestoreToTheUsualReads(): void { + $backend = new ReadAheadBackend(new LocalBackend($this->root), failSnapshotReads: 1); + + $this->assertRestored($this->restore($backend)); + + $this->assertSame([], $backend->asked, 'no reading ahead without a plan'); + $this->assertSame(2, $backend->snapshotReads, 'the restore itself read the snapshot again'); + } + public function testTheEmptySpoolOfAStoppedRestoreIsRemoved(): void { $target = $this->tempDir(); mkdir("$target/.ngb-restore-spool-0badc0de", 0700); // a restore that was killed @@ -195,6 +204,8 @@ final class ReadAheadBackend implements IBackend, IReadsAhead { private bool $unavailable = false, /** reading a pack the usual way fails */ private bool $packsGone = false, + /** this many reads of a snapshot fail, then they work */ + private int $failSnapshotReads = 0, ) { $this->counter = new PackOpenCountingBackend($inner); } @@ -238,7 +249,15 @@ final class ReadAheadBackend implements IBackend, IReadsAhead { $this->counter->put($path, $stream); } + public int $snapshotReads = 0; + public function get(string $path) { + if (str_starts_with($path, 'snapshots/')) { + $this->snapshotReads++; + if ($this->snapshotReads <= $this->failSnapshotReads) { + throw new BackendException('Connection lost reading ' . $path); + } + } if ($this->packsGone && str_starts_with($path, 'packs/')) { throw new BackendException('Not found: ' . $path); } From a384ab41bb56db34239a104a35f388ce884e1b43 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 10:31:21 +0200 Subject: [PATCH 11/12] App store description: Ensembia line --- appinfo/info.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/appinfo/info.xml b/appinfo/info.xml index 3b0fe43..210a3ce 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -39,8 +39,8 @@ All-in-One and hosted Nextcloud. 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. -Developed and sponsored by [Ensembia](https://www.ensembia.com), which also offers paid help with -restoring a backup after a crash.]]> +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 agpl Dillard Blom From 583592f69be3c483cab8459545f2fc484acf35fc Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 19 Oct 2026 10:34:01 +0200 Subject: [PATCH 12/12] Release 1.0.0-rc.4 (app version 0.9.10) --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3fdc86..35ed537 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,10 @@ 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. -## [Unreleased] +## [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 +released: its fixes are in here. ### Changed - A full restore over SFTP reads the packs ahead on a connection of its own, 32 requests in @@ -15,6 +18,7 @@ instead carried by the app description, the git tag, the release, and this file. Canada (122 ms) packs now come in at about 14 MiB/s instead of 2.8. More connections can be set with `restore_sftp_connections` (README). A part that cannot be read this way is read as before. Restores from other locations work as before. +- App store description: the line about Ensembia. ### Fixed - After a full restore stopped halfway, running it again with `--force` was refused when the