Skip to content

fix(discovery): bound OpenRouter free-model endpoint fan-out to one deadline - #939

Closed
seonghobae wants to merge 11 commits into
mainfrom
fix-openrouter-free-discovery-deadline
Closed

fix(discovery): bound OpenRouter free-model endpoint fan-out to one deadline#939
seonghobae wants to merge 11 commits into
mainfrom
fix-openrouter-free-discovery-deadline

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Why

_openrouter_free_model_endpoints() (in contextual_orchestrator/model_discovery.py) fetches per-model /endpoints data for every zero-priced OpenRouter model through an 8-worker ThreadPoolExecutor, with a per-request timeout. That per-request bound alone still lets total wall time scale with the free-catalog size: ceil(len(model_ids) / 8) sequential timeout waves. At today's ~94-model free catalog, that's up to 12 * 15s = 180s for this one enrichment step alone.

This was flagged by Devin Review on ContextualWisdomLab/.github#1463 (thread here): .github's contextual-orchestrator review sidecar has a 180s startup watchdog covering discovery + catalog build + preflight combined (scripts/ci/contextual_orchestrator_review_sidecar.sh's own comments already track this as a known, tight budget — see ContextualWisdomLab/.github#1455). This single OpenRouter enrichment call could exhaust that entire budget on its own in the worst case, on top of the ~6 other sequential discovery calls.

Confirmed the function is new relative to the sidecar's old vendored pin (git log -S "_openrouter_free_model_endpoints" → one commit, 6376d85), so this is a genuine regression risk introduced since that pin was last bumped — not a pre-existing characteristic Devin misattributed.

Live evidence: .github#1463's own Strix run (33348306414) shows the sidecar provisioning step completed successfully (~5m41s including clone+install, well under the watchdog) — so this is hardening against a worst-case tail risk, not a fix for an observed outage.

What changed

  • _openrouter_free_model_endpoints(): bounded to one overall timeout-second deadline via concurrent.futures.wait(futures, timeout=timeout) instead of executor.map() (which has no total-time bound and blocks the with-block exit on every submitted task regardless). Any model whose fetch hasn't completed by the shared deadline is reported as unmapped (None) — this is best-effort provider-privacy enrichment, not required for a model to be discovered. executor.shutdown(wait=False, cancel_futures=True) avoids blocking on stragglers.
  • Regression test (test_openrouter_free_model_endpoint_fetch_bounded_by_one_overall_deadline) simulates 200 permanently-hanging models and asserts total wall time stays under 2s (vs. an unbounded ~10s+ observed on the pre-fix code in a RED check).
  • Opt-in verbose/debug logging for the discovery path (--log-level / CONTEXTUAL_ORCHESTRATOR_LOG_LEVEL, off by default): per-provider and per-fetch timing/counts in model_discovery.py, wired through __main__.py. Never logs an api_key or provider payload/content — only identifiers, counts, and elapsed time. This gives operators visibility into exactly the kind of startup-budget question this PR investigates. Caught and fixed a real bug in my first pass: an unconditional setLevel call leaked global logger state across this repo's many in-process main() calls in tests, breaking test_telemetry.py's caplog.at_level("DEBUG") — fixed so _configure_logging() is a true no-op unless verbosity is actually requested.

Verification

  • python -m pytest tests -q (deselecting one pre-existing, unrelated environment gap — test_psychometric_routing.py's fast_mlsirm import isn't installed in this sandbox venv): 2803 passed, 1 skipped.
  • interrogate contextual_orchestrator/model_discovery.py contextual_orchestrator/__main__.py: 100% docstring coverage on both changed files.
  • RED-before-GREEN on the new deadline test: reverted the fix, confirmed the test fails (10s+ before erroring, proving the old code has no bound), restored the fix, confirmed it passes in 0.45s.

Generated by Claude Code

…eadline

_openrouter_free_model_endpoints() used a per-request timeout but no
overall deadline: with a fixed 8-worker thread pool, total wall time
still grew with the free-model count (ceil(n/8) sequential timeout
waves). At today's ~94-model free catalog that's up to 180s on its
own -- enough on its own to exhaust the contextual-orchestrator review
sidecar's 180s startup watchdog in ContextualWisdomLab/.github, flagged
by Devin Review on .github#1463. Bounded it to one shared timeout via
concurrent.futures.wait(), reporting any model not done by the deadline
as unmapped rather than waiting on further batches; this data is
best-effort provider-privacy enrichment, not required for discovery.
Regression test proves the bound holds regardless of catalog size.

