Files
callstack__agent-device/scripts/check-tmpdir-leaks-model.ts
T
Michał Pierzchała 23a3016e9b fix: stop the unit suite from leaking temp directories (#1593)
* fix: stop the unit suite from leaking temp directories

~650 test call sites across the unit suite create scratch directories via
fs.mkdtemp(path.join(os.tmpdir(), ...)) or shared factories (makeSessionStore)
with no cleanup, ever. Over time this accumulated 1.16M+ orphaned directories
in the real system tmpdir, slow enough to make tools that enumerate $TMPDIR at
startup (e.g. opencode) take 1-2 minutes to launch.

Rather than migrate every call site, redirect os.tmpdir() itself for the
lifetime of the whole `vitest run` invocation: scripts/vitest-tmpdir-global-setup.ts
wires in as vitest's globalSetup/globalTeardown, points TMPDIR at one
/tmp-rooted directory (verified: env mutations here propagate to every forked
worker, confirmed empirically), and removes it in one recursive rm after every
worker across every project finishes. Since os.tmpdir() reads TMPDIR on every
call, this covers all ~650 call sites without touching any of them.

Rooted at /tmp rather than nested inside the current (already deep, on macOS)
os.tmpdir(): that broke real AF_UNIX socket tests (runner-usbmux.test.ts) by
pushing socket paths past the 104-byte sun_path limit. A per-file afterAll
hook was tried first but proved unreliable — 5 of 7 workers in one run never
ran it before their process was torn down; the global setup/teardown pair
(one process, confirmed single execution) is the mechanism that's actually
guaranteed to run once.

Also adds:
- scripts/check-tmpdir-leaks.ts: CI/local guard asserting no
  agent-device-test-run-* directory survives a run (a leftover one means a
  worker was killed before cleanup could run).
- src/__tests__/test-utils/tmp-dir.ts: documented mkdtempForTest /
  mkdtempForTestSync helpers, the discoverable way to get a scratch dir going
  forward (mirrors the src/utils/exec.ts pattern for node:child_process).
- scripts/check-test-tmpdir-helper.ts: ratchet guard capping raw
  fs.mkdtemp/mkdtempSync call sites in test files at today's count (632); it
  can only shrink as call sites migrate to the helper.

* fix: make check-tmpdir-leaks scan the same root the fix actually uses

check-tmpdir-leaks.ts was scanning os.tmpdir() for leftover run
directories, but vitest-tmpdir-global-setup.ts creates them under a
hard-coded /tmp. On macOS those are different paths (TMPDIR is a deep
per-user /var/folders/.../T/ directory) — the guard could never find a
leak on the exact platform the original leak happened on, only on
Linux CI where os.tmpdir() already is /tmp.

Export TEST_RUN_TMP_ROOT and TEST_RUN_TMP_PREFIX from the global-setup
module and import them in the leak check instead of recomputing a path
that can drift. Switched from a fixed pid-based directory name to
fs.mkdtempSync so a same-named leftover from a prior killed run (or,
on a shared machine, another user) can't collide with a live run.

Also fixes two stale comments (in this file and ci.yml) that still
described the per-file afterAll hook design that was abandoned in
favor of the global setup/teardown pair, and notes the check only
covers vitest runs, not the node --test lanes (test:smoke,
test:integration:node).

Verified live: with the old code the guard reported no leaks even with
a real orphaned /tmp/agent-device-test-run-* directory present (left
by a command that got killed mid-run); with this fix it correctly
found and reported it.

* simplify: drop the tmpdir ratchet guard, keep the leak check local-only

Two guards were more than this needed:

- check:test-tmpdir-helper (ratchet on raw fs.mkdtemp call counts)
  protects nothing a bug could actually trigger — the leak is already
  fixed architecturally regardless of call-site count, so this was
  pure style/discoverability nudging. Dropped the script and its
  check:tooling/CI wiring; kept mkdtempForTest/mkdtempForTestSync in
  tmp-dir.ts as the documented option without enforcing it.

- check:tmpdir-leaks in CI added little: GitHub-hosted runners are
  destroyed after each job, so a leftover directory there is harmless
  by construction, and a worker getting killed mid-run would already
  surface as a job failure some other way. Its real value is local,
  on the long-lived dev machines where the original leak actually
  accumulated — kept it wired into check:unit, dropped the CI step.

* 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.

* refactor: migrate raw fs.mkdtemp call sites to mkdtempForTest(Sync)

Migrates 629 raw fs.mkdtemp(Sync)(path.join(os.tmpdir(), PREFIX)) call
sites across 168 test files to the mkdtempForTest / mkdtempForTestSync
helpers (src/__tests__/test-utils/tmp-dir.ts), so there's one
documented, discoverable way to get a scratch dir in a test — cleanup
already didn't depend on the call-site shape (the global TMPDIR
redirect covers any of them), this is purely for consistency and
discoverability, same reasoning as src/utils/exec.ts for
node:child_process. Existing manual per-test cleanup (fs.rm in
finally/afterEach/onTestFinished blocks) is untouched — the global
teardown is a fallback for killed workers, not a replacement for tests
cleaning up after themselves.

