Repository navigation
fix(infra): gate deployment checks and GCP uptime on /health body status - #1023
Conversation
/health is the liveness contract since LeanerCloud#488/LeanerCloud#646: it answers HTTP 200 even when every dependency is down and reports the verdict in the body's top-level "status" ("healthy" / "degraded"). Two verification layers still treated that 200 as meaningful and passed on a fully degraded deployment: - deployment-checks: check "health" asserted only status_code == 200, and the warmup provisioner returned on any non-000/502/503 status. The check now also requires jsondecode(body).status == "healthy", and warmup waits for a 200 whose body reports "healthy" (falling back to 200-only when python3 is unavailable). Gating on the body works unchanged on Lambda, where /ready does not exist. - monitoring/gcp: the uptime check's CONTAINS_STRING "healthy" matcher also matches "unhealthy" and every nested check's {"status":"healthy"}, so it passed on degraded replicas. It now exact-matches "healthy" at JSON path $.status. It stays on /health, not /ready. /health's status-code semantics are unchanged. Tests: terraform tests for both modules (mocked providers, plan only; they fail on the old config) wired into the terraform-validate CI job, plus a Go test pinning the top-level "status" contract on the server mux and the Lambda router. Closes LeanerCloud#649 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 7 billable files and costs up to $1.75. Or wait 38 minutes for your next included review. View limit details
Comment |
Independent review verdict: CLEAN at 7fe7114Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. Read-only, with no cloud credentials and no plan or apply against a real backend. The PR body was treated as a claim. Coordinator: claude-cc-reviewer. Blocking findings: none. Health body vs matchers:
Contract and CI:
Deploy impact (merge held for an explicit owner OK):
Note (out of scope): the Azure monitor still uses a substring What was attacked and how:
Gate:
|
…k file The lock file held a single h1 hash for hashicorp/http 3.5.0 (one platform), so `terraform init -lockfile=readonly` on the linux_amd64 CI runner could not verify the downloaded package and `terraform test` failed in the new "Test /health gate modules" step. Regenerated with `terraform providers lock -platform=linux_amd64 -platform=darwin_arm64 -platform=darwin_amd64`; the provider version is unchanged. Refs LeanerCloud#649 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
CI fix for "Validate Terraform (gcp)", new head Root cause: Fix: I regenerated the lock file with Evidence:
🤖 Generated with Claude Code |
|
Head moved to 66cbbf0. The reviewed 7fe7114 is unchanged ( |
|
Merge driver status (claude-cc-pr-merge, 2026-10-17T15:25:27Z): code gate met, merge BLOCKED on @cristim's explicit OK for the dev deploy. Merging to main runs the existing deploy workflows (Terraform apply to dev), and merge-driver sessions are not permitted to trigger that, so a maintainer must merge or approve it (stricter /health gate on every deploy). |
|
Independent delta review of 66cbbf0 (previous: 7fe7114): clean Reviewer: fresh non-author agent, dispatched by claude-cc-pr-merge-2. The earlier CLEAN verdict at 7fe7114 stands for the rest of the PR.
|
|
Refreshing CI (claude-cc-pr-merge-2): main changed directories this PR also touches after its CI ran. The merge-tree with main is clean (see the delta review), but CI must re-run on the current merge ref. Reopening now. |
|
Refreshing CI (claude-cc-pr-merge-2, round 1): main changed directories this PR touches after its CI ran. Reopening now. |
|
Gate record for 66cbbf0 (claude-cc-pr-merge-2):
|
Root cause
Since #488 / PR #646,
/healthis the liveness contract: it answers HTTP 200 even when every dependency is down. The verdict is in the body's top-level"status"("healthy"/"degraded"). Two verification layers still treated that 200 as the signal, so both passed on a fully degraded deployment:terraform/modules/deployment-checks:check "health"asserted onlystatus_code == 200. Thewarmupprovisioner also returned on any status other than 000/502/503, so the checks could run against a replica that was answering but had not finished initializing.terraform/modules/monitoring/gcp: the uptime check did have a content matcher, but it wasCONTAINS_STRING "healthy". That string is a substring of"unhealthy", and every nested check result is{"status":"healthy"}. A degraded body with the DB unreachable ({"checks":{"auth_store":{"status":"healthy"},"config_store":{"status":"unhealthy",...}},"status":"degraded"}) matched, so the matcher passed on degraded replicas. (The issue describes this check as "HTTP 200 only". In effect it was, but the reason is the substring match.)Fix
check "health"keeps the 200 assert and adds a second one:try(jsondecode(response_body).status, "") == "healthy". The error message names the reported status, or saysunparseable.warmupnow waits for an HTTP 200 whose body's top-levelstatusis"healthy". It parses the body withpython3, the same toolscripts/test-deployment.shalready uses for JSON. Ifpython3is missing it falls back to accepting a 200, and as before it gives up after 10 attempts and continues (on_failure = continue).MATCHES_JSON_PATHwith$.statusandEXACT_MATCH "healthy". It stays on/health, not/ready, as the issue says (pointing it at/readywould flap the alert on deploys).service_unavailablealready needs 180s of failures before it fires./health's status-code semantics are unchanged. Nothing here touches the endpoint or its body, which stays generic per #941.Why gate on the body rather than a per-platform
/readypathThe issue offered two options: a per-platform path variable (
/readyon containers,/healthplus body parsing on Lambda), or a Lambda/ready. Gating on the body status covers every platform with one rule and adds no new module input:HandleLambdaEventrunsensureDBbefore routing, so a DB failure already surfaces as a 5xx. A connected but degraded handler (for example the dev credential key) reportsdegradedin the body.healthyonly when every check is healthy. That is stricter than/ready, which stays 200 after a failed migration run, and strictness is what a post-deploy gate wants.How #648 / #647 can build on this
The two layers now have separate jobs. Post-deploy verification and uptime monitoring read the body
statusof/health. Traffic admission (GKE/Cloud Run readiness in #648, the Fargate ALB target group in #647) should use the status code of/ready, which answers 503 until initialization is done. Neither of those needs a body matcher.TestHealthBodyStatusIsTheVerdictpins the body half of the contract. If a later change wants the deployment check on/readyfor container platforms, adding a path variable to this module keeps the body assert.Tests (fail before, pass after)
terraform/modules/deployment-checks/tests/health.tftest.hcl(new): plan-only runs with a mockedhttpprovider, no network access, and provisioners are never run.healthy_body_passesdegraded_body_with_200_fails: the DB-unreachable body from the issue, with HTTP 200non_json_body_fails: an SPA index containing the word "healthy"non_200_failsOn the old
main.tf,degraded_body_with_200_failsfails, becausecheck.healthpassed when it was expected to fail.terraform/modules/monitoring/gcp/tests/uptime_check.tftest.hcl(new): mockedgoogleprovider. Asserts the path stays/healthand that there is exactly one matcher:MATCHES_JSON_PATH/$.status/EXACT_MATCH "healthy". It fails on the old config.internal/server/health_contract_test.go(new): decodes only the top-levelstatus, the way the gates read it, on both transports:CreateHTTPServermux, healthy and degradedhandleLambdaHTTPEventon/health, degradedIt also asserts that a degraded body still contains
"healthy", which is why a substring match cannot be the verdict.CI: a new
Test /health gate modulesstep interraform-validate(gcp matrix leg) runsinit -backend=false -lockfile=readonly,fmt -checkandtestfor both modules. Until now CI ranterraform testonly undercompute/aws/*andiac/federation/azure-target.Mutation probes, each caught:
!= "": the degraded run failsjson_pathchanged to$.checks: the uptime test failsHealthStatus.StatusJSON tag renamed tostate: the Go contract test failsVerification (static only; nothing deployed, no plan/apply against real accounts)
terraform testpasses for both modules on Terraform 1.10.5, the CI pin, and on 1.15.8, both with a temporaryTF_DATA_DIR.terraform fmt -check -recursive terraform/andterraform validatepass for both modules and forterraform/environments/{gcp,aws}(init -backend=false).shscript.shellcheck -s shis clean. I ran it against local stub servers:go test ./internal/server/ -count=1,./internal/mocks/, and./internal/api/ -run Healthpass (withAWS_CONFIG_FILEandAWS_SHARED_CREDENTIALS_FILEset to/dev/null).go vetpasses, and golangci-lint v2.10.1 reports 0 issues on./internal/server/.actionlint .github/workflows/ci.ymlis clean.git diff --checkis clean. git-secrets--scanon the changed files (temp copy, isolated HOME, patterns fromscripts/setup-git-secrets.sh) is clean, andscripts/test-git-secrets-allowlist.shgives 42 PASS, 0 FAIL.google ~> 5.0provider (lock file 5.45.2) accepts the schema, but I have not seen it against live Cloud Monitoring.Behaviour change to be aware of
degradednow produces acheck.healthwarning. Check blocks never fail the apply. Warmup waits up to about 50s longer for"healthy"before the checks run.degradedfor more than 180s now firesservice_unavailable. That includes the all-zero dev credential key, whichcheckCredentialStoresays monitoring should alert on in any deployed environment.Out of scope
terraform/modules/monitoring/azure/main.tf) has the same substring flaw:content_match = "healthy"also matches"unhealthy"and the nested checks. The issue described it as working, but it isn't. The azurerm standard web test has no JSON-path matcher, so fixing it needs a different approach and its own decision. Worth a follow-up issue.api_health_aliascheck (/api/health) still asserts only routing (200). Its job is to verify that the alias exists.Closes #649
🤖 Generated with Claude Code