Files
picoclaw/pkg/utils/http_retry_test.go
T
Tong Niu 44a52c0cf6 fix(tools): close resp.Body on retry cancel and cache http.Client instances (#940)
* fix(tools): close resp.Body on retry cancel and cache http.Client instances

Fix resp.Body leak in DoRequestWithRetry where req.Body (request) was
incorrectly closed instead of resp.Body (response) on context cancel.
Cache http.Client on web search/fetch provider structs and channel
adapters (WeCom, LINE) to avoid per-call allocation overhead.

* fix(channels): preserve original http client timeouts for LINE and WeCom

Split LINE single 60s client into infoClient (10s) for bot info lookups
and apiClient (30s) for messaging API calls. Lower WeCom cached client
base timeout from 60s to 30s (matching uploadMedia), and ensure it is
always >= the configured ReplyTimeout so the per-request context
deadline remains the effective limit.

* refactor(tools): extract timeout consts and deduplicate WebFetchTool constructors

Address PR review feedback from xiaket:
- Define searchTimeout, perplexityTimeout, fetchTimeout, defaultMaxChars,
  and maxRedirects as package-level consts instead of magic numbers.
- Remove misleading "No proxy" comment in NewWebFetchTool.
- Deduplicate NewWebFetchTool by delegating to NewWebFetchToolWithProxy.

* test(utils): add context cancellation test for DoRequestWithRetry

Verify that resp.Body is properly closed when the context is canceled
during retry sleep, covering the C8 resp.Body leak fix.

* fix(utils): close resp in test to satisfy bodyclose linter

* fix(utils): eliminate flakiness in context cancellation retry test

Synchronize cancellation using an onRoundTrip callback from the
transport wrapper instead of a timing-based context timeout. This
ensures the first client.Do completes before cancel fires, so
cancellation always hits during sleepWithCtx.
2026-03-01 16:55:46 +11:00

207 lines
5.2 KiB
Go

package utils
import (
"context"
"io"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestDoRequestWithRetry(t *testing.T) {
retryDelayUnit = time.Millisecond
t.Cleanup(func() { retryDelayUnit = time.Second })
testcases := []struct {
name string
serverBehavior func(*httptest.Server) int
wantSuccess bool
wantAttempts int
}{
{
name: "success-on-first-attempt",
serverBehavior: func(server *httptest.Server) int {
return 0
},
wantSuccess: true,
wantAttempts: 1,
},
{
name: "fail-all-attempts",
serverBehavior: func(server *httptest.Server) int {
return 4
},
wantSuccess: false,
wantAttempts: 3,
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
attempts := 0
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
if attempts <= tc.serverBehavior(nil) {
w.WriteHeader(http.StatusInternalServerError)
return
}
w.WriteHeader(http.StatusOK)
w.Write([]byte("success"))
}))
t.Cleanup(func() {
server.Close()
})
client := &http.Client{Timeout: 5 * time.Second}
req, err := http.NewRequest(http.MethodGet, server.URL, nil)
require.NoError(t, err)
resp, err := DoRequestWithRetry(client, req)
if tc.wantSuccess {
require.NoError(t, err)
require.NotNil(t, resp)
assert.Equal(t, http.StatusOK, resp.StatusCode)
resp.Body.Close()
} else {
require.NotNil(t, resp)
assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
resp.Body.Close()
}
assert.Equal(t, tc.wantAttempts, attempts)
})
}
}
func TestDoRequestWithRetry_ContextCancel(t *testing.T) {
// Use a long retry delay so cancellation always hits during sleepWithCtx.
retryDelayUnit = 10 * time.Second
t.Cleanup(func() { retryDelayUnit = time.Second })
bodyClosed := false
firstRoundTripDone := make(chan struct{}, 1)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte("error"))
}))
defer server.Close()
client := server.Client()
client.Timeout = 30 * time.Second
client.Transport = &bodyCloseTracker{
rt: client.Transport,
onClose: func() { bodyClosed = true },
// Signal after the first round-trip response is fully constructed on the client side.
onRoundTrip: func() {
select {
case firstRoundTripDone <- struct{}{}:
default:
}
},
trackURL: server.URL,
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
// Cancel the context after the first round-trip completes on the client side.
// This ensures client.Do has returned a valid resp (with body) and the retry
// loop is about to enter sleepWithCtx, where the cancel will be detected.
go func() {
<-firstRoundTripDone
cancel()
}()
req, err := http.NewRequestWithContext(ctx, http.MethodGet, server.URL, nil)
require.NoError(t, err)
resp, err := DoRequestWithRetry(client, req)
if resp != nil {
resp.Body.Close()
}
require.Error(t, err, "expected error from context cancellation")
assert.Nil(t, resp, "expected nil response when context is canceled")
assert.True(t, bodyClosed, "expected resp.Body to be closed on context cancellation")
}
// bodyCloseTracker wraps an http.RoundTripper and records when response bodies are closed.
type bodyCloseTracker struct {
rt http.RoundTripper
onClose func()
onRoundTrip func() // called after each successful round-trip
trackURL string
}
func (t *bodyCloseTracker) RoundTrip(req *http.Request) (*http.Response, error) {
resp, err := t.rt.RoundTrip(req)
if err != nil {
return resp, err
}
if strings.HasPrefix(req.URL.String(), t.trackURL) {
resp.Body = &closeNotifier{ReadCloser: resp.Body, onClose: t.onClose}
if t.onRoundTrip != nil {
t.onRoundTrip()
}
}
return resp, nil
}
// closeNotifier wraps an io.ReadCloser to detect Close calls.
type closeNotifier struct {
io.ReadCloser
onClose func()
}
func (c *closeNotifier) Close() error {
c.onClose()
return c.ReadCloser.Close()
}
func TestDoRequestWithRetry_Delay(t *testing.T) {
retryDelayUnit = time.Millisecond
t.Cleanup(func() { retryDelayUnit = time.Second })
var start time.Time
delays := []time.Duration{}
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if len(delays) == 0 {
delays = append(delays, 0)
w.WriteHeader(http.StatusInternalServerError)
return
}
if len(delays) == 1 {
start = time.Now()
delays = append(delays, 0)
w.WriteHeader(http.StatusInternalServerError)
return
}
if len(delays) == 2 {
elapsed := time.Since(start)
delays = append(delays, elapsed)
w.WriteHeader(http.StatusOK)
w.Write([]byte("success"))
}
}))
defer server.Close()
client := &http.Client{Timeout: 10 * time.Second}
req, err := http.NewRequest(http.MethodGet, server.URL, nil)
require.NoError(t, err)
resp, err := DoRequestWithRetry(client, req)
require.NoError(t, err)
require.NotNil(t, resp)
assert.Equal(t, http.StatusOK, resp.StatusCode)
resp.Body.Close()
assert.GreaterOrEqual(t, delays[2], time.Millisecond)
}