fix(banking): skip inaccessible EnableBanking accounts instead of failing the connection (#559)
## 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)
This commit is contained in:
parent
a9b90a200e
commit
46568700b2
|
|
@ -0,0 +1,17 @@
|
|||
<?php
|
||||
|
||||
namespace App\Exceptions\Banking;
|
||||
|
||||
use Exception;
|
||||
use Illuminate\Contracts\Debug\ShouldntReport;
|
||||
use Throwable;
|
||||
|
||||
class InaccessibleBankAccountException extends Exception implements ShouldntReport
|
||||
{
|
||||
public function __construct(
|
||||
string $message,
|
||||
?Throwable $previous = null,
|
||||
) {
|
||||
parent::__construct($message, 0, $previous);
|
||||
}
|
||||
}
|
||||
|
|
@ -4,6 +4,7 @@ namespace App\Services\Banking;
|
|||
|
||||
use App\Contracts\BankingProviderInterface;
|
||||
use App\Exceptions\Banking\ExpiredBankingSessionException;
|
||||
use App\Exceptions\Banking\InaccessibleBankAccountException;
|
||||
use App\Exceptions\Banking\TransientBankingProviderException;
|
||||
use Firebase\JWT\JWT;
|
||||
use Illuminate\Http\Client\ConnectionException;
|
||||
|
|
@ -111,6 +112,13 @@ class EnableBankingProvider implements BankingProviderInterface
|
|||
);
|
||||
}
|
||||
|
||||
if ($this->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<string, mixed>
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
<?php
|
||||
|
||||
use App\Exceptions\Banking\ExpiredBankingSessionException;
|
||||
use App\Exceptions\Banking\InaccessibleBankAccountException;
|
||||
use App\Exceptions\Banking\TransientBankingProviderException;
|
||||
use App\Services\Banking\EnableBankingProvider;
|
||||
use Illuminate\Contracts\Debug\ShouldntReport;
|
||||
|
|
@ -105,6 +106,58 @@ test('getBalances wraps an expired session 401 as a non-reportable expired sessi
|
|||
test()->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([
|
||||
|
|
|
|||
|
|
@ -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([
|
||||
|
|
|
|||
Loading…
Reference in New Issue