fix: address PR #662 review comments (bus drain, context timeouts, onebot leak)
- Drain buffered messages in MessageBus.Close() so they aren't silently lost - Replace all context.TODO() with context.WithTimeout(5s) across 7 call sites - Fix OneBot pending channel leak: send nil sentinel in Stop() and handle nil response in sendAPIRequest() to unblock waiting goroutines
This commit is contained in:
parent
26bee0b791
commit
6852f24025
7 changed files with 65 additions and 8 deletions
|
|
@ -122,12 +122,13 @@ func registerSharedTools(
|
||||||
// Message tool
|
// Message tool
|
||||||
messageTool := tools.NewMessageTool()
|
messageTool := tools.NewMessageTool()
|
||||||
messageTool.SetSendCallback(func(channel, chatID, content string) error {
|
messageTool.SetSendCallback(func(channel, chatID, content string) error {
|
||||||
msgBus.PublishOutbound(context.TODO(), bus.OutboundMessage{
|
pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||||
|
defer pubCancel()
|
||||||
|
return msgBus.PublishOutbound(pubCtx, bus.OutboundMessage{
|
||||||
Channel: channel,
|
Channel: channel,
|
||||||
ChatID: chatID,
|
ChatID: chatID,
|
||||||
Content: content,
|
Content: content,
|
||||||
})
|
})
|
||||||
return nil
|
|
||||||
})
|
})
|
||||||
agent.Tools.Register(messageTool)
|
agent.Tools.Register(messageTool)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -4,6 +4,8 @@ import (
|
||||||
"context"
|
"context"
|
||||||
"errors"
|
"errors"
|
||||||
"sync/atomic"
|
"sync/atomic"
|
||||||
|
|
||||||
|
"github.com/sipeed/picoclaw/pkg/logger"
|
||||||
)
|
)
|
||||||
|
|
||||||
// ErrBusClosed is returned when publishing to a closed MessageBus.
|
// ErrBusClosed is returned when publishing to a closed MessageBus.
|
||||||
|
|
@ -104,5 +106,41 @@ func (mb *MessageBus) SubscribeOutboundMedia(ctx context.Context) (OutboundMedia
|
||||||
func (mb *MessageBus) Close() {
|
func (mb *MessageBus) Close() {
|
||||||
if mb.closed.CompareAndSwap(false, true) {
|
if mb.closed.CompareAndSwap(false, true) {
|
||||||
close(mb.done)
|
close(mb.done)
|
||||||
|
|
||||||
|
// Drain buffered channels so messages aren't silently lost.
|
||||||
|
// Channels are NOT closed to avoid send-on-closed panics from concurrent publishers.
|
||||||
|
drained := 0
|
||||||
|
for {
|
||||||
|
select {
|
||||||
|
case <-mb.inbound:
|
||||||
|
drained++
|
||||||
|
default:
|
||||||
|
goto doneInbound
|
||||||
|
}
|
||||||
|
}
|
||||||
|
doneInbound:
|
||||||
|
for {
|
||||||
|
select {
|
||||||
|
case <-mb.outbound:
|
||||||
|
drained++
|
||||||
|
default:
|
||||||
|
goto doneOutbound
|
||||||
|
}
|
||||||
|
}
|
||||||
|
doneOutbound:
|
||||||
|
for {
|
||||||
|
select {
|
||||||
|
case <-mb.outboundMedia:
|
||||||
|
drained++
|
||||||
|
default:
|
||||||
|
goto doneMedia
|
||||||
|
}
|
||||||
|
}
|
||||||
|
doneMedia:
|
||||||
|
if drained > 0 {
|
||||||
|
logger.DebugCF("bus", "Drained buffered messages during close", map[string]any{
|
||||||
|
"count": drained,
|
||||||
|
})
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -306,6 +306,9 @@ func (c *OneBotChannel) sendAPIRequest(action string, params any, timeout time.D
|
||||||
|
|
||||||
select {
|
select {
|
||||||
case resp := <-ch:
|
case resp := <-ch:
|
||||||
|
if resp == nil {
|
||||||
|
return nil, fmt.Errorf("API request %s: channel stopped", action)
|
||||||
|
}
|
||||||
return resp, nil
|
return resp, nil
|
||||||
case <-time.After(timeout):
|
case <-time.After(timeout):
|
||||||
return nil, fmt.Errorf("API request %s timed out after %v", action, timeout)
|
return nil, fmt.Errorf("API request %s timed out after %v", action, timeout)
|
||||||
|
|
@ -353,7 +356,11 @@ func (c *OneBotChannel) Stop(ctx context.Context) error {
|
||||||
}
|
}
|
||||||
|
|
||||||
c.pendingMu.Lock()
|
c.pendingMu.Lock()
|
||||||
for echo := range c.pending {
|
for echo, ch := range c.pending {
|
||||||
|
select {
|
||||||
|
case ch <- nil: // non-blocking wake for blocked sendAPIRequest goroutines
|
||||||
|
default:
|
||||||
|
}
|
||||||
delete(c.pending, echo)
|
delete(c.pending, echo)
|
||||||
}
|
}
|
||||||
c.pendingMu.Unlock()
|
c.pendingMu.Unlock()
|
||||||
|
|
|
||||||
|
|
@ -4,6 +4,7 @@ import (
|
||||||
"context"
|
"context"
|
||||||
"strings"
|
"strings"
|
||||||
"sync"
|
"sync"
|
||||||
|
"time"
|
||||||
|
|
||||||
"github.com/sipeed/picoclaw/pkg/bus"
|
"github.com/sipeed/picoclaw/pkg/bus"
|
||||||
"github.com/sipeed/picoclaw/pkg/constants"
|
"github.com/sipeed/picoclaw/pkg/constants"
|
||||||
|
|
@ -127,7 +128,9 @@ func (s *Service) sendNotification(ev *events.DeviceEvent) {
|
||||||
}
|
}
|
||||||
|
|
||||||
msg := ev.FormatMessage()
|
msg := ev.FormatMessage()
|
||||||
msgBus.PublishOutbound(context.TODO(), bus.OutboundMessage{
|
pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||||
|
defer pubCancel()
|
||||||
|
msgBus.PublishOutbound(pubCtx, bus.OutboundMessage{
|
||||||
Channel: platform,
|
Channel: platform,
|
||||||
ChatID: userID,
|
ChatID: userID,
|
||||||
Content: msg,
|
Content: msg,
|
||||||
|
|
|
||||||
|
|
@ -308,7 +308,9 @@ func (hs *HeartbeatService) sendResponse(response string) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
msgBus.PublishOutbound(context.TODO(), bus.OutboundMessage{
|
pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||||
|
defer pubCancel()
|
||||||
|
msgBus.PublishOutbound(pubCtx, bus.OutboundMessage{
|
||||||
Channel: platform,
|
Channel: platform,
|
||||||
ChatID: userID,
|
ChatID: userID,
|
||||||
Content: response,
|
Content: response,
|
||||||
|
|
|
||||||
|
|
@ -294,7 +294,9 @@ func (t *CronTool) ExecuteJob(ctx context.Context, job *cron.CronJob) string {
|
||||||
output = fmt.Sprintf("Scheduled command '%s' executed:\n%s", job.Payload.Command, result.ForLLM)
|
output = fmt.Sprintf("Scheduled command '%s' executed:\n%s", job.Payload.Command, result.ForLLM)
|
||||||
}
|
}
|
||||||
|
|
||||||
t.msgBus.PublishOutbound(context.TODO(), bus.OutboundMessage{
|
pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||||
|
defer pubCancel()
|
||||||
|
t.msgBus.PublishOutbound(pubCtx, bus.OutboundMessage{
|
||||||
Channel: channel,
|
Channel: channel,
|
||||||
ChatID: chatID,
|
ChatID: chatID,
|
||||||
Content: output,
|
Content: output,
|
||||||
|
|
@ -304,7 +306,9 @@ func (t *CronTool) ExecuteJob(ctx context.Context, job *cron.CronJob) string {
|
||||||
|
|
||||||
// If deliver=true, send message directly without agent processing
|
// If deliver=true, send message directly without agent processing
|
||||||
if job.Payload.Deliver {
|
if job.Payload.Deliver {
|
||||||
t.msgBus.PublishOutbound(context.TODO(), bus.OutboundMessage{
|
pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||||
|
defer pubCancel()
|
||||||
|
t.msgBus.PublishOutbound(pubCtx, bus.OutboundMessage{
|
||||||
Channel: channel,
|
Channel: channel,
|
||||||
ChatID: chatID,
|
ChatID: chatID,
|
||||||
Content: job.Payload.Message,
|
Content: job.Payload.Message,
|
||||||
|
|
|
||||||
|
|
@ -218,7 +218,9 @@ After completing the task, provide a clear summary of what was done.`
|
||||||
// Send announce message back to main agent
|
// Send announce message back to main agent
|
||||||
if sm.bus != nil {
|
if sm.bus != nil {
|
||||||
announceContent := fmt.Sprintf("Task '%s' completed.\n\nResult:\n%s", task.Label, task.Result)
|
announceContent := fmt.Sprintf("Task '%s' completed.\n\nResult:\n%s", task.Label, task.Result)
|
||||||
sm.bus.PublishInbound(context.TODO(), bus.InboundMessage{
|
pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||||
|
defer pubCancel()
|
||||||
|
sm.bus.PublishInbound(pubCtx, bus.InboundMessage{
|
||||||
Channel: "system",
|
Channel: "system",
|
||||||
SenderID: fmt.Sprintf("subagent:%s", task.ID),
|
SenderID: fmt.Sprintf("subagent:%s", task.ID),
|
||||||
// Format: "original_channel:original_chat_id" for routing back
|
// Format: "original_channel:original_chat_id" for routing back
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue