test: own the temp namespace in the test runners - #2068
Merged
Conversation
Suites reach the temp directory through 933 mkdtemp(tmpdir(), …) calls across 250 files, and nothing removes what they create: the user's tmpdir had accumulated over 47,000 stale maka-* directories. Making each call site book-keep its own removal needs the same eight lines of cleanup in every one of those files, and stays correct only for as long as nobody forgets. Give each workspace test process its own TMPDIR instead, removed with the process tree when the workspace finishes however it ends, and do the same in the headless runner, which already isolates HOME and XDG but left the temp namespace pointing at the host. No call site changes. Verified on packages/runtime: 2961 tests / 2930 pass / 22 fail both before and after, with 51 leaked directories before and 0 after.
Review of the first commit found two holes in the ownership it claimed. mkdtemp ran before the try, so a synchronous throw from spawn left the tree behind — the exact shape of the leak this PR exists to stop. And an rm that throws inside finally replaced whatever the function was already reporting, so a cleanup failure would mask the real test failure. Move everything after the tree exists under the try, and let removal failure warn instead of propagate. The headless runner had the same masking in its pre-existing finally, so it gets the same treatment. The comment claiming the process tree is always down by then was wrong: when terminateWorkspace itself fails the tree is removed under a process that may still be writing. That is the right trade — the run is already failed and killed, and leaking the tree is worse — so the comment now says so instead of asserting something the code does not guarantee. Tests lock the spawn-throw and stop paths; verified by reverting the implementation, which turns the spawn-throw case red.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test suites reach the temp directory through 933
mkdtemp(join(tmpdir(), …))calls spread over 250 files, and nothing removes what they create. On a developer machine that runs the suite repeatedly this accumulates without bound — the tmpdir this was found on held over 47,000 stalemaka-*directories. CI containers are discarded after a run, so the cost lands entirely on local development: slower tmpdir traversal, noise for anything that scans it, and enough churn that "did this run leak?" stops being measurable.The obvious fix is to give each call site a
cleanuparray and anafterEach, which is the conventionbuiltin-tools-file-worker.test.tsalready follows. Applied across the repo that is the same eight lines of book-keeping in 250 files — roughly 2,000 lines whose correctness depends on nobody ever forgetting them again, which the current state shows is not a safe assumption.So this puts the temp namespace where the process lifetime already is.
runWorkspaceis the single spawn point for every workspace suite: it now hands each test process its ownTMPDIR/TMP/TEMPand removes the tree when the workspace finishes, however it finishes. The headless runner gets the same treatment — it already isolatesHOME, the XDG directories andAPPDATAinto a tree it deletes on exit, and simply left the temp namespace pointing at the host. No call site changes, and nothing new for a future test author to remember.Ownership is the whole point, so the exit paths matter: the tree is created before the
trythat removes it covers everything after it, including a synchronous throw fromspawn, and a removal that fails warns rather than replacing whatever result the run was already reporting.Verification
packages/runtimeagainst the same build, once through the runner and once directly (npm run test:dist, which is what the runner invokes and therefore the no-isolation control), counting top-levelmaka-*directories in the host tmpdir before and after:node --test scripts/run-workspace-tests-parallel.test.mjs scripts/run-headless-tests.test.mjs— 7/7 pass.rmturns the three namespace tests red.npm run test:scripts(the onenpm testpath that does not go through the runner) measured at zero leaked directories already, so both paths are now clean.npm run lint,npm run format:check— pass.Known boundaries
node --testby hand in a package) still leaks, by design: the isolation belongs to whatever starts the test process, and a barenode --testhas no such owner.packages/runtime-hostbuilds its Unix socket endpoint under a hardcoded/tmprather thanos.tmpdir()(src/control/endpoint.ts), so it is unaffected either way; it already prunes its own stale directories.TMP/TEMPvariables are set alongsideTMPDIRbut were not exercised — no Windows host was available.npm testacross every workspace.