Skip to content

Commit 9479e45

Browse files
committed
fix(hub): keep a head refused for capacity queued until a slot frees
1 parent b2b59df commit 9479e45

25 files changed

Lines changed: 368 additions & 74 deletions

‎apps/hub/src/github/coalescer.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ test("a paused-label head whose pull request cannot be read is left failed, and
333333
expect(mails).toEqual([]);
334334
expect(state.rows()).toEqual([expect.objectContaining({ status: "failed", error: "delivery failed: Error: 502 Bad Gateway" })]);
335335
const pr = { number: 8, headSha: "sha1", updatedAt: new Date(START).toISOString(), draft: false };
336-
const plan = planTenant({ repos: [{ name: REPO, prs: [pr], rows: state.rows() }], runs: new Map(), now: new Date(START + DEFAULT_RECONCILE_POLICY.backoffMaxMs), policy: DEFAULT_RECONCILE_POLICY });
336+
const plan = planTenant({ repos: [{ name: REPO, prs: [pr], rows: state.rows() }], runs: new Map(), roomless: new Set(), now: new Date(START + DEFAULT_RECONCILE_POLICY.backoffMaxMs), policy: DEFAULT_RECONCILE_POLICY });
337337
expect(plan.get(REPO)?.enqueue.map((entry) => entry.number)).toEqual([8]);
338338
});
339339

‎apps/hub/src/github/interleave.postgres.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ function stubDb() {
4141
}
4242

4343
async function noRuns(): Promise<ObservedRuns> {
44-
return { byRepo: new Map() };
44+
return { byRepo: new Map(), roomless: new Set() };
4545
}
4646

