trevato/forge

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

More
Forks1g fMapg aRuns11g rEventsFlows

A release is never held because main moved: the build accepts a green check of a descendant of the tagged commit #3968

Merged masoncrew opened this in answer to trevatoowner

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

ConversationFiles
Merged

mason/forge:mason/3966-a-release-is-never-held-because-main-mov → main 2 commits fork 2 threads · 2 unresolved

  • Merged. mason/3966-a-release-is-never-held-because-main-mov was merged into main as 49ccb1d.

Review

Sign in to review this change.
7 files changed +269 −17
README.md +1 −1
@@ -1915,7 +1915,7 @@ Below them the run page has a per-look web-vitals table: LCP, CLS and TBT (`webv
19151915
19161916 While the default branch's own last check is red, the forge holds its pulls' checks queued rather than fail each one on the same test (`src/checks/red_main.ts`): the vitals carry one card naming the failing test and how many checks wait, and main going green releases them. A check a person asks for again runs at once, and no check is held more than 20 minutes, so the pull that fixes main can always show green.
19171917
1918 `GET /api/v1/repos/:owner/:name/commits/:oid/check` answers the default branch's own check of one full commit id, `{commit, state, runId, summary, finishedAt}` with `state` `ok`, `failed`, `pending`, `superseded` (every check of it was cancelled, as when main moved on before it ran, and none will follow) or `none`, to whoever may read the repository's runs (`src/checks/commit_check.ts`). It is half of the forge's release gate: the release workflow builds only once it reads `ok` for the tagged commit, and the build boots the image it made (`docs/ops/ROLL.md`, "The gate of record").
1918 `GET /api/v1/repos/:owner/:name/commits/:oid/check` answers the default branch's check of one full commit id, `{commit, checkedCommit, state, runId, summary, finishedAt}` with `state` `ok`, `failed`, `pending`, `superseded` (every check of it was cancelled, as when main moved on before it ran, and none will follow) or `none`, to whoever may read the repository's runs (`src/checks/commit_check.ts`). When the commit's own check is `superseded` or `none`, the newest green (or else running) check of a later main commit that contains it answers instead, and `checkedCommit` names that commit: main moving on never holds a release (#3966). It is half of the forge's release gate: the release workflow builds only once it reads `ok` for the tagged commit, and the build boots the image it made (`docs/ops/ROLL.md`, "The gate of record").
19191919
19201920 A test file the shard gate clears because it passed alone is still a failure: when the default branch's own check log says `fails only beside others: <files> (shard-mates: <files>)`, the runner opens one issue per file (at most 5 per run, the rest named in the log as not filed) or comments on the one already open. The issue is authored as the repository owner and its body says it was filed by the check runner. A pull or other branch's check files nothing.
19211921
docs/ops/ROLL.md +6
@@ -156,6 +156,12 @@ A release ships on two proofs, and nothing else holds it:
156156 decides; a cancelled one is passed over when another check of the commit
157157 stands (a restart queues it again), and is `superseded` when none does:
158158 main moved on while it waited, and nothing will check that commit again.
159 Main moving does not hold a release (#3966): when the commit's own check
160 is `superseded` or `none`, the newest of main's checks of a later commit
161 that contains it (`git merge-base --is-ancestor`) answers instead — `ok`
162 when green, `pending` while running — since that full check ran the
163 tagged tree plus what landed after. A red later check is no verdict on
164 the tag and is passed over. `checkedCommit` names whose check answered.
159165 `release.yml`'s first gated step polls it every 30 s for up to two hours
160166 and builds only on `ok`; `failed` stops the release with the run's link,
161167 `superseded` stops it at once, and `none` stops it after ten minutes (a
src/actions/runs.ts +16 −6
@@ -8,7 +8,9 @@
88 * the system's own voice and takes no actor.
99 */
1010
11 import { commitCheck, isCommitId, type CommitCheck } from "../checks/commit_check.ts";
11 import { isCommitId, releaseCheck, type CommitCheck } from "../checks/commit_check.ts";
12 import { hasCommit, isAncestor } from "../git/local.ts";
13 import { repoDir } from "../git/repos.ts";
1214 import { mayViewId } from "../core/sealed.ts";
1315 import { requireVerified } from "./verify.ts";
1416 import { requireAbility } from "./roles.ts";
@@ -153,17 +155,25 @@ export function listRepoRuns(
153155 }
154156
155157 /**
156 * The default branch's own check of one commit (`checks/commit_check.ts`),
157 * for whoever may read the repository's runs: what a release waits on.
158 * The default branch's check that covers one commit (`checks/commit_check.ts`
159 * `releaseCheck`), for whoever may read the repository's runs: what a
160 * release waits on — the commit's own check, or a later main commit's that
161 * contains it when main moved on before the commit's own check ran (#3966).
158162 */
159 export function checkOfCommit(
163 export async function checkOfCommit(
160164 app: App,
161165 actor: User | undefined,
162166 repo: Repo,
163167 oid: string,
164 ): Result<CommitCheck, "not_found"> {
168 ): Promise<Result<CommitCheck, "not_found">> {
165169 if (!isCommitId(oid) || !mayReadRuns(app, actor, repo)) return err("not_found");
166 return ok(commitCheck(app.db, repo.id, repo.defaultBranch, oid));
170 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
171 return ok(
172 await releaseCheck(app.db, repo.id, repo.defaultBranch, oid, {
173 has: () => hasCommit(dir, oid),
174 contains: (descendant) => isAncestor(dir, oid, descendant),
175 }),
176 );
167177 }
168178
169179 /** A run by id, only if it belongs to a repository the actor can see. */
src/checks/commit_check.ts +90 −2
@@ -16,6 +16,14 @@
1616 * moved on while it waited (`superseded by <sha>`, `checks/service.ts`) or
1717 * a person stopped it — and the answer is `superseded`, never `none`, so a
1818 * release on that commit stops at once rather than waiting out its budget.
19 *
20 * {@link releaseCheck} is what a release reads (#3966): the tagged commit's
21 * own check when it has a verdict, and otherwise — main moved on and
22 * cancelled it, or the commit landed inside a batch that was checked only
23 * at its head — the newest of main's checks of a later commit that contains
24 * it. That check ran the full suite on the tagged tree plus what landed
25 * after, so its green covers the tag. `checkedCommit` names whose check
26 * answered.
1927 */
2028
2129 import type { Database } from "bun:sqlite";
@@ -27,6 +35,8 @@ export type CommitCheckState = "ok" | "failed" | "pending" | "superseded" | "non
2735
2836 export interface CommitCheck {
2937 readonly commit: string;
38 /** Whose check answered: `commit` itself, or a later main commit containing it. */
39 readonly checkedCommit: string;
3040 /**
3141 * `ok` passed, `failed` failed, `pending` queued or running, `superseded`
3242 * every check of it was cancelled and none will follow, `none` never checked.
@@ -59,12 +69,90 @@ export function commitCheck(
5969 ORDER BY state <> 'cancelled' DESC, id DESC LIMIT 1`,
6070 )
6171 .get(repoId, branch, oid, WINDOW);
62 if (row === null) return { commit: oid, state: "none", runId: null, summary: "", finishedAt: null };
72 if (row === null)
73 return { commit: oid, checkedCommit: oid, state: "none", runId: null, summary: "", finishedAt: null };
6374 const state: CommitCheckState =
6475 row.state === "ok" || row.state === "failed"
6576 ? row.state
6677 : row.state === "cancelled"
6778 ? "superseded"
6879 : "pending";
69 return { commit: oid, state, runId: row.id, summary: row.summary, finishedAt: row.finished_at };
80 return {
81 commit: oid,
82 checkedCommit: oid,
83 state,
84 runId: row.id,
85 summary: row.summary,
86 finishedAt: row.finished_at,
87 };
88 }
89
90 /** How many later commits main checked are asked whether they contain the tag. */
91 export const DESCENDANTS = 10;
92
93 /**
94 * The check a release on `oid` waits on. The commit's own `ok`, `failed` or
95 * `pending` answers as it is. When it is `superseded` or `none`, main's
96 * checks of later commits (newest first, at most {@link DESCENDANTS}
97 * distinct commits, each by its own newest non-cancelled check) are asked
98 * in turn: the first green one that `contains` the commit answers `ok`; a
99 * containing one still running answers `pending`, so the release waits for
100 * it. A red one is not a verdict on the tag and is passed over; when nothing
101 * green or running contains the commit the commit's own answer stands.
102 *
103 * `history.contains(descendant)` says whether `oid` is reachable from
104 * `descendant` (`git merge-base --is-ancestor`); `history.has()` says whether
105 * `oid` is a commit in the repository at all, and is asked once before any
106 * `contains`, so an oid the repository never saw costs one git process, not
107 * one per descendant (the endpoint is unauthenticated on a public repo).
108 * Both are passed in so this stays plain data.
109 */
110 export async function releaseCheck(
111 db: Database,
112 repoId: string,
113 branch: string,
114 oid: string,
115 history: { has: () => Promise<boolean>; contains: (descendant: string) => Promise<boolean> },
116 ): Promise<CommitCheck> {
117 const own = commitCheck(db, repoId, branch, oid);
118 if (own.state !== "superseded" && own.state !== "none") return own;
119 const rows = db
120 .query<
121 { id: string; commit_oid: string; state: string; summary: string; finished_at: string | null },
122 [string, string, string, number, string]
123 >(
124 `SELECT id, commit_oid, state, summary, finished_at
125 FROM (SELECT id, kind, issue_id, env, commit_oid, state, summary, finished_at
126 FROM run WHERE repo_id = ?1 ORDER BY id DESC LIMIT ?4)
127 WHERE kind = 'check' AND issue_id IS NULL AND env = ?2
128 AND commit_oid IS NOT NULL AND commit_oid NOT IN ('', ?3) AND state <> 'cancelled'
129 AND id > ?5
130 ORDER BY id DESC`,
131 )
132 .all(repoId, branch, oid, WINDOW, own.runId ?? "");
133 const seen = new Set<string>();
134 let waiting: CommitCheck | null = null;
135 let known: boolean | null = null;
136 for (const row of rows) {
137 if (seen.has(row.commit_oid)) continue;
sentinelcrew outdated

Medium: anyone can reach this unauthenticated on a public repo, and an oid that is not in the repo (none) still costs up to 50 git merge-base spawns per request. Before the loop, return own when oid is not a commit in the repo, and add a test where contains is never called for an unknown oid.
138 seen.add(row.commit_oid);
139 if (seen.size > DESCENDANTS) break;
140 if (row.state === "failed") continue;
141 const pending = row.state !== "ok";
142 if (pending && waiting !== null) continue;
143 known ??= await history.has();
144 if (!known) return own;
145 if (!(await history.contains(row.commit_oid))) continue;
146 const answer: CommitCheck = {
147 commit: oid,
148 checkedCommit: row.commit_oid,
149 state: pending ? "pending" : "ok",
150 runId: row.id,
151 summary: row.summary,
152 finishedAt: row.finished_at,
153 };
154 if (!pending) return answer;
155 waiting = answer;
156 }
157 return waiting ?? own;
70158 }
src/git/local.ts +7
@@ -861,6 +861,13 @@ export async function isAncestor(
861861 return out.ok;
862862 }
863863
864 /** Is `oid` a commit object in the repository? One `git cat-file -e`. */
865 export async function hasCommit(dir: string, oid: string): Promise<boolean> {
866 if (!isSafeRef(oid)) return false;
867 const out = await runLocal(["cat-file", "-e", `${oid}^{commit}`], { cwd: dir });
868 return out.ok;
869 }
870
864871 /** `rev-list --left-right --count a...b` — what each side has alone. */
865872 export async function aheadBehind(
866873 dir: string,
src/http/api.ts +4 −4
@@ -2740,12 +2740,12 @@ export function apiRoutes(router: Router<Handler>): void {
27402740 });
27412741 });
27422742
2743 // The default branch's own check of a full commit id: what a release
2744 // waits on before it builds (#2794). Never cached: `pending` turns.
2745 router.add("GET", "/api/v1/repos/:owner/:name/commits/:oid/check", (ctx) => {
2743 // The default branch's check covering a full commit id: what a release
2744 // waits on before it builds (#2794, #3966). Never cached: `pending` turns.
2745 router.add("GET", "/api/v1/repos/:owner/:name/commits/:oid/check", async (ctx) => {
27462746 const repo = loadRepo(ctx);
27472747 if (!repo) return notFound();
2748 const found = checkOfCommit(ctx.app, ctx.auth.user, repo, ctx.params.oid ?? "");
2748 const found = await checkOfCommit(ctx.app, ctx.auth.user, repo, ctx.params.oid ?? "");
27492749 if (!found.ok) return notFound();
27502750 return json(found.value, 200, { "cache-control": "no-store" });
27512751 });
test/commit-check.test.ts +145 −4
@@ -3,18 +3,23 @@
33 * (#2794): only main's own check counts (never a pull's fast tier, never
44 * another branch), the newest decides, a cancelled check says nothing, and
55 * the API answers it by full commit id to whoever may read the runs.
6 *
7 * A release is never held because main moved (#3966): when the tagged
8 * commit's own check was cancelled, or main never checked it alone, a green
9 * check of a later main commit that contains it answers for it.
610 */
7 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
11 import { afterAll, beforeAll, beforeEach, describe, expect, test } from "bun:test";
812 import type { Database } from "bun:sqlite";
913 import { mkdtemp, rm } from "node:fs/promises";
1014 import { tmpdir } from "node:os";
1115 import { join } from "node:path";
1216 import { createApp, type App } from "../src/app.ts";
1317 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
14 import { commitCheck } from "../src/checks/commit_check.ts";
18 import { commitCheck, DESCENDANTS, releaseCheck } from "../src/checks/commit_check.ts";
1519 import { migrations } from "../src/core/schema.ts";
1620 import { cancelRun, claimRun, createRun, finishRun, requeueCheck } from "../src/core/runs.ts";
17 import { createUser } from "../src/core/users.ts";
21 import { createUser, type User } from "../src/core/users.ts";
22 import type { Repo } from "../src/core/repos.ts";
1823 import { forgeHttp, type ForgeHttp } from "../src/http/server.ts";
1924 import { openDb } from "../src/kernel/db.ts";
2025 import { quietLogs } from "./helpers/globals.ts";
@@ -28,7 +33,7 @@ function check(db: Database, repoId: string, commit: string, opts: { env?: strin
2833 repoId,
2934 kind: "check",
3035 env: opts.env ?? "main",
31 commitOid: OID(commit),
36 commitOid: commit.length === 40 ? commit : OID(commit),
3237 reason: "push",
3338 ...(opts.issueId === undefined ? {} : { issueId: opts.issueId }),
3439 });
@@ -52,6 +57,7 @@ describe("commitCheck", () => {
5257 test("a commit main never checked is none", () => {
5358 expect(commitCheck(db, "r1", "main", OID("a"))).toEqual({
5459 commit: OID("a"),
60 checkedCommit: OID("a"),
5561 state: "none",
5662 runId: null,
5763 summary: "",
@@ -109,10 +115,105 @@ describe("commitCheck", () => {
109115 });
110116 });
111117
118 describe("releaseCheck", () => {
119 let db: Database;
120 beforeEach(() => {
121 db = openDb(":memory:", migrations);
122 db.run("INSERT INTO user (id, username, email, password_hash) VALUES ('u1', 'trevato', 't@x', '')");
123 db.run("INSERT INTO repo (id, owner_id, name) VALUES ('r1', 'u1', 'forge')");
124 });
125 /** `t` (the tag) is in every commit but `x`, which is on no line with it. */
126 const contains = async (descendant: string) => descendant !== OID("x");
127 const has = async () => true;
128 const asked = (oid: string) => releaseCheck(db, "r1", "main", OID(oid), { has, contains });
129
130 test("the tagged commit's own verdict answers as it is", async () => {
131 const own = check(db, "r1", "t");
132 finish(db, check(db, "r1", "u").id, "ok");
133 expect(await asked("t")).toMatchObject({ state: "pending", checkedCommit: OID("t"), runId: own.id });
134 claimRun(db, own.id);
135 finishRun(db, own.id, { state: "failed", summary: "check failed: 1 test failed" });
136 expect(await asked("t")).toMatchObject({ state: "failed", checkedCommit: OID("t") });
137 });
138
139 test("main moved on twice: the tag's cancelled check is answered by the descendant's green one", async () => {
140 const tagged = check(db, "r1", "t");
141 cancelRun(db, tagged.id, `superseded by ${OID("u").slice(0, 7)}`);
142 const first = check(db, "r1", "u");
143 cancelRun(db, first.id, `superseded by ${OID("v").slice(0, 7)}`);
144 const second = check(db, "r1", "v");
145 expect(await asked("t")).toMatchObject({ commit: OID("t"), checkedCommit: OID("v"), state: "pending", runId: second.id });
146 finish(db, second.id, "ok");
147 expect(await asked("t")).toMatchObject({
148 commit: OID("t"),
149 checkedCommit: OID("v"),
150 state: "ok",
151 runId: second.id,
152 summary: "check passed in 368s",
153 });
154 });
155
156 test("a commit main never checked alone, inside a checked batch, is answered by the batch's check", async () => {
157 finish(db, check(db, "r1", "v").id, "ok");
158 expect(await asked("t")).toMatchObject({ state: "ok", checkedCommit: OID("v") });
159 });
160
161 test("a later commit that does not contain the tag, or a red one, does not answer for it", async () => {
162 const tagged = check(db, "r1", "t");
163 cancelRun(db, tagged.id, "superseded by xxxxxxx");
164 finish(db, check(db, "r1", "x").id, "ok");
165 expect(await asked("t")).toMatchObject({ state: "superseded", checkedCommit: OID("t"), runId: tagged.id });
166 finish(db, check(db, "r1", "v").id, "failed");
167 expect((await asked("t")).state).toBe("superseded");
168 expect((await asked("y")).state).toBe("none");
169 });
170
171 test("a green check older than the tag's own is never read as covering it", async () => {
172 finish(db, check(db, "r1", "v").id, "ok");
173 const tagged = check(db, "r1", "t");
174 cancelRun(db, tagged.id, "stopped");
175 expect((await asked("t")).state).toBe("superseded");
176 });
177
178 test("the newest green descendant answers before an older one still running", async () => {
179 cancelRun(db, check(db, "r1", "t").id, "superseded");
180 check(db, "r1", "u");
181 const green = check(db, "r1", "v");
182 finish(db, green.id, "ok");
183 expect(await asked("t")).toMatchObject({ state: "ok", runId: green.id });
184 });
185
186 test("an oid the repository does not have answers none without asking any descendant", async () => {
187 for (const c of "uvw") finish(db, check(db, "r1", c).id, "ok");
188 let hasCalls = 0;
189 let containsCalls = 0;
190 const answer = await releaseCheck(db, "r1", "main", OID("z"), {
191 has: async () => (hasCalls++, false),
192 contains: async () => (containsCalls++, true),
193 });
194 expect(answer.state).toBe("none");
195 expect(hasCalls).toBe(1);
196 expect(containsCalls).toBe(0);
197 });
198
199 test("at most DESCENDANTS distinct later commits are asked", async () => {
200 for (let i = 0; i < DESCENDANTS + 5; i++) finish(db, check(db, "r1", i.toString(16).padStart(40, "0")).id, "ok");
201 let containsCalls = 0;
202 const answer = await releaseCheck(db, "r1", "main", OID("z"), {
203 has: async () => true,
204 contains: async () => (containsCalls++, false),
205 });
206 expect(answer.state).toBe("none");
207 expect(containsCalls).toBe(DESCENDANTS);
208 });
209 });
210
112211 describe("GET /api/v1/repos/:owner/:name/commits/:oid/check", () => {
113212 let dataDir: string;
114213 let app: App;
115214 let http: ForgeHttp;
215 let alice: User;
216 let forge: Repo;
116217
117218 const get = async (path: string) => {
118219 const res = await http.handle(new Request(`http://forge.test${path}`));
@@ -132,9 +233,11 @@ describe("GET /api/v1/repos/:owner/:name/commits/:oid/check", () => {
132233 adminIfFirst: true,
133234 });
134235 if (!made.ok) throw new Error(String(made.error));
236 alice = made.value;
135237 for (const [name, visibility] of [["forge", "public"], ["hidden", "private"]] as const) {
136238 const repo = await createRepo(app, made.value, { name, visibility });
137239 if (!repo.ok) throw new Error(String(repo.error));
240 if (name === "forge") forge = repo.value;
138241 await commitToBranch(app, made.value, repo.value, {
139242 message: "Start",
140243 files: [{ path: "README.md", content: `# ${name}\n` }],
@@ -162,4 +265,42 @@ describe("GET /api/v1/repos/:owner/:name/commits/:oid/check", () => {
162265 expect((await get(`/api/v1/repos/alice/forge/commits/${"E".repeat(40)}/check`)).status).toBe(404);
163266 expect((await get(`/api/v1/repos/alice/hidden/commits/${OID("e")}/check`)).status).toBe(404);
164267 });
268
269 test("a tag whose check two landings cancelled builds on the descendant's green check, never on a stranger's", async () => {
270 const repo = forge;
271 const land = async (path: string, branch = "main") => {
272 const done = await commitToBranch(app, alice, repo, {
273 message: `Land ${path}`,
274 branch,
275 files: [{ path, content: `${path}\n` }],
276 });
277 if (!done.ok) throw new Error(String(done.error));
278 return done.value.commitOid;
279 };
280 const tag = await land("release.txt");
281 const tagged = check(app.db, repo.id, tag);
282 const one = await land("one.txt");
283 cancelRun(app.db, tagged.id, `superseded by ${one.slice(0, 7)}`);
284 const first = check(app.db, repo.id, one);
285 const two = await land("two.txt");
286 cancelRun(app.db, first.id, `superseded by ${two.slice(0, 7)}`);
287 const second = check(app.db, repo.id, two);
288 const read = async () => JSON.parse((await get(`/api/v1/repos/alice/forge/commits/${tag}/check`)).body);
289 expect(await read()).toMatchObject({ commit: tag, checkedCommit: two, state: "pending" });
290 finish(app.db, second.id, "ok");
291 expect(await read()).toMatchObject({ commit: tag, checkedCommit: two, state: "ok", runId: second.id });
292
293 // A green check of a commit on another line does not contain the tag.
294 const side = await land("side.txt", "side");
295 const lone = check(app.db, repo.id, side);
296 finish(app.db, lone.id, "ok");
297 const sideTag = await land("side-two.txt", "side");
298 const sideCheck = check(app.db, repo.id, sideTag);
299 cancelRun(app.db, sideCheck.id, "superseded");
300 finish(app.db, check(app.db, repo.id, await land("three.txt")).id, "ok");
301 expect(JSON.parse((await get(`/api/v1/repos/alice/forge/commits/${sideTag}/check`)).body)).toMatchObject({
302 checkedCommit: sideTag,
303 state: "superseded",
304 });
305 });
165306 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs