Skip to content

[stable32] Handle duplicate key on trash unique constraint during concurrent deletes #4497

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

Merged
provokateurin merged 3 commits into from
Mar 12, 2026
+87 −12
Merged

[stable32] Handle duplicate key on trash unique constraint during concurrent deletes #4497

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
b5feb31
fix: handle duplicate key on trash unique constraint during concurren…
i2h3 Mar 5, 2026
e49dee7
fix: address review feedback for trash constraint handling
i2h3 Mar 6, 2026
4e17145
fix: tests
i2h3 Mar 11, 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
52 changes: 40 additions & 12 deletions lib/Trash/TrashBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,24 +251,50 @@ public function moveToTrash(IStorage $storage, string $internalPath): bool {

$trashFolder = $this->setupTrashFolder($folder, $storage->getUser());
$trashStorage = $trashFolder->getStorage();

$originalLocation = $internalPath;
if ($storage->instanceOfStorage(ISharedStorage::class)) {
/** @var Jail $jail */
$jail = $storage->getWrapperStorage();
$originalLocation = $jail->getUnjailedPath($originalLocation);
}

$deletedBy = $this->userSession->getUser();

// Insert the trash record before moving the file so that the
// on-disk name already matches the confirmed deleted_time.
// The unique constraint on (folder_id, name, deleted_time) uses
// second granularity, so concurrent deletes of same-named files
// can collide. Retry with an incremented timestamp on conflict.
$time = time();
for ($retry = 0; $retry < 5; $retry++) {
try {
$this->trashManager->addTrashItem($folderId, $name, $time, $originalLocation, $fileEntry->getId(), $deletedBy?->getUID() ?? '');
break;
} catch (\OCP\DB\Exception $e) {
if ($e->getReason() === \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
$time++;
} else {
throw $e;
}
}
}

$trashName = $name . '.d' . $time;
$targetInternalPath = $trashFolder->getInternalPath() . '/' . $trashName;
// until the fix from https://github.com/nextcloud/server/pull/49262 is in all versions we support we need to manually disable the optimization
if ($storage->instanceOfStorage(Encryption::class)) {
$result = $this->moveFromEncryptedStorage($storage, $trashStorage, $internalPath, $targetInternalPath);
} else {
$result = $trashStorage->moveFromStorage($storage, $internalPath, $targetInternalPath);
try {
if ($storage->instanceOfStorage(Encryption::class)) {
$result = $this->moveFromEncryptedStorage($storage, $trashStorage, $internalPath, $targetInternalPath);
} else {
$result = $trashStorage->moveFromStorage($storage, $internalPath, $targetInternalPath);
}
} catch (\Exception $e) {
// Move threw — clean up the DB record to avoid an orphaned trash entry
$this->trashManager->removeItem($folderId, $name, $time);
throw $e;
}
if ($result) {
$originalLocation = $internalPath;
if ($storage->instanceOfStorage(ISharedStorage::class)) {
$originalLocation = $storage->getWrapperStorage()->getUnjailedPath($originalLocation);
}

$deletedBy = $this->userSession->getUser();
$this->trashManager->addTrashItem($folderId, $name, $time, $originalLocation, $fileEntry->getId(), $deletedBy?->getUID() ?? '');

// some storage backends (object/encryption) can either already move the cache item or cause the target to be scanned
// so we only conditionally do the cache move here
if (!$trashStorage->getCache()->inCache($targetInternalPath)) {
Expand All @@ -279,6 +305,8 @@ public function moveToTrash(IStorage $storage, string $internalPath): bool {
$storage->getCache()->remove($internalPath);
}
} else {
// Move failed — clean up the DB record to avoid an orphaned trash entry
$this->trashManager->removeItem($folderId, $name, $time);
throw new \Exception('Failed to move Team folder item to trash');
}

Expand Down
47 changes: 47 additions & 0 deletions tests/Trash/TrashBackendTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use OCA\GroupFolders\Folder\FolderManager;
use OCA\GroupFolders\Mount\GroupFolderStorage;
use OCA\GroupFolders\Trash\TrashBackend;
use OCA\GroupFolders\Trash\TrashManager;
use OCP\Constants;
use OCP\Files\Folder;
use OCP\Files\IRootFolder;
Expand All @@ -37,6 +38,7 @@ class TrashBackendTest extends TestCase {

private string $folderName;
private TrashBackend $trashBackend;
private TrashManager $trashManager;
private FolderManager $folderManager;
private ACLManager $aclManager;
private RuleManager $ruleManager;
Expand All @@ -61,6 +63,7 @@ public function setUp(): void {
$groupBackend->addToGroup('normal', 'gf_normal');

$this->trashBackend = \OCP\Server::get(TrashBackend::class);
$this->trashManager = \OCP\Server::get(TrashManager::class);
$this->folderManager = \OCP\Server::get(FolderManager::class);
/** @var ACLManagerFactory $aclManagerFactory */
$aclManagerFactory = \OCP\Server::get(ACLManagerFactory::class);
Expand Down Expand Up @@ -289,4 +292,48 @@ public function testWrongOriginalLocation(): void {
$this->trashBackend->restoreItem($trashedOfUserA[0]);
$this->assertTrue($userAFolder->nodeExists('D/E/F/G'));
}

public function testMoveToTrashSameNameDifferentSubfolders(): void {
$this->loginAsUser('manager');

// Create two subfolders each containing a file with the same name
$folderA = $this->managerUserFolder->newFolder("{$this->folderName}/SubA");
$folderB = $this->managerUserFolder->newFolder("{$this->folderName}/SubB");
$fileA = $folderA->newFile('Readme.md', 'content A');
$fileB = $folderB->newFile('Readme.md', 'content B');

$this->assertTrue($this->managerUserFolder->nodeExists("{$this->folderName}/SubA/Readme.md"));
$this->assertTrue($this->managerUserFolder->nodeExists("{$this->folderName}/SubB/Readme.md"));

// Wait for the start of a new second so both deletes land in the same
// second and deterministically exercise the retry path.
$now = time();
while (time() === $now) {
usleep(10000);
}

// Delete both files — they share the same base name within the same
// group folder which previously triggered a unique constraint violation
// on (folder_id, name, deleted_time) when both deletes hit the same second.
$this->trashBackend->moveToTrash($fileA->getStorage(), $fileA->getInternalPath());
$this->trashBackend->moveToTrash($fileB->getStorage(), $fileB->getInternalPath());

$this->assertFalse($this->managerUserFolder->nodeExists("{$this->folderName}/SubA/Readme.md"));
$this->assertFalse($this->managerUserFolder->nodeExists("{$this->folderName}/SubB/Readme.md"));

// Both files must appear in the trash
$trashItems = $this->trashBackend->listTrashRoot($this->managerUser);
$this->assertCount(2, $trashItems, 'Both Readme.md files should be in the trash');

// The two trash entries must have distinct deleted_time values
$times = array_map(fn ($item) => $item->getDeletedTime(), $trashItems);
$this->assertCount(2, array_unique($times), 'Trash entries should have distinct deleted_time values');

// Verify the DB records exist
$dbRows = $this->trashManager->listTrashForFolders([$this->folderId]);
$dbReadmeRows = array_values(array_filter($dbRows, fn (array $row) => $row['name'] === 'Readme.md'));
$this->assertCount(2, $dbReadmeRows, 'Both Readme.md entries should exist in oc_group_folders_trash');

$this->logout();
}
}
Loading