From 14ef05be7ee63dd3530ea5288dd845dbb8dadb95 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:00:39 +0200 Subject: [PATCH 1/2] fix: Clean up trash items whose file is already gone from storage Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Trash/TrashBackend.php | 16 +++++++++-- tests/Trash/TrashBackendTest.php | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/lib/Trash/TrashBackend.php b/lib/Trash/TrashBackend.php index 0d0ac1ae8..5bae78243 100644 --- a/lib/Trash/TrashBackend.php +++ b/lib/Trash/TrashBackend.php @@ -222,6 +222,18 @@ private function unwrapJails(IStorage $storage, string $internalPath): array { return [$unJailedStorage, $unJailedInternalPath]; } + /** + * An item that is already gone from storage is not a failure to delete: its cache + * entry and trash record still have to be cleaned up, otherwise an interrupted + * delete leaves the item stuck in the trashbin forever. + */ + private function unlinkTrashNode(Node $node): bool { + $storage = $node->getStorage(); + $internalPath = $node->getInternalPath(); + + return $storage->unlink($internalPath) !== false || !$storage->file_exists($internalPath); + } + /** * @throws \LogicException * @throws \Exception @@ -247,7 +259,7 @@ public function removeItem(ITrashItem $item): void { throw new NotPermittedException(); } - if ($node->getStorage()->unlink($node->getInternalPath()) === false) { + if (!$this->unlinkTrashNode($node)) { throw new \Exception('Failed to remove item from trashbin'); } @@ -656,7 +668,7 @@ public function expire(Expiration $expiration): array { if ($expiration->isExpired($groupTrashItem['deleted_time'], $folder->quota > 0 && $folder->quota < ($size + $sizeInTrash))) { $this->logger->debug('expiring ' . $node->getPath()); - if ($node->getStorage()->unlink($node->getInternalPath()) === false) { + if (!$this->unlinkTrashNode($node)) { $this->logger->error('Failed to remove item from trashbin: ' . $node->getPath()); continue; } diff --git a/tests/Trash/TrashBackendTest.php b/tests/Trash/TrashBackendTest.php index dd5eb9401..17f58ccbe 100644 --- a/tests/Trash/TrashBackendTest.php +++ b/tests/Trash/TrashBackendTest.php @@ -11,6 +11,7 @@ use OC\Files\SetupManager; use OC\Group\Database; +use OCA\Files_Trashbin\Expiration; use OCA\Files_Trashbin\Trash\ITrashItem; use OCA\GroupFolders\ACL\Rule; use OCA\GroupFolders\ACL\RuleManager; @@ -412,4 +413,50 @@ public function testRepairMisplacedTrashItems(): void { $this->logout(); } + + public function testExpireWithMissingTrashFile(): void { + $this->loginAsUser('manager'); + + $file = $this->managerUserFolder->newFile("{$this->folderName}/gone.txt", 'content'); + $this->trashBackend->moveToTrash($file->getStorage(), $file->getInternalPath()); + + /** @var list $items */ + $items = $this->trashBackend->listTrashRoot($this->managerUser); + $this->assertCount(1, $items); + + // Simulate an expiry run that was interrupted after the unlink but before the + // cache entry and the trash record were cleaned up + $node = $items[0]->getTrashNode(); + $this->assertTrue($node->getStorage()->unlink($node->getInternalPath())); + + $expiration = $this->createMock(Expiration::class); + $expiration->method('isExpired')->willReturn(true); + $this->trashBackend->expire($expiration); + + $this->assertCount(0, $this->trashManager->listTrashForFolders([$this->folderId])); + $this->assertCount(0, $this->trashBackend->listTrashRoot($this->managerUser)); + + $this->logout(); + } + + public function testRemoveItemWithMissingTrashFile(): void { + $this->loginAsUser('manager'); + + $file = $this->managerUserFolder->newFile("{$this->folderName}/gone.txt", 'content'); + $this->trashBackend->moveToTrash($file->getStorage(), $file->getInternalPath()); + + /** @var list $items */ + $items = $this->trashBackend->listTrashRoot($this->managerUser); + $this->assertCount(1, $items); + + $node = $items[0]->getTrashNode(); + $this->assertTrue($node->getStorage()->unlink($node->getInternalPath())); + + $this->trashBackend->removeItem($items[0]); + + $this->assertCount(0, $this->trashManager->listTrashForFolders([$this->folderId])); + $this->assertCount(0, $this->trashBackend->listTrashRoot($this->managerUser)); + + $this->logout(); + } } From 7702620182e386b8269f1ebfa1e183235a2edf2a Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:13:19 +0200 Subject: [PATCH 2/2] fix(tests): reproduce the missing trash file state on object stores too Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- tests/Trash/TrashBackendTest.php | 38 +++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/tests/Trash/TrashBackendTest.php b/tests/Trash/TrashBackendTest.php index 17f58ccbe..4ab316b7b 100644 --- a/tests/Trash/TrashBackendTest.php +++ b/tests/Trash/TrashBackendTest.php @@ -24,8 +24,10 @@ use OCA\GroupFolders\Trash\TrashManager; use OCP\Constants; use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\Files\Cache\ICacheEntry; use OCP\Files\Folder; use OCP\Files\IRootFolder; +use OCP\Files\Node; use OCP\IDBConnection; use OCP\IUser; use OCP\Server; @@ -414,6 +416,34 @@ public function testRepairMisplacedTrashItems(): void { $this->logout(); } + /** + * Leave the item in the state an interrupted delete produces: the file is gone from + * storage while its cache entry is still there. Object stores drop the cache entry as + * part of unlink(), so it gets restored to reach the same state on every backend. + */ + private function removeTrashFileKeepingCacheEntry(Node $node): void { + $storage = $node->getStorage(); + $internalPath = $node->getInternalPath(); + $cache = $storage->getCache(); + + $entry = $cache->get($internalPath); + $this->assertInstanceOf(ICacheEntry::class, $entry); + $this->assertTrue($storage->unlink($internalPath)); + + if (!$cache->inCache($internalPath)) { + $cache->put($internalPath, [ + 'size' => $entry->getSize(), + 'mtime' => $entry->getMTime(), + 'storage_mtime' => $entry->getStorageMTime(), + 'mimetype' => $entry->getMimeType(), + 'etag' => $entry->getEtag(), + 'permissions' => $entry->getPermissions(), + ]); + } + + $this->assertTrue($cache->inCache($internalPath)); + } + public function testExpireWithMissingTrashFile(): void { $this->loginAsUser('manager'); @@ -424,10 +454,7 @@ public function testExpireWithMissingTrashFile(): void { $items = $this->trashBackend->listTrashRoot($this->managerUser); $this->assertCount(1, $items); - // Simulate an expiry run that was interrupted after the unlink but before the - // cache entry and the trash record were cleaned up - $node = $items[0]->getTrashNode(); - $this->assertTrue($node->getStorage()->unlink($node->getInternalPath())); + $this->removeTrashFileKeepingCacheEntry($items[0]->getTrashNode()); $expiration = $this->createMock(Expiration::class); $expiration->method('isExpired')->willReturn(true); @@ -449,8 +476,7 @@ public function testRemoveItemWithMissingTrashFile(): void { $items = $this->trashBackend->listTrashRoot($this->managerUser); $this->assertCount(1, $items); - $node = $items[0]->getTrashNode(); - $this->assertTrue($node->getStorage()->unlink($node->getInternalPath())); + $this->removeTrashFileKeepingCacheEntry($items[0]->getTrashNode()); $this->trashBackend->removeItem($items[0]);