123 - Prevent duplicate Lemmy posts from concurrent publish attempts

app/Models/RouteArticle.php

@@ -92,6 +92,10 @@ public function isRejected(): bool
 
     public function approve(): void
     {
+        if ($this->isApproved()) {
+            return;
+        }
+
         $this->update(['approval_status' => ApprovalStatusEnum::APPROVED]);
 
         event(new RouteArticleApproved($this));

app/Services/Publishing/ArticlePublishingService.php

@@ -12,10 +12,16 @@
 use App\Modules\Lemmy\Services\LemmyPublisher;
 use App\Services\Log\LogSaver;
 use Exception;
+use Illuminate\Contracts\Cache\LockTimeoutException;
+use Illuminate\Support\Facades\Cache;
 use RuntimeException;
 
 class ArticlePublishingService
 {
+    private const LOCK_TTL_SECONDS = 180;
+
+    private const LOCK_WAIT_SECONDS = 15;
+
     public function __construct(private LogSaver $logSaver) {}
 
     /**
@@ -64,6 +70,38 @@ public function publishRouteArticle(RouteArticle $routeArticle, array $extracted
      * @param  array<string, mixed>  $extractedData
      */
     private function publishToChannel(Article $article, array $extractedData, PlatformChannel $channel, mixed $account): ?ArticlePublication
+    {
+        $lock = Cache::lock("publish:{$article->id}:{$channel->id}", self::LOCK_TTL_SECONDS);
+
+        try {
+            return $lock->block(self::LOCK_WAIT_SECONDS, function () use ($article, $extractedData, $channel, $account) {
+                $alreadyPublished = ArticlePublication::where('article_id', $article->id)
+                    ->where('platform_channel_id', $channel->id)
+                    ->exists();
+
+                if ($alreadyPublished) {
+                    $this->logSaver->info('Skipping duplicate: already published to channel', $channel, [
+                        'article_id' => $article->id,
+                    ]);
+
+                    return null;
+                }
+
+                return $this->doPublishToChannel($article, $extractedData, $channel, $account);
+            });
+        } catch (LockTimeoutException $e) {
+            $this->logSaver->info('Skipping publish: another worker holds the lock', $channel, [
+                'article_id' => $article->id,
+            ]);
+
+            return null;
+        }
+    }
+
+    /**
+     * @param  array<string, mixed>  $extractedData
+     */
+    private function doPublishToChannel(Article $article, array $extractedData, PlatformChannel $channel, mixed $account): ?ArticlePublication
     {
         try {
             // Check if this URL or title was already posted to this channel

tests/Feature/DuplicatePublishTest.php

@@ -0,0 +1,167 @@
+<?php
+
+namespace Tests\Feature;
+
+use App\Enums\PublishStatusEnum;
+use App\Events\RouteArticleApproved;
+use App\Listeners\PublishApprovedArticleListener;
+use App\Models\Article;
+use App\Models\ArticlePublication;
+use App\Models\Feed;
+use App\Models\PlatformAccount;
+use App\Models\PlatformChannel;
+use App\Models\PlatformInstance;
+use App\Models\Route;
+use App\Models\RouteArticle;
+use App\Modules\Lemmy\Services\LemmyPublisher;
+use App\Services\Article\ArticleFetcher;
+use App\Services\Log\LogSaver;
+use App\Services\Notification\NotificationService;
+use App\Services\Publishing\ArticlePublishingService;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Facades\Event;
+use Mockery;
+use Tests\TestCase;
+
+/**
+ * Reproduces #123: one article reaching a Lemmy community twice.
+ *
+ * These drive the real listener and the real publishing service, faking only
+ * the Lemmy boundary, so the guard and the lock are actually exercised.
+ */
+class DuplicatePublishTest extends TestCase
+{
+    use RefreshDatabase;
+
+    /** @var array{RouteArticle, PlatformChannel, Article} */
+    private array $fixture;
+
+    private int $remoteCalls = 0;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $feed = Feed::factory()->create();
+        $instance = PlatformInstance::factory()->create();
+        $channel = PlatformChannel::factory()->create(['platform_instance_id' => $instance->id]);
+        $account = PlatformAccount::factory()->create();
+
+        /** @var Route $route */
+        $route = Route::factory()->active()->create([
+            'feed_id' => $feed->id,
+            'platform_channel_id' => $channel->id,
+        ]);
+
+        $channel->platformAccounts()->attach($account->id, ['is_active' => true, 'priority' => 50]);
+
+        $article = Article::factory()->create(['feed_id' => $feed->id]);
+
+        /** @var RouteArticle $routeArticle */
+        $routeArticle = RouteArticle::factory()->forRoute($route)->create([
+            'article_id' => $article->id,
+        ]);
+
+        $this->fixture = [$routeArticle, $channel, $article];
+    }
+
+    protected function tearDown(): void
+    {
+        Mockery::close();
+        parent::tearDown();
+    }
+
+    /**
+     * Real publishing service with only the Lemmy call faked, counting how many
+     * posts would actually be created remotely.
+     */
+    private function makeListener(): PublishApprovedArticleListener
+    {
+        $publisher = Mockery::mock(LemmyPublisher::class);
+        $publisher->shouldReceive('publishToChannel')
+            ->andReturnUsing(function () {
+                $this->remoteCalls++;
+
+                return ['post_view' => ['post' => ['id' => 2000000 + $this->remoteCalls]]];
+            });
+
+        $service = Mockery::mock(
+            ArticlePublishingService::class,
+            [app(LogSaver::class)]
+        )->makePartial();
+        $service->shouldAllowMockingProtectedMethods();
+        $service->shouldReceive('makePublisher')->andReturn($publisher);
+
+        $fetcher = Mockery::mock(ArticleFetcher::class);
+        $fetcher->shouldReceive('fetchArticleData')->andReturn(['title' => 'Test Article']);
+
+        return new PublishApprovedArticleListener($fetcher, $service, new NotificationService);
+    }
+
+    public function test_clicking_approve_twice_creates_only_one_remote_post(): void
+    {
+        Event::fake([RouteArticleApproved::class]);
+
+        [$routeArticle] = $this->fixture;
+
+        // The double-click: approve() must not dispatch a second time.
+        $routeArticle->approve();
+        $routeArticle->approve();
+
+        Event::assertDispatchedTimes(RouteArticleApproved::class, 1);
+    }
+
+    public function test_two_queued_listeners_create_only_one_remote_post(): void
+    {
+        [$routeArticle, $channel, $article] = $this->fixture;
+
+        // Two listeners already in flight. Running them back to back would not
+        // reproduce anything — the second would see the first's publication row
+        // and stop. The real race interleaves: the second listener reaches its
+        // duplicate check while the first is still inside its Lemmy call, before
+        // any row exists. That window is what the lock has to close.
+        $publisher = Mockery::mock(LemmyPublisher::class);
+        $publisher->shouldReceive('publishToChannel')
+            ->andReturnUsing(function () use ($routeArticle) {
+                $this->remoteCalls++;
+
+                if ($this->remoteCalls === 1) {
+                    $this->makeListener()->handle(new RouteArticleApproved($routeArticle->fresh()));
+                }
+
+                return ['post_view' => ['post' => ['id' => 2000000 + $this->remoteCalls]]];
+            });
+
+        $service = Mockery::mock(
+            ArticlePublishingService::class,
+            [app(LogSaver::class)]
+        )->makePartial();
+        $service->shouldAllowMockingProtectedMethods();
+        $service->shouldReceive('makePublisher')->andReturn($publisher);
+
+        $fetcher = Mockery::mock(ArticleFetcher::class);
+        $fetcher->shouldReceive('fetchArticleData')->andReturn(['title' => 'Test Article']);
+
+        $listener = new PublishApprovedArticleListener($fetcher, $service, new NotificationService);
+        $listener->handle(new RouteArticleApproved($routeArticle));
+
+        $this->assertSame(1, $this->remoteCalls, 'Two listeners must not both post to Lemmy.');
+
+        $this->assertSame(1, ArticlePublication::where('article_id', $article->id)
+            ->where('platform_channel_id', $channel->id)
+            ->count());
+    }
+
+    public function test_a_single_approval_publishes_exactly_once(): void
+    {
+        [$routeArticle, $channel, $article] = $this->fixture;
+
+        $this->makeListener()->handle(new RouteArticleApproved($routeArticle));
+
+        $this->assertSame(1, $this->remoteCalls);
+        $this->assertSame(PublishStatusEnum::PUBLISHED, $routeArticle->fresh()->publish_status);
+        $this->assertSame(1, ArticlePublication::where('article_id', $article->id)
+            ->where('platform_channel_id', $channel->id)
+            ->count());
+    }
+}

tests/Unit/Models/RouteArticleTest.php

@@ -3,6 +3,7 @@
 namespace Tests\Unit\Models;
 
 use App\Enums\ApprovalStatusEnum;
+use App\Events\RouteArticleApproved;
 use App\Models\Article;
 use App\Models\Feed;
 use App\Models\PlatformChannel;
@@ -10,6 +11,7 @@
 use App\Models\RouteArticle;
 use Illuminate\Database\QueryException;
 use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Facades\Event;
 use Tests\TestCase;
 
 class RouteArticleTest extends TestCase
@@ -60,6 +62,34 @@ public function test_route_article_can_be_approved(): void
         $this->assertEquals(ApprovalStatusEnum::APPROVED, $routeArticle->fresh()->approval_status);
     }
 
+    public function test_approving_dispatches_the_approved_event(): void
+    {
+        Event::fake([RouteArticleApproved::class]);
+
+        /** @var RouteArticle $routeArticle */
+        $routeArticle = RouteArticle::factory()->create();
+
+        $routeArticle->approve();
+
+        Event::assertDispatchedTimes(RouteArticleApproved::class, 1);
+    }
+
+    public function test_re_approving_an_approved_article_does_not_dispatch_again(): void
+    {
+        Event::fake([RouteArticleApproved::class]);
+
+        /** @var RouteArticle $routeArticle */
+        $routeArticle = RouteArticle::factory()->create();
+
+        // A double-click, or a UI action racing an API call, calls approve() twice.
+        // The second must be a no-op: each dispatch queues a publish listener.
+        $routeArticle->approve();
+        $routeArticle->approve();
+
+        Event::assertDispatchedTimes(RouteArticleApproved::class, 1);
+        $this->assertEquals(ApprovalStatusEnum::APPROVED, $routeArticle->fresh()->approval_status);
+    }
+
     public function test_route_article_can_be_rejected(): void
     {
         /** @var RouteArticle $routeArticle */

tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php

@@ -4,6 +4,7 @@
 
 use App\Enums\PlatformEnum;
 use App\Models\Article;
+use App\Models\ArticlePublication;
 use App\Models\Feed;
 use App\Models\PlatformAccount;
 use App\Models\PlatformChannel;
@@ -15,7 +16,10 @@
 use App\Services\Log\LogSaver;
 use App\Services\Publishing\ArticlePublishingService;
 use Exception;
+use Illuminate\Contracts\Cache\Lock;
+use Illuminate\Contracts\Cache\LockTimeoutException;
 use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Facades\Cache;
 use Mockery;
 use Tests\TestCase;
 
@@ -123,6 +127,73 @@ public function test_publish_route_article_successfully_publishes(): void
         ]);
     }
 
