Skip to content

Commit 10c63bd

Browse files
committed
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
1 parent 06a65c5 commit 10c63bd

3 files changed

Lines changed: 131 additions & 10 deletions

File tree

openkb/config.py

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
from __future__ import annotations
22

33
import contextlib
4-
import fcntl
54
import logging
65
import re
76
from pathlib import Path
87
from typing import Any, Iterator
98

109
import yaml
1110

12-
from openkb.locks import atomic_write_text
11+
from openkb.locks import atomic_write_text, flock, funlock
1312

1413
logger = logging.getLogger(__name__)
1514

@@ -34,11 +33,11 @@
3433
def _with_global_config_lock() -> Iterator[None]:
3534
GLOBAL_CONFIG_DIR.mkdir(parents=True, exist_ok=True)
3635
with GLOBAL_CONFIG_LOCK_PATH.open("a+", encoding="utf-8") as fh:
37-
fcntl.flock(fh.fileno(), fcntl.LOCK_EX)
36+
flock(fh, exclusive=True)
3837
try:
3938
yield
4039
finally:
41-
fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
40+
funlock(fh)
4241

4342

4443
def _atomic_yaml_dump(path: Path, config: dict[str, Any]) -> None:

openkb/locks.py

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,50 @@
77
from __future__ import annotations
88

99
import contextlib
10-
import fcntl
1110
import json
1211
import os
1312
import tempfile
1413
import threading
14+
import time
1515
from pathlib import Path
16-
from typing import Iterator
16+
from typing import IO, Iterator
17+
18+
try:
19+
import fcntl
20+
except ImportError: # pragma: no cover - Windows has no fcntl (simulated in tests)
21+
fcntl = None
22+
23+
24+
def flock(fh: IO, *, exclusive: bool) -> None:
25+
"""Acquire an advisory lock on an open file handle (cross-platform).
26+
27+
Uses ``fcntl.flock`` on POSIX. On Windows (no ``fcntl``) it falls back to
28+
``msvcrt.locking``, which provides only exclusive byte-range locks — shared
29+
requests are taken exclusively (over-locking is safe) — and the blocking
30+
behaviour of ``fcntl.flock`` is emulated by retrying the non-blocking lock.
31+
"""
32+
if fcntl is not None:
33+
fcntl.flock(fh.fileno(), fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH)
34+
return
35+
import msvcrt
36+
fh.seek(0)
37+
while True:
38+
try:
39+
msvcrt.locking(fh.fileno(), msvcrt.LK_NBLCK, 1)
40+
return
41+
except OSError:
42+
time.sleep(0.1)
43+
44+
45+
def funlock(fh: IO) -> None:
46+
"""Release a lock previously acquired with :func:`flock`."""
47+
if fcntl is not None:
48+
fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
49+
return
50+
import msvcrt
51+
fh.seek(0)
52+
msvcrt.locking(fh.fileno(), msvcrt.LK_UNLCK, 1)
53+
1754

1855
_LOCKS_GUARD = threading.Lock()
1956
_LOCAL_LOCKS: dict[Path, "_LocalRwLock"] = {}
@@ -106,14 +143,13 @@ def kb_lock(openkb_dir: Path, *, exclusive: bool) -> Iterator[None]:
106143
local_context = local_lock.write() if exclusive else local_lock.read()
107144
with local_context:
108145
with lock_path.open("a+", encoding="utf-8") as fh:
109-
mode = fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH
110-
fcntl.flock(fh.fileno(), mode)
146+
flock(fh, exclusive=exclusive)
111147
held[resolved] = (1, 0) if exclusive else (0, 1)
112148
try:
113149
yield
114150
finally:
115151
held.pop(resolved, None)
116-
fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
152+
funlock(fh)
117153

118154

119155
def kb_ingest_lock(openkb_dir: Path):
@@ -127,6 +163,10 @@ def kb_read_lock(openkb_dir: Path):
127163

128164

