fix: prevent assertions from returning wrong values during large iteration runs#7692
fix: prevent assertions from returning wrong values during large iteration runs#7692gopu-bruno wants to merge 2 commits intousebruno:mainfrom
Conversation
gopu-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughSwitched QuickJS initialization from eagerly creating and caching a reusable context to lazy-loading and caching the QuickJS WASM module. The module is loaded once and reused; each VM execution now creates a new QuickJS context via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-js/src/sandbox/quickjs/index.js (1)
59-89:⚠️ Potential issue | 🟠 MajorDispose every fresh QuickJS context to prevent WASM memory leaks.
Lines 59 and 100 allocate a new VM per evaluation but never release it. Each
newContext()must be explicitly disposed withvm.dispose(), or the underlying QuickJS/WASM resources will leak. Long-running workloads will accumulate contexts and shim handles until garbage collection.Handles are already disposed correctly (
result.error.dispose(),result.value.dispose()), so wrap the context lifecycle in a finally block:Fix for lines 59–89 (sync path)
- const vm = QuickJSModule.newContext(); - - try { + let vm; + try { + vm = QuickJSModule.newContext();} catch (error) { console.error('Error executing the script!', error); + } finally { + vm?.dispose(); }Fix for lines 99–169 (async path)
- try { + let vm; + try { const module = await loader(); - const vm = module.newContext(); + vm = module.newContext();} catch (error) { error.__isQuickJS = true; throw error; + } finally { + vm?.dispose(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/index.js` around lines 59 - 89, The QuickJS context created by QuickJSModule.newContext() isn't disposed, causing WASM memory leaks; update the sync path in the function that uses vm to ensure vm.dispose() is always called in a finally block after any handle disposals (result.error.dispose() / result.value.dispose()), so: create vm via QuickJSModule.newContext(), run the try block exactly as-is, then in finally call vm.dispose(); apply the same pattern to the async path that also creates a vm; keep existing handle disposal (result.error/result.value) before disposing the vm and ensure any shims added by addBruShimToContext/addBrunoRequestShimToContext/addBrunoResponseShimToContext and values from marshallToVm remain valid until after those disposals.
🤖 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/bruno-js/src/sandbox/quickjs/index.js`:
- Around line 17-19: QuickJSModule is set asynchronously via loader(), but
executeQuickJsVm() dereferences QuickJSModule/newContext before the try block;
move the creation of the QuickJS context (calls to QuickJSModule.newContext() or
newContext()) inside the try within executeQuickJsVm() and explicitly check for
QuickJSModule presence (or await loader() / throw a clear "QuickJS not loaded"
error) so the not-yet-loaded case is handled and falls into the catch path
instead of raising an uncaught exception.
---
Outside diff comments:
In `@packages/bruno-js/src/sandbox/quickjs/index.js`:
- Around line 59-89: The QuickJS context created by QuickJSModule.newContext()
isn't disposed, causing WASM memory leaks; update the sync path in the function
that uses vm to ensure vm.dispose() is always called in a finally block after
any handle disposals (result.error.dispose() / result.value.dispose()), so:
create vm via QuickJSModule.newContext(), run the try block exactly as-is, then
in finally call vm.dispose(); apply the same pattern to the async path that also
creates a vm; keep existing handle disposal (result.error/result.value) before
disposing the vm and ensure any shims added by
addBruShimToContext/addBrunoRequestShimToContext/addBrunoResponseShimToContext
and values from marshallToVm remain valid until after those disposals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fb60a04-a5eb-477d-99bc-e5960bda56b7
📒 Files selected for processing (1)
packages/bruno-js/src/sandbox/quickjs/index.js
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 (2)
packages/bruno-js/src/sandbox/quickjs/index.js (2)
59-88:⚠️ Potential issue | 🟠 MajorMissing
vm.dispose()— memory leak in long-running iterations.The context is created on line 60 but never disposed. Each invocation allocates WASM memory that won't be reclaimed until the process exits. Over 350+ iterations (the exact scenario this PR targets), this could accumulate significant memory.
Ensure disposal happens in all exit paths (success, error, and the outer catch).
🛠️ Proposed fix
try { const vm = QuickJSModule.newContext(); const { bru, req, res, ...variables } = externalContext; bru && addBruShimToContext(vm, bru); req && addBrunoRequestShimToContext(vm, req); res && addBrunoResponseShimToContext(vm, res); Object.entries(variables)?.forEach(([key, value]) => { vm.setProp(vm.global, key, marshallToVm(value, vm)); }); const templateLiteralText = `\`${externalScript}\``; const jsExpressionText = `${externalScript}`; let scriptText = scriptType === 'template-literal' ? templateLiteralText : jsExpressionText; const result = vm.evalCode(scriptText); if (result.error) { let e = vm.dump(result.error); result.error.dispose(); + vm.dispose(); return e; } else { let v = vm.dump(result.value); result.value.dispose(); + vm.dispose(); return v; } } catch (error) { console.error('Error executing the script!', error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/index.js` around lines 59 - 88, The QuickJS context created via QuickJSModule.newContext() is never disposed causing a WASM memory leak; ensure vm.dispose() is called on every exit path (after handling result from vm.evalCode(scriptText) and inside the catch and error branches). Update the block around QuickJSModule.newContext(), vm.evalCode(), and the result handling (inspect result.error / result.value) to always call vm.dispose() after dumping and disposing result.error/result.value, and also call vm.dispose() in the outer catch before rethrowing or returning to guarantee the context is freed.
167-167:⚠️ Potential issue | 🟠 MajorClarify why
vm.dispose()is commented out.Same concern as the sync version — without disposal, each async invocation leaks the context's WASM memory. If there's a specific reason (e.g., disposal causing issues with promise resolution), please document it. Otherwise, consider uncommenting or wrapping in a
finallyblock to ensure cleanup.- // vm.dispose(); + vm.dispose(); return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/index.js` at line 167, The commented-out vm.dispose() in the async QuickJS path leaves WASM memory leaking across async invocations; either restore cleanup by calling vm.dispose() in a finally block after the async run (so disposal always occurs even on errors) or add a clear code comment above the commented line explaining the exact reason it must remain disabled (including the specific promise-resolution issue and a ticket/issue link). Locate the async invocation flow where vm.dispose() is currently commented and implement the finally-based disposal (or documented exception) to prevent WASM memory leaks.
🤖 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/bruno-js/src/sandbox/quickjs/index.js`:
- Around line 59-88: The QuickJS context created via QuickJSModule.newContext()
is never disposed causing a WASM memory leak; ensure vm.dispose() is called on
every exit path (after handling result from vm.evalCode(scriptText) and inside
the catch and error branches). Update the block around
QuickJSModule.newContext(), vm.evalCode(), and the result handling (inspect
result.error / result.value) to always call vm.dispose() after dumping and
disposing result.error/result.value, and also call vm.dispose() in the outer
catch before rethrowing or returning to guarantee the context is freed.
- Line 167: The commented-out vm.dispose() in the async QuickJS path leaves WASM
memory leaking across async invocations; either restore cleanup by calling
vm.dispose() in a finally block after the async run (so disposal always occurs
even on errors) or add a clear code comment above the commented line explaining
the exact reason it must remain disabled (including the specific
promise-resolution issue and a ticket/issue link). Locate the async invocation
flow where vm.dispose() is currently commented and implement the finally-based
disposal (or documented exception) to prevent WASM memory leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32e41c60-9d8c-4f98-8cd2-bb621e209e4f
📒 Files selected for processing (1)
packages/bruno-js/src/sandbox/quickjs/index.js
1 participant
Description
When running collections with
--iteration-count=350, assertions usingres("...")queries would start returning wrong values (e.g., "bearer", "POST", "none") around iteration ~310. Once corrupted, all subsequentres()evaluations returned the same incorrect value.Root cause
executeQuickJsVmreused a singleQuickJSSyncContextfor every evaluation. Shims (addBruShimToContext,addBrunoResponseShimToContext, etc.) set properties onvm.globaleach call, and handles accumulated on the shared context over hundreds of iterations, eventually causing QuickJS to return stale/corrupted data.Changes
executeQuickJsVmcall instead of reusing a single shared context across all evaluationsexecuteQuickJsVmAsyncvialoader()instead of loading a new module every callFixes: #7167
Tracked: JIRA
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit