diff --git a/CHANGELOG.md b/CHANGELOG.md index 19e3e0e..3a1ea7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,32 @@ ## Changelog +### What was just completed (2026-02-18 session 40) +- **v0.12.4 — Correctness & Robustness Bug Fixes (TASK.md — 15 bugs fixed):** + + **CRITICAL fixes (data loss, panics):** + - **C1: `SetAppBackupBulk` data loss + nil map panic** — Fixed: now updates map IN PLACE instead of replacing it, so stacks absent from the input are preserved. Added nil guard for `s.AppBackup`. (`internal/settings/settings.go`) + - **C2: `UpdateStackConfig` nil Env map panic** — Added nil check `if appCfg.Env == nil { appCfg.Env = make(...) }` before the field assignment loop. (`internal/stacks/deploy.go`) + - **C3: `ValidateDump` missing scanner.Err() check** — Added `if err := scanner.Err()` check after the scan loop so I/O errors don't silently mark a partial dump as valid. (`internal/backup/dbdump.go`) + + **HIGH fixes (logic errors, resource leaks):** + - **H1: `nextDailyRun` DST bug** — Replaced `next.Add(24 * time.Hour)` with `time.Date(day+1, ...)` for correct scheduling across Europe/Budapest DST transitions. (`internal/scheduler/scheduler.go`) + - **H2: `nextDailyRun` repeated `LoadLocation`** — Cached timezone in package-level `sync.Once` variable; `getBudapestLocation()` now loaded only once. (`internal/scheduler/scheduler.go`) + - **H3: `settings.save()` .tmp file leak** — Added `os.Remove(tmpPath)` cleanup on `WriteFile` failure path. (`internal/settings/settings.go`) + - **H4: `SetNotificationPrefs` nil pointer panic** — Added nil guard at start of function, returns error instead of panicking. (`internal/settings/settings.go`) + - **H5: `appDirSize` ignores `Sscanf` return value** — Now checks `n != 1` and returns `(0, "?")` on parse failure. Same fix applied to `getDirSizeBytes` in `stacks/delete.go`. (`internal/backup/appdata.go`, `internal/stacks/delete.go`) + - **H6: `getDirSizeBytes` no timeout** — Added `exec.CommandContext` with 30s timeout. Added `"context"` import. (`internal/stacks/delete.go`) + - **H7: `dbdump.go` tmpFile not using `defer Close`** — Replaced explicit `tmpFile.Close()` call with `defer tmpFile.Close()` so the file handle is released even on panic. (`internal/backup/dbdump.go`) + - **H8: `UpdateCrossDriveStatus` misleading comment** — Updated comment to accurately describe the "does nothing if nil" behavior instead of claiming it "creates one if nil". (`internal/settings/settings.go`) + + **MEDIUM fixes (code quality, edge cases):** + - **M1: Custom `contains`/`containsBytes` replaced** — Removed bespoke `containsBytes` and simplified `contains` to delegate to `strings.Contains`. Added `"strings"` import. (`internal/notify/notifier.go`) + - **M2: `scheduler.Every()` doesn't validate interval** — Added early return with error log if `interval <= 0` to prevent panic in `time.NewTicker`. (`internal/scheduler/scheduler.go`) + - **M3: `executeJob` panic recovery missing `LastRun`** — Panic recovery defer now also sets `job.LastRun = time.Now()` so the job status shows a timestamp after a panic. (`internal/scheduler/scheduler.go`) + - **M4: `logPostStartStatus` goroutine captures env by reference** — Copies the env slice before launching the goroutine (`envCopy`). (`internal/stacks/manager.go`) + - **M5: Multiple `time.LoadLocation` calls in web package** — Added package-level `getTimezone()` with `sync.Once` in `funcmap.go`. Replaced all `time.LoadLocation("Europe/Budapest")` calls in the web package with `getTimezone()`. (`internal/web/funcmap.go`, `internal/web/handlers.go`) + + **Files modified (8):** `internal/settings/settings.go`, `internal/stacks/deploy.go`, `internal/backup/dbdump.go`, `internal/scheduler/scheduler.go`, `internal/backup/appdata.go`, `internal/stacks/delete.go`, `internal/stacks/manager.go`, `internal/notify/notifier.go`, `internal/web/funcmap.go`, `internal/web/handlers.go` + ### What was just completed (2026-02-17 session 39) - **v0.12.3 — Security & Correctness Bug Fixes (TASK.md — 33 bugs fixed):** diff --git a/controller/internal/backup/appdata.go b/controller/internal/backup/appdata.go index 610ec1f..597b6e4 100644 --- a/controller/internal/backup/appdata.go +++ b/controller/internal/backup/appdata.go @@ -147,7 +147,9 @@ func appDirSize(path string) (int64, string) { return 0, "?" } var size int64 - fmt.Sscanf(fields[0], "%d", &size) + if n, _ := fmt.Sscanf(fields[0], "%d", &size); n != 1 { + return 0, "?" + } return size, humanizeBytes(size) } diff --git a/controller/internal/backup/dbdump.go b/controller/internal/backup/dbdump.go index 7f23da9..0b7af06 100644 --- a/controller/internal/backup/dbdump.go +++ b/controller/internal/backup/dbdump.go @@ -181,13 +181,13 @@ func DumpOne(ctx context.Context, db DiscoveredDB, dumpDir string, logger *log.L result.Duration = time.Since(start) return result } + defer tmpFile.Close() cmd.Stdout = tmpFile var stderr strings.Builder cmd.Stderr = &stderr err = cmd.Run() - tmpFile.Close() if err != nil { os.Remove(tmpPath) @@ -293,6 +293,13 @@ func ValidateDump(filePath string, dbType DBType) DumpValidation { tableCount++ } } + + if err := scanner.Err(); err != nil { + v.Error = fmt.Sprintf("hiba az olvasás közben: %v", err) + log.Printf("[WARN] ValidateDump FAIL: %s — scanner error: %v", filePath, err) + return v + } + v.TableCount = tableCount if !headerFound { diff --git a/controller/internal/notify/notifier.go b/controller/internal/notify/notifier.go index 9c1cc08..caaef8a 100644 --- a/controller/internal/notify/notifier.go +++ b/controller/internal/notify/notifier.go @@ -7,6 +7,7 @@ import ( "io" "log" "net/http" + "strings" "sync" "time" @@ -314,14 +315,5 @@ func classifyWarning(message string) string { } func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsBytes(s, substr)) -} - -func containsBytes(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false + return strings.Contains(s, substr) } diff --git a/controller/internal/scheduler/scheduler.go b/controller/internal/scheduler/scheduler.go index 2d8785a..e9083c0 100644 --- a/controller/internal/scheduler/scheduler.go +++ b/controller/internal/scheduler/scheduler.go @@ -8,6 +8,23 @@ import ( "time" ) +var ( + budapestLoc *time.Location + budapestLocOnce sync.Once +) + +func getBudapestLocation() *time.Location { + budapestLocOnce.Do(func() { + loc, err := time.LoadLocation("Europe/Budapest") + if err != nil { + log.Printf("[ERROR] Cannot load Europe/Budapest timezone: %v — using UTC", err) + loc = time.UTC + } + budapestLoc = loc + }) + return budapestLoc +} + // JobFunc is the function signature for scheduler jobs. type JobFunc func(ctx context.Context) error @@ -41,6 +58,11 @@ func New(logger *log.Logger) *Scheduler { // Every registers a periodic job that runs every interval. func (s *Scheduler) Every(name string, interval time.Duration, fn JobFunc) { + if interval <= 0 { + s.logger.Printf("[ERROR] Periodic job %s has invalid interval %s — job not registered", name, interval) + return + } + s.mu.Lock() defer s.mu.Unlock() @@ -187,6 +209,7 @@ func (s *Scheduler) executeJob(job *Job, quiet bool) { if r := recover(); r != nil { s.mu.Lock() job.LastErr = fmt.Errorf("panic: %v", r) + job.LastRun = time.Now() s.mu.Unlock() s.logger.Printf("[ERROR] Job %s panicked: %v", job.Name, r) } @@ -238,18 +261,16 @@ func nextDailyRun(timeStr string) time.Time { return time.Now().Add(24 * time.Hour) } - loc, err := time.LoadLocation("Europe/Budapest") - if err != nil { - // Fallback to UTC if timezone not available - loc = time.UTC - } + loc := getBudapestLocation() now := time.Now().In(loc) next := time.Date(now.Year(), now.Month(), now.Day(), hour, min, 0, 0, loc) - // If the time has already passed today, schedule for tomorrow + // If the time has already passed today, schedule for tomorrow. + // Use time.Date with day+1 instead of Add(24h) to correctly handle DST transitions + // (spring forward/fall back in Europe/Budapest would shift by 1 hour with Add(24h)). if !next.After(now) { - next = next.Add(24 * time.Hour) + next = time.Date(now.Year(), now.Month(), now.Day()+1, hour, min, 0, 0, loc) } return next diff --git a/controller/internal/settings/settings.go b/controller/internal/settings/settings.go index d896af3..ea592b0 100644 --- a/controller/internal/settings/settings.go +++ b/controller/internal/settings/settings.go @@ -136,6 +136,7 @@ func (s *Settings) save() error { } if err := os.WriteFile(tmpPath, data, 0644); err != nil { + os.Remove(tmpPath) // clean up partial file return fmt.Errorf("writing tmp settings: %w", err) } @@ -215,6 +216,9 @@ func (s *Settings) GetNotificationPrefs() *NotificationPrefs { // SetNotificationPrefs updates notification preferences and saves to disk. // H17: Deep-copies prefs so caller mutations after the call don't affect stored state. func (s *Settings) SetNotificationPrefs(prefs *NotificationPrefs) error { + if prefs == nil { + return fmt.Errorf("notification preferences cannot be nil") + } s.mu.Lock() defer s.mu.Unlock() copy := *prefs @@ -267,17 +271,18 @@ func (s *Settings) GetAppBackupMap() map[string]bool { } // SetAppBackupBulk updates backup prefs for all stacks at once and saves to disk. -// Preserves existing CrossDrive configs. +// Preserves existing CrossDrive configs and stacks not present in the input. func (s *Settings) SetAppBackupBulk(prefs map[string]bool) error { s.mu.Lock() defer s.mu.Unlock() - newMap := make(map[string]AppBackupPrefs, len(prefs)) + if s.AppBackup == nil { + s.AppBackup = make(map[string]AppBackupPrefs) + } for name, enabled := range prefs { existing := s.AppBackup[name] // preserves CrossDrive existing.Enabled = enabled - newMap[name] = existing + s.AppBackup[name] = existing } - s.AppBackup = newMap return s.save() } @@ -321,7 +326,8 @@ func (s *Settings) SetCrossDriveConfig(stackName string, cfg *CrossDriveBackup) } // UpdateCrossDriveStatus updates runtime status fields for a cross-drive backup in-place. -// fn receives a pointer to the CrossDriveBackup (creates one if nil) and may mutate it. +// fn receives a pointer to the CrossDriveBackup and may mutate it. +// If no cross-drive config exists for the stack, does nothing and returns nil. func (s *Settings) UpdateCrossDriveStatus(stackName string, fn func(*CrossDriveBackup)) error { s.mu.Lock() defer s.mu.Unlock() diff --git a/controller/internal/stacks/delete.go b/controller/internal/stacks/delete.go index d629fd4..06214b1 100644 --- a/controller/internal/stacks/delete.go +++ b/controller/internal/stacks/delete.go @@ -2,6 +2,7 @@ package stacks import ( "bufio" + "context" "fmt" "os" "os/exec" @@ -278,7 +279,9 @@ func getDirSizeHuman(path string) string { // getDirSizeBytes returns the total size in bytes for a directory. func getDirSizeBytes(path string) int64 { - cmd := exec.Command("du", "-sb", path) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "du", "-sb", path) output, err := cmd.Output() if err != nil { return 0 @@ -286,7 +289,9 @@ func getDirSizeBytes(path string) int64 { fields := strings.Fields(string(output)) if len(fields) > 0 { var size int64 - fmt.Sscanf(fields[0], "%d", &size) + if n, _ := fmt.Sscanf(fields[0], "%d", &size); n != 1 { + return 0 + } return size } return 0 diff --git a/controller/internal/stacks/deploy.go b/controller/internal/stacks/deploy.go index e59b1f0..87954ee 100644 --- a/controller/internal/stacks/deploy.go +++ b/controller/internal/stacks/deploy.go @@ -233,6 +233,10 @@ func (m *Manager) UpdateStackConfig(name string, values map[string]string) error return fmt.Errorf("stack %q is not deployed yet", name) } + if appCfg.Env == nil { + appCfg.Env = make(map[string]string) + } + lockedSet := make(map[string]bool) for _, f := range appCfg.LockedFields { lockedSet[f] = true diff --git a/controller/internal/stacks/manager.go b/controller/internal/stacks/manager.go index 5ecd1f6..cd5a030 100644 --- a/controller/internal/stacks/manager.go +++ b/controller/internal/stacks/manager.go @@ -628,10 +628,12 @@ func truncateStr(s string, maxLen int) string { // logPostStartStatus queries container states after a start/deploy operation // and logs them. This runs asynchronously to avoid blocking the HTTP response. func (m *Manager) logPostStartStatus(name, stackDir string, env []string) { + envCopy := make([]string, len(env)) + copy(envCopy, env) go func() { time.Sleep(3 * time.Second) - output, err := m.composeExecCustomEnv(stackDir, env, "ps", "-a", "--format", "table {{.Name}}\t{{.Image}}\t{{.State}}\t{{.Status}}") + output, err := m.composeExecCustomEnv(stackDir, envCopy, "ps", "-a", "--format", "table {{.Name}}\t{{.Image}}\t{{.State}}\t{{.Status}}") if err != nil { m.logger.Printf("[WARN] Post-start status check failed for %s: %v", name, err) return diff --git a/controller/internal/web/funcmap.go b/controller/internal/web/funcmap.go index 623848a..c1e8df3 100644 --- a/controller/internal/web/funcmap.go +++ b/controller/internal/web/funcmap.go @@ -4,18 +4,34 @@ import ( "fmt" "html/template" "strings" + "sync" "time" "gitea.dooplex.hu/admin/felhom-controller/internal/backup" "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" ) +var ( + webTimezone *time.Location + webTimezoneOnce sync.Once +) + +// getTimezone returns the Europe/Budapest timezone, cached after first load. +// Falls back to UTC if tzdata is unavailable. +func getTimezone() *time.Location { + webTimezoneOnce.Do(func() { + loc, err := time.LoadLocation("Europe/Budapest") + if err != nil { + loc = time.UTC + } + webTimezone = loc + }) + return webTimezone +} + // templateFuncMap returns the FuncMap used by all HTML templates. func (s *Server) templateFuncMap() template.FuncMap { - loc, err := time.LoadLocation("Europe/Budapest") - if err != nil { - loc = time.UTC - } + loc := getTimezone() return template.FuncMap{ "stateColor": func(state stacks.ContainerState) string { diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index f81cdf1..c84a0f1 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -447,12 +447,7 @@ func (s *Server) backupsHandler(w http.ResponseWriter, r *http.Request) { } if cfg.LastRun != "" { if t, err := time.Parse(time.RFC3339, cfg.LastRun); err == nil { - // M7: Handle LoadLocation error — fall back to UTC if tzdata missing. - loc, err := time.LoadLocation("Europe/Budapest") - if err != nil { - loc = time.UTC - } - item.LastRunShort = t.In(loc).Format("01-02 15:04") + item.LastRunShort = t.In(getTimezone()).Format("01-02 15:04") } } fullStatus.CrossDriveSummary = append(fullStatus.CrossDriveSummary, item) @@ -543,11 +538,7 @@ func (s *Server) buildAppBackupRows( crossConfigs map[string]*settings.CrossDriveBackup, destLabels map[string]string, ) []AppBackupRow { - // M7: Handle LoadLocation error — fall back to UTC if tzdata missing. - loc, err := time.LoadLocation("Europe/Budapest") - if err != nil { - loc = time.UTC - } + loc := getTimezone() // Build a quick lookup: which stacks have a DB dump? dbStacks := make(map[string]bool)