Repository navigation
Conversation
Member
Author
|
/backport to stable35 |
Member
Author
|
/backport to stable34 |
Member
Author
|
/backport to stable33 |
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>
solracsf
force-pushed
the
fix/etag-on-lock-expiry
branch
from
September 9, 2026 09:48
5f3664e to
3e6a67a
Compare
This was referenced Sep 9, 2026
|
Hopefully this is merged soon, has been driving me nuts for years... |
Contributor
|
@solracsf thank you for creating this PR, looks good to me. If this PR is AI assisted, before I can approve and merge it, can you add the disclosure required by our AI policy https://github.com/nextcloud/.github/blob/master/AI_POLICY.md
|
Member
Author
|
Asking someone if they use AI today is the same than asking if they use Internet to connect to GitHub. Will do 😉 |
solracsf
force-pushed
the
fix/etag-on-lock-expiry
branch
from
October 12, 2026 15:37
3e6a67a to
1b28847
Compare
Member
Author
|
Done 🤖 |
| if ($node !== null) { | ||
| $this->propagateEtag($node); | ||
| } | ||
| } catch (Exception $e) { |
Contributor
There was a problem hiding this comment.
@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 branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #191
Related to #981
An explicit unlock moves the file's etag, a lock that expires does not, and that difference decides what sync clients see. The desktop only re-reads a file's lock properties when its etag changed, and it short-circuits three times over: an unchanged root etag skips the sync run (
folder.cppetagRetrieved), an unchanged directory etag setsParentNotChangedso children are never PROPFINDed, and an unchanged file etag yields instructionNONE, which the sync engine ignores. Lock state is written back only onUPDATE_METADATA. So an expired lock is dropped here and the client is never told.On Windows the client also sets
FILE_ATTRIBUTE_READONLYplus a deny ACL, reverted only from inside a propagation job, so the local file stays read-only indefinitely. Both issues report the same self-discovered workaround, locking and unlocking 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.#981 is referenced rather than closed on purpose. This removes the server side cause for it, but the client only lifts its local read-only state inside
if (item->_type == ItemTypeVirtualFile)insyncengine.cpp, so a hydrated file gets the notification and stays read-only anyway. That half is nextcloud/desktop#10453. #191 is about the stale lock state itself, which the journal now reconciles for every file.Covers the 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 through the owner's folder first because that is what sets up a filesystem for the job, which runs without a session.
Worth knowing: this does not make the notification channel correct. Faking an etag change is also what produces a conflicted copy when a file is locked while a client has local edits (#553), so the channel misfires in both directions. Giving lock state its own signal is a larger change than this one, and #1230 is a separate matter.