diff --git a/app/Filament/Resources/BackupScheduleResource.php b/app/Filament/Resources/BackupScheduleResource.php index 8ef28ce..c0d959e 100644 --- a/app/Filament/Resources/BackupScheduleResource.php +++ b/app/Filament/Resources/BackupScheduleResource.php @@ -41,6 +41,7 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Support\Facades\Bus; use Illuminate\Support\Str; use Illuminate\Validation\ValidationException; @@ -320,26 +321,32 @@ public static function table(Table $table): Table $user = auth()->user(); $scheduledFor = CarbonImmutable::now('UTC')->startOfMinute(); + $run = null; + for ($i = 0; $i < 5; $i++) { - $exists = BackupScheduleRun::query() - ->where('backup_schedule_id', $record->id) - ->where('scheduled_for', $scheduledFor) - ->exists(); - - if (! $exists) { + try { + $run = BackupScheduleRun::create([ + 'backup_schedule_id' => $record->id, + 'tenant_id' => $tenant->getKey(), + 'scheduled_for' => $scheduledFor->toDateTimeString(), + 'status' => BackupScheduleRun::STATUS_RUNNING, + 'summary' => null, + ]); break; + } catch (UniqueConstraintViolationException) { + $scheduledFor = $scheduledFor->addMinute(); } - - $scheduledFor = $scheduledFor->addMinute(); } - $run = BackupScheduleRun::create([ - 'backup_schedule_id' => $record->id, - 'tenant_id' => $tenant->getKey(), - 'scheduled_for' => $scheduledFor->toDateTimeString(), - 'status' => BackupScheduleRun::STATUS_RUNNING, - 'summary' => null, - ]); + if (! $run instanceof BackupScheduleRun) { + Notification::make() + ->title('Run already queued') + ->body('Please wait a moment and try again.') + ->warning() + ->send(); + + return; + } app(AuditLogger::class)->log( tenant: $tenant, @@ -387,26 +394,32 @@ public static function table(Table $table): Table $user = auth()->user(); $scheduledFor = CarbonImmutable::now('UTC')->startOfMinute(); + $run = null; + for ($i = 0; $i < 5; $i++) { - $exists = BackupScheduleRun::query() - ->where('backup_schedule_id', $record->id) - ->where('scheduled_for', $scheduledFor) - ->exists(); - - if (! $exists) { + try { + $run = BackupScheduleRun::create([ + 'backup_schedule_id' => $record->id, + 'tenant_id' => $tenant->getKey(), + 'scheduled_for' => $scheduledFor->toDateTimeString(), + 'status' => BackupScheduleRun::STATUS_RUNNING, + 'summary' => null, + ]); break; + } catch (UniqueConstraintViolationException) { + $scheduledFor = $scheduledFor->addMinute(); } - - $scheduledFor = $scheduledFor->addMinute(); } - $run = BackupScheduleRun::create([ - 'backup_schedule_id' => $record->id, - 'tenant_id' => $tenant->getKey(), - 'scheduled_for' => $scheduledFor->toDateTimeString(), - 'status' => BackupScheduleRun::STATUS_RUNNING, - 'summary' => null, - ]); + if (! $run instanceof BackupScheduleRun) { + Notification::make() + ->title('Retry already queued') + ->body('Please wait a moment and try again.') + ->warning() + ->send(); + + return; + } app(AuditLogger::class)->log( tenant: $tenant, @@ -470,26 +483,26 @@ public static function table(Table $table): Table /** @var BackupSchedule $record */ foreach ($records as $record) { $scheduledFor = CarbonImmutable::now('UTC')->startOfMinute(); + $run = null; + for ($i = 0; $i < 5; $i++) { - $exists = BackupScheduleRun::query() - ->where('backup_schedule_id', $record->id) - ->where('scheduled_for', $scheduledFor) - ->exists(); - - if (! $exists) { + try { + $run = BackupScheduleRun::create([ + 'backup_schedule_id' => $record->id, + 'tenant_id' => $tenant->getKey(), + 'scheduled_for' => $scheduledFor->toDateTimeString(), + 'status' => BackupScheduleRun::STATUS_RUNNING, + 'summary' => null, + ]); break; + } catch (UniqueConstraintViolationException) { + $scheduledFor = $scheduledFor->addMinute(); } - - $scheduledFor = $scheduledFor->addMinute(); } - $run = BackupScheduleRun::create([ - 'backup_schedule_id' => $record->id, - 'tenant_id' => $tenant->getKey(), - 'scheduled_for' => $scheduledFor->toDateTimeString(), - 'status' => BackupScheduleRun::STATUS_RUNNING, - 'summary' => null, - ]); + if (! $run instanceof BackupScheduleRun) { + continue; + } $createdRunIds[] = (int) $run->id; @@ -522,11 +535,17 @@ public static function table(Table $table): Table ])); } - Notification::make() + $notification = Notification::make() ->title('Runs dispatched') - ->body(sprintf('Queued %d run(s).', count($createdRunIds))) - ->success() - ->send(); + ->body(sprintf('Queued %d run(s).', count($createdRunIds))); + + if (count($createdRunIds) === 0) { + $notification->warning(); + } else { + $notification->success(); + } + + $notification->send(); }), BulkAction::make('bulk_retry') ->label('Retry') @@ -548,26 +567,26 @@ public static function table(Table $table): Table /** @var BackupSchedule $record */ foreach ($records as $record) { $scheduledFor = CarbonImmutable::now('UTC')->startOfMinute(); + $run = null; + for ($i = 0; $i < 5; $i++) { - $exists = BackupScheduleRun::query() - ->where('backup_schedule_id', $record->id) - ->where('scheduled_for', $scheduledFor) - ->exists(); - - if (! $exists) { + try { + $run = BackupScheduleRun::create([ + 'backup_schedule_id' => $record->id, + 'tenant_id' => $tenant->getKey(), + 'scheduled_for' => $scheduledFor->toDateTimeString(), + 'status' => BackupScheduleRun::STATUS_RUNNING, + 'summary' => null, + ]); break; + } catch (UniqueConstraintViolationException) { + $scheduledFor = $scheduledFor->addMinute(); } - - $scheduledFor = $scheduledFor->addMinute(); } - $run = BackupScheduleRun::create([ - 'backup_schedule_id' => $record->id, - 'tenant_id' => $tenant->getKey(), - 'scheduled_for' => $scheduledFor->toDateTimeString(), - 'status' => BackupScheduleRun::STATUS_RUNNING, - 'summary' => null, - ]); + if (! $run instanceof BackupScheduleRun) { + continue; + } $createdRunIds[] = (int) $run->id; @@ -600,11 +619,17 @@ public static function table(Table $table): Table ])); } - Notification::make() + $notification = Notification::make() ->title('Retries dispatched') - ->body(sprintf('Queued %d run(s).', count($createdRunIds))) - ->success() - ->send(); + ->body(sprintf('Queued %d run(s).', count($createdRunIds))); + + if (count($createdRunIds) === 0) { + $notification->warning(); + } else { + $notification->success(); + } + + $notification->send(); }), DeleteBulkAction::make('bulk_delete') ->visible(fn (): bool => static::currentTenantRole()?->canManageBackupSchedules() ?? false), diff --git a/specs/032-backup-scheduling-mvp/tasks.md b/specs/032-backup-scheduling-mvp/tasks.md index 03cca66..7a4403f 100644 --- a/specs/032-backup-scheduling-mvp/tasks.md +++ b/specs/032-backup-scheduling-mvp/tasks.md @@ -88,6 +88,7 @@ ### Tests (Pest) - [X] T029 [P] [US3] Add feature test tests/Feature/BackupScheduling/ApplyRetentionJobTest.php (keeps last N backup_sets; soft-deletes older) - [X] T038 [P] [US1] Add feature test tests/Feature/BackupScheduling/BackupScheduleBulkDeleteTest.php (bulk delete action regression) - [X] T039 [P] [US1] Extend tests/Feature/BackupScheduling/BackupScheduleBulkDeleteTest.php (operator cannot bulk delete) +- [X] T041 [P] [US3] Make manual dispatch actions idempotent under concurrency in app/Filament/Resources/BackupScheduleResource.php (avoid unique constraint 500); add regression in tests/Feature/BackupScheduling/RunNowRetryActionsTest.php ### Implementation diff --git a/tests/Feature/BackupScheduling/RunNowRetryActionsTest.php b/tests/Feature/BackupScheduling/RunNowRetryActionsTest.php index 276fcf4..ed39833 100644 --- a/tests/Feature/BackupScheduling/RunNowRetryActionsTest.php +++ b/tests/Feature/BackupScheduling/RunNowRetryActionsTest.php @@ -5,6 +5,7 @@ use App\Models\BackupSchedule; use App\Models\BackupScheduleRun; use App\Models\User; +use Carbon\CarbonImmutable; use Filament\Facades\Filament; use Illuminate\Support\Facades\Queue; use Livewire\Livewire; @@ -203,3 +204,67 @@ Queue::assertPushed(RunBackupScheduleJob::class, 2); $this->assertDatabaseCount('notifications', 1); }); + +test('operator can bulk retry even if a run already exists for this minute', function () { + Queue::fake(); + + [$user, $tenant] = createUserWithTenant(role: 'operator'); + + $scheduleA = BackupSchedule::query()->create([ + 'tenant_id' => $tenant->id, + 'name' => 'Nightly A', + 'is_enabled' => true, + 'timezone' => 'UTC', + 'frequency' => 'daily', + 'time_of_day' => '01:00:00', + 'days_of_week' => null, + 'policy_types' => ['deviceConfiguration'], + 'include_foundations' => true, + 'retention_keep_last' => 30, + ]); + + $scheduleB = BackupSchedule::query()->create([ + 'tenant_id' => $tenant->id, + 'name' => 'Nightly B', + 'is_enabled' => true, + 'timezone' => 'UTC', + 'frequency' => 'daily', + 'time_of_day' => '02:00:00', + 'days_of_week' => null, + 'policy_types' => ['deviceConfiguration'], + 'include_foundations' => true, + 'retention_keep_last' => 30, + ]); + + $scheduledFor = CarbonImmutable::now('UTC')->startOfMinute(); + BackupScheduleRun::query()->create([ + 'backup_schedule_id' => $scheduleA->id, + 'tenant_id' => $tenant->id, + 'scheduled_for' => $scheduledFor->toDateTimeString(), + 'status' => BackupScheduleRun::STATUS_RUNNING, + 'summary' => null, + ]); + + $this->actingAs($user); + Filament::setTenant($tenant, true); + + Livewire::test(ListBackupSchedules::class) + ->callTableBulkAction('bulk_retry', collect([$scheduleA, $scheduleB])); + + expect(BackupScheduleRun::query()->where('backup_schedule_id', $scheduleA->id)->count()) + ->toBe(2); + + $newRunA = BackupScheduleRun::query() + ->where('backup_schedule_id', $scheduleA->id) + ->orderByDesc('id') + ->first(); + + expect($newRunA)->not->toBeNull(); + expect($newRunA->scheduled_for->setTimezone('UTC')->toDateTimeString()) + ->toBe($scheduledFor->addMinute()->toDateTimeString()); + + expect(BackupScheduleRun::query()->where('backup_schedule_id', $scheduleB->id)->count()) + ->toBe(1); + + Queue::assertPushed(RunBackupScheduleJob::class, 2); +});