Skip to content

Commit 2ef3024

Browse files
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>
1 parent 65693d2 commit 2ef3024

8 files changed

Lines changed: 106 additions & 26 deletions

File tree

‎docs/agents/testing.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,10 @@ The CI Coverage job runs the suite through `pnpm test:coverage:ci`
311311
runner (`scripts/vitest-runner-timeout-setup.ts`, a setup file on every project): the runner owns
312312
the controller behind `context.signal` and aborts it with the timeout error it raises, so a test
313313
that merely *throws* the exact timeout message — immediately, or after blocking the event loop past
314-
its budget — never carries the mark. Error text is never consulted for eligibility;
314+
its budget — never carries the mark. `task.meta` is writable by test code, so the mark is not a
315+
flag but the run's secret: the lane mints it per run, and the setup file takes it out of the
316+
environment as it loads — before any test module is imported — so a test writing the marker itself
317+
has no value to write. Error text is never consulted for eligibility;
315318
`test/contention-retry-fixtures/` drives those forgeries through a real Vitest run in the gate. One
316319
assertion failure — in a listed file or not — fails the job on the first run, so a real regression
317320
can never be papered over.

‎scripts/lib/contention-retry-policy.test.ts‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import contentionRetryReporter, {
1515
runBlockers,
1616
writeFailureReport,
1717
} from './contention-retry-reporter.ts';
18-
import { RUNNER_TIMEOUT_META } from './runner-timeout-meta.ts';
18+
import { RUNNER_TIMEOUT_META, RUNNER_TIMEOUT_TOKEN_ENV } from './runner-timeout-meta.ts';
1919
import {
2020
CONTENTION_RETRY_FILES,
2121
expiredRetryEntries,
@@ -54,8 +54,11 @@ function assertionFailure(overrides: Partial<TestFailure> = {}): TestFailure {
5454
return failure({ message: ASSERTION_MESSAGE, timeout: false, ...overrides });
5555
}
5656

57+
/** The run's secret, minted by the lane and never visible to a test module. */
58+
const TOKEN = 'a4ec0a4a-0f77-4f2e-9d4a-6d7f1d3f0d21';
59+
5760
/** What the runner-timeout setup file writes on a test the runner aborted. */
58-
const RUNNER_ABORTED = { [RUNNER_TIMEOUT_META]: true };
61+
const RUNNER_ABORTED = { [RUNNER_TIMEOUT_META]: TOKEN };
5962

6063
function testCaseStub(
6164
errors: ReadonlyArray<Record<string, unknown>>,
@@ -152,26 +155,34 @@ test('the expiry gate fails the run before any test executes', async () => {
152155
});
153156

154157
test('only the runner-owned abort mark classifies a failure as a timeout', () => {
155-
assert.ok(isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED));
158+
assert.ok(isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED, TOKEN));
159+
// `task.meta` is test-writable, so a mark that is not the run's secret is worthless.
160+
for (const forged of [true, 'true', 'agentDeviceRunnerTimeout', '', undefined]) {
161+
assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], { [RUNNER_TIMEOUT_META]: forged }, TOKEN));
162+
}
163+
// No secret configured means nothing is eligible.
164+
assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED, undefined));
165+
assert.ok(!isRunnerTimeout([VITEST_TIMEOUT], RUNNER_ABORTED, ''));
156166
// Without the runner's mark, no message and no error shape can earn a retry.
157167
for (const impostor of [
158168
{ name: 'Error', message: VITEST_TIMEOUT.message },
159169
{ name: 'AssertionError', message: VITEST_TIMEOUT.message },
160170
{ name: 'Error', message: 'connect ETIMEDOUT 127.0.0.1:8080' },
161171
{ name: 'Error', message: 'Closing timeout while tearing down the daemon' },
162172
]) {
163-
assert.ok(!isRunnerTimeout([impostor], {}), `${impostor.name}: ${impostor.message}`);
164-
assert.ok(!isRunnerTimeout([impostor], undefined));
165-
assert.ok(!isRunnerTimeout([impostor], { [RUNNER_TIMEOUT_META]: 'yes' }));
173+
assert.ok(!isRunnerTimeout([impostor], {}, TOKEN), `${impostor.name}: ${impostor.message}`);
174+
assert.ok(!isRunnerTimeout([impostor], undefined, TOKEN));
175+
assert.ok(!isRunnerTimeout([impostor], { [RUNNER_TIMEOUT_META]: 'yes' }, TOKEN));
166176
}
167177
// An aborted test that also produced an assertion diff is a regression.
168178
assert.ok(
169179
!isRunnerTimeout(
170180
[VITEST_TIMEOUT, { name: 'AssertionError', message: 'x', expected: 1, actual: 2 }],
171181
RUNNER_ABORTED,
182+
TOKEN,
172183
),
173184
);
174-
assert.ok(!isRunnerTimeout([], RUNNER_ABORTED));
185+
assert.ok(!isRunnerTimeout([], RUNNER_ABORTED, TOKEN));
175186
});
176187

