From 9b7632f58563fc3b06eda62b06b90b26d4c2d0a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Tue, 7 Jul 2026 08:57:23 +0200 Subject: [PATCH] fix(banking): recover from EnableBanking 422 wrong-period instead of crashing the sync (PHP-LARAVEL-42) (#653) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem (Sentry PHP-LARAVEL-42) EnableBanking's `GET /accounts/{id}/transactions` returns **HTTP 422 "Wrong transactions period requested"** when the requested date range is wider than the bank is willing to serve. The catch-ladder in `EnableBankingProvider::getTransactions()` only matched 401/EXPIRED_SESSION, 400/AccountNotAccessible and 400/ASPSP_ERROR, so the 422 rethrew a **raw `RequestException`**. That: 1. escaped the per-account loop in `EnableBankingSyncer::sync` (which only skips `InaccessibleBankAccountException`), so **every remaining account in the connection stopped syncing too**; 2. hit the job's generic `catch (\Throwable)` → retried 3× (deterministic, always the same 422) → connection marked **Error** and **reported to Sentry**; 3. after the scheduled-retry budget (`consecutive_sync_failures >= 3`) the whole connection was **dropped from scheduled sync** until a manual reconnect. Real user impacted (active connection). The failing request was a ~92-day window on the incremental/linked path. ## Fix (3 commits) 1. **Classify the 422** — new `WrongTransactionsPeriodException` (`ShouldntReport`) thrown from `getTransactions()` when status is 422 and the message names the period. Also stop logging handled 422s at `error` level in the HTTP client callback. 2. **Clamp + retry** — on that exception, `TransactionSyncService::sync` restarts the account from page 1 with a progressively narrower window (`90 → 30 → 7` days before `date_to`), so the user keeps as much history as the bank will serve. `strategy='longest'` is dropped on the narrowed retry so the explicit `date_from` is honoured; re-fetched pages are idempotent (fingerprint dedup + date-keyed daily balances). 3. **Graceful skip** — if even the narrowest window is refused, the syncer skips just that account (like an inaccessible account) and keeps the connection Active, instead of failing the whole sync. ## Why draft (needs a human call, per two review agents) The crash fix itself is well-covered and safe. What needs sign-off is the **product tradeoff** the clamp introduces: - **First-sync history truncation.** First sync requests `now()->subYear()`. If the bank refuses a year, we narrow to ≤90 days and there is **no back-fill path** (incremental syncs only move `date_from` forward), so the skipped history is lost. This is bounded and logged, but it is a deliberate behaviour change. - **Incremental catch-up gap.** If the watermark is older than the bank's servable window, the days between the watermark and the clamp are never fetched (silent, but logged). - **Heuristics worth a human eye:** the ladder values `[90, 30, 7]`, dropping `strategy='longest'` on retry, and detecting the error by `status 422 + message contains "period"` (ideally confirmed against EnableBanking docs / a stable error code). Low-risk per review: duplicate imports (fingerprint + `(account_id, dedup_fingerprint)` unique index are robust to overlapping windows) and balances (truncated, not corrupted). ## Testing - Provider: 422 wrong-period → `WrongTransactionsPeriodException`; unrelated 422 stays a raw `RequestException`. - Service: clamps + retries once (asserts the clamped date and the `strategy` drop); gives up after the ladder is exhausted; does not retry an already-narrow window. - Syncer: a refused account is skipped, connection stays Active/unreported, siblings still sync. - Full `tests/Feature/OpenBanking` + `tests/Feature/Sync` green (315 tests); Pint and Larastan clean. Fixes PHP-LARAVEL-42 --- 🤖 Opened by the autonomous Sentry-triage loop. Draft on purpose — the data-truncation tradeoff above is a product decision for a human. --- .../WrongTransactionsPeriodException.php | 24 +++++ .../Banking/EnableBankingProvider.php | 25 ++++- .../Banking/Sync/EnableBankingSyncer.php | 9 +- .../Banking/TransactionSyncService.php | 100 +++++++++++++++--- .../OpenBanking/EnableBankingProviderTest.php | 40 +++++++ .../OpenBanking/SyncRetryAndLoggingTest.php | 45 ++++++++ .../TransactionSyncServiceTest.php | 89 ++++++++++++++++ 7 files changed, 311 insertions(+), 21 deletions(-) create mode 100644 app/Exceptions/Banking/WrongTransactionsPeriodException.php diff --git a/app/Exceptions/Banking/WrongTransactionsPeriodException.php b/app/Exceptions/Banking/WrongTransactionsPeriodException.php new file mode 100644 index 00000000..317b7828 --- /dev/null +++ b/app/Exceptions/Banking/WrongTransactionsPeriodException.php @@ -0,0 +1,24 @@ +isWrongPeriod($e)) { + throw new WrongTransactionsPeriodException( + 'EnableBanking rejected the requested transactions period as too wide.', + previous: $e, + ); + } + if (! $this->isAspspError($e)) { throw $e; } @@ -243,6 +251,20 @@ class EnableBankingProvider implements BankingProviderInterface && $errorName === 'AccountNotAccessibleException'; } + private function isWrongPeriod(RequestException $e): bool + { + $message = $this->errorBody($e)['message'] ?? null; + + // The bank refused the requested date range as too wide ("Wrong + // transactions period requested"). Keyed on 422 + the stable "period" + // token so genuine validation 422s (e.g. malformed dates) still surface. + // ponytail: message match; if EnableBanking adds a stable error code for + // this, key on that instead. + return $e->response->status() === 422 + && is_string($message) + && str_contains(strtolower($message), 'period'); + } + /** * @return array */ @@ -264,7 +286,8 @@ class EnableBankingProvider implements BankingProviderInterface $body = $response->json(); $error = is_array($body) ? ($body['error'] ?? null) : null; $isExpected = ($response->status() === 400 && $error === 'ASPSP_ERROR') - || ($response->status() === 401 && $error === 'EXPIRED_SESSION'); + || ($response->status() === 401 && $error === 'EXPIRED_SESSION') + || $response->status() === 422; Log::log($isExpected ? 'warning' : 'error', 'EnableBanking API error', [ 'status' => $response->status(), diff --git a/app/Services/Banking/Sync/EnableBankingSyncer.php b/app/Services/Banking/Sync/EnableBankingSyncer.php index e7724117..5bb0c0c5 100644 --- a/app/Services/Banking/Sync/EnableBankingSyncer.php +++ b/app/Services/Banking/Sync/EnableBankingSyncer.php @@ -3,6 +3,7 @@ namespace App\Services\Banking\Sync; use App\Exceptions\Banking\InaccessibleBankAccountException; +use App\Exceptions\Banking\WrongTransactionsPeriodException; use App\Jobs\SendDailyBankTransactionsSyncedEmailJob; use App\Models\BankingConnection; use App\Services\Banking\BalanceSyncService; @@ -63,13 +64,15 @@ class EnableBankingSyncer extends AbstractBankingConnectionSyncer $this->balanceSync->calculateHistoricalBalances($account); } } - } catch (InaccessibleBankAccountException) { - // A single account the bank no longer exposes must not break the + } catch (InaccessibleBankAccountException|WrongTransactionsPeriodException $e) { + // A single account the bank no longer exposes, or whose history + // window it refuses even after narrowing, 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', [ + Log::warning('Skipping unsyncable EnableBanking account during sync', [ 'connection_id' => $connection->id, 'account_id' => $account->id, + 'reason' => $e::class, ]); continue; diff --git a/app/Services/Banking/TransactionSyncService.php b/app/Services/Banking/TransactionSyncService.php index c6292ea1..ef05684f 100644 --- a/app/Services/Banking/TransactionSyncService.php +++ b/app/Services/Banking/TransactionSyncService.php @@ -4,12 +4,24 @@ namespace App\Services\Banking; use App\Contracts\BankingProviderInterface; use App\Enums\TransactionSource; +use App\Exceptions\Banking\WrongTransactionsPeriodException; use App\Models\Account; use Illuminate\Database\UniqueConstraintViolationException; +use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Log; class TransactionSyncService { + /** + * Fallback lookback windows (in days back from date_to) tried in order when + * the bank rejects the requested transactions period as too wide. Ordered + * widest-first so the user keeps as much history as the bank will serve; + * the last step is the floor before the account is skipped. + * + * @var list + */ + private const array WRONG_PERIOD_LOOKBACK_DAYS = [90, 30, 7]; + public function __construct( private BankingProviderInterface $provider, private TransactionDescriptionFormatter $descriptionFormatter, @@ -40,27 +52,59 @@ class TransactionSyncService // ever dwarfs its sync window, narrow this to the incoming batch's keys. [$knownFingerprints, $knownExternalIds] = $this->loadExistingDedupKeys($account); - do { - $result = $this->provider->getTransactions( - $account->external_account_id, - $dateFrom, - $dateTo, - $continuationKey, - $strategy, - ); + // The bank can reject the requested window as too wide (HTTP 422). When + // that happens, restart the account from the first page with a + // progressively narrower window so the user still gets the history the + // bank is willing to serve, instead of crashing the whole connection + // sync. Re-fetched pages are idempotent (dedup skips already-imported + // rows; daily balances are keyed by date), and strategy is dropped on + // the narrowed retry so the explicit date_from is honoured rather than + // overridden by "longest". + while (true) { + try { + $continuationKey = null; - foreach ($result['transactions'] as $transaction) { - if ($this->importTransaction($account, $transaction, $bankName, $knownFingerprints, $knownExternalIds)) { - $created++; + do { + $result = $this->provider->getTransactions( + $account->external_account_id, + $dateFrom, + $dateTo, + $continuationKey, + $strategy, + ); + + foreach ($result['transactions'] as $transaction) { + if ($this->importTransaction($account, $transaction, $bankName, $knownFingerprints, $knownExternalIds)) { + $created++; + } + + if ($saveDailyBalances) { + $this->trackDailyBalance($transaction, $dailyBalances); + } + } + + $continuationKey = $result['continuation_key']; + } while ($continuationKey); + + break; + } catch (WrongTransactionsPeriodException $e) { + $narrowedDateFrom = $this->nextNarrowerDateFrom($dateFrom, $dateTo); + + if ($narrowedDateFrom === null) { + throw $e; } - if ($saveDailyBalances) { - $this->trackDailyBalance($transaction, $dailyBalances); - } + Log::warning('EnableBanking rejected the transactions period; retrying with a narrower window', [ + 'account_id' => $account->id, + 'rejected_date_from' => $dateFrom, + 'retry_date_from' => $narrowedDateFrom, + 'date_to' => $dateTo, + ]); + + $dateFrom = $narrowedDateFrom; + $strategy = null; } - - $continuationKey = $result['continuation_key']; - } while ($continuationKey); + } if ($saveDailyBalances) { $this->saveDailyBalances($account, $dailyBalances); @@ -76,6 +120,28 @@ class TransactionSyncService return $created; } + /** + * The next window start strictly narrower (later) than the current one, + * stepping down the bounded lookback ladder. Returns null when no ladder + * step narrows the current window, so the caller gives up on the account + * rather than looping forever. Candidates are always <= date_to, so the + * window never inverts. Dates are 'Y-m-d', where string order is date order. + */ + private function nextNarrowerDateFrom(string $dateFrom, string $dateTo): ?string + { + $to = Carbon::parse($dateTo); + + foreach (self::WRONG_PERIOD_LOOKBACK_DAYS as $days) { + $candidate = $to->copy()->subDays($days)->toDateString(); + + if ($candidate > $dateFrom) { + return $candidate; + } + } + + return null; + } + /** * Import a single transaction, skipping duplicates. * diff --git a/tests/Feature/OpenBanking/EnableBankingProviderTest.php b/tests/Feature/OpenBanking/EnableBankingProviderTest.php index 59b1195e..78b66a92 100644 --- a/tests/Feature/OpenBanking/EnableBankingProviderTest.php +++ b/tests/Feature/OpenBanking/EnableBankingProviderTest.php @@ -3,6 +3,7 @@ use App\Exceptions\Banking\ExpiredBankingSessionException; use App\Exceptions\Banking\InaccessibleBankAccountException; use App\Exceptions\Banking\TransientBankingProviderException; +use App\Exceptions\Banking\WrongTransactionsPeriodException; use App\Services\Banking\EnableBankingProvider; use Illuminate\Contracts\Debug\ShouldntReport; use Illuminate\Http\Client\ConnectionException; @@ -158,6 +159,45 @@ test('getBalances wraps an inaccessible account 400 as a non-reportable error', test()->fail('Expected inaccessible bank account exception.'); }); +test('getTransactions wraps a 422 wrong-period as a non-reportable error', function () { + Http::fake([ + 'api.enablebanking.com/accounts/ext-123/transactions*' => Http::response([ + 'code' => 422, + 'message' => 'Wrong transactions period requested', + 'detail' => [ + 'message' => 'Timestamp no en periodo de tiempo aceptable', + ], + ], 422), + ]); + + $provider = enableBankingProviderForTest(); + + try { + $provider->getTransactions('ext-123', '2026-04-06', '2026-07-07'); + } catch (WrongTransactionsPeriodException $e) { + expect($e)->toBeInstanceOf(ShouldntReport::class) + ->and($e->getPrevious())->toBeInstanceOf(RequestException::class); + + return; + } + + test()->fail('Expected wrong transactions period exception.'); +}); + +test('getTransactions keeps unrelated 422 validation errors reportable', function () { + Http::fake([ + 'api.enablebanking.com/accounts/ext-123/transactions*' => Http::response([ + 'code' => 422, + 'message' => 'Invalid account identifier', + ], 422), + ]); + + $provider = enableBankingProviderForTest(); + + expect(fn () => $provider->getTransactions('ext-123', '2026-04-06', '2026-07-07')) + ->toThrow(RequestException::class); +}); + 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 22c28fc0..162da397 100644 --- a/tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php +++ b/tests/Feature/OpenBanking/SyncRetryAndLoggingTest.php @@ -5,6 +5,7 @@ use App\Enums\BankingSyncLogStatus; use App\Exceptions\Banking\ExpiredBankingSessionException; use App\Exceptions\Banking\InaccessibleBankAccountException; use App\Exceptions\Banking\TransientBankingProviderException; +use App\Exceptions\Banking\WrongTransactionsPeriodException; use App\Jobs\SyncAllBankingConnectionsJob; use App\Jobs\SyncBankingConnectionJob; use App\Mail\BankingConnectionExpiredEmail; @@ -322,6 +323,50 @@ test('an inaccessible account is skipped and the rest of the connection still sy expect($log->metadata['transactions_synced'])->toBe(2); }); +test('an account whose period the bank refuses is skipped, not crashed, and the rest 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' => 'narrow-refused-account', + ]); + + $transactionSync = Mockery::mock(TransactionSyncService::class); + $transactionSync->shouldReceive('sync')->andReturnUsing(function ($account) { + if ($account->external_account_id === 'narrow-refused-account') { + throw new WrongTransactionsPeriodException('EnableBanking refused every window.'); + } + + return 2; + }); + + $balanceSync = Mockery::mock(BalanceSyncService::class); + $balanceSync->shouldReceive('sync')->once(); + + $job = new SyncBankingConnectionJob($connection); + + // The refused account must not break the connection sync, mark it Error, or + // be reported — same resilience as an inaccessible account. + 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([ diff --git a/tests/Feature/OpenBanking/TransactionSyncServiceTest.php b/tests/Feature/OpenBanking/TransactionSyncServiceTest.php index 721272f7..a88ce545 100644 --- a/tests/Feature/OpenBanking/TransactionSyncServiceTest.php +++ b/tests/Feature/OpenBanking/TransactionSyncServiceTest.php @@ -2,6 +2,7 @@ use App\Contracts\BankingProviderInterface; use App\Enums\TransactionSource; +use App\Exceptions\Banking\WrongTransactionsPeriodException; use App\Models\Account; use App\Models\Bank; use App\Models\BankingConnection; @@ -674,3 +675,91 @@ test('sync dedupes against soft-deleted fingerprinted transactions', function () expect($account->transactions()->withTrashed()->count())->toBe(1); expect($account->transactions()->withTrashed()->first()->trashed())->toBeTrue(); }); + +test('sync clamps date_from and retries once when the bank rejects the period', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create(['user_id' => $user->id]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + + // Requested 92-day window is refused; the ladder's widest step (90 days + // before date_to = 2026-04-08) is retried, with strategy dropped to null. + $mockProvider->shouldReceive('getTransactions') + ->with('ext-123', '2026-04-06', '2026-07-07', null, 'longest') + ->once() + ->andThrow(new WrongTransactionsPeriodException('too wide')); + + $mockProvider->shouldReceive('getTransactions') + ->with('ext-123', '2026-04-08', '2026-07-07', null, null) + ->once() + ->andReturn([ + 'transactions' => [ + [ + 'transaction_id' => 'txn-001', + 'transaction_amount' => ['amount' => '50.00', 'currency' => 'EUR'], + 'credit_debit_indicator' => 'DBIT', + 'booking_date' => '2026-04-15', + 'remittance_information' => ['Grocery Store Purchase'], + ], + ], + 'continuation_key' => null, + ]); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + $created = $service->sync($account, '2026-04-06', '2026-07-07', 'longest', saveDailyBalances: false); + + expect($created)->toBe(1); + expect($account->transactions()->count())->toBe(1); +}); + +test('sync gives up on the account when even the narrowest window is rejected', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create(['user_id' => $user->id]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + + // Initial attempt + one per ladder step (90/30/7) = 4 refused attempts, + // then the service rethrows so the caller (syncer) can skip the account. + $mockProvider->shouldReceive('getTransactions') + ->times(4) + ->andThrow(new WrongTransactionsPeriodException('too wide')); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + + expect(fn () => $service->sync($account, '2026-04-06', '2026-07-07', saveDailyBalances: false)) + ->toThrow(WrongTransactionsPeriodException::class); +}); + +test('sync does not retry when the rejected window is already narrow', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create(['user_id' => $user->id]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + + // date_from is only 3 days back — no ladder step narrows it further, so the + // exception is rethrown immediately without a pointless retry. + $mockProvider->shouldReceive('getTransactions') + ->with('ext-123', '2026-07-04', '2026-07-07', null, null) + ->once() + ->andThrow(new WrongTransactionsPeriodException('too wide')); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + + expect(fn () => $service->sync($account, '2026-07-04', '2026-07-07', saveDailyBalances: false)) + ->toThrow(WrongTransactionsPeriodException::class); +});