Repository navigation
Conversation
📝 WalkthroughWalkthroughThe PR implements circuit breaker and retry mechanisms for OpenRouter API calls. A new gobreaker dependency is added, two new gateway files define circuit breaker initialization and retry logic, and main.go is updated to wrap OpenRouter calls with these patterns and handle circuit breaker open state errors. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway
participant CircuitBreaker as Circuit Breaker
participant Retry as Retry Logic
participant OpenRouter as OpenRouter API
Client->>Gateway: Verify Request (with correlation ID)
Gateway->>CircuitBreaker: Execute OpenRouter call
alt Circuit Open
CircuitBreaker-->>Gateway: ErrOpenState
Gateway-->>Client: 503 Service Unavailable
else Circuit Closed/Half-Open
CircuitBreaker->>Retry: Attempt request
loop Up to 3 attempts
Retry->>OpenRouter: HTTP Request
alt Success (status < 500)
OpenRouter-->>Retry: Response
Retry-->>CircuitBreaker: Success
else Failure
OpenRouter-->>Retry: Error/status >= 500
Note over Retry: Backoff & Retry
end
end
alt All retries failed
Retry-->>CircuitBreaker: Error (retry failed)
CircuitBreaker-->>Gateway: Error
Gateway-->>Client: 500/504 Error
else Request succeeded
CircuitBreaker-->>Gateway: Response
Gateway->>Gateway: Decode & validate
Gateway-->>Client: 200 Success / 4xx Error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@gateway/main.go`:
- Around line 551-554: The current call to openRouterCB.Execute passes the
original *http.Request (req) into doRequestWithRetry which causes request body
reuse failures on retries; after refactoring doRequestWithRetry to accept body
bytes and headers, change this Execute wrapper to call doRequestWithRetry with
the context, HTTP method, target URL, the request body bytes (reqBody) and
req.Header (e.g., doRequestWithRetry(ctx, "POST", openRouterURL, reqBody,
req.Header)) so each retry uses a fresh body reader and preserved headers
instead of the consumed req.
- Around line 576-584: The code currently uses unchecked type assertions for
resultBody -> choices -> choice -> message -> content which can panic; update
the parsing to use the comma-ok idiom step-by-step: assert choices as
[]interface{} with ok, ensure len>0, assert choices[0] as map[string]interface{}
with ok, assert its "message" key as map[string]interface{} with ok, then assert
"content" as string with ok; on any failed assertion return a clear error
instead of allowing a panic (reference the variables resultBody, choices,
choice, message, content).
- Around line 568-569: The code does a direct type assertion
result.(*http.Response) which will panic if result is nil (e.g., when
doRequestWithRetry returned (nil, error)); modify the Execute flow to check for
a nil result and the associated error before asserting: verify result != nil
(and/or check the error returned from doRequestWithRetry), handle or return the
error immediately if present, then perform the type assertion and defer
resp.Body.Close only after resp is confirmed non-nil; reference the
variables/result from doRequestWithRetry and the resp variable where the type
assertion currently occurs to implement this guard.
In `@gateway/openrouter_retry.go`:
- Around line 9-26: doRequestWithRetry currently reuses the same *http.Request
(body consumed) and leaks resp.Body on 5xx; change its signature to accept
either a request factory (func() (*http.Request, error)) or the body bytes plus
a method/URL so a fresh *http.Request is built per attempt, ensure each attempt
uses http.DefaultClient.Do on a newly constructed request, and always close
resp.Body when status >= 500 (or any non-success) before retrying; update
callers (e.g., where doRequestWithRetry is invoked in main.go) to supply the
builder/bytes so retries send a full body.
🧹 Nitpick comments (2)
gateway/openrouter_breaker.go (1)
11-19: Consider adding state change logging for observability.The circuit breaker configuration looks reasonable. However, adding an
OnStateChangecallback would provide valuable observability into when the circuit opens/closes, which aids debugging and monitoring in production.♻️ Suggested improvement
func init() { openRouterCB = gobreaker.NewCircuitBreaker(gobreaker.Settings{ Name: "OpenRouter", MaxRequests: 5, Timeout: 30 * time.Second, ReadyToTrip: func(c gobreaker.Counts) bool { return c.ConsecutiveFailures >= 3 }, + OnStateChange: func(name string, from gobreaker.State, to gobreaker.State) { + log.Printf("Circuit breaker %s: %s -> %s", name, from, to) + }, }) }gateway/main.go (1)
58-62: Unused struct definition.
VerifyResponseInternalappears to be unused in the provided code. If this is scaffolding for future work, consider adding a TODO comment or removing it to avoid dead code.
|
|
||
| for i := 0; i < 3; i++ { | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) |
There was a problem hiding this comment.
http.Client.Do consumes the request body stream
On the second iteration of the loop (the first retry), req.Body is already at EOF (empty)
The retry will send an empty body to OpenRouter, guaranteeing failure
You need to use req.GetBody to reset the body stream before every attempt
| return resp, nil | ||
| } | ||
|
|
||
| time.Sleep(backoff) |
There was a problem hiding this comment.
time.Sleep blocks blindly
If the user cancels the request or the 30s global timeout hits during this sleep, the goroutine keeps waiting unnecessarily
Better should use a select statement checking req.Context().Done() to exit immediately
If the loop reaches this line because of a 5xx status code (from line 17), the previous resp.Body was never closed
This leaks a network connection for every retry attempt
Use resp.Body.Close() before sleeping
| if !ok { | ||
| return "", fmt.Errorf("invalid response from AI provider: missing content") | ||
| } | ||
| choice := choices[0].(map[string]interface{}) |
There was a problem hiding this comment.
This is an unsafe type assertion
If OpenRouter returns a malformed structure or null for this field, the specific type assertion will fail and cause the entire Gateway server to panic and crash
Please use the safe comma-ok idiom: choice, ok := choices[0].(...)
… close response bodies, context-aware backoff
AnkanMisra
added
level:intermediate
Closes #101
Summary
Adds resilience to OpenRouter requests by introducing:
• Circuit breaker (sony/gobreaker)
• Exponential backoff retries (3 attempts)
• Immediate 503 when provider is unhealthy
Behavior
Changes
Summary by CodeRabbit
Greptile Overview
Greptile Summary
This PR adds circuit breaker and retry logic to OpenRouter API calls to improve resilience, but contains multiple critical bugs that will cause production issues.
Critical Issues:
http.Requestbody is consumed after first attemptArchitecture:
The retry implementation needs to be rewritten to handle request body consumption, close response bodies properly, respect context cancellation, and avoid retrying non-transient errors.
Confidence Score: 1/5
gateway/openrouter_retry.gorequires complete rewrite,gateway/main.goneeds type assertion safety restoredImportant Files Changed
callOpenRouter, adds 503 handling for open circuit, but removes safe type assertions causing potential panicsSequence Diagram
sequenceDiagram participant Client participant Gateway participant CircuitBreaker participant RetryLogic participant OpenRouter Client->>Gateway: POST /summarize Gateway->>CircuitBreaker: Execute(doRequestWithRetry) alt Circuit Open CircuitBreaker-->>Gateway: ErrOpenState Gateway-->>Client: 503 Service Unavailable else Circuit Closed/Half-Open CircuitBreaker->>RetryLogic: doRequestWithRetry() loop Retry up to 3 times RetryLogic->>OpenRouter: POST /chat/completions alt Success (< 500) OpenRouter-->>RetryLogic: 2xx/4xx response RetryLogic-->>CircuitBreaker: *http.Response CircuitBreaker-->>Gateway: Success Gateway->>Gateway: Parse JSON response Gateway-->>Client: 200 OK with summary else Server Error (5xx) OpenRouter-->>RetryLogic: 5xx response Note over RetryLogic: Body not closed (leak) RetryLogic->>RetryLogic: Sleep with exponential backoff else Network Error OpenRouter-->>RetryLogic: connection error RetryLogic->>RetryLogic: Sleep with exponential backoff end end alt All retries failed RetryLogic-->>CircuitBreaker: error CircuitBreaker->>CircuitBreaker: Increment failure count CircuitBreaker-->>Gateway: error Gateway-->>Client: 500 Internal Server Error end end(2/5) Greptile learns from your feedback when you react with thumbs up/down!