177188
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', () =
181192
fs.rmSync(report, { force: true });
182193
runCmdSync('pnpm', ['exec', 'vitest', 'run', '--config', FIXTURE_CONFIG], {
183194
cwd: repoRoot,
184-
env: { ...process.env, CONTENTION_RETRY_FAILURES: report },
195+
env: {
196+
...process.env,
197+
CONTENTION_RETRY_FAILURES: report,
198+
[RUNNER_TIMEOUT_TOKEN_ENV]: TOKEN,
199+
},
185200
allowFailure: true,
186201
});
187202
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', () =
193208
'timeout message thrown after blocking the event loop past the budget': false,
194209
'assertion failure after blocking the event loop past the budget': false,
195210
'plain assertion failure': false,
211+
// `task.meta` is test-writable, but the mark is the run's secret and the
212+
// setup file takes it out of the environment before any test module loads.
213+
'test writes the provenance marker itself': false,
214+
'test writes the marker after blocking the event loop past the budget': false,
215+
'test writes the marker from the environment it can still read': false,
196216
});
197217
// Only the genuine timeout may reach a rerun.
198218
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
206226
testCaseStub([{ name: 'AssertionError', message: VITEST_TIMEOUT.message }], {
207227
meta: {},
208228
}) as never,
229+
TOKEN,
209230
);
210231
assert.ok(impostor);
211232
assert.equal(impostor.timeout, false);
@@ -222,19 +243,20 @@ test('a test that timed out AND failed an assertion is not retry-eligible', () =
222243
VITEST_TIMEOUT,
223244
{ name: 'AssertionError', message: 'expected 1 to be 2', expected: 2, actual: 1 },
224245
]) as never,
246+
TOKEN,
225247
);
226248
assert.equal(mixed?.timeout, false);
227249
});
228250

