Merge pull request #61863 from nextcloud/fix/release-lock-failed-upload

fix(dav): release part-file lock when an upload fails

Author: CarlSchwan

apps/dav/lib/Connector/Sabre/Directory.php

@@ -125,11 +125,16 @@ public function createFile($name, $data = null) {
 			$node->acquireLock(ILockingProvider::LOCK_SHARED);
 			$this->fileView->lockFile($this->path . '/' . $name . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
 
-			$result = $node->put($data);
-
-			$this->fileView->unlockFile($this->path . '/' . $name . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
-			$node->releaseLock(ILockingProvider::LOCK_SHARED);
-			return $result;
+			try {
+				return $node->put($data);
+			} finally {
+				// Always release the locks, even when the upload failed or was
+				// interrupted. Otherwise a failed attempt leaves the exclusive
+				// part-file lock behind and every later upload to the same path
+				// keeps getting rejected with 423 until the lock TTL expires.
+				$this->fileView->unlockFile($this->path . '/' . $name . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
+				$node->releaseLock(ILockingProvider::LOCK_SHARED);
+			}
 		} catch (StorageNotAvailableException $e) {
 			throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage(), $e->getCode(), $e);
 		} catch (InvalidPathException $ex) {

apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php

@@ -25,6 +25,7 @@
 use OCP\Files\Mount\IMountPoint;
 use OCP\Files\Storage\IStorage;
 use OCP\Files\StorageNotAvailableException;
+use OCP\Lock\ILockingProvider;
 use PHPUnit\Framework\MockObject\MockObject;
 use Sabre\DAV\Exception\NotFound;
 use Test\Traits\UserTrait;
@@ -173,6 +174,47 @@ public function testDeleteFolderThrowsWhenDeletionFailed(): void {
 		$dir->delete();
 	}
 
+	/**
+	 * A failed or interrupted upload must not leave the exclusive part-file
+	 * lock behind. Otherwise every later upload to the same path keeps getting
+	 * rejected with "423 Locked" until the lock TTL expires (up to an hour),
+	 * createFile() therefore releases the locks in a finally block.
+	 */
+	public function testCreateFileReleasesPartFileLockOnFailure(): void {
+		$name = 'foo.txt';
+
+		$this->view->method('getRelativePath')->willReturnArgument(0);
+		$this->view->method('getAbsolutePath')->willReturnArgument(0);
+		$this->view->method('isCreatable')->willReturn(true);
+		// the target does not exist yet
+		$this->view->method('getFileInfo')->willReturn(false);
+		// make File::put() fail right after the locks have been acquired
+		$this->view->method('resolvePath')->willReturn([null, null]);
+
+		$released = [];
+		$this->view->method('unlockFile')
+			->willReturnCallback(function (string $path, int $type) use (&$released): bool {
+				$released[] = [$path, $type];
+				return true;
+			});
+
+		$dir = new Directory($this->view, $this->info);
+		$partLockPath = $dir->getPath() . '/' . $name . '.upload.part';
+
+		try {
+			$dir->createFile($name, 'test data');
+			$this->fail('Expected the failing upload to throw');
+		} catch (\Sabre\DAV\Exception\ServiceUnavailable) {
+			// expected: File::put() cannot resolve the storage
+		}
+
+		$this->assertContains(
+			[$partLockPath, ILockingProvider::LOCK_EXCLUSIVE],
+			$released,
+			'The exclusive .upload.part lock must be released after a failed upload',
+		);
+	}
+
 	public function testGetChildren(): void {
 		$info1 = $this->createMock(FileInfo::class);
 		$info2 = $this->createMock(FileInfo::class);