2 Commits

Author	SHA1	Message	Date
Michał Pierzchała	0fb38f1da2	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.	2026-08-18 17:48:12 +02:00
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