fix(ci): no concurrency group on rollback, database-migration, cleanup-staging or destroy-fargate-dev

[cristim]

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Where

• .github/workflows/rollback.yml - no concurrency: key; writes state key github-<env>/terraform.tfstate at :230
• .github/workflows/database-migration.yml - no concurrency: key
• .github/workflows/cleanup-staging.yml - no concurrency: key
• .github/workflows/destroy-fargate-dev.yml - no concurrency: key
• Contrast, all four deploy workflows declare one: deploy-aws-lambda.yml:29-31, deploy-aws-fargate.yml:28-30, deploy-azure.yml:29, deploy-gcp.yml:27
• deploy-aws-lambda.yml:150 writes the identical state key github-<environment>/terraform.tfstate

What

Four workflows that mutate the same Terraform state keys and the same production databases can run in unlimited parallel with each other, and in parallel with the deploy workflows. The deploy workflows' own concurrency groups do not cover them, and those groups are keyed on github.ref while the state key is ref-independent, so even between two deploy runs the group does not correspond to the resource being serialised.

Failure scenario

A rollback and a push-triggered deploy overlap. Both target github-<env>/terraform.tfstate. The deploy loses the lock race and fails; its if: failure() || cancelled() handler at deploy-aws-lambda.yml:190 then deletes the lock that the rollback is holding mid-apply, and a third run is free to acquire it and apply concurrently. The account in question holds the purchase-executing Lambda, its RDS instance and its secrets, so a split-brain state there is a production money-path outage, not a cosmetic one.

For database-migration.yml the equivalent is two concurrent migrate up runs against one production database, which produces the golang-migrate dirty state that requires manual CUDLY_FORCE_MIGRATION_VERSION recovery.

Fix direction

Add concurrency: {group: <state-key-or-db-identity>, cancel-in-progress: false} to each of the four workflows, keyed on the target (environment plus cloud), not on github.ref. While doing so, re-key the existing deploy-workflow groups the same way, since deploy-lambda-${{ github.ref }} does not serialise two runs that share a state key but differ in ref (a workflow_dispatch of environment=dev from a feature branch versus the push-to-main dev deploy are in different groups today and share one state key).

Related

• sec(ci): lambda deploy has same unsafe failure-path state-lock delete as #438 #1269 - the deploy-aws-lambda.yml failure-path lock delete; the concurrency gap is what turns that delete from "questionable" into "actively deletes another run's lock", so the two fixes belong together.

[cristim]

Verified against origin/main (not a local checkout), enumerating every job in all four workflows rather than sampling:

workflow	job	concurrency group
rollback.yml	validate	none
rollback.yml	rollback-aws-lambda	aws-tfstate-${{ inputs.environment }}
rollback.yml	rollback-aws-fargate	aws-tfstate-${{ inputs.environment }}
rollback.yml	rollback-gcp	gcp-tfstate-${{ inputs.environment }}
rollback.yml	rollback-azure	azure-tfstate-${{ inputs.environment }}
rollback.yml	summary	none
cleanup-staging.yml	guard	none
cleanup-staging.yml	destroy-aws-lambda	aws-tfstate-staging
cleanup-staging.yml	destroy-aws-fargate	aws-fargate-tfstate-staging
cleanup-staging.yml	destroy-azure	azure-tfstate-staging
cleanup-staging.yml	destroy-gcp	gcp-tfstate-staging
destroy-fargate-dev.yml	guard	none
destroy-fargate-dev.yml	destroy	aws-fargate-tfstate-dev
database-migration.yml	validate	none
database-migration.yml	migrate-aws	none
database-migration.yml	migrate-gcp	none
database-migration.yml	migrate-azure	none
database-migration.yml	summary	none

Three of the four workflows are covered. #1806 and #1801 gave every state-writing job in rollback.yml, cleanup-staging.yml and destroy-fargate-dev.yml a group. The jobs in those three that still have none (validate, summary, guard) touch no Terraform state, so they do not need one.

database-migration.yml is not covered at all: no top-level concurrency, and none on any of its five jobs. migrate-aws, migrate-gcp and migrate-azure run migrate up / migrate down against a live database. Two dispatches against the same cloud and environment can therefore overlap, and an up racing a down is a data-loss path, not merely lock contention. The validate job's staleness and step checks are evaluated before a concurrent run's migrate step lands, so they do not close this either.

Note that these jobs are not Terraform state writers (they only run terraform init plus terraform output -raw to read an endpoint), so the *-tfstate-* groups from #1806 / #1801 are the wrong key here. The group needs to identify the database being migrated, i.e. cloud plus environment.

Keeping this open, scoped down to database-migration.yml.

[cristim]

Correcting two things I stated earlier in this thread, plus two observations from the review of #1816. The fix itself is unchanged and still correct; these are corrections to the reasoning around it.

1. My "data-loss path" framing was overstated

I wrote earlier that two overlapping dispatches could interleave and that an up racing a down is a data-loss path. That overstates it.

golang-migrate v4.19.1's postgres driver takes an unconditional pg_advisory_lock inside Lock(), and that version exposes no x-no-lock option (the workflow's DB_URL sets no bypass either). So two concurrent migrate runs against one database do not corrupt schema_migrations. The second blocks, with the driver's own comment reading "This will wait indefinitely until the lock can be acquired."

What the concurrency group actually buys, and it is still worth having:

• a second dispatch no longer burns a runner blocked on the advisory lock for up to the 6-hour job ceiling
• cancel-in-progress: false prevents the genuine dirty-state risk, which comes from cancelling mid-migrate, not from concurrency itself

So the residual hazard is smaller than this issue originally claimed. That cuts toward shipping the fix, not away from it, but the claim should not be carried forward into future work.

2. An evicted migration does NOT report a green run

A comment in the first version of #1816 asserted that because the summary job never exits non-zero, an evicted queued migration "leaves the run green". That is false, and it is being removed from the PR.

Established empirically rather than from intuition: across roughly 700 GitHub Actions runs in five repositories, a cancelled job never once coexisted with a success run conclusion. In 109 runs matching this exact shape (at least one success job, at least one cancelled job, and an if: always() job that still ran), the run conclusion was cancelled 109 times and success zero times.

An evicted migration therefore surfaces as a cancelled run, which is visible. There is no silent-failure path here.

3. These jobs appear never to have run

gh api repos/LeanerCloud/CUDly/environments returns 5 environments: aws-fargate-dev, aws-fargate-staging, azure-, dev, gcp-. None of aws-db-*, gcp-db-* or azure-db-*. GitHub auto-creates a deployment environment when a job referencing it starts, which is demonstrably how azure- and gcp- came to exist, so their absence implies no migrate-* job has ever started.

That is consistent with LeanerCloud/cloud-commitments-platform#114: terraform/environments/aws/backend.tf declares a partial backend "s3" whose config comes from -backend-config=backends/<env>.tfbackend, and this workflow runs a bare terraform init, which cannot select a state non-interactively.

Two consequences: the concurrency key is right in principle (one state per environment means one database per environment), but that mapping only becomes real once LeanerCloud/cloud-commitments-platform#114 lands; and there is no runtime evidence available for this change, so static enumeration is the ceiling of what can be verified here.

4. This does not cover the application's own migration path

internal/server/app.go:519 wires runMigrationsFunc: migrations.RunMigrations, so the server runs migrations at startup. A Lambda, Cloud Run or Container App cold start concurrent with a workflow dispatch is a second writer that no GitHub concurrency group can serialize. The advisory lock in point 1 is what protects that case.

Out of scope for this issue, noted so that "migration concurrency: fixed" is not later read as covering it.