From 64b78e36801a34321ee2fc12ef41a2938d13c572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Fri, 29 May 2026 14:58:38 +0200 Subject: [PATCH] fix(banking): handle balance-fetch timeouts and silence handled retries (#450) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes two linked production banking-sync issues. ## PHP-LARAVEL-W — `cURL error 28: timed out` in `EnableBankingProvider::getBalances` (8 events, 4 users, High) `getBalances` called `$response->throw()` raw, so a connection timeout (or ASPSP error) escaped as an **unhandled** `ConnectionException`/`RequestException` and crashed the sync. `getTransactions` already wraps these in `TransientBankingProviderException` (which `implements ShouldntReport` and is handled as a transient, retryable error in `SyncBankingConnectionJob`). → `getBalances` now follows the exact same pattern. Genuine validation errors (non-ASPSP 4xx) stay reportable. ## PHP-LARAVEL-2D — `SyncBankingConnectionJob has been attempted too many times` (High, regressed) The hanging balance call above pushed the job past its 120s `timeout`, the worker was killed mid-job, and the retry tripped a `MaxAttemptsExceededException`. That exception is thrown by the queue worker (not catchable in `handle()`), and the job's `failed()` handler **already** records the terminal `Error` state on the connection — so the Sentry report is redundant operational noise. → Fixing W removes the main cause of the timeout. Additionally, `MaxAttemptsExceededException` is no longer reported **for this job only** (scoped via `dontReportWhen` on `$e->job?->resolveName()`); other jobs still report it. ## Tests - `getBalances` wraps connection failures and ASPSP errors as non-reportable transient errors; keeps non-ASPSP client errors reportable. - `MaxAttemptsExceededException` is not reported for `SyncBankingConnectionJob`, but still reported for other jobs. Fixes PHP-LARAVEL-W, PHP-LARAVEL-2D. --- ONBOARDING.md | 55 ++++++++++++++++ .../Banking/EnableBankingProvider.php | 27 +++++++- bootstrap/app.php | 5 ++ .../OpenBanking/EnableBankingProviderTest.php | 63 +++++++++++++++++++ .../SyncBankingConnectionJobTest.php | 20 ++++++ 5 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 ONBOARDING.md diff --git a/ONBOARDING.md b/ONBOARDING.md new file mode 100644 index 00000000..df7320c8 --- /dev/null +++ b/ONBOARDING.md @@ -0,0 +1,55 @@ +# Welcome to Whisper Money + +## How We Use Claude + +Based on Víctor Falcón's usage over the last 30 days: + +Work Type Breakdown: + Debug Fix ████████████████████ 100% + +Top Skills & Commands: + /sentry-cli ████████████████████ 1x/month + +Top MCP Servers: + _None used in this window_ + +## Your Setup Checklist + +### Codebases +- [ ] whisper-money — https://github.com/whisper-money/whisper-money + +### MCP Servers to Activate +- [ ] _None required yet_ — the team hasn't leaned on MCP servers in this window. Sentry access is via the `/sentry-cli` skill (see below). + +### Skills to Know About +- [ ] `/sentry-cli` — drives Sentry from the command line (view issues, events, projects, orgs, make API calls). The team uses it when triaging and fixing flagged errors before opening a PR. + +## Team Tips + +_TODO_ + +## Get Started + +_TODO_ + + diff --git a/app/Services/Banking/EnableBankingProvider.php b/app/Services/Banking/EnableBankingProvider.php index 1fe589f1..35eeecd7 100644 --- a/app/Services/Banking/EnableBankingProvider.php +++ b/app/Services/Banking/EnableBankingProvider.php @@ -129,9 +129,32 @@ class EnableBankingProvider implements BankingProviderInterface public function getBalances(string $accountId): array { - $response = $this->client()->get("/accounts/{$accountId}/balances"); + try { + $response = $this->client()->get("/accounts/{$accountId}/balances"); - $response->throw(); + $response->throw(); + } catch (ConnectionException $e) { + throw new TransientBankingProviderException( + 'EnableBanking did not respond while fetching account balances.', + provider: 'enablebanking', + previous: $e, + ); + } catch (RequestException $e) { + if (! $this->isAspspError($e)) { + throw $e; + } + + $body = $this->errorBody($e); + $providerCode = $body['error'] ?? null; + + throw new TransientBankingProviderException( + 'EnableBanking bank connector failed while fetching account balances.', + provider: 'enablebanking', + statusCode: $e->response->status(), + providerCode: is_string($providerCode) ? $providerCode : null, + previous: $e, + ); + } return $response->json(); } diff --git a/bootstrap/app.php b/bootstrap/app.php index 972f7362..402cb719 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -7,12 +7,14 @@ use App\Http\Middleware\HandleAppearance; use App\Http\Middleware\HandleInertiaRequests; use App\Http\Middleware\SetLocale; use App\Http\Middleware\SetSentryUser; +use App\Jobs\SyncBankingConnectionJob; use App\Services\AuthEntryPointService; use Illuminate\Foundation\Application; use Illuminate\Foundation\Configuration\Exceptions; use Illuminate\Foundation\Configuration\Middleware; use Illuminate\Http\Middleware\AddLinkHeadersForPreloadedAssets; use Illuminate\Http\Request; +use Illuminate\Queue\MaxAttemptsExceededException; use Sentry\Laravel\Integration; return Application::configure(basePath: dirname(__DIR__)) @@ -52,4 +54,7 @@ return Application::configure(basePath: dirname(__DIR__)) }) ->withExceptions(function (Exceptions $exceptions): void { Integration::handles($exceptions); + + $exceptions->dontReportWhen(fn (Throwable $e): bool => $e instanceof MaxAttemptsExceededException + && $e->job?->resolveName() === SyncBankingConnectionJob::class); })->create(); diff --git a/tests/Feature/OpenBanking/EnableBankingProviderTest.php b/tests/Feature/OpenBanking/EnableBankingProviderTest.php index 1379f5f7..bc8e7e96 100644 --- a/tests/Feature/OpenBanking/EnableBankingProviderTest.php +++ b/tests/Feature/OpenBanking/EnableBankingProviderTest.php @@ -71,6 +71,69 @@ test('getTransactions keeps non-ASPSP client errors reportable', function () { ->toThrow(RequestException::class); }); +test('getBalances wraps connection failures as non-reportable transient errors', function () { + Http::fake([ + 'api.enablebanking.com/accounts/ext-123/balances' => Http::failedConnection(), + ]); + + $provider = enableBankingProviderForTest(); + + try { + $provider->getBalances('ext-123'); + } catch (TransientBankingProviderException $e) { + expect($e)->toBeInstanceOf(ShouldntReport::class) + ->and($e->provider)->toBe('enablebanking') + ->and($e->statusCode)->toBeNull() + ->and($e->providerCode)->toBeNull() + ->and($e->getPrevious())->toBeInstanceOf(ConnectionException::class); + + return; + } + + test()->fail('Expected transient banking provider exception.'); +}); + +test('getBalances wraps EnableBanking ASPSP errors as non-reportable transient errors', function () { + Http::fake([ + 'api.enablebanking.com/accounts/ext-123/balances' => Http::response([ + 'code' => 400, + 'message' => 'Error interacting with ASPSP', + 'error' => 'ASPSP_ERROR', + ], 400), + ]); + + $provider = enableBankingProviderForTest(); + + try { + $provider->getBalances('ext-123'); + } catch (TransientBankingProviderException $e) { + expect($e)->toBeInstanceOf(ShouldntReport::class) + ->and($e->provider)->toBe('enablebanking') + ->and($e->statusCode)->toBe(400) + ->and($e->providerCode)->toBe('ASPSP_ERROR') + ->and($e->getPrevious())->toBeInstanceOf(RequestException::class); + + return; + } + + test()->fail('Expected transient banking provider exception.'); +}); + +test('getBalances keeps non-ASPSP client errors reportable', function () { + Http::fake([ + 'api.enablebanking.com/accounts/ext-123/balances' => Http::response([ + 'code' => 400, + 'message' => 'Invalid account', + 'error' => 'VALIDATION_ERROR', + ], 400), + ]); + + $provider = enableBankingProviderForTest(); + + expect(fn () => $provider->getBalances('ext-123')) + ->toThrow(RequestException::class); +}); + function enableBankingProviderForTest(): EnableBankingProvider { $privateKey = <<<'PEM' diff --git a/tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php b/tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php index 9a6fad92..074d631a 100644 --- a/tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php +++ b/tests/Feature/OpenBanking/SyncBankingConnectionJobTest.php @@ -18,8 +18,10 @@ use App\Services\Banking\BalanceSyncService; use App\Services\Banking\TransactionSyncService; use Carbon\Carbon; use GuzzleHttp\Psr7\Response; +use Illuminate\Contracts\Debug\ExceptionHandler; use Illuminate\Contracts\Queue\Job; use Illuminate\Http\Client\RequestException; +use Illuminate\Queue\MaxAttemptsExceededException; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Mail; use Illuminate\Support\Facades\Queue; @@ -1454,3 +1456,21 @@ test('successful sync clears rate_limited_until', function () { $connection->refresh(); expect($connection->rate_limited_until)->toBeNull(); }); + +test('does not report MaxAttemptsExceededException raised for SyncBankingConnectionJob', function () { + $queueJob = Mockery::mock(Job::class); + $queueJob->shouldReceive('resolveName')->andReturn(SyncBankingConnectionJob::class); + + $exception = MaxAttemptsExceededException::forJob($queueJob); + + expect(app(ExceptionHandler::class)->shouldReport($exception))->toBeFalse(); +}); + +test('still reports MaxAttemptsExceededException raised for other jobs', function () { + $queueJob = Mockery::mock(Job::class); + $queueJob->shouldReceive('resolveName')->andReturn(SendDailyBankTransactionsSyncedEmailJob::class); + + $exception = MaxAttemptsExceededException::forJob($queueJob); + + expect(app(ExceptionHandler::class)->shouldReport($exception))->toBeTrue(); +});