Preload workflow step bundle lazily#2517
Conversation
|
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
TooTallNate
force-pushed
the
nate/lazy-step-bundle-preload
branch
from
3ced86c to
5c9d38e
Compare
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Review — lazy step-bundle preload
Nice, well-scoped optimization. I traced the core correctness concern thoroughly and it holds up: the step/serde registrations moved into __workflow_steps.js are only ever consumed on the host step-execution path, and both executeStep call sites now await ensureStepBundleLoaded() first.
Why deferring the imports is safe (the part I most wanted to verify):
- The only host consumers of the step registry (
getStepFunction) and class registry (getSerializationClass) are reached viahydrateStepArguments/getStepRevivers/getClassRevivers— all insideexecuteStep, andgetReadable()is itself a"use step". Nothing outside step execution touches them. workflow/internal/builtinsonly defines"use step"functions, so it's safe to defer alongside user steps — builtin steps also dispatch throughexecuteStep.- Crucially, the workflow-VM replay uses a separate, per-VM class registry (
getRegistry(global)keyed on the VM global, populated by the compiledworkflowVMCode), so VM replay never depended on the host route's top-level imports — before or after this PR. So replaying an event log that contains class-typed step results in a step-less invocation does not regress.
The three preload entry points (bg-step at the top, run_started, and the lazy fallback inside ensureStepBundleLoaded) all converge correctly, and ??= + the generated route's own memoization make double-loads harmless. The two new runtime tests (no-step preload rejection is swallowed; parallel preload awaited before inline step_started) are well-constructed and exercise the real microtask ordering.
Required before merge:
- Missing changeset. This touches
@workflow/coreand@workflow/next;pnpm changeset status --since=mainreportschangeset needed. Per repo policy every PR needs one (patchfor both packages here). (.changeset/lazy-next-step-imports.mdis a different, already-merged PR.)
For reviewer awareness (intentional, not a bug): this is a behavior change in failure mode. Previously a broken step bundle failed the route module import → every invocation 500'd. Now a step-bundle load failure only surfaces on invocations that actually execute a step; pure-replay/suspend invocations succeed. The new test codifies this. Worth a line in the PR description.
CI: the failing Benchmark Vercel (express) / (nitro-v3) checks are bench.bench.ts timeouts against deployed apps on frameworks that use the unchanged base-builder and pass no preloadStepBundle (runtime change is a no-op for them) — unrelated/flaky. Unit Tests + nextjs-turbopack benchmark + all dev/postgres E2E pass.
Two minor inline notes below.
| const workflowEntrypointOptionsCode = | ||
| createWorkflowEntrypointOptionsCode(); | ||
| const workflowEntrypointOptionsObjectCode = workflowEntrypointOptionsCode | ||
| ? workflowEntrypointOptionsCode.replace(/^,\s*/, '') |
There was a problem hiding this comment.
Minor (maintainability): this couples the generated route to the exact , { ... } string shape returned by createWorkflowEntrypointOptionsCode() — stripping the leading comma so it can be spread into the new options object. It works today (the helper returns either '' or , { namespace: ... }), but if that helper's format ever changes (e.g. different spacing, or a non-leading-comma form) this silently emits broken route code with no type/test guard.
Consider either a short comment documenting the ^, dependency here, or having createWorkflowEntrypointOptionsCode() expose an object-literal variant (e.g. createWorkflowEntrypointOptionsObjectCode()) so both call sites share one source of truth. The : '{}' fallback + ...{} spread is harmless as-is.
| .then(() => undefined); | ||
| // The preload is speculative. Surface failures only if this request | ||
| // actually reaches a step execution path and awaits the promise. | ||
| stepBundlePreload.catch(() => {}); |
There was a problem hiding this comment.
Confirming intent (looks correct): ensureStepBundleLoaded() returns stepBundlePreload itself, so a genuine load failure still rejects and surfaces at the await before executeStep. This .catch(() => {}) is a separate subscription that only suppresses the unhandled-rejection warning for the speculative case where no step runs — good.
Nit: it re-attaches a fresh no-op handler on every startStepBundlePreload() call (each call after the first allocates another promise via .catch). Since the goal is one-time suppression, moving it inside the ??= initializer attaches it exactly once and reads more clearly:
stepBundlePreload ??= Promise.resolve()
.then(() => options.preloadStepBundle?.())
.then(() => undefined);
stepBundlePreload.catch(() => {}); // <- only when first createdPurely cosmetic; no behavioral change.
Follow-up: correctness deep-dive + isolated cold-start numbersDid a deeper correctness pass on the lazy-loading and measured the cold-start delta locally, since the CI benchmark numbers are end-to-end (noisy for cold start). Correctness — loads steps exactly when needed ✅The safety of the change rests on one invariant, which holds on every path I traced:
Every registry consumer is gated:
Workflow-VM replay is safe (the subtle part): the class registry is per-global ( Edge cases verified: retry/throttle re-deliveries (carry One intentional behavior change to call out: a broken step bundle previously failed the route module import (every invocation 500'd); now it only surfaces on step-executing invocations. More resilient, but worth a line in the PR description. Cold start — improves, with nuance
Isolated measurement (nextjs-turbopack workbench, fresh Node process, n=6)~30 ms of cold module-init is moved off the no-step critical path (and overlapped on the step path) for this AI-heavy app. Caveats: conservative lower bound (compiled user-workflow modules add more), measured on a fast local CPU (cold serverless is typically 2–5× slower), and the saving scales with the app's step-dependency weight — heavier step deps → larger win; a trivial app with cheap steps → negligible. Confirmed via the build output: the generated route no longer statically imports steps; the built Local verification
Outstanding
|
2 participants
Summary
__workflow_steps.jsmodule instead of top-level flow route imports.preloadStepBundleruntime hook that starts step bundle loading afterrun_startedor immediately for background step deliveries, then awaits it only before step execution.Testing
pnpm vitest run packages/core/src/runtime.test.tspnpm vitest run packages/next/src/builder-deferred.test.tsgit diff --checkNotes
pnpm --filter @workflow/core typecheckandpnpm --filter @workflow/next build; both currently fail on existing unrelated TypeScript issues instep-executor.tsand olderbuilder-deferred.tscall sites.