Better `child_process` error handling v2 - timeouts and actually fail process for error in scope (#62)

Follow-up to https://github.com/matrix-org/matrix-public-archive/pull/51

Better `child_process` error handling for a couple scenarios with the finger pointing at it 👉

Also make sure we handle all of these scenarios:

 1. Child process fork script throws an `uncaughtException` or `unhandledRejection`
    - These are captured and serialized back to the parent and stored in `childErrors` and exposed if we never get a successful rendered HTML response.
 2. Child process fails to startup 
    - Render process is rejected in the `child.on('error', ...` callback
 3. 👉 Child process times out and is aborted
    - Render process is rejected in the `child.on('error', ...` callback and any `childErrors` encountered are logged
 4. 👉 Child process fork script throws an error in scope of in `process.on('message', async (renderOptions) => {`
    - Child exits with code 1 and we reject the render process with the error
 5. Child process exits with code 1 (error)
    - Render process is rejected with any `childError` info
 6. Child process exits with code 0 (success) but never sends back any HTML
    - We have a `returnedData` data check and any child errors encountered are logged

server/hydrogen-render/1-render-hydrogen-to-string.js

@@ -23,10 +23,48 @@ if (!logOutputFromChildProcesses) {
   );
 }
 
+function assembleErrorAfterChildExitsWithErrors(exitCode, childErrors) {
+  assert(childErrors);
+
+  let extraErrorsMessage = '';
+  if (childErrors.length > 1) {
+    extraErrorsMessage = ` (somehow we saw ${
+      childErrors.length
+    } errors but we really always expect 1 error)\n${childErrors
+      .map((childError, index) => `${index}. ${childError.stack}`)
+      .join('\n')}`;
+  }
+
+  let childErrorToDisplay;
+  if (childErrors.length === 0) {
+    childErrorToDisplay = new Error('No child errors');
+    // Clear the stack trace part of the stack string out because this is just a
+    // note about the lack of errors, not an actual error and is just noisy with
+    // that extra fluff.
+    childErrorToDisplay.stack = childErrorToDisplay.message;
+  } else if (childErrors.length === 1) {
+    childErrorToDisplay = childErrors[0];
+  } else {
+    childErrorToDisplay = new Error('Multiple child errors listed above ^');
+    // Clear the stack trace part of the stack string out because this is just a
+    // note about the other errors, not an actual error and is just noisy with
+    // that extra fluff.
+    childErrorToDisplay.stack = childErrorToDisplay.message;
+  }
+
+  const childErrorSummary = new RethrownError(
+    `Child process exited with code ${exitCode}${extraErrorsMessage}`,
+    childErrorToDisplay
+  );
+
+  return childErrorSummary;
+}
+
 async function renderHydrogenToString(renderOptions) {
+  let abortTimeoutId;
   try {
-    let data = '';
     let childErrors = [];
+    let childExitCode = '(not set yet)';
 
     const controller = new AbortController();
     const { signal } = controller;
@@ -60,11 +98,12 @@ async function renderHydrogenToString(renderOptions) {
     child.send(renderOptions);
 
     // Stops the child process if it takes too long
-    setTimeout(() => {
+    abortTimeoutId = setTimeout(() => {
       controller.abort();
     }, RENDER_TIMEOUT);
 
-    await new Promise((resolve, reject) => {
+    const returnedData = await new Promise((resolve, reject) => {
+      let data = '';
       // Collect the data passed back by the child
       child.on('message', function (result) {
         if (result.error) {
@@ -85,53 +124,47 @@ async function renderHydrogenToString(renderOptions) {
       });
 
       child.on('close', (exitCode) => {
+        childExitCode = exitCode;
         // Exited successfully
         if (exitCode === 0) {
           resolve(data);
         } else {
-          let extraErrorsMessage = '';
+          const childErrorSummary = assembleErrorAfterChildExitsWithErrors(
-          if (childErrors.length > 1) {
+            childExitCode,
-            extraErrorsMessage = ` (somehow we saw ${
+            childErrors
-              childErrors.length
-            } errors but we really always expect 1 error)\n${childErrors
-              .map((childError, index) => `${index}. ${childError.stack}`)
-              .join('\n')}`;
-          }
-
-          let childErrorToDisplay = new Error('No child errors');
-          if (childErrors.length === 1) {
-            childErrorToDisplay = childErrors[0];
-          } else if (childErrors.length > 1) {
-            childErrorToDisplay = new Error('Multiple child errors listed above ^');
-          }
-
-          const error = new RethrownError(
-            `Child process failed with exit code ${exitCode}${extraErrorsMessage}`,
-            childErrorToDisplay
           );
-          reject(error);
+          reject(childErrorSummary);
         }
       });
 
       // When a problem occurs when spawning the process or gets aborted
       child.on('error', (err) => {
         if (err.name === 'AbortError') {
-          throw new RethrownError(
+          const childErrorSummary = assembleErrorAfterChildExitsWithErrors(
-            `Timed out while rendering Hydrogen to string so we aborted the child process after ${RENDER_TIMEOUT}ms`,
+            childExitCode,
-            err
+            childErrors
           );
+          reject(
+            new RethrownError(
+              `Timed out while rendering Hydrogen to string so we aborted the child process after ${RENDER_TIMEOUT}ms. Any child errors? (${childErrors.length})`,
+              childErrorSummary
+            )
+          );
+        } else {
+          reject(err);
         }
-
-        reject(err);
       });
     });
 
-    assert(
+    if (!returnedData) {
-      data,
+      const childErrorSummary = assembleErrorAfterChildExitsWithErrors(childExitCode, childErrors);
-      `No HTML sent from child process to render Hydrogen. Any child errors? (${childErrors.length})`
+      throw new RethrownError(
-    );
+        `No HTML sent from child process to render Hydrogen. Any child errors? (${childErrors.length})`,
+        childErrorSummary
+      );
+    }
 
-    return data;
+    return returnedData;
   } catch (err) {
     throw new RethrownError(
       `Failed to render Hydrogen to string. In order to reproduce, feed in these arguments into \`renderHydrogenToString(...)\`:\n    renderToString arguments: ${JSON.stringify(
@@ -139,6 +172,10 @@ async function renderHydrogenToString(renderOptions) {
       )}`,
       err
     );
+  } finally {
+    // We don't have to add a undefined/null check here because `clearTimeout`
+    // works with any value you give it and doesn't throw an error.
+    clearTimeout(abortTimeoutId);
   }
 }
 

server/hydrogen-render/2-render-hydrogen-to-string-fork-script.js

@@ -88,10 +88,10 @@ process.on('message', async (renderOptions) => {
     });
   } catch (err) {
     // We need to wait for the error to completely send to the parent
-    // process before we throw the error again and exit the process.
+    // process before we exit the process.
     await serializeError(err);
 
-    // Throw the error so the process fails and exits
+    // Fail the process and exit
-    throw err;
+    process.exit(1);
   }
 });

server/lib/rethrown-error.js

@@ -32,7 +32,7 @@ class RethrownError extends ExtendedError {
     super(message);
     if (!error) throw new Error('RethrownError requires a message and error');
     this.original = error;
-    this.newStack = this.stack;
+    this.originalStack = this.stack;
 
     // The number of lines that make up the message itself. We count this by the
     // number of `\n` and `+ 1` for the first line because it doesn't start with