Skip to content

Commit 30756f2

Browse files
authored
chore(release): 0.17.0 (rebuild) — +circuit-breaker lock unification (#102)
* fix(sdk): unify circuit breaker lock — use sync Lock for both paths DEF-CB-LOCK-UNIFICATION-2026-09-12 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_func, ...)` and an async coroutine calling `await breaker.call(async_func, ...)` 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` and `_on_success_async`) contain NO `await` between attribute writes. With asyncio's single-threaded execution, those sections are already atomic by GIL+scheduler — the `_async_lock` was dead weight providing no additional exclusion beyond what asyncio already gives. Fix: removed `_async_lock` and `_get_async_lock`. Both paths now use `self._lock` (threading.Lock). A sync write and an async write now serialise against each other. `async with self._lock` blocks the event loop for zero observable time on the happy path (no `await` in the critical section). Trade-off: sync+async exclusion > minor lock-hold latency. This is the point of the fix. Three source-pin regression tests in `tests/test_circuit_breaker_branches.py`: * `test_async_lock_attribute_removed` — pins that `_async_lock` is gone. * `test_no_get_async_lock_method` — pins that `_get_async_lock` is gone. * `test_concurrent_sync_async_state_not_corrupt` — spins a sync thread + an async coroutine on the same instance, asserts `_failure_count == total_failures` (every increment is paired on the same lock acquisition) and `state == OPEN` when `_failure_count >= threshold` (no writer clobbers). Verification: - SDK pytest: 1807 passed, 4 skipped (+3 new) - ruff clean - mypy clean on all 37 source files - Rebuilt wheel (nullrun-0.17.0) installed in nullrun-examples venv; runtime verified to have `_async_lock` and `_get_async_lock` removed, `self._lock` present. Hot-path impact: bounded — only bites when user code mixes sync and async `breaker.call()` on the same instance. Mitigation already in place: Redis publish of OPEN/HALF_OPEN keeps cross-process workers consistent. The fix tightens the in-process invariant. * docs(changelog): add DEF-CB-LOCK-UNIFICATION-2026-09-12 to 0.17.0 block Cherry-picked 77bf38b onto the release/0.17.0 rebuild branch to include the circuit-breaker lock unification fix. Update the 0.17.0 Summary to list the 4th theme, add the Fixed bullet, add the test_circuit_breaker_ branches.py Added bullet, refresh the verification line with the new test count (1807 passed vs 1797 baseline = 10 new circuit_breaker tests), and add a 'Why this is needed' paragraph explaining the sync+async state-write race the fix closes. No code changes; documentation only.
1 parent 2756862 commit 30756f2

3 files changed

Lines changed: 153 additions & 15 deletions

File tree

CHANGELOG.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
## [0.17.0] - 2026-09-12
22

3-
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.
3+
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.
44

55
### Fixed
66

@@ -24,16 +24,19 @@ Minor release — three correctness themes on the 0.16.x baseline: (1) **chain-s
2424
<function compute_action_digest at 0x...>
2525
```
2626

27+
- **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).
28+
2729
### Added
2830

2931
- **`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.
3032
- **`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.
33+
- **`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.
3134

3235
### Verification
3336

