trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns14g rEventsFlows

The merge queue's "red alone" mark on a pull clears when main moves, so a green pull rejoins without a new head #6506

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/6493-the-merge-queue-s-red-alone-mark-on-a-pu → main 1 commit fork

  • Merged. scout/6493-the-merge-queue-s-red-alone-mark-on-a-pu was merged into main as 435d6c3.

Review

Sign in to review this change.
3 files changed +59 −19
src/checks/queue.ts +16 −5
@@ -179,6 +179,8 @@ interface Waiting {
179179 /** A pull the line handed back red, while its head is the same (#4235). */
180180 interface Remembered {
181181 readonly head: string;
182 /** The base tip it went red on: a moved base is a new try (#6493). */
183 readonly base: string;
182184 /** The pulls its red tree held besides it: a clash when there are any. */
183185 readonly together: readonly number[];
184186 }
@@ -329,19 +331,28 @@ function lineOf(repoId: string): Line {
329331 * pull landed. A pull already in the line is answered `false` at once: the
330332 * entry already there carries it.
331333 */
332 export function enqueue(
334 export async function enqueue(
333335 repoId: string,
334336 entries: readonly Queued[],
335337 gate: GateCandidate,
336338 options: QueueOptions = {},
337339 ): Promise<boolean[]> {
338340 const line = lineOf(repoId);
341 // A red mark names the base it went red on; the tip is read only when a
342 // mark is there to compare it with.
343 const tips = new Map<string, string | undefined>();
344 for (const { pull, dir } of entries) {
345 if (!line.reds.has(pull.id) || tips.has(pull.baseBranch + "\0" + dir)) continue;
346 tips.set(pull.baseBranch + "\0" + dir, await resolveOid(dir, `refs/heads/${pull.baseBranch}`));
347 }
339348 const answers = entries.map((entry) => {
340349 if (line.holds.has(entry.pull.id)) return Promise.resolve(false);
341350 line.holds.add(entry.pull.id);
342351 const red = line.reds.get(entry.pull.id);
343 if (red !== undefined && red.head !== entry.pull.headOid) line.reds.delete(entry.pull.id);
344 const recheck = red !== undefined && red.head === entry.pull.headOid;
352 const tip = tips.get(entry.pull.baseBranch + "\0" + entry.dir);
353 const held = red !== undefined && red.head === entry.pull.headOid && (tip === undefined || red.base === tip);
354 if (red !== undefined && !held) line.reds.delete(entry.pull.id);
355 const recheck = held;
345356 if (line.suspects.get(entry.pull.id)?.head !== entry.pull.headOid) line.suspects.delete(entry.pull.id);
346357 return new Promise<boolean>((resolve) =>
347358 line.waiting.push({
@@ -634,7 +645,7 @@ async function solo(line: Line, gated: Gated, w: Waiting): Promise<{ readonly do
634645 return;
635646 }
636647 line.suspects.delete(pull.id);
637 line.reds.set(pull.id, { head: pull.headOid, together: [] });
648 line.reds.set(pull.id, { head: pull.headOid, base: on.baseOid, together: [] });
638649 log.info("red alone", { repo: line.repoId, pull: pull.number });
639650 w.entry.handBack(verdict, on.baseOid, []);
640651 w.settle(false);
@@ -876,7 +887,7 @@ async function judged(
876887 // with them, or red on its own — not gated alone on the base before
877888 // them to tell which. Remembered with them either way (#4235).
878889 const together = landed.map((q) => q.pull.number);
879 search.line.reds.set(culprit.pull.id, { head: culprit.pull.headOid, together });
890 search.line.reds.set(culprit.pull.id, { head: culprit.pull.headOid, base: candidate.baseOid, together });
880891 search.line.suspects.delete(culprit.pull.id);
881892 log.info(together.length === 0 ? "red on its own merge" : "red on top of its candidate", {
882893 repo: search.line.repoId,
test/merge-queue-clash.test.ts +32 −2
@@ -72,6 +72,10 @@ describe("the merge queue remembers clashes", () => {
7272 }
7373
7474 const repoId = "clash-line";
75 /** B red on its own merge on main as it is now: the mark names this base (#6493). */
76 const markedNow = async () => {
77 expect(await enqueue(repoId, [entry(2, git("rev-parse", "b"))], gate)).toEqual([false]);
78 };
7579 const together = (a: number, b: number) => gates.filter((g) => g.pulls.includes(a) && g.pulls.includes(b)).length;
7680
7781 test("A and B clash, C is fine: A and C land, B is handed back once on top of A", async () => {
@@ -90,6 +94,7 @@ describe("the merge queue remembers clashes", () => {
9094 });
9195
9296 test("B joining again at the same head is re-checked alone, after the fresh pulls, and cuts nothing off", async () => {
97 await markedNow();
9398 const b = entry(2, git("rev-parse", "b"));
9499 const [d, e] = [entry(4, await headAdding("d", "d.txt")), entry(5, await headAdding("e", "e.txt"))];
95100 const before = gates.length;
@@ -104,11 +109,12 @@ describe("the merge queue remembers clashes", () => {
104109 // Alone on main as it is now, B is red on its own merge.
105110 expect(told.at(-1)).toEqual({ pull: 2, together: [] });
106111 expect(queueClashes(repoId)).toBe(0);
107 // A red re-check is no red batch.
108 expect(redBatchRate(repoId)).toEqual({ batches: 3, red: 1 });
112 // A red re-check is no red batch; the fresh try before it was one.
113 expect(redBatchRate(repoId)).toEqual({ batches: 4, red: 2 });
109114 });
110115
111116 test("a fresh pull joining behind a re-check is not stacked on it", async () => {
117 await markedNow();
112118 const b = entry(2, git("rev-parse", "b"));
113119 let release = (): void => {};
114120 const held = new Promise<void>((r) => (release = r));
@@ -135,6 +141,30 @@ describe("the merge queue remembers clashes", () => {
135141 ]);
136142 });
137143
144 test("red alone, then main moves: the pull gates again as a fresh entry", async () => {
145 await markedNow();
146 const h = entry(8, await headAdding("h", "h.txt"));
147 expect(await enqueue(repoId, [h], gate)).toEqual([true]); // main moves
148 const b = entry(2, git("rev-parse", "b"));
149 const before = gates.length;
150 // Fresh: it rides with the new pull in one candidate, not behind it as a re-check.
151 const i = entry(9, await headAdding("i", "i.txt"));
152 expect(await enqueue(repoId, [b, i], gate)).toEqual([false, true]);
153 expect(gates.slice(before)[0]).toEqual({ pulls: [2, 9], ok: false });
154 });
155
156 test("red alone, main unmoved: the mark still holds it for a re-check", async () => {
157 await markedNow();
158 const before = gates.length;
159 const j = entry(10, await headAdding("j", "j.txt"));
160 const b = entry(2, git("rev-parse", "b"));
161 expect(await enqueue(repoId, [b, j], gate)).toEqual([false, true]);
162 expect(gates.slice(before)).toEqual([
163 { pulls: [10], ok: true },
164 { pulls: [2], ok: false },
165 ]);
166 });
167
138168 test("a push is a new head: B is forgotten, batched with the rest, and lands", async () => {
139169 const pushed = entry(2, await headAdding("b2", "b2.txt"));
140170 const g = entry(7, await headAdding("g", "g.txt"));
test/merge-queue.test.ts +11 −12
@@ -576,7 +576,7 @@ describe("the merge queue", () => {
576576 expect(gatesOf(repo)).toHaveLength(before);
577577 });
578578
579 test("a red pull stays out of the queue when main moves, and the re-gate checks it alone after the fresh pulls (#4235)", async () => {
579 test("a red pull stays out of the queue when main moves, and the re-gate gates it fresh with the pulls then waiting (#4235, #6493)", async () => {
580580 const repo = await autonomousRepo("red-stays-out");
581581 const a = await greenPull(repo, "a.ts");
582582 const red = await greenPull(repo, "red-x.ts");
@@ -596,19 +596,18 @@ describe("the merge queue", () => {
596596 before = gatesOf(repo).length;
597597 expect(await sweepGreen(app, repo)).toBe(1);
598598 expect(merged(repo, c)).toBe(true);
599 const [fresh, recheck] = gatesOf(repo).slice(before);
600 expect(fresh?.ok).toBe(true);
601 expect(git(repo, "ls-tree", "--name-only", fresh!.commit)).not.toContain("red-x.ts");
602 expect(recheck?.ok).toBe(false);
603 // Gated on main once c had landed: c is under it, and nothing else rode with it.
604 expect(git(repo, "ls-tree", "--name-only", recheck!.commit).split("\n").sort()).toEqual(
605 ["README.md", "a.ts", "b.ts", "c.ts", "forge.json", "red-x.ts"],
606 );
607 // /boards: one red candidate of three in the hour; the re-check is none.
599 // Main moved since it went red: it gates again as a fresh entry (#6493),
600 // in the candidate with c, not behind c as a re-check.
601 const [batch, ...rest] = gatesOf(repo).slice(before);
602 expect(batch?.ok).toBe(false);
603 expect(git(repo, "ls-tree", "--name-only", batch!.commit)).toContain("c.ts");
604 expect(git(repo, "ls-tree", "--name-only", batch!.commit)).toContain("red-x.ts");
605 expect(rest.at(-1)?.ok).toBe(true);
606 // /boards: two red candidates of four in the hour.
608607 const page = await (await fetch(`${servers.url}/boards`, { headers: { cookie: await login() } })).text();
609 expect(page).toMatch(/data-queue="alice\/red-stays-out">[\s\S]*?data-red-batches>1 of 3</);
608 expect(page).toMatch(/data-queue="alice\/red-stays-out">[\s\S]*?data-red-batches>2 of 4</);
610609 // The terminal's Flow dialog says the same, on a line of its own under the table.
611 expect(flowLines(app.db).join(" ").replace(/\s+/g, " ")).toMatch(/Red batches, last hour: [^.]*alice\/red-stays-out 1 of 3/);
610 expect(flowLines(app.db).join(" ").replace(/\s+/g, " ")).toMatch(/Red batches, last hour: [^.]*alice\/red-stays-out 2 of 4/);
612611 // The MCP rows keep their shape: the rate is the page's and the terminal's.
613612 expect(Object.keys((await mcpQueue()).find((q) => (q as { repo: string }).repo === "alice/red-stays-out") as object)).not.toContain("batches");
614613 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs