[IAmJSD]

Root cause

In cleanup-staging.yml's destroy-gcp job, the Cloud SQL step ended with an inline peering cleanup that:

1. ran terraform state rm module.networking.google_service_networking_connection.private_vpc_connection 2>/dev/null || true before the delete. Unlike the root-level Cloud SQL addresses fixed in #1971, this address is module-qualified, so it really removed state;
2. then ran gcloud services vpc-peerings delete --network=cudly-staging-vpc ... 2>/dev/null || echo "VPC peering already gone or not found", so a failed delete still reported success;
3. named the network by the literal cudly-staging-vpc instead of the network_name output the state already publishes. A renamed network would have made the delete a "not found" no-op while the state rm still ran.

The result was a live peering that no state tracked.

Fix

• New scripts/delete-owned-vpc-peering.sh TERRAFORM_STATE_DIR GCP_PROJECT_ID, run as its own step, Delete the Service Networking peering this state owns. It sits between the Cloud SQL step and Terraform Destroy.
  • It deletes only while the state still tracks the peering address. The check is an exact match of the address against an unfiltered terraform state list, piped through the shared scripts/select-owned-name.sh. A re-run after a finished cleanup is therefore a no-op, and a re-run after a failed delete retries it. I didn't use terraform state list ADDRESS because it exits 1 both when the address is absent and when the state can't be read (measured on Terraform 1.15.8 with a local state), so those two cases can't be told apart without swallowing an error.
  • The network comes from terraform output -json → .network_name.value. That output resolves through module.networking.network_name to google_compute_network.main.name, the network private_vpc_connection is attached to. If the output is not JSON, has no network_name, has a null value, or isn't an RFC 1035 name, the script exits 1 and leaves state alone.
  • Nothing is swallowed. There's no 2>/dev/null, || true or || echo, and no --async, so gcloud's exit status confirms the delete. terraform state rm runs only after the delete returns 0. A failed delete exits with gcloud's own status and leaves the address in state. The error message tells the operator how to remove the address by hand if the peering is confirmed gone already.
• New scripts/test-vpc-peering-delete-scope.sh is the fourth sibling suite, next to the ECR, RDS and Cloud SQL ones. It runs as the vpc-peering-delete-scope job in ci.yml and is listed in ci-success's needs. It checks:
  • Terraform chain: the environment's network_name output comes from module.networking.network_name, which is google_compute_network.main.name, and private_vpc_connection has network = google_compute_network.main.id.
  • Wiring: exactly one step with that name calls the script with the pinned state directory and "$PROJECT", with nothing appended. Inside destroy-gcp, the order is Cloud SQL step, then peering step, then Terraform Destroy.
  • Static checks on the script: one jq -er '.network_name.value' read, one delete using --network="$NETWORK", no literal network, no --async, one state rm "$PEERING_ADDR" after the delete, and nothing swallowed.
  • Behavior with stubbed terraform and gcloud (one shared call log; the gcloud stub accepts only the exact delete argv): the golden path on a renamed network, a failed delete, a failed state rm, the address missing from state with only near-miss addresses present, an empty state, the address on an unterminated last line, an unreadable state, nine malformed-output cases, malformed project ids, and arity errors.
  • Sweep: every workflow plus a recursive glob of scripts/ must have exactly one gcloud services vpc-peerings delete site, and it must be the script. Fixtures pin both directions: the pre-fix step is flagged, and prose that only mentions the command is not.
• scripts/lib/code-scan-awk.sh: the new suite is added to build_swept_scripts' exclusion list. Without it, the suite's own fixtures would be swept.

Verification

All of this is local, with stubbed terraform and gcloud and fixtures. Nothing calls real GCP, terraform state or any workflow.

• bash scripts/test-vpc-peering-delete-scope.sh → passed: 41, failed: 0.

• Fail before: I ran the new suite (plus the updated code-scan-awk.sh) against a git archive origin/main copy → passed: 17, failed: 24. The sweep names the pre-fix inline delete: cleanup-staging.yml:412 total 1. The wiring and step-order checks fail, and every behavior case fails because the script doesn't exist.