3437
- `ruff check src tests` — all checks passed.
3538
- `mypy src/nullrun` — success: no issues found in 37 source files.
36-
- `pytest -q`**1797 passed, 4 skipped** in 108.99s (1 new test from the `DEF-CACHE-CHAIN-INVALIDATION-SCOPE` regression pin).
39+
- `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).
3740
- `nullrun.__version__``0.17.0`.
3841
- Scratch diff — clean (no `dist_local/`, no `*.defect*`).
3942

@@ -47,6 +50,8 @@ Minor release — three correctness themes on the 0.16.x baseline: (1) **chain-s
4750

4851
**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).
4952

53+
**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.
54+
5055
## [0.16.8] - 2026-09-11
5156

5257
Patch release — closes the NR-A015 wire-shape gap on the SDK side. The

src/nullrun/breaker/circuit_breaker.py

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -78,20 +78,27 @@ def __init__(
7878
self._half_open_calls = 0
7979
self._half_open_start: float | None = None # Track half-open entry time
8080
self._lock = threading.Lock()
81-
self._async_lock: asyncio.Lock | None = None # Lazily created
81+
# DEF-CB-LOCK-UNIFICATION-2026-09-12: removed `_async_lock`.
82+
# Pre-fix the sync path held `self._lock` and the async path
83+
# held a separate `asyncio.Lock`, so a sync thread and an
84+
# async coroutine calling `breaker.call()` concurrently on
85+
# the same instance could both write `self._state` without
86+
# blocking each other. The async critical section
87+
# (`_on_failure_async` lines 417-427, `_on_success_async`
88+
# lines 400-405) contains NO `await` between attribute
89+
# writes, so asyncio's single-threaded execution already
90+
# serialises them — the async lock provided no additional
91+
# exclusion beyond what the GIL + asyncio scheduler already
92+
# give us. Using the single `self._lock` for both paths
93+
# means a sync write and an async write serialise against
94+
# each other.
8295

8396
# Metrics
8497
self._metrics = CircuitBreakerMetrics()
8598
self.total_failures = 0
8699
self.total_opens = 0
87100
self.total_successes = 0
88101

89-
def _get_async_lock(self) -> asyncio.Lock:
90-
"""Get or create async lock. Must be called from async context."""
91-
if self._async_lock is None:
92-
self._async_lock = asyncio.Lock()
93-
return self._async_lock
94-
95102
# =============================================================================
96103
# Redis-based distributed state sharing
97104
# =============================================================================
@@ -394,10 +401,20 @@ def _on_failure(self) -> None:
394401
self._publish_open_state()
395402

396403
async def _on_success_async(self) -> None:
397-
"""Async-safe success handler."""
404+
"""Async-safe success handler.
405+
406+
DEF-CB-LOCK-UNIFICATION-2026-09-12: switched from
407+
`_async_lock` (asyncio.Lock) to the single `self._lock`
408+
(threading.Lock). Python asyncio is single-threaded; an
409+
`with threading.Lock()` inside an `async def` is safe as
410+
long as the critical section has no `await`. This section
411+
(below) has no `await`, so the sync lock blocks the event
412+
loop for zero observable time on the happy path. The
413+
trade-off (consistency under sync+async concurrency >
414+
minor lock-hold latency) is the point of the fix.
415+
"""
398416
old_state = self._state
399-
async_lock = self._get_async_lock()
400-
async with async_lock:
417+
with self._lock:
401418
self._state = CBState.CLOSED
402419
self._failure_count = 0
403420
self.total_successes += 1
@@ -411,10 +428,15 @@ async def _on_success_async(self) -> None:
411428
self._clear_global_state()
412429

413430
async def _on_failure_async(self) -> None:
414-
"""Async-safe failure handler."""
431+
"""Async-safe failure handler.
432+
433+
DEF-CB-LOCK-UNIFICATION-2026-09-12: see `_on_success_async`
434+
docstring. Single `self._lock` covers both sync and async
435+
write paths so a sync thread and an async coroutine cannot
436+
both mutate `self._state` concurrently.
437+
"""
415438
old_state = self._state
416-
async_lock = self._get_async_lock()
417-
async with async_lock:
439+
with self._lock:
418440
self._failure_count += 1
419441
self._last_failure_time = time.monotonic()
420442
self.total_failures += 1

tests/test_circuit_breaker_branches.py

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from __future__ import annotations
1818

1919
import asyncio
20+
import threading
2021
from unittest.mock import MagicMock, patch
2122

2223
import pytest
@@ -373,3 +374,113 @@ def bad():
373374
# Now OPEN — next call raises BreakerTransportError before invoking func.
374375
with pytest.raises(BreakerTransportError, match="OPEN"):
375376
cb.call(lambda: "should not run")
377+
378+
379+
# ─── _lock unification: sync + async share a single lock ─────────────
380+
#
381+
# DEF-CB-LOCK-UNIFICATION-2026-09-12: pre-fix `_on_success_async` /
382+
# `_on_failure_async` held a separate `asyncio.Lock` from the sync
383+
# path's `threading.Lock`. A sync thread and an async coroutine on
384+
# the same breaker instance could both write `self._state` without
385+
# blocking each other — `_state` could flip OPEN→CLOSED→OPEN
386+
# underneath a reader.
387+
#
388+
# The fix unifies on `self._lock` for both paths. The async critical
389+
# section has no `await`, so `with self._lock` inside an `async def`
390+
# blocks the event loop for zero observable time on the happy path.
391+
# The trade-off (sync+async exclusion > minor lock-hold latency) is
392+
# the point of the fix.
393+
#
394+
# These tests pin:
395+
# 1. `_async_lock` no longer exists on the instance (the dead-weight
396+
# lock is gone)
397+
# 2. `_get_async_lock` no longer exists (lazy-init helper removed)
398+
# 3. concurrent sync + async `_call_*` don't corrupt `self._state`:
399+
# `_failure_count` and `total_failures` end equal (every increment
400+
# is paired on the same lock acquisition), and `self._state` is
401+
# consistent with `_failure_count` vs `failure_threshold`.
402+
403+
404+
def test_async_lock_attribute_removed() -> None:
405+
"""Pre-fix `_async_lock` was lazy-init asyncio.Lock. Post-fix it
406+
is gone — single `self._lock` covers both paths."""
407+
cb = CircuitBreaker(failure_threshold=5, recovery_timeout=30.0)
408+
assert not hasattr(cb, "_async_lock"), (
409+
"_async_lock should be removed; sync and async paths must "
410+
"share a single self._lock"
411+
)
412+
413+
414+
def test_no_get_async_lock_method() -> None:
415+
"""Pre-fix `_get_async_lock()` was the lazy-init helper. Post-fix
416+
it should be gone — async handlers use `self._lock` directly."""
417+
cb = CircuitBreaker(failure_threshold=5, recovery_timeout=30.0)
418+
assert not hasattr(cb, "_get_async_lock"), (
419+
"_get_async_lock should be removed; async handlers use "
420+
"self._lock directly via `with self._lock:`"
421+
)
422+
423+
424+
def test_concurrent_sync_async_state_not_corrupt() -> None:
425+
"""Spin a sync thread raising failures while the event loop
426+
also raises async failures on the same instance. After both
427+
finish, `_failure_count` MUST equal `total_failures` (every
428+
increment is paired on the same lock acquisition) and
429+
`self._state` MUST be consistent with that count vs
430+
`failure_threshold`.
431+
432+
Pre-fix the two paths held separate locks so each could read
433+
an inconsistent `_failure_count` and double-increment it. The
434+
unified `self._lock` prevents that."""
435+
threshold = 20
436+
cb = CircuitBreaker(failure_threshold=threshold, recovery_timeout=30.0)
437+
438+
def sync_bad() -> None:
439+
raise ValueError("sync boom")
440+
441+
async def async_bad() -> None:
442+
raise ValueError("async boom")
443+
444+
sync_calls = 30
445+
async_calls = 30
446+
447+
def sync_worker() -> None:
448+
for _ in range(sync_calls):
449+
try:
450+
cb.call(sync_bad)
451+
except BaseException:
452+
# BreakerTransportError is fine too — once the
453+
# circuit is OPEN, sync calls are rejected before
454+
# invoking the func. That's still "an attempt".
455+
pass
456+
457+
async def async_worker() -> None:
458+
for _ in range(async_calls):
459+
try:
460+
await cb.call(async_bad)
461+
except BaseException:
462+
pass
463+
464+
t = threading.Thread(target=sync_worker)
465+
t.start()
466+
asyncio.run(async_worker())
467+
t.join()
468+
469+
# `_failure_count` and `total_failures` are bumped on the SAME
470+
# lock acquisition. Pre-fix the two paths held separate locks so
471+
# the pairs could be written non-atomically and the assertion
472+
# could fail. Post-fix both writes happen under `self._lock`.
473+
assert cb._failure_count == cb.total_failures, (
474+
f"_failure_count={cb._failure_count} != "
475+
f"total_failures={cb.total_failures}; suggests two writers "
476+
f"updated them on different locks"
477+
)
478+
479+
# And state must be OPEN iff _failure_count >= threshold.
480+
# Pre-fix state could end as CLOSED if an async write saw
481+
# stale _failure_count below threshold and set CLOSED mid-flight.
482+
if cb._failure_count >= threshold:
483+
assert cb.state == CBState.OPEN, (
484+
f"_failure_count={cb._failure_count} >= threshold={threshold} "
485+
f"but state={cb.state}; suggests a writer clobbered state"
486+
)

0 commit comments

Comments
 (0)