Skip to content

fix(fallback): use shared CircuitBreaker instead of undefined constants #1485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
diegosouzapw merged 2 commits into from
Apr 21, 2026
Merged

fix(fallback): use shared CircuitBreaker instead of undefined constants #1485

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74549cf
fix(fallback): use shared CircuitBreaker instead of undefined constants
clousky2020 Apr 21, 2026
4d9ef29
fix(breaker): skip recording failures during cooldown to prevent inde…
clousky2020 Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 14 additions & 63 deletions open-sse/services/accountFallback.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,24 +49,9 @@ type ModelFailureState = {
};

// Provider-level failure tracking for circuit breaker behavior
type ProviderFailureEntry = {
failureCount: number;
lastFailureAt: number;
resetAfterMs: number;
cooldownUntil: number | null;
};

// Error codes that count toward provider-level failure threshold
const PROVIDER_FAILURE_ERROR_CODES = new Set([408, 500, 502, 503, 504]);

// Provider-level failure state map: providerId -> failure entry
const providerFailureState = new Map<string, ProviderFailureEntry>();
// Guard against synchronous re-entrant calls within the same event-loop tick.
// NOT a true mutex — Node.js is single-threaded, so different SSE streams
// can interleave across ticks. This Set prevents a single call from recursively
// re-entering recordProviderFailure within the same synchronous call stack.
const providerFailureLocks = new Set<string>();

// T06 (sub2api PR #1037): Signals that indicate permanent account deactivation.
// When a 401 body contains these strings, the account is permanently dead
// and should NOT be retried after token refresh.
Expand Down Expand Up @@ -529,64 +514,30 @@ export function getProviderCooldownRemainingMs(provider: string | null | undefin

/**
* Record a provider failure against the shared circuit breaker.
* Delegates to the existing CircuitBreaker utility which handles
* failure counting, threshold detection, and state transitions.
*
* IMPORTANT: If the breaker is already OPEN (in cooldown), we skip
* recording the failure to prevent resetting the cooldown timer.
* This matches the original behavior where failures during cooldown
* were ignored to avoid indefinite lockout.
*/
export function recordProviderFailure(
provider: string | null | undefined,
log?: { warn?: (...args: unknown[]) => void }
): void {
if (!provider) return;

// Guard against concurrent re-entrant calls within the same tick
if (providerFailureLocks.has(provider)) return;
providerFailureLocks.add(provider);

try {
const now = Date.now();
const entry = providerFailureState.get(provider);
const breaker = getProviderBreaker(provider);

@gemini-code-assist gemini-code-assist Bot Apr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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).

if (!breaker) return;

// Check if we're in cooldown period
if (entry && entry.cooldownUntil !== null && now < entry.cooldownUntil) {
return; // Already in cooldown, don't record
}
// Skip if already in cooldown to prevent timer reset (indefinite lockout bug)
if (!breaker.canExecute()) return;

// Check if failure window has expired
if (entry && now - entry.lastFailureAt > entry.resetAfterMs) {
// Window expired, reset count
providerFailureState.set(provider, {
failureCount: 1,
lastFailureAt: now,
resetAfterMs: PROVIDER_FAILURE_WINDOW_MS,
cooldownUntil: null,
});
return;
}
breaker._onFailure();

@gemini-code-assist gemini-code-assist Bot Apr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.


// Increment failure count
const newCount = entry ? entry.failureCount + 1 : 1;

if (newCount >= PROVIDER_FAILURE_THRESHOLD) {
// Threshold reached, enter cooldown
const cooldownUntil = now + PROVIDER_COOLDOWN_MS;
providerFailureState.set(provider, {
failureCount: newCount,
lastFailureAt: now,
resetAfterMs: PROVIDER_FAILURE_WINDOW_MS,
cooldownUntil,
});
log?.warn?.(
`[ProviderFailure] ${provider}: ${newCount} failures in ${PROVIDER_FAILURE_WINDOW_MS / 1000}s — entering ${PROVIDER_COOLDOWN_MS / 1000}s cooldown`
);
} else {
// Just increment counter
providerFailureState.set(provider, {
failureCount: newCount,
lastFailureAt: now,
resetAfterMs: PROVIDER_FAILURE_WINDOW_MS,
cooldownUntil: null,
});
}
} finally {
providerFailureLocks.delete(provider);
if (!breaker.canExecute()) {
log?.warn?.(`[ProviderFailure] ${provider}: circuit breaker opened after repeated failures`);
}
}

Expand Down