• Mutation probes on a copy of the fixed tree. Each one fails the suite:

  Mutation	Failed checks
  state rm moved before the delete	5
  --network=cudly-staging-vpc hardcoded	3
  --async added	7
  failed delete turned into exit 0	1
  selector replaced with grep -F ... | head -1 || true	3
  network-name validation removed	4
  || true appended to the workflow call	1
  an extra inline peering delete added to the workflow	1

• The sibling suites still pass: test-cloud-sql-delete-scope.sh 62/0, test-ecr-delete-selection.sh 59/0, test-rds-deletion-protection-scope.sh 58/0, test-aws-tfstate-platform-key.sh 26/0.

• actionlint .github/workflows/ci.yml .github/workflows/cleanup-staging.yml passes (shellcheck present).

• zizmor==1.29.0 --offline --persona=pedantic --min-severity=medium on both workflows → no findings.

• shellcheck is clean on delete-owned-vpc-peering.sh. shellcheck -S warning is clean on the new suite and on code-scan-awk.sh. The suite has only SC2015/SC2016 info notes, the same kind the sibling suites carry.

• git-secrets: I ran scripts/setup-git-secrets.sh in a fresh git init copy with an isolated HOME, then git secrets --scan on the changed files. Both are clean, and scripts/test-git-secrets-allowlist.sh gives 42 PASS, 0 FAIL.

• git diff --check is clean. No Go files changed, so go test, vet, golangci-lint and gocyclo don't apply.

Not covered by real-cloud evidence: I didn't run real gcloud services vpc-peerings delete or terraform state list against the GCS backend. The --async and exit-status semantics come from gcloud's documented behavior; the terraform state list semantics were measured locally on Terraform 1.15.8 (CI pins 1.10.0).

Out of scope / notes

• No retry. The old comment said GCP "needs extra time after Cloud SQL deletion to release VPC peering". If the delete fails transiently, the step now fails loudly with the address still in state, and a re-run retries it. Before, the failure was swallowed and terraform destroy then failed on google_compute_network, so the job failed either way. I didn't add a bounded retry loop.
• Peerings already leaked by the old code. If a peering was already removed from state by the old code and left live, the state no longer tracks it, so this script skips it. terraform destroy then fails on the network, as it does today. Such a peering has to be deleted by hand once.
• scripts/gcp-import-dev-state.sh runs gcloud services vpc-peerings list ... 2>/dev/null. That's a read-only existence probe, not a delete, so I left it alone.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, impact/internal, triaged, urgency/this-quarter, effort/s, type/security.

Closes #330

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• 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 2 (claude-fleet, cold): APPROVE 626f0fc

Findings (none blocking):

• [non-blocking] .github/workflows/cleanup-staging.yml:416-423 - The new peering step runs before Terraform Destroy, and destroy has no if:.
  • A failed peering delete now stops the job before destroy runs.
  • The most likely failure is GCP's "producer services still using this connection" right after the Cloud SQL delete, which the removed comment hinted at.
  • Before this PR, the job also failed, but only after destroy had removed everything except the network. Now the whole staging stack stays up, and keeps billing, until someone re-runs the job.
  • The body mentions there is no retry, but not this cost difference.
• [non-blocking] scripts/delete-owned-vpc-peering.sh:122-145 - Re-running after a failed state rm (with the delete already succeeded) is not a retry.
  • The address is still in state and gcloud returns NOT_FOUND, so the step fails on every re-run until someone runs state rm by hand.
  • This fails loudly by design, and the error text gives the command. Treating NOT_FOUND as success would be a reasonable follow-up.
• [non-blocking] delete-owned-vpc-peering.sh:103-107 - The OUTPUTS_TYPE != object check is redundant.
  • Without it, non-JSON fails under set -e, and [], a string or null make jq -er '.network_name.value' exit non-zero.
  • Deleting the check leaves all 41 checks passing; only the error message changes.
• [non-blocking] The project-id regex is copied from delete-owned-cloud-sql-instance.sh, as the comment says. Minor duplication.

