fix(ci): tolerate an isolated provider outage in provider-catalog-sync - #928
Conversation
provider-catalog-sync's embedded verification script hard-failed whenever any single provider's credential was rolled back after a discovery failure, even though bootstrap_provider_catalog_runtime already retains last-known-good models and keeps serving from the other providers by design. Every scheduled run has failed on BYTEZ_API_KEY since the schedule went live 5 days ago (44 of 46 runs) with the pool otherwise healthy. Distinguish a real configuration gap (secret never supplied) or an unexplained rollback (no providers_with_errors evidence -- could hide a real bug) from a supplied credential whose provider is named in providers_with_errors: only the former two still hard-fail; the isolated- outage case now emits a ::warning:: with the report's own evidence (providers_with_errors, catalog_refresh_failure_count, restored_credentials) and lets the job succeed. No production code changed. Documented the investigation, the live Bytez probe evidence, and the corroborating ContextualWisdomLab/.github http_status_500 signature in docs/product-technical-gap-baseline.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
📝 WalkthroughWalkthrough제공자 카탈로그 동기화 검증이 전체 자격 증명 일치 검사에서 원인별 검사로 변경되었습니다. 설정 누락과 설명되지 않은 롤백은 실패 처리합니다. 제공자 격리 검색 실패로 설명되는 누락은 경고로 처리합니다. 관련 장애 기록을 문서에 추가했습니다. Changes제공자 카탈로그 검증
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The workflow now tolerates an isolated provider outage, but its current check may also report success when multiple providers are unavailable, leaving scheduled synchronization on stale catalog data. The tolerated case should be restricted to exactly one mapped provider before merge. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ProviderCatalogSync
participant BootstrapReportValidation
participant ProviderModelSources
ProviderCatalogSync->>BootstrapReportValidation: 부트스트랩 보고서 검증
BootstrapReportValidation->>ProviderModelSources: 제공자별 오류 확인
ProviderModelSources-->>BootstrapReportValidation: 모델 소스 목록 반환
BootstrapReportValidation-->>ProviderCatalogSync: 실패 또는 경고 후 동기화 계속
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Split the 44/46-failure count into its schedule vs workflow_dispatch components (43 schedule: 42 failure + 1 cancelled; 3 workflow_dispatch: 2 failure + 1 skipped) instead of the looser "44 of 46 scheduled runs" phrasing, and independently confirmed the corroborating http_status_500 against the primary job log (contextual-orchestrator PR #921, job 99243631744, 2026-08-30T10:26:47Z) rather than only the paraphrase in the existing gap-baseline entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ce to one provider Address review findings on PR #928: - Devin: the soft-warning path treated any providers_with_errors entry as an isolated outage, including an authentication failure (an invalid, expired, or revoked credential) -- which would then stay silently disabled forever with no alert, every run. ProviderDiscoveryError.error_code already distinguishes these (http_status_401/403 vs timeout/transport_error/ other http_status_*) via model_discovery._provider_discovery_error_code; that information was computed but discarded before reaching the report. - CodeRabbit: the check could report success while multiple providers were degraded simultaneously (a broad outage, not an isolated blip), leaving scheduled sync silently serving a stale catalog. Fix: bucket each discovery failure into a small, report-safe classification (provider_error_classifications: authentication_failure | transient_failure) via a new _classify_discovery_error_code, threaded through ProviderCatalogSnapshot/ProviderCatalogBootstrapReport. Move the whole hard-fail/warn/ok decision out of inline YAML branching into a new, unit- tested evaluate_provider_credential_inventory(): still hard-fails an unconfigured secret, an unexplained rollback, an authentication failure, or more than one provider missing at once; only a single provider's transient-classified rollback, with its secret present, is tolerated as a warning. The workflow now just calls this function. New regression coverage in tests/test_provider_catalog_bootstrap.py (transient HTTP 500 tolerated; HTTP 401/403 still hard-fails; two simultaneous provider failures still hard-fail) and tests/test_provider_catalog_bootstrap_boundaries.py (the verdict function's own edge cases: fully healthy, unconfigured secret, unexplained rollback, non-string error code). 100% statement and docstring coverage on provider_catalog_bootstrap.py. Also tightened the gap-baseline doc's run-history wording (CodeRabbit) to avoid calling the 2 cancelled/skipped runs "failures". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
| @@ -1,5 +1,92 @@ | |||
| # Contextual Orchestrator: Product & Technical Gap Baseline | |||
|
|
|||
| ## 2026-08-30 provider-catalog-sync: no scheduled run has succeeded in 5 days over one provider; workflow check was too strict | |||
There was a problem hiding this comment.
🔍 Required research grounding is absent
This substantive CI process change adds no paper, citation, or applicability note. Repository governance requires research grounding for substantive feature or process PRs.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Declined, with reasoning noted in the doc entry. This PR is a CI reliability bugfix (isolating transient vs. permanent provider-discovery failures in a scheduled sync workflow's credential-inventory check) — an operational engineering pattern, not a novel algorithm or research claim in the sense docs/papers/ and the Fugu/TRINITY/Conductor-derived TDD convention target. No prior CI-only fix in this repo's history (abb9aaa6, b3278df7, c328c1e8, 1bbda718, 8abc4b45 — all .github/workflows/provider-catalog-sync.yml changes) attaches a paper either, so this follows established precedent. Happy to add one if a maintainer sees a specific applicable citation I'm missing.
Generated by Claude Code
Address Devin's second-pass finding on PR #928: the transient bucket was any http_status_* other than 401/403, so a persistent non-auth 4xx (400, 404, ...) -- almost always a genuinely broken integration (wrong endpoint, malformed request shape, a moved/retired API) rather than a self-resolving blip -- was tolerated forever, same underlying problem as the authentication-failure gap already fixed, just for a different code range. Narrow transient_failure to only standard-retry-semantics-retryable conditions: 408, 429, 5xx, timeout, transport_error. Everything else (a persistent non-auth 4xx, invalid_response, a successful-but-empty listing, or anything unrecognized) now collapses to unknown_failure and hard-fails. evaluate_provider_credential_inventory is reframed as default-deny: it now requires the classification to be exactly transient_failure to tolerate, rather than only excluding authentication_failure -- so a future new classification value is hard-fail by default, not silently allowed. New tests: a persistent 400/404/invalid_response still hard-fails (test_persistent_client_error_is_not_excused_as_a_transient_outage); 408/429/5xx are confirmed still tolerated (test_genuinely_retryable_http_statuses_are_transient). 100% statement and docstring coverage maintained. Declined Devin's research-grounding finding on the gap-baseline doc entry: this is a CI reliability bugfix (isolating transient vs. permanent provider failures), not a novel algorithm or research claim, and no prior CI-only fix in this repo's history attaches a paper either -- noted in the doc entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Address Devin's third-pass finding on PR #928 ("Durable rollback bypasses failure verdict"): evaluate_provider_credential_inventory's `if not missing: return ok` early return only looked at registered_credentials. On the run-scoped/ephemeral KV every test so far exercised, a failed provider's rollback restores None, so the credential leaves registered_credentials AND enters restored_credentials together -- the two were accidentally redundant. On a durable KV that already held a still-valid prior value for that credential, rollback restores that value instead of None: the credential never leaves registered_credentials, missing comes back empty, and the function returned healthy at the very first line -- without ever inspecting provider_error_classifications. After a scheduled sync's first successful run against the durable PostgreSQL KV, this made the entire auth-failure/persistent-4xx/multi-provider hard-fail logic silently unreachable: a revoked or rotated credential, or several providers failing at once, would both report ok=True forever. Fix: evaluate the union of `missing` and `restored_credentials` (filtered to expected_credential_names, mapped through the same provider_by_credential map) through the identical unconfigured/ unexplained/classification/bound checks, rather than `missing` alone. A name in restored_credentials with no corresponding providers_with_errors entry still hard-fails as an unexplained rollback, same as a fully-missing name would. New tests reproduce the durable-KV path end to end by pre-registering a credential before bootstrap so rollback restores a non-None prior value: test_durable_rollback_with_auth_failure_still_hard_fails, test_durable_rollback_with_two_simultaneous_failures_still_hard_fails, test_durable_rollback_with_single_transient_failure_is_still_tolerated (tests/test_provider_catalog_bootstrap.py), plus three unit-level cases directly against a report where registered_credentials is already complete (tests/test_provider_catalog_bootstrap_boundaries.py). Also fixed two stale "non-authentication discovery failure" mentions left over from the round-2 narrowing to say "transient" instead. 100% statement and docstring coverage maintained. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…for the bound Address Devin's fourth-pass finding on PR #928 ("One NVIDIA outage fails sync"): the multi-provider tolerance bound counted raw provider_name values, but nvidia_nim/nvidia_nim_sub are two KV credential names for one upstream outage domain (a load-balancing pair -- see PROVIDER_MODEL_SOURCES's own comment and model_discovery._provider_family, already used by select_provider_diverse_models for exactly this collapsing). A single NVIDIA-side blip failing both keys at once counted as two providers degraded and hard-failed -- exactly the isolated-outage case the tolerance exists for, misread as a broad one. Fix: route affected_providers through _provider_family before comparing against max_tolerated_missing_providers. The per-credential unconfigured/unexplained/classification checks are untouched -- they still key off the real provider_name, since providers_with_errors/ provider_error_classifications are recorded per source, not per family. New tests: both NVIDIA keys failing together with transient errors are still tolerated as one family (test_nvidia_primary_and_sub_outage_together_is_one_provider_family), contrasted with that same NVIDIA-family outage plus a genuinely distinct provider still hard-failing (test_nvidia_family_outage_plus_a_distinct_provider_still_hard_fails). 100% statement and docstring coverage maintained. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…suite run The targeted-suite count (79, not 82) and the full python -m pytest tests -q run were stale from an earlier in-flight snapshot. Record the actual completed result: 2781 passed, 1 skipped in 720.75s, with the pre-existing numpy-import collection gap in test_psychometric_routing.py noted as unrelated to this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
| docstring coverage on `provider_catalog_bootstrap.py`; targeted suite green (79 tests across | ||
| `tests/test_provider_bootstrap*.py`/`tests/test_provider_catalog_bootstrap*.py`); full |
There was a problem hiding this comment.
📝 Info: Targeted count includes parametrization
The 79 tests comprise 74 test functions and five extra cases generated by the six-case parametrized policy test.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ate readiness (#1449) Bypass-merged under standing organizational authorization: this docs-only ADR (no executable code) is structurally blocked from passing its own required noema-review check, since that check always executes main's current (pre-fix) contextual_orchestrator_review_sidecar.sh via the pull_request_target trust boundary — confirmed identically reproducing on ContextualWisdomLab/contextual-orchestrator#928, an unrelated PR, ruling out anything specific to this diff. All 30 Devin Review threads across 9 rounds are resolved; the final pass found 0 new issues. Real quality/security gates (Trivy, CodeQL, Semgrep, gitleaks, osv-scan, Scorecard) are clean. Full test suite re-verified at 1897 passing. Implementation is tracked separately in #1452.
#1452) Bypass-merged under standing organizational authorization: this PR's own noema-review and Strix required checks are structurally blocked by the exact bug this PR fixes, since pull_request_target checks always execute main's current (pre-fix) contextual_orchestrator_review_sidecar.sh, never this PR's own diff — confirmed identically reproducing on three independent instances (this PR itself, ContextualWisdomLab/contextual-orchestrator#928's noema-review, and that same PR's Strix scan), ruling out anything specific to this diff. All 26 Devin Review threads across 9+ combined rounds (spanning #1449 and this PR) are resolved or non-actionable info; the final pass found 0 new issues. Full test suite: 1930 passed, 1 skipped, 100% coverage and 100% docstrings on scripts/ci/. Real quality/security gates (Trivy, CodeQL, Semgrep, gitleaks, osv-scan, Scorecard) are clean. Implements the design merged in #1449.
|
Strix check status: failing twice in a row now with the same signature — This is not a bug in this PR's diff — it's NVIDIA NIM rate limiting on Strix's own LLM backend, the exact class of failure Confirmed separately: this same head SHA's Per this org's CI-handling convention, I've already used the one re-run to confirm this isn't a one-off flake — it reproduced identically. I'm not re-running a third time or attempting a code change, since there's nothing in this PR's diff to fix; this is upstream provider capacity. Keeping this PR watched — Strix should clear on its own once concurrent scan load across the org drops, or I'll re-run again once conditions look different rather than repeatedly retrying into the same limit. Generated by Claude Code |
2 participants
Summary
provider-catalog-sync(scheduled,.github/workflows/provider-catalog-sync.yml) has failed on every single scheduled run since it went live 5 days ago — 42failure+ 1cancelledof 43 scheduled runs,2026-08-25T09:01:27Zthrough today, zero scheduled successes ever — always withcredential inventory mismatch: ['BYTEZ_API_KEY'].bootstrap_provider_catalog_runtime: an isolated per-provider discovery failure rolls that provider's credential back and excludes it fromregistered_credentials— exactly the graceful degradation the function's own docstring promises. The workflow's verification script assertedregistered_credentials == all providersunconditionally, with no tolerance for this already-handled case.401(not a 500), and an independent corroborating log fromContextualWisdomLab/.githubthe same day showed a genuineprovider_discovery_failed provider=bytez code=http_status_500. Full writeup indocs/product-technical-gap-baseline.md.Design (revised across four review rounds — see thread replies)
The first cut only checked whether a missing credential's provider appeared anywhere in
providers_with_errors. Four rounds of review from Devin and CodeRabbit found real gaps, all now closed:None, soregistered_credentialslooks complete and the verdict function returnedok=Truebefore ever inspecting classification — in production, after the first successful durable-KV run, an invalid/rotated credential or several simultaneous failures would both pass silently forever.nvidia_nim/nvidia_nim_subare one outage domain, not two (Devin, round 4): they're two KV credential names for one upstream provider (load balancing), permodel_discovery._provider_family; the multi-provider bound counted them separately, so one NVIDIA-side blip hitting both keys would incorrectly hard-fail as "two providers degraded."Current design, all in
contextual_orchestrator/provider_catalog_bootstrap.py:ProviderDiscoveryError.error_codeis bucketed by_classify_discovery_error_code(), default-deny: onlyhttp_status_408/429/5xx+timeout/transport_errorgettransient_failure; everything else isunknown_failure.evaluate_provider_credential_inventory()evaluates the union ofmissingandrestored_credentials(so a durable rollback can't bypass classification), hard-fails an unconfigured secret, an unexplained rollback, a non-transient classification, or more than one provider family (via_provider_family) affected at once. Only a single family'stransient_failure-classified rollback is tolerated as a::warning::.Declined one finding: a research-grounding citation (this is a CI reliability bugfix, not a novel algorithm/research claim; no prior CI-only fix in this repo attaches a paper either).
Test plan
python -m pytest tests/test_provider_catalog_bootstrap.py tests/test_provider_catalog_bootstrap_boundaries.py tests/test_provider_bootstrap.py tests/test_provider_bootstrap_boundaries.py tests/test_provider_bootstrap_secret_normalization.py -q— 79 passed, covering every round's regression: transient tolerated / auth+persistent-4xx hard-fail / two distinct providers hard-fail / three durable-KV end-to-end scenarios / NVIDIA dual-key family collapsing vs. a genuinely distinct third providercoverage runonprovider_catalog_bootstrap.py— 100% statement coverage;interrogate— 100% docstring coverageyaml.safe_load+compile()on the workflow's embedded heredocs — parse/compile cleanlypython -m pytest tests -q(excludingtests/test_psychometric_routing.py, which fails to collect in this sandbox for lack ofnumpy— an unrelated pre-existing environment gap) — 2781 passed, 1 skipped in 720.75s, zero failures🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw