RomanNum3ral
/
whisper-money
mirror of https://github.com/whisper-money/whisper-money.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(banking): stop requesting a year of Enable Banking history on every sync (#755) 

## 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.
This commit is contained in:
Víctor Falcón 2026-08-10 15:56:45 +02:00 committed by GitHub
parent 33fc058f3c
commit 5baa677c2a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 298 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

127
app/Services/Banking/Sync/EnableBankingSyncer.php
View File

@ -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;
}
}

211
tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php
View File

@ -1,8 +1,10 @@
<?php
use App\Enums\BankingConnectionStatus;
use App\Enums\BankingSyncLogStatus;
use App\Enums\DripEmailType;
use App\Enums\TransactionSource;
use App\Exceptions\Banking\ExpiredBankingSessionException;
use App\Jobs\SendDailyBankTransactionsSyncedEmailJob;
use App\Jobs\SyncBankingConnectionJob;
use App\Jobs\SyncBinanceHistoricalBalancesJob;
@ -11,6 +13,7 @@ use App\Mail\BankTransactionsSyncedEmail;
use App\Models\Account;
use App\Models\Bank;
use App\Models\BankingConnection;
use App\Models\BankingSyncLog;
use App\Models\Transaction;
use App\Models\User;
use App\Models\UserMailLog;
@ -108,7 +111,7 @@ test('subsequent syncs do not calculate historical balances', function () {
runSync($job, $transactionSync, $balanceSync);
});
test('linked accounts sync from last transaction date and skip historical balances', function () {
test('linked accounts sync from the transaction watermark and skip historical balances', function () {
$user = User::factory()->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();