Merge pull request #7256 from pixelfed/fix/statusdelete-fanout-isolation

Isolate fanout delivery failures from StatusDelete local cleanup

app/Jobs/StatusPipeline/StatusDelete.php

@@ -217,15 +217,29 @@ class StatusDelete implements ShouldQueue
             'inboxes' => count($audience),
         ]);
 
-        ActivityPubDeliveryService::pool($profile, $audience, $activity, function ($res, $i) use ($audience, $status) {
-            Log::warning('StatusDelete: delivery failed', [
+        // Isolate federation delivery from local cleanup. pool() can throw
+        // synchronously (e.g. validateSender() rejects an inactive sender during
+        // account deletion, where profiles.status = 'delete'). If that exception
+        // escaped, unlinkRemoveMedia() — the whole point of this job — would be
+        // skipped and the status + its data would leak. Delivery is best-effort;
+        // local deletion is not.
+        try {
+            ActivityPubDeliveryService::pool($profile, $audience, $activity, function ($res, $i) use ($audience, $status) {
+                Log::warning('StatusDelete: delivery failed', [
+                    'status_id' => $status->id,
+                    'inbox' => $audience[$i] ?? null,
+                    'result' => $res instanceof \Throwable
+                        ? get_class($res).': '.$res->getMessage()
+                        : $res->status().' '.substr($res->body(), 0, 300),
+                ]);
+            });
+        } catch (\Throwable $e) {
+            Log::warning('StatusDelete: delivery aborted, proceeding to local cleanup', [
                 'status_id' => $status->id,
-                'inbox' => $audience[$i] ?? null,
-                'result' => $res instanceof \Throwable
-                    ? get_class($res).': '.$res->getMessage()
-                    : $res->status().' '.substr($res->body(), 0, 300),
+                'exception' => $e::class,
+                'error' => $e->getMessage(),
             ]);
-        });
+        }
 
         $this->unlinkRemoveMedia($status);
 

tests/Feature/StatusDeleteCleanupTest.php

@@ -2,8 +2,10 @@
 
 use App\Jobs\StatusPipeline\StatusDelete;
 use App\Models\DirectMessage;
+use App\Models\Follower;
 use App\Models\MediaTag;
 use App\Models\Notification;
+use App\Models\Profile;
 use App\Models\Status;
 use App\Models\StatusEdit;
 use App\Models\User;
@@ -148,3 +150,79 @@ it('cleans up even when the owning profile is soft deleted', function () {
 
     expect(Status::find($status->id))->toBeNull();
 });
+
+/*
+|--------------------------------------------------------------------------
+| StatusDelete cleanup is isolated from federation delivery failures
+|--------------------------------------------------------------------------
+|
+| Account deletion marks the profile inactive (status = 'delete') then dispatches
+| StatusDelete jobs. With federation enabled and a warm, non-empty follower
+| audience, fanoutDelete() calls ActivityPubDeliveryService::pool(), whose
+| validateSender() throws for an inactive sender. That exception must not abort
+| the job before unlinkRemoveMedia() runs, or the status leaks permanently.
+|
+*/
+
+it('deletes the status even when fanout delivery throws for an inactive sender', function () {
+    config(['federation.activitypub.enabled' => true]);
+
+    $owner = User::factory()->create();
+    $owner->refresh();
+    $profile = $owner->profile;
+
+    // Remote follower -> non-empty audience so fanoutDelete calls pool().
+    $remote = Profile::factory()->remote()->create([
+        'inbox_url' => 'https://remote.example/inbox',
+        'sharedInbox' => null,
+    ]);
+    Follower::create([
+        'profile_id' => $remote->id,
+        'following_id' => $profile->id,
+        'local_profile' => false,
+    ]);
+
+    expect($profile->fresh()->getAudienceInbox())->not->toBeEmpty();
+
+    // Account-deletion state: inactive sender -> validateSender() throws in pool().
+    $profile->status = 'delete';
+    $profile->save();
+
+    $status = Status::factory()->create(['profile_id' => $profile->id, 'type' => 'photo']);
+
+    // Must not throw, and must complete local cleanup.
+    (new StatusDelete($status))->handle();
+
+    expect(Status::find($status->id))->toBeNull();
+});
+
+it('decrements status_count once when fanout delivery throws for an inactive sender', function () {
+    config(['federation.activitypub.enabled' => true]);
+
+    $owner = User::factory()->create();
+    $owner->refresh();
+    $profile = $owner->profile;
+
+    $remote = Profile::factory()->remote()->create([
+        'inbox_url' => 'https://remote.example/inbox',
+        'sharedInbox' => null,
+    ]);
+    Follower::create([
+        'profile_id' => $remote->id,
+        'following_id' => $profile->id,
+        'local_profile' => false,
+    ]);
+
+    $profile->status = 'delete';
+    $profile->save();
+    $profile->status_count = 5;
+    $profile->saveQuietly();
+
+    $status = Status::factory()->create(['profile_id' => $profile->id, 'type' => 'photo']);
+
+    // The job completes (no throw), so the queue does not retry and re-decrement.
+    (new StatusDelete($status))->handle();
+
+    expect(Status::find($status->id))->toBeNull();
+    expect((int) $profile->fresh()->status_count)->toBe(4);
+});