fix(files_sharing): roll back shared storage locks on owner lock failure

apps/files_sharing/lib/SharedStorage.php

@@ -493,23 +493,54 @@ public function acquireLock(string $path, int $type, ILockingProvider $provider)
 		/** @var ILockingStorage $targetStorage */
 		[$targetStorage, $targetInternalPath] = $this->resolvePath($path);
 		$targetStorage->acquireLock($targetInternalPath, $type, $provider);
-		// lock the parent folders of the owner when locking the share as recipient
+
+		// Lock the owner parent folders when locking the share root as recipient.
 		if ($path === '') {
 			$sourcePath = $this->ownerUserFolder->getRelativePath($this->sourcePath);
-			$this->ownerView->lockFile(dirname($sourcePath), ILockingProvider::LOCK_SHARED, true);
+			$ownerParentPath = dirname($sourcePath);
+
+			try {
+				$this->ownerView->lockFile($ownerParentPath, ILockingProvider::LOCK_SHARED, true);
+			} catch (\Throwable $e) {
+				// Do not leave the source-node lock behind if locking its parent fails.
+				try {
+					$targetStorage->releaseLock($targetInternalPath, $type, $provider);
+				} catch (\Throwable $releaseException) {
+					$this->logger->error(
+						'Failed to roll back share lock after locking the owner parent failed',
+						[
+							'app' => 'files_sharing',
+							'exception' => $releaseException,
+							'shareId' => $this->getShareId(),
+						]
+					);
+				}
+
+				throw $e;
+			}
 		}
 	}
 
 	#[\Override]
 	public function releaseLock(string $path, int $type, ILockingProvider $provider): void {
 		/** @var ILockingStorage $targetStorage */
 		[$targetStorage, $targetInternalPath] = $this->resolvePath($path);
-		$targetStorage->releaseLock($targetInternalPath, $type, $provider);
-		// unlock the parent folders of the owner when unlocking the share as recipient
+
 		if ($path === '') {
 			$sourcePath = $this->ownerUserFolder->getRelativePath($this->sourcePath);
-			$this->ownerView->unlockFile(dirname($sourcePath), ILockingProvider::LOCK_SHARED, true);
+			$ownerParentPath = dirname($sourcePath);
+
+			// Release in reverse acquisition order. Always release the target lock,
+			// even if unlocking the owner parent fails.
+			try {
+				$this->ownerView->unlockFile($ownerParentPath, ILockingProvider::LOCK_SHARED, true);
+			} finally {
+				$targetStorage->releaseLock($targetInternalPath, $type, $provider);
+			}
+			return;
 		}
+
+		$targetStorage->releaseLock($targetInternalPath, $type, $provider);
 	}
 
 	#[\Override]

apps/files_sharing/tests/SharedStorageTest.php

@@ -18,8 +18,13 @@
 use OCP\AppFramework\Bootstrap\IBootContext;
 use OCP\Constants;
 use OCP\Files\Config\IMountProviderCollection;
+use OCP\Files\Folder;
+use OCP\Files\IRootFolder;
+use OCP\Files\Node;
 use OCP\Files\NotFoundException;
+use OCP\Files\Storage\IStorage;
 use OCP\IUserManager;
+use OCP\Lock\ILockingProvider;
 use OCP\Server;
 use OCP\Share\IShare;
 
