diff --git a/.changes/pr-pipeline/fix-pr-unblock-auth-stop-waits-3e9b41d7.md b/.changes/pr-pipeline/fix-pr-unblock-auth-stop-waits-3e9b41d7.md new file mode 100644 index 0000000000..5b2303c6c1 --- /dev/null +++ b/.changes/pr-pipeline/fix-pr-unblock-auth-stop-waits-3e9b41d7.md @@ -0,0 +1,8 @@ +--- +bump: patch +--- + +### Fixed + +- A pr-unblock run whose credential was refused (`class=auth`, a 401) now counts as a limit stop: its head waits `limits.fallback-reset` hours before the next dispatch instead of spending all `limits.limit-stops-per-head` runs at once, and the dispatcher logs a notice naming the class and the reset time (`.github/scripts/pr-unblock/limit-stop.mjs`, `dispatch.mjs`). +- `pr-run-activity-read.yml` now puts the lane record's class and reset on the check run it posts, so `limit-stop.mjs` can tell a usage limit or a refused credential from any other failure on resolve-conflicts, address-feedback and review-dependency checks; before, every such check read as a plain failure. diff --git a/.github/actions/resolve-config/callers.test.mjs b/.github/actions/resolve-config/callers.test.mjs index 569ee0bfec..d78bffbcf8 100644 --- a/.github/actions/resolve-config/callers.test.mjs +++ b/.github/actions/resolve-config/callers.test.mjs @@ -1941,6 +1941,22 @@ test("pr-run-activity-read.yml: the fallback call's lane record names why it ran ); }); +// limit-stop.mjs reads a stop only from the lane record on the check run's +// first summary line; without it, a refused credential or a usage limit on a +// read activity reads as a plain failure (the 2026-10-25 outage: three +// resolve-conflicts runs on one head in 23 minutes, then the cap). +test("pr-run-activity-read.yml: the posted check carries the lane record's class and reset", () => { + const { jobs } = workflow("pr-run-activity-read.yml"); + assert.equal(jobs.run.outputs["record-class"], "${{ steps.lane-record.outputs.failure-class }}"); + assert.equal(jobs.run.outputs["record-resets"], "${{ steps.lane-record.outputs.resets-at }}"); + const post = jobs.report.steps.find((s) => s.name === "Post the check run"); + assert.equal(post.with["lane-class"], "${{ needs.run.outputs.record-class }}"); + assert.equal(post.with["lane-resets"], "${{ needs.run.outputs.record-resets }}"); + // The same action, at the same pin, as the write workflow whose record limit-stop.mjs already reads. + const writeRecord = workflow("pr-run-activity-write.yml").jobs.run.steps.find((s) => s.id === "lane-record"); + assert.equal(jobs.run.steps.find((s) => s.id === "lane-record").uses, writeRecord.uses); +}); + // A caller whose handoff job read only that the first call posted no check // passes fallback handoff; the run job's input check must let it through. test("pr-run-activity-read.yml: the fallback input takes empty, credit-stop, no-key, handoff or fallback, and nothing else", () => { diff --git a/.github/scripts/pr-unblock/dispatch.mjs b/.github/scripts/pr-unblock/dispatch.mjs index 19571ad58e..24c3bd133c 100644 --- a/.github/scripts/pr-unblock/dispatch.mjs +++ b/.github/scripts/pr-unblock/dispatch.mjs @@ -10,7 +10,8 @@ // a held PR (do-not-merge, or the human-gated label unless another lane's // escalation put it on: gate-source.mjs prHold), or one whose current head // already carries a pr-unblock check run that handles it (limit-stop.mjs: a -// limit stop stops counting once its limit resets, a failed run until +// limit stop, a usage limit or a refused credential, stops counting once its +// limit resets, and the skip names its class and reset; a failed run until // limit-stops-per-head failures stand on the head, and a skip while held // once the hold is gone), and dispatches pr-unblock.yml on main for at most // maxDispatch of them, lowest PR number first. Authors are left to the lane's @@ -44,7 +45,7 @@ import { prHold } from "./gate-source.mjs"; import { runTitle, shareWorkflowRuns } from "./feedback-dispatch.mjs"; import { DEFAULTS as CI_FIX_DEFAULTS, dispatchCiFixes, humanGatedLabel, LIST_QUERY as CI_FIX_QUERY } from "./fix-ci-dispatch.mjs"; import { dispatchDocsFixes, HISTORY_QUERY as DOCS_FIX_HISTORY_QUERY, LIST_QUERY as DOCS_FIX_QUERY } from "./fix-docs-dispatch.mjs"; -import { anyHandles, cappedAt, permanentlyHandles } from "./limit-stop.mjs"; +import { anyHandles, cappedAt, limitStop, pendingStop, permanentlyHandles } from "./limit-stop.mjs"; import { budgetedFetch, serverFailure } from "./request-budget.mjs"; import { readConfig, readPolicy } from "./review-dependency.mjs"; @@ -189,6 +190,7 @@ export async function dispatchConflicting({ continue; } let handled; + let waiting = null; try { const { held, why } = await prHold({ labels: (node.labels?.nodes ?? []).map((label) => label?.name), @@ -207,11 +209,19 @@ export async function dispatchConflicting({ const existing = await api.listCheckRuns({ sha: node.headRefOid, checkName, appId }); const options = { limits, failuresRetry: true, heldSkipsRelease: true }; handled = anyHandles(existing, { now, ...options }); + // Handled only because a limit stop has not reset: the log names it. + waiting = handled && !anyHandles(existing, { now: Infinity, ...options }) ? pendingStop(existing, { now, limits }) : null; if (permanentlyHandles(existing, options)) { // Recording it means this warning shows once per head. const capped = cappedAt(existing, options); if (capped > 0) { - log(`::warning::#${node.number}: head ${node.headRefOid}'s ${checkName} failed or was skipped while held ${capped} time(s), the most one head gets; no ${activity} until a new push`); + // The newest finished run's stop class, so a cap a credential outage + // or a usage limit spent names that cause. + const newest = existing.filter((run) => run.status === "completed").sort((a, b) => Date.parse(b.completed_at ?? "") - Date.parse(a.completed_at ?? ""))[0]; + const stop = newest ? limitStop(newest, limits) : null; + log( + `::warning::#${node.number}: head ${node.headRefOid}'s ${checkName} failed or was skipped while held ${capped} time(s), the most one head gets; no ${activity} until a new push${stop ? `; the last run stopped on class=${stop.failureClass}` : ""}`, + ); } markHandled(record, checkName, node.headRefOid); } @@ -226,6 +236,12 @@ export async function dispatchConflicting({ ); continue; } + if (waiting) { + log( + `::notice::#${node.number}: head ${node.headRefOid}'s ${checkName} stopped on class=${waiting.failureClass}; no ${activity} before ${new Date(waiting.resetsAt).toISOString()}`, + ); + continue; + } if (handled) { log(`skip #${node.number}: head ${node.headRefOid} already has a ${checkName} check run`); continue; diff --git a/.github/scripts/pr-unblock/dispatch.test.mjs b/.github/scripts/pr-unblock/dispatch.test.mjs index 3f671a0427..5d98cdbb73 100644 --- a/.github/scripts/pr-unblock/dispatch.test.mjs +++ b/.github/scripts/pr-unblock/dispatch.test.mjs @@ -344,6 +344,54 @@ test("the resolved limits set how many reset limit stops hold a head", async () assert.deepEqual(dispatchedNumbers(one.api), []); }); +// The 2026-10-25 credential outage: every leg ended class=auth on a 401, and a +// failure with no lane record was retried at once, so #9826's head spent its +// three runs in 23 minutes and was recorded at the cap before the secrets were +// re-set. A refused credential now waits out fallback-reset like a usage limit, +// and the skip names the class and the reset. +const authRun = (completed) => ({ + ...failedRun(completed, null), + output: { title: "Failed", summary: "lane=pr-unblock activity=resolve-conflicts class=auth turns=1\n\nThe activity step failed." }, +}); + +test("a head whose run was refused its credential waits out fallback-reset, logged with its class, and is not recorded", async () => { + const now = Date.parse("2026-10-25T22:03:00Z"); + const refused = pr(41); + const reset = pr(42); + const unrecorded = pr(43); + const record = emptyRecord(); + const { api, lines, run } = harness( + { + pages: [[refused, reset, unrecorded]], + checked: new Map([ + [refused.headRefOid, [authRun("2026-10-25T21:44:25Z")]], + [reset.headRefOid, [authRun("2026-10-25T16:00:00Z")]], + [unrecorded.headRefOid, [failedRun("2026-10-25T21:44:25Z", null)]], + ]), + }, + { now, record, maxDispatch: 9 }, + ); + await run(); + // The unrecorded failure is retried at once, as before; the refused one waits. + assert.deepEqual(dispatchedNumbers(api), ["42", "43"]); + assert.ok( + lines.includes( + `::notice::#41: head ${refused.headRefOid}'s ${CHECK_NAME} stopped on class=auth; no resolve-conflicts before 2026-10-26T02:44:25.000Z`, + ), + lines.join("\n"), + ); + assert.deepEqual([...(record.handled.get(CHECK_NAME) ?? [])], []); +}); + +test("a head with a success beside an unreset stop is skipped as handled, not as waiting", async () => { + const now = Date.parse("2026-10-25T22:03:00Z"); + const done = pr(44); + const { api, lines, run } = harness({ pages: [[done]], checked: new Map([[done.headRefOid, [authRun("2026-10-25T21:44:25Z"), SUCCESS_RUN]]]) }, { now }); + await run(); + assert.deepEqual(dispatchedNumbers(api), []); + assert.ok(!lines.some((line) => line.includes("stopped on class=")), lines.join("\n")); +}); + // A failed resolve-conflicts run is retried on the same head until // limits.limit-stops-per-head (3 by default) failures stand on it. test("a failed run is retried until three failures stand on the head", async () => { @@ -1211,6 +1259,16 @@ test("a head at the resolve-conflicts cap is warned about once, as it is recorde assert.deepEqual([second.api.calls.checks, second.api.calls.timeline, second.api.calls.runs], [[], [], []]); }); +test("a head capped by refused-credential stops names the class in its cap warning", async () => { + const capped = pr(64); + const stops = ["2026-10-25T21:44:25Z", "2026-10-26T02:50:00Z", "2026-10-26T08:00:00Z"].map((at) => authRun(at)); + const { lines, run } = harness({ pages: [[capped]], checked: new Map([[capped.headRefOid, stops]]) }, { now: Date.parse("2026-10-26T14:00:00Z") }); + await run(); + assert.deepEqual(lines.filter((line) => line.startsWith("::warning::")), [ + `::warning::#64: head ${capped.headRefOid}'s ${CHECK_NAME} failed or was skipped while held 3 time(s), the most one head gets; no resolve-conflicts until a new push; the last run stopped on class=auth`, + ]); +}); + test("main records a fix-ci head at the per-head cap in DISPATCH_RECORD, warns once, and reads nothing for it on the next run", async () => { const dir = mkdtempSync(join(tmpdir(), "pr-unblock-record-")); const output = join(dir, "output"); diff --git a/.github/scripts/pr-unblock/limit-stop.mjs b/.github/scripts/pr-unblock/limit-stop.mjs index f0f8f301fc..4142e8d8bc 100644 --- a/.github/scripts/pr-unblock/limit-stop.mjs +++ b/.github/scripts/pr-unblock/limit-stop.mjs @@ -3,6 +3,9 @@ // it is the gate's completed neutral skip for an untrusted trigger, or a run // the usage limit stopped whose limit has reset: the lane's next trigger then // dispatches it again, at most `limits.limit-stops-per-head` times on one head. +// A run whose credential was refused (class `auth`, a 401) is a limit stop +// too: the next run on the same credential fails the same way, so the head +// waits `fallback-reset` hours instead of spending its stops at once. // // A limit stop is read only from a failed run's lane record, which // report-check-run writes from runner-computed outputs. The record is the first @@ -38,19 +41,39 @@ function summaryRecord(run) { return { failureClass: match[3] ?? "", resets: match[4] ?? "" }; } -// When the limit that stopped the failed `run` resets (epoch ms), or null when -// its lane record does not mark it a limit stop. The stated reset is held +// The lane record classes that stop a run until a reset. +const STOP_CLASSES = new Set(["rate-limit", "auth"]); + +// The failed `run`'s limit stop as { failureClass, resetsAt } (epoch ms), or +// null when its lane record does not mark it one. The stated reset is held // within the limits' clamp after the run ended. -export function limitResetsAt(run, limits = LIMIT_DEFAULTS) { +export function limitStop(run, limits = LIMIT_DEFAULTS) { const ended = Date.parse(run.completed_at ?? ""); if (!Number.isFinite(ended) || run.conclusion !== "failure") { return null; } const summary = summaryRecord(run); - if (summary?.failureClass !== "rate-limit") { + if (!STOP_CLASSES.has(summary?.failureClass)) { return null; } - return clampReset(summary.resets ? Date.parse(summary.resets) : null, ended, limits); + return { failureClass: summary.failureClass, resetsAt: clampReset(summary.resets ? Date.parse(summary.resets) : null, ended, limits) }; +} + +// When the limit that stopped the failed `run` resets (epoch ms), or null when +// it is no limit stop. +export const limitResetsAt = (run, limits = LIMIT_DEFAULTS) => limitStop(run, limits)?.resetsAt ?? null; + +// The limit stop among `runs` that has not reset by `now` and resets last, or +// null: what a dispatcher names when it skips a head that waits on one. +export function pendingStop(runs, { now, limits = LIMIT_DEFAULTS }) { + let pending = null; + for (const run of runs) { + const stop = limitStop(run, limits); + if (stop && now < stop.resetsAt && !(pending?.resetsAt >= stop.resetsAt)) { + pending = stop; + } + } + return pending; } const untrustedSkip = (run) => diff --git a/.github/scripts/pr-unblock/limit-stop.test.mjs b/.github/scripts/pr-unblock/limit-stop.test.mjs index 4c52c0a0f2..bb03071021 100644 --- a/.github/scripts/pr-unblock/limit-stop.test.mjs +++ b/.github/scripts/pr-unblock/limit-stop.test.mjs @@ -1,7 +1,7 @@ import assert from "node:assert/strict"; import { test } from "node:test"; -import { anyHandles, limitResetsAt, permanentlyHandles } from "./limit-stop.mjs"; +import { anyHandles, limitResetsAt, pendingStop, permanentlyHandles } from "./limit-stop.mjs"; const HOUR = 3_600_000; const ENDED = "2026-10-17T10:00:00Z"; @@ -34,6 +34,26 @@ test("a stated reset is held between one hour and seven days after the run ended assert.equal(limitResetsAt(posted(`${LIMIT} resets=2027-01-01T00:00:00Z`)), ended + 7 * 24 * HOUR); }); +// A 401 fails every run on that credential until a person re-sets it, so it +// waits like a usage limit that states no reset, and counts toward the cap. +test("a refused credential (class auth) is a limit stop that resets fallback-reset hours after the run ended", () => { + const AUTH = "lane=pr-unblock activity=resolve-conflicts class=auth turns=1"; + assert.equal(limitResetsAt(posted(AUTH)), ended + 5 * HOUR); + const stops = [1, 2, 3].map((id) => posted(AUTH, { id })); + assert.equal(anyHandles(stops.slice(0, 1), { now: ended + 5 * HOUR - 1, failuresRetry: true }), true); + assert.equal(anyHandles(stops.slice(0, 2), { now: ended + 5 * HOUR, failuresRetry: true }), false); + assert.equal(anyHandles(stops, { now: ended + 5 * HOUR, failuresRetry: true }), true); +}); + +test("pendingStop names the unreset stop that resets last, and nothing once all have reset", () => { + const auth = posted("lane=pr-unblock activity=resolve-conflicts class=auth", { id: 1 }); + const limit = posted(LIMIT_LINE, { id: 2 }); + assert.deepEqual(pendingStop([limit, auth], { now: ended }), { failureClass: "auth", resetsAt: ended + 5 * HOUR }); + assert.deepEqual(pendingStop([limit, auth], { now: ended + 4 * HOUR }), { failureClass: "auth", resetsAt: ended + 5 * HOUR }); + assert.equal(pendingStop([limit, auth], { now: ended + 5 * HOUR }), null); + assert.equal(pendingStop([run()], { now: ended }), null); +}); + test("another class, or a failed run with no summary record, is no limit stop", () => { assert.equal(limitResetsAt(posted("lane=pr-unblock activity=resolve-conflicts class=other turns=3")), null); assert.equal(limitResetsAt(run()), null); diff --git a/.github/workflows/pr-run-activity-read.yml b/.github/workflows/pr-run-activity-read.yml index 2fe15be0a0..0710911f94 100644 --- a/.github/workflows/pr-run-activity-read.yml +++ b/.github/workflows/pr-run-activity-read.yml @@ -518,6 +518,13 @@ jobs: lane-class: ${{ steps.act-key.outputs.lane-class || steps.act-token.outputs.lane-class || steps.act-fallback.outputs.lane-class }} credential-present: ${{ steps.key-present.outputs.present || steps.token-present.outputs.present || steps.fallback-present.outputs.present }} handed-off: ${{ steps.key-present.outputs.present == 'false' || steps.act-key.outputs.handoff == 'true' || steps.act-token.outputs.handoff == 'true' }} + # The lane record's class and reset, for the report job's check run, + # which limit-stop.mjs reads, as pr-run-activity-write.yml's lane-class. + # A forged limit stop can only restart the lane after a reset + # limit-stop.mjs clamps to at least `limits.reset-clamp-min` hours away, + # at most `limits.limit-stops-per-head` times on one head. + record-class: ${{ steps.lane-record.outputs.failure-class }} + record-resets: ${{ steps.lane-record.outputs.resets-at }} # Set after head code ran, for the scan job alone: each payload's byte # count and the sealed bundle, never a workflow_call output. carry-reply-bytes: ${{ steps.carry.outputs.reply-bytes }} @@ -3371,6 +3378,8 @@ jobs: not-run-reason: ${{ steps.head.outputs.not-run-reason }} act-outcome: ${{ needs.run.outputs.act-outcome }} gate-reason: ${{ steps.head.outputs.forged != 'true' && needs.run.outputs.gate-reason || '' }} + lane-class: ${{ needs.run.outputs.record-class }} + lane-resets: ${{ needs.run.outputs.record-resets }} head-sha: ${{ steps.head.outputs.sha }} pr-head-sha: ${{ steps.pr-head.outputs.sha }} can-commit: ${{ needs.run.outputs.can-commit }} diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index b2a944db45..ff9a9a0973 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -134,7 +134,9 @@ a trusted human's `workflow_dispatch`: - It skips a PR whose head already has a `pr-unblock / resolve-conflicts` check run from the GitHub Actions app, unless that run is a neutral `Skipped: untrusted-trigger`, or a `Skipped: awaiting-human` (a skip while held), a limit stop past its reset or a failure, until - `limits.limit-stops-per-head` of those last three stand on the head. + `limits.limit-stops-per-head` of those last three stand on the head. A limit stop is a run its + lane record marks `class=rate-limit` or `class=auth` (a refused credential); until it resets the + dispatcher logs a notice naming the class and the reset time. - It dispatches `pr-unblock.yml` on `refs/heads/main` for at most 3 PRs per run, lowest number first, passing `pr-number` and `dispatcher-run-id`. - A hand run or a re-run must be started by a listed human (`check-trusted-trigger` with @@ -2140,7 +2142,7 @@ with defaults. | Key | Default | What it sets | |---|---|---| -| `fallback-reset` | 5 | Hours after a limit stop that names no reset time before the limit counts as reset. | +| `fallback-reset` | 5 | Hours after a limit stop that names no reset time (every `auth` stop) before the limit counts as reset. | | `reset-clamp-min` | 1 | Fewest hours after a stop before a restart, whatever reset the stop names. | | `reset-clamp-max` | 168 | Most hours after a stop before a restart, whatever reset the stop names. | | `restarts-per-run` | 5 | Issues one work-unblock sweep restarts or re-fires. |