4747
function route(store: TriageStateStore, deliver: Deliver) {

‎apps/hub/src/github/manager-runs.postgres.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ test("PostgreSQL: the reconciler does not mail a head again while a live manager
8080
const queuedAt = new Date(now.getTime() - 20 * MINUTE).toISOString();
8181
const queued: PrTriageRow = { number: 7, headSha: "h1", status: "queued", attempts: 1, firstSeenAt: queuedAt, queuedAt, updatedAt: queuedAt };
8282
function plan(rows: PrTriageRow[], runs: Awaited<ReturnType<typeof managerRuns>>) {
83-
return planTenant({ repos: [{ name: REPO, prs: [open(7, now), open(8, now)], rows }], runs: runs.byRepo, now, policy: DEFAULT_RECONCILE_POLICY }).get(REPO)!;
83+
return planTenant({ repos: [{ name: REPO, prs: [open(7, now), open(8, now)], rows }], runs: runs.byRepo, roomless: runs.roomless, now, policy: DEFAULT_RECONCILE_POLICY }).get(REPO)!;
8484
}
8585

8686
const running = plan([queued], await managerRuns(TENANT, [pr7, pr8]));
@@ -114,7 +114,7 @@ test("PostgreSQL: on the listener path a head is mailed once while its manager r
114114
async function reconcile(afterMs: number): Promise<PrTriageRow> {
115115
now = new Date(start.getTime() + afterMs);
116116
const runs = await managerRuns(TENANT, [pr]);
117-
const plan = planTenant({ repos: [{ name: REPO, prs: [open(7, start)], rows }], runs: runs.byRepo, now, policy: DEFAULT_RECONCILE_POLICY }).get(REPO)!;
117+
const plan = planTenant({ repos: [{ name: REPO, prs: [open(7, start)], rows }], runs: runs.byRepo, roomless: runs.roomless, now, policy: DEFAULT_RECONCILE_POLICY }).get(REPO)!;
118118
rows = plan.rows;
119119
mails += plan.enqueue.length;
120120
return rows[0]!;
@@ -149,7 +149,7 @@ test("PostgreSQL: a manager that went away mid-pass fails its pass's head, so th
149149
expect(runs.byRepo.get(REPO)?.get(runKey(7, "h1"))).toEqual([expect.objectContaining({ runId: "run_gone", status: "failed" })]);
150150
const queuedAt = new Date(now.getTime() - MINUTE).toISOString();
151151
const queued: PrTriageRow = { number: 7, headSha: "h1", status: "queued", attempts: 1, firstSeenAt: queuedAt, queuedAt, updatedAt: queuedAt };
152-
const plan = planTenant({ repos: [{ name: REPO, prs: [open(7, now)], rows: [queued] }], runs: runs.byRepo, now, policy: DEFAULT_RECONCILE_POLICY }).get(REPO)!;
152+
const plan = planTenant({ repos: [{ name: REPO, prs: [open(7, now)], rows: [queued] }], runs: runs.byRepo, roomless: runs.roomless, now, policy: DEFAULT_RECONCILE_POLICY }).get(REPO)!;
153153
expect(plan.rows).toEqual([expect.objectContaining({ status: "failed", runId: "run_gone", error: "run failed" })]);
154154
expect(plan.enqueue).toEqual([]);
155155
});
@@ -194,7 +194,7 @@ test("PostgreSQL: a head whose manager ended while it waited for CI shows as wai
194194
expect(observed.byRepo.get(REPO)?.get(runKey(8, "h1"))).toEqual([expect.objectContaining({ runId: "run_waited", status: "completed", waitingSince: expect.any(String) })]);
195195
const firstSeenAt = new Date(now.getTime() - MINUTE).toISOString();
196196
const row: PrTriageRow = { number: 8, headSha: "h1", status: "new", attempts: 0, firstSeenAt, updatedAt: firstSeenAt };
197-
const plan = planTenant({ repos: [{ name: REPO, prs: [open(8, now)], rows: [row] }], runs: observed.byRepo, now, policy: DEFAULT_RECONCILE_POLICY }).get(REPO)!;
197+
const plan = planTenant({ repos: [{ name: REPO, prs: [open(8, now)], rows: [row] }], runs: observed.byRepo, roomless: observed.roomless, now, policy: DEFAULT_RECONCILE_POLICY }).get(REPO)!;
198198
expect(plan.rows).toEqual([expect.objectContaining({ number: 8, status: "triaged", runId: "run_waited" })]);
199199
expect(plan.enqueue).toEqual([]);
200200
});

‎apps/hub/src/github/manager-runs.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@
44
import type { DB } from "@intx/db";
55
import { unsettledBy } from "@corbits/triage-workflows/logic/settled";
66
import { managedPulls, type PullRequestRef } from "../manager-state.js";
7-
import { runKey, type ObservedRun } from "./reconcile-plan.js";
7+
import { pullKey, runKey, type ObservedRun } from "./reconcile-plan.js";
88
import type { VerdictRecord, VerdictRecordStore } from "./verdict-record-store.js";
99

10-
/** Runs by repository, then by `${number}@${headSha}`. */
11-
export type ObservedRuns = { byRepo: Map<string, Map<string, ObservedRun[]>> };
10+
/** Runs by repository, then by `${number}@${headSha}`; `roomless`, by `pullKey`, the pull requests whose last manager was refused for capacity. */
11+
export type ObservedRuns = { byRepo: Map<string, Map<string, ObservedRun[]>>; roomless: Set<string> };
1212

1313
/** The runs of `pulls`' managers. */
1414
export type ManagerRuns = (tenantId: string, pulls: readonly PullRequestRef[]) => Promise<ObservedRuns>;
@@ -23,6 +23,7 @@ export function createManagerRuns(db: DB["db"], verdicts: Pick<VerdictRecordStor
2323
const pulls = await managedPulls(db, tenantId, asked);
2424
const recorded = await verdicts.ofRuns(tenantId, [...new Set(pulls.flatMap((pull) => pull.history.map((pass) => pass.runId)))], asked);
2525
const byRepo: ObservedRuns["byRepo"] = new Map();
26+
const roomless = new Set<string>();
2627

2728
function observed(repo: string, number: number, head: string, run: ObservedRun): void {
2829
const byHead = byRepo.get(repo) ?? new Map<string, ObservedRun[]>();
@@ -33,8 +34,9 @@ export function createManagerRuns(db: DB["db"], verdicts: Pick<VerdictRecordStor
3334
else byHead.set(key, [run]);
3435
}
3536

36-
for (const { pr, live, gone, waiting } of pulls) {
37+
for (const { pr, live, gone, waiting, roomless: refused } of pulls) {
3738
const { source } = pr;
39+
if (refused) roomless.add(pullKey(pr.repo, pr.number));
3840
// The hub checked the run is up, so it is running now and never stuck.
3941
if (live?.head !== undefined) observed(pr.repo, pr.number, live.head, { runId: live.runId, source, status: "running", startedAt: now().toISOString(), ...(live.pass && { passAt: live.pass.takenAt }), ...(live.awaiting.length > 0 && { awaiting: live.awaiting }), ...(live.waitingSince !== undefined && { waitingSince: live.waitingSince }) });
4042
// No manager is up while the head waits only on CI; the reconciler wakes one once CI settles.
@@ -44,6 +46,6 @@ export function createManagerRuns(db: DB["db"], verdicts: Pick<VerdictRecordStor
4446
observed(pr.repo, pr.number, verdict.headSha, { runId: verdict.runId, source, status: "completed", startedAt: verdict.startedAt.toISOString(), ...settlesBy(verdict), ...(verdict.trigger !== null && { trigger: verdict.trigger }) });
4547
}
4648
}
47-
return { byRepo };
49+
return { byRepo, roomless };
4850
};
4951
}

‎apps/hub/src/github/pr-triage.test.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ function stubDb(repo: Record<string, unknown>) {
3131

3232
function observed(runs: ObservedRun[]): GithubPrTriageDeps["managerRuns"] {
3333
return async function managerRuns() {
34-
return { byRepo: new Map([[REPO, new Map([["8@abc123", runs]])]]), runCount: runs.length, verdicts: [], approved: [] };
34+
return { byRepo: new Map([[REPO, new Map([["8@abc123", runs]])]]), roomless: new Set<string>(), runCount: runs.length, verdicts: [], approved: [] };
3535
};
3636
}
3737

@@ -263,6 +263,7 @@ describe("createGithubPrTriage", () => {
263263
rows = planTenant({
264264
repos: [{ name: REPO, prs: [{ number: 8, headSha: "abc123", updatedAt: at(3_600_000), draft: false }], rows }],
265265
runs: new Map([[REPO, new Map([["8@abc123", [oldVerdict]]])]]),
266+
roomless: new Set(),
266267
now: NOW, policy: DEFAULT_RECONCILE_POLICY,
267268
}).get(REPO)!.rows;
268269
const second = await handler(ENABLED, s, deps)(request(), MEMBER);

‎apps/hub/src/github/reconcile-plan.test.ts‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { describe, expect, test } from "bun:test";
2-
import type { PrTriageRow } from "@corbits/triage-contracts";
3-
import { DEFAULT_RECONCILE_POLICY, planTenant, requeue, runKey, type ObservedRun, type OpenPr, type RepoPlan } from "./reconcile-plan.js";
2+
import { WAITING_FOR_CAPACITY, type PrTriageRow } from "@corbits/triage-contracts";
3+
import { DEFAULT_RECONCILE_POLICY, planTenant, pullKey, requeue, runKey, type ObservedRun, type OpenPr, type RepoPlan } from "./reconcile-plan.js";
44

55
const MINUTE = 60_000;
66
const T0 = new Date("2026-10-07T00:00:00.000Z");
@@ -20,9 +20,9 @@ function run(runId: string, status: ObservedRun["status"], startedMinute: number
2020

2121
const REPO = "acme/widgets";
2222

23-
function plan(prs: OpenPr[], rows: readonly PrTriageRow[], now: Date, runs: Array<[OpenPr, ObservedRun[]]> = [], maxInFlight = prs.length): RepoPlan {
23+
function plan(prs: OpenPr[], rows: readonly PrTriageRow[], now: Date, runs: Array<[OpenPr, ObservedRun[]]> = [], maxInFlight = prs.length, roomless: readonly number[] = []): RepoPlan {
2424
const byHead = new Map(runs.map(([open, observed]) => [runKey(open.number, open.headSha), observed]));
25-
const input = { repos: [{ name: REPO, prs, rows }], runs: new Map([[REPO, byHead]]), now, policy: { ...policy, maxInFlight } };
25+
const input = { repos: [{ name: REPO, prs, rows }], runs: new Map([[REPO, byHead]]), roomless: new Set(roomless.map((number) => pullKey(REPO, number))), now, policy: { ...policy, maxInFlight } };
2626
return planTenant(input).get(REPO)!;
2727
}
2828

@@ -217,7 +217,7 @@ describe("reconcile plan", () => {
217217
]);
218218
const rows = [row(1, { headSha: "sha1-old", status: "running", runId: "r_live", queuedAt: at(-6).toISOString() })];
219219
const prs = [pr(1, "sha1-new"), pr(2), pr(3), pr(4), pr(5), pr(6)];
220-
const plans = planTenant({ repos: [{ name: REPO, prs, rows }], runs, now: at(0), policy });
220+
const plans = planTenant({ repos: [{ name: REPO, prs, rows }], runs, roomless: new Set(), now: at(0), policy });
221221
expect(enqueued(plans.get(REPO)!)).toEqual([1, 2]);
222222
});
223223

@@ -272,6 +272,34 @@ describe("reconcile plan", () => {
272272
});
273273
});
274274

275+
describe("a head whose manager was refused for capacity", () => {
276+
test("stays queued past the never-started window and asks again every pass without spending an attempt, while another head's failures back off", () => {
277+
const heads = [pr(1), pr(2)];
278+
let rows = plan(heads, [], at(0)).rows;
279+
const runs: Array<[OpenPr, ObservedRun[]]> = [[pr(2), [run("run_a", "failed", 1)]]];
280+
const passes: Array<[number, number[], number]> = [[5, [1, 2], 2], [10, [1], 2], [15, [1, 2], 3], [20, [1], 3], [30, [1], 3]];
281+
for (const [minute, asked, failedAttempts] of passes) {
282+
if (minute === 10) runs[0]![1].push(run("run_b", "failed", 6));
283+
const pass = plan(heads, rows, at(minute), runs, 2, [1]);
284+
expect(enqueued(pass)).toEqual(asked);
285+
expect(pass.enqueue.find((entry) => entry.number === 1)?.reason).toBe(WAITING_FOR_CAPACITY);
286+
expect(status(pass, 1)).toMatchObject({ status: "queued", attempts: 1, error: WAITING_FOR_CAPACITY });
287+
expect(status(pass, 2)).toMatchObject({ attempts: failedAttempts });
288+
rows = pass.rows;
289+
}
290+
});
291+
292+
test("ages from the next pass as any queued head once a manager deployed after the refusal is not refused", () => {
293+
const queued = plan([pr(1)], [], at(0)).rows;
294+
const waiting = plan([pr(1)], queued, at(30), [], 1, [1]).rows;
295+
const placed = plan([pr(1)], waiting, at(31));
296+
expect(enqueued(placed)).toEqual([]);
297+
expect(status(placed, 1)).toEqual(expect.not.objectContaining({ error: expect.anything() }));
298+
expect(status(plan([pr(1)], placed.rows, at(41)), 1)).toMatchObject({ status: "queued", attempts: 1 });
299+
expect(status(plan([pr(1)], placed.rows, at(42)), 1)).toMatchObject({ status: "queued", attempts: 2 });
300+
});
301+
});
302+
275303
describe("triage again on a head that already has a verdict", () => {
276304
// The route's write on top of a triaged row: queued, queuedAt now, runId kept.
277305
const requeued: PrTriageRow = { number: 8, headSha: "sha8", status: "queued", attempts: 0, runId: "run-old", firstSeenAt: at(-120).toISOString(), queuedAt: at(-1).toISOString(), updatedAt: at(-1).toISOString() };

‎apps/hub/src/github/reconcile-plan.ts‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Decides, from what the host and the managers' runs say now, which open pull
22
// requests to queue again. Pure, so every recovery path is decided in one place.
3-
import { PR_TRIAGE_STUCK_RUN_MS, type AwaitedStage, type PrTriageRow, type RepoSource } from "@corbits/triage-contracts";
3+
import { PR_TRIAGE_STUCK_RUN_MS, WAITING_FOR_CAPACITY, type AwaitedStage, type PrTriageRow, type RepoSource } from "@corbits/triage-contracts";
44
import { MODEL_NOT_ASKED } from "@corbits/triage-workflows/logic/settled";
55

66
/** `labels` only where the host lists them. */
@@ -81,6 +81,8 @@ export type TenantPlanInput = {
8181
repos: readonly RepoPlanInput[];
8282
/** Every observed run, by repository, then by `${number}@${headSha}`, whether or not its repository or head is still here. */
8383
runs: ReadonlyMap<string, ReadonlyMap<string, readonly ObservedRun[]>>;
84+
/** By `pullKey`, the pull requests whose last manager every provisioner refused a placement for capacity. */
85+
roomless: ReadonlySet<string>;
8486
now: Date;
8587
policy: ReconcilePolicy;
8688
};
@@ -95,6 +97,10 @@ export function runKey(number: number, headSha: string): string {
9597
return `${number}@${headSha}`;
9698
}
9799

100+
export function pullKey(repo: string, number: number): string {
101+
return `${repo}#${number}`;
102+
}
103+
98104
export function runKeyNumber(key: string): number {
99105
return Number(key.slice(0, key.indexOf("@")));
100106
}
@@ -162,9 +168,11 @@ function sinceStale(row: PrTriageRow, runs: readonly ObservedRun[], policy: Reco
162168
* only sees runs since it was queued, so a head queued again on top of a
163169
* verdict stays queued until its new run starts. Runs from before the base
164170
* branch last moved settle nothing; one of them in flight holds the head
165-
* until it ends, and the head is stale then.
171+
* until it ends, and the head is stale then. A queued head whose last manager
172+
* was refused for capacity stays queued, never aging, until a manager deployed
173+
* since is not refused.
166174
*/
167-
function observe(row: PrTriageRow, observed: readonly ObservedRun[], now: Date, policy: ReconcilePolicy): PrTriageRow {
175+
function observe(row: PrTriageRow, observed: readonly ObservedRun[], roomless: boolean, now: Date, policy: ReconcilePolicy): PrTriageRow {
168176
const at = now.toISOString();
169177
const waiting = row.status === "queued" || row.status === "running";
170178
const current = sinceStale(row, observed, policy);
@@ -185,6 +193,12 @@ function observe(row: PrTriageRow, observed: readonly ObservedRun[], now: Date,
185193
const failed = withStatus(row, { status: "failed", run: ended, error: failure(ended, isStuck(ended, now, policy)) }, at);
186194
return row.status === "triaged" && ended.unsettled === MODEL_NOT_ASKED ? { ...failed, attempts: 0 } : failed;
187195
}
196+
if (row.status === "queued" && roomless) return withStatus(row, { status: "queued", error: WAITING_FOR_CAPACITY }, at);
197+
if (row.status === "queued" && row.error === WAITING_FOR_CAPACITY) {
198+
// A manager deployed since the refusal was not refused, so the head waits for it from now as any queued head does.
199+
const { error: _error, ...rest } = row;
200+
return { ...rest, queuedAt: at, updatedAt: at };
201+
}
188202
if (row.status === "queued" && row.queuedAt !== undefined && now.getTime() - ms(row.queuedAt) > policy.unstartedAfterMs) {
189203
return withStatus(row, { status: "failed", error: NEVER_STARTED }, at);
190204
}
@@ -208,6 +222,8 @@ function isDue(row: PrTriageRow, pr: OpenPr, runs: readonly ObservedRun[], now:
208222
if (row.status === "new") return now.getTime() - ms(pr.updatedAt) >= policy.webhookGraceMs;
209223
// Mail that reached no listener says nothing about the head, so it goes out again on the first pass a listener is up.
210224
if (row.status === "failed" && row.unrouted) return true;
225+
// A head waiting for capacity asks for a manager again on every pass; the hub's hold on a recent ask paces the deploys.
226+
if (row.status === "queued" && row.error === WAITING_FOR_CAPACITY) return true;
211227
if (row.status !== "failed" || row.attempts >= policy.maxAttempts) return false;
212228
const last = row.queuedAt === undefined ? -Infinity : ms(row.queuedAt);
213229
const verdict = runs.find((run) => run.runId === row.runId);
@@ -281,7 +297,7 @@ export function requeue(plan: RepoPlan, row: PrTriageRow, reason: string, now: D
281297
}
282298

283299
/** Queues due heads across the tenant's repositories, never-triaged ones first, while fewer than `maxInFlight` heads are queued or running, a manager waiting for CI aside. */
284-
export function planTenant({ repos, runs, now, policy }: TenantPlanInput): Map<string, RepoPlan> {
300+
export function planTenant({ repos, runs, roomless, now, policy }: TenantPlanInput): Map<string, RepoPlan> {
285301
const at = now.toISOString();
286302
const plans = new Map<string, RepoPlan>();
287303
const due: Due[] = [];
@@ -294,18 +310,22 @@ export function planTenant({ repos, runs, now, policy }: TenantPlanInput): Map<s
294310
const key = runKey(pr.number, pr.headSha);
295311
const current = fresh(pr, byHead.get(key), at);
296312
const runsOfHead = observed?.get(key) ?? [];
297-
const row = uncapped(observe(current, runsOfHead, now, policy), now, policy);
313+
const row = uncapped(observe(current, runsOfHead, roomless.has(pullKey(name, pr.number)), now, policy), now, policy);
298314
if (inFlight(row) && !parked(runsOfHead)) busy.add(`${name} ${key}`);
299315
if (isDue(row, pr, runsOfHead, now, policy)) due.push({ repo: name, index: next.length, row });
300316
next.push(row);
301317
}
302318
plans.set(name, { rows: next, enqueue: [] });
303319
}
304-
for (const { repo, index, row } of due.sort(byWait).slice(0, Math.max(policy.maxInFlight - busy.size, 0))) {
320+
// A head waiting for capacity already holds its slot, so it asks again whatever the others leave free.
321+
const holding = due.filter((entry) => inFlight(entry.row));
322+
const next = due.filter((entry) => !inFlight(entry.row)).sort(byWait).slice(0, Math.max(policy.maxInFlight - busy.size, 0));
323+
for (const { repo, index, row } of [...holding, ...next]) {
305324
const plan = plans.get(repo)!;
306-
const { runId: _run, error: _error, unrouted, ...rest } = row;
307-
const attempts = unrouted ? row.attempts : row.attempts + 1;
308-
plan.rows[index] = { ...rest, status: "queued", attempts, queuedAt: at, updatedAt: at };
325+
const { runId: _run, error, unrouted, ...rest } = row;
326+
const roomless = error === WAITING_FOR_CAPACITY;
327+
const attempts = unrouted || roomless ? row.attempts : row.attempts + 1;
328+
plan.rows[index] = { ...rest, status: "queued", attempts, queuedAt: at, updatedAt: at, ...(roomless && { error }) };
309329
const reason = row.error ?? "never triaged";
310330
const noWait = decideNow(attempts, policy);
311331
plan.enqueue.push({ number: row.number, reason, ...(noWait && { noWait }) });

0 commit comments

Comments
 (0)