test: prune abandoned test-run tmp directories at run setup (#1834)

* test: prune abandoned test-run tmp directories at run setup

A run killed before its teardown (tool-timeout SIGKILL, OOM, cancelled job)
left /tmp/agent-device-test-run-<pid>-* behind, and check:tmpdir-leaks — which
runs after test:unit in check:unit — flagged every dead-pid directory it
found. It could not tell this run's leak from a historical one, so one killed
run made every later, otherwise-green gate on the host fail.

Both TMPDIR redirection entry points (the Vitest global setup and the
node --test wrapper) now prune dead-pid run directories before creating their
own, printing one [tmpdir] line when they did; the post-run check keeps its
semantics and can now only ever name the run that just finished. Live owners
(a concurrent run in another worktree) are never touched.

The root/prefix constants move into check-tmpdir-leaks-model.ts, next to the
liveness classification, so the setup can import the prune without a cycle.

* test(tmpdir): a run directory is live while any process still holds it as TMPDIR, not only while its owner runs

Review (P1): owner-pid liveness alone would prune a directory out from under
the orphaned children of a SIGKILLed run — the node --test chain, Vitest forks,
or a daemon a test spawned all keep running with that TMPDIR. The liveness
model now reads every process's TMPDIR (ps -E on macOS, /proc/<pid>/environ
on Linux) and treats a run directory as live while its owner pid is alive OR
any process's TMPDIR points into it; both the prune and the post-run leak
check use it. Regression: a wrapped probe spawns a detached long-lived child,
only the wrapper is SIGKILLed, the next prune preserves the directory; after
every consumer exits, the next prune removes it. Planted red with owner-only
liveness: the orphaned directory is pruned.

Author: thymikee

docs/agents/testing.md

@@ -78,6 +78,18 @@ cleanup for a directory they created — that's the global teardown's job, and p
 already existed for other reasons should stay (it's the fallback global sweep that's new, not a
 replacement for tests being tidy).
 
+A run killed before its teardown (a tool timeout's SIGKILL, OOM, a cancelled job) leaves its
+`/tmp/agent-device-test-run-<pid>-*` directory behind. The next run on the host prunes every such
+directory that is genuinely abandoned (`pruneAbandonedRunDirectories`, called by both the Vitest
+global setup and the `node --test` wrapper) and prints one `[tmpdir] pruned …` line, so
+`check:tmpdir-leaks` after `test:unit` can only ever name the run that just finished. Abandoned
+means nobody owns it **and nobody uses it**: the owner pid in the name is dead and no live process
+has a `TMPDIR` inside it (read from `ps -E` on macOS, `/proc/<pid>/environ` on Linux). That second
+half matters because a SIGKILL of the wrapper or Vitest main process leaves its `node --test` chain,
+forked workers, and any daemon a test spawned running with that `TMPDIR` — they keep the directory
+until the last of them exits. A concurrent run in another worktree is live by both tests. If the
+check fails, the leak is this run's: a teardown that did not execute, not history.
+
 Keep tests behavioral. Do not assert shapes or cases TypeScript already proves.
 
 A test added as a regression pin must be shown to fail without the change it pins — vacuity is the

scripts/check-tmpdir-leaks-model.test.ts

@@ -3,7 +3,13 @@ import fs from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
 import { test } from 'node:test';
-import { findLeakedRunDirectories } from './check-tmpdir-leaks-model.ts';
+import {
+  findLeakedRunDirectories,
+  pruneAbandonedRunDirectories,
+  runDirectoryNameOf,
+} from './check-tmpdir-leaks-model.ts';
+
+const NONE: ReadonlySet<string> = new Set();
 
 function withScratchRoot(fn: (root: string) => void): void {
   const root = fs.mkdtempSync(path.join(os.tmpdir(), 'check-tmpdir-leaks-model-test-'));
@@ -17,15 +23,18 @@ function withScratchRoot(fn: (root: string) => void): void {
 test('a directory owned by a still-running process is not reported as a leak', () => {
   withScratchRoot((root) => {
     fs.mkdirSync(path.join(root, 'agent-device-test-run-4242-abcdef'));
-    const leaks = findLeakedRunDirectories(root, (pid) => pid === 4242);
+    const leaks = findLeakedRunDirectories(root, {
+      isAlive: (pid) => pid === 4242,
+      consumers: NONE,
+    });
     assert.deepEqual(leaks, []);
   });
 });
 
 test('a directory whose owning process has exited is reported as a leak', () => {
   withScratchRoot((root) => {
     fs.mkdirSync(path.join(root, 'agent-device-test-run-4242-abcdef'));
-    const leaks = findLeakedRunDirectories(root, () => false);
+    const leaks = findLeakedRunDirectories(root, { isAlive: () => false, consumers: NONE });
     assert.deepEqual(leaks, ['agent-device-test-run-4242-abcdef']);
   });
 });
@@ -38,7 +47,10 @@ test('a concurrent run from another worktree does not fail the check for this on
     fs.mkdirSync(path.join(root, 'agent-device-test-run-1111-aaaaaa'));
     fs.mkdirSync(path.join(root, 'agent-device-test-run-2222-bbbbbb'));
     const alivePids = new Set([1111, 2222]);
-    const leaks = findLeakedRunDirectories(root, (pid) => alivePids.has(pid));
+    const leaks = findLeakedRunDirectories(root, {
+      isAlive: (pid) => alivePids.has(pid),
+      consumers: NONE,
+    });
     assert.deepEqual(leaks, []);
   });
 });
