fix(core): preserve event-log order in hook-vs-sleep replay races - #2171
Conversation
🦋 Changeset detectedLatest commit: 9fe8530 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a deterministic-replay divergence in @workflow/core where a buffered hook_received payload could lose a Promise.race against a concurrently-resolving sleep (wait_completed) during replay, even when the committed event log indicates the hook branch won. The change anchors buffered hook payload delivery to its event-log position and introduces a cross-entity ordering barrier so later-in-log sleeps defer until earlier hook deliveries are observed.
Changes:
- Anchor buffered
hook_receivedpayload hydration to the payload’s log position viactx.promiseQueue(instead of scheduling at claim/iterator.next()time). - Add
ctx.pendingHookDeliveriesandawaitEarlierHookDeliveries()to deferwait_completedbehind earlier-in-log buffered hook deliveries. - Add characterization/regression tests covering hook-vs-sleep, hook-vs-step, and two-hook ordering under slow async hydration.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/workflow/sleep.ts | Defers wait_completed resolution behind earlier buffered hook deliveries. |
| packages/core/src/workflow/hook.ts | Reworks buffered hook payload handling to resolve at log position and register ordering barriers. |
| packages/core/src/private.ts | Adds pendingHookDeliveries to context and implements awaitEarlierHookDeliveries(). |
| packages/core/src/workflow.ts | Initializes pendingHookDeliveries in the runtime context. |
| packages/core/src/hook-sleep-interaction.test.ts | Adds regression/characterization tests for ordering across hook/sleep/step races. |
| packages/core/src/workflow/sleep.test.ts | Updates test harness context initialization with pendingHookDeliveries. |
| packages/core/src/step.test.ts | Updates test harness context initialization with pendingHookDeliveries. |
| packages/core/src/async-deserialization-ordering.test.ts | Updates test harness context initialization with pendingHookDeliveries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests▲ Vercel Production (1 failed)hono (1 failed):
🌍 Community Worlds (69 failed)mongodb-dev (1 failed):
redis-dev (1 failed):
turso-dev (1 failed):
turso (66 failed):
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
One blocking replay-ordering case remains; details inline.
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Additional blocking findings from local replay and release verification; details inline.
A buffered hook payload (a `hook_received` consumed before the workflow awaited the hook) was delivered to its consumer only at claim time (`iterator.next()`/`await`), and a concurrent `wait_completed` resolved synchronously with fewer microtask hops. When the workflow did `Promise.race([hook, sleep])`, sleep could win a race the committed event log says the hook won, surfacing as `CorruptedEventLogError` on replay (observed in production: a `step_created` for one step consumed by a different step's consumer). Fix, in three timing-independent parts: 1. Resolve a buffered hook payload through a `promiseQueue` slot chained at its log position (not at the later claim site), so resolution order stays anchored to the event log regardless of hydration/decryption time. 2. Register a per-delivery ordering barrier keyed by the source `hook_received` eventId (`ctx.pendingHookDeliveries`). A later-in-log entity (sleep's `wait_completed`) defers behind any earlier-in-log in-flight hook delivery via `awaitEarlierHookDeliveries`. 3. Release the barrier on a MACROTASK (`setTimeout(0)`) after the payload is claimed, so the consumer's branch decision — however many await hops deep through the async hook iterator — always commits before the deferring entity proceeds. This reuses the macrotask-boundary technique `scheduleWhenIdle` already relies on and is fully hop-count- and decryption-time independent (no microtask-hop heuristic). `awaitEarlierHookDeliveries` bounds its wait with a one-macrotask fallback so an unclaimed payload cannot deadlock a deferring entity. Adds characterization tests covering hook-vs-sleep (the repro), hook-vs-step, and a two-hook slow-decrypt ordering case. Pre-existing runtime bug on `stable`, independent of the OCC/fence work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Address review feedback on the hook-vs-sleep replay fix:
- Order branch-deciding deliveries (buffered hook payloads + wait
completions) by their event-log INDEX, not by `eventId` string compare.
`eventId` is world-assigned and not guaranteed to sort in creation order
(only the bundled ULID worlds do), so the prior comparison mis-ordered
against worlds like the Vercel world. (VaguelySerious/vercel-bot: blocking)
- Make ordering bidirectional and type-aware: a hook delivery is handed to
the workflow only after every earlier-in-log wait, and a wait only after
every earlier-in-log hook. Same-kind deliveries do not block one another
(sequential hook payloads stay unblocked). The gate is "the earlier
delivery resolved", not "won a timing race", so the outcome is independent
of microtask hops, hydration time, and `Promise.race` argument order.
- Do not reject a buffered payload promise before a consumer attaches a
handler. Capture the hydration outcome and build the consumer-facing
promise at claim time, so an unclaimed encrypted payload with no key no
longer crashes the process with an unhandled rejection. (pranaygp: P1)
- Retire delivery barriers when delivered, and at idle if never delivered,
so `pendingDeliveryBarriers` cannot retain an entry per abandoned payload.
(pranaygp: P2)
- Make `pendingDeliveryBarriers` optional and route all access through
guarded helpers, removing the required-vs-optional mismatch. (Copilot)
- Add a patch changeset for `@workflow/core`. (pranaygp/VaguelySerious)
Turns the forward-direction reproducers green (removes `.fails`). Adds
Pranay's two regression tests (unhandled-rejection, barrier leak). The
remaining `it.fails` ("...when wait completion wins") asserts an
unproducible replay — its reused sleep is already resolved by the time the
loop's `Promise.race` runs, so the resolved hook always wins the array-order
tie; documented inline and flagged for rework/removal.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sit-rep: review feedback addressed (pushed in 9fe8530)Rebased onto latest What changed
ReproducersThe forward-direction
|
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
| // hydration/ordering work runs in event-log order. | ||
| const queueAtCompletion = ctx.promiseQueue; | ||
| void queueAtCompletion | ||
| .then(() => awaitEarlierDeliveries(ctx, eventIndex, ['hook'])) |
There was a problem hiding this comment.
AI Review: Note
The ordering here is happens-before correct, and worth recording why it's robust: a wait_completed resolves only after awaitEarlierDeliveries(['hook']) has settled every earlier-in-log hook barrier, and a buffered hook resolves only after every earlier-in-log wait barrier. Because the gate is "the earlier delivery resolved" (a causal edge) rather than "won a microtask race", the winner of Promise.race([hook, sleep]) no longer depends on hydration time, await-hop depth, or race-argument order. I traced hook-vs-step (unchanged — steps register no barrier, and the hook's added hop still schedules its resolve ahead of an adjacent step slot) and wait-vs-wait (preserved via registration order on the shared/earlier queue tail); neither regresses. Detaching the resolve from the serial promiseQueue is necessary here (gating a queue slot on an earlier delivery whose own resolution is queue-driven would deadlock the queue) and the reasoning holds. No change requested.
| // branch was not taken / the run is suspending), resolve at idle so a | ||
| // later opposite-kind delivery gated on it cannot deadlock and the | ||
| // registry cannot leak an entry per abandoned delivery. | ||
| scheduleWhenIdle(ctx, finish); |
There was a problem hiding this comment.
AI Review: Nit
Every hook_received and wait_completed now registers an idle safety-net poll via scheduleWhenIdle. This is correct — it prevents a later opposite-kind delivery from deadlocking behind an abandoned one, and the barrier-leak regression test confirms entries are reclaimed. Minor cost only: a long-lived hook loop consuming thousands of payloads registers O(n) setTimeout(0) polls over the run's lifetime (one per payload, each firing once). Not a leak and almost certainly negligible, but worth a glance if a high-throughput hook workflow ever shows up in a timing-sensitive benchmark.
| // hand-built log encodes (`progressStep` at evnt_14). The fix for the real | ||
| // hook-vs-sleep ordering bug is verified by the sibling tests; this case | ||
| // should be re-worked (e.g. so the sleep is not pre-resolved at the race) | ||
| // or removed. See PR discussion. |
There was a problem hiding this comment.
AI Review: Nit
Agreed with the inline rationale that this it.fails asserts an outcome the workflow can't actually produce (the reused sleep is already resolved at the race, so the array-order tie-break picks the hook). Beyond being dead weight it's a fragile guard: if a future change ever makes the sleep branch reachable in this exact log, the body's assertions would pass and it.fails would itself start failing CI for what is arguably a correct change. Recommend reworking it so the sleep isn't pre-resolved at the race (e.g. order wait_completed after the second hook_received) or removing it. For what it's worth I wrote a clean, reproducible "sleep wins" case (a fresh sleep whose wait_completed is earlier in the log than the hook_received) — it passes on this branch in both sync and async-deserialization modes, and also passes on the base, which corroborates that the sleep-wins direction was already correct and that the real defect is the hook-should-win-but-loses direction the sibling repros cover.
|
AI Review: Note Context on the red Event Log Race Repro check, since it's the headline gate for this change — it is not blocking:
Suggestion (follow-up, not for this PR): make the resume land deterministically inside the loop window — e.g. bind it to elapsed iterations rather than a wall-clock delay that sits right at the total loop duration — so the gate stops flaking and a future real regression isn't masked by ambient noise. The other two red checks are unrelated: E2E Vercel Prod Tests (hono) failed on an output-stream test ( Validation done locally on |
Summary
Fixes a replay divergence where a buffered hook payload races a concurrent
sleep, andsleepcan win aPromise.racethat the committed event log says the hook won — surfacing asCorruptedEventLogErroron replay (seen in production as astep_createdfor one step being consumed by a different step's consumer).Fixes tests introduced in #2169
Root cause
hook_receivedconsumed before the workflow awaited the hook) was delivered to its consumer only at claim time (iterator.next()/await).wait_completedresolved synchronously in itspromiseQueueslot — no hydration, fewer microtask hops — while the hook payload reaches the consumer through the async hook iterator (yield await this), which adds hops.Promise.race([hook, sleep]), sleep could therefore preempt an earlier-in-log hook payload, diverging from the committed log.This is specific to hook-vs-sleep: hook-vs-step and two-hook ordering already resolve correctly (verified by the added characterization tests), because the existing serial
promiseQueuediscipline is decryption-time independent for those.Fix (three timing-independent parts)
promiseQueueslot chained at its log position (not at the later claim site), so ordering follows the event log regardless of hydration/decryption time.hook_receivedeventId (ctx.pendingHookDeliveries). A later-in-log entity (sleep) defers behind any earlier-in-log in-flight hook delivery viaawaitEarlierHookDeliveries.setTimeout(0)) after the payload is claimed, so the consumer's branch decision — however many await hops deep — always commits before the deferring entity proceeds. This reuses the same macrotask-boundary techniquescheduleWhenIdlealready relies on; no microtask-hop heuristic.awaitEarlierHookDeliveriesbounds its wait with a one-macrotask fallback so an unclaimed payload can't deadlock a deferring entity.Why timing-independent
Decryption time, hydration time, and consumer
await-chain depth are all irrelevant: a macrotask runs only after the entire pending microtask queue drains. Empirically validated by modeling the race (the iterator path costs 3 microtask hops; a fixed-K microtask release was fragile — the macrotask boundary is not).Tests
hook-sleep-interaction.test.ts: hook-vs-sleep (the repro, both sync + async-deser modes), hook-vs-step, two-hook slow-decrypt ordering.@workflow/coresuite: 630/630, typecheck clean, stable across repeated runs, no hangs.Scope
Pre-existing runtime bug on
stable; independent of the OCC / fenced-write work, so it can ship on its own.Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com