Fix P1/P2 infrastructure issues: panic recovery and optimizations

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.
This commit is contained in:
rcourtman 2025-11-07 09:55:22 +00:00
parent ba6d934204
commit 6ca4d9b750
4 changed files with 68 additions and 17 deletions

View file

@ -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

View file

@ -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

View file

@ -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()
}()
}
}

View file

@ -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")
}