From 2610c1ad2f86df127c094e714e46282ee1167ab8 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Fri, 2 Oct 2026 14:44:52 +0200 Subject: [PATCH] Expiring leases instead of long-held locks A backup step killed by a time limit, OOM or kill -9 kept Nextcloud's lock on its location for up to an hour, blocking every new start and the run itself. Locations and runs are now guarded by leases in ngb_leases with an expiry: a backup step holds them for its duration plus two minutes, restores refresh theirs every minute, prune holds 30 minutes, untrash 5. Nextcloud's lock is only used for the milliseconds in which a lease is checked and written. Also: starting a backup while one is already running simply continues it (no exclusive lease needed), and running to completion waits instead of spinning while a crashed step's lease has not expired. Tested on NC34: a backup killed four times at random moments resumes on its own after the lease expires (about two minutes), and restores bit-identically. --- appinfo/info.xml | 2 +- .../Version000005Date20261002040000.php | 33 +++++++ lib/Service/BackupService.php | 41 +++++---- lib/Service/LeaseService.php | 88 +++++++++++++++++++ lib/Service/PruneService.php | 23 +++-- lib/Service/RestoreService.php | 29 +++--- 6 files changed, 174 insertions(+), 42 deletions(-) create mode 100644 lib/Migration/Version000005Date20261002040000.php create mode 100644 lib/Service/LeaseService.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 105ab81..9b0bf02 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -5,7 +5,7 @@ NG Backup Encrypted, incremental backups of your Nextcloud with restore from the web interface - 0.0.16 + 0.0.17 agpl Dillard Blom NgBackup diff --git a/lib/Migration/Version000005Date20261002040000.php b/lib/Migration/Version000005Date20261002040000.php new file mode 100644 index 0000000..f7a2dbe --- /dev/null +++ b/lib/Migration/Version000005Date20261002040000.php @@ -0,0 +1,33 @@ +hasTable('ngb_leases')) { + return null; + } + $t = $schema->createTable('ngb_leases'); + $t->addColumn('id', Types::STRING, ['notnull' => true, 'length' => 32]); + $t->addColumn('name', Types::STRING, ['notnull' => true, 'length' => 128]); + $t->addColumn('mode', Types::STRING, ['notnull' => true, 'length' => 1]); + $t->addColumn('expires_at', Types::BIGINT, ['notnull' => true, 'unsigned' => true]); + $t->setPrimaryKey(['id']); + $t->addIndex(['name'], 'ngb_leases_name'); + return $schema; + } +} diff --git a/lib/Service/BackupService.php b/lib/Service/BackupService.php index c7b1a9a..d195801 100644 --- a/lib/Service/BackupService.php +++ b/lib/Service/BackupService.php @@ -42,6 +42,7 @@ class BackupService { private ILockingProvider $locking, private LoggerInterface $logger, private AlertService $alerts, + private LeaseService $leases, ) { } @@ -54,16 +55,15 @@ class BackupService { } // Check and create the run under the location's exclusive lock, so a cleanup cannot start // in between (it takes the same lock and refuses while a run exists). - $lock = PruneService::lockKey($target); - try { - $this->locking->acquireLock($lock, ILockingProvider::LOCK_EXCLUSIVE); - } catch (LockedException) { - throw new \RuntimeException('A cleanup or another start for ' . $target->getName() . ' is running; try again later'); + // A run that is already in progress is simply continued; no lease needed for that. + $running = $this->runs->findRunning($target->getId(), self::KIND_FULL); + if ($running !== []) { + return $running[0]; } try { - return $this->createRun($target, $label); - } finally { - $this->locking->releaseLock($lock, ILockingProvider::LOCK_EXCLUSIVE); + 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'); } } @@ -103,18 +103,18 @@ class BackupService { * run locked and returns immediately. */ public function step(Run $run, float $deadline): Run { - $lockKey = 'ng_backup/run/' . $run->getId(); + // Leases expire on their own: a step killed by a time limit or OOM blocks nothing for long. + $ttl = (int)min(3600, max(120, $deadline - microtime(true) + 120)); try { - $this->locking->acquireLock($lockKey, ILockingProvider::LOCK_EXCLUSIVE); + $runLease = $this->leases->acquire('run/' . $run->getId(), LeaseService::EXCLUSIVE, $ttl); } catch (LockedException) { - return $run; + return $run; // another process is doing this step } - // Shared lock on the location while this step writes: a cleanup (exclusive) cannot run now. - $targetLock = 'ng_backup/target/' . $run->getTargetId(); + // Shared lease on the location while this step writes: a cleanup (exclusive) cannot run now. try { - $this->locking->acquireLock($targetLock, ILockingProvider::LOCK_SHARED); + $targetLease = $this->leases->acquire('ng_backup/target/' . $run->getTargetId(), LeaseService::SHARED, $ttl); } catch (LockedException) { - $this->locking->releaseLock($lockKey, ILockingProvider::LOCK_EXCLUSIVE); + $this->leases->release($runLease); return $run; } try { @@ -171,15 +171,22 @@ class BackupService { } return $run; } finally { - $this->locking->releaseLock($targetLock, ILockingProvider::LOCK_SHARED); - $this->locking->releaseLock($lockKey, ILockingProvider::LOCK_EXCLUSIVE); + $this->leases->release($targetLease); + $this->leases->release($runLease); } } /** Run to completion in this process (occ, system cron), reporting progress. */ public function runToCompletion(Run $run, ?callable $progress = null, float $stepSeconds = 30): Run { while ($run->getStatus() === Run::RUNNING) { + $before = $run->getUpdatedAt(); $run = $this->step($run, microtime(true) + $stepSeconds); + if ($run->getStatus() === Run::RUNNING && $run->getUpdatedAt() === $before) { + // Step was not possible (another process holds the run, or a crashed step's lease has + // not expired yet): wait instead of spinning. + sleep(5); + $run = $this->runs->find($run->getId()); + } if ($progress !== null) { $progress($run); } diff --git a/lib/Service/LeaseService.php b/lib/Service/LeaseService.php new file mode 100644 index 0000000..3609365 --- /dev/null +++ b/lib/Service/LeaseService.php @@ -0,0 +1,88 @@ +locking->acquireLock($guard, ILockingProvider::LOCK_EXCLUSIVE); + try { + $now = time(); + $qb = $this->db->getQueryBuilder(); + $qb->delete('ngb_leases') + ->where($qb->expr()->eq('name', $qb->createNamedParameter($name))) + ->andWhere($qb->expr()->lt('expires_at', $qb->createNamedParameter($now))); + $qb->executeStatement(); + + $qb = $this->db->getQueryBuilder(); + $qb->select('mode')->from('ngb_leases')->where($qb->expr()->eq('name', $qb->createNamedParameter($name))); + $modes = $qb->executeQuery()->fetchAll(\PDO::FETCH_COLUMN); + if ($modes !== [] && ($mode === self::EXCLUSIVE || in_array(self::EXCLUSIVE, $modes, true))) { + throw new LockedException($name); + } + $id = bin2hex(random_bytes(16)); + $qb = $this->db->getQueryBuilder(); + $qb->insert('ngb_leases')->values([ + 'id' => $qb->createNamedParameter($id), + 'name' => $qb->createNamedParameter($name), + 'mode' => $qb->createNamedParameter($mode), + 'expires_at' => $qb->createNamedParameter($now + $ttl), + ]); + $qb->executeStatement(); + return $id; + } finally { + $this->locking->releaseLock($guard, ILockingProvider::LOCK_EXCLUSIVE); + } + } + + /** Extend a lease while its holder is still working. */ + public function refresh(string $id, int $ttl): void { + $qb = $this->db->getQueryBuilder(); + $qb->update('ngb_leases')->set('expires_at', $qb->createNamedParameter(time() + $ttl)) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($id))); + $qb->executeStatement(); + } + + public function release(string $id): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete('ngb_leases')->where($qb->expr()->eq('id', $qb->createNamedParameter($id))); + $qb->executeStatement(); + } + + /** Run $fn under a lease; $refresh() can be called during long work. */ + public function with(string $name, string $mode, int $ttl, callable $fn): mixed { + $id = $this->acquire($name, $mode, $ttl); + try { + return $fn(fn () => $this->refresh($id, $ttl)); + } finally { + $this->release($id); + } + } +} diff --git a/lib/Service/PruneService.php b/lib/Service/PruneService.php index 5ae078c..478591c 100644 --- a/lib/Service/PruneService.php +++ b/lib/Service/PruneService.php @@ -29,15 +29,16 @@ class PruneService { private ILockingProvider $locking, private AlertService $alerts, private KeyService $keys, + private LeaseService $leases, ) { } public function untrash(Target $target, string $snapshotId): void { - $this->lockExclusive($target); + $lease = $this->lockExclusive($target, 300); try { $this->doUntrash($target, $snapshotId); } finally { - $this->locking->releaseLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE); + $this->leases->release($lease); } } @@ -47,31 +48,27 @@ class PruneService { */ public function underStartLock(Target $target, callable $fn): mixed { try { - $this->locking->acquireLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE); + return $this->leases->with(self::lockKey($target), LeaseService::EXCLUSIVE, 60, fn () => $fn()); } catch (LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); } - try { - return $fn(); - } finally { - $this->locking->releaseLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE); - } } /** * Exclusive lock on the location, then make sure no run of any kind (backup, restore, export) * is in progress: their data may not be referenced by a snapshot yet, or is being read. */ - private function lockExclusive(Target $target): void { + private function lockExclusive(Target $target, int $ttl): string { try { - $this->locking->acquireLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE); + $lease = $this->leases->acquire(self::lockKey($target), LeaseService::EXCLUSIVE, $ttl); } catch (LockedException) { throw new \RuntimeException('A backup, restore or cleanup of ' . $target->getName() . ' is in progress; try again later'); } if ($this->runs->findRunning($target->getId()) !== []) { - $this->locking->releaseLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE); + $this->leases->release($lease); throw new \RuntimeException('A backup or restore of ' . $target->getName() . ' is running; try again later'); } + return $lease; } private function doUntrash(Target $target, string $snapshotId): void { @@ -126,7 +123,7 @@ class PruneService { if ($target->getAppendOnly()) { throw new \RuntimeException('Location ' . $target->getName() . ' is append-only: NG Backup does not delete anything there'); } - $this->lockExclusive($target); + $lease = $this->lockExclusive($target, 1800); try { $repo = $this->targets->repository($target); $times = []; @@ -160,7 +157,7 @@ class PruneService { $prune = $repo->prune($dryRun); return ['kept' => $keep, 'forgotten' => $forget, 'purged' => $purged, 'prune' => $prune]; } finally { - $this->locking->releaseLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE); + $this->leases->release($lease); } } } diff --git a/lib/Service/RestoreService.php b/lib/Service/RestoreService.php index 1750a1c..95a1d95 100644 --- a/lib/Service/RestoreService.php +++ b/lib/Service/RestoreService.php @@ -36,23 +36,17 @@ class RestoreService { public function __construct( private TargetService $targets, private IRootFolder $root, - private \OCP\Lock\ILockingProvider $locking, + private LeaseService $leases, ) { } /** Shared lock on the location while restoring: a cleanup (exclusive) must not delete what is being read. */ - private function withSharedLock(Target $target, callable $fn): mixed { - $key = PruneService::lockKey($target); + private function withSharedLock(Target $target, callable $fn, int $ttl = 600): mixed { try { - $this->locking->acquireLock($key, \OCP\Lock\ILockingProvider::LOCK_SHARED); + return $this->leases->with(PruneService::lockKey($target), LeaseService::SHARED, $ttl, $fn); } catch (\OCP\Lock\LockedException) { throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is running; try again later'); } - try { - return $fn(); - } finally { - $this->locking->releaseLock($key, \OCP\Lock\ILockingProvider::LOCK_SHARED); - } } /** @@ -88,7 +82,20 @@ class RestoreService { * @return array{restored:int, trashed:int, target:string} */ public function restoreUserFiles(Target $target, string $snapshotId, string $logicalPath, string $mode = self::MODE_NEW_FOLDER, ?callable $progress = null): array { - return $this->withSharedLock($target, fn () => $this->doRestoreUserFiles($target, $snapshotId, $logicalPath, $mode, $progress)); + return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId, $logicalPath, $mode, $progress) { + $last = time(); + // Keep the lease alive while files are being written. + $beat = function (int $done, int $total, string $name) use ($refresh, $progress, &$last): void { + if (time() - $last >= 60) { + $refresh(); + $last = time(); + } + if ($progress !== null) { + $progress($done, $total, $name); + } + }; + return $this->doRestoreUserFiles($target, $snapshotId, $logicalPath, $mode, $beat); + }); } private function doRestoreUserFiles(Target $target, string $snapshotId, string $logicalPath, string $mode, ?callable $progress): array { @@ -150,7 +157,7 @@ class RestoreService { if (!is_dir($directory) && !mkdir($directory, 0750, true) && !is_dir($directory)) { throw new \RuntimeException("Cannot create $directory"); } - return $this->withSharedLock($target, fn () => $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/')); + return $this->withSharedLock($target, fn () => $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/'), 3600); } private function writeFile(Repository $repo, Folder $dest, string $relPath, array $entry): void {