test(e2e): reclaim the run's temp dirs through a single per-run root - #358
Merged
Conversation
The E2E harness never removed the directories it created under $TMPDIR: launch.ts mkdtemp'd a user-data dir per run, and each temp-file suite mkdtemp'd a workspace dir but only unlinked the .md inside it. Measured on a dev box: 2 544 stale quoll-* dirs (912 of them quoll-e2e-*, 880 MB), and a clean run added 53 more. Give the run one root and one owner. launch.ts creates a single quoll-e2e- root holding `ud` (VS Code --user-data-dir) and `w` (parent of every suite workspace dir), exports the root through extensionTestsEnv, and removes exactly that path in a finally. Suites allocate only through temp-root's makeTempDir/makeTempDirSync, so nothing needs its own dir teardown and no code globs quoll-e2e-* (a parallel run owns its own root). Deliberate choices: - resolveRunTempWorkDir throws when the env var is missing rather than falling back to os.tmpdir(), which would silently restore the leak. - dispose() surfaces EBUSY/EACCES and launch.ts names the path and fails the run; an unreclaimable leak must not exit green. - exitCode replaces process.exit so the finally actually runs on failure. - Path segments stay at `ud`/`w`: VS Code opens its IPC socket under --user-data-dir and macOS caps those paths at 103 chars. - temp-dir-choke-point.test.ts default-denies mkdtemp/tmpdir calls outside the seam, scanning the TypeScript AST rather than source text so a mention inside a comment or string can neither trip nor vacuate it. Verified: clean run leaves zero new dirs in $TMPDIR (2 597 before and after, 103 passing); a runner killed mid-suite leaves exactly its own one.
Review round on the temp-root change turned up one leak path the design
missed and one defect the mechanical migration introduced.
The interrupt path still stranded the whole root. @vscode/test-electron
ends innerRunTests with `if (exitRequested && listenerCount('SIGINT') === 0)
process.exit(1)`, so the first Ctrl+C — the graceful close, where the child
exits 0 — hard-exited this process from inside the await and skipped the
finally entirely. Owning a SIGINT listener keeps the exit on our side so
dispose() runs; a second Ctrl+C still force-closes through the library's own
handler. SIGTERM has no graceful path there at all, so it reclaims and
re-exits 143. An interrupted run now exits 130 instead of reporting a pass.
The migration also dropped `${slug}` from three suites' temp-dir names
(`fmtdoc-`, `sbar-`, `2panel-`), so an undisposed dir no longer said which
test made it. Restored, and `makeTempDir` now rejects any slug that is not a
bare name — `path.join` normalises `../`, so a separator would have landed
the dir outside the run root where nothing disposes it.
Also from the review:
- launch.ts gets an injectable seam so the wiring the whole change exists
for — which env key carries the root, which dir VS Code gets, and that
dispose() runs on the failure path too — is pinned by unit tests. Restoring
`process.exit(1)` in the catch now turns them red; before, every suite
stayed green.
- The socket-budget test measured the absolute path, so it passed on CI
(`/tmp`, 56 chars of slack) and could never catch a rename of the `ud`
segment. It now measures only the part this module controls.
- dispose()'s loudness is pinned: `not.toThrow()` on an already-removed root
is satisfied by `force: true` alone, so a dispose() that swallowed
everything passed it.
- The choke-point guard matched call-site spelling, so `import { mkdtemp as
mkTmp }` or `fs["mkdtempSync"](…)` walked past it. Both are caught now,
with an in-memory case exercising the new branches.
- The partial-init rollback rethrows the original error rather than letting a
failed rollback replace it, and dispose() retries for ~3s so a still-
draining VS Code cannot flip a green run red.
- preflightFixturesDir throws instead of process.exit()ing, so the "never
exit past a live root" invariant no longer depends on statement order.
Verified: clean run 103 passing with zero new dirs in $TMPDIR (2 597 before
and after); restoring the old `process.exit(1)` turns the wiring test red.
Coverage for the partial-init rollback resisted both vi.spyOn (node:fs
properties are non-configurable) and any natural filesystem trigger; it is
tracked as a follow-up rather than left implied.
Two follow-ups from the second review round. The run root was built from `fs.realpathSync(os.tmpdir())`, which I had added only so an assertion could compare against the resolved path. On macOS that turns every document path the suites hand VS Code from `/var/folders/…` into `/private/var/folders/…`, and the disk-conflict watcher filters watcher events by exact URI string — so the event never matched the document and `dirty-doc-disk-conflict > keeps unsaved edits on 'Keep my edits'` timed out waiting for a prompt that could not arrive. Measured: 2 failures in 4 runs with the realpath in, 0 in 3 after removing it, against 0 in 2 for the pre-change harness under the same load. The assertion now pins the unresolved form, so reintroducing realpath turns it red. The SIGTERM handler is removed. Codex flagged at Conf 97 that it disposes the root while VS Code may still be running; a plain `kill` of the launcher alone leaves the child alive to recreate user-data-dir under the path just removed, trading one stranded root for a stranded root plus an orphaned editor. The library exposes no child pid and no graceful stop, so there is no correct handler to write here — an aborted run keeping its own single root is the accepted outcome, and that is now stated in the code. Also: the SIGINT listener no longer makes Ctrl+C a dead key during the VS Code download. The library's own handler only exists after the child spawns, so before that our listener was the only one and merely suppressed Node's default termination; it now reclaims and exits 130 when nothing else is listening. And the choke-point test's tmpdir exemption is anchored at the name boundary — an unanchored suffix also exempted a renamed import ending in `tmpdir`.
⚠️ Correction to 3c56d60: its message described removing the SIGTERM handler and adding the pre-spawn SIGINT branch, but neither change was in the commit — an intervening `git checkout HEAD -- test/extension/` (used to A/B the old harness against the flake) had silently reverted both. They land here, and the realpath fix that commit actually did carry is unaffected. Review round 2 found the signal handling entirely unpinned: deleting the SIGINT registration, the `??= 130`, or the pre-spawn branch all left the suite green, so the leak this PR closes could be silently reopened by a tidy-up. All four mutations now go red. - The pre-spawn branch compares against a baseline captured at registration rather than a bare `listenerCount() === 1`. "Has the library taken over?" means "did a listener appear after ours", which reads correctly both in a bare `node` run and under a host that already listens — the `=== 1` form was also untestable, since the test runner holds its own SIGINT listeners. - SIGTERM stays unhandled, with the reasoning in the code: the library exposes no child pid, so a handler could only reclaim while VS Code may still be running, and a plain `kill` of the launcher leaves the child free to recreate user-data-dir under the path just removed. - preflight moves onto the LaunchDeps seam. Its safety was never "it throws" but "it runs before the root exists" — a throw from after createRoot() escapes with no finally either — so the ordering is pinned instead of asserted in a comment that told readers position did not matter. - The partial-init rollback IS testable: `vi.mock` swaps the module-registry entry rather than redefining a property on the frozen node:fs namespace, which is what defeated `vi.spyOn`. My earlier note calling it impossible was wrong; the follow-up TODO is removed and the test replaces it. - The choke-point guard now pins its own exemption scope. Widening the `tmpdir` allowance tree-wide was a green edit, and `mkdirSync` is deliberately not banned — so that exemption is the only thing standing between the tree and a hand-rolled `fs.mkdirSync(path.join(os.tmpdir(), …))`. Verified: 4973 unit tests green; two consecutive E2E runs 103 passing with zero new roots; each new assertion confirmed by a killing mutation.
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
1 participant
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
The E2E harness never removed the directories it created under
$TMPDIR.launch.tsmkdtemp'd a--user-data-dirper run, and each temp-file suitemkdtemp'd a workspace dir but only unlinked the.mdinside it. Measured on a dev box on 2026-08-14: 2 544 stalequoll-*dirs (912 of themquoll-e2e-*, 880 MB), and a clean run added 53 more.This gives the run one root with one owner:
launch.tscreates a singlequoll-e2e-root holdingud(VS Code--user-data-dir) andw(parent of every suite workspace dir), exports the root to the Electron host throughextensionTestsEnv, and removes exactly that path in afinally.Changes
test/extension/temp-root.ts(new): owns the run root (createRunTempRoot/dispose) and is the only sanctioned allocation site (makeTempDir/makeTempDirSync).test/extension/launch.ts: creates the root, passes it viaextensionTestsEnv, points--user-data-dirinside it, and disposes it in afinally.process.exit→process.exitCodeso thefinallyactually runs on a failing run.test/extension/e2e/harness.ts+ 20 suites: allocate through the seam instead offs.mkdtemp(os.tmpdir(), …). Existing per-test file/dir teardown is unchanged — it still keeps the live run tidy; the root removal is the backstop for what it misses.test/extension/temp-dir-choke-point.test.ts(new): default-deniesmkdtemp/tmpdircalls outside the seam, walking the TypeScript AST rather than source text, so the word appearing in a comment or string can neither trip the guard nor vacuate it.test/extension/temp-root.test.ts(new): pins allocation placement, non-empty-root reclamation, idempotent dispose, the throw-on-missing-env contract, and the macOS socket-path budget.Deliberate choices worth a reviewer's eye:
resolveRunTempWorkDirthrows when the env var is missing instead of falling back toos.tmpdir()— a fallback would silently restore this exact leak.dispose()surfacesEBUSY/EACCES;launch.tsnames the path and fails the run. A leak we cannot reclaim must not exit green.ud/w: VS Code opens its IPC socket under--user-data-dirand macOS caps those paths at 103 chars, of which$TMPDIRalone is 48.quoll-e2e-*— a parallel run (CI shard, another worktree) owns its own root and must survive ours.pnpm compiledoes not type-check this tree (the root tsconfig excludestest/). The E2E program is compiled only bynode test/extension/build.mjs.Related
quoll-e2e-*user-data dirs — teardown in-process"Test Plan
pnpm test:unit— 248 files / 4961 tests passing (includes the 9 new ones)node test/extension/build.mjs— E2E program type-checksenv -u ELECTRON_RUN_AS_NODE pnpm test:e2e— 103 passing;quoll-*count in$TMPDIR2 597 before → 2 597 after (zero new dirs; the pre-change baseline run added 53)os.tmpdir()in a suite turns the choke-point test red; pointing the scan at an empty dir turns the coverage assertion reddispose()a no-op turns twotemp-rootcases red