From 411162cb1b97ada701a498a1f34b4366cc9898f5 Mon Sep 17 00:00:00 2001 From: "open-design-crew[bot]" <299007234+open-design-crew[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 02:57:04 +0000 Subject: [PATCH] [OPEND-4256] fix(sidecar): report a target's signal death as a real signal, not 128+N (#8995) * fix(sidecar): re-raise a target's terminating signal instead of encoding it as 128+N A supervised target that calls process.exit(137) was reported as SIGKILL because the supervisor folded signals into the numeric exit-code space and the packaged observer reversed 128+N back into a signal. The supervisor now terminates itself with the target's unforwarded signal (POSIX), so owners see a real signal; numeric exits pass through untouched and are classified as crashes. * fix(sidecar): restore default disposition before re-raising the target's signal Node handles SIGUSR1 (inspector) and ignores SIGPIPE, so a plain self-signal did not terminate the supervisor and the owner saw exit code 1 instead of the target's signal. Reset the disposition first; cover SIGUSR1/SIGPIPE with the real-supervisor fixture. --------- Co-authored-by: heggria --- apps/packaged/src/sidecars.ts | 8 +- apps/packaged/tests/sidecars.test.ts | 8 +- packages/sidecar/src/index.ts | 3 +- packages/sidecar/src/supervisor-exit.ts | 46 +++++------ packages/sidecar/src/supervisor.ts | 29 ++++++- .../sidecar/tests/fixtures/exiting-target.ts | 23 ++++++ .../sidecar/tests/supervisor-exit.test.ts | 78 ++++++++++++++----- 7 files changed, 135 insertions(+), 60 deletions(-) create mode 100644 packages/sidecar/tests/fixtures/exiting-target.ts diff --git a/apps/packaged/src/sidecars.ts b/apps/packaged/src/sidecars.ts index a84de7a7c6..51560bac08 100644 --- a/apps/packaged/src/sidecars.ts +++ b/apps/packaged/src/sidecars.ts @@ -20,7 +20,6 @@ import { import { getSidecarStatus, invokeSidecar, - signalFromSupervisorExitCode, spawnSidecar, stopSidecar, type SpawnedSidecar, @@ -838,9 +837,10 @@ export function observeDaemonExit( if (exited) return; exited = true; if (!ready) return; - // The observed child is the sidecar supervisor; it encodes a target killed - // by a signal it did not forward as 128 + N (see supervisorExitCode). - const targetSignal = signal ?? signalFromSupervisorExitCode(code); + // The observed child is the sidecar supervisor; it re-raises a target's + // unforwarded terminating signal (see supervisorExitDisposition), so only a + // real signal counts as killed. A numeric exit such as 137 stays a crash. + const targetSignal = signal; const reason = intentional ? 'clean-exit' : targetSignal ? 'killed' : code === 0 ? 'clean-exit' : code !== null ? 'crash' : 'unknown'; const exit = { exit_code: code, signal: targetSignal, intentional, occurred_at: new Date().toISOString(), reason } as const; // Invoke synchronously so the reporter can persist before yielding to network I/O. diff --git a/apps/packaged/tests/sidecars.test.ts b/apps/packaged/tests/sidecars.test.ts index c57d20bcff..bc12137c87 100644 --- a/apps/packaged/tests/sidecars.test.ts +++ b/apps/packaged/tests/sidecars.test.ts @@ -1283,15 +1283,15 @@ describe('ready daemon exit supervision', () => { expect(report).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ exit_code: code, signal, reason, intentional: false, occurred_at: expect.any(String) })); }); - it('classifies a supervisor exit of 128 + SIGKILL as killed (OPEND-4221)', async () => { + it.each([137, 134, 143])('keeps a numeric supervisor exit %s a crash, not a signal (OPEND-4256)', async (code) => { const child = Object.assign(new EventEmitter(), { exitCode: null, signalCode: null }); const report = vi.fn(async () => {}); const observer = observeDaemonExit(child, report); observer.markReady(); - child.emit('exit', 137, null); + child.emit('exit', code, null); await observer.flush(); - expect(report).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ exit_code: 137, signal: 'SIGKILL', - reason: 'killed', intentional: false })); + expect(report).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ exit_code: code, signal: null, + reason: 'crash', intentional: false })); }); it('marks intentional teardown before the signal arrives', async () => { const child = Object.assign(new EventEmitter(), { exitCode: null, signalCode: null }); diff --git a/packages/sidecar/src/index.ts b/packages/sidecar/src/index.ts index e07f62567a..06d0d6ddf8 100644 --- a/packages/sidecar/src/index.ts +++ b/packages/sidecar/src/index.ts @@ -34,7 +34,8 @@ export { resolveSourceRuntimeRoot, } from "./paths.js"; export { allocatePort } from "./port.js"; -export { signalFromSupervisorExitCode, supervisorExitCode } from "./supervisor-exit.js"; +export { supervisorExitDisposition } from "./supervisor-exit.js"; +export type { SupervisorExitDisposition } from "./supervisor-exit.js"; export type { SidecarClientOptions, SidecarConnection, diff --git a/packages/sidecar/src/supervisor-exit.ts b/packages/sidecar/src/supervisor-exit.ts index 2ce7013295..67a0c371e5 100644 --- a/packages/sidecar/src/supervisor-exit.ts +++ b/packages/sidecar/src/supervisor-exit.ts @@ -1,38 +1,28 @@ -import { constants } from "node:os"; - /** - * Exit status the supervisor reports for its target. + * How the supervisor should end once its target has exited. * - * Invariant: the supervisor's own exit must not hide how its target ended. + * Invariant: the supervisor's own exit must report how its target ended + * without folding signals into the numeric exit-code space. * - A stop the supervisor forwarded (SIGINT/SIGTERM from its owner) is an * orderly shutdown and exits 0. - * - A target killed by any other signal (e.g. an external SIGKILL) exits with - * the POSIX shell convention 128 + signal number, so observers that only see - * the supervisor can still tell the target was killed. + * - A target killed by any other signal makes the supervisor terminate itself + * with that same signal (POSIX), so observers see a real `signal`, never a + * number they would have to reverse-engineer. A target that calls + * `process.exit(137)` therefore stays `code=137, signal=null`. + * - Windows has no catchable signal termination; a signal exit there reports 1. * - Otherwise the target's own exit code passes through (unknown → 1). */ -export function supervisorExitCode( +export type SupervisorExitDisposition = + | { kind: "code"; code: number } + | { kind: "signal"; signal: NodeJS.Signals }; + +export function supervisorExitDisposition( code: number | null, signal: NodeJS.Signals | null, forwardedSignal: NodeJS.Signals | null, -): number { - if (forwardedSignal != null) return 0; - if (signal != null) { - const signalNumber = (constants.signals as Record)[signal]; - return signalNumber != null ? 128 + signalNumber : 1; - } - return code ?? 1; -} - -/** - * Inverse of {@link supervisorExitCode} for observers of a supervisor process: - * recovers the target's terminating signal from a 128 + N exit code. - */ -export function signalFromSupervisorExitCode(code: number | null): NodeJS.Signals | null { - if (code == null || code <= 128) return null; - const signalNumber = code - 128; - for (const [name, value] of Object.entries(constants.signals)) { - if (value === signalNumber) return name as NodeJS.Signals; - } - return null; + platform: NodeJS.Platform = process.platform, +): SupervisorExitDisposition { + if (forwardedSignal != null) return { kind: "code", code: 0 }; + if (signal != null) return platform === "win32" ? { kind: "code", code: 1 } : { kind: "signal", signal }; + return { kind: "code", code: code ?? 1 }; } diff --git a/packages/sidecar/src/supervisor.ts b/packages/sidecar/src/supervisor.ts index fa40f4d1df..2273492703 100644 --- a/packages/sidecar/src/supervisor.ts +++ b/packages/sidecar/src/supervisor.ts @@ -14,7 +14,7 @@ import { import type { SidecarGenerationHandoffRequest } from "./client.js"; import { requestJsonIpc } from "./json-ipc.js"; import { retireSupervisedSidecarTargetTree } from "./process-retirement.js"; -import { supervisorExitCode } from "./supervisor-exit.js"; +import { supervisorExitDisposition } from "./supervisor-exit.js"; import { readCurrentSidecarArgvStamp, resolvePrivateIpcPath, @@ -151,7 +151,8 @@ const ownerTimer = ownerPid == null ? null : setInterval(() => { ownerTimer?.unref(); let forwardedSignal: NodeJS.Signals | null = null; -for (const signal of ["SIGINT", "SIGTERM"] as const) { +const forwardingSignals = ["SIGINT", "SIGTERM"] as const; +for (const signal of forwardingSignals) { process.on(signal, () => { forwardedSignal = signal; if (child.pid != null) { @@ -181,7 +182,29 @@ function observeTarget(): void { return; } if (ownerTimer != null) clearInterval(ownerTimer); - process.exitCode = supervisorExitCode(code, signal, forwardedSignal); + const disposition = supervisorExitDisposition(code, signal, forwardedSignal); + if (disposition.kind === "code") { + process.exitCode = disposition.code; + return; + } + // Re-raise the target's terminating signal on the supervisor itself so the + // owner observes a real signal instead of a 128 + N number. + // Fallback if the signal is ignored (e.g. SIGPIPE) or cannot be raised. + process.exitCode = 1; + for (const forwarding of forwardingSignals) process.removeAllListeners(forwarding); + // Node installs its own handlers for some signals (SIGUSR1 starts the + // inspector, SIGPIPE is ignored), so a plain self-signal would not + // terminate. Adding and then removing a listener restores the default + // disposition, making the re-raised signal actually terminate. + try { + process.on(disposition.signal, () => {}); + process.removeAllListeners(disposition.signal); + } catch { + // SIGKILL/SIGSTOP cannot have listeners; they terminate by default. + } + try { + process.kill(process.pid, disposition.signal); + } catch {} }); } diff --git a/packages/sidecar/tests/fixtures/exiting-target.ts b/packages/sidecar/tests/fixtures/exiting-target.ts new file mode 100644 index 0000000000..95d785eb85 --- /dev/null +++ b/packages/sidecar/tests/fixtures/exiting-target.ts @@ -0,0 +1,23 @@ +// Supervised target that ends the way OD_TEST_EXIT_MODE asks, after it has +// announced readiness through OD_TEST_EXIT_READY. +import { writeFileSync } from "node:fs"; + +const mode = process.env.OD_TEST_EXIT_MODE ?? "code:0"; +const ready = process.env.OD_TEST_EXIT_READY; +if (ready != null) writeFileSync(ready, JSON.stringify({ pid: process.pid })); + +const [kind, value = ""] = mode.split(":"); +setTimeout(() => { + if (kind === "code") process.exit(Number(value)); + if (kind === "signal") { + // Reset Node's own SIGUSR1/SIGPIPE handling so the target really dies by + // the signal, like a non-Node target would. + const signal = value as NodeJS.Signals; + try { + process.on(signal, () => {}); + process.removeAllListeners(signal); + } catch {} + process.kill(process.pid, signal); + } +}, 50); +setInterval(() => {}, 1_000); diff --git a/packages/sidecar/tests/supervisor-exit.test.ts b/packages/sidecar/tests/supervisor-exit.test.ts index fcda7492a2..ff2bf20114 100644 --- a/packages/sidecar/tests/supervisor-exit.test.ts +++ b/packages/sidecar/tests/supervisor-exit.test.ts @@ -1,31 +1,69 @@ -import { constants } from "node:os"; +import { once } from "node:events"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; -import { signalFromSupervisorExitCode, supervisorExitCode } from "../src/supervisor-exit.js"; +import type { SidecarStamp } from "../src/index.js"; +import { spawnSidecar, stopSidecar } from "../src/index.js"; +import { supervisorExitDisposition } from "../src/supervisor-exit.js"; -describe("supervisorExitCode", () => { - it("reports a target killed by an external SIGKILL as 128 + N, not a clean exit", () => { - expect(supervisorExitCode(null, "SIGKILL", null)).toBe(128 + constants.signals.SIGKILL); - expect(supervisorExitCode(null, "SIGKILL", null)).not.toBe(0); +describe("supervisorExitDisposition", () => { + it("re-raises an unforwarded terminating signal instead of encoding it as a number", () => { + expect(supervisorExitDisposition(null, "SIGKILL", null, "darwin")).toEqual({ kind: "signal", signal: "SIGKILL" }); + expect(supervisorExitDisposition(null, "SIGTERM", null, "linux")).toEqual({ kind: "signal", signal: "SIGTERM" }); }); it("keeps an orderly stop it forwarded as a clean exit", () => { - expect(supervisorExitCode(null, "SIGTERM", "SIGTERM")).toBe(0); - expect(supervisorExitCode(0, null, "SIGINT")).toBe(0); + expect(supervisorExitDisposition(null, "SIGTERM", "SIGTERM", "darwin")).toEqual({ kind: "code", code: 0 }); + expect(supervisorExitDisposition(0, null, "SIGINT", "darwin")).toEqual({ kind: "code", code: 0 }); }); - it("passes through the target's own exit code", () => { - expect(supervisorExitCode(0, null, null)).toBe(0); - expect(supervisorExitCode(7, null, null)).toBe(7); - expect(supervisorExitCode(null, null, null)).toBe(1); - }); - - it("round-trips the signal for observers", () => { - for (const signal of ["SIGKILL", "SIGTERM", "SIGSEGV", "SIGABRT"] as const) { - expect(signalFromSupervisorExitCode(supervisorExitCode(null, signal, null))).toBe(signal); + it("passes numeric exits through untouched, including 128 + N look-alikes (OPEND-4256)", () => { + for (const code of [0, 7, 134, 137, 143]) { + expect(supervisorExitDisposition(code, null, null, "darwin")).toEqual({ kind: "code", code }); } - expect(signalFromSupervisorExitCode(0)).toBeNull(); - expect(signalFromSupervisorExitCode(7)).toBeNull(); - expect(signalFromSupervisorExitCode(null)).toBeNull(); + expect(supervisorExitDisposition(null, null, null, "darwin")).toEqual({ kind: "code", code: 1 }); + }); + + it("reports a signal death as a non-zero code on Windows", () => { + expect(supervisorExitDisposition(null, "SIGKILL", null, "win32")).toEqual({ kind: "code", code: 1 }); }); }); + +const fixture = fileURLToPath(new URL("./fixtures/exiting-target.ts", import.meta.url)); + +describe.skipIf(process.platform === "win32")("real supervisor exit reporting", () => { + it.each([ + ["code:137", 137, null], + ["code:134", 134, null], + ["code:143", 143, null], + ["code:7", 7, null], + ["code:0", 0, null], + ["signal:SIGKILL", null, "SIGKILL"], + ["signal:SIGTERM", null, "SIGTERM"], + ["signal:SIGUSR1", null, "SIGUSR1"], + ["signal:SIGPIPE", null, "SIGPIPE"], + ] as const)("target %s → supervisor code=%s signal=%s", async (mode, code, signal) => { + const root = await mkdtemp(join(tmpdir(), "open-design-supervisor-exit-")); + const stamp: SidecarStamp = { + channel: "local", namespace: `exit-${process.pid}-${mode.replace(/\W/g, "")}`, + source: "tools-dev", mode: "dev", app: "daemon", + }; + const spawned = await spawnSidecar({ + args: ["--import", "tsx", fixture], + command: process.execPath, + env: { ...process.env, OD_TEST_EXIT_MODE: mode }, + resources: { dataRoot: join(root, "data"), ownerPid: null, port: 0, runtimeRoot: join(root, "runtime") }, + stamp, + }); + try { + const [exitCode, exitSignal] = await once(spawned.process, "exit"); + expect({ code: exitCode, signal: exitSignal }).toEqual({ code, signal }); + } finally { + await stopSidecar(stamp, { killGraceMs: 1_000, termGraceMs: 0 }).catch(() => undefined); + await rm(root, { force: true, recursive: true }); + } + }, 20_000); +});