Skip to content

fix(files): release target lock when copy source is locked #62469

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
joshtrichards wants to merge 3 commits into
base: master
Choose a base branch
from
+82 −8
Draft

fix(files): release target lock when copy source is locked #62469

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
188c868
fix(view): release target lock when copy source is locked
joshtrichards Jul 24, 2026
1a6b45a
test(files): cover copy cleanup when source lock fails
joshtrichards Jul 24, 2026
894d092
chore(view): fix typo / missing bracket in refactor
joshtrichards Jul 24, 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
59 changes: 51 additions & 8 deletions lib/private/Files/View.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,13 +955,16 @@ public function copy($source, $target, $preserveMtime = false) {
}
$run = true;

$this->lockFile($target, ILockingProvider::LOCK_SHARED);
$this->lockFile($source, ILockingProvider::LOCK_SHARED);
$targetLocked = $this->lockFile($target, ILockingProvider::LOCK_SHARED);
$sourceLocked = false;
$lockTypePath1 = ILockingProvider::LOCK_SHARED;
$lockTypePath2 = ILockingProvider::LOCK_SHARED;

$operationException = null;
try {
$sourceLocked = $this->lockFile($source, ILockingProvider::LOCK_SHARED);
$exists = $this->file_exists($target);

if ($this->shouldEmitHooks($source) && $this->shouldEmitHooks($target)) {
\OC_Hook::emit(
Filesystem::CLASSNAME,
Expand All @@ -974,6 +977,7 @@ public function copy($source, $target, $preserveMtime = false) {
);
$this->emit_file_hooks_pre($exists, $target, $run);
}

if ($run) {
$mount1 = $this->getMount($source);
$mount2 = $this->getMount($target);
Expand Down Expand Up @@ -1014,15 +1018,54 @@ public function copy($source, $target, $preserveMtime = false) {
$this->emit_file_hooks_post($exists, $target);
}
}
} catch (\Exception $e) {
$this->unlockFile($target, $lockTypePath2);
$this->unlockFile($source, $lockTypePath1);
} catch (\Throwable $e) {
$operationException = $e;
throw $e;
}
} finally {
$cleanupException = null;

// Preserve the prior target-then-source unlock order, but do not
// leave the source locked if releasing the target itself fails.
try {
if ($targetLocked) {
$this->unlockFile($target, $lockTypePath2);
}
} catch (\Throwable $unlockException) {
if ($operationException !== null) {
$this->logger->error('Failed to release target lock after copy operation', [
'app' => 'core',
'source' => $source,
'target' => $target,
'exception' => $unlockException,
]);
} else {
$cleanupException = $unlockException;
}
}

try {
if ($sourceLocked) {
$this->unlockFile($source, $lockTypePath1);
}
} catch (\Throwable $unlockException) {
if ($operationException !== null || $cleanupException !== null) {
$this->logger->error('Failed to release source lock after copy operation', [
'app' => 'core',
'source' => $source,
'target' => $target,
'exception' => $unlockException,
]);
} else {
$cleanupException = $unlockException;
}
}

$this->unlockFile($target, $lockTypePath2);
$this->unlockFile($source, $lockTypePath1);
if ($operationException === null && $cleanupException !== null) {
throw $cleanupException;
}
}
}

return $result;
}

Expand Down
31 changes: 31 additions & 0 deletions tests/lib/Files/ViewTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2315,6 +2315,37 @@ function (): void {
}
}

public function testCopyUnlocksTargetWhenSourceLockCannotBeAcquired(): void {
$view = new View('/' . self::$user . '/files/');
$storage = new Temporary([]);

Filesystem::mount($storage, [], self::$user . '/');
$storage->mkdir('files');

$sourcePath = 'source.txt';
$targetPath = 'target.txt';
$view->file_put_contents($sourcePath, 'content');

$view->lockFile($sourcePath, ILockingProvider::LOCK_EXCLUSIVE);

try {
$view->copy($sourcePath, $targetPath);
$this->fail('Expected source-lock acquisition to fail');
} catch (LockedException) {
$this->assertNull(
$this->getFileLockType($view, $targetPath),
'The target lock acquired before the source lock must be released'
);
$this->assertSame(
ILockingProvider::LOCK_EXCLUSIVE,
$this->getFileLockType($view, $sourcePath),
'The pre-existing source lock must not be released by copy()'
);
} finally {
$view->unlockFile($sourcePath, ILockingProvider::LOCK_EXCLUSIVE);
}
}

/**
* Test rename operation: unlock first path when second path was locked
*/
Expand Down
Loading