@@ -47,15 +59,18 @@ test('a mix of active and abandoned directories reports only the abandoned one',
   withScratchRoot((root) => {
     fs.mkdirSync(path.join(root, 'agent-device-test-run-1111-aaaaaa')); // alive
     fs.mkdirSync(path.join(root, 'agent-device-test-run-3333-cccccc')); // exited
-    const leaks = findLeakedRunDirectories(root, (pid) => pid === 1111);
+    const leaks = findLeakedRunDirectories(root, {
+      isAlive: (pid) => pid === 1111,
+      consumers: NONE,
+    });
     assert.deepEqual(leaks, ['agent-device-test-run-3333-cccccc']);
   });
 });
 
 test('a directory with no parseable pid is conservatively reported as a leak', () => {
   withScratchRoot((root) => {
     fs.mkdirSync(path.join(root, 'agent-device-test-run-not-a-pid'));
-    const leaks = findLeakedRunDirectories(root, () => true);
+    const leaks = findLeakedRunDirectories(root, { isAlive: () => true, consumers: NONE });
     assert.deepEqual(leaks, ['agent-device-test-run-not-a-pid']);
   });
 });
@@ -64,7 +79,83 @@ test('non-matching directories and files are ignored', () => {
   withScratchRoot((root) => {
     fs.mkdirSync(path.join(root, 'unrelated-directory'));
     fs.writeFileSync(path.join(root, 'agent-device-test-run-4242-loose-file'), '');
-    const leaks = findLeakedRunDirectories(root, () => false);
+    const leaks = findLeakedRunDirectories(root, { isAlive: () => false, consumers: NONE });
     assert.deepEqual(leaks, []);
   });
 });
