Skip to content

Commit d6b6c89

Browse files
authored
feat(chat): wire chat role into ProviderResolver.resolve_with_fallback (#55) (#75)
The failover resolver (PR-D) had no production caller: chat.py still picked the first enabled provider directly, so model_defaults.candidates order and cooldown were dead UI surface. - chat endpoint: when the chat role has model_defaults.candidates, run through resolve_with_fallback() — connection-level failures (connect error/timeout/5xx, decision #7) fail over to the next candidate, business failures (4xx) re-raise immediately. Explicit provider_id and roles with no candidates keep the legacy single-provider path unchanged. - tool loops (JSON + XML) now raise LlmAdapterError with classify_retryable instead of a bare 502, so the resolver can distinguish failover-worthy failures; 502 conversion stays at the endpoint boundary. - resolver: skip disabled providers in resolve()/resolve_with_fallback() (governance choice, not liveness failure — no cooldown), add public has_candidates() for the legacy fallback branch. - tests: unit coverage for disabled-skip + has_candidates; new tests/integration/test_chat_failover_api.py exercises the production chain through POST /api/v1/chat (legacy path, retryable failover, business-error no-failover, all-unavailable 502, disabled-skip, explicit-provider bypass). - fix latent patch leak in test_resolver concurrency test (per-task unittest.mock.patch around an await races under asyncio.gather and leaked a get_adapter mock into later tests).
1 parent 9092fa5 commit d6b6c89

4 files changed

Lines changed: 490 additions & 21 deletions

File tree

backend/api/v1/chat.py

Lines changed: 84 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323

2424
from backend.control.agent_control import ACTION_REGISTRY, agent_control_service
2525
from backend.database import get_db
26+
from backend.llm import ResolverError, resolver
27+
from backend.llm.base import LlmAdapterError, classify_retryable
2628
from backend.models.provider import ModelProvider
2729
from backend.schemas.common import ApiResponse
2830
from backend.security.identity import RequestIdentity, get_request_identity
@@ -341,16 +343,25 @@ async def _build_proposal(
341343
)
342344

343345

344-
@router.post("", response_model=ApiResponse[ChatReply])
345-
async def chat(
346+
async def _chat_with_client(
347+
client: Any,
348+
model: str,
346349
body: ChatRequest,
347-
identity: RequestIdentity | None = Depends(_optional_request_identity),
348-
db: AsyncSession = Depends(get_db),
350+
db: AsyncSession,
351+
identity: RequestIdentity | None,
349352
) -> ApiResponse:
350-
provider = await _pick_provider(db, body.provider_id)
351-
client = await _build_client(provider)
352-
model = provider.default_model or "gpt-4o-mini"
353-
353+
"""Run the agent-dock tool loop against ``client`` with ``model``.
354+
355+
Extracted from the ``chat`` endpoint so the same loop serves both the
356+
legacy single-provider path and each candidate tried inside
357+
``ProviderResolver.resolve_with_fallback`` (model-provider runtime
358+
PR-E, issue #55). LLM-call failures are re-raised as
359+
:class:`~backend.llm.base.LlmAdapterError` carrying
360+
:func:`~backend.llm.base.classify_retryable`'s connection-vs-business
361+
split (decision #7) — the resolver only fails over connection-level
362+
failures, and the HTTP 502 conversion happens once at the endpoint
363+
boundary instead of being baked into the loop.
364+
"""
354365
system = SYSTEM_PROMPT
355366
if body.context:
356367
system += f"\n\n当前用户操作上下文 (JSON): {json.dumps(body.context, ensure_ascii=False)}"
@@ -366,9 +377,13 @@ async def chat(
366377
response = await client.chat.completions.create(
367378
model=model, messages=messages, tools=TOOLS, tool_choice="auto"
368379
)
380+
except LlmAdapterError:
381+
raise
369382
except Exception as exc:
370383
logger.error("chat llm error | %s", exc)
371-
raise HTTPException(status_code=502, detail=f"模型调用失败: {exc}") from exc
384+
raise LlmAdapterError(
385+
f"chat llm error: {exc}", retryable=classify_retryable(exc)
386+
) from exc
372387

373388
msg = response.choices[0].message
374389
tool_calls = msg.tool_calls or []
@@ -413,6 +428,61 @@ async def chat(
413428
return ApiResponse.ok(ChatReply(type="message", content="(达到工具调用步数上限, 请换个说法再试)"))
414429

415430

431+
async def _chat_single_provider(
432+
db: AsyncSession,
433+
body: ChatRequest,
434+
identity: RequestIdentity | None,
435+
provider_id: Optional[str],
436+
) -> ApiResponse:
437+
"""Legacy pre-failover chat path (issue #55): one provider — the
438+
explicit ``provider_id`` or the first enabled one — no candidate
439+
failover. Kept as the fallback when the ``chat`` role has no
440+
``model_defaults.candidates`` configured, so existing installs behave
441+
exactly as before the failover wiring.
442+
"""
443+
provider = await _pick_provider(db, provider_id)
444+
client = await _build_client(provider)
445+
model = provider.default_model or "gpt-4o-mini"
446+
return await _chat_with_client(client, model, body, db, identity)
447+
448+
449+
@router.post("", response_model=ApiResponse[ChatReply])
450+
async def chat(
451+
body: ChatRequest,
452+
identity: RequestIdentity | None = Depends(_optional_request_identity),
453+
db: AsyncSession = Depends(get_db),
454+
) -> ApiResponse:
455+
"""Agent dock chat (model-provider runtime PR-E, issue #55).
456+
457+
Provider selection now runs through
458+
:func:`backend.llm.resolver.ProviderResolver.resolve_with_fallback` for
459+
the ``chat`` role whenever ``model_defaults.candidates`` are configured
460+
for it: candidates are tried in order and a connection-level failure
461+
(connect error / timeout / 5xx — decision #7) fails over to the next
462+
candidate, while a business failure (4xx: bad key, malformed request)
463+
is re-raised immediately. An explicit ``provider_id`` still bypasses
464+
failover (the user picked that provider on purpose), and a role with no
465+
candidates falls back to the legacy first-enabled-provider lookup.
466+
"""
467+
if body.provider_id or not await resolver.has_candidates(db, "chat"):
468+
return await _chat_single_provider(db, body, identity, body.provider_id)
469+
470+
async def operation(adapter: Any, model_id: str) -> ApiResponse:
471+
provider = adapter.provider
472+
client = await _build_client(provider)
473+
model = model_id or provider.default_model or "gpt-4o-mini"
474+
return await _chat_with_client(client, model, body, db, identity)
475+
476+
try:
477+
return await resolver.resolve_with_fallback(db, "chat", operation)
478+
except (LlmAdapterError, ResolverError) as exc:
479+
# Business-level failure (re-raised immediately, no candidate left
480+
# untried) or every candidate skipped/failed → single 502 explaining
481+
# why; never leaks a provider api_key (adapters sanitize).
482+
logger.error("chat failover | %s", exc)
483+
raise HTTPException(status_code=502, detail=f"模型调用失败: {exc}") from exc
484+
485+
416486
@router.post("/confirm", response_model=ApiResponse[dict])
417487
async def confirm(
418488
body: ConfirmRequest,
@@ -502,9 +572,13 @@ async def _chat_xml(
502572
for _step in range(MAX_TOOL_STEPS):
503573
try:
504574
response = await client.chat.completions.create(model=model, messages=messages, max_tokens=1024)
575+
except LlmAdapterError:
576+
raise
505577
except Exception as exc:
506578
logger.error("chat(xml) llm error | %s", exc)
507-
raise HTTPException(status_code=502, detail=f"模型调用失败: {exc}") from exc
579+
raise LlmAdapterError(
580+
f"chat(xml) llm error: {exc}", retryable=classify_retryable(exc)
581+
) from exc
508582

509583
content = response.choices[0].message.content or ""
510584
calls = _parse_tool_use(content)

backend/llm/resolver.py

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -102,24 +102,37 @@ async def _candidates_for(self, db: AsyncSession, role: str) -> list[dict[str, A
102102
return []
103103
return list(row.candidates or [])
104104

105+
async def has_candidates(self, db: AsyncSession, role: str) -> bool:
106+
"""``True`` iff a non-empty ``model_defaults.candidates`` list exists
107+
for ``role``.
108+
109+
A cheap pre-check for callers that want to keep a legacy fallback
110+
path when the role has no failover order configured (e.g.
111+
``chat.py``: candidates configured → failover runtime; no candidates
112+
→ the pre-failover single-provider lookup, so existing installs
113+
keep working unchanged).
114+
"""
115+
return bool(await self._candidates_for(db, role))
116+
105117
async def resolve(self, db: AsyncSession, role: str) -> ResolvedModel | None:
106118
"""Return the primary (first) candidate configured for ``role``.
107119
108120
Returns ``None`` — rather than raising — when no ``model_defaults``
109-
row exists for ``role``, its ``candidates`` list is empty, or the
121+
row exists for ``role``, its ``candidates`` list is empty, the
110122
first candidate's ``provider_id`` no longer resolves to a real
111-
provider row: this is a direct "what's configured" lookup for
112-
callers that want the single default, not a "try until one works"
113-
search (that's :meth:`resolve_with_fallback`) — there is nothing to
114-
fail over to here, so a clean ``None`` is more useful to a caller
115-
than an exception for what is often just "nothing configured yet".
123+
provider row, or that provider is currently disabled: this is a
124+
direct "what's configured AND usable" lookup for callers that want
125+
the single default, not a "try until one works" search (that's
126+
:meth:`resolve_with_fallback`) — there is nothing to fail over to
127+
here, so a clean ``None`` is more useful to a caller than an
128+
exception for what is often just "nothing configured yet".
116129
"""
117130
candidates = await self._candidates_for(db, role)
118131
if not candidates:
119132
return None
120133
candidate = candidates[0]
121134
provider = await db.get(ModelProvider, candidate["provider_id"])
122-
if provider is None:
135+
if provider is None or not provider.enabled:
123136
return None
124137
return ResolvedModel(
125138
provider=provider,
@@ -140,7 +153,10 @@ async def resolve_with_fallback(
140153
- A candidate whose ``provider_id`` no longer resolves to a real
141154
provider row (deleted since the default was configured) is
142155
skipped the same way, without being put in cooldown (there is no
143-
"it" to cool down).
156+
"it" to cool down). A candidate whose provider row is currently
157+
``enabled=False`` is skipped identically — disabling a provider
158+
in governance is an operator choice, not a liveness failure, so
159+
it must not be used and must not go into cooldown either.
144160
- ``operation`` succeeding returns that result immediately — no
145161
further candidates are tried.
146162
- ``operation`` raising :class:`~backend.llm.base.LlmAdapterError`
@@ -170,6 +186,11 @@ async def resolve_with_fallback(
170186
# Candidate references a since-deleted provider — dead, but
171187
# not "this provider just failed", so no cooldown to set.
172188
continue
189+
if not provider.enabled:
190+
# Disabled in governance — an operator choice, not a
191+
# liveness failure: skip without cooldown, same as a
192+
# deleted provider.
193+
continue
173194
adapter = get_adapter(provider)
174195
tried += 1
175196
try:

0 commit comments

Comments
 (0)