fix: stop double-dispatching transaction listeners (N+1 insert into jobs) (#620)
## What & why Fixes the Sentry N+1 **PHP-LARAVEL-3V** — `N+1 insert into jobs` on `POST /transactions`. ### Root cause Laravel's **event discovery is on by default** (the base `EventServiceProvider` sets `$shouldDiscoverEvents = true`, and this app has no subclass to turn it off), so every listener in `app/Listeners` is already wired to its event by its `handle()` type-hint. `AppServiceProvider::boot()` was **also** registering six of them with `Event::listen(...)`, so each ended up registered **twice** — every queued listener published its job twice per event. Concretely, creating a transaction dispatched `AssignTransactionToBudget` and `CategorizeTransactionWithAi` twice each → repeated `insert into jobs` in one request. (The same bug made `PostStripeEventToDiscord` post to Discord twice per webhook, `ApplyAutomationRules` run twice, etc.) ## Changes 1. **`AppServiceProvider`** — drop the redundant explicit `Event::listen` calls and rely on discovery, which already wires every listener exactly once (`ScheduleDripEmailsListener` and `SyncUserToResendListener` were discovery-only already). Verified with `php artisan event:list`: each event now has a single listener, and the synchronous `ApplyAutomationRules` still runs first on `TransactionCreated`. 2. **`CategorizeTransactionWithAi`** — add `shouldQueue()` so the job isn't even published when it would immediately no-op (already categorized by a rule, encrypted, or user not AI-eligible — the default, since AI is opt-in). The same guard still runs in `handle()` because state can change while the job waits. Together these remove the second `insert into jobs` on transaction creation for the common (non-AI) path. ## Tests - New `tests/Feature/Ai/CategorizeTransactionWithAiListenerTest.php`: the AI job is queued **once** for an eligible uncategorized transaction, and **not at all** when the user has no AI consent or the transaction is already categorized. - Full suite: **1828 passing**. The one failure (`DashboardTest` → 409) is **pre-existing on `main`** (Inertia asset-version mismatch from a local build manifest) and reproduces with these changes reverted — unrelated to this PR. Fixes PHP-LARAVEL-3V
This commit is contained in:
parent
4120e12861
commit
0f8eca50d0
|
|
@ -3,6 +3,7 @@
|
|||
namespace App\Listeners;
|
||||
|
||||
use App\Events\TransactionCreated;
|
||||
use App\Models\Transaction;
|
||||
use App\Services\Ai\AiCategorizationGate;
|
||||
use App\Services\Ai\AiCategorizer;
|
||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||
|
|
@ -31,22 +32,44 @@ class CategorizeTransactionWithAi implements ShouldQueue
|
|||
return (string) config('ai_categorization.queue');
|
||||
}
|
||||
|
||||
/**
|
||||
* Avoid publishing a job that would immediately no-op. In the common case the
|
||||
* transaction is already categorized by the synchronous automation rules that
|
||||
* run before this listener, or the user simply isn't AI-eligible, so queuing
|
||||
* would only cost a wasted jobs-table insert and a wasted worker dequeue.
|
||||
*/
|
||||
public function shouldQueue(TransactionCreated $event): bool
|
||||
{
|
||||
return $this->shouldCategorize($event->transaction);
|
||||
}
|
||||
|
||||
public function handle(TransactionCreated $event): void
|
||||
{
|
||||
$transaction = $event->transaction;
|
||||
|
||||
if ($transaction->category_id !== null) {
|
||||
// Re-check at run time: eligibility or the category may have changed while
|
||||
// the job waited in the queue.
|
||||
if (! $this->shouldCategorize($transaction)) {
|
||||
return;
|
||||
}
|
||||
|
||||
$this->categorizer->run($transaction->user, new Collection([$transaction]));
|
||||
}
|
||||
|
||||
private function shouldCategorize(Transaction $transaction): bool
|
||||
{
|
||||
if ($transaction->category_id !== null) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if ($transaction->description_iv !== null) {
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
|
||||
$user = $transaction->user;
|
||||
|
||||
if ($user === null) {
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
|
||||
// Transactions imported during onboarding are deliberately skipped here:
|
||||
|
|
@ -54,13 +77,9 @@ class CategorizeTransactionWithAi implements ShouldQueue
|
|||
// end of onboarding, and whatever is left is categorized in a single batch
|
||||
// pass once onboarding completes (CategorizeOnboardingTransactionsJob).
|
||||
if (! $user->isOnboarded()) {
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
|
||||
if (! $this->gate->allows($user)) {
|
||||
return;
|
||||
}
|
||||
|
||||
$this->categorizer->run($user, new Collection([$transaction]));
|
||||
return $this->gate->allows($user);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -3,29 +3,17 @@
|
|||
namespace App\Providers;
|
||||
|
||||
use App\Contracts\BankingProviderInterface;
|
||||
use App\Events\TransactionCreated;
|
||||
use App\Events\TransactionDeleted;
|
||||
use App\Events\TransactionUpdated;
|
||||
use App\Http\Responses\RegisterResponse;
|
||||
use App\Listeners\ApplyAutomationRules;
|
||||
use App\Listeners\AssignTransactionToBudget;
|
||||
use App\Listeners\CategorizeTransactionWithAi;
|
||||
use App\Listeners\PostStripeEventToDiscord;
|
||||
use App\Listeners\UnassignTransactionFromBudget;
|
||||
use App\Listeners\UpdateLastLoggedInAt;
|
||||
use App\Services\Ai\Contracts\RuleSuggestionGenerator;
|
||||
use App\Services\Ai\Contracts\TransactionMatcher;
|
||||
use App\Services\Ai\LaravelAiRuleSuggestionGenerator;
|
||||
use App\Services\Ai\UncategorizedTransactionMatcher;
|
||||
use App\Services\Banking\EnableBankingProvider;
|
||||
use App\Services\Discord\DiscordWebhook;
|
||||
use Illuminate\Auth\Events\Login;
|
||||
use Illuminate\Cache\RateLimiting\Limit;
|
||||
use Illuminate\Support\Facades\Event;
|
||||
use Illuminate\Support\Facades\RateLimiter;
|
||||
use Illuminate\Support\ServiceProvider;
|
||||
use Laravel\Cashier\Cashier;
|
||||
use Laravel\Cashier\Events\WebhookReceived;
|
||||
use Laravel\Fortify\Contracts\RegisterResponse as RegisterResponseContract;
|
||||
|
||||
class AppServiceProvider extends ServiceProvider
|
||||
|
|
@ -59,14 +47,11 @@ class AppServiceProvider extends ServiceProvider
|
|||
*/
|
||||
public function boot(): void
|
||||
{
|
||||
Event::listen(TransactionCreated::class, ApplyAutomationRules::class);
|
||||
Event::listen(TransactionCreated::class, AssignTransactionToBudget::class);
|
||||
Event::listen(TransactionCreated::class, CategorizeTransactionWithAi::class);
|
||||
Event::listen(TransactionUpdated::class, AssignTransactionToBudget::class);
|
||||
Event::listen(TransactionDeleted::class, UnassignTransactionFromBudget::class);
|
||||
Event::listen(WebhookReceived::class, PostStripeEventToDiscord::class);
|
||||
Event::listen(Login::class, UpdateLastLoggedInAt::class);
|
||||
|
||||
// Event listeners are registered automatically via Laravel's event
|
||||
// discovery of the App\Listeners directory (matched by their handle()
|
||||
// type-hint). Do not also register them explicitly here — doing both
|
||||
// registers every listener twice, so each queued listener is dispatched
|
||||
// twice per event.
|
||||
RateLimiter::for('emails', function (object $job): Limit {
|
||||
return Limit::perSecond(30);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -0,0 +1,63 @@
|
|||
<?php
|
||||
|
||||
use App\Listeners\CategorizeTransactionWithAi;
|
||||
use App\Models\Category;
|
||||
use App\Models\Transaction;
|
||||
use App\Models\User;
|
||||
use Illuminate\Events\CallQueuedListener;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
|
||||
function aiEligibleUser(): User
|
||||
{
|
||||
$user = User::factory()->onboarded()->create();
|
||||
$user->recordAiConsent();
|
||||
|
||||
return $user;
|
||||
}
|
||||
|
||||
function aiCategorizationJobsQueued(): int
|
||||
{
|
||||
return Queue::pushed(CallQueuedListener::class)
|
||||
->filter(fn (CallQueuedListener $job): bool => $job->class === CategorizeTransactionWithAi::class)
|
||||
->count();
|
||||
}
|
||||
|
||||
it('queues an AI categorization job for an eligible, uncategorized transaction', function () {
|
||||
$user = aiEligibleUser();
|
||||
|
||||
Queue::fake();
|
||||
|
||||
Transaction::factory()->plaintext()->create([
|
||||
'user_id' => $user->id,
|
||||
'category_id' => null,
|
||||
]);
|
||||
|
||||
expect(aiCategorizationJobsQueued())->toBe(1);
|
||||
});
|
||||
|
||||
it('does not queue an AI categorization job when the user has no AI consent', function () {
|
||||
$user = User::factory()->onboarded()->create();
|
||||
|
||||
Queue::fake();
|
||||
|
||||
Transaction::factory()->plaintext()->create([
|
||||
'user_id' => $user->id,
|
||||
'category_id' => null,
|
||||
]);
|
||||
|
||||
expect(aiCategorizationJobsQueued())->toBe(0);
|
||||
});
|
||||
|
||||
it('does not queue an AI categorization job when the transaction is already categorized', function () {
|
||||
$user = aiEligibleUser();
|
||||
$category = Category::factory()->for($user)->create();
|
||||
|
||||
Queue::fake();
|
||||
|
||||
Transaction::factory()->plaintext()->create([
|
||||
'user_id' => $user->id,
|
||||
'category_id' => $category->id,
|
||||
]);
|
||||
|
||||
expect(aiCategorizationJobsQueued())->toBe(0);
|
||||
});
|
||||
|
|
@ -0,0 +1,33 @@
|
|||
<?php
|
||||
|
||||
use App\Events\TransactionCreated;
|
||||
use App\Events\TransactionDeleted;
|
||||
use App\Events\TransactionUpdated;
|
||||
use App\Listeners\AssignTransactionToBudget;
|
||||
use App\Listeners\UnassignTransactionFromBudget;
|
||||
use App\Models\Transaction;
|
||||
use App\Models\User;
|
||||
use Illuminate\Events\CallQueuedListener;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
|
||||
function queuedListenerCount(string $listener): int
|
||||
{
|
||||
return Queue::pushed(CallQueuedListener::class)
|
||||
->filter(fn (CallQueuedListener $job): bool => $job->class === $listener)
|
||||
->count();
|
||||
}
|
||||
|
||||
it('registers each transaction listener exactly once', function (string $event, string $listener): void {
|
||||
$user = User::factory()->onboarded()->create();
|
||||
$transaction = Transaction::factory()->plaintext()->create(['user_id' => $user->id]);
|
||||
|
||||
Queue::fake();
|
||||
|
||||
event(new $event($transaction));
|
||||
|
||||
expect(queuedListenerCount($listener))->toBe(1);
|
||||
})->with([
|
||||
'created dispatches budget assignment once' => [TransactionCreated::class, AssignTransactionToBudget::class],
|
||||
'updated dispatches budget assignment once' => [TransactionUpdated::class, AssignTransactionToBudget::class],
|
||||
'deleted dispatches budget unassignment once' => [TransactionDeleted::class, UnassignTransactionFromBudget::class],
|
||||
]);
|
||||
Loading…
Reference in New Issue