fix: retry corrupts the request body when the previous attempt's upload is still in flight

[knight42]

Problem

meshyd's RPC client retries 502/503/504 up to 50 times with SetDoNotParseResponse(true). When a proxy answers 503 before the request body upload finishes, retried requests reach meshy-serving truncated and shifted — surfacing as msgpack decode 400s (UnknownRemoteError) on /meshkit/unified-render-preview and friends. The corruption is silent: the wire Content-Length matches the corrupted body, so nothing fails client-side.

Root cause (two defects compounding):

1. Shared body cursor across attempts. For []byte/string/marshaled bodies, handleRequestBody caches a single bytes.Reader/strings.Reader and createHTTPRequest hands that same reader to every attempt; Backoff only Seek(0)s it. But net/http keeps writing the request body in a background goroutine after an early response (the Issue 7569 path in transport.go), so the previous attempt's writeLoop keeps advancing the shared cursor while the retry reads it — an unsynchronized data race (go test -race flags bytes.(*Reader).Seek at retry.go vs the transport read). The same applies to user-supplied seekable readers (SetBody(bytes.NewReader(...))), which bypass bodyReadSeeker.
2. Discarded responses never closed. Under DoNotParseResponse, execute returns without reading or closing the body, and the retry loop discards intermediate responses without closing them either. An unread body parks the transport readLoop, keeping the old connection — and its still-running upload — alive indefinitely across retries. (A body-less 503 doesn't trigger the bug because net/http then tears the connection down ~50ms after the response; real proxy/serving 503s carry bodies.)

Closing the discarded response alone is not sufficient: any close runs after Backoff's Seek(0), and the dying writeLoop consumes another ~100–260 KiB before the connection teardown kills it, with nothing rewinding afterward — reproduced 3/3 with an OnRetry-close mitigation.

Fix

• createHTTPRequest: each attempt gets its own snapshot of the body — a shallow copy for bytes.Reader/strings.Reader (cp := *v, the same idiom http.NewRequest uses for GetBody) and bytes.NewReader(v.Bytes()) for *bytes.Buffer: a fresh cursor over the same backing array, no data copy. A still-running previous upload reads its own copy and cannot interfere by construction; NewRequest recognizes the concrete types and derives Content-Length framing and GetBody itself. An io.ReaderAt+io.Seeker body (e.g. *os.File) gets a per-attempt io.SectionReader over its probed bounds — ReadAt is stateless, so attempts cannot interfere. Any other reader with retries enabled is rejected at request-build time with a clear error, instead of silently corrupting the retried request (streaming multipart is exempt: it replays via a fresh pipe per attempt). Without retries, such readers keep streaming with no copy.
• Backoff: once the retry is committed (after the wait), close — without draining at this layer, which would be time-unbounded on a stalled body — the discarded response's body; every abort path (RetryAfter error, context cancellation) returns the response with its body untouched for the caller. The retry loop no longer touches request bodies at all. Requires Go 1.27, where the transport itself drains a closed body with hard bounds (256 KiB / 50 ms, maybeDrainBody) and reuses the connection when the drain completes. The exhausted final attempt's response is left open for the caller, who must be able to read the error payload under DoNotParseResponse.

Validation

• TestRetryBodyNotCorruptedByInFlightUpload (table-driven over []byte, *bytes.Reader, *bytes.Buffer, and ReaderAt-sectionable bodies; TestRetryNonReplayableBodyRejected covers the fail-fast path) drives the early-503 + slow-drain scenario end to end: both variants fail on v2 HEAD (ContentLength=15794176 with Body length 15532032), pass with the fix, including -race.
• TestRetryExhaustedResponseBodyReadable guards the new close logic: passes on v2 HEAD and with the fix, fails if the final response is closed.
• Full suite: go test ./... -count=1 passes; go vet ./... clean.
• A standalone 64 MiB reproducer against meshyd's exact client config (503-retry, SetCloseConnection, SetDoNotParseResponse) shows 8.2M corrupted words shifted by +1343488 before the fix, and a byte-identical body plus 0 race-detector warnings after.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Three moderate issues remain in retry-body handling and response cleanup.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes retry-time request-body corruption and improves cleanup of discarded retry responses.

Changes:

• Creates independent per-attempt body readers.
• Drains and closes discarded responses.
• Adds an end-to-end regression test.

File summaries

File	Summary	Review findings
retry.go	Handles retry response cleanup and body rewinding.	Two moderate issues: the final response may be closed, and draining can block indefinitely.
retry_test.go	Adds regression coverage for in-flight upload corruption.	No final comments.
middleware.go	Builds per-attempt request-body views.	Moderate issue: direct seekable reader bodies may still share a cursor.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The closable-reader lifecycle and fallback rewind findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

middleware.go:270

• This optimization can drop the close responsibility for a valid user-supplied body. SetBody accepts arbitrary io.Reader, so a reader that also implements io.ReadCloser, io.ReadSeeker, io.ReaderAt, and Size() enters this branch; replacing it with a SectionReader means http.Client only closes the new no-op wrapper, not the original reader. That leaks resources such as a custom file/stream wrapper (and the same closer is also absent from GetBody). Preserve the original closer with an appropriate per-request lifetime, or exclude closable readers from this optimization.

	if bodyAt != nil {
		if bodySize == 0 {
			reqBody = http.NoBody
		} else {
			reqBody = io.NewSectionReader(bodyAt, bodyOff, bodySize)

retry.go:163

• createHTTPRequest only installs the SectionReader when Seek(0, io.SeekCurrent) succeeds, but this retry check treats every ReaderAt+Size body as if it were section-backed. A custom io.ReadSeeker can implement ReaderAt/Size while returning an error for that position probe; in that fallback path the transport consumes the shared seeker, then this branch skips the rewind and the next attempt reuses the exhausted cursor. Track whether the section view was actually installed (or otherwise fall back to the existing seek) before suppressing the rewind.

			if rs, ok := resp.Request.Body.(io.ReadSeeker); ok {
				// Same split as above: ReaderAt+Size readers are viewed via
				// per-attempt SectionReaders and must not be seeked.
				if _, _, ok := readerAtWithSize(rs); !ok {
					if _, err := rs.Seek(0, io.SeekStart); err != nil {
						return err
					}

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Retry setup can fail after closing the current response, and *bytes.Buffer bodies still require replay handling.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

middleware.go:227

• *bytes.Buffer is another supported io.Reader that http.NewRequestWithContext treats as replayable, but it falls through this helper and is passed unchanged on every attempt. After an early response, the first transport can still advance the buffer (or leave it at EOF), so a retry sends a truncated/empty body just like the bug this PR fixes. Add a shallow *bytes.Buffer snapshot here and cover it in the regression table.

retry.go:143

• This closes the response before the retry is guaranteed to happen: the Seek/resetFieldReaders calls below can fail, and sleepDuration or context cancellation can abort before the next operation(). Request.Execute still returns this same resp on those paths, so with DoNotParseResponse its RawBody() is already closed even though no replacement attempt occurred. Defer the close until retry setup and waiting have succeeded and immediately before the next attempt, or otherwise preserve the body on every abort path.

			if attempt < opts.maxRetries {
				if raw := resp.RawResponse; raw != nil && raw.Body != nil {
					_ = raw.Body.Close()
				}
			}

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Response-close timing, Go-version documentation, and string-reader test coverage need updates.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

retry.go:144

• This closes the response before sleepDuration and the context wait have confirmed that another attempt will actually run. If a custom RetryAfter returns an error (documented as a way to stop retrying), or the context is canceled during the wait, Backoff returns this same resp through Request.Execute without a new attempt, but its body is already closed; a DoNotParseResponse caller then cannot read the final error payload. Move the close until after the wait succeeds, immediately before starting the next attempt, or otherwise close only once a retry is committed.

retry_test.go:902

• The new regression test covers the []byte path (which creates a *bytes.Reader) and a direct *bytes.Reader, but not the separate *strings.Reader branch added in snapshotReader. Add string and direct *strings.Reader cases here so a regression in that retry path cannot pass the suite.

		"bytes body":  {body: payload},
		"reader body": {body: bytes.NewReader(payload)},

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Moderate issues remain around supported body types, abort-path response handling, generic readers, and Go/Bazel compatibility metadata.

Review details

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

middleware.go:229

• SetBody documents io.Reader as a supported body type, but *bytes.Buffer is not handled here. The original buffer is therefore passed to every attempt; if the first upload is still reading after an early response, the retry shares its cursor and can compute a shorter Content-Length and send truncated/shifted data—the same failure this helper fixes for *bytes.Reader and *strings.Reader. Add a shallow-copy case for *bytes.Buffer and cover it in the regression table.

go.mod:3

• This raises the module's minimum toolchain to Go 1.27, but README.md still says Go 1.20+ is recommended and lists Go 1.11+ as supported. That now gives users a false compatibility promise and should be updated to reflect the new minimum (or the go directive should not be raised).

go 1.27.0

go.mod:3

• This raises the module requirement to Go 1.27, but the repository's supported Bazel path still registers Go 1.19 (WORKSPACE:27) and README advertises Bazel build/test support (README.md:891-895). Because this fix relies on Go 1.27's bounded net/http response-body close behavior, Bazel consumers do not receive the stated stalled-body guarantee; update the Bazel toolchain/support metadata or make cleanup independent of that runtime version.

go 1.27.0

retry.go:140

• The body is closed before sleepDuration, the request-body reset, and the context wait have succeeded. If a custom RetryAfter returns an error, a seeker reset fails, or cancellation wins the following select, no next attempt runs and Request.Execute still returns this response; with DoNotParseResponse, its RawBody is already closed and the caller cannot inspect the error payload. Defer closing until the retry is guaranteed to proceed, or preserve the response when any of these abort paths is taken.

			if attempt < opts.maxRetries {
				if raw := resp.RawResponse; raw != nil && raw.Body != nil {
					_ = raw.Body.Close()
				}
			}

retry.go:158

• Closing the response does not join with net/http's still-running body write loop. For a supported io.ReadSeeker supplied through SetBody that is not exactly one of these concrete types (for example, a wrapper), this fallback still seeks the shared reader while the previous attempt may be reading it, so the race and body corruption remain. The fallback needs an independent snapshot or synchronization before Seek.

			if rs, ok := resp.Request.Body.(io.ReadSeeker); ok {
				// Same split as above: snapshotable readers are copied per
				// attempt and must not be seeked.
				if _, snapped := snapshotReader(rs); !snapped {
					if _, err := rs.Seek(0, io.SeekStart); err != nil {

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Six moderate findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

go.mod:3

• This module directive raises the minimum toolchain to Go 1.27, but README.md still advertises Go 1.20+ as the recommended/supported version and does not document the new requirement. Users following the published support guidance will encounter module/toolchain failures; update the supported-version documentation with this change.

go 1.27.0

middleware.go:270

• This wrapper holds g.mu while calling an arbitrary io.ReadSeeker.Read. If a wrapped seeker blocks or is very slow, v.Close() cannot acquire the mutex to poison the stale attempt, so Backoff can remain stuck indefinitely after an early response instead of starting the retry. Avoid holding the retry-control lock across an unbounded user read, or explicitly fail/abort this non-snapshotable case.

	return v.g.rs.Read(p)

retry.go:175

• guardedBodyView.Close takes g.mu, while Read holds that same mutex for the entire underlying rs.Read. If a custom or slow io.ReadSeeker blocks during the previous upload, this call waits indefinitely before closing the response or starting the retry, so the close-only path can still hang on a stalled request body. The fencing/cleanup needs an interruptible ordering or another design that does not wait on a non-interruptible reader here.

			if v := resp.Request.bodyGuardView; v != nil {
				_ = v.Close()
				if err := resp.Request.bodyGuard.rewind(); err != nil {

retry.go:131

• The old retry path rewound bodyReadSeeker and Request.Body before every retry hook; this change removes both rewinds and only resets multipart fields. For a direct non-snapshot io.ReadSeeker, the first transport attempt has already advanced the original reader, so an OnRetry hook that inspects the request body now sees EOF/partial data, and the hook on the final attempt is never rewound at all. Preserve a safe start-of-body view for hooks or explicitly document this API behavior change.

		// Multipart field readers are reset on every retried attempt, before
		// the hooks: retry hooks rely on reading them from the start (see
		// TestResetMultipartReaders), including on the final attempt.

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved body-guard, blocking-read, close-semantics, and retry-hook issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

retry.go:176

• For the guarded-seeker fallback, the request is rewound only after retryHooks run (the previous implementation rewound resp.Request.Body before invoking them). An OnRetry hook that inspects a wrapped seekable request body now observes the cursor at EOF, and the final-attempt hook has the same regression. Preserve a safe reset/snapshot for hooks before moving the rewind to the post-wait commit point.

middleware.go:295

• bodyGuard is cached on the Request without being tied to the body passed to attemptBodyView. Since user OnBeforeRequest middleware runs again on every retry and may replace Request.Body, changing from one opaque io.ReadSeeker to another reuses the first seeker's guard, so later attempts upload the original body instead of the replacement. Reinitialize the guard (and its view) whenever the underlying seeker changes.

		if r.bodyGuard == nil {
			start, err := rs.Seek(0, io.SeekCurrent)
			if err != nil {
				start = 0
			}

middleware.go:280

• This wrapper replaces a user-supplied io.ReadCloser seeker, but Close never delegates to the underlying reader. http.NewRequest/Transport normally preserve and close an io.ReadCloser request body after each attempt, so passing a file or custom closer now leaks its descriptor and skips its cleanup (and can prevent Close from unblocking a blocked read). Preserve the underlying body's close semantics while still keeping it open when a retry needs to rewind it.

// Close revokes this view's license to read. It never closes the underlying
// seeker: the transport closes the body after every attempt, the seeker must
// stay usable for the next one, and the caller owns the resource anyway.
func (v *guardedBodyView) Close() error {
	v.g.mu.Lock()
	v.closed = true
	v.g.mu.Unlock()
	return nil

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect reader buffering, resource cleanup, and documented Go-version compatibility.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

go.mod:3

• go 1.27.0 raises the module's minimum toolchain, but the public Supported Go Versions section still says Go 1.20 and above (README.md:86). Users following that documentation will fail to build with older Go versions; please update the support documentation in the same change.

go 1.27.0

middleware.go:520

• The new comment contains a duplicated clause (A reader resty cannot snapshot per attempt cannot be replayed), making the explanation grammatically unclear.

		// A reader resty cannot snapshot per attempt cannot be replayed

middleware.go:531

• When retries are enabled, a non-snapshotable io.Reader that also implements io.Closer (for example an *os.File or *io.PipeReader) is fully consumed here and replaced with r.bodyReadSeeker, so the original closer is never passed to net/http and is never closed. Before this path, the transport closed an io.ReadCloser request body; defer closing the original while buffering, including when io.ReadAll returns an error.

		if c.RetryCount > 0 {
			if _, ok := snapshotReader(body); !ok {
				data, err := io.ReadAll(body)
				if err != nil {
					return err
				}
				r.bodyReadSeeker = bytes.NewReader(data)

retry.go:167

• This comment reads and unclosed it keeps; reword it so the subject is clear.

		// response: nothing else will (e.g. DoNotParseResponse), and unclosed
		// it keeps the old connection and its possibly still-running upload

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Address multipart reader handling, closable body lifetime, and the unbounded test receive.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

retry_test.go:988

• This unbounded receive can hang the entire test suite when a request fails before the second server handler sends a result; the listener's read deadline does not unblock a receive when no result is sent. Use a timeout select and fail the test on timeout.

			got := <-results

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite