diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index c6ef424f5..d7013b3e1 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -252,6 +252,13 @@ jobs: name: Destroy Azure (staging) runs-on: ubuntu-latest needs: guard + # Shares the Azure Terraform state blob github-staging.terraform.tfstate + # with deploy-azure.yml and rollback.yml, so it shares their concurrency + # group: one writer per state file, whatever workflow or ref it came from + # (#1801). The suffix is a literal because this job's state key is too. + concurrency: + group: azure-tfstate-staging + cancel-in-progress: false permissions: id-token: write contents: read @@ -284,28 +291,16 @@ jobs: with: terraform_version: ${{ env.TF_VERSION }} - - name: Break stale Azure state lock + # This step used to also break the blob lease on the state file, with no + # age check and no --lease-break-period, so it broke a live lease held by + # a concurrent writer. Removed with #1801; 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. + - name: Write Terraform backend config (staging state) env: TF_BACKEND: ${{ secrets.TF_BACKEND_AZURE }} run: | printf '%s\nkey = "github-staging.terraform.tfstate"\n' "$TF_BACKEND" > /tmp/backend.tfbackend - STORAGE_ACCOUNT=$(grep -E '^\s*storage_account_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - CONTAINER=$(grep -E '^\s*container_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - RESOURCE_GROUP=$(grep -E '^\s*resource_group_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - STATE_KEY="github-staging.terraform.tfstate" - if [ -n "$STORAGE_ACCOUNT" ] && [ -n "$CONTAINER" ]; then - ACCOUNT_KEY=$(az storage account keys list \ - --account-name "${STORAGE_ACCOUNT}" \ - ${RESOURCE_GROUP:+--resource-group "${RESOURCE_GROUP}"} \ - --query '[0].value' -o tsv 2>/dev/null || echo "") - if [ -n "$ACCOUNT_KEY" ]; then - az storage blob lease break \ - --account-name "${STORAGE_ACCOUNT}" \ - --container-name "${CONTAINER}" \ - --blob-name "${STATE_KEY}" \ - --account-key "${ACCOUNT_KEY}" 2>/dev/null || echo "No lease to break (clean)" - fi - fi - name: Terraform Init (staging state) run: | diff --git a/.github/workflows/deploy-all.yml b/.github/workflows/deploy-all.yml index 81b1ee9c7..bf79df9ac 100644 --- a/.github/workflows/deploy-all.yml +++ b/.github/workflows/deploy-all.yml @@ -162,6 +162,18 @@ jobs: TF_BACKEND_GCP: ${{ secrets.TF_BACKEND_GCP }} # Deploy to Azure Container Apps + # + # Deliberately carries no `concurrency` (#1801). The Terraform state guard is + # the group on the called workflow's own build-and-deploy job, which runs as a + # real job of this run and is serialized there. That group is derived from the + # called workflow's own `prepare` output, which is not necessarily the + # `environment` computed above, so do not assume the two agree. + # + # Putting that same group on this caller job would deadlock: this job 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. deploy-azure: name: Deploy Azure Container Apps needs: determine-deployment diff --git a/.github/workflows/deploy-azure.yml b/.github/workflows/deploy-azure.yml index 387e4b7c3..f47f7b30a 100644 --- a/.github/workflows/deploy-azure.yml +++ b/.github/workflows/deploy-azure.yml @@ -26,9 +26,13 @@ permissions: id-token: write contents: read -concurrency: - group: deploy-azure-${{ github.ref }} - cancel-in-progress: false +# No workflow-level `concurrency` on purpose. What needs protecting is the +# Terraform state blob, 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` here was issue #1801: two refs resolving to the same environment +# landed in different groups and applied against one state file. See +# `build-and-deploy` below. on: push: @@ -104,6 +108,21 @@ jobs: name: Build & Deploy runs-on: ubuntu-latest needs: prepare + # Every run that writes github-.terraform.tfstate serializes + # here, whatever its ref and whatever workflow it came from. The group name + # is a 1:1 function of the state key, and the same literal prefix is used by + # cleanup-staging.yml and rollback.yml, which write the same blob. + # + # `prepare` rejects anything outside dev|staging|prod and fails the run, so + # this expression cannot evaluate to an empty suffix while this job runs -- + # that would collapse every environment into one group. + # + # cancel-in-progress stays false, stated explicitly rather than left to the + # default: cancelling mid-`terraform apply` is how you get a half-applied + # stack and a lease nobody releases. + concurrency: + group: azure-tfstate-${{ needs.prepare.outputs.environment }} + cancel-in-progress: false outputs: app_url: ${{ steps.deploy.outputs.app_url }} app_name: ${{ steps.deploy.outputs.app_name }} @@ -129,30 +148,19 @@ jobs: with: terraform_version: ${{ env.TF_VERSION }} - - name: Break stale state lock (if any) + # This step used to also run `az storage blob lease break` on the state + # blob, with no lease-age check and no --lease-break-period, so it broke a + # *live* lease immediately. Removed with #1801: the lease is the last line + # of defence against two writers, and a loud "Error acquiring the state + # lock" is the correct outcome of a real collision. A genuinely stranded + # lease (runner killed mid-apply) is recovered with `terraform + # force-unlock `, using the lock ID Terraform prints in that error. + - name: Write Terraform backend config env: TF_BACKEND: ${{ secrets.TF_BACKEND_AZURE }} ENVIRONMENT: ${{ needs.prepare.outputs.environment }} run: | printf '%s\nkey = "github-%s.terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend - STORAGE_ACCOUNT=$(grep -E '^\s*storage_account_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - CONTAINER=$(grep -E '^\s*container_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - RESOURCE_GROUP=$(grep -E '^\s*resource_group_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - if [ -n "$STORAGE_ACCOUNT" ] && [ -n "$CONTAINER" ]; then - STATE_KEY="github-${ENVIRONMENT}.terraform.tfstate" - ACCOUNT_KEY=$(az storage account keys list \ - --account-name "${STORAGE_ACCOUNT}" \ - ${RESOURCE_GROUP:+--resource-group "${RESOURCE_GROUP}"} \ - --query '[0].value' -o tsv 2>/dev/null || echo "") - if [ -n "$ACCOUNT_KEY" ]; then - echo "Breaking any stale blob lease on ${STATE_KEY}..." - az storage blob lease break \ - --account-name "${STORAGE_ACCOUNT}" \ - --container-name "${CONTAINER}" \ - --blob-name "${STATE_KEY}" \ - --account-key "${ACCOUNT_KEY}" 2>/dev/null || echo "No lease to break (clean)" - fi - fi - name: Terraform Init run: | @@ -293,31 +301,21 @@ jobs: the bootstrap builds the name from its own var.name_suffix. EOT - - name: Release state lock on failure - if: failure() || cancelled() - env: - ENVIRONMENT: ${{ needs.prepare.outputs.environment }} - run: | - STORAGE_ACCOUNT=$(grep -E '^\s*storage_account_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - CONTAINER=$(grep -E '^\s*container_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - RESOURCE_GROUP=$(grep -E '^\s*resource_group_name\s*=' /tmp/backend.tfbackend 2>/dev/null | tr -d ' "' | cut -d= -f2) - if [ -n "$STORAGE_ACCOUNT" ] && [ -n "$CONTAINER" ]; then - STATE_KEY="github-${ENVIRONMENT}.terraform.tfstate" - ACCOUNT_KEY=$(az storage account keys list \ - --account-name "${STORAGE_ACCOUNT}" \ - ${RESOURCE_GROUP:+--resource-group "${RESOURCE_GROUP}"} \ - --query '[0].value' -o tsv 2>/dev/null || echo "") - if [ -n "$ACCOUNT_KEY" ]; then - echo "Breaking Azure Blob lease on ${STATE_KEY}" - az storage blob lease break \ - --account-name "${STORAGE_ACCOUNT}" \ - --container-name "${CONTAINER}" \ - --blob-name "${STATE_KEY}" \ - --account-key "${ACCOUNT_KEY}" 2>/dev/null || echo "No lease to break (already clean)" - else - echo "WARNING: Could not get storage account key — lease may remain held" - fi - fi + # A "Release state lock on failure" step used to break the lease here on + # `failure() || cancelled()`. Removed with #1801. On failure Terraform + # ordinarily unlocks on its way out, so the step usually did nothing; on + # cancelled() a SIGINT'd apply is still shutting down and may still be + # writing state, so it broke the lease out from under it. That is the + # decisive half: a live lease must never be broken. + # + # The residue is real and is accepted, not denied. A lease IS stranded + # when the unlock call itself fails ("Error releasing the state lock", + # e.g. OIDC expiry mid-apply), when Terraform crashes, or when Actions + # SIGKILLs after the cancellation grace period. The azurerm backend takes + # an INFINITE lease, so none of those self-expire. Recovery is deliberate + # and manual: `terraform force-unlock ` with the ID Terraform prints. + # A step that clears those cases can only do so by also breaking live + # leases, which is the bug this issue is about. - name: Save deployment info run: | diff --git a/.github/workflows/rollback.yml b/.github/workflows/rollback.yml index 0110489c5..f00339480 100644 --- a/.github/workflows/rollback.yml +++ b/.github/workflows/rollback.yml @@ -398,6 +398,25 @@ jobs: timeout-minutes: 30 needs: validate if: inputs.cloud == 'azure' + # `terraform apply` against github-.terraform.tfstate, the same + # blob deploy-azure.yml and cleanup-staging.yml write, so it takes the same + # concurrency group (#1801). `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. + # + # Two consequences of sharing the group across workflows, both accepted as + # better than concurrent writers: + # - GitHub keeps one pending entry per group, so a queued rollback can be + # evicted by a later arrival. That surfaces: the summary job below exits + # 1 on any non-success result, so an evicted rollback reddens the run. + # - if the `environment:` binding below carries required reviewers (see + # #1660 for the live state, deliberately not restated here), it is + # undocumented whether a job parked awaiting approval holds its group. + # If it does, an unapproved rollback blocks deploys to that environment + # for the whole approval window. + concurrency: + group: azure-tfstate-${{ inputs.environment }} + cancel-in-progress: false permissions: id-token: write contents: read