Go: replay *WithBody request bodies across retries - #481
Conversation
|
Series — retry-contract follow-up (branch fresh off
#481 and #483 both touch |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug in the Go generated client’s retry loop: idempotent *WithBody(..., body io.Reader) operations could retry with an empty or truncated body because each attempt rebuilt the request from a single-use reader. The change snapshots the finalized first-attempt body (post request editors) and replays it across retries with net/http-consistent GetBody semantics, preserving ContentLength and bounding buffering.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Add request-body replay support to
doWithRetry(preferGetBody; otherwise buffer up to 1 MiB; over-cap demotes to single attempt). - Regenerate the Go client so the fix is reflected in
go/pkg/generated/client.gen.go. - Add Go tests (outside
pkg/generated) that pin retry body replay behavior and edge cases.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go/templates/client.tmpl | Implements body snapshot/replay logic in doWithRetry and the captureReplayBody helper. |
| go/pkg/generated/client.gen.go | Regenerated output reflecting the retry body replay changes. |
| go/pkg/basecamp/generated_withbody_replay_test.go | Adds contract tests to ensure retries resend correct bodies across retry scenarios. |
Comments suppressed due to low confidence (2)
go/pkg/basecamp/generated_withbody_replay_test.go:135
- On the simulated mid-stream network error path, the request body isn't closed before returning the error. Closing it makes the fake doer match net/http Transport semantics and avoids leaking the partially-consumed body between attempts.
// Read only a prefix, then fail like a mid-stream network error.
_, _ = io.ReadFull(req.Body, make([]byte, 5))
return nil, io.ErrUnexpectedEOF
go/pkg/basecamp/generated_withbody_replay_test.go:138
- The success path reads req.Body but doesn't close it. Closing it makes the fake transport behavior consistent with net/http and prevents resource leaks in the test helper.
attempt2Body, _ = io.ReadAll(req.Body)
return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader("")), Header: make(http.Header)}, nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b70b85ba8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
5b70b85 to
bbea162
Compare
|
Addressed the bot review:
9 red proofs + 2 controls green; drift, vet, golangci-lint clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbea1629f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bbea162 to
9892b58
Compare
|
Addressed the fresh re-review + the outstanding items:
Honest test taxonomy (13 total): 9 fail against the shipped |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9892b58aa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
go/templates/client.tmpl:347
- Capture the builder-provided GetBody/ContentLength on attempt 1 (before applyEditors) so we can later restore ContentLength when GetBody remains authoritative and wasn’t replaced by editors.
req, err := buildRequest()
if err != nil {
return nil, err
}
req = req.WithContext(ctx)
go/templates/client.tmpl:370
- When GetBody is authoritative and unchanged by editors, restore ContentLength to the builder value. Otherwise an editor can accidentally set ContentLength for a req.Body replacement that will be ignored (because GetBody still points at the original), causing a ContentLength/body mismatch.
bodyReplay = replay
replayContentLength = cl
if !retriable {
go/templates/client.tmpl:315
- doWithRetry normalizes attempt 1’s body to req.GetBody() bytes when GetBody is present, but it keeps whatever ContentLength editors set on the request. If an editor updates ContentLength for a req.Body replacement while forgetting to update GetBody, the SDK will ignore the editor body but retain the editor ContentLength, which can produce an invalid request (ContentLength mismatch) even on attempt 1.
Capture the pre-editor GetBody/ContentLength from the builder so ContentLength can be restored when GetBody wasn’t replaced by editors (i.e., we’re still using the builder’s GetBody snapshot).
This issue also appears in the following locations of the same file:
- line 343
- line 368
var bodyReplay func() (io.ReadCloser, error)
var replayContentLength int64
retryEligible := isIdempotent && maxAttempts > 1
9892b58 to
11401e5
Compare
|
Addressed the fresh re-review (goroutine leak) and the doc items:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11401e54c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
go/templates/client.tmpl:299
- The comment inside
if !retriablesays “large unbuffered body”, but this branch also runs when retries are simply ineligible (e.g., maxAttempts==1). Clarifying the comment helps keep the behavior explanation accurate.
if !retriable {
// The body can't be safely replayed (large unbuffered body):
// send it once and do not retry.
maxAttempts = 1
}
go/templates/client.tmpl:262
- On retries where the captured replay body is empty (initial request body was nil/http.NoBody), installReplay sets req.Body=nil. buildRequest via http.NewRequest uses http.NoBody, so this changes what request editors see on retries and can cause nil dereferences (e.g., editors that read req.Body). Prefer restoring http.NoBody to preserve net/http’s canonical non-nil empty body across attempts.
This issue also appears on line 295 of the same file.
} else {
req.Body = nil
req.GetBody = nil
}
go/pkg/basecamp/generated_withbody_replay_test.go:350
- This test comment references maxReplayBodyBytes, but there is no such constant in the Go code (only this comment mentions it). Updating the wording avoids implying a buffering threshold that doesn’t exist in the replay implementation.
// large (> maxReplayBodyBytes) valid body must stay fully replayable — the
// GetBody path never buffers, so the op still retries instead of being demoted
// to a single attempt.
func TestWithBodyReplay_EditorSetsBodyAndGetBodyNoDemotion(t *testing.T) {
payload := bytes.Repeat([]byte("z"), (1<<20)+512) // > maxReplayBodyBytes
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
go/templates/client.tmpl:267
- In installReplay, when there is no captured body to replay (bodyReplay == nil), the code sets req.Body = nil. For client requests, net/http typically uses http.NoBody to represent an empty body; leaving Body nil can break request editors or transports that assume req.Body is non-nil (and is inconsistent with the net/http contract this change set is aiming to follow). Set req.Body to http.NoBody instead of nil for the no-body replay case.
} else {
req.Body = nil
req.GetBody = nil
}
a721f23 to
a161fe3
Compare
Review carefully before merging. Consider a major version bump. |
|
Fixed the naturally-idempotent PUT/DELETE regression (was failing Go Tests + Conformance). Root cause: the new no-buffer policy keys retry off Fix:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a161fe36a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a161fe3 to
05637a0
Compare
|
Addressed both remaining items:
Taxonomy now 15 raw- |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05637a095f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
05637a0 to
fba7edc
Compare
|
Addressed the replay-handle leak:
Taxonomy now 17 raw- |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fba7edc5ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The generated client's doWithRetry rebuilds each attempt's request via
buildRequest(), which recreates it from the caller's single-use io.Reader.
The raw *WithBody(..., body io.Reader) methods therefore shipped an empty
(or, after a partial-read network failure, mid-stream) body on every retry
attempt — the 42 idempotent raw *WithBody ops (Update*/Reposition*/Set*/
Replace*/Toggle*/MarkAsRead) reaching BC3 over PUT/DELETE all retry, so a
transient 429/503/network failure could resend a truncated body.
doWithRetry now snapshots the first finalized request's body once (after
buildRequest + applyEditors) and replays THAT body on every later attempt,
under a documented, net/http-consistent contract:
- Retry-ineligible (single attempt) -> stream the finalized body untouched.
- GetBody present (retry-eligible) -> GetBody is AUTHORITATIVE. This attempt
is normalized to send GetBody()'s bytes too, so attempt 1 and every retry
ship byte-identical content even if an editor mutated req.Body out of sync
with GetBody (including an in-place strings.Reader.Reset). A large body with
a valid GetBody stays fully replayable.
- No GetBody -> an unrewindable, single-use stream: NOT buffered or retried.
Buffering would risk unbounded memory, and a blocking reader cannot be
interrupted (no general way to abort an io.Reader), so it is sent once.
Because that policy keys retry off GetBody, the SDK's own serialized bodies
must be snapshotable. marshalBody (and the multipart account-logo path) now
return *bytes.Reader instead of the old custom rewindableReader (which had no
GetBody), so the eight hand-written idempotent update methods
(Todos/Cards/CardSteps/Checkins/Projects/People/Schedules.Update, etc.) — and
UpdateAccountLogo — replay their bodies and keep retrying on transient
failures. rewindableReader is removed. Only an external caller passing a
genuinely streaming, non-snapshotable reader to a raw *WithBody method forgoes
retries, which is the safe behavior for an unrewindable stream.
On a retry the captured body is installed BEFORE the editors run (and
re-installed after), so a body-aware editor (Digest/HMAC) signs the bytes that
are actually sent, and an editor-installed retry body is discarded and closed.
A GetBody snapshot failure disables retries (single attempt with the valid
body) rather than failing an otherwise-sendable request; an installed replay
body is closed if a retry editor errors; and an explicitly empty body
(http.NoBody) is replayed as a non-nil empty body, not collapsed to nil. ContentLength is restored per
replay path.
Fix lives in go/templates/client.tmpl (regenerated) plus helpers.go's
marshalBody. Tests: 18 raw-*WithBody replay tests outside pkg/generated (11 fail against
the shipped d560496 client, 4 regression proofs, 3 controls, each red-proofed),
including an empty-body framing proof through a real net/http Transport; a PUBLIC-service retry proof (ProjectsService.Update retries a
503 and replays the full body — red-proofed against a non-snapshotable body);
and a marshalBody snapshotability test. Go conformance's naturally-idempotent
PUT/DELETE cases pass.
No API signature changes. A request editor changes a retrying request's body
only via req.GetBody.
Follows the retry-contract program (#456/#460/#461/#476).
fba7edc to
248535a
Compare
|
Addressed the framing item, and rebased onto the merged #482/#483:
Taxonomy now 18 raw- |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 248535a422
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if req.Body != nil { | ||
| _ = req.Body.Close() | ||
| } | ||
| req.Body = body |
There was a problem hiding this comment.
For an idempotent request using the default retry configuration, a request editor that replaces req.Body without also replacing the builder-created req.GetBody now has its body closed and silently overwritten before even the first attempt. This breaks the documented ability of WithRequestEditorFn to mutate a request: editors that compress, encrypt, or otherwise transform the payload can send the original bytes with headers describing the transformed bytes, even when no retry occurs. Preserve the editor-finalized body for the initial attempt and disable replay when it cannot be reproduced instead of treating a potentially stale GetBody as authoritative.
Useful? React with 👍 / 👎.
Problem
The generated client's
doWithRetryrebuilds each attempt's request viabuildRequest(), which recreates it from the caller's single-useio.Reader. The raw*WithBody(..., body io.Reader)methods therefore shipped an empty (or, after a partial-read network failure, mid-stream) body on every retry attempt. All 42 idempotent raw*WithBodyops (Update*/Reposition*/Set*/Replace*/Toggle*/MarkAsRead, reaching BC3 over PUT/DELETE) retry, so a transient 429/503/network failure could resend a truncated body.Fix
doWithRetrysnapshots the first finalized request's body once (afterbuildRequest+applyEditors) and replays it, under a documented,net/http-consistent contract:req.Body.GetBodypresent (retry-eligible) →GetBodyis authoritative. This attempt is normalized to sendGetBody()'s bytes too, so attempt 1 and every retry are byte-identical even if an editor mutatedreq.Bodyout of sync withGetBody(including an in-placestrings.Reader.Reset). A large body with a validGetBodystays fully replayable.GetBody→ an unrewindable, single-use stream: not buffered or retried. Buffering would risk unbounded memory, and a blocking reader cannot be interrupted (there is no general way to abort anio.Reader), so the request is sent exactly once.Because retry keys off
GetBody, the SDK's own serialized bodies must be snapshotable.marshalBody(and the multipart account-logo path) now return*bytes.Readerinstead of the old customrewindableReader(which had noGetBody), so the eight hand-written idempotent update methods (Todos/Cards/CardSteps/Checkins/Projects/People/Schedules.Update, etc.) andUpdateAccountLogokeep retrying on transient failures.rewindableReaderis removed. Only an external caller passing a genuinely streaming, non-snapshotable reader to a raw*WithBodymethod forgoes retries — the safe behavior for an unrewindable stream.On a retry the captured body is installed before the editors run (and re-installed after), so a body-aware editor (Digest/HMAC) signs the bytes actually sent, and an editor-installed retry body is discarded and closed. A GetBody snapshot failure disables retries (single attempt with the valid body) rather than failing an otherwise-sendable request, and an installed replay body is closed if a retry editor errors.
ContentLengthis restored per replay path. The fix lives ingo/templates/client.tmpl(regenerated).Tests
Outside
pkg/generatedper the repo rule — 18 tests, measured against the shippedd5604962client:ContentLength, editor-body inconsistency (replace + in-placeReset), retry-editor body discarded+closed, leaked replay body on editor error, orphaned pre-editor replay body when a retry editor replacesreq.Body(successful and replace-then-error paths), the Digest/HMAC regression; and an empty-body transfer-framing proof through a realnet/httpTransport (a wrappedhttp.NoBodywould switch the retry to chunked encoding — a fake doer can't observe framing).GetBodymust not be demoted; a GetBody snapshot failure must not fail the request; an explicitly empty body (http.NoBody) must be replayed as a non-nil empty body; and a body-aware editor observes the replay body on a retry.Plus a public-service retry proof (
ProjectsService.Updateretries a 503 and replays the full serialized body — red-proofed against a non-snapshotable body) and amarshalBodysnapshotability test. Each proof is individually red-proofed. Go conformance's naturally-idempotent PUT/DELETE cases pass;-race, drift,go vet,golangci-lintall green.Behavior note
For retry-eligible
*WithBodyops, attempt 1 now sendsGetBody()'s bytes (identical for correct callers). All public typed methods retry (their bodies are*bytes.Reader); only an external caller passing a non-snapshotable streaming reader to a raw*WithBodymethod is sent once. A request editor changes a retrying request's body only viareq.GetBody; an explicitly empty body (http.NoBody) stays a non-nil empty reader across retries. No API signature changes.Part of the retry-contract follow-up (#456 / #460 / #461 / #476). #482 and #483 are merged; this branch is rebased on top of them (shared
client.tmplregions are disjoint) and regenerated.