Skip to content

fix(lock): move the etag when a lock is dropped for having expired #1291

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

Open
solracsf wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(lock): move the etag when a lock is dropped for having expired #1291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 33 additions & 10 deletions lib/Service/LockService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public function getLockForNodeIds(array $nodeIds): array {
$expiredLocks = [];
foreach ($newLocks as $lock) {
if ($lock->getETA() === 0) {
$expiredLocks[] = $lock->getId();
$expiredLocks[] = $lock;
$locks[$lock->getFileId()] = false;
$this->lockCache[$lock->getFileId()] = false;
} else {
Expand All @@ -129,9 +129,7 @@ public function getLockForNodeIds(array $nodeIds): array {
}
}

if (count($expiredLocks) > 0) {
$this->locksRequest->removeIds($expiredLocks);
}
$this->removeLocks($expiredLocks);

return $locks;
}
Expand Down Expand Up @@ -163,7 +161,7 @@ public function lock(LockContext $lockScope): FileLock {
$this->logger->notice('locking file', ['fileLock' => $lock]);
$this->injectMetadata($lock);
$this->locksRequest->save($lock);
$this->propagateEtag($lockScope);
$this->propagateEtag($lockScope->getNode());
return $lock;
}
}
Expand Down Expand Up @@ -194,7 +192,7 @@ public function unlock(LockContext $lock, bool $force = false): FileLock {

$this->locksRequest->delete($known);
$this->lockCache[$lock->getNode()->getId()] = false;
$this->propagateEtag($lock);
$this->propagateEtag($lock->getNode());
$this->injectMetadata($known);
return $known;
}
Expand Down Expand Up @@ -278,7 +276,6 @@ public function unlockFile(int $fileId, ?string $userId, bool $force = false, in
$lockType,
$userId,
);
$this->propagateEtag($lock);
return $this->unlock($lock, $force);
}

