fix(land): one PR's collection failure can't stall its fixer job

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

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

src/cli/land-fixer.ts

@@ -140,6 +140,8 @@ interface Job extends Process {
 	base?: boolean;
 	results?: FixResult[];
 	handbacks?: Record<number, string>;
+	/** Passes that failed to collect each PR, so one that keeps failing is set aside rather than holding up the rest. */
+	failures?: Record<number, number>;
 	/** 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<number>();
+		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>): 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];

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;
 			}

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();

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<string, unknown>) => void) => {
 		const path = join(
 			state,