🐛 fix(soft): restore a live marker a stale break moved aside

docs/changelog/786.bugfix.rst

@@ -0,0 +1,2 @@
+:class:`~filelock.SoftFileLock` and :class:`~filelock.SoftFileLease` put back a live successor's marker that a racing
+stale break moved aside, so a third contender no longer acquires beside the successor.

docs/concepts.rst

@@ -657,6 +657,16 @@ cannot be read, and a marker from another host all read as held. The start token
 remove it after two seconds, so a half-written marker never wedges acquisition. This is the rule PostgreSQL, Qt
 ``QLockFile``, and Mercurial converge on: break a stale lock only on proof of death.
 
+Breaking a marker leaves a short window. The contender renames the marker to a private ``<lock>.break.*`` name, so only
+one of several breakers takes it, then checks the renamed file's inode and modification time. A mismatch means a peer
+broke the same marker and acquired between the stale check 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 creates a marker in the two syscalls between the
+rename and the link keeps the path, 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.
+
 Why is ReadWriteLock backed by SQLite?
 ======================================
 

docs/how-to.rst

@@ -773,6 +773,9 @@ Every platform stores a process start token in the marker to guard against PID r
 ``GetProcessTimes`` creation time. Malformed records follow a different rule: a waiter may evict them after two seconds.
 That recovery path is not fail closed.
 
+Two contenders that break the same stale marker can admit a third holder in a window of two syscalls; see
+:ref:`concepts:How does stale lock detection work across platforms?` for the details.
+
 ********************************
  Use fail-closed soft locks
 ********************************

src/filelock/_util.py

@@ -86,13 +86,15 @@ def break_lock_file(lock_file: str, mtime_before: float, ino_before: int) -> Non
 
     We rename before unlinking so that, of several processes racing to break the same lock, one takes the file and the
     rest get ``OSError``. If we find a newer modification time or another inode on the renamed file, a peer recreated
-    the lock after we judged it stale, and we leave that live file under the break name. Its holder keeps running with
-    no file at the lock path, so a third process can acquire the lock alongside it. We do not rename the file back,
-    because on POSIX that would replace any marker a third process created after our rename. StrictSoftFileLock has no
-    such race. Its sole way to remove another process's claim is an operator's ``force_break``. We compare inodes as
-    well as modification times because NFS and FAT store modification times at coarse granularity, and a peer that
-    recreates the lock within that granularity leaves the old mtime on it. We call ``lstat`` to avoid following a
-    symlink a peer swaps in after our stale check.
+    the lock after we judged it stale, so we hard-link it back to the lock path and drop the break name. ``link`` never
+    replaces an existing name, so a marker a third process created after our rename wins and the live one stays under
+    the break name. That leaves a window of two syscalls in which a third process can acquire beside the live holder.
+    Without ``os.link``, or on a filesystem that refuses hard links, the live marker stays under the break name.
+    StrictSoftFileLock has no such race. Its sole way to remove another process's claim is an operator's
+    ``force_break``. We compare inodes as well as modification times because NFS and FAT store modification times at
+    coarse granularity, and a peer that recreates the lock within that granularity leaves the old mtime on it. We call
+    ``lstat`` to avoid following a symlink a peer swaps in after our stale check, and put back only a regular file
+    because ``link`` would follow a symlink to its target.
 
     We add a random token to the break name so other processes cannot guess it and two breakers in one process do not
     share ``<lock>.break.<pid>``. With a shared name, the second breaker could rename a recreated live lock onto that
@@ -102,13 +104,22 @@ def break_lock_file(lock_file: str, mtime_before: float, ino_before: int) -> Non
     :param mtime_before: modification time the caller saw when it judged the lock stale.
     :param ino_before: inode number the caller saw when it judged the lock stale.
 
