Fix buildVM promise cleanup ordering (#2483)#2484
Conversation
Move vmCreationPromises cleanup from a try/finally inside the async IIFE to a .finally() on the promise chain after vmCreationPromises.set(). The IIFE's finally block runs synchronously before set() when code throws before the first await, leaving a rejected promise permanently stuck in the map and preventing retries for that bundle path. Add test verifying buildVM recovers after failure for a nonexistent bundle. Fixes #2483 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughStores VM-creation promises immediately to block concurrent builds, moves cleanup to a Changes
Sequence Diagram(s)sequenceDiagram
participant ClientA as Client A
participant ClientB as Client B
participant Promises as vmCreationPromises
participant FS as FileSystem
participant VM as VM Context
ClientA->>Promises: check for existing promise
alt no promise
ClientA->>Promises: store vmCreationPromise (immediately)
ClientA->>FS: read bundle (async)
end
ClientB->>Promises: sees vmCreationPromise -> await it
FS-->>ClientA: bundle contents
ClientA->>VM: vm.runInContext(bundle) -> set context & vmBundleFilePath
ClientA->>Promises: resolve vmCreationPromise
Promises-->>ClientB: promise settles, awaiting clients proceed
Promises->>Promises: storedPromise.catch().finally() -> delete map entry
Note right of VM: `removeVM(bundlePath)` can be called to delete VM/context and map entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryFixes a subtle race condition in Problem: When Solution: Moved cleanup from Key changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant buildVM
participant Map as vmCreationPromises Map
participant IIFE as Async IIFE
Note over Caller,IIFE: Scenario: Synchronous failure (e.g., file doesn't exist)
Caller->>buildVM: buildVM(nonexistent.js)
buildVM->>buildVM: Check if promise exists in map
buildVM->>IIFE: Create async IIFE
Note over IIFE: IIFE runs synchronously until first await
IIFE->>IIFE: readFileAsync throws (sync)<br/>before first await
Note over buildVM,IIFE: OLD CODE (Bug): finally runs immediately
IIFE--xIIFE: finally: vmCreationPromises.delete()
Note over Map: ❌ Map entry doesn't exist yet!
buildVM->>Map: vmCreationPromises.set(path, promise)
Note over Map: ❌ Rejected promise stuck in map
buildVM-->>Caller: Return rejected promise
Note over Caller,Map: NEW CODE (Fix): cleanup after set()
Caller->>buildVM: buildVM(nonexistent.js)
buildVM->>buildVM: Check if promise exists in map
buildVM->>IIFE: Create async IIFE
IIFE->>IIFE: readFileAsync throws
buildVM->>Map: vmCreationPromises.set(path, promise)
buildVM->>buildVM: Chain .catch().finally() on promise
Note over buildVM: .catch() swallows rejection
IIFE-->>buildVM: Promise rejects
buildVM->>Map: .finally(): vmCreationPromises.delete(path)
Note over Map: ✅ Map cleaned up correctly
buildVM-->>Caller: Return rejected promise
Last reviewed commit: caf0e0b |
size-limit report 📦
|
Code ReviewSummaryThe fix correctly addresses the race condition: when the async IIFE throws synchronously before its first Issues1. Test does not exercise the actual bug (most important)The new test triggers the bundle-not-found error via The bug only bites when code throws synchronously before the first A minimal fix to the test would be to mock one of the early synchronous calls (e.g. 2. Test's recovery assertion uses a different pathAfter failing for Minor observations
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const nonExistentPath = path.resolve(__dirname, './tmp/nonexistent-bundle.js'); | ||
|
|
||
| // First call should fail because the bundle file does not exist | ||
| await expect(buildVM(nonExistentPath)).rejects.toThrow(); |
There was a problem hiding this comment.
The failure here is triggered by readFileAsync(filePath, 'utf8') on line 297 of vm.ts, which is after the first await in the IIFE. Async failures were handled correctly by the old code too — vmCreationPromises.set() runs synchronously before any await completes, so the old finally inside the IIFE would fire after set() for async failures. This means the original buggy code would also pass this test.
To actually exercise the race condition the fix targets, the test needs to trigger a synchronous throw before the first await — e.g. by temporarily making getConfig() throw:
test('buildVM cleans up map after synchronous throw before first await', async () => {
const originalGetConfig = getConfig;
jest.spyOn(/* module */, 'getConfig').mockImplementationOnce(() => {
throw new Error('sync config failure');
});
await expect(buildVM(serverBundlePath)).rejects.toThrow('sync config failure');
// With the old code, serverBundlePath would be stuck in vmCreationPromises
// as a stale rejected promise. With the fix, the entry is deleted and this retry succeeds.
await expect(buildVM(serverBundlePath)).resolves.toBe(true);
});Without a test like this, there is no regression guard for the specific ordering bug.
|
|
||
| // After failure, the vmCreationPromises map should be cleaned up, | ||
| // allowing a retry with the correct path to succeed | ||
| await buildVM(serverBundlePath); |
There was a problem hiding this comment.
This recovery call uses a different path (serverBundlePath) from the failed call (nonExistentPath). The stale rejected promise, if any, would be stored under nonExistentPath, not serverBundlePath, so this assertion would succeed regardless of whether cleanup for nonExistentPath occurred. The comment above ("allowing a retry") is misleading — this isn't a retry for the same path.
Consider either retrying the same path (after creating the file or restoring a mock) or at least adding a direct assertion that nonExistentPath is no longer in vmCreationPromises after the failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-on-rails-pro-node-renderer/src/worker/vm.ts (1)
363-375:resetVMdoes not clearvmCreationPromises— potential stale-state risk in tests.
resetVM()clearsvmContextsbut leavesvmCreationPromisesintact. If a test callsresetVM()while abuildVMis still in-flight (or a rejected promise hasn't been cleaned up yet by the microtask), a subsequentbuildVMfor the same path would return the stale/rejected promise from the map. This doesn't appear to be triggered by current tests, but it's a latent footgun.Consider clearing
vmCreationPromisesinresetVMfor safety:Proposed fix
export function resetVM() { // Clear all VM contexts vmContexts.clear(); + vmCreationPromises.clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` around lines 363 - 375, The resetVM function currently clears vmContexts but leaves vmCreationPromises intact, risking stale or rejected promise reuse; update resetVM (the exported function named resetVM) to also clear the vmCreationPromises Map/collection so any in-flight or failed buildVM promises are removed, ensuring subsequent calls to buildVM won't return stale promises; locate the vmCreationPromises symbol in this module and call its clear/delete logic alongside vmContexts.clear() (and consider also removing the removeVM behavior if it should mirror reset semantics).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-on-rails-pro-node-renderer/tests/vm.test.ts`:
- Around line 545-559: The test uses different paths and may hit cached state so
it doesn't verify cleanup of the vmCreationPromises map; change the test to (1)
call resetVM() at the start to clear any cached vmContexts, (2) use the same
path (nonExistentPath) for both the failing attempt and the retry (create the
file between attempts or write serverBundlePath contents to nonExistentPath),
and (3) after the rejected buildVM(nonExistentPath) assert that
vmCreationPromises (internal map) has had its entry removed or that the map is
empty before retrying, then call buildVM(nonExistentPath) and assert
hasVMContextForBundle(nonExistentPath) is true so the test actually exercises
cleanup logic in buildVM.
---
Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/src/worker/vm.ts`:
- Around line 363-375: The resetVM function currently clears vmContexts but
leaves vmCreationPromises intact, risking stale or rejected promise reuse;
update resetVM (the exported function named resetVM) to also clear the
vmCreationPromises Map/collection so any in-flight or failed buildVM promises
are removed, ensuring subsequent calls to buildVM won't return stale promises;
locate the vmCreationPromises symbol in this module and call its clear/delete
logic alongside vmContexts.clear() (and consider also removing the removeVM
behavior if it should mirror reset semantics).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-on-rails-pro-node-renderer/src/worker/vm.tspackages/react-on-rails-pro-node-renderer/tests/vm.test.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
| // allowing a retry with the correct path to succeed | ||
| await buildVM(serverBundlePath); | ||
| expect(hasVMContextForBundle(serverBundlePath)).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Test doesn't validate the cleanup ordering fix
Medium Severity
This test doesn't actually exercise the fix it claims to verify. The failure path uses nonExistentPath while the retry uses serverBundlePath — a different key in vmCreationPromises — so the retry's success is entirely independent of whether the failed path's entry was cleaned up. Additionally, the nonexistent-file error occurs at await readFileAsync (the first await), making it an async failure, whereas the bug being fixed only affects synchronous throws before the first await. This test would pass identically on both old and new code, leaving the actual race condition fix with no regression coverage.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review: Fix buildVM promise cleanup orderingThe core fix is correct and well-motivated. Moving cleanup from Issues1. The test does not exercise the actual bug pathThe new test triggers the error via a non-existent file, which fails at: const bundleContents = await readFileAsync(filePath, 'utf8'); // line 297This is the first The race condition only bites when code throws synchronously before the first As written, the test passes with both the old code and the new code, so it doesn't actually verify the fix. 2. Misleading example in the inline commentThe comment at the fix site says:
…as an example of something that "throws before reaching any A more accurate example would be: 3. Hardcoded line number in commentThe comment reads:
Line numbers shift whenever the surrounding code changes, making this stale almost immediately. Referring to the call by name/description instead of by line number would be more durable. Minor pre-existing note (not introduced by this PR)
|
| // Why .finally() here instead of try/finally inside the async IIFE: | ||
| // | ||
| // An async IIFE executes synchronously until its first `await`. If the code | ||
| // throws before reaching any `await` (e.g. readFileAsync rejects for a |
There was a problem hiding this comment.
The example "readFileAsync rejects for a missing file" is inaccurate here. readFileAsync is called with await on line 297 and is the first await in the IIFE — so its rejection is asynchronous. By the time readFileAsync rejects, vmCreationPromises.set() has already run and there is no stale entry.
The synchronous-throw scenario (the actual bug path) is triggered by code that runs before await readFileAsync: e.g., vm.createContext(), extendContext(), or any of the vm.runInContext() calls throwing.
Suggest replacing the example:
| // throws before reaching any `await` (e.g. readFileAsync rejects for a | |
| // throws before reaching any `await` (e.g. `vm.createContext()` or | |
| // a `vm.runInContext()` call throwing synchronously before `readFileAsync`), |
| // An async IIFE executes synchronously until its first `await`. If the code | ||
| // throws before reaching any `await` (e.g. readFileAsync rejects for a | ||
| // missing file), the IIFE's internal finally block runs *synchronously* — | ||
| // before `vmCreationPromises.set()` on line 349 has executed. That means |
There was a problem hiding this comment.
Line number references become stale as the code evolves. Consider referring to the call semantically instead:
| // before `vmCreationPromises.set()` on line 349 has executed. That means | |
| // before `vmCreationPromises.set()` (below) has executed. That means |
Makes the subtle timing bug more approachable before diving into the technical explanation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review: Fix buildVM promise cleanup orderingThe fix correctly identifies and addresses a real race condition in JavaScript's async function behavior: when an Core fix is sound. A few issues worth addressing before merging: 1. Test doesn't exercise the actual bug pathThe new test triggers a failure via a nonexistent file, which fails at The bug only manifests for synchronous errors thrown before the first 2.
|
| const nonExistentPath = path.resolve(__dirname, './tmp/nonexistent-bundle.js'); | ||
|
|
||
| // First call should fail because the bundle file does not exist | ||
| await expect(buildVM(nonExistentPath)).rejects.toThrow(); |
There was a problem hiding this comment.
This test doesn't actually exercise the bug being fixed. The failure for nonExistentPath occurs at await readFileAsync(filePath, 'utf8'), which is an async error (after the first await in the IIFE). For async errors, the original try/finally inside the IIFE already runs after vmCreationPromises.set(), so the old code handles this case correctly.
The race condition only triggers for synchronous errors thrown before the first await. A test that properly validates the fix would need to trigger a sync failure, e.g.:
// mock getConfig() to throw synchronously
jest.spyOn(configModule, 'getConfig').mockImplementationOnce(() => { throw new Error('sync config error'); });
await expect(buildVM(somePath)).rejects.toThrow();
// restore normal config, then verify the same path can be retried
jest.restoreAllMocks();
await buildVM(somePath); // would hang forever with the old code (stale rejected promise in map)
|
|
||
| return vmCreationPromise; | ||
| } | ||
|
|
There was a problem hiding this comment.
removeVM only clears from vmContexts but not vmCreationPromises. If called while a build is in-flight, the promise entry stays in the map and any concurrent caller awaiting it will still receive the result of the removed build. Should also clear the promise map:
export function removeVM(bundlePath: string) {
vmContexts.delete(bundlePath);
vmCreationPromises.delete(bundlePath);
}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails-pro-node-renderer/src/worker/vm.ts (1)
377-381:⚠️ Potential issue | 🟡 Minor
resetVMshould also clearvmCreationPromisesfor a clean test slate.When
resetVM()is called synchronously during test setup, any in-flight promise's.finally()cleanup microtask may not have executed yet. This leaves stale entries invmCreationPromises. When the next test callsbuildVM()with the same bundle path, thehas()check at line 183 will find the stale promise and return it instead of creating a fresh build, potentially causing tests to fail with cached results.Proposed fix
export function resetVM() { // Clear all VM contexts vmContexts.clear(); + vmCreationPromises.clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` around lines 377 - 381, The resetVM function currently only clears vmContexts but must also clear vmCreationPromises to avoid returning stale in-flight promises from buildVM; update the resetVM implementation to call vmCreationPromises.clear() (in addition to vmContexts.clear()) so tests get a clean slate and subsequent buildVM has() checks won't find stale entries.
🧹 Nitpick comments (1)
packages/react-on-rails-pro-node-renderer/src/worker/vm.ts (1)
383-389:removeVMis currently unused — consider deferring until needed.This is dead code with a TODO acknowledging it's not yet used. If there's a planned use case, that's fine to keep. Otherwise, YAGNI suggests deferring to avoid maintaining untested surface area.
Also, if a build is somehow in progress for
bundlePath, this only removes the finished context but not the in-progress promise fromvmCreationPromises, so a concurrentbuildVMcall could still see the old promise. This may be fine if the function is only intended for post-build cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` around lines 383 - 389, removeVM is dead code and, if retained, is unsafe because it only deletes vmContexts and leaves any in-progress promise in vmCreationPromises; either remove the removeVM function entirely (defer until needed) or update it to also clear the corresponding entry in vmCreationPromises and document/handle in-progress builds (e.g., cancel or guard against race with buildVM). Locate the removeVM function and either delete it from the module or modify it so it deletes both vmContexts.delete(bundlePath) and vmCreationPromises.delete(bundlePath), and add a brief comment in buildVM about races with in-progress promises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/react-on-rails-pro-node-renderer/src/worker/vm.ts`:
- Around line 377-381: The resetVM function currently only clears vmContexts but
must also clear vmCreationPromises to avoid returning stale in-flight promises
from buildVM; update the resetVM implementation to call
vmCreationPromises.clear() (in addition to vmContexts.clear()) so tests get a
clean slate and subsequent buildVM has() checks won't find stale entries.
---
Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/src/worker/vm.ts`:
- Around line 383-389: removeVM is dead code and, if retained, is unsafe because
it only deletes vmContexts and leaves any in-progress promise in
vmCreationPromises; either remove the removeVM function entirely (defer until
needed) or update it to also clear the corresponding entry in vmCreationPromises
and document/handle in-progress builds (e.g., cancel or guard against race with
buildVM). Locate the removeVM function and either delete it from the module or
modify it so it deletes both vmContexts.delete(bundlePath) and
vmCreationPromises.delete(bundlePath), and add a brief comment in buildVM about
races with in-progress promises.
justin808
added
the
release:16.4.0-must-have
AbanoubGhadban
left a comment
AbanoubGhadban
left a comment
There was a problem hiding this comment.
Thanks for working on this! The .finally() cleanup ordering fix is a correct improvement — chaining it after .set() guarantees the map entry is always cleaned up regardless of when the promise settles.
However, I want to flag that the connection to the "ReactOnRails value exists" error described in #2483 doesn't hold. That error was caused by the old v3.3.1 architecture where context was a module-level variable that got overwritten by concurrent buildVM calls at the await boundary. This was already fixed in react_on_rails_pro#527, which introduced the vmCreationPromises Map and made context a local variable inside the IIFE — so concurrent calls can no longer share or overwrite each other's context.
The bug this PR actually fixes (stale rejected promise when code throws synchronously before the first await) is real but separate — it would permanently block retries for a bundle path, not cause a double-initialization of ReactOnRails. I'd suggest updating the PR description to reflect what this actually fixes.
| expect(vmContext1).toBe(vmContext2); | ||
| }); | ||
|
|
||
| test('buildVM recovers after failure for nonexistent bundle', async () => { |
There was a problem hiding this comment.
This test does not exercise the specific bug being fixed. Two issues:
- The failure happens at
readFileAsync(the firstawait), where the originaltry/finallycleanup already works correctly — thefinallyruns as a microtask afterset()has been called. - The retry uses
serverBundlePath(a different path thannonExistentPath), so even if a stale promise were left fornonExistentPath, it would not affect the retry.
To properly validate the fix, the test would need to trigger a synchronous throw before the first await (e.g., by mocking vm.createContext to throw), then retry with the same path and assert it succeeds.
…e comments - 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 <noreply@anthropic.com>
Review: Fix buildVM promise cleanup ordering (#2483)The root cause analysis is correct and well-documented. The fix is sound: moving cleanup from A few issues worth addressing: Bug:
|
| export function removeVM(bundlePath: string) { | ||
| vmContexts.delete(bundlePath); | ||
| vmCreationPromises.delete(bundlePath); | ||
| } |
There was a problem hiding this comment.
Good fix adding this to removeVM. But resetVM() (just above) still only calls vmContexts.clear() and does not clear vmCreationPromises. The two functions are now inconsistent.
If resetVM() is called while a build is in-flight, the pending promise survives the reset — so the next buildVM() call on the same path returns the stale promise via the has() check, bypassing the reset entirely.
resetVM should also call vmCreationPromises.clear().
| resetVM(); | ||
|
|
||
| const config = getConfig(); | ||
| config.supportModules = true; |
There was a problem hiding this comment.
This test mutates the shared config object (config.supportModules = true, config.stubTimers = false) without restoring it afterwards. If resetForTest in beforeEach doesn't reset these properties, tests that run after this one and rely on different config values could be affected.
Other tests in this file follow the same pattern (e.g. the Buffer and process in context describe block), so this may already be acceptable given how resetForTest works — but it's worth verifying resetForTest actually restores config state, or adding explicit teardown here.
| .catch(() => {}) | ||
| .finally(() => { | ||
| vmCreationPromises.delete(filePath); | ||
| }); |
There was a problem hiding this comment.
This 18-line comment block largely repeats the 7-line explanation already written just above the IIFE (lines 195–201). The whiteboard analogy is helpful for understanding the concept, but for ongoing maintenance two separate explanations of the same thing will drift out of sync.
Consider consolidating to a single comment at one location — either here or there, not both.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-on-rails-pro-node-renderer/src/worker/vm.ts (1)
351-368: Consider shortening this block comment to the technical invariant only.The analogy is helpful, but this is longer than needed in core runtime code and may be harder to maintain over time.
✂️ Suggested concise replacement
- // 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. + // Always clear `vmCreationPromises[filePath]` after this build settles. + // The cleanup is attached after `set()` so synchronous pre-`await` throws + // cannot leave a stale rejected promise that blocks retries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro-node-renderer/src/worker/vm.ts` around lines 351 - 368, Reduce the long explanatory analogy in the vm.ts comment to a concise technical invariant: explain only that synchronous throws (e.g., from vm.createContext() or vm.runInContext() before awaiting readFileAsync) can leave a stale rejected entry unless cleanup is chained after vmCreationPromises.set(), and that chaining .finally() on the stored promise ensures cleanup runs as a microtask after set() has executed; keep references to vmCreationPromises, vm.createContext(), vm.runInContext(), readFileAsync, and the .finally() cleanup behavior so the intent is unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/src/worker/vm.ts`:
- Around line 351-368: Reduce the long explanatory analogy in the vm.ts comment
to a concise technical invariant: explain only that synchronous throws (e.g.,
from vm.createContext() or vm.runInContext() before awaiting readFileAsync) can
leave a stale rejected entry unless cleanup is chained after
vmCreationPromises.set(), and that chaining .finally() on the stored promise
ensures cleanup runs as a microtask after set() has executed; keep references to
vmCreationPromises, vm.createContext(), vm.runInContext(), readFileAsync, and
the .finally() cleanup behavior so the intent is unambiguous.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-on-rails-pro-node-renderer/src/worker/vm.tspackages/react-on-rails-pro-node-renderer/tests/vm.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails-pro-node-renderer/tests/vm.test.ts
resetVM() only cleared vmContexts but not vmCreationPromises. If resetVM() was called while a VM creation was in-flight, the stale promise would remain in vmCreationPromises, causing subsequent buildVM() calls to return the old (possibly rejected) promise. This matches the cleanup already done in removeVM(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review: Fix buildVM promise cleanup orderingThe fix is correct and the root cause analysis is accurate. Well done. Core Fix AssessmentThe problem is real: an async IIFE that throws synchronously (before its first What's Good
Minor Issues1. The 2. The cleanup removes the 3. Comment verbosity The whiteboard analogy (9 lines) followed by the technical explanation (7 lines) in the same comment block is longer than necessary. The technical "Technically:" paragraph is sufficient on its own. 4. Test doesn't restore config on failure path The new test mutates VerdictApprove with suggestions. The bug fix is correct, the test directly validates the fixed scenario, and the |
| // set() guarantees cleanup runs as a microtask after the current synchronous | ||
| // execution, so set() has always run first. | ||
| void vmCreationPromise | ||
| .catch(() => {}) |
There was a problem hiding this comment.
The .catch(() => {}) here is load-bearing and its purpose should be documented. Without it, .finally() on the rejected vmCreationPromise would produce a second rejected promise on this cleanup chain — a separate unhandled rejection from the one already returned to callers via return vmCreationPromise. Adding .catch(() => {}) first swallows the rejection on this side-chain only, while the caller still receives the original rejected promise. Consider adding a brief comment:
| .catch(() => {}) | |
| // .catch() here prevents Node.js from flagging the cleanup chain itself as an | |
| // unhandled rejection. The original rejection still propagates to callers via | |
| // `return vmCreationPromise` below. | |
| void vmCreationPromise | |
| .catch(() => {}) | |
| .finally(() => { | |
| 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); |
There was a problem hiding this comment.
Worth noting explicitly that the .finally() also fires on success, not just failure — and that is intentional. vmCreationPromises only tracks in-flight builds. On success, once the promise resolves, this entry is cleaned up and future buildVM calls for the same path take the vmContexts.get() early-return path (line ~188) instead. A short note here would help future readers understand this is by design and not a bug:
| vmCreationPromises.set(filePath, vmCreationPromise); | |
| // Store the promise BEFORE any async work completes, so concurrent callers | |
| // find it via the has() check above. | |
| vmCreationPromises.set(filePath, vmCreationPromise); | |
| // vmCreationPromises only tracks *in-flight* builds; completed (or failed) | |
| // builds are cleaned up here. On success, future callers use the vmContexts | |
| // early-return path above. On failure, cleanup unblocks retries. |
| }); | ||
|
|
||
| // First call fails synchronously during vm.createContext() | ||
| await expect(buildVM(serverBundlePath)).rejects.toThrow('sync context creation failure'); |
There was a problem hiding this comment.
The timing here works correctly, but it's worth understanding why. When buildVM() throws synchronously inside the IIFE, vmCreationPromise is already a rejected promise by the time .finally() is scheduled. The await on this line drains the microtask queue — which includes the .finally() cleanup callback — before proceeding to the retry. So by the time line 572 executes, vmCreationPromises is already clean. This is subtle but correct; no explicit microtask flush is needed.
2 participants
Summary
Fixes an intermittent SSR error:
"The ReactOnRails value exists in the [object Object] scope, it may not be safe to overwrite it"Fixes the
buildVM()promise cleanup ordering so that a failed build doesn't leave a stale rejected promise permanently stuck in thevmCreationPromisesmap, blocking all retries for that bundle path.Root cause
The node renderer's
buildVM()inworker/vm.tsuses avmCreationPromisesMap to deduplicate concurrent calls for the same bundle. The cleanup (vmCreationPromises.delete()) was inside atry/finallyblock in the async IIFE, butvmCreationPromises.set()ran after the IIFE was created.An async IIFE executes synchronously until its first
await. If the code throws before anyawait, the IIFE'sfinallyblock runs synchronously — beforevmCreationPromises.set()stores the promise. This leaves a stale rejected promise permanently in the map.Here's the exact sequence with line numbers from the unpatched
vm.ts:Fix
Move cleanup from the IIFE's
try/finallyto a.finally()chained on the promise aftervmCreationPromises.set():Chaining
.finally()on the promise afterset()guarantees cleanup runs as a microtask after the current synchronous execution, soset()has always completed first — regardless of whether the failure is synchronous or asynchronous.Test plan
vm.test.tstests passhandleRenderRequest.test.tstests passbuildVM recovers after failure for nonexistent bundleverifies the cleanup ordering fixFixes #2483
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests