Stop the manual-disable flag from evicting providers from rotations

A provider carrying the legacy global manual-disable flag was permanently
evicted from every rotation it belonged to, and clients saw an upstream 429.
Observed in production on rotation/lisa: codex had valid credentials and was
never once called upstream all day.

Two independent defects combined:

1. is_rate_limited() honoured the manual flag, so every provider's
   handle_request() raised Exception("Provider rate limited") before any
   upstream call. The rotation scan deliberately ignores that flag, so the
   provider was still selected, and the synthetic error was then counted
   against its health — three of them armed a real failure cooldown. This
   contradicted the behaviour already documented in handlers.py: the flag
   gates nothing, neither direct calls nor rotation. Rotation membership is
   expressed per entry via `enabled: false`.

2. The rotation retry loop fed a provider's own availability-gate error into
   record_failure(), pushing disabled_until another full cooldown into the
   future on every attempt. Under steady traffic the cooldown could never
   elapse: the failure counter reached 447 against a threshold of 3, with no
   reactivation in any retained log.

is_rate_limited() is now cooldown-only; use is_manually_disabled() when the
manual flag is what you actually want. The dashboard already reports the two
states separately, so the toggle is unaffected.
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