fix: clear golangci-lint backlog and make lint job blocking
Addresses errcheck, gosec, revive, staticcheck, and unused findings
across the codebase (unchecked error returns, unsafe file inclusion
warnings on operator/test-controlled paths, missing package comments,
unused parameters, deprecated API usage). Also fixes two suppression
comments that were silently no-ops due to wrong syntax (#nosec needs
a leading '#', nolint reasons need '//' not '--').
With the backlog clear, drop continue-on-error from the CI lint job
per the plan left in b4b4806.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
+24
-10
@@ -1,3 +1,7 @@
|
||||
// Package service implements the core notification service: queueing,
|
||||
// worker-pool delivery with retry/backoff, in-memory notification tracking,
|
||||
// retention cleanup, and multi-tenant access control on top of the
|
||||
// domain and auth packages.
|
||||
package service
|
||||
|
||||
import (
|
||||
@@ -166,14 +170,12 @@ func (s *NotificationService) performCleanup() {
|
||||
|
||||
// Track which notifications to delete
|
||||
var toDelete []string
|
||||
var allNotifications []*domain.Notification
|
||||
|
||||
// First pass: identify expired notifications and collect all for sorting
|
||||
// First pass: identify expired notifications
|
||||
for id, notification := range s.notifications {
|
||||
if notification.CreatedAt.Before(expiredBefore) {
|
||||
toDelete = append(toDelete, id)
|
||||
}
|
||||
allNotifications = append(allNotifications, notification)
|
||||
}
|
||||
|
||||
// Delete expired notifications
|
||||
@@ -218,7 +220,7 @@ func (s *NotificationService) performCleanup() {
|
||||
}
|
||||
|
||||
// worker processes notifications from the queue
|
||||
func (s *NotificationService) worker(ctx context.Context, id int) {
|
||||
func (s *NotificationService) worker(ctx context.Context, _ int) {
|
||||
defer s.wg.Done()
|
||||
|
||||
for {
|
||||
@@ -284,7 +286,9 @@ func (s *NotificationService) processNotification(ctx context.Context, msg *doma
|
||||
notification.ID, notification.Type, account, err)
|
||||
notification.Status = domain.StatusFailed
|
||||
notification.LastError = fmt.Sprintf("failed to create notifier: %v", err)
|
||||
s.queue.Nack(ctx, msg.ID, false)
|
||||
if nackErr := s.queue.Nack(ctx, msg.ID, false); nackErr != nil {
|
||||
s.logger.Warnf("failed to nack message id=%s: %v", msg.ID, nackErr)
|
||||
}
|
||||
s.updateNotification(notification)
|
||||
return
|
||||
}
|
||||
@@ -313,13 +317,17 @@ func (s *NotificationService) processNotification(ctx context.Context, msg *doma
|
||||
notification.Status = domain.StatusFailed
|
||||
s.logger.Errorf("Notification send failed permanently - id=%s, type=%s, account=%s, recipients=%v, attempts=%d, error=%s",
|
||||
notification.ID, notification.Type, account, notification.Recipients, notification.RetryCount, notification.LastError)
|
||||
s.queue.Nack(ctx, msg.ID, false) // Don't requeue
|
||||
if nackErr := s.queue.Nack(ctx, msg.ID, false); nackErr != nil { // Don't requeue
|
||||
s.logger.Warnf("failed to nack message id=%s: %v", msg.ID, nackErr)
|
||||
}
|
||||
}
|
||||
} else {
|
||||
notification.Status = domain.StatusSent
|
||||
now := time.Now()
|
||||
notification.SentAt = &now
|
||||
s.queue.Ack(ctx, msg.ID)
|
||||
if ackErr := s.queue.Ack(ctx, msg.ID); ackErr != nil {
|
||||
s.logger.Warnf("failed to ack message id=%s: %v", msg.ID, ackErr)
|
||||
}
|
||||
s.logger.Infof("Notification sent successfully - id=%s, type=%s, account=%s, recipients=%v",
|
||||
notification.ID, notification.Type, account, notification.Recipients)
|
||||
}
|
||||
@@ -335,7 +343,9 @@ func (s *NotificationService) processNotification(ctx context.Context, msg *doma
|
||||
func (s *NotificationService) scheduleRetry(ctx context.Context, msg *domain.QueueMessage, notification *domain.Notification) {
|
||||
delay := s.retryDelay(notification.RetryCount)
|
||||
if delay <= 0 {
|
||||
s.queue.Nack(ctx, msg.ID, true) // Requeue immediately
|
||||
if err := s.queue.Nack(ctx, msg.ID, true); err != nil { // Requeue immediately
|
||||
s.logger.Warnf("failed to nack message id=%s: %v", msg.ID, err)
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
@@ -348,7 +358,9 @@ func (s *NotificationService) scheduleRetry(ctx context.Context, msg *domain.Que
|
||||
|
||||
select {
|
||||
case <-timer.C:
|
||||
s.queue.Nack(ctx, msg.ID, true) // Requeue after backoff
|
||||
if err := s.queue.Nack(ctx, msg.ID, true); err != nil { // Requeue after backoff
|
||||
s.logger.Warnf("failed to nack message id=%s: %v", msg.ID, err)
|
||||
}
|
||||
case <-ctx.Done():
|
||||
s.abandonRetry(msg, notification)
|
||||
case <-s.stopChan:
|
||||
@@ -362,7 +374,9 @@ func (s *NotificationService) scheduleRetry(ctx context.Context, msg *domain.Que
|
||||
// retry is still pending, so the notification isn't left stuck in "retrying"
|
||||
// forever and no goroutine lingers past shutdown.
|
||||
func (s *NotificationService) abandonRetry(msg *domain.QueueMessage, notification *domain.Notification) {
|
||||
s.queue.Nack(context.Background(), msg.ID, false) // Don't requeue
|
||||
if err := s.queue.Nack(context.Background(), msg.ID, false); err != nil { // Don't requeue
|
||||
s.logger.Warnf("failed to nack message id=%s: %v", msg.ID, err)
|
||||
}
|
||||
notification.Status = domain.StatusFailed
|
||||
s.updateNotification(notification)
|
||||
}
|
||||
|
||||
@@ -26,7 +26,7 @@ func TestConcurrentSendGetListNoRace(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
const numSenders = 8
|
||||
const sendsPerSender = 25
|
||||
|
||||
@@ -58,7 +58,7 @@ func TestTTLBasedCleanup(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
// Create old notification (created 2 seconds ago)
|
||||
oldTime := time.Now().Add(-2 * time.Second)
|
||||
@@ -128,7 +128,7 @@ func TestMaxSizeEnforcement(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
// Create 10 notifications
|
||||
for i := 0; i < 10; i++ {
|
||||
@@ -179,7 +179,7 @@ func TestCleanupRemovesOldestFirst(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
// Create notifications with distinct times
|
||||
baseTime := time.Now()
|
||||
@@ -234,7 +234,7 @@ func TestCleanupDisabled(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
// Create old notification
|
||||
oldTime := time.Now().Add(-2 * time.Second)
|
||||
@@ -278,7 +278,7 @@ func TestCleanupConcurrency(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
// Create some initial notifications
|
||||
for i := 0; i < 10; i++ {
|
||||
@@ -404,10 +404,11 @@ func TestCleanupGracefulShutdown(t *testing.T) {
|
||||
t.Errorf("Stop failed: %v", stopErr)
|
||||
}
|
||||
|
||||
// Verify notifications are still intact after graceful shutdown
|
||||
stats, err := svc.GetStats(context.Background())
|
||||
if err == nil && stats.TotalSent > 0 {
|
||||
// This is expected - notifications should persist through shutdown
|
||||
// Verify notifications are still intact after graceful shutdown - it's
|
||||
// expected that notifications persist through shutdown, so there's
|
||||
// nothing further to assert beyond GetStats succeeding.
|
||||
if stats, err := svc.GetStats(context.Background()); err == nil {
|
||||
t.Logf("stats after graceful shutdown: sent=%d", stats.TotalSent)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -432,7 +433,7 @@ func TestCleanupWithMixedNotificationStatuses(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
oldTime := time.Now().Add(-2 * time.Second)
|
||||
|
||||
@@ -499,7 +500,7 @@ func TestCleanupPerformance(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
// Create 5000 old notifications
|
||||
startTime := time.Now()
|
||||
|
||||
@@ -20,7 +20,7 @@ type alwaysFailNotifier struct {
|
||||
calls []time.Time
|
||||
}
|
||||
|
||||
func (n *alwaysFailNotifier) Send(ctx context.Context, notification *domain.Notification) (*domain.NotificationResult, error) {
|
||||
func (n *alwaysFailNotifier) Send(_ context.Context, notification *domain.Notification) (*domain.NotificationResult, error) {
|
||||
n.mu.Lock()
|
||||
n.calls = append(n.calls, time.Now())
|
||||
n.mu.Unlock()
|
||||
@@ -35,7 +35,7 @@ func (n *alwaysFailNotifier) Send(ctx context.Context, notification *domain.Noti
|
||||
|
||||
func (n *alwaysFailNotifier) Type() domain.NotificationType { return domain.TypeStdout }
|
||||
|
||||
func (n *alwaysFailNotifier) Validate(notification *domain.Notification) error { return nil }
|
||||
func (n *alwaysFailNotifier) Validate(_ *domain.Notification) error { return nil }
|
||||
|
||||
func (n *alwaysFailNotifier) Close() error { return nil }
|
||||
|
||||
@@ -71,7 +71,7 @@ func createFailingTestService(t *testing.T, fail domain.Notifier) *NotificationS
|
||||
|
||||
// waitForStatus polls GetNotification until it observes the notification in
|
||||
// the given status, or fails the test after timeout.
|
||||
func waitForStatus(t *testing.T, svc *NotificationService, ctx context.Context, id string, status domain.NotificationStatus, timeout time.Duration) *domain.Notification {
|
||||
func waitForStatus(ctx context.Context, t *testing.T, svc *NotificationService, id string, status domain.NotificationStatus, timeout time.Duration) *domain.Notification {
|
||||
t.Helper()
|
||||
|
||||
deadline := time.Now().Add(timeout)
|
||||
@@ -102,7 +102,7 @@ func TestRetryBackoffExponentialDelaysRequeue(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
notification := &domain.Notification{
|
||||
ID: "backoff-exponential-1",
|
||||
@@ -116,7 +116,7 @@ func TestRetryBackoffExponentialDelaysRequeue(t *testing.T) {
|
||||
t.Fatalf("Send failed: %v", err)
|
||||
}
|
||||
|
||||
waitForStatus(t, svc, ctx, notification.ID, domain.StatusFailed, 5*time.Second)
|
||||
waitForStatus(ctx, t, svc, notification.ID, domain.StatusFailed, 5*time.Second)
|
||||
|
||||
calls := fail.callTimes()
|
||||
if len(calls) != 3 {
|
||||
@@ -152,7 +152,7 @@ func TestRetryBackoffNoneIsImmediate(t *testing.T) {
|
||||
if err := svc.Start(ctx); err != nil {
|
||||
t.Fatalf("Failed to start service: %v", err)
|
||||
}
|
||||
defer svc.Stop()
|
||||
defer func() { _ = svc.Stop() }()
|
||||
|
||||
notification := &domain.Notification{
|
||||
ID: "backoff-none-1",
|
||||
@@ -167,7 +167,7 @@ func TestRetryBackoffNoneIsImmediate(t *testing.T) {
|
||||
t.Fatalf("Send failed: %v", err)
|
||||
}
|
||||
|
||||
waitForStatus(t, svc, ctx, notification.ID, domain.StatusFailed, 2*time.Second)
|
||||
waitForStatus(ctx, t, svc, notification.ID, domain.StatusFailed, 2*time.Second)
|
||||
elapsed := time.Since(start)
|
||||
|
||||
if elapsed > 1*time.Second {
|
||||
|
||||
@@ -10,11 +10,11 @@ import (
|
||||
"github.com/igodwin/notifier/internal/domain"
|
||||
)
|
||||
|
||||
// ctxForClient builds a context carrying an auth.AuthContext for the given
|
||||
// ctxForClient builds a context carrying an auth.Context for the given
|
||||
// client and roles, as REST/gRPC middleware would attach after authenticating
|
||||
// a request.
|
||||
func ctxForClient(clientID string, roles ...string) context.Context {
|
||||
return auth.ContextWithAuth(context.Background(), &auth.AuthContext{
|
||||
return auth.ContextWithAuth(context.Background(), &auth.Context{
|
||||
ClientID: clientID,
|
||||
Roles: roles,
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user