Fix descriptor closed error caused by workers restart while request is in progress (#1970)

A `descriptor closed` error happens when the node renderer restarts while it's still handling an in-progress request (especially if it's a streaming request that may take more time to handle).
Implemented a fix that master doesn't kill the worker directly; instead, it sends a graceful shutdown message to the worker.
When a worker receives a shutdown message, it disconnects from the cluster, preventing it from receiving further requests.
Then, the worker waits until all active requests are handled and then shuts itself down. The master initiates a new worker.
When a worker receives the shutdown message, if it doesn't shut down during the `gracefulWorkerRestartTimeout`, the master forcibly kills it.

Author: AbanoubGhadban

react_on_rails_pro/CHANGELOG.md

@@ -40,6 +40,7 @@ You can find the **package** version numbers from this repo's tags and below in
 
 ### Fixed
 - Fixed an issue where, when React Server Components (RSC) support was disabled, the Node Renderer unnecessarily requested bundles on every render. Now, bundles are only requested when actually needed, improving performance and reducing redundant network traffic. [PR 545](https://github.com/shakacode/react_on_rails_pro/pull/545) by [AbanoubGhadban](https://github.com/AbanoubGhadban).
+- **Fix `descriptor closed` error**: The errors happens when the node renderer restarts while it's still handling an in-progress request (especially if it's a streaming request that may take more time to handle). Implemented a fix that makes worker shutdown gracefully after it finishes all active requests. When a worker receives the shutdown message, if it doesn't shut down during the `gracefulWorkerRestartTimeout`, the master forcibly kills it. [PR #1970][https://github.com/shakacode/react_on_rails/pull/1970] by [AbanoubGhadban](https://github.com/AbanoubGhadban).
 
 ### Changed
 - Upgraded HTTPX dependency from 1.3.4 to ~> 1.5 (currently 1.5.1). [PR 520](https://github.com/shakacode/react_on_rails_pro/pull/520) by [AbanoubGhadban](https://github.com/AbanoubGhadban).

react_on_rails_pro/docs/installation.md

@@ -129,6 +129,9 @@ const config = {
   // allWorkersRestartInterval: 15,
   // time in minutes between each worker restarting when restarting all workers
   // delayBetweenIndividualWorkerRestarts: 2,
+  // Also, you can set he parameter gracefulWorkerRestartTimeout to force the worker to restart
+  // If it's the time for the worker to restart, the worker waits until it serves all active requests before restarting
+  // If a worker stuck because of a memory leakage or an infinite loop, you can set a timeout that master waits for it before killing the worker
 }
 
 // Renderer detects a total number of CPUs on virtual hostings like Heroku

react_on_rails_pro/docs/node-renderer/js-configuration.md

@@ -22,6 +22,7 @@ Here are the options available for the JavaScript renderer configuration object,
 If no password is set, no authentication will be required.
 1. **allWorkersRestartInterval** (default: `env.RENDERER_ALL_WORKERS_RESTART_INTERVAL`) - Interval in minutes between scheduled restarts of all workers. By default restarts are not enabled. If restarts are enabled, `delayBetweenIndividualWorkerRestarts` should also be set.
 1. **delayBetweenIndividualWorkerRestarts** (default: `env.RENDERER_DELAY_BETWEEN_INDIVIDUAL_WORKER_RESTARTS`) - Interval in minutes between individual worker restarts (when cluster restart is triggered). By default restarts are not enabled. If restarts are enabled, `allWorkersRestartInterval` should also be set.
+1. **gracefulWorkerRestartTimeout**: (default: `env.GRACEFUL_WORKER_RESTART_TIMEOUT`) - Time in seconds that the master waits for a worker to gracefully restart (after serving all active requests) before killing it. Use this when you want to avoid situations where a worker gets stuck in an infinite loop and never restarts. This config is only usable if worker restart is enabled. The timeout starts when the worker should restart; if it elapses without a restart, the worker is killed.
 1. **maxDebugSnippetLength** (default: 1000) - If the rendering request is longer than this, it will be truncated in exception and logging messages.
 1. **supportModules** - (default: `env.RENDERER_SUPPORT_MODULES || null`) - If set to true, `supportModules` enables the server-bundle code to call a default set of NodeJS global objects and functions that get added to the VM context: 
 `{ Buffer, TextDecoder, TextEncoder, URLSearchParams, ReadableStream, process, setTimeout, setInterval, setImmediate, clearTimeout, clearInterval, clearImmediate, queueMicrotask }`.

react_on_rails_pro/docs/node-renderer/troubleshooting.md

@@ -1,3 +1,5 @@
 # Node Renderer Troubleshooting
 
 - If you enabled restarts (having `allWorkersRestartInterval` and `delayBetweenIndividualWorkerRestarts`), you should set it with a high number to avoid the app from crashing because all Node renderer workers are stopped/killed.
+
+- If your app contains streamed pages that take too much time to be streamed to the client, ensure to not set the `gracefulWorkerRestartTimeout` parameter or set to a high number, so the worker is not killed while it's still serving an active request.

react_on_rails_pro/packages/node-renderer/src/master.ts

@@ -19,7 +19,12 @@ export = function masterRun(runningConfig?: Partial<Config>) {
 
   // Store config in app state. From now it can be loaded by any module using getConfig():
   const config = buildConfig(runningConfig);
-  const { workersCount, allWorkersRestartInterval, delayBetweenIndividualWorkerRestarts } = config;
+  const {
+    workersCount,
+    allWorkersRestartInterval,
+    delayBetweenIndividualWorkerRestarts,
+    gracefulWorkerRestartTimeout,
+  } = config;
 
   logSanitizedConfig();
 
@@ -48,9 +53,15 @@ export = function masterRun(runningConfig?: Partial<Config>) {
       allWorkersRestartInterval,
       delayBetweenIndividualWorkerRestarts,
     );
-    setInterval(() => {
-      restartWorkers(delayBetweenIndividualWorkerRestarts);
-    }, allWorkersRestartInterval * MILLISECONDS_IN_MINUTE);
+
+    const allWorkersRestartIntervalMS = allWorkersRestartInterval * MILLISECONDS_IN_MINUTE;
+    const scheduleWorkersRestart = () => {
+      void restartWorkers(delayBetweenIndividualWorkerRestarts, gracefulWorkerRestartTimeout).finally(() => {
+        setTimeout(scheduleWorkersRestart, allWorkersRestartIntervalMS);
+      });
+    };
+
+    setTimeout(scheduleWorkersRestart, allWorkersRestartIntervalMS);
   } else if (allWorkersRestartInterval || delayBetweenIndividualWorkerRestarts) {
     log.error(
       "Misconfiguration, please provide both 'allWorkersRestartInterval' and " +

react_on_rails_pro/packages/node-renderer/src/master/restartWorkers.ts

@@ -5,6 +5,7 @@
 
 import cluster from 'cluster';
 import log from '../shared/log';
+import { SHUTDOWN_WORKER_MESSAGE } from '../shared/utils';
 
 const MILLISECONDS_IN_MINUTE = 60000;
 
@@ -14,26 +15,47 @@ declare module 'cluster' {
   }
 }
 
-export = function restartWorkers(delayBetweenIndividualWorkerRestarts: number) {
+export = async function restartWorkers(
+  delayBetweenIndividualWorkerRestarts: number,
+  gracefulWorkerRestartTimeout: number | undefined,
+) {
   log.info('Started scheduled restart of workers');
 
-  let delay = 0;
   if (!cluster.workers) {
     throw new Error('No workers to restart');
   }
-  Object.values(cluster.workers).forEach((worker) => {
-    const killWorker = () => {
-      if (!worker) return;
-      log.debug('Kill worker #%d', worker.id);
-      // eslint-disable-next-line no-param-reassign -- necessary change
-      worker.isScheduledRestart = true;
-      worker.destroy();
-    };
-    setTimeout(killWorker, delay);
-    delay += delayBetweenIndividualWorkerRestarts * MILLISECONDS_IN_MINUTE;
-  });
-
-  setTimeout(() => {
-    log.info('Finished scheduled restart of workers');
-  }, delay);
+  for (const worker of Object.values(cluster.workers).filter((w) => !!w)) {
+    log.debug('Kill worker #%d', worker.id);
+    worker.isScheduledRestart = true;
+
+    worker.send(SHUTDOWN_WORKER_MESSAGE);
+
+    // It's inteded to restart worker in sequence, it shouldn't happens in parallel
+    // eslint-disable-next-line no-await-in-loop
+    await new Promise<void>((resolve) => {
+      let timeout: NodeJS.Timeout;
+
+      const onExit = () => {
+        clearTimeout(timeout);
+        resolve();
+      };
+      worker.on('exit', onExit);
+
+      // Zero means no timeout
+      if (gracefulWorkerRestartTimeout) {
+        timeout = setTimeout(() => {
+          log.debug('Worker #%d timed out, forcing kill it', worker.id);
+          worker.destroy();
+          worker.off('exit', onExit);
+          resolve();
+        }, gracefulWorkerRestartTimeout);
+      }
+    });
+    // eslint-disable-next-line no-await-in-loop
+    await new Promise((resolve) => {
+      setTimeout(resolve, delayBetweenIndividualWorkerRestarts * MILLISECONDS_IN_MINUTE);
+    });
+  }
+
+  log.info('Finished scheduled restart of workers');
 };

react_on_rails_pro/packages/node-renderer/src/shared/configBuilder.ts

@@ -58,6 +58,9 @@ export interface Config {
   allWorkersRestartInterval: number | undefined;
   // Time in minutes between each worker restarting when restarting all workers
   delayBetweenIndividualWorkerRestarts: number | undefined;
+  // Time in seconds to wait for worker to restart before killing it
+  // Set it to 0 or undefined to never kill the worker
+  gracefulWorkerRestartTimeout: number | undefined;
   // If the rendering request is longer than this, it will be truncated in exception and logging messages
   maxDebugSnippetLength: number;
   // @deprecated See https://www.shakacode.com/react-on-rails-pro/docs/node-renderer/error-reporting-and-tracing.
@@ -165,6 +168,10 @@ const defaultConfig: Config = {
     ? parseInt(env.RENDERER_DELAY_BETWEEN_INDIVIDUAL_WORKER_RESTARTS, 10)
     : undefined,
 
+  gracefulWorkerRestartTimeout: env.GRACEFUL_WORKER_RESTART_TIMEOUT
+    ? parseInt(env.GRACEFUL_WORKER_RESTART_TIMEOUT, 10)
+    : undefined,
+
   maxDebugSnippetLength: MAX_DEBUG_SNIPPET_LENGTH,
 
   // default to true if empty, otherwise it is set to false
@@ -195,6 +202,8 @@ function envValuesUsed() {
     RENDERER_DELAY_BETWEEN_INDIVIDUAL_WORKER_RESTARTS:
       !userConfig.delayBetweenIndividualWorkerRestarts &&
       env.RENDERER_DELAY_BETWEEN_INDIVIDUAL_WORKER_RESTARTS,
+    GRACEFUL_WORKER_RESTART_TIMEOUT:
+      !userConfig.gracefulWorkerRestartTimeout && env.GRACEFUL_WORKER_RESTART_TIMEOUT,
     INCLUDE_TIMER_POLYFILLS: !('includeTimerPolyfills' in userConfig) && env.INCLUDE_TIMER_POLYFILLS,
     REPLAY_SERVER_ASYNC_OPERATION_LOGS:
       !userConfig.replayServerAsyncOperationLogs && env.REPLAY_SERVER_ASYNC_OPERATION_LOGS,
@@ -209,6 +218,7 @@ function sanitizedSettings(aConfig: Partial<Config> | undefined, defaultValue?:
         password: aConfig.password != null ? '<MASKED>' : defaultValue,
         allWorkersRestartInterval: aConfig.allWorkersRestartInterval || defaultValue,
         delayBetweenIndividualWorkerRestarts: aConfig.delayBetweenIndividualWorkerRestarts || defaultValue,
+        gracefulWorkerRestartTimeout: aConfig.gracefulWorkerRestartTimeout || defaultValue,
       }
     : {};
 }

react_on_rails_pro/packages/node-renderer/src/shared/utils.ts

@@ -11,6 +11,8 @@ import type { RenderResult } from '../worker/vm';
 
 export const TRUNCATION_FILLER = '\n... TRUNCATED ...\n';
 
+export const SHUTDOWN_WORKER_MESSAGE = 'NODE_RENDERER_SHUTDOWN_WORKER';
+
 export function workerIdLabel() {
   // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- worker is nullable in the primary process
   return cluster?.worker?.id || 'NO WORKER ID';

react_on_rails_pro/packages/node-renderer/src/worker.ts

@@ -17,6 +17,7 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from './worker/typ
 import checkProtocolVersion from './worker/checkProtocolVersionHandler';
 import authenticate from './worker/authHandler';
 import { handleRenderRequest, type ProvidedNewBundle } from './worker/handleRenderRequest';
+import handleGracefulShutdown from './worker/handleGracefulShutdown';
 import {
   errorResponseResult,
   formatExceptionMessage,
@@ -127,6 +128,8 @@ export default function run(config: Partial<Config>) {
     ...fastifyServerOptions,
   });
 
+  handleGracefulShutdown(app);
+
   // We shouldn't have unhandled errors here, but just in case
   app.addHook('onError', (req, res, err, done) => {
     // Not errorReporter.error so that integrations can decide how to log the errors.

react_on_rails_pro/packages/node-renderer/src/worker/handleGracefulShutdown.ts

@@ -0,0 +1,49 @@
+import cluster from 'cluster';
+import { FastifyInstance } from './types';
+import { SHUTDOWN_WORKER_MESSAGE } from '../shared/utils';
+import log from '../shared/log';
+
+const handleGracefulShutdown = (app: FastifyInstance) => {
+  const { worker } = cluster;
+  if (!worker) {
+    log.error('handleGracefulShutdown is called on master, expected to call it on worker only');
+    return;
+  }
+
+  let activeRequestsCount = 0;
+  let isShuttingDown = false;
+
+  process.on('message', (msg) => {
+    if (msg === SHUTDOWN_WORKER_MESSAGE) {
+      log.debug('Worker #%d received graceful shutdown message', worker.id);
+      isShuttingDown = true;
+      if (activeRequestsCount === 0) {
+        log.debug('Worker #%d has no active requests, killing the worker', worker.id);
+        worker.destroy();
+      } else {
+        log.debug(
+          'Worker #%d has "%d" active requests, disconnecting the worker',
+          worker.id,
+          activeRequestsCount,
+        );
+        worker.disconnect();
+      }
+    }
+  });
+
+  app.addHook('onRequest', (_req, _reply, done) => {
+    activeRequestsCount += 1;
+    done();
+  });
+
+  app.addHook('onResponse', (_req, _reply, done) => {
+    activeRequestsCount -= 1;
+    if (isShuttingDown && activeRequestsCount === 0) {
+      log.debug('Worker #%d served all active requests and going to be killed', worker.id);
+      worker.destroy();
+    }
+    done();
+  });
+};
+
+export default handleGracefulShutdown;