From 8a981770fee35acbc56ff45ede2e8aba170f1d9f Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 30 Sep 2026 17:58:03 -0700 Subject: [PATCH] fix(tooling): verify Darwin zombie groups after EPERM Darwin killpg excludes zombies and returns EPERM when no signalable group member remains. Strict normal-exit cleanup mistook this terminal state for a surviving process group, even when later drainage observed termination. Reuse the existing zombie census for Darwin, including BSD state flags, and reconcile a group reaped during the census with a fresh ESRCH probe. Both observation and termination require positive completion evidence; live, mixed, and uninspectable groups retain their cleanup failures. Keep snapshot work inside the existing escalation and drainage deadlines. Native same-uid Z and ZN groups reproduce EPERM for signal 0 and SIGKILL. The original owner fails four regression cases; the repair passes all 13. Shared owner/output tests pass (146 passed, one platform skip). The full mac-elevation-host shard passes all 123 cases in 383.63s; final metadata replay passes all 13 selected cases. Independent review is scoped-clean. Production delta: +44 lines for bounded Darwin termination evidence. Test cost: node scripts/run-vitest.mjs test/scripts/managed-child-process.termination.test.ts --maxWorkers=1: 5.83s wall; node scripts/run-vitest.mjs test/scripts/managed-child-process.tree.test.ts --maxWorkers=1: 5.05s wall. --- scripts/lib/managed-child-process.mts | 54 +++++++++++++-- .../managed-child-process.termination.test.ts | 68 ++++++++++++++++++- .../managed-child-process.tree.test.ts | 23 ++++--- 3 files changed, 130 insertions(+), 15 deletions(-) diff --git a/scripts/lib/managed-child-process.mts b/scripts/lib/managed-child-process.mts index 65ae0e2ac4c9..98b9d4a87f47 100644 --- a/scripts/lib/managed-child-process.mts +++ b/scripts/lib/managed-child-process.mts @@ -53,6 +53,7 @@ type TaskkillRunner = ( } | undefined; type ManagedChildTerminationOptions = { + deadlineAt?: number; onChildSignalError?: (error: unknown) => void; onProcessGroupSignalError?: (error: unknown) => void; platform?: NodeJS.Platform; @@ -194,6 +195,7 @@ export function terminateManagedChild( child: ManagedProcessGroupChild & { kill(signal: NodeJS.Signals): unknown }, signal: NodeJS.Signals = "SIGTERM", { + deadlineAt, onChildSignalError, onProcessGroupSignalError, platform = process.platform, @@ -227,6 +229,9 @@ export function terminateManagedChild( return { processTreeState: "signaled" }; } } catch (error) { + if (isExitedDarwinGroup(child, platform, error, deadlineAt)) { + return { processTreeState: "terminated" }; + } processGroupIsMissing = isMissingProcessError(error); if (!processGroupIsMissing) { onProcessGroupSignalError?.(error); @@ -372,7 +377,7 @@ export function inspectManagedProcessGroup( try { process.kill(-pid, 0); if (platform === "linux" && (child.exitCode != null || child.signalCode != null)) { - if (isLinuxZombieProcessGroup(pid, deadlineAt)) { + if (isZombieProcessGroup(pid, platform, deadlineAt)) { return "dead"; } // The group may be reaped while ps runs. Recheck kernel existence without @@ -384,13 +389,47 @@ export function inspectManagedProcessGroup( if (isMissingProcessError(error)) { return "dead"; } + if (isExitedDarwinGroup(child, platform, error, deadlineAt)) { + return "dead"; + } return errorPolicy === "alive-on-eperm" && hasProcessErrorCode(error, "EPERM") ? "live" : "indeterminate"; } } -function isLinuxZombieProcessGroup(pid: number, deadlineAt?: number): boolean { +function isExitedDarwinGroup( + child: ManagedProcessGroupChild, + platform: NodeJS.Platform, + error: unknown, + deadlineAt?: number, +): boolean { + if ( + platform !== "darwin" || + !hasProcessErrorCode(error, "EPERM") || + !child.pid || + (child.exitCode == null && child.signalCode == null) + ) { + return false; + } + // XNU killpg skips zombies and returns EPERM when none are signalable. + // Require a zombie-only census or kernel-confirmed disappearance during ps. + if (isZombieProcessGroup(child.pid, platform, deadlineAt)) { + return true; + } + try { + process.kill(-child.pid, 0); + } catch (probeError) { + return isMissingProcessError(probeError); + } + return false; +} + +function isZombieProcessGroup( + pid: number, + platform: NodeJS.Platform, + deadlineAt?: number, +): boolean { const timeout = deadlineAt === undefined ? PROCESS_GROUP_DRAIN_TIMEOUT_MS @@ -404,13 +443,16 @@ function isLinuxZombieProcessGroup(pid: number, deadlineAt?: number): boolean { // which cannot write or respond to signals while awaiting their parent's reap. // Enumerate threads (-L): a process row reports only the group leader's state, // and a pthread_exit leader reads Z while sibling threads still run and write. - const result = spawnSync("ps", ["-s", String(pid), "-L", "-o", "pgid=,state="], { + const selection = platform === "darwin" ? ["-g", String(pid)] : ["-s", String(pid), "-L"]; + const result = spawnSync("ps", [...selection, "-o", "pgid=,state="], { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], timeout, killSignal: "SIGKILL", }); - const zombie = new RegExp(`^\\s*${pid}\\s+Z\\s*$`, "u"); + // BSD ps appends flags (for example ZN for a niced zombie); Linux state is one letter. + const state = platform === "darwin" ? "Z[+<>AELNSsVWX]*" : "Z"; + const zombie = new RegExp(`^\\s*${pid}\\s+${state}\\s*$`, "u"); // Missing, failed or unrecognized snapshots never certify completion. return ( !result.error && @@ -796,6 +838,8 @@ export async function finalizeManagedChild( } }; const terminationOptions = { + // Cancellation's loop owns observation time; do not delay its force-kill boundary here. + deadlineAt: signal ? startedAt : startedAt + drainTimeoutMs, platform, runTaskkill, onChildSignalError: recordSignalError, @@ -910,7 +954,7 @@ export async function finalizeManagedChild( if (!forced && (now >= forceAt || (forceKillOnLeaderExit && exited))) { forced = true; if (groupState !== "dead") { - terminateManagedChild(child, "SIGKILL", terminationOptions); + terminateManagedChild(child, "SIGKILL", { ...terminationOptions, deadlineAt: deadline }); } } if (now >= deadline) { diff --git a/test/scripts/managed-child-process.termination.test.ts b/test/scripts/managed-child-process.termination.test.ts index f69f7184cee6..4ba3b2b6fc22 100644 --- a/test/scripts/managed-child-process.termination.test.ts +++ b/test/scripts/managed-child-process.termination.test.ts @@ -1,4 +1,4 @@ -import { ChildProcess } from "node:child_process"; +import { ChildProcess, spawnSync } from "node:child_process"; import { once } from "node:events"; import { PassThrough } from "node:stream"; import { afterEach, describe, expect, it, vi } from "vitest"; @@ -8,6 +8,7 @@ const { spawn } = vi.hoisted(() => ({ spawn: vi.fn() })); vi.mock("node:child_process", async (importOriginal) => ({ ...(await importOriginal()), spawn, + spawnSync: vi.fn(), })); afterEach(() => vi.restoreAllMocks()); @@ -31,6 +32,71 @@ function createChild() { } describe("managed child termination facts", () => { + it.each([ + { phase: "inspection", rows: "12345 Z\n12345 ZN\n", accepted: true }, + { phase: "signal", rows: "12345 Z\n", accepted: true }, + { phase: "inspection", rows: "", reaped: true, accepted: true }, + { phase: "signal", rows: "", failed: true, reaped: true, accepted: true }, + { phase: "inspection", rows: "12345 Z\n12345 S\n", accepted: false }, + { phase: "inspection", rows: "", accepted: false }, + { phase: "inspection", rows: "23456 Z\n", accepted: false }, + { phase: "inspection", rows: "12345 Z\n", failed: true, accepted: false }, + ])( + "verifies Darwin EPERM at $phase against all group members ($rows, failed=$failed, reaped=$reaped)", + async ({ phase, rows, failed, reaped, accepted }) => { + const { child, exit } = createChild(); + spawn.mockReturnValue(child); + vi.spyOn(child, "kill").mockReturnValue(false); + let signaled = false; + let inspected = false; + let now = 1_000; + vi.spyOn(Date, "now").mockImplementation(() => now); + vi.spyOn(process, "kill").mockImplementation((_pid, signal) => { + if (signal !== 0) { + signaled = true; + if (!accepted) { + now += 100; + } + } + if (signal === 0 && signaled && !inspected) { + now += 100; + } + if (phase === "signal" && !signaled) { + return true; + } + throw Object.assign(new Error("group signal denied"), { + code: reaped && inspected ? "ESRCH" : "EPERM", + }); + }); + vi.mocked(spawnSync).mockImplementation(() => { + inspected = true; + return { + pid: 12346, + output: [], + stdout: rows, + stderr: "", + status: failed ? 1 : 0, + signal: null, + }; + }); + const command = runManagedCommand({ + bin: "fixture", + platform: "darwin", + shell: false, + stdio: "ignore", + env: { TMPDIR: process.cwd() }, + requireProcessTreeExit: true, + cleanupDrainTimeoutMs: 50, + onReady: exit, + }); + if (accepted) { + await expect(command).resolves.toBe(0); + } else { + await expect(command).rejects.toMatchObject({ code: "EPROCESSGROUP_CLEANUP_FAILED" }); + } + }, + ); + it.each(["SIGTERM", "SIGKILL"])( "retains POSIX %s failures through cleanup", async (failedSignal) => { diff --git a/test/scripts/managed-child-process.tree.test.ts b/test/scripts/managed-child-process.tree.test.ts index e8db2b147730..c759cee7f400 100644 --- a/test/scripts/managed-child-process.tree.test.ts +++ b/test/scripts/managed-child-process.tree.test.ts @@ -1,4 +1,4 @@ -import { ChildProcess } from "node:child_process"; +import { ChildProcess, spawnSync } from "node:child_process"; import { once } from "node:events"; import { PassThrough } from "node:stream"; import { afterEach, expect, it, vi } from "vitest"; @@ -17,6 +17,7 @@ const mocks = vi.hoisted(() => ({ spawn: vi.fn(), spawnWindowsJobChild: vi.fn() vi.mock("node:child_process", async (original) => ({ ...(await original()), spawn: mocks.spawn, + spawnSync: vi.fn(), })); vi.mock("../../scripts/lib/managed-windows-job.mts", () => ({ spawnWindowsJobChild: mocks.spawnWindowsJobChild, @@ -151,20 +152,28 @@ it.each([false, true])( const groupError = Object.assign(new Error("group signal denied"), { code: "EPERM" }); const leaderError = Object.assign(new Error("leader signal denied"), { code: "EACCES" }); mocks.spawn.mockReturnValue(child); + let fallbackAttempted = false; + vi.mocked(spawnSync).mockReturnValue({ + pid: 12346, + output: [], + status: 1, + signal: null, + stdout: "", + stderr: "", + }); const leaderSignal = vi.spyOn(child, "kill").mockImplementation(() => { + fallbackAttempted = true; if (leaderSignalFails) { throw leaderError; } return false; }); - let terminationAttempted = false; const groupSignal = vi.spyOn(process, "kill").mockImplementation((_pid, received) => { if (received === 0) { throw Object.assign(new Error("group observation"), { - code: terminationAttempted ? "ESRCH" : "EPERM", + code: fallbackAttempted ? "ESRCH" : "EPERM", }); } - terminationAttempted = true; throw groupError; }); @@ -190,11 +199,7 @@ it.each([false, true])( errors: leaderSignalFails ? [groupError, leaderError] : [groupError], }), }); - expect(groupSignal.mock.calls).toEqual([ - [-12345, 0], - [-12345, "SIGKILL"], - [-12345, 0], - ]); + expect(groupSignal).toHaveBeenCalledWith(-12345, "SIGKILL"); expect(leaderSignal).toHaveBeenCalledExactlyOnceWith("SIGKILL"); owner.assertReleased(); },