docs(ci): database-migration concurrency comment claims an evicted migration leaves the run green, which is false

Tracking a known-false comment merged with LeanerCloud/cloud-commitments-cli#1816, so it is not forgotten. Comment-only; no behavioural defect.

What is wrong

.github/workflows/database-migration.yml:252-255 currently reads:

#   - GitHub keeps one pending entry per group, so a queued migration can be
#     evicted by a later dispatch. Unlike rollback.yml, the `summary` job
#     below does not exit 1 on a non-success result, so an evicted migration
#     leaves the run green; read the per-cloud results it prints.

The "leaves the run green" clause is false. This is job-level concurrency, so an evicted queued job's result is cancelled, and a workflow run containing a cancelled job reports a run conclusion of cancelled, not success.

Established empirically during the review of LeanerCloud/cloud-commitments-cli#1816, not 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.

The rest of the sentence is accurate: summary genuinely is if: always() and never exits non-zero, and eviction of a queued dispatch is real.

Why it matters

The claim is wrong in the conservative direction, which is why it did not block the merge: it teaches an operator to distrust a green run that is actually trustworthy. But it is a stated justification that is false, sitting next to a guard whose correctness a future reader will judge from exactly this paragraph.

Second, smaller issue in the same block

The comment block runs to roughly 40 lines for 9 lines of config and largely duplicates the LeanerCloud/cloud-commitments-cli#1816 PR description. Rationale belongs in the PR, not the source. Worth trimming to what a reader cannot deduce from the code:

why the group is keyed on cloud plus environment and deliberately not the *-tfstate-* groups (these jobs take no state lock; the resource protected is the database)
why cancel-in-progress: false (cancelling mid-migrate leaves schema_migrations dirty)
why the || 'unset' sentinel exists (workflow_call passes environment as an unenforced type: string)

Fix

A corrected and trimmed version is already written and verified (comment-only diff, no group value changed). It was deliberately not pushed to LeanerCloud/cloud-commitments-cli#1816: CodeRabbit's review quota on this repo is roughly one review per hour shared across all open PRs, and at the time the queue held a privilege-escalation boundary fix (LeanerCloud/cloud-commitments-cli#1818) that needed the slot more than a comment edit did.

Good candidate to fold into the #114 fix, which touches this same workflow file, rather than spending a separate review cycle on a comment.

Verification bar

git diff must show comment lines only. Re-enumerate every job and its resolved concurrency group before and after and confirm no group value changed.

Recording the corrected text here so it does not live only in an ephemeral scratch directory. Written and verified before LeanerCloud/cloud-commitments-cli#1816 merged; held back only because pushing a comment-only change would have consumed the shared CodeRabbit slot that LeanerCloud/cloud-commitments-cli#1818 needed.

Replacement for the `migrate-aws` comment block

Replaces the current ~40 lines above concurrency: on migrate-aws in .github/workflows/database-migration.yml, including the false "leaves the run green" clause:

    # One group per cloud+environment (#1593): without it a second dispatch
    # burns a runner blocked on the migrator's advisory lock instead of
    # queueing. Not the `aws-tfstate-*` group that deploy and rollback use:
    # these steps take no state lock, and the resource being protected is the
    # database. `cancel-in-progress: false` because cancelling mid-`migrate`
    # leaves schema_migrations dirty. GitHub keeps one pending entry per group,
    # so a queued migration can be evicted by a later dispatch; read the
    # per-cloud results the `summary` job prints. `|| 'unset'` keeps a
    # `workflow_call` with an empty `environment` (an unenforced `type: string`)
    # out of the dev/staging/prod groups.
    concurrency:
      group: db-migrate-aws-${{ inputs.environment || 'unset' }}
      cancel-in-progress: false

migrate-gcp and migrate-azure stay short pointers to the above, trimmed from 5 lines to 3 each.

Note what changed in substance, not just length: the old text named the hazard as an evicted migration reporting green (false). The new text names the real hazard, a second dispatch burning a runner blocked on the advisory lock, and keeps the eviction fact without the false inference about run conclusion.

Every claim in the replacement, verified in-tree

claim	how verified	result
the migrator takes an advisory lock	`database-migration.yml:259` installs `golang-migrate/migrate/v4/cmd/migrate@v4.19.1`	exactly the version whose `postgres.go:241` holds an unconditional `pg_advisory_lock`
`aws-tfstate-*` is what deploy and rollback use	search across `.github/workflows/`	`deploy-aws-lambda.yml:222`, `rollback.yml:186`, `cleanup-staging.yml:106`
`workflow_call` `environment` is an unenforced string	`yaml.safe_load` of the `on:` block	`{"required": true, "type": "string"}`, versus `workflow_dispatch` being `type: choice, options: [dev, staging, prod]`
`summary` prints per-cloud results	`database-migration.yml:510-512`	`RESULT_AWS/GCP/AZURE: ${{ needs.migrate-*.result }}`

Verification already performed

YAML parses after the edit.
Job/group enumeration before and after: no group value changed, diff is comment lines only.

Whoever picks this up should re-run both checks rather than taking them on trust, since the file will have moved on.

Navigation Menu

Repository navigation

docs(ci): database-migration concurrency comment claims an evicted migration leaves the run green, which is false #201

What is wrong

Why it matters

Second, smaller issue in the same block

Fix

Verification bar

Replacement for the `migrate-aws` comment block

Every claim in the replacement, verified in-tree

Verification already performed

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

docs(ci): database-migration concurrency comment claims an evicted migration leaves the run green, which is false #201

Description

What is wrong

Why it matters

Second, smaller issue in the same block

Fix

Verification bar

Activity

cristim commented on Aug 13, 2026

Replacement for the migrate-aws comment block

Every claim in the replacement, verified in-tree

Verification already performed

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Replacement for the `migrate-aws` comment block