fix(ci): surface silently-dropped provider discovery errors in the review sidecar - #1426
Merged
Merged
Conversation
…view sidecar Prompted by a direct question about why a reproduction of the orchestrator/free pool-exhaustion incident only ever showed 3 of 5 configured providers (openrouter, nvidia_nim, nvidia_nim_sub) and never bytez/openai. Root cause: contextual_orchestrator_review_launcher.py's main() called `discovered, _ = discover_all_models()`, discarding the second tuple element. discover_all_models() already isolates and returns each provider's failure as a bounded, secret-free ProviderDiscoveryError (provider_name + a stable error_code like http_status_401/timeout/transport_error/invalid_response) -- the launcher just never looked at it, so an operator could not tell "this provider has zero free models" from "this provider's discovery silently failed", which is exactly what made the earlier reproduction inconclusive. Fix: add _log_discovery_errors(), called right after discover_all_models(), printing one `provider_discovery_failed provider=<name> code=<code>` line per error to stderr (non-fatal). Extend the sidecar's stream sanitizer with a matching bounded allowlist regex so this reaches CI evidence instead of falling into omitted_unstructured_lines=N -- the same class of gap #1425 closed for the fail-closed exit message. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough런처가 Changes프로바이더 검색 오류 가시성
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change makes provider-discovery failures visible in CI without changing provider-failure handling or introducing new production interfaces, dependencies, or deployment behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
Contributor
Author
|
Generated by Claude Code |
![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.
Devin's review on this PR caught a real gap in the fix itself: the new per-provider discovery-error diagnostic only ever reaches the failure-path ::error:: message. When another provider keeps startup successful (which is the common case once the free-pool exhaustion is fixed), $sidecar_stderr is never read again -- publish_sidecar_evidence only copies discovery/catalog/ policy JSON, never the raw stdout/stderr logs, and only strix.yml uploads an artifact at all (opencode-review.yml/noema-review.yml upload nothing). A partial provider failure would stay completely invisible. Fix: after healthz confirms, print a bounded (20-line) snapshot of $sidecar_stderr into the always-visible job log if it has any content. Cannot `wait_for_sidecar_sanitizers` here -- the sidecar keeps serving after a successful healthz, so its sanitizer never sees EOF and waiting would hang the workflow forever; this is a best-effort snapshot, not a guaranteed- complete read, which is the best available without adding IPC between the discovery phase and the main script. Co-Authored-By: Claude <noreply@anthropic.com>
Devin's follow-up review caught a real race in the previous commit's fix: `[ -s "$sidecar_stderr" ]` checked the file size immediately after healthz confirmed, with no guarantee the async sanitizer subprocess had actually flushed the discovery-error lines through yet. Under a slow sanitizer this would silently show nothing even when real warnings existed. Fix: the launcher's _log_discovery_errors() now always emits a trailing "discovery_diagnostics_complete" sentinel as its last stderr line (even with zero errors). Since the sanitizer processes lines strictly in order, once that sentinel has passed through, every earlier discovery-error line is guaranteed to have reached $sidecar_stderr too. The sidecar script polls for the sentinel (bounded, ~5s) instead of guessing from file size or a fixed sleep -- deterministic rather than probabilistic. Reproduced both the race and the fix directly: a minimal standalone harness with a deliberately slow (150ms/line) sanitizer lost data under the old size-check pattern; the sentinel-poll pattern captured it completely, 3/3 runs. Also caught and fixed a real bug the same reproduction surfaced: `grep -v` exits 1 when everything is filtered out (the common, healthy zero-warnings case), which under `set -o pipefail` would have aborted the whole script on every clean startup -- added the missing `|| true`. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Aug 30, 2026
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
Resolves the split-timeout/batched-preflight design (this branch) against main's independently-landed ADR-0005 diagnostic, bounded-retry preflight (#1449/#1452 and predecessors #1436/#1440/#1434/#1425/#1426/#1422/#1413/ #1401/#1442), a genuine semantic merge, not a mechanical pick: - Keeps this branch's core contribution: launcher.py splits startup-route admission (10s) from real serving (120s, via _build_model_client), and probes up to REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES (24) candidates in concurrent batches of REVIEW_PREFLIGHT_BATCH_SIZE (4). - Folds in main's ADR-0005 escalation logic (16-token base probe, escalate to the real serving budget on a "budget too small" signature, shared REVIEW_PREFLIGHT_MAX_ESCALATIONS=4 cap) into the per-candidate probe used by that batching, via a new _EscalationBudget class so the shared budget stays a hard, lock-enforced invariant across concurrently-probed candidates within one batch -- a plain int (correct for main's original sequential loop) cannot coordinate that safely under concurrency. - Folds in main's ADR-0005 bounded retry for the sidecar's own separate gateway smoke request (up to REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS=3 attempts, 120s each, retried only on no-response/transport failure), replacing this branch's single-attempt transport_timeout/transport_error classification, while keeping this branch's "orchestration":"route" field and the 413-body-limit stderr-message-capture test enhancement. - Keeps this branch's family_cap==total-route-budget default (24) over main's more conservative raise to 8, since batching's concurrency keeps worst-case wall time bounded even at the higher route/family count (recomputed and re-tested below). - Two designs were deliberately NOT combined, resolved in main's favor after tracing each side's intent (not guessed): Strix's orchestrator/auto routing, which this branch's history re-added but main's most recent, explicit owner decision (#1434) reverted to orchestrator/free-only with the accepted-risk rationale recorded in ADR-0003; and a "fail closed on any partial provider discovery error" gate this branch added ( _require_complete_discovery), which main never adopted and whose philosophy directly conflicts with main's demonstrated-in-production "log the failure, continue with whatever succeeded" handling -- live BandScope evidence in this PR's own comments shows single-provider hiccups (Bytez 5xx) are common and should not be fatal to the whole pool. - Updates the two main-side tests whose literal worst-case-time assertions were computed for the old sequential (non-batched) design (test_preflight_stage_limits_share_one_startup_budget, test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case) to the batching-aware arithmetic, and drops this branch's now-superseded tests (orchestrator/auto Strix routing, fail-closed-on-partial-discovery, single-attempt gateway transport classification). Verification: coverage run -m pytest tests (2096 passed, 1 pre-existing skip, 21 subtests, 100% coverage on scripts/ci); interrogate (100% docstrings); git diff --check clean; bash -n on both changed shell scripts; ruff check clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prompted by a direct question about the
orchestrator/freepool-exhaustion incident's reproduction: why did it only ever show 3 of 5 configured providers (openrouter,nvidia_nim,nvidia_nim_sub) and neverbytez/openai, despite all 5 credentials being registered?Root cause
contextual_orchestrator_review_launcher.py'smain()calleddiscovered, _ = discover_all_models(), discarding the second tuple element.discover_all_models()already isolates and returns each provider's failure as a bounded, secret-freeProviderDiscoveryError(provider_name+ a stableerror_codesuch ashttp_status_401/timeout/transport_error/invalid_response— verified by reading_provider_discovery_error_codeandProviderDiscoveryError.__init__directly incontextual-orchestrator) — the launcher simply never looked at it.This meant an operator reading CI logs could not tell "this provider has zero free models" from "this provider's discovery silently failed" — exactly the ambiguity that made an earlier ad hoc reproduction of the free-pool-exhaustion incident inconclusive about
bytez/openai.Fix
_log_discovery_errors()to the launcher, called immediately afterdiscover_all_models(), printing oneprovider_discovery_failed provider=<name> code=<code>line per error to stderr. Non-fatal, matchingdiscover_all_models()'s own "one provider's failure never blocks the others" contract.scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.pywith a matching bounded allowlist regex (mirroring the existingrequest_failedpattern) so this new diagnostic reaches CI evidence instead of falling intoomitted_unstructured_lines=N— the same class of gap fix(noema): surface real sidecar fail-closed diagnostics past redaction #1425 closed for the fail-closed exit message.docs/product-technical-gap-baseline.mdentry (append-only, no existing entry edited).This does not by itself restore
orchestrator/free— it only makes any future bytez/openai discovery failure visible instead of silently indistinguishable from "no free models today". The free-pool-exhaustion root cause and fix remain tracked separately.Validation
PYTHONPATH=. python3 -m coverage run -m pytest tests -q— 1878 passed, 1 skipped, 25 subtestsinterrogate— 100.0%git diff --check— cleancontextual_orchestrator_review_launcher.pyremains outside the coverage gate per this repo's pre-existing, documentedpyproject.tomlomission (imports the vendored orchestrator library, installed only inside the sidecar runtime); the new_log_discovery_errorshelper is covered by two new regression tests exercising it directly viarunpy.run_path, matching this file's existing test pattern for the module's other runtime-only helpers.Generated by Claude Code
Summary by CodeRabbit
버그 수정
문서화
테스트