Uh oh!
There was an error while loading. Please reload this page.
[core] Never ack orchestrator message before step dispatch sends complete (stable) - #2346
[core] Never ack orchestrator message before step dispatch sends complete (stable)#2346pranaygp wants to merge 2 commits into
Conversation
…lete handleSuspension created each step_created event and enqueued its step-dispatch queue message inside ops, but registered Promise.all(ops) on a detached waitUntil(...) in addition to awaiting it. The detached copy framed those progress-critical sends as droppable background work and re-threw send failures into a promise nothing consumes (unhandled rejection -> process exit 128), which could crash a deployment and leave the orchestrator message acked with the dispatch never sent, orphaning the step. It now strictly awaits the sends only, so a crash before they complete leaves the message un-acked for VQS to redeliver within the lease, re-creating the idempotent step_created and re-sending the dispatch instead of orphaning the step. Adds runtime.test.ts ack-ordering tests: the dispatch send must complete before ack, a hanging send must not ack, and a failing send must reject the handler (no ack) without rejecting any waitUntil promise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🧪 E2E Test ResultsNo test result files found. ❌ Some E2E test jobs failed:
Check the workflow run for details. |
🦋 Changeset detectedLatest commit: 63f4bc6 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 strengthens crash-safety for orchestrator queue handling in @workflow/core by ensuring the suspension handler does not allow progress-critical step-dispatch queue sends to run as detached background work that could be dropped (or crash the process via unhandled rejections) after the orchestrator message is acknowledged.
Changes:
- Remove the detached
waitUntil(Promise.all(ops))registration inhandleSuspension()and rely solely onawait Promise.all(ops)so step dispatch sends gate handler completion (and thus message ack). - Add a new runtime test suite that asserts ack-ordering and failure/hang behavior for step dispatch during suspension handling.
- Add a changeset for a patch release of
@workflow/coredescribing the ack-ordering invariant and the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/core/src/runtime/suspension-handler.ts | Removes detached waitUntil usage so step-dispatch sends complete before the handler resolves (preserving ack-ordering). |
packages/core/src/runtime.test.ts | Adds “ack ordering” tests that validate dispatch send completion gates handler resolution and failures prevent ack. |
.changeset/ack-after-step-dispatch.md | Patch changeset documenting the crash-safety/ack-ordering fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) { | ||
| const workflowRun = await makeRunningRun(opts.runId); | ||
| const order: string[] = []; | ||
There was a problem hiding this comment.
Clearing waitUntilPromises in driveHandler() is a reasonable defensive hardening, but the assertion isn't actually flaky as written. The only test that calls anyWaitUntilPromiseRejected() is the last test in this suite ("rejects the handler ... when the step-dispatch send fails"), and it's preceded by the two other suite tests, each followed by the afterEach that runs waitUntilPromises.length = 0. So the array is already empty when the rejection check runs — leftover entries from earlier describe blocks have been cleared.
The first test in the suite can see leftover entries, but it never inspects waitUntilPromises, so it isn't affected. Moving the reset into driveHandler() (or adding a beforeEach) would make it robust against future reordering/.only usage, but it's not a current correctness issue.
There was a problem hiding this comment.
Done — waitUntilPromises.length = 0 now runs at the start of driveHandler(). Agreed with @vercel-bot that this isn't a current correctness issue (the only test that inspects the array is the last in the suite, after two afterEach resets), but it makes the assertion robust against future reordering/.only. I also applied the no-op .catch() mock hardening here that was flagged on the main PR (#2345), so the two test files stay in sync. Fixed in 63f4bc6.
| "@workflow/core": patch | ||
| --- | ||
| Never ack the orchestrator's queue message before the step-dispatch sends that make the run progress are durably enqueued. The suspension handler created each `step_created` event and enqueued its step-dispatch queue message inside `ops`, but registered `Promise.all(ops)` on a detached `waitUntil(...)` in addition to awaiting it. The detached copy framed those progress-critical sends as droppable background work and re-threw send failures into a promise nothing consumes (unhandled rejection → process exit 128), which could crash a deployment and leave the orchestrator message acked with the dispatch never sent — orphaning the step (its `step_created` is persisted but no queue message exists anywhere to drive the run). It now strictly awaits the sends only, so a crash before they complete leaves the message un-acked and VQS redelivers within the lease, re-creating the (idempotent) `step_created` and re-sending the dispatch instead of orphaning the step. Complements the crash fix in #2336. |
There was a problem hiding this comment.
too verbose. match the format/terseness of other changesets
There was a problem hiding this comment.
Trimmed to match the sibling changesets (now ~4 lines, in line with abort-e2e-flakes.md etc). Fixed in 63f4bc6.
- Trim the changeset to match the terseness of sibling changesets. - Attach a no-op .catch() to each promise the waitUntil mock records so a rejecting promise can never surface as a real unhandled rejection in the test process; keep the original promise for allSettled inspection. - Reset waitUntilPromises at the start of driveHandler() so the rejection check only observes this invocation's promises (robust to reordering/.only). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pranaygp
commented
Jun 11, 2026
pranaygp
commented
Jun 11, 2026
Dependency update: #2347/#2349 are closed as duplicates of #2344 (the faithful-backport fix, which also repairs the stable lockfile and the resume-hook import). This PR should land after #2344. One heads-up for the rebase: #2344 converts suspension-handler.ts's |
| "@workflow/core": patch | ||
| --- | ||
| Strictly await the step-dispatch sends in the suspension handler instead of also registering them on a detached `waitUntil`, so the orchestrator's queue message is never acked before the dispatch that makes the run progress is durably enqueued. A crash before then leaves the message un-acked for VQS to redeliver and replay, rather than orphaning the step. Also removes the re-throw that turned send failures into an unhandled rejection (process exit 128). Complements #2336. |
There was a problem hiding this comment.
| Strictly await the step-dispatch sends in the suspension handler instead of also registering them on a detached `waitUntil`, so the orchestrator's queue message is never acked before the dispatch that makes the run progress is durably enqueued. A crash before then leaves the message un-acked for VQS to redeliver and replay, rather than orphaning the step. Also removes the re-throw that turned send failures into an unhandled rejection (process exit 128). Complements #2336. | |
| Fix unhandled rejection when `step_created`/`wait_created` calls fail in `waitUntil` |
VaguelySerious
commented
Jun 11, 2026
Closing in favor of #2353 |
Stable (4.x) backport of #2345.
What
In
suspension-handler.ts, the progress-criticalops— each step'sstep_createdevent write and its__wkf_step_${stepName}queue-dispatch send, pluswait_createdwrites — are now strictly awaited before the handler returns, instead of also being registered on a detachedwaitUntil(Promise.all(ops).catch(...)).This guarantees the invariant: the orchestrator's queue message is never acked until all work that makes the run progress is durably enqueued. A crash before the await leaves the orchestrator message un-acked, so VQS redelivers and replay re-drives the run — rather than acking and orphaning a step whose dispatch send was lost.
It also removes the
.catch(err => { ...throw err })re-throw that fed a detached, unconsumed promise: on a late send failure that produced anunhandledRejection, which Node turns intoprocess.exit(128)— the production crash signature behind the ~305s VQS-lease-recovery latency cluster (and the permanent orphans when the orchestrator chain had already concluded).Scope
Surgical and orthogonal to #2336 (the
safeWaitUntilcrash hardening). The genuinely-backgroundwaitUntilsites (stream flushes instart.ts/step-executor.ts/step-handler.ts, and the fire-and-forget inresume-hook.ts) are untouched — they're not progress-critical and shouldn't block the ack.Related
waitUntilfor suspension handler async operations #2345 — same fix onmain(5.x / V2). On main the step dispatch is hoisted toruntime.ts'sworkflowEntrypointand already awaited, so the main-sideopscontain only thestep_created/wait_createdcreates; this stable PR additionally keeps the per-step queue send in the awaited set.tscgreen on stable).pnpm install --frozen-lockfilegreen on stable).🤖 Generated with Claude Code