Merge branch 'fix/manual-disable-cooldown-loop'

Stop the legacy manual-disable flag from evicting providers from rotations,
and stop a provider's own availability gate from renewing its own cooldown.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

aisbf/handlers.py

@@ -116,19 +116,17 @@ def _raise_if_provider_unavailable(handler, provider_id: str) -> None:
     """Reject a direct request with 503 only when the provider is genuinely
     unavailable, saying why.
 
-    is_rate_limited() collapses three very different states — the legacy global
-    manual-disable flag, the failure cooldown and the usage-limit cooldown — into
-    one boolean. Those are NOT equivalent for a direct call:
-
-    * Disabling a provider is now expressed per rotation entry (``enabled: false``
-      in that rotation's config), honoured by the rotation scan. The legacy global
-      manual-disable flag no longer gates anything: not direct calls, not rotation.
-      A direct request that names the provider/model explicitly must always go
-      through — disabling a provider for a rotation must never take it offline
-      across the whole API.
-    * A failure cooldown or a usage-limit cooldown is a real availability problem:
-      calling upstream would just fail again, so a direct call is still rejected,
-      with the reason logged and echoed in the response detail.
+    is_rate_limited() reports the failure cooldown and the usage-limit cooldown.
+    Both are real availability problems: calling upstream would just fail again, so
+    a direct call is rejected, with the reason logged and echoed in the response
+    detail.
+
+    Disabling a provider is expressed per rotation entry (``enabled: false`` in that
+    rotation's config), honoured by the rotation scan. The legacy global
+    manual-disable flag gates nothing — not direct calls, not rotation — and
+    is_rate_limited() no longer consults it. A direct request that names the
+    provider/model explicitly must always go through: disabling a provider for a
+    rotation must never take it offline across the whole API.
     """
     if not handler.is_rate_limited():
         return
@@ -139,8 +137,8 @@ def _raise_if_provider_unavailable(handler, provider_id: str) -> None:
     in_failure_cooldown = bool(cooldown_until and cooldown_until > now)
     in_usage_cooldown = bool(usage_until and usage_until > now)
 
-    # The only remaining reason is_rate_limited() can be true is the manual
-    # dashboard toggle, which is rotation-only — let the direct call proceed.
+    # Race guard: the cooldown can lapse between is_rate_limited() above and the
+    # re-read here. Nothing else makes is_rate_limited() true, so let the call proceed.
     if not in_failure_cooldown and not in_usage_cooldown:
         return
 
@@ -4257,6 +4255,24 @@ class RotationHandler:
                     tried_models.append(current_model)
                     continue
 
+                # A provider that is already in an availability cooldown raises
+                # Exception("Provider rate limited") from its own gate
+                # (BaseProviderHandler.is_rate_limited) before any upstream call.
+                # That error is our own state echoing back, not evidence the provider
+                # is unhealthy. Recording it would push disabled_until another full
+                # cooldown into the future on every attempt, so under steady traffic
+                # the cooldown could never elapse and the provider would stay disabled
+                # forever (observed in production: a failure counter in the hundreds
+                # against a threshold of 3, with the cooldown never once expiring).
+                # Fail over and leave the cooldown to expire on its own.
+                if _provider_in_availability_cooldown(handler):
+                    logger.warning(
+                        f"Provider {provider_id} is still in an availability cooldown; failing over "
+                        f"to the next provider (not counted as a failure, cooldown left to expire)"
+                    )
+                    tried_models.append(current_model)
+                    continue
+
                 # Transient/retryable error: count it against the provider's health.
                 handler.record_failure()
 

aisbf/providers/base.py

@@ -1565,9 +1565,18 @@ class BaseProviderHandler:
         return bool(getattr(self, '_manual_disabled', False))
 
     def is_rate_limited(self) -> bool:
-        # Manual disable takes precedence and never auto-expires.
-        if getattr(self, '_manual_disabled', False):
-            return True
+        # NB: the legacy global manual-disable flag is deliberately NOT consulted
+        # here. Disabling a provider is expressed per rotation entry
+        # (``enabled: false``) and honoured by the rotation scan; the global flag
+        # gates nothing. Treating it as "rate limited" was actively harmful: every
+        # provider's handle_request() calls this method and raises
+        # Exception("Provider rate limited") when it is true, before any upstream
+        # call. The rotation scan (which correctly ignores the manual flag) would
+        # still select the provider, that synthetic error was then counted as a
+        # transient failure, and three of them armed a real failure cooldown — so a
+        # manually-flagged provider was permanently evicted from every rotation it
+        # belonged to, reported to clients as an upstream 429.
+        # Use is_manually_disabled() when you actually want the manual flag.
         disabled_until = self.error_tracking.get('disabled_until')
         if disabled_until:
             if disabled_until > time.time():

