From 1b288473d26c0309c5152f8b30be6beaf5e4f8e4 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Wed, 9 Sep 2026 11:10:15 +0200 Subject: [PATCH] fix: move the etag when a lock is dropped for having expired An explicit unlock moves the file's etag; a lock that goes away on its own does not. That difference is invisible on the server and decides what sync clients see, because the desktop only re-reads a file's lock properties when its etag changed. It short-circuits three times over: an unchanged root etag skips the sync run, an unchanged directory etag means the children are never PROPFINDed, and an unchanged file etag yields instruction NONE, which the sync engine ignores. Lock state is written back only on UPDATE_METADATA. So a lock that expires is removed here and the client never hears about it. On Windows the file also keeps FILE_ATTRIBUTE_READONLY and a deny ACL, which are only reverted from inside a propagation job, so the file stays read-only indefinitely. Both reporters of #191 and #981 found the same workaround by themselves: lock and unlock the file in the web UI, which moves the etag twice and lets the client catch up. #981 confirms both lock tables are empty while the file is still read-only. Move the etag on all three paths that drop an expired lock: the cleanup job, the single read, and the bulk read behind a directory PROPFIND. The file is resolved without a session, because the job has none. This does not make the notification channel right. Faking an etag change is also what produces a conflicted copy when a file is locked while someone has local edits (#553), so the channel misfires in both directions. Fixing that properly means giving lock state its own signal, which is a bigger change than this one. Green on MariaDB 11.8, PostgreSQL 16 and S3 as primary object store. Each of the three paths fails the new test without the change. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> fix(lock): route every expiry removal through removeLocks() Review of the previous commit found the delete and the etag propagation paired by convention at three call sites, and an etag moved before the permission check. - unlockFile() moved the etag before delegating to unlock(), which is where canUnlock() runs, so a refused unlock still told every sync client the file had changed. unlock() already moves it once the release succeeds, so the early call was both wrong and redundant. - the single read and the bulk read behind a directory PROPFIND now hand their expired locks to removeLocks() instead of repeating its body, which leaves one place that removes a lock and moves the etag together. - a failure to propagate is the fix failing silently, so it is logged at warning like the sibling failure in getDeprecatedLocks(), not at debug. Covers an app owned lock as well: its owner is an app id rather than an account, so it cannot resolve through an owner's folder and comes from the mount cache instead. That branch had no test. Green on MariaDB 11.8, PostgreSQL 16 and S3 as primary object store. Assisted-by: Claude:claude-opus-5-5 Assisted-by: Claude:claude-fable-5-1 Assisted-by: Codex:gpt-6 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Service/LockService.php | 43 +++++++++++++---- tests/Feature/LockFeatureTest.php | 76 +++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 10 deletions(-) diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index cbc643c6..15e7a082 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -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 { @@ -129,9 +129,7 @@ public function getLockForNodeIds(array $nodeIds): array { } } - if (count($expiredLocks) > 0) { - $this->locksRequest->removeIds($expiredLocks); - } + $this->removeLocks($expiredLocks); return $locks; } @@ -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; } } @@ -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; } @@ -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); } @@ -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.'); } @@ -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 { @@ -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) { + $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(), ]); diff --git a/tests/Feature/LockFeatureTest.php b/tests/Feature/LockFeatureTest.php index 75a5047f..95681a23 100644 --- a/tests/Feature/LockFeatureTest.php +++ b/tests/Feature/LockFeatureTest.php @@ -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; @@ -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; @@ -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> + */ + 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');