diff --git a/autocleanup.go b/autocleanup.go index ff25da4..0b1e4c8 100644 --- a/autocleanup.go +++ b/autocleanup.go @@ -518,9 +518,18 @@ func (tr *TaskRegistry) GetTask(name string) (*BackgroundTask, bool) { return task, exists } -// StopAllTasks stops all registered background tasks +// StopAllTasks stops all registered background tasks. +// +// Tasks can live in two places: those registered directly via +// TaskRegistry.RegisterTask (stored in tr.tasks) and process-global +// singletons created via TaskRegistry.CreateSingletonTask, which delegate to +// GetResourceManager().RegisterBackgroundTask and store the task in the +// ResourceManager's registry (see CreateSingletonTask below) — NOT in +// tr.tasks. Stop both so neither a direct task nor a singleton task leaks its +// ticker goroutine past the last Close. func (tr *TaskRegistry) StopAllTasks() { - // First, copy the tasks map to avoid deadlock with GetTaskCount() + // First, stop tasks registered directly in this registry. + // Copy the tasks map to avoid deadlock with GetTaskCount(). tr.mu.Lock() tasksCopy := make(map[string]*BackgroundTask, len(tr.tasks)) for name, task := range tr.tasks { @@ -530,13 +539,15 @@ func (tr *TaskRegistry) StopAllTasks() { tr.tasks = make(map[string]*BackgroundTask) tr.mu.Unlock() - // Now stop all tasks without holding the lock - for name, task := range tasksCopy { + for _, task := range tasksCopy { task.Stop() - if tr.logger != nil { - tr.logger.Debug("Stopped background task during shutdown: %s", name) - } } + + // Then stop process-global singleton tasks, which live in the + // ResourceManager's registry, not in tr.tasks (otherwise the singleton + // task teardown would be a silent no-op and leak every singleton task's + // ticker goroutine past the last Close). + GetResourceManager().StopAllTasks() } // GetTaskCount returns the number of active tasks diff --git a/review_r88_test.go b/review_r88_test.go new file mode 100644 index 0000000..899e521 --- /dev/null +++ b/review_r88_test.go @@ -0,0 +1,64 @@ +package traefikoidc + +import ( + "fmt" + "sync" + "sync/atomic" + "testing" + "time" +) + +// R88 regression: process-global singleton tasks are created via +// TaskRegistry.CreateSingletonTask but stored in the ResourceManager's +// registry, not in TaskRegistry.tasks. TaskRegistry.StopAllTasks previously +// iterated its own (always-empty) registry, silently leaking every singleton +// task's ticker goroutine past the last Close (memory-monitor, +// replay-cache-cleanup). It must now delegate to the ResourceManager that +// actually owns the tasks so a last-instance shutdown stops them. +func TestR88StopAllTasksStopsResourceManagerOwnedTasks(t *testing.T) { + rm := GetResourceManager() + reg := GetGlobalTaskRegistry() + + // Snapshot the currently-running singleton tasks so we can re-create them + // after the global teardown below and leave the shared suite un-polluted. + type restore struct { + name string + interval time.Duration + fn func() + logger *Logger + wg *sync.WaitGroup + } + var toRestore []restore + rm.tasksMu.RLock() + for name, task := range rm.tasks { + if atomic.LoadInt32(&task.started) == 1 && atomic.LoadInt32(&task.stopped) == 0 { + toRestore = append(toRestore, restore{name, task.interval, task.taskFunc, task.logger, task.externalWG}) + } + } + rm.tasksMu.RUnlock() + + var wg sync.WaitGroup + name := fmt.Sprintf("shutdown-probe-%d", time.Now().UnixNano()) + if _, err := reg.CreateSingletonTask(name, time.Hour, func() {}, newNoOpLogger(), &wg); err != nil { + t.Fatalf("CreateSingletonTask: %v", err) + } + if !rm.IsTaskRunning(name) { + t.Fatalf("expected singleton task %q to be running after CreateSingletonTask", name) + } + + // This is the exact shutdown path Close() relies on + // (utilities.go: lastInstance -> GetGlobalTaskRegistry().StopAllTasks()). + reg.StopAllTasks() + + if rm.IsTaskRunning(name) { + t.Fatalf("singleton task %q still running after StopAllTasks: global task teardown is a no-op (task lived in ResourceManager, registry iterated its own empty map)", name) + } + + // Restore the pre-existing running singletons so this global teardown does + // not destabilize sibling tests that still hold live instances. + for _, r := range toRestore { + if _, err := reg.CreateSingletonTask(r.name, r.interval, r.fn, r.logger, r.wg); err != nil { + t.Errorf("restore task %q: %v", r.name, err) + } + } +} diff --git a/singleton_resources.go b/singleton_resources.go index 8d56337..d4036f7 100644 --- a/singleton_resources.go +++ b/singleton_resources.go @@ -178,6 +178,24 @@ func (rm *ResourceManager) StopBackgroundTask(name string) error { return nil } +// StopAllTasks stops every background task registered with this ResourceManager. +// BackgroundTask.Stop is stopOnce-guarded, so repeated calls are safe. This is +// the terminal path for process-global singleton tasks; it must be called only +// when the last plugin instance is shutting down, otherwise it would kill +// cleanup for surviving instances sharing the same singletons. +func (rm *ResourceManager) StopAllTasks() { + rm.tasksMu.RLock() + tasks := make([]*BackgroundTask, 0, len(rm.tasks)) + for _, task := range rm.tasks { + tasks = append(tasks, task) + } + rm.tasksMu.RUnlock() + + for _, task := range tasks { + task.Stop() + } +} + // IsTaskRunning checks if a background task is running func (rm *ResourceManager) IsTaskRunning(name string) bool { rm.tasksMu.RLock()