fix(runtime): env-overridable sandbox hook/action timeout to de-flake CI (#3259)

.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.

.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

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. |
 

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<never>(() => {}) }) };
+  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

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<QuickJSScriptRunnerOptions>;
 
   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,
     };
   }

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);
+  });
+});

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.