Skip to content

Commit 8086252

Browse files
baozhoutaoclaude
andcommitted
fix(runtime): de-flake 250ms sandbox hook-timeout tests; honor runner timeout defaults in body-runner
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 <noreply@anthropic.com>
1 parent b28fd59 commit 8086252

4 files changed

Lines changed: 35 additions & 13 deletions

File tree

packages/runtime/src/sandbox/body-runner.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,11 @@ export function hookBodyRunnerFactory(
7171
name: hook.name,
7272
object: typeof (hook as any).object === 'string' ? (hook as any).object : undefined,
7373
},
74-
timeoutMs: (body as any).timeoutMs ?? 250,
74+
// When the body declares no timeout, leave opts.timeoutMs unset so the
75+
// runner's own configurable hook default applies (QuickJSScriptRunner
76+
// defaults to 250ms). Hardcoding the fallback here would make that
77+
// constructor option dead for hooks.
78+
timeoutMs: (body as any).timeoutMs,
7579
});
7680
applyMutationsToInput(engineCtx, result);
7781
} catch (err: any) {
@@ -123,7 +127,9 @@ export function actionBodyRunnerFactory(
123127
});
124128
const result = await runner.run(body, sandboxCtx, {
125129
origin: { kind: 'action', name: action.name, object: action.object },
126-
timeoutMs: (body as any).timeoutMs ?? action.timeoutMs ?? 5000,
130+
// As with hooks above: no declared timeout → let the runner's
131+
// configurable action default (5000ms) apply.
132+
timeoutMs: (body as any).timeoutMs ?? action.timeoutMs,
127133
});
128134
return result.value;
129135
} catch (err: any) {

packages/runtime/src/sandbox/nested-write-real-sqlite.integration.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,9 @@ describe('#1867 nested cross-object write — REAL SqlDriver (better-sqlite3, on
8686
engine.registerDriver(driver, true);
8787
await engine.init();
8888
for (const o of [EXPENSE_REPORT, EXPENSE_LINE]) engine.registry.registerObject(o as any);
89-
engine.setDefaultBodyRunner(hookBodyRunnerFactory(new QuickJSScriptRunner(), { ql: engine, appId: 'expense' }));
89+
// Generous hook budget — the subject is the nested-write path on a real
90+
// driver, not the 250ms default (see nested-write.integration.test.ts).
91+
engine.setDefaultBodyRunner(hookBodyRunnerFactory(new QuickJSScriptRunner({ hookTimeoutMs: 10_000 }), { ql: engine, appId: 'expense' }));
9092
bindHooksToEngine(engine, [ROLLUP_HOOK as any], { packageId: 'expense' });
9193
return engine;
9294
}

packages/runtime/src/sandbox/nested-write.integration.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,13 @@ describe('#1867 nested cross-object write from a hook (real engine + sandbox)',
9090
engine.registerDriver(driver, true);
9191
await engine.init();
9292
for (const o of [parent, child]) engine.registry.registerObject(o as any);
93+
// Generous hook budget: the subject here is nested-write correctness, not
94+
// the 250ms default. Each hook invocation compiles a fresh WASM module and
95+
// the nested parent hook compiles another inside the child's budget — on a
96+
// loaded CI machine that overhead alone blew 250ms ("hook
97+
// 'rollup_parent_total' exceeded timeout of 250ms").
9398
engine.setDefaultBodyRunner(
94-
hookBodyRunnerFactory(new QuickJSScriptRunner(), { ql: engine, appId: 'test' }),
99+
hookBodyRunnerFactory(new QuickJSScriptRunner({ hookTimeoutMs: 10_000 }), { ql: engine, appId: 'test' }),
95100
);
96101
});
97102

packages/runtime/src/sandbox/quickjs-runner.test.ts

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,13 @@ import { describe, it, expect } from 'vitest';
44
import { QuickJSScriptRunner, SandboxError } from './quickjs-runner.js';
55
import type { ScriptContext, ScriptRunOptions } from './script-runner.js';
66

7-
const runner = new QuickJSScriptRunner();
7+
// Generous hook budget: these tests exercise sandbox *behaviour*, not the stock
8+
// 250ms hook budget. Every invocation compiles a fresh WASM module, and nested
9+
// hooks compile another one inside the parent's budget — on a loaded CI machine
10+
// that fixed cost alone can blow 250ms and flake (e.g. "hook 'lvl4' exceeded
11+
// timeout of 250ms"). Tests that ARE about the default budget use
12+
// `defaultRunner` below.
13+
const runner = new QuickJSScriptRunner({ hookTimeoutMs: 10_000 });
814
const hookOpts: ScriptRunOptions = { origin: { kind: 'hook', name: 't' } };
915
const actionOpts: ScriptRunOptions = { origin: { kind: 'action', name: 't' } };
1016

@@ -364,6 +370,10 @@ describe('QuickJSScriptRunner — nested sandbox re-entrancy (#1867)', () => {
364370
// the 250ms hook default and killed mid-flight.
365371
// ---------------------------------------------------------------------------
366372
describe('QuickJSScriptRunner — timeout resolution honors body.timeoutMs (#1867)', () => {
373+
// Stock engine defaults (250ms hooks) — these tests assert the default budget
374+
// itself, so they must NOT use the generous shared `runner` above.
375+
const defaultRunner = new QuickJSScriptRunner();
376+
367377
it('honors a hook body timeoutMs above the 250ms hook default', async () => {
368378
// Host call settles at ~600ms — comfortably past the old 250ms hook cap but
369379
// within the body's declared 5000ms budget. Must resolve, not time out.
@@ -375,7 +385,7 @@ describe('QuickJSScriptRunner — timeout resolution honors body.timeoutMs (#186
375385
},
376386
}),
377387
};
378-
const r = await runner.runScript(
388+
const r = await defaultRunner.runScript(
379389
{
380390
language: 'js',
381391
source: "return await ctx.api.object('x').update({});",
@@ -390,27 +400,26 @@ describe('QuickJSScriptRunner — timeout resolution honors body.timeoutMs (#186
390400

391401
it('still applies the 250ms hook default when the body declares no timeoutMs', async () => {
392402
const api = { object: () => ({ update: () => new Promise<never>(() => {}) }) };
393-
const start = Date.now();
394403
await expect(
395-
runner.runScript(
404+
defaultRunner.runScript(
396405
{ language: 'js', source: "return await ctx.api.object('x').update({});", capabilities: ['api.write'] },
397406
ctx({ api }),
398407
hookOpts,
399408
),
400-
).rejects.toThrow(/timeout/i);
401-
// Fired near the 250ms default, not some larger value.
402-
expect(Date.now() - start).toBeLessThan(2000);
409+
// The error message embeds the effective budget — asserting on it proves
410+
// the 250ms default applied without a flaky wall-clock measurement.
411+
).rejects.toThrow(/timeout of 250ms/);
403412
}, 10000);
404413

405414
it('lets a hook body LOWER its timeout below the default', async () => {
406415
const api = { object: () => ({ update: () => new Promise<never>(() => {}) }) };
407416
await expect(
408-
runner.runScript(
417+
defaultRunner.runScript(
409418
{ language: 'js', source: "return await ctx.api.object('x').update({});", capabilities: ['api.write'], timeoutMs: 50 },
410419
ctx({ api }),
411420
hookOpts,
412421
),
413-
).rejects.toThrow(/timeout/i);
422+
).rejects.toThrow(/timeout of 50ms/);
414423
}, 10000);
415424
});
416425

0 commit comments

Comments
 (0)