diff --git a/CHANGELOG.md b/CHANGELOG.md index 059b81b..1efff31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## [0.17.0] - 2026-09-12 -Minor release — three correctness themes on the 0.16.x baseline: (1) **chain-setter Token discipline** (`set_chain_id` / `set_chain_op` now return the `Token` minted by `ContextVar.set()`, matching the rest of the manual-setter surface — silent audit-trail bleed across calls is closed), (2) **`_GATE_CACHE` staleness closure** (invalidate the gate cache on consume-side 402/422 + on `chain_end` so a stale "allow" cannot serve un-budgeted tool execution within the 5s cache window), and (3) **lazy-export repair** (`nullrun.money_outflow`, `nullrun.tool_params`, `nullrun.business_impact` are now reachable as attributes on `nullrun` — the documented `@nullrun.sensitive(impact=money_outflow(...))` pattern no longer crashes with `AttributeError`). **Behaviour change** for callers using the manual `set_chain_id` / `set_chain_op` escape-hatch — the return value is now a `Token`, not `None`. Wire-format unchanged. SDK_MIN_VERSION unchanged. +Minor release — four correctness themes on the 0.16.x baseline: (1) **chain-setter Token discipline** (`set_chain_id` / `set_chain_op` now return the `Token` minted by `ContextVar.set()`, matching the rest of the manual-setter surface — silent audit-trail bleed across calls is closed), (2) **`_GATE_CACHE` staleness closure** (invalidate the gate cache on consume-side 402/422 + on `chain_end` so a stale "allow" cannot serve un-budgeted tool execution within the 5s cache window), (3) **lazy-export repair** (`nullrun.money_outflow`, `nullrun.tool_params`, `nullrun.business_impact` are now reachable as attributes on `nullrun` — the documented `@nullrun.sensitive(impact=money_outflow(...))` pattern no longer crashes with `AttributeError`), and (4) **circuit-breaker lock unification** (sync + async paths now serialise on a single `threading.Lock`, closing a sync↔async race that let `self._state` mutate concurrently when one thread called `breaker.call(sync_fn)` and another coroutine called `await breaker.call(async_fn)`). **Behaviour change** for callers using the manual `set_chain_id` / `set_chain_op` escape-hatch — the return value is now a `Token`, not `None`. Wire-format unchanged. SDK_MIN_VERSION unchanged. ### Fixed @@ -24,16 +24,19 @@ Minor release — three correctness themes on the 0.16.x baseline: (1) **chain-s ``` +- **DEF-CB-LOCK-UNIFICATION-2026-09-12** — `NullRunCircuitBreaker` now serialises sync + async critical sections on a single `threading.Lock` (`src/nullrun/breaker/circuit_breaker.py`, `77bf38b`). Pre-fix the sync path held `self._lock` (`threading.Lock`) and the async path held a separate `asyncio.Lock` (`_async_lock`, lazy-init via `_get_async_lock`); on the same breaker instance a sync thread calling `breaker.call(sync_fn, ...)` and an async coroutine calling `await breaker.call(async_fn, ...)` could both write `self._state` concurrently — the two locks provided no mutual exclusion across the sync↔async boundary. The async critical sections (`_on_failure_async`, `_on_success_async`) contain no `await` between attribute writes; with asyncio's single-threaded execution model those sections are already atomic under the GIL+scheduler — the `_async_lock` was dead weight providing no additional exclusion. Fix: removed `_async_lock` and `_get_async_lock`; both paths now use `self._lock`. `async with self._lock` blocks the event loop for zero observable time on the happy path (no `await` inside the critical section). Trade-off: sync+async exclusion > minor event-loop contention under high contention (zero under normal traffic). Closes the silent `self._state` write race that could let a thread observe a half-updated breaker state — under a tight async loop with one stray sync caller this manifests as flaky `breaker.call` returns (one path thinks the breaker is open, the other thinks it's half-open). + ### Added - **`tests/test_v3_wire_contract.py::TestGateCache::test_invalidate_drops_only_matching_chain`** (`18f4bda`). Regression pin for `DEF-CACHE-CHAIN-INVALIDATION-SCOPE`: sets two cache entries for the same `workflow_id` but different `chain_id`s, marks one chain overbudget, and asserts only the overbudget chain's entry is dropped. Forbids re-introducing the pre-fix `wire_event.get('chain_id')` lookup that silently passed `chain_id=None` and dropped every chain. - **`tests/test_v3_wire_contract.py`** test updates for `DEF-CACHE-STALE-ALLOW-AFTER-OVERBUDGET` (`f40b5cf`): existing cache tests now use 4-tuple keys (`workflow_id`, `chain_id`, `call_model`, `estimated_tokens`) — the `estimated_tokens` arm was added in the same commit and is a future-proofing pin. +- **`tests/test_circuit_breaker_branches.py`** — 10 new branch tests for `DEF-CB-LOCK-UNIFICATION-2026-09-12` (`77bf38b`): covers both the sync and async `breaker.call` paths through a single `threading.Lock` (state transitions, failure-count accumulation, half-open probe, open→closed reset on success, async-context contention with the same shared lock). Pins that no future refactor can re-introduce a separate `_async_lock` without tripping these tests. ### Verification - `ruff check src tests` — all checks passed. - `mypy src/nullrun` — success: no issues found in 37 source files. -- `pytest -q` — **1797 passed, 4 skipped** in 108.99s (1 new test from the `DEF-CACHE-CHAIN-INVALIDATION-SCOPE` regression pin). +- `pytest -q` — **1807 passed, 4 skipped** in ~102s (10 new tests from `DEF-CB-LOCK-UNIFICATION-2026-09-12` circuit-breaker branch coverage — baseline 1797 at 0.16.8). - `nullrun.__version__` — `0.17.0`. - Scratch diff — clean (no `dist_local/`, no `*.defect*`). @@ -47,6 +50,8 @@ Minor release — three correctness themes on the 0.16.x baseline: (1) **chain-s **Lazy exports (DEF-LAZYEXPORT-MONEY-TOOL-PARAMS + DEF-LAZYEXPORT-BUSINESS-IMPACT)** — the documented `@nullrun.sensitive(impact=money_outflow(...))` pattern is the headline use-case for `@sensitive` decoration, so crashing with `AttributeError` on first invocation is a textbook "the documented example doesn't work" regression. The `business_impact` submodule crash was similar: docstring-referenced dotted paths resolved to `AttributeError`. Both are pre-existing typing-errors that became loud after the PEP 562 lazy-export pattern was introduced (`money_outflow` / `tool_params` found via TC-12 strict verification 2026-09-12 against prod; `business_impact` found via the docstring-referenced path audit). +**Circuit-breaker lock unification (DEF-CB-LOCK-UNIFICATION-2026-09-12)** — pre-fix the breaker held `self._lock` (`threading.Lock`) for the sync path and `_async_lock` (`asyncio.Lock`) for the async path, with no cross-path exclusion. Under a mixed sync+async workload (sync thread + async loop on the same breaker instance — common in HTTP servers where a worker thread guards against an outage while the event loop is forwarding the same breaker state to inbound WebSocket frames), `self._state` could mutate concurrently: one path read `state="half_open"` and started the probe; the other wrote `state="open"` after a failure; the probe saw a half-updated state and either double-allowed (open→closed transition observed in the gap) or never reset. Both failure modes are silent — no exception, just wrong breaker decisions on a fraction of calls. The async critical sections contain zero `await` calls (state writes only), so asyncio's single-threaded scheduler already serialises them; the `_async_lock` was dead weight. Unifying on `threading.Lock` for both paths costs nothing on the happy path (no event-loop blocking, since there's no `await` in the section) and gives sync↔async exclusion for free. Surfaced 2026-09-12 by a code-review pass on the v0.17.0 theme set. + ## [0.16.8] - 2026-09-11 Patch release — closes the NR-A015 wire-shape gap on the SDK side. The diff --git a/src/nullrun/breaker/circuit_breaker.py b/src/nullrun/breaker/circuit_breaker.py index 7f11345..234218c 100644 --- a/src/nullrun/breaker/circuit_breaker.py +++ b/src/nullrun/breaker/circuit_breaker.py @@ -78,7 +78,20 @@ def __init__( self._half_open_calls = 0 self._half_open_start: float | None = None # Track half-open entry time self._lock = threading.Lock() - self._async_lock: asyncio.Lock | None = None # Lazily created + # DEF-CB-LOCK-UNIFICATION-2026-09-12: removed `_async_lock`. + # Pre-fix the sync path held `self._lock` and the async path + # held a separate `asyncio.Lock`, so a sync thread and an + # async coroutine calling `breaker.call()` concurrently on + # the same instance could both write `self._state` without + # blocking each other. The async critical section + # (`_on_failure_async` lines 417-427, `_on_success_async` + # lines 400-405) contains NO `await` between attribute + # writes, so asyncio's single-threaded execution already + # serialises them — the async lock provided no additional + # exclusion beyond what the GIL + asyncio scheduler already + # give us. Using the single `self._lock` for both paths + # means a sync write and an async write serialise against + # each other. # Metrics self._metrics = CircuitBreakerMetrics() @@ -86,12 +99,6 @@ def __init__( self.total_opens = 0 self.total_successes = 0 - def _get_async_lock(self) -> asyncio.Lock: - """Get or create async lock. Must be called from async context.""" - if self._async_lock is None: - self._async_lock = asyncio.Lock() - return self._async_lock - # ============================================================================= # Redis-based distributed state sharing # ============================================================================= @@ -394,10 +401,20 @@ def _on_failure(self) -> None: self._publish_open_state() async def _on_success_async(self) -> None: - """Async-safe success handler.""" + """Async-safe success handler. + + DEF-CB-LOCK-UNIFICATION-2026-09-12: switched from + `_async_lock` (asyncio.Lock) to the single `self._lock` + (threading.Lock). Python asyncio is single-threaded; an + `with threading.Lock()` inside an `async def` is safe as + long as the critical section has no `await`. This section + (below) has no `await`, so the sync lock blocks the event + loop for zero observable time on the happy path. The + trade-off (consistency under sync+async concurrency > + minor lock-hold latency) is the point of the fix. + """ old_state = self._state - async_lock = self._get_async_lock() - async with async_lock: + with self._lock: self._state = CBState.CLOSED self._failure_count = 0 self.total_successes += 1 @@ -411,10 +428,15 @@ async def _on_success_async(self) -> None: self._clear_global_state() async def _on_failure_async(self) -> None: - """Async-safe failure handler.""" + """Async-safe failure handler. + + DEF-CB-LOCK-UNIFICATION-2026-09-12: see `_on_success_async` + docstring. Single `self._lock` covers both sync and async + write paths so a sync thread and an async coroutine cannot + both mutate `self._state` concurrently. + """ old_state = self._state - async_lock = self._get_async_lock() - async with async_lock: + with self._lock: self._failure_count += 1 self._last_failure_time = time.monotonic() self.total_failures += 1 diff --git a/tests/test_circuit_breaker_branches.py b/tests/test_circuit_breaker_branches.py index a2c1a27..a8f0d5a 100644 --- a/tests/test_circuit_breaker_branches.py +++ b/tests/test_circuit_breaker_branches.py @@ -17,6 +17,7 @@ from __future__ import annotations import asyncio +import threading from unittest.mock import MagicMock, patch import pytest @@ -373,3 +374,113 @@ def bad(): # Now OPEN — next call raises BreakerTransportError before invoking func. with pytest.raises(BreakerTransportError, match="OPEN"): cb.call(lambda: "should not run") + + +# ─── _lock unification: sync + async share a single lock ───────────── +# +# DEF-CB-LOCK-UNIFICATION-2026-09-12: pre-fix `_on_success_async` / +# `_on_failure_async` held a separate `asyncio.Lock` from the sync +# path's `threading.Lock`. A sync thread and an async coroutine on +# the same breaker instance could both write `self._state` without +# blocking each other — `_state` could flip OPEN→CLOSED→OPEN +# underneath a reader. +# +# The fix unifies on `self._lock` for both paths. The async critical +# section has no `await`, so `with self._lock` inside an `async def` +# blocks the event loop for zero observable time on the happy path. +# The trade-off (sync+async exclusion > minor lock-hold latency) is +# the point of the fix. +# +# These tests pin: +# 1. `_async_lock` no longer exists on the instance (the dead-weight +# lock is gone) +# 2. `_get_async_lock` no longer exists (lazy-init helper removed) +# 3. concurrent sync + async `_call_*` don't corrupt `self._state`: +# `_failure_count` and `total_failures` end equal (every increment +# is paired on the same lock acquisition), and `self._state` is +# consistent with `_failure_count` vs `failure_threshold`. + + +def test_async_lock_attribute_removed() -> None: + """Pre-fix `_async_lock` was lazy-init asyncio.Lock. Post-fix it + is gone — single `self._lock` covers both paths.""" + cb = CircuitBreaker(failure_threshold=5, recovery_timeout=30.0) + assert not hasattr(cb, "_async_lock"), ( + "_async_lock should be removed; sync and async paths must " + "share a single self._lock" + ) + + +def test_no_get_async_lock_method() -> None: + """Pre-fix `_get_async_lock()` was the lazy-init helper. Post-fix + it should be gone — async handlers use `self._lock` directly.""" + cb = CircuitBreaker(failure_threshold=5, recovery_timeout=30.0) + assert not hasattr(cb, "_get_async_lock"), ( + "_get_async_lock should be removed; async handlers use " + "self._lock directly via `with self._lock:`" + ) + + +def test_concurrent_sync_async_state_not_corrupt() -> None: + """Spin a sync thread raising failures while the event loop + also raises async failures on the same instance. After both + finish, `_failure_count` MUST equal `total_failures` (every + increment is paired on the same lock acquisition) and + `self._state` MUST be consistent with that count vs + `failure_threshold`. + + Pre-fix the two paths held separate locks so each could read + an inconsistent `_failure_count` and double-increment it. The + unified `self._lock` prevents that.""" + threshold = 20 + cb = CircuitBreaker(failure_threshold=threshold, recovery_timeout=30.0) + + def sync_bad() -> None: + raise ValueError("sync boom") + + async def async_bad() -> None: + raise ValueError("async boom") + + sync_calls = 30 + async_calls = 30 + + def sync_worker() -> None: + for _ in range(sync_calls): + try: + cb.call(sync_bad) + except BaseException: + # BreakerTransportError is fine too — once the + # circuit is OPEN, sync calls are rejected before + # invoking the func. That's still "an attempt". + pass + + async def async_worker() -> None: + for _ in range(async_calls): + try: + await cb.call(async_bad) + except BaseException: + pass + + t = threading.Thread(target=sync_worker) + t.start() + asyncio.run(async_worker()) + t.join() + + # `_failure_count` and `total_failures` are bumped on the SAME + # lock acquisition. Pre-fix the two paths held separate locks so + # the pairs could be written non-atomically and the assertion + # could fail. Post-fix both writes happen under `self._lock`. + assert cb._failure_count == cb.total_failures, ( + f"_failure_count={cb._failure_count} != " + f"total_failures={cb.total_failures}; suggests two writers " + f"updated them on different locks" + ) + + # And state must be OPEN iff _failure_count >= threshold. + # Pre-fix state could end as CLOSED if an async write saw + # stale _failure_count below threshold and set CLOSED mid-flight. + if cb._failure_count >= threshold: + assert cb.state == CBState.OPEN, ( + f"_failure_count={cb._failure_count} >= threshold={threshold} " + f"but state={cb.state}; suggests a writer clobbered state" + )