From 72cbb49ddf6b9010c47adb0d140fcddffa50cfc1 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 9 Nov 2026 13:46:36 -0800 Subject: [PATCH] Keep Free Tests planning under its job cap when the duration seed goes stale (GST-2193) (#4245) * Keep Free Tests planning under the job cap when the duration seed is stale (GST-2193) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Refresh the free-suite duration seed from run 42691699570 (GST-2193) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Admit the eight test files the fresh seed records over 60 seconds as annotated exceptions (GST-2193) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- .github/workflows/free-durations-record.yml | 46 +- changelog.d/free-seed-unseeded-cap.md | 4 + scripts/free-test-durations.json | 3078 ++++++++++--------- scripts/free-test-seed-allowlist.json | 10 +- scripts/test-free-shards.ts | 26 +- test/free-durations-record-workflow.test.ts | 44 +- test/free-tests-workflow-wiring.test.ts | 2 +- test/test-free-shards.test.ts | 56 + 8 files changed, 1813 insertions(+), 1453 deletions(-) create mode 100644 changelog.d/free-seed-unseeded-cap.md diff --git a/.github/workflows/free-durations-record.yml b/.github/workflows/free-durations-record.yml index 85749eea0..4f9c56081 100644 --- a/.github/workflows/free-durations-record.yml +++ b/.github/workflows/free-durations-record.yml @@ -9,7 +9,9 @@ name: Free-suite duration seeds # a seed PR. A file that fails while recording turns its job red but keeps its # duration (a red file is still a real cost) and is named in the PR body; an # interrupted recording, or more than 5 red files on either OS, proposes -# nothing (docs/TESTING_INTERNALS.md#free-suite-duration-seed). +# nothing (docs/TESTING_INTERNALS.md#free-suite-duration-seed). A seed PR +# closed without merging opens a blocker issue (the seed stays stale until the +# next recording lands), and a seed PR that merges closes it. on: schedule: @@ -20,9 +22,15 @@ on: description: Push the bot branch and open or refresh the seed PR (false records and reports only) type: boolean default: true + pull_request: + types: [closed] + branches: [main] + paths: + - scripts/free-test-durations.json + - scripts/free-test-durations-windows.json concurrency: - group: free-durations-record + group: ${{ github.event_name == 'pull_request' && format('free-durations-seed-pr-{0}', github.event.pull_request.number) || 'free-durations-record' }} cancel-in-progress: false permissions: @@ -30,6 +38,7 @@ permissions: jobs: linux: + if: github.event_name != 'pull_request' runs-on: ubicloud-standard-8 timeout-minutes: 45 steps: @@ -70,6 +79,7 @@ jobs: if-no-files-found: error windows: + if: github.event_name != 'pull_request' uses: ./.github/workflows/windows-free-tests.yml permissions: contents: read @@ -79,7 +89,7 @@ jobs: propose: needs: [linux, windows] - if: ${{ !cancelled() }} + if: ${{ !cancelled() && github.event_name != 'pull_request' }} runs-on: ubuntu-24.04 timeout-minutes: 10 permissions: @@ -174,3 +184,33 @@ jobs: gh workflow run free-tests.yml --repo "$GITHUB_REPOSITORY" --ref "$branch" gh workflow run windows-smoke.yml --repo "$GITHUB_REPOSITORY" --ref "$branch" gh workflow run quality-gate.yml --repo "$GITHUB_REPOSITORY" --ref "$branch" + + seed-pr-closed: + if: github.event_name == 'pull_request' && github.event.pull_request.head.ref == 'bot/free-duration-seeds' && github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: + issues: write + steps: + - name: Raise the stale-seed blocker when a seed PR closes unmerged, clear it when one merges + env: + GH_TOKEN: ${{ github.token }} + MERGED: ${{ github.event.pull_request.merged }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + set -euo pipefail + title="Blocker: free-suite duration seed PR closed unmerged" + existing=$(gh issue list --repo "$GITHUB_REPOSITORY" --state open --search "in:title \"$title\"" --json number --jq '.[0].number // empty') + if [ "$MERGED" = true ]; then + if [ -n "$existing" ]; then gh issue close "$existing" --repo "$GITHUB_REPOSITORY" --comment "Seed PR #$PR_NUMBER merged, so the seed is current again."; fi + exit 0 + fi + cat > "$RUNNER_TEMP/blocker.md" < file in TREE_MUTATING || file in S const planDigest = (plan: Omit): string => createHash('sha256').update(JSON.stringify(plan)).digest('hex'); -/** One immutable plan is shared by isolated CI machines; never repack per job. */ -export function createFreeCiPlan(files: string[], count: number, durations: Record, revision: string, shardJobs = 1): FreeCiPlan { +/** + * The largest packed prediction whose CI job still fits MAX_CI_JOB_TIMEOUT_MINUTES + * (inverse of ciShardJobTimeoutMinutes's prediction term). + */ +export function maxPredictedMsUnderJobCap(setupSeconds: number): number { + return Math.floor(((MAX_CI_JOB_TIMEOUT_MINUTES * 60 - setupSeconds) * 1000 - wallTimeoutForShard(FLAKY_RETRY_CAP)) / 3); +} + +/** + * One immutable plan is shared by isolated CI machines; never repack per job. + * With setupSeconds (--ci-plan --job-setup-seconds), a shard whose 99th-percentile + * estimates for unseeded files would push its job past the cap has its prediction + * cut back to the cap (GST-2193): a stale seed lengthens the job up to the cap and + * never fails the plan. Recorded durations are never cut, so a shard whose seeded + * files alone pass the cap still fails it. + */ +export function createFreeCiPlan(files: string[], count: number, durations: Record, revision: string, shardJobs = 1, setupSeconds?: number): FreeCiPlan { const readers = files.filter(file => !(file in TREE_MUTATING)); const exclusive = files.filter(file => file in TREE_MUTATING).sort(); const packed = packShardsByDuration(readers, count, durations); const planned = packed.shards.map((files, index) => ({ files, predictedMs: packed.predictedMs[index] })); if (exclusive.length) planned.push({ files: exclusive, predictedMs: exclusive.reduce((ms, file) => ms + (durations[file] ?? 0), 0) }); + const limitMs = setupSeconds === undefined ? Infinity : maxPredictedMsUnderJobCap(setupSeconds); + const seededMs = (shardFiles: string[]) => shardFiles.reduce((ms, file) => ms + (durations[normalizeRelativePath(file)] ?? 0), 0); const shards = planned.map(({ files, predictedMs }, index) => { const jobs = plannedShardJobs(files, shardJobs, runsSerialUnderJobs); - return { shard: index + 1, files, predictedMs, ...(jobs ? { jobs } : {}) }; + const capped = predictedMs > limitMs && seededMs(files) <= limitMs ? limitMs : predictedMs; + return { shard: index + 1, files, predictedMs: capped, ...(jobs ? { jobs } : {}) }; }); const body = { version: 1 as const, revision, shards }; return { ...body, id: planDigest(body) }; @@ -2036,7 +2054,7 @@ async function main(): Promise { appendStepSummary(line); } warnUnseededFreeFiles(ciFiles, durations); - const plan = createFreeCiPlan(ciFiles, options.shardCount, durations, revision, options.shardJobs); + const plan = createFreeCiPlan(ciFiles, options.shardCount, durations, revision, options.shardJobs, options.jobSetupSeconds); validateFreeCiPlan(plan, ciFiles, revision); const matrix = ciPlanMatrix(plan, options.jobSetupSeconds); if ('include' in matrix) { diff --git a/test/free-durations-record-workflow.test.ts b/test/free-durations-record-workflow.test.ts index 85f15f216..cadd07a99 100644 --- a/test/free-durations-record-workflow.test.ts +++ b/test/free-durations-record-workflow.test.ts @@ -20,7 +20,8 @@ afterEach(() => { for (const dir of dirs.splice(0)) rmSync(dir, { recursive: tru test('the recorder runs weekly before test-health and on dispatch, read-only except the propose job', () => { expect(recorder.on.schedule).toEqual([{ cron: '0 4 * * 1' }]); - expect(Object.keys(recorder.on).sort()).toEqual(['schedule', 'workflow_dispatch']); + expect(Object.keys(recorder.on).sort()).toEqual(['pull_request', 'schedule', 'workflow_dispatch']); + for (const job of ['linux', 'windows']) expect(recorder.jobs[job].if).toBe("github.event_name != 'pull_request'"); expect(Object.keys(recorder.on.workflow_dispatch.inputs)).toEqual(['open_pr']); expect(load('test-health.yml').on.schedule[0].cron).toBe('0 10 * * 1'); expect(recorder.permissions).toEqual({ contents: 'read' }); @@ -46,9 +47,9 @@ test('Linux records on the free-suite runner image, turns red on any red file, a }); test('Windows records through the windows-free-tests record_durations path, and propose needs both complete recordings', () => { - expect(recorder.jobs.windows).toEqual({ uses: './.github/workflows/windows-free-tests.yml', permissions: { contents: 'read', actions: 'read' }, with: { record_durations: true } }); + expect(recorder.jobs.windows).toEqual({ if: "github.event_name != 'pull_request'", uses: './.github/workflows/windows-free-tests.yml', permissions: { contents: 'read', actions: 'read' }, with: { record_durations: true } }); expect(recorder.jobs.propose.needs).toEqual(['linux', 'windows']); - expect(recorder.jobs.propose.if).toBe('${{ !cancelled() }}'); + expect(recorder.jobs.propose.if).toBe("${{ !cancelled() && github.event_name != 'pull_request' }}"); const downloads = recorder.jobs.propose.steps.filter((step: any) => step.uses?.startsWith('actions/download-artifact@')); expect(downloads.map((step: any) => step.with.name)).toEqual(['free-test-durations', 'free-test-durations-windows']); expect(downloads.every((step: any) => step['continue-on-error'] === undefined)).toBe(true); @@ -180,3 +181,40 @@ test('the drift check counts added and removed files, so an unchanged file set p expect(drifted.status, drifted.stderr).toBe(0); expect(drifted.stdout.trim()).toBe('"2 1"'); }); + +test('a seed PR closed unmerged raises one blocker issue and a red run; a merged seed PR clears it (GST-2193)', () => { + expect(recorder.on.pull_request).toEqual({ types: ['closed'], branches: ['main'], + paths: ['scripts/free-test-durations.json', 'scripts/free-test-durations-windows.json'] }); + expect(recorder.concurrency.group).toContain("format('free-durations-seed-pr-{0}', github.event.pull_request.number)"); + const job = recorder.jobs['seed-pr-closed']; + expect(job.if).toBe("github.event_name == 'pull_request' && github.event.pull_request.head.ref == 'bot/free-duration-seeds' && github.event.pull_request.head.repo.full_name == github.repository"); + expect(job.permissions).toEqual({ issues: 'write' }); + expect(job.steps.some((step: any) => step.uses?.startsWith('actions/checkout@'))).toBe(false); + const step = job.steps[0]; + const dir = mkdtempSync(path.join(os.tmpdir(), 'seed-blocker-')); + dirs.push(dir); + const run = (merged: boolean, existing: string) => { + const log = path.join(dir, `gh-${merged}-${existing || 'none'}.log`); + writeFileSync(path.join(dir, 'gh'), `#!/bin/sh\necho "$*" >> "${log}"\nif [ "$1 $2" = "issue list" ]; then echo "${existing}"; fi\n`, { mode: 0o755 }); + const result = spawnSync('bash', ['-c', step.run], { encoding: 'utf8', timeout: 10_000, + env: { ...process.env, PATH: `${dir}:${process.env.PATH}`, RUNNER_TEMP: dir, GITHUB_REPOSITORY: 'garrytan/gstack', MERGED: String(merged), PR_NUMBER: '3835' } }); + const calls = (() => { try { return readFileSync(log, 'utf8').trim().split('\n'); } catch { return []; } })(); + return { status: result.status, stdout: result.stdout, calls }; + }; + const raised = run(false, ''); + expect(raised.status).toBe(1); + expect(raised.stdout).toContain('::error::Seed PR #3835 closed unmerged'); + expect(raised.calls[1]).toBe(`issue create --repo garrytan/gstack --title Blocker: free-suite duration seed PR closed unmerged --body-file ${dir}/blocker.md`); + const body = readFileSync(path.join(dir, 'blocker.md'), 'utf8'); + expect(body).toContain('Seed PR #3835 was closed without merging'); + expect(body).toContain('`gh workflow run free-durations-record.yml --ref main`'); + const again = run(false, '41'); + expect(again.status).toBe(1); + expect(again.calls[1]).toBe(`issue comment 41 --repo garrytan/gstack --body-file ${dir}/blocker.md`); + const cleared = run(true, '41'); + expect(cleared.status).toBe(0); + expect(cleared.calls[1]).toBe('issue close 41 --repo garrytan/gstack --comment Seed PR #3835 merged, so the seed is current again.'); + const quiet = run(true, ''); + expect(quiet.status).toBe(0); + expect(quiet.calls).toHaveLength(1); +}); diff --git a/test/free-tests-workflow-wiring.test.ts b/test/free-tests-workflow-wiring.test.ts index 2de21d506..6d4dcaba6 100644 --- a/test/free-tests-workflow-wiring.test.ts +++ b/test/free-tests-workflow-wiring.test.ts @@ -331,7 +331,7 @@ describe('free-tests workflow wiring', () => { const setupSeconds = Number(workflow.env.FREE_SUITE_SETUP_SECONDS); expect(Number.isInteger(setupSeconds) && setupSeconds > 0).toBe(true); const plan = createFreeCiPlan(collectFreeTestFiles(), Number(workflow.env.FREE_SUITE_SHARDS), - loadFreeTestDurations() ?? {}, 'wiring', Number(workflow.env.FREE_SUITE_JOBS)); + loadFreeTestDurations() ?? {}, 'wiring', Number(workflow.env.FREE_SUITE_JOBS), setupSeconds); const declared = workflow.jobs['free-suite']['timeout-minutes']; const emitted = ciPlanMatrix(plan, setupSeconds); if (!('include' in emitted)) throw new Error('the CI matrix carries no per-shard timeouts'); diff --git a/test/test-free-shards.test.ts b/test/test-free-shards.test.ts index 2ef262197..4acd4970a 100644 --- a/test/test-free-shards.test.ts +++ b/test/test-free-shards.test.ts @@ -19,6 +19,7 @@ import { DEFAULT_WALL_TIMEOUT_MS, PER_FILE_WALL_MS, wallTimeoutForShard, + FLAKY_RETRY_CAP, recordingWallTimeoutMs, parseCliOptions, KNOWN_WINDOWS_INCOMPATIBLE, @@ -1123,6 +1124,61 @@ describe('test-free-shards: duration-aware packing (full-suite LPT)', () => { expect(ciPlanMatrix(plan, 300)).toEqual({ include: [{ shard: 1, timeout_minutes: 49 }, { shard: 2, timeout_minutes: 17 }] }); }); + test('unseeded files never push a shard past the job cap; their packed share is cut to fit (GST-2193)', () => { + // A stale seed packs every unseeded file at the seed's 99th percentile. On + // 2026-11-09 196 unseeded files put each shard at 77 of the 90-minute cap, + // and about 60 more would have failed every PR's plan. Recorded durations + // keep their full weight; only the unseeded estimate gives way. + const durations: Record = { 'test/slow.test.ts': 1_200_000 }; + for (let i = 0; i < 99; i += 1) durations[`test/s${i}.test.ts`] = 1_000; + const files = [...Object.keys(durations), 'test/new-a.test.ts', 'test/new-b.test.ts']; + expect(() => ciPlanMatrix(createFreeCiPlan(files, 1, durations, 'rev'), 300)).toThrow(/FREE_JOB_TIMEOUT_OVER_CAP/); + const plan = createFreeCiPlan(files, 1, durations, 'rev', 1, 300); + validateFreeCiPlan(plan, files, 'rev'); + expect(plan.shards[0].files).toEqual([...files].sort()); + expect(ciPlanMatrix(plan, 300)).toEqual({ include: [{ shard: 1, timeout_minutes: MAX_CI_JOB_TIMEOUT_MINUTES }] }); + const neededMs = wallTimeoutForPackedShard(plan.shards[0].predictedMs, WALL_BASE_MS, files.length) + wallTimeoutForShard(FLAKY_RETRY_CAP) + 300_000; + expect(neededMs).toBeLessThanOrEqual(MAX_CI_JOB_TIMEOUT_MINUTES * 60_000); + expect(plan.shards[0].predictedMs).toBeGreaterThanOrEqual(1_299_000); + }); + + test('a plan under the job cap is unchanged by the unseeded cap (GST-2193 hold)', () => { + const seeded = { 'test/a.test.ts': 750_000, 'test/b.test.ts': 1_000 }; + const plain = createFreeCiPlan(Object.keys(seeded), 2, seeded, 'rev'); + expect(createFreeCiPlan(Object.keys(seeded), 2, seeded, 'rev', 1, 300)).toEqual(plain); + expect(ciPlanMatrix(plain, 300)).toEqual({ include: [{ shard: 1, timeout_minutes: 49 }, { shard: 2, timeout_minutes: 17 }] }); + const partly = { 'test/a.test.ts': 600_000 }; + const files = ['test/a.test.ts', 'test/b.test.ts']; + const plainPartly = createFreeCiPlan(files, 1, partly, 'rev'); + expect(plainPartly.shards[0].predictedMs).toBe(1_200_000); + expect(createFreeCiPlan(files, 1, partly, 'rev', 1, 300)).toEqual(plainPartly); + expect(ciPlanMatrix(plainPartly, 300)).toEqual({ include: [{ shard: 1, timeout_minutes: 71 }] }); + }); + + test('recorded durations over the job cap still fail the plan; only the unseeded share is cut (GST-2193 hold)', () => { + const durations = { 'test/slow.test.ts': 1_700_000, 'test/b.test.ts': 99_000 }; + const plan = createFreeCiPlan([...Object.keys(durations), 'test/new.test.ts'], 1, durations, 'rev', 1, 300); + expect(plan.shards[0].predictedMs).toBe(1_799_000 + 1_700_000); + expect(() => ciPlanMatrix(plan, 300)).toThrow(/FREE_JOB_TIMEOUT_OVER_CAP: CI shard 1/); + }); + + test('--ci-plan with a stale seed keeps every job under the cap instead of failing the plan (GST-2193)', () => { + const committed = loadFreeTestDurations() ?? {}; + const kept = Object.keys(committed).sort().filter((_, index) => index % 4 === 0); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'free-seed-stale-')); + try { + const seedPath = path.join(dir, 'seed.json'); + fs.writeFileSync(seedPath, JSON.stringify({ version: 1, durations: Object.fromEntries(kept.map(file => [file, committed[file]])) })); + const planned = Bun.spawnSync([process.execPath, path.join(ROOT, 'scripts/test-free-shards.ts'), '--ci-plan', path.join(dir, 'plan.json'), + '--shards', '10', '--shard-jobs', '4', '--job-setup-seconds', '300'], { env: { ...process.env, GSTACK_FREE_TEST_DURATIONS: seedPath }, timeout: 10_000 }); + expect(planned.exitCode, planned.stderr.toString()).toBe(0); + const matrix = JSON.parse(planned.stdout.toString()) as { include: Array<{ timeout_minutes: number }> }; + expect(matrix.include.length).toBeGreaterThan(0); + expect(matrix.include.every(entry => entry.timeout_minutes <= MAX_CI_JOB_TIMEOUT_MINUTES)).toBe(true); + expect(planned.stderr.toString()).toMatch(/WARNING: \d+ free test files have no recorded duration/); + } finally { fs.rmSync(dir, { recursive: true, force: true }); } + }); + test('--job-setup-seconds is a --ci-plan flag with whole seconds from 1 to 3600', () => { expect(parseCliOptions(['--ci-plan', 'p.json', '--job-setup-seconds', '300']).jobSetupSeconds).toBe(300); expect(() => parseCliOptions(['--shards', '2', '--job-setup-seconds', '300'])).toThrow('--job-setup-seconds applies only to --ci-plan');