fix: wait for HTTP server drain during graceful shutdown#1745
fix: wait for HTTP server drain during graceful shutdown#1745AmanGIT07 wants to merge 2 commits into
Conversation
ServeConnect returned as soon as the listener closed, before in-flight requests finished draining, so callers tore down the database while requests were still running. Track the shutdown goroutines with a WaitGroup and wait for them before returning. Bound the metrics server shutdown with the same grace period and log its error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughServeConnect now coordinates graceful shutdown for the connect and optional metrics HTTP servers with a shared WaitGroup, uses timed shutdown contexts, logs shutdown outcomes, and waits for shutdown work to finish before returning. A new test covers context-cancelled shutdown for both server configurations. ChangesServeConnect Shutdown Coordination
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/server.go (1)
268-276: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEarly return on
ListenAndServefailure skipsshutdownWG.Wait.If
ListenAndServereturns a non-ErrServerClosederror (e.g., port bind failure),ServeConnectreturns at line 269 without waiting for the shutdown goroutine, which is still blocked on<-ctx.Done(). The goroutine will eventually unblock when the caller cancelsctx, but it outlives the function call. Consider returning aftershutdownWG.Wait()on this path as well, or documenting that the error path intentionally skips the wait.🔧 Proposed fix to wait on all return paths
// Start server if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { + // Ensure shutdown goroutines complete even on the error path. + shutdownWG.Wait() return fmt.Errorf("connect server failed: %w", err) }Note: the caller must cancel
ctxfor the shutdown goroutines to unblock; if they don't,Waitwill block indefinitely. Alternatively, keep the early return but document the intentional skip.
🧹 Nitpick comments (1)
pkg/server/server_test.go (1)
43-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing actual in-flight request drain behavior.
The test verifies
ServeConnectreturns after context cancellation, but doesn't verify the core PR claim: that in-flight requests finish draining beforeServeConnectreturns. A long-running handler that blocks until a signal would prove the drain actually waits. Without this, a regression that removesshutdownWG.Wait()would still pass this test (sinceShutdowncloses the listener quickly).💡 Suggested drain-verification test case
func TestServeConnectDrainsInflightRequests(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) var cfg Config cfg.Connect.Port = freePort(t) // Register a slow handler via a custom mux that ServeConnect can use. // Since ServeConnect builds its own mux, you'd need to either: // 1. Add a test-only hook to inject handlers, or // 2. Use the /ping endpoint with a deliberate delay via a wrapper. // // Minimal approach: verify that after cancel, an in-flight /ping // request still completes successfully. // This would require either a test hook in ServeConnect or // a separate test that directly exercises server.Shutdown + WaitGroup. }This may require a small test hook in
ServeConnectto inject a slow handler; if that's too invasive, consider a unit test for the shutdown coordination logic in isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1a4f9c2-0e14-4c56-b9e7-c5b10d0ebd5e
📒 Files selected for processing (2)
pkg/server/server.gopkg/server/server_test.go
Coverage Report for CI Build 29017513952Coverage increased (+0.3%) to 45.205%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Graceful shutdown now finishes draining in-flight requests before the
process tears down its dependencies.
Changes
ServeConnecttracks its shutdown goroutines with async.WaitGroupand waits for them after
ListenAndServereturns.server.Shutdownreturns early instead of also logging the"shutdown complete" message.
server, and its error is logged instead of dropped.
Technical Details
http.Server.Shutdowncloses the listener first and drains activerequests afterwards, so
ListenAndServereturns before the drainfinishes.
ServeConnectnow waits for the shutdown goroutines, so itonly returns once requests are done.
Test Plan
TestServeConnectReturnsAfterShutdownOnContextCancel, runsthe real server with and without the metrics listener
go test -race ./pkg/server/...passesgolangci-lint run pkg/server/reports 0 issues🤖 Generated with Claude Code