129165
def _fsync_directory(path: Path) -> None:
166+
if os.name == "nt":
167+
# Windows cannot open a directory handle to fsync it; os.replace is
168+
# already atomic on NTFS, so the parent-directory flush is a no-op.
169+
return
130170
fd = os.open(path, os.O_RDONLY)
131171
try:
132172
os.fsync(fd)
@@ -154,7 +194,8 @@ def atomic_write_bytes(path: Path, content: bytes) -> None:
154194
tmp_path = Path(tmp_name)
155195
try:
156196
with os.fdopen(fd, "wb") as fh:
157-
os.fchmod(fh.fileno(), _target_mode(path))
197+
if hasattr(os, "fchmod"): # not available on Windows
198+
os.fchmod(fh.fileno(), _target_mode(path))
158199
fh.write(content)
159200
fh.flush()
160201
os.fsync(fh.fileno())

tests/test_cross_platform_locks.py

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
"""Cross-platform behaviour for openkb.locks / openkb.config.
2+
3+
The locking layer (#86) originally hard-imported ``fcntl`` and called
4+
``os.fchmod`` / directory ``os.fsync`` unconditionally — all Unix-only — which
5+
crashed OpenKB at import time on Windows (``ModuleNotFoundError: No module
6+
named 'fcntl'``, reported in VectifyAI/OpenKB#93). These tests pin the
7+
platform-neutral behaviour and simulate the Windows path on this host.
8+
"""
9+
from __future__ import annotations
10+
11+
import os
12+
import subprocess
13+
import sys
14+
import types
15+
16+
import pytest
17+
18+
from openkb import locks
19+
20+
21+
def test_config_and_locks_import_without_fcntl():
22+
"""openkb.config / openkb.locks must import on a host without fcntl (Windows)."""
23+
code = (
24+
"import sys\n"
25+
"sys.modules['fcntl'] = None\n" # make `import fcntl` raise ImportError
26+
"import openkb.locks, openkb.config\n"
27+
"assert openkb.locks.fcntl is None\n"
28+
"print('OK')\n"
29+
)
30+
result = subprocess.run(
31+
[sys.executable, "-c", code], capture_output=True, text=True
32+
)
33+
assert result.returncode == 0, result.stderr
34+
assert "OK" in result.stdout
35+
36+
37+
def test_flock_funlock_roundtrip(tmp_path):
38+
"""flock/funlock acquire and release an advisory lock on the real platform."""
39+
lock_path = tmp_path / "test.lock"
40+
with lock_path.open("a+", encoding="utf-8") as fh:
41+
locks.flock(fh, exclusive=True)
42+
locks.funlock(fh) # must not raise
43+
44+
45+
def test_flock_uses_msvcrt_when_fcntl_absent(monkeypatch, tmp_path):
46+
"""When fcntl is unavailable (Windows), locking is delegated to msvcrt."""
47+
calls = []
48+
fake_msvcrt = types.SimpleNamespace(
49+
LK_LOCK=1, LK_NBLCK=2, LK_UNLCK=0,
50+
locking=lambda fd, mode, nbytes: calls.append((mode, nbytes)),
51+
)
52+
monkeypatch.setattr(locks, "fcntl", None)
53+
monkeypatch.setitem(sys.modules, "msvcrt", fake_msvcrt)
54+
55+
lock_path = tmp_path / "test.lock"
56+
with lock_path.open("a+", encoding="utf-8") as fh:
57+
locks.flock(fh, exclusive=True)
58+
locks.funlock(fh)
59+
60+
modes = [mode for mode, _ in calls]
61+
assert fake_msvcrt.LK_NBLCK in modes # acquire used the non-blocking lock
62+
assert fake_msvcrt.LK_UNLCK in modes # release unlocked
63+
64+
65+
def test_atomic_write_bytes_without_fchmod(monkeypatch, tmp_path):
66+
"""atomic_write_bytes must still work where os.fchmod is missing (Windows)."""
67+
monkeypatch.delattr(os, "fchmod", raising=False)
68+
target = tmp_path / "data.bin"
69+
locks.atomic_write_bytes(target, b"hello")
70+
assert target.read_bytes() == b"hello"
71+
72+
73+
def test_fsync_directory_skipped_on_windows(monkeypatch, tmp_path):
74+
"""Directory fsync (unsupported on Windows) must be skipped, not attempted."""
75+
monkeypatch.setattr(os, "name", "nt")
76+
77+
def _no_open(*args, **kwargs):
78+
raise AssertionError("os.open must not be called for dir fsync on Windows")
79+
80+
monkeypatch.setattr(os, "open", _no_open)
81+
locks._fsync_directory(tmp_path) # must return without touching os.open

0 commit comments

Comments
 (0)