Harden against process lifetime issues (microsoft#75)

* Use  rather than  events

* Move kill-children-of into node for greater control

* Handle communication errors in the server harness

Author: amcasey

package-lock.json

@@ -13,8 +13,8 @@
     "dependencies": {
         "@octokit/rest": "^16.43.2",
         "@typescript/github-link": "^0.2.1",
-        "@typescript/server-harness": "^0.3.0",
-        "@typescript/server-replay": "^0.2.6",
+        "@typescript/server-harness": "^0.3.3",
+        "@typescript/server-replay": "^0.2.7",
         "glob": "^7.2.3",
         "json5": "^2.2.1",
         "random-seed": "^0.3.0",

package.json

@@ -48,7 +48,7 @@ export function spawnWithTimeoutAsync(cwd: string, command: string, args: readon
         let stdout = "";
         let stderr = "";
 
-        childProcess.on("close", (code, signal) => {
+        childProcess.once("close", (code, signal) => {
             if (!timedOut) {
                 clearTimeout(timeout);
                 resolve({ stdout, stderr, code, signal });
@@ -65,10 +65,59 @@ export function spawnWithTimeoutAsync(cwd: string, command: string, args: readon
 
         const timeout = setTimeout(async () => {
             timedOut = true;
-            // Note that killing childProcess resets the PPID of each of its children to 1, so this has to happen first
-            await execAsync(path.join(__dirname, "..", "..", "scripts"), `./kill-children-of ${childProcess.pid}`);
-            childProcess.kill("SIGKILL"); // This may fail if the process exited when its children were killed
+            await killTree(childProcess);
             resolve(undefined);
         }, timeoutMs | 0); // Truncate to int
     });
 }
+
+function killTree(childProcess: cp.ChildProcessWithoutNullStreams): Promise<void> {
+    return new Promise<void>((resolve, reject) => {
+        // Ideally, we would wait for all of the processes to close, but we only get events for
+        // this one, so we'll kill it last and hope for the best.
+        childProcess.once("close", () => {
+            resolve();
+        });
+
+        cp.exec("ps -e -o pid,ppid --no-headers", (err, stdout) => {
+            if (err) {
+                reject (err);
+                return;
+            }
+
+            const childMap: Record<number, number[]> = {};
+            const pidList = stdout.trim().split(/\s+/);
+            for (let i = 0; i + 1 < pidList.length; i += 2) {
+                const childPid = +pidList[i];
+                const parentPid = +pidList[i + 1];
+                childMap[parentPid] ||= [];
+                childMap[parentPid].push(childPid);
+            }
+
+            if (!childMap[childProcess.pid]) {
+                // Descendent processes may still be alive, but we have no way to identify them
+                resolve();
+                return;
+            }
+
+            const strictDescendentPids: number[] = [];
+            const stack: number[] = [ childProcess.pid ];
+            while (stack.length) {
+                const pid = stack.pop()!;
+                if (pid !== childProcess.pid) {
+                    strictDescendentPids.push(pid);
+                }
+                const children = childMap[pid];
+                if (children) {
+                    stack.push(...children);
+                }
+            }
+
+            console.log(`Killing process ${childProcess.pid} and its descendents: ${strictDescendentPids.join(", ")}`);
+
+            strictDescendentPids.forEach(pid => process.kill(pid, "SIGKILL"));
+            childProcess.kill("SIGKILL");
+            // Resolve when we detect that childProcess has closed (above)
+        });
+    });
+}

scripts/kill-children-of

@@ -7,7 +7,7 @@ import path = require("path");
 import glob = require("glob");
 import { performance } from "perf_hooks";
 import randomSeed = require("random-seed");
-import { EXIT_BAD_ARGS, EXIT_UNHANDLED_EXCEPTION, EXIT_SERVER_EXIT_FAILED, EXIT_SERVER_CRASH, EXIT_SERVER_ERROR, EXIT_LANGUAGE_SERVICE_DISABLED } from "./exerciseServerConstants";
+import { EXIT_BAD_ARGS, EXIT_UNHANDLED_EXCEPTION, EXIT_SERVER_EXIT_FAILED, EXIT_SERVER_CRASH, EXIT_SERVER_ERROR, EXIT_LANGUAGE_SERVICE_DISABLED, EXIT_SERVER_COMMUNICATION_ERROR } from "./exerciseServerConstants";
 
 const testDirPlaceholder = "@PROJECT_ROOT@";
 
@@ -105,10 +105,17 @@ async function exerciseServerWorker(testDir: string, tsserverPath: string, repla
         }
     });
 
+    server.on("communicationError", async (err: any) => {
+        console.error(`Error communicating with server:\n${err}`);
+        exitExpected = true;
+        await server.kill();
+        process.exit(EXIT_SERVER_COMMUNICATION_ERROR);
+    });
+
     let exitExpected = false;
-    server.on("exit", (code: number | null) => {
+    server.on("close", (code: number | null, signal: NodeJS.Signals | null) => {
         if (!exitExpected) {
-            console.log(`Server exited prematurely with code ${code ?? "unknown"}`);
+            console.log(`Server exited prematurely with code ${code ?? "unknown"} and signal ${signal ?? "unknown"}`);
             process.exit(EXIT_SERVER_CRASH);
         }
     });

src/utils/execUtils.ts

@@ -3,4 +3,5 @@ export const EXIT_UNHANDLED_EXCEPTION = 2;
 export const EXIT_SERVER_EXIT_FAILED = 3;
 export const EXIT_SERVER_CRASH = 4; // The server process actually exited
 export const EXIT_SERVER_ERROR = 5; // The server process sent an error response
-export const EXIT_LANGUAGE_SERVICE_DISABLED = 6;
+export const EXIT_LANGUAGE_SERVICE_DISABLED = 6;
+export const EXIT_SERVER_COMMUNICATION_ERROR = 7;