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

[cristim]

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.

[cristim]

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.