Uh oh!
There was an error while loading. Please reload this page.
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 |
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/bashset -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/bashset -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/bashset -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.
There was a problem hiding this comment.
Valid on this branch. Current main already gates this path on error.name === 'InternalError' (and the dumped-object path does the same). That landed in #948. This PR should not be merged on top of that rewrite.
There was a problem hiding this comment.
@AlemTuzlak, understood. The finding is valid for this branch. Current main already contains the required guards from #948. This pull request should not merge on top of that rewrite.
You are interacting with an AI system.
Uh oh!
There was an error while loading. Please reload this page.
AlemTuzlak
left a comment
There was a problem hiding this comment.
Review
This PR described a real bug. I would not merge it onto current main.
The claimed bug
On the old asyncify isolate, timeout lived only in setInterruptHandler. That handler runs while QuickJS steps bytecode. It does not run while the VM waits on a host binding. So:
timeout: 100plus an 800ms host call finished after ~800ms- a host call that never settled hung
execute()forever
That diagnosis is correct for the July 6 code.
Why this PR must not land now
#948 merged on 2026-08-07 and already fixes the same host-call hang. It also removes the asyncify design this PR still uses.
Current main does this instead:
- tool bindings return QuickJS promises (no WASM stack suspend)
awaitWithDeadline()races the guest promise against a host timer- a timeout cancels pending tool calls and disposes the context
- later
execute()calls returnDisposedError
I confirmed the current tests on this tree:
- CPU-bound timeout returns
TimeoutError - a host binding that never settles returns
TimeoutErrorin about the deadline (100ms timeout, test finished in 124ms) - late host settlement does not crash the next context
dispose()during an in-flight timeout is safenormalizeErroralready requireserror.name === 'InternalError'before it mapsinterruptedtoTimeoutError
git merge-tree of this branch onto current main conflicts in isolate-context.ts and error-normalizer.ts. A merge would try to put the old asyncify orphan path back on top of the promise-bridge rewrite. That would undo #948.
CodeRabbit notes
Both open notes were valid against this PR's July 6 code:
- Gate
interruptedonInternalError— already onmain. - Always call
releaseTurn()ifdisposeVm()throws — that queue exists only for asyncify.mainuses a per-context queue and a different cleanup path.
The test-queue leak note is also specific to the old global asyncify queue.
Semantic clash
This PR keeps the context reusable after a CPU-bound timeout. #948 made every timeout terminal for that context. Code Mode already creates a context per run, so that later choice is the one main documents.
Recommendation: close this PR as superseded by #948. Do not rebase or merge main into it.
jan-kubica
commented
Aug 19, 2026
closing as superseded |
AlemTuzlak
commented
Aug 19, 2026
@jan-kubica thank you though! |
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