Carry run identity on step-dispatch messages; drop the blocking runs.get from the queued-step prologue - #3457
Carry run identity on step-dispatch messages; drop the blocking runs.get from the queued-step prologue#3457TooTallNate wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 03bf52c 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▲ Vercel Production (1 failed)fastify-quickjs (1 failed):
E2E Test SummarySummary
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ vercel-multi-region
|
There was a problem hiding this comment.
Pull request overview
This PR reduces queued step start latency and read amplification by carrying immutable run identity (runContext) on step-dispatch messages so the consumer can skip the blocking world.runs.get in the queued-step prologue, while still lazily fetching the run row only for the fan-out’s last completer.
Changes:
- Added
WorkflowInvokePayload.runContext(deploymentId/specVersion/startedAt/rootRunId) to the@workflow/worldqueue message schema. - Updated all step-dispatch producers (node suspension handler, QuickJS step queueing, runtime retry dispatch) to stamp
runContext, and updated deployment-mismatch re-enqueue to preservestepInput/runContext. - Updated the queued-step consumer path to use
runContextto avoidruns.get, with new tests covering bothrunContextand legacy-message behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/world/src/queue.ts | Introduces RunDispatchContextSchema and adds runContext to WorkflowInvokePayloadSchema. |
| packages/core/src/runtime/suspension-handler.ts | Stamps runContext on step-dispatch messages produced during suspension handling. |
| packages/core/src/runtime/suspension-handler.test.ts | Asserts resilient publishes include the expected runContext. |
| packages/core/src/runtime/quickjs-entrypoint.ts | Stamps runContext on QuickJS step-dispatch messages. |
| packages/core/src/runtime/helpers.ts | Adds helpers to compute rootRunId and build runDispatchContext from a run row. |
| packages/core/src/runtime.ts | Removes the queued-step prologue runs.get when runContext is present; preserves payload on re-route; lazy-fetches run row only for inline replay synthesis. |
| packages/core/src/runtime.test.ts | Adds consumer tests verifying runs.get is skipped with runContext and still occurs for legacy messages. |
| .changeset/step-dispatch-run-context.md | Declares minor bumps for @workflow/world and @workflow/core describing the new behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Durabench verification (sweep psweep-1786442289510, node engine, n=15/cell, iad1)
No regressions at either scale; semantics unchanged for legacy (no- |
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
| (await world.runs.get(runId, { | ||
| resolveData: 'none', | ||
| })); | ||
| if (replayRunRow.status !== 'running') { |
There was a problem hiding this comment.
AI Review: Note
This gate also returns on pending, and it returns silently. Under the fetch-free prologue this is the only status read the last completer performs, so a stale pending here abandons the fan-out's continuation: the final step_completed is already written, the inline replay never runs, and nothing is logged. The legacy prologue's equivalent early exit (line 1532) at least logs the observed status, so the same stall was diagnosable before.
This is not a regression in outcome (the legacy read would also have returned on a stale pending), but it moves the drop to a quieter place. Two small changes keep the intent and make it debuggable:
- gate on
isTerminalWorkflowRunStatus(replayRunRow.status)(exported from@workflow/world, already used inruntime/resume-hook.ts) so a non-terminal read falls through to the replay rather than dropping it; - add a
runtimeLogger.debugwith the observed status on the early exit, matching line 1533.
I reproduced the stall locally — see the note on runtime.test.ts.
There was a problem hiding this comment.
Agreed on both counts — and thanks for reproducing the stall; that made the severity unambiguous. Fixed in 03bf52c: the gate is now isTerminalWorkflowRunStatus(replayRunRow.status) with a runtimeLogger.debug logging the observed status on the early exit. A stale pending falls through to the inline replay (a run with completed steps has necessarily started, so pending here can only be a stale row), and the replay's next entity write is fenced server-side if the run truly ended meanwhile. Covered by the new two-phase fan-out test: the stale-pending variant asserts run_completed is still written, and it fails without this fix.
| // fan-out (vercel/workflow#3456). The run-status early | ||
| // exit is not lost: a terminal run rejects the | ||
| // `step_started` claim server-side (RunExpired → gone, | ||
| // terminal step → skipped). Older messages without the |
There was a problem hiding this comment.
AI Review: Note
"a terminal run rejects the step_started claim server-side" holds on world-vercel, but it is overbroad for the adapters in this repo. world-local (storage/events-storage.ts:996) and world-postgres (storage.ts:858) only raise RunExpiredError on a terminal run when the step's own status is not already running. A redelivery of a step that a previous delivery had already started therefore passes the claim on a cancelled/completed run and executes the user's step body, where the old prologue's status check would have skipped it. The result is discarded at the step_completed write, so nothing corrupts, but the side effects run.
Worth either narrowing this comment to the adapters that enforce it, or adding the run-status check to those two.
There was a problem hiding this comment.
You're right that the claim was overbroad — and rather than narrowing the comment, I closed the adapter gap in 03bf52c: world-local and world-postgres now reject step_started on a terminal run even when the step row still reads running. Starting work on a finished run is never valid (the carve-out exists so in-flight steps can write their terminal events, and step_completed/step_failed remain unchanged), so a redelivered start on a cancelled/completed run now gets RunExpiredError → gone → ack instead of re-running the body with an unconsumable outcome. Both worlds' suites pass (542 local / 179 postgres incl. the shared spec suite), a patch changeset covers the behavior change, and the prologue comment now states the contract precisely — including that this change is what makes it hold on the local adapters.
| ); | ||
| return; | ||
| } | ||
| runIdentity = runContext; |
There was a problem hiding this comment.
AI Review: Note
Nothing distinguishes the fetch-free path from the legacy one in telemetry, so neither adoption nor the claimed round-trip saving is measurable after rollout. During a skew window both paths run concurrently across deployments, and the only way to tell them apart will be inference from runs.get volume. A span attribute on the step-execution span (Attribute.StepResilientDispatchMaterialized is the existing precedent) would make this observable for the cost of one line.
There was a problem hiding this comment.
Added in 03bf52c: workflow.step.dispatch_prologue span attribute (run_context | runs_get), set on the step-execution handler span right where the prologue forks — one line, following the StepResilientDispatchMaterialized precedent. Adoption and the saved round trip are now directly queryable during skew windows.
|
|
||
| expect(response.status).toBe(204); | ||
| // The step executed to completion with the run identity from the message | ||
| // — no run fetch on the start path. (The all-done inline replay would |
There was a problem hiding this comment.
AI Review: Note
The comment is accurate, and it marks the gap: the lazy runs.get at runtime.ts:1777 and the new status gate at :1782 are the fan-out path this PR exists to optimize, and no test in the PR reaches them, because the harness always keeps an unrelated step pending.
I covered it locally with a two-phase test (not committed): phase 1 drives a real replay of a two-step Promise.all fan-out with WORKFLOW_MAX_INLINE_STEPS=1 so the runtime itself emits the queued step message and its seeded correlation id; phase 2 redelivers that exact message against the shared event log, making it the last completer. Results:
- the producer stamps
runContext(deploymentId,specVersion,rootRunId) on the queued message; - the last completer calls
runs.getexactly once — zero reads before the step, one for the inline replay — and reachesrun_completed; - with the lazy read returning
pending,step_completedis written andrun_completednever is: the run is silently abandoned. That is the empirical basis for the note onruntime.ts:1782.
The harness needs no new fixtures, just a fan-out workflow and a shared in-memory event log across the two deliveries — worth adding, since the third case is the one behavior change here that no existing test would catch.
There was a problem hiding this comment.
Added in 03bf52c, following your two-phase construction: phase 1 drives a real replay of a two-step Promise.all fan-out with WORKFLOW_MAX_INLINE_STEPS=1 (asserting the runtime's own queued message carries the stamped runContext), phase 2 redelivers that exact message against the shared event log as the last completer. Three variants: happy path (zero reads before the step, exactly one lazy runs.get, run_completed written), the stale-pending fall-through (passes only with the isTerminalWorkflowRunStatus gate — your reproduced stall, now pinned), and the genuinely-terminal skip. Thanks for the harness sketch — the shared-log two-delivery shape dropped in cleanly next to the existing suites.
| if (runContext) { | ||
| const ensureOutcome = | ||
| stepInput && metadata.attempt > 1 | ||
| ? await ensureStepFromMessage() |
There was a problem hiding this comment.
AI Review: Nit
The rationale above this call (line 1416: "in parallel with the run fetch below … at no wall-time cost") no longer describes this branch, where there is no run fetch to overlap with. The cost is unchanged (one round trip before step_started either way), so this is comment drift only — but on the fetch-free path the eager re-ensure is now the sole pre-step write, which makes it worth restating why it is still preferred over letting the in-band recovery handle a missing step_created.
There was a problem hiding this comment.
Comment drift fixed in 03bf52c: the doc now states both shapes — on the legacy prologue the eager ensure overlaps the run fetch (no wall-time cost); on the fetch-free path it is the sole pre-step write and is kept because a redelivered dispatch has already had its create race resolved, so one conditional write is cheaper than letting the bare start fail and paying the in-band recovery's extra start round trip. The fork site in the runContext branch restates the same rationale.
…king runs.get from the consumer prologue Closes #3456. Every queued step execution paid a runs.get round trip before its step_started claim — one RTT per branch on the TTLS-critical path, and under a 256-branch fan-out burst the read amplification drove that read to p90 ~5.1s (durabench parallel sweeps), smearing branch starts. The dispatch sites (node dispatch loop, delayed retries, the suspension handler's resilient publish, and the quickjs engine's queueStepMessage) now stamp WorkflowInvokePayload.runContext with the fields the consumer actually needs — deploymentId, specVersion, startedAt, rootRunId — all immutable for the life of a run and known from the run row the producer already holds. A consumer that receives it skips the run fetch: the run-status early exit is enforced by the step_started claim itself (RunExpired → gone, terminal step → skipped), guardDeployment takes the carried identity, and only the fan-out's LAST completer fetches the full run row, lazily, for its inline replay — once per fan-out instead of once per branch. The deployment-mismatch re-route now also preserves stepInput/runContext on the re-enqueued payload. Messages without runContext (older producers) keep the legacy prologue; messages are deployment-pinned, so mixed handling within one run cannot occur.
…nce in local worlds, prologue telemetry, last-completer coverage
- The last completer's lazy runs.get result is now gated on
isTerminalWorkflowRunStatus (with a debug log): a stale 'pending' read
— a run with completed steps has necessarily started — no longer
silently abandons the fan-out's continuation; it falls through to the
inline replay, whose next entity write is fenced server-side if the
run truly ended meanwhile.
- world-local / world-postgres now reject step_started on terminal runs
even when the step row still reads 'running' (a redelivered start a
previous delivery claimed): starting work on a finished run is never
valid, and previously the body re-ran with its outcome unconsumable.
In-flight steps still write their terminal events unchanged. This
closes the adapter gap behind the fetch-free prologue's reliance on
the step_started claim as the run-liveness check, and the prologue
comment now states the contract precisely.
- workflow.step.dispatch_prologue span attribute ('run_context' |
'runs_get') makes fetch-free adoption and the saved round trip
observable during version-skew windows.
- Restated why the eager redelivery re-ensure survives on the
fetch-free path (no run fetch to overlap; still cheaper than the
in-band recovery's failed-start round trip).
- New two-phase fan-out coverage: a real replay emits the queued step
message (asserting the stamped runContext), then its redelivery runs
as the LAST completer — zero reads before the step, exactly one lazy
runs.get, run completed; plus the stale-'pending' fall-through and
the genuinely-terminal skip.
f3d9a8e to
03bf52c
Compare
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 229937ms → this run 206100ms (Δ -23837ms, -10%) ℹ️ 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 |
Sim WorldSimulated world deterministic testing for races. Traces 🟠 Mint-ordered log — 6 fail of 41 total
Full trace: 🟢 Append-only log — 0 fail of 41 total
Full trace: |
Summary
Closes #3456. Stacked on #3365 (base:
resilient-step-dispatch) — same code region, and the sweep data motivating both came from that PR's benchmarking.Every queued step execution paid a blocking
world.runs.getbefore itsstep_startedclaim: one round trip per branch on the TTLS-critical path (~30–80ms p50), and under a 256-branch fan-out burst the read amplification drove that read to p90 ≈ 5.1s (durabench parallel sweeps,wrun_41KZR0MW890GWZK34RD4Y1JBDT), directly smearing branch starts across the ~17s TTLS cliff.What changed
@workflow/world: additiveWorkflowInvokePayload.runContext(deploymentId,specVersion,startedAtepoch-ms,rootRunId) — the run's immutable identity, stamped at dispatch time from the run row the producer already holds. Run status is deliberately not carried: liveness is enforced by thestep_startedclaim, which every World rejects on a terminal run (RunExpired→gone, terminal step →skipped) — same outcome as the old status check, minus the read.queueStepMessage): stamprunContext.runContext, the prologue makes zero reads —guardDeploymenttakes the carried identity (Pick<WorkflowRun, 'runId'|'deploymentId'|'specVersion'>is all it needs),executeStepparams come from the message, and only the fan-out's last completer fetches the full run row, lazily, for its inline replay: once per fan-out instead of once per branch. Legacy messages (norunContext) keep the exact previous path; messages are deployment-pinned so mixed handling within a run cannot occur.stepInput/runContexton the re-enqueued payload (previously dropped).Wins
Testing
runContext⇒runs.getnever called (start path); legacy message ⇒ exactly one fetch;runContext+ in-band step-missing recovery combined (still zero fetches).runContext.Verification plan: re-run the durabench parallel sweep at {64, 256} against this branch — expect TTFS/TTLS p50 improvement at 256 branches and no change in semantics elsewhere.