diff --git a/internal/server/http.go b/internal/server/http.go index 79a6b2efe..52cac5508 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -110,11 +110,20 @@ func (app *Application) backgroundInitDelay() time.Duration { // outage at boot does not permanently strand the replica. Lambda does not use // this path: cold starts there must stay lazy to avoid paying the database // connection cost on invocations that never touch the database. -func (app *Application) startBackgroundInit(ctx context.Context) { +// +// The returned channel is closed once the initializer has exited, so shutdown +// can wait for it before app.Close reads app.DB. +func (app *Application) startBackgroundInit(ctx context.Context) <-chan struct{} { + done := make(chan struct{}) if app.dbConfig == nil { - return // Not using PostgreSQL; nothing to initialize. + close(done) // Not using PostgreSQL; nothing to initialize. + return done } - go app.initializeUntilReady(ctx, app.ensureDB) + go func() { + defer close(done) + app.initializeUntilReady(ctx, app.ensureDB) + }() + return done } // initializeUntilReady calls init until it succeeds or ctx is done, so a @@ -146,6 +155,10 @@ func (app *Application) initializeUntilReady(ctx context.Context, init func(cont } } +// shutdownGracePeriod bounds draining in-flight requests plus waiting for the +// background initializer. A variable so tests can shorten it. +var shutdownGracePeriod = 30 * time.Second + // StartHTTPServer starts the HTTP server with graceful shutdown on SIGINT/SIGTERM. // It blocks until the server exits cleanly. In container orchestrators (Cloud Run, // Container Apps, Fargate) SIGTERM is the normal stop signal; without this wiring @@ -159,7 +172,7 @@ func StartHTTPServer(app *Application, port int) error { sigCtx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() - app.startBackgroundInit(sigCtx) + initDone := app.startBackgroundInit(sigCtx) serveErr := make(chan error, 1) go func() { @@ -177,11 +190,17 @@ func StartHTTPServer(app *Application, port int) error { // Received SIGINT or SIGTERM: drain in-flight requests then close the DB. stop() // release signal resources promptly log.Printf("Shutdown signal received; draining HTTP server (30s grace)...") - shutdownCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownGracePeriod) defer cancel() if err := server.Shutdown(shutdownCtx); err != nil { log.Printf("HTTP server forced shutdown: %v", err) } + // The initializer shares app.DB with app.Close; let it exit first. + select { + case <-initDone: + case <-shutdownCtx.Done(): + log.Printf("Background database initialization did not stop within the shutdown grace period") + } return nil } } diff --git a/internal/server/http_shutdown_test.go b/internal/server/http_shutdown_test.go new file mode 100644 index 000000000..32e895fb7 --- /dev/null +++ b/internal/server/http_shutdown_test.go @@ -0,0 +1,154 @@ +package server + +import ( + "context" + "errors" + "fmt" + "net" + "sync" + "syscall" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" + "github.com/LeanerCloud/cloud-commitments-platform/internal/secrets" +) + +// blockingResolver holds ensureDB inside secret resolution, the one step with no +// timeout of its own, so a background initialization attempt is provably still +// running when the shutdown signal arrives. +type blockingResolver struct { + secrets.Resolver + entered chan struct{} + release chan struct{} + enteredOnce sync.Once + releaseOnce sync.Once +} + +func (r *blockingResolver) GetSecret(context.Context, string) (string, error) { + r.enteredOnce.Do(func() { close(r.entered) }) + <-r.release + return "", errors.New("released") +} + +func (r *blockingResolver) unblock() { r.releaseOnce.Do(func() { close(r.release) }) } + +// startServerWithBlockedInit runs StartHTTPServer on a free port with the +// background initializer parked inside GetSecret, and returns once it is parked. +// The resolver is released on cleanup so a failing test cannot leak the goroutine. +func startServerWithBlockedInit(t *testing.T) (port int, resolver *blockingResolver, returned <-chan error) { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("reserve port: %v", err) + } + port = l.Addr().(*net.TCPAddr).Port + if err := l.Close(); err != nil { + t.Fatalf("release port: %v", err) + } + + resolver = &blockingResolver{entered: make(chan struct{}), release: make(chan struct{})} + t.Cleanup(resolver.unblock) + app := &Application{ + Version: "test", + dbConfig: &database.Config{PasswordSecret: "db-password"}, + secretResolver: resolver, + initRetryDelay: time.Millisecond, + } + + ret := make(chan error, 1) + go func() { ret <- StartHTTPServer(app, port) }() + + select { + case <-resolver.entered: + case <-time.After(5 * time.Second): + t.Fatal("background initialization never started") + } + + // Wait until the listener is bound. Otherwise a later "port is closed" check + // could pass simply because the server had not started listening yet. + addr := fmt.Sprintf("127.0.0.1:%d", port) + deadline := time.Now().Add(5 * time.Second) + for { + conn, err := net.DialTimeout("tcp", addr, 100*time.Millisecond) + if err == nil { + _ = conn.Close() + break + } + if time.Now().After(deadline) { + t.Fatalf("server never started listening on %s: %v", addr, err) + } + time.Sleep(10 * time.Millisecond) + } + return port, resolver, ret +} + +// sendSIGTERM signals this process. It is only safe after StartHTTPServer has +// installed its NotifyContext, which happens before the initializer starts. +func sendSIGTERM(t *testing.T) { + t.Helper() + if err := syscall.Kill(syscall.Getpid(), syscall.SIGTERM); err != nil { + t.Fatalf("send SIGTERM: %v", err) + } +} + +// TestStartHTTPServerWaitsForBackgroundInitOnShutdown pins #664: StartHTTPServer +// must not return (letting main run app.Close) while the background initializer +// is still inside ensureDB, because both touch app.DB. It also pins the order: +// the listener is closed (Shutdown ran) while the initializer is still parked, +// so the wait comes after draining, not before. +func TestStartHTTPServerWaitsForBackgroundInitOnShutdown(t *testing.T) { + port, resolver, returned := startServerWithBlockedInit(t) + sendSIGTERM(t) + + addr := fmt.Sprintf("127.0.0.1:%d", port) + deadline := time.Now().Add(5 * time.Second) + for { + conn, err := net.DialTimeout("tcp", addr, 100*time.Millisecond) + if err != nil { + break + } + _ = conn.Close() + if time.Now().After(deadline) { + t.Fatal("listener still accepting connections while shutdown should have started") + } + time.Sleep(10 * time.Millisecond) + } + + select { + case <-returned: + t.Fatal("StartHTTPServer returned while background initialization was still running") + case <-time.After(300 * time.Millisecond): + } + + resolver.unblock() + select { + case err := <-returned: + if err != nil { + t.Fatalf("StartHTTPServer returned %v after initialization stopped", err) + } + case <-time.After(5 * time.Second): + t.Fatal("StartHTTPServer did not return after the initializer exited") + } +} + +// TestStartHTTPServerShutdownWaitIsBounded pins the grace-period bound: an +// initializer that never exits (a wedged secret store ignoring its context) +// must delay shutdown by at most shutdownGracePeriod, not forever. +func TestStartHTTPServerShutdownWaitIsBounded(t *testing.T) { + old := shutdownGracePeriod + shutdownGracePeriod = 200 * time.Millisecond + t.Cleanup(func() { shutdownGracePeriod = old }) + + _, _, returned := startServerWithBlockedInit(t) + sendSIGTERM(t) + + select { + case err := <-returned: + if err != nil { + t.Fatalf("StartHTTPServer returned %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("StartHTTPServer did not return after the shutdown grace period with the initializer still blocked") + } +} diff --git a/internal/server/readiness_test.go b/internal/server/readiness_test.go index e0d4b1e8b..f68f77802 100644 --- a/internal/server/readiness_test.go +++ b/internal/server/readiness_test.go @@ -295,5 +295,12 @@ func TestStartBackgroundInitSkipsWhenNoDatabase(t *testing.T) { defer cancel() // Returns synchronously; if it started a loop it would block here. - app.startBackgroundInit(ctx) + done := app.startBackgroundInit(ctx) + + // Nothing to wait for, so shutdown must not stall on a channel nobody closes. + select { + case <-done: + default: + t.Fatal("done channel must already be closed when there is no database") + } }