feat(core): widen retained boundaries to plain data and standard built-ins - #3047
Conversation
🦋 Changeset detectedLatest commit: 3f7f12f 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
|
6590434 to
1695747
Compare
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
… of primitives-only args Replaces the isPrimitiveStepArgument allowlist with the GuestCodeStats sink that dehydrateStepArguments already exposes: a boundary retains unless serializing its step inputs actually executed workflow code (getters, proxy traps, custom serializers), plus a descriptor-walk probe for a replaced Error.prepareStackTrace — the one execution path the sink cannot see, because the serializer treats V8's engine stack getter as engine-provided. Plain data and standard built-ins (Map, Set, Date, RegExp, Error, typed arrays, URL, Headers) now stay on the fast path, including under prototype patching and polyfills, since serialization reads them through captured intrinsics.
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed at head as the incremental diff over #3046 (+312/−80).
The #3288 dependency is satisfied — but verify via rebase before merging. This branch predates #3288, so CI on the current head validates the gate against the pre-#3288 sink (the one with the bound-getter reporting hole your own body says is telemetry-grade only). I closed that gap locally: test-merged this branch with post-#3288 main, full build green, complete core suite 1818 passed / 3 expected fail, and the retention-relevant files green under WORKFLOW_RETAINED_VM=0 too. recordGuestCode survived #3288's hardened.ts rewrite with a compatible signature. Still, please rebase (or merge main) so CI blesses the actual shipped combination.
Verified in the code:
- The gate is now a ground-truth report (computed after
settlePhase(ops), so the sink is final), not a prediction — the right shape. Per-step sinks merged into the batch sink, with the span-attribution rationale documented at the merge site. - Sink concurrency is safe: guest code only executes inside
withGuestCodeStats's synchronous window (save/restore around a syncfn()); the parallel step dehydrations interleave only at await points, which sit outside the sink windows. No cross-attribution. getHostClassPrototypedescriptor-read hardening +once()per reducer set: an accessor planted on a sandbox global is recorded instead of silently executed, and the read lands exactly once per pass inside the sink scope.readErrorStackis the strongest part: recording any accessor-backedstackread (not probing for a formatter) correctly captures both hazards —prepareStackTraceexecution and the workflow-visible format-and-cache — and the self-deleting-formatter test proves why presence-probing would have been insufficient.
One ask — update the PR body. It describes an older design: a replacedPrepareStackTrace probe in suspension-handler.ts that "walks the VM realm's Error chain descriptor-wise" and runs "before dehydration as well as after". None of that exists in the final code — readErrorStack in reducers/common.ts replaced it. Consequently the body's claim that Error "keeps the fast path" is wrong for the shipped code: a natural V8 error's lazy stack accessor always records, so natural Error arguments demote (only rehydrated/data-property-stack errors retain). The changeset and the runtime-tuning docs already state this correctly ("or computed an Error's stack trace"), and the tests assert it (reports unsafe for an Error argument) — it's only the body that's stale, and reviewers/archaeologists will read it.
Loop-level coverage is exactly what I'd want: built-ins retain, core-js-style polyfills retain (data-valued additions), patched toISOString retains (captured intrinsics), getter/proxy/prepareStackTrace/self-deleting-formatter demote — all with byte-identical output to cold replay.
CI green. Approving; the rebase + body refresh are pre-merge housekeeping, not design concerns.
|
No backport to This is feature work: it widens the retained-VM fast path from primitives-only to plain data and standard built-ins, replacing the retention gate with a guest-code sink, and ships as a To override, re-run the Backport to stable workflow manually via |
2 participants
Summary
Third of the 3-PR stack from #2990 — replaces #3046's primitives-only retention gate with the guest-code report from #3257's hardened serialization (now on
main; independently simplified in #3288).handleSuspensionpasses aGuestCodeStatssink intodehydrateStepArguments(the out-param feat(core): side-effect-free serialization of workflow VM values #3257 added for exactly this) and retains the boundary only whenexecutionsstays empty — i.e. serializing the step inputs provably executed no workflow code.stackgetter is treated as engine-provided by the serializer, but it calls a workflow-definedError.prepareStackTracewhen one is set. The gate walks the VM realm'sErrorchain descriptor-wise (never invoking anything) and demotes if a formatter is present.Map,Set,Date,RegExp,Error, typed arrays,URL,Headers) keep the fast path — including under prototype patching and polyfills, since serialization reads them through captured intrinsics. Getters, proxies, custom[WORKFLOW_SERIALIZE]serializers, and step-function closures demote that one boundary to ordinary replay, exactly like the pre-retention runtime.Why this is safe (one invariant): step-argument serialization runs once at suspension and never re-runs during replay, so its only hazard is executing workflow code whose side effects live on in the retained VM. There is a single serialization path shared by all modes, so bytes cannot differ by mode; the report is a ground-truth record of whether workflow code ran, not a prediction.
An earlier revision of this PR carried its own hardened-serialization implementation (
operations.ts, a devalue patch, reducer rewrites — ~2,600 lines). All of that landed via #3257 (with a follow-up simplification split out as #3288), so this PR is now just the gate: ~40 lines of runtime change plus tests.Merge order: depends on #3288
The gate trusts the guest-code sink to record every workflow-code execution.
isEngineAccessoronmainstill has the documented bound-function hole (fn.bind()stringifies as native code, so a bound getter executes with an empty report) — #3288 closes it. Land #3288 before this PR; until then the sink is telemetry-grade, not gate-grade, for bound getters specifically.Panel review (Codex gpt-5.6-sol xhigh + Claude Fable 5 xhigh) drove three fixes now included: the
Error.prepareStackTraceprobe no longer throws on a clobberedErrorbinding (it demotes; a throw here would wedge the run on redelivery), the probe runs before dehydration as well as after (a formatter that runs during a stack read can delete itself), and the reducer guards'global[name]/ctor.prototypelookups now go through descriptor reads so an accessor planted on a sandbox global is recorded instead of silently executed. The__closureVarsFnlaundering caveat (deliberate use of the internalWORKFLOW_USE_STEPsymbol) remains accepted-by-design, as documented inhardened.ts; the docs no longer claim step closures demote.Review focus
The gate computation in
suspension-handler.ts(sink +replacedPrepareStackTraceprobe). Everything else is tests and docs.Validation
Full core unit suite green: 80 files, 1806 passed, 3 expected fails. Loop-level coverage proves built-in/polyfilled/patched-prototype args retain (
vmBuilds === 1) while getter/proxy/custom-serializer/prepareStackTracecases demote (vmBuilds > 1) with byte-identical output to a cold replay.Docs Preview
WORKFLOW_RETAINED_VMStack: #3045 → #3046 → this. #2990 stays open as the reference implementation.