+
+test('pruning removes only abandoned directories and returns their names', () => {
+  withScratchRoot((root) => {
+    fs.mkdirSync(path.join(root, 'agent-device-test-run-1111-aaaaaa')); // alive
+    fs.mkdirSync(path.join(root, 'agent-device-test-run-3333-cccccc', 'nested'), {
+      recursive: true,
+    }); // exited, non-empty
+    fs.mkdirSync(path.join(root, 'agent-device-test-run-not-a-pid')); // no owner
+    fs.mkdirSync(path.join(root, 'unrelated-directory'));
+    const pruned = pruneAbandonedRunDirectories(root, {
+      isAlive: (pid) => pid === 1111,
+      consumers: NONE,
+    });
+    assert.deepEqual(pruned.sort(), [
+      'agent-device-test-run-3333-cccccc',
+      'agent-device-test-run-not-a-pid',
+    ]);
+    assert.deepEqual(fs.readdirSync(root).sort(), [
+      'agent-device-test-run-1111-aaaaaa',
+      'unrelated-directory',
+    ]);
+    // Once pruned, the post-run check has nothing historical left to report.
+    assert.deepEqual(
+      findLeakedRunDirectories(root, { isAlive: (pid) => pid === 1111, consumers: NONE }),
+      [],
+    );
+  });
+});
+
+test('pruning nothing is a no-op that reports nothing', () => {
+  withScratchRoot((root) => {
+    fs.mkdirSync(path.join(root, 'agent-device-test-run-1111-aaaaaa'));
+    assert.deepEqual(
+      pruneAbandonedRunDirectories(root, { isAlive: () => true, consumers: NONE }),
+      [],
+    );
+    assert.deepEqual(fs.readdirSync(root), ['agent-device-test-run-1111-aaaaaa']);
+  });
+});
+
+test('a directory whose owner is dead but which some live process still holds as TMPDIR is not a leak', () => {
+  withScratchRoot((root) => {
+    // The motivating case: the wrapper/vitest owner was SIGKILLed, its node --test chain or
+    // forked workers (or a daemon a test spawned) are still running with TMPDIR inside the
+    // directory. Nobody may prune it until the last of them exits.
+    fs.mkdirSync(path.join(root, 'agent-device-test-run-4242-orphaned'));
+    fs.mkdirSync(path.join(root, 'agent-device-test-run-5555-finished'));
+    const consumers = new Set(['agent-device-test-run-4242-orphaned']);
+    assert.deepEqual(findLeakedRunDirectories(root, { isAlive: () => false, consumers }), [
+      'agent-device-test-run-5555-finished',
+    ]);
+    assert.deepEqual(pruneAbandonedRunDirectories(root, { isAlive: () => false, consumers }), [
+      'agent-device-test-run-5555-finished',
+    ]);
+    assert.deepEqual(fs.readdirSync(root), ['agent-device-test-run-4242-orphaned']);
+    // Once the consumers are gone it is an ordinary abandoned directory.
+    assert.deepEqual(
+      pruneAbandonedRunDirectories(root, { isAlive: () => false, consumers: NONE }),
+      ['agent-device-test-run-4242-orphaned'],
+    );
+    assert.deepEqual(fs.readdirSync(root), []);
+  });
+});
+
+test('a consumer is identified by the run directory its TMPDIR sits inside, at any depth', () => {
+  assert.equal(
+    runDirectoryNameOf('/tmp/agent-device-test-run-123-abc'),
+    'agent-device-test-run-123-abc',
+  );
+  assert.equal(
+    runDirectoryNameOf('/tmp/agent-device-test-run-123-abc/nested/deeper'),
+    'agent-device-test-run-123-abc',
+  );
+  assert.equal(runDirectoryNameOf('/tmp/other-123-abc'), undefined);
+  assert.equal(runDirectoryNameOf('/var/folders/x/T/agent-device-test-run-1-a'), undefined);
+});

scripts/check-tmpdir-leaks-model.ts

@@ -1,5 +1,23 @@
 import fs from 'node:fs';
-import { TEST_RUN_TMP_PREFIX } from './vitest-tmpdir-global-setup.ts';
+import path from 'node:path';
+import { runCmdSync } from '../src/utils/exec.ts';
+
+// Every test run (Vitest via scripts/vitest-tmpdir-global-setup.ts, node --test
+// via scripts/node-test-tmpdir.ts) redirects TMPDIR into one disposable,
+// pid-tagged directory under this root and removes it at teardown.
+//
+// Rooted at /tmp rather than nested inside the current os.tmpdir(): macOS's
+// per-user TMPDIR (/var/folders/.../T/) is already close to the 104-byte
+// sun_path limit AF_UNIX sockets need, and tests that bind real sockets
+// (e.g. runner-usbmux.test.ts) started hitting EINVAL once nested one level
+// deeper. /tmp is short enough to leave headroom for those.
+//
+// Both redirection mechanisms and check-tmpdir-leaks.ts import these two from
+// here rather than recomputing them, so they can't drift onto different
+// directories (os.tmpdir() != /tmp on macOS, where TMPDIR is a deep per-user
+// path).
+export const TEST_RUN_TMP_ROOT = '/tmp';
+export const TEST_RUN_TMP_PREFIX = 'agent-device-test-run-';
 
 const PID_SUFFIX = new RegExp(`^${TEST_RUN_TMP_PREFIX}(\\d+)-`);
 
