Merge pull request #204 from gadget-inc/signal-process-group-later

infiton · web-flow · commit 85366b1decc7 · 2025-08-08T16:12:13.000-04:00
Signal process group later
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "wds",
-  "version": "0.23.0",
+  "version": "0.24.0",
   "author": "Harry Brundage",
   "license": "MIT",
   "bin": {
diff --git a/spec/Supervisor.spec.ts b/spec/Supervisor.spec.ts
@@ -130,3 +130,135 @@ test("it can load a commonjs module inside a directory that contains a dot when
 
   await childExit(child);
 }, 10000);
+
+const runSignalOrderTest = async (signal: NodeJS.Signals) => {
+  const binPath = path.join(dirname, "../pkg/wds.bin.js");
+  const scriptPath = path.join(dirname, "fixtures/src/signal-order.ts");
+
+  const child = spawn("node", [binPath, scriptPath], {
+    stdio: ["ignore", "pipe", "pipe"],
+    env: process.env,
+  });
+
+  let stdout = "";
+  let isReady = false;
+  const onData = (data: Buffer) => {
+    stdout += data.toString();
+    if (!isReady && stdout.includes("parent:ready") && stdout.includes("grandchild:ready")) {
+      isReady = true;
+    }
+  };
+  child.stdout?.on("data", onData);
+
+  await new Promise<void>((resolve) => {
+    const checkReady = () => (isReady ? resolve() : setTimeout(checkReady, 50));
+    checkReady();
+  });
+
+  const wdsPid = child.pid;
+
+
+  child.kill(signal);
+
+  await childExit(child);
+
+  // give any SIGKILLs time to propagate
+  await new Promise((resolve) => setTimeout(resolve, 250));
+
+  let parentPid: number | undefined;
+  let grandchildPid: number | undefined;
+  const lines = stdout.split(/\r?\n/).filter(Boolean).map((line) => {
+    if (line.includes("parent:ready")) {
+      parentPid = parseInt(line.split(":")[2]);
+      return `parent:ready`;
+    } else if (line.includes("grandchild:ready")) {
+      grandchildPid = parseInt(line.split(":")[2]);
+      return `grandchild:ready`;
+    }
+    return line;
+  });
+
+  return { lines, wdsPid, parentPid, grandchildPid };
+}
+
+const isPidAlive = (pid: number) => {
+  try {
+    process.kill(pid, 0);
+    return true;
+  } catch (e) {
+    if (e.code === "ESRCH") {
+      return false;
+    }
+    throw e;
+  }
+}
+
+test("it kills the child first, then the process group on stop", async () => {
+  const { lines, wdsPid, parentPid, grandchildPid } = await runSignalOrderTest("SIGTERM");
+  expect(wdsPid).toBeDefined();
+  expect(parentPid).toBeDefined();
+  expect(grandchildPid).toBeDefined();
+  expect(parentPid).not.toBe(wdsPid);
+  expect(grandchildPid).not.toBe(wdsPid);
+  expect(parentPid).not.toBe(grandchildPid);
+
+  expect(isPidAlive(wdsPid!)).toBe(false);
+  expect(isPidAlive(parentPid!)).toBe(false);
+  expect(isPidAlive(grandchildPid!)).toBe(false);
+
+  expect(lines).toMatchInlineSnapshot(`
+    [
+      "parent:ready",
+      "grandchild:ready",
+      "parent:exit-SIGTERM",
+      "grandchild:sigterm",
+      "parent:grandchild-exit",
+    ]
+  `);
+}, 20000);
+
+test("it kills grandchildren if they have not shutdown by the time the parent process exits", async () => {
+  const { lines, wdsPid, parentPid, grandchildPid } = await runSignalOrderTest("SIGINT");
+  expect(wdsPid).toBeDefined();
+  expect(parentPid).toBeDefined();
+  expect(grandchildPid).toBeDefined();
+  expect(parentPid).not.toBe(wdsPid);
+  expect(grandchildPid).not.toBe(wdsPid);
+  expect(parentPid).not.toBe(grandchildPid);
+
+  expect(isPidAlive(wdsPid!)).toBe(false);
+  expect(isPidAlive(parentPid!)).toBe(false);
+  expect(isPidAlive(grandchildPid!)).toBe(false);
+
+  expect(lines).toMatchInlineSnapshot(`
+    [
+      "parent:ready",
+      "grandchild:ready",
+      "parent:exit-SIGINT",
+      "grandchild:sigint",
+      "parent:exit-timeout",
+    ]
+  `);
+}, 20000);
+
+test("it kills the whole process group if the child process doesn't exit before the timeout", async () => {
+  const { lines, wdsPid, parentPid, grandchildPid } = await runSignalOrderTest("SIGQUIT");
+  expect(wdsPid).toBeDefined();
+  expect(parentPid).toBeDefined();
+  expect(grandchildPid).toBeDefined();
+  expect(parentPid).not.toBe(wdsPid);
+  expect(grandchildPid).not.toBe(wdsPid);
+  expect(parentPid).not.toBe(grandchildPid);
+
+  expect(isPidAlive(wdsPid!)).toBe(false);
+  expect(isPidAlive(parentPid!)).toBe(false);
+  expect(isPidAlive(grandchildPid!)).toBe(false);
+
+  expect(lines).toMatchInlineSnapshot(`
+    [
+      "parent:ready",
+      "grandchild:ready",
+      "parent:sigquit",
+    ]
+  `);
+}, 20000);
diff --git a/spec/fixtures/src/grandchild.js b/spec/fixtures/src/grandchild.js
@@ -0,0 +1,21 @@
+console.log(`grandchild:ready:${process.pid}`);
+
+process.on("SIGINT", () => {
+  // swallow SIGINT and log
+  // let wds kill this process after a timeout
+  console.log("grandchild:sigint");
+});
+
+process.on("SIGQUIT", () => {
+  console.log("grandchild:sigquit");
+});
+
+process.on("SIGTERM", () => {
+  console.log("grandchild:sigterm");
+  process.exit(0);
+});
+
+// Keep alive
+setInterval(() => {}, 1e9);
+
+
diff --git a/spec/fixtures/src/signal-order.ts b/spec/fixtures/src/signal-order.ts
@@ -0,0 +1,38 @@
+import { spawn } from "child_process";
+import * as path from "path";
+
+// Spawn a long-lived grandchild that logs when it gets SIGTERM
+// Resolve from project root (wds runs child with cwd at project root)
+const grandchildPath = path.resolve(process.cwd(), "spec/fixtures/src/grandchild.js");
+const grandchild = spawn("node", [grandchildPath], {
+  stdio: ["ignore", "inherit", "inherit"],
+});
+
+console.log(`parent:ready:${process.pid}`);
+
+const exit = (signal: NodeJS.Signals) => {
+  console.log(`parent:exit-${signal}`);
+
+  grandchild.once("exit", () => {
+    console.log("parent:grandchild-exit");
+    process.exit(0);
+  });
+
+  setTimeout(() => {
+    console.log("parent:exit-timeout");
+    process.exit(0);
+  }, 2000);
+
+  grandchild.kill(signal);
+}
+
+process.on("SIGTERM", () => exit("SIGTERM"));
+process.on("SIGINT", () => exit("SIGINT"));
+process.on("SIGQUIT", () => {
+  console.log("parent:sigquit");
+});
+
+// Keep the process alive indefinitely until signaled
+setInterval(() => {}, 1e9);
+
+
diff --git a/src/Project.ts b/src/Project.ts
@@ -17,6 +17,8 @@ export class Project {
   supervisor!: Supervisor;
   watched = new PathTrie();
 
+  private shuttingDown?: Promise<void>;
+
   constructor(readonly workspaceRoot: string, readonly config: ProjectConfig, readonly compiler: Compiler) {}
 
   addShutdownCleanup(cleanup: () => void) {
@@ -62,11 +64,19 @@ export class Project {
   }
 
   async shutdown(code: number, signal: NodeJS.Signals = "SIGTERM") {
-    await this.supervisor.stop(signal);
-    for (const cleanup of this.cleanups) {
-      cleanup();
+    if (this.shuttingDown) {
+      return await this.shuttingDown;
     }
-    process.exit(code);
+
+    this.shuttingDown = (async () => {
+      await this.supervisor.stop(signal);
+      for (const cleanup of this.cleanups) {
+        cleanup();
+      }
+      process.exit(code);
+    })();
+
+    return await this.shuttingDown;
   }
 
   watchFile(path: string) {
diff --git a/src/Supervisor.ts b/src/Supervisor.ts
@@ -1,56 +1,70 @@
 import type { ChildProcess, StdioOptions } from "child_process";
 import { spawn } from "child_process";
-import { EventEmitter, once } from "events";
-import { setTimeout } from "timers/promises";
+import { EventEmitter } from "events";
 import type { Project } from "./Project.js";
 import type { RunOptions } from "./ProjectConfig.js";
 import { log } from "./utils.js";
 
 /** */
 export class Supervisor extends EventEmitter {
   process!: ChildProcess;
+  private stopping?: Promise<void>;
+
   constructor(readonly argv: string[], readonly socketPath: string, readonly options: RunOptions, readonly project: Project) {
     super();
   }
 
   /**
    * Stop the process with a given signal, then SIGKILL after a timeout
-   * Kills the whole process group so that any subprocesses of the process are also killed
+   * First signals only the ref'd process; once it exits, signal the rest of the process group.
+   * Falls back to SIGKILL on the group if the ref'd process doesn't exit in time.
    * See https://azimi.me/2014/12/31/kill-child_process-node-js.html for more information
    */
   async stop(signal: NodeJS.Signals = "SIGTERM") {
-    // if we never started the child process, we don't need to do anything
-    if (!this.process || !this.process.pid) return;
+    if (this.stopping) {
+      return await this.stopping;
+    }
 
-    // if the child process has already exited, we don't need to do anything
-    if (this.process.exitCode !== null) return;
+    this.stopping = (async () => {
+      // if we never started the child process, we don't need to do anything
+      if (!this.process || !this.process.pid) return;
 
-    const ref = this.process;
-    const exit = once(ref, "exit");
-    this.kill(signal);
+      // if the child process has already exited, we don't need to do anything
+      if (this.process.exitCode !== null) return;
 
-    await Promise.race([exit, setTimeout(5000)]);
-    if (!ref.killed) {
-      this.kill("SIGKILL", ref.pid);
-    }
-  }
+      // signal the child process and give if time to gracefully close, allow the child process
+      // the chance to attempt a graceful shutdown of any child processes it may have spawned
+      // once the child process exits, SIGKILL the rest of the process group that haven't exited yet
+      // if the child process doesn't exit in time, SIGKILL the whole process group
+      const ref = this.process;
+      const refPid = ref.pid;
 
-  kill(signal: NodeJS.Signals = "SIGKILL", pid = this.process?.pid) {
-    if (pid) {
-      try {
-        process.kill(-pid, signal);
-      } catch (error: any) {
-        if (error.code == "ESRCH" || error.code == "EPERM") {
-          // process can't be found or can't be killed again, its already dead
-        } else {
-          throw error;
-        }
+      log.debug(`stopping process ${refPid} with signal ${signal}`);
+
+      this.kill(signal, refPid, false);
+
+      const exited = await Promise.race([
+        new Promise<boolean>((resolve) => ref.once("exit", () => resolve(true))),
+        new Promise<boolean>((resolve) => setTimeout(() => resolve(false), 5000)),
+      ]);
+
+      if (exited) {
+        log.debug(`process ${refPid} exited successfully, killing process group`);
+      } else {
+        log.debug(`process ${refPid} did not exit in time, killing process group`);
       }
-    }
+
+      this.kill("SIGKILL", refPid, true);
+    })();
+
+    return await this.stopping;
   }
 
   restart() {
-    this.kill();
+    if (this.process?.pid) {
+      log.debug(`restarting process group ${this.process.pid} with SIGKILL`);
+      this.kill();
+    }
 
     const stdio: StdioOptions = [null, "inherit", "inherit"];
     if (!this.options.terminalCommands) {
@@ -96,4 +110,23 @@ export class Supervisor extends EventEmitter {
 
     return this.process;
   }
+
+  private kill(signal: NodeJS.Signals = "SIGKILL", pid = this.process?.pid, group = true) {
+    if (!pid) return;
+    if (group) {
+      log.debug(`killing process group ${pid} with signal ${signal}`);
+    } else {
+      log.debug(`killing process ${pid} with signal ${signal}`);
+    }
+    try {
+      if (group) {
+        process.kill(-pid, signal);
+      } else {
+        process.kill(pid, signal);
+      }
+    } catch (error: any) {
+      log.debug(`error killing process ${pid} with signal ${signal}: ${error.message}`);
+      if (error.code !== "ESRCH" && error.code !== "EPERM") throw error;
+    }
+  }
 }
diff --git a/src/index.ts b/src/index.ts
@@ -203,6 +203,10 @@ export const wds = async (options: RunOptions) => {
     log.debug(`process ${process.pid} got SIGTERM`);
     void project.shutdown(0, "SIGTERM");
   });
+  process.on("SIGQUIT", () => {
+    log.debug(`process ${process.pid} got SIGQUIT`);
+    void project.shutdown(0, "SIGQUIT");
+  });
 
   project.supervisor.process.on("exit", (code, signal) => {
     const logShutdown = (explanation: string) => {

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "wds",`
`3`		`- "version": "0.23.0",`
	`3`	`+ "version": "0.24.0",`
`4`	`4`	`"author": "Harry Brundage",`
`5`	`5`	`"license": "MIT",`
`6`	`6`	`"bin": {`