From 05a9de9d10daa5a670f8589ee4da086258f79ccc Mon Sep 17 00:00:00 2001 From: aman Date: Thu, 9 Jul 2026 17:30:36 +0530 Subject: [PATCH 1/4] fix: wait for HTTP server drain during graceful shutdown 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 --- pkg/server/server.go | 27 ++++++++++-- pkg/server/server_test.go | 87 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 pkg/server/server_test.go diff --git a/pkg/server/server.go b/pkg/server/server.go index 315da19a5..c5502dc94 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -4,17 +4,17 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "net/http" "net/http/httputil" "net/url" "strings" + "sync" "time" "golang.org/x/net/http2" "golang.org/x/net/http2/h2c" - "log/slog" - "github.com/prometheus/client_golang/prometheus" "github.com/raystack/salt/server/spa" @@ -208,6 +208,9 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api Handler: handler, } + // counts shutdown goroutines still draining their servers + var shutdownWG sync.WaitGroup + // start dedicated metrics server if configured if cfg.MetricsPort > 0 { metricsMux := http.NewServeMux() @@ -227,15 +230,27 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api logger.Error("metrics server failed", "err", err) } }() + shutdownWG.Add(1) go func() { + defer shutdownWG.Done() <-ctx.Done() - metricsServer.Shutdown(context.Background()) + + ctxShutdown, cancel := context.WithTimeout(context.Background(), connectServerGracePeriod) + defer cancel() + + if err := metricsServer.Shutdown(ctxShutdown); err != nil { + logger.ErrorContext(ctxShutdown, "metrics server shutdown error", "error", err) + return + } + logger.Info("Graceful shutdown of metrics server complete") }() } logger.Info("connect server starting", "port", cfg.Connect.Port) + shutdownWG.Add(1) go func() { + defer shutdownWG.Done() <-ctx.Done() ctxShutdown, cancel := context.WithTimeout(context.Background(), connectServerGracePeriod) @@ -243,6 +258,7 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api if err := server.Shutdown(ctxShutdown); err != nil { logger.ErrorContext(ctxShutdown, "HTTP shutdown error", "error", err) + return } logger.Info("Graceful shutdown of connect server complete") @@ -253,6 +269,11 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api return fmt.Errorf("connect server failed: %w", err) } + // Shutdown closes the listener first, which makes ListenAndServe return + // 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() return nil } diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go new file mode 100644 index 000000000..7d3022db1 --- /dev/null +++ b/pkg/server/server_test.go @@ -0,0 +1,87 @@ +package server + +import ( + "context" + "fmt" + "io" + "log/slog" + "net" + "net/http" + "testing" + "time" + + "github.com/prometheus/client_golang/prometheus" + "github.com/raystack/frontier/internal/api" +) + +func freePort(t *testing.T) int { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("could not find a free port: %v", err) + } + defer l.Close() + return l.Addr().(*net.TCPAddr).Port +} + +func waitForHTTP(t *testing.T, url string) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + resp, err := http.Get(url) + if err == nil { + resp.Body.Close() + if resp.StatusCode == http.StatusOK { + return + } + } + time.Sleep(50 * time.Millisecond) + } + t.Fatalf("server did not respond at %s in time", url) +} + +func TestServeConnectReturnsAfterShutdownOnContextCancel(t *testing.T) { + tests := []struct { + name string + withMetrics bool + }{ + {name: "connect server only", withMetrics: false}, + {name: "connect and metrics servers", withMetrics: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + var cfg Config + cfg.Connect.Port = freePort(t) + if tc.withMetrics { + cfg.MetricsPort = freePort(t) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + served := make(chan error, 1) + go func() { + served <- ServeConnect(ctx, logger, cfg, api.Deps{}, prometheus.NewRegistry()) + }() + + waitForHTTP(t, fmt.Sprintf("http://127.0.0.1:%d/ping", cfg.Connect.Port)) + if tc.withMetrics { + waitForHTTP(t, fmt.Sprintf("http://127.0.0.1:%d/metrics", cfg.MetricsPort)) + } + + cancel() + + select { + case err := <-served: + if err != nil { + t.Fatalf("ServeConnect returned error: %v", err) + } + case <-time.After(15 * time.Second): + t.Fatal("ServeConnect did not return after context cancel") + } + }) + } +} From 20759068311dc0b0a17ae15b750059c8da0d3962 Mon Sep 17 00:00:00 2001 From: aman Date: Thu, 9 Jul 2026 17:44:54 +0530 Subject: [PATCH 2/4] fix: use "err" log key in shutdown error logs Co-Authored-By: Claude Fable 5 --- pkg/server/server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/server/server.go b/pkg/server/server.go index c5502dc94..456a28118 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -239,7 +239,7 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api defer cancel() if err := metricsServer.Shutdown(ctxShutdown); err != nil { - logger.ErrorContext(ctxShutdown, "metrics server shutdown error", "error", err) + logger.ErrorContext(ctxShutdown, "metrics server shutdown error", "err", err) return } logger.Info("Graceful shutdown of metrics server complete") @@ -257,7 +257,7 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api defer cancel() if err := server.Shutdown(ctxShutdown); err != nil { - logger.ErrorContext(ctxShutdown, "HTTP shutdown error", "error", err) + logger.ErrorContext(ctxShutdown, "HTTP shutdown error", "err", err) return } From 86a21685e693b493135f25146163352fdb40fd36 Mon Sep 17 00:00:00 2001 From: aman Date: Mon, 13 Jul 2026 12:15:54 +0530 Subject: [PATCH 3/4] docs: note why the listen error path skips the shutdown wait Co-Authored-By: Claude Fable 5 --- pkg/server/server.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/server/server.go b/pkg/server/server.go index 456a28118..fd55831eb 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -266,6 +266,8 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api // Start server if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { + // No shutdownWG.Wait here: ctx is never cancelled on this path, so the + // shutdown goroutines only unblock when the process exits right after. return fmt.Errorf("connect server failed: %w", err) } From a068555a16618149c1c82d14d3af6e563e6f74ab Mon Sep 17 00:00:00 2001 From: aman Date: Tue, 14 Jul 2026 12:25:55 +0530 Subject: [PATCH 4/4] fix: cover h2c connections in graceful shutdown and prove the drain Share the http2.Server between the h2c handler and http2.ConfigureServer so Shutdown sends GOAWAY to hijacked h2c connections. Extract the duplicated shutdown goroutine into a gracefulShutdown helper that skips its completion log when the server never started, and add a slow-handler test that fails without the drain wait. Co-Authored-By: Claude Fable 5 --- pkg/server/server.go | 80 ++++++++++++++++++++++----------------- pkg/server/server_test.go | 63 ++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 34 deletions(-) diff --git a/pkg/server/server.go b/pkg/server/server.go index fd55831eb..d491f3514 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -200,7 +200,8 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api }) // Configure and create the server - handler := h2c.NewHandler(mux, &http2.Server{}) + h2s := &http2.Server{} + handler := h2c.NewHandler(mux, h2s) handler = connectinterceptors.WithConnectCORS(handler, cfg.ConnectCors) server := &http.Server{ @@ -208,6 +209,14 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api Handler: handler, } + // Shutdown cannot reach h2c connections on its own: h2c hijacks them out + // of the server's connection tracking. This hook makes Shutdown send + // GOAWAY on them so clients finish up and reconnect elsewhere + // (golang/go#26682). + if err := http2.ConfigureServer(server, h2s); err != nil { + return fmt.Errorf("configure http2 server: %w", err) + } + // counts shutdown goroutines still draining their servers var shutdownWG sync.WaitGroup @@ -224,61 +233,64 @@ func ServeConnect(ctx context.Context, logger *slog.Logger, cfg Config, deps api Addr: fmt.Sprintf(":%d", cfg.MetricsPort), Handler: metricsMux, } + metricsFailed := make(chan struct{}) go func() { logger.Info("metrics server starting", "port", cfg.MetricsPort) if err := metricsServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { logger.Error("metrics server failed", "err", err) + close(metricsFailed) } }() shutdownWG.Add(1) - go func() { - defer shutdownWG.Done() - <-ctx.Done() - - ctxShutdown, cancel := context.WithTimeout(context.Background(), connectServerGracePeriod) - defer cancel() - - if err := metricsServer.Shutdown(ctxShutdown); err != nil { - logger.ErrorContext(ctxShutdown, "metrics server shutdown error", "err", err) - return - } - logger.Info("Graceful shutdown of metrics server complete") - }() + go gracefulShutdown(ctx, logger, &shutdownWG, metricsServer, "metrics server", metricsFailed) } logger.Info("connect server starting", "port", cfg.Connect.Port) + serveFailed := make(chan struct{}) shutdownWG.Add(1) - go func() { - defer shutdownWG.Done() - <-ctx.Done() - - ctxShutdown, cancel := context.WithTimeout(context.Background(), connectServerGracePeriod) - defer cancel() - - if err := server.Shutdown(ctxShutdown); err != nil { - logger.ErrorContext(ctxShutdown, "HTTP shutdown error", "err", err) - return - } - - logger.Info("Graceful shutdown of connect server complete") - }() + go gracefulShutdown(ctx, logger, &shutdownWG, server, "connect server", serveFailed) // Start server if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { - // No shutdownWG.Wait here: ctx is never cancelled on this path, so the - // shutdown goroutines only unblock when the process exits right after. + close(serveFailed) + // No shutdownWG.Wait here: the metrics watcher only unblocks when ctx + // is cancelled, which never happens on this path; it dies with the + // process right after. return fmt.Errorf("connect server failed: %w", err) } - // Shutdown closes the listener first, which makes ListenAndServe return - // 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. + // Wait for Shutdown to finish draining HTTP/1.1 requests before returning, + // so callers don't tear down the database under them. Hijacked h2c + // connections are not tracked by Shutdown and are not waited on; they get + // GOAWAY through the http2.ConfigureServer hook instead. shutdownWG.Wait() return nil } +// gracefulShutdown drains srv within the grace period once ctx is cancelled. +// It returns without logging when the server already failed, so a server +// that never started is not reported as gracefully shut down. +func gracefulShutdown(ctx context.Context, logger *slog.Logger, wg *sync.WaitGroup, srv *http.Server, name string, failed <-chan struct{}) { + defer wg.Done() + + select { + case <-ctx.Done(): + case <-failed: + return + } + + ctxShutdown, cancel := context.WithTimeout(context.Background(), connectServerGracePeriod) + defer cancel() + + if err := srv.Shutdown(ctxShutdown); err != nil { + logger.ErrorContext(ctxShutdown, name+" shutdown error", "err", err) + return + } + + logger.Info("Graceful shutdown of " + name + " complete") +} + func getSessionCookieCutter(blockSecretKey string, hashSecretKey string, logger *slog.Logger) securecookie.Codec { var sessionCookieCutter securecookie.Codec if len(hashSecretKey) != 32 || len(blockSecretKey) != 32 { diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 7d3022db1..fc33ea814 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -7,6 +7,7 @@ import ( "log/slog" "net" "net/http" + "sync" "testing" "time" @@ -14,6 +15,68 @@ import ( "github.com/raystack/frontier/internal/api" ) +func TestGracefulShutdownDrainsInflightRequests(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + entered := make(chan struct{}) + requestDone := make(chan struct{}) + mux := http.NewServeMux() + mux.HandleFunc("/slow", func(w http.ResponseWriter, r *http.Request) { + close(entered) + time.Sleep(300 * time.Millisecond) + close(requestDone) + w.WriteHeader(http.StatusOK) + }) + + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + srv := &http.Server{Handler: mux} + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + var wg sync.WaitGroup + wg.Add(1) + go gracefulShutdown(ctx, logger, &wg, srv, "test server", make(chan struct{})) + + serveReturned := make(chan struct{}) + go func() { + defer close(serveReturned) + if err := srv.Serve(l); err != nil && err != http.ErrServerClosed { + t.Errorf("serve failed: %v", err) + } + }() + + requestErr := make(chan error, 1) + go func() { + resp, err := http.Get(fmt.Sprintf("http://%s/slow", l.Addr())) + if err == nil { + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + err = fmt.Errorf("unexpected status %d", resp.StatusCode) + } + } + requestErr <- err + }() + + <-entered + cancel() + + <-serveReturned + wg.Wait() + + select { + case <-requestDone: + default: + t.Fatal("shutdown wait released before the in-flight request finished") + } + if err := <-requestErr; err != nil { + t.Fatalf("in-flight request failed: %v", err) + } +} + func freePort(t *testing.T) int { t.Helper() l, err := net.Listen("tcp", "127.0.0.1:0")