QuickJS engine: host-side, side-effect-free serialization via handles - #3263
QuickJS engine: host-side, side-effect-free serialization via handles#3263TooTallNate wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 5b44f6b The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 E2E Test Results❌ Some tests failed ❌ Failed E2E Tests▲ Vercel Production (15 failed)astro-quickjs (1 failed):
example-quickjs (2 failed):
express-quickjs (2 failed):
fastify-quickjs (2 failed):
hono-quickjs (1 failed):
nextjs-turbopack-quickjs (1 failed):
nextjs-webpack-quickjs (1 failed):
nitro-quickjs (1 failed):
nuxt-quickjs (2 failed):
sveltekit-quickjs (1 failed):
vite-quickjs (1 failed):
💻 Local Development (12 failed)astro-stable-quickjs (1 failed):
express-stable-node (1 failed):
express-stable-quickjs (1 failed):
fastify-stable-quickjs (1 failed):
hono-stable-quickjs (1 failed):
nextjs-turbopack-canary-quickjs (1 failed):
nextjs-turbopack-stable-quickjs (1 failed):
nextjs-webpack-stable-quickjs (1 failed):
nitro-stable-quickjs (1 failed):
nuxt-stable-quickjs (1 failed):
sveltekit-stable-quickjs (1 failed):
vite-stable-quickjs (1 failed):
📦 Local Production (13 failed)astro-stable-quickjs (1 failed):
express-stable-quickjs (1 failed):
fastify-stable-quickjs (1 failed):
hono-stable-quickjs (1 failed):
nextjs-turbopack-canary-quickjs (1 failed):
nextjs-turbopack-stable-quickjs (1 failed):
nextjs-webpack-canary-quickjs (1 failed):
nextjs-webpack-stable-quickjs (1 failed):
nitro-stable-node (1 failed):
nitro-stable-quickjs (1 failed):
nuxt-stable-quickjs (1 failed):
sveltekit-stable-quickjs (1 failed):
vite-stable-quickjs (1 failed):
🐘 Local Postgres (12 failed)astro-stable-quickjs (1 failed):
express-stable-quickjs (1 failed):
fastify-stable-quickjs (1 failed):
hono-stable-quickjs (1 failed):
nextjs-turbopack-canary-quickjs (1 failed):
nextjs-turbopack-stable-quickjs (1 failed):
nextjs-webpack-canary-quickjs (1 failed):
nextjs-webpack-stable-quickjs (1 failed):
nitro-stable-quickjs (1 failed):
nuxt-stable-quickjs (1 failed):
sveltekit-stable-quickjs (1 failed):
vite-stable-quickjs (1 failed):
🪟 Windows (1 failed)nextjs-turbopack-quickjs (1 failed):
📋 Other (9 failed)e2e-local-dev-nest-stable-quickjs (1 failed):
e2e-local-dev-tanstack-start-quickjs (1 failed):
e2e-local-postgres-nest-stable-quickjs (1 failed):
e2e-local-postgres-tanstack-start-quickjs (1 failed):
e2e-local-prod-nest-stable-quickjs (1 failed):
e2e-local-prod-tanstack-start-quickjs (1 failed):
e2e-vercel-prod-nest-quickjs (1 failed):
e2e-vercel-prod-tanstack-start-quickjs (2 failed):
E2E Test SummarySummary
Details by Category❌ ▲ Vercel Production
❌ 💻 Local Development
❌ 📦 Local Production
❌ 🐘 Local Postgres
❌ 🪟 Windows
❌ 📋 Other
✅ vercel-multi-region
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
b9dd118 to
40773d2
Compare
a682051 to
6c12c68
Compare
(Re-applied onto the review-fixed base; original commits da27230 + 9814ed9 squashed.) Replace the in-VM serde bundle with a host-side codec (runtime/quickjs-serde.ts) built on quickjs-wasi 3.3's introspection primitives and devalue 5.9's pluggable stringify/parse operations — mirroring the node:vm engine's architecture. Review fixes incorporated: - reducer/reviver key sets are pinned against codec-devalue-vm's workflow mode by exhaustiveness tests (exact order for reducers — first match wins), so the handle-space codec can't silently drift from the shared value-space sets. - the devalue entry in minimumReleaseAgeExclude is removed: the exact version is pinned via the workspace catalog + lockfile, so the cooldown waiver was unnecessary (verified with both frozen and regular installs). - eval-string interpolation inherits the JSON.stringify(cid) hardening from the base branch.
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Reviewed the incremental diff (+2823) and validated empirically against quickjs-wasi 3.3.0. Two silent data-corruption blockers on the guest→host string read — but the good news is a host-side fix exists, is small, and is validated (contrary to the "needs quickjs-wasi 3.4" first read): local branch pgp/quickjs-host-serde-fix (can push), 49 parity tests including 7 new string edge cases, full serde+runtime suites green, and nullByteWorkflow passes end-to-end under WORKFLOW_VM=quickjs. Details inline at primitiveOf.
Also verified/confirmed here:
- Wire-format parity design is right and the reducer-key exhaustiveness pinning is good; side-effect-free classification holds (boot-sampled brands, captured intrinsics, descriptor reads; the spoofed-brand test passes). One wording nit:
getdoes invoke own getters (parity with the hardened node codec), so "side-effect-free" is true of classification, not extraction. - PRNG determinism on the non-snapshot path is clean — the host-side
monotonicFactory(() => rng())shares the one seeded instance with VMMath.random, and the interleaved draw sequence matches the old in-VM factory. - devalue 5.9.0 and quickjs-wasi 3.3.0 are stock npm, unpatched, and past the 48 h
minimumReleaseAge— installs are green; no dependency risk found. - Merge order:
git merge-treevsquickjs-vm-threshold-snapshotsreports no textual conflict, so #3048 → #3049 → this → rebase {#3250 → #3251} is mechanically safe and matches the PR body. The cost is all semantic — three silent breaks for the snapshot rebase: (1)__generateUlidis registered inline (the snapshot branch's own rule: host callbacks never inline — it won't be re-registered on restore); (2) the restore path's "serde survives in the heap" comment becomes false —createQuickJSSerde(vm)must run on restore; (3) the ULID monotonic factory's internal state is host-side now, so it's not captured by the snapshot and therngDrawsfast-forward lands on the wrong draw position. (The deletedvm-bundle-entry.tsdocumented the late-binding factory as the affordance for exactly this.) - Byte cache: never evicted (grows for the VM's lifetime); the suspend path passes
ensurePendingByteCache(vm)butcollectDrainOperationspasses no cache, so the samecorrelationId:fieldcan serialize twice on different paths; andglobalThis.__rawFieldsisn't cleaned up in afinally, so a throw mid-dumpPendingOpsleaves it observable to workflow code. - Handle lifecycle:
serialize()has no disposal sweep afterstringify— roughly one leaked handle per value node per call;shapeOfleaks the symbol-key descriptor handles; and the pointer-keyedidentitiesmap will produce false devalue ref-identity if the planned bulk-free arena ever reuses pointers (worth a comment now). - Stale references:
.github/workflows/tests.ymlstill listsvm-serde-bundle.generated.tsin the upload-artifact paths (the comment above it says to keep it aligned withturbo.json), and three comments inquickjs-entrypoint.tsstill referenceglobalThis[Symbol.for('workflow-serialize')]. - Changeset: given the wire-format work and dependency bumps,
minorfits better thanpatch(matters on astablebackport even though beta numbering ignores it).
Test-hygiene notes on the (otherwise well-built) parity suite: the side-effect test installs permanent prototype patches with no restore; dead code at the byte-compare helper; and the mid-suite serde re-creation papers over cross-test handle-state coupling rather than proving it absent.
| if (handle.isBool) return handle.toBoolean(); | ||
| if (handle.isNumber) return handle.toNumber(); | ||
| if (handle.isBigInt) return guestBigInt(handle); | ||
| return handle.toString(); |
There was a problem hiding this comment.
Blocker (CI-proven, all 15 quickjs legs): handle.toString() routes through JS_ToCString, which is NUL-terminated — any guest string containing U+0000 arrives silently truncated. This is the funnel for essentially every string crossing the boundary: primitives here, property keys, symbol descriptions, RegExp source, Headers/URL values. Worse, NUL-bearing object keys are silently dropped — the enumeration APIs (keys()/getOwnPropertyKeys()/propertyIsEnumerable()) truncate the key, the descriptor re-lookup misses, and the property vanishes (devl[{}]). And a second, independent corruption class rides the same read: QuickJS stores WTF-8, so one lone surrogate arrives as three U+FFFD (the reference codec yields one).
A host-side fix exists and is validated (branch pgp/quickjs-host-serde-fix) — no quickjs-wasi release needed:
guestString(): fast-pathtoString(), validated against the guest string's own.length(a plain data property, no guest code; truncation strictly shortens and the 1→3 surrogate expansion also mismatches, so equal lengths prove exactness). On mismatch, re-read via boot-capturedJSON.stringify— QuickJS implements well-formed stringify, so NULs escape as\u0000and lone surrogates as\ud800, and hostJSON.parserevives both byte-exactly (verified empirically for both classes).shapeOf(): enumerate string keys inside the VM via boot-capturedObject.keys(same set/order as the host-side iteration it replaces), read throughguestString.- Verified against 3.3.0:
newString(host→VM) andgetOwnPropertyDescriptor(string)are length-safe — only the read direction and enumeration need this.
Parity cases added: NUL in value, NUL in object key+value, NUL in RegExp source, lone surrogate, astral pair. nullByteWorkflow e2e passes with the fix.
| const shape = reduceErrorShape(value) as Record<string, unknown>; | ||
| // retryAfter is a guest Date (or string/number); normalize to an epoch | ||
| // timestamp exactly like the in-VM reducer. | ||
| let retryAfter = Date.now() + 1000; |
There was a problem hiding this comment.
Non-deterministic value written into the event log: the Date.now() + 1000 fallback when retryAfter is absent means a replay produces different bytes than the original serialization. Everything else in the error family (cause chains, registry subclasses) round-trips correctly — this is the one spot.
| return { | ||
| reducerKeys: Object.keys(reducers), | ||
| reviverKeys: Object.keys(revivers), | ||
| serialize(value: JSValueHandle): Uint8Array { |
There was a problem hiding this comment.
No disposal sweep after stringify — reducers create handle leaves (e.g. via dup() in collect) that are never freed: roughly one leaked handle per value node, per serialize call, unbounded within a VM's lifetime. Not a crash, but with #3049's long-lived sessions it accumulates across the whole inline batch. Eager finally disposal (or the bulk-free arena flagged in quickjs-wasi#26) closes it.
| * its bytes are computed once even though the op is re-collected on every | ||
| * suspension it stays pending through. | ||
| */ | ||
| const pendingByteCache = new WeakMap<QuickJS, Map<string, Uint8Array>>(); |
There was a problem hiding this comment.
Byte-cache notes: (1) never evicted — keys are correlationId:field, so a long run grows this monotonically for the VM's lifetime; (2) the suspend path passes the cache but collectDrainOperations doesn't, so one op can serialize twice on different paths — with any getter re-invocation that's two different byte sequences for what the log treats as one value; (3) identities in the serde is keyed on raw pointer and only cleared at dispose — safe today, but a future bulk-free arena reusing pointers would produce false devalue ref identity. Worth comments/guards now.
|
Fix branch pushed: |
Carries the #3049 merge (and through it main/#3048) into the host-serde engine. The merge was textually clean but needed one semantic adaptation: the live-feed terminal buffer added on the perf branch (__terminalBuffer / __registerResolver) originally buffered raw bytes and deserialized them in-guest via Symbol.for('workflow-deserialize') — a global this branch retires along with the VM serde bundle. The buffer now stores host-deserialized VM values instead: the no-resolver branches of step_completed / step_failed run the same serde.deserialize() path as their resolver branches and buffer the resulting value ('resolve_value' / 'reject_value'), so draining at promise construction only forwards it. Validated: core 1932 passed; hook (26/26), promiseRace and fail e2e green under WORKFLOW_VM=quickjs.
Summary
Moves the QuickJS engine's serialization entirely to the host, operating on
JSValueHandles — the serde bundle previously bundled by esbuild and evaluated inside the VM is gone. This mirrors the node:vm engine's architecture (the serializer is host code reaching into the sandbox realm) and is the QuickJS counterpart to #3257's side-effect-free serialization for node:vm.Built on:
classIdbrand checks,identity, descriptor reads,vm.construct, ephemeral functions; see feat: trap-free introspection primitives (isProxy, brand checks, descriptor reads) vercel-labs/quickjs-wasi#24/test: POC for host-side devalue serialization of guest values vercel-labs/quickjs-wasi#26)operationsfor stringify/parse (feat: pluggable operations for stringify sveltejs/devalue#172, feat: pluggable operations for parse sveltejs/devalue#173)Architecture
runtime/quickjs-serde.tsimplements the workflow wire codec over handles:JSValueHandleand falls back todefaultStringifyOperationsfor host values. Parse operations are handle-only — every revived value is built inside the VM through boot-captured constructors.classIdmap,isError,isProxy) — neverinstanceoforSymbol.toStringTag; extraction through boot-captured intrinsics invoked with explicit receivers; property access through descriptors. Patched prototypes and spoofed brands can no longer perturb serialization (aSymbol.toStringTag: 'Date'spoof serializes as the plain object it is — the previous codec crashed on that input). The only guest code executed is what the contract always executed:WORKFLOW_SERIALIZE/WORKFLOW_DESERIALIZE,__closureVarsFn,WORKFLOW_USE_STEPon revival.Math.random, so the interleaved draw sequence — and every generated ID — is identical to the in-VM factory's. Existing runs replay byte-for-byte.Wire-format parity
Event logs persist across SDK versions, so parity with the previous in-VM codec is load-bearing (old runs must replay; node-engine steps must read VM-serialized inputs). The old codec's value-space implementation is retained as
serialization/workflow-vm.ts(host reference codec) and a new parity suite byte-compares against it in both directions: 42 tests covering primitives/bigints/-0/NaN, containers, typed arrays/views/buffers, the full Error family with cause chains, shared refs + cycles, null-proto objects, boxed primitives, step-function proxies (closure vars + bound this), workflow refs, symbol-stamped stream handles, registry class instances, and patched-prototype/spoof resistance.Removed
scripts/build-vm-serde-bundle.js, the generatedvm-serde-bundle.generated.ts,serialization/vm-bundle-entry.ts, and the bundle eval in VM init.Testing
@workflow/coresuite: 1757 passed | 3 expected failWORKFLOW_VM=quickjs, dev server): hooks 26✓, AbortController 26✓, streams 7✓, retries/errors 7✓, instance methods ✓, recursivestart()(fibonacci) ✓Follow-up candidates