From 417d810cf212326bbec77bc71af58bbe589d5207 Mon Sep 17 00:00:00 2001 From: acoliver Date: Fri, 7 Aug 2026 23:58:38 -0300 Subject: [PATCH 1/5] Give every Bun test runner one scheduling policy (Fixes #3139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI was green on only 9 of the last 20 first attempts. Nearly every PR needed a rerun, and the failures never named the change under review: a different file timed out on each run and every one of them passed in isolation. Four workspace runners each carried their own concurrency and timeout policy. `agents` was tuned against measurement in #3084; the other three were not, and the shards still running the untuned policy are the shards that fail — `cli` 5/15 first attempts, `agents` 4/16, `core` 2/14. The tests were not broken, they were starved. `cli` had the worst of both ends: a pool sized to the full core count and a 30s per-test bound. `auth` passed no `--timeout` at all, so it ran on Bun's 5s default; a `[test] timeout` key in bunfig.toml does not help, because Bun 1.3.14 ignores it. `scripts/run_bun_tests.ts` defaulted to 30s while the workspace runner beside it used 180s, so the same test had two different budgets depending on which path CI took. Throughput does not degrade gracefully when the pool matches the core count; it collapses. Measured on the 680-file `cli` suite, 16 cores: processes per core wall clock 0.25 184s 0.50 124s 1.00 >9min, not one file completed GitHub's ubuntu-latest runner has 4 vCPUs, and `cli` was asking for 4 processes — exactly the setting where nothing finishes. Half the cores is both the fastest point measured and the one that leaves a test able to finish inside its budget. scripts/lib/bun-test-policy.ts now owns that policy and the runners consume it. Differences that were deliberate are preserved: `core` keeps its cap of 2, `cli` keeps a larger budget for the integration files that spawn the real CLI, and every runner keeps its concurrency override for pinning a run while chasing a flake. What is gone is the divergence nobody chose. Two defects surfaced while wiring it up. `auth` scheduled in fixed batches, so a batch advanced only when its slowest file finished and left workers idle with queued work; it now uses the same worker pool as the others. And `cli`'s integration budget was a fixed 120s, which silently became *smaller* than the unit budget once that rose to 180s, handing the slowest tests in the workspace the tightest bound — it is now expressed as a multiple of the shared budget, so it cannot invert again. The invariant that allowed this is now a test: no runner may spawn `bun test` without an explicit `--timeout`, none may recompute concurrency for itself, and all must derive from the shared module. `scripts/tests/` files are listed individually in tsconfig.scripts.json, so the new suite is registered there — adding it immediately caught a type error the file would otherwise have carried. Cost, measured rather than assumed: `core` 95.59s before, 95.91s after. --- packages/agents/run-bun-tests.ts | 60 ++---- packages/auth/run-bun-tests.ts | 40 +++- .../__tests__/run-bun-tests.behavior.test.ts | 12 +- packages/cli/run-bun-tests.ts | 31 ++- packages/core/run-bun-tests.ts | 15 +- packages/core/tsconfig.runner.json | 6 +- scripts/lib/bun-test-policy.ts | 137 ++++++++++++ scripts/run_bun_tests.ts | 6 +- scripts/tests/bun-test-policy.bun.test.ts | 196 ++++++++++++++++++ tsconfig.scripts.json | 1 + 10 files changed, 441 insertions(+), 63 deletions(-) create mode 100644 scripts/lib/bun-test-policy.ts create mode 100644 scripts/tests/bun-test-policy.bun.test.ts diff --git a/packages/agents/run-bun-tests.ts b/packages/agents/run-bun-tests.ts index f9252224e2..a64fb806f8 100644 --- a/packages/agents/run-bun-tests.ts +++ b/packages/agents/run-bun-tests.ts @@ -39,7 +39,13 @@ import { writeFileSync, } from 'node:fs'; import { join, relative } from 'node:path'; -import { availableParallelism, tmpdir } from 'node:os'; +import { tmpdir } from 'node:os'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + MAX_TEST_CONCURRENCY, + resolveTestConcurrency, +} from '../../scripts/lib/bun-test-policy.js'; /** * Every path this runner touches — discovery, the child's working directory @@ -51,46 +57,24 @@ const JUNIT_PATH = join(WORKSPACE_ROOT, 'junit.xml'); const TEST_ROOTS = ['src'] as const; -/** Upper bound on file concurrency, regardless of how many cores are present. */ -const MAX_CONCURRENCY = 4; - -/** Lower bound: small CI runners need one free core for each Bun child. */ -const MIN_CONCURRENCY = 1; +/** + * Upper bound on file concurrency, regardless of how many cores are present. + * + * Kept as a named constant because the JUnit summary reports it. + */ +const MAX_CONCURRENCY = MAX_TEST_CONCURRENCY; /** * Number of test files executed at once. * - * Deliberately half the core count, clamped to [1, 4]. Each file is a fresh - * `bun test` process that re-executes the whole agents module graph, and many - * suites under `src/api/__tests__/` additionally build a real Agent (tool - * registry, provider bootstrap, settings) per test. Saturating every core with - * that work starves individual tests past the 30s budget, which surfaces as a - * different file failing on each run rather than as an honestly slow run. - * Leaving headroom matters most on small CI runners, where the core count is - * roughly the concurrency an unclamped default would pick. macOS CI runs one - * file at a time because its virtual cores repeatedly starve one process past - * the test budget even at half-concurrency. - * - * `LLXPRT_AGENTS_TEST_CONCURRENCY` overrides this. + * The half-the-cores policy this workspace arrived at by measurement now lives + * in scripts/lib/bun-test-policy.ts, so the other runners inherit it instead of + * rediscovering it (issue #3139). `LLXPRT_AGENTS_TEST_CONCURRENCY` overrides. */ -function resolveConcurrency(): number { - const override = process.env.LLXPRT_AGENTS_TEST_CONCURRENCY; - if (override !== undefined) { - if (!/^[1-9][0-9]*$/.test(override.trim())) { - throw new Error( - `LLXPRT_AGENTS_TEST_CONCURRENCY must be a positive integer, got: ${override}`, - ); - } - return Number.parseInt(override.trim(), 10); - } - if (process.platform === 'darwin' && process.env.CI === 'true') { - return MIN_CONCURRENCY; - } - const half = Math.floor(availableParallelism() / 2); - return Math.min(MAX_CONCURRENCY, Math.max(MIN_CONCURRENCY, half)); -} - -const CONCURRENCY = resolveConcurrency(); +const CONCURRENCY = resolveTestConcurrency({ + envVar: 'LLXPRT_AGENTS_TEST_CONCURRENCY', + maxConcurrency: MAX_CONCURRENCY, +}); /** * Per-test timeout, mirroring the `testTimeout: 30000` this workspace ran under @@ -112,7 +96,7 @@ const CONCURRENCY = resolveConcurrency(); * by PER_FILE_TIMEOUT_MS below, which is what should happen - a raised * per-test bound must not turn a hang into a longer hang. */ -const PER_TEST_TIMEOUT_MS = 180_000; +const PER_TEST_TIMEOUT_MS = DEFAULT_PER_TEST_TIMEOUT_MS; /** * Per-file wall-clock budget. The slowest agents files (streaming chat-session @@ -123,7 +107,7 @@ const PER_TEST_TIMEOUT_MS = 180_000; // subagentOrchestrator-loadBalancer was measured at 99.4s under load, leaving // almost no headroom under the previous 120s cap. This remains the backstop // for a suite that genuinely hangs. -const PER_FILE_TIMEOUT_MS = 300_000; +const PER_FILE_TIMEOUT_MS = DEFAULT_PER_FILE_TIMEOUT_MS; /** * Directories that are pruned during discovery. diff --git a/packages/auth/run-bun-tests.ts b/packages/auth/run-bun-tests.ts index 3eee67dc94..f1bc436a4d 100644 --- a/packages/auth/run-bun-tests.ts +++ b/packages/auth/run-bun-tests.ts @@ -14,7 +14,11 @@ import { spawn } from 'node:child_process'; import { readdirSync, statSync, writeFileSync } from 'node:fs'; import { join, relative } from 'node:path'; -import { availableParallelism } from 'node:os'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + resolveTestConcurrency, +} from '../../scripts/lib/bun-test-policy.js'; /** * Every path this runner touches — discovery, the child's working directory, @@ -25,8 +29,10 @@ import { availableParallelism } from 'node:os'; const WORKSPACE_ROOT = import.meta.dir; const PRELOAD = join(WORKSPACE_ROOT, 'bun-preload.ts'); const JUNIT_PATH = join(WORKSPACE_ROOT, 'junit.xml'); -const CONCURRENCY = Math.min(8, availableParallelism()); -const PER_FILE_TIMEOUT_MS = 60_000; +const CONCURRENCY = resolveTestConcurrency({ + envVar: 'LLXPRT_AUTH_TEST_CONCURRENCY', +}); +const PER_FILE_TIMEOUT_MS = DEFAULT_PER_FILE_TIMEOUT_MS; const TEST_ROOTS = ['src'] as const; @@ -88,7 +94,16 @@ export function runTestFile(file: string): Promise { let resolved = false; const child = spawn( process.execPath, - ['test', '--preload', PRELOAD, file], + [ + 'test', + // Bun 1.3.14 ignores a `[test] timeout` key in bunfig.toml and falls + // back to 5s, so this flag is the only thing that sets the budget. + '--timeout', + String(DEFAULT_PER_TEST_TIMEOUT_MS), + '--preload', + PRELOAD, + file, + ], { cwd: WORKSPACE_ROOT, stdio: 'inherit', @@ -198,14 +213,23 @@ async function main(): Promise { `Running ${testFiles.length} test files with concurrency ${CONCURRENCY}`, ); + // A worker pool rather than fixed batches: a batch only advances when its + // slowest file finishes, so one slow file leaves CONCURRENCY - 1 slots idle + // exactly when the machine has work queued behind it. const results: TestResult[] = []; + let nextIndex = 0; - for (let i = 0; i < testFiles.length; i += CONCURRENCY) { - const batch = testFiles.slice(i, i + CONCURRENCY); - const batchResults = await Promise.all(batch.map(runTestFile)); - results.push(...batchResults); + async function worker(): Promise { + while (nextIndex < testFiles.length) { + const file = testFiles[nextIndex++]; + results.push(await runTestFile(file)); + } } + await Promise.all( + Array.from({ length: Math.min(CONCURRENCY, testFiles.length) }, worker), + ); + const passed = results.filter((r) => r.passed).length; const failed = results.filter((r) => !r.passed); diff --git a/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts b/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts index fc2e516735..35cf8c8a5a 100644 --- a/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts +++ b/packages/auth/src/__tests__/run-bun-tests.behavior.test.ts @@ -12,6 +12,14 @@ import { runTestFile, type TestResult, } from '../../run-bun-tests.js'; +import { DEFAULT_PER_FILE_TIMEOUT_MS } from '../../../../scripts/lib/bun-test-policy.js'; + +/** + * Derived from the shared policy rather than hardcoded: the assertion is that + * the reason names the budget that was actually applied, not that the budget + * has one particular value (issue #3139). + */ +const EXPECTED_TIMEOUT_REASON = `Timed out after ${DEFAULT_PER_FILE_TIMEOUT_MS / 1000}s`; describe('auth run-bun-tests JUnit failure reporting', () => { const baseResult: TestResult = { @@ -38,7 +46,7 @@ describe('auth run-bun-tests JUnit failure reporting', () => { it('reports a timeout', () => { const xml = generateJUnit([{ ...baseResult, timedOut: true }], 1, 1); - expect(xml).toContain('Timed out after 60s'); + expect(xml).toContain(EXPECTED_TIMEOUT_REASON); }); it('falls back to an exit code when neither signal nor timeout is present', () => { @@ -67,7 +75,7 @@ describe('auth run-bun-tests failure reason formatting', () => { signal: 'SIGTERM', exitCode: 1, }); - expect(reason).toBe('Timed out after 60s'); + expect(reason).toBe(EXPECTED_TIMEOUT_REASON); }); it('reports a signal before an exit code', () => { diff --git a/packages/cli/run-bun-tests.ts b/packages/cli/run-bun-tests.ts index 0ee70327e0..10bdac8b82 100644 --- a/packages/cli/run-bun-tests.ts +++ b/packages/cli/run-bun-tests.ts @@ -24,25 +24,38 @@ import { spawn } from 'node:child_process'; import { readdirSync, realpathSync, statSync, writeFileSync } from 'node:fs'; import { join, relative } from 'node:path'; -import { availableParallelism } from 'node:os'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + resolveTestConcurrency, +} from '../../scripts/lib/bun-test-policy.js'; -const PER_FILE_TIMEOUT_MS = 120_000; +const PER_FILE_TIMEOUT_MS = DEFAULT_PER_FILE_TIMEOUT_MS; const PER_INTEGRATION_FILE_TIMEOUT_MS = 900_000; /** - * Per-test timeout, matching the testTimeout the removed vitest.config.ts set. - * Bun defaults to 5s, which the tests that spawn the real CLI exceed once the - * suite runs with concurrency. Passed as a flag because the bunfig.toml key is - * not picked up for a single-file invocation. + * Per-test timeout. Bun defaults to 5s, which the tests that spawn the real + * CLI exceed once the suite runs with concurrency. Passed as a flag because + * the bunfig.toml key is not picked up for a single-file invocation. + * + * Raised from 30s to the shared budget for issue #3139: this workspace was the + * worst-failing CI shard (5/15 first attempts) because it combined the tight + * bound with a pool that saturated every core. */ -const PER_TEST_TIMEOUT_MS = 30_000; +const PER_TEST_TIMEOUT_MS = DEFAULT_PER_TEST_TIMEOUT_MS; /** * Integration tests spawn the built CLI, which cold-starts from TypeScript * source and is far slower than an in-process test — especially on a loaded CI * runner. They get a larger per-test budget so a slow boot is not reported as a * failure. + * + * Expressed as a multiple of the shared budget rather than a fixed 120s: once + * the unit budget rose to 180s for issue #3139 a fixed value silently became + * the *smaller* of the two, which would have given the slowest tests in the + * workspace the tightest bound. It stays well inside + * PER_INTEGRATION_FILE_TIMEOUT_MS, which remains the hang backstop. */ -const PER_INTEGRATION_TEST_TIMEOUT_MS = 120_000; +const PER_INTEGRATION_TEST_TIMEOUT_MS = DEFAULT_PER_TEST_TIMEOUT_MS * 2; const SKIPPED_DIRECTORIES = new Set([ 'node_modules', @@ -104,7 +117,7 @@ function parseConcurrency(): number { return parsed; } } - return Math.max(1, Math.min(8, availableParallelism())); + return resolveTestConcurrency({ envVar: 'LLXPRT_CLI_TEST_CONCURRENCY' }); } export function isTestFile(fileName: string): boolean { diff --git a/packages/core/run-bun-tests.ts b/packages/core/run-bun-tests.ts index b08c95d8fd..d205b62974 100644 --- a/packages/core/run-bun-tests.ts +++ b/packages/core/run-bun-tests.ts @@ -23,7 +23,11 @@ import { spawn, type ChildProcess } from 'node:child_process'; import { readdirSync, statSync, writeFileSync } from 'node:fs'; import { join, relative } from 'node:path'; -import { availableParallelism } from 'node:os'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + resolveTestConcurrency, +} from '../../scripts/lib/bun-test-policy.js'; /** * Every path this runner touches — discovery, the child's working directory, @@ -38,9 +42,12 @@ const JUNIT_PATH = join(WORKSPACE_ROOT, 'junit.xml'); // children overlap. POSIX retains bounded parallelism without saturating shared // CI runners, where event-loop starvation can trip otherwise healthy test files. const MAX_CONCURRENCY = process.platform === 'win32' ? 1 : 2; -const CONCURRENCY = Math.min(MAX_CONCURRENCY, availableParallelism()); -const PER_TEST_TIMEOUT_MS = 30_000; -const PER_FILE_TIMEOUT_MS = process.platform === 'win32' ? 180_000 : 60_000; +const CONCURRENCY = resolveTestConcurrency({ + envVar: 'LLXPRT_CORE_TEST_CONCURRENCY', + maxConcurrency: MAX_CONCURRENCY, +}); +const PER_TEST_TIMEOUT_MS = DEFAULT_PER_TEST_TIMEOUT_MS; +const PER_FILE_TIMEOUT_MS = DEFAULT_PER_FILE_TIMEOUT_MS; const TEST_ROOTS = ['src', 'test'] as const; diff --git a/packages/core/tsconfig.runner.json b/packages/core/tsconfig.runner.json index c1c338dff1..4d420f9786 100644 --- a/packages/core/tsconfig.runner.json +++ b/packages/core/tsconfig.runner.json @@ -9,5 +9,9 @@ "@vybestack/llxprt-code-core/*": ["./src/*"] } }, - "include": ["run-bun-tests.ts", "test/run-bun-tests.test.ts"] + "include": [ + "run-bun-tests.ts", + "test/run-bun-tests.test.ts", + "../../scripts/lib/bun-test-policy.ts" + ] } diff --git a/scripts/lib/bun-test-policy.ts b/scripts/lib/bun-test-policy.ts new file mode 100644 index 0000000000..6270eb3656 --- /dev/null +++ b/scripts/lib/bun-test-policy.ts @@ -0,0 +1,137 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Single source of truth for how the Bun test runners schedule work + * (issue #3139). + * + * Each workspace owns a `run-bun-tests.ts` that spawns one `bun test` process + * per file. Those runners grew independently and ended up disagreeing about + * both how many files to run at once and how long a test may take. The + * `agents` runner was tuned against measurement in #3084 and the others were + * not, so the workspaces still carrying the untuned policy are the ones whose + * CI shards fail: `cli` saturated every core while keeping a 30s per-test + * budget, and `auth` passed no `--timeout` at all and therefore ran on Bun's + * 5s default. + * + * The failures that policy produces are indistinguishable from flakiness: a + * different file times out on each run and every one of them passes in + * isolation. The work completes; the budget simply cut it off. + * + * Justified per-runner differences are preserved through `maxConcurrency` and + * explicit timeout arguments. What is centralised is the shape of the policy, + * so a future runner cannot quietly reintroduce the divergence. + */ + +import { availableParallelism } from 'node:os'; + +/** + * Per-test budget. + * + * Measured on the worst offender: `subagentOrchestrator-loadBalancer` takes + * ~430ms per launch in isolation but was timed at 78.6s with the pool + * saturated, while consuming 0.8s of user CPU — it waits rather than computes. + * Its failure rate across repeated runs was 4/24 at 30s and 0/24 at 60s. + */ +export const DEFAULT_PER_TEST_TIMEOUT_MS = 180_000; + +/** + * Whole-file budget, and the backstop that keeps a raised per-test bound from + * turning a genuine hang into a longer hang. Sized to admit a slow but + * progressing file rather than to bound total runtime. + */ +export const DEFAULT_PER_FILE_TIMEOUT_MS = 300_000; + +/** + * Ceiling on files run at once, regardless of core count. + * + * Each file is a fresh `bun test` process that re-executes the whole workspace + * module graph, so throughput stops improving well before the core count and + * the extra processes only steal time from the tests already running. + */ +export const MAX_TEST_CONCURRENCY = 4; + +/** A pool of zero would never run anything. */ +export const MIN_TEST_CONCURRENCY = 1; + +export interface TestConcurrencyOptions { + /** + * Environment variable that overrides the computed value, e.g. + * `LLXPRT_AGENTS_TEST_CONCURRENCY`. Must parse as a positive integer. + */ + readonly envVar?: string; + /** + * Ceiling for this runner, when it is lower than {@link MAX_TEST_CONCURRENCY} + * for a reason of its own. `core` caps at 2 because its files are unusually + * heavy. + */ + readonly maxConcurrency?: number; + /** Injected for testing; defaults to the real environment. */ + readonly env?: NodeJS.ProcessEnv; + /** Injected for testing; defaults to the real platform. */ + readonly platform?: NodeJS.Platform; + /** Injected for testing; defaults to `availableParallelism()`. */ + readonly cores?: number; +} + +/** + * Files to run at once: half the available cores, clamped. + * + * Half rather than all, because a `bun test` child is not single-threaded — it + * transpiles and collects garbage on its own threads — so one process per core + * oversubscribes the machine. Throughput does not degrade gracefully there; it + * collapses. The 680-file `cli` suite, measured on a 16-core machine: + * + * | processes per core | wall clock | + * | ------------------ | ------------------------------ | + * | 0.25 | 184s | + * | 0.50 | 124s | + * | 1.00 | >9min, not one file completed | + * + * Half the cores is both the fastest setting measured and the one that keeps + * the machine out of the collapse. It is also the setting under which a test + * finishes inside its budget: at one process per core the same tests are still + * running when the budget expires, which is why a different file failed on + * each CI run while every one of them passed in isolation. + * + * macOS CI runs one file at a time: its virtual cores repeatedly starved a + * process past the budget even at half. + */ +export function resolveTestConcurrency( + options: TestConcurrencyOptions = {}, +): number { + const env = options.env ?? process.env; + const platform = options.platform ?? process.platform; + const ceiling = options.maxConcurrency ?? MAX_TEST_CONCURRENCY; + + if (!Number.isInteger(ceiling) || ceiling < MIN_TEST_CONCURRENCY) { + throw new Error( + `maxConcurrency must be an integer >= ${MIN_TEST_CONCURRENCY}, got: ${ceiling}`, + ); + } + + if (options.envVar !== undefined) { + const override = env[options.envVar]; + if (override !== undefined && override.trim() !== '') { + if (!/^[1-9][0-9]*$/.test(override.trim())) { + throw new Error( + `${options.envVar} must be a positive integer, got: ${override}`, + ); + } + // Deliberately not clamped: an override exists so a human can exceed the + // default, most often to pin a run to 1 while chasing a flake. + return Number.parseInt(override.trim(), 10); + } + } + + if (platform === 'darwin' && env['CI'] === 'true') { + return MIN_TEST_CONCURRENCY; + } + + const cores = options.cores ?? availableParallelism(); + const half = Math.floor(cores / 2); + return Math.min(ceiling, Math.max(MIN_TEST_CONCURRENCY, half)); +} diff --git a/scripts/run_bun_tests.ts b/scripts/run_bun_tests.ts index 4cff9dd584..0ed2436fa8 100644 --- a/scripts/run_bun_tests.ts +++ b/scripts/run_bun_tests.ts @@ -41,6 +41,7 @@ import { import { dirname, resolve, join } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { resolveBunTestFiles, type BunTestFile } from './bun-test-roots.js'; +import { DEFAULT_PER_TEST_TIMEOUT_MS } from './lib/bun-test-policy.js'; import { buildVitestJsonReport, parseJUnitXml, @@ -328,7 +329,10 @@ function parseArgs(argv: string[]): CliOptions { const options: CliOptions = { workspace: null, tsconfig: null, - timeout: 30_000, + // Shared with the workspace runners (issue #3139): the two paths both run + // in CI, and a shard that used a tighter bound than the workspace runner + // failed work the workspace runner would have let finish. + timeout: DEFAULT_PER_TEST_TIMEOUT_MS, dryRun: false, junit: null, jsonReport: null, diff --git a/scripts/tests/bun-test-policy.bun.test.ts b/scripts/tests/bun-test-policy.bun.test.ts new file mode 100644 index 0000000000..be114765c0 --- /dev/null +++ b/scripts/tests/bun-test-policy.bun.test.ts @@ -0,0 +1,196 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Behavioral tests for the shared Bun test scheduling policy (issue #3139). + * + * The policy exists because four runners disagreeing about concurrency and + * timeouts made CI green on only 45% of first attempts. These tests pin the + * behaviour that fixes that, and the last suite pins the invariant that let + * the divergence happen in the first place: a runner may not spawn `bun test` + * without an explicit `--timeout`. + */ + +import { describe, it, expect } from 'bun:test'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { + DEFAULT_PER_FILE_TIMEOUT_MS, + DEFAULT_PER_TEST_TIMEOUT_MS, + MAX_TEST_CONCURRENCY, + MIN_TEST_CONCURRENCY, + resolveTestConcurrency, +} from '../lib/bun-test-policy.js'; + +const REPO_ROOT = join(import.meta.dir, '..', '..'); + +/** Every workspace runner that spawns `bun test` children. */ +const RUNNERS: string[] = [ + 'packages/agents/run-bun-tests.ts', + 'packages/cli/run-bun-tests.ts', + 'packages/core/run-bun-tests.ts', + 'packages/auth/run-bun-tests.ts', +]; + +function readRunner(runner: string): string { + return readFileSync(join(REPO_ROOT, runner), 'utf8'); +} + +describe('resolveTestConcurrency (issue #3139)', () => { + const linux: NodeJS.Platform = 'linux'; + + it('uses half the cores so a bun child is not fighting another for a core', () => { + expect(resolveTestConcurrency({ cores: 8, platform: linux, env: {} })).toBe( + 4, + ); + expect(resolveTestConcurrency({ cores: 4, platform: linux, env: {} })).toBe( + 2, + ); + }); + + it('clamps to the shared ceiling on a large machine', () => { + expect( + resolveTestConcurrency({ cores: 64, platform: linux, env: {} }), + ).toBe(MAX_TEST_CONCURRENCY); + }); + + it('never returns zero on a single-core machine', () => { + expect(resolveTestConcurrency({ cores: 1, platform: linux, env: {} })).toBe( + MIN_TEST_CONCURRENCY, + ); + }); + + it('honours a lower per-runner ceiling', () => { + expect( + resolveTestConcurrency({ + cores: 16, + platform: linux, + env: {}, + maxConcurrency: 2, + }), + ).toBe(2); + }); + + it('runs one file at a time on macOS CI', () => { + expect( + resolveTestConcurrency({ + cores: 12, + platform: 'darwin', + env: { CI: 'true' }, + }), + ).toBe(MIN_TEST_CONCURRENCY); + }); + + it('does not serialize macOS outside CI', () => { + expect( + resolveTestConcurrency({ cores: 12, platform: 'darwin', env: {} }), + ).toBe(MAX_TEST_CONCURRENCY); + }); + + it('lets an env override exceed the default, for pinning a run while chasing a flake', () => { + expect( + resolveTestConcurrency({ + cores: 4, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: '9' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toBe(9); + }); + + it('lets an env override pin a run to one file', () => { + expect( + resolveTestConcurrency({ + cores: 16, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: '1' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toBe(1); + }); + + it('ignores an unset or blank override rather than treating it as zero', () => { + expect( + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: ' ' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toBe(4); + }); + + it('rejects a non-numeric override instead of silently falling back', () => { + expect(() => + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: 'lots' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toThrow('must be a positive integer'); + }); + + it('rejects a zero override, which would run nothing', () => { + expect(() => + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: '0' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toThrow('must be a positive integer'); + }); + + it('rejects a nonsensical per-runner ceiling', () => { + expect(() => + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: {}, + maxConcurrency: 0, + }), + ).toThrow('maxConcurrency'); + }); +}); + +describe('shared timeout budgets (issue #3139)', () => { + it('gives a test more than the 30s bound that was measured to cut work off', () => { + expect(DEFAULT_PER_TEST_TIMEOUT_MS).toBeGreaterThan(60_000); + }); + + it('keeps the whole-file backstop above the per-test budget so a hang is still caught', () => { + expect(DEFAULT_PER_FILE_TIMEOUT_MS).toBeGreaterThan( + DEFAULT_PER_TEST_TIMEOUT_MS, + ); + }); +}); + +describe('runner invariants (issue #3139)', () => { + it('every runner passes an explicit --timeout to bun test', () => { + // Bun 1.3.14 ignores a `[test] timeout` key in bunfig.toml and falls back + // to 5s, so the flag is the only thing that actually sets the budget. The + // auth runner shipped without it and therefore ran on 5s. + const missing = RUNNERS.filter( + (runner) => !readRunner(runner).includes("'--timeout'"), + ); + expect(missing).toStrictEqual([]); + }); + + it('every runner derives its policy from the shared module', () => { + const missing = RUNNERS.filter( + (runner) => !readRunner(runner).includes('bun-test-policy.js'), + ); + expect(missing).toStrictEqual([]); + }); + + it('no runner recomputes concurrency from availableParallelism itself', () => { + const offenders = RUNNERS.filter((runner) => + readRunner(runner).includes('availableParallelism()'), + ); + expect(offenders).toStrictEqual([]); + }); +}); diff --git a/tsconfig.scripts.json b/tsconfig.scripts.json index a72f3f0308..228de8ca59 100644 --- a/tsconfig.scripts.json +++ b/tsconfig.scripts.json @@ -76,6 +76,7 @@ "scripts/bun-test-roots.ts", "scripts/run_bun_tests.ts", "scripts/bun-junit-to-json-report.ts", + "scripts/tests/bun-test-policy.bun.test.ts", "scripts/tests/bun-test-roots.bun.test.ts", "scripts/tests/bun-test-root-ownership.bun.test.ts", "scripts/tests/test-file-coverage.bun.test.ts", From ccb711c289d27df76d979e8dedad6f710aa32ee3 Mon Sep 17 00:00:00 2001 From: acoliver Date: Sat, 8 Aug 2026 00:16:25 -0300 Subject: [PATCH 2/5] Cover the shard runner with the same runner invariants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scripts/run_bun_tests.ts spawns bun test children and was changed by this PR to share the timeout policy, but it was missing from RUNNERS — so the invariants that pin the fix did not actually cover it. Removing its --timeout or its import of the shared module would have gone unnoticed, which is the exact failure this suite exists to prevent. It already satisfies all three invariants; adding it only closes the hole in the coverage. --- scripts/tests/bun-test-policy.bun.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/tests/bun-test-policy.bun.test.ts b/scripts/tests/bun-test-policy.bun.test.ts index be114765c0..0727c766e8 100644 --- a/scripts/tests/bun-test-policy.bun.test.ts +++ b/scripts/tests/bun-test-policy.bun.test.ts @@ -27,12 +27,17 @@ import { const REPO_ROOT = join(import.meta.dir, '..', '..'); -/** Every workspace runner that spawns `bun test` children. */ +/** + * Every runner that spawns `bun test` children — the workspace runners and the + * shard runner CI invokes alongside them. Both paths run in CI, and it was the + * disagreement between them that gave the same test two different budgets. + */ const RUNNERS: string[] = [ 'packages/agents/run-bun-tests.ts', 'packages/cli/run-bun-tests.ts', 'packages/core/run-bun-tests.ts', 'packages/auth/run-bun-tests.ts', + 'scripts/run_bun_tests.ts', ]; function readRunner(runner: string): string { From 88f4a7b9a3e5bf8519a5d9021f9aec0f3357c25b Mon Sep 17 00:00:00 2001 From: acoliver Date: Sat, 8 Aug 2026 00:33:15 -0300 Subject: [PATCH 3/5] Correct the core runner's stale timeout documentation The module comment still described a per-file timeout of 60s on POSIX and 180s on Windows. Those values moved to the shared policy in this PR, so the comment now contradicted the code it sits above. --- packages/core/run-bun-tests.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/core/run-bun-tests.ts b/packages/core/run-bun-tests.ts index d205b62974..91c4141866 100644 --- a/packages/core/run-bun-tests.ts +++ b/packages/core/run-bun-tests.ts @@ -13,9 +13,12 @@ * file as a separate `bun test ` invocation avoids the multi-file * process management that triggers the hang. * - * Each child process has a per-file timeout (60s on POSIX, 180s on Windows). - * If a file takes longer, the process is killed to prevent a single - * slow/hanging file from blocking the entire suite. + * Concurrency and both timeout budgets come from + * scripts/lib/bun-test-policy.ts, shared with the other runners (issue #3139). + * This workspace keeps its own lower concurrency cap because its files are + * unusually heavy; the budgets are the shared ones. If a file exceeds the + * per-file budget the process is killed, so a single hanging file cannot block + * the suite. * * Exit code is 0 if all files pass, 1 if any file fails. */ From aadb49b7b600fee08bb1ec3009ad64f8dc92d385 Mon Sep 17 00:00:00 2001 From: acoliver Date: Sat, 8 Aug 2026 01:09:11 -0300 Subject: [PATCH 4/5] Reap a timed-out child before freeing its worker slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit caught that auth reported a timed-out file from inside the timeout callback. kill() only sends a signal, so the result was produced while the child was still winding down — and because this PR converted auth from fixed batches to a worker pool, the freed slot is filled immediately. The pool would exceed its concurrency cap exactly when the machine is already struggling, which is the failure this PR exists to remove. cli had the same defect and already used a worker pool, so it was doing this in production. Both now settle from the exit handler and carry the timeout reason on a flag; core already awaited the reap and agents already settled from exit, so those were correct. killProcessTree sends SIGKILL to the process group, which cannot be ignored, so exit is guaranteed to arrive. Also rejects a concurrency override too large to be an exact integer: the digit-shape check accepted an arbitrarily long run of digits, which parseInt rounds to an imprecise Number that would then size the worker pool. I dropped the source-scanning test I first wrote for the reaping rule. core resolves inside its timeout callback too, but only after awaiting the kill, which is correct — and no text heuristic distinguishes that from resolving immediately. A guard that cannot tell right from wrong is worse than none, so the requirement is documented on the budget it belongs to instead. --- packages/auth/run-bun-tests.ts | 27 ++++++++++++----------- packages/cli/run-bun-tests.ts | 18 ++++++++++----- scripts/lib/bun-test-policy.ts | 20 +++++++++++++++-- scripts/tests/bun-test-policy.bun.test.ts | 24 ++++++++++++++++++++ 4 files changed, 68 insertions(+), 21 deletions(-) diff --git a/packages/auth/run-bun-tests.ts b/packages/auth/run-bun-tests.ts index f1bc436a4d..d802edd5ec 100644 --- a/packages/auth/run-bun-tests.ts +++ b/packages/auth/run-bun-tests.ts @@ -111,17 +111,18 @@ export function runTestFile(file: string): Promise { }, ); + // Set by the timer so the exit handler can report the real reason. The + // result is only produced once the process has actually been reaped: + // `kill()` only sends a signal and returns, so resolving from the timer + // would free the worker slot while the killed process was still running, + // letting the pool exceed its concurrency cap exactly when the machine is + // already struggling. This mattered less under the old fixed batches; with + // a worker pool the freed slot is filled immediately. + let killedByTimeout = false; + const timer = setTimeout(() => { - if (resolved) return; - resolved = true; + killedByTimeout = true; child.kill('SIGKILL'); - resolve({ - file, - passed: false, - exitCode: null, - timedOut: true, - signal: null, - }); }, PER_FILE_TIMEOUT_MS); child.on('exit', (code, signal) => { @@ -130,10 +131,10 @@ export function runTestFile(file: string): Promise { clearTimeout(timer); resolve({ file, - passed: code === 0, - exitCode: code, - timedOut: false, - signal: signal ?? null, + passed: !killedByTimeout && code === 0, + exitCode: killedByTimeout ? null : code, + timedOut: killedByTimeout, + signal: killedByTimeout ? null : (signal ?? null), }); }); diff --git a/packages/cli/run-bun-tests.ts b/packages/cli/run-bun-tests.ts index 10bdac8b82..d10dd04fab 100644 --- a/packages/cli/run-bun-tests.ts +++ b/packages/cli/run-bun-tests.ts @@ -199,11 +199,17 @@ function runTestFile(file: string): Promise { output += chunk.toString(); }); + // Set by the timer so the exit handler can report the real reason. The + // result is only produced once the process has actually exited: killing a + // tree only signals it, so resolving from the timer would free this worker + // slot while the tree was still winding down. The pool would then exceed + // its concurrency cap exactly when the machine is already struggling — + // which is how a timeout on one file turns into timeouts on others. + let killedByTimeout = false; + const timer = setTimeout(() => { - if (settled) return; - settled = true; + killedByTimeout = true; killProcessTree(child); - resolve({ file, passed: false, exitCode: null, timedOut: true, output }); }, fileTimeoutForFile(file)); child.on('exit', (code) => { @@ -212,9 +218,9 @@ function runTestFile(file: string): Promise { clearTimeout(timer); resolve({ file, - passed: code === 0, - exitCode: code, - timedOut: false, + passed: !killedByTimeout && code === 0, + exitCode: killedByTimeout ? null : code, + timedOut: killedByTimeout, output, }); }); diff --git a/scripts/lib/bun-test-policy.ts b/scripts/lib/bun-test-policy.ts index 6270eb3656..8167b0ec39 100644 --- a/scripts/lib/bun-test-policy.ts +++ b/scripts/lib/bun-test-policy.ts @@ -42,6 +42,15 @@ export const DEFAULT_PER_TEST_TIMEOUT_MS = 180_000; * Whole-file budget, and the backstop that keeps a raised per-test bound from * turning a genuine hang into a longer hang. Sized to admit a slow but * progressing file rather than to bound total runtime. + * + * A runner enforcing this must not report the file's result from inside the + * timeout callback. Killing a process — or a process tree — only signals it; + * resolving straight away frees the worker slot while the child is still + * winding down, so the pool exceeds its concurrency cap exactly when the + * machine is already struggling, and a timeout on one file becomes timeouts on + * others. Wait for the child to be reaped first, either by settling from the + * `exit` handler (see the agents, cli and auth runners) or by awaiting the kill + * (see the core runner). */ export const DEFAULT_PER_FILE_TIMEOUT_MS = 300_000; @@ -116,14 +125,21 @@ export function resolveTestConcurrency( if (options.envVar !== undefined) { const override = env[options.envVar]; if (override !== undefined && override.trim() !== '') { - if (!/^[1-9][0-9]*$/.test(override.trim())) { + const parsed = Number.parseInt(override.trim(), 10); + // The digit-shape check alone would accept an arbitrarily long run of + // digits, which parseInt rounds to an imprecise Number or to Infinity; + // that would silently become the size of the worker pool. + if ( + !/^[1-9][0-9]*$/.test(override.trim()) || + !Number.isSafeInteger(parsed) + ) { throw new Error( `${options.envVar} must be a positive integer, got: ${override}`, ); } // Deliberately not clamped: an override exists so a human can exceed the // default, most often to pin a run to 1 while chasing a flake. - return Number.parseInt(override.trim(), 10); + return parsed; } } diff --git a/scripts/tests/bun-test-policy.bun.test.ts b/scripts/tests/bun-test-policy.bun.test.ts index 0727c766e8..2595c8b433 100644 --- a/scripts/tests/bun-test-policy.bun.test.ts +++ b/scripts/tests/bun-test-policy.bun.test.ts @@ -139,6 +139,30 @@ describe('resolveTestConcurrency (issue #3139)', () => { ).toThrow('must be a positive integer'); }); + it('rejects an override too large to be an exact integer', () => { + // All digits, so the shape check alone accepts it, but parseInt rounds it + // to an imprecise Number — which would then size the worker pool. + expect(() => + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: '99999999999999999999999' }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toThrow('must be a positive integer'); + }); + + it('accepts the largest exactly representable override', () => { + expect( + resolveTestConcurrency({ + cores: 8, + platform: linux, + env: { LLXPRT_TEST_CONCURRENCY: String(Number.MAX_SAFE_INTEGER) }, + envVar: 'LLXPRT_TEST_CONCURRENCY', + }), + ).toBe(Number.MAX_SAFE_INTEGER); + }); + it('rejects a zero override, which would run nothing', () => { expect(() => resolveTestConcurrency({ From 684c134d1db3800ee8124174efcae363137c045e Mon Sep 17 00:00:00 2001 From: acoliver Date: Sat, 8 Aug 2026 01:45:15 -0300 Subject: [PATCH 5/5] Note that availableParallelism reports cores, not this process's share A full local suite on a machine running several checkouts at once still timed out two agents files that pass in about a second standalone. That is not something the policy can size around: availableParallelism() reports the machine's cores, so half of them is still more than the runner actually has when other work already owns the box. CI runners are dedicated, so the two coincide there and the issue this fixes is unaffected. The env override is the escape hatch on a shared machine, and the comment now says so rather than leaving the next person to rediscover it. --- scripts/lib/bun-test-policy.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/scripts/lib/bun-test-policy.ts b/scripts/lib/bun-test-policy.ts index 8167b0ec39..54fdf6da07 100644 --- a/scripts/lib/bun-test-policy.ts +++ b/scripts/lib/bun-test-policy.ts @@ -147,6 +147,11 @@ export function resolveTestConcurrency( return MIN_TEST_CONCURRENCY; } + // availableParallelism() reports the machine's cores, not this process's + // share of them. On a CI runner — which is dedicated — those are the same + // thing. On a development machine running several checkouts at once they are + // not, and half of the cores is still more than the runner actually has; the + // env override above is the escape hatch for that case. const cores = options.cores ?? availableParallelism(); const half = Math.floor(cores / 2); return Math.min(ceiling, Math.max(MIN_TEST_CONCURRENCY, half));