From f8f5e6855523fdc06c28c97e6c28541b0025110c Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Fri, 6 Jun 2025 03:53:03 +0000 Subject: [PATCH 1/3] fix: remove closed check when starting notification manager --- coderd/notifications/manager.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/coderd/notifications/manager.go b/coderd/notifications/manager.go index 1a2c418a014bb..dff4c39e44b80 100644 --- a/coderd/notifications/manager.go +++ b/coderd/notifications/manager.go @@ -53,7 +53,6 @@ type Manager struct { success, failure chan dispatchResult mu sync.Mutex // Protects following. - closed bool notifier *notifier runOnce sync.Once @@ -161,11 +160,6 @@ func (m *Manager) loop(ctx context.Context) error { }() m.mu.Lock() - if m.closed { - m.mu.Unlock() - return xerrors.New("manager already closed") - } - var eg errgroup.Group m.notifier = newNotifier(ctx, m.cfg, uuid.New(), m.log, m.store, m.handlers, m.helpers, m.metrics, m.clock) @@ -354,12 +348,13 @@ func (m *Manager) Stop(ctx context.Context) error { m.mu.Lock() defer m.mu.Unlock() - if m.closed { + m.log.Debug(context.Background(), "graceful stop requested") + + select { + case <-m.stop: return nil + default: } - m.closed = true - - m.log.Debug(context.Background(), "graceful stop requested") // If the notifier hasn't been started, we don't need to wait for anything. // This is only really during testing when we want to enqueue messages only but not deliver them. From 961777652a3de641bc1ea45ba34b4c652f550e1b Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Fri, 6 Jun 2025 05:42:24 +0000 Subject: [PATCH 2/3] reorder log --- coderd/notifications/manager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/notifications/manager.go b/coderd/notifications/manager.go index dff4c39e44b80..5378d2c6c897c 100644 --- a/coderd/notifications/manager.go +++ b/coderd/notifications/manager.go @@ -348,14 +348,14 @@ func (m *Manager) Stop(ctx context.Context) error { m.mu.Lock() defer m.mu.Unlock() - m.log.Debug(context.Background(), "graceful stop requested") - select { case <-m.stop: return nil default: } + m.log.Debug(context.Background(), "graceful stop requested") + // If the notifier hasn't been started, we don't need to wait for anything. // This is only really during testing when we want to enqueue messages only but not deliver them. if m.notifier != nil { From 116e9c2427e44d1e2c5d9bbdffac10ddea8ac615 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Fri, 6 Jun 2025 08:46:58 +0000 Subject: [PATCH 3/3] silence manager already closed error logs --- coderd/notifications/manager.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/coderd/notifications/manager.go b/coderd/notifications/manager.go index 5378d2c6c897c..11588a09fb797 100644 --- a/coderd/notifications/manager.go +++ b/coderd/notifications/manager.go @@ -53,6 +53,7 @@ type Manager struct { success, failure chan dispatchResult mu sync.Mutex // Protects following. + closed bool notifier *notifier runOnce sync.Once @@ -134,6 +135,8 @@ func (m *Manager) WithHandlers(reg map[database.NotificationMethod]Handler) { m.handlers = reg } +var ErrManagerAlreadyClosed = xerrors.New("manager already closed") + // Run initiates the control loop in the background, which spawns a given number of notifier goroutines. // Manager requires system-level permissions to interact with the store. // Run is only intended to be run once. @@ -145,7 +148,11 @@ func (m *Manager) Run(ctx context.Context) { go func() { err := m.loop(ctx) if err != nil { - m.log.Error(ctx, "notification manager stopped with error", slog.Error(err)) + if xerrors.Is(err, ErrManagerAlreadyClosed) { + m.log.Warn(ctx, "notification manager stopped with error", slog.Error(err)) + } else { + m.log.Error(ctx, "notification manager stopped with error", slog.Error(err)) + } } }() }) @@ -160,6 +167,11 @@ func (m *Manager) loop(ctx context.Context) error { }() m.mu.Lock() + if m.closed { + m.mu.Unlock() + return ErrManagerAlreadyClosed + } + var eg errgroup.Group m.notifier = newNotifier(ctx, m.cfg, uuid.New(), m.log, m.store, m.handlers, m.helpers, m.metrics, m.clock) @@ -348,11 +360,10 @@ func (m *Manager) Stop(ctx context.Context) error { m.mu.Lock() defer m.mu.Unlock() - select { - case <-m.stop: + if m.closed { return nil - default: } + m.closed = true m.log.Debug(context.Background(), "graceful stop requested")