From ef451ad7ebc15ae687ead0efb475ae7939e41af6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 12 Aug 2026 02:27:29 +0200 Subject: [PATCH 1/4] fix(ci/azure): serialize Terraform state writers on environment, not branch The Azure Terraform state blob is keyed on environment (github-.terraform.tfstate), but deploy-azure.yml serialized on branch (deploy-azure-${{ github.ref }}). prepare maps every non-dispatch event to dev, so runs on different refs landed in different concurrency groups, ran simultaneously, and applied against one state file. That collision would have been loud on its own, because the Azure blob lease makes the second writer fail with "Error acquiring the state lock". Three az storage blob lease break invocations removed that safety net: none checked that a lease was held, none checked its age, and none passed --lease-break-period, so the default broke an active lease immediately. The second run broke the first's live lease, both wrote, and the later writer's stale read dropped the earlier one's entries. Two role assignments ended up live in Azure but absent from state, 409ing every subsequent apply until imported by hand. Move the group to the build-and-deploy job, where needs is in scope (workflow-level concurrency sees only github, inputs and vars), and key it on the same value that builds the state key: group: azure-tfstate-${{ needs.prepare.outputs.environment }} Group and state file therefore cannot drift apart. Apply the same group to the other two writers of that blob, cleanup-staging.yml's destroy-azure and rollback.yml's rollback-azure, so serialization holds across workflows rather than within one. deploy-all.yml reaches the state only through deploy-azure.yml, whose job is already serialized, and gets no group of its own: the same group on the caller job would deadlock against the inner job queued behind it. Delete all three lease breaks. The two pre-emptive ones are the defect; each survives as a renamed step that still writes the backend config terraform init consumes. The failure-path one is deleted rather than made conditional because it was a no-op exactly when it was harmless: on failure Terraform has already unlocked on its way out, on cancellation a SIGINT'd apply may still be writing state and the step broke the lease out from under it, and a hard runner death runs no steps at all. cancel-in-progress stays false and is now stated explicitly, since cancelling mid-apply is how you get a half-applied stack and a lease nobody releases. Closes #1801 --- .github/workflows/cleanup-staging.yml | 31 ++++------ .github/workflows/deploy-all.yml | 6 ++ .github/workflows/deploy-azure.yml | 85 ++++++++++++--------------- .github/workflows/rollback.yml | 8 +++ 4 files changed, 65 insertions(+), 65 deletions(-) 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..2e4667445 100644 --- a/.github/workflows/deploy-all.yml +++ b/.github/workflows/deploy-all.yml @@ -162,6 +162,12 @@ jobs: TF_BACKEND_GCP: ${{ secrets.TF_BACKEND_GCP }} # Deploy to Azure Container Apps + # + # Deliberately carries no `concurrency` (#1801). The Terraform state guard is + # the `azure-tfstate-` group on the called workflow's own + # build-and-deploy job, which runs as a real job of this run and is serialized + # there. 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. 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..5f3ba3074 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,14 @@ 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 an ordinary failure + # Terraform unlocks on its way out, so the step 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; and if the runner dies hard no + # step runs at all. That leaves only a Terraform crash, where a stranded + # lease should surface loudly and be cleared with `terraform force-unlock`, + # not swept up by a step that also breaks live leases. - name: Save deployment info run: | diff --git a/.github/workflows/rollback.yml b/.github/workflows/rollback.yml index 0110489c5..19a4569ae 100644 --- a/.github/workflows/rollback.yml +++ b/.github/workflows/rollback.yml @@ -398,6 +398,14 @@ 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. + concurrency: + group: azure-tfstate-${{ inputs.environment }} + cancel-in-progress: false permissions: id-token: write contents: read From c09fe58d3acf211a4fbdc597f397936e7ff38916 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 12 Aug 2026 02:34:59 +0200 Subject: [PATCH 2/4] docs(ci/azure): correct and qualify the concurrency-group comments Review of #1803 found the deploy-all.yml comment asserted an equality that does not hold: the group on the called workflow's build-and-deploy job is derived from that workflow's own prepare output, which is not necessarily the environment deploy-all computed. Reword, and note that GitHub documents the sibling hazard for cancel-in-progress: true. Record two consequences of sharing the group across workflows on rollback-azure: GitHub keeps one pending entry per group so a queued rollback can be evicted by a later arrival (the summary job exits 1 on any non-success, so it is not silent), and it is undocumented whether a job parked awaiting environment approval holds its group. --- .github/workflows/deploy-all.yml | 13 +++++++++---- .github/workflows/rollback.yml | 10 ++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/.github/workflows/deploy-all.yml b/.github/workflows/deploy-all.yml index 2e4667445..868c79e0c 100644 --- a/.github/workflows/deploy-all.yml +++ b/.github/workflows/deploy-all.yml @@ -164,10 +164,15 @@ jobs: # Deploy to Azure Container Apps # # Deliberately carries no `concurrency` (#1801). The Terraform state guard is - # the `azure-tfstate-` group on the called workflow's own - # build-and-deploy job, which runs as a real job of this run and is serialized - # there. 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. + # 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` (caller and + # called sharing a group cancel each other); at `false` it stalls instead. deploy-azure: name: Deploy Azure Container Apps needs: determine-deployment diff --git a/.github/workflows/rollback.yml b/.github/workflows/rollback.yml index 19a4569ae..ffde08e2e 100644 --- a/.github/workflows/rollback.yml +++ b/.github/workflows/rollback.yml @@ -403,6 +403,16 @@ jobs: # 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 is ever given required reviewers + # (see the header note; none are configured today), it is undocumented + # whether a job parked awaiting approval holds its group. If it does, an + # unapproved rollback blocks deploys to that environment meanwhile. concurrency: group: azure-tfstate-${{ inputs.environment }} cancel-in-progress: false From 07767344651bc1c9e2575e4e434ae8a99be2f1f0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 12 Aug 2026 02:37:19 +0200 Subject: [PATCH 3/4] docs(ci/azure): stop the rollback comment restating live environment config Review round 2 on #1803. The comment added in c09fe58 asserted that no environment carries required reviewers today. cleanup-staging.yml deliberately refuses to restate that same fact, precisely so the comment cannot rot into false reassurance, and rollback.yml's own header instructs the reader to configure those reviewers. Point at #1660 instead; the conditional framing already carries the meaning without asserting current config. Also correct the deploy-all.yml note: GitHub's documented behaviour when caller and called share a group at cancel-in-progress: true is that the already-running caller is cancelled, not that the two cancel each other. --- .github/workflows/deploy-all.yml | 5 +++-- .github/workflows/rollback.yml | 9 +++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/workflows/deploy-all.yml b/.github/workflows/deploy-all.yml index 868c79e0c..bf79df9ac 100644 --- a/.github/workflows/deploy-all.yml +++ b/.github/workflows/deploy-all.yml @@ -171,8 +171,9 @@ jobs: # # 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` (caller and - # called sharing a group cancel each other); at `false` it stalls instead. + # 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/rollback.yml b/.github/workflows/rollback.yml index ffde08e2e..f00339480 100644 --- a/.github/workflows/rollback.yml +++ b/.github/workflows/rollback.yml @@ -409,10 +409,11 @@ jobs: # - 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 is ever given required reviewers - # (see the header note; none are configured today), it is undocumented - # whether a job parked awaiting approval holds its group. If it does, an - # unapproved rollback blocks deploys to that environment meanwhile. + # - 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 From 2c4f88ec60595b0ebb159d4ed55aa75e8e49466b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 12 Aug 2026 02:40:56 +0200 Subject: [PATCH 4/4] docs(ci/azure): stop overstating the failure-path lease coverage Adversarial review of #1803 showed the removal rationale claimed more than is true. A failed apply does not always unlock: the unlock call itself can fail ("Error releasing the state lock", e.g. OIDC expiry mid-apply), Terraform can crash, and Actions SIGKILLs after the cancellation grace period rather than letting the graceful shutdown finish. The azurerm backend takes an infinite lease, so none of those self-expire. State the residue instead of denying it. The decisive argument is unchanged and does not depend on the overstatement: a step that clears those cases can only do so by also breaking live leases, which is the defect this issue is about. Recovery stays deliberate and manual. --- .github/workflows/deploy-azure.yml | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/.github/workflows/deploy-azure.yml b/.github/workflows/deploy-azure.yml index 5f3ba3074..f47f7b30a 100644 --- a/.github/workflows/deploy-azure.yml +++ b/.github/workflows/deploy-azure.yml @@ -302,13 +302,20 @@ jobs: EOT # A "Release state lock on failure" step used to break the lease here on - # `failure() || cancelled()`. Removed with #1801: on an ordinary failure - # Terraform unlocks on its way out, so the step 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; and if the runner dies hard no - # step runs at all. That leaves only a Terraform crash, where a stranded - # lease should surface loudly and be cleared with `terraform force-unlock`, - # not swept up by a step that also breaks live leases. + # `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: |