diff --git a/.changeset/sandbox-timeout-env-override.md b/.changeset/sandbox-timeout-env-override.md new file mode 100644 index 0000000000..2b49d8b7d1 --- /dev/null +++ b/.changeset/sandbox-timeout-env-override.md @@ -0,0 +1,33 @@ +--- +'@objectstack/runtime': minor +'@objectstack/types': minor +--- + +feat(runtime): env-overridable sandbox hook/action timeout default (#3259) + +The QuickJS sandbox enforces a wall-clock deadline on every hook/action +invocation (250ms hooks / 5000ms actions). Each invocation compiles a fresh +WASM module, and a nested hook compiles ANOTHER one inside the parent's budget, +so on a heavily loaded or slow host — an oversubscribed CI runner, constrained +production hardware — that fixed VM-creation cost alone can trip the hook +default even while the VM is still making progress. On CI this surfaced as an +intermittent `hook '…' exceeded timeout of 250ms` flake on PRs that never +touched the sandbox path. + +The per-invocation timeout DEFAULT is now resolvable from the environment via +`resolveSandboxTimeoutMs` (`@objectstack/types`), which `QuickJSScriptRunner` +consults, so an operator can raise the floor once, deployment-wide, instead of +re-tuning every call site: + +- `OS_SANDBOX_HOOK_TIMEOUT_MS` — default hook budget (ms) +- `OS_SANDBOX_ACTION_TIMEOUT_MS` — default action budget (ms) + +Precedence is unchanged: an explicit `hookTimeoutMs` / `actionTimeoutMs` passed +to the runner still wins over the env var, and a body's own declared `timeoutMs` +still wins over the resolved default (the smaller of the explicit values). Only +a positive integer is honored; unset / empty / non-numeric / non-positive keeps +the built-in 250ms / 5000ms defaults, so behaviour is byte-for-byte unchanged +when the vars are absent — production is unaffected unless it opts in. + +CI's Test Core now sets `OS_SANDBOX_HOOK_TIMEOUT_MS=10000` so the shared-runner +load flake can't recur; genuine hangs stay bounded by each test's own timeout. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d67487569..db55d04ad7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,6 +15,17 @@ concurrency: group: ci-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true +# The QuickJS sandbox compiles a fresh WASM module per hook/action invocation +# (and a nested hook compiles another inside the parent's budget). Hosted runners +# are 4-vCPU and the test job oversubscribes them (`turbo --concurrency=4` + each +# vitest worker), so that fixed VM-creation cost alone intermittently tripped the +# 250ms hook default even while the VM was progressing — a load flake unrelated to +# the change under test (#3259). Raise the sandbox hook budget for the whole +# workflow; production keeps the 250ms default (this only sets it in CI). Real +# hangs are still bounded by each test's own vitest timeout. +env: + OS_SANDBOX_HOOK_TIMEOUT_MS: '10000' + jobs: filter: runs-on: ubuntu-latest diff --git a/content/docs/deployment/environment-variables.mdx b/content/docs/deployment/environment-variables.mdx index 923e35fde6..92c474bc74 100644 --- a/content/docs/deployment/environment-variables.mdx +++ b/content/docs/deployment/environment-variables.mdx @@ -331,6 +331,8 @@ the hosted ObjectOS Cloud control plane. | `OS_ENV_CACHE_TTL_MS` | number | `300000` | Env-record cache TTL in ms. | | `OS_ARTIFACT_CACHE_TTL_MS` | number | `300000` | Artifact-record cache TTL in ms. | | `OS_ARTIFACT_FETCH_TIMEOUT_MS` | number | `60000` | Timeout for remote artifact fetches. Only a positive value is honored — an unset, non-numeric, or `0` value falls back to the 60s default (pass `fetchTimeoutMs: 0` in `artifactSource` config to actually disable the timeout). | +| `OS_SANDBOX_HOOK_TIMEOUT_MS` | number | `250` | Default wall-clock budget for a sandboxed **hook** body (QuickJS). Each invocation compiles a fresh WASM module and a nested hook compiles another inside the parent's budget, so a loaded/slow host (e.g. an oversubscribed CI runner) may need a larger floor. Only a positive integer is honored; unset / non-numeric / non-positive keeps the 250ms default. A hook body's own declared `timeoutMs` still wins over this. | +| `OS_SANDBOX_ACTION_TIMEOUT_MS` | number | `5000` | Default wall-clock budget for a sandboxed **action** body (QuickJS). Same resolution rules as the hook variant above (positive integer only; an action body's own `timeoutMs` still wins). | | `OS_INLINE_SEED_BUDGET_MS` | number | `8000` | Time budget for synchronous seed execution at boot before deferring to a worker. | | `OS_TENANT_AUDIT` | flag | `1` | Set to `0` to silence the tenant-isolation audit warnings emitted by the SQL driver. | diff --git a/packages/runtime/src/sandbox/quickjs-runner.test.ts b/packages/runtime/src/sandbox/quickjs-runner.test.ts index a108fa42c4..783a322042 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.test.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.test.ts @@ -1,6 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { QuickJSScriptRunner, SandboxError } from './quickjs-runner.js'; import type { ScriptContext, ScriptRunOptions } from './script-runner.js'; @@ -370,9 +370,13 @@ 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(); + // Pin the hook default to 250ms EXPLICITLY (not the bare stock constructor) so + // these assertions on the effective budget stay independent of any ambient + // `OS_SANDBOX_HOOK_TIMEOUT_MS` the environment/CI may set (#3259). They assert + // the resolution logic — body.timeoutMs vs the runner default — which is + // identical whether that default came from the built-in constant or an + // explicit option. (A dedicated suite below covers the env override itself.) + const defaultRunner = new QuickJSScriptRunner({ hookTimeoutMs: 250 }); 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 @@ -423,6 +427,60 @@ describe('QuickJSScriptRunner — timeout resolution honors body.timeoutMs (#186 }, 10000); }); +// --------------------------------------------------------------------------- +// Env-overridable timeout defaults (#3259). Every invocation compiles a fresh +// WASM module, and a nested hook compiles another inside the parent's budget, so +// on a loaded/slow host (an oversubscribed CI runner) the 250ms hook default can +// trip even while the VM is making progress. `OS_SANDBOX_HOOK_TIMEOUT_MS` lets an +// operator raise the FALLBACK default without a code change; an explicit +// constructor option still wins over the env, and a body's own timeoutMs still +// wins over the resolved default. Each test constructs its runner AFTER setting +// the env (the constructor reads it once), and the env is saved/restored so a +// CI-wide value doesn't leak in or out. +// --------------------------------------------------------------------------- +describe('QuickJSScriptRunner — env-overridable timeout defaults (#3259)', () => { + const HOOK_ENV = 'OS_SANDBOX_HOOK_TIMEOUT_MS'; + let saved: string | undefined; + beforeEach(() => { + saved = process.env[HOOK_ENV]; + delete process.env[HOOK_ENV]; + }); + afterEach(() => { + if (saved === undefined) delete process.env[HOOK_ENV]; + else process.env[HOOK_ENV] = saved; + }); + + // A never-settling host call forces the deadline path; the SandboxError embeds + // the RESOLVED budget, so asserting on the message proves which timeout applied + // without a flaky wall-clock measurement. + const neverSettles = { object: () => ({ update: () => new Promise(() => {}) }) }; + const runHook = (r: QuickJSScriptRunner) => + r.runScript( + { language: 'js', source: "return await ctx.api.object('x').update({});", capabilities: ['api.write'] }, + ctx({ api: neverSettles }), + hookOpts, + ); + + it('falls back to the built-in 250ms hook default when the env var is unset', async () => { + await expect(runHook(new QuickJSScriptRunner())).rejects.toThrow(/timeout of 250ms/); + }, 10000); + + it('uses OS_SANDBOX_HOOK_TIMEOUT_MS as the default when set', async () => { + process.env[HOOK_ENV] = '150'; + await expect(runHook(new QuickJSScriptRunner())).rejects.toThrow(/timeout of 150ms/); + }, 10000); + + it('lets an explicit constructor option win over the env var', async () => { + process.env[HOOK_ENV] = '150'; + await expect(runHook(new QuickJSScriptRunner({ hookTimeoutMs: 50 }))).rejects.toThrow(/timeout of 50ms/); + }, 10000); + + it('ignores a non-numeric / non-positive env value and keeps the built-in default', async () => { + process.env[HOOK_ENV] = 'not-a-number'; + await expect(runHook(new QuickJSScriptRunner())).rejects.toThrow(/timeout of 250ms/); + }, 10000); +}); + 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 diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index 3fd29f77fb..39abad3fb3 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -17,9 +17,16 @@ * `ctx.api.object('foo').count(...)` and the host method runs in node). * * Trade-offs: - * - Per-invocation overhead is dominated by VM creation. We pool runtimes per - * `(origin.kind, capabilities-set)` to amortise startup. Pool size is bounded - * by `maxPooled` (default 8); evicted runtimes are disposed. + * - 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. The cost is + * real, though: a nested hook compiles a SECOND module inside the parent's + * budget, so on a loaded/slow host the fixed compile cost alone can trip the + * hook timeout. The per-invocation timeout default is therefore env-overridable + * via `OS_SANDBOX_HOOK_TIMEOUT_MS` / `OS_SANDBOX_ACTION_TIMEOUT_MS` + * (framework#3259) so an operator can raise the floor without a code change. * - Memory caps are advisory under quickjs (engine has no hard MB cap); the * runner uses `setMemoryLimit(memoryMb * 1MB)` which is best-effort. */ @@ -29,6 +36,7 @@ import { type QuickJSAsyncContext, type QuickJSHandle, } from 'quickjs-emscripten'; +import { resolveSandboxTimeoutMs } from '@objectstack/types'; import type { HookBody, ScriptBody, ExpressionBody, HookBodyCapability } from '@objectstack/spec/data'; import type { ScriptContext, @@ -55,9 +63,15 @@ export class QuickJSScriptRunner implements ScriptRunner { private opts: Required; constructor(opts: QuickJSScriptRunnerOptions = {}) { + // Precedence for the per-invocation timeout default: an explicit constructor + // option wins; else the deployment's `OS_SANDBOX_{HOOK,ACTION}_TIMEOUT_MS` + // env override (so a loaded/slow host — e.g. an oversubscribed CI runner — + // can raise the floor without a code change, framework#3259); else the + // built-in default. `resolveSandboxTimeoutMs` returns the built-in fallback + // untouched when the env var is unset, so default behaviour is unchanged. this.opts = { - hookTimeoutMs: opts.hookTimeoutMs ?? DEFAULT_HOOK_TIMEOUT_MS, - actionTimeoutMs: opts.actionTimeoutMs ?? DEFAULT_ACTION_TIMEOUT_MS, + hookTimeoutMs: opts.hookTimeoutMs ?? resolveSandboxTimeoutMs('hook', DEFAULT_HOOK_TIMEOUT_MS), + actionTimeoutMs: opts.actionTimeoutMs ?? resolveSandboxTimeoutMs('action', DEFAULT_ACTION_TIMEOUT_MS), memoryMb: opts.memoryMb ?? DEFAULT_MEMORY_MB, }; } diff --git a/packages/types/src/env.test.ts b/packages/types/src/env.test.ts index 45f9fae079..37f77e8637 100644 --- a/packages/types/src/env.test.ts +++ b/packages/types/src/env.test.ts @@ -6,6 +6,7 @@ import { readEnvWithDeprecation, resolveAllowDegradedTenancy, resolveSearchPinyinEnabled, + resolveSandboxTimeoutMs, isMcpServerEnabled, resolveMcpStdioAutoStart, } from './env.js'; @@ -237,3 +238,49 @@ describe('MCP switches — HTTP surface vs stdio auto-start are decoupled (#3167 expect(resolveMcpStdioAutoStart()).toEqual({ enabled: true, viaDeprecatedAlias: false }); }); }); + +describe('resolveSandboxTimeoutMs (#3259)', () => { + const HOOK = 'OS_SANDBOX_HOOK_TIMEOUT_MS'; + const ACTION = 'OS_SANDBOX_ACTION_TIMEOUT_MS'; + const origHook = process.env[HOOK]; + const origAction = process.env[ACTION]; + afterEach(() => { + if (origHook === undefined) delete process.env[HOOK]; + else process.env[HOOK] = origHook; + if (origAction === undefined) delete process.env[ACTION]; + else process.env[ACTION] = origAction; + }); + + it('returns the fallback unchanged when the var is unset', () => { + delete process.env[HOOK]; + delete process.env[ACTION]; + expect(resolveSandboxTimeoutMs('hook', 250)).toBe(250); + expect(resolveSandboxTimeoutMs('action', 5000)).toBe(5000); + }); + + it('reads the kind-specific var and parses a positive integer', () => { + process.env[HOOK] = '10000'; + process.env[ACTION] = '20000'; + expect(resolveSandboxTimeoutMs('hook', 250)).toBe(10000); + expect(resolveSandboxTimeoutMs('action', 5000)).toBe(20000); + }); + + it('does not cross the wires between the hook and action vars', () => { + process.env[HOOK] = '999'; + delete process.env[ACTION]; + expect(resolveSandboxTimeoutMs('hook', 250)).toBe(999); + expect(resolveSandboxTimeoutMs('action', 5000)).toBe(5000); // action unset → fallback + }); + + it('ignores empty / non-numeric / non-positive values and keeps the fallback', () => { + for (const bad of ['', ' ', 'abc', '0', '-5', 'NaN']) { + process.env[HOOK] = bad; + expect(resolveSandboxTimeoutMs('hook', 250), `value=${JSON.stringify(bad)}`).toBe(250); + } + }); + + it('tolerates a leading integer with trailing junk (parseInt semantics, as resolveOrgLimit)', () => { + process.env[HOOK] = '3000ms'; + expect(resolveSandboxTimeoutMs('hook', 250)).toBe(3000); + }); +}); diff --git a/packages/types/src/env.ts b/packages/types/src/env.ts index 26f1b6ae57..ad17986c73 100644 --- a/packages/types/src/env.ts +++ b/packages/types/src/env.ts @@ -239,6 +239,39 @@ export function resolveSearchPinyinEnabled(opts?: { locales?: readonly string[] return (opts?.locales ?? []).some((l) => /^zh([-_]|$)/i.test(String(l ?? '').trim())); } +/** + * SINGLE decision point for the sandbox script-runner's DEFAULT per-invocation + * timeout (ms), for a given origin kind, from the environment (framework#3259). + * + * The QuickJS sandbox enforces a wall-clock deadline on every hook/action + * invocation. The built-in defaults (250ms hooks / 5000ms actions) suit a warm, + * idle host — but every invocation compiles a fresh WASM module, and a nested + * hook compiles ANOTHER one inside the parent's budget, so on a heavily loaded + * or slow host (an oversubscribed CI runner, constrained production hardware) + * that fixed VM-creation cost alone can trip the hook default even while the VM + * is still making progress. That surfaced as an intermittent CI flake ("hook + * '…' exceeded timeout of 250ms"). This lets an operator raise the floor once, + * deployment-wide, instead of re-tuning every call site. + * + * Canonical vars (OS_{DOMAIN}_{NAME}, DOMAIN=SANDBOX): + * - hook → `OS_SANDBOX_HOOK_TIMEOUT_MS` + * - action → `OS_SANDBOX_ACTION_TIMEOUT_MS` + * + * Only a positive integer is honored; unset / empty / non-numeric / non-positive + * falls back to `fallback`, so behaviour is byte-for-byte unchanged when the var + * is absent. This is a FALLBACK default ONLY: an explicit `hookTimeoutMs` / + * `actionTimeoutMs` passed to the runner constructor still wins over it, and a + * body's own declared `timeoutMs` still wins over the resolved default per the + * runner's timeout-resolution rule (the smaller of the explicit values). + */ +export function resolveSandboxTimeoutMs(kind: 'hook' | 'action', fallback: number): number { + const name = kind === 'hook' ? 'OS_SANDBOX_HOOK_TIMEOUT_MS' : 'OS_SANDBOX_ACTION_TIMEOUT_MS'; + const raw = readEnvWithDeprecation(name, [], { silent: true }); + if (raw == null || String(raw).trim() === '') return fallback; + const n = Number.parseInt(String(raw).trim(), 10); + return Number.isFinite(n) && n > 0 ? n : fallback; +} + /** * Internal: clear the dedupe set. Test-only; exposed so suite-wide * deprecation warnings don't bleed between tests.