fix(compress): use OS-native file locking instead of a hand-rolled marker file

The O_CREAT|O_EXCL marker-file lock had no way to distinguish a crashed
holder from a slow-but-live one: a run exceeding the 10-minute staleness
window could have its lock stolen mid-write by a waiter, reintroducing the
exact interleaved-write race this feature exists to close. It also had a
busy-loop bug: a failed unlink during staleness reclaim was silently
treated as success, skipping the timeout check and sleep entirely.

Replace it with fcntl.flock (POSIX) / msvcrt.locking (Windows), both
stdlib. The OS releases the lock automatically when the holding process
dies or its file descriptor closes, so there's no staleness heuristic left
to get wrong. lock_path_for now resolves its own input and no longer
mkdirs as a side effect of computing a path; that mkdir moved into
file_lock, the only place that needs the directory to exist.

Rewrote tests/test_compress_concurrency.py for the new semantics: the
stale-lock-reclaim test is replaced by one that closes a locking fd to
simulate a crash and confirms the OS releases it immediately, and the
lock-path identity test now compares a relative and a resolved spelling
of the same file instead of comparing a function's output to itself.

Author: hunleyd

skills/caveman-compress/scripts/compress.py

@@ -19,6 +19,13 @@
 from pathlib import Path
 from typing import List
 
+_IS_WINDOWS = os.name == "nt" or sys.platform == "win32"
+
+if _IS_WINDOWS:
+    import msvcrt
+else:
+    import fcntl
+
 OUTER_FENCE_REGEX = re.compile(
     r"\A\s*(`{3,}|~{3,})[^\n]*\n(.*)\n\1\s*\Z", re.DOTALL
 )
@@ -72,14 +79,8 @@ def split_frontmatter(text: str):
 
 
 def _state_base_dir(kind: str) -> Path:
-    """Resolve the platform-aware base dir for caveman-compress's own state
-    (backups, locks). Shared by every state kind so the platform-detection
-    logic (Windows vs XDG) lives in one place.
-      - Windows: %LOCALAPPDATA%\\caveman-compress\\<kind>
-      - else:    $XDG_DATA_HOME/caveman-compress/<kind> if set,
-                 else ~/.local/share/caveman-compress/<kind>
-    """
-    if os.name == "nt" or sys.platform == "win32":
+    """Shared platform-aware base dir for caveman-compress state (backups, locks) — Windows uses %LOCALAPPDATA%, else $XDG_DATA_HOME or ~/.local/share."""
+    if _IS_WINDOWS:
         local_appdata = os.environ.get("LOCALAPPDATA")
         base = Path(local_appdata) if local_appdata else Path.home() / "AppData" / "Local"
     else:
@@ -89,18 +90,10 @@ def _state_base_dir(kind: str) -> Path:
 
 
 def backup_dir_for(filepath: Path) -> Path:
-    """Resolve the out-of-tree backup directory for a given source file.
-
-    Backups must live OUTSIDE the source directory so skill auto-loaders
-    (Claude Code rules/, opencode instructions/, etc.) stop re-ingesting the
-    `.original.md` copies as live files. The source file's parent-dir name is
-    mirrored under the base to reduce cross-project collisions (e.g. two
-    `task.md` files in different repos).
-    """
+    """Out-of-tree backup dir for filepath, keyed by its parent dir name — kept outside the source tree so skill auto-loaders don't re-ingest `.original.md` backups as live files."""
     return _state_base_dir("backups") / filepath.parent.name
 
 
-LOCK_STALE_SECONDS = 10 * 60  # crashed/killed process: long enough to outlast a slow Claude call + one retry, short enough not to wedge a repo indefinitely
 LOCK_WAIT_SECONDS = 120  # how long to wait for another session's in-progress compress before giving up
 LOCK_POLL_INTERVAL = 1.0
 
@@ -110,71 +103,62 @@ class LockTimeoutError(RuntimeError):
 
 
 def lock_path_for(filepath: Path) -> Path:
