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): give every account its turn when the bank refuses one of them 

`EnableBankingSyncer::sync` wrapped the transaction call but only caught
`InaccessibleBankAccountException` and `WrongTransactionsPeriodException`. A
`TransientBankingProviderException` — what EnableBanking's HTTP 400
`{"error":"ASPSP_ERROR"}` becomes, i.e. "the bank's connector failed" — propagated
out and abandoned the loop, so every account behind the failing one was skipped
along with its balance, cycle after cycle.

Verified in production on a CaixaBank connection with three accounts: account 1
kept importing transactions until 2026-07-18 (618 rows, 231 balance days), while
accounts 2 and 3 sat at zero transactions with their balances frozen at
2026-06-12 — the date of the last complete run. Five weeks of it. That user has
since deleted their account, so this ships as a latent fix rather than a rescue;
84 of the 260 live EnableBanking connections have two or more accounts.

Deliberately conservative about everything else. The failure is still raised once
every account has had its turn, so the connection keeps its Error state, its
retries and its unset `last_synced_at` exactly as before. Recording a partial run
as a success would have shown an Active badge and a fresh timestamp over an
account that had stopped updating, and nothing in the product distinguishes a
stale account from a fresh one — a quieter dead end than the one being fixed. It
would also have stamped `bank_transactions_email_cutoff_at` and consumed the
connection-level first sync, permanently losing the failing account's derived
balance history and reporting its eventual backfill as "new transactions today".

