Skip to content

fix(gcp): exclude uninitialized replicas from GKE and Cloud Run readiness - #1024

Merged
cristim merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-648
Oct 17, 2026
Merged

cristim merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-648

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Root cause

#646 (issue #488) added /ready, which answers 503 until the replica has finished initialization, and moved the Azure traffic-admission probes onto it. The GCP probes that decide traffic admission were not moved. They still checked /health, which answers 200 even on a cold replica, so pods and instances that were still initializing got traffic during rollouts and scale-out.

  • GKE: the readinessProbe and livenessProbe both used var.health_check_path (/health).
  • Cloud Run: Cloud Run starts sending traffic to an instance once its startup probe passes, so the startup probe acts as the readiness gate. It checked /health.

Fix

  • GKE (terraform/modules/compute/gcp/gke): adds a new variable readiness_probe_path, default /ready, validated to start with /. It drives the readinessProbe. Liveness stays on health_check_path (/health), so a slow init never gets the pod killed. This is the liveness complication the issue calls out. The variable descriptions now say which probe each one controls.

  • Cloud Run (terraform/modules/compute/gcp/cloud-run): the startup probe now checks /ready. A startup probe that never passes shuts the container down, so its failure budget is raised from 3 to 24 attempts (24 x 10 s, about 250 s including the 10 s initial delay). That covers the 120 s default migration timeout (CUDLY_MIGRATION_TIMEOUT) plus database connect retries. It also stays inside Cloud Run's 600 s limit on failure_threshold x period_seconds. timeout_seconds (3) is still at or below period_seconds. Liveness stays on /health.

  • GCP deploy smoke gate (.github/workflows/deploy-gcp.yml, test-deployment job): the old inline gate warmed up /api/auth/check-admin and then parsed the /health body, and those two requests could land on different Cloud Run instances. It is replaced with scripts/wait-for-healthy.sh against /ready, mirroring deploy-azure.yml after fix(server): exclude uninitialized replicas from readiness #646. A sparse checkout fetches the script. The fast gate needs 1 ready response within 5 attempts, 10 s apart. The rollout smoke gate needs 3 ready responses in a row within 30 attempts, 5 s apart. This PR does not modify scripts/wait-for-healthy.sh or its test.

Probe table

Consumer Before After
GKE readinessProbe /health (var.health_check_path) /ready (var.readiness_probe_path)
GKE livenessProbe /health /health (unchanged)
Cloud Run startup probe /health, 3 x 10 s (+10 s delay, about 40 s) /ready, 24 x 10 s (+10 s delay, about 250 s)
Cloud Run liveness probe /health /health (unchanged)
GCP deploy smoke gate (deploy-gcp.yml) warm-up /api/auth/check-admin + inline /health body parse (5 tries) scripts/wait-for-healthy.sh on /ready (1/5 fast gate, then 3-in-a-row/30)
GCP frontend LB (modules/frontend/gcp) serverless NEG, no health check unchanged (serverless NEGs do not support backend health checks; the Cloud Run startup probe is the gate)

GKE Ingress health check caveat

The GCE Ingress load-balancer health check is inferred from the pod readinessProbe, and only when the Ingress is created. GKE docs: "Changes to a Pod's readinessProbe don't affect the Ingress after it is created." So:

  • New GKE deployments get an LB health check on /ready.
  • On an existing deployment, re-applying moves the Service endpoints (pod readiness) to /ready, but the existing LB health check keeps the path it was created with. To move it, recreate the Ingress or edit the health check by hand.
  • Pinning the path with a BackendConfig would need a kubernetes_manifest CRD resource (cluster API access at plan time). That is left as a possible follow-up rather than added here.

Verification

