fix(runtime): sandbox host calls via deferred promises, deadline-bounded pump#1972
Merged
Merged
Conversation
…ded pump
The QuickJS sandbox exposed `ctx.api.object(x).find/update/...` via
`newAsyncifiedFunction`. Asyncify unwinds the WASM stack for each host call and
forbids a second call while the first is unwound ("the stack cannot be unwound
twice"). A script that awaits two host calls in sequence — e.g. the real
`lead_apply_convert` action doing `findOne()` then `update()` — hit exactly
that: the second call was driven from a resumed continuation inside
`executePendingJobs` (a non-async frame), which corrupted the wasm heap
(`memory access out of bounds` / `Assertion failed: p->ref_count == 0`) and,
when it limped along, exhausted the fixed 1000-iteration pump budget, surfacing
in production as:
action 'lead_apply_convert' did not resolve after 1000 pump iterations
Verified empirically that asyncify cannot drive 2+ host calls per invocation
under any driver (manual pump or module top-level await) — both crash.
Fix:
- Expose host API methods as deferred QuickJS promises (`vm.newPromise()`)
instead of asyncified functions. Sequential `await`s become ordinary promises
with no stack unwinding, so any number of host calls compose safely. (Host
method calls now require `await`, which all real actions already do; the
`.object(name)` proxy getter stays synchronous.)
- Bound the pump loop by the configured `timeoutMs` deadline instead of a magic
1000-iteration cap, so a slow-but-progressing script (many sequential writes,
or one write that synchronously drives a downstream record-change automation)
finishes within its timeout, while a stuck/never-settling host call is cut off
with a clear timeout error (the interrupt handler can't fire while parked on a
host promise, so this deadline check is the backstop).
Regression tests (quickjs-runner.test.ts): a host call settling after >1000
event-loop turns, >1000 sequential host calls, and a never-settling host call
that must time out. Before the fix the sequential case crashed the wasm and the
never-settling case hung; both now pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 17 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
github-actions
Bot
added
documentation
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
2 participants
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
Production error from
POST /api/v1/actions/mtc_lead/lead_apply_convert:Root cause
The QuickJS sandbox exposed
ctx.api.object(x).find/update/...vianewAsyncifiedFunction. Asyncify unwinds the WASM stack for each host call and forbids a second call while the first is unwound ("the stack cannot be unwound twice").A script that awaits two host calls in sequence — the real
lead_apply_convertaction doesfindOne()thenupdate()— hits exactly that: the second call is driven from a resumed continuation insideexecutePendingJobs(a non-async frame). That corrupts the wasm heap (memory access out of bounds/Assertion failed: p->ref_count == 0, the偶发 crash already noted in tianshun-mtc's test log) and, when it limps along instead, exhausts the fixed 1000-iteration pump budget — the production error above.I verified empirically that asyncify cannot drive 2+ host calls per invocation under any driver (manual
executePendingJobspump or module top-level await) — both crash with "cannot handle error in suspended function".Fix
vm.newPromise()) instead of asyncified functions. Sequentialawaits become ordinary promises with no stack unwinding, so any number of host calls compose safely. Host method calls now requireawait(every real action already does; the.object(name)proxy getter stays synchronous).timeoutMsinstead of a magic 1000-iteration cap. A slow-but-progressing script (many sequential writes, or one write that synchronously drives a downstream record-change automation) finishes within its timeout; a stuck / never-settling host call is cut off with a clear timeout error (the interrupt handler can't fire while parked on a host promise, so the deadline check is the backstop).Tests
New regression tests in
quickjs-runner.test.ts:body-runner.test.tsupdated: itsctx.api.object("opportunity").count()now usesawait(it previously relied on asyncify's magic-sync, the exact fragile behavior removed).All 31 sandbox tests pass. The one unrelated
publish-drafts seed rowsfailure in this package is pre-existing (fails identically onmainwithout these changes).