From 16a475b6cadadacd14e9d4b5d7984d1fd0b650f2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 10:44:08 +0200 Subject: [PATCH 1/3] fix(server): wait for background DB init before returning from shutdown startBackgroundInit now returns a channel closed when the initializer exits, and StartHTTPServer waits on it after server.Shutdown inside the same 30s grace period. Without the wait, main ran app.Close while ensureDB might still be writing app.DB. The test blocks ensureDB in secret resolution, sends SIGTERM, and asserts StartHTTPServer does not return until the initializer exits. Closes #664 Co-Authored-By: Claude Sonnet 5.5 --- internal/server/http.go | 23 +++++++-- internal/server/http_shutdown_test.go | 71 +++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 internal/server/http_shutdown_test.go diff --git a/internal/server/http.go b/internal/server/http.go index 79a6b2efe..5c41cc613 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 @@ -159,7 +168,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() { @@ -182,6 +191,12 @@ func StartHTTPServer(app *Application, port int) error { 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..3e1682536 --- /dev/null +++ b/internal/server/http_shutdown_test.go @@ -0,0 +1,71 @@ +package server + +import ( + "context" + "errors" + "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{} +} + +func (r *blockingResolver) GetSecret(context.Context, string) (string, error) { + close(r.entered) + <-r.release + return "", errors.New("released") +} + +// 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. +func TestStartHTTPServerWaitsForBackgroundInitOnShutdown(t *testing.T) { + resolver := &blockingResolver{entered: make(chan struct{}), release: make(chan struct{})} + app := &Application{ + Version: "test", + dbConfig: &database.Config{PasswordSecret: "db-password"}, + secretResolver: resolver, + initRetryDelay: time.Millisecond, + } + + returned := make(chan error, 1) + go func() { returned <- StartHTTPServer(app, 0) }() + + select { + case <-resolver.entered: + case <-time.After(5 * time.Second): + t.Fatal("background initialization never started") + } + + // The signal handler is installed before the initializer starts, so the + // process-wide SIGTERM is caught by StartHTTPServer's NotifyContext. + if err := syscall.Kill(syscall.Getpid(), syscall.SIGTERM); err != nil { + t.Fatalf("send SIGTERM: %v", err) + } + + select { + case <-returned: + t.Fatal("StartHTTPServer returned while background initialization was still running") + case <-time.After(300 * time.Millisecond): + } + + close(resolver.release) + 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") + } +} From bab6c251dd19031b38cc02fb3b0aa74d233bdda4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 10:54:53 +0200 Subject: [PATCH 2/3] test(server): pin shutdown order, grace bound and no-DB done channel Three mutants survived review of the shutdown wait: an unbounded wait, a no-DB path that leaves the done channel open, and the wait placed before server.Shutdown. Make the grace period a variable so a test can shorten it, assert the listener is closed while the initializer is still parked, assert the no-DB channel is already closed, and harden the blocking resolver (sync.Once, release on cleanup). Co-Authored-By: Claude Sonnet 5.5 --- internal/server/http.go | 6 +- internal/server/http_shutdown_test.go | 93 +++++++++++++++++++++++---- internal/server/readiness_test.go | 9 ++- 3 files changed, 93 insertions(+), 15 deletions(-) diff --git a/internal/server/http.go b/internal/server/http.go index 5c41cc613..52cac5508 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -155,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 @@ -186,7 +190,7 @@ 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) diff --git a/internal/server/http_shutdown_test.go b/internal/server/http_shutdown_test.go index 3e1682536..cdc8a3a72 100644 --- a/internal/server/http_shutdown_test.go +++ b/internal/server/http_shutdown_test.go @@ -3,6 +3,9 @@ package server import ( "context" "errors" + "fmt" + "net" + "sync" "syscall" "testing" "time" @@ -16,21 +19,36 @@ import ( // running when the shutdown signal arrives. type blockingResolver struct { secrets.Resolver - entered chan struct{} - release chan struct{} + entered chan struct{} + release chan struct{} + enteredOnce sync.Once + releaseOnce sync.Once } func (r *blockingResolver) GetSecret(context.Context, string) (string, error) { - close(r.entered) + r.enteredOnce.Do(func() { close(r.entered) }) <-r.release return "", errors.New("released") } -// 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. -func TestStartHTTPServerWaitsForBackgroundInitOnShutdown(t *testing.T) { - resolver := &blockingResolver{entered: make(chan struct{}), release: make(chan struct{})} +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"}, @@ -38,20 +56,48 @@ func TestStartHTTPServerWaitsForBackgroundInitOnShutdown(t *testing.T) { initRetryDelay: time.Millisecond, } - returned := make(chan error, 1) - go func() { returned <- StartHTTPServer(app, 0) }() + 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") } + return port, resolver, ret +} - // The signal handler is installed before the initializer starts, so the - // process-wide SIGTERM is caught by StartHTTPServer's NotifyContext. +// 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: @@ -59,7 +105,7 @@ func TestStartHTTPServerWaitsForBackgroundInitOnShutdown(t *testing.T) { case <-time.After(300 * time.Millisecond): } - close(resolver.release) + resolver.unblock() select { case err := <-returned: if err != nil { @@ -69,3 +115,24 @@ func TestStartHTTPServerWaitsForBackgroundInitOnShutdown(t *testing.T) { 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") + } } From b764ddc8cbee0acb8579c51d00b349557fd6a66d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 11:05:33 +0200 Subject: [PATCH 3/3] test(server): wait for the listener before signalling shutdown The "listener closed after SIGTERM" check could pass only because the server had not bound its port yet, so the wait-before-Shutdown mutant was not reliably killed. Poll with a deadline until the server accepts a connection before the helper returns, so SIGTERM is sent only once the listener is provably up. Co-Authored-By: Claude Sonnet 5.5 --- internal/server/http_shutdown_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/internal/server/http_shutdown_test.go b/internal/server/http_shutdown_test.go index cdc8a3a72..32e895fb7 100644 --- a/internal/server/http_shutdown_test.go +++ b/internal/server/http_shutdown_test.go @@ -64,6 +64,22 @@ func startServerWithBlockedInit(t *testing.T) (port int, resolver *blockingResol 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 }