Address PR review: fix test to exercise actual race condition, improve comments

justin808 · claude · justin808 · commit ee5bfbbcd101 · 2026-02-26T16:00:56.000-10:00
- Rewrite buildVM recovery test to trigger synchronous throw (via
  vm.createContext mock) before the first await, which is the actual
  bug scenario. The old test used a nonexistent file path which fails
  asynchronously at readFileAsync — already handled by the old code.
- Fix test to retry the SAME path after failure, proving the
  vmCreationPromises map was properly cleaned up.
- Fix inaccurate comment example in vm.ts (readFileAsync → vm.createContext)
- Replace stale line number reference with semantic reference
- Add vmCreationPromises.delete() to removeVM to prevent stale entries

Co-Authored-By: Claude Opus 4.6 &lt;noreply@anthropic.com&gt;
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
@@ -359,12 +359,13 @@ export async function buildVM(filePath: string) {
   // right order.
   //
   // Technically: an async IIFE runs synchronously until its first `await`. If
-  // it throws before any `await` (e.g. missing file), a try/finally inside the
-  // IIFE would run *before* set() on line 349 stores the promise — 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.
+  // 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(() => {
@@ -386,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
@@ -1,4 +1,5 @@
 import path from 'path';
+import vm from 'vm';
 import {
   uploadedBundlePath,
   createUploadedBundle,
@@ -542,18 +543,32 @@ describe('buildVM and runInVM', () => {
       expect(vmContext1).toBe(vmContext2);
     });
 
-    test('buildVM recovers after failure for nonexistent bundle', async () => {
+    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;
 
-      const nonExistentPath = path.resolve(__dirname, './tmp/nonexistent-bundle.js');
+      // 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');
 
-      // First call should fail because the bundle file does not exist
-      await expect(buildVM(nonExistentPath)).rejects.toThrow();
+      // Restore vm.createContext before retrying
+      createContextSpy.mockRestore();
 
-      // After failure, the vmCreationPromises map should be cleaned up,
-      // allowing a retry with the correct path to succeed
+      // 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();
     });
