Repository navigation
feat: add request timeout middleware with configurable limits - #36
Conversation
- Implement RequestTimeoutMiddleware with context-based cancellation - Add per-endpoint timeout configuration (AI: 30s, verifier: 2s, health: 2s, global: 60s) - Update callOpenRouter to accept context and use http.NewRequestWithContext - Update verifier calls to use context timeout with proper cancellation - Add environment variables for timeout configuration - Add comprehensive unit tests including timeout scenarios - Return 504 Gateway Timeout when deadline exceeded - Ensure no goroutine leaks with proper channel handling Fixes AnkanMisra#30
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds configurable per-request timeouts: new env vars and timeout getters, a Gin middleware that buffers and enforces per-route deadlines, context-aware verifier and OpenRouter HTTP calls (including signature change for callOpenRouter), and tests validating timeout propagation and middleware behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway as Gateway\n(Middleware)
participant Handler as Handler
participant Verifier as Verifier Service
participant OpenRouter as OpenRouter Service
autonumber
Client->>Gateway: POST /api/ai/summarize
rect rgb(220,235,255)
Note over Gateway: RequestTimeoutMiddleware sets ctx\n(global or AI-specific) and buffers response
Gateway->>Handler: Invoke handler with ctx (buffered)
end
rect rgb(240,250,230)
Note over Handler: Handler calls verifier then AI\nusing context-aware HTTP requests
Handler->>Verifier: HTTP POST with verifier ctx (~2s)
alt Verifier responds
Verifier-->>Handler: verification success
else Verifier timeout
Verifier--xHandler: context.DeadlineExceeded
Handler-->>Gateway: return 504 (verifier timeout)
Gateway-->>Client: 504 Gateway Timeout
end
end
alt Verifier success
Handler->>OpenRouter: HTTP POST with AI ctx (~30s)
alt OpenRouter responds
OpenRouter-->>Handler: AI response
Handler-->>Gateway: 200 OK + body
Gateway-->>Client: 200 OK (flush buffer)
else OpenRouter timeout
OpenRouter--xHandler: context.DeadlineExceeded
Handler-->>Gateway: 504 Gateway Timeout
Gateway-->>Client: 504 Gateway Timeout
end
end
rect rgb(250,220,220)
Note over Gateway: If middleware deadline exceeded,\nmiddleware aborts, discards buffered output, and returns 504
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
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}
There was a problem hiding this comment.
Additional Comments (4)
-
gateway/middleware.go, line 34-48 (link)logic: Race condition: the middleware can return with
c.AbortWithStatusJSON(504, ...)while the handler goroutine (line 30:c.Next()) is still writing to the response. Gin'sContextis not goroutine-safe. If the handler writes after line 44 executes, you'll get concurrent writes to the same HTTP response.Gin doesn't provide a built-in way to safely abort handlers running in goroutines. The standard pattern is to let the handler check
ctx.Done()and return early, rather than racing to write the response. -
gateway/main.go, line 143-150 (link)logic: Double timeout middleware creates redundancy: requests to
/api/ai/summarizepass through both the global timeout (60s) and the AI-specific timeout (30s). The tighter 30s limit applies, but this creates two nested timeout contexts and two concurrent goroutines waiting on the same request.If the intention is to have AI endpoints use 30s instead of 60s, remove the global middleware from AI routes. If both are needed for different purposes, clarify the design. Should AI endpoints have both a 60s global timeout AND a 30s AI-specific timeout, or just the 30s timeout?
-
gateway/main.go, line 146 (link)logic: Redundant timeout: health check gets both the global 60s timeout and a 2s timeout. The 2s limit applies but wastes resources with double context cancellation.
-
gateway/timeout_test.go, line 19-22 (link)style: Test doesn't prove timeout middleware works correctly because it assumes the handler goroutine stops executing. With the race condition in the middleware, the handler continues after timeout and could write to
c.JSON(200, ...)on line 22, causing concurrent response writes.
6 files reviewed, 4 comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
gateway/main.go (1)
168-176: Consider renaming for consistency with the fix at line 179.While this
contextvariable is scoped within the if-block and benign (the function returns before anycontext.package usage), renaming it topayCtx(or similar) alongside the fix at line 179 would improve consistency and avoid future confusion.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.examplegateway/config.gogateway/config_test.gogateway/main.gogateway/middleware.gogateway/timeout_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
gateway/timeout_test.go (1)
gateway/middleware.go (1)
RequestTimeoutMiddleware(13-51)
gateway/main.go (1)
gateway/middleware.go (1)
RequestTimeoutMiddleware(13-51)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 48-48: [UnorderedKey] The AI_REQUEST_TIMEOUT_SECONDS key should go before the REQUEST_TIMEOUT_SECONDS key
(UnorderedKey)
[warning] 52-52: [UnorderedKey] The HEALTH_CHECK_TIMEOUT_SECONDS key should go before the REQUEST_TIMEOUT_SECONDS key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Greptile Review
🔇 Additional comments (7)
.env.example (1)
44-52: LGTM! Timeout configuration is well-documented.The timeout environment variables are clearly documented with sensible defaults. The static analysis warnings about key ordering are stylistic preferences from dotenv-linter—grouping related timeout configurations together (global, AI-specific, verifier, health check) is more readable than strict alphabetical ordering.
gateway/config.go (1)
1-24: LGTM! Clean and consistent timeout configuration helpers.The timeout helper functions follow a clear, consistent pattern with sensible defaults that match the documented values in
.env.example. The implementation is straightforward and correct.gateway/middleware.go (1)
13-51: LGTM! Solid timeout middleware implementation with proper goroutine management.The middleware correctly uses context-based timeouts with buffered channels to prevent goroutine leaks. The pattern of running handlers in a separate goroutine while waiting on the context's
Done()channel is the standard approach for timeout middleware in Go.Key strengths:
- Buffered channels (size 1) ensure no goroutine blocks indefinitely
- Deferred
cancel()ensures proper cleanup- Panic recovery and re-panic preserves Gin's default behavior
- Clear 504 response on timeout
Note: The handler goroutine continues executing after timeout (acknowledged in your comment on lines 42-43). This is a known limitation of timeout middleware patterns—Gin's
AbortWithStatusJSONprevents the handler from writing to an already-sent response, so this is acceptable.gateway/timeout_test.go (2)
15-36: LGTM! Clear test of middleware timeout behavior.The test correctly validates that the middleware aborts with a 504 status and appropriate error message when a handler exceeds the timeout.
38-63: LGTM! Effective test of context timeout propagation.The test properly validates that
callOpenRouterrespects the context deadline by setting up a slow mock server and asserting a timeout error is returned.gateway/main.go (2)
142-151: Timeout middleware stacking is intentional but worth noting.The global timeout middleware (line 143) stacks with per-route timeouts (lines 146, 150). Since child contexts inherit parent deadlines, the effective timeout is
min(global, route-specific). This works correctly when route-specific timeouts are shorter than the global (which they are: 2s health, 30s AI vs. 60s global).If requirements change and a route needs a timeout longer than the global, the global would still cap it. Consider whether to remove the global middleware and rely solely on per-route timeouts, or document this constraint.
325-329: Context deadline check is correctly implemented here.Unlike
handleSummarize, this function doesn't shadow thecontextpackage, socontext.DeadlineExceededcorrectly references the standard library constant. The timeout detection logic is sound.
There was a problem hiding this comment.
Pull request overview
This PR implements request timeout middleware to prevent slow or hanging requests from consuming server resources. It introduces context-based timeout management with configurable limits for different endpoint types.
Key Changes:
- Added
RequestTimeoutMiddlewarewith goroutine-based timeout detection and context propagation - Configured per-endpoint timeouts: global (60s), AI endpoints (30s), verifier (2s), and health checks (2s)
- Updated
callOpenRouterand verifier calls to respect context deadlines
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| gateway/middleware.go | Implements timeout middleware with context cancellation, goroutine monitoring, and 504 response handling |
| gateway/config.go | Provides timeout configuration helper functions that read from environment variables |
| gateway/main.go | Applies timeout middleware to routes, updates service calls to use context-based timeouts, and handles timeout errors |
| gateway/timeout_test.go | Adds comprehensive tests for middleware timeout behavior and AI request timeout scenarios |
| gateway/config_test.go | Tests timeout configuration helper functions with default and custom values |
| .env.example | Documents new timeout-related environment variables with default values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| seconds := getEnvAsInt("REQUEST_TIMEOUT_SECONDS", 60) | ||
| return time.Duration(seconds) * time.Second | ||
| } | ||
|
|
||
| func getAITimeout() time.Duration { | ||
| seconds := getEnvAsInt("AI_REQUEST_TIMEOUT_SECONDS", 30) | ||
| return time.Duration(seconds) * time.Second | ||
| } | ||
|
|
||
| func getVerifierTimeout() time.Duration { | ||
| seconds := getEnvAsInt("VERIFIER_TIMEOUT_SECONDS", 2) | ||
| return time.Duration(seconds) * time.Second | ||
| } | ||
|
|
||
| func getHealthCheckTimeout() time.Duration { | ||
| seconds := getEnvAsInt("HEALTH_CHECK_TIMEOUT_SECONDS", 2) |
There was a problem hiding this comment.
The timeout helper functions don't validate that the configured values are positive. If someone sets an environment variable like REQUEST_TIMEOUT_SECONDS=0 or a negative value, it will create a zero or negative duration timeout. While context.WithTimeout will treat zero/negative durations as "already expired," this could lead to confusing behavior where all requests immediately timeout. Consider adding validation to ensure timeout values are positive, or document the minimum acceptable values.
| seconds := getEnvAsInt("REQUEST_TIMEOUT_SECONDS", 60) | |
| return time.Duration(seconds) * time.Second | |
| } | |
| func getAITimeout() time.Duration { | |
| seconds := getEnvAsInt("AI_REQUEST_TIMEOUT_SECONDS", 30) | |
| return time.Duration(seconds) * time.Second | |
| } | |
| func getVerifierTimeout() time.Duration { | |
| seconds := getEnvAsInt("VERIFIER_TIMEOUT_SECONDS", 2) | |
| return time.Duration(seconds) * time.Second | |
| } | |
| func getHealthCheckTimeout() time.Duration { | |
| seconds := getEnvAsInt("HEALTH_CHECK_TIMEOUT_SECONDS", 2) | |
| seconds := getEnvAsInt("REQUEST_TIMEOUT_SECONDS", 60) | |
| if seconds <= 0 { | |
| seconds = 60 | |
| } | |
| return time.Duration(seconds) * time.Second | |
| } | |
| func getAITimeout() time.Duration { | |
| seconds := getEnvAsInt("AI_REQUEST_TIMEOUT_SECONDS", 30) | |
| if seconds <= 0 { | |
| seconds = 30 | |
| } | |
| return time.Duration(seconds) * time.Second | |
| } | |
| func getVerifierTimeout() time.Duration { | |
| seconds := getEnvAsInt("VERIFIER_TIMEOUT_SECONDS", 2) | |
| if seconds <= 0 { | |
| seconds = 2 | |
| } | |
| return time.Duration(seconds) * time.Second | |
| } | |
| func getHealthCheckTimeout() time.Duration { | |
| seconds := getEnvAsInt("HEALTH_CHECK_TIMEOUT_SECONDS", 2) | |
| if seconds <= 0 { | |
| seconds = 2 | |
| } |
| summary, err := callOpenRouter(c.Request.Context(), req.Text) | ||
| if err != nil { | ||
| // If the error was due to a timeout, return 504 | ||
| if strings.Contains(strings.ToLower(err.Error()), "timeout") || c.Request.Context().Err() == context.DeadlineExceeded { |
There was a problem hiding this comment.
The timeout error detection is checking both the context error AND using string matching on the error message. However, using string matching with "timeout" is fragile and could match unrelated errors. Since callOpenRouter explicitly returns an error when ctx.Err() == context.DeadlineExceeded (line 326-327), and the verifier check already validates verifierCtx.Err() (line 206), the string matching is redundant. Consider only checking c.Request.Context().Err() == context.DeadlineExceeded for consistency.
| if strings.Contains(strings.ToLower(err.Error()), "timeout") || c.Request.Context().Err() == context.DeadlineExceeded { | |
| if c.Request.Context().Err() == context.DeadlineExceeded { |
| client := &http.Client{} | ||
| resp, err := client.Do(vreq) | ||
| if err != nil { | ||
| if verifierCtx.Err() == context.DeadlineExceeded { |
There was a problem hiding this comment.
The verifier timeout check only examines verifierCtx.Err() but doesn't consider that the parent context (c.Request.Context()) might have timed out first. If the parent middleware timeout fires before the verifier timeout, verifierCtx will be canceled but its Err() might be context.Canceled rather than context.DeadlineExceeded. This could result in returning a generic "Verification service unavailable" error instead of the more specific "Gateway Timeout" error. Consider also checking if c.Request.Context().Err() == context.DeadlineExceeded.
| if verifierCtx.Err() == context.DeadlineExceeded { | |
| if verifierCtx.Err() == context.DeadlineExceeded || c.Request.Context().Err() == context.DeadlineExceeded { |
| t.Fatalf("Expected duration around 1s, got %v", dur) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Extra blank line at the end of the file. Remove the trailing blank line.
| // Global request timeout middleware | ||
| r.Use(RequestTimeoutMiddleware(getRequestTimeout())) | ||
|
|
||
| // Health check with shorter timeout | ||
| r.GET("/healthz", RequestTimeoutMiddleware(getHealthCheckTimeout()), handleHealth) | ||
|
|
||
| // AI endpoints with AI-specific timeout | ||
| aiGroup := r.Group("/api/ai") | ||
| aiGroup.Use(RequestTimeoutMiddleware(getAITimeout())) | ||
| aiGroup.POST("/summarize", handleSummarize) |
There was a problem hiding this comment.
Multiple timeout middlewares are layered on the same routes, creating unnecessary goroutines. The global timeout (60s) at line 143 applies to ALL routes including /healthz and /api/ai/*, which then apply their own shorter timeouts (2s and 30s respectively). While this works correctly because the shorter timeout fires first, it's inefficient as it creates multiple monitoring goroutines per request. Consider restructuring to apply the global timeout only to routes without specific timeouts, or remove the global timeout entirely if all routes have specific timeouts.
| } | ||
|
|
||
| // Ensure it timed out approximately around 1s (give small margin) | ||
| if dur < 900*time.Millisecond || dur > 3*time.Second { |
There was a problem hiding this comment.
The timeout duration check has an overly permissive upper bound of 3 seconds when the timeout is set to 1 second. This test is checking that the timeout fires around 1 second, but allows up to 3 seconds, which could hide bugs where the timeout doesn't work properly. Consider tightening the upper bound to something like 1.5 seconds (1500ms) to catch regressions more reliably.
| if dur < 900*time.Millisecond || dur > 3*time.Second { | |
| if dur < 900*time.Millisecond || dur > 1500*time.Millisecond { |
| t.Fatalf("Expected duration around 1s, got %v", dur) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
There's no test case that verifies the middleware allows successful requests (that complete within the timeout) to proceed normally. All test cases focus on timeout scenarios. Consider adding a test that verifies a fast handler completes successfully and returns 200, ensuring the middleware doesn't interfere with normal operation.
|
@greptile review the pr and find all the issues |
There was a problem hiding this comment.
Additional Comments (4)
-
gateway/main.go, line 203 (link)logic: HTTP client should have timeout configured -
&http.Client{}has no timeout, so even with context timeout, the underlying TCP connection can hang indefinitely -
gateway/main.go, line 322 (link)logic: HTTP client should have timeout configured -
&http.Client{}has no timeout, so even with context timeout, the underlying TCP connection can hang indefinitely -
gateway/middleware.go, line 34-49 (link)logic: After timeout (line 44), the handler goroutine continues running and may try to write to the response after
AbortWithStatusJSONalready sent 504. This creates a race condition where the handler might write headers/body after the response is complete.The buffered channels prevent goroutine leaks, but there's no mechanism to stop the handler from writing to
cafter timeout. Consider checkingc.IsAborted()in handlers or usingc.Writer.Written()checks. Does your application handle the case where handlers attempt to write after AbortWithStatusJSON, or do handlers check c.IsAborted() before writing? -
gateway/main.go, line 143 (link)style: Global timeout middleware applies to ALL routes including
/docsand/openapi.yaml. Static file serving and documentation don't need 60s timeouts and could interfere with slow downloads.
6 files reviewed, 4 comments
|
@SujalTripathi Please Read all the docs, contributor, how to run test and other docs which are pretty well written This is a medium problem don't vibe code in 1 hour |
|
@greptile give the changes have to do now |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
gateway/timeout_test.go (1)
151-151: Remove trailing blank line.🔎 Proposed fix
t.Fatalf("Handler response leaked after timeout: %s", w.Body.String()) } } -
🧹 Nitpick comments (1)
gateway/timeout_test.go (1)
114-135: Consider extracting inline tests into separate test functions.The fast handler test (lines 116-124) and panic recovery test (lines 127-135) are embedded within
TestHandleSummarize_AIRequestTimeoutReturns504. While they test related middleware behavior, extracting them into dedicated functions (e.g.,TestRequestTimeoutMiddleware_AllowsFastHandlersandTestRequestTimeoutMiddleware_PreservesPanicRecovery) would improve test organization and make failures easier to diagnose.🔎 Proposed structure
Create two separate test functions:
func TestRequestTimeoutMiddleware_AllowsFastHandlers(t *testing.T) { gin.SetMode(gin.TestMode) r := gin.New() r.Use(RequestTimeoutMiddleware(1 * time.Second)) r.GET("/fast", func(c *gin.Context) { c.JSON(200, gin.H{"ok": true}) }) req, _ := http.NewRequest("GET", "/fast", nil) w := httptest.NewRecorder() r.ServeHTTP(w, req) if w.Code != 200 { t.Fatalf("Expected 200 for fast handler, got %d", w.Code) } } func TestRequestTimeoutMiddleware_PreservesPanicRecovery(t *testing.T) { gin.SetMode(gin.TestMode) r := gin.Default() // includes Recovery middleware r.Use(RequestTimeoutMiddleware(1 * time.Second)) r.GET("/panic", func(c *gin.Context) { panic("boom") }) req, _ := http.NewRequest("GET", "/panic", nil) w := httptest.NewRecorder() r.ServeHTTP(w, req) if w.Code != 500 { t.Fatalf("Expected 500 from panic + recovery, got %d", w.Code) } }Then remove lines 114-135 from the current test.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gateway/config.gogateway/config_test.gogateway/main.gogateway/middleware.gogateway/timeout_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/config.go
🧰 Additional context used
🧬 Code graph analysis (2)
gateway/timeout_test.go (1)
gateway/middleware.go (1)
RequestTimeoutMiddleware(81-127)
gateway/main.go (2)
gateway/middleware.go (1)
RequestTimeoutMiddleware(81-127)src/services/payment.ts (1)
PaymentContext(8-14)
🔇 Additional comments (5)
gateway/config_test.go (1)
26-65: LGTM! Comprehensive timeout configuration testing.The test properly validates default timeouts, custom environment variable overrides, and fallback behavior for non-positive values. The addition of
getHealthCheckTimeout()coverage addresses the previous review feedback.gateway/middleware.go (1)
81-127: Solid timeout implementation with proper buffering and cleanup.The middleware correctly:
- Runs handlers in a separate goroutine with panic recovery
- Buffers responses to avoid write races when timeouts occur
- Uses a buffered panic channel to prevent goroutine blocking
- Returns 504 with a clean JSON response on timeout while discarding the handler's buffered output
The context propagation (line 88) ensures downstream handlers can detect cancellation and stop work appropriately.
gateway/main.go (3)
146-152: Clean per-route timeout configuration.The health check and AI endpoints now have appropriate timeouts applied (2s and 30s respectively). The absence of a global timeout middleware avoids the inefficiency of layered timeout goroutines mentioned in previous reviews.
206-211: Proper error handling and timeout detection for verifier calls.The fixes address critical issues from previous reviews:
- Error from
http.NewRequestWithContextis now properly checked (lines 206-211)- Timeout detection checks both the verifier context and parent request context (line 217), handling the case where the parent middleware timeout fires first
Also applies to: 217-220
329-332: Context-aware OpenRouter implementation with proper error handling.The
callOpenRouterfunction now correctly:
- Accepts a context parameter for cancellation support
- Handles request creation errors (lines 329-332)
- Returns
context.DeadlineExceededwhen the context times out (lines 340-342), enabling callers to distinguish timeout errors from other failuresAlso applies to: 340-342
There was a problem hiding this comment.
Looking at the PR, you've implemented a comprehensive timeout middleware solution, but there are a few critical issues that need to be addressed before this can be merged:
Critical Issues to Fix:
1. HTTP Client Timeout Configuration
The most critical issue is that your HTTP clients are created without timeout configuration:
In gateway/main.go lines 213 and 336:
client := &http.Client{} // No timeout configured!This means even with your context timeouts, the underlying TCP connections can still hang indefinitely. You need to configure the HTTP client with a timeout:
client := &http.Client{
Timeout: 30 * time.Second, // or use the context timeout value
}
2. Redundant Timeout Middleware Layering
You're applying timeout middleware at both the global level and per-endpoint level, which creates confusion about which timeout actually applies. In main.go, you have:
- Health endpoint:
RequestTimeoutMiddleware(getHealthCheckTimeout())(2s) - AI group:
RequestTimeoutMiddleware(getAITimeout())(30s)
But there's no global timeout middleware applied, so this is actually fine. However, the architecture suggests you intended to have layered timeouts.
3. Minor: Error Handling Consistency
In the verifier timeout handling, you check both the verifier context and the parent request context for deadline exceeded. This is redundant since the verifier context is derived from the parent context.
Recommended Changes:
- Fix HTTP client timeouts (most important)
- Consider simplifying the timeout architecture - either use global + per-endpoint or just per-endpoint
- Add HTTP client timeout configuration to prevent hanging connections
The middleware implementation itself is solid with good goroutine management and panic handling. The main issue is that the HTTP clients bypass your context timeouts entirely.
… implement response writer methods; ensure panic and timeout behavior
|
@greptile @CodeRabbit full review and give changes have to do here |
🧩 Analysis chain🏁 Script executed: Length of output: 31677 @SujalTripathi - I've completed a full review of PR #36. Here are the critical bugs and issues that need to be fixed: 🚨 Critical Issues1. Missing Global Timeout MiddlewareLocation: Fix Required: r := gin.Default()
// Add global timeout middleware BEFORE other routes
r.Use(RequestTimeoutMiddleware(getRequestTimeout()))
// Then add specific route groups
r.GET("/healthz", RequestTimeoutMiddleware(getHealthCheckTimeout()), handleHealth)2. Code Indentation ErrorLocation: Current (broken): origWriter := c.Writer
bw := newBufferedWriter()
// replace the gin writer with a shim that uses bw and keeps orig writer
c.Writer = &responseWriterShim{bw: bw, orig: origWriter}
finished := make(chan struct{})
panicChan := make(chan interface{}, 1)Should be: origWriter := c.Writer
bw := newBufferedWriter()
// replace the gin writer with a shim that uses bw and keeps orig writer
c.Writer = &responseWriterShim{bw: bw, orig: origWriter}
finished := make(chan struct{})
panicChan := make(chan interface{}, 1)3. Redundant and Incorrect Timeout LogicLocation: Current (broken) in handleSummarize: verifierCtx, verifierCancel := context.WithTimeout(c.Request.Context(), getVerifierTimeout())
defer verifierCancel()
vreq, err := http.NewRequestWithContext(verifierCtx, "POST", verifierURL+"/verify", bytes.NewBuffer(verifyBody))
// ... then later calculates deadline and creates client with timeout
if deadline, ok := verifierCtx.Deadline(); ok {
remaining := time.Until(deadline)
if remaining <= 0 {
c.JSON(504, gin.H{"error": "Gateway Timeout", "message": "Verifier request timed out"})
return
}
client := &http.Client{Timeout: remaining}
resp, err = client.Do(vreq)
}Fix - Simplify to: verifierCtx, verifierCancel := context.WithTimeout(c.Request.Context(), getVerifierTimeout())
defer verifierCancel()
vreq, err := http.NewRequestWithContext(verifierCtx, "POST", verifierURL+"/verify", bytes.NewBuffer(verifyBody))
if err != nil {
c.JSON(500, gin.H{"error": "Invalid verifier request", "details": err.Error()})
return
}
vreq.Header.Set("Content-Type", "application/json")
// Use http.DefaultClient (or create one without timeout - context handles it)
resp, err := http.DefaultClient.Do(vreq)
if err != nil {
if errors.Is(err, context.DeadlineExceeded) {
c.JSON(504, gin.H{"error": "Gateway Timeout", "message": "Verifier request timed out"})
return
}
c.JSON(500, gin.H{"error": "Verification service unavailable"})
return
}
defer resp.Body.Close()Same issue in callOpenRouter - remove the complex deadline logic: req, err := http.NewRequestWithContext(ctx, "POST", openRouterURL, bytes.NewBuffer(reqBody))
if err != nil {
return "", fmt.Errorf("failed to create OpenRouter request: %w", err)
}
req.Header.Set("Authorization", "Bearer "+apiKey)
req.Header.Set("Content-Type", "application/json")
// Just use DefaultClient - context already has timeout
resp, err := http.DefaultClient.Do(req)
if err != nil {
if errors.Is(err, context.DeadlineExceeded) {
return "", context.DeadlineExceeded
}
return "", err
}
defer resp.Body.Close()4. Test Structure ViolationsLocation: Lines 85-100 test fast handler and panic recovery - these should be separate test functions: Fix - Split into: func TestRequestTimeoutMiddleware_AllowsFastHandlers(t *testing.T) {
gin.SetMode(gin.TestMode)
r := gin.New()
r.Use(RequestTimeoutMiddleware(1 * time.Second))
r.GET("/fast", func(c *gin.Context) { c.JSON(200, gin.H{"ok": true}) })
req, _ := http.NewRequest("GET", "/fast", nil)
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != 200 {
t.Fatalf("Expected 200 for fast handler, got %d", w.Code)
}
}
func TestRequestTimeoutMiddleware_PreservesPanicRecovery(t *testing.T) {
gin.SetMode(gin.TestMode)
r := gin.Default() // Uses Recovery middleware
r.Use(RequestTimeoutMiddleware(1 * time.Second))
r.GET("/panic", func(c *gin.Context) { panic("boom") })
req, _ := http.NewRequest("GET", "/panic", nil)
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != 500 {
t.Fatalf("Expected 500 from panic + recovery, got %d", w.Code)
}
}Remove lines 82-98 from 5. Fragile File PathLocation: Fix: err := godotenv.Load(".env") // Look in current directory
if err != nil {
// Try parent directory as fallback
err = godotenv.Load("../.env")
if err != nil {
log.Println("Warning: Error loading .env file")
}
}Or better - use
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
gateway/main.go (1)
349-362: Same redundant timeout pattern as the verifier call.This code duplicates the redundant timeout mechanism (context deadline + client timeout) seen in the verifier call. The same recommendation applies: rely on the request context's deadline for cancellation instead of also setting
http.Client.Timeout.🔎 Simplified approach using only context timeout
req, err := http.NewRequestWithContext(ctx, "POST", openRouterURL, bytes.NewBuffer(reqBody)) if err != nil { return "", fmt.Errorf("failed to create OpenRouter request: %w", err) } req.Header.Set("Authorization", "Bearer "+apiKey) req.Header.Set("Content-Type", "application/json") - // Configure HTTP client timeout based on request context's deadline if present - var resp *http.Response - if deadline, ok := ctx.Deadline(); ok { - remaining := time.Until(deadline) - if remaining <= 0 { - return "", context.DeadlineExceeded - } - client := &http.Client{Timeout: remaining} - resp, err = client.Do(req) - } else { - // No deadline on ctx — use configured AI timeout as a sensible default - client := &http.Client{Timeout: getAITimeout()} - resp, err = client.Do(req) - } + + // Context deadline will automatically cancel the request + client := &http.Client{} + resp, err := client.Do(req)If you want a fallback when no context deadline is set, keep the
elsebranch.
🧹 Nitpick comments (3)
gateway/middleware.go (2)
126-126: Usehttp.StatusGatewayTimeoutconstant instead of hardcoded 504.Replace the magic number with the standard library constant for better readability and maintainability.
🔎 Proposed fix
- origWriter.WriteHeader(504) + origWriter.WriteHeader(http.StatusGatewayTimeout)
95-96: Inconsistent indentation.Line 95's comment and line 96's assignment have mismatched indentation. Align the comment with the surrounding code.
🔎 Proposed fix
origWriter := c.Writer bw := newBufferedWriter() -// replace the gin writer with a shim that uses bw and keeps orig writer -c.Writer = &responseWriterShim{bw: bw, orig: origWriter} -finished := make(chan struct{}) + // replace the gin writer with a shim that uses bw and keeps orig writer + c.Writer = &responseWriterShim{bw: bw, orig: origWriter} + finished := make(chan struct{})gateway/main.go (1)
202-227: Redundant timeout mechanism: context deadline + client timeout.The code uses both context-based cancellation (via
verifierCtxinNewRequestWithContext) andhttp.Client.Timeout. Since the request context already has a deadline (line 203), the request will be automatically canceled when the context times out. Settingclient.Timeoutto the remaining deadline time (line 221) is redundant.Recommendation: Remove the client timeout configuration (lines 214-227) and rely solely on the context deadline. This simplifies the code and eliminates potential confusion about which timeout fires first.
🔎 Simplified approach using only context timeout
vreq, err := http.NewRequestWithContext(verifierCtx, "POST", verifierURL+"/verify", bytes.NewBuffer(verifyBody)) if err != nil { c.JSON(500, gin.H{"error": "Invalid verifier request", "details": err.Error()}) return } vreq.Header.Set("Content-Type", "application/json") - // Configure HTTP client with a timeout derived from the verifier context - var resp *http.Response - if deadline, ok := verifierCtx.Deadline(); ok { - remaining := time.Until(deadline) - if remaining <= 0 { - c.JSON(504, gin.H{"error": "Gateway Timeout", "message": "Verifier request timed out"}) - return - } - client := &http.Client{Timeout: remaining} - resp, err = client.Do(vreq) - } else { - // No deadline on verifierCtx - fall back to a conservative timeout - client := &http.Client{Timeout: getVerifierTimeout()} - resp, err = client.Do(vreq) - } + + // The request context will automatically cancel the request on timeout + client := &http.Client{} + resp, err := client.Do(vreq)Note: If you want a fallback timeout when no context deadline is set, keep the
elsebranch logic, but theif deadline, okbranch withremainingcalculation is unnecessary.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gateway/main.gogateway/middleware.go
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/main.go (2)
gateway/middleware.go (1)
RequestTimeoutMiddleware(84-131)src/services/payment.ts (1)
PaymentContext(8-14)
🔇 Additional comments (6)
gateway/main.go (6)
147-152: Route-specific timeouts properly implemented.The route structure correctly applies dedicated timeout middleware per endpoint (health check 2s, AI 30s) without layering multiple timeouts on the same route. This addresses the past concern about redundant timeout goroutines.
229-233: Good: Timeout detection checks both parent and child contexts.The error handling correctly checks both
verifierCtx(the child timeout) andc.Request.Context()(the parent middleware timeout) forDeadlineExceeded. This ensures that timeouts are properly detected regardless of which context fires first, addressing the previous Copilot feedback.
206-211: Good: Error handling for request creation added.The code now properly checks the error returned by
http.NewRequestWithContextand returns a 500 error if request creation fails. This prevents nil pointer panics and addresses a previous critical issue.
342-345: Good: Error handling for OpenRouter request creation added.Similar to the verifier call, the code now checks for errors when creating the OpenRouter request, preventing nil pointer panics. This addresses a previous critical issue.
257-263: Proper timeout detection for AI service calls.The error handling correctly identifies timeout errors by checking both
errors.Is(err, context.DeadlineExceeded)andc.Request.Context().Err(), ensuring 504 responses are returned for timeouts rather than generic 500 errors.
184-195: Variable shadowing issue resolved.The payment context variable is now named
paymentCtx(line 184), avoiding the shadowing of thecontextpackage that was flagged in previous reviews. The variable is correctly used in theVerifyRequeststruct (line 193).
There was a problem hiding this comment.
Additional Comments (3)
-
gateway/middleware.go, line 109-129 (link)logic: Race condition: handler goroutine can write to response after timeout.
When timeout occurs at line 121-128, the middleware sends 504 and returns. However, the handler goroutine (lines 100-108) continues running and may call
c.Next()which writes to the response. This creates a race where both the timeout handler and the request handler try to write toorigWritersimultaneously.The buffering mechanism prevents the handler's output from reaching the client, but the handler goroutine should be explicitly notified to stop processing. Consider using
c.Abort()before the timeout case, though this still doesn't stop the already-running handler goroutine. -
gateway/main.go, line 214-227 (link)style: Redundant timeout handling: both
http.Client.Timeoutand context deadline are used.When a context with deadline is passed to
http.NewRequestWithContext, the HTTP client automatically respects that deadline. Settingclient.Timeoutcreates a second, independent timeout mechanism. Ifremainingis slightly less than the actual context deadline due to timing, the client timeout might fire first, causing inconsistent error handling.The context deadline in
vreqalready handles the timeout, so explicitclient.Timeoutis unnecessary. -
gateway/main.go, line 349-362 (link)style: Redundant timeout handling: both
http.Client.Timeoutand context deadline are used.Same issue as the verifier call - the context deadline in the request already handles timeouts, making explicit
client.Timeoutredundant and potentially causing race conditions between two timeout mechanisms.
6 files reviewed, 3 comments
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
gateway/middleware.go (1)
84-131: CRITICAL: Unresolved race condition — handler can corrupt response after timeout.After the timeout fires at line 121, the middleware restores
c.Writer = origWriter(line 124) and writes the 504 response. However, the handler goroutine launched at line 100 is still running and can subsequently callc.Writer.Write()orc.JSON(), which will write toorigWriterand cause:
http: multiple response.WriteHeader callspanic- Response corruption (mixing 504 JSON with handler output)
- Data race on the underlying
http.ResponseWriterThis critical issue was flagged in the previous review but remains unfixed.
🔎 Proposed fix: Add a closed flag to prevent writes after timeout
Add a
closedflag and mutex tobufferedWriter:type bufferedWriter struct { buf *bytes.Buffer head http.Header status int wrote bool + closed bool + mu sync.RWMutex }Update write methods to check the closed flag:
func (b *bufferedWriter) Write(data []byte) (int, error) { + b.mu.RLock() + if b.closed { + b.mu.RUnlock() + return len(data), nil // Discard writes after timeout + } + b.mu.RUnlock() b.wrote = true return b.buf.Write(data) } func (b *bufferedWriter) WriteString(s string) (int, error) { + b.mu.RLock() + if b.closed { + b.mu.RUnlock() + return len(s), nil + } + b.mu.RUnlock() b.wrote = true return b.buf.WriteString(s) } func (b *bufferedWriter) WriteHeader(statusCode int) { + b.mu.RLock() + if b.closed { + b.mu.RUnlock() + return + } + b.mu.RUnlock() b.status = statusCode }In the timeout case, mark the buffer closed instead of restoring the writer:
case <-ctx.Done(): // Timeout exceeded — send 504 using the original writer and // discard buffered handler response. + bw.mu.Lock() + bw.closed = true + bw.mu.Unlock() c.Writer = origWriter origWriter.Header().Set("Content-Type", "application/json; charset=utf-8") origWriter.WriteHeader(504) _, _ = origWriter.Write([]byte(`{"error":"Gateway Timeout","message":"Request exceeded maximum allowed time"}`)) returnThis ensures that any subsequent writes from the handler goroutine are safely discarded.
🧹 Nitpick comments (1)
gateway/timeout_test.go (1)
114-135: Consider splitting unrelated test cases into separate functions.The fast handler test (lines 116-124) and panic handler test (lines 127-135) validate distinct middleware behaviors unrelated to the AI timeout scenario. Splitting these into
TestRequestTimeoutMiddleware_FastHandlerSucceedsandTestRequestTimeoutMiddleware_PanicHandlingwould improve test clarity and maintainability.🔎 Suggested structure
func TestRequestTimeoutMiddleware_FastHandlerSucceeds(t *testing.T) { gin.SetMode(gin.TestMode) r := gin.New() r.Use(RequestTimeoutMiddleware(1 * time.Second)) r.GET("/fast", func(c *gin.Context) { c.JSON(200, gin.H{"ok": true}) }) req, _ := http.NewRequest("GET", "/fast", nil) w := httptest.NewRecorder() r.ServeHTTP(w, req) if w.Code != 200 { t.Fatalf("Expected 200 for fast handler, got %d", w.Code) } } func TestRequestTimeoutMiddleware_PanicHandling(t *testing.T) { gin.SetMode(gin.TestMode) r := gin.Default() // Includes Recovery middleware r.Use(RequestTimeoutMiddleware(1 * time.Second)) r.GET("/panic", func(c *gin.Context) { panic("boom") }) req, _ := http.NewRequest("GET", "/panic", nil) w := httptest.NewRecorder() r.ServeHTTP(w, req) if w.Code != 500 { t.Fatalf("Expected 500 from panic + recovery, got %d", w.Code) } }Then remove lines 114-135 from
TestHandleSummarize_AIRequestTimeoutReturns504.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.examplegateway/config.gogateway/config_test.gogateway/main.gogateway/middleware.gogateway/timeout_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
gateway/timeout_test.go (1)
gateway/middleware.go (1)
RequestTimeoutMiddleware(84-131)
gateway/main.go (2)
gateway/middleware.go (1)
RequestTimeoutMiddleware(84-131)src/services/payment.ts (1)
PaymentContext(8-14)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 48-48: [UnorderedKey] The AI_REQUEST_TIMEOUT_SECONDS key should go before the REQUEST_TIMEOUT_SECONDS key
(UnorderedKey)
[warning] 52-52: [UnorderedKey] The HEALTH_CHECK_TIMEOUT_SECONDS key should go before the REQUEST_TIMEOUT_SECONDS key
(UnorderedKey)
🔇 Additional comments (11)
.env.example (1)
44-52: LGTM! Clear timeout configuration with sensible defaults.The new timeout configuration section is well-documented with appropriate default values: 60s global, 30s for AI endpoints, and 2s for verifier/health checks. These align with the PR objectives.
gateway/config.go (2)
5-13: LGTM! Proper validation prevents misconfiguration.The helper correctly clamps non-positive timeout values to sensible defaults, preventing configuration errors that could cause immediate request timeouts.
15-19: LGTM! Clean getter pattern with appropriate defaults.The timeout getters properly delegate to the validation helper with defaults that match the documented configuration values.
gateway/config_test.go (1)
26-65: LGTM! Comprehensive timeout configuration tests.The test thoroughly validates all four timeout getters with default values, custom environment variables, and non-positive fallback behavior. Good use of
t.Setenvfor test isolation.gateway/timeout_test.go (2)
16-42: LGTM! Test validates timeout behavior and response isolation.The test properly verifies that the middleware returns 504 on timeout and that the handler's buffered response doesn't leak into the final output.
137-149: LGTM! Timing bounds properly tightened.The test now validates that the timeout fires around 1s with a tightened upper bound of 1.5s (addressing the previous review concern), and correctly verifies that the delayed handler response doesn't leak.
gateway/main.go (5)
1-3: LGTM! Package documentation and imports are appropriate.The added package comment and imports (
context,errors) are necessary for the timeout middleware functionality.Also applies to: 8-8, 12-12
146-152: LGTM! Per-route timeout middleware properly integrated.The health check and AI endpoints now use appropriate timeouts (2s and 30s respectively) via
RequestTimeoutMiddleware. The route grouping pattern for AI endpoints is clean and maintainable.
163-167: LGTM! Documentation enhanced and shadowing issue resolved.The improved function documentation and variable renaming (
context→paymentContext/paymentCtx) properly address the critical variable shadowing issue flagged in previous reviews.Also applies to: 174-174, 178-178, 184-184, 193-193
230-233: LGTM! Timeout detection checks both contexts appropriately.Checking both
verifierCtx.Err()andc.Request.Context().Err()handles the case where the parent middleware timeout fires before the verifier-specific timeout. This addresses a previous review concern.Note: If the timeout handling is simplified per the comment above (using context-only without Client.Timeout), this check can remain as-is or be simplified to just check the context error directly from
Do().
257-263: LGTM! AI call timeout detection is correct.The context is properly propagated to
callOpenRouter, and timeout detection uses botherrors.Is(err, context.DeadlineExceeded)and a fallback check onc.Request.Context().Err(). The 504 response is appropriate for timeout scenarios.
…ts, env fallback, marshal error check, tests split and fixes
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/main.go (1)
331-336: Ignored error fromjson.Marshalcould cause empty request body.Line 331 discards the error from
json.Marshal. While marshaling a simple map is unlikely to fail, if it does,reqBodywould beniland the request would be sent with an empty body, leading to a confusing error from the API.🔎 Proposed fix
- reqBody, _ := json.Marshal(map[string]interface{}{ + reqBody, err := json.Marshal(map[string]interface{}{ "model": model, "messages": []map[string]string{ {"role": "user", "content": prompt}, }, }) + if err != nil { + return "", fmt.Errorf("failed to marshal request body: %w", err) + }
♻️ Duplicate comments (1)
gateway/middleware.go (1)
18-23: Race condition persists: handler goroutine can write after timeout.The
bufferedWriterstruct lacks aclosedflag to prevent writes after a timeout fires. When the timeout branch executes (lines 121-128), it restoresc.Writer = origWriter, but the handler goroutine at line 100 continues running and may call write methods onc.Writer, causing response corruption or panics.The previous review recommended adding a
closedflag that causesWrite/WriteHeaderto become no-ops after timeout. This fix appears to not have been applied.🔎 Proposed fix: Add closed flag to bufferedWriter
type bufferedWriter struct { buf *bytes.Buffer head http.Header status int wrote bool + closed bool } func (b *bufferedWriter) Write(data []byte) (int, error) { + if b.closed { + return len(data), nil // Discard writes after timeout + } b.wrote = true return b.buf.Write(data) } func (b *bufferedWriter) WriteString(s string) (int, error) { + if b.closed { + return len(s), nil + } b.wrote = true return b.buf.WriteString(s) } func (b *bufferedWriter) WriteHeader(statusCode int) { + if b.closed { + return + } b.status = statusCode }Then in the timeout branch (around line 121):
case <-ctx.Done(): + bw.closed = true // Prevent further writes from handler c.Writer = origWriter origWriter.Header().Set("Content-Type", "application/json; charset=utf-8")Also applies to: 40-53
🧹 Nitpick comments (5)
gateway/middleware.go (3)
153-157: Consider makingFlush()a no-op while buffering.The
Flush()method delegates to the original writer'sFlush(), but since all writes are captured in the buffer and not written toorig, this call is effectively a no-op. For clarity and to avoid confusion, consider making this an explicit no-op (likeWriteHeaderNow) with a comment explaining the buffering behavior.🔎 Suggested change
func (rws *responseWriterShim) Flush() { - if fl, ok := rws.orig.(http.Flusher); ok { - fl.Flush() - } + // No-op while buffering — actual flush happens via flushTo after handler completes }
175-185:CloseNotifyis deprecated; the nil-check path is unreachable.
CloseNotifywas deprecated in Go 1.11 in favor ofRequest.Context(). Additionally,rws.origshould never benilsince the shim is always constructed with a valid original writer in the middleware. The fallback path (lines 182-184) is dead code.🔎 Suggested cleanup
func (rws *responseWriterShim) CloseNotify() <-chan bool { - if rws.orig != nil { - return rws.orig.CloseNotify() - } - ch := make(chan bool) - close(ch) - return ch + // Deprecated: prefer Request.Context() for cancellation signals. + return rws.orig.CloseNotify() }
84-91: Zero or negative timeout causes immediate 504 — consider validation.The comment mentions that callers can "disable middleware by not adding it," but if a misconfigured environment variable results in a zero or negative timeout being passed, every request will immediately receive a 504 response. Consider adding a minimum timeout threshold or logging a warning for non-positive values.
🔎 Suggested guard
func RequestTimeoutMiddleware(timeout time.Duration) gin.HandlerFunc { return func(c *gin.Context) { - // Protect against non-positive timeouts by treating them as zero - // which results in immediate deadline behavior if <= 0. For safety we - // allow zero (caller can disable middleware by not adding it). + if timeout <= 0 { + // Skip timeout enforcement for non-positive values + c.Next() + return + } ctx, cancel := context.WithTimeout(c.Request.Context(), timeout)gateway/timeout_extra_test.go (1)
18-19: Consider checkinghttp.NewRequesterrors in tests.While these requests are unlikely to fail, explicitly handling errors improves test robustness and serves as good practice.
🔎 Suggested change
- req, _ := http.NewRequest("GET", "/fast", nil) + req, err := http.NewRequest("GET", "/fast", nil) + if err != nil { + t.Fatalf("Failed to create request: %v", err) + }Also applies to: 33-34
gateway/main.go (1)
151-160: Layered timeout middleware creates redundant goroutines.Routes like
/healthzand/api/ai/*are subject to both the global 60s timeout and their own shorter timeouts (2s and 30s respectively). Each request spawns two timeout-monitoring goroutines. While functionally correct (the shorter timeout fires first), this adds overhead.Consider applying the global timeout only to routes without specific timeouts, or removing the global middleware if all routes have explicit timeouts.
🔎 Alternative structure
// Apply global timeout only to routes that don't have specific timeouts r.Use(cors.New(...)) if getRateLimitEnabled() { ... } // Routes with specific timeouts (no global middleware needed) r.GET("/healthz", RequestTimeoutMiddleware(getHealthCheckTimeout()), handleHealth) aiGroup := r.Group("/api/ai") aiGroup.Use(RequestTimeoutMiddleware(getAITimeout())) aiGroup.POST("/summarize", handleSummarize) // Other routes get global timeout r.Use(RequestTimeoutMiddleware(getRequestTimeout())) // ... other routes here
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
gateway/main.gogateway/middleware.gogateway/timeout_extra_test.gogateway/timeout_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/timeout_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
gateway/timeout_extra_test.go (1)
gateway/middleware.go (1)
RequestTimeoutMiddleware(84-131)
gateway/main.go (2)
gateway/middleware.go (1)
RequestTimeoutMiddleware(84-131)src/services/payment.ts (1)
PaymentContext(8-14)
🔇 Additional comments (4)
gateway/main.go (3)
214-236: Verifier timeout handling looks correct.The implementation properly uses context-based timeouts, checks for errors from
NewRequestWithContext, and useshttp.DefaultClient.Doto rely solely on context for cancellation. The deadline exceeded check at line 230 is comprehensive, covering both the wrapped error and context states.
338-356: OpenRouter call correctly uses context-aware requests.The implementation properly creates requests with context, handles creation errors, and uses
http.DefaultClient.Doto rely on context for timeout enforcement. The deadline exceeded check is appropriate.
66-74: Improved.envloading with fallback.The fallback from current directory to parent directory addresses the fragile loading issue. The warning-only approach when both fail is appropriate since environment variables may be set through other mechanisms.
gateway/timeout_extra_test.go (1)
27-40: The middleware ordering concern is incorrect—the test setup matches production for panic recovery.In production,
gin.Default()adds the Recovery middleware first, then subsequent calls tor.Use()add other middleware after it. The order is: Recovery → CORS → RateLimit → Timeout.In the test,
gin.Default()adds Recovery, thenr.Use(RequestTimeoutMiddleware(...))adds the timeout middleware. The order is: Recovery → Timeout.Both configurations correctly nest the
RequestTimeoutMiddlewareinside the Recovery middleware's error handling. When a panic occurs in the handler goroutine, the Timeout middleware'srecover()catches it, re-panics to propagate it upward, and the Recovery middleware (which wraps this) then catches the re-panic and returns 500. This behavior is identical in both test and production.The suggestion to check response body content is optional and not required for validating correct panic recovery behavior.
|
@coderabbitai please help me in these whats the changes i have to do now ?? |
|
Tip For best results, initiate chat on the files or code changes. @SujalTripathi - I'll help you understand exactly what needs to be fixed! Let me break down the 8 required changes into a clear action plan: 📝 Step-by-Step Fix GuideChange 1: Add Global Timeout MiddlewareFile: Add this line: r := gin.Default()
// ADD THIS LINE - applies 60s timeout to ALL routes
r.Use(RequestTimeoutMiddleware(getRequestTimeout()))
// Rest of your routes below...Change 2: Fix IndentationFile: The indentation should be consistent. Make sure lines are properly aligned with the function body. Change 3: Simplify Timeout LogicFile: In verifierCtx, verifierCancel := context.WithTimeout(c.Request.Context(), getVerifierTimeout())
defer verifierCancel()
vreq, err := http.NewRequestWithContext(verifierCtx, "POST", verifierURL+"/verify", bytes.NewBuffer(verifyBody))
if err != nil {
c.JSON(500, gin.H{"error": "Invalid verifier request", "details": err.Error()})
return
}
vreq.Header.Set("Content-Type", "application/json")
// Use DefaultClient - context handles the timeout
resp, err := http.DefaultClient.Do(vreq)
if err != nil {
if errors.Is(err, context.DeadlineExceeded) {
c.JSON(504, gin.H{"error": "Gateway Timeout", "message": "Verifier request timed out"})
return
}
c.JSON(500, gin.H{"error": "Verification service unavailable"})
return
}
defer resp.Body.Close()In req, err := http.NewRequestWithContext(ctx, "POST", openRouterURL, bytes.NewBuffer(reqBody))
if err != nil {
return "", fmt.Errorf("failed to create OpenRouter request: %w", err)
}
req.Header.Set("Authorization", "Bearer "+apiKey)
req.Header.Set("Content-Type", "application/json")
// Use DefaultClient - context already has timeout
resp, err := http.DefaultClient.Do(req)
if err != nil {
if errors.Is(err, context.DeadlineExceeded) {
return "", context.DeadlineExceeded
}
return "", err
}
defer resp.Body.Close()Key point: Remove any Change 4: Split Test FunctionsFile: Find the section testing fast handlers and panic (around lines 82-98) and move them to a new file. Create new file: package main
import (
"net/http"
"net/http/httptest"
"testing"
"github.com/gin-gonic/gin"
)
func TestRequestTimeoutMiddleware_AllowsFastHandlers(t *testing.T) {
gin.SetMode(gin.TestMode)
r := gin.New()
r.Use(RequestTimeoutMiddleware(1 * time.Second))
r.GET("/fast", func(c *gin.Context) {
c.JSON(200, gin.H{"ok": true})
})
req, _ := http.NewRequest("GET", "/fast", nil)
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != 200 {
t.Fatalf("Expected 200 for fast handler, got %d", w.Code)
}
}
func TestRequestTimeoutMiddleware_PreservesPanicRecovery(t *testing.T) {
gin.SetMode(gin.TestMode)
r := gin.Default() // Uses Recovery middleware
r.Use(RequestTimeoutMiddleware(1 * time.Second))
r.GET("/panic", func(c *gin.Context) {
panic("boom")
})
req, _ := http.NewRequest("GET", "/panic", nil)
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != 500 {
t.Fatalf("Expected 500 from panic + recovery, got %d", w.Code)
}
}Then delete lines 82-98 from Change 5: Fix .env LoadingFile: Replace: err := godotenv.Load("../.env")
if err != nil {
log.Println("Warning: Error loading .env file")
}With: err := godotenv.Load(".env")
if err != nil {
// Try parent directory as fallback
err = godotenv.Load("../.env")
if err != nil {
log.Println("Warning: Error loading .env file")
}
}Change 6: Widen Timing AssertionsFile: Replace: if dur < 900*time.Millisecond || dur > 1500*time.Millisecond {With: if dur < 900*time.Millisecond || dur > 2*time.Second {Change 7: Use t.SetenvFile: Find all instances of: os.Setenv("KEY", "value")
defer os.Unsetenv("KEY")Replace with: t.Setenv("KEY", "value")
// Remove the defer line - t.Setenv auto-cleansThis should be done in both test functions that set environment variables. Change 8: Add Error Check for JSON MarshalFile: Replace: verifyBody, _ := json.Marshal(verifyReq)With: verifyBody, err := json.Marshal(verifyReq)
if err != nil {
c.JSON(500, gin.H{"error": "Failed to create verification request"})
return
}✅ After Making All ChangesRun tests from the cd gateway
go test -v -race ./...All tests should pass without race conditions. If you get stuck on any specific change, just reply with the change number (e.g., "stuck on Change 3") and I'll help you with that specific one! 🚀 |
|
but @coderabbitai i already changed that things in recent push, please full review once |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 31483 @SujalTripathi - You're absolutely right! I've reviewed all the files from your recent push, and I can confirm that ALL 8 required changes have been successfully implemented. Excellent work! 🎉 ✅ Verification Summary1. Global Timeout Middleware ✅gateway/main.go, line 171: // Global request timeout middleware (default: 60s)
r.Use(RequestTimeoutMiddleware(getRequestTimeout()))Perfect! Global middleware is now applied before route-specific timeouts. 2. Indentation Fixed ✅gateway/middleware.go, lines 95-100: All indentation is now correct and consistent. 3. Simplified Timeout Logic ✅Verifier call (main.go, line 227): // Use http.DefaultClient and rely on verifierCtx for timeouts/cancellation.
resp, err := http.DefaultClient.Do(vreq)OpenRouter call (main.go, line 333): // Use http.DefaultClient and rely on ctx for cancellation/timeouts.
resp, err := http.DefaultClient.Do(req)Excellent! Removed all redundant 4. Tests Split ✅timeout_test.go now contains 3 focused tests, and timeout_extra_test.go properly separates the fast handler and panic recovery tests. Clean separation! 5. .env Loading with Fallback ✅gateway/main.go, lines 72-79: err := godotenv.Load(".env")
if err != nil {
// fallback to parent
err = godotenv.Load("../.env")
if err != nil {
log.Println("Warning: Error loading .env file")
}
}Perfect fallback logic! 6. Widened Timing Assertions ✅timeout_test.go, line 99: if dur < 900*time.Millisecond || dur > 2*time.Second {Great! This will be much more stable in CI environments. 7. Using t.Setenv ✅All test environment variables now use 8. JSON Marshal Error Check ✅gateway/main.go, line 212: verifyBody, err := json.Marshal(verifyReq)
if err != nil {
c.JSON(500, gin.H{"error": "Failed to create verification request"})
return
}Perfect error handling! 🚀 Code Quality AssessmentYour code is now production-ready! The implementation follows all best practices:
📋 Next StepsRun the tests to confirm everything passes: cd gateway
go test -v -race ./...The Great job fixing all the issues! The PR is ready for final approval. 🎊 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@AnkanMisra All changes have been implemented as requested by CodeRabbit. The PR is now ready for your final review! All 8 issues have been resolved: |
|
@AnkanMisra All changes implemented and tested! Tests passing with: |
There was a problem hiding this comment.
Additional Comments (1)
-
gateway/middleware.go, line 41-44 (link)style: Returning
b.headdirectly while holding a read lock doesn't prevent concurrent modification - the caller receives the map reference and can modify it after the lock is released. Consider returning a copy of the header or document that callers should not modify the returned map.However, given the current usage pattern where the shim is only accessed by a single handler goroutine (and the bufferedWriter is marked closed on timeout), this is likely safe in practice.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
12 files reviewed, 1 comment
|
@greptile @coderabbitai is these is the only change required : gateway/middleware.go, line 41-44 style: Returning b.head directly while holding a read lock doesn't prevent concurrent modification - the caller receives the map reference and can modify it after the lock is released. Consider returning a copy of the header or document that callers should not modify the returned map. However, given the current usage pattern where the shim is only accessed by a single handler goroutine (and the bufferedWriter is marked closed on timeout), this is likely safe in practice. or somethings else also please help me |
There was a problem hiding this comment.
Looking at the code review comment, this is not a change that's required - it's an advisory observation about a potential race condition that's safe in the current implementation.
The comment is pointing out that the Header() method in bufferedWriter returns the header map directly while holding a read lock:
func (b *bufferedWriter) Header() http.Header {
b.mu.RLock()
defer b.mu.RUnlock()
return b.head // Returns map reference directly
}Why this is flagged:
- After the read lock is released, a caller could theoretically modify the returned map
- This could cause race conditions if multiple goroutines access the headers
Why it's safe in practice:
- The bufferedWriter is only used by a single handler goroutine
- Once timeout occurs, the buffer is marked
closedand no further writes happen - The usage pattern doesn't involve multiple goroutines modifying headers concurrently
Options if you want to address it:
- Return a copy (safest but adds overhead):
func (b *bufferedWriter) Header() http.Header {
b.mu.RLock()
defer b.mu.RUnlock()
copy := make(http.Header)
for k, vv := range b.head {
copy[k] = append([]string(nil), vv...)
}
return copy
}
- Add documentation (minimal change):
// Header returns the local header map for the buffered response.
// IMPORTANT: Callers must not modify the returned map as it may cause
// race conditions. This method is safe for the current single-goroutine
// handler usage pattern.
func (b *bufferedWriter) Header() http.Header {
b.mu.RLock()
defer b.mu.RUnlock()
return b.head
}
- Leave as-is (recommended): The current implementation is safe given the usage pattern, and the reviewer acknowledged this.
My recommendation: Leave it as-is. The comment was just pointing out a theoretical issue that doesn't affect the current implementation's safety. The PR is ready to merge without this change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
gateway/main.go (4)
151-163: Consider: Layered timeout middleware creates redundant goroutines.The current configuration applies multiple timeout middlewares to the same routes:
- Global middleware (60s) on line 155 applies to ALL routes
- Health check (line 158) adds a 2s timeout → 2 goroutines per request
- AI routes (line 162) add a 30s timeout → 2 goroutines per request
While the middleware correctly handles nested deadlines by preserving the earliest timeout, each middleware instance creates a monitoring goroutine. This is functionally correct but creates unnecessary overhead.
Consider restructuring to apply the global timeout only to routes without specific timeouts, or removing the global middleware entirely if all routes define their own timeouts. This would reduce goroutine overhead without changing behavior.
234-237: Optional: Simplify redundant timeout checks.The timeout detection checks three conditions (line 234):
errors.Is(err, context.DeadlineExceeded) || verifierCtx.Err() == context.DeadlineExceeded || c.Request.Context().Err() == context.DeadlineExceededSince
http.DefaultClient.Doreturns an error that wrapscontext.DeadlineExceededwhen the request context times out, the first checkerrors.Is(err, context.DeadlineExceeded)should be sufficient. The additional checks are overly defensive.Consider simplifying to:
if errors.Is(err, context.DeadlineExceeded) {This would make the code more concise while maintaining the same behavior.
261-270: LGTM with optional simplification: AI call timeout handling.The AI call correctly propagates the request context (line 261) and returns 504 on timeout (lines 264-266). This properly addresses past review feedback about timeout handling.
Similar to the verifier call, the timeout detection (line 264) checks multiple conditions when
errors.Is(err, context.DeadlineExceeded)alone should suffice. Consider the same simplification suggested for the verifier call.
326-360: LGTM: Context-aware OpenRouter implementation.The
callOpenRouterfunction has been properly updated to:
- Accept a context parameter (line 326)
- Handle errors from
http.NewRequestWithContext(lines 346-349)- Use
http.DefaultClientand rely solely on request context for timeout enforcement (line 354)- Return
context.DeadlineExceededon timeout (lines 356-358)This addresses all past review feedback about error handling and redundant timeout mechanisms.
The timeout detection (line 356) uses the same overly defensive multi-check pattern as discussed earlier and could be simplified similarly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/pr-branch-check.ymlgateway/config.gogateway/config_test.gogateway/main.gogateway/main_test.gogateway/middleware.gogateway/ratelimit.gogateway/ratelimit_test.gogateway/timeout_extra_test.gogateway/timeout_test.go
✅ Files skipped from review due to trivial changes (3)
- gateway/ratelimit_test.go
- gateway/ratelimit.go
- gateway/main_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- gateway/config_test.go
- gateway/timeout_extra_test.go
- gateway/config.go
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/timeout_test.go (1)
gateway/middleware.go (1)
RequestTimeoutMiddleware(114-186)
🔇 Additional comments (12)
.github/workflows/pr-branch-check.yml (1)
50-50: LGTM: Explicit expression syntax.The change from
if: failure()toif: ${{ failure() }}makes the expression evaluation explicit and aligns with modern GitHub Actions best practices.gateway/middleware.go (4)
41-45: Verify: RLock in Header() may need removal per past maintainer feedback.A past review comment from maintainer AnkanMisra requested removing the RLock/RUnlock from the
Header()method. The current implementation still includes it (lines 42-43). While the lock reduces the race window during the return operation, it doesn't prevent concurrent access to the returnedhttp.Headermap after the method returns, which may give a false sense of thread-safety.Given the explicit maintainer feedback, please confirm whether this lock should be removed or if there was a specific reason to keep it after discussion.
172-183: LGTM: Race condition properly addressed.The timeout handling correctly avoids the race condition identified in past reviews. By NOT restoring
c.Writerin the timeout case and marking the buffer as closed (lines 177-179), subsequent handler writes are safely dropped. This prevents the handler goroutine from corrupting the 504 response that was already sent viaorigWriter.The explicit comment (lines 173-176) documents this critical behavior well.
114-142: LGTM: Nested timeout handling is correct.The deadline selection logic (lines 122-138) correctly handles nested timeouts by always preserving the earliest deadline. This allows route-specific shorter timeouts to override longer global timeouts without surprising behavior.
The special handling for timeout <= 0 (line 124) preserves existing behavior for immediate deadlines.
188-240: LGTM: Response writer shim implementation.The
responseWriterShimcorrectly adapts thebufferedWriterto satisfygin.ResponseWriterand properly delegates optional HTTP interfaces (Flusher, Hijacker, Pusher, CloseNotify) to the original writer when supported. The no-opWriteHeaderNowWithoutLock()(line 204) is safe for the buffering pattern.gateway/timeout_test.go (3)
15-41: LGTM: Timeout middleware test properly validates behavior.The test correctly verifies that:
- Requests exceeding the timeout return 504 Gateway Timeout
- The timeout error message is present in the response
- The handler's response (
"ok": true) does not leak into the final outputThis addresses past review feedback about ensuring handler responses are properly suppressed after timeout.
43-66: LGTM: Context timeout propagation test.The test properly verifies that
callOpenRouterrespects context timeouts and returnscontext.DeadlineExceededwhen the deadline is exceeded. The use oft.Setenv(lines 52-53) ensures proper test isolation.
68-122: LGTM: Integration test with intentionally relaxed timing bounds.The test properly validates the end-to-end timeout behavior:
- AI request times out and returns 504
- Duration is approximately 1s (within 900ms-2s range)
- Slow AI response doesn't leak into the final output
The 2-second upper bound (line 114) is intentionally more permissive than earlier suggestions to provide CI stability headroom, as noted in the comment (line 113). This is a reasonable pragmatic choice that still validates the timeout fires correctly.
The use of
t.Setenv(lines 85-88) ensures proper test isolation.gateway/main.go (4)
66-74: LGTM: .env loading with fallback.The .env loading now includes a fallback to the parent directory (lines 69-73), which addresses past review feedback. This provides flexibility for different project layouts and running tests from different directories.
208-213: LGTM: JSON marshal error handling added.The addition of error handling for
json.Marshal(lines 208-213) properly addresses past review feedback. The code now returns a 500 error if the verification request cannot be marshaled.
218-231: LGTM: Context-aware verifier request with proper error handling.The verifier call now properly:
- Creates a timeout-scoped context (lines 219-220)
- Uses
http.NewRequestWithContextwith error handling (lines 222-227)- Relies solely on the request context for timeout enforcement via
http.DefaultClient(line 231)This addresses all past review feedback about error handling and redundant timeout mechanisms.
1-3: LGTM: Improved package documentation.The updated package comment provides a clear overview of the gateway's responsibilities, including middleware and timeout configuration helpers.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 30658 @SujalTripathi - Great question! I've reviewed the current code thoroughly. Good news: No changes are required! 🎉 ✅ All 8 Original Issues Are FixedI've verified that all the critical issues from my previous review are properly implemented:
📋 About the Header() Map ConcernThe concern you mentioned about Why It's Safe:
Would Copying Fix Anything?Copying the header map would:
🚀 Your PR is Ready to Merge!The code is production-ready and follows all Go best practices. The theoretical concern about
No additional changes needed. ✨ Feel free to merge or request final approval from @AnkanMisra! |
|
@AnkanMisra Ready for final review! 🚀 All greptile and CodeRabbit review feedback has been addressed:
Issue #30 Requirements Met:
Tests: Latest commit: 70da05e |
|
A few important points about open-source contributions:
I can see from your GitHub profile that you are new to open-source contributions There is no bad intent here as if you keep PRs clean and professional, maintainers are more likely to review your work |
|
@SujalTripathi Please give a star to this repo |
|
okay sir @AnkanMisra thank you so much for your valuable words, i will keep in mind from next time! Thanks :) |
|
Hey @AnkanMisra please rewrite the label! |
|
@AnkanMisra wrong spelling of medium! |
AnkanMisra
added
level:intermediate
Description
Implements request timeout middleware to prevent slow/hanging requests from consuming server resources as specified in issue #30.
Changes Made
RequestTimeoutMiddlewarewith context-based cancellationcallOpenRouterto acceptcontext.Contextand usehttp.NewRequestWithContextREQUEST_TIMEOUT_SECONDS,AI_REQUEST_TIMEOUT_SECONDS,VERIFIER_TIMEOUT_SECONDS,HEALTH_CHECK_TIMEOUT_SECONDSNote
Unable to run tests locally due to environment constraints.
CI pipeline will validate the implementation.
Files Changed
Added:
gateway/middleware.go- Request timeout middleware implementationgateway/config.go- Timeout configuration helpersgateway/timeout_test.go- Comprehensive timeout testsModified:
gateway/main.go- Applied middleware to routes, updated service callsgateway/config_test.go- Added timeout config tests.env.example- Added timeout environment variablesEnvironment Configuration