QuickJS engine: host-side, side-effect-free serialization via handles - #3263
Conversation
🦋 Changeset detectedLatest commit: 5e40673 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✅ All tests passed 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.
|
… assertions (analytics listing can omit attempt entirely)
…ed (encp) hook payloads open Main's sealed-box work (#3096) makes cross-deployment resumeHook() seal hook payloads to the target run's published X25519 public key. The shared start() path publishes that key regardless of engine, so QuickJS runs receive sealed payloads too — but the QuickJS entrypoint resolved only the bare symmetric key via importKey(), which cannot open encp envelopes. The first sealed hook payload wedged the run right after hook_received, timing out every hook/webhook e2e on Vercel prod (node:vm legs were fine — the node engine resolves the full capability via memoizeEncryptionKey). Resolve deriveRunPayloadKeys() in the entrypoint instead and widen the runtime's key types from CryptoKey to DecryptionKey. Writes stay symmetric (encrypt() with RunPayloadKeys takes the encr path). Regression test seals a payload exactly as resumeHook does and round-trips it through the VM.
…import, VM-leak guard, telemetry namespace, eval-string escaping - Deterministic crypto.getRandomValues/randomUUID in the VM bootstrap, drawing from the seeded Math.random (identical sequences to the node engine's vm/index.ts implementations); all crypto.subtle methods throw with step-function guidance. process.env exposed as a frozen copy, matching node. - Intl: throwing constructors (no ICU in QuickJS), and toLocale*-family methods (incl. localeCompare) throw when given an explicit locale so cross-engine divergence is loud instead of silently writing different values into the event log. No-argument forms keep working. - runtime.ts lazy-imports the QuickJS entrypoint at dispatch, keeping the ~1.3MB embedded WASM assets out of node-engine deployments. - runQuickJSWorkflow wraps the per-run phase so an exceptional exit disposes the VM instead of leaking it in a reused compute instance; corrected the misleading fail-loud comment (run_failed, not retry); warn when the event drain loop exhausts its iteration bound. - Telemetry attributes renamed quickjs.* → workflow.vm.* to stay in the file's workflow.* namespace. - Eval-string correlation-id interpolation uses JSON.stringify instead of quote-only escaping. - common-vm.test.ts pins the reducer/reviver superset invariant against common.ts so the duplicated sets can't silently drift. - Docs enumerate the remaining global-surface differences (subtle.digest, Intl, WebAssembly, Atomics); quickjs-entrypoint documents the known precondition-guard gap.
…tion + resumeId dedup) #1834 made resumeHook() fall back to enqueueing the run with a hookInput payload when the direct hook_received write fails transiently, with the runtime materializing the missing event on delivery. Only the node:vm path implemented it — the QuickJS dispatch returned before the node block, so the resilient payload was silently dropped and the new e2e timed out on every quickjs leg. - runtime.ts threads hookInput into runWorkflowWithQuickJS; the entrypoint materializes the missing hook_received after loading the event log (resumeId-keyed dedup, occurredAt from the resumeId ULID, local eventData substitution for lazy/ref responses, EntityConflict / HookNotFound handling) — mirroring the node block. - processEvents drops duplicate hook_received rows sharing a resumeId (first-in-log wins), matching the node engine's EventsConsumer dedup; the seen-set lives in the VM heap so it is deterministic per replay. Verified against the dev server with WORKFLOW_VM=quickjs: the resilient resume e2e passes and the materialization is observable in the logs; all 27 hook e2e tests green.
… WASM module caching
…loop event ceiling - Inline steps now claim via a lazy step_started carrying the input (step_created deferred, atomic create-claim in the world), with ownerMessageId stamped and authoritativeAttempt=1 — a concurrent invocation racing on the same fresh step loses with EntityConflictError and skips instead of both bare-starting the step and double-running the body. This also removes the stepsCreatedByUs set, whose 'created by us' invariant didn't survive the swallowed create-race conflict; redelivery backstops now key on hasCreatedEvent. - dispatchPendingOps' createdAttributeEvent/createdGetConflictHook signals are consumed again: when the loop exits suspended without ever reading back a self-written attr_set / getConflict hook_created (eventually-consistent listing lag), the entrypoint requeues immediately instead of parking the run awaiting_external with its unblocking event already written. - The server-supplied event ceiling is re-checked at the top of every continuation-loop turn (seenEventIds.size), so a single invocation fanning out inline can no longer grow the log arbitrarily past the operator's limit. The quickjs dispatch in runtime.ts converts MaxEventsExceededError into run_failed / MAX_EVENTS_EXCEEDED — the guard's throw previously nacked forever, parking runaway runs in 'running'. - Documented the deliberate decision that the platform function timeout is the only bound on inline chaining (budget parked per batch), matching the node engine.
(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.
There was a problem hiding this comment.
Fixed in f88165c. Confirmed empirically: toString() on "ab\u0000cd" returns "ab", and NUL-bearing keys truncate in the enumeration APIs (dropping when the truncated name fails the enumerability probe, colliding when a sibling shares it). The fix funnels every guest→host string through guestString() — truncation is detected by comparing against handle.length (the true guest length, so NUL-free strings pay only a property read) and recovered via in-VM JSON.stringify escaping, whose output is NUL-free by construction. Key enumeration verifies the fast host-string path against a guest Object.keys count + duplicate check and re-extracts through length-aware key handles on mismatch; get/hasOwn route NUL-bearing keys through handle-keyed access (vm.newString is length-aware, verified). The parse/build side was already safe (define() goes through guest key handles). Regression tests cover values (leading/trailing/middle/multi NUL), byte parity with the reference codec, and both enumeration corruption shapes; nullByteWorkflow passes under WORKFLOW_VM=quickjs locally.
| 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.
There was a problem hiding this comment.
Fixed in f88165c — the fallback now reads the GUEST clock (a captured Date.now, which sits on the deterministic replay clock at the WASI layer) instead of the host wall clock. The in-VM reducer's Date.now() was replay-stable by construction; the host port silently swapped it for wall time — this restores the original semantics rather than changing the wire shape (node's reducer always emits a number here, so omitting the field would have been a cross-engine format change).
| 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.
There was a problem hiding this comment.
Fixed in f88165c — serialize and deserialize now sweep every intermediate handle their pass creates (call/invoke results, descriptor reads, dups, parse-op constructions; deserialize escapes the root). One finding worth flagging: the obvious implementation — vm.withScope — is UNSAFE here. The library scope registers every handle constructed while active, including the ones handleHostCall wraps around C-owned argv pointers when a host callback runs (our Map/Set/Headers forEach visitors, mid-pass). Disposing those double-frees guest values; observed as WASM memory access out of bounds in the parity suite. The sweep therefore uses module-owned tracking fed only by this module's creation funnels, which is safe by construction. Filed as a follow-up note on the PR for an upstream fix (suspend the active scope in the trampoline, as getPromiseThen already does). identities is also cleared per pass now — with handles being freed, pointer reuse across passes could otherwise alias stale identity entries (your point 3 on the byte-cache thread).
There was a problem hiding this comment.
Follow-up: the library-side fix is now up as vercel-labs/quickjs-wasi#31 — host-callback this/argument handles become 'borrowed' (never scope-tracked, dispose() no-op, dup() for retention), with regression tests covering the exact forEach-visitor corruption shape found here. Once that ships, vm.withScope becomes a valid alternative to this PR's module-owned tracking; the tracking approach stays correct either way (it's a strict subset — only handles this module creates), so no change needed here.
| * 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.
There was a problem hiding this comment.
Fixed in f88165c — all three points: (1) eviction — the collection pass now surfaces settled ops (created + resolver-less + no abort in flight, which neither the suspension nor the drain filter can ever match again) in the same guest sweep, and their cache entries are dropped, bounding the cache by the live pending set; (2) collectDrainOperations now shares the per-VM cache, so an op serialized on the suspension path reuses those exact bytes at terminal drain instead of risking a second, getter-divergent byte sequence; (3) identities is now cleared per serialize pass (it only needs intra-pass stability), which makes it immune to pointer reuse — necessary anyway now that the pass-disposal sweep actually frees handles (see the :1967 thread).
|
Fix branch pushed: |
…hreads Merge resolution — main's #3048 finals carried into the inline-loop architecture: - namespace + run-origin nextTraceCarrier threaded through runWorkflowWithQuickJS into every publish (step handoffs, hook_conflict requeue, wait continuations, immediate requeues) - suspended-exit requeues converted to FRESH messages (never { timeoutSeconds } visibility-redelivery of the current message — the hookInput redelivery trap fixed on #3048); exit wait sweep enqueues the continuation for the soonest unscheduled wait directly - entrypoint-side hookInput materialization dropped in favor of main's engine-agnostic prologue re-ensure in runtime.ts (with #3230's (runId, resumeId) claim protocol); dispatch stays inside the replay loop's try so engine failures classify into run_failed - interrupt handler keeps the perf branch's per-burst mutable budget, with main's configurable getReplayTimeoutMs() as the ceiling Review fixes (PR #3049 threads): - CRITICAL overflow wedge: overflow steps are handed to the queue in the same turn their step_created is written, BEFORE the event feed — the feed always observes those writes and continued the loop, so the old handoff was unreachable on the only turn that classified the steps as fresh (the cause of promiseRaceStressTestWorkflow hanging in the quickjs CI legs) - backstop gating: the deliveryAttempt > 1 gate (common case on worlds that advance attempts on routine redeliveries) is replaced with the node engine's ownership decision table — lease-active steps owned by another message arm a DELAYED backstop for the lease remainder under an epoch-scoped key; owner redeliveries and expired/unstamped steps dispatch immediately under the bare-correlationId key. Ownership is derived host-side from observed step_started/step_retrying events - ack-without-requeue: inline step terminals the feed has not surfaced raise the requeue signal, so the loop never acks with durably written terminals and nothing scheduled to consume them - idempotency keys bucketed by purpose (dispatch / backstop:<epoch> / retry:<n>) so worlds that retire used keys cannot swallow a later publish for the same step - live-feed terminal buffering: step/wait/attr terminals arriving before this VM constructs the corresponding resolver are buffered (__terminalBuffer, mirroring __hookPayloadBuffer) and settle the promise at construction — the single-scan continuation path previously dropped them and the await never settled Validated: core 1888 passed, full e2e 136/136 under WORKFLOW_VM=quickjs (nextjs-turbopack dev, world-local).
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.
Serde performance: host-side (this PR) vs in-VM bundle (base)Benchmarked the question of whether host-side serde pays for its repeated WASM-boundary hops vs running devalue entirely inside the VM. It's the opposite: host-side serde is faster across every payload shape tested — 1.8× to 105×. Method: identical script on both branches' built dists — a 10-step workflow piping a payload step→step through
Why the intuition inverts: the boundary hops are cheap C calls per value node, but the in-VM approach runs the entire devalue codec as interpreted JS inside an interpreted VM — QuickJS-on-WASM executes JS roughly two orders of magnitude slower than V8's JIT, and every payload byte additionally round-trips through VM-heap Uint8Arrays and in-VM UTF-8 string handling. Host-side serde does the codec work at native V8 speed and only crosses the boundary to construct/read the final value graph. The 512 KB string is the purest illustration: one Two secondary effects worth noting:
So this PR is a performance improvement in addition to its correctness/hardening goals. Bench script available on request. |
Resolution: host-serde adaptations kept for all serde-threading and terminal-buffer conflicts (value-kind buffering via host deserialize); main's sleepWinsRace wait-continuation fix taken in the entrypoint scheduling sweep.
… pass-scoped handle disposal, byte-cache lifecycle - NUL (U+0000) safety across the WASM boundary: handle.toString() routes through JS_ToCString and silently truncates at the first NUL, and the C-string key APIs mangle NUL-bearing property keys (drop or collide). guestString() detects truncation by comparing against the handle's true guest length and recovers via in-VM JSON.stringify escaping; shapeOf verifies its fast host-string key list against a guest Object.keys count (+ duplicate check) and re-extracts through key handles on mismatch; get/hasOwn route NUL-bearing keys through length-aware guest string handles. All string funnels (primitives, symbol descriptions, error fields via chained/own reads, Headers entries, RegExp source/flags, URL href) go through guestString. Regression-tested down to the truncate-vs-collide enumeration shapes; fixes nullByteWorkflow on the quickjs e2e legs. - RetryableError's absent/invalid retryAfter fallback now reads the GUEST clock (the deterministic replay clock at the WASI layer) via a captured Date.now instead of the host wall clock — the in-VM reducer was replay-stable by construction and the host port silently lost that. - Pass-scoped handle disposal: serialize/deserialize sweep every intermediate handle their pass creates (call/invoke results, descriptor reads, dups, parse-op constructions), closing the ~one-leaked-handle-per-value-node growth across long-lived inline sessions. Implemented with module-owned tracking rather than vm.withScope: the library scope also captures the handles the host-callback trampoline wraps around C-owned argv pointers, and disposing those (Map/Set/Headers forEach visitors run mid-pass) double-frees guest values — observed as WASM memory corruption. identities is cleared per pass so freed-pointer reuse cannot alias entries across passes. - Byte-cache lifecycle: terminal drain now shares the per-VM cache with the suspension path (re-serializing an op at drain could re-invoke getters and produce different bytes for what the log treats as one value), and entries for settled ops — which neither collection filter can match again — are evicted, bounding the cache by the live pending set.
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 170730ms → this run 136530ms (Δ -34200ms, -20%) 1020 steps (queue-hop) Cumulative STSO time: 3183ms over 1 samples No 📜 Previous results (3)d101fa2Wed, 05 Aug 2026 22:30:33 GMT · run logs
ef693e3Tue, 04 Aug 2026 22:42:22 GMT · run logs
f88165cTue, 04 Aug 2026 21:46:52 GMT · run logs
ℹ️ Metric definitions & methodologyThe collapsed STSO distribution section above buckets every step gap of the sequential-steps run (not a sampled window), split by whether the step ending the gap ran inline — in the same warm process as the step before it, so the gap is pure framework overhead — or after a queue-hop — the first step of a fresh process, which pays queue dispatch, client reinit and event-log replay. Bars overlay the two runs: Best/P75/P90/P99 deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body (in-deployment start() → first step body, deployment clocks) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · SL: stream latency (in-deployment write → read propagation, readAt - writtenAt) · SO: stream overhead (end-to-end write+consume time beyond the modelled generation window) Scenarios — step: one trivial no-op step, no stream; no hooks, so the run stays in turbo mode (in-process fast path) · stream: one streaming step; no hooks, so the run stays in turbo mode (in-process fast path) · hook + stream: registers a hook before one step, which exits turbo mode (dispatch path) · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges, and WO is the whole-run overhead outside step bodies · stream latency: parallel reader/writer steps on a dedicated stream; SL is the in-deployment write->read propagation (readAt - writtenAt) · stream overhead (text): writer streams 300 variable-length text token deltas paced at 100/s for 3s (a haiku-size LLM's token throughput) while a parallel reader drains the whole stream; SO is the end-to-end write+consume time beyond the 3s generation window (overhead/backpressure) · stream overhead (structured): same workload as stream overhead (text), but each delta is an AI-SDK-style structured object ({ type: 'text-delta', id, text }) instead of a raw string, so the SO gap vs the text scenario is the added serialization cost 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · SO 250/500/1000 All metrics are measured from deployment-side timestamps only. Runs are triggered by an in-deployment route that stamps the anchor ( Cold starts are kept in the numbers on purpose — they are part of real bursty-workload latency. The workbench deployment cold-starts the |
… accounting, loud unregistered-callback failures 3.3.1 ships the three fixes this branch surfaced upstream: - Borrowed host-callback handles (vercel-labs/quickjs-wasi#31): the trampoline's this/argv handles are scope-exempt, making vm.withScope safe around host callbacks. The serde's module-owned pass-disposal apparatus (passDisposal/track/runWithPassDisposal and ~18 track() wraps) is replaced by withScope in serialize/deserialize — simpler, and strictly more complete: every handle constructed during the pass is swept, not just the ones our creation funnels saw. Bench parity confirmed (within ~10% on the 50k-node extreme case, unchanged elsewhere; still 2.6-100x over the in-VM codec). - Real memoryLimit accounting (vercel-labs/quickjs-wasi#33): the engine's 256 MB VM ceiling now actually bounds retained guest allocations (usable-size was 0 on wasm32-wasi before, so the limit never accumulated). - Unregistered host callbacks throw (vercel-labs/quickjs-wasi#34): guest calls into missing callbacks fail loud instead of silently returning undefined — protection this engine wants for snapshot-restore re-registration bugs. Also merges origin/main (undici 7.29.0).
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Focused review: two inline findings—one reproducible serialization correctness blocker and one portability concern.
| */ | ||
| const guestString = (handle: JSValueHandle): string => { | ||
| const fast = handle.toString(); | ||
| if (fast.length === handle.length) return fast; |
There was a problem hiding this comment.
[P1 blocker] The length equality is not sufficient to prove that JS_ToCString was lossless because its two known corruptions can cancel each other out. I reproduced this with a focused parity test using the guest string "\ud800\u0000a": the lone surrogate expands to three U+FFFD characters while the NUL truncates the two-code-unit suffix, so both fast.length and handle.length are 3 and this returns the corrupt fast value. The durable bytes were devl["���"]; the reference codec produced devl["�\u0000a"]. All existing 49 serde tests passed while this added case failed. Please make the fallback trigger on length mismatch or a U+FFFD in the fast result (legitimate replacement characters can safely take the slow path), and add mixed lone-surrogate+NUL cases for values and keys.
There was a problem hiding this comment.
Excellent catch — fixed in d101fa2, and your reproduction understated it slightly: probing this build showed a BARE lone surrogate corrupts with matching lengths (1:1 U+FFFD replacement, no NUL needed), and the JSON.stringify slow path was itself lossy for lone surrogates — QuickJS passes them through raw, and the C-string extraction of its output corrupts them. So the fix is two-part: (1) the fast value is accepted only on length match AND no U+FFFD (legit-U+FFFD strings take the loss-free slow path, per your suggestion); (2) the slow path now escapes INSIDE the VM to printable ASCII via a new captured escapeString intrinsic (per-code-unit \uXXXX on WTF-16, so lone surrogates survive) and JSON-parses host-side. The key-enumeration guard gains the same U+FFFD scan (lone-surrogate keys corrupt with count and uniqueness intact), and get/hasOwn route handle-keyed for keys carrying a NUL or UNPAIRED surrogate (vm.newString verified WTF-16-preserving; paired surrogates encode fine). Tests added: your exact canceling case, bare surrogates, legit-U+FFFD passthrough, reference-codec byte parity, and surrogate/mixed keys.
|
|
||
| function bytesToBase64(bytes: Uint8Array): string { | ||
| if (bytes.length === 0) return '.'; | ||
| return Buffer.from(bytes.buffer, bytes.byteOffset, bytes.byteLength).toString( |
There was a problem hiding this comment.
[P2; blocker if WASM-only/non-Node hosts are in scope] This adds an unconditional Node Buffer dependency to every binary serde path. In Cloudflare Workers and similar runtimes, typed-array/ArrayBuffer serialization now throws unless the bundler injects a Buffer polyfill. Main already has one Buffer.isBuffer() diagnostic on QuickJS replay, so there is existing portability debt, but this PR expands it into the codec itself and conflicts with the QuickJS engine's WASM-only portability goal. Please use the portable strategy used elsewhere: native Uint8Array base64 methods when available, Buffer only when Node is detected, and btoa/atob fallback.
There was a problem hiding this comment.
Fixed in d101fa2 — the base64 helpers are now feature-detected exactly as you suggested: Uint8Array.fromBase64/toBase64 when available, Buffer when present, btoa/atob loop otherwise. No unconditional Node dependency remains in the codec (the one pre-existing Buffer.isBuffer diagnostic on the replay path is debug-log-only and untouched here). The wire-parity suite pins that all three paths produce identical bytes on this runtime's active path.
…ase64 P1 — the guestString length check was insufficient: JS_ToCString has TWO corruptions (NUL truncation, lone-surrogate -> U+FFFD replacement) and they can cancel — the replacement expansion offsets the truncation so the extracted length matches the true guest length. A bare lone surrogate can also replace 1:1 with no length change at all. Worse, the JSON.stringify slow path was itself lossy for lone surrogates: QuickJS passes them through raw, and the C-string extraction of ITS output corrupts them. - guestString accepts the fast value only when length matches AND it contains no U+FFFD (legitimate U+FFFD strings take the loss-free slow path); the slow path now escapes INSIDE the VM to printable ASCII via a new captured escapeString intrinsic (WTF-16-safe per-code-unit \uXXXX escaping), then JSON-parses host-side. - shapeOf's fast-key acceptance adds a U+FFFD scan alongside the count/duplicate checks (lone-surrogate keys corrupt with count and uniqueness intact). - get()/hasOwn() route keys through guest string handles when they carry a NUL or an UNPAIRED surrogate (paired surrogates - emoji keys - encode fine through the C-string APIs; vm.newString is verified WTF-16-preserving for the handle path). - Tests: the reviewer's exact length-canceling case, bare lone surrogates, legit-U+FFFD passthrough, byte parity with the reference codec, and lone-surrogate/mixed keys. P2 — the codec's base64 helpers no longer carry an unconditional Node Buffer dependency: feature-detected Uint8Array.fromBase64/toBase64 when available, Buffer when present, btoa/atob loop otherwise — keeping WASM-only/non-Node hosts (Cloudflare Workers) viable.
3.4.0 ships lossless string transport (vercel-labs/quickjs-wasi#35 — found by this PR's review cycle), so the SDK-side detection and escape machinery is deleted wholesale: - guestString (length + U+FFFD detection, in-VM escape fallback) — plain toString() is lossless now - the escapeString / hasOwnCall / jsonStringify / objectKeys captured intrinsics - keyNeedsHandleLookup and the handle-keyed get/hasOwn routing — the library routes inexpressible keys itself - shapeOf's guest-key verification pass (count/duplicate/U+FFFD scans) — enumeration is lossless Net ~130 lines and four captured intrinsics removed; the serde now uses the plain quickjs-wasi surface everywhere. Test honesty fix that 3.4.0 forced: the earlier lone-surrogate round-trip tests passed only via mutual corruption — the pre-3.4.0 lossy host→guest transport corrupted the guest comparison literals identically to the wire. With an honest transport they exposed that the WIRE itself (devalue emits lone surrogates raw; the wire is UTF-8) degrades lone surrogates to U+FFFD — in the node engine's reference codec exactly as here, verified. Bug-compatible parity is the load-bearing property (event logs replay across engines), so those tests now assert byte parity with the reference codec plus guest-observed equality with the reference codec's own round trip; NULs are devalue-escaped and asserted to survive exactly. Wire-level surrogate preservation is a product-wide devalue/UTF-8 question, tracked separately from this engine.
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
ai review: Re-checked the latest head, lossless QuickJS string transport, portable base64 fallbacks, regression coverage, and current CI. No remaining code-review blockers found.
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 quickjs-wasi 3.3.0's host-side introspection primitives (
classIdbrand checks,identity, descriptor reads,vm.construct, ephemeral functions; see vercel-labs/quickjs-wasi#24/vercel-labs/quickjs-wasi#26) and devalue's pluggable stringify/parseoperations(already onmainvia #3257's devalue 5.9 bump).Performance
Counter-intuitively, host-side serde is faster across every payload shape — the boundary hops are cheap C calls, while the old approach ran the whole codec as interpreted JS inside an interpreted VM. Full-replay wall time (10-step payload-piping workflow, median of 7, details in this comment):
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.JS_ToCString-backed extraction truncates at the first NUL and mangles NUL-bearing property keys.guestString()detects truncation against the handle's true guest length and recovers via in-VMJSON.stringifyescaping; key enumeration verifies its fast path against a guestObject.keyscount and re-extracts through length-aware key handles on mismatch; NUL-bearing keys route through handle-keyed get/set.Math.random, so the interleaved draw sequence — and every generated ID — is identical to the in-VM factory's.RetryableError'sretryAfterfallback reads the guest's deterministic replay clock, not the host wall clock. Existing runs replay byte-for-byte.vm.withScope(requires quickjs-wasi ≥ 3.3.1, whose borrowed-handle fix — fix: exempt host-callback this/argument handles from scopes and dispose() vercel-labs/quickjs-wasi#31, found by this PR — makes scopes safe around host callbacks).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 parity suite byte-compares against it in both directions — 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, NUL-bearing strings/keys (including the truncate-collision enumeration shape), and patched-prototype/spoof resistance. Reducer/reviver key sets are pinned againstcodec-devalue-vm's so drift fails loudly.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: 1961 passed | 3 expected failWORKFLOW_VM=quickjs, dev server):nullByteWorkflow✓, hooks 27✓, sleep/step races 3✓Follow-up candidates
Upstream— fixed in fix: exempt host-callback this/argument handles from scopes and dispose() vercel-labs/quickjs-wasi#31, released in 3.3.1, and this PR now useshandleHostCallscope-tracking of C-owned argv pointerswithScopedirectly. 3.3.1 also makesmemoryLimitreal on wasm32-wasi (fix: account allocation sizes against memoryLimit on wasm32-wasi vercel-labs/quickjs-wasi#33) and unregistered host callbacks throw (fix: throw when calling an unregistered host callback vercel-labs/quickjs-wasi#34).