diff --git a/app/Services/Banking/Sync/EnableBankingSyncer.php b/app/Services/Banking/Sync/EnableBankingSyncer.php index 51500f1e..a1dafeea 100644 --- a/app/Services/Banking/Sync/EnableBankingSyncer.php +++ b/app/Services/Banking/Sync/EnableBankingSyncer.php @@ -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. diff --git a/tests/Feature/OpenBanking/EnableBankingPartialAccountFailureTest.php b/tests/Feature/OpenBanking/EnableBankingPartialAccountFailureTest.php new file mode 100644 index 00000000..6b3e3508 --- /dev/null +++ b/tests/Feature/OpenBanking/EnableBankingPartialAccountFailureTest.php @@ -0,0 +1,185 @@ +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(); +});