diff --git a/.changeset/fix-sandbox-deferred-promise-pump.md b/.changeset/fix-sandbox-deferred-promise-pump.md new file mode 100644 index 0000000000..aaff021baf --- /dev/null +++ b/.changeset/fix-sandbox-deferred-promise-pump.md @@ -0,0 +1,9 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): drive sandbox host calls with deferred promises and a deadline-bounded pump + +The QuickJS sandbox exposed `ctx.api.object(x).find/update/...` via `newAsyncifiedFunction`, which unwinds the WASM stack per host call and forbids a second call while the first is unwound. A script awaiting two host calls in sequence (e.g. an action doing `findOne()` then `update()`) drove the second call from a continuation resumed inside `executePendingJobs`, corrupting the wasm heap (`memory access out of bounds` / `p->ref_count == 0`) or exhausting the fixed 1000-iteration pump budget — surfacing as `action '…' did not resolve after 1000 pump iterations`. + +Host API methods are now exposed as deferred QuickJS promises (`vm.newPromise()`), so sequential `await`s compose with no stack unwinding, and the pump loop is bounded by the configured `timeoutMs` instead of a fixed iteration cap. Host **method** calls now require `await` (the `.object(name)` proxy getter stays synchronous); a stuck/never-settling host call is cut off with a clear timeout error. diff --git a/packages/runtime/src/sandbox/body-runner.test.ts b/packages/runtime/src/sandbox/body-runner.test.ts index ee00ff9915..38b8b812f0 100644 --- a/packages/runtime/src/sandbox/body-runner.test.ts +++ b/packages/runtime/src/sandbox/body-runner.test.ts @@ -81,7 +81,7 @@ describe('hookBodyRunnerFactory', () => { events: ['afterInsert'], body: { language: 'js', - source: 'return { opportunity_count: ctx.api.object("opportunity").count() };', + source: 'return { opportunity_count: await ctx.api.object("opportunity").count() };', capabilities: ['api.read'], }, } as any); diff --git a/packages/runtime/src/sandbox/quickjs-runner.test.ts b/packages/runtime/src/sandbox/quickjs-runner.test.ts index ce1b1d4c28..71897c4156 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.test.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.test.ts @@ -212,3 +212,103 @@ describe('QuickJSScriptRunner — async host APIs', () => { expect(r.mutatedInput).toMatchObject({ raw: 'abc-9', normalized: 'ABC-9' }); }); }); + +describe('QuickJSScriptRunner — long-running async host work (pump budget)', () => { + // Regression: an action's single `ctx.api.update(...)` can synchronously drive + // a large amount of awaited host work — e.g. a record-change flow that the + // engine runs inline inside the afterUpdate hook chain (see tianshun-mtc + // `lead_apply_convert` → `lead_convert_approval`). From the sandbox's view + // that is ONE asyncified host call that takes many event-loop turns to settle. + // + // The pump loop must bound that wait by the configured `timeoutMs`, NOT by a + // fixed iteration count: a legitimately-progressing call that needs >1000 + // event-loop turns but finishes well within the timeout must still resolve. + // The old fixed `pumps < 1000` cap fired in ~tens of ms and surfaced as + // "did not resolve after 1000 pump iterations" — the exact production error. + + it('resolves an action whose host call settles after >1000 event-loop turns', async () => { + const TURNS = 1500; // comfortably exceeds the old 1000-pump cap + let observed = 0; + const api = { + object: () => ({ + // One asyncified host call that internally needs many macrotask turns + // before its promise settles — mirrors a CRUD write that synchronously + // runs a long downstream automation. + update: async () => { + for (let i = 0; i < TURNS; i++) { + await new Promise((resolve) => setImmediate(resolve)); + } + observed = TURNS; + return { ok: true }; + }, + }), + }; + + const r = await runner.runScript( + { + language: 'js', + source: "return await ctx.api.object('wid').update({ id: 'x', status: 'pending' });", + capabilities: ['api.write'], + timeoutMs: 30000, + }, + ctx({ api }), + actionOpts, + ); + + expect(r.value).toEqual({ ok: true }); + expect(observed).toBe(TURNS); + }, 40000); + + it('resolves an action that makes >1000 sequential host calls', async () => { + const N = 1500; + let calls = 0; + const api = { + object: () => ({ + update: async () => { + calls++; + return { i: calls }; + }, + }), + }; + + const r = await runner.runScript( + { + language: 'js', + source: ` + const o = ctx.api.object('wid'); + for (let i = 0; i < ${N}; i++) { await o.update({ id: 'x', i }); } + return { calls: ${N} }; + `, + capabilities: ['api.write'], + timeoutMs: 30000, + }, + ctx({ api }), + actionOpts, + ); + + expect(r.value).toEqual({ calls: N }); + expect(calls).toBe(N); + }, 40000); + + it('still enforces the timeout on a host call that never settles', async () => { + const api = { + object: () => ({ + // Never resolves — must be killed by the deadline, not hang forever. + update: () => new Promise(() => {}), + }), + }; + + await expect( + runner.runScript( + { + language: 'js', + source: "return await ctx.api.object('wid').update({ id: 'x' });", + capabilities: ['api.write'], + timeoutMs: 300, + }, + ctx({ api }), + actionOpts, + ), + ).rejects.toThrow(/timeout/i); + }, 10000); +}); diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index 6e3e6d1d93..a5d64a9ee4 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -156,7 +156,7 @@ export class QuickJSScriptRunner implements ScriptRunner { // L2 scripts: wrap as async IIFE and use side-channel + asyncified pump. // Each pump iteration: - // 1. yield to the host event loop (lets asyncified host calls resolve) + // 1. yield to the host event loop (lets host promises settle) // 2. drain QuickJS pending jobs (advances the .then chain) // 3. read __result/__error from the VM const wrapped = args.origin.kind === 'hook' @@ -179,9 +179,20 @@ export class QuickJSScriptRunner implements ScriptRunner { } evalRes.value.dispose(); + // Drive the script's async continuations to completion. Each iteration + // yields to the host event loop (so in-flight host promises settle and + // resolve their VM-side deferred handles) and then drains the QuickJS job + // queue. The ONLY bound on how long we wait is the deadline: a slow but + // progressing script — many sequential host writes, or one write that + // synchronously drives a downstream record-change automation — must be + // allowed to finish within its timeout, and a stuck / never-settling host + // call is cut off here (the QuickJS interrupt handler can't fire while we + // are parked on a host promise, so this deadline check is the backstop). + // The previous fixed `pumps < 1000` cap fired in ~tens of ms on legitimate + // work and surfaced as "did not resolve after 1000 pump iterations". let pumps = 0; - while (pumps < 1000) { - // Yield to host event loop so any in-flight asyncified host promises resolve. + for (;;) { + // Yield to host event loop so any in-flight host promises resolve. await new Promise((resolve) => setImmediate(resolve)); const pending = runtime.executePendingJobs(); @@ -210,14 +221,11 @@ export class QuickJSScriptRunner implements ScriptRunner { if (Date.now() > deadline) { throw new SandboxError( - `${args.origin.kind} '${args.origin.name}' exceeded timeout of ${args.timeoutMs}ms`, + `${args.origin.kind} '${args.origin.name}' exceeded timeout of ${args.timeoutMs}ms (after ${pumps} pump iterations)`, ); } pumps++; } - throw new SandboxError( - `${args.origin.kind} '${args.origin.name}' did not resolve after ${pumps} pump iterations`, - ); } finally { // newAsyncContext() owns its WASM module; disposing the context disposes // the runtime + module together. @@ -230,8 +238,9 @@ export class QuickJSScriptRunner implements ScriptRunner { * the body declared it; missing methods throw at call-time inside the VM * with a clear diagnostic. * - * Host API methods are installed via {@link QuickJSAsyncContext.newAsyncifiedFunction} - * so they may return Promises (real ObjectQL `find/count/insert/...` are async). + * Host API methods are installed as deferred-promise functions (see + * {@link installApiMethod}) so they may return Promises (real ObjectQL + * `find/count/insert/...` are async) without asyncify's single-unwind limit. */ private installCtx( vm: QuickJSAsyncContext, @@ -320,11 +329,25 @@ export class QuickJSScriptRunner implements ScriptRunner { } /** - * Asyncified host-bound API method. + * Host-bound API method, exposed to the VM as an async function. + * + * IMPORTANT: this deliberately does NOT use `newAsyncifiedFunction`. Asyncify + * unwinds the WASM stack while a host call is in flight, and the engine forbids + * one asyncified call from running while another is unwound ("the stack cannot + * be unwound twice"). A script that awaits two host calls in sequence — e.g. the + * real `lead_apply_convert` action doing `findOne()` then `update()` — trips + * exactly that: the second call is driven from a resumed continuation inside + * `executePendingJobs` (a non-async frame), which corrupted the wasm heap + * (`memory access out of bounds` / `p->ref_count == 0`) and, when it limped + * along, blew the pump budget ("did not resolve after 1000 pump iterations"). * - * Awaits Promise return values from the host implementation and marshals the - * resolved value back into the VM. Capability check happens at call time and - * surfaces inside the VM as a thrown error with a clear diagnostic. + * Instead we hand the VM a real QuickJS promise (a deferred) and settle it from + * the host event loop. Sequential `await`s are then ordinary promises with no + * stack unwinding, so any number of host calls compose safely; the pump loop in + * {@link QuickJSScriptRunner.execute} drains the resulting jobs. + * + * The capability check runs synchronously at call time and surfaces inside the + * VM as a thrown error with a clear diagnostic. */ function installApiMethod( vm: QuickJSAsyncContext, @@ -336,7 +359,9 @@ function installApiMethod( required: HookBodyCapability, origin: ScriptOrigin, ): void { - const fn = vm.newAsyncifiedFunction(method, async (...argHandles) => { + const fn = vm.newFunction(method, (...argHandles) => { + // Capability gate — throw synchronously so the VM sees a normal exception at + // the call site (mirrors ctx.log / ctx.crypto gating). if (!caps.has(required)) { throw new SandboxError( `capability '${required}' not granted to ${origin.kind} '${origin.name}' (called ctx.api.object('${objectName}').${method})`, @@ -346,14 +371,37 @@ function installApiMethod( if (!apiAny || typeof apiAny.object !== 'function') { throw new SandboxError(`ctx.api unavailable in ${origin.kind} '${origin.name}'`); } + // Dump args now, while the handles are alive — they are freed when this + // function returns, long before the async work below runs. const args = argHandles.map((h) => vm.dump(h)); - const proxy = (apiAny.object as (n: string) => Record)(objectName); - const m = proxy[method] as ((...a: unknown[]) => unknown) | undefined; - if (typeof m !== 'function') { - throw new SandboxError(`ctx.api.object('${objectName}').${method} not implemented`); - } - const ret = await Promise.resolve(m.apply(proxy, args)); - return jsonToHandle(vm, ret); + + const deferred = vm.newPromise(); + void (async () => { + try { + const proxy = (apiAny.object as (n: string) => Record)(objectName); + const m = proxy[method] as ((...a: unknown[]) => unknown) | undefined; + if (typeof m !== 'function') { + throw new SandboxError(`ctx.api.object('${objectName}').${method} not implemented`); + } + const ret = await Promise.resolve(m.apply(proxy, args)); + if (!vm.alive) return; // VM disposed (e.g. timed out) before we settled. + const h = jsonToHandle(vm, ret); + deferred.resolve(h); + h.dispose(); + } catch (err) { + if (!vm.alive) return; + const errH = + err instanceof Error + ? vm.newError({ name: err.name || 'Error', message: err.message }) + : vm.newError({ name: 'Error', message: String(err) }); + deferred.reject(errH); + errH.dispose(); + } + })(); + // The pump loop is the sole driver of executePendingJobs, so the resolution + // propagates into the VM on a subsequent pump iteration — no nudge here, to + // avoid any re-entrant executePendingJobs. + return deferred.handle; }); vm.setProp(parent, method, fn); fn.dispose();