perf(ai): reduce N+1 in bulk category updates (PHP-LARAVEL-40, partial) (#624)
> **Draft on purpose — a partial fix + a design for the full one.** These commits are safe, behavior-preserving reductions of the N+1, but they do **not** fully resolve PHP-LARAVEL-40. The complete fix is a batch-aware refactor of the AI rule-learning path, which is delicate (getting it wrong silently mis-categorizes users' transactions). I've written the design below for a human to implement and diff against the current per-transaction behavior. Current user impact is low (~170 ms request, 1 event), so there's no urgency to rush the risky part. ## What Sentry **PHP-LARAVEL-40** — N+1 in `TransactionController@bulkUpdate` (PATCH `/transactions/bulk`). A bulk category update calls `CategoryOverrideHandler::record()` once per transaction, and each call runs the full AI rule-learning pipeline (`AiRuleLearner::forgetFromAiRules()` + `learnFromCorrection()`): loading all the user's AI rules, plucking every description, running matcher `count(*)` probes, and inserting a `category_corrections` row. For a 112-transaction batch that's ~112× each. The offending span is the per-transaction `category_corrections` insert. ## What this branch does (safe, partial) Two behavior-preserving reductions, each validated by the existing learning tests: 1. `perf(transactions): resolve the override handler once for bulk updates` — `bulkUpdate()` re-resolved `CategoryOverrideHandler` from the container every iteration. Resolve it once (also required for #2 to take effect). 2. `perf(ai): memoize the description corpus per user in AiRuleLearner` — `learnFromCorrection` reloaded and re-tokenized every one of the user's descriptions on each transaction. The corpus is immutable while only categories change, so memoize the document-frequency map + count per user. Removes one `SELECT` (and its tokenization) per transaction. 3. `test(ai): assert batch corrections learn correctly through the memoized corpus` — proves the second, memoized-corpus learning still yields a correct, distinct clause (not just that the query count dropped). 4. `test(ai): guard AiRuleLearner against a singleton binding` — the cache has no invalidation and is only safe while the learner is resolved fresh per request; a test now fails if it is ever bound singleton. ## What it does NOT do It does **not** resolve the flagged N+1. Still per-transaction: the `category_corrections` insert, `forgetFromAiRules`' reload of all AI rules, the matcher `total()`/`countMatchingAll()` overbroad probes, `releaseClauseFromOtherCorrectionRules`, and `existingCorrectionRule`. A learnable transaction still costs ~6–10 queries. Treat PHP-LARAVEL-40 as **reduced, not closed** (hence no `Fixes` keyword). ## Reviewed by two independent agents (architecture + product/correctness) Both verified the two perf commits are **behavior-preserving and safe**: the memo cannot go stale (nothing in the correction path mutates descriptions; the bulk `update()` only writes `category_id`/`category_source`/`ai_confidence`/`categorized_by_rule_id`, and runs after the loop), no cross-user leak (keyed by user_id; learner is transient, one user per request, no Octane), and the handler is stateless. The existing 34 learning tests exercise the corpus→rule computation on fresh loads. ## Proposed full fix (for a human to implement) Exploit that a bulk update sends **all transactions to the same category for one user.** Add `CategoryOverrideHandler::recordBulk(Collection, ?string)` backed by `AiRuleLearner::learnFromCorrections(array, string)`; load user-level data once, mutate in memory, write once: 1. **Batch-resolve** `categorized_by_rule_id` via one `whereIn` instead of per-txn `find()`; apply the **identical** per-txn learnable test against the map. 2. **Batch-insert** corrections: snapshot each AI-driven txn's *old* `from_category_id`/`source`/`confidence` during the loop, collect rows, one insert. (Note: raw `insert()` skips model events/UUID/timestamps — verify `CategoryCorrection` has no `creating` hook, else chunked `create` in one transaction.) 3. **Forget once**: union all learnable txns' merchant tokens, load AI rules once, apply the **whole union** to each rule *before* the delete-vs-save decision, save/delete each once. 4. **Learn once**: compute each clause (merchant or memoized-corpus tokens), dedupe within the batch and against the target rule, run `releaseClauseFromOtherCorrectionRules` for the union once, load/create the single target correction rule once, append all, save once. 5. Wrap 2–4 in one `DB::transaction`, and route the single-txn path through the same method (one-element collection) so the two can't drift. **Correctness invariants to preserve (call these out in review):** - Apply-all-then-decide for both `forget` and `releaseClause` deletes — an incremental forget/save/forget would delete a rule a later txn's clause should have kept. - Corrections must read each txn's **old** category/source/confidence — run before the bulk `update()`, as today. - Clause dedup must match `appendClause`'s loose `==` array comparison. - Validate with a golden-set test diffing learned/forgotten rules before/after against the current per-txn path over a mixed batch (merchant txns, description txns, encrypted/no-merchant skips, re-corrections that move a key between categories). ## Testing - `tests/Feature/Ai/` + `BulkUpdateTransactionsTest` + IDOR/decrypt: 153/153 green. `pint` clean. Refs PHP-LARAVEL-40
This commit is contained in:
parent
ad46e465be
commit
3d3f6daa77
|
|
@ -313,8 +313,10 @@ class TransactionController extends Controller
|
|||
if ($request->has('category_id')) {
|
||||
$newCategoryId = $request->input('category_id');
|
||||
|
||||
$overrideHandler = app(CategoryOverrideHandler::class);
|
||||
|
||||
foreach ($transactions as $transaction) {
|
||||
app(CategoryOverrideHandler::class)->record($transaction, $newCategoryId);
|
||||
$overrideHandler->record($transaction, $newCategoryId);
|
||||
}
|
||||
|
||||
$updateData['category_id'] = $newCategoryId;
|
||||
|
|
|
|||
|
|
@ -27,6 +27,23 @@ use Illuminate\Support\Str;
|
|||
*/
|
||||
class AiRuleLearner
|
||||
{
|
||||
/**
|
||||
* Per-user document-frequency corpus, memoized for the lifetime of this
|
||||
* instance. A bulk correction runs learnFromCorrection once per transaction
|
||||
* for the same user, and the description corpus is immutable while only
|
||||
* categories change — so loading and tokenizing every description on every
|
||||
* transaction (the N+1 in PHP-LARAVEL-40) is wasted work.
|
||||
*
|
||||
* SAFETY: this cache has no invalidation. It is only correct because the
|
||||
* learner is resolved fresh per request (never bound singleton/scoped) and
|
||||
* one instance only ever serves a single user. Do NOT bind this singleton or
|
||||
* reuse one instance across users/requests — the corpus would go stale and
|
||||
* leak. An arch test guards the non-singleton binding.
|
||||
*
|
||||
* @var array<string, array{frequency: array<string, int>, count: int}>
|
||||
*/
|
||||
private array $descriptionCorpus = [];
|
||||
|
||||
public function __construct(
|
||||
private readonly DescriptionTokenizer $tokenizer,
|
||||
private readonly TransactionMatcher $matcher,
|
||||
|
|
@ -153,16 +170,33 @@ class AiRuleLearner
|
|||
*/
|
||||
private function distinctiveDescriptionTokens(Transaction $transaction): array
|
||||
{
|
||||
$descriptions = Transaction::query()
|
||||
->where('user_id', $transaction->user_id)
|
||||
->whereNull('description_iv')
|
||||
->pluck('description')
|
||||
->all();
|
||||
$corpus = $this->descriptionCorpus($transaction->user_id);
|
||||
$threshold = $corpus['count'] * (float) config('ai_suggestions.noise_token_fraction');
|
||||
|
||||
$frequency = $this->tokenizer->documentFrequency($descriptions);
|
||||
$threshold = count($descriptions) * (float) config('ai_suggestions.noise_token_fraction');
|
||||
return $this->tokenizer->distinctiveTokens((string) $transaction->description, $corpus['frequency'], $threshold);
|
||||
}
|
||||
|
||||
return $this->tokenizer->distinctiveTokens((string) $transaction->description, $frequency, $threshold);
|
||||
/**
|
||||
* The user's description document-frequency map and corpus size, loaded once
|
||||
* per instance. Safe to memoize: descriptions are never mutated by a
|
||||
* categorization change, so the corpus is stable across a bulk correction.
|
||||
*
|
||||
* @return array{frequency: array<string, int>, count: int}
|
||||
*/
|
||||
private function descriptionCorpus(string $userId): array
|
||||
{
|
||||
return $this->descriptionCorpus[$userId] ??= (function () use ($userId): array {
|
||||
$descriptions = Transaction::query()
|
||||
->where('user_id', $userId)
|
||||
->whereNull('description_iv')
|
||||
->pluck('description')
|
||||
->all();
|
||||
|
||||
return [
|
||||
'frequency' => $this->tokenizer->documentFrequency($descriptions),
|
||||
'count' => count($descriptions),
|
||||
];
|
||||
})();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ use App\Models\User;
|
|||
use App\Services\Ai\AiRuleLearner;
|
||||
use App\Services\Ai\CategorizationOutcome;
|
||||
use App\Services\AutomationRuleService;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
||||
function expenseCategory(User $user): Category
|
||||
{
|
||||
|
|
@ -51,6 +52,61 @@ it('creates an ai-owned rule at the lowest priority and links the transaction',
|
|||
->and($transaction->refresh()->categorized_by_rule_id)->toBe($rule->id);
|
||||
});
|
||||
|
||||
it('resolves a fresh instance per container lookup so the memoized corpus cannot leak or go stale', function () {
|
||||
// The per-user corpus cache has no invalidation and is safe only while the
|
||||
// learner is never a singleton. Guard that invariant.
|
||||
expect(app(AiRuleLearner::class))->not->toBe(app(AiRuleLearner::class));
|
||||
});
|
||||
|
||||
it('learns each correction correctly across a batch while loading the corpus once', function () {
|
||||
$user = User::factory()->create();
|
||||
$target = expenseCategory($user);
|
||||
// A separate category keeps the corrected txns out of the "uncategorized"
|
||||
// count, so the overbroad guard (which needs uncategorized rows) is a no-op
|
||||
// and each correction actually learns a description rule.
|
||||
$existing = expenseCategory($user);
|
||||
|
||||
// Merchant-less, plaintext transactions force the description-token path,
|
||||
// which is what loads the per-user description corpus.
|
||||
$makeTxn = fn (string $description): Transaction => Transaction::factory()->plaintext()->create([
|
||||
'user_id' => $user->id,
|
||||
'category_id' => $existing->id,
|
||||
'creditor_name' => null,
|
||||
'debtor_name' => null,
|
||||
'description' => $description,
|
||||
]);
|
||||
|
||||
$first = $makeTxn('Netflix subscription');
|
||||
$second = $makeTxn('Spotify premium');
|
||||
|
||||
// One instance across the batch, mirroring the bulkUpdate loop where the
|
||||
// handler (and its learner) is resolved once.
|
||||
$learner = app(AiRuleLearner::class);
|
||||
|
||||
DB::enableQueryLog();
|
||||
$firstRule = $learner->learnFromCorrection($first, $target->id);
|
||||
$secondRule = $learner->learnFromCorrection($second, $target->id);
|
||||
$queries = collect(DB::getQueryLog());
|
||||
DB::disableQueryLog();
|
||||
|
||||
// The second learning ran against the memoized corpus and still produced a
|
||||
// distinct, valid clause: both corrections live in the one target rule.
|
||||
expect($firstRule)->not->toBeNull()
|
||||
->and($secondRule)->not->toBeNull()
|
||||
->and($secondRule->id)->toBe($firstRule->id)
|
||||
->and($secondRule->refresh()->rules_json)->toHaveKey('or')
|
||||
->and($secondRule->rules_json['or'])->toHaveCount(2);
|
||||
|
||||
// The corpus is the pluck of the `description` column (not the matcher's
|
||||
// count(*) probes, which also filter on description_iv), loaded once.
|
||||
$corpusLoads = $queries->filter(fn (array $q): bool => str_starts_with(strtolower(ltrim($q['query'])), 'select')
|
||||
&& str_contains($q['query'], 'description_iv')
|
||||
&& ! str_contains(strtolower($q['query']), 'count(')
|
||||
);
|
||||
|
||||
expect($corpusLoads)->toHaveCount(1);
|
||||
});
|
||||
|
||||
it('appends a new merchant to the existing ai rule for the same category', function () {
|
||||
$user = User::factory()->create();
|
||||
$category = expenseCategory($user);
|
||||
|
|
|
|||
Loading…
Reference in New Issue