fix(server): wait for background DB init before returning from shutdown

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
 	}
 }

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")
+	}
+}

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")
+	}
 }