fix(proxy): retry upstream storage-capacity 400 failures#1345
Conversation
📝 WalkthroughWalkthroughChanges新增上游存储容量型 HTTP 400 错误检测，将其分类为 上游存储容量错误重试
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces detection and handling for upstream storage-capacity failures that are incorrectly returned as HTTP 400 errors. It adds a helper function isRetryableUpstreamStorageCapacityError to identify these errors based on specific error markers in the response body, classifying them as retryable provider errors rather than non-retryable client errors. Additionally, it updates the proxy forwarder to bypass reactive rectification for these errors, ensuring they trigger provider failover and retries. Comprehensive unit tests have been added to verify this behavior. There are no review comments to address, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
CI note (verified against the current
Evidence: PR test run, dev baseline run, Codex permission failure, successful fallback review. These failures are unrelated to the storage-capacity classification path, so I am leaving the baseline test/workflow fixes out of this PR. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This is a narrow, well-scoped carve-out that correctly classifies a known upstream storage-capacity 400 as PROVIDER_ERROR ahead of the broad invalid request client-error rule, with a matching guard in the reactive rectifier. The logic is sound, the two-marker requirement prevents ordinary 400s from flipping retryable, the fake-200 exclusion is correct, and the forwarder short-circuit is necessary rather than redundant (validated against thinking-signature-rectifier.ts, whose /invalid request/i trigger would otherwise force a wasteful same-provider retry on a storage-full upstream). No issues met the reporting threshold.
PR Size: M
- Lines changed: 280 (280 additions, 0 deletions)
- Files changed: 5
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - The detector's precedence branch in
categorizeErrorAsyncis correctly placed after transport/client-abort checks and beforeisNonRetryableClientErrorAsync; both forwarder call sites pass the caught error and the{ matched: false }short-circuit falls through cleanly to PROVIDER_ERROR retry/switch. - Security (OWASP Top 10) - Marker matching uses case-insensitive
String.includes, not regex; no injection surface; no secrets. - Error handling - No new try/catch, promises, or fallbacks introduced; no silent-failure paths added.
- Type safety -
as constmarker tuple;error: Errorparam with internalinstanceof ProxyErrorguard;body != nullguard before.includes; noany. - Documentation accuracy - Comments explain the narrowness rationale and the fake-200 exclusion; no drift.
- Test coverage - 4 classification unit tests (priority over invalid-request rule, ordinary 400 stays non-retryable, both-markers-required, fake-200 not reinterpreted) plus retry-limit and hedge-first-byte path tests asserting retry + failure recording + provider switch.
- Code clarity - Detector is small and self-documenting; rectifier guard is a single early return.
Automated review by Claude AI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
# Conflicts: # src/app/v1/_lib/proxy/errors.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: bf73fb3a60
ℹ️ 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".
| ]); | ||
| }); | ||
|
|
||
| test("upstream storage-capacity 400 should retry, record failure, and switch provider", async () => { |
There was a problem hiding this comment.
When this suite runs in order, the preceding local-DB test sets categorizeErrorAsync with mockResolvedValue(5 as ErrorCategory), and the shared beforeEach only calls vi.clearAllMocks(), which preserves that mock implementation. This new storage-capacity test never restores the mock to PROVIDER_ERROR, so it enters the forwarder with the stale LOCAL_OVERLOAD category, skips the provider-error branch, and the recordFailure/failed-provider assertions won't be exercising the intended path. Set the mock explicitly in this test (or reset implementations in beforeEach) so the new feature has reliable unit coverage.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
Some upstream relays report a provider-side disk-capacity failure as HTTP 400. The response body can also contain the broad
invalid requesttext, so CCH currently classifies it as a non-retryable client error and never retries, switches provider, or trips the circuit breaker.This change narrows that behavior: only real HTTP 400 responses whose upstream body contains both
disk storage creation failedanddisk free-space floor reachedare reclassified asPROVIDER_ERROR, enabling the existing retry / provider-switch / circuit-breaker paths. Ordinary invalid-request 400s and synthetic fake-200 errors are left unchanged.Problem
An upstream relay can return HTTP 400 for what is actually a provider-side storage-capacity condition (disk-free floor reached during temp-file creation). Because the same body usually also contains the substring
invalid request, the default error rule (pattern: "非法请求|illegal request|invalid request",category: invalid_request) matches it first andcategorizeErrorAsyncreturnsNON_RETRYABLE_CLIENT_ERROR. As a result:This is the same misclassification family already addressed elsewhere in the error-categorization lineage: a provider-side failure whose body text trips a broad client-error rule and gets swallowed as non-retryable.
Related Issues / PRs:
model_errorrule and was classified asNON_RETRYABLE_CLIENT_ERROR, so CCH neither switched channels nor tripped the breaker.invalid_requestrule.SYSTEM_ERROR).Solution
Add a narrow, two-marker detector and wire it into two places:
categorizeErrorAsync(errors.ts) — insert a priority check ahead of the client-error rule matching.isRetryableUpstreamStorageCapacityError(error)returns true only when the error is aProxyErrorwithstatusCode === 400, is not a synthetic fake-200 error (message does not start withFAKE_200_andupstreamError.statusCodeInferred !== true), andupstreamError.body(lowercased) contains both markers. On match it returnsPROVIDER_ERROR, so the existing retry / failover / circuit-breaker paths run.tryApplyReactiveRectifier(forwarder.ts) — pass the caught error into the rectifier and short-circuit ({ matched: false }) when it is a retryable storage-capacity 400. This stops a reactive rectifier (which keys off broad phrases likeinvalid request) from forcing a same-provider rectified retry on an upstream that is storage-full, so the request instead falls through to normal provider-switching retry.The detector deliberately requires both stable markers so a single-phrase coincidence cannot flip an ordinary 400 into a retryable one, and it explicitly excludes synthetic fake-200 errors so a locally-inferred status cannot masquerade as a real upstream 400.
Changes
Core Changes
src/app/v1/_lib/proxy/errors.ts(+36)RETRYABLE_UPSTREAM_STORAGE_ERROR_MARKERSconstant (disk storage creation failed,disk free-space floor reached).isRetryableUpstreamStorageCapacityError(error)detector.categorizeErrorAsync(before the client-error rule check) returningPROVIDER_ERRORfor the matched signature.src/app/v1/_lib/proxy/forwarder.ts(+8)tryApplyReactiveRectifiertakes a newerrorparam and short-circuits when the error is a retryable storage-capacity 400.lastErroranderrorrespectively).Supporting Changes (Tests)
tests/unit/proxy/upstream-storage-capacity-error.test.ts(+81, new) — classification unit tests: priority over theinvalid_requestrule, ordinary 400 stays non-retryable, both markers required, fake-200 not reinterpreted.tests/unit/proxy/proxy-forwarder-retry-limit.test.ts(+63) — retry path: storage-capacity 400 retries, records failure, and switches provider.tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts(+92) — hedge path: storage-capacity 400 records failure and starts an alternative provider.Breaking Changes
None. The only signature change is to the module-private
tryApplyReactiveRectifier(new requirederrorparam), and both call sites withinforwarder.tsare updated. No public exports are removed or renamed, no schema/migration, no API route changes.Testing
Automated Tests
bun run typecheckpassed;bun run buildpassed (existing Edge Runtime warnings remain).bun run test: 7,023 passed, 13 skipped; the 6 failures are pre-existing and reproduce on cleandev(macOStrlocale + endpoint-circuit isolation baseline tests).Manual Testing
invalid request(no storage markers) still returns non-retryable, with no retry or breaker accounting.Checklist
devDescription enhanced by Claude AI
Greptile Summary
This PR fixes a misclassification where an upstream relay's storage-capacity failure reported as HTTP 400 was being swallowed as
NON_RETRYABLE_CLIENT_ERROR(due to the broadinvalid requestrule matching the same body), preventing retries, provider failover, and circuit-breaker accounting.isRetryableUpstreamStorageCapacityErrorinerrors.ts: a deliberately narrow two-marker detector (bothdisk storage creation failedanddisk free-space floor reachedrequired) that runs at a new priority level before the client-error rule check and returnsPROVIDER_ERROR, re-enabling the full retry/failover/circuit-breaker paths.tryApplyReactiveRectifier(forwarder.ts) so the reactive rectifier (which also keys on broad phrases likeinvalid request) cannot force a same-provider rectified retry on a storage-full upstream.Confidence Score: 5/5
The change is narrowly scoped to a two-marker body-text detector and an early exit in the reactive rectifier; ordinary 400s and fake-200 errors are explicitly excluded by separate guards and verified by independent test cases.
The detector requires both stable upstream markers before reclassifying an error, fake-200 exclusion is tested with each guard exercised independently, and both code paths (main retry and hedge) are covered by integration tests. The only call-site change is a new required
errorparameter on a module-private function, and both callers are updated.No files require special attention.
Important Files Changed
isRetryableUpstreamStorageCapacityErrordetector and inserts it at the correct priority incategorizeErrorAsyncbefore the broad client-error rule check; logic and guard conditions are correct.errorparam totryApplyReactiveRectifierand short-circuits when the error is a retryable storage-capacity 400; both call sites are correctly updated.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Upstream returns HTTP 400] --> B{isRetryableUpstreamStorageCapacityError?} B -->|checks| C{statusCode == 400\nAND not FAKE_200_\nAND not statusCodeInferred} C -->|yes| D{body contains\nboth markers?} D -->|yes| E[PROVIDER_ERROR\nretry + failover + circuit breaker] D -->|no| F[falls through to\nNON_RETRYABLE_CLIENT_ERROR\nif rule matches] C -->|no - synthetic fake-200| F B --> G{tryApplyReactiveRectifier} G -->|storage-capacity 400| H[matched: false\nskip same-provider rectifier retry] G -->|other error| I[normal reactive\nrectifier handling] E --> J[forwarder retries /\nswitches provider] H --> JReviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/dev..." | Re-trigger Greptile