Repository navigation
llm: degradation store cannot distinguish a dead credential from a transient failure — dead tokens retry every TTL forever #15022
Description
Activity
PR #15320 delivers the classifier. Two criteria are not met, and one of them is blocked by a contradiction in the existing design.
Verified the PR's central claims directly rather than taking them on trust:
TokenExpiredErrorhas exactly two raise sites —provider_auth.py:334(OAuth: expired with no refresh token) and:508(Session: expired, reconnect). Both are pure local checks against vault state with no network round trip.OAuthAuth._refresh's HTTP failures raise plainProviderAuthErrorand are untouched. So the classifier cannot misfire on a timeout or a flaky connection, and the "permanent" label is earned rather than guessed. This was the thing most worth checking, because the whole low-risk argument rests on it.- The fix is currently inert.
_get_auth_token()is defined atbase_provider.py:326and referenced by nothing except tests — every concrete provider (openai,anthropic,groq,mistral) readssettings["api_key"]directly. SoTokenExpiredErrorcannot fire on any live request today. The PR is correct infrastructure for a path nothing yet takes, which is consistent with Umbrella: provider quota headroom + multi-account failover — finish the declared-but-unbuilt quota monitor #15021's own "declared but unbuilt" framing, but it means no runtime behaviour changes on merge.
Criteria status
AC Verdict 2. Transient failure keeps today's behaviour (TTL, then retry) MET — only TokenExpiredErrordiverts; everything else stays on the existing path3. TokenExpiredErrorresults inneeds_reauth, not generic degradationMET at the store, but see the inertness note — no strategy is wired to a live request yet 5. Clearing is explicit — a successful auth clears the state MET — new clear(), called from_get_auth_token()on a successful vault-backedresolve_token()6. degraded_entries()reports the causeMET 1. A needs_reauthprovider is skipped by the registryNOT MET — see below 4. Exactly one alert per credential per cooldown tier, via alert_cooldownnot verified 7. Redis-unavailable fallback preserves the new behaviour not verified 8. Tests cover transient expiry, non-expiry, explicit clear, Redis-down partially Why AC1 cannot be satisfied without a decision
mark_degraded(provider, cause=NEEDS_REAUTH)writes the provider-only key.provider_registry.py:464asksis_degraded(name, model_name)— with a model — andis_degraded(provider_degradation.py:93) builds_make_key(provider, model)and checks that key alone. A provider-level mark is therefore invisible to every model-scoped selection, which is the path the registry actually uses.That is not an oversight in the PR. It is an explicitly asserted invariant in
test_provider_degradation.py:111:await store.mark_degraded("groq") assert await store.is_degraded("groq") is True # Model-scoped key must NOT match a provider-only mark. assert await store.is_degraded("groq", "llama3") is False
So the codebase asserts that a provider-level mark does not imply its models, while AC1 requires exactly that implication for
needs_reauth. Both cannot hold. The contradiction is worth stating plainly because the cheap fix — makingis_degradedfall back to the provider key — would silently overturn a deliberate, tested invariant, and I am not willing to do that inside a review on my own judgement.The distinction that probably resolves it: a transient provider mark arguably should not condemn every model, but a dead credential logically does — one credential authenticates all of that provider's models. So the right change is likely narrower than the cheap fix: let a
NEEDS_REAUTHmark imply its models, while leaving transient marks key-independent as today. That preserves the tested invariant for the case it was written for.I have not made that change. It needs a decision, and it is this issue's, not the PR's.
Blast radius while AC1 is unmet
Bounded, because the local-only check still fails fast on every attempt — no wasted network round trip, and no credential is wrongly parked. What is missing is the pre-flight registry skip for model-scoped requests, so a dead credential is rediscovered per call instead of being routed around. Given the path is inert today, nothing regresses.
Keeping this open. The PR is worth merging as correct, additive infrastructure with honest limits; it does not close this issue.
PR #15320 merged. The classifier and its tests are in; this issue stays open on the criteria recorded above — AC1 (registry skip) is blocked by the tested key-independence invariant and needs the
needs-decisioncall, and AC4/AC7 are unverified.Also unchanged: the path is inert until a concrete provider routes through
_get_auth_token(). Nothing regresses, and nothing yet exercises it.The
needs-decisioncall, stated — it was labelled but never framed, so there was nothing to answer.What is already delivered
llm_shared/provider_degradation.pyon mergedmainhas the cause split:DegradationCause.NEEDS_REAUTHat:83, non-expiring by construction (ttl = None if cause is DegradationCause.NEEDS_REAUTH else _DEGRADATION_TTL_SECONDS,:127), cause-awaredegraded_entries(), the alert tier at:71, andtests/test_provider_degradation_reauth.pycovering non-expiry, explicit clear, cause reporting and theTokenExpiredErrorwiring.What AC1 collides with
AC1: "A provider marked
needs_reauthis skipped by the registry and does not silently return to the pool after the degradation TTL."provider_registry.py:458-471deliberately does the opposite in one case:# All candidates are checked up-front so we can detect the all-degraded # edge case and fall through rather than returning None. ... all_degraded = len(candidates) > 0 and degraded_set == set(candidates) if all_degraded: logger.warning("degradation: all %d candidates degraded — proceeding anyway", ...)
and the skip at
:474is guarded byand not all_degraded.So when every candidate is
needs_reauth, the registry uses them anyway. That is #11519's deliberate availability choice — never returnNone— and AC1 cannot be satisfied without changing it. The two are not reconcilable by implementation.The asymmetry that decides it
The
all_degradedfall-through is sound forTRANSIENT, because a transient mark may be stale: the socket error was 200 seconds ago and the provider may well be healthy, so trying beats refusing. That reasoning does not transfer toneeds_reauth. A revoked or expired-beyond-refresh credential is not stale information — it is known-dead until an explicit re-auth clears it. Retrying is not an optimistic gamble, it is a guaranteed failed round-trip plus the caller's fallback latency, which is the exact cost this issue was filed about.Options
A — exempt
needs_reauthfrom the fall-through. Never select it; returnNonewhen it is all there is.
Gets AC1 literally. Reverses #11519's guarantee for this case: a caller that gets a slow failure today gets a fast one, which is a different error shape and may need handling.B — keep the fall-through.
needs_reauthchanges TTL behaviour and alerting only, never selection.
Preserves #11519 unchanged. AC1 as written becomes false and should be amended — the dead credential is still retried, just whenever it is the only option rather than every 300 s.C — exempt it, and fail with a typed, distinguishable error rather than a bare
None: callers can tell "all credentials need re-auth" from "no capacity".Recommendation: C
It gets AC1's intent — a known-dead credential is never silently retried — while answering the reason #11519 chose the fall-through, which was not "always try something" for its own sake but "do not hand the caller an unexplained
None". A typed error satisfies both. A is correct in behaviour and leaves callers worse informed than today; B is honest but keeps the defect and should then amend AC1 rather than leave it unticked forever.Whichever is chosen, AC4 and AC7 remain unverified independently of this, and the path stays inert until a concrete provider routes through
_get_auth_token().Framing only — no code change, and the choice is the owner's.
Parent: #15021
Problem
llm_shared/provider_degradation.pystores one boolean per(provider, model)with a singleenv-backed TTL (
AUTOBOT_PROVIDER_DEGRADATION_TTL_SECONDS, default 300 s). An expired refresh tokenand a transient socket error produce the identical state and the identical recovery.
The consequence: a credential that is permanently dead — revoked, expired beyond refresh, org access
removed — is retried every 300 s, forever. Nobody is told. Every retry costs a failed round-trip and
the caller pays the fallback latency, indefinitely.
TokenExpiredErroralready exists (llm_shared/provider_auth.py:85) and is raised by the authstrategies, but nothing routes it into the degradation store, so the information is discarded at the
moment it is most useful.
Scope
llm_shared/provider_degradation.py— the stored Redis value gains a cause.needs_reauthisnon-expiring: it clears on a successful re-auth, not on a TTL.
llm_shared/base_provider.py— catchTokenExpiredError/ auth failures and mark with theneeds_reauthcause rather than the generic degraded path.needs_reauthtransition raises an operator alert through the existingautobot_shared/alert_cooldown.py(multi-tier progressive cooldown, Feature: Multi-tier alert cooldown system to prevent notification fatigue #1948). Do not build a secondnotification path.
is_degraded()keeps its current signature and semantics for existing callers; the cause isadditive.
Acceptance criteria
needs_reauthis skipped by the registry and does not silently return tothe pool after the degradation TTL.
TokenExpiredErrorfrom any auth strategy (ApiKeyAuth,OAuthAuth,DeviceCodeAuth,SessionAuth) results inneeds_reauth, not generic degradation.needs_reauthtransition emits exactly one alert per credential per cooldown tier — verifiedagainst
alert_cooldown, not a new de-dup set.degraded_entries()reports the cause so the observability path can show why.provider_degradation_test.pycover: transient expiry,needs_reauthnon-expiry,explicit clear, and the Redis-down path.
Implementation Order
Wave: 1
Depends on: none
Unblocks: #15021 Task 4