@@ -14,22 +32,129 @@ function isProcessAlive(pid: number): boolean {
 }
 
 /**
- * Directories owned by a still-running process are a concurrent vitest run
- * (e.g. another worktree), not a leak — only report the ones whose owning
- * process has already exited without cleaning up after itself.
+ * The run directories some live process is still using: every process whose TMPDIR points
+ * into one. That is the ownership signal every consumer actually carries — the run's own
+ * setup exports it and every child (Vitest forks, the node --test chain, daemons a test
+ * spawned) inherits it — so a run whose owner was SIGKILLed while its children kept running
+ * is still "in use" until the last of them exits. Read from `ps -E` on macOS (environment
+ * is shown for the caller's own processes) and /proc/<pid>/environ on Linux; on either, a
+ * process this user cannot inspect contributes nothing, and its directory is then judged by
+ * its owner pid alone.
+ */
+export function liveRunDirectoryConsumers(): ReadonlySet<string> {
+  const consumers = new Set<string>();
+  for (const value of readAllProcessTmpdirs()) {
+    const name = runDirectoryNameOf(value);
+    if (name !== undefined) consumers.add(name);
+  }
+  return consumers;
+}
+
+/** `/tmp/agent-device-test-run-123-abc/nested` → `agent-device-test-run-123-abc`; else undefined. */
+export function runDirectoryNameOf(tmpdir: string): string | undefined {
+  const rootPrefix = `${TEST_RUN_TMP_ROOT}/`;
+  if (!tmpdir.startsWith(rootPrefix)) return undefined;
+  const name = tmpdir.slice(rootPrefix.length).split('/')[0] ?? '';
+  return name.startsWith(TEST_RUN_TMP_PREFIX) ? name : undefined;
+}
+
+function readAllProcessTmpdirs(): string[] {
+  return process.platform === 'linux' ? readProcTmpdirs() : readPsTmpdirs();
+}
+
+function readProcTmpdirs(): string[] {
+  return fs
+    .readdirSync('/proc')
+    .filter((entry) => /^\d+$/.test(entry))
+    .flatMap((pid) => tmpdirsOfEnviron(readEnvironOrEmpty(pid)));
+}
+
+/** Another user's process, or one that exited mid-scan, contributes nothing. */
+function readEnvironOrEmpty(pid: string): string {
+  try {
+    return fs.readFileSync(`/proc/${pid}/environ`, 'latin1');
+  } catch {
+    return '';
+  }
+}
+
+function tmpdirsOfEnviron(environ: string): string[] {
+  return environ
+    .split('\0')
+    .filter((pair) => pair.startsWith('TMPDIR='))
+    .map((pair) => pair.slice('TMPDIR='.length));
+}
+
+// macOS (and other BSDs): -E appends the environment to each command line. Every process's
+// environment is a few MB on a busy host — well past spawnSync's 1 MB default.
+function readPsTmpdirs(): string[] {
+  const listing = runCmdSync('ps', ['-axEww', '-o', 'command='], {
+    allowFailure: true,
+    maxBuffer: 64 * 1024 * 1024,
+  });
+  if (listing.exitCode !== 0) return [];
+  return [...listing.stdout.matchAll(/(?:^|\s)TMPDIR=(\S+)/g)].map((match) => match[1] as string);
+}
+
+export type RunDirectoryLiveness = Readonly<{
+  isAlive?: (pid: number) => boolean;
+  consumers?: ReadonlySet<string>;
+}>;
+
+/**
+ * A run directory is live while its owning process (the pid in its name) runs, OR while any
+ * process still holds it as TMPDIR — a concurrent run in another worktree, or the orphaned
+ * children of a killed run. Only the rest are leaks: nobody owns them and nobody uses them.
  */
 export function findLeakedRunDirectories(
   root: string,
-  isAlive: (pid: number) => boolean = isProcessAlive,
+  liveness: RunDirectoryLiveness = {},
 ): string[] {
+  const isAlive = liveness.isAlive ?? isProcessAlive;
+  const consumers = liveness.consumers ?? liveRunDirectoryConsumers();
   return fs
     .readdirSync(root, { withFileTypes: true })
     .filter((entry) => entry.isDirectory() && entry.name.startsWith(TEST_RUN_TMP_PREFIX))
     .filter((entry) => {
+      if (consumers.has(entry.name)) return false;
       const match = PID_SUFFIX.exec(entry.name);
       // No parseable pid means it didn't come from setup() as written — treat it as a leak.
       if (!match) return true;
       return !isAlive(Number(match[1]));
     })
     .map((entry) => entry.name);
 }
