Backup step: write the run only if no other process wrote it since it was loaded

CHANGELOG.md

@@ -17,9 +17,9 @@ instead carried by the app description, the git tag, the release, and this file.
 - A long first backup could end as failed although its snapshot was complete: when its final
   step took longer than the step's lease (150 s with `occ backup:run`), the background job took
   the run over, finished it a second time and failed on the snapshot it had already recorded,
-  overwriting the result. The lease now lasts at least 15 minutes, a step no longer overwrites a
+  overwriting the result. The lease now lasts at least 15 minutes, a step writes the run only if
-  run that another process wrote in the meantime, and recording a snapshot that is already
+  no other process wrote it since the step loaded it (in one database statement), and recording
-  recorded is no longer an error.
+  a snapshot that is already recorded is no longer an error.
 
 ## [0.9.4-beta.5] - 2026-10-09
 

lib/Db/RunMapper.php

@@ -17,6 +17,36 @@ final class RunMapper extends QBMapper {
 		parent::__construct($db, 'ngb_runs', Run::class);
 	}
 
+	/**
+	 * Write $run only if it is still running and unchanged since it was loaded (updated_at
+	 * $loadedAt), in one statement: two processes that loaded the same run cannot both write it.
+	 * The new updated_at is always later than $loadedAt, so every write is seen as a change.
+	 *
+	 * @return Run|null the written run, or null when another process wrote it first
+	 */
+	public function updateIfUnchanged(Run $run, int $loadedAt): ?Run {
+		$run->setUpdatedAt(max(time(), $loadedAt + 1));
+		$types = $run->getFieldTypes();
+		$qb = $this->db->getQueryBuilder();
+		$qb->update($this->getTableName());
+		foreach (array_keys($run->getUpdatedFields()) as $field) {
+			if ($field === 'id') {
+				continue;
+			}
+			$value = $run->{'get' . ucfirst($field)}();
+			$type = ($types[$field] ?? 'string') === 'integer' ? IQueryBuilder::PARAM_INT : IQueryBuilder::PARAM_STR;
+			$qb->set($run->propertyToColumn($field), $qb->createNamedParameter($value, $value === null ? IQueryBuilder::PARAM_NULL : $type));
+		}
+		$qb->where($qb->expr()->eq('id', $qb->createNamedParameter($run->getId(), IQueryBuilder::PARAM_INT)))
+			->andWhere($qb->expr()->eq('status', $qb->createNamedParameter(Run::RUNNING)))
+			->andWhere($qb->expr()->eq('updated_at', $qb->createNamedParameter($loadedAt, IQueryBuilder::PARAM_INT)));
+		if ($qb->executeStatement() !== 1) {
+			return null;
+		}
+		$run->resetUpdatedFields();
+		return $run;
+	}
+
 	public function find(int $id): Run {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('*')->from($this->getTableName())->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)));

lib/Service/BackupService.php

@@ -122,6 +122,7 @@ final class BackupService {
 			$this->leases->release($runLease);
 			return $run;
 		}
+		$loadedAt = null;
 		try {
 			$run = $this->runs->find($run->getId()); // fresh state after acquiring the lock
 			if ($run->getStatus() !== Run::RUNNING || $run->getKind() !== self::KIND_FULL) {
@@ -183,23 +184,27 @@ final class BackupService {
 			}
 			$run->setState(json_encode($state, JSON_THROW_ON_ERROR));
 			$run->setStats(json_encode(['db' => $state['dbStats'] ?? null, 'files' => $state['files']['stats']], JSON_THROW_ON_ERROR));
-			$run->setUpdatedAt(time());
+			// Another process may have taken the run over after this step's lease expired: then its
-			$current = $this->takenOver($run->getId(), $loadedAt);
+			// writes stand and this step's are dropped.
-			return $current ?? $this->runs->update($run);
+			return $this->runs->updateIfUnchanged($run, $loadedAt) ?? $this->runs->find($run->getId());
 		} catch (\Throwable $e) {
-			// Another process that took the run over (after this step's lease expired) may have
+			if ($loadedAt === null) {
-			// finished it in the meantime: its result stands, this step's error does not.
+				// The run could not even be loaded: there is nothing fresh to mark as failed, and
-			$current = isset($loadedAt) ? $this->takenOver($run->getId(), $loadedAt) : null;
+				// the caller's copy may be stale.
-			if ($current !== null) {
+				$this->logger->error('NG Backup run ' . $run->getId() . ': step could not load the run: ' . $e->getMessage(), ['exception' => $e]);
-				$this->logger->warning('NG Backup run ' . $run->getId() . ': a step that another process had taken over ended with: ' . $e->getMessage());
+				return $run;
-				return $current;
 			}
-			$this->logger->error('NG Backup run ' . $run->getId() . ' failed: ' . $e->getMessage(), ['exception' => $e]);
 			$run->setStatus(Run::FAILED);
 			$run->setError($e->getMessage());
 			$run->setFinishedAt(time());
-			$run->setUpdatedAt(time());
+			$failed = $this->runs->updateIfUnchanged($run, $loadedAt);
-			$run = $this->runs->update($run);
+			if ($failed === null) {
+				// Another process that took the run over may have finished it: its result stands.
+				$this->logger->warning('NG Backup run ' . $run->getId() . ': a step that another process had taken over ended with: ' . $e->getMessage());
+				return $this->runs->find($run->getId());
+			}
+			$run = $failed;
+			$this->logger->error('NG Backup run ' . $run->getId() . ' failed: ' . $e->getMessage(), ['exception' => $e]);
 			try {
 				$this->alerts->runFailed($run, isset($target) ? $target->getName() : '#' . $run->getTargetId());
 			} catch (\Throwable) {
@@ -211,15 +216,6 @@ final class BackupService {
 		}
 	}
 
-	/**
-	 * The run as stored, when another process has written it since this step loaded it (updated_at
-	 * $loadedAt); null when it is still this step's to write.
-	 */
-	private function takenOver(int $runId, int $loadedAt): ?Run {
-		$current = $this->runs->find($runId);
-		return $current->getUpdatedAt() !== $loadedAt || $current->getStatus() !== Run::RUNNING ? $current : null;
-	}
-
 	/** 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) {
@@ -259,7 +255,14 @@ final class BackupService {
 		$s->setCreatedAt(time());
 		$s->setFiles($files['stats']['files']);
 		$s->setBytes($files['stats']['bytes']);
+		try {
 			$this->snapshots->insert($s);
+		} catch (\OCP\DB\Exception $e) {
+			// Recorded by another process between the check above and this insert.
+			if ($e->getReason() !== \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
+				throw $e;
+			}
+		}
 	}
 
 	/** @return list<string> */