Skip to content

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

Merged
kixelated merged 5 commits into
mainfrom
quest/m1/fixer-isolate-failure
Nov 3, 2026
Merged

kixelated merged 5 commits into
mainfrom
quest/m1/fixer-isolate-failure

Conversation

@kixelated

@kixelated kixelated commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Implements quest/m1/fixer-isolate-failure.md and deletes it.

Problem

From moq-dev/moq.pro's land gate on 2026-11-02: when collecting a gate fixer job threw for one PR, collectFixers wrote the error to the job-wide job.error. It retried right away and then every minute. The PRs behind the failing one never moved, and once the stuck PR cleared, the job-wide error would have handed all of them back with that one PR's reason. moq-dev/quest#392 removed the cause seen that day (a merged PR), but any other persistent GitHub error on one PR could still wedge its job.

Change

  • Gate, collectFixers (src/cli/land-fixer.ts): a collection error is recorded in job.handbacks[<pr>] for the PR being collected, which is job.numbers[0]. Only that PR is handed back with it, and the job's other PRs keep their own results. job.error is used only when no PR is left. A reason already recorded, such as a crash's job.error or an earlier handback for that PR, still wins. This reuses the tested approach from the closed moq-dev/quest#393.
  • Bounded attempts: the job keeps failures[<pr>], the number of passes that failed to collect each PR. A pass counts once, however many retries it made. After COLLECTION_ATTEMPTS (3) failed passes, the PR is set aside: it is shifted off job.numbers, set aside <pr> is logged with the error, and collection goes on with the PRs behind it in the same pass. The base check runs only on a pass's first try, as before.
  • Watcher, collectAll (src/cli/land-watch-fix.ts): a watcher job holds one PR, so its error was already per-PR and did not block other jobs' collection. However, a job that kept failing to collect still counted against --fixer-jobs. With one slot, that blocked every other dropped PR from getting a fixer. The watcher now applies the same bound. After 3 failed passes (counted only by the pass's full collection), the job is finished with set aside <pr> and a warning, which frees its slot.
  • README: documents both behaviors.

Choices made without asking

  • What "set aside" does on the gate. The quest allowed "hand it back (or park it)". A handback is the GitHub call that keeps failing, so a set-aside PR is only released from the job. It stops being parked and rejoins batches as GitHub still queues it, with no handback comment. Its batch check then guards whether it lands. The alternative was to keep holding it locally, out of batches until a new maintainer pick. I rejected that because it would silently stall PRs after an outage of more than ~3 minutes: errors that reach collection have already outlasted the client's own 5xx and rate-limit retries. The cost of releasing is that a PR blamed for the failure may fail a batch again and get a new fixer run.
  • The bound is 3 passes (~3 minutes at the daemon's one-minute collection cadence). A pass's immediate retry does not count twice.
  • The first failure still becomes that PR's handback reason, as the job-wide error did before: one error that survives the client's retries hands the PR back rather than waiting for another pass.
  • The watcher path is covered too (the quest asked whether it was), and it gets the same bound. A watcher set-aside drops the round without a handback comment. The PR is already out of GitHub's merge queue.

Validation

  • test/land.test.ts
    • hands back only the PR whose collection GitHub failed, and keeps the rest of its job's results, from fix(land): the fixer drops a PR merged or closed while parked instead of looping on it #393: one failed read of PR 1 now gives ["handback 1", "requeued 2"]. On main, PR 2 would be handed back with PR 1's error.
    • sets aside a PR whose collection keeps failing, so the rest of its job goes on: every read of PR 1 fails. Passes 1 and 2 return [] with both PRs still parked. Pass 3 returns ["set aside 1", "requeued 2"]. The job is gone, nothing is parked, PR 1 keeps its auto-merge and gets no comment, and PR 2 is requeued at its pin.
  • test/land-watch.test.ts: a round whose handback GitHub keeps refusing is set aside, and frees its fixer slot. With one slot, every comment on PR 5 is refused. PR 6 waits for two passes, then gets ["set aside 5", "fixing 6"] and is requeued after its fixer runs.
  • just check and just test pass locally on head 02789844 (CLI: 993 passed, 2 skipped; Worker: 490 passed).

Review

Codex gpt-6.1-sol at medium effort (quest review --base origin/main --branch quest/m1/fixer-isolate-failure), round 2 of the session that reviewed 3ce01704, reviewed head 02789844188d83e012c8c94494d3ef6c127f4149 against origin/main f1cfe6c8: no findings.

That head merges origin/main at 574cf6e7 (moq-dev/quest#421, moq-dev/quest#433, and others) into the earlier head 5c34b010. One conflict was resolved by hand in quest/m1/README.md's Required list: this PR drops the link to its own quest, and main dropped the link to /quest/m1/wait-merge-heads.md, so both lines go. Main changed none of the PR's code files, and the PR's changed lines are unchanged. The diff against the base differs from 5c34b010's only in hunk offsets and context in README.md and quest/m1/README.md. A hand-resolved merge is outside the guide's exemptions, so the head was reviewed again.

Follow-ups

  • None needed for this quest. Sibling PRs touching the same functions (land-fixer/protected-commit moq-dev/quest#409, queue-watcher/flaky moq-dev/quest#406, and several others editing land-watch-fix.ts) may conflict in collectFixers/collectAll. Resolve those conflicts by merging, not rebasing.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

kixelated and others added 2 commits November 2, 2026 19:43
A GitHub error collecting a gate fixer job is recorded on the PR it hit,
not job-wide, so the job's other PRs keep their own results. A PR whose
collection fails three passes in a row is set aside: it leaves the job,
so the PRs behind it go on. The watcher sets aside a round that keeps
failing the same way, freeing its fixer slot.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title One PR's collection failure can't stall its fixer job fix(land): one PR's collection failure can't stall its fixer job Nov 3, 2026
@kixelated
kixelated marked this pull request as ready for review November 3, 2026 03:53
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f57a2876-0a34-49af-906d-c3a7b8693bb7
Commits

Reviewing files that changed from the base of the PR and between 574cf6e and 0278984.

Files selected for processing: 7
  • README.md
  • quest/m1/README.md
  • quest/m1/fixer-isolate-failure.md
  • src/cli/land-fixer.ts
  • src/cli/land-watch-fix.ts
  • test/land-watch.test.ts
  • test/land.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@kixelated
kixelated added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit 4c728f1 Nov 3, 2026
17 checks passed
@kixelated
kixelated deleted the quest/m1/fixer-isolate-failure branch November 3, 2026 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant