Fix buildVM promise cleanup ordering (#2483)

packages/react-on-rails-pro-node-renderer/src/worker/vm.ts

@@ -192,7 +192,13 @@ export async function buildVM(filePath: string) {
     return Promise.resolve(true);
   }
 
-  // Create a new promise for this VM creation
+  // Create the VM creation promise. The IIFE runs synchronously until its first
+  // `await`, so we must store it in the map immediately after creation — before
+  // the microtask queue is drained — to prevent concurrent callers from starting
+  // a duplicate build. Cleanup uses `.finally()` on the stored promise rather
+  // than a try/finally inside the IIFE, because an IIFE's finally block can
+  // execute synchronously (before `vmCreationPromises.set`) when the code throws
+  // before the first `await`, which would leave a stale rejected promise in the map.
   const vmCreationPromise = (async () => {
     try {
       const { supportModules, stubTimers, additionalContext } = getConfig();
@@ -335,22 +341,44 @@ export async function buildVM(filePath: string) {
       log.error({ error }, 'Caught Error when creating context in buildVM');
       errorReporter.error(error as Error);
       throw error;
-    } finally {
-      // Always remove the promise from the map when done
-      vmCreationPromises.delete(filePath);
     }
   })();
 
-  // Store the promise
+  // Store the promise BEFORE any async work completes, so concurrent callers
+  // find it via the has() check above.
   vmCreationPromises.set(filePath, vmCreationPromise);
 
+  // Clean up the map entry after the promise settles (fulfills or rejects).
+  //
+  // Analogy: We write jobs on a whiteboard so nobody starts duplicates. If we
+  // told the helper "erase it when you're done" but the helper failed so fast
+  // they erased it *before we wrote it down*, the failed job would be stuck on
+  // the whiteboard forever — blocking all retries. Instead, we attach a sticky
+  // note to the job saying "erase me when I'm done." The note can't activate
+  // until after we've written the job down, so erasing always happens in the
+  // right order.
+  //
+  // Technically: an async IIFE runs synchronously until its first `await`. If
+  // it throws before any `await` (e.g. `vm.createContext()` or a
+  // `vm.runInContext()` call throwing synchronously before `readFileAsync`),
+  // a try/finally inside the IIFE would run *before* `vmCreationPromises.set()`
+  // (below) has executed — leaving a stale rejected promise in the map that
+  // permanently poisons retries. Chaining .finally() on the promise *after*
+  // set() guarantees cleanup runs as a microtask after the current synchronous
+  // execution, so set() has always run first.
+  void vmCreationPromise
+    .catch(() => {})
+    .finally(() => {
+      vmCreationPromises.delete(filePath);
+    });
+
   return vmCreationPromise;
 }
 
 /** @internal Used in tests */
 export function resetVM() {
-  // Clear all VM contexts
   vmContexts.clear();
+  vmCreationPromises.clear();
 }
 
 // Optional: Add a method to remove a specific VM if needed
@@ -359,4 +387,5 @@ export function resetVM() {
  */
 export function removeVM(bundlePath: string) {
   vmContexts.delete(bundlePath);
+  vmCreationPromises.delete(bundlePath);
 }

packages/react-on-rails-pro-node-renderer/tests/vm.test.ts

@@ -1,4 +1,5 @@
 import path from 'path';
+import vm from 'vm';
 import {
   uploadedBundlePath,
   createUploadedBundle,
@@ -542,6 +543,36 @@ describe('buildVM and runInVM', () => {
       expect(vmContext1).toBe(vmContext2);
     });
 
+    test('buildVM recovers after synchronous throw before first await', async () => {
+      // Clear any cached VM for serverBundlePath from prior tests
+      resetVM();
+
+      const config = getConfig();
+      config.supportModules = true;
+      config.stubTimers = false;
+
+      // Mock vm.createContext to throw synchronously. This simulates a
+      // failure BEFORE the first `await` in the buildVM IIFE — the exact
+      // scenario the .finally() cleanup ordering was designed to handle.
+      // With the old code (try/finally inside the IIFE), a synchronous
+      // throw would run cleanup before vmCreationPromises.set(), leaving
+      // a stale rejected promise that permanently blocks retries.
+      const createContextSpy = jest.spyOn(vm, 'createContext').mockImplementationOnce(() => {
+        throw new Error('sync context creation failure');
+      });
+
+      // First call fails synchronously during vm.createContext()
+      await expect(buildVM(serverBundlePath)).rejects.toThrow('sync context creation failure');
+
+      // Restore vm.createContext before retrying
+      createContextSpy.mockRestore();
+
+      // Retry the SAME path — if vmCreationPromises wasn't cleaned up,
+      // this would return the stale rejected promise and fail
+      await buildVM(serverBundlePath);
+      expect(hasVMContextForBundle(serverBundlePath)).toBeTruthy();
+    });
+
     test('running runInVM before buildVM', async () => {
       resetVM();
       void prepareVM(true);