@@ -462,11 +467,17 @@ public function testInitWithNonExistingUser(): void {
 	public function testInitWithNotFoundSource(): void {
 		$share = $this->createMock(IShare::class);
 		$share->method('getShareOwner')->willReturn(self::TEST_FILES_SHARING_API_USER1);
-		$share->method('getNodeId')->willReturn(1);
-		$ownerView = $this->createMock(View::class);
-		$ownerView->method('getPath')->willThrowException(new NotFoundException());
+		$share->method('getNodeId')->willReturn(42);
+
+		$rootFolder = $this->createMock(IRootFolder::class);
+		$rootFolder->expects($this->once())
+			->method('getUserFolder')
+			->with(self::TEST_FILES_SHARING_API_USER1)
+			->willReturn($ownerUserFolder);
+
 		$storage = new SharedStorage([
-			'ownerView' => $ownerView,
+			'ownerView' => $this->createMock(View::class),
+			'rootFolder' => $rootFolder,
 			'superShare' => $share,
 			'groupedShares' => [$share],
 			'user' => 'user1',
@@ -475,6 +486,9 @@ public function testInitWithNotFoundSource(): void {
 		// trigger init
 		$this->assertInstanceOf(FailedStorage::class, $storage->getSourceStorage());
 		$this->assertInstanceOf(FailedCache::class, $storage->getCache());
+
+		$this->assertSame(0, $storage->getPermissions());
+		$this->assertFalse($storage->isReadable('missing.txt'));
 	}
 
 	public function testCopyPermissions(): void {
@@ -502,4 +516,131 @@ public function testCopyPermissions(): void {
 		$this->view->unlink($this->filename);
 		$this->shareManager->deleteShare($share);
 	}
+
+	public function testAcquireRootLockRollsBackTargetLockWhenOwnerParentLockFails(): void {
+		$events = [];
+		$provider = $this->createMock(ILockingProvider::class);
+		$ownerView = $this->createMock(View::class);
+		$sourceStorage = $this->createMock(IStorage::class);
+
+		$sourceStorage->expects($this->once())
+			->method('acquireLock')
+			->with('foo/bar.txt', ILockingProvider::LOCK_SHARED, $provider)
+			->willReturnCallback(function () use (&$events): void {
+				$events[] = 'target-acquire';
+			});
+		$sourceStorage->expects($this->once())
+			->method('releaseLock')
+			->with('foo/bar.txt', ILockingProvider::LOCK_SHARED, $provider)
+			->willReturnCallback(function () use (&$events): void {
+				$events[] = 'target-release';
+			});
+
+		$ownerView->expects($this->once())
+			->method('lockFile')
+			->with('foo', ILockingProvider::LOCK_SHARED, true)
+			->willReturnCallback(function () use (&$events): void {
+				$events[] = 'owner-lock';
+				throw new \RuntimeException('Owner parent is locked');
+			});
+
+		$storage = $this->createSharedStorageForRootLocking($ownerView, $sourceStorage);
+
+		try {
+			$storage->acquireLock('', ILockingProvider::LOCK_SHARED, $provider);
+			$this->fail('Expected owner-parent locking to fail');
+		} catch (\RuntimeException $e) {
+			$this->assertSame('Owner parent is locked', $e->getMessage());
+		}
+
+		$this->assertSame([
+			'target-acquire',
+			'owner-lock',
+			'target-release',
+		], $events);
+	}
+
+	public function testReleaseRootLockUnlocksOwnerParentBeforeTarget(): void {
+		$events = [];
+		$provider = $this->createMock(ILockingProvider::class);
+		$ownerView = $this->createMock(View::class);
+		$sourceStorage = $this->createMock(IStorage::class);
+
+		$ownerView->expects($this->once())
+			->method('unlockFile')
+			->with('foo', ILockingProvider::LOCK_SHARED, true)
+			->willReturnCallback(function () use (&$events): void {
+				$events[] = 'owner-release';
+			});
+		$sourceStorage->expects($this->once())
+			->method('releaseLock')
+			->with('foo/bar.txt', ILockingProvider::LOCK_SHARED, $provider)
+			->willReturnCallback(function () use (&$events): void {
+				$events[] = 'target-release';
+			});
+
+		$storage = $this->createSharedStorageForRootLocking($ownerView, $sourceStorage);
+		$storage->releaseLock('', ILockingProvider::LOCK_SHARED, $provider);
+
+		$this->assertSame([
+			'owner-release',
+			'target-release',
+		], $events);
+	}
+
+	public function testReleaseRootLockStillReleasesTargetWhenOwnerParentUnlockFails(): void {
+		$provider = $this->createMock(ILockingProvider::class);
+		$ownerView = $this->createMock(View::class);
+		$sourceStorage = $this->createMock(IStorage::class);
+
+		$ownerView->expects($this->once())
+			->method('unlockFile')
+			->with('foo', ILockingProvider::LOCK_SHARED, true)
+			->willThrowException(new \RuntimeException('Owner parent unlock failed'));
+		$sourceStorage->expects($this->once())
+			->method('releaseLock')
+			->with('foo/bar.txt', ILockingProvider::LOCK_SHARED, $provider);
+
+		$storage = $this->createSharedStorageForRootLocking($ownerView, $sourceStorage);
+
+		$this->expectException(\RuntimeException::class);
+		$this->expectExceptionMessage('Owner parent unlock failed');
+
+		$storage->releaseLock('', ILockingProvider::LOCK_SHARED, $provider);
+	}
+
+	private function createSharedStorageForRootLocking(
+		View $ownerView,
+		IStorage $sourceStorage,
+	): SharedStorage {
+		$share = $this->createMock(IShare::class);
+		$share->method('getShareOwner')->willReturn(self::TEST_FILES_SHARING_API_USER1);
+		$share->method('getNodeId')->willReturn(42);
+		$share->method('getPermissions')->willReturn(Constants::PERMISSION_ALL);
+
+		$sourceNode = $this->createMock(Node::class);
+		$sourceNode->method('getStorage')->willReturn($sourceStorage);
+		$sourceNode->method('getPath')
+		->willReturn('/' . self::TEST_FILES_SHARING_API_USER1 . '/files/foo/bar.txt');
+		$sourceNode->method('getInternalPath')->willReturn('foo/bar.txt');
+
+		$ownerUserFolder = $this->createMock(Folder::class);
+		$ownerUserFolder->method('getById')->with(42)->willReturn([$sourceNode]);
+		$ownerUserFolder->method('getRelativePath')->with(
+			'/' . self::TEST_FILES_SHARING_API_USER1 . '/files/foo/bar.txt'
+		)->willReturn('foo/bar.txt');
+
+		$rootFolder = $this->createMock(IRootFolder::class);
+		$rootFolder->method('getUserFolder')
+			->with(self::TEST_FILES_SHARING_API_USER1)
+			->willReturn($ownerUserFolder);
+
+		return new SharedStorage([
+			'ownerView' => $ownerView,
+			'rootFolder' => $rootFolder,
+			'superShare' => $share,
+			'groupedShares' => [$share],
+			'user' => self::TEST_FILES_SHARING_API_USER2,
+		]);
+	}
 }