Uh oh!
There was an error while loading. Please reload this page.
[codex] Forward port stale wait replay fix - #2038
Merged
Merged
Conversation
Contributor
🦋 Changeset detectedLatest commit: d2f724a 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 |
Contributor
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
Contributor
There was a problem hiding this comment.
Pull request overview
Forward-ports the “stale wait replay” race fix to main by making the V2 runtime refresh its event snapshot after committing elapsed wait_completed events, ensuring concurrent durable events (e.g. hook_received) are observed in deterministic order during replay.
Changes:
- Extends
EventResultto optionally includecursor/hasMorepagination metadata for preloadedrun_startedevent snapshots. - Updates runtime replay to (a) only trust preloaded events when
hasMore !== true, and (b) refresh events after elapsed wait completions using cursor-delta fetch with a full-reload fallback. - Adds a deterministic handler-level regression test for the hook-vs-sleep interleaving and ships a patch changeset.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/world/src/events.ts | Adds optional cursor/hasMore fields to EventResult for preloaded event snapshots. |
| packages/world-vercel/src/events.ts | Accepts/forwards cursor and hasMore from the backend in events.create responses. |
| packages/world-postgres/src/storage.ts | Populates cursor/hasMore for run_started preloaded events. |
| packages/world-local/src/storage/events-storage.ts | Returns preloaded events with pagination metadata (and caps preload size). |
| packages/core/src/runtime.ts | Uses preload pagination metadata and refreshes event snapshots after completing elapsed waits to preserve replay determinism. |
| packages/core/src/runtime/wait-completion-replay.test.ts | New deterministic regression test that simulates the stale snapshot + concurrent hook ordering race. |
| .changeset/bright-waits-refresh.md | Patch bumps for affected packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1107
to
1108
| // For run_started: include all events so the runtime can skip | ||
| // the initial events.list call and reduce TTFB. |
ContributorAuthor
There was a problem hiding this comment.
Updated the run_started preload comment to call out the one-page preload and hasMore check.
Comment on lines
748
to
+752
| for (const waitEvent of waitsToComplete) { | ||
| try { | ||
| const result = await world.events.create( | ||
| runId, | ||
| waitEvent, | ||
| { requestId } | ||
| ); | ||
| events.push(result.event!); | ||
| await world.events.create(runId, waitEvent, { | ||
| requestId, | ||
| }); |
ContributorAuthor
There was a problem hiding this comment.
Updated the dedupe comment to describe the refreshed wait-completion delta path instead of the old manual push behavior.
TooTallNate
approved these changes
May 20, 2026
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Approve — clean forward-port of #2029, with V2-specific adaptations
Forward-ports the stable wait-replay race fix to main. The diff against current main looks larger than the stable PR because the branch was forked before #2030 (which added optional stepName/token/resumeAt fields to several event schemas). After rebasing onto current main, the actual change is +593/-15 across 7 files, all consistent with what landed on stable.
Why the V2 adaptations are correct
I traced both versions side by side to make sure nothing was lost in translation:
- Cursor preservation on first iteration (
preloadedEventsCursorplumbed through toeventsCursor) — symmetric with stable. - Wait-completion refresh has TWO additions on main vs stable:
- Dedupe by
eventIdwhen merging delta back into existing events (line 803-810). On stable this is justevents.push(...newEvents.events). On main it's defensive against the V2 multi-iteration loop whereeventsmay persist across iterations ascachedEvents. eventsCursor = loaded.cursor ?? eventsCursor(line 811). Critical for the V2 loop — without it, the next iteration'sloadWorkflowRunEvents(runId, eventsCursor)would re-fetch the SAME delta. Stable's loop doesn't have this concern because it doesn't iterate.
- Dedupe by
stripEventDataRefsround-trip throughresolveDatais identical to the existingevents.createflow — no new code paths.
Correctness walk-through
Scenario: workflow with Promise.race([hook, sleep]), hook arrives durably just before handler commits wait_completed.
- Handler reads events at cursor
C. Sees elapsedwait_created, nohook_received. - Before handler commits
wait_completed: another writer commitshook_received(durable position > C). - Handler commits
wait_completed(durable position >hook_received). - Without fix: in-memory
eventsmissinghook_received. Replay takessleepbranch. Wrong. - With fix: refresh from cursor
C, get[hook_received, wait_completed]. Append. Replay takeshookbranch. Correct.
The sawAllWaitCompletions defensive check (line 795-800) is smart — if the delta-after-cursor doesn't include the wait completion this handler just wrote, the cursor wasn't a safe replay boundary (read replica lag, eventual consistency hiccup, etc.) and we fall back to a full reload from the beginning.
Test coverage
The four scenarios in wait-completion-replay.test.ts map cleanly onto the code paths:
| Test | Path exercised |
|---|---|
loads only events after the preloaded cursor | Happy path: preload + cursor + complete |
falls back to a full reload when preloaded events do not include a cursor | Backward compat: old worlds without cursor |
falls back to a full reload when preloaded events are partial | hasMore: true → discard preload, full reload |
falls back when cursor delta misses the attempted wait completion | Defensive: sawAllWaitCompletions fails |
Each scenario asserts both the number of listEvents calls AND the cursor used on each call, so the code paths are pinned, not just outcomes.
What I verified
- After rebasing onto
main: 7 files changed, +593/-15. Theevents.tsschema "removals" in the raw diff againstmainare artifacts of the branch being behind #2030 — they vanish on rebase. pnpm --filter @workflow/core test✓ (965/965 including the new 4 inwait-completion-replay.test.ts)pnpm --filter @workflow/core test packages/core/src/hook-sleep-interaction.test.ts✓ (the seeded-repro test that was deleted on stable still passes here — main keeps both old + new tests)- Typecheck for core, world, world-local, world-postgres, world-vercel ✓
- Build ✓
- CI: 107 success, 1 failure, 4 in-progress — the failure is
Benchmark Vercel (nextjs-turbopack), which is unrelated (benchmark timing) andmainis also failing it on recent commits. All E2E + Unit Tests pass.
Two minor nits (non-blocking)
world-postgreshasMore: falseis hardcoded (line 1291). The query has noLIMIT, so this is correct today. If someone ever adds aLIMIT N+1pattern to that query for pagination, the hardcodedfalsewould silently lie. Worth a comment like// hasMore = false: query has no LIMIT, so we always return all eventsto make the assumption explicit. Compare to the local world, wherehasMoreis derived from the paginated query.The dedupe comment in the iteration-N+1 fetch path (line 650-654) reads as if the cursor isn't advanced after the wait-completion refresh. But this PR DOES advance it (line 811:
eventsCursor = loaded.cursor ?? eventsCursor). The dedupe is now effectively defensive against worlds that don't always advance cursors (e.g., returnnullwhen no new events) — which is what makes it still worthwhile, but the comment could say so. Minor; understandable as-is.
Backport stance
Body says "this PR is itself a forward port from stable, so it should not be backported." Agreed — the backport action's AI should reach the same conclusion (the fix already exists on stable as #2029), and if it doesn't, manual override is fine.
Approving.
Contributor
No backport to This PR is explicitly a forward-port from To override, re-run the Backport to stable workflow manually via |
This was referenced May 20, 2026
5 tasks
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Forward-port the stale wait replay race fix from stable PR #2029 to
main.This keeps workflow replay deterministic when an elapsed
wait_completedis committed from a stale event snapshot while a concurrenthook_receivedevent landed durably before it. Onmain, the fix is adapted to the V2 replay loop/event cache:cursor/hasMoremetadata from preloadedrun_startedevent snapshotsForward-Port Note
This PR is itself a forward port from
stable, so it should not be backported.Validation
pnpm --filter @workflow/world buildpnpm --filter @workflow/utils buildpnpm --filter @workflow/errors buildpnpm --filter @workflow/serde buildpnpm --filter @workflow/world-local buildpnpm --filter @workflow/world-vercel buildpnpm --filter @workflow/core buildpnpm vitest run packages/core/src/runtime/wait-completion-replay.test.tspnpm vitest run packages/core/src/hook-sleep-interaction.test.tspnpm --filter @workflow/core typecheckpnpm --filter @workflow/world-local typecheckpnpm --filter @workflow/world-postgres typecheckpnpm --filter @workflow/world-vercel typecheckpnpm changeset status --since=mainpnpm biome check --files-ignore-unknown=true .changeset/bright-waits-refresh.md packages/core/src/runtime.ts packages/core/src/runtime/wait-completion-replay.test.ts packages/world/src/events.ts packages/world-local/src/storage/events-storage.ts packages/world-postgres/src/storage.ts packages/world-vercel/src/events.tsNote: local commands run under Node v25.2.1, which emits the repo's unsupported-engine warning. Biome exits successfully but reports existing warnings in the large runtime/storage files.