From 3ebfdf2abf1d38e8c144aff49b7a1eec8d00773b Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:34:48 -0500 Subject: [PATCH] fix(ci): re-dispatch a pr-unblock activity the broker refused as workflow-modified An address-feedback round in flight when a lane workflow file changes on main gets its push or reply token refused as workflow-modified, a re-run keeps the old file, and the head's check runs then hold it. pr-merge's refire-refused answered with an empty commit on its schedule, which restarts both review lanes before any new round, and #13048 was dispatched by hand before that pass came. The half-hourly tick's feedback sweep now sends such a round again first: a fresh attempt-1 dispatch on main's files at the same head, with the head's newest review run as the upstream, as every sweep dispatch. It acts once per refused run (only the newest run of the activity for the PR counts), only when the refused run posted a check on the current head within FAILED_RUN_WINDOW_HOURS, and under SWEEP_HEAD_CAP rounds at that head. Check runs started before the refused run ended stop counting. The refusal is read by refire-refused's tokenRefusal, now exported, and refire-refused leaves address-feedback's workflow-modified runs to the sweep. The dispatched-run record names the refused run in refire_of_run_id. Co-Authored-By: Claude Opus 5.5 --- .../maintenance-monitor-signals/signals.mjs | 3 +- .github/scripts/pr-merge/refire-refused.mjs | 31 ++++++-- .../scripts/pr-merge/refire-refused.test.mjs | 7 ++ .../scripts/pr-unblock/feedback-dispatch.mjs | 73 ++++++++++++++++++- .../pr-unblock/feedback-dispatch.test.mjs | 72 ++++++++++++++++++ .github/workflows/pr-feedback-dispatch.yml | 15 ++-- 6 files changed, 185 insertions(+), 16 deletions(-) diff --git a/.github/scripts/maintenance-monitor-signals/signals.mjs b/.github/scripts/maintenance-monitor-signals/signals.mjs index 73890a1ee22..02ebdb610ac 100644 --- a/.github/scripts/maintenance-monitor-signals/signals.mjs +++ b/.github/scripts/maintenance-monitor-signals/signals.mjs @@ -111,7 +111,8 @@ export function ceilingFindings({ now, budgetReadings = [], jobsInUse = null }) } // Broker refusals from request-lane-token's annotations. workflow-modified is -// left out: pr-merge re-fires those runs on its schedule. +// left out: pr-merge re-fires those runs on its schedule, and the feedback +// sweep sends a refused address-feedback round again. const RATE_LIMITED = /github-rate-limited|rate limit|\b429\b/i; const BROKER_DENY = /^lane-(?:token|check)-denied: the broker answered (\d{3}) with reason ([a-z]+(?:-[a-z]+)*)/; const BROKER_UNREACHABLE = /^broker-unreachable:/; diff --git a/.github/scripts/pr-merge/refire-refused.mjs b/.github/scripts/pr-merge/refire-refused.mjs index 9350d49b4d6..f8229de1eb3 100755 --- a/.github/scripts/pr-merge/refire-refused.mjs +++ b/.github/scripts/pr-merge/refire-refused.mjs @@ -3,7 +3,9 @@ // on its current head failed at "Request the lane token" because the token // broker refused it as workflow-modified. A rerun keeps its first event's // workflow files, so no rerun can pass; after the new head, the next -// pr-unblock run uses main's files. No model. +// pr-unblock run uses main's files. No model. An address-feedback run refused +// that way is left to the feedback sweep, which dispatches it again on main's +// files at the same head (the lane's `dispatcherRefires`). // // The new head is an empty commit: the head's own tree, the head as its only // parent, made with the lanes App token through the Git Data API and moved @@ -113,6 +115,11 @@ const LANES = Object.freeze({ // review lanes finish a head, and a failed check run there holds the head // (pr-unblock/feedback-dispatch.mjs). transientActivities: ["address-feedback"], + // The activities whose workflow-modified refusal the half-hourly tick's + // feedback sweep sends again as a fresh dispatch on main + // (pr-unblock/feedback-dispatch.mjs sweepRefused), so no empty commit + // restarts both review lanes for them. + dispatcherRefires: ["address-feedback"], transientStarts: " The push starts no run itself: both review lanes review the new head, and an address-feedback run starts after them if an unresolved thread is left for it.", }, @@ -252,17 +259,27 @@ async function pullOfRun({ actions, repo, run, lane, byNumber, ids, appId, mainS } // Why this run's token step failed, when an empty commit can help, or null: -// `{kind: "workflow-modified", path}` for the workflow file the broker named, -// else, for a run of one of the lane's `transientActivities`, -// `{kind: "transient", label}` for a TRANSIENT failure. +// tokenRefusal's answer, except a workflow-modified refusal of one of the +// lane's `dispatcherRefires`, which a dispatcher sends again on main. async function refusedFile({ actions, repo, run, lane }) { const activity = DISPATCH_TITLE.exec(String(run.display_title ?? ""))?.[1]; - const transientOk = (lane.transientActivities ?? []).includes(activity); - let transient = null; const jobs = (await actions("GET", `/repos/${repo}/actions/runs/${run.id}/jobs?filter=latest&per_page=100`))?.jobs; if (!Array.isArray(jobs)) { throw new Error(`run ${run.id}'s jobs have an unexpected shape`); } + const readAnnotations = (id) => actions("GET", `/repos/${repo}/check-runs/${id}/annotations?per_page=100`); + const why = await tokenRefusal({ jobs, readAnnotations, lane, activity }); + return why?.kind === "workflow-modified" && (lane.dispatcherRefires ?? []).includes(activity) ? null : why; +} + +// Why a lane run's token step failed, read from its latest `jobs` and each +// failed write job's annotations (`readAnnotations(job id)`), or null: +// `{kind: "workflow-modified", path}` for the workflow file the broker named, +// else, for a run of one of the lane's `transientActivities`, +// `{kind: "transient", label}` for a TRANSIENT failure. +export async function tokenRefusal({ jobs, readAnnotations, lane = LANES["pr-unblock.yml"], activity }) { + const transientOk = (lane.transientActivities ?? []).includes(activity); + let transient = null; for (const job of jobs) { // Only a write runner job requests the token. The jobs API can list a // completed job's steps empty or cut short: such a job whose list lacks @@ -273,7 +290,7 @@ async function refusedFile({ actions, repo, run, lane }) { if (!failedAtToken || !Number.isSafeInteger(job.id)) { continue; } - const annotations = await actions("GET", `/repos/${repo}/check-runs/${job.id}/annotations?per_page=100`); + const annotations = await readAnnotations(job.id); for (const annotation of Array.isArray(annotations) ? annotations : []) { if (annotation.annotation_level !== "failure") { continue; diff --git a/.github/scripts/pr-merge/refire-refused.test.mjs b/.github/scripts/pr-merge/refire-refused.test.mjs index b09dd183c2b..d8d053b1fa6 100644 --- a/.github/scripts/pr-merge/refire-refused.test.mjs +++ b/.github/scripts/pr-merge/refire-refused.test.mjs @@ -685,6 +685,13 @@ test("a token 403 for any reason other than workflow-modified is not re-fired, a } }); +test("an address-feedback run refused as workflow-modified is left to the feedback sweep, which dispatches it again on main", async () => { + const w = refusedWorld([pr(9)], { runOptions: { title: FEEDBACK.title, jobName: FEEDBACK.jobName } }); + assert.equal(await main({ env, github: w.github, actions: w.actions, now: NOW }), 0); + assert.deepEqual(pushes(w.calls), []); + assert.deepEqual(posts(w.calls), []); +}); + test("a transient token failure of an activity pr-unblock-dispatch retries itself is left to that tick", async () => { for (const activity of ["resolve-conflicts", "review-dependency", "fix-ci"]) { const w = refusedWorld([pr(9)], { runOptions: { title: `pr-unblock ${activity} #9`, messages: [DENIED_5XX] } }); diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index b652989afad..bbef3b4c4b1 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -135,6 +135,12 @@ // EARLIER_FAIL_READS cap may have left it. Nothing records an answer, so a // head answered "no earlier FAIL" may be read again inside that window. // +// Before every other sweep, the tick sends again a round the token broker +// refused as workflow-modified, once per refused run (sweepRefused): a round +// in flight when a lane workflow file changes on main is refused its push or +// reply token, a re-run keeps the old file, and the head's check runs would +// otherwise hold it. +// // A run of this file an outage lost, or whose dispatch job failed, dispatched // nothing the lanes trust when re-run: check-trusted-trigger's dispatcher // check accepts a dispatcher at attempt 1 or re-run by a listed human, so @@ -175,7 +181,9 @@ import { pathToFileURL } from "node:url"; import { commitPulls, detailSuffix, errorLabel, responseDetail } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { countRequests } from "../../actions/log-api-budget/rate-guard.mjs"; +import { postingRunId } from "../../actions/report-check-run/posting-run.mjs"; import { LOOKBACK_HOURS, LOST } from "../maintenance-reconcile-lost-runs/infra-failure.mjs"; +import { tokenRefusal } from "../pr-merge/refire-refused.mjs"; import { anyListed, fixRequesters, HOLD_LINE, humanGatedLabelFrom, isFixRequest, MARKER_AUTHOR_ID, NO_ADOPTION, onReviewedHead, ownerAdoption, ownerAuthored, readTriggers, REQUEST_HISTORY_QUERY, requestHistoryOf, requestSkip, requestSkipLine, restMentions, reviewedHeads, reviewerIdsFrom, threadAuthors, trustedHumansFrom, VERIFY_NOTE_MARKER } from "./address-feedback.mjs"; // fix-ci-dispatch.mjs imports HOLD_LINE from here. @@ -1120,6 +1128,65 @@ async function dispatchAtHead({ api, pull, repository, appSenderId, log, setting return dispatched === null ? null : { ...dispatched, upstreamRunId: upstream.id }; } +// A round the token broker refused as workflow-modified (refire-refused.mjs +// tokenRefusal): it was in flight when a lane workflow file changed on main, +// so its push or reply job's token was refused, and a re-run keeps the old +// file. The tick, before its other sweeps, sends the activity again as a +// fresh attempt-1 dispatch on main's files at the same head, with the head's +// newest completed review run as the upstream, as every sweep dispatch. +// Once per refused run: only the newest run of the activity for the PR +// counts, so the run this sends, or any later one, ends it. It counts only +// when it failed within FAILED_RUN_WINDOW_HOURS, posted a check run on the +// PR's current head (one head per refusal), and that head has fewer than +// SWEEP_HEAD_CAP address-feedback check runs, which bounds a chain of +// refusals. Check runs started before the refused run ended stop counting +// (requestSince), but a limit stop keeps its reset hold. A refusal for any +// other reason is left alone. The result names the refused run (refireOf), +// which the dispatched-run record keeps. +async function sweepRefused({ api, repository, appSenderId, log, settings = DEFAULTS, ...rest }) { + const context = { api, repository, appSenderId, log, settings, rest }; + const since = (rest.now ?? Date.now()) - FAILED_RUN_WINDOW_HOURS * 3_600_000; + let runs; + try { + runs = await api.listWorkflowRuns(settings.workflow); + } catch (error) { + log(`::warning::${settings.workflow} run listing failed (${error.message}); no refused round is sent again this tick`); + return null; + } + const refired = await sweepPulls(context, (pull) => refireRefused({ ...context, pull }, runs, since), await openPulls(context)); + if (refired === null) { + log("sweep: no round the broker refused as workflow-modified to send again"); + } + return refired; +} + +async function refireRefused(context, runs, since) { + const { api, pull, log, settings } = context; + const head = pull.head.sha; + const [last] = runs + .filter((run) => run.display_title === runTitle(settings.activity, pull.number)) + .sort((a, b) => Date.parse(b.created_at ?? "") - Date.parse(a.created_at ?? "")); + if (last?.status !== "completed" || last.conclusion !== "failure" || !RUN_ID.test(String(last.id)) || !(Date.parse(last.updated_at ?? "") >= since)) { + return null; + } + const rounds = await api.listCheckRuns({ sha: head, checkName: settings.checkName, appId: settings.appId }); + const posted = (checks) => checks.some((check) => postingRunId(check) === String(last.id)); + if (!posted(rounds) && !posted(await api.listCheckRuns({ sha: head, checkName: settings.replyCheckName, appId: settings.appId }))) { + return null; + } + const why = await tokenRefusal({ jobs: await api.listRunJobs(last.id), readAnnotations: api.listAnnotations, activity: settings.activity }); + if (why?.kind !== "workflow-modified") { + return null; + } + if (rounds.length >= SWEEP_HEAD_CAP) { + log(`skip: #${pull.number}: run ${last.id} was refused as workflow-modified (${why.path}), and head ${head} has ${rounds.length} rounds, the sweep's cap`); + return null; + } + log(`refire: #${pull.number}: run ${last.id} was refused as workflow-modified (${why.path}) at head ${head}; sending ${settings.activity} again on main`); + const dispatched = await dispatchAtHead(context, { requestSince: Date.parse(last.updated_at) }); + return dispatched === null ? null : { ...dispatched, refireOf: String(last.id) }; +} + // This file's workflow, whose lost runs the tick recovers. export const DISPATCH_WORKFLOW = ".github/workflows/pr-feedback-dispatch.yml"; @@ -1503,6 +1570,8 @@ export function createGitHubApi({ token, repository, apiUrl, graphqlUrl, fetch: } return listing.jobs; }, + // A job's check run annotations, one page: the token step's refusal. + listAnnotations: (id) => request(`${repo}/check-runs/${encodeURIComponent(id)}/annotations?per_page=100`), async dispatchWorkflow({ workflow, ref, inputs }) { const result = await request(`${repo}/actions/workflows/${workflow}/dispatches`, "POST", { ref, @@ -1600,10 +1669,10 @@ export async function main(env, { api, github, resolveSlot, verifyCapped, log = return 0; } // A lost run is recovered only on a tick the released heads leave idle. - const sweep = async () => (await sweepReleased(common)) ?? recoverLost({ ...common, now }); + const sweep = async () => (await sweepRefused(common)) ?? (await sweepReleased(common)) ?? recoverLost({ ...common, now }); const swept = await sweep().finally(summary); if (swept !== null) { - appendFileSync(env.GITHUB_OUTPUT, `run-id=${swept.workflowRunId}\nupstream-run-id=${swept.upstreamRunId}\n`); + appendFileSync(env.GITHUB_OUTPUT, `run-id=${swept.workflowRunId}\nupstream-run-id=${swept.upstreamRunId}\n${swept.refireOf ? `refire-of=${swept.refireOf}\n` : ""}`); } return 0; } diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index da1a68e5276..c7f6b2e21c0 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -2236,6 +2236,78 @@ test("a lost run whose dispatch job never started or was not lost, or this run i } }); +// A round in flight when a lane workflow file changed on main: its push job's +// token step got the broker's workflow-modified refusal (request-lane-token's +// annotation), after its address-feedback check posted on the head. +const REFUSED_RUN = 700; +const WORKFLOW_MODIFIED = "lane-token-denied: the broker answered 403 with reason workflow-modified: .github/workflows/pr-unblock.yml differs from refs/heads/main"; +const refusedRun = (overrides = {}) => ({ + id: REFUSED_RUN, + display_title: TITLE_42, + status: "completed", + conclusion: "failure", + created_at: "2026-10-17T13:00:00Z", + updated_at: "2026-10-17T13:40:00Z", + ...overrides, +}); +const refusedRound = (run = REFUSED_RUN) => ({ + ...roundRun("address-feedback", "success", "Passed", "2026-10-17T13:05:00Z"), + external_id: `pr-unblock:address-feedback:${run}:1`, +}); +const tokenJob = { id: 9001, name: "apply-feedback-patch / run", conclusion: "failure", steps: [{ name: "Request the lane token", conclusion: "failure" }] }; +function refusedApi({ runs = [refusedRun()], checks = [refusedRound()], message = WORKFLOW_MODIFIED, ...options } = {}) { + const api = sweepApi({ checksAt: { [HEAD]: checks }, workflowRuns: runs, ...options }); + api.jobReads = []; + api.listRunJobs = async (id) => { + api.jobReads.push(id); + return [tokenJob]; + }; + api.listAnnotations = async (id) => (id === tokenJob.id ? [{ annotation_level: "notice", message: "token=github-token" }, { annotation_level: "failure", message }] : []); + return api; +} + +test("a tick sends a round the broker refused as workflow-modified again once, on main, and the record names the refused run", async (t) => { + const api = refusedApi(); + const { output, lines } = await sweepMain(t, api, TICK, NOW); + assert.deepEqual(api.jobReads, [REFUSED_RUN]); + assert.deepEqual(api.calls.dispatch, [ + { workflow: "pr-unblock.yml", ref: "refs/heads/main", inputs: { "pr-number": "42", activity: "address-feedback", "dispatcher-run-id": RUN_ID } }, + ]); + assert.equal(output, `run-id=38779200001\nupstream-run-id=13\nrefire-of=${REFUSED_RUN}\n`); + assert.ok(lines.some((line) => line.startsWith("refire: #42: run 700 was refused as workflow-modified (.github/workflows/pr-unblock.yml)")), lines.join("\n")); +}); + +test("a refused round already sent again, by the tick or a person, is not sent a second time", async (t) => { + for (const [label, later] of [ + ["the refire is still going", refusedRun({ id: 701, status: "in_progress", conclusion: null, created_at: "2026-10-17T13:45:00Z" })], + ["the refire was refused as well", refusedRun({ id: 701, created_at: "2026-10-17T13:45:00Z", updated_at: "2026-10-17T13:50:00Z" })], + ["the refire passed", refusedRun({ id: 701, conclusion: "success", created_at: "2026-10-17T13:45:00Z" })], + ]) { + const api = refusedApi({ runs: [later, refusedRun()] }); + const { output } = await sweepMain(t, api, TICK, NOW); + assert.deepEqual(api.jobReads, [], label); + assert.deepEqual(api.calls.dispatch, [], label); + assert.equal(output, "", label); + } +}); + +test("a round refused for another reason, outside the window, on another head or at the cap is not sent again", async (t) => { + for (const [label, options, reads] of [ + ["a kill-switch 403", { message: "lane-token-denied: the broker answered 403 with reason kill-switch" }, [REFUSED_RUN]], + ["a broker 5xx", { message: "lane-token-denied: the broker answered 503 with reason broker-error" }, [REFUSED_RUN]], + ["another workflow file", { message: WORKFLOW_MODIFIED.replace("pr-unblock.yml", "pr-review-hosted.yml") }, [REFUSED_RUN]], + ["refused hours ago", { runs: [refusedRun({ updated_at: "2026-10-17T11:00:00Z" })] }, []], + ["the head moved since", { checks: [] }, []], + ["the head is at the cap", { checks: [refusedRound(), answeredRound, answeredRound] }, [REFUSED_RUN]], + ]) { + const api = refusedApi(options); + const { output } = await sweepMain(t, api, TICK, NOW); + assert.deepEqual(api.jobReads, reads, label); + assert.deepEqual(api.calls.dispatch, [], label); + assert.equal(output, "", label); + } +}); + // A verdict the kill switch deferred: the reconciler re-ran post-verify-notes // (attempt 2, github-actions[bot]), which posted the notes or found the stale // ones and uploaded pr-feedback-deferred--, since the gate diff --git a/.github/workflows/pr-feedback-dispatch.yml b/.github/workflows/pr-feedback-dispatch.yml index c28ff04ea43..02b5ba2201c 100644 --- a/.github/workflows/pr-feedback-dispatch.yml +++ b/.github/workflows/pr-feedback-dispatch.yml @@ -339,7 +339,7 @@ jobs: permissions: contents: read # check out main pull-requests: read # the PR, its comments and review threads - checks: read # skip a head that already has an address-feedback check run + checks: read # skip a head that already has an address-feedback check run; a refused round's annotations statuses: read # the sweep's read of a head's pr-unblock/address-feedback stop actions: write # list pr-unblock runs; dispatch pr-unblock.yml env: @@ -461,20 +461,23 @@ jobs: # The record of the run this run dispatched and the review run that # started it: an artifact only steps of this run can upload, read by the - # gate and the broker with one list call and no download. + # gate and the broker with one list call and no download. A round sent + # again after a workflow-modified refusal also names the refused run + # (refire_of_run_id). - name: Name the dispatched-run record id: record if: always() && steps.dispatch.outputs.run-id != '' env: RUN_ID: ${{ steps.dispatch.outputs.run-id }} UPSTREAM_RUN_ID: ${{ steps.dispatch.outputs.upstream-run-id }} + REFIRE_OF: ${{ steps.dispatch.outputs.refire-of }} run: | - if [[ ! "$RUN_ID" =~ ^[1-9][0-9]{0,14}$ || ! "$UPSTREAM_RUN_ID" =~ ^[1-9][0-9]{0,14}$ ]]; then - echo "::error::run-id or the upstream run id is not a run id" + if [[ ! "$RUN_ID" =~ ^[1-9][0-9]{0,14}$ || ! "$UPSTREAM_RUN_ID" =~ ^[1-9][0-9]{0,14}$ || ! "$REFIRE_OF" =~ ^([1-9][0-9]{0,14})?$ ]]; then + echo "::error::run-id, the upstream run id or refire-of is not a run id" exit 1 fi - printf '{"dispatcher_run_id":%s,"upstream_run_id":%s,"run_id":%s}\n' \ - "$GITHUB_RUN_ID" "$UPSTREAM_RUN_ID" "$RUN_ID" >"$RUNNER_TEMP/pr-feedback-dispatched.json" + printf '{"dispatcher_run_id":%s,"upstream_run_id":%s,"run_id":%s%s}\n' \ + "$GITHUB_RUN_ID" "$UPSTREAM_RUN_ID" "$RUN_ID" "${REFIRE_OF:+,\"refire_of_run_id\":$REFIRE_OF}" >"$RUNNER_TEMP/pr-feedback-dispatched.json" echo "name=pr-feedback-dispatched-$UPSTREAM_RUN_ID-$RUN_ID" >>"$GITHUB_OUTPUT" - name: Scan the upload for credentials