chore(cypress): Try to analyze failures

Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>

Author: DerDreschner

.github/workflows/integration-sqlite.yml

@@ -138,6 +138,9 @@ jobs:
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
 
+      - name: Print phpinfo
+        run: php -i
+
       - name: Set up dependencies
         run: |
           composer install

apps/files_versions/lib/Storage.php

@@ -426,20 +426,6 @@ private static function copyFileContents($view, $path1, $path2) {
 		try {
 			$view->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE);
 		} catch (\Throwable $e) {
-			// DIAGNOSTIC: dump all currently-held file locks to identify the
-			// concurrent holder blocking the restore (lock: -1 exclusive, >0 shared).
-			try {
-				$db = \OCP\Server::get(\OCP\IDBConnection::class);
-				$q = $db->getQueryBuilder();
-				$q->select('key', 'lock')->from('file_locks')
-					->where($q->expr()->neq('lock', $q->createNamedParameter(0, \OCP\DB\QueryBuilder\IQueryBuilder::PARAM_INT)));
-				$held = $q->executeQuery()->fetchAll();
-				\OCP\Server::get(\Psr\Log\LoggerInterface::class)->error(
-					'[restore-lock-diag] target=' . $internalPath2 . ' heldLocks=' . json_encode($held),
-					['app' => 'files_versions'],
-				);
-			} catch (\Throwable $ignore) {
-			}
 			// Acquiring the second lock can fail (e.g. the target file is
 			// transiently locked by a concurrent job under load). Release the
 			// first lock we already hold so a retry does not collide with a

apps/files_versions/lib/Versions/VersionManager.php

@@ -34,6 +34,8 @@ class VersionManager implements IVersionManager, IDeletableVersionBackend, INeed
 
 	public function __construct(
 		private IEventDispatcher $dispatcher,
+		private int $lockRetryBudgetMs = 10000,
+		private int $lockRetryInitialBackoffMs = 100,
 	) {
 	}
 
@@ -102,7 +104,7 @@ public function createVersion(IUser $user, FileInfo $file) {
 	#[\Override]
 	public function rollback(IVersion $version) {
 		$backend = $version->getBackend();
-		$result = self::handleAppLocks(fn (): ?bool => self::retryOnLock(fn (): ?bool => $backend->rollback($version)));
+		$result = self::handleAppLocks(fn (): ?bool => $this->retryOnLock(fn (): ?bool => $backend->rollback($version)));
 		// rollback doesn't have a return type yet and some implementations don't return anything
 		if ($result === null || $result === true) {
 			$this->dispatcher->dispatchTyped(new VersionRestoredEvent($version));
@@ -197,16 +199,14 @@ public function setMetadataValue(Node $node, int $revision, string $key, string
 	 * @return bool|null
 	 * @throws LockedException if the file stays locked for the whole budget
 	 */
-	private static function retryOnLock(callable $callback): ?bool {
-		// Total time we are willing to wait for a concurrent lock to clear.
-		$budgetMs = 15000;
+	private function retryOnLock(callable $callback): ?bool {
 		$waitedMs = 0;
-		$backoffMs = 100;
+		$backoffMs = $this->lockRetryInitialBackoffMs;
 		for ($attempt = 1; ; $attempt++) {
 			try {
 				return $callback();
 			} catch (LockedException $e) {
-				if ($waitedMs >= $budgetMs) {
+				if ($waitedMs >= $this->lockRetryBudgetMs) {
 					throw $e;
 				}
 				Server::get(LoggerInterface::class)->debug(

apps/files_versions/tests/StorageTest.php

@@ -8,10 +8,13 @@
 
 namespace OCA\files_versions\tests;
 
+use OC\Files\View;
 use OCA\Files_Versions\Expiration;
 use OCA\Files_Versions\Storage;
 use OCP\Files\IRootFolder;
 use OCP\Files\NotFoundException;
+use OCP\Lock\ILockingProvider;
+use OCP\Lock\LockedException;
 use OCP\Server;
 use Test\TestCase;
 use Test\Traits\UserTrait;
@@ -95,4 +98,42 @@ public function testExpireMaxAge(): void {
 		$this->assertCount(0, Storage::getVersions('version_test', 'folder1/sub1/file3'));
 		$this->assertCount(1, Storage::getVersions('version_test', 'folder2/file4'));
 	}
+
+	/**
+	 * Regression test: when copyFileContents() fails to acquire the lock on the
+	 * target file, the lock already held on the source file must be released
+	 * before the exception propagates. Otherwise the source lock leaks and a
+	 * retry (or any later access) collides with a lock nobody owns anymore.
+	 */
+	public function testCopyFileContentsReleasesSourceLockWhenTargetLockFails(): void {
+		$source = 'files/source.txt';
+		$target = 'files/target.txt';
+		$lockedException = new LockedException($target);
+
+		$storage = $this->createMock(\OC\Files\Storage\Storage::class);
+
+		$view = $this->createMock(View::class);
+		$view->method('resolvePath')->willReturn([$storage, 'internal']);
+
+		// The source lock is acquired first and succeeds; acquiring the target
+		// lock then throws (e.g. a concurrent holder under load).
+		$view->expects($this->exactly(2))
+			->method('lockFile')
+			->willReturnCallback(function (string $path) use ($target, $lockedException): void {
+				if ($path === $target) {
+					throw $lockedException;
+				}
+			});
+
+		// The already-held source lock must be released exactly once, and the
+		// never-acquired target lock must not be touched.
+		$view->expects($this->once())
+			->method('unlockFile')
+			->with($source, ILockingProvider::LOCK_EXCLUSIVE);
+
+		$this->expectExceptionObject($lockedException);
+
+		$method = new \ReflectionMethod(Storage::class, 'copyFileContents');
+		$method->invoke(null, $view, $source, $target);
+	}
 }

apps/files_versions/tests/Versions/VersionManagerTest.php

@@ -16,6 +16,7 @@
 use OCA\Files_Versions\Versions\VersionManager;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Storage\IStorage;
+use OCP\Lock\LockedException;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
@@ -141,4 +142,54 @@ public function testRollbackFailure(): void {
 
 		$this->assertFalse($manager->rollback($versionMock));
 	}
+
+	public function testRollbackRetriesWhileFileIsTransientlyLocked(): void {
+		$versionMock = $this->createMock(IVersion::class);
+		$backendMock = $this->createMock(IVersionBackend::class);
+		$versionMock->method('getBackend')->willReturn($backendMock);
+
+		// The live file is locked for the first two attempts (e.g. a concurrent
+		// read holds a shared lock) and then the lock clears.
+		$attempts = 0;
+		$backendMock->expects($this->exactly(3))
+			->method('rollback')
+			->with($versionMock)
+			->willReturnCallback(function () use (&$attempts): bool {
+				$attempts++;
+				if ($attempts < 3) {
+					throw new LockedException('files/foo.txt');
+				}
+				return true;
+			});
+
+		$dispatcherMock = $this->createMock(IEventDispatcher::class);
+		$dispatcherMock->expects($this->once())
+			->method('dispatchTyped')
+			->with($this->isInstanceOf(VersionRestoredEvent::class));
+
+		// Tiny retry budget/backoff so the test exercises the retry without waiting.
+		$manager = new VersionManager($dispatcherMock, 1000, 1);
+
+		$this->assertTrue($manager->rollback($versionMock));
+		$this->assertSame(3, $attempts);
+	}
+
+	public function testRollbackGivesUpAfterRetryBudgetExhausted(): void {
+		$versionMock = $this->createMock(IVersion::class);
+		$backendMock = $this->createMock(IVersionBackend::class);
+		$versionMock->method('getBackend')->willReturn($backendMock);
+
+		// The live file stays locked for the whole retry budget.
+		$backendMock->method('rollback')
+			->with($versionMock)
+			->willThrowException(new LockedException('files/foo.txt'));
+
+		$dispatcherMock = $this->createMock(IEventDispatcher::class);
+		$dispatcherMock->expects($this->never())->method('dispatchTyped');
+
+		$manager = new VersionManager($dispatcherMock, 5, 1);
+
+		$this->expectException(LockedException::class);
+		$manager->rollback($versionMock);
+	}
 }