From 3c9727162a15aa2af0da8e1087e7e816a4ce147d Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Thu, 2 Apr 2026 15:39:54 +0300 Subject: [PATCH] fix: address review findings for backup management - HIGH: Add sync.Mutex to backup Engine to prevent concurrent backup/restore operations - HIGH: Restore uses io.Copy instead of ReadFile to avoid OOM on large databases - HIGH: Send HTTP response before closing DB during restore, then perform destructive operations in a goroutine - HIGH: Create pre-restore safety backup before overwriting database - HIGH: Autobackup cron reschedules dynamically when settings change via callback pattern (same as DNS provider changes) --- cmd/server/main.go | 27 +++++++++---- internal/api/backups.go | 83 ++++++++++++++++++++++++++------------- internal/api/router.go | 12 ++++-- internal/api/settings.go | 7 ++++ internal/backup/engine.go | 5 +++ 5 files changed, 97 insertions(+), 37 deletions(-) diff --git a/cmd/server/main.go b/cmd/server/main.go index 717d179..9064ef2 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -224,10 +224,20 @@ func main() { } } - // Schedule autobackup if enabled. - if settings.BackupEnabled && settings.BackupIntervalHours > 0 { - interval := fmt.Sprintf("@every %dh", settings.BackupIntervalHours) - if _, err := cronScheduler.AddFunc(interval, func() { + // Schedule autobackup if enabled. Track entry ID for rescheduling. + var backupCronID cron.EntryID + scheduleAutobackup := func(enabled bool, intervalHours int) { + // Remove existing schedule if any. + if backupCronID != 0 { + cronScheduler.Remove(backupCronID) + backupCronID = 0 + slog.Info("autobackup: removed previous schedule") + } + if !enabled || intervalHours <= 0 { + return + } + interval := fmt.Sprintf("@every %dh", intervalHours) + id, err := cronScheduler.AddFunc(interval, func() { b, err := backupEngine.CreateBackup("auto") if err != nil { slog.Error("autobackup failed", "error", err) @@ -235,17 +245,19 @@ func main() { } slog.Info("autobackup completed", "id", b.ID, "filename", b.Filename) - // Prune after auto backup. currentSettings, err := db.GetSettings() if err == nil && currentSettings.BackupRetentionCount > 0 { backupEngine.Prune(currentSettings.BackupRetentionCount) } - }); err != nil { + }) + if err != nil { slog.Warn("failed to schedule autobackup", "error", err) } else { - slog.Info("autobackup scheduled", "interval_hours", settings.BackupIntervalHours) + backupCronID = id + slog.Info("autobackup scheduled", "interval_hours", intervalHours) } } + scheduleAutobackup(settings.BackupEnabled, settings.BackupIntervalHours) // Build API server. apiServer := api.NewServer(db, dockerClient, npmClient, dep, webhookHandler, eventBus, encKey) @@ -253,6 +265,7 @@ func main() { apiServer.SetProxyManager(proxyManager) apiServer.SetBackupEngine(backupEngine) apiServer.SetDBPath(dbPath) + apiServer.SetBackupSettingsChangedCallback(scheduleAutobackup) apiServer.SetDNSProvider(dnsProvider) apiServer.SetDNSProviderChangedCallback(func(provider dns.Provider) { dep.SetDNSProvider(provider) diff --git a/internal/api/backups.go b/internal/api/backups.go index 344bc8c..88080eb 100644 --- a/internal/api/backups.go +++ b/internal/api/backups.go @@ -1,9 +1,12 @@ package api import ( + "io" + "log/slog" "net/http" "os" "path/filepath" + "time" "github.com/alexei/docker-watcher/internal/store" "github.com/go-chi/chi/v5" @@ -93,7 +96,7 @@ func (s *Server) deleteBackup(w http.ResponseWriter, r *http.Request) { } // restoreBackup handles POST /api/backups/{id}/restore. -// This replaces the current database with the backup. The server should be restarted after. +// This replaces the current database with the backup and triggers a graceful shutdown. func (s *Server) restoreBackup(w http.ResponseWriter, r *http.Request) { if s.backupEngine == nil { respondError(w, http.StatusServiceUnavailable, "backup engine not initialized") @@ -107,36 +110,62 @@ func (s *Server) restoreBackup(w http.ResponseWriter, r *http.Request) { return } - // Read the backup file. - backupData, err := os.ReadFile(restorePath) - if err != nil { - respondError(w, http.StatusInternalServerError, "failed to read backup file: "+err.Error()) - return + // Create a safety backup before restore so the user can undo if needed. + if _, err := s.backupEngine.CreateBackup("pre-restore"); err != nil { + slog.Warn("failed to create pre-restore backup", "error", err) } - // Close the current database to release locks. - if err := s.store.Close(); err != nil { - respondError(w, http.StatusInternalServerError, "failed to close database: "+err.Error()) - return - } - - // Write backup over the main database file. - if err := os.WriteFile(s.dbPath, backupData, 0o644); err != nil { - respondError(w, http.StatusInternalServerError, "failed to write database: "+err.Error()) - return - } - - // Remove WAL and SHM files to ensure clean state. - os.Remove(s.dbPath + "-wal") - os.Remove(s.dbPath + "-shm") - + // Send the response BEFORE closing the DB so the client gets confirmation. respondJSON(w, http.StatusOK, map[string]any{ - "status": "restored", - "message": "Database restored. The server needs to be restarted to apply changes.", + "status": "restoring", + "message": "Database restore initiated. The server will restart shortly.", }) - // Signal the server to shut down gracefully so it can be restarted. - if s.shutdownFunc != nil { - go s.shutdownFunc() + // Flush the response. + if f, ok := w.(http.Flusher); ok { + f.Flush() } + + // Perform the destructive restore in a goroutine with a brief delay + // to allow the HTTP response to be fully sent. + go func() { + time.Sleep(500 * time.Millisecond) + + // Close the current database to release locks. + if err := s.store.Close(); err != nil { + slog.Error("restore: failed to close database", "error", err) + return + } + + // Copy the backup file over the main database using streaming (no full read into memory). + src, err := os.Open(restorePath) + if err != nil { + slog.Error("restore: failed to open backup file", "error", err) + return + } + defer src.Close() + + dst, err := os.Create(s.dbPath) + if err != nil { + slog.Error("restore: failed to create database file", "error", err) + return + } + defer dst.Close() + + if _, err := io.Copy(dst, src); err != nil { + slog.Error("restore: failed to copy backup to database", "error", err) + return + } + + // Remove WAL and SHM files to ensure clean state. + os.Remove(s.dbPath + "-wal") + os.Remove(s.dbPath + "-shm") + + slog.Info("restore: database replaced, triggering shutdown") + + // Signal the server to shut down gracefully so it can be restarted. + if s.shutdownFunc != nil { + s.shutdownFunc() + } + }() } diff --git a/internal/api/router.go b/internal/api/router.go index 82dfe82..7b24597 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -42,9 +42,10 @@ type Server struct { dnsProvider dns.Provider onDNSProviderChanged DNSProviderChangedFunc - backupEngine *backup.Engine - dbPath string - shutdownFunc func() // called after restore to trigger graceful shutdown + backupEngine *backup.Engine + dbPath string + shutdownFunc func() // called after restore to trigger graceful shutdown + onBackupSettingsChanged func(enabled bool, intervalHours int) // called when backup settings change } // NewServer creates a new API Server with all required dependencies. @@ -106,6 +107,11 @@ func (s *Server) SetShutdownFunc(fn func()) { s.shutdownFunc = fn } +// SetBackupSettingsChangedCallback sets the callback for when backup settings change. +func (s *Server) SetBackupSettingsChangedCallback(fn func(enabled bool, intervalHours int)) { + s.onBackupSettingsChanged = fn +} + // SetDNSProvider sets the current DNS provider on the server. func (s *Server) SetDNSProvider(provider dns.Provider) { s.dnsProviderMu.Lock() diff --git a/internal/api/settings.go b/internal/api/settings.go index 5ee133f..e67491f 100644 --- a/internal/api/settings.go +++ b/internal/api/settings.go @@ -205,6 +205,13 @@ func (s *Server) updateSettings(w http.ResponseWriter, r *http.Request) { go s.handleDNSSettingsChange(oldProvider, existing, updated) } + // Handle backup settings changes. + backupChanged := existing.BackupEnabled != updated.BackupEnabled || + existing.BackupIntervalHours != updated.BackupIntervalHours + if backupChanged && s.onBackupSettingsChanged != nil { + s.onBackupSettingsChanged(updated.BackupEnabled, updated.BackupIntervalHours) + } + respondJSON(w, http.StatusOK, map[string]string{"status": "updated"}) } diff --git a/internal/backup/engine.go b/internal/backup/engine.go index 8597918..3421748 100644 --- a/internal/backup/engine.go +++ b/internal/backup/engine.go @@ -5,6 +5,7 @@ import ( "log/slog" "os" "path/filepath" + "sync" "time" "github.com/alexei/docker-watcher/internal/store" @@ -12,6 +13,7 @@ import ( // Engine manages database backup operations. type Engine struct { + mu sync.Mutex store *store.Store dbPath string backupDir string @@ -38,6 +40,9 @@ func (e *Engine) BackupDir() string { // CreateBackup creates a new database backup using VACUUM INTO. // Returns the backup metadata record. func (e *Engine) CreateBackup(backupType string) (store.Backup, error) { + e.mu.Lock() + defer e.mu.Unlock() + timestamp := time.Now().UTC().Format("20060102-150405") filename := fmt.Sprintf("docker-watcher-%s-%s.db", backupType, timestamp) destPath := filepath.Join(e.backupDir, filename)