Merge pull request #6933 from pixelfed/fix/duplicate-media-attachment

Fix duplicate-key violation when importing remote media attachments

app/Util/ActivityPub/Helpers.php

@@ -25,6 +25,7 @@ use App\Services\SanitizeService;
 use App\Services\UserFilterService;
 use App\Util\Media\License;
 use Carbon\Carbon;
+use Illuminate\Database\UniqueConstraintViolationException;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\Validator;
 use Illuminate\Validation\Rule;
@@ -1141,7 +1142,9 @@ class Helpers
             }
 
             $mediaModel = self::createMediaAttachment($media, $status, $key);
-            self::handleMediaStorage($mediaModel);
+            if ($mediaModel) {
+                self::handleMediaStorage($mediaModel);
+            }
         }
 
         $status->viewType();
@@ -1170,16 +1173,35 @@ class Helpers
     }
 
     /**
-     * Create media attachment record
+     * Create media attachment record.
+     *
+     * Idempotent on the (status_id, media_path) unique key: if a row already
+     * exists (e.g. a re-fetch, an Announce racing another inbox job, or a
+     * duplicate url within one activity's attachments) the existing row is
+     * returned instead of triggering a duplicate-key violation.
+     *
+     * @return Media|null the newly created model, or null when the attachment
+     *                    already existed (so the caller can skip re-storage)
      */
-    public static function createMediaAttachment(array $media, Status $status, int $key): Media
+    public static function createMediaAttachment(array $media, Status $status, int $key): ?Media
     {
+        // Fast path: already imported for this status.
+        if (Media::whereStatusId($status->id)->whereMediaPath($media['url'])->exists()) {
+            return null;
+        }
+
         $mediaModel = new Media;
 
         self::setBasicMediaAttributes($mediaModel, $media, $status, $key);
         self::setOptionalMediaAttributes($mediaModel, $media);
 
-        $mediaModel->save();
+        try {
+            $mediaModel->save();
+        } catch (UniqueConstraintViolationException $e) {
+            // Lost a race with a concurrent inbox job that inserted the same
+            // (status_id, media_path). Treat as already-imported.
+            return null;
+        }
 
         return $mediaModel;
     }

tests/Feature/Federation/DuplicateMediaAttachmentTest.php

@@ -0,0 +1,90 @@
+<?php
+
+use App\Models\Media;
+use App\Models\Status;
+use App\Models\User;
+use App\Util\ActivityPub\Helpers;
+use Illuminate\Foundation\Testing\LazilyRefreshDatabase;
+
+uses(LazilyRefreshDatabase::class);
+
+/*
+|--------------------------------------------------------------------------
+| Duplicate media attachment on remote import
+|--------------------------------------------------------------------------
+|
+| Regression test for the 1062 duplicate-key violation on
+| media_status_id_media_path_unique when importing remote status attachments
+| (e.g. an Announce racing another inbox job, or a re-fetch). Media import
+| must be idempotent on (status_id, media_path).
+|
+*/
+
+function attachmentPayload(string $url): array
+{
+    return [
+        'type' => 'Document',
+        'mediaType' => 'image/jpeg',
+        'url' => $url,
+        'name' => 'alt text',
+        'blurhash' => 'UREVf}R:E2WB~qNKWBs.XURkxZofD+n~oJR-',
+        'width' => 768,
+        'height' => 1024,
+    ];
+}
+
+it('does not create a duplicate media row for the same status and url', function () {
+    $user = User::factory()->create();
+    $user->refresh();
+    $status = Status::factory()->create(['profile_id' => $user->profile->id, 'type' => 'photo']);
+
+    $url = 'https://files.mastodon.social/media_attachments/files/117/original/e9ab6f0314043b12.jpeg';
+    $payload = attachmentPayload($url);
+
+    $first = Helpers::createMediaAttachment($payload, $status, 0);
+    // Second call (simulating re-import / race) must not throw and must be a no-op.
+    $second = Helpers::createMediaAttachment($payload, $status, 0);
+
+    expect($first)->not->toBeNull();
+    expect($second)->toBeNull();
+    expect(Media::whereStatusId($status->id)->whereMediaPath($url)->count())->toBe(1);
+});
+
+it('creates distinct rows for different urls on the same status', function () {
+    $user = User::factory()->create();
+    $user->refresh();
+    $status = Status::factory()->create(['profile_id' => $user->profile->id, 'type' => 'photo:album']);
+
+    $a = Helpers::createMediaAttachment(attachmentPayload('https://files.mastodon.social/a.jpeg'), $status, 0);
+    $b = Helpers::createMediaAttachment(attachmentPayload('https://files.mastodon.social/b.jpeg'), $status, 1);
+
+    expect($a)->not->toBeNull();
+    expect($b)->not->toBeNull();
+    expect(Media::whereStatusId($status->id)->count())->toBe(2);
+});
+
+it('returns null when the row was inserted concurrently after the existence check', function () {
+    $user = User::factory()->create();
+    $user->refresh();
+    $status = Status::factory()->create(['profile_id' => $user->profile->id, 'type' => 'photo']);
+
+    $url = 'https://files.mastodon.social/race.jpeg';
+
+    // Pre-insert the row to simulate the concurrent winner.
+    Media::create([
+        'remote_media' => true,
+        'status_id' => $status->id,
+        'profile_id' => $status->profile_id,
+        'media_path' => $url,
+        'remote_url' => $url,
+        'mime' => 'image/jpeg',
+        'version' => 3,
+        'order' => 1,
+    ]);
+
+    // Should detect the existing row and return null without throwing.
+    $result = Helpers::createMediaAttachment(attachmentPayload($url), $status, 0);
+
+    expect($result)->toBeNull();
+    expect(Media::whereStatusId($status->id)->whereMediaPath($url)->count())->toBe(1);
+});