Against #330: the PR does all three things the issue asks:

• delete first, and state rm only after a 0 exit;
• nothing swallowed;
• the network read from the output, through outputs.tf:5-7, then modules/networking/gcp/outputs.tf:6-8, then main.tf:122-126.

Attacked:

• Blast radius:
  • The only other vpc-peerings site is the read-only list in scripts/gcp-import-dev-state.sh:238.
  • Removing the cd from the Cloud SQL step is harmless, because each later step sets its own directory.
  • select-owned-name.sh exits 0 when the address is absent, so the "not in state" path works.
• No TF_VAR_project_id on the peering step: checked against real Terraform v1.10.5, with a required variable unset and stdin from /dev/null.
  • state list, output -json and state rm all exit 0.
  • state list ADDR exits 1 for an absent address, which matches the author's reason for not using it.
• gcloud: services vpc-peerings delete waits by default (it takes [--async]), so its exit status confirms the delete.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • In-state check skipped: 2 FAIL.
  • exit "$rc" changed to exit 1: 1 FAILS.
  • jq -er changed to jq -r: 4 FAIL.
  • --project dropped: 6 FAIL.
  • --service dropped: 6 FAIL.
  • Project regex loosened to ^.*$: 3 FAIL.
  • || true added on state rm: 3 FAIL.
  • Type check removed: 0 fail. This is the redundancy above.

Verified locally:

• Head: bash scripts/test-vpc-peering-delete-scope.sh gives 41 passed, 0 failed. Sibling suites: cloud-sql 62/0, ecr 59/0, rds 58/0, aws-tfstate 26/0.
• Merged tree: 41/0, and cloud-sql 62/0. On main, only ci.yml moved, and it merges cleanly.
• Fail-before: the new suite on base gives 17 passed and 24 failed, including the sweep flagging the inline delete.
• Static checks: actionlint, shellcheck and zizmor --offline --persona=pedantic --min-severity=medium on both workflows are clean.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 626f0fc555d06655a4d37c195a53db8cecc6dce6

Blocking:

• none

Non-blocking:

• scripts/delete-owned-vpc-peering.sh: a run can delete the peering in GCP and then fail at terraform state rm, for example on a transient GCS backend error. On the next run the address is still in state, gcloud ... delete fails with "not found", and the job fails until someone runs state rm by hand. The error text gives the exact command, and the gcp-tfstate-staging concurrency group makes this unlikely, so this is acceptable. A follow-up could treat a confirmed-absent peering (from gcloud services vpc-peerings list --network=...) as already deleted.
• Without a bounded retry, a peering that GCP has not released yet just after the Cloud SQL delete now fails this step instead of terraform destroy. The job failed in that case before too. This version fails earlier and keeps state accurate, so it is a net improvement. The body already says so.

Attacked:

• Ordering and swallowing: the script runs gcloud (no --async) and then state rm only on rc 0. Nothing is masked. The network comes from output -json with a type check, jq -e and RFC 1035 validation, and project ids are validated. The workflow step is env-routed (no ${{ }} in run:), sits between the Cloud SQL step and Terraform Destroy in destroy-gcp, and the new CI job has contents: read and persist-credentials: false, and is in ci-success.needs.
• The new step drops the TF_VAR_project_id that the Cloud SQL step sets, so I checked that the script's terraform calls don't need it. On a local Terraform 1.10.5 state, with a required variable that has no default and no -var, output -json, state list and state rm all exit 0.
• Fail-before: I restored the merge-base cleanup-staging.yml and removed the new script, keeping the suite. Result: passed: 17, failed: 24. A mutation that runs state rm ... || true before the delete gives passed: 36, failed: 5. Both were restored afterwards.
• Ran: bash scripts/test-vpc-peering-delete-scope.sh → 41/0. The sibling suites (cloud-sql 62/0, ecr 59/0, rds 58/0, aws-tfstate 26/0) all pass with the code-scan-awk.sh exclusion change. Also ran shellcheck (script clean; -S warning clean on the suite and lib), actionlint 1.7.7 on ci.yml and cleanup-staging.yml (clean), and the secrets-scan ("clean; planted key flagged"). Nothing ran against real GCP. As the body says, this change only takes effect the next time someone triggers cleanup-staging.