A provider that never answered is rethrown immediately instead: `statusCode` is
null only on the `ConnectionException` path, and carrying on there would spend the
client's timeout per account against the job's 120s — a 26-account connection
already takes 62s when everything works.
This commit is contained in:
Víctor Falcón 2026-08-13 10:13:56 +02:00
parent 98f03db50c
commit 271b659260
2 changed files with 281 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -5,6 +5,7 @@ namespace App\Services\Banking\Sync;
use App\Enums\TransactionSource;
use App\Exceptions\Banking\ExpiredBankingSessionException;
use App\Exceptions\Banking\InaccessibleBankAccountException;
use App\Exceptions\Banking\TransientBankingProviderException;
use App\Exceptions\Banking\WrongTransactionsPeriodException;
use App\Jobs\SendDailyBankTransactionsSyncedEmailJob;
use App\Models\Account;
@ -48,20 +49,16 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer
public function sync(BankingConnection $connection, bool $isFirstSync): array
{
$dateTo = now()->toDateString();
$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;
$transactionFailure = null;
$connection->load('accounts.bank');
foreach ($connection->accounts as $account) {
$dateFrom = $this->resolveDateFrom($account, $dateTo, $forceFullWindow);
$strategy = $dateFrom < $shortWindowStart ? 'longest' : null;
$created = 0;
[$dateFrom, $strategy] = $this->resolveWindow($connection, $account, $dateTo, $isFirstSync);
try {
$created = $this->transactionSync->sync($account, $dateFrom, $dateTo, $strategy, saveDailyBalances: ! $account->isLinked());
@ -77,33 +74,12 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer
]);
continue;
} catch (TransientBankingProviderException $e) {
$transactionFailure ??= $this->recordAccountTransactionFailure($account, $e);
}
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.
if (! $this->syncBalances($account, $isFirstSync)) {
$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) {
@ -112,6 +88,14 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer
}
}
// Report the failure only once every account has had its turn. The run
// still fails, so the connection keeps its Error state, its retries and its
// unset last_synced_at exactly as before - the one thing that changes is
// that the accounts behind the failing one were attempted at all.
if ($transactionFailure !== null) {
throw $transactionFailure;
}
if ($isFirstSync) {
$connection->update(['bank_transactions_email_cutoff_at' => now()]);
} elseif ($connection->user->canReceiveEmails()) {
@ -125,6 +109,87 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer
];
}
/**
* The window to ask the bank for, and the strategy a window that wide needs.
*
* @return array{0: string, 1: string|null}
*/
private function resolveWindow(BankingConnection $connection, Account $account, string $dateTo, bool $isFirstSync): array
{
// 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;
$dateFrom = $this->resolveDateFrom($account, $dateTo, $forceFullWindow);
$shortWindowStart = now()->subDays(self::SHORT_WINDOW_DAYS)->toDateString();
return [$dateFrom, $dateFrom < $shortWindowStart ? 'longest' : null];
}
/**
* Note that the bank could not serve one account's transactions, and decide
* whether the remaining accounts are still worth trying.
*
* A provider that never answered will not answer for the next account either,
* and each further attempt costs the client's full timeout against the job's
* 120s - a connection with 26 accounts already spends a minute on the happy
* path. Only a reply that came back with a status says something about *this*
* account: the ConnectionException path is the one that leaves statusCode null.
*/
private function recordAccountTransactionFailure(Account $account, TransientBankingProviderException $e): TransientBankingProviderException
{
if ($e->statusCode === null) {
throw $e;
}
Log::warning('EnableBanking transaction sync failed for one account, continuing', [
'connection_id' => $account->banking_connection_id,
'account_id' => $account->id,
'status_code' => $e->statusCode,
'provider_code' => $e->providerCode,
'error' => $e->getMessage(),
]);
return $e;
}
/**
* Sync one account's balances, tolerating a provider that will not serve them.
*
* @return bool Whether the balances were synced
*/
private function syncBalances(Account $account, bool $isFirstSync): bool
{
try {
$this->balanceSync->sync($account);
if ($isFirstSync && ! $account->isLinked()) {
$this->balanceSync->calculateHistoricalBalances($account);
}
return true;
} 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.
Log::warning('EnableBanking balance sync failed, continuing', [
'connection_id' => $account->banking_connection_id,
'account_id' => $account->id,
'reason' => $e::class,
'error' => $e->getMessage(),
]);
return false;
}
}
/**
* 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.

185
tests/Feature/OpenBanking/EnableBankingPartialAccountFailureTest.php Normal file
View File

@ -0,0 +1,185 @@
<?php
use App\Enums\BankingConnectionStatus;
use App\Exceptions\Banking\TransientBankingProviderException;
use App\Jobs\SyncBankingConnectionJob;
use App\Models\Account;
use App\Models\BankingConnection;
use App\Models\User;
use App\Services\Banking\BalanceSyncService;
use App\Services\Banking\TransactionSyncService;
use GuzzleHttp\Psr7\Response;
use Illuminate\Contracts\Queue\Job;
use Illuminate\Http\Client\RequestException;
/**
* A connection whose accounts the bank serves unevenly. Production had a CaixaBank
* one in this shape for five weeks: account 1 importing transactions while accounts
* 2 and 3 sat frozen at the date of the last complete run.
*/
function enableBankingConnectionWithAccounts(int $count): BankingConnection
{
$user = User::factory()->onboarded()->create();
$connection = BankingConnection::factory()->create([
'user_id' => $user->id,
'status' => BankingConnectionStatus::Active,
'last_synced_at' => now()->subDay(),
'consecutive_sync_failures' => 0,
]);
for ($i = 1; $i <= $count; $i++) {
Account::factory()->connected()->create([
'user_id' => $user->id,
'banking_connection_id' => $connection->id,
'external_account_id' => "ext-{$i}",
]);
}
return $connection;
}
function aspspError(): TransientBankingProviderException
{
return new TransientBankingProviderException(
'EnableBanking bank connector failed while fetching account transactions.',
provider: 'enablebanking',
statusCode: 400,
providerCode: 'ASPSP_ERROR',
);
}
function finalAttemptJobFor(BankingConnection $connection): SyncBankingConnectionJob
{
$job = new SyncBankingConnectionJob($connection);
$job->job = Mockery::mock(Job::class);
$job->job->shouldReceive('attempts')->andReturn(3);
$job->job->shouldReceive('isReleased')->andReturn(false);
$job->job->shouldReceive('isDeletedOrReleased')->andReturn(false);
$job->job->shouldReceive('hasFailed')->andReturn(false);
return $job;
}
test('an account the bank cannot serve no longer starves the accounts behind it', function () {
$connection = enableBankingConnectionWithAccounts(3);
$refusedAccountId = $connection->accounts[1]->id;
$attempted = [];
$transactionSync = Mockery::mock(TransactionSyncService::class);
$transactionSync->shouldReceive('sync')->andReturnUsing(
function ($account) use ($refusedAccountId, &$attempted) {
$attempted[] = $account->id;
if ($account->id === $refusedAccountId) {
throw aspspError();
}
return 5;
}
);
$balancedAccounts = [];
$balanceSync = Mockery::mock(BalanceSyncService::class);
$balanceSync->shouldReceive('sync')->andReturnUsing(function ($account) use (&$balancedAccounts) {
$balancedAccounts[] = $account->id;
});
try {
runSync(finalAttemptJobFor($connection), $transactionSync, $balanceSync);
} catch (TransientBankingProviderException) {
// Expected: the run still fails, see the next test.
}
// All three accounts got their turn, including the balance of the one whose
// transactions the bank refused - balances come from a different endpoint.
expect($attempted)->toHaveCount(3);
expect($balancedAccounts)->toHaveCount(3);
});
test('a partially failing run is still recorded as failed', function () {
$connection = enableBankingConnectionWithAccounts(2);
$refusedAccountId = $connection->accounts[1]->id;
$transactionSync = Mockery::mock(TransactionSyncService::class);
$transactionSync->shouldReceive('sync')->andReturnUsing(function ($account) use ($refusedAccountId) {
if ($account->id === $refusedAccountId) {
throw aspspError();
}
return 5;
});
$balanceSync = Mockery::mock(BalanceSyncService::class);
$balanceSync->shouldReceive('sync')->andReturnNull();
try {
runSync(finalAttemptJobFor($connection), $transactionSync, $balanceSync);
} catch (TransientBankingProviderException) {
// Expected.
}
// Deliberately unchanged from before: an Active badge and a fresh timestamp
// over an account that is not updating would be a quieter dead end than the
// error state, and nothing in the UI distinguishes a stale account yet.
$connection->refresh();
expect($connection->status)->toBe(BankingConnectionStatus::Error);
expect($connection->last_synced_at->toDateString())->toBe(now()->subDay()->toDateString());
});
test('a provider that never answered stops the run instead of retrying every account', function () {
$connection = enableBankingConnectionWithAccounts(3);
$attempted = 0;
$transactionSync = Mockery::mock(TransactionSyncService::class);
$transactionSync->shouldReceive('sync')->andReturnUsing(function () use (&$attempted) {
$attempted++;
// No statusCode: the ConnectionException path, i.e. nothing came back.
throw new TransientBankingProviderException(
'EnableBanking did not respond while fetching account transactions.',
provider: 'enablebanking',
);
});
$balanceSync = Mockery::mock(BalanceSyncService::class);
$balanceSync->shouldReceive('sync')->andReturnNull();
try {
runSync(finalAttemptJobFor($connection), $transactionSync, $balanceSync);
} catch (TransientBankingProviderException) {
// Expected.
}
// Carrying on would spend the client's timeout per account against the job's
// 120s; a 26-account connection already takes a minute when everything works.
expect($attempted)->toBe(1);
});
test('a rate limit still reaches the job instead of being swallowed per account', function () {
$connection = enableBankingConnectionWithAccounts(3);
$attempted = 0;
$transactionSync = Mockery::mock(TransactionSyncService::class);
$transactionSync->shouldReceive('sync')->andReturnUsing(function () use (&$attempted) {
$attempted++;
throw new RequestException(new Illuminate\Http\Client\Response(
new Response(429, [], json_encode(['code' => 429, 'message' => 'Too many requests']))
));
});
$balanceSync = Mockery::mock(BalanceSyncService::class);
$balanceSync->shouldReceive('sync')->andReturnNull();
try {
runSync(finalAttemptJobFor($connection), $transactionSync, $balanceSync);
} catch (RequestException) {
// Expected.
}
// A 429 is a raw RequestException, so the new catch must not see it: swallowing
// it would keep burning a per-consent daily quota account after account.
expect($attempted)->toBe(1);
$connection->refresh();
expect($connection->rate_limited_until)->not->toBeNull();
});