b889473 · b7b422f
@@ -0,0 +1,29 @@
/**
 * Which queued checks belong to an approved pull (#6396), for `pickNext`.
 *
 * A check is an approved pull's when its own commit carries an approval and
 * no change requested: the rule `crew/finishing.ts` calls `approvedOnHead`,
 * read for the run's commit rather than the pull's current head, so a check
 * for a head since pushed over does not jump the queue.
 */
import type { Database } from "bun:sqlite";
import { reviewSummary } from "../core/reviews.ts";
import type { Run } from "../core/runs.ts";
/** Whether the run's pull has an approval on the run's commit and no change requested there. */
export function approvedForCheck(db: Database, run: Pick<Run, "issueId" | "commitOid">): boolean {
  if (run.issueId === null) return false;
  const seen = reviewSummary(db, run.issueId, run.commitOid);
  return seen.approvals > 0 && seen.changesRequested === 0;
}
/** `approvedForCheck`, asked at most once per run for as long as the predicate lives. */
export function approvedOnce(db: Database): (run: Run) => boolean {
  const known = new Map<string, boolean>();
  return (run) => {
    let yes = known.get(run.id);
    if (yes === undefined) known.set(run.id, (yes = approvedForCheck(db, run)));
    return yes;
  };
}
@@ -47,6 +47,7 @@
import {
import { Gate, runGit } from "../git/env.ts";
import { resolveOid } from "../git/merge.ts";
import { mergeForCheck } from "./merged.ts";
import { approvedOnce } from "./approved_first.ts";
import { checkOnApproval } from "./approval.ts";
import { checkKey, provenBy, recordProven } from "./proven.ts";
import { giveHistory } from "./history.ts";
@@ -209,30 +210,31 @@
export const STARVATION_MS = BUDGETS.laneWait / 2;
 * recently, so a push's own check is never stuck behind pulls that queued
 * hours earlier — unless something has already waited `STARVATION_MS` or
 * more, in which case the longest-waiting check goes first regardless of
 * how fresh the rest are. Ties (down to the same instant) fall back to id
 * order, which is itself creation order (`kernel/id.ts`).
 * how fresh the rest are. Checks of approved pulls (`approved`) go before
 * the rest, newest first among themselves (#6396): a check that is the last
 * thing between a pull and landing is not held up by a new pull's. Ties
 * (down to the same instant) fall back to id order, which is itself
 * creation order (`kernel/id.ts`).
 */
export function pickNext(
  queued: readonly Run[],
  now: number = Date.now(),
  approved: (run: Run) => boolean = () => false,
): Run | undefined {
  if (queued.length === 0) return undefined;
  const newer = (a: Run, b: Run): boolean =>
    a.createdAt > b.createdAt || (a.createdAt === b.createdAt && a.id > b.id);
  let oldest = queued[0]!;
  let newest = queued[0]!;
  let newest: Run | undefined;
  let newestApproved: Run | undefined;
  for (const run of queued) {
    if (
      run.createdAt < oldest.createdAt ||
      (run.createdAt === oldest.createdAt && run.id < oldest.id)
    )
      oldest = run;
    if (
      run.createdAt > newest.createdAt ||
      (run.createdAt === newest.createdAt && run.id > newest.id)
    )
      newest = run;
    if (newer(oldest, run)) oldest = run;
    if (newest === undefined || newer(run, newest)) newest = run;
    if (approved(run) && (newestApproved === undefined || newer(run, newestApproved)))
      newestApproved = run;
  }
  if (now - Date.parse(oldest.createdAt) >= STARVATION_MS) return oldest;
  return newest;
  return newestApproved ?? newest;
}
/**
@@ -438,11 +440,12 @@
export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
  /** `pickNext` among `queued`, passing over any whose account is at its limit. */
  function startable(queued: readonly Run[]): Run | undefined {
    let rest = queued;
    let next = pickNext(rest);
    const isApproved = approvedOnce(app.db);
      
        junipercrew
        18d ago
        outdated
      
      The predicate holds the real new logic (head-specific approval, no changes requested) and nothing tests it. Extract it into a named function and cover it with a scheduler-level test. Also memoise it: it queries once per queued run on every pickNext call, including the repeats in the atLimit loop.
    
    let next = pickNext(rest, Date.now(), isApproved);
    while (next !== undefined && atLimit(next)) {
      const held = next;
      rest = rest.filter((run) => run !== held);
      next = pickNext(rest);
      next = pickNext(rest, Date.now(), isApproved);
    }
    return next;
  }
@@ -0,0 +1,120 @@
/**
 * Which queued checks count as an approved pull's (#6396), read from real
 * reviews and fed to `pickNext` the way the checks service feeds it.
 */
import { afterAll, beforeAll, expect, test } from "bun:test";
import { mkdtemp, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { createApp, type App } from "../src/app.ts";
import { openPull } from "../src/actions/pulls.ts";
import { commitToBranch, createRepo } from "../src/actions/repos.ts";
import type { Pull } from "../src/core/pulls.ts";
import type { Repo } from "../src/core/repos.ts";
import { submitReview } from "../src/core/reviews.ts";
import { cancelRun, createRun } from "../src/core/runs.ts";
import { createUser, type User } from "../src/core/users.ts";
import { approvedForCheck, approvedOnce } from "../src/checks/approved_first.ts";
import { pickNext } from "../src/checks/service.ts";
let dataDir: string;
let app: App;
let alice: User;
let repo: Repo;
async function pull(n: number): Promise<Pull> {
  const branch = await commitToBranch(app, alice, repo, {
    branch: `feature/${n}`,
    base: "main",
    message: `Change ${n}`,
    files: [{ path: `f${n}.ts`, content: `export const f = ${n};\n` }],
  });
  if (!branch.ok) throw new Error(JSON.stringify(branch.error));
  const opened = await openPull(app, alice, repo, {
    title: `Change ${n}`,
    body: "",
    baseBranch: "main",
    headBranch: `feature/${n}`,
  });
  if (!opened.ok) throw new Error(String(opened.error));
  return opened.value;
}
const review = (p: Pull, state: "approved" | "changes_requested", commitOid = p.headOid) => {
  const r = submitReview(app.db, { issueId: p.id, authorId: alice.id, state, commitOid });
  if (!r.ok) throw new Error(r.error);
};
const check = (p: Pull, commitOid = p.headOid) =>
  createRun(app.db, { repoId: repo.id, kind: "check", env: `pull-${p.number}`, issueId: p.id, commitOid });
beforeAll(async () => {
  dataDir = await mkdtemp(join(tmpdir(), "forge-approved-first-"));
  app = createApp({ dataDir, port: 0 });
  const a = await createUser(app.db, {
    origin: "seed",
    trusted: true,
    username: "alice",
    email: "[email protected]",
    password: "a-good-passphrase",
    adminIfFirst: true,
  });
  if (!a.ok) throw new Error(a.error);
  alice = a.value;
  const made = await createRepo(app, alice, { name: "lane", visibility: "private" });
  if (!made.ok) throw new Error(String(made.error));
  repo = made.value;
  const seeded = await commitToBranch(app, alice, repo, {
    message: "Start",
    files: [{ path: "a.ts", content: "export const a = 0;\n" }],
  });
  if (!seeded.ok) throw new Error(JSON.stringify(seeded.error));
});
afterAll(async () => {
  app.close();
  await rm(dataDir, { recursive: true, force: true });
});
test("the older approved pull's check takes the slot; superseded and contested heads do not", async () => {
  const old = await pull(1);
  const contested = await pull(2);
  const superseded = await pull(3);
  const fresh = await pull(4);
  review(old, "approved");
  review(contested, "approved");
  review(contested, "changes_requested");
  // Approved on a head the pull has since moved past; the queued check is of the new one.
  review(superseded, "approved", "0".repeat(40));
  const runs = [old, contested, superseded, fresh].map((p) => check(p));
  const [oldRun, contestedRun, supersededRun, freshRun] = runs as [
    (typeof runs)[number], (typeof runs)[number], (typeof runs)[number], (typeof runs)[number],
  ];
  expect(approvedForCheck(app.db, oldRun)).toBe(true);
  expect(approvedForCheck(app.db, contestedRun)).toBe(false);
  expect(approvedForCheck(app.db, supersededRun)).toBe(false);
  expect(approvedForCheck(app.db, freshRun)).toBe(false);
  const now = Date.parse(freshRun.createdAt);
  expect(pickNext(runs, now, approvedOnce(app.db))?.id).toBe(oldRun.id);
  // Without it, the newest would have taken the slot.
  expect(pickNext(runs.filter((r) => r.id !== oldRun.id), now, approvedOnce(app.db))?.id).toBe(
    freshRun.id,
  );
  for (const run of runs) cancelRun(app.db, run.id);
});
test("a run with no pull is never approved, and each run is asked once", async () => {
  const p = await pull(5);
  review(p, "approved");
  const run = check(p);
  expect(approvedForCheck(app.db, { issueId: null, commitOid: p.headOid })).toBe(false);
  const ask = approvedOnce(app.db);
  expect(ask(run)).toBe(true);
  // A later change request is not seen: the answer was memoised for this pick.
  review(p, "changes_requested");
  expect(ask(run)).toBe(true);
  expect(approvedOnce(app.db)(run)).toBe(false);
  cancelRun(app.db, run.id);
});
@@ -859,6 +859,27 @@
describe("checks", () => {
    for (const run of [a, b, c]) cancelRun(app.db, run.id);
  });
  test("pickNext puts an approved pull's check first, newest among them, and the starved before all", () => {
    const a = createRun(app.db, { repoId: repo.id, kind: "check", env: "a" });
    const b = createRun(app.db, { repoId: repo.id, kind: "check", env: "b" });
    const c = createRun(app.db, { repoId: repo.id, kind: "check", env: "c" });
    const now = Date.parse(c.createdAt);
    const only = (...ids: string[]) => (run: { id: string }) => ids.includes(run.id);
    // An older approved check beats a newer unapproved one.
    expect(pickNext([a, b, c], now, only(a.id))?.id).toBe(a.id);
    // Newest first among the approved.
    expect(pickNext([a, b, c], now, only(a.id, b.id))?.id).toBe(b.id);
    // With none approved, the order is unchanged.
    expect(pickNext([a, b, c], now, () => false)?.id).toBe(c.id);
    // A starved unapproved check still goes first.
    const later = now + STARVATION_MS + 1_000;
    const old = { ...a, createdAt: new Date(now - 1_000).toISOString() };
    const fresh = { ...b, createdAt: new Date(later).toISOString() };
    const approvedFresh = { ...c, createdAt: new Date(later + 1).toISOString() };
    expect(pickNext([fresh, approvedFresh, old], later, only(approvedFresh.id))?.id).toBe(old.id);
    for (const run of [a, b, c]) cancelRun(app.db, run.id);
  });
  test("when a slot frees, the newest of several queued checks runs first", async () => {
    // Three checks, on three environments so none supersedes another. Their
    // commits are written before the slot is taken: written after, on a
		@@ -0,0 +1,29 @@
	1	/**
	2	* Which queued checks belong to an approved pull (#6396), for `pickNext`.
	3	*
	4	* A check is an approved pull's when its own commit carries an approval and
	5	* no change requested: the rule `crew/finishing.ts` calls `approvedOnHead`,
	6	* read for the run's commit rather than the pull's current head, so a check
	7	* for a head since pushed over does not jump the queue.
	8	*/
	9
	10	import type { Database } from "bun:sqlite";
	11	import { reviewSummary } from "../core/reviews.ts";
	12	import type { Run } from "../core/runs.ts";
	13
	14	/** Whether the run's pull has an approval on the run's commit and no change requested there. */
	15	export function approvedForCheck(db: Database, run: Pick<Run, "issueId" \| "commitOid">): boolean {
	16	if (run.issueId === null) return false;
	17	const seen = reviewSummary(db, run.issueId, run.commitOid);
	18	return seen.approvals > 0 && seen.changesRequested === 0;
	19	}
	20
	21	/** `approvedForCheck`, asked at most once per run for as long as the predicate lives. */
	22	export function approvedOnce(db: Database): (run: Run) => boolean {
	23	const known = new Map<string, boolean>();
	24	return (run) => {
	25	let yes = known.get(run.id);
	26	if (yes === undefined) known.set(run.id, (yes = approvedForCheck(db, run)));
	27	return yes;
	28	};
	29	}
		@@ -47,6 +47,7 @@ import {
47	47	import { Gate, runGit } from "../git/env.ts";
48	48	import { resolveOid } from "../git/merge.ts";
49	49	import { mergeForCheck } from "./merged.ts";
	50	import { approvedOnce } from "./approved_first.ts";
50	51	import { checkOnApproval } from "./approval.ts";
51	52	import { checkKey, provenBy, recordProven } from "./proven.ts";
52	53	import { giveHistory } from "./history.ts";
		@@ -209,30 +210,31 @@ export const STARVATION_MS = BUDGETS.laneWait / 2;
209	210	* recently, so a push's own check is never stuck behind pulls that queued
210	211	* hours earlier — unless something has already waited `STARVATION_MS` or
211	212	* more, in which case the longest-waiting check goes first regardless of
212		* how fresh the rest are. Ties (down to the same instant) fall back to id
213		* order, which is itself creation order (`kernel/id.ts`).
	213	* how fresh the rest are. Checks of approved pulls (`approved`) go before
	214	* the rest, newest first among themselves (#6396): a check that is the last
	215	* thing between a pull and landing is not held up by a new pull's. Ties
	216	* (down to the same instant) fall back to id order, which is itself
	217	* creation order (`kernel/id.ts`).
214	218	*/
215	219	export function pickNext(
216	220	queued: readonly Run[],
217	221	now: number = Date.now(),
	222	approved: (run: Run) => boolean = () => false,
218	223	): Run \| undefined {
219	224	if (queued.length === 0) return undefined;
	225	const newer = (a: Run, b: Run): boolean =>
	226	a.createdAt > b.createdAt \|\| (a.createdAt === b.createdAt && a.id > b.id);
220	227	let oldest = queued[0]!;
221		let newest = queued[0]!;
	228	let newest: Run \| undefined;
	229	let newestApproved: Run \| undefined;
222	230	for (const run of queued) {
223		if (
224		run.createdAt < oldest.createdAt \|\|
225		(run.createdAt === oldest.createdAt && run.id < oldest.id)
226		)
227		oldest = run;
228		if (
229		run.createdAt > newest.createdAt \|\|
230		(run.createdAt === newest.createdAt && run.id > newest.id)
231		)
232		newest = run;
	231	if (newer(oldest, run)) oldest = run;
	232	if (newest === undefined \|\| newer(run, newest)) newest = run;
	233	if (approved(run) && (newestApproved === undefined \|\| newer(run, newestApproved)))
	234	newestApproved = run;
233	235	}
234	236	if (now - Date.parse(oldest.createdAt) >= STARVATION_MS) return oldest;
235		return newest;
	237	return newestApproved ?? newest;
236	238	}
237	239
238	240	/**
		@@ -438,11 +440,12 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
438	440	/** `pickNext` among `queued`, passing over any whose account is at its limit. */
439	441	function startable(queued: readonly Run[]): Run \| undefined {
440	442	let rest = queued;
441		let next = pickNext(rest);
	443	const isApproved = approvedOnce(app.db);
		junipercrew 18d ago outdated The predicate holds the real new logic (head-specific approval, no changes requested) and nothing tests it. Extract it into a named function and cover it with a scheduler-level test. Also memoise it: it queries once per queued run on every `pickNext` call, including the repeats in the `atLimit` loop.
	444	let next = pickNext(rest, Date.now(), isApproved);
442	445	while (next !== undefined && atLimit(next)) {
443	446	const held = next;
444	447	rest = rest.filter((run) => run !== held);
445		next = pickNext(rest);
	448	next = pickNext(rest, Date.now(), isApproved);
446	449	}
447	450	return next;
448	451	}
		@@ -0,0 +1,120 @@
	1	/**
	2	* Which queued checks count as an approved pull's (#6396), read from real
	3	* reviews and fed to `pickNext` the way the checks service feeds it.
	4	*/
	5	import { afterAll, beforeAll, expect, test } from "bun:test";
	6	import { mkdtemp, rm } from "node:fs/promises";
	7	import { tmpdir } from "node:os";
	8	import { join } from "node:path";
	9	import { createApp, type App } from "../src/app.ts";
	10	import { openPull } from "../src/actions/pulls.ts";
	11	import { commitToBranch, createRepo } from "../src/actions/repos.ts";
	12	import type { Pull } from "../src/core/pulls.ts";
	13	import type { Repo } from "../src/core/repos.ts";
	14	import { submitReview } from "../src/core/reviews.ts";
	15	import { cancelRun, createRun } from "../src/core/runs.ts";
	16	import { createUser, type User } from "../src/core/users.ts";
	17	import { approvedForCheck, approvedOnce } from "../src/checks/approved_first.ts";
	18	import { pickNext } from "../src/checks/service.ts";
	19
	20	let dataDir: string;
	21	let app: App;
	22	let alice: User;
	23	let repo: Repo;
	24
	25	async function pull(n: number): Promise<Pull> {
	26	const branch = await commitToBranch(app, alice, repo, {
	27	branch: `feature/${n}`,
	28	base: "main",
	29	message: `Change ${n}`,
	30	files: [{ path: `f${n}.ts`, content: `export const f = ${n};\n` }],
	31	});
	32	if (!branch.ok) throw new Error(JSON.stringify(branch.error));
	33	const opened = await openPull(app, alice, repo, {
	34	title: `Change ${n}`,
	35	body: "",
	36	baseBranch: "main",
	37	headBranch: `feature/${n}`,
	38	});
	39	if (!opened.ok) throw new Error(String(opened.error));
	40	return opened.value;
	41	}
	42
	43	const review = (p: Pull, state: "approved" \| "changes_requested", commitOid = p.headOid) => {
	44	const r = submitReview(app.db, { issueId: p.id, authorId: alice.id, state, commitOid });
	45	if (!r.ok) throw new Error(r.error);
	46	};
	47
	48	const check = (p: Pull, commitOid = p.headOid) =>
	49	createRun(app.db, { repoId: repo.id, kind: "check", env: `pull-${p.number}`, issueId: p.id, commitOid });
	50
	51	beforeAll(async () => {
	52	dataDir = await mkdtemp(join(tmpdir(), "forge-approved-first-"));
	53	app = createApp({ dataDir, port: 0 });
	54	const a = await createUser(app.db, {
	55	origin: "seed",
	56	trusted: true,
	57	username: "alice",
	58	email: "[email protected]",
	59	password: "a-good-passphrase",
	60	adminIfFirst: true,
	61	});
	62	if (!a.ok) throw new Error(a.error);
	63	alice = a.value;
	64	const made = await createRepo(app, alice, { name: "lane", visibility: "private" });
	65	if (!made.ok) throw new Error(String(made.error));
	66	repo = made.value;
	67	const seeded = await commitToBranch(app, alice, repo, {
	68	message: "Start",
	69	files: [{ path: "a.ts", content: "export const a = 0;\n" }],
	70	});
	71	if (!seeded.ok) throw new Error(JSON.stringify(seeded.error));
	72	});
	73
	74	afterAll(async () => {
	75	app.close();
	76	await rm(dataDir, { recursive: true, force: true });
	77	});
	78
	79	test("the older approved pull's check takes the slot; superseded and contested heads do not", async () => {
	80	const old = await pull(1);
	81	const contested = await pull(2);
	82	const superseded = await pull(3);
	83	const fresh = await pull(4);
	84	review(old, "approved");
	85	review(contested, "approved");
	86	review(contested, "changes_requested");
	87	// Approved on a head the pull has since moved past; the queued check is of the new one.
	88	review(superseded, "approved", "0".repeat(40));
	89	const runs = [old, contested, superseded, fresh].map((p) => check(p));
	90	const [oldRun, contestedRun, supersededRun, freshRun] = runs as [
	91	(typeof runs)[number], (typeof runs)[number], (typeof runs)[number], (typeof runs)[number],
	92	];
	93
	94	expect(approvedForCheck(app.db, oldRun)).toBe(true);
	95	expect(approvedForCheck(app.db, contestedRun)).toBe(false);
	96	expect(approvedForCheck(app.db, supersededRun)).toBe(false);
	97	expect(approvedForCheck(app.db, freshRun)).toBe(false);
	98
	99	const now = Date.parse(freshRun.createdAt);
	100	expect(pickNext(runs, now, approvedOnce(app.db))?.id).toBe(oldRun.id);
	101	// Without it, the newest would have taken the slot.
	102	expect(pickNext(runs.filter((r) => r.id !== oldRun.id), now, approvedOnce(app.db))?.id).toBe(
	103	freshRun.id,
	104	);
	105	for (const run of runs) cancelRun(app.db, run.id);
	106	});
	107
	108	test("a run with no pull is never approved, and each run is asked once", async () => {
	109	const p = await pull(5);
	110	review(p, "approved");
	111	const run = check(p);
	112	expect(approvedForCheck(app.db, { issueId: null, commitOid: p.headOid })).toBe(false);
	113	const ask = approvedOnce(app.db);
	114	expect(ask(run)).toBe(true);
	115	// A later change request is not seen: the answer was memoised for this pick.
	116	review(p, "changes_requested");
	117	expect(ask(run)).toBe(true);
	118	expect(approvedOnce(app.db)(run)).toBe(false);
	119	cancelRun(app.db, run.id);
	120	});
		@@ -859,6 +859,27 @@ describe("checks", () => {
859	859	for (const run of [a, b, c]) cancelRun(app.db, run.id);
860	860	});
861	861
	862	test("pickNext puts an approved pull's check first, newest among them, and the starved before all", () => {
	863	const a = createRun(app.db, { repoId: repo.id, kind: "check", env: "a" });
	864	const b = createRun(app.db, { repoId: repo.id, kind: "check", env: "b" });
	865	const c = createRun(app.db, { repoId: repo.id, kind: "check", env: "c" });
	866	const now = Date.parse(c.createdAt);
	867	const only = (...ids: string[]) => (run: { id: string }) => ids.includes(run.id);
	868	// An older approved check beats a newer unapproved one.
	869	expect(pickNext([a, b, c], now, only(a.id))?.id).toBe(a.id);
	870	// Newest first among the approved.
	871	expect(pickNext([a, b, c], now, only(a.id, b.id))?.id).toBe(b.id);
	872	// With none approved, the order is unchanged.
	873	expect(pickNext([a, b, c], now, () => false)?.id).toBe(c.id);
	874	// A starved unapproved check still goes first.
	875	const later = now + STARVATION_MS + 1_000;
	876	const old = { ...a, createdAt: new Date(now - 1_000).toISOString() };
	877	const fresh = { ...b, createdAt: new Date(later).toISOString() };
	878	const approvedFresh = { ...c, createdAt: new Date(later + 1).toISOString() };
	879	expect(pickNext([fresh, approvedFresh, old], later, only(approvedFresh.id))?.id).toBe(old.id);
	880	for (const run of [a, b, c]) cancelRun(app.db, run.id);
	881	});
	882
862	883	test("when a slot frees, the newest of several queued checks runs first", async () => {
863	884	// Three checks, on three environments so none supersedes another. Their
864	885	// commits are written before the slot is taken: written after, on a