From 2a7b2442e7f1638d8ac1dd39efb93083c5844bb6 Mon Sep 17 00:00:00 2001 From: Sushant Poudel Date: Thu, 29 Oct 2026 07:51:20 +0545 Subject: [PATCH 1/3] fix: honor http.Client.Timeout in roundTripWithOptionalFollowRedirect Fixes #4584. roundTripWithOptionalFollowRedirect calls the configured RoundTripper directly so it can inspect a 301/302 instead of following or surfacing it. That is the whole point of the helper, but it also skips http.Client.Do - and http.Client.Do is where http.Client.Timeout is applied. A caller using WithTimeout therefore got a request that the timeout did not bound, while the same request through the client's ordinary path returned context.DeadlineExceeded. The deadline is now attached once, around the whole helper rather than per hop, so it spans redirects and body reads the way http.Client.Timeout does. On the success path the cancel func moves to a body wrapper and is released when the caller closes the body (the helper already documents that closing as the caller's job); every other exit releases it locally, which is also what go vet flagged in the first draft - the timer leaked when a transport returned a nil body. Two regression tests, both failing on master and passing here: * a RoundTripper that waits on req.Context().Done() plus a 20ms client timeout now returns context.DeadlineExceeded instead of waiting out the caller's context. The transport also has a 2s fallback return, so a regression fails the assertion rather than hanging the suite. * a redirect chain asserts that every hop sees the same deadline, which is what "spans the whole request" means; a per-hop timeout would give each call its own (the test compares the two deadlines). `go test ./...` and `go vet ./github` are clean. --- github/github.go | 52 +++++++++++++++++++++++- github/github_test.go | 94 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletion(-) diff --git a/github/github.go b/github/github.go index 5311df61c0f..ed80b8ef8e1 100644 --- a/github/github.go +++ b/github/github.go @@ -2283,6 +2283,56 @@ func sleepUntilResetWithBuffer(ctx context.Context, reset time.Time) error { // When using roundTripWithOptionalFollowRedirect, note that it // is the responsibility of the caller to close the response body. func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u string, maxRedirects int, opts ...RequestOption) (*http.Response, error) { + // http.Client.Timeout is applied by http.Client.Do, which this helper deliberately + // does not use: it calls the transport directly so the first 301/302 response can be + // inspected instead of being followed or surfaced. That skipped the timeout, so a + // caller using WithTimeout got a request unbounded by it. Attach the deadline here, + // once, so that redirect hops share it and body reads stay bounded - the same span + // http.Client.Do provides. + if timeout := c.client.Timeout; timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, timeout) + // The deadline has to outlive this function so the caller can read the body, so + // the cancel func moves to the body on the success path. Any other exit releases + // it here instead of leaking the timer. + handedOff := false + defer func() { + if !handedOff { + cancel() + } + }() + resp, err := c.roundTripFollowingRedirect(ctx, u, maxRedirects, opts...) + if err != nil { + return nil, err + } + if resp.Body != nil { + resp.Body = &cancelOnCloseBody{ReadCloser: resp.Body, cancel: cancel} + handedOff = true + } + return resp, nil + } + return c.roundTripFollowingRedirect(ctx, u, maxRedirects, opts...) +} + +// cancelOnCloseBody releases the timeout attached by +// roundTripWithOptionalFollowRedirect when the caller closes the response, the +// way http.Client releases its own timer. +type cancelOnCloseBody struct { + io.ReadCloser + cancel context.CancelFunc +} + +func (b *cancelOnCloseBody) Close() error { + err := b.ReadCloser.Close() + b.cancel() + return err +} + +// roundTripFollowingRedirect performs the request described by +// roundTripWithOptionalFollowRedirect, following at most maxRedirects same-origin +// redirects. The deadline, when one applies, is attached by the caller so it spans +// every hop. +func (c *Client) roundTripFollowingRedirect(ctx context.Context, u string, maxRedirects int, opts ...RequestOption) (*http.Response, error) { req, err := c.NewRequest(ctx, "GET", u, nil, opts...) if err != nil { return nil, err @@ -2306,7 +2356,7 @@ func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u stri if err := c.checkRedirectHost(u); err != nil { return nil, err } - resp, err = c.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects-1, opts...) + resp, err = c.roundTripFollowingRedirect(ctx, u, maxRedirects-1, opts...) } return resp, err } diff --git a/github/github_test.go b/github/github_test.go index a7522882304..be82055575b 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -4033,6 +4033,100 @@ func TestRoundTripWithOptionalFollowRedirect_MissingRedirectLocation(t *testing. } } +// TestRoundTripWithOptionalFollowRedirect_HonorsClientTimeout is the regression +// for a client timeout that never reached the transport: the helper calls the +// configured RoundTripper directly to inspect the first response, which skips +// http.Client.Do - and http.Client.Do is where http.Client.Timeout is applied. +// A RoundTripper that waits for its request context therefore used to wait out +// the caller's context instead of the configured deadline. +func TestRoundTripWithOptionalFollowRedirect_HonorsClientTimeout(t *testing.T) { + t.Parallel() + + const timeout = 20 * time.Millisecond + transport := roundTripperFunc(func(req *http.Request) (*http.Response, error) { + select { + case <-req.Context().Done(): + return nil, req.Context().Err() + case <-time.After(2 * time.Second): + // Fails rather than hangs when the deadline is not propagated. + return nil, errors.New("transport never saw the client deadline") + } + }) + + client, err := NewClient(WithTimeout(timeout), WithTransport(transport)) + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + start := time.Now() + _, err = client.roundTripWithOptionalFollowRedirect(t.Context(), "archive", 0) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("Expected a timeout error, got nil.") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("Expected context.DeadlineExceeded, got: %v", err) + } + if elapsed > time.Second { + t.Errorf("Request took %v; the %v client timeout was not applied to the transport", elapsed, timeout) + } +} + +// TestRoundTripWithOptionalFollowRedirect_TimeoutCoversAllRedirects pins the +// http.Client.Timeout contract - the deadline spans the whole request, not one +// hop - by checking that every transport call in a redirect chain shares one +// deadline instead of getting a fresh timeout per hop. +func TestRoundTripWithOptionalFollowRedirect_TimeoutCoversAllRedirects(t *testing.T) { + t.Parallel() + + const timeout = 30 * time.Millisecond + var deadlines []time.Time + var mu sync.Mutex + var calls int + + client, _, _ := setup(t) + transport := roundTripperFunc(func(req *http.Request) (*http.Response, error) { + mu.Lock() + calls++ + n := calls + if dl, ok := req.Context().Deadline(); ok { + deadlines = append(deadlines, dl) + } + mu.Unlock() + + if n == 1 { + return &http.Response{ + StatusCode: http.StatusMovedPermanently, + Header: http.Header{"Location": []string{baseURLPath + "/archive-target"}}, + Body: http.NoBody, + Request: req, + }, nil + } + select { + case <-req.Context().Done(): + return nil, req.Context().Err() + case <-time.After(2 * time.Second): + return nil, errors.New("transport never saw the client deadline") + } + }) + client.client.Timeout = timeout + client.client.Transport = transport + + if _, err := client.roundTripWithOptionalFollowRedirect(t.Context(), "archive", 2); err == nil { + t.Fatal("Expected a timeout error, got nil.") + } + + mu.Lock() + defer mu.Unlock() + if len(deadlines) != 2 { + t.Fatalf("Expected 2 transport calls with a deadline, got %d", len(deadlines)) + } + if !deadlines[0].Equal(deadlines[1]) { + t.Errorf("Each hop got its own deadline (%v then %v); http.Client.Timeout spans the whole request", deadlines[0], deadlines[1]) + } +} + // TestRoundTripWithOptionalFollowRedirect_AllowsSameHostRedirect ensures the // cross-host check does not break legitimate same-host 301 follow behavior // (the path that rate-limit redirection relies on). From ccea1e9f2c031c53a8f9bc3cf6957afba7f06470 Mon Sep 17 00:00:00 2001 From: Sushant Poudel Date: Thu, 29 Oct 2026 11:31:25 +0545 Subject: [PATCH 2/3] fix: drop the unused RequestOption from the redirect helpers golangci-lint (unparam) reports that opts always receives nil in roundTripWithOptionalFollowRedirect (github.go:2285). Every caller passes no request options and the helper only forwards them, so the variadic is removed from both helpers and from the NewRequest call. --- github/github.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/github/github.go b/github/github.go index ed80b8ef8e1..afa1a75b66e 100644 --- a/github/github.go +++ b/github/github.go @@ -2282,7 +2282,7 @@ func sleepUntilResetWithBuffer(ctx context.Context, reset time.Time) error { // When using roundTripWithOptionalFollowRedirect, note that it // is the responsibility of the caller to close the response body. -func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u string, maxRedirects int, opts ...RequestOption) (*http.Response, error) { +func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u string, maxRedirects int) (*http.Response, error) { // http.Client.Timeout is applied by http.Client.Do, which this helper deliberately // does not use: it calls the transport directly so the first 301/302 response can be // inspected instead of being followed or surfaced. That skipped the timeout, so a @@ -2301,7 +2301,7 @@ func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u stri cancel() } }() - resp, err := c.roundTripFollowingRedirect(ctx, u, maxRedirects, opts...) + resp, err := c.roundTripFollowingRedirect(ctx, u, maxRedirects) if err != nil { return nil, err } @@ -2311,7 +2311,7 @@ func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u stri } return resp, nil } - return c.roundTripFollowingRedirect(ctx, u, maxRedirects, opts...) + return c.roundTripFollowingRedirect(ctx, u, maxRedirects) } // cancelOnCloseBody releases the timeout attached by @@ -2332,8 +2332,8 @@ func (b *cancelOnCloseBody) Close() error { // roundTripWithOptionalFollowRedirect, following at most maxRedirects same-origin // redirects. The deadline, when one applies, is attached by the caller so it spans // every hop. -func (c *Client) roundTripFollowingRedirect(ctx context.Context, u string, maxRedirects int, opts ...RequestOption) (*http.Response, error) { - req, err := c.NewRequest(ctx, "GET", u, nil, opts...) +func (c *Client) roundTripFollowingRedirect(ctx context.Context, u string, maxRedirects int) (*http.Response, error) { + req, err := c.NewRequest(ctx, "GET", u, nil) if err != nil { return nil, err } @@ -2356,7 +2356,7 @@ func (c *Client) roundTripFollowingRedirect(ctx context.Context, u string, maxRe if err := c.checkRedirectHost(u); err != nil { return nil, err } - resp, err = c.roundTripFollowingRedirect(ctx, u, maxRedirects-1, opts...) + resp, err = c.roundTripFollowingRedirect(ctx, u, maxRedirects-1) } return resp, err } From dd08a20b74335a3d0d802c7aac95bd6aa2a8c49d Mon Sep 17 00:00:00 2001 From: Sushant Poudel Date: Thu, 29 Oct 2026 11:32:21 +0545 Subject: [PATCH 3/3] test: cover releasing the client timeout when the response is a success The successful-response path is the one case the existing tests do not reach: the deadline has to outlive the call so the caller can read the body, and Close has to release it. The new test pins both halves against the request context the transport actually saw. --- github/github_test.go | 46 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/github/github_test.go b/github/github_test.go index be82055575b..aa7df85d4e7 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -4073,6 +4073,52 @@ func TestRoundTripWithOptionalFollowRedirect_HonorsClientTimeout(t *testing.T) { } } +// TestRoundTripWithOptionalFollowRedirect_ReleaseTimeoutOnClose pins the other +// half of the http.Client.Timeout contract: on a successful response the +// deadline has to outlive the call, because the caller still has a body to +// read, and it has to be released when that body is closed so the timer does +// not leak. +func TestRoundTripWithOptionalFollowRedirect_ReleaseTimeoutOnClose(t *testing.T) { + t.Parallel() + + var reqCtx context.Context + client, _, _ := setup(t) + client.client.Timeout = time.Hour + client.client.Transport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { + reqCtx = req.Context() + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader("payload")), + Request: req, + }, nil + }) + + resp, err := client.roundTripWithOptionalFollowRedirect(t.Context(), "archive", 0) + if err != nil { + t.Fatalf("roundTripWithOptionalFollowRedirect returned error: %v", err) + } + if reqCtx == nil { + t.Fatal("the transport never saw a request context") + } + if err := reqCtx.Err(); err != nil { + t.Fatalf("the request context ended before the body was closed: %v", err) + } + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("reading the response body returned error: %v", err) + } + if got, want := string(body), "payload"; got != want { + t.Errorf("got body %q, want %q", got, want) + } + if err := resp.Body.Close(); err != nil { + t.Fatalf("closing the response body returned error: %v", err) + } + if err := reqCtx.Err(); !errors.Is(err, context.Canceled) { + t.Errorf("the request context was not cancelled when the body was closed: err = %v", err) + } +} + // TestRoundTripWithOptionalFollowRedirect_TimeoutCoversAllRedirects pins the // http.Client.Timeout contract - the deadline spans the whole request, not one // hop - by checking that every transport call in a redirect chain shares one