From cc97527c5ab3ef60e5e3dbd0e4b807e813f41d4f Mon Sep 17 00:00:00 2001 From: Maxim Babichev Date: Fri, 28 Nov 2025 19:42:24 +0300 Subject: [PATCH 1/3] Implement PostgreSQL row-level locking and optimize wallet locking mechanism - Added PostgresLockService for handling database-level locks with improved performance. - Updated WalletServiceProvider to resolve lock service based on configuration and database driver. - Enhanced caching strategy to ensure balance consistency when using PostgreSQL locks. - Updated documentation to reflect changes in locking behavior and benefits of using PostgreSQL. - Added tests for PostgresLockService to ensure correct functionality and performance. --- .github/workflows/phpunits.yaml | 2 +- .gitignore | 2 + config/config.php | 12 + docker-compose.yml | 23 ++ docs/guide/db/atomic-service.md | 2 +- docs/guide/db/race-condition.md | 35 +++ src/Internal/Service/PostgresLockService.php | 276 +++++++++++++++++ src/WalletServiceProvider.php | 73 ++++- .../Units/Service/PostgresLockServiceTest.php | 293 ++++++++++++++++++ 9 files changed, 707 insertions(+), 11 deletions(-) create mode 100644 docker-compose.yml create mode 100644 src/Internal/Service/PostgresLockService.php create mode 100644 tests/Units/Service/PostgresLockServiceTest.php diff --git a/.github/workflows/phpunits.yaml b/.github/workflows/phpunits.yaml index c808c74d0..a5661216d 100644 --- a/.github/workflows/phpunits.yaml +++ b/.github/workflows/phpunits.yaml @@ -21,7 +21,7 @@ jobs: php-versions: [8.3, 8.4, 8.5] databases: [testing, pgsql, mysql, mariadb] caches: [array, redis, memcached, database] - locks: [redis, memcached] + locks: [redis, memcached, database] services: redis: diff --git a/.gitignore b/.gitignore index 835f7141c..cf6ea7fc6 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,5 @@ build/ node_modules/ .deptrac.cache .phpunit.cache/ +PLAN.md +READMAP.md diff --git a/config/config.php b/config/config.php index c4ef3f09a..df5b48b70 100644 --- a/config/config.php +++ b/config/config.php @@ -84,6 +84,13 @@ /** * The driver for the cache. * + * Note: When using PostgreSQL with 'database' lock driver, the package + * automatically forces 'array' cache driver. This is CRITICAL because: + * 1. Before locking, balance MUST be read from DB with FOR UPDATE + * 2. This balance is synced to StorageService (state transaction) via multiSync() + * 3. External cache (database, redis, memcached) would be redundant and could cause inconsistencies + * 4. Array cache ensures balance is always fresh from DB within transaction + * * @var string */ 'driver' => env('WALLET_CACHE_DRIVER', 'array'), @@ -114,6 +121,11 @@ * - memcached * - database * + * When using 'database' driver with PostgreSQL, the package automatically + * uses PostgreSQL-specific row-level locks (SELECT ... FOR UPDATE) for + * better performance and consistency. For other databases, standard + * Laravel database locks are used. + * * @var string */ 'driver' => env('WALLET_LOCK_DRIVER', 'array'), diff --git a/docker-compose.yml b/docker-compose.yml new file mode 100644 index 000000000..17310d360 --- /dev/null +++ b/docker-compose.yml @@ -0,0 +1,23 @@ +version: '3.8' + +services: + postgres: + image: postgres:15-alpine + container_name: laravel-wallet-postgres + environment: + POSTGRES_USER: root + POSTGRES_PASSWORD: wallet + POSTGRES_DB: wallet + ports: + - "5432:5432" + volumes: + - postgres_data:/var/lib/postgresql/data + healthcheck: + test: ["CMD-SHELL", "pg_isready -U root"] + interval: 5s + timeout: 3s + retries: 5 + +volumes: + postgres_data: + diff --git a/docs/guide/db/atomic-service.md b/docs/guide/db/atomic-service.md index 96c5e345f..de779ed41 100644 --- a/docs/guide/db/atomic-service.md +++ b/docs/guide/db/atomic-service.md @@ -20,7 +20,7 @@ What's going on here? We block the wallet and raise the ad in the transaction (yes, atomic immediately starts the transaction - this is the main difference from LockServiceInterface). We raise the ad and deduct the amount from the wallet. If there are not enough funds to raise the ad, the error will complete the atomic operation and the transaction will roll back, and the lock on the wallet will be removed. -There is also an opportunity to block a lot of wallets. The operation is expensive, it generates N requests to the lock service. Maybe I'll optimize it in the future, but that's not for sure. +There is also an opportunity to block a lot of wallets. When using PostgreSQL with `lock.driver = 'database'`, the operation is optimized: all wallets are locked in a single database query (`SELECT ... FOR UPDATE`), significantly improving performance compared to multiple individual lock requests. --- diff --git a/docs/guide/db/race-condition.md b/docs/guide/db/race-condition.md index e640800b2..1503a92e3 100644 --- a/docs/guide/db/race-condition.md +++ b/docs/guide/db/race-condition.md @@ -34,4 +34,39 @@ You need `redis-server` and `php-redis`. Redis is recommended but not required. You can choose whatever the [framework](https://laravel.com/docs/8.x/cache#introduction) offers you. +## PostgreSQL Row-Level Locks + +When using PostgreSQL with `lock.driver = 'database'`, the package automatically uses PostgreSQL-specific row-level locks (`SELECT ... FOR UPDATE`) for optimal performance and data consistency. + +### Benefits + +- **Database-level locking**: Locks are managed directly by PostgreSQL, ensuring true atomicity +- **Better performance**: Single query locks multiple wallets at once, reducing database round trips +- **Automatic cache management**: The package automatically forces `array` cache driver when using PostgreSQL locks, as database-level locks ensure consistency without external cache synchronization + +### How It Works + +When you configure: +```php +'lock' => [ + 'driver' => 'database', +], +``` + +And your database connection is PostgreSQL, the package automatically: +1. Uses `PostgresLockService` instead of standard `LockService` +2. Locks wallets using `SELECT ... FOR UPDATE` at the database level +3. Forces `array` cache driver for optimal performance (external cache becomes redundant) + +### Important Notes + +- **Automatic selection**: No additional configuration needed - works automatically when `lock.driver = 'database'` and database is PostgreSQL +- **Array cache**: When using PostgreSQL locks, the package automatically forces `array` cache driver. This is **CRITICAL** because: + - Before locking, balance **MUST** be read from DB with `FOR UPDATE` + - This balance is synced to StorageService (state transaction) via `multiSync()` + - External cache (database, redis, memcached) would be redundant and could cause inconsistencies + - Array cache ensures balance is always fresh from DB within transaction +- **Other databases**: For non-PostgreSQL databases, standard Laravel database locks are used +- **Backward compatible**: All existing code continues to work without changes + It's simple! diff --git a/src/Internal/Service/PostgresLockService.php b/src/Internal/Service/PostgresLockService.php new file mode 100644 index 000000000..f50d28e7f --- /dev/null +++ b/src/Internal/Service/PostgresLockService.php @@ -0,0 +1,276 @@ +lockedKeys = $cacheFactory->store('array'); + } + + public function block(string $key, callable $callback): mixed + { + // Delegate to blocks() with single element array + return $this->blocks([$key], $callback); + } + + public function blocks(array $keys, callable $callback): mixed + { + // Filter out already blocked keys + $keysToLock = []; + foreach ($keys as $key) { + if (! $this->isBlocked($key)) { + $keysToLock[] = $key; + } + } + + // If all keys are already blocked, just execute callback + if ($keysToLock === []) { + return $callback(); + } + + // Sort keys to prevent deadlock + $sortedKeys = $this->sortKeys($keysToLock); + + // Normalize keys to UUIDs immediately + // Keys can be in two formats: + // 1. "wallet_lock::uuid" - full format (from AtomicService, tests) + // 2. "uuid" - just UUID (from BookkeeperService::multiAmount) + // 3. Non-UUID keys (e.g., from LockServiceTest using __METHOD__) + $uuids = []; + $nonUuidKeys = []; + + foreach ($sortedKeys as $key) { + // Extract UUID: remove prefix if present, otherwise key is UUID + $uuid = str_starts_with($key, self::LOCK_KEY) + ? str_replace(self::LOCK_KEY, '', $key) + : $key; + + if ($uuid === '') { + continue; + } + + // Simple check: UUID format is 36 chars with dashes (8-4-4-4-12) + // This is a lightweight check without full validation + if (strlen($uuid) === 36 && substr_count($uuid, '-') === 4) { + $uuids[] = $uuid; + } else { + $nonUuidKeys[] = $key; + } + } + + // Handle non-UUID keys: mark as blocked and execute callback without DB query + foreach ($nonUuidKeys as $key) { + $this->lockedKeys->put(self::INNER_KEYS.$key, true, $this->seconds); + } + + $connection = $this->connectionService->get(); + $inTransaction = $connection->transactionLevel() > 0; + + // If no UUIDs found, just execute callback + // For non-UUID keys inside transaction: keep locked until releases() (like UUID keys) + // For non-UUID keys outside transaction: clear in finally block + if ($uuids === []) { + if ($inTransaction) { + // Inside transaction: keep locked until releases() is called + return $callback(); + } + + // Outside transaction: clear after callback + try { + return $callback(); + } finally { + // Clear non-UUID keys after callback (similar to UUID keys in finally block) + foreach ($nonUuidKeys as $key) { + $this->lockedKeys->delete(self::INNER_KEYS.$key); + } + } + } + + if ($inTransaction) { + // ⚠️ CRITICAL: We are already inside a transaction! + // + // This happens in the following scenarios: + // 1. User created transaction manually (DB::beginTransaction()) + // 2. AtomicService::blocks() created transaction via databaseService->transaction() + // 3. BookkeeperService::multiAmount() called inside transaction and automatically locks wallet + // when record is not found in cache (RecordNotFoundException) + // + // AUTOMATIC LOCKING: + // - When user accesses $wallet->balanceInt inside transaction, + // this calls RegulatorService::amount() -> BookkeeperService::amount() -> multiAmount() + // - If record is not found in cache, BookkeeperService automatically calls + // lockService->blocks() to lock the wallet + // - This means lock can be called INSIDE an existing transaction + // + // In this case: + // - DO NOT create new transaction (we are already inside existing one) + // - Just set FOR UPDATE lock on existing transaction + // - Lock will be released automatically by PostgreSQL on commit/rollback + // - lockedKeys will be cleared via releases() after TransactionCommitted/RolledBack event + // - If wallets are already locked in this transaction, PostgreSQL will return them anyway + // (FOR UPDATE on already locked row in same transaction is safe and returns the row) + $this->lockWallets($uuids); + + return $callback(); + } + + // PostgresLockService creates transaction + // Clear lockedKeys after transaction completes to prevent accumulation in Octane + try { + return $connection->transaction(function () use ($uuids, $callback) { + $this->lockWallets($uuids); + + return $callback(); + }); + } finally { + // CRITICAL for Octane: clear lockedKeys after transaction completes + // This prevents accumulation in long-lived processes + foreach ($uuids as $uuid) { + $this->lockedKeys->delete(self::INNER_KEYS.$uuid); + } + } + } + + public function releases(array $keys): void + { + // Called from RegulatorService::purge() after TransactionCommitted/RolledBack + foreach ($keys as $key) { + // Normalize key to UUID (we store only UUIDs, not original key format) + $uuid = str_starts_with($key, self::LOCK_KEY) + ? str_replace(self::LOCK_KEY, '', $key) + : $key; + + if ($uuid !== '' && $this->lockedKeys->get(self::INNER_KEYS.$uuid) === true) { + // Clear lockedKeys - DB locks already released by PostgreSQL + $this->lockedKeys->delete(self::INNER_KEYS.$uuid); + } + } + } + + public function isBlocked(string $key): bool + { + // Normalize key to UUID (we store only UUIDs, not original key format) + $uuid = str_starts_with($key, self::LOCK_KEY) + ? str_replace(self::LOCK_KEY, '', $key) + : $key; + + if ($uuid === '') { + return false; + } + + return $this->lockedKeys->get(self::INNER_KEYS.$uuid) === true; + } + + /** + * Lock multiple wallets with FOR UPDATE and sync their balances to cache. + * + * CRITICAL: This method MUST read balance from DB before locking and sync it to state transaction. + * The balance is read with FOR UPDATE lock, then synced to StorageService (which uses array cache + * when PostgresLockService is active). This ensures balance is always fresh from DB within transaction. + * + * Optimized: single query for all wallets, single multiSync, single multiGet for verification. + * + * @param string[] $uuids Array of normalized UUIDs (already normalized, no prefix) + */ + private function lockWallets(array $uuids): void + { + if ($uuids === []) { + return; + } + + // CRITICAL: Read balance from DB with FOR UPDATE lock BEFORE syncing to state transaction + // This ensures we always have the latest balance from database, not from external cache + // OPTIMIZATION: Single query to lock all wallets at once + // SELECT * FROM wallets WHERE uuid IN (?, ?, ...) FOR UPDATE + try { + $wallets = Wallet::query() + ->whereIn('uuid', $uuids) + ->lockForUpdate() + ->get() + ->keyBy('uuid'); + } catch (QueryException $e) { + // PostgreSQL throws QueryException for invalid UUID format or other database errors + // Convert to ModelNotFoundException for consistency + throw new ModelNotFoundException( + 'Invalid wallet UUID or wallet not found: '.implode(', ', $uuids), + ExceptionInterface::MODEL_NOT_FOUND, + $e + ); + } + + // Extract balances from locked wallets (fresh from DB, not from cache) + // For wallets not found in DB (lazy creation), use balance 0 + $balances = []; + foreach ($uuids as $uuid) { + $wallet = $wallets->get($uuid); + if ($wallet !== null) { + // Wallet exists in DB - use balance from DB + $balances[$uuid] = $wallet->getOriginalBalanceAttribute(); + } else { + // Wallet doesn't exist in DB yet (lazy creation) - use balance 0 + // This is normal for new wallets that haven't been saved yet + $balances[$uuid] = '0'; + } + } + + // Mark all UUIDs as locked (store only UUID, already normalized) + foreach ($uuids as $uuid) { + $this->lockedKeys->put(self::INNER_KEYS.$uuid, true, $this->seconds); + } + + // CRITICAL: Sync balances to StorageService (state transaction) + // StorageService uses array cache when PostgresLockService is active, + // ensuring balance is stored in-memory for the transaction + // OPTIMIZATION: Single multiSync for all balances + $this->storageService->multiSync($balances); + + // OPTIMIZATION: Single multiGet to verify all balances at once + $cachedBalances = $this->storageService->multiGet($uuids); + + // CRITICAL CHECK: Verify cache sync for all wallets + foreach ($uuids as $uuid) { + $expectedBalance = $balances[$uuid]; + $cachedBalance = $cachedBalances[$uuid] ?? null; + + if ($cachedBalance !== $expectedBalance) { + throw new TransactionFailedException( + "CRITICAL: Cache sync failed for wallet {$uuid}. ". + "Expected: {$expectedBalance}, Got: {$cachedBalance}. ". + 'This may cause financial inconsistencies!', + ExceptionInterface::TRANSACTION_FAILED + ); + } + } + } + + private function sortKeys(array $keys): array + { + // Sort to prevent deadlock + $sorted = $keys; + sort($sorted); + + return $sorted; + } +} diff --git a/src/WalletServiceProvider.php b/src/WalletServiceProvider.php index 3eaaed4e9..cd36615e3 100644 --- a/src/WalletServiceProvider.php +++ b/src/WalletServiceProvider.php @@ -59,6 +59,7 @@ use Bavix\Wallet\Internal\Service\LockServiceInterface; use Bavix\Wallet\Internal\Service\MathService; use Bavix\Wallet\Internal\Service\MathServiceInterface; +use Bavix\Wallet\Internal\Service\PostgresLockService; use Bavix\Wallet\Internal\Service\StateService; use Bavix\Wallet\Internal\Service\StateServiceInterface; use Bavix\Wallet\Internal\Service\StorageService; @@ -182,7 +183,7 @@ public function register(): void $configure = config('wallet', []); $this->internal($configure['internal'] ?? []); - $this->services($configure['services'] ?? [], $configure['cache'] ?? []); + $this->services($configure['services'] ?? []); $this->repositories($configure['repositories'] ?? []); $this->transformers($configure['transformers'] ?? []); @@ -232,22 +233,32 @@ private function repositories(array $configure): void */ private function internal(array $configure): void { - $this->app->alias($configure['storage'] ?? StorageService::class, 'wallet.internal.storage'); - $this->app->when($configure['storage'] ?? StorageService::class) + $storageServiceClass = $configure['storage'] ?? StorageService::class; + $this->app->alias($storageServiceClass, 'wallet.internal.storage'); + $this->app->when($storageServiceClass) ->needs('$ttl') ->giveConfig('wallet.cache.ttl'); + // Register StorageServiceInterface binding so it can be injected + $this->app->bind(StorageServiceInterface::class, $storageServiceClass); + $this->app->singleton(ClockServiceInterface::class, $configure['clock'] ?? ClockService::class); $this->app->singleton(ConnectionServiceInterface::class, $configure['connection'] ?? ConnectionService::class); $this->app->singleton(DatabaseServiceInterface::class, $configure['database'] ?? DatabaseService::class); $this->app->singleton(DispatcherServiceInterface::class, $configure['dispatcher'] ?? DispatcherService::class); $this->app->singleton(JsonServiceInterface::class, $configure['json'] ?? JsonService::class); - $this->app->when($configure['lock'] ?? LockService::class) + // Resolve LockService class: if lock.driver = database, automatically select + // PostgresLockService when db = pgsql, otherwise use config or default + $lockServiceClass = config('wallet.lock.driver', 'array') === 'database' + ? $this->resolveLockService() + : ($configure['lock'] ?? $this->resolveLockService()); + + $this->app->when($lockServiceClass) ->needs('$seconds') ->giveConfig('wallet.lock.seconds', 1); - $this->app->singleton(LockServiceInterface::class, $configure['lock'] ?? LockService::class); + $this->app->singleton(LockServiceInterface::class, $lockServiceClass); $this->app->when($configure['math'] ?? MathService::class) ->needs('$scale') @@ -265,9 +276,8 @@ private function internal(array $configure): void /** * @param array $configure - * @param array{driver?: string|null} $cache */ - private function services(array $configure, array $cache): void + private function services(array $configure): void { $this->app->singleton(AssistantServiceInterface::class, $configure['assistant'] ?? AssistantService::class); $this->app->singleton(AtmServiceInterface::class, $configure['atm'] ?? AtmService::class); @@ -298,12 +308,15 @@ private function services(array $configure, array $cache): void // bookkeepper service $this->app->when(StorageServiceLockDecorator::class) ->needs(StorageServiceInterface::class) - ->give(function () use ($cache) { + ->give(function () { + // Force array cache when PostgresLockService is used + $cacheDriver = $this->resolveCacheDriver(); + return $this->app->make( 'wallet.internal.storage', [ 'cacheRepository' => $this->app->get(CacheFactory::class) - ->store($cache['driver'] ?? 'array'), + ->store($cacheDriver), ], ); }); @@ -550,4 +563,46 @@ private function bindObjectsProviders(): array { return [TransactionQueryHandlerInterface::class, TransferQueryHandlerInterface::class]; } + + /** + * Resolve the appropriate LockService class based on configuration and database driver. + * + * @return class-string + */ + private function resolveLockService(): string + { + // Early return if lock driver is not 'database' + if (config('wallet.lock.driver', 'array') !== 'database') { + return LockService::class; + } + + // Check if database driver is PostgreSQL + $connectionName = config('wallet.database.connection', config('database.default')); + if (config('database.connections.'.$connectionName.'.driver') === 'pgsql') { + return PostgresLockService::class; + } + + return LockService::class; + } + + /** + * Resolve the appropriate cache driver for StorageService. + */ + private function resolveCacheDriver(): string + { + // If PostgresLockService is used, force array cache + // This is CRITICAL because: + // 1. Before locking, we MUST read balance from DB with FOR UPDATE + // 2. We sync this balance to StorageService (state transaction) via multiSync() + // 3. External cache (database, redis, memcached) would be redundant and could cause inconsistencies + // 4. Array cache is in-memory and ensures balance is always fresh from DB within transaction + $lockServiceClass = $this->resolveLockService(); + + if ($lockServiceClass === PostgresLockService::class) { + return 'array'; + } + + // For all other cases - use configured cache driver + return config('wallet.cache.driver', 'array'); + } } diff --git a/tests/Units/Service/PostgresLockServiceTest.php b/tests/Units/Service/PostgresLockServiceTest.php new file mode 100644 index 000000000..06397181f --- /dev/null +++ b/tests/Units/Service/PostgresLockServiceTest.php @@ -0,0 +1,293 @@ +skipIfNotPostgresLockService(); + + /** @var User $user */ + $user = UserFactory::new()->create(); + $user->deposit(1000); + + $lock = app(LockServiceInterface::class); + self::assertInstanceOf(PostgresLockService::class, $lock); + + $key = 'wallet_lock::'.$user->wallet->uuid; + self::assertFalse($lock->isBlocked($key)); + + $result = $lock->block($key, static fn () => 'test'); + self::assertSame('test', $result); + self::assertFalse($lock->isBlocked($key)); + } + + public function testBlocksMultipleWallets(): void + { + $this->skipIfNotPostgresLockService(); + + /** @var User $user1 */ + /** @var User $user2 */ + /** @var User $user3 */ + [$user1, $user2, $user3] = UserFactory::times(3)->create(); + + $user1->deposit(1000); + $user2->deposit(2000); + $user3->deposit(3000); + + $lock = app(LockServiceInterface::class); + self::assertInstanceOf(PostgresLockService::class, $lock); + + $keys = [ + 'wallet_lock::'.$user1->wallet->uuid, + 'wallet_lock::'.$user2->wallet->uuid, + 'wallet_lock::'.$user3->wallet->uuid, + ]; + + foreach ($keys as $key) { + self::assertFalse($lock->isBlocked($key)); + } + + $result = $lock->blocks($keys, static fn () => 'test'); + self::assertSame('test', $result); + + foreach ($keys as $key) { + self::assertFalse($lock->isBlocked($key)); + } + } + + public function testBlockInTransaction(): void + { + $this->skipIfNotPostgresLockService(); + + /** @var User $user */ + $user = UserFactory::new()->create(); + $user->deposit(1000); + + $lock = app(LockServiceInterface::class); + $key = 'wallet_lock::'.$user->wallet->uuid; + + DB::beginTransaction(); + + $checkIsBlock = $lock->block($key, static fn () => $lock->isBlocked($key)); + self::assertTrue($checkIsBlock); + self::assertTrue($lock->isBlocked($key)); + + DB::commit(); + + // After commit, lockedKeys should still be set until releases() is called + self::assertTrue($lock->isBlocked($key)); + + $lock->releases([$key]); + self::assertFalse($lock->isBlocked($key)); + } + + public function testBlocksInTransaction(): void + { + $this->skipIfNotPostgresLockService(); + + /** @var User $user1 */ + /** @var User $user2 */ + [$user1, $user2] = UserFactory::times(2)->create(); + + $user1->deposit(1000); + $user2->deposit(2000); + + $lock = app(LockServiceInterface::class); + $keys = ['wallet_lock::'.$user1->wallet->uuid, 'wallet_lock::'.$user2->wallet->uuid]; + + DB::beginTransaction(); + + $checkIsBlock1 = $lock->blocks($keys, static fn () => $lock->isBlocked($keys[0])); + self::assertTrue($checkIsBlock1); + self::assertTrue($lock->isBlocked($keys[0])); + self::assertTrue($lock->isBlocked($keys[1])); + + DB::commit(); + + self::assertTrue($lock->isBlocked($keys[0])); + self::assertTrue($lock->isBlocked($keys[1])); + + $lock->releases($keys); + self::assertFalse($lock->isBlocked($keys[0])); + self::assertFalse($lock->isBlocked($keys[1])); + } + + public function testAutomaticLockingViaBookkeeperService(): void + { + $this->skipIfNotPostgresLockService(); + + /** @var User $user */ + $user = UserFactory::new()->create(); + $user->deposit(1000); + + // Clear cache to trigger automatic locking + $bookkeeper = app(BookkeeperServiceInterface::class); + $bookkeeper->forget($user->wallet); + + DB::beginTransaction(); + + // Accessing balance should trigger automatic locking + // BookkeeperService::multiAmount() calls lockService->blocks() with UUID (not wallet_lock::uuid) + $balance = $user->wallet->balanceInt; + self::assertSame(1000, $balance); + + $lock = app(LockServiceInterface::class); + // BookkeeperService uses UUID as key, not wallet_lock::uuid + $key = $user->wallet->uuid; + + // Lock should be set after accessing balance + self::assertTrue($lock->isBlocked($key)); + + DB::commit(); + + $lock->releases([$key]); + self::assertFalse($lock->isBlocked($key)); + } + + public function testReleases(): void + { + $this->skipIfNotPostgresLockService(); + + /** @var User $user1 */ + /** @var User $user2 */ + [$user1, $user2] = UserFactory::times(2)->create(); + + // Ensure wallets are created in database before transaction + $user1->deposit(0); + $user2->deposit(0); + + $lock = app(LockServiceInterface::class); + $keys = ['wallet_lock::'.$user1->wallet->uuid, 'wallet_lock::'.$user2->wallet->uuid]; + + DB::beginTransaction(); + + $lock->blocks($keys, static fn () => null); + + self::assertTrue($lock->isBlocked($keys[0])); + self::assertTrue($lock->isBlocked($keys[1])); + + DB::commit(); + + $lock->releases($keys); + + self::assertFalse($lock->isBlocked($keys[0])); + self::assertFalse($lock->isBlocked($keys[1])); + } + + public function testBlockedKeyPreventsDoubleLock(): void + { + $this->skipIfNotPostgresLockService(); + + /** @var User $user */ + $user = UserFactory::new()->create(); + + // Ensure wallet is created in database before transaction + $user->deposit(0); + + $lock = app(LockServiceInterface::class); + $key = 'wallet_lock::'.$user->wallet->uuid; + + DB::beginTransaction(); + + // First lock + $lock->block($key, static fn () => null); + self::assertTrue($lock->isBlocked($key)); + + // Second lock should not create new transaction, just execute callback + $result = $lock->block($key, static fn () => 'already locked'); + self::assertSame('already locked', $result); + self::assertTrue($lock->isBlocked($key)); + + DB::commit(); + + $lock->releases([$key]); + self::assertFalse($lock->isBlocked($key)); + } + + public function testCacheSyncAfterLock(): void + { + $this->skipIfNotPostgresLockService(); + + /** @var User $user */ + $user = UserFactory::new()->create(); + $user->deposit(1000); + + $lock = app(LockServiceInterface::class); + $key = 'wallet_lock::'.$user->wallet->uuid; + + // Lock should sync balance to cache + $lock->block($key, static fn () => null); + + // Balance should be accessible from cache + $balance = $user->wallet->balanceInt; + self::assertSame(1000, $balance); + } + + public function testMultipleWalletsCacheSync(): void + { + $this->skipIfNotPostgresLockService(); + + /** @var User $user1 */ + /** @var User $user2 */ + [$user1, $user2] = UserFactory::times(2)->create(); + + $user1->deposit(1000); + $user2->deposit(2000); + + $lock = app(LockServiceInterface::class); + $keys = ['wallet_lock::'.$user1->wallet->uuid, 'wallet_lock::'.$user2->wallet->uuid]; + + // Lock should sync all balances to cache + $lock->blocks($keys, static fn () => null); + + // Balances should be accessible from cache + self::assertSame(1000, $user1->wallet->balanceInt); + self::assertSame(2000, $user2->wallet->balanceInt); + } + + /** + * Skip test if PostgresLockService conditions are not met. + * Checks: database = pgsql AND lock.driver = database AND PostgresLockService is used. + */ + private function skipIfNotPostgresLockService(): void + { + // Check database driver + $dbDriver = config('database.default'); + $dbDriverActual = config('database.connections.'.$dbDriver.'.driver'); + + if ($dbDriver !== 'pgsql' || $dbDriverActual !== 'pgsql') { + $this->markTestSkipped( + 'PostgresLockService tests require PostgreSQL database (pgsql). Current: '.$dbDriver.'/'.$dbDriverActual + ); + } + + // Check lock driver + $lockDriver = config('wallet.lock.driver'); + if ($lockDriver !== 'database') { + $this->markTestSkipped( + 'PostgresLockService tests require wallet.lock.driver = database. Current: '.($lockDriver ?: 'empty') + ); + } + + // Verify that PostgresLockService is actually used + $lock = app(LockServiceInterface::class); + if (! ($lock instanceof PostgresLockService)) { + $this->markTestSkipped('PostgresLockService is not being used. LockService: '.get_class($lock)); + } + } +} From da28f1e2a2846b5fabeb3a56968c16040aabedfa Mon Sep 17 00:00:00 2001 From: Maxim Babichev Date: Sun, 7 Dec 2025 18:27:55 +0300 Subject: [PATCH 2/3] Refactor WalletServiceProvider and PostgresLockService for improved type safety and error handling - Enhanced WalletServiceProvider to ensure proper type handling for database connection and cache driver. - Updated PostgresLockService to improve error handling for database exceptions and optimized wallet locking queries. - Refactored test cases in PostgresLockServiceTest for better clarity and consistency in user creation. - Added type annotations for better code documentation and maintainability. --- src/Internal/Service/PostgresLockService.php | 43 +++++++++++++------ src/WalletServiceProvider.php | 13 ++++-- .../Units/Service/PostgresLockServiceTest.php | 37 +++++++++------- 3 files changed, 61 insertions(+), 32 deletions(-) diff --git a/src/Internal/Service/PostgresLockService.php b/src/Internal/Service/PostgresLockService.php index f50d28e7f..50f6415b7 100644 --- a/src/Internal/Service/PostgresLockService.php +++ b/src/Internal/Service/PostgresLockService.php @@ -7,7 +7,7 @@ use Bavix\Wallet\Internal\Exceptions\ExceptionInterface; use Bavix\Wallet\Internal\Exceptions\ModelNotFoundException; use Bavix\Wallet\Internal\Exceptions\TransactionFailedException; -use Bavix\Wallet\Models\Wallet; +use function config; use Illuminate\Contracts\Cache\Factory as CacheFactory; use Illuminate\Contracts\Cache\Repository as CacheRepository; use Illuminate\Database\QueryException; @@ -192,7 +192,7 @@ public function isBlocked(string $key): bool * * Optimized: single query for all wallets, single multiSync, single multiGet for verification. * - * @param string[] $uuids Array of normalized UUIDs (already normalized, no prefix) + * @param array $uuids Array of normalized UUIDs (already normalized, no prefix) */ private function lockWallets(array $uuids): void { @@ -203,21 +203,32 @@ private function lockWallets(array $uuids): void // CRITICAL: Read balance from DB with FOR UPDATE lock BEFORE syncing to state transaction // This ensures we always have the latest balance from database, not from external cache // OPTIMIZATION: Single query to lock all wallets at once - // SELECT * FROM wallets WHERE uuid IN (?, ?, ...) FOR UPDATE + // SELECT uuid, balance FROM wallets WHERE uuid IN (?, ?, ...) FOR UPDATE + $connection = $this->connectionService->get(); + $table = config('wallet.wallet.table', 'wallets'); + if (! is_string($table) || $table === '') { + throw new TransactionFailedException('Invalid wallet table name for locking'); + } + try { - $wallets = Wallet::query() + $wallets = $connection->table($table) + ->select(['uuid', 'balance']) ->whereIn('uuid', $uuids) ->lockForUpdate() ->get() ->keyBy('uuid'); } catch (QueryException $e) { - // PostgreSQL throws QueryException for invalid UUID format or other database errors - // Convert to ModelNotFoundException for consistency - throw new ModelNotFoundException( - 'Invalid wallet UUID or wallet not found: '.implode(', ', $uuids), - ExceptionInterface::MODEL_NOT_FOUND, - $e - ); + // Only map invalid UUID format to ModelNotFoundException, rethrow everything else + $sqlState = $e->errorInfo[0] ?? null; + if ($sqlState === '22P02') { + throw new ModelNotFoundException( + 'Invalid wallet UUID or wallet not found: '.implode(', ', $uuids), + ExceptionInterface::MODEL_NOT_FOUND, + $e + ); + } + + throw $e; } // Extract balances from locked wallets (fresh from DB, not from cache) @@ -226,15 +237,15 @@ private function lockWallets(array $uuids): void foreach ($uuids as $uuid) { $wallet = $wallets->get($uuid); if ($wallet !== null) { - // Wallet exists in DB - use balance from DB - $balances[$uuid] = $wallet->getOriginalBalanceAttribute(); + $balance = (string) ($wallet->balance ?? '0'); + assert($balance !== '', 'Balance should not be an empty string'); + $balances[$uuid] = $balance; } else { // Wallet doesn't exist in DB yet (lazy creation) - use balance 0 // This is normal for new wallets that haven't been saved yet $balances[$uuid] = '0'; } } - // Mark all UUIDs as locked (store only UUID, already normalized) foreach ($uuids as $uuid) { $this->lockedKeys->put(self::INNER_KEYS.$uuid, true, $this->seconds); @@ -265,6 +276,10 @@ private function lockWallets(array $uuids): void } } + /** + * @param list $keys + * @return list + */ private function sortKeys(array $keys): array { // Sort to prevent deadlock diff --git a/src/WalletServiceProvider.php b/src/WalletServiceProvider.php index cd36615e3..c4bdd0edd 100644 --- a/src/WalletServiceProvider.php +++ b/src/WalletServiceProvider.php @@ -578,7 +578,11 @@ private function resolveLockService(): string // Check if database driver is PostgreSQL $connectionName = config('wallet.database.connection', config('database.default')); - if (config('database.connections.'.$connectionName.'.driver') === 'pgsql') { + /** @var string $connectionName */ + $connectionName = is_string($connectionName) ? $connectionName : ''; + + $driver = config('database.connections.'.$connectionName.'.driver'); + if ($driver === 'pgsql') { return PostgresLockService::class; } @@ -602,7 +606,10 @@ private function resolveCacheDriver(): string return 'array'; } - // For all other cases - use configured cache driver - return config('wallet.cache.driver', 'array'); + // For all other cases - use configured cache driver (trust client config) + /** @var string $cacheDriver */ + $cacheDriver = config('wallet.cache.driver', 'array'); + + return $cacheDriver; } } diff --git a/tests/Units/Service/PostgresLockServiceTest.php b/tests/Units/Service/PostgresLockServiceTest.php index 06397181f..83ce4ddf8 100644 --- a/tests/Units/Service/PostgresLockServiceTest.php +++ b/tests/Units/Service/PostgresLockServiceTest.php @@ -40,10 +40,9 @@ public function testBlocksMultipleWallets(): void { $this->skipIfNotPostgresLockService(); - /** @var User $user1 */ - /** @var User $user2 */ - /** @var User $user3 */ - [$user1, $user2, $user3] = UserFactory::times(3)->create(); + $users = UserFactory::times(3)->create()->all(); + /** @var array{0: User, 1: User, 2: User} $users */ + [$user1, $user2, $user3] = $users; $user1->deposit(1000); $user2->deposit(2000); @@ -100,9 +99,9 @@ public function testBlocksInTransaction(): void { $this->skipIfNotPostgresLockService(); - /** @var User $user1 */ - /** @var User $user2 */ - [$user1, $user2] = UserFactory::times(2)->create(); + $users = UserFactory::times(2)->create()->all(); + /** @var array{0: User, 1: User} $users */ + [$user1, $user2] = $users; $user1->deposit(1000); $user2->deposit(2000); @@ -163,9 +162,9 @@ public function testReleases(): void { $this->skipIfNotPostgresLockService(); - /** @var User $user1 */ - /** @var User $user2 */ - [$user1, $user2] = UserFactory::times(2)->create(); + $users = UserFactory::times(2)->create()->all(); + /** @var array{0: User, 1: User} $users */ + [$user1, $user2] = $users; // Ensure wallets are created in database before transaction $user1->deposit(0); @@ -242,9 +241,9 @@ public function testMultipleWalletsCacheSync(): void { $this->skipIfNotPostgresLockService(); - /** @var User $user1 */ - /** @var User $user2 */ - [$user1, $user2] = UserFactory::times(2)->create(); + $users = UserFactory::times(2)->create()->all(); + /** @var array{0: User, 1: User} $users */ + [$user1, $user2] = $users; $user1->deposit(1000); $user2->deposit(2000); @@ -268,7 +267,14 @@ private function skipIfNotPostgresLockService(): void { // Check database driver $dbDriver = config('database.default'); + if (! is_string($dbDriver) || $dbDriver === '') { + $dbDriver = 'database'; + } + $dbDriverActual = config('database.connections.'.$dbDriver.'.driver'); + if (! is_string($dbDriverActual) || $dbDriverActual === '') { + $dbDriverActual = 'unknown'; + } if ($dbDriver !== 'pgsql' || $dbDriverActual !== 'pgsql') { $this->markTestSkipped( @@ -277,10 +283,11 @@ private function skipIfNotPostgresLockService(): void } // Check lock driver - $lockDriver = config('wallet.lock.driver'); + /** @var string $lockDriver */ + $lockDriver = config('wallet.lock.driver', ''); if ($lockDriver !== 'database') { $this->markTestSkipped( - 'PostgresLockService tests require wallet.lock.driver = database. Current: '.($lockDriver ?: 'empty') + 'PostgresLockService tests require wallet.lock.driver = database. Current: '.$lockDriver ); } From 2cb7d51810d98c8e3603bb48bae5ce9719569329 Mon Sep 17 00:00:00 2001 From: Maxim Babichev Date: Sun, 7 Dec 2025 19:10:58 +0300 Subject: [PATCH 3/3] Update GitHub Actions workflow to simplify coveralls check condition - Modified the coveralls check step in the PHP units workflow to always set the execute output to true, streamlining the process. --- .github/workflows/phpunits.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/phpunits.yaml b/.github/workflows/phpunits.yaml index a5661216d..b5a0d4da1 100644 --- a/.github/workflows/phpunits.yaml +++ b/.github/workflows/phpunits.yaml @@ -145,7 +145,8 @@ jobs: - name: Check coveralls id: coveralls-check - run: echo "execute=${{ matrix.php-versions == '8.3' && matrix.caches == 'array' && matrix.locks == 'redis' && matrix.databases == 'testing' }}" >> $GITHUB_OUTPUT + run: | + echo "execute=true" >> "$GITHUB_OUTPUT" - name: Prepare run test suite id: unit-prepare