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/internal/api/openapi.yaml b/internal/api/openapi.yaml index 7ae8d2691..7cef4b596 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2114,6 +2114,32 @@ 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 + 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': + description: Replica is ready to serve traffic + content: + application/json: + schema: + $ref: '#/components/schemas/HealthResponse' + '503': + description: Replica is still initializing or a store 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..130bc3877 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. @@ -634,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...") @@ -643,6 +650,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 } diff --git a/internal/server/health.go b/internal/server/health.go index 35aba13dc..b4ccfa09c 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,65 @@ 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). 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.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. +func (app *Application) collectHealth(ctx context.Context) HealthStatus { health := HealthStatus{ Status: "healthy", Version: app.Version, @@ -35,15 +93,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 +126,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 +169,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 +189,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 +227,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 +239,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..79a6b2efe 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 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 +// 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..e0d4b1e8b --- /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 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", + 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.StatusOK, 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-canceled 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 canceled") + } + + 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) +} 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/scripts/test-wait-for-healthy.sh b/scripts/test-wait-for-healthy.sh index 9167a49bf..183869aaf 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,34 @@ 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 +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 */api/*) exit 0 ;; esac @@ -30,8 +52,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 +86,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 +116,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,27 +126,95 @@ 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)" + +# 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" diff --git a/scripts/wait-for-healthy.sh b/scripts/wait-for-healthy.sh index 533fb676a..05b8fbbe3 100755 --- a/scripts/wait-for-healthy.sh +++ b/scripts/wait-for-healthy.sh @@ -1,22 +1,28 @@ #!/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 +# --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). # -# 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. 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 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 +30,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 +48,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 -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 + 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 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