From 78268be28bfdda568e32a4d107bed79b67af2d1a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 14:58:21 +0200 Subject: [PATCH 1/6] fix(server): add /ready endpoint with background database initialization /health always answered HTTP 200 with "degraded" in the body while lazy database initialization had not run on the process, so Azure readiness probes admitted cold replicas to traffic and the deployment smoke gate flapped (#488). - add /ready as the traffic-admission contract: 503 until this process has completed required initialization, sharing one collectHealth pipeline with /health so the two can never drift - keep /health as the liveness contract: always 200, verdict in the body - start background database initialization with retries at HTTP server startup so readiness no longer depends on user traffic; each attempt runs under a 10-minute bound so a wedged secret store becomes a retryable error instead of holding dbMu forever - collectHealth snapshots the store pointers under dbMu TryLock instead of racing ensureDB, reporting pending while initialization holds the lock; Lambda keeps lazy initialization and no /ready route - document /ready in the OpenAPI spec --- internal/api/openapi.yaml | 25 +++ internal/server/app.go | 5 + internal/server/health.go | 104 ++++++++--- internal/server/health_test.go | 4 +- internal/server/http.go | 76 ++++++++ internal/server/readiness_test.go | 299 ++++++++++++++++++++++++++++++ 6 files changed, 486 insertions(+), 27 deletions(-) create mode 100644 internal/server/readiness_test.go diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 7ae8d2691..a34f47bf1 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2114,6 +2114,31 @@ paths: schema: $ref: '#/components/schemas/HealthResponse' + /ready: + get: + operationId: readinessCheckRoot + tags: [Health] + summary: Readiness check (root path) + description: >- + Traffic-admission contract: answers 200 with the same payload as + /health once this replica has completed initialization, 503 while any + check is pending or failed. /health is the liveness counterpart and + always answers 200. + security: [] + responses: + '200': + description: Replica is ready to serve traffic + content: + application/json: + schema: + $ref: '#/components/schemas/HealthResponse' + '503': + description: Replica is still initializing or a check failed + content: + application/json: + schema: + $ref: '#/components/schemas/HealthResponse' + /api/health: get: operationId: healthCheck diff --git a/internal/server/app.go b/internal/server/app.go index d57f6bbea..a881566f8 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -86,6 +86,11 @@ type Application struct { migrationsTimeout time.Duration migrationMu sync.Mutex + // initRetryDelay is the pause between background database-initialization + // attempts (see startBackgroundInit). Zero means backgroundInitRetryDelay. + // Only tests set it. + initRetryDelay time.Duration + // encKeySource is the env var name that resolved the credential encryption // key (e.g. "CREDENTIAL_ENCRYPTION_KEY_SECRET_NAME"). Set during // reinitializeAfterConnect; surfaced via /health. diff --git a/internal/server/health.go b/internal/server/health.go index 35aba13dc..94733e55a 100644 --- a/internal/server/health.go +++ b/internal/server/health.go @@ -7,6 +7,10 @@ import ( "log" "net/http" "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" ) // HealthStatus represents the overall health of the application. @@ -23,11 +27,44 @@ type CheckResult struct { Message string `json:"message,omitempty"` } -// handleHealthCheck returns the health status of the application. +// handleHealthCheck returns the liveness status of the application. +// +// This endpoint is the LIVENESS contract: it always answers HTTP 200 so +// orchestrator liveness/startup probes never restart or kill a process that is +// up but whose dependencies are not connected yet. The verdict is in the JSON +// body ("healthy" / "degraded"). Traffic admission is a separate question +// answered by handleReadinessCheck (#488). func (app *Application) handleHealthCheck(w http.ResponseWriter, r *http.Request) { ctx, cancel := context.WithTimeout(r.Context(), 5*time.Second) defer cancel() + writeHealthResponse(w, app.appConfig.CORSAllowedOrigin, http.StatusOK, app.collectHealth(ctx)) +} + +// handleReadinessCheck returns the READINESS status of the application. +// +// Readiness is the traffic-admission contract: a replica that has not completed +// required initialization answers 503, so the load balancer stops sending it +// requests. /health only reported "degraded" in the body while still answering +// 200, which admitted cold replicas to traffic and broke the deployment smoke +// gate (#488). Non-degraded is required initialization done; the body carries +// the same checks as /health so an operator sees which one is still pending. +func (app *Application) handleReadinessCheck(w http.ResponseWriter, r *http.Request) { + ctx, cancel := context.WithTimeout(r.Context(), 5*time.Second) + defer cancel() + + health := app.collectHealth(ctx) + status := http.StatusOK + if health.Status != "healthy" { + status = http.StatusServiceUnavailable + } + writeHealthResponse(w, app.appConfig.CORSAllowedOrigin, status, health) +} + +// collectHealth runs every readiness/health check and folds them into one +// overall status. Shared by /health and /ready so the two endpoints can never +// drift on what "initialized" means. +func (app *Application) collectHealth(ctx context.Context) HealthStatus { health := HealthStatus{ Status: "healthy", Version: app.Version, @@ -35,15 +72,28 @@ func (app *Application) handleHealthCheck(w http.ResponseWriter, r *http.Request Checks: make(map[string]CheckResult), } - // Check configuration store - health.Checks["config_store"] = app.checkConfigStore(ctx) - if health.Checks["config_store"].Status != "healthy" { - health.Status = "degraded" - } + // Check configuration and auth stores. ensureDB rewrites Config/Auth/DB + // under dbMu and holds it for the full initialization, migrations + // included; these endpoints must never block behind that. Snapshot the + // pointers when the mutex is free, and while initialization holds it + // report both stores as pending rather than race on half-written state. + if app.dbMu.TryLock() { + configStore, authService, db := app.Config, app.Auth, app.DB + app.dbMu.Unlock() + + health.Checks["config_store"] = app.checkConfigStore(ctx, configStore, db) + if health.Checks["config_store"].Status != "healthy" { + health.Status = "degraded" + } - // Check auth store - health.Checks["auth_store"] = app.checkAuthStore(ctx) - if health.Checks["auth_store"].Status != "healthy" { + health.Checks["auth_store"] = app.checkAuthStore(ctx, authService) + if health.Checks["auth_store"].Status != "healthy" { + health.Status = "degraded" + } + } else { + pending := CheckResult{Status: "pending", Message: "database initialization in progress"} + health.Checks["config_store"] = pending + health.Checks["auth_store"] = pending health.Status = "degraded" } @@ -55,14 +105,14 @@ func (app *Application) handleHealthCheck(w http.ResponseWriter, r *http.Request health.Status = "degraded" } - // Always return 200 for the health endpoint so startup/liveness probes pass. - // The actual health status is in the JSON body. "degraded" means the app is - // running but some dependencies (like DB) aren't connected yet - this is - // expected during cold starts with lazy DB initialization. + return health +} - // Write response with security headers and CORS - setHealthResponseHeaders(w, app.appConfig.CORSAllowedOrigin) - w.WriteHeader(http.StatusOK) +// writeHealthResponse encodes a health payload with the security headers and +// CORS both health endpoints share. +func writeHealthResponse(w http.ResponseWriter, corsOrigin string, status int, health HealthStatus) { + setHealthResponseHeaders(w, corsOrigin) + w.WriteHeader(status) if err := json.NewEncoder(w).Encode(health); err != nil { // Body write failed after headers are sent; log for parity with // handleScheduledHTTP which already logs its encode error (04-L4). @@ -98,10 +148,12 @@ func (app *Application) checkMigrations() CheckResult { } } -// checkConfigStore checks the health of the configuration store. -func (app *Application) checkConfigStore(ctx context.Context) CheckResult { +// checkConfigStore checks the health of the configuration store. The store and +// connection come in as a snapshot taken under dbMu by collectHealth, so this +// function never reads pointers ensureDB may be rewriting. +func (app *Application) checkConfigStore(ctx context.Context, configStore config.StoreInterface, db *database.Connection) CheckResult { // Check if config store exists - if app.Config == nil { + if configStore == nil { // If using PostgreSQL with lazy initialization, DB might not be connected yet if app.dbConfig != nil { return CheckResult{ @@ -116,8 +168,8 @@ func (app *Application) checkConfigStore(ctx context.Context) CheckResult { } // If using PostgreSQL, check database connection health - if app.DB != nil { - if err := app.DB.HealthCheck(ctx); err != nil { + if db != nil { + if err := db.HealthCheck(ctx); err != nil { return CheckResult{ Status: "unhealthy", Message: fmt.Sprintf("Database health check failed: %v", err), @@ -154,9 +206,11 @@ func setHealthResponseHeaders(w http.ResponseWriter, corsOrigin string) { } } -// checkAuthStore checks the health of the auth store. -func (app *Application) checkAuthStore(ctx context.Context) CheckResult { - if app.Auth == nil { +// checkAuthStore checks the health of the auth store. The service comes in as +// a snapshot taken under dbMu by collectHealth, so this function never reads a +// pointer ensureDB may be rewriting. +func (app *Application) checkAuthStore(ctx context.Context, authService *auth.Service) CheckResult { + if authService == nil { return CheckResult{ Status: "unhealthy", Message: "Auth service not initialized", @@ -164,7 +218,7 @@ func (app *Application) checkAuthStore(ctx context.Context) CheckResult { } // Ping the database to verify connection is healthy - if err := app.Auth.Ping(ctx); err != nil { + if err := authService.Ping(ctx); err != nil { return CheckResult{ Status: "unhealthy", Message: fmt.Sprintf("Auth store ping failed: %v", err), diff --git a/internal/server/health_test.go b/internal/server/health_test.go index ff2c55263..ac3cdf451 100644 --- a/internal/server/health_test.go +++ b/internal/server/health_test.go @@ -378,7 +378,7 @@ func TestCheckConfigStore(t *testing.T) { tt.setupApp(app) } - result := app.checkConfigStore(ctx) + result := app.checkConfigStore(ctx, app.Config, app.DB) testutil.AssertEqual(t, tt.expectedStatus, result.Status) }) } @@ -414,7 +414,7 @@ func TestCheckAuthStore(t *testing.T) { tt.setupApp(app) } - result := app.checkAuthStore(ctx) + result := app.checkAuthStore(ctx, app.Auth) testutil.AssertEqual(t, tt.expectedStatus, result.Status) }) } diff --git a/internal/server/http.go b/internal/server/http.go index a275b57c8..5e8bf3e16 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -29,6 +29,12 @@ func CreateHTTPServer(app *Application, port int) *http.Server { // SCHEDULED_TASK_AUTH_MODE: oidc on GCP, bearer on Azure, disabled // for local dev. mux.HandleFunc("/health", app.handleHealthCheck) + // /ready is the traffic-admission contract, distinct from the /health + // liveness contract: it answers 503 until required initialization has + // completed on THIS process, so the load balancer stops routing to cold + // replicas (#488). Registered explicitly like /health and /version because + // the STATIC_DIR catch-all would otherwise serve the SPA index here. + mux.HandleFunc("/ready", app.handleReadinessCheck) // /version is a root-path (no /api prefix) public endpoint. Like /health // it must be registered explicitly: when STATIC_DIR is set the catch-all // "/" route serves the SPA, so an unregistered /version would return the @@ -72,6 +78,74 @@ func CreateHTTPServer(app *Application, port int) *http.Server { } } +// backgroundInitRetryDelay is the pause between background ensureDB attempts. +// Startup failures are usually a database that is not accepting connections +// yet, so retrying beats leaving the replica permanently unready. +const backgroundInitRetryDelay = 5 * time.Second + +// backgroundInitAttemptTimeout bounds a single background initialization +// attempt. It must cover the worst case of ensureDB's own bounded phases +// (connect retries plus migrationsTimeout); secret resolution inside ensureDB +// has no timeout of its own, so this cap is what turns a wedged attempt into +// a retryable error instead of a replica that holds dbMu forever. +const backgroundInitAttemptTimeout = 10 * time.Minute + +// backgroundInitDelay is the retry interval the background initializer uses. +// It reads the initRetryDelay field so tests can run the loop without waiting +// out the production interval; zero means the production default. +func (app *Application) backgroundInitDelay() time.Duration { + if app.initRetryDelay > 0 { + return app.initRetryDelay + } + return backgroundInitRetryDelay +} + +// startBackgroundInit initializes the database without waiting for user +// traffic. Readiness (#488) answers 503 until initialization completes, so a +// replica that only initializes on the first /api/ request would never receive +// that request through the load balancer and would stay unready forever. Each +// HTTP-serving process must therefore drive its own initialization. +// +// Retries on failure until it succeeds or ctx is done, so a transient database +// 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) { + if app.dbConfig == nil { + return // Not using PostgreSQL; nothing to initialize. + } + go app.initializeUntilReady(ctx, app.ensureDB) +} + +// initializeUntilReady calls init until it succeeds or ctx is done, so a +// transient database outage at boot does not permanently strand the replica out +// of rotation. It always makes at least one attempt before consulting ctx, so a +// context that is already cancelled cannot skip initialization entirely. +// +// Each attempt runs under backgroundInitAttemptTimeout: ensureDB bounds its +// connect and migration phases, but secret resolution has no timeout of its +// own, so without a per-attempt bound a wedged secret store would hold dbMu +// forever and leave the replica unready-but-alive with no retry. +func (app *Application) initializeUntilReady(ctx context.Context, init func(context.Context) error) { + for { + attemptCtx, cancel := context.WithTimeout(ctx, backgroundInitAttemptTimeout) + err := init(attemptCtx) + cancel() + if err == nil { + log.Println("Background database initialization complete") + return + } + delay := app.backgroundInitDelay() + log.Printf("Background database initialization failed: %v (retrying in %s)", err, delay) + select { + case <-ctx.Done(): + log.Printf("Background database initialization stopped: %v", ctx.Err()) + return + case <-time.After(delay): + } + } +} + // 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 @@ -85,6 +159,8 @@ func StartHTTPServer(app *Application, port int) error { sigCtx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() + app.startBackgroundInit(sigCtx) + serveErr := make(chan error, 1) go func() { serveErr <- server.ListenAndServe() diff --git a/internal/server/readiness_test.go b/internal/server/readiness_test.go new file mode 100644 index 000000000..48dd1a3e2 --- /dev/null +++ b/internal/server/readiness_test.go @@ -0,0 +1,299 @@ +package server + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" + "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil" +) + +// dbConfigForReadiness is the shape of a replica that has PostgreSQL configured +// and AutoMigrate on: its stores stay nil until initialization runs, which is +// exactly the state /ready must report as not-ready. +var dbConfigForReadiness = database.Config{Host: "unreachable", AutoMigrate: true} + +// twoColdInstances wires two independent Application values the way two cold +// replicas of the same deployment are wired: neither has been initialized, and +// warming one leaves the other untouched. Reproduces the #488 report where a +// successful warm-up request on one instance did not establish readiness on the +// other. +func twoColdInstances() (warmed, cold *Application) { + warmed = &Application{ + Version: "test", + Config: &mockConfigStoreForHealth{}, + Auth: createHealthyAuthService(), + } + cold = &Application{ + Version: "test", + dbConfig: &dbConfigForReadiness, + } + return warmed, cold +} + +// TestReadinessExcludesUninitializedReplica is the #488 regression test: a +// replica that never served user traffic must not be admitted. Pre-fix there +// was no /ready at all and /health answered 200, so the load balancer routed to +// it and the smoke test saw "degraded" from a replica nobody had initialized. +func TestReadinessExcludesUninitializedReplica(t *testing.T) { + warmed, cold := twoColdInstances() + + // The warm-up path: one instance has completed initialization. + warmedReq := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) + warmedRes := httptest.NewRecorder() + warmed.handleReadinessCheck(warmedRes, warmedReq) + testutil.AssertEqual(t, http.StatusOK, warmedRes.Code) + + // The second instance is still cold. It must answer non-2xx even though a + // sibling replica is perfectly ready: readiness is per-process. + coldReq := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) + coldRes := httptest.NewRecorder() + cold.handleReadinessCheck(coldRes, coldReq) + testutil.AssertEqual(t, http.StatusServiceUnavailable, coldRes.Code) + + var health HealthStatus + testutil.AssertNoError(t, json.Unmarshal(coldRes.Body.Bytes(), &health)) + testutil.AssertEqual(t, "degraded", health.Status) + // The body must still name the pending dependency: a 503 with an empty body + // tells an operator nothing about which check to fix. + testutil.AssertEqual(t, "pending", health.Checks["config_store"].Status) + testutil.AssertEqual(t, "unhealthy", health.Checks["auth_store"].Status) +} + +// TestReadinessPermanentInitializationFailure pins that a dependency that never +// becomes available keeps the replica out of rotation instead of admitting it +// with a 200. A cold replica that answers 503 forever is the correct outcome: +// the orchestrator's own failure threshold restarts it, and deployment +// verification fails within its budget. +func TestReadinessPermanentInitializationFailure(t *testing.T) { + // Config and Auth stay nil: the state of a process whose ensureDB keeps + // failing against an unreachable database. + app := &Application{Version: "test", dbConfig: &dbConfigForReadiness} + + for range 3 { + req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) + res := httptest.NewRecorder() + app.handleReadinessCheck(res, req) + testutil.AssertEqual(t, http.StatusServiceUnavailable, res.Code) + + var health HealthStatus + testutil.AssertNoError(t, json.Unmarshal(res.Body.Bytes(), &health)) + testutil.AssertEqual(t, "degraded", health.Status) + testutil.AssertEqual(t, "pending", health.Checks["config_store"].Status) + } +} + +// TestReadinessFailedMigrations pins the migration facet: a replica whose +// migrations permanently failed must not take traffic even though its stores +// connected. Pre-fix only the body said "degraded"; readiness now has to agree. +func TestReadinessFailedMigrations(t *testing.T) { + app := &Application{ + Version: "test", + Config: &mockConfigStoreForHealth{}, + Auth: createHealthyAuthService(), + dbConfig: &dbConfigForReadiness, + } + app.recordMigrationResult(context.DeadlineExceeded) + + req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) + res := httptest.NewRecorder() + app.handleReadinessCheck(res, req) + testutil.AssertEqual(t, http.StatusServiceUnavailable, res.Code) + + var health HealthStatus + testutil.AssertNoError(t, json.Unmarshal(res.Body.Bytes(), &health)) + testutil.AssertEqual(t, "failed", health.Checks["migrations"].Status) + testutil.AssertEqual(t, "degraded", health.Status) +} + +// TestReadinessPendingMigrations pins the pending case: AutoMigrate on, no +// migration attempt recorded yet, so this replica has not finished starting. +func TestReadinessPendingMigrations(t *testing.T) { + app := &Application{ + Version: "test", + Config: &mockConfigStoreForHealth{}, + Auth: createHealthyAuthService(), + dbConfig: &dbConfigForReadiness, + } + + req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) + res := httptest.NewRecorder() + app.handleReadinessCheck(res, req) + testutil.AssertEqual(t, http.StatusServiceUnavailable, res.Code) + + var health HealthStatus + testutil.AssertNoError(t, json.Unmarshal(res.Body.Bytes(), &health)) + testutil.AssertEqual(t, "pending", health.Checks["migrations"].Status) +} + +// TestReadinessDisabledMigrationsStayReady pins that turning AutoMigrate off is +// not a readiness failure: migrations run elsewhere, so this check reports +// "disabled" and a fully wired instance stays ready. Guards against making +// readiness stricter than the health contract it shares. +func TestReadinessDisabledMigrationsStayReady(t *testing.T) { + // Local copy: mutating the shared dbConfigForReadiness fixture would leak + // AutoMigrate=false into every other test in this file. + noMigrate := database.Config{Host: "unreachable", AutoMigrate: false} + app := &Application{ + Version: "test", + Config: &mockConfigStoreForHealth{}, + Auth: createHealthyAuthService(), + dbConfig: &noMigrate, + } + + req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) + res := httptest.NewRecorder() + app.handleReadinessCheck(res, req) + testutil.AssertEqual(t, http.StatusOK, res.Code) +} + +// TestLivenessStays200WhileDegraded pins the other half of the split: /health +// is the liveness contract and must keep answering 200 while degraded, so an +// orchestrator liveness probe never restarts a process that is up but whose +// dependencies are not connected. Pre-fix this was the only endpoint, which is +// why readiness could not be expressed at all. +func TestLivenessStays200WhileDegraded(t *testing.T) { + app := &Application{Version: "test", dbConfig: &dbConfigForReadiness} + + req := httptest.NewRequestWithContext(context.Background(), "GET", "/health", nil) + res := httptest.NewRecorder() + app.handleHealthCheck(res, req) + testutil.AssertEqual(t, http.StatusOK, res.Code) + + var health HealthStatus + testutil.AssertNoError(t, json.Unmarshal(res.Body.Bytes(), &health)) + testutil.AssertEqual(t, "degraded", health.Status) +} + +// TestReadinessWhileInitializationInProgress pins the non-blocking snapshot +// contract: while ensureDB holds dbMu, /ready must answer 503 with "pending" +// checks instead of blocking behind the initialization or racing on the store +// pointers it is rewriting. +func TestReadinessWhileInitializationInProgress(t *testing.T) { + app := &Application{Version: "test", dbConfig: &dbConfigForReadiness} + app.dbMu.Lock() + defer app.dbMu.Unlock() + + req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) + res := httptest.NewRecorder() + app.handleReadinessCheck(res, req) + testutil.AssertEqual(t, http.StatusServiceUnavailable, res.Code) + + var health HealthStatus + testutil.AssertNoError(t, json.Unmarshal(res.Body.Bytes(), &health)) + testutil.AssertEqual(t, "pending", health.Checks["config_store"].Status) + testutil.AssertEqual(t, "pending", health.Checks["auth_store"].Status) +} + +// TestReadyRouteRegistered pins that /ready is routed on the real server mux. +// A readiness handler that is never registered would leave the probe path +// serving the SPA index or 404, which an orchestrator reads as "not ready" +// forever; this test catches the wiring, not just the handler. +func TestReadyRouteRegistered(t *testing.T) { + app := &Application{ + Version: "test", + Config: &mockConfigStoreForHealth{}, + Auth: createHealthyAuthService(), + } + server := CreateHTTPServer(app, 0) + + rec := httptest.NewRecorder() + server.Handler.ServeHTTP(rec, httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil)) + testutil.AssertEqual(t, http.StatusOK, rec.Code) + + var health HealthStatus + testutil.AssertNoError(t, json.Unmarshal(rec.Body.Bytes(), &health)) + testutil.AssertEqual(t, "healthy", health.Status) +} + +// TestInitializeUntilReadyRetriesAfterTransientFailure pins the retry contract: +// a boot-time database outage must not strand the replica permanently unready. +// The first two attempts fail (a database still refusing connections), the +// third succeeds, and the loop must have made all three without any inbound +// request. Pre-fix there was no background initialization at all, so a cold +// replica stayed uninitialized until user traffic arrived. +func TestInitializeUntilReadyRetriesAfterTransientFailure(t *testing.T) { + app := &Application{Version: "test", initRetryDelay: time.Millisecond} + var mu sync.Mutex + attempts := 0 + + init := func(context.Context) error { + mu.Lock() + defer mu.Unlock() + attempts++ + if attempts < 3 { + return errors.New("dial tcp: connection refused") + } + return nil + } + + done := make(chan struct{}) + go func() { + defer close(done) + app.initializeUntilReady(context.Background(), init) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("initializeUntilReady did not return after init succeeded") + } + + mu.Lock() + defer mu.Unlock() + testutil.AssertEqual(t, 3, attempts) +} + +// TestInitializeUntilReadyStopsOnCancelledContext pins that shutdown stops the +// retry loop instead of leaving a goroutine connecting to a database that is +// going away. One attempt is always made, so an already-cancelled context still +// tries once. +func TestInitializeUntilReadyStopsOnCancelledContext(t *testing.T) { + app := &Application{Version: "test", initRetryDelay: time.Millisecond} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var mu sync.Mutex + attempts := 0 + init := func(context.Context) error { + mu.Lock() + defer mu.Unlock() + attempts++ + return errors.New("dial tcp: connection refused") + } + + done := make(chan struct{}) + go func() { + defer close(done) + app.initializeUntilReady(ctx, init) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("initializeUntilReady did not return after the context was cancelled") + } + + mu.Lock() + defer mu.Unlock() + testutil.AssertEqual(t, 1, attempts) +} + +// TestStartBackgroundInitSkipsWhenNoDatabase pins the Lambda/dev shape: with no +// dbConfig there is nothing to initialize, so no retry loop is started. Lambda +// must not pay for eager initialization on invocations that never touch the +// database. +func TestStartBackgroundInitSkipsWhenNoDatabase(t *testing.T) { + app := &Application{Version: "test"} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Returns synchronously; if it started a loop it would block here. + app.startBackgroundInit(ctx) +} From 89641a06ba7931465cfa791c6d8e6e3db80e7a50 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 14:59:19 +0200 Subject: [PATCH 2/6] fix(ci): gate Azure deployment verification on /ready The smoke gate warmed /api/ to trigger lazy initialization and then parsed the /health body, but the warm-up initialized whichever replica the load balancer picked, not the one the next /health request reached (#488). - wait-for-healthy.sh now polls /ready with curl --fail-with-body, so a 503 fails the attempt while still printing which dependency is pending; the warm-up request and HEALTH_WARMUP_PATH are gone because the server initializes in the background at startup - deploy-azure.yml drops its inline copy of the polling loop and calls the script for both the fast-fail gate (1x5) and the rollout-race gate (3x30), moving the sparse checkout ahead of them - the offline test replays the pre-fix inline loop with its original first-healthy-passes semantics to show it passed the rollout-race scenario, and the curl stub now only prints a 503 body when --fail-with-body was passed --- .github/workflows/deploy-azure.yml | 48 +++++++------------ scripts/test-wait-for-healthy.sh | 76 ++++++++++++++++++++---------- scripts/wait-for-healthy.sh | 56 +++++++++++----------- 3 files changed, 97 insertions(+), 83 deletions(-) diff --git a/.github/workflows/deploy-azure.yml b/.github/workflows/deploy-azure.yml index 5a3a18287..f3d480f5a 100644 --- a/.github/workflows/deploy-azure.yml +++ b/.github/workflows/deploy-azure.yml @@ -435,42 +435,30 @@ jobs: echo "Waiting 30 seconds for Container App to be ready..." sleep 30 - - name: Test health endpoint - run: | - URL="${{ steps.get-url.outputs.url }}" - echo "Testing health endpoint: $URL/health" - - for i in {1..5}; do - # /health reports but never triggers the lazy DB init on the HTTP - # server; any /api/ request does. /health below is the gate. - curl -s -o /dev/null --max-time 60 "$URL/api/auth/check-admin" || true - # /health returns HTTP 200 even when degraded; the body is the verdict. - if RESPONSE=$(curl -fsS --max-time 10 "$URL/health"); then - echo "Response: $RESPONSE" - if jq -e '.status == "healthy"' > /dev/null <<< "$RESPONSE"; then - echo "Health check passed!" - exit 0 - fi - fi - echo "Attempt $i failed, retrying in 10 seconds..." - sleep 10 - done - - echo "Health check failed after 5 attempts" - exit 1 - - # Needs the repo for scripts/wait-for-healthy.sh. Rollout race (#513): - # a probe can land on the new revision while it is still initializing - # (auth_store "not initialized"), so one unhealthy response must not fail - # the step. The script requires HEALTH_REQUIRED_STREAK consecutive healthy - # responses within HEALTH_MAX_ATTEMPTS rounds and fails with the last body - # otherwise. + # Needs the repo for scripts/wait-for-healthy.sh, used by both gates below. - name: Checkout code uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 with: persist-credentials: false sparse-checkout: scripts/wait-for-healthy.sh + # Fast fail gate: the server initializes its database in the background + # at startup, so no warm-up request is needed to make progress (#488). + # /ready answers 503 while this replica is still initializing, which + # curl -f turns into a failed attempt. + - name: Test readiness endpoint + env: + APP_URL: ${{ steps.get-url.outputs.url }} + HEALTH_REQUIRED_STREAK: "1" + HEALTH_MAX_ATTEMPTS: "5" + HEALTH_INTERVAL_SECONDS: "10" + run: bash scripts/wait-for-healthy.sh "$APP_URL" + + # Rollout race (#513): a probe can land on the new revision while it is + # still initializing (auth_store "not initialized"), so one not-ready + # response must not fail the step. The script requires + # HEALTH_REQUIRED_STREAK consecutive ready responses within + # HEALTH_MAX_ATTEMPTS rounds and fails with the last body otherwise. - name: Run smoke tests env: APP_URL: ${{ steps.get-url.outputs.url }} diff --git a/scripts/test-wait-for-healthy.sh b/scripts/test-wait-for-healthy.sh index 9167a49bf..9969f47c4 100755 --- a/scripts/test-wait-for-healthy.sh +++ b/scripts/test-wait-for-healthy.sh @@ -1,9 +1,9 @@ #!/usr/bin/env bash # test-wait-for-healthy.sh # -# Offline test for wait-for-healthy.sh: curl is stubbed via PATH. Also runs the -# pre-fix inline smoke loop (extracted verbatim from deploy-azure.yml) against -# the rollout-race scenario to show it fails there. +# Offline test for wait-for-healthy.sh: curl is stubbed via PATH. Also replays +# the pre-#488 inline smoke loop from deploy-azure.yml against the rollout-race +# scenario to show it passed there — the admission bug this changeset fixes. # # Exits 0 when all cases pass; exits 1 on any failure. @@ -15,12 +15,23 @@ WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT mkdir -p "$WORK/bin" -# Stub curl. Requests to /api/ (warmup) succeed silently. Each /health request -# consumes the next token from $SCENARIO; the last token repeats forever. -# Tokens: H = healthy body, U = degraded body, R = connection refused. +# Stub curl. Each health request consumes the next token from $SCENARIO; the +# last token repeats forever. +# H = HTTP 200 healthy +# D = HTTP 200 but degraded body: what /health returned pre-#488, which the +# pre-fix loop caught only by parsing the body +# U = HTTP 503 with the degraded body: what /ready returns, and what +# --fail-with-body still prints +# R = connection refused cat > "$WORK/bin/curl" <<'STUB' #!/usr/bin/env bash -for a in "$@"; do url="$a"; done +fwb=0 +for a in "$@"; do + url="$a" + [ "$a" = "--fail-with-body" ] && fwb=1 +done +# The pre-fix loop's warm-up request to /api/ succeeded silently and consumed no +# scenario token; only the gate request did. case "$url" in */api/*) exit 0 ;; esac @@ -30,8 +41,12 @@ i=$n ((i >= ${#tokens[@]})) && i=$((${#tokens[@]} - 1)) echo $((n + 1)) > "$STATE" case "${tokens[$i]}" in - H) echo '{"status":"healthy"}' ;; - U) echo '{"status":"degraded","checks":{"auth_store":{"status":"unhealthy","message":"auth store not initialized"}}}' ;; + H) echo '{"status":"healthy","checks":{}}' ;; + D) echo '{"status":"degraded","checks":{"auth_store":{"status":"unhealthy","message":"auth store not initialized"}}}' ;; + # Real curl only prints the body on a 503 when --fail-with-body was passed; + # modeling that keeps the failure-diagnostics coverage honest. + U) [ "$fwb" = 1 ] && echo '{"status":"degraded","checks":{"auth_store":{"status":"unhealthy","message":"auth store not initialized"}}}' + echo "curl: (22) The requested URL returned error: 503" >&2; exit 22 ;; R) echo "curl: (7) Failed to connect" >&2; exit 7 ;; esac STUB @@ -60,10 +75,10 @@ run_case() { fi } -run_case "unhealthy then healthy x3 passes" 0 "U H H H" -run_case "old revision healthy once, then unhealthy, then healthy x3 passes" 0 "H U U H H H" -run_case "unhealthy in the middle of a streak resets it" 1 "H H U H H U" "auth store not initialized" -run_case "healthy once then unhealthy forever fails with last body" 1 "H U" "auth store not initialized" +run_case "not ready then ready x3 passes" 0 "U H H H" +run_case "old revision ready once, then not ready, then ready x3 passes" 0 "H U U H H H" +run_case "not ready in the middle of a streak resets it" 1 "H H U H H U" "auth store not initialized" +run_case "ready once then not ready forever fails with the degraded body" 1 "H U" "auth store not initialized" run_case "connection refused forever fails with curl error" 1 "R" "Failed to connect" run_case "curl error mid-streak resets it (H R H H U, streak 3, must not pass)" 1 "H R H H U" "Failed to connect" @@ -90,8 +105,8 @@ run_env_case() { fi } -run_env_case "defaults: H H U H H H passes only at attempt 6 (streak 3)" 0 "H H U H H H" "Health check 6: healthy (3/3)" -run_env_case "defaults: two healthy then unhealthy forever exhausts 30 attempts" 1 "H H U" "after 30 attempts" +run_env_case "defaults: H H U H H H passes only at attempt 6 (streak 3)" 0 "H H U H H H" "Readiness check 6: ready (3/3)" +run_env_case "defaults: two ready then not ready forever exhausts 30 attempts" 1 "H H U" "after 30 attempts" run_env_case "streak 0 rejected" 2 "H" "HEALTH_REQUIRED_STREAK must be a positive integer" HEALTH_REQUIRED_STREAK=0 run_env_case "streak abc rejected" 2 "H" "HEALTH_REQUIRED_STREAK must be a positive integer" HEALTH_REQUIRED_STREAK=abc run_env_case "streak empty rejected" 2 "H" "HEALTH_REQUIRED_STREAK must be a positive integer" HEALTH_REQUIRED_STREAK= @@ -100,28 +115,41 @@ run_env_case "attempts abc rejected" 2 "H" "HEALTH_MAX_ATTEMPTS must be a positi run_env_case "attempts empty rejected" 2 "H" "HEALTH_MAX_ATTEMPTS must be a positive integer" HEALTH_MAX_ATTEMPTS= run_env_case "attempts below streak rejected" 2 "H" "could never pass" HEALTH_MAX_ATTEMPTS=2 HEALTH_REQUIRED_STREAK=3 -# Pre-fix logic, verbatim from deploy-azure.yml (URL var as the workflow set it). +# The pre-#488 loop from deploy-azure.yml, faithful to the original semantics: +# warm up on /api/ to trigger lazy DB init, gate on the /health BODY, and pass +# on the FIRST healthy response. D is the pre-fix /health contract: HTTP 200 +# with a degraded body. This is the logic that admitted a cold replica: the +# warm-up initializes whichever replica the load balancer picked, and the first +# healthy answer ends the loop before a cold replica's degraded answer is seen. old_inline() { local URL=https://app.example RESPONSE i for i in {1..3}; do + curl -s -o /dev/null --max-time 60 "${URL}/api/auth/check-admin" || true if RESPONSE=$(curl -fsS --max-time 10 "$URL/health") && jq -e '.status == "healthy"' > /dev/null <<< "$RESPONSE"; then echo "Health check $i: passed" - else - echo "Health check $i: failed: ${RESPONSE:-no response}" - return 1 + return 0 fi + echo "Health check $i: not healthy yet: ${RESPONSE:-no response}" sleep 0 done + return 1 } : > "$WORK/state" -if PATH="$WORK/bin:$PATH" STATE="$WORK/state" SCENARIO="H U H H" old_inline >/dev/null 2>&1; then - echo "FAIL: pre-fix inline loop unexpectedly passed the rollout-race scenario" - ((fail++)) || true -else - echo "PASS: pre-fix inline loop fails the rollout-race scenario (H U H H), the bug being fixed" +# H D H H models the rollout race: the warmed replica answers the first probe, +# a still-cold replica would answer the second. Passing on the first H is +# exactly the #488 admission bug, so the pre-fix loop MUST pass here. +if PATH="$WORK/bin:$PATH" STATE="$WORK/state" SCENARIO="H D H H" old_inline >/dev/null 2>&1; then + echo "PASS: pre-fix inline loop passes the rollout-race scenario (H D H H) on the first healthy response - the admission bug being fixed" ((pass++)) || true +else + echo "FAIL: pre-fix inline loop unexpectedly failed the rollout-race scenario" + ((fail++)) || true fi +# The current gate must reject that same premature pass: a not-ready replica +# resets the streak, so the gate only passes once answers are CONSISTENTLY ready. +run_case "rollout race H U H H H passes under the readiness gate" 0 "H U H H H" "Readiness check 5: ready (3/3)" + echo "passed=$pass failed=$fail" [[ "$fail" -eq 0 ]] diff --git a/scripts/wait-for-healthy.sh b/scripts/wait-for-healthy.sh index 533fb676a..8229f22d6 100755 --- a/scripts/wait-for-healthy.sh +++ b/scripts/wait-for-healthy.sh @@ -1,22 +1,27 @@ #!/usr/bin/env bash # wait-for-healthy.sh URL # -# Smoke gate for a just-deployed service. Polls URL/health until it reports -# status "healthy" HEALTH_REQUIRED_STREAK times in a row. An unhealthy or -# failed response inside the polling budget resets the streak instead of -# failing, because during a rollout a probe can hit a replica that is still -# initializing. If the budget (HEALTH_MAX_ATTEMPTS) runs out before the streak -# is reached, exits 1 and prints the last response (or curl error). +# Smoke gate for a just-deployed service. Polls URL/ready until curl -f +# accepts the response (any status below 400) HEALTH_REQUIRED_STREAK times in +# a row. A not-ready or failed response inside the polling budget resets the +# streak instead of failing, because during a +# rollout a probe can hit a replica that is still initializing. If the budget +# (HEALTH_MAX_ATTEMPTS) runs out before the streak is reached, exits 1 and +# prints the last response (or curl error). # -# Each round first sends one request to HEALTH_WARMUP_PATH: /health reports but -# never triggers the lazy DB/auth-store init on the HTTP server; any /api/ -# request does. +# /ready is the traffic-admission endpoint: it answers 503 until the replica it +# answers for has completed initialization, so the gate reflects the same +# contract the load balancer uses (#488). /health answers 200 even when +# degraded and is a liveness signal only. No warm-up request is needed: the +# server initializes its database in the background at startup. +# +# Requires curl 7.76+ for --fail-with-body, so a 503 still prints the JSON +# check detail that explains which dependency is not ready. # # Env (defaults suit the deploy workflows; tests shrink them): -# HEALTH_REQUIRED_STREAK consecutive healthy responses needed (3) +# HEALTH_REQUIRED_STREAK consecutive ready responses needed (3) # HEALTH_MAX_ATTEMPTS polling rounds before failing (30) # HEALTH_INTERVAL_SECONDS sleep between rounds (5) -# HEALTH_WARMUP_PATH path hit before each probe (/api/auth/check-admin) set -euo pipefail @@ -24,7 +29,6 @@ URL="${1:?usage: wait-for-healthy.sh URL}" REQUIRED_STREAK="${HEALTH_REQUIRED_STREAK-3}" MAX_ATTEMPTS="${HEALTH_MAX_ATTEMPTS-30}" INTERVAL="${HEALTH_INTERVAL_SECONDS:-5}" -WARMUP_PATH="${HEALTH_WARMUP_PATH:-/api/auth/check-admin}" # Fail loudly on bad limits: 0 or non-numeric would silently behave like 1, and # attempts < streak can never pass. A set-but-empty value is rejected too. @@ -43,29 +47,23 @@ streak=0 last="no response" for ((attempt = 1; attempt <= MAX_ATTEMPTS; attempt++)); do - curl -s -o /dev/null --max-time 30 "${URL}${WARMUP_PATH}" || true - - # /health returns HTTP 200 even when degraded; the body is the verdict. - if body=$(curl -fsS --max-time 10 "${URL}/health" 2>&1); then - last="$body" - if jq -e '.status == "healthy"' >/dev/null 2>&1 <<<"$body"; then - streak=$((streak + 1)) - echo "Health check ${attempt}: healthy (${streak}/${REQUIRED_STREAK})" - if ((streak >= REQUIRED_STREAK)); then - echo "All smoke tests passed!" - exit 0 - fi - else - streak=0 - echo "Health check ${attempt}: not healthy yet, streak reset: ${body}" + # /ready answers 503 until the replica has completed initialization; + # --fail-with-body keeps the JSON check detail so a failure says WHICH + # dependency is not ready. + if body=$(curl -fsS --fail-with-body --max-time 10 "${URL}/ready" 2>&1); then + streak=$((streak + 1)) + echo "Readiness check ${attempt}: ready (${streak}/${REQUIRED_STREAK})" + if ((streak >= REQUIRED_STREAK)); then + echo "Service is ready." + exit 0 fi else last="${body:-no response}" streak=0 - echo "Health check ${attempt}: request failed, streak reset: ${last}" + echo "Readiness check ${attempt}: not ready yet, streak reset: ${last}" fi sleep "$INTERVAL" done -echo "::error::Service not healthy ${REQUIRED_STREAK}x in a row after ${MAX_ATTEMPTS} attempts. Last response: ${last}" +echo "::error::Service not ready ${REQUIRED_STREAK}x in a row after ${MAX_ATTEMPTS} attempts. Last response: ${last}" exit 1 From 57aaacea2944227773c8f7db00914474cc976450 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 14:59:51 +0200 Subject: [PATCH 3/6] fix(infra): point Azure traffic-admission probes at /ready Readiness and origin health probes decide whether a replica receives traffic, so they must use the readiness contract, not the 200-always liveness endpoint (#488). - AKS and Container Apps readiness probes move from /health to /ready; liveness and startup probes stay on /health - the Front Door origin group health probe moves to /ready so the CDN layer stops re-admitting cold origins the platform probe excluded; Front Door treats all origins as healthy when every probe fails, so a transient database blip cannot hard-outage the site - nginx proxies /ready to the backend so it no longer falls through to the SPA catch-all, which would answer 200 with index.html --- scripts/nginx.conf | 8 ++++++++ terraform/modules/compute/azure/aks/main.tf | 5 ++++- terraform/modules/compute/azure/container-apps/main.tf | 7 +++++-- terraform/modules/frontend/azure/main.tf | 6 +++++- 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/scripts/nginx.conf b/scripts/nginx.conf index dff3f2b2b..29884de85 100644 --- a/scripts/nginx.conf +++ b/scripts/nginx.conf @@ -32,6 +32,14 @@ server { proxy_set_header Host $host; } + # Readiness endpoint - proxy to backend (#488); without this, /ready would + # fall through to the SPA catch-all and answer 200 with index.html. + location /ready { + proxy_pass http://app:8080; + proxy_http_version 1.1; + proxy_set_header Host $host; + } + # Scheduled tasks endpoint - proxy to backend location /api/scheduled { proxy_pass http://app:8080; diff --git a/terraform/modules/compute/azure/aks/main.tf b/terraform/modules/compute/azure/aks/main.tf index eb743c828..96ddf4bae 100644 --- a/terraform/modules/compute/azure/aks/main.tf +++ b/terraform/modules/compute/azure/aks/main.tf @@ -449,9 +449,12 @@ resource "kubernetes_deployment" "app" { failure_threshold = 3 } + # /ready answers 503 until this replica has completed database + # initialization, so a cold replica stays out of the Service + # endpoints (#488). readiness_probe { http_get { - path = "/health" + path = "/ready" port = 8080 } initial_delay_seconds = 10 diff --git a/terraform/modules/compute/azure/container-apps/main.tf b/terraform/modules/compute/azure/container-apps/main.tf index 13fcb65e3..a48170d5d 100644 --- a/terraform/modules/compute/azure/container-apps/main.tf +++ b/terraform/modules/compute/azure/container-apps/main.tf @@ -161,11 +161,14 @@ resource "azurerm_container_app" "main" { failure_count_threshold = 3 } - # Readiness probe + # Readiness probe. /ready answers 503 until this replica has completed + # database initialization, so a cold replica is never admitted to traffic + # (#488). /health would answer 200 with "degraded" in the body, which the + # probe cannot see. readiness_probe { transport = "HTTP" port = 8080 - path = "/health" + path = "/ready" interval_seconds = 10 timeout = 3 diff --git a/terraform/modules/frontend/azure/main.tf b/terraform/modules/frontend/azure/main.tf index 739eeea62..fea6bf39e 100644 --- a/terraform/modules/frontend/azure/main.tf +++ b/terraform/modules/frontend/azure/main.tf @@ -188,8 +188,12 @@ resource "azurerm_cdn_frontdoor_origin_group" "container_apps" { successful_samples_required = 3 } + # /ready answers 503 until the origin replica has completed initialization, + # so Front Door stops routing to cold origins (#488). When every origin + # fails its probe Front Door treats all as healthy, so a transient database + # blip cannot hard-outage the site through this probe. health_probe { - path = "/health" + path = "/ready" request_type = "GET" protocol = "Https" interval_in_seconds = 100 From 7e3331ad4519e303449b3107948b11f1cbe38cad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 15:17:46 +0200 Subject: [PATCH 4/6] style(server): use US spelling "canceled" in readiness comments The misspell linter enforces the canonical US spelling (migration 000089); three new comments used "cancelled". --- internal/server/http.go | 2 +- internal/server/readiness_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/server/http.go b/internal/server/http.go index 5e8bf3e16..79a6b2efe 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -120,7 +120,7 @@ func (app *Application) startBackgroundInit(ctx context.Context) { // initializeUntilReady calls init until it succeeds or ctx is done, so a // transient database outage at boot does not permanently strand the replica out // of rotation. It always makes at least one attempt before consulting ctx, so a -// context that is already cancelled cannot skip initialization entirely. +// context that is already canceled cannot skip initialization entirely. // // Each attempt runs under backgroundInitAttemptTimeout: ensureDB bounds its // connect and migration phases, but secret resolution has no timeout of its diff --git a/internal/server/readiness_test.go b/internal/server/readiness_test.go index 48dd1a3e2..478e7f14e 100644 --- a/internal/server/readiness_test.go +++ b/internal/server/readiness_test.go @@ -252,7 +252,7 @@ func TestInitializeUntilReadyRetriesAfterTransientFailure(t *testing.T) { // TestInitializeUntilReadyStopsOnCancelledContext pins that shutdown stops the // retry loop instead of leaving a goroutine connecting to a database that is -// going away. One attempt is always made, so an already-cancelled context still +// going away. One attempt is always made, so an already-canceled context still // tries once. func TestInitializeUntilReadyStopsOnCancelledContext(t *testing.T) { app := &Application{Version: "test", initRetryDelay: time.Millisecond} @@ -277,7 +277,7 @@ func TestInitializeUntilReadyStopsOnCancelledContext(t *testing.T) { select { case <-done: case <-time.After(5 * time.Second): - t.Fatal("initializeUntilReady did not return after the context was cancelled") + t.Fatal("initializeUntilReady did not return after the context was canceled") } mu.Lock() From 54f8fc3c7f4c45a37fd1c06286c87f33b77a3463 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 16:41:17 +0200 Subject: [PATCH 5/6] fix(server): close DB pool when admin-password resolution fails CodeRabbit review on #646: ensureDB stored dbConn in app.DB before resolveAdminPassword, and that failure path returned without closing or clearing it. The new background retry loop would leak one pool per 5-second attempt while a secret store stays unreachable. Close and clear, matching the reinitializeAfterConnect failure path below it. Not covered by a new unit test: reaching this branch requires a live PostgreSQL connection (NewConnection fails first otherwise), which the package's hermetic tests cannot fake; the fix mirrors the adjacent, already-reviewed cleanup path. --- internal/server/app.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/server/app.go b/internal/server/app.go index a881566f8..9bf525db2 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -648,6 +648,11 @@ func (app *Application) ensureDB(ctx context.Context) error { adminEmail := os.Getenv("ADMIN_EMAIL") adminPassword, err := app.resolveAdminPassword(ctx) if err != nil { + // Same cleanup as the reinitializeAfterConnect failure path below: + // the retry loop (and repeated lazy attempts) would otherwise leak + // a pool per attempt while secret resolution keeps failing. + dbConn.Close() + app.DB = nil return err // secret-resolution failure is still fatal — env/config, not a migration runtime error } From 574caf46b90191e83fda78c34f0bac97d460cfa5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 8 Oct 2026 16:21:05 +0200 Subject: [PATCH 6/6] fix(server): make wait-for-healthy work with real curl; keep failed migrations ready - wait-for-healthy.sh: drop -f; curl rejects -f together with --fail-with-body (exit 2), so the Azure readiness gate could never pass. The offline stub now rejects that combination and a real-curl check runs against throwaway localhost 200/503 servers. - /ready: only a pending migration run makes the replica unready. A failed run stays "degraded" in the body (and in /health) but answers 200, since ensureDB never retries migrations and 503 would pull every replica after one transient error. --- internal/api/openapi.yaml | 7 ++-- internal/server/app.go | 6 ++- internal/server/health.go | 25 +++++++++++- internal/server/readiness_test.go | 8 ++-- scripts/test-wait-for-healthy.sh | 66 +++++++++++++++++++++++++++++++ scripts/wait-for-healthy.sh | 13 +++--- 6 files changed, 108 insertions(+), 17 deletions(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index a34f47bf1..7cef4b596 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2122,8 +2122,9 @@ paths: description: >- Traffic-admission contract: answers 200 with the same payload as /health once this replica has completed initialization, 503 while any - check is pending or failed. /health is the liveness counterpart and - always answers 200. + store check is pending or failed or migrations have not run yet. A + failed migration run is reported in the body but still answers 200. + /health is the liveness counterpart and always answers 200. security: [] responses: '200': @@ -2133,7 +2134,7 @@ paths: schema: $ref: '#/components/schemas/HealthResponse' '503': - description: Replica is still initializing or a check failed + description: Replica is still initializing or a store check failed content: application/json: schema: diff --git a/internal/server/app.go b/internal/server/app.go index 9bf525db2..130bc3877 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -639,8 +639,10 @@ func (app *Application) ensureDB(ctx context.Context) error { log.Println("PostgreSQL connection established successfully") // Run migrations if AutoMigrate is enabled. Failures are non-fatal: - // we log, surface via /health's migrations check, and proceed. The app - // stays up; handlers that need the missing schema error at query time. + // we log, surface via the migrations check in /health and /ready, and + // proceed. The app stays up and ready (/ready only waits for the first + // attempt to finish); handlers that need the missing schema error at + // query time. // See specs/migration-resilience.md (or the plan) for the rationale. if app.dbConfig.AutoMigrate { log.Println("Running database migrations...") diff --git a/internal/server/health.go b/internal/server/health.go index 94733e55a..b4ccfa09c 100644 --- a/internal/server/health.go +++ b/internal/server/health.go @@ -47,20 +47,41 @@ func (app *Application) handleHealthCheck(w http.ResponseWriter, r *http.Request // required initialization answers 503, so the load balancer stops sending it // requests. /health only reported "degraded" in the body while still answering // 200, which admitted cold replicas to traffic and broke the deployment smoke -// gate (#488). Non-degraded is required initialization done; the body carries +// gate (#488). Ready means required initialization is done; the body carries // the same checks as /health so an operator sees which one is still pending. +// A failed migration run does not make the replica unready (see ready). func (app *Application) handleReadinessCheck(w http.ResponseWriter, r *http.Request) { ctx, cancel := context.WithTimeout(r.Context(), 5*time.Second) defer cancel() health := app.collectHealth(ctx) status := http.StatusOK - if health.Status != "healthy" { + if !health.ready() { status = http.StatusServiceUnavailable } writeHealthResponse(w, app.appConfig.CORSAllowedOrigin, status, health) } +// ready reports whether the replica may take traffic: the stores are connected +// and the first migration attempt has finished. A failed migration run stays +// "degraded" in the body but is still ready. ensureDB never retries migrations, +// so treating it as unready would pull every replica out of rotation after one +// transient error (e.g. a lock timeout) with nothing to bring them back. +func (h HealthStatus) ready() bool { + for name, check := range h.Checks { + if name == "migrations" { + if check.Status == "pending" { + return false + } + continue + } + if check.Status != "healthy" { + return false + } + } + return true +} + // collectHealth runs every readiness/health check and folds them into one // overall status. Shared by /health and /ready so the two endpoints can never // drift on what "initialized" means. diff --git a/internal/server/readiness_test.go b/internal/server/readiness_test.go index 478e7f14e..e0d4b1e8b 100644 --- a/internal/server/readiness_test.go +++ b/internal/server/readiness_test.go @@ -89,9 +89,9 @@ func TestReadinessPermanentInitializationFailure(t *testing.T) { } } -// TestReadinessFailedMigrations pins the migration facet: a replica whose -// migrations permanently failed must not take traffic even though its stores -// connected. Pre-fix only the body said "degraded"; readiness now has to agree. +// TestReadinessFailedMigrations pins the migration facet: a failed migration run +// is reported as "degraded" but the replica stays ready, because ensureDB never +// retries migrations and 503 would pull every replica after one transient error. func TestReadinessFailedMigrations(t *testing.T) { app := &Application{ Version: "test", @@ -104,7 +104,7 @@ func TestReadinessFailedMigrations(t *testing.T) { req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) res := httptest.NewRecorder() app.handleReadinessCheck(res, req) - testutil.AssertEqual(t, http.StatusServiceUnavailable, res.Code) + testutil.AssertEqual(t, http.StatusOK, res.Code) var health HealthStatus testutil.AssertNoError(t, json.Unmarshal(res.Body.Bytes(), &health)) diff --git a/scripts/test-wait-for-healthy.sh b/scripts/test-wait-for-healthy.sh index 9969f47c4..183869aaf 100755 --- a/scripts/test-wait-for-healthy.sh +++ b/scripts/test-wait-for-healthy.sh @@ -26,10 +26,21 @@ mkdir -p "$WORK/bin" cat > "$WORK/bin/curl" <<'STUB' #!/usr/bin/env bash fwb=0 +fail=0 for a in "$@"; do url="$a" [ "$a" = "--fail-with-body" ] && fwb=1 + # -f/--fail, alone or inside a short cluster such as -fsS. + case "$a" in + --*) [ "$a" = "--fail" ] && fail=1 ;; + -*f*) fail=1 ;; + esac done +# Real curl (7.76+) rejects -f/--fail together with --fail-with-body. +if [ "$fwb" = 1 ] && [ "$fail" = 1 ]; then + echo "curl: option --fail-with-body: is badly used here" >&2 + exit 2 +fi # The pre-fix loop's warm-up request to /api/ succeeded silently and consumed no # scenario token; only the gate request did. case "$url" in @@ -151,5 +162,60 @@ fi # resets the streak, so the gate only passes once answers are CONSISTENTLY ready. run_case "rollout race H U H H H passes under the readiness gate" 0 "H U H H H" "Readiness check 5: ready (3/3)" +# Real curl against a throwaway localhost server: the stub above cannot catch +# flag combinations that the installed curl rejects. Skipped when python3 is +# missing; curl older than 7.76 has no --fail-with-body and would fail here. +if command -v python3 > /dev/null; then + # Two servers: one always answers 200, the other 503 with a degraded body. + python3 - > "$WORK/port" <<'PY' & +import http.server, sys, threading + +class Handler(http.server.BaseHTTPRequestHandler): + def do_GET(self): + code = 200 if self.server.mode == "ok" else 503 + body = b'{"status":"degraded","checks":{"database":{"status":"unhealthy"}}}' if code == 503 else b'{"status":"healthy","checks":{}}' + self.send_response(code) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + def log_message(self, *args): + pass + +servers = [] +for mode in ("ok", "unready"): + s = http.server.HTTPServer(("127.0.0.1", 0), Handler) + s.mode = mode + servers.append(s) +print(servers[0].server_port, servers[1].server_port, flush=True) +for s in servers[1:]: + threading.Thread(target=s.serve_forever, daemon=True).start() +servers[0].serve_forever() +PY + SRV_PID=$! + trap 'kill "$SRV_PID" 2>/dev/null || true; wait "$SRV_PID" 2>/dev/null || true; rm -rf "$WORK"' EXIT + for _ in {1..50}; do [[ -s "$WORK/port" ]] && break; sleep 0.1; done + read -r OK_PORT UNREADY_PORT < "$WORK/port" + + out=$(HEALTH_INTERVAL_SECONDS=0 HEALTH_MAX_ATTEMPTS=3 HEALTH_REQUIRED_STREAK=3 \ + "$SUT" "http://127.0.0.1:${OK_PORT}" 2>&1) && real_ok=0 || real_ok=$? + if [[ "$real_ok" -eq 0 && "$out" == *"Service is ready."* ]]; then + echo "PASS: real curl accepts a 200 /ready" + ((pass++)) || true + else + echo "FAIL: real curl against a 200 /ready (exit $real_ok)"; echo "$out" + ((fail++)) || true + fi + + out=$(HEALTH_INTERVAL_SECONDS=0 HEALTH_MAX_ATTEMPTS=2 HEALTH_REQUIRED_STREAK=2 \ + "$SUT" "http://127.0.0.1:${UNREADY_PORT}" 2>&1) && real_503=0 || real_503=$? + if [[ "$real_503" -eq 1 && "$out" == *'"status":"degraded"'* ]]; then + echo "PASS: real curl rejects a 503 /ready and prints its body" + ((pass++)) || true + else + echo "FAIL: real curl against a 503 /ready (exit $real_503)"; echo "$out" + ((fail++)) || true + fi +fi + echo "passed=$pass failed=$fail" [[ "$fail" -eq 0 ]] diff --git a/scripts/wait-for-healthy.sh b/scripts/wait-for-healthy.sh index 8229f22d6..05b8fbbe3 100755 --- a/scripts/wait-for-healthy.sh +++ b/scripts/wait-for-healthy.sh @@ -1,10 +1,10 @@ #!/usr/bin/env bash # wait-for-healthy.sh URL # -# Smoke gate for a just-deployed service. Polls URL/ready until curl -f -# accepts the response (any status below 400) HEALTH_REQUIRED_STREAK times in -# a row. A not-ready or failed response inside the polling budget resets the -# streak instead of failing, because during a +# Smoke gate for a just-deployed service. Polls URL/ready until curl +# --fail-with-body accepts the response (any status below 400) +# HEALTH_REQUIRED_STREAK times in a row. A not-ready or failed response inside +# the polling budget resets the streak instead of failing, because during a # rollout a probe can hit a replica that is still initializing. If the budget # (HEALTH_MAX_ATTEMPTS) runs out before the streak is reached, exits 1 and # prints the last response (or curl error). @@ -16,7 +16,8 @@ # server initializes its database in the background at startup. # # Requires curl 7.76+ for --fail-with-body, so a 503 still prints the JSON -# check detail that explains which dependency is not ready. +# check detail that explains which dependency is not ready. Do not add -f: +# curl rejects -f/--fail together with --fail-with-body. # # Env (defaults suit the deploy workflows; tests shrink them): # HEALTH_REQUIRED_STREAK consecutive ready responses needed (3) @@ -50,7 +51,7 @@ for ((attempt = 1; attempt <= MAX_ATTEMPTS; attempt++)); do # /ready answers 503 until the replica has completed initialization; # --fail-with-body keeps the JSON check detail so a failure says WHICH # dependency is not ready. - if body=$(curl -fsS --fail-with-body --max-time 10 "${URL}/ready" 2>&1); then + if body=$(curl -sS --fail-with-body --max-time 10 "${URL}/ready" 2>&1); then streak=$((streak + 1)) echo "Readiness check ${attempt}: ready (${streak}/${REQUIRED_STREAK})" if ((streak >= REQUIRED_STREAK)); then