test: restore leaked listeners at file boundaries and pin guard-only leak fixes

.ai/skills/hack-repo-verify/SKILL.md

@@ -69,8 +69,12 @@ 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.
+replaced functions, which may still protect unsettled work. It keeps a test's
+leftover listeners until its file ends, then puts the file's listeners back, so
+a later `process.emit` is not blamed for them. Fix the named file's restore
+(`try`/`finally` or `afterEach`), not the later failures. To emit a real signal
+in a test, use `withOwnedProcessListeners` from
+`tests/helpers/owned-process-listeners.ts`.
 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 <KEY> changed`.

tests/global-state-guard.test.ts

@@ -16,7 +16,9 @@ import {
   diffGlobalState,
   diffNativeEnv,
   restoreGlobalState,
+  restoreProcessListeners,
 } from "./helpers/global-state-guard.ts";
+import { withOwnedProcessListeners } from "./helpers/owned-process-listeners.ts";
 
 const PRELOAD = resolve(
   import.meta.dir,
@@ -133,6 +135,52 @@ test("a replaced function or listener is reported but left for its owner", () =>
   expect(diffGlobalState(before)).toEqual([]);
 });
 
+test("the file-boundary listener restore removes leftovers, adds back missing listeners and keeps a kept once listener", async () => {
+  await withOwnedProcessListeners("SIGUSR2", () => {
+    let keptOnceCalls = 0;
+    const keptOnce = () => {
+      keptOnceCalls += 1;
+    };
+    const removed = () => undefined;
+    const leftover = () => undefined;
+    const secondLeftover = () => undefined;
+    process.once("SIGUSR2", keptOnce);
+    process.on("SIGUSR2", removed);
+    try {
+      const before = captureGlobalState();
+      process.removeListener("SIGUSR2", removed);
+      process.on("SIGUSR2", leftover);
+      process.once("SIGUSR2", secondLeftover);
+      expect(diffGlobalState(before)).toContain(
+        "process 'SIGUSR2' listeners changed (2 -> 3)"
+      );
+
+      expect(restoreProcessListeners(before)).toEqual(["SIGUSR2"]);
+
+      expect(sameItems(process.listeners("SIGUSR2"), [keptOnce, removed])).toBe(
+        true
+      );
+      expect(diffGlobalState(before)).toEqual([]);
+      expect(restoreProcessListeners(before)).toEqual([]);
+      // The kept listener was never re-added, so it is still a `once`.
+      process.emit("SIGUSR2");
+      expect(keptOnceCalls).toBe(1);
+      expect(sameItems(process.listeners("SIGUSR2"), [removed])).toBe(true);
+    } finally {
+      for (const listener of [keptOnce, removed, leftover, secondLeftover]) {
+        process.removeListener("SIGUSR2", listener);
+      }
+    }
+  });
+});
+
+function sameItems(left: readonly unknown[], right: readonly unknown[]) {
+  return (
+    left.length === right.length &&
+    left.every((item, index) => item === right[index])
+  );
+}
+
 const LEAK_IN_TEST = `import { test } from "bun:test";
 test("sets PATH and fails before restoring", () => {
   const saved = process.env.PATH;
@@ -159,7 +207,24 @@ test("sets a proxy and deletes it, which Bun's fetch never sees", () => {
 });
 `;
 
+// A once listener that this file never removes. Without the file-boundary
+// restore, it stays in the process and a later test that emits SIGUSR2
+// consumes it and is blamed for the change (HACK-1416).
+const LEAK_LISTENER = `import { test } from "bun:test";
+process.once("SIGUSR2", () => undefined);
+test("passes but leaves a SIGUSR2 listener installed", () => {});
+`;
+
 const LATER = `import { expect, test } from "bun:test";
+test("a later test that emits SIGUSR2 is not charged with an earlier listener", () => {
+  const own = () => undefined;
+  process.on("SIGUSR2", own);
+  try {
+    process.emit("SIGUSR2");
+  } finally {
+    process.off("SIGUSR2", own);
+  }
+});
 test("a later file still finds executables on PATH", () => {
   const child = Bun.spawnSync(["hack-guard-probe"], { env: { ...process.env } });
   expect(child.exitCode).toBe(0);
@@ -206,6 +271,10 @@ async function runNestedSuite(opts: {
     join(suite, "c-leak-native-proxy.test.ts"),
     LEAK_NATIVE_PROXY
   );
+  await writeFile(
+    join(suite, "c2-leak-signal-listener.test.ts"),
+    LEAK_LISTENER
+  );
   await writeFile(join(suite, "d-later.test.ts"), LATER);
   const child = Bun.spawn(
     [
@@ -214,6 +283,7 @@ async function runNestedSuite(opts: {
       "./a-leak-in-test.test.ts",
       "./b-leak-in-hooks.test.ts",
       "./c-leak-native-proxy.test.ts",
+      "./c2-leak-signal-listener.test.ts",
       "./d-later.test.ts",
     ],
     {
@@ -256,7 +326,16 @@ test("the guard names each leaking file, restores state, and later files pass",
   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).toContain(
+    "c2-leak-signal-listener.test.ts left process state changed after the file finished: process 'SIGUSR2' listeners changed (0 -> 1). Restored the environment, cwd and process listeners"
+  );
+  // The leftover listener was removed at the file boundary, so the later
+  // test that emits SIGUSR2 is not blamed for it.
+  expect(run.output).not.toContain("a test in d-later.test.ts");
+  expect(run.output).toContain(
+    "(pass) a later test that emits SIGUSR2 is not charged with an earlier listener"
+  );
+  expect(run.output).toContain("3 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");

tests/helpers/global-state-guard-preload.ts

@@ -2,8 +2,9 @@
  * `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.
+ * fail for an unrelated reason. Replaced functions are reported but kept;
+ * `restoreGlobalState` explains why. Process listeners are reported and kept
+ * per test, and put back at the file boundary (`restoreProcessListeners`).
  *
  * Two checks run:
  * - Per test: the state before the global `beforeEach` must match the state
@@ -14,8 +15,11 @@
  * - 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.
+ *   restore. A mismatch is reported at once, the pre-file environment and
+ *   process listeners are restored, and the run fails from the preload
+ *   `afterAll` with every leaking file. Restoring the listeners keeps a
+ *   finished file's leftover handler from being run or consumed by a later
+ *   test's `process.emit`, which would blame that later test (HACK-1416).
  *   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.
@@ -37,6 +41,7 @@ import {
   type GlobalStateSnapshot,
   type NativeEnv,
   restoreGlobalState,
+  restoreProcessListeners,
 } from "./global-state-guard.ts";
 
 /**
@@ -92,9 +97,10 @@ function checkFileBoundary(): void {
   if (changes.length > 0) {
     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."
+      "Restored the environment, cwd and process listeners from before the file so later files run clean."
     );
     restoreGlobalState(fileBaseline);
+    restoreProcessListeners(fileBaseline);
   }
   checkNativeEnv();
   fileBaseline = captureGlobalState();

tests/helpers/global-state-guard.ts

@@ -15,7 +15,11 @@
  * because fixtures may hold secret-shaped values.
  */
 
-/** Process events whose listener lists are compared (reported, not restored). */
+/**
+ * Process events whose listener lists are compared. A change is reported per
+ * test and per file, and restored only at the file boundary (see
+ * {@link restoreProcessListeners}).
+ */
 export const GUARDED_PROCESS_EVENTS = [
   "SIGINT",
   "SIGTERM",
@@ -278,11 +282,12 @@ function restoreEnvObject(snapshot: GlobalStateSnapshot): void {
  *
  * 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
+ * but deliberately not restored here. 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.
+ * reach the host. Listeners are the exception at a file boundary; see
+ * {@link restoreProcessListeners}.
  */
 export function restoreGlobalState(snapshot: GlobalStateSnapshot): void {
   restoreEnvObject(snapshot);
@@ -295,6 +300,67 @@ export function restoreGlobalState(snapshot: GlobalStateSnapshot): void {
   process.exitCode = snapshot.exitCode;
 }
 
+function countListeners(listeners: readonly Listener[]): Map<Listener, number> {
+  const counts = new Map<Listener, number>();
+  for (const listener of listeners) {
+    counts.set(listener, (counts.get(listener) ?? 0) + 1);
+  }
+  return counts;
+}
+
+/**
+ * Puts the guarded process listeners back to `snapshot` and returns the events
+ * it changed. The preload calls this at a file boundary after it has reported
+ * the change, so a listener a finished file left behind cannot run (or be
+ * consumed by) a later file's `process.emit` and get that later test blamed
+ * (HACK-1416).
+ *
+ * Only the difference is edited: a listener added since the snapshot is
+ * removed and a snapshot listener that went missing is added back with
+ * `process.on`. Listeners present in both stay untouched, so a `once`
+ * listener there keeps its `once` behavior; Bun does not expose `once`, so a
+ * missing listener always comes back as a persistent one, at the end of the
+ * list.
+ *
+ * Unlike replaced functions, removing a leftover listener cannot let unsettled
+ * work reach the host: it only stops that work from reacting to a signal or
+ * exit event of the shared test process.
+ */
+export function restoreProcessListeners(
+  snapshot: GlobalStateSnapshot
+): GuardedProcessEvent[] {
+  const restored: GuardedProcessEvent[] = [];
+  for (const event of GUARDED_PROCESS_EVENTS) {
+    const was = snapshot.listeners.get(event) ?? [];
+    const now = process.listeners(event) as Listener[];
+    if (sameListeners(was, now)) {
+      continue;
+    }
+    restored.push(event);
+    const wanted = countListeners(was);
+    const present = countListeners(now);
+    for (const [listener, count] of present) {
+      for (
+        let extra = count - (wanted.get(listener) ?? 0);
+        extra > 0;
+        extra--
+      ) {
+        process.removeListener(event, listener);
+      }
+    }
+    const kept = countListeners(process.listeners(event) as Listener[]);
+    for (const listener of was) {
+      const left = kept.get(listener) ?? 0;
+      if (left > 0) {
+        kept.set(listener, left - 1);
+      } else {
+        process.on(event, listener);
+      }
+    }
+  }
+  return restored;
+}
+
 /**
  * The environment Bun itself uses for `fetch` proxies and for `Bun.spawn`
  * without an `env` option, as key to value.

tests/helpers/owned-process-listeners.ts

@@ -0,0 +1,32 @@
+/**
+ * Lets a test emit a real process event without reaching listeners it does not
+ * own.
+ *
+ * Every test file shares one Bun process, so `process.emit("SIGINT")` in a
+ * test runs every SIGINT listener in that process, including a leftover from
+ * another file. {@link withOwnedProcessListeners} removes the listeners that
+ * are present, runs the callback, and adds them back afterwards, so the
+ * callback sees only the listeners it installs itself.
+ *
+ * Listeners the callback leaves behind are kept, so the test's own assertions
+ * and the global-state guard still see them. Bun does not expose `once`, so a
+ * saved `once` listener comes back as a persistent one. Between tests the
+ * guarded events normally have no listeners (the guard fails a file that
+ * leaves one), so the saved list is normally empty.
+ */
+export async function withOwnedProcessListeners<T>(
+  event: NodeJS.Signals | "exit",
+  run: () => Promise<T> | T
+): Promise<T> {
+  const saved = process.listeners(event);
+  for (const listener of saved) {
+    process.removeListener(event, listener);
+  }
+  try {
+    return await run();
+  } finally {
+    for (const listener of saved) {
+      process.on(event, listener);
+    }
+  }
+}

tests/helpers/retained-routing-adoption.ts

@@ -552,6 +552,11 @@ else {
       return originalKill.call(process, pid, signal);
     }),
   ];
+  const restoreDoubles = () => {
+    for (const spy of spies) {
+      spy.mockRestore();
+    }
+  };
   const receiptPath = join(
     root,
     ".hack/.internal/legacy-compose-adoption-v1/receipt.json"
@@ -682,14 +687,20 @@ else {
     releaseEnvironment: () => {
       restoreEnv("HACK_HOME", previousHome);
     },
+    /** Whether this fixture's own global doubles are still installed. */
+    doublesInstalled: () => process.kill !== originalKill,
+    /**
+     * Restores this fixture's own spies, and no other spy in the shared Bun
+     * process, whatever the lifetime says. Only for a case that has proven no
+     * work it started is still running; {@link cleanup} is the gated path.
+     */
+    restoreDoubles,
     cleanup: async () => {
       restoreEnv("HACK_HOME", previousHome);
       if (!lifetime.canRestore()) {
         return refuse();
       }
-      for (const spy of spies) {
-        spy.mockRestore();
-      }
+      restoreDoubles();
       await rm(outer, { recursive: true, force: true });
     },
   };

tests/loki-short-lived-logs.test.ts

@@ -20,6 +20,7 @@ import {
   lokiLogs,
   lokiSnapshot,
 } from "../src/ui/loki-logs.ts";
+import { withOwnedProcessListeners } from "./helpers/owned-process-listeners.ts";
 
 // HACK-1277: a one-shot Compose service that exits quickly must not make
 // Loki-backed `hack logs` look empty while `docker compose logs` has output.
@@ -573,3 +574,25 @@ test("Loki follow stops on SIGINT and leaves no listener behind", async () => {
     }
   );
 });
+
+test("Loki follow drops its SIGINT listener through a real SIGINT emit", async () => {
+  // The fixture owns every SIGINT listener, so the emit reaches only the
+  // follow's handler and not another file's or test's.
+  await withOwnedProcessListeners("SIGINT", () =>
+    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(1);
+        process.emit("SIGINT");
+        // The signal path itself removes the `once` listener, before the
+        // tail closes and the follow's `finally` runs.
+        expect(process.listenerCount("SIGINT")).toBe(0);
+        expect(await exit).toBe(0);
+        expect(process.listenerCount("SIGINT")).toBe(0);
+      }
+    )
+  );
+});