fix(core): don't strand a run when the max-deliveries terminal write is throttled (v4) - #4422
Merged
Merged
Conversation
Contributor
🦋 Changeset detectedLatest commit: a166367 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
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (106 failed)redis (21 failed):
turso (85 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No blocking issues were identified; only a non-blocking documentation nit remains.
Review effort: Lite
Findings: None
What changed in this PR
This PR hardens v4 max-delivery handling so transient terminal-write and re-queue failures do not strand workflow runs.
Changes:
- Retry transient
run_failedandstep_failedfailures. - Retry failed workflow re-queues and handle step conflicts.
- Add regression tests and a core changeset.
| File | Summary | Review note |
|---|---|---|
packages/core/src/runtime/step-handler.ts |
Improves terminal step recovery and re-queue handling. | — |
packages/core/src/runtime/step-handler.test.ts |
Adds recovery scenario coverage. | — |
packages/core/src/runtime.ts |
Retries transient terminal run writes. | Non-blocking nit: update the nearby max-delivery comment. |
packages/core/src/runtime.test.ts |
Tests terminal write behavior. | — |
.changeset/max-deliveries-transient-terminal-write.md |
Documents the patch release. | — |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pranaygp
reviewed
Sep 25, 2026
pranaygp
left a comment
Contributor
There was a problem hiding this comment.
Traced both give-up paths on stable and ran the tests in a stable worktree. step-handler.test.ts and runtime.test.ts give 38/38 passing. With runtime.ts and step-handler.ts reverted to origin/stable, 6 fail (4 step-handler, 2 runtime), which matches the description.
The EntityConflictError fall-through reads correctly to me. The case it rescues is a delivery that wrote step_failed and then failed to publish the wake. The next delivery conflicts and now re-queues instead of acking, and an extra wake costs one replay. RunExpiredError still acks.
Two small notes are inline. For context, on stable the queue retry directive ignores retryAfter (world-vercel/src/queue.ts:152), but past the ceiling the delivery-count backoff is already at the 900 s cap, so a 429's Retry-After is effectively respected anyway. On stable, world-postgres jobs use maxAttempts: 3, so this ceiling is never reached there.
| // Re-queue the workflow to handle the failed step. A failure here | ||
| // throws so the queue redelivers: acking would leave the step failed | ||
| // with no message left to wake the workflow. | ||
| await queueMessage( |
Contributor
There was a problem hiding this comment.
This is the one spot that throws on any error rather than checking isRetryableWorldError. A definitive publish failure would redeliver every ~15 min until the message TTL. It's bounded, and publish failures are almost always transient, so this is probably the right trade: acking here strands the run for certain. I'm pointing it out because it differs from the step_failed branch above.
| // abandon the run: acking here leaves it `running` with no message | ||
| // left to drive it. Throw so the queue redelivers; the redelivery is | ||
| // still past the ceiling, so it only retries this terminal write. | ||
| if (isRetryableWorldError(err)) { |
Contributor
There was a problem hiding this comment.
Same caveat as #4421, for anyone reading this later: this relies on the queue redelivering after delivery 49. VQS (24 h TTL, no delivery cap) and world-local (256) do. On this branch, world-postgres jobs cap at 3 attempts, so they never reach the ceiling.
pranaygp
approved these changes
Sep 25, 2026
pranaygp
left a comment
Contributor
There was a problem hiding this comment.
Approving. The step and workflow give-up paths now redeliver on transient failures instead of acking. The conflict fall-through re-queues correctly, and the new tests fail without the change (6/6 on stable). The inline notes are non-blocking.
This branch had an error being deployed
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
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
This is the v4 (
stable) counterpart of #4421. It targetsstabledirectly because the v1 step route (step-handler.ts) only exists here.mainremoved it in #3061.It came out of a customer report on v4: a
ThrottleErroron a step (POST /.well-known/workflow/v1/stepreturned 500 after a 17s 429) left a run stuck inrunning, and no error reached application code.A thrown
ThrottleErroris normally recovered by queue redelivery. What stranded runs was the give-up path that runs once a message passesMAX_QUEUE_DELIVERIES: it acked the message on any failure of its final writes. When backend pressure pushes a message to the ceiling, those same writes are likely to be throttled too.runtime.ts): when therun_failedwrite fails withisRetryableWorldError(429, 5xx,TRANSPORT/TIMEOUT), the handler now throws instead of acking. The queue redelivers with its backoff. The redelivered message is still past the ceiling, so it only retries this write and never replays the run.step-handler.ts):step_failedwrite now throws.EntityConflictErroronstep_failednow falls through to the re-queue instead of returning. The step may have been failed by this message's own earlier delivery, whose re-queue then failed. The cost is one extra replay.RunExpiredErrorstill acks silently.Definitive rejections (e.g. a 400) keep the existing logged give-up, because retrying them would only hot-loop.
Tests
step-handler.test.ts: covers a transient 429 or 5xx onstep_failed, a failed re-queue, re-queueing after a conflict, and the run-expired and 400 cases.runtime.test.ts: covers a transient 429 or 5xx onrun_failed, a successful write, and a 400.