Skip to content

fix(dav): report a lock that never expires as timeout 0, not -60 - #1271

Open
solracsf wants to merge 1 commit into
mainfrom
fix/dav-lock-timeout-infinite-sentinel
Open

fix(dav): report a lock that never expires as timeout 0, not -60#1271
solracsf wants to merge 1 commit into
mainfrom
fix/dav-lock-timeout-infinite-sentinel

Conversation

@solracsf

@solracsf solracsf commented Sep 6, 2026

Copy link
Copy Markdown
Member

Supersedes #1091, which reported the same symptom.

What goes wrong

lock_timeout defaults to -1 minutes and LockService::lock() multiplies it by 60, so a lock that never expires carries a lifetime of -60 seconds internally. That value was sent out raw on the nc:lock-timeout WebDAV property.

Clients don't treat a negative lifetime as "no expiry", they just add it to lock-time:

  • Android, FileActionsViewModel.getLockedUntil(), returns null only when lockTimeout == 0L; with -60 it computes lockTimestamp - 60 and renders an expiry one minute in the past, which is the reported "expires 1 minute ago" on a lock that is perfectly healthy.
  • Desktop parses the PROPFIND value with toULongLong() (networkjobs.cpp), so the negative value fails to parse and silently becomes 0 there, while the LOCK response path uses the signed toLongLong() (lockfilejobs.cpp) and stores -60. The two paths disagree today.

The fix

Send 0. That is the value clients already implement for "this lock does not expire": it was documented in this repo in #175 ("client implementations should properly handle this specific value") and Android checks for exactly it. Only the WebDAV property changes, the internal lifetime and ILock::getTimeout() are untouched.

The README had drifted to -1 in 807ca60, but -1 is the config value for lock_timeout, not the value on the wire, so that line goes back to 0. The config documentation added by that commit is correct and stays.

Why not omit the property, as #1091 does

Omitting it also fixes Android, since the parser defaults to 0 when the property is absent. But it turns a documented property into an absent one for every client that reads it, and it leaves the 0 contract from #175 unimplemented rather than honoured. Sending 0 fixes the same clients without dropping a property from the response.

Testing

LockFeatureTest::testInfiniteLockReportsNoExpiryOverWebdav drives LockPlugin::customProperties() and asserts both directions: 0 for a lock under the default config, 1800 for one taken with lock_timeout = 30. It fails before the change with Failed asserting that -60 is identical to 0 and passes after, on MariaDB 11.8, PostgreSQL 16 and Oracle 23. Full suite green on all three, psalm clean, php-cs-fixer clean.

setUp() now clears the lock_timeout app value so tests no longer inherit whatever the previous test left behind; the new test needs the real default to be in effect.

The default configuration is `lock_timeout = -1` minutes, which the lock
service turns into a lifetime of -60 seconds. That value went out raw on
the `nc:lock-timeout` WebDAV property, and clients add it to `lock-time`
to work out when the lock expires. So a lock that never expires arrived
as one that expired a minute ago, and the Android client happily showed
"1 minute ago" next to a perfectly healthy lock.

0 is the value the clients already implement for "this lock does not
expire" (documented here since #175, and Android checks for exactly
that), so send that instead. The README had drifted to `-1`, which was
the config value, not the property value; put it back.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
solracsf added a commit that referenced this pull request Sep 6, 2026
A lock that never expires has a lifetime of -1 internally, and that value
went out raw on the `nc:lock-timeout` WebDAV property. Clients don't read
a negative lifetime as "no expiry", they add it to `lock-time` and end up
with an expiry date in the past: the Android client returns no expiry
only for exactly 0, so anything negative shows a healthy lock as already
expired.

0 is the value the clients implement for "this lock does not expire",
documented here since #175. Only the property changes; the internal
lifetime and `ILock::getTimeout()` stay as they are.

Same change as #1271, kept here so the branch is correct on its own
whichever of the two lands first.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
solracsf added a commit that referenced this pull request Sep 7, 2026
A lock that never expires has a lifetime of -1 internally, and that value
went out raw on the `nc:lock-timeout` WebDAV property. Clients don't read
a negative lifetime as "no expiry", they add it to `lock-time` and end up
with an expiry date in the past: the Android client returns no expiry
only for exactly 0, so anything negative shows a healthy lock as already
expired.

0 is the value the clients implement for "this lock does not expire",
documented here since #175. Only the property changes; the internal
lifetime and `ILock::getTimeout()` stay as they are.

Same change as #1271, kept here so the branch is correct on its own
whichever of the two lands first.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant