🐛 fix(soft): break stale markers without emptying the lock path

docs/changelog/843.bugfix.rst

@@ -0,0 +1,14 @@
+Fix :class:`~filelock.SoftFileLock`, :class:`~filelock.MarkerSoftFileLock` and :class:`~filelock.SoftFileLease` removing
+or displacing a live holder's marker in five cases. ext4 gave a freed inode number to the next marker. A stalled
+:class:`~filelock.FileLock` removed its ``ENOSYS`` placeholder after a peer replaced it. A failed acquire cleaned up
+after a successor took the path. A stale break left the lock path empty while it checked the marker it had moved. A
+``lifetime`` break judged a marker by a reused inode number. On Linux and macOS a stale marker now changes hands in one
+atomic exchange. On other platforms without one, except Windows, including NFS and SMB, the marker stays in place and
+waits for ``timeout`` or ``break_lock()``. Restoring a marker no longer follows a symlink found under the break name. A
+holder that releases while a peer's exchange has its marker aside leaves a record no lock parses, so the put-back lands
+a marker that ages out. A lease heartbeat that finds another file at the lock path once, while a peer's break has its
+marker aside, waits for the next refresh before it reports a compromise. A contender that finds a dead breaker's marker
+puts back the marker that breaker left under its break name. A marker is the breaker's own only when it names the
+breaker's process on this host; the same process number on another host is a holder. A contender takes a stranded marker
+under a name of its own before it puts the marker back, so two contenders no longer reverse each other's exchange and
+remove the live holder's marker. A claim left behind by a failed put-back is retried on the next poll.

docs/concepts.rst

