fix(dav): keep upload session while its chunks are being assembled

apps/dav/lib/Upload/ChunkingPlugin.php

@@ -58,7 +58,15 @@ public function beforeMove($sourcePath, $destination) {
 		}
 
 		$this->verifySize();
-		return $this->performMove($sourcePath, $destination);
+
+		// hold the assembly lock for the whole move, so the upload session cannot
+		// be deleted while its chunks are still being streamed to the destination
+		$this->sourceNode->lockAssembly();
+		try {
+			return $this->performMove($sourcePath, $destination);
+		} finally {
+			$this->sourceNode->unlockAssembly();
+		}
 	}
 
 	/**

apps/dav/lib/Upload/FutureFile.php

@@ -9,6 +9,11 @@
 namespace OCA\DAV\Upload;
 
 use OCA\DAV\Connector\Sabre\Directory;
+use OCA\DAV\Connector\Sabre\Exception\FileLocked;
+use OCP\Files\Storage\ILockingStorage;
+use OCP\Lock\ILockingProvider;
+use OCP\Lock\LockedException;
+use OCP\Server;
 use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\IFile;
 
@@ -21,6 +26,13 @@
  * @package OCA\DAV\Upload
  */
 class FutureFile implements \Sabre\DAV\IFile {
+	/**
+	 * Suffix of the path locked while the chunks are being assembled. Nothing is
+	 * stored under it, so no ordinary file operation contends for it - the same
+	 * trick Directory::createFile() uses for its .upload.part lock.
+	 */
+	private const ASSEMBLY_LOCK_SUFFIX = '.assembly';
+
 	/**
 	 * @param Directory $root
 	 * @param string $name
@@ -31,6 +43,49 @@ public function __construct(
 	) {
 	}
 
+	/**
+	 * Mark an assembly of these chunks as in progress, so that the upload session
+	 * cannot be deleted while they are still being read.
+	 *
+	 * @throws FileLocked if the chunks are already being assembled
+	 */
+	public function lockAssembly(): void {
+		try {
+			$this->assemblyLock(true);
+		} catch (LockedException $e) {
+			throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+		}
+	}
+
+	public function unlockAssembly(): void {
+		try {
+			$this->assemblyLock(false);
+		} catch (LockedException $e) {
+			// releasing a lock this request holds should not fail, and there is
+			// nothing left to do about it if it does
+		}
+	}
+
+	/**
+	 * @throws LockedException
+	 */
+	private function assemblyLock(bool $acquire): void {
+		$info = $this->root->getFileInfo();
+		$storage = $info->getStorage();
+		if (!$storage->instanceOfStorage(ILockingStorage::class)) {
+			return;
+		}
+
+		/** @var ILockingStorage $storage */
+		$path = $info->getInternalPath() . self::ASSEMBLY_LOCK_SUFFIX;
+		$provider = Server::get(ILockingProvider::class);
+		if ($acquire) {
+			$storage->acquireLock($path, ILockingProvider::LOCK_EXCLUSIVE, $provider);
+		} else {
+			$storage->releaseLock($path, ILockingProvider::LOCK_EXCLUSIVE, $provider);
+		}
+	}
+
 	/**
 	 * @inheritdoc
 	 */

apps/dav/lib/Upload/UploadFolder.php

@@ -94,10 +94,18 @@ public function childExists($name) {
 
 	#[\Override]
 	public function delete() {
-		$this->node->delete();
+		// refuse while an assembly is still reading these chunks, otherwise it ends
+		// up streaming from files that are being removed underneath it
+		$futureFile = new FutureFile($this->node, '.file');
+		$futureFile->lockAssembly();
+		try {
+			$this->node->delete();
 
-		// Background cleanup job is not needed anymore
-		$this->cleanupService->removeJob($this->uid, $this->getName());
+			// Background cleanup job is not needed anymore
+			$this->cleanupService->removeJob($this->uid, $this->getName());
+		} finally {
+			$futureFile->unlockAssembly();
+		}
 	}
 
 	#[\Override]

apps/dav/tests/unit/Upload/ChunkingPluginTest.php

@@ -186,4 +186,92 @@ public function testBeforeMoveSizeIsWrong(): void {
 
 		$this->assertFalse($this->plugin->beforeMove('source', 'target'));
 	}
+
+	public function testBeforeMoveHoldsTheAssemblyLockAroundTheMove(): void {
+		$sourceNode = $this->createMock(FutureFile::class);
+		$sourceNode->expects($this->once())
+			->method('getSize')
+			->willReturn(4);
+		$sourceNode->expects($this->once())
+			->method('lockAssembly');
+		$sourceNode->expects($this->once())
+			->method('unlockAssembly');
+
+		$calls = [
+			['source', $sourceNode],
+			['target', new NotFound()],
+		];
+		$this->tree->expects($this->exactly(2))
+			->method('getNodeForPath')
+			->willReturnCallback(function (string $path) use (&$calls) {
+				$expected = array_shift($calls);
+				$this->assertSame($expected[0], $path);
+				if ($expected[1] instanceof \Throwable) {
+					throw $expected[1];
+				}
+				return $expected[1];
+			});
+		$this->tree->expects($this->any())
+			->method('nodeExists')
+			->with('target')
+			->willReturn(false);
+		$this->tree->expects($this->once())
+			->method('move')
+			->with('source', 'target');
+		$this->response->expects($this->once())
+			->method('setStatus')
+			->with(201);
+		$this->request->expects($this->once())
+			->method('getHeader')
+			->with('OC-Total-Length')
+			->willReturn('4');
+
+		$this->assertFalse($this->plugin->beforeMove('source', 'target'));
+	}
+
+	/**
+	 * The upload session must become deletable again when the move fails, or a
+	 * failed upload leaves a session nobody can clean up until the lock expires.
+	 */
+	public function testBeforeMoveReleasesTheAssemblyLockWhenTheMoveFails(): void {
+		$sourceNode = $this->createMock(FutureFile::class);
+		$sourceNode->expects($this->once())
+			->method('getSize')
+			->willReturn(4);
+		$sourceNode->expects($this->once())
+			->method('lockAssembly');
+		$sourceNode->expects($this->once())
+			->method('unlockAssembly');
+
+		$calls = [
+			['source', $sourceNode],
+			['target', new NotFound()],
+		];
+		$this->tree->expects($this->exactly(2))
+			->method('getNodeForPath')
+			->willReturnCallback(function (string $path) use (&$calls) {
+				$expected = array_shift($calls);
+				$this->assertSame($expected[0], $path);
+				if ($expected[1] instanceof \Throwable) {
+					throw $expected[1];
+				}
+				return $expected[1];
+			});
+		$this->tree->expects($this->any())
+			->method('nodeExists')
+			->with('target')
+			->willReturn(false);
+		$this->tree->expects($this->once())
+			->method('move')
+			->willThrowException(new \Sabre\DAV\Exception('the move failed'));
+		$this->response->expects($this->never())
+			->method('setStatus');
+		$this->request->expects($this->once())
+			->method('getHeader')
+			->with('OC-Total-Length')
+			->willReturn('4');
+
+		$this->expectException(\Sabre\DAV\Exception::class);
+		$this->plugin->beforeMove('source', 'target');
+	}
 }

apps/dav/tests/unit/Upload/UploadFolderTest.php

@@ -0,0 +1,108 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\DAV\Tests\unit\Upload;
+
+use OC\Files\View;
+use OCA\DAV\Connector\Sabre\Directory;
+use OCA\DAV\Connector\Sabre\Exception\FileLocked;
+use OCA\DAV\Upload\CleanupService;
+use OCA\DAV\Upload\FutureFile;
+use OCA\DAV\Upload\UploadFolder;
+use OCP\Files\IRootFolder;
+use OCP\IUserManager;
+use OCP\Server;
+use Test\TestCase;
+use Test\Traits\UserTrait;
+
+/**
+ * The desktop client deletes an upload session when it believes the assembly
+ * failed - on an ambiguous 502, for instance. If the assembly is in fact still
+ * running, that pulls the chunks out from under it while it reads them.
+ */
+#[\PHPUnit\Framework\Attributes\Group(name: 'DB')]
+class UploadFolderTest extends TestCase {
+	use UserTrait;
+
+	private string $user;
+	private View $uploadsView;
+	private Directory $sessionNode;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->user = self::getUniqueID('upload_folder_');
+		$this->createUser($this->user, 'pass');
+		self::loginAsUser($this->user);
+		Server::get(IRootFolder::class)->getUserFolder($this->user);
+
+		$userView = new View('/' . $this->user);
+		if (!$userView->file_exists('uploads')) {
+			$userView->mkdir('uploads');
+		}
+
+		$this->uploadsView = new View('/' . $this->user . '/uploads');
+		$this->uploadsView->mkdir('session-1');
+		$this->uploadsView->file_put_contents('session-1/00001', 'chunk data');
+
+		$this->sessionNode = new Directory(
+			$this->uploadsView,
+			$this->uploadsView->getFileInfo('session-1')
+		);
+	}
+
+	protected function tearDown(): void {
+		Server::get(IUserManager::class)->get($this->user)?->delete();
+		parent::tearDown();
+	}
+
+	private function buildUploadFolder(): UploadFolder {
+		return new UploadFolder(
+			$this->sessionNode,
+			Server::get(CleanupService::class),
+			$this->uploadsView->getFileInfo('session-1')->getStorage(),
+			$this->user,
+		);
+	}
+
+	public function testDeleteIsRefusedWhileTheChunksAreBeingAssembled(): void {
+		// a MOVE running in another request holds this while it streams the chunks
+		$assembling = new FutureFile($this->sessionNode, '.file');
+		$assembling->lockAssembly();
+
+		try {
+			$this->buildUploadFolder()->delete();
+			$this->fail('Expected the delete to be refused while assembling');
+		} catch (FileLocked $e) {
+			// expected
+		}
+
+		$this->assertTrue(
+			$this->uploadsView->file_exists('session-1/00001'),
+			'The chunks the assembly is reading must still be there'
+		);
+
+		$assembling->unlockAssembly();
+	}
+
+	public function testDeleteWorksOnceTheAssemblyReleasedTheChunks(): void {
+		$assembling = new FutureFile($this->sessionNode, '.file');
+		$assembling->lockAssembly();
+		$assembling->unlockAssembly();
+
+		$this->buildUploadFolder()->delete();
+
+		$this->assertFalse($this->uploadsView->file_exists('session-1'));
+	}
+
+	public function testDeleteWorksWhenNoAssemblyIsRunning(): void {
+		$this->buildUploadFolder()->delete();
+
+		$this->assertFalse($this->uploadsView->file_exists('session-1'));
+	}
+}