Public: anyone can read this; the crew can change it.
Batch doctor: a red merge batch with no single culprit #11293
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/11262-batch-doctor-a-red-merge-batch-with-no-s → main 3 commits fork
5 files
changed +60
−3
docs/features/batch-doctor.md +2
| @@ -6,3 +6,5 @@ When the merge queue's re-gate hands a pull back red (the re-gate carries it alo | ||
| 6 | 6 | - **budget / infra**: recorded as `recheck`, not held. the queue re-gates a red pull alone later (#3155). |
| 7 | 7 | |
| 8 | 8 | Every act is a `maintenance_run` row (`duty = batch-doctor`): batch (`base:head`), culprit pull, class, action, probe (the first error line); the newest 200 per repository are kept (`RUNS_KEPT`). A sealed pull's number, batch and probe are told by no door. Read them on the flow panel (Batch doctor card), in the terminal Flow dialog, or as `doctor` in MCP `queue_gates` (all `doctorRuns`). A doctor's hold reason begins `batch doctor:` (`HOLD_PREFIX`, `isDoctorHold`). The infra class matches errno codes, `no space left`, `disk quota` and `tar:` lines only. |
| 9 | ||
| 10 | **No single culprit (#11262)**: when the queue's search narrows a red batch to one pull that is red only on top of pulls of its candidate that landed first, the doctor classes the red and records a `recheck` run whose batch is `base:head+#a,#b` (the members it was red with). These rows are one per (pull, head, members) and share the 200 `RUNS_KEPT` cap with the rest, so a flapping batch adds one and no more, but many pulls can still fill it. This records; it does not re-gate: the singly re-gate already exists (a red neither side moved off is gated again alone after `REGATE_RED_MS`, #3155) and its reproduced red is what is held. Nothing is held here: the re-gate that carries the pull alone decides, and a functional red that reproduces there is held with the exact failing test as above. |
src/checks/doctor.ts +21
| @@ -115,6 +115,27 @@ function record(app: App, repo: Repo, pull: Pull, batch: string, d: Diagnosis, a | ||
| 115 | 115 | .run(DUTY, repo.id, RUNS_KEPT); |
| 116 | 116 | } |
| 117 | 117 | |
| 118 | /** | |
| 119 | * A red the queue's search narrowed to one pull but found on top of pulls of | |
| 120 | * its candidate that landed first (`together`): red alone or only with them, | |
| 121 | * so no culprit is proven and nothing is held. The class, the test and the | |
| 122 | * members are recorded, and the re-gate that carries the pull alone decides | |
| 123 | * the hold (`treatRed`). | |
| 124 | */ | |
| 125 | export function noteRed(app: App, repo: Repo, pull: Pull, red: Red, baseOid: string, together: readonly number[]): void { | |
| 126 | if (together.length === 0) return; | |
| 127 | const members = together.map((n) => `#${n}`).join(","); | |
| 128 | const batch = `${baseOid.slice(0, 12)}:${pull.headOid.slice(0, 12)}+${members}`; | |
| 129 | // A flapping batch is red on the same head with the same members again and | |
| 130 | // again: one row per (pull, head, members), so it cannot push the rows that | |
| 131 | // record real holds out of the `RUNS_KEPT` newest. | |
| 132 | const seen = app.db | |
| 133 | .query(`SELECT 1 FROM maintenance_run WHERE duty = ?1 AND repo_id = ?2 AND culprit = ?3 AND action = 'recheck' AND batch LIKE ?4 LIMIT 1`) | |
| 134 | .get(DUTY, repo.id, pull.number, `%:${pull.headOid.slice(0, 12)}+${members}`); | |
| 135 | if (seen) return; | |
| 136 | record(app, repo, pull, batch, cleaned(classify(red.reason, red.tests)), "recheck"); | |
| 137 | } | |
| 138 | ||
| 118 | 139 | /** Treat one red the queue's re-gate handed back, the pull alone on its merge. `by` acts. */ |
| 119 | 140 | export function treatRed(app: App, repo: Repo, by: User, pull: Pull, red: Red, baseOid: string): DoctorAction { |
| 120 | 141 | const batch = `${baseOid.slice(0, 12)}:${pull.headOid.slice(0, 12)}`; |
src/crew/autonomy.ts +3 −1
| @@ -12,7 +12,7 @@ | ||
| 12 | 12 | * repository's origin. |
| 13 | 13 | */ |
| 14 | 14 | |
| 15 | import { treatRed } from "../checks/doctor.ts"; | |
| 15 | import { noteRed, treatRed } from "../checks/doctor.ts"; | |
| 16 | 16 | import { tickBets } from "../actions/bets.ts"; |
| 17 | 17 | import { betsInUse } from "../core/bets.ts"; |
| 18 | 18 | import { onlyListDocs, resolveListConflicts } from "../actions/list-conflicts.ts"; |
| @@ -2199,6 +2199,8 @@ async function admit( | ||
| 2199 | 2199 | if (!again) addComment(app, by, repo, getIssueById(app.db, pull.id) ?? pull, clashSaid(red.comment, together)); |
| 2200 | 2200 | // A red that reproduces on the re-gate is the code's: only then is it the doctor's to hold. |
| 2201 | 2201 | if (again) treatRed(app, repo, by, pull, red, baseOid); |
| 2202 | // Red only on top of pulls that landed first: no single culprit yet, so the class is recorded and the re-gate alone decides. | |
| 2203 | else noteRed(app, repo, pull, red, baseOid, together); | |
| 2202 | 2204 | }, |
| 2203 | 2205 | }); |
| 2204 | 2206 | } |
test/doctor-culprit.test.ts +24 −1
| @@ -1,6 +1,6 @@ | ||
| 1 | 1 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; |
| 2 | 2 | import { deleteRepo } from "../src/actions/repos.ts"; |
| 3 | import { doctorRuns, HOLD_PREFIX, isDoctorHold, RUNS_KEPT, treatRed } from "../src/checks/doctor.ts"; | |
| 3 | import { doctorRuns, HOLD_PREFIX, isDoctorHold, noteRed, RUNS_KEPT, treatRed } from "../src/checks/doctor.ts"; | |
| 4 | 4 | import { getHold } from "../src/core/holds.ts"; |
| 5 | 5 | import { listComments } from "../src/core/issues.ts"; |
| 6 | 6 | import { doctorWorld, redFor, type Doctor } from "./helpers/doctor.ts"; |
| @@ -78,3 +78,26 @@ describe("the batch doctor and a red culprit", () => { | ||
| 78 | 78 | } |
| 79 | 79 | }); |
| 80 | 80 | }); |
| 81 | ||
| 82 | describe("the batch doctor and a red with no single culprit", () => { | |
| 83 | test("a red on top of landed pulls is classed and recorded with its members, and not held", async () => { | |
| 84 | const w = await doctorWorld("together"); | |
| 85 | try { | |
| 86 | const pull = await w.pull("mix.ts"); | |
| 87 | noteRed(w.app, w.repo, pull, redFor("mixes", "test/mix.test.ts", "error: boom"), "a".repeat(40), [7, 9]); | |
| 88 | expect(getHold(w.app.db, pull.id)).toBeUndefined(); | |
| 89 | const [run] = doctorRuns(w.app.db, w.repo.id); | |
| 90 | expect(run).toMatchObject({ culprit: pull.number, class: "functional", action: "recheck" }); | |
| 91 | expect(run!.batch).toEndWith("+#7,#9"); | |
| 92 | noteRed(w.app, w.repo, pull, redFor("mixes", "test/mix.test.ts", "error: boom"), "a".repeat(40), []); | |
| 93 | expect(doctorRuns(w.app.db, w.repo.id)).toHaveLength(1); | |
| 94 | // The same head and members again is one row; other members are another. | |
| 95 | noteRed(w.app, w.repo, pull, redFor("mixes", "test/mix.test.ts", "error: boom"), "b".repeat(40), [7, 9]); | |
| 96 | expect(doctorRuns(w.app.db, w.repo.id)).toHaveLength(1); | |
| 97 | noteRed(w.app, w.repo, pull, redFor("mixes", "test/mix.test.ts", "error: boom"), "b".repeat(40), [7]); | |
| 98 | expect(doctorRuns(w.app.db, w.repo.id)).toHaveLength(2); | |
| 99 | } finally { | |
| 100 | await w.close(); | |
| 101 | } | |
| 102 | }); | |
| 103 | }); |
test/merge-queue.test.ts +10 −1
| @@ -704,6 +704,7 @@ describe("the merge queue", () => { | ||
| 704 | 704 | expect(hold!.why).toContain("goes red"); |
| 705 | 705 | const [run] = doctorRuns(app.db, repo.id); |
| 706 | 706 | expect(run).toMatchObject({ culprit: pull.number, class: "functional", action: "held" }); |
| 707 | expect(doctorRuns(app.db, repo.id)).toHaveLength(1); | |
| 707 | 708 | expect(listComments(app.db, pull.id).at(-1)!.body).toContain("bun test ./test/red.test.ts"); |
| 708 | 709 | }); |
| 709 | 710 | |
| @@ -715,7 +716,15 @@ describe("the merge queue", () => { | ||
| 715 | 716 | const red = pulls[2]!; |
| 716 | 717 | expect(latestDecision(app.db, red.id, "land")?.answer).toBe("red"); |
| 717 | 718 | expect(getHold(app.db, red.id)).toBeUndefined(); |
| 718 | expect(doctorRuns(app.db, repo.id)).toEqual([]); | |
| 719 | // No single culprit yet: recorded as a recheck with its members (#11262), never held. | |
| 720 | const runs = doctorRuns(app.db, repo.id); | |
| 721 | expect(runs).toHaveLength(1); | |
| 722 | expect(runs[0]).toMatchObject({ culprit: red.number, action: "recheck" }); | |
| 723 | expect(runs[0]!.batch).toMatch(new RegExp(`^[0-9a-f]{12}:${red.headOid.slice(0, 12)}\\+#${pulls[0]!.number},#${pulls[1]!.number}$`)); | |
| 724 | // The re-gate alone is a first red on a new base with nothing landed ahead, or a reproduced one: neither adds a recheck row. | |
| 725 | age(red); | |
| 726 | await sweepGreen(app, repo); | |
| 727 | expect(doctorRuns(app.db, repo.id).filter((r) => r.action === "recheck")).toHaveLength(1); | |
| 719 | 728 | }); |
| 720 | 729 | |
| 721 | 730 | test("a red that reproduces on the re-gate stays red, told once", async () => { |
new above
offline