fix(view): make removeMount locking cleanup exception-safe

lib/private/Files/View.php

@@ -241,31 +241,63 @@ public function mkdir($path) {
 	 * @param string $path relative to data/
 	 */
 	protected function removeMount($mount, $path): bool {
-		if ($mount instanceof IMovableMount) {
-			// cut of /user/files to get the relative path to data/user/files
-			$pathParts = explode('/', $path, 4);
-			$relPath = '/' . $pathParts[3];
-			$this->lockFile($relPath, ILockingProvider::LOCK_SHARED, true);
+		if (!$mount instanceof IMovableMount) {
+			// do not allow deleting the storage's root / the mount point
+			// because for some storages it might delete the whole contents
+			// but isn't supposed to work that way
+			return false;
+		}
+
+		// cut of /user/files to get the relative path to data/user/files
+		$pathParts = explode('/', $path, 4);
+		$relPath = '/' . $pathParts[3];
+
+		$this->lockFile($relPath, ILockingProvider::LOCK_SHARED, true);
+		$lockType = ILockingProvider::LOCK_SHARED;
+
+		$operationException = null;
+
+		try {
 			\OC_Hook::emit(
 				Filesystem::CLASSNAME, 'umount',
-				[Filesystem::signal_param_path => $relPath]
+				[Filesystem::signal_param_path => $relPath],
 			);
+
 			$this->changeLock($relPath, ILockingProvider::LOCK_EXCLUSIVE, true);
+			$lockType = ILockingProvider::LOCK_EXCLUSIVE;
+
 			$result = $mount->removeMount();
+
 			$this->changeLock($relPath, ILockingProvider::LOCK_SHARED, true);
+			$lockType = ILockingProvider::LOCK_SHARED;
+
 			if ($result) {
 				\OC_Hook::emit(
 					Filesystem::CLASSNAME, 'post_umount',
 					[Filesystem::signal_param_path => $relPath]
 				);
 			}
-			$this->unlockFile($relPath, ILockingProvider::LOCK_SHARED, true);
 			return $result;
-		} else {
-			// do not allow deleting the storage's root / the mount point
-			// because for some storages it might delete the whole contents
-			// but isn't supposed to work that way
-			return false;
+		} catch (\Throwable $e) {
+			$operationException = $e;
+			throw $e;
+		} finally {
+			try {
+				$this->unlockFile($relPath, $lockType, true);
+			} catch (\Throwable $unlockException) {
+				if ($operationException === null) {
+					throw $unlockException;
+				}
+
+				$this->logger->error(
+					'Failed to release mount-point lock while handling a mount removal failure',
+					[
+						'app' => 'core',
+						'exception' => $unlockException,
+						'operationException' => $operationException,
+					]
+				);
+			}
 		}
 	}
 

tests/lib/Files/ViewTest.php

@@ -2644,6 +2644,43 @@ public function testRemoveMoveableMountPoint(): void {
 		$this->assertEquals('foo', $view->rmdir('mount'));
 	}
 
+	public function testRemoveMovableMountPointUnlocksAfterException(): void {
+		$mountPoint = '/' . self::$user . '/files/mount/';
+		$view = new View('/' . self::$user . '/files');
+
+		/** @var TestMoveableMountPoint&MockObject $mount */
+		$mount = $this->createMock(TestMoveableMountPoint::class);
+		$mount->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mount->method('getInternalPath')
+			->willReturn('');
+		$mount->expects($this->once())
+			->method('removeMount')
+			->willReturnCallback(function () use ($view): never {
+				$this->assertSame(
+					ILockingProvider::LOCK_EXCLUSIVE,
+					$this->getFileLockType($view, 'mount', true),
+					'The mount point must be exclusively locked during removal'
+				);
+
+				throw new \RuntimeException('Simulated mount removal failure');
+			});
+
+		Filesystem::getMountManager()->addMount($mount);
+
+		try {
+			$view->rmdir('mount');
+			$this->fail('Expected the mount removal exception to be rethrown');
+		} catch (\RuntimeException $e) {
+			$this->assertSame('Simulated mount removal failure', $e->getMessage());
+		}
+
+		$this->assertNull(
+			$this->getFileLockType($view, 'mount', true),
+			'The mount-point lock must be released after a failed mount removal'
+		);
+	}
+
 	public static function mimeFilterProvider(): array {
 		return [
 			[null, ['test1.txt', 'test2.txt', 'test3.md', 'test4.png']],