Skip to content

fix(proxy): classify errors by rule category to preserve failover for service errors #1103

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

Open
KevinShiCN wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix(proxy): classify errors by rule category to preserve failover for service errors #1103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 72 additions & 3 deletions src/app/v1/_lib/proxy/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,61 @@ function extractErrorContentForDetection(error: Error): string {
return error.message;
}

/**
* 真正属于 "客户端输入错误" 的规则 category 白名单
*
* 仅当匹配的规则 category 落入此集合时，对应错误才被分类为
* NON_RETRYABLE_CLIENT_ERROR（不重试 + 不计入熔断器 + 直接返回）。
*
* 设计原因：
* - error_rules 表允许用户自定义规则，category 字段可以是任意字符串
* - 历史实现中，任何匹配规则都会被当作 NON_RETRYABLE_CLIENT_ERROR
* - 但 service_error / network_error / rate_limit 这类 category 实际是
* 供应商侧或网络侧的可恢复错误，应进入正常 failover/熔断器流程，而不是
* 被白名单化导致 "stopping immediately" 不再切换供应商
*
* 通过显式白名单，确保只有真正的客户端输入错误（如 prompt_limit、
* validation_error、content_filter 等）触发不可重试逻辑。
*
* 此集合的设计原则：
* - 仅包含官方默认规则（DEFAULT_ERROR_RULES）实际使用的客户端错误 category
* - 用户自定义规则若使用集合外的 category（如 service_error），将不再
* 被错误地白名单化；这些错误会回退到 ProxyError 的标准分类逻辑
*/
const NON_RETRYABLE_CLIENT_ERROR_CATEGORIES = new Set<string>([
"prompt_limit",
"input_limit",
"context_limit",
"token_limit",
"content_filter",
"validation_error",
"thinking_error",
"parameter_error",
"pdf_limit",
"media_limit",
"cache_limit",
"invalid_request",
"model_error",
"store_error",
]);
Comment on lines +594 to +609

@greptile-apps greptile-apps Bot Apr 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Whitelist is a silent maintenance hazard when new default categories are added

The set is hard-coded and has no compile-time link to DEFAULT_ERROR_RULES. If a future default rule is added with a new category (e.g. "citation_limit") and the whitelist is not updated, that rule's errors will silently fall back to PROVIDER_ERROR/SYSTEM_ERROR instead of short-circuiting retries — the opposite of the current bug, but still incorrect.

A lightweight guard could be a unit test that asserts every category used by DEFAULT_ERROR_RULES is present in NON_RETRYABLE_CLIENT_ERROR_CATEGORIES, catching drift at CI time rather than in production.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/errors.ts
Line: 594-609

Comment:
**Whitelist is a silent maintenance hazard when new default categories are added**

The set is hard-coded and has no compile-time link to `DEFAULT_ERROR_RULES`. If a future default rule is added with a new category (e.g. `"citation_limit"`) and the whitelist is not updated, that rule's errors will silently fall back to `PROVIDER_ERROR`/`SYSTEM_ERROR` instead of short-circuiting retries — the opposite of the current bug, but still incorrect.

A lightweight guard could be a unit test that asserts every category used by `DEFAULT_ERROR_RULES` is present in `NON_RETRYABLE_CLIENT_ERROR_CATEGORIES`, catching drift at CI time rather than in production.

How can I resolve this? If you propose a fix, please make it concise.


/**
* 判断已匹配的检测结果是否应触发 NON_RETRYABLE_CLIENT_ERROR 分类
*
* @param result - errorRuleDetector 返回的检测结果
* @returns true = 视为不可重试客户端错误；false = 不触发该分类（让标准分类逻辑处理）
*/
function shouldTreatMatchedRuleAsNonRetryable(result: ErrorDetectionResult): boolean {
if (!result.matched) {
return false;
}
// category 必须显式存在，且落在白名单集合内
return (
typeof result.category === "string" &&
NON_RETRYABLE_CLIENT_ERROR_CATEGORIES.has(result.category)
);
}

/**
* 错误规则检测结果缓存
*
Expand Down Expand Up @@ -623,7 +678,7 @@ export async function getErrorDetectionResultAsync(error: Error): Promise<ErrorD
export function isNonRetryableClientError(error: Error): boolean {
const cached = errorDetectionCache.get(error);
if (cached) {
return cached.matched;
return shouldTreatMatchedRuleAsNonRetryable(cached);
}

const content = extractErrorContentForDetection(error);
Expand All @@ -637,19 +692,33 @@ export function isNonRetryableClientError(error: Error): boolean {
void detectErrorRuleOnceAsync(error).catch(() => undefined);
}

return result.matched;
return shouldTreatMatchedRuleAsNonRetryable(result);
}

/**
* 检测是否为不可重试的客户端输入错误（异步版本）
*
* 此函数会确保错误规则已加载后再进行检测
* 生产代码路径（forwarder、error-handler）应优先使用此版本
*
* 仅当匹配规则的 category 落入 NON_RETRYABLE_CLIENT_ERROR_CATEGORIES 白名单时
* 才返回 true。其他 category（如 service_error、network_error 等）即使匹配也
* 会回退到 ProxyError 的标准分类（PROVIDER_ERROR/SYSTEM_ERROR），从而保留
* 故障转移和熔断器逻辑。
*/
export async function isNonRetryableClientErrorAsync(error: Error): Promise<boolean> {
// 使用缓存的检测结果，避免重复执行规则匹配
const result = await detectErrorRuleOnceAsync(error);
return result.matched;
return shouldTreatMatchedRuleAsNonRetryable(result);
}

