fix(runtime): de-flake 250ms sandbox hook-timeout tests; honor runner timeout defaults in body-runner#3264
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 16 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
… 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CI's Test Core intermittently fails on QuickJS sandbox timeout tests — same area, different files, never reproducible locally:
packages/runtime/src/sandbox/nested-write.integration.test.ts—hook 'rollup_parent_total' exceeded timeout of 250ms (after 76/93 pump iterations), hit twice on PR fix(platform-objects): surface sys_user phone_number in list views and detail highlights #3263.packages/runtime/src/sandbox/quickjs-runner.test.ts—hook 'lvl4' exceeded timeout of 250ms (after 1068 pump iterations)insurvives a multi-level nested write chain, hit on main runs 29678362032 and 29677462748.Root cause: every sandbox invocation compiles a fresh WASM module (
newAsyncContext()), and a nested hook compiles another one inside the parent hook's 250ms budget. On a loaded CI runner that fixed cost alone can blow the budget — but the failing tests are about nested-write correctness, not about the budget.Also investigated
hook-wrappers.tsrunWithTimeout: not a budget source here — it only applies when hook metadata declarestimeout, which these tests don't. No change needed there.Changes
body-runner.ts— stop hardcoding the?? 250/?? 5000fallbacks as an explicitopts.timeoutMs. When neither the body nor the action declares a timeout, leave it unset soQuickJSScriptRunner'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— the shared runner getshookTimeoutMs: 10_000(those tests exercise sandbox behaviour, not the stock budget). The timeout-resolution suite ([P0] Hook sandbox crashes on nested cross-object writes — forces denormalized workarounds across templates #1867) uses a dedicated stock-default runner, and asserts the effective budget via the error message (timeout of 250ms/timeout of 50ms) instead of a wall-clock bound — fully de-timed.nested-write.integration.test.ts/nested-write-real-sqlite.integration.test.ts— construct the runner withhookTimeoutMs: 10_000; their subject is the hook → sandbox → nested-write path.No changeset: pure bug fix / test de-flake, no functional change to defaults.
Verification
packages/runtimesandbox suite (5 files, 50 tests): green across 3 consecutive runs.packages/runtimesuite: identical pass/fail set before vs. after the change (the 10 local failures are pre-existing worktree environment issues — unbuilt@objectstack/metadata-core, sandboxed network — present at baseline).🤖 Generated with Claude Code