fix(routing): select concrete free model groups - #971
Conversation
Exclude OpenRouter's aggregate free router while retaining discovered concrete free models. Group exact model identities across provider accounts and remove fixed inference deadlines, including readiness races. Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough모델 검색과 부트스트랩이 모델 그룹 및 fingerprint ID를 사용합니다. 공급자 호출은 선택적 타임아웃과 명시적 취소를 지원합니다. OpenRouter ZDR 라우팅, 임베딩 실패 기록, readiness 동시성, 레거시 에이전트 동기화 및 관련 회귀 검증을 갱신합니다. Changes모델 검색 및 부트스트랩
공급자 취소 및 readiness
ZDR 라우팅 및 임베딩
계약 및 운영 지원
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🔴 Critical · up to The current head still relies on temporary source-rewriting machinery instead of committed production fixes, while provider discovery can wait indefinitely or leak background work and embedding and failover paths retain correctness and privacy risks. These issues can cause hangs, resource exhaustion, incorrect routing, or loss of privacy guarantees, so the PR is not merge-ready and should remain blocked. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 52.20% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 341 functions across 44 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Routing contract: provider family is not a product grouping. Selection and measurement use |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Seongho Bae <me@seonghobae.me>
Signed-off-by: Seongho Bae <me@seonghobae.me>
Signed-off-by: Seongho Bae <me@seonghobae.me>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Seongho Bae <me@seonghobae.me>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Seongho Bae <me@seonghobae.me>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Seongho Bae <me@seonghobae.me>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Seongho Bae <me@seonghobae.me>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A fresh Devin Review pass on this PR (#971) flagged an Info-severity gap in the _DaemonWorkerPool added in 59b2fc8 to replace ThreadPoolExecutor for ProviderEmbeddingBatchBackend: shutdown() set no closed state, so a concurrent direct submit() could enqueue real work behind the shutdown sentinels every worker exits on -- work no worker would ever pick up again. Real ThreadPoolExecutor.submit() raises RuntimeError once shutdown() has run; _DaemonWorkerPool did not replicate that fail-fast contract. Traced every current submit() call site: ProviderEmbeddingBatchBackend only ever calls it from __init__ (before any external reference to self exists) and from start(), and both start() and close() serialize through the backend's own _executor_lock, with start() checking self._closed first. So today's actual production risk is narrow -- the backend's own locking already prevents the described race in practice, and its durable claim/recovery design would likely reclaim a stranded job on the next process start regardless. But _DaemonWorkerPool is a standalone primitive that should not depend on every future caller reproducing that locking discipline, and matching stdlib's fail-fast contract is the correct, minimal fix regardless of today's callers. Added a self._shutdown flag, set under the pool's existing _workers_lock inside shutdown() and checked under the same lock at the top of submit() (which now also enqueues under that lock, not before acquiring it) so the check and the enqueue/shutdown transition can never interleave. submit() now raises RuntimeError("cannot schedule new work after shutdown") instead of silently queuing behind the sentinels. Left worker lazy-spawn, cancel_futures draining, and queue join behavior untouched. Added tests/test_provider_embedding_batch_backend.py::test_daemon_worker_pool_submit_after_shutdown_raises_instead_of_stranding_work, which submits and completes one real job through the pool, calls shutdown(), then asserts a further submit() raises RuntimeError. tests/test_provider_embedding_batch_backend.py (27 tests) and tests/test_provider_embedding_batch_backend_process_exit.py (1 test) pass. Broader affected suite (batch_routing/cost_router/API-contract tests importing batch_routing, 14 files): 210 passed, the same 4 pre-existing tokenizer-unavailable ZDR failures in test_batch_embeddings.py (unrelated to this change, present before it). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
…er' into fix/model-group-timeout-openrouter
Append a one-line changelog fragment entry for the _DaemonWorkerPool fail-fast fix in 18a76eb, matching this file's existing per-PR fragment convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A fresh Devin Review pass on this PR (#971) flagged "Concurrent shutdown admits cancelled work" in _DaemonWorkerPool.shutdown (added in 59b2fc8, closed-state flag added in 18a76eb): when shutdown(cancel_futures=True) races submit(), the cancel_futures queue drain ran *before* admission closed (self._shutdown = True, under _workers_lock). A submit() landing in that gap could acquire the (still-unheld) lock, see self._shutdown still False, and enqueue real work strictly after the drain loop had already found the queue empty and exited -- that item then survived the cancellation, got a stop sentinel queued behind it, and ran on the worker shutdown() itself spawned and joined, despite cancel_futures=True. Fixed by reordering shutdown(): it now acquires _workers_lock, sets self._shutdown = True, and captures the worker list as one atomic step *first* -- before any draining or sentinel-queuing. submit() checks the same flag under the same lock, so the two methods can no longer interleave: a submit() that acquires the lock after this closes admission observes the closed pool and raises immediately (unchanged fail-fast contract from 18a76eb); a submit() already holding the lock is guaranteed to finish enqueuing before shutdown() can proceed, so its item is still present in the queue when the drain runs right after and is correctly cancelled. A worker that already dequeued and started an item before the drain runs is an inherent, expected race identical to ThreadPoolExecutor's own cancel_futures (it cannot interrupt in-flight work) -- not the bug here. Also made shutdown() idempotent against its own repeated-call behavior: a second call now observes self._shutdown already set and skips re-draining the queue and re-queuing stop sentinels (both only correct to do once), while still joining the already-captured worker list when wait=True. Added two regression tests to tests/test_daemon_worker_pool_shutdown.py: test_daemon_worker_pool_submit_racing_shutdown_cannot_execute forces the exact vulnerable interleaving deterministically (hooking the queue drain's empty-check with an Event, no real thread-timing luck involved) and asserts the racing submit() raises RuntimeError and its work never executes; verified this test fails against the pre-fix shutdown() (0 RuntimeErrors raised, work would have run) and passes against the fix. test_daemon_worker_pool_shutdown_is_idempotent asserts a second shutdown() call does not error. Full daemon-pool/provider-embedding/batch-routing suite (62 tests across tests/test_daemon_worker_pool_shutdown.py, tests/test_provider_embedding_batch_backend.py, tests/test_provider_embedding_batch_backend_process_exit.py, tests/test_batch_routing.py, tests/test_batch_routing_boundaries.py, tests/test_batch_routing_boundaries_extra.py): all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Two fresh Devin Review findings landed on .github/workflows/_temp_pr971_daemon_pool_shutdown.yml (added in 8caf844, one commit before the actual fix in 18a76eb landed as an ordinary commit): - "Obsolete repair workflow remains active": the workflow's "Prove RED regression on event head" step requires tests/test_daemon_worker_pool_shutdown.py to fail before it will apply its generated repair; current source already fixes the regression the workflow exists to react to. Verified directly: on this branch's head (558c847), and independently on the pre-this-PR state (before the present commit's shutdown-ordering fix), that test file already passes. Any dispatch hits "RED regression unexpectedly passed before production repair" and exits before ever reaching the self-removal step at the end -- the workflow is fully dead, not merely redundant. - "Repair workflow can rewrite the PR branch": its final step uses secrets.PR_REVIEW_MERGE_TOKEN to git commit and push generated changes directly to this PR's branch on a manual workflow_dispatch, bypassing normal PR review/merge controls -- a live exposure for anyone with dispatch permission on this repo, independent of whether the (now-moot) RED-regression guard currently blocks that step from being reached. Confirmed before removing: the file has exactly one commit (8caf844, 2026-09-02T14:35:29+09:00) and the branch has had no further commits in the ~14 minutes since, so this is not concurrent in-progress work; no paired helper script exists (unlike the earlier source-fix-971-review-quality.yml + source_fix_971_review_quality.py pair); and no other file in the repository references _temp_pr971_daemon_pool_shutdown.yml. This follows the branch's own established "no purpose-complete self-modifying/source-fix workflows" convention already applied to the earlier source-fix-971-review-quality.yml removal (9b8f609, 0247eca, d388564): remove confirmed-obsolete, security-carrying repair-workflow debris rather than leave it as inert (but exploitable) machinery. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
…g-fix session Adds a dated entry documenting today's remaining #971 session work not yet covered by an existing entry: the real main-merge conflict resolution (mergeable_state dirty -> blocked, tests/test_review_gateway.py resolved in favor of main's evidence-only-admission suite after main's 0db4e5a dropped max_agents), the two additional ThreadPoolExecutor-atexit-join fixes in endpoint_race.py and batch_routing.py's new _DaemonWorkerPool, the raw-Future callback-settlement regression fix, the _DaemonWorkerPool post-shutdown submit hardening, a verified-false-positive analysis of a proposed provider_routing equality guard, and a recap of the remaining open, large-scope work (bootstrap diversity heuristics, hourly-loop cancel-in-progress gap with a PR #1027 pointer, legacy identifier mixing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
…er' into fix/model-group-timeout-openrouter
| if agents_db | ||
| else () | ||
| ) | ||
| selected_ids = enabled_ids if agents_db else generated_selected_ids |
There was a problem hiding this comment.
🟡 Durable selection order is lost
selected_ids adopts alphabetically sorted enabled IDs for durable pools, discarding the selector’s cost and model-group order. Ephemeral reports retain that order.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -0,0 +1,61 @@ | |||
| """Regression coverage for bootstrap report identity consistency.""" | |||
There was a problem hiding this comment.
🔍 Regression misses direct check list
This test lacks a directly runnable entry and README check-list registration. The documented per-file workflow therefore omits the identity regression.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Fresh consumer evidence that should be carried by this owner lane rather than worked around in leaves:
Owner RED/GREEN acceptance to add before release: a long-lived |
Fresh cross-repo consumer evidence on PR #971 (keyverse#132, EgressWeave#235) independently hit the same owner-lane gap: no canonical immutable release means consumers vendor a raw source SHA, and no owner-side resumable/ checkpointed execution model means every leaf re-invents its own wall-clock timeout wrapper around the timeout=null inference contract. Records the gap, why it's owner-lane (two independent consumers, not one), scoped action items (SemVer release; a dedicated ADR for resumability with distinguishable terminal states), and the owner's own stated RED/GREEN acceptance criteria, per this repo's standing directive to keep the gap baseline current from live PR/consumer evidence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Acknowledged — recorded this evidence and the two scoped action items in This is genuinely two separable, architecturally significant pieces of work, not a small local fix, so I'm not folding an implementation into this PR (which is already close to mergeable on its own narrower scope: model-group timeout/thread-pool-shutdown fixes). Proposing to track them as their own dedicated tracks:
Will pick these up as the next highest-leverage gap items once the current PR review/fix backlog is exhausted, per the standing loop. Flag if you'd rather see either scoped differently or prioritized sooner. 🤖 Generated with Claude Code Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two real citation errors in the 2026-09-02 canonical-release/resumable- execution entry: docs/CWL-MASTER-CONTEXT.md does not exist in this repo (it is ContextualWisdomLab/.github's org-wide doc) and this repo's AGENTS.md carries no such rule directly -- corrected to cite it as the .github repo's document. The no-fixed-timeout policy citation named "ADR-0005", which is the .github repo's numbering for a related policy; this repo's own equivalent is docs/planning/adrs/0032-model-group-cost-aware-discovery.md -- corrected to cite that file directly with its exact text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
"RED to GREEN owner repair" failure — not a defect in this PR's diff, already self-resolvedInvestigated the failing required-looking check What it actually isThat job belongs to workflow Why it failedThe failed step is its own safety guard ( This run was queued for the push at Confirms it isn't live
ConclusionNot a defect introduced by this PR's diff — it's the same known org-wide GitHub Actions queue-congestion pattern hitting a now-already-deleted one-shot repair workflow's own stale-head guard. No source change is needed or applicable here; the machinery that produced this failing run is already gone from the branch. Current-head required checks (Security, CodeQL, Full unit and contract suite, etc.) remain the authoritative gate and are tracked separately. Generated by Claude Code |
3 participants
Current owner outcome — 2026-09-03
This PR remains the canonical
contextual-orchestratorowner lane for concrete free-model grouping, provider discovery/routing, timeout semantics, endpoint races, and provider-embedding runtime behavior. It is not merge-ready. GitHub still reportsdraft=false; a freshconvertPullRequestToDraftattempt failed in the connector GraphQL response becauseRepository.fullDatabaseIdis not available, so the body remains the explicit Draft-equivalent state. No predecessor-head check/review is being transferred.Current exact head:
696fd17431ce5937fb318e241837709c4eebb989onfix/model-group-timeout-openrouter, basemain@464da4715b495b5eaaa593eba3796e2d976ee0c9.Exact-head evidence boundary
Testsrun33653589654has materialized two exact-head jobs (Full unit and contract suite;NIM benchmark coverage, docstrings, and package smoke) but they are still queued with no runner assigned. Security, Fuzz, Semgrep, OSV, Scorecard, and Security Scan are likewise non-terminal.CodeQL PRrun33653592151terminatedstartup_failurebefore job materialization; its jobs endpoint reportstotal_count: 0, jobs: []. This organization-owned Actions defect has been handed to.github#712with exact-head RED/GREEN acceptance. It is platform evidence, not source-success evidence, and required gates remain fail-closed.Completed causal repairs on this successor line
tests/test_provider_bootstrap_report_identity.pywas added first at770dada2d11bf8ab939ded292929448fb7588d8e; source fix7369d48d134f2f02c41d4a3db9be2a5bf83a3537makes a durable bootstrap report the resolved persisted identity in bothselected_agent_idsandenabled_agent_ids, while ephemeral bootstrap retains generated IDs.provider_catalog_bootstrap.py.tests/test_provider_catalog_bootstrap_report_identity.pywas added at3acc6cd1438ff46dcd5e7c9d2fa8d639f16ee449; minimal source fixd02c97c5c9a548467d1286f160a9cc7498916810was diff-checked as exactly one file, +2/-1.CHANGELOG.d/bootstrap-report-persisted-identity.mdrecords the durable contract on the current head.Still-live repair boundaries
cancel-in-progress: falsecan serialize later hourly triggers behind an execution that never returns, while a leaf-authored finite model timeout would violate thetimeout=nullmodel contract. The needed boundary is durable/resumable contextual-orchestrator-owned execution/checkpoint/re-dispatch preserving exact-head identity across external runner termination.Stable architecture boundary
GitHub Actions callers request exactly
orchestrator/free; contextual-orchestrator alone owns provider discovery, capability/privacy/free-pool admission, ranking, serving, and failover. All configured credential sources may participate in global discovery. OpenAI-derived models remain excluded fromorchestrator/freewhile the global OpenAI integration remains available elsewhere. Private targets require ZDR admission. Default model inference timeout isnull; elapsed time alone does not terminate reasoning, streaming, or tool work. Explicit caller/user cancellation, provider termination, audited admin timeout, and infrastructure loss remain distinct states.Before ordinary merge: repair the remaining current-head owner defects test-first; preserve the no-self-modifying-source boundary and
orchestrator/freecontract; resolve only findings proven repaired or obsolete on an unchanged successor head; regenerate full exact-head tests/security/review/SBOM/provenance; and synchronize ADR/PRD/ARCHITECTURE/CHANGELOG/product-gap evidence. No force push, self-approval, administrative bypass, gate weakening, or predecessor-evidence transfer is authorized.