fix(ci): tolerate an isolated provider outage in provider-catalog-sync

.github/workflows/provider-catalog-sync.yml

@@ -97,15 +97,29 @@ jobs:
           python -m contextual_orchestrator.provider_catalog_bootstrap --model-limit 24 > provider-bootstrap-report.json
           python - <<'PY'
           import json
+          import os
           from pathlib import Path
 
           report = json.loads(Path('provider-bootstrap-report.json').read_text(encoding='utf-8'))
-          from contextual_orchestrator.provider_bootstrap import PROVIDER_CREDENTIAL_NAMES
+          # The credential-inventory verdict (hard-fail vs. tolerate-and-warn vs.
+          # fully healthy) is real, tested production logic -- not reimplemented
+          # here -- so its behavior has the package's own regression coverage
+          # (tests/test_provider_catalog_bootstrap*.py), not just a string match
+          # against this YAML. See its docstring for exactly which gaps still
+          # hard-fail (an unconfigured secret, an unexplained rollback, an
+          # authentication failure, or more than one provider degraded at once)
+          # versus the single isolated-provider-outage case
+          # `bootstrap_provider_catalog_runtime`'s own docstring says the design
+          # already tolerates (last-known-good models retained, pool still served).
+          from contextual_orchestrator.provider_catalog_bootstrap import (
+              evaluate_provider_credential_inventory,
+          )
 
-          expected = set(PROVIDER_CREDENTIAL_NAMES)
-          registered = set(report['registered_credentials'])
-          if registered != expected:
-              raise SystemExit(f'credential inventory mismatch: {sorted(expected - registered)}')
+          verdict = evaluate_provider_credential_inventory(report, os.environ)
+          if not verdict.ok:
+              raise SystemExit(verdict.hard_fail_reason)
+          if verdict.warning_message:
+              print(f'::warning title=Provider catalog degraded::{verdict.warning_message}')
           if report['catalog_backend'] != 'postgres':
               raise SystemExit('provider catalog is not PostgreSQL-backed')
           if report['catalog_model_count'] < 1 or report['eligible_model_count'] < 1:
