bug: Original response body is never closed

[yavorl]

go-github version: v87.0.0 (also present on master at time of writing)
Go version: 1.26

Summary

CheckResponse replaces r.Body with a buffered copy on every non-2xx response, before bareDo registers its deferred resp.Body.Close(). The deferred close therefore closes the in-memory replacement (io.NopCloser, a no-op) and the original body returned by http.Client.Do is never closed by anyone.

The unclosed body itself is symptomless: CheckResponse happens to read the original to EOF, so the connection is still reused, which is why this has gone unnoticed. It becomes observable when two conditions hold together:

1. The http.Client has Timeout set. net/http must then cancel the request at the deadline. For known stdlib transports it uses context.WithDeadline (no extra goroutine), but for any other RoundTripper it falls back to a legacy path (setRequestCancel) that wraps the body in cancelTimerBody and parks a watcher goroutine, which is stopped only by Close, not by read-to-EOF (client.go#L985-L1003 in Go 1.26.5).
2. The transport is a non-stdlib RoundTripper. This is the case for effectively every authenticated client: go-github's own WithAuthToken wraps the transport in a roundTripperFunc (github.go#L581), and the common alternatives (oauth2.Transport, ghinstallation, otelhttp.NewTransport) are wrappers too.

With both in place, every non-2xx API response parks one net/http.setRequestCancel.func4 goroutine for the full remainder of the client timeout.

The population is bounded (each goroutine self-terminates at the deadline), so this never accumulates into an unbounded leak, but any test suite using goleak (or similar) will flag these goroutines intermittently, and under a sustained error load a Timeout-sized window of goroutines stays resident (see Impact below).

The relevant code

bareDo (github.go#L1196-L1198 @ v87.0.0):

err = CheckResponse(resp)
if err != nil {
	defer resp.Body.Close()   // resp.Body is already the replacement here

CheckResponse (github.go#L1711-L1722 @ v87.0.0):

data, err := io.ReadAll(io.LimitReader(r.Body, maxErrorBodySize))
...
// Re-populate error response body because GitHub error responses are often
// undocumented and inconsistent.
// Issue #1136, #540.
r.Body = io.NopCloser(bytes.NewBuffer(data))

The body swap is intentional (#1136, #540) so callers can re-read error payloads; the bug is only that the original ReadCloser is dropped without being closed.

Reproduction

Self-contained, no credentials; go run . with the go.mod below.

// go.mod:
//   module ghleak
//   go 1.25
//   require github.com/google/go-github/v87 v87.0.0
package main

import (
	"context"
	"fmt"
	"net/http"
	"net/http/httptest"
	"runtime"
	"strings"
	"time"

	"github.com/google/go-github/v87/github"
)

func cancelTimerGoroutines() int {
	buf := make([]byte, 1<<20)
	n := runtime.Stack(buf, true)
	return strings.Count(string(buf[:n]), "net/http.setRequestCancel.func4")
}

// wrapper stands in for any custom RoundTripper: oauth2.Transport,
// otelhttp.NewTransport, ghinstallation, etc. With a wrapped transport,
// net/http's Client.Timeout falls back to its legacy cancel path, which
// parks a goroutine per request until the body is closed or the timeout
// elapses (net/http.setRequestCancel).
type wrapper struct{ base http.RoundTripper }

func (w wrapper) RoundTrip(r *http.Request) (*http.Response, error) { return w.base.RoundTrip(r) }

func main() {
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if strings.Contains(r.URL.Path, "/repos/o/found") {
			w.Write([]byte(`{"id": 1, "name": "found"}`)) // 200
			return
		}
		w.WriteHeader(404)
		w.Write([]byte(`{"message": "Not Found"}`))
	}))
	defer srv.Close()

	httpClient := &http.Client{
		Timeout:   10 * time.Second,
		Transport: wrapper{http.DefaultTransport},
	}
	gh, err := github.NewClient(
		github.WithHTTPClient(httpClient),
		github.WithEnterpriseURLs(srv.URL, srv.URL),
	)
	if err != nil {
		panic(err)
	}
	ctx := context.Background()

	for range 5 {
		if _, _, err := gh.Repositories.Get(ctx, "o", "found"); err != nil {
			panic(err)
		}
	}
	time.Sleep(100 * time.Millisecond)
	fmt.Printf("after 5 responses with status 200: %d cancel-timer goroutines\n", cancelTimerGoroutines())

	for range 5 {
		if _, _, err := gh.Repositories.Get(ctx, "o", "missing"); err == nil {
			panic("expected 404 error")
		}
	}
	time.Sleep(100 * time.Millisecond)
	fmt.Printf("after 5 responses with status 404: %d cancel-timer goroutines\n", cancelTimerGoroutines())

	time.Sleep(11 * time.Second)
	fmt.Printf("after the 10s client timeout elapses: %d cancel-timer goroutines\n", cancelTimerGoroutines())
}

Output:

after 5 responses with status 200: 0 cancel-timer goroutines
after 5 responses with status 404: 5 cancel-timer goroutines
after the 10s client timeout elapses: 0 cancel-timer goroutines

One goroutine parked per error response; 2xx responses (closed properly by Do) park none; all of them self-terminate when the client timeout elapses.

Impact

Each parked goroutine self-terminates when the client timeout elapses, so the steady-state population under sustained errors is bounded at roughly error responses/sec x Client.Timeout, at a few KB of stack each (plus the retained response/timer). For example, 10 error responses/sec with a 10s timeout holds ~100 goroutines resident (well under 1 MB); it takes extremes like 1,000/sec with an 80s timeout to reach ~80k goroutines / hundreds of MB. So in production this is mostly invisible; the practical cost is that goleak.VerifyTestMain (or similar leak detection code) in downstream test suites intermittently fails on net/http.setRequestCancel.func4 whenever the process exits within Timeout of the last non-2xx response, which is how we found it.

Notably, the library's own pre-emptive rate-limit check (checkRateLimitBeforeDo / checkSecondaryRateLimitBeforeDo) already dampens the rate-limited flavor of this: once a primary/secondary limit is recorded, subsequent calls short-circuit with a synthetic response and no network request, so they don't leak. The responses that do leak are the ones that always hit the network: 404s, non-rate-limit 403s, and 5xx.

How we hit it

Our integration suite runs goleak.VerifyTestMain. Tests that exercise GitHub error paths (4xx/5xx from a mock server) intermittently failed with batches of:

Goroutine NNN in state select, with net/http.setRequestCancel.func4 on top of the stack:
net/http.setRequestCancel.func4()
        .../src/net/http/client.go:407

whenever the process exited within Timeout seconds of the last non-2xx GitHub response.

Suggested fix

Capture the original body before CheckResponse gets a chance to replace it:

origBody := resp.Body
err = CheckResponse(resp)
if err != nil {
	defer origBody.Close()
	...

(or have CheckResponse close r.Body itself right after io.ReadAll, before reassigning it). bareDoIgnoreRedirects/other callers of CheckResponse may want the same treatment.

Disclaimer: I haven't actually tested the suggested fix resolves the issue but it should.

[gmlewis]

Pardon my skepticism, but this issue has been reported over and over again throughout the last decade and we have devoted a fair amount of effort to ensure that bodies are correctly closed.

So unless you can point out a recent commit that caused a regression in closing the body, this report is going to take a lower priority over other projects that are currently more demanding for my time.

[yavorl]

Feel free to triage it as you deem fit, but...

I haven't gone through the entire backlog, but the reproduction in the issue is deterministic (go run . ; ~90s). The mechanism is simple, the go-github is the caller of http.Client.Do,
so it owes the close on the body it received, and since #1363, CheckResponse substitutes the r.Body with a NopCloser without closing the original.

The regression commit is in #1772 - https://github.com/google/go-github/pull/1772/files#diff-9ba908d887f1d686a0a524b3cb3ff334b402122a9e5c4b8ba150a5fa6b5ff16bL561

Before this PR, Do had a single unconditional defer resp.Body.Close() registered before the CheckResponse() call, so it captured the original body. #1772 split it: the success path is
closed in Do after BareDo returns, and the error path got a new defer resp.Body.Close() registered after CheckResponse(). A deferred call captures its receiver at registration time, so on
the error path the defer now holds the substitute, and the original is never closed.

I'm happy to give the two-line fix PR a try (the capture of the original body + closing it) with a regression test.

[gmlewis]

Well, let's please take a look at your PR and see if it makes sense to incorporate into the repo.

[yavorl]

Submitted the PR.

One thing, while looking at the other callers. Both CopilotService.fetchMetricsReport (copilot.go:1215) and RepositoriesService.downloadReleaseAssetFromURL (repos_releases.go:378) have the same pattern. Both intentionally bypass the BareDo() call & call CheckResponse() directly, and then close the substituted body. So they are exhibiting the same behaviour and the BareDo() fix doesn't reach them.

I've deliberately left them out of the PR. I don't use those endpoints and didn't want to touch them without a maintainer's blessing. The same two-line fix applies if you want it. Happy to extend the PR or do a follow-up. I think it's best not to fix it inside CheckResponse() because it's a public method, that may lead to double closing in consumers, which is undefined behaviour for arbitrary io.Closer implementations, notable example being *os.File which returns os.ErrClosed on subsequent calls.

They also inherit the maxErrorBodySize consequence from the PR description: an error body larger than 1MiB is left partially drained as well as unclosed, so the connection is lost outright rather than just a timer goroutine parked. That said, 1MiB is a generous limit for error payloads, so this half is mostly theoretical. The parked goroutine is the consequence you'd actually observe.