From ca4021e49e7b05f17f76cd94ef03bad6b06e5004 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Fri, 14 Aug 2026 11:12:08 +1000 Subject: [PATCH 1/4] test(e2e): reclaim the run's temp dirs through a single per-run root 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. --- .../e2e/caret-handoff-mention-range.test.ts | 14 ++- test/extension/e2e/caret-handoff.test.ts | 16 ++- test/extension/e2e/crlf-roundtrip.test.ts | 4 +- .../decoration-external-edit-boundary.test.ts | 11 ++- .../e2e/dirty-doc-disk-conflict.test.ts | 4 +- .../e2e/external-edit-propagates.test.ts | 8 +- .../e2e/external-fs-write-propagates.test.ts | 4 +- .../e2e/format-document-active-edge.test.ts | 5 +- .../e2e/handoff-edit-applied-barrier.test.ts | 4 +- test/extension/e2e/harness.ts | 12 ++- .../e2e/hidden-webview-resync.test.ts | 12 ++- ...ost-rejects-edit-preserves-webview.test.ts | 4 +- .../e2e/lint-diagnostics-propagate.test.ts | 9 +- .../e2e/lint-problems-config-toggle.test.ts | 5 +- .../extension/e2e/minimal-range-apply.test.ts | 11 ++- .../extension/e2e/mixed-eol-roundtrip.test.ts | 11 ++- .../e2e/racing-applyedit-semantics.test.ts | 12 ++- .../e2e/remember-last-editor-surface.test.ts | 12 ++- .../e2e/status-bar-active-edge.test.ts | 5 +- .../e2e/toggle-editor-in-place-swap.test.ts | 12 ++- .../e2e/two-panel-config-caret.test.ts | 12 ++- test/extension/launch.ts | 40 +++++--- test/extension/temp-dir-choke-point.test.ts | 87 ++++++++++++++++ test/extension/temp-root.test.ts | 98 +++++++++++++++++++ test/extension/temp-root.ts | 78 +++++++++++++++ test/extension/tsconfig.json | 2 +- 26 files changed, 417 insertions(+), 75 deletions(-) create mode 100644 test/extension/temp-dir-choke-point.test.ts create mode 100644 test/extension/temp-root.test.ts create mode 100644 test/extension/temp-root.ts diff --git a/test/extension/e2e/caret-handoff-mention-range.test.ts b/test/extension/e2e/caret-handoff-mention-range.test.ts index a311c5cb..839ac183 100644 --- a/test/extension/e2e/caret-handoff-mention-range.test.ts +++ b/test/extension/e2e/caret-handoff-mention-range.test.ts @@ -16,10 +16,16 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, tick, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDir, + tick, + VIEW_TYPE, +} from "./harness"; const PROTOCOL = 1; const INSERT_AT_MENTIONED = "claude-code.insertAtMentioned"; @@ -45,7 +51,7 @@ describe("caret-handoff does not clobber the ⌘⌥K mention range", function () }); it("keeps the reveal's line-range selection through the insertAtMentioned read", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("mention-range"); tempFile = path.join(dir, "mention-range.md"); await fs.writeFile(tempFile, "line0\nline1\nline2\nline3\n"); const uri = vscode.Uri.file(tempFile); @@ -136,7 +142,7 @@ describe("caret-handoff does not clobber the ⌘⌥K mention range", function () // (2,5) — so the assertion can only pass if the ordinary switch actually // applied the re-reported caret (latch consumed, not stranded; a stranded // latch would skip the apply and leave the fresh editor at its default). - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("mention-range-recovery"); tempFile = path.join(dir, "mention-range-recovery.md"); await fs.writeFile(tempFile, "line0\nline1\nline2\nline3\n"); const uri = vscode.Uri.file(tempFile); diff --git a/test/extension/e2e/caret-handoff.test.ts b/test/extension/e2e/caret-handoff.test.ts index eb0b7b2e..02a4ce7a 100644 --- a/test/extension/e2e/caret-handoff.test.ts +++ b/test/extension/e2e/caret-handoff.test.ts @@ -1,9 +1,15 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, tick, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDir, + tick, + VIEW_TYPE, +} from "./harness"; const PROTOCOL = 1; @@ -25,7 +31,7 @@ describe("caret-handoff", function () { }); it("caret-report inbound mutates no document and posts no Document event (reducer bypass)", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("caret-bypass"); tempFile = path.join(dir, "caret-bypass.md"); await fs.writeFile(tempFile, "line0\nline1\nline2\n"); const uri = vscode.Uri.file(tempFile); @@ -76,7 +82,7 @@ describe("caret-handoff", function () { }); it("applies the tracked caret to the live text editor on Quoll→text-editor switch", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("caret-apply"); tempFile = path.join(dir, "caret-apply.md"); await fs.writeFile(tempFile, "line0\nline1\nline2\nline3\n"); const uri = vscode.Uri.file(tempFile); @@ -109,7 +115,7 @@ describe("caret-handoff", function () { }); it("posts a caret-apply with the tracked caret on text-editor→Quoll switch (Codex #1)", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("caret-push"); tempFile = path.join(dir, "caret-push.md"); await fs.writeFile(tempFile, "line0\nline1\nline2\nline3\nline4\n"); const uri = vscode.Uri.file(tempFile); diff --git a/test/extension/e2e/crlf-roundtrip.test.ts b/test/extension/e2e/crlf-roundtrip.test.ts index 3a219cdd..e394209e 100644 --- a/test/extension/e2e/crlf-roundtrip.test.ts +++ b/test/extension/e2e/crlf-roundtrip.test.ts @@ -1,6 +1,5 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; @@ -9,6 +8,7 @@ import { getHarness, isDocumentAfter, isDocumentEvent, + makeTempDir, VIEW_TYPE, } from "./harness"; @@ -47,7 +47,7 @@ describe("crlf-roundtrip", function () { it("preserves \\r\\n bytes end-to-end through the host write path", async () => { // Per-test temp file (mirrors external-edit-propagates) so a mid-test // failure does not leave a shared fixture dirty for subsequent tests. - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-crlf-")); + const dir = await makeTempDir("crlf"); tempFile = path.join(dir, "crlf.md"); // Initial on-disk bytes: pure CRLF. The trailing CRLF after the last // line gives the file two distinct CRLF separators so a single- diff --git a/test/extension/e2e/decoration-external-edit-boundary.test.ts b/test/extension/e2e/decoration-external-edit-boundary.test.ts index e11c0e37..2e57bf05 100644 --- a/test/extension/e2e/decoration-external-edit-boundary.test.ts +++ b/test/extension/e2e/decoration-external-edit-boundary.test.ts @@ -9,10 +9,15 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDir, + VIEW_TYPE, +} from "./harness"; describe("C4a external-edit byte-identity across a token boundary", function () { this.timeout(20000); @@ -36,7 +41,7 @@ describe("C4a external-edit byte-identity across a token boundary", function () // Per-test temp file so a mid-test failure does not leave any shared // fixture dirty for subsequent tests. Mirrors the pattern used by // external-edit-propagates.test.ts. - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-c4a-")); + const dir = await makeTempDir("c4a"); tempFile = path.join(dir, "boundary.md"); const initial = "**bold** rest"; await fs.writeFile(tempFile, initial); diff --git a/test/extension/e2e/dirty-doc-disk-conflict.test.ts b/test/extension/e2e/dirty-doc-disk-conflict.test.ts index 8ef06e48..b38c89b4 100644 --- a/test/extension/e2e/dirty-doc-disk-conflict.test.ts +++ b/test/extension/e2e/dirty-doc-disk-conflict.test.ts @@ -12,7 +12,6 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { @@ -20,6 +19,7 @@ import { getHarness, isDocumentAfter, isDocumentEvent, + makeTempDir, VIEW_TYPE, } from "./harness"; @@ -51,7 +51,7 @@ describe("dirty-doc-disk-conflict", function () { // Open the temp file in Quoll, seed, then dirty it via an in-session edit that // does NOT match disk. Returns the seed docVersion. async function openAndDirty(bodyEdit: string): Promise { - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-dirty-conflict-")); + tempDir = await makeTempDir("dirty-conflict"); tempFile = path.join(tempDir, "dirty-conflict.md"); await fs.writeFile(tempFile, "# Initial\n\nbody\n"); const uri = vscode.Uri.file(tempFile); diff --git a/test/extension/e2e/external-edit-propagates.test.ts b/test/extension/e2e/external-edit-propagates.test.ts index dc5b5e88..eeb15a60 100644 --- a/test/extension/e2e/external-edit-propagates.test.ts +++ b/test/extension/e2e/external-edit-propagates.test.ts @@ -1,6 +1,5 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; @@ -10,6 +9,7 @@ import { getHarness, isDocumentAfter, isDocumentEvent, + makeTempDir, tick, VIEW_TYPE, } from "./harness"; @@ -36,7 +36,7 @@ describe("external-edit-propagates", function () { it("propagates an externally-applied WorkspaceEdit as a higher-docVersion Document", async () => { // Per-test temp file so a mid-test failure does not leave the // shared fixture dirty for subsequent tests. - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("ext-edit"); tempFile = path.join(dir, "ext-edit.md"); await fs.writeFile(tempFile, "# Initial\n\nbody\n"); const uri = vscode.Uri.file(tempFile); @@ -75,7 +75,7 @@ describe("external-edit-propagates", function () { }); it("coalesces a burst of lock-free external edits into fewer Document posts (latest wins)", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("ext-edit-burst"); tempFile = path.join(dir, "ext-edit-burst.md"); await fs.writeFile(tempFile, "# Initial\n\nbody\n"); const uri = vscode.Uri.file(tempFile); @@ -143,7 +143,7 @@ describe("external-edit-propagates", function () { }); it("dispatches a lock-held racing external edit immediately (refused settlement posts the live version)", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("lock-race"); tempFile = path.join(dir, "lock-race.md"); await fs.writeFile(tempFile, "# Initial\n\nbody\n"); const uri = vscode.Uri.file(tempFile); diff --git a/test/extension/e2e/external-fs-write-propagates.test.ts b/test/extension/e2e/external-fs-write-propagates.test.ts index c9620947..8dc90733 100644 --- a/test/extension/e2e/external-fs-write-propagates.test.ts +++ b/test/extension/e2e/external-fs-write-propagates.test.ts @@ -33,7 +33,6 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { @@ -41,6 +40,7 @@ import { getHarness, isDocumentAfter, isDocumentEvent, + makeTempDir, VIEW_TYPE, } from "./harness"; import type { DocumentMessageShape, RecordedEventShape } from "./types"; @@ -70,7 +70,7 @@ describe("external-fs-write-propagates", function () { it("propagates an out-of-process fs.writeFile as a higher-docVersion Document", async () => { // Per-test temp dir so a mid-test failure does not leave the shared // fixture dirty for subsequent tests (mirrors external-edit-propagates). - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-fswrite-")); + tempDir = await makeTempDir("fswrite"); tempFile = path.join(tempDir, "ext-fs-write.md"); await fs.writeFile(tempFile, "# Initial\n\nbody\n"); const uri = vscode.Uri.file(tempFile); diff --git a/test/extension/e2e/format-document-active-edge.test.ts b/test/extension/e2e/format-document-active-edge.test.ts index cf8987ad..82fdb491 100644 --- a/test/extension/e2e/format-document-active-edge.test.ts +++ b/test/extension/e2e/format-document-active-edge.test.ts @@ -1,10 +1,9 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; -import { cleanupBetweenTests, getHarness, tick, VIEW_TYPE } from "./harness"; +import { cleanupBetweenTests, getHarness, makeTempDir, tick, VIEW_TYPE } from "./harness"; import type { PanelControlsShape, TestHarnessShape } from "./types"; // Pins the host-side routing of `quoll.formatDocument`: the command forwards a @@ -43,7 +42,7 @@ async function openTempQuoll( slug: string, previous: PanelControlsShape | null ): Promise<{ uri: vscode.Uri; file: string; panel: PanelControlsShape }> { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), `quoll-fmtdoc-${slug}-`)); + const dir = await makeTempDir(`fmtdoc-`); const file = path.join(dir, `${slug}.md`); await fs.writeFile(file, content); const uri = vscode.Uri.file(file); diff --git a/test/extension/e2e/handoff-edit-applied-barrier.test.ts b/test/extension/e2e/handoff-edit-applied-barrier.test.ts index f9e9325d..6294d107 100644 --- a/test/extension/e2e/handoff-edit-applied-barrier.test.ts +++ b/test/extension/e2e/handoff-edit-applied-barrier.test.ts @@ -12,7 +12,6 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; @@ -21,6 +20,7 @@ import { deferred, getHarness, isDocumentEvent, + makeTempDir, tick, VIEW_TYPE, } from "./harness"; @@ -41,7 +41,7 @@ describe("handoff edit-applied barrier", function () { // its uri. The seed content is short (3 lines) so the applied 40-line clamp // is non-vacuous. const openTempDoc = async (): Promise => { - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-barrier-")); + tempDir = await makeTempDir("barrier"); const tempFile = path.join(tempDir, "barrier.md"); await fs.writeFile(tempFile, "# seed\n\nbody\n"); const uri = vscode.Uri.file(tempFile); diff --git a/test/extension/e2e/harness.ts b/test/extension/e2e/harness.ts index 8516cc1d..4301c72e 100644 --- a/test/extension/e2e/harness.ts +++ b/test/extension/e2e/harness.ts @@ -1,7 +1,7 @@ import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; +import { makeTempDir } from "../temp-root"; import type { DocumentMessageShape, EditorConfigMessageShape, @@ -15,6 +15,14 @@ import type { export const EXTENSION_ID = "mtskf.quoll"; export const VIEW_TYPE = "quoll.editMarkdown"; +// Every temp dir an E2E suite creates lives under the run root that +// launch.ts made and disposes on exit — that single owner is why no suite +// needs dir teardown of its own, and why nothing here ever globs +// `quoll-e2e-*` (a parallel run owns its own root). Suites must NOT call +// fs.mkdtemp(os.tmpdir(), …); test/extension/temp-dir-choke-point.test.ts +// enforces that. Re-exported so suites keep importing from "./harness". +export { makeTempDir, makeTempDirSync } from "../temp-root"; + // __dirname at runtime is `out/test-e2e/e2e/`. Resolve up to the // repo root then back into the source-controlled fixtures directory. // Avoids needing to copy *.md into out/ as a build step. @@ -178,7 +186,7 @@ export async function openTempQuoll( slug: string, previous: PanelControlsShape | null ): Promise<{ uri: vscode.Uri; file: string; panel: PanelControlsShape }> { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), `quoll-e2e-${slug}-`)); + const dir = await makeTempDir(slug); const file = path.join(dir, `${slug}.md`); await fs.writeFile(file, content); const uri = vscode.Uri.file(file); diff --git a/test/extension/e2e/hidden-webview-resync.test.ts b/test/extension/e2e/hidden-webview-resync.test.ts index 232f8028..39f12488 100644 --- a/test/extension/e2e/hidden-webview-resync.test.ts +++ b/test/extension/e2e/hidden-webview-resync.test.ts @@ -1,9 +1,15 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, tick, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDir, + tick, + VIEW_TYPE, +} from "./harness"; describe("hidden-webview-resync", function () { this.timeout(25000); @@ -24,7 +30,7 @@ describe("hidden-webview-resync", function () { }); it("posts a fresh Document when a hidden panel becomes visible after an external edit", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-")); + const dir = await makeTempDir("hidden"); tempFile = path.join(dir, "hidden.md"); await fs.writeFile(tempFile, "# Original\n"); const uri = vscode.Uri.file(tempFile); diff --git a/test/extension/e2e/host-rejects-edit-preserves-webview.test.ts b/test/extension/e2e/host-rejects-edit-preserves-webview.test.ts index 7bbd85bf..ac91a3a2 100644 --- a/test/extension/e2e/host-rejects-edit-preserves-webview.test.ts +++ b/test/extension/e2e/host-rejects-edit-preserves-webview.test.ts @@ -1,6 +1,5 @@ import * as assert from "node:assert"; import * as fs from "node:fs"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; @@ -10,6 +9,7 @@ import { getHarness, hideQuollByOpeningOtherDoc, isDocumentEvent, + makeTempDirSync, openFixtureWithQuoll, tick, VIEW_TYPE, @@ -22,7 +22,7 @@ const isEditRejectedEvent = (e: { message: { type: string } }) => // resumption case, and the revert-check of the deferred-race guard) writes to a // throwaway file instead of mutating a committed fixture. function tempMd(name: string): vscode.Uri { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), "quoll-reject-")); + const dir = makeTempDirSync("reject"); const p = path.join(dir, name); fs.writeFileSync(p, "# Title\n\nbody\n", "utf8"); return vscode.Uri.file(p); diff --git a/test/extension/e2e/lint-diagnostics-propagate.test.ts b/test/extension/e2e/lint-diagnostics-propagate.test.ts index babf967c..4acdaf51 100644 --- a/test/extension/e2e/lint-diagnostics-propagate.test.ts +++ b/test/extension/e2e/lint-diagnostics-propagate.test.ts @@ -1,9 +1,8 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; -import { cleanupBetweenTests, getHarness, VIEW_TYPE } from "./harness"; +import { cleanupBetweenTests, getHarness, makeTempDir, VIEW_TYPE } from "./harness"; // Poll vscode.languages.getDiagnostics(uri) until `predicate` holds or the // deadline passes. Lint is debounced (250ms) in the webview, then posted across @@ -52,7 +51,7 @@ describe("lint-diagnostics-propagate", function () { }); it("mirrors lint into Problems with correct range, updates on fix, reopens, clears on close", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-lint-e2e-")); + const dir = await makeTempDir("lint"); tempFile = path.join(dir, "heading-skip.md"); // h1 -> h3 skips h2: heading-increment (MD001-equivalent) warning on "### Skip". await fs.writeFile(tempFile, "# Title\n\n### Skip\n"); @@ -113,7 +112,7 @@ describe("lint-diagnostics-propagate", function () { }); it("maps ranges correctly for a CRLF document (line/character is EOL-invariant)", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-lint-crlf-")); + const dir = await makeTempDir("lint-crlf"); tempFile = path.join(dir, "crlf.md"); // Same violation, CRLF line endings. An offset-based wire would mis-place // the range (CM is LF-internal, the TextDocument is CRLF); line/character @@ -147,7 +146,7 @@ describe("lint-diagnostics-propagate", function () { // (Task 3) and toLintDiagnostics is host-document-independent (Task 2), so the // host reproduces exactly the ranges the webview computed for its content. it("surfaces a violation introduced by an external edit, at the correct line", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-lint-dyn-")); + const dir = await makeTempDir("lint-dyn"); tempFile = path.join(dir, "baseline.md"); // Line 0 carries a single trailing space → a STABLE `no-trailing-spaces` // finding (a single trailing space is flagged; only exactly two on a diff --git a/test/extension/e2e/lint-problems-config-toggle.test.ts b/test/extension/e2e/lint-problems-config-toggle.test.ts index 5fea45fc..8e0f469c 100644 --- a/test/extension/e2e/lint-problems-config-toggle.test.ts +++ b/test/extension/e2e/lint-problems-config-toggle.test.ts @@ -1,9 +1,8 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; -import { cleanupBetweenTests, getHarness, VIEW_TYPE } from "./harness"; +import { cleanupBetweenTests, getHarness, makeTempDir, VIEW_TYPE } from "./harness"; const KEY = "quoll.lint.problems.enabled"; @@ -73,7 +72,7 @@ describe("lint-problems-config-toggle", function () { // depend on inherited settings state). await vscode.workspace.getConfiguration().update(KEY, true, vscode.ConfigurationTarget.Global); - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-lint-toggle-")); + const dir = await makeTempDir("lint-toggle"); tempFile = path.join(dir, "heading-skip.md"); // h1 -> h3 skips h2: a heading-increment warning on "### Skip" (line 2). await fs.writeFile(tempFile, "# Title\n\n### Skip\n"); diff --git a/test/extension/e2e/minimal-range-apply.test.ts b/test/extension/e2e/minimal-range-apply.test.ts index 7d1d0c96..2b6a40f9 100644 --- a/test/extension/e2e/minimal-range-apply.test.ts +++ b/test/extension/e2e/minimal-range-apply.test.ts @@ -1,10 +1,15 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDir, + VIEW_TYPE, +} from "./harness"; describe("minimal-range-apply", function () { this.timeout(20000); @@ -23,7 +28,7 @@ describe("minimal-range-apply", function () { }); it("emits a minimal WorkspaceEdit range, not a whole-document replace", async () => { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-min-")); + const dir = await makeTempDir("min"); tempFile = path.join(dir, "min.md"); const base = "# Title\n\nhello world\n"; await fs.writeFile(tempFile, base); diff --git a/test/extension/e2e/mixed-eol-roundtrip.test.ts b/test/extension/e2e/mixed-eol-roundtrip.test.ts index 9567ae51..d2550bfb 100644 --- a/test/extension/e2e/mixed-eol-roundtrip.test.ts +++ b/test/extension/e2e/mixed-eol-roundtrip.test.ts @@ -1,9 +1,14 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDir, + VIEW_TYPE, +} from "./harness"; /** * Mixed / CR-only EOL round-trip (TODO: webview-mixed-eol-roundtrip). @@ -44,7 +49,7 @@ describe("mixed-eol-roundtrip", function () { }); async function assertContract(originalBytes: Buffer): Promise { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-mixedeol-")); + const dir = await makeTempDir("mixedeol"); tempFile = path.join(dir, "doc.md"); // `as Uint8Array` bridges a Buffer vs Uint8Array // invariance in @types/node@20.16.0 + TS 5.9 (fs/Buffer.equals signatures); diff --git a/test/extension/e2e/racing-applyedit-semantics.test.ts b/test/extension/e2e/racing-applyedit-semantics.test.ts index 55a3bb08..804afc49 100644 --- a/test/extension/e2e/racing-applyedit-semantics.test.ts +++ b/test/extension/e2e/racing-applyedit-semantics.test.ts @@ -1,10 +1,16 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, tick, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDir, + tick, + VIEW_TYPE, +} from "./harness"; // Plan S5 — experiment: racing positional applyEdit semantics (#7, gates S6). // @@ -70,7 +76,7 @@ describe("racing-applyedit-semantics (Plan S5 experiment)", function () { }); async function openTemp(base: string): Promise { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), "quoll-e2e-s5-")); + const dir = await makeTempDir("s5"); tempFile = path.join(dir, "race.md"); await fs.writeFile(tempFile, base); const uri = vscode.Uri.file(tempFile); diff --git a/test/extension/e2e/remember-last-editor-surface.test.ts b/test/extension/e2e/remember-last-editor-surface.test.ts index 4855cc88..e31a3680 100644 --- a/test/extension/e2e/remember-last-editor-surface.test.ts +++ b/test/extension/e2e/remember-last-editor-surface.test.ts @@ -10,14 +10,20 @@ import * as assert from "node:assert"; import * as fs from "node:fs"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, tick, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDirSync, + tick, + VIEW_TYPE, +} from "./harness"; function tempMd(name: string): vscode.Uri { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), "quoll-surface-")); + const dir = makeTempDirSync("surface"); const p = path.join(dir, name); fs.writeFileSync(p, "# Title\n\nbody\n", "utf8"); return vscode.Uri.file(p); diff --git a/test/extension/e2e/status-bar-active-edge.test.ts b/test/extension/e2e/status-bar-active-edge.test.ts index 1b345bd5..a8ba60bd 100644 --- a/test/extension/e2e/status-bar-active-edge.test.ts +++ b/test/extension/e2e/status-bar-active-edge.test.ts @@ -1,9 +1,8 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; -import { cleanupBetweenTests, getHarness, tick, VIEW_TYPE } from "./harness"; +import { cleanupBetweenTests, getHarness, makeTempDir, tick, VIEW_TYPE } from "./harness"; import type { PanelControlsShape, StatusBarItemProbeShape, TestHarnessShape } from "./types"; // Pins the panel-side status-bar wiring PR #158 left untested: the item shows on @@ -49,7 +48,7 @@ async function openTempQuoll( previous: PanelControlsShape | null, openOptions?: vscode.TextDocumentShowOptions ): Promise<{ uri: vscode.Uri; file: string; panel: PanelControlsShape }> { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), `quoll-sbar-${slug}-`)); + const dir = await makeTempDir(`sbar-`); const file = path.join(dir, `${slug}.md`); await fs.writeFile(file, content); const uri = vscode.Uri.file(file); diff --git a/test/extension/e2e/toggle-editor-in-place-swap.test.ts b/test/extension/e2e/toggle-editor-in-place-swap.test.ts index f36f46ac..6798a455 100644 --- a/test/extension/e2e/toggle-editor-in-place-swap.test.ts +++ b/test/extension/e2e/toggle-editor-in-place-swap.test.ts @@ -8,14 +8,20 @@ import * as assert from "node:assert"; import * as fs from "node:fs"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; -import { cleanupBetweenTests, getHarness, isDocumentEvent, tick, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isDocumentEvent, + makeTempDirSync, + tick, + VIEW_TYPE, +} from "./harness"; function tempMd(name: string): vscode.Uri { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), "quoll-swap-")); + const dir = makeTempDirSync("swap"); const p = path.join(dir, name); fs.writeFileSync(p, "# Title\n\nbody\n", "utf8"); return vscode.Uri.file(p); diff --git a/test/extension/e2e/two-panel-config-caret.test.ts b/test/extension/e2e/two-panel-config-caret.test.ts index 95085c2f..036d0133 100644 --- a/test/extension/e2e/two-panel-config-caret.test.ts +++ b/test/extension/e2e/two-panel-config-caret.test.ts @@ -1,10 +1,16 @@ import * as assert from "node:assert"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; import { PROTOCOL_VERSION } from "./constants"; -import { cleanupBetweenTests, getHarness, isEditorConfigEvent, tick, VIEW_TYPE } from "./harness"; +import { + cleanupBetweenTests, + getHarness, + isEditorConfigEvent, + makeTempDir, + tick, + VIEW_TYPE, +} from "./harness"; import type { PanelControlsShape, RecordedEventShape, TestHarnessShape } from "./types"; const GUTTER_KEY = "quoll.lint.gutter.enabled"; @@ -42,7 +48,7 @@ async function openTempQuoll( slug: string, previous: PanelControlsShape | null ): Promise<{ uri: vscode.Uri; file: string; panel: PanelControlsShape }> { - const dir = await fs.mkdtemp(path.join(os.tmpdir(), `quoll-2panel-${slug}-`)); + const dir = await makeTempDir(`2panel-`); const file = path.join(dir, `${slug}.md`); await fs.writeFile(file, content); const uri = vscode.Uri.file(file); diff --git a/test/extension/launch.ts b/test/extension/launch.ts index 17874028..605a123d 100644 --- a/test/extension/launch.ts +++ b/test/extension/launch.ts @@ -4,9 +4,9 @@ // engines.vscode bumps in package.json, this constant bumps with it. import * as fs from "node:fs"; -import * as os from "node:os"; import * as path from "node:path"; import { runTests } from "@vscode/test-electron"; +import { createRunTempRoot, RUN_TEMP_ROOT_ENV } from "./temp-root"; const VS_CODE_VERSION = "1.94.0"; @@ -31,27 +31,45 @@ function preflightFixturesDir(): void { } async function main(): Promise { + preflightFixturesDir(); + + // VS Code creates a unix-domain IPC socket under user-data-dir; on + // macOS the socket path must fit in 103 chars. The repo path under + // ~/Dev/... + worktree name routinely exceeds that, so we put the + // user-data-dir in a short tmp path. mkdtemp guarantees a unique + // root per run so parallel CI shards don't collide. + // + // The root also parents every per-test workspace dir — the suites + // allocate through temp-root's makeTempDir, which reads the root from + // RUN_TEMP_ROOT_ENV below — so this one dispose() reclaims the run's + // whole tmp footprint. Before it, every run stranded its user-data dir + // plus one dir per temp-file test, forever (+53 dirs per run measured + // 2026-08-14, 2 544 accumulated). + const run = createRunTempRoot(); try { - preflightFixturesDir(); const extensionDevelopmentPath = path.resolve(__dirname, "../.."); const extensionTestsPath = path.resolve(__dirname, "./e2e/index"); - // VS Code creates a unix-domain IPC socket under user-data-dir; on - // macOS the socket path must fit in 103 chars. The repo path under - // ~/Dev/... + worktree name routinely exceeds that, so we put the - // user-data-dir in a short tmp path. mkdtemp guarantees a unique - // dir per run so parallel CI shards don't collide. - const userDataDir = fs.mkdtempSync(path.join(os.tmpdir(), "quoll-e2e-")); - await runTests({ version: VS_CODE_VERSION, extensionDevelopmentPath, extensionTestsPath, - launchArgs: ["--disable-extensions", `--user-data-dir=${userDataDir}`], + extensionTestsEnv: { [RUN_TEMP_ROOT_ENV]: run.root }, + launchArgs: ["--disable-extensions", `--user-data-dir=${run.userDataDir}`], }); } catch (err) { console.error("Failed to run E2E tests:", err); - process.exit(1); + // exitCode, not process.exit: exit() skips the finally below and would + // strand the whole root on every failing run. + process.exitCode = 1; + } finally { + try { + run.dispose(); + } catch (err) { + // Never silent — an unreclaimable root is the bug this owns. + console.error(`[e2e] failed to reclaim temp root ${run.root}:`, err); + process.exitCode = 1; + } } } diff --git a/test/extension/temp-dir-choke-point.test.ts b/test/extension/temp-dir-choke-point.test.ts new file mode 100644 index 00000000..e8a6c2e4 --- /dev/null +++ b/test/extension/temp-dir-choke-point.test.ts @@ -0,0 +1,87 @@ +import * as fs from "node:fs"; +import * as path from "node:path"; +import ts from "typescript"; +import { describe, expect, it } from "vitest"; + +// Default-deny: a suite that allocates straight from os.tmpdir() strands +// that dir forever — 2 544 accumulated, +53 per run, measured 2026-08-14. +// The only sanctioned allocation is temp-root.ts, whose dirs live under the +// per-run root that launch.ts disposes on exit. +// +// AST, not regex: a source-text scan trips on the word `mkdtemp` inside a +// comment or a string (this very file would trip it) and cannot tell a call +// from a mention. +const SCAN_ROOT = path.resolve(__dirname); +const BANNED = new Set(["mkdtemp", "mkdtempSync", "tmpdir"]); + +// The seam itself allocates — that is its job. +const ALLOCATION_SITE = path.join(SCAN_ROOT, "temp-root.ts"); +// The seam's test may NAME os.tmpdir() to pin where the root lands, but it +// still may not allocate: `mkdtemp*` stays banned there. +const TMPDIR_REFERENCE_ALLOWED = new Set([path.join(SCAN_ROOT, "temp-root.test.ts")]); + +function collectSources(dir: string, acc: string[] = []): string[] { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === "node_modules" || entry.name === "fixtures") { + continue; + } + collectSources(full, acc); + } else if (entry.name.endsWith(".ts")) { + acc.push(full); + } + } + return acc; +} + +function calleeName(node: ts.CallExpression): string { + const target = node.expression; + if (ts.isPropertyAccessExpression(target)) { + return target.name.text; + } + if (ts.isIdentifier(target)) { + return target.text; + } + return ""; +} + +describe("e2e temp-dir choke point", () => { + const sources = collectSources(SCAN_ROOT).filter((file) => file !== ALLOCATION_SITE); + + it("scans the whole test/extension tree", () => { + // Without this, a moved directory would make the scan below vacuously + // pass over an empty file list. + expect(sources.length).toBeGreaterThan(25); + expect(sources.some((f) => f.endsWith(path.join("e2e", "harness.ts")))).toBe(true); + expect(sources.some((f) => f.endsWith("launch.ts"))).toBe(true); + }); + + it("finds no temp-dir allocation outside temp-root.ts", () => { + const offenders: string[] = []; + for (const file of sources) { + const source = ts.createSourceFile( + file, + fs.readFileSync(file, "utf8"), + ts.ScriptTarget.ES2022, + true + ); + const visit = (node: ts.Node): void => { + if (ts.isCallExpression(node) && BANNED.has(calleeName(node))) { + const name = calleeName(node); + const exempt = name === "tmpdir" && TMPDIR_REFERENCE_ALLOWED.has(file); + if (!exempt) { + const { line } = source.getLineAndCharacterOfPosition(node.getStart(source)); + offenders.push(`${path.relative(SCAN_ROOT, file)}:${line + 1} ${name}()`); + } + } + ts.forEachChild(node, visit); + }; + visit(source); + } + expect( + offenders, + "use makeTempDir/makeTempDirSync from ./harness — temp-root.ts is the only allocation site" + ).toEqual([]); + }); +}); diff --git a/test/extension/temp-root.test.ts b/test/extension/temp-root.test.ts new file mode 100644 index 00000000..1ce7af88 --- /dev/null +++ b/test/extension/temp-root.test.ts @@ -0,0 +1,98 @@ +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { + createRunTempRoot, + makeTempDir, + makeTempDirSync, + RUN_TEMP_ROOT_ENV, + type RunTempRoot, + resolveRunTempWorkDir, +} from "./temp-root"; + +const created: RunTempRoot[] = []; + +const newRoot = (): RunTempRoot => { + const run = createRunTempRoot(); + created.push(run); + return run; +}; + +afterEach(() => { + for (const run of created.splice(0)) { + fs.rmSync(run.root, { recursive: true, force: true }); + } + delete process.env[RUN_TEMP_ROOT_ENV]; +}); + +describe("createRunTempRoot", () => { + it("gives each run a unique root holding the user-data and work dirs", () => { + const a = newRoot(); + const b = newRoot(); + expect(a.root).not.toBe(b.root); + expect(path.dirname(a.root)).toBe(fs.realpathSync(os.tmpdir())); + expect(path.basename(a.root).startsWith("quoll-e2e-")).toBe(true); + expect(path.dirname(a.userDataDir)).toBe(a.root); + expect(path.dirname(a.workDir)).toBe(a.root); + expect(fs.existsSync(a.userDataDir)).toBe(true); + expect(fs.existsSync(a.workDir)).toBe(true); + }); + + it("keeps the user-data path within the macOS socket budget", () => { + // VS Code opens its IPC socket under --user-data-dir and macOS caps + // those paths at 103 chars; an overlong path fails the launch, not a + // test, so pin the budget where a rename would trip it. + const run = newRoot(); + expect(run.userDataDir.length).toBeLessThanOrEqual(80); + }); + + it("reclaims a non-empty root", () => { + const run = newRoot(); + fs.writeFileSync(path.join(run.workDir, "leftover.md"), "# x\n"); + run.dispose(); + expect(fs.existsSync(run.root)).toBe(false); + }); + + it("dispose is idempotent on an already-removed root", () => { + const run = newRoot(); + run.dispose(); + expect(() => run.dispose()).not.toThrow(); + }); +}); + +describe("resolveRunTempWorkDir", () => { + it("returns the work dir under the exported root", () => { + const run = newRoot(); + expect(resolveRunTempWorkDir({ [RUN_TEMP_ROOT_ENV]: run.root })).toBe(run.workDir); + }); + + it("throws a diagnosable error when the runner did not export the root", () => { + expect(() => resolveRunTempWorkDir({})).toThrow(RUN_TEMP_ROOT_ENV); + expect(() => resolveRunTempWorkDir({})).toThrow(/launch\.ts/); + }); +}); + +describe("makeTempDir", () => { + it("allocates unique slug-named dirs under the run's work dir", async () => { + const run = newRoot(); + process.env[RUN_TEMP_ROOT_ENV] = run.root; + + const first = await makeTempDir("crlf"); + const second = await makeTempDir("crlf"); + const sync = makeTempDirSync("swap"); + + expect(first).not.toBe(second); + for (const dir of [first, second, sync]) { + expect(path.dirname(dir)).toBe(run.workDir); + expect(fs.existsSync(dir)).toBe(true); + } + expect(path.basename(first).startsWith("crlf-")).toBe(true); + expect(path.basename(sync).startsWith("swap-")).toBe(true); + + // The whole footprint goes with the ONE root removal — this is why no + // suite needs per-dir teardown of its own. + run.dispose(); + expect(fs.existsSync(first)).toBe(false); + }); +}); diff --git a/test/extension/temp-root.ts b/test/extension/temp-root.ts new file mode 100644 index 00000000..eb48c719 --- /dev/null +++ b/test/extension/temp-root.ts @@ -0,0 +1,78 @@ +// One temp root per E2E run. The process that CREATES the disposable state +// owns removing it: launch.ts mkdtemps this root, hands the path to the +// Electron host through extensionTestsEnv, and removes exactly this path in +// a finally. Nothing here ever globs `quoll-e2e-*` — a parallel run (a CI +// shard, a second agent's worktree) owns its own root and must survive ours. +import * as fs from "node:fs"; +import * as fsp from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; + +/** Env var carrying the run root into the Electron host. */ +export const RUN_TEMP_ROOT_ENV = "QUOLL_E2E_TEMP_ROOT"; + +// Deliberately two-letter: VS Code opens a unix-domain IPC socket under +// --user-data-dir and macOS caps those paths at 103 chars, of which $TMPDIR +// alone is 48. Nobody reads these paths, so a longer name buys no +// readability — only a launch-blocking risk. +const USER_DATA_SEGMENT = "ud"; +const WORK_SEGMENT = "w"; + +export interface RunTempRoot { + /** The only directory under os.tmpdir() this run may write to. */ + root: string; + /** VS Code --user-data-dir. */ + userDataDir: string; + /** Parent of every per-test workspace dir the suites allocate. */ + workDir: string; + /** Remove the whole root. Safe to call twice; THROWS if the filesystem + * refuses (EBUSY / EACCES) — a leak we cannot reclaim must be loud. */ + dispose(): void; +} + +export function createRunTempRoot(): RunTempRoot { + const root = fs.mkdtempSync(path.join(fs.realpathSync(os.tmpdir()), "quoll-e2e-")); + const userDataDir = path.join(root, USER_DATA_SEGMENT); + const workDir = path.join(root, WORK_SEGMENT); + try { + fs.mkdirSync(userDataDir); + fs.mkdirSync(workDir); + } catch (err) { + // Partial init still owns the root: reclaim it before the caller ever + // holds a handle, otherwise the throw leaks the dir we just created. + fs.rmSync(root, { recursive: true, force: true }); + throw err; + } + return { + root, + userDataDir, + workDir, + dispose(): void { + fs.rmSync(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); + }, + }; +} + +/** The work dir of the run this process belongs to. Throws (rather than + * silently falling back to os.tmpdir()) because a fallback would restore + * the very leak this module exists to close. */ +export function resolveRunTempWorkDir(env: NodeJS.ProcessEnv = process.env): string { + const root = env[RUN_TEMP_ROOT_ENV]; + if (!root) { + throw new Error( + `${RUN_TEMP_ROOT_ENV} is unset — the E2E suite must be launched through test/extension/launch.ts` + ); + } + return path.join(root, WORK_SEGMENT); +} + +/** Allocate a per-test workspace dir under the run root. This is the ONLY + * sanctioned temp-dir allocation in the E2E tree — enforced by + * test/extension/temp-dir-choke-point.test.ts. */ +export function makeTempDir(slug: string): Promise { + return fsp.mkdtemp(path.join(resolveRunTempWorkDir(), `${slug}-`)); +} + +export function makeTempDirSync(slug: string): string { + return fs.mkdtempSync(path.join(resolveRunTempWorkDir(), `${slug}-`)); +} diff --git a/test/extension/tsconfig.json b/test/extension/tsconfig.json index da8ec1ab..e2ea513f 100644 --- a/test/extension/tsconfig.json +++ b/test/extension/tsconfig.json @@ -14,5 +14,5 @@ "strict": true, "isolatedModules": false }, - "include": ["launch.ts", "e2e/**/*.ts"] + "include": ["launch.ts", "temp-root.ts", "e2e/**/*.ts"] } From 38b8e58d6b7831ea1bd1584feabb3277ecf0c2c1 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Fri, 14 Aug 2026 11:31:08 +1000 Subject: [PATCH 2/4] fix(e2e): close the interrupt leak and pin the reclaim wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../e2e/format-document-active-edge.test.ts | 2 +- .../e2e/status-bar-active-edge.test.ts | 2 +- .../e2e/two-panel-config-caret.test.ts | 2 +- test/extension/launch-wiring.test.ts | 58 ++++++++++++++ test/extension/launch.ts | 58 ++++++++++++-- test/extension/temp-dir-choke-point.test.ts | 80 ++++++++++++++++--- test/extension/temp-root.test.ts | 41 +++++++++- test/extension/temp-root.ts | 43 +++++++--- 8 files changed, 253 insertions(+), 33 deletions(-) create mode 100644 test/extension/launch-wiring.test.ts diff --git a/test/extension/e2e/format-document-active-edge.test.ts b/test/extension/e2e/format-document-active-edge.test.ts index 82fdb491..b8d763b9 100644 --- a/test/extension/e2e/format-document-active-edge.test.ts +++ b/test/extension/e2e/format-document-active-edge.test.ts @@ -42,7 +42,7 @@ async function openTempQuoll( slug: string, previous: PanelControlsShape | null ): Promise<{ uri: vscode.Uri; file: string; panel: PanelControlsShape }> { - const dir = await makeTempDir(`fmtdoc-`); + const dir = await makeTempDir(`fmtdoc-${slug}`); const file = path.join(dir, `${slug}.md`); await fs.writeFile(file, content); const uri = vscode.Uri.file(file); diff --git a/test/extension/e2e/status-bar-active-edge.test.ts b/test/extension/e2e/status-bar-active-edge.test.ts index a8ba60bd..67b47580 100644 --- a/test/extension/e2e/status-bar-active-edge.test.ts +++ b/test/extension/e2e/status-bar-active-edge.test.ts @@ -48,7 +48,7 @@ async function openTempQuoll( previous: PanelControlsShape | null, openOptions?: vscode.TextDocumentShowOptions ): Promise<{ uri: vscode.Uri; file: string; panel: PanelControlsShape }> { - const dir = await makeTempDir(`sbar-`); + const dir = await makeTempDir(`sbar-${slug}`); const file = path.join(dir, `${slug}.md`); await fs.writeFile(file, content); const uri = vscode.Uri.file(file); diff --git a/test/extension/e2e/two-panel-config-caret.test.ts b/test/extension/e2e/two-panel-config-caret.test.ts index 036d0133..cf63da46 100644 --- a/test/extension/e2e/two-panel-config-caret.test.ts +++ b/test/extension/e2e/two-panel-config-caret.test.ts @@ -48,7 +48,7 @@ async function openTempQuoll( slug: string, previous: PanelControlsShape | null ): Promise<{ uri: vscode.Uri; file: string; panel: PanelControlsShape }> { - const dir = await makeTempDir(`2panel-`); + const dir = await makeTempDir(`2panel-${slug}`); const file = path.join(dir, `${slug}.md`); await fs.writeFile(file, content); const uri = vscode.Uri.file(file); diff --git a/test/extension/launch-wiring.test.ts b/test/extension/launch-wiring.test.ts new file mode 100644 index 00000000..5a6b5b77 --- /dev/null +++ b/test/extension/launch-wiring.test.ts @@ -0,0 +1,58 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { runE2E } from "./launch"; +import type { RunTempRoot } from "./temp-root"; + +// The reclaim this PR exists for lives in launch.ts's wiring, not in the seam: +// which env key carries the root, which dir VS Code is given, and that +// dispose() runs on BOTH the pass and the fail path. Restoring `process.exit(1)` +// in the catch (the conventional CLI idiom) would keep every other suite green +// while silently skipping the finally — so pin it here. +const fakeRoot = (): RunTempRoot & { dispose: ReturnType } => ({ + root: "/t/r", + userDataDir: "/t/r/ud", + workDir: "/t/r/w", + dispose: vi.fn(), +}); + +afterEach(() => { + process.exitCode = undefined; + vi.restoreAllMocks(); +}); + +describe("runE2E wiring", () => { + it("hands the run root to the host and the user-data dir to VS Code", async () => { + const run = fakeRoot(); + const runTests = vi.fn().mockResolvedValue(0); + await runE2E({ runTests, createRoot: () => run }); + + const opts = runTests.mock.calls[0][0]; + expect(opts.extensionTestsEnv).toEqual({ QUOLL_E2E_TEMP_ROOT: "/t/r" }); + expect(opts.launchArgs).toContain("--user-data-dir=/t/r/ud"); + expect(run.dispose).toHaveBeenCalledTimes(1); + expect(process.exitCode).toBeUndefined(); + }); + + it("still reclaims the root when the suite fails", async () => { + const run = fakeRoot(); + const runTests = vi.fn().mockRejectedValue(new Error("suite failed")); + vi.spyOn(console, "error").mockImplementation(() => undefined); + + await runE2E({ runTests, createRoot: () => run }); + + expect(run.dispose).toHaveBeenCalledTimes(1); + expect(process.exitCode).toBe(1); + }); + + it("fails the run loudly when the root cannot be reclaimed", async () => { + const run = fakeRoot(); + run.dispose.mockImplementation(() => { + throw new Error("EBUSY"); + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await runE2E({ runTests: vi.fn().mockResolvedValue(0), createRoot: () => run }); + + expect(process.exitCode).toBe(1); + expect(errorSpy.mock.calls[0][0]).toContain("/t/r"); + }); +}); diff --git a/test/extension/launch.ts b/test/extension/launch.ts index 605a123d..e4b171c9 100644 --- a/test/extension/launch.ts +++ b/test/extension/launch.ts @@ -18,19 +18,31 @@ const VS_CODE_VERSION = "1.94.0"; // starts; the previous module-load-time `existsSync` inside harness.ts // ran on every test file's first require and crashed the Electron // runner with no mocha context, which triaged as an activation bug. +// Throws rather than process.exit()ing: once main() owns a run root, exit() +// would skip the reclaim in its finally. Keeping every preflight on the throw +// path makes that safety positional-order-independent. function preflightFixturesDir(): void { // __dirname at runtime is `out/test-e2e/`. Resolve up to the repo // root then back into the source-controlled fixtures directory. const fixturesDir = path.resolve(__dirname, "../..", "test/extension/e2e/fixtures"); if (!fs.existsSync(fixturesDir)) { - console.error( + throw new Error( `[e2e] FIXTURES_DIR misresolved: ${fixturesDir} — tsconfig outDir may have changed` ); - process.exit(1); } } -async function main(): Promise { +/** Injectable seam so the wiring below — which env key carries the root, which + * dir VS Code gets, and that dispose() runs on BOTH the pass and fail paths — + * is unit-testable without downloading and spawning Electron. */ +export interface LaunchDeps { + runTests: typeof runTests; + createRoot: typeof createRunTempRoot; +} + +export async function runE2E( + deps: LaunchDeps = { runTests, createRoot: createRunTempRoot } +): Promise { preflightFixturesDir(); // VS Code creates a unix-domain IPC socket under user-data-dir; on @@ -45,12 +57,33 @@ async function main(): Promise { // whole tmp footprint. Before it, every run stranded its user-data dir // plus one dir per temp-file test, forever (+53 dirs per run measured // 2026-08-14, 2 544 accumulated). - const run = createRunTempRoot(); + const run = deps.createRoot(); + // Own SIGINT ourselves. @vscode/test-electron ends innerRunTests with + // `if (exitRequested && process.listenerCount('SIGINT') === 0) process.exit(1)` + // — so without a listener here, the FIRST Ctrl+C (graceful close, child exits + // 0) hard-exits this process from inside the await and the finally below + // never runs, stranding the whole root. Holding a listener keeps the exit on + // our side; a second Ctrl+C still force-closes through the library's own + // handler. SIGTERM has no graceful path there at all, so reclaim and re-exit. + let interrupted = false; + const onSigint = (): void => { + interrupted = true; // the library's ctrlc1 does the graceful child close + }; + process.on("SIGINT", onSigint); + const onSigterm = (): void => { + try { + run.dispose(); + } catch (err) { + console.error(`[e2e] failed to reclaim temp root ${run.root}:`, err); + } + process.exit(143); // 128 + SIGTERM + }; + process.once("SIGTERM", onSigterm); try { const extensionDevelopmentPath = path.resolve(__dirname, "../.."); const extensionTestsPath = path.resolve(__dirname, "./e2e/index"); - await runTests({ + await deps.runTests({ version: VS_CODE_VERSION, extensionDevelopmentPath, extensionTestsPath, @@ -63,6 +96,12 @@ async function main(): Promise { // strand the whole root on every failing run. process.exitCode = 1; } finally { + process.removeListener("SIGINT", onSigint); + process.removeListener("SIGTERM", onSigterm); + if (interrupted) { + // An interrupted run is not a pass — without this it would exit 0. + process.exitCode ??= 130; // 128 + SIGINT + } try { run.dispose(); } catch (err) { @@ -73,4 +112,11 @@ async function main(): Promise { } } -void main(); +// `require.main === module` holds only under `node out/test-e2e/launch.js`, so +// the wiring test can import runE2E without spawning anything. +if (require.main === module) { + void runE2E().catch((err) => { + console.error(err instanceof Error ? err.message : err); + process.exitCode = 1; + }); +} diff --git a/test/extension/temp-dir-choke-point.test.ts b/test/extension/temp-dir-choke-point.test.ts index e8a6c2e4..647bb729 100644 --- a/test/extension/temp-dir-choke-point.test.ts +++ b/test/extension/temp-dir-choke-point.test.ts @@ -40,12 +40,51 @@ function calleeName(node: ts.CallExpression): string { if (ts.isPropertyAccessExpression(target)) { return target.name.text; } + // fs["mkdtempSync"](…) — an element access is still a call to the same thing. + if (ts.isElementAccessExpression(target) && ts.isStringLiteralLike(target.argumentExpression)) { + return target.argumentExpression.text; + } if (ts.isIdentifier(target)) { return target.text; } return ""; } +/** BANNED plus whatever local names those symbols were imported AS — without + * this, `import { mkdtemp as mkTmp }` walks straight past the guard, and that + * rename is something an import-organiser can introduce mechanically. */ +function bannedNamesIn(source: ts.SourceFile): Set { + const names = new Set(BANNED); + source.forEachChild((node) => { + if (!ts.isImportDeclaration(node)) { + return; + } + const named = node.importClause?.namedBindings; + if (named && ts.isNamedImports(named)) { + for (const spec of named.elements) { + if (BANNED.has((spec.propertyName ?? spec.name).text)) { + names.add(spec.name.text); + } + } + } + }); + return names; +} + +function findOffenders(source: ts.SourceFile, label: string): string[] { + const banned = bannedNamesIn(source); + const offenders: string[] = []; + const visit = (node: ts.Node): void => { + if (ts.isCallExpression(node) && banned.has(calleeName(node))) { + const { line } = source.getLineAndCharacterOfPosition(node.getStart(source)); + offenders.push(`${label}:${line + 1} ${calleeName(node)}()`); + } + ts.forEachChild(node, visit); + }; + visit(source); + return offenders; +} + describe("e2e temp-dir choke point", () => { const sources = collectSources(SCAN_ROOT).filter((file) => file !== ALLOCATION_SITE); @@ -66,22 +105,39 @@ describe("e2e temp-dir choke point", () => { ts.ScriptTarget.ES2022, true ); - const visit = (node: ts.Node): void => { - if (ts.isCallExpression(node) && BANNED.has(calleeName(node))) { - const name = calleeName(node); - const exempt = name === "tmpdir" && TMPDIR_REFERENCE_ALLOWED.has(file); - if (!exempt) { - const { line } = source.getLineAndCharacterOfPosition(node.getStart(source)); - offenders.push(`${path.relative(SCAN_ROOT, file)}:${line + 1} ${name}()`); - } - } - ts.forEachChild(node, visit); - }; - visit(source); + const found = findOffenders(source, path.relative(SCAN_ROOT, file)); + // The seam's own test may NAME os.tmpdir() to pin where the root lands; + // it still may not allocate, so only `tmpdir` is exempted there. + offenders.push( + ...(TMPDIR_REFERENCE_ALLOWED.has(file) + ? found.filter((entry) => !entry.endsWith("tmpdir()")) + : found) + ); } expect( offenders, "use makeTempDir/makeTempDirSync from ./harness — temp-root.ts is the only allocation site" ).toEqual([]); }); + + it("catches the bypasses a rename or an indexed call would open", () => { + // Exercises the branches above, which no real source file currently hits — + // without this they would be dead code that silently stops working. + const probe = ts.createSourceFile( + "probe.ts", + [ + 'import { mkdtemp as mkTmp } from "node:fs/promises";', + 'import * as os from "node:os";', + 'await mkTmp(os.tmpdir() + "/x-");', + 'fs["mkdtempSync"]("/tmp/y-");', + ].join("\n"), + ts.ScriptTarget.ES2022, + true + ); + expect(findOffenders(probe, "probe.ts")).toEqual([ + "probe.ts:3 mkTmp()", + "probe.ts:3 tmpdir()", + "probe.ts:4 mkdtempSync()", + ]); + }); }); diff --git a/test/extension/temp-root.test.ts b/test/extension/temp-root.test.ts index 1ce7af88..b8dcf481 100644 --- a/test/extension/temp-root.test.ts +++ b/test/extension/temp-root.test.ts @@ -39,14 +39,39 @@ describe("createRunTempRoot", () => { expect(fs.existsSync(a.workDir)).toBe(true); }); - it("keeps the user-data path within the macOS socket budget", () => { + it("keeps the code's own contribution inside the macOS socket budget", () => { // VS Code opens its IPC socket under --user-data-dir and macOS caps // those paths at 103 chars; an overlong path fails the launch, not a - // test, so pin the budget where a rename would trip it. + // test. Measure only the part this module controls — asserting the + // absolute length would pass on CI (where $TMPDIR is `/tmp`, 56 chars + // of slack) and so would never catch a rename of the segments. const run = newRoot(); - expect(run.userDataDir.length).toBeLessThanOrEqual(80); + const owned = run.userDataDir.slice(fs.realpathSync(os.tmpdir()).length); + expect(owned).toBe(`${path.sep}${path.basename(run.root)}${path.sep}ud`); + expect(owned.length).toBeLessThanOrEqual(24); }); + it.skipIf(process.getuid?.() === 0)( + "throws when the filesystem refuses, rather than leaking silently", + () => { + // The other half of the dispose contract. `not.toThrow()` on an + // already-removed root is satisfied by `force: true` alone, so a + // dispose() that swallowed everything would pass it — this case is what + // distinguishes "loud on unreclaimable" from "silently gives up". + const run = newRoot(); + const locked = path.join(run.workDir, "locked"); + fs.mkdirSync(locked); + fs.writeFileSync(path.join(locked, "f.md"), "x"); + fs.chmodSync(locked, 0o500); + try { + expect(() => run.dispose()).toThrow(); + } finally { + fs.chmodSync(locked, 0o700); // afterEach must still be able to reclaim + } + }, + 15000 // dispose retries with linear backoff before giving up + ); + it("reclaims a non-empty root", () => { const run = newRoot(); fs.writeFileSync(path.join(run.workDir, "leftover.md"), "# x\n"); @@ -95,4 +120,14 @@ describe("makeTempDir", () => { run.dispose(); expect(fs.existsSync(first)).toBe(false); }); + + it("refuses a slug that would escape the run's work dir", async () => { + // `path.join` normalises `../`, so a separator in a slug lands the dir + // outside the root — where nothing disposes it. The choke-point test + // governs who allocates; this governs where the result lands. + const run = newRoot(); + process.env[RUN_TEMP_ROOT_ENV] = run.root; + await expect(makeTempDir("../escape")).rejects.toThrow(/bare name/); + expect(() => makeTempDirSync("a/b")).toThrow(/bare name/); + }); }); diff --git a/test/extension/temp-root.ts b/test/extension/temp-root.ts index eb48c719..0691d714 100644 --- a/test/extension/temp-root.ts +++ b/test/extension/temp-root.ts @@ -20,11 +20,11 @@ const WORK_SEGMENT = "w"; export interface RunTempRoot { /** The only directory under os.tmpdir() this run may write to. */ - root: string; + readonly root: string; /** VS Code --user-data-dir. */ - userDataDir: string; + readonly userDataDir: string; /** Parent of every per-test workspace dir the suites allocate. */ - workDir: string; + readonly workDir: string; /** Remove the whole root. Safe to call twice; THROWS if the filesystem * refuses (EBUSY / EACCES) — a leak we cannot reclaim must be loud. */ dispose(): void; @@ -40,7 +40,14 @@ export function createRunTempRoot(): RunTempRoot { } catch (err) { // Partial init still owns the root: reclaim it before the caller ever // holds a handle, otherwise the throw leaks the dir we just created. - fs.rmSync(root, { recursive: true, force: true }); + // The rollback must never become the reported failure — whatever broke + // mkdir is the diagnosable cause (a read-only or full $TMPDIR breaks + // both), so log a failed rollback and rethrow the original. + try { + fs.rmSync(root, { recursive: true, force: true }); + } catch (cleanupErr) { + console.error(`[e2e] failed to reclaim partially-created temp root ${root}:`, cleanupErr); + } throw err; } return { @@ -48,7 +55,12 @@ export function createRunTempRoot(): RunTempRoot { userDataDir, workDir, dispose(): void { - fs.rmSync(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); + // ~3s of retries (linear backoff: 200, 400, … 1000). dispose() fires the + // instant the Electron main process closes, while VS Code's helpers may + // still be writing under user-data-dir. An ENOTEMPTY there is a race, + // not a leak, and must not flip a green run red; a throw past this + // budget means genuinely unreclaimable, which is worth failing for. + fs.rmSync(root, { recursive: true, force: true, maxRetries: 5, retryDelay: 200 }); }, }; } @@ -66,13 +78,26 @@ export function resolveRunTempWorkDir(env: NodeJS.ProcessEnv = process.env): str return path.join(root, WORK_SEGMENT); } +/** Reject anything that could climb out of the work dir. `path.join` + * normalises `../`, so a slug carrying a separator would land a dir OUTSIDE + * the run root — where nothing disposes it. The choke-point test governs who + * allocates; this governs where the result lands. */ +function assertSlug(slug: string): string { + if (!/^[a-z0-9][a-z0-9-]*$/i.test(slug)) { + throw new Error(`temp-dir slug must be a bare name, got ${JSON.stringify(slug)}`); + } + return slug; +} + /** Allocate a per-test workspace dir under the run root. This is the ONLY * sanctioned temp-dir allocation in the E2E tree — enforced by - * test/extension/temp-dir-choke-point.test.ts. */ -export function makeTempDir(slug: string): Promise { - return fsp.mkdtemp(path.join(resolveRunTempWorkDir(), `${slug}-`)); + * test/extension/temp-dir-choke-point.test.ts. + * `async` is load-bearing: `resolveRunTempWorkDir` / `assertSlug` throw + * synchronously, and a declared `Promise` must deliver that as a rejection. */ +export async function makeTempDir(slug: string): Promise { + return fsp.mkdtemp(path.join(resolveRunTempWorkDir(), `${assertSlug(slug)}-`)); } export function makeTempDirSync(slug: string): string { - return fs.mkdtempSync(path.join(resolveRunTempWorkDir(), `${slug}-`)); + return fs.mkdtempSync(path.join(resolveRunTempWorkDir(), `${assertSlug(slug)}-`)); } From 3c56d60d09253cd9ef9219cdf2f3f0f8db44ba63 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Fri, 14 Aug 2026 11:52:02 +1000 Subject: [PATCH 3/4] fix(e2e): stop realpath'ing os.tmpdir(), and drop the SIGTERM handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`. --- test/extension/temp-root.test.ts | 7 +++++-- test/extension/temp-root.ts | 7 ++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/test/extension/temp-root.test.ts b/test/extension/temp-root.test.ts index b8dcf481..798683b6 100644 --- a/test/extension/temp-root.test.ts +++ b/test/extension/temp-root.test.ts @@ -31,7 +31,10 @@ describe("createRunTempRoot", () => { const a = newRoot(); const b = newRoot(); expect(a.root).not.toBe(b.root); - expect(path.dirname(a.root)).toBe(fs.realpathSync(os.tmpdir())); + // os.tmpdir() unresolved — the module must NOT realpath it. On macOS the + // resolved form (`/private/var/…`) reaches VS Code as a document URI and + // breaks the disk-conflict watcher's exact-string event filter. + expect(path.dirname(a.root)).toBe(os.tmpdir()); expect(path.basename(a.root).startsWith("quoll-e2e-")).toBe(true); expect(path.dirname(a.userDataDir)).toBe(a.root); expect(path.dirname(a.workDir)).toBe(a.root); @@ -46,7 +49,7 @@ describe("createRunTempRoot", () => { // absolute length would pass on CI (where $TMPDIR is `/tmp`, 56 chars // of slack) and so would never catch a rename of the segments. const run = newRoot(); - const owned = run.userDataDir.slice(fs.realpathSync(os.tmpdir()).length); + const owned = run.userDataDir.slice(os.tmpdir().length); expect(owned).toBe(`${path.sep}${path.basename(run.root)}${path.sep}ud`); expect(owned.length).toBeLessThanOrEqual(24); }); diff --git a/test/extension/temp-root.ts b/test/extension/temp-root.ts index 0691d714..2ad20c41 100644 --- a/test/extension/temp-root.ts +++ b/test/extension/temp-root.ts @@ -31,7 +31,12 @@ export interface RunTempRoot { } export function createRunTempRoot(): RunTempRoot { - const root = fs.mkdtempSync(path.join(fs.realpathSync(os.tmpdir()), "quoll-e2e-")); + // os.tmpdir() as-is, NOT realpath'd. On macOS it returns the `/var/folders/…` + // symlink whose target is `/private/var/folders/…`, and the paths built here + // become document URIs inside VS Code. The disk-conflict watcher filters + // events by exact URI string, so handing it the resolved form makes those + // comparisons miss and the dirty-doc-disk-conflict suite time out. + const root = fs.mkdtempSync(path.join(os.tmpdir(), "quoll-e2e-")); const userDataDir = path.join(root, USER_DATA_SEGMENT); const workDir = path.join(root, WORK_SEGMENT); try { From 08109201e7ece4c88a556ee7e39ec3b39702702b Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Fri, 14 Aug 2026 11:59:23 +1000 Subject: [PATCH 4/4] test(e2e): pin the signal wiring, the preflight order, and the rollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⚠️ 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. --- test/extension/launch-wiring.test.ts | 99 ++++++++++++++++++- test/extension/launch.ts | 52 ++++++---- test/extension/temp-dir-choke-point.test.ts | 13 +++ test/extension/temp-root-partial-init.test.ts | 44 +++++++++ 4 files changed, 188 insertions(+), 20 deletions(-) create mode 100644 test/extension/temp-root-partial-init.test.ts diff --git a/test/extension/launch-wiring.test.ts b/test/extension/launch-wiring.test.ts index 5a6b5b77..c6fc9e80 100644 --- a/test/extension/launch-wiring.test.ts +++ b/test/extension/launch-wiring.test.ts @@ -14,6 +14,8 @@ const fakeRoot = (): RunTempRoot & { dispose: ReturnType } => ({ dispose: vi.fn(), }); +const noopPreflight = (): void => undefined; + afterEach(() => { process.exitCode = undefined; vi.restoreAllMocks(); @@ -23,7 +25,7 @@ describe("runE2E wiring", () => { it("hands the run root to the host and the user-data dir to VS Code", async () => { const run = fakeRoot(); const runTests = vi.fn().mockResolvedValue(0); - await runE2E({ runTests, createRoot: () => run }); + await runE2E({ runTests, createRoot: () => run, preflight: noopPreflight }); const opts = runTests.mock.calls[0][0]; expect(opts.extensionTestsEnv).toEqual({ QUOLL_E2E_TEMP_ROOT: "/t/r" }); @@ -37,7 +39,7 @@ describe("runE2E wiring", () => { const runTests = vi.fn().mockRejectedValue(new Error("suite failed")); vi.spyOn(console, "error").mockImplementation(() => undefined); - await runE2E({ runTests, createRoot: () => run }); + await runE2E({ runTests, createRoot: () => run, preflight: noopPreflight }); expect(run.dispose).toHaveBeenCalledTimes(1); expect(process.exitCode).toBe(1); @@ -50,9 +52,100 @@ describe("runE2E wiring", () => { }); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); - await runE2E({ runTests: vi.fn().mockResolvedValue(0), createRoot: () => run }); + await runE2E({ + runTests: vi.fn().mockResolvedValue(0), + createRoot: () => run, + preflight: noopPreflight, + }); expect(process.exitCode).toBe(1); expect(errorSpy.mock.calls[0][0]).toContain("/t/r"); }); + + it("never creates a root it would have no finally to reclaim", async () => { + // The preflight must run BEFORE createRoot: a throw from after it escapes + // with no finally, stranding the root. Ordering, not just the throw. + const createRoot = vi.fn(); + const preflight = vi.fn(() => { + throw new Error("[e2e] FIXTURES_DIR misresolved"); + }); + await expect(runE2E({ runTests: vi.fn(), createRoot, preflight })).rejects.toThrow( + /FIXTURES_DIR/ + ); + expect(createRoot).not.toHaveBeenCalled(); + }); +}); + +describe("runE2E signal wiring", () => { + it("stays passive while the library owns SIGINT, then reclaims and exits 130", async () => { + // Post-spawn, @vscode/test-electron's ctrlc1 is also listening (simulated + // here). We must NOT reclaim mid-run — the library is still gracefully + // closing the child — but the run must not report a pass either. + const run = fakeRoot(); + const before = process.listenerCount("SIGINT"); + const libraryHandler = (): void => undefined; + let duringRun = 0; + const runTests = vi.fn().mockImplementation(async () => { + // ctrlc1 is registered inside innerRunTests, i.e. only once the child has + // spawned — so it appears AFTER ours, which is what the baseline check + // detects. + process.on("SIGINT", libraryHandler); + duringRun = process.listenerCount("SIGINT"); + process.emit("SIGINT"); + expect(run.dispose).not.toHaveBeenCalled(); // passive: the finally reclaims + process.removeListener("SIGINT", libraryHandler); + return 0; + }); + + await runE2E({ runTests, createRoot: () => run, preflight: noopPreflight }); + + expect(duringRun).toBe(before + 2); + expect(run.dispose).toHaveBeenCalledTimes(1); + expect(process.exitCode).toBe(130); + expect(process.listenerCount("SIGINT")).toBe(before); + }); + + it("reclaims and exits itself when nothing else is listening (pre-spawn Ctrl+C)", async () => { + // Before the child spawns there is no ctrlc1, and merely holding a listener + // suppresses Node's default terminate — so Ctrl+C would otherwise do + // nothing at all for the whole download window. + const run = fakeRoot(); + const exit = vi.spyOn(process, "exit").mockImplementation((() => undefined) as never); + const runTests = vi.fn().mockImplementation(async () => { + process.emit("SIGINT"); + return 0; + }); + + await runE2E({ runTests, createRoot: () => run, preflight: noopPreflight }); + + expect(exit).toHaveBeenCalledWith(130); + // Before the exit, not merely "at some point" — the finally would satisfy that. + expect(run.dispose.mock.invocationCallOrder[0]).toBeLessThan(exit.mock.invocationCallOrder[0]); + }); + + it("does not downgrade a real failure to 130", async () => { + const run = fakeRoot(); + const libraryHandler = (): void => undefined; + vi.spyOn(console, "error").mockImplementation(() => undefined); + const runTests = vi.fn().mockImplementation(async () => { + process.on("SIGINT", libraryHandler); + process.emit("SIGINT"); + process.removeListener("SIGINT", libraryHandler); + throw new Error("suite failed"); + }); + + await runE2E({ runTests, createRoot: () => run, preflight: noopPreflight }); + + expect(process.exitCode).toBe(1); // the `??=`, not `=` + }); + + it("leaves no signal listeners behind on the happy path", async () => { + const before = process.listenerCount("SIGINT"); + await runE2E({ + runTests: vi.fn().mockResolvedValue(0), + createRoot: fakeRoot, + preflight: noopPreflight, + }); + expect(process.listenerCount("SIGINT")).toBe(before); + }); }); diff --git a/test/extension/launch.ts b/test/extension/launch.ts index e4b171c9..7d480ae7 100644 --- a/test/extension/launch.ts +++ b/test/extension/launch.ts @@ -18,9 +18,11 @@ const VS_CODE_VERSION = "1.94.0"; // starts; the previous module-load-time `existsSync` inside harness.ts // ran on every test file's first require and crashed the Electron // runner with no mocha context, which triaged as an activation bug. -// Throws rather than process.exit()ing: once main() owns a run root, exit() -// would skip the reclaim in its finally. Keeping every preflight on the throw -// path makes that safety positional-order-independent. +// Throws rather than process.exit()ing, and runs BEFORE the run root exists. +// Both halves matter: exit() would skip the reclaim in the finally, and a throw +// from after createRoot() escapes with no finally to reclaim either. The +// ordering is load-bearing, so it rides the LaunchDeps seam and is pinned by +// launch-wiring.test.ts rather than left to a comment. function preflightFixturesDir(): void { // __dirname at runtime is `out/test-e2e/`. Resolve up to the repo // root then back into the source-controlled fixtures directory. @@ -38,12 +40,13 @@ function preflightFixturesDir(): void { export interface LaunchDeps { runTests: typeof runTests; createRoot: typeof createRunTempRoot; + preflight: typeof preflightFixturesDir; } export async function runE2E( - deps: LaunchDeps = { runTests, createRoot: createRunTempRoot } + deps: LaunchDeps = { runTests, createRoot: createRunTempRoot, preflight: preflightFixturesDir } ): Promise { - preflightFixturesDir(); + deps.preflight(); // VS Code creates a unix-domain IPC socket under user-data-dir; on // macOS the socket path must fit in 103 chars. The repo path under @@ -64,21 +67,37 @@ export async function runE2E( // 0) hard-exits this process from inside the await and the finally below // never runs, stranding the whole root. Holding a listener keeps the exit on // our side; a second Ctrl+C still force-closes through the library's own - // handler. SIGTERM has no graceful path there at all, so reclaim and re-exit. + // handler. + // + // Baseline rather than a bare count: "has the library taken over?" means "did + // a listener appear after ours", which reads correctly in a bare `node` run + // (0 → 1) and under any host that already listens. + const sigintListenersBefore = process.listenerCount("SIGINT"); let interrupted = false; const onSigint = (): void => { - interrupted = true; // the library's ctrlc1 does the graceful child close - }; - process.on("SIGINT", onSigint); - const onSigterm = (): void => { - try { - run.dispose(); - } catch (err) { - console.error(`[e2e] failed to reclaim temp root ${run.root}:`, err); + interrupted = true; + // Post-spawn, the library's ctrlc1 has appeared and does the graceful child + // close, so we stay passive and reclaim in the finally. Pre-spawn — during + // downloadAndUnzipVSCode, minutes on a cold cache — ctrlc1 does not exist + // yet, and merely holding this listener suppresses Node's default + // termination. Without this branch Ctrl+C would be a dead key for that + // whole window. + if (process.listenerCount("SIGINT") <= sigintListenersBefore + 1) { + try { + run.dispose(); + } catch (err) { + console.error(`[e2e] failed to reclaim temp root ${run.root}:`, err); + } + process.exit(130); // 128 + SIGINT } - process.exit(143); // 128 + SIGTERM }; - process.once("SIGTERM", onSigterm); + process.on("SIGINT", onSigint); + // SIGTERM is deliberately NOT handled. The library exposes no child pid and + // no graceful stop, so a handler could only reclaim the root while VS Code + // may still be running — and a plain `kill` of this process 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. An aborted + // run keeping its own single root is the accepted outcome. try { const extensionDevelopmentPath = path.resolve(__dirname, "../.."); const extensionTestsPath = path.resolve(__dirname, "./e2e/index"); @@ -97,7 +116,6 @@ export async function runE2E( process.exitCode = 1; } finally { process.removeListener("SIGINT", onSigint); - process.removeListener("SIGTERM", onSigterm); if (interrupted) { // An interrupted run is not a pass — without this it would exit 0. process.exitCode ??= 130; // 128 + SIGINT diff --git a/test/extension/temp-dir-choke-point.test.ts b/test/extension/temp-dir-choke-point.test.ts index 647bb729..13a3f09a 100644 --- a/test/extension/temp-dir-choke-point.test.ts +++ b/test/extension/temp-dir-choke-point.test.ts @@ -139,5 +139,18 @@ describe("e2e temp-dir choke point", () => { "probe.ts:3 tmpdir()", "probe.ts:4 mkdtempSync()", ]); + + // Banning `tmpdir` is what stops a hand-rolled + // `fs.mkdirSync(path.join(os.tmpdir(), …))` — `mkdirSync` is deliberately + // not in BANNED — so the exemption must stay scoped to the seam's own + // test. Widening it tree-wide is otherwise a green edit. + const tmpdirProbe = ts.createSourceFile( + "probe.ts", + 'import * as os from "node:os";\nfs.mkdirSync(os.tmpdir() + "/x");', + ts.ScriptTarget.ES2022, + true + ); + expect(findOffenders(tmpdirProbe, "probe.ts")).toEqual(["probe.ts:2 tmpdir()"]); + expect([...TMPDIR_REFERENCE_ALLOWED]).toEqual([path.join(SCAN_ROOT, "temp-root.test.ts")]); }); }); diff --git a/test/extension/temp-root-partial-init.test.ts b/test/extension/temp-root-partial-init.test.ts new file mode 100644 index 00000000..9c4f5d94 --- /dev/null +++ b/test/extension/temp-root-partial-init.test.ts @@ -0,0 +1,44 @@ +import * as path from "node:path"; +import { describe, expect, it, vi } from "vitest"; + +// The rollback that reclaims a half-built root is the one branch whose whole +// job is "do not leak", in a module whose whole purpose is not leaking — so it +// gets a test even though the obvious route fails. +// +// `vi.spyOn(fs, "mkdirSync")` throws "Cannot redefine property": the node:fs +// namespace exposes non-configurable properties. `vi.mock` redefines nothing — +// it swaps the module-registry entry the module under test imports — so the +// path IS reachable. Own file, because the mock is file-scoped and would +// otherwise apply to every case in temp-root.test.ts. +const attempted: string[] = []; + +vi.mock("node:fs", async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + default: real, + mkdirSync: (...args: Parameters) => { + const target = String(args[0]); + attempted.push(target); + if (target.endsWith(`${path.sep}ud`)) { + throw new Error("ENOSPC: synthetic"); + } + return real.mkdirSync(...args); + }, + }; +}); + +const realFs = await vi.importActual("node:fs"); +const { createRunTempRoot } = await import("./temp-root"); + +describe("createRunTempRoot partial init", () => { + it("reclaims the half-built root and rethrows the original cause", () => { + // The rollback's own failure must never mask the diagnosable one. + expect(() => createRunTempRoot()).toThrow(/ENOSPC: synthetic/); + // Named exactly rather than diffed from a directory listing, so a parallel + // run cannot flake this. + const root = path.dirname(attempted[0]); + expect(path.basename(root).startsWith("quoll-e2e-")).toBe(true); + expect(realFs.existsSync(root)).toBe(false); + }); +});