Publish the most recent GitHub Release when several share a name - #763
Publish the most recent GitHub Release when several share a name#763AliSoftware wants to merge 4 commits into
Conversation
`publish_release` looked up the release to publish with `releases.find { |r| r.name == name }`, which returns whichever match the GitHub API happened to list first. That ordering is not part of the API contract, and a repository can legitimately host several releases with the same name: each `finalize_release` run creates its own draft, so re-running it for a version — e.g. after a crash is found during smoke-testing — leaves two drafts named `25.1` behind, targeting different commits.
That is what caused the WCiOS 25.1 incident: `publish_release` published the draft from the *first* run, so GitHub created the `25.1` tag on the older commit, and the 25.1.1 hotfix branched off that tag and silently dropped the crash fix that had actually shipped.
Both matching releases are now collected and the most recently created one is published, with the staler ones left untouched. `find_release`, which looks a release up by tag rather than by name, shares the same new `matching_releases` helper and so gains the same guarantee.
The sort key is the release `id` rather than `created_at`: GitHub rewrites `created_at` to the date of the commit a release targets once that release is published, so it is not a reliable creation timestamp — a published release can end up looking older than a draft created before it. Release `id`s are assigned in increasing order as releases are created, drafts included.
Two non-fatal warnings are also emitted, to make the situation visible in the CI logs: one when more than one release matches the name, and one when the release being published turns out not to be a draft anymore.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
| def find_release(repository:, version:) | ||
| release = client.releases(repository).find { |candidate| candidate.tag_name == version } | ||
| release = matching_releases(repository: repository) { |candidate| candidate.tag_name == version }.last |
There was a problem hiding this comment.
I passed this through Opus 5 and GPT 5.6 and they both agree there could be a hole in the logic as it is.
Consider this scenario, admittedly unlikely, but still...
- Re-running finalization can create multiple drafts with the same tag_name.
- One draft is eventually published and becomes the release associated with the tag.
- Another, newer draft may remain.
- A later asset upload or retry should update the published release...
- ...but the highest-ID rule instead selects the leftover draft.
I don't know how likely it is to upload assets or retry a release step after having already published a GH release, but I'm mindful of Murphy's Law.
Opus' suggestion
find_release picks the newest draft over the release that actually owns the tag
The name-based path is right; flagging that the tag-based one needs one more condition.
matching_releases(...).last returns the highest id regardless of draft, and for a tag lookup that isn't the same question as "most recently created". A git tag can back only one published release, so when a non-draft match exists it is the release for that tag — a higher-id draft sharing the same tag_name is a leftover, not a successor.
Not hypothetical: it's the state WCiOS is in right now, post-incident.
| id | tag_name |
draft |
|---|---|---|
| 351929162 | 25.1 | no — published, owns the 25.1 tag |
| 352002205 | 25.1 | yes — never cleaned up |
Run upload_github_release_assets(version: '25.1') against that repo today and this branch resolves to 352002205, the draft, every time. The artifacts land on a release nobody can see, existing_assets is computed against the wrong release so replace_existing silently replaces nothing, and the html_url returned to the caller is an untagged-352002205 link.
Before this PR the outcome was whatever order the API happened to list, so this isn't a regression. But the PR's point is turning an unlucky coin flip into a guarantee, and here it guarantees the wrong side — in precisely the repo state the PR exists to survive.
Suggestion, prefer a published match and fall back to the newest draft:
def find_release(repository:, version:)
matches = matching_releases(repository: repository) { |candidate| candidate.tag_name == version }
release = matches.reject { |candidate| candidate[:draft] }.last || matches.last
return release unless release.nil?
release_for_tag(repository: repository, version: version)
endTwo notes on that snippet:
candidate[:draft]rather than&:draftdeliberately —Sawyer::Resource#method_missingraisesNoMethodErrorfor fields the payload doesn't carry, andspec/github_helper_spec.rb:627builds a stub with nodraftkey that reachesclient.releasesat line 644. Hash access returnsnilthere instead, and it matches therelease[:id]style already used inmatching_releases.- The alternative is folding it into the sort key (
sort_by { |r| [r[:draft] ? 0 : 1, r[:id]] }), but I'd keepmatching_releasesa dumb "filter, order by creation" primitive and let each caller state its own tie-break —publish_releasegenuinely wants the opposite, highest id whether draft or not.
Either way, worth pinning with a test: published + newer draft on the same tag, asserting which wins. Today that behaviour is incidental, and it's exactly what the next person refactoring matching_releases would flip without noticing.
Sibling question on the same repo state: publish_github_release would now pick draft 352002205 and update_release(draft: false) it while tag 25.1 already exists on 351929162. Do you know whether GitHub 422s there or quietly attaches a second release to the tag? If it errors, that's a raw Octokit exception at the final step of a release. The new "not a draft, but has already been published" warning doesn't cover it, since the highest-id match is a draft.
There was a problem hiding this comment.
Good catch, and not hypothetical at all — I reproduced it before fixing. Stubbing the client with exactly what the API returns for woocommerce-ios today (published 351929162 and leftover draft 352002205, both tag_name: "25.1") and running the real code path:
>>> upload_release_assets('25.1') resolved to:
https://github.com/woocommerce/woocommerce-ios/releases/tag/untagged-352002205
So you're right that it guaranteed the wrong side, in precisely the repo state this PR exists to survive. Fixed in c9cacaa, with your suggested shape:
matches = matching_releases(repository: repository) { |candidate| candidate.tag_name == version }
release = matches.reject { |candidate| candidate[:draft] }.last || matches.lastI kept the tie-break at the call site rather than folding it into matching_releases, for the reason you gave: the two lookups are asking different questions. For a name lookup "newest wins" is right, since names are free-form labels and the latest draft targets the newest commit. For a tag lookup it isn't — a tag can back only one published release, so a non-draft match is the release for that tag. The fallback to the newest draft is preserved for the case where no published release claims the tag yet, which is what PCiOS relies on when uploading assets to an unpublished release.
Pinned with a regression test using those real ids, and I verified it fails against the previous logic rather than just passing against the new one:
expected: ("published-api-url")
got: ("draft-api-url")
Also updated the CHANGELOG, whose last sentence ("picks the most recent match too") was made inaccurate by this change.
One correction to the quoted rationale, for the record: it states that Sawyer::Resource#method_missing raises NoMethodError for fields the payload doesn't carry. I checked, and it returns nil instead — so &:draft would have worked too. candidate[:draft] is still the better choice for consistency with the release[:id] in matching_releases, just not for that reason.
On your sibling question
Do you know whether GitHub 422s there or quietly attaches a second release to the tag?
I don't, and I'd rather flag that than guess: I have not verified it empirically, and doing so would mean publishing a real draft against an existing tag on a real repo. The expectation is that GitHub rejects it, since tag_name has to be unique among published releases and the draft here points at a different commit than the one the tag already resolves to — but treat that as reasoning, not as an observed result.
Worth noting the blast radius is small either way: it needs a published release and a newer draft sharing a name, which is the leftover state a re-run of finalization leaves behind. If it does 422, it surfaces as a raw Octokit error at the last step of a release — loud and pointing at a genuinely broken state that needs a human, rather than silently tagging the wrong commit, which is what got us here. Happy to open a follow-up to catch it and re-raise as a UI.user_error! with a pointer to the duplicate drafts, if you think it's worth pre-empting.
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com> Co-authored-by: Olivier Halligon <olivier@halligon.net>
The applied suggestions left trailing whitespace on two blank lines inside the `details` heredoc, which `Layout/TrailingWhitespace` rejects, and reworded the already-published warning, which the spec was still matching on its old phrasing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Making the tag-based lookup deterministic by taking the highest id was the wrong tie-break for that question. A git tag can only ever back a single published release, so when one of the matches is not a draft it *is* the release for that tag; a more recent draft sharing the same `tag_name`—typically left behind by a re-run of the release finalization—is a leftover, not a successor. woocommerce-ios is in exactly that state after the 25.1 incident: published release 351929162 owns the `25.1` tag, and draft 352002205 still carries the same `tag_name` with a higher id. Under the previous tie-break, `upload_github_release_assets(version: '25.1')` resolved to the draft every time, so assets would land on a release nobody can see, `existing_assets` would be computed against the wrong release—silently defeating `replace_existing`—and the caller would get an `untagged-…` URL back. The lookup now prefers a published match and only falls back to the most recent draft when no published release claims the tag, which is the case when uploading assets to a release that has not been published yet. `matching_releases` stays a plain "filter, order by creation" primitive, with each caller stating its own tie-break: `publish_release` still wants the newest match whether draft or not. Reported by @mokagio in review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What does it do?
Fixes point 1 of AINFRA-2725: make
publish_github_releasepublish the latest matching GitHub Release rather than the first one the API happens to list.The bug
publish_releasefound the release to publish with:.findreturns the first match in whatever order the GitHub API listed them, which is not part of the API contract. And a repo can legitimately host several releases sharing a name: everyfinalize_releaserun creates its own draft, so re-running it for a version — as we do when smoke-testing turns up a crash — leaves two drafts named e.g.25.1, targeting different commits.That is what caused the WCiOS 25.1 incident: the draft from the first
finalize_releaserun got published, so GitHub created the25.1tag on the older commit. The 25.1.1 hotfix then branched off that tag and silently dropped the crash fix that had actually shipped in 25.1.The two releases involved, for reference:
created_atdbd800a(stale)558354d(correct)The fix
All matching releases are now collected and the most recently created one is published; staler ones are left untouched.
find_release— which looks a release up by tag instead of by name, and backsupload_github_release_assets— shares the newmatching_releaseshelper and gains the same guarantee.Two non-fatal
UI.importantwarnings were added so the situation is visible in the CI logs: one when more than one release matches the name, and one when the release being published turns out not to be a draft anymore.Why sort on
idand notcreated_atThe issue suggested sorting on the creation date, but
created_atis not trustworthy here: GitHub rewrites a release'screated_atto the date of the commit it targets once that release is published. You can see it in the table above — the published25.1reports06:18:29Z, which is exactlydbd800a's commit date, and the25.1.2draft'screated_atmoved from11:12:23Zto10:57:28Z(its commit date) the moment it was published.Since a commit always precedes the release object targeting it, publishing shifts a release's
created_atearlier. So iffinalize_releaseruns twice with no new commit in between and the second draft is published, itscreated_atcollapses back to the old commit date — making it sort as older than the stale draft, and a re-run would then pick the stale one. Releaseids have no such problem: GitHub assigns them in increasing order as releases are created, drafts included.Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendations.specs/*_spec.rb) if applicable.bundle exec rspecto run the whole test suite and ensure all your tests pass.CHANGELOG.mdfile.MIGRATION.mdfile — n/a, no breaking change.🤖 Generated with Claude Code