[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 <bshengtao@gmail.com>

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.

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 });

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,

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<string, number | undefined>)[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 };
 }

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 {}
   });
 }
 

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);

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);
+});