feat(robot): implement cross-platform file locking for data cache

d-biehl · d-biehl · commit 8ae4aec35048 · 2026-04-17T22:30:49.000+02:00
Replace the Unix-only flock-based locking with a platform-dispatched
implementation using LockFileEx/UnlockFileEx (Windows) and fcntl.flock
(Unix/macOS). The Windows path previously had no-op early returns,
meaning the exclusive lock used by `cache prune` provided no actual
protection on Windows.
diff --git a/packages/robot/src/robotcode/robot/diagnostics/data_cache.py b/packages/robot/src/robotcode/robot/diagnostics/data_cache.py
@@ -10,6 +10,13 @@
 
 from ..utils import get_robot_version_str
 
+if sys.platform == "win32":
+    import ctypes
+    import msvcrt
+    from ctypes import wintypes
+else:
+    import fcntl
+
 _M = TypeVar("_M")
 _D = TypeVar("_D")
 
@@ -83,23 +90,103 @@ def data(self) -> _D:
 
 CACHE_DIR_NAME = ".robotcode_cache"
 _LOCK_FILE_NAME = "cache.lock"
+_LOCK_LENGTH = 1
+
+
+if sys.platform == "win32":
+    _LOCKFILE_FAIL_IMMEDIATELY = 0x00000001
+    _LOCKFILE_EXCLUSIVE_LOCK = 0x00000002
+    _ERROR_LOCK_VIOLATION = 33
+    _ERROR_IO_PENDING = 997
+
+    class _Overlapped(ctypes.Structure):
+        _fields_ = [
+            ("Internal", ctypes.c_void_p),
+            ("InternalHigh", ctypes.c_void_p),
+            ("Offset", wintypes.DWORD),
+            ("OffsetHigh", wintypes.DWORD),
+            ("hEvent", wintypes.HANDLE),
+        ]
 
+    _kernel32 = ctypes.WinDLL("kernel32", use_last_error=True)
+    _kernel32.LockFileEx.argtypes = [
+        wintypes.HANDLE,
+        wintypes.DWORD,
+        wintypes.DWORD,
+        wintypes.DWORD,
+        wintypes.DWORD,
+        ctypes.POINTER(_Overlapped),
+    ]
+    _kernel32.LockFileEx.restype = wintypes.BOOL
+    _kernel32.UnlockFileEx.argtypes = [
+        wintypes.HANDLE,
+        wintypes.DWORD,
+        wintypes.DWORD,
+        wintypes.DWORD,
+        ctypes.POINTER(_Overlapped),
+    ]
+    _kernel32.UnlockFileEx.restype = wintypes.BOOL
+
+
+def _open_lock_file(cache_dir: Path) -> int:
+    lock_path = cache_dir / _LOCK_FILE_NAME
+    return os.open(str(lock_path), os.O_RDWR | os.O_CREAT, 0o666)
 
-def _acquire_shared_lock(cache_dir: Path) -> Optional[int]:
-    """Acquire a shared advisory lock on the cache directory.
 
-    Returns the file descriptor on Unix, None on Windows
-    (Windows already prevents deletion of open files).
-    """
-    if sys.platform == "win32":
-        return None
+if sys.platform == "win32":
 
-    import fcntl
+    def _lock_file(fd: int, *, exclusive: bool, blocking: bool) -> bool:
+        flags = 0
+        if exclusive:
+            flags |= _LOCKFILE_EXCLUSIVE_LOCK
+        if not blocking:
+            flags |= _LOCKFILE_FAIL_IMMEDIATELY
 
-    lock_path = cache_dir / _LOCK_FILE_NAME
-    fd = os.open(str(lock_path), os.O_RDWR | os.O_CREAT, 0o666)
+        overlapped = _Overlapped()
+        handle = wintypes.HANDLE(msvcrt.get_osfhandle(fd))
+        result = _kernel32.LockFileEx(handle, flags, 0, _LOCK_LENGTH, 0, ctypes.byref(overlapped))
+        if result:
+            return True
+
+        error = ctypes.get_last_error()
+        if not blocking and error in {_ERROR_LOCK_VIOLATION, _ERROR_IO_PENDING}:
+            return False
+
+        raise ctypes.WinError(error)
+
+    def _unlock_file(fd: int) -> None:
+        overlapped = _Overlapped()
+        handle = wintypes.HANDLE(msvcrt.get_osfhandle(fd))
+        result = _kernel32.UnlockFileEx(handle, 0, _LOCK_LENGTH, 0, ctypes.byref(overlapped))
+        if not result:
+            raise ctypes.WinError(ctypes.get_last_error())
+
+
+else:
+
+    def _lock_file(fd: int, *, exclusive: bool, blocking: bool) -> bool:
+        flags = fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH
+        if not blocking:
+            flags |= fcntl.LOCK_NB
+
+        try:
+            fcntl.flock(fd, flags)
+        except OSError:
+            if not blocking:
+                return False
+            raise
+
+        return True
+
+    def _unlock_file(fd: int) -> None:
+        fcntl.flock(fd, fcntl.LOCK_UN)
+
+
+def _acquire_shared_lock(cache_dir: Path) -> Optional[int]:
+    """Acquire a shared lock on the cache directory."""
+    fd = _open_lock_file(cache_dir)
     try:
