diff --git a/.changes/pr-pipeline/fix-unblock-unsent-head-sweep-5b7e2c91.md b/.changes/pr-pipeline/fix-unblock-unsent-head-sweep-5b7e2c91.md new file mode 100644 index 00000000000..bbaf9480b10 --- /dev/null +++ b/.changes/pr-pipeline/fix-unblock-unsent-head-sweep-5b7e2c91.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- The half-hourly address-feedback sweep (`pr-feedback-dispatch.yml`, `sweepUnsent`) now sends a lanes App pull request whose head has an unresolved review thread and no address-feedback round, 10 minutes after the last review or pr-merge run at that head finished, and no round for it was created since that review started. Before, a review completion that skipped on a cause that clears later, such as a pr-verify FAIL-cap read that did not fit in its time limit, left the head with no other trigger, and one advisory thread held the pull request until a person dispatched the round. diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index d2424dc9b54..57aa2f5611f 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -85,10 +85,13 @@ // 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 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. +// 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 +// (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. // // 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 @@ -767,6 +770,11 @@ export async function sweepReleased({ api, repository, appSenderId, log, setting 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) { @@ -912,6 +920,57 @@ async function sweepStale({ api, pull, repository, appSenderId, log, settings, r return { number, workflowRunId: id, upstreamRunId: upstream.id }; } +// pr-feedback-dispatch.yml's dispatch job timeout-minutes. +export const UNSENT_GRACE_MS = 10 * 60_000; + +// A head with an unresolved thread and no address-feedback check run, and no +// run of the activity for the PR created since its newest review run started: +// its review completion's dispatch skipped for a reason that may clear, such +// as verify-cap.mjs's unreadable FAIL cap, and no review run follows to send +// it again. A skip that stands costs the reads dispatchFeedback makes before +// it, each tick. +async function sweepUnsent(context) { + const { api, pull, repository, appSenderId, log, settings, rest } = context; + const head = pull.head.sha; + if ((await api.listCheckRuns({ sha: head, checkName: settings.checkName, appId: settings.appId })).length > 0) { + return null; + } + const unresolved = (await api.listReviewThreads(pull.number)).filter((thread) => thread.isResolved === false); + if (unresolved.length === 0) { + return null; + } + const upstream = await reviewRunAt({ api, head, repository, settings }); + if (upstream === null) { + return null; + } + // Each review, pr-merge and comment-relay run's completion at the head + // starts a dispatch in the head's concurrency group, not the sweep's, so the + // head waits out that job's timeout after the last of them. An unreadable + // time does not count. A pr-verify FAIL's dispatch runs in a verify- + // group at main's head, so the notes it posts as unresolved threads just + // before it stand in for it: the newest comment on one counts too. + const triggers = []; + for (const [path, event] of [...settings.reviewWorkflows.map((path) => [path, "pull_request"]), [MERGE_WORKFLOW, "pull_request_target"], [RELAY_WORKFLOW, RELAY_EVENT]]) { + triggers.push(...(await api.listCompletedRuns(path, head, event)).filter((run) => run.head_sha === head && run.event === event)); + } + const commented = unresolved.flatMap((thread) => thread.comments?.nodes ?? []).map((comment) => Date.parse(comment.createdAt ?? "")).filter(Number.isFinite); + const last = Math.max(...[upstream, ...triggers].map((run) => Date.parse(run.updated_at ?? "")), ...commented); + if (!(last <= (rest.now ?? Date.now()) - UNSENT_GRACE_MS)) { + return null; + } + // A round that ended before posting its check is sent once. + const reviewStarted = Date.parse(upstream.run_started_at ?? upstream.created_at ?? ""); + const sent = (await api.listWorkflowRuns(settings.workflow)).find( + (run) => run.display_title === runTitle(settings.activity, pull.number) && !(Date.parse(run.created_at ?? "") < reviewStarted), + ); + if (sent) { + log(`skip: #${pull.number}: ${settings.workflow} run ${sent.id} for ${settings.activity} started after head ${head}'s review`); + return null; + } + const dispatched = await dispatchFeedback({ ...rest, api, repository, appSenderId, log, settings, upstream }); + return dispatched === null ? null : { ...dispatched, upstreamRunId: upstream.id }; +} + // Whether the head's newest reply check released it, and whether releasing // skips reached SWEEP_HEAD_CAP there (logged). async function releaseState({ api, pull, log, settings }) { @@ -1304,8 +1363,8 @@ export function createGitHubApi({ token, repository, apiUrl, graphqlUrl, fetch: throw new Error(`commit ${sha} has more than 1000 statuses`); }, // The newest completed pull_request runs of a workflow file at a head. - async listCompletedRuns(path, head) { - const query = new URLSearchParams({ head_sha: head, event: "pull_request", status: "completed", per_page: "10" }); + async listCompletedRuns(path, head, event = "pull_request") { + const query = new URLSearchParams({ head_sha: head, event, status: "completed", per_page: "10" }); return (await request(`${repo}/actions/workflows/${path.split("/").at(-1)}/runs?${query}`)).workflow_runs ?? []; }, // A workflow file's newest 100 runs created since `since`: many ticks' diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index 2622bac0d46..8c52d463a73 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, dispatchFeedback, failureAnnotation, FAILED_RUN_WINDOW_HOURS, IDLE_OWNER_WINDOW_HOURS, main, NO_ADOPTION, ownerAdoption, reviewRunAt, SWEEP_HEAD_CAP, sweepReleased } from "./feedback-dispatch.mjs"; +import { createGitHubApi, dispatchFeedback, 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 { onReviewedHead, threadAuthors } from "./address-feedback.mjs"; import { verifyReader } from "./verify-cap.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; @@ -2515,9 +2515,13 @@ test("a review-lane hold at the head gates a github-actions[bot] lane thread (#1 assert.ok(lines.at(-1).includes("is labeled needs-human (review-hold: "), lines.at(-1)); }); -test("the sweep does not revisit a head the human gate skipped once the label is off", async () => { - // The skipped dispatch posted no reply check, so sweepOne finds no release. - const api = { ...fakeApi({ pulls: pull(), threads: [LANE_FINDING] }), listOpenPulls: async () => pull() }; +// #12578, #12604, #12605: each review completion skipped on an artifact listing +// past the caller's limit, a cause that clears on a later read, and no review +// run followed, so one advisory lane thread held each lanes App PR until a +// person dispatched it. +const DEADLINE = "verify:unreadable: the artifact listing does not fit in the caller's limit: repository artifacts: 13962 artifacts in 140 pages need 147 s"; +const REVIEWED = upstreamRun({ id: 13, run_started_at: "2026-10-17T12:00:00Z", updated_at: "2026-10-17T12:10:00Z" }); +const sweepUnsent = async (api, verifyCapped, now = Date.parse("2026-10-17T12:20:00Z")) => { const lines = []; const result = await sweepReleased({ api, @@ -2526,10 +2530,70 @@ test("the sweep does not revisit a head the human gate skipped once the label is appSenderId: APP_ID, trustedHumans: new Set([KYLE.databaseId]), resolveSlot: async () => resolvedLane(), + verifyCapped, + now, log: (line) => lines.push(line), }); - assert.equal(result, null); - assert.deepEqual(api.calls.dispatch, []); + return { result, lines }; +}; +const unsentApi = (options = {}) => { + const api = sweepApi({ checksAt: {}, threads: [LANE_FINDING], reviewRuns: { [HEAD]: [REVIEWED] }, ...options }); + api.listWorkflowRuns = async () => options.workflowRuns ?? []; + // Completed runs of the other workflows at the head, by path. + const listCompletedRuns = api.listCompletedRuns; + api.listCompletedRuns = async (path, head, event) => [...(await listCompletedRuns(path, head, event)), ...(options.others?.[path] ?? [])]; + return api; +}; + +test("the sweep sends a lanes App head with an unresolved thread that no round was sent for, once its skip clears", async () => { + // The review completion's read did not fit; the head stays unrecorded. + const record = emptyRecord(); + const skipped = await decide(fakeApi({ pulls: pull(), threads: [LANE_FINDING] }), resolvedLane(), { record, verifyCapped: () => DEADLINE }); + assert.equal(skipped.result, null); + assert.equal(record.handled.size, 0); + + // The tick's read fits, so the head is sent through its newest review run. + const api = unsentApi(); + const { result } = await sweepUnsent(api, () => null); + assert.deepEqual(result, { number: 42, workflowRunId: "38779200001", upstreamRunId: 13 }); + assert.deepEqual(api.calls.dispatch.map((call) => call.inputs["pr-number"]), ["42"]); + + // A read that still does not fit sends nothing, and the next tick tries again. + const still = unsentApi(); + assert.equal((await sweepUnsent(still, () => DEADLINE)).result, null); + assert.deepEqual(still.calls.dispatch, []); +}); + +test("the unsent-head sweep leaves a head with a round, no unresolved thread, a review inside the grace, or a run since its review", async () => { + for (const [label, options, now] of [ + ["a round at the head", { checksAt: { [HEAD]: [answeredRound] } }], + ["every thread resolved", { threads: [thread([CLAUDE], true)] }], + ["no completed review run", { reviewRuns: {} }], + ["a review completed inside the grace", {}, Date.parse("2026-10-17T12:10:00Z") + UNSENT_GRACE_MS - 1], + ["a review with no completion time", { reviewRuns: { [HEAD]: [upstreamRun({ id: 13, run_started_at: "2026-10-17T12:00:00Z" })] } }], + // Created first, so not reviewRunAt's run, but finished inside the grace. + [ + "the other review lane finished inside the grace", + { others: { ".github/workflows/pr-review-security-hosted.yml": [upstreamRun({ id: 12, path: ".github/workflows/pr-review-security-hosted.yml", created_at: "2026-10-17T11:59:00Z", updated_at: "2026-10-17T12:15:00Z" })] } }, + ], + ["a pr-merge run finished inside the grace", { others: { ".github/workflows/pr-merge.yml": [{ ...MERGE_RUN, conclusion: "success", updated_at: "2026-10-17T12:15:00Z" }] } }], + [ + "a comment relay finished inside the grace", + { others: { ".github/workflows/pr-review-comment-relay.yml": [upstreamRun({ id: 14, path: ".github/workflows/pr-review-comment-relay.yml", event: "pull_request_review_comment", conclusion: "success", updated_at: "2026-10-17T12:15:00Z" })] } }, + ], + // A pr-verify FAIL's notes, posted just before its dispatch in a group of its own. + ["a pr-verify note posted inside the grace", { threads: [{ ...LANE_FINDING, comments: { ...LANE_FINDING.comments, nodes: LANE_FINDING.comments.nodes.map((node) => ({ ...node, createdAt: "2026-10-17T12:15:00Z" })) } }] }], + ["a run since its review", { workflowRuns: [{ id: 9, status: "completed", display_title: TITLE_42, created_at: "2026-10-17T12:05:00Z" }] }], + ["do-not-merge", { openPulls: pull({ labels: [{ name: "do-not-merge" }] }) }], + ]) { + const api = unsentApi(options); + const { result } = await sweepUnsent(api, () => null, now); + assert.equal(result, null, label); + assert.deepEqual(api.calls.dispatch, [], label); + } + // A run from before the review does not hold it. + const older = unsentApi({ workflowRuns: [{ id: 8, status: "completed", display_title: TITLE_42, created_at: "2026-10-17T11:00:00Z" }] }); + assert.equal((await sweepUnsent(older, () => null)).result?.number, 42); }); // A label coming off a PR starts pr-merge.yml on pull_request_target; its diff --git a/.github/workflows/pr-feedback-dispatch.yml b/.github/workflows/pr-feedback-dispatch.yml index d49a4c70de3..579854f5d00 100644 --- a/.github/workflows/pr-feedback-dispatch.yml +++ b/.github/workflows/pr-feedback-dispatch.yml @@ -52,7 +52,9 @@ name: pr-feedback-dispatch # such head, it dispatches one head whose pr-unblock/address-feedback status # still fails though no review thread is left unresolved, once per stop, so # the run finds nothing to fix and report-stop-status clears the status -# (sweepStale). Taking +# (sweepStale). With neither, it dispatches one head with an unresolved thread +# and no address-feedback round, whose review completion skipped on a cause +# that may clear, such as an unreadable pr-verify FAIL cap (sweepUnsent). Taking # the tick's completion rather than a schedule of this file's own keeps # every dispatch a workflow_run dispatcher's, the one kind the gate and the # broker trust with a review run as its upstream.