Public: anyone can read this; the crew can change it.
A red merge batch is searched by disjoint halves, never by nested prefixes holding the same suspects #6373
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6353-a-red-merge-batch-is-searched-by-disjoin → main 2 commits fork 3 threads · 3 unresolved
3 files
changed +176
−51
src/checks/queue.ts +56 −44
| @@ -11,11 +11,10 @@ | ||
| 11 | 11 | * candidate gates on main, the next gates on top of it (`depth` deep): if |
| 12 | 12 | * the first lands whole, the next lands right after on the tree it was |
| 13 | 13 | * gated on; if not, the next is discarded and formed again on main. A red |
| 14 | * candidate is searched by gating several of its prefixes at once, one per | |
| 15 | * free slot: the longest green prefix lands, the culprit is in the steps up | |
| 16 | * to the first red prefix, and the steps after it wait for the next | |
| 17 | * candidate. With no slot free that is one prefix, the front half: plain | |
| 18 | * bisection. A single red step is the culprit and is handed back. | |
| 14 | * candidate is searched by gating disjoint segments of it at once, one per | |
| 15 | * free slot: the culprit is in the first red segment, searched again. With | |
| 16 | * no slot free that is one segment, the front half: plain bisection. A | |
| 17 | * single red step is the culprit and is handed back. | |
| 19 | 18 | * |
| 20 | 19 | * A gate cut off by the forge stopping is no verdict: nothing is handed |
| 21 | 20 | * back, and the forge that carries on admits the line again (#3155). A gate |
| @@ -790,13 +789,14 @@ async function round( | ||
| 790 | 789 | |
| 791 | 790 | /** |
| 792 | 791 | * A red candidate: one step is the culprit, handed back on the base it was |
| 793 | * red on. More are searched (#4053): the candidate is built again on the | |
| 794 | * base's tip and `k` of its prefixes are gated at once — one, the front | |
| 795 | * half, plus one per free check slot, evenly cut. The longest green prefix | |
| 796 | * before the first red one lands; the culprit is among the steps up to the | |
| 797 | * first red prefix (or, all green, the rest of the candidate, whose whole | |
| 798 | * tree is the red one), searched again; the steps after wait for the next | |
| 799 | * candidate. | |
| 792 | * red on. More are searched (#4053, #6353): its steps are cut into disjoint | |
| 793 | * segments, one gate per free check slot (at least the front half), each | |
| 794 | * built and gated on the base alone, so no pull is in two gates of a round. | |
| 795 | * The first red segment holds a culprit and is searched again, on top of a | |
| 796 | * green first segment, which lands; the other green ones, each cleared | |
| 797 | * alone, and the steps after wait for the next candidate. | |
| 798 | * All green, the interaction is between them: the first lands and the rest | |
| 799 | * are searched on it. | |
| 800 | 800 | */ |
| 801 | 801 | async function judged( |
| 802 | 802 | search: Search, |
| @@ -824,45 +824,57 @@ async function judged( | ||
| 824 | 824 | return []; |
| 825 | 825 | } |
| 826 | 826 | const n = steps.length; |
| 827 | const k = Math.min(n - 1, 1 + Math.max(0, room())); | |
| 828 | const cuts = [...new Set(Array.from({ length: k }, (_, j) => Math.ceil(((j + 1) * n) / (k + 1))))].filter( | |
| 829 | (c) => c >= 1 && c < n, | |
| 830 | ); | |
| 831 | const last = cuts.at(-1)!; | |
| 832 | const whole = await build(steps); | |
| 833 | if (whole === undefined || whole.steps.slice(0, last).some((s, i) => s.entry !== steps[i]!.entry) || whole.steps.length < last) { | |
| 827 | // One gate per free slot, at least the front half; with no slot free the | |
| 828 | // back half is not gated and waits for the next round. | |
| 829 | const free = Math.max(0, room()); | |
| 830 | const cut = free > 0 ? Math.min(n, 1 + free) : 2; | |
| 831 | const runs = split(steps, cut - 1); | |
| 832 | const segments = runs.slice(0, free > 0 ? cut : 1); | |
| 833 | const rest = runs.slice(segments.length).flat(); | |
| 834 | // Each segment is built and gated on the base alone: no step is in two | |
| 835 | // gates, so one culprit turns one gate red, not every prefix after it. | |
| 836 | const built = await Promise.all(segments.map((seg) => build(seg))); | |
| 837 | const gates = built.filter((c): c is Candidate => c !== undefined && c.steps.length > 0); | |
| 838 | if (gates.length !== segments.length || gates.some((c) => c.baseOid !== gates[0]!.baseOid)) { | |
| 834 | 839 | // The base moved under the search: nothing here is decided, and the |
| 835 | 840 | // next sweep admits these again onto the base as it is now. |
| 836 | 841 | for (const w of steps) w.settle(false); |
| 837 | 842 | return []; |
| 838 | 843 | } |
| 839 | const prefix = (c: number): Candidate => { | |
| 840 | const end = whole.steps[c - 1]!; | |
| 841 | return { ...whole, steps: whole.steps.slice(0, c), tipOid: end.commitOid, treeOid: end.treeOid }; | |
| 842 | }; | |
| 843 | const verdicts = await Promise.all(cuts.map((c) => gated(prefix(c)))); | |
| 844 | let green = 0; | |
| 845 | let stop: Exclude<LandVerdict, { lands: true }> | undefined; | |
| 846 | let stopAt = n; | |
| 847 | for (const [j, c] of cuts.entries()) { | |
| 848 | const v = verdicts[j]!; | |
| 849 | if (v.lands) { | |
| 850 | green = c; | |
| 851 | continue; | |
| 852 | } | |
| 853 | stop = v; | |
| 854 | stopAt = c; | |
| 855 | break; | |
| 856 | } | |
| 857 | if (green > 0) await land(prefix(green), steps.slice(0, green)); | |
| 858 | if (stop !== undefined && "interrupted" in stop) { | |
| 859 | for (const w of steps.slice(green)) w.settle(false); | |
| 844 | const verdicts = await Promise.all(gates.map((c) => gated(c))); | |
| 845 | if (verdicts.some((v) => !v.lands && "interrupted" in v)) { | |
| 846 | for (const w of steps) w.settle(false); | |
| 860 | 847 | return []; |
| 861 | 848 | } |
| 862 | const known = stop === undefined ? { treeOid: candidate.treeOid, red } : { treeOid: prefix(stopAt).treeOid, red: stop }; | |
| 863 | const now = [...landed, ...steps.slice(0, green).map((w) => w.entry)]; | |
| 864 | const left = await round(search, steps.slice(green, stopAt), known, now); | |
| 865 | return [...left, ...steps.slice(stopAt)]; | |
| 849 | const inGate = gates.map((c, i) => inCandidate(segments[i]!, c)); | |
| 850 | const bad = verdicts.findIndex((v) => !v.lands); | |
| 851 | if (bad < 0) { | |
| 852 | // Each green alone, the whole red: the interaction is between them. The | |
| 853 | // first lands, gated as it is on the base; the rest are searched on it. | |
| 854 | await land(gates[0]!, inGate[0]!); | |
| 855 | const next = [...inGate.slice(1).flat(), ...rest]; | |
| 856 | return round(search, next, { treeOid: candidate.treeOid, red }, [...landed, ...inGate[0]!.map((w) => w.entry)]); | |
| 857 | } | |
| 858 | // The first red segment holds a culprit and is searched again. A green | |
| 859 | // first segment before it lands as it was gated and the search goes on top | |
outdated
When |
||
| 860 | // of it; the other green segments, each gated alone, and the steps after | |
| 861 | // the red one wait, in line order, for the next candidate. | |
| 862 | const first = bad > 0; | |
| 863 | if (first) await land(gates[0]!, inGate[0]!); | |
| 864 | const now = first ? [...landed, ...inGate[0]!.map((w) => w.entry)] : landed; | |
| 865 | const before = inGate.slice(first ? 1 : 0, bad).flat(); | |
| 866 | const after = [...inGate.slice(bad + 1).flat(), ...rest]; | |
| 867 | const left = await round(search, inGate[bad]!, { treeOid: gates[bad]!.treeOid, red: verdicts[bad] as Red }, now); | |
| 868 | return [...before, ...left, ...after]; | |
| 869 | } | |
| 870 | ||
| 871 | /** The steps cut into `k` + 1 runs, evenly, none shared. */ | |
| 872 | function split<T>(steps: readonly T[], k: number): T[][] { | |
| 873 | const n = steps.length; | |
| 874 | const cuts = [0, ...Array.from({ length: k }, (_, j) => Math.ceil(((j + 1) * n) / (k + 1))), n]; | |
| 875 | const runs: T[][] = []; | |
| 876 | for (let i = 0; i + 1 < cuts.length; i++) if (cuts[i + 1]! > cuts[i]!) runs.push(steps.slice(cuts[i], cuts[i + 1])); | |
| 877 | return runs; | |
| 866 | 878 | } |
| 867 | 879 | |
| 868 | 880 | /** |
test/merge-queue-parallel.test.ts +10 −7
| @@ -163,13 +163,15 @@ describe("the merge queue in parallel", () => { | ||
| 163 | 163 | // beside the other seven and the next on main and the last on top. |
| 164 | 164 | await letGo(3); |
| 165 | 165 | // Wave three: the head was green alone; the eight without it are red |
| 166 | // again, and seven prefixes of them gate at once: three green, the fourth red. | |
| 167 | await letGo(7); | |
| 168 | // Wave four: the culprit is handed back without a gate of its own; the | |
| 169 | // four after it, the last and the head (at the tail) are one candidate. | |
| 166 | // again, and the eight are gated at once, no pull in two gates: one is red. | |
| 167 | await letGo(8); | |
| 168 | // Wave four: the first of the eight landed as gated; the culprit, red | |
| 169 | // alone, is gated on top of it and handed back. | |
| 170 | await letGo(1); | |
| 171 | // Wave five: the pulls cleared alone, with the head, are one candidate. | |
| 170 | 172 | await letGo(1); |
| 171 | 173 | expect(await sweep).toBe(9); |
| 172 | expect(wave - first).toBe(4); | |
| 174 | expect(wave - first).toBe(5); | |
| 173 | 175 | const red = pulls[4]!; |
| 174 | 176 | for (const pull of pulls) expect(merged(repo, pull)).toBe(pull !== red); |
| 175 | 177 | expect(latestDecision(app.db, red.id, "land")?.answer).toBe("red"); |
| @@ -184,8 +186,9 @@ describe("the merge queue in parallel", () => { | ||
| 184 | 186 | for (let i = 0; i < 10; i++) pulls.push(await greenPull(repo, i === 9 ? `red-${i}.ts` : `p${i}.ts`)); |
| 185 | 187 | const sweep = sweepGreen(app, repo); |
| 186 | 188 | await letGo(2); |
| 187 | // The first eight landed; the two behind are red on them, so one is | |
| 188 | // gated alone (the only prefix of two), and the culprit handed back. | |
| 189 | // The first eight landed; the two behind are red on them, so each is | |
| 190 | // gated alone: the culprit is handed back, the other gated again on main. | |
| 191 | await letGo(2); | |
| 189 | 192 | await letGo(1); |
| 190 | 193 | expect(await sweep).toBe(9); |
| 191 | 194 | for (const pull of pulls.slice(0, 9)) expect(merged(repo, pull)).toBe(true); |
test/merge-queue-search.test.ts added +110
| @@ -0,0 +1,110 @@ | ||
| 1 | /** | |
| 2 | * A red candidate is searched by disjoint segments, never by nested | |
| 3 | * prefixes holding the same suspects (#6353). The gate here is held open and | |
| 4 | * let go a wave at a time, so "at once" is counted, never timed; it reads | |
| 5 | * the candidate's tree: red when it holds `bad.txt`. | |
| 6 | */ | |
| 7 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 8 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 9 | import { tmpdir } from "node:os"; | |
| 10 | import { join } from "node:path"; | |
| 11 | import { type Candidate, enqueue, type Expect, type Queued } from "../src/checks/queue.ts"; | |
| 12 | import type { LandVerdict } from "../src/checks/tiers.ts"; | |
| 13 | import type { Pull } from "../src/core/pulls.ts"; | |
| 14 | ||
| 15 | describe("the merge queue searches a red candidate by disjoint segments", () => { | |
| 16 | let dir: string; | |
| 17 | const told: number[] = []; | |
| 18 | ||
| 19 | const git = (...args: string[]) => | |
| 20 | Bun.spawnSync(["git", "-C", dir, "-c", "user.name=t", "-c", "user.email=t@t", ...args]).stdout.toString().trim(); | |
| 21 | ||
| 22 | beforeAll(async () => { | |
| 23 | dir = await mkdtemp(join(tmpdir(), "forge-merge-queue-search-")); | |
| 24 | Bun.spawnSync(["git", "init", "-q", "-b", "main", dir]); | |
| 25 | git("commit", "-q", "--allow-empty", "-m", "start"); | |
| 26 | }); | |
| 27 | ||
| 28 | afterAll(async () => { | |
| 29 | await rm(dir, { recursive: true, force: true }); | |
| 30 | }); | |
| 31 | ||
| 32 | async function headAdding(branch: string, file: string): Promise<string> { | |
| 33 | git("checkout", "-q", "-B", branch, "main"); | |
| 34 | git("reset", "-q", "--hard", "main"); | |
| 35 | await Bun.write(join(dir, file), `${file}\n`); | |
| 36 | git("add", file); | |
| 37 | git("commit", "-q", "-m", file); | |
| 38 | git("checkout", "-q", "--detach"); | |
| 39 | return git("rev-parse", branch); | |
| 40 | } | |
| 41 | ||
| 42 | const verdictOf = (candidate: Candidate): LandVerdict => | |
| 43 | git("ls-tree", "--name-only", candidate.tipOid).split("\n").includes("bad.txt") | |
| 44 | ? { lands: false, reopen: true, comment: "red", learn: [] } | |
| 45 | : { lands: true }; | |
| 46 | ||
| 47 | function entry(number: number, headOid: string): Queued { | |
| 48 | return { | |
| 49 | pull: { id: `search-${number}`, number, baseBranch: "main", headOid } as Pull, | |
| 50 | dir, | |
| 51 | land: async (expect: Expect) => { | |
| 52 | if (git("rev-parse", "refs/heads/main") !== expect.baseOid) return undefined; | |
| 53 | const tip = git("commit-tree", expect.treeOid, "-p", expect.baseOid, "-m", `#${number}`); | |
| 54 | git("update-ref", "refs/heads/main", tip); | |
| 55 | return tip; | |
| 56 | }, | |
| 57 | release: () => {}, | |
| 58 | handBack: () => told.push(number), | |
| 59 | }; | |
| 60 | } | |
| 61 | ||
| 62 | test("a red 7-step candidate with 3 free slots: the gates share no step, and a culprit at step 5 is found in 2 rounds beyond the head's", async () => { | |
| 63 | const pulls: Queued[] = []; | |
| 64 | for (let i = 1; i <= 7; i++) pulls.push(entry(i, await headAdding(`s${i}`, i === 5 ? "bad.txt" : `f${i}.txt`))); | |
| 65 | let pending: { candidate: Candidate; go: (v: LandVerdict) => void }[] = []; | |
| 66 | const waves: number[][][] = []; | |
| 67 | const gate = (candidate: Candidate) => new Promise<LandVerdict>((go) => pending.push({ candidate, go })); | |
| 68 | const letGo = async (n: number) => { | |
| 69 | for (let i = 0; i < 2000 && pending.length < n; i++) await Bun.sleep(5); | |
| 70 | expect(pending).toHaveLength(n); | |
| 71 | const going = pending; | |
| 72 | pending = []; | |
| 73 | waves.push(going.map((p) => p.candidate.steps.map((s) => s.entry.pull.number))); | |
| 74 | for (const p of going) p.go(verdictOf(p.candidate)); | |
| 75 | }; | |
| 76 | const landed = enqueue("search", pulls, gate, { depth: 1, room: () => 3 }); | |
| 77 | await letGo(1); // the whole candidate: red | |
| 78 | await letGo(2); // its head alone (#4999), beside the six without it | |
| 79 | await letGo(4); // the six, cut into four segments gated at once | |
| 80 | await letGo(1); // the first segment landed as gated; the red one is gated on it | |
| 81 | await letGo(2); // the red segment, halved: the culprit is in it alone | |
| 82 | await letGo(1); // the pulls cleared alone, formed again as one candidate | |
| 83 | const answers = await landed; | |
| 84 | for (const wave of waves) { | |
| 85 | const seen = wave.flat(); | |
| 86 | expect(new Set(seen).size).toBe(seen.length); | |
| 87 | } | |
| 88 | expect(waves[3]).toEqual([[5, 6]]); | |
| 89 | expect(waves[4]!.flat().sort()).toEqual([5, 6]); | |
| 90 | expect(told).toEqual([5]); | |
| 91 | expect(answers[4]).toBe(false); | |
| 92 | expect(answers.filter(Boolean)).toHaveLength(6); | |
| 93 | }); | |
| 94 | ||
| 95 | test("with no slot free, the green front half lands and the red back half is not gated again", async () => { | |
| 96 | told.length = 0; | |
| 97 | const pulls: Queued[] = []; | |
| 98 | for (let i = 11; i <= 14; i++) pulls.push(entry(i, await headAdding(`t${i}`, i === 14 ? "bad.txt" : `f${i}.txt`))); | |
| 99 | const gates: number[][] = []; | |
| 100 | const gate = async (candidate: Candidate) => { | |
| 101 | gates.push(candidate.steps.map((s) => s.entry.pull.number)); | |
| 102 | return verdictOf(candidate); | |
| 103 | }; | |
| 104 | const answers = await enqueue("search-solo", pulls, gate, { depth: 1, room: () => 0 }); | |
| 105 | // The whole, then the front half; the back half has the whole's tree on it: known red. | |
| 106 | expect(gates).toEqual([[11, 12, 13, 14], [11, 12], [13]]); | |
| 107 | expect(told).toEqual([14]); | |
| 108 | expect(answers).toEqual([true, true, true, false]); | |
| 109 | }); | |
| 110 | }); |
new above
offline
treeOid: ""makes the next round gate the rest again. The rest, built on the segment that just landed, has the whole candidate's tree, which is already known to be red. Pass{ treeOid: candidate.treeOid, red }so that gate is skipped, as the old code did. With no slot free, this costs one extra full gate per red batch.