feat(admin): per-model LLM timeout view/set/clear/restore - #1010
feat(admin): per-model LLM timeout view/set/clear/restore#1010seonghobae wants to merge 2 commits into
Conversation
Closes the standing admin-web requirement in ContextualWisdomLab/.github's
docs/product-goal-directive.md §8: no per-model timeout config existed
anywhere -- ModelClient.timeout was one flat instance value (default 90s)
applied to every outbound call regardless of model.
- TaskOrchestrator gains list_model_timeouts/get_model_timeout/
set_model_timeout/clear_model_timeout, following the existing
model_group family's pattern exactly (KV-persisted via a new
"model_timeout_override" keyed kind + _StateStore.delete, audited via
the existing _append_audit_event, KeyError/ValueError on unknown model /
invalid value). MIN=1s/MAX=14400s bounds match the org's own
NOEMA_LLM_TIMEOUT_SECONDS precedent, not an invented number.
- New API: GET /api/v1/model_timeouts(/{model}), PATCH/DELETE
/api/v1/model_timeouts/{model} (PATCH to match this repo's existing
model_groups convention -- there is no do_PUT handler), declared in
api_contract.py.
- Real enforcement: ModelClient.model_timeout_resolver is wired to a
resolver that returns None absent an override (not the resolved
default), so chat()/stream_chat() omit the timeout kwarg entirely in the
common case -- identical to every existing caller's prior behavior --
and only pass an explicit timeout when an operator actually set one.
Verified against a fake transport capturing the real timeout= reaching
_open_provider for both the chat and streaming-chat paths.
- New "Model timeouts" panel in admin.py's existing Settings view
(bilingual en/ko), following the model_groups panel's exact
render/refresh/save pattern. Purely additive -- no existing line
changed, so test_admin_contract.py's 100+ exact-string assertions still
pass unmodified. Extends the existing /admin console per planning ADR
0033 (operative: defer React/Storybook until a concrete trigger is
met -- none is met here); does not touch the unbuilt admin_ui/ scaffold.
- Planning ADR 0042 records the design, including a regression this
change introduced and fixed before push: an earlier version always
resolved and passed a concrete timeout, which broke three existing
test files' strict-signature ModelClient._send_with_retry/_stream_send
mock stand-ins -- fixed by correcting the resolver's semantics (return
None absent an override), not by patching every test double.
29 new tests (tests/test_model_timeouts.py). Full suite: 3336 passed, 1
skipped (was 3307 passed, 1 skipped before this change). 100% docstring
coverage (interrogate) on every touched module.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 26 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
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}
| timeout_override = self._resolve_timeout(agent) | ||
| return ( | ||
| self._send_with_retry(agent, payload, destination) | ||
| if timeout_override is None | ||
| else self._send_with_retry(agent, payload, destination, timeout=timeout_override) |
There was a problem hiding this comment.
🟡 Local queue ignores model timeout
Under local contention, _local_provider_slot uses the default before _resolve_timeout reads the override. Requests can fail at the old deadline.
Prompt for agents
Resolve the model-specific timeout before entering _local_provider_slot in both ModelClient.chat and ModelClient.stream_chat. Use the effective override for local queue acquisition as well as provider I/O, while preserving the existing no-override call shape for _send_with_retry and _stream_send. Add contention tests where the override is longer and shorter than ModelClient.timeout so the queue deadline and transport timeout follow the same model setting.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Admin-editable per-model timeout overrides (model name -> seconds). | ||
| # Absent entries inherit self.client.timeout. Wired into the client as | ||
| # a resolver so every chat/stream call already routed through | ||
| # ModelClient picks up operator changes with no other call-site change. | ||
| self._model_timeout_overrides: dict[str, float] = {} | ||
| if isinstance(self.client, ModelClient): | ||
| self.client.model_timeout_resolver = self._model_timeout_override_for |
There was a problem hiding this comment.
🟡 Passthrough requests ignore model timeouts
Chat requests using tools call proxy_send, bypassing the new resolver. They keep the flat timeout despite an administrator's per-model override.
Prompt for agents
Apply the per-model timeout resolver to ModelClient's chat-compatible passthrough paths, including proxy_send/proxy_send_once for chat/completions and Responses requests. Thread the resolved timeout through _send_raw_with_retry and _open_provider while retaining capability-probe timeout behavior and the no-override call shape. Cover explicit-model tool and response-format requests through the HTTP API.
Was this helpful? React with 👍 or 👎 to provide feedback.
| previous = self._model_timeout_overrides.get(model) | ||
| self._model_timeout_overrides[model] = value | ||
| if self._store is not None: | ||
| self._store.save("model_timeout_override", model, {"model": model, "timeout_seconds": value}) |
There was a problem hiding this comment.
🟡 Failed saves still change timeouts
set_model_timeout mutates live state before persistence commits. A storage failure returns an error while changing the timeout until restart.
Prompt for agents
Make set_model_timeout and clear_model_timeout atomic across durable storage and the in-memory override registry. Persist first or roll back the in-memory mutation when _StateStore fails, and serialize concurrent set/clear operations so runtime state, returned status, audit records, and restart state cannot diverge. Add failure-injection tests for both save and delete paths.
Was this helpful? React with 👍 or 👎 to provide feedback.
| --- | ||
| id: "0042" | ||
| title: "Per-model LLM request timeout: admin-editable override, audited, inherited" | ||
| status: accepted | ||
| proposed_date: "2026-09-02" | ||
| accepted_date: "2026-09-02" | ||
| deciders: | ||
| - "repository maintainer" | ||
| affected_components: | ||
| - "contextual_orchestrator/orchestrator.py" | ||
| - "contextual_orchestrator/server.py" | ||
| - "contextual_orchestrator/api_contract.py" | ||
| - "contextual_orchestrator/admin.py" | ||
| related: | ||
| - path: "docs/planning/adrs/0033-admin-console-ui-tooling-boundary.md" | ||
| relation: informational | ||
| success_criteria: | ||
| - metric: "operator can view/set/clear per-model timeout" | ||
| target: "GET/PATCH/DELETE /api/v1/model_timeouts(/{model}) and a Settings-view panel in admin.py all round-trip" | ||
| source: "tests/test_model_timeouts.py" | ||
| - metric: "an override changes real outbound call behavior, not just stored config" | ||
| target: "ModelClient.chat/stream_chat pass the resolved per-model timeout into _send_with_retry/_stream_send -> _open_provider" | ||
| source: "tests/test_model_timeouts.py::test_chat_call_uses_the_resolved_per_model_timeout, ::test_stream_chat_uses_the_resolved_per_model_timeout" | ||
| - metric: "no override leaves every existing caller's exact prior call shape unchanged" | ||
| target: "the resolver wired into ModelClient returns None (not the resolved default) absent an override, so call sites omit the timeout kwarg entirely in the common case" | ||
| source: "tests/test_model_timeouts.py::test_wired_resolver_returns_none_for_a_model_with_no_override, ::test_bare_model_client_with_no_resolver_passes_no_timeout_override" | ||
| --- |
There was a problem hiding this comment.
🔍 Research grounding is absent
This substantive feature adds no paper, citation, link, or redistribution note. Repository governance requires research grounding for feature PRs.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _known_model_names(self) -> set[str]: | ||
| """Return the distinct provider model identifiers configured on any agent.""" | ||
| return {agent.model for agent in self.candidates} |
There was a problem hiding this comment.
📝 Info: Shared model names share overrides
Overrides key on agent.model, so every provider connection exposing one model identifier inherits the same setting. This matches the per-model API contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def list_model_timeouts(self) -> list[dict[str, Any]]: | ||
| """Return every configured model's effective timeout and override status.""" | ||
| names = sorted(self._known_model_names() | set(self._model_timeout_overrides)) | ||
| return [self._model_timeout_payload(name) for name in names] | ||
|
|
||
| def get_model_timeout(self, model: str) -> dict[str, Any]: | ||
| """Return one model's timeout detail plus its recent set/clear audit history. | ||
|
|
||
| Raises ``KeyError`` when ``model`` matches no configured agent and has | ||
| no (now-orphaned) override on record. | ||
| """ | ||
| if model not in self._known_model_names() and model not in self._model_timeout_overrides: | ||
| raise KeyError(model) | ||
| return self._model_timeout_payload(model, with_audit=True) |
There was a problem hiding this comment.
📝 Info: Orphaned overrides stay manageable
Removed models remain listed while an override exists. Operators can inspect and clear stale state, while new settings still require a configured model.
Was this helpful? React with 👍 or 👎 to provide feedback.
…anel
Live-browser verification of the model-timeout panel from the prior commit
(stood up the dev server, drove real Save/Restore/error/bilingual/mobile
flows against real API responses -- not just contract-test assertions)
surfaced two real bugs neither test_admin_contract.py nor
test_model_timeouts.py could catch, since both assert markup/behavior, not
actual rendering:
- "Restore default" had no visual difference between disabled and enabled
-- .btn:disabled had no CSS anywhere in admin.py; this panel is the first
thing in the console to ever set `disabled` on a .btn. Added
`.btn:disabled { opacity: .5; cursor: not-allowed; filter: none; }`.
- The shared Audit tab did not reflect new model_timeout_set/
model_timeout_cleared events without a full page reload. Verified
server-side the events *were* recorded correctly
(curl /admin/state showed them); state.recent_audit_events was only ever
populated by load()'s initial fetch. Added refreshAuditEvents() (re-fetch
/admin/state, re-render the Audit table), called after a successful
save/clear.
Scoped to this panel's own actions only -- model_groups' save/delete has
the identical staleness, but that is a pre-existing, wider pattern this
PR did not introduce and fixing it broadly is out of scope here.
Also verified live: bilingual (en/ko) rendering with no raw i18n keys,
localized aria-label, validation-error feedback, and mobile (375px) layout
-- the panel's table scrolls within its own .panel (overflow-x:auto from
the shared stylesheet) rather than the page, and every control stays
reachable and the fixed disabled-state styling stays visible at that width.
tests/test_admin_contract.py + tests/test_model_timeouts.py: 31/31 passed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Live-render verification (not just contract-test inspection)Stood up Verified against this org's UX checklist:
Two real bugs found in the live render (contract tests didn't catch either) — fixed in this same commit
Scoped this fix to the model-timeout panel's own save/clear actions only — the identical staleness exists for Not fixed, noted as pre-existing and out of scope: the feedback Full suite still green after both fixes ( |
| if (!response.ok) throw new Error(payload.error?.message || "Could not save model timeout"); | ||
| els.modelTimeoutFeedback.textContent = t("model_timeout_saved"); | ||
| await refreshModelTimeouts(); | ||
| await refreshAuditEvents(); |
There was a problem hiding this comment.
🟡 Successful timeout changes appear failed
When refreshAuditEvents fails after a timeout update succeeds, the shared handler replaces the success message with an error. Operators can repeat an applied change.
Prompt for agents
Treat the audit-panel refresh as best-effort after a successful timeout mutation. In contextual_orchestrator/admin.py, saveModelTimeout and clearModelTimeout currently let refreshAuditEvents failures reject the whole action after the server has committed it. Preserve the success result and message when the follow-up /admin/state fetch or JSON parsing fails, while optionally showing a separate audit-refresh warning.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const response = await apiFetch("/admin/state"); | ||
| if (!response.ok) return; | ||
| const payload = await response.json(); | ||
| state.recent_audit_events = payload.recent_audit_events || []; |
There was a problem hiding this comment.
🟡 Concurrent edits stale audit history
Overlapping timeout actions let refreshAuditEvents apply responses out of order. An older response can hide newer audit events until the next reload.
Prompt for agents
Prevent stale /admin/state responses from overwriting newer audit history in contextual_orchestrator/admin.py. Multiple saveModelTimeout or clearModelTimeout calls can overlap, and refreshAuditEvents currently applies every response. Serialize these refreshes, cancel superseded requests, or track a monotonically increasing request generation and apply only the newest response.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added
documentation
ChatGPT Codex Connector
|
No-heuristics RCA / supersession: this PR is not safe to land under the current routing contract. Its core serving behavior makes an operator-authored per-model wall-clock value a test-time-compute allocation, and I also re-fetched the current review evidence before acting. The branch still has substantive implementation findings (local queue path ignores the override, passthrough/tool requests bypass it, failed persistence can leave the live timeout mutated, and admin refresh races can misreport/stale audit state). Those findings are not being suppressed or bypassed. Closing/superseding this PR preserves the branch's independently useful admin/persistence/API exploration for a future evidence-backed allocator, but the current manual timeout-setting semantics must not become production authority. Do not delete the global/provider integration or reinterpret |
|
Repair-policy recheck of this closure (2026-09-02). Per the org's repair-not-close policy ("close is reserved for: explicit user File-level fact, independently re-verified against a fresh clone ( But the closure is still valid, independent of that branch, because it
That is a categorical objection to this PR's live-enforcement wiring becoming Delta is preserved, not orphaned. Nothing here is deleted: this PR's two Recorded in 🤖 Generated with Claude Code |
…yvault) (#1675) * docs(adr): record ecosystem admin-web architecture (Keyverse SSO + Keyvault) Cross-repo research pass (owner request: "관리자 웹 개발 (noema, contextual-orchestrator, keyverse) 및 상호 연계 준비") across all three named repos, cloned fresh -- not assumed -- before any design work. Records: Keyverse as the shared SSO provider for every admin web (design only, not yet wired); each repo's admin web as a thin frontend over its own backend (no shared cross-repo frontend package, matching contextual-orchestrator's own ADR 0033 reasoning); the Keyverse-as-Keyvault bounded-context decision and why service ABAC/RBAC and "login credential store" are NOT rebuilt from scratch (PR #103 already covers the former; the latter is Keyvault + per-service Anti-Corruption Layers, not a new module); and why noema got no code change this iteration (no admin-relevant HTTP surface exists yet to build a console on). Points to the two implemented slices from this same pass: ContextualWisdomLab/contextual-orchestrator#1010 (per-model LLM timeout admin surface, closing docs/product-goal-directive.md §8) and ContextualWisdomLab/keyverse#129 (Keyvault: namespaced encrypted-at-rest secrets store, plus ADRs 0014-0016 for the three-capability Keyverse research). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(adr-0021): correct stale claim that contextual-orchestrator#1010 shipped PR #1010 (the ADR's decision item 6, the timeout-admin-surface slice) was opened at 03:40:12Z, this ADR PR at 03:40:12Z, and #1010 was subsequently closed unmerged by the repo owner at 05:10:46Z the same day on a categorical objection to its live-enforcement wiring becoming production authority, plus four distinct unresolved correctness findings -- already repair-policy rechecked and confirmed a valid closure with delta preserved, not orphaned. Adds an Update section rather than rewriting the original decision record, so the ADR doesn't merge into main citing a closed PR as an implemented slice. Decisions 1-5 (SSO/Keyvault/ABAC-RBAC/credential-store shape) are unaffected; only item 6's implementation claim was stale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(adr): renumber ADR-0021 to ADR-0026 to resolve a numbering collision docs/adr/0021-hourly-review-repair-single-file-consolidation.md landed on main after this PR branched, so this ADR's own "0021 is the next free number" claim went stale. 0026 is the next free number after the current highest (0025, the CodeQL dispatch ADR). Renamed the file and updated its own title heading; no other file in the repo references the old number or filename. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Resolves the admin.py conflict between this branch's own audit-refresh fix (item 25: re-fetch /admin/state after a model_groups save/delete, matching PR #1010's fix for the sibling model-timeouts panel) and main's independently-landed, more complete fix for the exact same bug. Verified main's refreshModelGroupViews()/refreshAuditEvents() is a strict superset of this branch's own fix: it refreshes both model groups and audit events with per-call try/catch and a user-visible warning on partial failure (this branch's sequential-await version had no such handling -- a refresh failure would have overwritten the "saved successfully" message with a misleading error), and it also fixes the color-coding gap this branch's own PR description explicitly left as a separate unfixed item (main sets .style.color; this branch's CSS-class-based setModelGroupFeedback() helper was a working alternative, but tests/test_admin_contract.py's executable Node behavioral test asserts els.modelGroupFeedback.style.color directly, so main's mechanism is what's actually load-bearing here). Kept main's implementation for all three admin.py conflict blocks, removed the now-dead setModelGroupFeedback() helper and its .feedback-success/.feedback-error CSS rules (zero remaining callers), and updated the 9 substring assertions in the non-behavioral half of test_admin_contract.py's test_admin_surface_exists_for_enterprise_operations that referenced the removed helper/classes to assert main's actual mechanism instead. The deeper, executable test_model_group_mutations_refresh_audit_events (unconflicted, already covers the exact behavior) needed no changes. Full suite: 3391 passed, 1 skipped. admin.py 100% coverage, interrogate 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Closes the standing admin-web requirement recorded in
ContextualWisdomLab/.github'sdocs/product-goal-directive.md§8: an admin web where operators can view/set/clear/restore per-model LLM timeouts, with units, priority/inheritance, input validation, audit history, and an API contract.Research first (cloned fresh, no assumptions): no per-model timeout config existed anywhere in this repo --
ModelClient.timeoutwas one flat instance value (default 90s) applied to every outbound call regardless of model./admin(admin.py) is already a real, serving operator console (not a stub);admin_ui/(React+Storybook) is confirmed still the unmodified Vite scaffold, matching this repo's own superseded planning ADR 0036 -- so this PR extendsadmin.pyin place per the operative ADR 0033, and does not touchadmin_ui/.TaskOrchestrator.list_model_timeouts/get_model_timeout/set_model_timeout/clear_model_timeout-- follows the existingmodel_groupfamily's exact pattern: KV-persisted (_StateStore's new"model_timeout_override"keyed kind + a new.delete()method for clear/restore), audited via the existing_append_audit_event,KeyError/ValueErroron unknown model / invalid value.MIN_MODEL_TIMEOUT_SECONDS=1/MAX_MODEL_TIMEOUT_SECONDS=14400-- the 14400s ceiling matches this org's own already-evidencedNOEMA_LLM_TIMEOUT_SECONDSprecedent, not an invented number.GET /api/v1/model_timeouts(/{model}),PATCH/DELETE /api/v1/model_timeouts/{model}(PATCH, not PUT -- matchesmodel_groups' existing convention; there is nodo_PUThandler in this repo). Declared inapi_contract.py.ModelClient.model_timeout_resolveris wired to a resolver that returnsNoneabsent an override (not the resolved default), sochat()/stream_chat()omit thetimeoutkwarg entirely in the common case -- byte-identical to every existing caller's prior behavior -- and only pass an explicit override when an operator actually set one. Verified with a fake transport capturing the realtimeout=reaching_open_provideron both the chat and streaming-chat paths.admin.py's Settings view (bilingual en/ko), mirroring themodel_groupspanel's render/refresh/save pattern exactly. Purely additive -- no existing line changed, sotest_admin_contract.py's 100+ exact-string assertions keep passing unmodified.ModelClient._send_with_retry/_stream_sendmock stand-ins. Fixed by correcting the resolver's semantics (returnNoneabsent an override) -- the smaller, more correct fix, not patching every test double.Test plan
python3 -m pytest tests -q-- 3336 passed, 1 skipped (baseline before this PR: 3307 passed, 1 skipped)python3 -m pytest tests/test_model_timeouts.py -q-- 29 new tests (validation, inheritance, audit history, persistence-across-restart, realModelClient-level timeout wiring for both chat and streaming, HTTP CRUD + auth, admin-console UI-surface assertions)interrogate -von every touched module -- 100% docstring coveragetest_api_contract.py,test_admin_contract.py,test_conventions.py,test_planning_adr_identifiers.py-- all green, unmodified assertions still passWhat's left for the next iteration
route()/conduct()path this requirement is about) are wired; extending the same resolver to the remainingModelClienttransport methods is straightforward but left for a follow-up./admin-- a real cross-repo integration (seeContextualWisdomLab/.githubADR-0021 and the siblingkeyverseKeyvault PR from this same research pass), designed but not built in this PR.KeyverseCredentialBackendimplementing this repo's existingCredentialBackendProtocol, perkeyversePR's ADR-0016 -- noted as the natural next consumer ofkeyverse's new Keyvault, not started here.🤖 Generated with Claude Code