Also adds opt-in verbose/debug logging for provider discovery
(contextual_orchestrator.model_discovery: per-provider and per-fetch
timing/counts, never api_key or provider payload/content) via
--log-level or CONTEXTUAL_ORCHESTRATOR_LOG_LEVEL, off by default and a
true no-op when unset so it can't leak state across this repo's many
in-process main() calls in tests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj32ABZLZ2a6TPTyvYrRkg
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 41 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5fd0bf65-f50e-4309-8a61-3104b00f91b2

📥 Commits

Reviewing files that changed from the base of the PR and between c6c3a0c and 2df8935.

📒 Files selected for processing (5)
  • contextual_orchestrator/__main__.py
  • contextual_orchestrator/model_discovery.py
  • docs/product-technical-gap-baseline.md
  • tests/test_discover_models_cli.py
  • tests/test_model_discovery.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae

Copy link
Copy Markdown
Contributor Author

일반화: 앞 코멘트의 NVIDIA는 예시일 뿐입니다. 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor/endpoint/model ID라도 서로 다른 key의 모델·권한·가격·privacy·가용성·실패 상태가 같다고 추정하거나 dedupe하지 마십시오. 개념은 제거하며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 으로만 성립합니다.

Copy link
Copy Markdown
Contributor Author

Reviewed against this PR's diff — it doesn't touch dedup, credential-equivalence, or endpoint-racing logic (it's scoped to bounding _openrouter_free_model_endpoints()'s fan-out to one deadline, plus opt-in discovery logging).

Checked the existing code against the principle raised: _serving_identity() / _deduplicate_discovered_models() in model_discovery.py key on (provider_name, model_id), and nvidia_nim / nvidia_nim_sub are distinct provider_name values (separate ProviderModelSource entries — PROVIDER_MODEL_SOURCES lines 199 and 207), so today's dedup never collapses discovery results across those two credentials even when they surface the same model ID. Any cross-credential or cross-provider equivalence claim beyond that already has to go through the explicit, fail-closed EndpointEquivalenceContract in endpoint_race.py ("every equivalence field must be explicitly declared") rather than any implicit inference. This PR adds no new equivalence/dedup surface, so nothing to change here — noted for awareness on future discovery work.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

opencode-review failure on head af17236 is the expected "no current-head verdict yet" gate, not a defect: the job's own logic explicitly requires an APPROVED/CHANGES_REQUESTED review from opencode-agent on this exact head SHA before it can pass — it fails closed until the async dispatch actually posts one. This PR was just pushed; watching for the real dispatch verdict rather than re-triggering or treating this as a code failure.


Generated by Claude Code

…ability gap

