Enhance handleRenderRequest to support multiple new bundles and dependency timestamps

- Updated the `handleRenderRequest` function to accept an array of new bundles and dependency timestamps, improving flexibility in handling rendering requests.
- Refactored related functions to manage multiple bundles effectively, including the creation of directories for new bundles.
- Added tests to validate the handling of multiple bundles and scenarios where dependency timestamps are provided but not uploaded.
- Introduced a new fixture for secondary bundles to support testing of the updated functionality.

Author: AbanoubGhadban

packages/node-renderer/src/worker.ts

@@ -15,7 +15,7 @@ import fileExistsAsync from './shared/fileExistsAsync';
 import type { FastifyInstance, FastifyReply, FastifyRequest } from './worker/types';
 import checkProtocolVersion from './worker/checkProtocolVersionHandler';
 import authenticate from './worker/authHandler';
-import handleRenderRequest from './worker/handleRenderRequest';
+import { handleRenderRequest, type ProvidedNewBundle } from './worker/handleRenderRequest';
 import {
   errorResponseResult,
   formatExceptionMessage,
@@ -174,7 +174,10 @@ export default function run(config: Partial<Config>) {
   // the digest is part of the request URL. Yes, it's not used here, but the
   // server logs might show it to distinguish different requests.
   app.post<{
-    Body: { renderingRequest: string } & Record<string, Asset>;
+    Body: {
+      renderingRequest: string;
+      dependencyBundleTimestamps?: string[];
+    };
     // Can't infer from the route like Express can
     Params: { bundleTimestamp: string; renderRequestDigest: string };
   }>('/bundles/:bundleTimestamp/render/:renderRequestDigest', async (req, res) => {
@@ -193,13 +196,15 @@ export default function run(config: Partial<Config>) {
     //   await delay(100000);
     // }
 
-    const { renderingRequest } = req.body;
+    const { renderingRequest, dependencyBundleTimestamps } = req.body;
     const { bundleTimestamp } = req.params;
-    let providedNewBundle: Asset | undefined;
+    const providedNewBundles: ProvidedNewBundle[] = [];
     const assetsToCopy: Asset[] = [];
     Object.entries(req.body).forEach(([key, value]) => {
-      if (key === 'bundle') {
-        providedNewBundle = value as Asset;
+      if (key === 'bundle' && isAsset(value)) {
+        providedNewBundles.push({ timestamp: bundleTimestamp, bundle: value });
+      } else if (key.startsWith('bundle_') && isAsset(value)) {
+        providedNewBundles.push({ timestamp: key.replace('bundle_', ''), bundle: value });
       } else if (isAsset(value)) {
         assetsToCopy.push(value);
       }
@@ -211,7 +216,8 @@ export default function run(config: Partial<Config>) {
           const result = await handleRenderRequest({
             renderingRequest,
             bundleTimestamp,
-            providedNewBundle,
+            dependencyBundleTimestamps,
+            providedNewBundles,
             assetsToCopy,
           });
           await setResponse(result, res);

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

@@ -7,6 +7,7 @@
 
 import cluster from 'cluster';
 import path from 'path';
+import { mkdir } from 'fs/promises';
 import { lock, unlock } from '../shared/locks';
 import fileExistsAsync from '../shared/fileExistsAsync';
 import log from '../shared/log';
@@ -26,6 +27,11 @@ import { getConfig } from '../shared/configBuilder';
 import * as errorReporter from '../shared/errorReporter';
 import { buildVM, hasVMContextForBundle, runInVM } from './vm';
 
+export type ProvidedNewBundle = {
+  timestamp: string | number;
+  bundle: Asset;
+};
+
 async function prepareResult(
   renderingRequest: string,
   bundleFilePathPerTimestamp: string,
@@ -70,7 +76,8 @@ async function prepareResult(
 
 function getRequestBundleFilePath(bundleTimestamp: string | number) {
   const { bundlePath } = getConfig();
-  return path.join(bundlePath, `${bundleTimestamp}.js`);
+  const bundleDirectory = path.join(bundlePath, `${bundleTimestamp}`);
+  return path.join(bundleDirectory, `${bundleTimestamp}.js`);
 }
 
 /**
@@ -80,11 +87,13 @@ function getRequestBundleFilePath(bundleTimestamp: string | number) {
  * @param assetsToCopy might be null
  */
 async function handleNewBundleProvided(
-  bundleFilePathPerTimestamp: string,
-  providedNewBundle: Asset,
   renderingRequest: string,
+  providedNewBundle: ProvidedNewBundle,
   assetsToCopy: Asset[] | null | undefined,
-): Promise<ResponseResult> {
+): Promise<ResponseResult | undefined> {
+  const bundleFilePathPerTimestamp = getRequestBundleFilePath(providedNewBundle.timestamp);
+  const bundleDirectory = path.dirname(bundleFilePathPerTimestamp);
+  await mkdir(bundleDirectory, { recursive: true });
   log.info('Worker received new bundle: %s', bundleFilePathPerTimestamp);
 
   let lockAcquired = false;
@@ -104,22 +113,24 @@ async function handleNewBundleProvided(
     }
 
     try {
-      log.info(`Moving uploaded file ${providedNewBundle.savedFilePath} to ${bundleFilePathPerTimestamp}`);
-      await moveUploadedAsset(providedNewBundle, bundleFilePathPerTimestamp);
+      log.info(
+        `Moving uploaded file ${providedNewBundle.bundle.savedFilePath} to ${bundleFilePathPerTimestamp}`,
+      );
+      await moveUploadedAsset(providedNewBundle.bundle, bundleFilePathPerTimestamp);
       if (assetsToCopy) {
         await moveUploadedAssets(assetsToCopy);
       }
 
       log.info(
-        `Completed moving uploaded file ${providedNewBundle.savedFilePath} to ${bundleFilePathPerTimestamp}`,
+        `Completed moving uploaded file ${providedNewBundle.bundle.savedFilePath} to ${bundleFilePathPerTimestamp}`,
       );
     } catch (error) {
       const fileExists = await fileExistsAsync(bundleFilePathPerTimestamp);
       if (!fileExists) {
         const msg = formatExceptionMessage(
           renderingRequest,
           error,
-          `Unexpected error when moving the bundle from ${providedNewBundle.savedFilePath} \
+          `Unexpected error when moving the bundle from ${providedNewBundle.bundle.savedFilePath} \
 to ${bundleFilePathPerTimestamp})`,
         );
         log.error(msg);
@@ -131,20 +142,7 @@ to ${bundleFilePathPerTimestamp})`,
       );
     }
 
-    try {
-      // Either this process or another process placed the file. Because the lock is acquired, the
-      // file must be fully written
-      log.info('buildVM, bundleFilePathPerTimestamp', bundleFilePathPerTimestamp);
-      await buildVM(bundleFilePathPerTimestamp);
-      return prepareResult(renderingRequest, bundleFilePathPerTimestamp);
-    } catch (error) {
-      const msg = formatExceptionMessage(
-        renderingRequest,
-        error,
-        `Unexpected error when building the VM ${bundleFilePathPerTimestamp}`,
-      );
-      return Promise.resolve(errorResponseResult(msg));
-    }
+    return undefined;
   } finally {
     if (lockAcquired) {
       log.info('About to unlock %s from worker %i', lockfileName, workerIdLabel());
@@ -164,20 +162,44 @@ to ${bundleFilePathPerTimestamp})`,
   }
 }
 
+async function handleNewBundlesProvided(
+  renderingRequest: string,
+  providedNewBundles: ProvidedNewBundle[],
+  assetsToCopy: Asset[] | null | undefined,
+): Promise<ResponseResult | undefined> {
+  log.info('Worker received new bundles: %s', providedNewBundles);
+
+  // const handlingPromises = providedNewBundles.map((providedNewBundle) => handleNewBundleProvided(renderingRequest, providedNewBundle, assetsToCopy));
+  // const results = await Promise.all(handlingPromises);
+  // const errorResults = results.filter((result) => result?.status !== 200);
+  // if (errorResults.length > 0) {
+  //   return errorResults[0];
+  // }
+  // return undefined;
+  const handlingPromises = providedNewBundles.map((providedNewBundle) =>
+    handleNewBundleProvided(renderingRequest, providedNewBundle, assetsToCopy),
+  );
+  const results = await Promise.all(handlingPromises);
+  const errorResult = results.find((result) => result !== undefined);
+  return errorResult;
+}
+
 /**
  * Creates the result for the Fastify server to use.
  * @returns Promise where the result contains { status, data, headers } to
  * send back to the browser.
  */
-export = async function handleRenderRequest({
+export async function handleRenderRequest({
   renderingRequest,
   bundleTimestamp,
-  providedNewBundle,
+  dependencyBundleTimestamps,
+  providedNewBundles,
   assetsToCopy,
 }: {
   renderingRequest: string;
   bundleTimestamp: string | number;
-  providedNewBundle?: Asset | null;
+  dependencyBundleTimestamps?: string[] | number[];
+  providedNewBundles?: ProvidedNewBundle[] | null;
   assetsToCopy?: Asset[] | null;
 }): Promise<ResponseResult> {
   try {
@@ -189,19 +211,25 @@ export = async function handleRenderRequest({
     }
 
     // If gem has posted updated bundle:
-    if (providedNewBundle) {
-      return handleNewBundleProvided(
-        bundleFilePathPerTimestamp,
-        providedNewBundle,
-        renderingRequest,
-        assetsToCopy,
-      );
+    if (providedNewBundles) {
+      const result = await handleNewBundlesProvided(renderingRequest, providedNewBundles, assetsToCopy);
+      if (result) {
+        return result;
+      }
     }
 
     // Check if the bundle exists:
-    const fileExists = await fileExistsAsync(bundleFilePathPerTimestamp);
-    if (!fileExists) {
-      log.info(`No saved bundle ${bundleFilePathPerTimestamp}. Requesting a new bundle.`);
+    const notExistingBundles = await Promise.all(
+      [...(dependencyBundleTimestamps ?? []), bundleTimestamp].map(async (timestamp) => {
+        const bundleFilePath = getRequestBundleFilePath(timestamp);
+        const fileExists = await fileExistsAsync(bundleFilePath);
+        return !fileExists;
+      }),
+    );
+    if (notExistingBundles.some((bundle) => bundle)) {
+      log.info(
+        `No saved bundle${notExistingBundles.length > 1 ? 's' : ''} ${notExistingBundles.join(', ')}. Requesting a new bundle${notExistingBundles.length > 1 ? 's' : ''}.`,
+      );
       return Promise.resolve({
         headers: { 'Cache-Control': 'no-cache, no-store, max-age=0, must-revalidate' },
         status: 410,
@@ -224,4 +252,4 @@ export = async function handleRenderRequest({
     errorReporter.message(msg);
     return Promise.reject(error as Error);
   }
-};
+}

packages/node-renderer/tests/fixtures/secondary-bundle.js

@@ -0,0 +1,3 @@
+global.ReactOnRails = {
+  dummy: { html: 'Dummy Object from secondary bundle' },
+};