-    """Resolve the cross-session lock file path for a given (resolved) source file.
+    """Cross-session lock path for filepath's resolved form, keyed by a hash of the full path (not basename) so same-named files in different repos never contend for the same lock."""
+    digest = hashlib.sha256(str(filepath.resolve()).encode("utf-8")).hexdigest()[:16]
+    return _state_base_dir("locks") / f"{filepath.stem}-{digest}.lock"
 
-    Keyed by a hash of the full resolved path, not just the basename, so two
-    different files that happen to share a name (e.g. two `task.md` in
-    different repos) never contend for the same lock, while two concurrent
-    compress runs against the *same* file always do.
-    """
-    digest = hashlib.sha256(str(filepath).encode("utf-8")).hexdigest()[:16]
-    lock_dir = _state_base_dir("locks")
-    lock_dir.mkdir(parents=True, exist_ok=True)
-    return lock_dir / f"{filepath.stem}-{digest}.lock"
 
+def _try_lock_nonblocking(fd: int) -> None:
+    """Attempt the OS-native exclusive lock on fd; raises BlockingIOError if another process already holds it."""
+    if _IS_WINDOWS:
+        try:
+            msvcrt.locking(fd, msvcrt.LK_NBLCK, 1)
+        except OSError as e:
+            raise BlockingIOError(str(e)) from e
+    else:
+        fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
 
-def _steal_stale_lock(lock_path: Path) -> bool:
-    """Remove lock_path if it's older than LOCK_STALE_SECONDS (abandoned by a
-    crashed or killed process). Returns True if the lock is now gone."""
+
+def _unlock(fd: int) -> None:
+    """Release the OS-native lock on fd; swallows errors since callers use this in a finally block."""
     try:
-        age = time.time() - lock_path.stat().st_mtime
+        if _IS_WINDOWS:
+            msvcrt.locking(fd, msvcrt.LK_UNLCK, 1)
+        else:
+            fcntl.flock(fd, fcntl.LOCK_UN)
     except OSError:
-        return True  # already gone (e.g. the lock holder released it between our failed open and this stat)
-    if age > LOCK_STALE_SECONDS:
-        try:
-            lock_path.unlink()
-        except OSError:
-            pass
-        return True
-    return False
+        pass
 
 
 @contextlib.contextmanager
 def file_lock(filepath: Path):
-    """Cross-session, cross-platform exclusive lock keyed on ``filepath``.
-
-    Two Claude Code sessions calling compress_file on the same target
-    concurrently would otherwise interleave reads/writes of the same file —
-    one session's finished edit can get silently clobbered by the other's
-    compression pass, or the two writes can interleave into corrupt content.
-    ``os.O_CREAT | os.O_EXCL`` is an atomic "create only if absent" on both
-    POSIX and Windows, so it works as a lock with no extra dependency.
-    """
+    """Cross-session exclusive lock on filepath's resolved path, backed by the OS's own file lock (fcntl.flock on POSIX, msvcrt.locking on Windows) — a crashed or killed holder releases it automatically, so unlike a hand-rolled marker file there's no staleness bookkeeping to get wrong."""
     lock_path = lock_path_for(filepath)
-    deadline = time.monotonic() + LOCK_WAIT_SECONDS
-    while True:
-        try:
-            fd = os.open(lock_path, os.O_CREAT | os.O_EXCL | os.O_WRONLY)
-            with os.fdopen(fd, "w") as f:
-                f.write(f"{os.getpid()} {time.time()}")
-            break
-        except FileExistsError:
-            if _steal_stale_lock(lock_path):
-                continue  # stale lock cleared, retry immediately
-            if time.monotonic() >= deadline:
-                raise LockTimeoutError(
-                    f"Another caveman-compress run appears to be compressing {filepath} "
-                    f"(lock: {lock_path}). Giving up after {LOCK_WAIT_SECONDS}s — retry once "
-                    "it finishes, or remove the lock file by hand if you know it's stale."
-                )
-            time.sleep(LOCK_POLL_INTERVAL)
+    lock_path.parent.mkdir(parents=True, exist_ok=True)
+    fd = os.open(lock_path, os.O_CREAT | os.O_RDWR)
     try:
-        yield
-    finally:
+        if os.fstat(fd).st_size == 0:
+            os.write(fd, b"\0")  # msvcrt.locking needs at least one byte in the file to lock
+        os.lseek(fd, 0, 0)
+        deadline = time.monotonic() + LOCK_WAIT_SECONDS
+        while True:
+            try:
+                _try_lock_nonblocking(fd)
+                break
+            except BlockingIOError:
+                if time.monotonic() >= deadline:
+                    raise LockTimeoutError(
+                        f"Another caveman-compress run appears to be compressing {filepath} "
+                        f"(lock: {lock_path}). Giving up after {LOCK_WAIT_SECONDS}s — retry once "
+                        "it finishes."
+                    ) from None
+                time.sleep(LOCK_POLL_INTERVAL)
         try:
-            lock_path.unlink()
-        except OSError:
-            pass
+            yield
+        finally:
+            _unlock(fd)
+    finally:
+        os.close(fd)
 
 
 def is_sensitive_path(filepath: Path) -> bool:
@@ -360,17 +344,14 @@ def build_fix_prompt(original: str, compressed: str, errors: List[str]) -> str:
 
 
 def compress_file(filepath: Path) -> bool:
-    # Resolve first so the lock and every check below key off the same
-    # canonical path regardless of how the caller spelled it.
+    # Resolve first so the lock and every check below key off the same canonical path regardless of how the caller spelled it.
     filepath = filepath.resolve()
     with file_lock(filepath):
         return _compress_file_locked(filepath)
 
 
 def _compress_file_locked(filepath: Path) -> bool:
-    # Entire read-modify-write-validate-retry sequence below runs under
-    # compress_file's file_lock — nothing here executes without holding the
-    # lock for this exact resolved path.
+    """Body of compress_file; runs entirely under compress_file's file_lock for this resolved path."""
     MAX_FILE_SIZE = 500_000  # 500KB
     if not filepath.exists():
         raise FileNotFoundError(f"File not found: {filepath}")

tests/test_compress_concurrency.py

@@ -1,11 +1,4 @@
-"""Tests for the cross-session compress lock (concurrency corruption bug).
-
-Two Claude Code sessions running caveman-compress against the same
-CLAUDE.md concurrently used to interleave reads/writes with no coordination:
-one session's finished edit could get silently clobbered by the other's
-in-flight compression pass. `file_lock` serializes access per resolved
-target path so only one compress run touches a given file at a time.
-"""
+"""Tests for the cross-session compress lock — without it, two concurrent caveman-compress runs on the same file interleave reads/writes and silently corrupt output; file_lock serializes access per resolved target path."""
 
 import os
 import sys
@@ -23,15 +16,23 @@
 
 
 class LockPathTests(unittest.TestCase):
-    def test_same_resolved_path_yields_same_lock_path(self):
-        with tempfile.TemporaryDirectory() as data_home:
+    def test_relative_and_resolved_spellings_of_same_file_yield_same_lock_path(self):
+        with tempfile.TemporaryDirectory() as data_home, tempfile.TemporaryDirectory() as tmp:
             with mock.patch.dict(os.environ, {"XDG_DATA_HOME": data_home, "LOCALAPPDATA": data_home}):
-                p = Path("/tmp/some/dir/task.md")
-                self.assertEqual(compress_mod.lock_path_for(p), compress_mod.lock_path_for(p))
+                target_dir = Path(tmp) / "sub"
+                target_dir.mkdir()
+                (target_dir / "task.md").write_text("x")
+                resolved = (target_dir / "task.md").resolve()
+                relative_cwd = os.getcwd()
+                try:
+                    os.chdir(tmp)
+                    via_relative = compress_mod.lock_path_for(Path("sub/task.md"))
+                finally:
+                    os.chdir(relative_cwd)
+                self.assertEqual(via_relative, compress_mod.lock_path_for(resolved))
 
     def test_same_basename_different_dirs_yields_different_lock_paths(self):