-    :raises OSError: if the rename or the re-check fails, for example because the file vanished or another user owns it
-        in a sticky directory.
+    :raises OSError: if the rename, the re-check or the restore fails, for example because the file vanished or another
+        user owns it in a sticky directory.
 
     """
     break_path: Final[str] = f"{lock_file}.break.{os.getpid()}.{secrets.token_hex(16)}"
     Path(lock_file).rename(break_path)
-    if (st_after := os.lstat(break_path)).st_mtime > mtime_before or st_after.st_ino != ino_before:
+    if (st_after := os.lstat(break_path)).st_mtime <= mtime_before and st_after.st_ino == ino_before:
+        Path(break_path).unlink()
+    elif stat.S_ISREG(st_after.st_mode) and _HAS_LINK:  # pragma: needs hard-link
+        _restore_live_marker(break_path, lock_file)
+
+
+def _restore_live_marker(break_path: str, lock_file: str) -> None:  # pragma: needs hard-link
+    try:
+        os.link(break_path, lock_file)
+    except FileExistsError:
         return
     Path(break_path).unlink()
 
@@ -124,6 +135,8 @@ def touch(name: str, *, fd: int) -> None:
     os.utime(name, None, follow_symlinks=not _SUPPORTS_UTIME_NOFOLLOW)  # pragma: lacks utime-fd
 
 
+# Termux/Android CPython ships without os.link.
+_HAS_LINK: Final[bool] = hasattr(os, "link")
 # Retargeting os.utime to an open fd lets a heartbeat refresh the exact inode it verified instead of whatever the
 # pathname now names.
 _SUPPORTS_UTIME_FD: Final[bool] = sys.platform != "win32" and os.utime in os.supports_fd

tests/test_soft_stale.py

@@ -190,6 +190,27 @@ def test_stale_lock_rename_race(lock_path: Path, mocker: MockerFixture) -> None:
     _assert_times_out(lock_path)
 
 
+@pytest.mark.requires_hard_links
+def test_stale_break_racing_a_successor_admits_no_third_holder(  # pragma: needs hard-link
+    lock_path: Path, mocker: MockerFixture
+) -> None:
+    lock_path.write_text(_holder(_DEAD_PID), encoding="utf-8")
+    successor: Final[SoftFileLock] = SoftFileLock(lock_path, timeout=0)
+    rename: Final = Path.rename
+
+    def successor_breaks_and_acquires_first(source: Path, target: str) -> Path:
+        source.unlink()
+        successor.acquire()
+        return rename(source, target)
+
+    mocker.patch.object(Path, "rename", autospec=True, side_effect=successor_breaks_and_acquires_first)
+    _assert_times_out(lock_path)
+    mocker.stopall()
+
+    _assert_times_out(lock_path)
+    successor.release()
+
+
 @NEEDS_SYMLINK
 def test_symlinked_lock_file_is_not_followed(tmp_path: Path, lock_path: Path) -> None:  # pragma: needs symlink
     target = tmp_path / "target"

tests/test_util.py

@@ -13,6 +13,7 @@
 import pytest
 
 from filelock import SoftFileLock, Timeout
+from tests.capability_marks import NEEDS_SYMLINK
 
 if TYPE_CHECKING:
     from collections.abc import Callable, Generator
@@ -40,12 +41,12 @@ def test_break_lock_file_unlinks_a_dead_holders_marker(stale_lock: Path) -> None
         assert list(stale_lock.parent.glob("test.lock.break.*")) == []
 
 
-def _rewrite_in_place(lock: Path) -> None:
+def _rewrite_in_place(lock: Path) -> None:  # pragma: needs hard-link
     lock.write_text("live", encoding="utf-8")
     os.utime(lock, (time.time() + 10, time.time() + 10))
 
 
-def _replace_with_same_mtime(lock: Path) -> None:
+def _replace_with_same_mtime(lock: Path) -> None:  # pragma: needs hard-link
     # A filesystem with coarse modification times (NFS, FAT) gives a same-second recreation the old mtime, so only the
     # inode tells it apart. Writing the replacement while the original exists guarantees a fresh inode.
     stale: Final[os.stat_result] = lock.stat()
@@ -54,11 +55,12 @@ def _replace_with_same_mtime(lock: Path) -> None:
     replacement.replace(lock)
 
 
+@pytest.mark.requires_hard_links
 @pytest.mark.parametrize(
     "recreate",
     [pytest.param(_rewrite_in_place, id="mtime-advanced"), pytest.param(_replace_with_same_mtime, id="inode-changed")],
 )
-def test_break_lock_file_leaves_a_recreated_marker_aside(
+def test_break_lock_file_restores_a_recreated_marker(  # pragma: needs hard-link
     stale_lock: Path, mocker: MockerFixture, recreate: Callable[[Path], None]
 ) -> None:
     rename: Final = Path.rename
@@ -71,11 +73,53 @@ def peer_recreates_then_rename(source: Path, target: str) -> Path:
     with pytest.raises(Timeout):
         SoftFileLock(stale_lock).acquire(blocking=False)
 
-    # The live holder needs its marker, so it survives under the break name and the lock path stays empty.
+    assert (stale_lock.read_text(encoding="utf-8"), list(stale_lock.parent.glob("test.lock.break.*"))) == ("live", [])
+
+
+@pytest.mark.requires_hard_links
+def test_break_lock_file_keeps_a_third_marker_created_after_the_rename(  # pragma: needs hard-link
+    stale_lock: Path, mocker: MockerFixture
+) -> None:
+    rename: Final = Path.rename
+
+    def peer_recreates_then_third_creates(source: Path, target: str) -> Path:
+        _rewrite_in_place(source)
+        result: Final[Path] = rename(source, target)
+        source.write_text("third", encoding="utf-8")
+        return result
+
+    mocker.patch.object(Path, "rename", autospec=True, side_effect=peer_recreates_then_third_creates)
+    with pytest.raises(Timeout):
+        SoftFileLock(stale_lock).acquire(blocking=False)
+
+    # link never replaces, so the third marker keeps the path and the live one waits under the break name.
     assert (
+        stale_lock.read_text(encoding="utf-8"),
         [path.read_text(encoding="utf-8") for path in stale_lock.parent.glob("test.lock.break.*")],
-        stale_lock.exists(),
-    ) == (["live"], False)
+    ) == ("third", ["live"])
+
+
+@NEEDS_SYMLINK
+def test_break_lock_file_leaves_a_swapped_in_symlink_aside(  # pragma: needs symlink
+    stale_lock: Path, mocker: MockerFixture
+) -> None:
+    (target := stale_lock.with_name("target")).write_text("victim", encoding="utf-8")
+    rename: Final = Path.rename
+
+    def peer_swaps_in_a_symlink_then_rename(source: Path, destination: str) -> Path:
+        source.unlink()
+        source.symlink_to(target)
+        return rename(source, destination)
+
+    mocker.patch.object(Path, "rename", autospec=True, side_effect=peer_swaps_in_a_symlink_then_rename)
+    with pytest.raises(Timeout):
+        SoftFileLock(stale_lock).acquire(blocking=False)
+
+    # Linking the symlink back would follow it and publish the victim file's inode at the lock path.
+    assert ([path.is_symlink() for path in stale_lock.parent.glob("test.lock.break.*")], stale_lock.exists()) == (
+        [True],
+        False,
+    )
 
 
 def test_break_lock_file_lets_acquire_retry_after_the_marker_vanishes(stale_lock: Path, mocker: MockerFixture) -> None: