fix(locks): cross-platform file locking — fix Windows fcntl crash (#99)

* fix(locks): make file locking cross-platform (Windows support)

openkb/locks.py and openkb/config.py hard-imported fcntl and called
os.fchmod / directory os.fsync unconditionally — all Unix-only — so OpenKB
crashed at import on Windows (ModuleNotFoundError: No module named 'fcntl'),
surfaced in #93 once the Copilot extra_headers fix (#98) unblocked that user.

- locks.flock/funlock: advisory-lock helpers — fcntl on POSIX, msvcrt
  byte-range locks on Windows (exclusive-only; shared degrades to exclusive,
  fcntl's blocking acquire emulated via non-blocking retry).
- guard os.fchmod with hasattr; skip directory fsync on Windows
  (os.replace is already atomic on NTFS).
- config.py drops its direct fcntl import and uses locks.flock/funlock.

Adds tests/test_cross_platform_locks.py: simulates the no-fcntl (Windows)
path on POSIX via subprocess + a faked msvcrt.

Refs #93

* fix(locks): harden Windows lock-acquire loop (code review)

Addresses review findings on the msvcrt fallback:

- flock's Windows retry loop no longer spins forever: bound the wait by
  _WINDOWS_LOCK_TIMEOUT (default 3600s, OPENKB_LOCK_TIMEOUT override) so a
  stuck/never-released lock or a non-contention OSError surfaces as an error
  instead of an infinite, silent busy-loop. Add exponential backoff (was a
  fixed 100ms spin) and a one-time 'still waiting' warning.
- Document that shared locks degrade to exclusive on Windows (msvcrt has no
  shared mode), so concurrent in-process readers serialise there.
- Correct the _fsync_directory comment to not conflate NTFS atomicity with
  crash durability.

Tests: cover the retry-until-available and raise-after-timeout paths (the
previously-uncovered msvcrt contention logic), runnable on POSIX via a faked
msvcrt. Full suite 752 passed.

Author: KylinMountain

openkb/config.py

@@ -1,15 +1,14 @@
 from __future__ import annotations
 
 import contextlib
-import fcntl
 import logging
 import re
 from pathlib import Path
 from typing import Any, Iterator
 
 import yaml
 
-from openkb.locks import atomic_write_text
+from openkb.locks import atomic_write_text, flock, funlock
 
 logger = logging.getLogger(__name__)
 
@@ -34,11 +33,11 @@
 def _with_global_config_lock() -> Iterator[None]:
     GLOBAL_CONFIG_DIR.mkdir(parents=True, exist_ok=True)
     with GLOBAL_CONFIG_LOCK_PATH.open("a+", encoding="utf-8") as fh:
-        fcntl.flock(fh.fileno(), fcntl.LOCK_EX)
+        flock(fh, exclusive=True)
         try:
             yield
         finally:
-            fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
+            funlock(fh)
 
 
 def _atomic_yaml_dump(path: Path, config: dict[str, Any]) -> None:

openkb/locks.py

@@ -7,13 +7,78 @@
 from __future__ import annotations
 
 import contextlib
-import fcntl
 import json
+import logging
 import os
 import tempfile
 import threading
+import time
 from pathlib import Path
-from typing import Iterator
+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"))
+
+
+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.
+    """
+    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)
+
+
+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)
+
 
 _LOCKS_GUARD = threading.Lock()
 _LOCAL_LOCKS: dict[Path, "_LocalRwLock"] = {}
@@ -106,14 +171,13 @@ def kb_lock(openkb_dir: Path, *, exclusive: bool) -> Iterator[None]:
     local_context = local_lock.write() if exclusive else local_lock.read()
     with local_context:
         with lock_path.open("a+", encoding="utf-8") as fh:
-            mode = fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH
-            fcntl.flock(fh.fileno(), mode)
+            flock(fh, exclusive=exclusive)
             held[resolved] = (1, 0) if exclusive else (0, 1)
             try:
                 yield
             finally:
                 held.pop(resolved, None)
-                fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
+                funlock(fh)
 
 
 def kb_ingest_lock(openkb_dir: Path):
@@ -127,6 +191,11 @@ def kb_read_lock(openkb_dir: Path):
 
 
 def _fsync_directory(path: Path) -> None:
+    if os.name == "nt":
+        # Windows cannot open a directory handle to fsync it. os.replace is
+        # atomic on NTFS (no torn/partial state), though without the dir flush
+        # the rename's durability across a crash is weaker than on POSIX.
+        return
     fd = os.open(path, os.O_RDONLY)
     try:
         os.fsync(fd)
@@ -154,7 +223,8 @@ def atomic_write_bytes(path: Path, content: bytes) -> None:
     tmp_path = Path(tmp_name)
     try:
         with os.fdopen(fd, "wb") as fh:
-            os.fchmod(fh.fileno(), _target_mode(path))
+            if hasattr(os, "fchmod"):  # not available on Windows
+                os.fchmod(fh.fileno(), _target_mode(path))
             fh.write(content)
             fh.flush()
             os.fsync(fh.fileno())

tests/test_cross_platform_locks.py

@@ -0,0 +1,120 @@
+"""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.
+"""
+from __future__ import annotations
+
+import os
+import subprocess
+import sys
+import types
+
+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."""
+    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_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)
+
+    lock_path = tmp_path / "test.lock"
+    with lock_path.open("a+", encoding="utf-8") as fh:
+        locks.flock(fh, exclusive=True)
+        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)
+
+
+def test_atomic_write_bytes_without_fchmod(monkeypatch, tmp_path):
+    """atomic_write_bytes must still work where os.fchmod is missing (Windows)."""
+    monkeypatch.delattr(os, "fchmod", raising=False)
+    target = tmp_path / "data.bin"
+    locks.atomic_write_bytes(target, b"hello")
+    assert target.read_bytes() == b"hello"
+
+
+def test_fsync_directory_skipped_on_windows(monkeypatch, tmp_path):
+    """Directory fsync (unsupported on Windows) must be skipped, not attempted."""
+    monkeypatch.setattr(os, "name", "nt")
+
+    def _no_open(*args, **kwargs):
+        raise AssertionError("os.open must not be called for dir fsync on Windows")
+
+    monkeypatch.setattr(os, "open", _no_open)
+    locks._fsync_directory(tmp_path)  # must return without touching os.open