+    public function test_concurrent_publishes_produce_only_one_remote_post(): void
+    {
+        [$routeArticle, $channel, , $article] = $this->createRouteArticleWithAccount();
+
+        $remoteCalls = 0;
+
+        // A competing listener committed its publication while this one was
+        // between its duplicate check and its own insert. The second attempt
+        // must notice and skip — the unique index cannot retract a remote post.
+        $publisherDouble = Mockery::mock(LemmyPublisher::class);
+        $publisherDouble->shouldReceive('publishToChannel')
+            ->andReturnUsing(function () use (&$remoteCalls, $article, $channel) {
+                $remoteCalls++;
+
+                ArticlePublication::create([
+                    'article_id' => $article->id,
+                    'post_id' => 999,
+                    'platform_channel_id' => $channel->id,
+                    'published_by' => 'other-worker',
+                    'published_at' => now(),
+                    'platform' => $channel->platformInstance->platform->value,
+                    'publication_data' => [],
+                ]);
+
+                return ['post_view' => ['post' => ['id' => 900 + $remoteCalls]]];
+            });
+
+        $service = Mockery::mock(ArticlePublishingService::class, [$this->logSaver])->makePartial();
+        $service->shouldAllowMockingProtectedMethods();
+        $service->shouldReceive('makePublisher')->andReturn($publisherDouble);
+
+        $service->publishRouteArticle($routeArticle, ['title' => 'Hello']);
+        $service->publishRouteArticle($routeArticle, ['title' => 'Hello']);
+
+        $this->assertSame(1, $remoteCalls, 'The remote must be called once, not once per racing listener.');
+        $this->assertSame(1, ArticlePublication::where('article_id', $article->id)
+            ->where('platform_channel_id', $channel->id)
+            ->count());
+    }
+
+    public function test_losing_the_lock_race_skips_without_publishing(): void
+    {
+        [$routeArticle, $channel, , $article] = $this->createRouteArticleWithAccount();
+
+        // Another worker holds the lock, so block() gives up and throws. Faked
+        // rather than genuinely contended, so the test does not sit out the wait.
+        $lock = Mockery::mock(Lock::class);
+        $lock->shouldReceive('block')->once()->andThrow(new LockTimeoutException);
+        Cache::shouldReceive('lock')
+            ->with("publish:{$article->id}:{$channel->id}", 180)
+            ->andReturn($lock);
+
+        $publisherDouble = Mockery::mock(LemmyPublisher::class);
+        $publisherDouble->shouldNotReceive('publishToChannel');
+
+        $service = Mockery::mock(ArticlePublishingService::class, [$this->logSaver])->makePartial();
+        $service->shouldAllowMockingProtectedMethods();
+        $service->shouldReceive('makePublisher')->andReturn($publisherDouble);
+
+        // Must decline rather than throw: a LockTimeoutException would reach the
+        // caller's catch block and be recorded as a publish failure.
+        $result = $service->publishRouteArticle($routeArticle, ['title' => 'Hello']);
+
+        $this->assertNull($result);
+        $this->assertDatabaseCount('article_publications', 0);
+    }
+
     public function test_publish_route_article_handles_publishing_failure_gracefully(): void
     {
         [$routeArticle] = $this->createRouteArticleWithAccount();