diff --git a/app/Services/Banking/Sync/WiseSyncer.php b/app/Services/Banking/Sync/WiseSyncer.php index 0a75096b..df461b4d 100644 --- a/app/Services/Banking/Sync/WiseSyncer.php +++ b/app/Services/Banking/Sync/WiseSyncer.php @@ -2,13 +2,32 @@ namespace App\Services\Banking\Sync; +use App\Exceptions\Banking\TransientBankingProviderException; use App\Models\BankingConnection; use App\Services\Banking\WiseBalanceSyncService; use App\Services\Banking\WiseClient; use App\Services\Banking\WiseTransactionSyncService; +use Illuminate\Support\Facades\Log; class WiseSyncer extends AbstractBankingConnectionSyncer { + /** + * Wall-clock budget for the whole connection, sized against + * SyncBankingConnectionJob's 120s timeout. + * + * A safety net rather than a policy: with the cursor fixed a normal wallet + * finishes in a couple of requests. It exists so that a genuinely long history + * - or a provider answering slowly - stops the run short instead of getting the + * job killed, which is what used to leave last_synced_at unset and every cycle + * restarting the same first sync. + * + * Per connection, not per wallet: Wise creates one account per currency per + * profile, so a budget each would multiply straight past the job's timeout. + * Worst case is this budget plus one in-flight request (WiseClient caps those), + * which leaves headroom inside 120s. + */ + private const int SYNC_BUDGET_SECONDS = 90; + public function __construct( private WiseTransactionSyncService $transactionSync, private WiseBalanceSyncService $balanceSync, @@ -22,20 +41,52 @@ class WiseSyncer extends AbstractBankingConnectionSyncer $dateTo = now()->toDateString(); $client = new WiseClient($connection->api_token); + $deadline = now()->addSeconds(self::SYNC_BUDGET_SECONDS); $connection->load('accounts'); $transactionsPerAccount = []; + $balancesFailed = 0; + $walletsSkipped = 0; foreach ($connection->accounts as $account) { - $count = $this->transactionSync->sync($account, $client, $dateFrom, $dateTo); - $this->balanceSync->sync($account, $client); - $transactionsPerAccount[$account->name] = $count; + if (now()->gte($deadline)) { + // Better to leave a wallet for the next cycle than to have the job + // killed, which would discard the sync timestamp for all of them. + $walletsSkipped++; + + continue; + } + + // Balance first, and its failure must not cost the transactions: it is + // one request against a walk that can span many, so ordering it second + // meant a wallet whose history could not be walked was missing from net + // worth entirely. + try { + $this->balanceSync->sync($account, $client); + } catch (TransientBankingProviderException $e) { + $balancesFailed++; + + Log::warning('Wise balance sync failed, continuing', [ + 'account_id' => $account->id, + 'error' => $e->getMessage(), + ]); + } + + $transactionsPerAccount[$account->name] = $this->transactionSync->sync( + $account, + $client, + $dateFrom, + $dateTo, + $deadline, + ); } return [ 'transactions_synced' => array_sum($transactionsPerAccount), 'transactions_per_account' => $transactionsPerAccount, + 'balances_failed' => $balancesFailed, + 'wallets_skipped_on_budget' => $walletsSkipped, ]; } } diff --git a/app/Services/Banking/WiseClient.php b/app/Services/Banking/WiseClient.php index 77e9d808..51ff0dee 100644 --- a/app/Services/Banking/WiseClient.php +++ b/app/Services/Banking/WiseClient.php @@ -13,6 +13,15 @@ class WiseClient { private const BASE_URL = 'https://api.wise.com'; + /** + * Explicit rather than the framework's 30s default: the caller's time budget is + * stated as "the budget plus one in-flight request", which only holds if this + * class owns that number. Matches the sibling banking clients. + */ + private const int HTTP_TIMEOUT_SECONDS = 15; + + private const int HTTP_CONNECT_TIMEOUT_SECONDS = 5; + public function __construct(private string $apiToken) {} /** @@ -39,6 +48,10 @@ class WiseClient * Fetch paginated monetary activities for a profile. * Use `since`/`until` (ISO 8601) for date range and `cursor` for pagination. * + * The names are asymmetric and it matters: Wise returns the cursor as + * `cursor` but only reads it back as `nextCursor`. Sending it as `cursor` is + * silently ignored, so every request returns the first page again. + * * @return array{activities?: array, cursor?: string|null} */ public function getActivities(int $profileId, string $since, string $until, ?string $cursor = null): array @@ -50,7 +63,7 @@ class WiseClient ]; if ($cursor !== null) { - $params['cursor'] = $cursor; + $params['nextCursor'] = $cursor; } return $this->get("/v1/profiles/{$profileId}/activities", $params); @@ -97,6 +110,8 @@ class WiseClient private function client(): PendingRequest { return Http::baseUrl(self::BASE_URL) + ->timeout(self::HTTP_TIMEOUT_SECONDS) + ->connectTimeout(self::HTTP_CONNECT_TIMEOUT_SECONDS) ->withToken($this->apiToken) ->acceptJson() ->throw(function ($response, RequestException $exception) { diff --git a/app/Services/Banking/WiseTransactionSyncService.php b/app/Services/Banking/WiseTransactionSyncService.php index a280ccc3..e8b60e3a 100644 --- a/app/Services/Banking/WiseTransactionSyncService.php +++ b/app/Services/Banking/WiseTransactionSyncService.php @@ -4,6 +4,7 @@ namespace App\Services\Banking; use App\Enums\TransactionSource; use App\Models\Account; +use Carbon\CarbonInterface; use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Support\Facades\Log; @@ -15,9 +16,14 @@ class WiseTransactionSyncService * The account's `external_account_id` must be in the format * "{profileId}:{currency}" (e.g. "36875276:EUR"). * + * @param CarbonInterface|null $deadline Wall clock at which to stop paging and + * leave the rest for the next sync. The + * caller owns it because the budget is + * per connection, not per wallet. Null + * for callers under no time pressure. * @return int Number of new transactions created */ - public function sync(Account $account, WiseClient $client, string $dateFrom, string $dateTo): int + public function sync(Account $account, WiseClient $client, string $dateFrom, string $dateTo, ?CarbonInterface $deadline = null): int { if (! $account->external_account_id) { return 0; @@ -29,29 +35,18 @@ class WiseTransactionSyncService $until = $dateTo.'T23:59:59Z'; $cursor = null; $created = 0; + $ranOutOfBudget = false; do { $result = $client->getActivities((int) $profileId, $since, $until, $cursor); $activities = $result['activities'] ?? []; $cursor = $result['cursor'] ?? null; - foreach ($activities as $activity) { - // Skip zero-amount authorization checks and non-monetary types - if (($activity['type'] ?? '') === 'CARD_CHECK') { - continue; - } + $created += $this->importPage($account, $activities, $currency); - $parsed = $this->parseActivity($activity, $currency); - - if ($parsed === null) { - continue; - } - - if ($this->importTransaction($account, $activity, $parsed)) { - $created++; - } - } - } while ($cursor !== null && count($activities) > 0); + $hasMorePages = $cursor !== null && count($activities) > 0; + $ranOutOfBudget = $hasMorePages && $deadline !== null && now()->gte($deadline); + } while ($hasMorePages && ! $ranOutOfBudget); Log::info('Synced Wise transactions', [ 'account_id' => $account->id, @@ -61,6 +56,45 @@ class WiseTransactionSyncService 'date_to' => $dateTo, ]); + if ($ranOutOfBudget) { + // Warn, not info: pages were left behind and the next sync starts from + // last_synced_at, so this is history the user silently does not get. + Log::warning('Wise transaction walk stopped on its time budget', [ + 'account_id' => $account->id, + 'currency' => $currency, + 'new_transactions' => $created, + 'oldest_imported' => $account->transactions()->min('transaction_date'), + ]); + } + + return $created; + } + + /** + * @param array> $activities + * @return int Number of new transactions created from this page + */ + private function importPage(Account $account, array $activities, string $currency): int + { + $created = 0; + + foreach ($activities as $activity) { + // Skip zero-amount authorization checks and non-monetary types + if (($activity['type'] ?? '') === 'CARD_CHECK') { + continue; + } + + $parsed = $this->parseActivity($activity, $currency); + + if ($parsed === null) { + continue; + } + + if ($this->importTransaction($account, $activity, $parsed)) { + $created++; + } + } + return $created; } diff --git a/tests/Feature/OpenBanking/WisePaginationTest.php b/tests/Feature/OpenBanking/WisePaginationTest.php new file mode 100644 index 00000000..5626ed2b --- /dev/null +++ b/tests/Feature/OpenBanking/WisePaginationTest.php @@ -0,0 +1,64 @@ +onboarded()->create(['currency_code' => 'EUR']); + $connection = BankingConnection::factory()->wise()->create(['user_id' => $user->id]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => '35242290:EUR', + 'currency_code' => 'EUR', + ]); + + // Wise hands the cursor back as `cursor` but only reads it as `nextCursor`, + // so a fake that keys off `nextCursor` is what tells the two apart: sending + // the wrong name looks exactly like a one-page history that never ends. + Http::fake([ + 'api.wise.com/v1/profiles/*/activities*' => function ($request) { + $cursor = $request->data()['nextCursor'] ?? null; + + return Http::response(match ($cursor) { + null => [ + 'activities' => [activityFixture(1)], + 'cursor' => 'page-2', + ], + 'page-2' => [ + 'activities' => [activityFixture(2)], + 'cursor' => null, + ], + default => ['activities' => [], 'cursor' => null], + }); + }, + ]); + + $created = app(WiseTransactionSyncService::class)->sync( + $account, + new WiseClient('test-token'), + now()->subYear()->toDateString(), + now()->toDateString(), + ); + + expect($created)->toBe(2); + Http::assertSentCount(2); + expect($account->transactions()->pluck('description')->sort()->values()->all()) + ->toBe(['Purchase 1', 'Purchase 2']); +}); + +function activityFixture(int $n): array +{ + return [ + 'id' => "activity-{$n}", + 'type' => 'CARD_PAYMENT', + 'title' => "Purchase {$n}", + 'primaryAmount' => '10.00 EUR', + 'secondaryAmount' => '', + 'createdOn' => now()->subDays($n)->toIso8601String(), + ]; +} diff --git a/tests/Feature/OpenBanking/WiseSyncBudgetTest.php b/tests/Feature/OpenBanking/WiseSyncBudgetTest.php new file mode 100644 index 00000000..a26ad1d7 --- /dev/null +++ b/tests/Feature/OpenBanking/WiseSyncBudgetTest.php @@ -0,0 +1,123 @@ + Http::response([ + [ + 'id' => 44333087, + 'profileId' => 35242290, + 'balances' => [ + ['currency' => 'EUR', 'amount' => ['value' => 19.81, 'currency' => 'EUR']], + ], + ], + ]), + 'api.wise.com/v1/profiles/*/activities*' => function ($request) use ($secondsPerPage, $pages) { + $page = (int) str_replace('page-', '', $request->data()['nextCursor'] ?? 'page-1'); + Carbon::setTestNow(Carbon::now()->addSeconds($secondsPerPage)); + + return Http::response([ + 'activities' => [[ + 'id' => "activity-{$page}", + 'type' => 'CARD_PAYMENT', + 'title' => "Purchase {$page}", + 'primaryAmount' => '10.00 EUR', + 'secondaryAmount' => '', + 'createdOn' => now()->subDays($page)->toIso8601String(), + ]], + 'cursor' => $page < $pages ? 'page-'.($page + 1) : null, + ]); + }, + ]); +} + +function wiseWalletsFor(User $user, BankingConnection $connection, string ...$currencies): void +{ + foreach ($currencies as $currency) { + Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => '35242290:'.$currency, + 'currency_code' => $currency, + ]); + } +} + +afterEach(function () { + Carbon::setTestNow(); +}); + +test('a history longer than the budget stops short instead of getting the job killed', function () { + $user = User::factory()->onboarded()->create(['currency_code' => 'EUR']); + $connection = BankingConnection::factory()->wise()->create(['user_id' => $user->id]); + wiseWalletsFor($user, $connection, 'EUR'); + + fakeLongWiseHistory(); + + $metadata = app(WiseSyncer::class)->sync($connection, isFirstSync: true); + + // 20s of clock per page against a 90s budget: the fifth page closes it, so the + // walk stops well before the 40 pages on offer. + expect($metadata['transactions_synced'])->toBe(5); + expect($metadata['wallets_skipped_on_budget'])->toBe(0); +}); + +test('the budget covers the whole connection, not each wallet separately', function () { + $user = User::factory()->onboarded()->create(['currency_code' => 'EUR']); + $connection = BankingConnection::factory()->wise()->create(['user_id' => $user->id]); + // Wise creates one account per currency per profile, so multi-wallet is the + // normal case. A budget each would multiply past the job's 120s timeout and + // reproduce the original bug. + wiseWalletsFor($user, $connection, 'EUR', 'USD', 'GBP'); + + fakeLongWiseHistory(); + + $metadata = app(WiseSyncer::class)->sync($connection, isFirstSync: true); + + expect($metadata['wallets_skipped_on_budget'])->toBeGreaterThan(0); + // The whole run stays within one budget's worth of pages rather than one per + // wallet, which is what keeps it inside the job's timeout. + expect(count(Http::recorded()))->toBeLessThan(12); +}); + +test('a wallet whose balance request fails still gets its transactions', function () { + $user = User::factory()->onboarded()->create(['currency_code' => 'EUR']); + $connection = BankingConnection::factory()->wise()->create(['user_id' => $user->id]); + wiseWalletsFor($user, $connection, 'EUR'); + + Http::fake([ + 'api.wise.com/v2/borderless-accounts*' => Http::response(['message' => 'boom'], 500), + 'api.wise.com/v1/profiles/*/activities*' => Http::response([ + 'activities' => [[ + 'id' => 'activity-1', + 'type' => 'CARD_PAYMENT', + 'title' => 'Purchase 1', + 'primaryAmount' => '10.00 EUR', + 'secondaryAmount' => '', + 'createdOn' => now()->subDay()->toIso8601String(), + ]], + 'cursor' => null, + ]), + ]); + + $metadata = app(WiseSyncer::class)->sync($connection, isFirstSync: true); + + // Ordering the balance first must not make it load-bearing: before this, a + // balance hiccup would have cost the user their transactions too. + expect($metadata['balances_failed'])->toBe(1); + expect($metadata['transactions_synced'])->toBe(1); +});