Repository navigation
feat(fallback): add provider-level circuit breaker with configurable thresholds - #1473
Conversation
- create createSettingsApiHarness function with temp directory setup - add beforeEach/afterEach hooks for storage reset between tests - add after hook for cleanup - use dynamic imports after env setup to ensure proper initialization
- add provider-level circuit breaker config to PROVIDER_PROFILES - remove hardcoded threshold constants in favor of profile-based config - use getProviderProfile() to read thresholds with fallback defaults - support different failure tolerance per provider type
The mergeProviderProfile function was missing the three new fields added to PROVIDER_PROFILES (providerFailureThreshold, providerFailureWindowMs, providerCooldownMs). This caused tests to fail because the profile returned by getRuntimeProviderProfile did not include these fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. Remove 429 from PROVIDER_FAILURE_ERROR_CODES - 429 (rate limit) is already handled by model-level and account-level locks - Including it in provider-wide circuit breaker causes premature cooldown 2. Fix reference counting in ModelStatusContext - Changed registeredModels from Set to Map<string, number> - Prevents polling stop when one component unmounts while others still track the model 3. Fix model ID parsing for providers with slashes in model names - Use indexOf/substring instead of split to handle models like "modelscope/moonshotai/Kimi-K2.5" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 在访问 providerSpecificData 前对 connection 添加可选链操作符 - 防止 connection 为 null/undefined 时导致的运行时错误
There was a problem hiding this comment.
Code Review
This pull request refactors the resilience and circuit breaker logic, moving from a per-model availability system to a centralized provider-level circuit breaker and connection cooldown mechanism. It introduces new resilience settings, updates the API to support these configurations, and removes legacy model availability tracking. I have identified critical issues in the implementation of the new provider failure tracking, including the use of undefined constants and a redundant failure tracking mechanism that should be replaced by the existing circuit breaker utility. Additionally, there is a regression in connection-level error handling and a security-related inconsistency in error handling within the encryption module.
| 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); | ||
|
|
||
| // Check if we're in cooldown period | ||
| if (entry && entry.cooldownUntil !== null && now < entry.cooldownUntil) { | ||
| return; // Already in cooldown, don't record | ||
| } | ||
|
|
||
| // 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; | ||
| } | ||
|
|
||
| // 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
The recordProviderFailure function is broken because it references several undefined constants: PROVIDER_FAILURE_WINDOW_MS, PROVIDER_FAILURE_THRESHOLD, and PROVIDER_COOLDOWN_MS. Additionally, it implements a redundant failure tracking mechanism using a local providerFailureState Map, while the file already imports and uses the CircuitBreaker utility via getProviderBreaker. This function should be simplified to use the existing breaker logic.
/**
* Record a provider failure against the shared circuit breaker.
*/
export function recordProviderFailure(
provider: string | null | undefined,
log?: { warn?: (...args: unknown[]) => void }
): void {
if (!provider) return;
const breaker = getProviderBreaker(provider);
if (!breaker) return;
breaker._onFailure();
if (!breaker.canExecute()) {
log?.warn?.(
`[ProviderFailure] ${provider}: circuit breaker opened after repeated failures`
);
}
}| // 6. Mark account as quota-exhausted on 429 response | ||
| // For providers that route quota/cooldown at model scope, a 429 on one model | ||
| // does not mean the whole connection is exhausted. | ||
| if (result.status === 429 && shouldMarkAccountExhaustedFrom429(provider, model)) { | ||
| const passthroughModels = credentials.providerSpecificData?.passthroughModels; | ||
| if ( | ||
| result.status === 429 && | ||
| shouldMarkAccountExhaustedFrom429(provider, model, passthroughModels) | ||
| ) { | ||
| markAccountExhaustedFrom429(credentials.connectionId, provider); | ||
| } | ||
|
|
There was a problem hiding this comment.
Regression in connection-level resilience. The logic that updated the provider connection status in the database (e.g., setting rateLimitedUntil or testStatus) was removed from open-sse/handlers/chatCore.ts but has not been fully replaced in src/sse/handlers/chat.ts. While markAccountExhaustedFrom429 is called for 429 errors, other retryable failures (like 503) no longer trigger a connection-level cooldown in the database. This will lead to repeated attempts on failing connections in subsequent requests.
| // 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>(); |
There was a problem hiding this comment.
The providerFailureState and providerFailureLocks are unused if recordProviderFailure is refactored to use the shared CircuitBreaker utility. These should be removed to keep the module clean.
| // 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>(); | |
| // Error codes that count toward provider-level failure threshold | |
| const PROVIDER_FAILURE_ERROR_CODES = new Set([408, 500, 502, 503, 504]); | |
| // T06 (sub2api PR #1037): Signals that indicate permanent account deactivation. |
| `Auth tag validation likely failed.` | ||
| ); | ||
| return ciphertext; | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling. If decipher.final() fails (likely due to an invalid authentication tag), the function returns the original ciphertext. However, the earlier check at line 113 returns null to prevent sending encrypted data to providers. For security and consistency, this should also return null.
return null;76b0f07
into
cheaper-inference:release/v3.7.0
clousky2020
left a comment
There was a problem hiding this comment.
修复已应用
1. recordProviderFailure 未定义常量 ✅ 已修复
提交 74549cf 重写了函数,直接委托给现有的 getCircuitBreaker 工具。删除了冗余的 ProviderFailureEntry 类型、providerFailureState Map 和 providerFailureLocks Set。
2. 503 回归 ⚠️ 非问题
经代码分析,503 等服务器错误通过以下流程正确处理:
checkFallbackError将 503 识别为可重试状态markAccountUnavailable更新数据库rateLimitedUntil- 只有当
shouldFallback: false时才触发 provider breaker
3. 冗余代码 ✅ 已清理
同上,已在提交 74549cf 中删除。
4. encryption.ts 的一致性问题
这个文件不在本次 PR 的改动范围内。
请重新 review,谢谢!
feat(fallback): add provider-level circuit breaker with configurable thresholds
feat(fallback): add provider-level circuit breaker with configurable thresholds
Summary
PROVIDER_PROFILEScategory (oauth/apikey/local):providerFailureThreshold: number of failures before provider cooldown (oauth: 3, apikey: 5, local: 2)providerFailureWindowMs: time window for counting failures (oauth: 10min, apikey: 20min, local: 1min)providerCooldownMs: provider-level cooldown duration (oauth: 5min, apikey: 10min, local: 1min)EditConnectionModalwhereconnection?.providerSpecificDatawas accessed without optional chaining whenconnectioncould benull.Files changed
open-sse/config/constants.ts: Added provider-level circuit breaker fields toPROVIDER_PROFILESopen-sse/services/accountFallback.ts: Wired new fields intobuildProviderProfile, addedgetProviderProfilesrc/app/(dashboard)/dashboard/providers/[id]/page.tsx: Fixed null safety with?.operatortests/unit/settings-api.test.ts: Added test harness for settings isolationTest plan
npm run test:unit)npm run typecheck:core)🤖 Generated with Claude Code