Fix exiting of a hanging sub-command (#115)

* Using a Set to store cleanup tasks and remove command killing on "close"

* Adding a hanging command test

* Adding a test ensuring the killing of sub-process when interrupting the CLI

* Filtering stacks server-side

* Waiting for cli to close

* Fixing "interrupted" and "hanging" tests

packages/cli/src/index.test.ts

@@ -1,15 +1,28 @@
-import cp from "child_process";
+import cp from "node:child_process";
+import { URL } from "node:url";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import process from "node:process";
+
 import { expect } from "chai";
 import { it } from "mocha";
-import { URL } from "url";
 
 const cliPath = new URL("./cli.ts", import.meta.url).pathname;
 
 function cli(...args: string[]) {
   return cp.spawnSync(
     process.execPath,
     ["--import", "tsx", cliPath, ...args],
-    { encoding: 'utf8', env: { ...process.env, FORCE_COLOR: "false" }, timeout: 4_000 },
+    { encoding: 'utf8', env: { ...process.env, FORCE_COLOR: "false" }, timeout: 5_000 },
+  );
+}
+
+function spawnCli(...args: string[]) {
+  return cp.spawn(
+    process.execPath,
+    ["--import", "tsx", cliPath, ...args],
+    { stdio: "ignore", env: { ...process.env, FORCE_COLOR: "false" }, timeout: 5_000 },
   );
 }
 
@@ -19,6 +32,41 @@ function parseJsonOutput(output: string) {
   return JSON.parse(output.slice(jsonStart, jsonEnd + 1));
 }
 
+function waitForFile(filePath: string) {
+  return new Promise<string>(resolve => {
+    const listener = (stats: fs.Stats) => {
+      if (stats.isFile()) {
+        // Stop listening for changes
+        fs.unwatchFile(filePath, listener);
+        resolve(fs.readFileSync(filePath, "utf8"));
+      }
+    };
+    fs.watchFile(filePath, { interval: 10 }, listener);
+    if (fs.existsSync(filePath)) {
+      // Start by doing it once
+      listener(fs.statSync(filePath));
+    }
+  });
+}
+
+function isProcessRunning(pid: number) {
+  // Tried using kill to assert the process has exited:
+  // > This method will throw an error if the target pid does not exist.
+  // > As a special case, a signal of 0 can be used to test for the existence of a process.
+  // see https://nodejs.org/api/process.html#processkillpid-signal
+  // process.kill(pid, 0);
+  // ... but this didn't throw on Linux - instead we're calling good ol' "ps"
+  const result = cp.spawnSync("ps", ["-p", pid.toString(), "-o", "pid"], { encoding: "utf8" });
+  const [, match] = result.stdout.trim().split("\n");
+  return match === pid.toString();
+}
+
+async function waitForProcessToDie(pid: number, pollDelay = 100) {
+  while (isProcessRunning(pid)) {
+    await new Promise(resolve => setTimeout(resolve, pollDelay));
+  }
+}
+
 describe("Mocha Remote CLI", () => {
   it("prints --help", () => {
     const output = cli("--help");
@@ -102,6 +150,46 @@ describe("Mocha Remote CLI", () => {
     expect(output.status).equals(0);
   });
 
+  it("kills unresponsive command when interrupted", async () => {
+    const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "mocha-remote-cli-test-"));
+    const outFile = path.join(tempDir, "out.txt");
+    expect(fs.existsSync(outFile)).equals(false, "Expected the outfile to be missing");
+    const cliProcess = spawnCli("--port", "0", "--context", `endlessLoop,outFile=${outFile}`,  "--", "tsx", "src/test/hanging-client.ts");
+
+    const outFileContent = await waitForFile(outFile);
+    const success = cliProcess.kill("SIGINT"); // Interrupt Mocha Remote CLI
+    expect(success).equals(true, "Expected mocha-remote cli to exit when interrupted");
+
+    // Wait a bit for Mocha Remote CLI to forcefully kill the command process
+    await new Promise(resolve => cliProcess.once("close", resolve));
+    expect(cliProcess.exitCode).equals(0);
+
+    // Expect the command process to have died
+    const { pid } = JSON.parse(outFileContent);
+    expect(typeof pid).equals("number");
+    await waitForProcessToDie(pid);
+  });
+
+  it("exits clean and kills hanging commands", async () => {
+    const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "mocha-remote-cli-test-"));
+    const outFile = path.join(tempDir, "out.txt");
+    expect(fs.existsSync(outFile)).equals(false, "Expected the outfile to be missing");
+    // Using spawnCli because it ignores stdout, which makes the spawnSync hang for some reason
+    const cliProcess = spawnCli("--port", "0", "--context", `outFile=${outFile}`,  "--", "tsx", "src/test/hanging-client.ts");
+    await new Promise<void>(resolve => cliProcess.once("close", resolve));
+
+    expect(cliProcess.exitCode).equals(0, "Expected signal of the mocha-remote cli to remain a success");
+
+    // Checking the "exit" of the wrapped command
+    expect(fs.existsSync(outFile)).equals(true, "Expected the outfile to exists");
+    const outFileContent = await waitForFile(outFile);
+
+    // Expect the command process to have died
+    const { pid } = JSON.parse(outFileContent);
+    expect(typeof pid).equals("number");
+    await waitForProcessToDie(pid);
+  });
+
   describe("failures", () => {
     it("propagates failures as exit code", () => {
       const output = cli("--port", "0", "--context", "failure=Totally expected", "--", "tsx", "src/test/simple-client.ts");

packages/cli/src/index.ts

@@ -1,24 +1,28 @@
-import fs from "fs";
+import assert from "node:assert";
+import fs from "node:fs";
+import cp from "node:child_process";
+import { inspect } from "node:util";
+
 import yargs from 'yargs';
 import { hideBin } from 'yargs/helpers';
-import cp from "child_process";
 import chalk from "chalk";
-import { inspect } from "util";
+
+import { Server, ReporterOptions, CustomContext, WebSocket, ClientError } from "mocha-remote-server";
 
 const packageJsonPath = new URL("../package.json", import.meta.url);
 const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, "utf8"));
 
-import { Server, ReporterOptions, CustomContext, WebSocket, ClientError } from "mocha-remote-server";
-
 export type Logger = (...args: unknown[]) => void;
 
 export type CleanupTask = () => (void | Promise<void>);
-const cleanupTasks: CleanupTask[] = [];
+const cleanupTasks = new Set<CleanupTask>();
 
-async function cleanup() {
+function cleanup() {
   // Move the tasks into a local variable to avoid rerunning these on multiple invokations
-  const tasks = cleanupTasks.splice(0, cleanupTasks.length);
-  await Promise.all(tasks.map(task => task())).catch(err => {
+  const tasks = [...cleanupTasks];
+  cleanupTasks.clear();
+  // Execute a chain of promises
+  tasks.reduce((previous, task) => previous.then(task), Promise.resolve()).catch(err => {
     // eslint-disable-next-line no-console
     console.error(`Failed while cleaning up: ${err}`);
   });
@@ -76,10 +80,10 @@ type ServerOptions = {
 
 // Start the server
 export async function startServer({ log, server, command, exitOnError }: ServerOptions): Promise<void> {
-  cleanupTasks.push(async () => {
+  cleanupTasks.add(async () => {
     if (server.listening) {
-      log("🧹 Stopping server");
       await server.stop();
+      log("🧹 Stopped the server");
     }
   });
 
@@ -109,9 +113,7 @@ export async function startServer({ log, server, command, exitOnError }: ServerO
     if (code !== 1000 && exitOnError) {
       print(displayClient(ws), chalk.red("EXITTING"), "after an abnormal disconnect");
       process.exitCode = 1;
-      cleanup().then(() => {
-        process.exit();
-      });
+      cleanup();
     }
   });
 
@@ -158,19 +160,19 @@ export async function startServer({ log, server, command, exitOnError }: ServerO
         MOCHA_REMOTE_ID: server.config.id,
       }
     });
+    const commandPid = commandProcess.pid;
+    assert(typeof commandPid === "number", "Expected command process to have a pid");
 
-    const commandDescription = `${chalk.italic(...command)} (pid = ${chalk.bold(commandProcess.pid)})`
+    const commandDescription = `${chalk.italic(...command)} (pid = ${chalk.bold(commandPid)})`
 
-    cleanupTasks.push(() => {
-      if (commandProcess.connected) {
-        log(`🧹 Terminating: ${commandDescription}`);
-        const success = commandProcess.kill();
-        if (!success) {
-          log(`💀 Sending SIGKILL to pid = ${commandProcess.pid}`);
-          commandProcess.kill("SIGKILL");
-        }
-      }
-    });
+    const killCommandProcess = async () => {
+      // There's no need to await a close if the close was caused by a cleanup
+      commandProcess.removeListener("close", commandCloseListener);
+      commandProcess.kill();
+      log(`🧹 Terminated command (pid = ${chalk.bold(commandPid)})`);
+    };
+
+    cleanupTasks.add(killCommandProcess);
 
     /* eslint-disable-next-line no-console */
     log(
@@ -179,23 +181,23 @@ export async function startServer({ log, server, command, exitOnError }: ServerO
     );
 
     // We should exit when the command process exits - and vise versa
-    commandProcess.once("close", (code, signal) => {
+    const commandCloseListener = (code: number | null, signal: string | null) => {
+      // Avoid killing the process if it's already closed
+      cleanupTasks.delete(killCommandProcess);
+      // Print the exit and cause to the log
       const cause = exitCause(code, signal);
       log('\n' + chalk.dim(`Command exited (${cause})`));
       // Inherit exit code from sub-process if nothing has already sat it
       if (typeof code === "number" && typeof process.exitCode !== "number") {
         process.exitCode = code;
       }
       cleanup();
-    });
+    };
 
-    process.on("SIGINT", () => {
-      commandProcess.kill("SIGINT");
-    });
-
-    process.on("exit", () => {
-      cleanup();
-    });
+    commandProcess.once("close", commandCloseListener);
+
+    process.on("SIGINT", cleanup);
+    process.on("exit", cleanup);
   }
 }
 
@@ -344,6 +346,7 @@ export function run(args = hideBin(process.argv)): void {
             // Run once and exit with the failures as exit code
             server.run(failures => {
               process.exitCode = failures;
+              cleanup();
             });
           }
         },

packages/cli/src/test/hanging-client.ts

@@ -0,0 +1,24 @@
+import { Client } from "mocha-remote-client";
+
+import assert from "node:assert";
+import fs from "node:fs";
+
+new Client({
+  tests({ outFile, endlessLoop }) {
+    assert(typeof outFile === "string", "Expected a 'outFile' in context");
+    assert(fs.existsSync(outFile) === false, `Expected '${outFile}' to not exist`);
+    fs.writeFileSync(outFile, JSON.stringify({ pid: process.pid }), "utf8");
+
+    if (endlessLoop) {
+      // eslint-disable-next-line no-constant-condition
+      while (true) {
+        // Useful to simulate a process that cannot be exited gracefully
+      }
+    }
+
+    it("succeeds but doesn't exit", () => {});
+  }
+});
+
+// Do a long timeout to prevent an exit
+setTimeout(() => {}, 60 * 1000);

packages/client/src/serialization.ts

@@ -15,7 +15,7 @@ function toJSON(value: Record<string, unknown>): Record<string, unknown> {
     return {
       type: "error",
       message,
-      stack: stack ? filterStack(stack) : stack,
+      stack,
     };
   } else if (typeof value === "object" && typeof value.serialize === "function") {
     const result = value.serialize();
@@ -32,10 +32,6 @@ function toJSON(value: Record<string, unknown>): Record<string, unknown> {
   }
 }
 
-function filterStack(stack: string) {
-  return stack.split("\n").filter(line => line.includes("client/dist/node.bundle.cjs.js") === false).join("\n");
-}
-
 export function createReplacer(): Replacer {
   return function(this: unknown, key: string, value: unknown) {
     debug(`Replacing %s`, value);

packages/integration-tests/src/basic.test.ts

@@ -22,6 +22,7 @@ describe("basic", () => {
     await server.start();
     expect(server.url).to.oneOf([
       "ws://localhost:8090",
+      "ws://127.0.0.1:8090",
       "ws://[::1]:8090"
     ]);
   });

packages/server/src/Server.ts

@@ -143,7 +143,7 @@ export class Server extends ServerEventEmitter {
     this.debug("Server is stopping");
     await new Promise<void>((resolve, reject) => {
       if (this.wss) {
-        // Terminate all clients
+        // Close any client connections, giving a code and reason
         for (const ws of this.wss.clients) {
           ws.close(code, reason);
         }
@@ -158,6 +158,10 @@ export class Server extends ServerEventEmitter {
             resolve();
           }
         });
+        // Terminate any clients still connected, allowing the server to close
+        for (const ws of this.wss.clients) {
+          ws.terminate();
+        }
       } else {
         resolve();
       }

packages/server/src/serialization.test.ts

@@ -1,8 +1,8 @@
 import { expect } from "chai";
-import { Suite, Test } from "mocha";
+import { Test } from "mocha";
 import { stringify } from "flatted";
 
-import { deserialize, createReviver } from "./serialization";
+import { deserialize } from "./serialization";
 
 describe("Server serialization", () => {
 

packages/server/src/serialization.ts

@@ -5,13 +5,21 @@ const debug = extend("serialization");
 
 type Reviver = (this: unknown, key: string, value: unknown) => unknown;
 
+function smellsLikeMochaRemote(line: string) {
+  return line.includes("/mocha-remote");
+}
+
+function filterStack(stack: string) {
+  return stack.split("\n").filter(line => !smellsLikeMochaRemote(line)).join("\n");
+}
+
 export function createReviver(): Reviver {
 
   function reviveObject(obj: Record<string, unknown>) {
     if (obj.type === "error") {
       if (typeof obj.message === "string" && typeof obj.stack === "string") {
         const err = new Error(obj.message);
-        err.stack = obj.stack;
+        err.stack = filterStack(obj.stack);
         return err;
       } else {
         throw new Error("Expected Error to have message and stack");