diff --git a/docs/changelog/749.bugfix.rst b/docs/changelog/749.bugfix.rst new file mode 100644 index 00000000..014cf253 --- /dev/null +++ b/docs/changelog/749.bugfix.rst @@ -0,0 +1,2 @@ +Fix ``MarkerSoftFileLock`` acquisition and prevent contenders from evicting live protocol-2 owners after two seconds. +Reclaim recognized records after owner death; preserve unknown contracts. diff --git a/src/filelock/_marker.py b/src/filelock/_marker.py index f8e7fa6f..cb3f7a12 100644 --- a/src/filelock/_marker.py +++ b/src/filelock/_marker.py @@ -5,9 +5,9 @@ from contextlib import suppress from typing import Final, Literal, NamedTuple -from ._identity import host_name, process_start_token +from ._identity import host_name, owner_is_stale, process_start_token from ._soft import SoftFileLock, _read_lock_file -from ._util import write_all +from ._util import break_lock_file, write_all #: Protocol 1 is the legacy ``\n\n[\n]`` marker that :class:`SoftFileLock` still writes. #: Protocol 2 carries the owner mode and the lease claim. A protocol 1 reader treats a protocol 2 marker as malformed @@ -16,27 +16,15 @@ _MAX_PID: Final[int] = 2**31 - 1 -#: ``unknown`` is never published: it names a mode some other filelock wrote that this version cannot interpret. Such a -#: record still identifies a live owner, so it is parsed rather than read as malformed and aged out. -OwnerMode = Literal["lease", "unknown"] - - -class OwnerRecord(NamedTuple): - """The owner published in a protocol 2 marker.""" - - pid: int - hostname: str - mode: OwnerMode - token: str | None = None - lease_duration: float | None = None - start: int | None = None +#: Preserve unknown contracts so contenders cannot mistake them for malformed, reclaimable markers. +OwnerMode = Literal["lease", "exclusive", "unknown"] class MarkerSoftFileLock(SoftFileLock): """An existence lock whose marker carries a protocol 2 owner record.""" - #: Filled in by each mode so the published record states the contract its holder acquired under. - _owner_mode: OwnerMode + #: Age-based expiry requires a lease contract; an exclusive holder grants none. + _owner_mode: OwnerMode = "exclusive" @property def owner(self) -> OwnerRecord | None: @@ -78,6 +66,15 @@ def force_break(self) -> None: """ self.break_lock() + def _try_break_stale_lock(self) -> None: + with suppress(OSError, ValueError): + snapshot: Final[tuple[str | None, float, int]] = _read_lock_file(self.lock_file) + owner: Final[OwnerRecord | None] + if (owner := parse_marker(snapshot[0])) is None: + super()._try_break_stale_lock() + elif owner.mode != "unknown" and owner_is_stale(owner.pid, owner.hostname, owner.start): + break_lock_file(self.lock_file, *snapshot[1:]) + def _read_owner(self) -> OwnerRecord | None: with suppress(OSError, ValueError): return parse_marker(_read_lock_file(self.lock_file)[0]) @@ -95,6 +92,17 @@ def _published_record(self) -> OwnerRecord: ) +class OwnerRecord(NamedTuple): + """Keep optional metadata when reading unknown lock modes.""" + + pid: int + hostname: str + mode: OwnerMode + token: str | None = None + lease_duration: float | None = None + start: int | None = None + + def encode_marker(record: OwnerRecord) -> bytes: """Render an owner record as the bytes a protocol 2 marker holds.""" lines = [_PROTOCOL, f"pid={record.pid}", f"host={record.hostname}", f"mode={record.mode}"] @@ -127,7 +135,7 @@ def _build_record(fields: dict[str, str]) -> OwnerRecord | None: # A record naming no mode at all states no contract and stays malformed. if (published := fields.get("mode")) is None: return None - mode: OwnerMode = "lease" if published == "lease" else "unknown" + mode: Final[OwnerMode] = published if published in {"lease", "exclusive"} else "unknown" hostname = fields.get("host") if not hostname or "pid" not in fields: return None diff --git a/tests/test_marker_records.py b/tests/test_marker_records.py index 4e7e1221..445e8c2d 100644 --- a/tests/test_marker_records.py +++ b/tests/test_marker_records.py @@ -2,16 +2,20 @@ import os import time +from multiprocessing import get_context from typing import TYPE_CHECKING import pytest -from filelock import SoftFileLease, Timeout +from filelock import MarkerSoftFileLock, OwnerRecord, SoftFileLease, Timeout from filelock._identity import process_start_token -from filelock._marker import OwnerRecord, encode_marker +from filelock._marker import encode_marker +from tests.process_helpers import cleanup_processes if TYPE_CHECKING: + from multiprocessing.process import BaseProcess from pathlib import Path + from typing import Final from pytest_mock import MockerFixture @@ -158,3 +162,68 @@ def test_marker_force_break_removes_the_marker(tmp_path: Path) -> None: SoftFileLease(marker).force_break() assert not marker.exists() + + +@pytest.mark.parametrize("acquire", [pytest.param(True, id="acquire"), pytest.param(False, id="context")]) +def test_marker_lock_publishes_exclusive(marker: Path, *, acquire: bool) -> None: + lock: Final[MarkerSoftFileLock] = MarkerSoftFileLock(marker) + with lock.acquire() if acquire else lock: + owner: Final[OwnerRecord | None] + assert (owner := MarkerSoftFileLock(lock.lock_file).owner) is not None + assert (lock.is_locked, lock.is_lock_held_by_us, lock.pid, owner.mode) == (True, True, os.getpid(), "exclusive") + assert (lock.is_locked, lock.pid, lock.owner) == (False, None, None) + + +@pytest.mark.parametrize( + "contender_type", + [pytest.param(MarkerSoftFileLock, id="marker"), pytest.param(SoftFileLease, id="lease")], +) +def test_marker_lock_excludes_contenders(marker: Path, contender_type: type[MarkerSoftFileLock]) -> None: + with MarkerSoftFileLock(marker): + os.utime(marker, (0, 0)) + with pytest.raises(Timeout): + contender_type(marker, timeout=0.1).acquire() + + +@pytest.mark.parametrize( + "mode", + [ + pytest.param("exclusive", id="exclusive"), + pytest.param("lease", id="lease"), + pytest.param("future", id="unknown"), + ], +) +def test_marker_lock_preserves_live_records(marker: Path, mode: str) -> None: + with MarkerSoftFileLock(marker): + record: Final[str] = marker.read_text(encoding="utf-8").replace("mode=exclusive", f"mode={mode}") + marker.write_text(record + "token=t\nduration=1\n", encoding="utf-8") + os.utime(marker, (0, 0)) + with pytest.raises(Timeout): + MarkerSoftFileLock(marker, timeout=0.1).acquire() + + +def test_marker_lock_reclaims_dead_owner(marker: Path) -> None: + process: Final[BaseProcess] = get_context("spawn").Process(target=_leave_marker, args=(marker,)) + with cleanup_processes([process]): + process.start() + process.join(timeout=5) + assert process.exitcode == 0 + with MarkerSoftFileLock(marker, timeout=1) as lock: + assert lock.pid == os.getpid() + + +def test_marker_lock_reclaims_malformed_record(marker: Path) -> None: + marker.write_text("broken", encoding="utf-8") + os.utime(marker, (0, 0)) + with MarkerSoftFileLock(marker, timeout=1) as lock: + assert lock.pid == os.getpid() + + +@pytest.fixture +def marker(tmp_path: Path) -> Path: + return tmp_path / "a" + + +def _leave_marker(marker: Path) -> None: + with MarkerSoftFileLock(marker): + os._exit(0)