From b101139f0b564b9d1330804807bfe2e23e7d97a3 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 10 Aug 2026 13:35:59 +0100 Subject: [PATCH] fix(dav): refuse to delete an upload session while its chunks are being assembled The final MOVE of a chunked upload streams the chunks into the destination one after another, opening each only when it gets there. A client that gives up on the upload - the desktop client does this when the assembly MOVE answers with an ambiguous 502 - deletes the upload session, which pulls the remaining chunks out from under the assembly that is still reading them. The assembly then fails on a chunk that no longer exists, and the upload has to be retried from the start. Hold an exclusive lock for the duration of the assembly and take the same lock before deleting a session, so a delete that arrives mid-assembly is answered with 423 instead of destroying the chunks. The lock sits on a path with nothing behind it, the way createFile() guards its part file, so it does not collide with the deletion of the session that ends a successful move. The lock is held through the file locking provider rather than a distributed cache, so it also works on instances that have no memcache configured. Two paths remain outside it by design: the background upload cleanup job deletes sessions through the filesystem API, but only ever expires sessions whose files are all older than the TTL, and the lock itself expires with the locking provider's TTL, which bounds how long a crashed assembly keeps its session undeletable. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- apps/dav/lib/Upload/ChunkingPlugin.php | 10 +- apps/dav/lib/Upload/FutureFile.php | 55 +++++++++ apps/dav/lib/Upload/UploadFolder.php | 14 ++- .../tests/unit/Upload/ChunkingPluginTest.php | 88 ++++++++++++++ .../tests/unit/Upload/UploadFolderTest.php | 108 ++++++++++++++++++ 5 files changed, 271 insertions(+), 4 deletions(-) create mode 100644 apps/dav/tests/unit/Upload/UploadFolderTest.php diff --git a/apps/dav/lib/Upload/ChunkingPlugin.php b/apps/dav/lib/Upload/ChunkingPlugin.php index 0f71f3437f0fb..e7249e74a3f03 100644 --- a/apps/dav/lib/Upload/ChunkingPlugin.php +++ b/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(); + } } /** diff --git a/apps/dav/lib/Upload/FutureFile.php b/apps/dav/lib/Upload/FutureFile.php index 034b757d8f64b..33c0da88761e5 100644 --- a/apps/dav/lib/Upload/FutureFile.php +++ b/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 */ diff --git a/apps/dav/lib/Upload/UploadFolder.php b/apps/dav/lib/Upload/UploadFolder.php index 74512640a8f64..127aabe88866c 100644 --- a/apps/dav/lib/Upload/UploadFolder.php +++ b/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] diff --git a/apps/dav/tests/unit/Upload/ChunkingPluginTest.php b/apps/dav/tests/unit/Upload/ChunkingPluginTest.php index 62f2d5e9c98c1..00282dc47127d 100644 --- a/apps/dav/tests/unit/Upload/ChunkingPluginTest.php +++ b/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'); + } } diff --git a/apps/dav/tests/unit/Upload/UploadFolderTest.php b/apps/dav/tests/unit/Upload/UploadFolderTest.php new file mode 100644 index 0000000000000..2d13d8d403fe4 --- /dev/null +++ b/apps/dav/tests/unit/Upload/UploadFolderTest.php @@ -0,0 +1,108 @@ +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')); + } +}