From c3818fe2d7ca314dc2758f12887f11ab021b464d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 13 Aug 2026 00:15:13 +0200 Subject: [PATCH] fix(ci): serialize AWS and GCP Terraform state writers on environment, 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- github-/terraform.tfstate (S3) aws-fargate-tfstate- github-fargate-/terraform.tfstate (S3) gcp-tfstate- github-/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 --- .github/workflows/cleanup-staging.yml | 22 +++++++++ .github/workflows/deploy-all.yml | 8 ++++ .github/workflows/deploy-aws-fargate.yml | 26 ++++++++-- .github/workflows/deploy-aws-lambda.yml | 48 ++++++++++++------- .github/workflows/deploy-gcp.yml | 58 +++++++++++++++-------- .github/workflows/destroy-fargate-dev.yml | 21 ++++++-- .github/workflows/rollback.yml | 32 +++++++++++++ 7 files changed, 170 insertions(+), 45 deletions(-) diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index d7013b3e1..06ab63be6 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -98,6 +98,13 @@ jobs: name: Destroy AWS Lambda (staging) runs-on: ubuntu-24.04-arm needs: guard + # Shares s3:///github-staging/terraform.tfstate with + # deploy-aws-lambda.yml and rollback.yml, so it shares their concurrency + # group: one writer per state file, whatever workflow or ref it came from + # (#1806). The suffix is a literal because this job's state key is too. + concurrency: + group: aws-tfstate-staging + cancel-in-progress: false permissions: id-token: write contents: read @@ -175,6 +182,14 @@ jobs: name: Destroy AWS Fargate (staging) runs-on: ubuntu-24.04-arm needs: guard + # Shares s3:///github-fargate-staging/terraform.tfstate with + # deploy-aws-fargate.yml, so it shares that workflow's concurrency group + # (#1806). This is a different state object from destroy-aws-lambda's + # above, so the two jobs are deliberately in different groups and may run + # concurrently. The suffix is a literal because this job's state key is too. + concurrency: + group: aws-fargate-tfstate-staging + cancel-in-progress: false permissions: id-token: write contents: read @@ -322,6 +337,13 @@ jobs: name: Destroy GCP (staging) runs-on: ubuntu-latest needs: guard + # Shares gs:///github-staging/default.tfstate with deploy-gcp.yml + # and rollback.yml, so it shares their concurrency group (#1806). This job + # writes state twice: the `terraform state rm` calls below as well as the + # destroy. The suffix is a literal because this job's backend prefix is too. + concurrency: + group: gcp-tfstate-staging + cancel-in-progress: false permissions: id-token: write contents: read diff --git a/.github/workflows/deploy-all.yml b/.github/workflows/deploy-all.yml index 6f9a8a8a6..daeeda0db 100644 --- a/.github/workflows/deploy-all.yml +++ b/.github/workflows/deploy-all.yml @@ -50,6 +50,14 @@ on: release: types: [created] +# None of the four `uses:` caller jobs below carries a `concurrency` group, and +# that is deliberate (#1801, #1806). The Terraform state guard is the group on +# each called workflow's own deploying job, which runs as a real job of THIS run +# and is serialized there. Putting the same group on a caller job would deadlock: +# the caller would hold the group while waiting on the inner job queued behind +# it. GitHub documents the sibling hazard for `cancel-in-progress: true` (sharing +# a group between caller and called cancels the already-running caller); at +# `false` it stalls instead. The per-job note on `deploy-azure` spells this out. jobs: # Determine deployment strategy determine-deployment: diff --git a/.github/workflows/deploy-aws-fargate.yml b/.github/workflows/deploy-aws-fargate.yml index bcee839ea..99b07a580 100644 --- a/.github/workflows/deploy-aws-fargate.yml +++ b/.github/workflows/deploy-aws-fargate.yml @@ -25,10 +25,13 @@ permissions: id-token: write contents: read -concurrency: - group: deploy-fargate-${{ github.ref }} - cancel-in-progress: false - +# No workflow-level `concurrency` on purpose. What needs protecting is the +# Terraform state object, which is keyed on the environment, and only the +# job-level key can see `needs.prepare.outputs.environment` -- this level is +# limited to the `github`, `inputs` and `vars` contexts. Keying it on +# `github.ref` put two runs on different refs that both resolve to the same +# environment into different groups against one state file (#1806). The group +# lives on `deploy` below. on: workflow_dispatch: inputs: @@ -113,6 +116,21 @@ jobs: name: Deploy to Fargate runs-on: ubuntu-24.04-arm needs: prepare + # Every run that writes + # s3:///github-fargate-/terraform.tfstate serializes + # here, whatever ref or workflow it came from (#1806). This is a DIFFERENT + # state object from the Lambda one (github-/), so it takes its + # own group rather than sharing `aws-tfstate-*`; sharing would serialize two + # independent state files against each other for no gain. The suffix is the + # exact value the backend key below is built from, and `prepare` fails the + # run on anything outside dev|staging|prod so it can never be empty. + # + # `cancel-in-progress: false` is stated rather than left to the default: + # cancelling mid-`terraform apply` is how you get a half-applied stack and a + # lock nobody releases. + concurrency: + group: aws-fargate-tfstate-${{ needs.prepare.outputs.environment }} + cancel-in-progress: false outputs: alb_url: ${{ steps.outputs.outputs.alb_url }} service_name: ${{ steps.outputs.outputs.service_name }} diff --git a/.github/workflows/deploy-aws-lambda.yml b/.github/workflows/deploy-aws-lambda.yml index bccf4423f..8c177e033 100644 --- a/.github/workflows/deploy-aws-lambda.yml +++ b/.github/workflows/deploy-aws-lambda.yml @@ -30,10 +30,13 @@ name: Deploy to AWS Lambda permissions: contents: read -concurrency: - group: deploy-lambda-${{ github.ref }} - cancel-in-progress: false - +# No workflow-level `concurrency` on purpose. What needs protecting is the +# Terraform state object, which is keyed on the environment, and only the +# job-level key can see `needs.prepare.outputs.target_environment` -- this level +# is limited to the `github`, `inputs` and `vars` contexts. Keying it on +# `github.ref` put two runs on different refs that both resolve to the same +# environment into different groups against one state file (#1806). The group +# lives on `build-and-deploy` below. on: push: branches: [main] @@ -206,6 +209,18 @@ jobs: # change none of this repo's Environments have any. See #1648. id-token: write contents: read + # Every run that writes s3:///github-/terraform.tfstate + # serializes here, whatever ref or workflow it came from (#1806). The suffix + # is the exact value the backend key below is built from, so the group and + # the state object cannot drift apart, and `prepare` fails the run on + # anything outside dev|staging|prod so it can never be empty. + # + # `cancel-in-progress: false` is stated rather than left to the default: + # cancelling mid-`terraform apply` is how you get a half-applied stack and a + # lock nobody releases. + concurrency: + group: aws-tfstate-${{ needs.prepare.outputs.target_environment }} + cancel-in-progress: false # Bind to the named GitHub Environment matching the target so # secrets.* resolve to environment-scoped values when defined, # falling back to repo-scoped secrets otherwise. Without this, @@ -280,18 +295,19 @@ jobs: cd terraform/environments/aws terraform apply -auto-approve tfplan - - name: Release state lock on failure - if: failure() || cancelled() - env: - TF_BACKEND: ${{ secrets.TF_BACKEND_AWS }} - ENVIRONMENT: ${{ needs.prepare.outputs.target_environment }} - run: | - BUCKET=$(grep -E '^\s*bucket\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - if [ -n "$BUCKET" ]; then - LOCK_KEY="github-${ENVIRONMENT}/terraform.tfstate.tflock" - echo "Releasing S3 state lock: s3://${BUCKET}/${LOCK_KEY}" - aws s3 rm "s3://${BUCKET}/${LOCK_KEY}" 2>/dev/null || echo "No lock file found (already clean)" - fi + # No automatic state-lock release on failure. The step that used to live + # here ran on `failure() || cancelled()` with no age check and no check + # that the lock was this run's, so it deleted whatever lock object was + # there. The `cancelled()` half is the decisive one: GitHub runs those + # steps while `terraform apply` is still shutting down and may still be + # writing state, so it destroyed a lock out from under an active writer. + # A run that failed *because* it could not acquire the lock would also + # have deleted the lock held by the run still applying. Removed with + # #1806, matching the shape deploy-aws-fargate.yml already uses. + # Terraform releases its own lock on a clean apply error; a lock that + # survives a run means the run died abnormally, which needs operator + # confirmation that the owning run is dead. Recovery is + # `terraform force-unlock ` -- see runbooks/terraform-stuck-lock.md. - name: Get Terraform outputs id: outputs diff --git a/.github/workflows/deploy-gcp.yml b/.github/workflows/deploy-gcp.yml index 0a327f5b5..9432c394a 100644 --- a/.github/workflows/deploy-gcp.yml +++ b/.github/workflows/deploy-gcp.yml @@ -24,10 +24,13 @@ permissions: id-token: write contents: read -concurrency: - group: deploy-gcp-${{ github.ref }} - cancel-in-progress: false - +# No workflow-level `concurrency` on purpose. What needs protecting is the +# Terraform state object, which is keyed on the environment, and only the +# job-level key can see `needs.prepare.outputs.environment` -- this level is +# limited to the `github`, `inputs` and `vars` contexts. Keying it on +# `github.ref` put two runs on different refs that both resolve to `dev` into +# different groups against one state file (#1806). The group lives on +# `build-and-deploy` below. on: push: branches: [main] @@ -115,6 +118,18 @@ jobs: name: Build & Deploy runs-on: ubuntu-latest needs: prepare + # Every run that writes gs:///github-/default.tfstate + # serializes here, whatever ref or workflow it came from (#1806). The suffix + # is the exact value the backend prefix below is built from, so the group and + # the state object cannot drift apart, and `prepare` fails the run on + # anything outside dev|staging|prod so it can never be empty. + # + # `cancel-in-progress: false` is stated rather than left to the default: + # cancelling mid-`terraform apply` is how you get a half-applied stack and a + # lock nobody releases. + concurrency: + group: gcp-tfstate-${{ needs.prepare.outputs.environment }} + cancel-in-progress: false outputs: service_url: ${{ steps.deploy.outputs.service_url }} @@ -136,18 +151,20 @@ jobs: with: terraform_version: ${{ env.TF_VERSION }} + # This step used to `gsutil rm` the state lock object before every init, + # unconditionally and with no check that the lock was stale or anyone + # else's, so it deleted a live lock held by a concurrent writer. Removed + # with #1806; the job-level `concurrency` group above is what keeps + # writers apart now, and a loud "Error acquiring the state lock" is the + # correct outcome if one ever slips through. Recovery from a genuinely + # stranded lock is `terraform force-unlock ` -- see + # runbooks/terraform-stuck-lock.md. - name: Terraform Init env: TF_BACKEND: ${{ secrets.TF_BACKEND_GCP }} ENVIRONMENT: ${{ needs.prepare.outputs.environment }} run: | printf '%s\nprefix = "github-%s"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend - # Break any stale state lock from a previous failed run - BUCKET=$(grep -E '^\s*bucket\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - if [ -n "$BUCKET" ]; then - LOCK_FILE="gs://${BUCKET}/github-${ENVIRONMENT}/default.tflock" - gsutil rm "${LOCK_FILE}" 2>/dev/null || true - fi cd terraform/environments/gcp terraform init -backend-config=/tmp/backend.tfbackend @@ -173,17 +190,16 @@ jobs: SERVICE_URL=$(terraform output -raw cloud_run_service_url 2>/dev/null | grep -v '::' || echo "") echo "service_url=$SERVICE_URL" >> $GITHUB_OUTPUT - - name: Release state lock on failure - if: failure() || cancelled() - env: - ENVIRONMENT: ${{ needs.prepare.outputs.environment }} - run: | - BUCKET=$(grep -E '^\s*bucket\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - if [ -n "$BUCKET" ]; then - LOCK_FILE="gs://${BUCKET}/github-${ENVIRONMENT}/default.tflock" - echo "Releasing GCS state lock: ${LOCK_FILE}" - gsutil rm "${LOCK_FILE}" 2>/dev/null || echo "No lock file found (already clean)" - fi + # No automatic state-lock release on failure. The step that used to live + # here ran on `failure() || cancelled()` with no age check and no check + # that the lock was this run's, so it deleted whatever lock object was + # there. The `cancelled()` half is the decisive one: GitHub runs those + # steps while `terraform apply` is still shutting down and may still be + # writing state, so it destroyed a lock out from under an active writer. + # Terraform releases its own lock on a clean apply error; a lock that + # survives a run means the run died abnormally, which needs operator + # confirmation that the owning run is dead. Recovery is + # `terraform force-unlock ` -- see runbooks/terraform-stuck-lock.md. - name: Save deployment info run: | diff --git a/.github/workflows/destroy-fargate-dev.yml b/.github/workflows/destroy-fargate-dev.yml index 030e5ee26..a496befa0 100644 --- a/.github/workflows/destroy-fargate-dev.yml +++ b/.github/workflows/destroy-fargate-dev.yml @@ -75,6 +75,15 @@ jobs: name: Terraform Destroy (Fargate dev) runs-on: ubuntu-24.04-arm needs: guard + # Destroys s3:///github-fargate-dev/terraform.tfstate, the same + # state object deploy-aws-fargate.yml and cleanup-staging.yml write, so it + # takes the same concurrency group: one writer per state file, whatever + # workflow or ref it came from (#1806). The suffix is a literal because this + # job's state key is too. `cancel-in-progress: false` because cancelling + # mid-`terraform destroy` leaves a half-destroyed stack and a stuck lock. + concurrency: + group: aws-fargate-tfstate-dev + cancel-in-progress: false permissions: id-token: write contents: read @@ -103,15 +112,19 @@ jobs: with: terraform_version: ${{ env.TF_VERSION }} + # This step used to `aws s3 rm` the state lock object before every init, + # unconditionally and with no check that the lock was stale or anyone + # else's, so it deleted a live lock held by a concurrent writer. Removed + # with #1806; the job-level `concurrency` group above is what keeps + # writers apart now, and a loud "Error acquiring the state lock" is the + # correct outcome if one ever slips through. Recovery from a genuinely + # stranded lock is `terraform force-unlock ` -- see + # runbooks/terraform-stuck-lock.md. - name: Terraform Init env: TF_BACKEND: ${{ secrets.TF_BACKEND_AWS }} run: | printf '%s\nkey = "github-fargate-dev/terraform.tfstate"\n' "$TF_BACKEND" > /tmp/backend.tfbackend - BUCKET=$(grep -E '^\s*bucket\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - if [ -n "$BUCKET" ]; then - aws s3 rm "s3://${BUCKET}/github-fargate-dev/terraform.tfstate.tflock" 2>/dev/null || true - fi cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend diff --git a/.github/workflows/rollback.yml b/.github/workflows/rollback.yml index f00339480..37f6fd7ad 100644 --- a/.github/workflows/rollback.yml +++ b/.github/workflows/rollback.yml @@ -175,6 +175,16 @@ jobs: timeout-minutes: 30 needs: validate if: inputs.cloud == 'aws-lambda' + # `terraform apply` against s3:///github-/terraform.tfstate, + # the same object deploy-aws-lambda.yml and cleanup-staging.yml write, so it + # takes the same concurrency group (#1806). `inputs.environment` is a + # required `choice` constrained to dev|staging|prod, so the suffix is never + # empty; it is the same value this job interpolates into the state key below. + # The eviction and approval-gate caveats documented on `rollback-azure` apply + # here identically. + concurrency: + group: aws-tfstate-${{ inputs.environment }} + cancel-in-progress: false permissions: id-token: write contents: read @@ -263,6 +273,20 @@ jobs: timeout-minutes: 30 needs: validate if: inputs.cloud == 'aws-fargate' + # NOTE the group is `aws-tfstate-*`, NOT `aws-fargate-tfstate-*`. Despite + # the job name, the state key this job builds below is + # `github-/terraform.tfstate` -- the LAMBDA namespace -- not + # `github-fargate-/` as deploy-aws-fargate.yml uses. The group + # must name the object this job actually locks, or it would serialize + # against a state file it never touches while writing one unguarded, which + # is #1806 reproduced in a new place. That namespace mismatch is a real + # pre-existing defect (a Fargate rollback applies into the Lambda state), + # tracked in #1811 rather than changed here, because moving the key changes + # which infrastructure a rollback rewrites. When #1811 lands, this group + # moves to `aws-fargate-tfstate-*` in the same commit as the key. + concurrency: + group: aws-tfstate-${{ inputs.environment }} + cancel-in-progress: false permissions: id-token: write contents: read @@ -335,6 +359,14 @@ jobs: timeout-minutes: 30 needs: validate if: inputs.cloud == 'gcp' + # `terraform apply` against gs:///github-/default.tfstate, + # the same object deploy-gcp.yml and cleanup-staging.yml write, so it takes + # the same concurrency group (#1806). `inputs.environment` is a required + # `choice` constrained to dev|staging|prod, so the suffix is never empty; it + # is the same value this job interpolates into the backend prefix below. + concurrency: + group: gcp-tfstate-${{ inputs.environment }} + cancel-in-progress: false permissions: id-token: write contents: read