-        # Two repos each with their own CLAUDE.md must never contend for the
-        # same lock — only two runs against the *same* file should.
+        # Two repos each with their own CLAUDE.md must never contend for the same lock — only same-file runs should.
         with tempfile.TemporaryDirectory() as data_home:
             with mock.patch.dict(os.environ, {"XDG_DATA_HOME": data_home, "LOCALAPPDATA": data_home}):
                 a = compress_mod.lock_path_for(Path("/repo-a/CLAUDE.md"))
@@ -64,74 +65,89 @@ def try_second():
                 t2.start()
                 t1.join(timeout=5)
                 t2.join(timeout=5)
+                self.assertFalse(t1.is_alive())
+                self.assertFalse(t2.is_alive())
 
                 self.assertEqual(len(released_first_at), 1)
                 self.assertEqual(len(acquired_second_at), 1)
-                # The second lock must not be acquired before the first is released —
-                # this is the exact race that let two sessions interleave writes.
+                # The second lock must not be acquired before the first is released — this is the exact race that let two sessions interleave writes.
                 self.assertGreaterEqual(acquired_second_at[0], released_first_at[0])
 
-    def test_lock_file_removed_after_release(self):
+    def test_lock_file_persists_but_is_unlocked_after_release(self):
         with tempfile.TemporaryDirectory() as data_home:
             with mock.patch.dict(os.environ, {"XDG_DATA_HOME": data_home, "LOCALAPPDATA": data_home}):
                 target = Path("/tmp/whatever/CLAUDE.md")
                 lock_path = compress_mod.lock_path_for(target)
                 with compress_mod.file_lock(target):
                     self.assertTrue(lock_path.exists())
-                self.assertFalse(lock_path.exists())
+                # OS-native locks are held on the open file description, not the file's existence — the marker file itself is never deleted.
+                self.assertTrue(lock_path.exists())
+                start = time.monotonic()
+                with compress_mod.file_lock(target):
+                    pass
+                self.assertLess(time.monotonic() - start, 1)
 
-    def test_stale_lock_reclaimed_without_waiting_full_timeout(self):
+    def test_crashed_holder_lock_released_by_os_on_close(self):
         with tempfile.TemporaryDirectory() as data_home:
             with mock.patch.dict(os.environ, {"XDG_DATA_HOME": data_home, "LOCALAPPDATA": data_home}):
                 target = Path("/tmp/whatever/CLAUDE.md")
                 lock_path = compress_mod.lock_path_for(target)
-                lock_path.write_text("99999 0")
-                stale_mtime = time.time() - (compress_mod.LOCK_STALE_SECONDS + 5)
-                os.utime(lock_path, (stale_mtime, stale_mtime))
+                lock_path.parent.mkdir(parents=True, exist_ok=True)
+                fd = os.open(lock_path, os.O_CREAT | os.O_RDWR)
+                os.write(fd, b"\0")
+                os.lseek(fd, 0, 0)
+                compress_mod._try_lock_nonblocking(fd)
+                os.close(fd)  # simulates the holder process crashing/being killed without a clean unlock
 
                 start = time.monotonic()
                 with mock.patch.object(compress_mod, "LOCK_WAIT_SECONDS", 30):
                     with compress_mod.file_lock(target):
                         pass
-                elapsed = time.monotonic() - start
-                # Should reclaim near-instantly, not wait anywhere close to the
-                # (mocked, still generous) 30s wait budget.
-                self.assertLess(elapsed, 2)
+                # The OS releases the lock the instant the holder's fd closes — must succeed near-instantly, not wait out the budget.
+                self.assertLess(time.monotonic() - start, 2)
 
     def test_fresh_lock_not_stolen_and_times_out(self):
         with tempfile.TemporaryDirectory() as data_home:
             with mock.patch.dict(os.environ, {"XDG_DATA_HOME": data_home, "LOCALAPPDATA": data_home}):
                 target = Path("/tmp/whatever/CLAUDE.md")
