diff --git a/packages/react-on-rails-pro-node-renderer/src/worker/vm.ts b/packages/react-on-rails-pro-node-renderer/src/worker/vm.ts index 3567f9ea7c..a2508ec081 100644 --- a/packages/react-on-rails-pro-node-renderer/src/worker/vm.ts +++ b/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); } diff --git a/packages/react-on-rails-pro-node-renderer/tests/vm.test.ts b/packages/react-on-rails-pro-node-renderer/tests/vm.test.ts index 03e615ab81..6035191d64 100644 --- a/packages/react-on-rails-pro-node-renderer/tests/vm.test.ts +++ b/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);