From 953fccd0b4796529531039eaaf4586669ae66719 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 5 Oct 2026 22:03:19 +0900 Subject: [PATCH] fix: A green Unity package release pull request is merged even when GitHub lists its finished checks late The release-please path runs merge-package-release-pr with --no-wait right after the dispatch step has watched the release PR checks to completion. GitHub's per-workflow run listing (gh run list --workflow --branch) can keep showing such a run as missing or unfinished for tens of seconds: on 2026-10-05 a build-and-test run completed at 12:41:44, the merge step read the listing at 12:42:00 and saw nothing for the head, and the same query returned the completed run by 12:42:39. The pull request was left draft, and because every release-please run moves the release branch head, a re-run hit the same window again. Add --checks-grace-seconds: under --no-wait, a pass that is waiting only on the head's check runs is re-read until the grace runs out, then the pull request is left draft as before. A stale pin still decides in one pass, since the stamp takes far longer than any grace. release-please.yml passes 120 seconds. --- .github/workflows/release-please.yml | 5 +- .../automation/package_release_pr_merge.go | 65 ++++++++++++----- .../package_release_pr_merge_test.go | 73 +++++++++++++++++++ .../package_release_pr_ready_merge.go | 8 +- docs/dispatcher-pin-release-order.md | 4 + 5 files changed, 132 insertions(+), 23 deletions(-) diff --git a/.github/workflows/release-please.yml b/.github/workflows/release-please.yml index c1c297c0e4..553e4fb4bb 100644 --- a/.github/workflows/release-please.yml +++ b/.github/workflows/release-please.yml @@ -143,7 +143,9 @@ jobs: # dispatcher release pull request is open, and while the pin at its head # still disagrees with the dispatcher version the base branch manifest # releases. Both windows would otherwise ship a package that pins an - # older dispatcher than the one main is about to release. + # older dispatcher than the one main is about to release. The checks grace + # covers the run listing lagging the completion the previous step just + # watched; without it a green head is left draft until the next push. - name: Merge the Unity package release pull request when no dispatcher release is pending if: steps.target.outputs.branch == 'main' && steps.release_commit.outputs.skip != 'true' && steps.package_release_sync.outputs.ready != 'false' && steps.release.outputs.prs_created == 'true' && !cancelled() && (steps.release_pr_checks.outcome == 'success' || steps.release_pr_checks.outcome == 'failure') working-directory: cli/release-automation @@ -157,3 +159,4 @@ jobs: --base-branch "${TARGET_BRANCH}" --require-no-open-dispatcher-pr --no-wait + --checks-grace-seconds 120 diff --git a/cli/release-automation/internal/automation/package_release_pr_merge.go b/cli/release-automation/internal/automation/package_release_pr_merge.go index ed27a9f15f..eaf1c71bf3 100644 --- a/cli/release-automation/internal/automation/package_release_pr_merge.go +++ b/cli/release-automation/internal/automation/package_release_pr_merge.go @@ -22,6 +22,7 @@ type mergePackageReleasePRConfig struct { dispatcherTag string requireNoOpenDispatcherPR bool waitForMergeable bool + checksGrace time.Duration workflows []string timeout time.Duration interval time.Duration @@ -33,6 +34,17 @@ type mergePackageReleasePRDeps struct { runOutput func(context.Context, string, ...string) (string, error) } +// mergePackageReleasePRPassOutcome is what one pass of the wait loop +// concluded. The two waiting outcomes are kept apart because only a head whose +// check runs are not visible yet can be resolved within seconds. +type mergePackageReleasePRPassOutcome int + +const ( + mergePackageReleasePRPassSettled mergePackageReleasePRPassOutcome = iota + mergePackageReleasePRPassAwaitingPin + mergePackageReleasePRPassAwaitingChecks +) + type mergePackageReleasePullRequest struct { Number int `json:"number"` HeadRefName string `json:"headRefName"` @@ -110,6 +122,7 @@ func parseMergePackageReleasePRFlags(args []string) (mergePackageReleasePRConfig dispatcherTag := flagSet.String("dispatcher-tag", "", "dispatcher release tag the package pin must record (default: the tag the base branch manifest releases)") requireNoOpenDispatcherPR := flagSet.Bool("require-no-open-dispatcher-pr", false, "refuse to merge while a dispatcher release pull request is open") noWait := flagSet.Bool("no-wait", false, "decide once and leave the pull request draft instead of polling") + checksGraceSeconds := flagSet.Int("checks-grace-seconds", 0, "with --no-wait, keep re-reading for this long while the head's check runs are not listed as finished yet") timeoutMinutes := flagSet.Int("timeout-minutes", defaultMergePackageReleasePRTimeoutMinutes, "how long to wait for the pull request to become mergeable") intervalSeconds := flagSet.Int("interval-seconds", defaultMergePackageReleasePRIntervalSeconds, "how long to wait between polls") err := flagSet.Parse(args) @@ -126,6 +139,9 @@ func parseMergePackageReleasePRFlags(args []string) (mergePackageReleasePRConfig if *timeoutMinutes <= 0 || *intervalSeconds <= 0 { return mergePackageReleasePRConfig{}, fmt.Errorf("%s: --timeout-minutes and --interval-seconds must be positive", mergePackageReleasePRCommandName) } + if *checksGraceSeconds < 0 { + return mergePackageReleasePRConfig{}, fmt.Errorf("%s: --checks-grace-seconds must not be negative", mergePackageReleasePRCommandName) + } workflows, err := releasePRCheckWorkflowsFromEnvironment() if err != nil { @@ -138,6 +154,7 @@ func parseMergePackageReleasePRFlags(args []string) (mergePackageReleasePRConfig dispatcherTag: *dispatcherTag, requireNoOpenDispatcherPR: *requireNoOpenDispatcherPR, waitForMergeable: !*noWait, + checksGrace: time.Duration(*checksGraceSeconds) * time.Second, workflows: workflows, timeout: time.Duration(*timeoutMinutes) * time.Minute, interval: time.Duration(*intervalSeconds) * time.Second, @@ -152,19 +169,23 @@ func waitAndMergePackageReleasePR( deps mergePackageReleasePRDeps, ) int { deadline := deps.now().Add(config.timeout) + checksGraceDeadline := deps.now().Add(config.checksGrace) for { - settled, exitCode := attemptMergePackageReleasePR(ctx, stdout, stderr, config, deps) - if settled { + outcome, exitCode := attemptMergePackageReleasePR(ctx, stdout, stderr, config, deps) + if outcome == mergePackageReleasePRPassSettled { return exitCode } // The release-please path runs on every push to the base branch, so a // pull request that is not mergeable now is reconsidered by the next - // push rather than being waited out inside one workflow run. - if !config.waitForMergeable { + // push rather than being waited out inside one workflow run. The one + // exception is a head whose runs finished moments ago: the run listing + // lags their completion by tens of seconds, and the next push may never + // come, so that case alone is re-read until the checks grace runs out. + if !config.waitForMergeable && !withinChecksGrace(outcome, checksGraceDeadline, deps) { writeMergePackageReleasePRLine(stdout, "The Unity package release pull request is not mergeable yet; leaving it draft.") return 0 } - if !deps.now().Before(deadline) { + if config.waitForMergeable && !deps.now().Before(deadline) { writeMergePackageReleasePRLine(stderr, fmt.Errorf( "%s: waiting for the Unity package release pull request to record %s timed out; merge the pull request by hand once its checks pass", mergePackageReleasePRCommandName, mergePackageReleasePRAwaitedTag(config))) @@ -178,6 +199,14 @@ func waitAndMergePackageReleasePR( } } +func withinChecksGrace( + outcome mergePackageReleasePRPassOutcome, + checksGraceDeadline time.Time, + deps mergePackageReleasePRDeps, +) bool { + return outcome == mergePackageReleasePRPassAwaitingChecks && deps.now().Before(checksGraceDeadline) +} + // mergePackageReleasePRAwaitedTag names what the wait was for. The tag is // resolved per pass, so the timeout report has to describe the source rather // than a value when the caller named no tag. @@ -188,31 +217,31 @@ func mergePackageReleasePRAwaitedTag(config mergePackageReleasePRConfig) string return "the dispatcher release the " + config.baseBranch + " manifest publishes" } -// attemptMergePackageReleasePR runs one pass of the wait loop. settled is false -// while the pull request is still expected to reach a mergeable state, so the -// caller keeps polling until the deadline. +// attemptMergePackageReleasePR runs one pass of the wait loop. The outcome is +// a waiting one while the pull request is still expected to reach a mergeable +// state, so the caller keeps polling until the deadline. func attemptMergePackageReleasePR( ctx context.Context, stdout io.Writer, stderr io.Writer, config mergePackageReleasePRConfig, deps mergePackageReleasePRDeps, -) (settled bool, exitCode int) { +) (outcome mergePackageReleasePRPassOutcome, exitCode int) { releasePR, found, err := findPackageReleasePullRequest(ctx, config, deps) if err != nil { writeMergePackageReleasePRLine(stderr, err) - return true, 1 + return mergePackageReleasePRPassSettled, 1 } if !found { writeMergePackageReleasePRLine(stdout, "No open Unity package release pull request; nothing to merge.") - return true, 0 + return mergePackageReleasePRPassSettled, 0 } if config.requireNoOpenDispatcherPR { dispatcherPRIsOpen, err := openDispatcherReleasePullRequestExists(ctx, deps.runOutput, config.repository, config.baseBranch) if err != nil { writeMergePackageReleasePRLine(stderr, err) - return true, 1 + return mergePackageReleasePRPassSettled, 1 } // Waiting here cannot help: the dispatcher release has to be merged, // published and stamped, which takes far longer than this command runs. @@ -220,26 +249,26 @@ func attemptMergePackageReleasePR( writeMergePackageReleasePRLine(stdout, fmt.Sprintf( "PR #%d stays draft: a dispatcher release pull request is open; the dispatcher must be released and stamped first.", releasePR.Number)) - return true, 0 + return mergePackageReleasePRPassSettled, 0 } } dispatcherTag, err := mergePackageReleasePRDispatcherTag(ctx, stdout, config, deps) if err != nil { writeMergePackageReleasePRLine(stderr, err) - return true, 1 + return mergePackageReleasePRPassSettled, 1 } pinnedTag, err := packageReleasePullRequestPinnedTag(ctx, config, releasePR, deps) if err != nil { writeMergePackageReleasePRLine(stderr, err) - return true, 1 + return mergePackageReleasePRPassSettled, 1 } if pinnedTag != dispatcherTag { writeMergePackageReleasePRLine(stdout, fmt.Sprintf( "PR #%d head %s still pins %s; waiting for release-please to rebase it onto the stamp.", releasePR.Number, releasePR.HeadRefOID, pinnedTag)) - return false, 0 + return mergePackageReleasePRPassAwaitingPin, 0 } // The draft flag is not consulted as evidence: it exists to keep people from // merging this pull request, not to prove its checks ran. What proves that @@ -247,10 +276,10 @@ func attemptMergePackageReleasePR( // held to that head by --match-head-commit at the merge. checksPassed, exitCode := packageReleasePullRequestChecksPassed(ctx, stdout, stderr, config, releasePR, deps) if exitCode != 0 { - return true, exitCode + return mergePackageReleasePRPassSettled, exitCode } if !checksPassed { - return false, 0 + return mergePackageReleasePRPassAwaitingChecks, 0 } return readyAndMergePackageReleasePR(ctx, stdout, stderr, config, releasePR, pinnedTag, deps) diff --git a/cli/release-automation/internal/automation/package_release_pr_merge_test.go b/cli/release-automation/internal/automation/package_release_pr_merge_test.go index edf6be50a7..6452d27ddb 100644 --- a/cli/release-automation/internal/automation/package_release_pr_merge_test.go +++ b/cli/release-automation/internal/automation/package_release_pr_merge_test.go @@ -373,6 +373,79 @@ func TestMergePackageReleasePRWithNoWaitLeavesStalePinDraftAfterOnePass(t *testi assertMergePackageReleasePRListCount(t, stub, 1) } +// Verifies --no-wait with a checks grace re-reads a head whose finished runs the run listing does not show yet, and merges once they appear. +func TestMergePackageReleasePRWithNoWaitMergesWhenChecksAppearWithinGrace(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: map[string]string{}, + }, + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + }, + }, []string{"--dispatcher-tag", "dispatcher-v3.4.0", "--no-wait", "--checks-grace-seconds", "90"}) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "has not finished for this head; waiting.") + assertMergePackageReleasePRMergeCount(t, stub, "package123", 1) + assertMergePackageReleasePRListCount(t, stub, 2) +} + +// Verifies --no-wait with a checks grace still leaves the pull request draft, without failing the job, when the runs never appear within the grace. +func TestMergePackageReleasePRWithNoWaitLeavesDraftWhenChecksStayMissingPastGrace(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: map[string]string{}, + }, + }, []string{"--dispatcher-tag", "dispatcher-v3.4.0", "--no-wait", "--checks-grace-seconds", "90"}) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "is not mergeable yet; leaving it draft.") + assertMergePackageReleasePRNeverMerged(t, stub) + // The 30-second interval fits three re-reads into the 90-second grace after the first pass. + assertMergePackageReleasePRListCount(t, stub, 4) +} + +// Verifies the checks grace does not apply to a stale pin: the stamp takes far longer than the grace, so --no-wait still decides in one pass. +func TestMergePackageReleasePRWithNoWaitIgnoresChecksGraceForStalePin(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.3.1"), + runs: packageReleasePRRunsAt("package123"), + }, + }, []string{"--dispatcher-tag", "dispatcher-v3.4.0", "--no-wait", "--checks-grace-seconds", "90"}) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "is not mergeable yet; leaving it draft.") + assertMergePackageReleasePRNeverMerged(t, stub) + assertMergePackageReleasePRListCount(t, stub, 1) +} + +// Verifies a negative checks grace is rejected rather than silently treated as no grace. +func TestMergePackageReleasePRRejectsNegativeChecksGrace(t *testing.T) { + exitCode, _, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + {prListJSON: packageReleasePRListJSON("package123", true)}, + }, []string{"--dispatcher-tag", "dispatcher-v3.4.0", "--no-wait", "--checks-grace-seconds", "-1"}) + + if exitCode != 1 { + t.Fatalf("expected exit code 1, got %d", exitCode) + } + assertReleasePRCheckLogContains(t, stderr, "--checks-grace-seconds must not be negative") + assertMergePackageReleasePRListCount(t, stub, 0) +} + // Verifies a failed merge is a success rather than a failed job when the pull request is already merged: the command reports it and exits 0. func TestMergePackageReleasePRAcceptsAPullRequestAnotherRunMerged(t *testing.T) { exitCode, stdout, stderr, stub := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ diff --git a/cli/release-automation/internal/automation/package_release_pr_ready_merge.go b/cli/release-automation/internal/automation/package_release_pr_ready_merge.go index 21bf12947d..dd0f780506 100644 --- a/cli/release-automation/internal/automation/package_release_pr_ready_merge.go +++ b/cli/release-automation/internal/automation/package_release_pr_ready_merge.go @@ -45,21 +45,21 @@ func readyAndMergePackageReleasePR( releasePR mergePackageReleasePullRequest, pinnedTag string, deps mergePackageReleasePRDeps, -) (settled bool, exitCode int) { +) (outcome mergePackageReleasePRPassOutcome, exitCode int) { if releasePR.IsDraft { settled, exitCode := readyPackageReleasePRBeforeMerge(ctx, stdout, stderr, config, releasePR, deps) if settled { - return true, exitCode + return mergePackageReleasePRPassSettled, exitCode } } mergeErr := squashMergePackageReleasePR(ctx, config, releasePR, deps) if mergeErr != nil { - return true, resolvePackageReleasePRMergeFailure(ctx, stdout, stderr, config, releasePR, mergeErr, deps) + return mergePackageReleasePRPassSettled, resolvePackageReleasePRMergeFailure(ctx, stdout, stderr, config, releasePR, mergeErr, deps) } writeMergePackageReleasePRLine(stdout, fmt.Sprintf( "Merged Unity package release PR #%d at %s; it pins %s.", releasePR.Number, releasePR.HeadRefOID, pinnedTag)) - return true, 0 + return mergePackageReleasePRPassSettled, 0 } // readyPackageReleasePRBeforeMerge lifts the draft. settled is false only when diff --git a/docs/dispatcher-pin-release-order.md b/docs/dispatcher-pin-release-order.md index 33c5aebb5b..37ca9565ce 100644 --- a/docs/dispatcher-pin-release-order.md +++ b/docs/dispatcher-pin-release-order.md @@ -46,6 +46,10 @@ and is merged only by automation; the other two are the ones people merge. dispatcher release pull request is open **and** the pin at the package head already records the dispatcher version `main`'s manifest releases. Otherwise it leaves the pull request draft and reconsiders on the next push to `main`. + The one wait it does allow is `--checks-grace-seconds`: GitHub's per-workflow + run listing can show a run as unfinished for tens of seconds after it + completed, so a head whose checks are not yet listed as finished is re-read + for up to two minutes before the pull request is left draft. 4. The **project runner** release pull request carries no cross-component ordering constraint and can be merged at any time.