From aee356ab767e5cde5a2bad1bd05e3f96c376eceb Mon Sep 17 00:00:00 2001 From: John McLear Date: Wed, 8 Apr 2026 12:38:41 +0100 Subject: [PATCH] fix: use atomic git push in plugin npmpublish workflow (#7494) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plugin publish workflow ran `git push --follow-tags` after `pnpm version patch`. `--follow-tags` is non-atomic per ref: if a concurrent publish run won the race, the branch fast-forward would be rejected but the tag push would still land — leaving a dangling `vN+1` tag with no matching version-bump commit on the branch. Every subsequent push would then fail forever with `npm error fatal: tag 'vN+1' already exists`, because `pnpm version patch` would re-derive the same tag name from the unchanged `package.json`. On 2026-04-08, a single churn day (badge fixes + Dependabot merges firing back-to-back) put ~46 plugins into this state simultaneously. Recovery required hand-bumping `package.json` past the dangling tag on every affected repo, twice (a second wave appeared after the first sweep finished, racing the next wave of publishes). Fix: use `git push --atomic origin ` so the branch update and the tag update succeed or fail as a single server-side transaction. A rejected branch push now also rejects the tag push, the run aborts cleanly, and the next workflow tick can retry against the up-to-date refs without leaving any orphaned tags. Also derive the new tag name from `package.json` after the bump (rather than parsing pnpm version's stdout, which has historically varied) and pass it explicitly into the push. Adds a backend regression test that asserts the workflow file uses `--atomic`, does not contain a literal `git push --follow-tags` command (ignoring the historical comment), and includes both the branch ref and the freshly-bumped tag in the atomic push. The test gates against accidental reverts. This file is the source of truth that `bin/plugins/checkPlugin.ts` propagates into every `ether/ep_*` plugin's `.github/workflows/`, so the next `update-plugins` cron tick will roll the fix out across all plugins automatically. Co-authored-by: Claude Opus 4.6 (1M context) --- bin/plugins/lib/npmpublish.yml | 18 ++++- .../backend/specs/npmpublish-workflow.ts | 81 +++++++++++++++++++ 2 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 src/tests/backend/specs/npmpublish-workflow.ts diff --git a/bin/plugins/lib/npmpublish.yml b/bin/plugins/lib/npmpublish.yml index fe0fbdd3a..a7cf474a6 100644 --- a/bin/plugins/lib/npmpublish.yml +++ b/bin/plugins/lib/npmpublish.yml @@ -15,7 +15,7 @@ jobs: publish-npm: runs-on: ubuntu-latest permissions: - contents: write # for `git push --follow-tags` of the version bump + contents: write # for the atomic version-bump push (branch + tag) id-token: write # for npm OIDC trusted publishing steps: - uses: actions/setup-node@v6 @@ -60,8 +60,22 @@ jobs: git config user.name 'github-actions[bot]' git config user.email '41898282+github-actions[bot]@users.noreply.github.com' pnpm i + # `pnpm version patch` bumps package.json, makes a commit, and creates + # a `v` tag. Capture the new tag name from package.json + # rather than parsing pnpm's output, which has historically varied. pnpm version patch - git push --follow-tags + NEW_TAG="v$(node -p "require('./package.json').version")" + # CRITICAL: use --atomic so the branch update and the tag update + # succeed (or fail) as a single transaction on the server. The old + # `git push --follow-tags` was non-atomic per ref: if a concurrent + # publish run won the race, the branch fast-forward would be rejected + # but the tag push would still land — leaving a dangling tag with no + # matching commit on the branch. Subsequent runs would then forever + # try to bump to the same already-existing tag and fail with + # `tag 'vN+1' already exists`. With --atomic, a rejected branch push + # rejects the tag push too, and the next workflow tick can retry + # cleanly against the up-to-date refs. + git push --atomic origin "${GITHUB_REF_NAME}" "${NEW_TAG}" # This is required if the package has a prepare script that uses something # in dependencies or devDependencies. - diff --git a/src/tests/backend/specs/npmpublish-workflow.ts b/src/tests/backend/specs/npmpublish-workflow.ts new file mode 100644 index 000000000..8c08f507d --- /dev/null +++ b/src/tests/backend/specs/npmpublish-workflow.ts @@ -0,0 +1,81 @@ +'use strict'; + +// Regression test for bin/plugins/lib/npmpublish.yml. +// +// This file is the source-of-truth template that `bin/plugins/checkPlugin.ts` +// propagates into every `ether/ep_*` plugin's `.github/workflows/`. The +// version-bump step in it MUST use `git push --atomic` rather than the older +// `git push --follow-tags`, otherwise concurrent publish runs can leave +// dangling `vN+1` tags on plugin repos with no matching version-bump commit — +// at which point every subsequent push fails forever with +// `npm error fatal: tag 'vN+1' already exists` until someone reconciles the +// repo by hand. +// +// On 2026-04-08 a single churn day produced ~46 broken plugins this way; the +// recovery was painful enough to be worth a regression test. + +import {strict as assert} from 'assert'; +import * as fs from 'fs'; +import * as path from 'path'; + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..', '..'); +const NPMPUBLISH_YML = path.join(REPO_ROOT, 'bin', 'plugins', 'lib', 'npmpublish.yml'); + +describe(__filename, function () { + let yml: string; + + before(function () { + yml = fs.readFileSync(NPMPUBLISH_YML, 'utf8'); + }); + + it('uses git push --atomic for the version bump', function () { + assert.match( + yml, /git push --atomic\b/, + 'npmpublish.yml must use `git push --atomic` so the branch update and ' + + 'the tag push happen as a single transaction. Without --atomic, a ' + + 'rejected branch fast-forward (e.g. lost race against a concurrent ' + + 'publish run) can still leave the tag pushed, producing a dangling ' + + 'vN+1 tag and breaking every future publish on the plugin.', + ); + }); + + it('does not regress to `git push --follow-tags`', function () { + // Strip YAML comments before checking — the historical bug is described + // in a comment block above the new code, and that's an intentional + // forensic note, not a regression. We only care if the actual command + // line uses --follow-tags. + const commandLines = yml + .split('\n') + .filter((l) => !/^\s*#/.test(l)) + .join('\n'); + assert.doesNotMatch( + commandLines, /git push --follow-tags\b/, + '`git push --follow-tags` is non-atomic per ref and is the exact ' + + 'failure mode this workflow used to have. Use `git push --atomic ' + + 'origin ` instead.', + ); + }); + + it('pushes both the branch ref and the version tag in the atomic command', function () { + // Find the atomic push line and assert it carries at least two refspecs + // (the branch + the tag). We don't pin the exact variable names — just + // require that the line names something tag-shaped and something + // branch-shaped — but we DO require the new tag to be derived from the + // freshly-bumped package.json so it can't drift from what `pnpm version + // patch` actually wrote. + const lines = yml.split('\n'); + const pushLine = lines.find((l) => /git push --atomic\b/.test(l)); + assert.ok(pushLine, 'expected to find a `git push --atomic` line'); + // Branch ref — workflow_call inherits the caller's ref via GITHUB_REF_NAME. + assert.match( + pushLine!, /\$\{?GITHUB_REF_NAME\}?/, + 'atomic push must include the branch ref via $GITHUB_REF_NAME so it ' + + 'works for both `main`- and `master`-default plugins', + ); + // Tag ref — must reference the variable holding the just-bumped tag. + assert.match( + pushLine!, /\$\{?NEW_TAG\}?|\$\{?TAG\}?/, + 'atomic push must include the version tag (NEW_TAG / TAG) it just created', + ); + }); +});