Documents PR #939's fix and the separate, larger orchestrator/free
gap found while root-causing .github#1463's live Strix failure
(tracked as issue #940) -- a single model that rejects multi-tool-
call requests hard-fails the whole request with no failover, and
there's no capability signal to exclude it from candidate selection.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj32ABZLZ2a6TPTyvYrRkg
@seonghobae

Copy link
Copy Markdown
Contributor Author

최종 일반 계약: 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor, endpoint, model ID라도 서로 다른 key의 모델, 권한, 가격, privacy, 가용성, 실패 상태를 같다고 추정하거나 dedupe하지 않습니다. provider family는 없으며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 model_group으로만 성립합니다.

Copy link
Copy Markdown
Contributor Author

Understood — acting on this now as its own fix, separate from this PR's scope.

Found the actual violation: contextual_orchestrator/model_discovery.py's _provider_family() hardcodes {"nvidia_nim", "nvidia_nim_sub"} → "nvidia_nim", treating two independent KV credentials as one outage domain by assumption, not operator declaration. It's used in three places: select_bootstrap_discovered_agents (bootstrap diversity capping), provider_bootstrap.select_provider_diverse_models (same), and provider_catalog_bootstrap.evaluate_provider_credential_inventory (multi-provider-outage tolerance bound). All three infer credential equivalence instead of requiring it through the explicit model_group contract (contextual_orchestrator/model_group.py) this repo already has for exactly this purpose.

I'll remove the heuristic (treat every KV credential/provider_name as its own independent family — no collapsing) at all three call sites, update the tests that currently assert the collapsing behavior, and where removing it changes real operational tolerance (evaluate_provider_credential_inventory's max_tolerated_missing_providers bound, added in #928 specifically to avoid double-counting what was assumed to be one NVIDIA outage as two), adjust the bound honestly rather than reintroducing an equivalence assumption to compensate. Will push as its own PR referencing this thread.


Generated by Claude Code

@seonghobae

Copy link
Copy Markdown
Contributor Author

Propagation update: PR #941 is now merged to main at 42da1d5. This PR branch conflicts with automatic base update, so its implementation must preserve the main contract: every KV credential account is discovered independently; vendor or endpoint identity does not imply model equivalence; only explicit model_group membership shares routing evidence; peak observed RPM and TPM remain measured per account-model route. The protected base already enforces this contract even before this branch resolves its conflicts.

seonghobae and others added 3 commits August 31, 2026 11:51
…removal

#941 removed _provider_family() (nvidia_nim/nvidia_nim_sub collapsing)
but didn't touch tests/test_discovery_bootstrap_selection.py, leaving
test_bootstrap_selector_treats_nim_primary_and_sub_as_one_outage_domain
asserting the now-removed collapsing behavior. Updated to assert the
correct independent-provider behavior. Also folds this PR's logging
additions to match #941's "account="-prefixed message convention.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj32ABZLZ2a6TPTyvYrRkg
…iscovery-deadline

# Conflicts:
#	tests/test_discovery_bootstrap_selection.py
claude added 2 commits August 31, 2026 05:11
…iscovery-deadline

# Conflicts:
#	contextual_orchestrator/model_discovery.py
…iscovery-deadline

# Conflicts:
#	contextual_orchestrator/model_discovery.py
@seonghobae
seonghobae marked this pull request as ready for review August 31, 2026 05:30
devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

strix failed on head efdea2a after running its full ~2.5h duration (05:34:59–08:05:08 UTC) with no findings text, no summary, and no PR comment posted — the job's own check-run output is empty and the run's "Collect/Upload Strix reports artifact" steps succeeded, suggesting an internal tool timeout/crash during the scan itself rather than a security finding in this PR's diff. Not re-running it manually on that head: a new commit (Devin Review fixes, below) is being pushed now, which will dispatch a fresh Strix run on the new head anyway. Will watch that run; if it also fails without findings, will investigate further as a tool-level issue rather than treat it as this PR's code problem.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

noema-review failed on head 2df8935 with TimeoutError: timed out at noema_review_gate.py:656 (opener.open(request, timeout=120)). Not this PR's diff — noema_review_gate.py is canonical only in ContextualWisdomLab/.github and is materialized fresh from that repo's trusted main at review time (confirmed: this repo has no local copy of that script or noema-review.yml), so nothing in contextual-orchestrator can fix or shim around it.

Root cause: a hardcoded timeout=120 on the real review-completion HTTP call, three orders of magnitude under this org's own stated "Noema may take over two hours per model" policy. A fix is in progress upstream (.github#1507, not yet merged — actively driven with 31 commits and live cross-repo canary evidence from at least three other consumer repos hitting the identical failure). No consumer-side workaround exists per that PR's own investigation. Not re-running manually — this is a deterministic policy/timeout mismatch, not a flake, and a rerun on the same code would fail the same way. Watching .github#1507 for merge; this PR's noema-review will resolve automatically on its next scheduled re-check once that lands.


Generated by Claude Code

@seonghobae

Copy link
Copy Markdown
Contributor Author

Cross-PR integration contract: routing identity is provider-neutral model_group only; do not add or preserve a provider-family abstraction. OpenRouter discovery must retain concrete free model IDs, while the aggregate openrouter/free router is not a serving candidate. OpenCode, Noema, and Strix must call contextual-orchestrator. Do not impose fixed wall-clock deadlines on inference, initial ping, readiness/health, provider discovery, or OpenRouter ZDR-list retrieval; use explicit cancellation and evidence-backed transport failure instead. Reconcile this PR with #971 and central .github #1508 before merge.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Superseded by #971 and the organization no-wall-clock-deadline policy. This branch adds a shared elapsed-time deadline to OpenRouter free/ZDR discovery; #971 already discovers concrete zero-priced model IDs while excluding the aggregate openrouter/free, without a fixed discovery deadline. Do not port the deadline implementation. Any non-time observability can be proposed separately after #971 lands.

@seonghobae seonghobae closed this Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants