chore(release): 0.17.0 (rebuild) — +circuit-breaker lock unification

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
   <function compute_action_digest at 0x...>
   ```
 
+- **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

src/nullrun/breaker/circuit_breaker.py

@@ -78,20 +78,27 @@ 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()
         self.total_failures = 0
         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

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"
+        )