diff --git a/.changes/pr-pipeline/fix-feedback-sweep-every-released-head-5c1e9a07.md b/.changes/pr-pipeline/fix-feedback-sweep-every-released-head-5c1e9a07.md new file mode 100644 index 00000000000..7dc2fd17ef1 --- /dev/null +++ b/.changes/pr-pipeline/fix-feedback-sweep-every-released-head-5c1e9a07.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- The half-hourly address-feedback sweep now sends every eligible lanes App head on one tick, up to five (`SWEEP_TICK_CAP`), instead of only the first. Before, each released head after the first waited one more tick, about 30 minutes, per PR. The sweep runs its passes in the same order (fix request, released, stale stop, unsent, idle owner) and decides each PR at most once. It stops deciding after five minutes (`SWEEP_DECIDE_BUDGET_MS`), then sends the decided rounds back to back, so each dispatched-run record uploads seconds after its POST, well within the trusted-trigger gate's wait. `pr-feedback-dispatch.yml` records each send through its own numbered steps. Lost-run recovery still runs only on a tick that sent nothing. diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index b652989afad..c96b252431d 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -93,13 +93,14 @@ // main whose head's newest reply check took one of those skips, fewer than // SWEEP_HEAD_CAP times at that head, is decided as above with the // newest completed review run at that head as the upstream (reviewRunAt), -// which the record names. With no such head and no stale stop, it sends the -// first lanes App head with an unresolved thread that no round was sent for +// which the record names. It then decides each lanes App head with a stale +// stop (sweepStale), each with an unresolved thread that no round was sent for // (sweepUnsent): its review completion skipped on a cause that may clear, such -// as an unreadable FAIL cap. With none, it sends the first idle adopted owner -// PR whose reviewed head has no round yet, or whose round's stop no thread -// holds any more (sweepIdleOwner), since that PR's own review completion found -// it active. +// as an unreadable FAIL cap, and each idle adopted owner PR whose reviewed +// head has no round yet, or whose round's stop no thread holds any more +// (sweepIdleOwner), since that PR's own review completion found it active. +// The passes run in that order, after the fix requests below; a tick decides +// each PR at most once and at most SWEEP_TICK_CAP in all, then sends them. // // Before those, the tick takes a fix request nothing else would start: no // review run follows a reply on a head, and an earlier round's check run holds @@ -127,8 +128,9 @@ // keeps its thread workable on any head (address-feedback.mjs onReviewedHead), // so the review run after resolve-conflicts' push dispatches it. // -// A sweep dispatches at most once, since the -// dispatched-run record names one upstream and one run, and keeps no record. +// A sweep decides every round before it sends any, so each round's +// dispatched-run record, which names its upstream and its run, uploads seconds +// after its POST; it keeps no dispatch record. // The unsent sweep also sends a head with no unresolved thread and a fail note // from another head, while its newest review run completed within // EARLIER_FAIL_WINDOW_MS: its verifier read may have failed, or a pass's @@ -474,6 +476,9 @@ export async function dispatchFeedback({ // The verifier reads this pass may still spend on PRs with no unresolved // thread (EARLIER_FAIL_READS); main shares one across the pass's sweeps. earlierFailReads = { left: EARLIER_FAIL_READS }, + // The sweep decides every PR before it sends any: the decision then + // returns { number, send } and send() posts it. + defer = false, }) { // Each skip names the upstream run, so a relay or review run traces to its // dispatch decision in one log read. @@ -767,30 +772,33 @@ export async function dispatchFeedback({ ...(earlierFail.length > 0 ? { "verify-earlier-fail-heads": earlierFail.join(" ") } : {}), }, }; - let workflowRunId; - try { - ({ workflowRunId } = await api.dispatchWorkflow(request)); - } catch (error) { - if (!connectionLost(error)) { - throw error; + const send = async () => { + let workflowRunId; + try { + ({ workflowRunId } = await api.dispatchWorkflow(request)); + } catch (error) { + if (!connectionLost(error)) { + throw error; + } + // The connection broke before any answer, so GitHub may still have taken + // the POST: a run it started for this PR holds the second send, as the + // listing above would. This run never sends an HTTP error answer again; + // the run fails, and recoverLost may send it within + // FAILED_RUN_WINDOW_HOURS. + const since = new Date(now - 60_000).toISOString().replace(/\.\d{3}Z$/, "Z"); + const started = (await api.listRecentRuns(settings.workflow, since)).find((run) => holds(run, number)); + log(`::warning::#${number}: the ${settings.workflow} dispatch got no answer (${errorLabel(error)}); ${started ? `run ${started.id} for it is listed, so it is not sent again` : "sending it once more"}`); + ({ workflowRunId } = started ? { workflowRunId: started.id } : await api.dispatchWorkflow(request)); + } + const id = String(workflowRunId); + if (!RUN_ID.test(id)) { + throw new Error(`#${number}: dispatch returned no valid workflow_run_id`); } - // The connection broke before any answer, so GitHub may still have taken - // the POST: a run it started for this PR holds the second send, as the - // listing above would. This run never sends an HTTP error answer again; - // the run fails, and recoverLost may send it within - // FAILED_RUN_WINDOW_HOURS. - const since = new Date(now - 60_000).toISOString().replace(/\.\d{3}Z$/, "Z"); - const started = (await api.listRecentRuns(settings.workflow, since)).find((run) => holds(run, number)); - log(`::warning::#${number}: the ${settings.workflow} dispatch got no answer (${errorLabel(error)}); ${started ? `run ${started.id} for it is listed, so it is not sent again` : "sending it once more"}`); - ({ workflowRunId } = started ? { workflowRunId: started.id } : await api.dispatchWorkflow(request)); - } - const id = String(workflowRunId); - if (!RUN_ID.test(id)) { - throw new Error(`#${number}: dispatch returned no valid workflow_run_id`); - } - record.dispatched.set(head, { number, runId: id }); - log(`dispatched #${number} head ${head} upstream run ${upstreamRunId} workflow_run_id=${id}`); - return { number, workflowRunId: id }; + record.dispatched.set(head, { number, runId: id }); + log(`dispatched #${number} head ${head} upstream run ${upstreamRunId} workflow_run_id=${id}`); + return { number, workflowRunId: id }; + }; + return defer ? { number, send } : send(); } // The verifier reads one dispatcher pass spends on lanes App PRs with no @@ -818,41 +826,86 @@ export const earlierFailNoted = (threads, head) => const LOST_CONNECTION = new Set(["ECONNRESET", "UND_ERR_SOCKET"]); const connectionLost = (error) => error?.name === "TypeError" && error.message === "fetch failed" && LOST_CONNECTION.has(error.cause?.code); -// The sweep: the first open lanes App PR into main with a fix request newer -// than its head's last round (sweepFixRequest), else the first whose head's -// newest reply check took a releasing skip, under SWEEP_HEAD_CAP such checks, -// that dispatchFeedback dispatches for the head's newest completed review run. -// Returns dispatchFeedback's result with that run's id, or null. -export async function sweepReleased({ api, repository, appSenderId, log, settings = DEFAULTS, ...rest }) { - const context = { api, repository, appSenderId, log, settings, rest }; +// Rounds one tick sends (judgment: five bounds the extra dispatches and their +// reads per tick well under the GITHUB_TOKEN's 1,000 requests an hour). +export const SWEEP_TICK_CAP = 5; +// Time after which the sweep decides no further PR. The gate trusts a +// dispatched run only if it was created within 10 minutes of this run's start +// (check-trusted-trigger.mjs WINDOW_MS), and the dispatch job's timeout is 10 +// minutes too: setup (18 s on run 41759929522), this budget, the decision in +// flight (one verifier read, VERIFY_CAP_TIMEOUT_MS) and the sends leave about +// 2.5 minutes of margin (judgment). +export const SWEEP_DECIDE_BUDGET_MS = 5 * 60_000; + +// The sweep, in pass order: each open lanes App PR into main with a fix +// request newer than its head's last round (sweepFixRequest); each whose +// head's newest reply check took a releasing skip, under SWEEP_HEAD_CAP such +// checks (sweepOne); then sweepStale, sweepUnsent and sweepIdleOwner. Each PR +// is decided at most once a tick, until SWEEP_TICK_CAP PRs are decided or +// SWEEP_DECIDE_BUDGET_MS is spent. Only then are the decided rounds sent, back +// to back, so each record uploads seconds after its POST, well inside the +// gate's wait for it (check-trusted-trigger.mjs requireRecorded). Each send's +// result, with the head's newest completed review run id, goes to onSent; +// returns the first, or null. +export async function sweepReleased({ api, repository, appSenderId, log, settings = DEFAULTS, onSent = () => {}, clock = Date.now, ...rest }) { + const context = { api, repository, appSenderId, log, settings, rest: { ...rest, defer: true } }; const listed = await openPulls(context); const pulls = listed.filter((pull) => pull.user?.id === appSenderId); - const requested = await sweepPulls(context, (pull) => sweepFixRequest({ ...context, pull }), pulls); - if (requested !== null) { - return requested; - } - log("sweep: no head with a fix request newer than its last round"); - const dispatched = await sweepPulls(context, (pull) => sweepOne({ ...context, pull }), pulls); - if (dispatched !== null) { - return dispatched; - } - log("sweep: no head whose last round left it for another round to dispatch"); - const stale = await sweepPulls(context, (pull) => sweepStale({ ...context, pull }), pulls); - if (stale !== null) { - return stale; - } - log("sweep: no head with a failing address-feedback stop and nothing left to fix"); - const unsent = await sweepPulls(context, (pull) => sweepUnsent({ ...context, pull }), pulls); - if (unsent !== null) { - return unsent; - } - log("sweep: no head with an unresolved thread that no round was sent for"); const owners = listed.filter((pull) => pull.user?.id !== appSenderId); - const idle = owners.length === 0 ? null : await sweepPulls(context, (pull) => sweepIdleOwner({ ...context, pull }), owners); - if (idle === null) { - log("sweep: no idle owner PR whose reviewed head the lane has not answered"); + const decided = []; + const taken = new Set(); + const started = clock(); + const passes = [ + [sweepFixRequest, pulls, "sweep: no head with a fix request newer than its last round"], + [sweepOne, pulls, "sweep: no head whose last round left it for another round to dispatch"], + [sweepStale, pulls, "sweep: no head with a failing address-feedback stop and nothing left to fix"], + [sweepUnsent, pulls, "sweep: no head with an unresolved thread that no round was sent for"], + [sweepIdleOwner, owners, "sweep: no idle owner PR whose reviewed head the lane has not answered"], + ]; + decide: for (const [pass, rows, none] of passes) { + const before = decided.length; + for (const pull of rows) { + if (decided.length >= SWEEP_TICK_CAP) { + log(`sweep: ${SWEEP_TICK_CAP} rounds decided, the tick's cap; the next tick decides the rest`); + break decide; + } + if (clock() - started >= SWEEP_DECIDE_BUDGET_MS) { + log(`sweep: ${SWEEP_DECIDE_BUDGET_MS / 60_000} minutes spent deciding; the next tick decides the rest`); + break decide; + } + if (taken.has(pull.number)) { + continue; + } + try { + const round = await pass({ ...context, pull }); + if (round !== null) { + decided.push(round); + taken.add(pull.number); + } + } catch (error) { + // One PR's failed read leaves the PRs after it to the sweep. + log(`::warning::#${pull.number}: the sweep could not decide head ${pull.head.sha}: ${error.message}`); + } + } + if (decided.length === before) { + log(none); + } } - return idle; + const sent = []; + for (const { number, send, upstreamRunId } of decided) { + let result; + try { + result = { ...(await send()), upstreamRunId }; + } catch (error) { + // A POST the API rejects starts no round to hold the PR: log it and + // send the rounds after it, as the per-PR sweep did. + log(`::warning::#${number}: the sweep could not send its round: ${error.message}`); + continue; + } + sent.push(result); + onSent(result); + } + return sent[0] ?? null; } // Hours after an owner PR turns idle during which the tick still sends a head @@ -904,8 +957,8 @@ async function openPulls({ api, repository, appSenderId, settings, rest }) { ); } -// Each of `pulls` (default: the open lanes App PRs), decided by `decide` in -// listing order until one dispatches. +// The recovery's walk: each of `pulls` (default: the open lanes App PRs), +// decided by `decide` in listing order until one dispatches. async function sweepPulls(context, decide, pulls = null) { const { log } = context; for (const pull of pulls ?? (await openPulls(context)).filter((row) => row.user?.id === context.appSenderId)) { @@ -979,17 +1032,20 @@ async function sweepStale({ api, pull, repository, appSenderId, log, settings, r log(`skip: #${number}: head ${head} has no completed review run`); return null; } - const { workflowRunId } = await api.dispatchWorkflow({ - workflow: settings.workflow, - ref: `refs/heads/${settings.base}`, - inputs: { "pr-number": String(number), activity: settings.activity, "dispatcher-run-id": String(rest.runId) }, - }); - const id = String(workflowRunId); - if (!RUN_ID.test(id)) { - throw new Error(`#${number}: dispatch returned no valid workflow_run_id`); - } - log(`dispatched #${number} head ${head} to clear a stop with no thread left, upstream run ${upstream.id} workflow_run_id=${id}`); - return { number, workflowRunId: id, upstreamRunId: upstream.id }; + const send = async () => { + const { workflowRunId } = await api.dispatchWorkflow({ + workflow: settings.workflow, + ref: `refs/heads/${settings.base}`, + inputs: { "pr-number": String(number), activity: settings.activity, "dispatcher-run-id": String(rest.runId) }, + }); + const id = String(workflowRunId); + if (!RUN_ID.test(id)) { + throw new Error(`#${number}: dispatch returned no valid workflow_run_id`); + } + log(`dispatched #${number} head ${head} to clear a stop with no thread left, upstream run ${upstream.id} workflow_run_id=${id}`); + return { number, workflowRunId: id, upstreamRunId: upstream.id }; + }; + return rest.defer ? { number, send, upstreamRunId: upstream.id } : send(); } // pr-feedback-dispatch.yml's dispatch job timeout-minutes. @@ -1548,8 +1604,9 @@ export async function resolveFeedbackSlot({ number, head, env, github, settings }); } -// Writes run-id, the dispatched run, as a step output; the workflow records -// it with the upstream run id as an artifact on this run. +// Writes run-id, the dispatched run, as a step output (a tick's later sends as +// run-id-2 to run-id-); the workflow records each with its +// upstream run id as an artifact on this run. export async function main(env, { api, github, resolveSlot, verifyCapped, log = (line) => console.log(line), now = Date.now() } = {}) { for (const key of ["GITHUB_TOKEN", "GITHUB_REPOSITORY", "GITHUB_RUN_ID", "GITHUB_OUTPUT", "GITHUB_EVENT_PATH", "APP_SENDER_ID"]) { if (!env[key]) { @@ -1599,11 +1656,20 @@ export async function main(env, { api, github, resolveSlot, verifyCapped, log = log(`skip: run ${upstream.id} is not a scheduled tick`); 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 }); + // Each send's outputs are written as it is sent, so a later send's + // failure still leaves the earlier ones recorded: the first as run-id, + // the nth as run-id-. + let sends = 0; + const write = (swept) => { + sends += 1; + const suffix = sends === 1 ? "" : `-${sends}`; + appendFileSync(env.GITHUB_OUTPUT, `run-id${suffix}=${swept.workflowRunId}\nupstream-run-id${suffix}=${swept.upstreamRunId}\n`); + }; + // A lost run is recovered only on a tick the sweep sent nothing on. + const sweep = async () => (await sweepReleased({ ...common, onSent: write })) ?? 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`); + if (swept !== null && sends === 0) { + write(swept); } return 0; } diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index da1a68e5276..1a4b4636911 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -5,7 +5,7 @@ import { test } from "node:test"; import { LIMIT_DEFAULTS } from "../work-resolve-issue/limits.mjs"; import { emptyRecord, parseRecord, serializeRecord } from "./dispatch-record.mjs"; -import { createGitHubApi, DEFERRAL_MARKER, dispatchFeedback, EARLIER_FAIL_READS, EARLIER_FAIL_WINDOW_MS, failureAnnotation, FAILED_RUN_WINDOW_HOURS, IDLE_OWNER_WINDOW_HOURS, main, NO_ADOPTION, ownerAdoption, reviewRunAt, SWEEP_HEAD_CAP, sweepReleased, UNSENT_GRACE_MS } from "./feedback-dispatch.mjs"; +import { createGitHubApi, DEFERRAL_MARKER, dispatchFeedback, EARLIER_FAIL_READS, EARLIER_FAIL_WINDOW_MS, failureAnnotation, FAILED_RUN_WINDOW_HOURS, IDLE_OWNER_WINDOW_HOURS, main, NO_ADOPTION, ownerAdoption, reviewRunAt, SWEEP_DECIDE_BUDGET_MS, SWEEP_HEAD_CAP, SWEEP_TICK_CAP, sweepReleased, UNSENT_GRACE_MS } from "./feedback-dispatch.mjs"; import { onReviewedHead, threadAuthors } from "./address-feedback.mjs"; import { verifyReader } from "./verify-cap.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; @@ -1966,18 +1966,85 @@ test("a review completion at a head whose last round left a finding standing dis assert.equal(record.handled.size, 0); }); -test("a tick dispatches at most one head, and moves past one dispatchFeedback skips", async (t) => { - const second = pull({ number: 43, head: { sha: SECOND_HEAD, repo: { full_name: REPOSITORY } } })[0]; - const third = pull({ number: 44, head: { sha: "5".repeat(40), repo: { full_name: REPOSITORY } } })[0]; +// Released lanes App PRs 42, 43, ... at heads 1..., 4..., then 6..., 7..., each +// with its own review run (13, 14, ...), and a dispatch API that answers +// each POST with the next run id, logging decisions and sends in order. +const releasedSweepApi = (count, overrides = {}) => { + const heads = [HEAD, SECOND_HEAD, ...Array.from({ length: count }, (_, i) => String(i + 6).repeat(40).slice(0, 40))].slice(0, count); + const openPulls = heads.map((sha, i) => pull({ number: 42 + i, head: { sha, repo: { full_name: REPOSITORY } }, ...(overrides[42 + i] ?? {}) })[0]); const api = sweepApi({ - // PR 42 is labeled do-not-merge, so dispatchFeedback skips it. - openPulls: [...pull({ labels: [{ name: "do-not-merge" }] }), second, third], - checksAt: { [HEAD]: [failedRound, noModelTurn()], [SECOND_HEAD]: [failedRound, noModelTurn()], ["5".repeat(40)]: [failedRound, noModelTurn()] }, - reviewRuns: { [HEAD]: [upstreamRun({ id: 13 })], [SECOND_HEAD]: [upstreamRun({ id: 14, head_sha: SECOND_HEAD })], ["5".repeat(40)]: [upstreamRun({ id: 15, head_sha: "5".repeat(40) })] }, + openPulls, + checksAt: Object.fromEntries(heads.map((sha) => [sha, [failedRound, noModelTurn()]])), + reviewRuns: Object.fromEntries(heads.map((sha, i) => [sha, [upstreamRun({ id: 13 + i, head_sha: sha })]])), }); + api.events = []; + const listCompletedRuns = api.listCompletedRuns; + api.listCompletedRuns = async (path, head) => { + api.events.push(`decide ${head.slice(0, 1)}`); + return listCompletedRuns(path, head); + }; + api.dispatchWorkflow = async (request) => { + api.calls.dispatch.push(request); + api.events.push(`send #${request.inputs["pr-number"]}`); + return { workflowRunId: 38779200001 + api.calls.dispatch.length - 1 }; + }; + return api; +}; + +test("a tick sends every released head after deciding them all, and moves past one dispatchFeedback skips", async (t) => { + // PR 42 is labeled do-not-merge, so dispatchFeedback skips it. + const api = releasedSweepApi(3, { 42: { labels: [{ name: "do-not-merge" }] } }); const { output } = await sweepMain(t, api); - assert.deepEqual(api.calls.dispatch.map((request) => request.inputs["pr-number"]), ["43"]); - assert.equal(output, "run-id=38779200001\nupstream-run-id=14\n"); + assert.deepEqual(api.calls.dispatch.map((request) => request.inputs["pr-number"]), ["43", "44"]); + assert.equal(output, "run-id=38779200001\nupstream-run-id=14\nrun-id-2=38779200002\nupstream-run-id-2=15\n"); + // Every decision's reads come before the first POST. + const firstSend = api.events.findIndex((event) => event.startsWith("send")); + assert.ok(firstSend > 0 && api.events.slice(firstSend).every((event) => event.startsWith("send")), api.events.join(", ")); +}); + +test("a POST the API rejects is logged and the tick still sends the rounds after it", async (t) => { + const api = releasedSweepApi(3); + const dispatchWorkflow = api.dispatchWorkflow; + api.dispatchWorkflow = async (request) => { + if (request.inputs["pr-number"] === "42") { + api.calls.dispatch.push(request); + throw new Error("HTTP 502"); + } + return dispatchWorkflow(request); + }; + const { lines } = await sweepMain(t, api); + assert.deepEqual(api.calls.dispatch.map((request) => request.inputs["pr-number"]), ["42", "43", "44"]); + assert.ok(lines.some((line) => line.includes("#42: the sweep could not send its round: HTTP 502")), lines.join("\n")); +}); + +test("a tick sends at most SWEEP_TICK_CAP rounds and leaves the rest to the next tick", async (t) => { + const api = releasedSweepApi(SWEEP_TICK_CAP + 2); + const { output, lines } = await sweepMain(t, api); + assert.deepEqual(api.calls.dispatch.map((request) => request.inputs["pr-number"]), ["42", "43", "44", "45", "46"]); + assert.match(output, /^run-id-5=38779200005$/m); + assert.doesNotMatch(output, /run-id-6/); + assert.ok(lines.some((line) => line.includes("the tick's cap")), lines.join("\n")); +}); + +test("the sweep decides no further PR once SWEEP_DECIDE_BUDGET_MS is spent, and still sends what it decided", async () => { + const api = releasedSweepApi(3); + const lines = []; + const sent = []; + const first = await sweepReleased({ + api, + repository: REPOSITORY, + runId: RUN_ID, + appSenderId: APP_ID, + trustedHumans: new Set([KYLE.databaseId]), + resolveSlot: async () => resolvedLane(), + log: (line) => lines.push(line), + onSent: (result) => sent.push(result), + // The budget is spent once the first round is decided. + clock: () => (api.events.length > 0 ? SWEEP_DECIDE_BUDGET_MS : 0), + }); + assert.deepEqual(first, { number: 42, workflowRunId: "38779200001", upstreamRunId: 13 }); + assert.deepEqual(sent.map((result) => result.number), [42]); + assert.ok(lines.some((line) => line.includes("minutes spent deciding")), lines.join("\n")); }); test("a tick whose run listing is rate limited reads it once, dispatches nothing and ends green", async (t) => { @@ -3289,16 +3356,17 @@ test("a scheduled tick dispatches a fix request on an escalated thread and on a } }); -test("a fix request's dispatch goes before a released head, and takes the tick", async (t) => { +test("a fix request's dispatch goes before a released head", async (t) => { const second = pull({ number: 43, head: { sha: SECOND_HEAD, repo: { full_name: REPOSITORY } } })[0]; const api = requestApi({ openPulls: [second, ...pull()], reviewRuns: { [HEAD]: [upstreamRun({ id: 13 })], [SECOND_HEAD]: [upstreamRun({ id: 14, head_sha: SECOND_HEAD })] }, }); api.listCheckRuns = async ({ sha, checkName }) => (sha === HEAD ? [answeredRound, answeredReply] : [failedRound, noModelTurn()]).filter((run) => run.name === checkName); - api.listReviewThreads = async (number) => (number === 42 ? [askedThread({ author: KYLE_LOGIN, body: REQUEST })] : []); + api.listReviewThreads = async (number) => (number === 42 ? [askedThread({ author: KYLE_LOGIN, body: REQUEST })] : [thread([CLAUDE], false, SECOND_HEAD)]); await requestMain(t, api); - assert.deepEqual(api.calls.dispatch.map((request) => request.inputs["pr-number"]), ["42"]); + // PR 43 is listed first, but the fix-request pass decides PR 42 before it. + assert.deepEqual(api.calls.dispatch.map((request) => request.inputs["pr-number"]), ["42", "43"]); }); test("the sweep sends no fix request that does not qualify", async (t) => { @@ -3458,3 +3526,19 @@ test("reviewRunAt lists each review workflow's runs one after the other and pick assert.equal(upstream.id, 2); assert.deepEqual(events, ["a.yml start", "a.yml end", "b.yml start", "b.yml end"]); }); + +test("a PR two passes would each send is sent once a tick", async (t) => { + // A fix request after a released round: the fix-request pass and the + // released pass both take PR 42. + const api = requestApi({ checks: [failedRound, noModelTurn()] }); + await requestMain(t, api); + assert.deepEqual(api.calls.dispatch.map((request) => request.inputs["pr-number"]), ["42"]); +}); + +test("a tick that sent a round recovers no lost run", async (t) => { + const api = recoveryApi({ checksAt: { [HEAD]: [failedRound, noModelTurn()] } }); + const { output } = await sweepMain(t, api, TICK, NOW); + assert.equal(api.calls.dispatch.length, 1); + assert.deepEqual(api.listings, [], "no lost-run listing"); + assert.equal(output, "run-id=38779200001\nupstream-run-id=13\n"); +}); diff --git a/.github/workflows/pr-feedback-dispatch.yml b/.github/workflows/pr-feedback-dispatch.yml index c28ff04ea43..c7e857e290f 100644 --- a/.github/workflows/pr-feedback-dispatch.yml +++ b/.github/workflows/pr-feedback-dispatch.yml @@ -461,7 +461,9 @@ 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 sweep tick + # sends up to five rounds (feedback-dispatch.mjs SWEEP_TICK_CAP), each + # recorded by its own numbered steps below. - name: Name the dispatched-run record id: record if: always() && steps.dispatch.outputs.run-id != '' @@ -495,6 +497,138 @@ jobs: retention-days: 1 if-no-files-found: error + - name: Name the dispatched-run record 2 + id: record-2 + if: always() && steps.dispatch.outputs.run-id-2 != '' + env: + RUN_ID: ${{ steps.dispatch.outputs.run-id-2 }} + UPSTREAM_RUN_ID: ${{ steps.dispatch.outputs.upstream-run-id-2 }} + run: | + if [[ ! "$RUN_ID" =~ ^[1-9][0-9]{0,14}$ || ! "$UPSTREAM_RUN_ID" =~ ^[1-9][0-9]{0,14}$ ]]; then + echo "::error::run-id-2 or the upstream run id 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-2.json" + echo "name=pr-feedback-dispatched-$UPSTREAM_RUN_ID-$RUN_ID" >>"$GITHUB_OUTPUT" + + - name: Scan upload 2 for credentials + id: scan-upload-2 + if: >- + always() && steps.dispatch.outputs.run-id-2 != '' && steps.record-2.outcome == 'success' + uses: ./.base/.github/actions/scan-upload # zizmor: ignore[self-repository] + with: + path: ${{ runner.temp }}/pr-feedback-dispatched-2.json + + - name: Record dispatched run 2 + if: >- + (always() && steps.dispatch.outputs.run-id-2 != '' && steps.record-2.outcome == 'success') && steps.scan-upload-2.outputs.clean == 'true' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: ${{ steps.record-2.outputs.name }} + path: ${{ runner.temp }}/pr-feedback-dispatched-2.json + retention-days: 1 + if-no-files-found: error + + - name: Name the dispatched-run record 3 + id: record-3 + if: always() && steps.dispatch.outputs.run-id-3 != '' + env: + RUN_ID: ${{ steps.dispatch.outputs.run-id-3 }} + UPSTREAM_RUN_ID: ${{ steps.dispatch.outputs.upstream-run-id-3 }} + run: | + if [[ ! "$RUN_ID" =~ ^[1-9][0-9]{0,14}$ || ! "$UPSTREAM_RUN_ID" =~ ^[1-9][0-9]{0,14}$ ]]; then + echo "::error::run-id-3 or the upstream run id 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-3.json" + echo "name=pr-feedback-dispatched-$UPSTREAM_RUN_ID-$RUN_ID" >>"$GITHUB_OUTPUT" + + - name: Scan upload 3 for credentials + id: scan-upload-3 + if: >- + always() && steps.dispatch.outputs.run-id-3 != '' && steps.record-3.outcome == 'success' + uses: ./.base/.github/actions/scan-upload # zizmor: ignore[self-repository] + with: + path: ${{ runner.temp }}/pr-feedback-dispatched-3.json + + - name: Record dispatched run 3 + if: >- + (always() && steps.dispatch.outputs.run-id-3 != '' && steps.record-3.outcome == 'success') && steps.scan-upload-3.outputs.clean == 'true' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: ${{ steps.record-3.outputs.name }} + path: ${{ runner.temp }}/pr-feedback-dispatched-3.json + retention-days: 1 + if-no-files-found: error + + - name: Name the dispatched-run record 4 + id: record-4 + if: always() && steps.dispatch.outputs.run-id-4 != '' + env: + RUN_ID: ${{ steps.dispatch.outputs.run-id-4 }} + UPSTREAM_RUN_ID: ${{ steps.dispatch.outputs.upstream-run-id-4 }} + run: | + if [[ ! "$RUN_ID" =~ ^[1-9][0-9]{0,14}$ || ! "$UPSTREAM_RUN_ID" =~ ^[1-9][0-9]{0,14}$ ]]; then + echo "::error::run-id-4 or the upstream run id 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-4.json" + echo "name=pr-feedback-dispatched-$UPSTREAM_RUN_ID-$RUN_ID" >>"$GITHUB_OUTPUT" + + - name: Scan upload 4 for credentials + id: scan-upload-4 + if: >- + always() && steps.dispatch.outputs.run-id-4 != '' && steps.record-4.outcome == 'success' + uses: ./.base/.github/actions/scan-upload # zizmor: ignore[self-repository] + with: + path: ${{ runner.temp }}/pr-feedback-dispatched-4.json + + - name: Record dispatched run 4 + if: >- + (always() && steps.dispatch.outputs.run-id-4 != '' && steps.record-4.outcome == 'success') && steps.scan-upload-4.outputs.clean == 'true' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: ${{ steps.record-4.outputs.name }} + path: ${{ runner.temp }}/pr-feedback-dispatched-4.json + retention-days: 1 + if-no-files-found: error + + - name: Name the dispatched-run record 5 + id: record-5 + if: always() && steps.dispatch.outputs.run-id-5 != '' + env: + RUN_ID: ${{ steps.dispatch.outputs.run-id-5 }} + UPSTREAM_RUN_ID: ${{ steps.dispatch.outputs.upstream-run-id-5 }} + run: | + if [[ ! "$RUN_ID" =~ ^[1-9][0-9]{0,14}$ || ! "$UPSTREAM_RUN_ID" =~ ^[1-9][0-9]{0,14}$ ]]; then + echo "::error::run-id-5 or the upstream run id 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-5.json" + echo "name=pr-feedback-dispatched-$UPSTREAM_RUN_ID-$RUN_ID" >>"$GITHUB_OUTPUT" + + - name: Scan upload 5 for credentials + id: scan-upload-5 + if: >- + always() && steps.dispatch.outputs.run-id-5 != '' && steps.record-5.outcome == 'success' + uses: ./.base/.github/actions/scan-upload # zizmor: ignore[self-repository] + with: + path: ${{ runner.temp }}/pr-feedback-dispatched-5.json + + - name: Record dispatched run 5 + if: >- + (always() && steps.dispatch.outputs.run-id-5 != '' && steps.record-5.outcome == 'success') && steps.scan-upload-5.outputs.clean == 'true' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: ${{ steps.record-5.outputs.name }} + path: ${{ runner.temp }}/pr-feedback-dispatched-5.json + retention-days: 1 + if-no-files-found: error + - name: Log the API budget at the end if: always() continue-on-error: true diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index dab91b24fa6..d2d3cf0229c 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -245,9 +245,10 @@ person adds `do-not-merge`, since re-adding a label already on records no event) (the round handled nothing) or `finding-stands` (it left a finding standing at that head), and the round cap still bounds the retries; a limit stop keeps its reset hold and counts toward `limit-stops-per-head`; since such a head may see no later review, -each scheduled `pr-unblock-dispatch.yml` run's completion also starts a sweep that decides one -such head per tick as for its newest completed review run, under three such skips at that -head), and no `pr-unblock.yml` run titled +each scheduled `pr-unblock-dispatch.yml` run's completion also starts a sweep that decides each +such head as for its newest completed review run, under three such skips at that head, and +sends up to five rounds a tick (`SWEEP_TICK_CAP`) once every decision is made, each PR at most +once), and no `pr-unblock.yml` run titled `pr-unblock address-feedback #` is still queued or going (both lanes finishing at once). Then it runs the base's resolve-config for that PR and head with `address-feedback` selected, so only that slot's `applies-when` is decided, and dispatches only when the `pr-unblock` lane and its @@ -345,7 +346,7 @@ serves an adopted owner PR, posting its replies as the lanes App: - When: as fix-ci adopts one, no body line starts `Hold:`, and both the PR's last update and its head commit's committer date are at least `adoption.idle-hours` old (`ownerSkip`), so the lane never pushes onto a branch a session is still working on. A review usually finishes while the session - is active, so that dispatch skips; each scheduled tick's sweep then sends one idle owner PR whose + is active, so that dispatch skips; each scheduled tick's sweep then sends each idle owner PR, within its cap, whose reviewed head has no `pr-unblock / address-feedback` check run (`sweepIdleOwner`), deciding it as for its newest completed review run. It decides such a head only within `IDLE_OWNER_WINDOW_HOURS` of the PR turning idle, so a head with nothing for the lane is not @@ -402,7 +403,7 @@ thread, and one a trusted person answered after the lane escalated it, is worked even when its review is on an earlier head than the one the reviewed-head markers name. On a lanes App PR, the next half-hourly tick picks up a fix request on an unresolved thread the reviewer bot opened: each scheduled `pr-unblock-dispatch.yml` run's completion starts -`pr-feedback-dispatch.yml`, whose sweep sends one such head per tick, ahead of the heads it retries. +`pr-feedback-dispatch.yml`, whose sweep decides each such head ahead of the heads it retries, within its cap of five a tick. The request must be newer than the head's newest `pr-unblock / address-feedback` check run; check runs that started before it stop counting for that dispatch, except a limit stop, which keeps its reset hold. Each request is sent once, a head with three rounds or releasing skips is not sent, and