aisbf/routes/dashboard/providers.py

@@ -2190,7 +2190,8 @@ async def api_providers_manual_status(request: Request):
             manual = handler.is_manually_disabled()
             statuses[pid] = {
                 "manual_disabled": manual,
-                # rate_limited includes the manual flag; expose the cooldown-only state too
+                # rate_limited is cooldown-only (failure or usage limit); the manual
+                # flag is reported separately above and no longer gates requests.
                 "rate_limited": handler.is_rate_limited(),
             }
         except Exception:

tests/providers/test_provider_availability_gating.py

+"""Regression tests for provider availability gating.
+
+Covers the production failure where a manually-flagged provider was permanently
+evicted from every rotation it belonged to and clients saw an upstream 429.
+"""
+
+import time
+
+from aisbf.handlers import _provider_in_availability_cooldown
+from aisbf.providers.base import BaseProviderHandler
+
+
+def _handler(manual=False, disabled_until=None, usage_until=None):
+    """A bare handler carrying only the attributes the availability gates read."""
+    h = object.__new__(BaseProviderHandler)
+    h.provider_id = 'codex'
+    h._manual_disabled = manual
+    h._usage_disabled_until = usage_until
+    h.error_tracking = {'disabled_until': disabled_until, 'failures': 0}
+    return h
+
+
+class TestIsRateLimited:
+    def test_manual_disable_does_not_rate_limit(self):
+        """The legacy global manual flag must not gate requests.
+
+        Every provider's handle_request() raises Exception("Provider rate limited")
+        when this is true, before any upstream call. The rotation scan deliberately
+        ignores the manual flag, so the provider still got selected, and that
+        synthetic error was then counted against its health.
+        """
+        assert _handler(manual=True).is_rate_limited() is False
+
+    def test_active_failure_cooldown_rate_limits(self):
+        h = _handler(disabled_until=time.time() + 300)
+        assert h.is_rate_limited() is True
+
+    def test_active_usage_cooldown_rate_limits(self):
+        h = _handler(usage_until=time.time() + 300)
+        assert h.is_rate_limited() is True
+
+    def test_elapsed_cooldown_reactivates_with_fresh_budget(self):
+        h = _handler(disabled_until=time.time() - 1)
+        h.error_tracking['failures'] = 3
+        assert h.is_rate_limited() is False
+        # Reactivated on a fresh budget, not left on a hair-trigger.
+        assert h.error_tracking['failures'] == 0
+
+    def test_no_state_is_not_rate_limited(self):
+        assert _handler().is_rate_limited() is False
+
+
+class TestAvailabilityCooldownPredicate:
+    """The rotation scan skips on this predicate; it must ignore the manual flag."""
+
+    def test_manual_disable_is_not_an_availability_cooldown(self):
+        assert _provider_in_availability_cooldown(_handler(manual=True)) is False
+
+    def test_failure_cooldown_is_an_availability_cooldown(self):
+        assert _provider_in_availability_cooldown(
+            _handler(disabled_until=time.time() + 300)
+        ) is True
+
+    def test_usage_cooldown_is_an_availability_cooldown(self):
+        assert _provider_in_availability_cooldown(
+            _handler(usage_until=time.time() + 300)
+        ) is True
+
+    def test_expired_cooldown_is_not_an_availability_cooldown(self):
+        assert _provider_in_availability_cooldown(
+            _handler(disabled_until=time.time() - 1)
+        ) is False
+
+
+class TestCooldownDoesNotSelfExtend:
+    """A provider in cooldown must not have that cooldown renewed by its own gate.
+
+    handle_request() raises "Provider rate limited" before any upstream call while a
+    cooldown is active. The rotation retry loop used to feed that straight into
+    record_failure(), pushing disabled_until another full cooldown into the future on
+    every attempt — under steady traffic the cooldown could never elapse (observed in
+    production at 447 failures against a threshold of 3).
+    """
+
+    def test_gate_error_leaves_cooldown_untouched(self):
+        disabled_until = time.time() + 300
+        h = _handler(disabled_until=disabled_until)
+
+        # The guard in the retry loop keys off this predicate to skip record_failure().
+        assert _provider_in_availability_cooldown(h) is True
+
+        # record_failure() is what would have extended the window; confirm that had it
+        # run, it would indeed have pushed the deadline out — i.e. the guard matters.
+        assert h.error_tracking['disabled_until'] == disabled_until