/**
* 暴露内部白名单常量供测试使用
*
* 注意：这是只读副本，外部不应修改运行时白名单。
*/
export function getNonRetryableClientErrorCategoriesForTesting(): ReadonlySet<string> {
return NON_RETRYABLE_CLIENT_ERROR_CATEGORIES;
}
Comment on lines +715 to 722

@greptile-apps greptile-apps Bot Apr 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Misleading "read-only copy" comment; actual object reference is returned

The JSDoc says "这是只读副本" (this is a read-only copy), but the function returns the live NON_RETRYABLE_CLIENT_ERROR_CATEGORIES object, not a copy. ReadonlySet<string> is a TypeScript compile-time constraint only — a test that casts the return value to Set<string> can call .add() or .delete() and mutate the runtime whitelist, causing subtle cross-test pollution.

Either return a true copy, or update the comment to clarify that ReadonlySet is the only guard in place:

// Option A: return a real copy so the comment is accurate
export function getNonRetryableClientErrorCategoriesForTesting(): ReadonlySet<string> {
  return new Set(NON_RETRYABLE_CLIENT_ERROR_CATEGORIES);
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/errors.ts
Line: 715-722

Comment:
**Misleading "read-only copy" comment; actual object reference is returned**

The JSDoc says "这是只读副本" (this is a read-only copy), but the function returns the live `NON_RETRYABLE_CLIENT_ERROR_CATEGORIES` object, not a copy. `ReadonlySet<string>` is a TypeScript compile-time constraint only — a test that casts the return value to `Set<string>` can call `.add()` or `.delete()` and mutate the runtime whitelist, causing subtle cross-test pollution.

Either return a true copy, or update the comment to clarify that `ReadonlySet` is the only guard in place:

```ts
// Option A: return a real copy so the comment is accurate
export function getNonRetryableClientErrorCategoriesForTesting(): ReadonlySet<string> {
  return new Set(NON_RETRYABLE_CLIENT_ERROR_CATEGORIES);
}
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +715 to 722

@coderabbitai coderabbitai Bot Apr 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

JSDoc 与实现不一致：返回的不是“只读副本”

注释写的是“只读副本，外部不应修改运行时白名单”，但函数实际返回的是底层 Set 的同一引用，仅依赖 ReadonlySet<string> 的类型断言来阻止修改。调用方一旦使用 as Set<string> 类型断言或 JS 调用，就能直接修改运行时白名单，造成全局副作用。

建议二选一（任选其一即可消除歧义）：

方案 A：真正返回副本 
 export function getNonRetryableClientErrorCategoriesForTesting(): ReadonlySet<string> {
-  return NON_RETRYABLE_CLIENT_ERROR_CATEGORIES;
+  return new Set(NON_RETRYABLE_CLIENT_ERROR_CATEGORIES);
 }
方案 B：保留同一引用，但修正注释，明确仅靠类型层面保护 
-/**
- * 暴露内部白名单常量供测试使用
- *
- * 注意：这是只读副本，外部不应修改运行时白名单。
- */
+/**
+ * 暴露内部白名单常量供测试使用
+ *
+ * 注意：返回值通过 ReadonlySet 类型仅在编译期阻止修改，
+ * 运行时仍是同一引用；测试中切勿通过类型断言绕过只读约束。
+ */
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* 暴露内部白名单常量供测试使用
*
* 注意：这是只读副本，外部不应修改运行时白名单。
*/
export function getNonRetryableClientErrorCategoriesForTesting(): ReadonlySet<string> {
return NON_RETRYABLE_CLIENT_ERROR_CATEGORIES;
}
/**
* 暴露内部白名单常量供测试使用
*
* 注意：这是只读副本，外部不应修改运行时白名单。
*/
export function getNonRetryableClientErrorCategoriesForTesting(): ReadonlySet<string> {
return new Set(NON_RETRYABLE_CLIENT_ERROR_CATEGORIES);
}
Suggested change
/**
* 暴露内部白名单常量供测试使用
*
* 注意：这是只读副本，外部不应修改运行时白名单。
*/
export function getNonRetryableClientErrorCategoriesForTesting(): ReadonlySet<string> {
return NON_RETRYABLE_CLIENT_ERROR_CATEGORIES;
}
/**
* 暴露内部白名单常量供测试使用
*
* 注意：返回值通过 ReadonlySet 类型仅在编译期阻止修改，
* 运行时仍是同一引用；测试中切勿通过类型断言绕过只读约束。
*/
export function getNonRetryableClientErrorCategoriesForTesting(): ReadonlySet<string> {
return NON_RETRYABLE_CLIENT_ERROR_CATEGORIES;
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/app/v1/_lib/proxy/errors.ts` around lines 715 - 722, The JSDoc
incorrectly claims getNonRetryableClientErrorCategoriesForTesting returns a
read-only copy while it currently returns the original Set reference
NON_RETRYABLE_CLIENT_ERROR_CATEGORIES; either return an actual copy (e.g.,
construct and return a new Set from NON_RETRYABLE_CLIENT_ERROR_CATEGORIES) to
make it immutable at runtime, or update the JSDoc to state that the function
returns the same runtime Set and only TypeScript's ReadonlySet typing prevents
mutation; modify getNonRetryableClientErrorCategoriesForTesting and/or its JSDoc
accordingly and keep the symbol NON_RETRYABLE_CLIENT_ERROR_CATEGORIES as the
source of truth.


/**
Expand Down
Loading
Loading