From bc2062a4ec515be3d5791723238edff97c341fbe Mon Sep 17 00:00:00 2001 From: ProtocolWarden <32967198+ProtocolWarden@users.noreply.github.com> Date: Mon, 3 Aug 2026 15:34:47 -0400 Subject: [PATCH] fix(reconcile): give the prune lock a Windows backend `reconcile_lock` raised RuntimeError on any host without `fcntl`, so `prune --apply` was unrunnable on Windows. The reconciliation workflow dead-ended there, which is why consumer logs kept getting hand-pruned instead. `msvcrt.locking(LK_NBLCK)` is the direct analogue: non-blocking, exclusive, released on process death, and it conflicts with a second handle opened by the same process, so the existing nested-acquire test still holds. One difference shapes the file layout. `flock` is advisory and whole-file, but a `msvcrt` byte range is mandatory: a reader touching a locked byte gets PermissionError. The lock therefore claims a sentinel byte at offset 1024 while the pid stays at offset 0, where a contending run can still read it to name the holder. The pid is written as a fixed-width field rather than truncate+write, because truncation would cross the locked range. Both invariants have a test. Contention is matched on errno rather than exception type (`flock` reports EWOULDBLOCK, `msvcrt` reports EACCES and EDEADLOCK). An OSError outside that set is re-raised unchanged -- a bad fd must never be reported as "someone else holds it", which the previous BlockingIOError-only catch got right by accident and a broad `except OSError` would have got wrong. Not merely an unblock: this fixes 9 pre-existing Windows test failures, all the same RuntimeError -- four lock tests and five prune tests. Suite goes 25 failures -> 16, and the remaining 16 are a strict subset of the previous set (diffed by name, not counted). 457 -> 473 passed = 9 fixed + 7 new. 7 new tests: a backend exists on this platform; the pid stays readable while the lock is held (fails if the sentinel byte ever moves onto the pid field); the layout invariant; a stale longer pid cannot leave a tail; cross-process exclusion; reacquire after the holder is killed; and an unexpected OSError not being laundered into PruneLockHeld. The cross-process test has the child print its own `os.getpid()` rather than trusting `Popen.pid` -- a venv's python.exe can be a launcher shim, so the process that takes the lock is not always the one Popen returns. Co-Authored-By: Claude Opus 5 --- .console/log.md | 23 +++++ src/context_lifecycle/reconcile/lock.py | 109 ++++++++++++++++---- tests/test_reconcile_lock.py | 127 ++++++++++++++++++++++-- 3 files changed, 234 insertions(+), 25 deletions(-) diff --git a/.console/log.md b/.console/log.md index 5854b4b..ce99dc9 100644 --- a/.console/log.md +++ b/.console/log.md @@ -1,4 +1,27 @@ # Log +## 2026-08-03 — fix(reconcile): give the prune lock a Windows backend + +`reconcile_lock` raised `RuntimeError` without `fcntl`, so `prune --apply` was +unrunnable on Windows — which is why consumer logs got hand-pruned instead. + +`msvcrt.locking(LK_NBLCK)` is the direct analogue: non-blocking, exclusive, +released on process death, conflicts with a second handle in the same process. +One difference shapes the layout — `flock` is advisory and whole-file, but a +`msvcrt` range is mandatory, so a reader touching a locked byte gets +PermissionError. The lock claims a sentinel byte at offset 1024 while the pid +stays at 0, readable by a contending run that wants to name the holder; the pid +is a fixed-width field because truncating would cross the locked range. Both +invariants have a test. Contention now matches on errno (`flock` gives +EWOULDBLOCK, `msvcrt` EACCES/EDEADLOCK); anything else re-raises as itself, so a +bad fd is never reported as "someone else holds it". + +Not just an unblock: 9 pre-existing Windows failures were all this same +RuntimeError — four lock tests, five prune tests. Suite 25 failures -> 16, the +16 a strict subset of the old set (diffed by name). 457 -> 473 = 9 fixed + 7 new. + +The cross-process test has the child print its own `os.getpid()`: a venv's +python.exe can be a shim, so `Popen.pid` is not always the lock holder. + ## 2026-08-03 — fix(cli): stop console encoding from failing a command that succeeded `cl reconcile check` computed a GREEN verdict, then died printing it: diff --git a/src/context_lifecycle/reconcile/lock.py b/src/context_lifecycle/reconcile/lock.py index 4fcfa47..3edc36f 100644 --- a/src/context_lifecycle/reconcile/lock.py +++ b/src/context_lifecycle/reconcile/lock.py @@ -4,7 +4,7 @@ Two prunes applying concurrently against the same repo can interleave the archive append and the source trim (same-section archived twice; the -idempotency-by-heading guard races its own read). An exclusive ``flock`` on +idempotency-by-heading guard races its own read). An exclusive lock on ``.console/.reconcile.lock`` serializes appliers **on one host**; cross-host runs are already serialized by git (append-only archive + idempotent re-run, merged with ordinary rebase) — the lock closes the same-host window where no @@ -12,26 +12,99 @@ The lock file is untracked working state (like the worksheet) and is left in place after release; holding pid is recorded for diagnostics only. + +Platform backends +───────────────── +POSIX uses ``fcntl.flock``; Windows uses ``msvcrt.locking``. Both are +non-blocking, both release automatically if the holder dies, and both conflict +with a second handle opened by the *same* process — so a nested acquire raises +rather than silently succeeding. + +They differ in one way that shapes the file layout. ``flock`` is advisory and +whole-file, so any reader can still read the pid. ``msvcrt.locking`` locks a +byte *range* and Windows enforces it: a reader touching a locked byte gets +``PermissionError``. The lock therefore claims a single sentinel byte at +``_LOCK_OFFSET``, clear of the pid field at offset 0, so a contending run can +still read and name the holder. Writing the pid as a fixed-width field avoids +truncating the file, which would otherwise cross the locked range. + +Before 2026-08-03 this module raised on any non-POSIX host, so +``prune --apply`` was unrunnable on Windows — the reconciliation workflow +dead-ended there and logs got hand-pruned instead. """ from __future__ import annotations -try: - import fcntl -except ImportError: # POSIX-only. On Windows only `reconcile prune --apply` needs - fcntl = None # type: ignore[assignment] # it; importing this module must still work. +import errno import os -from contextlib import contextmanager from collections.abc import Iterator +from contextlib import contextmanager from pathlib import Path +try: + import fcntl +except ImportError: # Windows — msvcrt below is the backend there. + fcntl = None # type: ignore[assignment] +try: + import msvcrt +except ImportError: # POSIX — fcntl above is the backend there. + msvcrt = None # type: ignore[assignment] + LOCK_RELPATH = Path(".console") / ".reconcile.lock" +# Pid is written as a fixed-width field at offset 0, so a shorter pid cannot +# leave a longer predecessor's tail behind and no truncation is needed. +_PID_FIELD = 32 +# The byte msvcrt actually locks. Must sit clear of the pid field: on Windows a +# locked range is mandatory, and a contending run reads the pid to name the +# holder. Unused by the POSIX backend, whose flock covers the whole file. +_LOCK_OFFSET = 1024 + +# Errnos meaning "someone else holds it" rather than "the call was wrong". +# POSIX flock reports EWOULDBLOCK/EAGAIN; msvcrt reports EACCES, and EDEADLOCK +# once its internal retries are exhausted. +_CONTENDED = frozenset( + e for e in ( + getattr(errno, "EACCES", None), + getattr(errno, "EAGAIN", None), + getattr(errno, "EWOULDBLOCK", None), + getattr(errno, "EDEADLOCK", None), + getattr(errno, "EDEADLK", None), + ) + if e is not None +) + class PruneLockHeld(RuntimeError): """Raised when another prune --apply holds the repo's reconcile lock.""" +def _acquire(fd: int) -> None: + """Take the exclusive lock without blocking. Raises OSError if contended.""" + if fcntl is not None: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + return + os.lseek(fd, _LOCK_OFFSET, os.SEEK_SET) + msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) + + +def _release(fd: int) -> None: + if fcntl is not None: + fcntl.flock(fd, fcntl.LOCK_UN) + return + os.lseek(fd, _LOCK_OFFSET, os.SEEK_SET) + msvcrt.locking(fd, msvcrt.LK_UNLCK, 1) + + +def _read_holder(fd: int) -> str: + """Best-effort pid of the current holder, for the error message only.""" + try: + os.lseek(fd, 0, os.SEEK_SET) + return os.read(fd, _PID_FIELD).decode("utf-8", "replace").strip() + except OSError: + return "" + + @contextmanager def reconcile_lock(repo_root: Path) -> Iterator[None]: """Hold the exclusive per-repo reconcile lock for the duration of the block. @@ -41,33 +114,31 @@ def reconcile_lock(repo_root: Path) -> Iterator[None]: than queue behind it (the second run is a no-op anyway once the first lands). """ - if fcntl is None: + if fcntl is None and msvcrt is None: # pragma: no cover - neither backend raise RuntimeError( - "reconcile prune --apply requires POSIX fcntl file locking, " - "unavailable on this platform" + "reconcile prune --apply requires file locking (fcntl or msvcrt), " + "and neither is available on this platform" ) lock_path = Path(repo_root) / LOCK_RELPATH lock_path.parent.mkdir(parents=True, exist_ok=True) fd = os.open(lock_path, os.O_RDWR | os.O_CREAT, 0o644) try: try: - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) - except BlockingIOError: - holder = "" - try: - holder = os.read(fd, 64).decode("utf-8", "replace").strip() - except OSError: - pass + _acquire(fd) + except OSError as exc: + if exc.errno not in _CONTENDED: + raise # a real error (bad fd, I/O) — never report it as contention + holder = _read_holder(fd) detail = f" (held by pid {holder})" if holder else "" raise PruneLockHeld( f"another prune --apply is running against this repo{detail}; " f"lock: {lock_path}" ) from None - os.ftruncate(fd, 0) - os.write(fd, str(os.getpid()).encode("utf-8")) + os.lseek(fd, 0, os.SEEK_SET) + os.write(fd, str(os.getpid()).ljust(_PID_FIELD).encode("utf-8")) try: yield finally: - fcntl.flock(fd, fcntl.LOCK_UN) + _release(fd) finally: os.close(fd) diff --git a/tests/test_reconcile_lock.py b/tests/test_reconcile_lock.py index 0315f62..3de615b 100644 --- a/tests/test_reconcile_lock.py +++ b/tests/test_reconcile_lock.py @@ -4,11 +4,16 @@ from __future__ import annotations +import errno +import os +import subprocess +import sys from hashlib import sha256 from pathlib import Path import pytest +from context_lifecycle.reconcile import lock as lock_mod from context_lifecycle.reconcile.lock import ( LOCK_RELPATH, PruneLockHeld, @@ -56,9 +61,8 @@ def test_lock_is_exclusive_and_reentrant_after_release(tmp_path): entered = 0 with reconcile_lock(repo): entered += 1 - with pytest.raises(PruneLockHeld): - with reconcile_lock(repo): - pytest.fail("second acquire must not enter the block") + with pytest.raises(PruneLockHeld), reconcile_lock(repo): + pytest.fail("second acquire must not enter the block") with reconcile_lock(repo): # released → reacquirable entered += 1 assert entered == 2 @@ -74,9 +78,8 @@ def test_apply_refused_while_lock_held(tmp_path, monkeypatch): plan = build_plan(repo, private_root=private) log_before = _checksum(repo / ".console" / "log.md") - with reconcile_lock(repo): # another applier - with pytest.raises(PruneLockHeld): - apply_plan(repo, plan) + with reconcile_lock(repo), pytest.raises(PruneLockHeld): # another applier + apply_plan(repo, plan) # Refused apply mutated nothing. assert _checksum(repo / ".console" / "log.md") == log_before assert not private.exists() @@ -96,3 +99,115 @@ def test_lock_released_after_apply(tmp_path, monkeypatch): with reconcile_lock(repo): # would raise PruneLockHeld if apply leaked it reacquired = True assert reacquired + + +# ── cross-platform backend ─────────────────────────────────────────────────── +# Until 2026-08-03 this module raised on any non-POSIX host, so `prune --apply` +# was unrunnable on Windows. These pin that both backends actually lock, rather +# than that the module merely imports. + + +def test_a_backend_is_available_on_this_platform(): + """No supported platform may fall through to the 'neither backend' error.""" + assert (lock_mod.fcntl is not None) or (lock_mod.msvcrt is not None) + + +def test_holder_pid_is_recorded_and_readable_while_held(tmp_path): + """The pid field must stay readable while the lock is held. + + On Windows the locked range is mandatory, so this fails if the sentinel byte + is ever moved on top of the pid field — a contending run could then not name + the holder. + """ + (tmp_path / ".console").mkdir() + with reconcile_lock(tmp_path): + raw = (tmp_path / LOCK_RELPATH).read_bytes() + assert raw[:lock_mod._PID_FIELD].decode().strip() == str(os.getpid()) + + +def test_pid_field_and_lock_byte_do_not_overlap(): + """Layout invariant the Windows backend depends on.""" + assert lock_mod._LOCK_OFFSET >= lock_mod._PID_FIELD + + +def test_shorter_pid_cannot_leave_a_longer_predecessors_tail(tmp_path): + """Fixed-width field instead of truncate — truncation would cross the lock.""" + (tmp_path / ".console").mkdir() + lock_file = tmp_path / LOCK_RELPATH + lock_file.parent.mkdir(parents=True, exist_ok=True) + lock_file.write_bytes(b"9" * lock_mod._PID_FIELD) # a long stale pid + with reconcile_lock(tmp_path): + pass + recorded = lock_file.read_bytes()[: lock_mod._PID_FIELD].decode().strip() + assert recorded == str(os.getpid()) + assert "9" * 8 not in recorded + + +# The child reports its OWN pid rather than the test trusting `Popen.pid`: a +# venv's python.exe can be a launcher shim, so the process that takes the lock +# is not always the one Popen returns. +_CHILD = """ +import os, sys, time +from pathlib import Path +sys.path.insert(0, {src!r}) +from context_lifecycle.reconcile.lock import reconcile_lock +with reconcile_lock(Path({repo!r})): + print("HELD", os.getpid(), flush=True) + time.sleep(float({hold!r})) +""" + + +def test_lock_excludes_a_separate_process(tmp_path): + """The real contract: another *process* is refused while the lock is held. + + The same-process test above passes on any backend that tracks handles; only + a second process proves the OS is enforcing it. + """ + (tmp_path / ".console").mkdir() + src = str(Path(lock_mod.__file__).parents[3]) + child = subprocess.Popen( + [sys.executable, "-c", _CHILD.format(src=src, repo=str(tmp_path), hold="5")], + stdout=subprocess.PIPE, text=True, + ) + try: + held, _, child_pid = child.stdout.readline().strip().partition(" ") + assert held == "HELD", "child never acquired the lock" + with pytest.raises(PruneLockHeld) as exc, reconcile_lock(tmp_path): + pytest.fail("acquired a lock another process holds") + # The holder's pid is reported, which is the whole point of the layout. + assert child_pid and child_pid in str(exc.value) + assert str(os.getpid()) != child_pid, "child must be a separate process" + finally: + child.kill() + child.wait() + + +def test_lock_is_reacquirable_after_the_holding_process_dies(tmp_path): + """Both backends release on process death — no stale lock survives a crash.""" + (tmp_path / ".console").mkdir() + src = str(Path(lock_mod.__file__).parents[3]) + child = subprocess.Popen( + [sys.executable, "-c", _CHILD.format(src=src, repo=str(tmp_path), hold="30")], + stdout=subprocess.PIPE, text=True, + ) + assert child.stdout.readline().split()[0] == "HELD" + child.kill() + child.wait() + acquired = False + with reconcile_lock(tmp_path): # stale lock would raise here + acquired = True + assert acquired + + +def test_unexpected_oserror_is_not_reported_as_contention(tmp_path, monkeypatch): + """A real failure must surface as itself, not as 'someone else holds it'.""" + (tmp_path / ".console").mkdir() + + def _boom(fd): + raise OSError(errno.EIO, "disk fell over") + + monkeypatch.setattr(lock_mod, "_acquire", _boom) + with pytest.raises(OSError) as exc, reconcile_lock(tmp_path): + pytest.fail("must not enter the block") + assert not isinstance(exc.value, PruneLockHeld) + assert exc.value.errno == errno.EIO