From ea2736736cad41dc8af265046551b136db2c399f Mon Sep 17 00:00:00 2001 From: bagowix Date: Tue, 1 Sep 2026 14:28:46 +0400 Subject: [PATCH 1/2] fix: refuse a backoff the coordinated lane cannot honour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reopening a breaker with a shared `Storage` is the backend's decision, taken from `wait_duration_in_open` and its own clock. No failed-round count crosses the wire — `SharedState` carries mechanism rather than policy and has no field for one — so `wait_duration_backoff_multiplier` above 1.0 was read, validated, and then quietly dropped. The option looked enabled while every round waited exactly as long as the last, which is the trap the option exists to remove. Silence is the worst of the three answers available, so the combination is now a `ValueError` at construction: on `CircuitBreaker`, on `Registry`, and on the per-breaker `Registry.get(config=...)` override, which would otherwise slip past a registry built without one. Coordinated backoff is worth having and is tracked separately; refusing it loudly is what makes that discussion possible instead of leaving a dead option in the config. --- CHANGELOG.md | 11 +++++++++ docs/guides/configuration.md | 2 +- docs/guides/states.md | 11 +++++---- docs/llms-full.txt | 13 ++++++---- interlock/_engine.py | 27 +++++++++++++++++++++ interlock/config.py | 9 +++---- interlock/registry.py | 3 ++- tests/test_coordination.py | 46 ++++++++++++++++++++++++++++++++++++ 8 files changed, 107 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e1c4fa..cbc94e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,17 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed +- **A backoff the coordinated lane cannot honour is now refused instead of + ignored.** Reopening a breaker with a shared `Storage` is the backend's + decision, taken from `wait_duration_in_open` and its own clock; no + failed-round count crosses the wire, because `SharedState` carries mechanism + rather than policy and has no field for one. A + `wait_duration_backoff_multiplier` above `1.0` alongside a storage was + therefore read, validated and then silently dropped — the option looked + enabled while every round waited exactly as long as the last. It now raises + `ValueError` at construction, on `CircuitBreaker`, `Registry` and the + per-breaker `Registry.get(config=...)` override alike. + - **A probe that never reached the dependency no longer decides the round.** A `HALF_OPEN` probe asks one question — has the dependency recovered? — and every failure was taken as its answer, including failures that never left the diff --git a/docs/guides/configuration.md b/docs/guides/configuration.md index 0458bae..034d1d8 100644 --- a/docs/guides/configuration.md +++ b/docs/guides/configuration.md @@ -32,7 +32,7 @@ config = Config( | `permitted_calls_in_half_open` | `10` | Probe calls allowed while `HALF_OPEN`. | | `max_concurrent_probes` | `1` | Cap on **simultaneous** probes in `HALF_OPEN`. Must be in `[1, permitted_calls_in_half_open]`. | | `wait_duration_in_open` | `60.0` | Seconds to stay `OPEN` before the first probe is allowed. | -| `wait_duration_backoff_multiplier` | `1.0` | Multiplies the open wait after each consecutive failed probe round. `1.0` keeps it constant. Must be `>= 1`. | +| `wait_duration_backoff_multiplier` | `1.0` | Multiplies the open wait after each consecutive failed probe round. `1.0` keeps it constant. Must be `>= 1`, and must stay `1.0` on a breaker with a shared storage. | | `wait_duration_in_open_max` | `None` | Ceiling for the backed-off wait, in seconds. `None` leaves it uncapped; when set, must be `>= wait_duration_in_open`. | | `auto_transition` | `False` | When `True`, a timer moves the breaker `OPEN → HALF_OPEN` once the wait elapses, instead of waiting for the next call. See [States](states.md#proactive-transition-auto_transition). | | `window_type` | `COUNT_BASED` | `COUNT_BASED` or `TIME_BASED`. | diff --git a/docs/guides/states.md b/docs/guides/states.md index 2a2ff85..e869fe8 100644 --- a/docs/guides/states.md +++ b/docs/guides/states.md @@ -84,10 +84,13 @@ breaker = CircuitBreaker( The growing interval is also a signal in its own right: a breaker waiting out a blip looks nothing like one that has failed ten rounds in a row. -The backoff is local. Under a shared [storage](../integrations/redis.md) the -coordinated lane reopens on `wait_duration_in_open` and keeps no failed-round -count, so a coordinated breaker retries on the base wait no matter how many -rounds have failed. +The backoff is local, and deliberately refuses to pretend otherwise. Reopening +a coordinated breaker is decided by the backend from `wait_duration_in_open` and +its own clock, and no failed-round count crosses the wire, so a multiplier above +`1.0` alongside a shared [storage](../integrations/redis.md) raises `ValueError` +at construction rather than being accepted and ignored. Coordinated backoff is +[under discussion](https://github.com/bagowix/interlock/issues); until then a +coordinated breaker waits a constant interval. Growth stops after 64 consecutive failed rounds. Any sane multiplier has long since passed `wait_duration_in_open_max` by then, and an unbounded exponent diff --git a/docs/llms-full.txt b/docs/llms-full.txt index def849e..16b9549 100644 --- a/docs/llms-full.txt +++ b/docs/llms-full.txt @@ -1516,7 +1516,7 @@ config = Config( | `permitted_calls_in_half_open` | `10` | Probe calls allowed while `HALF_OPEN`. | | `max_concurrent_probes` | `1` | Cap on **simultaneous** probes in `HALF_OPEN`. Must be in `[1, permitted_calls_in_half_open]`. | | `wait_duration_in_open` | `60.0` | Seconds to stay `OPEN` before the first probe is allowed. | -| `wait_duration_backoff_multiplier` | `1.0` | Multiplies the open wait after each consecutive failed probe round. `1.0` keeps it constant. Must be `>= 1`. | +| `wait_duration_backoff_multiplier` | `1.0` | Multiplies the open wait after each consecutive failed probe round. `1.0` keeps it constant. Must be `>= 1`, and must stay `1.0` on a breaker with a shared storage. | | `wait_duration_in_open_max` | `None` | Ceiling for the backed-off wait, in seconds. `None` leaves it uncapped; when set, must be `>= wait_duration_in_open`. | | `auto_transition` | `False` | When `True`, a timer moves the breaker `OPEN → HALF_OPEN` once the wait elapses, instead of waiting for the next call. See [States](states.md#proactive-transition-auto_transition). | | `window_type` | `COUNT_BASED` | `COUNT_BASED` or `TIME_BASED`. | @@ -1658,10 +1658,13 @@ breaker = CircuitBreaker( The growing interval is also a signal in its own right: a breaker waiting out a blip looks nothing like one that has failed ten rounds in a row. -The backoff is local. Under a shared [storage](../integrations/redis.md) the -coordinated lane reopens on `wait_duration_in_open` and keeps no failed-round -count, so a coordinated breaker retries on the base wait no matter how many -rounds have failed. +The backoff is local, and deliberately refuses to pretend otherwise. Reopening +a coordinated breaker is decided by the backend from `wait_duration_in_open` and +its own clock, and no failed-round count crosses the wire, so a multiplier above +`1.0` alongside a shared [storage](../integrations/redis.md) raises `ValueError` +at construction rather than being accepted and ignored. Coordinated backoff is +[under discussion](https://github.com/bagowix/interlock/issues); until then a +coordinated breaker waits a constant interval. Growth stops after 64 consecutive failed rounds. Any sane multiplier has long since passed `wait_duration_in_open_max` by then, and an unbounded exponent diff --git a/interlock/_engine.py b/interlock/_engine.py index e519ee9..e8510b3 100644 --- a/interlock/_engine.py +++ b/interlock/_engine.py @@ -84,6 +84,32 @@ def validate_unreachable_exceptions( return types +def validate_backoff_support(*, config: Config, storage: Storage | AsyncStorage | None) -> None: + """Refuse a backoff the coordinated lane cannot honour. + + Reopening a coordinated breaker is the backend's decision, taken from + ``wait_duration_in_open`` and its own clock, and no failed-round count + crosses the wire — ``SharedState`` carries mechanism, not policy, and has no + field for one. A multiplier set alongside a storage would therefore be read, + validated, and then quietly ignored: the option would look enabled while + every round waited exactly as long as the last. + + Silence is the worst of the three possible answers here, so this is an + error. Coordinated backoff is tracked as a separate change; until then the + honest options are a constant wait or a local breaker. + + Raises: + ValueError: If a backoff multiplier is set on a coordinated breaker. + """ + if storage is not None and config.wait_duration_backoff_multiplier != 1.0: + raise ValueError( + 'wait_duration_backoff_multiplier has no effect on a breaker with shared ' + 'storage: reopening is decided by the backend from wait_duration_in_open, ' + 'and no failed-round count is shared. Leave it at 1.0, or drop the storage ' + f'to run this breaker locally. Got {config.wait_duration_backoff_multiplier!r}.' + ) + + @dataclass(frozen=True, slots=True) class Admission: """What ``_admit`` granted: the era it happened in, and probe provenance.""" @@ -128,6 +154,7 @@ def __init__( self._clock = clock self._classifier = classifier if classifier is not None else DefaultFailureClassifier() self._unreachable_exceptions = validate_unreachable_exceptions(unreachable_exceptions) + validate_backoff_support(config=config, storage=storage) self._listener = listener self._machine = StateMachine( config=config, diff --git a/interlock/config.py b/interlock/config.py index 1440f55..b785e9c 100644 --- a/interlock/config.py +++ b/interlock/config.py @@ -35,10 +35,11 @@ class Config: constant wait then retries forever at full rate, and the growing interval is itself the signal that the dependency is not merely slow to recover. - The backoff is a local decision: with a shared ``Storage`` the coordinated - lane asks the backend to reopen after ``wait_duration_in_open``, and the - failed-round count lives in no shared state, so a coordinated breaker retries - on the base wait however many rounds have failed. + The backoff is a local decision. Reopening a coordinated breaker is the + backend's, taken from ``wait_duration_in_open`` and its own clock, and no + failed-round count crosses the wire — so a multiplier above ``1.0`` is + **refused** on a breaker with a shared ``Storage`` rather than accepted and + ignored. ``auto_transition`` opts into a timer that proactively moves a breaker from ``OPEN`` to ``HALF_OPEN`` once ``wait_duration_in_open`` elapses, emitting the diff --git a/interlock/registry.py b/interlock/registry.py index fcef294..6ee8e4b 100644 --- a/interlock/registry.py +++ b/interlock/registry.py @@ -9,7 +9,7 @@ from contextlib import AsyncExitStack, ExitStack from interlock._clock import SystemClock -from interlock._engine import validate_unreachable_exceptions +from interlock._engine import validate_backoff_support, validate_unreachable_exceptions from interlock._initial_state import validate_initial_state from interlock.breaker import CircuitBreaker from interlock.config import Config @@ -75,6 +75,7 @@ def __init__( self._listener = listener self._storage = storage self._unreachable_exceptions = validate_unreachable_exceptions(unreachable_exceptions) + validate_backoff_support(config=self._config, storage=storage) self._breakers: dict[str, CircuitBreaker] = {} self._lock = threading.Lock() diff --git a/tests/test_coordination.py b/tests/test_coordination.py index 7ff3d93..fbc7d4e 100644 --- a/tests/test_coordination.py +++ b/tests/test_coordination.py @@ -1487,3 +1487,49 @@ def test__shared_view__closed_at_the_same_version__does_not_reset_the_local_mach # Same version: a duplicate or out-of-order delivery, not a recovery. assert breaker.state is State.OPEN + + +def test__breaker__backoff_with_shared_storage__rejected(fake_clock: FakeClock) -> None: + """The combination has to fail loudly: the coordinated lane cannot honour a backoff. + + Reopening is decided by the backend from ``wait_duration_in_open``, and no + failed-round count crosses the wire, so a multiplier set here would be read, + validated and then quietly ignored — the exact trap the option exists to + remove. + """ + with pytest.raises(ValueError, match='wait_duration_backoff_multiplier'): + CircuitBreaker( + name='payments', + config=Config(wait_duration_backoff_multiplier=2.0), + clock=fake_clock, + storage=InMemoryStorage(clock=fake_clock), + ) + + +def test__registry__backoff_with_shared_storage__rejected(fake_clock: FakeClock) -> None: + """Fail at registry construction, not at the first ``get`` that happens to run.""" + with pytest.raises(ValueError, match='wait_duration_backoff_multiplier'): + Registry( + config=Config(wait_duration_backoff_multiplier=2.0), + clock=fake_clock, + storage=InMemoryStorage(clock=fake_clock), + ) + + +def test__registry__backoff_in_a_per_breaker_config__rejected(fake_clock: FakeClock) -> None: + """``get`` takes a config override, and that route must not slip past the guard.""" + registry = Registry(clock=fake_clock, storage=InMemoryStorage(clock=fake_clock)) + + with pytest.raises(ValueError, match='wait_duration_backoff_multiplier'): + registry.get('payments', config=Config(wait_duration_backoff_multiplier=2.0)) + + +def test__breaker__backoff_without_storage__accepted(fake_clock: FakeClock) -> None: + """Only the combination is refused; a local breaker keeps its backoff.""" + breaker = CircuitBreaker( + name='payments', + config=Config(wait_duration_backoff_multiplier=2.0), + clock=fake_clock, + ) + + assert breaker.state is State.CLOSED From 331b0b1fa0849970a9e0976c60e90747d887ef8c Mon Sep 17 00:00:00 2001 From: bagowix Date: Tue, 1 Sep 2026 14:47:21 +0400 Subject: [PATCH 2/2] docs: document the ValueError the storage guard raises `CircuitBreaker`, `Registry` and `Registry.get` can all refuse a backoff asked for alongside a storage, and none of their `Raises` sections said so. The override route through `get(config=...)` had no `Raises` section at all, which is the one a caller is least likely to expect. --- interlock/breaker.py | 6 +++++- interlock/registry.py | 11 ++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/interlock/breaker.py b/interlock/breaker.py index ac0c6c6..278f565 100644 --- a/interlock/breaker.py +++ b/interlock/breaker.py @@ -72,7 +72,11 @@ class CircuitBreaker: Raises: TypeError: If ``unreachable_exceptions`` is not a tuple of ``Exception`` subclasses. - ValueError: If ``initial_state`` is not a supported stable state. + ValueError: If ``initial_state`` is not a supported stable state, or if + ``config`` asks for a backoff (``wait_duration_backoff_multiplier`` + above ``1.0``) alongside a ``storage``: reopening is then the + backend's decision and no failed-round count is shared, so the + backoff could not be honoured. """ def __init__( diff --git a/interlock/registry.py b/interlock/registry.py index 6ee8e4b..a624d66 100644 --- a/interlock/registry.py +++ b/interlock/registry.py @@ -53,7 +53,11 @@ class Registry: Raises: TypeError: If ``unreachable_exceptions`` is not a tuple of ``Exception`` subclasses. - ValueError: If ``initial_state`` is not a supported stable state. + ValueError: If ``initial_state`` is not a supported stable state, or if + ``config`` asks for a backoff (``wait_duration_backoff_multiplier`` + above ``1.0``) alongside a ``storage``: reopening is then the + backend's decision and no failed-round count is shared, so the + backoff could not be honoured. """ def __init__( @@ -89,6 +93,11 @@ def get(self, name: str, *, config: Config | None = None) -> CircuitBreaker: Returns: The cached or newly created breaker. + + Raises: + ValueError: If ``config`` asks for a backoff on a registry that holds a + storage. The override route reaches the same constraint as the + registry's own config. """ # A hit reads the dict without the lock: this is the per-request path of # every transport integration, and a breaker is never replaced or