fix(proxy): fail over provider-local model 404s - #1354
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough新增 provider-local 模型不可用 404 的识别与分类逻辑，并调整转发器以跳过同供应商重试和 reactive rectifier、切换备选供应商；测试覆盖消息匹配、错误分类、hedge 行为、重试限制及 durableTerminal 回调。 ChangesProvider-local 模型 404 处理
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/app/v1/_lib/proxy/errors.ts`:
- Around line 580-585: Update isProviderLocalModelUnavailableError to reject
errors with statusCodeInferred set, and match the marker only against
error.message and error.upstreamError?.body, excluding rawBody. In
tests/unit/proxy/provider-local-model-unavailable.test.ts lines 61-71, replace
the rawBody-positive case with a regression asserting that statusCodeInferred:
true does not match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b9e9e6a-c860-4112-b1a6-aa878c5f57c9
📒 Files selected for processing (5)
src/app/v1/_lib/proxy/errors.tssrc/app/v1/_lib/proxy/forwarder.tstests/unit/proxy/provider-local-model-unavailable.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/proxy-forwarder-retry-limit.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d65cba22e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/v1/_lib/proxy/forwarder.ts (1)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win使用
@/路径别名导入errors。第 78 行仍使用相对路径
./errors，违反 TypeScript 文件统一使用@/映射src/的规范。请改为@/app/v1/_lib/proxy/errors。🤖 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` at line 78, Update the errors import in the forwarder module to use the `@/` alias, changing the relative ./errors reference to `@/app/v1/_lib/proxy/errors` while preserving the existing imported symbols.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Line 78: Update the errors import in the forwarder module to use the `@/` alias,
changing the relative ./errors reference to `@/app/v1/_lib/proxy/errors` while
preserving the existing imported symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31cf8ec0-50c5-4b3a-9f5c-87cabed77e10
📒 Files selected for processing (3)
src/app/v1/_lib/proxy/forwarder.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/proxy-forwarder-retry-limit.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/proxy/proxy-forwarder-retry-limit.test.ts
- tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app/v1/_lib/proxy/errors.ts (1)
581-587: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win不要使用
rawBody进行错误规则匹配。当前虽然排除了
statusCodeInferred === true，但后续仍把upstreamError.rawBody纳入 marker 检测。rawBody的契约明确规定其仅用于响应返回，不参与规则匹配；否则当它与message/body不一致时，仍会误将错误分类为RESOURCE_NOT_FOUND，进而改变故障转移行为。建议修复
- return [error.message, error.upstreamError?.body, error.upstreamError?.rawBody].some((content) => + return [error.message, error.upstreamError?.body].some((content) => content?.toLowerCase().includes(PROVIDER_LOCAL_MODEL_UNAVAILABLE_MARKER) );🤖 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/errors.ts` around lines 581 - 587, 更新 ProxyError 的 404 错误规则匹配逻辑，禁止将 upstreamError.rawBody 传入 marker 检测；仅使用契约允许参与匹配的 message/body 字段，并保留现有 statusCodeInferred 与 RESOURCE_NOT_FOUND 分类行为。
🤖 Prompt for all review comments with 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.
Duplicate comments:
In `@src/app/v1/_lib/proxy/errors.ts`:
- Around line 581-587: 更新 ProxyError 的 404 错误规则匹配逻辑，禁止将 upstreamError.rawBody 传入
marker 检测；仅使用契约允许参与匹配的 message/body 字段，并保留现有 statusCodeInferred 与
RESOURCE_NOT_FOUND 分类行为。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd98b0e6-16b1-4117-96a2-eb4f4dd3b8a5
📒 Files selected for processing (2)
src/app/v1/_lib/proxy/errors.tstests/unit/proxy/provider-local-model-unavailable.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/proxy/provider-local-model-unavailable.test.ts
|
CI note after the latest push:
Code Quality, API, Integration, Docker Build, CodeRabbit, and Greptile checks pass. |
# Conflicts: # src/app/v1/_lib/proxy/errors.ts # src/app/v1/_lib/proxy/forwarder.ts # tests/unit/proxy/proxy-forwarder-retry-limit.test.ts
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
model_not_foundand synthetic fake-200 classifications unchangedRoot cause
The response
not supported by any configured account in this groupwas matched by the broadmodel_not_foundclient-error rule. One Provider could therefore abort the entire race, including a healthy in-flight candidate or a ready fallback.This change classifies only the exact real HTTP 404 marker as
RESOURCE_NOT_FOUND. It does not change ordinary model-not-found errors, fake-200 handling, or circuit-breaker accounting.Validation
bunx vitest run tests/unit/proxy/provider-local-model-unavailable.test.ts tests/unit/proxy/proxy-forwarder-retry-limit.test.ts tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts(54 tests)bun run typecheckbun run format:checkbun run lintgit diff --checkRelated to #1340.
Greptile Summary
This PR makes provider-local model failures fall through to other Providers. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Upstream error] --> B{Real HTTP 404 with account-group marker?} B -- No --> C[Use existing classification] B -- Yes --> D[Classify as Provider-local resource failure] D --> E[Skip rectification] E --> F[Skip another attempt on the same Provider] F --> G{Another Provider or active hedge?} G -- Yes --> H[Continue request] G -- No --> I[Return exhausted-Providers response]Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/dev..." | Re-trigger Greptile