229251
test('the lane reporter keeps the real error message a timeout is classified by', () => {
230-
const failed = failedTestCase(testCaseStub([VITEST_TIMEOUT]) as never);
252+
const failed = failedTestCase(testCaseStub([VITEST_TIMEOUT]) as never, TOKEN);
231253
assert.deepEqual(failed, {
232254
file: `${repoRoot}/${LISTED}`,
233255
testName: 'opens a session',
234256
message: TIMEOUT_MESSAGE,
235257
timeout: true,
236258
});
237-
assert.equal(failedTestCase(testCaseStub([], { state: 'passed' }) as never), null);
259+
assert.equal(failedTestCase(testCaseStub([], { state: 'passed' }) as never, TOKEN), null);
238260

239261
const target = path.join(repoRoot, '.tmp/contention-retry/reporter-gate.json');
240262
writeFailureReport({ failures: [failure()], blockers: [] }, target);

‎scripts/lib/contention-retry-reporter.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,22 @@ import path from 'node:path';
88
import type { Reporter, TestCase, TestModule } from 'vitest/node';
99
import { isRunnerTimeout, type RunBlocker, type TestFailure } from './contention-retry.ts';
1010
import { drainRunBlockers } from './run-blocker-bus.ts';
11+
import { RUNNER_TIMEOUT_TOKEN_ENV } from './runner-timeout-meta.ts';
1112

1213
export const FAILURE_FILE_ENV = 'CONTENTION_RETRY_FAILURES';
1314

1415
export type FailureReport = { failures: TestFailure[]; blockers: RunBlocker[] };
1516

1617
/** The failed-test view of a Vitest test case, or null when it did not fail. */
17-
export function failedTestCase(testCase: TestCase): TestFailure | null {
18+
export function failedTestCase(testCase: TestCase, token: string | undefined): TestFailure | null {
1819
const result = testCase.result();
1920
if (result.state !== 'failed') return null;
2021
const errors = result.errors ?? [];
2122
return {
2223
file: (testCase.module as TestModule).moduleId,
2324
testName: testCase.fullName,
2425
message: errors.map((error) => `${error.name}: ${error.message}`).join('\n'),
25-
timeout: isRunnerTimeout(errors, testCase.meta()),
26+
timeout: isRunnerTimeout(errors, testCase.meta(), token),
2627
};
2728
}
2829

@@ -59,7 +60,7 @@ export default function contentionRetryReporter(): Reporter {
5960
const failures: TestFailure[] = [];
6061
return {
6162
onTestCaseResult(testCase: TestCase): void {
62-
const failed = failedTestCase(testCase);
63+
const failed = failedTestCase(testCase, process.env[RUNNER_TIMEOUT_TOKEN_ENV]);
6364
if (failed) failures.push(failed);
6465
},
6566
onTestRunEnd(

‎scripts/lib/contention-retry-run.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { runCmdStreaming, runCmdSync } from '../../src/utils/exec.ts';
1414
import { parseScriptArgs } from './cli-args.ts';
1515
import { runWithContentionRetry, type TestRun } from './contention-retry-lane.ts';
1616
import { FAILURE_FILE_ENV } from './contention-retry-reporter.ts';
17+
import { RUNNER_TIMEOUT_TOKEN_ENV } from './runner-timeout-meta.ts';
1718
import { processBlockers } from './contention-retry-blockers.ts';
1819
import { parseFailureReport, type RunBlocker, type TestFailure } from './contention-retry.ts';
1920

@@ -42,6 +43,9 @@ const reportPath = path.resolve(repoRoot, args.report);
4243
const envelopePath = path.resolve(repoRoot, args.envelope);
4344
const summaryPath = args.summary ?? process.env.GITHUB_STEP_SUMMARY;
4445

46+
/** The capability the runner needs to mark a timeout; test modules never see it. */
47+
const TIMEOUT_TOKEN = crypto.randomUUID();
48+
4549
async function runVitest(extra: string[], report: string): Promise<TestRun> {
4650
fs.mkdirSync(path.dirname(report), { recursive: true });
4751
fs.rmSync(report, { force: true });
@@ -52,7 +56,7 @@ async function runVitest(extra: string[], report: string): Promise<TestRun> {
5256
};
5357
const result = await runCmdStreaming('pnpm', ['exec', 'vitest', 'run', ...extra], {
5458
cwd: repoRoot,
55-
env: { ...process.env, [FAILURE_FILE_ENV]: report },
59+
env: { ...process.env, [FAILURE_FILE_ENV]: report, [RUNNER_TIMEOUT_TOKEN_ENV]: TIMEOUT_TOKEN },
5660
allowFailure: true,
5761
onStdoutChunk: (chunk) => capture(chunk, (text) => process.stdout.write(text)),
5862
onStderrChunk: (chunk) => capture(chunk, (text) => process.stderr.write(text)),

‎scripts/lib/contention-retry.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,8 +195,12 @@ export type ReportedError = {
195195
};
196196

197197
/** A test the runner aborted at its timeout — the only failure this lane retries. */
198-
export function isRunnerTimeout(errors: readonly ReportedError[], meta: unknown): boolean {
199-
return runnerTimedOut(meta) && errors.length > 0 && errors.every(hasNoAssertionDiff);
198+
export function isRunnerTimeout(
199+
errors: readonly ReportedError[],
200+
meta: unknown,
201+
token: string | undefined,
202+
): boolean {
203+
return runnerTimedOut(meta, token) && errors.length > 0 && errors.every(hasNoAssertionDiff);
200204
}
201205

202206
/** A timeout that also failed an assertion is a regression, not contention. */

‎scripts/lib/runner-timeout-meta.ts‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,24 @@
11
// The provenance mark the runner-timeout setup file writes on a test, and the
22
// contention retry lane (#1419) reads back through `TestCase.meta()`.
3+
//
4+
// `task.meta` is writable by test code, so the mark is not a boolean but a
5+
// per-run secret the lane generates. Setup files are imported before any test
6+
// module, and the setup file takes the secret out of the environment as it
7+
// loads, so no test body can read the value it would have to forge.
38

49
export const RUNNER_TIMEOUT_META = 'agentDeviceRunnerTimeout';
510

11+
export const RUNNER_TIMEOUT_TOKEN_ENV = 'CONTENTION_RETRY_TIMEOUT_TOKEN';
12+
13+
/** Reads the run's secret and removes it from `env`, leaving nothing for a test to find. */
14+
export function takeRunnerTimeoutToken(env: NodeJS.ProcessEnv): string | undefined {
15+
const token = env[RUNNER_TIMEOUT_TOKEN_ENV];
16+
delete env[RUNNER_TIMEOUT_TOKEN_ENV];
17+
return token;
18+
}
19+
620
/** True when the runner itself aborted the test with its own timeout error. */
7-
export function runnerTimedOut(meta: unknown): boolean {
8-
return (meta as Record<string, unknown> | undefined)?.[RUNNER_TIMEOUT_META] === true;
21+
export function runnerTimedOut(meta: unknown, token: string | undefined): boolean {
22+
if (!token) return false;
23+
return (meta as Record<string, unknown> | undefined)?.[RUNNER_TIMEOUT_META] === token;
924
}

‎scripts/vitest-runner-timeout-setup.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,27 @@
55
// controller, and it aborts with the timeout error it raised. A test body can
66
// throw whatever it likes, including the runner's exact timeout text, without
77
// ever aborting the signal.
8+
//
9+
// The mark is the run's secret, taken out of the environment here — before any
10+
// test module is imported — so a test writing `task.meta` has nothing to write.
811

912
import { beforeEach, onTestFailed } from 'vitest';
10-
import { RUNNER_TIMEOUT_META } from './lib/runner-timeout-meta.ts';
13+
import { RUNNER_TIMEOUT_META, takeRunnerTimeoutToken } from './lib/runner-timeout-meta.ts';
14+
15+
const TOKEN = takeRunnerTimeoutToken(process.env);
1116

1217
/** Distinguishes the timeout abort from a run cancellation, on the runner's own reason. */
1318
const RUNNER_TIMEOUT_REASON = /^(Test|Hook) timed out in \d+ms\./;
1419

20+
function abortedByRunnerTimeout(signal: AbortSignal): boolean {
21+
if (!signal.aborted || !(signal.reason instanceof Error)) return false;
22+
return RUNNER_TIMEOUT_REASON.test(signal.reason.message);
23+
}
24+
1525
beforeEach(({ signal, task }) => {
1626
onTestFailed(() => {
17-
if (
18-
signal.aborted &&
19-
signal.reason instanceof Error &&
20-
RUNNER_TIMEOUT_REASON.test(signal.reason.message)
21-
) {
22-
task.meta[RUNNER_TIMEOUT_META] = true;
27+
if (TOKEN && abortedByRunnerTimeout(signal)) {
28+
(task.meta as Record<string, unknown>)[RUNNER_TIMEOUT_META] = TOKEN;
2329
}
2430
});
2531
});

‎test/contention-retry-fixtures/timeout-provenance.fixture.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,19 @@
33
// and never by the normal suites. Each name is asserted there.
44

55
import { expect, test } from 'vitest';
6+
import {
7+
RUNNER_TIMEOUT_META,
8+
RUNNER_TIMEOUT_TOKEN_ENV,
9+
} from '../../scripts/lib/runner-timeout-meta.ts';
610

711
const RUNNER_TIMEOUT_MESSAGE =
812
'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".';
913

14+
/** What a test body can do to `task.meta`, which the policy must not trust. */
15+
function forgeMark(task: { meta: object }, value: unknown): void {
16+
(task.meta as Record<string, unknown>)[RUNNER_TIMEOUT_META] = value;
17+
}
18+
1019
function blockPastBudget(): void {
1120
const until = Date.now() + 200;
1221
while (Date.now() < until) {
@@ -35,3 +44,19 @@ test('assertion failure after blocking the event loop past the budget', () => {
3544
test('plain assertion failure', () => {
3645
expect('DEVICE_IN_USE').toBe('OK');
3746
});
47+
48+
test('test writes the provenance marker itself', ({ task }) => {
49+
forgeMark(task, true);
50+
throw new Error('DEVICE_IN_USE');
51+
});
52+
53+
test('test writes the marker after blocking the event loop past the budget', ({ task }) => {
54+
forgeMark(task, true);
55+
blockPastBudget();
56+
throw new Error(RUNNER_TIMEOUT_MESSAGE);
57+
}, 50);
58+
59+
test('test writes the marker from the environment it can still read', ({ task }) => {
60+
forgeMark(task, process.env[RUNNER_TIMEOUT_TOKEN_ENV] ?? 'guess');
61+
throw new Error(RUNNER_TIMEOUT_MESSAGE);
62+
});

0 commit comments

Comments
 (0)