Public: anyone can read this; the crew can change it.
Sentinel: queue:check is past its budget: 11 checks wait behind one lane; the oldest since 01:04; the lane runs #4656 for 5 min (limit 60). #4849
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/4784-sentinel-queue-check-is-past-its-budget → main 1 commit fork 1 thread · 1 unresolved
3 files
changed +148
−2
src/crew/conflict_room.ts +5
| @@ -15,6 +15,11 @@ | ||
| 15 | 15 | * run in flight is always allowed, so a busy lane slows the sweep but |
| 16 | 16 | * never stops it. A pull left out waits for a later tick, and if main |
| 17 | 17 | * moves meanwhile, one merge brings it past both landings. |
| 18 | * | |
| 19 | * The finishing pass (`finishing.ts`) asks the same room for approved | |
| 20 | * pulls: it queues before the sweep, and the sweep counts its runs as in | |
| 21 | * flight, so between them a tick never starts more than the lane holds | |
| 22 | * (#4784). | |
| 18 | 23 | */ |
| 19 | 24 | |
| 20 | 25 | import type { Database } from "bun:sqlite"; |
src/crew/finishing.ts +13 −2
| @@ -12,6 +12,10 @@ | ||
| 12 | 12 | * base, resolve keeping both intents, re-run the check) is queued for |
| 13 | 13 | * its builder — the agent on it, else its author, else any builder — |
| 14 | 14 | * and it takes that slot. |
| 15 | * - Its push is a check, so it also waits for room in the check lane, as | |
| 16 | * the conflict sweep's runs do (`conflict_room.ts`): a landing that | |
| 17 | * conflicts ten approved pulls at once must not put ten checks in the | |
| 18 | * lane together (#4784). | |
| 15 | 19 | * - After two such runs ended without a push (the head still carries the |
| 16 | 20 | * approval, so neither pushed), it is handed to a person: assigned to |
| 17 | 21 | * whoever turned autonomy on, so it is on their review list, with the |
| @@ -36,6 +40,7 @@ import { activeRunFor } from "../core/runs.ts"; | ||
| 36 | 40 | import { getUserById } from "../core/users.ts"; |
| 37 | 41 | import { mergeable, resolveOid } from "../git/merge.ts"; |
| 38 | 42 | import { createLog } from "../kernel/log.ts"; |
| 43 | import { readConflictRoom } from "./conflict_room.ts"; | |
| 39 | 44 | import { CHECK_FAILED_LIMIT, lastHumanWord, queueAgentRun } from "./dispatch.ts"; |
| 40 | 45 | import { HANDOVER_NOTE } from "./guard.ts"; |
| 41 | 46 | import { pickBuilder } from "./route.ts"; |
| @@ -101,6 +106,7 @@ export async function finishApproved(app: App, repo: Repo, free: number): Promis | ||
| 101 | 106 | const ids = new Set(open.map((p) => p.id)); |
| 102 | 107 | for (const id of read.keys()) if (!ids.has(id)) read.delete(id); |
| 103 | 108 | let slots = free; |
| 109 | let room = readConflictRoom(app.db); | |
| 104 | 110 | for (const pull of open.sort((a, b) => a.number - b.number)) { |
| 105 | 111 | if (!pull.authorIsAgent || pull.mergedAt !== null) continue; |
| 106 | 112 | if (getHold(app.db, pull.id) !== undefined) continue; |
| @@ -109,8 +115,11 @@ export async function finishApproved(app: App, repo: Repo, free: number): Promis | ||
| 109 | 115 | if (files === undefined) continue; |
| 110 | 116 | const issue = getIssueById(app.db, pull.id); |
| 111 | 117 | if (!issue || issue.state !== "open") continue; |
| 112 | const move = finishOne(app, repo, pull, issue, files, builders, slots); | |
| 113 | if (move.did === "queued update") slots--; | |
| 118 | const move = finishOne(app, repo, pull, issue, files, builders, slots, room); | |
| 119 | if (move.did === "queued update") { | |
| 120 | slots--; | |
| 121 | room--; | |
| 122 | } | |
| 114 | 123 | moves.push(move); |
| 115 | 124 | say(repo, pull, move); |
| 116 | 125 | } |
| @@ -125,6 +134,7 @@ function finishOne( | ||
| 125 | 134 | files: readonly string[], |
| 126 | 135 | builders: readonly Agent[], |
| 127 | 136 | slots: number, |
| 137 | room: number, | |
| 128 | 138 | ): FinishMove { |
| 129 | 139 | const not = (why: string): FinishMove => ({ pull: pull.number, did: "not moved", why }); |
| 130 | 140 | if (activeRunFor(app.db, "agent", pull.id) !== undefined) return { pull: pull.number, did: "in flight" }; |
| @@ -138,6 +148,7 @@ function finishOne( | ||
| 138 | 148 | if (builder === undefined) return not("no builder on the crew"); |
| 139 | 149 | if (failedUpdates(app, issue, builder) >= CHECK_FAILED_LIMIT) return handOver(app, repo, pull, issue, files); |
| 140 | 150 | if (slots <= 0) return not("waits for a builder slot"); |
| 151 | if (room <= 0) return not("waits for room in the check lane"); | |
| 141 | 152 | const run = queueAgentRun(app, repo, issue, builder, "conflict", undefined, { |
| 142 | 153 | notBefore: new Date().toISOString(), |
| 143 | 154 | attempt: 0, |
test/finishing-room.test.ts added +130
| @@ -0,0 +1,130 @@ | ||
| 1 | /** | |
| 2 | * The finishing pass asks the check lane for room, as the conflict sweep | |
| 3 | * does (#4784): a landing that conflicts several approved pulls at once | |
| 4 | * queues no more bring-up-to-date runs — each a push, each a check — than | |
| 5 | * the lane has slots for, however many builder slots are free. | |
| 6 | */ | |
| 7 | import { afterAll, beforeAll, expect, spyOn, 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 { createApp, type App } from "../src/app.ts"; | |
| 12 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; | |
| 13 | import { openPull } from "../src/actions/pulls.ts"; | |
| 14 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 15 | import { setCrewRole } from "../src/core/agents.ts"; | |
| 16 | import { setCapacity } from "../src/core/capacity.ts"; | |
| 17 | import type { Pull } from "../src/core/pulls.ts"; | |
| 18 | import type { Repo } from "../src/core/repos.ts"; | |
| 19 | import { submitReview } from "../src/core/reviews.ts"; | |
| 20 | import { createRun, listRuns } from "../src/core/runs.ts"; | |
| 21 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; | |
| 22 | import { autonomyTick } from "../src/crew/autonomy.ts"; | |
| 23 | ||
| 24 | let dataDir: string; | |
| 25 | let app: App; | |
| 26 | let alice: User; | |
| 27 | let repo: Repo; | |
| 28 | const pulls: Pull[] = []; | |
| 29 | const conflicts = () => | |
| 30 | listRuns(app.db, repo.id, { kind: "agent" }).filter( | |
| 31 | (r) => r.reason === "conflict" && pulls.some((p) => p.id === r.issueId), | |
| 32 | ); | |
| 33 | ||
| 34 | async function tick(): Promise<void> { | |
| 35 | const spy = spyOn(console, "info").mockImplementation(() => {}); | |
| 36 | try { | |
| 37 | await autonomyTick(app, new Date(), async () => 0); | |
| 38 | } finally { | |
| 39 | spy.mockRestore(); | |
| 40 | } | |
| 41 | } | |
| 42 | ||
| 43 | beforeAll(async () => { | |
| 44 | dataDir = await mkdtemp(join(tmpdir(), "forge-finishing-room-")); | |
| 45 | app = createApp({ dataDir, port: 0 }); | |
| 46 | const a = await createUser(app.db, { | |
| 47 | origin: "seed", | |
| 48 | trusted: true, | |
| 49 | username: "alice", | |
| 50 | email: "[email protected]", | |
| 51 | password: "a-good-passphrase", | |
| 52 | adminIfFirst: true, | |
| 53 | }); | |
| 54 | if (!a.ok) throw new Error(a.error); | |
| 55 | alice = a.value; | |
| 56 | const made = await createRepo(app, alice, { name: "engine", visibility: "private" }); | |
| 57 | if (!made.ok) throw new Error(String(made.error)); | |
| 58 | repo = made.value; | |
| 59 | const seeded = await commitToBranch(app, alice, repo, { | |
| 60 | message: "Start", | |
| 61 | files: [{ path: "shared.ts", content: "export const shared = 0;\n" }], | |
| 62 | }); | |
| 63 | if (!seeded.ok) throw new Error(JSON.stringify(seeded.error)); | |
| 64 | for (const name of ["mason", "quill"]) { | |
| 65 | const agent = await createAgent(app, alice, { username: name }); | |
| 66 | if (!agent.ok) throw new Error(String(agent.error)); | |
| 67 | expect(addToCrew(app, alice, repo, name).ok).toBe(true); | |
| 68 | } | |
| 69 | const mason = getUserByName(app.db, "mason")!; | |
| 70 | const quill = getUserByName(app.db, "quill")!; | |
| 71 | setCrewRole(app.db, repo.id, quill.id, "reviewer"); | |
| 72 | ||
| 73 | // Three approved pulls that all touch the file main is about to change. | |
| 74 | for (const n of [1, 2, 3]) { | |
| 75 | const branch = await commitToBranch(app, alice, repo, { | |
| 76 | branch: `feature/${n}`, | |
| 77 | base: "main", | |
| 78 | message: `Shared, branch ${n}'s way`, | |
| 79 | files: [{ path: "shared.ts", content: `export const shared = ${n};\n` }], | |
| 80 | }); | |
| 81 | if (!branch.ok) throw new Error(JSON.stringify(branch.error)); | |
| 82 | const opened = await openPull(app, mason, repo, { | |
| 83 | title: `Shared the way of ${n}`, | |
| 84 | body: "", | |
| 85 | baseBranch: "main", | |
| 86 | headBranch: `feature/${n}`, | |
| 87 | }); | |
| 88 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 89 | pulls.push(opened.value); | |
| 90 | const approved = submitReview(app.db, { | |
| 91 | issueId: opened.value.id, | |
| 92 | authorId: quill.id, | |
| 93 | state: "approved", | |
| 94 | commitOid: opened.value.headOid, | |
| 95 | }); | |
| 96 | if (!approved.ok) throw new Error(approved.error); | |
| 97 | } | |
| 98 | const moved = await commitToBranch(app, alice, repo, { | |
| 99 | branch: "main", | |
| 100 | message: "Shared, main's way", | |
| 101 | files: [{ path: "shared.ts", content: "export const shared = 'main';\n" }], | |
| 102 | }); | |
| 103 | if (!moved.ok) throw new Error(JSON.stringify(moved.error)); | |
| 104 | await Bun.sleep(5); | |
| 105 | // Builder slots for all three: the lane, not the crew, is what limits. | |
| 106 | app.db | |
| 107 | .query("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?2, autonomy_slots = 3 WHERE id = ?1") | |
| 108 | .run(repo.id, alice.id); | |
| 109 | }); | |
| 110 | ||
| 111 | afterAll(async () => { | |
| 112 | app.close(); | |
| 113 | await rm(dataDir, { recursive: true, force: true }); | |
| 114 | }); | |
| 115 | ||
| 116 | test("a full lane takes one approved pull's update, not one per free builder slot", async () => { | |
| 117 | setCapacity(app.db, "checks.concurrency", "1", alice.id); | |
| 118 | createRun(app.db, { repoId: repo.id, kind: "check", env: "main", reason: "push" }); | |
| 119 | await tick(); | |
| 120 | expect(conflicts()).toHaveLength(1); | |
| 121 | // That one is in flight and the lane is still full: nothing more. | |
| 122 | await tick(); | |
| 123 | expect(conflicts()).toHaveLength(1); | |
| 124 | }); | |
| 125 | ||
| 126 | test("once the lane has slots, the rest are brought up to date", async () => { | |
| 127 | setCapacity(app.db, "checks.concurrency", "8", alice.id); | |
| 128 | await tick(); | |
| 129 | expect(new Set(conflicts().map((r) => r.issueId))).toEqual(new Set(pulls.map((p) => p.id))); | |
| 130 | }); |
new above
offline
Nit: the move reason is a free string and shows up in logs. If the capacity page ever lists it, word it for a person, for example "waiting for a free check slot".