From c2ca6c0b73dcfc2534e223bc40653a8e40bb51b3 Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Mon, 8 Jun 2026 16:13:30 +0300 Subject: [PATCH] fix(deployer): wire pre-deploy backup into the unified dispatch path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit auto_backup_before_deploy silently did nothing — MaybeBackupBeforeDeploy's only caller was the legacy executeDeploy pipeline, removed in the workload-first cutover. Reconnect it as maybeBackupBeforeDeploy(), invoked from DispatchPlugin after the source resolves and before it runs, so the setting fires for every source kind. Fail-open: a nil backuper, a settings-load error, or a backup failure skips the snapshot without blocking the deploy. Adds predeploy_backup_test.go asserting the wiring. --- internal/deployer/deployer.go | 30 ++++-- internal/deployer/dispatch.go | 14 ++- internal/deployer/dispatch_test.go | 10 +- internal/deployer/predeploy_backup_test.go | 107 +++++++++++++++++++++ 4 files changed, 143 insertions(+), 18 deletions(-) create mode 100644 internal/deployer/predeploy_backup_test.go diff --git a/internal/deployer/deployer.go b/internal/deployer/deployer.go index d459262..dfe8957 100644 --- a/internal/deployer/deployer.go +++ b/internal/deployer/deployer.go @@ -100,20 +100,34 @@ func (d *Deployer) SetPreDeployBackuper(b PreDeployBackuper) { d.backuper = b } -// MaybeBackupBeforeDeploy creates a "pre-deploy" Tinyforge DB snapshot when -// the setting is enabled. Failures are logged but do not abort the deploy: -// missing a backup is preferable to refusing to ship a fix. Exposed so -// Source plugins can opt into the same behaviour. -func (d *Deployer) MaybeBackupBeforeDeploy(deployID string, settings store.Settings) { - if !settings.AutoBackupBeforeDeploy || d.backuper == nil { +// maybeBackupBeforeDeploy takes a "pre-deploy" Tinyforge DB snapshot before a +// deploy when the operator enabled auto_backup_before_deploy. It is called on +// the unified deploy path (DispatchPlugin) so the setting actually fires — its +// predecessor was orphaned when the legacy executeDeploy pipeline (its only +// caller) was removed in the workload-first cutover, silently disabling the +// setting. +// +// Fail-open: a nil backuper, a settings-load error, or a backup failure all +// skip the snapshot without blocking the deploy — missing a backup is +// preferable to refusing to ship a fix. +func (d *Deployer) maybeBackupBeforeDeploy(workloadID string) { + if d.backuper == nil { + return + } + settings, err := d.store.GetSettings() + if err != nil { + slog.Warn("pre-deploy backup: load settings", "workload", workloadID, "error", err) + return + } + if !settings.AutoBackupBeforeDeploy { return } backup, err := d.backuper.CreateBackup("pre-deploy") if err != nil { - slog.Warn("pre-deploy backup failed", "deploy_id", deployID, "error", err) + slog.Warn("pre-deploy backup failed", "workload", workloadID, "error", err) return } - slog.Info("pre-deploy backup created", "deploy_id", deployID, "backup_id", backup.ID, "filename", backup.Filename) + slog.Info("pre-deploy backup created", "workload", workloadID, "backup_id", backup.ID, "filename", backup.Filename) } // SetDNSProvider sets the DNS provider for managing DNS records during deployments. diff --git a/internal/deployer/dispatch.go b/internal/deployer/dispatch.go index 2b8b4f6..78689fc 100644 --- a/internal/deployer/dispatch.go +++ b/internal/deployer/dispatch.go @@ -9,11 +9,10 @@ import ( ) // DispatchPlugin routes a DeploymentIntent for w to the matching Source -// plugin. This is the new unified deploy path; the legacy executeDeploy -// remains in place until Phase 6 ports image-deploy logic into -// source/image. While both exist, callers must pick: webhook/registry -// triggers + image deploys still go through the legacy path, while -// /api/hooks/generic + the unified webhook ingress go through here. +// plugin. This is the unified deploy path for every source kind (the legacy +// executeDeploy pipeline was removed in the workload-first cutover). When the +// operator enables auto_backup_before_deploy, a pre-deploy Tinyforge DB +// snapshot is taken here, after the source resolves and before it runs. func (d *Deployer) DispatchPlugin(ctx context.Context, w plugin.Workload, intent plugin.DeploymentIntent) error { if err := d.beginDispatch(); err != nil { metrics.DeploysTotal.Inc(w.SourceKind, "rejected_draining") @@ -29,6 +28,11 @@ func (d *Deployer) DispatchPlugin(ctx context.Context, w plugin.Workload, intent metrics.DeploysTotal.Inc("unknown", "unknown_source") return fmt.Errorf("dispatch %s: %w", w.Name, err) } + // Optional operator-enabled pre-deploy DB snapshot. Fail-open: never + // blocks shipping a deploy. Runs before any source-internal idempotency + // check (e.g. the image source's same-tag short-circuit), so a same-tag + // redeploy still snapshots — "backup before every deploy attempt". + d.maybeBackupBeforeDeploy(w.ID) err = src.Deploy(ctx, d.PluginDeps(), w, intent) outcome := "success" if err != nil { diff --git a/internal/deployer/dispatch_test.go b/internal/deployer/dispatch_test.go index af6755a..4b4a7f1 100644 --- a/internal/deployer/dispatch_test.go +++ b/internal/deployer/dispatch_test.go @@ -21,9 +21,9 @@ import ( type fakeSource struct { kind string - mu sync.Mutex - deployErr error - teardownErr error + mu sync.Mutex + deployErr error + teardownErr error reconcileErr error deployCount atomic.Int32 @@ -34,8 +34,8 @@ type fakeSource struct { lastDeps plugin.Deps } -func (f *fakeSource) Kind() string { return f.kind } -func (f *fakeSource) SchemaSample() any { return struct{}{} } +func (f *fakeSource) Kind() string { return f.kind } +func (f *fakeSource) SchemaSample() any { return struct{}{} } func (f *fakeSource) Validate(json.RawMessage) error { return nil } func (f *fakeSource) Deploy(_ context.Context, deps plugin.Deps, _ plugin.Workload, intent plugin.DeploymentIntent) error { diff --git a/internal/deployer/predeploy_backup_test.go b/internal/deployer/predeploy_backup_test.go new file mode 100644 index 0000000..e230b08 --- /dev/null +++ b/internal/deployer/predeploy_backup_test.go @@ -0,0 +1,107 @@ +package deployer + +import ( + "context" + "errors" + "sync/atomic" + "testing" + + "github.com/alexei/tinyforge/internal/store" + "github.com/alexei/tinyforge/internal/workload/plugin" +) + +// fakeBackuper records pre-deploy backup calls so the dispatch wiring can be +// asserted. err (when set) simulates a backup failure. +type fakeBackuper struct { + count atomic.Int32 + lastType atomic.Value // string + err error +} + +func (f *fakeBackuper) CreateBackup(backupType string) (store.Backup, error) { + f.count.Add(1) + f.lastType.Store(backupType) + if f.err != nil { + return store.Backup{}, f.err + } + return store.Backup{ID: "b1", Filename: "tinyforge-pre-deploy.db"}, nil +} + +func setAutoBackup(t *testing.T, d *Deployer, enabled bool) { + t.Helper() + s, err := d.store.GetSettings() + if err != nil { + t.Fatalf("get settings: %v", err) + } + s.AutoBackupBeforeDeploy = enabled + if err := d.store.UpdateSettings(s); err != nil { + t.Fatalf("update settings: %v", err) + } +} + +// Regression: the pre-deploy backup hook was orphaned after the cutover (no +// caller on DispatchPlugin), making auto_backup_before_deploy a silent no-op. +func TestDispatchPlugin_PreDeployBackup_FiresWhenEnabled(t *testing.T) { + resetFake(t) + d := newTestDeployer(t) + b := &fakeBackuper{} + d.SetPreDeployBackuper(b) + setAutoBackup(t, d, true) + + if err := d.DispatchPlugin(context.Background(), sampleWorkload(), plugin.DeploymentIntent{}); err != nil { + t.Fatalf("dispatch: %v", err) + } + if got := b.count.Load(); got != 1 { + t.Fatalf("CreateBackup called %d times, want 1", got) + } + if bt, _ := b.lastType.Load().(string); bt != "pre-deploy" { + t.Fatalf("backup type = %q, want pre-deploy", bt) + } + if got := dispatchTestSource.deployCount.Load(); got != 1 { + t.Fatalf("Deploy ran %d times, want 1", got) + } +} + +func TestDispatchPlugin_PreDeployBackup_SkippedWhenDisabled(t *testing.T) { + resetFake(t) + d := newTestDeployer(t) + b := &fakeBackuper{} + d.SetPreDeployBackuper(b) + setAutoBackup(t, d, false) + + if err := d.DispatchPlugin(context.Background(), sampleWorkload(), plugin.DeploymentIntent{}); err != nil { + t.Fatalf("dispatch: %v", err) + } + if got := b.count.Load(); got != 0 { + t.Fatalf("CreateBackup called %d times, want 0 (setting off)", got) + } +} + +func TestDispatchPlugin_PreDeployBackup_NilBackuperNoPanic(t *testing.T) { + resetFake(t) + d := newTestDeployer(t) + setAutoBackup(t, d, true) // enabled, but no backuper wired + + if err := d.DispatchPlugin(context.Background(), sampleWorkload(), plugin.DeploymentIntent{}); err != nil { + t.Fatalf("dispatch must not panic/fail with a nil backuper: %v", err) + } + if got := dispatchTestSource.deployCount.Load(); got != 1 { + t.Fatalf("Deploy ran %d times, want 1", got) + } +} + +func TestDispatchPlugin_PreDeployBackup_FailOpen(t *testing.T) { + resetFake(t) + d := newTestDeployer(t) + b := &fakeBackuper{err: errors.New("disk full")} + d.SetPreDeployBackuper(b) + setAutoBackup(t, d, true) + + // A failed backup is logged but must NOT block the deploy. + if err := d.DispatchPlugin(context.Background(), sampleWorkload(), plugin.DeploymentIntent{}); err != nil { + t.Fatalf("deploy must succeed when backup fails (fail-open): %v", err) + } + if got := dispatchTestSource.deployCount.Load(); got != 1 { + t.Fatalf("Deploy ran %d times, want 1 (despite backup failure)", got) + } +}