From 5baa677c2af3d7f3a37aabd24193bba30722bc3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Mon, 10 Aug 2026 15:56:45 +0200 Subject: [PATCH] fix(banking): stop requesting a year of Enable Banking history on every sync (#755) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why Enable Banking connections re-requested **a year of transaction history every 6 hours**. Trade Republic connections failed ~70% of their syncs with HTTP 429 (220 of 307 attempts in the last 7 days, against 1–4% for other ASPSPs), and the cause was ours: 1. `EnableBankingSyncer::sync()` persisted the transactions, then the balance call threw a 429, the exception bubbled up to `SyncBankingConnectionJob`, and the whole run was marked failed. 2. `last_synced_at` is only written after a clean `sync()`, so it never got written. 15 of 16 Trade Republic connections have zero rows in `account_balances` and `last_synced_at = NULL` weeks after connecting. 3. `$isFirstSync = ! $connection->last_synced_at || $this->fullSync` was therefore permanently true, so every run used `now()->subYear()` with `strategy = 'longest'` plus `calculateHistoricalBalances()`. Paginating a year of history four times a day is what trips the rate limit. This is **not Trade Republic specific**: CaixaBank and Eurocaja Rural show the same 429 pattern at lower volume, and the fix applies to every Enable Banking ASPSP. ## What changed All in `app/Services/Banking/Sync/EnableBankingSyncer.php`. **1. The fetch window comes from the transaction watermark, not from `last_synced_at`.** The `linked` branch already did this; the lookup is now shared by both branches: - **Watermark found** → `date_from` = that transaction's date minus a 3-day overlap (banks post transactions with retroactive value dates, so the previous no-overlap watermark could silently miss them), no `longest` strategy. - **No watermark** → unchanged: one year back with `strategy = 'longest'`. That is the genuine first sync. This is the fuse: even if everything else fails, a routine sync asks for a few days instead of a year. The two branches' differing balance handling (`saveDailyBalances: false` for linked accounts, `calculateHistoricalBalances()` on first sync for unlinked) is untouched. **2. A failing balance call no longer fails the whole sync.** It is logged, counted, and surfaced as `balance_failed` in the array `sync()` returns, so it lands in `banking_sync_logs.metadata` instead of being silently swallowed. The run finishes clean → `last_synced_at` gets written → `$isFirstSync` stops being permanently true → `calculateHistoricalBalances()` stops running every 6 hours. **Two exceptions are deliberately still fatal** (both raised by review, see below): an expired session, and a **429**. ## Deviation from the brief, worth a look The brief asked for *every* balance failure to be non-fatal, including the 429. Both review passes flagged the same problem with that: a 429 escapes `EnableBankingProvider` as a raw `RequestException`, and that is exactly what `SyncBankingConnectionJob::isRateLimitError()` matches to set `rate_limited_until`. Swallowing it would have removed the only backoff — and Enable Banking quotas are **per-consent daily access counts** (`Maximum daily access exceeded`, `Allowed number of accesses exceeded for consent`), so a connection that lost its backoff would keep burning the remaining quota on every 6-hourly cycle and never get its balances. So a balance 429 is re-thrown and the existing backoff (untouched, as the brief required) applies. The transactions from that run are still persisted, and the connection stays `active`. Every other balance failure is non-fatal as specified. Three more findings from review, fixed in the second commit: - **`--full` still forces the year-wide window.** A first sync on a connection that has already synced can only come from that flag, so it beats the watermark. Without this, `banking:sync --full` had become a no-op for the window — the operator's only remedy for a gap in history. - **Windows reaching back more than 90 days keep `strategy = 'longest'`.** A dormant account with an old watermark would otherwise be rejected (422) and walked down the `[90, 30, 7]` narrowing ladder, which advances the watermark past the span it never fetched — a silent, permanent gap. - **The watermark counts trashed rows** (the dedup already uses `withTrashed()`, so re-fetching them creates nothing), and the future-date clamp no longer eats the 3-day overlap. ## Out of scope No migration, no new watermark column, no change to the `rate_limited_until` backoff, no manual reset of the broken production connections — the first clean sync clears `error_message` on its own. Two things worth a follow-up, not fixed here: `calculateHistoricalBalances()` is still gated on the connection-level `$isFirstSync` while the window is now per-account, so an account added to an already-synced connection pulls a year of transactions without a balance backfill; and 61 accounts have never received a bank transaction at all, so they stay on the year-wide window until one lands. ## QA Ran the whole chain with nothing mocked but the network (real `EnableBankingProvider`, `Http::fake`), checking the request that actually leaves for the bank: | Case | Request that goes out | |---|---| | Account with a watermark at `2026-08-08`, today `2026-08-10` | `…/transactions?date_from=2026-08-05&date_to=2026-08-10` — 5 days, no `strategy` | | Account with no bank transactions | `…/transactions?date_from=2025-08-10&date_to=2026-08-10&strategy=longest` — unchanged | | Balances returns 429 `Maximum daily access exceeded` | transactions persisted, `status = active`, `rate_limited_until = 2026-08-11 00:00:00` (next UTC midnight) | Against the production database (read-only), for the 475 syncable Enable Banking accounts: | After the fix | Accounts | Avg. days requested | |---|---|---| | Watermark → short window | 382 | 13.7 | | No watermark → 1 year + `longest` | 61 | 365 | | Watermark older than 90d → wide + `longest` | 32 | 173 | All 12 Trade Republic accounts have a watermark, averaging **7.1 days** — down from 365 on every run. ## Tests `tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php`: - an account with existing Enable Banking transactions asks for `watermark - 3 days`, not a year (the test that matters) - an account without them still asks for the year with `longest` - `--full` beats the watermark - a non-429 balance failure → connection stays `active`, `last_synced_at` written, the run's transactions persisted, `balance_failed: 1` in the sync log metadata - a 429 balance failure → transactions persisted and the backoff still applied - an expired session during the balance call is not swallowed Full suite green (2080 passed), `pint` and `phpstan` clean. --- .../Banking/Sync/EnableBankingSyncer.php | 127 ++++++++--- .../SyncBankingConnectionJobTest.php | 211 +++++++++++++++++- 2 files changed, 298 insertions(+), 40 deletions(-) diff --git a/app/Services/Banking/Sync/EnableBankingSyncer.php b/app/Services/Banking/Sync/EnableBankingSyncer.php index 3572ce1d..51500f1e 100644 --- a/app/Services/Banking/Sync/EnableBankingSyncer.php +++ b/app/Services/Banking/Sync/EnableBankingSyncer.php @@ -3,16 +3,33 @@ namespace App\Services\Banking\Sync; use App\Enums\TransactionSource; +use App\Exceptions\Banking\ExpiredBankingSessionException; use App\Exceptions\Banking\InaccessibleBankAccountException; use App\Exceptions\Banking\WrongTransactionsPeriodException; use App\Jobs\SendDailyBankTransactionsSyncedEmailJob; +use App\Models\Account; use App\Models\BankingConnection; use App\Services\Banking\BalanceSyncService; use App\Services\Banking\TransactionSyncService; +use Illuminate\Http\Client\RequestException; use Illuminate\Support\Facades\Log; class EnableBankingSyncer extends AbstractBankingConnectionSyncer { + /** + * Days of already-synced history re-requested on every sync. Banks post + * transactions with retroactive value dates, so starting exactly at the + * watermark would silently miss them. + */ + private const int WATERMARK_OVERLAP_DAYS = 3; + + /** + * Windows reaching further back than this still ask for the 'longest' + * strategy. Banks refuse wide unattended windows, and the narrowing retry + * that follows would skip the history in between for good. + */ + private const int SHORT_WINDOW_DAYS = 90; + public function __construct( private TransactionSyncService $transactionSync, private BalanceSyncService $balanceSync, @@ -30,45 +47,24 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer public function sync(BankingConnection $connection, bool $isFirstSync): array { - $dateFrom = $isFirstSync - ? now()->subYear()->toDateString() - : ($connection->last_synced_at?->toDateString() ?? now()->subMonth()->toDateString()); $dateTo = now()->toDateString(); - $strategy = $isFirstSync ? 'longest' : null; + $shortWindowStart = now()->subDays(self::SHORT_WINDOW_DAYS)->toDateString(); + + // A first sync on a connection that has synced before can only come from + // the --full flag, an explicit request to re-pull the whole history. + $forceFullWindow = $isFirstSync && $connection->last_synced_at !== null; $transactionsPerBank = []; + $balanceFailed = 0; $connection->load('accounts.bank'); foreach ($connection->accounts as $account) { + $dateFrom = $this->resolveDateFrom($account, $dateTo, $forceFullWindow); + $strategy = $dateFrom < $shortWindowStart ? 'longest' : null; + try { - if ($account->isLinked()) { - // Only bank-sourced rows move the watermark. A manual or - // imported transaction dated later would otherwise shrink - // the fetch window and skip bank history for good. - $lastTransaction = $account->transactions() - ->where('source', TransactionSource::EnableBanking) - ->latest('transaction_date') - ->first(); - - $linkedDateFrom = $lastTransaction - ? $lastTransaction->transaction_date->toDateString() - : $dateFrom; - - if ($linkedDateFrom > $dateTo) { - $linkedDateFrom = $dateTo; - } - - $created = $this->transactionSync->sync($account, $linkedDateFrom, $dateTo, $strategy, saveDailyBalances: false); - $this->balanceSync->sync($account); - } else { - $created = $this->transactionSync->sync($account, $dateFrom, $dateTo, $strategy); - $this->balanceSync->sync($account); - - if ($isFirstSync) { - $this->balanceSync->calculateHistoricalBalances($account); - } - } + $created = $this->transactionSync->sync($account, $dateFrom, $dateTo, $strategy, saveDailyBalances: ! $account->isLinked()); } catch (InaccessibleBankAccountException|WrongTransactionsPeriodException $e) { // A single account the bank no longer exposes, or whose history // window it refuses even after narrowing, must not break the @@ -83,6 +79,33 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer continue; } + try { + $this->balanceSync->sync($account); + + if ($isFirstSync && ! $account->isLinked()) { + $this->balanceSync->calculateHistoricalBalances($account); + } + } catch (\Throwable $e) { + // An expired consent needs the user to reconnect, and a rate + // limit has to reach the job so it applies the provider backoff: + // swallowing it would keep burning the remaining daily quota. + if ($e instanceof ExpiredBankingSessionException || $this->isRateLimit($e)) { + throw $e; + } + + // Anything else is not worth losing the run over. Balances are a + // nice-to-have next to the transactions we just persisted, and + // failing here leaves last_synced_at unset. + $balanceFailed++; + + Log::warning('EnableBanking balance sync failed, continuing', [ + 'connection_id' => $connection->id, + 'account_id' => $account->id, + 'reason' => $e::class, + 'error' => $e->getMessage(), + ]); + } + if ($created > 0) { $bankName = $account->bank->name ?? __('Unknown Bank'); $transactionsPerBank[$bankName] = ($transactionsPerBank[$bankName] ?? 0) + $created; @@ -95,6 +118,46 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer SendDailyBankTransactionsSyncedEmailJob::dispatch($connection->user, now()->toDateString()); } - return ['transactions_synced' => array_sum($transactionsPerBank), 'transactions_per_bank' => $transactionsPerBank]; + return [ + 'transactions_synced' => array_sum($transactionsPerBank), + 'transactions_per_bank' => $transactionsPerBank, + 'balance_failed' => $balanceFailed, + ]; + } + + /** + * Start of the window to fetch for an account: just before the last + * transaction the bank sent us, or a year back when it never sent one. + * + * Asking for what came after the watermark is what keeps a routine sync to + * a handful of days. Re-paginating a year on every scheduled run is what + * trips the provider's rate limit. + */ + private function resolveDateFrom(Account $account, string $dateTo, bool $forceFullWindow): string + { + // Only bank-sourced rows move the watermark: a manual or imported + // transaction dated later would shrink the window and skip bank history + // for good. Trashed rows still count, because the dedup that follows + // sees them too and would re-import nothing anyway. + $watermark = $forceFullWindow ? null : $account->transactions() + ->withTrashed() + ->where('source', TransactionSource::EnableBanking) + ->latest('transaction_date') + ->value('transaction_date'); + + if (! $watermark) { + return now()->subYear()->toDateString(); + } + + // Future-dated rows (standing orders) must not push the window past + // today, but the overlap applies either way. + $start = $watermark->toDateString() > $dateTo ? now() : $watermark; + + return $start->copy()->subDays(self::WATERMARK_OVERLAP_DAYS)->toDateString(); + } + + private function isRateLimit(\Throwable $e): bool + { + return $e instanceof RequestException && $e->response->status() === 429; } } diff --git a/tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php b/tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php index d505b3b5..fc6e7eb9 100644 --- a/tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php +++ b/tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php @@ -1,8 +1,10 @@ onboarded()->create(); $connection = BankingConnection::factory()->create([ 'user_id' => $user->id, @@ -130,7 +133,7 @@ test('linked accounts sync from last transaction date and skip historical balanc $transactionSync->shouldReceive('sync') ->once() ->withArgs(function ($acct, $dateFrom, $dateTo, $strategy) { - return $dateFrom === '2025-12-15'; + return $dateFrom === '2025-12-12'; }) ->andReturn(0); @@ -142,7 +145,7 @@ test('linked accounts sync from last transaction date and skip historical balanc runSync($job, $transactionSync, $balanceSync); }); -test('a manual transaction does not move the linked account sync window', function () { +test('a manual transaction does not move the sync window', function () { $user = User::factory()->onboarded()->create(); $connection = BankingConnection::factory()->create([ 'user_id' => $user->id, @@ -171,7 +174,7 @@ test('a manual transaction does not move the linked account sync window', functi $transactionSync = Mockery::mock(TransactionSyncService::class); $transactionSync->shouldReceive('sync') ->once() - ->withArgs(fn ($acct, $dateFrom, $dateTo, $strategy) => $dateFrom === '2025-12-15') + ->withArgs(fn ($acct, $dateFrom, $dateTo, $strategy) => $dateFrom === '2025-12-12') ->andReturn(0); $balanceSync = Mockery::mock(BalanceSyncService::class); @@ -182,7 +185,7 @@ test('a manual transaction does not move the linked account sync window', functi runSync($job, $transactionSync, $balanceSync); }); -test('linked accounts clamp linkedDateFrom to today when last transaction is in future', function () { +test('clamps the fetch window to today when the last transaction is in the future', function () { Carbon::setTestNow('2026-05-02 12:00:00'); $user = User::factory()->onboarded()->create(); @@ -199,14 +202,14 @@ test('linked accounts clamp linkedDateFrom to today when last transaction is in Transaction::factory()->enableBanking()->plaintext()->create([ 'user_id' => $user->id, 'account_id' => $account->id, - 'transaction_date' => '2026-05-04', + 'transaction_date' => '2026-05-10', ]); $transactionSync = Mockery::mock(TransactionSyncService::class); $transactionSync->shouldReceive('sync') ->once() ->withArgs(function ($acct, $dateFrom, $dateTo, $strategy) { - return $dateFrom === '2026-05-02' && $dateTo === '2026-05-02'; + return $dateFrom === '2026-04-29' && $dateTo === '2026-05-02'; }) ->andReturn(0); @@ -218,6 +221,185 @@ test('linked accounts clamp linkedDateFrom to today when last transaction is in runSync($job, $transactionSync, $balanceSync); }); +test('an account with bank transactions syncs from the watermark instead of a year back', function () { + Carbon::setTestNow('2026-05-02 12:00:00'); + + $user = User::factory()->onboarded()->create(); + // The broken production state: transactions were already imported but the + // run never finished, so last_synced_at is still null. + $connection = BankingConnection::factory()->create([ + 'user_id' => $user->id, + 'last_synced_at' => null, + ]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + Transaction::factory()->enableBanking()->plaintext()->create([ + 'user_id' => $user->id, + 'account_id' => $account->id, + 'transaction_date' => '2026-04-28', + ]); + + $transactionSync = Mockery::mock(TransactionSyncService::class); + $transactionSync->shouldReceive('sync') + ->once() + ->withArgs(fn ($acct, $dateFrom, $dateTo, $strategy) => $dateFrom === '2026-04-25' + && $dateTo === '2026-05-02' + && $strategy === null) + ->andReturn(0); + + $balanceSync = Mockery::mock(BalanceSyncService::class); + $balanceSync->shouldReceive('sync')->once(); + $balanceSync->shouldReceive('calculateHistoricalBalances')->once(); + + $job = new SyncBankingConnectionJob($connection); + runSync($job, $transactionSync, $balanceSync); +}); + +test('a failing balance call does not fail the whole sync', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create([ + 'user_id' => $user->id, + 'last_synced_at' => null, + ]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $transactionSync = Mockery::mock(TransactionSyncService::class); + $transactionSync->shouldReceive('sync')->once()->andReturnUsing(function () use ($user, $account) { + Transaction::factory()->enableBanking()->plaintext()->create([ + 'user_id' => $user->id, + 'account_id' => $account->id, + ]); + + return 1; + }); + + $balanceSync = Mockery::mock(BalanceSyncService::class); + $balanceSync->shouldReceive('sync')->once()->andThrow( + new RequestException(new Illuminate\Http\Client\Response(new Response(500))) + ); + $balanceSync->shouldNotReceive('calculateHistoricalBalances'); + + $job = new SyncBankingConnectionJob($connection); + runSync($job, $transactionSync, $balanceSync); + + $connection->refresh(); + $log = BankingSyncLog::query()->latest('created_at')->first(); + + expect($connection->status)->toBe(BankingConnectionStatus::Active) + ->and($connection->last_synced_at)->not->toBeNull() + ->and($account->transactions()->count())->toBe(1) + ->and($log->status)->toBe(BankingSyncLogStatus::Success) + ->and($log->metadata['balance_failed'])->toBe(1); +}); + +test('a rate limited balance call keeps the transactions and still backs off', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create([ + 'user_id' => $user->id, + 'last_synced_at' => null, + ]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $transactionSync = Mockery::mock(TransactionSyncService::class); + $transactionSync->shouldReceive('sync')->once()->andReturnUsing(function () use ($user, $account) { + Transaction::factory()->enableBanking()->plaintext()->create([ + 'user_id' => $user->id, + 'account_id' => $account->id, + ]); + + return 1; + }); + + $balanceSync = Mockery::mock(BalanceSyncService::class); + $balanceSync->shouldReceive('sync')->once()->andThrow( + new RequestException(new Illuminate\Http\Client\Response(new Response(429))) + ); + + $job = new SyncBankingConnectionJob($connection); + runSync($job, $transactionSync, $balanceSync); + + $connection->refresh(); + + // The quota is per consent: swallowing the 429 here would keep the next + // scheduled runs burning what is left of it. + expect($connection->status)->toBe(BankingConnectionStatus::Active) + ->and($connection->rate_limited_until)->not->toBeNull() + ->and($connection->rate_limited_until->isFuture())->toBeTrue() + ->and($account->transactions()->count())->toBe(1); +}); + +test('an expired session during the balance call is not swallowed', function () { + Mail::fake(); + + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create([ + 'user_id' => $user->id, + 'last_synced_at' => now()->subDay(), + ]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $transactionSync = Mockery::mock(TransactionSyncService::class); + $transactionSync->shouldReceive('sync')->once()->andReturn(0); + + $balanceSync = Mockery::mock(BalanceSyncService::class); + $balanceSync->shouldReceive('sync')->once()->andThrow( + new ExpiredBankingSessionException('Reconnect required.') + ); + + $job = new SyncBankingConnectionJob($connection); + runSync($job, $transactionSync, $balanceSync); + + $connection->refresh(); + + expect($connection->status)->toBe(BankingConnectionStatus::Expired); +}); + +test('an account without bank transactions still pulls a year with the longest strategy', function () { + Carbon::setTestNow('2026-05-02 12:00:00'); + + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create([ + 'user_id' => $user->id, + 'last_synced_at' => null, + ]); + Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $transactionSync = Mockery::mock(TransactionSyncService::class); + $transactionSync->shouldReceive('sync') + ->once() + ->withArgs(fn ($acct, $dateFrom, $dateTo, $strategy) => $dateFrom === '2025-05-02' + && $dateTo === '2026-05-02' + && $strategy === 'longest') + ->andReturn(0); + + $balanceSync = Mockery::mock(BalanceSyncService::class); + $balanceSync->shouldReceive('sync')->once(); + $balanceSync->shouldReceive('calculateHistoricalBalances')->once(); + + $job = new SyncBankingConnectionJob($connection); + runSync($job, $transactionSync, $balanceSync); +}); + test('mixed linked and new accounts in same connection', function () { $user = User::factory()->onboarded()->create(); $connection = BankingConnection::factory()->create([ @@ -1076,6 +1258,8 @@ test('binance subsequent sync does not dispatch historical job', function () { }); test('fullSync flag forces first-sync behavior on already-synced connection', function () { + Carbon::setTestNow('2026-05-02 12:00:00'); + $user = User::factory()->onboarded()->create(); $connection = BankingConnection::factory()->create([ 'user_id' => $user->id, @@ -1087,8 +1271,19 @@ test('fullSync flag forces first-sync behavior on already-synced connection', fu 'external_account_id' => 'ext-123', ]); + // --full is the operator remedy for a gap in the history, so it must beat + // the watermark that would otherwise keep the window to a few days. + Transaction::factory()->enableBanking()->plaintext()->create([ + 'user_id' => $user->id, + 'account_id' => $account->id, + 'transaction_date' => '2026-04-28', + ]); + $transactionSync = Mockery::mock(TransactionSyncService::class); - $transactionSync->shouldReceive('sync')->once()->andReturn(0); + $transactionSync->shouldReceive('sync') + ->once() + ->withArgs(fn ($acct, $dateFrom, $dateTo, $strategy) => $dateFrom === '2025-05-02' && $strategy === 'longest') + ->andReturn(0); $balanceSync = Mockery::mock(BalanceSyncService::class); $balanceSync->shouldReceive('sync')->once();