fix(aws): gate Fargate ALB traffic admission on /ready

[IAmJSD]

Root cause

PR #646 (#488) added /ready, which answers 503 until the process has finished initialization, and moved every Azure traffic-admission check to it. AWS Fargate was left on the old contract: the ALB target group health check still used /health. /health answers HTTP 200 even while a task is still initializing ("degraded"), so the ALB marked cold tasks healthy and sent production traffic to them during rollouts and scale-out.

The deploy smoke gate in deploy-aws-fargate.yml also still used the pre-#488 pattern. It sent a warm-up request to /api/auth/check-admin, then parsed the /health body. The warm-up only initializes whichever task the ALB happens to pick.

Fix

• ALB target group (terraform/modules/compute/aws/fargate): health_check_path now defaults to /ready, and terraform/environments/aws/compute.tf passes /ready. The matcher stays "200", so a 503 counts as not ready.
• ECS service grace period: ECS replaces a task once its target group reports it unhealthy. I added health_check_grace_period_seconds (default 300, validated as a whole number ≥ 0) so a task still connecting to the database or running migrations isn't killed while it initializes. During the grace period the ALB still keeps the unready task out of rotation.
• Short database blips: the target group's unhealthy_threshold default goes from 3 to 6. /ready also answers 503 while the database is unreachable, and ECS replaces any target the ALB marks unhealthy. At the 30s interval, ECS now starts replacing a task after about 180s of continuous 503s, not 90s, so a routine RDS Multi-AZ failover (typically 60–120s) doesn't cycle every task. Recovery after the database returns is healthy_threshold 2 × 30s, which is unchanged.
• ECS container healthCheck stays on /health. It is liveness (failing it replaces the task), so it must not depend on the database.
• Smoke gate script (scripts/wait-for-healthy.sh): only a 2xx response counts as ready. A 3xx is not ready, and the log line names the redirect target (%{redirect_url}). Redirects are deliberately not followed. For other non-2xx responses, the status and body are printed. Before this change, any status below 400 passed, so a 301 counted as ready.
• Gate URL: there is a new fargate_app_url output in terraform/environments/aws/outputs.tf. When the module's HTTPS listener exists (api_https_url != null) it is https://api-<env>.<subdomain_zone_name>, otherwise http://<ALB DNS>. The api-<env> alias (dns_records.tf) points at the same ALB, and the listener's wildcard certificate covers it. None of the existing outputs gives a URL that works here: fargate_alb_dns_name/api_url get a 301 when HTTPS is on, and api_https_url (https://<ALB DNS>) fails TLS verification against that certificate. The deploy job exports it as app_url, and test-deployment gates on it.
• Deploy smoke gate: the inline loop is replaced with scripts/wait-for-healthy.sh on /ready, mirroring deploy-azure.yml. There are two steps: a fast-fail readiness step (streak 1, 30 × 10s, which covers the 300s grace period) and the rollout-race smoke step (streak 3 within 30 × 5s). The warm-up request is gone.

Check	Before	After
ALB target group health_check.path (traffic admission)	/health (200 while degraded)	/ready (503 until initialized)
ALB matcher / interval / healthy threshold	200 / 30s / 2	unchanged
ALB unhealthy_threshold	3 (about 90s to replacement)	6 (about 180s to replacement)
ECS service health_check_grace_period_seconds	unset (0)	default 300
ECS container healthCheck (liveness)	curl -f localhost:8080/health, startPeriod 60	unchanged
Gate URL	http://<ALB DNS> (301 when HTTPS is on)	fargate_app_url (https alias when HTTPS is on)
deploy-aws-fargate.yml test gate	warm-up, then /health + jq, 10 × 10s	wait-for-healthy.sh on /ready, 30 × 10s
deploy-aws-fargate.yml smoke step	3 × /health body parse	wait-for-healthy.sh, 3 consecutive ready within 30 attempts
wait-for-healthy.sh verdict	any status < 400	2xx only; 3xx logged with its target

CloudFormation parity: cloudformation/stacks/CUDly defines no ECS, Fargate or target group resources.

Other callers of wait-for-healthy.sh: deploy-azure.yml (https Container App URL) and the GCP caller in #1024 (https Cloud Run service_url). Both hit the app directly over https and get a 2xx when ready, so 2xx-only changes nothing for them.

Tests

• terraform/modules/compute/aws/fargate_readiness_test.go has six static Go tests over the TF, workflow and output files:

  • the ALB check uses /ready with matcher 200;
  • the container check stays on /health;
  • the grace period is at least 180s;
  • unhealthy_threshold × interval is at least 150s, the threshold is at most 10, and the env doesn't override them lower;
  • the gate uses wait-for-healthy.sh on app_url with no warm-up, no /health, no http:// prefixing, and a fast-gate budget at least as long as the grace period;
  • fargate_app_url switches to the https alias exactly when the HTTPS listener exists.

• scripts/test-wait-for-healthy.sh: all 19 existing cases still pass, plus 4 new ones (23/23):

  • a 301 forever fails and names the target;
  • a 301 mid-streak resets the streak;
  • a 503 keeps its status and body;
  • a real curl against a local server answering 301 on every path (the reviewer's repro) now fails.

  Against the old script, those 4 new cases fail and the 301 server case passes vacuously (exit 0).

• Mutation probes: 25 of 25 killed. They cover the target group, variables, env override, workflow URL/budget/warm-up//health, the output's https switch, and in the script: accepting 3xx, accepting any status, -L, dropping the redirect message, and dropping the body.

Verification

Static and offline checks only. Nothing was planned, applied or deployed against any account.

• terraform fmt -check -recursive + terraform init -backend=false (temporary data dir, removed afterwards) + validate on terraform/modules/compute/aws/fargate and terraform/environments/aws: clean and valid. Lock files unchanged.
• go test ./terraform/modules/compute/aws (with AWS_CONFIG_FILE/AWS_SHARED_CREDENTIALS_FILE=/dev/null), go vet, golangci-lint v2.10.1: ok, 0 issues. No non-test Go files changed.
• shellcheck on both scripts: clean. actionlint (all workflows): clean. zizmor 1.29.0 --offline --persona=pedantic --min-severity=medium .github/workflows/ (as in CI): no findings.
• git-secrets scan of the changed files in a fresh git init temp repo with an isolated HOME: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.

Operators: re-apply the AWS Fargate environment. This is an in-place update of aws_lb_target_group.main and aws_ecs_service.main plus the new output. For HTTPS deployments the gate now resolves api-<env>.<subdomain_zone_name>, so the subdomain zone must be delegated. If it isn't, the gate fails loudly with a DNS error rather than passing.

Behaviour change for the owner to acknowledge

On ECS, failing the ALB health check also triggers task replacement, and /ready answers 503 when the database is unreachable at runtime. The chosen mitigation is unhealthy_threshold 6: a database outage still cycles tasks, but only after about 180s of continuous 503s (about 90s before this change). The trade-off is that a task whose own database connection breaks stays in rotation up to about 180s. A dead process is still caught sooner by the container liveness check, and a stopped task is deregistered right away. When every target is unhealthy the ALB fails open. Flagged for owner acknowledgement.

Out of scope

• Lambda deploy/rollback health checks: Lambda keeps lazy init and has no /ready route.
• terraform/modules/deployment-checks and the GCP uptime monitor (fix(infra): deployment checks and GCP uptime monitor gate on tautological /health 200 #649), and GCP probes (fix(gcp): exclude uninitialized replicas from GKE and Cloud Run readiness #648).
• The curl .../health hint in quick_start_commands (terraform/environments/aws/outputs.tf) and docs/DEPLOYMENT.md. That output is shared with Lambda, which has no /ready, so it isn't a one-line fix.

Closes #647

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 8 billable files and costs up to $2.00.}

Or wait 38 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: ee4365d7-4c23-45f5-95c2-a40ef95a6db6

📥 Commits

Reviewing files that changed from the base of the PR and between 110a496 and 14e957f.

📒 Files selected for processing: 8

• .github/workflows/deploy-aws-fargate.yml
• scripts/test-wait-for-healthy.sh
• scripts/wait-for-healthy.sh
• terraform/environments/aws/compute.tf
• terraform/environments/aws/outputs.tf
• terraform/modules/compute/aws/fargate/main.tf
• terraform/modules/compute/aws/fargate/variables.tf
• terraform/modules/compute/aws/fargate_readiness_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review of b775945: draft: blocks ready

Scope: .github/workflows/deploy-aws-fargate.yml, terraform/environments/aws/compute.tf, terraform/modules/compute/aws/fargate/{main,variables}.tf, terraform/modules/compute/aws/fargate_readiness_test.go. The PR body is treated as a claim; everything below was checked against code at this SHA.

Findings

1. BLOCKING: the deploy readiness gate passes vacuously when HTTPS is enabled. .github/workflows/deploy-aws-fargate.yml:~295-319 (both new steps) call scripts/wait-for-healthy.sh with APP_URL=http://<alb dns> (the URL is built in the "Get ALB URL" step from fargate_alb_dns_name, terraform/environments/aws/outputs.tf:121). When enable_https is true (terraform/environments/aws/compute.tf:193, any deployment with frontend domains plus a zone), the port 80 listener returns a 301 redirect (terraform/modules/compute/aws/fargate/main.tf:501-513). The script uses curl --fail-with-body without -L (scripts/wait-for-healthy.sh:50), which accepts any status below 400, so a 301 counts as "ready". Reproduced: a local server answering 301 on every path makes HEALTH_REQUIRED_STREAK=3 bash scripts/wait-for-healthy.sh http://127.0.0.1:<port> print "Service is ready." and exit 0. The replaced gate failed closed in that configuration (a 301 has an empty body, so jq -e '.status == "healthy"' failed). Net effect: for HTTPS deployments the PR turns a loud-failing gate into one that can never fail, which is the opposite of the issue's intent. Azure is unaffected because its URL is https. Suggested fix: make the script accept only 2xx (for example -o /dev/null -w '%{http_code}' and a ^2 check, keeping the body for the failure message) and add a 301 case to scripts/test-wait-for-healthy.sh; or have the workflow poll the https URL. A workflow-level static test asserting the gate cannot pass on a redirect would be a good regression test.

2. NON-BLOCKING (owner decision, already disclosed in the PR body): ALB health on /ready also drives ECS task replacement. /ready returns 503 when db.HealthCheck or authService.Ping fails at runtime (internal/server/health.go:96-115, 66-82; main.tf:468-480). With the module defaults (interval 30, unhealthy_threshold 3, variables.tf:162-184), a DB outage or Multi-AZ failover longer than about 90s makes ECS stop and replace every task; the ALB fail-open only helps until the targets are actually stopped. Previously the service kept answering (SPA, /health, non-DB routes) and recovered as soon as the DB did; now recovery needs full task start plus init (up to the 300s grace). I would raise the environment's unhealthy_threshold (or document the choice in docs/DEPLOYMENT.md) before ready, but this is a product call, not a defect in the diff.

3. NON-BLOCKING: the test-deployment step budget (60s sleep plus 10 x 10s = about 160s) is shorter than the grace period/init budget (about 135s DB connect retries plus migrations, per the PR text). Previously the warm-up request could not shorten that either, so this is no regression, but a slow first boot can fail the fast gate where the old loop would equally fail. Consider HEALTH_MAX_ATTEMPTS of 30 for the first gate.

4. NON-BLOCKING: terraform/environments/aws/outputs.tf:250 and docs/DEPLOYMENT.md still tell operators to curl /health. The PR lists this as out of scope; fine.

No other defects survived refutation.

What I attacked

• Source of truth for /ready and /health: internal/server/http.go:31-37 registers both on the mux; handlers in internal/server/health.go:30-57 need no auth and no request-triggered DB init. /ready is 200 or 503, so matcher "200" is right. startBackgroundInit (http.go:116, started at http.go:183) drives init without traffic, so a cold task does become ready on its own; the migrations "pending" state is the only gate and a failed migration stays ready (health.go:66-82), so no never-healthy trap. Probes are cheap (5s context timeout vs the target group's 5s timeout, variables.tf:168).
• Container healthCheck (main.tf:681) stays on /health with startPeriod 60, correct as liveness. ECS service has a load_balancer block (main.tf:714-718) so the grace period is valid, and the deployment circuit breaker (main.tf:733-736) is unchanged.
• Callers: grepped all .tf/.yml/.sh/.md for health_check_path, the fargate module and the new variable. The only module caller is terraform/environments/aws/compute.tf:154; no other env, profile or tfvars sets the variable, and the new variable has a default, so nothing else breaks. CloudFormation has no ECS/ALB resources.
• terraform fmt -check -recursive on the module and environment: clean. terraform init -backend=false plus terraform validate on both: valid (TF_DATA_DIR in scratch, lock files untouched). actionlint on the workflow: clean. Nothing planned or applied.
• Tests: go test -run TestFargate ./terraform/modules/compute/aws passes at the SHA. Fail-before: restoring origin/main's four changed files makes TestFargateALBHealthCheckUsesReadiness, TestFargateServiceHasHealthCheckGracePeriod and TestFargateDeploySmokeGateUsesReadiness fail (the liveness test is a guard, passes both ways). Mutation probes, each reverted at once: matcher "200" -> "200-499" fails the ALB test; grace default 300 -> 0 fails the grace test. Note these static tests cannot catch finding 1; it is a behavior of the shared script.
• Scope/size: 212 additions (under the 400 cap) across 5 files; main.tf was already over 500 lines (1296) before this PR, +13 here.
• Labels: PR carries effort/m, impact/many, priority/p2, severity/medium, triaged, type/bug, urgency/this-quarter; identical to #647. OK.
• CodeRabbit: only "Review skipped: draft pull request"; no threads, nothing to resolve. I did not ping it.

Gate status (origin/main Review gate)

• Independent adversarial review naming the SHA: this comment, but verdict is "draft: blocks ready".
• Actionable findings resolved: no, finding 1 open.
• CI on the SHA: both runs ("CI - Build & Test" run 38965748718, "pre-commit" 38965748768) were queued/pending when I checked, all checks pending, so there is no green CI yet (a fork run may need maintainer approval). CodeRabbit is optional per the gate.
• Local verification on macOS: static only (terraform fmt/validate, actionlint, Go static tests, wait-for-healthy.sh redirect repro). No real-account or live ALB verification exists, and none was done by me.
• Verdict, SHA and evidence recorded on the PR: this comment.

Blocks ready

• Finding 1 (gate accepts a 301 redirect, passes vacuously on HTTPS deployments).
• CI green on the final SHA.
• Owner acknowledgement of finding 2 (outage-time task churn), or a higher unhealthy_threshold.

[IAmJSD]

Thanks for the review. New head: 14e957f8 (one commit added on top of b775945b; no rebase, no force-push). Replies point by point:

1. BLOCKING: the gate passed vacuously on a 301. Fixed in two places.

• Script: scripts/wait-for-healthy.sh now judges the status itself (curl -sS -o <file> -w '%{http_code} %{redirect_url}', no -f). Only 2xx counts as ready. A 3xx is not ready and is logged as HTTP 301 redirect to <target>; redirects are not followed, so point the gate at the URL that serves the app. I didn't add -L. Other non-2xx responses still print the status and the body.
• Script tests: scripts/test-wait-for-healthy.sh is 23/23. The 19 existing cases pass and there are 4 new ones: 301 forever fails and names the target, 301 mid-streak resets the streak, 503 keeps its body, and a real-curl case against a local server answering 301 on every path (your repro). Against the old script those 4 fail, and the 301 server case exits 0 ("Service is ready."), which reproduces the bug.
• Workflow: there is a new output, fargate_app_url. When the module's HTTPS listener exists (api_https_url != null) it is https:// + the api-<env> alias FQDN, otherwise module.compute_fargate[0].api_url (http://<ALB DNS>). deploy exports it as app_url, and test-deployment gates on it with no more http:// prefixing.
• Why a new output: none of the existing ones works. fargate_alb_dns_name/api_url get the 301, and api_https_url (https://<ALB DNS>) fails TLS verification because the listener's certificate is *.<subdomain_zone_name>. The alias (dns_records.tf) exists whenever HTTPS is enabled and points at the same ALB.
• Other callers: deploy-azure.yml and fix(gcp): exclude uninitialized replicas from GKE and Cloud Run readiness #1024's GCP caller both use the platforms' https URLs, so 2xx-only changes nothing for them.

2. Task churn during a database outage. I took the smallest safe change and raised the target group's unhealthy_threshold default from 3 to 6. At the 30s interval, ECS now starts replacing a task after about 180s of continuous 503s, not 90s, which covers a typical Multi-AZ failover (60–120s). The static test TestFargateALBToleratesShortDatabaseBlip pins threshold × interval ≥ 150s and the threshold at most 10, and checks that the env doesn't override them lower. The trade-off is in the PR body, and it is still flagged for owner acknowledgement.

3. First gate budget. Raised from 10 to 30 × 10s (300s), so it covers the 300s grace period. The workflow test pins this: attempts × interval must be at least the grace period.

4. Docs/outputs still say curl /health. Left out of scope. quick_start_commands is shared with Lambda, which has no /ready, so it isn't a one-line fix.

Evidence (static and offline only; nothing planned, applied or deployed):

• Terraform: fmt -check -recursive + init -backend=false (temporary data dir) + validate on the module and the AWS env: clean and valid. Lock files unchanged.
• Go: go test ./terraform/modules/compute/aws (6 Fargate tests), go vet, golangci-lint v2.10.1: ok, 0 issues.
• Mutation probes: 25/25 killed, including accept-3xx, accept-any-status, -L, the alb_url gate, http:// prefixing, the old 10-attempt budget, https://<ALB DNS> in the output, and unhealthy_threshold 3 or an env override to 2.
• Shell and workflows: shellcheck on both scripts: clean. actionlint (all workflows): clean. zizmor 1.29.0 --offline --persona=pedantic --min-severity=medium .github/workflows/: no findings.
• git-secrets: fresh git init temp repo with an isolated HOME: clean. Allowlist self-test 42/0.
• Merge checks: git merge-tree --write-tree of HEAD against origin/main, fork/fix/issue-648 and fork/fix/issue-649: all clean.

🤖 Generated with Claude Code

[cristim]

Independent delta review of 14e957f (previous: b775945): no actionable findings

OLD is an ancestor of NEW (one commit on top, no rebase). I reviewed git diff OLD NEW as new code and re-checked the full PR diff against origin/main (merge-tree: no conflicts). The author's reply is a claim; everything below was run.

Previous findings

1. BLOCKING, gate passed on a 301: RESOLVED.
   • Reproduction: a local server answering 301 on every path, HEALTH_REQUIRED_STREAK=3 HEALTH_MAX_ATTEMPTS=3. OLD script (b775945): "Readiness check 3: ready (3/3) / Service is ready.", exit 0 (wrong). NEW script (scripts/wait-for-healthy.sh:53-90): every attempt "not ready yet ... HTTP 301 redirect to https://app.example.invalid/ready", ends with ::error::Service not ready 3x ..., exit 1.
   • Regression cases: scripts/test-wait-for-healthy.sh with the OLD script swapped in fails 4 cases (301 forever, 301 mid-streak, 503 status/body, and "real curl against a 301 /ready" using a real python server); with the NEW script all 23 pass, including real curl 200, 503 and 301.
   • Script logic: curl has no -f now, -w '%{http_code} %{redirect_url}' is judged in a case; only 2?? counts, 3?? and others reset the streak, a curl transport error resets it and reports stderr. The temp dir is cleaned by trap. No other caller breaks: Azure uses an https URL and 2xx.
   • Root cause also addressed: the workflow now gates on a new fargate_app_url output (terraform/environments/aws/outputs.tf:131-144), exported as app_url (deploy-aws-fargate.yml:227-233, 273-294). It is https on the api-<env> alias exactly when api_https_url != null and the alias exists. I checked the sources: the alias is created when subdomain_zone_name != "" (dns_records.tf:39-40) and enable_https requires a non-empty zone (compute.tf:193), so the alias always exists when HTTPS is on; the listener certificate is *.<zone> (acm.tf:7), which covers api-<env>.<zone> (one label). Otherwise it falls back to http://<ALB DNS>, which forwards (main.tf:501-513). The "Get app URL" step fails loudly if the output is empty or has no scheme.
2. NON-BLOCKING, fast-gate budget vs init time: RESOLVED. Fast gate is now 30 x 10s = 300s (deploy-aws-fargate.yml:~317), at least the 300s grace period; a Go test pins attempts x interval >= grace default.
3. NON-BLOCKING, ECS replacement on DB outage: PARTIALLY RESOLVED, and acceptable. unhealthy_threshold default is now 6 (variables.tf:180-184): about 180s of continuous /ready 503 before the ALB marks a target unhealthy, which covers a typical 60-120s Multi-AZ failover. It still cycles tasks on an outage longer than about 3 minutes; that is the inherent cost of readiness-driven admission on ECS and is disclosed in the PR body. 6 is within the ALB limit of 10 and no other module/env overrides the variable (grepped). No action required.
4. NON-BLOCKING, /health hints in docs/outputs: unchanged, out of scope as stated.

New code attacked

• fargate_readiness_test.go new tests: TestFargateALBToleratesShortDatabaseBlip, the app_url wiring assertions and TestFargateAppURLOutputFollowsListener. Probe 1: unhealthy_threshold default 6 -> 3 fails the blip test. Probe 2: restoring OLD deploy-aws-fargate.yml and outputs.tf fails the smoke-gate test and the app_url output test. Both reverted at once. go test ./terraform/modules/compute/aws passes at the SHA.
• terraform fmt -check -recursive (module + env): clean. terraform init -backend=false + validate on both: valid. actionlint: clean. scripts/test-wait-for-healthy.sh: passed=23 failed=0. Nothing planned or applied.
• Edge: terraform output -raw fargate_app_url is null for non-fargate platforms, but this workflow only deploys fargate and a null output fails the "Get app URL" step loudly rather than passing.
• Labels still match #647. CodeRabbit: only the "draft not reviewed" notice, no threads; not pinged.

Nits (NON-BLOCKING)

• PR additions are now 469 (cap guideline 400); mostly tests and test scaffolding for the script. Not worth splitting at this point.

Gate status (origin/main Review gate)

• Independent review naming the SHA: this comment.
• Actionable findings: none open.
• CI on 14e957f: "CI - Build & Test" (run 38967021268) queued, "pre-commit" (38967021277) in progress at the time I checked, 14 checks pending, 7 passing, 1 skipping; no action_required runs. Not green yet, and I did not wait.
• Local verification (macOS): static plus script reproduction with a local 301/503/200 server and real curl; no live ALB or real-account verification. It is still a draft.

Blocks ready

• Green CI on this SHA, and taking the PR out of draft. Nothing else.