fix(launcher): forward SIGINT/SIGTERM signals to child process (#1145)

The Node.js launcher wrapper (bin/ocr.js) was not forwarding signals
to the Go binary child process, causing orphaned processes when the
parent received SIGINT or SIGTERM.

Changes:
- Replace spawnSync with spawn for asynchronous child process handling
- Add signal handlers for SIGINT and SIGTERM that forward signals to child
- Add error event handler for spawn failures (ENOENT, EACCES)
- Properly propagate child exit status including signal-based termination
- Add integration tests to verify signal forwarding behavior

Fixes issue where Ctrl+C or kill commands left the Go process running
after the Node.js wrapper exited.

Resolves: #1142

.github/workflows/action-contract.yml

@@ -10,7 +10,11 @@ on:
     branches: [main]
     paths:
       - 'action.yml'
+      - 'bin/ocr.js'
+      - 'bin/ocr.test.js'
       - 'package.json'
+      - 'scripts/platform.js'
+      - 'scripts/version.js'
       - 'scripts/github-actions/**'
       - 'examples/github_actions/README.md'
       - '.github/workflows/action-contract.yml'
@@ -18,7 +22,11 @@ on:
     branches: [main]
     paths:
       - 'action.yml'
+      - 'bin/ocr.js'
+      - 'bin/ocr.test.js'
       - 'package.json'
+      - 'scripts/platform.js'
+      - 'scripts/version.js'
       - 'scripts/github-actions/**'
       - 'examples/github_actions/README.md'
       - '.github/workflows/action-contract.yml'
@@ -48,3 +56,6 @@ jobs:
 
       - name: Test GitHub Actions contracts
         run: npm run test:github-actions
+
+      - name: Test launcher contracts
+        run: npm run test:launcher

.github/workflows/ci.yml

@@ -139,6 +139,9 @@ jobs:
       - name: Test
         run: go test -count=1 ./...
 
+      - name: Test Node launcher
+        run: npm run test:launcher
+
       - name: Build
         run: go build -o opencodereview.exe ./cmd/opencodereview
 

bin/ocr.js

@@ -5,7 +5,7 @@
 
 "use strict";
 
-const { spawnSync, spawn } = require("child_process");
+const { spawn } = require("child_process");
 const path = require("path");
 const fs = require("fs");
 const os = require("os");
@@ -25,7 +25,39 @@ function launcherExitCode(result) {
   return result.status ?? (result.error ? 1 : 0);
 }
 
-module.exports = { launcherExitCode };
+// Returns a signal handler that forwards the signal to a child process.
+function forwardSignal(child, signal) {
+  return () => {
+    child.kill(signal);
+  };
+}
+
+function installSignalHandlers(child, platform, signalTarget) {
+  const handlers = [];
+  function add(signal, handler) {
+    handlers.push([signal, handler]);
+    signalTarget.on(signal, handler);
+  }
+
+  // Windows sends console Ctrl+C to both attached processes. Keep the wrapper
+  // alive so it can wait for the binary, but do not call child.kill("SIGINT"):
+  // Node maps that operation to an abrupt TerminateProcess call on Windows.
+  if (platform === "win32") {
+    add("SIGINT", () => {});
+  } else {
+    for (const signal of ["SIGINT", "SIGTERM"]) {
+      add(signal, forwardSignal(child, signal));
+    }
+  }
+
+  return () => {
+    for (const [signal, handler] of handlers) {
+      signalTarget.removeListener(signal, handler);
+    }
+  };
+}
+
+module.exports = { launcherExitCode, forwardSignal, installSignalHandlers };
 
 if (require.main !== module) {
   // Required as a module (tests); the launcher body below must not run.
@@ -77,9 +109,33 @@ if (!process.env.OCR_NO_UPDATE) {
   }
 }
 
-const result = spawnSync(binaryPath, process.argv.slice(2), {
+const child = spawn(binaryPath, process.argv.slice(2), {
   stdio: "inherit",
   env: process.env,
 });
 
-process.exit(launcherExitCode(result));
+const removeSignalHandlers = installSignalHandlers(
+  child,
+  process.platform,
+  process
+);
+
+let settled = false;
+function settle(result) {
+  if (settled) return;
+  settled = true;
+  removeSignalHandlers();
+  process.exitCode = launcherExitCode(result);
+}
+
+// Spawn failures surface through "error", not "close" alone. Setting
+// exitCode lets a piped diagnostic drain before the wrapper terminates.
+child.on("error", (err) => {
+  console.error(`[ERROR] Failed to start ${binaryPath}: ${err.message}`);
+  settle({ error: err });
+});
+
+// Wait for child to exit and propagate its exit status
+child.on("close", (code, signal) => {
+  settle({ status: code, signal: signal });
+});

bin/ocr.test.js

@@ -5,8 +5,9 @@
 
 const assert = require("assert");
 const os = require("os");
+const { EventEmitter } = require("events");
 
-const { launcherExitCode } = require("./ocr");
+const { launcherExitCode, installSignalHandlers } = require("./ocr");
 
 // Normal exits pass the child's status through unchanged.
 assert.strictEqual(launcherExitCode({ status: 0 }), 0);
@@ -33,3 +34,227 @@ assert.strictEqual(
 );
 
 console.log("ocr launcher exit-code tests passed");
+
+// POSIX signals are relayed to the child and all installed handlers are
+// removable once the child has settled.
+{
+  const signalTarget = new EventEmitter();
+  const relayed = [];
+  const remove = installSignalHandlers(
+    { kill: (signal) => relayed.push(signal) },
+    "linux",
+    signalTarget
+  );
+  signalTarget.emit("SIGINT");
+  signalTarget.emit("SIGTERM");
+  assert.deepStrictEqual(relayed, ["SIGINT", "SIGTERM"]);
+  remove();
+  assert.strictEqual(signalTarget.listenerCount("SIGINT"), 0);
+  assert.strictEqual(signalTarget.listenerCount("SIGTERM"), 0);
+}
+
+// On Windows, console Ctrl+C already reaches both attached processes. The
+// launcher keeps itself alive but must not turn SIGINT into TerminateProcess.
+{
+  const signalTarget = new EventEmitter();
+  const relayed = [];
+  const remove = installSignalHandlers(
+    { kill: (signal) => relayed.push(signal) },
+    "win32",
+    signalTarget
+  );
+  signalTarget.emit("SIGINT");
+  assert.deepStrictEqual(relayed, []);
+  assert.strictEqual(signalTarget.listenerCount("SIGTERM"), 0);
+  remove();
+  assert.strictEqual(signalTarget.listenerCount("SIGINT"), 0);
+}
+
+console.log("ocr launcher signal policy tests passed");
+
+const { spawn } = require("child_process");
+const path = require("path");
+const fs = require("fs");
+const {
+  BINARY_FILENAME,
+  getPlatformPackageName,
+} = require("../scripts/platform");
+
+function createLauncherFixture() {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), "ocr-launcher-test-"));
+  const packageName = getPlatformPackageName();
+  assert.ok(packageName, `unsupported test platform: ${process.platform}-${process.arch}`);
+
+  for (const relativePath of [
+    "bin/ocr.js",
+    "scripts/platform.js",
+    "scripts/version.js",
+    "package.json",
+  ]) {
+    const destination = path.join(root, relativePath);
+    fs.mkdirSync(path.dirname(destination), { recursive: true });
+    fs.copyFileSync(path.join(__dirname, "..", relativePath), destination);
+  }
+
+  const packageDir = path.join(root, "node_modules", ...packageName.split("/"));
+  const binaryPath = path.join(packageDir, "bin", BINARY_FILENAME);
+  fs.mkdirSync(path.dirname(binaryPath), { recursive: true });
+  fs.writeFileSync(
+    path.join(packageDir, "package.json"),
+    JSON.stringify({ name: packageName, version: "0.0.0" })
+  );
+
+  if (process.platform === "win32") {
+    fs.copyFileSync(process.execPath, binaryPath);
+  } else {
+    fs.symlinkSync(process.execPath, binaryPath);
+  }
+
+  const home = path.join(root, "home");
+  fs.mkdirSync(home);
+  return {
+    root,
+    home,
+    binaryPath,
+    launcher: path.join(root, "bin", "ocr.js"),
+    nodePath: path.join(root, "node_modules"),
+  };
+}
+
+function writeTarget(fixture, name, source) {
+  const target = path.join(fixture.root, name);
+  fs.writeFileSync(target, source);
+  return target;
+}
+
+function runLauncher(fixture, args, signal, readyToken) {
+  return new Promise((resolve, reject) => {
+    const nodePath = process.env.NODE_PATH
+      ? `${fixture.nodePath}${path.delimiter}${process.env.NODE_PATH}`
+      : fixture.nodePath;
+    const launcher = spawn(process.execPath, [fixture.launcher, ...args], {
+      stdio: ["ignore", "pipe", "pipe"],
+      env: {
+        ...process.env,
+        HOME: fixture.home,
+        USERPROFILE: fixture.home,
+        NODE_PATH: nodePath,
+        OCR_NO_UPDATE: "1",
+      },
+    });
+
+    let output = "";
+    let signalSent = !signal;
+    let finished = false;
+
+    function onData(chunk) {
+      output += chunk.toString();
+      if (!signalSent && output.includes(readyToken)) {
+        signalSent = true;
+        launcher.kill(signal);
+      }
+    }
+
+    launcher.stdout.on("data", onData);
+    launcher.stderr.on("data", onData);
+
+    const guard = setTimeout(() => {
+      if (finished) return;
+      finished = true;
+      launcher.kill("SIGKILL");
+      reject(new Error(`launcher test timed out; output: ${output}`));
+    }, 8000);
+
+    launcher.on("error", (err) => {
+      if (finished) return;
+      finished = true;
+      clearTimeout(guard);
+      reject(err);
+    });
+    launcher.on("close", (code, receivedSignal) => {
+      if (finished) return;
+      finished = true;
+      clearTimeout(guard);
+      resolve({ code, signal: receivedSignal, output, signalSent });
+    });
+  });
+}
+
+async function testExitCodePropagation(fixture) {
+  const target = writeTarget(fixture, "exit-23.js", "process.exit(23);\n");
+  const result = await runLauncher(fixture, [target]);
+  assert.strictEqual(result.code, 23, "launcher must propagate a normal exit code");
+}
+
+async function testGracefulSignalRelay(fixture, signal) {
+  const ready = `TARGET_READY_${signal}`;
+  const received = `TARGET_RECEIVED_${signal}`;
+  const target = writeTarget(
+    fixture,
+    `relay-${signal}.js`,
+    `process.on(${JSON.stringify(signal)}, () => {\n` +
+      `  console.log(${JSON.stringify(received)});\n` +
+      `  process.exit(7);\n` +
+      `});\n` +
+      `console.log(${JSON.stringify(ready)});\n` +
+      `setTimeout(() => process.exit(3), 5000);\n`
+  );
+  const result = await runLauncher(fixture, [target], signal, ready);
+
+  assert.ok(result.signalSent, `${signal}: target never reported ready`);
+  assert.ok(
+    result.output.includes(received),
+    `${signal}: launcher did not relay the signal; output: ${result.output}`
+  );
+  assert.strictEqual(result.code, 7, `${signal}: child exit code was not propagated`);
+}
+
+async function testSignalDeathExitCode(fixture) {
+  const ready = "TARGET_READY_FOR_SIGTERM";
+  const target = writeTarget(
+    fixture,
+    "signal-death.js",
+    `console.log(${JSON.stringify(ready)});\n` +
+      `setTimeout(() => process.exit(3), 5000);\n`
+  );
+  const result = await runLauncher(fixture, [target], "SIGTERM", ready);
+
+  assert.strictEqual(
+    result.code,
+    128 + os.constants.signals.SIGTERM,
+    "launcher must map child signal death to 128 + signo"
+  );
+}
+
+async function testSpawnFailure(fixture) {
+  fs.unlinkSync(fixture.binaryPath);
+  fs.mkdirSync(fixture.binaryPath);
+  const result = await runLauncher(fixture, []);
+
+  assert.strictEqual(result.code, 1, `spawn failure should exit 1; ${result.output}`);
+  assert.ok(
+    result.output.includes("Failed to start"),
+    `spawn failure should be reported; output: ${result.output}`
+  );
+}
+
+(async () => {
+  const fixture = createLauncherFixture();
+  try {
+    await testExitCodePropagation(fixture);
+    if (process.platform === "win32") {
+      console.log("skipping POSIX signal relay cases on win32");
+    } else {
+      await testGracefulSignalRelay(fixture, "SIGINT");
+      await testGracefulSignalRelay(fixture, "SIGTERM");
+      await testSignalDeathExitCode(fixture);
+    }
+    await testSpawnFailure(fixture);
+    console.log("ocr launcher integration tests passed");
+  } finally {
+    fs.rmSync(fixture.root, { recursive: true, force: true });
+  }
+})().catch((err) => {
+  console.error(err.stack || err.message);
+  process.exitCode = 1;
+});

