From 3ee12bb84bcef2ce24045a25d4258a089e021445 Mon Sep 17 00:00:00 2001 From: Canitas Date: Sun, 18 Oct 2026 01:49:31 -0700 Subject: [PATCH] fix: stop every process a run started on Windows, whatever its parents On Windows CI, MSYS's bash runs npm's script through a fork that exits, which breaks the parent chain taskkill /T follows, so a stopped or timed-out check left npm and its processes running (#490). Under Bun each process runProcess starts goes into a Windows Job Object, which what it starts joins however its parents come and go, and a stop or time limit terminates the job. Under Node and elsewhere the tree is stopped as before. A test reproduces the case on any machine with a shell that starts a process in the background and exits. --- packages/core/tests/gate.test.ts | 3 +- packages/core/tests/process-tree.test.ts | 45 ++++++++++++++ src/process.ts | 76 ++++++++++++++++++++++-- 3 files changed, 116 insertions(+), 8 deletions(-) create mode 100644 packages/core/tests/process-tree.test.ts diff --git a/packages/core/tests/gate.test.ts b/packages/core/tests/gate.test.ts index 6d3b9dba..2e42e030 100644 --- a/packages/core/tests/gate.test.ts +++ b/packages/core/tests/gate.test.ts @@ -349,8 +349,7 @@ describe("a run that ends before the gate verifies it", () => { // Stopped processes are gone within moments, not the minute they wait: Linux reaps a killed orphan on its own time. const alive = (pid: number): boolean => { try { process.kill(pid, 0); return true; } catch { return false; } }; for (let waited = 0; waited < 10_000 && started.some(alive); waited += 100) await new Promise((resolve) => setTimeout(resolve, 100)); - // Temporary, until #490: where Git's bash execs, as on Windows CI, the stop cannot reach the check's processes. - if (process.platform !== "win32") expect(started.filter(alive)).toEqual([]); + expect(started.filter(alive)).toEqual([]); }, 60_000); test("a run that changed nothing settles with no receipt", async () => { diff --git a/packages/core/tests/process-tree.test.ts b/packages/core/tests/process-tree.test.ts new file mode 100644 index 00000000..bded94e1 --- /dev/null +++ b/packages/core/tests/process-tree.test.ts @@ -0,0 +1,45 @@ +import { afterEach, expect, test } from "bun:test"; +import { existsSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { runProcess } from "../../../src/process.ts"; + +/** + * Stopping a run ends every process it started, even one whose parent has + * already exited (#490): Git's bash runs a script through a process that + * exits, which leaves the script's processes with no living parent to find + * them by. Here the first process starts a shell, `cmd` or `sh`, which starts + * the last in the background and exits, as bash's does. The shell is not a + * Node process: Bun and Node end their own children when they exit, which + * would hide the case. + */ +const left: number[] = []; +const alive = (pid: number): boolean => { try { process.kill(pid, 0); return true; } catch { return false; } }; +afterEach(() => { for (const pid of left.splice(0)) if (alive(pid)) process.kill(pid); }); + +test("a stop ends a process whose parent already exited, as a stopped check's must be", async () => { + const cwd = mkdtempSync(join(tmpdir(), "tesota-core-process-tree-")); + const ready = join(cwd, "ready.txt"); + const shellPid = join(cwd, "shell.txt"); + writeFileSync(join(cwd, "last.cjs"), `require("node:fs").writeFileSync(${JSON.stringify(ready)}, String(process.pid));\nsetTimeout(() => {}, 60000);\n`); + const node = JSON.stringify(process.execPath); + const shell = process.platform === "win32" + ? `spawn("cmd.exe", ["/d", "/c", "start", "/b", "", ${node}, "last.cjs"], { stdio: "ignore" })` + : `spawn("sh", ["-c", ${JSON.stringify(`${JSON.stringify(process.execPath)} last.cjs &`)}], { stdio: "ignore" })`; + writeFileSync(join(cwd, "first.cjs"), `const { spawn } = require("node:child_process");\nconst shell = ${shell};\n` + + `require("node:fs").writeFileSync(${JSON.stringify(shellPid)}, String(shell.pid));\nsetTimeout(() => {}, 60000);\n`); + const controller = new AbortController(); + const running = runProcess(process.execPath, ["first.cjs"], cwd, controller.signal, undefined); + while (!existsSync(ready) || readFileSync(ready, "utf8") === "") await new Promise((resolve) => setTimeout(resolve, 20)); + const last = Number(readFileSync(ready, "utf8")); + left.push(last); + // The stop comes once the shell is gone, so the last process has no living parent. + const between = Number(readFileSync(shellPid, "utf8")); + while (alive(between)) await new Promise((resolve) => setTimeout(resolve, 20)); + expect(alive(last)).toBe(true); + controller.abort(); + expect((await running).ended).toBe("cancelled"); + // Gone within moments, not the minute it waits: Linux reaps a killed orphan on its own time. + for (let waited = 0; waited < 10_000 && alive(last); waited += 100) await new Promise((resolve) => setTimeout(resolve, 100)); + expect(alive(last)).toBe(false); +}, 60_000); diff --git a/src/process.ts b/src/process.ts index 54275a18..e4707bc8 100644 --- a/src/process.ts +++ b/src/process.ts @@ -1,5 +1,5 @@ import { type ChildProcess, spawn } from "node:child_process"; -import { Effect, Option } from "effect"; +import { Effect, Exit, Option } from "effect"; import { join } from "node:path"; /** How a process run ended: it exited, ran past its limit, was stopped, or could not start. */ @@ -43,6 +43,62 @@ function stopTree(pid: number): Promise { return Promise.resolve(); } +/** kernel32's job functions as Bun's FFI binds them (#490). */ +interface Kernel32Jobs { + CreateJobObjectW(attributes: null, name: null): unknown; + OpenProcess(access: number, inherit: number, pid: number): unknown; + AssignProcessToJobObject(job: unknown, process: unknown): number; + TerminateJobObject(job: unknown, exitCode: number): number; + CloseHandle(handle: unknown): number; +} + +/** A failed call's handle: null, or zero as a number or a bigint, as Bun gives a pointer. */ +const isNull = (handle: unknown): boolean => handle === null || handle === 0 || handle === 0n; + +// Named apart so that Node, which has no `bun:ffi` and runs the root tests, never resolves it. +const bunFfi = "bun:ffi"; +let kernel32: Promise | undefined; + +/** + * kernel32's job functions, on Windows under Bun, which runs Tesota; none + * under Node or elsewhere. Loaded once, when a process is first run. + */ +function windowsJobs(): Promise { + kernel32 ??= (async () => { + if (process.platform !== "win32" || process.versions["bun"] === undefined) return undefined; + const { dlopen, FFIType } = await import(bunFfi) as { + dlopen(name: string, symbols: Record): { symbols: Kernel32Jobs }; + FFIType: Record<"ptr" | "u32" | "i32", number>; + }; + return dlopen("kernel32.dll", { + CreateJobObjectW: { args: [FFIType.ptr, FFIType.ptr], returns: FFIType.ptr }, + OpenProcess: { args: [FFIType.u32, FFIType.i32, FFIType.u32], returns: FFIType.ptr }, + AssignProcessToJobObject: { args: [FFIType.ptr, FFIType.ptr], returns: FFIType.i32 }, + TerminateJobObject: { args: [FFIType.ptr, FFIType.u32], returns: FFIType.i32 }, + CloseHandle: { args: [FFIType.ptr], returns: FFIType.i32 }, + }).symbols; + })(); + return kernel32; +} + +/** + * A Windows job holding the process `pid` and every process it starts, which + * inherit it however their parents come and go: Git's bash runs a script + * through a process that exits, which breaks the parent chain `taskkill /T` + * follows (#490). Undefined when the process could not be put in one. + */ +function jobFor(jobs: Kernel32Jobs, pid: number): unknown { + const job = jobs.CreateJobObjectW(null, null); + if (isNull(job)) return undefined; + const processSetQuota = 0x0100, processTerminate = 0x0001; + const handle = jobs.OpenProcess(processSetQuota | processTerminate, 0, pid); + const assigned = !isNull(handle) && jobs.AssignProcessToJobObject(job, handle) !== 0; + if (!isNull(handle)) jobs.CloseHandle(handle); + if (assigned) return job; + jobs.CloseHandle(job); + return undefined; +} + /** What a run collects as it goes: the end of what it printed, and the start of each stream apart. */ interface Collected { output: string; stdout: string; stderr: string } @@ -77,6 +133,7 @@ export async function runProcess(executable: string, args: readonly string[], cw if (got[stream].length < streamLimit) got[stream] = (got[stream] + text).slice(0, streamLimit); onOutput?.(chunk); }; + const jobs = await windowsJobs(); const start = Effect.sync(() => { const child = spawn(executable, [...args], { cwd, windowsHide: true, detached: process.platform !== "win32", stdio: [options.input === undefined ? "ignore" : "pipe", "pipe", "pipe"], @@ -88,12 +145,19 @@ export async function runProcess(executable: string, args: readonly string[], cw child.stdin.on("error", () => undefined); child.stdin.end(options.input); } - return child; + // Put in its job before it can start anything, so all it starts is in the job too. + return { child, job: jobs === undefined || child.pid === undefined ? undefined : jobFor(jobs, child.pid) }; }); - // Whatever ended the run, a process still running is stopped with its tree before the run reports. - const stop = (child: ChildProcess) => Effect.promise(() => child.exitCode === null && child.signalCode === null && child.pid !== undefined - ? stopTree(child.pid) : Promise.resolve()); - const run = Effect.acquireUseRelease(start, (child) => { + // A run a stop or the time limit ended has its processes stopped before it reports: on Windows its whole job, however + // their parents came and went; elsewhere, or without a job, its tree. One that ended by itself leaves what outlived it. + const stop = ({ child, job }: { child: ChildProcess; job: unknown }, exit: Exit.Exit, Error>) => + Effect.promise(async () => { + const stopped = !Exit.isSuccess(exit) || Option.isNone(exit.value) || (child.exitCode === null && child.signalCode === null); + if (stopped && jobs !== undefined && job !== undefined) jobs.TerminateJobObject(job, 1); + else if (stopped && child.pid !== undefined) await stopTree(child.pid); + if (jobs !== undefined && job !== undefined) jobs.CloseHandle(job); + }); + const run = Effect.acquireUseRelease(start, ({ child }) => { const ended = closed(child); return timeoutMs === undefined ? Effect.map(ended, Option.some) : Effect.timeoutOption(ended, timeoutMs); }, stop);