fix(channels): address review feedback on QQ channel implementation

- Fix goroutine leak: reinitialize done channel and sync.Once in Start()
  to prevent multiple janitor goroutines on restart
- Fix double-close panic: guard close(done) with sync.Once in Stop()
- Fix StartTyping context: use c.ctx (channel lifecycle) instead of
  caller's ctx (request lifecycle) for typing goroutine
- Refactor: extract getChatKind() helper to deduplicate chatType lookup
  across Send(), StartTyping(), and SendMedia()
- Fix: use new(atomic.Uint64) instead of taking address of local var
- Fix: require explicit http(s):// scheme in URL regex to avoid false
  positives on version strings like "1.2.3"
- Optimize: collect expired keys before deleting in dedupJanitor to
  reduce lock hold time
- Fix: remove MaxMessageLength zero-value override in NewQQChannel
  since defaults.go already sets 2000
This commit is contained in:
Hoshina 2026-03-07 20:37:41 +08:00
parent f2e3945b46
commit bd9a932d73

View file

@ -53,17 +53,13 @@ type QQChannel struct {
muDedup sync.Mutex muDedup sync.Mutex
// done is closed on Stop to shut down the dedup janitor. // done is closed on Stop to shut down the dedup janitor.
done chan struct{} done chan struct{}
stopOnce sync.Once
} }
func NewQQChannel(cfg config.QQConfig, messageBus *bus.MessageBus) (*QQChannel, error) { func NewQQChannel(cfg config.QQConfig, messageBus *bus.MessageBus) (*QQChannel, error) {
maxLen := cfg.MaxMessageLength
if maxLen == 0 {
maxLen = 2000
}
base := channels.NewBaseChannel("qq", cfg, messageBus, cfg.AllowFrom, base := channels.NewBaseChannel("qq", cfg, messageBus, cfg.AllowFrom,
channels.WithMaxMessageLength(maxLen), channels.WithMaxMessageLength(cfg.MaxMessageLength),
channels.WithGroupTrigger(cfg.GroupTrigger), channels.WithGroupTrigger(cfg.GroupTrigger),
channels.WithReasoningChannelID(cfg.ReasoningChannelID), channels.WithReasoningChannelID(cfg.ReasoningChannelID),
) )
@ -83,6 +79,10 @@ func (c *QQChannel) Start(ctx context.Context) error {
logger.InfoC("qq", "Starting QQ bot (WebSocket mode)") logger.InfoC("qq", "Starting QQ bot (WebSocket mode)")
// Reinitialize shutdown signal for clean restart.
c.done = make(chan struct{})
c.stopOnce = sync.Once{}
// create token source // create token source
credentials := &token.QQBotCredentials{ credentials := &token.QQBotCredentials{
AppID: c.config.AppID, AppID: c.config.AppID,
@ -143,8 +143,8 @@ func (c *QQChannel) Stop(ctx context.Context) error {
logger.InfoC("qq", "Stopping QQ bot") logger.InfoC("qq", "Stopping QQ bot")
c.SetRunning(false) c.SetRunning(false)
// Signal the dedup janitor to stop. // Signal the dedup janitor to stop (idempotent).
close(c.done) c.stopOnce.Do(func() { close(c.done) })
if c.cancel != nil { if c.cancel != nil {
c.cancel() c.cancel()
@ -153,18 +153,22 @@ func (c *QQChannel) Stop(ctx context.Context) error {
return nil return nil
} }
// getChatKind returns the chat type for a given chatID ("group" or "direct").
func (c *QQChannel) getChatKind(chatID string) string {
if v, ok := c.chatType.Load(chatID); ok {
if k, ok := v.(string); ok {
return k
}
}
return "direct"
}
func (c *QQChannel) Send(ctx context.Context, msg bus.OutboundMessage) error { func (c *QQChannel) Send(ctx context.Context, msg bus.OutboundMessage) error {
if !c.IsRunning() { if !c.IsRunning() {
return channels.ErrNotRunning return channels.ErrNotRunning
} }
// Determine chat type (fallback to "direct" if not tracked). chatKind := c.getChatKind(msg.ChatID)
chatKind := "direct"
if v, ok := c.chatType.Load(msg.ChatID); ok {
if k, ok := v.(string); ok {
chatKind = k
}
}
// Build message with content. // Build message with content.
msgToCreate := &dto.MessageToCreate{ msgToCreate := &dto.MessageToCreate{
@ -241,12 +245,7 @@ func (c *QQChannel) StartTyping(ctx context.Context, chatID string) (func(), err
return func() {}, nil return func() {}, nil
} }
chatKind := "direct" chatKind := c.getChatKind(chatID)
if kv, ok := c.chatType.Load(chatID); ok {
if k, ok := kv.(string); ok {
chatKind = k
}
}
sendTyping := func(sendCtx context.Context) { sendTyping := func(sendCtx context.Context) {
typingMsg := &dto.MessageToCreate{ typingMsg := &dto.MessageToCreate{
@ -273,9 +272,9 @@ func (c *QQChannel) StartTyping(ctx context.Context, chatID string) (func(), err
} }
// Send immediately. // Send immediately.
sendTyping(ctx) sendTyping(c.ctx)
typingCtx, cancel := context.WithCancel(ctx) typingCtx, cancel := context.WithCancel(c.ctx)
go func() { go func() {
ticker := time.NewTicker(typingResend) ticker := time.NewTicker(typingResend)
defer ticker.Stop() defer ticker.Stop()
@ -301,12 +300,7 @@ func (c *QQChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMessage)
return channels.ErrNotRunning return channels.ErrNotRunning
} }
chatKind := "direct" chatKind := c.getChatKind(msg.ChatID)
if v, ok := c.chatType.Load(msg.ChatID); ok {
if k, ok := v.(string); ok {
chatKind = k
}
}
store := c.GetMediaStore() store := c.GetMediaStore()
if store == nil { if store == nil {
@ -406,8 +400,7 @@ func (c *QQChannel) handleC2CMessage() event.C2CMessageEventHandler {
c.lastMsgID.Store(senderID, data.ID) c.lastMsgID.Store(senderID, data.ID)
// Reset msg_seq counter for new inbound message. // Reset msg_seq counter for new inbound message.
var counter atomic.Uint64 c.msgSeqCounters.Store(senderID, new(atomic.Uint64))
c.msgSeqCounters.Store(senderID, &counter)
metadata := map[string]string{} metadata := map[string]string{}
@ -478,8 +471,7 @@ func (c *QQChannel) handleGroupATMessage() event.GroupATMessageEventHandler {
c.lastMsgID.Store(data.GroupID, data.ID) c.lastMsgID.Store(data.GroupID, data.ID)
// Reset msg_seq counter for new inbound message. // Reset msg_seq counter for new inbound message.
var counter atomic.Uint64 c.msgSeqCounters.Store(data.GroupID, new(atomic.Uint64))
c.msgSeqCounters.Store(data.GroupID, &counter)
metadata := map[string]string{ metadata := map[string]string{
"group_id": data.GroupID, "group_id": data.GroupID,
@ -533,13 +525,18 @@ func (c *QQChannel) dedupJanitor() {
case <-c.done: case <-c.done:
return return
case <-ticker.C: case <-ticker.C:
// Collect expired keys under read-like scan.
c.muDedup.Lock() c.muDedup.Lock()
now := time.Now() now := time.Now()
var expired []string
for id, ts := range c.dedup { for id, ts := range c.dedup {
if now.Sub(ts) >= dedupTTL { if now.Sub(ts) >= dedupTTL {
delete(c.dedup, id) expired = append(expired, id)
} }
} }
for _, id := range expired {
delete(c.dedup, id)
}
c.muDedup.Unlock() c.muDedup.Unlock()
} }
} }
@ -550,10 +547,12 @@ func isHTTPURL(s string) bool {
return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://") return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://")
} }
// urlPattern matches URLs like http(s)://domain.tld/path and bare domain.tld/path patterns. // urlPattern matches URLs with explicit http(s):// scheme.
// Only scheme-prefixed URLs are matched to avoid false positives on bare text
// like version numbers (e.g., "1.2.3") or domain-like fragments.
var urlPattern = regexp.MustCompile( var urlPattern = regexp.MustCompile(
`(?i)` + `(?i)` +
`(?:https?://)?` + // optional scheme `https?://` + // required scheme
`(?:[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+` + // domain parts `(?:[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+` + // domain parts
`[a-zA-Z]{2,}` + // TLD `[a-zA-Z]{2,}` + // TLD
`(?:[/?#]\S*)?`, // optional path/query/fragment `(?:[/?#]\S*)?`, // optional path/query/fragment
@ -563,14 +562,10 @@ var urlPattern = regexp.MustCompile(
// to prevent QQ's URL blacklist from rejecting the message. // to prevent QQ's URL blacklist from rejecting the message.
func sanitizeURLs(text string) string { func sanitizeURLs(text string) string {
return urlPattern.ReplaceAllStringFunc(text, func(match string) string { return urlPattern.ReplaceAllStringFunc(text, func(match string) string {
// Split into scheme + rest. // Split into scheme + rest (scheme is always present).
var scheme, rest string idx := strings.Index(match, "://")
if idx := strings.Index(match, "://"); idx != -1 { scheme := match[:idx+3]
scheme = match[:idx+3] rest := match[idx+3:]
rest = match[idx+3:]
} else {
rest = match
}
// Find where the domain ends (first / ? or #). // Find where the domain ends (first / ? or #).
domainEnd := len(rest) domainEnd := len(rest)