-                lock_path = compress_mod.lock_path_for(target)
-                lock_path.write_text(f"{os.getpid()} {time.time()}")
+                held = threading.Event()
+                release = threading.Event()
 
-                with mock.patch.object(compress_mod, "LOCK_WAIT_SECONDS", 0.2), \
-                     mock.patch.object(compress_mod, "LOCK_POLL_INTERVAL", 0.02):
-                    with self.assertRaises(compress_mod.LockTimeoutError):
-                        with compress_mod.file_lock(target):
-                            pass  # pragma: no cover - must never be reached
+                def hold():
+                    with compress_mod.file_lock(target):
+                        held.set()
+                        release.wait(timeout=5)
+
+                holder = threading.Thread(target=hold)
+                holder.start()
+                held.wait(timeout=5)
+                try:
+                    with mock.patch.object(compress_mod, "LOCK_WAIT_SECONDS", 0.2), \
+                         mock.patch.object(compress_mod, "LOCK_POLL_INTERVAL", 0.02):
+                        with self.assertRaises(compress_mod.LockTimeoutError):
+                            with compress_mod.file_lock(target):
+                                pass  # pragma: no cover - must never be reached
+                finally:
+                    release.set()
+                    holder.join(timeout=5)
+                self.assertFalse(holder.is_alive())
 
     def test_lock_released_on_exception_inside_block(self):
         with tempfile.TemporaryDirectory() as data_home:
             with mock.patch.dict(os.environ, {"XDG_DATA_HOME": data_home, "LOCALAPPDATA": data_home}):
                 target = Path("/tmp/whatever/CLAUDE.md")
-                lock_path = compress_mod.lock_path_for(target)
                 with self.assertRaises(ValueError):
                     with compress_mod.file_lock(target):
                         raise ValueError("boom")
-                self.assertFalse(lock_path.exists())
+                start = time.monotonic()
+                with compress_mod.file_lock(target):
+                    pass
+                self.assertLess(time.monotonic() - start, 1)
 
 
 class CompressFileLockIntegrationTests(unittest.TestCase):
     def test_concurrent_compress_calls_serialize_instead_of_interleaving(self):
-        # Two threads calling compress_file on the SAME file concurrently used
-        # to interleave reads/writes with no coordination. With the lock, the
-        # second call only starts once the first has fully finished (backup
-        # written, target written, lock released) — so it deterministically
-        # hits the existing "backup already exists" guard instead of racing
-        # the first call's in-flight write. Neither outcome is corruption;
-        # what matters is there's exactly one call_claude invocation (no
-        # overlap) and the target ends up with the first call's clean output.
+        # Two threads on the SAME file used to interleave; the lock instead serializes them so exactly one call_claude runs.
         with tempfile.TemporaryDirectory() as tmp, tempfile.TemporaryDirectory() as data_home:
             with mock.patch.dict(os.environ, {"XDG_DATA_HOME": data_home, "LOCALAPPDATA": data_home}):
                 original = "# Heading\n\nProse to compress, long enough to pass the identity check here.\n"
@@ -162,15 +178,15 @@ def run():
                 t2.start()
                 t1.join(timeout=10)
                 t2.join(timeout=10)
+                self.assertFalse(t1.is_alive())
+                self.assertFalse(t2.is_alive())
 
-                # Exactly one compression ever ran — the lock prevented the
-                # second thread from ever reading/writing the file while the
-                # first was mid-flight. That's what closes the actual race.
+                # Exactly one compression ran — the lock stopped the second thread from touching the file while the first was mid-flight.
                 self.assertEqual(len(call_starts), 1)
                 self.assertEqual(len(results), 2)
                 self.assertEqual(sorted(results), [False, True])
                 self.assertEqual(path.read_text(encoding="utf-8"), compressed)
-                self.assertFalse(lock_path.exists())
+                self.assertTrue(lock_path.exists())
 
 
 if __name__ == "__main__":