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)
This commit is contained in:
+20
-7
@@ -224,10 +224,20 @@ func main() {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Schedule autobackup if enabled.
|
// Schedule autobackup if enabled. Track entry ID for rescheduling.
|
||||||
if settings.BackupEnabled && settings.BackupIntervalHours > 0 {
|
var backupCronID cron.EntryID
|
||||||
interval := fmt.Sprintf("@every %dh", settings.BackupIntervalHours)
|
scheduleAutobackup := func(enabled bool, intervalHours int) {
|
||||||
if _, err := cronScheduler.AddFunc(interval, func() {
|
// 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")
|
b, err := backupEngine.CreateBackup("auto")
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Error("autobackup failed", "error", err)
|
slog.Error("autobackup failed", "error", err)
|
||||||
@@ -235,17 +245,19 @@ func main() {
|
|||||||
}
|
}
|
||||||
slog.Info("autobackup completed", "id", b.ID, "filename", b.Filename)
|
slog.Info("autobackup completed", "id", b.ID, "filename", b.Filename)
|
||||||
|
|
||||||
// Prune after auto backup.
|
|
||||||
currentSettings, err := db.GetSettings()
|
currentSettings, err := db.GetSettings()
|
||||||
if err == nil && currentSettings.BackupRetentionCount > 0 {
|
if err == nil && currentSettings.BackupRetentionCount > 0 {
|
||||||
backupEngine.Prune(currentSettings.BackupRetentionCount)
|
backupEngine.Prune(currentSettings.BackupRetentionCount)
|
||||||
}
|
}
|
||||||
}); err != nil {
|
})
|
||||||
|
if err != nil {
|
||||||
slog.Warn("failed to schedule autobackup", "error", err)
|
slog.Warn("failed to schedule autobackup", "error", err)
|
||||||
} else {
|
} 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.
|
// Build API server.
|
||||||
apiServer := api.NewServer(db, dockerClient, npmClient, dep, webhookHandler, eventBus, encKey)
|
apiServer := api.NewServer(db, dockerClient, npmClient, dep, webhookHandler, eventBus, encKey)
|
||||||
@@ -253,6 +265,7 @@ func main() {
|
|||||||
apiServer.SetProxyManager(proxyManager)
|
apiServer.SetProxyManager(proxyManager)
|
||||||
apiServer.SetBackupEngine(backupEngine)
|
apiServer.SetBackupEngine(backupEngine)
|
||||||
apiServer.SetDBPath(dbPath)
|
apiServer.SetDBPath(dbPath)
|
||||||
|
apiServer.SetBackupSettingsChangedCallback(scheduleAutobackup)
|
||||||
apiServer.SetDNSProvider(dnsProvider)
|
apiServer.SetDNSProvider(dnsProvider)
|
||||||
apiServer.SetDNSProviderChangedCallback(func(provider dns.Provider) {
|
apiServer.SetDNSProviderChangedCallback(func(provider dns.Provider) {
|
||||||
dep.SetDNSProvider(provider)
|
dep.SetDNSProvider(provider)
|
||||||
|
|||||||
+56
-27
@@ -1,9 +1,12 @@
|
|||||||
package api
|
package api
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"io"
|
||||||
|
"log/slog"
|
||||||
"net/http"
|
"net/http"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"time"
|
||||||
|
|
||||||
"github.com/alexei/docker-watcher/internal/store"
|
"github.com/alexei/docker-watcher/internal/store"
|
||||||
"github.com/go-chi/chi/v5"
|
"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.
|
// 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) {
|
func (s *Server) restoreBackup(w http.ResponseWriter, r *http.Request) {
|
||||||
if s.backupEngine == nil {
|
if s.backupEngine == nil {
|
||||||
respondError(w, http.StatusServiceUnavailable, "backup engine not initialized")
|
respondError(w, http.StatusServiceUnavailable, "backup engine not initialized")
|
||||||
@@ -107,36 +110,62 @@ func (s *Server) restoreBackup(w http.ResponseWriter, r *http.Request) {
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
// Read the backup file.
|
// Create a safety backup before restore so the user can undo if needed.
|
||||||
backupData, err := os.ReadFile(restorePath)
|
if _, err := s.backupEngine.CreateBackup("pre-restore"); err != nil {
|
||||||
if err != nil {
|
slog.Warn("failed to create pre-restore backup", "error", err)
|
||||||
respondError(w, http.StatusInternalServerError, "failed to read backup file: "+err.Error())
|
|
||||||
return
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// Close the current database to release locks.
|
// Send the response BEFORE closing the DB so the client gets confirmation.
|
||||||
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")
|
|
||||||
|
|
||||||
respondJSON(w, http.StatusOK, map[string]any{
|
respondJSON(w, http.StatusOK, map[string]any{
|
||||||
"status": "restored",
|
"status": "restoring",
|
||||||
"message": "Database restored. The server needs to be restarted to apply changes.",
|
"message": "Database restore initiated. The server will restart shortly.",
|
||||||
})
|
})
|
||||||
|
|
||||||
// Signal the server to shut down gracefully so it can be restarted.
|
// Flush the response.
|
||||||
if s.shutdownFunc != nil {
|
if f, ok := w.(http.Flusher); ok {
|
||||||
go s.shutdownFunc()
|
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()
|
||||||
|
}
|
||||||
|
}()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -42,9 +42,10 @@ type Server struct {
|
|||||||
dnsProvider dns.Provider
|
dnsProvider dns.Provider
|
||||||
onDNSProviderChanged DNSProviderChangedFunc
|
onDNSProviderChanged DNSProviderChangedFunc
|
||||||
|
|
||||||
backupEngine *backup.Engine
|
backupEngine *backup.Engine
|
||||||
dbPath string
|
dbPath string
|
||||||
shutdownFunc func() // called after restore to trigger graceful shutdown
|
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.
|
// NewServer creates a new API Server with all required dependencies.
|
||||||
@@ -106,6 +107,11 @@ func (s *Server) SetShutdownFunc(fn func()) {
|
|||||||
s.shutdownFunc = fn
|
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.
|
// SetDNSProvider sets the current DNS provider on the server.
|
||||||
func (s *Server) SetDNSProvider(provider dns.Provider) {
|
func (s *Server) SetDNSProvider(provider dns.Provider) {
|
||||||
s.dnsProviderMu.Lock()
|
s.dnsProviderMu.Lock()
|
||||||
|
|||||||
@@ -205,6 +205,13 @@ func (s *Server) updateSettings(w http.ResponseWriter, r *http.Request) {
|
|||||||
go s.handleDNSSettingsChange(oldProvider, existing, updated)
|
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"})
|
respondJSON(w, http.StatusOK, map[string]string{"status": "updated"})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import (
|
|||||||
"log/slog"
|
"log/slog"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"sync"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
"github.com/alexei/docker-watcher/internal/store"
|
"github.com/alexei/docker-watcher/internal/store"
|
||||||
@@ -12,6 +13,7 @@ import (
|
|||||||
|
|
||||||
// Engine manages database backup operations.
|
// Engine manages database backup operations.
|
||||||
type Engine struct {
|
type Engine struct {
|
||||||
|
mu sync.Mutex
|
||||||
store *store.Store
|
store *store.Store
|
||||||
dbPath string
|
dbPath string
|
||||||
backupDir string
|
backupDir string
|
||||||
@@ -38,6 +40,9 @@ func (e *Engine) BackupDir() string {
|
|||||||
// CreateBackup creates a new database backup using VACUUM INTO.
|
// CreateBackup creates a new database backup using VACUUM INTO.
|
||||||
// Returns the backup metadata record.
|
// Returns the backup metadata record.
|
||||||
func (e *Engine) CreateBackup(backupType string) (store.Backup, error) {
|
func (e *Engine) CreateBackup(backupType string) (store.Backup, error) {
|
||||||
|
e.mu.Lock()
|
||||||
|
defer e.mu.Unlock()
|
||||||
|
|
||||||
timestamp := time.Now().UTC().Format("20060102-150405")
|
timestamp := time.Now().UTC().Format("20060102-150405")
|
||||||
filename := fmt.Sprintf("docker-watcher-%s-%s.db", backupType, timestamp)
|
filename := fmt.Sprintf("docker-watcher-%s-%s.db", backupType, timestamp)
|
||||||
destPath := filepath.Join(e.backupDir, filename)
|
destPath := filepath.Join(e.backupDir, filename)
|
||||||
|
|||||||
Reference in New Issue
Block a user