fix(backup): ensure cleanup on dump failures and skip canceled start
- commit
- 6ee3fd8de31701ae4c366efa7588381ecb7ec970
- author
- brice · · 2026-02-07 17:00 UTC
- parent
- 63aabba
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +59 -2
M internal/usecase/backup/service.go +1 -2
| @@ -85,6 +85,7 @@ func (s *Service) RunBackup(ctx context.Context, domainName, dbName string) (*do | |||
| 85 | 85 | execCtx, cancelExec := context.WithTimeout(ctx, backupExecTimeout) | |
| 86 | 86 | defer cancelExec() | |
| 87 | 87 | dumpPath := fmt.Sprintf("/tmp/gordon-backup-%d.bak", started.UnixNano()) | |
| 88 | + | defer s.cleanupDumpFile(db.ContainerID, dumpPath) | |
| 88 | 89 | ||
| 89 | 90 | execResult, err := s.runtime.ExecInContainer(execCtx, db.ContainerID, []string{"sh", "-c", pgDumpToPathCommand(dumpPath, db.Name)}) | |
| 90 | 91 | if err != nil { | |
| @@ -97,8 +98,6 @@ func (s *Service) RunBackup(ctx context.Context, domainName, dbName string) (*do | |||
| 97 | 98 | return nil, fmt.Errorf("pg_dump failed with exit code %d: %s", execResult.ExitCode, string(execResult.Stderr)) | |
| 98 | 99 | } | |
| 99 | 100 | ||
| 100 | - | defer s.cleanupDumpFile(db.ContainerID, dumpPath) | |
| 101 | - | ||
| 102 | 101 | dumpStream, err := s.runtime.CopyFromContainer(execCtx, db.ContainerID, dumpPath) | |
| 103 | 102 | if err != nil { | |
| 104 | 103 | if errors.Is(execCtx.Err(), context.DeadlineExceeded) { | |
| @@ -85,6 +85,7 @@ func (s *Service) RunBackup(ctx context.Context, domainName, dbName string) (*do | |||||
| 85 | execCtx, cancelExec := context.WithTimeout(ctx, backupExecTimeout) | 85 | execCtx, cancelExec := context.WithTimeout(ctx, backupExecTimeout) | ||
| 86 | defer cancelExec() | 86 | defer cancelExec() | ||
| 87 | dumpPath := fmt.Sprintf("/tmp/gordon-backup-%d.bak", started.UnixNano()) | 87 | dumpPath := fmt.Sprintf("/tmp/gordon-backup-%d.bak", started.UnixNano()) | ||
| 88 | + | defer s.cleanupDumpFile(db.ContainerID, dumpPath) | |||
| 88 | 89 | ||||
| 89 | execResult, err := s.runtime.ExecInContainer(execCtx, db.ContainerID, []string{"sh", "-c", pgDumpToPathCommand(dumpPath, db.Name)}) | 90 | execResult, err := s.runtime.ExecInContainer(execCtx, db.ContainerID, []string{"sh", "-c", pgDumpToPathCommand(dumpPath, db.Name)}) | ||
| 90 | if err != nil { | 91 | if err != nil { | ||
| @@ -97,8 +98,6 @@ func (s *Service) RunBackup(ctx context.Context, domainName, dbName string) (*do | |||||
| 97 | return nil, fmt.Errorf("pg_dump failed with exit code %d: %s", execResult.ExitCode, string(execResult.Stderr)) | 98 | return nil, fmt.Errorf("pg_dump failed with exit code %d: %s", execResult.ExitCode, string(execResult.Stderr)) | ||
| 98 | } | 99 | } | ||
| 99 | 100 | ||||
| 100 | - | defer s.cleanupDumpFile(db.ContainerID, dumpPath) | |||
| 101 | - | ||||
| 102 | dumpStream, err := s.runtime.CopyFromContainer(execCtx, db.ContainerID, dumpPath) | 101 | dumpStream, err := s.runtime.CopyFromContainer(execCtx, db.ContainerID, dumpPath) | ||
| 103 | if err != nil { | 102 | if err != nil { | ||
| 104 | if errors.Is(execCtx.Err(), context.DeadlineExceeded) { | 103 | if errors.Is(execCtx.Err(), context.DeadlineExceeded) { | ||
M internal/usecase/backup/service_test.go +25 -0
| @@ -84,6 +84,31 @@ func TestService_RunBackup_Postgres(t *testing.T) { | |||
| 84 | 84 | assert.Equal(t, int64(len("backup-data")), result.Job.SizeBytes) | |
| 85 | 85 | } | |
| 86 | 86 | ||
| 87 | + | func TestService_RunBackup_CleansUpDumpWhenPgDumpFails(t *testing.T) { | |
| 88 | + | runtime := outmocks.NewMockContainerRuntime(t) | |
| 89 | + | storage := outmocks.NewMockBackupStorage(t) | |
| 90 | + | containerSvc := inmocks.NewMockContainerService(t) | |
| 91 | + | ||
| 92 | + | containerSvc.EXPECT().ListAttachments(mock.Anything, "app.example.com").Return([]domain.Attachment{ | |
| 93 | + | {Name: "postgres", Image: "postgres:17", ContainerID: "db123", Status: "running"}, | |
| 94 | + | }) | |
| 95 | + | ||
| 96 | + | runtime.EXPECT().ExecInContainer(mock.Anything, "db123", mock.MatchedBy(func(cmd []string) bool { | |
| 97 | + | return len(cmd) == 3 && cmd[0] == "sh" && cmd[1] == "-c" && bytes.Contains([]byte(cmd[2]), []byte("pg_dump -Fc")) | |
| 98 | + | })).Return(&outiface.ExecResult{ExitCode: 2, Stderr: []byte("dump failed")}, nil) | |
| 99 | + | ||
| 100 | + | runtime.EXPECT().ExecInContainer(mock.Anything, "db123", mock.MatchedBy(func(cmd []string) bool { | |
| 101 | + | return len(cmd) == 3 && cmd[0] == "sh" && cmd[1] == "-c" && bytes.Contains([]byte(cmd[2]), []byte("rm -f")) | |
| 102 | + | })).Return(&outiface.ExecResult{ExitCode: 0}, nil) | |
| 103 | + | ||
| 104 | + | svc := NewService(runtime, storage, containerSvc, domain.BackupConfig{}, zerowrap.Default()) | |
| 105 | + | ||
| 106 | + | result, err := svc.RunBackup(context.Background(), "app.example.com", "postgres") | |
| 107 | + | require.Error(t, err) | |
| 108 | + | assert.Nil(t, result) | |
| 109 | + | assert.Contains(t, err.Error(), "pg_dump failed") | |
| 110 | + | } | |
| 111 | + | ||
| 87 | 112 | func TestSelectDatabaseRequiresExplicitNameWhenMultipleDetected(t *testing.T) { | |
| 88 | 113 | db, err := selectDatabase([]domain.DBInfo{ | |
| 89 | 114 | {Name: "postgres"}, | |
| @@ -84,6 +84,31 @@ func TestService_RunBackup_Postgres(t *testing.T) { | |||||
| 84 | assert.Equal(t, int64(len("backup-data")), result.Job.SizeBytes) | 84 | assert.Equal(t, int64(len("backup-data")), result.Job.SizeBytes) | ||
| 85 | } | 85 | } | ||
| 86 | 86 | ||||
| 87 | + | func TestService_RunBackup_CleansUpDumpWhenPgDumpFails(t *testing.T) { | |||
| 88 | + | runtime := outmocks.NewMockContainerRuntime(t) | |||
| 89 | + | storage := outmocks.NewMockBackupStorage(t) | |||
| 90 | + | containerSvc := inmocks.NewMockContainerService(t) | |||
| 91 | + | ||||
| 92 | + | containerSvc.EXPECT().ListAttachments(mock.Anything, "app.example.com").Return([]domain.Attachment{ | |||
| 93 | + | {Name: "postgres", Image: "postgres:17", ContainerID: "db123", Status: "running"}, | |||
| 94 | + | }) | |||
| 95 | + | ||||
| 96 | + | runtime.EXPECT().ExecInContainer(mock.Anything, "db123", mock.MatchedBy(func(cmd []string) bool { | |||
| 97 | + | return len(cmd) == 3 && cmd[0] == "sh" && cmd[1] == "-c" && bytes.Contains([]byte(cmd[2]), []byte("pg_dump -Fc")) | |||
| 98 | + | })).Return(&outiface.ExecResult{ExitCode: 2, Stderr: []byte("dump failed")}, nil) | |||
| 99 | + | ||||
| 100 | + | runtime.EXPECT().ExecInContainer(mock.Anything, "db123", mock.MatchedBy(func(cmd []string) bool { | |||
| 101 | + | return len(cmd) == 3 && cmd[0] == "sh" && cmd[1] == "-c" && bytes.Contains([]byte(cmd[2]), []byte("rm -f")) | |||
| 102 | + | })).Return(&outiface.ExecResult{ExitCode: 0}, nil) | |||
| 103 | + | ||||
| 104 | + | svc := NewService(runtime, storage, containerSvc, domain.BackupConfig{}, zerowrap.Default()) | |||
| 105 | + | ||||
| 106 | + | result, err := svc.RunBackup(context.Background(), "app.example.com", "postgres") | |||
| 107 | + | require.Error(t, err) | |||
| 108 | + | assert.Nil(t, result) | |||
| 109 | + | assert.Contains(t, err.Error(), "pg_dump failed") | |||
| 110 | + | } | |||
| 111 | + | ||||
| 87 | func TestSelectDatabaseRequiresExplicitNameWhenMultipleDetected(t *testing.T) { | 112 | func TestSelectDatabaseRequiresExplicitNameWhenMultipleDetected(t *testing.T) { | ||
| 88 | db, err := selectDatabase([]domain.DBInfo{ | 113 | db, err := selectDatabase([]domain.DBInfo{ | ||
| 89 | {Name: "postgres"}, | 114 | {Name: "postgres"}, | ||
M internal/usecase/cron/scheduler.go +7 -0
| @@ -128,6 +128,13 @@ func (s *Scheduler) Start(ctx context.Context) { | |||
| 128 | 128 | default: | |
| 129 | 129 | } | |
| 130 | 130 | ||
| 131 | + | select { | |
| 132 | + | case <-ctx.Done(): | |
| 133 | + | s.started.Store(false) | |
| 134 | + | return | |
| 135 | + | default: | |
| 136 | + | } | |
| 137 | + | ||
| 131 | 138 | ticker := time.NewTicker(time.Minute) | |
| 132 | 139 | go func() { | |
| 133 | 140 | defer ticker.Stop() | |
| @@ -128,6 +128,13 @@ func (s *Scheduler) Start(ctx context.Context) { | |||||
| 128 | default: | 128 | default: | ||
| 129 | } | 129 | } | ||
| 130 | 130 | ||||
| 131 | + | select { | |||
| 132 | + | case <-ctx.Done(): | |||
| 133 | + | s.started.Store(false) | |||
| 134 | + | return | |||
| 135 | + | default: | |||
| 136 | + | } | |||
| 137 | + | ||||
| 131 | ticker := time.NewTicker(time.Minute) | 138 | ticker := time.NewTicker(time.Minute) | ||
| 132 | go func() { | 139 | go func() { | ||
| 133 | defer ticker.Stop() | 140 | defer ticker.Stop() | ||
M internal/usecase/cron/scheduler_test.go +26 -0
| @@ -199,3 +199,29 @@ func TestSchedulerRunNowComputesNextRunFromJobFinishTime(t *testing.T) { | |||
| 199 | 199 | require.Len(t, entries, 1) | |
| 200 | 200 | assert.Equal(t, time.Date(2026, 2, 7, 14, 0, 0, 0, time.UTC), entries[0].NextRun) | |
| 201 | 201 | } | |
| 202 | + | ||
| 203 | + | func TestSchedulerStartNoOpWhenContextAlreadyCanceled(t *testing.T) { | |
| 204 | + | s := NewScheduler(zerowrap.Default()) | |
| 205 | + | now := time.Date(2026, 2, 7, 12, 0, 0, 0, time.UTC) | |
| 206 | + | s.nowFn = func() time.Time { return now } | |
| 207 | + | ||
| 208 | + | runs := 0 | |
| 209 | + | err := s.Add("canceled-job", "canceled job", domain.CronSchedule{Preset: domain.ScheduleDaily}, func(context.Context) error { | |
| 210 | + | runs++ | |
| 211 | + | return nil | |
| 212 | + | }) | |
| 213 | + | require.NoError(t, err) | |
| 214 | + | ||
| 215 | + | s.mu.Lock() | |
| 216 | + | s.entries["canceled-job"].nextRun = now.Add(-time.Minute) | |
| 217 | + | s.mu.Unlock() | |
| 218 | + | ||
| 219 | + | ctx, cancel := context.WithCancel(context.Background()) | |
| 220 | + | cancel() | |
| 221 | + | ||
| 222 | + | s.Start(ctx) | |
| 223 | + | time.Sleep(50 * time.Millisecond) | |
| 224 | + | ||
| 225 | + | assert.Equal(t, 0, runs) | |
| 226 | + | assert.False(t, s.started.Load()) | |
| 227 | + | } | |
| @@ -199,3 +199,29 @@ func TestSchedulerRunNowComputesNextRunFromJobFinishTime(t *testing.T) { | |||||
| 199 | require.Len(t, entries, 1) | 199 | require.Len(t, entries, 1) | ||
| 200 | assert.Equal(t, time.Date(2026, 2, 7, 14, 0, 0, 0, time.UTC), entries[0].NextRun) | 200 | assert.Equal(t, time.Date(2026, 2, 7, 14, 0, 0, 0, time.UTC), entries[0].NextRun) | ||
| 201 | } | 201 | } | ||
| 202 | + | ||||
| 203 | + | func TestSchedulerStartNoOpWhenContextAlreadyCanceled(t *testing.T) { | |||
| 204 | + | s := NewScheduler(zerowrap.Default()) | |||
| 205 | + | now := time.Date(2026, 2, 7, 12, 0, 0, 0, time.UTC) | |||
| 206 | + | s.nowFn = func() time.Time { return now } | |||
| 207 | + | ||||
| 208 | + | runs := 0 | |||
| 209 | + | err := s.Add("canceled-job", "canceled job", domain.CronSchedule{Preset: domain.ScheduleDaily}, func(context.Context) error { | |||
| 210 | + | runs++ | |||
| 211 | + | return nil | |||
| 212 | + | }) | |||
| 213 | + | require.NoError(t, err) | |||
| 214 | + | ||||
| 215 | + | s.mu.Lock() | |||
| 216 | + | s.entries["canceled-job"].nextRun = now.Add(-time.Minute) | |||
| 217 | + | s.mu.Unlock() | |||
| 218 | + | ||||
| 219 | + | ctx, cancel := context.WithCancel(context.Background()) | |||
| 220 | + | cancel() | |||
| 221 | + | ||||
| 222 | + | s.Start(ctx) | |||
| 223 | + | time.Sleep(50 * time.Millisecond) | |||
| 224 | + | ||||
| 225 | + | assert.Equal(t, 0, runs) | |||
| 226 | + | assert.False(t, s.started.Load()) | |||
| 227 | + | } | |||