-        fcntl.flock(fd, fcntl.LOCK_SH)
+        _lock_file(fd, exclusive=False, blocking=True)
     except Exception:
         os.close(fd)
         raise
@@ -111,10 +198,7 @@ def _release_lock(fd: Optional[int]) -> None:
     if fd is None:
         return
     try:
-        if sys.platform != "win32":
-            import fcntl
-
-            fcntl.flock(fd, fcntl.LOCK_UN)
+        _unlock_file(fd)
     finally:
         os.close(fd)
 
@@ -127,26 +211,20 @@ def exclusive_cache_lock(cache_dir: Path) -> Iterator[bool]:
     False if another process holds a shared lock (cache is in use).
     The lock is held until the context manager exits.
     """
-    if sys.platform == "win32":
-        yield True
-        return
-
-    import fcntl
-
     lock_path = cache_dir / _LOCK_FILE_NAME
     if not lock_path.exists():
         yield True
         return
 
     fd = os.open(str(lock_path), os.O_RDWR)
+    acquired = False
     try:
-        fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
-        yield True
-    except OSError:
-        yield False
+        acquired = _lock_file(fd, exclusive=True, blocking=False)
+        yield acquired
     finally:
         try:
-            fcntl.flock(fd, fcntl.LOCK_UN)
+            if acquired:
+                _unlock_file(fd)
         except OSError:
             pass
         os.close(fd)
diff --git a/tests/robotcode/robot/diagnostics/test_data_cache_locking.py b/tests/robotcode/robot/diagnostics/test_data_cache_locking.py
@@ -1,5 +1,6 @@
-"""Tests for SqliteDataCache advisory file locking."""
+"""Tests for SqliteDataCache file locking."""
 
+import os
 import sys
 from pathlib import Path
 
@@ -12,8 +13,6 @@
     exclusive_cache_lock,
 )
 
-pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="Advisory file locking is Unix-only")
-
 
 class TestSharedLocking:
     def test_lock_file_created_on_open(self, tmp_path: Path) -> None:
@@ -27,7 +26,6 @@ def test_two_caches_can_open_same_dir(self, tmp_path: Path) -> None:
         cache1 = SqliteDataCache(cache_dir)
         cache2 = SqliteDataCache(cache_dir)
 
-        # Both can read and write
         cache1.save_entry(CacheSection.LIBRARY, "e1", None, "data1")
         cache2.save_entry(CacheSection.LIBRARY, "e2", None, "data2")
 
@@ -67,13 +65,11 @@ def test_exclusive_blocked_by_multiple_shared(self, tmp_path: Path) -> None:
 
         cache1.close()
 
-        # Still blocked by cache2
         with exclusive_cache_lock(cache_dir) as acquired:
             assert not acquired
 
         cache2.close()
 
-        # Now should succeed
         with exclusive_cache_lock(cache_dir) as acquired:
             assert acquired
 
@@ -88,7 +84,6 @@ def test_exclusive_succeeds_when_no_cache_open(self, tmp_path: Path) -> None:
     def test_exclusive_succeeds_when_no_lock_file(self, tmp_path: Path) -> None:
         cache_dir = tmp_path / "cache"
         cache_dir.mkdir(parents=True)
-        # No lock file exists
 
         with exclusive_cache_lock(cache_dir) as acquired:
             assert acquired
@@ -100,19 +95,18 @@ def test_exclusive_lock_held_during_context(self, tmp_path: Path) -> None:
 
         with exclusive_cache_lock(cache_dir) as acquired:
             assert acquired
-            # While we hold exclusive, a new shared lock should block
-            # (opening a new SqliteDataCache would try LOCK_SH which
-            # is compatible with LOCK_EX held by the same process on Linux,
-            # so we test with a raw flock instead)
-            import fcntl
-            import os
+            if sys.platform != "win32":
+                import fcntl
 
-            fd = os.open(str(cache_dir / _LOCK_FILE_NAME), os.O_RDWR)
-            try:
-                with pytest.raises(BlockingIOError):
-                    fcntl.flock(fd, fcntl.LOCK_SH | fcntl.LOCK_NB)
-            finally:
-                os.close(fd)
+                fd = os.open(str(cache_dir / _LOCK_FILE_NAME), os.O_RDWR)
+                try:
+                    with pytest.raises(BlockingIOError):
+                        fcntl.flock(fd, fcntl.LOCK_SH | fcntl.LOCK_NB)
+                finally:
+                    os.close(fd)
+            else:
+                with exclusive_cache_lock(cache_dir) as nested_acquired:
+                    assert not nested_acquired
 
 
 class TestLockingWithPrune:
@@ -126,7 +120,8 @@ def test_prune_safe_when_cache_closed(self, tmp_path: Path) -> None:
 
         with exclusive_cache_lock(cache_dir) as acquired:
             assert acquired
-            shutil.rmtree(cache_dir)
+
+        shutil.rmtree(cache_dir)
 
         assert not cache_dir.exists()
 
@@ -136,7 +131,6 @@ def test_prune_blocked_when_cache_open(self, tmp_path: Path) -> None:
 
         with exclusive_cache_lock(cache_dir) as acquired:
             assert not acquired
-            # Cache dir should still exist
             assert cache_dir.exists()
 
         cache.close()
