From ddd6514a77fb4c19c51f0dabf861440b2cfd8cfa Mon Sep 17 00:00:00 2001 From: myrmidex Date: Mon, 10 Aug 2026 00:05:17 +0200 Subject: [PATCH] 119 - Back off and give up on repeatedly failing publishes --- app/Actions/PublishRouteArticleAction.php | 4 + app/Jobs/PublishNextArticleJob.php | 5 ++ app/Models/RouteArticle.php | 31 ++++++++ ...blish_retry_tracking_to_route_articles.php | 26 +++++++ .../Actions/PublishRouteArticleActionTest.php | 43 +++++++++++ tests/Unit/Jobs/PublishNextArticleJobTest.php | 67 ++++++++++++++++ tests/Unit/Models/RouteArticleRetryTest.php | 76 +++++++++++++++++++ 7 files changed, 252 insertions(+) create mode 100644 database/migrations/2024_01_01_000018_add_publish_retry_tracking_to_route_articles.php create mode 100644 tests/Unit/Models/RouteArticleRetryTest.php diff --git a/app/Actions/PublishRouteArticleAction.php b/app/Actions/PublishRouteArticleAction.php index 46357425..cb2b3180 100644 --- a/app/Actions/PublishRouteArticleAction.php +++ b/app/Actions/PublishRouteArticleAction.php @@ -41,6 +41,7 @@ public function execute(RouteArticle $routeArticle): PublishOutcome : PublishOutcome::failure('Could not recover the article content to publish'); } catch (Exception $e) { $routeArticle->update(['publish_status' => PublishStatusEnum::ERROR]); + $routeArticle->recordPublishAttemptFailed(); ActionPerformed::dispatch('Failed to publish article', LogLevelEnum::ERROR, [ 'article_id' => $article->id, @@ -94,6 +95,7 @@ private function resolvePublishData(Article $article): array private function recordPublished(RouteArticle $routeArticle): void { $routeArticle->update(['publish_status' => PublishStatusEnum::PUBLISHED]); + $routeArticle->clearPublishAttempts(); ActionPerformed::dispatch('Published article', LogLevelEnum::INFO, [ 'article_id' => $routeArticle->article->id, @@ -117,11 +119,13 @@ private function recordFailed(RouteArticle $routeArticle, PublishOutcome $outcom $article = $routeArticle->article; $routeArticle->update(['publish_status' => PublishStatusEnum::ERROR]); + $routeArticle->recordPublishAttemptFailed(); ActionPerformed::dispatch('No publication created for article', LogLevelEnum::WARNING, [ 'article_id' => $article->id, 'title' => $article->title, 'reason' => $outcome->reason, + 'attempt' => $routeArticle->publish_attempts, ]); $this->notificationService->send( diff --git a/app/Jobs/PublishNextArticleJob.php b/app/Jobs/PublishNextArticleJob.php index e4ce99c7..e667ca65 100644 --- a/app/Jobs/PublishNextArticleJob.php +++ b/app/Jobs/PublishNextArticleJob.php @@ -47,6 +47,11 @@ public function handle(PublishRouteArticleAction $publishRouteArticle): void // Get the oldest approved route_article that hasn't been published to its channel yet $routeArticle = RouteArticle::where('approval_status', ApprovalStatusEnum::APPROVED) + ->where('publish_attempts', '<', RouteArticle::MAX_PUBLISH_ATTEMPTS) + ->where(function ($query) { + $query->whereNull('next_attempt_at') + ->orWhere('next_attempt_at', '<=', now()); + }) ->whereDoesntHave('article.articlePublications', function ($query) { $query->whereColumn('article_publications.platform_channel_id', 'route_articles.platform_channel_id'); }) diff --git a/app/Models/RouteArticle.php b/app/Models/RouteArticle.php index 8b0bc42e..999c61cd 100644 --- a/app/Models/RouteArticle.php +++ b/app/Models/RouteArticle.php @@ -18,6 +18,8 @@ * @property int $article_id * @property ApprovalStatusEnum $approval_status * @property PublishStatusEnum $publish_status + * @property int $publish_attempts + * @property Carbon|null $next_attempt_at * @property Carbon|null $validated_at * @property Carbon $created_at * @property Carbon $updated_at @@ -33,12 +35,16 @@ class RouteArticle extends Model 'article_id', 'approval_status', 'publish_status', + 'publish_attempts', + 'next_attempt_at', 'validated_at', ]; protected $casts = [ 'approval_status' => ApprovalStatusEnum::class, 'publish_status' => PublishStatusEnum::class, + 'publish_attempts' => 'integer', + 'next_attempt_at' => 'datetime', 'validated_at' => 'datetime', ]; @@ -105,4 +111,29 @@ public function reject(): void { $this->update(['approval_status' => ApprovalStatusEnum::REJECTED]); } + + private const RETRY_BACKOFF_MINUTES = [5, 30, 120, 360]; + + public const MAX_PUBLISH_ATTEMPTS = 4; + + public function recordPublishAttemptFailed(): void + { + $attempts = $this->publish_attempts + 1; + $backoff = self::RETRY_BACKOFF_MINUTES; + + $this->update([ + 'publish_attempts' => $attempts, + 'next_attempt_at' => now()->addMinutes($backoff[$attempts - 1] ?? end($backoff)), + ]); + } + + public function clearPublishAttempts(): void + { + $this->update(['publish_attempts' => 0, 'next_attempt_at' => null]); + } + + public function hasExhaustedPublishAttempts(): bool + { + return $this->publish_attempts >= self::MAX_PUBLISH_ATTEMPTS; + } } diff --git a/database/migrations/2024_01_01_000018_add_publish_retry_tracking_to_route_articles.php b/database/migrations/2024_01_01_000018_add_publish_retry_tracking_to_route_articles.php new file mode 100644 index 00000000..0e7e3a9e --- /dev/null +++ b/database/migrations/2024_01_01_000018_add_publish_retry_tracking_to_route_articles.php @@ -0,0 +1,26 @@ +unsignedTinyInteger('publish_attempts')->default(0)->after('publish_status'); + $table->timestamp('next_attempt_at')->nullable()->after('publish_attempts'); + + $table->index('next_attempt_at'); + }); + } + + public function down(): void + { + Schema::table('route_articles', function (Blueprint $table) { + $table->dropIndex(['next_attempt_at']); + $table->dropColumn(['publish_attempts', 'next_attempt_at']); + }); + } +}; diff --git a/tests/Unit/Actions/PublishRouteArticleActionTest.php b/tests/Unit/Actions/PublishRouteArticleActionTest.php index 5a805bd5..1919b5ce 100644 --- a/tests/Unit/Actions/PublishRouteArticleActionTest.php +++ b/tests/Unit/Actions/PublishRouteArticleActionTest.php @@ -177,6 +177,49 @@ public function test_publish_fails_when_fallback_fetch_returns_nothing(): void ]); } + public function test_repeated_failures_exhaust_the_retry_attempts(): void + { + $routeArticle = $this->createRouteArticle([], unvalidated: true); + + $fetcher = Mockery::mock(ArticleFetcher::class); + $fetcher->shouldReceive('fetchArticleData')->andReturn([]); + + $publishingService = Mockery::mock(ArticlePublishingService::class); + $action = new PublishRouteArticleAction($fetcher, $publishingService, new NotificationService); + + for ($i = 0; $i < RouteArticle::MAX_PUBLISH_ATTEMPTS; $i++) { + $action->execute($routeArticle); + $routeArticle->refresh(); + } + + $this->assertSame(RouteArticle::MAX_PUBLISH_ATTEMPTS, $routeArticle->publish_attempts); + $this->assertTrue($routeArticle->hasExhaustedPublishAttempts()); + } + + public function test_a_successful_publish_resets_earlier_failed_attempts(): void + { + $routeArticle = $this->createRouteArticle([ + 'description' => 'Stored description', + ]); + $routeArticle->update(['publish_attempts' => 2, 'next_attempt_at' => now()->subMinute()]); + + $fetcher = Mockery::mock(ArticleFetcher::class); + $fetcher->shouldNotReceive('fetchArticleData'); + + $publishingService = Mockery::mock(ArticlePublishingService::class); + $publishingService->shouldReceive('publishRouteArticle') + ->once() + ->andReturn(PublishOutcome::published($this->makePublication())); + + (new PublishRouteArticleAction($fetcher, $publishingService, new NotificationService)) + ->execute($routeArticle); + + $routeArticle->refresh(); + + $this->assertSame(0, $routeArticle->publish_attempts); + $this->assertNull($routeArticle->next_attempt_at); + } + public function test_publish_fails_when_fallback_recovers_only_a_title(): void { $routeArticle = $this->createRouteArticle([], unvalidated: true); diff --git a/tests/Unit/Jobs/PublishNextArticleJobTest.php b/tests/Unit/Jobs/PublishNextArticleJobTest.php index 63adbef7..3823c7a3 100644 --- a/tests/Unit/Jobs/PublishNextArticleJobTest.php +++ b/tests/Unit/Jobs/PublishNextArticleJobTest.php @@ -5,6 +5,7 @@ use App\Actions\PublishRouteArticleAction; use App\Enums\NotificationSeverityEnum; use App\Enums\NotificationTypeEnum; +use App\Enums\PublishStatusEnum; use App\Exceptions\PublishException; use App\Jobs\PublishNextArticleJob; use App\Models\Article; @@ -398,6 +399,72 @@ public function test_handle_creates_notification_on_publish_exception(): void $this->assertStringContainsString('Failing Article', $notification->title); } + public function test_handle_skips_route_articles_that_are_not_due_for_retry(): void + { + $routeArticle = $this->createApprovedRouteArticle(); + $routeArticle->update(['publish_attempts' => 1, 'next_attempt_at' => now()->addMinutes(5)]); + + $articleFetcherMock = Mockery::mock(ArticleFetcher::class); + $articleFetcherMock->shouldNotReceive('fetchArticleData'); + + $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); + $publishingServiceMock->shouldNotReceive('publishRouteArticle'); + + $job = new PublishNextArticleJob; + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); + + $this->assertSame(PublishStatusEnum::UNPUBLISHED, $routeArticle->fresh()->publish_status); + } + + public function test_handle_skips_route_articles_that_exhausted_their_attempts(): void + { + $routeArticle = $this->createApprovedRouteArticle(); + $routeArticle->update([ + 'publish_attempts' => RouteArticle::MAX_PUBLISH_ATTEMPTS, + 'next_attempt_at' => now()->subDay(), + ]); + + $articleFetcherMock = Mockery::mock(ArticleFetcher::class); + $articleFetcherMock->shouldNotReceive('fetchArticleData'); + + $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); + $publishingServiceMock->shouldNotReceive('publishRouteArticle'); + + $job = new PublishNextArticleJob; + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); + + $this->assertSame(PublishStatusEnum::UNPUBLISHED, $routeArticle->fresh()->publish_status); + } + + public function test_handle_publishes_a_later_article_when_the_oldest_is_backing_off(): void + { + $blocked = $this->createApprovedRouteArticle(['title' => 'Blocked Article']); + $blocked->update([ + 'created_at' => now()->subDays(2), + 'publish_attempts' => 1, + 'next_attempt_at' => now()->addMinutes(5), + ]); + + $next = $this->createApprovedRouteArticle(['title' => 'Next Article']); + $next->update(['created_at' => now()->subDay()]); + + $articleFetcherMock = Mockery::mock(ArticleFetcher::class); + $articleFetcherMock->shouldReceive('fetchArticleData') + ->once() + ->andReturn(['title' => 'Next Article', 'description' => 'Test description']); + + $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); + $publishingServiceMock->shouldReceive('publishRouteArticle') + ->once() + ->with(Mockery::on(fn ($ra) => $ra->id === $next->id), Mockery::any()) + ->andReturn(PublishOutcome::published($this->makePublication())); + + $job = new PublishNextArticleJob; + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); + + $this->assertSame(PublishStatusEnum::PUBLISHED, $next->fresh()->publish_status); + } + public function test_job_can_be_serialized(): void { $job = new PublishNextArticleJob; diff --git a/tests/Unit/Models/RouteArticleRetryTest.php b/tests/Unit/Models/RouteArticleRetryTest.php new file mode 100644 index 00000000..5ba2ed0f --- /dev/null +++ b/tests/Unit/Models/RouteArticleRetryTest.php @@ -0,0 +1,76 @@ +approved()->create(); + + $this->freezeTime(function () use ($routeArticle) { + $routeArticle->recordPublishAttemptFailed(); + + $this->assertSame(1, $routeArticle->publish_attempts); + $this->assertSame( + now()->addMinutes(5)->format('Y-m-d H:i:s'), + $routeArticle->next_attempt_at->format('Y-m-d H:i:s') + ); + }); + } + + public function test_backoff_grows_with_each_failure(): void + { + /** @var RouteArticle $routeArticle */ + $routeArticle = RouteArticle::factory()->approved()->create(); + + $this->freezeTime(function () use ($routeArticle) { + foreach ([5, 30, 120, 360] as $expectedDelay) { + $routeArticle->recordPublishAttemptFailed(); + + $this->assertSame( + now()->addMinutes($expectedDelay)->format('Y-m-d H:i:s'), + $routeArticle->next_attempt_at->format('Y-m-d H:i:s'), + "Attempt {$routeArticle->publish_attempts} should wait {$expectedDelay} minutes" + ); + } + }); + } + + public function test_attempts_are_exhausted_after_the_configured_maximum(): void + { + /** @var RouteArticle $routeArticle */ + $routeArticle = RouteArticle::factory()->approved()->create(); + + for ($i = 0; $i < RouteArticle::MAX_PUBLISH_ATTEMPTS - 1; $i++) { + $routeArticle->recordPublishAttemptFailed(); + $this->assertFalse($routeArticle->hasExhaustedPublishAttempts()); + } + + $routeArticle->recordPublishAttemptFailed(); + + $this->assertTrue($routeArticle->hasExhaustedPublishAttempts()); + } + + public function test_a_successful_publish_clears_previous_attempts(): void + { + /** @var RouteArticle $routeArticle */ + $routeArticle = RouteArticle::factory()->approved()->create(); + + $routeArticle->recordPublishAttemptFailed(); + $routeArticle->recordPublishAttemptFailed(); + + $routeArticle->clearPublishAttempts(); + + $this->assertSame(0, $routeArticle->publish_attempts); + $this->assertNull($routeArticle->next_attempt_at); + $this->assertFalse($routeArticle->hasExhaustedPublishAttempts()); + } +}