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')); + } +}