@@ -230,10 +230,10 @@ Losing a private name makes the publisher retry; it never converts an unpublishe
     exclusion; remove the file only after the complete lock protocol has stopped using it.
 
     Filesystems without ``flock``: when ``flock`` fails with ``ENOSYS``, the lock switches to :class:`SoftFileLock
-    <filelock.SoftFileLock>` and emits a warning, unless constructed with ``fallback_to_soft=False``. Before switching it
-    removes the empty file only if this acquire created it. A file that already existed may be another process's soft
-    marker, so it stays: the soft lock waits on a live holder and reclaims an empty leftover once it has stayed
-    unchanged for two seconds.
+    <filelock.SoftFileLock>` and emits a warning, unless constructed with ``fallback_to_soft=False``. Before switching
+    it removes the empty file only if this acquire created it. It does so while still holding the file open, with the
+    exchange a stale break uses, so a peer's marker that replaced it is put back rather than deleted. A file that
+    already existed may be another process's soft marker, so it stays and the soft lock waits on it.
 
 **Other platforms without fcntl**
     Falls back to :class:`SoftFileLock <filelock.SoftFileLock>` and emits a warning. The lock is not enforced by the OS,
@@ -596,10 +596,15 @@ Malformed and legacy markers
 
 A soft lock treats a record it cannot parse as malformed, not as a holder. A plain :class:`SoftFileLock
 <filelock.SoftFileLock>` self-heals a malformed marker once it has watched it stay unchanged for a short grace window on
-its own monotonic clock, which absorbs the brief gap between creating a marker and writing its record. A strict lock
-never age-breaks, so it fails closed on an unreadable claim. A marker written by an older filelock stays readable: the
-process start token is an integer, so a 3.29 reader parses a newer marker as well-formed and stays conservative rather
-than evicting a live holder.
+its own monotonic clock, which absorbs the brief gap between creating a marker and writing its record.
+
+It tells markers apart by modification time, inode number and change time (the time the kernel last changed the file's
+metadata). An owner can set a file's modification time, and ext4 hands a freed inode number to the next file, but no
+process can set the change time. A new marker therefore never inherits the time spent watching an old one.
+
+A strict lock never age-breaks, so it fails closed on an unreadable claim. A marker written by an older filelock stays
+readable: the process start token is an integer, so a 3.29 reader parses a newer marker as well-formed and stays
+conservative rather than evicting a live holder.
 
 Filesystem support matrix
 =========================
@@ -692,19 +697,39 @@ container runtimes create, keeps the token. A process that cannot read ``/proc/s
 ``ProcSubset=pid`` hides it) records ``starttime`` without the boot id. A contender then compares ``starttime`` alone
 whenever either side lacks the boot id.
 
-Breaking a marker leaves a short window. Right before breaking it the contender reads the marker's inode and
-modification time again and leaves it alone if they moved since the stale check, the way Mercurial re-reads a lock under
-its break lock before removing it. It then renames the marker to a private ``<lock>.break.*`` name, so only one of
-several breakers takes it, and checks the renamed file's inode and modification time once more. A mismatch means a peer
-recreated the marker between that last read and the rename, so the contender hard-links that live marker back to the
-lock path and drops the break name. A third process that takes the empty path in the two syscalls between the rename
-and the link keeps it, whether a soft lock creating its marker or a :class:`StrictSoftFileLock
-<filelock.StrictSoftFileLock>` publishing its sentinel, and the live marker stays under the break name. Both then hold
-the lock. Mercurial
-closes this window by serializing breakers on a separate ``<lock>.break`` lock; filelock does not, because a breaker that
-crashes would leave that lock needing stale detection of its own and a 3.29 peer never takes it. Where ``os.link`` is
-missing or the filesystem refuses hard links, the live marker stays under the break name. The same applies to the
-``lifetime`` and :class:`SoftFileLease <filelock.SoftFileLease>` expiry breaks.
+A contender never leaves the lock path empty while it breaks a marker. Right before breaking it reads the marker's inode
+(the kernel's identity for a file, independent of its name) and modification time again. If they moved since the stale
+check, it leaves the marker alone, the way Mercurial re-reads a lock under its break lock before removing it. On Linux
+(``renameat2``) and macOS (``renamex_np``) it then writes a complete marker of its own under a private
+``<lock>.break.*`` name and exchanges the two names in one step. A contender that polls meanwhile finds a marker and
+waits.
+
+If the file it took is the one judged stale, it removes that file and holds the lock from then on. A peer that
+exchanges after that moves the holder's marker aside only while it checks what it took, and puts it back. A holder that
+releases while its marker is aside leaves a record no lock parses, so the put-back lands a marker that ages out.
+Otherwise a peer recreated the marker, and a second exchange puts the live marker back before its own is removed.
+
+The private name is ``<lock>.break.<pid>.<token>``: the breaker's PID, then 32 random hex digits. A breaker killed
+between its exchange and the put-back leaves its own marker at the lock path and a live holder's marker under that name.
+A contender that finds the breaker's marker dead first looks in the lock's directory for names that start with
+``<lock>.break.<pid>.``, where ``<pid>`` is the dead breaker's. It first renames the record to
+``<lock>.restore.<pid>.<token>`` with its own PID, so only one of several contenders acts on it; a record whose
+recoverer died there is claimable again, and a claim a failed put-back left in the same process is retried on the next
+poll. It then exchanges back a marker whose owner is alive or cannot be proven dead, and removes one whose owner is
+dead. Where the filesystem refuses that exchange, the lock counts as held until ``timeout`` or ``break_lock()``. The
+placeholder a break writes while it works carries the same PID, so a contender that evicts a placeholder a killed
+process left finds the same names.
+
+The contender holds the judged marker open from before it reads the record until the break ends. ext4 gives a freed
+inode number to the next file created, so a successor's marker could otherwise carry the number of the marker judged
+stale.
+
+Windows has no such exchange, but it refuses to rename a marker its live holder keeps open. There the contender renames
+the marker aside, checks it, and links back a marker it took by mistake; ``link`` never replaces a name. Anywhere else
+without an exchange, including NFS and SMB, which refuse it, a contender does not move a marker at all, because the path
+would stand empty beside a possibly live holder. The marker then counts as held until ``timeout`` or ``break_lock()``.
+The ``lifetime`` and :class:`SoftFileLease <filelock.SoftFileLease>` expiry breaks follow the same rules, and a failed
+acquire removes its own marker by the same exchange before it closes it.
 
 A PID means something only inside its PID namespace, and two containers in one pod share the hostname and the boot id
 without sharing one. On Linux the recorded hostname therefore names the namespace too, as ``<host>?pidns-<inode>`` with
@@ -718,13 +743,14 @@ host's namespace reads such a marker from a container that shares the hostname (
 names another process or none. Before it calls that owner dead it looks for a task in another namespace whose innermost
 PID (``NSpid`` in ``/proc/<pid>/status``) and start token match the record, since ``starttime`` and the boot id read the
 same in every PID namespace, and keeps the marker if one exists. From a time namespace with a boottime offset it matches
-the innermost PID alone. Three limits remain. A marker without a start token, a kernel before
-4.1 without ``NSpid``, or a ``/proc`` this process cannot list leaves the check nothing to find. Where
-``/proc/self/ns/pid`` cannot be read, the bare hostname is all that separates namespaces. And a contender never reclaims
-a marker from another namespace: a container that crashes and is replaced comes back in a new namespace. A
-:class:`~filelock.Timeout` names such a holder. Containers that can crash while holding a soft lock should use
-:class:`SoftFileLease <filelock.SoftFileLease>`, set ``lifetime``, or call ``break_lock()`` at entrypoint once nothing
-else can hold the path.
+the innermost PID alone.
+
+Three limits remain. A marker without a start token, a kernel before 4.1 without ``NSpid``, or a ``/proc`` this process
+cannot list leaves the check nothing to find. Where ``/proc/self/ns/pid`` cannot be read, the bare hostname is all that
+separates namespaces. And a contender never reclaims a marker from another namespace: a container that crashes and is
+replaced comes back in a new namespace. A :class:`~filelock.Timeout` names such a holder. Containers that can crash
+while holding a soft lock should use :class:`SoftFileLease <filelock.SoftFileLease>`, set ``lifetime``, or call
+``break_lock()`` at entrypoint once nothing else can hold the path.
 
 Why is ReadWriteLock backed by SQLite?
 ======================================

docs/how-to.rst

@@ -812,8 +812,9 @@ Every platform stores a process start token in the marker to guard against PID r
 them unchanged for two seconds.
 That recovery path is not fail closed.
 
-A contender breaking a stale marker while a peer recreates it can admit a third holder, soft or strict, in a window of
-two syscalls; see :ref:`concepts:How does stale lock detection work across platforms?` for the details.
+A contender breaks a stale marker without the lock path ever standing empty. Where it cannot (NFS, SMB and platforms
+without an atomic exchange, other than Windows) it leaves the marker in place; see :ref:`concepts:How does stale lock
+detection work across platforms?` for the details.
 
 ********************************
  Use fail-closed soft locks

src/filelock/_api.py

@@ -38,7 +38,7 @@
 from weakref import WeakKeyDictionary, WeakValueDictionary, ref
 
 from filelock._error import SoftFileLockLifetimeWarning, Timeout
-from filelock._util import break_lock_file, ensure_directory_exists
+from filelock._util import ensure_directory_exists
 
 #: No explicit file permission mode was passed. Lock files then open with 0o666 so umask and default ACLs pick
 #: the final permissions, and fchmod is skipped to preserve POSIX default ACL inheritance.
@@ -1665,18 +1665,7 @@ def _commit_release(self) -> None:
         self._context.claim_root = None
 
     def _try_break_expired_lock(self) -> None:
-        """Remove the lock file if its modification time exceeds the configured :attr:`lifetime`."""
-        if (lifetime := self._context.lifetime) is None:
-            return
-        with contextlib.suppress(OSError):
-            # lstat, not stat: an attacker with write access to the lock directory can replace a held
-            # lock file with a symlink pointing at an old file, making stat() report the target's stale
-            # mtime so a waiter breaks a live lock and two processes hold it at once. lstat reads the
-            # symlink's own mtime, matching the O_NOFOLLOW reads elsewhere.
-            st = os.lstat(self._lock_path)
-            if time.time() - st.st_mtime < lifetime:
-                return
-            break_lock_file(self._lock_path, st.st_mtime, st.st_ino)
+        """Remove the lock file once it is older than :attr:`lifetime`; only an existence lock accepts one."""
 
     def _check_give_up(
         self,

src/filelock/_lease.py

@@ -28,7 +28,7 @@
 from filelock._identity import owner_is_stale
 from filelock._marker import MarkerSoftFileLock, OwnerMode, OwnerRecord, parse_marker
 from filelock._soft import _read_lock_file
-from filelock._util import break_lock_file, touch
+from filelock._util import pin_inode, touch
 
 if TYPE_CHECKING:
     from collections.abc import Callable, Mapping
@@ -239,14 +239,22 @@ def _published_record(self) -> OwnerRecord:
         return super()._published_record()._replace(token=self._claim.token, lease_duration=self._lease_duration)
 
     def _try_break_stale_lock(self) -> None:
+        # A break can fail for reasons a contender must ride out rather than raise on: a peer broke the marker first,
+        # or Windows refuses to rename a file whose holder still has it open. Poll again instead.
+        with suppress(OSError), pin_inode(self._lock_path) as pinned:
+            self._try_break_pinned_lock(pinned)
+
+    def _try_break_pinned_lock(self, pinned: int | None) -> None:
         if (peer := self._read_peer()) is None:
             # Not a readable protocol 2 lease record: a partial write, a foreign or legacy protocol 1 marker, or the
             # strict sentinel. The base self-heal evicts a genuinely malformed marker once it ages past the grace
             # window and leaves a legitimate legacy or strict holder in place, so a corrupt marker no longer wedges
             # every lease contender until its own timeout.
             super()._try_break_stale_lock()
             return
-        owner, mtime, ino = peer
+        owner, marker = peer
+        if pinned not in {None, marker.st_ino}:  # pragma: win32 no cover  # Windows pins nothing
+            return  # replaced before the read, so nothing keeps the number read from reuse
         # Only a peer that published a lease agreed to be superseded by age; an exclusive owner is reclaimed only once
         # it is provably dead, and a contract this version does not know never is. Raise the mismatch outside the read
         # so the suppression cannot swallow it.
@@ -256,22 +264,19 @@ def _try_break_stale_lock(self) -> None:
                 f"{self._lease_duration!r}s; every contender for a path must agree on lease_duration"
             )
             raise LeaseSettingsMismatch(msg)
-        # A break can fail for reasons a contender must ride out rather than raise on: a peer broke the marker first,
-        # or Windows refuses to rename a file whose holder still has it open. Poll again instead.
-        with suppress(OSError):
-            # A dead or recycled owner is reclaimed at once, whether it held a lease or an exclusive marker; a live
-            # lease owner whose marker stops being refreshed is superseded on the schedule every contender agreed to.
-            if owner.mode != "unknown" and owner_is_stale(owner.pid, owner.hostname, owner.start):
-                break_lock_file(self._lock_path, mtime, ino)
-                return
-            if owner.mode == "lease" and self._marker_unchanged_for(mtime, ino) >= self._lease_duration:
-                break_lock_file(self._lock_path, mtime, ino)
-
-    def _read_peer(self) -> tuple[OwnerRecord, float, int] | None:
+        # A dead or recycled owner is reclaimed at once, whether it held a lease or an exclusive marker; a live lease
+        # owner whose marker stops being refreshed is superseded on the schedule every contender agreed to.
+        if owner.mode != "unknown" and owner_is_stale(owner.pid, owner.hostname, owner.start):
+            self._take_over(marker, owner.pid)
+            return
+        if owner.mode == "lease" and self._marker_unchanged_for(marker) >= self._lease_duration:
+            self._take_over(marker, None)
+
+    def _read_peer(self) -> tuple[OwnerRecord, os.stat_result] | None:
         with suppress(OSError, ValueError):
-            content, mtime, ino = _read_lock_file(self._lock_path)
+            content, marker = _read_lock_file(self._lock_path)
             if (owner := parse_marker(content)) is not None:
-                return owner, mtime, ino
+                return owner, marker
         return None
 
     def _start_heartbeat(self, claim: _LeaseClaim, fd: int, identity: tuple[int, int], token: str) -> None:
@@ -347,8 +352,9 @@ def run(self) -> None:
             if outcome == "ok":
                 last_success, missing = time.monotonic(), False
                 continue
-            # A peer's stale break moves a live marker aside for two syscalls, so only a second miss in a row is a loss.
-            if outcome == "marker-missing" and not missing:
+            # A peer's stale break exchanges a live marker aside for the length of its check, so only a second miss or
+            # change in a row is a loss.
+            if outcome in {"marker-missing", "owner-changed"} and not missing:
                 missing = True
                 continue
             if outcome == "transient" and time.monotonic() - last_success < self.duration - self.interval: