From f1a57bbcabf5cf3da9654ac4d54d056dc6b4b7ea Mon Sep 17 00:00:00 2001 From: Edward Cheng-I Wu <132531341+Imbad0202@users.noreply.github.com> Date: Fri, 11 Sep 2026 14:47:26 +0800 Subject: [PATCH] fix: shared file-lock helper with msvcrt backend for the remaining fcntl sites (#845) (#847) * fix: shared file-lock helper with msvcrt backend for the six fcntl sites (#845) scripts/file_lock.py owns the backend choice (fcntl.flock on POSIX, msvcrt.locking on byte 0 on Windows) and routes adjudication_activity, inquiry_branch_ledger, review_criteria_binding, and ars_mark_read through acquire()/release(). POSIX lock sequences are unchanged. Per-site Windows decisions: adjudication reads degrade to exclusive with a 5 s bounded wait; the review-criteria manifest lock is capped at 30 s on Windows only; the inquiry ledger alpha keeps refusing non-POSIX hosts. Two finally blocks that released an unacquired lock now release only what they acquired. SETUP docs state the best-effort Windows posture; no Windows CI job is added. Refs #845, #843, #844. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131cZMWBPPeEFiqgEPFZ3X2 * fix(file_lock): interrupted attempts honour the deadline; pin adjudication wait policy (#845) Cross-model review round 1 (gpt-6-astra, xhigh): a persistent InterruptedError could retry past the bound; the Windows-shape test did not exercise adjudication's reader-waits / writer-does-not-wait policy; the adjudication contention message now names LockTimeout instead of BlockingIOError, recorded in the CHANGELOG rather than masked. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131cZMWBPPeEFiqgEPFZ3X2 * refactor(file_lock): held() context manager, single BACKEND source, one fake msvcrt (#845) /simplify pass (four cleanup reviewers): the release-only-if-acquired invariant moves into file_lock.held() and review_criteria_binding / inquiry_branch_ledger use it; runtime branches key off BACKEND and SHARED_LOCKS_SUPPORTED is dropped; EINTR joins the retryable errno set and the unreachable EDEADLK entry goes; backend calls are deduplicated; all four consumers try the sibling import first so one module instance is shared; the Windows fake lives once in tests/fake_msvcrt.py; test scaffolding is folded into a lock_pair fixture and a parametrized wait test. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131cZMWBPPeEFiqgEPFZ3X2 * fix(file_lock): keep lock acquisition and the guarded body in separate try blocks (#845) Cross-model review round 3 (gpt-6-astra, xhigh): wrapping the body in the same handler that translates LockTimeout meant a contended inner lock inside the body was reported as the outer manifest/passport lock failing. Both consumers now acquire in their own try block and release only after a successful acquire; held() is dropped from the helper. The subprocess test pins that a LockTimeout raised inside the binding body surfaces as itself. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131cZMWBPPeEFiqgEPFZ3X2 * test(file_lock): let the body LockTimeout leave _locked() so the attribution check bites (#845) Cross-model review round 4: the inner LockTimeout was caught inside the binding body, so the erroneous outer translation would still have passed. Verified by mutation: restoring the outer translation fails this test. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131cZMWBPPeEFiqgEPFZ3X2 * ci(673): whitelist scripts/test_file_lock.py as a non-consumer importer of the activity runtime (#845) The shared file-lock test imports adjudication_activity in a subprocess to exercise its lock backend under a fake msvcrt; it never reads or writes an activity store. The exact-owner whitelist is the lint's route for that. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0131cZMWBPPeEFiqgEPFZ3X2 --------- Co-authored-by: Claude Fable 5.1 --- CHANGELOG.md | 2 + docs/SETUP.md | 2 + docs/SETUP.zh-TW.md | 2 + scripts/_ci_pytest_manifest.toml | 4 + scripts/adjudication_activity.py | 23 +- scripts/ars_mark_read.py | 60 +--- scripts/check_673_adjudication_activity.py | 4 + scripts/file_lock.py | 114 +++++++ scripts/inquiry_branch_ledger.py | 39 +-- scripts/review_criteria_binding.py | 21 +- scripts/test_adjudication_activity.py | 9 +- scripts/test_ars_mark_read.py | 4 +- scripts/test_file_lock.py | 353 +++++++++++++++++++++ scripts/test_inquiry_branch_ledger.py | 9 +- tests/fake_msvcrt.py | 43 +++ 15 files changed, 606 insertions(+), 83 deletions(-) create mode 100644 scripts/file_lock.py create mode 100644 scripts/test_file_lock.py create mode 100644 tests/fake_msvcrt.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f5631ecf..09062da1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable changes to this project will be documented in this file. - **`/ars-mark-read` no longer fails on Windows: the ledger lock has an `msvcrt` backend (#843, PR #844 by @dajiaohuang).** `scripts/ars_mark_read.py` imported the POSIX-only `fcntl` module at load time, so on Windows the documented CLI raised `ModuleNotFoundError` before argument parsing and every test in `tests/test_mark_read_args.py` failed. The module now imports `fcntl` where available and falls back to `msvcrt`; two small helpers (`_lock_nonblocking`, `_unlock`) select `fcntl.flock(LOCK_EX | LOCK_NB)` on POSIX and `msvcrt.locking(LK_NBLCK, 1)` on Windows, inside the unchanged bounded retry loop (Windows contention raises `EACCES`, which the loop already retries). Review dropped a proposed empty-file NUL pre-write because `msvcrt.locking` can lock a byte beyond EOF and the write sat outside the retry loop. The POSIX path is byte-for-byte the same lock sequence. This fixes one entry point only: the remaining `fcntl` imports (`adjudication_activity`, `inquiry_branch_ledger`, `review_criteria_binding`, and their tests) are tracked in #845, and there is no Windows CI job, so Windows behaviour rests on the contributor's reported 4-passed focused run. +- **One shared file-lock helper replaces the six per-file `fcntl` sites; Windows gets a documented `msvcrt` backend (#845).** `scripts/adjudication_activity.py` and `scripts/review_criteria_binding.py` still imported POSIX-only `fcntl` at load time (so their CLIs and test modules failed on Windows before parsing arguments), `scripts/inquiry_branch_ledger.py` carried its own try/except, and `scripts/ars_mark_read.py` carried the #844 backend split inline. New `scripts/file_lock.py` owns the backend choice (`BACKEND`): `acquire(fd, exclusive=, timeout=)` / `release(fd)` over `fcntl.flock` on POSIX and `msvcrt.locking` on byte 0 on Windows, never writing the lock file, with contention on either backend surfacing as one `LockTimeout` (a `BlockingIOError` carrying `EAGAIN`). The POSIX lock sequences are unchanged; the one POSIX-visible difference is textual: the adjudication store's contention message now embeds `LockTimeout` where it embedded `BlockingIOError` (the error code `ERROR:LOCK` and exit status are the same, and nothing parses the class name). A signal that interrupts a lock attempt is retried but never past the deadline. The two semantic gaps are decided per site rather than hidden: the adjudication store's shared read lock degrades to an exclusive lock with a 5-second bounded wait where shared locks are unavailable (writers keep the non-waiting exclusive lock); the review-criteria manifest lock still blocks indefinitely on POSIX and is capped at `WINDOWS_BLOCKING_WAIT_SECONDS` (30 s) on Windows, surfacing as `BindingError`; the inquiry branch ledger alpha keeps refusing non-POSIX hosts (it now checks `file_lock.BACKEND`) because its durable writes have no Windows verification. Two release paths that unlocked an unacquired lock in `finally` (a no-op under `flock`, an `EACCES` under `msvcrt` that would have masked the real error) now acquire in their own `try` block and release only after a successful acquire, so a `LockTimeout` raised inside the guarded body is also never reported as the outer lock failing. `scripts/test_file_lock.py` (CI manifest id `845-shared-file-lock`) covers both backends: the real `fcntl` backend for contention, bounded and blocking waits, shared/exclusive interplay, and `tests/fake_msvcrt.py` (one model of the documented `_locking` contract) for the Windows branch, plus a subprocess test that imports all four consumers with `fcntl` blocked and exercises each site's Windows decision. `docs/SETUP.md` / `SETUP.zh-TW.md` state the platform posture. No Windows CI job is added; real Windows verification remains a manual step requested from the #843 reporter. + - **Socratic non-convergence path F6 no longer ranks or preselects a direction, and the two reference files no longer carry their own auto-end round count (#834).** `deep-research/references/failure_paths.md` § F6 offered "[the most promising direction]", told the mentor to "identify the 1-2 directions with the most convergence potential", and prescribed "restrict discussion scope", contradicting the #735 non-ranking boundary that `POSITIONING.md` and the mentor agent carry (the directions are the user's own, so this was ranking and preselection, not generation). F6 and `socratic_mode_protocol.md` § Dialogue Management Rules also still said "round 15 → end" after #490 made the mentor agent's § Auto-End Conditions (Precise) the single authority (40 goal-oriented / 60 exploratory). F6 now lists the directions the user has expressed in the order they were expressed, leaves the choice to the user, and names the visible exit marker on the `full`-mode option; both reference files point at the agent file for round caps and state none of their own. `scripts/test_socratic_rq_non_generation_contract.py` gains ranking/preselection-vocabulary and own-round-count checks (plus a pointer/heading parity check) that fail on the pre-fix bytes. This closes a contract contradiction between prompt surfaces found during a cross-model fact-check of the v3.21.2 claim surfaces; it claims no breadth or diversity improvement (that remains #659) and no measured behavior change. - **OpenAI request builders no longer send parameters GPT-6 Astra rejects; Astra's API effort set is validated before any request (#823).** The executable smoke entrypoint (`scripts/cross_model_smoke_test.sh`) and the canonical OpenAI example in `shared/cross_model_verification.md` sent `temperature: 0.1` to `/v1/responses`; the official Astra migration guide lists `temperature`, `top_p`, and `top_logprobs` as unsupported, so a caller following the v3.21.2 recommendation built an API-incompatible request before the grounding checks could run. Both builders drop the sampling parameter (Gemini and compatible-provider examples keep theirs). Astra's documented API effort vocabulary (`low|medium|high|xhigh|max`) replaces the "not confirmed" wording and lives in one canonical per-model table, `scripts/cross_model_verification/openai_effort_guard.sh`, sourced by both builders; an explicitly configured Astra value outside that set now fails with `CROSS-MODEL-ERROR: invalid_astra_reasoning_effort` before `curl` runs, an unset effort still omits the field so the provider default applies, and ids without a table row stay pass-through. A hermetic test in `scripts/test_cross_model_verification_guards.py` executes both shipped builders (the smoke script and the documented Bash example located by content) against a fake `curl` and asserts the emitted JSON and that both source the guard. Astra stays provisional: request compatibility is not a bakeoff result. diff --git a/docs/SETUP.md b/docs/SETUP.md index 8026f8bf..55379637 100644 --- a/docs/SETUP.md +++ b/docs/SETUP.md @@ -26,6 +26,8 @@ curl -fsSL https://claude.ai/install.sh | bash irm https://claude.ai/install.ps1 | iex ``` +**Platform support.** macOS and Linux are the tested platforms; CI runs on Ubuntu only. Windows is best-effort: the scripts that lock files share one helper (`scripts/file_lock.py`) with an `msvcrt` backend, no Windows CI job exists, and Windows behaviour rests on contributor verification (#843, #845). On Windows, shared read locks degrade to exclusive locks with a short wait, indefinite lock waits are capped at 30 seconds, and the inquiry branch ledger alpha refuses to run. +
Alternative: npm install (deprecated) diff --git a/docs/SETUP.zh-TW.md b/docs/SETUP.zh-TW.md index fd3a0b06..c27c81e6 100644 --- a/docs/SETUP.zh-TW.md +++ b/docs/SETUP.zh-TW.md @@ -26,6 +26,8 @@ curl -fsSL https://claude.ai/install.sh | bash irm https://claude.ai/install.ps1 | iex ``` +**平台支援。** macOS 與 Linux 是經過測試的平台，CI 只在 Ubuntu 上執行。Windows 屬盡力支援：會鎖檔的 script 共用一個 helper（`scripts/file_lock.py`），內含 `msvcrt` 後端；沒有 Windows CI job，Windows 行為仰賴貢獻者驗證（#843、#845）。在 Windows 上，共享讀取鎖會降級為獨占鎖並短暫等待，無限期的鎖等待上限為 30 秒，探究分支帳本（alpha）會拒絕執行。 +
替代方案：npm 安裝（已棄用） diff --git a/scripts/_ci_pytest_manifest.toml b/scripts/_ci_pytest_manifest.toml index 8035137e..fbb40768 100644 --- a/scripts/_ci_pytest_manifest.toml +++ b/scripts/_ci_pytest_manifest.toml @@ -679,3 +679,7 @@ path = "scripts/test_score_calibration_run.py" [[pytest]] id = "653-calibration-measurement-row" path = "scripts/test_build_calibration_measurement_row.py" + +[[pytest]] +id = "845-shared-file-lock" +path = "scripts/test_file_lock.py" diff --git a/scripts/adjudication_activity.py b/scripts/adjudication_activity.py index 9ec839b7..35fa612b 100644 --- a/scripts/adjudication_activity.py +++ b/scripts/adjudication_activity.py @@ -12,7 +12,6 @@ from __future__ import annotations import argparse import copy import errno -import fcntl import hashlib import json import os @@ -27,6 +26,11 @@ from typing import Any, Iterator, Sequence from jsonschema import Draft202012Validator, FormatChecker from referencing import Registry, Resource +try: # Dual-path import: sibling module on sys.path vs package import. + import file_lock +except ImportError: # pragma: no cover - package-import path + from scripts import file_lock # type: ignore[no-redef] + REPO_ROOT = Path(__file__).resolve().parents[1] INPUT_SCHEMA_PATH = REPO_ROOT / "shared/contracts/activity/adjudication_activity_input.schema.json" @@ -355,6 +359,13 @@ def _read_artifact_relative(root: Path, relative: str, limit: int) -> bytes: os.close(opened_fd) +# Readers take a shared lock under flock and never wait. msvcrt has no shared +# mode, so there a read degrades to an exclusive lock with this short bounded +# wait so two concurrent readers do not turn into a spurious LOCK failure; +# writers keep the non-waiting exclusive lock on both backends. +READER_FALLBACK_WAIT_SECONDS = 5.0 + + @contextmanager def _store_lock(path: Path, *, exclusive: bool) -> Iterator[None]: lock_path = path.with_name(path.name + ".lock") @@ -363,8 +374,12 @@ def _store_lock(path: Path, *, exclusive: bool) -> Iterator[None]: info = os.fstat(fd) if not stat.S_ISREG(info.st_mode) or info.st_nlink != 1 or info.st_size != 0: raise ActivityError("LOCK", "lock metadata is not an empty single-linked regular file") - operation = (fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH) | fcntl.LOCK_NB - fcntl.flock(fd, operation) + wait = ( + 0.0 + if exclusive or file_lock.BACKEND == "fcntl" + else READER_FALLBACK_WAIT_SECONDS + ) + file_lock.acquire(fd, exclusive=exclusive, timeout=wait) except ActivityError: if "fd" in locals(): os.close(fd) @@ -376,7 +391,7 @@ def _store_lock(path: Path, *, exclusive: bool) -> Iterator[None]: try: yield finally: - fcntl.flock(fd, fcntl.LOCK_UN) + file_lock.release(fd) os.close(fd) diff --git a/scripts/ars_mark_read.py b/scripts/ars_mark_read.py index dba83001..8a3ec625 100644 --- a/scripts/ars_mark_read.py +++ b/scripts/ars_mark_read.py @@ -36,11 +36,9 @@ Behavior summary: from __future__ import annotations import argparse -import errno import os import sys import tempfile -import time import uuid from contextlib import contextmanager from datetime import datetime, timezone @@ -49,11 +47,10 @@ from typing import Any, Iterator import yaml -try: - import fcntl -except ModuleNotFoundError: # pragma: no cover - exercised on Windows - fcntl = None # type: ignore[assignment] - import msvcrt +try: # Dual-path import: sibling module on sys.path vs package import. + import file_lock +except ImportError: # pragma: no cover - package-import path + from scripts import file_lock # type: ignore[no-redef] try: from scripts.human_read_attestation_resolver import ( @@ -79,23 +76,6 @@ READ_SCOPE_LEVELS = ("full_text", "sections", "abstract_only", "toc_only", "unkn LOCATOR_MAX_LEN = 200 NOTE_MAX_LEN = 1000 LEDGER_LOCK_TIMEOUT_SECONDS = 10.0 -LEDGER_LOCK_POLL_SECONDS = 0.05 - - -def _lock_nonblocking(fd: int) -> None: - """Acquire one byte of the peer lock using the host OS backend.""" - if fcntl is not None: - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) - return - msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) - - -def _unlock(fd: int) -> None: - """Release the peer lock using the host OS backend.""" - if fcntl is not None: - fcntl.flock(fd, fcntl.LOCK_UN) - return - msvcrt.locking(fd, msvcrt.LK_UNLCK, 1) class LedgerLockError(RuntimeError): @@ -157,27 +137,15 @@ def _ledger_lock( acquired = False try: - os.lseek(fd, 0, os.SEEK_SET) - deadline = time.monotonic() + timeout - while True: - try: - _lock_nonblocking(fd) - acquired = True - break - except InterruptedError: - continue - except OSError as exc: - if exc.errno in (errno.EACCES, errno.EAGAIN): - remaining = deadline - time.monotonic() - if remaining <= 0: - raise LedgerLockError( - f"timed out after {timeout:g}s waiting for peer lock" - ) from exc - time.sleep(min(LEDGER_LOCK_POLL_SECONDS, remaining)) - continue - raise LedgerLockError( - f"cannot acquire peer lock: {exc}" - ) from exc + try: + file_lock.acquire(fd, exclusive=True, timeout=timeout) + except file_lock.LockTimeout as exc: + raise LedgerLockError( + f"timed out after {timeout:g}s waiting for peer lock" + ) from exc + except OSError as exc: + raise LedgerLockError(f"cannot acquire peer lock: {exc}") from exc + acquired = True yield finally: @@ -185,7 +153,7 @@ def _ledger_lock( release_error: OSError | None = None if acquired: try: - _unlock(fd) + file_lock.release(fd) except OSError as exc: release_error = exc try: diff --git a/scripts/check_673_adjudication_activity.py b/scripts/check_673_adjudication_activity.py index aca1ab9b..68a0afda 100644 --- a/scripts/check_673_adjudication_activity.py +++ b/scripts/check_673_adjudication_activity.py @@ -72,6 +72,10 @@ PYTHON_EXECUTION_WHITELIST = { # Prompt-size regression test names the bounded #673 documentation block; # it neither imports nor executes the activity runtime. Path("scripts/test_v3_6_7_phase_6_6.py"), + # Shared file-lock helper test (#845): imports the activity runtime in a + # subprocess only to exercise its lock backend under a fake msvcrt; it + # never reads, renders, or writes an activity store as a consumer. + Path("scripts/test_file_lock.py"), } LIMITATION_SENTENCE = ( diff --git a/scripts/file_lock.py b/scripts/file_lock.py new file mode 100644 index 00000000..a639fd91 --- /dev/null +++ b/scripts/file_lock.py @@ -0,0 +1,114 @@ +#!/usr/bin/env python3 +"""Shared advisory file locking for ARS scripts (#845). + +One backend per host, chosen at import time and published as ``BACKEND``: + +* ``"fcntl"`` (POSIX): ``flock``. Shared and exclusive modes, blocking and + non-blocking, exactly as before #845. +* ``"msvcrt"`` (Windows): ``locking`` on byte 0 of the lock file. This + backend is best-effort and has no CI coverage; it differs from ``flock`` in + two documented ways that callers decide about rather than paper over: + + - there is no shared mode, so ``exclusive=False`` takes an exclusive lock; + - there is no indefinite blocking wait, so ``timeout=None`` polls for at + most ``WINDOWS_BLOCKING_WAIT_SECONDS`` and then raises ``LockTimeout``. + +The helper never reads or writes the lock file. ``msvcrt.locking`` may lock +a byte beyond end-of-file, so an empty lock file is valid on both backends; +callers that require the lock file to stay empty keep that invariant. + +Contention on either backend surfaces as ``LockTimeout`` (a +``BlockingIOError`` carrying ``EAGAIN``), including ``timeout=0``, so a +caller can treat "someone else holds it" uniformly. Every other ``OSError`` +propagates unchanged. +""" +from __future__ import annotations + +import errno +import os +import time + +try: + import fcntl + + BACKEND = "fcntl" +except ModuleNotFoundError: # pragma: no cover - exercised on Windows + import msvcrt # type: ignore[import-not-found] + + BACKEND = "msvcrt" + +WINDOWS_BLOCKING_WAIT_SECONDS = 30.0 +POLL_SECONDS = 0.05 + +# EAGAIN / EWOULDBLOCK: flock non-blocking contention. EACCES: msvcrt +# contention. EINTR: a signal interrupted the attempt; retried like +# contention so the deadline still bounds it. +_RETRYABLE_ERRNOS = frozenset( + {errno.EAGAIN, errno.EWOULDBLOCK, errno.EACCES, errno.EINTR} +) + + +class LockTimeout(BlockingIOError): + """The lock was still held by someone else when the wait ran out.""" + + def __init__(self, waited: float) -> None: + super().__init__(errno.EAGAIN, f"lock still held after {waited:g}s") + + +def _flock(fd: int, *, exclusive: bool, blocking: bool) -> None: + operation = fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH + if not blocking: + operation |= fcntl.LOCK_NB + fcntl.flock(fd, operation) + + +def _msvcrt_byte0(fd: int, mode: int) -> None: + os.lseek(fd, 0, os.SEEK_SET) + msvcrt.locking(fd, mode, 1) + + +def _try_once(fd: int, *, exclusive: bool) -> None: + """One non-blocking attempt; raises OSError with a retryable errno if held.""" + if BACKEND == "fcntl": + _flock(fd, exclusive=exclusive, blocking=False) + else: + _msvcrt_byte0(fd, msvcrt.LK_NBLCK) + + +def acquire(fd: int, *, exclusive: bool = True, timeout: float | None) -> None: + """Acquire an advisory lock on ``fd``. + + ``timeout=None`` blocks until the lock is free (bounded on Windows, see + module docstring); ``timeout=0`` makes a single attempt; ``timeout>0`` + polls until the deadline. Raises ``LockTimeout`` when the lock is still + held at the end of the wait. + """ + if timeout is not None and timeout < 0: + raise ValueError("lock timeout must be non-negative") + + if timeout is None and BACKEND == "fcntl": + _flock(fd, exclusive=exclusive, blocking=True) + return + + wait = WINDOWS_BLOCKING_WAIT_SECONDS if timeout is None else float(timeout) + deadline = time.monotonic() + wait + while True: + try: + _try_once(fd, exclusive=exclusive) + return + except OSError as exc: + if exc.errno not in _RETRYABLE_ERRNOS: + raise + remaining = deadline - time.monotonic() + if remaining <= 0: + raise LockTimeout(wait) from exc + time.sleep(min(POLL_SECONDS, remaining)) + + +def release(fd: int) -> None: + """Release a lock taken with :func:`acquire`.""" + if BACKEND == "fcntl": + fcntl.flock(fd, fcntl.LOCK_UN) + else: + _msvcrt_byte0(fd, msvcrt.LK_UNLCK) + diff --git a/scripts/inquiry_branch_ledger.py b/scripts/inquiry_branch_ledger.py index 7355c1e0..301a07c7 100644 --- a/scripts/inquiry_branch_ledger.py +++ b/scripts/inquiry_branch_ledger.py @@ -54,7 +54,6 @@ import os import re import stat import sys -import time import unicodedata import uuid from contextlib import contextmanager @@ -62,12 +61,8 @@ from datetime import datetime from pathlib import Path, PurePosixPath from typing import Any, Callable, Iterable, Iterator, Mapping, NoReturn, Sequence -try: # POSIX is the supported durable-publication platform for this alpha. - import fcntl -except ImportError: # pragma: no cover - exercised only on non-POSIX hosts - fcntl = None # type: ignore[assignment] - try: # Dual-path import: script invocation vs package import under pytest. + import file_lock from research_workflow_profile import ( JCS_SAFE_INTEGER_MAX, ContractError as ProfileContractError, @@ -77,6 +72,7 @@ try: # Dual-path import: script invocation vs package import under pytest. validate_profile, ) except ImportError: # pragma: no cover - package-import path + from scripts import file_lock # type: ignore[no-redef] from scripts.research_workflow_profile import ( JCS_SAFE_INTEGER_MAX, ContractError as ProfileContractError, @@ -1668,7 +1664,10 @@ def _assert_safe_ledger_target(passport: Path, ledger: Path) -> None: @contextmanager def _transaction_lock(passport: Path, *, timeout_seconds: float = 30.0) -> Iterator[None]: - if fcntl is None: + # POSIX is the supported durable-publication platform for this alpha; the + # msvcrt backend has no CI coverage, so the ledger refuses rather than run + # its durable writes under an unverified lock (#845). + if file_lock.BACKEND != "fcntl": raise ContractError( "concurrency protection unavailable on this platform; refusing ledger access" ) @@ -1687,24 +1686,26 @@ def _transaction_lock(passport: Path, *, timeout_seconds: float = 30.0) -> Itera _require_regular_nonsymlink(lock_path, "transaction_lock") flags = os.O_RDWR | os.O_CREAT | getattr(os, "O_NOFOLLOW", 0) fd = os.open(lock_path, flags, 0o600) - deadline = time.monotonic() + normalized_timeout try: if not stat.S_ISREG(os.fstat(fd).st_mode): raise ContractError(f"transaction_lock: must be a regular file: {lock_path}") - while True: - try: - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) - break - except BlockingIOError: - if time.monotonic() >= deadline: - raise ContractError( - f"passport locked by another session: {passport}" - ) - time.sleep(0.05) + try: + file_lock.acquire(fd, exclusive=True, timeout=normalized_timeout) + except file_lock.LockTimeout: + raise ContractError( + f"passport locked by another session: {passport}" + ) from None + except BaseException: + os.close(fd) + raise + # Separate from acquisition: a LockTimeout raised inside the body must not + # be reported as "passport locked", and release runs only after a + # successful acquire (an unheld release is an error under msvcrt). + try: yield finally: try: - fcntl.flock(fd, fcntl.LOCK_UN) + file_lock.release(fd) finally: os.close(fd) diff --git a/scripts/review_criteria_binding.py b/scripts/review_criteria_binding.py index 1e56fa2f..8277044b 100644 --- a/scripts/review_criteria_binding.py +++ b/scripts/review_criteria_binding.py @@ -8,7 +8,6 @@ verdict. from __future__ import annotations import argparse -import fcntl import hashlib import json import os @@ -23,6 +22,11 @@ from typing import Any, Iterator, NoReturn from resolve_review_target_context import ContractError as ResolverError from resolve_review_target_context import resolve +try: # Dual-path import: sibling module on sys.path vs package import. + import file_lock +except ImportError: # pragma: no cover - package-import path + from scripts import file_lock # type: ignore[no-redef] + SCHEMA_VERSION = "review-criteria-binding/1.0" FINDINGS_VERSION = "constructive-review-findings/1.0" @@ -501,11 +505,22 @@ def _locked(path: Path) -> Iterator[None]: fd = os.open(lock_path, flags, 0o600) except OSError as exc: raise BindingError(f"cannot open lock {lock_path}: {exc}") from exc + # Acquisition and the guarded body are separate try blocks so a + # LockTimeout raised inside the body is never blamed on this lock, and the + # release runs only after a successful acquire (an unheld release is an + # error under msvcrt). + try: + file_lock.acquire(fd, exclusive=True, timeout=None) + except file_lock.LockTimeout as exc: + os.close(fd) + raise BindingError(f"manifest lock {lock_path}: {exc}") from exc + except BaseException: + os.close(fd) + raise try: - fcntl.flock(fd, fcntl.LOCK_EX) yield finally: - fcntl.flock(fd, fcntl.LOCK_UN) + file_lock.release(fd) os.close(fd) diff --git a/scripts/test_adjudication_activity.py b/scripts/test_adjudication_activity.py index c4d1a939..7426b240 100644 --- a/scripts/test_adjudication_activity.py +++ b/scripts/test_adjudication_activity.py @@ -3,7 +3,6 @@ from __future__ import annotations import copy -import fcntl import hashlib import json from pathlib import Path @@ -14,6 +13,7 @@ import jsonschema import pytest from scripts import adjudication_activity as activity +from scripts import file_lock from scripts import check_re_review_synthesis as re_review from scripts.test_check_re_review_synthesis import emit, scenario_g2d @@ -862,8 +862,11 @@ def test_symlink_artifact_and_lock_contention_fail_without_mutation( before = store.read_bytes() lock_path = store.with_name(store.name + ".lock") with lock_path.open("r+b") as lock_file: - fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) - code, stdout, stderr = _call(capsys, "validate", "--store", str(store)) + file_lock.acquire(lock_file.fileno(), exclusive=True, timeout=0) + try: + code, stdout, stderr = _call(capsys, "validate", "--store", str(store)) + finally: + file_lock.release(lock_file.fileno()) assert code == 7 assert stdout == "" assert "ERROR:LOCK" in stderr diff --git a/scripts/test_ars_mark_read.py b/scripts/test_ars_mark_read.py index 6c3d4011..e0f020e4 100644 --- a/scripts/test_ars_mark_read.py +++ b/scripts/test_ars_mark_read.py @@ -807,7 +807,7 @@ class TestLockedLedgerTransaction(unittest.TestCase): log_path = Path(tmp) / "passport_human_read_log.yaml" lock_path = ars_mark_read._ledger_lock_path(log_path) lock_fd = os.open(lock_path, os.O_RDWR | os.O_CREAT, 0o600) - ars_mark_read.fcntl.flock(lock_fd, ars_mark_read.fcntl.LOCK_EX) + ars_mark_read.file_lock.acquire(lock_fd, exclusive=True, timeout=0) try: with self.assertRaisesRegex( ars_mark_read.LedgerLockError, "timed out" @@ -817,7 +817,7 @@ class TestLockedLedgerTransaction(unittest.TestCase): ): self.fail("contended lock must not be acquired") finally: - ars_mark_read.fcntl.flock(lock_fd, ars_mark_read.fcntl.LOCK_UN) + ars_mark_read.file_lock.release(lock_fd) os.close(lock_fd) def test_cli_lock_failure_is_visible_without_traceback(self) -> None: diff --git a/scripts/test_file_lock.py b/scripts/test_file_lock.py new file mode 100644 index 00000000..7db92be6 --- /dev/null +++ b/scripts/test_file_lock.py @@ -0,0 +1,353 @@ +"""Tests for the shared advisory file-lock helper (#845). + +The real backend on the CI host is ``fcntl``. The ``msvcrt`` branch is +exercised through ``tests.fake_msvcrt`` (the documented Windows semantics the +helper depends on), so the Windows code path is tested for logic, not for +platform behaviour. Real Windows verification remains a manual step (#845). +""" +from __future__ import annotations + +import errno +import os +import subprocess +import sys +import threading +import time +from pathlib import Path +from typing import Iterator + +import pytest + +from scripts import file_lock +from tests.fake_msvcrt import FakeMsvcrt + + +def _open_lock(path: Path) -> int: + return os.open(path, os.O_RDWR | os.O_CREAT, 0o600) + + +@pytest.fixture +def lock_pair(tmp_path: Path) -> Iterator[tuple[int, int]]: + """Two independent descriptors on one lock file, closed afterwards.""" + lock = tmp_path / "x.lock" + a = _open_lock(lock) + b = _open_lock(lock) + try: + yield a, b + finally: + os.close(a) + os.close(b) + + +@pytest.fixture +def fake_windows(monkeypatch: pytest.MonkeyPatch) -> FakeMsvcrt: + fake = FakeMsvcrt() + monkeypatch.setattr(file_lock, "BACKEND", "msvcrt") + monkeypatch.setattr(file_lock, "msvcrt", fake, raising=False) + return fake + + +# -------------------------------------------------------------------------- +# Real backend (fcntl on the CI host) +# -------------------------------------------------------------------------- + + +def test_exclusive_lock_is_held_until_released(lock_pair: tuple[int, int]) -> None: + a, b = lock_pair + file_lock.acquire(a, exclusive=True, timeout=0) + with pytest.raises(BlockingIOError) as info: + file_lock.acquire(b, exclusive=True, timeout=0) + assert isinstance(info.value, file_lock.LockTimeout) + assert info.value.errno == errno.EAGAIN + file_lock.release(a) + file_lock.acquire(b, exclusive=True, timeout=0) + file_lock.release(b) + + +def test_bounded_wait_expires_after_timeout(lock_pair: tuple[int, int]) -> None: + a, b = lock_pair + file_lock.acquire(a, exclusive=True, timeout=0) + started = time.monotonic() + with pytest.raises(file_lock.LockTimeout, match="0.2"): + file_lock.acquire(b, exclusive=True, timeout=0.2) + assert 0.15 <= time.monotonic() - started < 3.0 + file_lock.release(a) + + +@pytest.mark.parametrize("timeout", [5.0, None]) +def test_waiting_acquire_succeeds_when_holder_releases( + lock_pair: tuple[int, int], timeout: float | None +) -> None: + a, b = lock_pair + file_lock.acquire(a, exclusive=True, timeout=0) + threading.Timer(0.15, file_lock.release, args=(a,)).start() + started = time.monotonic() + file_lock.acquire(b, exclusive=True, timeout=timeout) + assert time.monotonic() - started >= 0.1 + file_lock.release(b) + + +def test_negative_timeout_is_rejected(tmp_path: Path) -> None: + fd = _open_lock(tmp_path / "x.lock") + try: + with pytest.raises(ValueError): + file_lock.acquire(fd, exclusive=True, timeout=-1) + finally: + os.close(fd) + + +def test_helper_never_writes_to_the_lock_file(tmp_path: Path) -> None: + lock = tmp_path / "x.lock" + fd = _open_lock(lock) + try: + for exclusive in (True, False): + file_lock.acquire(fd, exclusive=exclusive, timeout=0) + file_lock.release(fd) + finally: + os.close(fd) + assert lock.stat().st_size == 0 + + +@pytest.mark.skipif(file_lock.BACKEND != "fcntl", reason="no shared locks") +def test_shared_locks_coexist_and_exclude_writers(tmp_path: Path) -> None: + lock = tmp_path / "x.lock" + r1, r2, w = (_open_lock(lock) for _ in range(3)) + try: + file_lock.acquire(r1, exclusive=False, timeout=0) + file_lock.acquire(r2, exclusive=False, timeout=0) + with pytest.raises(file_lock.LockTimeout): + file_lock.acquire(w, exclusive=True, timeout=0) + file_lock.release(r1) + file_lock.release(r2) + file_lock.acquire(w, exclusive=True, timeout=0) + with pytest.raises(file_lock.LockTimeout): + file_lock.acquire(r1, exclusive=False, timeout=0) + file_lock.release(w) + finally: + for fd in (r1, r2, w): + os.close(fd) + + +# -------------------------------------------------------------------------- +# msvcrt branch through the fake module +# -------------------------------------------------------------------------- + + +def test_windows_exclusive_contention_and_release( + lock_pair: tuple[int, int], fake_windows: FakeMsvcrt, tmp_path: Path +) -> None: + a, b = lock_pair + file_lock.acquire(a, exclusive=True, timeout=0) + with pytest.raises(file_lock.LockTimeout): + file_lock.acquire(b, exclusive=True, timeout=0) + file_lock.release(a) + file_lock.acquire(b, exclusive=True, timeout=0) + file_lock.release(b) + assert (tmp_path / "x.lock").stat().st_size == 0 + + +def test_windows_shared_request_degrades_to_exclusive( + lock_pair: tuple[int, int], fake_windows: FakeMsvcrt +) -> None: + r1, r2 = lock_pair + file_lock.acquire(r1, exclusive=False, timeout=0) + with pytest.raises(file_lock.LockTimeout): + file_lock.acquire(r2, exclusive=False, timeout=0) + file_lock.release(r1) + + +def test_windows_positions_descriptor_at_zero_before_locking( + tmp_path: Path, fake_windows: FakeMsvcrt +) -> None: + # The fake raises AssertionError if the descriptor is not at offset 0. + fd = _open_lock(tmp_path / "x.lock") + try: + os.lseek(fd, 7, os.SEEK_SET) + file_lock.acquire(fd, exclusive=True, timeout=0) + os.lseek(fd, 3, os.SEEK_SET) + file_lock.release(fd) + finally: + os.close(fd) + + +def test_windows_blocking_acquire_is_bounded( + lock_pair: tuple[int, int], fake_windows: FakeMsvcrt, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(file_lock, "WINDOWS_BLOCKING_WAIT_SECONDS", 0.2) + a, b = lock_pair + file_lock.acquire(a, exclusive=True, timeout=0) + started = time.monotonic() + with pytest.raises(file_lock.LockTimeout, match="0.2"): + file_lock.acquire(b, exclusive=True, timeout=None) + assert 0.15 <= time.monotonic() - started < 3.0 + file_lock.release(a) + + +def test_windows_blocking_acquire_succeeds_on_release( + lock_pair: tuple[int, int], fake_windows: FakeMsvcrt +) -> None: + a, b = lock_pair + file_lock.acquire(a, exclusive=True, timeout=0) + threading.Timer(0.15, file_lock.release, args=(a,)).start() + file_lock.acquire(b, exclusive=True, timeout=None) + file_lock.release(b) + + +def test_persistent_interruption_still_honours_the_deadline( + tmp_path: Path, fake_windows: FakeMsvcrt, monkeypatch: pytest.MonkeyPatch +) -> None: + def locking(fd: int, mode: int, nbytes: int) -> None: + raise InterruptedError(errno.EINTR, "Interrupted system call") + + monkeypatch.setattr(fake_windows, "locking", locking) + fd = _open_lock(tmp_path / "x.lock") + try: + started = time.monotonic() + with pytest.raises(file_lock.LockTimeout): + file_lock.acquire(fd, exclusive=True, timeout=0.05) + assert time.monotonic() - started < 3.0 + finally: + os.close(fd) + + +def test_windows_unexpected_oserror_propagates_unchanged( + tmp_path: Path, fake_windows: FakeMsvcrt, monkeypatch: pytest.MonkeyPatch +) -> None: + def locking(fd: int, mode: int, nbytes: int) -> None: + raise OSError(errno.EBADF, "Bad file descriptor") + + monkeypatch.setattr(fake_windows, "locking", locking) + fd = _open_lock(tmp_path / "x.lock") + try: + with pytest.raises(OSError) as info: + file_lock.acquire(fd, exclusive=True, timeout=1.0) + assert info.value.errno == errno.EBADF + assert not isinstance(info.value, file_lock.LockTimeout) + finally: + os.close(fd) + + +# -------------------------------------------------------------------------- +# Consumer import shape with fcntl absent (subprocess, fake msvcrt) +# -------------------------------------------------------------------------- + +_WINDOWS_SHAPE_SCRIPT = r''' +import importlib.abc, os, pathlib, sys, tempfile, threading, time + +class _BlockFcntl(importlib.abc.MetaPathFinder): + def find_spec(self, name, path=None, target=None): + if name == "fcntl": + raise ModuleNotFoundError("No module named 'fcntl'", name="fcntl") + return None + +sys.meta_path.insert(0, _BlockFcntl()) +sys.modules.pop("fcntl", None) +from tests.fake_msvcrt import FakeMsvcrt +sys.modules["msvcrt"] = FakeMsvcrt() +sys.path.insert(0, os.path.join(os.getcwd(), "scripts")) + +import file_lock, ars_mark_read, review_criteria_binding, adjudication_activity, inquiry_branch_ledger + +assert file_lock.BACKEND == "msvcrt" +for consumer in (ars_mark_read, review_criteria_binding, adjudication_activity, inquiry_branch_ledger): + assert consumer.file_lock is file_lock, consumer.__name__ +tmp = pathlib.Path(tempfile.mkdtemp()) + +# adjudication: a read degrades to an exclusive lock; the lock file stays empty +store = tmp / "activity.json" +with adjudication_activity._store_lock(store, exclusive=False): + store_lock = store.with_name(store.name + ".lock") + assert store_lock.stat().st_size == 0 +print("ADJUDICATION_READ_OK") + +# adjudication policy: a reader waits (bounded), a writer does not wait +adjudication_activity.READER_FALLBACK_WAIT_SECONDS = 0.3 +held = os.open(store_lock, os.O_RDWR | os.O_CREAT, 0o600) +file_lock.acquire(held, timeout=0) +started = time.monotonic() +try: + with adjudication_activity._store_lock(store, exclusive=True): + raise SystemExit("writer acquired a held lock") +except adjudication_activity.ActivityError: + assert time.monotonic() - started < 0.2, "writer must not wait" +started = time.monotonic() +try: + with adjudication_activity._store_lock(store, exclusive=False): + raise SystemExit("reader acquired a held lock") +except adjudication_activity.ActivityError: + assert 0.25 <= time.monotonic() - started < 3.0, "reader must wait the bounded window" +threading.Timer(0.1, file_lock.release, args=(held,)).start() +with adjudication_activity._store_lock(store, exclusive=False): + pass +os.close(held) +print("ADJUDICATION_POLICY_OK") + +# inquiry: the alpha refuses non-POSIX hosts +try: + with inquiry_branch_ledger._transaction_lock(tmp / "passport.yaml"): + raise SystemExit("inquiry did not refuse") +except inquiry_branch_ledger.ContractError as exc: + assert "unavailable on this platform" in str(exc) +print("INQUIRY_REFUSES_OK") + +# review-criteria binding: the blocking wait is bounded and reports BindingError +file_lock.WINDOWS_BLOCKING_WAIT_SECONDS = 0.1 +manifest = tmp / "m.json" +manifest.write_text("{}") +lock_path = manifest.with_name(f".{manifest.name}.lock") +fd = os.open(lock_path, os.O_RDWR | os.O_CREAT, 0o600) +file_lock.acquire(fd, timeout=0) +try: + with review_criteria_binding._locked(manifest): + raise SystemExit("binding lock acquired while held") +except review_criteria_binding.BindingError as exc: + assert "still held after 0.1s" in str(exc), str(exc) +file_lock.release(fd) +os.close(fd) +# a LockTimeout raised inside the body must leave _locked() as itself, not as +# BindingError blaming the manifest lock +inner = os.open(lock_path, os.O_RDWR | os.O_CREAT, 0o600) +try: + with review_criteria_binding._locked(manifest): + file_lock.acquire(inner, timeout=0) + raise SystemExit("inner acquire succeeded while the manifest lock is held") +except review_criteria_binding.BindingError as exc: + raise SystemExit(f"body LockTimeout was blamed on the manifest lock: {exc}") +except file_lock.LockTimeout: + pass +finally: + os.close(inner) +with review_criteria_binding._locked(manifest): + pass +print("BINDING_BOUNDED_OK") + +# ars-mark-read: bounded ledger lock with visible contention +log = tmp / "passport_human_read_log.yaml" +with ars_mark_read._ledger_lock(log): + try: + with ars_mark_read._ledger_lock(log, timeout_seconds=0.05): + raise SystemExit("nested ledger lock acquired") + except ars_mark_read.LedgerLockError as exc: + assert "timed out" in str(exc) +print("MARK_READ_OK") +''' + + +def test_consumers_import_and_behave_with_fcntl_absent() -> None: + repo_root = Path(__file__).resolve().parents[1] + result = subprocess.run( + [sys.executable, "-c", _WINDOWS_SHAPE_SCRIPT], + cwd=repo_root, + capture_output=True, + text=True, + timeout=60, + ) + assert result.returncode == 0, result.stderr + for marker in ( + "ADJUDICATION_READ_OK", + "ADJUDICATION_POLICY_OK", + "INQUIRY_REFUSES_OK", + "BINDING_BOUNDED_OK", + "MARK_READ_OK", + ): + assert marker in result.stdout, result.stdout diff --git a/scripts/test_inquiry_branch_ledger.py b/scripts/test_inquiry_branch_ledger.py index e8ce292d..bd2df7b1 100644 --- a/scripts/test_inquiry_branch_ledger.py +++ b/scripts/test_inquiry_branch_ledger.py @@ -40,10 +40,7 @@ from scripts.research_workflow_profile import ( seal_profile, ) -try: - import fcntl -except ImportError: # pragma: no cover - POSIX-only alpha coverage - fcntl = None # type: ignore[assignment] +from scripts import file_lock REPO_ROOT = Path(__file__).resolve().parent.parent @@ -1330,7 +1327,7 @@ def test_absent_and_orphan_pointer_states(tmp_path: Path) -> None: def test_shared_passport_sidecar_excludes_another_current_writer( tmp_path: Path, ) -> None: - if fcntl is None: + if file_lock.BACKEND != "fcntl": pytest.skip("POSIX advisory locking is unavailable") profile = _profile() passport = _passport(tmp_path / "passport.yaml") @@ -1338,7 +1335,7 @@ def test_shared_passport_sidecar_excludes_another_current_writer( lock_path.touch(mode=0o600) with lock_path.open("r+") as held: - fcntl.flock(held.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + file_lock.acquire(held.fileno(), exclusive=True, timeout=0) with pytest.raises(ContractError, match="passport locked by another session"): load_bound_ledger( passport, diff --git a/tests/fake_msvcrt.py b/tests/fake_msvcrt.py new file mode 100644 index 00000000..5bdb5d6d --- /dev/null +++ b/tests/fake_msvcrt.py @@ -0,0 +1,43 @@ +"""A stand-in for the Windows ``msvcrt`` module, for testing ``scripts/file_lock``. + +It models only the contract the helper relies on (Microsoft ``_locking``): + +* the lock covers ``nbytes`` from the current file position, so the helper + must position the descriptor at offset 0 before every call; +* one holder per (file, region): a second descriptor, or the same one again, + fails immediately with ``EACCES`` under ``LK_NBLCK``; +* ``LK_UNLCK`` by a non-holder fails with ``EACCES``; +* there is no shared mode. + +An instance is installed as ``sys.modules["msvcrt"]`` (or patched onto the +helper module); attribute access is all ``import msvcrt`` needs. Stdlib only, +so a subprocess can import it before anything else. +""" +from __future__ import annotations + +import errno +import os + + +class FakeMsvcrt: + LK_NBLCK = 2 + LK_UNLCK = 0 + + def __init__(self) -> None: + self.holders: dict[tuple[int, int], int] = {} + + def locking(self, fd: int, mode: int, nbytes: int) -> None: + info = os.fstat(fd) + key = (info.st_dev, info.st_ino) + if os.lseek(fd, 0, os.SEEK_CUR) != 0 or nbytes != 1: + raise AssertionError("helper must lock exactly byte 0") + if mode == self.LK_NBLCK: + if key in self.holders: + raise OSError(errno.EACCES, "Permission denied") + self.holders[key] = fd + elif mode == self.LK_UNLCK: + if self.holders.get(key) != fd: + raise OSError(errno.EACCES, "Permission denied") + del self.holders[key] + else: + raise AssertionError(f"helper must not use locking mode {mode}")