fix: Close the original response body on non-2xx responses by yavorl · Pull Request #4486 · google/go-github

yavorl · 2026-08-25T19:56:04Z

CheckResponse substitutes r.Body with a re-readable NopCloser copy on every
non-2xx response (#1363). Since #1772, the error-path defer resp.Body.Close() in
bareDo is registered after that substitution, so the deferred close releases the
copy and the network body is never closed. CopilotService.fetchMetricsReport and RepositoriesService.downloadReleaseAssetFromURL have the same pattern.

Consequences of the unclosed network body:

With an http.Client that has Timeout set and a wrapped transport (which
includes clients built via WithAuthToken), every non-2xx response parks one
net/http.setRequestCancel.func4 goroutine for the remainder of the timeout.
Bounded, but it intermittently fails goleak-checked test suites downstream.
Error bodies larger than maxErrorBodySize are only partially drained, so the
connection is additionally lost. 1Mb seems generous so it can be a non-issue.

The fix captures the network body before calling CheckResponse and closes that
instead. The #1363 behavior (re-readable error bodies) is
unchanged: the substitute is left open for callers.

The regression test wraps the test client's transport to observe the network
body and asserts it is closed after an error response. It fails on master and
passes with this change; the full package test suite passes.

commit 1: a failing regression test
commit 2: the fix; capture the original body & close it
commit 3: similar regression tests for the other 2 call sites
commit 4: fix the 2 remaining callers & update the CheckResponse() godoc

CheckResponse substitutes r.Body with a re-readable NopCloser copy on error responses, so asserting on resp.Body.Close alone cannot catch a leak of the network body. Wrap the test client's transport and record whether the body it returned is closed. This test fails at this commit; the fix follows in the next one.

google-cla · 2026-08-25T19:56:16Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

`CheckResponse` substitutes `r.Body` with a re-readable `NopCloser` copy on every non-2xx response (google#1363). Since google#1772, the error-path `defer resp.Body.Close()` in `bareDo` is registered after that substitution, so the deferred close releases the copy and the network body is never closed. `CopilotService.fetchMetricsReport` and `RepositoriesService.downloadReleaseAssetFromURL` have the same pattern. Consequences of the unclosed network body: - With an `http.Client` that has `Timeout` set and a wrapped transport (which includes clients built via `WithAuthToken`), every non-2xx response parks one `net/http.setRequestCancel.func4` goroutine for the remainder of the timeout. Bounded, but it intermittently fails `goleak`-checked test suites downstream. - Error bodies larger than `maxErrorBodySize` are only partially drained, so the connection is additionally lost. The fix captures the network body before calling CheckResponse and closes that instead. The google#1363 behavior (re-readable error bodies) is unchanged: the substitute is left open for callers.

codecov · 2026-08-25T20:41:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.52%. Comparing base (f9fec07) to head (7356d4a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4486   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files         195      195           
  Lines       17729    17732    +3     
=======================================
+ Hits        17468    17471    +3     
  Misses        261      261

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

yavorl · 2026-08-25T20:44:28Z

+type closeRecorder struct {
+	io.ReadCloser
+	closed *bool
+}


having this round tripper is also a precondition for the bug. Not a necessary precondition for the test because we're asserting that the underlying cause can't happen - Close() not being called.

gmlewis

Ah, I see what you are saying, @yavorl.
I would like to cover all possible unclosed bodies, if possible, so that this issue can truly be recorded as solved. What are your thoughts as to what it would take to accomplish this?

LGTM.

cc: @stevehipwell - @alexandear - @Not-Dhananjay-Mishra

yavorl · 2026-08-25T23:59:12Z

Ah, I see what you are saying, @yavorl. I would like to cover all possible unclosed bodies, if possible, so that this issue can truly be recorded as solved. What are your thoughts as to what it would take to accomplish this?

LGTM.

cc: @stevehipwell - @alexandear - @Not-Dhananjay-Mishra

The fix is the same, I have to see whether the test pattern translates to the other invocations. Would you like me to try it in this PR or a separate follow-up?

gmlewis · 2026-08-26T00:11:09Z

Well, I'm fine either way. If you want to keep it separate in a new PR, that is fine with me too. Thank you, @yavorl!

yavorl · 2026-08-26T00:51:30Z

Done it in the same PR. Added a comment as well to the godoc of CheckResponse()

gmlewis

Thank you, @yavorl!
LGTM.

alexandear · 2026-08-28T16:46:53Z

+	// CheckResponse substitutes r.Body with a re-readable copy on error
+	// responses, so capture the network body first: it is the one that must
+	// be closed.
+	origBody := resp.Body


Could we centralize the fix to one place? I think it's possible to patch only CheckResponse.

The patch is something like this:

diff --git a/github/copilot.go b/github/copilot.go index adbfae32..9dd24eb8 100644 --- a/github/copilot.go +++ b/github/copilot.go @@ -1212,12 +1212,8 @@ func (s *CopilotService) fetchMetricsReport(ctx context.Context, url string) (*h return nil, nil, err } - // CheckResponse substitutes resp.Body with a re-readable copy on error - // responses, so capture the original body first: it is the one that must - // be closed. - origBody := resp.Body if err := CheckResponse(resp); err != nil { - _ = origBody.Close() + resp.Body.Close() return nil, newResponse(resp), err } diff --git a/github/copilot_test.go b/github/copilot_test.go index ee1f804a..4ddbab41 100644 --- a/github/copilot_test.go +++ b/github/copilot_test.go @@ -3212,8 +3212,8 @@ func TestCopilotService_DownloadDailyMetrics(t *testing.T) { } } -// CheckResponse substitutes resp.Body with a re-readable copy on error -// responses; fetchMetricsReport must still close the original body it replaces. +// CheckResponse closes the original resp.Body and substitutes a re-readable +// copy on error responses; fetchMetricsReport must not leak the original body. func TestCopilotService_fetchMetricsReport_closesOriginalBodyOnErrorResponse(t *testing.T) { t.Parallel() client, mux, _ := setup(t) diff --git a/github/github.go b/github/github.go index efb4c677..bfa0bd10 100644 --- a/github/github.go +++ b/github/github.go @@ -1282,13 +1282,9 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro c.rateMu.Unlock() } - // CheckResponse substitutes r.Body with a re-readable copy on error - // responses, so capture the network body first: it is the one that must - // be closed. - origBody := resp.Body err = CheckResponse(resp) if err != nil { - defer origBody.Close() + defer resp.Body.Close() // Special case for AcceptedErrors. If an AcceptedError // has been encountered, the response's payload will be // added to the AcceptedError and returned. @@ -1795,11 +1791,9 @@ func (e *Error) UnmarshalJSON(data []byte) error { // API error responses are expected to have response // body, and a JSON response body that maps to [ErrorResponse]. // -// On error responses other than 202 Accepted, CheckResponse consumes r.Body -// and replaces it with an in-memory copy so that the error body can be -// re-read. Closing r.Body after CheckResponse returns therefore closes only -// the copy: to release the original body and its underlying connection, -// capture r.Body before the call and close the captured body instead. +// On error responses other than 202 Accepted, CheckResponse consumes and +// closes r.Body, then replaces it with an in-memory copy so that the error +// body can be re-read by the caller. // // The error type will be *[RateLimitError] for rate limit exceeded errors, // *[AcceptedError] for 202 Accepted status codes, @@ -1822,9 +1816,11 @@ func CheckResponse(r *http.Response) error { errorResponse = &ErrorResponse{Response: r} } } - // Re-populate error response body because GitHub error responses are often - // undocumented and inconsistent. + // Close the original body and re-populate the error response body with an + // in-memory copy, because GitHub error responses are often undocumented and + // inconsistent and the caller may need to re-read them. // Issue #1136, #540. + _ = r.Body.Close() r.Body = io.NopCloser(bytes.NewBuffer(data)) switch { case r.StatusCode == http.StatusUnauthorized && strings.HasPrefix(r.Header.Get(headerOTP), "required"): diff --git a/github/github_test.go b/github/github_test.go index 776abaf4..77b346c6 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -2308,8 +2308,8 @@ func (r *closeRecorder) Close() error { return r.ReadCloser.Close() } -// CheckResponse substitutes resp.Body with a re-readable copy on error -// responses; the network body it replaces must still be closed. +// CheckResponse closes the original resp.Body and substitutes a re-readable +// copy on error responses; the network body must not leak. func TestDo_closesOriginalBodyOnErrorResponse(t *testing.T) { t.Parallel() client, mux, _ := setup(t) diff --git a/github/repos_releases.go b/github/repos_releases.go index 3361a5d7..889afb6d 100644 --- a/github/repos_releases.go +++ b/github/repos_releases.go @@ -375,12 +375,8 @@ func (s *RepositoriesService) downloadReleaseAssetFromURL(ctx context.Context, f if err != nil { return nil, err } - // CheckResponse substitutes resp.Body with a re-readable copy on error - // responses, so capture the original body first: it is the one that must - // be closed. - origBody := resp.Body if err := CheckResponse(resp); err != nil { - _ = origBody.Close() + _ = resp.Body.Close() return nil, err } return resp.Body, nil diff --git a/github/repos_releases_test.go b/github/repos_releases_test.go index 4fb85c75..4208513d 100644 --- a/github/repos_releases_test.go +++ b/github/repos_releases_test.go @@ -537,9 +537,9 @@ func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToError(t *testi } } -// CheckResponse substitutes resp.Body with a re-readable copy on error -// responses; downloadReleaseAssetFromURL must still close the original body it -// replaces. Unlike its sibling tests, the recorder wraps the follow-redirects +// CheckResponse closes the original resp.Body and substitutes a re-readable +// copy on error responses; downloadReleaseAssetFromURL must not leak the +// original body. Unlike its sibling tests, the recorder wraps the follow-redirects // client's transport: that client, not the library client, performs the // redirected request, so wrapping the library client would only ever observe // the first hop's correctly-closed redirect response and never the leak.

The reason I didn't do it initially is that this is a behaviour change, callers might start seeing errors where there were none before.

Reason being - Response.Body is only guaranteed to be an io.ReadCloser. Custom transports, test doubles, and direct CheckResponse callers routinely supply non-Transport bodies - so the only close semantics a public API may rely on are io.Closer's, which states that a second Close is undefined behaviour. So if someone was diligent about this before and they closed the body themselves, theoretically they can start seeing errors or panics.

That said, if you would still prefer to go with that approach after my argument. I'd be happy to apply the patch.

Fair point. Thanks for the explanation.

Thank you for the review & thank you for the merge @gmlewis

gmlewis · 2026-08-28T17:41:24Z

Thank you, @yavorl and @alexandear!
Merging.

yavorl marked this pull request as draft August 25, 2026 19:56

yavorl force-pushed the fix-error-response-body-close branch from 5c52ef3 to 25b7434 Compare August 25, 2026 20:35

yavorl commented Aug 25, 2026

View reviewed changes

yavorl marked this pull request as ready for review August 25, 2026 20:47

yavorl changed the title ~~fix: close the original response body on non-2xx responses~~ fix: Close the original response body on non-2xx responses Aug 25, 2026

gmlewis added the NeedsReview PR is awaiting a review before merging. label Aug 25, 2026

gmlewis approved these changes Aug 25, 2026

View reviewed changes

yavorl added 2 commits August 26, 2026 03:26

Add regression tests for the other 2 callers of CheckResponse()

afa7265

Fix the remaining call sites & update the method godoc

8c8468b

gmlewis approved these changes Aug 26, 2026

View reviewed changes

alexandear suggested changes Aug 28, 2026

View reviewed changes

alexandear approved these changes Aug 28, 2026

View reviewed changes

Merge branch 'master' into fix-error-response-body-close

7356d4a

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 28, 2026

gmlewis merged commit c48dbf5 into google:master Aug 28, 2026
15 checks passed

Uh oh!

Conversation

yavorl commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla Bot commented Aug 25, 2026

Uh oh!

codecov Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yavorl Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

yavorl commented Aug 25, 2026

Uh oh!

gmlewis commented Aug 26, 2026

Uh oh!

yavorl commented Aug 26, 2026

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

alexandear Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

alexandear Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

yavorl Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yavorl Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

alexandear Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

yavorl Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yavorl commented Aug 25, 2026 •

edited

Loading

codecov Bot commented Aug 25, 2026 •

edited

Loading

yavorl Aug 25, 2026 •

edited

Loading

yavorl Aug 28, 2026 •

edited

Loading