From 6efbde8f7902dca45c5101d053bf585e8f086fa0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Jul 2026 17:17:32 +0000 Subject: [PATCH] =?UTF-8?q?perf(runtime):=20drop=20asyncify=20=E2=80=94=20?= =?UTF-8?q?sandbox=20on=20the=20sync=20QuickJS=20variant=20(ADR-0102=20D2,?= =?UTF-8?q?=20#3296)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of #3275. Switch the QuickJS sandbox from the asyncify build (newAsyncContext) to the already-installed sync release variant (newQuickJSWASMModule().newContext()), keeping one physically isolated WASM module per invocation (D2/D4). Asyncify's only justification — suspending the WASM stack across a host call — disappeared with the #1867 deferred-promise + pump redesign, so nothing depended on it. Wins: smaller binary, faster compile/instantiate + per-instruction, and removal of the suspended-stack failure class. - evalCodeAsync → evalCode; QuickJSAsyncContext → QuickJSContext throughout. - Disposal: the sync QuickJSWASMModule has no dispose() — dispose the context (frees its runtime), drop `mod` so GC reclaims the WASM instance + linear memory. Isolation invariant preserved. - Latent-leak fix surfaced by the stricter sync teardown: host ctx.api calls hand the VM a vm.newPromise() deferred that was never dispose()d (the newPromise contract requires it). Asyncify tolerated the leak; the sync build's JS_FreeRuntime aborted ("Assertion failed: list_empty") when a context was torn down with a pending never-settled host call (the timeout path). Deferreds are now tracked and disposed before context teardown. - New RSS soak test guards that per-invocation modules don't ratchet RSS (GC reclaims them). - ADR-0102 D2 updated with the verified disposal model. 58 sandbox + 574 runtime tests green; RSS soak green. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_011iJLjqToxNv1aYP3syQtRp --- .changeset/sandbox-drop-asyncify.md | 26 +++++ ...2-sandbox-cpu-budget-and-engine-variant.md | 2 +- .../src/sandbox/quickjs-runner.rss.test.ts | 48 ++++++++ .../runtime/src/sandbox/quickjs-runner.ts | 108 ++++++++++++------ 4 files changed, 146 insertions(+), 38 deletions(-) create mode 100644 .changeset/sandbox-drop-asyncify.md create mode 100644 packages/runtime/src/sandbox/quickjs-runner.rss.test.ts diff --git a/.changeset/sandbox-drop-asyncify.md b/.changeset/sandbox-drop-asyncify.md new file mode 100644 index 0000000000..eb566bd444 --- /dev/null +++ b/.changeset/sandbox-drop-asyncify.md @@ -0,0 +1,26 @@ +--- +'@objectstack/runtime': patch +--- + +perf(runtime): drop asyncify — sandbox runs on the sync QuickJS variant (ADR-0102 D2, #3296) + +Phase 2 of #3275. The QuickJS sandbox switches from the asyncify build +(`newAsyncContext`) to the already-installed sync release variant +(`newQuickJSWASMModule().newContext()`), keeping one physically isolated WASM +module per invocation (ADR-0102 D2/D4). Asyncify's only justification — suspending +the WASM stack across a host call — disappeared with the #1867 deferred-promise + +pump redesign, so nothing depended on it. Wins: smaller binary, faster +compile/instantiate, faster per-instruction, and removal of an entire class of +suspended-stack failure modes. + +Also fixes a latent resource leak surfaced by the stricter sync teardown: host +`ctx.api` calls hand the VM a `vm.newPromise()` deferred that was never +`dispose()`d (the newPromise contract requires it). The asyncify build tolerated +the leak; the sync build's `JS_FreeRuntime` aborted (`Assertion failed: +list_empty`) when a context was torn down with a pending, never-settled host call +(the timeout path). Deferreds are now tracked and disposed before context +teardown. + +Memory: the sync `QuickJSWASMModule` has no `dispose()`; its WebAssembly instance ++ linear memory are GC-reclaimed when the reference is dropped. A new RSS soak +test guards that per-invocation modules don't ratchet RSS. diff --git a/docs/adr/0102-sandbox-cpu-budget-and-engine-variant.md b/docs/adr/0102-sandbox-cpu-budget-and-engine-variant.md index 96949ef785..e26e479310 100644 --- a/docs/adr/0102-sandbox-cpu-budget-and-engine-variant.md +++ b/docs/adr/0102-sandbox-cpu-budget-and-engine-variant.md @@ -48,7 +48,7 @@ Settled (Phase 1): the ceiling is both a constructor option (`wallCeilingMs`, ne Switch `newAsyncContext()` → `newQuickJSWASMModule().newContext()` (the sync release variant `@jitl/quickjs-wasmfile-release-sync` is already installed) and `evalCodeAsync` → `evalCode`. The sync surface is otherwise identical (`newPromise`, `executePendingJobs`, `setInterruptHandler`, `setMemoryLimit`, `setMaxStackSize`). - **Invariant (normative): one fresh, isolated WASM module per invocation.** `newQuickJSWASMModule()` creates a new WebAssembly instance with its own linear memory; nothing may downgrade this to runtime- or context-level sharing (see D4). -- Disposal transfers from the context to the pair: dispose context, then module — the module owns the linear memory. +- Disposal (verified against 0.32): `QuickJSWASMModule.newContext()` docs "the runtime will be disposed when the context is disposed", and — unlike the async convenience — the sync `QuickJSWASMModule` exposes **no `dispose()`**. So we `dispose()` the context (frees its runtime + QuickJS allocations), then drop the module reference; the module's WebAssembly instance + linear memory are reclaimed by **GC**. This is the library's intended sync pattern (only `newContext()` on a *shared* module shares memory), and it preserves the per-invocation-isolation invariant — but reclamation is GC-timed rather than the async path's explicit module dispose. **This is the one behavioural delta of D2 and MUST be gated by the RSS soak** (see Verification): burst hook-storm load must not ratchet RSS. - Wins: no asyncify state machine on every call (per-instruction speedup), smaller binary, faster compile/instantiate, and removal of an entire class of suspended-stack failure modes the current code only avoids by convention. ### D3 — Compile the WASM once; instantiate per invocation (deferrable) diff --git a/packages/runtime/src/sandbox/quickjs-runner.rss.test.ts b/packages/runtime/src/sandbox/quickjs-runner.rss.test.ts new file mode 100644 index 0000000000..ac987b80ea --- /dev/null +++ b/packages/runtime/src/sandbox/quickjs-runner.rss.test.ts @@ -0,0 +1,48 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * RSS soak (ADR-0102 D2). Each invocation instantiates a FRESH sync WASM module + * (`newQuickJSWASMModule()`) for per-invocation memory isolation. Unlike the old + * asyncify path, the sync `QuickJSWASMModule` has no `dispose()` — its WebAssembly + * instance + linear memory are reclaimed by GC when the reference is dropped. + * + * This guards the one behavioural delta of dropping asyncify: many back-to-back + * invocations must NOT ratchet RSS. A true leak (module memory never reclaimed) + * would add hundreds of MB to GBs across a few hundred runs; GC slack is far + * smaller than the bound. No `--expose-gc` needed — the point is that *natural* + * GC keeps it bounded. + */ + +import { describe, it, expect } from 'vitest'; +import { QuickJSScriptRunner } from './quickjs-runner.js'; +import type { ScriptContext, ScriptRunOptions } from './script-runner.js'; + +describe('QuickJSScriptRunner — RSS stays bounded across many invocations (ADR-0102 D2)', () => { + it('does not ratchet RSS over 300 hook invocations (per-invocation modules are GC-reclaimed)', async () => { + const runner = new QuickJSScriptRunner({ hookTimeoutMs: 10_000 }); + const api = { object: () => ({ count: async () => 7 }) }; + const opts: ScriptRunOptions = { origin: { kind: 'hook', name: 'rss' } }; + const ctx = (): ScriptContext => ({ input: { n: 1 }, api }); + const run = () => + runner.runScript( + { language: 'js', source: "return { c: await ctx.api.object('x').count({}) };", capabilities: ['api.read'] }, + ctx(), + opts, + ); + + // Warm up (module compile caches, V8 JIT) then take a baseline. + for (let i = 0; i < 40; i++) await run(); + (globalThis as { gc?: () => void }).gc?.(); + const baseRss = process.memoryUsage().rss; + + for (let i = 0; i < 300; i++) await run(); + (globalThis as { gc?: () => void }).gc?.(); + await new Promise((r) => setTimeout(r, 100)); + const growthMb = (process.memoryUsage().rss - baseRss) / 1024 / 1024; + + // A genuine leak (fresh ~multi-MB WASM instance per run, never reclaimed) + // would add ≳1GB over 300 runs. A generous 400MB bound distinguishes a leak + // from GC slack / heap noise without being flaky. + expect(growthMb, `RSS grew ${growthMb.toFixed(1)}MB over 300 runs`).toBeLessThan(400); + }, 60000); +}); diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index 433b7cba72..4aab2e580b 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -17,11 +17,11 @@ * `ctx.api.object('foo').count(...)` and the host method runs in node). * * Trade-offs: - * - Per-invocation overhead is dominated by VM creation: every call compiles a - * fresh WASM module via `newAsyncContext()` and disposes it on settle (see - * {@link QuickJSScriptRunner.execute}). Runtimes are deliberately NOT pooled — - * a shared module hit HostRef double-free crashes when contexts were disposed - * concurrently, and the per-invocation isolate sidesteps that (ADR-0102 D4). + * - Per-invocation overhead is dominated by VM creation: every call instantiates + * a fresh **sync** WASM module via `newQuickJSWASMModule()` (no asyncify) and + * drops it on settle (see {@link QuickJSScriptRunner.execute}). Modules are + * deliberately NOT shared — separate modules are physically memory-isolated, so + * a QuickJS heap bug can't reach another invocation's data (ADR-0102 D2/D4). * - Budgeting is CPU-time, not wall-clock (ADR-0102 D1): the per-invocation * `timeoutMs` bounds VM-active time (env defaults `OS_SANDBOX_HOOK_TIMEOUT_MS` / * `OS_SANDBOX_ACTION_TIMEOUT_MS`; `OS_SANDBOX_WALL_CEILING_MS` the wall @@ -33,8 +33,9 @@ */ import { - newAsyncContext, - type QuickJSAsyncContext, + newQuickJSWASMModule, + type QuickJSContext, + type QuickJSDeferredPromise, type QuickJSHandle, } from 'quickjs-emscripten'; import { resolveSandboxTimeoutMs } from '@objectstack/types'; @@ -164,11 +165,14 @@ export class QuickJSScriptRunner implements ScriptRunner { ctx: ScriptContext; origin: ScriptOrigin; }): Promise { - // Each invocation gets its own WebAssembly module via newAsyncContext(). - // This is the canonical "per-invocation isolate" model and avoids the - // shared-runtime HostRef double-free issues we hit with a singleton - // QuickJSAsyncWASMModule when contexts are disposed concurrently. - const vm = await newAsyncContext(); + // Each invocation gets its OWN WebAssembly module (fresh linear memory) via + // newQuickJSWASMModule() — the sync release variant, no asyncify (ADR-0102 + // D2). Separate modules are physically memory-isolated, so a QuickJS heap bug + // can't reach another invocation's marshalled data (D4). Asyncify was dropped + // because nothing suspends the WASM stack anymore: host calls are deferred + // QuickJS promises drained by the pump loop, never `newAsyncifiedFunction`. + const mod = await newQuickJSWASMModule(); + const vm = mod.newContext(); const runtime = vm.runtime; runtime.setMemoryLimit(args.memoryMb * 1024 * 1024); runtime.setMaxStackSize(512 * 1024); @@ -221,8 +225,17 @@ export class QuickJSScriptRunner implements ScriptRunner { // its commit/rollback settled). const txState: TxState = { api: null, handle: null, open: false }; + // Every host call hands the VM a `vm.newPromise()` deferred; the newPromise + // contract requires each be `dispose()`d. On the settled path the runner + // already lets them go, which the old asyncify build tolerated — but the sync + // variant's `JS_FreeRuntime` aborts (`Assertion failed: list_empty`) if the + // context is torn down while a deferred is still pending (a timed-out + // never-settling host call). We track them and dispose any survivors in the + // finally, before `vm.dispose()`. + const deferreds = new Set(); + try { - this.installCtx(vm, args.ctx, new Set(args.capabilities), args.origin, txState); + this.installCtx(vm, args.ctx, new Set(args.capabilities), args.origin, txState, deferreds); // L1 expressions are pure-sync: evaluate and read __result. if (args.isExpression) { @@ -253,7 +266,7 @@ export class QuickJSScriptRunner implements ScriptRunner { return { value, durationMs: Date.now() - start }; } - // L2 scripts: wrap as async IIFE and use side-channel + asyncified pump. + // L2 scripts: wrap as async IIFE and use a side-channel + pump loop. // Each pump iteration: // 1. yield to the host event loop (lets host promises settle) // 2. drain QuickJS pending jobs (advances the .then chain) @@ -271,7 +284,7 @@ export class QuickJSScriptRunner implements ScriptRunner { );`; sliceStart = Date.now(); - const evalRes = await vm.evalCodeAsync(wrapped); + const evalRes = vm.evalCode(wrapped); cpuMs += Date.now() - sliceStart; if (evalRes.error) { const budget = budgetError(0); @@ -392,8 +405,25 @@ export class QuickJSScriptRunner implements ScriptRunner { } } } - // newAsyncContext() owns its WASM module; disposing the context disposes - // the runtime + module together. + // Free every host-side deferred (settled or not) — the `newPromise` + // contract, and mandatory before context teardown on the sync variant: a + // pending deferred left alive makes `JS_FreeRuntime` abort. Best-effort so + // one bad handle can't mask the real outcome. + for (const d of deferreds) { + if (d.alive) { + try { + d.dispose(); + } catch { + /* already gone / VM aborted — ignore */ + } + } + } + deferreds.clear(); + // Dispose the context — this frees its runtime + all QuickJS allocations + // (QuickJSWASMModule.newContext: "the runtime will be disposed when the + // context is disposed"). The sync `QuickJSWASMModule` has no dispose(); + // dropping `mod` at function scope lets GC reclaim its WebAssembly instance + // + linear memory, preserving per-invocation isolation (ADR-0102 D2). vm.dispose(); } } @@ -408,11 +438,12 @@ export class QuickJSScriptRunner implements ScriptRunner { * `find/count/insert/...` are async) without asyncify's single-unwind limit. */ private installCtx( - vm: QuickJSAsyncContext, + vm: QuickJSContext, ctx: ScriptContext, caps: Set, origin: ScriptOrigin, txState: TxState, + deferreds: Set, ): void { setGlobalJson(vm, '__input', ctx.input); setGlobalJson(vm, '__previous', ctx.previous); @@ -450,8 +481,8 @@ export class QuickJSScriptRunner implements ScriptRunner { const wrap = vm.newObject(); const READ = ['find', 'findOne', 'count', 'aggregate'] as const; const WRITE = ['insert', 'update', 'delete', 'updateMany', 'deleteMany', 'upsert'] as const; - for (const m of READ) installApiMethod(vm, wrap, m, objectName, ctx, caps, 'api.read', origin, txState); - for (const m of WRITE) installApiMethod(vm, wrap, m, objectName, ctx, caps, 'api.write', origin, txState); + for (const m of READ) installApiMethod(vm, wrap, m, objectName, ctx, caps, 'api.read', origin, txState, deferreds); + for (const m of WRITE) installApiMethod(vm, wrap, m, objectName, ctx, caps, 'api.write', origin, txState, deferreds); return wrap; }); vm.setProp(apiObj, 'object', objectFn); @@ -477,6 +508,7 @@ export class QuickJSScriptRunner implements ScriptRunner { ); } const deferred = vm.newPromise(); + deferreds.add(deferred); void (async () => { try { await run(); @@ -615,26 +647,26 @@ interface TxState { /** * 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"). + * IMPORTANT: host calls settle through a real QuickJS promise (a deferred), NOT + * an asyncified call. Asyncify unwound the WASM stack while a host call was in + * flight and forbade one asyncified call running while another was unwound ("the + * stack cannot be unwound twice"): a script awaiting two host calls in sequence — + * e.g. the real `lead_apply_convert` action doing `findOne()` then `update()` — + * tripped exactly that, corrupting the wasm heap (`memory access out of bounds` / + * `p->ref_count == 0`) and blowing the pump budget. Shedding that dependency is + * precisely what let the runner drop the asyncify build entirely and move to the + * sync variant (ADR-0102 D2). * - * 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 + * We hand the VM a deferred promise and settle it from the host event loop. + * Sequential `await`s are 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, + vm: QuickJSContext, parent: QuickJSHandle, method: string, objectName: string, @@ -643,6 +675,7 @@ function installApiMethod( required: HookBodyCapability, origin: ScriptOrigin, txState: TxState, + deferreds: Set, ): void { const fn = vm.newFunction(method, (...argHandles) => { // Capability gate — throw synchronously so the VM sees a normal exception at @@ -661,6 +694,7 @@ function installApiMethod( const args = argHandles.map((h) => vm.dump(h)); const deferred = vm.newPromise(); + deferreds.add(deferred); void (async () => { try { // While a transaction is open, resolve the repository from the @@ -731,7 +765,7 @@ function safeJsonStringify(v: unknown): string { } /** Marshal a host JSON-serializable value into a QuickJS handle. */ -function jsonToHandle(vm: QuickJSAsyncContext, v: unknown): QuickJSHandle { +function jsonToHandle(vm: QuickJSContext, v: unknown): QuickJSHandle { const json = safeJsonStringify(v); const r = vm.evalCode(`(${json})`); if (r.error) { @@ -742,7 +776,7 @@ function jsonToHandle(vm: QuickJSAsyncContext, v: unknown): QuickJSHandle { return r.value; } -function setGlobalJson(vm: QuickJSAsyncContext, name: string, v: unknown): void { +function setGlobalJson(vm: QuickJSContext, name: string, v: unknown): void { const json = safeJsonStringify(v); const result = vm.evalCode(`(${json})`); if (result.error) { @@ -753,7 +787,7 @@ function setGlobalJson(vm: QuickJSAsyncContext, name: string, v: unknown): void result.value.dispose(); } -function setObjectJson(vm: QuickJSAsyncContext, parent: QuickJSHandle, key: string, v: unknown): void { +function setObjectJson(vm: QuickJSContext, parent: QuickJSHandle, key: string, v: unknown): void { const json = safeJsonStringify(v); const result = vm.evalCode(`(${json})`); if (result.error) { @@ -773,7 +807,7 @@ function setObjectJson(vm: QuickJSAsyncContext, parent: QuickJSHandle, key: stri * Returns `undefined` if the read fails for any reason — callers fall back to * the script's return value in that case. */ -function readCtxInputJson(vm: QuickJSAsyncContext): Record | undefined { +function readCtxInputJson(vm: QuickJSContext): Record | undefined { try { const r = vm.evalCode(`JSON.stringify(globalThis.__ctx && globalThis.__ctx.input || null)`); if (r.error) {