From b2d504f503c1824c441030dad7d07862d4a82f12 Mon Sep 17 00:00:00 2001 From: myrmidex Date: Sun, 2 Aug 2026 15:45:26 +0200 Subject: [PATCH] 123 - Prevent duplicate Lemmy posts from concurrent publish attempts --- app/Models/RouteArticle.php | 4 + .../Publishing/ArticlePublishingService.php | 38 ++++ tests/Feature/DuplicatePublishTest.php | 167 ++++++++++++++++++ tests/Unit/Models/RouteArticleTest.php | 30 ++++ .../ArticlePublishingServiceTest.php | 71 ++++++++ 5 files changed, 310 insertions(+) create mode 100644 tests/Feature/DuplicatePublishTest.php diff --git a/app/Models/RouteArticle.php b/app/Models/RouteArticle.php index 7e71b524..8b0bc42e 100644 --- a/app/Models/RouteArticle.php +++ b/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)); diff --git a/app/Services/Publishing/ArticlePublishingService.php b/app/Services/Publishing/ArticlePublishingService.php index 73079e1d..4d427fca 100644 --- a/app/Services/Publishing/ArticlePublishingService.php +++ b/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 $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 $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 diff --git a/tests/Feature/DuplicatePublishTest.php b/tests/Feature/DuplicatePublishTest.php new file mode 100644 index 00000000..a1d37646 --- /dev/null +++ b/tests/Feature/DuplicatePublishTest.php @@ -0,0 +1,167 @@ +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()); + } +} diff --git a/tests/Unit/Models/RouteArticleTest.php b/tests/Unit/Models/RouteArticleTest.php index 97b656ea..a08ee3de 100644 --- a/tests/Unit/Models/RouteArticleTest.php +++ b/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 */ diff --git a/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php b/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php index 61a8d17b..cfdfd9f4 100644 --- a/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php +++ b/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();