refactor(locks): delegate cross-platform file locking to portalocker

openkb/locks.py

@@ -2,82 +2,37 @@
 
 The lock protocol is advisory and intended for local filesystem access by
 OpenKB processes. It does not guarantee cross-host coordination on networked
-or synced filesystems where ``fcntl.flock`` may be unavailable or inconsistent.
+or synced filesystems where the underlying OS lock may be unavailable or
+inconsistent.
 """
 from __future__ import annotations
 
 import contextlib
 import json
-import logging
 import os
 import tempfile
 import threading
-import time
 from pathlib import Path
 from typing import IO, Iterator
 
-logger = logging.getLogger(__name__)
-
-try:
-    import fcntl
-except ImportError:  # pragma: no cover - Windows has no fcntl (simulated in tests)
-    fcntl = None
-
-# Upper bound (seconds) on the Windows lock-acquire wait. fcntl.flock blocks in
-# the kernel indefinitely; the msvcrt fallback polls, so without a cap a genuine
-# error (or a never-released lock) would hang the process forever. Generous by
-# default so it never trips on a lock legitimately held through a long compile;
-# override via OPENKB_LOCK_TIMEOUT for constrained environments.
-_WINDOWS_LOCK_TIMEOUT = float(os.getenv("OPENKB_LOCK_TIMEOUT", "3600"))
+import portalocker
 
 
 def flock(fh: IO, *, exclusive: bool) -> None:
     """Acquire an advisory lock on an open file handle (cross-platform).
 
-    Uses ``fcntl.flock`` on POSIX. On Windows (no ``fcntl``) it falls back to
-    ``msvcrt.locking``, which provides only **exclusive** byte-range locks: a
-    shared (``exclusive=False``) request is taken exclusively. Over-locking is
-    safe for correctness but does not allow concurrent readers on Windows — and
-    because the in-process :class:`_LocalRwLock` admits multiple readers, truly
-    concurrent in-process readers serialise (and wait) on Windows. The blocking
-    acquire of ``fcntl.flock`` is emulated by retrying the non-blocking lock
-    with backoff, bounded by ``_WINDOWS_LOCK_TIMEOUT`` so a stuck lock raises
-    instead of hanging forever.
+    Delegates to :mod:`portalocker`, which is fcntl-backed on POSIX and
+    msvcrt/Win32-backed on Windows. The call blocks until the lock is acquired,
+    matching ``fcntl.flock``. Shared (``exclusive=False``) locks are honoured
+    where the platform supports them; on Windows they may be taken exclusively
+    (portalocker handles the platform differences).
     """
-    if fcntl is not None:
-        fcntl.flock(fh.fileno(), fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH)
-        return
-    import msvcrt
-    fh.seek(0)
-    start = time.monotonic()
-    delay = 0.05
-    warned = False
-    while True:
-        try:
-            msvcrt.locking(fh.fileno(), msvcrt.LK_NBLCK, 1)
-            return
-        except OSError:
-            elapsed = time.monotonic() - start
-            if elapsed >= _WINDOWS_LOCK_TIMEOUT:
-                raise  # surface a stuck/never-released lock instead of hanging
-            if not warned and elapsed >= 5:
-                logger.warning(
-                    "Still waiting for file lock on %s ...",
-                    getattr(fh, "name", "<lock>"),
-                )
-                warned = True
-            time.sleep(delay)
-            delay = min(delay * 2, 1.0)
+    portalocker.lock(fh, portalocker.LOCK_EX if exclusive else portalocker.LOCK_SH)
 
 
 def funlock(fh: IO) -> None:
     """Release a lock previously acquired with :func:`flock`."""
-    if fcntl is not None:
-        fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
-        return
-    import msvcrt
-    fh.seek(0)
-    msvcrt.locking(fh.fileno(), msvcrt.LK_UNLCK, 1)
+    portalocker.unlock(fh)
 
 
 _LOCKS_GUARD = threading.Lock()

pyproject.toml

@@ -44,6 +44,7 @@ dependencies = [
     "json-repair==0.59.10",
     "prompt_toolkit==3.0.52",
     "rich==15.0.0",
+    "portalocker==3.2.0",
 ]
 
 [project.urls]

tests/test_cross_platform_locks.py

@@ -1,104 +1,55 @@
 """Cross-platform behaviour for openkb.locks / openkb.config.
 
