fix: Honor http.Client.Timeout in roundTripWithOptionalFollowRedirect

github/github.go

@@ -2282,8 +2282,58 @@ 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) {
-	req, err := c.NewRequest(ctx, "GET", u, nil, opts...)
+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
+	// 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)
+		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)
+}
+
+// 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) (*http.Response, error) {
+	req, err := c.NewRequest(ctx, "GET", u, nil)
 	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)
 	}
 	return resp, err
 }

github/github_test.go

@@ -4033,6 +4033,146 @@ 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_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
+// 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).