Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 302
fix(isolate-quickjs): enforce timeout while suspended in host calls#905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
jan-kubica
wants to merge
1
commit into
TanStack:main
from
jan-kubica:fix/isolate-quickjs-timeout-host-calls
Uh oh!
There was an error while loading. Please reload this page.
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| --- | ||
| '@tanstack/ai-isolate-quickjs': patch | ||
| --- | ||
| Enforce `timeout` as a wall-clock deadline that covers host-call suspensions. | ||
| Previously the only deadline mechanism was QuickJS's `setInterruptHandler`, which | ||
| fires only while the sandbox is stepping bytecode. While the isolate is | ||
| asyncify-suspended awaiting a host binding's Promise, no interrupt fires, so the | ||
| configured `timeout` was silently ignored: a run with `timeout: 100` and an | ||
| 800 ms host binding resolved successfully after ~800 ms, and a hung host binding | ||
| could hang `execute()` indefinitely. | ||
| `execute()` now races the run against a wall-clock timer, so `timeout` bounds the | ||
| entire execution including time spent suspended in a host call. On the deadline | ||
| it returns a normalized `TimeoutError` immediately. The suspended host Promise | ||
| cannot be cancelled and the VM cannot be freed mid-asyncify, so disposal and the | ||
| internal execution-queue turn are deferred until the orphaned run unwinds (the | ||
| still-armed interrupt handler aborts it the moment the host Promise settles), | ||
| which prevents a late host result from resuming a dead execution and avoids | ||
| leaking the context. CPU-bound loops are still interrupted as before and leave | ||
| the context reusable. The public API is unchanged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -10,6 +10,21 @@ import type { ExecutionResult, IsolateContext } from '@tanstack/ai-code-mode' | ||
| */ | ||
| let globalExecQueue: Promise<void> = Promise.resolve() | ||
| /** Sentinel resolved by the wall-clock deadline race. */ | ||
| const TIMED_OUT = Symbol('quickjs-isolate-timeout') | ||
| /** | ||
| * Grace window granted, after the wall-clock timer fires, for the in-VM | ||
| * interrupt handler to settle a CPU-bound run before it is treated as a host | ||
| * call still suspended in asyncify. A CPU-bound loop trips the interrupt at | ||
| * (almost) the same instant the timer fires; the grace keeps that path on the | ||
| * normal, reusable-context branch instead of orphaning the VM. | ||
| */ | ||
| const TIMEOUT_INTERRUPT_GRACE_MS = 10 | ||
| const delay = (ms: number): Promise<void> => | ||
| new Promise((resolve) => setTimeout(resolve, ms)) | ||
| /** | ||
| * IsolateContext implementation using QuickJS WASM | ||
| */ | ||
| @@ -19,6 +34,13 @@ export class QuickJSIsolateContext implements IsolateContext { | ||
| private readonly timeout: number | ||
| private disposed = false | ||
| private executing = false | ||
| private timedOut = false | ||
| /** | ||
| * A run that timed out while a host call was suspended in asyncify. The VM | ||
| * cannot be disposed until this settles, so disposal and the global-queue | ||
| * turn are handed off to a continuation on it. `null` when no run is orphaned. | ||
| */ | ||
| private orphanedRun: Promise<void> | null = null | ||
| constructor(vm: QuickJSAsyncContext, logs: Array<string>, timeout: number) { | ||
| this.vm = vm | ||
| @@ -28,21 +50,14 @@ export class QuickJSIsolateContext implements IsolateContext { | ||
| async execute<T = unknown>(code: string): Promise<ExecutionResult<T>> { | ||
| if (this.disposed) { | ||
| return { | ||
| success: false, | ||
| error: { | ||
| name: 'DisposedError', | ||
| message: 'Context has been disposed', | ||
| }, | ||
| logs: [], | ||
| } | ||
| return this.disposedResult<T>() | ||
| } | ||
| // Serialize through the global queue to prevent concurrent | ||
| // WASM asyncify suspensions across contexts. | ||
| let resolve!: () => void | ||
| let releaseTurn!: () => void | ||
| const myTurn = new Promise<void>((r) => { | ||
| resolve = r | ||
| releaseTurn = r | ||
| }) | ||
| const waitForPrev = globalExecQueue | ||
| globalExecQueue = myTurn | ||
| @@ -51,114 +66,206 @@ export class QuickJSIsolateContext implements IsolateContext { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- dispose() may be called concurrently while awaiting the queue | ||
| if (this.disposed) { | ||
| resolve() | ||
| return { | ||
| success: false, | ||
| error: { | ||
| name: 'DisposedError', | ||
| message: 'Context has been disposed', | ||
| }, | ||
| logs: [], | ||
| } | ||
| releaseTurn() | ||
| return this.disposedResult<T>() | ||
| } | ||
| this.executing = true | ||
| this.logs.length = 0 | ||
| const releaseVmAfterFatalLimit = () => { | ||
| if (this.disposed) return | ||
| try { | ||
| this.vm.runtime.setInterruptHandler(() => false) | ||
| } catch { | ||
| // ignore if runtime is already torn down | ||
| } | ||
| this.disposed = true | ||
| this.vm.dispose() | ||
| } | ||
| // Set when a wall-clock timeout leaves a host call suspended in asyncify: | ||
| // interrupt-handler reset, VM disposal, and the queue-turn release are then | ||
| // owned by the orphaned-run continuation rather than the finally block. | ||
| let deferredToOrphan = false | ||
| const fail = (error: unknown) => { | ||
| const fail = (error: unknown): ExecutionResult<T> => { | ||
| const normalized = normalizeError(error) | ||
| if (isFatalQuickJSLimitError(normalized)) { | ||
| releaseVmAfterFatalLimit() | ||
| this.disposeVm() | ||
| } | ||
| return { | ||
| success: false as const, | ||
| success: false, | ||
| error: normalized, | ||
| logs: [...this.logs], | ||
| } | ||
| } | ||
| try { | ||
| const wrappedCode = wrapCode(code) | ||
| const deadline = Date.now() + this.timeout | ||
| this.vm.runtime.setInterruptHandler(() => { | ||
| return Date.now() > deadline | ||
| }) | ||
| // Drives the wrapped code to completion and marshals its result. On a | ||
| // deadline interrupt `evalCodeAsync` resolves with an error result, which | ||
| // `unwrapResult` throws and `fail` normalizes. | ||
| const runToSettled = async (): Promise<ExecutionResult<T>> => { | ||
| const result = await this.vm.evalCodeAsync(wrapCode(code)) | ||
| try { | ||
| const result = await this.vm.evalCodeAsync(wrappedCode) | ||
| const promiseHandle = this.vm.unwrapResult(result) | ||
| // evalCodeAsync returns a Promise handle (our wrapper is an async IIFE). | ||
| // Use resolvePromise + executePendingJobs to properly await the | ||
| // QuickJS promise without re-entering the WASM asyncify state. | ||
| const nativePromise = this.vm.resolvePromise(promiseHandle) | ||
| promiseHandle.dispose() | ||
| this.vm.runtime.executePendingJobs() | ||
| const resolvedResult = await nativePromise | ||
| const valueHandle = this.vm.unwrapResult(resolvedResult) | ||
| const dumpedResult = this.vm.dump(valueHandle) | ||
| valueHandle.dispose() | ||
| let parsedResult: T | ||
| try { | ||
| const promiseHandle = this.vm.unwrapResult(result) | ||
| // evalCodeAsync returns a Promise handle (our wrapper is an async IIFE). | ||
| // Use resolvePromise + executePendingJobs to properly await the | ||
| // QuickJS promise without re-entering the WASM asyncify state. | ||
| const nativePromise = this.vm.resolvePromise(promiseHandle) | ||
| promiseHandle.dispose() | ||
| this.vm.runtime.executePendingJobs() | ||
| const resolvedResult = await nativePromise | ||
| const valueHandle = this.vm.unwrapResult(resolvedResult) | ||
| const dumpedResult = this.vm.dump(valueHandle) | ||
| valueHandle.dispose() | ||
| if (typeof dumpedResult === 'string') { | ||
| try { | ||
| parsedResult = JSON.parse(dumpedResult) as T | ||
| } catch { | ||
| parsedResult = dumpedResult as T | ||
| } | ||
| } else { | ||
| if (typeof dumpedResult === 'string') { | ||
| try { | ||
| parsedResult = JSON.parse(dumpedResult) as T | ||
| } catch { | ||
| parsedResult = dumpedResult as T | ||
| } | ||
| } else { | ||
| parsedResult = dumpedResult as T | ||
| } | ||
| return { | ||
| success: true, | ||
| value: parsedResult, | ||
| logs: [...this.logs], | ||
| } | ||
| } catch (unwrapError) { | ||
| return fail(unwrapError) | ||
| // The wall-clock deadline may have fired while this run was suspended in | ||
| // a host call; the caller already received the timeout result, so a late | ||
| // success must not be surfaced as if the execution had finished in time. | ||
| if (this.timedOut) { | ||
| return this.timeoutResult<T>() | ||
| } | ||
| } finally { | ||
| // fail() may set disposed when releasing the VM after memory/stack limit errors | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- disposed set in fail() | ||
| if (!this.disposed) { | ||
| this.vm.runtime.setInterruptHandler(() => false) | ||
| return { | ||
| success: true, | ||
| value: parsedResult, | ||
| logs: [...this.logs], | ||
| } | ||
| } catch (unwrapError) { | ||
| return fail(unwrapError) | ||
| } | ||
| } | ||
| try { | ||
| // The interrupt handler only fires while the VM is stepping bytecode, so | ||
| // it cannot preempt a host binding whose Promise is awaited via asyncify. | ||
| // Keep it for CPU-bound loops, but also race the run against a wall-clock | ||
| // timer so `timeout` bounds the whole execution, host suspensions included. | ||
| const deadline = Date.now() + this.timeout | ||
| this.vm.runtime.setInterruptHandler(() => Date.now() > deadline) | ||
| const run = runToSettled() | ||
| let timeoutTimer: ReturnType<typeof setTimeout> | undefined | ||
| const wallClock = new Promise<typeof TIMED_OUT>((resolve) => { | ||
| timeoutTimer = setTimeout(() => resolve(TIMED_OUT), this.timeout) | ||
| }) | ||
| const outcome = await Promise.race([run, wallClock]) | ||
| if (timeoutTimer !== undefined) { | ||
| clearTimeout(timeoutTimer) | ||
| } | ||
| if (outcome !== TIMED_OUT) { | ||
| return outcome | ||
| } | ||
| // The interrupt fires at (almost) the same instant for a CPU-bound loop; | ||
| // give the in-VM interrupt a brief window to settle the run before | ||
| // treating this as a host call still suspended in asyncify. | ||
| const graced = await Promise.race([ | ||
| run, | ||
| delay(TIMEOUT_INTERRUPT_GRACE_MS).then( | ||
| (): typeof TIMED_OUT => TIMED_OUT, | ||
| ), | ||
| ]) | ||
| if (graced !== TIMED_OUT) { | ||
| return graced | ||
| } | ||
| // Still suspended in a host call. The VM cannot be disposed now (freeing | ||
| // it mid-asyncify corrupts the stack the suspended call resumes into) and | ||
| // the host Promise cannot be cancelled. Leave the interrupt handler armed | ||
| // so the VM aborts the instant the host Promise resolves and it resumes | ||
| // stepping, then dispose the VM and release the queue turn once the | ||
| // orphaned run unwinds. This keeps the single-asyncify-suspension | ||
| // invariant that the global queue exists to protect. | ||
| this.timedOut = true | ||
| deferredToOrphan = true | ||
| this.orphanedRun = run | ||
| .catch(() => undefined) | ||
| .then(() => { | ||
| this.disposeVm() | ||
| releaseTurn() | ||
| }) | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return this.timeoutResult<T>() | ||
| } catch (error) { | ||
| return fail(error) | ||
| } finally { | ||
| // On the orphan path the continuation owns interrupt-handler reset, VM | ||
| // disposal, and the queue-turn release; the still-armed handler is what | ||
| // lets the suspended run abort when it resumes. | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- disposed may be set by fail() after a fatal limit error | ||
| if (!deferredToOrphan && !this.disposed) { | ||
| this.vm.runtime.setInterruptHandler(() => false) | ||
| } | ||
| this.executing = false | ||
| resolve() | ||
| if (!deferredToOrphan) { | ||
| releaseTurn() | ||
| } | ||
| } | ||
| } | ||
| async dispose(): Promise<void> { | ||
| if (this.disposed) return | ||
| // A timed-out execution may have left a host call suspended in asyncify. | ||
| // Its continuation disposes the VM once the call unwinds; wait for that | ||
| // rather than freeing the VM out from under the suspended call. | ||
| if (this.orphanedRun) { | ||
| await this.orphanedRun | ||
| return | ||
| } | ||
| // If an execution is in flight, wait for the global queue to drain | ||
| // before disposing the VM. Otherwise the asyncified callback would | ||
| // try to access a freed context. | ||
| if (this.executing) { | ||
| await globalExecQueue | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- the orphaned continuation may have disposed while we awaited the queue | ||
| if (this.disposed) return | ||
| this.disposed = true | ||
| this.vm.dispose() | ||
| } | ||
| /** Reset the interrupt handler and tear the VM down exactly once. */ | ||
| private disposeVm(): void { | ||
| if (this.disposed) return | ||
| try { | ||
| this.vm.runtime.setInterruptHandler(() => false) | ||
| } catch { | ||
| // ignore if the runtime is already torn down | ||
| } | ||
| this.disposed = true | ||
| this.vm.dispose() | ||
| } | ||
| private disposedResult<T>(): ExecutionResult<T> { | ||
| return { | ||
| success: false, | ||
| error: { | ||
| name: 'DisposedError', | ||
| message: 'Context has been disposed', | ||
| }, | ||
| logs: [], | ||
| } | ||
| } | ||
| private timeoutResult<T>(): ExecutionResult<T> { | ||
| return { | ||
| success: false, | ||
| error: { | ||
| name: 'TimeoutError', | ||
| message: `Code execution exceeded timeout of ${this.timeout}ms`, | ||
| }, | ||
| logs: [...this.logs], | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: TanStack/ai
Length of output: 22246
🏁 Script executed:
Repository: TanStack/ai
Length of output: 2258
🏁 Script executed:
Repository: TanStack/ai
Length of output: 4272
Gate the
interruptedmatch onerror.name === 'InternalError'.As written, any host error whose message contains
interruptedis normalized toTimeoutError, which can mask the real failure and drop the original error name/message. Matching the QuickJSInternalError: interruptedshape keeps unrelated interruptions from being misclassified.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlemTuzlak, understood. The finding is valid for this branch. Currentmainalready contains the required guards from#948. This pull request should not merge on top of that rewrite.You are interacting with an AI system.