From 6d897c990e473a9d3a34bd1b6547f39b9bc344fb Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 10:45:04 +0900 Subject: [PATCH 1/4] fix(sidecar): fill the preflight served set lazily to a readiness target The catalog becomes a candidate list (up to 24, per-account cap 8, the tier-then-round-robin order from #1939) and _preflight_review_agents probes it in order, stopping at REVIEW_PREFLIGHT_TARGET_READY = 8 ready routes or REVIEW_PREFLIGHT_MAX_PROBES = 16 probes, whichever comes first. A permanently dead candidate (NIM lists gemma-3-12b/4b and answers 404 on every run) costs one probe instead of a served slot; a healthy pool stops early. Why: after #1939's four-per-account slice, each NVIDIA key's slots were its first four models alphabetically -- deepseek-v4-flash, deepseek-v4-pro and the two gemma-3 404s -- so each key served two contended routes. Preflight ready fell from 6/12 to 1-3/12 and noema-review in .github went from 7 successes / 14 failures to 0 / 22 (lane jan's census on #1948, verdict-step conclusions). - REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES 12 -> 24, PRIMARY_ROUTE_LIMIT 8 -> 16; sidecar ORCHESTRATOR_CATALOG_LIMIT default and the composite action's catalog_limit default 12 -> 24. - Report gains candidate_count, target_ready, probe_budget; probed_count now counts probes sent; rejected_count = probed - ready - deferred. #1947's deferral pass pairs exactly the probed prefix. - ADR-0029 (new; 0028 is claimed by #1668), ADR-0003 section 2 stage-budget sentence amended, CHANGELOG. Tests: four new lazy-fill tests plus the updated stage-limit invariant; all five fail on #1947's tree (negative control). Gate: 2913 passed, 1 skipped, coverage 100% (13150 statements, 0 missed), interrogate 100%. Refs #1939, #1947, #1948. Co-Authored-By: Claude Fable 5.1 --- .../orchestrator-free-sidecar/action.yml | 4 +- CHANGELOG.md | 4 + ...ntextual-orchestrator-vendored-free-zdr.md | 9 +- docs/adr/0029-sidecar-preflight-lazy-fill.md | 47 ++++++++ ...contextual_orchestrator_review_launcher.py | 41 +++++-- .../contextual_orchestrator_review_sidecar.sh | 7 +- ...l_orchestrator_review_runtime_preflight.py | 111 +++++++++++++++++- 7 files changed, 201 insertions(+), 22 deletions(-) create mode 100644 docs/adr/0029-sidecar-preflight-lazy-fill.md diff --git a/.github/actions/orchestrator-free-sidecar/action.yml b/.github/actions/orchestrator-free-sidecar/action.yml index 196c86b0f6..edddfe1bc3 100644 --- a/.github/actions/orchestrator-free-sidecar/action.yml +++ b/.github/actions/orchestrator-free-sidecar/action.yml @@ -6,9 +6,9 @@ inputs: required: false default: "false" catalog_limit: - description: Maximum discovered route catalog size for the sidecar preflight. + description: Maximum discovered route catalog size for the sidecar preflight (a candidate list probed lazily to a readiness target, ADR-0029). required: false - default: "12" + default: "24" catalog_account_cap: description: Maximum routes admitted from one credential account. required: false diff --git a/CHANGELOG.md b/CHANGELOG.md index 600728538b..cdb7632af3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +### Review sidecar preflight fills the served set lazily to a readiness target + +- `_preflight_review_agents` now treats the catalog as a candidate list, probed in its tier-then-round-robin order until `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes are spent (ADR-0029). The candidate budget rises from 12 to 24 (`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`; auto pool split 16 free / 8 priced; the sidecar's `ORCHESTRATOR_CATALOG_LIMIT` default follows), the per-account cap stays 8. A permanently dead candidate -- NIM lists `gemma-3-12b`/`gemma-3-4b` and answers 404 on every run -- now costs one probe instead of a served slot, and a healthy pool stops early instead of always probing every candidate. Motivation: after #1939's four-per-account slice each NVIDIA key's slots were its first four models alphabetically, two of them those 404s, so preflight readiness fell from 6/12 to 1–3/12 and `noema-review` on this repository went from 7 successes / 14 failures to 0 / 22. The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_count` counts probes actually sent. ADR-0003's stage-budget sentence is amended. Refs #1939, #1947, #1948. + ### 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`. diff --git a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md index 04dc04c7a2..a39f0595cf 100644 --- a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md +++ b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md @@ -50,9 +50,12 @@ all five, and auto-optimize routing by cost. route rejects the real runtime request contract does it rebuild once from fully price-attested routes and record the rejected primary attempt. This is evidence-triggered failover, not an arbitrary free/paid mixing ratio. - Both stages share one twelve-route startup budget: no more than eight routes - enter the free primary stage and only its remaining capacity may enter priced - fallback. Full discovery counts remain in policy evidence, and the transient + Both stages share one bounded startup budget of twenty-four candidates: no + more than sixteen enter the free primary stage and only its remaining + capacity may enter priced fallback. Candidates are probed lazily in catalog + order until eight routes are ready or sixteen probes are spent per stage + (ADR-0029), so a dead candidate costs one probe, not a served slot. Full + discovery counts remain in policy evidence, and the transient priced catalog is removed immediately after loading. 3. **ZDR-first within each cost tier**: `scripts/ci/zdr_policy.py` defines ZDR the way OpenRouter does ("a provider will not store your data for any period diff --git a/docs/adr/0029-sidecar-preflight-lazy-fill.md b/docs/adr/0029-sidecar-preflight-lazy-fill.md new file mode 100644 index 0000000000..e5abca67e0 --- /dev/null +++ b/docs/adr/0029-sidecar-preflight-lazy-fill.md @@ -0,0 +1,47 @@ +# ADR-0029: Review sidecar preflight fills the served set lazily to a readiness target + +- **Status:** Proposed +- **Date:** 2026-09-06 +- **Scope:** `scripts/ci/contextual_orchestrator_review_launcher.py` (`_preflight_review_agents`, the stage limits), `scripts/ci/contextual_orchestrator_review_sidecar.sh` (`ORCHESTRATOR_CATALOG_LIMIT` default), ADR-0003 §2's stage budget sentence +- **Amends:** ADR-0003 (the "twelve-route startup budget" clause). ADR-0005's attempt counts are historical and are not restored. + +## Problem + +The review sidecar selected a fixed catalog of twelve routes and probed every one of them, then served whatever was ready. `.github#1939` made the selection diverse (round-robin across credential accounts inside each cost/ZDR tier, four routes per account), which was right, but it exposed a second defect: the per-account slice is filled from an alphabetically sorted model list, and for both NVIDIA NIM keys the first four models are `deepseek-v4-flash`, `deepseek-v4-pro`, `gemma-3-12b`, `gemma-3-4b`. NIM lists the two `gemma-3` models but answers `404` to every chat request on every run observed. Each NVIDIA key therefore served two working routes, both the most contended models, while the pre-#1939 eight-slot fill had reached `meta/llama-3.2-11b`, `llama-3.2-90b` and `meta/muse-glimmer-30b`, which were ready in every Strix artifact of that afternoon. + +Measured on `ContextualWisdomLab/.github` (lane jan's census on `#1948`, verdict-step conclusions only, draft skips excluded): + +| window | preflight ready of 12 | `noema-review` success / failure | +|---|---|---| +| before `#1939` (`main@f2f91b80`, 2026-09-05T17:25Z) | 6, 6, 5 (16:37–16:56Z artifacts) | 7 / 14 | +| after | 1–3 (23:47Z onward) | 0 / 22 | + +The evening's rate-limit pressure is a confound; the mechanism is not. A fixed slice from a list with dead entries wastes the slice, and probing every candidate regardless of how many are already ready spends per-key rate budget (`#1948`) for nothing. + +## Constraints + +1. No model name is hard-coded anywhere in the fill; a dead candidate is discovered by its probe, not by a list. +2. Probe spend per sidecar boot stays bounded and is stated as a number, because the probes themselves consume the per-key budgets the served routes need (`#1948`). +3. `#1947`'s deferral (a probed route that answered a transient status is kept behind the ready routes) applies unchanged to whatever was probed. +4. ADR-0003's evidence-triggered priced fallback (only after every free candidate rejects) keeps its shape; the two stages still share one startup budget. +5. `ready_count` keeps its meaning (routes proven ready by a probe) so the peers' post-merge discriminators stay comparable. + +## Decision + +The catalog is a **candidate list**, not the served set. `build_zdr_prioritized_catalog` keeps its tier-then-round-robin order (`#1939`) and is asked for up to `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24` candidates (per-account cap unchanged at 8; the sidecar's `ORCHESTRATOR_CATALOG_LIMIT` default rises from 12 to 24). `_preflight_review_agents` probes candidates **in that order and stops** as soon as `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes have been spent, whichever comes first. The auto pool's split becomes 16 free candidates and up to 8 priced fallback candidates; each stage's list fits the probe cap, so the worst case remains "every candidate probed once". + +The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_count` now counts probes actually sent, and `rejected_count` is `probed − ready − deferred`. Unprobed candidates get no `routes` row. + +## Consequences + +- **Good:** a dead candidate costs one probe and yields its place to the next candidate in the same account's list; a healthy hour stops after about eight to twelve probes instead of always twelve; a bad hour is bounded at sixteen probes per stage. +- **Cost:** in an hour where nothing is ready the sidecar sends up to 16 probes per stage where it sent 12, a third more against already exhausted keys. This is the price of finding routes past the dead ones; `#1948`'s shared rate ledger is the lever above it. +- **Unchanged:** a route that answers the probe and then goes silent at request time still costs the gateway's full retry budget (`contextual-orchestrator#1045`); readiness is measured at 16 tokens (`#1454`). +- **Discriminator:** post-merge, `probed_count` versus `candidate_count` per boot and `ready_count` of the served set, read from the `runtime preflight summary` in the job log or the `noema-sidecar-evidence` artifact, compared with the table above. + +## Alternatives considered + +- **Raise the per-account cap back to 8 with a 12-route limit** — restores the pre-#1939 pool but reintroduces the single-account fill that `#1939` fixed; the 404s would still occupy slots. +- **Exclude models that 404 by name** — a hard-coded exclusion list the next discovery change silently invalidates; rejected by constraint 1. The discovery-side question (why NIM lists models it does not serve) remains open in `contextual-orchestrator`. +- **Family-level round-robin before the per-account cap** (jan's fallback proposal) — reduces same-family contention but does not touch dead candidates; can be layered later if the census shows family contention as the residual. +- **Probe all 24 candidates** — best served set, double the probe spend in the hour that can least afford it; rejected by constraint 2. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 27917eba4e..cb95df8b74 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -45,8 +45,18 @@ # Provider-neutral sampling: several modern endpoints reject non-default # temperatures, while 1.0 is the OpenAI-compatible default. REVIEW_TEMPERATURE = 1.0 -REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 12 -REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT = 8 +# Lazy fill (ADR-0029): the catalog is a *candidate* list, probed in its +# tier-then-round-robin order until REVIEW_PREFLIGHT_TARGET_READY routes are +# ready or REVIEW_PREFLIGHT_MAX_PROBES probes are spent, whichever comes first. +# A permanently dead candidate (NIM lists gemma-3-12b/4b but answers 404 on +# every run) then costs one probe instead of a served slot, and a healthy hour +# stops early instead of always probing every candidate. The two stage limits +# bound the candidate lists (auto pool: 16 free, the remainder priced); probe +# spend per stage is bounded by REVIEW_PREFLIGHT_MAX_PROBES, not by them. +REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24 +REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT = 16 +REVIEW_PREFLIGHT_TARGET_READY = 8 +REVIEW_PREFLIGHT_MAX_PROBES = 16 # ADR-0005: a single fixed max_tokens cannot fit every model in a heterogeneous # pool -- some spend internal reasoning tokens before visible content and need # more, others have a real completion ceiling a large budget would exceed. The @@ -409,6 +419,13 @@ def _preflight_review_agents( response, both fields are absent entirely (there is no response to describe) rather than silently retaining the base attempt's values. + Candidates are probed lazily in catalog order (ADR-0029): probing stops + once ``REVIEW_PREFLIGHT_TARGET_READY`` routes are ready or + ``REVIEW_PREFLIGHT_MAX_PROBES`` probes have been spent, so a dead + candidate costs one probe rather than a served slot and a healthy pool is + not probed to exhaustion. Unprobed candidates get no ``routes`` row; + ``candidate_count`` minus ``probed_count`` counts them. + Args: agents: Selected zero-cost model agents. client: Vendored ``ModelClient``-compatible transport. @@ -428,6 +445,8 @@ def _preflight_review_agents( viable: list[object] = [] routes: list[dict[str, object]] = [] for agent in agents: + if len(viable) >= REVIEW_PREFLIGHT_TARGET_READY or len(routes) >= REVIEW_PREFLIGHT_MAX_PROBES: + break row: dict[str, object] = { "agent_id": str(getattr(agent, "id", "")), "provider": str(getattr(agent, "provider_name", "") or "unknown"), @@ -554,7 +573,9 @@ def _preflight_review_agents( # 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). + # row per *probed* agent in ``agents`` order (every branch above appends + # once; lazy fill stops before the unprobed tail), so ``zip`` pairs + # exactly the probed prefix. deferred: list[object] = [] if viable: for agent, row in zip(agents, routes): @@ -566,10 +587,13 @@ def _preflight_review_agents( deferred.append(_demote_agent(agent, REVIEW_PREFLIGHT_DEFERRED_PRIORITY_PENALTY)) report: dict[str, object] = { "contract": "strix-plain-chat-preflight-v2", - "probed_count": len(agents), + "candidate_count": len(agents), + "probed_count": len(routes), "ready_count": len(viable), "deferred_count": len(deferred), - "rejected_count": len(agents) - len(viable) - len(deferred), + "rejected_count": len(routes) - len(viable) - len(deferred), + "target_ready": REVIEW_PREFLIGHT_TARGET_READY, + "probe_budget": REVIEW_PREFLIGHT_MAX_PROBES, "escalations_used": escalations_used, "escalation_budget": REVIEW_PREFLIGHT_MAX_ESCALATIONS, "routes": routes, @@ -589,9 +613,10 @@ def _preflight_with_fallback( The two stages share ADR-0005's one ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` budget for the whole preflight run, not one budget each: the primary stage's ending ``escalations_used`` is passed as the fallback stage's - starting point, so a run that rejects all 8 primary routes and then - probes 4 fallback routes still spends at most 4 escalations total (12 - base attempts + 4 escalations). This bounds request count, not individual + starting point, so a run that rejects all 16 primary candidates and then + probes 8 fallback candidates still spends at most 4 escalations total (at + most ``REVIEW_PREFLIGHT_MAX_PROBES`` base attempts per stage + 4 + escalations). This bounds request count, not individual model response or sidecar readiness time. Both stages' reports remain in the result: the fallback (or sole) stage's report carries the run's final, cumulative ``escalations_used``, and diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index a96e854a51..f563abeea6 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -35,11 +35,12 @@ SIDECAR_LOG_SANITIZER="$ORG_REPO_ROOT/scripts/ci/sanitize_contextual_orchestrato # finishes, letting the shell script wait for a deterministic marker instead # of guessing whether the async sanitizer has caught up. SIDECAR_DISCOVERY_DIAGNOSTICS_SENTINEL="discovery_diagnostics_complete" -CATALOG_LIMIT="${ORCHESTRATOR_CATALOG_LIMIT:-12}" +CATALOG_LIMIT="${ORCHESTRATOR_CATALOG_LIMIT:-24}" # Each KV credential is an independent account, including two credentials for # the same vendor or endpoint. The account cap prevents one credential from -# consuming the bounded twelve-route preflight catalog without inventing a -# provider-family equivalence relation. +# consuming the bounded preflight candidate list (24 candidates, probed lazily +# to a readiness target -- ADR-0029) without inventing a provider-family +# equivalence relation. CATALOG_ACCOUNT_CAP="${ORCHESTRATOR_CATALOG_ACCOUNT_CAP:-8}" ORCHESTRATOR_GITHUB_ENV="${GITHUB_ENV:-}" sidecar_python="$(command -v python3)" diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 902237bdad..5846a7ec78 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1419,10 +1419,10 @@ def test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case 10s), blowing past Layer 1's 180s healthz-readiness watchdog and contradicting the ADR's own claimed 160s worst case. - This drives all 8 primary routes and all 4 fallback routes (the exact - ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`` split) through a response that + This drives all 16 primary candidates and all 8 fallback candidates (the + exact ``REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`` split) through a response that always qualifies for escalation and never resolves, so every one of the - 12 candidates *would* escalate if the budget were not shared. Asserts + 24 candidates *would* escalate if the budget were not shared. Asserts the run spends at most ``REVIEW_PREFLIGHT_MAX_ESCALATIONS`` escalations in total (not per stage), and that the resulting worst-case attempt count keeps total elapsed time at or under 160s -- both stages' escalation @@ -1458,8 +1458,10 @@ def test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case assert report["primary_attempt"]["escalations_used"] == max_escalations total_attempts = len(client.calls) - # Exactly the ADR's own worst-case arithmetic: 12 base attempts (one per - # candidate across both stages) + 4 escalations (the shared cap) = 16. + # Exactly the ADR's own worst-case arithmetic: 24 base attempts (one per + # candidate across both stages -- nothing is ready, so lazy fill never + # stops early, and each stage's list fits REVIEW_PREFLIGHT_MAX_PROBES) + + # 4 escalations (the shared cap) = 28. assert total_attempts == total_route_limit + max_escalations @@ -1472,8 +1474,13 @@ def test_preflight_stage_limits_share_one_startup_budget() -> None: fallback = namespace["_bounded_fallback_catalog_limit"]( 99, primary_count=primary ) - assert (primary, fallback) == (8, 4) + assert (primary, fallback) == (16, 8) assert primary + fallback == namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] + # Lazy fill (ADR-0029): each stage's candidate list fits its probe budget, + # so the worst case is still "every candidate probed once". + assert primary <= namespace["REVIEW_PREFLIGHT_MAX_PROBES"] + assert fallback <= namespace["REVIEW_PREFLIGHT_MAX_PROBES"] + assert namespace["REVIEW_PREFLIGHT_TARGET_READY"] < primary def test_catalog_account_cap_defaults_to_the_caller_supplied_policy_default( @@ -2064,3 +2071,95 @@ def test_sidecar_stream_sanitizer_passes_deferred_preflight_lines() -> None: 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") + + +def test_preflight_fills_lazily_and_stops_at_the_readiness_target() -> None: + """Probing stops once ``REVIEW_PREFLIGHT_TARGET_READY`` routes are ready (ADR-0029). + + Post-#1939 census (2026-09-06, .github#1948): the fixed 4+4+4 slice took + each NVIDIA key's first four models alphabetically, two of which answer + 404 on every run, so each key served two contended routes and noema went + from 7/14 to 0/22. A longer candidate list probed lazily lets a healthy + pool stop early and a dead candidate cost one probe instead of a slot. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + target = namespace["REVIEW_PREFLIGHT_TARGET_READY"] + agents = _preflight_agents(*(f"nvidia_{index}" for index in range(target + 4))) + client = _ProbeClient({agent.id: _openai_text("OK") for agent in agents}) + + served, report = preflight(agents, client=client) + + assert [agent.id for agent in served] == [agent.id for agent in agents[:target]] + assert len(client.calls) == target + assert (report["candidate_count"], report["probed_count"], report["ready_count"]) == ( + target + 4, + target, + target, + ) + assert (report["rejected_count"], report["deferred_count"]) == (0, 0) + assert (report["target_ready"], report["probe_budget"]) == ( + target, + namespace["REVIEW_PREFLIGHT_MAX_PROBES"], + ) + assert len(report["routes"]) == target + + +def test_preflight_dead_candidates_cost_a_probe_not_a_served_slot() -> None: + """Two 404s at the head of the list are probed past; the fill still reaches the target.""" + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + target = namespace["REVIEW_PREFLIGHT_TARGET_READY"] + dead = _preflight_agents("nvidia_gemma12", "nvidia_gemma4") + live = _preflight_agents(*(f"openrouter_{index}" for index in range(target + 2))) + outcomes: dict[str, object] = {agent.id: _StatusError(404) for agent in dead} + outcomes.update({agent.id: _openai_text("OK") for agent in live}) + + served, report = preflight([*dead, *live], client=_ProbeClient(outcomes)) + + assert [agent.id for agent in served] == [agent.id for agent in live[:target]] + assert report["probed_count"] == target + 2 + assert (report["ready_count"], report["rejected_count"], report["deferred_count"]) == (target, 2, 0) + assert [row["status"] for row in report["routes"][:2]] == ["rejected", "rejected"] + + +def test_preflight_probe_budget_bounds_a_dead_hour() -> None: + """With nothing ready, probing stops at ``REVIEW_PREFLIGHT_MAX_PROBES`` and the stage fails.""" + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + budget = namespace["REVIEW_PREFLIGHT_MAX_PROBES"] + agents = _preflight_agents(*(f"nvidia_{index}" for index in range(budget + 8))) + client = _ProbeClient({agent.id: _StatusError(429) for agent in agents}) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight(agents, client=client) + + report = failure.value.report + assert len(client.calls) == budget + assert (report["candidate_count"], report["probed_count"], report["ready_count"]) == ( + budget + 8, + budget, + 0, + ) + assert (report["rejected_count"], report["deferred_count"]) == (budget, 0) + + +def test_preflight_lazy_fill_keeps_deferral_for_probed_transient_routes() -> None: + """A 429 met on the way to the target is deferred; candidates past the stop get no row.""" + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + target = namespace["REVIEW_PREFLIGHT_TARGET_READY"] + agents = _preflight_agents("openrouter_a", *(f"nvidia_{index}" for index in range(target + 3))) + outcomes: dict[str, object] = {agent.id: _openai_text("OK") for agent in agents} + outcomes["openrouter_a"] = _StatusError(429) + + served, report = preflight(agents, client=_ProbeClient(outcomes)) + + assert [agent.id for agent in served] == [ + *(f"nvidia_{index}" for index in range(target)), + "openrouter_a", + ] + assert report["probed_count"] == target + 1 + assert (report["ready_count"], report["deferred_count"], report["rejected_count"]) == (target, 1, 0) + assert served[-1].priority == -namespace["REVIEW_PREFLIGHT_DEFERRED_PRIORITY_PENALTY"] + assert report["routes"][0]["status"] == "deferred" From b270bde14c54b77dda6ce0748f50965523076ca4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 11:26:30 +0900 Subject: [PATCH 2/4] fix(sidecar): cap every preflight stage's candidate list at the probe budget Review fixes from lane peer 1's head verification of #1949: - The production pool is `free` (sidecar default, no fallback stage), where _bounded_primary_catalog_limit returned the full 24 while the probe cap is 16, so candidates 17-24 could never be probed and candidate_count minus probed_count would have been meaningless as evidence. Every stage's list is now min(total, REVIEW_PREFLIGHT_MAX_PROBES): the free pool lists 16 candidates (12 before), the auto pool keeps 16 free + up to 8 priced. - The launcher's own ORCHESTRATOR_CATALOG_LIMIT default follows the sidecar (12 -> 24). - The sidecar's job-log echo of the preflight JSON grows from 160 to 400 lines: 16 probed routes at 8-10 lines each plus the header overran the old cap exactly in the dead hour; the artifact copy was always complete. - ADR-0029 and CHANGELOG state the free-pool bound and the wall-time cost of silent probes (one artifact: 19 probes, 805 s). Test: the stage-limit test now asserts the free pool's single stage equals the probe cap. Gate: 2913 passed, 1 skipped, coverage 100% (13150 statements, 0 missed), interrogate 100%. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 +- docs/adr/0029-sidecar-preflight-lazy-fill.md | 4 +++- ...contextual_orchestrator_review_launcher.py | 19 ++++++++++++++----- .../contextual_orchestrator_review_sidecar.sh | 5 ++++- ...l_orchestrator_review_runtime_preflight.py | 6 ++++++ 5 files changed, 28 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdb7632af3..dc55b94975 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ### Review sidecar preflight fills the served set lazily to a readiness target -- `_preflight_review_agents` now treats the catalog as a candidate list, probed in its tier-then-round-robin order until `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes are spent (ADR-0029). The candidate budget rises from 12 to 24 (`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`; auto pool split 16 free / 8 priced; the sidecar's `ORCHESTRATOR_CATALOG_LIMIT` default follows), the per-account cap stays 8. A permanently dead candidate -- NIM lists `gemma-3-12b`/`gemma-3-4b` and answers 404 on every run -- now costs one probe instead of a served slot, and a healthy pool stops early instead of always probing every candidate. Motivation: after #1939's four-per-account slice each NVIDIA key's slots were its first four models alphabetically, two of them those 404s, so preflight readiness fell from 6/12 to 1–3/12 and `noema-review` on this repository went from 7 successes / 14 failures to 0 / 22. The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_count` counts probes actually sent. ADR-0003's stage-budget sentence is amended. Refs #1939, #1947, #1948. +- `_preflight_review_agents` now treats the catalog as a candidate list, probed in its tier-then-round-robin order until `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes are spent (ADR-0029). The two-stage candidate budget rises from 12 to 24 (`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`; auto pool split 16 free / 8 priced; the sidecar's and the launcher's `ORCHESTRATOR_CATALOG_LIMIT` defaults follow), every stage's list is capped at the probe budget so the production `free` pool lists 16 candidates (12 before), and the per-account cap stays 8. The sidecar's job-log echo of the preflight JSON grows from 160 to 400 lines so 16 probed routes are not cut off exactly in the dead hour the summary matters. A permanently dead candidate -- NIM lists `gemma-3-12b`/`gemma-3-4b` and answers 404 on every run -- now costs one probe instead of a served slot, and a healthy pool stops early instead of always probing every candidate. Motivation: after #1939's four-per-account slice each NVIDIA key's slots were its first four models alphabetically, two of them those 404s, so preflight readiness fell from 6/12 to 1–3/12 and `noema-review` on this repository went from 7 successes / 14 failures to 0 / 22. The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_count` counts probes actually sent. ADR-0003's stage-budget sentence is amended. Refs #1939, #1947, #1948. ### Review sidecar preflight keeps transient-rejected routes as deferred failover diff --git a/docs/adr/0029-sidecar-preflight-lazy-fill.md b/docs/adr/0029-sidecar-preflight-lazy-fill.md index e5abca67e0..b0f46fce1a 100644 --- a/docs/adr/0029-sidecar-preflight-lazy-fill.md +++ b/docs/adr/0029-sidecar-preflight-lazy-fill.md @@ -28,7 +28,9 @@ The evening's rate-limit pressure is a confound; the mechanism is not. A fixed s ## Decision -The catalog is a **candidate list**, not the served set. `build_zdr_prioritized_catalog` keeps its tier-then-round-robin order (`#1939`) and is asked for up to `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24` candidates (per-account cap unchanged at 8; the sidecar's `ORCHESTRATOR_CATALOG_LIMIT` default rises from 12 to 24). `_preflight_review_agents` probes candidates **in that order and stops** as soon as `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes have been spent, whichever comes first. The auto pool's split becomes 16 free candidates and up to 8 priced fallback candidates; each stage's list fits the probe cap, so the worst case remains "every candidate probed once". +The catalog is a **candidate list**, not the served set. `build_zdr_prioritized_catalog` keeps its tier-then-round-robin order (`#1939`) and is asked for up to `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24` candidates (per-account cap unchanged at 8; the sidecar's `ORCHESTRATOR_CATALOG_LIMIT` default rises from 12 to 24). `_preflight_review_agents` probes candidates **in that order and stops** as soon as `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes have been spent, whichever comes first. The auto pool's split becomes 16 free candidates and up to 8 priced fallback candidates. Every stage's list is additionally capped at `REVIEW_PREFLIGHT_MAX_PROBES`, so the production `free` pool (the sidecar default; it has no fallback stage) lists 16 candidates, not 24 with an unreachable tail, and the worst case in every pool remains "every candidate probed once". A silent candidate's probe costs up to one transport timeout (one artifact spent 805 s on 19 probes), so the probe cap bounds preflight wall time as well as request count. + +The sidecar's job-log echo of the preflight JSON (`sed -n '1,400p'`, previously 160 lines) now fits 16 probed routes; the artifact copy was always complete. The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_count` now counts probes actually sent, and `rejected_count` is `probed − ready − deferred`. Unprobed candidates get no `routes` row. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index cb95df8b74..3809a809fd 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -50,9 +50,13 @@ # ready or REVIEW_PREFLIGHT_MAX_PROBES probes are spent, whichever comes first. # A permanently dead candidate (NIM lists gemma-3-12b/4b but answers 404 on # every run) then costs one probe instead of a served slot, and a healthy hour -# stops early instead of always probing every candidate. The two stage limits -# bound the candidate lists (auto pool: 16 free, the remainder priced); probe -# spend per stage is bounded by REVIEW_PREFLIGHT_MAX_PROBES, not by them. +# stops early instead of always probing every candidate. MAX_TOTAL_ROUTES is +# the two-stage total (auto pool: 16 free, up to 8 priced); every stage's list +# is additionally capped at MAX_PROBES, so the production ``free`` pool lists +# 16 candidates and no candidate is ever listed that cannot be probed. A +# silent candidate's probe costs up to one transport timeout (19 probes took +# 805 s in one artifact), so MAX_PROBES bounds preflight wall time as well as +# request count. REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24 REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT = 16 REVIEW_PREFLIGHT_TARGET_READY = 8 @@ -712,7 +716,12 @@ def _bounded_primary_catalog_limit( total_limit = min(requested_limit, REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES) if pool == "auto" and has_free_rows: return min(total_limit, REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT) - return total_limit + # ADR-0029: a stage never lists more candidates than it may probe. The + # production pool is ``free`` (no fallback stage), so without this bound + # it would list 24 candidates of which the last eight could never be + # reached under REVIEW_PREFLIGHT_MAX_PROBES -- an unreachable tail that + # would also make candidate_count - probed_count meaningless as evidence. + return min(total_limit, REVIEW_PREFLIGHT_MAX_PROBES) def _bounded_fallback_catalog_limit( @@ -998,7 +1007,7 @@ def main(argv: list[str] | None = None) -> int: zdr_endpoints=zdr_endpoints, checker=is_zdr_model, ) - requested_catalog_limit = int(os.environ.get("ORCHESTRATOR_CATALOG_LIMIT", "12")) + requested_catalog_limit = int(os.environ.get("ORCHESTRATOR_CATALOG_LIMIT", "24")) primary_limit = _bounded_primary_catalog_limit( requested_catalog_limit, pool=args.pool, has_free_rows=bool(admitted_free_rows) ) diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index f563abeea6..5e87840c67 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -689,4 +689,7 @@ fi log "policy evidence summary:" sed -n '1,80p' "$policy_report" || true log "runtime preflight summary:" -sed -n '1,160p' "$preflight_report" || true +# 16 probed routes at 8-10 lines each plus the header run past the old +# 160-line cap exactly in the dead hour the summary matters most (ADR-0029); +# the artifact copy was always complete, only the job-log echo was cut. +sed -n '1,400p' "$preflight_report" || true diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 5846a7ec78..0c4119971d 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1481,6 +1481,12 @@ def test_preflight_stage_limits_share_one_startup_budget() -> None: assert primary <= namespace["REVIEW_PREFLIGHT_MAX_PROBES"] assert fallback <= namespace["REVIEW_PREFLIGHT_MAX_PROBES"] assert namespace["REVIEW_PREFLIGHT_TARGET_READY"] < primary + # The production pool is ``free`` (sidecar default; no fallback stage): + # its single stage must also fit the probe cap, or the last candidates + # could never be probed and candidate_count would overstate the list. + free_pool = namespace["_bounded_primary_catalog_limit"](99, pool="free", has_free_rows=True) + assert free_pool == namespace["REVIEW_PREFLIGHT_MAX_PROBES"] == 16 + assert namespace["_bounded_fallback_catalog_limit"](99, primary_count=free_pool) == 8 def test_catalog_account_cap_defaults_to_the_caller_supplied_policy_default( From 82ed98d7896ae82b4dd20c2504324d16a781dc0c Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 12:46:29 +0900 Subject: [PATCH 3/4] fix(sidecar): skip an account after two consecutive 429s at preflight Lane jan rebuilt the real 2026-09-06 candidate order from #1938's Strix artifact (comment on #1949): under the plain sixteen-probe walk the served set is about five ready + five deferred and the readiness target of eight is unreachable, because the tier round-robin spends five probes on an account whose every free route has answered 429 in every artifact since 21:00Z and four on the two dead gemma-3 entries per key, while the routes that were ready in every pre-#1939 artifact (llama-3.2-11b/90b, muse-glimmer-30b) sit past the cap. A 429 at preflight is a per-key answer. Once one credential account has answered 429 to REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429 = 2 consecutive probes, its remaining candidates are skipped without a probe and the walk continues with the other accounts' next candidates; the two probed routes are still deferred (#1947). Under jan's order the same sixteen probes now reach both keys' llama routes and the target; a fully rate-limited hour costs two probes per account instead of the whole budget. - The production free pool lists the full 24 candidates again (probe cap 16): the tail past the cap is reachable exactly when an account is skipped, and the report separates skipped_count from the unreached remainder, which answers the earlier unreachable-tail objection. - The deferral pass pairs rows with the agents actually probed (a new `probed` list), not positionally with `agents` -- with skips those no longer line up; the artifact-order test alone missed this because its skips fell after its deferred rows, so a dedicated ordering test pins it. - Report gains skipped_count and account_skip_after_429. ADR-0029 and the CHANGELOG record the rule and the projection; jan's family-interleave layer is recorded as the reserve alternative. Tests: dead-hour budget test now uses 404s (24 -> 16 probes, no skips); three accounts x 8 all 429 -> 6 probes, 18 skipped; jan's order -> a llama route on each key, ready 8 within 16 probes, deferred = the two probed OpenRouter routes, skipped >= 3; row/agent pairing after skips. Negative control: the new tests fail on this PR's previous head; the pairing test fails on the pre-fix walk. Gate: 2916 passed, 1 skipped, coverage 100% (0 missed), interrogate 100%. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 2 +- docs/adr/0029-sidecar-preflight-lazy-fill.md | 6 +- ...contextual_orchestrator_review_launcher.py | 72 ++++++-- ...l_orchestrator_review_runtime_preflight.py | 166 ++++++++++++++++-- 4 files changed, 214 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5e7a09394..427c0d80ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Review sidecar preflight fills the served set lazily to a readiness target -- `_preflight_review_agents` now treats the catalog as a candidate list, probed in its tier-then-round-robin order until `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes are spent (ADR-0029). The two-stage candidate budget rises from 12 to 24 (`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`; auto pool split 16 free / 8 priced; the sidecar's and the launcher's `ORCHESTRATOR_CATALOG_LIMIT` defaults follow), every stage's list is capped at the probe budget so the production `free` pool lists 16 candidates (12 before), and the per-account cap stays 8. The sidecar's job-log echo of the preflight JSON grows from 160 to 400 lines so 16 probed routes are not cut off exactly in the dead hour the summary matters. A permanently dead candidate -- NIM lists `gemma-3-12b`/`gemma-3-4b` and answers 404 on every run -- now costs one probe instead of a served slot, and a healthy pool stops early instead of always probing every candidate. Motivation: after #1939's four-per-account slice each NVIDIA key's slots were its first four models alphabetically, two of them those 404s, so preflight readiness fell from 6/12 to 1–3/12 and `noema-review` on this repository went from 7 successes / 14 failures to 0 / 22. The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_count` counts probes actually sent. ADR-0003's stage-budget sentence is amended. Refs #1939, #1947, #1948. +- `_preflight_review_agents` now treats the catalog as a candidate list, probed in its tier-then-round-robin order until `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes are spent (ADR-0029). The two-stage candidate budget rises from 12 to 24 (`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`; auto pool split 16 free / 8 priced; the sidecar's and the launcher's `ORCHESTRATOR_CATALOG_LIMIT` defaults follow), the production `free` pool lists all 24 (12 before), and the per-account cap stays 8. An account that answers 429 to `REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429 = 2` consecutive probes has its remaining candidates skipped without a probe (a 429 is a per-key answer), so the probes it would have spent reach the other accounts' next candidates — under the real 2026-09-06 order that is the difference between about five ready routes and the target of eight — and a fully rate-limited hour costs two probes per account instead of the whole budget; the report gains `skipped_count` and `account_skip_after_429`. The sidecar's job-log echo of the preflight JSON grows from 160 to 400 lines so 16 probed routes are not cut off exactly in the dead hour the summary matters. A permanently dead candidate -- NIM lists `gemma-3-12b`/`gemma-3-4b` and answers 404 on every run -- now costs one probe instead of a served slot, and a healthy pool stops early instead of always probing every candidate. Motivation: after #1939's four-per-account slice each NVIDIA key's slots were its first four models alphabetically, two of them those 404s, so preflight readiness fell from 6/12 to 1–3/12 and `noema-review` on this repository went from 7 successes / 14 failures to 0 / 22. The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_count` counts probes actually sent. ADR-0003's stage-budget sentence is amended. Refs #1939, #1947, #1948. ### Review sidecar preflight keeps transient-rejected routes as deferred failover diff --git a/docs/adr/0029-sidecar-preflight-lazy-fill.md b/docs/adr/0029-sidecar-preflight-lazy-fill.md index b0f46fce1a..141eba1b1a 100644 --- a/docs/adr/0029-sidecar-preflight-lazy-fill.md +++ b/docs/adr/0029-sidecar-preflight-lazy-fill.md @@ -28,7 +28,9 @@ The evening's rate-limit pressure is a confound; the mechanism is not. A fixed s ## Decision -The catalog is a **candidate list**, not the served set. `build_zdr_prioritized_catalog` keeps its tier-then-round-robin order (`#1939`) and is asked for up to `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24` candidates (per-account cap unchanged at 8; the sidecar's `ORCHESTRATOR_CATALOG_LIMIT` default rises from 12 to 24). `_preflight_review_agents` probes candidates **in that order and stops** as soon as `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes have been spent, whichever comes first. The auto pool's split becomes 16 free candidates and up to 8 priced fallback candidates. Every stage's list is additionally capped at `REVIEW_PREFLIGHT_MAX_PROBES`, so the production `free` pool (the sidecar default; it has no fallback stage) lists 16 candidates, not 24 with an unreachable tail, and the worst case in every pool remains "every candidate probed once". A silent candidate's probe costs up to one transport timeout (one artifact spent 805 s on 19 probes), so the probe cap bounds preflight wall time as well as request count. +The catalog is a **candidate list**, not the served set. `build_zdr_prioritized_catalog` keeps its tier-then-round-robin order (`#1939`) and is asked for up to `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24` candidates (per-account cap unchanged at 8; the sidecar's `ORCHESTRATOR_CATALOG_LIMIT` default rises from 12 to 24). `_preflight_review_agents` probes candidates **in that order and stops** as soon as `REVIEW_PREFLIGHT_TARGET_READY = 8` routes are ready or `REVIEW_PREFLIGHT_MAX_PROBES = 16` probes have been spent, whichever comes first. The auto pool's split becomes 16 free candidates and up to 8 priced fallback candidates; the production `free` pool (the sidecar default; it has no fallback stage) lists all 24. A silent candidate's probe costs up to one transport timeout (one artifact spent 805 s on 19 probes), so the probe cap bounds preflight wall time as well as request count. + +**Account skip.** A 429 at preflight is a per-key answer, not a per-model one. Once one credential account has answered 429 to `REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429 = 2` consecutive probes, its remaining candidates are skipped without a probe and the walk continues with the other accounts' next candidates; the two probed routes are still deferred. Under the real 2026-09-06 candidate order (lane jan's table on `#1949`, rebuilt from `#1938`'s Strix artifact: both NVIDIA keys list deepseek ×2, gemma-3 ×2 (404), gemma-4-31b (empty), then the llama and muse routes; every OpenRouter free route answers 429) the plain sixteen-probe walk yields about five ready and five deferred and the readiness target is unreachable, because five probes go to an account whose every route had answered 429 in every artifact since 21:00Z and four to the dead gemma-3 entries. With the skip, the same sixteen probes reach both keys' `llama-3.2` routes and the target of eight. This is why the free pool lists 24 candidates while probing at most 16: the tail is reachable exactly when an account is skipped, and the report separates `skipped_count` from the unreached remainder (`candidate_count − probed_count − skipped_count`). A rate-limited hour therefore costs two probes per account instead of the full budget. The sidecar's job-log echo of the preflight JSON (`sed -n '1,400p'`, previously 160 lines) now fits 16 probed routes; the artifact copy was always complete. @@ -45,5 +47,5 @@ The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_c - **Raise the per-account cap back to 8 with a 12-route limit** — restores the pre-#1939 pool but reintroduces the single-account fill that `#1939` fixed; the 404s would still occupy slots. - **Exclude models that 404 by name** — a hard-coded exclusion list the next discovery change silently invalidates; rejected by constraint 1. The discovery-side question (why NIM lists models it does not serve) remains open in `contextual-orchestrator`. -- **Family-level round-robin before the per-account cap** (jan's fallback proposal) — reduces same-family contention but does not touch dead candidates; can be layered later if the census shows family contention as the residual. +- **Family-level interleave inside each account's list before the cap** (jan's second layer) — would make each NVIDIA key's first six candidates span deepseek, gemma, llama, muse, minimax, mistral, but it needs a model-family equivalence derived from names, which ADR-0003/#1468 deliberately avoid; kept in reserve if the post-merge census shows same-family contention as the residual after the account skip. - **Probe all 24 candidates** — best served set, double the probe spend in the hour that can least afford it; rejected by constraint 2. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index 3809a809fd..39c43e24e1 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -32,7 +32,10 @@ from pathlib import Path from typing import Any, Callable -from scripts.ci.contextual_orchestrator_review_policy import FREE_POOL_CREDENTIAL_NAMES +from scripts.ci.contextual_orchestrator_review_policy import ( + FREE_POOL_CREDENTIAL_NAMES, + provider_account, +) # The vendored server's generic 64 KiB default is intentionally conservative. @@ -51,16 +54,28 @@ # A permanently dead candidate (NIM lists gemma-3-12b/4b but answers 404 on # every run) then costs one probe instead of a served slot, and a healthy hour # stops early instead of always probing every candidate. MAX_TOTAL_ROUTES is -# the two-stage total (auto pool: 16 free, up to 8 priced); every stage's list -# is additionally capped at MAX_PROBES, so the production ``free`` pool lists -# 16 candidates and no candidate is ever listed that cannot be probed. A -# silent candidate's probe costs up to one transport timeout (19 probes took -# 805 s in one artifact), so MAX_PROBES bounds preflight wall time as well as -# request count. +# the two-stage total (auto pool: 16 free, up to 8 priced; the production +# ``free`` pool lists all 24). A silent candidate's probe costs up to one +# transport timeout (19 probes took 805 s in one artifact), so MAX_PROBES +# bounds preflight wall time as well as request count. Candidates past the +# probe cap are reached only through the account-skip rule below, and the +# report separates ``skipped_count`` from the unreached tail so the evidence +# stays readable. REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 24 REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT = 16 REVIEW_PREFLIGHT_TARGET_READY = 8 REVIEW_PREFLIGHT_MAX_PROBES = 16 +# A 429 at preflight is a per-key answer, not a per-model one: once one +# credential account has answered 429 to this many probes in a row, its +# remaining candidates are skipped without a probe and the walk moves on to +# the other accounts' next candidates. Under the real 2026-09-06 candidate +# order (jan's table on #1949) the round-robin would otherwise spend five of +# sixteen probes on an account whose every free route had answered 429 in +# every artifact since 21:00Z, and the readiness target was unreachable; with +# the skip the same sixteen probes reach both keys' llama routes. The two +# probed routes are still deferred (#1947); a skipped candidate is neither +# probed nor served. +REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429 = 2 # ADR-0005: a single fixed max_tokens cannot fit every model in a heterogeneous # pool -- some spend internal reasoning tokens before visible content and need # more, others have a real completion ceiling a large budget would exceed. The @@ -427,8 +442,12 @@ def _preflight_review_agents( once ``REVIEW_PREFLIGHT_TARGET_READY`` routes are ready or ``REVIEW_PREFLIGHT_MAX_PROBES`` probes have been spent, so a dead candidate costs one probe rather than a served slot and a healthy pool is - not probed to exhaustion. Unprobed candidates get no ``routes`` row; - ``candidate_count`` minus ``probed_count`` counts them. + not probed to exhaustion. An account that has answered 429 to + ``REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429`` consecutive probes has its + remaining candidates skipped without a probe (a 429 is a per-key answer). + Unprobed candidates get no ``routes`` row; ``skipped_count`` counts the + skipped ones and ``candidate_count - probed_count - skipped_count`` the + unreached tail. Args: agents: Selected zero-cost model agents. @@ -448,9 +467,22 @@ def _preflight_review_agents( """ viable: list[object] = [] routes: list[dict[str, object]] = [] + consecutive_429: dict[str, int] = {} + skipped = 0 + # One entry per probe, in probe order: ``routes[i]`` describes + # ``probed[i]``. Skipped candidates appear in neither, so the deferral pass + # below must pair rows with this list, not with ``agents``. + probed: list[object] = [] for agent in agents: if len(viable) >= REVIEW_PREFLIGHT_TARGET_READY or len(routes) >= REVIEW_PREFLIGHT_MAX_PROBES: break + account = provider_account(str(getattr(agent, "provider_name", "") or "unknown")) + if consecutive_429.get(account, 0) >= REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429: + skipped += 1 + continue + # Cleared here; only a 429 answer below restores it, incremented. + streak_429 = consecutive_429.pop(account, 0) + probed.append(agent) row: dict[str, object] = { "agent_id": str(getattr(agent, "id", "")), "provider": str(getattr(agent, "provider_name", "") or "unknown"), @@ -471,6 +503,8 @@ def _preflight_review_agents( response = client.proxy_send_once(agent, "chat/completions", base_payload) except Exception as exc: # noqa: BLE001 - sanitize at the provider boundary _record_provider_exception(row, exc) + if row.get("http_status") == 429: + consecutive_429[account] = streak_429 + 1 routes.append(row) continue if _chat_response_has_text(response): @@ -577,12 +611,11 @@ def _preflight_review_agents( # 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 *probed* agent in ``agents`` order (every branch above appends - # once; lazy fill stops before the unprobed tail), so ``zip`` pairs - # exactly the probed prefix. + # row per *probed* agent in probe order (every branch above appends once), + # and ``probed`` the matching agents -- skipped candidates are in neither. deferred: list[object] = [] if viable: - for agent, row in zip(agents, routes): + for agent, row in zip(probed, routes): if ( row.get("status") == "rejected" and row.get("http_status") in REVIEW_PREFLIGHT_DEFERRABLE_HTTP_STATUS @@ -596,8 +629,10 @@ def _preflight_review_agents( "ready_count": len(viable), "deferred_count": len(deferred), "rejected_count": len(routes) - len(viable) - len(deferred), + "skipped_count": skipped, "target_ready": REVIEW_PREFLIGHT_TARGET_READY, "probe_budget": REVIEW_PREFLIGHT_MAX_PROBES, + "account_skip_after_429": REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429, "escalations_used": escalations_used, "escalation_budget": REVIEW_PREFLIGHT_MAX_ESCALATIONS, "routes": routes, @@ -716,12 +751,11 @@ def _bounded_primary_catalog_limit( total_limit = min(requested_limit, REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES) if pool == "auto" and has_free_rows: return min(total_limit, REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT) - # ADR-0029: a stage never lists more candidates than it may probe. The - # production pool is ``free`` (no fallback stage), so without this bound - # it would list 24 candidates of which the last eight could never be - # reached under REVIEW_PREFLIGHT_MAX_PROBES -- an unreachable tail that - # would also make candidate_count - probed_count meaningless as evidence. - return min(total_limit, REVIEW_PREFLIGHT_MAX_PROBES) + # ADR-0029: the production pool is ``free`` (no fallback stage) and lists + # the full two-stage budget. Candidates past REVIEW_PREFLIGHT_MAX_PROBES + # are reached only when the account-skip rule frees probes; the report's + # ``skipped_count`` keeps that tail distinguishable from an early stop. + return total_limit def _bounded_fallback_catalog_limit( diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 0c4119971d..96d27bcd82 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -1476,17 +1476,18 @@ def test_preflight_stage_limits_share_one_startup_budget() -> None: ) assert (primary, fallback) == (16, 8) assert primary + fallback == namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] - # Lazy fill (ADR-0029): each stage's candidate list fits its probe budget, - # so the worst case is still "every candidate probed once". + # Lazy fill (ADR-0029): the auto stages each fit the probe budget, so + # their worst case is still "every candidate probed once". assert primary <= namespace["REVIEW_PREFLIGHT_MAX_PROBES"] assert fallback <= namespace["REVIEW_PREFLIGHT_MAX_PROBES"] assert namespace["REVIEW_PREFLIGHT_TARGET_READY"] < primary - # The production pool is ``free`` (sidecar default; no fallback stage): - # its single stage must also fit the probe cap, or the last candidates - # could never be probed and candidate_count would overstate the list. + # The production pool is ``free`` (sidecar default; no fallback stage) and + # lists the whole budget: candidates past the probe cap are reachable only + # through the account-skip rule, and the report says how many were skipped. free_pool = namespace["_bounded_primary_catalog_limit"](99, pool="free", has_free_rows=True) - assert free_pool == namespace["REVIEW_PREFLIGHT_MAX_PROBES"] == 16 - assert namespace["_bounded_fallback_catalog_limit"](99, primary_count=free_pool) == 8 + assert free_pool == namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] == 24 + assert free_pool > namespace["REVIEW_PREFLIGHT_MAX_PROBES"] + assert namespace["_bounded_fallback_catalog_limit"](99, primary_count=free_pool) == 0 def test_catalog_account_cap_defaults_to_the_caller_supplied_policy_default( @@ -2130,12 +2131,12 @@ def test_preflight_dead_candidates_cost_a_probe_not_a_served_slot() -> None: def test_preflight_probe_budget_bounds_a_dead_hour() -> None: - """With nothing ready, probing stops at ``REVIEW_PREFLIGHT_MAX_PROBES`` and the stage fails.""" + """With nothing ready and no 429, probing stops at ``REVIEW_PREFLIGHT_MAX_PROBES`` and the stage fails.""" namespace = _load_launcher() preflight = namespace["_preflight_review_agents"] budget = namespace["REVIEW_PREFLIGHT_MAX_PROBES"] agents = _preflight_agents(*(f"nvidia_{index}" for index in range(budget + 8))) - client = _ProbeClient({agent.id: _StatusError(429) for agent in agents}) + client = _ProbeClient({agent.id: _StatusError(404) for agent in agents}) with pytest.raises(namespace["ReviewPreflightError"]) as failure: preflight(agents, client=client) @@ -2147,7 +2148,152 @@ def test_preflight_probe_budget_bounds_a_dead_hour() -> None: budget, 0, ) - assert (report["rejected_count"], report["deferred_count"]) == (budget, 0) + assert (report["rejected_count"], report["deferred_count"], report["skipped_count"]) == (budget, 0, 0) + + +def test_preflight_skips_an_account_after_consecutive_429s() -> None: + """A rate-limited hour costs two probes per account, not the whole budget. + + A 429 at preflight is a per-key answer. Once one credential account has + answered 429 to REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429 probes in a row, its + remaining candidates are skipped without a probe. With every account + rate-limited the walk ends after two probes per account and the stage + fails as before (no route is ready, so nothing is deferred either). + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + skip_after = namespace["REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429"] + accounts = ("nvidia_nim", "nvidia_nim_sub", "openrouter") + agents = [ + SimpleNamespace(id=f"{account}_{index}", provider_name=account, model=f"{account}/m{index}", priority=-index) + for index in range(8) + for account in accounts + ] + client = _ProbeClient({agent.id: _StatusError(429) for agent in agents}) + + with pytest.raises(namespace["ReviewPreflightError"]) as failure: + preflight(agents, client=client) + + report = failure.value.report + assert len(client.calls) == skip_after * len(accounts) == 6 + assert report["probed_count"] == 6 + assert report["skipped_count"] == len(agents) - 6 + assert report["account_skip_after_429"] == skip_after + assert [call[0].id for call in client.calls] == [ + agent.id for agent in agents[: skip_after * len(accounts)] + ] + + +def _artifact_order_candidates() -> tuple[list[SimpleNamespace], dict[str, object]]: + """Rebuild the 2026-09-06 candidate order and probe answers from jan's #1949 table. + + Two NVIDIA keys list the same models alphabetically -- two deepseek routes, + the two gemma-3 entries that answer 404 on every run, a gemma-4 entry that + answers an empty completion, then the llama and muse routes that were ready + in every pre-#1939 artifact -- and every OpenRouter free route answers 429. + The catalog interleaves the three accounts tier-round-robin, eight each. + """ + nvidia_models = [ + "deepseek-v4-flash", + "deepseek-v4-pro", + "gemma-3-12b", + "gemma-3-4b", + "gemma-4-31b", + "llama-3.2-11b", + "llama-3.2-90b", + "muse-glimmer-30b", + ] + openrouter_models = [f"free-{index}" for index in range(8)] + per_account = { + "nvidia_nim": nvidia_models, + "nvidia_nim_sub": nvidia_models, + "openrouter": openrouter_models, + } + agents: list[SimpleNamespace] = [] + for index in range(8): + for account, models in per_account.items(): + agents.append( + SimpleNamespace( + id=f"{account}_{models[index]}", + provider_name=account, + model=f"{account}/{models[index]}", + priority=-len(agents), + ) + ) + outcomes: dict[str, object] = {} + for agent in agents: + model = agent.model.split("/", 1)[1] + if agent.provider_name == "openrouter": + outcomes[agent.id] = _StatusError(429) + elif model.startswith("gemma-3"): + outcomes[agent.id] = _StatusError(404) + elif model.startswith("gemma-4"): + outcomes[agent.id] = {"choices": [{"finish_reason": "stop", "message": {"content": ""}}]} + else: + outcomes[agent.id] = _openai_text("OK") + return agents, outcomes + + +def test_preflight_reaches_both_keys_llama_routes_under_the_artifact_order() -> None: + """Under the real candidate order the sixteen probes reach a non-deepseek route on each key. + + Without the account skip the round-robin spends five probes on OpenRouter's + 429s and the target of eight is unreachable (about five ready + five + deferred, jan's table on #1949); with it the same budget reaches both + keys' llama routes and the target. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + agents, outcomes = _artifact_order_candidates() + client = _ProbeClient(outcomes) + + served, report = preflight(agents, client=client) + + served_ids = [agent.id for agent in served] + for key in ("nvidia_nim", "nvidia_nim_sub"): + assert any(agent_id.startswith(f"{key}_llama") for agent_id in served_ids), served_ids + assert report["ready_count"] == namespace["REVIEW_PREFLIGHT_TARGET_READY"] + assert report["probed_count"] <= namespace["REVIEW_PREFLIGHT_MAX_PROBES"] + assert report["deferred_count"] == namespace["REVIEW_PREFLIGHT_ACCOUNT_SKIP_AFTER_429"] + assert report["skipped_count"] >= 3 + assert len(client.calls) == report["probed_count"] + # The deferred agents are the two OpenRouter routes that were actually + # probed, not whichever agents happen to share their index once skips + # have shifted the row list. + assert served_ids[report["ready_count"] :] == ["openrouter_free-0", "openrouter_free-1"] + + +def test_preflight_deferral_pairs_rows_with_probed_agents_after_skips() -> None: + """After an account is skipped, deferred rows still map to the agents that were probed. + + Order: X answers 429 twice (then is skipped), Y is ready, Z answers 429 + once after X's skips began. Pairing rows with the original agent list + would demote the skipped X candidates instead of Z. + """ + namespace = _load_launcher() + preflight = namespace["_preflight_review_agents"] + + def agent(account: str, index: int) -> SimpleNamespace: + return SimpleNamespace(id=f"{account}{index}", provider_name=account, model=f"{account}/m{index}", priority=0) + + agents = [ + agent("X", 1), agent("X", 2), agent("Y", 1), agent("X", 3), agent("Z", 1), + agent("Y", 2), agent("X", 4), agent("Z", 2), agent("X", 5), agent("Y", 3), + ] + outcomes: dict[str, object] = { + "X1": _StatusError(429), "X2": _StatusError(429), "X3": _StatusError(429), + "X4": _StatusError(429), "X5": _StatusError(429), "Z1": _StatusError(429), + "Y1": _openai_text("OK"), "Y2": _openai_text("OK"), "Y3": _openai_text("OK"), + "Z2": _openai_text("OK"), + } + client = _ProbeClient(outcomes) + + served, report = preflight(agents, client=client) + + assert [call[0].id for call in client.calls] == ["X1", "X2", "Y1", "Z1", "Y2", "Z2", "Y3"] + assert (report["ready_count"], report["deferred_count"], report["skipped_count"]) == (4, 3, 3) + assert [a.id for a in served] == ["Y1", "Y2", "Z2", "Y3", "X1", "X2", "Z1"] + assert all(a.priority == -namespace["REVIEW_PREFLIGHT_DEFERRED_PRIORITY_PENALTY"] for a in served[4:]) def test_preflight_lazy_fill_keeps_deferral_for_probed_transient_routes() -> None: From 3a1e10b85d5c87904e8cc7dc0be0208f627ca15f Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 13:55:51 +0900 Subject: [PATCH 4/4] docs(adr-0029): state the probe cap's wall-time bound with the first post-pin measurement .github#1661 run 34008191123 (04:48Z, sidecar pin 414f2297): both NVIDIA keys' deepseek-v4-pro preflight probes held for 90.06 s and 90.10 s -- a 16-token probe can spend the full receive timeout -- and the hour had zero ready routes of twelve. So the probe cap is a wall-time bound as well: a fully silent hour costs at most 16 x 90 s = 24 minutes of preflight against 18 today, and the account-skip rule cuts a rate-limited hour to two probes per account. Docs only; no code change. Co-Authored-By: Claude Fable 5.1 --- docs/adr/0029-sidecar-preflight-lazy-fill.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0029-sidecar-preflight-lazy-fill.md b/docs/adr/0029-sidecar-preflight-lazy-fill.md index 141eba1b1a..d2528b9883 100644 --- a/docs/adr/0029-sidecar-preflight-lazy-fill.md +++ b/docs/adr/0029-sidecar-preflight-lazy-fill.md @@ -39,7 +39,7 @@ The report gains `candidate_count`, `target_ready` and `probe_budget`; `probed_c ## Consequences - **Good:** a dead candidate costs one probe and yields its place to the next candidate in the same account's list; a healthy hour stops after about eight to twelve probes instead of always twelve; a bad hour is bounded at sixteen probes per stage. -- **Cost:** in an hour where nothing is ready the sidecar sends up to 16 probes per stage where it sent 12, a third more against already exhausted keys. This is the price of finding routes past the dead ones; `#1948`'s shared rate ledger is the lever above it. +- **Cost:** in an hour where nothing is ready the sidecar sends up to 16 probes per stage where it sent 12, a third more against already exhausted keys. This is the price of finding routes past the dead ones; `#1948`'s shared rate ledger is the lever above it. The cap is also a wall-time bound: a 16-token probe can hold the full 90 s receive timeout (`#1661` run 34008191123, 04:48Z, both NVIDIA keys' deepseek-v4-pro probes at 90.06 s and 90.10 s), so a fully silent hour costs at most 16 × 90 s = 24 minutes of preflight against 18 today, and the account-skip rule cuts a rate-limited hour to two probes per account. - **Unchanged:** a route that answers the probe and then goes silent at request time still costs the gateway's full retry budget (`contextual-orchestrator#1045`); readiness is measured at 16 tokens (`#1454`). - **Discriminator:** post-merge, `probed_count` versus `candidate_count` per boot and `ready_count` of the served set, read from the `runtime preflight summary` in the job log or the `noema-sidecar-evidence` artifact, compared with the table above.