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 (#793) 

> Sentry's MCP token is still expired, so this came from the production
DB again.

## The bug

`EnableBankingSyncer::sync` loops a connection's accounts doing
transactions-then-balances. The transaction call was wrapped, but only
for `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 on a CaixaBank connection with three accounts:

| account | transactions | balance days | last balance |
|---|---|---|---|
| 1 | 618 | 231 | 2026-07-19 |
| 2 | 0 | 13 | **2026-06-12** |
| 3 | 0 | 13 | **2026-06-12** |

2026-06-12 is the connection's `last_synced_at` — the last time a run
completed. Account 1 kept importing for another five weeks; 2 and 3
never got another turn.

**That user has since deleted their account, so this ships as a latent
fix, not a rescue.** 84 of the 260 live EnableBanking connections have
two or more accounts.

## Two things I had wrong

I opened this from a different pair of connections and the product
review took both apart with data I hadn't gathered — request durations.

- **Openbank (4 accounts, 0/0 on two of them)**: I read it as
starvation. It fails in **567–1,358 ms**, less than a single account's
work (a healthy Openbank account is ~3.7s), so it is failing on the
*first* call. And all four live Openbank connections stopped syncing
within four minutes of each other on 2026-08-11 18:03–18:07. That is a
**bank-wide connector outage**, not a per-account fault. Its earlier
zero-attempt days were 429s on the daily PSU quota, at 10.7s / 14.4s /
20.7s in — a different failure this diff deliberately does not touch.
- **Renta 4 (0 transactions in 67 days)**: 62 of those days had **zero
attempts**, because the connection had dropped out of the scheduled
rotation — the bug #782 fixed. Genuine consecutive retries: five days.

The mechanism is real; my examples of it weren't. The CaixaBank
connection is.

## Deliberately conservative

The first version let a partial run report success. Both reviews pushed
back and they were right, so it no longer does — the failure is raised
once every account has had its turn. The connection keeps its Error
state, its retries and its unset `last_synced_at` exactly as today.
**The only thing that changes is that the accounts behind the failing
one get attempted at all.**

What recording it as a success would have cost, all verified in the
code:

- **An Active badge and a fresh "Last synced" over an account that had
stopped updating.** `manage-accounts.tsx:285` renders every synced
account as `Syncing`, hardcoded; there is no per-account sync state
anywhere in the product, and the new metadata key had no reader. That is
a quieter dead end than the one being fixed.
- **Permanent loss of the failing account's derived balance history.**
`calculateHistoricalBalances` is gated on the *connection's* first sync.
On a partial first run it no-ops for the failing account (no
transactions yet), and once `last_synced_at` is set it is never called
again — so when that account finally backfills a year, its daily
balances are never computed while its siblings have them.
- **A "618 new transactions" email.** Stamping
`bank_transactions_email_cutoff_at` on a partial first sync means the
failing account's eventual backfill all lands after the cutoff, which is
precisely what the cutoff exists to suppress.
- The failing account's in-cycle retries would have dropped from 3 to 1.

A provider that never answered is rethrown immediately rather than
tolerated: `statusCode` is null only on the `ConnectionException` path,
and carrying on there spends the client's 20s timeout per account
against the job's 120s. Prod: a 26-account connection already takes 62s
when everything works, and a 5-account one has peaked at 67s. Without
this guard the fix would have turned a provider timeout into a killed
job.

## Verification

`tests/Feature/OpenBanking`: 358 tests, 348 pass, and the **same 10
failures as clean main** (Inertia page-render tests hitting the SSR
`/render` endpoint, no local server). 4 new tests driven through the
existing `runSync()` helper so they assert the job-level outcome, each
verified to fail with only its own change reverted:

- the starvation case (remove the catch → fails),
- the unreachable-provider guard (remove it → fails),
- **a 429 still reaches the job** — the property I was most worried
about. 429s arrive as a raw `RequestException`, never as
`TransientBankingProviderException`, so the new catch cannot swallow one
and keep burning a per-consent daily quota account after account.
Confirmed against 30 days of prod logs: 821 rate-limit failures, every
one recorded as `RequestException`, zero as the wrapped type.
- and that a partial run still leaves the connection in Error with
`last_synced_at` untouched.

`pint`, `dry` and `crap` green — `sync` was already at complexity 13
before this and the new branches took it to 16, so `resolveWindow`,
`recordAccountTransactionFailure` and `syncBalances` are extracted and
it now sits under 10.

## Follow-ups, not done here

- **The real systemic problem is quota, not this.** 260 rate-limit
events across 26 connections and 24 users in 7 days. The default backoff
is one hour when the message doesn't say "daily", and the scheduler runs
every six — so the backoff expires long before the next cycle and
changes nothing. Trade Republic connections 429 on every single cycle,
which is why 11 users have transactions but no balance at all. That
wants a design, not a patch, and it is the biggest thing in this
subsystem.
- **No per-account sync state.** Until that exists, a connection can
only be all-good or all-bad, which is what forced the conservative
choice above.
- **No escalation for a connection that never succeeds.** Since #757
correctly stopped counting transient failures, "The bank provider is
temporarily unavailable. We will try syncing again later." is a
permanent state with no threshold, no copy change and no email.
- **`WrongTransactionsPeriodException` still skips the balance call.**
The bank refused a date range; `/balances` takes none. Same argument as
this fix, one line, left out to keep the diff to one behaviour.
- **`WiseSyncer`'s transaction call is still unwrapped** — the same bug
class in the file #788 touched. Lower stakes (one token, one host) but
worth closing.

## Auto-merge

Enabled. The behaviour change is a single `catch` that lets the loop
finish, with every other observable — status, timestamp, retries,
notifications, first-sync side effects — deliberately identical to
today. It is additive for the accounts that were being skipped and a
no-op for single-account connections, which are 7,980 of the ~12,500
runs in the last 14 days.
This commit is contained in:
Víctor Falcón 2026-08-13 10:27:18 +02:00 committed by GitHub
parent 8fef50f829
commit 57bcdf89c9
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
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();
});