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

* refactor(locks): delegate file locking to portalocker

Replace the hand-rolled fcntl/msvcrt flock/funlock (merged in #99) with
portalocker, which is fcntl-backed on POSIX and msvcrt/Win32-backed on Windows
with maintained, cross-platform-tested behaviour. Removes the hand-written
Windows retry/timeout loop that could not be exercised on POSIX.

- flock/funlock now delegate to portalocker.lock/unlock.
- Drop the guarded 'import fcntl', the msvcrt fallback, and _WINDOWS_LOCK_TIMEOUT.
- Keep the os.fchmod guard and Windows directory-fsync skip (atomic writes,
  which portalocker does not cover).
- Pin portalocker==3.2.0 (BSD-3) to match the exact-pin dependency policy.

Note: true shared (reader) locks on Windows would still need pywin32; without
it portalocker uses msvcrt (exclusive). Not added — in-process concurrent KB
reads are rare. Refs #93.

Tests: swap the msvcrt-internals tests for a cross-process exclusion test that
verifies flock takes a real OS lock, plus the retained atomic-write/fsync-skip
guards. Full suite 749 passed.

* fix(locks): review fixes — accurate docs, fcntl import guard, test hardening

Addresses /code-review findings on the portalocker refactor:

- flock docstring corrected: portalocker uses Win32 LockFileEx (pywin32, pulled
  in automatically on Windows) for SHARED locks, so concurrent readers ARE
  honoured; EXCLUSIVE uses msvcrt (retries ~10s then raises, not an infinite
  block); failures raise portalocker.LockException, not OSError.
- Re-add the issue #93 regression guard: assert no openkb module hard-imports
  the Unix-only fcntl at module level (replaces the dropped import-without-fcntl
  test without depending on portalocker internals).
- Strengthen the cross-process lock test: assert both BLOCKED (while held) and
  ACQUIRED (after release), and check the probe's exit code so an ImportError
  surfaces clearly instead of an empty-stdout false failure.
- Drop the now-dead 'import pytest' / 'import portalocker' from the test module.

* test(locks): resolve code-quality bot nits — single openkb import style

Drop the unused 'import openkb' and derive the package dir from
locks.__file__ instead, so the test module uses one import style for openkb
(github-code-quality bot). The unused 'import portalocker' was already removed
in the prior commit.

Author: KylinMountain

openkb/locks.py

@@ -2,82 +2,42 @@
 
 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`:
+
+    - **POSIX** — ``fcntl.flock``; the call blocks indefinitely until acquired.
+    - **Windows** — shared locks use the Win32 ``LockFileEx`` API (``pywin32``,
+      which portalocker pulls in automatically on Windows), so concurrent
+      readers are honoured; exclusive locks use ``msvcrt.locking``, which
+      retries for ~10s and then raises rather than blocking indefinitely.
+
+    On failure portalocker raises :class:`portalocker.LockException` — note this
+    is *not* an ``OSError`` (e.g. on filesystems without working lock support).
     """
-    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,83 @@
 """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 verifiable on
+POSIX; portalocker carries its own Windows test coverage.
 """
 from __future__ import annotations
 
+import ast
 import os
 import subprocess
 import sys
-import types
-
-import pytest
+from pathlib import Path
 
 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 _module_level_imports_fcntl(path: Path) -> bool:
+    """True if the module has a top-level ``import fcntl`` / ``from fcntl import``."""
+    tree = ast.parse(path.read_text(encoding="utf-8"))
+    for node in tree.body:  # module-level statements only (import-time crash risk)
+        if isinstance(node, ast.Import) and any(a.name == "fcntl" for a in node.names):
+            return True
+        if isinstance(node, ast.ImportFrom) and node.module == "fcntl":
+            return True
+    return False
 
-def test_flock_funlock_roundtrip(tmp_path):
-    """flock/funlock acquire and release an advisory lock on the real platform."""
-    lock_path = tmp_path / "test.lock"
-    with lock_path.open("a+", encoding="utf-8") as fh:
-        locks.flock(fh, exclusive=True)
-        locks.funlock(fh)  # must not raise
 
+def test_openkb_modules_do_not_hard_import_fcntl():
+    """Guards issue #93: OpenKB's own modules must import on Windows (no bare fcntl)."""
+    pkg_dir = Path(locks.__file__).parent  # locks.py lives in the openkb package
+    offenders = [
+        str(py.relative_to(pkg_dir))
+        for py in pkg_dir.rglob("*.py")
+        if _module_level_imports_fcntl(py)
+    ]
+    assert not offenders, f"Unix-only fcntl hard-imported at module level in: {offenders}"
 
-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_funlock_roundtrip(tmp_path):
+    """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
 
-    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
+def test_flock_exclusive_excludes_other_process(tmp_path):
+    """An exclusive flock is a real OS lock: it excludes another process while
+    held, and the lock is acquirable again once released."""
+    lock_path = tmp_path / "test.lock"
+    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"
     )
-    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)
+    def run_probe() -> str:
+        result = subprocess.run(
+            [sys.executable, "-c", probe], capture_output=True, text=True
+        )
+        assert result.returncode == 0, result.stderr  # probe itself ran cleanly
+        return result.stdout.strip()
+
+    fh = lock_path.open("a+", encoding="utf-8")
+    locks.flock(fh, exclusive=True)
+    try:
+        assert run_probe() == "BLOCKED"  # held → other process is excluded
+    finally:
+        locks.funlock(fh)
+        fh.close()
+    assert run_probe() == "ACQUIRED"  # released → other process can acquire
 
 
 def test_atomic_write_bytes_without_fchmod(monkeypatch, tmp_path):