fix(core): isolate lifecycle reporting and drain stream work - #4216
TooTallNate merged 1 commit into
Conversation
Co-Authored-By: Nathan Rajlich <71256+TooTallNate@users.noreply.github.com>
🦋 Changeset detectedLatest commit: b506cc1 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
|
| Passed | Failed | Skipped | Total | |
|---|---|---|---|---|
| ✅ ▲ Vercel Production | 3670 | 0 | 731 | 4401 |
| ✅ 💻 Local Development | 4014 | 0 | 550 | 4564 |
| ✅ 📦 Local Production | 4014 | 0 | 550 | 4564 |
| ✅ 🐘 Local Postgres | 4014 | 0 | 550 | 4564 |
| ✅ 🪟 Windows | 324 | 0 | 2 | 326 |
| ✅ 🌐 Cross-language Conformance | 68 | 0 | 76 | 144 |
| ✅ vercel-http-transport | 825 | 0 | 153 | 978 |
| ✅ vercel-multi-region | 27 | 0 | 0 | 27 |
| ✅ vercel-ws-transport | 559 | 0 | 93 | 652 |
| Total | 17515 | 0 | 2705 | 20220 |
Details by Category
✅ ▲ Vercel Production
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ astro-node | 133 | 0 | 30 |
| ✅ astro-quickjs | 133 | 0 | 30 |
| ✅ example-node | 133 | 0 | 30 |
| ✅ example-quickjs | 133 | 0 | 30 |
| ✅ express-node | 133 | 0 | 30 |
| ✅ express-quickjs | 133 | 0 | 30 |
| ✅ fastify-node | 133 | 0 | 30 |
| ✅ fastify-quickjs | 133 | 0 | 30 |
| ✅ hono-node | 133 | 0 | 30 |
| ✅ hono-quickjs | 133 | 0 | 30 |
| ✅ nest-node | 133 | 0 | 30 |
| ✅ nest-quickjs | 133 | 0 | 30 |
| ✅ nextjs-turbopack-node | 160 | 0 | 3 |
| ✅ nextjs-turbopack-quickjs | 160 | 0 | 3 |
| ✅ nextjs-webpack-node | 160 | 0 | 3 |
| ✅ nextjs-webpack-quickjs | 160 | 0 | 3 |
| ✅ nitro-node | 133 | 0 | 30 |
| ✅ nitro-quickjs | 133 | 0 | 30 |
| ✅ nuxt-node | 133 | 0 | 30 |
| ✅ nuxt-quickjs | 133 | 0 | 30 |
| ✅ python-node | 66 | 0 | 97 |
| ✅ sveltekit-node | 152 | 0 | 11 |
| ✅ sveltekit-quickjs | 152 | 0 | 11 |
| ✅ tanstack-start-node | 133 | 0 | 30 |
| ✅ tanstack-start-quickjs | 133 | 0 | 30 |
| ✅ vite-node | 133 | 0 | 30 |
| ✅ vite-quickjs | 133 | 0 | 30 |
✅ 💻 Local Development
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ astro-stable-node | 134 | 0 | 29 |
| ✅ astro-stable-quickjs | 134 | 0 | 29 |
| ✅ express-stable-node | 134 | 0 | 29 |
| ✅ express-stable-quickjs | 134 | 0 | 29 |
| ✅ fastify-stable-node | 134 | 0 | 29 |
| ✅ fastify-stable-quickjs | 134 | 0 | 29 |
| ✅ hono-stable-node | 134 | 0 | 29 |
| ✅ hono-stable-quickjs | 134 | 0 | 29 |
| ✅ nest-stable-node | 134 | 0 | 29 |
| ✅ nest-stable-quickjs | 134 | 0 | 29 |
| ✅ nextjs-turbopack-canary-node | 162 | 0 | 1 |
| ✅ nextjs-turbopack-canary-quickjs | 162 | 0 | 1 |
| ✅ nextjs-turbopack-stable-node | 162 | 0 | 1 |
| ✅ nextjs-turbopack-stable-quickjs | 162 | 0 | 1 |
| ✅ nextjs-webpack-canary-node | 162 | 0 | 1 |
| ✅ nextjs-webpack-canary-quickjs | 162 | 0 | 1 |
| ✅ nextjs-webpack-stable-node | 162 | 0 | 1 |
| ✅ nextjs-webpack-stable-quickjs | 162 | 0 | 1 |
| ✅ nitro-stable-node | 134 | 0 | 29 |
| ✅ nitro-stable-quickjs | 134 | 0 | 29 |
| ✅ nuxt-stable-node | 134 | 0 | 29 |
| ✅ nuxt-stable-quickjs | 134 | 0 | 29 |
| ✅ sveltekit-stable-node | 153 | 0 | 10 |
| ✅ sveltekit-stable-quickjs | 153 | 0 | 10 |
| ✅ tanstack-start-node | 134 | 0 | 29 |
| ✅ tanstack-start-quickjs | 134 | 0 | 29 |
| ✅ vite-stable-node | 134 | 0 | 29 |
| ✅ vite-stable-quickjs | 134 | 0 | 29 |
✅ 📦 Local Production
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ astro-stable-node | 134 | 0 | 29 |
| ✅ astro-stable-quickjs | 134 | 0 | 29 |
| ✅ express-stable-node | 134 | 0 | 29 |
| ✅ express-stable-quickjs | 134 | 0 | 29 |
| ✅ fastify-stable-node | 134 | 0 | 29 |
| ✅ fastify-stable-quickjs | 134 | 0 | 29 |
| ✅ hono-stable-node | 134 | 0 | 29 |
| ✅ hono-stable-quickjs | 134 | 0 | 29 |
| ✅ nest-stable-node | 134 | 0 | 29 |
| ✅ nest-stable-quickjs | 134 | 0 | 29 |
| ✅ nextjs-turbopack-canary-node | 162 | 0 | 1 |
| ✅ nextjs-turbopack-canary-quickjs | 162 | 0 | 1 |
| ✅ nextjs-turbopack-stable-node | 162 | 0 | 1 |
| ✅ nextjs-turbopack-stable-quickjs | 162 | 0 | 1 |
| ✅ nextjs-webpack-canary-node | 162 | 0 | 1 |
| ✅ nextjs-webpack-canary-quickjs | 162 | 0 | 1 |
| ✅ nextjs-webpack-stable-node | 162 | 0 | 1 |
| ✅ nextjs-webpack-stable-quickjs | 162 | 0 | 1 |
| ✅ nitro-stable-node | 134 | 0 | 29 |
| ✅ nitro-stable-quickjs | 134 | 0 | 29 |
| ✅ nuxt-stable-node | 134 | 0 | 29 |
| ✅ nuxt-stable-quickjs | 134 | 0 | 29 |
| ✅ sveltekit-stable-node | 153 | 0 | 10 |
| ✅ sveltekit-stable-quickjs | 153 | 0 | 10 |
| ✅ tanstack-start-node | 134 | 0 | 29 |
| ✅ tanstack-start-quickjs | 134 | 0 | 29 |
| ✅ vite-stable-node | 134 | 0 | 29 |
| ✅ vite-stable-quickjs | 134 | 0 | 29 |
✅ 🐘 Local Postgres
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ astro-stable-node | 134 | 0 | 29 |
| ✅ astro-stable-quickjs | 134 | 0 | 29 |
| ✅ express-stable-node | 134 | 0 | 29 |
| ✅ express-stable-quickjs | 134 | 0 | 29 |
| ✅ fastify-stable-node | 134 | 0 | 29 |
| ✅ fastify-stable-quickjs | 134 | 0 | 29 |
| ✅ hono-stable-node | 134 | 0 | 29 |
| ✅ hono-stable-quickjs | 134 | 0 | 29 |
| ✅ nest-stable-node | 134 | 0 | 29 |
| ✅ nest-stable-quickjs | 134 | 0 | 29 |
| ✅ nextjs-turbopack-canary-node | 162 | 0 | 1 |
| ✅ nextjs-turbopack-canary-quickjs | 162 | 0 | 1 |
| ✅ nextjs-turbopack-stable-node | 162 | 0 | 1 |
| ✅ nextjs-turbopack-stable-quickjs | 162 | 0 | 1 |
| ✅ nextjs-webpack-canary-node | 162 | 0 | 1 |
| ✅ nextjs-webpack-canary-quickjs | 162 | 0 | 1 |
| ✅ nextjs-webpack-stable-node | 162 | 0 | 1 |
| ✅ nextjs-webpack-stable-quickjs | 162 | 0 | 1 |
| ✅ nitro-stable-node | 134 | 0 | 29 |
| ✅ nitro-stable-quickjs | 134 | 0 | 29 |
| ✅ nuxt-stable-node | 134 | 0 | 29 |
| ✅ nuxt-stable-quickjs | 134 | 0 | 29 |
| ✅ sveltekit-stable-node | 153 | 0 | 10 |
| ✅ sveltekit-stable-quickjs | 153 | 0 | 10 |
| ✅ tanstack-start-node | 134 | 0 | 29 |
| ✅ tanstack-start-quickjs | 134 | 0 | 29 |
| ✅ vite-stable-node | 134 | 0 | 29 |
| ✅ vite-stable-quickjs | 134 | 0 | 29 |
✅ 🪟 Windows
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ nextjs-turbopack-node | 162 | 0 | 1 |
| ✅ nextjs-turbopack-quickjs | 162 | 0 | 1 |
✅ 🌐 Cross-language Conformance
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ python | 68 | 0 | 76 |
✅ vercel-http-transport
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ example | 133 | 0 | 30 |
| ✅ express | 133 | 0 | 30 |
| ✅ hono | 133 | 0 | 30 |
| ✅ nextjs-turbopack | 160 | 0 | 3 |
| ✅ nitro | 133 | 0 | 30 |
| ✅ vite | 133 | 0 | 30 |
✅ vercel-multi-region
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ nextjs-turbopack | 27 | 0 | 0 |
✅ vercel-ws-transport
| App | Passed | Failed | Skipped |
|---|---|---|---|
| ✅ example | 133 | 0 | 30 |
| ✅ express | 133 | 0 | 30 |
| ✅ nextjs-turbopack | 160 | 0 | 3 |
| ✅ vite | 133 | 0 | 30 |
📊 Workflow Benchmarkscommit Backend:
Streams
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 149528ms → this run 153078ms (Δ +3550ms, +2%) 📈 CRTT drill-down vs main (RTT distributions & profiles)RTT over stream progress (avg per tenth of stream, bars scaled min→max): RTT by chunk size (avg per log size bin, ~160B → ~12KB serialized, bars scaled min→max): Delivery jitter over stream progress (avg positive CDV per tenth of stream, bars scaled min→max): ℹ️ Metric definitions & methodologyStreams: first-chunk RTT (the stream-open path, before any buffering/backpressure), CRTT percentiles, and worst delivery stall (CDV max). Cells are medians across iterations; per-run values in the artifacts. No 🔴/🟢 marks until targets attach. The collapsed STSO distribution section above buckets every step gap, split inline (same warm process — pure framework overhead) vs queue-hop (fresh process — dispatch, reinit, replay). The collapsed CRTT drill-down: per-variant RTT histograms (fixed log bins, 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) · Fan-out TTFS: fan-out time to first step (in-deployment start() → first of the parallel step bodies to complete) · Fan-out TTLS: fan-out time to last step (in-deployment start() → last of the parallel step bodies to complete, i.e. when the Promise.all resolves) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · CRTT: chunk round-trip time (per-chunk write → read latency, one clock domain: deployment → stream backend → same deployment) · CDV: chunk delay variation / delivery jitter (inter-arrival gap minus inter-write gap per seq-adjacent pair; skew-free; the row is each run's MAX positive value, so one stall moves it) 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 · Promise.all(100 steps): 100 trivial no-op steps started together in a single Promise.all; Fan-out TTFS is the first of them to complete and Fan-out TTLS the last, both from the in-deployment clientStart, so their gap is the spread the runtime adds across the fan-out · paced control (100/s, 60B): the control: 300 tiny (~60B) deltas metronome-paced at 100/s — zero workload structure, so it reads the transport floor and flush cadence, and disambiguates transport-wide vs workload-specific when a replay row moves · size sweep (100/s, 160B-12KB): same pacing as the control with deltas padded in rotation across seven log-spaced sizes (~160B–12KB) — rotation decouples size from stream position, so it isolates whether chunk size causes latency · replay gateway-gpt-5.4-nano-2000t (1x): raw provider SSE cadence captured at the AI gateway boundary (gpt-5.4-nano, the most popular gateway model; per-token deltas p50 208B = the modal production chunk size), replayed exactly as measured — the typical customer's workload; its CDV is the typical customer's real delivery jitter · replay eve-gpt-5.6-sol-2000t (1x): a captured eve turn (gpt-5.6-sol, the most-used demanding eve model; ~2000 output tokens = production p50 turn length) replayed exactly as measured — eve's envelope protocol re-ships the cumulative message so sizes ramp 142B→13KB; the demanding outlier tenant's reality · replay eve-gpt-5.6-sol-2000t (2x): the same eve capture at 2x — the headroom/stress row; real fast-tier models emit the same chunk sizes at proportionally higher rate, so time compression is a faithful speed model · first chunk (pooled): every run's seq-0 RTT pooled across all stream scenarios — the first chunk precedes any workload differentiation, so pooling samples one shared stream-open path with exact percentiles Replay cadences (semantic sha256) — eve-gpt-5.6-sol-2000t 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 All timestamps are deployment-side; runs are triggered in-deployment, so the CI runner and api.vercel.com sit outside every measured window. TTFS = Cold starts stay in the numbers (real bursty-workload latency, inflates P75+); Best is the warm floor. |
Sim WorldSimulated world deterministic testing for races. Traces 🟠 world-sim scenario book — 1 fail of 41 total
Full trace: |
About these numbersSizes are gzip; parentheses show the change against
|
There was a problem hiding this comment.
🟡 Changes recommended
Address the completion-dispatch logging path and add current Docs Preview links before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens @workflow/core lifecycle reporting so terminal writes remain authoritative and hydrated stream work drains correctly.
Changes:
- Isolates hook, scheduling, logging, and handler failures.
- Moves lifecycle dispatch after successful terminal writes.
- Adds hydration, stream-draining, regression tests, documentation, and a changeset.
File summaries
| File | Description |
|---|---|
packages/core/test-utils/lifecycle-hooks.ts |
Shared lifecycle test utilities |
packages/core/src/types.ts |
Realm-safe error formatting |
packages/core/src/types.test.ts |
Error-formatting regression tests |
packages/core/src/serialization.ts |
Hydration policy support |
packages/core/src/serialization-lifecycle.test.ts |
Stream hydration and writable tests |
packages/core/src/runtime/run-metadata.test.ts |
Metadata hydration coverage |
packages/core/src/runtime/replay-budget.test.ts |
Dispatch ordering tests |
packages/core/src/runtime/quickjs-lifecycle.test.ts |
QuickJS lifecycle coverage |
packages/core/src/runtime/quickjs-entrypoint.ts |
Post-write completion dispatch |
packages/core/src/runtime/max-deliveries-lifecycle.test.ts |
Max-delivery tests |
packages/core/src/runtime/lifecycle-hooks.ts |
Safe dispatch and stream draining |
packages/core/src/runtime/lifecycle-hooks.test.ts |
Dispatcher and hydration regression tests |
packages/core/src/runtime/deployment-guard.ts |
Post-write failure dispatch |
packages/core/src/runtime/deployment-guard.test.ts |
Deployment-guard coverage |
packages/core/src/runtime.ts |
Max-delivery dispatch ordering |
packages/core/README.md |
Lifecycle behavior documentation |
docs/content/docs/v5/observability/lifecycle-hooks.mdx |
Lifecycle guide updates |
docs/content/docs/v5/api-reference/workflow-api/register-lifecycle-hooks.mdx |
Lifecycle API documentation updates |
.changeset/lifecycle-reporting-isolation.md |
Core patch changeset |
Review details
Suppressed comments (1)
packages/core/src/runtime/quickjs-entrypoint.ts:2031
terminalCreateEventhas already succeeded before this dispatch, butwfdiag('exit_completed', ...)is still inside the surroundingtry. If the diagnostic logger (which ultimately callsconsole.debug) throws, the catch path exits before this line and the persisted run never invokesonRunCompleted. Keep the diagnostic call from being able to suppress lifecycle dispatch (for example, isolate it from the write catch).
dispatchRunCompletedHooks(runId, workflowName);
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Keep the dynamic `workflow/api` import inside the `NEXT_RUNTIME === "nodejs"` guard. Next.js also compiles `instrumentation.ts` for the Edge runtime. A top-level static import pulls Node.js-only dependencies into that compilation and breaks webpack Edge builds, even if the registration call is guarded. | ||
|
|
||
| `registerLifecycleHooks` returns an unregister function. You can register multiple hook sets, and handlers run in registration order. | ||
| `registerLifecycleHooks` returns an unregister function. You can register multiple hook sets, and handlers run in registration order. Registrations are not deduplicated: register each hook set once per process, and call its unregister function before registering it again during hot reload or module re-evaluation. Otherwise, repeated registrations invoke the same handler multiple times for each transition. |
pranaygp
left a comment
There was a problem hiding this comment.
Review of the follow-up. Short version: the terminal-write isolation and the stream drain are correct, and I found no correctness bugs in the diff. Most of what follows is about docs and topology. Several of the items below date from #3678, but this PR's docs are the ones people will copy, so they seem worth settling here or in linked follow-ups.
Verified locally
- Unit tests:
- The 8 changed test files: 116 tests pass.
- The full
@workflow/coreunit suite: 2610 pass. The only failures are e2e suites that needDEPLOYMENT_URL.
- The docs code samples typecheck, 3/3.
- Dispatch sites: in
runtime.ts:865-891,deployment-guard.ts:196-237andquickjs-entrypoint.ts:2003-2031, everycatchbranch now returns or throws before dispatch. Conflict, expired and failed writes never dispatch. - The drain loop (
lifecycle-hooks.ts:181-197) held up under a randomized property test against the real dispatcher:- 300 trials, with nested ops up to depth 3, random pipe rejections, and throws during partial hydration and in handlers.
waitUntilnever settled while an op pushed before settlement was still outstanding.- The loop terminates because
drainedstrictly increases.
- The documented Next.js pattern,
await import("workflow/api")ininstrumentation.ts, works end to end:- Next 16.3.4 on turbopack and on webpack, under
next startand standalonenode server.js. - Each handler fired exactly once per transition, and the registry held 1 entry.
- The
Symbol.forregistry correctly bridges the Instrumentation layer's copy of@workflow/coreand the App Route layer's copy.
- Next 16.3.4 on turbopack and on webpack, under
Topology: the docs' "register everywhere" claim does not hold on some Vercel builds
- Nuxt 4 / Nitro v2, Astro, Nest, and the CLI
vercel-build-output-apitarget on Vercel: hooks never fire, silently.- These integrations emit
.well-known/workflow/v1/flow.funcas a self-contained esbuild bundle throughVercelBuildOutputAPIBuilder:packages/builders/src/vercel-build-output-api.ts:26-48packages/nitro/src/index.ts:230-239andbuilders.ts:38packages/astro/src/plugin.ts:68-69packages/nest/src/vercel-builder.ts:229-262packages/cli/src/commands/build.ts:84-85
- That function carries the queue trigger, so it is the one that writes terminal events. It never loads the app's startup code (Nitro plugins, Astro middleware, Nest bootstrap), so the registry is empty there.
- Dispatch then returns without logging anything (
lifecycle-hooks.ts:168). I confirmed this with a scratch Nitro v2 build using the Vercel preset. - The obvious workaround, registering at the top level of a workflow file, makes things worse. The VM bundle resolves
workflow/apito the throwing stub, which breaks every workflow in that file. - "Any module that loads at startup works" (lifecycle-hooks.mdx:18) and "(which
instrumentation.tsguarantees)" (:95) are therefore not accurate in general. - Nitro v3 (Vite, TanStack Start, and the Express/Hono/Fastify setups), SvelteKit (
hooks.server.ts) and Next are fine, because their flow route is compiled into the app's own server.
- These integrations emit
- Next.js before 16.3.0 on Vercel has a cold-start race.
- Before 16.3.0,
RouteModule.preparedoes notawaitensureInstrumentationRegistered(vercel/next.js#93993, fixed in #94306 and first released in v16.3.0). - So a cold flow invocation can write the terminal event before the dynamic
import("workflow/api")has resolved, and the handlers are skipped. @workflow/nextallowsnext: >13(packages/next/package.json:54).- 16.3.4 awaits it (
route-module.js:367-373).
- Before 16.3.0,
- Nitro calls plugins without awaiting them. An async plugin that copies the Next
await import()pattern has the same race. Nitro users should register with a static import and a synchronous call. - No diagnostics. Every failure above is invisible. Consider a one-time debug log when a terminal event is written and the registry is empty, at least in dev.
Suggestion: have each builder own registration. Something like withWorkflow(config, { lifecycleHooks: "./lib/workflow-hooks.ts" }) or workflow({ lifecycleHooks }) would inject a host-side import of that module into the generated flow and webhook routes. It must never go into the VM bundle.
- That guarantees presence on every function that writes terminal events.
- It removes the Next version requirement, the
NEXT_RUNTIMEguard and the dynamic-import caveat. - With a fixed internal key (see the hot-reload section), it also fixes HMR.
Until then, please replace the prose with a per-framework table: where to register, "Next.js ≥ 16.3", and which Vercel builds are not supported yet.
Module copies: instanceof is unreliable in handlers (docs change needed)
Next bundles instrumentation.ts separately from app routes. withWorkflow does not externalize workflow or @workflow/core (packages/next/src/index.ts:493-499). So the process holds two copies of @workflow/core, @workflow/errors and the user's own modules. This happens with a static import too; the dynamic import is not the cause. The ESM/CJS dual-package hazard does not apply, because these packages are ESM-only.
The registry, World cache, step registry and OTel global are all shared via globalThis (scripts/lint/module-scope-state.mjs reports 0 findings). Class identity is not shared. Tested on both bundlers, inside handlers registered from instrumentation.ts:
| Check | Result |
|---|---|
run instanceof Run |
false |
error instanceof WorkflowRunFailedError |
false; .is() works |
error.cause instanceof MyError for a WORKFLOW_SERIALIZE class |
false (the SWC registration is last-writer-wins, and the route copy evaluates later) |
Plain Error subclass (no custom serialization) |
comes back as a generic Error, name preserved |
cause instanceof FatalError |
depends on which @workflow/errors copy loaded first (first-writer-wins global, packages/errors/src/index.ts:1114-1170) |
Side effect on route code. On a standalone or Vercel-style server, where next.config is not evaluated at runtime, adding the documented instrumentation.ts import makes the Instrumentation layer's @workflow/errors the process-wide FatalError. Route-side err.cause instanceof FatalError then flips from true to false. I A/B tested this on the same build, on turbopack and on webpack. The root cause predates this PR, but these docs route every lifecycle-hooks user into it.
Asks:
- In both pages, say that handler parameters come from the runtime's module copy. Show
WorkflowRunFailedError.is(error),FatalError.is(error.cause),error.cause.nameanderrorCodein the examples instead ofinstanceof. - Soften "registered Error subclass identity preserved" (lifecycle-hooks.mdx:53, and the JSDoc at
lifecycle-hooks.ts:41). - Longer term, a
Symbol.hasInstancebrand onRun,WorkflowRunFailedErrorand the errors classes would makeinstanceofwork across copies. - The "module copies" unit test (
lifecycle-hooks.test.ts:320-331) never loads a second module instance. Usingvi.resetModules()to register through copy A and dispatch through copy B would cover it. Today only the Next-only e2e test covers this.
Hot reload: dedupe should live in the SDK, not in user code
The PR asks users to "unregister the previous hooks before registering again", which in practice means writing a globalThis singleton themselves. What I measured:
next dev(turbopack and webpack), documentedinstrumentation.tspattern: no duplicates. Next memoizesregister()per process (instrumentation-globals.external.js:81-86). The flip side is that edits to the handlers are ignored until the dev server restarts, and the docs don't say so.- Duplicates occur on the other paths the docs point people to:
- With Vite plus
nitro/vite, the Nitro plugin re-runs on every server-file edit in the sameglobalThis. In the worst case I saw 5 handlers fire for one run.closehooks andimport.meta.hot.disposenever fire, so no framework-native cleanup is possible. - Editing SvelteKit's
hooks.server.tsre-runsinit. - Module-scope registration inside a Next route or lib re-registers on every HMR update. Webpack dev also evaluates it once per route entry.
nitro dev(v3) is safe, because each rebuild gets a fresh worker.
- With Vite plus
- Step functions accumulate handlers. Only the
workflowexport condition gets the throwing stub; step bundles get the real function. Calling it from a step therefore adds a handler on every execution. The stub's error message (api-workflow.ts:15-19) says "Move this call to a step function", which leads users straight into that leak. Please giveregisterLifecycleHooksits own stub message ("register at startup, e.g. ininstrumentation.ts"), and warn or throw when it is called inside step context.
Suggestion: ship the registration-key API the PR description deferred: registerLifecycleHooks(hooks, { key?: string }).
- The same key replaces the existing registration in place, keeping its position.
- A stale unregister from an older module instance becomes a no-op.
- No key keeps today's append semantics, so libraries and the app can still coexist.
- Store it in the same
Symbol.forregistry.
A userland copy of these semantics under vite dev kept the registry at 1, and edits applied. The docs then drop the unregister instructions and just show { key: "app" }. The builder-owned registration above can use a fixed internal key.
Pre-seed the handler's Run (avoid the reads the docs warn about)
Handlers get a bare new Run(runId) (lifecycle-hooks.ts:220, :273). One handler awaiting run.workflowName, run.status and run.createdAt makes 3 runs.get calls, and returnValue adds 1–2 more. The dispatcher already has every one of these values in memory.
What every one of the 8 dispatch sites already has:
runtime.ts:455, :885, :3466, :3679, :5299deployment-guard.ts:230replay-budget.ts:186quickjs-entrypoint.ts:2031, :2383
The known values are the immutable workflowName, the terminal status (terminal states never change, run.ts:585), the exact persisted output or error bytes and the key they were written with, errorCode, and usually the updated run record. world.events.create returns EventResult.run (world/src/events.ts:962-966) in world-local, Postgres and the hosted backend, but all 8 sites discard it.
Proposal: add an @internal createTerminalRun(runId, seed) in run.ts, backed by a module-level WeakMap.
- The public constructor and
WORKFLOW_SERIALIZEstay unchanged, and nothing new is exported fromworkflow/api. statusandworkflowNameresolve with no read.- The timestamps come from the record when there is one.
returnValuegoes through the existing#resolveTerminalReturnValue, using the writer's bytes and key.exists, streams,cancelandwakeUpstill read.- The seed does not survive serialization, so a
Runpassed tostart()falls back to reads. - The writer's steady-state path gains nothing: dispatch still returns early when no hooks are registered.
Two correctness constraints:
- Retention. Only world-local returns the purged record with
expiredAtfrom the terminal write. Postgres purges in a separate transaction (retention.ts:128-137), and the hosted backend purges after it responds. For a run with zero retention (purgesUserDataOnFinish(record.attributes)), do not seedreturnValue. Otherwise the handler would get a value wheregetRun(id).returnValuethrowsRunExpiredError. Always take payloads from the writer's bytes, never fromrecord.output, which may be a ref descriptor on the hosted backend. - Deployment guard. Its
undefinedkey means "this payload is unencrypted", not "this run has no key" (deployment-guard.ts:196-200). Keep the payload key separate from the run's key. Prime#getEncryptionKeyonly when the key is defined; otherwiserun.getReadable()breaks on encrypted runs.
Tests to add:
- Zero reads for the seeded accessors.
- Decryption with an encryption key.
- A failed run's
returnValuerejects with the seedederrorCodeand the same cause identity. - Expiry with zero retention.
- No record → fall back to reads.
- Deployment-guard key handling.
- The seed does not survive serialization.
With this in, the "lazy access defers those reads; it does not make them free" paragraphs (lifecycle-hooks.mdx:48, register-lifecycle-hooks.mdx:56) can instead say that these accessors resolve from the terminal write.
Stream-lifetime and observability (from my earlier pass)
- Leaked lock → unbounded
waitUntil. A handler that callsgetReader()on a readable inerror.causeand never releases it keeps the drain, and theworkflow.lifecycle.onRunFailedspan, open until the function's max duration (lifecycle-hooks.ts:185-196); I reproduced this.reader.cancel()settles it, and an unused writable settles in about 50 ms. Please make this a warning callout with atry { … } finally { await reader.cancel() }snippet. Optionally, bound only the post-handler drain (cancel and log after N seconds), which keeps the cost on the rare path. logFailurecan drop a handler failure (lifecycle-hooks.ts:141-155). All its fields are built inside onetry. IfString(err)throws (for example a thrown object whosetoStringthrows), no log line is emitted at all; I reproduced this. Compute each field defensively.- A throwing hook getter is logged as
handler threw(:164). Something likehandler property access threwwould let people tell a bad registration apart from a failing handler. - The span is thin.
workflow.lifecycle.${event}has noworkflow.run_idorworkflow.nameattribute. Handler errors are swallowed before the span sees them, so its status is OK even when every handler threw. Its duration now includes the drain. Please add those attributes and a span event (or a count) for handler failures, and mention the span inobservability/tracing.mdx. - Hydration fallback loses information. Any revive failure (for example a
WORKFLOW_SERIALIZEclass that isn't registered on the host) replaces the whole cause withError('Failed to hydrate workflow run error'). The original name and message are lost, and the exception is never logged. That undercuts the Sentry use case. Degrading per value, or at least logging the reason, would help.Run.returnValue(run.ts:574-579) has the same catch-all. - Docs scope for stream lifetime. The drain covers streams from
error.causeonly.Run.returnValuehydrates with a throwawayopsarray (run.ts:551,:569). So stream pipes started throughawait run.returnValueinonRunCompleted, or by un-awaited orsetTimeoutconsumption, are not kept alive bywaitUntil. Worth one sentence. The "waitUntilscope" sentence is also Vercel-specific and could start with "On Vercel, …". - Writables. A hydrated writable in the cause still forwards to the failed run's stream, or to the parent run's stream when forwarded. A reporting handler can therefore append to a run's stream after its terminal event. This matches
run.returnValue, but an explicit "writes still reach the run's stream" line would stop anyone assumingerror.causeis inert.
Nits
- The Docs Preview links in the description return 404. The pages live at
/v5/docs/observability/lifecycle-hooksand/v5/docs/api-reference/workflow-api/register-lifecycle-hooks. - In the API reference, the dedupe note is under Returns but missing from Behavior.
RunCompletedHookParams extends RunHookParams {}could betype RunCompletedHookParams = RunHookParams, which avoids Biome'snoEmptyInterface.- Missing tests: a thrown value whose
toStringthrows; a handler that cancels a readable mid-read; the randomized nested-ops drain check (cheap to keep as a regression test). - Separate from this PR:
whats-new.mdx:103-118says plain user Error subclasses "keep their class". In my tests a subclass withoutWORKFLOW_SERIALIZEcame back as a genericErrorwith only itsname.
Recommendation: the code changes look good to merge. Before this ships, I'd fix the docs:
- the per-framework and Next ≥ 16.3 caveats;
.is()instead ofinstanceof;- the leaked-lock warning;
- a restart note for
next dev.
I'd track builder-owned registration, the { key } API and Run pre-seeding as follow-ups (happy to open issues).
pranaygp
left a comment
There was a problem hiding this comment.
Approving: the code changes are correct and well tested (see my review above for what I verified). The docs and topology items there, and the builder-owned registration / { key } API / Run pre-seeding follow-ups, don't block this PR. Please pick up the docs caveats (.is() over instanceof, Next ≥ 16.3 and unsupported Vercel builds, the leaked-lock warning, the next dev restart note) here or in a fast follow.
|
No backport to This is a follow-up correctness pass on the lifecycle-hooks feature ( To override, re-run the Backport to stable workflow manually via |
Summary
Follow-up to #3678, implementing the correctness fixes and targeted simplifications from Alex's review.
try/catchblocks. Rejected writes still never dispatch.opsarray in the dispatcher'swaitUntilscope. Drain it after the handler loop, including on partial hydration or handler failure. UseallSettledand drain subsequent batches so one rejected pipe cannot shorten another pipe's lifetime and nested operations are included.ExternalReviverOptionsdirectly tohydrateRunError, preserving default hydration and custom-reviver compatibility while avoiding a redundant external-reviver set.Review decisions
prepare/invokepair rather than introducing the proposedparams as neverassertion. Shared diagnostic logging and parameter interfaces provide the smaller simplifications without that cast.Tests
waitUntiltest capture that fails when expected dispatches never arrive; negative assertions observe the synchronous scheduling boundary.Verified locally:
pnpm test --filter=@workflow/core --output-logs=errors-onlypnpm build --output-logs=errors-only(28 tasks)pnpm typecheck --output-logs=errors-only(43 tasks)pnpm test:docs -t 'lifecycle-hooks|register-lifecycle-hooks'(3 passed)git diff --check, andnode scripts/check-changesets.mjsDocs Preview
Preview links require Vercel team access.