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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0131cZMWBPPeEFiqgEPFZ3X2

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Edward Cheng-I Wu
2026-09-11 14:47:26 +08:00
committed by GitHub
parent c7af8b9017
commit f1a57bbcab
15 changed files with 606 additions and 83 deletions
+2
View File
@@ -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.
+2
View File
@@ -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.
<details>
<summary>Alternative: npm install (deprecated)</summary>
+2
View File
@@ -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 jobWindows 行為仰賴貢獻者驗證(#843#845)。在 Windows 上,共享讀取鎖會降級為獨占鎖並短暫等待,無限期的鎖等待上限為 30 秒探究分支帳本alpha會拒絕執行。
<details>
<summary>替代方案npm 安裝(已棄用)</summary>
+4
View File
@@ -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"
+19 -4
View File
@@ -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)
+14 -46
View File
@@ -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:
@@ -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 = (
+114
View File
@@ -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)
+20 -19
View File
@@ -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)
+18 -3
View File
@@ -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)
+6 -3
View File
@@ -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
+2 -2
View File
@@ -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:
+353
View File
@@ -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
+3 -6
View File
@@ -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,
+43
View File
@@ -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}")