diff --git a/.github/workflows/create-release.yml b/.github/workflows/create-release.yml index 29909ecdcf..59cfdac798 100644 --- a/.github/workflows/create-release.yml +++ b/.github/workflows/create-release.yml @@ -12,23 +12,23 @@ on: name: Create Release -# A `workflow_run` trigger runs against the tip of the default branch, not -# against the sha of the run that triggered it, so two Vendor Update runs -# completing close together produce two Create Release runs evaluating the -# *same* tip -- observed on 78ec96afb (runs 34894022730 and 34894646592, six -# minutes apart) and on 1e0ebc53b. Both would then read PCAPKIT_TAG_EXISTS -# before either had tagged, which is a double-publish race. Serialise instead: -# never cancel a release run, just make the second wait and find the tag -# already there. -# Deliberately still keyed on the ref, unlike the validation workflows, and the -# distinction is the point: for them a dropped run means a commit nobody checked, -# which is unrecoverable, whereas here a dropped run means a publish that did not -# happen twice. This workflow tags, uploads to PyPI and uploads to Anaconda, and -# unlike cron-conda and cron-vendor it has *no* job-level concurrency to fall back -# on, so this group is the only thing serialising it. Keying it per run would let -# two runs tag and publish concurrently, which is the worse failure. +# One release run at a time per ref. A ``workflow_run`` runs on ``main``'s tip, +# so two Vendor Update runs completing close together start two runs on one tip +# (78ec96afb: runs 34894022730 and 34894646592, six minutes apart), and both +# would read PCAPKIT_TAG_EXISTS before either had tagged. Unlike cron-conda and +# cron-vendor, no job-level group backs this one up, so it never cancels a run in +# progress, and a run that may release is keyed on its ref, never on itself. +# +# #1705: a group holds one pending run and cancels it when another joins, so a +# run whose Vendor Update did not conclude ``success`` -- one ``version_check`` +# skips, so it releases nothing -- is keyed on its own ``github.run_id``. Keyed on +# the ref, such a run, 38499295798, displaced the pending 38499150441. Now only a +# run that may release can wait in ``main``'s group or displace the one waiting +# there, evaluating ``main``'s newer tip instead. A tag push keeps its tag's group. +# GitHub documents the one pending run, and ``github.run_id`` as unique per run: +# https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency concurrency: - group: create-release-${{ github.ref }} + group: create-release-${{ (github.event_name != 'workflow_run' || github.event.workflow_run.conclusion == 'success') && github.ref || github.run_id }} cancel-in-progress: false # Every job below with an effect outside this run declares a deployment diff --git a/docs/source/contributing/releasing.rst b/docs/source/contributing/releasing.rst index 205e8a92d8..6a839cb0d2 100644 --- a/docs/source/contributing/releasing.rst +++ b/docs/source/contributing/releasing.rst @@ -116,6 +116,15 @@ when some target's evidence is not yet ``'true'`` common Vendor Update run that bumped nothing costs a minute, not a full-suite run. +A run that may release waits for the one ahead of it on the same ref and never +cancels it (``create-release.yml:30-32``), since two Vendor Update runs +completing close together raise two runs on the same tip. A group holds one +pending run, which a newer one cancels, so a run whose Vendor Update did not +conclude ``success`` is keyed on its own ``github.run_id`` instead +(`#1705 `__). It releases +nothing, so it neither waits behind a release nor displaces a run waiting +there; only a newer run that may release, reading ``main``'s newer tip, can. + One Commit per Release ---------------------- diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index d193074cd8..e27a5b718b 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -383,6 +383,14 @@ skipping because the tag a *previous* run already created still exists): this one can skip the very first attempt, before any tag is ever created, purely because the upstream Vendor Update run did not itself conclude ``success``. +Such a run is also kept out of the release's concurrency group +(``create-release.yml:30-32``, +`#1705 `__). It is keyed on +its own ``github.run_id`` rather than on the ref, so it cannot cancel a run +waiting there with release work. Keyed on the ref, run 38499295798 joined at +02:06:32Z on 2026-10-14, and the pending 38499150441, whose Vendor Update had +succeeded, was cancelled a second later. + ``pull_request_target`` ----------------------- diff --git a/tests/project/test_release_concurrency_1705.py b/tests/project/test_release_concurrency_1705.py new file mode 100644 index 0000000000..309ba4f56a --- /dev/null +++ b/tests/project/test_release_concurrency_1705.py @@ -0,0 +1,131 @@ +# -*- coding: utf-8 -*- +"""#1705: a Create Release run that cannot release must not displace one that can. + +Create Release serialises on one workflow-level group per ref, and a group holds +one pending run, cancelling it when another joins. Every Vendor Update completion +raises a run, whatever its conclusion, and the one ``version_check`` skips used +to join ``main``'s group too. On 2026-10-14, read off the runs' timestamps (each +Create Release run was created 2-3 s after a Vendor Update run completed): + +* 38497299656, from Vendor Update 38497182777 (``success``), ran until 02:12:48Z; +* 38499150441, from 38498670543 (``success``), was pending from 02:04:54Z; +* 38499295798, from 38499290316 (``skipped``), joined at 02:06:32Z and + cancelled it; +* 38499303910, from 38499299431 (``skipped``), cancelled that in turn, and was + the run that eventually ran. + +So the group is now keyed on the ref only for a run ``version_check`` can run; +any other run is keyed on its own ``github.run_id``. These tests evaluate the +group with the evaluator :mod:`tests.project.test_release_gates` pins. + +""" + +from __future__ import annotations + +import re +import unittest + +from tests.project.test_release_gates import (HUMAN_ACTOR_ID, NULL, RELEASE_APP_BOT_ID, WORKFLOW, + declared_if, evaluate_condition, evaluate_expression, + job_blocks) + +#: The workflow-level ``concurrency:`` block: a one-line ``group:`` holding one +#: interpolation after a literal prefix, then ``cancel-in-progress:``. +_CONCURRENCY = re.compile( + r'^concurrency:\n' + r' group: (?P[^$\n]*)\$\{\{(?P[^\n]*)\}\}\n' + r' cancel-in-progress: (?P\S+)\n', + re.M, +) + +#: Conclusions a Vendor Update run can end with, besides ``success``. +OTHER_CONCLUSIONS = ('skipped', 'failure', 'cancelled', 'timed_out', 'neutral', + 'action_required', 'stale', 'startup_failure') + +MAIN = 'refs/heads/main' +TAG = 'refs/tags/v1.5.0' + + +def run_context(run_id: 'str', event_name: 'str' = 'workflow_run', + conclusion: 'object' = 'success', ref: 'str' = MAIN, + actor_id: 'str' = HUMAN_ACTOR_ID) -> 'dict[str, object]': + """One run's ``github``/``vars`` context; a ``push`` carries no ``workflow_run``.""" + event = {} # type: dict[str, object] + if event_name == 'workflow_run': + event = {'workflow_run': {'conclusion': conclusion}} + return { + 'github': { + 'event_name': event_name, + 'ref': ref, + 'ref_name': ref.split('/', 2)[2], + 'run_id': run_id, + 'actor_id': actor_id, + 'event': event, + }, + 'vars': {'RELEASE_APP_BOT_ID': RELEASE_APP_BOT_ID}, + } + + +class TestReleaseConcurrency(unittest.TestCase): + """The group each kind of run joins, evaluated.""" + + @classmethod + def setUpClass(cls) -> None: + cls.text = WORKFLOW.read_text(encoding='utf-8') + match = _CONCURRENCY.search(cls.text) + if match is None: + raise AssertionError(f'no workflow-level concurrency group in {WORKFLOW}') + cls.prefix = match.group('prefix') + cls.body = match.group('body') + cls.cancel = match.group('cancel') + cls.version_check = declared_if(job_blocks(cls.text)['version_check']) + + def group(self, context: 'dict[str, object]') -> 'str': + value = evaluate_expression('${{' + self.body + '}}', context) + self.assertIsInstance(value, str, f'the group key evaluated to {value!r}') + return self.prefix + str(value) + + def test_a_workflow_run_shares_the_group_exactly_when_version_check_runs(self) -> None: + """Anything else releases nothing, so it gets a group of its own.""" + for conclusion in ('success', NULL) + OTHER_CONCLUSIONS: + with self.subTest(conclusion=conclusion): + context = run_context('38499295798', conclusion=conclusion) + runs = evaluate_condition(self.version_check, context) + self.assertEqual(self.group(context) == f'create-release-{MAIN}', runs, + f'version_check {"runs" if runs else "skips"} on a Vendor Update ' + f'that concluded {conclusion!r}, but the run joins ' + f'{self.group(context)!r}') + + def test_skipped_vendor_updates_cannot_displace_run_38499150441(self) -> None: + """The 2026-10-14 runs: neither skipped one meets the pending run, nor each other.""" + pending = self.group(run_context('38499150441')) + first = self.group(run_context('38499295798', conclusion='skipped')) + second = self.group(run_context('38499303910', conclusion='skipped')) + self.assertEqual(pending, f'create-release-{MAIN}') + self.assertEqual(len({pending, first, second}), 3, (pending, first, second)) + self.assertFalse(first.startswith('create-release-refs/'), first) + + def test_runs_that_may_release_still_wait_for_each_other(self) -> None: + """The double-publish guard the group was for: never per run, never cancelling.""" + self.assertEqual(self.cancel, 'false') + self.assertEqual(self.group(run_context('34894022730')), + self.group(run_context('34894646592'))) + for actor_id in (HUMAN_ACTOR_ID, RELEASE_APP_BOT_ID): + for run_id in ('1', '2'): + with self.subTest(actor_id=actor_id, run_id=run_id): + context = run_context(run_id, event_name='push', ref=TAG, actor_id=actor_id) + self.assertEqual(self.group(context), f'create-release-{TAG}') + + def test_the_scanner_agrees_with_yaml(self) -> None: + """What :data:`_CONCURRENCY` reads is what a YAML parse reads.""" + try: + import yaml # pylint: disable=import-outside-toplevel + except ImportError: + self.skipTest('PyYAML is only installed with the test extra') + parsed = yaml.safe_load(self.text)['concurrency'] + self.assertEqual(parsed, {'group': self.prefix + '${{' + self.body + '}}', + 'cancel-in-progress': False}) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/project/test_unit_tests_queue.py b/tests/project/test_unit_tests_queue.py index 26a3b33a0b..11f9e19458 100644 --- a/tests/project/test_unit_tests_queue.py +++ b/tests/project/test_unit_tests_queue.py @@ -602,6 +602,7 @@ def test_the_scan_is_not_blind(self) -> None: RELEASE_CITATIONS: 'dict[str, list[tuple[Optional[str], str, Optional[str]]]]' = { 'docs/source/contributing/releasing.rst': [ (None, 'push:', '- completed'), + (None, 'concurrency:', 'cancel-in-progress: false'), ('version_check', 'if: >-', 'vars.RELEASE_APP_BOT_ID) }}'), ('version_check', '- name: Checkout code', 'fetch-depth: 0'), ('version_check', 'print(pcapkit.__version__)', None), @@ -628,6 +629,7 @@ def test_the_scan_is_not_blind(self) -> None: ], 'docs/source/contributing/workflows.rst': [ (None, 'workflow_run:', '- completed'), + (None, 'concurrency:', 'cancel-in-progress: false'), ('version_check', 'if: >-', 'vars.RELEASE_APP_BOT_ID) }}'), ('unit-tests', 'needs: [ version_check ]', None), ('unit-tests', 'uses: ./.github/workflows/unit-tests.yml', None),