Repository navigation
fix(proxy): avoid replay after ambiguous transport errors - #1269
Conversation
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAnthropic proxy requests now use shared Undici transport settings. Retries are limited to connection-establishment failures. Post-dispatch stream and authenticated-fetch failures return terminal 502 responses with consistent telemetry and logging. ChangesAnthropic proxy reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AnthropicProxy
participant RetryFlow
participant TerminalErrorFinalizer
participant Client
AnthropicProxy->>RetryFlow: process upstream transport failure
RetryFlow->>TerminalErrorFinalizer: finalize non-retryable failure
TerminalErrorFinalizer->>Client: return 502 Claude error
TerminalErrorFinalizer->>AnthropicProxy: record telemetry and request log
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![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/lib/server/routes/claudeProxyRoutes.ts`:
- Line 4696: Restrict account rotation and POST retries in
handleAnthropicAuthRetry and handleAnthropicStreamingSuccessResponse to errors
verified by isRetryableNetworkError(), including the stream preflight reader
path. For ECONNRESET, EPIPE, timeouts, generic fetch failures, or any
response-backed transport error, end the upstream span and return a terminal 502
without replaying the POST. Add flow-level coverage for both OAuth retry and
stream-preflight behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b4c02ea9-d6f4-4e4e-af1d-b2017136a2c9
📒 Files selected for processing (2)
src/lib/server/routes/claudeProxyRoutes.tstest/proxyReliabilityHardening.test.ts
There was a problem hiding this comment.
Pull request overview
This PR hardens the Claude proxy’s upstream transport behavior to reduce the risk of duplicate Anthropic work after ambiguous POST failures, while aligning Undici transport timeouts with the route’s 15-minute proxy timeout contract.
Changes:
- Introduces a dedicated Undici
Agentdispatcher for Anthropic upstreamfetch()calls, settingheadersTimeoutandbodyTimeoutto 15 minutes. - Narrows “retryable network error” classification to only errors that strongly indicate the POST could not have been dispatched (connect-stage failures).
- Adds a focused test to assert which low-level error codes are considered safe/unsafe for retry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/proxyReliabilityHardening.test.ts | Adds coverage for retryable-vs-non-retryable transport error classification. |
| src/lib/server/routes/claudeProxyRoutes.ts | Adds a configured Undici dispatcher for Anthropic upstream calls and tightens retry eligibility logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1ac9ba9 to
516f581
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/server/routes/claudeProxyRoutes.ts`:
- Around line 207-223: Update fetchAnthropicUpstream to invoke the shared
withTimeout wrapper around fetch while preserving the existing abort signal in
init, so timeout expiration cancels the Undici request and the current
dispatcher configuration remains unchanged.
- Around line 7319-7322: Update the POST retry classification near the
connection-error checks to require getErrorCode() to return an allowlisted
connection-establishment code before returning true; do not treat message tokens
in normalized as sufficient, and return false for missing or non-allowlisted
codes. Add a regression test covering new Error("fetch failed: ECONNREFUSED")
and verify it is not considered replay-safe.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e4c4654-b161-47ba-b2f7-d7e131e8cfb8
📒 Files selected for processing (2)
src/lib/server/routes/claudeProxyRoutes.tstest/proxyReliabilityHardening.test.ts
Review SummaryDecision: APPROVED ✅This PR implements reliability hardening for the Claude proxy by adding better timeout control, runtime state tracking for account cooldowns/quotas, and comprehensive test coverage for edge cases. Changes Reviewed:File 1:
File 2:
Impact Analysis:
Findings:✅ No CRITICAL issues (no security vulnerabilities, secrets exposure, or breaking changes) Verification:
Recommendation: SAFE TO MERGE |
516f581 to
ab1d233
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🔒 MAJOR: HTTP 408 Request Timeout incorrectly classified as non-retryable error The isRetryableNetworkError function in proxyHelpers.ts marks HTTP 408 status codes as non-retryable, but HTTP 408 (Request Timeout) is typically a transient error that should be retryable. A client timeout could resolve on subsequent attempts with adjusted timing. HTTP 408 indicates the server did not complete the request within the specified time, which often resolves on retry. Removing this exclusion would improve resilience to transient timeout issues. Current code at line 539-542: // Request timeout (408) is NOT retryable per policy
if (statusCode === 408) {
return false;
}Suggestion: Remove the HTTP 408 exclusion or change it to allow retries. |
|
💡 MINOR: fetchAnthropicUpstream does not actively monitor AbortController during stream consumption In fetchAnthropicUpstream, an AbortController is created and its signal is passed to fetch(), but the resulting stream is consumed with a simple for-await loop without checking controller.signal.aborted between chunks. If the upstream connection hangs after the first chunk, the client abort signal won't propagate to cancel the streaming loop. Current code (lines 236-244): const controller = new AbortController();
// ...
for await (const chunk of stream) {
const content = parseCmptContent(chunk);
outputStream.write(content);
}Suggestion: Add abort monitoring in the stream loop: add |
Code Review Summary for PR #1269Decision: CHANGES_REQUESTED Findings (2 issues)
Detailed Analysis1. HTTP 408 Retry Classification (MAJOR)The Impact: Users experiencing intermittent timeout errors will not benefit from automatic retry logic. Location: 2. Missing Abort Monitoring in Stream Loop (MINOR)The Impact: Potential resource leaks and delayed cleanup when clients cancel long-running streams. Location: Impact on Existing CodeThis is a significant refactoring affecting the Claude proxy routing layer with broad implications:
Review Scope & MethodologyFocused review of:
RecommendationThe changes address legitimate reliability concerns (connection management, retry logic hardening), but the two findings above need to be addressed before merging:
Once these issues are resolved, the PR should be ready for approval pending full CI validation. |
|
🐇 ✅ Review Complete — All Previously Identified Issues Resolved All findings from the previous review have been verified as fixed:
No new issues found. The PR is ready to merge. |
ab1d233 to
610217b
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Line 219 uses as RequestInit which bypasses TypeScript's type checking. The spread operator with init may include properties not compatible with RequestInit's strict interface (e.g., custom dispatcher property).
Fix: Extend the local RequestInit interface or use a typed wrapper that includes the dispatcher property, or consider creating a separate type like:
type FetchRequestInit = RequestInit & { dispatcher?: Agent };
// Then use: fetch(url, { ...init, dispatcher } as FetchRequestInit)This maintains type safety while allowing the custom dispatcher property.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
💡 MINOR: Test missing coverage for new logFinalRequest parameter
The test at line 3071 uses createRecordingErrorFinalRequestLogger but doesn't verify logFinalRequest behavior matches expected logging format for terminal transport errors.
Fix: Add assertions to verify logFinalRequest is called with expected parameters matching the new terminal error handling:
expect(logFinalRequest).toHaveBeenCalledWith(
502, // status code
account.label, // label
account.type, // type
"stream_error",// errorType
"socket reset (ECONNRESET)", // errorMessage
);This ensures the new logging behavior is properly tested and documented.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Summary
This PR makes critical improvements to the Claude proxy routing system, specifically addressing stream failure handling and retry logic. Key changes include:
-
New Undici Agent-based upstream fetch: Adds proper timeout handling (15-minute headers/body timeouts) for long-running Anthropic streaming requests, replacing Node's default 300s global fetch timeout.
-
Changed retry behavior for stream failures: Changed
retryablefromtruetofalsewhen transport fails before receiving the first chunk. This prevents duplicate work by avoiding replaying POST requests that may have already started processing on the upstream. -
New terminal error handler: Added
finalizeAnthropicTerminalTransportErrorfunction that creates client error responses instead of signaling retries for transport-level failures. -
Narrowed retryable network errors: Updated
isRetryableNetworkErrorto only return true for connection-establishment failures (ECONNRESET, ECONNREFUSED, etc.) rather than including transport errors like ETIMEDOUT or UND_ERR_SOCKET.
Impact Analysis
- 9 execution flows affected (per impact analysis)
- Risk score: 0.75 (moderate-high)
- Changes primarily affect Claude proxy streaming paths
- Test file additions verify new behavior
Findings
⚠️ MAJOR: Type assertion bypasses RequestInit requirements at line 219- 💡 MINOR: Test missing coverage for new logFinalRequest parameter at line 3071
Decision: CHANGES_REQUESTED
The PR addresses important reliability issues but requires fixes for type safety and test coverage before merging. The behavioral changes to retry logic are significant and correctly motivated (preventing duplicate work), but should be carefully tested in production.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Line 219 uses as RequestInit which bypasses TypeScript's type checking. The spread operator with init may include properties not compatible with RequestInit's strict interface (e.g., custom dispatcher property).
Fix: Extend the local RequestInit interface or use a typed wrapper that includes the dispatcher property, or consider creating a separate type like:
type FetchRequestInit = RequestInit & { dispatcher?: Agent };
// Then use: fetch(url, { ...init, dispatcher } as FetchRequestInit)This maintains type safety while allowing the custom dispatcher property.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
💡 MINOR: Test missing coverage for new logFinalRequest parameter
The test at line 3071 uses createRecordingErrorFinalRequestLogger but doesn't verify logFinalRequest behavior matches expected logging format for terminal transport errors.
Fix: Add assertions to verify logFinalRequest is called with expected parameters matching the new terminal error handling:
expect(logFinalRequest).toHaveBeenCalledWith(
502, // status code
account.label, // label
account.type, // type
"stream_error",// errorType
"socket reset (ECONNRESET)", // errorMessage
);This ensures the new logging behavior is properly tested and documented.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Summary
This PR makes critical improvements to the Claude proxy routing system, specifically addressing stream failure handling and retry logic. Key changes include:
-
New Undici Agent-based upstream fetch: Adds proper timeout handling (15-minute headers/body timeouts) for long-running Anthropic streaming requests, replacing Node's default 300s global fetch timeout.
-
Changed retry behavior for stream failures: Changed
retryablefromtruetofalsewhen transport fails before receiving the first chunk. This prevents duplicate work by avoiding replaying POST requests that may have already started processing on the upstream. -
New terminal error handler: Added
finalizeAnthropicTerminalTransportErrorfunction that creates client error responses instead of signaling retries for transport-level failures. -
Narrowed retryable network errors: Updated
isRetryableNetworkErrorto only return true for connection-establishment failures (ECONNRESET, ECONNREFUSED, etc.) rather than including transport errors like ETIMEDOUT or UND_ERR_SOCKET.
Impact Analysis
- 9 execution flows affected (per impact analysis)
- Risk score: 0.75 (moderate-high)
- Changes primarily affect Claude proxy streaming paths
- Test file additions verify new behavior
Findings
⚠️ MAJOR: Type assertion bypasses RequestInit requirements at line 219- 💡 MINOR: Test missing coverage for new logFinalRequest parameter at line 3071
Decision: CHANGES_REQUESTED
The PR addresses important reliability issues but requires fixes for type safety and test coverage before merging. The behavioral changes to retry logic are significant and correctly motivated (preventing duplicate work), but should be carefully tested in production.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
This PR makes critical improvements to the Claude proxy routing system, specifically addressing stream failure handling and retry logic. Key changes include:
- New Undici Agent-based upstream fetch with proper 15-minute timeout handling for long-running Anthropic streaming requests
- Changed retry behavior from
retryable: truetoretryable: falsewhen transport fails before receiving first chunk (prevents duplicate work) - Added
finalizeAnthropicTerminalTransportErrorfunction for terminal error handling - Narrowed
isRetryableNetworkErrorto only connection-establishment failures
Impact: 9 execution flows affected, risk score 0.75.
Findings:
- MAJOR: Type assertion bypasses RequestInit requirements at line 219 - type safety issue
- MINOR: Test missing coverage for new logFinalRequest parameter at line 3071
The behavioral changes to retry logic are correctly motivated but require fixes for type safety and test coverage.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
This PR makes critical improvements to the Claude proxy routing system, specifically addressing stream failure handling and retry logic. Key changes include:
- New Undici Agent-based upstream fetch with proper 15-minute timeout handling for long-running Anthropic streaming requests
- Changed retry behavior from
retryable: truetoretryable: falsewhen transport fails before receiving first chunk (prevents duplicate work) - Added
finalizeAnthropicTerminalTransportErrorfunction for terminal error handling - Narrowed
isRetryableNetworkErrorto only connection-establishment failures
Impact: 9 execution flows affected, risk score 0.75.
Findings:
- MAJOR: Type assertion bypasses RequestInit requirements at line 219 - type safety issue
- MINOR: Test missing coverage for new logFinalRequest parameter at line 3071
The behavioral changes to retry logic are correctly motivated but require fixes for type safety and test coverage.
🛡️ Yama Review Verdict: CHANGES_REQUESTEDSeverity counts — 🔒 CRITICAL: 0 · PR #1269 refactors Claude proxy routes with two major changes: (1) new fetchAnthropicUpstream() using Undici Agent for timeout alignment, and (2) changed retry logic to prevent replaying ambiguous POST requests across accounts. Impact analysis shows 9 affected flows, 500 nodes impacted across 39 files, risk score 0.75 (HIGH). Five findings were submitted and accepted: one MAJOR concern about network error classification change (ECONNRESET/ETIMEDOUT no longer retryable), one MAJOR concern about type assertion bypassing RequestInit requirements, and three MINOR concerns about missing test coverage. The MAJOR finding requires reconsideration as it contradicts resilient proxy design principles. Findings behind this verdict
|
610217b to
4fd2e43
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
The test at line 1640 tests Consider adding a test case for error objects without a cause.code property to verify they return false. |
|
The logic assumes that ANY transport error after authentication means the POST has already been dispatched. However, there's a race condition: if the AbortSignal.timeout fires before the POST leaves the queue, the error will be classified as non-retryable even though no request was sent. Add a flag or check to distinguish between 'aborted before dispatch' vs 'aborted after dispatch'. |
|
💡 MINOR: The test at line 1640 tests Consider adding a test case for error objects without a cause.code property to verify they return false. |
|
💡 MINOR: The logic assumes that ANY transport error after authentication means the POST has already been dispatched. However, there's a race condition: if the AbortSignal.timeout fires before the POST leaves the queue, the error will be classified as non-retryable even though no request was sent. Add a flag or check to distinguish between 'aborted before dispatch' vs 'aborted after dispatch'. |
Review Summary for PR #1269Decision: CHANGES_REQUESTED Key Findings (3 accepted findings):🔒 MAJOR (Line 7370) - Network error classification changedThe 💡 MINOR (Line 209) - Missing test coverage for fetchAnthropicUpstreamThe new 💡 MINOR (Line 5232) - Missing test coverage for finalizeAnthropicTerminalTransportErrorThe new Impact on existing code:
Review scope:This PR refactors the Claude proxy route with two major changes:
The MAJOR finding requires explicit justification before merge. The MINOR findings suggest adding test coverage for the new functions to ensure correctness. Recommendation:CHANGES_REQUESTED - The MAJOR finding regarding network error classification needs reconsideration. ECONNRESET typically indicates transient network conditions that should be retried on another account. The current change contradicts the resilient proxy design principle. |
|
🎉 This PR is included in version 10.8.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Verification
pnpm exec vitest run test/proxyReliabilityHardening.test.ts test/proxyAnalysis.test.ts(86 passed)pnpm run build:cliScope
Repository-only change. The live proxy was not restarted or configured.
Summary by CodeRabbit
Bug Fixes
Tests