diff --git a/README.md b/README.md index 4e7b97a2..757379fd 100644 --- a/README.md +++ b/README.md @@ -641,7 +641,11 @@ parking it is collected at that merge: a requeue pins it, and a fix, built on th 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 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 +status or signal, with the tail of the job's retained log. A GitHub error while +collecting one PR hands back that PR alone, and the job's other PRs keep their own +results. A PR whose collection fails three passes in a row, as when GitHub refuses its +handback, is set aside with a logged warning: it leaves the job without a handback +comment, so it rejoins batches as GitHub still queues it, and the PRs behind it go on. A validated fix whose push GitHub refuses, such as a merged base's `.github/workflows` changes that the App lacks the `workflows` permission to push, is handed back with its resolution. So is a fix that edits protected paths beyond what the base merge carries, such as a conflict in @@ -862,6 +866,8 @@ 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. +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. The watcher also keeps the repository's flaky tests in `flaky.json` in its state directory: each flaky verdict, across every PR, by the failure signature diff --git a/quest/m1/README.md b/quest/m1/README.md index f3c3fc6d..29c0442a 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -26,6 +26,5 @@ earlier "Later work" milestone became m2. - [Quest log theme](/quest/m1/theme/README.md) - an MMO-style identity, copy, board, landing page, and CLI output for the demo and launch - [Launch material](/quest/m1/launch.md) - README, quickstart, demo, comparison, and launch-post drafts - [Workers in a 5-minute cache](/quest/m1/worker-cache.md) - re-plan #276's worker/coordinator split and gate tickets, after confirming the cache premise -- [One PR's collection failure can't stall its fixer job](/quest/m1/fixer-isolate-failure.md) - errors stay on their PR, and a PR that keeps failing is set aside - [Retire Decisions needed](/quest/m1/decisions-needed.md) - only `## Waiting` hands a draft back, once no open draft uses the old heading - [Review harness check](/quest/m1/review-harness.md) - recurring each milestone: newest pinned review models, and the live sandbox test passes diff --git a/quest/m1/fixer-isolate-failure.md b/quest/m1/fixer-isolate-failure.md deleted file mode 100644 index 7b75767c..00000000 --- a/quest/m1/fixer-isolate-failure.md +++ /dev/null @@ -1,34 +0,0 @@ -# [S] One PR's collection failure can't stall its fixer job - -## Goal - -When collecting a land fixer job fails for one PR, the failure is recorded -on that PR alone, and a PR whose handback keeps failing is set aside after a -bounded number of attempts. The job's other PRs are still requeued or handed -back on their own merits. A test with one PR whose GitHub calls always fail -shows its siblings moving on. - -## Plan - -From moq-dev/moq.pro's land gate on 2026-11-02: a PR that merged while its -batch was parked wedged the fixer job for about 7 hours. Collection retried a -refused `disablePullRequestAutoMerge` every minute, the PRs behind it never -moved, and the job-wide `job.error` the retry wrote would have handed them back -with the wrong reason. [moq-dev/quest#392](https://github.com/moq-dev/quest/pull/392) -fixed that cause narrowly (a merged or closed PR is dropped). - -- In `collectFixers` (`src/cli/land-fixer.ts`), write a collection error to - `job.handbacks[]`, not `job.error`. Use the job-wide error only when no - PR is left. A reason already recorded, such as a crash, still wins. -- After N consecutive failed collections of the same PR, hand it back (or park - it) with the error and shift it off `job.numbers`, so a persistent GitHub - error can't block the rest. -- The closed duplicate [moq-dev/quest#393](https://github.com/moq-dev/quest/pull/393) - has a tested version of the first point; reuse it. -- The host gate is frozen except for fixes the queue-watcher switch needs, but - moq.pro still lands through it. Confirm the change also covers the watcher's - fixer path, or record that it only applies to the gate. - -## Related - -- [The land fixer resolves every mechanical failure itself](/quest/m0/land-fixer/README.md) - owns what the fixer does with a PR diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index 44f45430..f0d3c455 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -140,6 +140,8 @@ interface Job extends Process { base?: boolean; results?: FixResult[]; handbacks?: Record; + /** Passes that failed to collect each PR, so one that keeps failing is set aside rather than holding up the rest. */ + failures?: Record; /** PRs a handback is taking out of the queue, recorded before their auto-merge is disabled. */ left?: number[]; /** Validated fixes GitHub refused to publish, handed back for a maintainer to push. */ @@ -847,27 +849,73 @@ export async function collectFixers( continue; } - try { - output.push(...(await collectJob(root, state, path, job, deps, token, timeout, read, checkBase))); - } catch (error) { - // The gate's own base check failing to run is not the fixer's failure: retry it later. - if (error instanceof BaseCheckError) { - deps.warn(`quest land fixer: base check pending for ${name}: ${String(error.cause)}`); - continue; - } - // Keep the first reason: a GitHub error while handing back a crash must not replace the crash. - job.error ??= `fixer collection failed: ${String(error)}; run /quest-iterate`; + // The first try checks the base if it must; a retry only hands back, so it never checks it. + const failed = new Set(); + for (let retry = false; ; retry = true) { try { + output.push( + ...(await collectJob( + root, + state, + path, + job, + deps, + token, + timeout, + read, + retry ? undefined : checkBase, + )), + ); + break; + } catch (error) { + // The gate's own base check failing to run is not the fixer's failure: retry it later. + if (error instanceof BaseCheckError) { + deps.warn(`quest land fixer: base check pending for ${name}: ${String(error.cause)}`); + break; + } + const aside = failCollection(job, error, failed); save(path, job); - output.push(...(await collectJob(root, state, path, job, deps, token, timeout, read))); - } catch (failure) { - deps.warn(`quest land fixer: handback pending for ${name}: ${String(failure)}`); + if (aside !== undefined) { + deps.warn( + `quest land fixer: set aside ${aside} from ${name} after ${COLLECTION_ATTEMPTS} failed collections: ${String(error)}`, + ); + output.push(`set aside ${aside}`); + } else if (retry) { + deps.warn(`quest land fixer: handback pending for ${name}: ${String(error)}`); + break; + } } } } return output; } +/** Passes in a row that fail to collect the same PR before it is set aside, so its job's other PRs go on. */ +export const COLLECTION_ATTEMPTS = 3; + +/** + * Record a failure collecting `job` against the PR it was working on, its first PR left: that PR alone + * is handed back with it, and its job's other PRs keep their own results. A reason already recorded, + * such as a crash's, still wins. A PR that keeps failing, as when GitHub refuses its handback, is set + * aside: it leaves the job, so it rejoins batches as GitHub still queues it, and its number is returned. + */ +function failCollection(job: Job, error: unknown, failed: Set): number | undefined { + const reason = `fixer collection failed: ${String(error)}; run /quest-iterate`; + const number = job.numbers[0]; + if (number === undefined) { + job.error ??= reason; + return undefined; + } + if (!job.error) (job.handbacks ??= {})[number] ??= reason; + // A pass's retry of the same PR is not another failed collection. + const failures = (job.failures ??= {}); + if (!failed.has(number)) failures[number] = (failures[number] ?? 0) + 1; + failed.add(number); + if (failures[number] < COLLECTION_ATTEMPTS) return undefined; + job.numbers.shift(); + return number; +} + /** An exit status as a cause: a signal by name (143 is SIGTERM), or a crash with its code (101 is a Rust panic). */ function exited(code: string): string { const signal = Object.entries(osConstants.signals).find(([, number]) => 128 + number === Number(code))?.[0]; diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 80e1d7b6..5942bfb9 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -11,6 +11,7 @@ import { EFFORT_VARIABLE, type FixResult, parseFixResult, readFixResult } from " import { author, cited, + COLLECTION_ATTEMPTS, comment, conflictFailure, FixerRefusal, @@ -94,6 +95,8 @@ interface WatchJob extends Process { handback?: string; /** A validated fix whose push GitHub refused, handed back for `quest land apply`. */ resolution?: Resolution; + /** Passes that failed to collect it, so one that keeps failing is set aside rather than holding a fixer slot. */ + failures?: number; } const JOB = /^watch-job-.*\.json$/; @@ -212,9 +215,21 @@ async function collectAll( try { output.push(...(await collect(github, state, path, job, context, deps))); } catch (error) { + // A round that keeps failing, as when GitHub refuses its handback, is set aside, so it frees its + // fixer slot for the other dropped PRs. Only the pass's full collection counts, once. + if (!merges) job.failures = (job.failures ?? 0) + 1; + if ((job.failures ?? 0) >= COLLECTION_ATTEMPTS) { + deps.warn( + `quest land watch: set aside ${job.entry.number} from ${name} after ${COLLECTION_ATTEMPTS} failed collections: ${String(error)}`, + ); + finish(path, job); + output.push(`set aside ${job.entry.number}`); + continue; + } // Once a round has asked GitHub to take its head back, it may already be in the queue: it never // becomes a handback on an error, and the next pass retries it, which re-enqueues only if needed. if (job.requeuing !== undefined) { + save(path, job); deps.warn(`quest land watch: fixer round ${name} pending: ${String(error)}`); continue; } diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index ecf61439..f433034c 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -11,7 +11,7 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; import type { GitHubDependencies } from "../src/cli/github"; import { readRemote } from "../src/cli/land"; import { parseResolution } from "../src/cli/land-apply"; -import { FLAKY_LIMIT, history } from "../src/cli/land-fixer"; +import { COLLECTION_ATTEMPTS, FLAKY_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"; @@ -1446,6 +1446,69 @@ git commit -qm 'disable render_timeout'`; expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); }); + test("a round whose handback GitHub keeps refusing is set aside, and frees its fixer slot", async () => { + const { repo, state, fix, context } = setup(1); + const base = repo.main(); + const removed = (number: number, minute: number): Pull => { + const head = repo.pull(number, { [`feature-${number}.txt`]: "new\n" }); + return { + number, + head, + enqueues: [{ createdAt: at(1), actor: "maintainer" }], + removals: [ + { + id: `R${number}`, + createdAt: at(minute), + reason: "failed_checks", + actor: "github-merge-queue", + before: head, + }, + ], + }; + }; + const github = { + ...world( + [removed(5, 4), removed(6, 5)], + [failedRun(5, at(3), base), { ...failedRun(6, at(3), base), id: 8 }], + ), + origin: repo.origin, + }; + github.now = Date.parse(at(6)); + fix.round( + 5, + `printf '[{"number":%s,"action":"handback","diagnosis":"the test asserts the old behavior","recommendation":"decide which behavior is right"}]' "$2" > quest-land-result.json\n`, + ); + fix.round(6, flakyScript); + // One fixer slot: 6 waits for 5's. + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + "removed 5 failed_checks fix", + "removed 6 failed_checks fix", + "fixing 5", + ]); + await exited(state); + // GitHub refuses every comment on 5, so its handback never posts. + const refusing = () => { + const dependencies = deps(github); + const original = dependencies.fetch; + dependencies.fetch = (async (input, init) => { + if (init?.method === "POST" && new URL(String(input)).pathname.endsWith("/issues/5/comments")) + return Response.json({ message: "refused" }, { status: 422 }); + return original(input, init); + }) as typeof fetch; + return dependencies; + }; + for (let pass = 1; pass < COLLECTION_ATTEMPTS; pass++) + expect(await watchPass(REPOSITORY, { state }, refusing(), context)).toEqual([]); + expect(fix.calls()).toHaveLength(1); + expect(await watchPass(REPOSITORY, { state }, refusing(), context)).toEqual(["set aside 5", "fixing 6"]); + expect( + readdirSync(state).filter((name) => name.startsWith("watch-job-") && name.endsWith(".json")), + ).toHaveLength(1); + await exited(state); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + `requeued 6 ${github.pulls[1].head}`, + ]); + }); test("a person's removal ends the pick, even of a round re-enqueued just before a restart", async () => { const { repo, state, fix, context } = setup(); const base = repo.main(); diff --git a/test/land.test.ts b/test/land.test.ts index 6ef0724b..61f4e3c0 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -25,6 +25,7 @@ import { ACTIONS_APP, type Pull, type World, fakeGitHub } from "../cloudflare/de import { authenticatedRead, gitAuthentication } from "../src/cli/land-git"; import { ADMITTED, + COLLECTION_ATTEMPTS, collectFixers, validateFix, fixerMarkdown, @@ -2995,6 +2996,50 @@ fi`); expect(fixture.forge.comment(1)).toContain("head moved while fixing"); expect(fixture.jobs()).toEqual([]); }); + const blameFirst = `printf '[{"number":1,"action":"handback","diagnosis":"breaks the check","recommendation":"run quest-iterate","blame":true},{"number":2,"action":"requeue","reason":"collateral: shared the batch with 1"}]' > quest-land-result.json`; + /** `deps` with GitHub failing `fails` reads of PR 1, every one when unset. */ + const failingPull = (deps: GitHubDependencies, fails = Number.POSITIVE_INFINITY): GitHubDependencies => { + const original = deps.fetch; + return { + ...deps, + fetch: (async (input, init) => { + if (String(input).endsWith("/pulls/1") && (init?.method ?? "GET") === "GET" && fails-- > 0) + return Response.json({ message: "Not Found" }, { status: 404 }); + return original(input, init); + }) as typeof fetch, + }; + }; + test("hands back only the PR whose collection GitHub failed, and keeps the rest of its job's results", async () => { + const fixture = await fixerFixture(blameFirst, { collateral: true }); + await fixture.finished; + expect(await fixture.collect(failingPull(fixture.deps, 1))).toEqual(["handback 1", "requeued 2"]); + expect(fixture.jobs()).toEqual([]); + expect(fixture.pull.landing).toBe(false); + expect(fixture.forge.comment(1)).toContain("fixer collection failed"); + const collateral = fixture.collateral!; + expect(collateral.landing).toBe(true); + expect(fixture.forge.comment(2)).toBeUndefined(); + }); + test("sets aside a PR whose collection keeps failing, so the rest of its job goes on", async () => { + const fixture = await fixerFixture(blameFirst, { collateral: true }); + await fixture.finished; + const deps = failingPull(fixture.deps); + // Each pass retries it once; neither the retry nor the PR behind it counts as another failure. + for (let pass = 1; pass < COLLECTION_ATTEMPTS; pass++) { + expect(await fixture.collect(deps)).toEqual([]); + expect([...parked(fixture.state.state).keys()]).toEqual([1, 2]); + } + expect(await fixture.collect(deps)).toEqual(["set aside 1", "requeued 2"]); + expect(fixture.jobs()).toEqual([]); + // It was never handed back: it stays queued as GitHub has it, and rejoins batches. + expect(parked(fixture.state.state).size).toBe(0); + expect(fixture.pull.landing).toBe(true); + expect(fixture.forge.comment(1)).toBeUndefined(); + const collateral = fixture.collateral!; + expect(collateral.landing).toBe(true); + expect(collateral.autoMerge?.commitBody).toBe(pin(collateral.head!)); + expect(fixture.forge.comment(2)).toBeUndefined(); + }); const editJob = (state: string, change: (job: Record) => void) => { const path = join( state,