[world-vercel] Bound HTTP timeouts and retry transport timeouts - #3169
Merged
Conversation
The shared undici Agent set no headersTimeout/bodyTimeout, so undici's 5-minute defaults applied. That is longer than the queue's 300s visibility timeout, so one connection that accepted a request and then went quiet outlived the lease the invocation was holding: the message redelivered, the run replayed, and it surfaced as a ~305s gap between two consecutive workflow events with no error recorded anywhere. Set headersTimeout/bodyTimeout to 30s — under both the queue's 60s visibility-extension interval and makeRequest's 60s deadline — so a stalled socket fails fast as a typed UND_ERR_*_TIMEOUT while callers can still react. Add those two codes to the RetryAgent's errorCodes (undici's defaults omit them, so a stalled socket was never retried) and cap maxRetries at 2 so the worst case stays inside the visibility window even when a caller wraps the call in its own retries. POST is never retried by the RetryAgent, so event writes get the type-aware in-process retry from #2675, backported here (minus attr_set, which does not exist on this line). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 187ab97 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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
🧪 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
|
karthikscale3
left a comment
karthikscale3
left a comment
Contributor
There was a problem hiding this comment.
One documentation nit; I did not find a blocking correctness issue or regression in the implementation.
| * `UND_ERR_HEADERS_TIMEOUT` while callers still have time to react — rather than | ||
| * as an opaque outer abort, or not at all. | ||
| * | ||
| * Override with `WORKFLOW_VERCEL_HEADERS_TIMEOUT_MS`; `0` disables the timeout. |
Contributor
There was a problem hiding this comment.
nit: These are new user-configurable environment variables, but the repo guidelines require every such variable to be documented. Could we add both WORKFLOW_VERCEL_HEADERS_TIMEOUT_MS and WORKFLOW_VERCEL_BODY_TIMEOUT_MS to docs/content/docs/deploying/world/vercel-world.mdx (and ideally the package README), including the 30s defaults and that 0 disables each timeout?
karthikscale3
approved these changes
Jul 28, 2026
karthikscale3
left a comment
karthikscale3
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the timeout bounds and event retry behavior. The world-vercel build and all 200 package tests pass locally; no blocking correctness issues found.
pranaygp
reviewed
Jul 28, 2026
pranaygp
left a comment
pranaygp
left a comment
Contributor
There was a problem hiding this comment.
Reviewed with the claims checked against their sources rather than taken on faith: I read every retryable: true handler in workflow-server's event-create path, undici 7.28.0's RetryHandler source, and @vercel/queue 0.3.1, and ran the package suite locally (200 tests pass, typecheck clean).
What verified cleanly:
- Every event-sourced (v3/v4) classification in
EVENT_RETRY_ELIGIBILITYmatches the server: conditional creates → 409 forrun/step/wait/hook_created; terminal-state.where()guards on run/step transitions (InvalidOperationStateErroris HTTP 409);run_startedearly-returns the running run without a duplicate row;hook_disposedis a conditional delete; and the three exclusions are right (startStepPatchdoes.add({attempt: 1}),step_retrying's pending→pending patch passes its non-terminal guard and would append a duplicate row,hook_receivedhas no guard). Entity handlers do run before the event-row insert, as the module doc claims. - undici's
RetryHandlerapplies itsmethodsgate to botherrorCodesandstatusCodesretries, so POSTs are truly never retried by the dispatcher — the in-process and dispatcher budgets don't stack. A POST 5xx surfaces asUND_ERR_REQ_RETRY, whichTRANSIENT_CODEScovers.RETRY_ERROR_CODES= undici defaults + the two timeout codes, exactly as documented. Firewall-challenge 429s land in the classifier as status 429 → not retried in-process, consistent with the dispatcher's 429 policy. @vercel/queue0.3.1'sDIRECTIVE_CALL_ATTEMPTSis indeed 3, so the budget test's* 3factor matches reality.
One real issue (inline on events.ts): the retry wrapper also covers the v1Compat legacy branch, whose endpoints don't have the guards the classification relies on — most notably v1 run_created mints the runId server-side, so a retry whose original landed creates a duplicate run. One-line guard suggested. main has the same shape from #2675, so the guard wants forward-porting too.
Forward-port note: main already has event-retry.ts (#2675) but not the http-client.ts half of this PR — 5.x still runs undici's 5-minute defaults with no timeout codes in errorCodes. This PR should get a companion on main so the actual production fix isn't stable-only.
E2E: Vercel Prod / Local / Postgres / Windows all green; the Community Worlds failures are the known non-blocking redis/turso set that also fails on main.
| // queue delivery. Non-retryable types (step_started, step_retrying, | ||
| // hook_received) run once. See ./event-retry for the validated per-event | ||
| // classification. | ||
| return await withEventPostRetry( |
Contributor
There was a problem hiding this comment.
The retry wrapper also covers createWorkflowRunEventInner's v1Compat branch, but EVENT_RETRY_ELIGIBILITY was validated against the event-sourced handlers only — the legacy endpoints don't share those guards:
run_created+v1Compat(astart()/recreateRunof a specVersion-1 run) goes toPOST /v1/runs/create, which mints the runId server-side (RunsService.create→RunId.make();CreateWorkflowRunRequestcarries no client key). A retry whose original landed creates a second run — there's nothing to 409 on.wait_completed+v1Compat(wakeUpRunon a legacy run) hits the server'shandleLegacyEvent, which is a bareevent.createwith a freshEventId.make()and no duplicate guard — a retry appends a duplicate event row. (Benign for legacy runs in practice, but it breaks the "no duplicate row" invariant the classification promises.)run_cancelled+v1Compatgoes throughcancelWorkflowRunV1, a PUT viamakeRequest— which the dispatcher's RetryAgent already retries, so this stacks up to 3×3 attempts. Idempotent in outcome; just noting the amplification.
Suggest bypassing the in-process retry for the legacy branch:
return params?.v1Compat
? await createWorkflowRunEventInner(id, data, params, config)
: await withEventPostRetry(
() => createWorkflowRunEventInner(id, data, params, config),
data.eventType
);(main's events.ts has the identical shape from #2675, so the guard would want forward-porting as well.)
| * Declared `satisfies Record<WorkflowEventType, …>` so adding or removing a | ||
| * world event type without classifying it is a compile error. | ||
| */ | ||
| export const EVENT_RETRY_ELIGIBILITY = { |
Contributor
There was a problem hiding this comment.
Worth stating in this docstring that the classification is derived from (and only valid for) the event-sourced create path — the v1Compat legacy endpoints (/v1/runs/create, /v1/runs/:id/events) don't share these guards; see the comment on events.ts for the concrete divergence.
For the event-sourced path itself I cross-checked every entry against workflow-server's handlers and they all hold (conditional creates → 409; terminal-state .where() guards with InvalidOperationStateError = 409; run_started early-return; hook_disposed conditional delete; and the three exclusions match startStepPatch's .add({attempt: 1}), step_retrying's pending→pending patch, and the unguarded hook_received).
| // Each attempt can burn a full headers timeout. The queue client wraps its | ||
| // acknowledge call in its own 3 attempts, so the product must stay under | ||
| // the 300s visibility timeout or a hung ack still loses the lease. | ||
| expect(attempts * headersTimeout * 3).toBeLessThan(300_000); |
Contributor
There was a problem hiding this comment.
Nit: this worst-case product excludes both backoff layers — the RetryAgent sleeps 500ms + 1s between its 3 attempts (~1.5s per cycle), and the queue client adds 250ms × attempt between its 3 DIRECTIVE_CALL_ATTEMPTS — so the true worst case is ~275s, not 270s. Still comfortably under 300s today, but the assertion as written would accept WORKFLOW_VERCEL_HEADERS_TIMEOUT_MS-class changes up to 33s (3 × 33 × 3 = 297 < 300) while the real budget crosses the window. Consider adding the backoff terms to the arithmetic, or asserting against a smaller bound to preserve headroom.
| * | ||
| * Exported so a test can assert the timeout codes stay in the list. | ||
| */ | ||
| export const RETRY_ERROR_CODES = [ |
Contributor
There was a problem hiding this comment.
Optional/question: was leaving UND_ERR_CONNECT_TIMEOUT out deliberate? A connect timeout fires before the request is written, so it's the safest retry of the lot — safer than the two timeout codes this PR adds. Today it's only covered by the bespoke single-attempt retryConnectTimeout path in makeRequest (#3013) for flagged GETs; event POSTs get it via TRANSIENT_CODES, but a plain GET/PUT through this dispatcher never retries it. Fine to keep the backport diff minimal — just checking it was considered.
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
3 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.
Problem
The shared undici
Agentinworld-vercelsets noheadersTimeout/bodyTimeout, so undici's 5-minute defaults apply. That is longer than the queue's 300s visibility timeout, so a connection that accepts a request and then goes quiet outlives the lease the invocation is holding: the message redelivers, the run replays, and the whole thing is invisible — nostep_retrying, noerrorCode, just a ~305s gap between two consecutive workflow events.This was found on a production customer on 4.5.0: 56 of 33,822 runs over 4 days (0.17%) each contain exactly one gap of 302.9–308.3s. The tightness of that distribution is what identifies it as a fixed timeout rather than workload variance. Two contributing signatures showed up in the outgoing-fetch logs for the invocations that went silent:
UND_ERR_HEADERS_TIMEOUTat ~300,300ms against the queue host — undici's defaultheadersTimeout, on a path that has no other deadline (the queue client gets this dispatcher but does not go throughmakeRequest).POST /api/v3/runs/<wrun>/eventsaborted at 60,003ms —REQUEST_TIMEOUT_MSinutils.ts— with zero retries, becauseRetryAgentnever retries a POST.In both cases the invocation returned HTTP 200 having renewed its queue lease but never acked it.
Change
Timeouts (
http-client.ts):headersTimeoutandbodyTimeoutare set to 30s. That is under both the queue's 60s visibility-extension interval andmakeRequest's 60s deadline, so a stalled socket surfaces as a typed, retryableUND_ERR_*_TIMEOUTwhile callers can still react — instead of as an opaque outer abort, or (off themakeRequestpath) not until the lease is already gone. Overridable viaWORKFLOW_VERCEL_HEADERS_TIMEOUT_MS/WORKFLOW_VERCEL_BODY_TIMEOUT_MS;0disables.Retries for timeouts:
UND_ERR_HEADERS_TIMEOUT/UND_ERR_BODY_TIMEOUTare added to theRetryAgent'serrorCodes(undici's defaults omit them, so a stalled socket was never retried at all), andmaxRetriesdrops from undici's 5 to 2. These codes are ambiguous — the request may have been processed — so they stay scoped toRetryAgent's defaultmethods, which never includes POST. The PUTs that go through this dispatcher are the legacy v1/v2 entity updates (cancel run, update step), idempotent in outcome; stream appends bypass the dispatcher entirely.The retry budget is bounded deliberately: 3 attempts × 30s ≈ 92s with backoff, and the queue client wraps its acknowledge call in its own 3 attempts, so the product stays under the 300s visibility window. A test asserts that arithmetic so raising either constant fails.
POST event writes: since
RetryAgentcan't cover them, this backports the type-aware in-process retry from #2675 (event-retry.ts), whose transient set already includes the timeout codes.attr_setis dropped — it doesn't exist on this line.step_started,step_retrying, andhook_receivedstay at one attempt.Validation
packages/world-vercel: 200 unit tests pass; workspacetypecheckclean. New tests cover the env parsing (including the0and unparseable cases), the presence of the timeout codes, the retry-budget arithmetic, and — against a real socket that accepts and never responds — that the request fails withUND_ERR_HEADERS_TIMEOUTrather than hanging.Two out-of-band probes against a hung local server, using the built dist and the real
createWorkflowRunEvent(not committed):step_completedUND_ERR_HEADERS_TIMEOUTat 3× the configured timeoutstep_startedUND_ERR_HEADERS_TIMEOUT, no attempt-counter double-incrementA raw POST through the dispatcher fails at the configured timeout with exactly one request reaching the server, confirming the dispatcher itself does not re-issue POSTs.
🤖 Generated with Claude Code