Skip to content

Commit bde563e

Browse files
authored
fix(ci): serialize AWS and GCP Terraform state writers on environment, not branch (#1812)
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
1 parent 17a568f commit bde563e

7 files changed

Lines changed: 170 additions & 45 deletions

File tree

‎.github/workflows/cleanup-staging.yml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,13 @@ jobs:
9898
name: Destroy AWS Lambda (staging)
9999
runs-on: ubuntu-24.04-arm
100100
needs: guard
101+
# Shares s3://<bucket>/github-staging/terraform.tfstate with
102+
# deploy-aws-lambda.yml and rollback.yml, so it shares their concurrency
103+
# group: one writer per state file, whatever workflow or ref it came from
104+
# (#1806). The suffix is a literal because this job's state key is too.
105+
concurrency:
106+
group: aws-tfstate-staging
107+
cancel-in-progress: false
101108
permissions:
102109
id-token: write
103110
contents: read
@@ -175,6 +182,14 @@ jobs:
175182
name: Destroy AWS Fargate (staging)
176183
runs-on: ubuntu-24.04-arm
177184
needs: guard
185+
# Shares s3://<bucket>/github-fargate-staging/terraform.tfstate with
186+
# deploy-aws-fargate.yml, so it shares that workflow's concurrency group
187+
# (#1806). This is a different state object from destroy-aws-lambda's
188+
# above, so the two jobs are deliberately in different groups and may run
189+
# concurrently. The suffix is a literal because this job's state key is too.
190+
concurrency:
191+
group: aws-fargate-tfstate-staging
192+
cancel-in-progress: false
178193
permissions:
179194
id-token: write
180195
contents: read
@@ -322,6 +337,13 @@ jobs:
322337
name: Destroy GCP (staging)
323338
runs-on: ubuntu-latest
324339
needs: guard
340+
# Shares gs://<bucket>/github-staging/default.tfstate with deploy-gcp.yml
341+
# and rollback.yml, so it shares their concurrency group (#1806). This job
342+
# writes state twice: the `terraform state rm` calls below as well as the
343+
# destroy. The suffix is a literal because this job's backend prefix is too.
344+
concurrency:
345+
group: gcp-tfstate-staging
346+
cancel-in-progress: false
325347
permissions:
326348
id-token: write
327349
contents: read

‎.github/workflows/deploy-all.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,14 @@ on:
5050
release:
5151
types: [created]
5252

53+
# None of the four `uses:` caller jobs below carries a `concurrency` group, and
54+
# that is deliberate (#1801, #1806). The Terraform state guard is the group on
55+
# each called workflow's own deploying job, which runs as a real job of THIS run
56+
# and is serialized there. Putting the same group on a caller job would deadlock:
57+
# the caller would hold the group while waiting on the inner job queued behind
58+
# it. GitHub documents the sibling hazard for `cancel-in-progress: true` (sharing
59+
# a group between caller and called cancels the already-running caller); at
60+
# `false` it stalls instead. The per-job note on `deploy-azure` spells this out.
5361
jobs:
5462
# Determine deployment strategy
5563
determine-deployment:

‎.github/workflows/deploy-aws-fargate.yml‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,13 @@ permissions:
2525
id-token: write
2626
contents: read
2727

28-
concurrency:
29-
group: deploy-fargate-${{ github.ref }}
30-
cancel-in-progress: false
31-
28+
# No workflow-level `concurrency` on purpose. What needs protecting is the
29+
# Terraform state object, which is keyed on the environment, and only the
30+
# job-level key can see `needs.prepare.outputs.environment` -- this level is
31+
# limited to the `github`, `inputs` and `vars` contexts. Keying it on
32+
# `github.ref` put two runs on different refs that both resolve to the same
33+
# environment into different groups against one state file (#1806). The group
34+
# lives on `deploy` below.
3235
on:
3336
workflow_dispatch:
3437
inputs:
@@ -113,6 +116,21 @@ jobs:
113116
name: Deploy to Fargate
114117
runs-on: ubuntu-24.04-arm
115118
needs: prepare
119+
# Every run that writes
120+
# s3://<bucket>/github-fargate-<environment>/terraform.tfstate serializes
121+
# here, whatever ref or workflow it came from (#1806). This is a DIFFERENT
122+
# state object from the Lambda one (github-<environment>/), so it takes its
123+
# own group rather than sharing `aws-tfstate-*`; sharing would serialize two
124+
# independent state files against each other for no gain. The suffix is the
125+
# exact value the backend key below is built from, and `prepare` fails the
126+
# run on anything outside dev|staging|prod so it can never be empty.
127+
#
128+
# `cancel-in-progress: false` is stated rather than left to the default:
129+
# cancelling mid-`terraform apply` is how you get a half-applied stack and a
130+
# lock nobody releases.
131+
concurrency:
132+
group: aws-fargate-tfstate-${{ needs.prepare.outputs.environment }}
133+
cancel-in-progress: false
116134
outputs:
117135
alb_url: ${{ steps.outputs.outputs.alb_url }}
118136
service_name: ${{ steps.outputs.outputs.service_name }}

‎.github/workflows/deploy-aws-lambda.yml‎

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,13 @@ name: Deploy to AWS Lambda
3030
permissions:
3131
contents: read
3232

33-
concurrency:
34-
group: deploy-lambda-${{ github.ref }}
35-
cancel-in-progress: false
36-
33+
# No workflow-level `concurrency` on purpose. What needs protecting is the
34+
# Terraform state object, which is keyed on the environment, and only the
35+
# job-level key can see `needs.prepare.outputs.target_environment` -- this level
36+
# is limited to the `github`, `inputs` and `vars` contexts. Keying it on
37+
# `github.ref` put two runs on different refs that both resolve to the same
38+
# environment into different groups against one state file (#1806). The group
39+
# lives on `build-and-deploy` below.
3740
on:
3841
push:
3942
branches: [main]
@@ -206,6 +209,18 @@ jobs:
206209
# change none of this repo's Environments have any. See #1648.
207210
id-token: write
208211
contents: read
212+
# Every run that writes s3://<bucket>/github-<environment>/terraform.tfstate
213+
# serializes here, whatever ref or workflow it came from (#1806). The suffix
214+
# is the exact value the backend key below is built from, so the group and
215+
# the state object cannot drift apart, and `prepare` fails the run on
216+
# anything outside dev|staging|prod so it can never be empty.
217+
#
218+
# `cancel-in-progress: false` is stated rather than left to the default:
219+
# cancelling mid-`terraform apply` is how you get a half-applied stack and a
220+
# lock nobody releases.
221+
concurrency:
222+
group: aws-tfstate-${{ needs.prepare.outputs.target_environment }}
223+
cancel-in-progress: false
209224
# Bind to the named GitHub Environment matching the target so
210225
# secrets.* resolve to environment-scoped values when defined,
211226
# falling back to repo-scoped secrets otherwise. Without this,
@@ -280,18 +295,19 @@ jobs:
280295
cd terraform/environments/aws
281296
terraform apply -auto-approve tfplan
282297
283-
- name: Release state lock on failure
284-
if: failure() || cancelled()
285-
env:
286-
TF_BACKEND: ${{ secrets.TF_BACKEND_AWS }}
287-
ENVIRONMENT: ${{ needs.prepare.outputs.target_environment }}
288-
run: |
289-
BUCKET=$(grep -E '^\s*bucket\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2)
290-
if [ -n "$BUCKET" ]; then
291-
LOCK_KEY="github-${ENVIRONMENT}/terraform.tfstate.tflock"
292-
echo "Releasing S3 state lock: s3://${BUCKET}/${LOCK_KEY}"
293-
aws s3 rm "s3://${BUCKET}/${LOCK_KEY}" 2>/dev/null || echo "No lock file found (already clean)"
294-
fi
298+
# No automatic state-lock release on failure. The step that used to live
299+
# here ran on `failure() || cancelled()` with no age check and no check
300+
# that the lock was this run's, so it deleted whatever lock object was
301+
# there. The `cancelled()` half is the decisive one: GitHub runs those
302+
# steps while `terraform apply` is still shutting down and may still be
303+
# writing state, so it destroyed a lock out from under an active writer.
304+
# A run that failed *because* it could not acquire the lock would also
305+
# have deleted the lock held by the run still applying. Removed with
306+
# #1806, matching the shape deploy-aws-fargate.yml already uses.
307+
# Terraform releases its own lock on a clean apply error; a lock that
308+
# survives a run means the run died abnormally, which needs operator
309+
# confirmation that the owning run is dead. Recovery is
310+
# `terraform force-unlock <ID>` -- see runbooks/terraform-stuck-lock.md.
295311

296312
- name: Get Terraform outputs
297313
id: outputs

‎.github/workflows/deploy-gcp.yml‎

Lines changed: 37 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,13 @@ permissions:
2424
id-token: write
2525
contents: read
2626

27-
concurrency:
28-
group: deploy-gcp-${{ github.ref }}
29-
cancel-in-progress: false
30-
27+
# No workflow-level `concurrency` on purpose. What needs protecting is the
28+
# Terraform state object, which is keyed on the environment, and only the
29+
# job-level key can see `needs.prepare.outputs.environment` -- this level is
30+
# limited to the `github`, `inputs` and `vars` contexts. Keying it on
31+
# `github.ref` put two runs on different refs that both resolve to `dev` into
32+
# different groups against one state file (#1806). The group lives on
33+
# `build-and-deploy` below.
3134
on:
3235
push:
3336
branches: [main]
@@ -115,6 +118,18 @@ jobs:
115118
name: Build & Deploy
116119
runs-on: ubuntu-latest
117120
needs: prepare
121+
# Every run that writes gs://<bucket>/github-<environment>/default.tfstate
122+
# serializes here, whatever ref or workflow it came from (#1806). The suffix
123+
# is the exact value the backend prefix below is built from, so the group and
124+
# the state object cannot drift apart, and `prepare` fails the run on
125+
# anything outside dev|staging|prod so it can never be empty.
126+
#
127+
# `cancel-in-progress: false` is stated rather than left to the default:
128+
# cancelling mid-`terraform apply` is how you get a half-applied stack and a
129+
# lock nobody releases.
130+
concurrency:
131+
group: gcp-tfstate-${{ needs.prepare.outputs.environment }}
132+
cancel-in-progress: false
118133
outputs:
119134
service_url: ${{ steps.deploy.outputs.service_url }}
120135

@@ -136,18 +151,20 @@ jobs:
136151
with:
137152
terraform_version: ${{ env.TF_VERSION }}
138153

154+
# This step used to `gsutil rm` the state lock object before every init,
155+
# unconditionally and with no check that the lock was stale or anyone
156+
# else's, so it deleted a live lock held by a concurrent writer. Removed
157+
# with #1806; the job-level `concurrency` group above is what keeps
158+
# writers apart now, and a loud "Error acquiring the state lock" is the
159+
# correct outcome if one ever slips through. Recovery from a genuinely
160+
# stranded lock is `terraform force-unlock <ID>` -- see
161+
# runbooks/terraform-stuck-lock.md.
139162
- name: Terraform Init
140163
env:
141164
TF_BACKEND: ${{ secrets.TF_BACKEND_GCP }}
142165
ENVIRONMENT: ${{ needs.prepare.outputs.environment }}
143166
run: |
144167
printf '%s\nprefix = "github-%s"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend
145-
# Break any stale state lock from a previous failed run
146-
BUCKET=$(grep -E '^\s*bucket\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2)
147-
if [ -n "$BUCKET" ]; then
148-
LOCK_FILE="gs://${BUCKET}/github-${ENVIRONMENT}/default.tflock"
149-
gsutil rm "${LOCK_FILE}" 2>/dev/null || true
150-
fi
151168
cd terraform/environments/gcp
152169
terraform init -backend-config=/tmp/backend.tfbackend
153170
@@ -173,17 +190,16 @@ jobs:
173190
SERVICE_URL=$(terraform output -raw cloud_run_service_url 2>/dev/null | grep -v '::' || echo "")
174191
echo "service_url=$SERVICE_URL" >> $GITHUB_OUTPUT
175192
176-
- name: Release state lock on failure
177-
if: failure() || cancelled()
178-
env:
179-
ENVIRONMENT: ${{ needs.prepare.outputs.environment }}
180-
run: |
181-
BUCKET=$(grep -E '^\s*bucket\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2)
182-
if [ -n "$BUCKET" ]; then
183-
LOCK_FILE="gs://${BUCKET}/github-${ENVIRONMENT}/default.tflock"
184-
echo "Releasing GCS state lock: ${LOCK_FILE}"
185-
gsutil rm "${LOCK_FILE}" 2>/dev/null || echo "No lock file found (already clean)"
186-
fi
193+
# No automatic state-lock release on failure. The step that used to live
194+
# here ran on `failure() || cancelled()` with no age check and no check
195+
# that the lock was this run's, so it deleted whatever lock object was
196+
# there. The `cancelled()` half is the decisive one: GitHub runs those
197+
# steps while `terraform apply` is still shutting down and may still be
198+
# writing state, so it destroyed a lock out from under an active writer.
199+
# Terraform releases its own lock on a clean apply error; a lock that
200+
# survives a run means the run died abnormally, which needs operator
201+
# confirmation that the owning run is dead. Recovery is
202+
# `terraform force-unlock <ID>` -- see runbooks/terraform-stuck-lock.md.
187203

188204
- name: Save deployment info
189205
run: |

‎.github/workflows/destroy-fargate-dev.yml‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,15 @@ jobs:
7575
name: Terraform Destroy (Fargate dev)
7676
runs-on: ubuntu-24.04-arm
7777
needs: guard
78+
# Destroys s3://<bucket>/github-fargate-dev/terraform.tfstate, the same
79+
# state object deploy-aws-fargate.yml and cleanup-staging.yml write, so it
80+
# takes the same concurrency group: one writer per state file, whatever
81+
# workflow or ref it came from (#1806). The suffix is a literal because this
82+
# job's state key is too. `cancel-in-progress: false` because cancelling
83+
# mid-`terraform destroy` leaves a half-destroyed stack and a stuck lock.
84+
concurrency:
85+
group: aws-fargate-tfstate-dev
86+
cancel-in-progress: false
7887
permissions:
7988
id-token: write
8089
contents: read
@@ -103,15 +112,19 @@ jobs:
103112
with:
104113
terraform_version: ${{ env.TF_VERSION }}
105114

115+
# This step used to `aws s3 rm` the state lock object before every init,
116+
# unconditionally and with no check that the lock was stale or anyone
117+
# else's, so it deleted a live lock held by a concurrent writer. Removed
118+
# with #1806; the job-level `concurrency` group above is what keeps
119+
# writers apart now, and a loud "Error acquiring the state lock" is the
120+
# correct outcome if one ever slips through. Recovery from a genuinely
121+
# stranded lock is `terraform force-unlock <ID>` -- see
122+
# runbooks/terraform-stuck-lock.md.
106123
- name: Terraform Init
107124
env:
108125
TF_BACKEND: ${{ secrets.TF_BACKEND_AWS }}
109126
run: |
110127
printf '%s\nkey = "github-fargate-dev/terraform.tfstate"\n' "$TF_BACKEND" > /tmp/backend.tfbackend
111-
BUCKET=$(grep -E '^\s*bucket\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2)
112-
if [ -n "$BUCKET" ]; then
113-
aws s3 rm "s3://${BUCKET}/github-fargate-dev/terraform.tfstate.tflock" 2>/dev/null || true
114-
fi
115128
cd terraform/environments/aws
116129
terraform init -backend-config=/tmp/backend.tfbackend
117130

‎.github/workflows/rollback.yml‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,16 @@ jobs:
175175
timeout-minutes: 30
176176
needs: validate
177177
if: inputs.cloud == 'aws-lambda'
178+
# `terraform apply` against s3://<bucket>/github-<environment>/terraform.tfstate,
179+
# the same object deploy-aws-lambda.yml and cleanup-staging.yml write, so it
180+
# takes the same concurrency group (#1806). `inputs.environment` is a
181+
# required `choice` constrained to dev|staging|prod, so the suffix is never
182+
# empty; it is the same value this job interpolates into the state key below.
183+
# The eviction and approval-gate caveats documented on `rollback-azure` apply
184+
# here identically.
185+
concurrency:
186+
group: aws-tfstate-${{ inputs.environment }}
187+
cancel-in-progress: false
178188
permissions:
179189
id-token: write
180190
contents: read
@@ -263,6 +273,20 @@ jobs:
263273
timeout-minutes: 30
264274
needs: validate
265275
if: inputs.cloud == 'aws-fargate'
276+
# NOTE the group is `aws-tfstate-*`, NOT `aws-fargate-tfstate-*`. Despite
277+
# the job name, the state key this job builds below is
278+
# `github-<environment>/terraform.tfstate` -- the LAMBDA namespace -- not
279+
# `github-fargate-<environment>/` as deploy-aws-fargate.yml uses. The group
280+
# must name the object this job actually locks, or it would serialize
281+
# against a state file it never touches while writing one unguarded, which
282+
# is #1806 reproduced in a new place. That namespace mismatch is a real
283+
# pre-existing defect (a Fargate rollback applies into the Lambda state),
284+
# tracked in #1811 rather than changed here, because moving the key changes
285+
# which infrastructure a rollback rewrites. When #1811 lands, this group
286+
# moves to `aws-fargate-tfstate-*` in the same commit as the key.
287+
concurrency:
288+
group: aws-tfstate-${{ inputs.environment }}
289+
cancel-in-progress: false
266290
permissions:
267291
id-token: write
268292
contents: read
@@ -335,6 +359,14 @@ jobs:
335359
timeout-minutes: 30
336360
needs: validate
337361
if: inputs.cloud == 'gcp'
362+
# `terraform apply` against gs://<bucket>/github-<environment>/default.tfstate,
363+
# the same object deploy-gcp.yml and cleanup-staging.yml write, so it takes
364+
# the same concurrency group (#1806). `inputs.environment` is a required
365+
# `choice` constrained to dev|staging|prod, so the suffix is never empty; it
366+
# is the same value this job interpolates into the backend prefix below.
367+
concurrency:
368+
group: gcp-tfstate-${{ inputs.environment }}
369+
cancel-in-progress: false
338370
permissions:
339371
id-token: write
340372
contents: read

0 commit comments

Comments
 (0)