Repository navigation
fix(fallback): use shared CircuitBreaker instead of undefined constants - #1485
Conversation
recordProviderFailure was referencing PROVIDER_FAILURE_WINDOW_MS, PROVIDER_FAILURE_THRESHOLD, and PROVIDER_COOLDOWN_MS which don't exist, causing runtime ReferenceError. Replace with direct delegation to the existing getCircuitBreaker utility which already handles failure counting, threshold detection, and state transitions. Remove redundant ProviderFailureEntry type, providerFailureState Map, and providerFailureLocks Set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.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 refactors the provider-level circuit breaker logic in open-sse/services/accountFallback.ts by replacing manual state management and re-entrancy locks with a shared CircuitBreaker utility. While this simplifies the code, the review identifies a regression where the removal of the failure window causes errors to accumulate indefinitely without a reset mechanism. Additionally, the implementation incorrectly uses connection-level configuration thresholds instead of the specific provider-level settings, leading to shorter cooldown periods than intended.
| }); | ||
| return; | ||
| } | ||
| breaker._onFailure(); |
There was a problem hiding this comment.
The migration to the shared CircuitBreaker utility introduces a regression in failure tracking logic. The original implementation included a 'failure window' (resetAfterMs) that reset the failure count if errors were sufficiently spread out (lines 552-562 in the previous version). The CircuitBreaker utility lacks this sliding window and only resets failureCount upon a successful execution or manual reset. Since this service does not appear to record successes for the provider-level breaker (no call to _onSuccess or clearProviderFailure on successful requests), failures will accumulate indefinitely across the process lifetime, eventually tripping the breaker even for rare, non-consecutive errors.
| try { | ||
| const now = Date.now(); | ||
| const entry = providerFailureState.get(provider); | ||
| const breaker = getProviderBreaker(provider); |
There was a problem hiding this comment.
The getProviderBreaker helper (defined at line 488) is configured using profile.failureThreshold and profile.resetTimeoutMs, which are connection-level resilience settings. For provider-level circuit breaking, it should use profile.providerFailureThreshold and profile.providerCooldownMs. This discrepancy causes the provider-level breaker to use incorrect thresholds and a significantly shorter cooldown than intended (e.g., 1 minute instead of the 5 minutes defined in PROVIDER_PROFILES for OAuth providers).
3502c5a
into
cheaper-inference:release/v3.7.0
…ts (cheaper-inference#1485) Integrated into release/v3.7.0
…ts (cheaper-inference#1485) Integrated into release/v3.7.0
Summary
Fixes a runtime
ReferenceErrorinrecordProviderFailure()where three constants (PROVIDER_FAILURE_WINDOW_MS,PROVIDER_FAILURE_THRESHOLD,PROVIDER_COOLDOWN_MS) were being used but never defined.The fix removes the redundant custom provider failure tracking implementation (64 lines) and delegates to the existing shared
CircuitBreakerutility which already handles failure counting, threshold detection, and state transitions.Also fixes: Indefinite lockout bug - skip recording failures when already in cooldown to prevent resetting the cooldown timer.
Related Issues
Validation
Tests Added Or Updated
CircuitBreakerinfrastructureaccount-fallback-service.test.tscovers the affected functionsCoverage Notes
getCircuitBreaker()and_onFailure()patternsReviewer Notes
src/sse/handlers/chat.ts:628,856