@@ -115,7 +129,7 @@ jobs:
           if report['enabled_agent_ids'] or report['durable_agent_pool']:
               raise SystemExit('ephemeral Actions sync must not claim agent-pool activation')
           print(json.dumps({
-              'registered_credentials': sorted(registered),
+              'registered_credentials': sorted(report['registered_credentials']),
               'live_discovered_model_count': report['live_discovered_model_count'],
               'catalog_model_count': report['catalog_model_count'],
               'last_known_good_model_count': report['last_known_good_model_count'],

contextual_orchestrator/provider_catalog_bootstrap.py

@@ -28,6 +28,7 @@
     DiscoveredModel,
     ProviderDiscoveryError,
     ProviderModelSource,
+    _provider_family,
     agent_id_for,
     discover_all_models,
     refresh_price_book,
@@ -37,6 +38,7 @@
     analyze_discovered_privacy_policies,
 )
 from .provider_bootstrap import (
+    PROVIDER_CREDENTIAL_NAMES,
     ProviderBootstrapError,
     _synchronize_durable_agent_pool,
     collect_provider_credentials,
@@ -55,6 +57,56 @@
 
 _CATALOG_REFRESH_EVIDENCE_LOCK = threading.Lock()
 
+# The classification a discovery failure collapses to for the
+# credential-rollback report. Classification defaults to non-tolerable
+# (``UNKNOWN_FAILURE_CLASSIFICATION``): only a code that is unambiguously one
+# specific, self-resolving condition is ever promoted out of it. An
+# authentication failure (a credential the provider itself rejects) is never
+# treated as an isolated, self-resolving outage: left alone, a genuinely
+# invalid/expired/revoked credential would stay silently disabled forever
+# with the rollback path quietly excusing it every run. A transient failure
+# is narrowed to conditions standard retry semantics call retryable -- a rate
+# limit (429), a request-timeout status (408), any 5xx server error, or a
+# below-HTTP-layer timeout/transport failure -- and nothing else. A
+# persistent 4xx other than 401/403 (400 Bad Request, 404 Not Found, ...) or
+# an unparseable response almost always means a genuinely broken
+# integration -- a wrong endpoint, a malformed request shape, or a provider
+# that moved/retired the API -- not a blip that clears on its own, so it is
+# deliberately left non-tolerable even though it is also not an
+# authentication failure specifically. Only this exact vocabulary is ever
+# attached to a report; a raw provider/test error string never reaches it
+# (see ``_classify_discovery_error_code``).
+AUTHENTICATION_FAILURE_CLASSIFICATION = "authentication_failure"
+TRANSIENT_FAILURE_CLASSIFICATION = "transient_failure"
+UNKNOWN_FAILURE_CLASSIFICATION = "unknown_failure"
+_AUTHENTICATION_FAILURE_ERROR_CODES = frozenset({"http_status_401", "http_status_403"})
+_TRANSIENT_NON_HTTP_ERROR_CODES = frozenset({"timeout", "transport_error"})
+_TRANSIENT_HTTP_STATUS_CODES = frozenset(
+    {"http_status_408", "http_status_429"} | {f"http_status_{code}" for code in range(500, 600)}
+)
+
+
+def _classify_discovery_error_code(error_code: object) -> str:
+    """Bucket one raw discovery error code into the report-safe vocabulary.
+
+    ``_provider_discovery_error_code`` (``model_discovery.py``) only ever
+    produces ``http_status_<code>``, ``timeout``, ``transport_error``, or
+    ``invalid_response`` along the real discovery path. Anything else --
+    including a test double's free-form string -- collapses to
+    ``UNKNOWN_FAILURE_CLASSIFICATION`` (the same non-tolerable default a
+    persistent 4xx or an unparseable response gets), so arbitrary text can
+    never reach a report consumed outside this process and an unrecognized
+    condition is never mistaken for a self-resolving one.
+    """
+    if not isinstance(error_code, str):
+        return UNKNOWN_FAILURE_CLASSIFICATION
+    normalized = error_code.strip().casefold()
+    if normalized in _AUTHENTICATION_FAILURE_ERROR_CODES:
+        return AUTHENTICATION_FAILURE_CLASSIFICATION
+    if normalized in _TRANSIENT_NON_HTTP_ERROR_CODES or normalized in _TRANSIENT_HTTP_STATUS_CODES:
+        return TRANSIENT_FAILURE_CLASSIFICATION
+    return UNKNOWN_FAILURE_CLASSIFICATION
+
 
 @dataclass(frozen=True)
 class ProviderCatalogSnapshot:
@@ -65,6 +117,7 @@ class ProviderCatalogSnapshot:
     last_known_good_model_count: int
     refresh_failure_count: int
     providers_with_errors: tuple[str, ...]
+    provider_error_classifications: tuple[tuple[str, str], ...]
 
 
 @dataclass(frozen=True)
@@ -88,6 +141,7 @@ class ProviderCatalogBootstrapReport:
     catalog_backend: str
     catalog_refresh_failure_count: int
     providers_with_errors: tuple[str, ...]
+    provider_error_classifications: tuple[tuple[str, str], ...]
     priced_model_count: int
     privacy_assessment_count: int
     catalog_refreshes: tuple[CatalogRefreshEvidence, ...]
@@ -107,6 +161,7 @@ def as_dict(self) -> dict[str, object]:
             "catalog_backend": self.catalog_backend,
             "catalog_refresh_failure_count": self.catalog_refresh_failure_count,
             "providers_with_errors": list(self.providers_with_errors),
+            "provider_error_classifications": dict(self.provider_error_classifications),
             "priced_model_count": self.priced_model_count,
             "privacy_assessment_count": self.privacy_assessment_count,
             "catalog_refreshes": [
@@ -124,6 +179,189 @@ def as_dict(self) -> dict[str, object]:
         }
 
 
+@dataclass(frozen=True)
+class ProviderCredentialInventoryVerdict:
+    """Secret-free verdict for one provider-credential-inventory check.
+
+    ``ok`` is False for every case that must still fail the calling workflow
+    (``hard_fail_reason`` explains which); ``ok`` is True either because the
+    inventory is complete (both messages ``None``) or because exactly one
+    provider's isolated, transient discovery failure is tolerated
+    (``warning_message`` explains which, for visibility -- this case must
+    never pass silently).
+    """
+
+    ok: bool
+    hard_fail_reason: str | None
+    warning_message: str | None
+
+
+def evaluate_provider_credential_inventory(
+    report: Mapping[str, object],
+    environ: Mapping[str, str],
+    *,
+    provider_model_sources: Sequence[ProviderModelSource] = PROVIDER_MODEL_SOURCES,
+    expected_credential_names: Sequence[str] = PROVIDER_CREDENTIAL_NAMES,
+    max_tolerated_missing_providers: int = 1,
+) -> ProviderCredentialInventoryVerdict:
+    """Judge a bootstrap report's gap (if any) from ``PROVIDER_CREDENTIAL_NAMES``.
+
+    Mirrors ``bootstrap_provider_catalog_runtime``'s own graceful-degradation
+    design (last-known-good models retained, pool still served) by tolerating
+    -- as a warning, not a failure -- exactly one provider's discovery
+    failure this run when the report's own evidence classifies it
+    ``TRANSIENT_FAILURE_CLASSIFICATION`` (see ``_classify_discovery_error_code``:
+    only a narrow, genuinely retryable set of conditions -- a rate limit, a
+    request timeout, a 5xx, a transport failure -- ever gets that
+    classification). Every other gap still hard-fails, because each is
+    exactly a case the tolerance must not silently swallow:
+
+    - a credential never supplied to the caller at all (a real configuration
+      gap, checked against ``environ`` -- bootstrap transport only, never a
+      runtime secret read);
+    - a rollback with no ``providers_with_errors`` evidence tying it to a
+      discovery failure (could hide a real bug elsewhere);
+    - a rollback whose classification is anything other than transient --
+      an authentication failure (a credential the provider itself rejected),
+      a persistent non-auth 4xx (a wrong endpoint, a malformed request
+      shape), an unparseable response, or an unrecognized code. Defaulting
+      to hard-fail here (rather than allow-listing only authentication
+      failures) matters because a permanently broken integration is just as
+      capable of silently passing forever as an invalid credential is;
+    - more than ``max_tolerated_missing_providers`` provider *families*
+      (``model_discovery._provider_family`` -- the same mapping
+      ``select_provider_diverse_models`` uses; it only collapses
+      ``nvidia_nim``/``nvidia_nim_sub``, one upstream outage domain
+      registered under two KV credential names for load balancing, not two
+      independent providers) affected at once -- a broad outage, not the
+      isolated single-provider blip this tolerance exists for, and reason
+      enough to suspect the catalog itself is running stale.
+
+    The set of credentials actually judged against those checks is the union
+    of two things, not just names absent from ``report["registered_credentials"]``:
+    also every name in ``report["restored_credentials"]``.
+    ``_restore_provider_credentials_atomically`` writes a name there whenever
+    that provider's discovery failed *this run*, regardless of what the
+    rollback happened to restore. On a KV that has never held that name
+    before (a fresh registration, or the run-scoped ephemeral store this
+    package's own tests use), rollback restores ``None`` and the name also
+    drops out of ``registered_credentials`` -- the "missing" case. But on a
+    KV that already held a still-valid value for that name from an earlier
+    successful run, rollback restores *that* value instead: the credential
+    stays present in ``registered_credentials`` even though this run's own
+    discovery for it failed. Judging only the "missing" set would return
+    healthy at the very first check for that case without ever looking at
+    ``provider_error_classifications`` -- silently reopening every hard-fail
+    case above (an auth failure, several simultaneous failures) the moment a
+    provider has ever registered successfully before, which is exactly the
+    class of regression this function exists to prevent. A name reaching
+    ``restored_credentials`` with no corresponding ``providers_with_errors``
+    entry still hard-fails as an unexplained rollback below, same as it
+    would for a fully-missing name -- being in ``restored_credentials`` is
+    not itself treated as proof of a legitimate, classifiable failure.
+    """
+    expected = set(expected_credential_names)
+    registered = {
+        name for name in report.get("registered_credentials", ()) if isinstance(name, str)
+    }
+    missing = expected - registered
+    restored = {
+        name
+        for name in report.get("restored_credentials", ())
+        if isinstance(name, str) and name in expected
+    }
+    # A credential can fail this run's discovery yet still land back in
+    # ``registered`` (a durable-KV rollback restoring an old-but-valid prior
+    # value) -- see the docstring. ``missing`` alone is therefore not the
+    # complete set of credentials this run needs to justify; union in every
+    # name rollback actually touched this run.
+    to_evaluate = sorted(missing | restored)
+    if not to_evaluate:
+        return ProviderCredentialInventoryVerdict(True, None, None)
+
+    provider_by_credential = {
+        source.credential_name: source.provider_name for source in provider_model_sources
+    }
+    providers_with_errors = {
+        name for name in report.get("providers_with_errors", ()) if isinstance(name, str)
+    }
+    error_classifications = dict(report.get("provider_error_classifications", {}) or {})
+
+    unconfigured = sorted(
+        name for name in to_evaluate if not (environ.get(name) or "").strip()
+    )
+    if unconfigured:
+        return ProviderCredentialInventoryVerdict(
+            False,
+            f"credential inventory mismatch: not configured in secrets: {unconfigured}",
+            None,
+        )
+
+    unexplained = sorted(
+        name
+        for name in to_evaluate
+        if provider_by_credential.get(name) not in providers_with_errors
+    )
+    if unexplained:
+        return ProviderCredentialInventoryVerdict(
+            False,
+            f"credential inventory mismatch: unexplained rollback for: {unexplained}",
+            None,
+        )
+
+    non_transient = sorted(
+        name
+        for name in to_evaluate
+        if error_classifications.get(provider_by_credential.get(name, ""))
+        != TRANSIENT_FAILURE_CLASSIFICATION
+    )
+    if non_transient:
+        observed = {
+            name: error_classifications.get(
+                provider_by_credential.get(name, ""), UNKNOWN_FAILURE_CLASSIFICATION
+            )
+            for name in non_transient
+        }
+        return ProviderCredentialInventoryVerdict(
+            False,
+            "credential inventory mismatch: not a tolerated transient outage "
+            f"for: {observed}",
+            None,
+        )
+
+    # Collapse through the same provider-family mapping
+    # ``select_provider_diverse_models`` already uses for diversity selection
+    # (``model_discovery._provider_family``): nvidia_nim/nvidia_nim_sub are
+    # two KV credential names for one upstream outage domain (a load-
+    # balancing pair, not two independent providers -- see
+    # PROVIDER_MODEL_SOURCES's own comment). Counting them separately would
+    # hard-fail a single NVIDIA-side outage that happens to affect both keys
+    # at once, which is exactly the isolated-outage case this tolerance
+    # exists for, not the broad-outage case it's meant to catch.
+    affected_providers = sorted(
+        {_provider_family(provider_by_credential.get(name, name)) for name in to_evaluate}
+    )
+    if len(affected_providers) > max_tolerated_missing_providers:
+        return ProviderCredentialInventoryVerdict(
+            False,
+            "credential inventory mismatch: too many providers degraded at once "
+            f"({len(affected_providers)} > {max_tolerated_missing_providers}): "
+            f"{affected_providers}",
+            None,
+        )
+
+    return ProviderCredentialInventoryVerdict(
+        True,
+        None,
+        f"provider catalog degraded: {to_evaluate} rolled back after an isolated, "
+        "transient discovery failure "
+        f"(providers_with_errors={sorted(providers_with_errors)}, "
+        f"catalog_refresh_failure_count={report.get('catalog_refresh_failure_count')}, "
+        f"restored_credentials={report.get('restored_credentials')}); catalog still "
+        "serves from last-known-good/other-provider models.",
+    )
+
+
 def build_provider_catalog_store() -> ProviderCatalogStore:
     """Build a catalog store colocated with the active credential backend."""
     backend = get_backend()
@@ -202,11 +440,20 @@ def refresh_persisted_provider_catalog(
     for model in discovered:
         live_by_account.setdefault(_model_key(model), []).append(model)
 
-    failed_names = {error.provider_name for error in errors}
+    # Last write wins for a provider with more than one error this refresh;
+    # every real caller (discover_all_models) raises at most one
+    # ProviderDiscoveryError per source, so this only matters for adversarial
+    # test doubles.
+    raw_error_code_by_provider = {error.provider_name: error.error_code for error in errors}
+    failed_names = set(raw_error_code_by_provider)
     effective: list[DiscoveredModel] = []
     last_known_good_count = 0
     refresh_failures = 0
     providers_with_errors: set[str] = set(failed_names)
+    error_classifications: dict[str, str] = {
+        provider_name: _classify_discovery_error_code(raw_code)
+        for provider_name, raw_code in raw_error_code_by_provider.items()
+    }
 
     for source in sources:
         if source.credential_name not in registered:
@@ -220,6 +467,16 @@ def refresh_persisted_provider_catalog(
             store.record_failure(source, error_code="empty_provider_catalog")
             refresh_failures += 1
             providers_with_errors.add(source.provider_name)
+            # A successful-but-empty listing carries no HTTP status of its
+            # own to classify, and -- same reasoning as a persistent 4xx --
+            # is at least as likely to be a genuinely broken integration (a
+            # wrong task/query filter on our side, or a provider account
+            # with zero eligible models) as a self-resolving blip. Default
+            # it to the same non-tolerable bucket rather than assuming
+            # transient.
+            error_classifications.setdefault(
+                source.provider_name, UNKNOWN_FAILURE_CLASSIFICATION
+            )
         else:
             eligible_ids = {
                 model.model_id
@@ -253,6 +510,7 @@ def refresh_persisted_provider_catalog(
         last_known_good_model_count=last_known_good_count,
         refresh_failure_count=refresh_failures,
         providers_with_errors=tuple(sorted(providers_with_errors)),
+        provider_error_classifications=tuple(sorted(error_classifications.items())),
     )
 
 
@@ -391,6 +649,7 @@ def bootstrap_provider_catalog_runtime(
             catalog_backend=store.backend_name,
             catalog_refresh_failure_count=snapshot.refresh_failure_count,
             providers_with_errors=snapshot.providers_with_errors,
+            provider_error_classifications=snapshot.provider_error_classifications,
             priced_model_count=priced_count,
             privacy_assessment_count=privacy_assessment_count,
             catalog_refreshes=catalog_refreshes,