From 05d4bae0af1fece9196fabde61a624cf19472da9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Sun, 5 Jul 2026 11:31:23 +0200 Subject: [PATCH] fix(queue): raise retry_after above the longest job timeout (PHP-LARAVEL-2D) (#645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What & why Fixes **PHP-LARAVEL-2D** — `MaxAttemptsExceededException: CategorizeUncategorizedTransactionsJob has been attempted too many times.` (recurring, still firing as of yesterday, ~18 events). ### Root cause The `database` queue connection used `retry_after = 90s`, but several jobs declare a `$timeout` far above it (up to **600s**). `retry_after` is **per-connection**. When a job outlives its 90s reservation, the DB queue driver hands it to a second worker. For a `tries = 1` job that immediately raises `MaxAttemptsExceededException` — and, worse, the body can execute twice. `CategorizeUncategorizedTransactionsJob` runs on the `ai` queue, which the deploy runs with **two concurrent workers** (`docker/supervisor/supervisord.conf`), so the double-run re-bills Gemini and resets progress — the exact harm its `tries = 1` was meant to prevent. At `retry_after = 90` **8 jobs** violated the invariant (600s rule jobs, 300s AI jobs, 120s sync jobs). ## Changes (one concern per commit) 1. **fix(queue): raise `retry_after` above the longest job timeout** — default `90 → 900s` (env-overridable via `DB_QUEUE_RETRY_AFTER`), clearing the 600s longest job with margin. Adds `QueueConfigTest` asserting every `app/Jobs` `$timeout` stays `< retry_after` so a new long job can't silently re-break it. 2. **fix(ai): de-duplicate the backfill per user** — `retry_after` stops one dispatch being re-reserved, but not a *duplicate* dispatch (double "Enable AI" click / re-enable mid-run) on the 2-worker `ai` queue. Implement `ShouldBeUnique` keyed on user id, mirroring the sibling `RetryTransientAiCategorizationJob`. 3. **test(queue): widen the guard** to also scan queued Mailables (`app/Mail`) and Notifications (`app/Notifications`), which share the connection and honor `$timeout`. 4. **docs(queue): correct the `retry_after` comment** — per-job `$timeout` takes precedence over the worker `--timeout` (which is currently unset), so the earlier "must sit between" phrasing was inaccurate. ## Verification - `QueueConfigTest` fails at the old `retry_after=90` (8 offenders) and passes at `900`. - `vendor/bin/pint --test` clean; 131 tests across `tests/Feature/Ai`, AI-consent flow, and queue config pass. - Confirmed against the real deploy: 2 workers on the `ai` queue, `pcntl` installed (SIGALRM enforced), workers set no `--timeout` so each job's own `$timeout` bounds execution below 900s. ## Reviewed by two independent agents (architecture + product) Both concluded **ship it**. The double-dispatch gap they flagged is closed by change #2. Neither found a correctness regression. ## Operational notes (no code change needed, flagged for awareness) - **Reclaim window:** `retry_after` is per-connection, so `emails`/`default` also inherit the 90→900s window. A job orphaned by a *hard* worker crash (OOM/SIGKILL) is now reclaimed after up to ~15 min instead of ~90s. Graceful deploys (SIGTERM + `stopwaitsecs=3600`) don't orphan jobs, so this only bites on hard crashes — an acceptable trade. A dedicated connection for the long AI queue would remove it; left as a possible follow-up. - **Worker `--timeout`:** the fix relies on `pcntl` staying installed and long jobs keeping a `$timeout`. Setting an explicit `--timeout` (< 900) on the supervisord `queue:work` commands would make it self-enforcing; not done here to avoid changing short-job hang detection. - If prod sets `DB_QUEUE_RETRY_AFTER` explicitly, ensure it's above the longest job timeout (≥ 600, ideally 900). --- ...CategorizeUncategorizedTransactionsJob.php | 20 ++++++++- config/queue.php | 9 +++- ...ategorizeUncategorizedTransactionsTest.php | 9 ++++ tests/Feature/QueueConfigTest.php | 41 +++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/QueueConfigTest.php diff --git a/app/Jobs/CategorizeUncategorizedTransactionsJob.php b/app/Jobs/CategorizeUncategorizedTransactionsJob.php index dcea2e7d..c39694f9 100644 --- a/app/Jobs/CategorizeUncategorizedTransactionsJob.php +++ b/app/Jobs/CategorizeUncategorizedTransactionsJob.php @@ -5,6 +5,7 @@ namespace App\Jobs; use App\Models\User; use App\Services\Ai\AiCategorizationGate; use App\Services\Ai\AiCategorizer; +use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; use Illuminate\Support\Facades\Cache; @@ -15,11 +16,17 @@ use Throwable; * consent outside of onboarding. Progress is written to the cache so the * transactions page can poll it and surface live progress while the batch runs. * + * De-duplicated per user: a second dispatch (double "Enable AI" click, or + * re-enabling consent while a run is still in flight) would read the same + * pending-transaction snapshot on a concurrent worker and re-bill the model for + * work already underway — the exact harm tries=1 targets but cannot prevent, + * since tries only bounds re-attempts of one dispatch, not duplicate dispatches. + * * ponytail: mirrors CategorizeOnboardingTransactionsJob's selection + chunking; * kept separate so the onboarding pass stays progress-free. Fold the two * together if a third caller ever needs the same loop. */ -class CategorizeUncategorizedTransactionsJob implements ShouldQueue +class CategorizeUncategorizedTransactionsJob implements ShouldBeUnique, ShouldQueue { use Queueable; @@ -34,8 +41,19 @@ class CategorizeUncategorizedTransactionsJob implements ShouldQueue */ public int $tries = 1; + /** + * Safety TTL for the unique lock in case a worker dies mid-run; comfortably + * longer than a full run. + */ + public int $uniqueFor = 1800; + public function __construct(public User $user, public string $jobId) {} + public function uniqueId(): string + { + return $this->user->id; + } + public function viaQueue(): string { return (string) config('ai_categorization.queue'); diff --git a/config/queue.php b/config/queue.php index 4fe599aa..bcc38761 100644 --- a/config/queue.php +++ b/config/queue.php @@ -40,7 +40,14 @@ return [ 'connection' => env('DB_QUEUE_CONNECTION'), 'table' => env('DB_QUEUE_TABLE', 'jobs'), 'queue' => env('DB_QUEUE', 'default'), - 'retry_after' => (int) env('DB_QUEUE_RETRY_AFTER', 90), + // retry_after must exceed the longest job $timeout on this connection + // (currently 600s). Otherwise a long job outlives its reservation, the + // queue hands it to a second worker, and a tries=1 job dies with + // MaxAttemptsExceededException (and re-runs its side effects). Each + // job's execution ceiling — its own $timeout, or the worker --timeout + // when a job declares none — must stay below this value. QueueConfigTest + // guards the $timeout side. + 'retry_after' => (int) env('DB_QUEUE_RETRY_AFTER', 900), 'after_commit' => true, ], diff --git a/tests/Feature/Ai/CategorizeUncategorizedTransactionsTest.php b/tests/Feature/Ai/CategorizeUncategorizedTransactionsTest.php index 854025e6..9edf1be3 100644 --- a/tests/Feature/Ai/CategorizeUncategorizedTransactionsTest.php +++ b/tests/Feature/Ai/CategorizeUncategorizedTransactionsTest.php @@ -9,6 +9,7 @@ use App\Models\Transaction; use App\Models\User; use App\Services\Ai\CategorizeTransactions; use App\Services\Ai\CategoryCatalog; +use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Support\Facades\Bus; use Illuminate\Support\Facades\Cache; @@ -218,3 +219,11 @@ it('categorizes the most recent transactions first', function () { expect($order)->toBe([$newest->id, $middle->id, $oldest->id]); }); + +it('de-duplicates the backfill per user so a concurrent dispatch cannot double-bill', function () { + $user = User::factory()->create(); + $job = new CategorizeUncategorizedTransactionsJob($user, 'job-x'); + + expect($job)->toBeInstanceOf(ShouldBeUnique::class) + ->and($job->uniqueId())->toBe($user->id); +}); diff --git a/tests/Feature/QueueConfigTest.php b/tests/Feature/QueueConfigTest.php new file mode 100644 index 00000000..55b3e4f6 --- /dev/null +++ b/tests/Feature/QueueConfigTest.php @@ -0,0 +1,41 @@ +flatMap(fn (string $dir): array => File::allFiles(app_path($dir))) + ->map(function ($file): string { + $relative = Str::of($file->getPathname()) + ->after(app_path().DIRECTORY_SEPARATOR) + ->replace(DIRECTORY_SEPARATOR, '\\') + ->replaceLast('.php', ''); + + return 'App\\'.$relative; + }) + ->filter(fn (string $class): bool => class_exists($class)) + ->mapWithKeys(function (string $class): array { + $timeout = (new ReflectionClass($class))->getDefaultProperties()['timeout'] ?? null; + + return [$class => $timeout]; + }) + ->filter(fn ($timeout): bool => is_int($timeout) && $timeout >= $retryAfter); + + expect($offenders->all())->toBe( + [], + 'These jobs have a $timeout >= retry_after ('.$retryAfter.'s); raise DB_QUEUE_RETRY_AFTER above the longest job timeout.' + ); +});