diff --git a/README.md b/README.md index 29457d07..a6bb9307 100644 --- a/README.md +++ b/README.md @@ -558,7 +558,10 @@ follows the fixer's Git alternates. Pack indexes are rebuilt with checks every object and link, since the import enables `fetch.fsckObjects=true`. It records `mechanical fix by : ` and the new head in `## Review`, and enables head-pinned auto-merge again on the fixed head, which keeps the place of the pick it descends from. Healthy PRs go back to batches unchanged, -without a comment. Forks refusing maintainer edits, refused pushes, invalid results, +without a comment. A PR the daemon moved to its own recorded base merge before +parking it is collected at that merge: a requeue pins it, and a fix, built on the +pin, replaces it with a lease, since building on it would revert its newer base. +Any other moved head hands back. Forks refusing maintainer edits, refused pushes, invalid results, crashes, or the 60-minute fixer deadline hand back with a marked comment naming the exit status or signal, with the tail of the job's retained log. A validated fix whose push GitHub refuses, such as a merged base's `.github/workflows` changes that the App lacks @@ -645,7 +648,10 @@ and is rebuilt and checked on the new base. After an API refusal, the daemon re-reads mergeability and briefly retries pending mergeability or a base refresh. It also retries while GitHub still reports the pin as the head of a PR it just prepared, since GitHub can refuse the -merge for that stale head; only another writer's head ejects `head_moved`. +merge for that stale head; only another writer's head ejects `head_moved`. It +likewise retries while GitHub calls the merge it just pushed conflicting, since +that merge holds the tested base; if GitHub still does after about 15 seconds, +the next round rebuilds the queue on the base as it then is. A confirmed conflict parks only that PR for the fixer and a moved head ejects only that PR; the rest stays queued. A persistent policy refusal ejects as `merge_refused` without the fixer. Already-merged PRs are skipped. An unexpected API squash parent means the diff --git a/quest/m0/land-fixer/mergeability.md b/quest/m0/land-fixer/mergeability.md index 85abaf28..d7643194 100644 --- a/quest/m0/land-fixer/mergeability.md +++ b/quest/m0/land-fixer/mergeability.md @@ -27,3 +27,8 @@ Since batch triage ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/35 `merge_conflict` already parks the PR for the fixer (`fail` in `src/cli/land-serve.ts`) instead of ejecting it, and the rest of the batch stays queued; this quest adds the local merge before parking. + +GitHub also flags the gate's own freshly pushed base merge as conflicting, from +the pin's stale mergeability ([moq-dev/quest#342](https://github.com/moq-dev/quest/pull/342)). +`mergePinned` now retries that case, since the merge holds the tested base. What +remains is a head the gate did not merge in that round, which GitHub flags anyway. diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index dbd0e26d..e1a83d7b 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -36,7 +36,7 @@ import { COMMENT_LIMIT, resolution, resolutionSection, type Resolution } from ". import { readFixResult, parseBaseFix, parseFixResult, type BaseFix, type FixResult } from "./land-fix"; import { readRemote, type RemoteRead } from "./land"; import { importFixer } from "./land-fixer-repository"; -import { push, remoteHead } from "./land-head"; +import { isPrepared, push, remoteHead } from "./land-head"; import { BASE_JOB, type Halt, haltNotice, readHalt, saveHalt } from "./land-halt"; import { isParked, parked } from "./land-park"; import { excerpt, type Failure } from "./land-serve"; @@ -725,7 +725,12 @@ async function collectJob( ); const auto = object(queued.node).autoMergeRequest; let previous = attempt(state, number)!; - const head = result?.action === "fixed" ? result.head : entry.head; + // The head the PR stands at: its pinned head, or the gate's own base merge on it for this pick, as + // when the gate pushed that merge and then failed to land it (moq-dev/quest#342). The gate's merge is + // known only from its private record, never from its forgeable author or message. + const standing = + current !== entry.head && isPrepared(root, state, { ...entry, head: current }) ? current : entry.head; + const head = result?.action === "fixed" ? result.head : standing; const actor = { agent: result?.agent, model: result?.model }; const queuedAs = auto == null ? null : string(object(auto).enabledAt); // This collection already enabled auto-merge again on the fixed head, before a restart. @@ -739,7 +744,7 @@ async function collectJob( if (!samePick(previous, entry)) error = "fixer was superseded by a newer maintainer pick; keeping its work untouched"; // Never displace a newer maintainer pick, closed PR, or writer's commit. - if (pull.state !== "open" || (current !== entry.head && !(job.published.includes(number) && current === head))) + if (pull.state !== "open" || (current !== standing && !(job.published.includes(number) && current === head))) error = "PR closed or head moved while fixing; inspect and pick it again"; const left = job.left?.includes(number) ?? false; // A parked PR keeps its auto-merge; one an older job's ejection left without it is requeued below. @@ -781,6 +786,9 @@ async function collectJob( save(path, job); } if (result?.action === "fixed" && current !== head) { + // The fix descends from the pin, not from a gate merge the PR stands at: it replaces that + // merge, as the gate replaces its own, since building on it would revert the newer base it + // merged. The lease still refuses any writer's commit. const pushed = await push( root, `https://github.com/${entry.repository}.git`, @@ -788,12 +796,17 @@ async function collectJob( head, `refs/heads/${entry.branch}`, token, + standing, ); if (pushed.status !== 0) { // GitHub refuses an App's push of a workflow change it lacks the permission for, and a - // fork's branch rules may refuse any. The maintainer's own push may still pass. + // fork's branch rules may refuse any. The maintainer's own push may still pass, but + // `quest land apply` rebuilds only on the head the fixer worked from. try { - const ready = resolution(root, entry.head, job.plan.base.sha, head); + const ready = + standing === entry.head + ? resolution(root, entry.head, job.plan.base.sha, head) + : undefined; if (ready) (job.resolutions ??= {})[number] = ready; } catch (error) { deps.warn(`quest land fixer: cannot hand back #${number}'s resolution: ${String(error)}`); @@ -805,7 +818,7 @@ async function collectJob( if (result?.action === "fixed") { // GitHub shows a push a moment later. Wait for our own head; only a third head is a real move. let latest = object(await (await github.request(pullPath)).json()); - for (let wait = 0; object(latest.head).sha === entry.head && wait < SYNC_PAUSES; wait++) { + for (let wait = 0; object(latest.head).sha === standing && wait < SYNC_PAUSES; wait++) { await github.pause(1000); latest = object(await (await github.request(pullPath)).json()); } diff --git a/src/cli/land-head.ts b/src/cli/land-head.ts index a3597e21..c2dec373 100644 --- a/src/cli/land-head.ts +++ b/src/cli/land-head.ts @@ -68,7 +68,7 @@ export function forgetHeads(state: string, number: number): void { if (name.startsWith(`head-${number}-`) && name.endsWith(".json")) unlinkSync(join(state, name)); } -function isPrepared(root: string, state: string, entry: Entry): boolean { +export function isPrepared(root: string, state: string, entry: Entry): boolean { const path = recordPath(state, entry.number, entry.head); if (!existsSync(path)) return false; const record = JSON.parse(readFileSync(path, "utf8")) as Prepared; @@ -94,7 +94,10 @@ export async function push( /** What `ref` must be before the push; empty when it must not exist yet. */ lease = from, ) { - const workflows = git(root, ["diff", "--name-only", from, to, "--", ".github/workflows"]) !== ""; + // A push that replaces another head than the one it builds on can change workflows relative to either. + const workflows = [...new Set([from, lease])].some( + (old) => old !== "" && git(root, ["diff", "--name-only", old, to, "--", ".github/workflows"]) !== "", + ); const credential = await token(workflows); return tryGit(root, ["push", "--quiet", remote, `${to}:${ref}`, `--force-with-lease=${ref}:${lease}`], { env: gitAuthentication(credential), diff --git a/src/cli/land-serve.ts b/src/cli/land-serve.ts index 13471f5e..7053035e 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -341,9 +341,16 @@ async function reportStalled(github: GitHub, plan: Plan, state: string, now: num type MergeResult = { sha: string } | { reason: "head_moved" | "merge_conflict" | "merge_refused"; detail: string } | null; +/** + * How many times a merge is retried while GitHub catches up on a push, 15.5 s of backoff in all. A + * merge still unsettled after them throws, and the next round rebuilds the queue as GitHub then shows it. + */ +const SETTLE_RETRIES = 5; + /** * Retry only GitHub's pending mergeability or base refresh. Any other refusal, such as squash being * disabled or a rule the bot cannot bypass, would refuse every round after a full check, so it ejects. + * `previous` is the head before the gate pushed its own base merge as `head`, if it did. */ async function mergePinned(github: GitHub, path: string, head: string, previous = head): Promise { for (let attempt = 0; ; attempt++) { @@ -365,14 +372,20 @@ async function mergePinned(github: GitHub, path: string, head: string, previous // for it: a 409, or a 405 when the pin conflicts with the base that push merged. Neither is a writer. const current = object(pull.head).sha; if (previous !== head && (current === previous || (response.status === 409 && current === head))) { - if (attempt >= 3) throw new Error(`${detail}; prepared head has not propagated`); + if (attempt >= SETTLE_RETRIES) throw new Error(`${detail}; prepared head has not propagated`); await github.pause(500 * 2 ** attempt); continue; } if (current !== head) return { reason: "head_moved", detail }; - if (pull.mergeable === false || pull.mergeable_state === "dirty") return { reason: "merge_conflict", detail }; - if (pull.mergeable === null || /base branch was modified/i.test(String(result.message))) { - if (attempt >= 3) throw new Error(`${detail}; mergeability did not settle`); + const conflict = pull.mergeable === false || pull.mergeable_state === "dirty"; + // Once GitHub shows the gate's own merge, it can still call it conflicting from the pin's stale + // mergeability, in its refusal or in the PR (moq-dev/quest#342). That merge already holds the + // base it was checked on, so it cannot conflict with it: wait. Only a base that moved since can + // conflict, and then the retries run out and the next round rebuilds on that base. + const stale = previous !== head && (conflict || /merge conflict/i.test(String(result.message))); + if (conflict && !stale) return { reason: "merge_conflict", detail }; + if (stale || pull.mergeable === null || /base branch was modified/i.test(String(result.message))) { + if (attempt >= SETTLE_RETRIES) throw new Error(`${detail}; mergeability did not settle`); await github.pause(500 * 2 ** attempt); continue; } diff --git a/test/land.test.ts b/test/land.test.ts index 1161fc0b..baf585d1 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -819,6 +819,73 @@ describe.skipIf(process.platform === "win32")("quest land", () => { expect(forge.comment(1)).toBeUndefined(); }); + // moq-dev/quest#342: GitHub already showed the gate's base merge as the head, but still called it + // conflicting from the pin's mergeability, and the gate parked it for the fixer as a merge_conflict. + test.each(["false", "dirty"])( + "a pushed head's stale conflict report (mergeable %s) is retried rather than parked", + async (mode) => { + const forge = new Forge(); + const pull = forge.pull("Old", { "old.txt": "yes" }); + const pin = pull.head; + forge.advance({ "new.txt": "yes" }); + const deps = await forge.deps(); + const plan = await resolveQueue(REPOSITORY, undefined, deps); + pull.mergeRefusals = [{ status: 405, message: "Pull Request has merge conflicts" }]; + let attempts = 0; + forge.beforeMerge = () => { + attempts++; + // GitHub settles by the second attempt. + pull.mergeable = attempts === 1 && mode === "false" ? false : undefined; + pull.mergeableState = attempts === 1 && mode === "dirty" ? "dirty" : undefined; + }; + const pauses: number[] = []; + deps.sleep = async (delay) => { + pauses.push(delay); + }; + expect(await serveBatch(forge.clone(), plan, sandbox(), deps, async () => deps.token)).toEqual([ + `landed 1 ${pull.squash}`, + ]); + // Both attempts merge the gate's merge, which GitHub already showed, never the pin. + expect(forge.merges.map(({ body }) => body.sha)).toEqual([pull.head, pull.head]); + expect(pull.head).not.toBe(pin); + expect(pauses).toEqual([500]); + expect(forge.comment(1)).toBeUndefined(); + }, + ); + + test("a pushed head GitHub keeps calling conflicting is left queued for the next round, which lands it", async () => { + const forge = new Forge(); + const pull = forge.pull("Old", { "old.txt": "yes" }); + forge.advance({ "new.txt": "yes" }); + const deps = await forge.deps(); + const plan = await resolveQueue(REPOSITORY, undefined, deps); + pull.mergeRefusals = Array.from({ length: 6 }, () => ({ + status: 405 as const, + message: "Pull Request has merge conflicts", + })); + pull.mergeable = false; + const pauses: number[] = []; + deps.sleep = async (delay) => { + pauses.push(delay); + }; + const root = forge.clone(); + const options = sandbox(); + await expect(serveBatch(root, plan, options, deps, async () => deps.token)).rejects.toThrow( + "mergeability did not settle", + ); + expect(pauses).toEqual([500, 1000, 2000, 4000, 8000]); + expect(forge.comment(1)).toBeUndefined(); + expect(pull.landing).toBe(true); + expect(pull.labels).toContain("quest:merge"); + // Once GitHub settles, the next round accepts the gate's recorded merge and lands it. + pull.mergeable = undefined; + const next = await resolvePreparedQueue(root, options.state, REPOSITORY, undefined, deps); + expect(next.queue[0].head).toBe(pull.head); + expect(await serveBatch(root, next, options, deps, async () => deps.token)).toEqual([ + `landed 1 ${pull.squash}`, + ]); + }); + test("restart rejects a writer's commit after a recorded merge", async () => { const forge = new Forge(); forge.pull("Old", { "old.txt": "yes" }); @@ -1603,16 +1670,29 @@ mv '${result}.new' '${result}' ); const launched = eventFile(options.state, (name) => name === "fixer-result"); try { - const output = await serveBatch(forge.clone(), plan, options, deps, async () => deps.token); - expect(forge.merges.map(({ number }) => number)).toEqual([1, 2]); + const root = forge.clone(); + const run = serveBatch(root, plan, options, deps, async () => deps.token); + let output: string[] = []; + if (reason === "merge_conflict") { + // The gate's own merge of PR 1's squash into PR 2 holds that base, so GitHub calling it + // conflicting right after the push is not taken as a conflict: the next round rebuilds it. + await expect(run).rejects.toThrow("mergeability did not settle"); + expect(forge.comment(2)).toBeUndefined(); + } else output = await run; + expect([...new Set(forge.merges.map(({ number }) => number))]).toEqual([1, 2]); expect(forge.repo.pulls[0].merged).toBe(true); // PR 3 passed the check with it: it stays queued, without a comment, to be rebuilt on the new base. expect(forge.repo.pulls[2].landing).toBe(true); expect(forge.comment(3)).toBeUndefined(); expect(forge.repo.pulls[3].landing).toBe(true); if (reason === "merge_conflict") { - // A conflict parks the PR for the fixer, keeping its auto-merge. - expect(output).toEqual([`landed 1 ${forge.repo.pulls[0].squash}`, "parked 2"]); + // A round on PR 2 alone merges that recorded head as it stands, with no push, and GitHub + // still calls it conflicting: a conflict parks the PR for the fixer, keeping its auto-merge. + forge.settle(); + const next = await resolvePreparedQueue(root, options.state, REPOSITORY, undefined, deps); + const alone = { ...next, queue: next.queue.filter((entry) => entry.number === 2) }; + expect(alone.queue[0].head).toBe(forge.repo.pulls[1].head); + expect(await serveBatch(root, alone, options, deps, async () => deps.token)).toEqual(["parked 2"]); await launched; expect(readFileSync(result, "utf8").trim().split("\n").slice(1)).toEqual(["2"]); expect(forge.repo.pulls[1].landing).toBe(true); @@ -1671,7 +1751,7 @@ mv '${result}.new' '${result}' const plan = await resolveQueue(REPOSITORY, undefined, deps); const message = mode === "base modified" ? "Base branch was modified. Review and try the merge again" : mode; - pull.mergeRefusals = Array.from({ length: 4 }, () => ({ status: 405 as const, message })); + pull.mergeRefusals = Array.from({ length: 6 }, () => ({ status: 405 as const, message })); forge.beforeMerge = () => { pull.mergeable = mode === "false" ? false : mode === "pending" ? null : true; pull.mergeableState = mode === "dirty" ? "dirty" : mode === "required check" ? "blocked" : "clean"; @@ -1706,7 +1786,7 @@ mv '${result}.new' '${result}' expect(forge.comment(1)).toBeUndefined(); expect(pull.labels).toContain("quest:merge"); } - if (mode === "pending" || mode === "base modified") expect(pauses).toEqual([500, 1000, 2000]); + if (mode === "pending" || mode === "base modified") expect(pauses).toEqual([500, 1000, 2000, 4000, 8000]); }, ); @@ -2567,6 +2647,99 @@ printf '{"action":"fixed","head":"%s","reason":"edit the flake"}' "$(git rev-par expect(await fixture.collect(deps)).toEqual(["requeued 1"]); expect(fixture.pull.body).toContain(`(head ${fixture.pull.head})`); }); + + /** + * moq-dev/quest#342: once another PR landed, the gate pushed and recorded its own merge of the new + * base into PR 1's pin, then failed to land it and parked the PR, so its fixer job holds the pin. + */ + async function gateMerge(fixture: Awaited>): Promise { + fixture.forge.advance({ "landed.txt": "another PR\n" }); + const deps = await fixture.forge.deps(); + const plan = await resolveQueue(REPOSITORY, undefined, deps); + const link = (await build(fixture.root, plan)).links[0]; + const prepared = await prepareHead( + fixture.root, + fixture.state.state, + plan, + plan.queue[0], + plan.base.sha, + link.tree, + async () => deps.token, + ); + if (!("head" in prepared)) throw new Error(`no gate merge: ${JSON.stringify(prepared)}`); + expect(prepared.head).not.toBe(fixture.plan.queue[0].head); + return prepared.head; + } + test("requeues a PR at the gate's own merge pushed while it was parked, pinned there", async () => { + const fixture = await fixerFixture(requeueScript); + await fixture.finished; + const merged = await gateMerge(fixture); + expect(await fixture.collect()).toEqual(["requeued 1"]); + expect(fixture.pull.head).toBe(merged); + expect(fixture.pull.autoMerge?.commitBody).toBe(pin(merged)); + expect(fixture.pull.labels).toContain("quest:merge"); + expect(fixture.forge.comment(1)).toBeUndefined(); + const next = await resolveQueue(REPOSITORY, undefined, fixture.deps); + expect(next.eject).toEqual([]); + expect(next.queue.map(({ head, approved }) => [head, approved])).toEqual([[merged, merged]]); + }); + test("replaces the gate's own merge with a fix built on the pin, and pins the fix", async () => { + const fixture = await fixerFixture(fixedScript()); + await fixture.finished; + const merged = await gateMerge(fixture); + expect(await fixture.collect()).toEqual(["requeued 1"]); + const fixed = fixture.pull.head!; + expect(fixed).not.toBe(merged); + expect(git(fixture.root, "rev-parse", `${fixed}^`)).toBe(fixture.plan.queue[0].head); + expect(fixture.pull.autoMerge?.commitBody).toBe(pin(fixed)); + expect(fixture.pull.body).toContain(`(head ${fixed})`); + expect(fixture.pull.labels).toContain("quest:merge"); + }); + test("hands back with the fixer's diagnosis at the gate's own merge, not as a moved head", async () => { + const fixture = await fixerFixture( + `printf '[{"number":1,"action":"handback","diagnosis":"behavior change required","recommendation":"run quest-iterate"}]' > quest-land-result.json`, + ); + await fixture.finished; + const merged = await gateMerge(fixture); + expect(await fixture.collect()).toEqual(["handback 1"]); + expect(fixture.pull.head).toBe(merged); + expect(fixture.pull.landing).toBe(false); + expect(fixture.forge.comment(1)).toContain("behavior change required"); + expect(fixture.forge.comment(1)).not.toContain("head moved"); + }); + test.each(["forged", "writer"])( + "hands back a %s commit at the gate merge's place, keeping the fix unpushed", + async (mode) => { + const fixture = await fixerFixture(fixedScript()); + await fixture.finished; + const merged = await gateMerge(fixture); + // A copy of the gate's merge in everything but its timestamp, or a writer's commit on it. + const [tree, parents] = git(fixture.forge.origin, "show", "-s", "--format=%T|%P", merged).split("|"); + const env = { + ...process.env, + GIT_AUTHOR_NAME: "moq-bot", + GIT_AUTHOR_EMAIL: "moq-bot@users.noreply.github.com", + GIT_COMMITTER_NAME: "moq-bot", + GIT_COMMITTER_EMAIL: "moq-bot@users.noreply.github.com", + GIT_AUTHOR_DATE: "2001-01-01T00:00:00Z", + GIT_COMMITTER_DATE: "2001-01-01T00:00:00Z", + }; + const moved = execFileSync( + "git", + [ + "commit-tree", + tree, + ...(mode === "forged" ? parents.split(" ") : [merged]).flatMap((parent) => ["-p", parent]), + ], + { cwd: fixture.forge.origin, encoding: "utf8", input: "quest land: merge tested base\n", env }, + ).trim(); + git(fixture.forge.origin, "update-ref", "refs/heads/topic-1", moved); + expect(await fixture.collect()).toEqual(["handback 1"]); + expect(fixture.pull.head).toBe(moved); + expect(fixture.pull.landing).toBe(false); + expect(fixture.forge.comment(1)).toContain("head moved"); + }, + ); test.each([ [ "handback",