Static checks only. Nothing was deployed or planned against a real account, and no real endpoint was called.

  • New terraform/modules/compute/gcp/readiness_probe_test.go, in the same Go-over-TF-source style as the package's other tests:
    • Before the fix (main): both tests fail. The GKE readiness path is var.health_check_path, readiness_probe_path is undeclared, the Cloud Run startup path is "/health", and the startup window is 30 s.
    • After the fix: both pass.
  • Mutation probes. Each one was caught, then reverted:
    • GKE liveness switched to var.readiness_probe_path
    • readiness_probe_path default set to /health
    • health_check_path default set to /ready
    • Cloud Run failure_threshold = 61 (610 s window, over the 600 s limit)
    • Cloud Run startup timeout_seconds greater than period_seconds
    • Cloud Run liveness switched to /ready
  • go test ./terraform/modules/compute/gcp/ passes. go vet is clean. golangci-lint v2.10.1 reports 0 issues on the package. gocyclo does not apply (no non-test Go changed).
  • terraform fmt -check -recursive terraform/modules/compute/gcp passes. terraform init -backend=false plus validate succeeds on the gke and cloud-run modules (temporary data dirs, deleted afterwards).
  • Smoke gate (commit 683beaf):
    • actionlint 1.7.12 passes, both the local binary and the digest-pinned CI image.
    • zizmor==1.29.0 --offline --persona=pedantic --min-severity=medium .github/workflows/ reports no findings.
    • shellcheck is clean on scripts/wait-for-healthy.sh and its test.
    • scripts/test-wait-for-healthy.sh gives 19 pass, 0 fail.
    • Local stub servers on 127.0.0.1, run with the workflow's own streak and attempt values (interval set to 0): /ready 503 x3 then 200 passes both gates; never-ready fails both with exit 1 and prints the last 503 body.
    • git merge-tree --write-tree HEAD fork/fix/issue-649 is clean, so it does not conflict with fix(infra): gate deployment checks and GCP uptime on /health body status #1023.
  • git-secrets scan on an isolated temporary copy is clean. scripts/test-git-secrets-allowlist.sh gives 42 PASS, 0 FAIL. checkov and tflint are not installed locally, so CI covers them.

Operators: re-apply the GKE or Cloud Run stack for the probe changes to take effect. For GKE Ingress, see the caveat above.

Out of scope

Closes #648

🤖 Generated with Claude Code

…ness

After LeanerCloud#488 added /ready (503 until the replica has completed
initialization), the GCP traffic-admission probes still gated on
/health, which answers 200 on a cold replica.

- GKE: new readiness_probe_path variable (default /ready) drives the
  readinessProbe; the liveness probe stays on health_check_path
  (/health) so a slow init does not get the pod restarted.
