Skip to content

fix(proxy): exclude upstream 400/422 from breaker #1228

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

Draft
ROOOO wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

fix(proxy): exclude upstream 400/422 from breaker #1228

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
11 changes: 11 additions & 0 deletions src/app/v1/_lib/proxy/forwarder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ function decodeRequestBodyAsJson(body: BodyInit | undefined): Record<string, unk
}
}

function isNonRetryableUpstreamRequestError(error: Error): error is ProxyError {
return error instanceof ProxyError && (error.statusCode === 400 || error.statusCode === 422);
}

const OUTBOUND_TRANSPORT_HEADER_BLACKLIST = [
"content-length",
"connection",
Expand Down Expand Up @@ -1684,6 +1688,13 @@ export class ProxyForwarder {
}
}

if (
errorCategory === ErrorCategory.PROVIDER_ERROR &&
isNonRetryableUpstreamRequestError(lastError)
) {
errorCategory = ErrorCategory.NON_RETRYABLE_CLIENT_ERROR;
}
Comment on lines +1691 to +1696

@gemini-code-assist gemini-code-assist Bot May 30, 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

While this correctly maps 400/422 upstream errors to NON_RETRYABLE_CLIENT_ERROR in the standard forwarding path, this mapping is completely missing in the streaming hedged path (sendStreamingWithHedge -> handleAttemptFailure).

As a result, if a streaming hedged request fails with an upstream 400 or 422 error:

  1. It will still be treated as PROVIDER_ERROR.
  2. It will count against the provider's circuit breaker via recordFailure.
  3. It will trigger launchAlternative() and attempt to failover/retry with other providers.

To ensure consistent behavior across both paths, please apply this mapping in handleAttemptFailure (around line 3910) as well, and add a corresponding test case in proxy-forwarder-retry-limit.test.ts to cover the hedged path.

Comment on lines +1691 to +1696

@coderabbitai coderabbitai Bot May 30, 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 | 🟠 Major | ⚡ Quick win

400/422 仅在非 hedge 路径重分类，流式请求仍可能误重试并计入熔断。

这段改动只覆盖 ProxyForwarder.send() 主路径；sendStreamingWithHedge() 的 handleAttemptFailure 没有复用同一判定，流式场景下 400/422 仍会按 PROVIDER_ERROR 处理，和本次“上游 400/422 不重试且不计熔断”的目标不一致。建议在 hedge 失败分类处同步加入同样的重分类分支。

建议补丁 
diff --git a/src/app/v1/_lib/proxy/forwarder.ts b/src/app/v1/_lib/proxy/forwarder.ts
@@
       if (reactiveRectifierResult.matched) {
@@
       }
+
+      if (
+        errorCategory === ErrorCategory.PROVIDER_ERROR &&
+        isNonRetryableUpstreamRequestError(error)
+      ) {
+        errorCategory = ErrorCategory.NON_RETRYABLE_CLIENT_ERROR;
+        lastErrorCategory = errorCategory;
+      }
 
       if (errorCategory === ErrorCategory.NON_RETRYABLE_CLIENT_ERROR) {
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 1691 - 1696, The
hedge/streaming failure path doesn't apply the same 400/422 reclassification as
ProxyForwarder.send(); update sendStreamingWithHedge()—specifically inside its
handleAttemptFailure logic—to check when errorCategory ===
ErrorCategory.PROVIDER_ERROR && isNonRetryableUpstreamRequestError(lastError)
and then set errorCategory = ErrorCategory.NON_RETRYABLE_CLIENT_ERROR so
streaming attempts with upstream 400/422 are treated non-retryable and do not
count toward circuit-breaker metrics (same fix as in ProxyForwarder.send()).


// ⭐ 3. 不可重试的客户端输入错误处理（不计入熔断器，不重试，立即返回）
if (errorCategory === ErrorCategory.NON_RETRYABLE_CLIENT_ERROR) {
const detectionResult = await getErrorDetectionResultAsync(lastError);
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/proxy/proxy-forwarder-retry-limit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,41 @@ describe("ProxyForwarder - retry limit enforcement", () => {
});
});

describe("ProxyForwarder - client request upstream errors", () => {
beforeEach(() => {
vi.clearAllMocks();
vi.mocked(categorizeErrorAsync).mockResolvedValue(ErrorCategory.PROVIDER_ERROR);
});

test("upstream 400 should not retry or count against provider circuit breaker", async () => {
const session = createSession();
const provider = createProvider({
providerVendorId: null,
maxRetryAttempts: 4,
});
session.setProvider(provider);

const doForward = vi.spyOn(
ProxyForwarder as unknown as { doForward: (...args: unknown[]) => unknown },
"doForward"
);
doForward.mockImplementationOnce(async () => {
throw new ProxyError("Provider returned 400: Bad Request", 400, {
body: '{"detail":"Bad Request"}',
providerId: provider.id,
providerName: provider.name,
});
});

await expect(ProxyForwarder.send(session)).rejects.toMatchObject({
statusCode: 400,
});

expect(doForward).toHaveBeenCalledTimes(1);
expect(mocks.recordFailure).not.toHaveBeenCalled();
});
Comment on lines +903 to +929

@coderabbitai coderabbitai Bot May 30, 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 | ⚡ Quick win

建议补一条 422 用例，避免目标行为只被 400 半覆盖。

当前新增断言只覆盖 400；本次行为定义同时包含 422，建议并列增加 ProxyError(..., 422) 场景，断言同样“只调用 1 次 doForward 且不调用 recordFailure”。

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/proxy/proxy-forwarder-retry-limit.test.ts` around lines 903 - 929,
在 tests/unit/proxy/proxy-forwarder-retry-limit.test.ts 中补一条与现有 "upstream 400
should not retry or count against provider circuit breaker" 对应的用例来覆盖 422
场景：复制当前测试逻辑但将 doForward.mockImplementationOnce 抛出的 ProxyError 状态码改为 422，并对
ProxyForwarder.send(session) 断言会 reject 且 statusCode 为 422，同时断言
ProxyForwarder.doForward 仅被调用一次并且 mocks.recordFailure 未被调用（保留对
createSession、createProvider、provider.maxRetryAttempts 等相同的设置和断言）。

});

describe("ProxyForwarder - endpoint stickiness on retry", () => {
beforeEach(() => {
vi.clearAllMocks();
Expand Down
Loading