Skip to content

Commit 44a52c0

Browse files
authored
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.
1 parent b3c3b02 commit 44a52c0

File tree

8 files changed

+230
-79
lines changed

8 files changed

+230
-79
lines changed

pkg/agent/loop.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func registerSharedTools(
9999
}
100100

101101
// Web tools
102-
if searchTool := tools.NewWebSearchTool(tools.WebSearchToolOptions{
102+
searchTool, err := tools.NewWebSearchTool(tools.WebSearchToolOptions{
103103
BraveAPIKey: cfg.Tools.Web.Brave.APIKey,
104104
BraveMaxResults: cfg.Tools.Web.Brave.MaxResults,
105105
BraveEnabled: cfg.Tools.Web.Brave.Enabled,
@@ -113,10 +113,18 @@ func registerSharedTools(
113113
PerplexityMaxResults: cfg.Tools.Web.Perplexity.MaxResults,
114114
PerplexityEnabled: cfg.Tools.Web.Perplexity.Enabled,
115115
Proxy: cfg.Tools.Web.Proxy,
116-
}); searchTool != nil {
116+
})
117+
if err != nil {
118+
logger.ErrorCF("agent", "Failed to create web search tool", map[string]any{"error": err.Error()})
119+
} else if searchTool != nil {
117120
agent.Tools.Register(searchTool)
118121
}
119-
agent.Tools.Register(tools.NewWebFetchToolWithProxy(50000, cfg.Tools.Web.Proxy))
122+
fetchTool, err := tools.NewWebFetchToolWithProxy(50000, cfg.Tools.Web.Proxy)
123+
if err != nil {
124+
logger.ErrorCF("agent", "Failed to create web fetch tool", map[string]any{"error": err.Error()})
125+
} else {
126+
agent.Tools.Register(fetchTool)
127+
}
120128

121129
// Hardware tools (I2C, SPI) - Linux only, returns error on other platforms
122130
agent.Tools.Register(tools.NewI2CTool())

pkg/channels/line/line.go

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,13 @@ type replyTokenEntry struct {
4545
type LINEChannel struct {
4646
*channels.BaseChannel
4747
config config.LINEConfig
48-
botUserID string // Bot's user ID
49-
botBasicID string // Bot's basic ID (e.g. @216ru...)
50-
botDisplayName string // Bot's display name for text-based mention detection
51-
replyTokens sync.Map // chatID -> replyTokenEntry
52-
quoteTokens sync.Map // chatID -> quoteToken (string)
48+
infoClient *http.Client // for bot info lookups (short timeout)
49+
apiClient *http.Client // for messaging API calls
50+
botUserID string // Bot's user ID
51+
botBasicID string // Bot's basic ID (e.g. @216ru...)
52+
botDisplayName string // Bot's display name for text-based mention detection
53+
replyTokens sync.Map // chatID -> replyTokenEntry
54+
quoteTokens sync.Map // chatID -> quoteToken (string)
5355
ctx context.Context
5456
cancel context.CancelFunc
5557
}
@@ -69,6 +71,8 @@ func NewLINEChannel(cfg config.LINEConfig, messageBus *bus.MessageBus) (*LINECha
6971
return &LINEChannel{
7072
BaseChannel: base,
7173
config: cfg,
74+
infoClient: &http.Client{Timeout: 10 * time.Second},
75+
apiClient: &http.Client{Timeout: 30 * time.Second},
7276
}, nil
7377
}
7478

@@ -104,8 +108,7 @@ func (c *LINEChannel) fetchBotInfo() error {
104108
}
105109
req.Header.Set("Authorization", "Bearer "+c.config.ChannelAccessToken)
106110

107-
client := &http.Client{Timeout: 10 * time.Second}
108-
resp, err := client.Do(req)
111+
resp, err := c.infoClient.Do(req)
109112
if err != nil {
110113
return err
111114
}
@@ -644,8 +647,7 @@ func (c *LINEChannel) callAPI(ctx context.Context, endpoint string, payload any)
644647
req.Header.Set("Content-Type", "application/json")
645648
req.Header.Set("Authorization", "Bearer "+c.config.ChannelAccessToken)
646649

647-
client := &http.Client{Timeout: 30 * time.Second}
648-
resp, err := client.Do(req)
650+
resp, err := c.apiClient.Do(req)
649651
if err != nil {
650652
return channels.ClassifyNetError(err)
651653
}

pkg/channels/wecom/app.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ const (
3232
type WeComAppChannel struct {
3333
*channels.BaseChannel
3434
config config.WeComAppConfig
35+
client *http.Client
3536
accessToken string
3637
tokenExpiry time.Time
3738
tokenMu sync.RWMutex
@@ -129,10 +130,18 @@ func NewWeComAppChannel(cfg config.WeComAppConfig, messageBus *bus.MessageBus) (
129130
channels.WithReasoningChannelID(cfg.ReasoningChannelID),
130131
)
131132

133+
// Client timeout must be >= the configured ReplyTimeout so the
134+
// per-request context deadline is always the effective limit.
135+
clientTimeout := 30 * time.Second
136+
if d := time.Duration(cfg.ReplyTimeout) * time.Second; d > clientTimeout {
137+
clientTimeout = d
138+
}
139+
132140
ctx, cancel := context.WithCancel(context.Background())
133141
return &WeComAppChannel{
134142
BaseChannel: base,
135143
config: cfg,
144+
client: &http.Client{Timeout: clientTimeout},
136145
ctx: ctx,
137146
cancel: cancel,
138147
processedMsgs: make(map[string]bool),
@@ -306,8 +315,7 @@ func (c *WeComAppChannel) uploadMedia(ctx context.Context, accessToken, mediaTyp
306315
}
307316
req.Header.Set("Content-Type", writer.FormDataContentType())
308317

309-
client := &http.Client{Timeout: 30 * time.Second}
310-
resp, err := client.Do(req)
318+
resp, err := c.client.Do(req)
311319
if err != nil {
312320
return "", channels.ClassifyNetError(err)
313321
}
@@ -364,8 +372,7 @@ func (c *WeComAppChannel) sendImageMessage(ctx context.Context, accessToken, use
364372
}
365373
req.Header.Set("Content-Type", "application/json")
366374

367-
client := &http.Client{Timeout: time.Duration(timeout) * time.Second}
368-
resp, err := client.Do(req)
375+
resp, err := c.client.Do(req)
369376
if err != nil {
370377
return channels.ClassifyNetError(err)
371378
}
@@ -746,8 +753,7 @@ func (c *WeComAppChannel) sendTextMessage(ctx context.Context, accessToken, user
746753
}
747754
req.Header.Set("Content-Type", "application/json")
748755

749-
client := &http.Client{Timeout: time.Duration(timeout) * time.Second}
750-
resp, err := client.Do(req)
756+
resp, err := c.client.Do(req)
751757
if err != nil {
752758
return channels.ClassifyNetError(err)
753759
}

pkg/channels/wecom/bot.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
type WeComBotChannel struct {
2626
*channels.BaseChannel
2727
config config.WeComConfig
28+
client *http.Client
2829
ctx context.Context
2930
cancel context.CancelFunc
3031
processedMsgs map[string]bool // Message deduplication: msg_id -> processed
@@ -93,10 +94,18 @@ func NewWeComBotChannel(cfg config.WeComConfig, messageBus *bus.MessageBus) (*We
9394
channels.WithReasoningChannelID(cfg.ReasoningChannelID),
9495
)
9596

97+
// Client timeout must be >= the configured ReplyTimeout so the
98+
// per-request context deadline is always the effective limit.
99+
clientTimeout := 30 * time.Second
100+
if d := time.Duration(cfg.ReplyTimeout) * time.Second; d > clientTimeout {
101+
clientTimeout = d
102+
}
103+
96104
ctx, cancel := context.WithCancel(context.Background())
97105
return &WeComBotChannel{
98106
BaseChannel: base,
99107
config: cfg,
108+
client: &http.Client{Timeout: clientTimeout},
100109
ctx: ctx,
101110
cancel: cancel,
102111
processedMsgs: make(map[string]bool),
@@ -450,8 +459,7 @@ func (c *WeComBotChannel) sendWebhookReply(ctx context.Context, userID, content
450459
}
451460
req.Header.Set("Content-Type", "application/json")
452461

453-
client := &http.Client{Timeout: time.Duration(timeout) * time.Second}
454-
resp, err := client.Do(req)
462+
resp, err := c.client.Do(req)
455463
if err != nil {
456464
return channels.ClassifyNetError(err)
457465
}

0 commit comments

Comments
 (0)