Public: anyone can read this; the crew can change it.
Pulls closed as already on the base before 4e481725 stop counting as landed in the vitals and home KPIs #8759
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/8738-pulls-closed-as-already-on-the-base-befo → main 1 commit fork
4 files
changed +72
−2
src/core/migration-pins/20261102200000-pull-carried-backfill.pin added +1
| @@ -0,0 +1,1 @@ | ||
| 1 | da6aee0f22805c087d9579a51df785eeed885565dbf3880bb328e5ad04aea96c |
src/core/migrations/20261102200000-pull-carried-backfill.ts added +11
| @@ -0,0 +1,11 @@ | ||
| 1 | // The pulls closed as carried before `pull.carried` existed (#8738): a carried | |
| 2 | // close recorded the base it found as its merged commit (`closeAsMerged` in | |
| 3 | // `core/pulls.ts`), which no real landing does — a merge or squash writes a | |
| 4 | // new commit, a fast-forward the head. Imported pulls have no merged commit. | |
| 5 | // A carried close onto an earlier landing of the same pull is not matched: | |
| 6 | // that commit did land once, and the home figures count it once. | |
| 7 | export default ` | |
| 8 | UPDATE pull SET carried = 1 | |
| 9 | WHERE carried = 0 AND merged_at IS NOT NULL | |
| 10 | AND merged_oid IS NOT NULL AND merged_oid != '' AND merged_oid = base_oid; | |
| 11 | `; |
src/flow/vitals.ts +2 −2
| @@ -77,9 +77,9 @@ export function readVitals(db: Database, now = new Date(), publicOnly = false): | ||
| 77 | 77 | (SELECT COUNT(*) FROM issue i JOIN pull p ON p.issue_id = i.id JOIN repo r ON r.id = i.repo_id |
| 78 | 78 | WHERE (?4 = 0 OR r.visibility = 'public') AND i.kind = 'pull' AND i.created_at >= ?2 AND i.created_at <= ?3) AS opened6h, |
| 79 | 79 | (SELECT COUNT(*) FROM pull p JOIN issue i ON i.id = p.issue_id JOIN repo r ON r.id = i.repo_id |
| 80 | WHERE (?4 = 0 OR r.visibility = 'public') AND p.base_branch = r.default_branch AND p.merged_at >= ?1 AND p.merged_at <= ?3) AS landed1h, | |
| 80 | WHERE (?4 = 0 OR r.visibility = 'public') AND p.base_branch = r.default_branch AND p.carried = 0 AND p.merged_at >= ?1 AND p.merged_at <= ?3) AS landed1h, | |
| 81 | 81 | (SELECT COUNT(*) FROM pull p JOIN issue i ON i.id = p.issue_id JOIN repo r ON r.id = i.repo_id |
| 82 | WHERE (?4 = 0 OR r.visibility = 'public') AND p.base_branch = r.default_branch AND p.merged_at >= ?2 AND p.merged_at <= ?3) AS landed6h`, | |
| 82 | WHERE (?4 = 0 OR r.visibility = 'public') AND p.base_branch = r.default_branch AND p.carried = 0 AND p.merged_at >= ?2 AND p.merged_at <= ?3) AS landed6h`, | |
| 83 | 83 | ) |
| 84 | 84 | .get(h1, h6, to, publicOnly ? 1 : 0); |
| 85 | 85 | const pile = readChecksPile(db, publicOnly); |
test/flow-carried-backfill.test.ts added +58
| @@ -0,0 +1,58 @@ | ||
| 1 | /** Pulls closed as carried before `pull.carried` existed (#8738) are marked by the backfill, and the vitals and home figures leave them out. */ | |
| 2 | import { afterAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtempSync, rmSync } from "node:fs"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { migrationFiles } from "../src/core/schema.ts"; | |
| 7 | import { openDb } from "../src/kernel/db.ts"; | |
| 8 | import { readKpis } from "../src/flow/kpis.ts"; | |
| 9 | import { readVitals } from "../src/flow/vitals.ts"; | |
| 10 | ||
| 11 | const BACKFILL = "20261102200000-pull-carried-backfill"; | |
| 12 | const dir = mkdtempSync(join(tmpdir(), "carried-backfill-")); | |
| 13 | afterAll(() => rmSync(dir, { recursive: true, force: true })); | |
| 14 | ||
| 15 | const MIN = 60_000; | |
| 16 | ||
| 17 | test("the backfill marks the old carried closes, and only the real landings count", () => { | |
| 18 | const at = migrationFiles.findIndex((m) => m.name === BACKFILL); | |
| 19 | expect(at).toBeGreaterThan(0); | |
| 20 | const path = join(dir, "forge.db"); | |
| 21 | const before = openDb(path, migrationFiles.slice(0, at)); | |
| 22 | before.run("INSERT INTO user (id, username, email, password_hash) VALUES ('u', 'u', 'u@x', 'x')"); | |
| 23 | before.run("INSERT INTO repo (id, owner_id, name, visibility) VALUES ('r', 'u', 'r', 'public')"); | |
| 24 | const branch = before.query<{ b: string }, []>("SELECT default_branch AS b FROM repo WHERE id = 'r'").get()!.b; | |
| 25 | const now = Date.now(); | |
| 26 | let n = 0; | |
| 27 | const pull = (mergedOid: string | null, baseOid: string, ago: number | null): void => { | |
| 28 | n++; | |
| 29 | before.run( | |
| 30 | "INSERT INTO issue (id, repo_id, number, kind, title, author_id, state) VALUES (?1, 'r', ?2, 'pull', 'p', 'u', 'closed')", | |
| 31 | [`p${n}`, n], | |
| 32 | ); | |
| 33 | before.run( | |
| 34 | `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, merged_at, merged_oid) | |
| 35 | VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)`, | |
| 36 | [`p${n}`, branch, `b${n}`, baseOid, `head${n}`, ago === null ? null : new Date(now - ago).toISOString(), mergedOid], | |
| 37 | ); | |
| 38 | }; | |
| 39 | // Two real landings: each wrote its own commit on the base it found. | |
| 40 | pull("land1", "base1", 10 * MIN); | |
| 41 | pull("land2", "base2", 20 * MIN); | |
| 42 | // Three carried closes, as `closeAsMerged` recorded them before 4e481725: the base it found as the merged commit. | |
| 43 | for (let i = 0; i < 3; i++) pull(`tip${i}`, `tip${i}`, (30 + i) * MIN); | |
| 44 | // An imported merged pull has no merged commit; an unmerged pull none either. Neither is touched. | |
| 45 | pull(null, "", 120 * MIN); | |
| 46 | pull(null, "base9", null); | |
| 47 | before.close(); | |
| 48 | ||
| 49 | const db = openDb(path, migrationFiles); | |
| 50 | const marked = db.query<{ id: string }, []>("SELECT issue_id AS id FROM pull WHERE carried = 1 ORDER BY issue_id").all(); | |
| 51 | expect(marked.map((r) => r.id)).toEqual(["p3", "p4", "p5"]); | |
| 52 | const vitals = readVitals(db, new Date(now)); | |
| 53 | expect(vitals.landed1h).toBe(2); | |
| 54 | // The import counts in six hours, as it did before: it is not a carried close. | |
| 55 | expect(vitals.landed6h).toBe(3); | |
| 56 | expect(readKpis(db, new Date(now)).landed).toBe(2); | |
| 57 | db.close(); | |
| 58 | }); |
new above
offline