Store existing isolated in global and reuse it #1914
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes encompass multiple files primarily focused on enhancing the handling of global variables and the instantiation of isolated virtual machines within the bot's engine. Key modifications include the introduction of a new function Changes
Sequence Diagram(s)sequenceDiagram
participant Bot
participant Store
participant CodeRunner
participant Isolate
Bot->>Store: resetVariablesGlobals()
Store->>Isolate: Dispose current isolate
Isolate-->>Store: Isolate disposed
Store->>Store: Set isolate to undefined
Bot->>CodeRunner: createInlineSyncCodeRunner()
CodeRunner->>Store: Retrieve isolate
alt Isolate exists
CodeRunner-->>Isolate: Use existing isolate
else Isolate does not exist
CodeRunner->>Isolate: Create new isolate
end
CodeRunner-->>Bot: Return code runner instance
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/variables/src/codeRunners.ts (1)
Line range hint
6-19: Review timeout and security boundariesThe code execution has several potential concerns:
- The 10-second timeout might be too long for inline code execution
- No memory limits are set on the isolate
- The global context exposure via
jail.setSync("global", jail.derefInto())might be unnecessaryConsider applying these security improvements:
- const isolate = variablesGlobals.isolate ?? new ivm.Isolate(); + const isolate = variablesGlobals.isolate ?? new ivm.Isolate({ memoryLimit: 128 }); const context = isolate.createContextSync(); const jail = context.global; - jail.setSync("global", jail.derefInto()); variables.forEach((v) => { jail.setSync(v.id, parseTransferrableValue(parseGuessedValueType(v.value))); }); return (code: string) => context.evalClosureSync( `return (function() { return new Function($0)(); }())`, [code], - { result: { copy: true }, timeout: 10000 }, + { result: { copy: true }, timeout: 5000 }, );packages/variables/src/executeFunction.ts (2)
39-42: Good improvement using Map for variable updatesThe switch from object to Map is a good improvement for variable management. However, consider adding type safety:
- const variableUpdates = new Map<string, unknown>(); + const variableUpdates = new Map<string, Variable['value']>(); - const setVariable = (key: string, value: any) => { + const setVariable = (key: string, value: Variable['value']) => {
Line range hint
84-94: Improve type safety in variable mappingThe variable mapping logic could benefit from stronger typing and error handling.
- newVariables: Array.from(variableUpdates.entries()) + newVariables: Array.from(variableUpdates.entries() as [string, Variable['value']][]) .map(([name, value]) => { const existingVariable = variables.find((v) => v.name === name); - if (!existingVariable) return; + if (!existingVariable) { + console.warn(`Variable ${name} not found in existing variables`); + return; + } return { id: existingVariable.id, name: existingVariable.name, value, }; })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/bot-engine/src/continueBotFlow.ts(2 hunks)packages/variables/src/codeRunners.ts(2 hunks)packages/variables/src/executeFunction.ts(3 hunks)packages/variables/src/parseVariables.ts(2 hunks)packages/variables/src/store.ts(1 hunks)
🔇 Additional comments (5)
packages/variables/src/store.ts (1)
3-5: Consider thread safety for multi-tenant scenarios
The global state pattern could lead to race conditions if multiple bot instances try to access/modify the isolate simultaneously. Consider implementing a proper synchronization mechanism or instance pooling.
Let's check for concurrent usage patterns:
packages/variables/src/parseVariables.ts (2)
3-3: LGTM: Import statement updated to use synchronous code runner.
The change from createCodeRunner to createInlineSyncCodeRunner aligns with the PR's objective of improving isolate management.
77-79: Verify error handling in synchronous execution.
The switch to synchronous code execution could potentially affect error handling behavior. Ensure that errors are still properly caught and handled in the try-catch block.
✅ Verification successful
Error handling is properly implemented in the synchronous execution
Looking at the code implementation, error handling is properly maintained:
- The
evaluateInlineCodefunction inparseVariables.tshas a try-catch block that falls back to parsing variables directly if the code execution fails - The
createInlineSyncCodeRunneruses isolated-vm for safe code execution with:- Proper context isolation
- 10-second timeout protection
- Safe value transfer between contexts
- Secure function evaluation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in the codebase
rg -A 5 "createInlineSyncCodeRunner.*catch"
Length of output: 45
Script:
#!/bin/bash
# Let's first check the implementation of createInlineSyncCodeRunner
ast-grep --pattern 'function createInlineSyncCodeRunner'
# Also search for any try-catch blocks around the usage
rg -A 5 "try.*createInlineSyncCodeRunner"
# Let's also check the file directly to understand the context
cat packages/variables/src/parseVariables.ts
Length of output: 5705
Script:
#!/bin/bash
# Let's check the implementation of createInlineSyncCodeRunner in the codeRunners file
cat packages/variables/src/codeRunners.ts
# Also search for any tests related to error handling
rg -A 5 "test.*error.*createInlineSyncCodeRunner|describe.*error.*createInlineSyncCodeRunner"
Length of output: 2182
packages/bot-engine/src/continueBotFlow.ts (2)
32-32: LGTM: Import statement added for variables globals reset.
The addition of resetVariablesGlobals import aligns with the PR's objective of managing global isolate state.
71-71: Verify the reset sequence.
The placement of resetVariablesGlobals() after resetGlobals() seems logical, but we should verify that this order is correct and that there are no dependencies between these resets that could cause issues.
✅ Verification successful
Let me gather more information about these reset functions to understand their implementations and dependencies.
Based on the gathered information, I can now provide the final response:
The reset sequence is correctly ordered and safe.
The implementation shows that these functions are independent and handle different concerns:
resetGlobals()resets email sending count and hash state in bot-engineresetVariablesGlobals()disposes and resets the isolated VM instance used for variable execution
The current order in continueBotFlow.ts is appropriate as there are no dependencies between these functions. Each handles its own isolated state cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other reset function calls to understand the reset sequence
rg -A 5 "resetGlobals\(\)|resetVariablesGlobals\(\)"
# Search for any potential dependencies between these functions
ast-grep --pattern 'function resetGlobals() {
$$$
}'
Length of output: 1097
Script:
#!/bin/bash
# Search for the implementation of both reset functions
ast-grep --pattern 'function resetVariablesGlobals() {
$$$
}'
# Look for any shared state or variables between these functions
rg -B 2 -A 5 "let.*Global|const.*Global|var.*Global"
# Search for any imports or declarations of these functions
rg "resetGlobals|resetVariablesGlobals" --type ts
Length of output: 6305
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/variables/src/codeRunners.ts (1)
Line range hint
6-57: Extract shared configuration and setup logic.Both runners share similar timeout values and context setup patterns. Consider extracting these to reduce duplication.
Create a shared utility:
const EVAL_TIMEOUT_MS = 10000; const createIsolateContext = () => { const isolate = getOrCreateIsolate(); const context = isolate.createContextSync(); const jail = context.global; jail.setSync("global", jail.derefInto()); return { isolate, context, jail }; };Then update both runners to use it:
export const createInlineSyncCodeRunner = ({ variables }: { variables: Variable[] }) => { const { context, jail } = createIsolateContext(); variables.forEach((v) => { jail.setSync(v.id, parseTransferrableValue(parseGuessedValueType(v.value))); }); return (code: string) => context.evalClosureSync( `return (function() { return new Function($0)(); }())`, [code], { result: { copy: true }, timeout: EVAL_TIMEOUT_MS }, ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/variables/src/codeRunners.ts(2 hunks)packages/variables/src/executeFunction.ts(3 hunks)packages/variables/src/getOrCreateIsolate.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/variables/src/executeFunction.ts
🔇 Additional comments (3)
packages/variables/src/getOrCreateIsolate.ts (1)
4-9: LGTM! Clean singleton pattern implementation.
The implementation correctly ensures a single Isolate instance is created and reused globally.
packages/variables/src/codeRunners.ts (2)
Line range hint 6-22: LGTM! Good refactor using centralized isolate management.
The function has been renamed to better reflect its synchronous nature and now uses the centralized isolate management.
6-8: Verify all callers are updated after the rename.
The function has been renamed from createCodeRunner to createInlineSyncCodeRunner. Let's verify all callers have been updated.
✅ Verification successful
All references to the renamed function have been properly updated
The verification shows that all occurrences of createInlineSyncCodeRunner are consistent:
- Function definition in
codeRunners.ts - Import and usage in
parseVariables.ts - No remaining references to the old name
createCodeRunner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old name
rg "createCodeRunner"
# Search for usage of the new name to confirm proper migration
rg "createInlineSyncCodeRunner"
Length of output: 332
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Could improve #1913