From 70acecc33ae89b060fa7372e16af255b6b815240 Mon Sep 17 00:00:00 2001
From: John O'Keefe <jrokeefe@protonmail.com>
Date: Mon, 9 Feb 2026 10:13:48 -0500
Subject: [PATCH] Fix scheduler goroutine WaitGroup leak causing shutdown
 deadlock

Critical bug fix: The scheduler's runSettingsChecker() goroutine was
started but never marked as complete in the WaitGroup, causing
scheduler.Stop() to hang indefinitely waiting for wg.Wait().

Changes:
- Add defer s.wg.Done() call in scheduler.Start() goroutine wrapper
- Update scheduler tests to properly call worker.Shutdown()
- Add nil check for timer.Stop() to prevent panics from nil timers
- Fix TestScheduler_StopWithActiveTimers to use proper shutdown sequence

Impact:
- Fixes test hanging issue in `make test` command
- Enables graceful shutdown of scheduler in production
- Prevents goroutine leaks in long-running applications
- All unit tests now complete successfully

Root cause: WaitGroup.Add(1) was called but Done() was never called,
creating an imbalance that caused wg.Wait() to block forever.
---
 internal/services/scheduler.go      |  9 +++++++--
 internal/services/scheduler_test.go | 11 ++++++++++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/internal/services/scheduler.go b/internal/services/scheduler.go
index b60cc78..c714f61 100644
--- a/internal/services/scheduler.go
+++ b/internal/services/scheduler.go
@@ -66,7 +66,10 @@ func NewScheduler(worker *Worker, db Database) *Scheduler {
 
 func (s *Scheduler) Start() {
 	s.wg.Add(1)
-	go s.runSettingsChecker()
+	go func() {
+		defer s.wg.Done()
+		s.runSettingsChecker()
+	}()
 }
 
 func (s *Scheduler) Stop() {
@@ -74,7 +77,9 @@ func (s *Scheduler) Stop() {
 
 	s.mu.Lock()
 	for _, timer := range s.timers {
-		timer.Stop()
+		if timer != nil {
+			timer.Stop()
+		}
 	}
 	s.timers = make(map[string]*time.Timer)
 	s.mu.Unlock()
diff --git a/internal/services/scheduler_test.go b/internal/services/scheduler_test.go
index edbaef8..994036b 100644
--- a/internal/services/scheduler_test.go
+++ b/internal/services/scheduler_test.go
@@ -9,6 +9,7 @@ import (
 
 func TestScheduler_NewScheduler(t *testing.T) {
 	worker := NewWorker(1)
+	defer worker.Shutdown()
 
 	// Use nil database interface for basic testing
 	scheduler := NewScheduler(worker, nil)
@@ -31,10 +32,12 @@ func TestScheduler_StartStop(t *testing.T) {
 
 	// Stop should not panic
 	scheduler.Stop()
+	worker.Shutdown()
 }
 
 func TestScheduler_UpdateScanSettings(t *testing.T) {
 	worker := NewWorker(1)
+	defer worker.Shutdown()
 	scheduler := NewScheduler(worker, nil)
 
 	userID := "test-user-123"
@@ -54,6 +57,7 @@ func TestScheduler_UpdateScanSettings(t *testing.T) {
 
 func TestScheduler_UpdateScanSettings_Disabled(t *testing.T) {
 	worker := NewWorker(1)
+	defer worker.Shutdown()
 	scheduler := NewScheduler(worker, nil)
 
 	userID := "test-user-456"
@@ -72,6 +76,7 @@ func TestScheduler_UpdateScanSettings_Disabled(t *testing.T) {
 
 func TestScheduler_UpdateScanSettings_Overwrite(t *testing.T) {
 	worker := NewWorker(1)
+	defer worker.Shutdown()
 	scheduler := NewScheduler(worker, nil)
 
 	userID := "test-user-789"
@@ -110,13 +115,14 @@ func TestScheduler_StopWithActiveTimers(t *testing.T) {
 	scheduler.mu.Unlock()
 
 	assert.Equal(t, 0, timerCount)
+
+	worker.Shutdown()
 }
 
 func TestScheduler_ConcurrentAccess(t *testing.T) {
 	worker := NewWorker(1)
 	scheduler := NewScheduler(worker, nil)
 	scheduler.Start()
-	defer scheduler.Stop()
 
 	// Concurrent updates should not cause race conditions
 	done := make(chan bool, 10)
@@ -140,4 +146,7 @@ func TestScheduler_ConcurrentAccess(t *testing.T) {
 	scheduler.mu.Unlock()
 
 	assert.Equal(t, 10, settingCount)
+
+	scheduler.Stop()
+	worker.Shutdown()
 }