From ba6d9342046145899e1dbb5c9ff4525a4c16e7ae Mon Sep 17 00:00:00 2001 From: rcourtman Date: Fri, 7 Nov 2025 09:49:55 +0000 Subject: [PATCH] Fix critical P0 infrastructure concurrency issues This commit addresses 3 critical P0 race conditions and resource leaks in core infrastructure: **P0-1: Discovery Service Goroutine Leak** (service.go:468, 488) - **Problem**: ForceRefresh() and SetSubnet() spawned unbounded goroutines without checking if scan already in progress - **Impact**: Rapid API calls create goroutine explosion, resource exhaustion - **Fix**: - ForceRefresh: Check isScanning before spawning goroutine (lines 470-476) - SetSubnet: Check isScanning, defer scan if already running (lines 491-504) - Both now log when skipping to aid debugging **P0-2: Config Persistence Unlock/Relock Race** (persistence.go:1177-1206) - **Problem**: LoadNodesConfig() unlocked RLock, called SaveNodesConfig (acquires Lock), then relocked - **Impact**: Another goroutine could modify config between unlock/relock, causing migrated data loss - **Fix**: - Copy instance slices while holding RLock to ensure consistency (lines 1189-1194) - Release lock, save copies, then return without relocking (lines 1196-1205) - Prevents TOCTOU vulnerability where migrations could be overwritten **P0-3: Config Watcher Channel Close Race** (watcher.go:19-178) - **Problem**: Stop() used select-check-close pattern vulnerable to concurrent calls - **Impact**: Multiple Stop() calls panic on double-close - **Fix**: - Added sync.Once field stopOnce to ConfigWatcher struct (line 26) - Changed Stop() to use stopOnce.Do() ensuring single execution (lines 175-178) - Removed racy select-based guard All fixes maintain backwards compatibility and add defensive logging for operational visibility. --- internal/config/persistence.go | 36 +++++++++++++++++++++++----------- internal/config/watcher.go | 12 +++++------- internal/discovery/service.go | 22 ++++++++++++++++++--- 3 files changed, 49 insertions(+), 21 deletions(-) diff --git a/internal/config/persistence.go b/internal/config/persistence.go index beef2ab..7aaa5e7 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -1174,23 +1174,37 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) { } } - // If any migrations were applied, save the updated configuration - if migrationApplied { - log.Info().Msg("Migrations applied, saving updated configuration") - // Need to unlock before saving to avoid deadlock - c.mu.RUnlock() - if err := c.SaveNodesConfig(config.PVEInstances, config.PBSInstances, config.PMGInstances); err != nil { - log.Error().Err(err).Msg("Failed to save configuration after migration") - } - c.mu.RLock() - } - log.Info().Str("file", c.nodesFile). Int("pve", len(config.PVEInstances)). Int("pbs", len(config.PBSInstances)). Int("pmg", len(config.PMGInstances)). Bool("encrypted", c.crypto != nil). Msg("Nodes configuration loaded") + + // If any migrations were applied, save the updated configuration after releasing the read lock + // This prevents unlock/relock race condition where another goroutine could modify config + // between unlock and relock, causing migrated data to be lost + if migrationApplied { + // Make copies while still holding read lock to ensure consistency + pveCopy := make([]PVEInstance, len(config.PVEInstances)) + copy(pveCopy, config.PVEInstances) + pbsCopy := make([]PBSInstance, len(config.PBSInstances)) + copy(pbsCopy, config.PBSInstances) + pmgCopy := make([]PMGInstance, len(config.PMGInstances)) + copy(pmgCopy, config.PMGInstances) + + // Release read lock before saving (SaveNodesConfig acquires write lock) + c.mu.RUnlock() + + log.Info().Msg("Migrations applied, saving updated configuration") + if err := c.SaveNodesConfig(pveCopy, pbsCopy, pmgCopy); err != nil { + log.Error().Err(err).Msg("Failed to save configuration after migration") + } + + // Don't reacquire lock - we're returning + return &config, nil + } + return &config, nil } diff --git a/internal/config/watcher.go b/internal/config/watcher.go index c200003..ab7dce0 100644 --- a/internal/config/watcher.go +++ b/internal/config/watcher.go @@ -23,6 +23,7 @@ type ConfigWatcher struct { apiTokensPath string watcher *fsnotify.Watcher stopChan chan struct{} + stopOnce sync.Once // Ensures Stop() can only close channel once lastModTime time.Time mockLastModTime time.Time apiTokensLastModTime time.Time @@ -170,14 +171,11 @@ func (cw *ConfigWatcher) Start() error { // Stop stops the config watcher func (cw *ConfigWatcher) Stop() { - select { - case <-cw.stopChan: - // Already stopped - return - default: + // Use sync.Once to prevent double-close panic + cw.stopOnce.Do(func() { close(cw.stopChan) - } - cw.watcher.Close() + cw.watcher.Close() + }) } // ReloadConfig manually triggers a config reload (e.g., from SIGHUP) diff --git a/internal/discovery/service.go b/internal/discovery/service.go index 0615644..a272975 100644 --- a/internal/discovery/service.go +++ b/internal/discovery/service.go @@ -466,6 +466,15 @@ func (s *Service) IsScanning() bool { // ForceRefresh triggers an immediate scan func (s *Service) ForceRefresh() { + // Check if scan is already in progress to prevent goroutine leak + s.mu.RLock() + if s.isScanning { + s.mu.RUnlock() + log.Debug().Msg("Scan already in progress, skipping ForceRefresh") + return + } + s.mu.RUnlock() + go s.performScan() } @@ -480,12 +489,19 @@ func (s *Service) SetInterval(interval time.Duration) { // SetSubnet updates the subnet to scan func (s *Service) SetSubnet(subnet string) { s.mu.Lock() - defer s.mu.Unlock() s.subnet = subnet + // Check if scan is already in progress to prevent goroutine leak + alreadyScanning := s.isScanning + s.mu.Unlock() + log.Info().Str("subnet", subnet).Msg("Updated discovery subnet") - // Trigger immediate rescan with new subnet - go s.performScan() + // Trigger immediate rescan with new subnet if not already scanning + if !alreadyScanning { + go s.performScan() + } else { + log.Debug().Msg("Scan already in progress, new subnet will be used in next scan") + } } // GetStatus returns the current service status