feat(052): finalize async add policies

app/Jobs/AddPoliciesToBackupSetJob.php

@@ -44,23 +44,33 @@ public function handle(
     ): void {
         $run = BulkOperationRun::with(['tenant', 'user'])->find($this->bulkRunId);
 
-        if (! $run || $run->status !== 'pending') {
+        if (! $run) {
             return;
         }
 
-        $bulkOperationService->start($run);
+        $started = BulkOperationRun::query()
+            ->whereKey($run->getKey())
+            ->where('status', 'pending')
+            ->update(['status' => 'running']);
+
+        if ($started === 0) {
+            return;
+        }
+
+        $run->refresh();
 
         $tenant = $run->tenant ?? Tenant::query()->find($run->tenant_id);
 
+        try {
             if (! $tenant instanceof Tenant) {
-            $this->appendRunFailure($run, [
-                'type' => 'run',
-                'item_id' => (string) $this->backupSetId,
-                'reason_code' => 'backup_set_not_found',
-                'reason' => $bulkOperationService->sanitizeFailureReason('Tenant not found for run.'),
-            ]);
-
-            $bulkOperationService->fail($run, 'Tenant not found for run.');
+                $this->markRunFailed(
+                    bulkOperationService: $bulkOperationService,
+                    run: $run,
+                    tenant: null,
+                    itemId: (string) $this->backupSetId,
+                    reasonCode: 'unknown',
+                    reason: 'Tenant not found for run.',
+                );
 
                 return;
             }
@@ -71,27 +81,27 @@ public function handle(
                 ->first();
 
             if (! $backupSet) {
-            $this->appendRunFailure($run, [
-                'type' => 'run',
-                'item_id' => (string) $this->backupSetId,
-                'reason_code' => 'backup_set_not_found',
-                'reason' => $bulkOperationService->sanitizeFailureReason('Backup set not found.'),
-            ]);
-
-            $bulkOperationService->fail($run, 'Backup set not found.');
+                $this->markRunFailed(
+                    bulkOperationService: $bulkOperationService,
+                    run: $run,
+                    tenant: $tenant,
+                    itemId: (string) $this->backupSetId,
+                    reasonCode: 'backup_set_not_found',
+                    reason: 'Backup set not found.',
+                );
 
                 return;
             }
 
             if ($backupSet->trashed()) {
-            $this->appendRunFailure($run, [
-                'type' => 'run',
-                'item_id' => (string) $backupSet->getKey(),
-                'reason_code' => 'backup_set_archived',
-                'reason' => $bulkOperationService->sanitizeFailureReason('Backup set is archived.'),
-            ]);
-
-            $bulkOperationService->fail($run, 'Backup set is archived.');
+                $this->markRunFailed(
+                    bulkOperationService: $bulkOperationService,
+                    run: $run,
+                    tenant: $tenant,
+                    itemId: (string) $backupSet->getKey(),
+                    reasonCode: 'backup_set_archived',
+                    reason: 'Backup set is archived.',
+                );
 
                 return;
             }
@@ -414,8 +424,10 @@ public function handle(
 
             $message .= '.';
 
+            $partial = $run->status === 'completed_with_errors' || $foundationFailures > 0;
+
             $notification = Notification::make()
-            ->title($run->failed > 0 ? 'Add Policies Completed (partial)' : 'Add Policies Completed')
+                ->title($partial ? 'Add Policies Completed (partial)' : 'Add Policies Completed')
                 ->body($message)
                 ->actions([
                     \Filament\Actions\Action::make('view_run')
@@ -423,7 +435,7 @@ public function handle(
                         ->url(BulkOperationRunResource::getUrl('view', ['record' => $run], tenant: $tenant)),
                 ]);
 
-        if ($run->failed > 0) {
+            if ($partial) {
                 $notification->warning();
             } else {
                 $notification->success();
@@ -432,6 +444,24 @@ public function handle(
             $notification
                 ->sendToDatabase($run->user)
                 ->send();
+        } catch (Throwable $throwable) {
+            $run->refresh();
+
+            if (in_array($run->status, ['completed', 'completed_with_errors'], true)) {
+                throw $throwable;
+            }
+
+            $this->markRunFailed(
+                bulkOperationService: $bulkOperationService,
+                run: $run,
+                tenant: $tenant instanceof Tenant ? $tenant : null,
+                itemId: (string) $this->backupSetId,
+                reasonCode: 'unknown',
+                reason: $throwable->getMessage(),
+            );
+
+            throw $throwable;
+        }
     }
 
     /**
@@ -470,6 +500,56 @@ private function appendRunFailure(BulkOperationRun $run, array $entry): void
         $run->update(['failures' => $failures]);
     }
 
+    private function markRunFailed(
+        BulkOperationService $bulkOperationService,
+        BulkOperationRun $run,
+        ?Tenant $tenant,
+        string $itemId,
+        string $reasonCode,
+        string $reason,
+    ): void {
+        $reason = $bulkOperationService->sanitizeFailureReason($reason);
+
+        $this->appendRunFailure($run, [
+            'type' => 'run',
+            'item_id' => $itemId,
+            'reason_code' => $reasonCode,
+            'reason' => $reason,
+        ]);
+
+        try {
+            $bulkOperationService->fail($run, $reason);
+        } catch (Throwable) {
+            $run->update(['status' => 'failed']);
+        }
+
+        $this->notifyRunFailed($run, $tenant, $reason);
+    }
+
+    private function notifyRunFailed(BulkOperationRun $run, ?Tenant $tenant, string $reason): void
+    {
+        if (! $run->user) {
+            return;
+        }
+
+        $notification = Notification::make()
+            ->title('Add Policies Failed')
+            ->body($reason);
+
+        if ($tenant instanceof Tenant) {
+            $notification->actions([
+                \Filament\Actions\Action::make('view_run')
+                    ->label('View run')
+                    ->url(BulkOperationRunResource::getUrl('view', ['record' => $run], tenant: $tenant)),
+            ]);
+        }
+
+        $notification
+            ->danger()
+            ->sendToDatabase($run->user)
+            ->send();
+    }
+
     private function mapGraphFailureReasonCode(?int $status): string
     {
         return match (true) {

app/Services/BulkOperationService.php

@@ -171,7 +171,11 @@ public function complete(BulkOperationRun $run): void
             return;
         }
 
-        $status = $run->failed > 0 ? 'completed_with_errors' : 'completed';
+        $failureEntries = collect($run->failures ?? []);
+        $hasFailures = $run->failed > 0
+            || $failureEntries->contains(fn (array $entry): bool => ($entry['type'] ?? 'failed') !== 'skipped');
+
+        $status = $hasFailures ? 'completed_with_errors' : 'completed';
 
         $updated = BulkOperationRun::query()
             ->whereKey($run->id)

specs/052-async-add-policies/tasks.md

@@ -29,8 +29,8 @@ ## Path Conventions (Laravel)
 
 ## Phase 1: Setup (Shared Infrastructure)
 
-- [ ] T001 [P] Confirm existing run infra supports this operation (BulkOperationRun statuses + unique idempotency index) and document mapping queued↔pending, partial↔completed_with_errors in specs/052-async-add-policies/plan.md
-- [ ] T002 [P] Decide and standardize taxonomy strings: operationType (`backup_set.add_policies`), resource (`backup_set`), action (`add_policies`) in specs/052-async-add-policies/spec.md and plan.md
+- [x] T001 [P] Confirm existing run infra supports this operation (BulkOperationRun statuses + unique idempotency index) and document mapping queued↔pending, partial↔completed_with_errors in specs/052-async-add-policies/plan.md
+- [x] T002 [P] Decide and standardize taxonomy strings: operationType (`backup_set.add_policies`), resource (`backup_set`), action (`add_policies`) in specs/052-async-add-policies/spec.md and plan.md
 
 ---
 
@@ -42,15 +42,15 @@ ## Phase 2: User Story 1 - Add selected policies without blocking (Priority: P1)
 
 ### Tests (write first) ⚠️
 
-- [ ] T010 [P] [US1] Update/replace sync-path assertions in tests/Feature/Filament/BackupSetPolicyPickerTableTest.php to assert job dispatch + run creation
-- [ ] T011 [P] [US1] Add fail-hard guard test to ensure no Graph calls occur during the bulk action (mock `App\\Services\\Graph\\GraphClientInterface` and assert `->never()`)
+- [x] T010 [P] [US1] Update/replace sync-path assertions in tests/Feature/Filament/BackupSetPolicyPickerTableTest.php to assert job dispatch + run creation
+- [x] T011 [P] [US1] Add fail-hard guard test to ensure no Graph calls occur during the bulk action (mock `App\\Services\\Graph\\GraphClientInterface` and assert `->never()`)
 
 ### Implementation
 
-- [ ] T020 [US1] Create queued job to add policies to a backup set in app/Jobs/AddPoliciesToBackupSetJob.php (uses run lifecycle + safe failures via BulkOperationService)
-- [ ] T021 [US1] Update bulk action handler in app/Livewire/BackupSetPolicyPickerTable.php to create/reuse BulkOperationRun and dispatch AddPoliciesToBackupSetJob (no inline snapshot capture)
-- [ ] T022 [US1] Emit “queued” notification with “View run” link on submit (Filament DB notification preferred) from app/Livewire/BackupSetPolicyPickerTable.php
-- [ ] T023 [US1] Emit completion/failure DB notification from app/Jobs/AddPoliciesToBackupSetJob.php with a safe summary
+- [x] T020 [US1] Create queued job to add policies to a backup set in app/Jobs/AddPoliciesToBackupSetJob.php (uses run lifecycle + safe failures via BulkOperationService)
+- [x] T021 [US1] Update bulk action handler in app/Livewire/BackupSetPolicyPickerTable.php to create/reuse BulkOperationRun and dispatch AddPoliciesToBackupSetJob (no inline snapshot capture)
+- [x] T022 [US1] Emit “queued” notification with “View run” link on submit (Filament DB notification preferred) from app/Livewire/BackupSetPolicyPickerTable.php
+- [x] T023 [US1] Emit completion/failure DB notification from app/Jobs/AddPoliciesToBackupSetJob.php with a safe summary
 
 **Checkpoint**: US1 complete — submit is non-blocking, job executes, run is visible.
 
@@ -64,13 +64,13 @@ ## Phase 3: User Story 2 - Double click / repeated submissions are deduplicated
 
 ### Tests ⚠️
 
-- [ ] T030 [P] [US2] Add idempotency test in tests/Feature/BackupSets/BackupSetAddPoliciesIdempotencyTest.php (or extend existing picker test) asserting one run + one queued job
+- [x] T030 [P] [US2] Add idempotency test in tests/Feature/BackupSets/BackupSetAddPoliciesIdempotencyTest.php (or extend existing picker test) asserting one run + one queued job
 
 ### Implementation
 
-- [ ] T031 [US2] Implement deterministic selection hashing and idempotency key creation using app/Support/RunIdempotency.php (sorted policy ids + option flags), and reuse active runs via findActiveBulkOperationRun
-- [ ] T032 [US2] Handle race conditions safely (unique index collisions) by recovering the existing run rather than failing the request
-- [ ] T033 [US2] Ensure the action is always async (no `dispatchSync` path for small selections) in app/Livewire/BackupSetPolicyPickerTable.php
+- [x] T031 [US2] Implement deterministic selection hashing and idempotency key creation using app/Support/RunIdempotency.php (sorted policy ids + option flags), and reuse active runs via findActiveBulkOperationRun
+- [x] T032 [US2] Handle race conditions safely (unique index collisions) by recovering the existing run rather than failing the request
+- [x] T033 [US2] Ensure the action is always async (no `dispatchSync` path for small selections) in app/Livewire/BackupSetPolicyPickerTable.php
 
 **Checkpoint**: US2 complete — double clicks are safe and deduped.
 
@@ -84,15 +84,15 @@ ## Phase 4: User Story 3 - Failures are visible and safe (Priority: P3)
 
 ### Tests ⚠️
 
-- [ ] T040 [P] [US3] Add tenant isolation test for the created run (403 cross-tenant) in tests/Feature/BackupSets/BackupSetAddPoliciesTenantIsolationTest.php (or extend tests/Feature/RunAuthorizationTenantIsolationTest.php)
-- [ ] T041 [P] [US3] Add sanitization test: failure reason containing token-like content is stored as redacted (exercise BulkOperationService::sanitizeFailureReason)
+- [x] T040 [P] [US3] Add tenant isolation test for the created run (403 cross-tenant) in tests/Feature/BackupSets/BackupSetAddPoliciesTenantIsolationTest.php (or extend tests/Feature/RunAuthorizationTenantIsolationTest.php)
+- [x] T041 [P] [US3] Add sanitization test: failure reason containing token-like content is stored as redacted (exercise BulkOperationService::sanitizeFailureReason)
 
 ### Implementation
 
-- [ ] T042 [US3] Ensure job records per-item failures with sanitized reasons and does not store raw Graph payloads in run failures or notifications (app/Jobs/AddPoliciesToBackupSetJob.php)
-- [ ] T043 [US3] Record stable failure `reason_code` values (per spec.md) alongside sanitized short text in run failures (app/Jobs/AddPoliciesToBackupSetJob.php and/or app/Services/BulkOperationService.php)
-- [ ] T044 [US3] Record “already in backup set” as `skipped` (with reason_code `already_in_backup_set`) and ensure counts match spec.md (app/Jobs/AddPoliciesToBackupSetJob.php)
-- [ ] T045 [US3] Ensure job processes all items (no circuit breaker abort) and run status reflects partial completion (app/Jobs/AddPoliciesToBackupSetJob.php)
+- [x] T042 [US3] Ensure job records per-item failures with sanitized reasons and does not store raw Graph payloads in run failures or notifications (app/Jobs/AddPoliciesToBackupSetJob.php)
+- [x] T043 [US3] Record stable failure `reason_code` values (per spec.md) alongside sanitized short text in run failures (app/Jobs/AddPoliciesToBackupSetJob.php and/or app/Services/BulkOperationService.php)
+- [x] T044 [US3] Record “already in backup set” as `skipped` (with reason_code `already_in_backup_set`) and ensure counts match spec.md (app/Jobs/AddPoliciesToBackupSetJob.php)
+- [x] T045 [US3] Ensure job processes all items (no circuit breaker abort) and run status reflects partial completion (app/Jobs/AddPoliciesToBackupSetJob.php)
 
 **Checkpoint**: US3 complete — failures are safe and observable; tenant isolation holds.
 
@@ -100,5 +100,5 @@ ### Implementation
 
 ## Phase 5: Polish & Validation
 
-- [ ] T050 [P] Run formatting on changed files with ./vendor/bin/pint --dirty
-- [ ] T051 Run targeted tests: ./vendor/bin/sail artisan test --filter=BackupSetAddPolicies
+- [x] T050 [P] Run formatting on changed files with ./vendor/bin/pint --dirty
+- [x] T051 Run targeted tests: ./vendor/bin/sail artisan test --filter=BackupSetAddPolicies

tests/Unit/BulkOperationRunProgressTest.php

@@ -59,3 +59,33 @@
 
     expect($run->total_items)->toBe(2);
 });
+
+test('bulk operation completion treats non-skipped failure entries as errors even when failed is zero', function () {
+    $tenant = Tenant::factory()->create();
+    $user = User::factory()->create();
+
+    $service = app(BulkOperationService::class);
+    $run = $service->createRun($tenant, $user, 'backup_set', 'add_policies', ['1'], 1);
+
+    $service->start($run);
+    $service->recordSuccess($run);
+
+    $run->update([
+        'failures' => [
+            [
+                'type' => 'foundation',
+                'item_id' => 'foundation',
+                'reason' => 'Forbidden',
+                'reason_code' => 'graph_forbidden',
+                'timestamp' => now()->toIso8601String(),
+            ],
+        ],
+    ]);
+
+    $service->complete($run);
+
+    $run->refresh();
+
+    expect($run->failed)->toBe(0)
+        ->and($run->status)->toBe('completed_with_errors');
+});