Repository navigation
feat(routing): LGKP remembers last good account per provider - #2338
diegosouzapw merged 4 commits into
Conversation
Previously LGKP only tracked which provider last succeeded. Now it also
tracks the specific connection/account ID, so the same account is reused
on the next request instead of falling back to provider-level matching.
Changes:
- settings.ts: getLKGP returns LKGPRecord { provider, connectionId },
setLKGP accepts optional connectionId, backward-compatible parsing
- readCache.ts: cache layer updated for LKGPRecord shape
- combo.ts: both write paths (handleComboChat + handleRoundRobinCombo)
pass target.connectionId to setLKGP; both read paths (lkgp strategy
+ auto strategy) extract connectionId and match on it first
- routerStrategy.ts: RoutingContext gains lastKnownGoodConnectionId,
LKGPStrategyImpl includes account info in decision reason
Closes cheaper-inference#2262
Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces account-level tracking for the Last Known Good Provider (LKGP) by incorporating a connectionId into the LKGP records, database schema, and routing context. While the logic in combo.ts correctly prioritizes matches using both provider and connection ID, the implementation in LKGPStrategyImpl within routerStrategy.ts currently only filters by provider and defaults to the first available candidate. This oversight prevents true account-level prioritization in that strategy and renders the updated decision reason string misleading, as it may display an account ID that was not strictly enforced during selection.
| const candidates = pool.filter( | ||
| (c) => c.provider === context.lastKnownGoodProvider && c.circuitBreakerState !== "OPEN" | ||
| ); | ||
| if (best) { | ||
| if (candidates.length > 0) { | ||
| const best = candidates[0]; |
There was a problem hiding this comment.
The LKGPStrategyImpl does not actually prioritize the specific account identified by lastKnownGoodConnectionId. It filters the pool by provider and simply selects the first available candidate (candidates[0]). If multiple accounts exist for the same provider, this logic will not necessarily pick the one that previously succeeded, which contradicts the PR's goal of account-level LGKP for the auto strategy. To fix this, ProviderCandidate should include the connectionId (populated in buildAutoCandidates), and this logic should attempt to find a matching candidate before defaulting to the first one in the pool.
| model: best.model, | ||
| strategy: this.name, | ||
| reason: `LKGP: using last known good provider ${best.provider}`, | ||
| reason: `LKGP: using last known good provider ${best.provider}${context.lastKnownGoodConnectionId ? ` (account ${context.lastKnownGoodConnectionId})` : ""}`, |
There was a problem hiding this comment.
The decision reason string is misleading because it includes the account ID even though the selection logic above (line 138) doesn't actually guarantee that the specific account was chosen. The selection logic should be updated to match the account ID before this reason string can be considered accurate.
Address Gemini code review: auto strategy candidates are provider-level only (ProviderCandidate has no connectionId), so passing connectionId through RoutingContext was misleading. Account-level matching only works in the standalone lkgp strategy path which uses ResolvedComboTarget. Co-Authored-By: OpenClaude (mimo-v2.5-pro) <openclaude@gitlawb.com>
|
@ gemini-code-assist Good catch. Fixed in 81b6b40:
Account-level matching is correctly handled only in the standalone |
|
Thank you for the update, @oyi77. These changes look correct and improve the maintainability of the routing logic by removing the unused variable and cleaning up the |
193e7a6
into
cheaper-inference:release/v3.8.0
|
Thank you @oyi77 for this improvement! Your LKGP enhancement — storing the last known good account (connectionId) alongside the provider — has been merged into release/v3.8.0. We added unit tests for the new connectionId-aware getLKGP/setLKGP signatures and backward-compat parsing of old plain-string values. This will ship in the upcoming v3.8.0 release. Great contribution! |
…th (#2359) Combo dispatch and the combo test button used to crash with 'TypeError: e.startsWith is not a function' when a step's modelStr failed to resolve (regression after #2338 added per-account LKGP routing for local/Docker providers). The TypeScript annotation says the field is always a string, but malformed combo rows leaked through to the dispatch path. Two defensive boundary guards: 1. open-sse/services/combo.ts LKGP fallback findIndex — type-check target.modelStr before calling startsWith. 2. src/app/api/combos/test/route.ts testComboTarget — coerce target.modelStr at the entry, surface a clean 'Combo step is missing a model id' error instead of crashing. Regression test parses the source and asserts no unguarded target.modelStr.<string-method> usages remain in combo.ts, so a future refactor that reintroduces the pattern fails loudly.
…-inference#2338) Integrated into release/v3.8.0 — added unit tests for connectionId-aware getLKGP/setLKGP
…th (cheaper-inference#2359) Combo dispatch and the combo test button used to crash with 'TypeError: e.startsWith is not a function' when a step's modelStr failed to resolve (regression after cheaper-inference#2338 added per-account LKGP routing for local/Docker providers). The TypeScript annotation says the field is always a string, but malformed combo rows leaked through to the dispatch path. Two defensive boundary guards: 1. open-sse/services/combo.ts LKGP fallback findIndex — type-check target.modelStr before calling startsWith. 2. src/app/api/combos/test/route.ts testComboTarget — coerce target.modelStr at the entry, surface a clean 'Combo step is missing a model id' error instead of crashing. Regression test parses the source and asserts no unguarded target.modelStr.<string-method> usages remain in combo.ts, so a future refactor that reintroduces the pattern fails loudly.
…-inference#2338) Integrated into release/v3.8.0 — added unit tests for connectionId-aware getLKGP/setLKGP
…th (cheaper-inference#2359) Combo dispatch and the combo test button used to crash with 'TypeError: e.startsWith is not a function' when a step's modelStr failed to resolve (regression after cheaper-inference#2338 added per-account LKGP routing for local/Docker providers). The TypeScript annotation says the field is always a string, but malformed combo rows leaked through to the dispatch path. Two defensive boundary guards: 1. open-sse/services/combo.ts LKGP fallback findIndex — type-check target.modelStr before calling startsWith. 2. src/app/api/combos/test/route.ts testComboTarget — coerce target.modelStr at the entry, surface a clean 'Combo step is missing a model id' error instead of crashing. Regression test parses the source and asserts no unguarded target.modelStr.<string-method> usages remain in combo.ts, so a future refactor that reintroduces the pattern fails loudly.
…-inference#2338) Integrated into release/v3.8.0 — added unit tests for connectionId-aware getLKGP/setLKGP
…th (cheaper-inference#2359) Combo dispatch and the combo test button used to crash with 'TypeError: e.startsWith is not a function' when a step's modelStr failed to resolve (regression after cheaper-inference#2338 added per-account LKGP routing for local/Docker providers). The TypeScript annotation says the field is always a string, but malformed combo rows leaked through to the dispatch path. Two defensive boundary guards: 1. open-sse/services/combo.ts LKGP fallback findIndex — type-check target.modelStr before calling startsWith. 2. src/app/api/combos/test/route.ts testComboTarget — coerce target.modelStr at the entry, surface a clean 'Combo step is missing a model id' error instead of crashing. Regression test parses the source and asserts no unguarded target.modelStr.<string-method> usages remain in combo.ts, so a future refactor that reintroduces the pattern fails loudly.
Summary
{ provider, connectionId }shape on readChanges
src/lib/db/settings.tsgetLKGP()returnsLKGPRecord { provider, connectionId? },setLKGP()accepts optionalconnectionId, backward-compatible parsing of old plain-string valuessrc/lib/db/readCache.tsLKGPRecordshape,setCachedLKGP()passes throughconnectionIdopen-sse/services/combo.tshandleComboChat+handleRoundRobinCombo) passtarget.connectionIdtosetLKGP(); both read paths (lkgpstrategy +autostrategy) extractconnectionIdfrom the record and match on it first before falling back to provider-levelopen-sse/services/autoCombo/routerStrategy.tsRoutingContextgainslastKnownGoodConnectionId?,LKGPStrategyImpllogs account info in decision reasonHow it works
Before: LGKP stored
"openai"→ next request tries allopenaiaccounts in order until one works.After: LGKP stores
{ provider: "openai", connectionId: "conn-abc123" }→ next request prioritizesconn-abc123directly, skipping accounts that previously failed.Test plan
tsc --noEmit)db-settings-crud,db-read-cache,combo-routing-engine)Related Issues
Closes #2262
🤖 Generated with OpenClaude