Backport #3139: fix(core): order step-result deliveries against wait/hook deliveries by event-log position - #3173
Open
github-actions[bot] wants to merge 2 commits into
Open
Backport #3139: fix(core): order step-result deliveries against wait/hook deliveries by event-log position#3173github-actions[bot] wants to merge 2 commits into
github-actions[bot] wants to merge 2 commits into
Conversation
…by event-log position (#3139) * fix(core): order step-result deliveries against wait/hook deliveries by event-log position Two production runs on `@workflow/core@5.0.0-beta.36` burned all three divergence-recovery replays at the same event and terminated with CORRUPTED_EVENT_LOG: wrun_41KYJENABV0GSF5YTE9EETV5DD (step vs wait) wrun_41KYJEE01S0GPC9RWT5MEKVCX8 (step vs hook) Replay divergence: step event step_created for step_X belongs to "A", but the current step consumer is "B" `useStep` proxies draw deterministic ULIDs in invocation order, so the ULID -> stepName allocation is a function of the order in which promise resolutions are delivered to workflow code. The delivery-barrier registry pinned that order to event-log position for hook payloads and wait completions, but step results were delivered straight off the serial `promiseQueue` — and their latency varies between replays of the SAME invocation, because the first replay pays full hydration while later replays memo-hit primitive results in the shared `ReplayPayloadCache`. A step completion adjacent in the log to a `wait_completed` was therefore delivered wait-first on a cold replay and step-first on a warm one; whichever order the invocation that wrote the follow-up `step_created` events happened to see became law, and every replay computing the other order diverged permanently. Step results and step failures now register a 'step' delivery barrier at their event-log index and resolve from a detached continuation after every relevant earlier-in-log delivery, mirroring the hook payload path: hydration stays inside the serial queue slot (which also releases `pendingDeliveries`), while the barrier wait and the resolve run off the queue so a queue slot never blocks on a resolution the queue itself drives. Waits and hook payloads likewise defer behind earlier step results. Two details are what actually make the ordering hold, and both were found by testing rather than by reading the code: The deferral set is captured while CONSUMING the event, not at the start of the hydration slot. Captured at slot start it is not merely less deterministic, it is usually empty: an earlier delivery whose own slot runs first on the serial queue has typically already resolved and deregistered its barrier before the later slot begins, so the later delivery does not defer at all. Every event in one drain window is consumed before any slot runs, so consumption time sees all of them. A delivery that had to wait then yields a macrotask before resolving. An earlier delivery being "delivered" only means its `resolve()` ran; the branch it woke may need arbitrarily many further microtask hops before it reaches its next `useStep` call (a `for await` over a hook resumes the generator, settles the promise from `next()`, and only then runs the loop body). Ordering the `resolve()` calls alone therefore buys a fixed hop or two of margin and leaves a hop-count race that holds only for the shortest consumers; yielding a macrotask lets the earlier branch drain completely, whatever its shape. One asymmetry is load-bearing: a step result skips any earlier delivery that will not resolve on its own, i.e. one blocked directly or transitively on a buffered hook payload no consumer has claimed. Such a payload is delivered only when the workflow next reads the hook, and reaching that read commonly requires the step result itself, so gating the step on it stalls the run until the barrier's idle safety net fires — which then releases every delivery queued behind that payload at once and loses the very race the ordering exists to protect. Waits and hooks keep gating on unclaimed payloads, where waiting for the claim IS the guarantee. Tests come in two files. `step-delivery-ordering.test.ts` is byte-identical to the file in the repro-only companion PR #3137 apart from two `it.fails` markers there (which let a repro-only branch have green CI); `sed 's/it\.fails(/it(/g' | cmp` verifies it. Each of its five cases replays one committed log twice through a shared `ReplayPayloadCache`, and the two warm-replay cases fail on main with the production error text. `step-delivery-hop-count.test.ts` exists because those five cases cannot tell "delivered in log order" apart from "resolves a hop or two later than before". It replays logs a live run legitimately produced — the live invocation received the two events in separate deliveries, so the first branch finished long before the second event existed — while the replay receives both in one drain window, and pads the consumer with a varying number of extra awaits so hop count is the only variable. It covers step results against both wait completions and hook payloads, plus step FAILURES against wait completions, since a rejection decides whether a `catch` continuation runs and so which ULID the `useStep` there draws. All 18 cases fail on main; of the 12 that predate the macrotask, 9 still fail with the resolve-ordering-only version of this fix; all 18 pass here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com> * fix(core): close remaining delivery-barrier ordering gaps Follow-up on the step-delivery barrier work, addressing three cases the registry did not yet cover. Each has a regression test in the new `delivery-barrier-coverage.test.ts` that reproduces the production `ReplayDivergenceError` when its fix is reverted. - Step results now defer behind earlier STEP results. The old exclusion assumed the serial `promiseQueue` fixes step-vs-step order, which stopped holding once a step began resolving from a detached continuation instead of its queue slot: two steps consumed in different drain windows can disagree on their deferral set, and the earlier one — parked on the macrotask yield — gets overtaken. - `sleep.ts` and `hook.ts` (waiting-consumer path) now capture their deferral at event-consumption time, as `step.ts` already does. Reading the registry after their queue work misses an earlier step or hook that delivered and retired its barrier in the meantime, skipping both the gate and the macrotask yield. The buffered hook payload path deliberately keeps evaluating at claim time; a consumption-time snapshot there stalls the e2e `hookWithSleepWorkflow`. - Abort deliveries participate in the registry. `_setAborted` fires the signal's listeners, which may invoke a step and draw a ULID, so an abort is as branch-deciding as any other delivery. Also memoizes `resolvesOnItsOwn`. The walk is exponential in the number of live hook/wait barriers, and the registry is not bounded — a fan-out of `Promise.race([hook, sleep])` branches accumulates one barrier per branch per kind (49 measured for 24 branches). At 40 barriers a single scan took 92s before, and is instant after. --------- Signed-off-by: Pranay Prakash <pranay.gp@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Nathan Rajlich <n@n8.io> Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
🦋 Changeset detectedLatest commit: f3f7f81 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Contributor
Author
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (102 failed)redis (19 failed):
turso (83 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
TooTallNate
approved these changes
Jul 29, 2026
TooTallNate
left a comment
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Faithful backport. I verified it mechanically rather than by reading, and pushed no fixes — nothing needed changing.
Method
For each file I compared the change #3139 made on main against the change this PR makes on stable, hunk by hunk (diff of the +/- lines of git diff 0d6ec4387 2941b1c36 -- <file> vs git diff stable backport -- <file>), so pre-existing main↔stable divergence cancels out and only genuine backport deltas remain.
| file | result |
|---|---|
workflow/hook.ts |
byte-identical change |
workflow/sleep.ts |
byte-identical change |
private.ts |
identical but for 3 doc lines |
step.ts |
4 deltas, all necessary — below |
step-delivery-ordering.test.ts |
API renames only, no logic changed |
step-delivery-hop-count.test.ts |
API renames + error-shape adaptation |
delivery-barrier-coverage.test.ts |
abort case removed + API renames, nothing else |
.changeset |
drops "aborts" |
Every deviation, and why each is right
Abort dropped. packages/core/src/workflow/abort-controller.ts doesn't exist on stable, so the abort hunk, the abort describe, the changeset mention, and the "three cases" docblock (now "two", with an explicit note that the case isn't portable) are all correctly removed. I checked this is a complete omission rather than a partial one: stable has no abort/signal source files at all, and the only AbortController string in packages/core/src is an unrelated doc mention in step-hydration-cache.ts. pendingDeliveries++ appears only in hook.ts and step.ts on stable, so there's no third delivery participant that should have been wired in.
ReplayPayloadCache → stepHydrationCache. stable has no replay-payload-cache.ts; step_completed correctly uses getOrHydrateStepReturnValue(ctx.stepHydrationCache, …) in place of ctx.replayPayloadCache.getStepResult(…). Renames are applied consistently through comments and test harnesses. Zero dangling references — I grepped the changed files for ReplayPayloadCache, replayPayloadCache, prepareEventPayload, primitiveStepResults, getStepResult, hydrateStepError, dehydrateStepError, abort-controller, _setAborted: all absent.
step_failed restructured, not weakened. This is the delta worth the most scrutiny, because stable's handler is a different shape: it builds a FatalError synchronously from eventData.error.message/stack, where main awaits hydrateStepError. The backport keeps the semantics that matter — barrier registered as 'step', deferral captured at event-consumption time, reject moved into the detached earlierDelivered.then(…) — and correctly omits pendingDeliveries++/--, since there's no async hydration window to hold the counter across. That matches main's effective behaviour anyway: there the counter is released in the slot's finally, before the detached defer, so both branches sit at 0 during the defer window.
One comment rewritten. stable has a stepHydrationCache comment claiming the cache lookup happens in the queue slot "(and still resolves via resolve)". The backport drops that parenthetical, which the fix makes false. main never had it. Good catch by whoever resolved this — easy thing to leave stale.
dehydrateStepError → plain { message, stack } in the hop-count fixture, matching what stable's handler actually reads.
CorruptedEventLogError → ReplayDivergenceError in one docblock line. Both types exist on stable; this preserves the wording already there rather than importing main's. Fine either way, and it's a comment.
The tests are still real guards
The failure mode I was most worried about in an AI-resolved backport is a test adapted into something that passes trivially. So I reverted each fix individually on this branch and confirmed the corresponding guard fires:
| revert | result |
|---|---|
DEFER_BEHIND.step drops 'step' |
step-vs-step test fails |
sleep.ts deferral read late again |
wait hop tests fail at 8/20/50 |
hook.ts deferral read late again |
hook hop tests fail at 8/20/50 |
resolvesOnItsOwn memo removed |
scan guard takes 90.6 s (vs instant) |
step_failed detached reject removed |
all 6 step-failure hop cases fail |
step_completed detached resolve removed |
15 failures across all three files |
Also: no it.fails / it.skip / .only anywhere in the three new test files, and the suite's 3 expected-fails are pre-existing on stable (workflow.test.ts, hook-sleep-interaction.test.ts) — same two files as before the backport.
Local verification
Build and typecheck clean. packages/core unit suite 43 files, 824 passed | 3 expected fail. The 12 ordering-sensitive files run 8× consecutively, 209 passed | 3 expected fail every time. Biome reports the same 5 pre-existing complexity warnings as the unmodified stable, none new.
Note on CI — it had not run
Worth flagging beyond this PR: every workflow here was sitting in action_required because the PR was opened by github-actions[bot], so Tests, Lint, Docs Checks and Event Log Race Repro had never executed. The only green checks were Socket, DCO and the Vercel deploys, which is easy to misread as "CI passing". I approved the runs.
Post-approval, everything relevant is green: Unit Tests on both ubuntu and windows, Vitest Plugin Tests, Node.js Module Build Errors, all E2E Local Dev / Local Prod / Local Postgres apps, all 11 E2E Vercel Prod apps, E2E Windows, E2E Required Check, and Lint.
The only failures are E2E Community World (Turso), (Redis) and (MongoDB) — which fail and cancel identically on each of the last four stable pushes, including at this PR's merge base 99d1a0ce1. Pre-existing and unrelated.
The later Merge branch 'stable' commit only pulled in unrelated stable work (version packages, world-vercel timeouts); all 8 backported files are byte-identical to what I reviewed at 61d0fbcc1.
Approving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated backport of #3139 to
stable(backport job run).AI recommendation: This is a correctness fix for a production data-loss class bug: step results were delivered outside the delivery-barrier registry, so a memo-warm replay could deliver them ahead of an earlier-in-log
wait_completed/hook_received, permanently diverging the ULID allocation and terminating runs withCORRUPTED_EVENT_LOG. I verified the affected code exists onstable:packages/core/src/private.tsthere haspendingDeliveryBarrierswithDeliveryKind = 'hook' | 'wait', andstep.tsresolvesstep_completedstraight from itspromiseQueueslot behind a primitive-result memo (stepHydrationCache), i.e. the same latency asymmetry and the same gap. Note for the human resolving the cherry-pick:packages/core/src/workflow/abort-controller.tsdoes not exist onstable, so that part of the diff (and the abort case in the new coverage test) must be dropped rather than applied.Merge conflicts were resolved by AI (opencode with
anthropic/claude-opus-5). Please review the conflict resolution carefully before merging.