From 58ed88dd276341a7c9fd165ff440a1f235131816 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Thu, 22 Oct 2026 04:56:26 -0400 Subject: [PATCH 1/3] test: fail and contain process-state leaks between test files Every file in `bun test` shares one Bun process. A test that changed PATH, HOME, HACK_HOME or cwd and then failed before its restore leaked that change into every later file, which then failed with "Executable not found in $PATH: git/bun" and hid the first failure (HACK-1327). - Preload tests/helpers/global-state-guard-preload.ts from bunfig.toml. It snapshots the environment, cwd, umask, exit code, guarded global functions and process listeners around every test and at every file boundary, fails the leaking test or the run with a value-free message naming the file, and restores the environment, cwd, umask and exit code so later files run clean. Replaced functions and listeners are reported but kept, because they may still sandbox a timed-out test's unsettled work. - Fix leaks it and a review found: fzf replaced the process.env object; the VM-file, retained-build and retained-source-bind helpers could fail setup after changing PATH/HOME with no restore registered; the retained routing and retained-file fixtures kept HACK_HOME/PATH on the fixture when their lifetime was unknown; the protected-files negative left a process.kill spy for later files. Refs HACK-1327 Co-Authored-By: Claude Opus 5.5 --- .ai/skills/hack-repo-verify/SKILL.md | 9 + bunfig.toml | 3 + tests/fzf.test.ts | 12 +- tests/global-state-guard.test.ts | 221 ++++++++++++++ tests/helpers/global-state-guard-preload.ts | 119 ++++++++ tests/helpers/global-state-guard.ts | 285 ++++++++++++++++++ tests/helpers/native-compose-vm-files.ts | 24 +- tests/helpers/retained-build-adoption.ts | 9 +- tests/helpers/retained-routing-adoption.ts | 10 +- .../helpers/retained-source-bind-adoption.ts | 9 +- ...e-compose-adoption-file-generation.test.ts | 10 +- ...ompose-adoption-routing-generation.test.ts | 33 +- ...ive-config-protected-files-fixture.test.ts | 6 +- 13 files changed, 736 insertions(+), 14 deletions(-) create mode 100644 tests/global-state-guard.test.ts create mode 100644 tests/helpers/global-state-guard-preload.ts create mode 100644 tests/helpers/global-state-guard.ts diff --git a/.ai/skills/hack-repo-verify/SKILL.md b/.ai/skills/hack-repo-verify/SKILL.md index bd075c080..33530f151 100644 --- a/.ai/skills/hack-repo-verify/SKILL.md +++ b/.ai/skills/hack-repo-verify/SKILL.md @@ -63,6 +63,15 @@ Host and container `dist` remain separate. This checks compatibility, not exact source provenance. Raw `bun test ` remains available for focused tests; it bypasses the suite preflight and does not prepare compiler-dependent fixtures. +All test files share one Bun process. `bunfig.toml` preloads +[the global-state guard](../../../tests/helpers/global-state-guard-preload.ts) for +every `bun test` run: a test or file that leaves the environment, cwd, umask, +exit code, a guarded global function or a process listener changed fails with a +`[global-state-guard]` message naming the file. The guard restores the +environment, cwd, umask and exit code so later files run clean; it keeps +replaced functions and listeners, which may still protect unsettled work. Fix +the named file's restore (`try`/`finally` or `afterEach`), not the later failures. + For release workflow edits, run the pinned syntax/context check also enforced by CI: `mise x actionlint@1.7.12 -- actionlint -shellcheck= -pyflakes= .github/workflows/ci.yml .github/workflows/release.yml .github/workflows/prerelease.yml .github/workflows/bun-crash-experiment.yml`. This validates event globs and reusable workflow contexts; Bun YAML parsing alone does not. diff --git a/bunfig.toml b/bunfig.toml index 98123b590..ad2273f52 100644 --- a/bunfig.toml +++ b/bunfig.toml @@ -1,2 +1,5 @@ [test] root = "./tests" +# Fails a test or file that leaves PATH, HOME, cwd or other process state +# changed, and restores it so later files run clean (HACK-1327). +preload = ["./tests/helpers/global-state-guard-preload.ts"] diff --git a/tests/fzf.test.ts b/tests/fzf.test.ts index f44e7e125..6aaa28c3d 100644 --- a/tests/fzf.test.ts +++ b/tests/fzf.test.ts @@ -9,16 +9,24 @@ import { isFzfAvailable, resetFzfPathCacheForTests, } from "../src/ui/fzf.ts"; +import { restoreEnv } from "./helpers/env.ts"; + +const TOUCHED_ENV = ["HACK_FZF_PATH", "HACK_HOME", "HOME", "PATH"] as const; describe("fzf", () => { - const originalEnv = { ...process.env }; + let savedEnv: Map = new Map(); beforeEach(() => { + // Edit the live process.env in place and restore only the keys these + // cases touch; replacing the object would detach it from Bun.env. + savedEnv = new Map(TOUCHED_ENV.map((key) => [key, process.env[key]])); resetFzfPathCacheForTests(); }); afterEach(() => { - process.env = { ...originalEnv }; + for (const [key, value] of savedEnv) { + restoreEnv(key, value); + } resetFzfPathCacheForTests(); }); diff --git a/tests/global-state-guard.test.ts b/tests/global-state-guard.test.ts new file mode 100644 index 000000000..f7cdad5e2 --- /dev/null +++ b/tests/global-state-guard.test.ts @@ -0,0 +1,221 @@ +import { afterEach, expect, test } from "bun:test"; +import { + chmod, + mkdir, + mkdtemp, + realpath, + rm, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; + +import { restoreEnv } from "./helpers/env.ts"; +import { + captureGlobalState, + diffGlobalState, + restoreGlobalState, +} from "./helpers/global-state-guard.ts"; + +const PRELOAD = resolve( + import.meta.dir, + "helpers/global-state-guard-preload.ts" +); +const KEY = "HACK_GLOBAL_STATE_GUARD_TEST"; +const owned: string[] = []; + +afterEach(async () => { + await Promise.all( + owned.splice(0).map((path) => rm(path, { recursive: true, force: true })) + ); +}); + +async function tempDir(): Promise { + const dir = await realpath( + await mkdtemp(join(tmpdir(), "hack-global-state-guard-")) + ); + owned.push(dir); + return dir; +} + +test("diff names added, changed and removed env keys and a cwd change, without values", async () => { + const dir = await tempDir(); + const before = captureGlobalState(); + const savedPath = process.env.PATH; + const savedKey = process.env[KEY]; + const cwd = process.cwd(); + try { + process.env[KEY] = "secret-shaped-value"; + process.env.PATH = "/nonexistent"; + process.chdir(dir); + const changes = diffGlobalState(before); + expect(changes).toContain("cwd changed"); + expect(changes).toContain( + `env ${KEY} ${savedKey === undefined ? "added" : "changed"}` + ); + expect(changes).toContain("env PATH changed"); + expect(changes.join("\n")).not.toContain("secret-shaped-value"); + expect(changes.join("\n")).not.toContain("/nonexistent"); + expect( + diffGlobalState(before, captureGlobalState(), { + allowedEnvChanges: new Map([[KEY, "test allowlist"]]), + }) + ).not.toContain(`env ${KEY} added`); + } finally { + process.chdir(cwd); + restoreEnv("PATH", savedPath); + restoreEnv(KEY, savedKey); + } + expect(diffGlobalState(before)).toEqual([]); +}); + +test("restore puts back env keys, the env object, cwd and exitCode in place", async () => { + const dir = await tempDir(); + const before = captureGlobalState(); + const envObject = process.env; + const savedPath = process.env.PATH; + const savedHome = process.env.HOME; + const cwd = process.cwd(); + const exitCode = process.exitCode; + try { + process.env.PATH = "/nonexistent"; + Reflect.deleteProperty(process.env, "HOME"); + process.env[KEY] = "1"; + process.chdir(dir); + process.exitCode = 3; + process.env = { PATH: "/replaced" }; + expect(diffGlobalState(before)).toContain("process.env object replaced"); + + restoreGlobalState(before); + + expect(process.env).toBe(envObject); + expect(process.env.PATH).toBe(savedPath); + expect(process.env.HOME).toBe(savedHome); + expect(process.env[KEY]).toBeUndefined(); + expect(process.cwd()).toBe(cwd); + expect(process.exitCode ?? 0).toBe(exitCode ?? 0); + expect(diffGlobalState(before)).toEqual([]); + } finally { + process.env = envObject; + process.chdir(cwd); + process.exitCode = exitCode ?? 0; + restoreEnv("PATH", savedPath); + restoreEnv("HOME", savedHome); + restoreEnv(KEY, undefined); + } +}); + +test("a replaced function or listener is reported but left for its owner", () => { + const before = captureGlobalState(); + const fetchDescriptor = Object.getOwnPropertyDescriptor(globalThis, "fetch"); + const listener = () => undefined; + const fake = (() => + Promise.reject(new Error("refused"))) as unknown as typeof fetch; + try { + globalThis.fetch = fake; + process.on("SIGUSR2", listener); + const changes = diffGlobalState(before); + expect(changes).toContain("globalThis.fetch replaced"); + expect(changes).toContain("process 'SIGUSR2' listeners changed"); + restoreGlobalState(before); + expect(globalThis.fetch).toBe(fake); + expect(process.listeners("SIGUSR2")).toContain(listener); + } finally { + process.removeListener("SIGUSR2", listener); + if (fetchDescriptor) { + Object.defineProperty(globalThis, "fetch", fetchDescriptor); + } + } + expect(diffGlobalState(before)).toEqual([]); +}); + +const LEAK_IN_TEST = `import { test } from "bun:test"; +test("sets PATH and fails before restoring", () => { + const saved = process.env.PATH; + process.env.PATH = "/nonexistent"; + if (saved !== "never") throw new Error("early failure before restore"); + process.env.PATH = saved; +}); +`; + +const LEAK_IN_HOOKS = `import { beforeAll, test } from "bun:test"; +beforeAll(() => { + process.env.HACK_HOME = "/nonexistent/hack-home"; + process.chdir("/"); +}); +test("passes but never restores", () => {}); +`; + +const LATER = `import { expect, test } from "bun:test"; +test("a later file still finds executables on PATH", () => { + const child = Bun.spawnSync(["hack-guard-probe"], { env: { ...process.env } }); + expect(child.exitCode).toBe(0); + expect(process.env.HACK_HOME).toBeUndefined(); + console.log("LATER-FILE-PASSED"); +}); +`; + +async function runNestedSuite(opts: { + readonly preload: boolean; +}): Promise<{ readonly code: number; readonly output: string }> { + const dir = await tempDir(); + const bin = join(dir, "bin"); + const home = join(dir, "home"); + const suite = join(dir, "suite"); + await mkdir(bin); + await mkdir(home); + await mkdir(suite); + await writeFile(join(bin, "hack-guard-probe"), "#!/bin/sh\nexit 0\n"); + await chmod(join(bin, "hack-guard-probe"), 0o755); + await writeFile( + join(suite, "bunfig.toml"), + opts.preload ? `[test]\npreload = [${JSON.stringify(PRELOAD)}]\n` : "" + ); + await writeFile(join(suite, "a-leak-in-test.test.ts"), LEAK_IN_TEST); + await writeFile(join(suite, "b-leak-in-hooks.test.ts"), LEAK_IN_HOOKS); + await writeFile(join(suite, "c-later.test.ts"), LATER); + const child = Bun.spawn( + [ + process.execPath, + "test", + "./a-leak-in-test.test.ts", + "./b-leak-in-hooks.test.ts", + "./c-later.test.ts", + ], + { + cwd: suite, + env: { PATH: bin, HOME: home, TMPDIR: dir, NO_COLOR: "1" }, + stdout: "pipe", + stderr: "pipe", + } + ); + const [stdout, stderr, code] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + return { code, output: `${stdout}\n${stderr}` }; +} + +test("without the guard, an early leak cascades into a later file", async () => { + const run = await runNestedSuite({ preload: false }); + expect(run.code).not.toBe(0); + expect(run.output).toContain("Executable not found in $PATH"); + expect(run.output).toContain( + "(fail) a later file still finds executables on PATH" + ); +}, 60_000); + +test("the guard names each leaking file, restores state, and later files pass", async () => { + const run = await runNestedSuite({ preload: true }); + expect(run.code).not.toBe(0); + expect(run.output).toContain( + "[global-state-guard] a test in a-leak-in-test.test.ts left process state changed: env PATH changed." + ); + expect(run.output).toContain( + "b-leak-in-hooks.test.ts left process state changed after the file finished: cwd changed; env HACK_HOME added" + ); + expect(run.output).toContain("1 test file(s) leaked process state"); + expect(run.output).not.toContain("Executable not found"); + expect(run.output).toContain("LATER-FILE-PASSED"); +}, 60_000); diff --git a/tests/helpers/global-state-guard-preload.ts b/tests/helpers/global-state-guard-preload.ts new file mode 100644 index 000000000..54ea8478f --- /dev/null +++ b/tests/helpers/global-state-guard-preload.ts @@ -0,0 +1,119 @@ +/** + * `bun test` preload (see `bunfig.toml`): fails loudly when a test or a test + * file leaves process-global state changed, names the file, and restores the + * environment, working directory, umask and exit code so later files do not + * fail for an unrelated reason. Replaced functions and process listeners are + * reported but kept; `restoreGlobalState` explains why. + * + * Two checks run: + * - Per test: the state before the global `beforeEach` must match the state + * after the global `afterEach`. Bun runs preload `beforeEach` hooks before a + * file's own hooks and preload `afterEach` hooks after them, so a file's + * `beforeEach`/`afterEach` pair that restores correctly passes. A mismatch + * fails that test, restores the pre-test environment and cwd, and names the file. + * - Per file: when Bun loads the next test file, the state must match the + * state before the previous file loaded. This catches module-scope and + * `beforeAll`/`afterAll` leaks, including an `afterAll` that never ran its + * restore. A mismatch is reported at once, the pre-file environment is restored, + * and the run fails from the preload `afterAll` with every leaking file. + * + * Bun calls a runtime plugin's `onLoad` for a test file after the previous + * file's `afterAll` hooks and before the new file's module code, which makes + * it the file boundary. The plugin returns the file's source unchanged. + */ +import { afterAll, afterEach, beforeEach } from "bun:test"; +import { readFileSync } from "node:fs"; +import { relative } from "node:path"; + +import { + captureGlobalState, + diffGlobalState, + type GlobalStateSnapshot, + restoreGlobalState, +} from "./global-state-guard.ts"; + +/** + * Environment keys a test file may change without a restore. Every entry + * needs a reason. Keep this list empty unless a change is outside the tests' + * control. + */ +const ALLOWED_ENV_CHANGES: ReadonlyMap = new Map(); + +const ROOT = process.cwd(); +const TEST_FILE = /\.test\.[cm]?[jt]sx?$/; + +let fileBaseline: GlobalStateSnapshot = captureGlobalState(); +let currentFile: string | undefined; +const fileLeaks: string[] = []; + +function displayPath(path: string | undefined): string { + return path === undefined ? "(preload)" : relative(ROOT, path); +} + +function checkFileBoundary(): void { + const changes = diffGlobalState(fileBaseline, captureGlobalState(), { + allowedEnvChanges: ALLOWED_ENV_CHANGES, + }); + if (changes.length > 0) { + const message = `${displayPath(currentFile)} left process state changed after the file finished: ${changes.join("; ")}`; + fileLeaks.push(message); + console.error( + `\n[global-state-guard] ${message}. Restored the environment and cwd from before the file so later files run clean.\n` + ); + restoreGlobalState(fileBaseline); + } + fileBaseline = captureGlobalState(); +} + +Bun.plugin({ + name: "global-state-guard", + setup(build) { + build.onLoad({ filter: TEST_FILE }, (args) => { + if (args.path === Bun.main && args.path !== currentFile) { + checkFileBoundary(); + currentFile = args.path; + } + const loader = args.path.endsWith("x") + ? args.path.endsWith(".jsx") + ? "jsx" + : "tsx" + : args.path.endsWith(".ts") + ? "ts" + : "js"; + return { contents: readFileSync(args.path, "utf8"), loader }; + }); + }, +}); + +let testBaseline: GlobalStateSnapshot | undefined; + +beforeEach(() => { + testBaseline = captureGlobalState(); +}); + +afterEach(() => { + const before = testBaseline; + testBaseline = undefined; + if (before === undefined) { + return; + } + const changes = diffGlobalState(before, captureGlobalState(), { + allowedEnvChanges: ALLOWED_ENV_CHANGES, + }); + if (changes.length === 0) { + return; + } + restoreGlobalState(before); + throw new Error( + `[global-state-guard] a test in ${displayPath(Bun.main)} left process state changed: ${changes.join("; ")}. Restore it in try/finally or afterEach; the guard restored the pre-test environment and cwd.` + ); +}); + +afterAll(() => { + checkFileBoundary(); + if (fileLeaks.length > 0) { + throw new Error( + `[global-state-guard] ${fileLeaks.length} test file(s) leaked process state:\n${fileLeaks.join("\n")}` + ); + } +}); diff --git a/tests/helpers/global-state-guard.ts b/tests/helpers/global-state-guard.ts new file mode 100644 index 000000000..61cc9fd02 --- /dev/null +++ b/tests/helpers/global-state-guard.ts @@ -0,0 +1,285 @@ +/** + * Snapshot, compare and restore the process-global state that Bun shares + * between every test file in one `bun test` run. + * + * Bun runs all files in one process. A test that changes `PATH`, `HOME`, + * `HACK_HOME`, the working directory or another process global and then fails + * before its own restore leaks that change into every later file. Those files + * then fail for unrelated reasons (for example "Executable not found in $PATH: + * git"), which hides the first, real failure. The preload in + * `tests/helpers/global-state-guard-preload.ts` uses this module to name the + * file that leaked and to put the environment and working directory back so + * later files run clean. + * + * Reports name keys and kinds only. They never include environment values, + * because fixtures may hold secret-shaped values. + */ + +/** Process events whose listener lists are compared (reported, not restored). */ +export const GUARDED_PROCESS_EVENTS = [ + "SIGINT", + "SIGTERM", + "SIGHUP", + "SIGQUIT", + "SIGUSR1", + "SIGUSR2", + "exit", + "beforeExit", + "uncaughtException", + "unhandledRejection", +] as const; + +type GuardedProcessEvent = (typeof GUARDED_PROCESS_EVENTS)[number]; +type Listener = (...args: unknown[]) => void; + +interface GlobalBinding { + readonly name: string; + readonly owner: object; + readonly key: PropertyKey; +} + +/** + * Globals that tests replace with fakes. Each entry is compared by identity and + * written back on restore. + */ +const GUARDED_BINDINGS: readonly GlobalBinding[] = [ + { name: "globalThis.fetch", owner: globalThis, key: "fetch" }, + { name: "globalThis.setTimeout", owner: globalThis, key: "setTimeout" }, + { name: "globalThis.clearTimeout", owner: globalThis, key: "clearTimeout" }, + { name: "globalThis.setInterval", owner: globalThis, key: "setInterval" }, + { name: "globalThis.clearInterval", owner: globalThis, key: "clearInterval" }, + { name: "globalThis.Date", owner: globalThis, key: "Date" }, + { name: "Bun.spawn", owner: Bun, key: "spawn" }, + { name: "Bun.spawnSync", owner: Bun, key: "spawnSync" }, + { name: "Bun.which", owner: Bun, key: "which" }, + { name: "process.exit", owner: process, key: "exit" }, + { name: "process.kill", owner: process, key: "kill" }, +]; + +/** The guarded state; {@link restoreGlobalState} puts back its process data. */ +export interface GlobalStateSnapshot { + readonly cwd: string; + readonly umask: number; + /** `process.exitCode`, with unset read as 0: Bun ignores assigning `undefined`. */ + readonly exitCode: NonNullable; + readonly envObject: NodeJS.ProcessEnv; + readonly env: Readonly>; + readonly listeners: ReadonlyMap; + readonly bindings: ReadonlyMap; + readonly platform: PropertyDescriptor | undefined; +} + +function readUmask(): number { + // process.umask() with no argument reads the mask without changing it. + return process.umask(); +} + +function copyEnv(env: NodeJS.ProcessEnv): Record { + const copy: Record = {}; + for (const key of Object.keys(env)) { + const value = env[key]; + if (value !== undefined) { + copy[key] = value; + } + } + return copy; +} + +/** Captures the guarded process state. Cheap enough to run around every test. */ +export function captureGlobalState(): GlobalStateSnapshot { + const listeners = new Map(); + for (const event of GUARDED_PROCESS_EVENTS) { + listeners.set(event, process.listeners(event) as Listener[]); + } + const bindings = new Map(); + for (const binding of GUARDED_BINDINGS) { + bindings.set( + binding.name, + Object.getOwnPropertyDescriptor(binding.owner, binding.key) + ); + } + return { + cwd: process.cwd(), + umask: readUmask(), + exitCode: process.exitCode ?? 0, + envObject: process.env, + env: copyEnv(process.env), + listeners, + bindings, + platform: Object.getOwnPropertyDescriptor(process, "platform"), + }; +} + +function sameDescriptor( + left: PropertyDescriptor | undefined, + right: PropertyDescriptor | undefined +): boolean { + if (left === undefined || right === undefined) { + return left === right; + } + return ( + left.value === right.value && + left.get === right.get && + left.set === right.set && + left.writable === right.writable + ); +} + +function sameListeners( + left: readonly Listener[], + right: readonly Listener[] +): boolean { + return ( + left.length === right.length && + left.every((listener, index) => listener === right[index]) + ); +} + +/** Options for {@link diffGlobalState}. */ +export interface DiffGlobalStateOptions { + /** Environment keys that may differ, each with the reason it may. */ + readonly allowedEnvChanges?: ReadonlyMap; +} + +function diffProcessData( + before: GlobalStateSnapshot, + current: GlobalStateSnapshot +): string[] { + const changes: string[] = []; + if (current.cwd !== before.cwd) { + changes.push("cwd changed"); + } + if (current.umask !== before.umask) { + changes.push("umask changed"); + } + if (current.exitCode !== before.exitCode) { + changes.push("process.exitCode changed"); + } + if (current.envObject !== before.envObject) { + changes.push("process.env object replaced"); + } + return changes; +} + +function envChange( + was: string | undefined, + now: string | undefined +): string | undefined { + if (was === now) { + return; + } + if (was === undefined) { + return "added"; + } + return now === undefined ? "removed" : "changed"; +} + +function diffEnv( + before: GlobalStateSnapshot, + current: GlobalStateSnapshot, + allowed: ReadonlyMap | undefined +): string[] { + const keys = new Set([ + ...Object.keys(before.env), + ...Object.keys(current.env), + ]); + const changes: string[] = []; + for (const key of [...keys].sort()) { + const change = allowed?.has(key) + ? undefined + : envChange(before.env[key], current.env[key]); + if (change !== undefined) { + changes.push(`env ${key} ${change}`); + } + } + return changes; +} + +function diffProcessHooks( + before: GlobalStateSnapshot, + current: GlobalStateSnapshot +): string[] { + const changes: string[] = []; + for (const event of GUARDED_PROCESS_EVENTS) { + if ( + !sameListeners( + before.listeners.get(event) ?? [], + current.listeners.get(event) ?? [] + ) + ) { + changes.push(`process '${event}' listeners changed`); + } + } + for (const binding of GUARDED_BINDINGS) { + if ( + !sameDescriptor( + before.bindings.get(binding.name), + current.bindings.get(binding.name) + ) + ) { + changes.push(`${binding.name} replaced`); + } + } + if (!sameDescriptor(before.platform, current.platform)) { + changes.push("process.platform replaced"); + } + return changes; +} + +/** + * Lists how `current` differs from `before`, as value-free lines such as + * `env PATH changed` or `cwd changed`. An empty list means no leak. + */ +export function diffGlobalState( + before: GlobalStateSnapshot, + current: GlobalStateSnapshot = captureGlobalState(), + opts: DiffGlobalStateOptions = {} +): string[] { + return [ + ...diffProcessData(before, current), + ...diffEnv(before, current, opts.allowedEnvChanges), + ...diffProcessHooks(before, current), + ]; +} + +function restoreEnvObject(snapshot: GlobalStateSnapshot): void { + if (process.env !== snapshot.envObject) { + process.env = snapshot.envObject; + } + const env = process.env; + for (const key of Object.keys(env)) { + if (!Object.hasOwn(snapshot.env, key)) { + Reflect.deleteProperty(env, key); + } + } + for (const [key, value] of Object.entries(snapshot.env)) { + if (env[key] !== value) { + env[key] = value; + } + } +} + +/** + * Puts the process data back to `snapshot`: the environment, working + * directory, umask and `process.exitCode`. The original `process.env` object + * is reinstated and edited in place, so `Bun.env` and child-process defaults + * keep seeing the same environment. + * + * Replaced functions (`Bun.spawn`, `process.kill`, `fetch`, timers), process + * listeners and `process.platform` are reported by {@link diffGlobalState} + * but deliberately not restored. A leak there usually comes from a test that + * timed out while its work is still running, and that fake is what keeps the + * leftover work away from real processes, signals, the network and the + * Docker socket. Restoring the real function under that work could let it + * reach the host. + */ +export function restoreGlobalState(snapshot: GlobalStateSnapshot): void { + restoreEnvObject(snapshot); + if (process.cwd() !== snapshot.cwd) { + process.chdir(snapshot.cwd); + } + if (readUmask() !== snapshot.umask) { + process.umask(snapshot.umask); + } + process.exitCode = snapshot.exitCode; +} diff --git a/tests/helpers/native-compose-vm-files.ts b/tests/helpers/native-compose-vm-files.ts index f19c9bf25..aa826d841 100644 --- a/tests/helpers/native-compose-vm-files.ts +++ b/tests/helpers/native-compose-vm-files.ts @@ -15,8 +15,18 @@ import { restoreEnv } from "./env.ts"; const cleanups: (() => Promise)[] = []; /** Register this in each test file; Bun caches imports but scopes test hooks. */ export async function cleanupVmFileFixtures() { + // Run every cleanup even when one fails, so a failed `rm` cannot skip the + // environment restore of another fixture. + const failures: unknown[] = []; for (const cleanup of cleanups.splice(0).reverse()) { - await cleanup(); + try { + await cleanup(); + } catch (error: unknown) { + failures.push(error); + } + } + if (failures.length > 0) { + throw failures[0]; } } const environmentKeys = [ @@ -54,6 +64,14 @@ export async function vmFileFixture() { const compiler = resolve( process.env.HACK_CONFIG_COMPILER_BINARY ?? "dist/hack-config-compiler" ); + // Register the restore before the first change, so a setup step that fails + // below still puts PATH, HOME and the Docker keys back. + cleanups.push(async () => { + for (const key of environmentKeys) { + restoreEnv(key, saved[key]); + } + await rm(root, { recursive: true, force: true }); + }); for (const key of environmentKeys) { Reflect.deleteProperty(process.env, key); } @@ -98,10 +116,6 @@ export async function vmFileFixture() { }); cleanups.push(async () => { server.stop(true); - for (const key of environmentKeys) { - restoreEnv(key, saved[key]); - } - await rm(root, { recursive: true, force: true }); }); const docker = join(root, "docker"); await writeFile( diff --git a/tests/helpers/retained-build-adoption.ts b/tests/helpers/retained-build-adoption.ts index 2fcbd7283..e213a12d6 100644 --- a/tests/helpers/retained-build-adoption.ts +++ b/tests/helpers/retained-build-adoption.ts @@ -132,7 +132,14 @@ else red(); await chmod(binary, 0o700); const previousPath = process.env.PATH; process.env.PATH = outer; - const compiler = await managedEnvCompilerFixture(join(outer, "compiler")); + let compiler: string; + try { + compiler = await managedEnvCompilerFixture(join(outer, "compiler")); + } catch (error: unknown) { + // No fixture (and so no cleanup) reaches the caller; restore PATH here. + restoreEnv("PATH", previousPath); + throw error; + } function isModel(value: unknown): value is typeof model { return ( isRecord(value) && diff --git a/tests/helpers/retained-routing-adoption.ts b/tests/helpers/retained-routing-adoption.ts index d8d630905..f9ad4ac7a 100644 --- a/tests/helpers/retained-routing-adoption.ts +++ b/tests/helpers/retained-routing-adoption.ts @@ -673,14 +673,22 @@ else { throw error; } }, + /** + * Restores HACK_HOME only. Safe while the lifetime is unknown: a HACK_HOME + * left pointing at this fixture leaks into every later file in the Bun + * process (HACK-1327). The spies and root stay until {@link cleanup}. + */ + releaseEnvironment: () => { + restoreEnv("HACK_HOME", previousHome); + }, cleanup: async () => { + restoreEnv("HACK_HOME", previousHome); if (!lifetime.canRestore()) { return refuse(); } for (const spy of spies) { spy.mockRestore(); } - restoreEnv("HACK_HOME", previousHome); await rm(outer, { recursive: true, force: true }); }, }; diff --git a/tests/helpers/retained-source-bind-adoption.ts b/tests/helpers/retained-source-bind-adoption.ts index 7338bcce5..e1b17a6fa 100644 --- a/tests/helpers/retained-source-bind-adoption.ts +++ b/tests/helpers/retained-source-bind-adoption.ts @@ -87,7 +87,14 @@ else if(kind==='network'&&selected===network)console.log(JSON.stringify({id:netw await chmod(binary, 0o700); const previousPath = process.env.PATH; process.env.PATH = outer; - const compiler = await managedEnvCompilerFixture(join(outer, "compiler")); + let compiler: string; + try { + compiler = await managedEnvCompilerFixture(join(outer, "compiler")); + } catch (error: unknown) { + // No fixture (and so no cleanup) reaches the caller; restore PATH here. + restoreEnv("PATH", previousPath); + throw error; + } return { root, outer, diff --git a/tests/native-compose-adoption-file-generation.test.ts b/tests/native-compose-adoption-file-generation.test.ts index d635d2a9a..2af3464b8 100644 --- a/tests/native-compose-adoption-file-generation.test.ts +++ b/tests/native-compose-adoption-file-generation.test.ts @@ -69,6 +69,7 @@ let root: string; let projectRoot: string; let binary: string; let priorPath: string | undefined; +let pathReplaced = false; let fixture: { running: boolean; failed: boolean; @@ -167,9 +168,17 @@ else {const source=JSON.parse(await Bun.stdin.text());const {schema_version,...p ` ); await chmod(binary, 0o700); + pathReplaced = true; process.env.PATH = root; }); afterEach(async () => { + // PATH is restored even when the lifetime is unknown: a PATH left pointing + // at this fixture root hides git and bun from every later file in the Bun + // process (HACK-1327). The root and spawn spy stay retained. + if (pathReplaced) { + pathReplaced = false; + restoreEnv("PATH", priorPath); + } if (!lifetime?.canRestore()) { uncertain = true; throw new Error( @@ -178,7 +187,6 @@ afterEach(async () => { } spawnSpy?.mockRestore(); spawnSpy = undefined; - restoreEnv("PATH", priorPath); await rm(root, { recursive: true, force: true }); }); async function save() { diff --git a/tests/native-compose-adoption-routing-generation.test.ts b/tests/native-compose-adoption-routing-generation.test.ts index d6a382ed6..d381e66f2 100644 --- a/tests/native-compose-adoption-routing-generation.test.ts +++ b/tests/native-compose-adoption-routing-generation.test.ts @@ -1,6 +1,6 @@ -import { test as boundedTest, expect, spyOn } from "bun:test"; +import { test as boundedTest, expect, mock, spyOn } from "bun:test"; import * as fs from "node:fs/promises"; -import { readFile } from "node:fs/promises"; +import { readFile, rm } from "node:fs/promises"; import { join } from "node:path"; import { DEFAULT_COMPOSE_STARTUP_TIMEOUT_MS } from "../src/lib/compose-startup-budget.ts"; import { isRecord } from "../src/lib/guards.ts"; @@ -9,6 +9,7 @@ import { LegacyComposeAdoptedGenerationError } from "../src/lib/native-compose-a import { legacyComposePublicationRefusal } from "../src/lib/native-compose-adoption-publication-diagnostics.ts"; import { runLegacyComposeRetainedRoutingOperation } from "../src/lib/native-compose-adoption-routing-execution.ts"; import * as privateState from "../src/lib/native-compose-private-state.ts"; +import { restoreEnv } from "./helpers/env.ts"; import { type RetainedRoutingStoreTimeoutMs, ROUTING_CANARY, @@ -75,6 +76,11 @@ const test = ( } finally { if (!lifetime.canRestore()) { fixtureUncertain = true; + // Keep the spies and root for the unsettled work, but never leave + // HACK_HOME pointing at this fixture for later files (HACK-1327). + if (issued) { + h.releaseEnvironment(); + } if (!failed) { failed = true; failure = new Error( @@ -858,6 +864,29 @@ boundedTest( }, 1000 ); +boundedTest( + "a refused teardown still restores HACK_HOME so later files never inherit the fixture home", + async () => { + const before = process.env.HACK_HOME; + const lifetime = retainedRoutingFixtureLifetime(Date.now() + 5000); + const fixture = await retainedRoutingFixture(lifetime); + try { + expect(process.env.HACK_HOME).toBe(fixture.home); + fixture.releaseEnvironment(); + expect(process.env.HACK_HOME).toBe(before); + process.env.HACK_HOME = fixture.home; + lifetime.retain(); + await expect(fixture.cleanup()).rejects.toThrow(); + expect(process.env.HACK_HOME).toBe(before); + } finally { + restoreEnv("HACK_HOME", before); + // The refused teardown kept the fixture spies; nothing else is mocked here. + mock.restore(); + await rm(fixture.outer, { recursive: true, force: true }); + } + }, + 5000 +); boundedTest( "known callback settlement permits fixture teardown before its unchanged deadline", async () => { diff --git a/tests/native-config-protected-files-fixture.test.ts b/tests/native-config-protected-files-fixture.test.ts index aaa190763..fde295a42 100644 --- a/tests/native-config-protected-files-fixture.test.ts +++ b/tests/native-config-protected-files-fixture.test.ts @@ -770,7 +770,7 @@ test.skipIf(process.platform !== "darwin")( } ); -// This intentional-unknown negative is last: its root and global spy remain retained until process exit. +// This intentional-unknown negative is last: its root remains retained until process exit. test("closed-pipe survivor refuses completion and permanently vetoes restoration without a former-group signal", async () => { const pidPath = join(directory, "descendant"), child = join(directory, "child.ts"), @@ -847,4 +847,8 @@ test("closed-pipe survivor refuses completion and permanently vetoes restoration expect(descendantJoined).toBe(true); expect(lifetime!.canRestore()).toBe(false); expectedUnknown = true; + // The descendant has joined, so this pass-through spy guards nothing more. + // Restore it here: the vetoed teardown would otherwise leave process.kill + // replaced for every later file in the Bun process (HACK-1327). + signal.mockRestore(); }); From 33cebd7390c0722587cd32bdbe0c7e85d5aa3db8 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Thu, 22 Oct 2026 10:04:03 -0400 Subject: [PATCH 2/3] test: catch proxy keys Bun keeps after a delete, and release the launchctl shim per test The M3 full suite at the first guard commit failed 16 shared-host-lifecycle cases (helpers reported as foreign listeners or never ready) and flagged tests/e2e-launchd-guard.test.ts for a process exit listener. - Root cause of the shared-host cascade: the engine-identity ambient-proxy test set HTTP(S)_PROXY and their lower-case forms, then restored them with a delete. Bun 1.4 copies assignments to proxy keys into the environment that fetch and Bun.spawn (without env) use, but never a delete, so every later loopback fetch in the process went through the dead proxy while process.env looked clean. restoreEnv now assigns "" before deleting. - The guard now also compares Bun's native environment at each file boundary (one /usr/bin/env child per file), names the file, and turns proxies off natively with NO_PROXY=* because Bun cannot take a deleted key back. Restores of deleted keys assign "" first as well. - The e2e harness's launchctl shim registered a process exit listener that lived for the whole Bun process; releaseLaunchctlGuard removes the shim and listener, and e2e-launchd-guard calls it after each test. Refs HACK-1327 Co-Authored-By: Claude Opus 5.5 --- .ai/skills/hack-repo-verify/SKILL.md | 3 + tests/e2e-launchd-guard.test.ts | 17 +++ tests/e2e/harness.ts | 30 +++++- tests/global-state-guard.test.ts | 114 +++++++++++++++++++- tests/helpers/env.ts | 7 ++ tests/helpers/global-state-guard-preload.ts | 43 +++++++- tests/helpers/global-state-guard.ts | 101 ++++++++++++++++- 7 files changed, 303 insertions(+), 12 deletions(-) diff --git a/.ai/skills/hack-repo-verify/SKILL.md b/.ai/skills/hack-repo-verify/SKILL.md index 33530f151..2c83dba60 100644 --- a/.ai/skills/hack-repo-verify/SKILL.md +++ b/.ai/skills/hack-repo-verify/SKILL.md @@ -71,6 +71,9 @@ exit code, a guarded global function or a process listener changed fails with a environment, cwd, umask and exit code so later files run clean; it keeps replaced functions and listeners, which may still protect unsettled work. Fix the named file's restore (`try`/`finally` or `afterEach`), not the later failures. +Restore environment keys with `restoreEnv` from `tests/helpers/env.ts`: Bun keeps +a deleted `HTTP_PROXY`-style key for `fetch` and `Bun.spawn` without `env`, and +the guard reports that as `native env changed`. For release workflow edits, run the pinned syntax/context check also enforced by CI: `mise x actionlint@1.7.12 -- actionlint -shellcheck= -pyflakes= .github/workflows/ci.yml .github/workflows/release.yml .github/workflows/prerelease.yml .github/workflows/bun-crash-experiment.yml`. diff --git a/tests/e2e-launchd-guard.test.ts b/tests/e2e-launchd-guard.test.ts index a669b8d1d..6e2f6ca7c 100644 --- a/tests/e2e-launchd-guard.test.ts +++ b/tests/e2e-launchd-guard.test.ts @@ -12,6 +12,7 @@ import { buildCliEnv, describeLaunchAgentChange, fingerprintLaunchAgentPlist, + releaseLaunchctlGuard, } from "./e2e/harness.ts"; /* @@ -24,6 +25,9 @@ import { let cleanupRoot: string | null = null; afterEach(async () => { + // The shim and its process exit listener are per process in the e2e runner; + // here every test file shares one Bun process, so release them per test. + releaseLaunchctlGuard(); if (cleanupRoot) { await rm(cleanupRoot, { recursive: true, force: true }); cleanupRoot = null; @@ -129,3 +133,16 @@ test("the LaunchAgent canary reports content, mtime, creation and removal by has }) ).toEndWith("-> absent"); }); + +test("releasing the launchctl guard removes its shim and exit listener", async () => { + const listeners = process.listenerCount("exit"); + const env = buildCliEnv({ hackHome: "/tmp/hack-e2e-home-guard" }); + const guardDir = (env.PATH ?? "").split(":")[0] ?? ""; + expect(process.listenerCount("exit")).toBe(listeners + 1); + expect((await stat(guardDir)).isDirectory()).toBe(true); + releaseLaunchctlGuard(); + expect(process.listenerCount("exit")).toBe(listeners); + expect(await stat(guardDir).catch(() => null)).toBeNull(); + const next = buildCliEnv({ hackHome: "/tmp/hack-e2e-home-guard" }); + expect((next.PATH ?? "").split(":")[0]).not.toBe(guardDir); +}); diff --git a/tests/e2e/harness.ts b/tests/e2e/harness.ts index c8339f60b..e64fed106 100644 --- a/tests/e2e/harness.ts +++ b/tests/e2e/harness.ts @@ -272,7 +272,11 @@ function applyLaunchdGuard( return guarded; } -type LaunchctlGuard = { readonly dir: string; readonly logPath: string }; +type LaunchctlGuard = { + readonly dir: string; + readonly logPath: string; + readonly removeOnExit: () => void; +}; let launchctlGuard: LaunchctlGuard | null = null; function ensureLaunchctlGuard(): LaunchctlGuard { @@ -299,13 +303,31 @@ function ensureLaunchctlGuard(): LaunchctlGuard { ].join("\n") ); chmodSync(shim, 0o755); - process.once("exit", () => { + const removeOnExit = () => { rmSync(dir, { recursive: true, force: true }); - }); - launchctlGuard = { dir, logPath }; + }; + process.once("exit", removeOnExit); + launchctlGuard = { dir, logPath, removeOnExit }; return launchctlGuard; } +/** + * Removes the launchctl guard shim and its process `exit` listener now. The + * e2e runner keeps one shim for its whole process; a `bun test` file that + * builds CLI env calls this in `afterEach`, because every test file shares + * one Bun process and a leftover listener would leak into later files. The + * next `buildCliEnv` call creates a fresh shim. + */ +export function releaseLaunchctlGuard(): void { + if (!launchctlGuard) { + return; + } + const { removeOnExit } = launchctlGuard; + launchctlGuard = null; + process.removeListener("exit", removeOnExit); + removeOnExit(); +} + /** * launchctl verbs the guard shim refused during this process, in call order. * Any entry means a CLI invocation tried to manage the real hackd service. diff --git a/tests/global-state-guard.test.ts b/tests/global-state-guard.test.ts index f7cdad5e2..281223eb1 100644 --- a/tests/global-state-guard.test.ts +++ b/tests/global-state-guard.test.ts @@ -14,6 +14,7 @@ import { restoreEnv } from "./helpers/env.ts"; import { captureGlobalState, diffGlobalState, + diffNativeEnv, restoreGlobalState, } from "./helpers/global-state-guard.ts"; @@ -146,6 +147,15 @@ beforeAll(() => { test("passes but never restores", () => {}); `; +// Port 9 on loopback refuses connections, so a request sent through this +// proxy fails at once instead of waiting for DNS or a timeout. +const LEAK_NATIVE_PROXY = `import { test } from "bun:test"; +test("sets a proxy and deletes it, which Bun's fetch never sees", () => { + process.env.HTTP_PROXY = "http://127.0.0.1:9"; + delete process.env.HTTP_PROXY; +}); +`; + const LATER = `import { expect, test } from "bun:test"; test("a later file still finds executables on PATH", () => { const child = Bun.spawnSync(["hack-guard-probe"], { env: { ...process.env } }); @@ -153,6 +163,22 @@ test("a later file still finds executables on PATH", () => { expect(process.env.HACK_HOME).toBeUndefined(); console.log("LATER-FILE-PASSED"); }); +test("a later file still reaches a loopback server directly", async () => { + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + fetch: () => new Response("direct"), + }); + try { + const response = await fetch("http://127.0.0.1:" + server.port + "/", { + signal: AbortSignal.timeout(5000), + }); + expect(await response.text()).toBe("direct"); + console.log("LATER-FETCH-PASSED"); + } finally { + server.stop(true); + } +}); `; async function runNestedSuite(opts: { @@ -173,14 +199,19 @@ async function runNestedSuite(opts: { ); await writeFile(join(suite, "a-leak-in-test.test.ts"), LEAK_IN_TEST); await writeFile(join(suite, "b-leak-in-hooks.test.ts"), LEAK_IN_HOOKS); - await writeFile(join(suite, "c-later.test.ts"), LATER); + await writeFile( + join(suite, "c-leak-native-proxy.test.ts"), + LEAK_NATIVE_PROXY + ); + await writeFile(join(suite, "d-later.test.ts"), LATER); const child = Bun.spawn( [ process.execPath, "test", "./a-leak-in-test.test.ts", "./b-leak-in-hooks.test.ts", - "./c-later.test.ts", + "./c-leak-native-proxy.test.ts", + "./d-later.test.ts", ], { cwd: suite, @@ -204,6 +235,10 @@ test("without the guard, an early leak cascades into a later file", async () => expect(run.output).toContain( "(fail) a later file still finds executables on PATH" ); + // The deleted proxy still routes a later file's loopback fetch. + expect(run.output).toContain( + "(fail) a later file still reaches a loopback server directly" + ); }, 60_000); test("the guard names each leaking file, restores state, and later files pass", async () => { @@ -215,7 +250,80 @@ test("the guard names each leaking file, restores state, and later files pass", expect(run.output).toContain( "b-leak-in-hooks.test.ts left process state changed after the file finished: cwd changed; env HACK_HOME added" ); - expect(run.output).toContain("1 test file(s) leaked process state"); + expect(run.output).toContain( + "c-leak-native-proxy.test.ts left Bun's native environment changed after the file finished: native env HTTP_PROXY changed." + ); + expect(run.output).toContain("2 test file(s) leaked process state"); + expect(run.output).not.toContain("127.0.0.1:9"); expect(run.output).not.toContain("Executable not found"); expect(run.output).toContain("LATER-FILE-PASSED"); + expect(run.output).toContain("LATER-FETCH-PASSED"); }, 60_000); + +test("the native diff names changed keys only and treats empty as unset", () => { + const before = new Map([ + ["PATH", "/bin"], + ["HTTP_PROXY", ""], + ]); + const current = new Map([ + ["PATH", "/bin"], + ["HTTPS_PROXY", "http://user:secret-shaped@127.0.0.1:9"], + ]); + expect(diffNativeEnv(before, current)).toEqual(["HTTPS_PROXY"]); + expect(diffNativeEnv(before, new Map([["PATH", "/bin"]]))).toEqual([]); + expect(diffNativeEnv(before, current).join("\n")).not.toContain("secret"); +}); + +// Runs in its own process: once a proxy key is deleted, Bun ignores later +// assignments to it, so checking this in the shared test process would +// depend on which files ran before. +const RESTORE_ENV_PROBE = `import { restoreEnv } from ${JSON.stringify(resolve(import.meta.dir, "helpers/env.ts"))}; +import { captureNativeEnv, diffNativeEnv } from ${JSON.stringify(resolve(import.meta.dir, "helpers/global-state-guard.ts"))}; +const before = captureNativeEnv(); +process.env.HTTP_PROXY = "http://127.0.0.1:9"; +const set = captureNativeEnv(); +if (process.argv[2] === "bare-delete") delete process.env.HTTP_PROXY; +else restoreEnv("HTTP_PROXY", undefined); +const after = captureNativeEnv(); +console.log(JSON.stringify({ + js: process.env.HTTP_PROXY ?? null, + set: before && set ? diffNativeEnv(before, set) : null, + after: before && after ? diffNativeEnv(before, after) : null, +})); +`; + +async function runRestoreEnvProbe(mode: string): Promise { + const dir = await tempDir(); + const script = join(dir, "probe.ts"); + await writeFile(script, RESTORE_ENV_PROBE); + const child = Bun.spawn([process.execPath, script, mode], { + cwd: dir, + env: { PATH: "/usr/bin:/bin", HOME: dir, TMPDIR: dir }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, code] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + if (code !== 0) { + throw new Error(`restoreEnv probe exited ${code}: ${stderr}`); + } + return JSON.parse(stdout); +} + +test("restoreEnv leaves no proxy in Bun's native environment; a bare delete does", async () => { + expect(await runRestoreEnvProbe("restore-env")).toEqual({ + js: null, + set: ["HTTP_PROXY"], + after: [], + }); + // If a Bun upgrade makes deletes reach the native environment, this fails + // and the empty-assignment workaround in restoreEnv can go. + expect(await runRestoreEnvProbe("bare-delete")).toEqual({ + js: null, + set: ["HTTP_PROXY"], + after: ["HTTP_PROXY"], + }); +}, 30_000); diff --git a/tests/helpers/env.ts b/tests/helpers/env.ts index 9421edf6f..4c271fd43 100644 --- a/tests/helpers/env.ts +++ b/tests/helpers/env.ts @@ -2,9 +2,16 @@ * Sets one environment variable, or removes it when `value` is undefined. Assigning `undefined` * to a `process.env` key stores the string "undefined" in Bun 1.4 (as in Node), which then leaks * into later tests in the same process, so a missing value must be deleted instead. + * + * Before a delete, the key is set to "". Bun 1.4 copies assignments to the proxy keys + * (`HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` and their lower-case forms) into the environment that + * `fetch` and `Bun.spawn` without `env` use, but a delete never reaches it. Without the empty + * assignment, a proxy a test set and then deleted keeps routing every later `fetch` in the + * process, including loopback requests, through that proxy (HACK-1327). An empty proxy is no proxy. */ export function restoreEnv(key: string, value: string | undefined): void { if (value === undefined) { + process.env[key] = ""; Reflect.deleteProperty(process.env, key); } else { process.env[key] = value; diff --git a/tests/helpers/global-state-guard-preload.ts b/tests/helpers/global-state-guard-preload.ts index 54ea8478f..a7041435d 100644 --- a/tests/helpers/global-state-guard-preload.ts +++ b/tests/helpers/global-state-guard-preload.ts @@ -16,6 +16,9 @@ * `beforeAll`/`afterAll` leaks, including an `afterAll` that never ran its * restore. A mismatch is reported at once, the pre-file environment is restored, * and the run fails from the preload `afterAll` with every leaking file. + * This check also compares Bun's native environment (one short `/usr/bin/env` + * child per file), which keeps a proxy key that a test set and then deleted + * although `process.env` no longer shows it. * * Bun calls a runtime plugin's `onLoad` for a test file after the previous * file's `afterAll` hooks and before the new file's module code, which makes @@ -27,8 +30,12 @@ import { relative } from "node:path"; import { captureGlobalState, + captureNativeEnv, diffGlobalState, + diffNativeEnv, + disableNativeProxies, type GlobalStateSnapshot, + type NativeEnv, restoreGlobalState, } from "./global-state-guard.ts"; @@ -43,6 +50,7 @@ const ROOT = process.cwd(); const TEST_FILE = /\.test\.[cm]?[jt]sx?$/; let fileBaseline: GlobalStateSnapshot = captureGlobalState(); +let nativeBaseline: NativeEnv | undefined = captureNativeEnv(); let currentFile: string | undefined; const fileLeaks: string[] = []; @@ -50,18 +58,45 @@ function displayPath(path: string | undefined): string { return path === undefined ? "(preload)" : relative(ROOT, path); } +function reportFileLeak(message: string, repair: string): void { + fileLeaks.push(message); + console.error(`\n[global-state-guard] ${message}. ${repair}\n`); +} + +/** + * Checks Bun's native environment after `process.env` was compared and + * restored, so only a change that `process.env` cannot show remains: a proxy + * key that was set and then deleted (see `captureNativeEnv`). + */ +function checkNativeEnv(): void { + const current = captureNativeEnv(); + if (nativeBaseline === undefined || current === undefined) { + nativeBaseline = current; + return; + } + const keys = diffNativeEnv(nativeBaseline, current); + if (keys.length > 0) { + reportFileLeak( + `${displayPath(currentFile)} left Bun's native environment changed after the file finished: ${keys.map((key) => `native env ${key} changed`).join("; ")}. process.env no longer shows it, but fetch proxies and Bun.spawn without env still use it; restore proxy keys with restoreEnv from tests/helpers/env.ts`, + "Bun cannot take a deleted key back, so the guard set NO_PROXY=* in Bun's native environment for the rest of the run: later fetches go direct." + ); + disableNativeProxies(fileBaseline); + } + nativeBaseline = captureNativeEnv(); +} + function checkFileBoundary(): void { const changes = diffGlobalState(fileBaseline, captureGlobalState(), { allowedEnvChanges: ALLOWED_ENV_CHANGES, }); if (changes.length > 0) { - const message = `${displayPath(currentFile)} left process state changed after the file finished: ${changes.join("; ")}`; - fileLeaks.push(message); - console.error( - `\n[global-state-guard] ${message}. Restored the environment and cwd from before the file so later files run clean.\n` + reportFileLeak( + `${displayPath(currentFile)} left process state changed after the file finished: ${changes.join("; ")}`, + "Restored the environment and cwd from before the file so later files run clean." ); restoreGlobalState(fileBaseline); } + checkNativeEnv(); fileBaseline = captureGlobalState(); } diff --git a/tests/helpers/global-state-guard.ts b/tests/helpers/global-state-guard.ts index 61cc9fd02..2b47b760e 100644 --- a/tests/helpers/global-state-guard.ts +++ b/tests/helpers/global-state-guard.ts @@ -242,6 +242,15 @@ export function diffGlobalState( ]; } +/** + * Removes one key. The empty assignment first clears Bun's own copy of a proxy + * key, which a delete alone never reaches (see {@link captureNativeEnv}). + */ +function deleteEnvKey(env: NodeJS.ProcessEnv, key: string): void { + env[key] = ""; + Reflect.deleteProperty(env, key); +} + function restoreEnvObject(snapshot: GlobalStateSnapshot): void { if (process.env !== snapshot.envObject) { process.env = snapshot.envObject; @@ -249,7 +258,7 @@ function restoreEnvObject(snapshot: GlobalStateSnapshot): void { const env = process.env; for (const key of Object.keys(env)) { if (!Object.hasOwn(snapshot.env, key)) { - Reflect.deleteProperty(env, key); + deleteEnvKey(env, key); } } for (const [key, value] of Object.entries(snapshot.env)) { @@ -283,3 +292,93 @@ export function restoreGlobalState(snapshot: GlobalStateSnapshot): void { } process.exitCode = snapshot.exitCode; } + +/** + * The environment Bun itself uses for `fetch` proxies and for `Bun.spawn` + * without an `env` option, as key to value. + */ +export type NativeEnv = ReadonlyMap; + +// Bound at load so a leaked `Bun.spawnSync` fake can never answer for Bun. +const realSpawnSync = Bun.spawnSync.bind(Bun); + +/** + * Reads Bun's native environment by running `/usr/bin/env -0` without an + * `env` option, or returns undefined when that is not possible. + * + * `process.env` does not show all of it. Bun 1.4 starts this environment + * from the startup environment and copies later assignments to the proxy + * keys (`HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` and their lower-case forms) + * into it, but a delete never reaches it. A test that sets a proxy and then + * deletes it leaves `process.env` looking clean while every later `fetch` in + * the process, loopback included, still goes through that proxy (HACK-1327). + * `restoreEnv` in `tests/helpers/env.ts` avoids this by assigning "" first. + * Other keys never change here: Bun does not copy assignments to them. + * Costs one short child process; the preload runs it once per test file. + */ +export function captureNativeEnv(): NativeEnv | undefined { + try { + const child = realSpawnSync(["/usr/bin/env", "-0"], { + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + }); + if (child.exitCode !== 0 || !child.stdout) { + return; + } + const env = new Map(); + for (const entry of child.stdout.toString().split("\0")) { + const separator = entry.indexOf("="); + if (separator > 0) { + env.set(entry.slice(0, separator), entry.slice(separator + 1)); + } + } + return env; + } catch { + return; + } +} + +/** An empty value and an absent key mean the same to Bun: no proxy. */ +function nativeValue(env: NativeEnv, key: string): string | undefined { + const value = env.get(key); + return value === "" ? undefined : value; +} + +/** + * Lists the keys whose native value differs from `before`, sorted. An empty + * value and an absent key compare equal. Never includes values. + */ +export function diffNativeEnv(before: NativeEnv, current: NativeEnv): string[] { + const keys = new Set([...before.keys(), ...current.keys()]); + return [...keys] + .filter((key) => nativeValue(before, key) !== nativeValue(current, key)) + .sort(); +} + +const NO_PROXY_KEYS = ["NO_PROXY", "no_proxy"] as const; + +/** + * Turns proxies off in Bun's native environment for the rest of the process, + * and leaves `process.env` as `snapshot` had it. + * + * A stale native key cannot be put back: once a key is deleted from + * `process.env`, Bun 1.4 ignores every later assignment to it as well. The + * only repair left is `NO_PROXY=*`: it is assigned (Bun copies it) and deleted + * again (Bun keeps the copy), so later `fetch` calls go direct while + * `process.env` is unchanged. Children spawned without `env` still inherit + * the stale proxy key next to `NO_PROXY=*`. If a no-proxy key was itself + * deleted earlier, Bun ignores this too; the next file check then reports it + * again. + */ +export function disableNativeProxies(snapshot: GlobalStateSnapshot): void { + const env = process.env; + for (const key of NO_PROXY_KEYS) { + env[key] = "*"; + Reflect.deleteProperty(env, key); + const value = snapshot.env[key]; + if (value !== undefined) { + env[key] = value; + } + } +} From 9870275fa92ed06c3172224cca5839e57161216b Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Fri, 23 Oct 2026 02:38:22 -0400 Subject: [PATCH 3/3] fix(logs): remove the Loki follow SIGINT listener on every exit path `lokiTail` registered a `process.once("SIGINT")` handler and never removed it, so every follow that ended by socket close (the normal path) left a stale handler in the process. In the shared `bun test` process the next `process.emit("SIGINT")`, in tests/native-authored-project-oneoff.test.ts, consumed those `once` handlers and the global-state guard blamed that test for a listener change it did not cause. `runAuthoredCommand` in src/lib/native-authored-command.ts is symmetric; the one-off file passes on its own. The handler is now installed only while the tail is open and removed in a `finally`, so a SIGINT-fired (self-removing) handler and a normal close both leave the listener list as it was. Two regression tests cover both paths and fail without the fix. The guard now reports listener counts (`2 -> 0`) so a consumed handler from an earlier file is distinguishable from one added by the test it names. Refs HACK-1392, HACK-1327 Co-Authored-By: Claude Fable 5.1 --- src/ui/loki-logs.ts | 35 ++++++++++------- tests/global-state-guard.test.ts | 5 ++- tests/helpers/global-state-guard.ts | 16 ++++---- tests/loki-short-lived-logs.test.ts | 59 +++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 22 deletions(-) diff --git a/src/ui/loki-logs.ts b/src/ui/loki-logs.ts index eb095aefd..8820980c0 100644 --- a/src/ui/loki-logs.ts +++ b/src/ui/loki-logs.ts @@ -408,10 +408,6 @@ async function lokiTail(opts: LokiLogsParams): Promise { } }; - process.once("SIGINT", () => { - stop(); - }); - ws.addEventListener("error", () => { exitCode = 1; if (opts.json && opts.streamContext) { @@ -485,7 +481,7 @@ async function lokiTail(opts: LokiLogsParams): Promise { } }); - await new Promise((resolve) => { + const closedSocket = new Promise((resolve) => { ws.addEventListener("close", () => { closed = true; clearNoEntriesTimer(); @@ -493,16 +489,27 @@ async function lokiTail(opts: LokiLogsParams): Promise { }); }); - if (opts.json && opts.streamContext) { - writeLogStreamEvent({ - event: buildLogStreamEndEvent({ - context: opts.streamContext, - reason: exitCode === 0 ? "closed" : "error", - }), - }); + // Installed only while the tail is open and removed on every exit path. A + // `once` listener that never fires would otherwise outlive the follow and + // the next SIGINT in the process would run a stale stop (HACK-1392). + const onSigInt = () => { + stop(); + }; + process.once("SIGINT", onSigInt); + try { + await closedSocket; + if (opts.json && opts.streamContext) { + writeLogStreamEvent({ + event: buildLogStreamEndEvent({ + context: opts.streamContext, + reason: exitCode === 0 ? "closed" : "error", + }), + }); + } + return exitCode; + } finally { + process.off("SIGINT", onSigInt); } - - return exitCode; } function safeJsonParse(text: string): unknown { diff --git a/tests/global-state-guard.test.ts b/tests/global-state-guard.test.ts index 281223eb1..e3ff41fc0 100644 --- a/tests/global-state-guard.test.ts +++ b/tests/global-state-guard.test.ts @@ -110,6 +110,7 @@ test("a replaced function or listener is reported but left for its owner", () => const before = captureGlobalState(); const fetchDescriptor = Object.getOwnPropertyDescriptor(globalThis, "fetch"); const listener = () => undefined; + const count = process.listenerCount("SIGUSR2"); const fake = (() => Promise.reject(new Error("refused"))) as unknown as typeof fetch; try { @@ -117,7 +118,9 @@ test("a replaced function or listener is reported but left for its owner", () => process.on("SIGUSR2", listener); const changes = diffGlobalState(before); expect(changes).toContain("globalThis.fetch replaced"); - expect(changes).toContain("process 'SIGUSR2' listeners changed"); + expect(changes).toContain( + `process 'SIGUSR2' listeners changed (${count} -> ${count + 1})` + ); restoreGlobalState(before); expect(globalThis.fetch).toBe(fake); expect(process.listeners("SIGUSR2")).toContain(listener); diff --git a/tests/helpers/global-state-guard.ts b/tests/helpers/global-state-guard.ts index 2b47b760e..948de7aff 100644 --- a/tests/helpers/global-state-guard.ts +++ b/tests/helpers/global-state-guard.ts @@ -201,13 +201,15 @@ function diffProcessHooks( ): string[] { const changes: string[] = []; for (const event of GUARDED_PROCESS_EVENTS) { - if ( - !sameListeners( - before.listeners.get(event) ?? [], - current.listeners.get(event) ?? [] - ) - ) { - changes.push(`process '${event}' listeners changed`); + const was = before.listeners.get(event) ?? []; + const now = current.listeners.get(event) ?? []; + if (!sameListeners(was, now)) { + // The counts tell a listener left behind (n -> n+1) from one consumed + // here that an earlier file left behind (n+1 -> n): a `once` listener + // removes itself when the test emits that signal. + changes.push( + `process '${event}' listeners changed (${was.length} -> ${now.length})` + ); } } for (const binding of GUARDED_BINDINGS) { diff --git a/tests/loki-short-lived-logs.test.ts b/tests/loki-short-lived-logs.test.ts index 74092a964..f5a5eb439 100644 --- a/tests/loki-short-lived-logs.test.ts +++ b/tests/loki-short-lived-logs.test.ts @@ -514,3 +514,62 @@ test("Loki follow stays quiet when an entry arrives before the notice delay", as } ); }); + +// --- Loki follow: SIGINT listener ownership (HACK-1392) ---------------------- +// All test files share one Bun process. A follow that leaves its `once` +// SIGINT listener behind makes the next SIGINT in the process run a stale +// stop, and the global-state guard then blames whichever test emits it. + +function sameListeners(left: readonly T[], right: readonly T[]): boolean { + return ( + left.length === right.length && + left.every((listener, index) => listener === right[index]) + ); +} + +const FOLLOW_PARAMS = { + query: '{project="demo",service="api"}', + follow: true, + tail: 10, + pretty: false, + showProjectPrefix: true, +} as const; + +test("Loki follow removes its SIGINT listener once the tail closes", async () => { + const before = process.listeners("SIGINT"); + await withFakeLokiTail( + () => undefined, + async ({ baseUrl, sockets }) => { + captureStdout(); + const exit = lokiLogs({ baseUrl, ...FOLLOW_PARAMS }); + await waitFor(() => sockets.length === 1, "the tail connection"); + expect(process.listenerCount("SIGINT")).toBe(before.length + 1); + for (const socket of sockets) { + socket.close(); + } + expect(await exit).toBe(0); + expect(sameListeners(process.listeners("SIGINT"), before)).toBe(true); + } + ); +}); + +test("Loki follow stops on SIGINT and leaves no listener behind", async () => { + const before = process.listeners("SIGINT"); + await withFakeLokiTail( + () => undefined, + async ({ baseUrl, sockets }) => { + captureStdout(); + const exit = lokiLogs({ baseUrl, ...FOLLOW_PARAMS }); + await waitFor(() => sockets.length === 1, "the tail connection"); + const added = process + .listeners("SIGINT") + .filter((listener) => !before.includes(listener)); + expect(added).toHaveLength(1); + // Run the follow's own handler instead of emitting SIGINT, which would + // reach every listener in the shared test process. + added[0]?.("SIGINT"); + expect(await exit).toBe(0); + expect(sameListeners(process.listeners("SIGINT"), before)).toBe(true); + } + ); +});