Batch: pre-claim inline steps in the same batch - #3568
Conversation
🦋 Changeset detectedLatest commit: df0aabc 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✅ 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 🟠 world-sim scenario book — 1 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}
| 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.
There was a problem hiding this comment.
Answering rather than changing the gate, because the data to justify a change isn't there.
You're right that the round-trip argument doesn't survive the claims being concurrent. runtime.ts maps inlineExecutions and launches all of them, so N lazy claims are ~1 RTT wall-clock, not N. A pair-only batch of 2–3 saves no round trip and does give up the optimistic claim/body overlap. The description's reasoning generalises past N=1 in the wrong direction.
The tail argument you offer is the plausible one — one POST is a single sample where N concurrent claims are a max-of-N, and the trace's 356/1110/375 spread says the tail dominates. But it is unmeasured. The benchmark has no N=2..3 scenario: fanout-ttfs is Promise.all(100 steps), which always folds and always has eager company, so it cannot separate the pair-only case. Nothing on this PR tells us whether a 3-step Promise.all got faster or slower.
That matters more than it looks, because the benchmark shows a TTFS regression on the three scenarios that structurally cannot fold at all (posted separately). Until that is explained I would not touch this gate in either direction — narrowing it to the "≥1 other batchable event" disjunct is a behaviour change we would also be shipping unmeasured.
Concretely: this wants a Promise.all(3 steps) benchmark scenario before the ≥2 disjunct is either justified or removed. Filing that as the follow-up rather than guessing here.
There was a problem hiding this comment.
Correcting my earlier reply on this thread: I framed the trade-off as if optimistic inline start were always the baseline. It is not, and that changes the answer depending on configuration.
isOptimisticInlineStartEnabled() is false when unset. Turbo is on by default and passes forceOptimisticStart, but only under a narrow condition (runtime.ts):
const forceOptimisticStart =
turbo &&
!suspensionResult.hasAttributeEvents &&
!suspensionResult.waitTimeout &&
!suspensionResult.hasHookEvents &&
!suspensionResult.hasAwaitedHookCreation &&
!openHookWaitState.openHook &&
!openHookWaitState.openWait;So there are two regimes, and your critique lands differently in each:
Optimistic start active (turbo, no attr/wait/hook events, no open hook or wait). The body runs before the claim is confirmed. Folding into a pair gives up that overlap, and — as you argued — saves no round trip, because the N lazy claims already went out concurrently. This is the case where the >= 2 disjunct has a real cost and only the tail argument (one sample vs max-of-N) could justify it. Still unmeasured.
Optimistic start inactive (a wait timeout, an open hook or wait, WORKFLOW_TURBO=0, an explicit WORKFLOW_OPTIMISTIC_INLINE_START=0, or suppressOptimisticStart). The lazy path already awaits the claim before running the body, so there is no overlap to give up. Folding is close to free here, and the durable-running-before-body window the pairs introduce already exists on this path.
Worth noting the overlap with the fold's own gate is only partial: batchFanoutEligible excludes hook and attribute writes, so those never reach either mechanism — but waitTimeout and openHookWaitState are not in the fold's gate. A fan-out alongside a pending wait or an open hook therefore folds into pairs while optimistic start is off, which is exactly the regime where the fold costs nothing in overlap terms.
So the honest position on the gate: your objection is correct in the optimistic regime and does not apply in the awaited one. A Promise.all(3 steps) benchmark scenario would still settle it, and it should be run in both regimes rather than one.
| // 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.
There was a problem hiding this comment.
Checked both consumers; neither assumes it.
front trace viewer (packages/web-shared/src/lib/trace-builder.ts): groupEventsByCorrelation buckets events by correlationId, so the only ordering it consumes is per-step — which the pair preserves. cascadeSpans sorts by startTime (timestamps), not by slot or array position, and computeLatestKnownTime takes a max. Nothing reads log order.
ClickHouse (workflow_observability.workflow_runs_v1, fed by mv_workflow_runs_v1 from workflow_operations_raw_v2): every column is a commutative aggregate — sum/min/max/groupUniqArrayArray — and every argMax keys on a source timestamp, not on slot. latestRunEvent is argMaxIf by timestamp over run-level events only. workflow_operations_raw_v2 itself is one row per event with no ordering semantics. Usage facts are emitted per committed event server-side, so they inherit nothing either.
So the weaker contract holds for both. The one thing worth knowing for anyone reading those tables: a batch stamps one occurredAt for the whole chunk, so sibling events in a fold can share a timestamp and argMax ties resolve arbitrarily among them. That predates this PR (it came in with the fold in #3025) and no current column depends on breaking such a tie.
| }); | ||
| 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.
There was a problem hiding this comment.
You're right, and my own review pass got this wrong — I checked it against the other call site, concluded "consistent with the lazy path", and stopped. Consistency was the wrong test: both sites emitting completed is exactly what makes the attribute useless.
Changed to running in 53e933a. Took that over omitting so the two skip classes stay distinguishable in a query rather than one of them becoming an absent attribute. Comment notes that the 409 only proves the step exists and that its claim winner is the one executing.
| // 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.
There was a problem hiding this comment.
Fixed in 53e933a — narrowed in the batchCommittedSlotCeiling docstring: the echo is only fully suppressed for a single-chunk fold, and on a multi-chunk fan-out an inline terminal write issued before the trailing chunks land still names a position below them and still draws a report. Bounded and self-correcting, recorded so it reads as expected rather than as a bug.
| '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.
There was a problem hiding this comment.
Fixed in 53e933a — entries sorted by event type before joining, with a comment recording why (Map iteration is first-seen order, so a pre-claimed fold and a pure eager fold rendered the same composition as different strings).
| // 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.
There was a problem hiding this comment.
Added in 53e933a — the comment now says the pair is already durable at that point, so the throw lands with the step claimed and its body unrun, recovered on redelivery through owned-recovery rather than the request failing cleanly.
VaguelySerious
changed the title
VaguelySerious
changed the title
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: blocking issues found
| // nothing the caller's post-return work reads from the commits, | ||
| // so nothing gates. | ||
| if (pairChunkIndex >= 0) { | ||
| await commits[pairChunkIndex]; |
There was a problem hiding this comment.
AI Review: Blocking
A pair-chunk rejection escapes the handler while the trailing chunks are still in flight, which is the thing settlePhase exists to prevent.
On this path the foreground awaits only commits[pairChunkIndex]. When that rejects, the flush op rejects, settlePhase(ops) sees the failure, and handleSuspension throws — but trailing (the sibling chunk commits and every chunk's publishes) is only .catch(() => {})'d, never joined. deferredBatchWork is never handed to the caller either, since suspensionResult was never assigned, so nothing downstream can join it.
settlePhase's own docstring states why this matters: "a sibling create that lands after the rejection escaped commits an event whose correlation id came from the abandoned replay's seeded sequence … and it races the restart's reload while doing so." The runtime's catch (suspensionError) re-derives the replay in-process, so the restart runs concurrently with those writes.
This is new in the second commit. With sequential chunks a failed chunk meant later chunks never POSTed at all.
Verified locally (34 steps, one pair, gated createBatch, chunk 1 = pair chunk rejects with a 500 while chunk 2's POST is outstanding):
✓ escapes with the trailing chunk still in flight
[probe] published 3 step messages AFTER handleSuspension rejected
expect(trailingSettled).toBe(false) holds at the moment the handler rejects, and chunk 2 then commits its creates and publishes its 3 queue messages afterwards. The non-opt-in path (await trailing) is fine; only the opt-in path has the hole, and the runtime always opts in.
Settling before rethrowing looks sufficient:
if (pairChunkIndex >= 0) {
try {
await commits[pairChunkIndex];
} catch (err) {
await trailing.catch(() => {});
throw err;
}
}Same class of gap on the caller side: anything that throws in runtime.ts between the handler's return and the Promise.all([dispatchesSettled, deferredBatchWork]) join (assertNoInFlightOwnedSteps, computeStepLatencyTracking, the executor setup) abandons the deferred work too. Worth a comment at least about why that one is acceptable.
There was a problem hiding this comment.
Fixed in 1c64070. The allowDeferredBatchWork failure path now settles trailing before rethrowing:
try {
await Promise.all(pairCommits);
} catch (err) {
await trailing.catch(() => {});
throw err;
}Regression test added (settles the trailing chunk before a pair-chunk failure escapes) — it asserts probe(pending) === 'pending' and trailingSettled === false at the moment the pair chunk rejects, then that both flip once chunk 2 is released. Verified it fails on the pre-fix code (probe returns settled).
Left the caller-side gap alone: anything throwing in runtime.ts between the handler's return and the Promise.all([dispatchesSettled, deferredBatchWork]) join still abandons the deferred work. That one bottoms out in delivery failure plus redelivery, so it converges without a new barrier — flagging it here rather than changing it.
| // has the started row — processed second — decide, which is | ||
| // exactly the single path's semantics (create lost + claim | ||
| // won still runs the body; create won + claim lost skips). | ||
| inlineClaims.set(entry.correlationId, { owned: false }); |
There was a problem hiding this comment.
AI Review: Blocking
A transport retry of a pair chunk that already committed converges to 409 on both rows, and this branch reads that as "a concurrent writer owns the step" — so the invocation that actually owns it skips the body.
world-vercel's createBatch computes retryConvergent = true for a [step_created, step_started] pair (from #3025) and passes batchIdempotent: true to withEventPostRetry. So on UND_ERR_REQ_RETRY / a socket timeout / a transient 5xx after the transaction committed, the retry returns HTTP 200 with per-item 409s, and nothing in the response distinguishes "someone else created it" from "my own first attempt created it".
Result: owned: false → executeStep returns skipped before any write. The step is running, stamped with this message's ownerMessageId, and nobody is executing it. The caller does not dispatch it either (it is in inlineCorrelationIds). Recovery waits for the ownership lease: isStepOwnershipActive is true and step.ownerMessageId !== metadata.messageId on every later wake, so the dispatch pass arms a delayed backstop for stepLeaseRemainingSeconds — up to INLINE_OWNERSHIP_LEASE_SECONDS = 860.
The lazy path this replaces does not have that failure mode, deliberately: EVENT_RETRY_ELIGIBILITY.step_started.retryable === false, so a lost response fails the delivery, the message redelivers in seconds, and owned-recovery re-executes. The pair path converts that into a silent ~14-minute stall for the step.
Two directions, either works:
- drop
step_started-carrying batches fromretryConvergent(accept the redelivery, matching the single path's reasoning), or - keep the retry but make a pair
409on a retried batch fail the delivery instead of resolving toowned: false—withEventPostRetrywould need to report that it re-attempted.
The owned: false reading is only sound for a first attempt.
There was a problem hiding this comment.
Fixed in 1c64070, taking the first of the two options: retryConvergent in world-vercel now excludes any batch carrying a step_started, pair or not.
const retryConvergent = events.every(
({ event }) =>
event.eventType !== 'step_started' && event.eventType !== 'step_retrying'
);Trade-off worth naming: a transient blip on the pair chunk now fails the delivery even when nothing landed, where the retry would sometimes have absorbed it. That is exactly the single-POST path's existing trade-off for step_started, so it is no worse than the lazy path it replaces — but if the lost retry turns out to matter for a wide fan-out's first chunk, the follow-up is to have withEventPostRetry report that it re-attempted and fail only the ambiguous case.
The existing retries a transient 5xx in-process test used transitionEvents(), which contains a born-running pair, so it was retargeted at a creates-plus-terminal batch and a new test pins the pair case as single-attempt. Spec docstring in packages/world/src/events.ts updated too: the pair does converge, it is just not actionable for the caller, which is the part the old wording missed.
| if (failure) throw failure.reason; | ||
| })(); | ||
|
|
||
| const pairChunkIndex = chunks.findIndex((chunk) => |
There was a problem hiding this comment.
AI Review: Note
findIndex assumes at most one chunk carries pairs, and nothing enforces the constant coupling that makes that true.
Today it holds: lazyInlineCorrelationIds is the first getMaxInlineSteps() entries of stepItems, so pairs always occupy orders 0..2N-1 and sort to the front, and 2 * MAX_MAX_INLINE_STEPS === 32 === MAX_BATCH_FANOUT_EVENTS. So every pair lands in chunks[0].
If either constant moves — MAX_MAX_INLINE_STEPS up, or MAX_BATCH_FANOUT_EVENTS down to track a smaller server budget — pairs spill into chunks[1]. The chunker keeps each pair intact (that part is fine), but pairChunkIndex only gates on the first pair-carrying chunk, so a spilled pair returns no inlineClaims entry. runtime.ts then falls back to lazyStepInput for that step and executeStep sends a lazy step_started — racing this same invocation's still-in-flight deferred pair for the same step. Whichever loses gets a 409; if the lazy start loses, executeStep returns skipped, nobody runs the body, and it becomes the 860s-lease stall described in the other comment.
The PR description says "the chunker refuses anyway should the constants diverge" — it refuses to split a pair, which is not the same guarantee. Either gate on every pair-carrying chunk:
await Promise.all(
chunks.flatMap((chunk, i) =>
chunk.some((e) => e.kind === 'inline-started') ? [commits[i]] : []
)
);or add a static assert that 2 * MAX_MAX_INLINE_STEPS <= MAX_BATCH_FANOUT_EVENTS next to the chunker so a constant change fails loudly instead of degrading into the race.
There was a problem hiding this comment.
Fixed in 1c64070. Both halves:
findIndex→ aflatMapover every pair-carrying chunk, so the return gates on all of them.constants.test.tspins2 * MAX_MAX_INLINE_STEPS <= MAX_BATCH_FANOUT_EVENTSwith a comment explaining that raising the inline cap has to raise the chunk cap with it (and re-check the server's per-batch transaction budget). Went with a test rather than a type-level assert — the type version needed the literals hardcoded twice, which is worse than what it guards.
| ## The runtime integration (suspension fan-out fold) | ||
|
|
||
| **On by default.** The suspension handler folds a **clean fan-out** — the suspension's eager `step_created` and `wait_created` writes — into `createBatch` calls of at most 32 events (mirroring the server's transaction budgets). The fold only engages when the World implements `createBatch`, the run is on slot identity, and the suspension carries no attribute writes, no hook writes, and no resilient step dispatch; everything else keeps the single-event path byte-for-byte. Lazy-inline steps keep deferring their `step_created` to the lazy start exactly as before. | ||
| **On by default.** The suspension handler folds a **clean fan-out** — the suspension's eager `step_created` and `wait_created` writes — into `createBatch` calls of at most 32 events (mirroring the server's transaction budgets). Chunks of a larger fan-out commit **concurrently**: slot assignment is the World's, so parallel chunks race for slot ranges exactly like the pre-fold path's parallel single writes did, and per-entity conditions — not commit order — carry correctness. The fold only engages when the World implements `createBatch`, the run is on slot identity, and the suspension carries no attribute writes, no hook writes, and no resilient step dispatch; everything else keeps the single-event path byte-for-byte. |
There was a problem hiding this comment.
AI Review: Note
No changeset. This touches three published packages (@workflow/core, @workflow/world, @workflow/world-vercel) with a user-visible latency/behavior change, so it needs a real pnpm changeset, not --empty.
Separate coverage note while in this file: createBatch is implemented only by world-vercel, so the fold — and every pre-claimed pair path — is unreachable on world-local and world-postgres. No E2E Local * lane exercises this at all; the only end-to-end signal is E2E Vercel Prod / Preview. Worth stating in the changelog page, since it also means the kill switch (WORKFLOW_BATCH_TRANSITIONS=0) is the only lever if this misbehaves in production.
There was a problem hiding this comment.
Both done in 1c64070: changeset added (@workflow/core / @workflow/world / @workflow/world-vercel, patch), and this page now states that createBatch is Vercel-World-only so every other World keeps the single-event path and never sends a pair. Also documented the retry rule the other thread changed, since it is now user-visible behavior.
| * the claimed step. Mutually exclusive with `lazyStepInput`: the input | ||
| * already rode the pair's `step_created`, and the claimed step carries it. | ||
| */ | ||
| preclaimedStart?: PreclaimedInlineStart; |
There was a problem hiding this comment.
AI Review: Nit
"Mutually exclusive with lazyStepInput" is a comment, not a constraint — StepExecutorParams allows both. runtime.ts upholds it (the ternary sets one or the other), but the executor now has three s.lazyStepInput !== undefined || s.preclaimedStart !== undefined sites plus the params.preclaimedStart === undefined && term in optimisticStart that all quietly depend on it. A discriminated union on the start mode, or an assert(!(params.lazyStepInput && params.preclaimedStart)), would make a future caller's mistake a failure instead of a silently dead branch.
There was a problem hiding this comment.
Done in 1c64070 — assert in executeStep rather than a type refactor, since splitting StepExecutorParams into a discriminated union touches every call site for a constraint only one caller can violate.
Event Log Race Repro
Run History
Config14 runs / step-storm 6, hook-storm 6, hook-sleep 2 / c8 / 6x8 / watchdog 2500ms / step 2200±250ms / stagger 400ms / poke 750ms / poke max 64 / timeout 240000ms |
AI Review: BlockingBenchmark: the fan-out win is real and large, but three scenarios that cannot fold at all regress consistently, and I can't attribute it. I pulled the
All six PR observations (2 runs × 3 TTFS scenarios) sit above the maximum of 9 main samples. Fan-out p75 is 812/975 against a main median of 2440 — a ~60% improvement, which is the point of the PR and it clearly works. Why this is not the fold. So the cost is ambient rather than logic — deployed bundle size, module init, cold start — which cold-start-dominated TTFS would amplify. I did not localise it further; a workbench route-bundle diff is the next step if the numbers reproduce. What would settle it: the branch has since been rebased onto current main ( |
AI Review: BlockingThe benchmark on Baseline, Not failed writes. Production The likely trigger is that chunk commits are unbounded in width. I have not read the server's allocator under contention, so I'm not claiming where the gap is opened — only that the client-side change from serial to unbounded-parallel chunk commits is what distinguishes this run from the clean one, and that the failure is corruption rather than latency. Worth noting this also reframes the TTFS regression I posted above: at Suggested direction: cap the concurrency (a small pool — 2–4 in flight) rather than |
AI Review: Correction — retracting the previous commentThe event-log-hole finding above is not attributable to this PR. Retracting it. Main alone reproduces it, same signature, no PR code: What I got wrong: I compared against For whoever picks up the real issue: it hits 100-step and 1020-step fan-outs on main, holes are permanent rather than the partial read Consequence for this PR: the Benchmark lane is non-required and is currently red on main too, so it does not gate here. It does mean fresh benchmark numbers are unreliable until the platform settles — including the TTFS comparison I posted above, which I am leaving open rather than treating as resolved in either direction. |
|
AI Review: Note The 17 red checks on Those lanes ran between roughly 23:40 and 00:10 UTC, inside a Production is back to baseline (1 corrupted run per 5-minute bucket since ~01:50 UTC), so I've re-run both workflows on this head: The pre-incident |
AI Review: Blocking — TTFS regression confirmed on a healthy platformThe rerun on
~+390ms on The wins are equally real and worth stating: fan-out TTFS p75 846 (−54%) / p90 879 (−65%), fan-out TTLS −33%/−36%, CRTT control and sweep −39%/−41% avg, and STSO cumulative 240925ms → 206974ms (−14% over 1019 samples) with the whole histogram shifting left. This PR clearly does what it set out to do. What I have ruled out as the cause:
So it is something unconditional on the single-step path that I cannot identify by reading. The one unconditional runtime change left is the Suggested bisect: benchmark I would not merge this until that is explained. |
AI Review: Correction — the TTFS regression is a harness artifact, not this PRRetracting my TTFS blocking comment. I found the cause, and it is the measurement.
environment: ${{ github.ref == 'refs/heads/main' && 'production' || 'preview' }}
WORKFLOW_VERCEL_ENV: ${{ github.ref == 'refs/heads/main' && 'production' || 'preview' }}Main benchmarks a production deployment that carries continuous traffic; a PR benchmarks a freshly created preview deployment. TTFS is cold-start dominated, so the comparison is structurally biased against every PR — and the baseline artifacts I built my "main p90 1008–1316" band from are all production runs. Every open PR with a benchmark comment shows the same thing on
1587 sits inside the 1229–1690 band of six unrelated PRs. #3650 and #3645 make the mechanism plain: both got a lucky warm So there is no TTFS regression here, and my three-runs-agree argument was comparing preview against production the whole way. What made it look robust — reproducible, immune to code changes, present on scenarios where the fold cannot even engage — was exactly the signature of an environmental constant, and I read it as evidence of a subtle code path instead. The code reading also holds up now that I trace it properly: on the single-step path Separate issue worth filing on the tooling: the 🔴 TTFS markers fire on every PR, so they carry no signal and would mask a real TTFS regression. Either benchmark PRs against a warmed preview, or compare PR-preview against a preview baseline built from main, or drop the delta for cold-start-dominated metrics. Happy to open that separately — it is not this PR's problem to fix. With this withdrawn, what the run actually shows for this PR is the intended trade and no downside: fan-out TTFS p75 846 (−54%) / p90 879 (−65%), fan-out TTLS −33%/−36%, STSO cumulative 240925ms → 206974ms (−14% over 1019 samples), CRTT −39%/−41%. |
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.
Three fixes from review of the deferred/parallel-chunk fold. 1. A pair-chunk rejection escaped `handleSuspension` while the trailing chunks' commits and publishes were still in flight. `deferredBatchWork` never reaches the caller once the handler throws, so nothing joined that work — exactly the state `settlePhase` exists to prevent: a sibling create landing after the rejection commits an event from the abandoned replay's seeded sequence and races the caller's restart reload. The failure path now settles `trailing` before rethrowing. 2. Every pair-carrying chunk gates the return, not just the first. Pairs sort to the front and two rows per inline step fit inside one chunk, so this is one commit today, but `findIndex` silently degraded if either cap moved: a pair in an unawaited chunk yields no `inlineClaims` entry, the caller falls back to a lazy `step_started`, and that races this same fold's in-flight pair for the same step. constants.test.ts now pins the cap relationship. 3. A batch carrying a `step_started` is no longer retried in-process. The born-running pair does converge to a 409, but the pre-claim caller reads a pair 409 as "a concurrent writer owns this step" and skips the body — and on a retry that is indistinguishable from "my own first attempt committed the pair". Skipping there stranded a running step stamped with this invocation's own message id until the ownership lease expired (860s), where the single-POST path deliberately fails the delivery and recovers through owned-recovery in seconds. Same reasoning `EVENT_RETRY_ELIGIBILITY` already applies to `step_started`. Also asserts `lazyStepInput` / `preclaimedStart` mutual exclusivity in executeStep instead of only documenting it, and adds the changeset. Tests: +1 suspension-handler (pair-chunk failure settles the trailing chunk before escaping — fails without fix 1), +1 constants (cap relationship), +1 world-vercel (a born-running pair batch is single-attempt), and the existing batch-retry test retargeted at an entity-conditioned batch. Full @workflow/core unit suite 2178 green, @workflow/world-vercel 514 green, typecheck green across core / world / world-vercel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dispatch/deferred-batch joins now sit between the step promises' creation and the `Promise.all` that reads them, so a body rejecting in that window had no handler attached at the microtask checkpoint — an unhandledRejection, fatal under Node's default --unhandled-rejections=throw. A 412 fenced claim races exactly that window, and `deferredBatchWork` widens it by a trailing-chunk round trip. Attach a no-op catch at creation, the same way `dispatchesSettled` already does two lines up; the awaits below still decide the outcome. Review follow-ups: - `workflow.batch.shape` is sorted by event type. Map iteration is first-seen order, so a pre-claimed fold and a pure eager fold rendered the same composition as different strings, which is not groupable as a dimension. - A lost pre-claim reports StepSkipReason `running`, not `completed`. The pair's 409 says the step already exists and its claim winner is executing; the other skip site is a genuine terminal-state conflict, and tagging both `completed` left the attribute unable to separate the two. - `batchCommittedSlotCeiling`'s docstring now says the echo is only fully suppressed for a single-chunk fold: on a multi-chunk fan-out an inline terminal write issued before the trailing chunks land still names a position below them and still draws a report. - The defensive throw on a missing dehydrated input records where it lands — the pair is already durable, so it fails with the step claimed and its body unrun, recovered on redelivery via owned-recovery rather than failing cleanly. No regression test for the unhandledRejection: the existing inlineClaimRejectionScenario runs both steps inline, so `dispatches` is empty and the join resolves in a microtask — the window never opens and a test there passes with or without the fix. Reproducing it needs a scenario with a backgrounded step and a slow queue publish alongside the fenced claim. Full @workflow/core unit suite 2178 green, @workflow/world-vercel 514 green, typecheck and biome clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Batch encoding is a separate path from the single-event POST, so the frame meta had no coverage: the only assertion was at the World-call boundary. Adds a wire-level test that a pre-claimed pair's step_started half carries computeInstanceId in its frame meta and the step_created half does not. Verified it fails when the threading in createWorkflowRunEventBatch is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
world-local and world-postgres do not implement createBatch, so the fold never engages there — but the runtime passes ownerMessageId and allowDeferredBatchWork unconditionally. The existing "keeps the single path when the World lacks createBatch" test passed neither, so it never covered the pre-claim path at all. Assert the inertness with the params the runtime actually sends: no claims, no deferred work, no slot ceiling, the lazy-inline step still carrying its input, and no step_started reaching the world. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No backport to This is a latency optimization and new capability, not a defect fix: it adds pre-claimed To override, re-run the Backport to stable workflow manually via |
requestIdattribution oncreateBatchServer needs nothing, 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, and every pair-carrying chunk gates the return. Pairs sort to the front of the batch and two rows per inline step fit inside one chunk (
2 x MAX_MAX_INLINE_STEPS == MAX_BATCH_FANOUT_EVENTS, pinned byconstants.test.ts), so today that is one chunk — but the gate is a filter over all of them rather than afindIndex, because a pair in a chunk the caller never waited for yields no verdict and the caller would fall back to a lazystep_startedracing this fold's own in-flight pair.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: 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 behaviour unchanged.Semantics & trade-offs
step_startedruns single-attempt. The pair converges to a 409 on a transport retry, but that 409 is indistinguishable from "my own earlier attempt committed it", and reading it as a lost claim would skip a body this invocation owns — stranding arunningstep under its own ownership stamp until the lease expires. Same reasoningEVENT_RETRY_ELIGIBILITYalready applies to the single-POSTstep_started: fail the delivery, let redelivery recover through owned-recovery.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: 2195 passed (3 expected-fail).@workflow/world-vercel: 530 passed. Typecheck and biome green acrossworld/world-vercel/core.🤖 Generated with Claude Code
Review round 3 (fixes applied)
deferredBatchWorknever reaches the caller oncehandleSuspensionthrows, so the failure path settlestrailingitself before rethrowing — the invariantsettlePhasedocuments (a sibling create must not land during the caller's replay restart). Regression-tested; the test fails without the fix.findIndexon the first), plus aconstants.test.tsassertion pinning the cap relationship that makes "one chunk" true today.step_startedare no longer auto-retried (see Semantics above).unhandledRejection. The dispatch/deferred-batch joins now sit between the step promises' creation and thePromise.allthat reads them, so a body rejecting in that window had no handler at the microtask checkpoint — fatal under Node's default--unhandled-rejections=throw, and a 412 fenced claim races exactly that window. A no-op catch is attached at creation, the same waydispatchesSettledalready does.workflow.batch.shapeis sorted by event type, so the same composition renders one string (Map iteration is first-seen order, which differs between a pre-claimed fold and a pure eager one).StepSkipReason('running'), notcompleted, so the attribute can separate "already done" from "lost the claim".batchCommittedSlotCeiling's docstring narrowed: the skipped-slot echo is only fully suppressed for a single-chunk fold.computeInstanceIdreaches the v4 frame meta (and is absent when unset).lazyStepInput/preclaimedStartmutual exclusion asserted rather than only documented.Known gaps
unhandledRejectionfix. The existinginlineClaimRejectionScenarioruns both steps inline, sodispatchesis empty and the join resolves in a microtask — the window never opens and a test there passes either way. Reproducing it needs a scenario with a backgrounded step and a slow queue publish alongside the fenced claim.>= 2 inline stepsdisjunct is unmeasured. N lazy claims already go out concurrently, so a pair-only batch of 2-3 saves no round trip and gives up the optimistic claim/body overlap; the tail argument (one sample vs max-of-N) is plausible but untested, andfanout-ttfsis 100 steps with eager company so it cannot isolate the case. Wants aPromise.all(3 steps)benchmark scenario.createBatchisworld-vercel-only, so noE2E Locallane runs the fold at all (the pre-claim path's inertness on Worlds withoutcreateBatchis asserted insuspension-handler.test.ts, so local/postgres keep the lazy path with the runtime's real params).benchmarks.ymltargets a preview deployment on PRs and production on main, and TTFS is cold-start dominated. Six unrelated open PRs showttfs/stepp90 of 1229-1690 against the same baseline; this PR's 1587 is inside that band. The measured effects that ARE comparable within a run: fan-out TTFS p75 -54% / p90 -65%, fan-out TTLS -33%/-36%, STSO cumulative -14% over 1019 samples, CRTT -39%/-41%.