Conversation
Ports the hardening reviewed on stream-py#288. The label is applied when the Release PR opens, not when it merges, so closed-unmerged Release PRs hold slots in the listing forever and one page eventually stops containing the real pending release.
| if: >- | ||
| !cancelled() && | ||
| (github.event_name == 'push' || inputs.publish_tag == '') && | ||
| (github.ref_name == 'master' || endsWith(github.ref_name, '.x')) && | ||
| needs.detect.outputs.pending != 'true' |
There was a problem hiding this comment.
A failed detect no longer skips this job. If the find step aborts before it writes $GITHUB_OUTPUT, needs.detect.outputs.pending is empty, '' != 'true' is true, and !cancelled() lets this job run. The one call still unguarded is nums="$(gh api --paginate ...)" on line 88, so a single non-2xx there now produces the outcome of defect 1: release-please opens a second Release PR on top of the untagged release, while tests and release still skip on their implicit success(). Nothing gets tagged and a duplicate Release PR appears.
Gating on the explicit value is fail-closed for failed, skipped and cancelled alike, and it makes the repeated event and ref guard redundant (the comment above can lose its second half).
| if: >- | |
| !cancelled() && | |
| (github.event_name == 'push' || inputs.publish_tag == '') && | |
| (github.ref_name == 'master' || endsWith(github.ref_name, '.x')) && | |
| needs.detect.outputs.pending != 'true' | |
| if: needs.detect.outputs.pending == 'false' |
There was a problem hiding this comment.
Fixed in 3b1f371, taking your suggestion exactly:
if: needs.detect.outputs.pending == 'false'The fail-open reading is right and it is the same defect this PR set out to remove, arriving through the gate rather than the query. Fail-closed also settles the Verify go.mod major question you raise: no reliance on how GitHub propagates a failed job's outputs, since anything that is not the literal false holds the job back.
The duplicated event and ref guard is removed with it.
| # eventually stop containing the genuinely pending release, which reads as | ||
| # "nothing to release" and passes. merged_at comes back in the listing, so | ||
| # filtering on it here keeps the per-PR lookups below to real candidates. | ||
| nums="$(gh api --paginate "repos/${GITHUB_REPOSITORY}/issues" \ |
There was a problem hiding this comment.
This assignment is the only one left unguarded, and --paginate turns it from one request into N, so the bash -e abort this PR set out to remove is now more likely here, not less. Combined with the new !cancelled() on release-pr, that abort is no longer fail-safe either.
Guard it the way the per-PR lookup is guarded, and treat a failure as pending rather than as nothing to release.
There was a problem hiding this comment.
Fixed in 3b1f371. The listing call is guarded the same way, and a failure there sets lookup_failed rather than leaving nums empty:
if ! nums="$(gh api --paginate ...)"; then
echo "::warning::Could not list pending releases."
nums=""
lookup_failed=true
fiYou are right that --paginate made this the most likely call to fail, not the least.
| # Under `bash -e` an unguarded assignment from a non-2xx would abort the | ||
| # step, which would fail detect and skip release-pr with it. | ||
| if ! sha="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${n}" \ | ||
| --jq 'select(.base.ref == env.BASE) | .merge_commit_sha // empty')"; then |
There was a problem hiding this comment.
merge_commit_sha is populated on closed-unmerged PRs too, with GitHub's speculative test-merge commit, and merged_at is already in this very response. Dropping the check leaves the listing filter on line 90 as the single line of defence, for no saving.
Live example: getstream-go#156 ("chore(main): release 8.0.0") is closed, not merged, still carries autorelease: pending, has base.ref == main, and merge_commit_sha == a38332a0b92b398f7fb8a7b5884fcbcf118e02e2. That commit is not on main and never will be. If such a PR reaches this loop the new jq returns its sha, sha != HEAD_SHA, and pending=true on every push forever: no Release PR, no tag, one warning annotation.
| --jq 'select(.base.ref == env.BASE) | .merge_commit_sha // empty')"; then | |
| --jq 'select(.merged_at != null and .base.ref == env.BASE) | .merge_commit_sha // empty')"; then |
There was a problem hiding this comment.
Fixed in 3b1f371, merged_at != null is back in the per-PR filter.
Verified your example rather than taking it on trust:
$ gh api repos/GetStream/getstream-go/pulls/156 --jq '{state, merged_at, base: .base.ref, merge_commit_sha}'
{"state":"closed","merged_at":null,"base":"main","merge_commit_sha":"a38332a0..."}
$ gh api repos/GetStream/getstream-go/issues/156 --jq '[.labels[].name]'
["autorelease: pending"]
$ git merge-base --is-ancestor a38332a0 origin/main; echo $?
1
So the landmine is already in place, not hypothetical. It also means go has one label slot permanently consumed, which is the accumulation the pagination fix addresses.
| # step, which would fail detect and skip release-pr with it. | ||
| if ! sha="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${n}" \ | ||
| --jq 'select(.base.ref == env.BASE) | .merge_commit_sha // empty')"; then | ||
| echo "::warning::Could not read PR #${n}; skipping it." |
There was a problem hiding this comment.
A failed lookup now reads as "nothing to release". release-please swaps the label for autorelease: tagged when it tags, so in steady state nums holds exactly one number: the pending release. One 502 on it warns, continues, and the loop ends with sha="", so the step prints "No pending release on master", sets pending=false, and release-pr opens a second Release PR on top of the untagged one while the tag job never runs. Aborting was wrong, but reading unknown as "nothing pending" is worse than aborting.
With two stuck candidates it also misreports: skipping the newest lets an older one match, so the "Release stuck" warning names the wrong PR and tells the operator to re-run the wrong sha.
Set lookup_failed=true here, and after the loop treat [ -z "$sha" ] && [ "$lookup_failed" = true ] as pending=true, so an unknown answer stands down instead of releasing.
There was a problem hiding this comment.
Fixed in 3b1f371. lookup_failed is set on both failure paths, and the branch reads:
if [ -z "$sha" ] && [ "$lookup_failed" = true ]; then
pending=true
echo "::warning::Could not determine whether a release is pending on ${BASE}; standing down."
elif [ -z "$sha" ]; then
echo "No pending release on ${BASE}."Given how often this logic has been wrong, I stubbed gh and ran the branch:
| scenario | pending | ready |
|---|---|---|
| nothing pending | false | false |
| pending at this commit | true | true |
| pending at another sha | true | false |
| listing call fails | true | false |
| candidate call fails | true | false |
The misreporting case you raise, where skipping the newest lets an older candidate match and the warning names the wrong PR, is now covered too: any skipped candidate sets lookup_failed, so a wrong-PR match cannot be reported as a clean result.
| if: needs.detect.outputs.pending != 'true' | ||
| # No status function of its own would mean an implicit success(), so one transient | ||
| # gh api error inside detect would stop the reversible half too. Its own guard still | ||
| # has to be repeated here, because detect is skipped on the dispatch path and this |
There was a problem hiding this comment.
Six repos now carry this same 55-line detect job, and this PR is the second hand-propagation of a fix to all of them (the body says the step is "now identical to stream-py's", which is the tell). A reusable workflow in GetStream/.github called with uses: would make the next fix one PR instead of six that can drift.
It has already drifted: getstream-go carries this identical comment, but its detect is guarded only by if: github.ref_name == 'main' || endsWith(github.ref_name, '.x') and its workflow_dispatch takes no inputs, so detect is never skipped on the dispatch path there.
There was a problem hiding this comment.
Both halves taken.
The duplication is gone in 3b1f371: gating on the explicit pending == 'false' makes the repeated guard redundant, so it and the sentence justifying it are both removed.
On the reusable workflow in GetStream/.github: agreed, and this round is the evidence for it. Six copies drifted within two days, and this review caught defects that had to be hand-carried to all six plus stream-py. Not doing it inside this PR, because it changes how every repo resolves its release workflow and wants to be its own change. Recording it on CHA-2963 with the other cross-repo items.
| echo "Release PR #${num} merged at \`${sha}\` and was never tagged, so no Release PR will be opened or refreshed until it clears." | ||
| echo | ||
| echo "Re-run the \`Release\` run for \`${sha}\`. If that commit is genuinely broken, remove the \`autorelease: pending\` label from #${num} by hand and release forward." | ||
| } >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
The PR names the problem as "a stuck release stays stuck until somebody opens the run and reads the annotation", and a step summary is also only visible when somebody opens the run. The conclusion stays green and nothing notifies anyone.
gh pr comment "$num" on the stuck Release PR reaches its subscribers and is one line. Guard it with a marker check so every later push does not add another.
There was a problem hiding this comment.
Fixed in 3b1f371. The stuck branch now comments on the Release PR itself, guarded by a per-sha marker so later pushes do not pile up:
marker="<!-- release-stuck:${sha} -->"
seen="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${num}/comments" --paginate --jq '.[].body' || echo "")"
if ! printf '%s' "$seen" | grep -qF "$marker"; then
...
gh pr comment "$num" --body-file "${RUNNER_TEMP}/release-stuck.md" || echo "::warning::Could not comment on #${num}."
fidetect takes pull-requests: write for it, which is the one cost. The comment write is || echo, so it can never be what fails the job.
Keeping the step summary as well: it is free and it is where someone already in the run will look.
|
The title cuts a real release. This repo squash-merges with
|
The guarded lookups read a failed call as nothing to release, which is worse than the abort they replaced: release-pr would open a second Release PR on top of an untagged one. An unknown answer now stands down, release-pr gates on the explicit pending=false, the per-PR jq keeps its merged_at check against speculative merge commits on closed-unmerged PRs, and a stuck release is announced on its own Release PR.
Ticket
CHA-2963
Problem
Three defects in
detect, all found in review on stream-py#288 and all present here.autorelease: pendingwhen it opens a Release PR, not when it merges, so every Release PR closed without merging keeps the label forever and holds a slot. Once 20 such items exist, the genuinely pending release drops off page one,pendingreads false, andrelease-prproposes a second Release PR on top of the untagged one.run:withoutshell:isbash -e, andsha="$(gh api ...)"takes that call's exit status. One non-2xx on any candidate aborts the step, which failsdetectand, throughneeds: detect, skipsrelease-prwith it.release-pr'sifhas no status function, so an implicitsuccess()applies. A transient error indetecttherefore also stops the reversible half, which nothing else gates.The stand-down path was also silent: a green run with every job skipped looks exactly like a normal push with nothing to release, so a stuck release stays stuck until somebody opens the run and reads the annotation.
Solution
--paginatewithper_page=100, and aselect(.pull_request.merged_at != null)pre-filter.merged_atis in the listing payload, so the per-PR loop now only fetches real candidates, and only to readbase.ref, which the listing does not carry.release-prgains!cancelled()and repeatsdetect's own event and ref guard, so adetectblip no longer holds it back while the dispatch path still skips correctly.$GITHUB_STEP_SUMMARYnaming the PR, the sha and both ways out.The
Find a merged Release PR waiting to be taggedstep is now identical to stream-py's.How to verify
Detect pending releaseprintsNo pending release on <branch>and onlyRelease PRruns.Pending release #N will be tagged at <sha>, the suite runs, then the tag job.