-
Notifications
You must be signed in to change notification settings - Fork 34
feat(gateway): add graceful shutdown with signal handling #41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b4d0043
d94d91f
4b63721
1be2d92
a432b1b
c910d79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,8 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sync" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "os/signal" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "syscall" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+23
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix indentation to use tabs instead of spaces. The import statements appear to use spaces for indentation instead of tabs. Run π Fix formatting- "os/signal"
- "syscall"
+ "os/signal"
+ "syscall"π Committable suggestion
Suggested change
π€ Prompt for AI Agents
Comment on lines
+23
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import formatting is incorrect - these imports have leading spaces and should align with the other imports above.
Suggested change
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! Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/main.go
Line: 23:24
Comment:
Import formatting is incorrect - these imports have leading spaces and should align with the other imports above.
```suggestion
"os/signal"
"syscall"
```
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/ethereum/go-ethereum/crypto" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/gin-contrib/cors" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -157,6 +159,7 @@ func main() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // deadline; the middleware implementation always uses the earliest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // deadline when nested timeouts are present to avoid surprising behavior. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| r.Use(RequestTimeoutMiddleware(getRequestTimeout())) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| r.Use(TrackInFlightRequests()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Health check with shorter timeout (2s) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| r.GET("/healthz", RequestTimeoutMiddleware(getHealthCheckTimeout()), handleHealth) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -187,8 +190,49 @@ func main() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| port = "3000" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Printf("Go Gateway running on port %s", port) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| r.Run(":" + port) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addr := ":" + port | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| srv := &http.Server{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Addr: addr, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Handler: r, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ReadHeaderTimeout: 5 * time.Second, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ReadTimeout: 10 * time.Second, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WriteTimeout: 60 * time.Second, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| IdleTimeout: 120 * time.Second, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| go func() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Printf("[INFO] Gateway listening on %s", addr) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Fatalf("[FATAL] listen error: %v", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+205
to
+210
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Using π Proposed fix using error channel+ errChan := make(chan error, 1)
+
go func() {
log.Printf("[INFO] Gateway listening on %s", addr)
- if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed {
- log.Fatalf("[FATAL] listen error: %v", err)
- }
+ errChan <- srv.ListenAndServe()
}()
// ---- Graceful shutdown ----
quit := make(chan os.Signal, 1)
signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)
- <-quit
- log.Println("[INFO] Shutdown signal received, draining connections...")
+ select {
+ case err := <-errChan:
+ if err != nil && err != http.ErrServerClosed {
+ log.Fatalf("[FATAL] Server failed to start: %v", err)
+ }
+ return
+ case <-quit:
+ log.Println("[INFO] Shutdown signal received, draining connections...")
+ }
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
if err := srv.Shutdown(ctx); err != nil {
log.Printf("[ERROR] Server forced to shutdown: %v", err)
} else {
log.Println("[OK] Server shutdown completed")
}π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // ---- Graceful shutdown ---- | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| quit := make(chan os.Signal, 1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <-quit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Println("[INFO] Shutdown signal received, draining connections...") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| active := GetActiveRequestCount() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if active > 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Printf("[INFO] Waiting for %d in-flight request(s)...", active) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WaitForInFlightRequests() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Println("[INFO] All in-flight requests completed") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+219
to
+224
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Redundant and race-prone shutdown logic This manual request tracking and waiting is both redundant and architecturally incorrect:
Correct approach: Remove lines 219-224 entirely and rely solely on Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/main.go
Line: 219:224
Comment:
**CRITICAL: Redundant and race-prone shutdown logic**
This manual request tracking and waiting is both redundant and architecturally incorrect:
1. **`http.Server.Shutdown()` already waits for active connections** - From Go docs: "Shutdown works by first closing all open listeners, then closing all idle connections, and then waiting indefinitely for connections to return to idle and then shut down." So the call on line 229 will wait anyway.
2. **Race condition**: New requests can arrive between line 222 (when WaitForInFlightRequests returns) and line 229 (when srv.Shutdown is called). The server is still accepting connections during this window.
3. **Misleading logs**: Line 223 logs "All in-flight requests completed" but the server is still running and accepting new requests until line 229.
4. **No timeout**: `WaitForInFlightRequests()` blocks forever if a request hangs. The 30-second timeout on line 226 only applies to `srv.Shutdown()`, not this manual wait.
**Correct approach**: Remove lines 219-224 entirely and rely solely on `srv.Shutdown()` to wait for active connections, OR redesign to stop accepting new connections before waiting (which is what Shutdown already does).
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer cancel() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := srv.Shutdown(ctx); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Printf("[ERROR] Server forced to shutdown: %v", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Println("[OK] Server shutdown completed") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+213
to
+234
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race condition: wait for in-flight requests after stopping new connections. The current sequence waits for in-flight requests (lines 219-223) before calling
The standard graceful shutdown pattern is to call π§ Recommended fixEither remove the manual wait and rely on <-quit
log.Println("[INFO] Shutdown signal received, draining connections...")
- active := GetActiveRequestCount()
- if active > 0 {
- log.Printf("[INFO] Waiting for %d in-flight request(s)...", active)
- WaitForInFlightRequests()
- log.Println("[INFO] All in-flight requests completed")
- }
-
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
if err := srv.Shutdown(ctx); err != nil {
log.Printf("[ERROR] Server forced to shutdown: %v", err)
} else {
log.Println("[OK] Server shutdown completed")
}Or, if logging the active count is important, just log it without the manual wait: <-quit
log.Println("[INFO] Shutdown signal received, draining connections...")
active := GetActiveRequestCount()
if active > 0 {
log.Printf("[INFO] %d in-flight request(s) detected, waiting for completion...", active)
- WaitForInFlightRequests()
- log.Println("[INFO] All in-flight requests completed")
}
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
if err := srv.Shutdown(ctx); err != nil {
log.Printf("[ERROR] Server forced to shutdown: %v", err)
} else {
log.Println("[OK] Server shutdown completed")
}
π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // handleSummarize handles POST /api/ai/summarize requests. It validates | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "sync" | ||
| "sync/atomic" | ||
|
|
||
| "github.com/gin-gonic/gin" | ||
| ) | ||
|
|
||
| var ( | ||
| activeRequestsWG sync.WaitGroup | ||
| activeRequestCnt int64 | ||
| ) | ||
|
Comment on lines
+10
to
+13
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Package-level state prevents test isolation and concurrent execution. The package-level
Consider refactoring to use a struct-based approach: type RequestTracker struct {
wg sync.WaitGroup
cnt int64
}
func NewRequestTracker() *RequestTracker {
return &RequestTracker{}
}
func (rt *RequestTracker) Middleware() gin.HandlerFunc {
return func(c *gin.Context) {
rt.wg.Add(1)
atomic.AddInt64(&rt.cnt, 1)
defer func() {
atomic.AddInt64(&rt.cnt, -1)
rt.wg.Done()
}()
c.Next()
}
}
func (rt *RequestTracker) Wait() {
rt.wg.Wait()
}
func (rt *RequestTracker) Count() int64 {
return atomic.LoadInt64(&rt.cnt)
}This allows each test to create its own isolated
Comment on lines
+10
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Global state with no reset mechanism creates issues in tests and server restarts These global variables are never reset, which causes problems:
Better approach: Either make these per-server instance variables, or provide a reset function for tests. Example: // For tests
func ResetRequestTracking() {
activeRequestsWG = sync.WaitGroup{}
atomic.StoreInt64(&activeRequestCnt, 0)
}Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/request_tracker.go
Line: 10:13
Comment:
**Global state with no reset mechanism creates issues in tests and server restarts**
These global variables are never reset, which causes problems:
1. **Test pollution**: If multiple tests create servers (like in shutdown_test.go), the counters persist between tests, causing incorrect counts
2. **Server restart issues**: If the server is stopped and restarted within the same process, the counts will be wrong
3. **The test in shutdown_test.go only works by accident** because it's the only test, but running multiple shutdown tests would fail
**Better approach**: Either make these per-server instance variables, or provide a reset function for tests. Example:
```go
// For tests
func ResetRequestTracking() {
activeRequestsWG = sync.WaitGroup{}
atomic.StoreInt64(&activeRequestCnt, 0)
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| // TrackInFlightRequests tracks active HTTP requests. | ||
| func TrackInFlightRequests() gin.HandlerFunc { | ||
| return func(c *gin.Context) { | ||
| activeRequestsWG.Add(1) | ||
| atomic.AddInt64(&activeRequestCnt, 1) | ||
|
|
||
| defer func() { | ||
| atomic.AddInt64(&activeRequestCnt, -1) | ||
| activeRequestsWG.Done() | ||
| }() | ||
|
|
||
| c.Next() | ||
| } | ||
| } | ||
|
|
||
| // WaitForInFlightRequests blocks until all active requests finish. | ||
| func WaitForInFlightRequests() { | ||
| activeRequestsWG.Wait() | ||
| } | ||
|
|
||
| // GetActiveRequestCount returns the current number of active requests. | ||
| func GetActiveRequestCount() int64 { | ||
| return atomic.LoadInt64(&activeRequestCnt) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,67 @@ | ||||||||||||||||||||||||||||||||||||||||||
| package main | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||||||||||
| "net/http/httptest" | ||||||||||||||||||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| "github.com/gin-gonic/gin" | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
| func TestGracefulShutdown_WaitsForInFlightRequests(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||
| gin.SetMode(gin.TestMode) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| r := gin.New() | ||||||||||||||||||||||||||||||||||||||||||
| r.Use(TrackInFlightRequests()) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Simulate slow handler | ||||||||||||||||||||||||||||||||||||||||||
| r.GET("/slow", func(c *gin.Context) { | ||||||||||||||||||||||||||||||||||||||||||
| time.Sleep(200 * time.Millisecond) | ||||||||||||||||||||||||||||||||||||||||||
| c.Status(http.StatusOK) | ||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| srv := &http.Server{ | ||||||||||||||||||||||||||||||||||||||||||
| Handler: r, | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Start test server | ||||||||||||||||||||||||||||||||||||||||||
| ln := httptest.NewUnstartedServer(r) | ||||||||||||||||||||||||||||||||||||||||||
| ln.Config = srv | ||||||||||||||||||||||||||||||||||||||||||
| ln.Start() | ||||||||||||||||||||||||||||||||||||||||||
| defer ln.Close() | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+24
to
+32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test server setup is incorrect. The test mixes
π οΈ Proposed fixUse the httptest server directly without creating a separate - srv := &http.Server{
- Handler: r,
- }
-
// Start test server
- ln := httptest.NewUnstartedServer(r)
- ln.Config = srv
- ln.Start()
- defer ln.Close()
+ ts := httptest.NewServer(r)
+ defer ts.Close()Then update the shutdown logic: - // Shutdown server
- ctx, cancel := context.WithTimeout(context.Background(), time.Second)
- defer cancel()
-
- start := time.Now()
- if err := srv.Shutdown(ctx); err != nil {
- t.Fatalf("shutdown failed: %v", err)
- }
+ // Close the test server (httptest.Server doesn't support graceful shutdown)
+ start := time.Now()
+ ts.Close()Note:
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Make request in background | ||||||||||||||||||||||||||||||||||||||||||
| done := make(chan struct{}) | ||||||||||||||||||||||||||||||||||||||||||
| go func() { | ||||||||||||||||||||||||||||||||||||||||||
| resp, err := http.Get(ln.URL + "/slow") | ||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||
| t.Errorf("request failed: %v", err) | ||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| resp.Body.Close() | ||||||||||||||||||||||||||||||||||||||||||
| close(done) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test should verify that the request completed successfully by checking the response status code, not just that it didn't error. A connection could be closed mid-request and still not return an error, but the response would be incomplete. Add verification:
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/shutdown_test.go
Line: 36:43
Comment:
The test should verify that the request completed successfully by checking the response status code, not just that it didn't error. A connection could be closed mid-request and still not return an error, but the response would be incomplete.
Add verification:
```suggestion
go func() {
resp, err := http.Get(ln.URL + "/slow")
if err != nil {
t.Errorf("request failed: %v", err)
return
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
t.Errorf("expected status 200, got %d", resp.StatusCode)
}
close(done)
}()
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||
| }() | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsafe use of Line 39 calls π Proposed fixCapture the error in the goroutine and check it in the main test flow: // Make request in background
- done := make(chan struct{})
+ done := make(chan error)
go func() {
resp, err := http.Get(ln.URL + "/slow")
if err != nil {
- t.Errorf("request failed: %v", err)
- return
+ done <- err
+ return
}
resp.Body.Close()
- close(done)
+ done <- nil
}()
// Give request time to start
time.Sleep(50 * time.Millisecond)
// Shutdown server
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()
start := time.Now()
if err := srv.Shutdown(ctx); err != nil {
t.Fatalf("shutdown failed: %v", err)
}
WaitForInFlightRequests()
elapsed := time.Since(start)
- <-done
+ if err := <-done; err != nil {
+ t.Fatalf("request failed: %v", err)
+ }
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Give request time to start | ||||||||||||||||||||||||||||||||||||||||||
| time.Sleep(50 * time.Millisecond) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+46
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sleep-based synchronization creates a race condition. The 50ms sleep (line 47) assumes the background request will have started by then, but this is not guaranteed. The request might:
This makes the test flaky. β±οΈ Proposed fix using proper synchronizationUse a channel to signal when the request has actually started: // Make request in background
- done := make(chan struct{})
+ requestStarted := make(chan struct{})
+ requestDone := make(chan struct{})
+
go func() {
+ // Signal that we're about to make the request
+ close(requestStarted)
resp, err := http.Get(ln.URL + "/slow")
if err != nil {
t.Errorf("request failed: %v", err)
return
}
resp.Body.Close()
- close(done)
+ close(requestDone)
}()
- // Give request time to start
- time.Sleep(50 * time.Millisecond)
+ // Wait for request to actually start
+ <-requestStarted
+ // Give it a moment to enter the handler
+ time.Sleep(10 * time.Millisecond)Or better yet, modify the slow handler to signal when it starts: + handlerStarted := make(chan struct{})
r.GET("/slow", func(c *gin.Context) {
+ close(handlerStarted)
time.Sleep(200 * time.Millisecond)
c.Status(http.StatusOK)
})
// ... later ...
- // Give request time to start
- time.Sleep(50 * time.Millisecond)
+ // Wait for handler to start
+ <-handlerStarted
π€ Prompt for AI Agents
Comment on lines
+46
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race condition: Sleep-based synchronization is unreliable Using
Better approach: Use proper synchronization with channels or wait for the request to actually be in-flight. Example: started := make(chan struct{})
go func() {
// Signal when request enters the handler
close(started)
resp, err := http.Get(ln.URL + "/slow")
// ...
}()
<-started // Wait for request to actually startOr check Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/shutdown_test.go
Line: 46:47
Comment:
**Race condition: Sleep-based synchronization is unreliable**
Using `time.Sleep(50 * time.Millisecond)` to "give request time to start" is a classic test race condition:
- On slow systems (CI, loaded machines), 50ms may not be enough and the test will fail
- On fast systems, the request might already be complete by then
- This makes tests flaky and unpredictable
**Better approach**: Use proper synchronization with channels or wait for the request to actually be in-flight. Example:
```go
started := make(chan struct{})
go func() {
// Signal when request enters the handler
close(started)
resp, err := http.Get(ln.URL + "/slow")
// ...
}()
<-started // Wait for request to actually start
```
Or check `GetActiveRequestCount() > 0` in a loop with timeout instead of sleeping.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Shutdown server | ||||||||||||||||||||||||||||||||||||||||||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||||||||||||||||||||||||||||||||||||||||||
| defer cancel() | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| start := time.Now() | ||||||||||||||||||||||||||||||||||||||||||
| if err := srv.Shutdown(ctx); err != nil { | ||||||||||||||||||||||||||||||||||||||||||
| t.Fatalf("shutdown failed: %v", err) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| WaitForInFlightRequests() | ||||||||||||||||||||||||||||||||||||||||||
| elapsed := time.Since(start) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+53
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test doesn't actually verify what it claims to test This test has a fundamental flaw: The test is measuring that What should be tested instead:
Current test: Measures Shutdown() waiting for requests β (not our code) Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/shutdown_test.go
Line: 53:59
Comment:
**Test doesn't actually verify what it claims to test**
This test has a fundamental flaw: `srv.Shutdown(ctx)` on line 54 already waits for active connections to complete (this is documented behavior of http.Server.Shutdown). So by the time line 58 calls `WaitForInFlightRequests()`, the request has already finished.
The test is measuring that `http.Server.Shutdown()` works (which is a given), not that our middleware tracking works correctly.
**What should be tested instead:**
1. Verify that `GetActiveRequestCount()` returns the correct count WHILE requests are in-flight
2. Test that requests complete successfully (check response status code)
3. Test the interaction between the tracking middleware and actual shutdown
Current test: Measures Shutdown() waiting for requests β (not our code)
Should test: Our tracking middleware correctly counts requests β (not tested)
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| <-done | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Assert shutdown waited for request | ||||||||||||||||||||||||||||||||||||||||||
| if elapsed < 200*time.Millisecond { | ||||||||||||||||||||||||||||||||||||||||||
| t.Fatalf("shutdown did not wait for in-flight request") | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency is unrelated to graceful shutdown functionality and is not used anywhere in the codebase. This should be removed from this PR as it:
Verified with:
grep -r "zkvm_runtime" --include="*.go" .(no matches found)Prompt To Fix With AI