Persist bounded model-group routing observations - #911
Conversation
|
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옵트인 SQLite 라우팅 관찰 저장소가 컨텍스트 키와 완료 시각을 저장하고 공유합니다. CLI와 오케스트레이터가 저장소를 구성하고 라우팅 경로에 연결합니다. Responses의 빈 Changes라우팅 관찰 지속성
요청 경로 동작 수정
프로젝트 기록 갱신
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The opt-in durable-routing path can turn successful requests into failures during storage errors, lose routing observations when provider identity changes, and expose raw exception details in service logs. The PR should not merge until these bounded correctness, availability, and observability risks are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant CLI
participant TaskOrchestrator
participant ModelGroupRouter
participant SqliteRoutingObservationStore
participant SQLite
CLI->>TaskOrchestrator: 시간 창과 state_db 전달
TaskOrchestrator->>SqliteRoutingObservationStore: 저장소 초기화
TaskOrchestrator->>ModelGroupRouter: transport 및 quality 라우터 연결
ModelGroupRouter->>SqliteRoutingObservationStore: context_key와 observed_at을 포함한 관찰값 append
SqliteRoutingObservationStore->>SQLite: 관찰값 저장 및 공유 최대 보존 창 기준 정리
TaskOrchestrator->>ModelGroupRouter: refresh()
ModelGroupRouter->>SqliteRoutingObservationStore: active_contexts로 조회
SqliteRoutingObservationStore->>SQLite: 현재 컨텍스트 관찰값 조회
SQLite-->>SqliteRoutingObservationStore: 관찰값 반환
SqliteRoutingObservationStore-->>ModelGroupRouter: 관찰값 목록 반환
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 47.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 111 functions across 11 files. (9 skipped: 8 unsupported, 1 too large.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![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.
![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.
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.
|
Reusable conflict resolver stopped fail-closed. Executable or structured-data conflicts require semantic resolution at exact head |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Reusable conflict resolver stopped fail-closed. Exact PR head: Executable or structured-data conflicts require semantic resolution:
|
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.
|
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. |
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.
|
Cross-PR integration contract: routing identity is provider-neutral |
|
Model-group/no-timeout contract audit against parent #971 exact head |
|
Scheduled review-feedback autofix for this PR head.
|
|
@cwl-noema-review @opencode-agent Re-run independent review on exact head |
|
Scheduled review-feedback autofix for this PR head.
|
|
Review-only status update, respecting your explicit hold on this branch — nothing pushed, nothing rebased. I investigated resolving this PR's merge conflict against current Then I read your most recent comments on this thread (2026-09-01, after the snapshot I'd been given) — the explicit "must not merge as-is," the request to reconcile with #971 and The resolved merge is sitting uncommitted-to-origin locally (not lost) — happy to push it once you've decided this branch is ready to move, or to hand the specific bug list above to whoever ends up doing the real rebase, so that work isn't re-discovered from scratch. Generated by Claude Code |
|
Scheduled review-feedback autofix for this PR head.
|
seonghobae
added
priority: medium
ChatGPT Codex Connector
|
Routine staleness triage: this is the PR matching the org's known stale-base marker — its recorded I deliberately did not attempt a merge/push here: this session's shared environment shows clear signs of another in-progress process already mid-resolution on this exact branch (an existing local checkout with unpushed commits including a Flagging for visibility in case that other effort stalls or is abandoned: as of this comment the PR branch on GitHub is still unchanged (head Generated by Claude Code |
|
Re-verified this PR fresh (head still
Root-caused both original CI failures from job logs anyway, since that's independent of whether a merge proceeds:
Both are review-infrastructure gaps (consistent with the #868/#857/#906/#912-cycle pattern already flagged in this thread), not code defects this PR needs to fix. No action taken beyond this diagnosis — leaving the merge and any check re-run to whoever completes the #971 reconciliation, per your hold. Generated by Claude Code |
|
Scheduled review-feedback autofix for this PR head.
|
Resolves 8 conflicted files on this 5-day-old branch (CHANGELOG.md, __main__.py, model_group.py, orchestrator.py, provider_errors.py, docs/library_research.md, docs/product-technical-gap-baseline.md, tests/test_embeddings_model_pool_http_honesty.py). The two docs/CHANGELOG conflicts were the usual "both sides append a distinct dated entry" shape (kept both). The code conflicts needed real feature-merging, not a pick-one-side resolution, since main had independently evolved the exact same call sites this branch touches: - model_group.py's observe_success/_apply_success_locked: kept this branch's persist_observation/_apply_success_locked architecture (already used by 5+ other call sites org-wide) but merged in main's independently-added optional-latency support (latency_seconds: float | None, for successes with no honest single-attempt timing, e.g. shared Batch API calls) and its clamped-is-not-None guard -- which required widening _apply_success_locked's own latency param to Optional and adding the matching guard there too, since its original unconditional ewma update would have crashed on None. - orchestrator.py's _orchestrated_provider_completion synthesis/repair retry loop: took main's current while-loop structure (EffortProfileError handling, schema-repair-once logic -- this branch predates that whole refactor) but restored this branch's durable-persistence context-key tracking (synthesis_context_key/repair_context_key, _record_group_failure(_for_agent) wrapper calls) at each of the 4 failure-recording call sites, and fixed one condition (candidate.group_name and not (request_too_large OR capability_mismatch)) this branch had dropped relative to main. - _invoke's return shape: main independently widened it to a 4-tuple (output, served_id, served_model, usage); this branch's competing widening was a named _InvocationResult dataclass (output, served_id, usage, observation_context_key) with a 3-item __iter__ for tuple-unpack compat. Merged: added served_model as _InvocationResult's 3rd field, extended __iter__ to yield it, and fixed both _InvocationResult(...) construction sites plus the __invoke race-path usage extraction (was reading the stale outcome.value[2] as usage; main's caller-side code already used outcome.value[3] a few lines above it -- confirmed via call()'s own `-> tuple[str, str, str, ...]` return annotation that index 2 is now served_model, not usage). - provider_errors.py: kept this branch's _sanitize_provider_message_text extraction (reused at a second call site) but folded in main's independently-added _SAFE_SCHEMA_DIAGNOSTIC check, which the extraction had been missing. Two rounds of full-suite failures beyond the marked conflicts, both fixed: 1. route_once had a leftover `getattr(invocation, ...)` reference to a variable a different call site's resolution had removed -- caused a NameError cascading into ~448 failures across every test whose fixture transitively calls route_once. Fixed by capturing the _InvocationResult object instead of discarding it after unpacking. 2. Two unrelated post-fix failures: an ADR-0039 number collision (this branch's own "time-windowed-routing-observations" ADR vs. an independently-landed main ADR of the same number) -- renumbered to 0042 (next free slot before the 0124 block), fixed 3 cross-references in CHANGELOG.md/library_research.md/product-technical-gap-baseline.md. And a genuinely unrelated main-side token_counting.py rewrite (old heuristic word-count estimator replaced with a strict exact-model-allowlist policy that fails closed for unknown model names) broke this branch's own test_embedding_attempts_keep_their_original_routing_context, which never got the ExactSyntheticCounter workaround every sibling test in the same file already uses for its "mock-planner" fixture model name -- applied the same pattern. Full suite: 3445 passed, 1 skipped. provider_errors.py 100% coverage, model_group.py 98% (pre-existing gaps in unrelated validation branches, confirmed my own added guard is fully covered), interrogate 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| if success: | ||
| if isinstance(latency_seconds, bool) or not isinstance(latency_seconds, (int, float)): | ||
| raise TypeError("successful observations require numeric latency_seconds") |
There was a problem hiding this comment.
🔴 Successful batches fail during finalization
Accepted batches pass no latency from _finalize_batch_row, but append rejects such successful observations. Durable routing aborts after provider completion.
Prompt for agents
Allow successful routing observations whose per-attempt latency is unknown. The existing ModelGroupRouter.observe_success contract explicitly accepts latency_seconds=None for batch results and still records the Bernoulli success. Update contextual_orchestrator/routing_observation_store.py schema validation and replay handling in contextual_orchestrator/model_group.py so these rows persist and restore the success count without changing latency or throughput EWMAs. Add an opt-in TaskOrchestrator batch-route regression test covering an accepted result with routing_observation_window_seconds enabled.
Was this helpful? React with 👍 or 👎 to provide feedback.
| self._routing_observation_store = ( | ||
| SqliteRoutingObservationStore( | ||
| state_db, | ||
| routing_observation_window_seconds, | ||
| start_heartbeat=False, | ||
| ) |
There was a problem hiding this comment.
🟡 Failed startup leaves retention lease
When later constructor validation fails, SqliteRoutingObservationStore remains unclosed. Its stale lease can delay pruning for twice the configured window.
Prompt for agents
Make TaskOrchestrator initialization clean up SqliteRoutingObservationStore whenever any later initialization step fails. Prefer validating all constructor arguments before creating durable resources, or wrap subsequent setup in exception-safe cleanup. Verify that a failed initialization removes its routing_observation_registrations row rather than merely avoiding heartbeat startup.
Was this helpful? React with 👍 or 👎 to provide feedback.
| observation_context_key = ( | ||
| orchestrator._routing_observation_context_for_agent(embedding_agent) | ||
| ) |
There was a problem hiding this comment.
🟡 Incomplete embedding jobs abort failover
When a synchronous embedding job returns incomplete, its failure bypasses _record_group_failure and the captured observation_context_key. Storage errors return 500 instead of continuing failover.
Prompt for agents
In the synchronous /v1/embeddings loop, route the non-completed-document failure through TaskOrchestrator._record_group_failure with the observation_context_key captured before the attempt. This must preserve the expected member failover when durable observation persistence fails and must attribute an in-flight attempt to its original agent shape.
Was this helpful? React with 👍 or 👎 to provide feedback.
| *, | ||
| observation_context_key: str | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
🟡 Reassigned batches contaminate replacement quality
When an agent changes after batch execution but before judging, _record receives no captured context. The old answer updates the replacement's quality evidence.
Prompt for agents
Capture each batch attempt's routing observation context from the ModelAgent used for execution and carry it through batch row finalization into _realtime_route_judge. Do not resolve the context from the current pool at judge time. Add a concurrent reassignment regression similar to the stream and embedding context tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _register_retention_window(self, connection: sqlite3.Connection) -> None: | ||
| """Persist the largest configured shared-retention window for this database.""" | ||
| connection.execute( | ||
| "INSERT INTO routing_observation_metadata(metadata_key, metadata_value) " | ||
| "VALUES(?, ?) " | ||
| "ON CONFLICT(metadata_key) DO UPDATE SET " | ||
| "metadata_value = MAX(routing_observation_metadata.metadata_value, excluded.metadata_value)", | ||
| (self._MAX_RETENTION_WINDOW_KEY, self._window_seconds), | ||
| ) |
There was a problem hiding this comment.
🔍 Retention metadata no longer governs pruning
max_retention_window_seconds is persisted and tested, but pruning reads active registrations only. Remove or clearly label this legacy metadata to avoid false operational assumptions.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
routing_observationsstore for transport and quality ledgers.Validation
pytest -q— 2537 passedruff checkon changed Python filessemgrep --config p/pythonon changed runtime files — 0 findingspython -m compileall -q contextual_orchestratorgit diff --checkBased on
origin/mainb21645116b352967e50fc497b87eb745b9cc8c61.Summary by CodeRabbit
새로운 기능
--routing-observation-window-seconds및--state-db옵션으로 설정할 수 있습니다.버그 수정
seed및top_logprobs가 불필요한 비스트리밍 처리를 유발하지 않습니다.