-The locking layer (#86) originally hard-imported ``fcntl`` and called
-``os.fchmod`` / directory ``os.fsync`` unconditionally — all Unix-only — which
-crashed OpenKB at import time on Windows (``ModuleNotFoundError: No module
-named 'fcntl'``, reported in VectifyAI/OpenKB#93). These tests pin the
-platform-neutral behaviour and simulate the Windows path on this host.
+File locking is delegated to :mod:`portalocker` (fcntl on POSIX, msvcrt/Win32
+on Windows), so OpenKB no longer hard-imports the Unix-only ``fcntl``. The
+atomic-write path still special-cases the Unix-only ``os.fchmod`` and directory
+``os.fsync``. These tests pin the platform-neutral behaviour that is verifiable
+on POSIX; portalocker carries its own Windows test coverage.
 """
 from __future__ import annotations
 
 import os
 import subprocess
 import sys
-import types
 
+import portalocker
 import pytest
 
 from openkb import locks
 
 
-def test_config_and_locks_import_without_fcntl():
-    """openkb.config / openkb.locks must import on a host without fcntl (Windows)."""
-    code = (
-        "import sys\n"
-        "sys.modules['fcntl'] = None\n"  # make `import fcntl` raise ImportError
-        "import openkb.locks, openkb.config\n"
-        "assert openkb.locks.fcntl is None\n"
-        "print('OK')\n"
-    )
-    result = subprocess.run(
-        [sys.executable, "-c", code], capture_output=True, text=True
-    )
-    assert result.returncode == 0, result.stderr
-    assert "OK" in result.stdout
-
-
 def test_flock_funlock_roundtrip(tmp_path):
-    """flock/funlock acquire and release an advisory lock on the real platform."""
+    """flock/funlock acquire and release both exclusive and shared locks."""
     lock_path = tmp_path / "test.lock"
     with lock_path.open("a+", encoding="utf-8") as fh:
         locks.flock(fh, exclusive=True)
+        locks.funlock(fh)
+        locks.flock(fh, exclusive=False)
         locks.funlock(fh)  # must not raise
 
 
-def test_flock_uses_msvcrt_when_fcntl_absent(monkeypatch, tmp_path):
-    """When fcntl is unavailable (Windows), locking is delegated to msvcrt."""
-    calls = []
-    fake_msvcrt = types.SimpleNamespace(
-        LK_LOCK=1, LK_NBLCK=2, LK_UNLCK=0,
-        locking=lambda fd, mode, nbytes: calls.append((mode, nbytes)),
-    )
-    monkeypatch.setattr(locks, "fcntl", None)
-    monkeypatch.setitem(sys.modules, "msvcrt", fake_msvcrt)
-
+def test_flock_exclusive_blocks_other_process(tmp_path):
+    """An exclusive flock is a real OS lock that excludes another process."""
     lock_path = tmp_path / "test.lock"
-    with lock_path.open("a+", encoding="utf-8") as fh:
-        locks.flock(fh, exclusive=True)
+    fh = lock_path.open("a+", encoding="utf-8")
+    locks.flock(fh, exclusive=True)
+    try:
+        probe = (
+            "import portalocker\n"
+            f"fh = open({str(lock_path)!r}, 'a+')\n"
+            "try:\n"
+            "    portalocker.lock(fh, portalocker.LOCK_EX | portalocker.LOCK_NB)\n"
+            "    print('ACQUIRED')\n"
+            "except portalocker.LockException:\n"
+            "    print('BLOCKED')\n"
+        )
+        result = subprocess.run(
+            [sys.executable, "-c", probe], capture_output=True, text=True
+        )
+        assert "BLOCKED" in result.stdout, result.stdout + result.stderr
+    finally:
         locks.funlock(fh)
-
-    modes = [mode for mode, _ in calls]
-    assert fake_msvcrt.LK_NBLCK in modes  # acquire used the non-blocking lock
-    assert fake_msvcrt.LK_UNLCK in modes  # release unlocked
-
-
-def test_flock_retries_until_lock_available(monkeypatch, tmp_path):
-    """The Windows fallback retries the non-blocking lock until it succeeds."""
-    attempts = {"n": 0}
-
-    def fake_locking(fd, mode, nbytes):
-        attempts["n"] += 1
-        if attempts["n"] < 3:
-            raise OSError("locked")  # contention on the first two tries
-
-    fake_msvcrt = types.SimpleNamespace(
-        LK_LOCK=1, LK_NBLCK=2, LK_UNLCK=0, locking=fake_locking
-    )
-    monkeypatch.setattr(locks, "fcntl", None)
-    monkeypatch.setattr(locks, "_WINDOWS_LOCK_TIMEOUT", 5.0)
-    monkeypatch.setitem(sys.modules, "msvcrt", fake_msvcrt)
-
-    with (tmp_path / "test.lock").open("a+", encoding="utf-8") as fh:
-        locks.flock(fh, exclusive=True)
-
-    assert attempts["n"] == 3  # retried twice, succeeded on the third
-
-
-def test_flock_raises_after_timeout(monkeypatch, tmp_path):
-    """A never-released Windows lock surfaces an error instead of hanging forever."""
-    def always_locked(fd, mode, nbytes):
-        raise OSError("locked")
-
-    fake_msvcrt = types.SimpleNamespace(
-        LK_LOCK=1, LK_NBLCK=2, LK_UNLCK=0, locking=always_locked
-    )
-    monkeypatch.setattr(locks, "fcntl", None)
-    monkeypatch.setattr(locks, "_WINDOWS_LOCK_TIMEOUT", 0.2)
-    monkeypatch.setitem(sys.modules, "msvcrt", fake_msvcrt)
-
-    with (tmp_path / "test.lock").open("a+", encoding="utf-8") as fh:
-        with pytest.raises(OSError):
-            locks.flock(fh, exclusive=True)
+        fh.close()
 
 
 def test_atomic_write_bytes_without_fchmod(monkeypatch, tmp_path):