Expand Down Expand Up @@ -309,7 +306,7 @@ public function getDeprecatedLocks(int $limit = 0): array {
public function getLockFromFileId(int $fileId): FileLock {
$lock = $this->locksRequest->getFromFileId($fileId);
if ($lock->getETA() === 0) {
$this->locksRequest->delete($lock);
$this->removeLocks([$lock]);
throw new LockNotFoundException('lock is ignored and deleted as being too old.');
}

Expand Down Expand Up @@ -377,6 +374,9 @@ public function removeLocks(array $locks): void {
$this->logger->notice('removing locks', ['ids' => $ids]);

$this->locksRequest->removeIds($ids);
foreach ($locks as $lock) {
$this->propagateEtagForLock($lock);
}
}

public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock {
Expand Down Expand Up @@ -436,8 +436,31 @@ public function getRemoteLockFromDav(int $nodeId, ?Node $node = null): ?FileLock
}
}

private function propagateEtag(LockContext $lockContext): void {
$node = $lockContext->getNode();
/**
* A lock that goes away on its own has to move the etag the way an explicit
* unlock does, because sync clients only re-read the lock properties of a
* file whose etag changed.
*
* Runs without a session, for the cleanup job: the owner's folder gives a
* scoped lookup for a user lock, and the root falls back on the mount cache
* for an app owned lock, whose owner is an app id rather than an account.
*/
private function propagateEtagForLock(FileLock $lock): void {
try {
$node = null;
if ($this->userManager->userExists($lock->getOwner())) {
$node = $this->rootFolder->getUserFolder($lock->getOwner())->getFirstNodeById($lock->getFileId());
}
$node ??= $this->rootFolder->getFirstNodeById($lock->getFileId());
if ($node !== null) {
$this->propagateEtag($node);
}
} catch (Exception $e) {

@benjaminfrueh benjaminfrueh Oct 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@solracsf Could we catch \Throwable here instead of Exception?
removeLocks() deletes all rows before it updates any etag, so if one lock throws an Error, the loop aborts. The remaining locks, which could be fine, are then already deleted without their etags ever changing.

Suggested change
} catch (Exception $e) {
} catch (\Throwable $e) {

$this->logger->warning('Failed to propagate the etag of an expired lock', ['exception' => $e]);
}
}

private function propagateEtag(Node $node): void {
$node->getStorage()->getCache()->update($node->getId(), [
'etag' => uniqid(),
]);
Expand Down
76 changes: 76 additions & 0 deletions tests/Feature/LockFeatureTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use OCA\FilesLock\AppInfo\Application;
use OCA\FilesLock\ConfigLexicon;
use OCA\FilesLock\DAV\LockPlugin;
use OCA\FilesLock\Exceptions\LockNotFoundException;
use OCA\FilesLock\Model\FileLock;
use OCA\FilesLock\Service\LockService;
use OCP\AppFramework\Utility\ITimeFactory;
Expand All @@ -24,6 +25,7 @@
use OCP\Lock\ManuallyLockedException;
use OCP\Share\IManager as IShareManager;
use OCP\Share\IShare;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Group;
use PHPUnit\Framework\MockObject\MockObject;
use Sabre\DAV\Locks\LockInfo;
Expand Down Expand Up @@ -189,6 +191,80 @@ public function testLockEtagShare(): void {
self::assertNotEquals($oldEtag, $file->getEtag());
}

/**
* Every way a lock can be dropped for having expired.
*
* @return array<string, list<\Closure(LockService, int): void>>
*/
public static function expiryTriggers(): array {
return [
'the cleanup job' => [static function (LockService $service, int $fileId): void {
$service->removeLocks($service->getDeprecatedLocks());
}],
'a single read' => [static function (LockService $service, int $fileId): void {
try {
$service->getLockFromFileId($fileId);
} catch (LockNotFoundException) {
}
}],
'the bulk read behind a directory PROPFIND' => [static function (LockService $service, int $fileId): void {
$service->getLockForNodeIds([$fileId]);
}],
];
}

/**
* A lock that expires has to move the etag the way an explicit unlock does.
* Sync clients only re-read a file's lock properties when its etag changed,
* so without this the desktop keeps showing an expired lock and keeps the
* local file read-only until someone locks and unlocks it by hand.
*
* @param \Closure(LockService, int): void $expire
*/
#[DataProvider('expiryTriggers')]
public function testExpiredLockPropagatesEtag(\Closure $expire): void {
\OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigLexicon::LOCK_TIMEOUT, 30);
$file = $this->loginAndGetUserFolder(self::TEST_USER1)->newFile('etag_test', 'etag_test');
$this->lockManager->lock(new LockContext($file, ILock::TYPE_USER, self::TEST_USER1));

$file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('etag_test');
$fileId = $file->getId();
$lockedEtag = $file->getEtag();
$lockedRootEtag = $this->loginAndGetUserFolder(self::TEST_USER1)->getEtag();

$this->toTheFuture(30 * 60 + 1);
$service = \OCP\Server::get(LockService::class);
$service->clearCache();
$expire($service, $fileId);

$file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('etag_test');
$newRootEtag = $this->loginAndGetUserFolder(self::TEST_USER1)->getEtag();

self::assertNotEquals($lockedEtag, $file->getEtag(), 'the file etag must change when the lock expires');
self::assertNotEquals($lockedRootEtag, $newRootEtag, 'and the change must propagate up the tree');
}

/**
* An app lock is owned by an app id, not by an account, so the file cannot be
* resolved through an owner's folder and has to come from the mount cache.
*/
public function testExpiredAppLockPropagatesEtag(): void {
\OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, ConfigLexicon::LOCK_TIMEOUT, 30);
$file = $this->loginAndGetUserFolder(self::TEST_USER1)->newFile('etag_test', 'etag_test');
$this->lockManager->lock(new LockContext($file, ILock::TYPE_APP, 'collaborative_app'));

$file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('etag_test');
$lockedEtag = $file->getEtag();

$this->toTheFuture(30 * 60 + 1);
$service = \OCP\Server::get(LockService::class);
$service->clearCache();
$service->removeLocks($service->getDeprecatedLocks());

$file = $this->loginAndGetUserFolder(self::TEST_USER1)->get('etag_test');
self::assertNotEquals($lockedEtag, $file->getEtag(), 'an app owned lock has no account to resolve through');
}

public function testUnlockEtagShare(): void {
$file = $this->loginAndGetUserFolder(self::TEST_USER1)
->newFile('etag_test', 'etag_test');
Expand Down
Loading