diff --git a/.changes/pr-pipeline/fix-retry-lost-main-run-reviews-7c41e0b9.md b/.changes/pr-pipeline/fix-retry-lost-main-run-reviews-7c41e0b9.md new file mode 100644 index 0000000000..e2b8780499 --- /dev/null +++ b/.changes/pr-pipeline/fix-retry-lost-main-run-reviews-7c41e0b9.md @@ -0,0 +1,10 @@ +--- +bump: patch +--- + +### Fixed + +- A pr-review-main run whose first attempt failed before any lane's verdict write started (every + credential leg stopped) is now re-run once, recorded, by the lost-run reconciler, as a lost + pr-verify run is, so a lane-source pull request no longer waits for its next push to be + reviewed from main. diff --git a/.github/scripts/maintenance-reconcile-lost-runs/fixtures/jobs-40892239796.json b/.github/scripts/maintenance-reconcile-lost-runs/fixtures/jobs-40892239796.json new file mode 100644 index 0000000000..13170c7c76 --- /dev/null +++ b/.github/scripts/maintenance-reconcile-lost-runs/fixtures/jobs-40892239796.json @@ -0,0 +1,365 @@ +{ + "jobs": [ + { + "conclusion": "failure", + "id": 123511844321, + "name": "review (claude-security-review, security-review, review-security, CLAUDE_SECURITY_REVIEW_DISABLED)", + "status": "completed", + "steps": [ + { + "conclusion": "success", + "name": "Set up job", + "number": 1, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Check out the default branch tip", + "number": 2, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Set up Node", + "number": 3, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Check the kill switch", + "number": 4, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Report the kill switch", + "number": 5, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Check the trigger", + "number": 6, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Select a lane-source pull request", + "number": 7, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Write the diff", + "number": 8, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Select the trusted PR text", + "number": 9, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Copy the review criteria", + "number": 10, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Resolve the review's model", + "number": 11, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Check the credential", + "number": 12, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Apply the Claude managed settings", + "number": 13, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Isolate the runner", + "number": 14, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Write the prompt and schema", + "number": 15, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Review the pull request", + "number": 16, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Report the review's outcome", + "number": 17, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Upload the sealed transcript", + "number": 18, + "status": "completed" + }, + { + "conclusion": "skipped", + "name": "Write the verdict", + "number": 19, + "status": "completed" + }, + { + "conclusion": "skipped", + "name": "Upload the verdict", + "number": 20, + "status": "completed" + }, + { + "conclusion": "skipped", + "name": "Summarize the verdict", + "number": 21, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Log the API budget", + "number": 22, + "status": "completed" + }, + { + "conclusion": "failure", + "name": "Fail without a verdict", + "number": 23, + "status": "completed" + }, + { + "conclusion": "skipped", + "name": "Post Set up Node", + "number": 45, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Post Check out the default branch tip", + "number": 46, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Complete job", + "number": 47, + "status": "completed" + } + ] + }, + { + "conclusion": "failure", + "id": 123511844887, + "name": "review (claude-review, code-review, review-code, CLAUDE_REVIEW_DISABLED)", + "status": "completed", + "steps": [ + { + "conclusion": "success", + "name": "Set up job", + "number": 1, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Check out the default branch tip", + "number": 2, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Set up Node", + "number": 3, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Check the kill switch", + "number": 4, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Report the kill switch", + "number": 5, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Check the trigger", + "number": 6, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Select a lane-source pull request", + "number": 7, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Write the diff", + "number": 8, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Select the trusted PR text", + "number": 9, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Copy the review criteria", + "number": 10, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Resolve the review's model", + "number": 11, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Check the credential", + "number": 12, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Apply the Claude managed settings", + "number": 13, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Isolate the runner", + "number": 14, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Write the prompt and schema", + "number": 15, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Review the pull request", + "number": 16, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Report the review's outcome", + "number": 17, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Upload the sealed transcript", + "number": 18, + "status": "completed" + }, + { + "conclusion": "skipped", + "name": "Write the verdict", + "number": 19, + "status": "completed" + }, + { + "conclusion": "skipped", + "name": "Upload the verdict", + "number": 20, + "status": "completed" + }, + { + "conclusion": "skipped", + "name": "Summarize the verdict", + "number": 21, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Log the API budget", + "number": 22, + "status": "completed" + }, + { + "conclusion": "failure", + "name": "Fail without a verdict", + "number": 23, + "status": "completed" + }, + { + "conclusion": "skipped", + "name": "Post Set up Node", + "number": 45, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Post Check out the default branch tip", + "number": 46, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Complete job", + "number": 47, + "status": "completed" + } + ] + }, + { + "conclusion": "success", + "id": 123512264027, + "name": "record", + "status": "completed", + "steps": [ + { + "conclusion": "success", + "name": "Set up job", + "number": 1, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Download this run's verdicts", + "number": 2, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Record the counts", + "number": 3, + "status": "completed" + }, + { + "conclusion": "success", + "name": "Complete job", + "number": 4, + "status": "completed" + } + ] + } + ], + "total_count": 3 +} diff --git a/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs b/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs index b60a30230c..9725a38802 100755 --- a/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs +++ b/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs @@ -22,11 +22,12 @@ // flaky test the diff did not cause can pass. pr-unblock's fix-ci does not // wait for this re-run unless it is already queued or going, so it may run // beside a fix-ci run (CI minutes, no model). -// - Lane: a pr-verify run whose first attempt ended as above or failure (a -// judgment that broke, or a run the kill switch stopped, which fails), and -// whose verdict write never started: a judgment that finished is never taken -// again, so a FAIL the write or upload then lost is held for a person, with -// the reason in the summary. Its trigger check trusts a re-run +// - Lane: a pr-verify or pr-review-main run whose first attempt ended as above +// or failure (a judgment that broke, a review every credential leg stopped +// on, or a pr-verify run the kill switch stopped, which fails), and whose +// verdict write never started in any job: a judgment that finished is never +// taken again, so a FAIL the write or upload then lost is held for a person, +// with the reason in the summary. Its trigger check trusts a re-run // github-actions[bot] starts only through this workflow's record of it, the // artifact rerunRecordName(run id, head SHA) (check-trusted-trigger's README). The // plan step names each record, the workflow uploads them, and only then does @@ -59,7 +60,7 @@ // ci-status. Re-run as a Switch is, with no lookback; a re-run that fails is // a warning, not a failed run, since the head's next push or a person's // re-run still clears it. -// Other lost lane runs (the review lanes, refine) are listed in the summary for +// Other lost lane runs (the pull_request review lanes, refine) are listed in the summary for // a person: their gates or the lanes token broker refuse a re-run // github-actions[bot] starts. Their own paths recover them instead: the babysit // merge gate re-runs a lost review run under the owner's token, and the @@ -71,8 +72,8 @@ // Nothing is read from GitHub while githubstatus.com shows Actions as anything // but operational, or the status cannot be read, so a re-run never adds load // mid-incident. At most MAX_IN_FLIGHT re-runs are in flight at once (PR CI -// sets and pr-verify runs, counting those already running): merge-critical PRs -// (auto-merge set) first, then oldest PR, then the oldest pr-verify runs. +// sets and recorded lane runs, counting those already running): merge-critical PRs +// (auto-merge set) first, then oldest PR, then the oldest lane runs. // Writes are at least WRITE_GAP_MS apart. The first rate-limit error stops the // run. // @@ -133,10 +134,6 @@ export const CI_WORKFLOWS = new Set([ ".github/workflows/pr-test-windows.yml", ".github/workflows/pr-check-managed-files-hosted.yml", ]); -// Lane workflows whose trigger check takes this workflow's record -// (reconciler-workflow) and that mint no broker token. Each starts on -// workflow_run from main, so its runs are listed by workflow, not by PR head. -export const RECORDED_LANES = [".github/workflows/pr-verify.yml"]; // The CI workflows a recorded lane runs after on workflow_run. A re-run that // makes attempt 2 of one is recorded too: the lane trusts that attempt's // github-actions[bot] only through its record. A later attempt gets none, and @@ -183,6 +180,17 @@ export const JUDGE_STEP = "Judge the pull request"; export const JUDGE_PREDECESSOR = "Write the prompt and schema"; // The step after JUDGE_STEP, which runs only on the judgment's true outcome. export const WRITE_STEP = "Write the verdict"; +// pr-review-main's judge step, between the same predecessor and verdict write. +export const REVIEW_STEP = "Review the pull request"; +// Lane workflows whose trigger check takes this workflow's record +// (reconciler-workflow) and that mint no broker token, each with its judge +// step. Each starts on workflow_run from main, so its runs are listed by +// workflow, not by PR head; the trigger check binds the re-run to the one open +// PR whose head is still the upstream run's. +export const RECORDED_LANES = new Map([ + [".github/workflows/pr-verify.yml", JUDGE_STEP], + [".github/workflows/pr-review-main.yml", REVIEW_STEP], +]); export const MAX_IN_FLIGHT = 3; export const WRITE_GAP_MS = 1000; // Pages of 100 read per status query of a lane's run listing; a listing that @@ -344,7 +352,7 @@ const NOT_STARTED = "before any of its jobs started"; // Why the run's first attempt must not be re-run, or undefined when it may be: // no job started any step, or a verdict write started. An unreadable listing // throws. -async function holdReason(call, repo, runId) { +async function holdReason(call, repo, runId, judge) { const listing = await call("GET", `/repos/${repo}/actions/runs/${runId}/attempts/1/jobs?per_page=100`); if (!Array.isArray(listing?.jobs) || listing.total_count > 100) { throw new Error("its job listing is not one complete page"); @@ -362,7 +370,7 @@ async function holdReason(call, repo, runId) { .filter((s) => s?.conclusion != null && s.conclusion !== "skipped"); // An annotation can be echoed text, so a cut list that reaches the judge // step or its predecessor stays held rather than read as a lost runner. - if (cut.some((job) => job.steps?.some((s) => s?.name === JUDGE_PREDECESSOR || s?.name === JUDGE_STEP))) { + if (cut.some((job) => job.steps?.some((s) => s?.name === JUDGE_PREDECESSOR || s?.name === judge))) { return "after its judge step may have started"; } // The judge step is continue-on-error, so it lists success even when it @@ -670,7 +678,7 @@ async function plan({ repo, call, say, since, now, readStatus, judgeSpec, laneAp ready.sort((a, b) => position.get(a[0].pr) - position.get(b[0].pr)); const created = encodeURIComponent(`>=${new Date(since).toISOString().replace(/\.\d{3}Z$/, "Z")}`); const lanes = []; - for (const lanePath of RECORDED_LANES) { + for (const [lanePath, judge] of RECORDED_LANES) { try { const file = lanePath.slice(lanePath.lastIndexOf("/") + 1); const byId = new Map(); @@ -696,7 +704,7 @@ async function plan({ repo, call, say, since, now, readStatus, judgeSpec, laneAp const unpublished = []; for (const r of runs.filter((run) => isLostLane(run, since))) { try { - const held = await holdReason(call, repo, r.id); + const held = await holdReason(call, repo, r.id, judge); if (held) { const why = held === NOT_STARTED ? "; a newer run judges the head, so it is not re-run" : "; not re-run"; say(`lane run ${r.id} (${r.path}) ended ${r.conclusion} ${held}${why}`); @@ -707,13 +715,11 @@ async function plan({ repo, call, say, since, now, readStatus, judgeSpec, laneAp noteFailure(`lane run ${r.id}`, error); } } - // The record's head is the pr-verify run's own head_sha (main's tip), not + // The record's head is the lane run's own head_sha (main's tip), not // the pull request's head: the PR binding is the live gate's job // (check-trusted-trigger and resolve-pr), never this record's. lanes.push( - ...unpublished - .sort(byAge) - .map((r) => ({ kind: "lane", id: r.id, path: r.path, conclusion: r.conclusion, head: r.head_sha, created_at: r.created_at, record: rerunRecordName(r.id, r.head_sha) })), + ...unpublished.map((r) => ({ kind: "lane", id: r.id, path: r.path, conclusion: r.conclusion, head: r.head_sha, created_at: r.created_at, record: rerunRecordName(r.id, r.head_sha) })), ); } catch (error) { noteFailure(lanePath, error); @@ -765,7 +771,7 @@ async function plan({ repo, call, say, since, now, readStatus, judgeSpec, laneAp say(`#${set[0].pr}: deferred, ${MAX_IN_FLIGHT} re-runs in flight`); } } - for (const lane of lanes) { + for (const lane of lanes.sort(byAge)) { if (room > 0) { items.push(lane); room -= 1; diff --git a/.github/scripts/maintenance-reconcile-lost-runs/reconcile.test.mjs b/.github/scripts/maintenance-reconcile-lost-runs/reconcile.test.mjs index 93b29686b1..15076fbcf6 100644 --- a/.github/scripts/maintenance-reconcile-lost-runs/reconcile.test.mjs +++ b/.github/scripts/maintenance-reconcile-lost-runs/reconcile.test.mjs @@ -19,7 +19,7 @@ import { test } from "node:test"; import { createGitHub, GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { createRateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; -import { GITHUB_STATUS_URL, JUDGE_PREDECESSOR, JUDGE_STEP, KILL_SWITCH_NOTE, main, NPM_STATUS_URL, PR_UNBLOCK_STOP_NOTE, prefilterSpec, WRITE_STEP } from "./reconcile.mjs"; +import { GITHUB_STATUS_URL, JUDGE_PREDECESSOR, JUDGE_STEP, KILL_SWITCH_NOTE, main, NPM_STATUS_URL, PR_UNBLOCK_STOP_NOTE, prefilterSpec, REVIEW_STEP, WRITE_STEP } from "./reconcile.mjs"; import { switchHold } from "../pr-merge/merge-holds.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; @@ -30,6 +30,7 @@ const CHECKS = ".github/workflows/pr-require-checks.yml"; const WINDOWS = ".github/workflows/pr-test-windows.yml"; const REVIEW = ".github/workflows/pr-review-hosted.yml"; const VERIFY = ".github/workflows/pr-verify.yml"; +const MAIN_REVIEW = ".github/workflows/pr-review-main.yml"; const MAIN_TIP = "e".repeat(40); const sha = (n) => String(n).padStart(40, "0"); @@ -501,9 +502,9 @@ test("at most 3 verify runs are recorded and re-run in one run", async () => { assert.deepEqual(records(outputs), [201, 202, 203].map((id) => `reconcile-rerun-${id}-2-${MAIN_TIP}`)); }); -// pr-verify's jobs, each a leg (key, subscription, fallback), as name and body. -const verifyLegs = () => { - const workflow = readFileSync(new URL("../../workflows/pr-verify.yml", import.meta.url), "utf8"); +// A lane workflow's jobs (pr-verify's legs by default), as name and body. +const verifyLegs = (file = "pr-verify.yml") => { + const workflow = readFileSync(new URL(`../../workflows/${file}`, import.meta.url), "utf8"); const jobs = workflow.slice(workflow.search(/^jobs:$/m)); return [...jobs.matchAll(/^ {2}([a-z][a-z0-9-]*):\n((?: {4,}.*\n|\s*\n)+)/gm)].map(([, name, body]) => ({ name, body })); }; @@ -530,6 +531,25 @@ test("pr-verify still names its judge step as the reconciler reads it, in every } }); +// A review leg whose step names drifted would lose its hold, and one whose +// trigger check took no reconciler record would refuse every re-run. +test("pr-review-main still names its review and write steps as the reconciler reads them, and takes its record, in every review leg", () => { + const legs = verifyLegs("pr-review-main.yml").filter(({ body }) => /^ {6}- name: Review the pull request$/m.test(body)); + assert.equal(legs.length, 3); + for (const { name, body } of legs) { + const steps = legSteps(body); + const names = steps.map((s) => s.match(/^ {6}- name: (.+)$/m)[1]); + const review = steps.findIndex((s) => /^ {8}id: review$/m.test(s)); + assert.equal(names[review], REVIEW_STEP, name); + assert.equal(names[review - 1], JUDGE_PREDECESSOR, name); + const writes = steps.filter((s) => s.startsWith(` - name: ${WRITE_STEP}\n`)); + assert.equal(writes.length, 1, name); + assert.match(writes[0], /\n[ ]+if: steps\.review\.outcome == 'success'/, name); + const trigger = steps.find((s) => s.startsWith(" - name: Check the trigger\n")); + assert.match(trigger, /\n[ ]+reconciler-workflow: \.github\/workflows\/maintenance-reconcile-lost-runs\.yml\n/, name); + } +}); + test("a verify run lost during its judge step is recorded and re-run once", async () => { for (const conclusion of ["failure", "cancelled"]) { const { code, reruns, outputs } = await reconcile({ pulls: [], runs: [{ ...verifyRun(201), jobs: verifyJob(conclusion) }] }); @@ -813,6 +833,66 @@ test("a verify run whose jobs cannot be read is not re-run", async () => { assert.deepEqual(records(outputs), ["", "", ""]); }); +// pr-review-main run 40892239796's first-attempt jobs: both lanes reviewed and +// stopped on a credit stop, so the verdict write skipped and "Fail without a +// verdict" failed each job; the run ended failure with no verdict. +const MAIN_REVIEW_JOBS = fixture("jobs-40892239796.json").jobs; +const mainReviewRun = (id, opts = {}, jobs = MAIN_REVIEW_JOBS) => ({ + ...run(id, MAIN_TIP, { path: MAIN_REVIEW, event: "workflow_run", conclusion: "failure", ...opts }), + head_branch: "main", + jobs, +}); + +test("a main-run review that ended failure with no verdict is recorded and re-run once", async () => { + const { code, reruns, outputs } = await reconcile({ pulls: [], runs: [mainReviewRun(301)] }); + assert.equal(code, 0); + assert.deepEqual(reruns, [301]); + assert.deepEqual(records(outputs), [`reconcile-rerun-301-2-${MAIN_TIP}`, "", ""]); +}); + +test("a main-run review at attempt 2, or none of whose jobs started, is never re-run or recorded", async () => { + const notStarted = MAIN_REVIEW_JOBS.map((job) => ({ ...job, conclusion: "cancelled", steps: [] })); + for (const lost of [mainReviewRun(301, { attempt: 2 }), mainReviewRun(301, { conclusion: "cancelled" }, notStarted)]) { + const { code, reruns, outputs } = await reconcile({ pulls: [], runs: [lost] }); + assert.equal(code, 0); + assert.deepEqual(reruns, []); + assert.deepEqual(records(outputs), ["", "", ""]); + } +}); + +// One lane wrote its verdict before the run failed: a re-run would review the +// head again, so the run is held, as a pr-verify run is. +test("a main-run review whose verdict write started in any lane is held with its reason", async () => { + const [first, ...rest] = MAIN_REVIEW_JOBS; + const wrote = { ...first, steps: first.steps.map((s) => (s.name === WRITE_STEP ? { ...s, conclusion: "success" } : s)) }; + const dir = tempDir("reconcile-summary-"); + try { + const summary = path.join(dir, "summary.md"); + writeFileSync(summary, ""); + const { code, reruns, outputs } = await reconcile({ pulls: [], runs: [mainReviewRun(301, {}, [wrote, ...rest])], env: { GITHUB_STEP_SUMMARY: summary } }); + assert.equal(code, 0); + assert.deepEqual(reruns, []); + assert.deepEqual(records(outputs), ["", "", ""]); + assert.match(readFileSync(summary, "utf8"), /lane run 301 \(\.github\/workflows\/pr-review-main\.yml\) ended failure after its verdict write started/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("a main-run review re-run shares the cap of 3 with pr-verify re-runs, oldest first", async () => { + const { reruns, outputs } = await reconcile({ + pulls: [], + runs: [ + verifyRun(201, { created: "2026-10-19T19:00:00Z" }), + mainReviewRun(301, { created: "2026-10-19T18:00:00Z" }), + verifyRun(202, { created: "2026-10-19T19:10:00Z" }), + mainReviewRun(302, { created: "2026-10-19T19:20:00Z" }), + ], + }); + assert.deepEqual(reruns, [301, 201, 202]); + assert.deepEqual(records(outputs), [301, 201, 202].map((id) => `reconcile-rerun-${id}-2-${MAIN_TIP}`)); +}); + test("a non-operational Actions status records nothing", async () => { const { outputs, calls } = await reconcile({ pulls: [], runs: [verifyRun(201)], status: statusWith("major_outage") }); assert.deepEqual(calls, []); @@ -1305,9 +1385,10 @@ test("passed heads whose spec nobody touched since their checks ran cost one Gra const count = (part) => reads.filter((r) => r.includes(part)).length; assert.equal(count("/actions/artifacts?"), 0); assert.equal(count("POST /graphql"), 1); - // The pulls page, one run listing per head, nine pr-verify status - // listings, one intake listing and the one pre-filter query. - assert.equal(reads.length, 1 + numbers.length + 9 + 1 + 1); + // The pulls page, one run listing per head, nine status listings for each + // of pr-verify and pr-review-main, one intake listing and the one + // pre-filter query. + assert.equal(reads.length, 1 + numbers.length + 2 * 9 + 1 + 1); }); }); diff --git a/.github/workflows/maintenance-reconcile-lost-runs.yml b/.github/workflows/maintenance-reconcile-lost-runs.yml index 8f2ca2e790..067f871b62 100644 --- a/.github/workflows/maintenance-reconcile-lost-runs.yml +++ b/.github/workflows/maintenance-reconcile-lost-runs.yml @@ -3,7 +3,7 @@ name: maintenance-reconcile-lost-runs # Re-runs PR CI and verify runs a GitHub outage ended without a result, so # nobody has to re-run them by hand. Every 30 minutes it looks at each open, # non-draft pull request into main from a branch of this repository, and at -# the last day's pr-verify runs. A CI workflow whose newest run on the PR's +# the last day's pr-verify and pr-review-main runs. A CI workflow whose newest run on the PR's # current head, or a pr-verify run, ended cancelled, timed_out or # startup_failure, on its first attempt, is re-run once with this job's own # token. The attempt number is the record: a run at attempt 2 was already @@ -13,7 +13,9 @@ name: maintenance-reconcile-lost-runs # is off. A pr-verify run whose verdict write started (it runs only when the # judgment succeeded) is not re-run, even when the write or upload failed, so # a head is never judged twice; the summary names the hold. A pr-verify run none of whose jobs started is not -# re-run: a newer run for the same head judges it. +# re-run: a newer run for the same head judges it. A pr-review-main run is +# taken the same way: one that ended failure with no lane's verdict write +# started (every credential leg stopped) is re-run once, recorded. # # A CI run on the PR's current head that ended failure only from an outside # outage (each failed job hit its time limit, failed in an install step, or @@ -50,9 +52,9 @@ name: maintenance-reconcile-lost-runs # untriaged; otherwise, and once a dispatch triages it, the issue stays without # the floor label. # -# pr-verify's trigger check trusts a re-run github-actions[bot] starts only +# pr-verify's and pr-review-main's trigger checks trust a re-run github-actions[bot] starts only # through this run's record of it: the plan step names an artifact -# reconcile-rerun--2- for each pr-verify run it will re-run, +# reconcile-rerun--2- for each pr-verify or pr-review-main run it will re-run, # and for each pr-require-checks run whose re-run makes its attempt 2 (pr-verify # runs after it), the upload steps below write them, and only then does the act # step re-run those runs. That no other run can upload into this run's artifacts is @@ -65,7 +67,7 @@ name: maintenance-reconcile-lost-runs # It does nothing while githubstatus.com shows Actions as anything but # operational, or the page cannot be read, so it adds no load mid-incident. At # most 3 re-runs are in flight at once, auto-merge PRs first, and its writes -# are at least a second apart. A lost review or refine lane run is listed in +# are at least a second apart. A lost pull_request review or refine lane run is listed in # the run summary for a person: those lanes, or the lanes token broker, refuse # a re-run github-actions[bot] starts. No model, and no PR code is checked out. # Rules: .github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs. @@ -141,7 +143,7 @@ jobs: export VERIFY_AUTHORS LANE_IDENTITY SPEC_FROM_BODY_APPS node .base/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs - # One record per pr-verify run, and per pr-require-checks run made attempt + # One record per pr-verify or pr-review-main run, and per pr-require-checks run made attempt # 2, the act step re-runs, uploaded first. A failed upload fails the job, # so that run is not re-run. - name: Record re-run 1 diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index a19b4f9048..4c9e9a509c 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -35,8 +35,10 @@ name: pr-review-main # leg writes the same verdict, so the record job and the gate never see # which leg ran; the lane record names the credential and why it ran. A # lane no leg leaves a verdict for (a kill switch, a failure, the last leg -# stopping) stays unbound, as it was before this workflow, and the next push -# reviews again. +# stopping) stays unbound, as it was before this workflow. A run that failed +# before any lane's verdict write started is re-run once, recorded, by the +# lost-run reconciler (maintenance-reconcile-lost-runs.yml); otherwise the +# next push reviews again. # # Every local action runs from the main checkout at ./.base. zizmor's # self-repository advice (`$/...`) is ignored on those lines, as in diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 624b413f81..841ed0d0b5 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1127,8 +1127,10 @@ the rules that only tighten in code. Every leg writes the same verdict, so neither the record job nor the gate depends on the leg; the lane record (`report-lane-outcome`) names the credential and why the leg ran. A handed-off leg stays green, so a later leg's verdict still - comes from a successful run; a run that leaves no verdict leaves the lanes unbound until the - next push. + comes from a successful run. A run whose first attempt failed before any lane's verdict write + started (every leg stopped) is re-run once, recorded, by the lost-run reconciler, as a lost + pr-verify run is (`maintenance-reconcile-lost-runs`); any other run that leaves no verdict + leaves the lanes unbound until the next push. `merge.diff-check.denied-paths` (empty here) joins the person classes. `test_merge_holds_sync.py` derives the JS and Python import closures and requires them on the