extensions/vscode/src/extension/services/CliService.ts

@@ -115,6 +115,9 @@ export class CliService {
       const proc = spawn(resolveBin(this.cliPath), args, {
         cwd,
         env: envExtra ? { ...getShellEnv(), ...envExtra } : getShellEnv(),
+        // A dedicated POSIX process group lets cancellation escalate without
+        // leaving the native CLI or any of its subprocesses behind.
+        detached: process.platform !== 'win32',
       });
       this.current = proc;
       let stdout = '';
@@ -160,10 +163,34 @@ export class CliService {
 
   cancel(): void {
     if (this.current && this.current.pid) {
-      this.current.kill('SIGTERM');
       const proc = this.current;
+
+      // Windows does not deliver POSIX signals to the launcher. Node maps
+      // kill('SIGTERM') to TerminateProcess, which kills only the launcher and
+      // can orphan its native child. taskkill /T terminates the complete tree.
+      if (process.platform === 'win32') {
+        const treeKill = spawn('taskkill', ['/pid', String(proc.pid), '/t', '/f'], {
+          stdio: 'ignore',
+          windowsHide: true,
+        });
+        const fallback = () => {
+          if (proc.exitCode === null && proc.signalCode === null) proc.kill('SIGKILL');
+        };
+        treeKill.once('error', fallback);
+        treeKill.once('close', (code) => {
+          if (code !== 0) fallback();
+        });
+        return;
+      }
+
+      proc.kill('SIGTERM');
       const forceKillTimer = setTimeout(() => {
-        if (proc.exitCode === null && proc.signalCode === null) proc.kill('SIGKILL');
+        if (proc.exitCode !== null || proc.signalCode !== null) return;
+        try {
+          process.kill(-proc.pid!, 'SIGKILL');
+        } catch (err) {
+          if ((err as NodeJS.ErrnoException).code !== 'ESRCH') proc.kill('SIGKILL');
+        }
       }, 3000);
       proc.once('close', () => clearTimeout(forceKillTimer));
     }

extensions/vscode/src/extension/services/__tests__/CliService.cancel.test.ts

@@ -13,6 +13,11 @@ jest.mock('child_process', () => ({
 }));
 
 const mockedSpawn = jest.mocked(spawn);
+const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform');
+
+function setPlatform(platform: NodeJS.Platform): void {
+  Object.defineProperty(process, 'platform', { value: platform });
+}
 
 function createMockProcess() {
   const proc = Object.assign(new EventEmitter(), {
@@ -33,9 +38,15 @@ function createMockProcess() {
 }
 
 describe('CliService.cancel', () => {
+  beforeEach(() => {
+    jest.spyOn(process, 'kill').mockImplementation(() => true);
+  });
+
   afterEach(() => {
     jest.useRealTimers();
     mockedSpawn.mockReset();
+    jest.restoreAllMocks();
+    if (platformDescriptor) Object.defineProperty(process, 'platform', platformDescriptor);
   });
 
   it('子进程忽略 SIGTERM 时，超时后发送 SIGKILL', () => {
@@ -48,8 +59,15 @@ describe('CliService.cancel', () => {
     svc.cancel();
     jest.advanceTimersByTime(3000);
 
-    expect(proc.kill).toHaveBeenNthCalledWith(1, 'SIGTERM');
-    expect(proc.kill).toHaveBeenNthCalledWith(2, 'SIGKILL');
+    expect(mockedSpawn).toHaveBeenNthCalledWith(
+      1,
+      'node',
+      [],
+      expect.objectContaining({ detached: true }),
+    );
+    expect(proc.kill).toHaveBeenCalledTimes(1);
+    expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+    expect(process.kill).toHaveBeenCalledWith(-123, 'SIGKILL');
   });
 
   it('子进程已正常退出时，不再发送 SIGKILL', async () => {
@@ -67,5 +85,83 @@ describe('CliService.cancel', () => {
 
     expect(proc.kill).toHaveBeenCalledTimes(1);
     expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+    expect(process.kill).not.toHaveBeenCalled();
+  });
+
+  it('falls back to the launcher PID when the POSIX group kill fails', () => {
+    jest.useFakeTimers();
+    jest.mocked(process.kill).mockImplementation(() => {
+      throw Object.assign(new Error('not permitted'), { code: 'EPERM' });
+    });
+    const proc = createMockProcess();
+    mockedSpawn.mockReturnValue(proc as unknown as ReturnType<typeof spawn>);
+
+    const svc = new CliService('node');
+    void svc.runRaw([], '.', () => {});
+    svc.cancel();
+    jest.advanceTimersByTime(3000);
+
+    expect(process.kill).toHaveBeenCalledWith(-123, 'SIGKILL');
+    expect(proc.kill).toHaveBeenNthCalledWith(1, 'SIGTERM');
+    expect(proc.kill).toHaveBeenNthCalledWith(2, 'SIGKILL');
+  });
+
+  it('kills the complete process tree on Windows', () => {
+    setPlatform('win32');
+    const proc = createMockProcess();
+    const treeKill = createMockProcess();
+    mockedSpawn
+      .mockReturnValueOnce(proc as unknown as ReturnType<typeof spawn>)
+      .mockReturnValueOnce(treeKill as unknown as ReturnType<typeof spawn>);
+
+    const svc = new CliService('node');
+    void svc.runRaw([], '.', () => {});
+    svc.cancel();
+
+    expect(mockedSpawn).toHaveBeenNthCalledWith(
+      1,
+      'node',
+      [],
+      expect.objectContaining({ detached: false }),
+    );
+    expect(mockedSpawn).toHaveBeenNthCalledWith(
+      2,
+      'taskkill',
+      ['/pid', '123', '/t', '/f'],
+      { stdio: 'ignore', windowsHide: true },
+    );
+    expect(proc.kill).not.toHaveBeenCalled();
+  });
+
+  it('falls back when taskkill cannot be started on Windows', () => {
+    setPlatform('win32');
+    const proc = createMockProcess();
+    const treeKill = createMockProcess();
+    mockedSpawn
+      .mockReturnValueOnce(proc as unknown as ReturnType<typeof spawn>)
+      .mockReturnValueOnce(treeKill as unknown as ReturnType<typeof spawn>);
+
+    const svc = new CliService('node');
+    void svc.runRaw([], '.', () => {});
+    svc.cancel();
+    treeKill.emit('error', new Error('taskkill unavailable'));
+
+    expect(proc.kill).toHaveBeenCalledWith('SIGKILL');
+  });
+
+  it('falls back when taskkill exits unsuccessfully on Windows', () => {
+    setPlatform('win32');
+    const proc = createMockProcess();
+    const treeKill = createMockProcess();
+    mockedSpawn
+      .mockReturnValueOnce(proc as unknown as ReturnType<typeof spawn>)
+      .mockReturnValueOnce(treeKill as unknown as ReturnType<typeof spawn>);
+
+    const svc = new CliService('node');
+    void svc.runRaw([], '.', () => {});
+    svc.cancel();
+    treeKill.emit('close', 1);
+
+    expect(proc.kill).toHaveBeenCalledWith('SIGKILL');
   });
 });