Skip to content

Commit 02a6d4b

Browse files
committed
fix: don't flag a concurrent vitest run's tmpdir as a leak
check-tmpdir-leaks.ts reported every agent-device-test-run-* directory as a leak, but a concurrent vitest run in another worktree legitimately keeps its own directory present until its own teardown finishes. On a machine that regularly runs several worktrees at once, that made check:unit fail on unrelated in-progress work. Embed the owning process's pid in the directory name (still random- suffixed via mkdtempSync, so same-pid reuse across separate runs can't collide) and have the leak check skip any directory whose pid is still alive (process.kill(pid, 0)) — only directories whose owning process already exited without running its globalTeardown are real leaks. Split the pure logic into check-tmpdir-leaks-model.ts (findLeakedRunDirectories, with an injectable liveness check for testing) so it has a real regression suite, including the concurrent-run case, instead of only being exercised by hand.
1 parent 3eb1d71 commit 02a6d4b

5 files changed

Lines changed: 127 additions & 17 deletions

File tree

package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,12 +136,13 @@
136136
"check:command-docs": "vitest run --project unit-core src/__tests__/command-doc-coverage.test.ts",
137137
"check:replay-compat": "node --experimental-strip-types scripts/check-replay-compat-provenance.ts",
138138
"check:tmpdir-leaks": "node --experimental-strip-types scripts/check-tmpdir-leaks.ts",
139+
"check:tmpdir-leaks:test": "node --experimental-strip-types --test scripts/check-tmpdir-leaks-model.test.ts",
139140
"check:freerange": "fr",
140141
"check:quick": "pnpm lint && pnpm typecheck",
141142
"sync:mcp-metadata": "node scripts/sync-mcp-metadata.mjs",
142143
"check:mcp-metadata": "node scripts/sync-mcp-metadata.mjs --check",
143144
"version": "pnpm sync:mcp-metadata && git add server.json",
144-
"check:tooling": "pnpm format:check && 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 && pnpm check:package",
145+
"check:tooling": "pnpm format:check && pnpm lint && pnpm typecheck && pnpm check:layering && pnpm depgraph:test && pnpm check:production-exports && pnpm check:tmpdir-leaks:test && pnpm check:mcp-metadata && pnpm build && pnpm check:bundle-owner-files && pnpm check:package",
145146
"check:unit": "pnpm check:contention-retry && pnpm test:unit && pnpm check:tmpdir-leaks && pnpm test:smoke",
146147
"check": "pnpm check:tooling && pnpm check:fallow && pnpm check:unit",
147148
"prepack": "pnpm check:mcp-metadata && pnpm package:npm",
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
import assert from 'node:assert/strict';
2+
import fs from 'node:fs';
3+
import os from 'node:os';
4+
import path from 'node:path';
5+
import { test } from 'node:test';
6+
import { findLeakedRunDirectories } from './check-tmpdir-leaks-model.ts';
7+
8+
function withScratchRoot(fn: (root: string) => void): void {
9+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'check-tmpdir-leaks-model-test-'));
10+
try {
11+
fn(root);
12+
} finally {
13+
fs.rmSync(root, { recursive: true, force: true });
14+
}
15+
}
16+
17+
test('a directory owned by a still-running process is not reported as a leak', () => {
18+
withScratchRoot((root) => {
19+
fs.mkdirSync(path.join(root, 'agent-device-test-run-4242-abcdef'));
20+
const leaks = findLeakedRunDirectories(root, (pid) => pid === 4242);
21+
assert.deepEqual(leaks, []);
22+
});
23+
});
24+
25+
test('a directory whose owning process has exited is reported as a leak', () => {
26+
withScratchRoot((root) => {
27+
fs.mkdirSync(path.join(root, 'agent-device-test-run-4242-abcdef'));
28+
const leaks = findLeakedRunDirectories(root, () => false);
29+
assert.deepEqual(leaks, ['agent-device-test-run-4242-abcdef']);
30+
});
31+
});
32+
33+
test('a concurrent run from another worktree does not fail the check for this one', () => {
34+
withScratchRoot((root) => {
35+
// Simulates two worktrees running vitest at once: pid 1111 (this
36+
// invocation, still alive) and pid 2222 (a different worktree's
37+
// in-progress run, also alive).
38+
fs.mkdirSync(path.join(root, 'agent-device-test-run-1111-aaaaaa'));
39+
fs.mkdirSync(path.join(root, 'agent-device-test-run-2222-bbbbbb'));
40+
const alivePids = new Set([1111, 2222]);
41+
const leaks = findLeakedRunDirectories(root, (pid) => alivePids.has(pid));
42+
assert.deepEqual(leaks, []);
43+
});
44+
});
45+
46+
test('a mix of active and abandoned directories reports only the abandoned one', () => {
47+
withScratchRoot((root) => {
48+
fs.mkdirSync(path.join(root, 'agent-device-test-run-1111-aaaaaa')); // alive
49+
fs.mkdirSync(path.join(root, 'agent-device-test-run-3333-cccccc')); // exited
50+
const leaks = findLeakedRunDirectories(root, (pid) => pid === 1111);
51+
assert.deepEqual(leaks, ['agent-device-test-run-3333-cccccc']);
52+
});
53+
});
54+
55+
test('a directory with no parseable pid is conservatively reported as a leak', () => {
56+
withScratchRoot((root) => {
57+
fs.mkdirSync(path.join(root, 'agent-device-test-run-not-a-pid'));
58+
const leaks = findLeakedRunDirectories(root, () => true);
59+
assert.deepEqual(leaks, ['agent-device-test-run-not-a-pid']);
60+
});
61+
});
62+
63+
test('non-matching directories and files are ignored', () => {
64+
withScratchRoot((root) => {
65+
fs.mkdirSync(path.join(root, 'unrelated-directory'));
66+
fs.writeFileSync(path.join(root, 'agent-device-test-run-4242-loose-file'), '');
67+
const leaks = findLeakedRunDirectories(root, () => false);
68+
assert.deepEqual(leaks, []);
69+
});
70+
});
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import fs from 'node:fs';
2+
import { TEST_RUN_TMP_PREFIX } from './vitest-tmpdir-global-setup.ts';
3+
4+
const PID_SUFFIX = new RegExp(`^${TEST_RUN_TMP_PREFIX}(\\d+)-`);
5+
6+
export function isProcessAlive(pid: number): boolean {
7+
try {
8+
process.kill(pid, 0);
9+
return true;
10+
} catch (error) {
11+
// EPERM means the pid exists but we lack permission to signal it — still alive.
12+
return (error as NodeJS.ErrnoException).code === 'EPERM';
13+
}
14+
}
15+
16+
/**
17+
* Directories owned by a still-running process are a concurrent vitest run
18+
* (e.g. another worktree), not a leak — only report the ones whose owning
19+
* process has already exited without cleaning up after itself.
20+
*/
21+
export function findLeakedRunDirectories(
22+
root: string,
23+
isAlive: (pid: number) => boolean = isProcessAlive,
24+
): string[] {
25+
return fs
26+
.readdirSync(root, { withFileTypes: true })
27+
.filter((entry) => entry.isDirectory() && entry.name.startsWith(TEST_RUN_TMP_PREFIX))
28+
.filter((entry) => {
29+
const match = PID_SUFFIX.exec(entry.name);
30+
// No parseable pid means it didn't come from setup() as written — treat it as a leak.
31+
if (!match) return true;
32+
return !isAlive(Number(match[1]));
33+
})
34+
.map((entry) => entry.name);
35+
}