- Cloud Run: the startup probe (Cloud Run's admission gate) now checks
  /ready, with its failure budget widened from 3 to 24 x 10 s so a
  slow-but-recovering init (120 s default migration timeout plus
  connect retries) does not restart-loop the container. Liveness stays
  on /health.
- Static Go tests over the module sources pin both probe paths, the
  variable defaults and the startup window bounds.

Closes LeanerCloud#648

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 5 billable files and costs up to $1.25.

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: e1f5d5ba-57b0-4419-8988-3f9e816d0fc4
📥 Commits

Reviewing files that changed from the base of the PR and between 110a496 and 683beaf.

📒 Files selected for processing: 5
  • .github/workflows/deploy-gcp.yml
  • terraform/modules/compute/gcp/cloud-run/main.tf
  • terraform/modules/compute/gcp/gke/main.tf
  • terraform/modules/compute/gcp/gke/variables.tf
  • terraform/modules/compute/gcp/readiness_probe_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-quarter Within the quarter impact/many Affects most users effort/m Days type/bug Defect triaged Item has been triaged labels Oct 16, 2026
The GCP test-deployment job still used the pre-LeanerCloud#488 inline smoke gate:
warm up /api/auth/check-admin, then parse the /health body. The two
requests could land on different Cloud Run instances, and /health is a
liveness signal. Replace both inline steps with scripts/wait-for-healthy.sh
against /ready, with the same fast-gate (1 of 5) and rollout smoke-gate
(3 in a row within 30) parameters deploy-azure.yml uses since LeanerCloud#646.

Refs LeanerCloud#648

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Independent review verdict: CLEAN at aabe506

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. Read-only, with no cloud credentials and no plan or apply. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Probes vs the Go server. All of them hit port 8080. /ready is routed at internal/server/http.go:37 and returns 503 until ready() passes (health.go:53-83).

Platform Probe Path Timings
GKE readiness var.readiness_probe_path (default /ready) unchanged (gke/main.tf ~598-607, variables.tf:226-233)
GKE liveness /health (unchanged)
Cloud Run startup /ready delay 10, period 10, timeout 3, failure_threshold 24 (~250s) (cloud-run/main.tf ~195-203)
Cloud Run liveness /health (unchanged)

/ready cannot get stuck:

  • startBackgroundInit (http.go:104-124) runs the DB init at boot with retries, so readiness flips without waiting for a request.
  • A failed migration still counts as ready (health.go:66-70), so nothing is permanently unready.
  • Cloud Run semantics are right: the startup probe is the admission gate.

Notes (non-blocking):

  1. If the DB is down for more than about 250s at deploy, the Cloud Run revision never becomes ready and the deploy fails. Before this PR, /health would have passed. This fails safe.
  2. Closes #648 slightly overclaims. fix(gcp): exclude uninitialized replicas from GKE and Cloud Run readiness #648's third bullet (the deploy-gcp.yml smoke gate) is explicitly left out of scope in the body, so the issue should stay open or a follow-up should be filed.

Deploy impact (merge held for an explicit owner OK):

  • deploy-gcp.yml runs on every push to main.
  • compute.tf instantiates cloud-run or gke by compute_platform. Cloud Run gets a new revision and GKE a rolling Deployment update; nothing is replaced.
  • Existing GKE Ingress LB health checks are not moved, as the body documents.

What was attacked and how:

  • terraform init -backend=false and validate pass in both modules, and terraform fmt -check -recursive passes.
  • go test -count=1 ./terraform/modules/compute/gcp/... passes. The test extracts blocks by brace matching and fails loud on any missing block, variable or number. It asserts the paths, defaults, a 180-600s startup window, and timeout <= period.
  • Mutation probe not completed: the reviewer's scratch edit (Cloud Run startup path to /health) was blocked by the local permission policy, and it was not worked around. The test's "/ready" path assertion would fail on that change; this was checked by reading, and the author reports the same mutation caught.

Gate:

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Delta verdict: CLEAN at 683beaf (same reviewer, scope git diff aabe5069 683beaf0, which changes only .github/workflows/deploy-gcp.yml).

  • The URL is fresh and safe. The gate passes APP_URL from needs.build-and-deploy.outputs.service_url, the fresh deploy output (workflow lines 262-268 and 290/303), through the environment rather than shell interpolation. It goes to the unchanged scripts/wait-for-healthy.sh, which polls /ready.
  • Bounded budgets. Readiness is 5×10s after a 30s sleep. The smoke check needs 3 consecutive passes within 30×5s. Terraform apply already waits for the Cloud Run startup probe (about 250s).
  • Fails the job. The script exits 1, with no || true and no continue-on-error. Nothing secret is echoed.
  • Other deploys unchanged. The script itself is unchanged, so deploy-azure and ci are unaffected.
  • Checks. shellcheck, actionlint and the zizmor pre-commit hook pass.

This also resolves note 2 of the main verdict: #648's third bullet (the deploy smoke gate) is now covered, so Closes #648 is accurate. The merge is still held for @cristim's OK. (claude-cc-reviewer)

@IAmJSD

IAmJSD commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

New head: 683beaf0 (adds a commit on top of aabe5069, no rebase). It does the issue's third bullet: the GCP deploy smoke gate now uses scripts/wait-for-healthy.sh against /ready, the same way deploy-azure.yml does.

Change: in the deploy-gcp.yml test-deployment job, the inline warm-up plus /health body-parse gate and the 3x /health smoke loop are replaced by:

  • a sparse checkout of scripts/wait-for-healthy.sh
  • a fast gate: 1 ready response within 5 attempts, 10 s apart
  • a rollout smoke gate: 3 ready responses in a row within 30 attempts, 5 s apart

The script itself is untouched. The gate's URL is the Cloud Run uri (https), so no redirect is involved. This relies on #1025's 2xx-only change to the script.

Evidence (local; nothing ran against GCP):

  • actionlint 1.7.12 passes, both the local binary and the digest-pinned CI image (rhysd/actionlint:1.7.12@sha256:b1934ee5...).

  • zizmor==1.29.0 --offline --persona=pedantic --min-severity=medium .github/workflows/ reports no findings.

  • shellcheck is clean on scripts/wait-for-healthy.sh and scripts/test-wait-for-healthy.sh.

  • bash scripts/test-wait-for-healthy.sh gives 19 pass, 0 fail.

  • Stub servers on 127.0.0.1, run with the workflow's streak and attempt values (interval 0):

    Scenario Fast gate (1/5) Smoke gate (3/30)
    /ready 503 x3 then 200 exit 0 exit 0
    never ready exit 1, last 503 body printed exit 1, last 503 body printed
  • go test ./terraform/modules/compute/gcp/ passes (the probe tests from the first commit).

  • The workflow YAML parses, and git diff --check is clean.

  • git-secrets on a fresh git init temporary repo with an isolated HOME is clean, and scripts/test-git-secrets-allowlist.sh gives 42 pass, 0 fail.

  • git merge-tree --write-tree HEAD fork/fix/issue-649 (fix(infra): gate deployment checks and GCP uptime on /health body status #1023) and git merge-tree --write-tree HEAD fork/fix/issue-647 (fix(aws): gate Fargate ALB traffic admission on /ready #1025) are both clean. fix(infra): gate deployment checks and GCP uptime on /health body status #1023 does not touch deploy-gcp.yml or the script.

🤖 Generated with Claude Code

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

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 (GKE/Cloud Run readiness on /ready).

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Independent delta review of 683beaf (previous: aabe506): clean

Reviewer: fresh non-author agent, dispatched by claude-cc-pr-merge-2. The earlier CLEAN verdict at aabe506 stands for the rest of the PR.

  • OLD is an ancestor of NEW. The delta is .github/workflows/deploy-gcp.yml (+32/-42): the inline health and smoke loops are replaced by scripts/wait-for-healthy.sh against /ready.
  • Deploy safety: no on:, permissions, id-token, environment: or secrets. lines are touched (the job keeps id-token: write, contents: read). The new checkout uses persist-credentials: false and the same SHA-pinned actions/checkout, with a sparse checkout as in deploy-azure.yml:443. The URL now arrives via env: APP_URL and is quoted, which is safer than the old inline ${{ }}.
  • wait-for-healthy.sh (unchanged from main) fails closed: set -euo pipefail, limit validation exits 2, curl --max-time 10 --fail-with-body, errors reset the streak, and exhaustion exits 1. shellcheck and actionlint are clean. scripts/test-wait-for-healthy.sh passes 19/0.
  • Non-blocking:
    1. A 3xx would count as ready, but there are no redirects here and /ready returns 200 or 503.
    2. The ::error:: line echoes our own service's body.
    3. The gate keys on /ready instead of the /health body, as intended by fix(server/azure): exclude uninitialized replicas from readiness #488/fix(gcp): exclude uninitialized replicas from GKE and Cloud Run readiness #648.
  • git merge-tree with main is clean.

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

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.

@cristim cristim closed this Oct 17, 2026
@cristim cristim reopened this Oct 17, 2026
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Refreshing CI (claude-cc-pr-merge-2, round 1): main changed directories this PR touches after its CI ran. Reopening now.

@cristim cristim closed this Oct 17, 2026
@cristim cristim reopened this Oct 17, 2026
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Refreshing CI (claude-cc-pr-merge-2, round 2): main changed directories this PR touches after its CI ran. Reopening now.

@cristim cristim closed this Oct 17, 2026
@cristim cristim reopened this Oct 17, 2026
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Gate record for 683beaf (claude-cc-pr-merge-2):

  • Independent adversarial review: CLEAN at aabe506 (fix(gcp): exclude uninitialized replicas from GKE and Cloud Run readiness #1024 (comment)), plus a clean independent delta review of this exact SHA (fix(gcp): exclude uninitialized replicas from GKE and Cloud Run readiness #1024 (comment)). The local verification evidence and its limits are in those comments: fixture or mock level, no plan or apply against a real backend.
  • Owner hold lifted 2026-10-17 (~/.claude/decisions/2026-10-17-platform-holds-lifted.md). GCP readiness probes and deploy gate on /ready (merge applies to dev).
  • CI: the latest 'CI - Build & Test' run on this SHA is completed and green, and the required checks pass. Both were re-checked at merge time, including the guard against main changing the same packages since CI ran.
  • Labels mirror the linked issue. No unresolved threads. CodeRabbit: skipped (quota).
  • Merging with squash and --match-head-commit; no --admin.

@cristim
cristim merged commit b47dbfd into LeanerCloud:main Oct 17, 2026
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(gcp): exclude uninitialized replicas from GKE and Cloud Run readiness

2 participants