Full restore goes on where it stopped; SMB recovers from a failed read

The DR test of 1.0.2 over SMB stopped after 392 GiB of 983: the connection to the Storage Box
dropped, and the smbclient extension then returns a string of length -1 for a failed read (it
stores smbc_read()'s -1 in an unsigned length). The read loop counted that as data and went
backwards forever at 100% CPU. Reads over SMB are now checked; a failed one is read again the
usual way over a new connection.

backup:restore:full that runs again (--force) leaves the data files already in place with the
snapshot's size and modification time, as a backup takes them as unchanged, and reads ahead only
what is still missing: after 392 GiB it goes on instead of starting over.

lib/Backend/StorageBackend.php

@@ -412,7 +412,8 @@ final class StorageBackend implements IBackend, IReadsAhead {
 				}
 			}
 		} catch (\Throwable) {
-			// read the usual way below, which reports what is wrong
+			// read the usual way below, which reports what is wrong, over a new connection
+			$this->smbReconnect();
 		}
 		$fh = $this->get($path);
 		try {
@@ -555,7 +556,8 @@ final class StorageBackend implements IBackend, IReadsAhead {
 			try {
 				$state->lseek($fh, $offset, SEEK_SET, $url);
 				for ($got = 0; $got < $length;) {
-					$chunk = (string)$state->read($fh, min(self::SMB_READ, $length - $got), $url);
+					$want = min(self::SMB_READ, $length - $got);
+					$chunk = self::smbRead($state, $fh, $want, $url);
 					if ($chunk === '') {
 						throw new BackendException('Short read in ' . $url);
 					}
@@ -590,22 +592,67 @@ final class StorageBackend implements IBackend, IReadsAhead {
 		} catch (\Throwable) {
 			return null;
 		}
+		$data = '';
 		try {
-			$data = '';
 			do {
-				/** @psalm-suppress MixedMethodCall */
-				$chunk = (string)$state->read($fh, self::SMB_READ, $url);
+				$chunk = self::smbRead($state, $fh, self::SMB_READ, $url);
 				$data .= $chunk;
 			} while ($chunk !== '');
-			return $data;
 		} catch (\Throwable) {
-			return null;
-		} finally {
-			try {
-				/** @psalm-suppress MixedMethodCall */
-				$state->close($fh, $url);
-			} catch (\Throwable) {
-			}
+			$data = null;
+		}
+		try {
+			/** @psalm-suppress MixedMethodCall */
+			$state->close($fh, $url);
+		} catch (\Throwable) {
+		}
+		if ($data === null) {
+			$this->smbReconnect(); // a read failed: the usual way reads it over a new connection
+		}
+		return $data;
+	}
+
+	/**
+	 * One read of at most $bytes through the native state, checked. When libsmbclient fails a read
+	 * (seen when the connection to the server dropped during a long restore), the smbclient
+	 * extension does not return false but a string whose length is -1, because it stores
+	 * smbc_read()'s -1 in an unsigned length; icewind passes it on. Counting that as data made
+	 * the read loop go backwards forever at 100% CPU (DR test 2026-11-05, after 392 GiB).
+	 *
+	 * @param resource $fh
+	 * @psalm-suppress MixedMethodCall, MixedArgument the state is icewind's NativeState
+	 */
+	private static function smbRead(object $state, $fh, int $bytes, string $url): string {
+		$chunk = $state->read($fh, $bytes, $url);
+		$n = is_string($chunk) ? strlen($chunk) : -1;
+		if ($n < 0 || $n > $bytes) {
+			throw new BackendException('Read failed in ' . $url);
+		}
+		return $chunk;
+	}
+
+	/**
+	 * After a failed read: drop icewind's connection state, so the next read (the usual way, or
+	 * the next range) connects again instead of failing on a connection that is gone.
+	 *
+	 * @psalm-suppress UndefinedClass icewind/smb comes with files_external, not with this app
+	 */
+	private function smbReconnect(): void {
+		if (!$this->storage instanceof \OCA\Files_External\Lib\Storage\SMB) {
+			return;
+		}
+		try {
+			(function (): void {
+				/** @psalm-suppress UndefinedThisPropertyFetch */
+				$share = $this->share;
+				if ($share instanceof \Icewind\SMB\Native\NativeShare) {
+					(function (): void {
+						/** @psalm-suppress UndefinedThisPropertyAssignment */
+						$this->state = null;
+					})->call($share);
+				}
+			})->call($this->storage);
+		} catch (\Throwable) {
 		}
 	}
 
@@ -657,12 +704,15 @@ final class StorageBackend implements IBackend, IReadsAhead {
 			try {
 				foreach ($abs as $i => [$path, $offset, $length]) {
 					$fh = null;
+					$out = false;
 					try {
 						$chunks = $this->smbChunks($path, $offset, $length);
 						$out = $chunks === null ? false : fopen($file, 'w+b');
 						if ($chunks !== null && $out !== false) {
 							foreach ($chunks as $chunk) {
-								fwrite($out, $chunk);
+								if (fwrite($out, $chunk) !== strlen($chunk)) {
+									throw new BackendException('Cannot write to the spool');
+								}
 							}
 							$fh = ftell($out) === $length && rewind($out) ? $out : null;
 							if ($fh === null) {
@@ -670,7 +720,11 @@ final class StorageBackend implements IBackend, IReadsAhead {
 							}
 						}
 					} catch (\Throwable) {
-						$fh = null; // read the usual way, which reports why
+						if (is_resource($out)) {
+							fclose($out);
+						}
+						$fh = null; // read the usual way, which reports why, over a new connection
+						$this->smbReconnect();
 					} finally {
 						@unlink($file); // an open stream keeps its data
 					}

lib/Repository/Repository.php

@@ -224,14 +224,18 @@ final class Repository {
 	 * @param array<string, string>|null $skipped salvage: an array to collect the files that could
 	 *             not be read or decrypted (path => reason), which are then left out instead of
 	 *             stopping the restore; null (default) stops at the first such file
+	 * @param bool $keepUnchanged leave a file that is already in place with the size and
+	 *             modification time of the snapshot, as a backup takes such a file as unchanged:
+	 *             a restore that was stopped and runs again goes on where it was, without reading
+	 *             what it already restored from the location again
 	 */
-	public function restore(string $snapshotId, string $target, string $prefix = '', bool $stripPrefix = false, ?callable $heartbeat = null, ?array &$skipped = null): int {
+	public function restore(string $snapshotId, string $target, string $prefix = '', bool $stripPrefix = false, ?callable $heartbeat = null, ?array &$skipped = null, bool $keepUnchanged = false): int {
 		if ($prefix !== '' && !self::isSafePath(rtrim($prefix, '/'))) {
 			throw new RepositoryException('Invalid path prefix');
 		}
-		$plan = $this->readAhead($snapshotId, $prefix, $target);
+		$plan = $this->readAhead($snapshotId, $prefix, $target, $stripPrefix, $keepUnchanged);
 		try {
-			return $this->restoreEntries($snapshotId, $target, $prefix, $stripPrefix, $heartbeat, $plan, $skipped);
+			return $this->restoreEntries($snapshotId, $target, $prefix, $stripPrefix, $heartbeat, $plan, $skipped, $keepUnchanged);
 		} finally {
 			$plan?->close();
 			if ($plan !== null) {
@@ -245,7 +249,7 @@ final class Repository {
 	 * 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 {
+	private function readAhead(string $snapshotId, string $prefix, string $target, bool $stripPrefix, bool $keepUnchanged): ?ReadAheadPlan {
 		if (!$this->backend instanceof IReadsAhead || !$this->backend->canReadAhead()) {
 			return null;
 		}
@@ -260,6 +264,9 @@ final class Repository {
 				if (!self::underPrefix($entry['p'], $prefix)) {
 					continue;
 				}
+				if ($keepUnchanged && self::inPlace(self::destination($target, $entry['p'], $prefix, $stripPrefix), $entry)) {
+					continue;
+				}
 				foreach ($entry['b'] as $id) {
 					try {
 						[$pack, $offset, $length] = $this->index->get($id);
@@ -289,18 +296,19 @@ final class Repository {
 	}
 
 	/** @param array<string, string>|null $skipped see restore() */
-	private function restoreEntries(string $snapshotId, string $target, string $prefix, bool $stripPrefix, ?callable $heartbeat, ?ReadAheadPlan $plan, ?array &$skipped): int {
+	private function restoreEntries(string $snapshotId, string $target, string $prefix, bool $stripPrefix, ?callable $heartbeat, ?ReadAheadPlan $plan, ?array &$skipped, bool $keepUnchanged): int {
 		$count = 0;
 		$cleaned = [];
 		foreach ($this->streamEntries($snapshotId) as $entry) {
 			if (!self::underPrefix($entry['p'], $prefix)) {
 				continue;
 			}
-			$relPath = $entry['p'];
-			if ($stripPrefix && $prefix !== '') {
-				$relPath = $entry['p'] === rtrim($prefix, '/') ? basename($entry['p']) : substr($entry['p'], strlen($prefix));
+			$dest = self::destination($target, $entry['p'], $prefix, $stripPrefix);
+			if ($keepUnchanged && self::inPlace($dest, $entry)) {
+				// below the target as written by an earlier run of this restore (checked then)
+				$count++;
+				continue;
 			}
-			$dest = rtrim($target, '/') . '/' . $relPath;
 			if (!is_dir(dirname($dest))) {
 				mkdir(dirname($dest), 0700, true);
 			}
@@ -358,6 +366,26 @@ final class Repository {
 		return $count;
 	}
 
+	/** Where restore() writes the snapshot path $path. */
+	private static function destination(string $target, string $path, string $prefix, bool $stripPrefix): string {
+		$relPath = $path;
+		if ($stripPrefix && $prefix !== '') {
+			$relPath = $path === rtrim($prefix, '/') ? basename($path) : substr($path, strlen($prefix));
+		}
+		return rtrim($target, '/') . '/' . $relPath;
+	}
+
+	/**
+	 * A regular file (no link) at $dest with the size and modification time of the snapshot
+	 * entry: what a backup takes as unchanged, and what a finished restore of it leaves.
+	 *
+	 * @param array{s: int, m: int, ...} $entry
+	 */
+	private static function inPlace(string $dest, array $entry): bool {
+		clearstatcache(true, $dest);
+		return !is_link($dest) && is_file($dest) && filesize($dest) === $entry['s'] && filemtime($dest) === $entry['m'];
+	}
+
 	/** $prefix is "" (everything) or ends with "/"; the path without that "/" is a file to restore too. */
 	private static function underPrefix(string $path, string $prefix): bool {
 		return $prefix === '' || str_starts_with($path, $prefix) || $path === rtrim($prefix, '/');

lib/Service/RestoreFullService.php

@@ -186,7 +186,7 @@ final class RestoreFullService {
 			}
 			if ($dbError !== null) {
 				$salvageDir = $dataDir . '/ngb-salvage-' . substr($snapshotId, 0, 12);
-				$dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $salvageDir . '/data', stripPrefix: true, heartbeat: $beat, skipped: $skipped);
+				$dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $salvageDir . '/data', stripPrefix: true, heartbeat: $beat, skipped: $skipped, keepUnchanged: true);
 				$this->restore->restoreToDirectory($target, $snapshotId, 'config', $salvageDir . '/config', stripPrefix: true, heartbeat: $beat, skipped: $skipped);
 				return ['dbReport' => ['skipped' => [], 'restored' => [], 'created' => [], 'missing' => []], 'dataFiles' => $dataFiles, 'configKeys' => 0,
 					'dbConsistent' => true, 'missingApps' => $check['missingApps'], 'skipped' => $skipped ?? [], 'dbError' => $dbError,
@@ -209,7 +209,7 @@ final class RestoreFullService {
 				$backupConfig = [];
 			}
 
-			$dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $dataDir, stripPrefix: true, heartbeat: $beat, skipped: $skipped);
+			$dataFiles = $this->restore->restoreToDirectory($target, $snapshotId, 'data', $dataDir, stripPrefix: true, heartbeat: $beat, skipped: $skipped, keepUnchanged: true);
 
 			// Last step: nothing after this may need $this->targets->repository($target) again.
 			if ($backupConfig !== []) {

lib/Service/RestoreService.php

@@ -169,15 +169,18 @@ final class RestoreService {
 	 * @param bool $stripPrefix write into $directory itself instead of $directory/$prefix (see
 	 *             Repository::restore()); used to restore "data/" or "config/" straight into the
 	 *             real data/config directory for occ backup:restore:full.
+	 * @param bool $keepUnchanged leave files already in place with the snapshot's size and
+	 *             modification time (see Repository::restore()): a full restore that runs again
+	 *             goes on where it stopped
 	 */
-	public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory, bool $stripPrefix = false, ?callable $heartbeat = null, ?array &$skipped = null): int {
+	public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory, bool $stripPrefix = false, ?callable $heartbeat = null, ?array &$skipped = null, bool $keepUnchanged = false): int {
 		if ($prefix !== '' && !Repository::isSafePath($prefix)) {
 			throw new \InvalidArgumentException('Invalid path: ' . $prefix);
 		}
 		if (!is_dir($directory) && !mkdir($directory, 0750, true) && !is_dir($directory)) {
 			throw new \RuntimeException("Cannot create $directory");
 		}
-		return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId, $prefix, $directory, $stripPrefix, $heartbeat, &$skipped): int {
+		return $this->withSharedLock($target, function (callable $refresh) use ($target, $snapshotId, $prefix, $directory, $stripPrefix, $heartbeat, &$skipped, $keepUnchanged): int {
 			$own = LeaseService::throttled($refresh);
 			$beat = static function () use ($own, $heartbeat): void {
 				$own();
@@ -185,7 +188,7 @@ final class RestoreService {
 					$heartbeat();
 				}
 			};
-			return $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/', $stripPrefix, $beat, $skipped);
+			return $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/', $stripPrefix, $beat, $skipped, $keepUnchanged);
 		}, 3600);
 	}
 

tests/Unit/Repository/RepositoryTest.php

@@ -101,6 +101,34 @@ class RepositoryTest extends TestCase {
 		$this->assertFileExists($stripped . '/note-1.txt');
 	}
 
+	public function testARestoreThatRunsAgainKeepsWhatIsInPlaceAndRestoresTheRest(): void {
+		$location = $this->tempDir();
+		$backend = new LocalBackend($location);
+		$repo = Repository::init($backend, 'pw');
+		$r = $repo->backupDirectory($this->src);
+		$target = $this->tempDir();
+		$repo->restore($r['snapshot'], $target);
+
+		// as a stopped restore leaves it: one file not there yet, one with other content
+		unlink($target . '/docs/d1/note-1.txt');
+		file_put_contents($target . '/docs/d0/note-0.txt', 'something else');
+		$this->assertSame(34, $repo->restore($r['snapshot'], $target, keepUnchanged: true));
+		$this->assertSame(self::hashTree($this->src), self::hashTree($target));
+
+		// all in place: nothing is read from the location, which could not give it anyway now
+		self::removeTree($location . '/packs');
+		$this->assertSame(34, Repository::open($backend, 'pw')->restore($r['snapshot'], $target, keepUnchanged: true));
+		$this->expectException(\Throwable::class);
+		Repository::open($backend, 'pw')->restore($r['snapshot'], $target);
+	}
+
+	private static function removeTree(string $dir): void {
+		foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($dir, \FilesystemIterator::SKIP_DOTS), \RecursiveIteratorIterator::CHILD_FIRST) as $f) {
+			$f->isDir() ? rmdir($f->getPathname()) : unlink($f->getPathname());
+		}
+		rmdir($dir);
+	}
+
 	public function testRestoreRemovesTemporariesOfAStoppedRestore(): void {
 		$repo = Repository::init($this->backend, 'pw');
 		$r = $repo->backupDirectory($this->src);