From 02c3cdb19c420bdacf11933b53b8db5fda07d159 Mon Sep 17 00:00:00 2001 From: Sakurapainting Date: Thu, 2 Apr 2026 13:53:11 +0800 Subject: [PATCH] fix(channels): preserve startup errors and cover fail-fast semantics --- pkg/channels/manager.go | 44 ++++++++------ pkg/channels/manager_test.go | 112 ++++++++++++++++++++++++++++++++++- 2 files changed, 136 insertions(+), 20 deletions(-) diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index b9bcd98d3..239448a1c 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -13,7 +13,6 @@ import ( "math" "net/http" "sort" - "strings" "sync" "time" @@ -515,7 +514,8 @@ func (m *Manager) StartAll(ctx context.Context) error { dispatchCtx, cancel := context.WithCancel(ctx) m.dispatchTask = &asyncTask{cancel: cancel} - failedStarts := make(map[string]string) + failedStarts := make([]error, 0, len(m.channels)) + failedNames := make([]string, 0, len(m.channels)) for name, channel := range m.channels { logger.InfoCF("channels", "Starting channel", map[string]any{ @@ -526,8 +526,8 @@ func (m *Manager) StartAll(ctx context.Context) error { "channel": name, "error": err.Error(), }) - fmt.Printf("Failed to start channel %s: %v\n", name, err) - failedStarts[name] = err.Error() + failedStarts = append(failedStarts, fmt.Errorf("channel %s: %w", name, err)) + failedNames = append(failedNames, name) continue } // Lazily create worker only after channel starts successfully @@ -543,24 +543,28 @@ func (m *Manager) StartAll(ctx context.Context) error { m.dispatchTask = nil } - details := make([]string, 0, len(failedStarts)) - for name, reason := range failedStarts { - details = append(details, fmt.Sprintf("%s: %s", name, reason)) - } - sort.Strings(details) - if len(details) == 0 { + sort.Strings(failedNames) + if len(failedStarts) == 0 { return fmt.Errorf("failed to start any enabled channels") } - return fmt.Errorf("failed to start any enabled channels: %s", strings.Join(details, "; ")) + + logger.ErrorCF("channels", "All enabled channels failed to start", map[string]any{ + "failed": len(failedNames), + "total": len(m.channels), + "failed_channels": failedNames, + }) + + return fmt.Errorf("failed to start any enabled channels: %w", errors.Join(failedStarts...)) } - if len(failedStarts) > 0 { - failedNames := make([]string, 0, len(failedStarts)) - for name := range failedStarts { - failedNames = append(failedNames, name) - } + if len(failedNames) > 0 { sort.Strings(failedNames) - fmt.Printf("Warning: failed to start channels: %s\n", strings.Join(failedNames, ", ")) + logger.WarnCF("channels", "Some channels failed to start", map[string]any{ + "failed": len(failedNames), + "started": len(m.workers), + "total": len(m.channels), + "failed_channels": failedNames, + }) } // Start the dispatcher that reads from the bus and routes to workers @@ -584,7 +588,11 @@ func (m *Manager) StartAll(ctx context.Context) error { }() } - logger.InfoC("channels", "All channels started") + logger.InfoCF("channels", "Channel startup completed", map[string]any{ + "started": len(m.workers), + "failed": len(failedNames), + "total": len(m.channels), + }) return nil } diff --git a/pkg/channels/manager_test.go b/pkg/channels/manager_test.go index e76212905..937b32d2c 100644 --- a/pkg/channels/manager_test.go +++ b/pkg/channels/manager_test.go @@ -19,6 +19,8 @@ import ( type mockChannel struct { BaseChannel sendFn func(ctx context.Context, msg bus.OutboundMessage) error + startFn func(ctx context.Context) error + stopFn func(ctx context.Context) error sentMessages []bus.OutboundMessage placeholdersSent int editedMessages int @@ -33,8 +35,19 @@ func (m *mockChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]stri return nil, m.sendFn(ctx, msg) } -func (m *mockChannel) Start(ctx context.Context) error { return nil } -func (m *mockChannel) Stop(ctx context.Context) error { return nil } +func (m *mockChannel) Start(ctx context.Context) error { + if m.startFn != nil { + return m.startFn(ctx) + } + return nil +} + +func (m *mockChannel) Stop(ctx context.Context) error { + if m.stopFn != nil { + return m.stopFn(ctx) + } + return nil +} func (m *mockChannel) SendPlaceholder(ctx context.Context, chatID string) (string, error) { m.placeholdersSent++ @@ -86,6 +99,101 @@ func newTestManager() *Manager { return &Manager{ channels: make(map[string]Channel), workers: make(map[string]*channelWorker), + bus: bus.NewMessageBus(), + } +} + +func TestStartAll_AllChannelsFail_ReturnsJoinedError(t *testing.T) { + m := newTestManager() + errA := errors.New("channel-a start failed") + errB := errors.New("channel-b start failed") + + m.channels["a"] = &mockChannel{ + startFn: func(_ context.Context) error { return errA }, + } + m.channels["b"] = &mockChannel{ + startFn: func(_ context.Context) error { return errB }, + } + + err := m.StartAll(t.Context()) + if err == nil { + t.Fatal("expected StartAll to fail when all channels fail") + } + if !strings.Contains(err.Error(), "failed to start any enabled channels") { + t.Fatalf("unexpected error: %v", err) + } + if !errors.Is(err, errA) { + t.Fatalf("expected error to wrap errA, got: %v", err) + } + if !errors.Is(err, errB) { + t.Fatalf("expected error to wrap errB, got: %v", err) + } + if len(m.workers) != 0 { + t.Fatalf("expected no workers on full startup failure, got %d", len(m.workers)) + } + if m.dispatchTask != nil { + t.Fatal("expected dispatch task to be cleared on full startup failure") + } +} + +func TestStartAll_PartialFailure_StartsSuccessfulWorkers(t *testing.T) { + m := newTestManager() + errBad := errors.New("bad channel start failed") + processed := make(chan struct{}, 1) + + m.channels["good"] = &mockChannel{ + sendFn: func(_ context.Context, msg bus.OutboundMessage) error { + if msg.Channel == "good" { + select { + case processed <- struct{}{}: + default: + } + } + return nil + }, + } + m.channels["bad"] = &mockChannel{ + startFn: func(_ context.Context) error { return errBad }, + } + + err := m.StartAll(t.Context()) + if err != nil { + t.Fatalf("expected StartAll to succeed with partial channel failures, got: %v", err) + } + if len(m.workers) != 1 { + t.Fatalf("expected exactly 1 active worker, got %d", len(m.workers)) + } + if _, ok := m.workers["good"]; !ok { + t.Fatal("expected worker for successful channel 'good'") + } + if _, ok := m.workers["bad"]; ok { + t.Fatal("did not expect worker for failed channel 'bad'") + } + if m.dispatchTask == nil { + t.Fatal("expected dispatch task to run when at least one channel starts") + } + + pubCtx, pubCancel := context.WithTimeout(context.Background(), 2*time.Second) + defer pubCancel() + if err := m.bus.PublishOutbound(pubCtx, bus.OutboundMessage{ + Channel: "good", + ChatID: "chat-1", + Content: "hello", + }); err != nil { + t.Fatalf("PublishOutbound() error = %v", err) + } + + select { + case <-processed: + // worker processed outbound message as expected + case <-time.After(2 * time.Second): + t.Fatal("expected successful channel worker to process outbound message") + } + + stopCtx, stopCancel := context.WithTimeout(context.Background(), 2*time.Second) + defer stopCancel() + if err := m.StopAll(stopCtx); err != nil { + t.Fatalf("StopAll() error = %v", err) } }