Repository navigation
fix(combo): clear LKGP pin when its target fails, not only set it on success - #10034
Conversation
d635c55 to
54f491e
Compare
54f491e to
0fdb6b0
Compare
…success
setLKGP() was only ever called on success — nothing invalidated a "last
known good provider" pin once that provider started failing, so a
*separate* subsequent request kept re-selecting the same just-failed
target via applyStrategyOrdering.ts's LKGP reordering.
Live incident: an OpenClaw request to combo "default" (routerStrategy:
lkgp) got a real reasoning + apply_patch tool call from
opencode-zen/big-pickle, then 3 separate follow-up requests over the
next ~2 minutes each independently re-selected the same big-pickle
target and each timed out with "504 Stream produced no non-ping SSE
event within 95000ms" before the client gave up — instead of failing
over to any of the combo's other 12 models.
Root cause confirmed via code read: circuit breaker and model lockout
deliberately don't react to this failure class (isStreamReadinessFailureErrorBody
exempts STREAM_READINESS_TIMEOUT/combo_target_timeout 504s from tripping
the provider breaker, and REQUEST_SCOPED_UPSTREAM_ERROR_CODES suppresses
model-lockout recording for the same class — both intentional, to avoid
poisoning a healthy provider on request-specific timing). Nothing else
in the system was clearing the stale LKGP pin, so it kept winning
target-selection ordering for every new top-level request.
Fix: add clearLKGP(comboName, modelId) to src/lib/db/settings/lkgp.ts,
export it through settings.ts/localDb.ts, and call it (mirroring the
existing setLKGP-on-success call pattern exactly, same two keys) in both
combo.ts's per-target failure paths -- handleComboChat's "Done retrying
this model" block and handleRoundRobinCombo's structurally identical
twin -- right where a target is finally given up on and the loop moves
to the next one.
TDD: new regression test in tests/unit/combo-routing-engine.test.ts
("clears LKGP after the last-known-good target fails") reproduces the
exact live scenario -- confirmed failing against the pre-fix code,
passing after. Added direct unit coverage for clearLKGP itself in
tests/unit/db-settings-crud.test.ts (deletes only the targeted key,
sibling keys survive; no-op on an unset key doesn't throw) and
registered the new export in db-settings-split.test.ts's public API
surface characterization test.
Test plan:
- Full combo/LKGP-related suite (combo-routing-engine, db-settings-crud,
db-settings-split, combo-strategy-fallbacks,
combo-selected-connection-success,
delete-provider-connection-invalidates-lkgp-8887, db-read-cache) --
183/183 passing.
- npx tsc --noEmit -- clean for all changed files (pre-existing unrelated
errors elsewhere in the same test files confirmed identical against a
pristine upstream/release/v3.8.50 checkout, zero diff at those lines).
- npm run lint -- clean (new test's any usage properly typed, not left
to inflate the file's frozen any-budget suppression).
⚠️ base-red inherited: diegosouzapw#9985
0fdb6b0 to
e5cc749
Compare
|
Verified via TDD in an isolated probe: reverted combo.ts/settings.ts/settings/lkgp.ts/localDb.ts to base and confirmed the new regression test in combo-routing-engine.test.ts fails cleanly (LKGP pin stays set after the target's only retry fails); restored the PR's changes and the full combo/LKGP-related suite passes (88+53+42 tests across the touched and sibling files). clearLKGP() follows the existing db/settings domain-module and re-export conventions exactly, and the two call sites in combo.ts mirror the pre-existing setLKGP-on-success pattern (same fire-and-forget shape, same two keys, same non-fatal error handling). eslint (with the frozen suppressions) and typecheck:core are clean on all touched files. One minor, non-blocking observation: the Note: the base-red that was failing this PR's CI (#9985) was drained today — base-reds PR #10213 just merged into |
|
Merged — thanks @hartmark! LKGP pins now clear on failure; the new settings module follows the db domain-module pattern cleanly. |
…success (diegosouzapw#10034) setLKGP() was only ever called on success — nothing invalidated a "last known good provider" pin once that provider started failing, so a *separate* subsequent request kept re-selecting the same just-failed target via applyStrategyOrdering.ts's LKGP reordering. Live incident: an OpenClaw request to combo "default" (routerStrategy: lkgp) got a real reasoning + apply_patch tool call from opencode-zen/big-pickle, then 3 separate follow-up requests over the next ~2 minutes each independently re-selected the same big-pickle target and each timed out with "504 Stream produced no non-ping SSE event within 95000ms" before the client gave up — instead of failing over to any of the combo's other 12 models. Root cause confirmed via code read: circuit breaker and model lockout deliberately don't react to this failure class (isStreamReadinessFailureErrorBody exempts STREAM_READINESS_TIMEOUT/combo_target_timeout 504s from tripping the provider breaker, and REQUEST_SCOPED_UPSTREAM_ERROR_CODES suppresses model-lockout recording for the same class — both intentional, to avoid poisoning a healthy provider on request-specific timing). Nothing else in the system was clearing the stale LKGP pin, so it kept winning target-selection ordering for every new top-level request. Fix: add clearLKGP(comboName, modelId) to src/lib/db/settings/lkgp.ts, export it through settings.ts/localDb.ts, and call it (mirroring the existing setLKGP-on-success call pattern exactly, same two keys) in both combo.ts's per-target failure paths -- handleComboChat's "Done retrying this model" block and handleRoundRobinCombo's structurally identical twin -- right where a target is finally given up on and the loop moves to the next one. TDD: new regression test in tests/unit/combo-routing-engine.test.ts ("clears LKGP after the last-known-good target fails") reproduces the exact live scenario -- confirmed failing against the pre-fix code, passing after. Added direct unit coverage for clearLKGP itself in tests/unit/db-settings-crud.test.ts (deletes only the targeted key, sibling keys survive; no-op on an unset key doesn't throw) and registered the new export in db-settings-split.test.ts's public API surface characterization test. Test plan: - Full combo/LKGP-related suite (combo-routing-engine, db-settings-crud, db-settings-split, combo-strategy-fallbacks, combo-selected-connection-success, delete-provider-connection-invalidates-lkgp-8887, db-read-cache) -- 183/183 passing. - npx tsc --noEmit -- clean for all changed files (pre-existing unrelated errors elsewhere in the same test files confirmed identical against a pristine upstream/release/v3.8.50 checkout, zero diff at those lines). - npm run lint -- clean (new test's any usage properly typed, not left to inflate the file's frozen any-budget suppression).⚠️ base-red inherited: diegosouzapw#9985 (cherry picked from commit c9daf99)
…success (diegosouzapw#10034) setLKGP() was only ever called on success — nothing invalidated a "last known good provider" pin once that provider started failing, so a *separate* subsequent request kept re-selecting the same just-failed target via applyStrategyOrdering.ts's LKGP reordering. Live incident: an OpenClaw request to combo "default" (routerStrategy: lkgp) got a real reasoning + apply_patch tool call from opencode-zen/big-pickle, then 3 separate follow-up requests over the next ~2 minutes each independently re-selected the same big-pickle target and each timed out with "504 Stream produced no non-ping SSE event within 95000ms" before the client gave up — instead of failing over to any of the combo's other 12 models. Root cause confirmed via code read: circuit breaker and model lockout deliberately don't react to this failure class (isStreamReadinessFailureErrorBody exempts STREAM_READINESS_TIMEOUT/combo_target_timeout 504s from tripping the provider breaker, and REQUEST_SCOPED_UPSTREAM_ERROR_CODES suppresses model-lockout recording for the same class — both intentional, to avoid poisoning a healthy provider on request-specific timing). Nothing else in the system was clearing the stale LKGP pin, so it kept winning target-selection ordering for every new top-level request. Fix: add clearLKGP(comboName, modelId) to src/lib/db/settings/lkgp.ts, export it through settings.ts/localDb.ts, and call it (mirroring the existing setLKGP-on-success call pattern exactly, same two keys) in both combo.ts's per-target failure paths -- handleComboChat's "Done retrying this model" block and handleRoundRobinCombo's structurally identical twin -- right where a target is finally given up on and the loop moves to the next one. TDD: new regression test in tests/unit/combo-routing-engine.test.ts ("clears LKGP after the last-known-good target fails") reproduces the exact live scenario -- confirmed failing against the pre-fix code, passing after. Added direct unit coverage for clearLKGP itself in tests/unit/db-settings-crud.test.ts (deletes only the targeted key, sibling keys survive; no-op on an unset key doesn't throw) and registered the new export in db-settings-split.test.ts's public API surface characterization test. Test plan: - Full combo/LKGP-related suite (combo-routing-engine, db-settings-crud, db-settings-split, combo-strategy-fallbacks, combo-selected-connection-success, delete-provider-connection-invalidates-lkgp-8887, db-read-cache) -- 183/183 passing. - npx tsc --noEmit -- clean for all changed files (pre-existing unrelated errors elsewhere in the same test files confirmed identical against a pristine upstream/release/v3.8.50 checkout, zero diff at those lines). - npm run lint -- clean (new test's any usage properly typed, not left to inflate the file's frozen any-budget suppression).⚠️ base-red inherited: diegosouzapw#9985
Summary
setLKGP()(Last Known Good Provider,src/lib/db/settings/lkgp.ts) was onlyever called on success — nothing invalidated a pin once that provider started
failing, so a separate subsequent request kept re-selecting the same
just-failed target via
applyStrategyOrdering.ts's LKGP reordering.Live incident: an OpenClaw request to combo
default(
routerStrategy: lkgp) got a real reasoning +apply_patchtool call fromopencode-zen/big-pickle. Three separate follow-up requests over the next~2 minutes each independently re-selected the same
big-pickletarget andeach timed out with
504 Stream produced no non-ping SSE event within 95000msbefore the client gave up — instead of failing over to any of thecombo's other 12 models.
Root cause: confirmed via code read that circuit breaker and model
lockout deliberately don't react to this failure class —
isStreamReadinessFailureErrorBodyexemptsSTREAM_READINESS_TIMEOUT/combo_target_timeout504s from tripping the provider breaker, andREQUEST_SCOPED_UPSTREAM_ERROR_CODESsuppresses model-lockout recording forthe same class (both intentional — avoid poisoning a healthy provider on
request-specific timing). Nothing else in the system was clearing the stale
LKGP pin, so it kept winning target-selection ordering for every new
top-level request.
Fix
clearLKGP(comboName, modelId)tosrc/lib/db/settings/lkgp.ts,exported through
settings.ts/localDb.ts.setLKGP-on-success call pattern exactly,same two keys —
target.executionKeyandcombo.id || combo.name) inboth of
combo.ts's per-target failure paths:handleComboChat's "Doneretrying this model" block and
handleRoundRobinCombo's structurallyidentical twin — right where a target is finally given up on and the loop
moves to the next one.
Test plan
tests/unit/combo-routing-engine.test.ts("clears LKGP after the last-known-good target fails") reproduces the
exact live scenario — confirmed failing against the pre-fix code, passing
after.
clearLKGPitself intests/unit/db-settings-crud.test.ts(deletes only the targeted key,sibling keys survive; no-op on an unset key doesn't throw) and registered
the new export in
db-settings-split.test.ts's public API surfacecharacterization test.
combo-routing-engine,db-settings-crud,db-settings-split,combo-strategy-fallbacks,combo-selected-connection-success,delete-provider-connection-invalidates-lkgp-8887,db-read-cache) —183/183 passing.
npx tsc --noEmit— clean for all changed files (pre-existing unrelatederrors elsewhere in the same test files confirmed identical against a
pristine
upstream/release/v3.8.50checkout, zero diff at those lines).npm run lint— clean (new test'sanyusage properly typed instead ofinflating the file's frozen
any-budget suppression).