scripts/check-tmpdir-leaks.ts

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,27 @@
1-
// Fails if any agent-device-test-run-* directory remains under
2-
// TEST_RUN_TMP_ROOT after the unit suite finishes. scripts/vitest-tmpdir-global-setup.ts
3-
// creates exactly one such directory per `vitest run` invocation and removes
4-
// it in its globalTeardown; a leftover one means that invocation's process
5-
// was killed (crash, OOM, timeout) before teardown could run. Run this after
6-
// `pnpm test:unit`, not concurrently with it.
1+
// Fails if any agent-device-test-run-* directory under TEST_RUN_TMP_ROOT is
2+
// abandoned — its owning process has exited without running its
3+
// globalTeardown (crash, OOM, timeout kill). Directories owned by a still-
4+
// running process are left alone: a concurrent `vitest run` in another
5+
// worktree keeps its own directory present until its own teardown, which is
6+
// not a leak. See check-tmpdir-leaks-model.ts for the liveness check.
77
//
88
// Only covers vitest runs. node --test lanes (test:smoke,
99
// test:integration:node, ...) still use the real os.tmpdir() unredirected.
1010

11-
import fs from 'node:fs';
1211
import path from 'node:path';
13-
import { TEST_RUN_TMP_PREFIX, TEST_RUN_TMP_ROOT } from './vitest-tmpdir-global-setup.ts';
12+
import { findLeakedRunDirectories } from './check-tmpdir-leaks-model.ts';
13+
import { TEST_RUN_TMP_ROOT } from './vitest-tmpdir-global-setup.ts';
1414

