fix(ai-isolate-quickjs): bridge host tools with native promises#946
fix(ai-isolate-quickjs): bridge host tools with native promises#946TheBoyWhoLivedd wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughQuickJS isolate execution now uses synchronous contexts with native promise bridging for asynchronous host tools. Execution coordinates deadlines and terminal disposal, while tests cover repeated awaits, context reuse, concurrent calls, and timeout behavior. ChangesQuickJS promise bridge
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant QuickJSWrapper
participant QuickJSBinding
participant ToolBinding
participant QuickJSRuntime
QuickJSWrapper->>QuickJSBinding: invoke injected tool promise
QuickJSBinding->>ToolBinding: execute parsed input
ToolBinding-->>QuickJSBinding: return success or error JSON
QuickJSBinding->>QuickJSRuntime: resolve promise and run pending jobs
QuickJSRuntime-->>QuickJSWrapper: return value or throw error
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ai-isolate-quickjs/src/isolate-context.ts (1)
96-131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
executePendingJobs()results in both QuickJS drains
packages/ai-isolate-quickjs/src/isolate-context.ts#L106: check theexecutePendingJobs()result and dispose any returned error handle instead of letting an interrupted drain leaveawait nativePromisehanging.packages/ai-isolate-quickjs/src/isolate-driver.ts#L74: do the same here so the VM-side wakeup doesn’t swallow the error or leak the handle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-isolate-quickjs/src/isolate-context.ts` around lines 96 - 131, Handle the return value from executePendingJobs() in both packages/ai-isolate-quickjs/src/isolate-context.ts lines 96-131 and packages/ai-isolate-quickjs/src/isolate-driver.ts lines 54-100: detect interrupted/error results, dispose any returned error handle, and propagate the failure so awaiting the QuickJS promise cannot hang or silently leak resources.
🧹 Nitpick comments (1)
packages/ai-isolate-quickjs/src/isolate-driver.ts (1)
150-160: 🚀 Performance & Scalability | 🔵 TrivialSingle
toolQueueserializes every host tool call for the whole context, not just per binding.
enqueueis created once percreateContextcall and shared across all bindings injected in the loop below (Line 205-207), confirmed by the "preserves one active host call for Promise.all" test. This means calling two independent tools concurrently (e.g.Promise.all([toolA(), toolB()])) is now fully serialized on the host side too, even though native promises (unlike Asyncify) no longer require a single in-flight suspension. If this broad serialization is intentional (e.g., for predictable ordering/log interleaving), it's worth documenting; otherwise, consider scoping the queue per binding to allow independent tools to execute concurrently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-isolate-quickjs/src/isolate-driver.ts` around lines 150 - 160, Scope the host-call queue to each injected binding instead of sharing the single toolQueue created in createContext across all bindings. Move or create the queue alongside each binding’s enqueue logic in the binding-injection loop, preserving serialization for repeated calls to the same tool while allowing independent tools to execute concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/ai-isolate-quickjs/src/isolate-context.ts`:
- Around line 96-131: Handle the return value from executePendingJobs() in both
packages/ai-isolate-quickjs/src/isolate-context.ts lines 96-131 and
packages/ai-isolate-quickjs/src/isolate-driver.ts lines 54-100: detect
interrupted/error results, dispose any returned error handle, and propagate the
failure so awaiting the QuickJS promise cannot hang or silently leak resources.
---
Nitpick comments:
In `@packages/ai-isolate-quickjs/src/isolate-driver.ts`:
- Around line 150-160: Scope the host-call queue to each injected binding
instead of sharing the single toolQueue created in createContext across all
bindings. Move or create the queue alongside each binding’s enqueue logic in the
binding-injection loop, preserving serialization for repeated calls to the same
tool while allowing independent tools to execute concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1acaa342-f4f1-4fa1-b079-223e3369eccc
📒 Files selected for processing (4)
.changeset/quickjs-native-promises.mdpackages/ai-isolate-quickjs/src/isolate-context.tspackages/ai-isolate-quickjs/src/isolate-driver.tspackages/ai-isolate-quickjs/tests/isolate-driver.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai-isolate-quickjs/src/isolate-context.ts`:
- Around line 103-124: Update requestVmDispose and the host-call tracking in
beginHostCall so dispose does not wait indefinitely for pending host promises;
add cancellation or a bounded teardown fallback that guarantees vm.dispose()
eventually runs even when a host call never completes, while preserving the
existing immediate teardown when no calls are pending and preventing duplicate
disposal.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09fcf589-23ab-4785-b52a-5e2d50cc5ad9
📒 Files selected for processing (3)
packages/ai-isolate-quickjs/src/isolate-context.tspackages/ai-isolate-quickjs/src/isolate-driver.tspackages/ai-isolate-quickjs/tests/isolate-driver.test.ts
|
Hi @TheBoyWhoLivedd , thank you for raising a PR to fix this issue. Another PR was raised just after yours - #948 which has a more complete solution to the problem. Whilst I'd usually take the first solution, it was so close in timing, I'd prefer to go with 948 as it's closer to done. I'd invite you to please review #948 as well and contribute anything you think needs changing. I'm closing this one |
|
Thanks for the explanation. I’ll follow #948. I’m glad the teardown work from this PR was useful during its review. |
2 participants
🎯 Changes
Replace Asyncify-based host tool callbacks in
@tanstack/ai-isolate-quickjswith native QuickJS promises returned from synchronous host functions.The Asyncify bridge can corrupt a reused QuickJS context when guest code performs multiple sequential awaited host calls within one execution. Depending on the number of awaits and guest allocations, the failure can appear during that execution or during a later evaluation on the same context.
This change:
The failure is independently reproduced here:
https://github.com/TheBoyWhoLivedd/quickjs-emscripten-asyncify-multi-await-repro
Related upstream QuickJS work:
The TanStack change does not depend on the upstream patch. It avoids the affected Asyncify path and provides a safe implementation for currently published QuickJS versions.
A patch changeset for
@tanstack/ai-isolate-quickjsis included.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests