fix(isolate-quickjs): enforce timeout while suspended in host calls#905
fix(isolate-quickjs): enforce timeout while suspended in host calls#905jan-kubica wants to merge 1 commit into
Conversation
The only execution deadline was QuickJS's setInterruptHandler, which fires only while the sandbox steps bytecode. While the isolate is asyncify-suspended awaiting a host binding's Promise, no interrupt fires, so timeout was silently ignored and a slow or hung host binding could make execute() run far past the deadline or hang forever. execute() now races the run against a wall-clock timer so timeout bounds the whole execution, host-call suspensions included. On the deadline it returns a normalized TimeoutError immediately; because the suspended host Promise cannot be cancelled and the VM cannot be freed mid-asyncify, VM disposal and the execution-queue turn are deferred until the orphaned run unwinds (the still-armed interrupt handler aborts it once the host Promise settles). CPU-bound loops are still interrupted and leave the context reusable. Public API is unchanged.
📝 WalkthroughWalkthroughThis PR changes ChangesWall-clock timeout enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant QuickJSIsolateContext
participant VM as QuickJS VM
participant HostBinding
Caller->>QuickJSIsolateContext: execute(code)
QuickJSIsolateContext->>VM: evalCodeAsync(code)
VM->>HostBinding: suspend on host call (asyncify)
QuickJSIsolateContext->>QuickJSIsolateContext: race(run, wallClockTimer)
QuickJSIsolateContext->>QuickJSIsolateContext: timer wins, grace race
QuickJSIsolateContext->>QuickJSIsolateContext: mark timedOut, store orphanedRun
QuickJSIsolateContext-->>Caller: timeoutResult()
HostBinding-->>VM: host promise settles later
VM-->>QuickJSIsolateContext: run settles (suppressed by timedOut)
QuickJSIsolateContext->>VM: disposeVm()
QuickJSIsolateContext->>QuickJSIsolateContext: release queue turn
Related PRs: None identified. Suggested labels: package: ai-isolate-quickjs, bug Suggested reviewers: None identified. Poem 🚥 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 |
![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
🧹 Nitpick comments (1)
packages/ai-isolate-quickjs/tests/isolate-driver.test.ts (1)
155-201: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest 1's un-awaited orphaned run leaks timing into Test 2 via the shared execution queue.
execute()serializes all contexts through a module-levelglobalExecQueue, and for a timed-out host call,releaseTurn()is deferred until the orphaned run settles (seeisolate-context.ts:this.orphanedRun = run.catch(...).then(() => { this.disposeVm(); releaseTurn() })). Test 1 (hostDurationMs: 800,timeout: 100) never waits for that settlement or disposes its context, so its queue turn stays open for ~800ms after the test function itself returns.Since Test 2 immediately calls
context.execute(...)on a different context, that call blocks onwaitForPrev = globalExecQueue(still Test 1's unresolved turn) before its own CPU-bound timeout logic even starts. Test 2'selapsedMsmeasurement then largely reflects queue-wait time from Test 1, not the CPU-loop interrupt latency it's meant to verify — it currently passes only because of the generous 2000ms bound, but the coupling is fragile (reordering tests or CI slowness could break it confusingly).Have Test 1 wait for the host call to settle (or
await context.dispose()) before finishing, mirroring the cleanup pattern already used in Tests 3 and 4.🔧 Suggested fix
it('enforces the wall-clock timeout while a host call is suspended', async () => { const hostDurationMs = 800 const slow = makeBinding('slow', async () => { await sleep(hostDurationMs) return { done: true } }) const driver = createQuickJSIsolateDriver({ timeout: 100 }) const context = await driver.createContext({ bindings: { slow } }) const startedAt = Date.now() const result = await context.execute('return await slow({})') const elapsedMs = Date.now() - startedAt expect(result.success).toBe(false) expect(result.error?.name).toBe('TimeoutError') // Returns at the deadline, not after the full host call completes. expect(elapsedMs).toBeLessThan(hostDurationMs / 2) + + // Let the orphaned host promise settle so this test's turn on the + // shared execution queue is released before the next test runs. + await sleep(hostDurationMs) })🤖 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 155 - 201, Test 1 leaves a timed-out host-call run unresolved, which keeps the module-level globalExecQueue occupied and can skew Test 2 timing. In the execute timeout tests, make the first case wait for the suspended host call to fully settle or explicitly dispose the created context before the test ends, using the existing context.execute and context.dispose flow. This should mirror the cleanup approach used later in the same describe block so the CPU-bound timeout assertion measures only the interrupt path and not queue wait time.
🤖 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 25-35: Update the error normalization logic in error-normalizer.ts
so the QuickJS interruption case only maps to TIMEOUT_ERROR when the exception
is actually an InternalError with an interrupted message. In the normalization
branch that checks lower.includes('interrupted'), add a guard on error.name ===
'InternalError' before returning the timeout shape. Keep the existing
TimeoutError mapping and stack preservation, but avoid classifying unrelated
host errors that merely mention interrupted.
In `@packages/ai-isolate-quickjs/src/isolate-context.ts`:
- Around line 188-193: The orphan cleanup chain in isolate-context should always
release the global queue even if VM disposal fails. Update the
`this.orphanedRun` flow so `releaseTurn()` is guaranteed to run independently of
`disposeVm()` in the orphan handling path, and make sure the `disposeVm`/cleanup
logic in `isolate-context` does not leave the continuation rejected without a
handler.
---
Nitpick comments:
In `@packages/ai-isolate-quickjs/tests/isolate-driver.test.ts`:
- Around line 155-201: Test 1 leaves a timed-out host-call run unresolved, which
keeps the module-level globalExecQueue occupied and can skew Test 2 timing. In
the execute timeout tests, make the first case wait for the suspended host call
to fully settle or explicitly dispose the created context before the test ends,
using the existing context.execute and context.dispose flow. This should mirror
the cleanup approach used later in the same describe block so the CPU-bound
timeout assertion measures only the interrupt path and not queue wait time.
🪄 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: cbbce21f-262e-4be3-a0d4-a03b680c3980
📒 Files selected for processing (4)
.changeset/quickjs-timeout-host-calls.mdpackages/ai-isolate-quickjs/src/error-normalizer.tspackages/ai-isolate-quickjs/src/isolate-context.tspackages/ai-isolate-quickjs/tests/isolate-driver.test.ts
| // QuickJS raises `InternalError: interrupted` when the interrupt handler | ||
| // trips the execution deadline. Surface it as a timeout so a CPU-bound | ||
| // deadline and a wall-clock (host-call) deadline report the same error. | ||
| if (lower.includes('interrupted')) { | ||
| return { | ||
| name: TIMEOUT_ERROR, | ||
| message: 'Code execution exceeded timeout', | ||
| stack: error.stack, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the normalizer and nearby isolate error handling.
printf '\n== error-normalizer.ts ==\n'
cat -n packages/ai-isolate-quickjs/src/error-normalizer.ts
printf '\n== search for normalizeError / TIMEOUT_ERROR / interrupted ==\n'
rg -n "normalizeError|TIMEOUT_ERROR|interrupted|stack overflow|out of memory|memory alloc" packages/ai-isolate-quickjs packages -S
printf '\n== inspect isolate-context usage ==\n'
cat -n packages/ai-isolate-quickjs/src/isolate-context.tsRepository: TanStack/ai
Length of output: 22246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the package files to inspect any tests or docs that describe the expected error shape.
fd -a -t f . packages/ai-isolate-quickjs | sed 's#^\./##' | sort
printf '\n== tests mentioning QuickJS errors ==\n'
rg -n "TimeoutError|InternalError: interrupted|interrupted|stack overflow|out of memory|memory alloc" packages/ai-isolate-quickjs -SRepository: TanStack/ai
Length of output: 2258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact implementation and any tests around timeout normalization.
cat -n packages/ai-isolate-quickjs/src/error-normalizer.ts
printf '\n---\n'
rg -n "TimeoutError|interrupted|InternalError: interrupted|error-normalizer" packages/ai-isolate-quickjs -SRepository: TanStack/ai
Length of output: 4272
Gate the interrupted match on error.name === 'InternalError'.
As written, any host error whose message contains interrupted is normalized to TimeoutError, which can mask the real failure and drop the original error name/message. Matching the QuickJS InternalError: interrupted shape keeps unrelated interruptions from being misclassified.
🤖 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 25 - 35,
Update the error normalization logic in error-normalizer.ts so the QuickJS
interruption case only maps to TIMEOUT_ERROR when the exception is actually an
InternalError with an interrupted message. In the normalization branch that
checks lower.includes('interrupted'), add a guard on error.name ===
'InternalError' before returning the timeout shape. Keep the existing
TimeoutError mapping and stack preservation, but avoid classifying unrelated
host errors that merely mention interrupted.
| this.orphanedRun = run | ||
| .catch(() => undefined) | ||
| .then(() => { | ||
| this.disposeVm() | ||
| releaseTurn() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release the global queue even if orphan cleanup fails.
releaseTurn() currently runs after disposeVm(). If teardown throws, the global exec queue stays blocked and the orphan continuation can reject without a handler.
Proposed fix
this.orphanedRun = run
.catch(() => undefined)
.then(() => {
- this.disposeVm()
- releaseTurn()
+ try {
+ this.disposeVm()
+ } finally {
+ releaseTurn()
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.orphanedRun = run | |
| .catch(() => undefined) | |
| .then(() => { | |
| this.disposeVm() | |
| releaseTurn() | |
| }) | |
| this.orphanedRun = run | |
| .catch(() => undefined) | |
| .then(() => { | |
| try { | |
| this.disposeVm() | |
| } finally { | |
| releaseTurn() | |
| } | |
| }) |
🤖 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 188 - 193,
The orphan cleanup chain in isolate-context should always release the global
queue even if VM disposal fails. Update the `this.orphanedRun` flow so
`releaseTurn()` is guaranteed to run independently of `disposeVm()` in the
orphan handling path, and make sure the `disposeVm`/cleanup logic in
`isolate-context` does not leave the continuation rejected without a handler.
1 participant
timeoutis currently only enforced while the VM is stepping bytecode: the deadline lives insetInterruptHandler, which the interpreter polls, so it never fires while the isolate is asyncify-suspended awaiting a host binding. Withtimeout: 100and a binding that takes 800ms,execute()resolves successfully after ~800ms — and a binding that never resolves hangsexecute()forever. CPU-bound code was fine; only the host-call path was affected.This makes the timeout a wall-clock deadline for the whole execution by racing the run against a timer. When the timer fires first, a short grace window lets a near-simultaneous in-VM interrupt settle normally (so CPU-bound timeouts keep the context reusable); otherwise
execute()returns aTimeoutErrorimmediately. A pending host promise can't be cancelled and the context can't be freed mid-asyncify, so the orphaned run keeps the interrupt handler armed (the VM aborts as soon as the host promise settles) and disposal plus the exec-queue turn happen in a continuation on it — no context leak, and the single-suspension invariant still holds.InternalError: interruptedis also normalized toTimeoutErrorso both deadline paths report the same error name. No public API changes.Tests: a host call slower than the timeout now errors at ~the deadline instead of the host duration; CPU-bound loops still time out with a reusable context; a host promise settling after the timeout doesn't crash, resume, or leave an unhandled rejection;
dispose()after a timeout waits for the orphaned run to unwind.pnpm test:lib/test:types/test:eslint/ publint all pass. Includes a patch changeset.Summary by CodeRabbit
Bug Fixes
Tests