fix(banking): recover from EnableBanking 422 wrong-period instead of crashing the sync (PHP-LARAVEL-42) (#653)

## 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.
This commit is contained in:
Víctor Falcón 2026-07-07 08:57:23 +02:00 committed by GitHub
parent a38dc5dd95
commit 9b7632f585
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 311 additions and 21 deletions

View File

@ -0,0 +1,24 @@
<?php
namespace App\Exceptions\Banking;
use Exception;
use Illuminate\Contracts\Debug\ShouldntReport;
use Throwable;
/**
* The banking provider rejected the requested transactions date range as wider
* than the bank is willing to serve (EnableBanking HTTP 422 "Wrong transactions
* period requested"). Recoverable by retrying with a narrower window, so it is
* not reported: the sync layer clamps and retries, and only skips the account
* if even the narrowest window is refused.
*/
class WrongTransactionsPeriodException extends Exception implements ShouldntReport
{
public function __construct(
string $message,
?Throwable $previous = null,
) {
parent::__construct($message, 0, $previous);
}
}

View File

@ -6,6 +6,7 @@ use App\Contracts\BankingProviderInterface;
use App\Exceptions\Banking\ExpiredBankingSessionException;
use App\Exceptions\Banking\InaccessibleBankAccountException;
use App\Exceptions\Banking\TransientBankingProviderException;
use App\Exceptions\Banking\WrongTransactionsPeriodException;
use Firebase\JWT\JWT;
use Illuminate\Http\Client\ConnectionException;
use Illuminate\Http\Client\PendingRequest;
@ -119,6 +120,13 @@ class EnableBankingProvider implements BankingProviderInterface
);
}
if ($this->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<string, mixed>
*/
@ -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(),

View File

@ -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;

View File

@ -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<int>
*/
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.
*

View File

@ -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([

View File

@ -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([

View File

@ -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);
});