Skip to content

fix(trashbin): keep cache and db consistent #57360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
hweihwang wants to merge 3 commits into
base: master
Choose a base branch
from
+113 −23
Draft

fix(trashbin): keep cache and db consistent #57360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ecc1a7
fix: keep trashbin cache and db in sync
hweihwang Jan 6, 2026
19c08c7
fix(trashbin): keep metadata consistent on move
hweihwang Jan 6, 2026
b5f9df8
perf(trashbin): avoid full rescan for uncached moves
hweihwang Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 94 additions & 23 deletions apps/files_trashbin/lib/Trashbin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,16 +296,70 @@

$configuredTrashbinSize = static::getConfiguredTrashbinSize($owner);
if ($configuredTrashbinSize >= 0 && $sourceInfo->getSize() >= $configuredTrashbinSize) {
$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
return false;
}

// 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 {
$moveSuccessful = true;
$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) {
$trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
} else {
$sizeDifference = $sourceInfo->getSize();
if ($sizeDifference < 0) {
$sizeDifference = null;
}
$trashStorage->getUpdater()->update($trashInternalPath, null, $sizeDifference);

Check failure on line 362 in apps/files_trashbin/lib/Trashbin.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

InvalidScalarArgument 

apps/files_trashbin/lib/Trashbin.php:362:67: InvalidScalarArgument: Argument 3 of OCP\Files\Cache\IUpdater::update expects int|null, but float|int<0, max>|null provided (see https://psalm.dev/012)
}
} catch (CopyRecursiveException $e) {
$moveSuccessful = false;
Expand All @@ -328,24 +382,31 @@
} else {
$trashStorage->getUpdater()->remove($trashInternalPath);
}
return false;
$moveSuccessful = false;
}

if ($moveSuccessful) {
// there is still a possibility that the file has been deleted by a remote user
$deletedBy = self::overwriteDeletedBy($user);

$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));
$result = $query->executeStatement();
if (!$result) {
Server::get(LoggerInterface::class)->error('trash bin database couldn\'t be updated', ['app' => 'files_trashbin']);
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) {
Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path),
'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]);

Expand Down Expand Up @@ -682,13 +743,6 @@
$size = 0;

if ($timestamp) {
$query = Server::get(IDBConnection::class)->getQueryBuilder();
$query->delete('files_trash')
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
$query->executeStatement();

$file = static::getTrashFilename($filename, $timestamp);
} else {
$file = $filename;
Expand All @@ -699,6 +753,14 @@
try {
$node = $userRoot->get('/files_trashbin/files/' . $file);
} catch (NotFoundException $e) {
if ($timestamp) {
$query = Server::get(IDBConnection::class)->getQueryBuilder();
$query->delete('files_trash')
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
$query->executeStatement();
}
return $size;
}

Expand All @@ -712,6 +774,15 @@
$node->delete();
self::emitTrashbinPostDelete('/files_trashbin/files/' . $file);

if ($timestamp) {
$query = Server::get(IDBConnection::class)->getQueryBuilder();
$query->delete('files_trash')
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
$query->executeStatement();
}

return $size;
}

Expand Down
19 changes: 19 additions & 0 deletions apps/files_trashbin/tests/StorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,25 @@ public function testSingleStorageDeleteFile(): void {
$this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.')));
}

public function testTrashEntryCreatedWhenSourceNotInCache(): void {
$this->userView->file_put_contents('uncached.txt', 'foo');

[$storage, $internalPath] = $this->userView->resolvePath('uncached.txt');
$cache = $storage->getCache();
$cache->remove($internalPath);
$this->assertFalse($cache->inCache($internalPath));

$this->userView->unlink('uncached.txt');

$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
$this->assertCount(1, $results);
$name = $results[0]->getName();
$this->assertEquals('uncached.txt', substr($name, 0, strrpos($name, '.')));

[$trashStorage, $trashInternalPath] = $this->rootView->resolvePath('/' . $this->user . '/files_trashbin/files/' . $name);
$this->assertTrue($trashStorage->getCache()->inCache($trashInternalPath));
}

/**
* Test that deleting a folder puts it into the trashbin.
*/
Expand Down
Loading