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

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p1Next up; this sprintNext up; this sprint

   severity/highSignificant harmSignificant harm

   urgency/this-sprintWithin the current sprintWithin the current sprint

   impact/internalTeam-internal onlyTeam-internal only

   effort/sHoursHours

   type/bugDefectDefect

   on Jul 28, 2026
2. cristim commented on Aug 13, 2026

   

   cristim

   on Aug 13, 2026

   MemberAuthor

   More actions

   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.
3. cristim commented on Aug 13, 2026

   

   cristim

   on Aug 13, 2026

   MemberAuthor

   More actions

   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.
4. cristim

   closed this as completedin #1816on Aug 13, 2026