From 581de0bed4829d72774dc1797c0101658fba9bf3 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 18:17:58 +0000 Subject: [PATCH] ci-audit: lesson 1 names a job held back on its own label; planted repo runs PR jobs on Ubicloud Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/ci-audit/lessons.ts | 8 +++++++- test/ci-audit.test.ts | 24 ++++++++++++++++++++++-- test/helpers/ci-audit-planted.ts | 21 +++++++++++---------- 3 files changed, 40 insertions(+), 13 deletions(-) diff --git a/lib/ci-audit/lessons.ts b/lib/ci-audit/lessons.ts index 94b467217..839a554f7 100644 --- a/lib/ci-audit/lessons.ts +++ b/lib/ci-audit/lessons.ts @@ -36,8 +36,14 @@ export function lessonRunners(c: LessonContext): Lesson { const slow = priced.filter(({ g }) => (g.wait.p90 ?? 0) > 60 && (g.wait.p90 ?? 0) > (g.run.p50 ?? 0)); if (!slow.length) return mk(1, 'pass', `every job's p90 wait is under a minute or below its own running time`, { evidence: groups.slice(0, 3).map(g => `${g.key}: wait p50 ${fmt(g.wait.p50)}, p90 ${fmt(g.wait.p90)} (n=${g.n})`) }); const burst = isBurst(slow.flatMap(({ g }) => g.longWaits)); + const fastSibling = (g: typeof groups[number]) => groups.find(o => o !== g && o.labels === g.labels && (o.wait.p90 ?? 0) <= 60); + const pinned = slow.filter(({ g }) => fastSibling(g)); const hosted = slow.filter(({ p }) => p.kind === 'github-hosted'); const evidence = slow.slice(0, 5).map(({ g, p }) => `${g.key} on ${p.label}: wait p50 ${fmt(g.wait.p50)}, p90 ${fmt(g.wait.p90)} for ${fmt(g.run.p50)} of work; ${g.longWaits.length} of ${g.n} waited over a minute`); + if (pinned.length === slow.length && !burst) { + const { g } = pinned[0]!; + return mk(1, 'finding', `${g.key} waits for a machine (p90 ${fmt(g.wait.p90)}) while ${fastSibling(g)!.key} on the same runner label starts in seconds`, { evidence, fix: `look for what holds only this job back: a concurrency group, a required or reusable workflow, or a runner-group limit on ${g.labels}; a faster runner would not help` }); + } if (burst) return mk(1, 'finding', 'long waits cluster in a few hours: the repo starts more jobs at once than its runner pool takes', { evidence, fix: 'cap concurrency (concurrency groups with cancel-in-progress, fewer parallel jobs during bursts) before changing runners' }); const weekly = hosted.reduce((a, { g }) => a + (g.jobMinutes / Math.max(1, c.m.windowDays)) * 7, 0); const caution = !c.data.isPrivate && hosted.length ? 'a third-party runner executes fork PR code on its machines: review that before moving pull_request jobs' : undefined; @@ -154,7 +160,7 @@ export function lessonParallel(c: LessonContext): Lesson { const slowest = c.m.groups.filter(g => g.workflow === top.workflow).sort((a, b) => (b.wait.p50 ?? 0) + (b.setup.p50 ?? 0) + (b.run.p50 ?? 0) - ((a.wait.p50 ?? 0) + (a.setup.p50 ?? 0) + (a.run.p50 ?? 0)))[0]; const waiting = slowest && (slowest.wait.p50 ?? 0) > (slowest.setup.p50 ?? 0) + (slowest.run.p50 ?? 0); const late = `${top.workflow} finishes last on ${Math.round(top.share * 100)}% of PR heads, a median ${fmt(gap)} after everything else`; - return mk(7, 'finding', waiting ? `${late}, mostly waiting for a machine (${slowest!.key}: wait p50 ${fmt(slowest!.wait.p50)} for ${fmt((slowest!.setup.p50 ?? 0) + (slowest!.run.p50 ?? 0))} of work)` : late, { evidence, fix: waiting ? `move ${top.workflow}'s jobs to the same fast runner pool as the rest (lesson 1); splitting a job that mostly waits would not help` : `run everything in parallel and split ${top.workflow}'s slowest job (move unrelated steps into their own job)`, saving: { prMinutes: +((gap / 60) * top.share).toFixed(1), jobMinutesPerWeek: null, usdPerMonth: null, measured: true, note: 'upper bound per PR: the slowest check can finish no earlier than the next one' } }); + return mk(7, 'finding', waiting ? `${late}, mostly waiting for a machine (${slowest!.key}: wait p50 ${fmt(slowest!.wait.p50)} for ${fmt((slowest!.setup.p50 ?? 0) + (slowest!.run.p50 ?? 0))} of work)` : late, { evidence, fix: waiting ? `fix the wait (lesson 1 says where it comes from); splitting a job that mostly waits would not help` : `run everything in parallel and split ${top.workflow}'s slowest job (move unrelated steps into their own job)`, saving: { prMinutes: +((gap / 60) * top.share).toFixed(1), jobMinutesPerWeek: null, usdPerMonth: null, measured: true, note: 'upper bound per PR: the slowest check can finish no earlier than the next one' } }); } /** A test-runner concurrency flag in a run line (bun --max-concurrency, jest --maxWorkers, vitest --maxConcurrency, make -j). */ diff --git a/test/ci-audit.test.ts b/test/ci-audit.test.ts index a3aaeecb9..07b826915 100644 --- a/test/ci-audit.test.ts +++ b/test/ci-audit.test.ts @@ -4,7 +4,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { EXIT, main } from '../lib/ci-audit/cli'; import type { Run } from '../lib/ci-audit/collect'; -import { concurrencyFlag } from '../lib/ci-audit/lessons'; +import { concurrencyFlag, lessonRunners } from '../lib/ci-audit/lessons'; import { cohort, measure, phases } from '../lib/ci-audit/measure'; import { priceRunner } from '../lib/ci-audit/prices'; import { ISSUE_MARKER, planIssue } from '../lib/ci-audit/publish'; @@ -89,7 +89,8 @@ describe('planted repo (three problems, two decoys)', () => { expect(lesson(6).summary).toContain('no cache is saved from main'); expect(lesson(6).summary).toContain('100% of sampled cache restores missed'); expect(lesson(7).summary).toMatch(/lint\.yml finishes last on 100% of PR heads, a median 3\.\d min after everything else, mostly waiting for a machine/); - expect(lesson(7).fix).toContain('fast runner pool'); + expect(lesson(7).fix).toContain('fix the wait'); + expect(lesson(1).fix).toContain('same fast runner pool'); expect(lesson(1).evidence.join(' ')).toMatch(/lint on ubuntu-24\.04: wait p50 8\.0 min/); expect(lesson(8).summary).toContain('no test exercises the skip logic'); const all = JSON.stringify(r.lessons); @@ -170,6 +171,24 @@ describe('job phases', () => { }); }); +describe('lesson 1', () => { + const group = (key: string, labels: string, waitP90: number, runP50: number) => ({ key, workflow: key, job: key, labels, n: 10, wait: { n: 10, p50: waitP90 / 2, p90: waitP90 }, setup: { n: 10, p50: 5, p90: 5 }, run: { n: 10, p50: runP50, p90: runP50 }, longWaits: waitP90 > 60 ? Array.from({ length: 10 }, (_, i) => `2026-10-${10 + i}T0${i % 10}:00:00Z`) : [], jobMinutes: 1, billedMinutes: 10 }); + const ctx = (groups: any[]) => ({ data: { isPrivate: false }, m: { groups, windowDays: 7, sampledRuns: 10 }, st: null } as any); + + test('a GitHub-hosted job that waits while the rest run on a faster pool: move it', () => { + const l = lessonRunners(ctx([group('lint', 'ubuntu-24.04', 500, 5), group('test', 'ubicloud-standard-2', 20, 180)])); + expect(l.state).toBe('finding'); + expect(l.fix).toContain('same fast runner pool'); + }); + + test('a job that waits while another job on the same label starts at once: a faster runner would not help', () => { + const l = lessonRunners(ctx([group('lint', 'ubuntu-24.04', 500, 5), group('test', 'ubuntu-24.04', 8, 180)])); + expect(l.state).toBe('finding'); + expect(l.summary).toContain('on the same runner label starts in seconds'); + expect(l.fix).toContain('a faster runner would not help'); + }); +}); + describe('prices', () => { test('GitHub standard runners are free on public repos; Ubicloud bills by VM family', () => { expect(priceRunner('ubuntu-24.04', false).usdPerMinute).toBe(0); @@ -254,6 +273,7 @@ describe('Saturday workflow template', () => { expect(doc.on.schedule).toEqual([{ cron: '0 15 * * 6' }]); expect('workflow_dispatch' in doc.on).toBe(true); expect(doc.permissions).toEqual({ actions: 'read', contents: 'read', issues: 'write' }); + expect(doc.jobs.audit.if).toBe('github.ref_name == github.event.repository.default_branch'); }); test('audits within the GITHUB_TOKEN hourly limit and always publishes the outcome', () => { diff --git a/test/helpers/ci-audit-planted.ts b/test/helpers/ci-audit-planted.ts index 09485dac8..e0ffec817 100644 --- a/test/helpers/ci-audit-planted.ts +++ b/test/helpers/ci-audit-planted.ts @@ -1,7 +1,8 @@ /** * A synthetic GitHub repo for /ci-audit with three planted problems and two decoys. * planted: no cache saved from main (every PR restore misses); a GitHub-hosted - * 5-second lint job that waits ~8 min for a machine and finishes last; + * 5-second lint job that waits ~8 min for a machine and finishes last + * while every other PR job runs on Ubicloud and starts in ~15 s; * a path-filtered e2e workflow that no test exercises. * decoys: a full-history checkout that already uses filter: blob:none; a * windows job on main that waits over a minute but runs for ten. @@ -25,7 +26,7 @@ on: pull_request: jobs: test: - runs-on: ubuntu-24.04 + runs-on: ubicloud-standard-2 steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 @@ -35,12 +36,12 @@ jobs: - run: npm ci - run: npm test typecheck: - runs-on: ubuntu-24.04 + runs-on: ubicloud-standard-2 steps: - uses: actions/checkout@v4 - run: npx tsc --noEmit release-notes: - runs-on: ubuntu-24.04 + runs-on: ubicloud-standard-2 steps: - uses: actions/checkout@v4 with: @@ -64,7 +65,7 @@ on: paths: ['src/**'] jobs: e2e: - runs-on: ubuntu-24.04 + runs-on: ubicloud-standard-2 steps: - uses: actions/checkout@v4 - run: npm ci @@ -111,17 +112,17 @@ function synth(): Data { const sha = `${(0xa000 + i).toString(16)}${'0'.repeat(36)}`; const pr = 100 + i; const ci = run('ci.yml', 'pull_request', sha, T, pr); - const test = job(ci.id, 'test', 'ubuntu-24.04', T + 3000, 5 + (i % 4), [['Set up job', 2], ['Run actions/checkout@v4', 3], ['Run actions/setup-node@v4', 25], ['Run npm ci', 40 + (i % 5)], ['Run npm test', 150 + (i * 7) % 50], ['Complete job', 1]]); + const test = job(ci.id, 'test', 'ubicloud-standard-2', T + 3000, 15 + (i % 4), [['Set up job', 2], ['Run actions/checkout@v4', 3], ['Run actions/setup-node@v4', 25], ['Run npm ci', 40 + (i % 5)], ['Run npm test', 150 + (i * 7) % 50], ['Complete job', 1]]); logs[test.j.id] = [`${iso(T)} Cache not found for input keys: node-cache-Linux-x64-npm-${i}`, `${iso(T)} npm ci finished`].join('\n'); - const tc = job(ci.id, 'typecheck', 'ubuntu-24.04', T + 3000, 4 + (i % 3), [['Set up job', 2], ['Run actions/checkout@v4', 3], ['Run npx tsc --noEmit', 12], ['Complete job', 1]]); - const rn = job(ci.id, 'release-notes', 'ubuntu-24.04', T + 3000, 4, [['Set up job', 2], ['Run actions/checkout@v4', 4], ['Run git log --oneline -20', 1], ['Complete job', 1]]); + const tc = job(ci.id, 'typecheck', 'ubicloud-standard-2', T + 3000, 14 + (i % 3), [['Set up job', 2], ['Run actions/checkout@v4', 3], ['Run npx tsc --noEmit', 12], ['Complete job', 1]]); + const rn = job(ci.id, 'release-notes', 'ubicloud-standard-2', T + 3000, 14, [['Set up job', 2], ['Run actions/checkout@v4', 4], ['Run git log --oneline -20', 1], ['Complete job', 1]]); setOnly(ci, Math.max(test.end, tc.end, rn.end)); const lint = run('lint.yml', 'pull_request', sha, T, pr); const lj = job(lint.id, 'lint', 'ubuntu-24.04', T + 2000, 420 + (i * 37) % 120, [['Set up job', 1], ['Run actions/checkout@v4', 2], ['Run npx oxlint', 5], ['Complete job', 1]]); setOnly(lint, lj.end); if (i % 2 === 0) { const e2e = run('e2e.yml', 'pull_request', sha, T, pr); - const ej = job(e2e.id, 'e2e', 'ubuntu-24.04', T + 2000, 6, [['Set up job', 2], ['Run actions/checkout@v4', 3], ['Run npm ci', 40], ['Run npm run e2e', 120], ['Complete job', 1]]); + const ej = job(e2e.id, 'e2e', 'ubicloud-standard-2', T + 2000, 16, [['Set up job', 2], ['Run actions/checkout@v4', 3], ['Run npm ci', 40], ['Run npm run e2e', 120], ['Complete job', 1]]); setOnly(e2e, ej.end); } if (i % 12 === 5) { @@ -202,7 +203,7 @@ const near = (a: string, b: string, gap = 240) => new RegExp(`(${a})[^\\n]{0,${g export const PLANTED = [ { id: 'unseeded-cache', re: near('cache', 'main\\b|default branch|miss|not found|cold|seed') }, { id: 'lint-waits-last', re: near('lint', '(wait|queue|machine|runner)[^\\n]{0,160}\\d+(\\.\\d+)?\\s*(min|minutes|m\\b|s\\b|sec)') }, - { id: 'untested-path-filter', re: near('e2e|path[- ]?filter|paths:|skip logic|skips? (PRs|runs)', 'no test|untested|not tested|nothing tests|without (a |any )?test|no .{0,20}test (covers|exercises)') }, + { id: 'untested-path-filter', re: near('e2e|path[- ]?filter|paths:|skip logic|skips? (PRs|runs)', 'no test|untested|not tested|nothing (tests|checks|verifies)|without (a |any )?test|no .{0,20}(test|check) (covers|exercises|checks)') }, ] as const; /** Decoys a reply must not present as something to fix. */