Body-stale:

• none

_{Reviewer: rA3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Fixed the held item (a failed peering delete skipped terraform destroy). New head: 46df186295a3c47214aa80fe3fa5edcc2ae8fe04. It is a merge of origin/main (clean) plus one commit.

What changed

• cleanup-staging.yml, destroy-gcp:

  • The peering step now has id: delete-peering and continue-on-error: true. A failed delete no longer skips Terraform Destroy, so the job removes everything it can, as it did before this PR.
  • A new last step, Fail if the peering delete failed, has if: ${{ !cancelled() && steps.delete-peering.outcome == 'failure' }}. It writes an ::error:: annotation and runs exit 1, so the job still fails and the failure is not swallowed. I used !cancelled() instead of the implicit success() so the step still reports when the destroy fails too.
  • The order is still Cloud SQL, then peering, then destroy. A failed Cloud SQL step still skips the destroy, as it did before.

• delete-owned-vpc-peering.sh is unchanged. state rm still runs only after a successful delete. When the delete fails, the address stays in state, so the destroy still tracks the peering.

• test-vpc-peering-delete-scope.sh adds a small model of the runner's step semantics for destroy-gcp. It covers if: with success/failure/always/cancelled and steps.ID.outcome|conclusion, the implicit success(), and continue-on-error. Any if: it does not understand fails the check. It pins these cases:

  • nothing fails: destroy runs and the job passes;
  • the peering delete fails: destroy runs and the job fails;
  • peering and destroy both fail: the job fails, and no step is skipped;
  • the Cloud SQL delete fails: destroy is skipped and the job fails.

  Five mutants of the real workflow must each be flagged: no continue-on-error, the reporting step removed, the step id renamed, the reporting step on the implicit success(), and if: always() on the destroy. The existing behavior case (a failed delete exits with gcloud's status and removes no state) still pins that the state rm is skipped.

Checks (all local; nothing ran the workflow or called GCP)

• The suite passes 47/0 under both gawk and mawk. Fail-before: with this PR's previous cleanup-staging.yml, the new check fails, because Terraform Destroy is skipped when the peering step fails.
• The sibling suites pass: cloud-sql 62/0, ecr 59/0, rds 58/0, aws-tfstate 26/0, tfstate-concurrency-group 41/0.
• actionlint 1.7.12 on the whole repo: clean. shellcheck -S warning on the suite: clean.
• secrets-scan: "clean; planted key flagged". test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
• pre-commit on the changed files: all hooks pass, including actionlint and the workflow injection audit. trivy isn't installed on the host, so I ran the same trivy config command through aquasec/trivy:0.69.3 (exit 0).

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 46df186295a3c47214aa80fe3fa5edcc2ae8fe04

Blocking:

• none

Non-blocking:

• .github/workflows/cleanup-staging.yml:449-453: when the peering delete fails, Terraform Destroy still runs and the job then fails. If destroy itself removes the peering (which is possible once Cloud SQL is gone), the job still goes red even though staging ended up clean. A re-run is a no-op (the address has left state), so this is conservative, not wrong. It's worth knowing for whoever triages a red cleanup run.
• delete-owned-vpc-peering.sh: a peering that is already gone in GCP but still in state (deleted by hand) makes gcloud exit non-zero, so the step fails. Destroy then reconciles it and the job fails once. The error message gives the manual state rm command, which is acceptable.

Attacked:

• Fail-before: in the scratch tree I restored origin/main's cleanup-staging.yml and removed the new script. bash scripts/test-vpc-peering-delete-scope.sh gave passed: 18, failed: 29. The wiring, step order, continue-on-error/reporting mutants and every behavior case failed. On the head it gives 47/0. A mutation that appends || true to the gcloud delete inside the script fails 3 checks (swallow, failed-delete status, error text). Both restored, git status clean.
• I hand-ran the script against my own terraform/gcloud stubs. Golden path: state list, output, delete on the output-named network, then state rm, in that order. A gcloud exit of 7 propagates as rc=7 with no state rm. A failing state list exits 1 before any delete. A near-miss ..._x address is not matched.
• Terraform chain: outputs.tf network_name comes from module.networking.network_name, which is google_compute_network.main.name, which private_vpc_connection.network uses. The CI SA has roles/servicenetworking.networksAdmin and compute.networkAdmin, so the delete that used to be swallowed won't now fail on permissions. The job keeps the gcp-tfstate-staging concurrency group, and no expressions are interpolated in run:.
• Sibling suites pass: cloud-sql 62/0, ecr 59/0, rds 58/0, tfstate-key 26/0. shellcheck is clean on the script, and -S warning is clean on the suite and code-scan-awk.sh. The workflows parse, secrets-scan.sh printed "clean; planted key flagged", and GitHub reports MERGEABLE/CLEAN with closingIssuesReferences [330]. All four suggested-fix points in sec(ci): GCP staging cleanup removes the Service Networking peering from state before a swallowed delete #330 are met. No real gcloud or terraform was run.

Body-stale:

• "bash scripts/test-vpc-peering-delete-scope.sh → passed: 41, failed: 0" and the fail-before "17 passed, 24 failed": on this head it's 47/0, and my fail-before gave 18/29. The later commit added continue-on-error checks.
• The Fix section doesn't mention the later design (commit "run terraform destroy when the GCP peering delete fails"). The peering step is continue-on-error: true, Terraform Destroy runs anyway, and a final Fail if the peering delete failed step fails the job. The "No retry" note ("the step now fails loudly") is still true, but a reader would assume destroy is skipped, and it isn't.

_{Reviewer: rB25. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 46df186295a3c47214aa80fe3fa5edcc2ae8fe04

Blocking:

• none

Non-blocking:

• scripts/select-owned-name.sh: the header's caller list (ECR, RDS, Cloud SQL) doesn't name the new delete-owned-vpc-peering.sh caller. Only a doc gap.
• .github/workflows/cleanup-staging.yml (delete-peering step): if the peering is already gone in GCP while the address is still in state, gcloud returns not-found. The step then fails, Terraform Destroy refreshes the address out of state, and the job fails. The re-run is a no-op and passes. That is loud and safe, and it matches what the PR says it intends, but expect one red run in that case.

Attacked:

• Read the script, the new step, the continue-on-error and final !cancelled() && steps.delete-peering.outcome == 'failure' gate, and the output-to-network chain in terraform/environments/gcp/outputs.tf and terraform/modules/networking/gcp/main.tf:122. Checked that select-owned-name.sh exits 0 on an empty selection, so the "not in state" branch is reachable under set -e. Checked that state rm can only run after gcloud returns 0.
• Fail-before: restored main's cleanup-staging.yml and removed the script. The suite went from 47/0 to 18 passed, 29 failed. Mutation: I changed the failed-delete branch to fall through to state rm. The suite flags it ("a failed delete exits with gcloud's status and removes no state").
• Ran bash scripts/test-vpc-peering-delete-scope.sh (47/0) and the sibling suites: cloud-sql 62/0, ecr 59/0, rds 58/0, aws-tfstate 26/0, tfstate-concurrency 41/0. Also ran actionlint on both workflows, shellcheck on the script, shellcheck -S warning on the suite and code-scan-awk.sh, and git diff --check. All were clean. No Go files changed.

Body-stale:

• The body says the suite reports passed: 41, failed: 0. On this head it reports 47/0.
• The body describes the peering step as failing loudly. It doesn't mention that the step is now continue-on-error, so Terraform Destroy still runs, and that a separate final step fails the job. The behaviour is right; only the description lags.

_{Reviewer: rB26. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 46df186295a3c47214aa80fe3fa5edcc2ae8fe04 merged onto main (99f0b614), together with #1144, #1210, #1213, #1220 in that order (combined tree 3c7f65ac, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 46df1862 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 46df186295a3c47214aa80fe3fa5edcc2ae8fe04.