From 14a7f8646cad266d567035a7beb9cf6cfd90d54c Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 25 Mar 2026 12:43:47 +0100 Subject: [PATCH] fix(backup): prevent notification failures from affecting backup status - Wrap notification calls in try-catch blocks to log failures instead - Prevent failed() method from overwriting successful backup status - Skip failure notifications if backup already completed successfully - Ensures post-backup errors (e.g. notification failures) never retroactively mark successful backups as failed Fixes #9088 --- app/Jobs/DatabaseBackupJob.php | 61 ++++++++++++++------ tests/Feature/DatabaseBackupJobTest.php | 76 +++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 16 deletions(-) diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index b55c324be..041d31bad 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -399,7 +399,15 @@ public function handle(): void 's3_uploaded' => null, ]); } - $this->team?->notify(new BackupFailed($this->backup, $this->database, $this->error_output ?? $this->backup_output ?? $e->getMessage(), $database)); + try { + $this->team?->notify(new BackupFailed($this->backup, $this->database, $this->error_output ?? $this->backup_output ?? $e->getMessage(), $database)); + } catch (\Throwable $notifyException) { + Log::channel('scheduled-errors')->warning('Failed to send backup failure notification', [ + 'backup_id' => $this->backup->uuid, + 'database' => $database, + 'error' => $notifyException->getMessage(), + ]); + } continue; } @@ -439,11 +447,20 @@ public function handle(): void 'local_storage_deleted' => $localStorageDeleted, ]); - // Send appropriate notification - if ($s3UploadError) { - $this->team->notify(new BackupSuccessWithS3Warning($this->backup, $this->database, $database, $s3UploadError)); - } else { - $this->team->notify(new BackupSuccess($this->backup, $this->database, $database)); + // Send appropriate notification (wrapped in try-catch so notification + // failures never affect backup status — see GitHub issue #9088) + try { + if ($s3UploadError) { + $this->team->notify(new BackupSuccessWithS3Warning($this->backup, $this->database, $database, $s3UploadError)); + } else { + $this->team->notify(new BackupSuccess($this->backup, $this->database, $database)); + } + } catch (\Throwable $e) { + Log::channel('scheduled-errors')->warning('Failed to send backup success notification', [ + 'backup_id' => $this->backup->uuid, + 'database' => $database, + 'error' => $e->getMessage(), + ]); } } } @@ -710,20 +727,32 @@ public function failed(?Throwable $exception): void $log = ScheduledDatabaseBackupExecution::where('uuid', $this->backup_log_uuid)->first(); if ($log) { - $log->update([ - 'status' => 'failed', - 'message' => 'Job permanently failed after '.$this->attempts().' attempts: '.($exception?->getMessage() ?? 'Unknown error'), - 'size' => 0, - 'filename' => null, - 'finished_at' => Carbon::now(), - ]); + // Don't overwrite a successful backup status — a post-backup error + // (e.g. notification failure) should not retroactively mark the backup + // as failed (see GitHub issue #9088) + if ($log->status !== 'success') { + $log->update([ + 'status' => 'failed', + 'message' => 'Job permanently failed after '.$this->attempts().' attempts: '.($exception?->getMessage() ?? 'Unknown error'), + 'size' => 0, + 'filename' => null, + 'finished_at' => Carbon::now(), + ]); + } } - // Notify team about permanent failure - if ($this->team) { + // Notify team about permanent failure (only if backup didn't already succeed) + if ($this->team && $log?->status !== 'success') { $databaseName = $log?->database_name ?? 'unknown'; $output = $this->backup_output ?? $exception?->getMessage() ?? 'Unknown error'; - $this->team->notify(new BackupFailed($this->backup, $this->database, $output, $databaseName)); + try { + $this->team->notify(new BackupFailed($this->backup, $this->database, $output, $databaseName)); + } catch (\Throwable $e) { + Log::channel('scheduled-errors')->warning('Failed to send backup permanent failure notification', [ + 'backup_id' => $this->backup->uuid, + 'error' => $e->getMessage(), + ]); + } } } } diff --git a/tests/Feature/DatabaseBackupJobTest.php b/tests/Feature/DatabaseBackupJobTest.php index 37c377dab..05cb21f12 100644 --- a/tests/Feature/DatabaseBackupJobTest.php +++ b/tests/Feature/DatabaseBackupJobTest.php @@ -120,6 +120,82 @@ expect($unrelatedBackup->save_s3)->toBeTruthy(); }); +test('failed method does not overwrite successful backup status', function () { + $team = Team::factory()->create(); + + $backup = ScheduledDatabaseBackup::create([ + 'frequency' => '0 0 * * *', + 'save_s3' => false, + 'database_type' => 'App\Models\StandalonePostgresql', + 'database_id' => 1, + 'team_id' => $team->id, + ]); + + $log = ScheduledDatabaseBackupExecution::create([ + 'uuid' => 'test-uuid-success-guard', + 'database_name' => 'test_db', + 'filename' => '/backup/test.dmp', + 'scheduled_database_backup_id' => $backup->id, + 'status' => 'success', + 'message' => 'Backup completed successfully', + 'size' => 1024, + ]); + + $job = new DatabaseBackupJob($backup); + + $reflection = new ReflectionClass($job); + + $teamProp = $reflection->getProperty('team'); + $teamProp->setValue($job, $team); + + $logUuidProp = $reflection->getProperty('backup_log_uuid'); + $logUuidProp->setValue($job, 'test-uuid-success-guard'); + + // Simulate a post-backup failure (e.g. notification error) + $job->failed(new Exception('Request to the Resend API failed')); + + $log->refresh(); + expect($log->status)->toBe('success'); + expect($log->message)->toBe('Backup completed successfully'); + expect($log->size)->toBe(1024); +}); + +test('failed method updates status when backup was not successful', function () { + $team = Team::factory()->create(); + + $backup = ScheduledDatabaseBackup::create([ + 'frequency' => '0 0 * * *', + 'save_s3' => false, + 'database_type' => 'App\Models\StandalonePostgresql', + 'database_id' => 1, + 'team_id' => $team->id, + ]); + + $log = ScheduledDatabaseBackupExecution::create([ + 'uuid' => 'test-uuid-pending-guard', + 'database_name' => 'test_db', + 'filename' => '/backup/test.dmp', + 'scheduled_database_backup_id' => $backup->id, + 'status' => 'pending', + ]); + + $job = new DatabaseBackupJob($backup); + + $reflection = new ReflectionClass($job); + + $teamProp = $reflection->getProperty('team'); + $teamProp->setValue($job, $team); + + $logUuidProp = $reflection->getProperty('backup_log_uuid'); + $logUuidProp->setValue($job, 'test-uuid-pending-guard'); + + $job->failed(new Exception('Some real failure')); + + $log->refresh(); + expect($log->status)->toBe('failed'); + expect($log->message)->toContain('Some real failure'); +}); + test('s3 storage has scheduled backups relationship', function () { $team = Team::factory()->create();