From 80862528c8aa3f5811fdadf47dacf70d00312798 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Sun, 19 Jul 2026 04:13:16 -0700 Subject: [PATCH] fix(runtime): de-flake 250ms sandbox hook-timeout tests; honor runner timeout defaults in body-runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's Test Core intermittently failed sandbox tests with 'hook ... exceeded timeout of 250ms' (nested-write.integration.test.ts 'rollup_parent_total' on PR #3263 twice; quickjs-runner.test.ts 'lvl4' on main runs 29678362032 / 29677462748). Root cause: every sandbox invocation compiles a fresh WASM module (newAsyncContext), and a nested hook compiles another one inside the parent hook's budget — on a loaded CI runner that fixed cost alone can blow the 250ms default, while the tests in question are about nested-write correctness, not the budget. - body-runner: stop hardcoding the 250ms/5000ms fallbacks as an explicit opts.timeoutMs — when neither body nor action declares a timeout, leave it unset so QuickJSScriptRunner's constructor defaults (hookTimeoutMs / actionTimeoutMs, same 250/5000 values) apply. Default behavior is unchanged; the previously dead constructor option now works. - quickjs-runner.test.ts: shared runner gets hookTimeoutMs 10s (behavioral tests); the timeout-resolution suite uses a dedicated stock-default runner and asserts the effective budget via the error message ('timeout of 250ms'/'50ms') instead of a wall-clock bound. - both nested-write integration tests: construct the runner with hookTimeoutMs 10s — their subject is the hook → sandbox → nested-write path, not the default budget. hook-wrappers.ts runWithTimeout was investigated and is not a budget source here: it only applies when hook metadata declares 'timeout', which these tests do not. Co-Authored-By: Claude Fable 5 --- packages/runtime/src/sandbox/body-runner.ts | 10 +++++-- ...sted-write-real-sqlite.integration.test.ts | 4 ++- .../sandbox/nested-write.integration.test.ts | 7 ++++- .../src/sandbox/quickjs-runner.test.ts | 27 ++++++++++++------- 4 files changed, 35 insertions(+), 13 deletions(-) diff --git a/packages/runtime/src/sandbox/body-runner.ts b/packages/runtime/src/sandbox/body-runner.ts index 82923bd873..c0cf89f483 100644 --- a/packages/runtime/src/sandbox/body-runner.ts +++ b/packages/runtime/src/sandbox/body-runner.ts @@ -71,7 +71,11 @@ export function hookBodyRunnerFactory( name: hook.name, object: typeof (hook as any).object === 'string' ? (hook as any).object : undefined, }, - timeoutMs: (body as any).timeoutMs ?? 250, + // When the body declares no timeout, leave opts.timeoutMs unset so the + // runner's own configurable hook default applies (QuickJSScriptRunner + // defaults to 250ms). Hardcoding the fallback here would make that + // constructor option dead for hooks. + timeoutMs: (body as any).timeoutMs, }); applyMutationsToInput(engineCtx, result); } catch (err: any) { @@ -123,7 +127,9 @@ export function actionBodyRunnerFactory( }); const result = await runner.run(body, sandboxCtx, { origin: { kind: 'action', name: action.name, object: action.object }, - timeoutMs: (body as any).timeoutMs ?? action.timeoutMs ?? 5000, + // As with hooks above: no declared timeout → let the runner's + // configurable action default (5000ms) apply. + timeoutMs: (body as any).timeoutMs ?? action.timeoutMs, }); return result.value; } catch (err: any) { diff --git a/packages/runtime/src/sandbox/nested-write-real-sqlite.integration.test.ts b/packages/runtime/src/sandbox/nested-write-real-sqlite.integration.test.ts index adb49c55e6..e31897f27a 100644 --- a/packages/runtime/src/sandbox/nested-write-real-sqlite.integration.test.ts +++ b/packages/runtime/src/sandbox/nested-write-real-sqlite.integration.test.ts @@ -86,7 +86,9 @@ describe('#1867 nested cross-object write — REAL SqlDriver (better-sqlite3, on engine.registerDriver(driver, true); await engine.init(); for (const o of [EXPENSE_REPORT, EXPENSE_LINE]) engine.registry.registerObject(o as any); - engine.setDefaultBodyRunner(hookBodyRunnerFactory(new QuickJSScriptRunner(), { ql: engine, appId: 'expense' })); + // Generous hook budget — the subject is the nested-write path on a real + // driver, not the 250ms default (see nested-write.integration.test.ts). + engine.setDefaultBodyRunner(hookBodyRunnerFactory(new QuickJSScriptRunner({ hookTimeoutMs: 10_000 }), { ql: engine, appId: 'expense' })); bindHooksToEngine(engine, [ROLLUP_HOOK as any], { packageId: 'expense' }); return engine; } diff --git a/packages/runtime/src/sandbox/nested-write.integration.test.ts b/packages/runtime/src/sandbox/nested-write.integration.test.ts index 53561aa077..57c143aa1d 100644 --- a/packages/runtime/src/sandbox/nested-write.integration.test.ts +++ b/packages/runtime/src/sandbox/nested-write.integration.test.ts @@ -90,8 +90,13 @@ describe('#1867 nested cross-object write from a hook (real engine + sandbox)', engine.registerDriver(driver, true); await engine.init(); for (const o of [parent, child]) engine.registry.registerObject(o as any); + // Generous hook budget: the subject here is nested-write correctness, not + // the 250ms default. Each hook invocation compiles a fresh WASM module and + // the nested parent hook compiles another inside the child's budget — on a + // loaded CI machine that overhead alone blew 250ms ("hook + // 'rollup_parent_total' exceeded timeout of 250ms"). engine.setDefaultBodyRunner( - hookBodyRunnerFactory(new QuickJSScriptRunner(), { ql: engine, appId: 'test' }), + hookBodyRunnerFactory(new QuickJSScriptRunner({ hookTimeoutMs: 10_000 }), { ql: engine, appId: 'test' }), ); }); diff --git a/packages/runtime/src/sandbox/quickjs-runner.test.ts b/packages/runtime/src/sandbox/quickjs-runner.test.ts index 7e0b25786e..a108fa42c4 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.test.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.test.ts @@ -4,7 +4,13 @@ import { describe, it, expect } from 'vitest'; import { QuickJSScriptRunner, SandboxError } from './quickjs-runner.js'; import type { ScriptContext, ScriptRunOptions } from './script-runner.js'; -const runner = new QuickJSScriptRunner(); +// Generous hook budget: these tests exercise sandbox *behaviour*, not the stock +// 250ms hook budget. Every invocation compiles a fresh WASM module, and nested +// hooks compile another one inside the parent's budget — on a loaded CI machine +// that fixed cost alone can blow 250ms and flake (e.g. "hook 'lvl4' exceeded +// timeout of 250ms"). Tests that ARE about the default budget use +// `defaultRunner` below. +const runner = new QuickJSScriptRunner({ hookTimeoutMs: 10_000 }); const hookOpts: ScriptRunOptions = { origin: { kind: 'hook', name: 't' } }; const actionOpts: ScriptRunOptions = { origin: { kind: 'action', name: 't' } }; @@ -364,6 +370,10 @@ describe('QuickJSScriptRunner — nested sandbox re-entrancy (#1867)', () => { // the 250ms hook default and killed mid-flight. // --------------------------------------------------------------------------- describe('QuickJSScriptRunner — timeout resolution honors body.timeoutMs (#1867)', () => { + // Stock engine defaults (250ms hooks) — these tests assert the default budget + // itself, so they must NOT use the generous shared `runner` above. + const defaultRunner = new QuickJSScriptRunner(); + it('honors a hook body timeoutMs above the 250ms hook default', async () => { // Host call settles at ~600ms — comfortably past the old 250ms hook cap but // within the body's declared 5000ms budget. Must resolve, not time out. @@ -375,7 +385,7 @@ describe('QuickJSScriptRunner — timeout resolution honors body.timeoutMs (#186 }, }), }; - const r = await runner.runScript( + const r = await defaultRunner.runScript( { language: 'js', source: "return await ctx.api.object('x').update({});", @@ -390,27 +400,26 @@ describe('QuickJSScriptRunner — timeout resolution honors body.timeoutMs (#186 it('still applies the 250ms hook default when the body declares no timeoutMs', async () => { const api = { object: () => ({ update: () => new Promise(() => {}) }) }; - const start = Date.now(); await expect( - runner.runScript( + defaultRunner.runScript( { language: 'js', source: "return await ctx.api.object('x').update({});", capabilities: ['api.write'] }, ctx({ api }), hookOpts, ), - ).rejects.toThrow(/timeout/i); - // Fired near the 250ms default, not some larger value. - expect(Date.now() - start).toBeLessThan(2000); + // The error message embeds the effective budget — asserting on it proves + // the 250ms default applied without a flaky wall-clock measurement. + ).rejects.toThrow(/timeout of 250ms/); }, 10000); it('lets a hook body LOWER its timeout below the default', async () => { const api = { object: () => ({ update: () => new Promise(() => {}) }) }; await expect( - runner.runScript( + defaultRunner.runScript( { language: 'js', source: "return await ctx.api.object('x').update({});", capabilities: ['api.write'], timeoutMs: 50 }, ctx({ api }), hookOpts, ), - ).rejects.toThrow(/timeout/i); + ).rejects.toThrow(/timeout of 50ms/); }, 10000); });