Migrated with a one-off AST-based codemod (oxc-parser, since regex
mismatched multi-line calls and complex prefix expressions like
`options?.tempPrefix ?? 'default-'`) rather than by hand across 168
files. The codemod isn't included — it doesn't need to survive this
commit. Caught and fixed one real bug in it during review: a small
number of files declare a second import statement later in the file,
after some of the matched call sites, which broke a naive "insert
after the textually-last ImportDeclaration" placement; fixed to insert
after the top contiguous import block instead, plus a self-check that
re-parses every generated file before writing it.

Also fixes 5 fallow dead-code findings the branch introduced: the
vitest globalSetup functions (setup/teardown) are only referenced by
the config-string path vitest.config.ts hands to globalSetup, invisible
to static analysis — suppressed with the documented convention.
isProcessAlive didn't need to be exported (nothing outside the module
uses it). And dropped a barrel re-export of the new helpers from
test-utils/index.ts: nothing actually imports through the barrel
(matching the existing makeSessionStore convention, which is imported
directly from store-factory.ts everywhere despite also being
barrel-exported), so the re-export was genuinely dead.

Documents the convention in docs/agents/testing.md.

Verified: full unit suite (5308 tests) passes except the one
pre-existing, unrelated package-exports.test.ts failure; typecheck,
lint, and format all clean; fallow audit clean against the PR base.

* fix: correct fallow suppression token and drop unused barrel re-export

These were meant to be part of 397cdd6d7 (verified locally before that
commit) but didn't actually get staged — caught by CI's Fallow Code
Quality check re-running against the pushed commit, which still had
the plural 'unused-exports' token (fallow expects singular
'unused-export') and the dead barrel re-export.

* fix: migrate the two mkdtemp call sites the rebase silently reintroduced

Rebasing onto main pulled in #1594's two new test cases in this file,
added independently of this branch's migration, still using raw
fs.mkdtempSync(path.join(os.tmpdir(), ...)). Git's line-based merge
found no textual conflict with this branch's removal of the os import
(the changes touch non-overlapping regions), so it silently produced
a file that doesn't typecheck. Migrated both to mkdtempForTestSync for
consistency with the rest of the file, caught by CI's Typecheck,
Fallow Code Quality, and FreeRange checks re-running against the
pushed commit.

* test: pin Vitest tmpdir lifecycle

* fix: preserve the Swift cache across test runs
2026-08-05 08:56:39 +02:00

36 lines
1.2 KiB
TypeScript

import fs from 'node:fs';
import { TEST_RUN_TMP_PREFIX } from './vitest-tmpdir-global-setup.ts';
const PID_SUFFIX = new RegExp(`^${TEST_RUN_TMP_PREFIX}(\\d+)-`);
function isProcessAlive(pid: number): boolean {
try {
process.kill(pid, 0);
return true;
} catch (error) {
// EPERM means the pid exists but we lack permission to signal it — still alive.
return (error as NodeJS.ErrnoException).code === 'EPERM';
}
}
/**
* 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.
*/
export function findLeakedRunDirectories(
root: string,
isAlive: (pid: number) => boolean = isProcessAlive,
): string[] {
return fs
.readdirSync(root, { withFileTypes: true })
.filter((entry) => entry.isDirectory() && entry.name.startsWith(TEST_RUN_TMP_PREFIX))
.filter((entry) => {
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);
}