fix: wait for HTTP server drain during graceful shutdown#1745
fix: wait for HTTP server drain during graceful shutdown#1745AmanGIT07 wants to merge 3 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, timed shutdown contexts, and shutdown outcome logging. Tests cover 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>
There was a problem hiding this comment.
This fixes a real problem. http.Server.Shutdown closes the listener first and finishes requests after. So ListenAndServe returns while requests are still running. StartServer in cmd/serve.go then runs its deferred Close() calls right away (DB client, session service, billing services). Before this change, those could close while requests were still being served.
The WaitGroup approach is correct. The wait cannot hang, because server.Shutdown returns within the 10s grace period. If the server fails to start, the function returns before the Wait(), so a bad port cannot deadlock. The metrics server shutdown also improves: it now has a timeout, logs errors, and gets waited on.
Two things worth fixing before merge (details inline):
- The wait does not cover h2c connections. That is most gRPC traffic on this server.
- The new test passes with or without the fix, so it does not prove the new behavior.
Not part of this PR, but related: ServeUI has no shutdown handling at all. Its http.ListenAndServe ignores the context.
| // while in-flight requests are still draining. Wait for the drain to | ||
| // finish so callers don't tear down the database and other dependencies | ||
| // under requests that are still running. | ||
| shutdownWG.Wait() |
There was a problem hiding this comment.
This wait only helps HTTP/1.1 requests. The server wraps the mux in h2c.NewHandler(mux, &http2.Server{}). h2c hijacks the connection and hands it to http2.Server.ServeConn. The net/http docs say Shutdown does not wait for or close hijacked connections.
I checked x/net v0.44.0 (the version in go.mod). The hook that makes shutdown send GOAWAY to HTTP/2 connections is only set up inside http2.ConfigureServer, and this code never calls it. So gRPC clients get no GOAWAY and no wait (gRPC needs HTTP/2, and over plain TCP that means h2c). Their requests can still get cut when the process shuts down.
Two suggestions:
- Call
http2.ConfigureServer(server, h2s)with the samehttp2.Serveryou pass toh2c.NewHandler. ThenShutdownsends GOAWAY, and h2c clients stop opening new streams. This is the known workaround from x/net/http2/h2c: support closure of all connections and graceful shutdown golang/go#26682. It still does not wait for running streams, but it is much better than nothing. - The comment above this line promises more than the code does: the wait covers HTTP/1.1 requests, not hijacked h2c connections. It would be accurate as something like: "Wait for Shutdown to finish draining HTTP/1.1 requests. Hijacked h2c connections are not tracked by Shutdown and are not waited on."
| logger.ErrorContext(ctxShutdown, "metrics server shutdown error", "err", err) | ||
| return | ||
| } | ||
| logger.Info("Graceful shutdown of metrics server complete") |
There was a problem hiding this comment.
Nit: if the metrics server never started (say the port was busy), this goroutine still logs "Graceful shutdown of metrics server complete" when the context is canceled. A bit misleading. Not blocking.
| t.Fatalf("server did not respond at %s in time", url) | ||
| } | ||
|
|
||
| func TestServeConnectReturnsAfterShutdownOnContextCancel(t *testing.T) { |
There was a problem hiding this comment.
This test also passes on main, without the WaitGroup. There, ServeConnect returned after cancel too, just too early. So the test does not prove the fix. It is still worth keeping: the 15s timeout would catch a future deadlock in the Wait.
One way to test the real behavior, which also removes duplication: the shutdown goroutine now appears twice (connect and metrics, almost identical). Pull it into a small helper like gracefulShutdown(ctx, logger, srv, name, wg). Then unit test the helper with a plain http.Server and a slow handler: start a request that sleeps, cancel the context, and check that the serve function only returns after the request finished.
Two small notes on the test itself:
freePortcloses the probe listener and then reuses the port. Another process can grab it in between. This is the standard trick, just a known source of flakes in CI.- The test boots the full connect stack with an empty
api.Deps{}. That works today. It will break if handler construction ever touches deps, for reasons unrelated to shutdown.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 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