Batch: pre-claim inline steps as born-running pairs in the suspension fold - #3568
Batch: pre-claim inline steps as born-running pairs in the suspension fold#3568pranaygp wants to merge 2 commits into
Conversation
|
🧪 E2E Test Results✅ All tests passed 🛠 Infra Events (absorbed by the harness)Platform anomalies the e2e harness detected and worked around (e.g. a run the queue never picked up, replaced by a fresh run). Clustered timestamps indicate a backend blip; a steady drip indicates a platform issue worth escalating.
E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 🌐 Cross-language Conformance
✅ vercel-multi-region
|
Sim WorldSimulated world deterministic testing for races. Traces 🟠 Mint-ordered log — 3 fail of 41 total
Full trace: 🟢 Append-only log — 0 fail of 41 total
Full trace: |
There was a problem hiding this comment.
Pull request overview
This PR extends the batched suspension fan-out path to pre-claim lazy inline steps by folding each inline step into an adjacent [step_created, step_started] pair inside the same createBatch write, eliminating per-inline-step claim POST overhead. It also threads per-event computeInstanceId through the batch contract, updates inline execution to consume pre-claimed verdicts, and overlaps inline bodies with background dispatch publishes while preserving failure semantics.
Changes:
- Add per-event
computeInstanceIdtoBatchEventRequestand thread it through the world-vercel batch wire format. - Implement “pre-claimed inline pairs” in the suspension handler and plumb
inlineClaims+batchCommittedSlotCeilinginto runtime inline execution. - Add
preclaimedStartsupport toexecuteStepand record thepreclaimedStartoptimization in step latency telemetry; update docs for the spec/runtime behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/world/src/events.ts | Extends BatchEventRequest with optional computeInstanceId. |
| packages/world-vercel/src/events.ts | Includes per-event computeInstanceId in batch frame meta when provided. |
| packages/core/src/runtime/suspension-handler.ts | Folds lazy inline steps into batched created+started pairs; returns inlineClaims and batchCommittedSlotCeiling. |
| packages/core/src/runtime/suspension-handler.test.ts | Adds coverage for pair folding, ownership stamping, 409 handling, chunk integrity, and slot ceiling behavior. |
| packages/core/src/runtime/step-latency.ts | Adds preclaimedStart optimization flag to latency event data. |
| packages/core/src/runtime/step-executor.ts | Introduces PreclaimedInlineStart + preclaimedStart parameter to run/skip inline bodies without a start write. |
| packages/core/src/runtime/step-executor.test.ts | Tests owned preclaimed execution (no start write) and lost-claim skip (no writes). |
| packages/core/src/runtime.ts | Passes ownerMessageId, runs publishes concurrently with inline bodies, and folds batchCommittedSlotCeiling into slot snapshots. |
| docs/content/docs/v5/changelog/batched-event-writes.mdx | Documents the new batch request field and the pre-claimed inline pair behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const inputs = events.map(({ event, occurredAt, computeInstanceId }) => { | ||
| const { payload, meta } = splitEventDataForV4(event); | ||
| return { | ||
| runId, | ||
| eventType: event.eventType, | ||
| specVersion: event.specVersion ?? 2, | ||
| ...(event.correlationId ? { correlationId: event.correlationId } : {}), | ||
| // Under slot identity this is the source of the durable createdAt, so | ||
| // the caller's logical time is what every replay observes. | ||
| occurredAt: occurredAt ?? new Date(), | ||
| // Per-event compute attribution (pre-claimed inline starts) — rides the | ||
| // frame meta exactly like the single POST's CreateEventParams field. | ||
| ...(computeInstanceId !== undefined ? { computeInstanceId } : {}), | ||
| // Batch responses carry entities for bookkeeping, not payload reads — |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| // left running past this handler would race its own | ||
| // redelivery. | ||
| try { | ||
| await dispatchesSettled; |
There was a problem hiding this comment.
An inline step body that rejects (e.g. a 412 stale-claim PreconditionFailedError) while the concurrent dispatch publishes (dispatchesSettled) are still pending has no rejection handler attached, producing an unhandledRejection that can crash the process under Node's default --unhandled-rejections=throw.
Restacked onto main after #3025's squash-merge; folds in the review-round changes to the flush loop (per-write requestId attribution on createBatch, and the seeded/advancing slot-bump expectation, now shared with the pre-claim ceiling). Fold each lazy-inline step's deferred writes into the batched fan-out as an adjacent [step_created, step_started] pair: the created row carries the input, the started row is a bare ownership-stamped claim the server folds into one born-running create. The whole scheduling turn commits as ONE durable write, inline bodies start straight off that commit (in parallel with the VQS publishes for backgrounded steps), and executeStep gains a pre-claimed mode that runs or skips the body off the batch's per-event verdict - a pair 409 is the same skipped outcome as losing the lazy claim. The lone-inline case keeps the optimistic lazy path (a pair-only batch buys nothing over the single claim). Also threads per-event computeInstanceId through the World batch request, and folds the batch's committed slot ceiling into the inline slot snapshot so terminal writes stop being answered with reports echoing the batch's own events.
Production trace of a 67-event fan-out showed the three batch chunks POSTing back-to-back (~230ms each) with no bodies or queue messages until all three settled (~670ms). Three changes: - Chunks now POST concurrently. Slot assignment is the server's, so parallel chunks race for slot ranges exactly like the pre-fold path's parallel single writes did; entity conditions, not commit order, carry correctness. The foreign-interleaving diagnostic is computed once over the whole fold (committed span vs seed) instead of per chunk. - Per-chunk continuation: each chunk's step-execution queue messages publish the moment ITS creates are durable (in-flush, via stepDispatch, same message shape and idempotency key as the caller's dispatch pass - the affected steps are pre-reported in queuedStepCorrelationIds so the caller skips them). Only the chunk carrying the inline pairs gates handleSuspension's return (opt-in via allowDeferredBatchWork); trailing chunk commits + all publishes ride result.deferredBatchWork, which the runtime joins next to the dispatch join before it can ack - the every-create-durable-before-ack contract is unchanged, the bodies just start off the pair chunk instead of the slowest chunk. - OTel: batch identity attributes (workflow.batch.size, per-type workflow.batch.shape) now live on the world.events.createBatch span (instrumentObject) instead of the http POST span, which keeps only wire-level facts (transport, bytes) and no longer sets workflow.event.type - that attribute names a single event write and tagging a batch with its first event's type misclassifies traffic.
| batchFanoutEligible && | ||
| ownerMessageId !== undefined && | ||
| lazyInlineCorrelationIds.size > 0 && | ||
| (lazyInlineCorrelationIds.size >= 2 || |
There was a problem hiding this comment.
AI [question]: This disjunct may contradict the reasoning behind the lone-inline exclusion, because the claims it replaces were already concurrent.
runtime.ts invokes run() inside inlineExecutions.map(...), so N lazy step_started POSTs go out in parallel — N concurrent claims cost ~1 RTT, not N. The exclusion just above is justified as "a pair-only batch costs the same round trip as the single claim while giving up the claim/body overlap and bump-and-report", and that argument generalizes past N=1: a pair-only batch of any size also costs one round trip and also gives up the overlap.
It is also the common shape rather than an edge case. With MAX_INLINE_STEPS = 3 (constants.ts:167), a plain 3-step Promise.all fan-out is 3 inline + 0 eager: the second disjunct evaluates 3 - 3 + 0 = 0, but size >= 2 is true, so it folds — trading turbo's claim/body overlap for no round-trip saving.
There is a good counter-argument the description doesn't make: one POST is a single latency sample where N concurrent claims are a max-of-N, and the trace's own 356/1110/375 spread shows the tail dominates. Folding may well win on p99 for that reason alone. But if that is the justification it should be the stated one, since the round-trip argument doesn't survive the claims being concurrent.
Has the N=2..3-with-nothing-else case been measured? If the tail argument holds, worth recording it in this comment; if not, the gate arguably wants the "≥1 other batchable event" disjunct only.
| // joins before it can ack (below, next to the | ||
| // dispatch join) — so the durability contract | ||
| // is unchanged while the bodies start earlier. | ||
| allowDeferredBatchWork: true, |
There was a problem hiding this comment.
AI [question]: This opt-in changes an ordering property that held before it, and I'd like to confirm nothing downstream depends on the old one.
Bodies start off the pair chunk's commit while trailing chunks ride deferredBatchWork, so a fast inline body can write step_completed before a trailing chunk commits its step_createds. Previously — including #3025 — await Promise.all(dispatches) gated the bodies, so every create in the fold was durable before any body ran. The new contract is only "every create durable before ack", which is strictly weaker: the log can now hold a step's terminal event at a lower slot than a sibling's created event.
The replay path looks safe: matching is by correlation id, slots stay dense, and creates are idempotent, so a redelivery after a trailing-chunk failure re-creates the missing steps correctly.
What I can't rule out is consumers outside the replay path — the ClickHouse analytics ingest and the run-details UI reconstruct run shape from the event stream, and either could reasonably assume created-precedes-terminal globally rather than per-step. Is that assumption made anywhere? Asking for confirmation rather than a change.
| }); | ||
| span?.setAttributes({ | ||
| ...Attribute.StepSkipped(true), | ||
| ...Attribute.StepSkipReason('completed'), |
There was a problem hiding this comment.
AI [suggested fix]: completed looks like the wrong value here, and reusing it costs the attribute its only useful distinction.
StepSkipReason is typed Step['status'] (semantic-conventions.ts:327). The pre-existing site further down this file sets completed on the EntityConflictError path whose comment reads "Step in terminal state, skipping" — accurate there. But this site's own comment says "a concurrent writer owns this step", which is most likely running, and the pair's 409 doesn't actually reveal the winner's status at all.
Those two are the only places StepSkipped / StepSkipReason are set anywhere in the package, so tagging both completed means the attribute reads 100% completed and cannot separate "skipped because already done" from "skipped because it lost the claim" — which is the question you'd query it for.
Suggest running, or omitting the reason since the verdict doesn't carry one.
| // name a pre-batch position and be answered with a | ||
| // skipped-slot report echoing the events this | ||
| // suspension just committed. | ||
| const batchSlotCeiling = |
There was a problem hiding this comment.
AI [note]: This fix is partial under the round-2 architecture, and the description reads as unconditional.
batchCommittedSlotCeiling only folds in slots from chunks that have committed, but the bodies start off the pair chunk while trailing chunks are still in flight. So on a multi-chunk fold, an inline terminal write issued before the trailing chunks land still names a position below them and still draws a skipped-slot report — the thing this change removes, partially reintroduced by the per-chunk deferral.
Bounded (trailing chunks only, big fan-outs only) and self-correcting, so not worth restructuring. Worth narrowing the claim to single-chunk folds so the next reader doesn't chase a report that is expected.
| 'workflow.batch.size': events.length, | ||
| 'workflow.batch.shape': [...counts] | ||
| .map(([type, count]) => `${type}:${count}`) | ||
| .join(','), |
There was a problem hiding this comment.
AI [suggested fix]: The shape string isn't canonical. It's built from a Map in first-seen order, so identical batch compositions emit step_created:17,step_started:3 or step_started:3,step_created:17 depending on frame order — and pre-claimed pairs change that order relative to a pure eager fold.
Sorting the entries before joining makes this a groupable dimension instead of a string every consumer has to parse and re-normalize.
| // Unreachable: the same prep op that enqueued the pair set | ||
| // this entry, and the flush awaited every prep above. | ||
| throw new WorkflowWorldError( | ||
| `no dehydrated input for pre-claimed step ${entry.correlationId}`, |
There was a problem hiding this comment.
AI [note]: Worth recording where this throw lands: the pair is already durable by this point, so the failure mode is "step claimed, body never runs, recovered on redelivery via owned-recovery" rather than "request fails cleanly". Fine for a defensive assert on an unreachable path — just worth a clause in the comment, since "unreachable" here still costs a redelivery rather than being free.
Was stacked on #3025 (now merged — this PR is restacked onto
main, with the review-round flush changes folded in: per-writerequestIdattribution oncreateBatch, and the seeded/advancing slot-bump expectation now shared with the pre-claim ceiling). Server needs nothing — #646 already ships the born-running fold (step_created+step_startedfor the same step in one batch → one running attempt-1 create).Motivation
In a 20-step fan-out, #3025 folds the 17 eager
step_createds into onecreateBatchPOST — but the 3 lazy-inline steps still fire individualstep_startedclaim POSTs (production trace: 356ms / 1.11s / 375ms each). Those claims are pure overhead on the batch path: the suspension already holds the dehydrated inputs, and the server can fold a[step_created, step_started]pair into one born-running create.What this does
1. Pre-claimed pairs in the suspension fold (
suspension-handler.ts). When the batched fan-out engages and has company for them, each lazy-inline step joins the batch as an adjacent pair: the created row carries the input, the started row is a bare claim stamped with the invocation'sownerMessageId(newSuspensionHandlerParamsfield) and per-eventcomputeInstanceId— the exact claim shape the lazystep_startedwould have sent, settled by the batch. Pair verdicts come back asSuspensionHandlerResult.inlineClaims:200→{ owned: true, step, batchPostSentAtMs, claimCompletedAtMs }— the readback entity (input re-attached locally, since batch responses return refs lazily);409→{ owned: false }— a concurrent writer owns the step.Pairs are never split across the 32-event chunk boundary (with the inline cap at 16 a straddle is structurally unreachable; the chunker refuses anyway should the constants diverge).
Eligibility: fold gate from #3025 ∧
ownerMessageIdpresent ∧ (≥2inline steps ∨≥1other batchable event). A lone inline step with nothing else to batch keeps the optimistic lazy path — a pair-only batch costs the same round trip as the single claim while giving up the claim/body overlap and bump-and-report.2. Pre-claimed mode in
executeStep(preclaimedStart: PreclaimedInlineStart).owned: falsereturns{ type: 'skipped' }before any write — the same outcome as losing the lazy claim (this also short-circuits the unregistered-step fallback: a step this handler doesn't own is not its to fail).owned: trueskips both start paths entirely and runs the body against the claimed step; the batch timestamps stand in for the claim's telemetry anchors (RSFS end, TTRstep_claim_ms), and the terminal write has no in-flight claim to reconcile — the 1.11s claim settlement the trace shows before a completion write is gone. Latency events tag a newpreclaimedStartoptimization.3. Bodies overlap the VQS publishes (
runtime.ts). The dispatch publishes and the inline executions now launch concurrently off the one commit point — previously bodies waited forawait Promise.all(dispatches). The failure contract is preserved by joiningdispatchesSettledbefore step results are read (and on the no-inline early return), after in-flight bodies settle — so a publish failure still redelivers, and no owned body is left running past the handler.4. Slot-snapshot ceiling (
batchCommittedSlotCeiling). The batch's own events aren't in the loaded log, so inline terminal writes used to name a pre-batch position and get answered with a skipped-slot report echoing the events this suspension just wrote (~batch-size events per completion POST on big fan-outs). The runtime now folds the batch's highest committed slot into the inline slot snapshot.5. World spec:
BatchEventRequest.computeInstanceId?: string— per-event compute attribution, same as the single create'sCreateEventParams;world-vercelthreads it into the frame meta (the server already forwards it to usage facts per frame).Round 2: parallel chunks + per-chunk continuation (from production trace feedback)
A 67-event fan-out trace showed the three batch chunks POSTing back-to-back (~230ms each), with no inline bodies and no queue messages until all three settled (~670ms). Rearchitected:
maxCommittedSlot − seed + 1 − committedCountis exactly the events other writers interleaved.stepDispatchplumbing, same message shape and step-identity idempotency key as the caller's dispatch pass, pre-reported throughqueuedStepCorrelationIdsso the caller skips them. Publish-after-create now holds per chunk rather than per fold.allowDeferredBatchWork(runtime opt-in) letshandleSuspensionreturn once the chunk carrying the inline pairs commits — bodies start off that — while trailing chunk commits + all publishes rideresult.deferredBatchWork, which the runtime joins next to the dispatch join before the invocation can ack. The durability contract (every create durable before ack) is unchanged; a trailing failure still fails the delivery, and the crash window is the same owned-recovery/idempotent-redispatch story the pairs already carry. The terminal drain doesn't opt in and keeps everything-durable-at-return.Expected trace shape after this: the N chunk POSTs overlap (~1 RTT total), chunk-1's bodies and each chunk's VQS publishes start at that chunk's commit, and the previously-empty ~450ms gap disappears.
OTel:
workflow.batch.size/ per-typeworkflow.batch.shapemoved from thehttp POSTspan to theworld.events.createBatchspan (set ininstrumentObject); the transport span keeps only wire-level facts (workflow.batch.bytes, transport) and no longer setsworkflow.event.type— that attribute names a single event write, and tagging a batch with its first event's type misclassifies traffic.Tests: +4 (concurrent POSTs asserted via gated mocks; pair-chunk-gated return with pending
deferredBatchWork; per-chunk publish timing, message shape + idempotency key; trailing-chunk failure surfacing through the deferred join; no-opt-in behavior unchanged) — 49 in the suspension-handler file; full core unit suite 2176 green.Semantics & trade-offs
WORKFLOW_BATCH_TRANSITIONS=0disables the whole fold, pairs included.Also sets up the executor mode the sequential deferral (
[completed(N), created(N+1), started(N+1)]at the next lazy start) will reuse.Testing
@workflow/coreunit suite: 2168 passed.@workflow/world-vercel: 508 passed. Typecheck green acrossworld/world-vercel/core. (packages/worldspec-version.test.tsfailure is pre-existing on the base commit.)🤖 Generated with Claude Code