15-
const leaks = fs
16-
.readdirSync(TEST_RUN_TMP_ROOT, { withFileTypes: true })
17-
.filter((entry) => entry.isDirectory() && entry.name.startsWith(TEST_RUN_TMP_PREFIX));
15+
const leaks = findLeakedRunDirectories(TEST_RUN_TMP_ROOT);
1816

1917
if (leaks.length > 0) {
20-
const details = leaks.map((entry) => `- ${path.join(TEST_RUN_TMP_ROOT, entry.name)}`).join('\n');
18+
const details = leaks.map((name) => `- ${path.join(TEST_RUN_TMP_ROOT, name)}`).join('\n');
2119
throw new Error(
22-
`Found ${leaks.length} leftover ${TEST_RUN_TMP_PREFIX}* director${leaks.length === 1 ? 'y' : 'ies'} in ${TEST_RUN_TMP_ROOT}:\n${details}\n` +
23-
'A vitest run was killed before its globalTeardown could run; investigate the run that produced them.',
20+
`Found ${leaks.length} abandoned agent-device-test-run-* director${leaks.length === 1 ? 'y' : 'ies'} in ${TEST_RUN_TMP_ROOT}:\n${details}\n` +
21+
'Their owning process has already exited, so their globalTeardown never ran (crash, OOM, timeout kill); investigate the run that produced them.',
2422
);
2523
}
2624

2725
process.stdout.write(
28-
`No leaked ${TEST_RUN_TMP_PREFIX}* directories found in ${TEST_RUN_TMP_ROOT}.\n`,
26+
`No abandoned agent-device-test-run-* directories found in ${TEST_RUN_TMP_ROOT}.\n`,
2927
);

scripts/vitest-tmpdir-global-setup.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,13 @@ export const TEST_RUN_TMP_PREFIX = 'agent-device-test-run-';
2626
let testRunTmpDir: string;
2727

2828
export function setup(): void {
29-
testRunTmpDir = fs.mkdtempSync(path.join(TEST_RUN_TMP_ROOT, TEST_RUN_TMP_PREFIX));
29+
// The pid is embedded so check-tmpdir-leaks.ts can tell a directory that's
30+
// still in active use (its vitest process is alive — a concurrent run in
31+
// another worktree, say) apart from one actually abandoned by a killed
32+
// process; the trailing mkdtemp suffix still guards against same-pid reuse.
33+
testRunTmpDir = fs.mkdtempSync(
34+
path.join(TEST_RUN_TMP_ROOT, `${TEST_RUN_TMP_PREFIX}${process.pid}-`),
35+
);
3036
process.env.TMPDIR = testRunTmpDir;
3137
}
3238

0 commit comments

Comments
 (0)