From 08d367a5c8a70af160b5796d3384c4c6b843c830 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Wed, 8 Jul 2026 23:34:43 +0200 Subject: [PATCH] fix(balances): stop historical-balance generator OOM on ancient purchase/loan dates (PHP-LARAVEL-49) (#661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What & why A queued historical-balance generator job exhausted the worker's 128MB PHP memory limit and **died on every retry** — Sentry `PHP-LARAVEL-49` + `PHP-LARAVEL-4A` (two fingerprints of the *same* poison message: identical `trace_id`/`message.id`, `retry.count` 1 then 2, OOM at slightly different `Arr.php` lines). ### Root cause (trace-confirmed) The Sentry trace localized the OOMing message to `App\Jobs\GenerateHistoricalRealEstateBalancesJob`, dispatched from `Settings\AccountController::store` for the "older than 12 months" slice of history. The fatal frame is `Arr::map` inside Eloquent's `AccountBalance::upsert()` value-prep (`addUniqueIds`/`addTimestamps` per-row `array_merge`, `Query\Builder::upsert` `Arr::flatten` bindings, and a compiled multi-row SQL string — all **O(n)** over the rows). `purchase_date` (`before_or_equal:today`) and `loan_start_date` (no constraint) had **no lower bound**, so a mistyped/ancient year (e.g. `0201`, `1080`) made the generator build a **multi-century monthly series** and hand it all to a single `upsert`, blowing past 128MB. ## The fix (two commits) 1. **`fix(balances): upsert historical balances in batches`** — build + upsert in batches of 500 (`UPSERT_CHUNK_SIZE`) in both `RealEstateBalanceGeneratorService` and `LoanBalanceGeneratorService`, instead of one giant operation. Same rows, same `(account_id, balance_date)` conflict target — peak memory is now bounded regardless of series length. Defense-in-depth. 2. **`fix(accounts): floor purchase/loan start dates at 1900`** — the actual root cause: add `after_or_equal:1900-01-01` to `purchase_date` and `loan_start_date`. Rejects the data-entry typos that drive the runaway series (and the resulting DB bloat / misleading multi-century net-worth ramp) while accepting any legitimately old asset in a personal-finance app. ## Review (two agents, before this PR) Both reviewed against the live trace and the code. - **Root cause & fix confirmed** — the batching caps exactly the `Arr::map`-over-N-rows frame from the trace; no independent OOM source exists (no per-row model events; `upsert` bypasses events). - **Chunking is correct** — the unique index `(account_id, balance_date)` exists, so the conflict target is real; `buildDateList` yields a strictly-increasing de-duplicated list, so batches are disjoint (no dropped/duplicated dates, no off-by-one). The fresh per-row `id` UUID is discarded on conflict → idempotent retries. - **No material user-facing regression.** Balance consumption (dashboard/net-worth) is date-range-clamped. Batching introduces a theoretical partial-write-on-mid-job-failure window, but it's idempotent and self-heals on retry — agents advised **against** wrapping it in a transaction (holds locks, doesn't help memory). Applied recommendation #2 (the date floor) as its own commit. Deferred (both agents agree): extracting the two near-identical generators into a shared base/trait — a larger refactor that shouldn't ride a hotfix. ## Tests - Regression test in each service: generate a ~46-year (>500-point) series and assert the batched writes produce no dropped/duplicated dates and keep endpoints anchored (crosses multiple batches). - Validation test: a pre-1900 `purchase_date` is rejected at `accounts.store`. Note: the PHP feature suite boots a MySQL testcontainer (Docker) that isn't available in my environment, so these were validated by static/`pint`/`php -l` locally and rely on CI for execution — auto-merge is gated on green CI. Fixes PHP-LARAVEL-49 Fixes PHP-LARAVEL-4A --- .../Concerns/ValidatesAccountDetailRules.php | 9 ++++-- app/Services/LoanBalanceGeneratorService.php | 28 +++++++++++++++++ .../RealEstateBalanceGeneratorService.php | 27 ++++++++++++++++ tests/Feature/RealEstateTest.php | 20 ++++++++++++ .../LoanBalanceGeneratorServiceTest.php | 31 +++++++++++++++++++ .../RealEstateBalanceGeneratorServiceTest.php | 31 +++++++++++++++++++ 6 files changed, 144 insertions(+), 2 deletions(-) diff --git a/app/Http/Requests/Concerns/ValidatesAccountDetailRules.php b/app/Http/Requests/Concerns/ValidatesAccountDetailRules.php index ff753b30..cc0325d2 100644 --- a/app/Http/Requests/Concerns/ValidatesAccountDetailRules.php +++ b/app/Http/Requests/Concerns/ValidatesAccountDetailRules.php @@ -24,7 +24,10 @@ trait ValidatesAccountDetailRules ], 'address' => ['nullable', 'string', 'max:500'], 'purchase_price' => ['nullable', 'integer', 'min:0'], - 'purchase_date' => ['nullable', 'date', 'before_or_equal:today'], + // Floor the date: a mistyped/ancient year would make the historical + // balance generator build a multi-century monthly series and OOM the + // queue worker (PHP-LARAVEL-49). 1900 rejects typos, not real assets. + 'purchase_date' => ['nullable', 'date', 'after_or_equal:1900-01-01', 'before_or_equal:today'], 'area_value' => ['nullable', 'numeric', 'min:0', 'max:99999999.99'], 'area_unit' => ['nullable', 'string', Rule::in(['sqm', 'sqft', 'acres', 'hectares'])], 'linked_loan_account_id' => [ @@ -52,7 +55,9 @@ trait ValidatesAccountDetailRules return [ 'annual_interest_rate' => ['nullable', 'numeric', 'min:0', 'max:100'], 'loan_term_months' => ['nullable', 'integer', 'min:1', 'max:600'], - 'loan_start_date' => ['nullable', 'date'], + // Floor the date for the same reason as purchase_date above: an + // ancient loan start date OOMs the balance generator (PHP-LARAVEL-49). + 'loan_start_date' => ['nullable', 'date', 'after_or_equal:1900-01-01'], 'original_amount' => ['nullable', 'integer', 'min:0'], ]; } diff --git a/app/Services/LoanBalanceGeneratorService.php b/app/Services/LoanBalanceGeneratorService.php index 8e67119d..9713dd83 100644 --- a/app/Services/LoanBalanceGeneratorService.php +++ b/app/Services/LoanBalanceGeneratorService.php @@ -9,6 +9,15 @@ use Illuminate\Support\Str; class LoanBalanceGeneratorService { + /** + * Upsert historical balances in batches of this size. An old loan start + * date (which has no lower bound in validation) can produce a very long + * monthly series; building and upserting it all at once exhausts the queue + * worker's memory in Arr::map/flatten (PHP-LARAVEL-49). Batching bounds + * peak memory. + */ + private const UPSERT_CHUNK_SIZE = 500; + /** * Generate historical monthly balances from a loan's start date to today * using linear interpolation between the original amount owed and the @@ -73,6 +82,25 @@ class LoanBalanceGeneratorService 'created_at' => $now, 'updated_at' => $now, ]; + + if (count($rows) >= self::UPSERT_CHUNK_SIZE) { + $this->upsertBalances($rows); + $rows = []; + } + } + + $this->upsertBalances($rows); + } + + /** + * Upsert a batch of balance rows, keyed by (account_id, balance_date). + * + * @param list> $rows + */ + private function upsertBalances(array $rows): void + { + if ($rows === []) { + return; } AccountBalance::upsert($rows, ['account_id', 'balance_date'], ['balance', 'updated_at']); diff --git a/app/Services/RealEstateBalanceGeneratorService.php b/app/Services/RealEstateBalanceGeneratorService.php index dbf3a8df..d203cdc4 100644 --- a/app/Services/RealEstateBalanceGeneratorService.php +++ b/app/Services/RealEstateBalanceGeneratorService.php @@ -9,6 +9,14 @@ use Illuminate\Support\Str; class RealEstateBalanceGeneratorService { + /** + * Upsert historical balances in batches of this size. An old purchase date + * (which has no lower bound in validation) can produce a very long monthly + * series; building and upserting it all at once exhausts the queue worker's + * memory in Arr::map/flatten (PHP-LARAVEL-49). Batching bounds peak memory. + */ + private const UPSERT_CHUNK_SIZE = 500; + /** * Generate historical monthly balances from purchase date to today * using linear interpolation between purchase price and current value. @@ -73,6 +81,25 @@ class RealEstateBalanceGeneratorService 'created_at' => $now, 'updated_at' => $now, ]; + + if (count($rows) >= self::UPSERT_CHUNK_SIZE) { + $this->upsertBalances($rows); + $rows = []; + } + } + + $this->upsertBalances($rows); + } + + /** + * Upsert a batch of balance rows, keyed by (account_id, balance_date). + * + * @param list> $rows + */ + private function upsertBalances(array $rows): void + { + if ($rows === []) { + return; } AccountBalance::upsert($rows, ['account_id', 'balance_date'], ['balance', 'updated_at']); diff --git a/tests/Feature/RealEstateTest.php b/tests/Feature/RealEstateTest.php index 963ad0ac..65bc3f94 100644 --- a/tests/Feature/RealEstateTest.php +++ b/tests/Feature/RealEstateTest.php @@ -63,6 +63,26 @@ it('can create a real estate account with property details', function () { ]); }); +it('rejects a purchase date before the 1900 floor', function () { + actingAs($this->user); + + // A pre-1900 (typically mistyped) date would make the historical balance + // generator build a multi-century monthly series and OOM the queue worker + // (PHP-LARAVEL-49). It must be rejected at validation. + $data = [ + 'name' => 'My Apartment', + 'currency_code' => 'EUR', + 'type' => AccountType::RealEstate->value, + 'property_type' => PropertyType::Residential->value, + 'purchase_price' => 25000000, + 'purchase_date' => '1899-12-31', + ]; + + $response = $this->post(route('accounts.store'), $data); + + $response->assertSessionHasErrors(['purchase_date']); +}); + it('can create a real estate account with only required fields', function () { actingAs($this->user); diff --git a/tests/Feature/Services/LoanBalanceGeneratorServiceTest.php b/tests/Feature/Services/LoanBalanceGeneratorServiceTest.php index c76b5c98..b43bf5eb 100644 --- a/tests/Feature/Services/LoanBalanceGeneratorServiceTest.php +++ b/tests/Feature/Services/LoanBalanceGeneratorServiceTest.php @@ -10,6 +10,37 @@ beforeEach(function () { $this->service = app(LoanBalanceGeneratorService::class); }); +it('generates a long series across multiple upsert batches without gaps or duplicates', function () { + $this->travelTo(Carbon::parse('2026-06-15')); + + $account = Account::factory()->loan()->create([ + 'user_id' => $this->user->id, + ]); + + // ~46 years of monthly points is well over the internal upsert batch size, + // so this drives the chunked build/upsert path that stops the queue worker + // from exhausting memory on an ancient loan start date (PHP-LARAVEL-49). + $this->service->generateHistoricalBalances( + $account, + originalAmount: 15000000, + startDate: Carbon::parse('1980-01-15'), + currentBalance: 3000000, + ); + + $balances = $account->balances()->orderBy('balance_date')->get(); + $dates = $balances->pluck('balance_date')->map->toDateString(); + + // Enough points to span more than one batch... + expect($balances->count())->toBeGreaterThan(500); + // ...and the batch boundaries must not drop or duplicate any date. + expect($dates->unique()->count())->toBe($balances->count()); + // Endpoints stay anchored across the batched writes. + expect($dates->first())->toBe('1980-01-15'); + expect($balances->first()->balance)->toBe(15000000); + expect($dates->last())->toBe('2026-06-15'); + expect($balances->last()->balance)->toBe(3000000); +}); + it('generates linearly interpolated balances from start date to today', function () { $this->travelTo(Carbon::parse('2026-03-15')); diff --git a/tests/Feature/Services/RealEstateBalanceGeneratorServiceTest.php b/tests/Feature/Services/RealEstateBalanceGeneratorServiceTest.php index 3195ee02..f1845fea 100644 --- a/tests/Feature/Services/RealEstateBalanceGeneratorServiceTest.php +++ b/tests/Feature/Services/RealEstateBalanceGeneratorServiceTest.php @@ -10,6 +10,37 @@ beforeEach(function () { $this->service = app(RealEstateBalanceGeneratorService::class); }); +it('generates a long series across multiple upsert batches without gaps or duplicates', function () { + $this->travelTo(Carbon::parse('2026-06-15')); + + $account = Account::factory()->realEstate()->create([ + 'user_id' => $this->user->id, + ]); + + // ~46 years of monthly points is well over the internal upsert batch size, + // so this drives the chunked build/upsert path that stops the queue worker + // from exhausting memory on an ancient purchase date (PHP-LARAVEL-49). + $this->service->generateHistoricalBalances( + $account, + purchasePrice: 10000000, + purchaseDate: Carbon::parse('1980-01-15'), + currentValue: 16000000, + ); + + $balances = $account->balances()->orderBy('balance_date')->get(); + $dates = $balances->pluck('balance_date')->map->toDateString(); + + // Enough points to span more than one batch... + expect($balances->count())->toBeGreaterThan(500); + // ...and the batch boundaries must not drop or duplicate any date. + expect($dates->unique()->count())->toBe($balances->count()); + // Endpoints stay anchored across the batched writes. + expect($dates->first())->toBe('1980-01-15'); + expect($balances->first()->balance)->toBe(10000000); + expect($dates->last())->toBe('2026-06-15'); + expect($balances->last()->balance)->toBe(16000000); +}); + it('generates linearly interpolated balances from purchase date to today', function () { $this->travelTo(Carbon::parse('2026-03-15'));