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.
💡 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".
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".
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 decides once how a retry reproduces the first finalized body,
under a documented, net/http-consistent contract that never rewrites req.Body:
- Attempt 1 always sends req.Body exactly as buildRequest + the request
editors left it, so an editor that compresses/encrypts/transforms the
payload (and sets matching headers) is honored — even with no retry.
- A retry replays via req.GetBody, exactly as net/http replays a body on a
307/308 redirect — but only when the builder's body survived the editors
unchanged (checked against the pre-editor reference) and GetBody probes OK.
- If an editor REPLACED req.Body, the builder's GetBody is stale and the
finalized bytes cannot be reproduced, so the request is sent once (not
retried with the wrong body). An in-place mutation without a GetBody update
is undetectable and, like a redirect, would replay GetBody's snapshot;
editors should replace the body or keep req.GetBody in sync.
- No GetBody / non-idempotent -> single attempt; nil and http.NoBody are
reproduced faithfully (the sentinel is installed unwrapped so retry framing
stays identical — Content-Length: 0, never chunked).
The SDK's own hand-written idempotent updates (Todos/Cards/CardSteps/Checkins/
Projects/People/Schedules.Update + UpdateAccountLogo) marshal bodies via
marshalBody, which now returns *bytes.Reader (net/http snapshots it into
GetBody) instead of the old rewindableReader, so they keep retrying. On a retry
the replay body is installed before the editors run (so body-aware editors sign
the sent bytes) and owned via a close-once handle that is released after the
editor phase even if an editor replaces it. ContentLength is restored per path.
Fix lives in go/templates/client.tmpl (regenerated) plus helpers.go. Tests: 18
raw-*WithBody replay tests outside pkg/generated (9 fail against the shipped
client — empty/truncated retry body, wrong ContentLength, replaced-body sent
verbatim then not retried, in-place mutation sent on attempt 1, empty-body
framing through a real net/http Transport, leaked/orphaned replay handles,
etc.; the rest regression proofs and controls; each red-proofed); a public-
service retry proof (ProjectsService.Update); and a marshalBody snapshotability
test. Go conformance's naturally-idempotent PUT/DELETE cases pass.
No API signature changes.
Follows the retry-contract program (#456/#460/#461/#476).
248535a to
47bbdb5
Compare
|
@jeremy — heads up on a contract decision (this is a reversal worth your eye): The earlier review had me make attempt 1 normalize I switched to the net/http-consistent contract: attempt 1 sends
Fixed + red-proofed; 18 tests; conformance 93/0; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
go/templates/client.tmpl:174
- The Replay contract comment says the first attempt is “normalized to send GetBody()'s bytes”, but the implementation (and the later captureReplayBody comment) explicitly keeps attempt 1 as
req.Bodyfinalized by editors and only usesGetBodyfor retries. This mismatch is likely to confuse future maintainers about what bytes are actually sent on attempt 1 vs retries.
// GetBody authoritative: this attempt is ALSO normalized to send GetBody()'s
// bytes, so every attempt — first and retries — ships identical bytes. A
// retrying request's body is therefore changed by an editor only through
// req.GetBody (set req.Body and req.GetBody together, as net/http requires).
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
doWithRetrynow decides once how a retry reproduces the first finalized body, under a documented,net/http-consistent contract that never rewritesreq.Body:req.Bodyexactly asbuildRequest+ the request editors left it — so an editor that compresses/encrypts/transforms the payload (and sets matching headers) is honored, even with no retry.req.GetBody, exactly as net/http replays a body on a 307/308 redirect — but only when the builder's body survived the editors unchanged (checked against the pre-editor reference) andGetBodyprobes OK.req.Body, the builder'sGetBodyis stale and the finalized bytes can't be reproduced, so the request is sent once (not retried with the wrong body). An in-place mutation without aGetBodyupdate is undetectable and, like a redirect, would replayGetBody's snapshot — editors should replace the body or keepreq.GetBodyin sync.GetBody/ non-idempotent → single attempt.nilandhttp.NoBodyare reproduced faithfully (the sentinel is installed unwrapped so retry framing stays identical —Content-Length: 0, never chunked).The SDK's own hand-written idempotent updates (
Todos/Cards/CardSteps/Checkins/Projects/People/Schedules.Update+UpdateAccountLogo) marshal bodies viamarshalBody, which now returns*bytes.Reader(net/http snapshots it intoGetBody) instead of the oldrewindableReader, so they keep retrying. On a retry the replay body is installed before the editors run (so body-aware editors sign the sent bytes) and owned via a close-once handle released after the editor phase even if an editor replaces it.ContentLengthis restored per path. The fix lives ingo/templates/client.tmpl(regenerated) plushelpers.go.Tests
Outside
pkg/generatedper the repo rule — 18 tests (9 fail against the shipped client): empty/truncated retry body, wrong retryContentLength, no-GetBody stream sent once, partial-read byte-0 restart, replaced body sent verbatim then not retried (with and without a matching GetBody), in-place mutation sent on attempt 1, empty-body transfer framing through a realnet/httpTransport, orphaned/leaked replay handles (replacement + replace-then-error), a signing editor (readsGetBody) whose digest matches the sent body on both attempts, and empty-body-not-nil; the rest are regression proofs and controls. Each proof is individually red-proofed. Plus a public-service retry proof (ProjectsService.Update) and amarshalBodysnapshotability test. Go conformance's naturally-idempotent PUT/DELETE cases pass;-race, drift,go vet,golangci-lintgreen.Behavior note
Attempt 1 sends
req.Bodyas the editors finalized it (honoring body-transforming editors); retries replay viareq.GetBodywhen the body was not replaced, else the request is sent once. Editors that transform the body must keepreq.GetBodyin sync for retries (per the net/http contract). 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.