From 46568700b2c0610566a7a35efe54810ea51e3925 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Fri, 19 Jun 2026 08:59:05 +0200 Subject: [PATCH] fix(banking): skip inaccessible EnableBanking accounts instead of failing the connection (#559) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Sentry issue [PHP-LARAVEL-3J](https://whisper-money.sentry.io/issues/PHP-LARAVEL-3J) started escalating again — but with a **different** error than the 401 expired-session fixed in #557. Sentry groups both under the same `getTransactions` culprit: ``` RequestException: HTTP request returned status code 400: {"code":400,"message":"Account not found","detail":{"message":"Account not found","error_name":"AccountNotAccessibleException"}} ``` When EnableBanking returns `400 AccountNotAccessibleException` for **one** account (closed, or no longer covered by the consent), the raw `RequestException` was re-thrown. This: 1. **Aborted the whole connection's account loop** in `EnableBankingSyncer` — accounts after the dead one never synced. 2. Marked the **entire connection** `Error` (`friendlyErrorMessage` default), even though its other accounts were fine. 3. **Reported to Sentry** on every scheduled retry (escalating noise). Confirmed in production (`agent:db --prod`): connection `019ea143-…` is `active` with `valid_until` in the future (2026-09-05) and **6 accounts**; one (`08bbcdf9-…`) returns the 400, and the connection is now stuck in `error` with the generic "try again later" message. 2 users affected. ## Fix A single account the bank no longer exposes must not break the rest of the connection. - New domain exception `InaccessibleBankAccountException` (implements `ShouldntReport`, like `TransientBankingProviderException` / `ExpiredBankingSessionException`). - `EnableBankingProvider::getTransactions()` / `getBalances()` wrap a `400 AccountNotAccessibleException` in it (keyed on `detail.error_name`) instead of re-throwing the raw `RequestException`. - `EnableBankingSyncer` catches it **per account**, logs a warning, and `continue`s — the remaining accounts sync and the connection stays `Active`. Connections currently stuck in `error` from this self-heal on the next scheduled sync (they're `Error` + under the retry cap + `valid_until` future, so the scheduler re-dispatches them; the dead account is now skipped). Composes with #558: a user can permanently *stop syncing* such an account from the manage-accounts screen. This PR only stops one dead account from breaking automated syncs in the meantime. ## Tests - `EnableBankingProviderTest`: `getTransactions` / `getBalances` wrap a `400 AccountNotAccessibleException` as a non-reportable `InaccessibleBankAccountException`. - `SyncRetryAndLoggingTest`: with one inaccessible and one good account, the good one still syncs, the connection stays `Active`, and nothing is thrown. All three failed before the fix. Full `tests/Feature/OpenBanking` suite passes (276), pint clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- .../InaccessibleBankAccountException.php | 17 ++++++ .../Banking/EnableBankingProvider.php | 26 +++++++++ .../Banking/Sync/EnableBankingSyncer.php | 50 ++++++++++------- .../OpenBanking/EnableBankingProviderTest.php | 53 +++++++++++++++++++ .../OpenBanking/SyncRetryAndLoggingTest.php | 44 +++++++++++++++ 5 files changed, 172 insertions(+), 18 deletions(-) create mode 100644 app/Exceptions/Banking/InaccessibleBankAccountException.php diff --git a/app/Exceptions/Banking/InaccessibleBankAccountException.php b/app/Exceptions/Banking/InaccessibleBankAccountException.php new file mode 100644 index 00000000..744004ad --- /dev/null +++ b/app/Exceptions/Banking/InaccessibleBankAccountException.php @@ -0,0 +1,17 @@ +isInaccessibleAccount($e)) { + throw new InaccessibleBankAccountException( + 'EnableBanking account is no longer accessible while fetching transactions.', + previous: $e, + ); + } + if (! $this->isAspspError($e)) { throw $e; } @@ -155,6 +163,13 @@ class EnableBankingProvider implements BankingProviderInterface ); } + if ($this->isInaccessibleAccount($e)) { + throw new InaccessibleBankAccountException( + 'EnableBanking account is no longer accessible while fetching balances.', + previous: $e, + ); + } + if (! $this->isAspspError($e)) { throw $e; } @@ -217,6 +232,17 @@ class EnableBankingProvider implements BankingProviderInterface && ($body['error'] ?? null) === 'EXPIRED_SESSION'; } + private function isInaccessibleAccount(RequestException $e): bool + { + $detail = $this->errorBody($e)['detail'] ?? null; + $errorName = is_array($detail) ? ($detail['error_name'] ?? null) : null; + + // ponytail: the documented per-account 400; widen if other terminal + // account-level codes surface for a single account. + return $e->response->status() === 400 + && $errorName === 'AccountNotAccessibleException'; + } + /** * @return array */ diff --git a/app/Services/Banking/Sync/EnableBankingSyncer.php b/app/Services/Banking/Sync/EnableBankingSyncer.php index 9f329407..e7724117 100644 --- a/app/Services/Banking/Sync/EnableBankingSyncer.php +++ b/app/Services/Banking/Sync/EnableBankingSyncer.php @@ -2,10 +2,12 @@ namespace App\Services\Banking\Sync; +use App\Exceptions\Banking\InaccessibleBankAccountException; use App\Jobs\SendDailyBankTransactionsSyncedEmailJob; use App\Models\BankingConnection; use App\Services\Banking\BalanceSyncService; use App\Services\Banking\TransactionSyncService; +use Illuminate\Support\Facades\Log; class EnableBankingSyncer extends AbstractBankingConnectionSyncer { @@ -37,28 +39,40 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer $connection->load('accounts.bank'); foreach ($connection->accounts as $account) { - if ($account->isLinked()) { - $lastTransaction = $account->transactions() - ->latest('transaction_date') - ->first(); + try { + if ($account->isLinked()) { + $lastTransaction = $account->transactions() + ->latest('transaction_date') + ->first(); - $linkedDateFrom = $lastTransaction - ? $lastTransaction->transaction_date->toDateString() - : $dateFrom; + $linkedDateFrom = $lastTransaction + ? $lastTransaction->transaction_date->toDateString() + : $dateFrom; - if ($linkedDateFrom > $dateTo) { - $linkedDateFrom = $dateTo; + 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); + } } + } catch (InaccessibleBankAccountException) { + // A single account the bank no longer exposes must not break the + // whole connection sync. Skip it; the user can stop syncing it + // from the manage-accounts screen. + Log::warning('Skipping inaccessible EnableBanking account during sync', [ + 'connection_id' => $connection->id, + 'account_id' => $account->id, + ]); - $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); - } + continue; } if ($created > 0) { diff --git a/tests/Feature/OpenBanking/EnableBankingProviderTest.php b/tests/Feature/OpenBanking/EnableBankingProviderTest.php index 66323fc1..59b1195e 100644 --- a/tests/Feature/OpenBanking/EnableBankingProviderTest.php +++ b/tests/Feature/OpenBanking/EnableBankingProviderTest.php @@ -1,6 +1,7 @@ fail('Expected expired banking session exception.'); }); +test('getTransactions wraps an inaccessible account 400 as a non-reportable error', function () { + Http::fake([ + 'api.enablebanking.com/accounts/ext-123/transactions*' => Http::response([ + 'code' => 400, + 'message' => 'Account not found', + 'detail' => [ + 'message' => 'Account not found', + 'error_name' => 'AccountNotAccessibleException', + ], + ], 400), + ]); + + $provider = enableBankingProviderForTest(); + + try { + $provider->getTransactions('ext-123', now()->toDateString(), now()->toDateString()); + } catch (InaccessibleBankAccountException $e) { + expect($e)->toBeInstanceOf(ShouldntReport::class) + ->and($e->getPrevious())->toBeInstanceOf(RequestException::class); + + return; + } + + test()->fail('Expected inaccessible bank account exception.'); +}); + +test('getBalances wraps an inaccessible account 400 as a non-reportable error', function () { + Http::fake([ + 'api.enablebanking.com/accounts/ext-123/balances' => Http::response([ + 'code' => 400, + 'message' => 'Account not found', + 'detail' => [ + 'message' => 'Account not found', + 'error_name' => 'AccountNotAccessibleException', + ], + ], 400), + ]); + + $provider = enableBankingProviderForTest(); + + try { + $provider->getBalances('ext-123'); + } catch (InaccessibleBankAccountException $e) { + expect($e)->toBeInstanceOf(ShouldntReport::class) + ->and($e->getPrevious())->toBeInstanceOf(RequestException::class); + + return; + } + + test()->fail('Expected inaccessible bank account exception.'); +}); + test('getTransactions keeps non-ASPSP client errors reportable', function () { Http::fake([ 'api.enablebanking.com/accounts/ext-123/transactions*' => Http::response([ diff --git a/tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php b/tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php index b3a331b2..9bdd9991 100644 --- a/tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php +++ b/tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php @@ -3,6 +3,7 @@ use App\Enums\BankingConnectionStatus; use App\Enums\BankingSyncLogStatus; use App\Exceptions\Banking\ExpiredBankingSessionException; +use App\Exceptions\Banking\InaccessibleBankAccountException; use App\Exceptions\Banking\TransientBankingProviderException; use App\Jobs\SyncAllBankingConnectionsJob; use App\Jobs\SyncBankingConnectionJob; @@ -197,6 +198,49 @@ test('expired session marks the connection expired and emails the user instead o }); }); +test('an inaccessible account is skipped and the rest of the connection still syncs', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create([ + 'user_id' => $user->id, + 'last_synced_at' => now()->subDay(), + ]); + Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'good-account', + ]); + Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'dead-account', + ]); + + $transactionSync = Mockery::mock(TransactionSyncService::class); + $transactionSync->shouldReceive('sync')->andReturnUsing(function ($account) { + if ($account->external_account_id === 'dead-account') { + throw new InaccessibleBankAccountException('EnableBanking account is no longer accessible.'); + } + + return 2; + }); + + $balanceSync = Mockery::mock(BalanceSyncService::class); + $balanceSync->shouldReceive('sync')->once(); + + $job = new SyncBankingConnectionJob($connection); + + // One dead account must not break the whole connection sync or be reported. + runSync($job, $transactionSync, $balanceSync); + + $connection->refresh(); + expect($connection->status)->toBe(BankingConnectionStatus::Active); + expect($connection->error_message)->toBeNull(); + + $log = BankingSyncLog::where('banking_connection_id', $connection->id)->first(); + expect($log->status)->toBe(BankingSyncLogStatus::Success); + expect($log->metadata['transactions_synced'])->toBe(2); +}); + test('consecutive sync failures accumulate across dispatch cycles', function () { $user = User::factory()->onboarded()->create(); $connection = BankingConnection::factory()->error()->create([