From 4d0d86838a90abff5ba927c8cd117743020dcc6c Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sat, 28 Feb 2026 12:56:59 -0500 Subject: [PATCH] refactor(core): remove scheduler and simplify app lifecycle - Delete scheduler.go and scheduler_test.go (no longer needed) - Simplify App struct by removing Handler interface dependency - Remove StartScheduler/StopScheduler from app lifecycle - Update main.go to not pass handler to app constructor - Remove scheduler mock from app tests, simplify test coverage --- cmd/server/main.go | 4 +- internal/app/app.go | 31 +-- internal/app/app_test.go | 321 +--------------------------- internal/services/scheduler.go | 236 -------------------- internal/services/scheduler_test.go | 152 ------------- 5 files changed, 7 insertions(+), 737 deletions(-) delete mode 100644 internal/services/scheduler.go delete mode 100644 internal/services/scheduler_test.go diff --git a/cmd/server/main.go b/cmd/server/main.go index bed9e1d..febc3e6 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -181,14 +181,14 @@ func main() { } // Register all routes and get ebook handler - ebookHandler := router.RegisterRoutes(routerConfig) + _ = router.RegisterRoutes(routerConfig) // ======================================================================== // APPLICATION LIFECYCLE MANAGEMENT // ======================================================================== // Create app with lifecycle management - application := app.New(e, ebookHandler) + application := app.New(e) // ======================================================================== // START SERVER (managed by app lifecycle) diff --git a/internal/app/app.go b/internal/app/app.go index 11bcaaf..4aa417c 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -12,26 +12,18 @@ import ( "github.com/labstack/echo/v4" ) -// Handler interface for services that need lifecycle management -type Handler interface { - StartScheduler() - StopScheduler() -} - // App manages application lifecycle and graceful shutdown type App struct { echo *echo.Echo - handler Handler shutdownTimeout time.Duration shutdownMutex sync.Mutex shutdownDone chan struct{} } // New creates a new App instance -func New(echo *echo.Echo, handler Handler) *App { +func New(echo *echo.Echo) *App { return &App{ echo: echo, - handler: handler, shutdownTimeout: 30 * time.Second, shutdownDone: make(chan struct{}), } @@ -41,9 +33,6 @@ func New(echo *echo.Echo, handler Handler) *App { func (a *App) Start() error { log.Println("Starting application lifecycle management...") - // Start background services - a.startBackgroundServices() - // Setup signal handling for graceful shutdown sigChan := make(chan os.Signal, 1) signal.Notify(sigChan, @@ -66,20 +55,6 @@ func (a *App) Start() error { return nil } -// startBackgroundServices starts all background services -func (a *App) startBackgroundServices() { - log.Println("Starting background services...") - - // Start scheduler for auto-scanning - go func() { - a.handler.StartScheduler() - log.Println("Scheduler started") - }() - - // Note: Watch mode is started by the handlers package - // after a 2-second delay, so we don't duplicate it here -} - // Shutdown performs graceful shutdown of all services func (a *App) Shutdown() error { a.shutdownMutex.Lock() @@ -112,10 +87,6 @@ func (a *App) Shutdown() error { log.Printf("Error stopping HTTP server: %v", err) } - // Stop scheduler - log.Println("Stopping scheduler...") - a.handler.StopScheduler() - log.Println("All services stopped") }() diff --git a/internal/app/app_test.go b/internal/app/app_test.go index c508bc9..6bee51c 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -1,63 +1,21 @@ package app import ( - "context" - "os" - "syscall" "testing" "time" "github.com/labstack/echo/v4" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -// mockHandler is a mock implementation of the Handler interface for testing -type mockHandler struct { - startSchedulerCalled bool - stopSchedulerCalled bool - startDelay time.Duration - stopDelay time.Duration - startError error - stopError error -} - -func (m *mockHandler) StartScheduler() { - m.startSchedulerCalled = true - if m.startDelay > 0 { - time.Sleep(m.startDelay) - } - if m.startError != nil { - panic(m.startError) - } -} - -func (m *mockHandler) StopScheduler() { - m.stopSchedulerCalled = true - if m.stopDelay > 0 { - time.Sleep(m.stopDelay) - } - if m.stopError != nil { - panic(m.stopError) - } -} - -// reset resets the mock handler state -func (m *mockHandler) reset() { - m.startSchedulerCalled = false - m.stopSchedulerCalled = false -} - // TestApp_New tests App constructor func TestApp_New(t *testing.T) { e := echo.New() - handler := &mockHandler{} - app := New(e, handler) + app := New(e) assert.NotNil(t, app, "App should not be nil") assert.Equal(t, e, app.echo, "Echo instance should be stored") - assert.Equal(t, handler, app.handler, "Handler should be stored") assert.Equal(t, 30*time.Second, app.shutdownTimeout, "Default shutdown timeout should be 30 seconds") assert.NotNil(t, app.shutdownDone, "Shutdown done channel should be initialized") } @@ -65,8 +23,7 @@ func TestApp_New(t *testing.T) { // TestApp_SetShutdownTimeout tests configurable shutdown timeout func TestApp_SetShutdownTimeout(t *testing.T) { e := echo.New() - handler := &mockHandler{} - app := New(e, handler) + app := New(e) customTimeout := 15 * time.Second app.SetShutdownTimeout(customTimeout) @@ -77,8 +34,7 @@ func TestApp_SetShutdownTimeout(t *testing.T) { // TestApp_ShutdownDone tests shutdown done channel func TestApp_ShutdownDone(t *testing.T) { e := echo.New() - handler := &mockHandler{} - app := New(e, handler) + app := New(e) channel := app.ShutdownDone() assert.NotNil(t, channel, "ShutdownDone should return a channel") @@ -92,282 +48,13 @@ func TestApp_ShutdownDone(t *testing.T) { } } -// TestApp_Start_BackgroundServices tests background service startup -func TestApp_Start_BackgroundServices(t *testing.T) { - e := echo.New() - handler := &mockHandler{ - startDelay: 100 * time.Millisecond, // Short delay to verify async startup - } - app := New(e, handler) - - // Start the app in a goroutine (simulating main.go) - done := make(chan error, 1) - go func() { - // This will block until shutdown signal - err := app.Start() - done <- err - }() - - // Wait a bit for background services to start - time.Sleep(150 * time.Millisecond) - - // Verify scheduler was started - assert.True(t, handler.startSchedulerCalled, "StartScheduler should be called") - - // Send shutdown signal to clean up - done2 := make(chan bool) - go func() { - // Send shutdown signal - process, err := os.FindProcess(os.Getpid()) - require.NoError(t, err, "Should find current process") - err = process.Signal(syscall.SIGTERM) - done2 <- (err == nil) - }() - - // Wait for app to finish - select { - case err := <-done: - assert.NoError(t, err, "App should shut down without error") - case <-time.After(5 * time.Second): - t.Fatal("App did not shut down within timeout") - } - - <-done2 -} - -// TestApp_Shutdown_GracefulShutdown tests graceful shutdown sequence -func TestApp_Shutdown_GracefulShutdown(t *testing.T) { - e := echo.New() - handler := &mockHandler{ - stopDelay: 50 * time.Millisecond, - } - app := New(e, handler) - - // Test shutdown - err := app.Shutdown() - - assert.NoError(t, err, "Shutdown should complete without error") - assert.True(t, handler.stopSchedulerCalled, "StopScheduler should be called") -} - -// TestApp_Shutdown_ThreadSafety tests thread-safe shutdown -func TestApp_Shutdown_ThreadSafety(t *testing.T) { - e := echo.New() - handler := &mockHandler{} - app := New(e, handler) - - // Call shutdown multiple times concurrently - errors := make(chan error, 3) - for i := 0; i < 3; i++ { - go func() { - errors <- app.Shutdown() - }() - } - - // Collect results - for i := 0; i < 3; i++ { - err := <-errors - // First call should succeed, subsequent calls should also succeed (no-op) - assert.NoError(t, err, "Concurrent shutdown calls should not error") - } - - // Verify handler was stopped only once - assert.True(t, handler.stopSchedulerCalled, "StopScheduler should be called at least once") -} - -// TestApp_Shutdown_Timeout tests shutdown timeout handling -func TestApp_Shutdown_Timeout(t *testing.T) { - e := echo.New() - handler := &mockHandler{ - stopDelay: 2 * time.Second, // Longer than shutdown timeout - } - app := New(e, handler) - app.SetShutdownTimeout(100 * time.Millisecond) // Set very short timeout - - // Test shutdown with timeout - err := app.Shutdown() - - assert.Error(t, err, "Shutdown should timeout and return error") - assert.True(t, handler.stopSchedulerCalled, "StopScheduler should still be called") -} - -// TestApp_Shutdown_ClosesEchoServer tests HTTP server shutdown -func TestApp_Shutdown_ClosesEchoServer(t *testing.T) { - e := echo.New() - handler := &mockHandler{} - - // Start a simple server - go func() { - e.Start(":0") // Use random port - }() - - // Give server time to start - time.Sleep(100 * time.Millisecond) - - app := New(e, handler) - - // Shutdown should close the echo server - err := app.Shutdown() - - assert.NoError(t, err, "Shutdown should complete") - assert.True(t, handler.stopSchedulerCalled, "StopScheduler should be called") -} - -// TestApp_SignalHandling tests signal handling (SIGINT, SIGTERM, SIGQUIT) -func TestApp_SignalHandling(t *testing.T) { - tests := []struct { - name string - signal os.Signal - }{ - {"SIGINT", syscall.SIGINT}, - {"SIGTERM", syscall.SIGTERM}, - {"SIGQUIT", syscall.SIGQUIT}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - e := echo.New() - handler := &mockHandler{} - app := New(e, handler) - - // Start app in goroutine - done := make(chan error, 1) - go func() { - done <- app.Start() - }() - - // Wait for background services to start - time.Sleep(100 * time.Millisecond) - assert.True(t, handler.startSchedulerCalled, "StartScheduler should be called") - - // Send signal - process, err := os.FindProcess(os.Getpid()) - require.NoError(t, err, "Should find current process") - - err = process.Signal(tt.signal) - require.NoError(t, err, "Should send signal") - - // Wait for shutdown - select { - case err := <-done: - assert.NoError(t, err, "App should shut down without error") - assert.True(t, handler.stopSchedulerCalled, "StopScheduler should be called") - case <-time.After(5 * time.Second): - t.Fatal("App did not shut down within timeout") - } - }) - } -} - -// TestApp_Integration_StartupSequence tests complete startup sequence -func TestApp_Integration_StartupSequence(t *testing.T) { - if testing.Short() { - t.Skip("Skipping integration test in short mode") - } - - e := echo.New() - handler := &mockHandler{} - app := New(e, handler) - - // This test simulates the sequence in cmd/server/main.go: - // 1. Create app - // 2. Start HTTP server in background - // 3. Call app.Start() which blocks until signal - - // Step 1: App already created above - assert.NotNil(t, app, "App should be created") - - // Step 2: Start HTTP server (simulated) - serverReady := make(chan bool) - go func() { - // In real main.go, this would be: e.Start(":" + cfg.ServerPort) - // For testing, we don't actually start the server - serverReady <- true - <-context.Background().Done() // Block until done - }() - - <-serverReady - - // Step 3: Start app lifecycle in background - appDone := make(chan error, 1) - go func() { - appDone <- app.Start() - }() - - // Verify background services started - time.Sleep(150 * time.Millisecond) - assert.True(t, handler.startSchedulerCalled, "Scheduler should be started") - - // Send shutdown signal (simulating user pressing Ctrl+C) - process, err := os.FindProcess(os.Getpid()) - require.NoError(t, err) - err = process.Signal(syscall.SIGINT) - require.NoError(t, err) - - // Wait for graceful shutdown - select { - case err := <-appDone: - assert.NoError(t, err, "App should shut down gracefully") - assert.True(t, handler.stopSchedulerCalled, "Scheduler should be stopped") - case <-time.After(5 * time.Second): - t.Fatal("App did not complete shutdown within timeout") - } -} - -// TestApp_HandlerInterface tests that Handler interface is properly implemented -func TestApp_HandlerInterface(t *testing.T) { - // This test verifies that the mock handler satisfies the Handler interface - var _ Handler = &mockHandler{} -} - -// TestApp_AutoStartVerification tests auto-start functionality -func TestApp_AutoStartVerification(t *testing.T) { - t.Run("Auto-start runs asynchronously", func(t *testing.T) { - e := echo.New() - handler := &mockHandler{ - startDelay: 100 * time.Millisecond, - } - app := New(e, handler) - - // Start the app - it should start background services and return immediately - // Then we send a shutdown signal right away - done := make(chan error, 1) - go func() { - // This will start services in background, then wait for signal - err := app.Start() - done <- err - }() - - // Wait a bit for background services to start - time.Sleep(150 * time.Millisecond) - - // Verify scheduler was started in background - assert.True(t, handler.startSchedulerCalled, "StartScheduler should be called in background") - - // Send shutdown signal - process, err := os.FindProcess(os.Getpid()) - require.NoError(t, err) - _ = process.Signal(syscall.SIGTERM) - - // Wait for clean shutdown - select { - case err := <-done: - assert.NoError(t, err, "App should shut down gracefully") - case <-time.After(5 * time.Second): - t.Fatal("App did not shut down within timeout") - } - }) -} - // BenchmarkApp_Shutdown benchmarks the shutdown process func BenchmarkApp_Shutdown(b *testing.B) { e := echo.New() - handler := &mockHandler{} b.ResetTimer() for i := 0; i < b.N; i++ { - handler.reset() - app := New(e, handler) + app := New(e) app.Shutdown() } } diff --git a/internal/services/scheduler.go b/internal/services/scheduler.go deleted file mode 100644 index 3340fce..0000000 --- a/internal/services/scheduler.go +++ /dev/null @@ -1,236 +0,0 @@ -package services - -import ( - "bookhoard/internal/database" - "context" - "fmt" - "log" - "strconv" - "sync" - "time" - - "github.com/google/uuid" - "github.com/jackc/pgx/v5/pgtype" -) - -type Scheduler struct { - worker *Worker - db Database - timers map[string]*time.Timer - mu sync.RWMutex - scanSettings map[string]ScanSetting - ctx context.Context - cancel context.CancelFunc - wg sync.WaitGroup -} - -type Database interface { - GetSystemSetting(ctx context.Context, settingKey string) (string, error) - ListLibraries(ctx context.Context) ([]database.ListLibrariesRow, error) - GetLibraryFolders(ctx context.Context, libraryID pgtype.UUID) ([]database.LibraryFolders, error) -} - -type Library struct { - ID pgtype.UUID - Name string -} - -type LibraryFolder struct { - ID pgtype.UUID - LibraryID pgtype.UUID - FolderPath string -} - -type ScanSetting struct { - UserID string - Enabled bool - Frequency int -} - -func NewScheduler(worker *Worker, db Database) *Scheduler { - ctx, cancel := context.WithCancel(context.Background()) - - return &Scheduler{ - worker: worker, - db: db, - timers: make(map[string]*time.Timer), - scanSettings: make(map[string]ScanSetting), - ctx: ctx, - cancel: cancel, - } -} - -func (s *Scheduler) Start() { - s.wg.Add(1) - go func() { - defer s.wg.Done() - s.runSettingsChecker() - }() -} - -func (s *Scheduler) Stop() { - s.cancel() - - s.mu.Lock() - for _, timer := range s.timers { - if timer != nil { - timer.Stop() - } - } - s.timers = make(map[string]*time.Timer) - s.mu.Unlock() - - s.wg.Wait() -} - -func (s *Scheduler) runSettingsChecker() { - ticker := time.NewTicker(5 * time.Minute) - defer ticker.Stop() - - for { - select { - case <-ticker.C: - s.checkAndScheduleScans() - case <-s.ctx.Done(): - return - } - } -} - -func (s *Scheduler) checkAndScheduleScans() { - ctx, cancel := context.WithTimeout(s.ctx, 30*time.Second) - defer cancel() - - libraries, err := s.db.ListLibraries(ctx) - if err != nil { - log.Printf("Error fetching libraries for scan scheduling: %v", err) - return - } - - autoScanEnabledStr, err := s.db.GetSystemSetting(ctx, "auto_scan_enabled") - if err != nil { - log.Printf("Error getting auto_scan_enabled setting: %v", err) - return - } - - autoScanEnabled, err := strconv.ParseBool(autoScanEnabledStr) - if err != nil { - log.Printf("Error parsing auto_scan_enabled: %v", err) - return - } - - if !autoScanEnabled { - return - } - - scanFrequencyStr, err := s.db.GetSystemSetting(ctx, "scan_frequency_minutes") - if err != nil { - log.Printf("Error getting scan_frequency_minutes setting: %v", err) - return - } - - scanFrequency, err := strconv.Atoi(scanFrequencyStr) - if err != nil { - log.Printf("Error parsing scan_frequency_minutes: %v", err) - return - } - - if scanFrequency < 15 { - return - } - - for _, library := range libraries { - libraryIDStr := fmt.Sprintf("%x", library.ID.Bytes) - - s.mu.Lock() - currentSetting, exists := s.scanSettings[libraryIDStr] - scanSetting := ScanSetting{ - UserID: libraryIDStr, - Enabled: autoScanEnabled, - Frequency: scanFrequency, - } - s.scanSettings[libraryIDStr] = scanSetting - s.mu.Unlock() - - if !exists || currentSetting.Frequency != scanSetting.Frequency { - s.scheduleLibraryScan(ctx, library.ID, libraryIDStr, scanSetting.Frequency) - } - } -} - -func (s *Scheduler) scheduleLibraryScan(ctx context.Context, libraryID pgtype.UUID, userID string, frequencyMinutes int) { - userIDStr := fmt.Sprintf("%x", libraryID.Bytes) - - s.mu.Lock() - defer s.mu.Unlock() - - timerID := userIDStr + "-scan" - - if existingTimer, exists := s.timers[timerID]; exists { - existingTimer.Stop() - } - - duration := time.Duration(frequencyMinutes) * time.Minute - timer := time.AfterFunc(duration, func() { - s.triggerScheduledScan(ctx, libraryID, userIDStr) - - s.scheduleLibraryScan(ctx, libraryID, userID, frequencyMinutes) - }) - - s.timers[timerID] = timer - - libraryIDStr := fmt.Sprintf("%x", libraryID.Bytes) - log.Printf("Scheduled scan for library %s every %d minutes", libraryIDStr, frequencyMinutes) -} - -func (s *Scheduler) triggerScheduledScan(ctx context.Context, libraryID pgtype.UUID, userID string) { - libraryIDStr := fmt.Sprintf("%x", libraryID.Bytes) - log.Printf("Triggering scheduled scan for library %s", libraryIDStr) - - folders, err := s.db.GetLibraryFolders(ctx, libraryID) - if err != nil { - log.Printf("Error getting folders for library %s: %v", libraryIDStr, err) - return - } - - if len(folders) == 0 { - log.Printf("No folders configured for library %s, skipping scan", libraryIDStr) - return - } - - folderPaths := make([]string, len(folders)) - for i, folder := range folders { - folderPaths[i] = folder.FolderPath - } - - job := &Job{ - ID: uuid.New().String(), - Type: JobTypeScan, - Params: map[string]interface{}{ - "library_id": fmt.Sprintf("%x", libraryID.Bytes), - "folders": folderPaths, - "admin_id": userID, - "db": s.db, - }, - Status: JobStatusPending, - Context: ctx, - } - - if err := s.worker.EnqueueJob(job); err != nil { - log.Printf("Error enqueuing scan job: %v", err) - } else { - libraryIDStr := fmt.Sprintf("%x", libraryID.Bytes) - log.Printf("Enqueued scan job %s for library %s", job.ID, libraryIDStr) - } -} - -func (s *Scheduler) UpdateScanSettings(userID string, enabled bool, frequencyMinutes int) { - s.mu.Lock() - defer s.mu.Unlock() - - s.scanSettings[userID] = ScanSetting{ - UserID: userID, - Enabled: enabled, - Frequency: frequencyMinutes, - } -} diff --git a/internal/services/scheduler_test.go b/internal/services/scheduler_test.go deleted file mode 100644 index 994036b..0000000 --- a/internal/services/scheduler_test.go +++ /dev/null @@ -1,152 +0,0 @@ -package services - -import ( - "testing" - - "github.com/google/uuid" - "github.com/stretchr/testify/assert" -) - -func TestScheduler_NewScheduler(t *testing.T) { - worker := NewWorker(1) - defer worker.Shutdown() - - // Use nil database interface for basic testing - scheduler := NewScheduler(worker, nil) - - assert.NotNil(t, scheduler) - assert.NotNil(t, scheduler.worker) - assert.NotNil(t, scheduler.timers) - assert.NotNil(t, scheduler.scanSettings) - assert.NotNil(t, scheduler.ctx) - assert.NotNil(t, scheduler.cancel) -} - -func TestScheduler_StartStop(t *testing.T) { - worker := NewWorker(1) - scheduler := NewScheduler(worker, nil) - - // Start should not panic - scheduler.Start() - assert.NotNil(t, scheduler.ctx) - - // 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" - - // Update scan settings - scheduler.UpdateScanSettings(userID, true, 30) - - scheduler.mu.Lock() - settings, exists := scheduler.scanSettings[userID] - scheduler.mu.Unlock() - - assert.True(t, exists) - assert.Equal(t, userID, settings.UserID) - assert.True(t, settings.Enabled) - assert.Equal(t, 30, settings.Frequency) -} - -func TestScheduler_UpdateScanSettings_Disabled(t *testing.T) { - worker := NewWorker(1) - defer worker.Shutdown() - scheduler := NewScheduler(worker, nil) - - userID := "test-user-456" - - // Update scan settings to disabled - scheduler.UpdateScanSettings(userID, false, 60) - - scheduler.mu.Lock() - settings, exists := scheduler.scanSettings[userID] - scheduler.mu.Unlock() - - assert.True(t, exists) - assert.False(t, settings.Enabled) - assert.Equal(t, 60, settings.Frequency) -} - -func TestScheduler_UpdateScanSettings_Overwrite(t *testing.T) { - worker := NewWorker(1) - defer worker.Shutdown() - scheduler := NewScheduler(worker, nil) - - userID := "test-user-789" - - // First update - scheduler.UpdateScanSettings(userID, true, 30) - - // Overwrite with different settings - scheduler.UpdateScanSettings(userID, false, 45) - - scheduler.mu.Lock() - settings, exists := scheduler.scanSettings[userID] - scheduler.mu.Unlock() - - assert.True(t, exists) - assert.False(t, settings.Enabled) - assert.Equal(t, 45, settings.Frequency) -} - -func TestScheduler_StopWithActiveTimers(t *testing.T) { - worker := NewWorker(1) - scheduler := NewScheduler(worker, nil) - - // Add some fake timers - scheduler.mu.Lock() - scheduler.timers["timer1"] = nil - scheduler.timers["timer2"] = nil - scheduler.timers["timer3"] = nil - scheduler.mu.Unlock() - - // Stop should clear timers - scheduler.Stop() - - scheduler.mu.Lock() - timerCount := len(scheduler.timers) - 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() - - // Concurrent updates should not cause race conditions - done := make(chan bool, 10) - - for i := 0; i < 10; i++ { - go func(index int) { - userID := uuid.New().String() - scheduler.UpdateScanSettings(userID, true, 30) - done <- true - }(i) - } - - // Wait for all goroutines - for i := 0; i < 10; i++ { - <-done - } - - // Verify all settings were stored - scheduler.mu.Lock() - settingCount := len(scheduler.scanSettings) - scheduler.mu.Unlock() - - assert.Equal(t, 10, settingCount) - - scheduler.Stop() - worker.Shutdown() -}