fix: fail over current-main passthrough providers - #763
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches📝 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 |
|
@opencode-agent Review exact current head |
|
@opencode-agent Review exact head |
|
@opencode-agent Review exact current head SHA f06ba91. Use only same-head required Checks and changed-file evidence; publish a fresh formal verdict. Approve only when all required evidence and protected review conditions are satisfied. |
|
@opencode-agent review the exact current head |
|
@opencode-agent fix this exact-head defect on the existing branch before reviewing: the new failover class is exported only from |
Pull request was converted to draft
…ct-rebased' into HEAD # Conflicts: # contextual_orchestrator/__init__.py
…bedding-models' into HEAD # Conflicts: # README.md # contextual_orchestrator/orchestrator.py # tests/test_local_gateway.py
|
Current HEAD Exact-head verification:
Please review this exact HEAD with @opencode-agent. No self-approval or bypass is used. |
![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.
|
Exact current HEAD was revalidated in its existing stack: focused capability-isolation, one-shot passthrough, failover, local-gateway, and embedding honesty suite passed 133 tests; Ruff, compileall, and diff checks passed. The branch remains based on the current reasoning-contract parent; no force push or self-approval was used. @opencode-agent review this exact SHA only and publish a formal verdict. |
|
Exact current HEAD |
|
Exact-head revalidation for PR #763 Verified
Please independently review exactly this HEAD, including one-attempt-per-candidate behavior, explicit-model stickiness, cause-chain classification, local Responses translation/model switching, and preservation of embedding auto-selection. Do not merge until the current stack and all protected gates are terminal and approved. |
|
Remote concurrent work advanced the branch to |
|
Exact current HEAD |
Exact-head validation — PR #763
@opencode-agent please review only exact current HEAD |
…effort-contract-rebased' into repair/pr763-latest-parent
5b43ebf
into
fix/auto-reasoning-effort-contract-rebased
|
Exact-head reconciliation completed on the existing branch.
Hosted required Checks and an independent formal approval are still authoritative. No merge or approval claim is made while they are pending. |
|
Correction to the preceding evidence comment: the exact base HEAD is |
| with _local_provider_slot(agent, self.local_concurrency, self.timeout): | ||
| return self._send_raw_with_retry( | ||
| agent, | ||
| normalized_endpoint, | ||
| payload, | ||
| destination, | ||
| allow_transient_retries=False, | ||
| ) |
There was a problem hiding this comment.
🔍 One-shot passthrough drops local chat max_tokens default
proxy_send defaults max_tokens for local chat/completions passthrough (orchestrator.py, locked by test_local_chat_passthrough_applies_bounded_controls_for_final_synthesis). proxy_send_once omits that branch and sends local chat/completions through the generic path at orchestrator.py:1567-1574 with no default. Adaptive passthrough failover always calls proxy_send_once (passthrough_failover.py), so a local gateway on the virtual-model path loses the bounded control and the provider default can truncate output. Only the local responses branch was carried over.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for candidate_agent in candidates: | ||
| upstream = dict(payload) | ||
| upstream["model"] = candidate_agent.model | ||
| try: | ||
| result = _proxy_send_once(self.client, candidate_agent, endpoint, upstream) | ||
| except Exception as exc: | ||
| if not adaptive_request or not _is_adaptive_failover_error(exc): | ||
| raise | ||
| last_error = exc | ||
| self._record_failure(candidate_agent.id) | ||
| with self._circuit_lock: | ||
| state = self._circuit.setdefault( | ||
| candidate_agent.id, | ||
| {"failures": 0.0, "opened_at": 0.0}, | ||
| ) | ||
| state["failures"] = max( | ||
| state["failures"], | ||
| float(self.circuit_failure_threshold), | ||
| ) | ||
| state["opened_at"] = time.monotonic() | ||
| continue | ||
| self._record_success(candidate_agent.id) | ||
| return result |
There was a problem hiding this comment.
🔍 Failover attributes fallback tokens to the primary agent
When adaptive passthrough succeeds on a fallback candidate (passthrough_failover.py), only the raw dict is returned. proxy_completion records the step with agent_id = the primary agent and usage from the fallback response (orchestrator.py), so the trace and spend ledger attribute the served model's tokens to a model that did not serve. The seam cannot report which agent actually served.
Was this helpful? React with 👍 or 👎 to provide feedback.
| destination = self._validate_provider(agent) # pragma: no cover | ||
| if normalized_endpoint == "responses" and _is_local_provider_url(agent.base_url): | ||
| chat_payload = _responses_to_chat_payload(payload) | ||
| chat_payload.setdefault("max_tokens", self.max_output_tokens) |
There was a problem hiding this comment.
📝 Info: One-shot local responses ignores request-scoped max_output_tokens
proxy_send reads self._request_setting("max_output_tokens", ...) for local responses translation (orchestrator.py), but proxy_send_once uses self.max_output_tokens directly (orchestrator.py:1557), ignoring a thread-local override from request_settings(). Minor divergence on the failover path.
Was this helpful? React with 👍 or 👎 to provide feedback.
| with self._circuit_lock: | ||
| state = self._circuit.setdefault( | ||
| candidate_agent.id, | ||
| {"failures": 0.0, "opened_at": 0.0}, | ||
| ) | ||
| state["failures"] = max( | ||
| state["failures"], | ||
| float(self.circuit_failure_threshold), | ||
| ) | ||
| state["opened_at"] = time.monotonic() |
There was a problem hiding this comment.
🔍 Adaptive failover forces the shared circuit breaker open on a single failure
On each adaptive passthrough failure, the code both calls self._record_failure(agent.id) and then forces state["failures"] = max(failures, threshold) and state["opened_at"] = now, opening the circuit for that agent after a single 429/5xx. Because the circuit state (self._circuit) is shared with the ordinary chat/_invoke failover path, one transient passthrough failure will mark that agent circuit-open for circuit_reset_seconds across ALL paths, potentially deprioritizing an otherwise-healthy agent for normal chat routing. The description says adaptive failures "open the existing circuit breaker immediately," so this appears intentional, but the cross-path impact of a single transient blip is aggressive and worth confirming against expected routing behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| def _is_adaptive_failover_error(error: BaseException) -> bool: | ||
| """Classify transient or stale-candidate failures through provider wrappers.""" | ||
| for candidate in _provider_error_chain(error): | ||
| if is_transient_error(candidate): | ||
| return True | ||
| if ( | ||
| isinstance(candidate, urllib.error.HTTPError) | ||
| and candidate.code in _CANDIDATE_UNAVAILABLE_HTTP_STATUS | ||
| ): | ||
| return True |
There was a problem hiding this comment.
📝 Info: Implicit exception context (not suppressed) is treated as a failover signal
_is_adaptive_failover_error traverses __cause__ and, when not suppressed, __context__. This means a non-transient wrapper error whose implicit __context__ incidentally holds a transient 429 (e.g. an unrelated exception was being handled when a terminal error was raised without raise ... from None) would be classified as adaptive and trigger failover. The suppressed-context case (raise ... from None) is correctly excluded and tested, but incidental (non-suppressed) transient context could over-trigger failover. This matches the documented "bounded, cycle-safe cause/context chain" design, but the reliance on implicit context is a subtle heuristic worth keeping in mind if provider SDKs raise terminal errors while handling transient ones.
Was this helpful? React with 👍 or 👎 to provide feedback.
| raise RuntimeError( | ||
| f"all {len(candidates)} candidate agents failed for passthrough endpoint={endpoint}" | ||
| ) from last_error |
There was a problem hiding this comment.
📝 Info: Adaptive single-candidate transient failures now surface as a generic RuntimeError
For an adaptive (virtual/omitted-model) request where _failover_candidates yields a single candidate that fails transiently, the loop in passthrough_failover.py exhausts and raises RuntimeError("all 1 candidate agents failed ...") from last_error, discarding the provider's original error type at the top of the stack (it is preserved only as __cause__). Callers that previously inspected the raw provider HTTPError (e.g. 429 with Retry-After) from the base path will now see a wrapped RuntimeError. This is consistent with the exhaustion test (test_passthrough_provider_failover.py) but differs from the sticky concrete-model path which re-raises the original error.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ranked = [ | ||
| agent | ||
| for agent in self._ranked_agents("", capability) | ||
| for agent in self._ranked_agents("", capability, require_chat_model=False) | ||
| if not agent.disabled | ||
| and capability in agent.tags | ||
| and capability not in agent.provider_exclusions |
There was a problem hiding this comment.
📝 Info: select_capability_agent intentionally bypasses the chat-model gate
select_capability_agent calls _ranked_agents("", capability, require_chat_model=False) (orchestrator.py), so non-chat models (e.g. text-embedding-3-large) can still be selected for capability-tag-driven paths like embeddings. This is correct for the embeddings route (which uses embed_many, not chat), but note that the transport-level chat() guard would fail closed if such an agent were ever routed to chat(). The separation of transport-compat vs role-eligibility is the intended design per the incident doc.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| if not is_chat_compatible_model_id(agent.model): | ||
| return { | ||
| "agent_id": agent.id, | ||
| "model": agent.model, | ||
| "status": "not_ready", | ||
| "latency_ms": round((time.monotonic() - started) * 1000, 2), | ||
| "error_type": "ValueError", | ||
| "failure_code": "non_chat_model", | ||
| } |
There was a problem hiding this comment.
🔍 Readiness probe reports embedding agents as not_ready
ModelClient.probe now returns status=not_ready, failure_code=non_chat_model before any transport for any agent whose model id is not chat-compatible (orchestrator.py:985-993). Provider readiness reports iterate enabled workers, so a correctly configured embedding agent (model id like text-embedding-3-large) will always surface as not_ready in /api/v1/provider_readiness/latest. The incident doc item 9 states this is intended for chat transport, but combined with BUG-0001 it means embedding agents are effectively invisible/unusable through both readiness and auto-selection paths. Reviewer should confirm the readiness surface for embedding-only pools is acceptable.
Was this helpful? React with 👍 or 👎 to provide feedback.
| normalized_endpoint = endpoint.strip("/") | ||
| if normalized_endpoint.startswith("v1/"): | ||
| normalized_endpoint = normalized_endpoint[3:] | ||
| if ( | ||
| normalized_endpoint in {"chat/completions", "completions", "responses"} | ||
| and not is_chat_compatible_model_id(agent.model) | ||
| ): | ||
| raise ValueError( | ||
| f"model {agent.model!r} is not chat-compatible and cannot serve {agent.id!r}" | ||
| ) | ||
| if agent.base_url.startswith("mock://"): | ||
| return self._mock_raw(agent, normalized_endpoint, payload) |
There was a problem hiding this comment.
📝 Info: proxy_send endpoint normalization fixes prior double-v1 path
proxy_send/proxy_send_once now strip a leading v1/ from the endpoint before building the provider URL (orchestrator.py:1376-1378, 1402-1404). Previously passing /v1/chat/completions or /v1/responses would (a) build a double /v1/v1/... provider path via _provider_url and (b) fail the endpoint.strip("/") == "responses" mock/local-responses detection. The normalization is a genuine behavioral fix, exercised by the new parametrized test_local_responses_passthrough_adapts_to_chat_transport. No current production caller passed a v1/-prefixed endpoint, so this only adds tolerance.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def select_cheapest_discovered_agent( | ||
| discovered: list[DiscoveredModel], price_book: "PriceBook" | ||
| ) -> DiscoveredModel | None: | ||
| """Pick the lowest-cost discovered model per the price book (auto-optimization). | ||
| """Pick the lowest-cost general chat-agent model per the price book. | ||
|
|
||
| Reuses :func:`~contextual_orchestrator.batch_routing.cheapest_upstream`, the | ||
| existing cost-optimizing upstream selector. Call :func:`refresh_price_book` | ||
| first so discovered pricing is visible; an unpriced candidate costs ``0`` | ||
| Uses the same representative request cost as the top-N selector. Call | ||
| :func:`refresh_price_book` first so discovered pricing is visible; an | ||
| unpriced candidate costs ``0`` | ||
| under that selector's documented contract and is treated as free, not | ||
| unknown -- so a genuinely unpriced provider (e.g. Bytez, priced by | ||
| GPU-second rather than per token) will always look cheapest here. Fine for | ||
| "auto-pick something free to try," but callers doing real cost comparison | ||
| should refresh pricing for every candidate they care about first. | ||
| """ | ||
| if not discovered: | ||
| eligible = [model for model in discovered if is_general_chat_agent_model_id(model.model_id)] | ||
| if not eligible: | ||
| return None | ||
| candidates = [{"provider": model.provider_name, "model": model.model_id} for model in discovered] | ||
| winner = cheapest_upstream(candidates, price_book) | ||
| if winner is None: | ||
| return None | ||
| for model in discovered: | ||
| if model.provider_name == winner["provider"] and model.model_id == winner["model"]: | ||
| return model | ||
| return None # pragma: no cover - winner always comes from candidates | ||
| return min(eligible, key=lambda model: _discovered_cost(model, price_book)) | ||
|
|
||
|
|
||
| def select_top_n_cheapest_discovered_agents( | ||
| discovered: list[DiscoveredModel], price_book: "PriceBook", limit: int | ||
| ) -> list[DiscoveredModel]: | ||
| """Return the ``limit`` lowest-cost discovered models, cheapest first. | ||
|
|
||
| For bootstrapping a CI sidecar (or any first-boot pool) with more than one | ||
| enabled agent for failover, without hand-picking which discovered models to | ||
| trust. Same pricing contract as :func:`select_cheapest_discovered_agent`. | ||
| """ | ||
| if limit <= 0 or not discovered: | ||
| """Return the ``limit`` cheapest general chat-agent models in ascending cost.""" | ||
| if limit <= 0: | ||
| return [] | ||
| eligible = [model for model in discovered if is_general_chat_agent_model_id(model.model_id)] | ||
| if not eligible: | ||
| return [] | ||
|
|
||
| return sorted(eligible, key=lambda model: _discovered_cost(model, price_book))[:limit] | ||
|
|
||
| def _cost(model: DiscoveredModel) -> float: | ||
| cost, _currency = price_book.compute_cost(model.provider_name, model.model_id, 1000, 1000) | ||
| return cost | ||
|
|
||
| return sorted(discovered, key=_cost)[:limit] | ||
| def _discovered_cost(model: DiscoveredModel, price_book: "PriceBook") -> float: | ||
| """Price the representative discovery request used by both selectors.""" | ||
| cost, _currency = price_book.compute_cost(model.provider_name, model.model_id, 1000, 1000) | ||
| return cost |
There was a problem hiding this comment.
📝 Info: cheapest-agent refactor keeps assumed 1000/1000 cost and input-order ties
select_cheapest_discovered_agent was rewritten from cheapest_upstream to min(eligible, key=_discovered_cost) (model_discovery.py:319-322, 338-341). _discovered_cost uses compute_cost(..., 1000, 1000), matching cheapest_upstream's default assumed shape, and min keeps the first minimum (input order) like cheapest_upstream's documented tie behavior, so selection results are equivalent aside from the new eligibility filter. No behavior regression here.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@opencode-agent Review exact current HEAD |
1 participant
Why
Protected
fix/auto-reasoning-effort-contract-rebased@ba8b4b8ab181bcd2c64b871d0984006452e4c753contains the integrated HTTP-honesty stack and OpenAI-compatible front door. One buyer-visible reliability gap remains: raw tool, structured-output, and Responses requests can consume same-model retries on a saturated or retired provider candidate without advancing to another configured model.Current reliability slice
contextual-orchestratorand omitted-model passthrough may advance across capability-ranked agents.raise ... from causeis authoritative; context deliberately suppressed withraise ... from Noneis not traversed.tools,tool_choice,response_format, Responses input, endpoint choice, and caller payload immutability are preserved.python -m contextual_orchestrator --servepath constructs the failover-capable orchestrator.Local-provider semantic repair
A first one-shot implementation called
_send_rawdirectly. That removed retries but also bypassed two behaviors owned by the built-inModelClient.proxy_sendpath:The current implementation publishes
ModelClient.proxy_send_once()at the owning transport layer. It preserves those semantics while replacing only_send_raw_with_retrywith one_send_rawcall. Focused regressions lock local Responses translation, local model-switch coordination, caller immutability, and proof that the retry wrapper is never entered.Test-first evidence
The branch records RED regressions before the corresponding bounded production repairs for:
Exact current identity
fix/auto-reasoning-effort-contract-rebased@ba8b4b8ab181bcd2c64b871d0984006452e4c753531c74f49f228929425b485838f18e355aaa0cdfExact-head repository Tests, Fuzz, Security, Security Scan, Semgrep, semantic review, and protected integration remain authoritative.
Dependencies and reconciliation
This PR owns passthrough failover only. PR #768 owns ordinary-chat capability isolation, PR #769 owns the runtime/test dependency boundary, and PR #770 owns honest provider-diverse discovery bootstrap. After #768 lands, this branch must rebase and ensure
proxy_send_once()applies the shared chat-transport gate before mock or provider I/O and that every adaptive candidate is ordinary-role eligible. The branch is now reconciled with #765 atba8b4b8ab181bcd2c64b871d0984006452e4c753in merge commit531c74f49f228929425b485838f18e355aaa0cdf; future parent movement requires another exact-head refresh.Keep Draft until prerequisite integration/reconciliation, exact-head verification, and a qualifying current-head formal review complete. No bypass, self-approval, or predecessor-head evidence transfer.
Review repair: preserve embedding auto-selection
_ranked_agentskeeps the general-chat model-id filter for chat roles and failover.select_capability_agentopts out of that chat-only filter, so real embedding identifiers such astext-embedding-3-largeremain selectable by the embeddings endpoint.text-embedding-3-largeinstead of a chat-shaped mock model.Exact validation on head
531c74f49f228929425b485838f18e355aaa0cdf:git diff --check: passed2026-08-21 parent reconciliation
Head
615f0b64078781cc7fa7d7f747737d50657971a2is reconciled with parent #765 at39072a654261c3570496849bb4da1e2c340e2fbc. A livegpt-5.6-solAzure rejection exposed that the one-shot passthrough path bypassed ADR 0016 temperature capability negotiation. The current head reuses the shared raw transport with transient retries disabled: HTTP 429 still makes exactly one provider call and preserves the original error, while an explicit unsupported temperature may be removed once on the same endpoint. Focused passthrough/provider tests passed (28); the exact combined repository suite passed 1,600 tests in 541.73 seconds. Hosted checks and independent protected approval remain authoritative.