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 683beee96f..efd2771e96 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 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. + ### Sidecar sanitizer keeps the exception type and innermost frame per traceback - `scripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py` now reduces each Python traceback in the sidecar stream to one line, `unexpected_exception type= frame=contextual_orchestrator/.py::` (the type identifier and the innermost package frame only; the exception message, source echoes and non-package frames are never re-emitted; a traceback cut off by the sidecar dying or without a package frame reports `unknown`). The previous single, once-per-stream `sidecar emitted an unexpected exception` line kept neither the count nor the type: `.github#1812`'s strix run (33993155419) ended on 83 gateway `500 internal_error` responses -- the orchestrator's generic request handler prints one traceback per unhandled exception -- and no artifact could say which exception escaped or where. Chain sentences (`During handling of the above exception…`, `The above exception was the direct cause…`) are consumed, so a chained exception yields cause then effect. diff --git a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md index 8905005d95..9b0749f258 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..d2528b9883 --- /dev/null +++ b/docs/adr/0029-sidecar-preflight-lazy-fill.md @@ -0,0 +1,51 @@ +# 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; 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. + +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. 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. + +## 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 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 27917eba4e..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. @@ -45,8 +48,34 @@ # 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. MAX_TOTAL_ROUTES is +# 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 @@ -409,6 +438,17 @@ 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. 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. client: Vendored ``ModelClient``-compatible transport. @@ -427,7 +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"), @@ -448,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): @@ -554,10 +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 agent in ``agents`` order (every branch above appends once). + # 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 @@ -566,10 +624,15 @@ 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), + "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, @@ -589,9 +652,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 @@ -687,6 +751,10 @@ 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: 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 @@ -973,7 +1041,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 250c783446..38d9551a32 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)" @@ -688,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 69e62153ff..0944c7ed2f 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,20 @@ 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): 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) 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_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( @@ -2196,3 +2210,240 @@ 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 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(404) 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"], 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: + """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"