fix(ai-isolate-quickjs): replace asyncify host functions with promise bridging #948
fix(ai-isolate-quickjs): replace asyncify host functions with promise bridging #948SilvioYMollov wants to merge 10 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThe QuickJS isolate driver now uses synchronous contexts and promise-based host-tool bridging. Execution tracks shared deadlines and pending cancellations, normalizes timeout interrupts, disposes timed-out contexts, and adds coverage for sequential, concurrent, and terminal timeout behavior. ChangesQuickJS promise bridge and execution lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant QuickJS
participant QuickJSIsolateContext
participant ToolBinding
QuickJS->>QuickJSIsolateContext: execute guest code
QuickJSIsolateContext->>ToolBinding: invoke promise-backed binding
ToolBinding-->>QuickJSIsolateContext: return result or error envelope
QuickJSIsolateContext-->>QuickJS: resolve guest promise
QuickJSIsolateContext->>QuickJSIsolateContext: enforce deadline and dispose on timeout
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
SilvioYMollov
changed the title
SilvioYMollov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
276-288: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
dispose()can double-dispose (or wrongly free a leaked VM) after awaiting the queue.
dispose()checksthis.disposedonce, before awaitingthis.execQueue. If anexecute()call is in flight and it finishes viareleaseAfterTimeout()/releaseVmAfterFatalError()whiledispose()is waiting,this.disposedbecomestrueandthis.vmmay already be disposed (or intentionally left undisposed becauseguestSettledwas stillfalse— the documented "leak instead of crash" case for an unsettled program promise). Once the await resolves,dispose()unconditionally doesthis.disposed = true; this.vm.dispose()again:
- If the VM was already disposed → double-dispose (this library errors on invalid/duplicate handle operations).
- If the VM was intentionally left un-disposed because the guest program promise never settled → this call disposes it anyway, which is exactly the "freeing a runtime that still holds an unsettled program promise aborts the shared WASM module" failure this file's own comments warn about.
Neither existing timeout test calls
dispose()concurrently with an in-flightexecute(), so this path is untested.🐛 Proposed fix
if (this.executing) { await this.execQueue + // The in-flight execute() may have already disposed (or intentionally + // left undisposed) the VM via releaseAfterTimeout/releaseVmAfterFatalError. + if (this.disposed) return }🤖 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 276 - 288, Recheck this.disposed after awaiting this.execQueue in dispose() and return without touching the VM if execution cleanup already disposed or intentionally retained it. Only set this.disposed and call this.vm.dispose() when the context remains undisposed after the wait, preserving the unsettled-program leak behavior.
🧹 Nitpick comments (1)
packages/ai-isolate-quickjs/tests/isolate-driver.test.ts (1)
217-237: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding a regression test for
dispose()racing an in-flight timeout.These tests cover the sequential case (timeout resolves, then
dispose()is called afterward). They don't cover callingcontext.dispose()while a timing-outexecute()is still in-flight — which is exactly the scenario where thedispose()double-release issue (flagged inisolate-context.ts) manifests. Not blocking, but would have caught that bug.🤖 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/tests/isolate-driver.test.ts` around lines 217 - 237, Add a regression test alongside the existing timeout disposal test that starts a timing-out context.execute call, invokes context.dispose() before execution settles, and awaits both promises. Assert disposal resolves successfully and the in-flight execution completes with the expected timeout or disposed outcome, confirming no double-release failure.
🤖 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/error-normalizer.ts`:
- Around line 46-54: Update normalizeError in
packages/ai-isolate-quickjs/src/error-normalizer.ts to detect interrupted
QuickJS errors in the dumped plain-object branch, requiring name InternalError
and an interrupted message before returning TimeoutError. Keep the existing
Error-instance check similarly scoped to InternalError, and update the
jobs.error handling in packages/ai-isolate-quickjs/src/isolate-context.ts only
as needed to preserve releaseAfterTimeout() for these normalized interrupts.
In `@packages/ai-isolate-quickjs/src/isolate-driver.ts`:
- Around line 67-140: Update the settled handler in injectBinding so the
QuickJSDeferredPromise created by vm.newPromise() is disposed after
executePendingJobs() completes, including the jobs.error handling path. Ensure
disposal occurs once per tool call after pending jobs are drained, while
preserving the existing runtime-alive guard and error cleanup.
---
Outside diff comments:
In `@packages/ai-isolate-quickjs/src/isolate-context.ts`:
- Around line 276-288: Recheck this.disposed after awaiting this.execQueue in
dispose() and return without touching the VM if execution cleanup already
disposed or intentionally retained it. Only set this.disposed and call
this.vm.dispose() when the context remains undisposed after the wait, preserving
the unsettled-program leak behavior.
---
Nitpick comments:
In `@packages/ai-isolate-quickjs/tests/isolate-driver.test.ts`:
- Around line 217-237: Add a regression test alongside the existing timeout
disposal test that starts a timing-out context.execute call, invokes
context.dispose() before execution settles, and awaits both promises. Assert
disposal resolves successfully and the in-flight execution completes with the
expected timeout or disposed outcome, confirming no double-release failure.
🪄 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: 12c506fa-80e8-43ab-ac16-5c53f12bbffb
📒 Files selected for processing (5)
.changeset/quickjs-promise-bridge.mdpackages/ai-isolate-quickjs/src/error-normalizer.tspackages/ai-isolate-quickjs/src/isolate-context.tspackages/ai-isolate-quickjs/src/isolate-driver.tspackages/ai-isolate-quickjs/tests/isolate-driver.test.ts
| // QuickJS reports a fired interrupt handler as "InternalError: interrupted". | ||
| if (lower.includes('interrupted')) { | ||
| return { | ||
| name: TIMEOUT_ERROR, | ||
| message: 'Code execution timed out', | ||
| stack: error.stack, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files packages/ai-isolate-quickjs/src | sed -n '1,200p'
echo '--- error-normalizer.ts ---'
wc -l packages/ai-isolate-quickjs/src/error-normalizer.ts
sed -n '1,220p' packages/ai-isolate-quickjs/src/error-normalizer.ts
echo '--- isolate-context.ts ---'
wc -l packages/ai-isolate-quickjs/src/isolate-context.ts
sed -n '1,280p' packages/ai-isolate-quickjs/src/isolate-context.ts
echo '--- normalizeError usages ---'
rg -n "normalizeError|interrupted|InternalError|TIMEOUT_ERROR|jobs.error|executePendingJobs|pendingJobs" packages/ai-isolate-quickjs/srcRepository: TanStack/ai
Length of output: 14755
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
Path('packages/ai-isolate-quickjs/src/error-normalizer.ts'),
Path('packages/ai-isolate-quickjs/src/isolate-context.ts'),
]
for p in files:
print(f"===== {p} =====")
text = p.read_text()
for needle in ['interrupted', 'InternalError', 'TIMEOUT_ERROR', 'normalizeError', 'jobs.error', 'executePendingJobs']:
if needle in text:
print(f"-- contains {needle}")
print()
PYRepository: TanStack/ai
Length of output: 512
Handle QuickJS interrupts in the dumped-object path too
normalizeError() only maps "interrupted" to TimeoutError for Error instances. The jobs.error path in packages/ai-isolate-quickjs/src/isolate-context.ts dumps the QuickJS error into a plain object first, so interrupts there still normalize to InternalError and skip releaseAfterTimeout(). Add the same InternalError/interrupted check to the object branch, and keep the Error branch scoped to InternalError to avoid misclassifying unrelated errors.
📍 Affects 2 files
packages/ai-isolate-quickjs/src/error-normalizer.ts#L46-L54(this comment)packages/ai-isolate-quickjs/src/isolate-context.ts#L161-L198
🤖 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/error-normalizer.ts` around lines 46 - 54,
Update normalizeError in packages/ai-isolate-quickjs/src/error-normalizer.ts to
detect interrupted QuickJS errors in the dumped plain-object branch, requiring
name InternalError and an interrupted message before returning TimeoutError.
Keep the existing Error-instance check similarly scoped to InternalError, and
update the jobs.error handling in
packages/ai-isolate-quickjs/src/isolate-context.ts only as needed to preserve
releaseAfterTimeout() for these normalized interrupts.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tombeckenham
left a comment
tombeckenham
left a comment
There was a problem hiding this comment.
Review summary
Strong root-cause fix for #947 / quickjs-emscripten#258. Promise bridging, host-side deadlines, terminal timeouts, and abandoned-tool dispose are the right direction — and the regression suite is substantially better than the competing PR.
Requesting changes for one FreeRuntime hole and a few important follow-ups before merge.
Freshness
- Behind
mainby ~6 commits; MERGEABLE, no real conflict withpackages/ai-isolate-quickjs(only a version bump on main). - Still fully applicable; soft rebase is fine, not blocking.
- Prefer this PR over #946 as the landing vehicle (more complete lifecycle, migration notes, broader tests). One idea worth porting from #946: cancel pending host deferreds on fatal OOM/stack, not only timeout.
Must fix
- Fatal OOM/stack path does not settle tool deferreds before
vm.dispose()(inline). promise.settledearly-return can skip deferred dispose (inline).
Should fix
- Timeout path should assert shared WASM still healthy (new context after hanging-tool timeout / late host settle) — dispose path already does this.
- Stale product docs still describe asyncify + global queue (
docs/code-mode/code-mode-isolates.md, package README, code-mode skill). - Early
jobs.errorafterguestSettled = falsecan leave a reusable dirty context (inline). guestSettledintro comment is inaccurate for busy-loop interrupt (inline).
Strengths
- Correct asyncify removal; dual timeout (interrupt + host deadline).
- Intentional leak when guest cannot settle after timeout.
- Excellent
Promise.raceabandon + dispose regression (process-level WASM health). - Changeset migration notes (sync WASM + terminal timeouts) match code-mode per-execution context usage.
- Review follow-ups (dispose race recheck, deferred settle on dispose) look solid.
Process
- Full TanStack CI has not run on this fork head (only CodeRabbit/Socket) — please ensure workflows are approved/green before merge.
- After the fatal-path cancel lands, unit tests covering “OOM with in-flight tool → new context still works” would lock the invariant.
| @@ -76,10 +158,37 @@ export class QuickJSIsolateContext implements IsolateContext { | |||
| this.vm.dispose() | |||
There was a problem hiding this comment.
Critical — FreeRuntime hole on fatal limits
releaseAfterTimeout and dispose() both cancel pendingCancels before vm.dispose() because unsettled QuickJS deferreds abort the shared WASM module (list_empty(&rt->gc_obj_list)). This path does not.
Repro shape:
const p = hangTool({}) // registers deferred + pendingCancel
return 'x'.repeat(8 * 1024 * 1024) // MemoryLimitError while deferred still liveExisting OOM tests never leave a tool in flight, so this is unguarded.
Suggested fix: best-effort cancel + grace tick (same as timeout/dispose) before free; if the sick heap cannot cancel, catch and prefer intentional leak over aborting the shared module. See also #946’s requestVmDispose(), which already cancels on both timeout and fatal limits.
Test to add: start a never-resolving tool, then OOM (or stack overflow); assert a new context from the same driver still executes successfully.
There was a problem hiding this comment.
Thank you for pointing this out! Fatal limit cleanup now settles pending tool deferred before disposing the VM. I also added an OOM regression test that verifies a new context still works afterward, using your repro suggestion.
| // active execute() the interrupt deadline is 0, so a stray job from an | ||
| // abandoned execution is interrupted instead of running unbounded. | ||
| void promise.settled.then(() => { | ||
| if (!vm.runtime.alive) return |
There was a problem hiding this comment.
Important — early return skips promise.dispose()
If settled runs after the runtime is already dead, this returns before the try/finally that calls promise.dispose(). Paired with the fatal path that frees without cancelling deferreds, host-side deferred lifetimes can outlive FreeRuntime.
Please dispose unconditionally, e.g.:
void promise.settled.then(() => {
try {
if (vm.runtime.alive) {
const jobs = vm.runtime.executePendingJobs()
// ...
}
} finally {
if (promise.alive) promise.dispose()
}
})| if (jobs.error) { | ||
| const dumped: unknown = this.vm.dump(jobs.error) | ||
| jobs.error.dispose() | ||
| return await fail(dumped) |
There was a problem hiding this comment.
Important — dirty context reuse after early jobs.error
Here guestSettled is already false and nativePromise is never awaited. fail() only terminal-disposes for TimeoutError / memory / stack.
For any other jobs.error, the context stays reusable while the prior program promise / jobs may still be pending — next execute() can interleave leftover work under a fresh deadline.
Prefer one of: await/cancel the program promise, treat as terminal (dispose), or fully drain/cancel outstanding work before returning a non-terminal error.
There was a problem hiding this comment.
Fixed by treating an early pending-job error as terminal and cancelling any outstanding work before safely releasing the context.
|
|
||
| const releaseVmAfterFatalLimit = () => { | ||
| // True until a program promise is in flight; sync failure paths (parse | ||
| // errors, interrupted straight-line code) leave no unsettled guest state. |
There was a problem hiding this comment.
Nit — comment contradicts busy-loop timeout path
This says interrupted straight-line code leaves no unsettled guest state, but the busy-loop timeout sets guestSettled = false before executePendingJobs() and relies on the intentional VM leak when it stays false (releaseAfterTimeout).
Suggest rewriting along the lines of: starts true; cleared once resolvePromise is armed; stays false if the program promise never settles (e.g. interrupt during a spin); only pre-arm failures leave it true so timeout release can dispose.
| expect(second.success).toBe(false) | ||
| expect(second.error?.name).toBe('DisposedError') | ||
|
|
||
| await expect(context.dispose()).resolves.toBeUndefined() |
There was a problem hiding this comment.
Important — assert process-level WASM health after timeout cancel
This correctly checks context terminality (DisposedError on reuse). The dispose + abandoned-tool test below goes further and proves a new context still works after FreeRuntime-sensitive cleanup.
Please extend this path similarly:
- After hanging-tool
TimeoutError,createContext+executeon the same driver succeeds. - Optionally: host tool resolves after timeout cancel; second settle must no-op without poisoning the module.
That locks the same invariant the dispose path already guards.
There was a problem hiding this comment.
Extended the test coverage, it now resolves the host tool after timeout cancellation and verifies that a new context still executes successfully, confirming the late settlement is a no-op and the shared WASM module remains healthy.
|
View your CI Pipeline Execution ↗ for commit 3ddf160
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-acp
@tanstack/ai-angular
@tanstack/ai-anthropic
@tanstack/ai-bedrock
@tanstack/ai-claude-code
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-codex
@tanstack/ai-devtools-core
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-grok-build
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-mcp
@tanstack/ai-mistral
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-opencode
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-sandbox
@tanstack/ai-sandbox-cloudflare
@tanstack/ai-sandbox-daytona
@tanstack/ai-sandbox-docker
@tanstack/ai-sandbox-local-process
@tanstack/ai-sandbox-sprites
@tanstack/ai-sandbox-vercel
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-utils
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/openai-base
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
…live in injectBinding
…ts stale job leakage
2 participants
Changes
Replaces the Asyncify-based host tool bridge in @tanstack/ai-isolate-quickjs with native QuickJS promises, and makes execution timeouts actually work under the new model.
Problem
Two bugs with the same root cause:
// crashes on the second await
const a = await get({ path: "/a" });
const b = await get({ path: "/b" });
Fix
Tests
Regression tests for sequential awaits, Promise.all tool calls, TimeoutError on timeout, and context disposal after timeout.
Related
Summary by CodeRabbit
TimeoutError, cancel pending tool calls, and make the execution context terminal (subsequent executions fail withDisposedError).