From 0e42fbcaf341532616ef0a83404dce8d0fd5b42a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 09:18:00 +0900 Subject: [PATCH 1/3] fix(sidecar): keep transient-rejected preflight routes as deferred failover The review sidecar's preflight discarded a route the moment its 16-token probe raised, including on 429 and 5xx, while the serving gateway treats exactly those statuses as transient: it retries the route and fails over across it (provider_errors.PROVIDER_STATUS_SURFACES marks 429 retryable; orchestrator.TRANSIENT_HTTP_STATUS). Under concurrent CI load the probes spend the per-key budgets themselves: noema-review run 33993637015 (.github#1687, 2026-09-05) rejected 11 of 12 routes -- six with 429, three of them on NVIDIA keys whose sibling routes were ready -- served the single ready route for 542 s and returned 502. Routes whose probe answered with a status in the gateway's transient set are now kept as deferred, ranked after every ready route by a catalog priority penalty, so failover has somewhere to go. A probe that timed out records no http_status and stays rejected, so the silent route whose request costs the full two-layer retry budget is never admitted on that evidence. ready_count is unchanged, deferred_count is reported, and with no ready route the stage still fails, so ADR-0005's priced-catalog fallback contract is untouched. The escalation path is not touched. The stream sanitizer admits preflight_route_deferred next to preflight_route_rejected. Tests: deferral order and priorities, all-transient still fails, frozen and plain agent demotion, deferred log line, sanitizer pass/drop/scrub; all five fail against main's launcher and sanitizer. Gate on this tree: 2909 passed, coverage 100%, interrogate 100% (final commit differs from the gated tree by one comment; the touched module and interrogate re-run). Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 4 + ...contextual_orchestrator_review_launcher.py | 68 +++++++++- ..._contextual_orchestrator_sidecar_stream.py | 5 +- ...l_orchestrator_review_runtime_preflight.py | 121 ++++++++++++++++++ 4 files changed, 191 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55a2a2f211..600728538b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +### Review sidecar preflight keeps transient-rejected routes as deferred failover + +- `_preflight_review_agents` no longer discards a route whose 16-token probe answered with a status the serving gateway itself retries and fails over across (`408 409 425 429 500 502 503 504 529`, the vendored orchestrator's `TRANSIENT_HTTP_STATUS`). Such routes are kept as **deferred**, ranked after every ready route by a catalog-priority penalty, so a stalled or rate-limited ready route has somewhere to fail over to; `ready_count` is unchanged, a new `deferred_count` is reported, and `rejected_count` covers only routes the gateway would not retry either (404, auth failures, invalid responses). With no ready route the stage still fails as before, so ADR-0005's priced-catalog fallback contract is untouched. Motivation: `noema-review` run 33993637015 (2026-09-05) rejected 11 of 12 routes -- six with 429, three of them on NVIDIA keys whose sibling routes were ready -- served the single ready route for 542 s and returned 502; under this rule the same run would have served 1 ready + 6 deferred. The sanitized stream gains a `preflight_route_deferred` line alongside `preflight_route_rejected`. + ### Noema review ships sidecar evidence on failure - `noema-review.yml` now uploads `strix_runs/contextual-orchestrator-sidecar.stderr.log` and `strix_runs/contextual-orchestrator-preflight.json` as the `noema-sidecar-evidence` artifact when the verdict phase fails (`if: failure()`, the same pinned `actions/upload-artifact` Strix uses, `if-no-files-found: ignore`, 5-day retention). Until now a failed Noema run left `artifacts=0` -- run `33981136873` spent 3122 s walking six ready routes twice each and ended in HTTP 502 with no per-route trace anywhere but the sidecar's stderr -- so the only diagnosis available was the caller's one-line summary. The stderr file is the sanitizer's bounded allowlist output (`sanitize_contextual_orchestrator_sidecar_stream.py`), the same file Strix already publishes in `strix-reports`; per-attempt route outcomes still need an allowlisted structured line from the orchestrator to appear in it. Refs #1935, #1939. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 502843c994..85f0fbc4b1 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -22,6 +22,8 @@ from __future__ import annotations import argparse +import copy +import dataclasses import json import logging import os @@ -63,6 +65,28 @@ # Shared cap on how many candidates in one preflight run may use the # escalation retry above. It bounds request count, never model response time. REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4 +# Probe outcomes the serving gateway itself treats as transient -- it retries +# the same route and then fails over across exactly these statuses +# (contextual_orchestrator.orchestrator.TRANSIENT_HTTP_STATUS at the vendored +# pin; provider_errors.PROVIDER_STATUS_SURFACES marks 429 retryable). A route +# that answered one of them to the 16-token probe is not known to be dead; it +# was rate-limited or unlucky in the second the probe ran, very often because +# the probe itself spent the per-key budget. Discarding it left the serving +# set with nothing to fail over to: on 2026-09-05 a noema-review preflight +# rejected 11 of 12 routes -- six of them with 429 -- served the one ready +# route for 542 s and returned 502. Such routes are kept as *deferred*, ranked +# after every ready route, so failover has somewhere to go. Only a route that +# *answered* with one of these statuses qualifies (a probe that timed out +# records no http_status and stays rejected), so deferral never admits, on the +# strength of a probe that already showed it, the silent route whose request +# costs the full two-layer retry budget. Keep this set in sync with the +# vendored orchestrator's; a status the gateway would not retry must not be +# deferred. +REVIEW_PREFLIGHT_DEFERRABLE_HTTP_STATUS = frozenset({408, 409, 425, 429, 500, 502, 503, 504, 529}) +# Subtracted from a deferred route's catalog priority so the orchestrator's +# ranking (higher priority first; catalog priorities are 0..-11) never places a +# deferred route ahead of a ready one. +REVIEW_PREFLIGHT_DEFERRED_PRIORITY_PENALTY = 1000 class ReviewPreflightError(RuntimeError): @@ -280,6 +304,22 @@ def _record_provider_exception(row: dict[str, object], exc: Exception) -> None: row.pop("reasoning_without_content", None) +def _demote_agent(agent: object, penalty: int) -> object: + """Return a copy of ``agent`` whose ``priority`` is lowered by ``penalty``. + + Serving agents are frozen ``ModelAgent`` dataclasses, so the copy goes + through :func:`dataclasses.replace`; the plain objects tests use are + shallow-copied and assigned. A missing ``priority`` counts as 0, matching + the dataclass default. + """ + priority = int(getattr(agent, "priority", 0)) - penalty + if dataclasses.is_dataclass(agent) and not isinstance(agent, type): + return dataclasses.replace(agent, priority=priority) + demoted = copy.copy(agent) + demoted.priority = priority + return demoted + + def _response_has_reasoning_without_content(response: object) -> bool: """Return whether a response matches the vendored "reasoning, no content" signature. @@ -508,11 +548,28 @@ def _preflight_review_agents( ) routes.append(row) + # Deferral pass: a route rejected with a status the serving gateway would + # retry and fail over across is kept behind the ready routes instead of + # being discarded -- but only once at least one route is ready. With no + # ready route the run still fails this stage exactly as before, so + # _preflight_with_fallback's "priced catalog only after every primary + # route rejects" contract (ADR-0005) is unchanged. ``routes`` holds one + # row per agent in ``agents`` order (every branch above appends once). + deferred: list[object] = [] + if viable: + for agent, row in zip(agents, routes): + if ( + row.get("status") == "rejected" + and row.get("http_status") in REVIEW_PREFLIGHT_DEFERRABLE_HTTP_STATUS + ): + row["status"] = "deferred" + deferred.append(_demote_agent(agent, REVIEW_PREFLIGHT_DEFERRED_PRIORITY_PENALTY)) report: dict[str, object] = { "contract": "strix-plain-chat-preflight-v2", "probed_count": len(agents), "ready_count": len(viable), - "rejected_count": len(agents) - len(viable), + "deferred_count": len(deferred), + "rejected_count": len(agents) - len(viable) - len(deferred), "escalations_used": escalations_used, "escalation_budget": REVIEW_PREFLIGHT_MAX_ESCALATIONS, "routes": routes, @@ -521,7 +578,7 @@ def _preflight_review_agents( raise ReviewPreflightError( "no provider route passed the Strix plain-chat preflight", report ) - return viable, report + return [*viable, *deferred], report def _preflight_with_fallback( @@ -581,8 +638,9 @@ def _log_preflight_rejections(report: dict[str, object]) -> None: if not isinstance(routes, list): return for row in routes: - if not isinstance(row, dict) or row.get("status") != "rejected": + if not isinstance(row, dict) or row.get("status") not in ("rejected", "deferred"): continue + event = f"preflight_route_{row['status']}" # Re-validate rather than trust the caller's own sanitization: this # print reaches the sidecar's sanitized stderr stream unchanged, so an # out-of-contract value here (not a plain identifier) must degrade to @@ -602,13 +660,13 @@ def _log_preflight_rejections(report: dict[str, object]) -> None: http_status = row.get("http_status") if isinstance(http_status, int) and not isinstance(http_status, bool) and 100 <= http_status <= 599: print( - f"preflight_route_rejected provider={provider} " + f"{event} provider={provider} " f"error_type={error_type} http_status={http_status}", file=sys.stderr, ) else: print( - f"preflight_route_rejected provider={provider} error_type={error_type}", + f"{event} provider={provider} error_type={error_type}", file=sys.stderr, ) diff --git a/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py b/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py index 51b9a5df27..a3227f2e32 100644 --- a/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py +++ b/scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py @@ -16,7 +16,7 @@ r"code=(?P[A-Za-z0-9_.-]{1,64})" ) _PREFLIGHT_ROUTE_REJECTED = re.compile( - r"preflight_route_rejected provider=(?P[a-z][a-z0-9_]{0,63}) " + r"preflight_route_(?Prejected|deferred) provider=(?P[a-z][a-z0-9_]{0,63}) " r"error_type=(?P[A-Za-z_][A-Za-z0-9_]{0,63})" r"(?: http_status=(?P[1-5][0-9]{2}))?" ) @@ -116,7 +116,8 @@ def sanitize_line(line: str) -> str | None: preflight_route_rejected = _PREFLIGHT_ROUTE_REJECTED.search(stripped) if preflight_route_rejected is not None: summary = ( - f"preflight_route_rejected provider={preflight_route_rejected.group('provider')} " + f"preflight_route_{preflight_route_rejected.group('event')} " + f"provider={preflight_route_rejected.group('provider')} " f"error_type={preflight_route_rejected.group('error_type')}" ) http_status = preflight_route_rejected.group("http_status") diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index d0ace81e8b..902237bdad 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1943,3 +1943,124 @@ def test_main_configures_sidecar_logging_before_touching_credentials() -> None: credentials_at = source.index("registered = register_review_credentials(os.environ)") assert configure_at < credentials_at assert "from contextual_orchestrator.debug_logging import configure_logging" in source + + +def _preflight_agents(*ids: str) -> list[SimpleNamespace]: + """Return catalog-shaped agents with descending priorities, one per id.""" + return [ + SimpleNamespace(id=agent_id, provider_name=agent_id.split("_")[0], model=f"{agent_id}/m", priority=-index) + for index, agent_id in enumerate(ids) + ] + + +class _StatusError(Exception): + """Exception with a ``code`` attribute, the shape ``_safe_http_status`` reads.""" + + def __init__(self, code: int) -> None: + super().__init__(f"HTTP Error {code}") + self.code = code + + +def test_preflight_defers_transient_probe_statuses_behind_ready_routes() -> None: + """A 429/5xx probe answer keeps the route, ranked after every ready route. + + noema-review run 33993637015 (2026-09-05) rejected 11 of 12 routes -- six + with 429 -- served the single ready route for 542 s and returned 502. The + serving gateway retries and fails over across exactly these statuses, so + discarding them at preflight left it nowhere to go. + """ + namespace = _load_launcher() + agents = _preflight_agents("nvidia_ready", "openrouter_limited", "nvidia_missing", "nvidia_down") + client = _ProbeClient( + { + "nvidia_ready": {"choices": [{"message": {"content": "OK"}, "finish_reason": "stop"}]}, + "openrouter_limited": _StatusError(429), + "nvidia_missing": _StatusError(404), + "nvidia_down": _StatusError(503), + } + ) + served, report = namespace["_preflight_review_agents"](agents, client=client) + + assert [agent.id for agent in served] == ["nvidia_ready", "openrouter_limited", "nvidia_down"] + assert served[0].priority == 0 + penalty = namespace["REVIEW_PREFLIGHT_DEFERRED_PRIORITY_PENALTY"] + assert served[1].priority == -1 - penalty + assert served[2].priority == -3 - penalty + assert agents[1].priority == -1, "deferral must not mutate the caller's agent" + assert report["ready_count"] == 1 + assert report["deferred_count"] == 2 + assert report["rejected_count"] == 1 + statuses = {row["agent_id"]: row["status"] for row in report["routes"]} + assert statuses == { + "nvidia_ready": "ready", + "openrouter_limited": "deferred", + "nvidia_missing": "rejected", + "nvidia_down": "deferred", + } + + +def test_preflight_still_fails_when_no_route_is_ready() -> None: + """All-transient rejections keep failing the stage so the priced fallback still runs.""" + namespace = _load_launcher() + agents = _preflight_agents("openrouter_a", "nvidia_b") + client = _ProbeClient({"openrouter_a": _StatusError(429), "nvidia_b": _StatusError(429)}) + with pytest.raises(namespace["ReviewPreflightError"]) as excinfo: + namespace["_preflight_review_agents"](agents, client=client) + report = excinfo.value.report + assert report["ready_count"] == 0 + assert report["deferred_count"] == 0 + assert report["rejected_count"] == 2 + assert {row["status"] for row in report["routes"]} == {"rejected"} + + +def test_demote_agent_handles_frozen_dataclasses_and_plain_objects() -> None: + """The serving ``ModelAgent`` is a frozen dataclass; test doubles are plain objects.""" + import dataclasses + + namespace = _load_launcher() + + @dataclasses.dataclass(frozen=True) + class _Frozen: + id: str + priority: int = 0 + + frozen = _Frozen(id="a", priority=-2) + demoted = namespace["_demote_agent"](frozen, 1000) + assert demoted.priority == -1002 and frozen.priority == -2 + plain = SimpleNamespace(id="b") + demoted_plain = namespace["_demote_agent"](plain, 1000) + assert demoted_plain.priority == -1000 and not hasattr(plain, "priority") + + +def test_log_preflight_rejections_reports_deferred_routes(capsys: pytest.CaptureFixture[str]) -> None: + """Deferred routes get their own bounded line so the stream tells them apart.""" + namespace = _load_launcher() + namespace["_log_preflight_rejections"]( + { + "routes": [ + {"provider": "openrouter", "status": "deferred", "error_type": "HTTPError", "http_status": 429}, + {"provider": "nvidia_nim", "status": "rejected", "error_type": "HTTPError", "http_status": 404}, + {"provider": "nvidia_nim_sub", "status": "ready"}, + ] + } + ) + err = capsys.readouterr().err + assert "preflight_route_deferred provider=openrouter error_type=HTTPError http_status=429" in err + assert "preflight_route_rejected provider=nvidia_nim error_type=HTTPError http_status=404" in err + assert "nvidia_nim_sub" not in err + + +def test_sidecar_stream_sanitizer_passes_deferred_preflight_lines() -> None: + """``preflight_route_deferred`` reaches the artifact with the same bounded fields as rejected.""" + sanitizer = _load_sanitizer() + sanitize_line = sanitizer["sanitize_line"] + deferred = "preflight_route_deferred provider=openrouter error_type=HTTPError http_status=429" + rejected = "preflight_route_rejected provider=nvidia_nim error_type=HTTPError http_status=404" + assert sanitize_line(deferred) == deferred + assert sanitize_line(rejected) == rejected + assert sanitize_line("preflight_route_deferred provider=openrouter error_type=HTTPError") == ( + "preflight_route_deferred provider=openrouter error_type=HTTPError" + ) + assert sanitize_line("preflight_route_paused provider=openrouter error_type=HTTPError http_status=429") is None + assert sanitize_line(deferred + " token=sk-secret") is not None + assert "sk-secret" not in sanitize_line(deferred + " token=sk-secret") From 53c0a87b7fa50a7d1d3f402198922d4cd3ad76a7 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 10:23:23 +0900 Subject: [PATCH 2/3] docs(sidecar): describe the deferral bound in passthrough terms The review request is served by the passthrough path (one attempt per candidate via proxy_send_once, stream=False), so a silent route costs one socket-silence timeout, not a multi-layer retry budget. Comment-only. Co-Authored-By: Claude Fable 5.1 --- scripts/ci/contextual_orchestrator_review_launcher.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 85f0fbc4b1..7cd94e2d77 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -78,10 +78,10 @@ # after every ready route, so failover has somewhere to go. Only a route that # *answered* with one of these statuses qualifies (a probe that timed out # records no http_status and stays rejected), so deferral never admits, on the -# strength of a probe that already showed it, the silent route whose request -# costs the full two-layer retry budget. Keep this set in sync with the -# vendored orchestrator's; a status the gateway would not retry must not be -# deferred. +# strength of a probe that already showed it, the silent route whose single +# passthrough attempt would sit through the socket-silence timeout. Keep this +# set in sync with the vendored orchestrator's; a status the gateway would not +# retry must not be deferred. REVIEW_PREFLIGHT_DEFERRABLE_HTTP_STATUS = frozenset({408, 409, 425, 429, 500, 502, 503, 504, 529}) # Subtracted from a deferred route's catalog priority so the orchestrator's # ranking (higher priority first; catalog priorities are 0..-11) never places a From 37a1129aeaa27ebc8b3d8999c83b060008d0123d Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 10:29:47 +0900 Subject: [PATCH 3/3] docs(sidecar): state the deferral bound in terms of the gateway retry budget The first noema-sidecar-evidence trace (.github#1661 run 33995553859) shows the review path is _invoke: a silent route costs two rounds of three 90 s timeouts. Word the constant's comment on that basis instead of the single-attempt passthrough wording of the previous commit. Comment-only. Co-Authored-By: Claude Fable 5.1 --- scripts/ci/contextual_orchestrator_review_launcher.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 7cd94e2d77..27917eba4e 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -78,10 +78,10 @@ # after every ready route, so failover has somewhere to go. Only a route that # *answered* with one of these statuses qualifies (a probe that timed out # records no http_status and stays rejected), so deferral never admits, on the -# strength of a probe that already showed it, the silent route whose single -# passthrough attempt would sit through the socket-silence timeout. Keep this -# set in sync with the vendored orchestrator's; a status the gateway would not -# retry must not be deferred. +# strength of a probe that already showed it, the silent route whose serving +# request would spend the gateway's full retry budget in 90 s timeouts. Keep +# this set in sync with the vendored orchestrator's; a status the gateway +# would not retry must not be deferred. REVIEW_PREFLIGHT_DEFERRABLE_HTTP_STATUS = frozenset({408, 409, 425, 429, 500, 502, 503, 504, 529}) # Subtracted from a deferred route's catalog priority so the orchestrator's # ranking (higher priority first; catalog priorities are 0..-11) never places a