diff --git a/README.md b/README.md index 9de74eeb..cd3942c0 100644 --- a/README.md +++ b/README.md @@ -844,9 +844,9 @@ head the watcher itself put back in the queue continues the watcher's pick instead, until a handback ends it: the watcher's re-enqueue, GitHub's auto-merge of it, or such a workflow picking the fixer's push. A failed build or a conflict is recorded for the land fixer in `removal-.json`, with the -full log beside it. A push since the removal, a person's removal, or a reason -the watcher does not know ends the pick, and the comment says the PR waits for -a maintainer to enqueue it again. The state directory remembers each removal +full log beside it. A push since the removal, a person's removal, a hold, or a +reason the watcher does not know ends the pick, and the comment says the PR +waits for a maintainer to enqueue it again. The state directory remembers each removal handled, so a restart neither misses nor repeats one; a crash between commenting and recording finds its own comment's marker and posts nothing. The first pass reads the ten minutes before it. @@ -861,8 +861,9 @@ enqueues only the head auto-merge was armed on, with `expectedHeadOid` bound to it: GitHub keeps auto-merge across a push, but a push by anyone but the land fixer ends the pick, and a head it cannot tell yet waits for the next pass. An armed PR whose required checks fail, or that conflicts, is left alone for -`/quest-iterate` and a new pick. The watcher's own re-enqueues are among these -picks. A refusal is logged once per pick and retried each pass. +`/quest-iterate` and a new pick. One another session holds waits, armed, until +its hold clears. The watcher's own re-enqueues are among these picks. A refusal +is logged once per pick and retried each pass. With `--fixer`, the watcher takes each recorded removal, oldest first, while its PR still waits as the queue dropped it: @@ -903,6 +904,15 @@ handback, from a stop rule, the fixer, a refusal, a crash, or a timeout, is posted with its reason and a recommendation, and the PR leaves the watcher's care until a maintainer enqueues it again. A round whose pick ended while it ran, by a writer's push or enqueue, is not applied, and its comment says so. + +The watcher leaves a PR another session holds to that session: one with a hold +label, such as the `quest:active` that `/quest-iterate` takes. It reads the +PR's labels when it handles the removal, before it starts the fixer, and again +before it pushes a result or re-enqueues the PR, so a hold taken while the +fixer runs counts too. A hold ends the pick, as a person's removal does: the +watcher records that first and stops acting on the PR, then posts one comment +saying so, best-effort and never retried. A maintainer enqueues it again once +its session is done. A held flaky-test disable PR is left for a maintainer too. A round whose collection fails three passes in a row, as when GitHub refuses its handback comment, is set aside with a logged warning, freeing its fixer slot. diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index 8e027956..dc71728d 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -153,7 +153,6 @@ work, done once the Required quests below have merged. ## Required -- [Leave held PRs alone](/quest/m0/land-fixer/holds.md) - the watcher never fixes, pushes to, or re-enqueues a PR another session holds - [Workflow permission](/quest/m0/land-fixer/workflows-permission.md) - waits on the maintainer to grant moq-bot `workflows`, so pushes touching workflows are accepted ## Related diff --git a/quest/m0/land-fixer/holds.md b/quest/m0/land-fixer/holds.md deleted file mode 100644 index 46e951c5..00000000 --- a/quest/m0/land-fixer/holds.md +++ /dev/null @@ -1,47 +0,0 @@ -# [S] The land fixer leaves a held PR alone - -## Goal - -`quest land watch` neither runs the fixer on a PR another session holds, nor -pushes a fixer result to it or re-enqueues it. A hold is any hold label -`isHoldLabel` accepts (`src/core/hold.ts`), such as the `quest:active` that -`/quest-iterate` takes, or an older pin's `quest:iterate`. The watcher -checks for a hold before it starts a job and again before it pushes or -re-enqueues. A held PR gets one comment saying the watcher left it to the -session that holds it. - -Non-goals: the frozen batching gate (`quest land serve`), which only -moq-dev/moq.pro still uses until it is -[retired](/quest/m0/queue-watcher/retire.md). - -## Plan - -Decided 2026-11-03 (maintainer, /quest-complete on moq-dev/moq, planned -through moq-dev/moq's coordinator): plan this follow-up. Milestone: m0, -under the land fixer epic. moq-dev/moq lands through this fixer every day, -and m0's Plan ranks the land work first. - -What happened on -[moq-dev/moq#6949](https://github.com/moq-dev/moq/pull/6949): the -maintainer added `quest:iterate` at 2026-11-01 16:41Z for a `/quest-iterate` -session. The fixer still pushed its merge `7c10f33cbc` at 16:52Z, and moq-bot -re-armed `quest:merge` at 16:56Z, while the session was pushing its own -fixes on the same branch. The hold was lifted at 17:22Z. That was the host -gate. `quest land watch`, which replaced the gate on moq-dev/moq, has the -same gap: `src/cli/land-watch.ts` and `src/cli/land-watch-fix.ts` never read -the PR's labels for holds. - -Implementation guidance: - -- Read the hold labels with the snapshot the watcher already takes for each - PR the queue drops, and again right before a result is pushed or the PR is - re-enqueued, so a hold taken while the fixer runs is respected too. -- Open question for the starter: what happens to the pick of a held PR? - Recommended: the hold ends it, as a person's removal from the queue does - (`endPick`). A maintainer enqueues the PR again once its session finishes, - as `/quest-iterate`'s `/quest-merge` hand-off already does. The other - option is to wait and fix it once the hold clears, but by then the session - has usually pushed, and a push by anyone but the fixer already ends the - pick. -- Add a regression test: a PR the queue drops while it holds `quest:active` - is neither fixed nor re-enqueued. diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 9b23446f..744ef2cb 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -2,7 +2,8 @@ // the background and re-enqueued at the back of the queue, or handed back to a person. import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { basename, join } from "node:path"; -import { GitHub, type GitHubDependencies, GitHubGraphQLError, object, type ObjectValue, string } from "./github"; +import { isHoldLabel } from "../core"; +import { array, GitHub, type GitHubDependencies, GitHubGraphQLError, object, type ObjectValue, string } from "./github"; import { type Queued, queueHead } from "../github/merge"; import { UNREFUSED } from "./hold"; import { baseRemote, type Entry, mergeBase, type RemoteRead } from "./land"; @@ -113,12 +114,45 @@ const JUDGED: Record = { const PULL = `query($id: ID!) { node(id: $id) { ... on PullRequest { state baseRefName headRefOid mergeStateStatus mergeQueueEntry { enqueuedAt } autoMergeRequest { enabledAt } + labels(first: 100) { nodes { name } } } } }`; export async function readPull(github: GitHub, id: string): Promise { return object((await github.graphql(PULL, { id })).node); } +/** Whether another session holds the PR with a hold label, such as `/quest-iterate`'s `quest:active`. */ +export const isHeld = (pull: ObjectValue): boolean => + array(object(pull.labels).nodes).some((label) => isHoldLabel(string(label.name))); + +/** What the watcher says on a held PR, whatever step found the hold and whether a fix was pushed. */ +export const HELD = + "Another session holds this pull request, so the watcher stops acting on it and leaves it to that session: the hold ends the pick, and a maintainer enqueues it again once that session is done."; + +/** + * Say once, best-effort, that the watcher left a held PR to its session. The caller has already ended + * the pick in its state, so a comment GitHub refuses is only warned about and never retried. + */ +async function sayHeld( + github: GitHub, + repository: string, + entry: Pick, + warn: (text: string) => void, +): Promise { + try { + await post(github, { repository }, entry, `quest land watch: ${HELD}`, "held", MERGER); + } catch (error) { + warn(`quest land watch: hold comment on ${entry.number} not posted: ${String(error)}`); + } +} + +/** A hold found right before a push or a re-enqueue. */ +class Held extends Error { + constructor() { + super("held by another session"); + } +} + /** * Whether the PR is in the merge queue, or armed since `since` to join it once its required checks * pass. An older request is not a pick of the head the queue dropped. @@ -254,7 +288,7 @@ async function collectAll( } /** - * Take a removal waiting for the fixer, if its PR still waits as the queue dropped it: merge a + * Take a removal waiting for the fixer, if its PR still waits as the queue dropped it, unheld: merge a * conflict's base with the quest merge driver, or start the fixer when `slot` allows. Returns what it * did, if anything. */ @@ -296,6 +330,12 @@ async function take( mark(`${started[1].merged ? "merged" : "fixer"} ${started[0]}`); return undefined; } + if (isHeld(pull)) { + mark("held"); + endPick(state, number, entry.enabledAt); + await sayHeld(github, repository, entry, deps.warn); + return `dropped ${number} held`; + } const conflict = removal.reason === "merge_conflict"; // A conflict is merged with the current base; a failed build is fixed on the base it was built on. const base = conflict ? current : failure.plan.base; @@ -413,7 +453,8 @@ function finish(path: string, job: WatchJob): void { /** * Collect a finished round: record it in its pick's history, and either put the PR back in the queue, * at a fixed head or unchanged, or hand it back, as a stop rule, the fixer, or a refusal says. A - * round whose pick ended while it ran is dropped, and the PR says so. Returns what it did. + * round whose pick ended while it ran, or whose PR another session holds, is dropped, and the PR says + * so. Returns what it did. */ async function collect( github: GitHub, @@ -498,6 +539,16 @@ async function collect( : queued(pull, job.dropped) && !ours && job.requeuing === undefined ? "a maintainer enqueued it again while the fixer ran, and that pick stands" : undefined; + // Leave a held PR to its session: the pick and the round end first, and the hold's comment replaces + // the fixer's note. + const leave = async (): Promise => { + endPick(state, number, entry.enabledAt); + finish(path, job); + await sayHeld(github, job.repository, marked, deps.warn); + return [`dropped ${number} held`]; + }; + // A head this round already asked GitHub to queue, and GitHub took, stays: the hold came after it. + if (isHeld(pull) && !(job.requeuing !== undefined && queued(pull, job.dropped))) return leave(); if (ended) { await note(); await post( @@ -579,6 +630,9 @@ async function collect( job.published = head; save(path, job); } + // Validating the fix takes a while, and a session may have taken the PR since. A head already + // pushed needs no push, and `requeue` checks the hold before any enqueue. + if (!ours && isHeld(await readPull(github, entry.id))) throw new Held(); await publishHead(github, context.root, job, entry, entry.head, current, head, deps, context.token, { reason: result.reason, attribution, @@ -591,6 +645,7 @@ async function collect( job.requeued = await requeue(github, entry.id, head, job.base.ref, job.dropped); save(path, job); } catch (failure) { + if (failure instanceof Held) return [...said, ...(await leave())]; if (!(failure instanceof FixerRefusal)) throw failure; handback = failure.message; if (failure.resolution) job.resolution = failure.resolution; @@ -633,7 +688,8 @@ async function collect( /** * Put `head` back in `base`'s merge queue, at the back. A PR queued or armed at it since the queue * dropped it, at `dropped`, needs nothing more: a restart finds its own request, and a workflow may pick - * the fixer's push first. A refusal, or a PR retargeted from `base`, hands the PR back. + * the fixer's push first. A refusal, or a PR retargeted from `base`, hands the PR back; a hold leaves + * it to the session that holds it. */ async function requeue(github: GitHub, id: string, head: string, base: string, dropped: string): Promise { const how = (pull: ObjectValue): Queued => (pull.mergeQueueEntry != null ? "enqueued" : "armed"); @@ -641,6 +697,7 @@ async function requeue(github: GitHub, id: string, head: string, base: string, d if (string(pull.baseRefName) !== base) throw new FixerRefusal(`retargeted from ${base} before it was re-enqueued`); if (string(pull.headRefOid) !== head) throw new FixerRefusal("head moved before it was re-enqueued"); if (!queued(pull, dropped)) { + if (isHeld(pull)) throw new Held(); try { await queueHead( (query, variables) => github.graphql(query, variables), @@ -665,7 +722,7 @@ async function requeue(github: GitHub, id: string, head: string, base: string, d /** * Open each validated disable as a PR of its own and enqueue it like any other, with no maintainer's * pick: its Review records the fixer's change. A disable that fails to advance is retried on the next - * pass; one GitHub refuses is left for a maintainer. + * pass; one GitHub refuses, or another session holds, is left for a maintainer. */ async function advanceDisables( github: GitHub, @@ -680,7 +737,15 @@ async function advanceDisables( if (!test.disable || test.disable.done !== undefined) continue; try { output.push( - await openDisable(github, repository, context, test, test.disable, () => saveFlaky(state, tests)), + await openDisable( + github, + repository, + context, + test, + test.disable, + () => saveFlaky(state, tests), + deps.warn, + ), ); } catch (error) { deps.warn(`quest land watch: disable of ${test.disable.quest} pending: ${String(error)}`); @@ -708,6 +773,7 @@ async function openDisable( test: FlakyTest, disable: Disable, record: () => void, + warn: (text: string) => void, ): Promise { const name = questName(disable.quest); if (disable.id === undefined) { @@ -745,14 +811,22 @@ async function openDisable( disable.id = string(pull.node_id); record(); } + const entry = { number: disable.number!, enabledAt: disable.head }; try { disable.done = await requeue(github, disable.id, disable.head, disable.base.ref, ""); } catch (error) { + if (error instanceof Held) { + // Left for a maintainer, as a refused one is, once recorded. + disable.done = "held"; + record(); + await sayHeld(github, repository, entry, warn); + return `disable held ${name} ${disable.number}`; + } if (!(error instanceof FixerRefusal)) throw error; await post( github, { repository }, - { number: disable.number!, enabledAt: disable.head }, + entry, `quest land watch: ${fixerText(error.message)}, so this pull request waits for a maintainer to enqueue it.`, "refused", MERGER, diff --git a/src/cli/land-watch.ts b/src/cli/land-watch.ts index 32fbd10a..7f8cf28a 100644 --- a/src/cli/land-watch.ts +++ b/src/cli/land-watch.ts @@ -28,7 +28,16 @@ import { save, startDaemon, } from "./land-serve"; -import { advanceFixes, endPick, type FixContext, readPull, readRemoval, requeuedHeads } from "./land-watch-fix"; +import { + advanceFixes, + endPick, + type FixContext, + HELD, + isHeld, + readPull, + readRemoval, + requeuedHeads, +} from "./land-watch-fix"; import { enabledHead, headBefore } from "./status"; export { readRemoval } from "./land-watch-fix"; @@ -49,9 +58,10 @@ export interface WatchOptions { /** * What a removal means for its pick: the fixer's to take (`fix`), or the end of the pick, left alone * until a writer enqueues the PR again: a push since the queue dropped it (`pushed`), a person's - * removal (`removed`), or a reason the watcher does not know (`unrecognized`). + * removal (`removed`), a hold label another session keeps on it (`held`), or a reason the watcher + * does not know (`unrecognized`). */ -export type Verdict = "fix" | "pushed" | "removed" | "unrecognized" | "unknown"; +export type Verdict = "fix" | "pushed" | "removed" | "held" | "unrecognized" | "unknown"; /** A `merge_group` job that failed, or was still running when its checks timed out. */ export interface FailedJob { @@ -139,6 +149,7 @@ const OPEN = `query($owner: String!, $name: String!, $base: String!, $qualified: nodes { id number url title headRefOid headRefName maintainerCanModify mergeStateStatus headRepository { nameWithOwner } + labels(first: 100) { nodes { name } } mergeQueueEntry { id } autoMergeRequest { enabledAt commitBody } removals: timelineItems(since: $since, first: 20, itemTypes: [REMOVED_FROM_MERGE_QUEUE_EVENT]) { ${REMOVALS} } @@ -227,7 +238,7 @@ async function pickOf( * `expectedHeadOid` bound to it: GitHub keeps auto-merge across a writer's push, but that push ends the * pick, and a head `enabledHead` cannot tell yet waits for the next pass. The PR is read again just * before the enqueue, so a request disabled or a PR retargeted since the pass read it is not enqueued, - * which `expectedHeadOid` alone would not catch. An armed pick whose required + * which `expectedHeadOid` alone would not catch, and neither is a PR another session holds. An armed pick whose required * checks fail, or that conflicts, is not ready and is left alone. A refusal is reported once per pick * and retried each pass; one because GitHub queued the PR first says nothing. */ @@ -260,7 +271,9 @@ async function enqueueArmed( string(now.headRefOid) !== head || now.mergeQueueEntry != null || request === null || - string(request.enabledAt) !== armed + string(request.enabledAt) !== armed || + // Another session holds it: leave the pick armed, for the next pass once the hold clears. + isHeld(now) ) continue; try { @@ -389,6 +402,7 @@ const AFTER: Record string> = { `\n\nIts head has moved to ${current} since, and a push by anyone but the land fixer ends the pick: the watcher leaves this pull request alone until a maintainer enqueues it again.`, removed: () => "\n\nA person's removal ends the pick: the watcher leaves this pull request alone until a maintainer enqueues it again.", + held: () => `\n\n${HELD}`, unrecognized: () => "\n\nThe watcher does not recognize this reason, so it leaves this pull request alone until a maintainer enqueues it again.", unknown: () => @@ -434,6 +448,9 @@ async function handle( // An unknown head never authorizes the fixer: wait for GitHub to list the push, then end the pick. if (known === null && now - Date.parse(at) < UNLISTED) return null; const head = known ?? current; + // A hold recorded for this removal, as before a crash, ends the pick even once it clears. + const prior = readRemoval(state, number); + const held = (prior?.event === string(event.id) && prior.verdict === "held") || isHeld(pull); const { pick, queued } = head === current ? first : await pickOf(github, string(pull.id), event, head, app, own); const verdict: Verdict = known === null @@ -442,9 +459,11 @@ async function handle( ? "pushed" : reason === "manual" ? "removed" - : FIXABLE.includes(reason) - ? "fix" - : "unrecognized"; + : !FIXABLE.includes(reason) + ? "unrecognized" + : held + ? "held" + : "fix"; const build = reason === "failed_checks" || reason === "checks_timed_out" ? await failedBuild( @@ -483,17 +502,6 @@ async function handle( build.jobs.length === 0 && reason !== "merge_conflict" && FIXABLE.includes(reason) ? `\n\nThe watcher found no failing \`merge_group\` job for it before ${at}.` : ""; - await post( - github, - { repository }, - { number, enabledAt: pick.at }, - `quest land watch: the merge queue removed this pull request at ${head} because ${because(reason, actor, base.ref)} (\`${reason.replace(/[^a-z_]/g, "")}\`).${AFTER[verdict](current, fixing)}${missing}`, - `removed ${string(event.id)}`, - "quest land watch", - undefined, - (room) => jobsSection(blocks, room), - ); - const entry: Entry = { id: string(pull.id), number, @@ -533,12 +541,29 @@ async function handle( } : {}), }; - // A removal GitHub showed late never replaces the record of a newer one. - if ((readRemoval(state, number)?.at ?? "") <= at) { + // Recorded before its comment, so a hold ends the pick whatever becomes of the comment. A removal + // GitHub showed late never replaces the record of a newer one. + if ((prior?.at ?? "") <= at) { save(join(state, `removal-${number}.json`), removal); // Any other verdict ends the pick: its fixer history goes, so a later enqueue starts afresh. if (verdict !== "fix") endPick(state, number, pick.at); } + const said = post( + github, + { repository }, + { number, enabledAt: pick.at }, + `quest land watch: the merge queue removed this pull request at ${head} because ${because(reason, actor, base.ref)} (\`${reason.replace(/[^a-z_]/g, "")}\`).${AFTER[verdict](current, fixing)}${missing}`, + `removed ${string(event.id)}`, + "quest land watch", + undefined, + (room) => jobsSection(blocks, room), + ); + // A held removal's comment is the hold's, so it is best-effort: a refusal fails no pass and is not retried. + if (verdict === "held") + await said.catch((error: unknown) => + note(`quest land watch: hold comment on ${number} not posted: ${String(error)}`), + ); + else await said; return `removed ${number} ${reason} ${verdict}`; } diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index 45907da3..1faac133 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -15,7 +15,7 @@ import { CHECK_LOG } from "../src/cli/land-checks"; import { COLLECTION_ATTEMPTS, REQUEUE_LIMIT, history } from "../src/cli/land-fixer"; import { privateRepository } from "../src/cli/land-serve"; import { jobsSection, readRemoval, SETTLE, watchPass, watchPermissions } from "../src/cli/land-watch"; -import type { FixContext } from "../src/cli/land-watch-fix"; +import { type FixContext, HELD } from "../src/cli/land-watch-fix"; import { type FlakyTest, RECENT, readFlaky, saveFlaky, writeKnownFlaky } from "../src/cli/land-watch-flaky"; import { FLAKY_INPUT, type KnownFlaky } from "../src/cli/land-fix"; import { configure, GIT_SETTINGS, initRepository, tempDir } from "./fixture"; @@ -66,6 +66,7 @@ interface Pull { branch?: string; /** Its head branch's pushes, oldest first, in place of those its removals imply. */ pushes?: { sha: string; at: string }[]; + labels?: string[]; } interface Job { id: number; @@ -95,8 +96,8 @@ interface World { now: number; /** The upstream repository that holds each PR's branch, `branch-`. */ origin?: string; - /** A comment GitHub refuses to post, once. */ - refuse?: (body: string) => boolean; + /** Texts whose comment GitHub refuses to post, each once, found in `# `. */ + refuse?: string[]; /** A read of a PR's queue state that fails, once. */ unreadable?: (pull: Pull) => boolean; /** PRs whose push activity GitHub has not listed yet. */ @@ -108,6 +109,8 @@ interface World { onActivity?: (pull: Pull) => void; /** GitHub's refusal of an enqueue of `pull`, if it refuses one. */ refuseEnqueue?: (pull: Pull) => string | undefined; + /** Called on each read of a PR's queue state, as a change GitHub makes while the watcher reads it. */ + onPull?: (pull: Pull) => void; } const at = (minutes: number) => new Date(Date.UTC(2026, 10, 1, 12, minutes)).toISOString().replace(".000", ""); @@ -122,6 +125,7 @@ function headOf(world: World, pull: Pull): string { } const branchOf = (pull: Pull) => pull.branch ?? `branch-${pull.number}`; +const labelsOf = (pull: Pull) => ({ nodes: (pull.labels ?? []).map((name) => ({ name })) }); function git(cwd: string, ...args: string[]): string { return execFileSync("git", args, { cwd, encoding: "utf8" }).trim(); @@ -226,6 +230,7 @@ function deps(world: World): GitHubDependencies { } if (query.includes("mergeStateStatus") && !query.includes("pullRequests(states: OPEN")) { const pull = pullOf(); + world.onPull?.(pull); if (world.unreadable?.(pull)) { world.unreadable = undefined; return Response.json({ errors: [{ type: "INTERNAL", message: "unavailable" }] }); @@ -239,6 +244,7 @@ function deps(world: World): GitHubDependencies { mergeStateStatus: pull.mergeState ?? "CLEAN", mergeQueueEntry: pull.queued ? { enqueuedAt: pull.queued } : null, autoMergeRequest: pull.armed ? { enabledAt: pull.armed } : null, + labels: labelsOf(pull), }, }, }); @@ -281,6 +287,7 @@ function deps(world: World): GitHubDependencies { mergeStateStatus: pull.mergeState ?? "CLEAN", mergeQueueEntry: pull.queued ? { id: `MQE_${pull.number}` } : null, autoMergeRequest: pull.armed ? { enabledAt: pull.armed, commitBody: "" } : null, + labels: labelsOf(pull), removals: removalPage(pull, variables.since, null), })), pageInfo: { hasNextPage: false, endCursor: null }, @@ -361,8 +368,10 @@ function deps(world: World): GitHubDependencies { const list = (world.comments[Number(comments[1])] ??= []); if (init?.method === "POST") { const comment = { user: { login: `${APP}[bot]` }, body: JSON.parse(String(init.body)).body }; - if (world.refuse?.(comment.body)) { - world.refuse = undefined; + const refused = + world.refuse?.findIndex((text) => `#${comments[1]} ${comment.body}`.includes(text)) ?? -1; + if (refused >= 0) { + world.refuse!.splice(refused, 1); return Response.json({ message: "refused" }, { status: 422 }); } list.push(comment); @@ -757,12 +766,15 @@ test("a refused enqueue of an armed pick is reported once and retried, and says expect(JSON.parse(readFileSync(join(state, "watch.json"), "utf8")).refused).toEqual({}); }); -test("an armed pick withdrawn or retargeted while the pass reads it is not enqueued", async () => { +test("an armed pick withdrawn, retargeted, or held while the pass reads it is not enqueued", async () => { const state = tempDir(); const github = world([ { number: 32, head: HEAD, enqueues: [], removals: [], armed: at(2) }, { number: 33, head: HEAD, enqueues: [], removals: [], armed: at(2) }, { number: 34, head: HEAD, enqueues: [], removals: [], armed: at(2) }, + // Held when the pass reads it, or once it does. + { number: 35, head: HEAD, enqueues: [], removals: [], armed: at(2), labels: ["quest:active"] }, + { number: 36, head: HEAD, enqueues: [], removals: [], armed: at(2) }, ]); github.now = Date.parse(at(4)); github.onActivity = (pull) => { @@ -770,9 +782,19 @@ test("an armed pick withdrawn or retargeted while the pass reads it is not enque if (pull.number === 33) pull.base = "release"; // Disabled and armed again: a new pick, which the next pass weighs on its own. if (pull.number === 34) pull.armed = at(3); + if (pull.number === 36) pull.labels = ["quest:iterate"]; }; expect(await watchPass(REPOSITORY, { state }, deps(github))).toEqual([]); expect(github.mutations).toEqual([]); + + // The holds clear with the pick still armed at its head: the next pass enqueues it, with 34's new pick. + github.onActivity = undefined; + for (const pull of github.pulls.slice(3)) pull.labels = []; + expect(await watchPass(REPOSITORY, { state }, deps(github))).toEqual([ + `enqueued 34 ${HEAD}`, + `enqueued 35 ${HEAD}`, + `enqueued 36 ${HEAD}`, + ]); }); test("the watcher's tokens ask for writes only with a fixer, and pushes for workflows only when they carry one", () => { @@ -1091,12 +1113,15 @@ describe.skipIf(process.platform === "win32")("the watcher's land fixer", () => // retried rather than handed back, and finds its own enqueue. github.unreadable = (pull) => pull.queued !== undefined; expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + // A session takes it now: the head GitHub already took back stays queued. + pull.labels = ["quest:active"]; // Then GitHub refuses the outcome comment once: the round stays requeued, and only its comment is retried. - github.refuse = (body) => body.includes("re-enqueued this pull request"); + github.refuse = ["re-enqueued this pull request"]; expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); const [line] = await watchPass(REPOSITORY, { state }, deps(github), context); const fixed = git(repo.origin, "rev-parse", "refs/heads/branch-5"); expect(line).toBe(`requeued 5 ${fixed}`); + pull.labels = undefined; expect(git(repo.origin, "rev-parse", `${fixed}^`)).toBe(head); expect(github.mutations).toEqual([`enqueuePullRequest 5 ${fixed}`]); expect(pull.body).toContain(`mechanical fix by Codex: \`add the missing file\` (head ${fixed})`); @@ -1412,17 +1437,19 @@ printf '[{"number":%s,"action":"fixed","head":"%s","reason":"preserve both confl expect(readFlaky(state)[0].verdicts).toHaveLength(1); }); - test("a test seen flaking twice is disabled in its own pull request, which the watcher enqueues", async () => { - const { repo, state, fix, context } = setup(); - const base = repo.main(); - const heads = [5, 6, 7].map((number) => repo.pull(number, { [`feature-${number}.txt`]: "new\n" })); - const github = { ...world([], []), origin: repo.origin }; - /** A flaky requeue naming render_timeout, disabling it at the checkout's HEAD after `script`. */ - const flaky = (script: string) => `${script} + test.each([false, true])( + "a test seen flaking twice is disabled in its own pull request, which the watcher enqueues unless it is held (held: %s)", + async (held) => { + const { repo, state, fix, context } = setup(); + const base = repo.main(); + const heads = [5, 6, 7].map((number) => repo.pull(number, { [`feature-${number}.txt`]: "new\n" })); + const github = { ...world([], []), origin: repo.origin }; + /** A flaky requeue naming render_timeout, disabling it at the checkout's HEAD after `script`. */ + const flaky = (script: string) => `${script} printf '[{"number":%s,"action":"requeue","reason":"healthy; flaky test","cause":"flaky","failure":"%s","disable":"%s"}]' "$2" "$FAILURE" "$(git rev-parse HEAD)" > quest-land-result.json `; - /** A disabling commit on base that skips the test, adds its follow-up quest, and writes `extra`. */ - const disabling = (extra: string) => `grep -q 'just test: render_timeout' .git/quest-land-flaky.json + /** A disabling commit on base that skips the test, adds its follow-up quest, and writes `extra`. */ + const disabling = (extra: string) => `grep -q 'just test: render_timeout' .git/quest-land-flaky.json git checkout -q -b disable base mkdir -p quest/m0/flaky printf '# [S] Fix or delete render_timeout\\n\\n## Goal\\n\\nFix the flaky test or delete it.\\n' > quest/m0/flaky/render-timeout.md @@ -1431,82 +1458,105 @@ printf 'skip render_timeout\\n' > skip.txt ${extra} git add quest skip.txt ${extra ? "flake.nix" : ""} git commit -qm 'disable render_timeout'`; - // The first verdict's disable is ignored: one judgment alone never disables a test. The second's edits a - // protected path and is refused, and the third's is opened as its own pull request. - // Each names the test with different quoting and spacing, which still makes one test. - fix.round(5, flaky("[ ! -e .git/quest-land-flaky.json ]\nFAILURE='just test: `render_timeout`'")); - fix.round( - 6, - flaky(`${disabling("printf 'changed\\n' > flake.nix")}\nFAILURE='just test: \\"render_timeout\\"'`), - ); - fix.round(7, flaky(`${disabling("")}\nFAILURE='just test: render_timeout'`)); - const said: string[][] = []; - for (const [index, number] of [5, 6, 7].entries()) { - const minute = (index + 1) * 10; - github.pulls.push({ - number, - head: heads[index], - enqueues: [{ createdAt: at(minute - 5), actor: "maintainer" }], - removals: [ - { - id: `R${number}`, - createdAt: at(minute), - reason: "failed_checks", - actor: "github-merge-queue", - before: heads[index], - }, + // The first verdict's disable is ignored: one judgment alone never disables a test. The second's edits a + // protected path and is refused, and the third's is opened as its own pull request. + // Each names the test with different quoting and spacing, which still makes one test. + fix.round(5, flaky("[ ! -e .git/quest-land-flaky.json ]\nFAILURE='just test: `render_timeout`'")); + fix.round( + 6, + flaky(`${disabling("printf 'changed\\n' > flake.nix")}\nFAILURE='just test: \\"render_timeout\\"'`), + ); + fix.round(7, flaky(`${disabling("")}\nFAILURE='just test: render_timeout'`)); + const said: string[][] = []; + for (const [index, number] of [5, 6, 7].entries()) { + const minute = (index + 1) * 10; + github.pulls.push({ + number, + head: heads[index], + enqueues: [{ createdAt: at(minute - 5), actor: "maintainer" }], + removals: [ + { + id: `R${number}`, + createdAt: at(minute), + reason: "failed_checks", + actor: "github-merge-queue", + before: heads[index], + }, + ], + }); + github.runs.push({ ...failedRun(number, at(minute - 2), base), id: number * 10 }); + github.now = Date.parse(at(minute + 1)); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + `removed ${number} failed_checks fix`, + `fixing ${number}`, + ]); + await exited(state); + github.now = Date.parse(at(minute + 2)); + // A session takes the disable's pull request as soon as it opens. + if (held && number === 7) + github.onPull = (pull) => { + if (pull.number === 8) pull.labels = ["quest:active"]; + }; + said.push(await watchPass(REPOSITORY, { state }, deps(github), context)); + } + const branch = github.pulls.find((pull) => pull.number === 8)?.branch; + expect(branch).toMatch(/^quest-land\/flaky-[0-9a-f]{12}$/); + const head = git(repo.origin, "rev-parse", `refs/heads/${branch}`); + expect(said).toEqual([ + ["disable ignored 5: first flaky verdict", `requeued 5 ${heads[0]}`], + [expect.stringMatching(/^disable refused 6: Error: off-limits fixer edit/), `requeued 6 ${heads[1]}`], + [ + `disable 7 ${head}`, + `requeued 7 ${heads[2]}`, + held ? "disable held render-timeout 8" : "disabled render-timeout 8", ], - }); - github.runs.push({ ...failedRun(number, at(minute - 2), base), id: number * 10 }); - github.now = Date.parse(at(minute + 1)); - expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ - `removed ${number} failed_checks fix`, - `fixing ${number}`, ]); - await exited(state); - github.now = Date.parse(at(minute + 2)); - said.push(await watchPass(REPOSITORY, { state }, deps(github), context)); - } - const branch = github.pulls.find((pull) => pull.number === 8)?.branch; - expect(branch).toMatch(/^quest-land\/flaky-[0-9a-f]{12}$/); - const head = git(repo.origin, "rev-parse", `refs/heads/${branch}`); - expect(said).toEqual([ - ["disable ignored 5: first flaky verdict", `requeued 5 ${heads[0]}`], - [expect.stringMatching(/^disable refused 6: Error: off-limits fixer edit/), `requeued 6 ${heads[1]}`], - [`disable 7 ${head}`, `requeued 7 ${heads[2]}`, "disabled render-timeout 8"], - ]); - // The disable is the fixer's commit on the base, enqueued like any other pull request with no pick. - expect(git(repo.origin, "rev-parse", `${head}^`)).toBe(base); - expect(git(repo.origin, "show", `${head}:quest/m0/flaky/render-timeout.md`)).toContain("Fix the flaky test"); - expect(github.mutations).toEqual([ - `enqueuePullRequest 5 ${heads[0]}`, - `enqueuePullRequest 6 ${heads[1]}`, - `enqueuePullRequest 7 ${heads[2]}`, - `enqueuePullRequest 8 ${head}`, - ]); - const opened = github.pulls.find((pull) => pull.number === 8)!; - expect(opened.title).toBe("quest land: disable the flaky test render-timeout"); - expect(opened.body).toContain( - "The land fixer judged `just test: render_timeout` flaky 3 times, most recently in", - ); - expect(opened.body).toContain( - "#5 ([job 1](https://github.com/upstream/project/actions/runs/50/job/71)), #6 ([job 1](https://github.com/upstream/project/actions/runs/60/job/71))", - ); - expect(opened.body).toContain("adds its follow-up quest, `quest/m0/flaky/render-timeout.md`"); - expect(opened.body).toContain(`## Review\n\nmechanical fix by Codex: \``); - expect(opened.body).toContain(`(head ${head})`); - expect(readFlaky(state)).toMatchObject([ - { - failure: "just test: render_timeout", - verdicts: [{ number: 5 }, { number: 6 }, { number: 7 }], - disable: { head, quest: "quest/m0/flaky/render-timeout.md", number: 8, done: "enqueued" }, - }, - ]); + if (held) { + // Left for a maintainer, even once the hold clears. + github.onPull = undefined; + github.pulls.find((pull) => pull.number === 8)!.labels = []; + expect(github.comments[8].map((comment) => comment.body)).toEqual([expect.stringContaining(HELD)]); + } + // The disable is the fixer's commit on the base, enqueued like any other pull request with no pick. + expect(git(repo.origin, "rev-parse", `${head}^`)).toBe(base); + expect(git(repo.origin, "show", `${head}:quest/m0/flaky/render-timeout.md`)).toContain( + "Fix the flaky test", + ); + expect(github.mutations).toEqual([ + `enqueuePullRequest 5 ${heads[0]}`, + `enqueuePullRequest 6 ${heads[1]}`, + `enqueuePullRequest 7 ${heads[2]}`, + ...(held ? [] : [`enqueuePullRequest 8 ${head}`]), + ]); + const opened = github.pulls.find((pull) => pull.number === 8)!; + expect(opened.title).toBe("quest land: disable the flaky test render-timeout"); + expect(opened.body).toContain( + "The land fixer judged `just test: render_timeout` flaky 3 times, most recently in", + ); + expect(opened.body).toContain( + "#5 ([job 1](https://github.com/upstream/project/actions/runs/50/job/71)), #6 ([job 1](https://github.com/upstream/project/actions/runs/60/job/71))", + ); + expect(opened.body).toContain("adds its follow-up quest, `quest/m0/flaky/render-timeout.md`"); + expect(opened.body).toContain(`## Review\n\nmechanical fix by Codex: \``); + expect(opened.body).toContain(`(head ${head})`); + expect(readFlaky(state)).toMatchObject([ + { + failure: "just test: render_timeout", + verdicts: [{ number: 5 }, { number: 6 }, { number: 7 }], + disable: { + head, + quest: "quest/m0/flaky/render-timeout.md", + number: 8, + done: held ? "held" : "enqueued", + }, + }, + ]); - // A restart neither opens nor enqueues it again, and the disabled test is no longer offered as known. - expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); - expect(github.mutations).toHaveLength(4); - }); + // A restart neither opens nor enqueues it again, and the disabled test is no longer offered as known. + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + expect(github.mutations).toHaveLength(held ? 3 : 4); + }, + ); test("a pick that ends while the fixer runs is left alone, and every handback ends the pick", async () => { const { repo, state, fix, context } = setup(5); @@ -1749,4 +1799,106 @@ git commit -qm 'disable render_timeout'`; github.now = Date.parse(at(27)); expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([`requeued 12 ${head}`]); }); + test("a held PR is neither fixed nor re-enqueued, whenever the hold is taken", async () => { + const { repo, state, fix, context } = setup(3); + const base = repo.main(); + const heads = new Map( + [5, 6, 7, 8, 9].map((number) => [number, repo.pull(number, { [`f-${number}.txt`]: "new\n" })]), + ); + const removed = (number: number, labels?: string[]): Pull => ({ + number, + head: heads.get(number)!, + labels, + enqueues: [{ createdAt: at(1), actor: "maintainer" }], + removals: [ + { + id: `R${number}`, + createdAt: at(5), + reason: "failed_checks", + actor: "github-merge-queue", + before: heads.get(number), + }, + ], + }); + // 5 is held when the queue drops it; `quest:merge`, an older pin's leftover, holds nothing. + const pulls = [removed(5, ["quest:active"]), removed(6, ["quest:merge"]), removed(7), removed(8), removed(9)]; + const github = { + ...world( + pulls, + pulls.map((pull) => ({ ...failedRun(pull.number, at(3), base), id: pull.number })), + ), + origin: repo.origin, + }; + github.now = Date.parse(at(6)); + fix.round(6, fixScript("fixed.txt")); + fix.round(7, fixScript("fixed.txt")); + fix.round(8, flakyScript); + // Each hold the watcher sees ends the pick, and its comment is best-effort: GitHub refuses 5's here, + // and 7's below, and neither is retried once the hold clears. + github.refuse = ["Another session holds"]; + // Three fixer slots: 9 waits. + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + expect.stringMatching(/^quest land watch: hold comment on 5 not posted: /), + "removed 5 failed_checks held", + "removed 6 failed_checks fix", + "removed 7 failed_checks fix", + "removed 8 failed_checks fix", + "removed 9 failed_checks fix", + "fixing 6", + "fixing 7", + "fixing 8", + ]); + // A crash before the pass marked 5's removal handled: its recorded hold still ends the pick once it + // clears, and the pass that handles it again posts the removal's comment. + pulls[0].labels = []; + const progress = join(state, "watch.json"); + const { handled, ...rest } = JSON.parse(readFileSync(progress, "utf8")); + writeFileSync(progress, JSON.stringify({ ...rest, handled: { ...handled, 5: {} } })); + await exited(state, 3); + // A session takes 6 while the watcher validates its fix, 7 right after the watcher pushes its fix, + // 8 while its fixer runs, and 9 while it waits. + const reads = new Map(); + const takeAt = new Map([ + [6, 2], + [7, 3], + ]); + github.onPull = (pull) => { + const read = (reads.get(pull.number) ?? 0) + 1; + reads.set(pull.number, read); + if (takeAt.get(pull.number) === read) pull.labels = [...(pull.labels ?? []), "quest:active"]; + }; + pulls[3].labels = ["quest:iterate"]; + pulls[4].labels = ["quest:active"]; + github.refuse = ["#7 quest land watch: Another session holds"]; + github.now = Date.parse(at(7)); + expect((await watchPass(REPOSITORY, { state }, deps(github), context)).sort()).toEqual([ + "dropped 6 held", + "dropped 7 held", + "dropped 8 held", + "dropped 9 held", + "removed 5 failed_checks held", + ]); + expect(github.mutations).toEqual([]); + expect(git(repo.origin, "rev-parse", "refs/heads/branch-6")).toBe(heads.get(6)); + expect(git(repo.origin, "rev-parse", "refs/heads/branch-7")).not.toBe(heads.get(7)); + expect(fix.calls().map((call) => call.split(" ").at(-1))).toEqual(["6", "7", "8"]); + for (const pull of pulls) expect(history(state, pull.number)).toBeUndefined(); + expect(readRemoval(state, 5)!.verdict).toBe("held"); + expect(readRemoval(state, 9)!.taken).toBe("held"); + + // The holds clear: nothing is fixed, re-enqueued, or commented again. + github.onPull = undefined; + for (const pull of pulls) pull.labels = []; + github.now = Date.parse(at(8)); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + expect(github.mutations).toEqual([]); + const held = (number: number) => + (github.comments[number] ?? []).filter((comment) => comment.body.includes(HELD)).length; + expect([5, 6, 7, 8, 9].map(held)).toEqual([1, 1, 0, 1, 1]); + // 8's round is not applied, as one whose pick ended is not: its flaky verdict does not count, and its + // fixer's note is not posted, so the hold's comment is the only one after the removal's. + expect(readFlaky(state)).toEqual([]); + expect(github.comments[8]).toHaveLength(2); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + }); });