perf(core): initialize lazy hook replay from hook_received stream - #3345
perf(core): initialize lazy hook replay from hook_received stream#3345karthikscale3 wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 2daae33 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 E2E Test Results❌ Some tests failed ❌ Failed E2E Tests📋 Other (1 failed)e2e-local-prod-nest-stable-node (1 failed):
E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 📋 Other
✅ vercel-multi-region
|
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 133947ms → this run 140317ms (Δ +6370ms, +5%) 1020 steps (queue-hop) Cumulative STSO time: main 2956ms → this run 3278ms (Δ +322ms, +11%) 📜 Previous results (1)549d6f0Wed, 05 Aug 2026 01:54:01 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 |
On a lazy hook queue delivery, the consumer's idempotent hook_received re-ensure is hoisted above run_started and doubles as the invocation's setup request: it asks the World to return the current replay log with the write (new advisory CreateEventParams.preloadEvents), so one HTTP request yields the canonical event, the reconstructed run, and the complete replay log — skipping both the run_started POST and the initial events.list. - world: optional `preloadEvents?: true` on CreateEventParams, the hook_received dual of skipPreload; Worlds may ignore it - world-vercel: createHookReceivedPreloadEventV4 sends the frame Accept on eligible hook_received posts and decodes either response mode — frames via the response decoder extracted from the LIST consumer (GET behavior unchanged), CBOR via the shared materialized-response mapping. The run is reconstructed from run_created/run_started (plus attr_set folds), the canonical event found by x-wf-event-id, and resumeId now survives frame decoding so the runtime can match it - core: new fast path before the generic run-state setup, guarded on hookInput.resumeId + payloadDigest; a validated COMPLETE preload (hasMore false — this path has no cursor-continuation machinery) initializes workflowRun/preloadedEvents/maxEventsLimit directly, anything else falls back to the run_started setup without re-posting the hook; error classification matches the existing re-ensure (terminal → consume, transient → redeliver); setup source reported via workflow.resume_setup_source (never workflow.hook.resilient_resume_materialized, which stays a recovery-only signal) - producer resumeHook() is unchanged and never sets preloadEvents Based directly on main (no dependency on #3124/#3191); pairs with workflow-server's streamed hook_received replay-log response, which deploys first — the SDK negotiates per request and falls back safely against older servers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
549d6f0 to
2daae33
Compare
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed at 549d6f0. Locally: build + typecheck green, core 1920 passed / 3 expected fail, world-vercel 339 — with the 12 consumer-preload tests covering every fallback and error branch I went looking for.
What held up under scrutiny:
- The completeness validation is the right shape: run +
startedAt, non-empty events, non-null cursor,hasMore === false, numericmaxEvents(this response plays run_started's role, so a missing ceiling would silently disable event-limit enforcement — good catch), both lifecycle events, and thehook_receivedmatching thisresumeId. On the cursor requirement: I confirmed the backend synthesizes a cursor on the final page even for single-page logs, so the check doesn't dead-letter short runs — but that server behavior is now load-bearing for this fast path; a code comment noting the dependency would help the next person. - Error classification is byte-for-byte consistent with the existing re-ensure: HookNotFound/RunExpired consume the delivery; everything else (EntityConflict, truncated stream, transport) rethrows for redelivery and the
(runId, resumeId)claim converges. And the deliberate omission ofHookResilientResumeMaterialized(this path carries no recovery signal) keeps that metric honest. - The terminal-event check before engine dispatch correctly plugs the QuickJS gap (it dispatches before the node loop's terminal detection), and
preloadedEventsCompleteas an explicit attestation — rather than widening the first-invocation heuristic — is the safer design. resumeIdthreading throughbuildEventFromV4is essential and easy to miss: without it, frame-decoded events would silently fail the matching check and the fast path would never fire. The comment says exactly that.remoteRefBehavior: 'resolve'override on the preload request is right (v4 has no refs endpoint to hydrate lazy descriptors mid-replay), andreconstructRunFromReplayEventscarries every field downstream consumers read — includingdeploymentIdandspecVersion, which the in-flight deployment-affinity and slot-identity work key off.
Three asks before merge:
- Rebase — the PR is currently CONFLICTING with main (a one-file test conflict in
events.test.tsvs #3334). - Coordinate with #2960 (deployment-affinity guard, also open). Its design places the guard ahead of the lazy-hook re-ensure with the explicit invariant "a misrouted resume writes nothing here" — this fast path hoists the
hook_receivedwrite above where that guard will sit. I believe the combination is still safe (the write is idempotent, involves no key derivation, and the guard still precedes any replay/step execution), but whichever PR lands second must reconcile the placement and rewrite that comment — the "writes nothing" invariant will no longer be literally true, and it should be weakened deliberately rather than silently. - Changeset bump:
@workflow/worldgains a new public interface field (preloadEvents) plus documentedEventResultsemantics — per the convention we've applied on recent PRs, new API surface on the world interface should beminor, notpatch.
For the record, the paired backend PR's red trigger lane ran with main's SDK (no Accept header → this feature dormant), and its failures match the varied preview-lane flakiness other backend branches see — not this pair. The real proof of the active path will be this PR's own e2e once rebased, since the backend half is already deployable ahead.
Nice perf win with a genuinely safe fallback story. Approving.
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Reviewed together with vercel/workflow-server#706 across correctness, perf, compat, observability, and docs. The core design checks out: the preload validation is sufficient against truncation and staleness (the decoder hard-requires the _end sentinel; the completeness checks cover the rest), ordering matches events.list by construction (same server query, same decode, no client re-sort), the fallback never re-posts the hook, and the compat matrix (old server, server rollback, preload-unaware worlds, Accept-header-only opt-in) verifies clean. Inline comments for the specifics.
One process ask: the streamed path is never exercised end-to-end pre-merge — the server PR's trigger tests SDK main (dormant CBOR path only) and this PR's e2e ran against production without #706 (fallback only). Suggest merging + deploying #706 first, then re-running this PR's Vercel Prod e2e lanes before merge so frames → validation → replay-init runs against the real server at least once.
Non-blocking: docs/content/docs/v5/changelog/resilient-resume.mdx describes this flow and could take a one-paragraph update for the consumer fast path.
| ); | ||
| hookEnsured = true; | ||
| // Note: unlike the re-ensure below, this hoisted write | ||
| // does NOT set HookResilientResumeMaterialized — it |
There was a problem hiding this comment.
Telemetry regression: with this hoisted write never setting HookResilientResumeMaterialized, and hookEnsured = true making the re-ensure block's set site (~L1925, the only remaining one) unreachable for every resumeId+digest delivery, workflow.hook.resilient_resume_materialized stops being emitted fleet-wide once this ships — on both the stream and fallback outcomes, so old servers are affected too.
The producer-side pair workflow.hook.resilient_resume still fires, so anything pairing recovery-begin with recovery-complete will read as 100% never-completing, and workflow.resume_setup_source can't stand in (it can't distinguish a consumer-materialized recovery from ordinary convergence — the v4 response doesn't expose eventWasCreated to the SDK).
Worth either plumbing an equivalent signal through the preload response, or explicitly accepting the loss and updating the attribute's doc in semantic-conventions.ts (it still documents materialized as the completion of the recovery path).
| // step path already loaded the run. | ||
| let hookEnsured = false; | ||
| if ( | ||
| !workflowRun && |
There was a problem hiding this comment.
This gate checks only the message shape — no world/server preload-capability check — so the hoisted write runs on every lazy resume even where nothing can honor preloadEvents (old or rolled-back servers; world-local — world-postgres never takes the lazy path). Two consequences, neither blocking:
- On those backends the path becomes
hook_received+run_started+events.list— one more request than main, because main's Option A skip (re-ensure elided when the producer's write is already visible in the run_started preload) is now unreachable for resumeId-carrying deliveries. The surviving Option A block below is effectively dead code for this shape — worth removing or re-gating. - Against a pre-Friendlier and actionable error messages #706 v4 server, the preload attempt sends
remoteRefBehavior: 'resolve'(world-vercel events.ts:754), so the old server S3-resolves and echoes the full hook payload in a CBOR response the consumer discards — potentially large payloads per resume during a rollout window or rollback.
| // the delivery here, before any engine runs. Same | ||
| // outcome as the run_started path's non-running | ||
| // status check. | ||
| if (hasRecordedTerminalRunEvent(result.events, runId)) { |
There was a problem hiding this comment.
Nit: this early return happens before the span?.setAttributes below, so deliveries consumed against an already-terminal run carry neither WorkflowRunStatus nor workflow.resume_setup_source. The baseline path recorded the terminal status on the span before its equivalent skip — queries segmenting wasted deliveries by run status lose these spans on the fast path; only the log line remains.
| * the same event-frame sequence LIST uses, ending with the `_end` sentinel. | ||
| * A truncated stream (EOF without the sentinel) throws; the write is | ||
| * deduplicated by the server's `(runId, resumeId)` constraint, so retrying | ||
| * the whole request is safe and converges on the same canonical event. |
There was a problem hiding this comment.
This docstring is right that the (runId, resumeId) constraint makes retrying safe — but withEventPostRetry still classifies hook_received as retryable: false ("no server guard → a retry duplicates the row", event-retry.ts:137), which is now stale for the digest+resumeId shape. Since this single POST now carries the whole invocation setup, one transient ECONNRESET costs a full queue redelivery + cold invocation + full re-stream, where a ~200ms inline retry (as run_started gets today) would recover. Cheap follow-up: mark the digest+resumeId shape inline-retryable.
| * deduplicated by the server's `(runId, resumeId)` constraint, so retrying | ||
| * the whole request is safe and converges on the same canonical event. | ||
| */ | ||
| export async function createHookReceivedPreloadEventV4( |
There was a problem hiding this comment.
Per the repo rule (every world-vercel request path must inject trace context, covered in trace-propagation.test.ts — this exact file regressed cross-service correlation once before): this new request shape currently inherits injection via fetchV4 → instrumentedFetch, which is correct, but trace-propagation.test.ts is untouched and the new tests assert Accept/meta/remoteRefBehavior but never traceparent. A later refactor giving this call its own fetch (e.g. streaming-specific abort/timeout handling) would silently break correlation for the new hot path — exactly how the original events-v4 regression happened. Can we add the propagation test for this path?
| * ignored for other event types. Producer-side `resumeHook()` must not set | ||
| * it. | ||
| */ | ||
| preloadEvents?: true; |
There was a problem hiding this comment.
The JSDoc names the fields a World may return, but the honored-only-if contract lives only in core's validation code: complete log (hasMore: false), a valid cursor, run + maxEvents, both lifecycle events, and the hook_received matching the delivery's resumeId. And unlike the sinceCursor doc above, nothing states the returned log must be read atomically consistent with this write, with the same ordering semantics as events.list.
A community world author mirroring the sinceCursor pattern (first page, hasMore: true) would silently never activate the fast path — confusing but safe. Worse: a non-transactional read that happens to contain the lifecycle events and the matching hook_received passes structural validation while missing a concurrently committed event, and replay initializes from a wrong log — the CORRUPTED_EVENT_LOG class this validation exists to prevent, on a path the validation cannot catch. Suggest spelling out the completeness + consistency rules here (and optionally a paragraph in the worlds docs).
| let events: Event[]; | ||
| let eventsFetchedPages = 0; | ||
| const usePreloaded = isFirstInvocation(preloadedEvents); | ||
| const usePreloaded = |
There was a problem hiding this comment.
Test gap: the new consumer-preload tests drive only the node:vm engine, so this gate — trusting an attested-complete preload as the full log, and (critically) NOT trusting a fallback run_started preload beyond first invocation — has zero coverage. If a future edit drops the preloadedEventsComplete === true arm or the length guard, QuickJS hook resumes would replay from a bounded run_started page and silently drop the log tail. One QuickJS-engine test each for "attested complete → used as full log" and "fallback preload → refetches via events.list" would pin it.
3 participants
What
On a lazy hook queue delivery, the consumer's idempotent
hook_receivedre-ensure is hoisted aboverun_startedand asks the World to return the current replay log with the write (new advisoryCreateEventParams.preloadEvents). When a complete preload comes back, the invocation initializes replay from that one request and skips both therun_startedPOST and the initialevents.list; otherwise it falls back to the existing setup without re-posting the hook.Why
Each removed round trip sits directly on hook-resume latency (queue receipt → replay start). Folding the re-ensure and the replay load into one request cuts consumer startup/TTFS by roughly two request latencies on the normal lazy-resume path.
Notes
preloadEvents; a preload is trusted only when validated as complete (run +hasMore: false+maxEvents+ lifecycle events + the matchingresumeId). A terminal event in the preload consumes the delivery before engine dispatch.resumeHook()is unchanged and never setspreloadEvents.workflow.resume_setup_source(hook_received_stream|hook_received_fallback).🤖 Generated with Claude Code