From d96229f370d19a168fec8b0753de5358885f2c1c Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Fri, 6 Nov 2026 17:34:33 +0100 Subject: [PATCH] Release the locks of a killed restore when it runs again; legacy test without empty folders A killed restore left its leases for up to two hours, so the first backup of the recovered server was refused. A backup from before 1.0.3 records no empty folders: the test now expects that. --- CHANGELOG.md | 2 ++ PLAN.md | 12 ++++++++++-- lib/Service/BackupService.php | 2 +- lib/Service/LeaseService.php | 9 +++++++++ lib/Service/RestoreFullService.php | 6 ++++++ tests/integration/full-restore-roundtrip.sh | 14 ++++++++++---- 6 files changed, 38 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a60e48..1d1cb15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ shown under 1.0.0. The git tag of a release candidate goes after the date. - A restore that goes on after a stop also removes the temporary files in folders where every file is already in place, and checks that those folders are inside the restore directory before it leaves their files alone. +- After a restore that was killed and then ran again, a backup no longer has to wait up to two hours + for the locks of the killed run. - A restore only removes read-ahead folders of its own (`.ngb-restore-spool-` with 8 hex digits). - A restore keeps its lock on the location during a long row of files that are already in place. - A database restore that fails no longer leaves behind tables it created, which could stop the diff --git a/PLAN.md b/PLAN.md index 6f72fdf..ca8cb1d 100644 --- a/PLAN.md +++ b/PLAN.md @@ -458,8 +458,16 @@ in README "Known limitations": (Nextcloud zet de PHP-tijdslimiet ruim, een gestopte stap wordt overgedaan); wel eerlijke beurten. Bin leest in delen met teller en balk (besluit 2026-11-06, ook met oog op meerdere backups per dag). Getest: unit (volle schijf op echte tmpfs), integratie NC35 sqlite/pgsql/mysql, sftp, salvage, - salvage-files; Bin in een echte browser met vertraagde verzoeken (1 van 7 t/m 7). Nog: kleine - volledige simulaties, reviews vóór de PR, README-regels bij de versiecommit. + salvage-files; Bin in een echte browser met vertraagde verzoeken (1 van 7 t/m 7). Nog: reviews + vóór de PR, README-regels bij de versiecommit. + **Simulaties (2026-11-06, NC 34.0.4, 2 GB, lokale SFTP, `~/dev-local/ngb-sim/sim.sh`):** normaal + (pgsql), stop en hervat (pgsql → MariaDB, restore met kill -9 gestopt), volle schijf met `--salvage`: + alle drie geslaagd, alle 3.101 gebruikersbestanden en 7 lege mappen gelijk, share, agenda en WebDAV + werken. Vond één fout, opgelost: na een kill bleven vergrendelingen tot 2 uur staan, waardoor de + backup van stap 11 werd geweigerd. `files:scan` "Updated" zijn in de gemeten run alleen mappen + (mtime van mappen wordt niet teruggezet, dus nieuwe mtime en etag; bestanden ongemoeid). "Removed": + o.a. `cache` van een gebruiker, die de backup uitsluit. Waarschijnlijk verklaart dit ook de 50.080 + "updated" op nc01, niet op nc01 zelf vastgesteld. - **Branches (besluit 2026-11-06):** werk voor een versie op een branch met die naam (`1.0.3`); `polish` alleen voor kleine cosmetische zaken, per geval bekijken of en wanneer die gemerged worden. SMB staat uit op sub5 en sub6; sub6 is het testaccount, eerst aanzetten voor een SMB-test. diff --git a/lib/Service/BackupService.php b/lib/Service/BackupService.php index 3951f3d..0d45f55 100644 --- a/lib/Service/BackupService.php +++ b/lib/Service/BackupService.php @@ -79,7 +79,7 @@ final class BackupService { try { return $this->leases->with(PruneService::lockKey($target), LeaseService::EXCLUSIVE, 60, fn () => $this->createRun($target, $label)); } catch (LockedException) { - throw new \RuntimeException('A cleanup or another start for ' . $target->getName() . ' is running; try again later'); + throw new \RuntimeException('A cleanup, a restore or another start for ' . $target->getName() . ' is running, or one was stopped by force (that clears by itself within two hours); try again later'); } } diff --git a/lib/Service/LeaseService.php b/lib/Service/LeaseService.php index 43437af..ced84ac 100644 --- a/lib/Service/LeaseService.php +++ b/lib/Service/LeaseService.php @@ -76,6 +76,15 @@ final class LeaseService { $qb->executeStatement(); } + /** Remove the shared leases on $name, for a caller that knows their holders were stopped. */ + public function dropShared(string $name): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete('ngb_leases') + ->where($qb->expr()->eq('name', $qb->createNamedParameter($name))) + ->andWhere($qb->expr()->eq('mode', $qb->createNamedParameter(self::SHARED))); + $qb->executeStatement(); + } + /** Wraps $refresh so calling it often (per file, per row batch) only touches the database every $seconds. */ public static function throttled(callable $refresh, int $seconds = 60): \Closure { $last = time(); diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index d3cb528..b463251 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -151,6 +151,12 @@ final class RestoreFullService { // The database cannot go into this installation; the files can. $check['dbUnreadable'] = 'the database cannot be restored here: ' . $this->describeBlockers($check); } + if ($interrupted) { + // A restore that was killed leaves its leases, for up to two hours: they would keep + // the first backup of the recovered server waiting after this restore completes. + // In maintenance mode, on a partly restored instance, nothing else reads the location. + $this->leases->dropShared(PruneService::lockKey($target)); + } $appConfig->setValueInt(\OCA\NgBackup\AppInfo\Application::APP_ID, self::INTERRUPTED_KEY, time()); $result = $this->restoreInMaintenance($target, $snapshotId, $skipMissingTables, $check, $salvage); $appConfig->deleteKey(\OCA\NgBackup\AppInfo\Application::APP_ID, self::INTERRUPTED_KEY); diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index fb274e1..f4bff65 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -326,10 +326,16 @@ run_on ngb-frt-restore backup:status >/dev/null || fail "backup:status fails aft run_on ngb-frt-restore user:list | grep frtest >/dev/null || fail "the backed-up user is missing from the restored database" docker exec ngb-frt-restore test -f /var/www/html/data/frtest/files/Docs/note.txt || fail "restored file is missing" [ "$(docker exec ngb-frt-restore cat /var/www/html/data/frtest/files/Docs/note.txt)" = "hello disaster recovery" ] || fail "restored file content differs" -docker exec ngb-frt-restore test -d /var/www/html/data/frtest/files/Empty/Nested || fail "the empty folder did not come back" -run_on ngb-frt-restore files:scan frtest >/dev/null || fail "files:scan fails after restore" -docker exec ngb-frt-restore test -d /var/www/html/data/frtest/files/Empty/Nested || fail "files:scan removed the empty folder" -ok "the empty folder came back and stays after files:scan" +if [ "$LEGACY" = legacy ]; then + # Backups made before 1.0.3 do not record empty folders + docker exec ngb-frt-restore test -e /var/www/html/data/frtest/files/Empty/Nested && fail "an empty folder came back from a backup that does not record them" + run_on ngb-frt-restore files:scan frtest >/dev/null || fail "files:scan fails after restore" +else + docker exec ngb-frt-restore test -d /var/www/html/data/frtest/files/Empty/Nested || fail "the empty folder did not come back" + run_on ngb-frt-restore files:scan frtest >/dev/null || fail "files:scan fails after restore" + docker exec ngb-frt-restore test -d /var/www/html/data/frtest/files/Empty/Nested || fail "files:scan removed the empty folder" + ok "the empty folder came back and stays after files:scan" +fi # New rows after the restore: a new file (file cache) and a new user must not collide with # restored rows (ID counters after the data). docker exec ngb-frt-restore sh -c "echo new > /var/www/html/data/frtest/files/Docs/after-restore.txt && chown www-data /var/www/html/data/frtest/files/Docs/after-restore.txt"