From 6ca4d9b750ddc9cc5bc3f9a188a2e5523030cf16 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Fri, 7 Nov 2025 09:55:22 +0000 Subject: [PATCH] Fix P1/P2 infrastructure issues: panic recovery and optimizations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit addresses 4 P1 important issues and 1 P2 optimization in infrastructure components: **P1-1: Missing Panic Recovery in Discovery Service** (service.go:172-195, 499-542) - **Problem**: No panic recovery in Start(), ForceRefresh(), SetSubnet() goroutines - **Impact**: Silent service death if scan panics, broken discovery with no monitoring - **Fix**: - Wrapped initial scan goroutine with defer/recover (lines 172-182) - Wrapped scanLoop goroutine with defer/recover (lines 185-195) - Wrapped ForceRefresh scan with defer/recover (lines 499-509) - Wrapped SetSubnet scan with defer/recover (lines 532-542) - All log panics with stack traces for debugging **P1-2: Missing Panic Recovery in Config Watcher Callback** (watcher.go:546-556) - **Problem**: User-provided onMockReload callback could panic and crash watcher - **Impact**: Panicking callback kills watcher goroutine, no config updates - **Fix**: Wrapped callback invocation with defer/recover and stack trace logging **P1-3: Session Store Stop() Using Send Instead of Close** (session_store.go:16-84) - **Problem**: Stop() used channel send which blocks if nobody reads - **Impact**: Stop() hangs if backgroundWorker already exited - **Fix**: - Added sync.Once field stopOnce (line 22) - Changed Stop() to use close() within stopOnce.Do() (lines 80-84) - Prevents double-close panic and ensures all readers are signaled **P2-1: Backup Cleanup Inefficient O(n²) Sort** (persistence.go:1424-1427) - **Problem**: Bubble sort used to sort backups by modification time - **Impact**: Inefficient for large backup counts (>100 files) - **Fix**: - Replaced bubble sort with sort.Slice() using O(n log n) algorithm - Added "sort" import (line 9) - Maintains same oldest-first ordering for deletion logic All fixes add defensive programming without changing external behavior. Panic recovery ensures services continue operating even with bugs, while optimization reduces cleanup time for backup-heavy environments. --- internal/api/session_store.go | 9 ++++-- internal/config/persistence.go | 13 ++++----- internal/config/watcher.go | 12 +++++++- internal/discovery/service.go | 51 ++++++++++++++++++++++++++++++---- 4 files changed, 68 insertions(+), 17 deletions(-) diff --git a/internal/api/session_store.go b/internal/api/session_store.go index fd96aee..9007365 100644 --- a/internal/api/session_store.go +++ b/internal/api/session_store.go @@ -19,6 +19,7 @@ type SessionStore struct { dataPath string saveTicker *time.Ticker stopChan chan bool + stopOnce sync.Once // Ensures Stop() can only close channel once } func sessionHash(token string) string { @@ -76,9 +77,11 @@ func (s *SessionStore) backgroundWorker() { // Stop gracefully stops the session store func (s *SessionStore) Stop() { - s.saveTicker.Stop() - s.stopChan <- true - s.save() + s.stopOnce.Do(func() { + s.saveTicker.Stop() + close(s.stopChan) // Use close instead of send to signal all readers + s.save() + }) } // CreateSession creates a new session diff --git a/internal/config/persistence.go b/internal/config/persistence.go index 7aaa5e7..5b40721 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "sync" "time" @@ -1421,14 +1422,10 @@ func (c *ConfigPersistence) cleanupOldBackups(pattern string) { files = append(files, fileInfo{path: match, modTime: info.ModTime()}) } - // Sort oldest first - for i := 0; i < len(files)-1; i++ { - for j := i + 1; j < len(files); j++ { - if files[i].modTime.After(files[j].modTime) { - files[i], files[j] = files[j], files[i] - } - } - } + // Sort oldest first using efficient sort + sort.Slice(files, func(i, j int) bool { + return files[i].modTime.Before(files[j].modTime) + }) // Delete oldest backups (keep last 10) toDelete := len(files) - maxBackups diff --git a/internal/config/watcher.go b/internal/config/watcher.go index ab7dce0..6bf992f 100644 --- a/internal/config/watcher.go +++ b/internal/config/watcher.go @@ -543,6 +543,16 @@ func (cw *ConfigWatcher) reloadMockConfig() { // Trigger callback to restart backend if set if callback != nil { log.Info().Msg("Triggering backend restart due to mock.env change") - go callback() + go func() { + defer func() { + if r := recover(); r != nil { + log.Error(). + Interface("panic", r). + Stack(). + Msg("Recovered from panic in mock.env callback") + } + }() + callback() + }() } } diff --git a/internal/discovery/service.go b/internal/discovery/service.go index a272975..0067d12 100644 --- a/internal/discovery/service.go +++ b/internal/discovery/service.go @@ -168,10 +168,31 @@ func (s *Service) Start(ctx context.Context) { Msg("Starting background discovery service") // Do initial scan immediately - go s.performScan() + // Initial scan with panic recovery + go func() { + defer func() { + if r := recover(); r != nil { + log.Error(). + Interface("panic", r). + Stack(). + Msg("Recovered from panic in initial discovery scan") + } + }() + s.performScan() + }() - // Start background scanning loop - go s.scanLoop() + // Start background scanning loop with panic recovery + go func() { + defer func() { + if r := recover(); r != nil { + log.Error(). + Interface("panic", r). + Stack(). + Msg("Recovered from panic in discovery scan loop") + } + }() + s.scanLoop() + }() } // Stop stops the background discovery service @@ -475,7 +496,17 @@ func (s *Service) ForceRefresh() { } s.mu.RUnlock() - go s.performScan() + go func() { + defer func() { + if r := recover(); r != nil { + log.Error(). + Interface("panic", r). + Stack(). + Msg("Recovered from panic in ForceRefresh scan") + } + }() + s.performScan() + }() } // SetInterval updates the scan interval @@ -498,7 +529,17 @@ func (s *Service) SetSubnet(subnet string) { // Trigger immediate rescan with new subnet if not already scanning if !alreadyScanning { - go s.performScan() + go func() { + defer func() { + if r := recover(); r != nil { + log.Error(). + Interface("panic", r). + Stack(). + Msg("Recovered from panic in SetSubnet scan") + } + }() + s.performScan() + }() } else { log.Debug().Msg("Scan already in progress, new subnet will be used in next scan") }