From 4d0ef970d9b271b4fe7bcc0b0489b67777007764 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 10:38:07 +0000 Subject: [PATCH 01/12] test(ci): single-retry policy for enumerated contention-flaky files --- .github/workflows/ci.yml | 18 +- AGENTS.md | 4 +- docs/agents/testing.md | 26 ++ package.json | 4 +- scripts/lib/contention-retry-lane.ts | 146 +++++++++ scripts/lib/contention-retry-policy.test.ts | 196 ++++++++++++ scripts/lib/contention-retry-run.ts | 143 +++++++++ scripts/lib/contention-retry.ts | 321 ++++++++++++++++++++ vitest.config.ts | 22 +- 9 files changed, 862 insertions(+), 18 deletions(-) create mode 100644 scripts/lib/contention-retry-lane.ts create mode 100644 scripts/lib/contention-retry-policy.test.ts create mode 100644 scripts/lib/contention-retry-run.ts create mode 100644 scripts/lib/contention-retry.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9ec149dca..37b354937 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -252,10 +252,26 @@ jobs: - name: Test changed-line coverage gate run: pnpm check:coverage-changed:test + # The retry list is an enumerated set of owned waivers, so an expired entry + # must fail before the suite runs rather than quietly keeping its retry. + - name: Check contention retry policy + run: pnpm check:contention-retry + + # Wrapped in the single-retry policy (#1419): a timeout-shaped failure in + # an enumerated contention-flaky file reruns that file once and reports it + # in the job summary. Assertion failures fail here on the first run. - name: Run coverage env: OUTPUT_ECONOMY_BASE: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }} - run: pnpm test:coverage + run: pnpm test:coverage:ci + + - name: Upload contention-retry envelope + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: contention-retry-envelope + if-no-files-found: ignore + path: .tmp/contention-retry/lane-envelope.json # Reuses the lcov the coverage step just wrote (never runs coverage twice) # and fails when changed-line coverage < the threshold in diff --git a/AGENTS.md b/AGENTS.md index 6e1e28c92..5b01b172d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -224,7 +224,9 @@ connect errors, retry policy, or command typing, start in - Contention flakes: `request-handler-catalog` ("specialized daemon routes...") and the doctor provider scenario time out under host load. Before believing a regression, rerun in isolation AND reproduce on plain `origin/main` under the same load. A changing failure set that passes in - isolation is contention, not your change. + isolation is contention, not your change. The files CI may rerun once for a timeout-shaped failure + are enumerated in `scripts/lib/contention-retry.ts` (docs/agents/testing.md, "Contention retry + policy"). ## Docs & skills diff --git a/docs/agents/testing.md b/docs/agents/testing.md index a35e808d4..6df7a00d7 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -380,6 +380,32 @@ Use `AGENT_DEVICE_IOS_E2E_TIER=full` for the nightly subset. Step history, cover screenshots, recordings, traces, and failure context are written below `test/artifacts/ios-simulator/` and uploaded by the existing shared artifact action. The six Settings replays remain additive OS-chrome coverage and are not modified by this suite. +## Contention retry policy (enumerated, timeouts only) + +Some test files stub a real binary and then spawn or wait on it, so under host load they fail for a +reason that has nothing to do with the diff. The set of such files is **enumerated** in +`scripts/lib/contention-retry.ts` (`CONTENTION_RETRY_FILES`) — never a glob, which would silently +enroll every future file under a directory. That one constant also derives `SUBPROCESS_STUB_TESTS`, +the serialized `subprocess-stub` Vitest project in `vitest.config.ts`, so the execution contract and +the retry policy cannot drift apart. + +The CI Coverage job runs the suite through `pnpm test:coverage:ci` +(`scripts/lib/contention-retry-run.ts`), which applies one rule: + +- **Timeouts only.** A rerun happens only when *every* failure in the run is timeout-shaped **and** + lands in a listed file. One assertion failure — in a listed file or not — fails the job on the + first run, so a real regression can never be papered over by a retry. +- **One retry, of the failed files only.** Not the suite, and never twice. +- **Retries stay visible.** Every retried file is named in the job summary with its tracking issue + and review date, and the run writes the shared scheduled-lane envelope + (`scripts/lib/lane-envelope.ts`, #1430) with the retry count, so a permanently flaky file shows up + as lane health rather than as a green check. + +Adding a file to the list is a reviewed waiver in the ADR 0011 sense: a `reason` naming the concrete +spawn/wait that makes it contention-flaky, a `trackingIssue` for removing that wait, and a `reviewBy` +date. `pnpm check:contention-retry` (its own CI step, and part of `pnpm check:unit`) fails on an +expired entry, a missing file, or a glob, so an entry is renewed or removed rather than inherited. + ## Speed rules (experiment-backed, 2026-07-04) Measured on the full unit suite (340 files, 3,210 tests, 48s wall at ~7x parallelism): diff --git a/package.json b/package.json index 21595b77e..39758c666 100644 --- a/package.json +++ b/package.json @@ -142,7 +142,7 @@ "check:mcp-metadata": "node scripts/sync-mcp-metadata.mjs --check", "version": "node scripts/sync-mcp-metadata.mjs && git add server.json", "check:tooling": "pnpm lint && pnpm typecheck && pnpm check:layering && pnpm depgraph:test && pnpm check:production-exports && pnpm check:mcp-metadata && pnpm build && pnpm check:bundle-owner-files", - "check:unit": "pnpm test:unit && pnpm test:smoke", + "check:unit": "pnpm check:contention-retry && pnpm test:unit && pnpm test:smoke", "check": "pnpm check:tooling && pnpm check:fallow && pnpm check:unit", "prepack": "pnpm check:mcp-metadata && pnpm build:all && pnpm package:apple-runner:npm && pnpm package:android-snapshot-helper:npm && pnpm package:android-ime-helper:npm", "typecheck": "tsc -p tsconfig.json && tsc -p examples/sdk/tsconfig.json", @@ -159,6 +159,8 @@ "test": "vitest run --project unit-core --project subprocess-stub", "test:unit": "vitest run --project unit-core --project subprocess-stub", "test:coverage": "vitest run --coverage", + "test:coverage:ci": "node --experimental-strip-types scripts/lib/contention-retry-run.ts --coverage", + "check:contention-retry": "node --experimental-strip-types --test scripts/lib/contention-retry-policy.test.ts", "test:integration:provider": "vitest run --project provider-integration", "test:integration:progress": "node --experimental-strip-types scripts/integration-progress.ts", "test:integration:progress:check": "node --experimental-strip-types scripts/integration-progress.ts --check", diff --git a/scripts/lib/contention-retry-lane.ts b/scripts/lib/contention-retry-lane.ts new file mode 100644 index 000000000..5211f6826 --- /dev/null +++ b/scripts/lib/contention-retry-lane.ts @@ -0,0 +1,146 @@ +// Orchestration for the single-retry policy (#1419): run the suite, and when it +// fails only with timeout-shaped failures in enumerated files, rerun exactly +// those files once. Kept free of process spawning and file writing so both +// acceptance cases — an injected assertion failure fails on the first run, an +// injected timeout passes on retry — are driven directly in the gate test. + +import { laneEnvelope, type LaneEnvelope } from './lane-envelope.ts'; +import { + CONTENTION_RETRY_FILES, + expiredRetryEntries, + formatRetrySummary, + normalizeTestFile, + planContentionRetry, + type RetryOutcome, + type RetryPlan, + type TestFailure, +} from './contention-retry.ts'; + +export type TestRun = { + ok: boolean; + failures: readonly TestFailure[]; +}; + +/** Telemetry payload for scheduled-lane health (#1430). */ +export type ContentionRetryTelemetry = { + /** Files rerun in this job, with the test that timed out. */ + retried: ReadonlyArray<{ file: string; testName: string; trackingIssue: string }>; + retryCount: number; + retryOutcome: RetryOutcome | null; + listSize: number; +}; + +export type ContentionRetryResult = { + ok: boolean; + summary: string; + envelope: LaneEnvelope; +}; + +export type ContentionRetryOptions = { + /** Runs the whole suite. */ + runAll: () => Promise; + /** Reruns exactly the given files. */ + runFiles: (files: readonly string[]) => Promise; + commit: string; + configHash: string; + vitestVersion: string; + startedAtMs: number; + now?: () => number; + /** Clock for the waiver expiry gate. */ + today?: Date; +}; + +const LANE_ID = 'unit-contention-retry'; + +export async function runWithContentionRetry( + options: ContentionRetryOptions, +): Promise { + const today = options.today ?? new Date(); + const expired = expiredRetryEntries(today); + if (expired.length > 0) { + const lines = expired.map( + (entry) => `- \`${entry.file}\` (review by ${entry.reviewBy}, ${entry.trackingIssue})`, + ); + return finish(options, { + ok: false, + summary: [ + '## Contention retry (#1419)', + '', + 'Retry list expired — renew the review date or remove the entry:', + '', + ...lines, + '', + ].join('\n'), + plan: undefined, + outcome: null, + }); + } + + const first = await options.runAll(); + if (first.ok) { + return finish(options, { ok: true, summary: '', plan: undefined, outcome: null }); + } + + const plan = planContentionRetry(first.failures); + if (!plan.retry) { + return finish(options, { + ok: false, + summary: formatRetrySummary({ plan }), + plan, + outcome: null, + }); + } + + const retried = await options.runFiles(plan.files); + const outcome: RetryOutcome = retried.ok ? 'passed' : 'failed'; + return finish(options, { + ok: retried.ok, + summary: formatRetrySummary({ plan, outcome }), + plan, + outcome, + }); +} + +function finish( + options: ContentionRetryOptions, + state: { + ok: boolean; + summary: string; + plan: RetryPlan | undefined; + outcome: RetryOutcome | null; + }, +): ContentionRetryResult { + const plan = state.plan; + const retried = plan?.retry + ? plan.failures.map((failure) => { + const file = normalizeTestFile(failure.file); + return { + file, + testName: failure.testName, + trackingIssue: + CONTENTION_RETRY_FILES.find((entry) => entry.file === file)?.trackingIssue ?? '', + }; + }) + : []; + return { + ok: state.ok, + summary: state.summary, + envelope: laneEnvelope({ + lane: LANE_ID, + commit: options.commit, + tool: { vitest: options.vitestVersion }, + configHash: options.configHash, + startedAtMs: options.startedAtMs, + now: options.now?.(), + // The retry set is enumerated, not randomized. + seed: null, + result: state.ok ? 'pass' : 'fail', + data: { + retried, + retryCount: retried.length, + retryOutcome: state.outcome, + listSize: CONTENTION_RETRY_FILES.length, + }, + }), + }; +} diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts new file mode 100644 index 000000000..9c5301f0b --- /dev/null +++ b/scripts/lib/contention-retry-policy.test.ts @@ -0,0 +1,196 @@ +// Gate for the contention single-retry policy (#1419). A retry that can hide an +// assertion failure is worse than no retry at all, so both halves are asserted +// here: the shape of the enumerated list (owned waivers, real files, no globs) +// and the decisions the lane makes from a failed run. + +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { test } from 'node:test'; +import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; +import { + CONTENTION_RETRY_FILES, + expiredRetryEntries, + formatRetrySummary, + isTimeoutShapedFailure, + planContentionRetry, + SUBPROCESS_STUB_TESTS, + type TestFailure, +} from './contention-retry.ts'; + +const repoRoot = path.resolve(import.meta.dirname, '../..'); + +const LISTED = 'src/daemon/__tests__/request-router-open.test.ts'; +const TIMEOUT_MESSAGE = 'Error: Test timed out in 5000ms.\n at open()'; +const ASSERTION_MESSAGE = 'AssertionError: expected "DEVICE_IN_USE" to be "OK"'; + +function failure(overrides: Partial = {}): TestFailure { + return { file: LISTED, testName: 'opens a session', message: TIMEOUT_MESSAGE, ...overrides }; +} + +function lane(runs: { first: TestRun; retry?: TestRun; today?: Date }): { + result: ReturnType; + rerun: string[][]; +} { + const rerun: string[][] = []; + const result = runWithContentionRetry({ + runAll: () => Promise.resolve(runs.first), + runFiles: (files) => { + rerun.push([...files]); + return Promise.resolve(runs.retry ?? { ok: true, failures: [] }); + }, + commit: 'c'.repeat(40), + configHash: 'sha256:deadbeef', + vitestVersion: '4.1.8', + startedAtMs: 0, + now: () => 1_000, + today: runs.today, + }); + return { result, rerun }; +} + +test('every retry-list entry is an owned waiver naming why the file spawns or waits', () => { + for (const entry of CONTENTION_RETRY_FILES) { + assert.ok( + fs.existsSync(path.join(repoRoot, entry.file)), + `${entry.file} does not exist — remove the retry entry with the test`, + ); + assert.ok( + !/[*?{}[\]]/.test(entry.file), + `${entry.file} looks like a glob; the retry set is enumerated`, + ); + assert.match( + entry.reason, + /spawn|wait|poll|socket/i, + `${entry.file} must name the spawn/wait that makes it contention-flaky`, + ); + assert.match( + entry.trackingIssue, + /^https:\/\/github\.com\/callstack\/agent-device\/issues\/\d+$/, + ); + assert.match(entry.reviewBy, /^\d{4}-\d{2}-\d{2}$/); + } + const files = CONTENTION_RETRY_FILES.map((entry) => entry.file); + assert.equal(new Set(files).size, files.length, 'duplicate retry entries'); +}); + +test('the repeat offenders and every subprocess-stub file are retry-eligible', () => { + const files = new Set(CONTENTION_RETRY_FILES.map((entry) => entry.file)); + for (const offender of [ + 'src/daemon/__tests__/request-router-open.test.ts', + 'src/platforms/apple/core/__tests__/runner-client.test.ts', + 'src/platforms/apple/core/__tests__/runner-xctestrun.test.ts', + 'scripts/__tests__/help-conformance-bench.test.ts', + ]) { + assert.ok(files.has(offender), `${offender} must stay retry-eligible`); + } + for (const stub of SUBPROCESS_STUB_TESTS) assert.ok(files.has(stub)); + assert.ok(SUBPROCESS_STUB_TESTS.length < CONTENTION_RETRY_FILES.length); +}); + +test('vitest projects read the shared constant instead of re-listing globs', () => { + const config = fs.readFileSync(path.join(repoRoot, 'vitest.config.ts'), 'utf8'); + assert.match(config, /from '\.\/scripts\/lib\/contention-retry\.ts'/); +}); + +test('an expired waiver fails the gate; the committed list is not expired', () => { + assert.deepEqual(expiredRetryEntries(new Date()), []); + const expired = expiredRetryEntries(new Date('2099-01-01T00:00:00Z')); + assert.equal(expired.length, CONTENTION_RETRY_FILES.length); +}); + +test('the expiry gate fails the run before any test executes', async () => { + const { result, rerun } = lane({ + first: { ok: false, failures: [failure()] }, + today: new Date('2099-01-01T00:00:00Z'), + }); + const resolved = await result; + assert.equal(resolved.ok, false); + assert.match(resolved.summary, /Retry list expired/); + assert.deepEqual(rerun, []); +}); + +test('timeout shapes are recognized and assertion failures are not', () => { + assert.ok(isTimeoutShapedFailure(TIMEOUT_MESSAGE)); + assert.ok(isTimeoutShapedFailure('Error: Hook timed out in 10000 ms.')); + assert.ok(!isTimeoutShapedFailure(ASSERTION_MESSAGE)); + assert.ok(!isTimeoutShapedFailure('Error: expected timeout hint to be set')); +}); + +test('a timeout in a listed file retries exactly that file, once', () => { + const plan = planContentionRetry([failure()]); + assert.deepEqual(plan, { retry: true, files: [LISTED], failures: [failure()] }); +}); + +test('an assertion failure in a listed file never retries', () => { + const plan = planContentionRetry([failure({ message: ASSERTION_MESSAGE })]); + assert.equal(plan.retry, false); + assert.match(plan.reason, /assertion failures never retry/); +}); + +test('a timeout outside the list never retries, even alongside eligible ones', () => { + const plan = planContentionRetry([ + failure(), + failure({ file: 'src/daemon/__tests__/session-store.test.ts' }), + ]); + assert.equal(plan.retry, false); + assert.match(plan.reason, /outside the enumerated retry list/); +}); + +test('an injected assertion failure in a listed file fails the job on the first run', async () => { + const { result, rerun } = lane({ + first: { ok: false, failures: [failure({ message: ASSERTION_MESSAGE })] }, + }); + const resolved = await result; + assert.equal(resolved.ok, false); + assert.deepEqual(rerun, [], 'an assertion failure must never be rerun'); + assert.equal(resolved.envelope.data.retryCount, 0); + assert.equal(resolved.envelope.result, 'fail'); +}); + +test('an injected timeout in a listed file passes on retry with a visible summary line', async () => { + const { result, rerun } = lane({ first: { ok: false, failures: [failure()] } }); + const resolved = await result; + assert.equal(resolved.ok, true); + assert.deepEqual(rerun, [[LISTED]]); + assert.match( + resolved.summary, + /Retried 1 timeout-shaped file\(s\) once — outcome: \*\*passed\*\*/, + ); + assert.match(resolved.summary, new RegExp(LISTED.replaceAll('.', '\\.'))); + assert.equal(resolved.envelope.data.retryCount, 1); + assert.equal(resolved.envelope.data.retryOutcome, 'passed'); + assert.equal(resolved.envelope.result, 'pass'); +}); + +test('a file that fails again after its one retry fails the job', async () => { + const { result } = lane({ + first: { ok: false, failures: [failure()] }, + retry: { ok: false, failures: [failure()] }, + }); + const resolved = await result; + assert.equal(resolved.ok, false); + assert.match(resolved.summary, /outcome: \*\*failed\*\*/); + assert.equal(resolved.envelope.data.retryOutcome, 'failed'); +}); + +test('a green run reports no retry and still emits lane telemetry', async () => { + const { result } = lane({ first: { ok: true, failures: [] } }); + const resolved = await result; + assert.equal(resolved.ok, true); + assert.equal(resolved.summary, ''); + assert.equal(resolved.envelope.lane, 'unit-contention-retry'); + assert.equal(resolved.envelope.data.retryCount, 0); + assert.equal(resolved.envelope.data.listSize, CONTENTION_RETRY_FILES.length); +}); + +test('the summary names the tracking issue and review date of every retried file', () => { + const summary = formatRetrySummary({ + plan: planContentionRetry([failure()]), + outcome: 'passed', + }); + const entry = CONTENTION_RETRY_FILES.find((candidate) => candidate.file === LISTED); + assert.ok(entry); + assert.match(summary, new RegExp(entry.trackingIssue.replaceAll('/', '\\/'))); + assert.match(summary, new RegExp(entry.reviewBy)); +}); diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts new file mode 100644 index 000000000..0340cc852 --- /dev/null +++ b/scripts/lib/contention-retry-run.ts @@ -0,0 +1,143 @@ +// CI entrypoint for the contention single-retry policy (#1419). +// +// node --experimental-strip-types scripts/lib/contention-retry-run.ts --coverage +// +// Runs Vitest once; if the only failures are timeout-shaped and live in the +// enumerated retry list (scripts/lib/contention-retry.ts), reruns exactly those +// files once. Every retried file is named in the job summary, and the run writes +// the shared scheduled-lane envelope so retry counts feed lane health (#1430). + +import crypto from 'node:crypto'; +import fs from 'node:fs'; +import path from 'node:path'; +import { runCmdStreaming, runCmdSync } from '../../src/utils/exec.ts'; +import { parseScriptArgs } from './cli-args.ts'; +import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; +import { normalizeTestFile, type TestFailure } from './contention-retry.ts'; + +const USAGE = `Usage: node --experimental-strip-types scripts/lib/contention-retry-run.ts [options] + + --coverage Run the full suite under coverage (the CI Coverage job) + --project Vitest projects for the first run (default: all) + --report Vitest JSON report path (default: .tmp/contention-retry/report.json) + --envelope Lane envelope path (default: .tmp/contention-retry/lane-envelope.json) + --summary Markdown summary sink (default: $GITHUB_STEP_SUMMARY) +`; + +const repoRoot = path.resolve(import.meta.dirname, '../..'); +const DEFAULT_REPORT = '.tmp/contention-retry/report.json'; +const DEFAULT_ENVELOPE = '.tmp/contention-retry/lane-envelope.json'; + +const args = parseScriptArgs(process.argv.slice(2), USAGE, { + coverage: { type: 'boolean', default: false }, + project: { type: 'string' }, + report: { type: 'string', default: DEFAULT_REPORT }, + envelope: { type: 'string', default: DEFAULT_ENVELOPE }, + summary: { type: 'string' }, +}); + +const reportPath = path.resolve(repoRoot, args.report); +const envelopePath = path.resolve(repoRoot, args.envelope); +const summaryPath = args.summary ?? process.env.GITHUB_STEP_SUMMARY; + +function reporterArgs(report: string): string[] { + return ['--reporter=default', '--reporter=json', `--outputFile.json=${report}`]; +} + +async function runVitest(extra: string[], report: string): Promise { + fs.mkdirSync(path.dirname(report), { recursive: true }); + fs.rmSync(report, { force: true }); + const result = await runCmdStreaming( + 'pnpm', + ['exec', 'vitest', 'run', ...extra, ...reporterArgs(report)], + { + cwd: repoRoot, + allowFailure: true, + onStdoutChunk: (chunk) => process.stdout.write(chunk), + onStderrChunk: (chunk) => process.stderr.write(chunk), + }, + ); + return { ok: result.exitCode === 0, failures: readFailures(report) }; +} + +/** + * Vitest's JSON reporter, narrowed at the trust boundary. A run that dies before + * writing a report yields no failures, which the policy reads as "nothing + * retry-eligible" — the job fails, which is the safe direction. + */ +function readFailures(report: string): readonly TestFailure[] { + if (!fs.existsSync(report)) return []; + const parsed: unknown = JSON.parse(fs.readFileSync(report, 'utf8')); + const suites = (parsed as { testResults?: unknown }).testResults; + if (!Array.isArray(suites)) return []; + const failures: TestFailure[] = []; + for (const suite of suites) { + const { name, assertionResults } = suite as { + name?: unknown; + assertionResults?: unknown; + }; + if (typeof name !== 'string' || !Array.isArray(assertionResults)) continue; + for (const assertion of assertionResults) { + const { status, fullName, title, failureMessages } = assertion as { + status?: unknown; + fullName?: unknown; + title?: unknown; + failureMessages?: unknown; + }; + if (status !== 'failed') continue; + const messages = Array.isArray(failureMessages) ? failureMessages : []; + failures.push({ + file: normalizeTestFile(name, repoRoot), + testName: typeof fullName === 'string' ? fullName : String(title ?? 'unknown test'), + message: messages.filter((message) => typeof message === 'string').join('\n'), + }); + } + } + return failures; +} + +function vitestVersion(): string { + const manifest: unknown = JSON.parse( + fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8'), + ); + const version = (manifest as { devDependencies?: Record }).devDependencies + ?.vitest; + return version ?? 'unknown'; +} + +function configHash(): string { + const hash = crypto.createHash('sha256'); + for (const file of ['vitest.config.ts', 'scripts/lib/contention-retry.ts']) { + hash.update(fs.readFileSync(path.join(repoRoot, file))); + } + return `sha256:${hash.digest('hex').slice(0, 16)}`; +} + +const projects = (args.project ?? '') + .split(',') + .map((entry) => entry.trim()) + .filter(Boolean) + .flatMap((name) => ['--project', name]); + +const startedAtMs = Date.now(); +const result = await runWithContentionRetry({ + runAll: () => runVitest([...projects, ...(args.coverage ? ['--coverage'] : [])], reportPath), + // The rerun is file-scoped and coverage-free on purpose: coverage thresholds + // are a whole-suite verdict, and a threshold miss is not timeout-shaped, so it + // never reaches this branch. + runFiles: (files) => runVitest([...files], `${reportPath}.retry.json`), + commit: runCmdSync('git', ['rev-parse', 'HEAD'], { cwd: repoRoot }).stdout.trim(), + configHash: configHash(), + vitestVersion: vitestVersion(), + startedAtMs, +}); + +fs.mkdirSync(path.dirname(envelopePath), { recursive: true }); +fs.writeFileSync(envelopePath, `${JSON.stringify(result.envelope, null, 2)}\n`); + +if (result.summary) { + process.stdout.write(`\n${result.summary}`); + if (summaryPath) fs.appendFileSync(summaryPath, result.summary); +} + +process.exitCode = result.ok ? 0 : 1; diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts new file mode 100644 index 000000000..ce6a4646a --- /dev/null +++ b/scripts/lib/contention-retry.ts @@ -0,0 +1,321 @@ +// Single-retry policy for contention-flaky test files (issue #1419, umbrella #1412 Track B). +// +// Some unit-test files stub a real binary and then spawn or wait on it, so under +// host load their production code takes a generic timeout path and the file +// fails for a reason that has nothing to do with the diff (AGENTS.md, "Known +// environment traps"). Rerunning the whole suite hides real regressions and +// burns runner minutes, so the policy is deliberately narrow: +// +// - the retry set is ENUMERATED here, never a glob: a glob would silently +// enroll every future file under a directory, +// - only timeout-shaped failures retry; an assertion failure in a listed file +// fails the job on the first run, +// - one retry, of the failed files only, and every retry is reported in the +// job summary so a permanently flaky file cannot hide behind a green check, +// - each entry is an owned waiver in the ADR 0011 sense: a tracking issue plus +// a review date, and an expired entry fails the gate until it is renewed or +// removed. + +/** One enumerated retry-eligible file, owned like an ADR 0011 waiver. */ +export type ContentionRetryEntry = { + /** Repo-relative test file path. Exact path — never a glob. */ + file: string; + /** + * Why this file spawns or waits. Required: the retry list may only grow with a + * concrete contention mechanism named at the entry, not by habit. + */ + reason: string; + /** Issue tracking the removal of the underlying real-time wait. */ + trackingIssue: string; + /** ISO date (YYYY-MM-DD). Past this, the gate fails until renewed or removed. */ + reviewBy: string; + /** + * True for files that also run serialized in the `subprocess-stub` Vitest + * project: they stub a binary on PATH, so two of them running at once race + * over the same stub budget. Serialization bounds that contention; it does not + * remove the real waits, which is why they are retry-eligible too. + */ + serializedStub?: true; +}; + +const SUBPROCESS_STUB_ISSUE = 'https://github.com/callstack/agent-device/issues/1098'; +const CONTENTION_ISSUE = 'https://github.com/callstack/agent-device/issues/1419'; +const FUZZ_ISSUE = 'https://github.com/callstack/agent-device/issues/1414'; +const REVIEW_BY = '2026-10-31'; + +/** + * The retry-eligible set: every `SUBPROCESS_STUB_TESTS` file (vitest.config.ts — + * serializing them bounds contention but does not remove the real waits) plus + * the repeat offenders observed failing timeout-shaped on unrelated diffs. + * `contention-retry-policy.test.ts` asserts the subprocess-stub half stays in + * sync with the vitest projects. + */ +export const CONTENTION_RETRY_FILES: readonly ContentionRetryEntry[] = [ + { + file: 'src/platforms/android/__tests__/app-lifecycle-install.test.ts', + reason: 'Stubs adb on PATH and spawns it per case, waiting real install retry/poll time.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/android/__tests__/app-lifecycle-open.test.ts', + reason: 'Stubs adb on PATH and spawns it, waiting real activity-launch poll time.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/android/__tests__/device-input-state.test.ts', + reason: 'Stubs adb on PATH and spawns it, waiting real input-state poll time.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/android/__tests__/input-actions.test.ts', + reason: 'Stubs adb on PATH and spawns it once per input action, waiting real retry time.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/android/__tests__/notifications.test.ts', + reason: 'Stubs adb on PATH and spawns it, waiting real shade-settle poll time.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/android/__tests__/settings.test.ts', + reason: 'Stubs adb on PATH and spawns it, waiting real settings-apply poll time.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/daemon/__tests__/runtime-hints.test.ts', + reason: 'Stubs platform binaries on PATH and spawns them to derive runtime hints.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/apple/core/__tests__/apps.test.ts', + reason: 'Stubs xcrun/simctl on PATH and spawns them, waiting real app-state poll budgets.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/apple/core/__tests__/interactions.test.ts', + reason: 'Stubs xcrun/simctl on PATH and spawns them, waiting real interaction settle budgets.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/apple/core/__tests__/simulator.test.ts', + reason: 'Stubs xcrun/simctl on PATH and spawns them, waiting real boot-poll budgets.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/apple/core/__tests__/physical-device-screenshot.test.ts', + reason: 'Stubs devicectl on PATH and spawns it, waiting real capture budgets.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/apple/core/__tests__/screenshot.test.ts', + reason: 'Stubs xcrun/simctl on PATH and spawns them, waiting real capture budgets.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/apple/core/__tests__/screenshot-status-bar.test.ts', + reason: 'Stubs xcrun/simctl on PATH and spawns them, waiting real status-bar override time.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/platforms/apple/core/__tests__/devicectl.test.ts', + reason: 'Stubs devicectl on PATH and spawns it, waiting real device-poll budgets.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/__tests__/client-metro.test.ts', + reason: 'Stubs npx plus the package managers and spawns a real Metro dev server per case.', + trackingIssue: SUBPROCESS_STUB_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'scripts/fuzz/harness.test.ts', + reason: 'Spawns a node subprocess or worker per case; one target hangs on purpose (#1414).', + trackingIssue: FUZZ_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'scripts/fuzz/corpus-replay.test.ts', + reason: 'Replays the fuzz corpus through the worker watchdog, waiting its per-case budget.', + trackingIssue: FUZZ_ISSUE, + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'scripts/repo-health/run.test.ts', + reason: 'Spawns the repo-health CLI over the real tree and waits for it to exit (#1423).', + trackingIssue: 'https://github.com/callstack/agent-device/issues/1423', + reviewBy: REVIEW_BY, + serializedStub: true, + }, + { + file: 'src/daemon/__tests__/request-router-open.test.ts', + reason: 'Drives real open routing over keyed locks, waiting on lock/session settle budgets.', + trackingIssue: CONTENTION_ISSUE, + reviewBy: REVIEW_BY, + }, + { + file: 'src/platforms/apple/core/__tests__/runner-client.test.ts', + reason: 'Drives runner transport connect/retry against a real socket, waiting retry backoff.', + trackingIssue: CONTENTION_ISSUE, + reviewBy: REVIEW_BY, + }, + { + file: 'src/platforms/apple/core/__tests__/runner-xctestrun.test.ts', + reason: 'Stubs xcodebuild on PATH and spawns it for xctestrun preparation and cache checks.', + trackingIssue: CONTENTION_ISSUE, + reviewBy: REVIEW_BY, + }, + { + file: 'scripts/__tests__/help-conformance-bench.test.ts', + reason: 'Spawns the real bench script per case in --dry-run mode and waits for it to exit.', + trackingIssue: CONTENTION_ISSUE, + reviewBy: REVIEW_BY, + }, +]; + +/** + * The `subprocess-stub` Vitest project's file set, derived from the one list so + * the retry policy and the execution contract cannot drift apart. + * `vitest.config.ts` consumes this for both the unit-core exclude and the + * subprocess-stub include. + */ +export const SUBPROCESS_STUB_TESTS: readonly string[] = CONTENTION_RETRY_FILES.filter( + (entry) => entry.serializedStub, +).map((entry) => entry.file); + +export function isRetryEligibleFile(file: string): boolean { + return CONTENTION_RETRY_FILES.some((entry) => entry.file === normalizeTestFile(file)); +} + +/** Absolute or `./`-prefixed paths arrive from reporters; the list is repo-relative. */ +export function normalizeTestFile(file: string, repoRoot?: string): string { + let normalized = file.replaceAll('\\', '/'); + if (repoRoot) { + const prefix = `${repoRoot.replaceAll('\\', '/').replace(/\/$/, '')}/`; + if (normalized.startsWith(prefix)) normalized = normalized.slice(prefix.length); + } + return normalized.replace(/^\.\//, ''); +} + +/** Entries whose review date has passed, in list order. */ +export function expiredRetryEntries(now: Date): readonly ContentionRetryEntry[] { + const today = now.toISOString().slice(0, 10); + return CONTENTION_RETRY_FILES.filter((entry) => entry.reviewBy < today); +} + +// Vitest reports a timeout as a plain Error with a fixed message shape; there is +// no structured signal on the JSON reporter's `failureMessages`, so this is an +// owned exception to the repo's "typed signals over message sniffing" rule and +// stays confined to this classifier. Erring towards "not a timeout" is the safe +// direction: it fails the job on the first run. +const TIMEOUT_SHAPED = [ + /\btest timed out in \d+\s*ms\b/i, + /\bhook timed out in \d+\s*ms\b/i, + /\bclosing timeout\b/i, + /\bETIMEDOUT\b/, +]; + +export function isTimeoutShapedFailure(message: string): boolean { + return TIMEOUT_SHAPED.some((pattern) => pattern.test(message)); +} + +/** One failed test case as read from a runner report. */ +export type TestFailure = { + file: string; + testName: string; + message: string; +}; + +export type RetryPlan = + | { retry: false; reason: string; blocked: readonly TestFailure[] } + | { retry: true; files: readonly string[]; failures: readonly TestFailure[] }; + +/** + * Decide whether a failed run may retry. Retry requires *every* failure to be a + * timeout in a listed file: one assertion failure anywhere fails the job, so a + * real regression can never be papered over by a rerun. + */ +export function planContentionRetry(failures: readonly TestFailure[]): RetryPlan { + if (failures.length === 0) { + return { retry: false, reason: 'no test failures to retry', blocked: [] }; + } + const blocked = failures.filter( + (failure) => !isRetryEligibleFile(failure.file) || !isTimeoutShapedFailure(failure.message), + ); + if (blocked.length > 0) { + const assertionShaped = blocked.filter((failure) => isRetryEligibleFile(failure.file)); + return { + retry: false, + reason: + assertionShaped.length > 0 + ? 'a listed file failed for a non-timeout reason (assertion failures never retry)' + : 'a failure landed outside the enumerated retry list', + blocked, + }; + } + const files = [...new Set(failures.map((failure) => normalizeTestFile(failure.file)))].sort(); + return { retry: true, files, failures }; +} + +export type RetryOutcome = 'passed' | 'failed'; + +export type RetrySummaryInput = { + plan: RetryPlan; + outcome?: RetryOutcome; +}; + +/** Markdown for the job summary: every retried file is named, always. */ +export function formatRetrySummary({ plan, outcome }: RetrySummaryInput): string { + const lines = ['## Contention retry (#1419)', '']; + if (!plan.retry) { + lines.push(`No retry: ${plan.reason}.`, ''); + for (const failure of plan.blocked) { + lines.push(`- \`${normalizeTestFile(failure.file)}\` — ${failure.testName}`); + } + return `${lines.join('\n').trimEnd()}\n`; + } + lines.push( + `Retried ${plan.files.length} timeout-shaped file(s) once — outcome: **${outcome ?? 'pending'}**.`, + '', + '| File | Failing test | Tracking issue | Review by |', + '| --- | --- | --- | --- |', + ); + for (const failure of plan.failures) { + const file = normalizeTestFile(failure.file); + const entry = CONTENTION_RETRY_FILES.find((candidate) => candidate.file === file); + lines.push( + `| \`${file}\` | ${failure.testName} | ${entry?.trackingIssue ?? '—'} | ${entry?.reviewBy ?? '—'} |`, + ); + } + return `${lines.join('\n')}\n`; +} diff --git a/vitest.config.ts b/vitest.config.ts index d8e17153d..703170ebf 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,4 +1,5 @@ import { defineConfig } from 'vitest/config'; +import { SUBPROCESS_STUB_TESTS } from './scripts/lib/contention-retry.ts'; import slowTestGateReporter from './scripts/vitest-slow-test-reporter.ts'; // Tests that stub a real binary (adb/xcrun/npx) by mutating process.env.PATH and @@ -10,19 +11,10 @@ import slowTestGateReporter from './scripts/vitest-slow-test-reporter.ts'; // docs/agents/testing.md "tests must not wait real time"). Serialized below with // per-file isolation so only one such file spawns stubs at a time, the same // execution contract the pre-split android index.test.ts aggregation provided. -export const SUBPROCESS_STUB_TESTS = [ - 'src/platforms/android/__tests__/{app-lifecycle-install,app-lifecycle-open,device-input-state,input-actions,notifications,settings}.test.ts', - 'src/daemon/__tests__/runtime-hints.test.ts', - 'src/platforms/apple/core/__tests__/{apps,interactions,simulator,physical-device-screenshot,screenshot,screenshot-status-bar,devicectl}.test.ts', - // Stubs npx + the package managers on PATH and spawns a real Metro dev server per case. - 'src/__tests__/client-metro.test.ts', - // Proves the parser fuzz harness still fails (#1414): every case spawns a node subprocess or a - // worker thread and one target is a deliberate hang, so it waits real watchdog time. - 'scripts/fuzz/harness.test.ts', - // Replays the fuzz regression corpus (#1414) through that same worker watchdog, so a promoted - // hang case fails against its per-case budget instead of wedging the unit job. - 'scripts/fuzz/corpus-replay.test.ts', -]; +// The enumerated file list, with each file's contention reason and its owned +// waiver, lives in scripts/lib/contention-retry.ts — the same constant the CI +// single-retry policy (#1419) reads, so the two cannot drift. +export { SUBPROCESS_STUB_TESTS }; export default defineConfig({ test: { @@ -56,7 +48,7 @@ export default defineConfig({ // The Maestro conformance oracle runs via `node --test` in its own CI // job (scripts/maestro-conformance), like the layering guard. ], - exclude: SUBPROCESS_STUB_TESTS, + exclude: [...SUBPROCESS_STUB_TESTS], setupFiles: [ 'src/__tests__/hermetic-env-setup.ts', 'src/__tests__/process-memo-setup.ts', @@ -71,7 +63,7 @@ export default defineConfig({ // provided without leaking module caches between split files. test: { name: 'subprocess-stub', - include: SUBPROCESS_STUB_TESTS, + include: [...SUBPROCESS_STUB_TESTS], setupFiles: [ 'src/__tests__/hermetic-env-setup.ts', 'src/__tests__/process-memo-setup.ts', From 3bc0cef8079fdac1207cd5da5d1dab82a9fc956b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 10:40:08 +0000 Subject: [PATCH 02/12] fix: satisfy fallow --- scripts/lib/contention-retry-policy.test.ts | 26 +++++++++++++++ scripts/lib/contention-retry-run.ts | 35 ++----------------- scripts/lib/contention-retry.ts | 37 ++++++++++++++++++++- 3 files changed, 64 insertions(+), 34 deletions(-) diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts index 9c5301f0b..7b5926722 100644 --- a/scripts/lib/contention-retry-policy.test.ts +++ b/scripts/lib/contention-retry-policy.test.ts @@ -13,6 +13,7 @@ import { expiredRetryEntries, formatRetrySummary, isTimeoutShapedFailure, + parseVitestFailures, planContentionRetry, SUBPROCESS_STUB_TESTS, type TestFailure, @@ -117,6 +118,31 @@ test('timeout shapes are recognized and assertion failures are not', () => { assert.ok(!isTimeoutShapedFailure('Error: expected timeout hint to be set')); }); +test('vitest JSON failures are read as absolute paths, names, and messages', () => { + const failures = parseVitestFailures( + { + testResults: [ + { + name: `${repoRoot}/${LISTED}`, + assertionResults: [ + { status: 'passed', fullName: 'closes a session', failureMessages: [] }, + { status: 'failed', fullName: 'opens a session', failureMessages: [TIMEOUT_MESSAGE] }, + ], + }, + { name: 'broken-suite' }, + ], + }, + repoRoot, + ); + assert.deepEqual(failures, [failure()]); +}); + +test('an unreadable report yields no retry-eligible failures', () => { + assert.deepEqual(parseVitestFailures({}, repoRoot), []); + assert.deepEqual(parseVitestFailures({ testResults: 'nope' }, repoRoot), []); + assert.equal(planContentionRetry([]).retry, false); +}); + test('a timeout in a listed file retries exactly that file, once', () => { const plan = planContentionRetry([failure()]); assert.deepEqual(plan, { retry: true, files: [LISTED], failures: [failure()] }); diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index 0340cc852..5f8e5fda0 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -13,7 +13,7 @@ import path from 'node:path'; import { runCmdStreaming, runCmdSync } from '../../src/utils/exec.ts'; import { parseScriptArgs } from './cli-args.ts'; import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; -import { normalizeTestFile, type TestFailure } from './contention-retry.ts'; +import { parseVitestFailures, type TestFailure } from './contention-retry.ts'; const USAGE = `Usage: node --experimental-strip-types scripts/lib/contention-retry-run.ts [options] @@ -60,40 +60,9 @@ async function runVitest(extra: string[], report: string): Promise { return { ok: result.exitCode === 0, failures: readFailures(report) }; } -/** - * Vitest's JSON reporter, narrowed at the trust boundary. A run that dies before - * writing a report yields no failures, which the policy reads as "nothing - * retry-eligible" — the job fails, which is the safe direction. - */ function readFailures(report: string): readonly TestFailure[] { if (!fs.existsSync(report)) return []; - const parsed: unknown = JSON.parse(fs.readFileSync(report, 'utf8')); - const suites = (parsed as { testResults?: unknown }).testResults; - if (!Array.isArray(suites)) return []; - const failures: TestFailure[] = []; - for (const suite of suites) { - const { name, assertionResults } = suite as { - name?: unknown; - assertionResults?: unknown; - }; - if (typeof name !== 'string' || !Array.isArray(assertionResults)) continue; - for (const assertion of assertionResults) { - const { status, fullName, title, failureMessages } = assertion as { - status?: unknown; - fullName?: unknown; - title?: unknown; - failureMessages?: unknown; - }; - if (status !== 'failed') continue; - const messages = Array.isArray(failureMessages) ? failureMessages : []; - failures.push({ - file: normalizeTestFile(name, repoRoot), - testName: typeof fullName === 'string' ? fullName : String(title ?? 'unknown test'), - message: messages.filter((message) => typeof message === 'string').join('\n'), - }); - } - } - return failures; + return parseVitestFailures(JSON.parse(fs.readFileSync(report, 'utf8')), repoRoot); } function vitestVersion(): string { diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts index ce6a4646a..31e61523a 100644 --- a/scripts/lib/contention-retry.ts +++ b/scripts/lib/contention-retry.ts @@ -213,7 +213,7 @@ export const SUBPROCESS_STUB_TESTS: readonly string[] = CONTENTION_RETRY_FILES.f (entry) => entry.serializedStub, ).map((entry) => entry.file); -export function isRetryEligibleFile(file: string): boolean { +function isRetryEligibleFile(file: string): boolean { return CONTENTION_RETRY_FILES.some((entry) => entry.file === normalizeTestFile(file)); } @@ -256,6 +256,41 @@ export type TestFailure = { message: string; }; +/** + * Failures from Vitest's JSON reporter, narrowed at the trust boundary. A run + * that dies before writing a report parses to zero failures, which the policy + * reads as "nothing retry-eligible": the job fails, the safe direction. + */ +export function parseVitestFailures(report: unknown, repoRoot?: string): readonly TestFailure[] { + const suites = (report as { testResults?: unknown }).testResults; + if (!Array.isArray(suites)) return []; + return suites.flatMap((suite) => suiteFailures(suite, repoRoot)); +} + +function suiteFailures(suite: unknown, repoRoot: string | undefined): TestFailure[] { + const { name, assertionResults } = suite as { name?: unknown; assertionResults?: unknown }; + if (typeof name !== 'string' || !Array.isArray(assertionResults)) return []; + const file = normalizeTestFile(name, repoRoot); + return assertionResults + .map((assertion) => assertionFailure(file, assertion)) + .filter((failure) => failure !== undefined); +} + +function assertionFailure(file: string, assertion: unknown): TestFailure | undefined { + const { status, fullName, failureMessages } = assertion as { + status?: unknown; + fullName?: unknown; + failureMessages?: unknown; + }; + if (status !== 'failed') return undefined; + const messages = Array.isArray(failureMessages) ? failureMessages : []; + return { + file, + testName: typeof fullName === 'string' ? fullName : 'unknown test', + message: messages.filter((message) => typeof message === 'string').join('\n'), + }; +} + export type RetryPlan = | { retry: false; reason: string; blocked: readonly TestFailure[] } | { retry: true; files: readonly string[]; failures: readonly TestFailure[] }; From 4306733a2f527aa001295e44ca769479fa1b7ac2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 10:43:30 +0000 Subject: [PATCH 03/12] test(ci): read failures through a lane reporter so timeouts stay distinguishable --- scripts/lib/contention-retry-policy.test.ts | 22 ++++------- scripts/lib/contention-retry-reporter.ts | 44 +++++++++++++++++++++ scripts/lib/contention-retry-run.ts | 31 +++++++++------ scripts/lib/contention-retry.ts | 43 ++++++++------------ 4 files changed, 88 insertions(+), 52 deletions(-) create mode 100644 scripts/lib/contention-retry-reporter.ts diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts index 7b5926722..f143b9b1b 100644 --- a/scripts/lib/contention-retry-policy.test.ts +++ b/scripts/lib/contention-retry-policy.test.ts @@ -13,7 +13,7 @@ import { expiredRetryEntries, formatRetrySummary, isTimeoutShapedFailure, - parseVitestFailures, + parseFailureReport, planContentionRetry, SUBPROCESS_STUB_TESTS, type TestFailure, @@ -118,18 +118,12 @@ test('timeout shapes are recognized and assertion failures are not', () => { assert.ok(!isTimeoutShapedFailure('Error: expected timeout hint to be set')); }); -test('vitest JSON failures are read as absolute paths, names, and messages', () => { - const failures = parseVitestFailures( +test('reporter failures are read as repo-relative paths, names, and messages', () => { + const failures = parseFailureReport( { - testResults: [ - { - name: `${repoRoot}/${LISTED}`, - assertionResults: [ - { status: 'passed', fullName: 'closes a session', failureMessages: [] }, - { status: 'failed', fullName: 'opens a session', failureMessages: [TIMEOUT_MESSAGE] }, - ], - }, - { name: 'broken-suite' }, + failures: [ + { file: `${repoRoot}/${LISTED}`, testName: 'opens a session', message: TIMEOUT_MESSAGE }, + { testName: 'no file' }, ], }, repoRoot, @@ -138,8 +132,8 @@ test('vitest JSON failures are read as absolute paths, names, and messages', () }); test('an unreadable report yields no retry-eligible failures', () => { - assert.deepEqual(parseVitestFailures({}, repoRoot), []); - assert.deepEqual(parseVitestFailures({ testResults: 'nope' }, repoRoot), []); + assert.deepEqual(parseFailureReport({}, repoRoot), []); + assert.deepEqual(parseFailureReport({ failures: 'nope' }, repoRoot), []); assert.equal(planContentionRetry([]).retry, false); }); diff --git a/scripts/lib/contention-retry-reporter.ts b/scripts/lib/contention-retry-reporter.ts new file mode 100644 index 000000000..1201cdbf6 --- /dev/null +++ b/scripts/lib/contention-retry-reporter.ts @@ -0,0 +1,44 @@ +// Failure sink for the contention single-retry policy (#1419). +// +// Vitest's built-in `json` reporter serializes a timeout as a bare +// `STACK_TRACE_ERROR`, which is exactly the distinction the policy is built on +// (a timeout may retry, an assertion failure may not), so the lane reads its own +// reporter instead: it keeps each failed test's real error message, and nothing +// else. Enabled per run by the lane entrypoint via `CONTENTION_RETRY_FAILURES`. + +import fs from 'node:fs'; +import path from 'node:path'; +import type { Reporter, TestCase, TestModule } from 'vitest/node'; +import type { TestFailure } from './contention-retry.ts'; + +export const FAILURE_FILE_ENV = 'CONTENTION_RETRY_FAILURES'; + +export type FailureReport = { failures: TestFailure[] }; + +export default function contentionRetryReporter(): Reporter { + const failures: TestFailure[] = []; + let root = ''; + return { + onInit(ctx: { config: { root: string } }): void { + root = ctx.config.root; + }, + onTestCaseResult(testCase: TestCase): void { + const result = testCase.result(); + if (result.state !== 'failed') return; + const moduleId = (testCase.module as TestModule).moduleId; + failures.push({ + file: root && moduleId.startsWith(root) ? moduleId.slice(root.length + 1) : moduleId, + testName: testCase.fullName, + message: (result.errors ?? []) + .map((error) => `${error.name ?? 'Error'}: ${error.message}`) + .join('\n'), + }); + }, + onTestRunEnd(): void { + const target = process.env[FAILURE_FILE_ENV]; + if (!target) return; + fs.mkdirSync(path.dirname(target), { recursive: true }); + fs.writeFileSync(target, `${JSON.stringify({ failures } satisfies FailureReport)}\n`); + }, + }; +} diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index 5f8e5fda0..5eb5b5719 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -13,19 +13,20 @@ import path from 'node:path'; import { runCmdStreaming, runCmdSync } from '../../src/utils/exec.ts'; import { parseScriptArgs } from './cli-args.ts'; import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; -import { parseVitestFailures, type TestFailure } from './contention-retry.ts'; +import { FAILURE_FILE_ENV } from './contention-retry-reporter.ts'; +import { parseFailureReport, type TestFailure } from './contention-retry.ts'; const USAGE = `Usage: node --experimental-strip-types scripts/lib/contention-retry-run.ts [options] --coverage Run the full suite under coverage (the CI Coverage job) --project Vitest projects for the first run (default: all) - --report Vitest JSON report path (default: .tmp/contention-retry/report.json) + --report Failure report path (default: .tmp/contention-retry/failures.json) --envelope Lane envelope path (default: .tmp/contention-retry/lane-envelope.json) --summary Markdown summary sink (default: $GITHUB_STEP_SUMMARY) `; const repoRoot = path.resolve(import.meta.dirname, '../..'); -const DEFAULT_REPORT = '.tmp/contention-retry/report.json'; +const DEFAULT_REPORT = '.tmp/contention-retry/failures.json'; const DEFAULT_ENVELOPE = '.tmp/contention-retry/lane-envelope.json'; const args = parseScriptArgs(process.argv.slice(2), USAGE, { @@ -40,18 +41,22 @@ const reportPath = path.resolve(repoRoot, args.report); const envelopePath = path.resolve(repoRoot, args.envelope); const summaryPath = args.summary ?? process.env.GITHUB_STEP_SUMMARY; -function reporterArgs(report: string): string[] { - return ['--reporter=default', '--reporter=json', `--outputFile.json=${report}`]; -} - async function runVitest(extra: string[], report: string): Promise { fs.mkdirSync(path.dirname(report), { recursive: true }); fs.rmSync(report, { force: true }); const result = await runCmdStreaming( 'pnpm', - ['exec', 'vitest', 'run', ...extra, ...reporterArgs(report)], + [ + 'exec', + 'vitest', + 'run', + ...extra, + '--reporter=default', + '--reporter=./scripts/lib/contention-retry-reporter.ts', + ], { cwd: repoRoot, + env: { ...process.env, [FAILURE_FILE_ENV]: report }, allowFailure: true, onStdoutChunk: (chunk) => process.stdout.write(chunk), onStderrChunk: (chunk) => process.stderr.write(chunk), @@ -62,7 +67,7 @@ async function runVitest(extra: string[], report: string): Promise { function readFailures(report: string): readonly TestFailure[] { if (!fs.existsSync(report)) return []; - return parseVitestFailures(JSON.parse(fs.readFileSync(report, 'utf8')), repoRoot); + return parseFailureReport(JSON.parse(fs.readFileSync(report, 'utf8')), repoRoot); } function vitestVersion(): string { @@ -76,7 +81,11 @@ function vitestVersion(): string { function configHash(): string { const hash = crypto.createHash('sha256'); - for (const file of ['vitest.config.ts', 'scripts/lib/contention-retry.ts']) { + for (const file of [ + 'vitest.config.ts', + 'scripts/lib/contention-retry.ts', + 'scripts/lib/contention-retry-reporter.ts', + ]) { hash.update(fs.readFileSync(path.join(repoRoot, file))); } return `sha256:${hash.digest('hex').slice(0, 16)}`; @@ -94,7 +103,7 @@ const result = await runWithContentionRetry({ // The rerun is file-scoped and coverage-free on purpose: coverage thresholds // are a whole-suite verdict, and a threshold miss is not timeout-shaped, so it // never reaches this branch. - runFiles: (files) => runVitest([...files], `${reportPath}.retry.json`), + runFiles: (files) => runVitest([...files], reportPath.replace(/\.json$/, '.retry.json')), commit: runCmdSync('git', ['rev-parse', 'HEAD'], { cwd: repoRoot }).stdout.trim(), configHash: configHash(), vitestVersion: vitestVersion(), diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts index 31e61523a..f94dfa438 100644 --- a/scripts/lib/contention-retry.ts +++ b/scripts/lib/contention-retry.ts @@ -257,37 +257,26 @@ export type TestFailure = { }; /** - * Failures from Vitest's JSON reporter, narrowed at the trust boundary. A run - * that dies before writing a report parses to zero failures, which the policy - * reads as "nothing retry-eligible": the job fails, the safe direction. + * Failures as written by the lane's reporter (`contention-retry-reporter.ts`), + * narrowed at the trust boundary. A run that dies before writing the file parses + * to zero failures, which the policy reads as "nothing retry-eligible": the job + * fails, the safe direction. */ -export function parseVitestFailures(report: unknown, repoRoot?: string): readonly TestFailure[] { - const suites = (report as { testResults?: unknown }).testResults; - if (!Array.isArray(suites)) return []; - return suites.flatMap((suite) => suiteFailures(suite, repoRoot)); +export function parseFailureReport(report: unknown, repoRoot?: string): readonly TestFailure[] { + const failures = (report as { failures?: unknown }).failures; + if (!Array.isArray(failures)) return []; + return failures + .map((entry) => readFailure(entry, repoRoot)) + .filter((entry) => entry !== undefined); } -function suiteFailures(suite: unknown, repoRoot: string | undefined): TestFailure[] { - const { name, assertionResults } = suite as { name?: unknown; assertionResults?: unknown }; - if (typeof name !== 'string' || !Array.isArray(assertionResults)) return []; - const file = normalizeTestFile(name, repoRoot); - return assertionResults - .map((assertion) => assertionFailure(file, assertion)) - .filter((failure) => failure !== undefined); -} - -function assertionFailure(file: string, assertion: unknown): TestFailure | undefined { - const { status, fullName, failureMessages } = assertion as { - status?: unknown; - fullName?: unknown; - failureMessages?: unknown; - }; - if (status !== 'failed') return undefined; - const messages = Array.isArray(failureMessages) ? failureMessages : []; +function readFailure(entry: unknown, repoRoot: string | undefined): TestFailure | undefined { + const { file, testName, message } = entry as Partial>; + if (typeof file !== 'string') return undefined; return { - file, - testName: typeof fullName === 'string' ? fullName : 'unknown test', - message: messages.filter((message) => typeof message === 'string').join('\n'), + file: normalizeTestFile(file, repoRoot), + testName: typeof testName === 'string' ? testName : 'unknown test', + message: typeof message === 'string' ? message : '', }; } From b53eee9a5ba8382422b6fd0175e4ca3de46ef95d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 10:44:58 +0000 Subject: [PATCH 04/12] test(ci): cover the lane reporter and drop its duplicated boilerplate --- scripts/lib/contention-retry-policy.test.ts | 31 +++++++++++++++++ scripts/lib/contention-retry-reporter.ts | 38 ++++++++++++--------- 2 files changed, 52 insertions(+), 17 deletions(-) diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts index f143b9b1b..670957897 100644 --- a/scripts/lib/contention-retry-policy.test.ts +++ b/scripts/lib/contention-retry-policy.test.ts @@ -8,6 +8,10 @@ import fs from 'node:fs'; import path from 'node:path'; import { test } from 'node:test'; import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; +import contentionRetryReporter, { + failedTestCase, + writeFailureReport, +} from './contention-retry-reporter.ts'; import { CONTENTION_RETRY_FILES, expiredRetryEntries, @@ -118,6 +122,33 @@ test('timeout shapes are recognized and assertion failures are not', () => { assert.ok(!isTimeoutShapedFailure('Error: expected timeout hint to be set')); }); +test('the lane reporter keeps the real error message a timeout is classified by', () => { + const testCase = { + fullName: 'opens a session', + module: { moduleId: `${repoRoot}/${LISTED}` }, + result: () => ({ + state: 'failed', + errors: [{ name: 'Error', message: 'Test timed out in 5000ms.' }], + }), + }; + const failed = failedTestCase(testCase as unknown as Parameters[0]); + assert.deepEqual(failed, { + file: `${repoRoot}/${LISTED}`, + testName: 'opens a session', + message: 'Error: Test timed out in 5000ms.', + }); + + const passing = { ...testCase, result: () => ({ state: 'passed', errors: [] }) }; + assert.equal(failedTestCase(passing as unknown as Parameters[0]), null); + + const target = path.join(repoRoot, '.tmp/contention-retry/reporter-gate.json'); + writeFailureReport([failure()], target); + assert.deepEqual(parseFailureReport(JSON.parse(fs.readFileSync(target, 'utf8')), repoRoot), [ + failure(), + ]); + assert.ok(contentionRetryReporter().onTestCaseResult); +}); + test('reporter failures are read as repo-relative paths, names, and messages', () => { const failures = parseFailureReport( { diff --git a/scripts/lib/contention-retry-reporter.ts b/scripts/lib/contention-retry-reporter.ts index 1201cdbf6..f88173165 100644 --- a/scripts/lib/contention-retry-reporter.ts +++ b/scripts/lib/contention-retry-reporter.ts @@ -5,6 +5,8 @@ // (a timeout may retry, an assertion failure may not), so the lane reads its own // reporter instead: it keeps each failed test's real error message, and nothing // else. Enabled per run by the lane entrypoint via `CONTENTION_RETRY_FAILURES`. +// +// Module ids are written as-is; `parseFailureReport` makes them repo-relative. import fs from 'node:fs'; import path from 'node:path'; @@ -15,30 +17,32 @@ export const FAILURE_FILE_ENV = 'CONTENTION_RETRY_FAILURES'; export type FailureReport = { failures: TestFailure[] }; +/** The failed-test view of a Vitest test case, or null when it did not fail. */ +export function failedTestCase(testCase: TestCase): TestFailure | null { + const result = testCase.result(); + if (result.state !== 'failed') return null; + return { + file: (testCase.module as TestModule).moduleId, + testName: testCase.fullName, + message: (result.errors ?? []).map((error) => `${error.name}: ${error.message}`).join('\n'), + }; +} + +export function writeFailureReport(failures: readonly TestFailure[], target: string): void { + fs.mkdirSync(path.dirname(target), { recursive: true }); + fs.writeFileSync(target, `${JSON.stringify({ failures } satisfies FailureReport)}\n`); +} + export default function contentionRetryReporter(): Reporter { const failures: TestFailure[] = []; - let root = ''; return { - onInit(ctx: { config: { root: string } }): void { - root = ctx.config.root; - }, onTestCaseResult(testCase: TestCase): void { - const result = testCase.result(); - if (result.state !== 'failed') return; - const moduleId = (testCase.module as TestModule).moduleId; - failures.push({ - file: root && moduleId.startsWith(root) ? moduleId.slice(root.length + 1) : moduleId, - testName: testCase.fullName, - message: (result.errors ?? []) - .map((error) => `${error.name ?? 'Error'}: ${error.message}`) - .join('\n'), - }); + const failed = failedTestCase(testCase); + if (failed) failures.push(failed); }, onTestRunEnd(): void { const target = process.env[FAILURE_FILE_ENV]; - if (!target) return; - fs.mkdirSync(path.dirname(target), { recursive: true }); - fs.writeFileSync(target, `${JSON.stringify({ failures } satisfies FailureReport)}\n`); + if (target) writeFailureReport(failures, target); }, }; } From 8e46fb18782c7fb3a3f0d315624463d8d6d7fc28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 10:59:11 +0000 Subject: [PATCH 05/12] chore(fallow): own the retry lane's tool-loaded export seams --- .fallowrc.json | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.fallowrc.json b/.fallowrc.json index 66428e28f..8d75518a1 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -18,6 +18,7 @@ "src/utils/png-worker.ts", "scripts/patch-xcuitest-runner-icon.ts", "scripts/runner-request-count/run.ts", + "scripts/lib/contention-retry-run.ts", "src/utils/update-check-entry.ts", "examples/sdk/client-session.ts", "examples/sdk/metro-runtime.ts", @@ -77,6 +78,14 @@ "GatedKeysAreResolverKeys" ] }, + { + "comment": "Contention retry lane (#1419): the reporter default is loaded by Vitest from a `--reporter=` string, and SUBPROCESS_STUB_TESTS is consumed by vitest.config.ts \u2014 a tool config outside --production analysis.", + "file": "scripts/lib/{contention-retry.ts,contention-retry-reporter.ts}", + "exports": [ + "default", + "SUBPROCESS_STUB_TESTS" + ] + }, { "comment": "Mutation-lane seam: readTestScope is consumed by vitest.mutation.config.ts, a tool config outside --production analysis.", "file": "scripts/mutation/test-scope.ts", From 452f268a4cc4215e3f1fd7f4b9937fcec0ea5244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 11:25:43 +0000 Subject: [PATCH 06/12] test(ci): block retries on non-test failures and classify timeouts structurally --- docs/agents/testing.md | 16 +- scripts/lib/contention-retry-blockers.ts | 41 ++++ scripts/lib/contention-retry-lane.ts | 28 +-- scripts/lib/contention-retry-policy.test.ts | 211 +++++++++++++++++--- scripts/lib/contention-retry-reporter.ts | 60 ++++-- scripts/lib/contention-retry-run.ts | 60 +++--- scripts/lib/contention-retry.ts | 122 ++++++++--- vitest.config.ts | 20 +- 8 files changed, 446 insertions(+), 112 deletions(-) create mode 100644 scripts/lib/contention-retry-blockers.ts diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 6df7a00d7..a3f6cdf5b 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -392,10 +392,18 @@ the retry policy cannot drift apart. The CI Coverage job runs the suite through `pnpm test:coverage:ci` (`scripts/lib/contention-retry-run.ts`), which applies one rule: -- **Timeouts only.** A rerun happens only when *every* failure in the run is timeout-shaped **and** - lands in a listed file. One assertion failure — in a listed file or not — fails the job on the - first run, so a real regression can never be papered over by a retry. -- **One retry, of the failed files only.** Not the suite, and never twice. +- **Timeouts only, classified structurally.** A rerun happens only when *every* failure in the run is + a timeout raised by the runner itself **and** lands in a listed file. The lane's own reporter + (`scripts/lib/contention-retry-reporter.ts`) classifies the live error object — Vitest's exact + timeout template, a plain `Error`, no `expected`/`actual` — so an assertion whose *message* + mentions a timeout cannot opt itself into a retry. One assertion failure — in a listed file or not + — fails the job on the first run, so a real regression can never be papered over by a retry. +- **Anything a rerun cannot re-check blocks the retry.** Unhandled errors, module load/setup errors, + a coverage-threshold miss, or a nonzero exit no failed test explains are recorded as blockers + (`scripts/lib/contention-retry-blockers.ts`) and fail the job, so a green retry can never erase a + second, unrelated failure from the same run. +- **One retry, of the failed files only.** Not the suite, and never twice. Two timed-out tests in one + file are one retry, and count as one. - **Retries stay visible.** Every retried file is named in the job summary with its tracking issue and review date, and the run writes the shared scheduled-lane envelope (`scripts/lib/lane-envelope.ts`, #1430) with the retry count, so a permanently flaky file shows up diff --git a/scripts/lib/contention-retry-blockers.ts b/scripts/lib/contention-retry-blockers.ts new file mode 100644 index 000000000..d4ff1d6dd --- /dev/null +++ b/scripts/lib/contention-retry-blockers.ts @@ -0,0 +1,41 @@ +// Process-level blockers for the contention single-retry policy (#1419). +// +// The reporter can only see what happened inside the run. Two failure classes +// live outside it and would otherwise be erased by a green retry: a coverage +// verdict (computed after the tests, and a whole-suite property a file-scoped +// rerun cannot re-establish), and any nonzero exit no failed test explains +// (worker crash, config error, report never written). Both are recorded as +// blockers, which forbid the retry outright. + +import type { RunBlocker } from './contention-retry.ts'; + +/** Vitest's own coverage verdict lines (`checkCoverages`, vitest 4). */ +const COVERAGE_FAILURE = [ + /ERROR: Coverage for [a-z]+ \([\d.]+%\) does not meet (?:global )?threshold/i, + /ERROR: Coverage for [a-z]+ \([\d.]+%\) does not meet .*minimum threshold/i, +]; + +export type ProcessOutcome = { + ok: boolean; + /** Combined stdout+stderr of the run. */ + output: string; + failureCount: number; +}; + +export function processBlockers(outcome: ProcessOutcome): RunBlocker[] { + if (outcome.ok) return []; + const blockers: RunBlocker[] = []; + const coverage = outcome.output + .split('\n') + .filter((line) => COVERAGE_FAILURE.some((pattern) => pattern.test(line))); + for (const line of coverage) { + blockers.push({ kind: 'coverage threshold', detail: line.trim() }); + } + if (outcome.failureCount === 0 && blockers.length === 0) { + blockers.push({ + kind: 'unexplained failure', + detail: 'the run exited nonzero with no failed test recorded — see the job log', + }); + } + return blockers; +} diff --git a/scripts/lib/contention-retry-lane.ts b/scripts/lib/contention-retry-lane.ts index 5211f6826..30a5718a9 100644 --- a/scripts/lib/contention-retry-lane.ts +++ b/scripts/lib/contention-retry-lane.ts @@ -13,18 +13,21 @@ import { planContentionRetry, type RetryOutcome, type RetryPlan, + type RunBlocker, type TestFailure, } from './contention-retry.ts'; export type TestRun = { ok: boolean; failures: readonly TestFailure[]; + /** Non-test failures; any of them forbids a retry. */ + blockers?: readonly RunBlocker[]; }; /** Telemetry payload for scheduled-lane health (#1430). */ export type ContentionRetryTelemetry = { - /** Files rerun in this job, with the test that timed out. */ - retried: ReadonlyArray<{ file: string; testName: string; trackingIssue: string }>; + /** Files rerun in this job (one entry per file, not per failed test). */ + retried: ReadonlyArray<{ file: string; testNames: readonly string[]; trackingIssue: string }>; retryCount: number; retryOutcome: RetryOutcome | null; listSize: number; @@ -81,7 +84,7 @@ export async function runWithContentionRetry( return finish(options, { ok: true, summary: '', plan: undefined, outcome: null }); } - const plan = planContentionRetry(first.failures); + const plan = planContentionRetry(first.failures, first.blockers ?? []); if (!plan.retry) { return finish(options, { ok: false, @@ -111,16 +114,17 @@ function finish( }, ): ContentionRetryResult { const plan = state.plan; + // Keyed by file, matching what actually reran: two timed-out tests in one file + // are one retry, not two. const retried = plan?.retry - ? plan.failures.map((failure) => { - const file = normalizeTestFile(failure.file); - return { - file, - testName: failure.testName, - trackingIssue: - CONTENTION_RETRY_FILES.find((entry) => entry.file === file)?.trackingIssue ?? '', - }; - }) + ? plan.files.map((file) => ({ + file, + testNames: plan.failures + .filter((failure) => normalizeTestFile(failure.file) === file) + .map((failure) => failure.testName), + trackingIssue: + CONTENTION_RETRY_FILES.find((entry) => entry.file === file)?.trackingIssue ?? '', + })) : []; return { ok: state.ok, diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts index 670957897..9589a80ea 100644 --- a/scripts/lib/contention-retry-policy.test.ts +++ b/scripts/lib/contention-retry-policy.test.ts @@ -8,15 +8,17 @@ import fs from 'node:fs'; import path from 'node:path'; import { test } from 'node:test'; import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; +import { processBlockers } from './contention-retry-blockers.ts'; import contentionRetryReporter, { failedTestCase, + runBlockers, writeFailureReport, } from './contention-retry-reporter.ts'; import { CONTENTION_RETRY_FILES, expiredRetryEntries, formatRetrySummary, - isTimeoutShapedFailure, + isVitestTimeoutError, parseFailureReport, planContentionRetry, SUBPROCESS_STUB_TESTS, @@ -26,11 +28,35 @@ import { const repoRoot = path.resolve(import.meta.dirname, '../..'); const LISTED = 'src/daemon/__tests__/request-router-open.test.ts'; -const TIMEOUT_MESSAGE = 'Error: Test timed out in 5000ms.\n at open()'; +// Vitest's own timeout error, verbatim (@vitest/runner `makeTimeoutError`). +const VITEST_TIMEOUT = { + name: 'Error', + message: + 'Test timed out in 5000ms.\nIf this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".', +}; +const TIMEOUT_MESSAGE = `Error: ${VITEST_TIMEOUT.message}`; const ASSERTION_MESSAGE = 'AssertionError: expected "DEVICE_IN_USE" to be "OK"'; function failure(overrides: Partial = {}): TestFailure { - return { file: LISTED, testName: 'opens a session', message: TIMEOUT_MESSAGE, ...overrides }; + return { + file: LISTED, + testName: 'opens a session', + message: TIMEOUT_MESSAGE, + timeout: true, + ...overrides, + }; +} + +function assertionFailure(overrides: Partial = {}): TestFailure { + return failure({ message: ASSERTION_MESSAGE, timeout: false, ...overrides }); +} + +function testCaseStub(errors: ReadonlyArray>, state = 'failed'): unknown { + return { + fullName: 'opens a session', + module: { moduleId: `${repoRoot}/${LISTED}` }, + result: () => ({ state, errors }), + }; } function lane(runs: { first: TestRun; retry?: TestRun; today?: Date }): { @@ -115,56 +141,179 @@ test('the expiry gate fails the run before any test executes', async () => { assert.deepEqual(rerun, []); }); -test('timeout shapes are recognized and assertion failures are not', () => { - assert.ok(isTimeoutShapedFailure(TIMEOUT_MESSAGE)); - assert.ok(isTimeoutShapedFailure('Error: Hook timed out in 10000 ms.')); - assert.ok(!isTimeoutShapedFailure(ASSERTION_MESSAGE)); - assert.ok(!isTimeoutShapedFailure('Error: expected timeout hint to be set')); +test("only the runner's own timeout error classifies as a timeout", () => { + assert.ok(isVitestTimeoutError(VITEST_TIMEOUT)); + assert.ok( + isVitestTimeoutError({ + name: 'Error', + message: + 'Hook timed out in 10000ms.\nIf this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".', + }), + ); + // An assertion must not be able to talk its way into a retry, whatever it says. + for (const impostor of [ + { name: 'AssertionError', message: VITEST_TIMEOUT.message }, + { name: 'AssertionError', message: 'expected ETIMEDOUT, got ECONNRESET' }, + { name: 'Error', message: `saw: ${VITEST_TIMEOUT.message}` }, + { name: 'Error', message: 'connect ETIMEDOUT 127.0.0.1:8080' }, + { name: 'Error', message: 'Closing timeout while tearing down the daemon' }, + { name: 'Error', message: VITEST_TIMEOUT.message, expected: 'OK', actual: 'DEVICE_IN_USE' }, + ]) { + assert.ok(!isVitestTimeoutError(impostor), `${impostor.name}: ${impostor.message}`); + } +}); + +test('an assertion message quoting a timeout still fails on the first run', async () => { + const impostor = failedTestCase( + testCaseStub([{ name: 'AssertionError', message: VITEST_TIMEOUT.message }]) as never, + ); + assert.ok(impostor); + assert.equal(impostor.timeout, false); + const { result, rerun } = lane({ + first: { ok: false, failures: [{ ...impostor, file: LISTED }] }, + }); + assert.equal((await result).ok, false); + assert.deepEqual(rerun, []); +}); + +test('a test that timed out AND failed an assertion is not retry-eligible', () => { + const mixed = failedTestCase( + testCaseStub([ + VITEST_TIMEOUT, + { name: 'AssertionError', message: 'expected 1 to be 2', expected: 2, actual: 1 }, + ]) as never, + ); + assert.equal(mixed?.timeout, false); }); test('the lane reporter keeps the real error message a timeout is classified by', () => { - const testCase = { - fullName: 'opens a session', - module: { moduleId: `${repoRoot}/${LISTED}` }, - result: () => ({ - state: 'failed', - errors: [{ name: 'Error', message: 'Test timed out in 5000ms.' }], - }), - }; - const failed = failedTestCase(testCase as unknown as Parameters[0]); + const failed = failedTestCase(testCaseStub([VITEST_TIMEOUT]) as never); assert.deepEqual(failed, { file: `${repoRoot}/${LISTED}`, testName: 'opens a session', - message: 'Error: Test timed out in 5000ms.', + message: TIMEOUT_MESSAGE, + timeout: true, }); - - const passing = { ...testCase, result: () => ({ state: 'passed', errors: [] }) }; - assert.equal(failedTestCase(passing as unknown as Parameters[0]), null); + assert.equal(failedTestCase(testCaseStub([], 'passed') as never), null); const target = path.join(repoRoot, '.tmp/contention-retry/reporter-gate.json'); - writeFailureReport([failure()], target); - assert.deepEqual(parseFailureReport(JSON.parse(fs.readFileSync(target, 'utf8')), repoRoot), [ - failure(), - ]); + writeFailureReport({ failures: [failure()], blockers: [] }, target); + assert.deepEqual(parseFailureReport(JSON.parse(fs.readFileSync(target, 'utf8')), repoRoot), { + failures: [failure()], + blockers: [], + }); assert.ok(contentionRetryReporter().onTestCaseResult); }); +test('the Coverage lane keeps the configured reporters, failure sink included', async () => { + const { reporters } = await import('../../vitest.config.ts'); + const plain = reporters({}); + const laneReporters = reporters({ CONTENTION_RETRY_FAILURES: '/tmp/failures.json' }); + assert.equal(plain.length, 2, 'default + slow-test gate'); + assert.equal( + laneReporters.length, + plain.length + 1, + 'the failure sink is added, never substituted', + ); + // The slow-test gate must survive into the retry lane. + assert.ok(laneReporters.every((reporter) => Boolean(reporter))); + assert.ok(typeof laneReporters.at(-1) === 'object'); + const runner = fs.readFileSync( + path.join(repoRoot, 'scripts/lib/contention-retry-run.ts'), + 'utf8', + ); + assert.ok( + !/['"]--reporter/.test(runner), + 'a --reporter flag replaces the configured reporters and would drop the slow-test gate', + ); +}); + +test('non-test failures block the retry instead of being rerun away', async () => { + const covered = processBlockers({ + ok: false, + failureCount: 1, + output: [ + ' Test Files 1 failed (11)', + 'ERROR: Coverage for lines (79.5%) does not meet global threshold (80%)', + ].join('\n'), + }); + assert.deepEqual( + covered.map((blocker) => blocker.kind), + ['coverage threshold'], + ); + assert.deepEqual( + processBlockers({ ok: false, failureCount: 0, output: 'Error: worker exited' }).map( + (blocker) => blocker.kind, + ), + ['unexplained failure'], + ); + assert.deepEqual(processBlockers({ ok: true, failureCount: 0, output: '' }), []); + + const moduleErrors = runBlockers( + [ + { moduleId: `${repoRoot}/${LISTED}`, errors: () => [{ message: 'Cannot find module x' }] }, + ] as never, + [{ name: 'Error', message: 'Unhandled rejection\n at foo' }], + ); + assert.deepEqual( + moduleErrors.map((blocker) => blocker.kind), + ['unhandled error', 'module error'], + ); + + // A retry-eligible timeout alongside any of them still fails the job. + for (const blockers of [covered, moduleErrors]) { + const { result, rerun } = lane({ first: { ok: false, failures: [failure()], blockers } }); + const resolved = await result; + assert.equal(resolved.ok, false); + assert.deepEqual(rerun, [], 'a blocked run must never be rerun'); + assert.equal(resolved.envelope.data.retryCount, 0); + assert.match(resolved.summary, /No retry: the run failed for a reason a rerun cannot re-check/); + } +}); + +test('two timed-out tests in one file are one retry, counted once', async () => { + const failures = [failure(), failure({ testName: 'closes a session' })]; + const plan = planContentionRetry(failures); + assert.deepEqual(plan.retry && plan.files, [LISTED]); + const { result, rerun } = lane({ first: { ok: false, failures } }); + const resolved = await result; + assert.deepEqual(rerun, [[LISTED]]); + assert.equal(resolved.envelope.data.retryCount, 1); + assert.deepEqual(resolved.envelope.data.retried, [ + { + file: LISTED, + testNames: ['opens a session', 'closes a session'], + trackingIssue: CONTENTION_RETRY_FILES.find((entry) => entry.file === LISTED)?.trackingIssue, + }, + ]); + assert.match(resolved.summary, /Retried 1 timeout-shaped file\(s\)/); + assert.equal(resolved.summary.split('\n').filter((line) => line.includes(LISTED)).length, 1); +}); + test('reporter failures are read as repo-relative paths, names, and messages', () => { const failures = parseFailureReport( { failures: [ - { file: `${repoRoot}/${LISTED}`, testName: 'opens a session', message: TIMEOUT_MESSAGE }, + { + file: `${repoRoot}/${LISTED}`, + testName: 'opens a session', + message: TIMEOUT_MESSAGE, + timeout: true, + }, { testName: 'no file' }, ], }, repoRoot, ); - assert.deepEqual(failures, [failure()]); + assert.deepEqual(failures.failures, [failure()]); }); test('an unreadable report yields no retry-eligible failures', () => { - assert.deepEqual(parseFailureReport({}, repoRoot), []); - assert.deepEqual(parseFailureReport({ failures: 'nope' }, repoRoot), []); + assert.deepEqual(parseFailureReport({}, repoRoot), { failures: [], blockers: [] }); + assert.deepEqual(parseFailureReport({ failures: 'nope' }, repoRoot), { + failures: [], + blockers: [], + }); assert.equal(planContentionRetry([]).retry, false); }); @@ -174,7 +323,7 @@ test('a timeout in a listed file retries exactly that file, once', () => { }); test('an assertion failure in a listed file never retries', () => { - const plan = planContentionRetry([failure({ message: ASSERTION_MESSAGE })]); + const plan = planContentionRetry([assertionFailure()]); assert.equal(plan.retry, false); assert.match(plan.reason, /assertion failures never retry/); }); @@ -190,7 +339,7 @@ test('a timeout outside the list never retries, even alongside eligible ones', ( test('an injected assertion failure in a listed file fails the job on the first run', async () => { const { result, rerun } = lane({ - first: { ok: false, failures: [failure({ message: ASSERTION_MESSAGE })] }, + first: { ok: false, failures: [assertionFailure()] }, }); const resolved = await result; assert.equal(resolved.ok, false); diff --git a/scripts/lib/contention-retry-reporter.ts b/scripts/lib/contention-retry-reporter.ts index f88173165..704cfaf39 100644 --- a/scripts/lib/contention-retry-reporter.ts +++ b/scripts/lib/contention-retry-reporter.ts @@ -1,36 +1,68 @@ // Failure sink for the contention single-retry policy (#1419). // -// Vitest's built-in `json` reporter serializes a timeout as a bare -// `STACK_TRACE_ERROR`, which is exactly the distinction the policy is built on -// (a timeout may retry, an assertion failure may not), so the lane reads its own -// reporter instead: it keeps each failed test's real error message, and nothing -// else. Enabled per run by the lane entrypoint via `CONTENTION_RETRY_FAILURES`. +// The lane needs two things no built-in reporter gives it together: each failed +// test's *live* error object (so a timeout is classified structurally rather +// than from rendered text — Vitest's `json` reporter serializes a timeout as a +// bare `STACK_TRACE_ERROR`), and every failure that is not a failed test case. +// The latter are recorded as blockers, which forbid a retry outright: without +// them a timeout retry could turn a run green that also failed for an unrelated +// reason. // +// Composed into the configured reporters by vitest.config.ts whenever +// `CONTENTION_RETRY_FAILURES` is set, so the slow-test gate stays in the lane. // Module ids are written as-is; `parseFailureReport` makes them repo-relative. import fs from 'node:fs'; import path from 'node:path'; import type { Reporter, TestCase, TestModule } from 'vitest/node'; -import type { TestFailure } from './contention-retry.ts'; +import { isVitestTimeoutError, type RunBlocker, type TestFailure } from './contention-retry.ts'; export const FAILURE_FILE_ENV = 'CONTENTION_RETRY_FAILURES'; -export type FailureReport = { failures: TestFailure[] }; +export type FailureReport = { failures: TestFailure[]; blockers: RunBlocker[] }; /** The failed-test view of a Vitest test case, or null when it did not fail. */ export function failedTestCase(testCase: TestCase): TestFailure | null { const result = testCase.result(); if (result.state !== 'failed') return null; + const errors = result.errors ?? []; return { file: (testCase.module as TestModule).moduleId, testName: testCase.fullName, - message: (result.errors ?? []).map((error) => `${error.name}: ${error.message}`).join('\n'), + message: errors.map((error) => `${error.name}: ${error.message}`).join('\n'), + // Every error must be a runner timeout: a test that timed out *and* failed + // an assertion is not retry-eligible. + timeout: errors.length > 0 && errors.every((error) => isVitestTimeoutError(error)), }; } -export function writeFailureReport(failures: readonly TestFailure[], target: string): void { +/** + * Failures that no rerun of the failed files can re-check: unhandled errors, and + * modules that failed outside a test case (import/setup/teardown errors, which + * report no failed test at all). + */ +export function runBlockers( + testModules: readonly TestModule[], + unhandledErrors: readonly { name?: unknown; message?: unknown }[], +): RunBlocker[] { + const blockers: RunBlocker[] = unhandledErrors.map((error) => ({ + kind: 'unhandled error', + detail: `${String(error.name ?? 'Error')}: ${String(error.message ?? '')}`.split('\n')[0] ?? '', + })); + for (const testModule of testModules) { + for (const error of testModule.errors()) { + blockers.push({ + kind: 'module error', + detail: `${testModule.moduleId}: ${String(error.message ?? '').split('\n')[0] ?? ''}`, + }); + } + } + return blockers; +} + +export function writeFailureReport(report: FailureReport, target: string): void { fs.mkdirSync(path.dirname(target), { recursive: true }); - fs.writeFileSync(target, `${JSON.stringify({ failures } satisfies FailureReport)}\n`); + fs.writeFileSync(target, `${JSON.stringify(report)}\n`); } export default function contentionRetryReporter(): Reporter { @@ -40,9 +72,13 @@ export default function contentionRetryReporter(): Reporter { const failed = failedTestCase(testCase); if (failed) failures.push(failed); }, - onTestRunEnd(): void { + onTestRunEnd( + testModules: readonly TestModule[], + unhandledErrors: readonly { name?: unknown; message?: unknown }[], + ): void { const target = process.env[FAILURE_FILE_ENV]; - if (target) writeFailureReport(failures, target); + if (!target) return; + writeFailureReport({ failures, blockers: runBlockers(testModules, unhandledErrors) }, target); }, }; } diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index 5eb5b5719..0ce52343c 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -14,7 +14,8 @@ import { runCmdStreaming, runCmdSync } from '../../src/utils/exec.ts'; import { parseScriptArgs } from './cli-args.ts'; import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; import { FAILURE_FILE_ENV } from './contention-retry-reporter.ts'; -import { parseFailureReport, type TestFailure } from './contention-retry.ts'; +import { processBlockers } from './contention-retry-blockers.ts'; +import { parseFailureReport, type RunBlocker, type TestFailure } from './contention-retry.ts'; const USAGE = `Usage: node --experimental-strip-types scripts/lib/contention-retry-run.ts [options] @@ -44,29 +45,38 @@ const summaryPath = args.summary ?? process.env.GITHUB_STEP_SUMMARY; async function runVitest(extra: string[], report: string): Promise { fs.mkdirSync(path.dirname(report), { recursive: true }); fs.rmSync(report, { force: true }); - const result = await runCmdStreaming( - 'pnpm', - [ - 'exec', - 'vitest', - 'run', - ...extra, - '--reporter=default', - '--reporter=./scripts/lib/contention-retry-reporter.ts', + // No `--reporter` flag: that would replace the reporters configured in + // vitest.config.ts and drop the slow-test gate from this lane. The env var + // composes the failure sink in alongside them. + let output = ''; + const capture = (chunk: string, write: (text: string) => void): void => { + output += chunk; + write(chunk); + }; + const result = await runCmdStreaming('pnpm', ['exec', 'vitest', 'run', ...extra], { + cwd: repoRoot, + env: { ...process.env, [FAILURE_FILE_ENV]: report }, + allowFailure: true, + onStdoutChunk: (chunk) => capture(chunk, (text) => process.stdout.write(text)), + onStderrChunk: (chunk) => capture(chunk, (text) => process.stderr.write(text)), + }); + const reported = readReport(report); + const ok = result.exitCode === 0; + return { + ok, + failures: reported.failures, + blockers: [ + ...reported.blockers, + ...processBlockers({ ok, output, failureCount: reported.failures.length }), ], - { - cwd: repoRoot, - env: { ...process.env, [FAILURE_FILE_ENV]: report }, - allowFailure: true, - onStdoutChunk: (chunk) => process.stdout.write(chunk), - onStderrChunk: (chunk) => process.stderr.write(chunk), - }, - ); - return { ok: result.exitCode === 0, failures: readFailures(report) }; + }; } -function readFailures(report: string): readonly TestFailure[] { - if (!fs.existsSync(report)) return []; +function readReport(report: string): { + failures: readonly TestFailure[]; + blockers: readonly RunBlocker[]; +} { + if (!fs.existsSync(report)) return { failures: [], blockers: [] }; return parseFailureReport(JSON.parse(fs.readFileSync(report, 'utf8')), repoRoot); } @@ -85,6 +95,7 @@ function configHash(): string { 'vitest.config.ts', 'scripts/lib/contention-retry.ts', 'scripts/lib/contention-retry-reporter.ts', + 'scripts/lib/contention-retry-blockers.ts', ]) { hash.update(fs.readFileSync(path.join(repoRoot, file))); } @@ -100,9 +111,10 @@ const projects = (args.project ?? '') const startedAtMs = Date.now(); const result = await runWithContentionRetry({ runAll: () => runVitest([...projects, ...(args.coverage ? ['--coverage'] : [])], reportPath), - // The rerun is file-scoped and coverage-free on purpose: coverage thresholds - // are a whole-suite verdict, and a threshold miss is not timeout-shaped, so it - // never reaches this branch. + // The rerun is file-scoped and coverage-free on purpose: a coverage verdict is + // a whole-suite property that a file-scoped rerun cannot re-establish, so a + // coverage failure in the first run is a blocker (`processBlockers`) and never + // reaches this branch. runFiles: (files) => runVitest([...files], reportPath.replace(/\.json$/, '.retry.json')), commit: runCmdSync('git', ['rev-parse', 'HEAD'], { cwd: repoRoot }).stdout.trim(), configHash: configHash(), diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts index f94dfa438..62c330e86 100644 --- a/scripts/lib/contention-retry.ts +++ b/scripts/lib/contention-retry.ts @@ -233,20 +233,37 @@ export function expiredRetryEntries(now: Date): readonly ContentionRetryEntry[] return CONTENTION_RETRY_FILES.filter((entry) => entry.reviewBy < today); } -// Vitest reports a timeout as a plain Error with a fixed message shape; there is -// no structured signal on the JSON reporter's `failureMessages`, so this is an -// owned exception to the repo's "typed signals over message sniffing" rule and -// stays confined to this classifier. Erring towards "not a timeout" is the safe -// direction: it fails the job on the first run. -const TIMEOUT_SHAPED = [ - /\btest timed out in \d+\s*ms\b/i, - /\bhook timed out in \d+\s*ms\b/i, - /\bclosing timeout\b/i, - /\bETIMEDOUT\b/, -]; +/** + * Vitest's own timeout error, verbatim (`makeTimeoutError`, @vitest/runner + * 4.1.8): a plain `Error` whose whole message is the runner's template. + */ +const VITEST_TIMEOUT_MESSAGE = + /^(Test|Hook) timed out in \d+ms\.\nIf this is a long-running (test|hook), pass a timeout value as the last argument/; + +/** The error shape the lane reporter classifies, as Vitest hands it over. */ +export type ReportedError = { + name?: unknown; + message?: unknown; + expected?: unknown; + actual?: unknown; +}; -export function isTimeoutShapedFailure(message: string): boolean { - return TIMEOUT_SHAPED.some((pattern) => pattern.test(message)); +/** + * Whether an error is a timeout *raised by the runner itself*, the only failure + * this lane may retry. + * + * Vitest exposes no reason code for a timeout, so the classifier runs against + * the live error object inside the reporter rather than against rendered text, + * and requires all three of: the runner's exact message template, a plain + * `Error` (assertion libraries set `name` to `AssertionError`), and no + * `expected`/`actual` diff fields. A test whose *assertion message* merely + * mentions a timeout therefore cannot opt itself into a retry — see the gate + * test. Erring towards "not a timeout" fails the job on the first run. + */ +export function isVitestTimeoutError(error: ReportedError): boolean { + if (error.name !== 'Error') return false; + if (error.expected !== undefined || error.actual !== undefined) return false; + return typeof error.message === 'string' && VITEST_TIMEOUT_MESSAGE.test(error.message); } /** One failed test case as read from a runner report. */ @@ -254,34 +271,63 @@ export type TestFailure = { file: string; testName: string; message: string; + /** Classified by the reporter from the live error object, never from text. */ + timeout: boolean; }; +/** + * A failure that is not a failed test case — an unhandled error, a module that + * failed to load, a coverage-threshold miss, a nonzero exit nothing else + * explains. These can never be retried away: they block the plan outright, so a + * timeout retry can never erase a second, unrelated failure from the same run. + */ +export type RunBlocker = { kind: string; detail: string }; + /** * Failures as written by the lane's reporter (`contention-retry-reporter.ts`), * narrowed at the trust boundary. A run that dies before writing the file parses * to zero failures, which the policy reads as "nothing retry-eligible": the job * fails, the safe direction. */ -export function parseFailureReport(report: unknown, repoRoot?: string): readonly TestFailure[] { - const failures = (report as { failures?: unknown }).failures; - if (!Array.isArray(failures)) return []; - return failures - .map((entry) => readFailure(entry, repoRoot)) - .filter((entry) => entry !== undefined); +export function parseFailureReport( + report: unknown, + repoRoot?: string, +): { failures: readonly TestFailure[]; blockers: readonly RunBlocker[] } { + const { failures, blockers } = report as { failures?: unknown; blockers?: unknown }; + return { + failures: Array.isArray(failures) + ? failures.map((entry) => readFailure(entry, repoRoot)).filter((entry) => entry !== undefined) + : [], + blockers: Array.isArray(blockers) + ? blockers.map((entry) => readBlocker(entry)).filter((entry) => entry !== undefined) + : [], + }; +} + +function readBlocker(entry: unknown): RunBlocker | undefined { + const { kind, detail } = entry as { kind?: unknown; detail?: unknown }; + if (typeof kind !== 'string') return undefined; + return { kind, detail: typeof detail === 'string' ? detail : '' }; } function readFailure(entry: unknown, repoRoot: string | undefined): TestFailure | undefined { - const { file, testName, message } = entry as Partial>; + const { file, testName, message, timeout } = entry as Partial>; if (typeof file !== 'string') return undefined; return { file: normalizeTestFile(file, repoRoot), testName: typeof testName === 'string' ? testName : 'unknown test', message: typeof message === 'string' ? message : '', + timeout: timeout === true, }; } export type RetryPlan = - | { retry: false; reason: string; blocked: readonly TestFailure[] } + | { + retry: false; + reason: string; + blocked: readonly TestFailure[]; + blockers: readonly RunBlocker[]; + } | { retry: true; files: readonly string[]; failures: readonly TestFailure[] }; /** @@ -289,12 +335,25 @@ export type RetryPlan = * timeout in a listed file: one assertion failure anywhere fails the job, so a * real regression can never be papered over by a rerun. */ -export function planContentionRetry(failures: readonly TestFailure[]): RetryPlan { +export function planContentionRetry( + failures: readonly TestFailure[], + blockers: readonly RunBlocker[] = [], +): RetryPlan { + if (blockers.length > 0) { + return { + retry: false, + reason: `the run failed for a reason a rerun cannot re-check (${blockers + .map((blocker) => blocker.kind) + .join(', ')})`, + blocked: failures, + blockers, + }; + } if (failures.length === 0) { - return { retry: false, reason: 'no test failures to retry', blocked: [] }; + return { retry: false, reason: 'no test failures to retry', blocked: [], blockers }; } const blocked = failures.filter( - (failure) => !isRetryEligibleFile(failure.file) || !isTimeoutShapedFailure(failure.message), + (failure) => !isRetryEligibleFile(failure.file) || !failure.timeout, ); if (blocked.length > 0) { const assertionShaped = blocked.filter((failure) => isRetryEligibleFile(failure.file)); @@ -305,6 +364,7 @@ export function planContentionRetry(failures: readonly TestFailure[]): RetryPlan ? 'a listed file failed for a non-timeout reason (assertion failures never retry)' : 'a failure landed outside the enumerated retry list', blocked, + blockers, }; } const files = [...new Set(failures.map((failure) => normalizeTestFile(failure.file)))].sort(); @@ -323,6 +383,9 @@ export function formatRetrySummary({ plan, outcome }: RetrySummaryInput): string const lines = ['## Contention retry (#1419)', '']; if (!plan.retry) { lines.push(`No retry: ${plan.reason}.`, ''); + for (const blocker of plan.blockers) { + lines.push(`- **${blocker.kind}** — ${blocker.detail}`); + } for (const failure of plan.blocked) { lines.push(`- \`${normalizeTestFile(failure.file)}\` — ${failure.testName}`); } @@ -331,14 +394,17 @@ export function formatRetrySummary({ plan, outcome }: RetrySummaryInput): string lines.push( `Retried ${plan.files.length} timeout-shaped file(s) once — outcome: **${outcome ?? 'pending'}**.`, '', - '| File | Failing test | Tracking issue | Review by |', + '| File | Failing test(s) | Tracking issue | Review by |', '| --- | --- | --- | --- |', ); - for (const failure of plan.failures) { - const file = normalizeTestFile(failure.file); + for (const file of plan.files) { const entry = CONTENTION_RETRY_FILES.find((candidate) => candidate.file === file); + const tests = plan.failures + .filter((failure) => normalizeTestFile(failure.file) === file) + .map((failure) => failure.testName) + .join(', '); lines.push( - `| \`${file}\` | ${failure.testName} | ${entry?.trackingIssue ?? '—'} | ${entry?.reviewBy ?? '—'} |`, + `| \`${file}\` | ${tests} | ${entry?.trackingIssue ?? '—'} | ${entry?.reviewBy ?? '—'} |`, ); } return `${lines.join('\n')}\n`; diff --git a/vitest.config.ts b/vitest.config.ts index 703170ebf..ba54f9d44 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,4 +1,8 @@ +import type { Reporter } from 'vitest/node'; import { defineConfig } from 'vitest/config'; +import contentionRetryReporter, { + FAILURE_FILE_ENV, +} from './scripts/lib/contention-retry-reporter.ts'; import { SUBPROCESS_STUB_TESTS } from './scripts/lib/contention-retry.ts'; import slowTestGateReporter from './scripts/vitest-slow-test-reporter.ts'; @@ -16,6 +20,20 @@ import slowTestGateReporter from './scripts/vitest-slow-test-reporter.ts'; // single-retry policy (#1419) reads, so the two cannot drift. export { SUBPROCESS_STUB_TESTS }; +/** + * Reporters, composed here rather than on the retry lane's command line: a + * `--reporter` flag *replaces* this list, which would silently drop the + * slow-test gate from the Coverage job. The retry lane opts its failure sink in + * through the environment instead. + */ +export function reporters(env: NodeJS.ProcessEnv = process.env): Array { + return [ + 'default', + slowTestGateReporter(), + ...(env[FAILURE_FILE_ENV] ? [contentionRetryReporter()] : []), + ]; +} + export default defineConfig({ test: { // Wall-clock discipline: unit tests must not wait real time. Measured @@ -26,7 +44,7 @@ export default defineConfig({ // --no-isolate = 205s wall vs 48s (module state thrashes across files), // threads = no change. slowTestThreshold: 500, - reporters: ['default', slowTestGateReporter()], + reporters: reporters(), projects: [ { test: { From 39484b5beb39b387fc51fa0292dccd596ea43088 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 11:40:46 +0000 Subject: [PATCH 07/12] test(ci): decide retry eligibility from runner metadata and route gate verdicts through blockers --- docs/agents/testing.md | 19 ++-- scripts/lib/contention-retry-policy.test.ts | 96 ++++++++++++++++++--- scripts/lib/contention-retry-reporter.ts | 32 ++++--- scripts/lib/contention-retry-run.ts | 1 + scripts/lib/contention-retry.ts | 41 ++++++--- scripts/lib/run-blocker-bus.ts | 23 +++++ scripts/vitest-slow-test-reporter.ts | 8 ++ 7 files changed, 181 insertions(+), 39 deletions(-) create mode 100644 scripts/lib/run-blocker-bus.ts diff --git a/docs/agents/testing.md b/docs/agents/testing.md index a3f6cdf5b..b077a6aca 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -392,16 +392,23 @@ the retry policy cannot drift apart. The CI Coverage job runs the suite through `pnpm test:coverage:ci` (`scripts/lib/contention-retry-run.ts`), which applies one rule: -- **Timeouts only, classified structurally.** A rerun happens only when *every* failure in the run is - a timeout raised by the runner itself **and** lands in a listed file. The lane's own reporter - (`scripts/lib/contention-retry-reporter.ts`) classifies the live error object — Vitest's exact - timeout template, a plain `Error`, no `expected`/`actual` — so an assertion whose *message* - mentions a timeout cannot opt itself into a retry. One assertion failure — in a listed file or not - — fails the job on the first run, so a real regression can never be papered over by a retry. +- **Timeouts only, decided from runner metadata.** A rerun happens only when *every* failure in the + run is a test the runner itself aborted, in a listed file. Eligibility keys on structured runner + facts test code cannot author (`isRunnerTimeout`): the test must have consumed its whole configured + budget — `TestCase.options.timeout` versus `diagnostic().duration` — with Vitest's timeout template + and a diff-free plain `Error` required as corroboration. A hand-thrown `Error` carrying the exact + timeout message returns long before the budget and is refused; a test that really does run to its + budget is aborted by the runner, so the error is the runner's. One assertion failure — in a listed + file or not — fails the job on the first run, so a real regression can never be papered over. - **Anything a rerun cannot re-check blocks the retry.** Unhandled errors, module load/setup errors, a coverage-threshold miss, or a nonzero exit no failed test explains are recorded as blockers (`scripts/lib/contention-retry-blockers.ts`) and fail the job, so a green retry can never erase a second, unrelated failure from the same run. +- **Gates that fail a run without failing a test publish structurally.** A reporter-level verdict + (the slow-test ratchet setting `process.exitCode = 1`) is invisible in test results, so it is + recorded on the shared blocker channel `scripts/lib/run-blocker-bus.ts`; the retry lane's failure + sink drains it and refuses the rerun. **Any new gate reporter must call `recordRunBlocker`**, and + must be ordered before the sink in `reporters()`. - **One retry, of the failed files only.** Not the suite, and never twice. Two timed-out tests in one file are one retry, and count as one. - **Retries stay visible.** Every retried file is named in the job summary with its tracking issue diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts index 9589a80ea..a76196e87 100644 --- a/scripts/lib/contention-retry-policy.test.ts +++ b/scripts/lib/contention-retry-policy.test.ts @@ -18,7 +18,7 @@ import { CONTENTION_RETRY_FILES, expiredRetryEntries, formatRetrySummary, - isVitestTimeoutError, + isRunnerTimeout, parseFailureReport, planContentionRetry, SUBPROCESS_STUB_TESTS, @@ -51,14 +51,23 @@ function assertionFailure(overrides: Partial = {}): TestFailure { return failure({ message: ASSERTION_MESSAGE, timeout: false, ...overrides }); } -function testCaseStub(errors: ReadonlyArray>, state = 'failed'): unknown { +const TIMEOUT_MS = 5000; + +function testCaseStub( + errors: ReadonlyArray>, + overrides: { state?: string; timeoutMs?: number; durationMs?: number } = {}, +): unknown { return { fullName: 'opens a session', module: { moduleId: `${repoRoot}/${LISTED}` }, - result: () => ({ state, errors }), + options: { timeout: overrides.timeoutMs ?? TIMEOUT_MS }, + diagnostic: () => ({ duration: overrides.durationMs ?? TIMEOUT_MS + 4 }), + result: () => ({ state: overrides.state ?? 'failed', errors }), }; } +const RAN_TO_BUDGET = { timeoutMs: TIMEOUT_MS, durationMs: TIMEOUT_MS + 4 }; + function lane(runs: { first: TestRun; retry?: TestRun; today?: Date }): { result: ReturnType; rerun: string[][]; @@ -141,15 +150,26 @@ test('the expiry gate fails the run before any test executes', async () => { assert.deepEqual(rerun, []); }); -test("only the runner's own timeout error classifies as a timeout", () => { - assert.ok(isVitestTimeoutError(VITEST_TIMEOUT)); +test('only a test the runner aborted at its own budget classifies as a timeout', () => { + assert.ok(isRunnerTimeout([VITEST_TIMEOUT], RAN_TO_BUDGET)); assert.ok( - isVitestTimeoutError({ - name: 'Error', - message: - 'Hook timed out in 10000ms.\nIf this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".', - }), + isRunnerTimeout( + [ + { + name: 'Error', + message: + 'Hook timed out in 10000ms.\nIf this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".', + }, + ], + RAN_TO_BUDGET, + ), ); + // Runner metadata is the primary evidence: test code cannot make its own + // failure consume the whole configured budget and still choose the error. + assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], { timeoutMs: TIMEOUT_MS, durationMs: 12 })); + assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], { timeoutMs: undefined, durationMs: 99_999 })); + assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], { timeoutMs: 0, durationMs: 0 })); + assert.ok(!isRunnerTimeout([], RAN_TO_BUDGET)); // An assertion must not be able to talk its way into a retry, whatever it says. for (const impostor of [ { name: 'AssertionError', message: VITEST_TIMEOUT.message }, @@ -159,10 +179,25 @@ test("only the runner's own timeout error classifies as a timeout", () => { { name: 'Error', message: 'Closing timeout while tearing down the daemon' }, { name: 'Error', message: VITEST_TIMEOUT.message, expected: 'OK', actual: 'DEVICE_IN_USE' }, ]) { - assert.ok(!isVitestTimeoutError(impostor), `${impostor.name}: ${impostor.message}`); + assert.ok(!isRunnerTimeout([impostor], RAN_TO_BUDGET), `${impostor.name}: ${impostor.message}`); } }); +test('an exact-template Error thrown by the test itself never retries', async () => { + // The forgery the classifier must refuse: right name, right message, right + // shape — but the test returned long before its budget was up. + const forged = failedTestCase( + testCaseStub([{ name: 'Error', message: VITEST_TIMEOUT.message }], { durationMs: 7 }) as never, + ); + assert.ok(forged); + assert.equal(forged.timeout, false, 'a hand-thrown timeout message is not a runner timeout'); + const { result, rerun } = lane({ first: { ok: false, failures: [{ ...forged, file: LISTED }] } }); + const resolved = await result; + assert.equal(resolved.ok, false); + assert.deepEqual(rerun, []); + assert.equal(resolved.envelope.data.retryCount, 0); +}); + test('an assertion message quoting a timeout still fails on the first run', async () => { const impostor = failedTestCase( testCaseStub([{ name: 'AssertionError', message: VITEST_TIMEOUT.message }]) as never, @@ -194,7 +229,7 @@ test('the lane reporter keeps the real error message a timeout is classified by' message: TIMEOUT_MESSAGE, timeout: true, }); - assert.equal(failedTestCase(testCaseStub([], 'passed') as never), null); + assert.equal(failedTestCase(testCaseStub([], { state: 'passed' }) as never), null); const target = path.join(repoRoot, '.tmp/contention-retry/reporter-gate.json'); writeFailureReport({ failures: [failure()], blockers: [] }, target); @@ -271,6 +306,43 @@ test('non-test failures block the retry instead of being rerun away', async () = } }); +test('a gate that fails the run without failing a test blocks the retry', async () => { + const { default: slowTestGateReporter } = await import('../vitest-slow-test-reporter.ts'); + const gate = slowTestGateReporter(); + const exitCode = process.exitCode; + const stderr = console.error; + console.error = () => {}; + try { + gate.onInit?.({ config: { root: repoRoot } } as never); + // Passing, but far past the unit budget: no failed test, run must still fail. + gate.onTestCaseResult?.({ + name: 'INJECTED slow test', + fullName: 'INJECTED slow test', + module: { moduleId: `${repoRoot}/${LISTED}` }, + diagnostic: () => ({ duration: 30_000 }), + result: () => ({ state: 'passed', errors: [] }), + } as never); + gate.onTestRunEnd?.([] as never, [] as never, 'failed' as never); + } finally { + console.error = stderr; + process.exitCode = exitCode; + } + // The gate published its verdict on the shared channel; the sink drains it. + const blockers = runBlockers([], []); + assert.deepEqual( + blockers.map((blocker) => blocker.kind), + ['slow-test gate'], + ); + assert.deepEqual(runBlockers([], []), [], 'draining is one-shot'); + + // A retry-eligible timeout in the same run must not rerun the gate away. + const { result, rerun } = lane({ first: { ok: false, failures: [failure()], blockers } }); + const resolved = await result; + assert.equal(resolved.ok, false); + assert.deepEqual(rerun, []); + assert.match(resolved.summary, /slow-test gate/); +}); + test('two timed-out tests in one file are one retry, counted once', async () => { const failures = [failure(), failure({ testName: 'closes a session' })]; const plan = planContentionRetry(failures); diff --git a/scripts/lib/contention-retry-reporter.ts b/scripts/lib/contention-retry-reporter.ts index 704cfaf39..adeacedcc 100644 --- a/scripts/lib/contention-retry-reporter.ts +++ b/scripts/lib/contention-retry-reporter.ts @@ -15,7 +15,8 @@ import fs from 'node:fs'; import path from 'node:path'; import type { Reporter, TestCase, TestModule } from 'vitest/node'; -import { isVitestTimeoutError, type RunBlocker, type TestFailure } from './contention-retry.ts'; +import { isRunnerTimeout, type RunBlocker, type TestFailure } from './contention-retry.ts'; +import { drainRunBlockers } from './run-blocker-bus.ts'; export const FAILURE_FILE_ENV = 'CONTENTION_RETRY_FAILURES'; @@ -30,25 +31,34 @@ export function failedTestCase(testCase: TestCase): TestFailure | null { file: (testCase.module as TestModule).moduleId, testName: testCase.fullName, message: errors.map((error) => `${error.name}: ${error.message}`).join('\n'), - // Every error must be a runner timeout: a test that timed out *and* failed - // an assertion is not retry-eligible. - timeout: errors.length > 0 && errors.every((error) => isVitestTimeoutError(error)), + // Decided from runner metadata (budget consumed) plus the error shape, so + // test code cannot forge it; a test that timed out *and* failed an assertion + // is not retry-eligible either. + timeout: isRunnerTimeout(errors, { + timeoutMs: testCase.options.timeout, + durationMs: testCase.diagnostic()?.duration, + }), }; } /** - * Failures that no rerun of the failed files can re-check: unhandled errors, and - * modules that failed outside a test case (import/setup/teardown errors, which - * report no failed test at all). + * Failures that no rerun of the failed files can re-check: verdicts published by + * other gate reporters, unhandled errors, and modules that failed outside a test + * case (import/setup/teardown errors, which report no failed test at all). */ export function runBlockers( testModules: readonly TestModule[], unhandledErrors: readonly { name?: unknown; message?: unknown }[], ): RunBlocker[] { - const blockers: RunBlocker[] = unhandledErrors.map((error) => ({ - kind: 'unhandled error', - detail: `${String(error.name ?? 'Error')}: ${String(error.message ?? '')}`.split('\n')[0] ?? '', - })); + // Gate reporters that fail the run without failing a test publish here. + const blockers: RunBlocker[] = drainRunBlockers(); + for (const error of unhandledErrors) { + blockers.push({ + kind: 'unhandled error', + detail: + `${String(error.name ?? 'Error')}: ${String(error.message ?? '')}`.split('\n')[0] ?? '', + }); + } for (const testModule of testModules) { for (const error of testModule.errors()) { blockers.push({ diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index 0ce52343c..c96f6a408 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -96,6 +96,7 @@ function configHash(): string { 'scripts/lib/contention-retry.ts', 'scripts/lib/contention-retry-reporter.ts', 'scripts/lib/contention-retry-blockers.ts', + 'scripts/lib/run-blocker-bus.ts', ]) { hash.update(fs.readFileSync(path.join(repoRoot, file))); } diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts index 62c330e86..ce7425f9f 100644 --- a/scripts/lib/contention-retry.ts +++ b/scripts/lib/contention-retry.ts @@ -248,19 +248,40 @@ export type ReportedError = { actual?: unknown; }; +/** The runner-owned facts a timeout claim is checked against. */ +export type TimeoutEvidence = { + /** `TestCase.options.timeout` — the runner's configured budget for this test. */ + timeoutMs: number | undefined; + /** `TestCase.diagnostic().duration` — how long the runner let it run. */ + durationMs: number | undefined; +}; + /** - * Whether an error is a timeout *raised by the runner itself*, the only failure - * this lane may retry. + * Whether a failed test was aborted by the runner's own timeout, the only + * failure this lane may retry. * - * Vitest exposes no reason code for a timeout, so the classifier runs against - * the live error object inside the reporter rather than against rendered text, - * and requires all three of: the runner's exact message template, a plain - * `Error` (assertion libraries set `name` to `AssertionError`), and no - * `expected`/`actual` diff fields. A test whose *assertion message* merely - * mentions a timeout therefore cannot opt itself into a retry — see the gate - * test. Erring towards "not a timeout" fails the job on the first run. + * Vitest 4 ships no reason code on the timeout error, so eligibility is decided + * primarily by structured runner metadata that test code cannot author: the test + * must have consumed its entire configured budget (`options.timeout`, measured + * by `diagnostic().duration`). A forged `throw new Error()` + * fails here — it returns long before the budget — and a test that *does* run to + * its budget is aborted by the runner, so the error the runner reports is its + * own. The message template and error shape are then required as corroboration: + * a plain `Error` (assertion libraries set `name`) with no `expected`/`actual` + * diff fields. All of it must hold, and every error on the test must qualify; + * anything unclassifiable fails the job on the first run. */ -export function isVitestTimeoutError(error: ReportedError): boolean { +export function isRunnerTimeout( + errors: readonly ReportedError[], + evidence: TimeoutEvidence, +): boolean { + const { timeoutMs, durationMs } = evidence; + if (typeof timeoutMs !== 'number' || timeoutMs <= 0) return false; + if (typeof durationMs !== 'number' || durationMs < timeoutMs) return false; + return errors.length > 0 && errors.every((error) => isTimeoutErrorShape(error)); +} + +function isTimeoutErrorShape(error: ReportedError): boolean { if (error.name !== 'Error') return false; if (error.expected !== undefined || error.actual !== undefined) return false; return typeof error.message === 'string' && VITEST_TIMEOUT_MESSAGE.test(error.message); diff --git a/scripts/lib/run-blocker-bus.ts b/scripts/lib/run-blocker-bus.ts new file mode 100644 index 000000000..36c9e27ba --- /dev/null +++ b/scripts/lib/run-blocker-bus.ts @@ -0,0 +1,23 @@ +// Structured blocker channel between Vitest reporters (#1419). +// +// A gate reporter that fails a run without failing a test (the slow-test +// ratchet, for example) is invisible to anything that only reads test results: +// the contention retry lane would see one retry-eligible timeout, rerun it, and +// the green rerun would erase the gate's verdict. Gates publish their verdict +// here instead, and the retry lane's failure sink drains it — both reporters run +// in the same Vitest process, and the sink is ordered last in +// `vitest.config.ts`'s `reporters()` so every gate has already reported. + +import type { RunBlocker } from './contention-retry.ts'; + +const blockers: RunBlocker[] = []; + +/** Publish a verdict that fails the run without failing a test. */ +export function recordRunBlocker(blocker: RunBlocker): void { + blockers.push(blocker); +} + +/** Take everything published so far, clearing the channel. */ +export function drainRunBlockers(): RunBlocker[] { + return blockers.splice(0, blockers.length); +} diff --git a/scripts/vitest-slow-test-reporter.ts b/scripts/vitest-slow-test-reporter.ts index 53f657576..fc06c5125 100644 --- a/scripts/vitest-slow-test-reporter.ts +++ b/scripts/vitest-slow-test-reporter.ts @@ -1,4 +1,5 @@ import type { Reporter, TestCase, TestModule } from 'vitest/node'; +import { recordRunBlocker } from './lib/run-blocker-bus.ts'; /** * Slow-test ratchet (see docs/agents/testing.md "Speed rules"). @@ -99,6 +100,13 @@ export default function slowTestGateReporter(): Reporter { // eslint-disable-next-line no-console if (reportSlowTests(offenders, (message) => console.error(message))) { process.exitCode = 1; + // This gate fails the run without failing a test, so it must be + // published structurally: otherwise a retry lane sees only the + // retry-eligible failures and a green rerun erases this verdict. + recordRunBlocker({ + kind: 'slow-test gate', + detail: `${offenders.filter((offender) => offender.enforce).length} test(s) exceeded the wall-clock budget`, + }); } }, }; From 331ce740aacd37ed98b99faf3ac58047e91ca646 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 12:28:44 +0000 Subject: [PATCH 08/12] refactor(ci): name the retry policy's rules in code instead of comments --- .fallowrc.json | 5 +-- scripts/lib/contention-retry-blockers.ts | 10 +---- scripts/lib/contention-retry-lane.ts | 3 +- scripts/lib/contention-retry-reporter.ts | 27 +++---------- scripts/lib/contention-retry-run.ts | 7 ---- scripts/lib/contention-retry.ts | 50 ++++++++---------------- scripts/lib/run-blocker-bus.ts | 15 ++----- scripts/vitest-slow-test-reporter.ts | 3 -- vitest.config.ts | 15 ++----- 9 files changed, 33 insertions(+), 102 deletions(-) diff --git a/.fallowrc.json b/.fallowrc.json index 8d75518a1..df083557b 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -81,10 +81,7 @@ { "comment": "Contention retry lane (#1419): the reporter default is loaded by Vitest from a `--reporter=` string, and SUBPROCESS_STUB_TESTS is consumed by vitest.config.ts \u2014 a tool config outside --production analysis.", "file": "scripts/lib/{contention-retry.ts,contention-retry-reporter.ts}", - "exports": [ - "default", - "SUBPROCESS_STUB_TESTS" - ] + "exports": ["default", "SUBPROCESS_STUB_TESTS"] }, { "comment": "Mutation-lane seam: readTestScope is consumed by vitest.mutation.config.ts, a tool config outside --production analysis.", diff --git a/scripts/lib/contention-retry-blockers.ts b/scripts/lib/contention-retry-blockers.ts index d4ff1d6dd..36248a075 100644 --- a/scripts/lib/contention-retry-blockers.ts +++ b/scripts/lib/contention-retry-blockers.ts @@ -1,11 +1,5 @@ -// Process-level blockers for the contention single-retry policy (#1419). -// -// The reporter can only see what happened inside the run. Two failure classes -// live outside it and would otherwise be erased by a green retry: a coverage -// verdict (computed after the tests, and a whole-suite property a file-scoped -// rerun cannot re-establish), and any nonzero exit no failed test explains -// (worker crash, config error, report never written). Both are recorded as -// blockers, which forbid the retry outright. +// Blockers a reporter cannot see, read from the run's process outcome (#1419): +// the coverage verdict, and any nonzero exit no failed test explains. import type { RunBlocker } from './contention-retry.ts'; diff --git a/scripts/lib/contention-retry-lane.ts b/scripts/lib/contention-retry-lane.ts index 30a5718a9..57b97763a 100644 --- a/scripts/lib/contention-retry-lane.ts +++ b/scripts/lib/contention-retry-lane.ts @@ -114,8 +114,7 @@ function finish( }, ): ContentionRetryResult { const plan = state.plan; - // Keyed by file, matching what actually reran: two timed-out tests in one file - // are one retry, not two. + // Keyed by file, matching what actually reran. const retried = plan?.retry ? plan.files.map((file) => ({ file, diff --git a/scripts/lib/contention-retry-reporter.ts b/scripts/lib/contention-retry-reporter.ts index adeacedcc..25d1dd3f1 100644 --- a/scripts/lib/contention-retry-reporter.ts +++ b/scripts/lib/contention-retry-reporter.ts @@ -1,16 +1,7 @@ -// Failure sink for the contention single-retry policy (#1419). -// -// The lane needs two things no built-in reporter gives it together: each failed -// test's *live* error object (so a timeout is classified structurally rather -// than from rendered text — Vitest's `json` reporter serializes a timeout as a -// bare `STACK_TRACE_ERROR`), and every failure that is not a failed test case. -// The latter are recorded as blockers, which forbid a retry outright: without -// them a timeout retry could turn a run green that also failed for an unrelated -// reason. -// -// Composed into the configured reporters by vitest.config.ts whenever -// `CONTENTION_RETRY_FAILURES` is set, so the slow-test gate stays in the lane. -// Module ids are written as-is; `parseFailureReport` makes them repo-relative. +// Failure sink for the contention single-retry policy (#1419): writes each +// failed test with its retry eligibility, plus every failure that is not a +// failed test case. Enabled by `CONTENTION_RETRY_FAILURES`; module ids are +// written as-is and `parseFailureReport` makes them repo-relative. import fs from 'node:fs'; import path from 'node:path'; @@ -31,9 +22,6 @@ export function failedTestCase(testCase: TestCase): TestFailure | null { file: (testCase.module as TestModule).moduleId, testName: testCase.fullName, message: errors.map((error) => `${error.name}: ${error.message}`).join('\n'), - // Decided from runner metadata (budget consumed) plus the error shape, so - // test code cannot forge it; a test that timed out *and* failed an assertion - // is not retry-eligible either. timeout: isRunnerTimeout(errors, { timeoutMs: testCase.options.timeout, durationMs: testCase.diagnostic()?.duration, @@ -41,16 +29,11 @@ export function failedTestCase(testCase: TestCase): TestFailure | null { }; } -/** - * Failures that no rerun of the failed files can re-check: verdicts published by - * other gate reporters, unhandled errors, and modules that failed outside a test - * case (import/setup/teardown errors, which report no failed test at all). - */ +/** Failures that rerunning the failed files cannot re-check. */ export function runBlockers( testModules: readonly TestModule[], unhandledErrors: readonly { name?: unknown; message?: unknown }[], ): RunBlocker[] { - // Gate reporters that fail the run without failing a test publish here. const blockers: RunBlocker[] = drainRunBlockers(); for (const error of unhandledErrors) { blockers.push({ diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index c96f6a408..f3a68087b 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -45,9 +45,6 @@ const summaryPath = args.summary ?? process.env.GITHUB_STEP_SUMMARY; async function runVitest(extra: string[], report: string): Promise { fs.mkdirSync(path.dirname(report), { recursive: true }); fs.rmSync(report, { force: true }); - // No `--reporter` flag: that would replace the reporters configured in - // vitest.config.ts and drop the slow-test gate from this lane. The env var - // composes the failure sink in alongside them. let output = ''; const capture = (chunk: string, write: (text: string) => void): void => { output += chunk; @@ -112,10 +109,6 @@ const projects = (args.project ?? '') const startedAtMs = Date.now(); const result = await runWithContentionRetry({ runAll: () => runVitest([...projects, ...(args.coverage ? ['--coverage'] : [])], reportPath), - // The rerun is file-scoped and coverage-free on purpose: a coverage verdict is - // a whole-suite property that a file-scoped rerun cannot re-establish, so a - // coverage failure in the first run is a blocker (`processBlockers`) and never - // reaches this branch. runFiles: (files) => runVitest([...files], reportPath.replace(/\.json$/, '.retry.json')), commit: runCmdSync('git', ['rev-parse', 'HEAD'], { cwd: repoRoot }).stdout.trim(), configHash: configHash(), diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts index ce7425f9f..cb6df1314 100644 --- a/scripts/lib/contention-retry.ts +++ b/scripts/lib/contention-retry.ts @@ -233,14 +233,11 @@ export function expiredRetryEntries(now: Date): readonly ContentionRetryEntry[] return CONTENTION_RETRY_FILES.filter((entry) => entry.reviewBy < today); } -/** - * Vitest's own timeout error, verbatim (`makeTimeoutError`, @vitest/runner - * 4.1.8): a plain `Error` whose whole message is the runner's template. - */ -const VITEST_TIMEOUT_MESSAGE = +/** Vitest's runner timeout error, verbatim (`makeTimeoutError`, @vitest/runner 4.1.8). */ +const RUNNER_TIMEOUT_MESSAGE = /^(Test|Hook) timed out in \d+ms\.\nIf this is a long-running (test|hook), pass a timeout value as the last argument/; -/** The error shape the lane reporter classifies, as Vitest hands it over. */ +/** An error as the lane reporter receives it from Vitest. */ export type ReportedError = { name?: unknown; message?: unknown; @@ -248,43 +245,28 @@ export type ReportedError = { actual?: unknown; }; -/** The runner-owned facts a timeout claim is checked against. */ -export type TimeoutEvidence = { - /** `TestCase.options.timeout` — the runner's configured budget for this test. */ +/** `TestCase.options.timeout` and `TestCase.diagnostic().duration`. */ +export type TestBudget = { timeoutMs: number | undefined; - /** `TestCase.diagnostic().duration` — how long the runner let it run. */ durationMs: number | undefined; }; -/** - * Whether a failed test was aborted by the runner's own timeout, the only - * failure this lane may retry. - * - * Vitest 4 ships no reason code on the timeout error, so eligibility is decided - * primarily by structured runner metadata that test code cannot author: the test - * must have consumed its entire configured budget (`options.timeout`, measured - * by `diagnostic().duration`). A forged `throw new Error()` - * fails here — it returns long before the budget — and a test that *does* run to - * its budget is aborted by the runner, so the error the runner reports is its - * own. The message template and error shape are then required as corroboration: - * a plain `Error` (assertion libraries set `name`) with no `expected`/`actual` - * diff fields. All of it must hold, and every error on the test must qualify; - * anything unclassifiable fails the job on the first run. - */ -export function isRunnerTimeout( - errors: readonly ReportedError[], - evidence: TimeoutEvidence, -): boolean { - const { timeoutMs, durationMs } = evidence; +/** A test the runner aborted at its timeout — the only failure this lane retries. */ +export function isRunnerTimeout(errors: readonly ReportedError[], budget: TestBudget): boolean { + return consumedWholeBudget(budget) && errors.length > 0 && errors.every(raisedByRunnerTimeout); +} + +/** The runner aborts a test only once it has run for its whole configured budget. */ +function consumedWholeBudget({ timeoutMs, durationMs }: TestBudget): boolean { if (typeof timeoutMs !== 'number' || timeoutMs <= 0) return false; - if (typeof durationMs !== 'number' || durationMs < timeoutMs) return false; - return errors.length > 0 && errors.every((error) => isTimeoutErrorShape(error)); + return typeof durationMs === 'number' && durationMs >= timeoutMs; } -function isTimeoutErrorShape(error: ReportedError): boolean { +/** The runner raises a plain `Error` holding its template, never an assertion diff. */ +function raisedByRunnerTimeout(error: ReportedError): boolean { if (error.name !== 'Error') return false; if (error.expected !== undefined || error.actual !== undefined) return false; - return typeof error.message === 'string' && VITEST_TIMEOUT_MESSAGE.test(error.message); + return typeof error.message === 'string' && RUNNER_TIMEOUT_MESSAGE.test(error.message); } /** One failed test case as read from a runner report. */ diff --git a/scripts/lib/run-blocker-bus.ts b/scripts/lib/run-blocker-bus.ts index 36c9e27ba..08ccb5455 100644 --- a/scripts/lib/run-blocker-bus.ts +++ b/scripts/lib/run-blocker-bus.ts @@ -1,23 +1,16 @@ -// Structured blocker channel between Vitest reporters (#1419). -// -// A gate reporter that fails a run without failing a test (the slow-test -// ratchet, for example) is invisible to anything that only reads test results: -// the contention retry lane would see one retry-eligible timeout, rerun it, and -// the green rerun would erase the gate's verdict. Gates publish their verdict -// here instead, and the retry lane's failure sink drains it — both reporters run -// in the same Vitest process, and the sink is ordered last in -// `vitest.config.ts`'s `reporters()` so every gate has already reported. +// How a gate reporter that fails a run without failing a test makes that verdict +// visible to the other reporters in the run (#1419). import type { RunBlocker } from './contention-retry.ts'; const blockers: RunBlocker[] = []; -/** Publish a verdict that fails the run without failing a test. */ +/** Publish a verdict that fails the run. Every gate reporter must call this. */ export function recordRunBlocker(blocker: RunBlocker): void { blockers.push(blocker); } -/** Take everything published so far, clearing the channel. */ +/** Take everything published so far, clearing the channel. Drained after the gates run. */ export function drainRunBlockers(): RunBlocker[] { return blockers.splice(0, blockers.length); } diff --git a/scripts/vitest-slow-test-reporter.ts b/scripts/vitest-slow-test-reporter.ts index fc06c5125..d69dae6e8 100644 --- a/scripts/vitest-slow-test-reporter.ts +++ b/scripts/vitest-slow-test-reporter.ts @@ -100,9 +100,6 @@ export default function slowTestGateReporter(): Reporter { // eslint-disable-next-line no-console if (reportSlowTests(offenders, (message) => console.error(message))) { process.exitCode = 1; - // This gate fails the run without failing a test, so it must be - // published structurally: otherwise a retry lane sees only the - // retry-eligible failures and a green rerun erases this verdict. recordRunBlocker({ kind: 'slow-test gate', detail: `${offenders.filter((offender) => offender.enforce).length} test(s) exceeded the wall-clock budget`, diff --git a/vitest.config.ts b/vitest.config.ts index ba54f9d44..20a53b7f7 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -20,18 +20,11 @@ import slowTestGateReporter from './scripts/vitest-slow-test-reporter.ts'; // single-retry policy (#1419) reads, so the two cannot drift. export { SUBPROCESS_STUB_TESTS }; -/** - * Reporters, composed here rather than on the retry lane's command line: a - * `--reporter` flag *replaces* this list, which would silently drop the - * slow-test gate from the Coverage job. The retry lane opts its failure sink in - * through the environment instead. - */ +/** Reporters for every lane; a `--reporter` flag would replace them, so no lane passes one. */ export function reporters(env: NodeJS.ProcessEnv = process.env): Array { - return [ - 'default', - slowTestGateReporter(), - ...(env[FAILURE_FILE_ENV] ? [contentionRetryReporter()] : []), - ]; + const gates: Array = ['default', slowTestGateReporter()]; + // The failure sink drains the gates' verdicts, so it reports after them. + return env[FAILURE_FILE_ENV] ? [...gates, contentionRetryReporter()] : gates; } export default defineConfig({ From 7145e752abb1d422fd5439dbada82707024a1ee5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 12:42:32 +0000 Subject: [PATCH 09/12] test(ci): mark runner-aborted timeouts inside the runner instead of inferring them --- .fallowrc.json | 8 ++ docs/agents/testing.md | 17 ++-- scripts/lib/contention-retry-policy.test.ts | 95 ++++++++++--------- scripts/lib/contention-retry-reporter.ts | 5 +- scripts/lib/contention-retry-run.ts | 1 + scripts/lib/contention-retry.ts | 34 ++----- scripts/lib/runner-timeout-meta.ts | 9 ++ scripts/vitest-runner-timeout-setup.ts | 25 +++++ .../timeout-provenance.fixture.ts | 37 ++++++++ .../vitest.fixture.config.ts | 13 +++ vitest.config.ts | 32 +++---- 11 files changed, 174 insertions(+), 102 deletions(-) create mode 100644 scripts/lib/runner-timeout-meta.ts create mode 100644 scripts/vitest-runner-timeout-setup.ts create mode 100644 test/contention-retry-fixtures/timeout-provenance.fixture.ts create mode 100644 test/contention-retry-fixtures/vitest.fixture.config.ts diff --git a/.fallowrc.json b/.fallowrc.json index df083557b..a195a74c8 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -19,6 +19,9 @@ "scripts/patch-xcuitest-runner-icon.ts", "scripts/runner-request-count/run.ts", "scripts/lib/contention-retry-run.ts", + "scripts/vitest-runner-timeout-setup.ts", + "test/contention-retry-fixtures/vitest.fixture.config.ts", + "test/contention-retry-fixtures/timeout-provenance.fixture.ts", "src/utils/update-check-entry.ts", "examples/sdk/client-session.ts", "examples/sdk/metro-runtime.ts", @@ -83,6 +86,11 @@ "file": "scripts/lib/{contention-retry.ts,contention-retry-reporter.ts}", "exports": ["default", "SUBPROCESS_STUB_TESTS"] }, + { + "comment": "Contention retry gate fixtures (#1419): the config default is loaded by the child `vitest run --config` the gate test spawns.", + "file": "test/contention-retry-fixtures/vitest.fixture.config.ts", + "exports": ["default"] + }, { "comment": "Mutation-lane seam: readTestScope is consumed by vitest.mutation.config.ts, a tool config outside --production analysis.", "file": "scripts/mutation/test-scope.ts", diff --git a/docs/agents/testing.md b/docs/agents/testing.md index b077a6aca..91abcae06 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -392,14 +392,15 @@ the retry policy cannot drift apart. The CI Coverage job runs the suite through `pnpm test:coverage:ci` (`scripts/lib/contention-retry-run.ts`), which applies one rule: -- **Timeouts only, decided from runner metadata.** A rerun happens only when *every* failure in the - run is a test the runner itself aborted, in a listed file. Eligibility keys on structured runner - facts test code cannot author (`isRunnerTimeout`): the test must have consumed its whole configured - budget — `TestCase.options.timeout` versus `diagnostic().duration` — with Vitest's timeout template - and a diff-free plain `Error` required as corroboration. A hand-thrown `Error` carrying the exact - timeout message returns long before the budget and is refused; a test that really does run to its - budget is aborted by the runner, so the error is the runner's. One assertion failure — in a listed - file or not — fails the job on the first run, so a real regression can never be papered over. +- **Timeouts only, proven by the runner.** A rerun happens only when *every* failure in the run is a + test the runner itself aborted, in a listed file. Eligibility comes from a mark written inside the + runner (`scripts/vitest-runner-timeout-setup.ts`, a setup file on every project): the runner owns + the controller behind `context.signal` and aborts it with the timeout error it raises, so a test + that merely *throws* the exact timeout message — immediately, or after blocking the event loop past + its budget — never carries the mark. Error text is never consulted for eligibility; + `test/contention-retry-fixtures/` drives those forgeries through a real Vitest run in the gate. One + assertion failure — in a listed file or not — fails the job on the first run, so a real regression + can never be papered over. - **Anything a rerun cannot re-check blocks the retry.** Unhandled errors, module load/setup errors, a coverage-threshold miss, or a nonzero exit no failed test explains are recorded as blockers (`scripts/lib/contention-retry-blockers.ts`) and fail the job, so a green retry can never erase a diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts index a76196e87..b249b61de 100644 --- a/scripts/lib/contention-retry-policy.test.ts +++ b/scripts/lib/contention-retry-policy.test.ts @@ -7,6 +7,7 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; import { test } from 'node:test'; +import { runCmdSync } from '../../src/utils/exec.ts'; import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; import { processBlockers } from './contention-retry-blockers.ts'; import contentionRetryReporter, { @@ -14,6 +15,7 @@ import contentionRetryReporter, { runBlockers, writeFailureReport, } from './contention-retry-reporter.ts'; +import { RUNNER_TIMEOUT_META } from './runner-timeout-meta.ts'; import { CONTENTION_RETRY_FILES, expiredRetryEntries, @@ -26,6 +28,7 @@ import { } from './contention-retry.ts'; const repoRoot = path.resolve(import.meta.dirname, '../..'); +const FIXTURE_CONFIG = 'test/contention-retry-fixtures/vitest.fixture.config.ts'; const LISTED = 'src/daemon/__tests__/request-router-open.test.ts'; // Vitest's own timeout error, verbatim (@vitest/runner `makeTimeoutError`). @@ -51,23 +54,21 @@ function assertionFailure(overrides: Partial = {}): TestFailure { return failure({ message: ASSERTION_MESSAGE, timeout: false, ...overrides }); } -const TIMEOUT_MS = 5000; +/** What the runner-timeout setup file writes on a test the runner aborted. */ +const RUNNER_ABORTED = { [RUNNER_TIMEOUT_META]: true }; function testCaseStub( errors: ReadonlyArray>, - overrides: { state?: string; timeoutMs?: number; durationMs?: number } = {}, + overrides: { state?: string; meta?: Record } = {}, ): unknown { return { fullName: 'opens a session', module: { moduleId: `${repoRoot}/${LISTED}` }, - options: { timeout: overrides.timeoutMs ?? TIMEOUT_MS }, - diagnostic: () => ({ duration: overrides.durationMs ?? TIMEOUT_MS + 4 }), + meta: () => overrides.meta ?? RUNNER_ABORTED, result: () => ({ state: overrides.state ?? 'failed', errors }), }; } -const RAN_TO_BUDGET = { timeoutMs: TIMEOUT_MS, durationMs: TIMEOUT_MS + 4 }; - function lane(runs: { first: TestRun; retry?: TestRun; today?: Date }): { result: ReturnType; rerun: string[][]; @@ -150,57 +151,61 @@ test('the expiry gate fails the run before any test executes', async () => { assert.deepEqual(rerun, []); }); -test('only a test the runner aborted at its own budget classifies as a timeout', () => { - assert.ok(isRunnerTimeout([VITEST_TIMEOUT], RAN_TO_BUDGET)); - assert.ok( - isRunnerTimeout( - [ - { - name: 'Error', - message: - 'Hook timed out in 10000ms.\nIf this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".', - }, - ], - RAN_TO_BUDGET, - ), - ); - // Runner metadata is the primary evidence: test code cannot make its own - // failure consume the whole configured budget and still choose the error. - assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], { timeoutMs: TIMEOUT_MS, durationMs: 12 })); - assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], { timeoutMs: undefined, durationMs: 99_999 })); - assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], { timeoutMs: 0, durationMs: 0 })); - assert.ok(!isRunnerTimeout([], RAN_TO_BUDGET)); - // An assertion must not be able to talk its way into a retry, whatever it says. +test('only the runner-owned abort mark classifies a failure as a timeout', () => { + assert.ok(isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED)); + // Without the runner's mark, no message and no error shape can earn a retry. for (const impostor of [ + { name: 'Error', message: VITEST_TIMEOUT.message }, { name: 'AssertionError', message: VITEST_TIMEOUT.message }, - { name: 'AssertionError', message: 'expected ETIMEDOUT, got ECONNRESET' }, - { name: 'Error', message: `saw: ${VITEST_TIMEOUT.message}` }, { name: 'Error', message: 'connect ETIMEDOUT 127.0.0.1:8080' }, { name: 'Error', message: 'Closing timeout while tearing down the daemon' }, - { name: 'Error', message: VITEST_TIMEOUT.message, expected: 'OK', actual: 'DEVICE_IN_USE' }, ]) { - assert.ok(!isRunnerTimeout([impostor], RAN_TO_BUDGET), `${impostor.name}: ${impostor.message}`); + assert.ok(!isRunnerTimeout([impostor], {}), `${impostor.name}: ${impostor.message}`); + assert.ok(!isRunnerTimeout([impostor], undefined)); + assert.ok(!isRunnerTimeout([impostor], { [RUNNER_TIMEOUT_META]: 'yes' })); } + // An aborted test that also produced an assertion diff is a regression. + assert.ok( + !isRunnerTimeout( + [VITEST_TIMEOUT, { name: 'AssertionError', message: 'x', expected: 1, actual: 2 }], + RUNNER_ABORTED, + ), + ); + assert.ok(!isRunnerTimeout([], RUNNER_ABORTED)); }); -test('an exact-template Error thrown by the test itself never retries', async () => { - // The forgery the classifier must refuse: right name, right message, right - // shape — but the test returned long before its budget was up. - const forged = failedTestCase( - testCaseStub([{ name: 'Error', message: VITEST_TIMEOUT.message }], { durationMs: 7 }) as never, - ); - assert.ok(forged); - assert.equal(forged.timeout, false, 'a hand-thrown timeout message is not a runner timeout'); - const { result, rerun } = lane({ first: { ok: false, failures: [{ ...forged, file: LISTED }] } }); - const resolved = await result; - assert.equal(resolved.ok, false); - assert.deepEqual(rerun, []); - assert.equal(resolved.envelope.data.retryCount, 0); +test('a real Vitest run marks only runner-aborted tests as retry-eligible', () => { + // The classifier's inputs come from the runner, so this case drives a real + // child Vitest run over test/contention-retry-fixtures/ rather than stubs. + const report = path.join(repoRoot, '.tmp/contention-retry/fixture-gate.json'); + fs.rmSync(report, { force: true }); + runCmdSync('pnpm', ['exec', 'vitest', 'run', '--config', FIXTURE_CONFIG], { + cwd: repoRoot, + env: { ...process.env, CONTENTION_RETRY_FAILURES: report }, + allowFailure: true, + }); + const { failures } = parseFailureReport(JSON.parse(fs.readFileSync(report, 'utf8')), repoRoot); + assert.deepEqual(Object.fromEntries(failures.map((entry) => [entry.testName, entry.timeout])), { + 'genuine runner timeout': true, + 'timeout message thrown immediately': false, + // The forgery this gate exists for: blocking the event loop past the + // budget does not abort the signal, so the throw stays a plain failure. + 'timeout message thrown after blocking the event loop past the budget': false, + 'assertion failure after blocking the event loop past the budget': false, + 'plain assertion failure': false, + }); + // Only the genuine timeout may reach a rerun. + const listed = failures.map((entry) => ({ ...entry, file: LISTED })); + const plan = planContentionRetry(listed); + assert.equal(plan.retry, false); + assert.equal(planContentionRetry(listed.filter((entry) => entry.timeout)).retry, true); }); test('an assertion message quoting a timeout still fails on the first run', async () => { const impostor = failedTestCase( - testCaseStub([{ name: 'AssertionError', message: VITEST_TIMEOUT.message }]) as never, + testCaseStub([{ name: 'AssertionError', message: VITEST_TIMEOUT.message }], { + meta: {}, + }) as never, ); assert.ok(impostor); assert.equal(impostor.timeout, false); diff --git a/scripts/lib/contention-retry-reporter.ts b/scripts/lib/contention-retry-reporter.ts index 25d1dd3f1..503242280 100644 --- a/scripts/lib/contention-retry-reporter.ts +++ b/scripts/lib/contention-retry-reporter.ts @@ -22,10 +22,7 @@ export function failedTestCase(testCase: TestCase): TestFailure | null { file: (testCase.module as TestModule).moduleId, testName: testCase.fullName, message: errors.map((error) => `${error.name}: ${error.message}`).join('\n'), - timeout: isRunnerTimeout(errors, { - timeoutMs: testCase.options.timeout, - durationMs: testCase.diagnostic()?.duration, - }), + timeout: isRunnerTimeout(errors, testCase.meta()), }; } diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index f3a68087b..c3343c9e4 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -94,6 +94,7 @@ function configHash(): string { 'scripts/lib/contention-retry-reporter.ts', 'scripts/lib/contention-retry-blockers.ts', 'scripts/lib/run-blocker-bus.ts', + 'scripts/vitest-runner-timeout-setup.ts', ]) { hash.update(fs.readFileSync(path.join(repoRoot, file))); } diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts index cb6df1314..c01b3c8c8 100644 --- a/scripts/lib/contention-retry.ts +++ b/scripts/lib/contention-retry.ts @@ -8,14 +8,16 @@ // // - the retry set is ENUMERATED here, never a glob: a glob would silently // enroll every future file under a directory, -// - only timeout-shaped failures retry; an assertion failure in a listed file -// fails the job on the first run, +// - only tests the runner itself aborted at their timeout retry; an assertion +// failure in a listed file fails the job on the first run, // - one retry, of the failed files only, and every retry is reported in the // job summary so a permanently flaky file cannot hide behind a green check, // - each entry is an owned waiver in the ADR 0011 sense: a tracking issue plus // a review date, and an expired entry fails the gate until it is renewed or // removed. +import { runnerTimedOut } from './runner-timeout-meta.ts'; + /** One enumerated retry-eligible file, owned like an ADR 0011 waiver. */ export type ContentionRetryEntry = { /** Repo-relative test file path. Exact path — never a glob. */ @@ -233,10 +235,6 @@ export function expiredRetryEntries(now: Date): readonly ContentionRetryEntry[] return CONTENTION_RETRY_FILES.filter((entry) => entry.reviewBy < today); } -/** Vitest's runner timeout error, verbatim (`makeTimeoutError`, @vitest/runner 4.1.8). */ -const RUNNER_TIMEOUT_MESSAGE = - /^(Test|Hook) timed out in \d+ms\.\nIf this is a long-running (test|hook), pass a timeout value as the last argument/; - /** An error as the lane reporter receives it from Vitest. */ export type ReportedError = { name?: unknown; @@ -245,28 +243,14 @@ export type ReportedError = { actual?: unknown; }; -/** `TestCase.options.timeout` and `TestCase.diagnostic().duration`. */ -export type TestBudget = { - timeoutMs: number | undefined; - durationMs: number | undefined; -}; - /** A test the runner aborted at its timeout — the only failure this lane retries. */ -export function isRunnerTimeout(errors: readonly ReportedError[], budget: TestBudget): boolean { - return consumedWholeBudget(budget) && errors.length > 0 && errors.every(raisedByRunnerTimeout); -} - -/** The runner aborts a test only once it has run for its whole configured budget. */ -function consumedWholeBudget({ timeoutMs, durationMs }: TestBudget): boolean { - if (typeof timeoutMs !== 'number' || timeoutMs <= 0) return false; - return typeof durationMs === 'number' && durationMs >= timeoutMs; +export function isRunnerTimeout(errors: readonly ReportedError[], meta: unknown): boolean { + return runnerTimedOut(meta) && errors.length > 0 && errors.every(hasNoAssertionDiff); } -/** The runner raises a plain `Error` holding its template, never an assertion diff. */ -function raisedByRunnerTimeout(error: ReportedError): boolean { - if (error.name !== 'Error') return false; - if (error.expected !== undefined || error.actual !== undefined) return false; - return typeof error.message === 'string' && RUNNER_TIMEOUT_MESSAGE.test(error.message); +/** A timeout that also failed an assertion is a regression, not contention. */ +function hasNoAssertionDiff(error: ReportedError): boolean { + return error.expected === undefined && error.actual === undefined; } /** One failed test case as read from a runner report. */ diff --git a/scripts/lib/runner-timeout-meta.ts b/scripts/lib/runner-timeout-meta.ts new file mode 100644 index 000000000..4909a5ade --- /dev/null +++ b/scripts/lib/runner-timeout-meta.ts @@ -0,0 +1,9 @@ +// The provenance mark the runner-timeout setup file writes on a test, and the +// contention retry lane (#1419) reads back through `TestCase.meta()`. + +export const RUNNER_TIMEOUT_META = 'agentDeviceRunnerTimeout'; + +/** True when the runner itself aborted the test with its own timeout error. */ +export function runnerTimedOut(meta: unknown): boolean { + return (meta as Record | undefined)?.[RUNNER_TIMEOUT_META] === true; +} diff --git a/scripts/vitest-runner-timeout-setup.ts b/scripts/vitest-runner-timeout-setup.ts new file mode 100644 index 000000000..b40aef8ce --- /dev/null +++ b/scripts/vitest-runner-timeout-setup.ts @@ -0,0 +1,25 @@ +// Marks, from inside the runner, every test the runner itself aborted at its +// timeout — the only failures the contention retry lane (#1419) may rerun. +// +// `context.signal` is the runner's own AbortSignal: only the runner holds its +// controller, and it aborts with the timeout error it raised. A test body can +// throw whatever it likes, including the runner's exact timeout text, without +// ever aborting the signal. + +import { beforeEach, onTestFailed } from 'vitest'; +import { RUNNER_TIMEOUT_META } from './lib/runner-timeout-meta.ts'; + +/** Distinguishes the timeout abort from a run cancellation, on the runner's own reason. */ +const RUNNER_TIMEOUT_REASON = /^(Test|Hook) timed out in \d+ms\./; + +beforeEach(({ signal, task }) => { + onTestFailed(() => { + if ( + signal.aborted && + signal.reason instanceof Error && + RUNNER_TIMEOUT_REASON.test(signal.reason.message) + ) { + task.meta[RUNNER_TIMEOUT_META] = true; + } + }); +}); diff --git a/test/contention-retry-fixtures/timeout-provenance.fixture.ts b/test/contention-retry-fixtures/timeout-provenance.fixture.ts new file mode 100644 index 000000000..07d402cf0 --- /dev/null +++ b/test/contention-retry-fixtures/timeout-provenance.fixture.ts @@ -0,0 +1,37 @@ +// Failures the contention retry policy must classify, run by the gate test in a +// child Vitest process (test/contention-retry-fixtures/vitest.fixture.config.ts) +// and never by the normal suites. Each name is asserted there. + +import { expect, test } from 'vitest'; + +const RUNNER_TIMEOUT_MESSAGE = + 'Test timed out in 50ms.\nIf this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".'; + +function blockPastBudget(): void { + const until = Date.now() + 200; + while (Date.now() < until) { + // Deny the runner's timeout timer a turn until the budget is long gone. + } +} + +test('genuine runner timeout', async () => { + await new Promise((resolve) => setTimeout(resolve, 5_000)); +}, 50); + +test('timeout message thrown immediately', () => { + throw new Error(RUNNER_TIMEOUT_MESSAGE); +}, 50); + +test('timeout message thrown after blocking the event loop past the budget', () => { + blockPastBudget(); + throw new Error(`FORGED ${RUNNER_TIMEOUT_MESSAGE}`); +}, 50); + +test('assertion failure after blocking the event loop past the budget', () => { + blockPastBudget(); + expect('DEVICE_IN_USE').toBe('OK'); +}, 50); + +test('plain assertion failure', () => { + expect('DEVICE_IN_USE').toBe('OK'); +}); diff --git a/test/contention-retry-fixtures/vitest.fixture.config.ts b/test/contention-retry-fixtures/vitest.fixture.config.ts new file mode 100644 index 000000000..404ad2cdf --- /dev/null +++ b/test/contention-retry-fixtures/vitest.fixture.config.ts @@ -0,0 +1,13 @@ +// Child-process Vitest config for the contention retry gate's fixtures: the +// same setup and reporters the real lanes use, pointed at the fixture file. + +import { defineConfig } from 'vitest/config'; +import { reporters } from '../../vitest.config.ts'; + +export default defineConfig({ + test: { + include: ['test/contention-retry-fixtures/*.fixture.ts'], + reporters: reporters(), + setupFiles: ['scripts/vitest-runner-timeout-setup.ts'], + }, +}); diff --git a/vitest.config.ts b/vitest.config.ts index 20a53b7f7..9745c9282 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -20,6 +20,13 @@ import slowTestGateReporter from './scripts/vitest-slow-test-reporter.ts'; // single-retry policy (#1419) reads, so the two cannot drift. export { SUBPROCESS_STUB_TESTS }; +/** Every project loads the same setup, including the runner-timeout provenance hook. */ +const SETUP_FILES = [ + 'scripts/vitest-runner-timeout-setup.ts', + 'src/__tests__/hermetic-env-setup.ts', + 'src/__tests__/process-memo-setup.ts', +]; + /** Reporters for every lane; a `--reporter` flag would replace them, so no lane passes one. */ export function reporters(env: NodeJS.ProcessEnv = process.env): Array { const gates: Array = ['default', slowTestGateReporter()]; @@ -60,10 +67,7 @@ export default defineConfig({ // job (scripts/maestro-conformance), like the layering guard. ], exclude: [...SUBPROCESS_STUB_TESTS], - setupFiles: [ - 'src/__tests__/hermetic-env-setup.ts', - 'src/__tests__/process-memo-setup.ts', - ], + setupFiles: SETUP_FILES, }, }, { @@ -75,10 +79,7 @@ export default defineConfig({ test: { name: 'subprocess-stub', include: [...SUBPROCESS_STUB_TESTS], - setupFiles: [ - 'src/__tests__/hermetic-env-setup.ts', - 'src/__tests__/process-memo-setup.ts', - ], + setupFiles: SETUP_FILES, fileParallelism: false, isolate: true, maxWorkers: 1, @@ -88,30 +89,21 @@ export default defineConfig({ test: { name: 'provider-integration', include: ['test/integration/provider-scenarios/**/*.test.ts'], - setupFiles: [ - 'src/__tests__/hermetic-env-setup.ts', - 'src/__tests__/process-memo-setup.ts', - ], + setupFiles: SETUP_FILES, }, }, { test: { name: 'interaction-contract', include: ['test/integration/interaction-contract/**/*.test.ts'], - setupFiles: [ - 'src/__tests__/hermetic-env-setup.ts', - 'src/__tests__/process-memo-setup.ts', - ], + setupFiles: SETUP_FILES, }, }, { test: { name: 'output-economy', include: ['test/output-economy/**/*.test.ts'], - setupFiles: [ - 'src/__tests__/hermetic-env-setup.ts', - 'src/__tests__/process-memo-setup.ts', - ], + setupFiles: SETUP_FILES, }, }, ], From a06b38bbd2be7b9140ce080dee678d7e1b4f7bd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 13:50:27 +0000 Subject: [PATCH 10/12] test(ci): make timeout provenance a per-run secret, not a writable flag Cover direct task.meta mutation in the real child-Vitest fixture gate. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- docs/agents/testing.md | 5 ++- scripts/lib/contention-retry-policy.test.ts | 42 ++++++++++++++----- scripts/lib/contention-retry-reporter.ts | 7 ++-- scripts/lib/contention-retry-run.ts | 6 ++- scripts/lib/contention-retry.ts | 8 +++- scripts/lib/runner-timeout-meta.ts | 19 ++++++++- scripts/vitest-runner-timeout-setup.ts | 20 +++++---- .../timeout-provenance.fixture.ts | 25 +++++++++++ 8 files changed, 106 insertions(+), 26 deletions(-) diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 91abcae06..bd8459a70 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -397,7 +397,10 @@ The CI Coverage job runs the suite through `pnpm test:coverage:ci` runner (`scripts/vitest-runner-timeout-setup.ts`, a setup file on every project): the runner owns the controller behind `context.signal` and aborts it with the timeout error it raises, so a test that merely *throws* the exact timeout message — immediately, or after blocking the event loop past - its budget — never carries the mark. Error text is never consulted for eligibility; + its budget — never carries the mark. `task.meta` is writable by test code, so the mark is not a + flag but the run's secret: the lane mints it per run, and the setup file takes it out of the + environment as it loads — before any test module is imported — so a test writing the marker itself + has no value to write. Error text is never consulted for eligibility; `test/contention-retry-fixtures/` drives those forgeries through a real Vitest run in the gate. One assertion failure — in a listed file or not — fails the job on the first run, so a real regression can never be papered over. diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts index b249b61de..a707972e3 100644 --- a/scripts/lib/contention-retry-policy.test.ts +++ b/scripts/lib/contention-retry-policy.test.ts @@ -15,7 +15,7 @@ import contentionRetryReporter, { runBlockers, writeFailureReport, } from './contention-retry-reporter.ts'; -import { RUNNER_TIMEOUT_META } from './runner-timeout-meta.ts'; +import { RUNNER_TIMEOUT_META, RUNNER_TIMEOUT_TOKEN_ENV } from './runner-timeout-meta.ts'; import { CONTENTION_RETRY_FILES, expiredRetryEntries, @@ -54,8 +54,11 @@ function assertionFailure(overrides: Partial = {}): TestFailure { return failure({ message: ASSERTION_MESSAGE, timeout: false, ...overrides }); } +/** The run's secret, minted by the lane and never visible to a test module. */ +const TOKEN = 'a4ec0a4a-0f77-4f2e-9d4a-6d7f1d3f0d21'; + /** What the runner-timeout setup file writes on a test the runner aborted. */ -const RUNNER_ABORTED = { [RUNNER_TIMEOUT_META]: true }; +const RUNNER_ABORTED = { [RUNNER_TIMEOUT_META]: TOKEN }; function testCaseStub( errors: ReadonlyArray>, @@ -152,7 +155,14 @@ test('the expiry gate fails the run before any test executes', async () => { }); test('only the runner-owned abort mark classifies a failure as a timeout', () => { - assert.ok(isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED)); + assert.ok(isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED, TOKEN)); + // `task.meta` is test-writable, so a mark that is not the run's secret is worthless. + for (const forged of [true, 'true', 'agentDeviceRunnerTimeout', '', undefined]) { + assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], { [RUNNER_TIMEOUT_META]: forged }, TOKEN)); + } + // No secret configured means nothing is eligible. + assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED, undefined)); + assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED, '')); // Without the runner's mark, no message and no error shape can earn a retry. for (const impostor of [ { name: 'Error', message: VITEST_TIMEOUT.message }, @@ -160,18 +170,19 @@ test('only the runner-owned abort mark classifies a failure as a timeout', () => { name: 'Error', message: 'connect ETIMEDOUT 127.0.0.1:8080' }, { name: 'Error', message: 'Closing timeout while tearing down the daemon' }, ]) { - assert.ok(!isRunnerTimeout([impostor], {}), `${impostor.name}: ${impostor.message}`); - assert.ok(!isRunnerTimeout([impostor], undefined)); - assert.ok(!isRunnerTimeout([impostor], { [RUNNER_TIMEOUT_META]: 'yes' })); + assert.ok(!isRunnerTimeout([impostor], {}, TOKEN), `${impostor.name}: ${impostor.message}`); + assert.ok(!isRunnerTimeout([impostor], undefined, TOKEN)); + assert.ok(!isRunnerTimeout([impostor], { [RUNNER_TIMEOUT_META]: 'yes' }, TOKEN)); } // An aborted test that also produced an assertion diff is a regression. assert.ok( !isRunnerTimeout( [VITEST_TIMEOUT, { name: 'AssertionError', message: 'x', expected: 1, actual: 2 }], RUNNER_ABORTED, + TOKEN, ), ); - assert.ok(!isRunnerTimeout([], RUNNER_ABORTED)); + assert.ok(!isRunnerTimeout([], RUNNER_ABORTED, TOKEN)); }); test('a real Vitest run marks only runner-aborted tests as retry-eligible', () => { @@ -181,7 +192,11 @@ test('a real Vitest run marks only runner-aborted tests as retry-eligible', () = fs.rmSync(report, { force: true }); runCmdSync('pnpm', ['exec', 'vitest', 'run', '--config', FIXTURE_CONFIG], { cwd: repoRoot, - env: { ...process.env, CONTENTION_RETRY_FAILURES: report }, + env: { + ...process.env, + CONTENTION_RETRY_FAILURES: report, + [RUNNER_TIMEOUT_TOKEN_ENV]: TOKEN, + }, allowFailure: true, }); const { failures } = parseFailureReport(JSON.parse(fs.readFileSync(report, 'utf8')), repoRoot); @@ -193,6 +208,11 @@ test('a real Vitest run marks only runner-aborted tests as retry-eligible', () = 'timeout message thrown after blocking the event loop past the budget': false, 'assertion failure after blocking the event loop past the budget': false, 'plain assertion failure': false, + // `task.meta` is test-writable, but the mark is the run's secret and the + // setup file takes it out of the environment before any test module loads. + 'test writes the provenance marker itself': false, + 'test writes the marker after blocking the event loop past the budget': false, + 'test writes the marker from the environment it can still read': false, }); // Only the genuine timeout may reach a rerun. const listed = failures.map((entry) => ({ ...entry, file: LISTED })); @@ -206,6 +226,7 @@ test('an assertion message quoting a timeout still fails on the first run', asyn testCaseStub([{ name: 'AssertionError', message: VITEST_TIMEOUT.message }], { meta: {}, }) as never, + TOKEN, ); assert.ok(impostor); assert.equal(impostor.timeout, false); @@ -222,19 +243,20 @@ test('a test that timed out AND failed an assertion is not retry-eligible', () = VITEST_TIMEOUT, { name: 'AssertionError', message: 'expected 1 to be 2', expected: 2, actual: 1 }, ]) as never, + TOKEN, ); assert.equal(mixed?.timeout, false); }); test('the lane reporter keeps the real error message a timeout is classified by', () => { - const failed = failedTestCase(testCaseStub([VITEST_TIMEOUT]) as never); + const failed = failedTestCase(testCaseStub([VITEST_TIMEOUT]) as never, TOKEN); assert.deepEqual(failed, { file: `${repoRoot}/${LISTED}`, testName: 'opens a session', message: TIMEOUT_MESSAGE, timeout: true, }); - assert.equal(failedTestCase(testCaseStub([], { state: 'passed' }) as never), null); + assert.equal(failedTestCase(testCaseStub([], { state: 'passed' }) as never, TOKEN), null); const target = path.join(repoRoot, '.tmp/contention-retry/reporter-gate.json'); writeFailureReport({ failures: [failure()], blockers: [] }, target); diff --git a/scripts/lib/contention-retry-reporter.ts b/scripts/lib/contention-retry-reporter.ts index 503242280..accd58679 100644 --- a/scripts/lib/contention-retry-reporter.ts +++ b/scripts/lib/contention-retry-reporter.ts @@ -8,13 +8,14 @@ import path from 'node:path'; import type { Reporter, TestCase, TestModule } from 'vitest/node'; import { isRunnerTimeout, type RunBlocker, type TestFailure } from './contention-retry.ts'; import { drainRunBlockers } from './run-blocker-bus.ts'; +import { RUNNER_TIMEOUT_TOKEN_ENV } from './runner-timeout-meta.ts'; export const FAILURE_FILE_ENV = 'CONTENTION_RETRY_FAILURES'; export type FailureReport = { failures: TestFailure[]; blockers: RunBlocker[] }; /** The failed-test view of a Vitest test case, or null when it did not fail. */ -export function failedTestCase(testCase: TestCase): TestFailure | null { +export function failedTestCase(testCase: TestCase, token: string | undefined): TestFailure | null { const result = testCase.result(); if (result.state !== 'failed') return null; const errors = result.errors ?? []; @@ -22,7 +23,7 @@ export function failedTestCase(testCase: TestCase): TestFailure | null { file: (testCase.module as TestModule).moduleId, testName: testCase.fullName, message: errors.map((error) => `${error.name}: ${error.message}`).join('\n'), - timeout: isRunnerTimeout(errors, testCase.meta()), + timeout: isRunnerTimeout(errors, testCase.meta(), token), }; } @@ -59,7 +60,7 @@ export default function contentionRetryReporter(): Reporter { const failures: TestFailure[] = []; return { onTestCaseResult(testCase: TestCase): void { - const failed = failedTestCase(testCase); + const failed = failedTestCase(testCase, process.env[RUNNER_TIMEOUT_TOKEN_ENV]); if (failed) failures.push(failed); }, onTestRunEnd( diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index c3343c9e4..52fc4d345 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -14,6 +14,7 @@ import { runCmdStreaming, runCmdSync } from '../../src/utils/exec.ts'; import { parseScriptArgs } from './cli-args.ts'; import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; import { FAILURE_FILE_ENV } from './contention-retry-reporter.ts'; +import { RUNNER_TIMEOUT_TOKEN_ENV } from './runner-timeout-meta.ts'; import { processBlockers } from './contention-retry-blockers.ts'; import { parseFailureReport, type RunBlocker, type TestFailure } from './contention-retry.ts'; @@ -42,6 +43,9 @@ const reportPath = path.resolve(repoRoot, args.report); const envelopePath = path.resolve(repoRoot, args.envelope); const summaryPath = args.summary ?? process.env.GITHUB_STEP_SUMMARY; +/** The capability the runner needs to mark a timeout; test modules never see it. */ +const TIMEOUT_TOKEN = crypto.randomUUID(); + async function runVitest(extra: string[], report: string): Promise { fs.mkdirSync(path.dirname(report), { recursive: true }); fs.rmSync(report, { force: true }); @@ -52,7 +56,7 @@ async function runVitest(extra: string[], report: string): Promise { }; const result = await runCmdStreaming('pnpm', ['exec', 'vitest', 'run', ...extra], { cwd: repoRoot, - env: { ...process.env, [FAILURE_FILE_ENV]: report }, + env: { ...process.env, [FAILURE_FILE_ENV]: report, [RUNNER_TIMEOUT_TOKEN_ENV]: TIMEOUT_TOKEN }, allowFailure: true, onStdoutChunk: (chunk) => capture(chunk, (text) => process.stdout.write(text)), onStderrChunk: (chunk) => capture(chunk, (text) => process.stderr.write(text)), diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts index c01b3c8c8..cdb137639 100644 --- a/scripts/lib/contention-retry.ts +++ b/scripts/lib/contention-retry.ts @@ -244,8 +244,12 @@ export type ReportedError = { }; /** A test the runner aborted at its timeout — the only failure this lane retries. */ -export function isRunnerTimeout(errors: readonly ReportedError[], meta: unknown): boolean { - return runnerTimedOut(meta) && errors.length > 0 && errors.every(hasNoAssertionDiff); +export function isRunnerTimeout( + errors: readonly ReportedError[], + meta: unknown, + token: string | undefined, +): boolean { + return runnerTimedOut(meta, token) && errors.length > 0 && errors.every(hasNoAssertionDiff); } /** A timeout that also failed an assertion is a regression, not contention. */ diff --git a/scripts/lib/runner-timeout-meta.ts b/scripts/lib/runner-timeout-meta.ts index 4909a5ade..216352a43 100644 --- a/scripts/lib/runner-timeout-meta.ts +++ b/scripts/lib/runner-timeout-meta.ts @@ -1,9 +1,24 @@ // The provenance mark the runner-timeout setup file writes on a test, and the // contention retry lane (#1419) reads back through `TestCase.meta()`. +// +// `task.meta` is writable by test code, so the mark is not a boolean but a +// per-run secret the lane generates. Setup files are imported before any test +// module, and the setup file takes the secret out of the environment as it +// loads, so no test body can read the value it would have to forge. export const RUNNER_TIMEOUT_META = 'agentDeviceRunnerTimeout'; +export const RUNNER_TIMEOUT_TOKEN_ENV = 'CONTENTION_RETRY_TIMEOUT_TOKEN'; + +/** Reads the run's secret and removes it from `env`, leaving nothing for a test to find. */ +export function takeRunnerTimeoutToken(env: NodeJS.ProcessEnv): string | undefined { + const token = env[RUNNER_TIMEOUT_TOKEN_ENV]; + delete env[RUNNER_TIMEOUT_TOKEN_ENV]; + return token; +} + /** True when the runner itself aborted the test with its own timeout error. */ -export function runnerTimedOut(meta: unknown): boolean { - return (meta as Record | undefined)?.[RUNNER_TIMEOUT_META] === true; +export function runnerTimedOut(meta: unknown, token: string | undefined): boolean { + if (!token) return false; + return (meta as Record | undefined)?.[RUNNER_TIMEOUT_META] === token; } diff --git a/scripts/vitest-runner-timeout-setup.ts b/scripts/vitest-runner-timeout-setup.ts index b40aef8ce..cb6f447df 100644 --- a/scripts/vitest-runner-timeout-setup.ts +++ b/scripts/vitest-runner-timeout-setup.ts @@ -5,21 +5,27 @@ // controller, and it aborts with the timeout error it raised. A test body can // throw whatever it likes, including the runner's exact timeout text, without // ever aborting the signal. +// +// The mark is the run's secret, taken out of the environment here — before any +// test module is imported — so a test writing `task.meta` has nothing to write. import { beforeEach, onTestFailed } from 'vitest'; -import { RUNNER_TIMEOUT_META } from './lib/runner-timeout-meta.ts'; +import { RUNNER_TIMEOUT_META, takeRunnerTimeoutToken } from './lib/runner-timeout-meta.ts'; + +const TOKEN = takeRunnerTimeoutToken(process.env); /** Distinguishes the timeout abort from a run cancellation, on the runner's own reason. */ const RUNNER_TIMEOUT_REASON = /^(Test|Hook) timed out in \d+ms\./; +function abortedByRunnerTimeout(signal: AbortSignal): boolean { + if (!signal.aborted || !(signal.reason instanceof Error)) return false; + return RUNNER_TIMEOUT_REASON.test(signal.reason.message); +} + beforeEach(({ signal, task }) => { onTestFailed(() => { - if ( - signal.aborted && - signal.reason instanceof Error && - RUNNER_TIMEOUT_REASON.test(signal.reason.message) - ) { - task.meta[RUNNER_TIMEOUT_META] = true; + if (TOKEN && abortedByRunnerTimeout(signal)) { + (task.meta as Record)[RUNNER_TIMEOUT_META] = TOKEN; } }); }); diff --git a/test/contention-retry-fixtures/timeout-provenance.fixture.ts b/test/contention-retry-fixtures/timeout-provenance.fixture.ts index 07d402cf0..a08caa88e 100644 --- a/test/contention-retry-fixtures/timeout-provenance.fixture.ts +++ b/test/contention-retry-fixtures/timeout-provenance.fixture.ts @@ -3,10 +3,19 @@ // and never by the normal suites. Each name is asserted there. import { expect, test } from 'vitest'; +import { + RUNNER_TIMEOUT_META, + RUNNER_TIMEOUT_TOKEN_ENV, +} from '../../scripts/lib/runner-timeout-meta.ts'; const RUNNER_TIMEOUT_MESSAGE = 'Test timed out in 50ms.\nIf this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".'; +/** What a test body can do to `task.meta`, which the policy must not trust. */ +function forgeMark(task: { meta: object }, value: unknown): void { + (task.meta as Record)[RUNNER_TIMEOUT_META] = value; +} + function blockPastBudget(): void { const until = Date.now() + 200; while (Date.now() < until) { @@ -35,3 +44,19 @@ test('assertion failure after blocking the event loop past the budget', () => { test('plain assertion failure', () => { expect('DEVICE_IN_USE').toBe('OK'); }); + +test('test writes the provenance marker itself', ({ task }) => { + forgeMark(task, true); + throw new Error('DEVICE_IN_USE'); +}); + +test('test writes the marker after blocking the event loop past the budget', ({ task }) => { + forgeMark(task, true); + blockPastBudget(); + throw new Error(RUNNER_TIMEOUT_MESSAGE); +}, 50); + +test('test writes the marker from the environment it can still read', ({ task }) => { + forgeMark(task, process.env[RUNNER_TIMEOUT_TOKEN_ENV] ?? 'guess'); + throw new Error(RUNNER_TIMEOUT_MESSAGE); +}); From 87455445084fdc0dc363a29c44137c731ad5feb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 17:42:39 +0000 Subject: [PATCH 11/12] test(ci): retry the failed files in the first run's project and coverage modes Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- docs/agents/testing.md | 5 ++- scripts/lib/contention-retry-args.ts | 31 ++++++++++++++++++ scripts/lib/contention-retry-policy.test.ts | 36 +++++++++++++++++++++ scripts/lib/contention-retry-run.ts | 19 +++++++---- 4 files changed, 83 insertions(+), 8 deletions(-) create mode 100644 scripts/lib/contention-retry-args.ts diff --git a/docs/agents/testing.md b/docs/agents/testing.md index bd8459a70..57392cb06 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -414,7 +414,10 @@ The CI Coverage job runs the suite through `pnpm test:coverage:ci` sink drains it and refuses the rerun. **Any new gate reporter must call `recordRunBlocker`**, and must be ordered before the sink in `reporters()`. - **One retry, of the failed files only.** Not the suite, and never twice. Two timed-out tests in one - file are one retry, and count as one. + file are one retry, and count as one. The rerun keeps the first run's execution modes — the same + `--project` selection and the same V8 instrumentation (`scripts/lib/contention-retry-args.ts`), so + a coverage-job failure is never accepted by a run that could not reproduce it. Its coverage lands + in `coverage/contention-retry/`, leaving the first run's report as the changed-line gate's evidence. - **Retries stay visible.** Every retried file is named in the job summary with its tracking issue and review date, and the run writes the shared scheduled-lane envelope (`scripts/lib/lane-envelope.ts`, #1430) with the retry count, so a permanently flaky file shows up diff --git a/scripts/lib/contention-retry-args.ts b/scripts/lib/contention-retry-args.ts new file mode 100644 index 000000000..296b87beb --- /dev/null +++ b/scripts/lib/contention-retry-args.ts @@ -0,0 +1,31 @@ +// Vitest argv for the two runs of the contention retry lane (#1419). The retry +// must execute the failed files the same way the first run did — same projects, +// same V8 instrumentation — or a failure the Coverage job produced could be +// accepted by a run that could not reproduce it. + +export type RunModes = { projects: readonly string[]; coverage: boolean }; + +/** Where the retry's own coverage lands, so the first run's report stays the gate's evidence. */ +export const RETRY_COVERAGE_DIR = 'coverage/contention-retry'; + +function projectArgs(modes: RunModes): string[] { + return modes.projects.flatMap((name) => ['--project', name]); +} + +export function firstRunArgs(modes: RunModes): string[] { + return [...projectArgs(modes), ...(modes.coverage ? ['--coverage'] : [])]; +} + +export function retryRunArgs(modes: RunModes, files: readonly string[]): string[] { + if (!modes.coverage) return [...projectArgs(modes), ...files]; + // Global thresholds describe the whole suite; a handful of files can never meet + // them. A first-run threshold failure is a blocker, so the retry never runs. + return [ + ...projectArgs(modes), + '--coverage', + `--coverage.reportsDirectory=${RETRY_COVERAGE_DIR}`, + '--coverage.thresholds.statements=0', + '--coverage.thresholds.lines=0', + ...files, + ]; +} diff --git a/scripts/lib/contention-retry-policy.test.ts b/scripts/lib/contention-retry-policy.test.ts index a707972e3..0829b32e3 100644 --- a/scripts/lib/contention-retry-policy.test.ts +++ b/scripts/lib/contention-retry-policy.test.ts @@ -16,6 +16,7 @@ import contentionRetryReporter, { writeFailureReport, } from './contention-retry-reporter.ts'; import { RUNNER_TIMEOUT_META, RUNNER_TIMEOUT_TOKEN_ENV } from './runner-timeout-meta.ts'; +import { firstRunArgs, RETRY_COVERAGE_DIR, retryRunArgs } from './contention-retry-args.ts'; import { CONTENTION_RETRY_FILES, expiredRetryEntries, @@ -290,6 +291,41 @@ test('the Coverage lane keeps the configured reporters, failure sink included', ); }); +test('the retry reruns the failed files in the first run modes', () => { + const modes = { projects: ['unit-core', 'subprocess-stub'], coverage: true }; + assert.deepEqual(firstRunArgs(modes), [ + '--project', + 'unit-core', + '--project', + 'subprocess-stub', + '--coverage', + ]); + const retry = retryRunArgs(modes, [LISTED]); + assert.deepEqual(retry, [ + '--project', + 'unit-core', + '--project', + 'subprocess-stub', + '--coverage', + `--coverage.reportsDirectory=${RETRY_COVERAGE_DIR}`, + // A subset of files can never meet whole-suite thresholds, and a first-run + // threshold failure is a blocker, so the retry never runs after one. + '--coverage.thresholds.statements=0', + '--coverage.thresholds.lines=0', + LISTED, + ]); + // The gate reads the first run's report; the retry must not overwrite it. + assert.notEqual(RETRY_COVERAGE_DIR, 'coverage'); + assert.deepEqual(retryRunArgs({ projects: [], coverage: false }, [LISTED]), [LISTED]); + + const runner = fs.readFileSync( + path.join(repoRoot, 'scripts/lib/contention-retry-run.ts'), + 'utf8', + ); + assert.match(runner, /runAll: \(\) => runVitest\(firstRunArgs\(modes\)/); + assert.match(runner, /runFiles: \(files\) =>\s*runVitest\(retryRunArgs\(modes, files\)/); +}); + test('non-test failures block the retry instead of being rerun away', async () => { const covered = processBlockers({ ok: false, diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index 52fc4d345..8389393c2 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -12,6 +12,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { runCmdStreaming, runCmdSync } from '../../src/utils/exec.ts'; import { parseScriptArgs } from './cli-args.ts'; +import { firstRunArgs, retryRunArgs, type RunModes } from './contention-retry-args.ts'; import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts'; import { FAILURE_FILE_ENV } from './contention-retry-reporter.ts'; import { RUNNER_TIMEOUT_TOKEN_ENV } from './runner-timeout-meta.ts'; @@ -95,6 +96,7 @@ function configHash(): string { for (const file of [ 'vitest.config.ts', 'scripts/lib/contention-retry.ts', + 'scripts/lib/contention-retry-args.ts', 'scripts/lib/contention-retry-reporter.ts', 'scripts/lib/contention-retry-blockers.ts', 'scripts/lib/run-blocker-bus.ts', @@ -105,16 +107,19 @@ function configHash(): string { return `sha256:${hash.digest('hex').slice(0, 16)}`; } -const projects = (args.project ?? '') - .split(',') - .map((entry) => entry.trim()) - .filter(Boolean) - .flatMap((name) => ['--project', name]); +const modes: RunModes = { + projects: (args.project ?? '') + .split(',') + .map((entry) => entry.trim()) + .filter(Boolean), + coverage: args.coverage, +}; const startedAtMs = Date.now(); const result = await runWithContentionRetry({ - runAll: () => runVitest([...projects, ...(args.coverage ? ['--coverage'] : [])], reportPath), - runFiles: (files) => runVitest([...files], reportPath.replace(/\.json$/, '.retry.json')), + runAll: () => runVitest(firstRunArgs(modes), reportPath), + runFiles: (files) => + runVitest(retryRunArgs(modes, files), reportPath.replace(/\.json$/, '.retry.json')), commit: runCmdSync('git', ['rev-parse', 'HEAD'], { cwd: repoRoot }).stdout.trim(), configHash: configHash(), vitestVersion: vitestVersion(), From dca9be9e406c444be5d8419cba69ada8abae8d2d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 09:48:16 +0000 Subject: [PATCH 12/12] fix: drop deleted repo-health file from the retry list after #1480 Rebase onto main post-#1480: the SkillGym/repo-health descope deleted scripts/repo-health/run.test.ts, whose CONTENTION_RETRY_FILES entry would now fail this PR's own missing-file check, and inlined the slow-test budgets into the reporter, resolving the budgets-module import. Envelope comments now point at scripts/lib/lane-envelope.ts instead of the closed #1430. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FUv7bvbWNryuXgSBuqTtep --- scripts/lib/contention-retry-lane.ts | 2 +- scripts/lib/contention-retry-run.ts | 2 +- scripts/lib/contention-retry.ts | 7 ------- scripts/lib/lane-envelope.ts | 2 +- 4 files changed, 3 insertions(+), 10 deletions(-) diff --git a/scripts/lib/contention-retry-lane.ts b/scripts/lib/contention-retry-lane.ts index 57b97763a..58cb235b6 100644 --- a/scripts/lib/contention-retry-lane.ts +++ b/scripts/lib/contention-retry-lane.ts @@ -24,7 +24,7 @@ export type TestRun = { blockers?: readonly RunBlocker[]; }; -/** Telemetry payload for scheduled-lane health (#1430). */ +/** Telemetry payload written to the shared lane envelope (scripts/lib/lane-envelope.ts). */ export type ContentionRetryTelemetry = { /** Files rerun in this job (one entry per file, not per failed test). */ retried: ReadonlyArray<{ file: string; testNames: readonly string[]; trackingIssue: string }>; diff --git a/scripts/lib/contention-retry-run.ts b/scripts/lib/contention-retry-run.ts index 8389393c2..e51a6df42 100644 --- a/scripts/lib/contention-retry-run.ts +++ b/scripts/lib/contention-retry-run.ts @@ -5,7 +5,7 @@ // Runs Vitest once; if the only failures are timeout-shaped and live in the // enumerated retry list (scripts/lib/contention-retry.ts), reruns exactly those // files once. Every retried file is named in the job summary, and the run writes -// the shared scheduled-lane envelope so retry counts feed lane health (#1430). +// the shared lane envelope (scripts/lib/lane-envelope.ts) so retry counts stay observable. import crypto from 'node:crypto'; import fs from 'node:fs'; diff --git a/scripts/lib/contention-retry.ts b/scripts/lib/contention-retry.ts index cdb137639..3e0043102 100644 --- a/scripts/lib/contention-retry.ts +++ b/scripts/lib/contention-retry.ts @@ -172,13 +172,6 @@ export const CONTENTION_RETRY_FILES: readonly ContentionRetryEntry[] = [ reviewBy: REVIEW_BY, serializedStub: true, }, - { - file: 'scripts/repo-health/run.test.ts', - reason: 'Spawns the repo-health CLI over the real tree and waits for it to exit (#1423).', - trackingIssue: 'https://github.com/callstack/agent-device/issues/1423', - reviewBy: REVIEW_BY, - serializedStub: true, - }, { file: 'src/daemon/__tests__/request-router-open.test.ts', reason: 'Drives real open routing over keyed locks, waiting on lock/session settle budgets.', diff --git a/scripts/lib/lane-envelope.ts b/scripts/lib/lane-envelope.ts index 8c0c24342..1a03c294d 100644 --- a/scripts/lib/lane-envelope.ts +++ b/scripts/lib/lane-envelope.ts @@ -1,4 +1,4 @@ -// Standard artifact envelope for scheduled lanes (issue #1430). +// Standard artifact envelope for scheduled lanes. // // A scheduled lane can go dark or drift for weeks while PR CI stays green, so // every artifact it uploads must say — without a human reading logs — which