+
+/**
+ * Removes the run directories an earlier, already-exited run left behind and
+ * returns their names. A run's setup calls this before creating its own
+ * directory, so the post-run leak check (check-tmpdir-leaks.ts) can only ever
+ * report the run that just finished: a directory abandoned by an earlier run
+ * that was killed before its teardown (SIGKILL on a tool timeout, OOM, a
+ * cancelled CI job) is by construction the same thing that teardown would have
+ * removed, and leaving it in place made every later, otherwise-green gate on
+ * the host fail for a run it never ran. Live owners are never touched, so a
+ * concurrent run in another worktree keeps its directory.
+ */
+export function pruneAbandonedRunDirectories(
+  root: string,
+  liveness: RunDirectoryLiveness = {},
+): string[] {
+  const abandoned = findLeakedRunDirectories(root, liveness);
+  for (const name of abandoned) {
+    fs.rmSync(path.join(root, name), { recursive: true, force: true });
+  }
+  return abandoned;
+}
+
+/**
+ * One stderr line, only when something was pruned: an earlier run on this
+ * host died before its teardown, which the operator should know (a tool
+ * timeout killed it, say) without it being a failure of this run.
+ */
+export function reportPrunedRunDirectories(pruned: readonly string[]): void {
+  if (pruned.length === 0) return;
+  process.stderr.write(
+    `[tmpdir] pruned ${pruned.length} abandoned ${TEST_RUN_TMP_PREFIX}* director${pruned.length === 1 ? 'y' : 'ies'} left by an earlier killed run\n`,
+  );
+}

scripts/check-tmpdir-leaks.ts

@@ -1,19 +1,21 @@
 // Fails if any agent-device-test-run-* directory under TEST_RUN_TMP_ROOT is
 // abandoned — its owning process has exited without running its cleanup
-// (crash, OOM, timeout kill). Directories owned by a still-running process
-// are left alone: a concurrent `vitest run` (or node --test lane, wrapped by
-// scripts/node-test-tmpdir.ts) in another worktree keeps its own directory
-// present until its own teardown, which is not a leak. See
-// check-tmpdir-leaks-model.ts for the liveness check.
+// (crash, OOM, timeout kill) AND no live process still holds it as TMPDIR.
+// Directories owned by a still-running process are left alone: a concurrent
+// `vitest run` (or node --test lane, wrapped by scripts/node-test-tmpdir.ts) in
+// another worktree keeps its own directory present until its own teardown,
+// which is not a leak; so are the orphaned children of a killed run, until the
+// last of them exits. See check-tmpdir-leaks-model.ts for the liveness model.
 //
 // Covers both redirection mechanisms sharing this root/prefix: Vitest's
 // globalSetup/globalTeardown (scripts/vitest-tmpdir-global-setup.ts) and the
 // node --test wrapper (scripts/node-test-tmpdir.ts, #1595) that every
-// `node --test` package.json script now runs through.
+// `node --test` package.json script now runs through. Both prune what an
+// earlier killed run left behind before creating their own directory, so a
+// failure here names the run that just finished — never a historical one.
 
 import path from 'node:path';
-import { findLeakedRunDirectories } from './check-tmpdir-leaks-model.ts';
-import { TEST_RUN_TMP_ROOT } from './vitest-tmpdir-global-setup.ts';
+import { TEST_RUN_TMP_ROOT, findLeakedRunDirectories } from './check-tmpdir-leaks-model.ts';
 
 const leaks = findLeakedRunDirectories(TEST_RUN_TMP_ROOT);