From eedd8dce3ca68b4492cf46d02575f1ef3497a485 Mon Sep 17 00:00:00 2001 From: Hoang Pham Date: Tue, 6 Jan 2026 18:40:21 +0700 Subject: [PATCH] fix(trashbin): keep metadata consistent on move Signed-off-by: Hoang Pham --- apps/files_trashbin/lib/Trashbin.php | 134 ++++++++++++++++----------- 1 file changed, 78 insertions(+), 56 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 54f8a44b96b..6cc88cb0410 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -299,12 +299,60 @@ class Trashbin implements IEventListener { $configuredTrashbinSize = static::getConfiguredTrashbinSize($owner); if ($configuredTrashbinSize >= 0 && $sourceInfo->getSize() >= $configuredTrashbinSize) { + $trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); return false; } - try { - $moveSuccessful = true; + // there is still a possibility that the file has been deleted by a remote user + $deletedBy = self::overwriteDeletedBy($user); + $deleteTrashRow = static function () use ($owner, $filename, $timestamp): void { + $query = Server::get(IDBConnection::class)->getQueryBuilder(); + $query->delete('files_trash') + ->where($query->expr()->eq('user', $query->createNamedParameter($owner))) + ->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename))) + ->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp))); + $query->executeStatement(); + }; + + $query = Server::get(IDBConnection::class)->getQueryBuilder(); + $query->insert('files_trash') + ->setValue('id', $query->createNamedParameter($filename)) + ->setValue('timestamp', $query->createNamedParameter($timestamp)) + ->setValue('location', $query->createNamedParameter($location)) + ->setValue('user', $query->createNamedParameter($owner)) + ->setValue('deleted_by', $query->createNamedParameter($deletedBy)); + $inserted = false; + try { + $inserted = ($query->executeStatement() === 1); + } catch (\Throwable $e) { + Server::get(LoggerInterface::class)->error( + 'trash bin database insert failed', + [ + 'app' => 'files_trashbin', + 'exception' => $e, + 'user' => $owner, + 'filename' => $filename, + 'timestamp' => $timestamp, + ] + ); + } + if (!$inserted) { + Server::get(LoggerInterface::class)->error( + 'trash bin database couldn\'t be updated, skipping trash move', + [ + 'app' => 'files_trashbin', + 'user' => $owner, + 'filename' => $filename, + 'timestamp' => $timestamp, + ] + ); + $trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + return false; + } + + $moveSuccessful = true; + try { $inCache = $sourceStorage->getCache()->inCache($sourceInternalPath); $trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); if ($inCache) { @@ -333,65 +381,39 @@ class Trashbin implements IEventListener { } else { $trashStorage->getUpdater()->remove($trashInternalPath); } - return false; + $moveSuccessful = false; + } + + if (!$moveSuccessful) { + Server::get(LoggerInterface::class)->error( + 'trash move failed, removing trash metadata and payload', + [ + 'app' => 'files_trashbin', + 'user' => $owner, + 'filename' => $filename, + 'timestamp' => $timestamp, + ] + ); + $deleteTrashRow(); + if ($trashStorage->file_exists($trashInternalPath)) { + if ($trashStorage->is_dir($trashInternalPath)) { + $trashStorage->rmdir($trashInternalPath); + } else { + $trashStorage->unlink($trashInternalPath); + } + } + $trashStorage->getUpdater()->remove($trashInternalPath); } if ($moveSuccessful) { - // there is still a possibility that the file has been deleted by a remote user - $deletedBy = self::overwriteDeletedBy($user); + Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path), + 'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]); - $query = Server::get(IDBConnection::class)->getQueryBuilder(); - $query->insert('files_trash') - ->setValue('id', $query->createNamedParameter($filename)) - ->setValue('timestamp', $query->createNamedParameter($timestamp)) - ->setValue('location', $query->createNamedParameter($location)) - ->setValue('user', $query->createNamedParameter($owner)) - ->setValue('deleted_by', $query->createNamedParameter($deletedBy)); - $inserted = false; - try { - $inserted = ($query->executeStatement() === 1); - } catch (\Throwable $e) { - Server::get(LoggerInterface::class)->error( - 'trash bin database insert failed', - [ - 'app' => 'files_trashbin', - 'exception' => $e, - 'user' => $owner, - 'filename' => $filename, - 'timestamp' => $timestamp, - ] - ); - } - if (!$inserted) { - Server::get(LoggerInterface::class)->error( - 'trash bin database couldn\'t be updated, removing trash payload', - [ - 'app' => 'files_trashbin', - 'user' => $owner, - 'filename' => $filename, - 'timestamp' => $timestamp, - ] - ); - if ($trashStorage->file_exists($trashInternalPath)) { - if ($trashStorage->is_dir($trashInternalPath)) { - $trashStorage->rmdir($trashInternalPath); - } else { - $trashStorage->unlink($trashInternalPath); - } - } - $trashStorage->getUpdater()->remove($trashInternalPath); - $moveSuccessful = false; - } - if ($inserted) { - Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path), - 'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]); + self::retainVersions($filename, $owner, $ownerPath, $timestamp); - self::retainVersions($filename, $owner, $ownerPath, $timestamp); - - // if owner !== user we need to also add a copy to the users trash - if ($user !== $owner && $ownerOnly === false) { - self::copyFilesToUser($ownerPath, $owner, $file_path, $user, $timestamp); - } + // if owner !== user we need to also add a copy to the users trash + if ($user !== $owner && $ownerOnly === false) { + self::copyFilesToUser($ownerPath, $owner, $file_path, $user, $timestamp); } }