fix(ci): serialize AWS and GCP Terraform state writers on environment, not branch - #1812
Conversation
…, not branch Both halves of #1801 were fixed for Azure only in #1803 and were still live on AWS and GCP. Half 1: the state key is built from the environment but the concurrency group was keyed on `github.ref`, so two runs on different refs that resolve to the same environment landed in different groups and applied against one state file. Workflow-level `concurrency` cannot see `needs`, so each group moves to the job that writes state, keyed on the same value that builds the state key: aws-tfstate-<env> github-<env>/terraform.tfstate (S3) aws-fargate-tfstate-<env> github-fargate-<env>/terraform.tfstate (S3) gcp-tfstate-<env> github-<env>/default.tfstate (GCS) Applied to all ten previously ungrouped state-mutating jobs across deploy-aws-lambda.yml, deploy-aws-fargate.yml, deploy-gcp.yml, destroy-fargate-dev.yml, cleanup-staging.yml and rollback.yml, so serialization holds across workflows, not just within one. `cancel-in-progress: false` on every one: cancelling mid-apply leaves a half-applied stack and a stuck lock. Half 2: four steps deleted the state lock object with no age check and no check that the lock was this run's. Two ran unconditionally before `terraform init`, two on `failure() || cancelled()`. The `cancelled()` half is the decisive one: those steps run while `terraform apply` is still shutting down, destroying a lock the dying run may still be using. All four are removed rather than made conditional, so a real collision fails loudly with "Error acquiring the state lock". deploy-aws-fargate.yml's operator-gated `clear_stale_lock` step is kept as the recovery path. destroy-fargate-dev.yml was not named in the issue but writes github-fargate-dev/terraform.tfstate and carried both defects. rollback.yml's rollback-aws-fargate takes the aws-tfstate-* group because its backend key is the Lambda namespace, not the Fargate one. That pre-existing mismatch is tracked in #1811. Closes #1806
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughGitHub Actions workflows now serialize Terraform operations by environment-specific state. The changes cover deployment, cleanup, destroy, and rollback jobs. Automatic Terraform lock deletion was removed from AWS and GCP workflows. ChangesTerraform state protection
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This change serializes Terraform state writes by environment and removes unsafe automatic lock deletion while retaining only explicit operator-confirmed recovery. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant DeploymentWorkflow
participant GitHubActionsConcurrency
participant TerraformState
DeploymentWorkflow->>GitHubActionsConcurrency: Resolve deployment environment
GitHubActionsConcurrency->>TerraformState: Serialize matching state writers
DeploymentWorkflow->>TerraformState: Apply Terraform changes
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Closes #1806
Both halves of #1801 were fixed for Azure only in #1803. This lands the equivalent for AWS and GCP. It is not a copy-paste of #1803: the environment is derived differently per workflow, Fargate uses a separate state namespace and needs its own group, and one lock-removal step is deliberately kept.
Enumeration, not sampling
.github/workflows/holds 16.ymlfiles (a 17th entry isREADME.md). All 16 were classified by grepping for backend config, state keys, bucket names and state-mutatingterraformsubcommands, not by filename.aws_sanity.ymlrg -l -i 'terraform|tfstate|aws s3|gsutil'-> no matchazure_sanity.ymlfrontend-build.ymlfrontend-build-sentinel.ymlfrontend-e2e.ymlci.ymlterraformmentions, allterraform init -backend=false+validateat:386. No backend, no lockpre-commit.ymlterraform_validatehookdatabase-migration.ymlterraform initat:258,:355,:432thenterraform outputonly.outputtakes no lock. (Those are bareinitwith no-backend-configagainst a partial backend block, flagged in #1803 and still unfixed; either way nothing is written)deploy-all.ymlterraformmention, in a comment. Reaches state only viauses:deploy-azure.ymldeploy-aws-lambda.ymlterraform apply:281; keygithub-<env>/terraform.tfstatedeploy-aws-fargate.ymlterraform apply:179; keygithub-fargate-<env>/terraform.tfstatedeploy-gcp.ymlterraform apply:170; prefixgithub-<env>(GCS)destroy-fargate-dev.ymlterraform destroy:143; keygithub-fargate-dev/terraform.tfstatecleanup-staging.ymlterraform destroy:169,:246,:316,:412plus fourterraform state rmat:395-400rollback.ymlterraform apply:238,:326,:389,:481Line numbers above are
origin/mainat17a568f42.This differs from the issue. The issue names three workflows for half 1 and two steps for half 2. The real candidate set is larger in two places and smaller in one:
destroy-fargate-dev.ymlis a state writer the issue does not mention, and it carried both defects: no concurrency group at all, and an unconditionalaws s3 rmof the.tflockbefore everyterraform init(:111-114).deploy-gcp.ymlhad two lock deletions, not one. The issue cites thefailure() || cancelled()step. There was also an unconditionalgsutil rminsideTerraform Init(:145-150) that deleted the lock on every single run, before init, with no condition whatsoever. That is strictly worse than the one the issue names.deploy-aws-fargate.ymlno longer has half 2. The issue lists it under half 1 only, which is right, but for completeness: its lock-removal step (:138-152) is already gated oninputs.clear_stale_lock == true, aworkflow_dispatch-only boolean defaulting false, and its failure-path release step was already deleted with a comment explaining why. That step is retained (justified below).Enumerated mechanically rather than by eye: a script parses every workflow, selects jobs whose
run:blocks contain a state-mutatingterraformsubcommand, and prints the group next to the state key.Half 1: group on the job that writes state, keyed on the value that builds the state key
Workflow-level
concurrencycannot seeneeds(constraint proven below), so each group lives on the deploying job. Three distinct state objects across AWS and GCP, therefore three group prefixes:aws-tfstate-<env>s3://<bucket>/github-<env>/terraform.tfstateaws-fargate-tfstate-<env>s3://<bucket>/github-fargate-<env>/terraform.tfstategcp-tfstate-<env>gs://<bucket>/github-<env>/default.tfstateFargate gets its own prefix rather than sharing
aws-tfstate-*. Sharing would serialize two independent state files against each other, buying nothing and costing queue evictions.Every group and the state key it guards, side by side, at post-merge line numbers:
deploy-aws-lambda.ymlbuild-and-deploy:222aws-tfstate-${{ needs.prepare.outputs.target_environment }}:255ENVIRONMENT: needs.prepare.outputs.target_environment->:257key = "github-%s/terraform.tfstate"deploy-aws-fargate.ymldeploy:132aws-fargate-tfstate-${{ needs.prepare.outputs.environment }}:175ENVIRONMENT: needs.prepare.outputs.environment->:177key = "github-fargate-%s/terraform.tfstate"deploy-gcp.ymlbuild-and-deploy:131gcp-tfstate-${{ needs.prepare.outputs.environment }}:165ENVIRONMENT: needs.prepare.outputs.environment->:167prefix = "github-%s"destroy-fargate-dev.ymldestroy:85literalaws-fargate-tfstate-dev:127literalgithub-fargate-dev/terraform.tfstatecleanup-staging.ymldestroy-aws-lambda:106literalaws-tfstate-staging:137literalgithub-staging/terraform.tfstatecleanup-staging.ymldestroy-aws-fargate:191literalaws-fargate-tfstate-staging:222literalgithub-fargate-staging/terraform.tfstatecleanup-staging.ymldestroy-gcp:345literalgcp-tfstate-staging:379literalprefix = "github-staging"rollback.ymlrollback-aws-lambda:186aws-tfstate-${{ inputs.environment }}:240ENVIRONMENT: inputs.environment->:244key = "github-%s/terraform.tfstate"rollback.ymlrollback-aws-fargate:288aws-tfstate-${{ inputs.environment }}:342ENVIRONMENT: inputs.environment->:346key = "github-%s/terraform.tfstate"rollback.ymlrollback-gcp:368gcp-tfstate-${{ inputs.environment }}:412ENVIRONMENT: inputs.environment->:417prefix = "github-%s"That is 10 AWS/GCP state-mutating jobs, every one of them previously ungrouped.
rollback-aws-fargatetakes the Lambda group on purposeDespite its name, that job's backend key is
github-<env>/terraform.tfstate, the Lambda namespace, notgithub-fargate-<env>/. Giving itaws-fargate-tfstate-*would have serialized it against a state file it never touches while leaving the one it does write unguarded, which is exactly this bug reproduced in a new place. So the group names the object the job actually locks.The underlying namespace mismatch is a real pre-existing defect (a Fargate rollback applies
compute_platform=fargateinto the Lambda state). It is filed as #1811 rather than changed here, because moving the key changes which infrastructure a rollback rewrites and may strand resources already written to the wrong file. A comment at the job says the group moves when #1811 lands.Workflow-level
concurrencyremoved from all three deploy workflowsdeploy-lambda-${{ github.ref }},deploy-fargate-${{ github.ref }}anddeploy-gcp-${{ github.ref }}are deleted, replaced by a comment saying why the level had to change. Keeping them would imply a state guard they never provided, and they group runs the job-level key deliberately does not (aproddispatch and amainpush write different state files and have no reason to queue behind each other). Same call as #1803 made for Azure. The image build is unaffected: tags derive from the git commit, so distinct refs produce distinct tags and same-ref runs were never racing on a tag.deploy-all.ymlgets no group, only a commentIt reaches AWS and GCP state exclusively through
uses:, and the called workflow's deploying job runs as a real job of that run and is serialized there. A group on the caller job would deadlock: the caller holds the group while waiting on the inner job queued behind it. #1803 documented this fordeploy-azure; the note is generalized to all four callers.Half 2: four lock deletions removed, one retained
deploy-gcp.ymlgsutil rmof.tflockinsideTerraform Initdeploy-gcp.ymlRelease state lock on failure,if: failure() || cancelled()deploy-aws-lambda.ymlRelease state lock on failure,if: failure() || cancelled()destroy-fargate-dev.ymlaws s3 rmof.tflockinsideTerraform Initdeploy-aws-fargate.ymlClear stale state lock (operator-triggered only),if: inputs.clear_stale_lock == trueDeleted rather than made conditional, per #1803's decisive argument:
cancelled()steps run whileterraform applyis still gracefully shutting down and may still be writing state, so the step destroys a lock the dying run is still using. Thefailure()half is no better: a run that failed because it could not acquire the lock deletes the lock held by the run that is still applying. Neither step checked age, and neither checked the lock was this run's. A loudError acquiring the state lockis the correct outcome of a real collision.The two unconditional pre-
initdeletions were worse than either: they fired on every run regardless of outcome, so any second writer wiped the first's live lock before even trying to acquire one.The retained step is justified:
clear_stale_lockis aworkflow_dispatch-only boolean defaulting tofalse, undeclared on theworkflow_callpath (so it renders null and the== trueguard is false). It only fires when an operator explicitly asks for it, having confirmed the owning run is dead. That is the operator-confirmed recovery shape this issue asks for, not the blanket delete it asks to remove. It also gives the four deleted steps somewhere to point: every replacement comment namesterraform force-unlock <ID>andrunbooks/terraform-stuck-lock.md.Trade accepted, stated plainly: a Terraform crash that strands a lock now wedges the pipeline until someone runs
force-unlock, instead of the next run silently clearing it. That is the point.Verification
actionlinton all 7 touched files, this branchactionlinton the same 7 files atorigin/maindiffof the two, normalized to stripfile:line:colprefixes and source guttersSC2086/SC2129info/style note on steps this PR does not touchconcurrencyin a scratch copy ofdeploy-gcp.ymlcontext "needs" is not allowed here. available contexts are "github", "inputs", "vars"pre-commit run --files <7 files>actionlint is not wired into CI or pre-commit, so it gates nothing either way.
The negative control is what makes the clean run evidence rather than silence: it proves actionlint actually enforces context availability for
concurrency, so a clean result on the job-level form means the expression resolves, not that nothing was checked.The group suffix can never be empty, established by execution
An empty suffix would collapse every run into one group, a new bug. A script extracts each
preparejob'sset-envscript verbatim from the committed YAML and executes it under 12 input combinations, 36 executions across the three deploy workflows:deploy-aws-fargate.ymlis stricter still: it has nopushorreleasetrigger, so every empty-input row exits 1.What guarantees this is the
dev|staging|prodallowlist underset -euo pipefail, not the default arms. Empty string does not match, falls to*), and exits 1. Each deploying job then declaresneeds: preparewith noif:(verified programmatically across all 13 state-mutating jobs, printed alongside the group), and a job whoseneedsfailed is skipped, so it never dispatches and never occupies a group. Job-levelconcurrencyis evaluated at dispatch, afterneedsresolves, so there is no ordering in which an empty value claims a group.The three
rollback.ymljobs and the four literal-suffix jobs cannot be empty by construction:inputs.environmentis a requiredchoiceofdev|staging|prodonrollback.yml's only trigger (workflow_dispatch, noworkflow_call), and the rest are literals.One vocabulary across writers
Groups only serialize against each other if the same environment produces the identical string. All ten AWS/GCP suffixes resolve to exactly
dev,stagingorprod: fromprepare's runtime allowlist (deploy workflows, needed becauseworkflow_callinputs are free-form strings), from atype: choice(rollback.yml), or from a literal (cleanup-staging.yml,destroy-fargate-dev.yml). Nodevelopment/devdivergence is reachable.What remains unproven
mainexercisesaws-tfstate-devandgcp-tfstate-dev.deploy-aws-fargate.yml,destroy-fargate-dev.yml,cleanup-staging.ymlandrollback.ymlare all dispatch-only and will not be touched by CI on this PR. The first real cross-workflow collision after merge is the actual test.cancel-in-progress: falseprotects the running job, not the pending one. Arollback-aws-lambdaqueued behind an in-flight prod deploy is cancelled if another deploy queues into the same group.rollback.yml's summary job exits 1 on any non-success, so that surfaces;cleanup-staging.ymlanddestroy-fargate-dev.ymlhave no aggregator, so an evicted destroy shows only as a cancelled job. Inherent to sharing one group across workflows, and still better than concurrent writers.environment:bindings is untested. Nine of the ten grouped jobs are bound to deployment environments (all butdeploy-gcp.yml'sbuild-and-deploy). If required-reviewer rules are ever configured (per sec(ci): deployment environments have no protection rules, so environment-bound credentialed jobs run unapproved #1660, none exist today), it is unclear whether a job parked awaiting approval holds its group. If it does, an unapproved rollback blocks deploys to that environment for the approval window. Same unknown fix(ci/azure): serialize Terraform state writers on environment, not branch #1803 recorded; not resolvable from the docs and not tested.aws-tfstate-*,aws-fargate-tfstate-*andgcp-tfstate-*literals are matched by convention. A future writer added without the group would silently not serialize, the same class of silent failure as the original bug. Nothing enforces the shared azure-tfstate-* concurrency group across the three state writers #1807 already tracks this for Azure; it now covers three more prefixes.terraform planalso takes a state lock, so plan-only paths could in principle contend. None exist here: every workflow that plans also applies, in the same job, inside the same group. Noted rather than assumed away.printftemplates, not from the buckets.TF_BACKEND_*are secrets and were not resolved.Follow-up filed
rollback-aws-fargatebuilds its backend key from the Lambda namespace, so a Fargate rollback applies into the Lambda state. Pre-existing; this PR keys its group on the object it actually locks and leaves the key alone.Summary by CodeRabbit
Improvements
Operational Changes