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/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/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..a624d66 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 @@ -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__( @@ -75,6 +79,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() @@ -88,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 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