Skip to content

Commit 6aef086

Browse files
RichardAndersonRichardAnderson
authored
[Fix] Backups: one failing backup no longer aborts the whole scheduled run (#1234)
* fix: resolve backup problem where a server has been deleted * fix: code rabbit review
1 parent 7766484 commit 6aef086

4 files changed

Lines changed: 152 additions & 12 deletions

File tree

‎app/Console/Commands/RunBackupCommand.php‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
use App\Models\Backup;
77
use Cron\CronExpression;
88
use Illuminate\Console\Command;
9+
use Illuminate\Support\Facades\Log;
10+
use Throwable;
911

1012
class RunBackupCommand extends Command
1113
{
@@ -16,26 +18,36 @@ class RunBackupCommand extends Command
1618
public function handle(): void
1719
{
1820
$total = 0;
21+
$failed = 0;
1922

2023
Backup::query()
2124
->where('enabled', true)
2225
->whereNull('status')
2326
->whereHas('server')
2427
->with('server')
25-
->chunkById(100, function ($backups) use (&$total): void {
28+
->chunkById(100, function ($backups) use (&$total, &$failed): void {
2629
/** @var Backup $backup */
2730
foreach ($backups as $backup) {
2831
if (! CronExpression::isValidExpression((string) $backup->interval)) {
2932
continue;
3033
}
3134

3235
if ((new CronExpression((string) $backup->interval))->isDue(now(), config('app.timezone'))) {
33-
app(RunBackup::class)->run($backup);
34-
$total++;
36+
try {
37+
app(RunBackup::class)->run($backup);
38+
$total++;
39+
} catch (Throwable $e) {
40+
Log::warning('Failed to run backup', [
41+
'backup_id' => $backup->id,
42+
'server_id' => $backup->server_id,
43+
'error' => $e->getMessage(),
44+
]);
45+
$failed++;
46+
}
3547
}
3648
}
3749
});
3850

39-
$this->info("{$total} backups started");
51+
$this->info("{$total} backups started, {$failed} failed");
4052
}
4153
}

‎database/migrations/2026_08_17_114119_delete_orphaned_backups_and_add_foreign_keys.php‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
<?php
2+
3+
use Illuminate\Database\Migrations\Migration;
4+
use Illuminate\Database\Query\Builder;
5+
use Illuminate\Database\Schema\Blueprint;
6+
use Illuminate\Support\Facades\DB;
7+
use Illuminate\Support\Facades\Schema;
8+
9+
return new class extends Migration
10+
{
11+
/**
12+
* Orphaned rows are deleted before the foreign keys are added, because a foreign key
13+
* cannot be created on a table that already violates it.
14+
*/
15+
public function up(): void
16+
{
17+
DB::transaction(function (): void {
18+
DB::table('backups')
19+
->whereNotExists(fn (Builder $query) => $query
20+
->select(DB::raw(1))
21+
->from('servers')
22+
->whereColumn('servers.id', 'backups.server_id'))
23+
->delete();
24+
25+
DB::table('backup_files')
26+
->whereNotExists(fn (Builder $query) => $query
27+
->select(DB::raw(1))
28+
->from('backups')
29+
->whereColumn('backups.id', 'backup_files.backup_id'))
30+
->delete();
31+
});
32+
33+
Schema::table('backups', function (Blueprint $table): void {
34+
$table->foreign('server_id')->references('id')->on('servers')->cascadeOnDelete();
35+
});
36+
37+
Schema::table('backup_files', function (Blueprint $table): void {
38+
$table->unsignedBigInteger('backup_id')->change();
39+
});
40+
41+
Schema::table('backup_files', function (Blueprint $table): void {
42+
$table->foreign('backup_id')->references('id')->on('backups')->cascadeOnDelete();
43+
});
44+
}
45+
46+
/**
47+
* The schema is fully reverted, but rows deleted by up() cannot be restored.
48+
*/
49+
public function down(): void
50+
{
51+
Schema::table('backup_files', function (Blueprint $table): void {
52+
$table->dropForeign(['backup_id']);
53+
});
54+
55+
Schema::table('backups', function (Blueprint $table): void {
56+
$table->dropForeign(['server_id']);
57+
});
58+
59+
Schema::table('backup_files', function (Blueprint $table): void {
60+
$table->unsignedInteger('backup_id')->change();
61+
});
62+
}
63+
};

‎tests/Feature/BackupTest.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use App\StorageProviders\Local;
2222
use Illuminate\Foundation\Testing\RefreshDatabase;
2323
use Illuminate\Support\Facades\Bus;
24+
use Illuminate\Support\Facades\DB;
2425
use Illuminate\Support\Facades\Http;
2526
use Illuminate\Testing\TestResponse;
2627
use Illuminate\Validation\ValidationException;
@@ -554,6 +555,7 @@
554555
});
555556

556557
test('delete orphaned backup file hard deletes without dispatching job', function () {
558+
DB::statement('PRAGMA defer_foreign_keys = ON');
557559
Bus::fake();
558560

559561
$backup = Backup::factory()->create([
@@ -573,6 +575,29 @@
573575
Bus::assertNotDispatched(DeleteFileJob::class);
574576
});
575577

578+
test('deleting a server row cascades to its backups and backup files', function () {
579+
$server = Server::factory()->create([
580+
'project_id' => $this->user->currentProject->id,
581+
'user_id' => $this->user->id,
582+
]);
583+
$backup = Backup::factory()->create([
584+
'type' => BackupType::FILE,
585+
'server_id' => $server->id,
586+
'storage_id' => $this->storageProvider->id,
587+
'path' => '/home/vito/y.com',
588+
'status' => null,
589+
]);
590+
$file = BackupFile::factory()->create([
591+
'backup_id' => $backup->id,
592+
'status' => BackupFileStatus::CREATED,
593+
]);
594+
595+
DB::table('servers')->where('id', $server->id)->delete();
596+
597+
$this->assertDatabaseMissing('backups', ['id' => $backup->id]);
598+
$this->assertDatabaseMissing('backup_files', ['id' => $file->id]);
599+
});
600+
576601
test('see global backups list scoped to current project', function () {
577602
$this->actingAs($this->user);
578603

‎tests/Unit/Commands/RunBackupCommandTest.php‎

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
<?php
22

3+
use App\Actions\Backup\RunBackup;
34
use App\Enums\BackupFileStatus;
45
use App\Enums\BackupStatus;
56
use App\Facades\SSH;
@@ -10,7 +11,9 @@
1011
use Illuminate\Foundation\Testing\RefreshDatabase;
1112
use Illuminate\Support\Carbon;
1213
use Illuminate\Support\Facades\Bus;
14+
use Illuminate\Support\Facades\DB;
1315
use Illuminate\Support\Facades\Http;
16+
use Illuminate\Support\Facades\Log;
1417

1518
uses(RefreshDatabase::class);
1619

@@ -43,7 +46,7 @@ function vitoPestUnitCommandsRunBackupCommandTestCreateBackup(array $attributes)
4346

4447
test('run without any backups', function () {
4548
$this->artisan('backups:run')
46-
->expectsOutput('0 backups started');
49+
->expectsOutput('0 backups started, 0 failed');
4750
});
4851

4952
test('runs backups that are due', function () {
@@ -54,7 +57,7 @@ function vitoPestUnitCommandsRunBackupCommandTestCreateBackup(array $attributes)
5457
vitoPestUnitCommandsRunBackupCommandTestCreateBackup(['interval' => '0 * * * *']);
5558

5659
$this->artisan('backups:run')
57-
->expectsOutput('1 backups started');
60+
->expectsOutput('1 backups started, 0 failed');
5861
});
5962

6063
test('does not run backups that are not due', function () {
@@ -64,7 +67,7 @@ function vitoPestUnitCommandsRunBackupCommandTestCreateBackup(array $attributes)
6467
vitoPestUnitCommandsRunBackupCommandTestCreateBackup(['interval' => '30 * * * *']);
6568

6669
$this->artisan('backups:run')
67-
->expectsOutput('0 backups started');
70+
->expectsOutput('0 backups started, 0 failed');
6871
});
6972

7073
test('runs custom interval backups when due', function () {
@@ -75,7 +78,7 @@ function vitoPestUnitCommandsRunBackupCommandTestCreateBackup(array $attributes)
7578
vitoPestUnitCommandsRunBackupCommandTestCreateBackup(['interval' => '5 10 * * *']);
7679

7780
$this->artisan('backups:run')
78-
->expectsOutput('1 backups started');
81+
->expectsOutput('1 backups started, 0 failed');
7982
});
8083

8184
test('does not run disabled backups', function () {
@@ -85,7 +88,7 @@ function vitoPestUnitCommandsRunBackupCommandTestCreateBackup(array $attributes)
8588
vitoPestUnitCommandsRunBackupCommandTestCreateBackup(['interval' => '* * * * *', 'enabled' => false]);
8689

8790
$this->artisan('backups:run')
88-
->expectsOutput('0 backups started');
91+
->expectsOutput('0 backups started, 0 failed');
8992
});
9093

9194
test('does not run backups being deleted', function () {
@@ -95,7 +98,7 @@ function vitoPestUnitCommandsRunBackupCommandTestCreateBackup(array $attributes)
9598
vitoPestUnitCommandsRunBackupCommandTestCreateBackup(['interval' => '* * * * *', 'status' => BackupStatus::DELETING]);
9699

97100
$this->artisan('backups:run')
98-
->expectsOutput('0 backups started');
101+
->expectsOutput('0 backups started, 0 failed');
99102
});
100103

101104
test('runs enabled backup even after a failed run', function () {
@@ -111,18 +114,55 @@ function vitoPestUnitCommandsRunBackupCommandTestCreateBackup(array $attributes)
111114
]);
112115

113116
$this->artisan('backups:run')
114-
->expectsOutput('1 backups started');
117+
->expectsOutput('1 backups started, 0 failed');
118+
});
119+
120+
test('continues to the next backup when one fails', function () {
121+
SSH::fake();
122+
Log::spy();
123+
Carbon::setTestNow('2026-06-19 10:00:00');
124+
125+
$first = vitoPestUnitCommandsRunBackupCommandTestCreateBackup(['interval' => '0 * * * *']);
126+
vitoPestUnitCommandsRunBackupCommandTestCreateBackup(['interval' => '0 * * * *']);
127+
128+
$calls = 0;
129+
$this->mock(RunBackup::class, function ($mock) use (&$calls): void {
130+
$mock->shouldReceive('run')
131+
->twice()
132+
->andReturnUsing(function (Backup $backup) use (&$calls): BackupFile {
133+
$calls++;
134+
135+
if ($calls === 1) {
136+
throw new RuntimeException('boom');
137+
}
138+
139+
return BackupFile::factory()->create([
140+
'backup_id' => $backup->id,
141+
'status' => BackupFileStatus::CREATED,
142+
]);
143+
});
144+
});
145+
146+
$this->artisan('backups:run')
147+
->expectsOutput('1 backups started, 1 failed');
148+
149+
Log::shouldHaveReceived('warning')->withArgs(
150+
fn (string $message, array $context): bool => $context['backup_id'] === $first->id
151+
&& $context['server_id'] === $this->server->id
152+
&& $context['error'] === 'boom'
153+
);
115154
});
116155

117156
test('does not run backups whose server is missing', function () {
157+
DB::statement('PRAGMA defer_foreign_keys = ON');
118158
SSH::fake();
119159
Bus::fake();
120160
Carbon::setTestNow('2026-06-19 10:00:00');
121161

122162
$backup = vitoPestUnitCommandsRunBackupCommandTestCreateBackup(['interval' => '0 * * * *', 'server_id' => 999999]);
123163

124164
$this->artisan('backups:run')
125-
->expectsOutput('0 backups started');
165+
->expectsOutput('0 backups started, 0 failed');
126166

127167
$this->assertDatabaseHas('backups', ['id' => $backup->id, 'server_id' => 999999]);
128168
Bus::assertNothingDispatched();

0 commit comments

Comments
 (0)