fix(relay): set Request.GetBody so the HTTP/2 transport can transparently retry after an upstream stream reset#6249
Open
BlockchainHe wants to merge 2 commits into
Conversation
…ntly retry after an upstream stream reset
The outbound request body is a type-erased io.Reader over BodyStorage, so
net/http cannot derive Request.GetBody (it only does so for *bytes.Reader,
*bytes.Buffer and *strings.Reader). With GetBody nil, the HTTP/2 transport
cannot transparently retry a request once the body has been written and the
upstream resets the stream with a retryable error (REFUSED_STREAM, or a
connection-level GOAWAY); the relay request then fails with:
http2: Transport: cannot retry err [...] after Request.Body was written;
define Request.GetBody to avoid this error
This affects every relay path that goes through DoApiRequest (chat, claude,
gemini, responses, embedding, image, rerank).
BodyStorage (memory and disk) already implements io.Seeker, so replay support
only needed wiring:
- NewOutboundJSONBody additionally returns a getBody that rewinds the storage
and hands out a fresh non-closing reader. The transport only calls GetBody
after the previous attempt's body has been abandoned, so the rewind cannot
race an in-flight read.
- RelayInfo carries it in the new UpstreamRequestGetBody field, set alongside
UpstreamRequestBodySize by the handlers that build storage-backed bodies.
- applyUpstreamGetBody (symmetric with applyUpstreamContentLength) wires it
into DoApiRequest/DoFormRequest/DoTaskApiRequest, only when req.GetBody is
still nil.
Also remove the hand-rolled GetBody override in DoTaskApiRequest: it returned
the same already-consumed reader, so any transport-level replay would have
silently sent an empty body, and it clobbered the correct snapshot-based
GetBody that net/http derives from the *bytes.Reader bodies the task adaptors
pass in. For non-replayable bodies GetBody now stays nil, so a retry fails
loudly instead of corrupting the request.
Covered by unit tests plus an end-to-end raw-frame HTTP/2 test that resets
the first stream with REFUSED_STREAM after the body is written and asserts
the transport transparently retries with the complete body.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
WalkthroughChangesUpstream body replay
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@relay/common/outbound_body.go`:
- Around line 26-43: Update NewOutboundJSONBody’s getBody closure to create and
return a fresh reader backed by an independent copy of the original body data,
rather than rewinding and reusing storage. Preserve the existing error
propagation and io.ReadCloser return contract so retries and redirects each
receive an independently positioned reader.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6be0eeda-9a45-45a6-8380-8b75e651d239
📒 Files selected for processing (13)
relay/channel/api_request.gorelay/channel/api_request_getbody_test.gorelay/chat_completions_via_responses.gorelay/claude_handler.gorelay/common/outbound_body.gorelay/common/outbound_body_test.gorelay/common/relay_info.gorelay/compatible_handler.gorelay/embedding_handler.gorelay/gemini_handler.gorelay/image_handler.gorelay/rerank_handler.gorelay/responses_handler.go
Per the http.Request.GetBody contract ("returns a new copy of Body"),
each call must yield a reader with its own cursor. The previous
implementation rewound and reused the shared BodyStorage, so two
consecutive GetBody readers would interfere with each other, and a
replay could disturb the primary body's offset under extreme transport
timing (e.g. attempt N's body write not yet fully abandoned when the
transport builds attempt N+1).
Instead of snapshotting the payload (an extra copy), add
BodyStorage.NewReader, which returns an independent zero-copy reader:
- memory mode: a fresh bytes.Reader over the same immutable backing
array;
- disk mode: a separate file descriptor over the cache file, so the
transport closing a replayed body only closes that descriptor.
NewOutboundJSONBody's getBody now simply hands out storage.NewReader,
and once the handler releases the storage, GetBody fails with
ErrStorageClosed instead of replaying stale data.
Tests: interleaved reads across two replay readers and the primary
body each observe exactly their own byte stream, for both the memory
and the disk-backed storage; the existing GetBody and HTTP/2 retry
suites still pass (h2 e2e tests flake-free with -count=20).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
relay/common/outbound_body_test.go (1)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
testify/assertfor non-fatal value checks.As per coding guidelines, Go backend tests must use
testify/assertfor non-fatal value checks, reservingtestify/requirestrictly for setup and fatal assertions. Therequire.Equalandrequire.EqualValuescalls throughout this test file act as non-fatal checks for the replay payloads and should be updated. Ensure"github.com/stretchr/testify/assert"is imported if not already present.
relay/common/outbound_body_test.go#L20-L26: Changerequire.EqualValues(t, len(payload), size)andrequire.Equal(t, payload, first)to useassert.relay/common/outbound_body_test.go#L36-L36: Changerequire.Equaltoassert.Equal.relay/common/outbound_body_test.go#L58-L68: Changerequire.Equaltoassert.Equalon lines 58 and 67.relay/common/outbound_body_test.go#L84-L84: Changerequire.Equaltoassert.Equal.relay/common/outbound_body_test.go#L96-L111: Changerequire.Equaltoassert.Equalon lines 96, 101, 106, and 111.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/common/outbound_body_test.go` around lines 20 - 26, In relay/common/outbound_body_test.go, replace the non-fatal require.EqualValues and require.Equal checks throughout the outbound body tests with assert.EqualValues and assert.Equal, including the checks in lines 20-26, 36, 58, 67, 84, 96, 101, 106, and 111; add the testify/assert import if needed, while retaining require for setup and fatal assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@relay/common/outbound_body_test.go`:
- Around line 20-26: In relay/common/outbound_body_test.go, replace the
non-fatal require.EqualValues and require.Equal checks throughout the outbound
body tests with assert.EqualValues and assert.Equal, including the checks in
lines 20-26, 36, 58, 67, 84, 96, 101, 106, and 111; add the testify/assert
import if needed, while retaining require for setup and fatal assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e0a4352-193b-41c0-b2c7-e1b8eacf3796
📒 Files selected for processing (3)
common/body_storage.gorelay/common/outbound_body.gorelay/common/outbound_body_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- relay/common/outbound_body.go
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
1 participant
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.
📝 Description
While relaying to an HTTP/2 upstream, requests started failing with:
Go's transport can transparently retry these resets (
REFUSED_STREAM/ gracefulGOAWAY), but only whenRequest.GetBodyis set. The relay never sets it — the outbound body is a type-erased reader overBodyStorage— so every such reset surfaces to the caller even though a retry would have succeeded. While fixing this I also noticedDoTaskApiRequestinstalls aGetBodythat re-reads an already-consumed reader: a retry there would silently send an empty body, which is worse than failing.The fix hands out independent replay readers from
BodyStorage(a newNewReader()on both the memory and disk implementations — zero-copy in both modes), carries the replay hook onRelayInfonext toUpstreamRequestBodySize, and injects it only when net/http didn't already derive a correctGetBody. The broken override is removed. Unit tests plus a raw-frame HTTP/2 end-to-end test cover both the transparent-retry path and the empty-body regression.Full analysis: problem, fix design, and test coverage
Problem
1. Requests on the main relay paths can never be transparently retried by Go's HTTP/2 transport
Per net/http semantics, the HTTP/2 transport can transparently retry a request that fails with a retry-safe error — a stream-level
RST_STREAM(REFUSED_STREAM)(RFC 9113 §8.7: the upstream guarantees the stream was not processed; common with proxy/CDN-fronted upstreams under load or during graceful restarts) or a graceful connection-levelGOAWAY. However, once the request body has been written, retrying requiresRequest.GetBodyto be non-nil; otherwise the request fails outright:The outbound body built by
DoApiRequestis a type-erasedio.Reader(common.ReaderOnly(BodyStorage)), andnet/httponly auto-derivesGetBodyfor*bytes.Reader/*bytes.Buffer/*strings.Reader. SoGetBodystaysnilon every path going throughDoApiRequest— chat / claude / gemini / responses / embedding / image / rerank. This is the other half of the same type-erasure issue thatapplyUpstreamContentLengthalready handles forContentLength.2. The hand-rolled
GetBodyinDoTaskApiRequestsilently replays an empty body (worse than failing)This returns the same, already-consumed reader: if the transport ever retries (or a redirect needs to replay the body), the second attempt sends an empty body. Moreover, most task adaptors pass
*bytes.Readerbodies, for whichnet/httphad already derived a correct snapshot-basedGetBody— this code overwrote a correct implementation with a broken one.Fix
BodyStorageretains the full payload for the lifetime of the request (in memory, or in a disk-backed temp file above the configured threshold), so replay support only needed wiring:BodyStoragegains aNewReader() (io.ReadCloser, error)method returning an independent reader per call, as required by theRequest.GetBodycontract ("a new copy of Body"): the memory implementation returns a freshbytes.Readerover the same immutable backing array (zero-copy), and the disk implementation opens a fresh file descriptor (zero-copy, kernel page-cache backed). Independent cursors mean replays can never race an in-flight read.NewOutboundJSONBodynow also returnsgetBody func() (io.ReadCloser, error)backed byNewReader(); closing a replayed body never affects the underlying storage (its lifecycle stays owned by the handler'sdefer closer.Close()), and once the storage is released,GetBodyfails withErrStorageClosedinstead of replaying stale data.RelayInfogains anUpstreamRequestGetBodyfield, set at the same call sites (8 handlers, 9 call sites) and with the same lifecycle as the existingUpstreamRequestBodySize.applyUpstreamGetBodyhelper (symmetric withapplyUpstreamContentLength) wires it intoDoApiRequest/DoFormRequest/DoTaskApiRequest, and only injects whenreq.GetBody == nil— it never overrides a correct auto-derived implementation.DoTaskApiRequestis removed:*bytes.Reader/*bytes.Buffer-style bodies fall back tonet/http's snapshot-basedGetBody, and non-replayable type-erased bodies keepGetBody == nil, so a retry fails loudly instead of silently sending a corrupted (empty) request.Tests
relay/channel/api_request_getbody_test.go), against a raw-frame HTTP/2 test server that reads the full request body, answers the first attempt withRST_STREAM(REFUSED_STREAM), and serves the retried stream normally with 200:TestUpstreamGetBody_HTTP2RetryAfterUpstreamStreamReset: builds the request the same wayDoApiRequestdoes (type-erased body + bothapplyUpstream*helpers) and asserts the transport retries transparently and the server receives a byte-identical body on both attempts;TestUpstreamGetBody_HTTP2CannotRetryWithoutGetBody: negative case reproducing the pre-fixcannot retry err ... after Request.Body was writtenfailure.TestNewOutboundJSONBody_GetBodyReadersAreIndependent(+_DiskStorage): two readers obtained fromGetBodyare read interleaved and each still yields the complete, correct payload — independent cursors verified for both the memory- and disk-backed storage modes.TestApplyUpstreamGetBody_*: injectedGetBodyis non-nil and two consecutive calls both return the full body; an existingGetBodyis never overridden; without a replay source it staysnil.TestNewOutboundJSONBody_*(relay/common/outbound_body_test.go): after the main body is consumed (or partially read),getBodyreplays the full body repeatedly; closing a replayed body does not affect the storage lifecycle.TestDoTaskApiRequest_KeepsReplayableGetBody: regression test for problem 2 — after the request completes,req.GetBodystill returns the full body repeatedly (it returned an empty body before the fix).go build ./...passes;go vetreports no new findings on the touched packages (identical to the main baseline);go test ./relay/... ./common/...all pass; the two HTTP/2 end-to-end tests run flake-free with-count=20.🚀 Type of change
🔗 Related Issue
✅ 提交前检查项 / Checklist
Bug fix, I have filed or linked a corresponding issue, and I am not classifying design trade-offs, expectation mismatches, or misunderstandings as bugs.📸 Proof of Work
Genuine pre-fix reproduction, captured by running the same e2e scenario without the
applyUpstreamGetBodywiring:Summary by CodeRabbit
Bug Fixes
Tests