Repository navigation
Backport #4828: [world-vercel] Fix HTTP stream append retries after 429 - #4840
Merged
Merged
Conversation
Adapted for stable, which lacks main's shared throttle waiter: the append retry loop carries its own bounded Retry-After wait (30s cumulative, never past the request deadline). Co-authored-by: Muhammadjon Marufov <marufov254@gmail.com> Signed-off-by: Alex Langenfeld <alex.langenfeld@vercel.com>
🦋 Changeset detectedLatest commit: 493716e 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
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (190 failed)mongodb (85 failed):
redis (20 failed):
turso (85 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
Contributor
VaguelySerious
approved these changes
Oct 13, 2026
VaguelySerious
left a comment
VaguelySerious
left a comment
Member
There was a problem hiding this comment.
(AI) Preliminary review: AI review: no blocking issues
| ) { | ||
| throw error; | ||
| } | ||
| await sleep(500 * 2 ** retry, signal); |
Member
There was a problem hiding this comment.
AI Review: Note
This backoff is the one stable-specific interaction that differs from main: on main, appends run with timeoutMs: null, but on stable fetchStreamMutation passes the 30s getStreamMutationTimeoutMs() deadline. The 429 path checks that deadline before sleeping, but this sleep does not, so when the backend is unreachable the request-level AbortSignal.timeout fires during the backoff and the real cause is lost.
I checked against a closed loopback port:
WORKFLOW_VERCEL_STREAM_TIMEOUT_MS=30000: 6 attempts, ~15.5s,TRANSPORT ... (ECONNREFUSED). Correct.WORKFLOW_VERCEL_STREAM_TIMEOUT_MS=3000:TIMEOUT ... timed out after 3001ms, and the cause is onlyTimeoutError: The operation was aborted due to timeout. No ECONNREFUSED anywhere in the chain.
With the default 30s, a blackholed host (undici's default 10s connect timeout) gets the same result: roughly 10s + 0.5s + 10s + 1s, then the third attempt is aborted, and the user sees "timed out after 30000ms" when no connection was ever made.
Suggestion: apply the same deadline guard here and rethrow the last connect error when Date.now() + backoff >= deadlineMs. That keeps the ECONNREFUSED/UND_ERR_CONNECT_TIMEOUT diagnosis in the surfaced error.
| ); | ||
| if ( | ||
| throttledMs + waitMs > STREAM_APPEND_THROTTLE_BUDGET_MS || | ||
| (deadlineMs !== undefined && Date.now() + waitMs >= deadlineMs) |
Member
There was a problem hiding this comment.
AI Review: Note
This guard stops the wait from crossing the deadline, but it puts no floor under the time left for the retried request. With the default 30s stream timeout and Retry-After: 29, the loop waits ~29s and the fresh append gets less than 1s, which is not enough for a multi-chunk page of several MB. That attempt is then aborted mid-upload, so it surfaces as TIMEOUT with an uncertain outcome (the server may have persisted it), instead of the clean, known-not-persisted 429 we would have surfaced by not waiting.
I confirmed the boundary empirically. With a 900ms deadline and Retry-After: 1, the 429 surfaces right away (1 request). With a 1200ms deadline, the loop waits 1s and retries with ~190ms left. Main's throttle waiter reserves headroom before the deadline (THROTTLE_DEADLINE_RESERVE_MS). A similar reserve here, such as only waiting when deadlineMs - (Date.now() + waitMs) still leaves a few seconds, would keep this "never makes things worse than surfacing the 429".
| * both the cumulative budget and the request deadline; otherwise the 429 | ||
| * response is returned so the caller surfaces it. | ||
| */ | ||
| async function fetchStreamAppend( |
Member
There was a problem hiding this comment.
AI Review: Note
The throttle half of this loop (cumulative 30s budget, 1s floor/default when Retry-After is absent, the request-deadline guard, the 5-retry cap shared between 429s and connect failures, abort during sleep) is new code that exists only on stable, because main uses its shared throttle waiter. It has the least coverage, though. stream-append-retry.test.ts only exercises Retry-After: 60 > budget, and main's policy tests in http-core.test.ts weren't ported (they target the other waiter).
I checked these by hand and they behave as intended:
- 429 with no
Retry-After: 6 requests over ~5s, then HTTP 429 surfaces. - Short deadline: the 429 surfaces without waiting.
- H2 over TLS (the real
allowH2stream agent, which the PR's H1-only tests don't cover): the full body is resent for a small payload, for 4 MiB, and when the server answers 429 before reading the body.
Please add a few unit tests around fetchStreamAppend (exported for testing, or driven through instrumentedFetch with fake timers) so these limits are pinned on the 4.x line. The H2/TLS case is worth adding too, since that is the production transport.
| return response; | ||
| } | ||
| throttledMs += waitMs; | ||
| console.warn( |
Member
There was a problem hiding this comment.
AI Review: Nit
Observability. Intermediate attempts are invisible apart from this warn. Pre-connect retries log nothing, httpLog/the span only describe the final response, and the x-vercel-id/x-vercel-mitigated diagnostics of the throttled attempts are dropped when the body is cancelled. Consider including formatVercelDiagnostics(response.headers) in this warning, logging the pre-connect retries (code and backoff), and/or recording an attempt count on the span. That would let a slow append be attributed to throttling rather than latency.
| } | ||
| // The append service rejects 429s before persistence, so a fresh request | ||
| // cannot duplicate the chunk. | ||
| const retryAfter = parseRetryAfter(response.headers.get('Retry-After')); |
Member
There was a problem hiding this comment.
AI Review: Nit
Main's version of this fix also taught parseRetryAfter to accept the HTTP-date form of Retry-After. Here a date value parses to undefined and falls back to the 1s floor, so the loop retries well before the server asked it to, and the extra attempts are likely 429s too. The impact is low because it is bounded by the 5-retry cap, but porting that small parseRetryAfter change would keep the two lines consistent.
| * uncertain network failures can duplicate an already-persisted append. | ||
| * The built-in append dispatcher disables retries; the SDK retries fresh | ||
| * requests above fetch for confirmed 429s and selected connection failures. | ||
| * A custom dispatcher disables that SDK append retry loop. |
Member
There was a problem hiding this comment.
AI Review: Nit
The README's "Custom dispatcher" section (packages/world-vercel/README.md:17) still says requests "default to a shared undici RetryAgent that handles connection pooling and retries". That is no longer true for stream appends after this change. Main updated that sentence in the same fix ("use shared undici dispatchers for connection pooling"), so it is worth porting here too.
- Wait (for a 429 or a pre-connect backoff) only when the resend would still have half the request timeout, at most 10s, before the deadline; otherwise surface the 429 or rethrow the connect error instead of timing out. - Parse HTTP-date Retry-After values, rounded up to whole seconds. - Log pre-connect retries and the throttled attempt's x-vercel diagnostics, and record http.request.resend_count on the client span. - Pin the retry policy with unit tests and cover the TLS HTTP/2 transport. - README: dispatchers no longer retry on the append path. Signed-off-by: Alex Langenfeld <alex.langenfeld@vercel.com>
This branch was previously 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
2 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.
Summary & Motivation
stable has the same bug as #4628: on
@workflow/world-vercel@4.7.7, an append retried after a 429 reuses a fetch body that was already consumed, so it fails withUND_ERR_REQ_CONTENT_LENGTH_MISMATCH. The fix is Muhammadjon Marufov's (#4828).http.request.resend_counton the client span.Retry-Afternow also accepts HTTP dates.Test Plan
Unit tests cover the retry policy, and new integration tests cover HTTP/1.1 and TLS HTTP/2. The new append tests fail on unmodified stable and pass with this change. The #4628 repro, adapted to
writeToStream, fails against published 4.7.7.I also ran it against a closed port:
WORKFLOW_VERCEL_STREAM_TIMEOUT_MS=3000, it surfacesECONNREFUSEDafter about 0.5s.ECONNREFUSEDafter 5 resends (about 15.5s) rather than timing out.The full
@workflow/world-vercelsuite passes locally andtscis clean.