fix(locks): cross-platform file locking — fix Windows fcntl crash - #99
Merged
Conversation
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
![github-code-quality[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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.
KylinMountain
added a commit
that referenced
this pull request
Jun 15, 2026
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openkb/locks.pyandopenkb/config.pyhard-importedfcntland calledos.fchmod/ directoryos.fsyncunconditionally — all Unix-only — so OpenKB crashed at import time on Windows:This is a Windows-only regression from the KB-locks feature (#86). It surfaced in #93 once the Copilot
extra_headersfix (#98) let that (Windows) user get past the header error.Changes
locks.flock/locks.funlock— advisory-lock helpers. POSIX usesfcntl.flock; Windows falls back tomsvcrt.lockingbyte-range locks (exclusive-only — shared requests degrade to exclusive, which is safe — andfcntl.flock's blocking acquire is emulated via a non-blocking retry).os.fchmodwithhasattr(absent on Windows).fsyncon Windows (can't fsync a directory handle;os.replaceis already atomic on NTFS).config.pydrops its directfcntlimport and useslocks.flock/funlock.Testing
tests/test_cross_platform_locks.pysimulates the no-fcntl(Windows) path on POSIX — a subprocess reproduces the import crash, and a fakedmsvcrtexercises the fallback dispatch.Refs #93